[PATCH v1 0/2] scsi: ufs: fix waiting time for reference clock

[PATCH v1 0/2] scsi: ufs: fix waiting time for reference clock

[Stanley Chu]

Hi,

This patchset adds waiting time for reference clock in both common path and MediaTek UFS implementions.

Stanley Chu (2):
  scsi: ufs: add required delay after gating reference clock
  scsi: ufs: ufs-mediatek: add waiting time for reference clock

 drivers/scsi/ufs/ufs-mediatek.c | 46 +++++++++++++++++++++++++++++++--
 drivers/scsi/ufs/ufs-mediatek.h |  2 ++
 drivers/scsi/ufs/ufshcd.c       |  8 +++++-
 3 files changed, 53 insertions(+), 3 deletions(-)

-- 
2.18.0

[PATCH v1 1/2] scsi: ufs: add required delay after gating reference clock

[Stanley Chu]

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

[PATCH v1 2/2] scsi: ufs: ufs-mediatek: add waiting time for reference clock

[Stanley Chu]

Some delays may be required either after gating or before ungating
reference clock for device according to vendor requirements.

Note that in UFS 3.0, the delay time after gating reference
clock can be defined by attribute bRefClkGatingWaitTime. Use the
formal value instead if it can be queried from device.

Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufs-mediatek.c | 46 +++++++++++++++++++++++++++++++--
 drivers/scsi/ufs/ufs-mediatek.h |  2 ++
 2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-mediatek.c b/drivers/scsi/ufs/ufs-mediatek.c
index 9d05962feb15..de650822c9d9 100644
--- a/drivers/scsi/ufs/ufs-mediatek.c
+++ b/drivers/scsi/ufs/ufs-mediatek.c
@@ -100,6 +100,17 @@ static int ufs_mtk_bind_mphy(struct ufs_hba *hba)
 	return err;
 }
 
+static void ufs_mtk_udelay(unsigned long us)
+{
+	if (!us)
+		return;
+
+	if (us < 10)
+		udelay(us);
+	else
+		usleep_range(us, us + 10);
+}
+
 static int ufs_mtk_setup_ref_clk(struct ufs_hba *hba, bool on)
 {
 	struct ufs_mtk_host *host = ufshcd_get_variant(hba);
@@ -112,6 +123,7 @@ static int ufs_mtk_setup_ref_clk(struct ufs_hba *hba, bool on)
 
 	if (on) {
 		ufs_mtk_ref_clk_notify(on, res);
+		ufs_mtk_udelay(host->ref_clk_ungating_wait_us);
 		ufshcd_writel(hba, REFCLK_REQUEST, REG_UFS_REFCLK_CTRL);
 	} else {
 		ufshcd_writel(hba, REFCLK_RELEASE, REG_UFS_REFCLK_CTRL);
@@ -137,12 +149,29 @@ static int ufs_mtk_setup_ref_clk(struct ufs_hba *hba, bool on)
 
 out:
 	host->ref_clk_enabled = on;
-	if (!on)
+	if (!on) {
+		ufs_mtk_udelay(host->ref_clk_gating_wait_us);
 		ufs_mtk_ref_clk_notify(on, res);
+	}
 
 	return 0;
 }
 
+static void ufs_mtk_setup_ref_clk_wait_us(struct ufs_hba *hba,
+					  u16 gating_us, u16 ungating_us)
+{
+	struct ufs_mtk_host *host = ufshcd_get_variant(hba);
+
+	if (hba->dev_info.clk_gating_wait_us) {
+		host->ref_clk_gating_wait_us =
+			hba->dev_info.clk_gating_wait_us;
+	} else {
+		host->ref_clk_gating_wait_us = gating_us;
+	}
+
+	host->ref_clk_ungating_wait_us = ungating_us;
+}
+
 static u32 ufs_mtk_link_get_state(struct ufs_hba *hba)
 {
 	u32 val;
@@ -502,10 +531,23 @@ static void ufs_mtk_dbg_register_dump(struct ufs_hba *hba)
 static int ufs_mtk_apply_dev_quirks(struct ufs_hba *hba)
 {
 	struct ufs_dev_info *dev_info = &hba->dev_info;
+	u16 mid = dev_info->wmanufacturerid;
 
-	if (dev_info->wmanufacturerid == UFS_VENDOR_SAMSUNG)
+	if (mid == UFS_VENDOR_SAMSUNG)
 		ufshcd_dme_set(hba, UIC_ARG_MIB(PA_TACTIVATE), 6);
 
+	/*
+	 * Decide waiting time before gating reference clock and
+	 * after ungating reference clock according to vendors'
+	 * requirements.
+	 */
+	if (mid == UFS_VENDOR_SAMSUNG)
+		ufs_mtk_setup_ref_clk_wait_us(hba, 1, 1);
+	else if (mid == UFS_VENDOR_SKHYNIX)
+		ufs_mtk_setup_ref_clk_wait_us(hba, 30, 30);
+	else if (mid == UFS_VENDOR_TOSHIBA)
+		ufs_mtk_setup_ref_clk_wait_us(hba, 100, 32);
+
 	return 0;
 }
 
diff --git a/drivers/scsi/ufs/ufs-mediatek.h b/drivers/scsi/ufs/ufs-mediatek.h
index 492414e5f481..4c787b99fe41 100644
--- a/drivers/scsi/ufs/ufs-mediatek.h
+++ b/drivers/scsi/ufs/ufs-mediatek.h
@@ -92,6 +92,8 @@ struct ufs_mtk_host {
 	struct ufs_hba *hba;
 	struct phy *mphy;
 	bool ref_clk_enabled;
+	u16 ref_clk_ungating_wait_us;
+	u16 ref_clk_gating_wait_us;
 };
 
 #endif /* !_UFS_MEDIATEK_H */
-- 
2.18.0

Re: [PATCH v1 1/2] scsi: ufs: add required delay after gating reference clock

[Can Guo]

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__,

Re: [PATCH v1 1/2] scsi: ufs: add required delay after gating reference clock

[Stanley Chu]

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

Re: [PATCH v1 1/2] scsi: ufs: add required delay after gating reference clock

[Can Guo]

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.

Re: [PATCH v1 1/2] scsi: ufs: add required delay after gating reference clock

[Stanley Chu]

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

Re: [PATCH v1 1/2] scsi: ufs: add required delay after gating reference clock

[Can Guo]

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.

Re: [PATCH v1 1/2] scsi: ufs: add required delay after gating reference clock

[Bart Van Assche]

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.

Re: [PATCH v1 1/2] scsi: ufs: add required delay after gating reference clock

[Stanley Chu]

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

Re: [PATCH v1 1/2] scsi: ufs: add required delay after gating reference clock

[Can Guo]

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.

Re: [PATCH v1 1/2] scsi: ufs: add required delay after gating reference clock

[Stanley Chu]

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

Re: [PATCH v1 1/2] scsi: ufs: add required delay after gating reference clock

[Can Guo]

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

Re: [PATCH v1 1/2] scsi: ufs: add required delay after gating reference clock

[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