[PATCH V3 0/5] Enable UFS MCQ support for SM8650 and SM8750

[PATCH V3 0/5] Enable UFS MCQ support for SM8650 and SM8750

[Ram Kumar Dwivedi]

This patch series enables Multi-Circular Queue (MCQ) support for the UFS
host controller on Qualcomm SM8650 and SM8750 platforms. MCQ improves
performance and scalability by allowing multiple hardware queues.

The series streamlines MCQ resource mapping by using a single MCQ region
mapping instead of multiple separate resource regions, making the driver
more maintainable and less prone to resource mapping errors.

Patch 1 streamlines UFS MCQ resource mapping with a single MCQ region
mapping, simplifying the current approach that involved multiple resource
mappings and dynamic resource allocation.

Patch 2 refactors MCQ register dump logic to align with the new resource
mapping approach, updating function signatures and using direct base
addresses.

Patch 3 removes the unused ufshcd_res_info structure and associated enum
definitions that are no longer needed after the resource mapping refactor.

Patches 4 and 5 update the device trees for SM8650 and SM8750 respectively
to enable MCQ by adding the necessary register mappings and MSI parent.

Tested on SM8650 and SM8750.

Changes from v2:
1. Removed dt-bindings patch as existing binding supports required
   reg-names format.
2. Added patch to refactor MCQ register dump logic for new resource
   mapping.
3. Added patch to remove unused ufshcd_res_info structure from UFS core.
4. Changed reg-names from "ufs_mem" to "std" in device tree patches.
5. Reordered patches with driver changes first, then device tree changes.
6. Updated SM8750 MCQ region size from 0x2000 to 0x15000

Nitin Rawat (3):
  ufs: ufs-qcom: Streamline UFS MCQ resource mapping
  ufs: ufs-qcom: Refactor MCQ register dump logic
  scsi: ufs: core: Remove unused ufshcd_res_info structure

Palash Kambar (1):
  arm64: dts: qcom: sm8750: Enable MCQ support for UFS controller

Ram Kumar Dwivedi (1):
  arm64: dts: qcom: sm8650: Enable MCQ support for UFS controller

 arch/arm64/boot/dts/qcom/sm8650.dtsi |   7 +-
 arch/arm64/boot/dts/qcom/sm8750.dtsi |   8 +-
 drivers/ufs/host/ufs-qcom.c          | 180 +++++++++++----------------
 drivers/ufs/host/ufs-qcom.h          |  22 +++-
 include/ufs/ufshcd.h                 |  25 ----
 5 files changed, 104 insertions(+), 138 deletions(-)

-- 
2.50.1

[PATCH V3 1/5] ufs: ufs-qcom: Streamline UFS MCQ resource mapping

[Ram Kumar Dwivedi]

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

[PATCH V3 2/5] ufs: ufs-qcom: Refactor MCQ register dump logic

[Ram Kumar Dwivedi]

From: Nitin Rawat <quic_nitirawa@quicinc.com>

Refactor MCQ register dump to align with the new resource mapping.
As part of refactor, below changes are done:

- Update ufs_qcom_dump_regs() function signature to accept direct
  base address instead of resource ID enum
- Modify ufs_qcom_dump_mcq_hci_regs() to use hba->mcq_base and
  calculated addresses from MCQ operation info
- Replace enum ufshcd_res with direct memory-mapped I/O addresses

Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
---
 drivers/ufs/host/ufs-qcom.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 6c6a385543ef..c1915f426ef8 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1742,7 +1742,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;
@@ -1755,7 +1755,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,
@@ -1766,30 +1766,34 @@ 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 ufshcd_mcq_opr_info_t *opr = &hba->mcq_opr[0];
+	void __iomem *mcq_vs_base = hba->mcq_base + UFS_MEM_VS_BASE;
+
 	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 "},
+		{mcq_vs_base, 0x0, 5 * 4, "MCQ VS-0 "},
+		{opr->base, 0x0, 256 * 4, "MCQ SQD-0 "},
+		{opr->base, 0x400, 256 * 4, "MCQ SQD-1 "},
+		{opr->base, 0x800, 256 * 4, "MCQ SQD-2 "},
+		{opr->base, 0xc00, 256 * 4, "MCQ SQD-3 "},
+		{opr->base, 0x1000, 256 * 4, "MCQ SQD-4 "},
+		{opr->base, 0x1400, 256 * 4, "MCQ SQD-5 "},
+		{opr->base, 0x1800, 256 * 4, "MCQ SQD-6 "},
+		{opr->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();
 	}
 }
