v3: scsi: ufshcd: use a macro for UFS versions

v3: scsi: ufshcd: use a macro for UFS versions

[Caleb Connolly]

When using a device with UFS > 2.1 the error "invalid UFS version" is
misleadingly printed. There was a patch for this almost a year
ago to which this solution was suggested.

This series replaces the use of the growing UFSHCI_VERSION_xy macros with
an inline function to encode a major and minor version into the scheme
used on devices, that being:

        (major << 8) + (minor << 4)

I dealt with the different encoding used for UFS 1.x by converting it
to match the newer versions in ufshcd_get_ufs_version(). That means it's
possible to use comparisons for version checks, e.g.

        if (hba->ufs_version < ufshci_version(3, 0))
                ...

I've also dropped the "invalid UFS version" check entirely as it seems to
be more misleading than useful, and hasn't been accurate for a long time.

This has been tested on a device with UFS 3.0 and a device with UFS 2.1,
however I don't own any older devices to test with.

        Caleb
---
Changes since v1:
 * Switch from macro to static inline function
 * Address Christoph's formatting comments
 * Add Nitin's signoff on patch 3 ("scsi: ufshcd: remove version check")
Resend:
 * Fix patches 1/3 referencing the macro from v1
   instead of the new inline function
Changes since v2:
 * Remove excessive parentheses from ufshci_version()
 * Pick up Christoph's Reviewed-by

Caleb Connolly (3):
      scsi: ufshcd: use a function to calculate versions
      scsi: ufs: qcom: use ufshci_version function
      scsi: ufshcd: remove version check

 drivers/scsi/ufs/ufs-qcom.c |  4 +--
 drivers/scsi/ufs/ufshcd.c   | 66 ++++++++++++++++++---------------------------
 drivers/scsi/ufs/ufshci.h   | 17 +++++++-----
 3 files changed, 38 insertions(+), 49 deletions(-)

[PATCH v3 1/3] scsi: ufshcd: use a function to calculate versions

[Caleb Connolly]

Update the driver to use a function for referencing the UFS version.
This replaces the UFSHCI_VERSION_xy macros, and supports comparisons
where they did not.

