diff mbox series

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

Message ID 20240815-ufs-bug-fix-v2-1-b373afae888f@linaro.org
State Superseded
Headers show
Series ufs: qcom: Fix probe failure on SM8550 SoC due to broken LSDBS field | expand

Commit Message

Manivannan Sadhasivam via B4 Relay Aug. 15, 2024, 5:16 a.m. UTC 
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(-)

Comments

Bart Van Assche Aug. 15, 2024, 6:09 p.m. UTC | #1 
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.
Manivannan Sadhasivam Aug. 16, 2024, 5:03 a.m. UTC | #2 
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
diff mbox series

Patch

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,
 };