[PATCH v1 0/2] ufs: allow vendor disable wb toggle in clock scaling

[PATCH v1 0/2] ufs: allow vendor disable wb toggle in clock scaling

[peter.wang]

From: Peter Wang <peter.wang@mediatek.com>

Mediatek ufs do not want to toggle write booster when clock scaling. 
This patch set allow vendor disable wb toggle in clock scaling.

Peter Wang (2):
  ufs: core: interduce a choice of wb toggle in clock scaling
  ufs: host: support wb toggle with clock scaling

 drivers/ufs/core/ufs-sysfs.c | 3 ++-
 drivers/ufs/core/ufshcd.c    | 8 +++++---
 drivers/ufs/host/ufs-qcom.c  | 2 +-
 include/ufs/ufshcd.h         | 7 +++++++
 4 files changed, 15 insertions(+), 5 deletions(-)

-- 
2.18.0

[PATCH v1 1/2] ufs: core: interduce a choice of wb toggle in clock scaling

[peter.wang]

From: Peter Wang <peter.wang@mediatek.com>

Vendor may wan't disable/enable write booster while clock scaling.
Introduce a flag UFSHCD_CAP_WB_WITH_CLK_SCALING to decouple WB and
clock scaling.

UFSHCD_CAP_WB_WITH_CLK_SCALING only valid when UFSHCD_CAP_CLK_SCALING
is set. Just like UFSHCD_CAP_HIBERN8_WITH_CLK_GATING is valid only when
UFSHCD_CAP_CLK_GATING set.

Signed-off-by: Peter Wang <peter.wang@mediatek.com>
---
 drivers/ufs/core/ufs-sysfs.c | 3 ++-
 drivers/ufs/core/ufshcd.c    | 8 +++++---
 include/ufs/ufshcd.h         | 7 +++++++
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
index 0a088b47d557..8fa1679ffd1a 100644
--- a/drivers/ufs/core/ufs-sysfs.c
+++ b/drivers/ufs/core/ufs-sysfs.c
@@ -225,7 +225,8 @@ static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
 	unsigned int wb_enable;
 	ssize_t res;
 