Suggested-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Caleb Connolly <caleb@connolly.tech>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/ufs/ufshcd.c | 64 +++++++++++++++++----------------------
 drivers/scsi/ufs/ufshci.h | 18 ++++++-----
 2 files changed, 38 insertions(+), 44 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 77161750c9fb..f4d4b885d4df 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -669,23 +669,12 @@ int ufshcd_wait_for_register(struct ufs_hba *hba, u32 reg, u32 mask,
  */
 static inline u32 ufshcd_get_intr_mask(struct ufs_hba *hba)
 {
-	u32 intr_mask = 0;
+	if (hba->ufs_version == ufshci_version(1, 0))
+		return INTERRUPT_MASK_ALL_VER_10;
+	if (hba->ufs_version <= ufshci_version(2, 0))
+		return INTERRUPT_MASK_ALL_VER_11;
 
-	switch (hba->ufs_version) {
-	case UFSHCI_VERSION_10:
-		intr_mask = INTERRUPT_MASK_ALL_VER_10;
-		break;
-	case UFSHCI_VERSION_11:
-	case UFSHCI_VERSION_20:
-		intr_mask = INTERRUPT_MASK_ALL_VER_11;
-		break;
-	case UFSHCI_VERSION_21:
-	default:
-		intr_mask = INTERRUPT_MASK_ALL_VER_21;
-		break;
-	}
-
-	return intr_mask;
+	return INTERRUPT_MASK_ALL_VER_21;
 }
 
 /**
@@ -696,10 +685,22 @@ static inline u32 ufshcd_get_intr_mask(struct ufs_hba *hba)
  */
 static inline u32 ufshcd_get_ufs_version(struct ufs_hba *hba)
 {
+	u32 ufshci_ver;
+
 	if (hba->quirks & UFSHCD_QUIRK_BROKEN_UFS_HCI_VERSION)
-		return ufshcd_vops_get_ufs_hci_version(hba);
+		ufshci_ver = ufshcd_vops_get_ufs_hci_version(hba);
+	else
+		ufshci_ver = ufshcd_readl(hba, REG_UFS_VERSION);
 
-	return ufshcd_readl(hba, REG_UFS_VERSION);
+	/*
+	 * UFSHCI v1.x uses a different version scheme, in order
+	 * to allow the use of comparisons with the ufshci_version
+	 * function, we convert it to the same scheme as ufs 2.0+.
+	 */
+	if (ufshci_ver & 0x00010000)
+		return ufshci_version(1, ufshci_ver & 0x00000100);
+
+	return ufshci_ver;
 }
 
 /**
@@ -931,8 +932,7 @@ static inline bool ufshcd_is_hba_active(struct ufs_hba *hba)
 u32 ufshcd_get_local_unipro_ver(struct ufs_hba *hba)
 {
 	/* HCI version 1.0 and 1.1 supports UniPro 1.41 */
-	if ((hba->ufs_version == UFSHCI_VERSION_10) ||
-	    (hba->ufs_version == UFSHCI_VERSION_11))
+	if (hba->ufs_version <= ufshci_version(1, 1))
 		return UFS_UNIPRO_VER_1_41;
 	else
 		return UFS_UNIPRO_VER_1_6;
@@ -2335,7 +2335,7 @@ static void ufshcd_enable_intr(struct ufs_hba *hba, u32 intrs)
 {
 	u32 set = ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
 
-	if (hba->ufs_version == UFSHCI_VERSION_10) {
+	if (hba->ufs_version == ufshci_version(1, 0)) {
 		u32 rw;
 		rw = set & INTERRUPT_MASK_RW_VER_10;
 		set = rw | ((set ^ intrs) & intrs);
@@ -2355,7 +2355,7 @@ static void ufshcd_disable_intr(struct ufs_hba *hba, u32 intrs)
 {
 	u32 set = ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
 
-	if (hba->ufs_version == UFSHCI_VERSION_10) {
+	if (hba->ufs_version == ufshci_version(1, 0)) {
 		u32 rw;
 		rw = (set & INTERRUPT_MASK_RW_VER_10) &
 			~(intrs & INTERRUPT_MASK_RW_VER_10);
@@ -2518,8 +2518,7 @@ static int ufshcd_compose_devman_upiu(struct ufs_hba *hba,
 	u8 upiu_flags;
 	int ret = 0;
 
-	if ((hba->ufs_version == UFSHCI_VERSION_10) ||
-	    (hba->ufs_version == UFSHCI_VERSION_11))
+	if (hba->ufs_version <= ufshci_version(1, 1))
 		lrbp->command_type = UTP_CMD_TYPE_DEV_MANAGE;
 	else
 		lrbp->command_type = UTP_CMD_TYPE_UFS_STORAGE;
@@ -2546,8 +2545,7 @@ static int ufshcd_comp_scsi_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 	u8 upiu_flags;
 	int ret = 0;
 
-	if ((hba->ufs_version == UFSHCI_VERSION_10) ||
-	    (hba->ufs_version == UFSHCI_VERSION_11))
+	if (hba->ufs_version <= ufshci_version(1, 1))
 		lrbp->command_type = UTP_CMD_TYPE_SCSI;
 	else
 		lrbp->command_type = UTP_CMD_TYPE_UFS_STORAGE;
@@ -6561,15 +6559,10 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
 	ufshcd_prepare_lrbp_crypto(NULL, lrbp);
 	hba->dev_cmd.type = cmd_type;
 
-	switch (hba->ufs_version) {
-	case UFSHCI_VERSION_10:
-	case UFSHCI_VERSION_11:
+	if (hba->ufs_version <= ufshci_version(1, 1))
 		lrbp->command_type = UTP_CMD_TYPE_DEV_MANAGE;
-		break;
-	default:
+	else
 		lrbp->command_type = UTP_CMD_TYPE_UFS_STORAGE;
-		break;
-	}
 
 	/* update the task tag in the request upiu */
 	req_upiu->header.dword_0 |= cpu_to_be32(tag);
@@ -9298,10 +9291,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	/* Get UFS version supported by the controller */
 	hba->ufs_version = ufshcd_get_ufs_version(hba);
 
-	if ((hba->ufs_version != UFSHCI_VERSION_10) &&
-	    (hba->ufs_version != UFSHCI_VERSION_11) &&
-	    (hba->ufs_version != UFSHCI_VERSION_20) &&
-	    (hba->ufs_version != UFSHCI_VERSION_21))
+	if (hba->ufs_version < ufshci_version(1, 0))
 		dev_err(hba->dev, "invalid UFS version 0x%x\n",
 			hba->ufs_version);
 
diff --git a/drivers/scsi/ufs/ufshci.h b/drivers/scsi/ufs/ufshci.h
index 6795e1f0e8f8..335ff7b5a935 100644
--- a/drivers/scsi/ufs/ufshci.h
+++ b/drivers/scsi/ufs/ufshci.h
@@ -74,13 +74,17 @@ enum {
 #define MINOR_VERSION_NUM_MASK		UFS_MASK(0xFFFF, 0)
 #define MAJOR_VERSION_NUM_MASK		UFS_MASK(0xFFFF, 16)
 
-/* Controller UFSHCI version */
-enum {
-	UFSHCI_VERSION_10 = 0x00010000, /* 1.0 */
-	UFSHCI_VERSION_11 = 0x00010100, /* 1.1 */
-	UFSHCI_VERSION_20 = 0x00000200, /* 2.0 */
-	UFSHCI_VERSION_21 = 0x00000210, /* 2.1 */
-};
+/*
+ * Controller UFSHCI version
+ * - 2.x and newer use the following scheme:
+ *   major << 8 + minor << 4
+ * - 1.x has been converted to match this in
+ *   ufshcd_get_ufs_version()
+ */
+static inline u32 ufshci_version(u32 major, u32 minor)
+{
+	return (major << 8) + (minor << 4);
+}
 
 /*
  * HCDDID - Host Controller Identification Descriptor
-- 
2.29.2

[PATCH v3 2/3] scsi: ufs: qcom: use ufshci_version function

[Caleb Connolly]

Replace the UFSHCI_VERSION_xy macros.

Signed-off-by: Caleb Connolly <caleb@connolly.tech>
---
 drivers/scsi/ufs/ufs-qcom.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
index f97d7b0ae3b6..2d54dce0eeda 100644
--- a/drivers/scsi/ufs/ufs-qcom.c
+++ b/drivers/scsi/ufs/ufs-qcom.c
@@ -809,9 +809,9 @@ static u32 ufs_qcom_get_ufs_hci_version(struct ufs_hba *hba)
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
 
 	if (host->hw_ver.major == 0x1)
-		return UFSHCI_VERSION_11;
+		return ufshci_version(1, 1);
 	else
-		return UFSHCI_VERSION_20;
+		return ufshci_version(2, 0);
 }
 
 /**
-- 
2.29.2

[PATCH v3 3/3] scsi: ufshcd: remove version check

[Caleb Connolly]

This check is redundant as all UFS versions are currently supported.

Signed-off-by: Nitin Rawat <nitirawa@codeaurora.org>
Signed-off-by: Caleb Connolly <caleb@connolly.tech>
---
 drivers/scsi/ufs/ufshcd.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index f4d4b885d4df..a6f317f0dc9b 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -9291,10 +9291,6 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	/* Get UFS version supported by the controller */
 	hba->ufs_version = ufshcd_get_ufs_version(hba);
 
-	if (hba->ufs_version < ufshci_version(1, 0))
-		dev_err(hba->dev, "invalid UFS version 0x%x\n",
-			hba->ufs_version);
-
 	/* Get Interrupt bit mask per version */
 	hba->intr_mask = ufshcd_get_intr_mask(hba);
 
-- 
2.29.2

RE: [PATCH v3 1/3] scsi: ufshcd: use a function to calculate versions

[Avri Altman]

> @@ -9298,10 +9291,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem
> *mmio_base, unsigned int irq)
>         /* Get UFS version supported by the controller */
>         hba->ufs_version = ufshcd_get_ufs_version(hba);
> 
> -       if ((hba->ufs_version != UFSHCI_VERSION_10) &&
> -           (hba->ufs_version != UFSHCI_VERSION_11) &&
> -           (hba->ufs_version != UFSHCI_VERSION_20) &&
> -           (hba->ufs_version != UFSHCI_VERSION_21))
> +       if (hba->ufs_version < ufshci_version(1, 0))
>                 dev_err(hba->dev, "invalid UFS version 0x%x\n",
>                         hba->ufs_version);
Here you replaces the specific allowable values, with an expression
That doesn't really reflects those values.

Re: [PATCH v3 1/3] scsi: ufshcd: use a function to calculate versions

[Caleb Connolly]

Hi Avri,

On 10/03/2021 4:34 pm, Avri Altman wrote:
>> @@ -9298,10 +9291,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem
>> *mmio_base, unsigned int irq)
>>          /* Get UFS version supported by the controller */
>>          hba->ufs_version = ufshcd_get_ufs_version(hba);
>>
>> -       if ((hba->ufs_version != UFSHCI_VERSION_10) &&
>> -           (hba->ufs_version != UFSHCI_VERSION_11) &&
>> -           (hba->ufs_version != UFSHCI_VERSION_20) &&
>> -           (hba->ufs_version != UFSHCI_VERSION_21))
>> +       if (hba->ufs_version < ufshci_version(1, 0))
>>                  dev_err(hba->dev, "invalid UFS version 0x%x\n",
>>                          hba->ufs_version);
> Here you replaces the specific allowable values, with an expression
> That doesn't really reflects those values.

I took this approach based on feedback from previous patches:

https://lore.kernel.org/linux-scsi/d1b23943b6b3ae6c1f6af041cc592932@codeaurora.org/

https://lkml.org/lkml/2020/4/25/159


Patch 3 of this series removes this check entirely, as it is neither 
accurate or useful.

The driver does not fail when printing this error, nor is the list of 
"valid" UFS versions here kept up to date, I struggle to see a situation 
in which that error message would actually be helpful. Responses to 
previous patches (above) that added UFS 3.0 to the list have all 
suggested that removing this check is a more sensible approach.

Regards,

Caleb

RE: [PATCH v3 1/3] scsi: ufshcd: use a function to calculate versions

[Avri Altman]

> Hi Avri,
> 
> On 10/03/2021 4:34 pm, Avri Altman wrote:
> >> @@ -9298,10 +9291,7 @@ int ufshcd_init(struct ufs_hba *hba, void
> __iomem
> >> *mmio_base, unsigned int irq)
> >>          /* Get UFS version supported by the controller */
> >>          hba->ufs_version = ufshcd_get_ufs_version(hba);
> >>
> >> -       if ((hba->ufs_version != UFSHCI_VERSION_10) &&
> >> -           (hba->ufs_version != UFSHCI_VERSION_11) &&
> >> -           (hba->ufs_version != UFSHCI_VERSION_20) &&
> >> -           (hba->ufs_version != UFSHCI_VERSION_21))
> >> +       if (hba->ufs_version < ufshci_version(1, 0))
> >>                  dev_err(hba->dev, "invalid UFS version 0x%x\n",
> >>                          hba->ufs_version);
> > Here you replaces the specific allowable values, with an expression
> > That doesn't really reflects those values.
> 
> I took this approach based on feedback from previous patches:
> 
> https://lore.kernel.org/linux-
> scsi/d1b23943b6b3ae6c1f6af041cc592932@codeaurora.org/
> 
> https://lkml.org/lkml/2020/4/25/159
> 
> 
> Patch 3 of this series removes this check entirely, as it is neither
> accurate or useful.
I noticed that.

> 
> The driver does not fail when printing this error, nor is the list of
> "valid" UFS versions here kept up to date, I struggle to see a situation
> in which that error message would actually be helpful. Responses to
> previous patches (above) that added UFS 3.0 to the list have all
> suggested that removing this check is a more sensible approach.
OK.

Thanks,
Avri

Re: v3: scsi: ufshcd: use a macro for UFS versions

[Bean Huo]

On Wed, 2021-03-10 at 15:33 +0000, Caleb Connolly wrote:
> When using a device with UFS > 2.1 the error "invalid UFS version" is
> misleadingly printed. There was a patch for this almost a year
> ago to which this solution was suggested.
> 
> This series replaces the use of the growing UFSHCI_VERSION_xy macros
> with
> an inline function to encode a major and minor version into the
> scheme
> used on devices, that being:
> 
>         (major << 8) + (minor << 4)
> 
> I dealt with the different encoding used for UFS 1.x by converting it
> to match the newer versions in ufshcd_get_ufs_version(). That means
> it's
> possible to use comparisons for version checks, e.g.
> 
>         if (hba->ufs_version < ufshci_version(3, 0))
>                 ...
> 
> I've also dropped the "invalid UFS version" check entirely as it
> seems to
> be more misleading than useful, and hasn't been accurate for a long
> time.
> 
> This has been tested on a device with UFS 3.0 and a device with UFS
> 2.1,
> however I don't own any older devices to test with.
> 
>         Caleb
> ---
> Changes since v1:
>  * Switch from macro to static inline function
>  * Address Christoph's formatting comments
>  * Add Nitin's signoff on patch 3 ("scsi: ufshcd: remove version
> check")
> Resend:
>  * Fix patches 1/3 referencing the macro from v1
>    instead of the new inline function
> Changes since v2:
>  * Remove excessive parentheses from ufshci_version()
>  * Pick up Christoph's Reviewed-by
> 
> Caleb Connolly (3):
>       scsi: ufshcd: use a function to calculate versions
>       scsi: ufs: qcom: use ufshci_version function
>       scsi: ufshcd: remove version check
> 
>  drivers/scsi/ufs/ufs-qcom.c |  4 +--
>  drivers/scsi/ufs/ufshcd.c   | 66 ++++++++++++++++++-----------------
> ----------
>  drivers/scsi/ufs/ufshci.h   | 17 +++++++-----
>  3 files changed, 38 insertions(+), 49 deletions(-)
> 
> 
Thanks for your patch, good look to me.

Reviewed-by: Bean Huo <beanhuo@micron.com>

Re: v3: scsi: ufshcd: use a macro for UFS versions

[Martin K. Petersen]

Caleb,

> When using a device with UFS > 2.1 the error "invalid UFS version" is
> misleadingly printed. There was a patch for this almost a year
> ago to which this solution was suggested.

Applied to 5.13/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: v3: scsi: ufshcd: use a macro for UFS versions

[Martin K. Petersen]

On Wed, 10 Mar 2021 15:33:30 +0000, Caleb Connolly wrote:

> When using a device with UFS > 2.1 the error "invalid UFS version" is
> misleadingly printed. There was a patch for this almost a year
> ago to which this solution was suggested.
> 
> This series replaces the use of the growing UFSHCI_VERSION_xy macros with
> an inline function to encode a major and minor version into the scheme
> used on devices, that being:
> 
> [...]

Applied to 5.13/scsi-queue, thanks!

[1/3] scsi: ufshcd: use a function to calculate versions
      https://git.kernel.org/mkp/scsi/c/514288180178
[2/3] scsi: ufs: qcom: use ufshci_version function
      https://git.kernel.org/mkp/scsi/c/f065aca20a26
[3/3] scsi: ufshcd: remove version check
      https://git.kernel.org/mkp/scsi/c/4f5e51c0ebf0

-- 
Martin K. Petersen	Oracle Linux Engineering