diff mbox series

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

Message ID 20210310153215.371227-2-caleb@connolly.tech
State Superseded
Headers show
Series [v3,1/3] scsi: ufshcd: use a function to calculate versions | expand

Commit Message

Caleb Connolly March 10, 2021, 3:33 p.m. UTC 
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(-)

Comments

Avri Altman March 10, 2021, 4:34 p.m. UTC | #1 
> @@ -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.
Caleb Connolly March 10, 2021, 5:06 p.m. UTC | #2 
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
Avri Altman March 11, 2021, 7:35 a.m. UTC | #3 
> 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
diff mbox series

Patch

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