-	if (!ufshcd_is_wb_allowed(hba) || ufshcd_is_clkscaling_supported(hba)) {
+	if (!ufshcd_is_wb_allowed(hba) || (ufshcd_is_clkscaling_supported(hba)
+		&& ufshcd_can_wb_during_scaling(hba))) {
 		/*
 		 * If the platform supports UFSHCD_CAP_CLK_SCALING, turn WB
 		 * on/off will be done while clock scaling up/down.
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index c7b337480e3e..5d1f2ff0f596 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -1301,9 +1301,11 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up)
 	}
 
 	/* Enable Write Booster if we have scaled up else disable it */
-	downgrade_write(&hba->clk_scaling_lock);
-	is_writelock = false;
-	ufshcd_wb_toggle(hba, scale_up);
+	if (ufshcd_can_wb_during_scaling(hba)) {
+		downgrade_write(&hba->clk_scaling_lock);
+		is_writelock = false;
+		ufshcd_wb_toggle(hba, scale_up);
+	}
 
 out_unprepare:
 	ufshcd_clock_scaling_unprepare(hba, is_writelock);
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index a92271421718..4bd35d68acde 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -648,6 +648,9 @@ enum ufshcd_caps {
 	 * notification if it is supported by the UFS device.
 	 */
 	UFSHCD_CAP_TEMP_NOTIF				= 1 << 11,
+
+	/* Allow WB with clk scaling */
+	UFSHCD_CAP_WB_WITH_CLK_SCALING			= 1 << 12,
 };
 
 struct ufs_hba_variant_params {
@@ -1004,6 +1007,10 @@ static inline bool ufshcd_is_wb_allowed(struct ufs_hba *hba)
 {
 	return hba->caps & UFSHCD_CAP_WB_EN;
 }
+static inline bool ufshcd_can_wb_during_scaling(struct ufs_hba *hba)
+{
+	return hba->caps & UFSHCD_CAP_WB_WITH_CLK_SCALING;
+}
 
 #define ufshcd_writel(hba, val, reg)	\
 	writel((val), (hba)->mmio_base + (reg))
-- 
2.18.0

Re: [PATCH v1 1/2] ufs: core: interduce a choice of wb toggle in clock scaling

[Bean Huo]

On Thu, 2022-07-28 at 15:16 +0800, peter.wang@mediatek.com wrote:
> From: Peter Wang <peter.wang@mediatek.com>
> 
> Vendor may wan't disable/enable write booster while clock scaling.
> Introduce a flag UFSHCD_CAP_WB_WITH_CLK_SCALING to decouple WB and
> clock scaling.
> 
> UFSHCD_CAP_WB_WITH_CLK_SCALING only valid when UFSHCD_CAP_CLK_SCALING
> is set. Just like UFSHCD_CAP_HIBERN8_WITH_CLK_GATING is valid only
> when
> UFSHCD_CAP_CLK_GATING set.
> 

Hi Peter,

probably "interduce" is a typo in your subject?

Kind regards,
Bean

Re: [PATCH v1 1/2] ufs: core: interduce a choice of wb toggle in clock scaling

[Peter Wang]

On 7/29/22 5:41 AM, Bean Huo wrote:
> On Thu, 2022-07-28 at 15:16 +0800, peter.wang@mediatek.com wrote:
>> From: Peter Wang <peter.wang@mediatek.com>
>>
>> Vendor may wan't disable/enable write booster while clock scaling.
>> Introduce a flag UFSHCD_CAP_WB_WITH_CLK_SCALING to decouple WB and
>> clock scaling.
>>
>> UFSHCD_CAP_WB_WITH_CLK_SCALING only valid when UFSHCD_CAP_CLK_SCALING
>> is set. Just like UFSHCD_CAP_HIBERN8_WITH_CLK_GATING is valid only
>> when
>> UFSHCD_CAP_CLK_GATING set.
>>
> Hi Peter,
>
> probably "interduce" is a typo in your subject?
>
> Kind regards,
> Bean


Hi Bean,

Yes, it typo.

Thanks.
Peter

[PATCH v1 2/2] ufs: host: support wb toggle with clock scaling

[peter.wang]

From: Peter Wang <peter.wang@mediatek.com>

Set UFSHCD_CAP_WB_WITH_CLK_SCALING for qcom to compatible legacy design.

Signed-off-by: Peter Wang <peter.wang@mediatek.com>
---
 drivers/ufs/host/ufs-qcom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index f10d4668814c..f8c9a78e7776 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -869,7 +869,7 @@ static void ufs_qcom_set_caps(struct ufs_hba *hba)
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
 
 	hba->caps |= UFSHCD_CAP_CLK_GATING | UFSHCD_CAP_HIBERN8_WITH_CLK_GATING;
-	hba->caps |= UFSHCD_CAP_CLK_SCALING;
+	hba->caps |= UFSHCD_CAP_CLK_SCALING | UFSHCD_CAP_WB_WITH_CLK_SCALING;
 	hba->caps |= UFSHCD_CAP_AUTO_BKOPS_SUSPEND;
 	hba->caps |= UFSHCD_CAP_WB_EN;
 	hba->caps |= UFSHCD_CAP_CRYPTO;
-- 
2.18.0

Re: [PATCH v1 2/2] ufs: host: support wb toggle with clock scaling

[Bean Huo]

On Thu, 2022-07-28 at 15:16 +0800, peter.wang@mediatek.com wrote:
> From: Peter Wang <peter.wang@mediatek.com>
> 
> Set UFSHCD_CAP_WB_WITH_CLK_SCALING for qcom to compatible legacy
> design.
> 
> Signed-off-by: Peter Wang <peter.wang@mediatek.com>
> ---
>  drivers/ufs/host/ufs-qcom.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-
> qcom.c
> index f10d4668814c..f8c9a78e7776 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -869,7 +869,7 @@ static void ufs_qcom_set_caps(struct ufs_hba
> *hba)
>         struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>  
>         hba->caps |= UFSHCD_CAP_CLK_GATING |
> UFSHCD_CAP_HIBERN8_WITH_CLK_GATING;
> -       hba->caps |= UFSHCD_CAP_CLK_SCALING;
> +       hba->caps |= UFSHCD_CAP_CLK_SCALING |
> UFSHCD_CAP_WB_WITH_CLK_SCALING;
>         hba->caps |= UFSHCD_CAP_AUTO_BKOPS_SUSPEND;
>         hba->caps |= UFSHCD_CAP_WB_EN;
>         hba->caps |= UFSHCD_CAP_CRYPTO;

Hi peter, 

If WB is on/off based on clk scaling up/down is legacy design, maybe
you have a more advanced design. It is true there is an issue since we
didn't differentiate the read or write. WB is only for write. How to
know this time clk scaling is for write from driver level, not possible
now.

Kind regards,
Bean

Re: [PATCH v1 2/2] ufs: host: support wb toggle with clock scaling

[Peter Wang]

On 7/29/22 5:57 AM, Bean Huo wrote:
> On Thu, 2022-07-28 at 15:16 +0800, peter.wang@mediatek.com wrote:
>> From: Peter Wang <peter.wang@mediatek.com>
>>
>> Set UFSHCD_CAP_WB_WITH_CLK_SCALING for qcom to compatible legacy
>> design.
>>
>> Signed-off-by: Peter Wang <peter.wang@mediatek.com>
>> ---
>>   drivers/ufs/host/ufs-qcom.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-
>> qcom.c
>> index f10d4668814c..f8c9a78e7776 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -869,7 +869,7 @@ static void ufs_qcom_set_caps(struct ufs_hba
>> *hba)
>>          struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>>   
>>          hba->caps |= UFSHCD_CAP_CLK_GATING |
>> UFSHCD_CAP_HIBERN8_WITH_CLK_GATING;
>> -       hba->caps |= UFSHCD_CAP_CLK_SCALING;
>> +       hba->caps |= UFSHCD_CAP_CLK_SCALING |
>> UFSHCD_CAP_WB_WITH_CLK_SCALING;
>>          hba->caps |= UFSHCD_CAP_AUTO_BKOPS_SUSPEND;
>>          hba->caps |= UFSHCD_CAP_WB_EN;
>>          hba->caps |= UFSHCD_CAP_CRYPTO;
> Hi peter,
>
> If WB is on/off based on clk scaling up/down is legacy design, maybe
> you have a more advanced design. It is true there is an issue since we
> didn't differentiate the read or write. WB is only for write. How to
> know this time clk scaling is for write from driver level, not possible
> now.
>
> Kind regards,
> Bean

Hi Bean,

Yes, we don't know if clock scaling up/down is for write or read.
But we want to keep WB always on even clock scaling down.
This is why we need this patch, let different host can choose toggle wb 
or not.

Thanks.
Peter

RE: [PATCH v1 0/2] ufs: allow vendor disable wb toggle in clock scaling

[Avri Altman]

> 
> From: Peter Wang <peter.wang@mediatek.com>
> 
> Mediatek ufs do not want to toggle write booster when clock scaling.
> This patch set allow vendor disable wb toggle in clock scaling.
> 
> Peter Wang (2):
>   ufs: core: interduce a choice of wb toggle in clock scaling
>   ufs: host: support wb toggle with clock scaling
It would make more sense to squash both patches and add it as #6 after - 
" Support clk-scaling to optimize power consumption" in your earlier 
" Provide features and fixes in MediaTek platforms" series.

Either way, Looks good to me.

Thanks,
Avri

Re: [PATCH v1 0/2] ufs: allow vendor disable wb toggle in clock scaling

[Peter Wang]

On 7/29/22 4:43 AM, Avri Altman wrote:
>> From: Peter Wang <peter.wang@mediatek.com>
>>
>> Mediatek ufs do not want to toggle write booster when clock scaling.
>> This patch set allow vendor disable wb toggle in clock scaling.
>>
>> Peter Wang (2):
>>    ufs: core: interduce a choice of wb toggle in clock scaling
>>    ufs: host: support wb toggle with clock scaling
> It would make more sense to squash both patches and add it as #6 after -
> " Support clk-scaling to optimize power consumption" in your earlier
> " Provide features and fixes in MediaTek platforms" series.
>
> Either way, Looks good to me.
>
> Thanks,
> Avri


Hi Arvi,

Yes, it make more sense to enable clock scaling in mediatek platform first.
I will wait "Provide features and fixes in MediaTek platforms" patch set 
accepted than update this patch.

Thanks.
Peter

Re: [PATCH v1 0/2] ufs: allow vendor disable wb toggle in clock scaling

[Bart Van Assche]

On 7/28/22 00:16, peter.wang@mediatek.com wrote:
> Mediatek ufs do not want to toggle write booster when clock scaling.
> This patch set allow vendor disable wb toggle in clock scaling.

I don't like this approach. Whether or not to toggle the write booster 
when scaling the clock is not dependent on the host controller and hence 
should not depend on the host controller driver.

Has it been considered to add a sysfs attribute in the UFS driver core 
to control this behavior?

Thanks,

Bart.

Re: [PATCH v1 0/2] ufs: allow vendor disable wb toggle in clock scaling

[Bean Huo]

On Thu, 2022-07-28 at 14:09 -0700, Bart Van Assche wrote:
> On 7/28/22 00:16, peter.wang@mediatek.com wrote:
> > Mediatek ufs do not want to toggle write booster when clock
> > scaling.
> > This patch set allow vendor disable wb toggle in clock scaling.
> 
> I don't like this approach. Whether or not to toggle the write
> booster 
> when scaling the clock is not dependent on the host controller and
> hence 
> should not depend on the host controller driver.
> 
> Has it been considered to add a sysfs attribute in the UFS driver
> core 
> to control this behavior?
> 

Bart, 
we already have wb_on sysfs node, but it only allows to write this node
when clock scaling is not supported.


static ssize_t wb_on_store(..)
{
	struct ufs_hba *hba = dev_get_drvdata(dev);
	unsigned int wb_enable;
	ssize_t res;

	if (ufshcd_is_clkscaling_supported(hba)) {
		/*
		 * If the platform supports
UFSHCD_CAP_AUTO_BKOPS_SUSPEND,
		 * turn WB on/off will be done while clock scaling
up/down.
		 */
		dev_warn(dev, "To control WB through wb_on is not
allowed!\n");
		return -EOPNOTSUPP;
	}


Kind regards,
Bean

> Thanks,
> 
> Bart.

Re: [PATCH v1 0/2] ufs: allow vendor disable wb toggle in clock scaling

[Stanley Chu]

Hi,

On Fri, Jul 29, 2022 at 5:27 AM Bean Huo <huobean@gmail.com> wrote:
>
> On Thu, 2022-07-28 at 14:09 -0700, Bart Van Assche wrote:
> > On 7/28/22 00:16, peter.wang@mediatek.com wrote:
> > > Mediatek ufs do not want to toggle write booster when clock
> > > scaling.
> > > This patch set allow vendor disable wb toggle in clock scaling.
> >
> > I don't like this approach. Whether or not to toggle the write
> > booster
> > when scaling the clock is not dependent on the host controller and
> > hence
> > should not depend on the host controller driver.
> >
> > Has it been considered to add a sysfs attribute in the UFS driver
> > core
> > to control this behavior?
> >
>
> Bart,
> we already have wb_on sysfs node, but it only allows to write this node
> when clock scaling is not supported.
>
>
> static ssize_t wb_on_store(..)
> {
>         struct ufs_hba *hba = dev_get_drvdata(dev);
>         unsigned int wb_enable;
>         ssize_t res;
>
>         if (ufshcd_is_clkscaling_supported(hba)) {
>                 /*
>                  * If the platform supports
> UFSHCD_CAP_AUTO_BKOPS_SUSPEND,
>                  * turn WB on/off will be done while clock scaling
> up/down.
>                  */
>                 dev_warn(dev, "To control WB through wb_on is not
> allowed!\n");
>                 return -EOPNOTSUPP;
>         }
>
>
> Kind regards,
> Bean
>
> > Thanks,
> >
> > Bart.

Acked to this patch series.

Clk-Scaling is aimed to save power by fine-tuning any possible
resources depending on the workload.

Currently below items would be tuned,
1. Clk rate
2. Gear
3. Write Booster switch on/off

The truth is that each host (and device) vendor has different designs
to reduce the power, therefore those tunings may not be suitable or
need different ways for all host platforms.

Take below cases for example,

1. The clk cannot be set at different rates directly because it is
shared with other users.

2. The Write Booster feature does not need to be disabled because this
impacts the performance too much. Performance may be even worse than
the case when clk-scaling is disabled. In addition, system power may
be not reduced if a long data write period is harmful to the system
low-power design.

Thanks,
Stanley

Re: [PATCH v1 0/2] ufs: allow vendor disable wb toggle in clock scaling

[Peter Wang]

On 7/29/22 5:09 AM, Bart Van Assche wrote:
> On 7/28/22 00:16, peter.wang@mediatek.com wrote:
>> Mediatek ufs do not want to toggle write booster when clock scaling.
>> This patch set allow vendor disable wb toggle in clock scaling.
>
> I don't like this approach. Whether or not to toggle the write booster 
> when scaling the clock is not dependent on the host controller and 
> hence should not depend on the host controller driver.
>
> Has it been considered to add a sysfs attribute in the UFS driver core 
> to control this behavior?
>
> Thanks,
>
> Bart.

Hi Bart,

Write booster binding with clock scaling is not make sense.
Clock scaling should always do clock scaling related things, and write 
bootster is not related to clock, right?
So Mediatek don't want to toggle wb with clock scaling.
Consider legacy design is binding, so we provide a flag to decouple them 
instead remove ufshcd_wb_toggle directly.
Or, do you think we can direct remove ufshcd_wb_toggle in clock scaling 
and only let sysfs to control wb behavior?

Thanks.
Peter

Re: [PATCH v1 0/2] ufs: allow vendor disable wb toggle in clock scaling

[Bart Van Assche]

On 8/1/22 07:30, Peter Wang wrote:
> Or, do you think we can direct remove ufshcd_wb_toggle in clock scaling 
> and only let sysfs to control wb behavior?

I think it's worth asking the people who introduced this feature whether 
it can be removed.

Hi Asutosh,

Commit 3d17b9b5ab11 ("scsi: ufs: Add write booster feature support") 
introduced the following code in ufshcd_devfreq_scale():

+       /* Enable Write Booster if we have scaled up else disable it */
+       up_write(&hba->clk_scaling_lock);
+       ufshcd_wb_ctrl(hba, scale_up);
+       down_write(&hba->clk_scaling_lock);

Would you mind if the code for enabling/disabling the WriteBooster is 
removed again from ufshcd_devfreq_scale() and that a new mechanism is 
introduced for controlling the WriteBooster mechanism?

Thanks,

Bart.

Re: [PATCH v1 0/2] ufs: allow vendor disable wb toggle in clock scaling

[Asutosh Das (asd)]

Hello,

On 8/1/2022 9:43 AM, Bart Van Assche wrote:
> On 8/1/22 07:30, Peter Wang wrote:
>> Or, do you think we can direct remove ufshcd_wb_toggle in clock 
>> scaling and only let sysfs to control wb behavior?
> 
> I think it's worth asking the people who introduced this feature whether 
> it can be removed.
> 
> Hi Asutosh,
> 
> Commit 3d17b9b5ab11 ("scsi: ufs: Add write booster feature support") 
> introduced the following code in ufshcd_devfreq_scale():
> 
> +       /* Enable Write Booster if we have scaled up else disable it */
> +       up_write(&hba->clk_scaling_lock);
> +       ufshcd_wb_ctrl(hba, scale_up);
> +       down_write(&hba->clk_scaling_lock);
> 
> Would you mind if the code for enabling/disabling the WriteBooster is 
> removed again from ufshcd_devfreq_scale() and that a new mechanism is 
> introduced for controlling the WriteBooster mechanism?
> 
Qualcomm would need the load based toggling of WB during scaling.
What would the new mechanism be for controlling WB mechanism?

> Thanks,
> 
> Bart.

Re: [PATCH v1 0/2] ufs: allow vendor disable wb toggle in clock scaling

[Christoph Hellwig]

Please fix up your wording.  A vendor can't do anything at all in Linux.
A driver might be able to disable things for a specific device, though.

Re: [PATCH v1 0/2] ufs: allow vendor disable wb toggle in clock scaling

[Bart Van Assche]

On 8/1/22 10:57, Christoph Hellwig wrote:
> Please fix up your wording.  A vendor can't do anything at all in Linux.
> A driver might be able to disable things for a specific device, though.

Agreed that the description should become more clear. I think Peter 
wanted to refer to the "vendor driver" where he wrote "vendor".

Bart.

Re: [PATCH v1 0/2] ufs: allow vendor disable wb toggle in clock scaling

[Christoph Hellwig]

On Mon, Aug 01, 2022 at 11:12:17AM -0700, Bart Van Assche wrote:
> Agreed that the description should become more clear. I think Peter wanted
> to refer to the "vendor driver" where he wrote "vendor".

vendor driver makes just as little sense.  hardware interfaces do not
map to vendors in any sensible way.

Re: [PATCH v1 0/2] ufs: allow vendor disable wb toggle in clock scaling

[Peter Wang]

On 8/2/22 2:14 AM, Christoph Hellwig wrote:
> On Mon, Aug 01, 2022 at 11:12:17AM -0700, Bart Van Assche wrote:
>> Agreed that the description should become more clear. I think Peter wanted
>> to refer to the "vendor driver" where he wrote "vendor".
> vendor driver makes just as little sense.  hardware interfaces do not
> map to vendors in any sensible way.

Hi Christoph,

Will change commit message.

Thanks

Peter