| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <013a91f9-7a8e-4606-9f5a-5d675f78117c@quicinc.com>
Date: Wed, 3 Sep 2025 12:57:58 +0530
From: Nitin Rawat <quic_nitirawa@...cinc.com>
To: Manivannan Sadhasivam <mani@...nel.org>,
Ram Kumar Dwivedi
<quic_rdwivedi@...cinc.com>
CC: <andersson@...nel.org>, <konradybcio@...nel.org>, <robh@...nel.org>,
<krzk+dt@...nel.org>, <conor+dt@...nel.org>,
<James.Bottomley@...senpartnership.com>, <martin.petersen@...cle.com>,
<linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-scsi@...r.kernel.org>
Subject: Re: [PATCH V3 1/5] ufs: ufs-qcom: Streamline UFS MCQ resource mapping
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@...cinc.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@...cinc.com>
>> Signed-off-by: Ram Kumar Dwivedi <quic_rdwivedi@...cinc.com>
>> Signed-off-by: Nitin Rawat <quic_nitirawa@...cinc.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
Powered by blists - more mailing lists