From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

Stop acquiring the base HCI memory region twice. Instead, because of
the need of getting the offset of MCQ regions from the controller base,
get the resource for the main region and store it separately.

Demand all the regions are provided in DT and don't try to make
guesses, circumventing the memory map provided in FDT.

There are currently no platforms with MCQ enabled, so there is no
functional change.

Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
 drivers/ufs/host/ufs-qcom.c      | 58 ++++------------------------------------
 drivers/ufs/host/ufshcd-pltfrm.c |  4 ++-
 include/ufs/ufshcd.h             |  2 +-
 3 files changed, 9 insertions(+), 55 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 8dd9709cbdeef6ede5faa434fcb853e11950721f..67929a3e6e6242a93ed4c84cb2d2f7f10de4aa5e 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -28,12 +28,6 @@
 #include "ufshcd-pltfrm.h"
 #include "ufs-qcom.h"
 
-#define MCQ_QCFGPTR_MASK	GENMASK(7, 0)
-#define MCQ_QCFGPTR_UNIT	0x200
-#define MCQ_SQATTR_OFFSET(c) \
-	((((c) >> 16) & MCQ_QCFGPTR_MASK) * MCQ_QCFGPTR_UNIT)
-#define MCQ_QCFG_SIZE	0x40
-
 /* De-emphasis for gear-5 */
 #define DEEMPHASIS_3_5_dB	0x04
 #define NO_DEEMPHASIS		0x0
@@ -1899,7 +1893,6 @@ static void ufs_qcom_config_scaling_param(struct ufs_hba *hba,
 
 /* Resources */
 static const struct ufshcd_res_info ufs_res_info[RES_MAX] = {
-	{.name = "std",},
 	{.name = "mcq",},
 	/* Submission Queue DAO */
 	{.name = "mcq_sqd",},
@@ -1917,7 +1910,6 @@ 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));
@@ -1929,12 +1921,6 @@ static int ufs_qcom_mcq_config_resource(struct ufs_hba *hba)
 							     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;
 		}
 
@@ -1948,63 +1934,29 @@ static int ufs_qcom_mcq_config_resource(struct ufs_hba *hba)
 		}
 	}
 
-	/* MCQ resource provided in DT */
 	res = &hba->res[RES_MCQ];
-	/* Bail if MCQ resource is provided */
 	if (res->base)
-		goto out;
+		return -EINVAL;
 
