[RFC/RFT PATCH 0/5] Clean up UFS(-qcom) MCQ situation

[RFC/RFT PATCH 0/5] Clean up UFS(-qcom) MCQ situation

[Konrad Dybcio]

The initial implementation was quite messy, including requesting
regions that do not really exist in hardware (or at least not in the
way they were described).

As we have no users (and the corresponding dt-bindings were never even
accepted), remove a whole lot of boilerplate code and clean up the
software's expectations.

Note that this revision does not fix the bindings defficiency yet.

Compile-tested only & not the best code I've written, but I'm looking
for feedback whether this approach is acceptable.

Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
Konrad Dybcio (5):
      ufs: ufs-qcom: Fix UFS base region name in MCQ case
      ufs: ufs-qcom: Remove inferred MCQ mappings
      ufs: ufs-qcom: Don't try to map inexistent regions
      ufs: ufs-qcom: Rename "mcq_sqd" to "mcq_opr"
      ufs: ufs-qcom: Kill ufshcd_res_info

 drivers/ufs/host/ufs-qcom.c      | 151 ++++++++++-----------------------------
 drivers/ufs/host/ufs-qcom.h      |   4 ++
 drivers/ufs/host/ufshcd-pltfrm.c |   4 +-
 include/ufs/ufshcd.h             |  26 +------
 4 files changed, 45 insertions(+), 140 deletions(-)
---
base-commit: 26ffb3d6f02cd0935fb9fa3db897767beee1cb2a
change-id: 20250704-topic-qcom_ufs_mcq_cleanup-3e7614ae06a8

Best regards,
-- 
Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

[PATCH RFC/RFT 1/5] ufs: ufs-qcom: Fix UFS base region name in MCQ case

[Konrad Dybcio]

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

There is no need to reinvent the wheel. There are no users yet, and the
dt-bindings were never updated to accommodate for this, so fix it while
we still easily can.

