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(-)
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 <[email protected]>
Signed-off-by: Caleb Connolly <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
---
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
Replace the UFSHCI_VERSION_xy macros.
Signed-off-by: Caleb Connolly <[email protected]>
---
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
This check is redundant as all UFS versions are currently supported.
Signed-off-by: Nitin Rawat <[email protected]>
Signed-off-by: Caleb Connolly <[email protected]>
---
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
> @@ -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.
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/[email protected]/
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
> 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/[email protected]/
>
> 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
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 <[email protected]>
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
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