Skip to contentSkip to conversation

ufs: allow host driver disable wb toggle druing clock scaling

PATCH Superseded [email protected] 2 participants 7 messages Aug 2, 2022 – Aug 3, 2022

Subsystems (5)
SCSI Arm Qualcomm mailing list Arm MediaTek UFS host controller +1 more subsystems

v3 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

[email protected]

author
#0 lore raw

v3 cover ufs: allow host driver disable wb toggle druing clock scaling

From: Peter Wang <[email protected]>

Mediatek ufs do not want to toggle write booster during clock scaling. This patch set allow host driver disable wb toggle during clock scaling.

Peter Wang (2):
ufs: core: introduce a choice of wb toggle during 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
author
#1 lore raw

PATCH v3 1/2 ufs: core: introduce a choice of wb toggle during clock scaling

From: Peter Wang <[email protected]>

Host driver may want to disable/enable write booster during 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]>
Reviewed-by Stanley Chu <[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 3d367be71728..d86826e958e6 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
reviewer
#2 lore raw

Re: [PATCH v3 1/2] ufs: core: introduce a choice of wb toggle during clock scaling

include/ufs/ufshcd.h enum ufshcd_caps { · line 652

Quoted 1 line · On 8/2/22 07:32, [email protected] wrote:
include/ufs/ufshcd.h +1 -0
+	/* Allow WB with clk scaling */

This comment is misleading. Consider changing this comment into something like "Enable WB when scaling up the clock and disable WB when scaling the clock down".

include/ufs/ufshcd.h enum ufshcd_caps { · line 653

Quoted 1 line
include/ufs/ufshcd.h +1 -0
+	UFSHCD_CAP_WB_WITH_CLK_SCALING			= 1 << 12,

Whether or not the WriteBooster is toggled when the clock is scaled is not a host controller capability. It is a policy that is tied by this patch to the host controller. So I think that using the "UFSHCD_CAP_" prefix is misleading. Consider using e.g. the prefix UFSHCD_POLICY_.

include/ufs/ufshcd.h static inline bool ufshcd_is_wb_allowed(struct ufs_hba *hba) · line 1013

Quoted 4 lines
include/ufs/ufshcd.h +4 -0
+static inline bool ufshcd_can_wb_during_scaling(struct ufs_hba *hba)
+{
+	return hba->caps & UFSHCD_CAP_WB_WITH_CLK_SCALING;
+}

The name of this function is misleading. Consider renaming this function into e.g. ufshcd_enable_wb_if_scaling_up().

Thanks,

Bart.

author
#3 lore raw

Re: [PATCH v3 1/2] ufs: core: introduce a choice of wb toggle during clock scaling

Hi Bart,

Quoted 4 lines · Bart Van Assche
Quoted 1 line · On 8/2/22 07:32, [email protected] wrote:

+    /* Allow WB with clk scaling */

This comment is misleading. Consider changing this comment into something like "Enable WB when scaling up the clock and disable WB when scaling the clock down".

Will change comment next version, thanks.

Quoted 5 lines
Quoted 2 lines

+ UFSHCD_CAP_WB_WITH_CLK_SCALING            = 1 << 12,

Whether or not the WriteBooster is toggled when the clock is scaled is not a host controller capability. It is a policy that is tied by this patch to the host controller. So I think that using the "UFSHCD_CAP_" prefix is misleading. Consider using e.g. the prefix UFSHCD_POLICY_.

The prefix UFSHCD_CAP_ is used in ufshcd_caps and not all of them is host capability. Ex. UFSHCD_CAP_HIBERN8_WITH_CLK_GATING is a policy host send hiberb8 with clk gating or not. Ex. UFSHCD_CAP_WB_EN is a host policy to turn-on WriteBooster or not.
So, I think it is not suitable to break the naming rule in ufshcd_caps now.

Quoted 10 lines

include/ufs/ufshcd.h static inline bool ufshcd_is_wb_allowed(struct ufs_hba *hba) · line 1013

Quoted 4 lines
include/ufs/ufshcd.h +4 -0
+static inline bool ufshcd_can_wb_during_scaling(struct ufs_hba *hba)
+{
+    return hba->caps & UFSHCD_CAP_WB_WITH_CLK_SCALING;
+}

The name of this function is misleading. Consider renaming this function into e.g. ufshcd_enable_wb_if_scaling_up().

Thanks,

Bart.

Will change function name next version.

Thanks.
Peter

author
#4 lore raw

PATCH v3 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]>
Reviewed-by Stanley Chu <[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
reviewer
#5 lore raw

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

Quoted 15 lines · On 8/2/22 07:32, [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]>
Reviewed-by Stanley Chu <[email protected]>
---
  drivers/ufs/host/ufs-qcom.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/ufs/host/ufs-qcom.c +0 -0
index f10d4668814c..f8c9a78e7776 100644

drivers/ufs/host/ufs-qcom.c static void ufs_qcom_set_caps(struct ufs_hba *hba) · line 875

Quoted 8 lines
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;
  	hba->caps |= UFSHCD_CAP_AUTO_BKOPS_SUSPEND;
  	hba->caps |= UFSHCD_CAP_WB_EN;
  	hba->caps |= UFSHCD_CAP_CRYPTO;

A patch series should be bisectable. Without this patch the previous patch in this series introduces a regression for Qualcomm controllers. So I think that the two patches should be combined into a single patch.

Thanks,

Bart.

author
#6 lore raw

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

Quoted 27 lines · Bart Van Assche
Quoted 15 lines · On 8/2/22 07:32, [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]>
Reviewed-by Stanley Chu <[email protected]>

---
  drivers/ufs/host/ufs-qcom.c | 2 +-

  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/ufs/host/ufs-qcom.c +0 -0
index f10d4668814c..f8c9a78e7776 100644

drivers/ufs/host/ufs-qcom.c static void ufs_qcom_set_caps(struct ufs_hba *hba) · line 875

Quoted 8 lines
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;
       hba->caps |= UFSHCD_CAP_AUTO_BKOPS_SUSPEND;
       hba->caps |= UFSHCD_CAP_WB_EN;
       hba->caps |= UFSHCD_CAP_CRYPTO;

A patch series should be bisectable. Without this patch the previous patch in this series introduces a regression for Qualcomm controllers. So I think that the two patches should be combined into a single patch.

Quoted 3 lines

Thanks,

Bart.

Hi Bart,

Will combine into a single patch next version.

Thanks.
Peter