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

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

[Manivannan Sadhasivam via B4 Relay]

Hi,

This series fixes the probe failure on the Qcom SM8550 SoC due to the broken
SDBS 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>
---
Manivannan Sadhasivam (3):
      ufs: core: Rename LSDB to SDBS to reflect the UFSHCI 4.0 spec
      ufs: core: Add a quirk for handling broken SDBS field in controller capabilities register
      ufs: qcom: Add UFSHCD_QUIRK_BROKEN_SDBS_CAP for SM8550 SoC

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

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

[PATCH 1/3] ufs: core: Rename LSDB to SDBS 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 'SDBS'. So let's use the same
terminology in the driver to align with the spec.

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..168b9dbc3ada 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
+	 * SDBS_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->sdbs_sup = !FIELD_GET(MASK_SDBS_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->sdbs_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..d44b19cf9f82 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 sdbs_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..b60212865e90 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_SDBS_SUPPORT			= 0x20000000,
 	MASK_MCQ_SUPPORT			= 0x40000000,
 };
 

-- 
2.25.1

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

[Bart Van Assche]

On 8/14/24 10:15 AM, Manivannan Sadhasivam via B4 Relay wrote:
> UFSHCI 4.0 spec names the 'Legacy Queue & Single Doorbell Support' field in
> Controller Capabilities register as 'SDBS'. So let's use the same
> terminology in the driver to align with the spec.

If a rename happens, we should use the name from the spec. I found the
following in the UFSHCI 4.0 specification: "Legacy Single DoorBell 
Support (LSDBS)". So please either rename SDBS into LSDBS or drop this
patch.

Thanks,

Bart.

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

[Manivannan Sadhasivam]

On Wed, Aug 14, 2024 at 10:27:48AM -0700, Bart Van Assche wrote:
> On 8/14/24 10:15 AM, Manivannan Sadhasivam via B4 Relay wrote:
> > UFSHCI 4.0 spec names the 'Legacy Queue & Single Doorbell Support' field in
> > Controller Capabilities register as 'SDBS'. So let's use the same
> > terminology in the driver to align with the spec.
> 
> If a rename happens, we should use the name from the spec. I found the
> following in the UFSHCI 4.0 specification: "Legacy Single DoorBell Support
> (LSDBS)". So please either rename SDBS into LSDBS or drop this
> patch.
> 

Hmm. I looked into the editorial version of the 4.0 spec that I got access to
and that used SDBS. Maybe that got changed in the final version. Will change it
to LSDBS.

- Mani

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

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

[Manivannan Sadhasivam via B4 Relay]

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

