[PATCH 1/2] scsi: ufs: use MASK_EE_STATUS

[PATCH 1/2] scsi: ufs: use MASK_EE_STATUS

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

MASK_EE_STATUS added by 66ec6d59 was unused, but it seems to have been
defined to do this.

Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
---
 drivers/scsi/ufs/ufshcd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 096e95b..9278666 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4717,7 +4717,7 @@ static int ufshcd_disable_ee(struct ufs_hba *hba, u16 mask)
 		goto out;
 
 	val = hba->ee_ctrl_mask & ~mask;
-	val &= 0xFFFF; /* 2 bytes */
+	val &= MASK_EE_STATUS;
 	err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
 			QUERY_ATTR_IDN_EE_CONTROL, 0, 0, &val);
 	if (!err)
@@ -4745,7 +4745,7 @@ static int ufshcd_enable_ee(struct ufs_hba *hba, u16 mask)
 		goto out;
 
 	val = hba->ee_ctrl_mask | mask;
-	val &= 0xFFFF; /* 2 bytes */
+	val &= MASK_EE_STATUS;
 	err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
 			QUERY_ATTR_IDN_EE_CONTROL, 0, 0, &val);
 	if (!err)
-- 
2.9.3

[PATCH 2/2] scsi: ufs: make ufshcd_get_lists_status() register operation obvious

[kusumi.tomohiro]

From: Tomohiro Kusumi <tkusumi@tuxera.com>

It could be just cmp 0xe instead of >>1 and cmp 0x7, with readable code.

Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
---
 drivers/scsi/ufs/ufshcd.c | 11 +----------
 drivers/scsi/ufs/ufshci.h |  4 ++++
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 9278666..ad9532b 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -668,16 +668,7 @@ static inline void ufshcd_outstanding_req_clear(struct ufs_hba *hba, int tag)
  */
 static inline int ufshcd_get_lists_status(u32 reg)
 {
-	/*
-	 * The mask 0xFF is for the following HCS register bits
-	 * Bit		Description
-	 *  0		Device Present
-	 *  1		UTRLRDY
-	 *  2		UTMRLRDY
-	 *  3		UCRDY
-	 * 4-7		reserved
-	 */
-	return ((reg & 0xFF) >> 1) ^ 0x07;
+	return !((reg & UFSHCD_STATUS_READY) == (u32)UFSHCD_STATUS_READY);
 }
 
 /**
diff --git a/drivers/scsi/ufs/ufshci.h b/drivers/scsi/ufs/ufshci.h
index d14e9b9..8cd4f0e 100644
--- a/drivers/scsi/ufs/ufshci.h
+++ b/drivers/scsi/ufs/ufshci.h
@@ -159,6 +159,10 @@ enum {
 #define DEVICE_ERROR_INDICATOR			UFS_BIT(5)
 #define UIC_POWER_MODE_CHANGE_REQ_STATUS_MASK	UFS_MASK(0x7, 8)
 
+#define UFSHCD_STATUS_READY	(UTP_TRANSFER_REQ_LIST_READY |\
+				UTP_TASK_REQ_LIST_READY |\
+				UIC_COMMAND_READY)
+
 enum {
 	PWR_OK		= 0x0,
 	PWR_LOCAL	= 0x01,
-- 
2.9.3

Re: [PATCH 1/2] scsi: ufs: use MASK_EE_STATUS

[Martin K. Petersen]

> From: Tomohiro Kusumi <tkusumi@tuxera.com>
>
> MASK_EE_STATUS added by 66ec6d59 was unused, but it seems to have been
> defined to do this.
>
> Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>

Subhash: Please review these two. Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 1/2] scsi: ufs: use MASK_EE_STATUS

[Subhash Jadavani]

On 2017-04-20 05:01, kusumi.tomohiro@gmail.com wrote:
> From: Tomohiro Kusumi <tkusumi@tuxera.com>
> 
> MASK_EE_STATUS added by 66ec6d59 was unused, but it seems to have been
> defined to do this.
> 
> Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 096e95b..9278666 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -4717,7 +4717,7 @@ static int ufshcd_disable_ee(struct ufs_hba
> *hba, u16 mask)
>  		goto out;
> 
>  	val = hba->ee_ctrl_mask & ~mask;
> -	val &= 0xFFFF; /* 2 bytes */
> +	val &= MASK_EE_STATUS;
>  	err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
>  			QUERY_ATTR_IDN_EE_CONTROL, 0, 0, &val);
>  	if (!err)
> @@ -4745,7 +4745,7 @@ static int ufshcd_enable_ee(struct ufs_hba *hba, 
> u16 mask)
>  		goto out;
> 
>  	val = hba->ee_ctrl_mask | mask;
> -	val &= 0xFFFF; /* 2 bytes */
> +	val &= MASK_EE_STATUS;
>  	err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_WRITE_ATTR,
>  			QUERY_ATTR_IDN_EE_CONTROL, 0, 0, &val);
>  	if (!err)

Looks good to me.
Reviewed-by: Subhash Jadavani <subhashj@codeaurora.org>

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/2] scsi: ufs: make ufshcd_get_lists_status() register operation obvious

[Subhash Jadavani]

