[v2,2/3] ufs: core: Add a quirk for handling broken LSDBS field in controller capabilities register

From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

'Legacy Queue & Single Doorbell Support (LSDBS)' field in the controller
capabilities register is supposed to be reserved for UFSHCI 3.0 based
controllers and should read as 0. But some controllers may report bogus
value of 1 due to the hardware bug. So let's add a quirk to handle those
controllers.

If the quirk is enabled by the controller driver, then LSDBS register field
will be ignored and legacy/single doorbell mode is assumed to be enabled
always.

Tested-by: Amit Pundir <amit.pundir@linaro.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/ufs/core/ufshcd.c | 6 +++++-
 include/ufs/ufshcd.h      | 7 +++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

On 8/14/24 10:16 PM, Manivannan Sadhasivam via B4 Relay wrote:
> +	/*
> +	 * This quirk needs to be enabled if the host controller has the broken
> +	 * Legacy Queue & Single Doorbell Support (LSDBS) field in Controller
> +	 * Capabilities register.
> +	 */
> +	UFSHCD_QUIRK_BROKEN_LSDBS_CAP			= 1 << 25,

The above comment is misleading because it suggests that the definition
of this bit in the UFSHCI specification is broken, which is not the
case. How about this comment?

	/*
	 * This quirk indicates that the controller reports the value 1
	 * (not supported) in the Legacy Single DoorBell Support (LSDBS)
	 * bit although it supports the legacy single doorbell mode.
	 */

Thanks,

Bart.

On 8/14/24 10:16 PM, Manivannan Sadhasivam via B4 Relay wrote:
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 0b1787074215..8c9ff8696bcd 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -2426,7 +2426,11 @@ static inline int ufshcd_hba_capabilities(struct ufs_hba *hba)
>   	 * 0h: legacy single doorbell support is available
>   	 * 1h: indicate that legacy single doorbell support has been removed
>   	 */
> -	hba->lsdbs_sup = !FIELD_GET(MASK_LSDBS_SUPPORT, hba->capabilities);
> +	if (!(hba->quirks & UFSHCD_QUIRK_BROKEN_LSDBS_CAP))
> +		hba->lsdbs_sup = !FIELD_GET(MASK_LSDBS_SUPPORT, hba->capabilities);
> +	else
> +		hba->lsdbs_sup = true;
> +
>   	if (!hba->mcq_sup)
>   		return 0;

An additional question: since the next patch only sets
UFSHCD_QUIRK_BROKEN_LSDBS_CAP for a board with a UFSHCI 3.0 controller,
do we really need the new quirk or can we replace the "!(hba->quirks &
UFSHCD_QUIRK_BROKEN_LSDBS_CAP)" test with a test that verifies that the
UFSHCI controller implements version 4.0 or later of the specification?

Thanks,

Bart.

On Thu, Aug 15, 2024 at 11:25:38AM -0700, Bart Van Assche wrote:
> On 8/14/24 10:16 PM, Manivannan Sadhasivam via B4 Relay wrote:
> > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> > index 0b1787074215..8c9ff8696bcd 100644
> > --- a/drivers/ufs/core/ufshcd.c
> > +++ b/drivers/ufs/core/ufshcd.c
> > @@ -2426,7 +2426,11 @@ static inline int ufshcd_hba_capabilities(struct ufs_hba *hba)
> >   	 * 0h: legacy single doorbell support is available
> >   	 * 1h: indicate that legacy single doorbell support has been removed
> >   	 */
> > -	hba->lsdbs_sup = !FIELD_GET(MASK_LSDBS_SUPPORT, hba->capabilities);
> > +	if (!(hba->quirks & UFSHCD_QUIRK_BROKEN_LSDBS_CAP))
> > +		hba->lsdbs_sup = !FIELD_GET(MASK_LSDBS_SUPPORT, hba->capabilities);
> > +	else
> > +		hba->lsdbs_sup = true;
> > +
> >   	if (!hba->mcq_sup)
> >   		return 0;
> 
> An additional question: since the next patch only sets
> UFSHCD_QUIRK_BROKEN_LSDBS_CAP for a board with a UFSHCI 3.0 controller,
> do we really need the new quirk or can we replace the "!(hba->quirks &
> UFSHCD_QUIRK_BROKEN_LSDBS_CAP)" test with a test that verifies that the
> UFSHCI controller implements version 4.0 or later of the specification?
> 

Ok. First I made a mistake by believing that SM8550 is a 3.0 based controller.
But by looking into the internal documentation, I learned that it is a 4.0
controller without MCQ support. So version check is not possible (and I need to
fix the description as well).

Also, while looking into the version info I found that the Qcom driver sets
UFSHCD_QUIRK_BROKEN_UFS_HCI_VERSION quirk and the callback function
get_ufs_hci_version() just hardcodes the version to 2.0. But the recent SoCs do
reveal the UFSHCD version info correctly in REG_UFS_VERSION register. So the
quirk might only be applicable for 2.0 controllers (not sure if those are
supported now). Will check that and remove that quirk altogether.

- Mani

On Thu, Aug 15, 2024 at 11:12:57AM -0700, Bart Van Assche wrote:
> On 8/14/24 10:16 PM, Manivannan Sadhasivam via B4 Relay wrote:
> > +	/*
> > +	 * This quirk needs to be enabled if the host controller has the broken
> > +	 * Legacy Queue & Single Doorbell Support (LSDBS) field in Controller
> > +	 * Capabilities register.
> > +	 */
> > +	UFSHCD_QUIRK_BROKEN_LSDBS_CAP			= 1 << 25,
> 
> The above comment is misleading because it suggests that the definition
> of this bit in the UFSHCI specification is broken, which is not the
> case.

Really? I don't see where the comment implies that the bit in the specification
is broken. It clearly says that the 'host controller has the broken bit'.

>How about this comment?
> 
> 	/*
> 	 * This quirk indicates that the controller reports the value 1
> 	 * (not supported) in the Legacy Single DoorBell Support (LSDBS)
> 	 * bit although it supports the legacy single doorbell mode.

But this comment is more elaborative. So I'll use it, thanks!

- Mani