-	/* 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;
-	}
-
-	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_res_info *sqdao_res;
 	struct ufshcd_mcq_opr_info_t *opr;
 	int i;
 
-	mem_res = &hba->res[RES_UFS];
 	sqdao_res = &hba->res[RES_MCQ_SQD];
-
-	if (!mem_res->base || !sqdao_res->base)
+	if (!sqdao_res->base)
 		return -EINVAL;
 
 	for (i = 0; i < OPR_MAX; i++) {
 		opr = &hba->mcq_opr[i];
 		opr->offset = sqdao_res->resource->start -
-			      mem_res->resource->start + 0x40 * i;
+			      hba->hci_res->start + 0x40 * i;
 		opr->stride = 0x100;
 		opr->base = sqdao_res->base + 0x40 * i;
 	}
diff --git a/drivers/ufs/host/ufshcd-pltfrm.c b/drivers/ufs/host/ufshcd-pltfrm.c
index ffe5d1d2b2158882d369e4d3c902633b81378dba..0ba13ab59eafe6e5c4f8db61691628a4905eb52f 100644
--- a/drivers/ufs/host/ufshcd-pltfrm.c
+++ b/drivers/ufs/host/ufshcd-pltfrm.c
@@ -463,8 +463,9 @@ int ufshcd_pltfrm_init(struct platform_device *pdev,
 	void __iomem *mmio_base;
 	int irq, err;
 	struct device *dev = &pdev->dev;
+	struct resource *hci_res;
 
-	mmio_base = devm_platform_ioremap_resource(pdev, 0);
+	mmio_base = devm_platform_get_and_ioremap_resource(pdev, 0, &hci_res);
 	if (IS_ERR(mmio_base))
 		return PTR_ERR(mmio_base);
 
@@ -479,6 +480,7 @@ int ufshcd_pltfrm_init(struct platform_device *pdev,
 	}
 
 	hba->vops = vops;
+	hba->hci_res = hci_res;
 
 	err = ufshcd_parse_clock_info(hba);
 	if (err) {
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 9b3515cee71178c96f42f757a01d975606f64c9e..28132ff759afbd3bf8977bc481da225d95fd461c 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -808,7 +808,6 @@ struct ufshcd_res_info {
 };
 
 enum ufshcd_res {
-	RES_UFS,
 	RES_MCQ,
 	RES_MCQ_SQD,
 	RES_MCQ_SQIS,
@@ -970,6 +969,7 @@ enum ufshcd_mcq_opr {
  */
 struct ufs_hba {
 	void __iomem *mmio_base;
+	struct resource *hci_res;
 
 	/* Virtual memory reference */
 	struct utp_transfer_cmd_desc *ucdl_base_addr;

-- 
2.50.0

On Fri, Jul 04, 2025 at 07:36:10PM GMT, Konrad Dybcio wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 
> Stop acquiring the base HCI memory region twice. Instead, because of
> the need of getting the offset of MCQ regions from the controller base,
> get the resource for the main region and store it separately.
> 
> Demand all the regions are provided in DT and don't try to make
> guesses, circumventing the memory map provided in FDT.
> 

IIRC, during the MCQ review, Can/Asutosh justified the manual resource parsing
due to some platforms just having a flat 'MCQ' region. So they ended up manually
allocating the rest of the regions based on hw capabilities.

So there is no such requirement to support those platforms now?

- Mani

> There are currently no platforms with MCQ enabled, so there is no
> functional change.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> ---
>  drivers/ufs/host/ufs-qcom.c      | 58 ++++------------------------------------
>  drivers/ufs/host/ufshcd-pltfrm.c |  4 ++-
>  include/ufs/ufshcd.h             |  2 +-
>  3 files changed, 9 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 8dd9709cbdeef6ede5faa434fcb853e11950721f..67929a3e6e6242a93ed4c84cb2d2f7f10de4aa5e 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -28,12 +28,6 @@
>  #include "ufshcd-pltfrm.h"
>  #include "ufs-qcom.h"
>  
> -#define MCQ_QCFGPTR_MASK	GENMASK(7, 0)
> -#define MCQ_QCFGPTR_UNIT	0x200
> -#define MCQ_SQATTR_OFFSET(c) \
> -	((((c) >> 16) & MCQ_QCFGPTR_MASK) * MCQ_QCFGPTR_UNIT)
> -#define MCQ_QCFG_SIZE	0x40
> -
>  /* De-emphasis for gear-5 */
>  #define DEEMPHASIS_3_5_dB	0x04
>  #define NO_DEEMPHASIS		0x0
> @@ -1899,7 +1893,6 @@ static void ufs_qcom_config_scaling_param(struct ufs_hba *hba,
>  
>  /* Resources */
>  static const struct ufshcd_res_info ufs_res_info[RES_MAX] = {
> -	{.name = "std",},
>  	{.name = "mcq",},
>  	/* Submission Queue DAO */
>  	{.name = "mcq_sqd",},
> @@ -1917,7 +1910,6 @@ 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));
> @@ -1929,12 +1921,6 @@ static int ufs_qcom_mcq_config_resource(struct ufs_hba *hba)
>  							     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;
>  		}
>  
> @@ -1948,63 +1934,29 @@ static int ufs_qcom_mcq_config_resource(struct ufs_hba *hba)
>  		}
>  	}
>  
> -	/* MCQ resource provided in DT */
>  	res = &hba->res[RES_MCQ];
> -	/* Bail if MCQ resource is provided */
>  	if (res->base)
> -		goto out;
> +		return -EINVAL;
>  
> -	/* 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;
> -	}
> -
> -	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_res_info *sqdao_res;
>  	struct ufshcd_mcq_opr_info_t *opr;
>  	int i;
>  
> -	mem_res = &hba->res[RES_UFS];
>  	sqdao_res = &hba->res[RES_MCQ_SQD];
> -
> -	if (!mem_res->base || !sqdao_res->base)
> +	if (!sqdao_res->base)
>  		return -EINVAL;
>  
>  	for (i = 0; i < OPR_MAX; i++) {
>  		opr = &hba->mcq_opr[i];
>  		opr->offset = sqdao_res->resource->start -
> -			      mem_res->resource->start + 0x40 * i;
> +			      hba->hci_res->start + 0x40 * i;
>  		opr->stride = 0x100;
>  		opr->base = sqdao_res->base + 0x40 * i;
>  	}
> diff --git a/drivers/ufs/host/ufshcd-pltfrm.c b/drivers/ufs/host/ufshcd-pltfrm.c
> index ffe5d1d2b2158882d369e4d3c902633b81378dba..0ba13ab59eafe6e5c4f8db61691628a4905eb52f 100644
> --- a/drivers/ufs/host/ufshcd-pltfrm.c
> +++ b/drivers/ufs/host/ufshcd-pltfrm.c
> @@ -463,8 +463,9 @@ int ufshcd_pltfrm_init(struct platform_device *pdev,
>  	void __iomem *mmio_base;
>  	int irq, err;
>  	struct device *dev = &pdev->dev;
> +	struct resource *hci_res;
>  
> -	mmio_base = devm_platform_ioremap_resource(pdev, 0);
> +	mmio_base = devm_platform_get_and_ioremap_resource(pdev, 0, &hci_res);
>  	if (IS_ERR(mmio_base))
>  		return PTR_ERR(mmio_base);
>  
> @@ -479,6 +480,7 @@ int ufshcd_pltfrm_init(struct platform_device *pdev,
>  	}
>  
>  	hba->vops = vops;
> +	hba->hci_res = hci_res;
>  
>  	err = ufshcd_parse_clock_info(hba);
>  	if (err) {
> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index 9b3515cee71178c96f42f757a01d975606f64c9e..28132ff759afbd3bf8977bc481da225d95fd461c 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -808,7 +808,6 @@ struct ufshcd_res_info {
>  };
>  
>  enum ufshcd_res {
> -	RES_UFS,
>  	RES_MCQ,
>  	RES_MCQ_SQD,
>  	RES_MCQ_SQIS,
> @@ -970,6 +969,7 @@ enum ufshcd_mcq_opr {
>   */
>  struct ufs_hba {
>  	void __iomem *mmio_base;
> +	struct resource *hci_res;
>  
>  	/* Virtual memory reference */
>  	struct utp_transfer_cmd_desc *ucdl_base_addr;
> 
> -- 
> 2.50.0
> 

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

On 7/8/25 1:13 PM, Manivannan Sadhasivam wrote:
> On Fri, Jul 04, 2025 at 07:36:10PM GMT, Konrad Dybcio wrote:
>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>
>> Stop acquiring the base HCI memory region twice. Instead, because of
>> the need of getting the offset of MCQ regions from the controller base,
>> get the resource for the main region and store it separately.
>>
>> Demand all the regions are provided in DT and don't try to make
>> guesses, circumventing the memory map provided in FDT.
>>
> 
> IIRC, during the MCQ review, Can/Asutosh justified the manual resource parsing
> due to some platforms just having a flat 'MCQ' region. So they ended up manually
> allocating the rest of the regions based on hw capabilities.
> 
> So there is no such requirement to support those platforms now?

I read the spec pdf some more and I think that this series is wrong
(but the current state of the code needs improvements too)

The "problem" is that *all* platforms have a "flat MCQ region", because
the UFSHCI specification mandates that. We have a block of:

----------------
| SUBMISSION_Q |
----------------
| COMPLETION_Q |
----------------

register subregions, repeated 32 times, each block being 0x40-long in
total. They're at a fixed offset from the UFSHC base, specified in
MCQCAP.QCFGPTR inside UFSHC (i.e. ufshcd_mcq_queue_cfg_addr()).

then, we have an equal amount of

---------------------
| SUBMISSION_Q_DAO  |
---------------------
| SUBMISSION_Q_ISAO |
---------------------
| COMPLETION_Q_DAO  |
---------------------
| COMPLETION_Q_ISAO |
---------------------

blocks (although these are allowed to be placed anywhere). For
reference, the QC UFS block looks like:

--------------------
| UFS_PHY (QMP)    |      - 0x4000
--------------------
| UFSHCD           |      + 0x0
--------------------
| UFS_ICE          |      + 0x4000
------------------------ # "MCQ" start
| SUBMISSION_Q     | x |  + 0x20000
-                  - 3 |
| COMPLETION_Q     | 2 |
------------------------
| VENDOR_SPECIFIC  |      + 0x20000 + 0x4000
-------------------------
| SUBMISSION_Q_DAO  |   | + 0x20000 + 0x5000
| SUBMISSION_Q_ISAO | x |
-                   - 3 |
| COMPLETION_Q_DAO  | 2 |
| COMPLETION_Q_ISAO |   |
------------------------ # "MCQ" end

(at least on SM8650)

So we can't just map the whole ufs-adjacent region like (I would
assume) the spec designers had in mind, as the ICE is expressed
as a separate device and we don't want to overlap-map regions.

We could maybe map the MCQ region in its entirety though and rely
on fixed offsets within it, promised they won't change with the
next platforms.  I would hope that's the case (so we could simply
mimic what happens in e.g. ufs-mediatek), but I'll ask around to
make sure.

Konrad

On 7/4/25 10:36 AM, Konrad Dybcio wrote:
> There are currently no platforms with MCQ enabled, so there is no
> functional change.

Hmm ... my understanding is that your employer provides multiple
development boards that support MCQ?

Thanks,

Bart.

On 7/7/25 7:51 PM, Bart Van Assche wrote:
> On 7/4/25 10:36 AM, Konrad Dybcio wrote:
>> There are currently no platforms with MCQ enabled, so there is no
>> functional change.
> 
> Hmm ... my understanding is that your employer provides multiple
> development boards that support MCQ?

The commit message refers to the state of the upstream kernel, where
none of the platforms we support today use MCQ (which I can tell as
it would require describing an additional register region in DT).

Konrad