[v1,1/1] scsi: ufs: set device as active power mode after resetting device
diff mbox series

Message ID 20200327095835.10293-1-stanley.chu@mediatek.com
State Mainlined
Commit 1764fa2ab97ade8de7269eb34f2740c54e38fc4a
Headers show
Series
  • [v1,1/1] scsi: ufs: set device as active power mode after resetting device
Related show

Commit Message

Stanley Chu March 27, 2020, 9:58 a.m. UTC
Currently ufshcd driver assumes that bInitPowerMode parameter
is not changed by any vendors thus device power mode can be set as
"Active" during initialization.

According to UFS JEDEC specification, device power mode shall be
"Active" after HW Reset is triggered if the bInitPowerMode parameter
in Device Descriptor is default value.

By above description, we can set device power mode as "Active" after
device reset is triggered by vendor's callback. With this change,
the link startup performance can be improved in some cases
by not setting link_startup_again as true in ufshcd_link_startup().

Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufshcd.c | 13 -------------
 drivers/scsi/ufs/ufshcd.h | 14 ++++++++++++++
 2 files changed, 14 insertions(+), 13 deletions(-)

Comments

Can Guo March 28, 2020, 2:14 a.m. UTC | #1
On 2020-03-27 17:58, Stanley Chu wrote:
> Currently ufshcd driver assumes that bInitPowerMode parameter
> is not changed by any vendors thus device power mode can be set as
> "Active" during initialization.
> 
> According to UFS JEDEC specification, device power mode shall be
> "Active" after HW Reset is triggered if the bInitPowerMode parameter
> in Device Descriptor is default value.
> 
> By above description, we can set device power mode as "Active" after
> device reset is triggered by vendor's callback. With this change,
> the link startup performance can be improved in some cases
> by not setting link_startup_again as true in ufshcd_link_startup().
> 
> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 13 -------------
>  drivers/scsi/ufs/ufshcd.h | 14 ++++++++++++++
>  2 files changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 227660a1a446..f0a35b289b7c 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -171,19 +171,6 @@ enum {
>  #define ufshcd_clear_eh_in_progress(h) \
>  	((h)->eh_flags &= ~UFSHCD_EH_IN_PROGRESS)
> 
> -#define ufshcd_set_ufs_dev_active(h) \
> -	((h)->curr_dev_pwr_mode = UFS_ACTIVE_PWR_MODE)
> -#define ufshcd_set_ufs_dev_sleep(h) \
> -	((h)->curr_dev_pwr_mode = UFS_SLEEP_PWR_MODE)
> -#define ufshcd_set_ufs_dev_poweroff(h) \
> -	((h)->curr_dev_pwr_mode = UFS_POWERDOWN_PWR_MODE)
> -#define ufshcd_is_ufs_dev_active(h) \
> -	((h)->curr_dev_pwr_mode == UFS_ACTIVE_PWR_MODE)
> -#define ufshcd_is_ufs_dev_sleep(h) \
> -	((h)->curr_dev_pwr_mode == UFS_SLEEP_PWR_MODE)
> -#define ufshcd_is_ufs_dev_poweroff(h) \
> -	((h)->curr_dev_pwr_mode == UFS_POWERDOWN_PWR_MODE)
> -
>  struct ufs_pm_lvl_states ufs_pm_lvl_states[] = {
>  	{UFS_ACTIVE_PWR_MODE, UIC_LINK_ACTIVE_STATE},
>  	{UFS_ACTIVE_PWR_MODE, UIC_LINK_HIBERN8_STATE},
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index b7bd81795c24..7a9d1d170719 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -129,6 +129,19 @@ enum uic_link_state {
>  #define ufshcd_set_link_hibern8(hba) ((hba)->uic_link_state = \
>  				    UIC_LINK_HIBERN8_STATE)
> 
> +#define ufshcd_set_ufs_dev_active(h) \
> +	((h)->curr_dev_pwr_mode = UFS_ACTIVE_PWR_MODE)
> +#define ufshcd_set_ufs_dev_sleep(h) \
> +	((h)->curr_dev_pwr_mode = UFS_SLEEP_PWR_MODE)
> +#define ufshcd_set_ufs_dev_poweroff(h) \
> +	((h)->curr_dev_pwr_mode = UFS_POWERDOWN_PWR_MODE)
> +#define ufshcd_is_ufs_dev_active(h) \
> +	((h)->curr_dev_pwr_mode == UFS_ACTIVE_PWR_MODE)
> +#define ufshcd_is_ufs_dev_sleep(h) \
> +	((h)->curr_dev_pwr_mode == UFS_SLEEP_PWR_MODE)
> +#define ufshcd_is_ufs_dev_poweroff(h) \
> +	((h)->curr_dev_pwr_mode == UFS_POWERDOWN_PWR_MODE)
> +
>  /*
>   * UFS Power management levels.
>   * Each level is in increasing order of power savings.
> @@ -1091,6 +1104,7 @@ static inline void
> ufshcd_vops_device_reset(struct ufs_hba *hba)
>  {
>  	if (hba->vops && hba->vops->device_reset) {
>  		hba->vops->device_reset(hba);
> +		ufshcd_set_ufs_dev_active(hba);
>  		ufshcd_update_reg_hist(&hba->ufs_stats.dev_reset, 0);
>  	}
>  }

Reviewed-by: Can Guo <cang@codeaurora.org>
Can Guo March 28, 2020, 2:30 a.m. UTC | #2
Hi Stanley,

On 2020-03-28 10:14, Can Guo wrote:
> On 2020-03-27 17:58, Stanley Chu wrote:
>> Currently ufshcd driver assumes that bInitPowerMode parameter
>> is not changed by any vendors thus device power mode can be set as
>> "Active" during initialization.
>> 
>> According to UFS JEDEC specification, device power mode shall be
>> "Active" after HW Reset is triggered if the bInitPowerMode parameter
>> in Device Descriptor is default value.
>> 
>> By above description, we can set device power mode as "Active" after
>> device reset is triggered by vendor's callback. With this change,
>> the link startup performance can be improved in some cases
>> by not setting link_startup_again as true in ufshcd_link_startup().
>> 
>> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
>> ---
>>  drivers/scsi/ufs/ufshcd.c | 13 -------------
>>  drivers/scsi/ufs/ufshcd.h | 14 ++++++++++++++
>>  2 files changed, 14 insertions(+), 13 deletions(-)
>> 
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index 227660a1a446..f0a35b289b7c 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -171,19 +171,6 @@ enum {
>>  #define ufshcd_clear_eh_in_progress(h) \
>>  	((h)->eh_flags &= ~UFSHCD_EH_IN_PROGRESS)
>> 
>> -#define ufshcd_set_ufs_dev_active(h) \
>> -	((h)->curr_dev_pwr_mode = UFS_ACTIVE_PWR_MODE)
>> -#define ufshcd_set_ufs_dev_sleep(h) \
>> -	((h)->curr_dev_pwr_mode = UFS_SLEEP_PWR_MODE)
>> -#define ufshcd_set_ufs_dev_poweroff(h) \
>> -	((h)->curr_dev_pwr_mode = UFS_POWERDOWN_PWR_MODE)
>> -#define ufshcd_is_ufs_dev_active(h) \
>> -	((h)->curr_dev_pwr_mode == UFS_ACTIVE_PWR_MODE)
>> -#define ufshcd_is_ufs_dev_sleep(h) \
>> -	((h)->curr_dev_pwr_mode == UFS_SLEEP_PWR_MODE)
>> -#define ufshcd_is_ufs_dev_poweroff(h) \
>> -	((h)->curr_dev_pwr_mode == UFS_POWERDOWN_PWR_MODE)
>> -
>>  struct ufs_pm_lvl_states ufs_pm_lvl_states[] = {
>>  	{UFS_ACTIVE_PWR_MODE, UIC_LINK_ACTIVE_STATE},
>>  	{UFS_ACTIVE_PWR_MODE, UIC_LINK_HIBERN8_STATE},
>> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
>> index b7bd81795c24..7a9d1d170719 100644
>> --- a/drivers/scsi/ufs/ufshcd.h
>> +++ b/drivers/scsi/ufs/ufshcd.h
>> @@ -129,6 +129,19 @@ enum uic_link_state {
>>  #define ufshcd_set_link_hibern8(hba) ((hba)->uic_link_state = \
>>  				    UIC_LINK_HIBERN8_STATE)
>> 
>> +#define ufshcd_set_ufs_dev_active(h) \
>> +	((h)->curr_dev_pwr_mode = UFS_ACTIVE_PWR_MODE)
>> +#define ufshcd_set_ufs_dev_sleep(h) \
>> +	((h)->curr_dev_pwr_mode = UFS_SLEEP_PWR_MODE)
>> +#define ufshcd_set_ufs_dev_poweroff(h) \
>> +	((h)->curr_dev_pwr_mode = UFS_POWERDOWN_PWR_MODE)
>> +#define ufshcd_is_ufs_dev_active(h) \
>> +	((h)->curr_dev_pwr_mode == UFS_ACTIVE_PWR_MODE)
>> +#define ufshcd_is_ufs_dev_sleep(h) \
>> +	((h)->curr_dev_pwr_mode == UFS_SLEEP_PWR_MODE)
>> +#define ufshcd_is_ufs_dev_poweroff(h) \
>> +	((h)->curr_dev_pwr_mode == UFS_POWERDOWN_PWR_MODE)
>> +
>>  /*
>>   * UFS Power management levels.
>>   * Each level is in increasing order of power savings.
>> @@ -1091,6 +1104,7 @@ static inline void
>> ufshcd_vops_device_reset(struct ufs_hba *hba)
>>  {
>>  	if (hba->vops && hba->vops->device_reset) {
>>  		hba->vops->device_reset(hba);
>> +		ufshcd_set_ufs_dev_active(hba);
>>  		ufshcd_update_reg_hist(&hba->ufs_stats.dev_reset, 0);
>>  	}
>>  }
> 
> Reviewed-by: Can Guo <cang@codeaurora.org>

I guess what you also want my patch -

"scsi: ufs: full reinit upon resume if link was off"

Please help review it, thanks.

Best regard,
Can Guo.
Asutosh Das (asd) March 30, 2020, 5 p.m. UTC | #3
Hi,

On 3/27/2020 2:58 AM, Stanley Chu wrote:
> Currently ufshcd driver assumes that bInitPowerMode parameter
> is not changed by any vendors thus device power mode can be set as
> "Active" during initialization.
> 
> According to UFS JEDEC specification, device power mode shall be
> "Active" after HW Reset is triggered if the bInitPowerMode parameter
> in Device Descriptor is default value.
> 
> By above description, we can set device power mode as "Active" after
> device reset is triggered by vendor's callback. With this change,
> the link startup performance can be improved in some cases
> by not setting link_startup_again as true in ufshcd_link_startup().
> 
> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
> ---

Reviewed-by: Asutosh Das <asutoshd@codeaurora.org>

>   drivers/scsi/ufs/ufshcd.c | 13 -------------
>   drivers/scsi/ufs/ufshcd.h | 14 ++++++++++++++
>   2 files changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 227660a1a446..f0a35b289b7c 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -171,19 +171,6 @@ enum {
>   #define ufshcd_clear_eh_in_progress(h) \
>   	((h)->eh_flags &= ~UFSHCD_EH_IN_PROGRESS)
>   
> -#define ufshcd_set_ufs_dev_active(h) \
> -	((h)->curr_dev_pwr_mode = UFS_ACTIVE_PWR_MODE)
> -#define ufshcd_set_ufs_dev_sleep(h) \
> -	((h)->curr_dev_pwr_mode = UFS_SLEEP_PWR_MODE)
> -#define ufshcd_set_ufs_dev_poweroff(h) \
> -	((h)->curr_dev_pwr_mode = UFS_POWERDOWN_PWR_MODE)
> -#define ufshcd_is_ufs_dev_active(h) \
> -	((h)->curr_dev_pwr_mode == UFS_ACTIVE_PWR_MODE)
> -#define ufshcd_is_ufs_dev_sleep(h) \
> -	((h)->curr_dev_pwr_mode == UFS_SLEEP_PWR_MODE)
> -#define ufshcd_is_ufs_dev_poweroff(h) \
> -	((h)->curr_dev_pwr_mode == UFS_POWERDOWN_PWR_MODE)
> -
>   struct ufs_pm_lvl_states ufs_pm_lvl_states[] = {
>   	{UFS_ACTIVE_PWR_MODE, UIC_LINK_ACTIVE_STATE},
>   	{UFS_ACTIVE_PWR_MODE, UIC_LINK_HIBERN8_STATE},
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index b7bd81795c24..7a9d1d170719 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -129,6 +129,19 @@ enum uic_link_state {
>   #define ufshcd_set_link_hibern8(hba) ((hba)->uic_link_state = \
>   				    UIC_LINK_HIBERN8_STATE)
>   
> +#define ufshcd_set_ufs_dev_active(h) \
> +	((h)->curr_dev_pwr_mode = UFS_ACTIVE_PWR_MODE)
> +#define ufshcd_set_ufs_dev_sleep(h) \
> +	((h)->curr_dev_pwr_mode = UFS_SLEEP_PWR_MODE)
> +#define ufshcd_set_ufs_dev_poweroff(h) \
> +	((h)->curr_dev_pwr_mode = UFS_POWERDOWN_PWR_MODE)
> +#define ufshcd_is_ufs_dev_active(h) \
> +	((h)->curr_dev_pwr_mode == UFS_ACTIVE_PWR_MODE)
> +#define ufshcd_is_ufs_dev_sleep(h) \
> +	((h)->curr_dev_pwr_mode == UFS_SLEEP_PWR_MODE)
> +#define ufshcd_is_ufs_dev_poweroff(h) \
> +	((h)->curr_dev_pwr_mode == UFS_POWERDOWN_PWR_MODE)
> +
>   /*
>    * UFS Power management levels.
>    * Each level is in increasing order of power savings.
> @@ -1091,6 +1104,7 @@ static inline void ufshcd_vops_device_reset(struct ufs_hba *hba)
>   {
>   	if (hba->vops && hba->vops->device_reset) {
>   		hba->vops->device_reset(hba);
> +		ufshcd_set_ufs_dev_active(hba);
>   		ufshcd_update_reg_hist(&hba->ufs_stats.dev_reset, 0);
>   	}
>   }
>
Martin K. Petersen April 1, 2020, 1:54 a.m. UTC | #4
Stanley,

> Currently ufshcd driver assumes that bInitPowerMode parameter is not
> changed by any vendors thus device power mode can be set as "Active"
> during initialization.

Applied to 5.7/scsi-queue, thanks!

Patch
diff mbox series

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 227660a1a446..f0a35b289b7c 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -171,19 +171,6 @@  enum {
 #define ufshcd_clear_eh_in_progress(h) \
 	((h)->eh_flags &= ~UFSHCD_EH_IN_PROGRESS)
 
-#define ufshcd_set_ufs_dev_active(h) \
-	((h)->curr_dev_pwr_mode = UFS_ACTIVE_PWR_MODE)
-#define ufshcd_set_ufs_dev_sleep(h) \
-	((h)->curr_dev_pwr_mode = UFS_SLEEP_PWR_MODE)
-#define ufshcd_set_ufs_dev_poweroff(h) \
-	((h)->curr_dev_pwr_mode = UFS_POWERDOWN_PWR_MODE)
-#define ufshcd_is_ufs_dev_active(h) \
-	((h)->curr_dev_pwr_mode == UFS_ACTIVE_PWR_MODE)
-#define ufshcd_is_ufs_dev_sleep(h) \
-	((h)->curr_dev_pwr_mode == UFS_SLEEP_PWR_MODE)
-#define ufshcd_is_ufs_dev_poweroff(h) \
-	((h)->curr_dev_pwr_mode == UFS_POWERDOWN_PWR_MODE)
-
 struct ufs_pm_lvl_states ufs_pm_lvl_states[] = {
 	{UFS_ACTIVE_PWR_MODE, UIC_LINK_ACTIVE_STATE},
 	{UFS_ACTIVE_PWR_MODE, UIC_LINK_HIBERN8_STATE},
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index b7bd81795c24..7a9d1d170719 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -129,6 +129,19 @@  enum uic_link_state {
 #define ufshcd_set_link_hibern8(hba) ((hba)->uic_link_state = \
 				    UIC_LINK_HIBERN8_STATE)
 
+#define ufshcd_set_ufs_dev_active(h) \
+	((h)->curr_dev_pwr_mode = UFS_ACTIVE_PWR_MODE)
+#define ufshcd_set_ufs_dev_sleep(h) \
+	((h)->curr_dev_pwr_mode = UFS_SLEEP_PWR_MODE)
+#define ufshcd_set_ufs_dev_poweroff(h) \
+	((h)->curr_dev_pwr_mode = UFS_POWERDOWN_PWR_MODE)
+#define ufshcd_is_ufs_dev_active(h) \
+	((h)->curr_dev_pwr_mode == UFS_ACTIVE_PWR_MODE)
+#define ufshcd_is_ufs_dev_sleep(h) \
+	((h)->curr_dev_pwr_mode == UFS_SLEEP_PWR_MODE)
+#define ufshcd_is_ufs_dev_poweroff(h) \
+	((h)->curr_dev_pwr_mode == UFS_POWERDOWN_PWR_MODE)
+
 /*
  * UFS Power management levels.
  * Each level is in increasing order of power savings.
@@ -1091,6 +1104,7 @@  static inline void ufshcd_vops_device_reset(struct ufs_hba *hba)
 {
 	if (hba->vops && hba->vops->device_reset) {
 		hba->vops->device_reset(hba);
+		ufshcd_set_ufs_dev_active(hba);
 		ufshcd_update_reg_hist(&hba->ufs_stats.dev_reset, 0);
 	}
 }