| Message ID | 20240617210844.337476-5-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | UFS patches for kernel 6.11 | expand |
Hi Bart, > UFSHCI controllers that are compliant with the UFSHCI 4.0 standard report > the maximum number of supported commands in the controller capabilities > register. Use that value if .get_hba_mac == NULL. > > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/ufs/core/ufs-mcq.c 12 +++++++----- > include/ufs/ufshcd.h 4 +++- > include/ufs/ufshci.h 2 +- > 3 files changed, 11 insertions(+), 7 deletions(-) > > diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c > index 0482c7a1e419..d6f966f4abef 100644 > --- a/drivers/ufs/core/ufs-mcq.c > +++ b/drivers/ufs/core/ufs-mcq.c > @@ -138,7 +138,6 @@ EXPORT_SYMBOL_GPL(ufshcd_mcq_queue_cfg_addr); > * > * MAC - Max. Active Command of the Host Controller (HC) > * HC wouldn't send more than this commands to the device. > - * It is mandatory to implement get_hba_mac() to enable MCQ mode. > * Calculates and adjusts the queue depth based on the depth > * supported by the HC and ufs device. > */ > @@ -146,10 +145,13 @@ int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba) > { > int mac = -EOPNOTSUPP; This initialization can be removed. > > - if (!hba->vops !hba->vops->get_hba_mac) > - goto err; > - > - mac = hba->vops->get_hba_mac(hba); > + if (!hba->vops !hba->vops->get_hba_mac) { > + hba->capabilities = > + ufshcd_readl(hba, REG_CONTROLLER_CAPABILITIES); > + mac = (hba->capabilities & MASK_TRANSFER_REQUESTS_SLOTS) + 1; > + } else { > + mac = hba->vops->get_hba_mac(hba); > + } > if (mac < 0) > goto err; > > diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h > index d4d63507d090..d32637d267f3 100644 > --- a/include/ufs/ufshcd.h > +++ b/include/ufs/ufshcd.h > @@ -325,7 +325,9 @@ struct ufs_pwr_mode_info { > * @event_notify: called to notify important events > * @reinit_notify: called to notify reinit of UFSHCD during max gear switch > * @mcq_config_resource: called to configure MCQ platform resources > - * @get_hba_mac: called to get vendor specific mac value, mandatory for mcq mode > + * @get_hba_mac: reports maximum number of outstanding commands supported by > + * the controller. Should be implemented for UFSHCI 4.0 or later > + * controllers that are not compliant with the UFSHCI 4.0 specification. > * @op_runtime_config: called to config Operation and runtime regs Pointers > * @get_outstanding_cqs: called to get outstanding completion queues > * @config_esi: called to config Event Specific Interrupt > diff --git a/include/ufs/ufshci.h b/include/ufs/ufshci.h > index c50f92bf2e1d..899077bba2d2 100644 > --- a/include/ufs/ufshci.h > +++ b/include/ufs/ufshci.h > @@ -67,7 +67,7 @@ enum { > > /* Controller capability masks */ > enum { > - MASK_TRANSFER_REQUESTS_SLOTS = 0x0000001F, > + MASK_TRANSFER_REQUESTS_SLOTS = 0x000000FF, > MASK_NUMBER_OUTSTANDING_RTT = 0x0000FF00, > MASK_TASK_MANAGEMENT_REQUEST_SLOTS = 0x00070000, > MASK_EHSLUTRD_SUPPORTED = 0x00400000, Reviewed-by: Daejun Park <daejun7.park@samsung.com> Thanks, Daejun
On 6/17/24 6:28 PM, Daejun Park wrote: >> @@ -146,10 +145,13 @@ int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba) >> { >> int mac = -EOPNOTSUPP; > This initialization can be removed. I will remove the initialization. Thanks, Bart.
On Mon, Jun 17, 2024 at 02:07:43PM -0700, Bart Van Assche wrote: > UFSHCI controllers that are compliant with the UFSHCI 4.0 standard report > the maximum number of supported commands in the controller capabilities > register. Use that value if .get_hba_mac == NULL. > > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/ufs/core/ufs-mcq.c | 12 +++++++----- > include/ufs/ufshcd.h | 4 +++- > include/ufs/ufshci.h | 2 +- > 3 files changed, 11 insertions(+), 7 deletions(-) > > diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c > index 0482c7a1e419..d6f966f4abef 100644 > --- a/drivers/ufs/core/ufs-mcq.c > +++ b/drivers/ufs/core/ufs-mcq.c > @@ -138,7 +138,6 @@ EXPORT_SYMBOL_GPL(ufshcd_mcq_queue_cfg_addr); > * > * MAC - Max. Active Command of the Host Controller (HC) > * HC wouldn't send more than this commands to the device. > - * It is mandatory to implement get_hba_mac() to enable MCQ mode. > * Calculates and adjusts the queue depth based on the depth > * supported by the HC and ufs device. > */ > @@ -146,10 +145,13 @@ int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba) > { > int mac = -EOPNOTSUPP; > > - if (!hba->vops || !hba->vops->get_hba_mac) > - goto err; > - > - mac = hba->vops->get_hba_mac(hba); > + if (!hba->vops || !hba->vops->get_hba_mac) { > + hba->capabilities = > + ufshcd_readl(hba, REG_CONTROLLER_CAPABILITIES); > + mac = (hba->capabilities & MASK_TRANSFER_REQUESTS_SLOTS) + 1; > + } else { > + mac = hba->vops->get_hba_mac(hba); > + } > if (mac < 0) > goto err; > > diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h > index d4d63507d090..d32637d267f3 100644 > --- a/include/ufs/ufshcd.h > +++ b/include/ufs/ufshcd.h > @@ -325,7 +325,9 @@ struct ufs_pwr_mode_info { > * @event_notify: called to notify important events > * @reinit_notify: called to notify reinit of UFSHCD during max gear switch > * @mcq_config_resource: called to configure MCQ platform resources > - * @get_hba_mac: called to get vendor specific mac value, mandatory for mcq mode > + * @get_hba_mac: reports maximum number of outstanding commands supported by > + * the controller. Should be implemented for UFSHCI 4.0 or later > + * controllers that are not compliant with the UFSHCI 4.0 specification. > * @op_runtime_config: called to config Operation and runtime regs Pointers > * @get_outstanding_cqs: called to get outstanding completion queues > * @config_esi: called to config Event Specific Interrupt > diff --git a/include/ufs/ufshci.h b/include/ufs/ufshci.h > index c50f92bf2e1d..899077bba2d2 100644 > --- a/include/ufs/ufshci.h > +++ b/include/ufs/ufshci.h > @@ -67,7 +67,7 @@ enum { > > /* Controller capability masks */ > enum { > - MASK_TRANSFER_REQUESTS_SLOTS = 0x0000001F, > + MASK_TRANSFER_REQUESTS_SLOTS = 0x000000FF, This should be a separate fix that comes before this patch. But I believe this came up during MCQ review as well and I don't remember what was the reply from Can. 0x1F is the mask for SDB mode and 0xFF is the mask for MCQ mode. Can, can you comment more? - Mani
+ Can On Wed, Jun 19, 2024 at 12:43:29PM +0530, Manivannan Sadhasivam wrote: > On Mon, Jun 17, 2024 at 02:07:43PM -0700, Bart Van Assche wrote: > > UFSHCI controllers that are compliant with the UFSHCI 4.0 standard report > > the maximum number of supported commands in the controller capabilities > > register. Use that value if .get_hba_mac == NULL. > > > > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > > --- > > drivers/ufs/core/ufs-mcq.c | 12 +++++++----- > > include/ufs/ufshcd.h | 4 +++- > > include/ufs/ufshci.h | 2 +- > > 3 files changed, 11 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c > > index 0482c7a1e419..d6f966f4abef 100644 > > --- a/drivers/ufs/core/ufs-mcq.c > > +++ b/drivers/ufs/core/ufs-mcq.c > > @@ -138,7 +138,6 @@ EXPORT_SYMBOL_GPL(ufshcd_mcq_queue_cfg_addr); > > * > > * MAC - Max. Active Command of the Host Controller (HC) > > * HC wouldn't send more than this commands to the device. > > - * It is mandatory to implement get_hba_mac() to enable MCQ mode. > > * Calculates and adjusts the queue depth based on the depth > > * supported by the HC and ufs device. > > */ > > @@ -146,10 +145,13 @@ int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba) > > { > > int mac = -EOPNOTSUPP; > > > > - if (!hba->vops || !hba->vops->get_hba_mac) > > - goto err; > > - > > - mac = hba->vops->get_hba_mac(hba); > > + if (!hba->vops || !hba->vops->get_hba_mac) { > > + hba->capabilities = > > + ufshcd_readl(hba, REG_CONTROLLER_CAPABILITIES); > > + mac = (hba->capabilities & MASK_TRANSFER_REQUESTS_SLOTS) + 1; > > + } else { > > + mac = hba->vops->get_hba_mac(hba); > > + } > > if (mac < 0) > > goto err; > > > > diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h > > index d4d63507d090..d32637d267f3 100644 > > --- a/include/ufs/ufshcd.h > > +++ b/include/ufs/ufshcd.h > > @@ -325,7 +325,9 @@ struct ufs_pwr_mode_info { > > * @event_notify: called to notify important events > > * @reinit_notify: called to notify reinit of UFSHCD during max gear switch > > * @mcq_config_resource: called to configure MCQ platform resources > > - * @get_hba_mac: called to get vendor specific mac value, mandatory for mcq mode > > + * @get_hba_mac: reports maximum number of outstanding commands supported by > > + * the controller. Should be implemented for UFSHCI 4.0 or later > > + * controllers that are not compliant with the UFSHCI 4.0 specification. > > * @op_runtime_config: called to config Operation and runtime regs Pointers > > * @get_outstanding_cqs: called to get outstanding completion queues > > * @config_esi: called to config Event Specific Interrupt > > diff --git a/include/ufs/ufshci.h b/include/ufs/ufshci.h > > index c50f92bf2e1d..899077bba2d2 100644 > > --- a/include/ufs/ufshci.h > > +++ b/include/ufs/ufshci.h > > @@ -67,7 +67,7 @@ enum { > > > > /* Controller capability masks */ > > enum { > > - MASK_TRANSFER_REQUESTS_SLOTS = 0x0000001F, > > + MASK_TRANSFER_REQUESTS_SLOTS = 0x000000FF, > > This should be a separate fix that comes before this patch. But I believe this > came up during MCQ review as well and I don't remember what was the reply from > Can. 0x1F is the mask for SDB mode and 0xFF is the mask for MCQ mode. > > Can, can you comment more? > Oops. Can is not CCed. Added now. - Mani > - Mani > > -- > மணிவண்ணன் சதாசிவம்
On Wed, 2024-06-19 at 13:27 +0530, Manivannan Sadhasivam wrote: > > > --- a/include/ufs/ufshci.h > > > +++ b/include/ufs/ufshci.h > > > @@ -67,7 +67,7 @@ enum { > > > > > > /* Controller capability masks */ > > > enum { > > > -MASK_TRANSFER_REQUESTS_SLOTS= 0x0000001F, > > > +MASK_TRANSFER_REQUESTS_SLOTS= 0x000000FF, > > > > This should be a separate fix that comes before this patch. But I > believe this > > came up during MCQ review as well and I don't remember what was the > reply from > > Can. 0x1F is the mask for SDB mode and 0xFF is the mask for MCQ > mode. > > > > Can, can you comment more? > > > Hi Bart and Mani, It is correct that 0x1F is SDB mode. ufshcd_mcq_decide_queue_depth is running before mcq enable. So that value read is still SDB value, not MCQ value. Thanks. Peter > Oops. Can is not CCed. Added now. > > - Mani > > > > - Mani > > > > -- > > மணிவண்ணன் சதாசிவம் > > -- > மணிவண்ணன் சதாசிவம்
On Fri, Jun 21, 2024 at 03:32:18AM +0000, Peter Wang (王信友) wrote: > On Wed, 2024-06-19 at 13:27 +0530, Manivannan Sadhasivam wrote: > > > > --- a/include/ufs/ufshci.h > > > > +++ b/include/ufs/ufshci.h > > > > @@ -67,7 +67,7 @@ enum { > > > > > > > > /* Controller capability masks */ > > > > enum { > > > > -MASK_TRANSFER_REQUESTS_SLOTS= 0x0000001F, > > > > +MASK_TRANSFER_REQUESTS_SLOTS= 0x000000FF, > > > > > > This should be a separate fix that comes before this patch. But I > > believe this > > > came up during MCQ review as well and I don't remember what was the > > reply from > > > Can. 0x1F is the mask for SDB mode and 0xFF is the mask for MCQ > > mode. > > > > > > Can, can you comment more? > > > > > > > Hi Bart and Mani, > > It is correct that 0x1F is SDB mode. > ufshcd_mcq_decide_queue_depth is running before mcq enable. > So that value read is still SDB value, not MCQ value. > I don't quite understand. My concern was that if we change the mask for MCQ, then existing 'nutrs' value for SDB could be impacted with this change. Perhaps we should use different masks? - Mani > > Thanks. > Peter > > > > Oops. Can is not CCed. Added now. > > > > - Mani > > > > > > > - Mani > > > > > > -- > > > மணிவண்ணன் சதாசிவம் > > > > -- > > மணிவண்ணன் சதாசிவம்
On Sun, 2024-06-23 at 19:03 +0530, manivannan.sadhasivam@linaro.org wrote: > > > > > > Hi Bart and Mani, > > > > It is correct that 0x1F is SDB mode. > > ufshcd_mcq_decide_queue_depth is running before mcq enable. > > So that value read is still SDB value, not MCQ value. > > > > I don't quite understand. My concern was that if we change the mask > for MCQ, > then existing 'nutrs' value for SDB could be impacted with this > change. > > Perhaps we should use different masks? > > - Mani > Hi Mani, Yes, it is better use different mask with different mode. And we can only use 0xFF mask if 0x300[0] = 1 (MCQ enable) use 0x1F mask if 0x300[0] = 0 (Single doorbell mode) Mediatek design in MCQ mode is 64, Single doorbell mode is 32. Thanks. Peter
On 6/24/24 1:39 AM, Peter Wang (王信友) wrote: > On Sun, 2024-06-23 at 19:03 +0530, manivannan.sadhasivam@linaro.org >> Perhaps we should use different masks? > > Yes, it is better use different mask with different mode. I will introduce different masks for the SDB and MCQ modes. Thanks, Bart.
diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c index 0482c7a1e419..d6f966f4abef 100644 --- a/drivers/ufs/core/ufs-mcq.c +++ b/drivers/ufs/core/ufs-mcq.c @@ -138,7 +138,6 @@ EXPORT_SYMBOL_GPL(ufshcd_mcq_queue_cfg_addr); * * MAC - Max. Active Command of the Host Controller (HC) * HC wouldn't send more than this commands to the device. - * It is mandatory to implement get_hba_mac() to enable MCQ mode. * Calculates and adjusts the queue depth based on the depth * supported by the HC and ufs device. */ @@ -146,10 +145,13 @@ int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba) { int mac = -EOPNOTSUPP; - if (!hba->vops || !hba->vops->get_hba_mac) - goto err; - - mac = hba->vops->get_hba_mac(hba); + if (!hba->vops || !hba->vops->get_hba_mac) { + hba->capabilities = + ufshcd_readl(hba, REG_CONTROLLER_CAPABILITIES); + mac = (hba->capabilities & MASK_TRANSFER_REQUESTS_SLOTS) + 1; + } else { + mac = hba->vops->get_hba_mac(hba); + } if (mac < 0) goto err; diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h index d4d63507d090..d32637d267f3 100644 --- a/include/ufs/ufshcd.h +++ b/include/ufs/ufshcd.h @@ -325,7 +325,9 @@ struct ufs_pwr_mode_info { * @event_notify: called to notify important events * @reinit_notify: called to notify reinit of UFSHCD during max gear switch * @mcq_config_resource: called to configure MCQ platform resources - * @get_hba_mac: called to get vendor specific mac value, mandatory for mcq mode + * @get_hba_mac: reports maximum number of outstanding commands supported by + * the controller. Should be implemented for UFSHCI 4.0 or later + * controllers that are not compliant with the UFSHCI 4.0 specification. * @op_runtime_config: called to config Operation and runtime regs Pointers * @get_outstanding_cqs: called to get outstanding completion queues * @config_esi: called to config Event Specific Interrupt diff --git a/include/ufs/ufshci.h b/include/ufs/ufshci.h index c50f92bf2e1d..899077bba2d2 100644 --- a/include/ufs/ufshci.h +++ b/include/ufs/ufshci.h @@ -67,7 +67,7 @@ enum { /* Controller capability masks */ enum { - MASK_TRANSFER_REQUESTS_SLOTS = 0x0000001F, + MASK_TRANSFER_REQUESTS_SLOTS = 0x000000FF, MASK_NUMBER_OUTSTANDING_RTT = 0x0000FF00, MASK_TASK_MANAGEMENT_REQUEST_SLOTS = 0x00070000, MASK_EHSLUTRD_SUPPORTED = 0x00400000,
UFSHCI controllers that are compliant with the UFSHCI 4.0 standard report the maximum number of supported commands in the controller capabilities register. Use that value if .get_hba_mac == NULL. Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/ufs/core/ufs-mcq.c | 12 +++++++----- include/ufs/ufshcd.h | 4 +++- include/ufs/ufshci.h | 2 +- 3 files changed, 11 insertions(+), 7 deletions(-)