linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v7 4/8] scsi: ufs-qcom: Adjust bus bandwidth voting and unvoting
       [not found] <1580978008-9327-1-git-send-email-cang@codeaurora.org>
@ 2020-02-06  8:33 ` Can Guo
  2020-02-06  8:33 ` [PATCH v7 7/8] scsi: ufs-qcom: Delay specific time before gate ref clk Can Guo
  1 sibling, 0 replies; 6+ messages in thread
From: Can Guo @ 2020-02-06  8:33 UTC (permalink / raw)
  To: asutoshd, nguyenb, hongwus, rnayak, linux-scsi, kernel-team,
	saravanak, salyzyn, cang
  Cc: Andy Gross, Bjorn Andersson, Alim Akhtar, Avri Altman,
	James E.J. Bottomley, Martin K. Petersen,
	open list:ARM/QUALCOMM SUPPORT, open list

The bus bandwidth voting is required to be done before the bus clocks
are enabled, and the unvoting is required to be done only after the bus
clocks are disabled.

Signed-off-by: Can Guo <cang@codeaurora.org>
Reviewed-by: Asutosh Das <asutoshd@codeaurora.org>
---
 drivers/scsi/ufs/ufs-qcom.c | 57 +++++++++++++++++++++++++++++++--------------
 1 file changed, 39 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
index c69c29a1c..85d7c17 100644
--- a/drivers/scsi/ufs/ufs-qcom.c
+++ b/drivers/scsi/ufs/ufs-qcom.c
@@ -38,7 +38,6 @@ enum {
 
 static struct ufs_qcom_host *ufs_qcom_hosts[MAX_UFS_QCOM_HOSTS];
 
-static int ufs_qcom_set_bus_vote(struct ufs_qcom_host *host, int vote);
 static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host);
 static int ufs_qcom_set_dme_vs_core_clk_ctrl_clear_div(struct ufs_hba *hba,
 						       u32 clk_cycles);
@@ -674,7 +673,7 @@ static void ufs_qcom_get_speed_mode(struct ufs_pa_layer_attr *p, char *result)
 	}
 }
 
-static int ufs_qcom_set_bus_vote(struct ufs_qcom_host *host, int vote)
+static int __ufs_qcom_set_bus_vote(struct ufs_qcom_host *host, int vote)
 {
 	int err = 0;
 
@@ -705,7 +704,7 @@ static int ufs_qcom_update_bus_bw_vote(struct ufs_qcom_host *host)
 
 	vote = ufs_qcom_get_bus_vote(host, mode);
 	if (vote >= 0)
-		err = ufs_qcom_set_bus_vote(host, vote);
+		err = __ufs_qcom_set_bus_vote(host, vote);
 	else
 		err = vote;
 
@@ -716,6 +715,35 @@ static int ufs_qcom_update_bus_bw_vote(struct ufs_qcom_host *host)
 	return err;
 }
 
+static int ufs_qcom_set_bus_vote(struct ufs_hba *hba, bool on)
+{
+	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
+	int vote, err;
+
+	/*
+	 * In case ufs_qcom_init() is not yet done, simply ignore.
+	 * This ufs_qcom_set_bus_vote() shall be called from
+	 * ufs_qcom_init() after init is done.
+	 */
+	if (!host)
+		return 0;
+
+	if (on) {
+		vote = host->bus_vote.saved_vote;
+		if (vote == host->bus_vote.min_bw_vote)
+			ufs_qcom_update_bus_bw_vote(host);
+	} else {
+		vote = host->bus_vote.min_bw_vote;
+	}
+
+	err = __ufs_qcom_set_bus_vote(host, vote);
+	if (err)
+		dev_err(hba->dev, "%s: set bus vote failed %d\n",
+				 __func__, err);
+
+	return err;
+}
+
 static ssize_t
 show_ufs_to_mem_max_bus_bw(struct device *dev, struct device_attribute *attr,
 			char *buf)
@@ -792,7 +820,7 @@ static int ufs_qcom_update_bus_bw_vote(struct ufs_qcom_host *host)
 	return 0;
 }
 
