From: Nitin Rawat <quic_nitirawa@quicinc.com>

The current MCQ resource configuration involves multiple resource
mappings and dynamic resource allocation.

Simplify the resource mapping by directly mapping the single "mcq"
resource from device tree to hba->mcq_base instead of mapping multiple
separate resources (RES_UFS, RES_MCQ, RES_MCQ_SQD, RES_MCQ_VS).

It also uses predefined offsets for MCQ doorbell registers (SQD,
CQD, SQIS, CQIS) relative to the MCQ base,providing clearer memory
layout clarity.

Additionally update vendor-specific register offset UFS_MEM_CQIS_VS
offset from 0x8 to 0x4008 to align with the hardware programming guide.

The new approach assumes the device tree provides a single "mcq"
resource that encompasses the entire MCQ configuration space, making
the driver more maintainable and less prone to resource mapping errors.

Co-developed-by: Ram Kumar Dwivedi <quic_rdwivedi@quicinc.com>
Signed-off-by: Ram Kumar Dwivedi <quic_rdwivedi@quicinc.com>
Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
---
 drivers/ufs/host/ufs-qcom.c | 146 +++++++++++++-----------------------
 drivers/ufs/host/ufs-qcom.h |  22 +++++-
 2 files changed, 73 insertions(+), 95 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 9574fdc2bb0f..6c6a385543ef 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1910,116 +1910,73 @@ static void ufs_qcom_config_scaling_param(struct ufs_hba *hba,
 	hba->clk_scaling.suspend_on_no_request = true;
 }
 
