Re: [PATCH v4 9/9] scsi: ufs: Make .get_hba_mac() optional

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

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

-- 
மணிவண்ணன் சதாசிவம்