-static int ufs_qcom_set_bus_vote(struct ufs_qcom_host *host, int vote)
+static int ufs_qcom_set_bus_vote(struct ufs_hba *host, bool on)
 {
 	return 0;
 }
@@ -1030,8 +1058,7 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
 				 enum ufs_notify_change_status status)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
-	int err;
-	int vote = 0;
+	int err = 0;
 
 	/*
 	 * In case ufs_qcom_init() is not yet done, simply ignore.
@@ -1041,28 +1068,21 @@ static int ufs_qcom_setup_clocks(struct ufs_hba *hba, bool on,
 	if (!host)
 		return 0;
 
-	if (on && (status == POST_CHANGE)) {
+	if (on && (status == PRE_CHANGE)) {
+		err = ufs_qcom_set_bus_vote(hba, true);
+	} else if (on && (status == POST_CHANGE)) {
 		/* enable the device ref clock for HS mode*/
 		if (ufshcd_is_hs_mode(&hba->pwr_info))
 			ufs_qcom_dev_ref_clk_ctrl(host, true);
-		vote = host->bus_vote.saved_vote;
-		if (vote == host->bus_vote.min_bw_vote)
-			ufs_qcom_update_bus_bw_vote(host);
-
 	} else if (!on && (status == PRE_CHANGE)) {
 		if (!ufs_qcom_is_link_active(hba)) {
 			/* disable device ref_clk */
 			ufs_qcom_dev_ref_clk_ctrl(host, false);
 		}
-
-		vote = host->bus_vote.min_bw_vote;
+	} else if (!on && (status == POST_CHANGE)) {
+		err = ufs_qcom_set_bus_vote(hba, false);
 	}
 
-	err = ufs_qcom_set_bus_vote(host, vote);
-	if (err)
-		dev_err(hba->dev, "%s: set bus vote failed %d\n",
-				__func__, err);
-
 	return err;
 }
 
@@ -1238,6 +1258,7 @@ static int ufs_qcom_init(struct ufs_hba *hba)
 	ufs_qcom_set_caps(hba);
 	ufs_qcom_advertise_quirks(hba);
 
+	ufs_qcom_set_bus_vote(hba, true);
 	ufs_qcom_setup_clocks(hba, true, POST_CHANGE);
 
 	if (hba->dev->id < MAX_UFS_QCOM_HOSTS)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v7 7/8] scsi: ufs-qcom: Delay specific time before gate ref clk
       [not found] <1580978008-9327-1-git-send-email-cang@codeaurora.org>
  2020-02-06  8:33 ` [PATCH v7 4/8] scsi: ufs-qcom: Adjust bus bandwidth voting and unvoting Can Guo
@ 2020-02-06  8:33 ` Can Guo
  2020-02-06 20:33   ` Bjorn Andersson
  1 sibling, 1 reply; 6+ messages in thread
From: Can Guo @ 2020-02-06  8:33 UTC (permalink / raw)
  To: asutoshd, nguyenb, hongwus, rnayak, linux-scsi, kernel-team,
	saravanak, salyzyn, cang
  Cc: Andy Gross, Bjorn Andersson, Alim Akhtar, Avri Altman,
	James E.J. Bottomley, Martin K. Petersen,
	open list:ARM/QUALCOMM SUPPORT, open list

After enter hibern8, as UFS JEDEC ver 3.0 requires, a specific gating wait
time is required before disable the device reference clock. If it is not
specified, use the old delay.

Signed-off-by: Can Guo <cang@codeaurora.org>
Reviewed-by: Asutosh Das <asutoshd@codeaurora.org>
Reviewed-by: Hongwu Su <hongwus@codeaurora.org>
---
 drivers/scsi/ufs/ufs-qcom.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
index 85d7c17..39eefa4 100644
--- a/drivers/scsi/ufs/ufs-qcom.c
+++ b/drivers/scsi/ufs/ufs-qcom.c
@@ -833,6 +833,8 @@ static int ufs_qcom_bus_register(struct ufs_qcom_host *host)
 
 static void ufs_qcom_dev_ref_clk_ctrl(struct ufs_qcom_host *host, bool enable)
 {
+	unsigned long gating_wait;
+
 	if (host->dev_ref_clk_ctrl_mmio &&
 	    (enable ^ host->is_dev_ref_clk_enabled)) {
 		u32 temp = readl_relaxed(host->dev_ref_clk_ctrl_mmio);
@@ -845,11 +847,25 @@ static void ufs_qcom_dev_ref_clk_ctrl(struct ufs_qcom_host *host, bool enable)
 		/*
 		 * If we are here to disable this clock it might be immediately
 		 * after entering into hibern8 in which case we need to make
-		 * sure that device ref_clk is active at least 1us after the
+		 * sure that device ref_clk is active for specific time after
 		 * hibern8 enter.
 		 */
-		if (!enable)
-			udelay(1);
+		if (!enable) {
+			gating_wait = host->hba->dev_info.clk_gating_wait_us;
+			if (!gating_wait) {
+				udelay(1);
+			} else {
+				/*
+				 * 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.
+				 */
+				gating_wait += 10;
+				usleep_range(gating_wait, gating_wait + 10);
+			}
+		}
 
 		writel_relaxed(temp, host->dev_ref_clk_ctrl_mmio);
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v7 7/8] scsi: ufs-qcom: Delay specific time before gate ref clk
  2020-02-06  8:33 ` [PATCH v7 7/8] scsi: ufs-qcom: Delay specific time before gate ref clk Can Guo
