[v1,1/2] scsi: ufs: add required delay after gating reference clock
diff mbox series

Message ID 20200217093559.16830-2-stanley.chu@mediatek.com
State New
Headers show
Series
  • scsi: ufs: fix waiting time for reference clock
Related show

Commit Message

Stanley Chu Feb. 17, 2020, 9:35 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.

Currently this time is detected and stored in
hba->dev_info.clk_gating_wait_us but applied to vendor implementatios only.
Make it applied to reference clock named as "ref_clk" in device tree in
common path.

Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufshcd.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Can Guo Feb. 17, 2020, 12:58 p.m. UTC | #1
On 2020-02-17 17:35, Stanley Chu 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.
> 
> Currently this time is detected and stored in
> hba->dev_info.clk_gating_wait_us but applied to vendor implementatios 
> only.
> Make it applied to reference clock named as "ref_clk" in device tree in
> common path.
> 
> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 744b8254220c..7f60721f54d1 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -7417,8 +7417,10 @@ static int __ufshcd_setup_clocks(struct ufs_hba
> *hba, bool on,
>  	struct ufs_clk_info *clki;
>  	struct list_head *head = &hba->clk_list_head;
>  	unsigned long flags;
> +	unsigned long wait_us;
>  	ktime_t start = ktime_get();
>  	bool clk_state_changed = false;
> +	bool ref_clk;
> 
>  	if (list_empty(head))
>  		goto out;
> @@ -7436,7 +7438,8 @@ static int __ufshcd_setup_clocks(struct ufs_hba
> *hba, bool on,
> 
>  	list_for_each_entry(clki, head, list) {
>  		if (!IS_ERR_OR_NULL(clki->clk)) {
> -			if (skip_ref_clk && !strcmp(clki->name, "ref_clk"))
> +			ref_clk = !strcmp(clki->name, "ref_clk") ? true : false;
> +			if (skip_ref_clk && ref_clk)
>  				continue;
> 
>  			clk_state_changed = on ^ clki->enabled;
> @@ -7449,6 +7452,9 @@ static int __ufshcd_setup_clocks(struct ufs_hba
> *hba, bool on,
>  				}
>  			} else if (!on && clki->enabled) {
>  				clk_disable_unprepare(clki->clk);
> +				wait_us = hba->dev_info.clk_gating_wait_us;
> +				if (ref_clk && wait_us)
> +					usleep_range(wait_us, wait_us + 10);

Hi Stanley,

If wait_us is 1us, it would be inappropriate to use usleep_range() here.
You have checks of the delay in patch #2, but why it is not needed here?

Thanks,
Can Guo.

>  			}
>  			clki->enabled = on;
>  			dev_dbg(hba->dev, "%s: clk: %s %sabled\n", __func__,
Stanley Chu Feb. 17, 2020, 1:12 p.m. UTC | #2
Hi Can,


> >  			} else if (!on && clki->enabled) {
> >  				clk_disable_unprepare(clki->clk);
> > +				wait_us = hba->dev_info.clk_gating_wait_us;
> > +				if (ref_clk && wait_us)
> > +					usleep_range(wait_us, wait_us + 10);
> 
> Hi Stanley,
> 
> If wait_us is 1us, it would be inappropriate to use usleep_range() here.
> You have checks of the delay in patch #2, but why it is not needed here?
> 
> Thanks,
> Can Guo.

You are right. I could make that delay checking as common function so it
can be used here as well to cover all possible values.

Thanks for suggestion.
Stanley
Can Guo Feb. 17, 2020, 1:22 p.m. UTC | #3
On 2020-02-17 21:12, Stanley Chu wrote:
> Hi Can,
> 
> 
>> >  			} else if (!on && clki->enabled) {
>> >  				clk_disable_unprepare(clki->clk);
>> > +				wait_us = hba->dev_info.clk_gating_wait_us;
>> > +				if (ref_clk && wait_us)
>> > +					usleep_range(wait_us, wait_us + 10);
>> 
>> Hi St,anley,
>> 
>> If wait_us is 1us, it would be inappropriate to use usleep_range() 
>> here.
>> You have checks of the delay in patch #2, but why it is not needed 
>> here?
>> 
>> Thanks,
>> Can Guo.
> 
> You are right. I could make that delay checking as common function so 
> it
> can be used here as well to cover all possible values.
> 
> Thanks for suggestion.
> Stanley

Hi Stanley,

One more thing, as in patch #2, you have already added delays in your
ufshcd_vops_setup_clocks(OFF, PRE_CHANGE) path, plus this delay here,
don't you delay for 2*bRefClkGatingWaitTime in ufshcd_setup_clocks()?
As the delay added in your vops also delays the actions of turning
off all the other clocks in ufshcd_setup_clocks(), you don't need the
delay here again, do you agree?

Thanks,
Can Guo.
Stanley Chu Feb. 17, 2020, 1:34 p.m. UTC | #4
Hi Can,

On Mon, 2020-02-17 at 21:22 +0800, Can Guo wrote:
> On 2020-02-17 21:12, Stanley Chu wrote:
> > Hi Can,
> > 
> > 
> >> >  			} else if (!on && clki->enabled) {
> >> >  				clk_disable_unprepare(clki->clk);
> >> > +				wait_us = hba->dev_info.clk_gating_wait_us;
> >> > +				if (ref_clk && wait_us)
> >> > +					usleep_range(wait_us, wait_us + 10);
> >> 
> >> Hi St,anley,
> >> 
> >> If wait_us is 1us, it would be inappropriate to use usleep_range() 
> >> here.
> >> You have checks of the delay in patch #2, but why it is not needed 
> >> here?
> >> 
> >> Thanks,
> >> Can Guo.
> > 
> > You are right. I could make that delay checking as common function so 
> > it
> > can be used here as well to cover all possible values.
> > 
> > Thanks for suggestion.
> > Stanley
> 
> Hi Stanley,
> 
> One more thing, as in patch #2, you have already added delays in your
> ufshcd_vops_setup_clocks(OFF, PRE_CHANGE) path, plus this delay here,
> don't you delay for 2*bRefClkGatingWaitTime in ufshcd_setup_clocks()?
> As the delay added in your vops also delays the actions of turning
> off all the other clocks in ufshcd_setup_clocks(), you don't need the
> delay here again, do you agree?

MediaTek driver is not using reference clocks named as "ref_clk" defined
in device tree, thus the delay specific for "ref_clk" in
ufshcd_setup_clocks() will not be applied in MediaTek platform.

This patch is aimed to add delay for this kind of "ref_clk" used by any
future vendors.

Anyway thanks for the reminding : )

> 
> Thanks,
> Can Guo.


Thanks,
Stanley
Can Guo Feb. 17, 2020, 1:42 p.m. UTC | #5
On 2020-02-17 21:34, Stanley Chu wrote:
> Hi Can,
> 
> On Mon, 2020-02-17 at 21:22 +0800, Can Guo wrote:
>> On 2020-02-17 21:12, Stanley Chu wrote:
>> > Hi Can,
>> >
>> >
>> >> >  			} else if (!on && clki->enabled) {
>> >> >  				clk_disable_unprepare(clki->clk);
>> >> > +				wait_us = hba->dev_info.clk_gating_wait_us;
>> >> > +				if (ref_clk && wait_us)
>> >> > +					usleep_range(wait_us, wait_us + 10);
>> >>
>> >> Hi St,anley,
>> >>
>> >> If wait_us is 1us, it would be inappropriate to use usleep_range()
>> >> here.
>> >> You have checks of the delay in patch #2, but why it is not needed
>> >> here?
>> >>
>> >> Thanks,
>> >> Can Guo.
>> >
>> > You are right. I could make that delay checking as common function so
>> > it
>> > can be used here as well to cover all possible values.
>> >
>> > Thanks for suggestion.
>> > Stanley
>> 
>> Hi Stanley,
>> 
>> One more thing, as in patch #2, you have already added delays in your
>> ufshcd_vops_setup_clocks(OFF, PRE_CHANGE) path, plus this delay here,
>> don't you delay for 2*bRefClkGatingWaitTime in ufshcd_setup_clocks()?
>> As the delay added in your vops also delays the actions of turning
>> off all the other clocks in ufshcd_setup_clocks(), you don't need the
>> delay here again, do you agree?
> 
> MediaTek driver is not using reference clocks named as "ref_clk" 
> defined
> in device tree, thus the delay specific for "ref_clk" in
> ufshcd_setup_clocks() will not be applied in MediaTek platform.
> 
> This patch is aimed to add delay for this kind of "ref_clk" used by any
> future vendors.
> 
> Anyway thanks for the reminding : )
> 
>> 
>> Thanks,
>> Can Guo.
> 
> 
> Thanks,
> Stanley

Hi Stanley,

Then we are unluckily hit by this change. We have ref_clk in DT, thus
this change would add unwanted delays to our platforms. but still we
disable device's ref_clk in vops. :)

Could you please hold on patch #1 first? I need sometime to have a
dicussion with my colleagues on this.

Thanks.
Can Guo.
Bart Van Assche Feb. 17, 2020, 4:17 p.m. UTC | #6
On 2020-02-17 01:35, Stanley Chu wrote:
> -			if (skip_ref_clk && !strcmp(clki->name, "ref_clk"))
> +			ref_clk = !strcmp(clki->name, "ref_clk") ? true : false;
> +			if (skip_ref_clk && ref_clk)

Since the " ? true : false" part is superfluous, please leave it out.

Thanks,

Bart.
Stanley Chu Feb. 18, 2020, 12:50 a.m. UTC | #7
Hi Bart,

On Mon, 2020-02-17 at 08:17 -0800, Bart Van Assche wrote:
> On 2020-02-17 01:35, Stanley Chu wrote:
> > -			if (skip_ref_clk && !strcmp(clki->name, "ref_clk"))
> > +			ref_clk = !strcmp(clki->name, "ref_clk") ? true : false;
> > +			if (skip_ref_clk && ref_clk)
> 
> Since the " ? true : false" part is superfluous, please leave it out.

Will fix this in next version.

> Thanks,
> 
> Bart.

Thanks,
Stanley Chu
Can Guo Feb. 19, 2020, 2:35 a.m. UTC | #8
Hi Stanely,

On 2020-02-17 21:42, Can Guo wrote:
> On 2020-02-17 21:34, Stanley Chu wrote:
>> Hi Can,
>> 
>> On Mon, 2020-02-17 at 21:22 +0800, Can Guo wrote:
>>> On 2020-02-17 21:12, Stanley Chu wrote:
>>> > Hi Can,
>>> >
>>> >
>>> >> >  			} else if (!on && clki->enabled) {
>>> >> >  				clk_disable_unprepare(clki->clk);
>>> >> > +				wait_us = hba->dev_info.clk_gating_wait_us;
>>> >> > +				if (ref_clk && wait_us)
>>> >> > +					usleep_range(wait_us, wait_us + 10);
>>> >>
>>> >> Hi St,anley,
>>> >>
>>> >> If wait_us is 1us, it would be inappropriate to use usleep_range()
>>> >> here.
>>> >> You have checks of the delay in patch #2, but why it is not needed
>>> >> here?
>>> >>
>>> >> Thanks,
>>> >> Can Guo.
>>> >
>>> > You are right. I could make that delay checking as common function so
>>> > it
>>> > can be used here as well to cover all possible values.
>>> >
>>> > Thanks for suggestion.
>>> > Stanley
>>> 
>>> Hi Stanley,
>>> 
>>> One more thing, as in patch #2, you have already added delays in your
>>> ufshcd_vops_setup_clocks(OFF, PRE_CHANGE) path, plus this delay here,
>>> don't you delay for 2*bRefClkGatingWaitTime in ufshcd_setup_clocks()?
>>> As the delay added in your vops also delays the actions of turning
>>> off all the other clocks in ufshcd_setup_clocks(), you don't need the
>>> delay here again, do you agree?
>> 
>> MediaTek driver is not using reference clocks named as "ref_clk" 
>> defined
>> in device tree, thus the delay specific for "ref_clk" in
>> ufshcd_setup_clocks() will not be applied in MediaTek platform.
>> 
>> This patch is aimed to add delay for this kind of "ref_clk" used by 
>> any
>> future vendors.
>> 
>> Anyway thanks for the reminding : )
>> 
>>> 
>>> Thanks,
>>> Can Guo.
>> 
>> 
>> Thanks,
>> Stanley
> 
> Hi Stanley,
> 
> Then we are unluckily hit by this change. We have ref_clk in DT, thus
> this change would add unwanted delays to our platforms. but still we
> disable device's ref_clk in vops. :)
> 
> Could you please hold on patch #1 first? I need sometime to have a
> dicussion with my colleagues on this.
> 
> Thanks.
> Can Guo.

Since we all need this delay here, how about put the delay in the
entrence of ufshcd_setup_clocks(), before vops_setup_clocks()?
If so, we can remove all the delays we added in our vops since the
delay anyways delays everything inside ufshcd_setup_clocks().

Meanwhile, if you want to modify the delay
(hba->dev_info.clk_gating_wait_us) for some reasons, say for specific
UFS devices, you still can do it in vops_apply_dev_quirks().

What do you say?

Thanks,
Can Guo.
Stanley Chu Feb. 19, 2020, 9:11 a.m. UTC | #9
Hi Can,

On Wed, 2020-02-19 at 10:35 +0800, Can Guo wrote:

> Since we all need this delay here, how about put the delay in the
> entrence of ufshcd_setup_clocks(), before vops_setup_clocks()?
> If so, we can remove all the delays we added in our vops since the
> delay anyways delays everything inside ufshcd_setup_clocks().
> 

Always putting the delay in the entrance of ufshcd_setup_clocks() may
add unwanted delay for vendors, just like your current implementation,
or some other vendors who do not want to disable the reference clock.

I think current patch is more reasonable because the delay is applied to
clock only named as "ref_clk" specifically.

If you needs to keep "ref_clk" in DT, would you consider to remove the
delay in your ufs_qcom_dev_ref_clk_ctrl() and let the delay happens via
common ufshcd_setup_clocks() only? However you may still need delay if
call path comes from ufs_qcom_pwr_change_notify().

What do you think?

> Meanwhile, if you want to modify the delay
> (hba->dev_info.clk_gating_wait_us) for some reasons, say for specific
> UFS devices, you still can do it in vops_apply_dev_quirks().
> 
> What do you say?
> 
> Thanks,
> Can Guo.

Thanks,
Stanley Chu
Can Guo Feb. 19, 2020, 10:33 a.m. UTC | #10
Hi Stanley,

On 2020-02-19 17:11, Stanley Chu wrote:
> Hi Can,
> 
> On Wed, 2020-02-19 at 10:35 +0800, Can Guo wrote:
> 
>> Since we all need this delay here, how about put the delay in the
>> entrence of ufshcd_setup_clocks(), before vops_setup_clocks()?
>> If so, we can remove all the delays we added in our vops since the
>> delay anyways delays everything inside ufshcd_setup_clocks().
>> 
> 
> Always putting the delay in the entrance of ufshcd_setup_clocks() may
> add unwanted delay for vendors, just like your current implementation,
> or some other vendors who do not want to disable the reference clock.
> 
> I think current patch is more reasonable because the delay is applied 
> to
> clock only named as "ref_clk" specifically.
> 
> If you needs to keep "ref_clk" in DT, would you consider to remove the
> delay in your ufs_qcom_dev_ref_clk_ctrl() and let the delay happens via
> common ufshcd_setup_clocks() only? However you may still need delay if
> call path comes from ufs_qcom_pwr_change_notify().
> 
> What do you think?
> 

I agree current change is more reasonable from what it looks, but the 
fact
is that I canont remove the delay in ufs_qcom_dev_ref_clk_ctrl() even 
with
this change. On our platforms, ref_clk in DT serves multipule purposes,
the ref_clk provided to UFS device is actually controlled in
ufs_qcom_dev_ref_clk_ctrl(), which comes before where this change kicks 
start,
so if I remove the delay in ufs_qcom_dev_ref_clk_ctrl(), this change 
cannot
provide us the correct delay before gate the ref_clk provided to UFS 
device.

> Always putting the delay in the entrance of ufshcd_setup_clocks() may
> add unwanted delay for vendors, just like your current implementation,
> or some other vendors who do not want to disable the reference clock.

I meant if we put the delay in the entrance, I will be able to remove
the delay in ufs_qcom_dev_ref_clk_ctrl(). Meanwhile, we can add proper
checks before the delay to make sure it is initiated only if ref_clk 
needs
to be disabled, i.e:

if(!on && !skip_ref_clk && hba->dev_info.clk_gating_wait_us)
     usleep_range();

Does this look better to you?

Anyways, we will see regressions with this change on our platforms, can 
we
have more discussions before get it merged? It should be OK if you go 
with
patch #2 alone first, right? Thanks.

Best regards,
Can Guo.

>> Meanwhile, if you want to modify the delay
>> (hba->dev_info.clk_gating_wait_us) for some reasons, say for specific
>> UFS devices, you still can do it in vops_apply_dev_quirks().
>> 
>> What do you say?
>> 
>> Thanks,
>> Can Guo.
> 
> Thanks,
> Stanley Chu
Stanley Chu Feb. 20, 2020, 1:30 p.m. UTC | #11
Hi Can,

On Wed, 2020-02-19 at 18:33 +0800, Can Guo wrote:
> Hi Stanley,
> 
> On 2020-02-19 17:11, Stanley Chu wrote:
> > Hi Can,
> > 
> > On Wed, 2020-02-19 at 10:35 +0800, Can Guo wrote:
> > 
> >> Since we all need this delay here, how about put the delay in the
> >> entrence of ufshcd_setup_clocks(), before vops_setup_clocks()?
> >> If so, we can remove all the delays we added in our vops since the
> >> delay anyways delays everything inside ufshcd_setup_clocks().
> >> 
> > 
> > Always putting the delay in the entrance of ufshcd_setup_clocks() may
> > add unwanted delay for vendors, just like your current implementation,
> > or some other vendors who do not want to disable the reference clock.
> > 
> > I think current patch is more reasonable because the delay is applied 
> > to
> > clock only named as "ref_clk" specifically.
> > 
> > If you needs to keep "ref_clk" in DT, would you consider to remove the
> > delay in your ufs_qcom_dev_ref_clk_ctrl() and let the delay happens via
> > common ufshcd_setup_clocks() only? However you may still need delay if
> > call path comes from ufs_qcom_pwr_change_notify().
> > 
> > What do you think?
> > 
> 
> I agree current change is more reasonable from what it looks, but the 
> fact
> is that I canont remove the delay in ufs_qcom_dev_ref_clk_ctrl() even 
> with
> this change. On our platforms, ref_clk in DT serves multipule purposes,
> the ref_clk provided to UFS device is actually controlled in
> ufs_qcom_dev_ref_clk_ctrl(), which comes before where this change kicks 
> start,
> so if I remove the delay in ufs_qcom_dev_ref_clk_ctrl(), this change 
> cannot
> provide us the correct delay before gate the ref_clk provided to UFS 
> device.

> > Always putting the delay in the entrance of ufshcd_setup_clocks() may
> > add unwanted delay for vendors, just like your current implementation,
> > or some other vendors who do not want to disable the reference clock.
> 
> I meant if we put the delay in the entrance, I will be able to remove
> the delay in ufs_qcom_dev_ref_clk_ctrl(). Meanwhile, we can add proper
> checks before the delay to make sure it is initiated only if ref_clk 
> needs
> to be disabled, i.e:
> 
> if(!on && !skip_ref_clk && hba->dev_info.clk_gating_wait_us)
>      usleep_range();
> 
> Does this look better to you?

Firstly thanks so much for above details.

Again this statement may also add unwanted delay if some other vendors
does not have "ref_clk" in DT or they don't/can't disable the reference
clock provided to UFS device.

> 
> Anyways, we will see regressions with this change on our platforms, can 
> we
> have more discussions before get it merged? It should be OK if you go 
> with
> patch #2 alone first, right? Thanks.

Now the fact is that this change will impact your flow and it seems no
solid conclusion yet. Sure I could drop patch #1 and submit patch #2
only first : )

