scsi: ufshcd: use a macro for UFS versions

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 in dmesg. There was a patch for this almost a year
ago to which this solution was suggested, lets avoid growing a list of
versions and just use a macro instead.

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

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_VER(3, 0))
                ...


I've tested this on a device with UFS 3.0 and a device with UFS 2.1
however I don't own any older versions to test with.

        Caleb
---
Caleb Connolly (3):
      scsi: ufshcd: switch to a version macro
      scsi: ufs: qcom: use UFSHCI_VER macro
      scsi: ufshcd: remove version check

 drivers/scsi/ufs/ufs-qcom.c |  4 +--
 drivers/scsi/ufs/ufshcd.c   | 65 ++++++++++++++++------------------------
 drivers/scsi/ufs/ufshci.h   | 16 +++++-----
 3 files changed, 36 insertions(+), 49 deletions(-)

[PATCH 1/3] scsi: ufshcd: switch to a version macro

[Caleb Connolly]

Update the driver to use a macro for referencing the UFS version.

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

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 77161750c9fb..458f0382292f 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_VER(1, 0))
+		return INTERRUPT_MASK_ALL_VER_10;
+	if (hba->ufs_version <= UFSHCI_VER(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,21 @@ 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_VER
+	 * macro, we convert it to the same scheme as ufs 2.0+.
+	 */
+	if (ufshci_ver & 0x00010000)
+		ufshci_ver = UFSHCI_VER(1, ufshci_ver & 0x00000100);
+
+	return ufshci_ver;
 }
 
 /**
@@ -931,8 +931,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_VER(1, 1))
 		return UFS_UNIPRO_VER_1_41;
 	else
 		return UFS_UNIPRO_VER_1_6;
@@ -2335,7 +2334,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_VER(1, 0)) {
 		u32 rw;
 		rw = set & INTERRUPT_MASK_RW_VER_10;
 		set = rw | ((set ^ intrs) & intrs);
@@ -2355,7 +2354,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_VER(1, 0)) {
 		u32 rw;
 		rw = (set & INTERRUPT_MASK_RW_VER_10) &
 			~(intrs & INTERRUPT_MASK_RW_VER_10);
@@ -2518,8 +2517,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_VER(1, 1))
 		lrbp->command_type = UTP_CMD_TYPE_DEV_MANAGE;
 	else
 		lrbp->command_type = UTP_CMD_TYPE_UFS_STORAGE;
@@ -2546,8 +2544,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_VER(1, 1))
 		lrbp->command_type = UTP_CMD_TYPE_SCSI;
 	else
 		lrbp->command_type = UTP_CMD_TYPE_UFS_STORAGE;
@@ -6561,15 +6558,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_VER(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 +9290,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_VER(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..39a9c85f2ac0 100644
--- a/drivers/scsi/ufs/ufshci.h
+++ b/drivers/scsi/ufs/ufshci.h
@@ -74,13 +74,15 @@ 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()
+ */
+#define UFSHCI_VER(major, minor) \
+	((major << 8) + (minor << 4))
 
 /*
  * HCDDID - Host Controller Identification Descriptor
-- 
2.29.2

Re: [PATCH 1/3] scsi: ufshcd: switch to a version macro

[Christoph Hellwig]

This looks like a really nice improvement!

A bunch of comments below:

> @@ -696,10 +685,21 @@ 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;

missing eempty line after the declaration.

>  	if (hba->quirks & UFSHCD_QUIRK_BROKEN_UFS_HCI_VERSION)
> +		ufshci_ver = ufshcd_vops_get_ufs_hci_version(hba);
> +	else
> +		ufshci_ver = 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_VER
> +	 * macro, we convert it to the same scheme as ufs 2.0+.
> +	 */
> +	if (ufshci_ver & 0x00010000)
> +		ufshci_ver = UFSHCI_VER(1, ufshci_ver & 0x00000100);
> +
> +	return ufshci_ver;

I'd use early returns here to clean this up a bit:

 	if (hba->quirks & UFSHCD_QUIRK_BROKEN_UFS_HCI_VERSION)
		ufshci_ver = ufshcd_vops_get_ufs_hci_version(hba);

	...
	ufshci_ver = ufshcd_readl(hba, REG_UFS_VERSION);
	if (ufshci_ver & 0x00010000)
		return UFSHCI_VER(1, ufshci_ver & 0x00000100);
	return ufshci_ver;

> +#define UFSHCI_VER(major, minor) \
> +	((major << 8) + (minor << 4))

This needs braces around major and minor.  Or better just convert it
to an inline function (and use a lower case name).

Re: [PATCH 1/3] scsi: ufshcd: switch to a version macro

[Caleb Connolly]

Hi Christoph,

On 08/03/2021 8:00 am, Christoph Hellwig wrote:
> This looks like a really nice improvement!
>
> A bunch of comments below:
>
>> @@ -696,10 +685,21 @@ 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;
> missing eempty line after the declaration.
>
>>   	if (hba->quirks & UFSHCD_QUIRK_BROKEN_UFS_HCI_VERSION)
>> +		ufshci_ver = ufshcd_vops_get_ufs_hci_version(hba);
>> +	else
>> +		ufshci_ver = 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_VER
>> +	 * macro, we convert it to the same scheme as ufs 2.0+.
>> +	 */
>> +	if (ufshci_ver & 0x00010000)
>> +		ufshci_ver = UFSHCI_VER(1, ufshci_ver & 0x00000100);
>> +
>> +	return ufshci_ver;
> I'd use early returns here to clean this up a bit:
>
>   	if (hba->quirks & UFSHCD_QUIRK_BROKEN_UFS_HCI_VERSION)
> 		ufshci_ver = ufshcd_vops_get_ufs_hci_version(hba);
>
> 	...
> 	ufshci_ver = ufshcd_readl(hba, REG_UFS_VERSION);
> 	if (ufshci_ver & 0x00010000)
> 		return UFSHCI_VER(1, ufshci_ver & 0x00000100);
> 	return ufshci_ver;
>
>> +#define UFSHCI_VER(major, minor) \
>> +	((major << 8) + (minor << 4))
> This needs braces around major and minor.  Or better just convert it
> to an inline function (and use a lower case name).

Other (similar) implementations, like NVME_VS() use a macro here, is an 
inline function just personal preference?

I'm perfectly happy either way, so I'll switch to your suggestion.

Thanks for the review.

Regards,

Caleb

Re: [PATCH 1/3] scsi: ufshcd: switch to a version macro

[Christoph Hellwig]

On Mon, Mar 08, 2021 at 10:42:43AM +0000, Caleb Connolly wrote:
> >> +#define UFSHCI_VER(major, minor) \
> >> +	((major << 8) + (minor << 4))
> > This needs braces around major and minor.  Or better just convert it
> > to an inline function (and use a lower case name).
> 
> Other (similar) implementations, like NVME_VS() use a macro here, is an 
> inline function just personal preference?
> 
> I'm perfectly happy either way, so I'll switch to your suggestion.

In general inline functions are always preferred over non-trivial
macros.  Macros are required for a few cases where e.g. otherwise the
include dependencies would turn into a nightmare.

[PATCH 2/3] scsi: ufs: qcom: use UFSHCI_VER macro

[Caleb Connolly]

Use the new version macro, instead of the old enum.

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..00ae0476f2cc 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_VER(1, 1);
 	else
-		return UFSHCI_VERSION_20;
+		return UFSHCI_VER(2, 0);
 }
 
 /**
-- 
2.29.2

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

[Caleb Connolly]

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

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 458f0382292f..f2ca9d497a1c 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -9290,10 +9290,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_VER(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