'Legacy Queue & Single Doorbell Support (SDBS)' 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 and MCQ is not supported,
then 'hba->sdbs_sup' field will be ignored and the SCSI device will be
added in legacy/single doorbell mode.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/ufs/core/ufshcd.c | 5 +++--
 include/ufs/ufshcd.h      | 7 +++++++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 168b9dbc3ada..acb6f261ecc2 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -10512,8 +10512,9 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	}
 
 	if (!is_mcq_supported(hba)) {
-		if (!hba->sdbs_sup) {
-			dev_err(hba->dev, "%s: failed to initialize (legacy doorbell mode not supported)\n",
+		if (!(hba->quirks & UFSHCD_QUIRK_BROKEN_SDBS_CAP) && !hba->sdbs_sup) {
+			dev_err(hba->dev,
+				"%s: failed to initialize (legacy doorbell mode not supported)\n",
 				__func__);
 			err = -EINVAL;
 			goto out_disable;
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index d44b19cf9f82..85c6ea28d45d 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 (SDBS) field in Controller
+	 * Capabilities register.
+	 */
+	UFSHCD_QUIRK_BROKEN_SDBS_CAP			= 1 << 25,
 };
 
 enum ufshcd_caps {

-- 
2.25.1

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

[Bart Van Assche]

On 8/14/24 10:15 AM, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> 'Legacy Queue & Single Doorbell Support (SDBS)' 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 and MCQ is not supported,
> then 'hba->sdbs_sup' field will be ignored and the SCSI device will be
> added in legacy/single doorbell mode.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>   drivers/ufs/core/ufshcd.c | 5 +++--
>   include/ufs/ufshcd.h      | 7 +++++++
>   2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 168b9dbc3ada..acb6f261ecc2 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -10512,8 +10512,9 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
>   	}
>   
>   	if (!is_mcq_supported(hba)) {
> -		if (!hba->sdbs_sup) {
> -			dev_err(hba->dev, "%s: failed to initialize (legacy doorbell mode not supported)\n",
> +		if (!(hba->quirks & UFSHCD_QUIRK_BROKEN_SDBS_CAP) && !hba->sdbs_sup) {
> +			dev_err(hba->dev,
> +				"%s: failed to initialize (legacy doorbell mode not supported)\n",
>   				__func__);
>   			err = -EINVAL;
>   			goto out_disable;

Adding a check for the UFSHCD_QUIRK_BROKEN_SDBS_CAP quirk everywhere
hba->sdbs_sup is tested is wrong. Please move this check to the code
that assigns a value to hba->sdbs_sup.

Thanks,

Bart.

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

[Manivannan Sadhasivam]

On Wed, Aug 14, 2024 at 10:29:11AM -0700, Bart Van Assche wrote:
> On 8/14/24 10:15 AM, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > 
> > 'Legacy Queue & Single Doorbell Support (SDBS)' 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 and MCQ is not supported,
> > then 'hba->sdbs_sup' field will be ignored and the SCSI device will be
> > added in legacy/single doorbell mode.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >   drivers/ufs/core/ufshcd.c | 5 +++--
> >   include/ufs/ufshcd.h      | 7 +++++++
> >   2 files changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> > index 168b9dbc3ada..acb6f261ecc2 100644
> > --- a/drivers/ufs/core/ufshcd.c
> > +++ b/drivers/ufs/core/ufshcd.c
> > @@ -10512,8 +10512,9 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
> >   	}
> >   	if (!is_mcq_supported(hba)) {
> > -		if (!hba->sdbs_sup) {
> > -			dev_err(hba->dev, "%s: failed to initialize (legacy doorbell mode not supported)\n",
> > +		if (!(hba->quirks & UFSHCD_QUIRK_BROKEN_SDBS_CAP) && !hba->sdbs_sup) {
> > +			dev_err(hba->dev,
> > +				"%s: failed to initialize (legacy doorbell mode not supported)\n",
> >   				__func__);
> >   			err = -EINVAL;
> >   			goto out_disable;
> 
> Adding a check for the UFSHCD_QUIRK_BROKEN_SDBS_CAP quirk everywhere
> hba->sdbs_sup is tested is wrong. Please move this check to the code
> that assigns a value to hba->sdbs_sup.
> 

I thought it wouldn't matter since it is used in just one place, but I agree it
is best to keep it in assignment itself.

- Mani

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

[PATCH 3/3] ufs: qcom: Add UFSHCD_QUIRK_BROKEN_SDBS_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 (SDBS)' 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 SDBS 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_SDBS_CAP quirk for SM8550 SoC so that the
ufshcd driver could use legacy doorbell mode correctly.

Fixes: 674f8bfb1848 ("ufs: qcom: Add UFSHCD_QUIRK_BROKEN_SDBS_CAP for SM8550 SoC")
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..391b814c318e 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_SDBS_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 0/3] ufs: qcom: Fix probe failure on SM8550 SoC due to broken SDBS field

[Amit Pundir]

On Wed, 14 Aug 2024 at 22:45, Manivannan Sadhasivam via B4 Relay
<devnull+manivannan.sadhasivam.linaro.org@kernel.org> wrote:
>
> Hi,
>
> This series fixes the probe failure on the Qcom SM8550 SoC due to the broken
> SDBS field in the host controller capabilities register.
>
> Please consider this series for v6.11 as it fixes a regression.

Thank you Mani. This series fixes the UFS regression reported on
SM8550-HDK with v6.11-rc2.

Tested-by: Amit Pundir <amit.pundir@linaro.org>

>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
> Manivannan Sadhasivam (3):
>       ufs: core: Rename LSDB to SDBS to reflect the UFSHCI 4.0 spec
>       ufs: core: Add a quirk for handling broken SDBS field in controller capabilities register
>       ufs: qcom: Add UFSHCD_QUIRK_BROKEN_SDBS_CAP for SM8550 SoC
>
>  drivers/ufs/core/ufshcd.c   | 9 +++++----
>  drivers/ufs/host/ufs-qcom.c | 6 +++++-
>  include/ufs/ufshcd.h        | 9 ++++++++-
>  include/ufs/ufshci.h        | 2 +-
>  4 files changed, 19 insertions(+), 7 deletions(-)
> ---
> base-commit: 7c626ce4bae1ac14f60076d00eafe71af30450ba
> change-id: 20240814-ufs-bug-fix-4427fb01b860
>
> Best regards,
> --
> Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>
>
>