-- 
2.50.1

[PATCH V3 3/5] scsi: ufs: core: Remove unused ufshcd_res_info structure

[Ram Kumar Dwivedi]

From: Nitin Rawat <quic_nitirawa@quicinc.com>

Remove the ufshcd_res_info structure and associated enum ufshcd_res
definitions from the UFS host controller header. These were previously
used for MCQ resource mapping but are no longer needed following recent
refactoring to use direct base addresses instead of multiple separate
resource regions

Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
---
 include/ufs/ufshcd.h | 25 -------------------------
 1 file changed, 25 deletions(-)

diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 1d3943777584..a7bcf7c7a1af 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -794,30 +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_UFS,
-	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 +1103,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.1

[PATCH V3 4/5] arm64: dts: qcom: sm8650: Enable MCQ support for UFS controller

[Ram Kumar Dwivedi]

Enable Multi-Circular Queue (MCQ) support for the UFS host controller
on the Qualcomm SM8650 platform by updating the device tree node. This
includes adding new register region for MCQ and specifying the MSI parent
required for MCQ operation.

Signed-off-by: Ram Kumar Dwivedi <quic_rdwivedi@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sm8650.dtsi | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
index d6794901f06b..18c4ebf3c1a6 100644
--- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
@@ -3950,7 +3950,10 @@ ufs_mem_phy: phy@1d80000 {
 
 		ufs_mem_hc: ufshc@1d84000 {
 			compatible = "qcom,sm8650-ufshc", "qcom,ufshc", "jedec,ufs-2.0";
-			reg = <0 0x01d84000 0 0x3000>;
+			reg = <0 0x01d84000 0 0x3000>,
+			      <0 0x1da0000 0 0x15000>;
+			reg-names = "std",
+				    "mcq";
 
 			interrupts = <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH 0>;
 
@@ -3988,6 +3991,8 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
 
 			iommus = <&apps_smmu 0x60 0>;
 
+			msi-parent = <&gic_its 0x60>;
+
 			lanes-per-direction = <2>;
 			qcom,ice = <&ice>;
 
-- 
2.50.1

[PATCH V3 5/5] arm64: dts: qcom: sm8750: Enable MCQ support for UFS controller

[Ram Kumar Dwivedi]

From: Palash Kambar <quic_pkambar@quicinc.com>

Enable Multi-Circular Queue (MCQ) support for the UFS host controller
on the Qualcomm SM8750 platform by updating the device tree node. This
includes adding new register region for MCQ and specifying the MSI parent
required for MCQ operation.

Signed-off-by: Palash Kambar <quic_pkambar@quicinc.com>
Signed-off-by: Ram Kumar Dwivedi <quic_rdwivedi@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sm8750.dtsi | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8750.dtsi b/arch/arm64/boot/dts/qcom/sm8750.dtsi
index 79ca262f5811..e55edc0a6e6e 100644
--- a/arch/arm64/boot/dts/qcom/sm8750.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8750.dtsi
@@ -3329,7 +3329,10 @@ ufs_mem_phy: phy@1d80000 {
 
 		ufs_mem_hc: ufs@1d84000 {
 			compatible = "qcom,sm8750-ufshc", "qcom,ufshc", "jedec,ufs-2.0";
-			reg = <0x0 0x01d84000 0x0 0x3000>;
+			reg = <0x0 0x01d84000 0x0 0x3000>,
+			      <0x0 0x1da0000  0x0 0x15000>;
+			reg-names = "std",
+				    "mcq";
 
 			interrupts = <GIC_SPI 265 IRQ_TYPE_LEVEL_HIGH>;
 
@@ -3363,11 +3366,12 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
 					     "cpu-ufs";
 
 			power-domains = <&gcc GCC_UFS_PHY_GDSC>;
+
 			required-opps = <&rpmhpd_opp_nom>;
 
 			iommus = <&apps_smmu 0x60 0>;
 			dma-coherent;
-
+			msi-parent = <&gic_its 0x60>;
 			lanes-per-direction = <2>;
 
 			phys = <&ufs_mem_phy>;
-- 
2.50.1

Re: [PATCH V3 3/5] scsi: ufs: core: Remove unused ufshcd_res_info structure

[Krzysztof Kozlowski]

On 21/08/2025 13:24, Ram Kumar Dwivedi wrote:
> From: Nitin Rawat <quic_nitirawa@quicinc.com>
> 
> Remove the ufshcd_res_info structure and associated enum ufshcd_res
> definitions from the UFS host controller header. These were previously
> used for MCQ resource mapping but are no longer needed following recent
> refactoring to use direct base addresses instead of multiple separate
> resource regions
> 
> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>

Incomplete SoB chain.

But anyway this makes no sense as independent patch. First you remove
users of it making it redundant... and then you remove it? No.

Organize your patches in logical chunks.

Best regards,
Krzysztof

Re: [PATCH V3 4/5] arm64: dts: qcom: sm8650: Enable MCQ support for UFS controller

[Krzysztof Kozlowski]

On 21/08/2025 13:24, Ram Kumar Dwivedi wrote:
> Enable Multi-Circular Queue (MCQ) support for the UFS host controller
> on the Qualcomm SM8650 platform by updating the device tree node. This
> includes adding new register region for MCQ and specifying the MSI parent
> required for MCQ operation.
> 
> Signed-off-by: Ram Kumar Dwivedi <quic_rdwivedi@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sm8650.dtsi | 7 ++++++-

I don't understand why you combine DTS patch into UFS patchset. This
creates impression of dependent work, which would be a trouble for merging.

Best regards,
Krzysztof

Re: [PATCH V3 1/5] ufs: ufs-qcom: Streamline UFS MCQ resource mapping

[Manivannan Sadhasivam]

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
-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH V3 2/5] ufs: ufs-qcom: Refactor MCQ register dump logic

[Manivannan Sadhasivam]

On Thu, Aug 21, 2025 at 04:54:00PM GMT, Ram Kumar Dwivedi wrote:
> From: Nitin Rawat <quic_nitirawa@quicinc.com>
> 
> Refactor MCQ register dump to align with the new resource mapping.
> As part of refactor, below changes are done:
> 
> - Update ufs_qcom_dump_regs() function signature to accept direct
>   base address instead of resource ID enum
> - Modify ufs_qcom_dump_mcq_hci_regs() to use hba->mcq_base and
>   calculated addresses from MCQ operation info
> - Replace enum ufshcd_res with direct memory-mapped I/O addresses
> 
> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>

Missing your s-o-b tag. Please spare some time to check these rudimentary rules
before submitting.

- Mani

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

Re: [PATCH V3 2/5] ufs: ufs-qcom: Refactor MCQ register dump logic

[Ram Kumar Dwivedi]

On 22-Aug-25 2:38 PM, Manivannan Sadhasivam wrote:
> On Thu, Aug 21, 2025 at 04:54:00PM GMT, Ram Kumar Dwivedi wrote:
>> From: Nitin Rawat <quic_nitirawa@quicinc.com>
>>
>> Refactor MCQ register dump to align with the new resource mapping.
>> As part of refactor, below changes are done:
>>
>> - Update ufs_qcom_dump_regs() function signature to accept direct
>>   base address instead of resource ID enum
>> - Modify ufs_qcom_dump_mcq_hci_regs() to use hba->mcq_base and
>>   calculated addresses from MCQ operation info
>> - Replace enum ufshcd_res with direct memory-mapped I/O addresses
>>
>> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
> 
> Missing your s-o-b tag. Please spare some time to check these rudimentary rules
> before submitting.
Hi Mani,

sure, I will take care of this going forward.

Thanks,
Ram.

> 
> - Mani
> 

Re: [PATCH V3 4/5] arm64: dts: qcom: sm8650: Enable MCQ support for UFS controller

[Manivannan Sadhasivam]

On Thu, Aug 21, 2025 at 01:49:36PM GMT, Krzysztof Kozlowski wrote:
> On 21/08/2025 13:24, Ram Kumar Dwivedi wrote:
> > Enable Multi-Circular Queue (MCQ) support for the UFS host controller
> > on the Qualcomm SM8650 platform by updating the device tree node. This
> > includes adding new register region for MCQ and specifying the MSI parent
> > required for MCQ operation.
> > 
> > Signed-off-by: Ram Kumar Dwivedi <quic_rdwivedi@quicinc.com>
> > ---
> >  arch/arm64/boot/dts/qcom/sm8650.dtsi | 7 ++++++-
> 
> I don't understand why you combine DTS patch into UFS patchset. This
> creates impression of dependent work, which would be a trouble for merging.
> 

What trouble? Even if the DTS depends on the driver/bindings change, can't it
still go through a different tree for the same cycle? It happened previously as
well, unless the rule changed now.

- Mani

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

Re: [PATCH V3 4/5] arm64: dts: qcom: sm8650: Enable MCQ support for UFS controller

[Krzysztof Kozlowski]

On 29/08/2025 18:18, Manivannan Sadhasivam wrote:
> On Thu, Aug 21, 2025 at 01:49:36PM GMT, Krzysztof Kozlowski wrote:
>> On 21/08/2025 13:24, Ram Kumar Dwivedi wrote:
>>> Enable Multi-Circular Queue (MCQ) support for the UFS host controller
>>> on the Qualcomm SM8650 platform by updating the device tree node. This
>>> includes adding new register region for MCQ and specifying the MSI parent
>>> required for MCQ operation.
>>>
>>> Signed-off-by: Ram Kumar Dwivedi <quic_rdwivedi@quicinc.com>
>>> ---
>>>  arch/arm64/boot/dts/qcom/sm8650.dtsi | 7 ++++++-
>>
>> I don't understand why you combine DTS patch into UFS patchset. This
>> creates impression of dependent work, which would be a trouble for merging.
>>
> 
> What trouble? Even if the DTS depends on the driver/bindings change, can't it
> still go through a different tree for the same cycle? It happened previously as

It all depends on sort of dependency.

> well, unless the rule changed now.

No, the point is that there is absolutely nothing relevant between the
DTS and drivers here. Combining unrelated patches, completely different
ones, targeting different subsystems into one patchset was always a
mistake. This makes only life of maintainers more difficult, for no gain.


Best regards,
Krzysztof

Re: [PATCH V3 4/5] arm64: dts: qcom: sm8650: Enable MCQ support for UFS controller

[Manivannan Sadhasivam]

On Sat, Aug 30, 2025 at 10:43:09AM GMT, Krzysztof Kozlowski wrote:
> On 29/08/2025 18:18, Manivannan Sadhasivam wrote:
> > On Thu, Aug 21, 2025 at 01:49:36PM GMT, Krzysztof Kozlowski wrote:
> >> On 21/08/2025 13:24, Ram Kumar Dwivedi wrote:
> >>> Enable Multi-Circular Queue (MCQ) support for the UFS host controller
> >>> on the Qualcomm SM8650 platform by updating the device tree node. This
> >>> includes adding new register region for MCQ and specifying the MSI parent
> >>> required for MCQ operation.
> >>>
> >>> Signed-off-by: Ram Kumar Dwivedi <quic_rdwivedi@quicinc.com>
> >>> ---
> >>>  arch/arm64/boot/dts/qcom/sm8650.dtsi | 7 ++++++-
> >>
> >> I don't understand why you combine DTS patch into UFS patchset. This
> >> creates impression of dependent work, which would be a trouble for merging.
> >>
> > 
> > What trouble? Even if the DTS depends on the driver/bindings change, can't it
> > still go through a different tree for the same cycle? It happened previously as
> 
> It all depends on sort of dependency.
> 
> > well, unless the rule changed now.
> 
> No, the point is that there is absolutely nothing relevant between the
> DTS and drivers here. Combining unrelated patches, completely different
> ones, targeting different subsystems into one patchset was always a
> mistake. This makes only life of maintainers more difficult, for no gain.
> 

Ok. Since patch 2 is just a refactoring, it should not be required for enabling
MCQ. But it is not clear if that is the case.

@Ram/Nitin: Please confirm if MCQ can be enabled without patch 2. If yes, then
post the DTS separately, otherwise, you need to rewrite the commit message of
patch 2 to state it explicitly.

- Mani

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

Re: [PATCH V3 4/5] arm64: dts: qcom: sm8650: Enable MCQ support for UFS controller

[Krzysztof Kozlowski]

On 01/09/2025 06:15, Manivannan Sadhasivam wrote:
>>>>
>>>> I don't understand why you combine DTS patch into UFS patchset. This
>>>> creates impression of dependent work, which would be a trouble for merging.
>>>>
>>>
>>> What trouble? Even if the DTS depends on the driver/bindings change, can't it
>>> still go through a different tree for the same cycle? It happened previously as
>>
>> It all depends on sort of dependency.
>>
>>> well, unless the rule changed now.
>>
>> No, the point is that there is absolutely nothing relevant between the
>> DTS and drivers here. Combining unrelated patches, completely different
>> ones, targeting different subsystems into one patchset was always a
>> mistake. This makes only life of maintainers more difficult, for no gain.
>>
> 
> Ok. Since patch 2 is just a refactoring, it should not be required for enabling
> MCQ. But it is not clear if that is the case.
> 
> @Ram/Nitin: Please confirm if MCQ can be enabled without patch 2. If yes, then
> post the DTS separately, otherwise, you need to rewrite the commit message of
> patch 2 to state it explicitly.

Dependency of DTS on driver would be another issue and in any case must
be clearly documented, not implicit via patch order.

Best regards,
Krzysztof

Re: [PATCH V3 4/5] arm64: dts: qcom: sm8650: Enable MCQ support for UFS controller

[Nitin Rawat]

On 9/1/2025 1:35 PM, Krzysztof Kozlowski wrote:
> On 01/09/2025 06:15, Manivannan Sadhasivam wrote:
>>>>>
>>>>> I don't understand why you combine DTS patch into UFS patchset. This
>>>>> creates impression of dependent work, which would be a trouble for merging.
>>>>>
>>>>
>>>> What trouble? Even if the DTS depends on the driver/bindings change, can't it
>>>> still go through a different tree for the same cycle? It happened previously as
>>>
>>> It all depends on sort of dependency.
>>>
>>>> well, unless the rule changed now.
>>>
>>> No, the point is that there is absolutely nothing relevant between the
>>> DTS and drivers here. Combining unrelated patches, completely different
>>> ones, targeting different subsystems into one patchset was always a
>>> mistake. This makes only life of maintainers more difficult, for no gain.
>>>
>>
>> Ok. Since patch 2 is just a refactoring, it should not be required for enabling
>> MCQ. But it is not clear if that is the case.
>>
>> @Ram/Nitin: Please confirm if MCQ can be enabled without patch 2. If yes, then
>> post the DTS separately, otherwise, you need to rewrite the commit message of
>> patch 2 to state it explicitly.
> 
> Dependency of DTS on driver would be another issue and in any case must
> be clearly documented, not implicit via patch order.

Hi Krzysztof/Mani,

We've verified that the driver and DTS function correctly when tested 
independently. We Will submit separate patches for the driver and DTS.

Regards,
Nitin




> 
> Best regards,
> Krzysztof
> 

Re: [PATCH V3 3/5] scsi: ufs: core: Remove unused ufshcd_res_info structure

[Nitin Rawat]

On 8/21/2025 5:18 PM, Krzysztof Kozlowski wrote:
> On 21/08/2025 13:24, Ram Kumar Dwivedi wrote:
>> From: Nitin Rawat <quic_nitirawa@quicinc.com>
>>
>> Remove the ufshcd_res_info structure and associated enum ufshcd_res
>> definitions from the UFS host controller header. These were previously
>> used for MCQ resource mapping but are no longer needed following recent
>> refactoring to use direct base addresses instead of multiple separate
>> resource regions
>>
>> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
> 
> Incomplete SoB chain.
> 
> But anyway this makes no sense as independent patch. First you remove
> users of it making it redundant... and then you remove it? No.

Hi Krzysztof,

The driver changes are in the UFS Qualcomm platform driver, which uses 
the definitions, while ufshcd.h is part of the UFS core driver. Hence 
kept in 2 separate patch.

Thanks,
Nitin

> 
> Organize your patches in logical chunks.
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH V3 3/5] scsi: ufs: core: Remove unused ufshcd_res_info structure

[Krzysztof Kozlowski]

On 01/09/2025 18:08, Nitin Rawat wrote:
> 
> 
> On 8/21/2025 5:18 PM, Krzysztof Kozlowski wrote:
>> On 21/08/2025 13:24, Ram Kumar Dwivedi wrote:
>>> From: Nitin Rawat <quic_nitirawa@quicinc.com>
>>>
>>> Remove the ufshcd_res_info structure and associated enum ufshcd_res
>>> definitions from the UFS host controller header. These were previously
>>> used for MCQ resource mapping but are no longer needed following recent
>>> refactoring to use direct base addresses instead of multiple separate
>>> resource regions
>>>
>>> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
>>
>> Incomplete SoB chain.
>>
>> But anyway this makes no sense as independent patch. First you remove
>> users of it making it redundant... and then you remove it? No.
> 
> Hi Krzysztof,
> 
> The driver changes are in the UFS Qualcomm platform driver, which uses 
> the definitions, while ufshcd.h is part of the UFS core driver. Hence 
> kept in 2 separate patch.
Don't explain the obvious but address the comment. I am going to repeat
since you just respond whatever:

This makes no sense as independent patch. First you remove
users of it making it redundant... and then you remove it? No.

Best regards,
Krzysztof

Re: [PATCH V3 1/5] ufs: ufs-qcom: Streamline UFS MCQ resource mapping

[Nitin Rawat]

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

Re: [PATCH V3 3/5] scsi: ufs: core: Remove unused ufshcd_res_info structure

[Manivannan Sadhasivam]

On Mon, Sep 01, 2025 at 09:38:25PM GMT, Nitin Rawat wrote:
> 
> 
> On 8/21/2025 5:18 PM, Krzysztof Kozlowski wrote:
> > On 21/08/2025 13:24, Ram Kumar Dwivedi wrote:
> > > From: Nitin Rawat <quic_nitirawa@quicinc.com>
> > > 
> > > Remove the ufshcd_res_info structure and associated enum ufshcd_res
> > > definitions from the UFS host controller header. These were previously
> > > used for MCQ resource mapping but are no longer needed following recent
> > > refactoring to use direct base addresses instead of multiple separate
> > > resource regions
> > > 
> > > Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
> > 
> > Incomplete SoB chain.
> > 
> > But anyway this makes no sense as independent patch. First you remove
> > users of it making it redundant... and then you remove it? No.
> 
> Hi Krzysztof,
> 
> The driver changes are in the UFS Qualcomm platform driver, which uses the
> definitions, while ufshcd.h is part of the UFS core driver. Hence kept in 2
> separate patch.
> 

No, that is not a logical split. When the users are removed, the unused
definitions also have to be removed even if the definitions are in a different
file.

So I believe you need to remove 'ufshcd_res_info' in patch 1 and 'ufshcd_res' in
patch 2.

- Mani

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

Re: [PATCH V3 3/5] scsi: ufs: core: Remove unused ufshcd_res_info structure

[Nitin Rawat]

On 9/3/2025 6:58 PM, Manivannan Sadhasivam wrote:
> On Mon, Sep 01, 2025 at 09:38:25PM GMT, Nitin Rawat wrote:
>>
>>
>> On 8/21/2025 5:18 PM, Krzysztof Kozlowski wrote:
>>> On 21/08/2025 13:24, Ram Kumar Dwivedi wrote:
>>>> From: Nitin Rawat <quic_nitirawa@quicinc.com>
>>>>
>>>> Remove the ufshcd_res_info structure and associated enum ufshcd_res
>>>> definitions from the UFS host controller header. These were previously
>>>> used for MCQ resource mapping but are no longer needed following recent
>>>> refactoring to use direct base addresses instead of multiple separate
>>>> resource regions
>>>>
>>>> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
>>>
>>> Incomplete SoB chain.
>>>
>>> But anyway this makes no sense as independent patch. First you remove
>>> users of it making it redundant... and then you remove it? No.
>>
>> Hi Krzysztof,
>>
>> The driver changes are in the UFS Qualcomm platform driver, which uses the
>> definitions, while ufshcd.h is part of the UFS core driver. Hence kept in 2
>> separate patch.
>>
> 
> No, that is not a logical split. When the users are removed, the unused
> definitions also have to be removed even if the definitions are in a different
> file.
> 
> So I believe you need to remove 'ufshcd_res_info' in patch 1 and 'ufshcd_res' in
> patch 2.

Agree with this. Hence I have taken care of this in v4.


> 
> - Mani
>