diff mbox series

[v4] ufs: allow host driver disable wb toggle druing clock scaling

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

Commit Message

Peter Wang (王信友) Aug. 3, 2022, 3:03 a.m. UTC 
From: Peter Wang <peter.wang@mediatek.com>

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

So, 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.

Set UFSHCD_CAP_WB_WITH_CLK_SCALING for qcom to compatible legacy design in
the same time.

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 +++++---
 drivers/ufs/host/ufs-qcom.c  |  2 +-
 include/ufs/ufshcd.h         | 10 ++++++++++
 4 files changed, 18 insertions(+), 5 deletions(-)

Comments

Bart Van Assche Aug. 3, 2022, 7:28 p.m. UTC | #1 
On 8/2/22 20:03, peter.wang@mediatek.com wrote:
> 

disable -> to disable?

toggle -> toggling?

druing -> during?

> diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
> index 0a088b47d557..7f41f2a69b04 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_enable_wb_if_scaling_up(hba))) {

The "&&" is misplaced - it should occur at the end of the previous line. 
Isn't this something that checkpatch complains about?

>   	/* 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_enable_wb_if_scaling_up(hba)) {
> +		downgrade_write(&hba->clk_scaling_lock);
> +		is_writelock = false;
> +		ufshcd_wb_toggle(hba, scale_up);
> +	}

Since this code is being modified, please move the "/* Enable" comment 
to where it should occur (just above the ufshcd_wb_toggle() call).

> @@ -1004,6 +1010,10 @@ static inline bool ufshcd_is_wb_allowed(struct ufs_hba *hba)
>   {
>   	return hba->caps & UFSHCD_CAP_WB_EN;
>   }
> +static inline bool ufshcd_enable_wb_if_scaling_up(struct ufs_hba *hba)
> +{
> +	return hba->caps & UFSHCD_CAP_WB_WITH_CLK_SCALING;
> +}

It seems like a blank line is missing above the new function definition?

Otherwise this patch looks good to me.

Thanks,

Bart.
Peter Wang (王信友) Aug. 4, 2022, 2:49 a.m. UTC | #2 
On 8/4/22 3:28 AM, Bart Van Assche wrote:
> On 8/2/22 20:03, peter.wang@mediatek.com wrote:
>>
>
> disable -> to disable?
>
> toggle -> toggling?
>
> druing -> during?
>
>> diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
>> index 0a088b47d557..7f41f2a69b04 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_enable_wb_if_scaling_up(hba))) {
>
> The "&&" is misplaced - it should occur at the end of the previous 
> line. Isn't this something that checkpatch complains about?
>
>>       /* 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_enable_wb_if_scaling_up(hba)) {
>> +        downgrade_write(&hba->clk_scaling_lock);
>> +        is_writelock = false;
>> +        ufshcd_wb_toggle(hba, scale_up);
>> +    }
>
> Since this code is being modified, please move the "/* Enable" comment 
> to where it should occur (just above the ufshcd_wb_toggle() call).
>
>> @@ -1004,6 +1010,10 @@ static inline bool ufshcd_is_wb_allowed(struct 
>> ufs_hba *hba)
>>   {
>>       return hba->caps & UFSHCD_CAP_WB_EN;
>>   }
>> +static inline bool ufshcd_enable_wb_if_scaling_up(struct ufs_hba *hba)
>> +{
>> +    return hba->caps & UFSHCD_CAP_WB_WITH_CLK_SCALING;
>> +}
>
> It seems like a blank line is missing above the new function definition?
>
> Otherwise this patch looks good to me.
>
> Thanks,

Hi Bart,


Will fix next version.


Thanks

Peter

>
> Bart.
diff mbox series

Patch

diff --git a/drivers/ufs/core/ufs-sysfs.c b/drivers/ufs/core/ufs-sysfs.c
index 0a088b47d557..7f41f2a69b04 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_enable_wb_if_scaling_up(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..ac50fbe8aeb8 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_enable_wb_if_scaling_up(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/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;
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index a92271421718..5a8fabb9f008 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -648,6 +648,12 @@  enum ufshcd_caps {
 	 * notification if it is supported by the UFS device.
 	 */
 	UFSHCD_CAP_TEMP_NOTIF				= 1 << 11,
+
+	/*
+	 * Enable WriteBooster when scaling up the clock and disable
+	 * WriteBooster when scaling the clock down.
+	 */
+	UFSHCD_CAP_WB_WITH_CLK_SCALING			= 1 << 12,
 };
 
 struct ufs_hba_variant_params {
@@ -1004,6 +1010,10 @@  static inline bool ufshcd_is_wb_allowed(struct ufs_hba *hba)
 {
 	return hba->caps & UFSHCD_CAP_WB_EN;
 }
+static inline bool ufshcd_enable_wb_if_scaling_up(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))