diff mbox series

[v5,6/8] scsi: ufs: Add dev ref clock gating wait time support

Message ID 1580721472-10784-7-git-send-email-cang@codeaurora.org (mailing list archive)
State Superseded
Headers show
Series UFS driver general fixes bundle 4 | expand

Commit Message

Can Guo Feb. 3, 2020, 9:17 a.m. UTC 
In UFS version 3.0, a newly added attribute bRefClkGatingWaitTime defines
the minimum time for which the reference clock is required by device during
transition to LS-MODE or HIBERN8 state. Make this change to reflect the new
requirement by adding delays before turning off the clock.

Signed-off-by: Can Guo <cang@codeaurora.org>
Reviewed-by: Asutosh Das <asutoshd@codeaurora.org>
---
 drivers/scsi/ufs/ufs.h    |  3 +++
 drivers/scsi/ufs/ufshcd.c | 40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

Comments

Bean Huo Feb. 4, 2020, 3:26 p.m. UTC | #1 
Hi, Can
> 
> In UFS version 3.0, a newly added attribute bRefClkGatingWaitTime defines the
> minimum time for which the reference clock is required by device during
> transition to LS-MODE or HIBERN8 state. Make this change to reflect the new
> requirement by adding delays before turning off the clock.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
> Reviewed-by: Asutosh Das <asutoshd@codeaurora.org>

This looks fine.

Reviewed-by: Bean Huo <beanhuo@micron.com>

Thanks, 

//Bean
Stanley Chu Feb. 5, 2020, 2:50 a.m. UTC | #2 
Hi Can,