Fixes: c263b4ef737e ("scsi: ufs: core: mcq: Configure resource regions")
Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
 drivers/ufs/host/ufs-qcom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 318dca7fe3d735431e252e8a2a699ec1b7a36618..8dd9709cbdeef6ede5faa434fcb853e11950721f 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1899,7 +1899,7 @@ static void ufs_qcom_config_scaling_param(struct ufs_hba *hba,
 
 /* Resources */
 static const struct ufshcd_res_info ufs_res_info[RES_MAX] = {
-	{.name = "ufs_mem",},
+	{.name = "std",},
 	{.name = "mcq",},
 	/* Submission Queue DAO */
 	{.name = "mcq_sqd",},

-- 
2.50.0

[PATCH RFC/RFT 2/5] ufs: ufs-qcom: Remove inferred MCQ mappings

[Konrad Dybcio]

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

[PATCH RFC/RFT 3/5] ufs: ufs-qcom: Don't try to map inexistent regions

[Konrad Dybcio]

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

[CS]Q[DI] regions are intertwined within each op region (of which there
are many) and aren't actually separate register block.
Remove the confusing logic that suggests otherwise and simplify the
code a lot.

Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
 drivers/ufs/host/ufs-qcom.c | 107 ++++++++++++++++----------------------------
 drivers/ufs/host/ufs-qcom.h |   4 ++
 2 files changed, 43 insertions(+), 68 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 67929a3e6e6242a93ed4c84cb2d2f7f10de4aa5e..52dc0da042cb62a6c28b40e429773808299e102f 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1715,7 +1715,7 @@ static void ufs_qcom_dump_testbus(struct ufs_hba *hba)
 }
 
 static int ufs_qcom_dump_regs(struct ufs_hba *hba, size_t offset, size_t len,
-			      const char *prefix, enum ufshcd_res id)
+			      const char *prefix, void __iomem *base)
 {
 	u32 *regs __free(kfree) = NULL;
 	size_t pos;
@@ -1728,7 +1728,7 @@ static int ufs_qcom_dump_regs(struct ufs_hba *hba, size_t offset, size_t len,
 		return -ENOMEM;
 
 	for (pos = 0; pos < len; pos += 4)
-		regs[pos / 4] = readl(hba->res[id].base + offset + pos);
+		regs[pos / 4] = readl(base + offset + pos);
 
 	print_hex_dump(KERN_ERR, prefix,
 		       len > 4 ? DUMP_PREFIX_OFFSET : DUMP_PREFIX_NONE,
@@ -1739,30 +1739,31 @@ static int ufs_qcom_dump_regs(struct ufs_hba *hba, size_t offset, size_t len,
 
 static void ufs_qcom_dump_mcq_hci_regs(struct ufs_hba *hba)
 {
+	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
 	struct dump_info {
+		void __iomem *base;
 		size_t offset;
 		size_t len;
 		const char *prefix;
-		enum ufshcd_res id;
 	};
 
 	struct dump_info mcq_dumps[] = {
-		{0x0, 256 * 4, "MCQ HCI-0 ", RES_MCQ},
-		{0x400, 256 * 4, "MCQ HCI-1 ", RES_MCQ},
-		{0x0, 5 * 4, "MCQ VS-0 ", RES_MCQ_VS},
-		{0x0, 256 * 4, "MCQ SQD-0 ", RES_MCQ_SQD},
-		{0x400, 256 * 4, "MCQ SQD-1 ", RES_MCQ_SQD},
-		{0x800, 256 * 4, "MCQ SQD-2 ", RES_MCQ_SQD},
-		{0xc00, 256 * 4, "MCQ SQD-3 ", RES_MCQ_SQD},
-		{0x1000, 256 * 4, "MCQ SQD-4 ", RES_MCQ_SQD},
-		{0x1400, 256 * 4, "MCQ SQD-5 ", RES_MCQ_SQD},
-		{0x1800, 256 * 4, "MCQ SQD-6 ", RES_MCQ_SQD},
-		{0x1c00, 256 * 4, "MCQ SQD-7 ", RES_MCQ_SQD},
+		{hba->mcq_base, 0x0, 256 * 4, "MCQ HCI-0 "},
+		{hba->mcq_base, 0x400, 256 * 4, "MCQ HCI-1 "},
+		{host->mcq_vs_base, 0x0, 5 * 4, "MCQ VS-0 "},
+		{host->opr_start_base, 0x0, 256 * 4, "MCQ SQD-0 "},
+		{host->opr_start_base, 0x400, 256 * 4, "MCQ SQD-1 "},
+		{host->opr_start_base, 0x800, 256 * 4, "MCQ SQD-2 "},
+		{host->opr_start_base, 0xc00, 256 * 4, "MCQ SQD-3 "},
+		{host->opr_start_base, 0x1000, 256 * 4, "MCQ SQD-4 "},
+		{host->opr_start_base, 0x1400, 256 * 4, "MCQ SQD-5 "},
+		{host->opr_start_base, 0x1800, 256 * 4, "MCQ SQD-6 "},
+		{host->opr_start_base, 0x1c00, 256 * 4, "MCQ SQD-7 "},
 	};
 
 	for (int i = 0; i < ARRAY_SIZE(mcq_dumps); i++) {
 		ufs_qcom_dump_regs(hba, mcq_dumps[i].offset, mcq_dumps[i].len,
-				   mcq_dumps[i].prefix, mcq_dumps[i].id);
+				   mcq_dumps[i].prefix, mcq_dumps[i].base);
 		cond_resched();
 	}
 }
@@ -1891,74 +1892,44 @@ static void ufs_qcom_config_scaling_param(struct ufs_hba *hba,
 }
 #endif
 
-/* Resources */
-static const struct ufshcd_res_info ufs_res_info[RES_MAX] = {
-	{.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;
-	int i, ret;
+	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
+	struct resource *sqd_res;
 
-	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);
-			continue;
-		}
-
-		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;
-		}
-	}
-
-	res = &hba->res[RES_MCQ];
-	if (res->base)
+	hba->mcq_base = devm_platform_ioremap_resource_byname(pdev, "mcq");
+	if (!hba->mcq_base)
 		return -EINVAL;
 
-	hba->mcq_base = res->base;
+	sqd_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mcq_sqd");
+	if (!sqd_res)
+		return -EINVAL;
+
+	host->opr_start_base = devm_ioremap_resource(hba->dev, sqd_res);
+	if (!host->opr_start_base)
+		return -EINVAL;
+
+	host->opr_start_off = sqd_res->start - hba->hci_res->start;
+
+	host->mcq_vs_base = devm_platform_ioremap_resource_byname(pdev, "mcq_vs");
+	if (!host->mcq_vs_base)
+		return -EINVAL;
 
 	return 0;
 }
 
 static int ufs_qcom_op_runtime_config(struct ufs_hba *hba)
 {
-	struct ufshcd_res_info *sqdao_res;
+	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
 	struct ufshcd_mcq_opr_info_t *opr;
 	int i;
 
-	sqdao_res = &hba->res[RES_MCQ_SQD];
-	if (!sqdao_res->base)
-		return -EINVAL;
-
 	for (i = 0; i < OPR_MAX; i++) {
 		opr = &hba->mcq_opr[i];
-		opr->offset = sqdao_res->resource->start -
-			      hba->hci_res->start + 0x40 * i;
+		opr->offset = host->opr_start_off + 0x40 * i;
 		opr->stride = 0x100;
-		opr->base = sqdao_res->base + 0x40 * i;
+		opr->base = host->opr_start_base + 0x40 * i;
 	}
 
 	return 0;
@@ -1973,12 +1944,12 @@ 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];
+	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
 