@ 2020-02-06 20:33   ` Bjorn Andersson
  2020-02-07  1:09     ` Can Guo
  0 siblings, 1 reply; 6+ messages in thread
From: Bjorn Andersson @ 2020-02-06 20:33 UTC (permalink / raw)
  To: Can Guo
  Cc: asutoshd, nguyenb, hongwus, rnayak, linux-scsi, kernel-team,
	saravanak, salyzyn, Andy Gross, Alim Akhtar, Avri Altman,
	James E.J. Bottomley, Martin K. Petersen,
	open list:ARM/QUALCOMM SUPPORT, open list

On Thu 06 Feb 00:33 PST 2020, Can Guo wrote:

> After enter hibern8, as UFS JEDEC ver 3.0 requires, a specific gating wait
> time is required before disable the device reference clock. If it is not
> specified, use the old delay.
> 
> Signed-off-by: Can Guo <cang@codeaurora.org>
> Reviewed-by: Asutosh Das <asutoshd@codeaurora.org>
> Reviewed-by: Hongwu Su <hongwus@codeaurora.org>
> ---
>  drivers/scsi/ufs/ufs-qcom.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
> index 85d7c17..39eefa4 100644
> --- a/drivers/scsi/ufs/ufs-qcom.c
> +++ b/drivers/scsi/ufs/ufs-qcom.c
> @@ -833,6 +833,8 @@ static int ufs_qcom_bus_register(struct ufs_qcom_host *host)
>  
>  static void ufs_qcom_dev_ref_clk_ctrl(struct ufs_qcom_host *host, bool enable)
>  {
> +	unsigned long gating_wait;
> +
>  	if (host->dev_ref_clk_ctrl_mmio &&
>  	    (enable ^ host->is_dev_ref_clk_enabled)) {
>  		u32 temp = readl_relaxed(host->dev_ref_clk_ctrl_mmio);
> @@ -845,11 +847,25 @@ static void ufs_qcom_dev_ref_clk_ctrl(struct ufs_qcom_host *host, bool enable)
>  		/*
>  		 * If we are here to disable this clock it might be immediately
>  		 * after entering into hibern8 in which case we need to make
> -		 * sure that device ref_clk is active at least 1us after the
> +		 * sure that device ref_clk is active for specific time after
>  		 * hibern8 enter.
>  		 */
> -		if (!enable)
> -			udelay(1);
> +		if (!enable) {
> +			gating_wait = host->hba->dev_info.clk_gating_wait_us;
> +			if (!gating_wait) {

Afaict this can't happen, because in patch 6 you check for gating_wait
being 0 and if so set it to 0xff.

> +				udelay(1);
> +			} else {
> +				/*
> +				 * 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.
> +				 */
> +				gating_wait += 10;
> +				usleep_range(gating_wait, gating_wait + 10);

I presume there's no strong requirement on the max, so how about using a
substantially larger max - say 1k, or 10k - to allow the usleep_range()
to do it's job?


PS. Please include linux-arm-msm@ on all the patches in the series, not
just two of them.

Regards,
Bjorn

> +			}
> +		}
>  
>  		writel_relaxed(temp, host->dev_ref_clk_ctrl_mmio);
>  
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v7 7/8] scsi: ufs-qcom: Delay specific time before gate ref clk
  2020-02-06 20:33   ` Bjorn Andersson
@ 2020-02-07  1:09     ` Can Guo
  2020-02-07  2:10       ` Bjorn Andersson
  0 siblings, 1 reply; 6+ messages in thread
From: Can Guo @ 2020-02-07  1:09 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: asutoshd, nguyenb, hongwus, rnayak, linux-scsi, kernel-team,
	saravanak, salyzyn, Andy Gross, Alim Akhtar, Avri Altman,
	James E.J. Bottomley, Martin K. Petersen,
	open list:ARM/QUALCOMM SUPPORT, open list

On 2020-02-07 04:33, Bjorn Andersson wrote:
> On Thu 06 Feb 00:33 PST 2020, Can Guo wrote:
> 
>> After enter hibern8, as UFS JEDEC ver 3.0 requires, a specific gating 
>> wait
>> time is required before disable the device reference clock. If it is 
>> not
>> specified, use the old delay.
>> 
>> Signed-off-by: Can Guo <cang@codeaurora.org>
>> Reviewed-by: Asutosh Das <asutoshd@codeaurora.org>
>> Reviewed-by: Hongwu Su <hongwus@codeaurora.org>
>> ---
>>  drivers/scsi/ufs/ufs-qcom.c | 22 +++++++++++++++++++---
>>  1 file changed, 19 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
>> index 85d7c17..39eefa4 100644
>> --- a/drivers/scsi/ufs/ufs-qcom.c
>> +++ b/drivers/scsi/ufs/ufs-qcom.c
>> @@ -833,6 +833,8 @@ static int ufs_qcom_bus_register(struct 
>> ufs_qcom_host *host)
>> 
>>  static void ufs_qcom_dev_ref_clk_ctrl(struct ufs_qcom_host *host, 
>> bool enable)
>>  {
>> +	unsigned long gating_wait;
>> +
>>  	if (host->dev_ref_clk_ctrl_mmio &&
>>  	    (enable ^ host->is_dev_ref_clk_enabled)) {
>>  		u32 temp = readl_relaxed(host->dev_ref_clk_ctrl_mmio);
>> @@ -845,11 +847,25 @@ static void ufs_qcom_dev_ref_clk_ctrl(struct 
>> ufs_qcom_host *host, bool enable)
>>  		/*
>>  		 * If we are here to disable this clock it might be immediately
>>  		 * after entering into hibern8 in which case we need to make
>> -		 * sure that device ref_clk is active at least 1us after the
>> +		 * sure that device ref_clk is active for specific time after
>>  		 * hibern8 enter.
>>  		 */
>> -		if (!enable)
>> -			udelay(1);
>> +		if (!enable) {
>> +			gating_wait = host->hba->dev_info.clk_gating_wait_us;
>> +			if (!gating_wait) {
> 
> Afaict this can't happen, because in patch 6 you check for gating_wait
> being 0 and if so set it to 0xff.
> 

Sorry, I was intended to give clk_gating_wait_us values only if it is
a UFS3.0 device. I will revise patch 6/8.

>> +				udelay(1);
>> +			} else {
>> +				/*
>> +				 * 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.
>> +				 */
>> +				gating_wait += 10;
>> +				usleep_range(gating_wait, gating_wait + 10);
> 
> I presume there's no strong requirement on the max, so how about using 
> a
> substantially larger max - say 1k, or 10k - to allow the usleep_range()
> to do it's job?
> 
> 
> PS. Please include linux-arm-msm@ on all the patches in the series, not
> just two of them.
> 
> Regards,
> Bjorn
> 

bRefClkGatingWaitTime, as vendor defined in their device attribute is 
usually
around 50~100, 1k or 10k delay makes it too large. usleep_range() works 
well
so long as the delay is within (10us - 20ms), so I added 10 to make sure 
it is
above 10us.

SLEEPING FOR ~USECS OR SMALL MSECS ( 10us - 20ms):
	* Use usleep_range
https://www.kernel.org/doc/Documentation/timers/timers-howto.txt

Thanks,

Can Guo.

>> +			}
>> +		}
>> 
>>  		writel_relaxed(temp, host->dev_ref_clk_ctrl_mmio);
>> 
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v7 7/8] scsi: ufs-qcom: Delay specific time before gate ref clk
  2020-02-07  1:09     ` Can Guo
@ 2020-02-07  2:10       ` Bjorn Andersson
  2020-02-08  0:10         ` Can Guo
  0 siblings, 1 reply; 6+ messages in thread
From: Bjorn Andersson @ 2020-02-07  2:10 UTC (permalink / raw)
  To: Can Guo
  Cc: asutoshd, nguyenb, hongwus, rnayak, linux-scsi, kernel-team,
	saravanak, salyzyn, Andy Gross, Alim Akhtar, Avri Altman,
	James E.J. Bottomley, Martin K. Petersen,
	open list:ARM/QUALCOMM SUPPORT, open list

On Thu 06 Feb 17:09 PST 2020, Can Guo wrote:

> On 2020-02-07 04:33, Bjorn Andersson wrote:
> > On Thu 06 Feb 00:33 PST 2020, Can Guo wrote:
> > 
> > > After enter hibern8, as UFS JEDEC ver 3.0 requires, a specific
> > > gating wait
> > > time is required before disable the device reference clock. If it is
> > > not
> > > specified, use the old delay.
> > > 
> > > Signed-off-by: Can Guo <cang@codeaurora.org>
> > > Reviewed-by: Asutosh Das <asutoshd@codeaurora.org>
> > > Reviewed-by: Hongwu Su <hongwus@codeaurora.org>
> > > ---
> > >  drivers/scsi/ufs/ufs-qcom.c | 22 +++++++++++++++++++---
> > >  1 file changed, 19 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
> > > index 85d7c17..39eefa4 100644
> > > --- a/drivers/scsi/ufs/ufs-qcom.c
> > > +++ b/drivers/scsi/ufs/ufs-qcom.c
> > > @@ -833,6 +833,8 @@ static int ufs_qcom_bus_register(struct
> > > ufs_qcom_host *host)
> > > 
> > >  static void ufs_qcom_dev_ref_clk_ctrl(struct ufs_qcom_host *host,
> > > bool enable)
> > >  {
> > > +	unsigned long gating_wait;
> > > +
> > >  	if (host->dev_ref_clk_ctrl_mmio &&
> > >  	    (enable ^ host->is_dev_ref_clk_enabled)) {
> > >  		u32 temp = readl_relaxed(host->dev_ref_clk_ctrl_mmio);
> > > @@ -845,11 +847,25 @@ static void ufs_qcom_dev_ref_clk_ctrl(struct
> > > ufs_qcom_host *host, bool enable)
> > >  		/*
> > >  		 * If we are here to disable this clock it might be immediately
> > >  		 * after entering into hibern8 in which case we need to make
> > > -		 * sure that device ref_clk is active at least 1us after the
> > > +		 * sure that device ref_clk is active for specific time after
> > >  		 * hibern8 enter.
> > >  		 */
> > > -		if (!enable)
> > > -			udelay(1);
> > > +		if (!enable) {
> > > +			gating_wait = host->hba->dev_info.clk_gating_wait_us;
> > > +			if (!gating_wait) {
> > 
> > Afaict this can't happen, because in patch 6 you check for gating_wait
> > being 0 and if so set it to 0xff.
> > 
> 
> Sorry, I was intended to give clk_gating_wait_us values only if it is
> a UFS3.0 device. I will revise patch 6/8.
> 

Okay, sounds good.

> > > +				udelay(1);
> > > +			} else {
> > > +				/*
> > > +				 * 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.
> > > +				 */
> > > +				gating_wait += 10;
> > > +				usleep_range(gating_wait, gating_wait + 10);
> > 
> > I presume there's no strong requirement on the max, so how about using a
> > substantially larger max - say 1k, or 10k - to allow the usleep_range()
> > to do it's job?
> > 
> > 
> > PS. Please include linux-arm-msm@ on all the patches in the series, not
> > just two of them.
> > 
> > Regards,
> > Bjorn
> > 
> 
> bRefClkGatingWaitTime, as vendor defined in their device attribute is
> usually
> around 50~100, 1k or 10k delay makes it too large. usleep_range() works well
> so long as the delay is within (10us - 20ms), so I added 10 to make sure it
> is
> above 10us.
> 

I meant specifically the second parameter, i.e:
  usleep_range(bRefClkGatingWaitTime + 10, bRefClkGatingWaitTime + 1000);

As you're not guaranteed an upper bound of this sleep anyway you might
as well give usleep_range() a window of a millisecond (or more) to give
it the flexibility of matching other timer events.

The only drawback with this is that you might "waste" a millisecond.

Regards,
Bjorn

> SLEEPING FOR ~USECS OR SMALL MSECS ( 10us - 20ms):
> 	* Use usleep_range
> https://www.kernel.org/doc/Documentation/timers/timers-howto.txt
> 
> Thanks,
> 
> Can Guo.
> 
> > > +			}
> > > +		}
> > > 
> > >  		writel_relaxed(temp, host->dev_ref_clk_ctrl_mmio);
> > > 
> > > --
> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> > > Forum,
> > > a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v7 7/8] scsi: ufs-qcom: Delay specific time before gate ref clk
  2020-02-07  2:10       ` Bjorn Andersson
@ 2020-02-08  0:10         ` Can Guo
  0 siblings, 0 replies; 6+ messages in thread
From: Can Guo @ 2020-02-08  0:10 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: asutoshd, nguyenb, hongwus, rnayak, linux-scsi, kernel-team,
	saravanak, salyzyn, Andy Gross, Alim Akhtar, Avri Altman,
	James E.J. Bottomley, Martin K. Petersen,
	open list:ARM/QUALCOMM SUPPORT, open list

On 2020-02-07 10:10, Bjorn Andersson wrote:
> On Thu 06 Feb 17:09 PST 2020, Can Guo wrote:
> 
>> On 2020-02-07 04:33, Bjorn Andersson wrote:
>> > On Thu 06 Feb 00:33 PST 2020, Can Guo wrote:
>> >
>> > > After enter hibern8, as UFS JEDEC ver 3.0 requires, a specific
>> > > gating wait
>> > > time is required before disable the device reference clock. If it is
>> > > not
>> > > specified, use the old delay.
>> > >
>> > > Signed-off-by: Can Guo <cang@codeaurora.org>
>> > > Reviewed-by: Asutosh Das <asutoshd@codeaurora.org>
>> > > Reviewed-by: Hongwu Su <hongwus@codeaurora.org>
>> > > ---
>> > >  drivers/scsi/ufs/ufs-qcom.c | 22 +++++++++++++++++++---
>> > >  1 file changed, 19 insertions(+), 3 deletions(-)
>> > >
>> > > diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
>> > > index 85d7c17..39eefa4 100644
>> > > --- a/drivers/scsi/ufs/ufs-qcom.c
>> > > +++ b/drivers/scsi/ufs/ufs-qcom.c
>> > > @@ -833,6 +833,8 @@ static int ufs_qcom_bus_register(struct
>> > > ufs_qcom_host *host)
>> > >
>> > >  static void ufs_qcom_dev_ref_clk_ctrl(struct ufs_qcom_host *host,
>> > > bool enable)
>> > >  {
>> > > +	unsigned long gating_wait;
>> > > +
>> > >  	if (host->dev_ref_clk_ctrl_mmio &&
>> > >  	    (enable ^ host->is_dev_ref_clk_enabled)) {
>> > >  		u32 temp = readl_relaxed(host->dev_ref_clk_ctrl_mmio);
>> > > @@ -845,11 +847,25 @@ static void ufs_qcom_dev_ref_clk_ctrl(struct
>> > > ufs_qcom_host *host, bool enable)
>> > >  		/*
>> > >  		 * If we are here to disable this clock it might be immediately
>> > >  		 * after entering into hibern8 in which case we need to make
>> > > -		 * sure that device ref_clk is active at least 1us after the
>> > > +		 * sure that device ref_clk is active for specific time after
>> > >  		 * hibern8 enter.
>> > >  		 */
>> > > -		if (!enable)
>> > > -			udelay(1);
>> > > +		if (!enable) {
>> > > +			gating_wait = host->hba->dev_info.clk_gating_wait_us;
>> > > +			if (!gating_wait) {
>> >
>> > Afaict this can't happen, because in patch 6 you check for gating_wait
>> > being 0 and if so set it to 0xff.
>> >
>> 
>> Sorry, I was intended to give clk_gating_wait_us values only if it is
>> a UFS3.0 device. I will revise patch 6/8.
>> 
> 
> Okay, sounds good.
> 
>> > > +				udelay(1);
>> > > +			} else {
>> > > +				/*
>> > > +				 * 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.
>> > > +				 */
>> > > +				gating_wait += 10;
>> > > +				usleep_range(gating_wait, gating_wait + 10);
>> >
>> > I presume there's no strong requirement on the max, so how about using a
>> > substantially larger max - say 1k, or 10k - to allow the usleep_range()
>> > to do it's job?
>> >
>> >
>> > PS. Please include linux-arm-msm@ on all the patches in the series, not
>> > just two of them.
>> >
>> > Regards,
>> > Bjorn
>> >
>> 
>> bRefClkGatingWaitTime, as vendor defined in their device attribute is
>> usually
>> around 50~100, 1k or 10k delay makes it too large. usleep_range() 
>> works well
>> so long as the delay is within (10us - 20ms), so I added 10 to make 
>> sure it
>> is
>> above 10us.
>> 
> 
> I meant specifically the second parameter, i.e:
>   usleep_range(bRefClkGatingWaitTime + 10, bRefClkGatingWaitTime + 
> 1000);
> 
> As you're not guaranteed an upper bound of this sleep anyway you might
> as well give usleep_range() a window of a millisecond (or more) to give
> it the flexibility of matching other timer events.
> 
> The only drawback with this is that you might "waste" a millisecond.
> 
> Regards,
> Bjorn
> 

Hi Bjorn,

Device ref clk gate/ungate can happen very frequently if clk gating is
enabled. The "wasted" milliseconds might be accumulated to affect
read/write latency, i.e:

If read/write requests come, clk ungate kicks start, it flushes ongoing
gate work if any, if we "waste" a millisecond here, the read/write
requests will be delayed a millisecond. I am not sure how much it may
impact the overall performance in a long term test.

Thanks,

Can Guo.

>> SLEEPING FOR ~USECS OR SMALL MSECS ( 10us - 20ms):
>> 	* Use usleep_range
>> https://www.kernel.org/doc/Documentation/timers/timers-howto.txt
>> 
>> Thanks,
>> 
>> Can Guo.
>> 
>> > > +			}
>> > > +		}
>> > >
>> > >  		writel_relaxed(temp, host->dev_ref_clk_ctrl_mmio);
>> > >
>> > > --
>> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>> > > Forum,
>> > > a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-02-08  0:11 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <1580978008-9327-1-git-send-email-cang@codeaurora.org>
2020-02-06  8:33 ` [PATCH v7 4/8] scsi: ufs-qcom: Adjust bus bandwidth voting and unvoting Can Guo
2020-02-06  8:33 ` [PATCH v7 7/8] scsi: ufs-qcom: Delay specific time before gate ref clk Can Guo
2020-02-06 20:33   ` Bjorn Andersson
2020-02-07  1:09     ` Can Guo
2020-02-07  2:10       ` Bjorn Andersson
2020-02-08  0:10         ` Can Guo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).