* [PATCH V3 1/5] ufs: ufs-qcom: Streamline UFS MCQ resource mapping
2025-08-21 11:23 [PATCH V3 0/5] Enable UFS MCQ support for SM8650 and SM8750 Ram Kumar Dwivedi
@ 2025-08-21 11:23 ` Ram Kumar Dwivedi
2025-08-22 9:04 ` Manivannan Sadhasivam
2025-08-21 11:24 ` [PATCH V3 2/5] ufs: ufs-qcom: Refactor MCQ register dump logic Ram Kumar Dwivedi
` (3 subsequent siblings)
4 siblings, 1 reply; 21+ messages in thread
From: Ram Kumar Dwivedi @ 2025-08-21 11:23 UTC (permalink / raw)
To: andersson, konradybcio, robh, krzk+dt, conor+dt, mani,
James.Bottomley, martin.petersen
Cc: linux-arm-msm, devicetree, linux-kernel, linux-scsi
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH V3 1/5] ufs: ufs-qcom: Streamline UFS MCQ resource mapping
2025-08-21 11:23 ` [PATCH V3 1/5] ufs: ufs-qcom: Streamline UFS MCQ resource mapping Ram Kumar Dwivedi
@ 2025-08-22 9:04 ` Manivannan Sadhasivam
2025-09-03 7:27 ` Nitin Rawat
0 siblings, 1 reply; 21+ messages in thread
From: Manivannan Sadhasivam @ 2025-08-22 9:04 UTC (permalink / raw)
To: Ram Kumar Dwivedi
Cc: andersson, konradybcio, robh, krzk+dt, conor+dt, James.Bottomley,
martin.petersen, linux-arm-msm, devicetree, linux-kernel,
linux-scsi
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
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH V3 1/5] ufs: ufs-qcom: Streamline UFS MCQ resource mapping
2025-08-22 9:04 ` Manivannan Sadhasivam
@ 2025-09-03 7:27 ` Nitin Rawat
0 siblings, 0 replies; 21+ messages in thread
From: Nitin Rawat @ 2025-09-03 7:27 UTC (permalink / raw)
To: Manivannan Sadhasivam, Ram Kumar Dwivedi
Cc: andersson, konradybcio, robh, krzk+dt, conor+dt, James.Bottomley,
martin.petersen, linux-arm-msm, devicetree, linux-kernel,
linux-scsi
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH V3 2/5] ufs: ufs-qcom: Refactor MCQ register dump logic
2025-08-21 11:23 [PATCH V3 0/5] Enable UFS MCQ support for SM8650 and SM8750 Ram Kumar Dwivedi
2025-08-21 11:23 ` [PATCH V3 1/5] ufs: ufs-qcom: Streamline UFS MCQ resource mapping Ram Kumar Dwivedi
@ 2025-08-21 11:24 ` Ram Kumar Dwivedi
2025-08-22 9:08 ` Manivannan Sadhasivam
2025-08-21 11:24 ` [PATCH V3 3/5] scsi: ufs: core: Remove unused ufshcd_res_info structure Ram Kumar Dwivedi
` (2 subsequent siblings)
4 siblings, 1 reply; 21+ messages in thread
From: Ram Kumar Dwivedi @ 2025-08-21 11:24 UTC (permalink / raw)
To: andersson, konradybcio, robh, krzk+dt, conor+dt, mani,
James.Bottomley, martin.petersen
Cc: linux-arm-msm, devicetree, linux-kernel, linux-scsi
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH V3 2/5] ufs: ufs-qcom: Refactor MCQ register dump logic
2025-08-21 11:24 ` [PATCH V3 2/5] ufs: ufs-qcom: Refactor MCQ register dump logic Ram Kumar Dwivedi
@ 2025-08-22 9:08 ` Manivannan Sadhasivam
2025-08-22 17:49 ` Ram Kumar Dwivedi
0 siblings, 1 reply; 21+ messages in thread
From: Manivannan Sadhasivam @ 2025-08-22 9:08 UTC (permalink / raw)
To: Ram Kumar Dwivedi
Cc: andersson, konradybcio, robh, krzk+dt, conor+dt, James.Bottomley,
martin.petersen, linux-arm-msm, devicetree, linux-kernel,
linux-scsi
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
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V3 2/5] ufs: ufs-qcom: Refactor MCQ register dump logic
2025-08-22 9:08 ` Manivannan Sadhasivam
@ 2025-08-22 17:49 ` Ram Kumar Dwivedi
0 siblings, 0 replies; 21+ messages in thread
From: Ram Kumar Dwivedi @ 2025-08-22 17:49 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: andersson, konradybcio, robh, krzk+dt, conor+dt, James.Bottomley,
martin.petersen, linux-arm-msm, devicetree, linux-kernel,
linux-scsi
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
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH V3 3/5] scsi: ufs: core: Remove unused ufshcd_res_info structure
2025-08-21 11:23 [PATCH V3 0/5] Enable UFS MCQ support for SM8650 and SM8750 Ram Kumar Dwivedi
2025-08-21 11:23 ` [PATCH V3 1/5] ufs: ufs-qcom: Streamline UFS MCQ resource mapping Ram Kumar Dwivedi
2025-08-21 11:24 ` [PATCH V3 2/5] ufs: ufs-qcom: Refactor MCQ register dump logic Ram Kumar Dwivedi
@ 2025-08-21 11:24 ` Ram Kumar Dwivedi
2025-08-21 11:48 ` Krzysztof Kozlowski
2025-08-21 11:24 ` [PATCH V3 4/5] arm64: dts: qcom: sm8650: Enable MCQ support for UFS controller Ram Kumar Dwivedi
2025-08-21 11:24 ` [PATCH V3 5/5] arm64: dts: qcom: sm8750: " Ram Kumar Dwivedi
4 siblings, 1 reply; 21+ messages in thread
From: Ram Kumar Dwivedi @ 2025-08-21 11:24 UTC (permalink / raw)
To: andersson, konradybcio, robh, krzk+dt, conor+dt, mani,
James.Bottomley, martin.petersen
Cc: linux-arm-msm, devicetree, linux-kernel, linux-scsi
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH V3 3/5] scsi: ufs: core: Remove unused ufshcd_res_info structure
2025-08-21 11:24 ` [PATCH V3 3/5] scsi: ufs: core: Remove unused ufshcd_res_info structure Ram Kumar Dwivedi
@ 2025-08-21 11:48 ` Krzysztof Kozlowski
2025-09-01 16:08 ` Nitin Rawat
0 siblings, 1 reply; 21+ messages in thread
From: Krzysztof Kozlowski @ 2025-08-21 11:48 UTC (permalink / raw)
To: Ram Kumar Dwivedi, andersson, konradybcio, robh, krzk+dt,
conor+dt, mani, James.Bottomley, martin.petersen
Cc: linux-arm-msm, devicetree, linux-kernel, linux-scsi
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V3 3/5] scsi: ufs: core: Remove unused ufshcd_res_info structure
2025-08-21 11:48 ` Krzysztof Kozlowski
@ 2025-09-01 16:08 ` Nitin Rawat
2025-09-02 6:18 ` Krzysztof Kozlowski
2025-09-03 13:28 ` Manivannan Sadhasivam
0 siblings, 2 replies; 21+ messages in thread
From: Nitin Rawat @ 2025-09-01 16:08 UTC (permalink / raw)
To: Krzysztof Kozlowski, Ram Kumar Dwivedi, andersson, konradybcio,
robh, krzk+dt, conor+dt, mani, James.Bottomley, martin.petersen
Cc: linux-arm-msm, devicetree, linux-kernel, linux-scsi
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
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V3 3/5] scsi: ufs: core: Remove unused ufshcd_res_info structure
2025-09-01 16:08 ` Nitin Rawat
@ 2025-09-02 6:18 ` Krzysztof Kozlowski
2025-09-03 13:28 ` Manivannan Sadhasivam
1 sibling, 0 replies; 21+ messages in thread
From: Krzysztof Kozlowski @ 2025-09-02 6:18 UTC (permalink / raw)
To: Nitin Rawat, Ram Kumar Dwivedi, andersson, konradybcio, robh,
krzk+dt, conor+dt, mani, James.Bottomley, martin.petersen
Cc: linux-arm-msm, devicetree, linux-kernel, linux-scsi
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V3 3/5] scsi: ufs: core: Remove unused ufshcd_res_info structure
2025-09-01 16:08 ` Nitin Rawat
2025-09-02 6:18 ` Krzysztof Kozlowski
@ 2025-09-03 13:28 ` Manivannan Sadhasivam
2025-09-03 13:34 ` Nitin Rawat
1 sibling, 1 reply; 21+ messages in thread
From: Manivannan Sadhasivam @ 2025-09-03 13:28 UTC (permalink / raw)
To: Nitin Rawat
Cc: Krzysztof Kozlowski, Ram Kumar Dwivedi, andersson, konradybcio,
robh, krzk+dt, conor+dt, James.Bottomley, martin.petersen,
linux-arm-msm, devicetree, linux-kernel, linux-scsi
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
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V3 3/5] scsi: ufs: core: Remove unused ufshcd_res_info structure
2025-09-03 13:28 ` Manivannan Sadhasivam
@ 2025-09-03 13:34 ` Nitin Rawat
0 siblings, 0 replies; 21+ messages in thread
From: Nitin Rawat @ 2025-09-03 13:34 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: Krzysztof Kozlowski, Ram Kumar Dwivedi, andersson, konradybcio,
robh, krzk+dt, conor+dt, James.Bottomley, martin.petersen,
linux-arm-msm, devicetree, linux-kernel, linux-scsi
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
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH V3 4/5] arm64: dts: qcom: sm8650: Enable MCQ support for UFS controller
2025-08-21 11:23 [PATCH V3 0/5] Enable UFS MCQ support for SM8650 and SM8750 Ram Kumar Dwivedi
` (2 preceding siblings ...)
2025-08-21 11:24 ` [PATCH V3 3/5] scsi: ufs: core: Remove unused ufshcd_res_info structure Ram Kumar Dwivedi
@ 2025-08-21 11:24 ` Ram Kumar Dwivedi
2025-08-21 11:49 ` Krzysztof Kozlowski
2025-08-21 11:24 ` [PATCH V3 5/5] arm64: dts: qcom: sm8750: " Ram Kumar Dwivedi
4 siblings, 1 reply; 21+ messages in thread
From: Ram Kumar Dwivedi @ 2025-08-21 11:24 UTC (permalink / raw)
To: andersson, konradybcio, robh, krzk+dt, conor+dt, mani,
James.Bottomley, martin.petersen
Cc: linux-arm-msm, devicetree, linux-kernel, linux-scsi
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
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH V3 4/5] arm64: dts: qcom: sm8650: Enable MCQ support for UFS controller
2025-08-21 11:24 ` [PATCH V3 4/5] arm64: dts: qcom: sm8650: Enable MCQ support for UFS controller Ram Kumar Dwivedi
@ 2025-08-21 11:49 ` Krzysztof Kozlowski
2025-08-29 16:18 ` Manivannan Sadhasivam
0 siblings, 1 reply; 21+ messages in thread
From: Krzysztof Kozlowski @ 2025-08-21 11:49 UTC (permalink / raw)
To: Ram Kumar Dwivedi, andersson, konradybcio, robh, krzk+dt,
conor+dt, mani, James.Bottomley, martin.petersen
Cc: linux-arm-msm, devicetree, linux-kernel, linux-scsi
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V3 4/5] arm64: dts: qcom: sm8650: Enable MCQ support for UFS controller
2025-08-21 11:49 ` Krzysztof Kozlowski
@ 2025-08-29 16:18 ` Manivannan Sadhasivam
2025-08-30 8:43 ` Krzysztof Kozlowski
0 siblings, 1 reply; 21+ messages in thread
From: Manivannan Sadhasivam @ 2025-08-29 16:18 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Ram Kumar Dwivedi, andersson, konradybcio, robh, krzk+dt,
conor+dt, James.Bottomley, martin.petersen, linux-arm-msm,
devicetree, linux-kernel, linux-scsi
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
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V3 4/5] arm64: dts: qcom: sm8650: Enable MCQ support for UFS controller
2025-08-29 16:18 ` Manivannan Sadhasivam
@ 2025-08-30 8:43 ` Krzysztof Kozlowski
2025-09-01 4:15 ` Manivannan Sadhasivam
0 siblings, 1 reply; 21+ messages in thread
From: Krzysztof Kozlowski @ 2025-08-30 8:43 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: Ram Kumar Dwivedi, andersson, konradybcio, robh, krzk+dt,
conor+dt, James.Bottomley, martin.petersen, linux-arm-msm,
devicetree, linux-kernel, linux-scsi
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V3 4/5] arm64: dts: qcom: sm8650: Enable MCQ support for UFS controller
2025-08-30 8:43 ` Krzysztof Kozlowski
@ 2025-09-01 4:15 ` Manivannan Sadhasivam
2025-09-01 8:05 ` Krzysztof Kozlowski
0 siblings, 1 reply; 21+ messages in thread
From: Manivannan Sadhasivam @ 2025-09-01 4:15 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Ram Kumar Dwivedi, andersson, konradybcio, robh, krzk+dt,
conor+dt, James.Bottomley, martin.petersen, linux-arm-msm,
devicetree, linux-kernel, linux-scsi
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
--
மணிவண்ணன் சதாசிவம்
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V3 4/5] arm64: dts: qcom: sm8650: Enable MCQ support for UFS controller
2025-09-01 4:15 ` Manivannan Sadhasivam
@ 2025-09-01 8:05 ` Krzysztof Kozlowski
2025-09-01 14:50 ` Nitin Rawat
0 siblings, 1 reply; 21+ messages in thread
From: Krzysztof Kozlowski @ 2025-09-01 8:05 UTC (permalink / raw)
To: Manivannan Sadhasivam
Cc: Ram Kumar Dwivedi, andersson, konradybcio, robh, krzk+dt,
conor+dt, James.Bottomley, martin.petersen, linux-arm-msm,
devicetree, linux-kernel, linux-scsi
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V3 4/5] arm64: dts: qcom: sm8650: Enable MCQ support for UFS controller
2025-09-01 8:05 ` Krzysztof Kozlowski
@ 2025-09-01 14:50 ` Nitin Rawat
0 siblings, 0 replies; 21+ messages in thread
From: Nitin Rawat @ 2025-09-01 14:50 UTC (permalink / raw)
To: Krzysztof Kozlowski, Manivannan Sadhasivam
Cc: Ram Kumar Dwivedi, andersson, konradybcio, robh, krzk+dt,
conor+dt, James.Bottomley, martin.petersen, linux-arm-msm,
devicetree, linux-kernel, linux-scsi
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
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH V3 5/5] arm64: dts: qcom: sm8750: Enable MCQ support for UFS controller
2025-08-21 11:23 [PATCH V3 0/5] Enable UFS MCQ support for SM8650 and SM8750 Ram Kumar Dwivedi
` (3 preceding siblings ...)
2025-08-21 11:24 ` [PATCH V3 4/5] arm64: dts: qcom: sm8650: Enable MCQ support for UFS controller Ram Kumar Dwivedi
@ 2025-08-21 11:24 ` Ram Kumar Dwivedi
4 siblings, 0 replies; 21+ messages in thread
From: Ram Kumar Dwivedi @ 2025-08-21 11:24 UTC (permalink / raw)
To: andersson, konradybcio, robh, krzk+dt, conor+dt, mani,
James.Bottomley, martin.petersen
Cc: linux-arm-msm, devicetree, linux-kernel, linux-scsi
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
^ permalink raw reply related [flat|nested] 21+ messages in thread