| Message ID | 20240702204020.2489324-10-bvanassche@acm.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | UFS patches for kernel 6.11 | expand |
On Tue, 2024-07-02 at 13:39 -0700, Bart Van Assche wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > 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 | 20 ++++++++++++++------ > drivers/ufs/core/ufshcd-priv.h | 1 + > drivers/ufs/core/ufshcd.c | 6 ++++-- > include/ufs/ufshcd.h | 4 +++- > include/ufs/ufshci.h | 1 + > 5 files changed, 23 insertions(+), 9 deletions(-) > > diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c > index 0482c7a1e419..b2cf34a1fe48 100644 > --- a/drivers/ufs/core/ufs-mcq.c > +++ b/drivers/ufs/core/ufs-mcq.c > @@ -138,18 +138,21 @@ 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. > */ > int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba) > { > - int mac = -EOPNOTSUPP; > + int mac; > > - 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_MCQ; > + mac++; > + } else { > + mac = hba->vops->get_hba_mac(hba); > + } > if (mac < 0) > goto err; > > @@ -423,6 +426,11 @@ void ufshcd_mcq_enable(struct ufs_hba *hba) > } > EXPORT_SYMBOL_GPL(ufshcd_mcq_enable); > > +void ufshcd_mcq_disable(struct ufs_hba *hba) > +{ > + ufshcd_rmwl(hba, MCQ_MODE_SELECT, 0, REG_UFS_MEM_CFG); > +} > + > void ufshcd_mcq_config_esi(struct ufs_hba *hba, struct msi_msg *msg) > { > ufshcd_writel(hba, msg->address_lo, REG_UFS_ESILBA); > diff --git a/drivers/ufs/core/ufshcd-priv.h > b/drivers/ufs/core/ufshcd-priv.h > index 88ce93748305..ce36154ce963 100644 > --- a/drivers/ufs/core/ufshcd-priv.h > +++ b/drivers/ufs/core/ufshcd-priv.h > @@ -64,6 +64,7 @@ void ufshcd_auto_hibern8_update(struct ufs_hba > *hba, u32 ahit); > void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag, > struct cq_entry *cqe); > int ufshcd_mcq_init(struct ufs_hba *hba); > +void ufshcd_mcq_disable(struct ufs_hba *hba); > int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba); > int ufshcd_mcq_memory_alloc(struct ufs_hba *hba); > struct ufs_hw_queue *ufshcd_mcq_req_to_hwq(struct ufs_hba *hba, > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index b3444f9ce130..9e0290c6c2d3 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -8753,13 +8753,15 @@ static int ufshcd_device_init(struct ufs_hba > *hba, bool init_dev_params) > if (ret) > return ret; > if (is_mcq_supported(hba) && !hba->scsi_host_added) { > + ufshcd_mcq_enable(hba); > + hba->mcq_enabled = true; > ret = ufshcd_alloc_mcq(hba); > if (!ret) { > ufshcd_config_mcq(hba); > - ufshcd_mcq_enable(hba); > - hba->mcq_enabled = true; > } else { > /* Continue with SDB mode */ > + ufshcd_mcq_disable(hba); > + hba->mcq_enabled = false; > use_mcq_mode = false; > dev_err(hba->dev, "MCQ mode is > disabled, err=%d\n", > ret); > diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h > index c0e28a512b3c..6518997930f3 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 8d0cc73537c6..38fe97971a65 100644 > --- a/include/ufs/ufshci.h > +++ b/include/ufs/ufshci.h > @@ -68,6 +68,7 @@ enum { > /* Controller capability masks */ > enum { > MASK_TRANSFER_REQUESTS_SLOTS_SDB = 0x0000001F, > + MASK_TRANSFER_REQUESTS_SLOTS_MCQ = 0x000000FF, > MASK_NUMBER_OUTSTANDING_RTT = 0x0000FF00, > MASK_TASK_MANAGEMENT_REQUEST_SLOTS = 0x00070000, > MASK_EHSLUTRD_SUPPORTED = 0x00400000, Reviewed-by: Peter Wang <peter.wang@mediatek.com>
On Tue, Jul 02, 2024 at 01:39:17PM -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 | 20 ++++++++++++++------ > drivers/ufs/core/ufshcd-priv.h | 1 + > drivers/ufs/core/ufshcd.c | 6 ++++-- > include/ufs/ufshcd.h | 4 +++- > include/ufs/ufshci.h | 1 + > 5 files changed, 23 insertions(+), 9 deletions(-) > > diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c > index 0482c7a1e419..b2cf34a1fe48 100644 > --- a/drivers/ufs/core/ufs-mcq.c > +++ b/drivers/ufs/core/ufs-mcq.c > @@ -138,18 +138,21 @@ 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. > */ > int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba) > { > - int mac = -EOPNOTSUPP; > + int mac; > > - 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_MCQ; > + mac++; Can you add a comment to state that the MAC value is 0 based? > + } else { > + mac = hba->vops->get_hba_mac(hba); > + } > if (mac < 0) > goto err; > > @@ -423,6 +426,11 @@ void ufshcd_mcq_enable(struct ufs_hba *hba) > } > EXPORT_SYMBOL_GPL(ufshcd_mcq_enable); > > +void ufshcd_mcq_disable(struct ufs_hba *hba) > +{ > + ufshcd_rmwl(hba, MCQ_MODE_SELECT, 0, REG_UFS_MEM_CFG); > +} > + > void ufshcd_mcq_config_esi(struct ufs_hba *hba, struct msi_msg *msg) > { > ufshcd_writel(hba, msg->address_lo, REG_UFS_ESILBA); > diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h > index 88ce93748305..ce36154ce963 100644 > --- a/drivers/ufs/core/ufshcd-priv.h > +++ b/drivers/ufs/core/ufshcd-priv.h > @@ -64,6 +64,7 @@ void ufshcd_auto_hibern8_update(struct ufs_hba *hba, u32 ahit); > void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag, > struct cq_entry *cqe); > int ufshcd_mcq_init(struct ufs_hba *hba); > +void ufshcd_mcq_disable(struct ufs_hba *hba); > int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba); > int ufshcd_mcq_memory_alloc(struct ufs_hba *hba); > struct ufs_hw_queue *ufshcd_mcq_req_to_hwq(struct ufs_hba *hba, > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index b3444f9ce130..9e0290c6c2d3 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -8753,13 +8753,15 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params) > if (ret) > return ret; > if (is_mcq_supported(hba) && !hba->scsi_host_added) { > + ufshcd_mcq_enable(hba); > + hba->mcq_enabled = true; If the 'mcq_enabled' assignment goes hand in hand with ufshcd_mcq_{enable/disable}, why shouldn't it be moved inside? - Mani > ret = ufshcd_alloc_mcq(hba); > if (!ret) { > ufshcd_config_mcq(hba); > - ufshcd_mcq_enable(hba); > - hba->mcq_enabled = true; > } else { > /* Continue with SDB mode */ > + ufshcd_mcq_disable(hba); > + hba->mcq_enabled = false; > use_mcq_mode = false; > dev_err(hba->dev, "MCQ mode is disabled, err=%d\n", > ret); > diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h > index c0e28a512b3c..6518997930f3 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 8d0cc73537c6..38fe97971a65 100644 > --- a/include/ufs/ufshci.h > +++ b/include/ufs/ufshci.h > @@ -68,6 +68,7 @@ enum { > /* Controller capability masks */ > enum { > MASK_TRANSFER_REQUESTS_SLOTS_SDB = 0x0000001F, > + MASK_TRANSFER_REQUESTS_SLOTS_MCQ = 0x000000FF, > MASK_NUMBER_OUTSTANDING_RTT = 0x0000FF00, > MASK_TASK_MANAGEMENT_REQUEST_SLOTS = 0x00070000, > MASK_EHSLUTRD_SUPPORTED = 0x00400000,
On 7/3/24 6:22 AM, Manivannan Sadhasivam wrote: > On Tue, Jul 02, 2024 at 01:39:17PM -0700, Bart Van Assche wrote: >> - 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_MCQ; >> + mac++; > > Can you add a comment to state that the MAC value is 0 based? Sure. >> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c >> index b3444f9ce130..9e0290c6c2d3 100644 >> --- a/drivers/ufs/core/ufshcd.c >> +++ b/drivers/ufs/core/ufshcd.c >> @@ -8753,13 +8753,15 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params) >> if (ret) >> return ret; >> if (is_mcq_supported(hba) && !hba->scsi_host_added) { >> + ufshcd_mcq_enable(hba); >> + hba->mcq_enabled = true; > > If the 'mcq_enabled' assignment goes hand in hand with > ufshcd_mcq_{enable/disable}, why shouldn't it be moved inside? If an UFSHCI controller is reset, the controller is reset from MCQ mode to SDB mode and it is derived from the hba->mcq_enabled structure member that MCQ was enabled before the reset. In other words, moving all hba->mcq_enabled assignments into ufshcd_mcq_{enable/disable}() would break the code that resets the UFSHCI controller. Thanks, Bart.
On Wed, Jul 03, 2024 at 01:36:46PM -0700, Bart Van Assche wrote: > On 7/3/24 6:22 AM, Manivannan Sadhasivam wrote: > > On Tue, Jul 02, 2024 at 01:39:17PM -0700, Bart Van Assche wrote: > > > - 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_MCQ; > > > + mac++; > > > > Can you add a comment to state that the MAC value is 0 based? > > Sure. > > > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > > > index b3444f9ce130..9e0290c6c2d3 100644 > > > --- a/drivers/ufs/core/ufshcd.c > > > +++ b/drivers/ufs/core/ufshcd.c > > > @@ -8753,13 +8753,15 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params) > > > if (ret) > > > return ret; > > > if (is_mcq_supported(hba) && !hba->scsi_host_added) { > > > + ufshcd_mcq_enable(hba); > > > + hba->mcq_enabled = true; > > > > If the 'mcq_enabled' assignment goes hand in hand with > > ufshcd_mcq_{enable/disable}, why shouldn't it be moved inside? > > If an UFSHCI controller is reset, the controller is reset from MCQ mode > to SDB mode and it is derived from the hba->mcq_enabled structure member > that MCQ was enabled before the reset. In other words, moving all > hba->mcq_enabled assignments into ufshcd_mcq_{enable/disable}() would > break the code that resets the UFSHCI controller. > Hmm, could you please point me to the code that does this? I tried looking for it but couldn't spot. - Mani
On 7/6/24 9:33 AM, Manivannan Sadhasivam wrote: > On Wed, Jul 03, 2024 at 01:36:46PM -0700, Bart Van Assche wrote: >> If an UFSHCI controller is reset, the controller is reset from MCQ mode >> to SDB mode and it is derived from the hba->mcq_enabled structure member >> that MCQ was enabled before the reset. In other words, moving all >> hba->mcq_enabled assignments into ufshcd_mcq_{enable/disable}() would >> break the code that resets the UFSHCI controller. > > Hmm, could you please point me to the code that does this? I tried looking for > it but couldn't spot. * There is no "hba->mcq_enabled = false;" statement anywhere in the UFS driver core. This shows that the reset code does not reset hba->mcq_enabled. * From the UFSHCI specification, offset 300h, global config register: in the "reset" column I see 0h for the queue type (QT) bit. In other words, if a UFS host controller is reset, if MCQ is enabled (QT=1), it must be disabled (QT=0) until the application processor enables it again. Bart.
On Sat, Jul 06, 2024 at 08:24:06PM -0700, Bart Van Assche wrote: > On 7/6/24 9:33 AM, Manivannan Sadhasivam wrote: > > On Wed, Jul 03, 2024 at 01:36:46PM -0700, Bart Van Assche wrote: > > > If an UFSHCI controller is reset, the controller is reset from MCQ mode > > > to SDB mode and it is derived from the hba->mcq_enabled structure member > > > that MCQ was enabled before the reset. In other words, moving all > > > hba->mcq_enabled assignments into ufshcd_mcq_{enable/disable}() would > > > break the code that resets the UFSHCI controller. > > > > Hmm, could you please point me to the code that does this? I tried looking for > > it but couldn't spot. > > * There is no "hba->mcq_enabled = false;" statement anywhere in the UFS > driver core. This shows that the reset code does not reset > hba->mcq_enabled. Right. So this flag is used to reconfigure the MCQ mode upon HCI reset. Previously we never disabled MCQ mode as well. But that is being changed by this patch. > * From the UFSHCI specification, offset 300h, global config register: > in the "reset" column I see 0h for the queue type (QT) bit. In other > words, if a UFS host controller is reset, if MCQ is enabled (QT=1), > it must be disabled (QT=0) until the application processor enables it > again. > So this means, once the HCI is reset, the QT field will become '0' by default. So if MCQ mode is supposed to be enabled, then driver has to explicitly enable it. But only place where you disable MCQ is when ufshcd_alloc_mcq() fails (in this patch). Then you also set 'hba->mcq_enabled = false' afterwards. I fail to understand why you cannot move the assignment to the enable/disable API itself. If you do not set the flag after calling the ufshcd_mcq_disable() API, your argument makes sense. But that's not the case. Perhaps I'm missing anything obvious? - Mani
On 7/8/24 3:44 AM, Manivannan Sadhasivam wrote: > On Sat, Jul 06, 2024 at 08:24:06PM -0700, Bart Van Assche wrote: >> On 7/6/24 9:33 AM, Manivannan Sadhasivam wrote: >>> On Wed, Jul 03, 2024 at 01:36:46PM -0700, Bart Van Assche wrote: >>>> If an UFSHCI controller is reset, the controller is reset from MCQ mode >>>> to SDB mode and it is derived from the hba->mcq_enabled structure member >>>> that MCQ was enabled before the reset. In other words, moving all >>>> hba->mcq_enabled assignments into ufshcd_mcq_{enable/disable}() would >>>> break the code that resets the UFSHCI controller. >>> >>> Hmm, could you please point me to the code that does this? I tried looking for >>> it but couldn't spot. >> >> * There is no "hba->mcq_enabled = false;" statement anywhere in the UFS >> driver core. This shows that the reset code does not reset >> hba->mcq_enabled. > > Right. So this flag is used to reconfigure the MCQ mode upon HCI reset. > Previously we never disabled MCQ mode as well. But that is being changed by this > patch. Only in an error path. >> * From the UFSHCI specification, offset 300h, global config register: >> in the "reset" column I see 0h for the queue type (QT) bit. In other >> words, if a UFS host controller is reset, if MCQ is enabled (QT=1), >> it must be disabled (QT=0) until the application processor enables it >> again. >> > > So this means, once the HCI is reset, the QT field will become '0' by default. > So if MCQ mode is supposed to be enabled, then driver has to explicitly enable > it. > > But only place where you disable MCQ is when ufshcd_alloc_mcq() fails (in this > patch). Then you also set 'hba->mcq_enabled = false' afterwards. I fail to > understand why you cannot move the assignment to the enable/disable API itself. > > If you do not set the flag after calling the ufshcd_mcq_disable() API, your > argument makes sense. But that's not the case. Perhaps I'm missing anything > obvious? What do you want? That I move the "hba->mcq_enabled = false;" statement into ufshcd_mcq_disable()? That would be wrong because (a) it would introduce a confusing behavior difference between ufshcd_mcq_enable() (does not modify hba->mcq_enabled) and ufshcd_mcq_disable() (would modify hba->mcq_enabled if I implement your proposal) and (b) wouldn't reduce the code size because ufshcd_mcq_disable() only has one caller. Bart.
On Mon, Jul 08, 2024 at 10:10:43AM -0700, Bart Van Assche wrote: > On 7/8/24 3:44 AM, Manivannan Sadhasivam wrote: > > On Sat, Jul 06, 2024 at 08:24:06PM -0700, Bart Van Assche wrote: > > > On 7/6/24 9:33 AM, Manivannan Sadhasivam wrote: > > > > On Wed, Jul 03, 2024 at 01:36:46PM -0700, Bart Van Assche wrote: > > > > > If an UFSHCI controller is reset, the controller is reset from MCQ mode > > > > > to SDB mode and it is derived from the hba->mcq_enabled structure member > > > > > that MCQ was enabled before the reset. In other words, moving all > > > > > hba->mcq_enabled assignments into ufshcd_mcq_{enable/disable}() would > > > > > break the code that resets the UFSHCI controller. > > > > > > > > Hmm, could you please point me to the code that does this? I tried looking for > > > > it but couldn't spot. > > > > > > * There is no "hba->mcq_enabled = false;" statement anywhere in the UFS > > > driver core. This shows that the reset code does not reset > > > hba->mcq_enabled. > > > > Right. So this flag is used to reconfigure the MCQ mode upon HCI reset. > > Previously we never disabled MCQ mode as well. But that is being changed by this > > patch. > > Only in an error path. > > > > * From the UFSHCI specification, offset 300h, global config register: > > > in the "reset" column I see 0h for the queue type (QT) bit. In other > > > words, if a UFS host controller is reset, if MCQ is enabled (QT=1), > > > it must be disabled (QT=0) until the application processor enables it > > > again. > > > > > > > So this means, once the HCI is reset, the QT field will become '0' by default. > > So if MCQ mode is supposed to be enabled, then driver has to explicitly enable > > it. > > > > But only place where you disable MCQ is when ufshcd_alloc_mcq() fails (in this > > patch). Then you also set 'hba->mcq_enabled = false' afterwards. I fail to > > understand why you cannot move the assignment to the enable/disable API itself. > > > > If you do not set the flag after calling the ufshcd_mcq_disable() API, your > > argument makes sense. But that's not the case. Perhaps I'm missing anything > > obvious? > > What do you want? That I move the "hba->mcq_enabled = false;" statement > into ufshcd_mcq_disable()? That would be wrong because (a) it would > introduce a confusing behavior difference between ufshcd_mcq_enable() > (does not modify hba->mcq_enabled) and ufshcd_mcq_disable() (would > modify hba->mcq_enabled if I implement your proposal) and (b) wouldn't > reduce the code size because ufshcd_mcq_disable() only has one caller. > No, I'm not asking to move the assignment inside just the ufshcd_mcq_disable() API, but for both. You are saying that doing so will break UFSHCI reset, but I fail to understand how. After your series applied, 'hba->mcq_enabled' is set to true in 2 places of ufshcd.c. And in those 2 places, ufshcd_mcq_enable() is accompanied. There is only one place you haven't added the assignment which is during the start of ufshcd_device_init():: /* Reconfigure MCQ upon reset */ if (hba->mcq_enabled && !init_dev_params) { ufshcd_config_mcq(hba); ufshcd_mcq_enable(hba); } And this makes sense because, if mcq_enabled was already set to true, then you are not setting the flag again. But even if you have moved the 'hba->mcq_enabled = true' assignment inside ufshcd_mcq_enable(), it wouldn't cause any issues. Where does the assignment actually breaking the reset code? This part I don't understand. - Mani
diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c index 0482c7a1e419..b2cf34a1fe48 100644 --- a/drivers/ufs/core/ufs-mcq.c +++ b/drivers/ufs/core/ufs-mcq.c @@ -138,18 +138,21 @@ 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. */ int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba) { - int mac = -EOPNOTSUPP; + int mac; - 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_MCQ; + mac++; + } else { + mac = hba->vops->get_hba_mac(hba); + } if (mac < 0) goto err; @@ -423,6 +426,11 @@ void ufshcd_mcq_enable(struct ufs_hba *hba) } EXPORT_SYMBOL_GPL(ufshcd_mcq_enable); +void ufshcd_mcq_disable(struct ufs_hba *hba) +{ + ufshcd_rmwl(hba, MCQ_MODE_SELECT, 0, REG_UFS_MEM_CFG); +} + void ufshcd_mcq_config_esi(struct ufs_hba *hba, struct msi_msg *msg) { ufshcd_writel(hba, msg->address_lo, REG_UFS_ESILBA); diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h index 88ce93748305..ce36154ce963 100644 --- a/drivers/ufs/core/ufshcd-priv.h +++ b/drivers/ufs/core/ufshcd-priv.h @@ -64,6 +64,7 @@ void ufshcd_auto_hibern8_update(struct ufs_hba *hba, u32 ahit); void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag, struct cq_entry *cqe); int ufshcd_mcq_init(struct ufs_hba *hba); +void ufshcd_mcq_disable(struct ufs_hba *hba); int ufshcd_mcq_decide_queue_depth(struct ufs_hba *hba); int ufshcd_mcq_memory_alloc(struct ufs_hba *hba); struct ufs_hw_queue *ufshcd_mcq_req_to_hwq(struct ufs_hba *hba, diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index b3444f9ce130..9e0290c6c2d3 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -8753,13 +8753,15 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params) if (ret) return ret; if (is_mcq_supported(hba) && !hba->scsi_host_added) { + ufshcd_mcq_enable(hba); + hba->mcq_enabled = true; ret = ufshcd_alloc_mcq(hba); if (!ret) { ufshcd_config_mcq(hba); - ufshcd_mcq_enable(hba); - hba->mcq_enabled = true; } else { /* Continue with SDB mode */ + ufshcd_mcq_disable(hba); + hba->mcq_enabled = false; use_mcq_mode = false; dev_err(hba->dev, "MCQ mode is disabled, err=%d\n", ret); diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h index c0e28a512b3c..6518997930f3 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 8d0cc73537c6..38fe97971a65 100644 --- a/include/ufs/ufshci.h +++ b/include/ufs/ufshci.h @@ -68,6 +68,7 @@ enum { /* Controller capability masks */ enum { MASK_TRANSFER_REQUESTS_SLOTS_SDB = 0x0000001F, + MASK_TRANSFER_REQUESTS_SLOTS_MCQ = 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 | 20 ++++++++++++++------ drivers/ufs/core/ufshcd-priv.h | 1 + drivers/ufs/core/ufshcd.c | 6 ++++-- include/ufs/ufshcd.h | 4 +++- include/ufs/ufshci.h | 1 + 5 files changed, 23 insertions(+), 9 deletions(-)