| Message ID | 20200217093559.16830-2-stanley.chu@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | scsi: ufs: fix waiting time for reference clock | expand |
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__,
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
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__,
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(-)