On Mon, 2020-02-03 at 01:17 -0800, Can Guo wrote:
> In UFS version 3.0, a newly added attribute bRefClkGatingWaitTime defines
> the minimum time for which the reference clock is required by device during
> transition to LS-MODE or HIBERN8 state. Make this change to reflect the new
> requirement by adding delays before turning off the clock.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
> Reviewed-by: Asutosh Das <asutoshd@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufs.h    |  3 +++
>  drivers/scsi/ufs/ufshcd.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 43 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index cfe3803..304076e 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -167,6 +167,7 @@ enum attr_idn {
>  	QUERY_ATTR_IDN_FFU_STATUS		= 0x14,
>  	QUERY_ATTR_IDN_PSA_STATE		= 0x15,
>  	QUERY_ATTR_IDN_PSA_DATA_SIZE		= 0x16,
> +	QUERY_ATTR_IDN_REF_CLK_GATING_WAIT_TIME	= 0x17,
>  };
>  
>  /* Descriptor idn for Query requests */
> @@ -534,6 +535,8 @@ struct ufs_dev_info {
>  	u16 wmanufacturerid;
>  	/*UFS device Product Name */
>  	u8 *model;
> +	u16 spec_version;
> +	u32 clk_gating_wait_us;
>  };
>  
>  /**
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index e8f7f9d..d5c547b 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -91,6 +91,9 @@
>  /* default delay of autosuspend: 2000 ms */
>  #define RPM_AUTOSUSPEND_DELAY_MS 2000
>  
> +/* Default value of wait time before gating device ref clock */
> +#define UFSHCD_REF_CLK_GATING_WAIT_US 0xFF /* microsecs */
> +
>  #define ufshcd_toggle_vreg(_dev, _vreg, _on)				\
>  	({                                                              \
>  		int _ret;                                               \
> @@ -3281,6 +3284,37 @@ static inline int ufshcd_read_unit_desc_param(struct ufs_hba *hba,
>  				      param_offset, param_read_buf, param_size);
>  }
>  
> +static int ufshcd_get_ref_clk_gating_wait(struct ufs_hba *hba)
> +{
> +	int err = 0;
> +	u32 gating_wait = UFSHCD_REF_CLK_GATING_WAIT_US;
> +
> +	if (hba->dev_info.spec_version >= 0x300) {
> +		err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
> +				QUERY_ATTR_IDN_REF_CLK_GATING_WAIT_TIME, 0, 0,
> +				&gating_wait);
> +		if (err)
> +			dev_err(hba->dev, "Failed reading bRefClkGatingWait. err = %d, use default %uus\n",
> +					 err, gating_wait);
> +
> +		if (gating_wait == 0) {
> +			gating_wait = UFSHCD_REF_CLK_GATING_WAIT_US;
> +			dev_err(hba->dev, "Undefined ref clk gating wait time, use default %uus\n",
> +					 gating_wait);
> +		}
> +
> +		/*
> +		 * bRefClkGatingWaitTime defines the minimum time for which the
> +		 * reference clock is required by device during transition from
> +		 * HS-MODE to LS-MODE or HIBERN8 state. Give it more time to be
> +		 * on the safe side.
> +		 */
> +		hba->dev_info.clk_gating_wait_us = gating_wait + 50;


Not sure if the additional 50us wait time here is too large.

Is there any special reason to fix it as "50"?


Thanks,
Stanley

>  				      &dev_info->model, SD_ASCII_STD);
> @@ -7003,6 +7041,8 @@ static int ufshcd_device_params_init(struct ufs_hba *hba)
>  		goto out;
>  	}
>  
> +	ufshcd_get_ref_clk_gating_wait(hba);
> +
>  	ufs_fixup_device_setup(hba);
>  
>  	if (!ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_READ_FLAG,
Can Guo Feb. 5, 2020, 4:52 a.m. UTC | #3 
On 2020-02-05 10:50, Stanley Chu wrote:
> Hi Can,
> 
> On Mon, 2020-02-03 at 01:17 -0800, Can Guo wrote:
>> In UFS version 3.0, a newly added attribute bRefClkGatingWaitTime 
>> defines
>> the minimum time for which the reference clock is required by device 
>> during
>> transition to LS-MODE or HIBERN8 state. Make this change to reflect 
>> the new
>> requirement by adding delays before turning off the clock.
>> 
>> Signed-off-by: Can Guo <cang@codeaurora.org>
>> Reviewed-by: Asutosh Das <asutoshd@codeaurora.org>
>> ---
>>  drivers/scsi/ufs/ufs.h    |  3 +++
>>  drivers/scsi/ufs/ufshcd.c | 40 
>> ++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 43 insertions(+)
>> 
>> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
>> index cfe3803..304076e 100644
>> --- a/drivers/scsi/ufs/ufs.h
>> +++ b/drivers/scsi/ufs/ufs.h
>> @@ -167,6 +167,7 @@ enum attr_idn {
>>  	QUERY_ATTR_IDN_FFU_STATUS		= 0x14,
>>  	QUERY_ATTR_IDN_PSA_STATE		= 0x15,
>>  	QUERY_ATTR_IDN_PSA_DATA_SIZE		= 0x16,
>> +	QUERY_ATTR_IDN_REF_CLK_GATING_WAIT_TIME	= 0x17,
>>  };
>> 
>>  /* Descriptor idn for Query requests */
>> @@ -534,6 +535,8 @@ struct ufs_dev_info {
>>  	u16 wmanufacturerid;
>>  	/*UFS device Product Name */
>>  	u8 *model;
>> +	u16 spec_version;
>> +	u32 clk_gating_wait_us;
>>  };
>> 
>>  /**
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index e8f7f9d..d5c547b 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -91,6 +91,9 @@
>>  /* default delay of autosuspend: 2000 ms */
>>  #define RPM_AUTOSUSPEND_DELAY_MS 2000
>> 
>> +/* Default value of wait time before gating device ref clock */
>> +#define UFSHCD_REF_CLK_GATING_WAIT_US 0xFF /* microsecs */
>> +
>>  #define ufshcd_toggle_vreg(_dev, _vreg, _on)				\
>>  	({                                                              \
>>  		int _ret;                                               \
>> @@ -3281,6 +3284,37 @@ static inline int 
>> ufshcd_read_unit_desc_param(struct ufs_hba *hba,
>>  				      param_offset, param_read_buf, param_size);
>>  }
>> 
>> +static int ufshcd_get_ref_clk_gating_wait(struct ufs_hba *hba)
>> +{
>> +	int err = 0;
>> +	u32 gating_wait = UFSHCD_REF_CLK_GATING_WAIT_US;
>> +
>> +	if (hba->dev_info.spec_version >= 0x300) {
>> +		err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
>> +				QUERY_ATTR_IDN_REF_CLK_GATING_WAIT_TIME, 0, 0,
>> +				&gating_wait);
>> +		if (err)
>> +			dev_err(hba->dev, "Failed reading bRefClkGatingWait. err = %d, use 
>> default %uus\n",
>> +					 err, gating_wait);
>> +
>> +		if (gating_wait == 0) {
>> +			gating_wait = UFSHCD_REF_CLK_GATING_WAIT_US;
>> +			dev_err(hba->dev, "Undefined ref clk gating wait time, use default 
>> %uus\n",
>> +					 gating_wait);
>> +		}
>> +
>> +		/*
>> +		 * bRefClkGatingWaitTime defines the minimum time for which the
>> +		 * reference clock is required by device during transition from
>> +		 * HS-MODE to LS-MODE or HIBERN8 state. Give it more time to be
>> +		 * on the safe side.
>> +		 */
>> +		hba->dev_info.clk_gating_wait_us = gating_wait + 50;
> 
> 
> Not sure if the additional 50us wait time here is too large.
> 
> Is there any special reason to fix it as "50"?
> 
> 
> Thanks,
> Stanley
> 

Hi Stanley,

We used to ask vendors about it, 50 is somehow agreed by them. Do you 
have a
better value in mind?

For me, I just wanted to give it 10, so that we can directly use 
usleep_range
with it, no need to decide whether to use udelay or usleep_range.

Thanks,
Can Guo.

>>  				      &dev_info->model, SD_ASCII_STD);
>> @@ -7003,6 +7041,8 @@ static int ufshcd_device_params_init(struct 
>> ufs_hba *hba)
>>  		goto out;
>>  	}
>> 
>> +	ufshcd_get_ref_clk_gating_wait(hba);
>> +
>>  	ufs_fixup_device_setup(hba);
>> 
>>  	if (!ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_READ_FLAG,
Stanley Chu Feb. 6, 2020, 12:55 a.m. UTC | #4 
Hi Can,

On Wed, 2020-02-05 at 12:52 +0800, Can Guo wrote:


> Hi Stanley,
> 
> We used to ask vendors about it, 50 is somehow agreed by them. Do you 
> have a
> better value in mind?
> 
> For me, I just wanted to give it 10, so that we can directly use 
> usleep_range
> with it, no need to decide whether to use udelay or usleep_range.

Actually I do not have any value in mind because I guess the 50us here
is just a margin time added for safety as your comments: "Give it more
time to be on the safe side".

An example case is that some vendors only specify 1us in
bRefClkGatingWaitTime, so this 50us may be too long compared to device's
requirement. If such device really needs this additional 50us, it shall
be specified in bRefClkGatingWaitTime.

So if this additional delay does not have any special reason or not
mentioned by UFS specification, would you consider move it to vendor
specific implementations. By this way, it would be more flexible to be
controlled by vendors or by platforms.

Thanks,
Stanley

> 
> Thanks,
> Can Guo.
> 
> >>  				      &dev_info->model, SD_ASCII_STD);
Can Guo Feb. 6, 2020, 2:39 a.m. UTC | #5 
On 2020-02-06 08:55, Stanley Chu wrote:
> Hi Can,
> 
> On Wed, 2020-02-05 at 12:52 +0800, Can Guo wrote:
> 
> 
>> Hi Stanley,
>> 
>> We used to ask vendors about it, 50 is somehow agreed by them. Do you
>> have a
>> better value in mind?
>> 
>> For me, I just wanted to give it 10, so that we can directly use
>> usleep_range
>> with it, no need to decide whether to use udelay or usleep_range.
> 
> Actually I do not have any value in mind because I guess the 50us here
> is just a margin time added for safety as your comments: "Give it more
> time to be on the safe side".
> 
> An example case is that some vendors only specify 1us in
> bRefClkGatingWaitTime, so this 50us may be too long compared to 
> device's
> requirement. If such device really needs this additional 50us, it shall
> be specified in bRefClkGatingWaitTime.
> 
> So if this additional delay does not have any special reason or not
> mentioned by UFS specification, would you consider move it to vendor
> specific implementations. By this way, it would be more flexible to be
> controlled by vendors or by platforms.
> 
> Thanks,
> Stanley
> 
>> 
>> Thanks,
>> Can Guo.
>> 
>> >>  				      &dev_info->model, SD_ASCII_STD);

