[PATCH v2 0/3] ufs: qcom: Fix probe failure on SM8550 SoC due to broken LSDBS field

[PATCH v2 0/3] ufs: qcom: Fix probe failure on SM8550 SoC due to broken LSDBS field

[Manivannan Sadhasivam via B4 Relay]

Hi,

This series fixes the probe failure on the Qcom SM8550 SoC due to the broken
LSDBS field in the host controller capabilities register.

Please consider this series for v6.11 as it fixes a regression.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
Changes in v2:
- Changed SDBS to LSDBS as per the final version of UFSHCI 4.0 spec
- Moved the quirk check to assignment
- Used correct fixes tag in patch 3/3
- Added tested-by tags
- Link to v1: https://lore.kernel.org/r/20240814-ufs-bug-fix-v1-0-5eb49d5f7571@linaro.org

---
Manivannan Sadhasivam (3):
      ufs: core: Rename LSDB to LSDBS to reflect the UFSHCI 4.0 spec
      ufs: core: Add a quirk for handling broken LSDBS field in controller capabilities register
      ufs: qcom: Add UFSHCD_QUIRK_BROKEN_LSDBS_CAP for SM8550 SoC

 drivers/ufs/core/ufshcd.c   | 10 +++++++---
 drivers/ufs/host/ufs-qcom.c |  6 +++++-
 include/ufs/ufshcd.h        |  9 ++++++++-
 include/ufs/ufshci.h        |  2 +-
 4 files changed, 21 insertions(+), 6 deletions(-)
---
base-commit: 7c626ce4bae1ac14f60076d00eafe71af30450ba
change-id: 20240814-ufs-bug-fix-4427fb01b860

Best regards,
-- 
Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

[PATCH v2 1/3] ufs: core: Rename LSDB to LSDBS to reflect the UFSHCI 4.0 spec

[Manivannan Sadhasivam via B4 Relay]

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

UFSHCI 4.0 spec names the 'Legacy Queue & Single Doorbell Support' field in
Controller Capabilities register as 'LSDBS'. So let's use the same
terminology in the driver to align with the spec.

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      | 2 +-
 include/ufs/ufshci.h      | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 0b3d0c8e0dda..0b1787074215 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2418,7 +2418,7 @@ static inline int ufshcd_hba_capabilities(struct ufs_hba *hba)
 
 	/*
 	 * The UFSHCI 3.0 specification does not define MCQ_SUPPORT and
-	 * LSDB_SUPPORT, but [31:29] as reserved bits with reset value 0s, which
+	 * LSDBS_SUPPORT, but [31:29] as reserved bits with reset value 0s, which
 	 * means we can simply read values regardless of version.
 	 */
 	hba->mcq_sup = FIELD_GET(MASK_MCQ_SUPPORT, hba->capabilities);
@@ -2426,7 +2426,7 @@ 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->lsdb_sup = !FIELD_GET(MASK_LSDB_SUPPORT, hba->capabilities);
+	hba->lsdbs_sup = !FIELD_GET(MASK_LSDBS_SUPPORT, hba->capabilities);
 	if (!hba->mcq_sup)
 		return 0;
 
