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 <[email protected]>
---
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);
}
clki->enabled = on;
dev_dbg(hba->dev, "%s: clk: %s %sabled\n", __func__,
--
2.18.0
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 <[email protected]>
> ---
> 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__,
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
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.
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
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.
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.
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
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.
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
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
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