| Message ID | 271ed77a0ff46390c90fdcde71890d8cec83b8c9.1665017636.git.quic_asutoshd@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Multi Circular Queue Support | expand |
Hi Asutosh Das, >Define the mcq resources and add support to ioremap >the resource regions. > >Co-developed-by: Can Guo <quic_cang@quicinc.com> >Signed-off-by: Can Guo <quic_cang@quicinc.com> >Signed-off-by: Asutosh Das <quic_asutoshd@quicinc.com> >--- > drivers/ufs/core/ufs-mcq.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++ > include/ufs/ufshcd.h | 28 +++++++++++++ > 2 files changed, 127 insertions(+) > >diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c >index 659398d..7d0a37a 100644 >--- a/drivers/ufs/core/ufs-mcq.c >+++ b/drivers/ufs/core/ufs-mcq.c >@@ -18,6 +18,11 @@ > #define UFS_MCQ_NUM_DEV_CMD_QUEUES 1 > #define UFS_MCQ_MIN_POLL_QUEUES 0 > >+#define MCQ_QCFGPTR_MASK GENMASK(7, 0) >+#define MCQ_QCFGPTR_UNIT 0x200 >+#define MCQ_SQATTR_OFFSET(c) \ >+ ((((c) >> 16) & MCQ_QCFGPTR_MASK) * MCQ_QCFGPTR_UNIT) >+#define MCQ_QCFG_SIZE 0x40 > > static int rw_queue_count_set(const char *val, const struct kernel_param *kp) > { >@@ -67,6 +72,97 @@ module_param_cb(poll_queues, &poll_queue_count_ops, &poll_queues, 0644); > MODULE_PARM_DESC(poll_queues, > "Number of poll queues used for r/w. Default value is 1"); > >+/* 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 ufshcd_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 = 0; >+ >+ 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 -ENOMEM; >+ continue; >+ } else if (i == RES_UFS) { >+ res_mem = res->resource; >+ res->base = hba->mmio_base; >+ continue; >+ } >+ >+ res->base = devm_ioremap_resource(hba->dev, res->resource); >+ if (IS_ERR(res->base)) { >+ dev_err(hba->dev, "Failed to map res %s, err=%d\n", >+ res->name, (int)PTR_ERR(res->base)); >+ res->base = NULL; >+ ret = PTR_ERR(res->base); I think res->base has already NULL value. Thanks, Daejun
Hi Asutosh Das, >Define the mcq resources and add support to ioremap >the resource regions. > >Co-developed-by: Can Guo <quic_cang@quicinc.com> >Signed-off-by: Can Guo <quic_cang@quicinc.com> >Signed-off-by: Asutosh Das <quic_asutoshd@quicinc.com> >--- > drivers/ufs/core/ufs-mcq.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++ > include/ufs/ufshcd.h | 28 +++++++++++++ > 2 files changed, 127 insertions(+) > >diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c >index 659398d..7d0a37a 100644 >--- a/drivers/ufs/core/ufs-mcq.c >+++ b/drivers/ufs/core/ufs-mcq.c >@@ -18,6 +18,11 @@ > #define UFS_MCQ_NUM_DEV_CMD_QUEUES 1 > #define UFS_MCQ_MIN_POLL_QUEUES 0 > >+#define MCQ_QCFGPTR_MASK GENMASK(7, 0) >+#define MCQ_QCFGPTR_UNIT 0x200 >+#define MCQ_SQATTR_OFFSET(c) \ >+ ((((c) >> 16) & MCQ_QCFGPTR_MASK) * MCQ_QCFGPTR_UNIT) >+#define MCQ_QCFG_SIZE 0x40 > > static int rw_queue_count_set(const char *val, const struct kernel_param *kp) > { >@@ -67,6 +72,97 @@ module_param_cb(poll_queues, &poll_queue_count_ops, &poll_queues, 0644); > MODULE_PARM_DESC(poll_queues, > "Number of poll queues used for r/w. Default value is 1"); > >+/* 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 ufshcd_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 = 0; >+ >+ 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 -ENOMEM; >+ continue; >+ } else if (i == RES_UFS) { >+ res_mem = res->resource; >+ res->base = hba->mmio_base; >+ continue; >+ } >+ >+ res->base = devm_ioremap_resource(hba->dev, res->resource); >+ if (IS_ERR(res->base)) { >+ dev_err(hba->dev, "Failed to map res %s, err=%d\n", >+ res->name, (int)PTR_ERR(res->base)); >+ res->base = NULL; >+ ret = PTR_ERR(res->base); >+ return ret; >+ } >+ } >+ >+ /* MCQ resource provided in DT */ >+ res = &hba->res[RES_MCQ]; >+ /* Bail if NCQ resource is provided */ Maybe MCQ? >+ if (res->base) >+ goto out; >+ >+ /* Manually allocate MCQ resource from ufs_mem */ >+ res_mcq = res->resource; Why we assign the value to res_mcq? >+ res_mcq = devm_kzalloc(hba->dev, sizeof(*res_mcq), GFP_KERNEL); >+ if (!res_mcq) { >+ dev_err(hba->dev, "Failed to allocate MCQ resource\n"); >+ return ret; >+ } >+ >+ 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); Should we free the res_mcq? >+ return ret; >+ } >+ >+ res->base = devm_ioremap_resource(hba->dev, res_mcq); >+ if (IS_ERR(res->base)) { >+ dev_err(hba->dev, "MCQ registers mapping failed, err=%d\n", >+ (int)PTR_ERR(res->base)); Should we call remove_resource and free the res_mcq? Thanks, Daejun
Hi Eddie, On 10/14/2022 5:08 PM, Eddie Huang wrote: > Hi Asutosh, > > On Wed, 2022-10-05 at 18:06 -0700, Asutosh Das wrote: >> Define the mcq resources and add support to ioremap >> the resource regions. >> >> Co-developed-by: Can Guo <quic_cang@quicinc.com> >> Signed-off-by: Can Guo <quic_cang@quicinc.com> >> Signed-off-by: Asutosh Das <quic_asutoshd@quicinc.com> >> --- >> drivers/ufs/core/ufs-mcq.c | 99 >> ++++++++++++++++++++++++++++++++++++++++++++++ >> include/ufs/ufshcd.h | 28 +++++++++++++ >> 2 files changed, 127 insertions(+) >> >> diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c >> index 659398d..7d0a37a 100644 >> --- a/drivers/ufs/core/ufs-mcq.c >> +++ b/drivers/ufs/core/ufs-mcq.c >> @@ -18,6 +18,11 @@ >> #define UFS_MCQ_NUM_DEV_CMD_QUEUES 1 >> #define UFS_MCQ_MIN_POLL_QUEUES 0 >> >> +#define MCQ_QCFGPTR_MASK GENMASK(7, 0) >> +#define MCQ_QCFGPTR_UNIT 0x200 >> +#define MCQ_SQATTR_OFFSET(c) \ >> + ((((c) >> 16) & MCQ_QCFGPTR_MASK) * MCQ_QCFGPTR_UNIT) >> +#define MCQ_QCFG_SIZE 0x40 >> >> static int rw_queue_count_set(const char *val, const struct >> kernel_param *kp) >> { >> @@ -67,6 +72,97 @@ module_param_cb(poll_queues, >> &poll_queue_count_ops, &poll_queues, 0644); >> MODULE_PARM_DESC(poll_queues, >> "Number of poll queues used for r/w. Default value is >> 1"); >> >> +/* 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 ufshcd_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 = 0; >> + >> + 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 -ENOMEM; >> + continue; >> + } else if (i == RES_UFS) { >> + res_mem = res->resource; >> + res->base = hba->mmio_base; >> + continue; >> + } >> + >> + res->base = devm_ioremap_resource(hba->dev, res- >>> resource); >> + if (IS_ERR(res->base)) { >> + dev_err(hba->dev, "Failed to map res %s, >> err=%d\n", >> + res->name, (int)PTR_ERR(res- >>> base)); >> + res->base = NULL; >> + ret = PTR_ERR(res->base); >> + return ret; >> + } >> + } >> + >> + /* MCQ resource provided in DT */ >> + res = &hba->res[RES_MCQ]; >> + /* Bail if NCQ resource is provided */ >> + if (res->base) >> + goto out; >> + >> + /* Manually allocate MCQ resource from ufs_mem */ >> + res_mcq = res->resource; >> + res_mcq = devm_kzalloc(hba->dev, sizeof(*res_mcq), GFP_KERNEL); >> + if (!res_mcq) { >> + dev_err(hba->dev, "Failed to allocate MCQ resource\n"); >> + return ret; >> + } >> + >> + 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; >> + } >> + > Mediatek UFS hardware put MCQ SQ head/tail and SQ IS/IE together (SQ0 > head, SQ0 tail, SQ0 IS, SQ0 IE, CQ0 head, CQ0 tail....), which means > mcq_sqd register range interleave with mcq_sqis. I suggest let vendor > customize config mcq resource function to fit vendor's register > assignment In your case, which is similar to ours, you can just provide the res of SQD in DT, then use the ufshcd_mcq_vops_op_runtime_config() introduced in Patch #8 to configure each operation and runtime pointers like we are doing in ufs_qcom_op_runtime_config(). Please let us know if it is not enough for your case. Regards, Can Guo. > > Regards, > Eddie Huang > >
Hi Can, > Subject: Re: [PATCH v2 06/17] ufs: core: mcq: Configure resource > regions > Date: Fri, 14 Oct 2022 17:31:52 +0800 > From: Can Guo <quic_cang@quicinc.com> > To: Eddie Huang <eddie.huang@medaitek.com>, Asutosh Das < > quic_asutoshd@quicinc.com>, quic_nitirawa@quicinc.com, > quic_rampraka@quicinc.com, quic_bhaskarv@quicinc.com, > quic_richardp@quicinc.com, linux-scsi@vger.kernel.org > CC: linux-arm-msm@vger.kernel.org, quic_nguyenb@quicinc.com, > quic_xiaosenh@quicinc.com, bvanassche@acm.org, avri.altman@wdc.com, > mani@kernel.org, beanhuo@micron.com > > > Hi Eddie, > > On 10/14/2022 5:08 PM, Eddie Huang wrote: > > Hi Asutosh, > > > > On Wed, 2022-10-05 at 18:06 -0700, Asutosh Das wrote: > > > Define the mcq resources and add support to ioremap > > > the resource regions. > > > > > > Co-developed-by: Can Guo <quic_cang@quicinc.com> > > > Signed-off-by: Can Guo <quic_cang@quicinc.com> > > > Signed-off-by: Asutosh Das <quic_asutoshd@quicinc.com> > > > --- > > > drivers/ufs/core/ufs-mcq.c | 99 > > > ++++++++++++++++++++++++++++++++++++++++++++++ > > > include/ufs/ufshcd.h | 28 +++++++++++++ > > > 2 files changed, 127 insertions(+) > > > > > > diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs- > > > mcq.c > > > index 659398d..7d0a37a 100644 > > > --- a/drivers/ufs/core/ufs-mcq.c > > > +++ b/drivers/ufs/core/ufs-mcq.c > > > @@ -18,6 +18,11 @@ > > > #define UFS_MCQ_NUM_DEV_CMD_QUEUES 1 > > > #define UFS_MCQ_MIN_POLL_QUEUES 0 > > > +#define MCQ_QCFGPTR_MASK GENMASK(7, 0) > > > +#define MCQ_QCFGPTR_UNIT 0x200 > > > +#define MCQ_SQATTR_OFFSET(c) \ > > > + ((((c) >> 16) & MCQ_QCFGPTR_MASK) * MCQ_QCFGPTR_UNIT) > > > +#define MCQ_QCFG_SIZE 0x40 > > > static int rw_queue_count_set(const char *val, const struct > > > kernel_param *kp) > > > { > > > @@ -67,6 +72,97 @@ module_param_cb(poll_queues, > > > &poll_queue_count_ops, &poll_queues, 0644); > > > MODULE_PARM_DESC(poll_queues, > > > "Number of poll queues used for r/w. Default value is > > > 1"); > > > +/* 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 ufshcd_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 = 0; > > > + > > > + 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 -ENOMEM; > > > + continue; > > > + } else if (i == RES_UFS) { > > > + res_mem = res->resource; > > > + res->base = hba->mmio_base; > > > + continue; > > > + } > > > + > > > + res->base = devm_ioremap_resource(hba->dev, res- > > > > resource); > > > + if (IS_ERR(res->base)) { > > > + dev_err(hba->dev, "Failed to map res %s, > > > err=%d\n", > > > + res->name, (int)PTR_ERR(res- > > > > base)); > > > + res->base = NULL; > > > + ret = PTR_ERR(res->base); > > > + return ret; > > > + } > > > + } > > > + > > > + /* MCQ resource provided in DT */ > > > + res = &hba->res[RES_MCQ]; > > > + /* Bail if NCQ resource is provided */ > > > + if (res->base) > > > + goto out; > > > + > > > + /* Manually allocate MCQ resource from ufs_mem */ > > > + res_mcq = res->resource; > > > + res_mcq = devm_kzalloc(hba->dev, sizeof(*res_mcq), GFP_KERNEL); > > > + if (!res_mcq) { > > > + dev_err(hba->dev, "Failed to allocate MCQ resource\n"); > > > + return ret; > > > + } > > > + > > > + 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; > > > + } > > > + > > Mediatek UFS hardware put MCQ SQ head/tail and SQ IS/IE together > > (SQ0 > > head, SQ0 tail, SQ0 IS, SQ0 IE, CQ0 head, CQ0 tail....), which > > means > > mcq_sqd register range interleave with mcq_sqis. I suggest let > > vendor > > customize config mcq resource function to fit vendor's register > > assignment > > In your case, which is similar to ours, you can just provide the res > of SQD in DT, then use the > > ufshcd_mcq_vops_op_runtime_config() introduced in Patch #8 to > configure each operation > Let me explain more detail about Mediatek UFS register assignment: a. 0x0 ~ 0x5FF: UFS common register b. 0x1600 ~ : MCQ register c. 0x2200: UFS common vendor specific register d. 0x2800: SQ/CQ head/tail pointer and interrupt status/enable register 0x2800 SQ0_head 0x2814 SQ0_IS 0x281C CQ0_head 0x2824 CQ0_IS The register location meet UFSHCI4.0 spec definition In legacy doorbell mode, need region a, c registers In MCQ mode, need region a, b, c, d registers As you can see, region b in the middle of region a and c. If I declare "mcq" in device tree, i.e. reg = <0, 0x11270000, 0, 0x2300>, <0, 0x11271600, 0, 0x1400>; reg-names = "ufs_mem", "mcq"; Linux kernel boot fail due to register region overlapped and devm_ioremap_resource() return error. If I don't declare "mcq" region in device tree, Linux kernel still boot fail due to ufshcd_mcq_config_resource() call devm_ioreamap_resource() using calculated res_mcq which is overlapped with ufs_mem. We treat UFS as a single IP, so we suggest: 1. Map whole UFS register (include MCQ) in ufshcd_pltfrm_init() 2. In ufshcd_mcq_config_resource() assign mcq_base address directly, ie, hba->mcq_base = hba->mmio_base + MCQ_SQATTR_OFFSET(hba- >mcq_capabilities) 3. In ufshcd_mcq_vops_op_runtime_config(), assign SQD, SQIS, CQD, CQIS base, offset and stride This is why I propose ufshcd_mcq_config_resource() to be customized, not in common code Regards, Eddie Huang
On Fri, Oct 07 2022 at 19:41 -0700, Daejun Park wrote: >Hi Asutosh Das, > >>Define the mcq resources and add support to ioremap [...] >>+ 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)); >>+ res->base = NULL; >>+ ret = PTR_ERR(res->base); >I think res->base has already NULL value. > Thanks, I will fix it. >Thanks, >Daejun
On Fri, Oct 07 2022 at 20:44 -0700, Daejun Park wrote: >Hi Asutosh Das, > [...] >>+ res = &hba->res[RES_MCQ]; >>+ /* Bail if NCQ resource is provided */ >Maybe MCQ? Thanks, will fix it. > >>+ if (res->base) >>+ goto out; >>+ >>+ /* Manually allocate MCQ resource from ufs_mem */ >>+ res_mcq = res->resource; >Why we assign the value to res_mcq? > Hmm, good point, will fix it. >>+ res_mcq = devm_kzalloc(hba->dev, sizeof(*res_mcq), GFP_KERNEL); >>+ if (!res_mcq) { >>+ dev_err(hba->dev, "Failed to allocate MCQ resource\n"); >>+ return ret; >>+ } >>+ >>+ 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); >Should we free the res_mcq? > yes, will add it. Thanks. >>+ return ret; >>+ } >>+ >>+ res->base = devm_ioremap_resource(hba->dev, res_mcq); >>+ if (IS_ERR(res->base)) { >>+ dev_err(hba->dev, "MCQ registers mapping failed, err=%d\n", >>+ (int)PTR_ERR(res->base)); >Should we call remove_resource and free the res_mcq? > I'll add the cleanup here. >Thanks, >Daejun
On Mon, Oct 17 2022 at 02:27 -0700, Eddie Huang wrote: >Hi Can, > [...] >Let me explain more detail about Mediatek UFS register assignment: >a. 0x0 ~ 0x5FF: UFS common register >b. 0x1600 ~ : MCQ register >c. 0x2200: UFS common vendor specific register >d. 0x2800: SQ/CQ head/tail pointer and interrupt status/enable register > 0x2800 SQ0_head > 0x2814 SQ0_IS > 0x281C CQ0_head > 0x2824 CQ0_IS > >The register location meet UFSHCI4.0 spec definition > >In legacy doorbell mode, need region a, c registers >In MCQ mode, need region a, b, c, d registers > >As you can see, region b in the middle of region a and c. If I declare >"mcq" in device tree, i.e. >reg = <0, 0x11270000, 0, 0x2300>, > <0, 0x11271600, 0, 0x1400>; >reg-names = "ufs_mem", "mcq"; > >Linux kernel boot fail due to register region overlapped and >devm_ioremap_resource() return error. > >If I don't declare "mcq" region in device tree, Linux kernel still boot >fail due to ufshcd_mcq_config_resource() call devm_ioreamap_resource() >using calculated res_mcq which is overlapped with ufs_mem. > >We treat UFS as a single IP, so we suggest: >1. Map whole UFS register (include MCQ) in ufshcd_pltfrm_init() >2. In ufshcd_mcq_config_resource() assign mcq_base address directly, >ie, > hba->mcq_base = hba->mmio_base + MCQ_SQATTR_OFFSET(hba- >>mcq_capabilities) >3. In ufshcd_mcq_vops_op_runtime_config(), assign SQD, SQIS, CQD, CQIS >base, offset and stride > >This is why I propose ufshcd_mcq_config_resource() to be customized, >not in common code How about we add a vops to ufshcd_mcq_config_resource(). SoC vendors should make sure that the vops populates the mcq_base. >Regards, >Eddie Huang > >
Hi Asutosh, On Mon, 2022-10-17 at 18:47 -0700, Asutosh Das wrote: > > > On Mon, Oct 17 2022 at 02:27 -0700, Eddie Huang wrote: > > Hi Can, > > > > [...] > > > We treat UFS as a single IP, so we suggest: > > 1. Map whole UFS register (include MCQ) in ufshcd_pltfrm_init() > > 2. In ufshcd_mcq_config_resource() assign mcq_base address > > directly, > > ie, > > hba->mcq_base = hba->mmio_base + MCQ_SQATTR_OFFSET(hba- > > > mcq_capabilities) > > > > 3. In ufshcd_mcq_vops_op_runtime_config(), assign SQD, SQIS, CQD, > > CQIS > > base, offset and stride > > > > This is why I propose ufshcd_mcq_config_resource() to be > > customized, > > not in common code > > How about we add a vops to ufshcd_mcq_config_resource(). > SoC vendors should make sure that the vops populates the mcq_base. > It is good to add vops to ufshcd_mcq_config_resource() let SoC vendors populate mcq_base Thanks, Eddie Huang
On Tue, Oct 18 2022 at 19:57 -0700, Eddie Huang wrote: >Hi Asutosh, > [...] >> >> How about we add a vops to ufshcd_mcq_config_resource(). >> SoC vendors should make sure that the vops populates the mcq_base. >> > >It is good to add vops to ufshcd_mcq_config_resource() let SoC vendors >populate mcq_base > While adding the vops it occurred to me that it'd remain empty because ufs-qcom doesn't need it. And I'm not sure how MTK register space is laid out. So please can you help add a vops in the next version? I can address the other comment and push the series. Please let me know. > >Thanks, >Eddie Huang > -asd >
On 10/19/22 12:50, Asutosh Das wrote: > While adding the vops it occurred to me that it'd remain empty because > ufs-qcom > doesn't need it. And I'm not sure how MTK register space is laid out. > So please can you help add a vops in the next version? > I can address the other comment and push the series. Please do not introduce new vops without adding at least one implementation of the vop in the same patch series. How about letting MediaTek add more vops as necessary in a separate patch series and focusing in this patch series on UFSHCI 4.0 compliance? Thanks, Bart.
On Wed, Oct 19 2022 at 14:07 -0700, Bart Van Assche wrote: >On 10/19/22 12:50, Asutosh Das wrote: >>While adding the vops it occurred to me that it'd remain empty >>because ufs-qcom >>doesn't need it. And I'm not sure how MTK register space is laid out. >>So please can you help add a vops in the next version? >>I can address the other comment and push the series. > >Please do not introduce new vops without adding at least one >implementation of the vop in the same patch series. How about letting >MediaTek add more vops as necessary in a separate patch series and >focusing in this patch series on UFSHCI 4.0 compliance? > Yep, I agree, I'll push the version shortly without the vops. -asd >Thanks, > >Bart.
On Wed, 2022-10-19 at 14:06 -0700, Bart Van Assche wrote: > On 10/19/22 12:50, Asutosh Das wrote: > > While adding the vops it occurred to me that it'd remain empty > > because > > ufs-qcom > > doesn't need it. And I'm not sure how MTK register space is laid > > out. > > So please can you help add a vops in the next version? > > I can address the other comment and push the series. > > Please do not introduce new vops without adding at least one > implementation of the vop in the same patch series. How about > letting > MediaTek add more vops as necessary in a separate patch series and > focusing in this patch series on UFSHCI 4.0 compliance? > I am not sure whether other SoC vendor's register definition follow this patch's arrangement or not. As I explain Mediatek treat UFS as a single IP and map whole UFS register address space one time. To speed up landing this series, please bypass this vops. I will send a separate patch to add this vops Eddie
diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c index 659398d..7d0a37a 100644 --- a/drivers/ufs/core/ufs-mcq.c +++ b/drivers/ufs/core/ufs-mcq.c @@ -18,6 +18,11 @@ #define UFS_MCQ_NUM_DEV_CMD_QUEUES 1 #define UFS_MCQ_MIN_POLL_QUEUES 0 +#define MCQ_QCFGPTR_MASK GENMASK(7, 0) +#define MCQ_QCFGPTR_UNIT 0x200 +#define MCQ_SQATTR_OFFSET(c) \ + ((((c) >> 16) & MCQ_QCFGPTR_MASK) * MCQ_QCFGPTR_UNIT) +#define MCQ_QCFG_SIZE 0x40 static int rw_queue_count_set(const char *val, const struct kernel_param *kp) { @@ -67,6 +72,97 @@ module_param_cb(poll_queues, &poll_queue_count_ops, &poll_queues, 0644); MODULE_PARM_DESC(poll_queues, "Number of poll queues used for r/w. Default value is 1"); +/* 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 ufshcd_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 = 0; + + 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 -ENOMEM; + continue; + } else if (i == RES_UFS) { + res_mem = res->resource; + res->base = hba->mmio_base; + continue; + } + + res->base = devm_ioremap_resource(hba->dev, res->resource); + if (IS_ERR(res->base)) { + dev_err(hba->dev, "Failed to map res %s, err=%d\n", + res->name, (int)PTR_ERR(res->base)); + res->base = NULL; + ret = PTR_ERR(res->base); + return ret; + } + } + + /* MCQ resource provided in DT */ + res = &hba->res[RES_MCQ]; + /* Bail if NCQ resource is provided */ + if (res->base) + goto out; + + /* Manually allocate MCQ resource from ufs_mem */ + res_mcq = res->resource; + res_mcq = devm_kzalloc(hba->dev, sizeof(*res_mcq), GFP_KERNEL); + if (!res_mcq) { + dev_err(hba->dev, "Failed to allocate MCQ resource\n"); + return ret; + } + + res_mcq->start = res_mem->start + + MCQ_SQATTR_OFFSET(hba->mcq_capabilities); + res_mcq->end = res_mcq->start + hba->nr_hw_queues * MCQ_QCFG_SIZE - 1; + res_mcq->flags = res_mem->flags; + res_mcq->name = "mcq"; + + ret = insert_resource(&iomem_resource, res_mcq); + if (ret) { + dev_err(hba->dev, "Failed to insert MCQ resource, err=%d\n", ret); + return ret; + } + + res->base = devm_ioremap_resource(hba->dev, res_mcq); + if (IS_ERR(res->base)) { + dev_err(hba->dev, "MCQ registers mapping failed, err=%d\n", + (int)PTR_ERR(res->base)); + ret = PTR_ERR(res->base); + res->base = NULL; + return ret; + } + +out: + hba->mcq_base = res->base; + return 0; +} + static int ufshcd_mcq_config_nr_queues(struct ufs_hba *hba) { int i; @@ -107,7 +203,10 @@ int ufshcd_mcq_init(struct ufs_hba *hba) int ret; ret = ufshcd_mcq_config_nr_queues(hba); + if (ret) + return ret; + ret = ufshcd_mcq_config_resource(hba); return ret; } diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h index 298e103..5a5132d 100644 --- a/include/ufs/ufshcd.h +++ b/include/ufs/ufshcd.h @@ -720,6 +720,30 @@ struct ufs_hba_monitor { }; /** + * 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 ufs_hba - per adapter private structure * @mmio_base: UFSHCI base register address * @ucdl_base_addr: UFS Command Descriptor base address @@ -829,6 +853,8 @@ struct ufs_hba_monitor { * @mcq_sup: is mcq supported by UFSHC * @nr_hw_queues: number of hardware queues configured * @nr_queues: number of Queues of different queue types + * @res: array of resource info of MCQ registers + * @mcq_base: Multi circular queue registers base address */ struct ufs_hba { void __iomem *mmio_base; @@ -981,6 +1007,8 @@ struct ufs_hba { bool mcq_sup; unsigned int nr_hw_queues; unsigned int nr_queues[HCTX_MAX_TYPES]; + struct ufshcd_res_info res[RES_MAX]; + void __iomem *mcq_base; }; static inline bool is_mcq_supported(struct ufs_hba *hba)