-	if (!mcq_vs_res->base)
+	if (!host->mcq_vs_base)
 		return -EINVAL;
 
-	*ocqs = readl(mcq_vs_res->base + UFS_MEM_CQIS_VS);
+	*ocqs = readl(host->mcq_vs_base + UFS_MEM_CQIS_VS);
 
 	return 0;
 }
diff --git a/drivers/ufs/host/ufs-qcom.h b/drivers/ufs/host/ufs-qcom.h
index 0a5cfc2dd4f7d999dac9cbd671a078a65f877b68..7300e91a435607a2cef1a4f12a8c5c1201586783 100644
--- a/drivers/ufs/host/ufs-qcom.h
+++ b/drivers/ufs/host/ufs-qcom.h
@@ -281,6 +281,10 @@ struct ufs_qcom_host {
 	u32 phy_gear;
 
 	bool esi_enabled;
+
+	void __iomem *opr_start_base;
+	resource_size_t opr_start_off;
+	void __iomem *mcq_vs_base;
 };
 
 struct ufs_qcom_drvdata {

-- 
2.50.0

[PATCH RFC/RFT 4/5] ufs: ufs-qcom: Rename "mcq_sqd" to "mcq_opr"

[Konrad Dybcio]

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

SQD is a confusing name for the register block that hosts all the
opregions, each one of which contains SQD/CQD/SQI/CQI subregions.

Rename it since there are not even dt-bindings for this and therefore
no users

Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
 drivers/ufs/host/ufs-qcom.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 52dc0da042cb62a6c28b40e429773808299e102f..2953b86029cfa1e5fdaa75e2917cad79576947e2 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1896,21 +1896,21 @@ static int ufs_qcom_mcq_config_resource(struct ufs_hba *hba)
 {
 	struct platform_device *pdev = to_platform_device(hba->dev);
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
-	struct resource *sqd_res;
+	struct resource *opr_res;
 
 	hba->mcq_base = devm_platform_ioremap_resource_byname(pdev, "mcq");
 	if (!hba->mcq_base)
 		return -EINVAL;
 
-	sqd_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mcq_sqd");
-	if (!sqd_res)
+	opr_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mcq_opr");
+	if (!opr_res)
 		return -EINVAL;
 
-	host->opr_start_base = devm_ioremap_resource(hba->dev, sqd_res);
+	host->opr_start_base = devm_ioremap_resource(hba->dev, opr_res);
 	if (!host->opr_start_base)
 		return -EINVAL;
 
-	host->opr_start_off = sqd_res->start - hba->hci_res->start;
+	host->opr_start_off = opr_res->start - hba->hci_res->start;
 
 	host->mcq_vs_base = devm_platform_ioremap_resource_byname(pdev, "mcq_vs");
 	if (!host->mcq_vs_base)

-- 
2.50.0

[PATCH RFC/RFT 5/5] ufs: ufs-qcom: Kill ufshcd_res_info

[Konrad Dybcio]

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

This is not used by any driver and doesn't seem like it's going to be.
Remove it.

Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
---
 include/ufs/ufshcd.h | 24 ------------------------
 1 file changed, 24 deletions(-)

diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 28132ff759afbd3bf8977bc481da225d95fd461c..e99df617ac31e983d452f8983ea0a5498ed64962 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -794,29 +794,6 @@ struct ufs_hba_monitor {
 	bool enabled;
 };
 
-/**
- * struct ufshcd_res_info_t - MCQ related resource regions
- *
- * @name: resource name
- * @resource: pointer to resource region
- * @base: register base address
- */
-struct ufshcd_res_info {
-	const char *name;
-	struct resource *resource;
-	void __iomem *base;
-};
-
-enum ufshcd_res {
-	RES_MCQ,
-	RES_MCQ_SQD,
-	RES_MCQ_SQIS,
-	RES_MCQ_CQD,
-	RES_MCQ_CQIS,
-	RES_MCQ_VS,
-	RES_MAX,
-};
-
 /**
  * struct ufshcd_mcq_opr_info_t - Operation and Runtime registers
  *
@@ -1127,7 +1104,6 @@ struct ufs_hba {
 	bool lsdb_sup;
 	bool mcq_enabled;
 	bool mcq_esi_enabled;
-	struct ufshcd_res_info res[RES_MAX];
 	void __iomem *mcq_base;
 	struct ufs_hw_queue *uhq;
 	struct ufs_hw_queue *dev_cmd_queue;

-- 
2.50.0

Re: [PATCH RFC/RFT 1/5] ufs: ufs-qcom: Fix UFS base region name in MCQ case

[Bart Van Assche]

On 7/4/25 10:36 AM, Konrad Dybcio wrote:
> There is no need to reinvent the wheel. There are no users yet, and the
> dt-bindings were never updated to accommodate for this, so fix it while
> we still easily can.

The patch description should explain why this patch is considered a fix
and also why the kernel driver is modified instead of the device tree.

Thanks,

Bart.

Re: [PATCH RFC/RFT 2/5] ufs: ufs-qcom: Remove inferred MCQ mappings

[Bart Van Assche]

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.

Re: [PATCH RFC/RFT 5/5] ufs: ufs-qcom: Kill ufshcd_res_info

[Bart Van Assche]

On 7/4/25 10:36 AM, Konrad Dybcio wrote:
> This is not used by any driver and doesn't seem like it's going to be.
> Remove it.

The above description seems misleading to me. I think it should say that
previous patches from this series removed all users of what is removed
by this patch rather than suggesting that what has been removed never
had any users.

Bart.

Re: [PATCH RFC/RFT 1/5] ufs: ufs-qcom: Fix UFS base region name in MCQ case

[Manivannan Sadhasivam]

On Fri, Jul 04, 2025 at 07:36:09PM GMT, Konrad Dybcio wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 
> There is no need to reinvent the wheel. There are no users yet, and the
> dt-bindings were never updated to accommodate for this, so fix it while
> we still easily can.
> 

What are you fixing here? Please be explicit. "std" region is not at all in the
device memory map? Or it was present in some earlier ones and removed in the
final tape out version?

- Mani

> Fixes: c263b4ef737e ("scsi: ufs: core: mcq: Configure resource regions")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> ---
>  drivers/ufs/host/ufs-qcom.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 318dca7fe3d735431e252e8a2a699ec1b7a36618..8dd9709cbdeef6ede5faa434fcb853e11950721f 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -1899,7 +1899,7 @@ static void ufs_qcom_config_scaling_param(struct ufs_hba *hba,
>  
>  /* Resources */
>  static const struct ufshcd_res_info ufs_res_info[RES_MAX] = {
> -	{.name = "ufs_mem",},
> +	{.name = "std",},
>  	{.name = "mcq",},
>  	/* Submission Queue DAO */
>  	{.name = "mcq_sqd",},
> 
> -- 
> 2.50.0
> 

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

Re: [PATCH RFC/RFT 2/5] ufs: ufs-qcom: Remove inferred MCQ mappings

[Manivannan Sadhasivam]

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
> 

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

Re: [RFC/RFT PATCH 0/5] Clean up UFS(-qcom) MCQ situation

[Manivannan Sadhasivam]

On Fri, Jul 04, 2025 at 07:36:08PM GMT, Konrad Dybcio wrote:
> The initial implementation was quite messy, including requesting
> regions that do not really exist in hardware (or at least not in the
> way they were described).
> 
> As we have no users (and the corresponding dt-bindings were never even
> accepted), remove a whole lot of boilerplate code and clean up the
> software's expectations.
> 
> Note that this revision does not fix the bindings defficiency yet.
> 
> Compile-tested only & not the best code I've written, but I'm looking
> for feedback whether this approach is acceptable.
> 

I pushed for the bindings change within Qcom for a very long time, but it
didn't materialize. I don't think it makes sense to accept any series
targeting MCQ without bindings change (especially when it touches the memory
regions).

So please include the bindings change when you post the *real* series.

- Mani

> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> ---
> Konrad Dybcio (5):
>       ufs: ufs-qcom: Fix UFS base region name in MCQ case
>       ufs: ufs-qcom: Remove inferred MCQ mappings
>       ufs: ufs-qcom: Don't try to map inexistent regions
>       ufs: ufs-qcom: Rename "mcq_sqd" to "mcq_opr"
>       ufs: ufs-qcom: Kill ufshcd_res_info
> 
>  drivers/ufs/host/ufs-qcom.c      | 151 ++++++++++-----------------------------
>  drivers/ufs/host/ufs-qcom.h      |   4 ++
>  drivers/ufs/host/ufshcd-pltfrm.c |   4 +-
>  include/ufs/ufshcd.h             |  26 +------
>  4 files changed, 45 insertions(+), 140 deletions(-)
> ---
> base-commit: 26ffb3d6f02cd0935fb9fa3db897767beee1cb2a
> change-id: 20250704-topic-qcom_ufs_mcq_cleanup-3e7614ae06a8
> 
> Best regards,
> -- 
> Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> 

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

Re: [PATCH RFC/RFT 1/5] ufs: ufs-qcom: Fix UFS base region name in MCQ case

[Konrad Dybcio]

On 7/8/25 12:34 PM, Manivannan Sadhasivam wrote:
> On Fri, Jul 04, 2025 at 07:36:09PM GMT, Konrad Dybcio wrote:
>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>
>> There is no need to reinvent the wheel. There are no users yet, and the
>> dt-bindings were never updated to accommodate for this, so fix it while
>> we still easily can.
>>
> 
> What are you fixing here? Please be explicit. "std" region is not at all in the
> device memory map? Or it was present in some earlier ones and removed in the
> final tape out version?
> 
> - Mani

I simply failed to describe the issue.

As of today, the MCQ code refers to the region that our bindings call
"std" by the name "ufs_mem" (which this patch fixes).
Totally same thing in hardware, but it would obviously not work without
DT changes (which would be rightfully rejected as there is no reason to
change the name).

Konrad

Re: [PATCH RFC/RFT 2/5] ufs: ufs-qcom: Remove inferred MCQ mappings

[Konrad Dybcio]

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

Re: [PATCH RFC/RFT 2/5] ufs: ufs-qcom: Remove inferred MCQ mappings

[Konrad Dybcio]

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