@@ -10512,7 +10512,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	}
 
 	if (!is_mcq_supported(hba)) {
-		if (!hba->lsdb_sup) {
+		if (!hba->lsdbs_sup) {
 			dev_err(hba->dev, "%s: failed to initialize (legacy doorbell mode not supported)\n",
 				__func__);
 			err = -EINVAL;
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index cac0cdb9a916..c6ab1c671ad7 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -1109,7 +1109,7 @@ struct ufs_hba {
 	bool ext_iid_sup;
 	bool scsi_host_added;
 	bool mcq_sup;
-	bool lsdb_sup;
+	bool lsdbs_sup;
 	bool mcq_enabled;
 	struct ufshcd_res_info res[RES_MAX];
 	void __iomem *mcq_base;
diff --git a/include/ufs/ufshci.h b/include/ufs/ufshci.h
index 9917c7743d80..35013ba71b75 100644
--- a/include/ufs/ufshci.h
+++ b/include/ufs/ufshci.h
@@ -77,7 +77,7 @@ enum {
 	MASK_OUT_OF_ORDER_DATA_DELIVERY_SUPPORT	= 0x02000000,
 	MASK_UIC_DME_TEST_MODE_SUPPORT		= 0x04000000,
 	MASK_CRYPTO_SUPPORT			= 0x10000000,
-	MASK_LSDB_SUPPORT			= 0x20000000,
+	MASK_LSDBS_SUPPORT			= 0x20000000,
 	MASK_MCQ_SUPPORT			= 0x40000000,
 };
 

-- 
2.25.1

Re: [PATCH v2 1/3] ufs: core: Rename LSDB to LSDBS to reflect the UFSHCI 4.0 spec

[Bart Van Assche]

On 8/14/24 10:16 PM, Manivannan Sadhasivam via B4 Relay wrote:
>   	/*
>   	 * The UFSHCI 3.0 specification does not define MCQ_SUPPORT and
> -	 * LSDB_SUPPORT, but [31:29] as reserved bits with reset value 0s, which
> +	 * LSDBS_SUPPORT, but [31:29] as reserved bits with reset value 0s, which
>   	 * means we can simply read values regardless of version.
>   	 */

Hmm ... neither MCQ_SUPPORT nor LSDBS_SUPPORT occurs in the UFSHCI 4.0 
specification. I found the acronyms "MCQS" and "LSDBS" in that
specification. I propose either not to modify the above comment or to 
use the acronyms used in the UFSHCI 4.0 standard.

>   	hba->mcq_sup = FIELD_GET(MASK_MCQ_SUPPORT, hba->capabilities);
> @@ -2426,7 +2426,7 @@ 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->lsdb_sup = !FIELD_GET(MASK_LSDB_SUPPORT, hba->capabilities);
> +	hba->lsdbs_sup = !FIELD_GET(MASK_LSDBS_SUPPORT, hba->capabilities);
>   	if (!hba->mcq_sup)
>   		return 0;

The final "s" in "lsdbs" stands for "support" so there are now two
references to the word "support" in the "lsdbs_sup" member name. Isn't
the original structure member name ("lsdb_sup") better because it 
doesn't have that redundancy?

>   	MASK_CRYPTO_SUPPORT			= 0x10000000,
> -	MASK_LSDB_SUPPORT			= 0x20000000,
> +	MASK_LSDBS_SUPPORT			= 0x20000000,
>   	MASK_MCQ_SUPPORT			= 0x40000000,

Same comment here: in the constant name "MASK_LSDBS_SUPPORT" there are
two references to the word "support". Isn't the original name better?
Additionally, this change introduces an inconsistency between the
constant names "MASK_LSDBS_SUPPORT" and "MASK_MCQ_SUPPORT". The former
name includes the acronym from the spec (LSDBS) but the latter name not
(MCQS). Wouldn't it be better to leave this change out?

Thanks,

Bart.

Re: [PATCH v2 1/3] ufs: core: Rename LSDB to LSDBS to reflect the UFSHCI 4.0 spec

[Manivannan Sadhasivam]

On Thu, Aug 15, 2024 at 11:09:06AM -0700, Bart Van Assche wrote:
> On 8/14/24 10:16 PM, Manivannan Sadhasivam via B4 Relay wrote:
> >   	/*
> >   	 * The UFSHCI 3.0 specification does not define MCQ_SUPPORT and
> > -	 * LSDB_SUPPORT, but [31:29] as reserved bits with reset value 0s, which
> > +	 * LSDBS_SUPPORT, but [31:29] as reserved bits with reset value 0s, which
> >   	 * means we can simply read values regardless of version.
> >   	 */
> 
> Hmm ... neither MCQ_SUPPORT nor LSDBS_SUPPORT occurs in the UFSHCI 4.0
> specification. I found the acronyms "MCQS" and "LSDBS" in that
> specification. I propose either not to modify the above comment or to use
> the acronyms used in the UFSHCI 4.0 standard.
> 
> >   	hba->mcq_sup = FIELD_GET(MASK_MCQ_SUPPORT, hba->capabilities);
> > @@ -2426,7 +2426,7 @@ 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->lsdb_sup = !FIELD_GET(MASK_LSDB_SUPPORT, hba->capabilities);
> > +	hba->lsdbs_sup = !FIELD_GET(MASK_LSDBS_SUPPORT, hba->capabilities);
> >   	if (!hba->mcq_sup)
> >   		return 0;
> 
> The final "s" in "lsdbs" stands for "support" so there are now two
> references to the word "support" in the "lsdbs_sup" member name. Isn't
> the original structure member name ("lsdb_sup") better because it doesn't
> have that redundancy?
> 
> >   	MASK_CRYPTO_SUPPORT			= 0x10000000,
> > -	MASK_LSDB_SUPPORT			= 0x20000000,
> > +	MASK_LSDBS_SUPPORT			= 0x20000000,
> >   	MASK_MCQ_SUPPORT			= 0x40000000,
> 
> Same comment here: in the constant name "MASK_LSDBS_SUPPORT" there are
> two references to the word "support". Isn't the original name better?
> Additionally, this change introduces an inconsistency between the
> constant names "MASK_LSDBS_SUPPORT" and "MASK_MCQ_SUPPORT". The former
> name includes the acronym from the spec (LSDBS) but the latter name not
> (MCQS). Wouldn't it be better to leave this change out?
> 

Hmm, agree. My intention was to align with the spec, but then the _SUPPORT
suffix is screwing it up :/

I'll drop the patch then.

- Mani

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

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

[Manivannan Sadhasivam via B4 Relay]

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(-)

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;
 
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index c6ab1c671ad7..250adb6eddb6 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -676,6 +676,13 @@ enum ufshcd_quirks {
 	 * the standard best practice for managing keys).
 	 */
 	UFSHCD_QUIRK_KEYS_IN_PRDT			= 1 << 24,
+
+	/*
+	 * 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,
 };
 
 enum ufshcd_caps {

-- 
2.25.1

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

[Bart Van Assche]

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.

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

[Manivannan Sadhasivam]

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

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

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

[Bart Van Assche]

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.

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

[Manivannan Sadhasivam]

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

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

[PATCH v2 3/3] ufs: qcom: Add UFSHCD_QUIRK_BROKEN_LSDBS_CAP for SM8550 SoC

[Manivannan Sadhasivam via B4 Relay]

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

SM8550 SoC supports the UFSHCI 3.0 spec, but it reports a bogus value of
1 in the reserved 'Legacy Queue & Single Doorbell Support (LSDBS)' field of
the Controller Capabilities register. This field is supposed to read 0 as
per the spec.

But starting with commit 0c60eb0cc320 ("scsi: ufs: core: Check LSDBS cap
when !mcq"), ufshcd driver is now relying on the LSDBS field to decide when
to use the legacy doorbell mode if MCQ is not supported. And this ends up
breaking UFS on SM8550:

ufshcd-qcom 1d84000.ufs: ufshcd_init: failed to initialize (legacy doorbell mode not supported)
ufshcd-qcom 1d84000.ufs: error -EINVAL: Initialization failed with error -22

So use the UFSHCD_QUIRK_BROKEN_LSDBS_CAP quirk for SM8550 SoC so that the
ufshcd driver could use legacy doorbell mode correctly.

Fixes: 0c60eb0cc320 ("scsi: ufs: core: Check LSDBS cap when !mcq")
Tested-by: Amit Pundir <amit.pundir@linaro.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/ufs/host/ufs-qcom.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 810e637047d0..c87fdc849c62 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -857,6 +857,9 @@ static void ufs_qcom_advertise_quirks(struct ufs_hba *hba)
 
 	if (host->hw_ver.major > 0x3)
 		hba->quirks |= UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH;
+
+	if (of_device_is_compatible(hba->dev->of_node, "qcom,sm8550-ufshc"))
+		hba->quirks |= UFSHCD_QUIRK_BROKEN_LSDBS_CAP;
 }
 
 static void ufs_qcom_set_phy_gear(struct ufs_qcom_host *host)
@@ -1847,7 +1850,8 @@ static void ufs_qcom_remove(struct platform_device *pdev)
 }
 
 static const struct of_device_id ufs_qcom_of_match[] __maybe_unused = {
-	{ .compatible = "qcom,ufshc"},
+	{ .compatible = "qcom,ufshc" },
+	{ .compatible = "qcom,sm8550-ufshc" },
 	{},
 };
 MODULE_DEVICE_TABLE(of, ufs_qcom_of_match);

-- 
2.25.1

Re: [PATCH v2 3/3] ufs: qcom: Add UFSHCD_QUIRK_BROKEN_LSDBS_CAP for SM8550 SoC

[Bart Van Assche]

On 8/14/24 10:16 PM, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> SM8550 SoC supports the UFSHCI 3.0 spec, but it reports a bogus value of
> 1 in the reserved 'Legacy Queue & Single Doorbell Support (LSDBS)' field of
> the Controller Capabilities register. This field is supposed to read 0 as
> per the spec.
> 
> But starting with commit 0c60eb0cc320 ("scsi: ufs: core: Check LSDBS cap
> when !mcq"), ufshcd driver is now relying on the LSDBS field to decide when
> to use the legacy doorbell mode if MCQ is not supported. And this ends up
> breaking UFS on SM8550:
> 
> ufshcd-qcom 1d84000.ufs: ufshcd_init: failed to initialize (legacy doorbell mode not supported)
> ufshcd-qcom 1d84000.ufs: error -EINVAL: Initialization failed with error -22
> 
> So use the UFSHCD_QUIRK_BROKEN_LSDBS_CAP quirk for SM8550 SoC so that the
> ufshcd driver could use legacy doorbell mode correctly.
> 
> Fixes: 0c60eb0cc320 ("scsi: ufs: core: Check LSDBS cap when !mcq")

Since this patch depends on the previous two patches, the previous two
patches probably need a "Cc: stable" tag. Otherwise the stable
maintainers will have a hard time figuring out which patches this patch
depends on.

Since this patch by itself looks good to me:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH v2 3/3] ufs: qcom: Add UFSHCD_QUIRK_BROKEN_LSDBS_CAP for SM8550 SoC

[Manivannan Sadhasivam]

On Thu, Aug 15, 2024 at 11:01:47AM -0700, Bart Van Assche wrote:
> On 8/14/24 10:16 PM, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > 
> > SM8550 SoC supports the UFSHCI 3.0 spec, but it reports a bogus value of
> > 1 in the reserved 'Legacy Queue & Single Doorbell Support (LSDBS)' field of
> > the Controller Capabilities register. This field is supposed to read 0 as
> > per the spec.
> > 
> > But starting with commit 0c60eb0cc320 ("scsi: ufs: core: Check LSDBS cap
> > when !mcq"), ufshcd driver is now relying on the LSDBS field to decide when
> > to use the legacy doorbell mode if MCQ is not supported. And this ends up
> > breaking UFS on SM8550:
> > 
> > ufshcd-qcom 1d84000.ufs: ufshcd_init: failed to initialize (legacy doorbell mode not supported)
> > ufshcd-qcom 1d84000.ufs: error -EINVAL: Initialization failed with error -22
> > 
> > So use the UFSHCD_QUIRK_BROKEN_LSDBS_CAP quirk for SM8550 SoC so that the
> > ufshcd driver could use legacy doorbell mode correctly.
> > 
> > Fixes: 0c60eb0cc320 ("scsi: ufs: core: Check LSDBS cap when !mcq")
> 
> Since this patch depends on the previous two patches, the previous two
> patches probably need a "Cc: stable" tag. Otherwise the stable
> maintainers will have a hard time figuring out which patches this patch
> depends on.
> 

Well, I have not CCed stable list for this patch intentionally as the offending
commit got merged in v6.11-rc2. So there is no need of backport. Once this
series gets merged into one of the v6.11-rcS, all will be good.

> Since this patch by itself looks good to me:
> 
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
> 

Thanks!

- Mani

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