Thanks,
Stanley Chu

Patch
diff mbox series

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 744b8254220c..7f60721f54d1 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7417,8 +7417,10 @@  static int __ufshcd_setup_clocks(struct ufs_hba *hba, bool on,
 	struct ufs_clk_info *clki;
 	struct list_head *head = &hba->clk_list_head;
 	unsigned long flags;
+	unsigned long wait_us;
 	ktime_t start = ktime_get();
 	bool clk_state_changed = false;
+	bool ref_clk;
 
 	if (list_empty(head))
 		goto out;
@@ -7436,7 +7438,8 @@  static int __ufshcd_setup_clocks(struct ufs_hba *hba, bool on,
 
 	list_for_each_entry(clki, head, list) {
 		if (!IS_ERR_OR_NULL(clki->clk)) {
-			if (skip_ref_clk && !strcmp(clki->name, "ref_clk"))
+			ref_clk = !strcmp(clki->name, "ref_clk") ? true : false;
+			if (skip_ref_clk && ref_clk)
 				continue;
 
 			clk_state_changed = on ^ clki->enabled;
@@ -7449,6 +7452,9 @@  static int __ufshcd_setup_clocks(struct ufs_hba *hba, bool on,
 				}
 			} else if (!on && clki->enabled) {
 				clk_disable_unprepare(clki->clk);
+				wait_us = hba->dev_info.clk_gating_wait_us;
+				if (ref_clk && wait_us)
+					usleep_range(wait_us, wait_us + 10);
 			}
 			clki->enabled = on;
 			dev_dbg(hba->dev, "%s: clk: %s %sabled\n", __func__,