diff mbox series

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

Message ID 20220802143223.1276-2-peter.wang@mediatek.com (mailing list archive)
State Superseded
Headers show
Series ufs: allow host driver disable wb toggle druing clock scaling | expand

Commit Message

Peter Wang (王信友) Aug. 2, 2022, 2:32 p.m. UTC 
From: Peter Wang <peter.wang@mediatek.com>

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 <peter.wang@mediatek.com>
Reviewed-by: Stanley Chu <stanley.chu@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(-)

Comments

Bart Van Assche Aug. 2, 2022, 6:44 p.m. UTC | #1 
On 8/2/22 07:32, peter.wang@mediatek.com 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".

> +	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_.

 > +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.
Peter Wang (王信友) Aug. 3, 2022, 2:44 a.m. UTC | #2 
Hi Bart,

On 8/3/22 2:44 AM, Bart Van Assche wrote:
> On 8/2/22 07:32, peter.wang@mediatek.com 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.

>
>> + 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.


> > +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
diff mbox series

Patch

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 3d367be71728..d86826e958e6 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))