-/* Resources */
-static const struct ufshcd_res_info ufs_res_info[RES_MAX] = {
-	{.name = "ufs_mem",},
-	{.name = "mcq",},
-	/* Submission Queue DAO */
-	{.name = "mcq_sqd",},
-	/* Submission Queue Interrupt Status */
-	{.name = "mcq_sqis",},
-	/* Completion Queue DAO */
-	{.name = "mcq_cqd",},
-	/* Completion Queue Interrupt Status */
-	{.name = "mcq_cqis",},
-	/* MCQ vendor specific */
-	{.name = "mcq_vs",},
-};
-
 static int ufs_qcom_mcq_config_resource(struct ufs_hba *hba)
 {
 	struct platform_device *pdev = to_platform_device(hba->dev);
-	struct ufshcd_res_info *res;
-	struct resource *res_mem, *res_mcq;
-	int i, ret;
-
-	memcpy(hba->res, ufs_res_info, sizeof(ufs_res_info));
-
-	for (i = 0; i < RES_MAX; i++) {
-		res = &hba->res[i];
-		res->resource = platform_get_resource_byname(pdev,
-							     IORESOURCE_MEM,
-							     res->name);
-		if (!res->resource) {
-			dev_info(hba->dev, "Resource %s not provided\n", res->name);
-			if (i == RES_UFS)
-				return -ENODEV;
-			continue;
-		} else if (i == RES_UFS) {
-			res_mem = res->resource;
-			res->base = hba->mmio_base;
-			continue;
-		}
+	struct resource *res;
 
-		res->base = devm_ioremap_resource(hba->dev, res->resource);
-		if (IS_ERR(res->base)) {
-			dev_err(hba->dev, "Failed to map res %s, err=%d\n",
-					 res->name, (int)PTR_ERR(res->base));
-			ret = PTR_ERR(res->base);
-			res->base = NULL;
-			return ret;
-		}
+	/* Map the MCQ configuration region */
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mcq");
+	if (!res) {
+		dev_err(hba->dev, "MCQ resource not found in device tree\n");
+		return -ENODEV;
 	}
 
-	/* MCQ resource provided in DT */
-	res = &hba->res[RES_MCQ];
-	/* Bail if MCQ resource is provided */
-	if (res->base)
-		goto out;
-
-	/* Explicitly allocate MCQ resource from ufs_mem */
-	res_mcq = devm_kzalloc(hba->dev, sizeof(*res_mcq), GFP_KERNEL);
-	if (!res_mcq)
-		return -ENOMEM;
-
-	res_mcq->start = res_mem->start +
-			 MCQ_SQATTR_OFFSET(hba->mcq_capabilities);
-	res_mcq->end = res_mcq->start + hba->nr_hw_queues * MCQ_QCFG_SIZE - 1;
-	res_mcq->flags = res_mem->flags;
-	res_mcq->name = "mcq";
-
-	ret = insert_resource(&iomem_resource, res_mcq);
-	if (ret) {
-		dev_err(hba->dev, "Failed to insert MCQ resource, err=%d\n",
-			ret);
-		return ret;
+	hba->mcq_base = devm_ioremap_resource(hba->dev, res);
+	if (IS_ERR(hba->mcq_base)) {
+		dev_err(hba->dev, "Failed to map MCQ region: %ld\n",
+			PTR_ERR(hba->mcq_base));
+		return PTR_ERR(hba->mcq_base);
 	}
 
-	res->base = devm_ioremap_resource(hba->dev, res_mcq);
-	if (IS_ERR(res->base)) {
-		dev_err(hba->dev, "MCQ registers mapping failed, err=%d\n",
-			(int)PTR_ERR(res->base));
-		ret = PTR_ERR(res->base);
-		goto ioremap_err;
-	}
-
-out:
-	hba->mcq_base = res->base;
 	return 0;
-ioremap_err:
-	res->base = NULL;
-	remove_resource(res_mcq);
-	return ret;
 }
 
 static int ufs_qcom_op_runtime_config(struct ufs_hba *hba)
 {
-	struct ufshcd_res_info *mem_res, *sqdao_res;
 	struct ufshcd_mcq_opr_info_t *opr;
 	int i;
+	u32 doorbell_offsets[OPR_MAX];
 
-	mem_res = &hba->res[RES_UFS];
-	sqdao_res = &hba->res[RES_MCQ_SQD];
-
-	if (!mem_res->base || !sqdao_res->base)
+	if (!hba->mcq_base) {
+		dev_err(hba->dev, "MCQ base not mapped\n");
 		return -EINVAL;
+	}
+
+	/*
+	 * Configure doorbell address offsets in MCQ configuration registers.
+	 * These values are offsets relative to mmio_base (UFS_HCI_BASE).
+	 *
+	 * Memory Layout:
+	 * - mmio_base = UFS_HCI_BASE
+	 * - mcq_base  = MCQ_CONFIG_BASE = mmio_base + (UFS_QCOM_MCQCAP_QCFGPTR * 0x200)
+	 * - Doorbell registers are at: mmio_base + (UFS_QCOM_MCQCAP_QCFGPTR * 0x200) +
+	 * -				UFS_QCOM_MCQ_SQD_OFFSET
+	 * - Which is also: mcq_base +  UFS_QCOM_MCQ_SQD_OFFSET
+	 */
+
+	doorbell_offsets[OPR_SQD] = UFS_QCOM_SQD_ADDR_OFFSET;
+	doorbell_offsets[OPR_SQIS] = UFS_QCOM_SQIS_ADDR_OFFSET;
+	doorbell_offsets[OPR_CQD] = UFS_QCOM_CQD_ADDR_OFFSET;
+	doorbell_offsets[OPR_CQIS] = UFS_QCOM_CQIS_ADDR_OFFSET;
 
+	/*
+	 * Configure MCQ operation registers.
+	 *
+	 * The doorbell registers are physically located within the MCQ region:
+	 * - doorbell_physical_addr = mmio_base + doorbell_offset
+	 * - doorbell_physical_addr = mcq_base + (doorbell_offset - MCQ_CONFIG_OFFSET)
+	 */
 	for (i = 0; i < OPR_MAX; i++) {
 		opr = &hba->mcq_opr[i];
-		opr->offset = sqdao_res->resource->start -
-			      mem_res->resource->start + 0x40 * i;
-		opr->stride = 0x100;
-		opr->base = sqdao_res->base + 0x40 * i;
+		opr->offset = doorbell_offsets[i];  /* Offset relative to mmio_base */
+		opr->stride = UFS_QCOM_MCQ_STRIDE;  /* 256 bytes between queues */
+
+		/*
+		 * Calculate the actual doorbell base address within MCQ region:
+		 * base = mcq_base + (doorbell_offset - MCQ_CONFIG_OFFSET)
+		 */
+		opr->base = hba->mcq_base + (opr->offset - UFS_QCOM_MCQ_CONFIG_OFFSET);
 	}
 
 	return 0;
@@ -2034,12 +1991,13 @@ static int ufs_qcom_get_hba_mac(struct ufs_hba *hba)
 static int ufs_qcom_get_outstanding_cqs(struct ufs_hba *hba,
 					unsigned long *ocqs)
 {
-	struct ufshcd_res_info *mcq_vs_res = &hba->res[RES_MCQ_VS];
-
-	if (!mcq_vs_res->base)
+	if (!hba->mcq_base) {
+		dev_err(hba->dev, "MCQ base not mapped\n");
 		return -EINVAL;
+	}
 
-	*ocqs = readl(mcq_vs_res->base + UFS_MEM_CQIS_VS);
+	/* Read from MCQ vendor-specific register in MCQ region */
+	*ocqs = readl(hba->mcq_base + UFS_MEM_CQIS_VS);
 
 	return 0;
 }
diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index e0e129af7c16..8c2c94390a50 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -33,6 +33,25 @@
 #define DL_VS_CLK_CFG_MASK GENMASK(9, 0)
 #define DME_VS_CORE_CLK_CTRL_DME_HW_CGC_EN             BIT(9)
 
+/* Qualcomm MCQ Configuration */
+#define UFS_QCOM_MCQCAP_QCFGPTR     224  /* 0xE0 in hex */
+#define UFS_QCOM_MCQ_CONFIG_OFFSET  (UFS_QCOM_MCQCAP_QCFGPTR * 0x200)  /* 0x1C000 */
+
+/* Doorbell offsets within MCQ region (relative to MCQ_CONFIG_BASE) */
+#define UFS_QCOM_MCQ_SQD_OFFSET     0x5000
+#define UFS_QCOM_MCQ_CQD_OFFSET     0x5080
+#define UFS_QCOM_MCQ_SQIS_OFFSET    0x5040
+#define UFS_QCOM_MCQ_CQIS_OFFSET    0x50C0
+#define UFS_QCOM_MCQ_STRIDE         0x100
+
+/* Calculated doorbell address offsets (relative to mmio_base) */
+#define UFS_QCOM_SQD_ADDR_OFFSET    (UFS_QCOM_MCQ_CONFIG_OFFSET + UFS_QCOM_MCQ_SQD_OFFSET)
+#define UFS_QCOM_CQD_ADDR_OFFSET    (UFS_QCOM_MCQ_CONFIG_OFFSET + UFS_QCOM_MCQ_CQD_OFFSET)
+#define UFS_QCOM_SQIS_ADDR_OFFSET   (UFS_QCOM_MCQ_CONFIG_OFFSET + UFS_QCOM_MCQ_SQIS_OFFSET)
+#define UFS_QCOM_CQIS_ADDR_OFFSET   (UFS_QCOM_MCQ_CONFIG_OFFSET + UFS_QCOM_MCQ_CQIS_OFFSET)
+
+#define REG_UFS_MCQ_STRIDE          UFS_QCOM_MCQ_STRIDE
+
 /* QCOM UFS host controller vendor specific registers */
 enum {
 	REG_UFS_SYS1CLK_1US                 = 0xC0,
@@ -96,7 +115,8 @@ enum {
 };
 
 enum {
-	UFS_MEM_CQIS_VS		= 0x8,
+	UFS_MEM_VS_BASE         = 0x4000,
+	UFS_MEM_CQIS_VS		= 0x4008,
 };
 
 #define UFS_CNTLR_2_x_x_VEN_REGS_OFFSET(x)	(0x000 + x)
-- 
2.50.1

On Thu, Aug 21, 2025 at 04:53:59PM GMT, Ram Kumar Dwivedi wrote:
> From: Nitin Rawat <quic_nitirawa@quicinc.com>
> 
> The current MCQ resource configuration involves multiple resource
> mappings and dynamic resource allocation.
> 
> Simplify the resource mapping by directly mapping the single "mcq"
> resource from device tree to hba->mcq_base instead of mapping multiple
> separate resources (RES_UFS, RES_MCQ, RES_MCQ_SQD, RES_MCQ_VS).
> 
> It also uses predefined offsets for MCQ doorbell registers (SQD,
> CQD, SQIS, CQIS) relative to the MCQ base,providing clearer memory
> layout clarity.
> 
> Additionally update vendor-specific register offset UFS_MEM_CQIS_VS
> offset from 0x8 to 0x4008 to align with the hardware programming guide.
> 
> The new approach assumes the device tree provides a single "mcq"
> resource that encompasses the entire MCQ configuration space, making
> the driver more maintainable and less prone to resource mapping errors.
> 

Also make it clear that the binding only requires a single 'mcq' region and not
the separate ones as the driver is using. Otherwise, it sounds like a breakage.

> Co-developed-by: Ram Kumar Dwivedi <quic_rdwivedi@quicinc.com>
> Signed-off-by: Ram Kumar Dwivedi <quic_rdwivedi@quicinc.com>
> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>

Tag order is messed up. Please fix it.

> ---
>  drivers/ufs/host/ufs-qcom.c | 146 +++++++++++++-----------------------
>  drivers/ufs/host/ufs-qcom.h |  22 +++++-
>  2 files changed, 73 insertions(+), 95 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 9574fdc2bb0f..6c6a385543ef 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -1910,116 +1910,73 @@ static void ufs_qcom_config_scaling_param(struct ufs_hba *hba,
>  	hba->clk_scaling.suspend_on_no_request = true;
>  }
>  
> -/* Resources */
> -static const struct ufshcd_res_info ufs_res_info[RES_MAX] = {
> -	{.name = "ufs_mem",},
> -	{.name = "mcq",},
> -	/* Submission Queue DAO */
> -	{.name = "mcq_sqd",},
> -	/* Submission Queue Interrupt Status */
> -	{.name = "mcq_sqis",},
> -	/* Completion Queue DAO */
> -	{.name = "mcq_cqd",},
> -	/* Completion Queue Interrupt Status */
> -	{.name = "mcq_cqis",},
> -	/* MCQ vendor specific */
> -	{.name = "mcq_vs",},
> -};
> -
>  static int ufs_qcom_mcq_config_resource(struct ufs_hba *hba)
>  {
>  	struct platform_device *pdev = to_platform_device(hba->dev);
> -	struct ufshcd_res_info *res;
> -	struct resource *res_mem, *res_mcq;
> -	int i, ret;
> -
> -	memcpy(hba->res, ufs_res_info, sizeof(ufs_res_info));
> -
> -	for (i = 0; i < RES_MAX; i++) {
> -		res = &hba->res[i];
> -		res->resource = platform_get_resource_byname(pdev,
> -							     IORESOURCE_MEM,
> -							     res->name);
> -		if (!res->resource) {
> -			dev_info(hba->dev, "Resource %s not provided\n", res->name);
> -			if (i == RES_UFS)
> -				return -ENODEV;
> -			continue;
> -		} else if (i == RES_UFS) {
> -			res_mem = res->resource;
> -			res->base = hba->mmio_base;
> -			continue;
> -		}
> +	struct resource *res;
>  
> -		res->base = devm_ioremap_resource(hba->dev, res->resource);
> -		if (IS_ERR(res->base)) {
> -			dev_err(hba->dev, "Failed to map res %s, err=%d\n",
> -					 res->name, (int)PTR_ERR(res->base));
> -			ret = PTR_ERR(res->base);
> -			res->base = NULL;
> -			return ret;
> -		}
> +	/* Map the MCQ configuration region */
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mcq");
> +	if (!res) {
> +		dev_err(hba->dev, "MCQ resource not found in device tree\n");
> +		return -ENODEV;
>  	}
>  
> -	/* MCQ resource provided in DT */
> -	res = &hba->res[RES_MCQ];
> -	/* Bail if MCQ resource is provided */
> -	if (res->base)
> -		goto out;
> -
> -	/* Explicitly allocate MCQ resource from ufs_mem */
> -	res_mcq = devm_kzalloc(hba->dev, sizeof(*res_mcq), GFP_KERNEL);
> -	if (!res_mcq)
> -		return -ENOMEM;
> -
> -	res_mcq->start = res_mem->start +
> -			 MCQ_SQATTR_OFFSET(hba->mcq_capabilities);
> -	res_mcq->end = res_mcq->start + hba->nr_hw_queues * MCQ_QCFG_SIZE - 1;
> -	res_mcq->flags = res_mem->flags;
> -	res_mcq->name = "mcq";
> -
> -	ret = insert_resource(&iomem_resource, res_mcq);
> -	if (ret) {
> -		dev_err(hba->dev, "Failed to insert MCQ resource, err=%d\n",
> -			ret);
> -		return ret;
> +	hba->mcq_base = devm_ioremap_resource(hba->dev, res);
> +	if (IS_ERR(hba->mcq_base)) {
> +		dev_err(hba->dev, "Failed to map MCQ region: %ld\n",

Do you really need to print errnos of size 'long int'?

> +			PTR_ERR(hba->mcq_base));
> +		return PTR_ERR(hba->mcq_base);
>  	}
>  
> -	res->base = devm_ioremap_resource(hba->dev, res_mcq);
> -	if (IS_ERR(res->base)) {
> -		dev_err(hba->dev, "MCQ registers mapping failed, err=%d\n",
> -			(int)PTR_ERR(res->base));
> -		ret = PTR_ERR(res->base);
> -		goto ioremap_err;
> -	}
> -
> -out:
> -	hba->mcq_base = res->base;
>  	return 0;
> -ioremap_err:
> -	res->base = NULL;
> -	remove_resource(res_mcq);
> -	return ret;
>  }
>  
>  static int ufs_qcom_op_runtime_config(struct ufs_hba *hba)
>  {
> -	struct ufshcd_res_info *mem_res, *sqdao_res;
>  	struct ufshcd_mcq_opr_info_t *opr;
>  	int i;
> +	u32 doorbell_offsets[OPR_MAX];
>  
> -	mem_res = &hba->res[RES_UFS];
> -	sqdao_res = &hba->res[RES_MCQ_SQD];
> -
> -	if (!mem_res->base || !sqdao_res->base)
> +	if (!hba->mcq_base) {
> +		dev_err(hba->dev, "MCQ base not mapped\n");
>  		return -EINVAL;
> +	}

Is it possible to hit this error?

> +
> +	/*
> +	 * Configure doorbell address offsets in MCQ configuration registers.
> +	 * These values are offsets relative to mmio_base (UFS_HCI_BASE).
> +	 *
> +	 * Memory Layout:
> +	 * - mmio_base = UFS_HCI_BASE
> +	 * - mcq_base  = MCQ_CONFIG_BASE = mmio_base + (UFS_QCOM_MCQCAP_QCFGPTR * 0x200)
> +	 * - Doorbell registers are at: mmio_base + (UFS_QCOM_MCQCAP_QCFGPTR * 0x200) +
> +	 * -				UFS_QCOM_MCQ_SQD_OFFSET
> +	 * - Which is also: mcq_base +  UFS_QCOM_MCQ_SQD_OFFSET
> +	 */
> +
> +	doorbell_offsets[OPR_SQD] = UFS_QCOM_SQD_ADDR_OFFSET;
> +	doorbell_offsets[OPR_SQIS] = UFS_QCOM_SQIS_ADDR_OFFSET;
> +	doorbell_offsets[OPR_CQD] = UFS_QCOM_CQD_ADDR_OFFSET;
> +	doorbell_offsets[OPR_CQIS] = UFS_QCOM_CQIS_ADDR_OFFSET;
>  
> +	/*
> +	 * Configure MCQ operation registers.
> +	 *
> +	 * The doorbell registers are physically located within the MCQ region:
> +	 * - doorbell_physical_addr = mmio_base + doorbell_offset
> +	 * - doorbell_physical_addr = mcq_base + (doorbell_offset - MCQ_CONFIG_OFFSET)
> +	 */
>  	for (i = 0; i < OPR_MAX; i++) {
>  		opr = &hba->mcq_opr[i];
> -		opr->offset = sqdao_res->resource->start -
> -			      mem_res->resource->start + 0x40 * i;
> -		opr->stride = 0x100;
> -		opr->base = sqdao_res->base + 0x40 * i;
> +		opr->offset = doorbell_offsets[i];  /* Offset relative to mmio_base */
> +		opr->stride = UFS_QCOM_MCQ_STRIDE;  /* 256 bytes between queues */
> +
> +		/*
> +		 * Calculate the actual doorbell base address within MCQ region:
> +		 * base = mcq_base + (doorbell_offset - MCQ_CONFIG_OFFSET)
> +		 */
> +		opr->base = hba->mcq_base + (opr->offset - UFS_QCOM_MCQ_CONFIG_OFFSET);
>  	}
>  
>  	return 0;
> @@ -2034,12 +1991,13 @@ static int ufs_qcom_get_hba_mac(struct ufs_hba *hba)
>  static int ufs_qcom_get_outstanding_cqs(struct ufs_hba *hba,
>  					unsigned long *ocqs)
>  {
> -	struct ufshcd_res_info *mcq_vs_res = &hba->res[RES_MCQ_VS];
> -
> -	if (!mcq_vs_res->base)
> +	if (!hba->mcq_base) {
> +		dev_err(hba->dev, "MCQ base not mapped\n");
>  		return -EINVAL;
> +	}

Same here.

>  
> -	*ocqs = readl(mcq_vs_res->base + UFS_MEM_CQIS_VS);
> +	/* Read from MCQ vendor-specific register in MCQ region */
> +	*ocqs = readl(hba->mcq_base + UFS_MEM_CQIS_VS);
>  
>  	return 0;
>  }
> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
> index e0e129af7c16..8c2c94390a50 100644
> --- a/drivers/ufs/host/ufs-qcom.h
> +++ b/drivers/ufs/host/ufs-qcom.h
> @@ -33,6 +33,25 @@
>  #define DL_VS_CLK_CFG_MASK GENMASK(9, 0)
>  #define DME_VS_CORE_CLK_CTRL_DME_HW_CGC_EN             BIT(9)
>  
> +/* Qualcomm MCQ Configuration */
> +#define UFS_QCOM_MCQCAP_QCFGPTR     224  /* 0xE0 in hex */
> +#define UFS_QCOM_MCQ_CONFIG_OFFSET  (UFS_QCOM_MCQCAP_QCFGPTR * 0x200)  /* 0x1C000 */
> +
> +/* Doorbell offsets within MCQ region (relative to MCQ_CONFIG_BASE) */
> +#define UFS_QCOM_MCQ_SQD_OFFSET     0x5000
> +#define UFS_QCOM_MCQ_CQD_OFFSET     0x5080
> +#define UFS_QCOM_MCQ_SQIS_OFFSET    0x5040
> +#define UFS_QCOM_MCQ_CQIS_OFFSET    0x50C0
> +#define UFS_QCOM_MCQ_STRIDE         0x100
> +
> +/* Calculated doorbell address offsets (relative to mmio_base) */
> +#define UFS_QCOM_SQD_ADDR_OFFSET    (UFS_QCOM_MCQ_CONFIG_OFFSET + UFS_QCOM_MCQ_SQD_OFFSET)
> +#define UFS_QCOM_CQD_ADDR_OFFSET    (UFS_QCOM_MCQ_CONFIG_OFFSET + UFS_QCOM_MCQ_CQD_OFFSET)
> +#define UFS_QCOM_SQIS_ADDR_OFFSET   (UFS_QCOM_MCQ_CONFIG_OFFSET + UFS_QCOM_MCQ_SQIS_OFFSET)
> +#define UFS_QCOM_CQIS_ADDR_OFFSET   (UFS_QCOM_MCQ_CONFIG_OFFSET + UFS_QCOM_MCQ_CQIS_OFFSET)
> +
> +#define REG_UFS_MCQ_STRIDE          UFS_QCOM_MCQ_STRIDE
> +
>  /* QCOM UFS host controller vendor specific registers */
>  enum {
>  	REG_UFS_SYS1CLK_1US                 = 0xC0,
> @@ -96,7 +115,8 @@ enum {
>  };
>  
>  enum {
> -	UFS_MEM_CQIS_VS		= 0x8,
> +	UFS_MEM_VS_BASE         = 0x4000,
> +	UFS_MEM_CQIS_VS		= 0x4008,

Why are these offsets 'enum'? Can't they be fixed definitions like other
offsets?

- Mani
-- 
மணிவண்ணன் சதாசிவம்

On 8/22/2025 2:34 PM, Manivannan Sadhasivam wrote:
> On Thu, Aug 21, 2025 at 04:53:59PM GMT, Ram Kumar Dwivedi wrote:
>> From: Nitin Rawat <quic_nitirawa@quicinc.com>
>>
>> The current MCQ resource configuration involves multiple resource
>> mappings and dynamic resource allocation.
>>
>> Simplify the resource mapping by directly mapping the single "mcq"
>> resource from device tree to hba->mcq_base instead of mapping multiple
>> separate resources (RES_UFS, RES_MCQ, RES_MCQ_SQD, RES_MCQ_VS).
>>
>> It also uses predefined offsets for MCQ doorbell registers (SQD,
>> CQD, SQIS, CQIS) relative to the MCQ base,providing clearer memory
>> layout clarity.
>>
>> Additionally update vendor-specific register offset UFS_MEM_CQIS_VS
>> offset from 0x8 to 0x4008 to align with the hardware programming guide.
>>
>> The new approach assumes the device tree provides a single "mcq"
>> resource that encompasses the entire MCQ configuration space, making
>> the driver more maintainable and less prone to resource mapping errors.
>>
> 
> Also make it clear that the binding only requires a single 'mcq' region and not
> the separate ones as the driver is using. Otherwise, it sounds like a breakage.

Sure, I'll update this in commit text as part of next patch set.


> 
>> Co-developed-by: Ram Kumar Dwivedi <quic_rdwivedi@quicinc.com>
>> Signed-off-by: Ram Kumar Dwivedi <quic_rdwivedi@quicinc.com>
>> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
> 
> Tag order is messed up. Please fix it.

Sure, I'll address this in next patch set.


> 
>> ---
>>   drivers/ufs/host/ufs-qcom.c | 146 +++++++++++++-----------------------
>>   drivers/ufs/host/ufs-qcom.h |  22 +++++-
>>   2 files changed, 73 insertions(+), 95 deletions(-)
>>
>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>> index 9574fdc2bb0f..6c6a385543ef 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -1910,116 +1910,73 @@ static void ufs_qcom_config_scaling_param(struct ufs_hba *hba,
>>   	hba->clk_scaling.suspend_on_no_request = true;
>>   }
>>   
>> -/* Resources */
>> -static const struct ufshcd_res_info ufs_res_info[RES_MAX] = {
>> -	{.name = "ufs_mem",},
>> -	{.name = "mcq",},
>> -	/* Submission Queue DAO */
>> -	{.name = "mcq_sqd",},
>> -	/* Submission Queue Interrupt Status */
>> -	{.name = "mcq_sqis",},
>> -	/* Completion Queue DAO */
>> -	{.name = "mcq_cqd",},
>> -	/* Completion Queue Interrupt Status */
>> -	{.name = "mcq_cqis",},
>> -	/* MCQ vendor specific */
>> -	{.name = "mcq_vs",},
>> -};
>> -
>>   static int ufs_qcom_mcq_config_resource(struct ufs_hba *hba)
>>   {
>>   	struct platform_device *pdev = to_platform_device(hba->dev);
>> -	struct ufshcd_res_info *res;
>> -	struct resource *res_mem, *res_mcq;
>> -	int i, ret;
>> -
>> -	memcpy(hba->res, ufs_res_info, sizeof(ufs_res_info));
>> -
>> -	for (i = 0; i < RES_MAX; i++) {
>> -		res = &hba->res[i];
>> -		res->resource = platform_get_resource_byname(pdev,
>> -							     IORESOURCE_MEM,
>> -							     res->name);
>> -		if (!res->resource) {
>> -			dev_info(hba->dev, "Resource %s not provided\n", res->name);
>> -			if (i == RES_UFS)
>> -				return -ENODEV;
>> -			continue;
>> -		} else if (i == RES_UFS) {
>> -			res_mem = res->resource;
>> -			res->base = hba->mmio_base;
>> -			continue;
>> -		}
>> +	struct resource *res;
>>   
>> -		res->base = devm_ioremap_resource(hba->dev, res->resource);
>> -		if (IS_ERR(res->base)) {
>> -			dev_err(hba->dev, "Failed to map res %s, err=%d\n",
>> -					 res->name, (int)PTR_ERR(res->base));
>> -			ret = PTR_ERR(res->base);
>> -			res->base = NULL;
>> -			return ret;
>> -		}
>> +	/* Map the MCQ configuration region */
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mcq");
>> +	if (!res) {
>> +		dev_err(hba->dev, "MCQ resource not found in device tree\n");
>> +		return -ENODEV;
>>   	}
>>   
>> -	/* MCQ resource provided in DT */
>> -	res = &hba->res[RES_MCQ];
>> -	/* Bail if MCQ resource is provided */
>> -	if (res->base)
>> -		goto out;
>> -
>> -	/* Explicitly allocate MCQ resource from ufs_mem */
>> -	res_mcq = devm_kzalloc(hba->dev, sizeof(*res_mcq), GFP_KERNEL);
>> -	if (!res_mcq)
>> -		return -ENOMEM;
>> -
>> -	res_mcq->start = res_mem->start +
>> -			 MCQ_SQATTR_OFFSET(hba->mcq_capabilities);
>> -	res_mcq->end = res_mcq->start + hba->nr_hw_queues * MCQ_QCFG_SIZE - 1;
>> -	res_mcq->flags = res_mem->flags;
>> -	res_mcq->name = "mcq";
>> -
>> -	ret = insert_resource(&iomem_resource, res_mcq);
>> -	if (ret) {
>> -		dev_err(hba->dev, "Failed to insert MCQ resource, err=%d\n",
>> -			ret);
>> -		return ret;
>> +	hba->mcq_base = devm_ioremap_resource(hba->dev, res);
>> +	if (IS_ERR(hba->mcq_base)) {
>> +		dev_err(hba->dev, "Failed to map MCQ region: %ld\n",
> 
> Do you really need to print errnos of size 'long int'?



On Failure devm_ioremap_resource, returns an error pointer using ERR_PTR 
which is type long. Hence %ld is used.



> 
>> +			PTR_ERR(hba->mcq_base));
>> +		return PTR_ERR(hba->mcq_base);
>>   	}
>>   
>> -	res->base = devm_ioremap_resource(hba->dev, res_mcq);
>> -	if (IS_ERR(res->base)) {
>> -		dev_err(hba->dev, "MCQ registers mapping failed, err=%d\n",
>> -			(int)PTR_ERR(res->base));
>> -		ret = PTR_ERR(res->base);
>> -		goto ioremap_err;
>> -	}
>> -
>> -out:
>> -	hba->mcq_base = res->base;
>>   	return 0;
>> -ioremap_err:
>> -	res->base = NULL;
>> -	remove_resource(res_mcq);
>> -	return ret;
>>   }
>>   
>>   static int ufs_qcom_op_runtime_config(struct ufs_hba *hba)
>>   {
>> -	struct ufshcd_res_info *mem_res, *sqdao_res;
>>   	struct ufshcd_mcq_opr_info_t *opr;
>>   	int i;
>> +	u32 doorbell_offsets[OPR_MAX];
>>   
>> -	mem_res = &hba->res[RES_UFS];
>> -	sqdao_res = &hba->res[RES_MCQ_SQD];
>> -
>> -	if (!mem_res->base || !sqdao_res->base)
>> +	if (!hba->mcq_base) {
>> +		dev_err(hba->dev, "MCQ base not mapped\n");
>>   		return -EINVAL;
>> +	}
> 
> Is it possible to hit this error?

No as per current code. So i can remove this in next patch set.

> 
>> +
>> +	/*
>> +	 * Configure doorbell address offsets in MCQ configuration registers.
>> +	 * These values are offsets relative to mmio_base (UFS_HCI_BASE).
>> +	 *
>> +	 * Memory Layout:
>> +	 * - mmio_base = UFS_HCI_BASE
>> +	 * - mcq_base  = MCQ_CONFIG_BASE = mmio_base + (UFS_QCOM_MCQCAP_QCFGPTR * 0x200)
>> +	 * - Doorbell registers are at: mmio_base + (UFS_QCOM_MCQCAP_QCFGPTR * 0x200) +
>> +	 * -				UFS_QCOM_MCQ_SQD_OFFSET
>> +	 * - Which is also: mcq_base +  UFS_QCOM_MCQ_SQD_OFFSET
>> +	 */
>> +
>> +	doorbell_offsets[OPR_SQD] = UFS_QCOM_SQD_ADDR_OFFSET;
>> +	doorbell_offsets[OPR_SQIS] = UFS_QCOM_SQIS_ADDR_OFFSET;
>> +	doorbell_offsets[OPR_CQD] = UFS_QCOM_CQD_ADDR_OFFSET;
>> +	doorbell_offsets[OPR_CQIS] = UFS_QCOM_CQIS_ADDR_OFFSET;
>>   
>> +	/*
>> +	 * Configure MCQ operation registers.
>> +	 *
>> +	 * The doorbell registers are physically located within the MCQ region:
>> +	 * - doorbell_physical_addr = mmio_base + doorbell_offset
>> +	 * - doorbell_physical_addr = mcq_base + (doorbell_offset - MCQ_CONFIG_OFFSET)
>> +	 */
>>   	for (i = 0; i < OPR_MAX; i++) {
>>   		opr = &hba->mcq_opr[i];
>> -		opr->offset = sqdao_res->resource->start -
>> -			      mem_res->resource->start + 0x40 * i;
>> -		opr->stride = 0x100;
>> -		opr->base = sqdao_res->base + 0x40 * i;
>> +		opr->offset = doorbell_offsets[i];  /* Offset relative to mmio_base */
>> +		opr->stride = UFS_QCOM_MCQ_STRIDE;  /* 256 bytes between queues */
>> +
>> +		/*
>> +		 * Calculate the actual doorbell base address within MCQ region:
>> +		 * base = mcq_base + (doorbell_offset - MCQ_CONFIG_OFFSET)
>> +		 */
>> +		opr->base = hba->mcq_base + (opr->offset - UFS_QCOM_MCQ_CONFIG_OFFSET);
>>   	}
>>   
>>   	return 0;
>> @@ -2034,12 +1991,13 @@ static int ufs_qcom_get_hba_mac(struct ufs_hba *hba)
>>   static int ufs_qcom_get_outstanding_cqs(struct ufs_hba *hba,
>>   					unsigned long *ocqs)
>>   {
>> -	struct ufshcd_res_info *mcq_vs_res = &hba->res[RES_MCQ_VS];
>> -
>> -	if (!mcq_vs_res->base)
>> +	if (!hba->mcq_base) {
>> +		dev_err(hba->dev, "MCQ base not mapped\n");
>>   		return -EINVAL;
>> +	}
> 
> Same here.

Sure, I'll address this in next patch set.


> 
>>   
>> -	*ocqs = readl(mcq_vs_res->base + UFS_MEM_CQIS_VS);
>> +	/* Read from MCQ vendor-specific register in MCQ region */
>> +	*ocqs = readl(hba->mcq_base + UFS_MEM_CQIS_VS);
>>   
>>   	return 0;
>>   }
>> diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
>> index e0e129af7c16..8c2c94390a50 100644
>> --- a/drivers/ufs/host/ufs-qcom.h
>> +++ b/drivers/ufs/host/ufs-qcom.h
>> @@ -33,6 +33,25 @@
>>   #define DL_VS_CLK_CFG_MASK GENMASK(9, 0)
>>   #define DME_VS_CORE_CLK_CTRL_DME_HW_CGC_EN             BIT(9)
>>   
>> +/* Qualcomm MCQ Configuration */
>> +#define UFS_QCOM_MCQCAP_QCFGPTR     224  /* 0xE0 in hex */
>> +#define UFS_QCOM_MCQ_CONFIG_OFFSET  (UFS_QCOM_MCQCAP_QCFGPTR * 0x200)  /* 0x1C000 */
>> +
>> +/* Doorbell offsets within MCQ region (relative to MCQ_CONFIG_BASE) */
>> +#define UFS_QCOM_MCQ_SQD_OFFSET     0x5000
>> +#define UFS_QCOM_MCQ_CQD_OFFSET     0x5080
>> +#define UFS_QCOM_MCQ_SQIS_OFFSET    0x5040
>> +#define UFS_QCOM_MCQ_CQIS_OFFSET    0x50C0
>> +#define UFS_QCOM_MCQ_STRIDE         0x100
>> +
>> +/* Calculated doorbell address offsets (relative to mmio_base) */
>> +#define UFS_QCOM_SQD_ADDR_OFFSET    (UFS_QCOM_MCQ_CONFIG_OFFSET + UFS_QCOM_MCQ_SQD_OFFSET)
>> +#define UFS_QCOM_CQD_ADDR_OFFSET    (UFS_QCOM_MCQ_CONFIG_OFFSET + UFS_QCOM_MCQ_CQD_OFFSET)
>> +#define UFS_QCOM_SQIS_ADDR_OFFSET   (UFS_QCOM_MCQ_CONFIG_OFFSET + UFS_QCOM_MCQ_SQIS_OFFSET)
>> +#define UFS_QCOM_CQIS_ADDR_OFFSET   (UFS_QCOM_MCQ_CONFIG_OFFSET + UFS_QCOM_MCQ_CQIS_OFFSET)
>> +
>> +#define REG_UFS_MCQ_STRIDE          UFS_QCOM_MCQ_STRIDE
>> +
>>   /* QCOM UFS host controller vendor specific registers */
>>   enum {
>>   	REG_UFS_SYS1CLK_1US                 = 0xC0,
>> @@ -96,7 +115,8 @@ enum {
>>   };
>>   
>>   enum {
>> -	UFS_MEM_CQIS_VS		= 0x8,
>> +	UFS_MEM_VS_BASE         = 0x4000,
>> +	UFS_MEM_CQIS_VS		= 0x4008,
> 
> Why are these offsets 'enum'? Can't they be fixed definitions like other
> offsets?

Sure, I'll address this in next patch set.

Thanks,
Nitin

> 
> - Mani