Hi Stanley,

FYI, the default values in bRefClkGatingWaitTime from vendors are around
50 - 100.

I agree with you. I will just remove the extra delay here and let's
handle it in our own platform drivers.

Thanks,
Can Guo.
diff mbox series

Patch

diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index cfe3803..304076e 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -167,6 +167,7 @@  enum attr_idn {
 	QUERY_ATTR_IDN_FFU_STATUS		= 0x14,
 	QUERY_ATTR_IDN_PSA_STATE		= 0x15,
 	QUERY_ATTR_IDN_PSA_DATA_SIZE		= 0x16,
+	QUERY_ATTR_IDN_REF_CLK_GATING_WAIT_TIME	= 0x17,
 };
 
 /* Descriptor idn for Query requests */
@@ -534,6 +535,8 @@  struct ufs_dev_info {
 	u16 wmanufacturerid;
 	/*UFS device Product Name */
 	u8 *model;
+	u16 spec_version;
+	u32 clk_gating_wait_us;
 };
 
 /**
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index e8f7f9d..d5c547b 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -91,6 +91,9 @@ 
 /* default delay of autosuspend: 2000 ms */
 #define RPM_AUTOSUSPEND_DELAY_MS 2000
 
+/* Default value of wait time before gating device ref clock */
+#define UFSHCD_REF_CLK_GATING_WAIT_US 0xFF /* microsecs */
+
 #define ufshcd_toggle_vreg(_dev, _vreg, _on)				\
 	({                                                              \
 		int _ret;                                               \
@@ -3281,6 +3284,37 @@  static inline int ufshcd_read_unit_desc_param(struct ufs_hba *hba,
 				      param_offset, param_read_buf, param_size);
 }
 
+static int ufshcd_get_ref_clk_gating_wait(struct ufs_hba *hba)
+{
+	int err = 0;
+	u32 gating_wait = UFSHCD_REF_CLK_GATING_WAIT_US;
+
+	if (hba->dev_info.spec_version >= 0x300) {
+		err = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
+				QUERY_ATTR_IDN_REF_CLK_GATING_WAIT_TIME, 0, 0,
+				&gating_wait);
+		if (err)
+			dev_err(hba->dev, "Failed reading bRefClkGatingWait. err = %d, use default %uus\n",
+					 err, gating_wait);
+
+		if (gating_wait == 0) {
+			gating_wait = UFSHCD_REF_CLK_GATING_WAIT_US;
+			dev_err(hba->dev, "Undefined ref clk gating wait time, use default %uus\n",
+					 gating_wait);
+		}
+
+		/*
+		 * bRefClkGatingWaitTime defines the minimum time for which the
+		 * reference clock is required by device during transition from
+		 * HS-MODE to LS-MODE or HIBERN8 state. Give it more time to be
+		 * on the safe side.
+		 */
+		hba->dev_info.clk_gating_wait_us = gating_wait + 50;
+	}
+
+	return err;
+}
+
 /**
  * ufshcd_memory_alloc - allocate memory for host memory space data structures
  * @hba: per adapter instance
@@ -6626,6 +6660,10 @@  static int ufs_get_device_desc(struct ufs_hba *hba)
 	dev_info->wmanufacturerid = desc_buf[DEVICE_DESC_PARAM_MANF_ID] << 8 |
 				     desc_buf[DEVICE_DESC_PARAM_MANF_ID + 1];
 
+	/* getting Specification Version in big endian format */
+	dev_info->spec_version = desc_buf[DEVICE_DESC_PARAM_SPEC_VER] << 8 |
+				      desc_buf[DEVICE_DESC_PARAM_SPEC_VER + 1];
+
 	model_index = desc_buf[DEVICE_DESC_PARAM_PRDCT_NAME];
 	err = ufshcd_read_string_desc(hba, model_index,
 				      &dev_info->model, SD_ASCII_STD);
@@ -7003,6 +7041,8 @@  static int ufshcd_device_params_init(struct ufs_hba *hba)
 		goto out;
 	}
 
+	ufshcd_get_ref_clk_gating_wait(hba);
+
 	ufs_fixup_device_setup(hba);
 
 	if (!ufshcd_query_flag_retry(hba, UPIU_QUERY_OPCODE_READ_FLAG,