ufs: allow vendor disable wb toggle in clock scaling

PATCH Superseded [email protected] 7 participants 19 messages Jul 28, 2022 – Aug 2, 2022

Subsystems (5)

SCSI Arm Qualcomm mailing list Arm MediaTek UFS host controller +1 more subsystems

lore

Share & tools

lore

[email protected]

author

2022-07-28 07:16 UTC #0 lore raw

cover ufs: allow vendor disable wb toggle in clock scaling

From: Peter Wang <[email protected]>

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(-)

Signature · 1 line

2.18.0

[email protected]

author

2022-07-28 07:16 UTC #1 lore raw

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

From: Peter Wang <[email protected]>

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 <[email protected]>

---
 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(-)

drivers/ufs/core/ufs-sysfs.c +2 -1

index 0a088b47d557..8fa1679ffd1a 100644
@@ -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.

drivers/ufs/core/ufshcd.c +5 -3

index c7b337480e3e..5d1f2ff0f596 100644
@@ -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);

include/ufs/ufshcd.h +7 -0

index a92271421718..4bd35d68acde 100644
@@ -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))

Signature · 1 line

2.18.0

Bean Huo

2022-07-28 21:41 UTC #2 lore raw

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

Quoted 11 lines · On Thu, 2022-07-28 at 15:16 +0800, [email protected] wrote:

From: Peter Wang <[email protected]>
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

Peter Wang

author

2022-08-01 14:31 UTC #3 lore raw

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

Quoted 12 lines · Bean Huo

Quoted 11 lines · On Thu, 2022-07-28 at 15:16 +0800, [email protected] wrote:
From: Peter Wang <[email protected]>
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.

Quoted 6 lines

Hi Peter,
probably "interduce" is a typo in your subject?
Kind regards,
Bean

Hi Bean,

Yes, it typo.

Thanks.
Peter

[email protected]

author

2022-07-28 07:16 UTC #4 lore raw

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

In reply to [email protected]

From: Peter Wang <[email protected]>

Set UFSHCD_CAP_WB_WITH_CLK_SCALING for qcom to compatible legacy design.

Signed-off-by Peter Wang <[email protected]>

---
 drivers/ufs/host/ufs-qcom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/ufs/host/ufs-qcom.c +1 -1

index f10d4668814c..f8c9a78e7776 100644
@@ -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;

Signature · 1 line

2.18.0

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

Peter Wang

author

2022-08-01 14:32 UTC #6 lore raw

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

Quoted 30 lines · Bean Huo

Quoted 24 lines · On Thu, 2022-07-28 at 15:16 +0800, [email protected] wrote:
From: Peter Wang <[email protected]>
Set UFSHCD_CAP_WB_WITH_CLK_SCALING for qcom to compatible legacy design.
Signed-off-by Peter Wang <[email protected]>
---
  drivers/ufs/host/ufs-qcom.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/ufs/host/ufs- renamed from drivers/ufs/host/ufs-qcom.c +0 -0
qcom.c
index f10d4668814c..f8c9a78e7776 100644
drivers/ufs/host/ufs-qcom.c +1 -1
@@ -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;
Quoted 3 lines
         hba->caps |= UFSHCD_CAP_AUTO_BKOPS_SUSPEND;
         hba->caps |= UFSHCD_CAP_WB_EN;
         hba->caps |= UFSHCD_CAP_CRYPTO;
Hi peter,

Quoted 8 lines

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

Avri Altman

reviewer

2022-07-28 20:43 UTC #7 lore raw

In reply to [email protected]

Quoted 9 lines

From: Peter Wang <[email protected]>
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

Peter Wang

author

2022-08-01 14:28 UTC #8 lore raw

Quoted 8 lines · Avri Altman

Quoted 8 lines
From: Peter Wang <[email protected]>
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

Quoted 8 lines

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

Bart Van Assche

reviewer

2022-07-28 21:09 UTC #9 lore raw

In reply to [email protected]

Quoted 2 lines · On 7/28/22 00:16, [email protected] 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.

Bean Huo

2022-07-28 21:26 UTC #10 lore raw

Quoted 11 lines · Bart Van Assche

Quoted 3 lines · On 7/28/22 00:16, [email protected] 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.

Quoted 4 lines

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

Quoted 3 lines

Thanks,
Bart.

Stanley Chu

2022-08-01 02:11 UTC #11 lore raw

Hi,

Quoted 37 lines · Bean Huo

Quoted 11 lines · Bart Van Assche
Quoted 3 lines · On 7/28/22 00:16, [email protected] 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.
Quoted 4 lines
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

Quoted 11 lines

allowed!\n");
                return -EOPNOTSUPP;
        }
Kind regards,
Bean
Quoted 3 lines
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

Peter Wang

author

2022-08-01 14:30 UTC #12 lore raw

In reply to Bart Van Assche

Quoted 11 lines · Bart Van Assche

Quoted 2 lines · On 7/28/22 00:16, [email protected] 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

Bart Van Assche

reviewer

2022-08-01 16:43 UTC #13 lore raw

Quoted 1 line · Peter Wang

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.

Asutosh Das (asd)

2022-08-01 17:59 UTC #14 lore raw

Hello,

Quoted 7 lines · Bart Van Assche

Quoted 1 line · Peter Wang
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,

Quoted 9 lines

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?

Quoted 3 lines

Thanks,
Bart.

Christoph Hellwig

2022-08-01 17:57 UTC #15 lore raw

In reply to [email protected]

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.

Bart Van Assche

reviewer

2022-08-01 18:12 UTC #16 lore raw

Quoted 2 lines · 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.

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

Bart.

Christoph Hellwig

2022-08-01 18:14 UTC #17 lore raw

Quoted 2 lines · Bart Van Assche

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.

Peter Wang

author

2022-08-02 03:25 UTC #18 lore raw

Quoted 5 lines · Christoph Hellwig

Quoted 2 lines · Bart Van Assche
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