On 2017-04-20 05:01, kusumi.tomohiro@gmail.com wrote:
> From: Tomohiro Kusumi <tkusumi@tuxera.com>
> 
> It could be just cmp 0xe instead of >>1 and cmp 0x7, with readable 
> code.
> 
> Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 11 +----------
>  drivers/scsi/ufs/ufshci.h |  4 ++++
>  2 files changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 9278666..ad9532b 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -668,16 +668,7 @@ static inline void
> ufshcd_outstanding_req_clear(struct ufs_hba *hba, int tag)
>   */
>  static inline int ufshcd_get_lists_status(u32 reg)
>  {
> -	/*
> -	 * The mask 0xFF is for the following HCS register bits
> -	 * Bit		Description
> -	 *  0		Device Present
> -	 *  1		UTRLRDY
> -	 *  2		UTMRLRDY
> -	 *  3		UCRDY
> -	 * 4-7		reserved
> -	 */
> -	return ((reg & 0xFF) >> 1) ^ 0x07;
> +	return !((reg & UFSHCD_STATUS_READY) == (u32)UFSHCD_STATUS_READY);

yes, this makes it more readable, but i am not sure why we will have to 
type cast UFSHCD_STATUS_READY macro to u32 type? i think it is 
redundant, let me know if you think otherwise.

>  }
> 
>  /**
> diff --git a/drivers/scsi/ufs/ufshci.h b/drivers/scsi/ufs/ufshci.h
> index d14e9b9..8cd4f0e 100644
> --- a/drivers/scsi/ufs/ufshci.h
> +++ b/drivers/scsi/ufs/ufshci.h
> @@ -159,6 +159,10 @@ enum {
>  #define DEVICE_ERROR_INDICATOR			UFS_BIT(5)
>  #define UIC_POWER_MODE_CHANGE_REQ_STATUS_MASK	UFS_MASK(0x7, 8)
> 
> +#define UFSHCD_STATUS_READY	(UTP_TRANSFER_REQ_LIST_READY |\
> +				UTP_TASK_REQ_LIST_READY |\
> +				UIC_COMMAND_READY)
> +
>  enum {
>  	PWR_OK		= 0x0,
>  	PWR_LOCAL	= 0x01,

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/2] scsi: ufs: make ufshcd_get_lists_status() register operation obvious

[Tomohiro Kusumi]

Hi,
Thanks for your review.

> yes, this makes it more readable, but i am not sure why we will have to type cast UFSHCD_STATUS_READY macro to u32 type? i think it is redundant, let me know if you think otherwise.

Sorry, nothing really.
I'll resend without u32 cast in short (i.e. change to below).

return !((reg & UFSHCD_STATUS_READY) == UFSHCD_STATUS_READY);


2017-04-26 19:49 GMT+03:00 Subhash Jadavani <subhashj@codeaurora.org>:
> On 2017-04-20 05:01, kusumi.tomohiro@gmail.com wrote:
>>
>> From: Tomohiro Kusumi <tkusumi@tuxera.com>
>>
>> It could be just cmp 0xe instead of >>1 and cmp 0x7, with readable code.
>>
>> Signed-off-by: Tomohiro Kusumi <tkusumi@tuxera.com>
>> ---
>>  drivers/scsi/ufs/ufshcd.c | 11 +----------
>>  drivers/scsi/ufs/ufshci.h |  4 ++++
>>  2 files changed, 5 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 9278666..ad9532b 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -668,16 +668,7 @@ static inline void
>> ufshcd_outstanding_req_clear(struct ufs_hba *hba, int tag)
>>   */
>>  static inline int ufshcd_get_lists_status(u32 reg)
>>  {
>> -       /*
>> -        * The mask 0xFF is for the following HCS register bits
>> -        * Bit          Description
>> -        *  0           Device Present
>> -        *  1           UTRLRDY
>> -        *  2           UTMRLRDY
>> -        *  3           UCRDY
>> -        * 4-7          reserved
>> -        */
>> -       return ((reg & 0xFF) >> 1) ^ 0x07;
>> +       return !((reg & UFSHCD_STATUS_READY) == (u32)UFSHCD_STATUS_READY);
>
>
> yes, this makes it more readable, but i am not sure why we will have to type
> cast UFSHCD_STATUS_READY macro to u32 type? i think it is redundant, let me
> know if you think otherwise.
>
>>  }
>>
>>  /**
>> diff --git a/drivers/scsi/ufs/ufshci.h b/drivers/scsi/ufs/ufshci.h
>> index d14e9b9..8cd4f0e 100644
>> --- a/drivers/scsi/ufs/ufshci.h
>> +++ b/drivers/scsi/ufs/ufshci.h
>> @@ -159,6 +159,10 @@ enum {
>>  #define DEVICE_ERROR_INDICATOR                 UFS_BIT(5)
>>  #define UIC_POWER_MODE_CHANGE_REQ_STATUS_MASK  UFS_MASK(0x7, 8)
>>
>> +#define UFSHCD_STATUS_READY    (UTP_TRANSFER_REQ_LIST_READY |\
>> +                               UTP_TASK_REQ_LIST_READY |\
>> +                               UIC_COMMAND_READY)
>> +
>>  enum {
>>         PWR_OK          = 0x0,
>>         PWR_LOCAL       = 0x01,
>
>
> --
> The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project

Re: [PATCH 1/2] scsi: ufs: use MASK_EE_STATUS

[Martin K. Petersen]

> MASK_EE_STATUS added by 66ec6d59 was unused, but it seems to have been
> defined to do this.

Applied to 4.12/scsi-queue. Thank you!

-- 
Martin K. Petersen	Oracle Linux Engineering