[PATCH 0/8] Support Multi-frequency scale for UFS

[PATCH 0/8] Support Multi-frequency scale for UFS

[Ziqi Chen]

With OPP V2 enabled, devfreq can scale clocks amongst multiple frequency
plans. However, the gear speed is only toggled between min and max during
clock scaling. Enable multi-level gear scaling by mapping clock frequencies
to gear speeds, so that when devfreq scales clock frequencies we can put
the UFS link at the appropraite gear speeds accordingly.

This series has been tested on below platforms -
SM8650 + UFS3.1
SM8750 + UFS4.0


Can Guo (6):
  scsi: ufs: core: Pass target_freq to clk_scale_notify() vops
  scsi: ufs: qcom: Pass target_freq to clk scale pre and post change
  scsi: ufs: core: Add a vops to map clock frequency to gear speed
  scsi: ufs: qcom: Implement the freq_to_gear_speed() vops
  scsi: ufs: core: Enable multi-level gear scaling
  scsi: ufs: core: Toggle Write Booster during clock scaling base on
    gear speed

Ziqi Chen (2):
  scsi: ufs: core: Check if scaling up is required when disable clkscale
  ARM: dts: msm: Use Operation Points V2 for UFS on SM8650

 arch/arm64/boot/dts/qcom/sm8650.dtsi | 51 ++++++++++++++++----
 drivers/ufs/core/ufshcd-priv.h       | 17 +++++--
 drivers/ufs/core/ufshcd.c            | 71 +++++++++++++++++++++-------
 drivers/ufs/host/ufs-mediatek.c      |  1 +
 drivers/ufs/host/ufs-qcom.c          | 60 ++++++++++++++++++-----
 include/ufs/ufshcd.h                 |  8 +++-
 6 files changed, 166 insertions(+), 42 deletions(-)

-- 
2.34.1

[PATCH 1/8] scsi: ufs: core: Pass target_freq to clk_scale_notify() vops

[Ziqi Chen]

From: Can Guo <quic_cang@quicinc.com>

If OPP V2 is used, devfreq clock scaling may scale clock amongst more than
two freqs, so just passing up/down to vops clk_scale_notify() is not enough
to cover the intermediate clock freqs between the min and max freqs. Hence
pass the target_freq to clk_scale_notify() to allow the vops to perform
corresponding configurations with regard to the clock freqs.

Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
Signed-off-by: Can Guo <quic_cang@quicinc.com>
---
 drivers/ufs/core/ufshcd-priv.h  | 7 ++++---
 drivers/ufs/core/ufshcd.c       | 4 ++--
 drivers/ufs/host/ufs-mediatek.c | 1 +
 drivers/ufs/host/ufs-qcom.c     | 5 +++--
 include/ufs/ufshcd.h            | 2 +-
 5 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index 9ffd94ddf8c7..0549b65f71ed 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -117,11 +117,12 @@ static inline u32 ufshcd_vops_get_ufs_hci_version(struct ufs_hba *hba)
 	return ufshcd_readl(hba, REG_UFS_VERSION);
 }
 
-static inline int ufshcd_vops_clk_scale_notify(struct ufs_hba *hba,
-			bool up, enum ufs_notify_change_status status)
+static inline int ufshcd_vops_clk_scale_notify(struct ufs_hba *hba, bool up,
+					       unsigned long target_freq,
+					       enum ufs_notify_change_status status)
 {
 	if (hba->vops && hba->vops->clk_scale_notify)
-		return hba->vops->clk_scale_notify(hba, up, status);
+		return hba->vops->clk_scale_notify(hba, up, target_freq, status);
 	return 0;
 }
 
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index acc3607bbd9c..8d295cc827cc 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -1157,7 +1157,7 @@ static int ufshcd_scale_clks(struct ufs_hba *hba, unsigned long freq,
 	int ret = 0;
 	ktime_t start = ktime_get();
 
-	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, PRE_CHANGE);
+	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, freq, PRE_CHANGE);
 	if (ret)
 		goto out;
 
@@ -1168,7 +1168,7 @@ static int ufshcd_scale_clks(struct ufs_hba *hba, unsigned long freq,
 	if (ret)
 		goto out;
 
-	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, POST_CHANGE);
+	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, freq, POST_CHANGE);
 	if (ret) {
 		if (hba->use_pm_opp)
 			ufshcd_opp_set_rate(hba,
diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
index 135cd78109e2..977dd0caaef6 100644
--- a/drivers/ufs/host/ufs-mediatek.c
+++ b/drivers/ufs/host/ufs-mediatek.c
@@ -1643,6 +1643,7 @@ static void ufs_mtk_clk_scale(struct ufs_hba *hba, bool scale_up)
 }
 
 static int ufs_mtk_clk_scale_notify(struct ufs_hba *hba, bool scale_up,
+				    unsigned long target_freq,
 				    enum ufs_notify_change_status status)
 {
 	if (!ufshcd_is_clkscaling_supported(hba))
diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 68040b2ab5f8..b6eef975dc46 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1333,8 +1333,9 @@ static int ufs_qcom_clk_scale_down_post_change(struct ufs_hba *hba)
 	return ufs_qcom_set_core_clk_ctrl(hba, false);
 }
 
-static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba,
-		bool scale_up, enum ufs_notify_change_status status)
+static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, bool scale_up,
+				     unsigned long target_freq,
+				     enum ufs_notify_change_status status)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
 	int err;
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index d7aca9e61684..a4dac897a169 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -344,7 +344,7 @@ struct ufs_hba_variant_ops {
 	void    (*exit)(struct ufs_hba *);
 	u32	(*get_ufs_hci_version)(struct ufs_hba *);
 	int	(*set_dma_mask)(struct ufs_hba *);
-	int	(*clk_scale_notify)(struct ufs_hba *, bool,
+	int (*clk_scale_notify)(struct ufs_hba *, bool, unsigned long,
 				    enum ufs_notify_change_status);
 	int	(*setup_clocks)(struct ufs_hba *, bool,
 				enum ufs_notify_change_status);
-- 
2.34.1

Re: [PATCH 1/8] scsi: ufs: core: Pass target_freq to clk_scale_notify() vops

[Manivannan Sadhasivam]

On Thu, Jan 16, 2025 at 05:11:42PM +0800, Ziqi Chen wrote:
> From: Can Guo <quic_cang@quicinc.com>
> 
> If OPP V2 is used, devfreq clock scaling may scale clock amongst more than
> two freqs,

'amongst more than two freqs': I couldn't parse this.

> so just passing up/down to vops clk_scale_notify() is not enough
> to cover the intermediate clock freqs between the min and max freqs. Hence
> pass the target_freq to clk_scale_notify() to allow the vops to perform
> corresponding configurations with regard to the clock freqs.
> 

Add a note that the 'target_freq' is not used in this commit.

> Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> Signed-off-by: Can Guo <quic_cang@quicinc.com>

Signed-off-by tag order is not correct here. This implies that Ziqi originally
worked on it, then Can took over and submitted. But it seems the reverse.

- Mani

> ---
>  drivers/ufs/core/ufshcd-priv.h  | 7 ++++---
>  drivers/ufs/core/ufshcd.c       | 4 ++--
>  drivers/ufs/host/ufs-mediatek.c | 1 +
>  drivers/ufs/host/ufs-qcom.c     | 5 +++--
>  include/ufs/ufshcd.h            | 2 +-
>  5 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
> index 9ffd94ddf8c7..0549b65f71ed 100644
> --- a/drivers/ufs/core/ufshcd-priv.h
> +++ b/drivers/ufs/core/ufshcd-priv.h
> @@ -117,11 +117,12 @@ static inline u32 ufshcd_vops_get_ufs_hci_version(struct ufs_hba *hba)
>  	return ufshcd_readl(hba, REG_UFS_VERSION);
>  }
>  
> -static inline int ufshcd_vops_clk_scale_notify(struct ufs_hba *hba,
> -			bool up, enum ufs_notify_change_status status)
> +static inline int ufshcd_vops_clk_scale_notify(struct ufs_hba *hba, bool up,
> +					       unsigned long target_freq,
> +					       enum ufs_notify_change_status status)
>  {
>  	if (hba->vops && hba->vops->clk_scale_notify)
> -		return hba->vops->clk_scale_notify(hba, up, status);
> +		return hba->vops->clk_scale_notify(hba, up, target_freq, status);
>  	return 0;
>  }
>  
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index acc3607bbd9c..8d295cc827cc 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -1157,7 +1157,7 @@ static int ufshcd_scale_clks(struct ufs_hba *hba, unsigned long freq,
>  	int ret = 0;
>  	ktime_t start = ktime_get();
>  
> -	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, PRE_CHANGE);
> +	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, freq, PRE_CHANGE);
>  	if (ret)
>  		goto out;
>  
> @@ -1168,7 +1168,7 @@ static int ufshcd_scale_clks(struct ufs_hba *hba, unsigned long freq,
>  	if (ret)
>  		goto out;
>  
> -	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, POST_CHANGE);
> +	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, freq, POST_CHANGE);
>  	if (ret) {
>  		if (hba->use_pm_opp)
>  			ufshcd_opp_set_rate(hba,
> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
> index 135cd78109e2..977dd0caaef6 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c
> @@ -1643,6 +1643,7 @@ static void ufs_mtk_clk_scale(struct ufs_hba *hba, bool scale_up)
>  }
>  
>  static int ufs_mtk_clk_scale_notify(struct ufs_hba *hba, bool scale_up,
> +				    unsigned long target_freq,
>  				    enum ufs_notify_change_status status)
>  {
>  	if (!ufshcd_is_clkscaling_supported(hba))
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 68040b2ab5f8..b6eef975dc46 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -1333,8 +1333,9 @@ static int ufs_qcom_clk_scale_down_post_change(struct ufs_hba *hba)
>  	return ufs_qcom_set_core_clk_ctrl(hba, false);
>  }
>  
> -static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba,
> -		bool scale_up, enum ufs_notify_change_status status)
> +static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, bool scale_up,
> +				     unsigned long target_freq,
> +				     enum ufs_notify_change_status status)
>  {
>  	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>  	int err;
> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index d7aca9e61684..a4dac897a169 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -344,7 +344,7 @@ struct ufs_hba_variant_ops {
>  	void    (*exit)(struct ufs_hba *);
>  	u32	(*get_ufs_hci_version)(struct ufs_hba *);
>  	int	(*set_dma_mask)(struct ufs_hba *);
> -	int	(*clk_scale_notify)(struct ufs_hba *, bool,
> +	int (*clk_scale_notify)(struct ufs_hba *, bool, unsigned long,
>  				    enum ufs_notify_change_status);
>  	int	(*setup_clocks)(struct ufs_hba *, bool,
>  				enum ufs_notify_change_status);
> -- 
> 2.34.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 1/8] scsi: ufs: core: Pass target_freq to clk_scale_notify() vops

[Ziqi Chen]

Hi Mani,

Thanks for you review~

On 1/19/2025 3:11 PM, Manivannan Sadhasivam wrote:
> On Thu, Jan 16, 2025 at 05:11:42PM +0800, Ziqi Chen wrote:
>> From: Can Guo <quic_cang@quicinc.com>
>>
>> If OPP V2 is used, devfreq clock scaling may scale clock amongst more than
>> two freqs,
> 
> 'amongst more than two freqs': I couldn't parse this.
>

It means that the devfreq framework will tell UFS core driver the 
devfreq freq, then UFS core driver will find the recommended freq from 
our freq table based on the devfreq freq. For legacy mode , we can only 
have 2 frequencies in the table. But if the OPP V2 is used, we can have 
3 , 4 or more freq tables. You can refer to my PATCH 8/8.

>> so just passing up/down to vops clk_scale_notify() is not enough
>> to cover the intermediate clock freqs between the min and max freqs. Hence
>> pass the target_freq to clk_scale_notify() to allow the vops to perform
>> corresponding configurations with regard to the clock freqs.
>>
> 
> Add a note that the 'target_freq' is not used in this commit.
>

Sorry, I don't very understand this comment, I mentioned the 
"target_freq" in the commit message,  Could you let me know what note 
you want me do add?

>> Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>> Signed-off-by: Can Guo <quic_cang@quicinc.com>
> 
> Signed-off-by tag order is not correct here. This implies that Ziqi originally
> worked on it, then Can took over and submitted. But it seems the reverse.

Thanks for your reminder. Is below tag order OK ?
     Signed-off-by: Can Guo <quic_cang@quicinc.com>
     Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
     Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> 
> - Mani

-Ziqi

> 
>> ---
>>   drivers/ufs/core/ufshcd-priv.h  | 7 ++++---
>>   drivers/ufs/core/ufshcd.c       | 4 ++--
>>   drivers/ufs/host/ufs-mediatek.c | 1 +
>>   drivers/ufs/host/ufs-qcom.c     | 5 +++--
>>   include/ufs/ufshcd.h            | 2 +-
>>   5 files changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
>> index 9ffd94ddf8c7..0549b65f71ed 100644
>> --- a/drivers/ufs/core/ufshcd-priv.h
>> +++ b/drivers/ufs/core/ufshcd-priv.h
>> @@ -117,11 +117,12 @@ static inline u32 ufshcd_vops_get_ufs_hci_version(struct ufs_hba *hba)
>>   	return ufshcd_readl(hba, REG_UFS_VERSION);
>>   }
>>   
>> -static inline int ufshcd_vops_clk_scale_notify(struct ufs_hba *hba,
>> -			bool up, enum ufs_notify_change_status status)
>> +static inline int ufshcd_vops_clk_scale_notify(struct ufs_hba *hba, bool up,
>> +					       unsigned long target_freq,
>> +					       enum ufs_notify_change_status status)
>>   {
>>   	if (hba->vops && hba->vops->clk_scale_notify)
>> -		return hba->vops->clk_scale_notify(hba, up, status);
>> +		return hba->vops->clk_scale_notify(hba, up, target_freq, status);
>>   	return 0;
>>   }
>>   
>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>> index acc3607bbd9c..8d295cc827cc 100644
>> --- a/drivers/ufs/core/ufshcd.c
>> +++ b/drivers/ufs/core/ufshcd.c
>> @@ -1157,7 +1157,7 @@ static int ufshcd_scale_clks(struct ufs_hba *hba, unsigned long freq,
>>   	int ret = 0;
>>   	ktime_t start = ktime_get();
>>   
>> -	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, PRE_CHANGE);
>> +	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, freq, PRE_CHANGE);
>>   	if (ret)
>>   		goto out;
>>   
>> @@ -1168,7 +1168,7 @@ static int ufshcd_scale_clks(struct ufs_hba *hba, unsigned long freq,
>>   	if (ret)
>>   		goto out;
>>   
>> -	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, POST_CHANGE);
>> +	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, freq, POST_CHANGE);
>>   	if (ret) {
>>   		if (hba->use_pm_opp)
>>   			ufshcd_opp_set_rate(hba,
>> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
>> index 135cd78109e2..977dd0caaef6 100644
>> --- a/drivers/ufs/host/ufs-mediatek.c
>> +++ b/drivers/ufs/host/ufs-mediatek.c
>> @@ -1643,6 +1643,7 @@ static void ufs_mtk_clk_scale(struct ufs_hba *hba, bool scale_up)
>>   }
>>   
>>   static int ufs_mtk_clk_scale_notify(struct ufs_hba *hba, bool scale_up,
>> +				    unsigned long target_freq,
>>   				    enum ufs_notify_change_status status)
>>   {
>>   	if (!ufshcd_is_clkscaling_supported(hba))
>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>> index 68040b2ab5f8..b6eef975dc46 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -1333,8 +1333,9 @@ static int ufs_qcom_clk_scale_down_post_change(struct ufs_hba *hba)
>>   	return ufs_qcom_set_core_clk_ctrl(hba, false);
>>   }
>>   
>> -static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba,
>> -		bool scale_up, enum ufs_notify_change_status status)
>> +static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, bool scale_up,
>> +				     unsigned long target_freq,
>> +				     enum ufs_notify_change_status status)
>>   {
>>   	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>>   	int err;
>> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
>> index d7aca9e61684..a4dac897a169 100644
>> --- a/include/ufs/ufshcd.h
>> +++ b/include/ufs/ufshcd.h
>> @@ -344,7 +344,7 @@ struct ufs_hba_variant_ops {
>>   	void    (*exit)(struct ufs_hba *);
>>   	u32	(*get_ufs_hci_version)(struct ufs_hba *);
>>   	int	(*set_dma_mask)(struct ufs_hba *);
>> -	int	(*clk_scale_notify)(struct ufs_hba *, bool,
>> +	int (*clk_scale_notify)(struct ufs_hba *, bool, unsigned long,
>>   				    enum ufs_notify_change_status);
>>   	int	(*setup_clocks)(struct ufs_hba *, bool,
>>   				enum ufs_notify_change_status);
>> -- 
>> 2.34.1
>>
> 

Re: [PATCH 1/8] scsi: ufs: core: Pass target_freq to clk_scale_notify() vops

[Manivannan Sadhasivam]

On Mon, Jan 20, 2025 at 08:01:03PM +0800, Ziqi Chen wrote:
> Hi Mani,
> 
> Thanks for you review~
> 
> On 1/19/2025 3:11 PM, Manivannan Sadhasivam wrote:
> > On Thu, Jan 16, 2025 at 05:11:42PM +0800, Ziqi Chen wrote:
> > > From: Can Guo <quic_cang@quicinc.com>
> > > 
> > > If OPP V2 is used, devfreq clock scaling may scale clock amongst more than
> > > two freqs,
> > 
> > 'amongst more than two freqs': I couldn't parse this.
> > 
> 
> It means that the devfreq framework will tell UFS core driver the devfreq
> freq, then UFS core driver will find the recommended freq from our freq
> table based on the devfreq freq. For legacy mode , we can only have 2
> frequencies in the table. But if the OPP V2 is used, we can have 3 , 4 or
> more freq tables. You can refer to my PATCH 8/8.
> 

I got the motive, but the wording is not correct.

> > > so just passing up/down to vops clk_scale_notify() is not enough
> > > to cover the intermediate clock freqs between the min and max freqs. Hence
> > > pass the target_freq to clk_scale_notify() to allow the vops to perform
> > > corresponding configurations with regard to the clock freqs.
> > > 
> > 
> > Add a note that the 'target_freq' is not used in this commit.
> > 
> 
> Sorry, I don't very understand this comment, I mentioned the "target_freq"
> in the commit message,  Could you let me know what note you want me do add?
> 

This patch is introducing 'target_freq' as a parameter, but it is not used. I
was asking you to mention that it will be used in successive commits.

> > > Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> > > Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> > > Signed-off-by: Can Guo <quic_cang@quicinc.com>
> > 
> > Signed-off-by tag order is not correct here. This implies that Ziqi originally
> > worked on it, then Can took over and submitted. But it seems the reverse.
> 
> Thanks for your reminder. Is below tag order OK ?
>     Signed-off-by: Can Guo <quic_cang@quicinc.com>
>     Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>     Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> > 
> > - Mani
> 
> -Ziqi
> 
> > 
> > > ---
> > >   drivers/ufs/core/ufshcd-priv.h  | 7 ++++---
> > >   drivers/ufs/core/ufshcd.c       | 4 ++--
> > >   drivers/ufs/host/ufs-mediatek.c | 1 +
> > >   drivers/ufs/host/ufs-qcom.c     | 5 +++--
> > >   include/ufs/ufshcd.h            | 2 +-
> > >   5 files changed, 11 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
> > > index 9ffd94ddf8c7..0549b65f71ed 100644
> > > --- a/drivers/ufs/core/ufshcd-priv.h
> > > +++ b/drivers/ufs/core/ufshcd-priv.h
> > > @@ -117,11 +117,12 @@ static inline u32 ufshcd_vops_get_ufs_hci_version(struct ufs_hba *hba)
> > >   	return ufshcd_readl(hba, REG_UFS_VERSION);
> > >   }
> > > -static inline int ufshcd_vops_clk_scale_notify(struct ufs_hba *hba,
> > > -			bool up, enum ufs_notify_change_status status)
> > > +static inline int ufshcd_vops_clk_scale_notify(struct ufs_hba *hba, bool up,
> > > +					       unsigned long target_freq,
> > > +					       enum ufs_notify_change_status status)
> > >   {
> > >   	if (hba->vops && hba->vops->clk_scale_notify)
> > > -		return hba->vops->clk_scale_notify(hba, up, status);
> > > +		return hba->vops->clk_scale_notify(hba, up, target_freq, status);
> > >   	return 0;
> > >   }
> > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> > > index acc3607bbd9c..8d295cc827cc 100644
> > > --- a/drivers/ufs/core/ufshcd.c
> > > +++ b/drivers/ufs/core/ufshcd.c
> > > @@ -1157,7 +1157,7 @@ static int ufshcd_scale_clks(struct ufs_hba *hba, unsigned long freq,
> > >   	int ret = 0;
> > >   	ktime_t start = ktime_get();
> > > -	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, PRE_CHANGE);
> > > +	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, freq, PRE_CHANGE);
> > >   	if (ret)
> > >   		goto out;
> > > @@ -1168,7 +1168,7 @@ static int ufshcd_scale_clks(struct ufs_hba *hba, unsigned long freq,
> > >   	if (ret)
> > >   		goto out;
> > > -	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, POST_CHANGE);
> > > +	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, freq, POST_CHANGE);
> > >   	if (ret) {
> > >   		if (hba->use_pm_opp)
> > >   			ufshcd_opp_set_rate(hba,
> > > diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
> > > index 135cd78109e2..977dd0caaef6 100644
> > > --- a/drivers/ufs/host/ufs-mediatek.c
> > > +++ b/drivers/ufs/host/ufs-mediatek.c
> > > @@ -1643,6 +1643,7 @@ static void ufs_mtk_clk_scale(struct ufs_hba *hba, bool scale_up)
> > >   }
> > >   static int ufs_mtk_clk_scale_notify(struct ufs_hba *hba, bool scale_up,
> > > +				    unsigned long target_freq,
> > >   				    enum ufs_notify_change_status status)
> > >   {
> > >   	if (!ufshcd_is_clkscaling_supported(hba))
> > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> > > index 68040b2ab5f8..b6eef975dc46 100644
> > > --- a/drivers/ufs/host/ufs-qcom.c
> > > +++ b/drivers/ufs/host/ufs-qcom.c
> > > @@ -1333,8 +1333,9 @@ static int ufs_qcom_clk_scale_down_post_change(struct ufs_hba *hba)
> > >   	return ufs_qcom_set_core_clk_ctrl(hba, false);
> > >   }
> > > -static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba,
> > > -		bool scale_up, enum ufs_notify_change_status status)
> > > +static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, bool scale_up,
> > > +				     unsigned long target_freq,
> > > +				     enum ufs_notify_change_status status)
> > >   {
> > >   	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
> > >   	int err;
> > > diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> > > index d7aca9e61684..a4dac897a169 100644
> > > --- a/include/ufs/ufshcd.h
> > > +++ b/include/ufs/ufshcd.h
> > > @@ -344,7 +344,7 @@ struct ufs_hba_variant_ops {
> > >   	void    (*exit)(struct ufs_hba *);
> > >   	u32	(*get_ufs_hci_version)(struct ufs_hba *);
> > >   	int	(*set_dma_mask)(struct ufs_hba *);
> > > -	int	(*clk_scale_notify)(struct ufs_hba *, bool,
> > > +	int (*clk_scale_notify)(struct ufs_hba *, bool, unsigned long,
> > >   				    enum ufs_notify_change_status);
> > >   	int	(*setup_clocks)(struct ufs_hba *, bool,
> > >   				enum ufs_notify_change_status);
> > > -- 
> > > 2.34.1
> > > 
> > 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 1/8] scsi: ufs: core: Pass target_freq to clk_scale_notify() vops

[Ziqi Chen]

On 1/20/2025 11:36 PM, Manivannan Sadhasivam wrote:
> On Mon, Jan 20, 2025 at 08:01:03PM +0800, Ziqi Chen wrote:
>> Hi Mani,
>>
>> Thanks for you review~
>>
>> On 1/19/2025 3:11 PM, Manivannan Sadhasivam wrote:
>>> On Thu, Jan 16, 2025 at 05:11:42PM +0800, Ziqi Chen wrote:
>>>> From: Can Guo <quic_cang@quicinc.com>
>>>>
>>>> If OPP V2 is used, devfreq clock scaling may scale clock amongst more than
>>>> two freqs,
>>>
>>> 'amongst more than two freqs': I couldn't parse this.
>>>
>>
>> It means that the devfreq framework will tell UFS core driver the devfreq
>> freq, then UFS core driver will find the recommended freq from our freq
>> table based on the devfreq freq. For legacy mode , we can only have 2
>> frequencies in the table. But if the OPP V2 is used, we can have 3 , 4 or
>> more freq tables. You can refer to my PATCH 8/8.
>>
> 
> I got the motive, but the wording is not correct.
> 

OK , let me express it in another way:
"Instead of only two frequencies, If OPP V2 is used, the UFS devfreq 
clock scaling may scale the clock among multiple frequencies."
How about this?

>>>> so just passing up/down to vops clk_scale_notify() is not enough
>>>> to cover the intermediate clock freqs between the min and max freqs. Hence
>>>> pass the target_freq to clk_scale_notify() to allow the vops to perform
>>>> corresponding configurations with regard to the clock freqs.
>>>>
>>>
>>> Add a note that the 'target_freq' is not used in this commit.
>>>
>>
>> Sorry, I don't very understand this comment, I mentioned the "target_freq"
>> in the commit message,  Could you let me know what note you want me do add?
>>
> 
> This patch is introducing 'target_freq' as a parameter, but it is not used. I
> was asking you to mention that it will be used in successive commits.
> 

OK , thank you, I will add this note in next version.

-Ziqi

>>>> Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>>>> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>>>> Signed-off-by: Can Guo <quic_cang@quicinc.com>
>>>
>>> Signed-off-by tag order is not correct here. This implies that Ziqi originally
>>> worked on it, then Can took over and submitted. But it seems the reverse.
>>
>> Thanks for your reminder. Is below tag order OK ?
>>      Signed-off-by: Can Guo <quic_cang@quicinc.com>
>>      Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>>      Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>>>
>>> - Mani
>>
>> -Ziqi
>>
>>>
>>>> ---
>>>>    drivers/ufs/core/ufshcd-priv.h  | 7 ++++---
>>>>    drivers/ufs/core/ufshcd.c       | 4 ++--
>>>>    drivers/ufs/host/ufs-mediatek.c | 1 +
>>>>    drivers/ufs/host/ufs-qcom.c     | 5 +++--
>>>>    include/ufs/ufshcd.h            | 2 +-
>>>>    5 files changed, 11 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
>>>> index 9ffd94ddf8c7..0549b65f71ed 100644
>>>> --- a/drivers/ufs/core/ufshcd-priv.h
>>>> +++ b/drivers/ufs/core/ufshcd-priv.h
>>>> @@ -117,11 +117,12 @@ static inline u32 ufshcd_vops_get_ufs_hci_version(struct ufs_hba *hba)
>>>>    	return ufshcd_readl(hba, REG_UFS_VERSION);
>>>>    }
>>>> -static inline int ufshcd_vops_clk_scale_notify(struct ufs_hba *hba,
>>>> -			bool up, enum ufs_notify_change_status status)
>>>> +static inline int ufshcd_vops_clk_scale_notify(struct ufs_hba *hba, bool up,
>>>> +					       unsigned long target_freq,
>>>> +					       enum ufs_notify_change_status status)
>>>>    {
>>>>    	if (hba->vops && hba->vops->clk_scale_notify)
>>>> -		return hba->vops->clk_scale_notify(hba, up, status);
>>>> +		return hba->vops->clk_scale_notify(hba, up, target_freq, status);
>>>>    	return 0;
>>>>    }
>>>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>>>> index acc3607bbd9c..8d295cc827cc 100644
>>>> --- a/drivers/ufs/core/ufshcd.c
>>>> +++ b/drivers/ufs/core/ufshcd.c
>>>> @@ -1157,7 +1157,7 @@ static int ufshcd_scale_clks(struct ufs_hba *hba, unsigned long freq,
>>>>    	int ret = 0;
>>>>    	ktime_t start = ktime_get();
>>>> -	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, PRE_CHANGE);
>>>> +	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, freq, PRE_CHANGE);
>>>>    	if (ret)
>>>>    		goto out;
>>>> @@ -1168,7 +1168,7 @@ static int ufshcd_scale_clks(struct ufs_hba *hba, unsigned long freq,
>>>>    	if (ret)
>>>>    		goto out;
>>>> -	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, POST_CHANGE);
>>>> +	ret = ufshcd_vops_clk_scale_notify(hba, scale_up, freq, POST_CHANGE);
>>>>    	if (ret) {
>>>>    		if (hba->use_pm_opp)
>>>>    			ufshcd_opp_set_rate(hba,
>>>> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c
>>>> index 135cd78109e2..977dd0caaef6 100644
>>>> --- a/drivers/ufs/host/ufs-mediatek.c
>>>> +++ b/drivers/ufs/host/ufs-mediatek.c
>>>> @@ -1643,6 +1643,7 @@ static void ufs_mtk_clk_scale(struct ufs_hba *hba, bool scale_up)
>>>>    }
>>>>    static int ufs_mtk_clk_scale_notify(struct ufs_hba *hba, bool scale_up,
>>>> +				    unsigned long target_freq,
>>>>    				    enum ufs_notify_change_status status)
>>>>    {
>>>>    	if (!ufshcd_is_clkscaling_supported(hba))
>>>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>>>> index 68040b2ab5f8..b6eef975dc46 100644
>>>> --- a/drivers/ufs/host/ufs-qcom.c
>>>> +++ b/drivers/ufs/host/ufs-qcom.c
>>>> @@ -1333,8 +1333,9 @@ static int ufs_qcom_clk_scale_down_post_change(struct ufs_hba *hba)
>>>>    	return ufs_qcom_set_core_clk_ctrl(hba, false);
>>>>    }
>>>> -static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba,
>>>> -		bool scale_up, enum ufs_notify_change_status status)
>>>> +static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, bool scale_up,
>>>> +				     unsigned long target_freq,
>>>> +				     enum ufs_notify_change_status status)
>>>>    {
>>>>    	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>>>>    	int err;
>>>> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
>>>> index d7aca9e61684..a4dac897a169 100644
>>>> --- a/include/ufs/ufshcd.h
>>>> +++ b/include/ufs/ufshcd.h
>>>> @@ -344,7 +344,7 @@ struct ufs_hba_variant_ops {
>>>>    	void    (*exit)(struct ufs_hba *);
>>>>    	u32	(*get_ufs_hci_version)(struct ufs_hba *);
>>>>    	int	(*set_dma_mask)(struct ufs_hba *);
>>>> -	int	(*clk_scale_notify)(struct ufs_hba *, bool,
>>>> +	int (*clk_scale_notify)(struct ufs_hba *, bool, unsigned long,
>>>>    				    enum ufs_notify_change_status);
>>>>    	int	(*setup_clocks)(struct ufs_hba *, bool,
>>>>    				enum ufs_notify_change_status);
>>>> -- 
>>>> 2.34.1
>>>>
>>>
> 

Re: [PATCH 1/8] scsi: ufs: core: Pass target_freq to clk_scale_notify() vops

[Manivannan Sadhasivam]

On Mon, Jan 20, 2025 at 08:01:03PM +0800, Ziqi Chen wrote:
> Hi Mani,
> 
> Thanks for you review~
> 
> On 1/19/2025 3:11 PM, Manivannan Sadhasivam wrote:
> > On Thu, Jan 16, 2025 at 05:11:42PM +0800, Ziqi Chen wrote:
> > > From: Can Guo <quic_cang@quicinc.com>
> > > 
> > > If OPP V2 is used, devfreq clock scaling may scale clock amongst more than
> > > two freqs,
> > 
> > 'amongst more than two freqs': I couldn't parse this.
> > 
> 
> It means that the devfreq framework will tell UFS core driver the devfreq
> freq, then UFS core driver will find the recommended freq from our freq
> table based on the devfreq freq. For legacy mode , we can only have 2
> frequencies in the table. But if the OPP V2 is used, we can have 3 , 4 or
> more freq tables. You can refer to my PATCH 8/8.
> 
> > > so just passing up/down to vops clk_scale_notify() is not enough
> > > to cover the intermediate clock freqs between the min and max freqs. Hence
> > > pass the target_freq to clk_scale_notify() to allow the vops to perform
> > > corresponding configurations with regard to the clock freqs.
> > > 
> > 
> > Add a note that the 'target_freq' is not used in this commit.
> > 
> 
> Sorry, I don't very understand this comment, I mentioned the "target_freq"
> in the commit message,  Could you let me know what note you want me do add?
> 
> > > Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> > > Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> > > Signed-off-by: Can Guo <quic_cang@quicinc.com>
> > 
> > Signed-off-by tag order is not correct here. This implies that Ziqi originally
> > worked on it, then Can took over and submitted. But it seems the reverse.
> 
> Thanks for your reminder. Is below tag order OK ?
>     Signed-off-by: Can Guo <quic_cang@quicinc.com>
>     Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>

'Co-developed-by' is not needed unless you also worked on the patch. I guess you
are just sending the patch authored by Can, so you can drop this and keep your
'Signed-off-by' tag.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

[PATCH 2/8] scsi: ufs: qcom: Pass target_freq to clk scale pre and post change

[Ziqi Chen]

From: Can Guo <quic_cang@quicinc.com>

If OPP V2 is used, devfreq clock scaling may scale clock amongst more than
two freqs. In the case of scaling up, the devfreq may decide to scale the
clock to an intermidiate freq base on load, but the clock scale up pre
change operation uses settings for the max clock freq unconditionally. Fix
it by passing the target_freq to clock scale up pre change so that the
correct settings for the target_freq can be used.

In the case of scaling down, the clock scale down post change operation
is doing fine, because it reads the actual clock rate to tell freq, but to
keep symmetry with clock scale up pre change operation, just use the
target_freq instead of reading clock rate.

Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
Signed-off-by: Can Guo <quic_cang@quicinc.com>
---
 drivers/ufs/host/ufs-qcom.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index b6eef975dc46..1e8a23eb8c13 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -97,7 +97,7 @@ static const struct __ufs_qcom_bw_table {
 };
 
 static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host);
-static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up);
+static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, unsigned long freq);
 
 static struct ufs_qcom_host *rcdev_to_ufs_host(struct reset_controller_dev *rcd)
 {
@@ -524,7 +524,7 @@ static int ufs_qcom_link_startup_notify(struct ufs_hba *hba,
 			return -EINVAL;
 		}
 
-		err = ufs_qcom_set_core_clk_ctrl(hba, true);
+		err = ufs_qcom_set_core_clk_ctrl(hba, ULONG_MAX);
 		if (err)
 			dev_err(hba->dev, "cfg core clk ctrl failed\n");
 		/*
@@ -1231,7 +1231,7 @@ static int ufs_qcom_set_clk_40ns_cycles(struct ufs_hba *hba,
 	return ufshcd_dme_set(hba, UIC_ARG_MIB(PA_VS_CORE_CLK_40NS_CYCLES), reg);
 }
 
-static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up)
+static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, unsigned long freq)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
 	struct list_head *head = &hba->clk_list_head;
@@ -1245,10 +1245,11 @@ static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up)
 		    !strcmp(clki->name, "core_clk_unipro")) {
 			if (!clki->max_freq)
 				cycles_in_1us = 150; /* default for backwards compatibility */
-			else if (is_scale_up)
+			else if (freq == ULONG_MAX)
 				cycles_in_1us = ceil(clki->max_freq, (1000 * 1000));
 			else
-				cycles_in_1us = ceil(clk_get_rate(clki->clk), (1000 * 1000));
+				cycles_in_1us = ceil(freq, (1000 * 1000));
+
 			break;
 		}
 	}
@@ -1285,7 +1286,7 @@ static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up)
 	return ufs_qcom_set_clk_40ns_cycles(hba, cycles_in_1us);
 }
 
-static int ufs_qcom_clk_scale_up_pre_change(struct ufs_hba *hba)
+static int ufs_qcom_clk_scale_up_pre_change(struct ufs_hba *hba, unsigned long freq)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
 	struct ufs_pa_layer_attr *attr = &host->dev_req_params;
@@ -1298,7 +1299,7 @@ static int ufs_qcom_clk_scale_up_pre_change(struct ufs_hba *hba)
 		return ret;
 	}
 	/* set unipro core clock attributes and clear clock divider */
-	return ufs_qcom_set_core_clk_ctrl(hba, true);
+	return ufs_qcom_set_core_clk_ctrl(hba, freq);
 }
 
 static int ufs_qcom_clk_scale_up_post_change(struct ufs_hba *hba)
@@ -1327,10 +1328,10 @@ static int ufs_qcom_clk_scale_down_pre_change(struct ufs_hba *hba)
 	return err;
 }
 
-static int ufs_qcom_clk_scale_down_post_change(struct ufs_hba *hba)
+static int ufs_qcom_clk_scale_down_post_change(struct ufs_hba *hba, unsigned long freq)
 {
 	/* set unipro core clock attributes and clear clock divider */
-	return ufs_qcom_set_core_clk_ctrl(hba, false);
+	return ufs_qcom_set_core_clk_ctrl(hba, freq);
 }
 
 static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, bool scale_up,
@@ -1349,7 +1350,7 @@ static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, bool scale_up,
 		if (err)
 			return err;
 		if (scale_up)
-			err = ufs_qcom_clk_scale_up_pre_change(hba);
+			err = ufs_qcom_clk_scale_up_pre_change(hba, target_freq);
 		else
 			err = ufs_qcom_clk_scale_down_pre_change(hba);
 
@@ -1361,7 +1362,7 @@ static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, bool scale_up,
 		if (scale_up)
 			err = ufs_qcom_clk_scale_up_post_change(hba);
 		else
-			err = ufs_qcom_clk_scale_down_post_change(hba);
+			err = ufs_qcom_clk_scale_down_post_change(hba, target_freq);
 
 
 		if (err) {
-- 
2.34.1

Re: [PATCH 2/8] scsi: ufs: qcom: Pass target_freq to clk scale pre and post change

[Manivannan Sadhasivam]

On Thu, Jan 16, 2025 at 05:11:43PM +0800, Ziqi Chen wrote:
> From: Can Guo <quic_cang@quicinc.com>
> 
> If OPP V2 is used, devfreq clock scaling may scale clock amongst more than
> two freqs.

Same comment as previous patch.

> In the case of scaling up, the devfreq may decide to scale the
> clock to an intermidiate freq base on load, but the clock scale up pre
> change operation uses settings for the max clock freq unconditionally. Fix
> it by passing the target_freq to clock scale up pre change so that the
> correct settings for the target_freq can be used.
> 
> In the case of scaling down, the clock scale down post change operation
> is doing fine, because it reads the actual clock rate to tell freq, but to
> keep symmetry with clock scale up pre change operation, just use the
> target_freq instead of reading clock rate.
> 
> Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> Signed-off-by: Can Guo <quic_cang@quicinc.com>
> ---
>  drivers/ufs/host/ufs-qcom.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index b6eef975dc46..1e8a23eb8c13 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -97,7 +97,7 @@ static const struct __ufs_qcom_bw_table {
>  };
>  
>  static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host);
> -static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up);
> +static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, unsigned long freq);
>  
>  static struct ufs_qcom_host *rcdev_to_ufs_host(struct reset_controller_dev *rcd)
>  {
> @@ -524,7 +524,7 @@ static int ufs_qcom_link_startup_notify(struct ufs_hba *hba,
>  			return -EINVAL;
>  		}
>  
> -		err = ufs_qcom_set_core_clk_ctrl(hba, true);
> +		err = ufs_qcom_set_core_clk_ctrl(hba, ULONG_MAX);
>  		if (err)
>  			dev_err(hba->dev, "cfg core clk ctrl failed\n");
>  		/*
> @@ -1231,7 +1231,7 @@ static int ufs_qcom_set_clk_40ns_cycles(struct ufs_hba *hba,
>  	return ufshcd_dme_set(hba, UIC_ARG_MIB(PA_VS_CORE_CLK_40NS_CYCLES), reg);
>  }
>  
> -static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up)
> +static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, unsigned long freq)
>  {
>  	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>  	struct list_head *head = &hba->clk_list_head;
> @@ -1245,10 +1245,11 @@ static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up)
>  		    !strcmp(clki->name, "core_clk_unipro")) {
>  			if (!clki->max_freq)
>  				cycles_in_1us = 150; /* default for backwards compatibility */
> -			else if (is_scale_up)
> +			else if (freq == ULONG_MAX)
>  				cycles_in_1us = ceil(clki->max_freq, (1000 * 1000));
>  			else
> -				cycles_in_1us = ceil(clk_get_rate(clki->clk), (1000 * 1000));
> +				cycles_in_1us = ceil(freq, (1000 * 1000));

Consider switching to HZ_PER_MHZ in a separate patch later.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 2/8] scsi: ufs: qcom: Pass target_freq to clk scale pre and post change

[Ziqi Chen]

Hi Mani,

Thanks for you review~

On 1/19/2025 3:22 PM, Manivannan Sadhasivam wrote:
> On Thu, Jan 16, 2025 at 05:11:43PM +0800, Ziqi Chen wrote:
>> From: Can Guo <quic_cang@quicinc.com>
>>
>> If OPP V2 is used, devfreq clock scaling may scale clock amongst more than
>> two freqs.
> 
> Same comment as previous patch.
> 

please see my response in previous patch.

>> In the case of scaling up, the devfreq may decide to scale the
>> clock to an intermidiate freq base on load, but the clock scale up pre
>> change operation uses settings for the max clock freq unconditionally. Fix
>> it by passing the target_freq to clock scale up pre change so that the
>> correct settings for the target_freq can be used.
>>
>> In the case of scaling down, the clock scale down post change operation
>> is doing fine, because it reads the actual clock rate to tell freq, but to
>> keep symmetry with clock scale up pre change operation, just use the
>> target_freq instead of reading clock rate.
>>
>> Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>> Signed-off-by: Can Guo <quic_cang@quicinc.com>
>> ---
>>   drivers/ufs/host/ufs-qcom.c | 23 ++++++++++++-----------
>>   1 file changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>> index b6eef975dc46..1e8a23eb8c13 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -97,7 +97,7 @@ static const struct __ufs_qcom_bw_table {
>>   };
>>   
>>   static void ufs_qcom_get_default_testbus_cfg(struct ufs_qcom_host *host);
>> -static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up);
>> +static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, unsigned long freq);
>>   
>>   static struct ufs_qcom_host *rcdev_to_ufs_host(struct reset_controller_dev *rcd)
>>   {
>> @@ -524,7 +524,7 @@ static int ufs_qcom_link_startup_notify(struct ufs_hba *hba,
>>   			return -EINVAL;
>>   		}
>>   
>> -		err = ufs_qcom_set_core_clk_ctrl(hba, true);
>> +		err = ufs_qcom_set_core_clk_ctrl(hba, ULONG_MAX);
>>   		if (err)
>>   			dev_err(hba->dev, "cfg core clk ctrl failed\n");
>>   		/*
>> @@ -1231,7 +1231,7 @@ static int ufs_qcom_set_clk_40ns_cycles(struct ufs_hba *hba,
>>   	return ufshcd_dme_set(hba, UIC_ARG_MIB(PA_VS_CORE_CLK_40NS_CYCLES), reg);
>>   }
>>   
>> -static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up)
>> +static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, unsigned long freq)
>>   {
>>   	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>>   	struct list_head *head = &hba->clk_list_head;
>> @@ -1245,10 +1245,11 @@ static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up)
>>   		    !strcmp(clki->name, "core_clk_unipro")) {
>>   			if (!clki->max_freq)
>>   				cycles_in_1us = 150; /* default for backwards compatibility */
>> -			else if (is_scale_up)
>> +			else if (freq == ULONG_MAX)
>>   				cycles_in_1us = ceil(clki->max_freq, (1000 * 1000));
>>   			else
>> -				cycles_in_1us = ceil(clk_get_rate(clki->clk), (1000 * 1000));
>> +				cycles_in_1us = ceil(freq, (1000 * 1000));
> 
> Consider switching to HZ_PER_MHZ in a separate patch later
Sure, Thanks for suggestion,  will update in next version.

> 
> - Mani

-Ziqi

> 

[PATCH 3/8] scsi: ufs: core: Add a vops to map clock frequency to gear speed

[Ziqi Chen]

From: Can Guo <quic_cang@quicinc.com>

Add a vops to map UFS host controller clock frequencies to the maximum
supported UFS high speed gear speeds. During clock scaling, we can map the
target clock frequency, demanded by devfreq, to the maximum supported gear
speed, so that devfreq can scale the gear to the highest gear speed
supported at the target clock frequency, instead of just scaling up/down
the gear between the min and max gear speeds.

Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
Signed-off-by: Can Guo <quic_cang@quicinc.com>
---
 drivers/ufs/core/ufshcd-priv.h | 10 ++++++++++
 include/ufs/ufshcd.h           |  3 +++
 2 files changed, 13 insertions(+)

diff --git a/drivers/ufs/core/ufshcd-priv.h b/drivers/ufs/core/ufshcd-priv.h
index 0549b65f71ed..a8f05fc6e830 100644
--- a/drivers/ufs/core/ufshcd-priv.h
+++ b/drivers/ufs/core/ufshcd-priv.h
@@ -277,6 +277,16 @@ static inline int ufshcd_mcq_vops_config_esi(struct ufs_hba *hba)
 	return -EOPNOTSUPP;
 }
 
+static inline int ufshcd_vops_freq_to_gear_speed(struct ufs_hba *hba,
+						 unsigned long freq,
+						 u32 *gear)
+{
+	if (hba->vops && hba->vops->freq_to_gear_speed)
+		return hba->vops->freq_to_gear_speed(hba, freq, gear);
+
+	return -EOPNOTSUPP;
+}
+
 extern const struct ufs_pm_lvl_states ufs_pm_lvl_states[];
 
 /**
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index a4dac897a169..8c7c497d63d3 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -336,6 +336,7 @@ struct ufs_pwr_mode_info {
  * @get_outstanding_cqs: called to get outstanding completion queues
  * @config_esi: called to config Event Specific Interrupt
  * @config_scsi_dev: called to configure SCSI device parameters
+ * @freq_to_gear_speed: called to map clock frequency to the max supported gear speed
  */
 struct ufs_hba_variant_ops {
 	const char *name;
@@ -387,6 +388,8 @@ struct ufs_hba_variant_ops {
 				       unsigned long *ocqs);
 	int	(*config_esi)(struct ufs_hba *hba);
 	void	(*config_scsi_dev)(struct scsi_device *sdev);
+	int (*freq_to_gear_speed)(struct ufs_hba *hba, unsigned long freq,
+					u32 *gear);
 };
 
 /* clock gating state  */
-- 
2.34.1

[PATCH 4/8] scsi: ufs: qcom: Implement the freq_to_gear_speed() vops

[Ziqi Chen]

From: Can Guo <quic_cang@quicinc.com>

Implement the freq_to_gear_speed() vops to map the unipro core clock
frequency to the corresponding maximum supported gear speed.

Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
Signed-off-by: Can Guo <quic_cang@quicinc.com>
---
 drivers/ufs/host/ufs-qcom.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 1e8a23eb8c13..64263fa884f5 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1803,6 +1803,37 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba)
 	return ret;
 }
 
+static int ufs_qcom_freq_to_gear_speed(struct ufs_hba *hba, unsigned long freq, u32 *gear)
+{
+	int ret = 0;
+
+	switch (freq) {
+	case 403000000:
+		*gear = UFS_HS_G5;
+		break;
+	case 300000000:
+		*gear = UFS_HS_G4;
+		break;
+	case 201500000:
+		*gear = UFS_HS_G3;
+		break;
+	case 150000000:
+	case 100000000:
+		*gear = UFS_HS_G2;
+		break;
+	case 75000000:
+	case 37500000:
+		*gear = UFS_HS_G1;
+		break;
+	default:
+		ret = -EINVAL;
+		dev_err(hba->dev, "Unsupported clock freq\n");
+		break;
+	}
+
+	return ret;
+}
+
 /*
  * struct ufs_hba_qcom_vops - UFS QCOM specific variant operations
  *
@@ -1833,6 +1864,7 @@ static const struct ufs_hba_variant_ops ufs_hba_qcom_vops = {
 	.op_runtime_config	= ufs_qcom_op_runtime_config,
 	.get_outstanding_cqs	= ufs_qcom_get_outstanding_cqs,
 	.config_esi		= ufs_qcom_config_esi,
+	.freq_to_gear_speed	= ufs_qcom_freq_to_gear_speed,
 };
 
 /**
-- 
2.34.1

RE: [PATCH 4/8] scsi: ufs: qcom: Implement the freq_to_gear_speed() vops

[Avri Altman]

> From: Can Guo <quic_cang@quicinc.com>
> 
> Implement the freq_to_gear_speed() vops to map the unipro core clock
> frequency to the corresponding maximum supported gear speed.
> 
> Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> Signed-off-by: Can Guo <quic_cang@quicinc.com>
> ---
>  drivers/ufs/host/ufs-qcom.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c index
> 1e8a23eb8c13..64263fa884f5 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -1803,6 +1803,37 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba)
>         return ret;
>  }
> 
> +static int ufs_qcom_freq_to_gear_speed(struct ufs_hba *hba, unsigned
> +long freq, u32 *gear) {
> +       int ret = 0;
> +
> +       switch (freq) {
Maybe you can use here the UNIPRO_CORE_CLK_FREQ_xx ?

Thanks,
Avri
> +       case 403000000:
> +               *gear = UFS_HS_G5;
> +               break;
> +       case 300000000:
> +               *gear = UFS_HS_G4;
> +               break;
> +       case 201500000:
> +               *gear = UFS_HS_G3;
> +               break;
> +       case 150000000:
> +       case 100000000:
> +               *gear = UFS_HS_G2;
> +               break;
> +       case 75000000:
> +       case 37500000:
> +               *gear = UFS_HS_G1;
> +               break;
> +       default:
> +               ret = -EINVAL;
> +               dev_err(hba->dev, "Unsupported clock freq\n");
> +               break;
> +       }
> +
> +       return ret;
> +}
> +
>  /*
>   * struct ufs_hba_qcom_vops - UFS QCOM specific variant operations
>   *
> @@ -1833,6 +1864,7 @@ static const struct ufs_hba_variant_ops
> ufs_hba_qcom_vops = {
>         .op_runtime_config      = ufs_qcom_op_runtime_config,
>         .get_outstanding_cqs    = ufs_qcom_get_outstanding_cqs,
>         .config_esi             = ufs_qcom_config_esi,
> +       .freq_to_gear_speed     = ufs_qcom_freq_to_gear_speed,
>  };
> 
>  /**
> --
> 2.34.1

Re: [PATCH 4/8] scsi: ufs: qcom: Implement the freq_to_gear_speed() vops

[Ziqi Chen]

Hi Avri,

Thanks for your review~

On 1/17/2025 5:40 AM, Avri Altman wrote:
>> From: Can Guo <quic_cang@quicinc.com>
>>
>> Implement the freq_to_gear_speed() vops to map the unipro core clock
>> frequency to the corresponding maximum supported gear speed.
>>
>> Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>> Signed-off-by: Can Guo <quic_cang@quicinc.com>
>> ---
>>   drivers/ufs/host/ufs-qcom.c | 32 ++++++++++++++++++++++++++++++++
>>   1 file changed, 32 insertions(+)
>>
>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c index
>> 1e8a23eb8c13..64263fa884f5 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -1803,6 +1803,37 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba)
>>          return ret;
>>   }
>>
>> +static int ufs_qcom_freq_to_gear_speed(struct ufs_hba *hba, unsigned
>> +long freq, u32 *gear) {
>> +       int ret = 0;
>> +
>> +       switch (freq) {
> Maybe you can use here the UNIPRO_CORE_CLK_FREQ_xx
The UNIPRO_CORE_CLK_FREQ_xx is used for "cycles_in_1us" which be handled 
by ceil() function.  It is not an exact frequency number and is not 
appropriate for use here.

> 
> Thanks,
> Avri

-Ziqi

>> +       case 403000000:
>> +               *gear = UFS_HS_G5;
>> +               break;
>> +       case 300000000:
>> +               *gear = UFS_HS_G4;
>> +               break;
>> +       case 201500000:
>> +               *gear = UFS_HS_G3;
>> +               break;
>> +       case 150000000:
>> +       case 100000000:
>> +               *gear = UFS_HS_G2;
>> +               break;
>> +       case 75000000:
>> +       case 37500000:
>> +               *gear = UFS_HS_G1;
>> +               break;
>> +       default:
>> +               ret = -EINVAL;
>> +               dev_err(hba->dev, "Unsupported clock freq\n");
>> +               break;
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>>   /*
>>    * struct ufs_hba_qcom_vops - UFS QCOM specific variant operations
>>    *
>> @@ -1833,6 +1864,7 @@ static const struct ufs_hba_variant_ops
>> ufs_hba_qcom_vops = {
>>          .op_runtime_config      = ufs_qcom_op_runtime_config,
>>          .get_outstanding_cqs    = ufs_qcom_get_outstanding_cqs,
>>          .config_esi             = ufs_qcom_config_esi,
>> +       .freq_to_gear_speed     = ufs_qcom_freq_to_gear_speed,
>>   };
>>
>>   /**
>> --
>> 2.34.1
> 

Re: [PATCH 4/8] scsi: ufs: qcom: Implement the freq_to_gear_speed() vops

[Manivannan Sadhasivam]

On Thu, Jan 16, 2025 at 05:11:45PM +0800, Ziqi Chen wrote:
> From: Can Guo <quic_cang@quicinc.com>
> 
> Implement the freq_to_gear_speed() vops to map the unipro core clock
> frequency to the corresponding maximum supported gear speed.
> 
> Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> Signed-off-by: Can Guo <quic_cang@quicinc.com>
> ---
>  drivers/ufs/host/ufs-qcom.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 1e8a23eb8c13..64263fa884f5 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -1803,6 +1803,37 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba)
>  	return ret;
>  }
>  
> +static int ufs_qcom_freq_to_gear_speed(struct ufs_hba *hba, unsigned long freq, u32 *gear)
> +{
> +	int ret = 0;

Please do not initialize ret with 0. Return the actual value directly.

> +
> +	switch (freq) {
> +	case 403000000:
> +		*gear = UFS_HS_G5;
> +		break;
> +	case 300000000:
> +		*gear = UFS_HS_G4;
> +		break;
> +	case 201500000:
> +		*gear = UFS_HS_G3;
> +		break;
> +	case 150000000:
> +	case 100000000:
> +		*gear = UFS_HS_G2;
> +		break;
> +	case 75000000:
> +	case 37500000:
> +		*gear = UFS_HS_G1;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		dev_err(hba->dev, "Unsupported clock freq\n");

Print the freq.

- Mani

> +		break;
> +	}
> +
> +	return ret;
> +}
> +
>  /*
>   * struct ufs_hba_qcom_vops - UFS QCOM specific variant operations
>   *
> @@ -1833,6 +1864,7 @@ static const struct ufs_hba_variant_ops ufs_hba_qcom_vops = {
>  	.op_runtime_config	= ufs_qcom_op_runtime_config,
>  	.get_outstanding_cqs	= ufs_qcom_get_outstanding_cqs,
>  	.config_esi		= ufs_qcom_config_esi,
> +	.freq_to_gear_speed	= ufs_qcom_freq_to_gear_speed,
>  };
>  
>  /**
> -- 
> 2.34.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 4/8] scsi: ufs: qcom: Implement the freq_to_gear_speed() vops

[Ziqi Chen]

Hi Mani，

Thanks for your comments~

On 1/19/2025 3:30 PM, Manivannan Sadhasivam wrote:
> On Thu, Jan 16, 2025 at 05:11:45PM +0800, Ziqi Chen wrote:
>> From: Can Guo <quic_cang@quicinc.com>
>>
>> Implement the freq_to_gear_speed() vops to map the unipro core clock
>> frequency to the corresponding maximum supported gear speed.
>>
>> Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>> Signed-off-by: Can Guo <quic_cang@quicinc.com>
>> ---
>>   drivers/ufs/host/ufs-qcom.c | 32 ++++++++++++++++++++++++++++++++
>>   1 file changed, 32 insertions(+)
>>
>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>> index 1e8a23eb8c13..64263fa884f5 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -1803,6 +1803,37 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba)
>>   	return ret;
>>   }
>>   
>> +static int ufs_qcom_freq_to_gear_speed(struct ufs_hba *hba, unsigned long freq, u32 *gear)
>> +{
>> +	int ret = 0 >
> Please do not initialize ret with 0. Return the actual value directly.
>

If we don't initialize ret here, for the cases of freq matched in the 
table, it will return an unknown ret value. It is not make sense, right?

Or you may want to say we don't need “ret” , just need to return gear 
value? But we need this "ret" to check whether the freq is invalid.

>> +
>> +	switch (freq) {
>> +	case 403000000:
>> +		*gear = UFS_HS_G5;
>> +		break;
>> +	case 300000000:
>> +		*gear = UFS_HS_G4;
>> +		break;
>> +	case 201500000:
>> +		*gear = UFS_HS_G3;
>> +		break;
>> +	case 150000000:
>> +	case 100000000:
>> +		*gear = UFS_HS_G2;
>> +		break;
>> +	case 75000000:
>> +	case 37500000:
>> +		*gear = UFS_HS_G1;
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
>> +		dev_err(hba->dev, "Unsupported clock freq\n");
> 
> Print the freq.

Ok, thank for your suggestion, we can print freq with dev_dbg() in next 
version.

> 
> - Mani >

-Ziqi

>> +		break;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>   /*
>>    * struct ufs_hba_qcom_vops - UFS QCOM specific variant operations
>>    *
>> @@ -1833,6 +1864,7 @@ static const struct ufs_hba_variant_ops ufs_hba_qcom_vops = {
>>   	.op_runtime_config	= ufs_qcom_op_runtime_config,
>>   	.get_outstanding_cqs	= ufs_qcom_get_outstanding_cqs,
>>   	.config_esi		= ufs_qcom_config_esi,
>> +	.freq_to_gear_speed	= ufs_qcom_freq_to_gear_speed,
>>   };
>>   
>>   /**
>> -- 
>> 2.34.1
>>
> 

Re: [PATCH 4/8] scsi: ufs: qcom: Implement the freq_to_gear_speed() vops

[Manivannan Sadhasivam]

On Mon, Jan 20, 2025 at 08:07:07PM +0800, Ziqi Chen wrote:
> Hi Mani，
> 
> Thanks for your comments~
> 
> On 1/19/2025 3:30 PM, Manivannan Sadhasivam wrote:
> > On Thu, Jan 16, 2025 at 05:11:45PM +0800, Ziqi Chen wrote:
> > > From: Can Guo <quic_cang@quicinc.com>
> > > 
> > > Implement the freq_to_gear_speed() vops to map the unipro core clock
> > > frequency to the corresponding maximum supported gear speed.
> > > 
> > > Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> > > Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> > > Signed-off-by: Can Guo <quic_cang@quicinc.com>
> > > ---
> > >   drivers/ufs/host/ufs-qcom.c | 32 ++++++++++++++++++++++++++++++++
> > >   1 file changed, 32 insertions(+)
> > > 
> > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> > > index 1e8a23eb8c13..64263fa884f5 100644
> > > --- a/drivers/ufs/host/ufs-qcom.c
> > > +++ b/drivers/ufs/host/ufs-qcom.c
> > > @@ -1803,6 +1803,37 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba)
> > >   	return ret;
> > >   }
> > > +static int ufs_qcom_freq_to_gear_speed(struct ufs_hba *hba, unsigned long freq, u32 *gear)
> > > +{
> > > +	int ret = 0 >
> > Please do not initialize ret with 0. Return the actual value directly.
> > 
> 
> If we don't initialize ret here, for the cases of freq matched in the table,
> it will return an unknown ret value. It is not make sense, right?
> 
> Or you may want to say we don't need “ret” , just need to return gear value?
> But we need this "ret" to check whether the freq is invalid.
> 

I meant to say that you can just return 0 directly in success case and -EINVAL
in the case of error.

> > > +
> > > +	switch (freq) {
> > > +	case 403000000:
> > > +		*gear = UFS_HS_G5;
> > > +		break;
> > > +	case 300000000:
> > > +		*gear = UFS_HS_G4;
> > > +		break;
> > > +	case 201500000:
> > > +		*gear = UFS_HS_G3;
> > > +		break;
> > > +	case 150000000:
> > > +	case 100000000:
> > > +		*gear = UFS_HS_G2;
> > > +		break;
> > > +	case 75000000:
> > > +	case 37500000:
> > > +		*gear = UFS_HS_G1;
> > > +		break;
> > > +	default:
> > > +		ret = -EINVAL;
> > > +		dev_err(hba->dev, "Unsupported clock freq\n");
> > 
> > Print the freq.
> 
> Ok, thank for your suggestion, we can print freq with dev_dbg() in next
> version.
> 

No, use dev_err() with the freq.

- Mani 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 4/8] scsi: ufs: qcom: Implement the freq_to_gear_speed() vops

[Ziqi Chen]

On 1/20/2025 11:41 PM, Manivannan Sadhasivam wrote:
> On Mon, Jan 20, 2025 at 08:07:07PM +0800, Ziqi Chen wrote:
>> Hi Mani，
>>
>> Thanks for your comments~
>>
>> On 1/19/2025 3:30 PM, Manivannan Sadhasivam wrote:
>>> On Thu, Jan 16, 2025 at 05:11:45PM +0800, Ziqi Chen wrote:
>>>> From: Can Guo <quic_cang@quicinc.com>
>>>>
>>>> Implement the freq_to_gear_speed() vops to map the unipro core clock
>>>> frequency to the corresponding maximum supported gear speed.
>>>>
>>>> Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>>>> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>>>> Signed-off-by: Can Guo <quic_cang@quicinc.com>
>>>> ---
>>>>    drivers/ufs/host/ufs-qcom.c | 32 ++++++++++++++++++++++++++++++++
>>>>    1 file changed, 32 insertions(+)
>>>>
>>>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>>>> index 1e8a23eb8c13..64263fa884f5 100644
>>>> --- a/drivers/ufs/host/ufs-qcom.c
>>>> +++ b/drivers/ufs/host/ufs-qcom.c
>>>> @@ -1803,6 +1803,37 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba)
>>>>    	return ret;
>>>>    }
>>>> +static int ufs_qcom_freq_to_gear_speed(struct ufs_hba *hba, unsigned long freq, u32 *gear)
>>>> +{
>>>> +	int ret = 0 >
>>> Please do not initialize ret with 0. Return the actual value directly.
>>>
>>
>> If we don't initialize ret here, for the cases of freq matched in the table,
>> it will return an unknown ret value. It is not make sense, right?
>>
>> Or you may want to say we don't need “ret” , just need to return gear value?
>> But we need this "ret" to check whether the freq is invalid.
>>
> 
> I meant to say that you can just return 0 directly in success case and -EINVAL
> in the case of error.
> 
Hi Mani,

If we don't print freq here , I think your suggestion is very good. If 
we print freq in this function , using "ret" to indicate success case 
and failure case and print freq an the end of this function is the way 
to avoid code bloat.

How do you think about it?

>>>> +
>>>> +	switch (freq) {
>>>> +	case 403000000:
>>>> +		*gear = UFS_HS_G5;
>>>> +		break;
>>>> +	case 300000000:
>>>> +		*gear = UFS_HS_G4;
>>>> +		break;
>>>> +	case 201500000:
>>>> +		*gear = UFS_HS_G3;
>>>> +		break;
>>>> +	case 150000000:
>>>> +	case 100000000:
>>>> +		*gear = UFS_HS_G2;
>>>> +		break;
>>>> +	case 75000000:
>>>> +	case 37500000:
>>>> +		*gear = UFS_HS_G1;
>>>> +		break;
>>>> +	default:
>>>> +		ret = -EINVAL;
>>>> +		dev_err(hba->dev, "Unsupported clock freq\n");
>>>
>>> Print the freq.
>>
>> Ok, thank for your suggestion, we can print freq with dev_dbg() in next
>> version.
>>
> 
> No, use dev_err() with the freq. >
> - Mani
>
I think use dev_err() here does not make sense:

1. This print is not an error message , just an information print. Using 
dev_err() reduces the readability of this code.
2. This prints will be print very frequent, I afraid it will increase 
the latency of clock scaling.


How do you think ?

-Ziqi

Re: [PATCH 4/8] scsi: ufs: qcom: Implement the freq_to_gear_speed() vops

[Manivannan Sadhasivam]

On Tue, Jan 21, 2025 at 11:52:42AM +0800, Ziqi Chen wrote:
> 
> 
> On 1/20/2025 11:41 PM, Manivannan Sadhasivam wrote:
> > On Mon, Jan 20, 2025 at 08:07:07PM +0800, Ziqi Chen wrote:
> > > Hi Mani，
> > > 
> > > Thanks for your comments~
> > > 
> > > On 1/19/2025 3:30 PM, Manivannan Sadhasivam wrote:
> > > > On Thu, Jan 16, 2025 at 05:11:45PM +0800, Ziqi Chen wrote:
> > > > > From: Can Guo <quic_cang@quicinc.com>
> > > > > 
> > > > > Implement the freq_to_gear_speed() vops to map the unipro core clock
> > > > > frequency to the corresponding maximum supported gear speed.
> > > > > 
> > > > > Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> > > > > Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> > > > > Signed-off-by: Can Guo <quic_cang@quicinc.com>
> > > > > ---
> > > > >    drivers/ufs/host/ufs-qcom.c | 32 ++++++++++++++++++++++++++++++++
> > > > >    1 file changed, 32 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> > > > > index 1e8a23eb8c13..64263fa884f5 100644
> > > > > --- a/drivers/ufs/host/ufs-qcom.c
> > > > > +++ b/drivers/ufs/host/ufs-qcom.c
> > > > > @@ -1803,6 +1803,37 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba)
> > > > >    	return ret;
> > > > >    }
> > > > > +static int ufs_qcom_freq_to_gear_speed(struct ufs_hba *hba, unsigned long freq, u32 *gear)
> > > > > +{
> > > > > +	int ret = 0 >
> > > > Please do not initialize ret with 0. Return the actual value directly.
> > > > 
> > > 
> > > If we don't initialize ret here, for the cases of freq matched in the table,
> > > it will return an unknown ret value. It is not make sense, right?
> > > 
> > > Or you may want to say we don't need “ret” , just need to return gear value?
> > > But we need this "ret" to check whether the freq is invalid.
> > > 
> > 
> > I meant to say that you can just return 0 directly in success case and -EINVAL
> > in the case of error.
> > 
> Hi Mani,
> 
> If we don't print freq here , I think your suggestion is very good. If we
> print freq in this function , using "ret" to indicate success case and
> failure case and print freq an the end of this function is the way to avoid
> code bloat.
> 
> How do you think about it?
> 

I don't understand how code bloat comes into picture here. I'm just asking for
this:

static int ufs_qcom_freq_to_gear_speed(struct ufs_hba *hba, unsigned long freq, u32 *gear)
{
	switch (freq) {
	case 403000000:
		*gear = UFS_HS_G5;
		break;
	...

	default:
		dev_err(hba->dev, "Unsupported clock freq: %ld\n", freq);
		return -EINVAL;
	}
	
	return 0;
}

> > > > > +
> > > > > +	switch (freq) {
> > > > > +	case 403000000:
> > > > > +		*gear = UFS_HS_G5;
> > > > > +		break;
> > > > > +	case 300000000:
> > > > > +		*gear = UFS_HS_G4;
> > > > > +		break;
> > > > > +	case 201500000:
> > > > > +		*gear = UFS_HS_G3;
> > > > > +		break;
> > > > > +	case 150000000:
> > > > > +	case 100000000:
> > > > > +		*gear = UFS_HS_G2;
> > > > > +		break;
> > > > > +	case 75000000:
> > > > > +	case 37500000:
> > > > > +		*gear = UFS_HS_G1;
> > > > > +		break;
> > > > > +	default:
> > > > > +		ret = -EINVAL;
> > > > > +		dev_err(hba->dev, "Unsupported clock freq\n");
> > > > 
> > > > Print the freq.
> > > 
> > > Ok, thank for your suggestion, we can print freq with dev_dbg() in next
> > > version.
> > > 
> > 
> > No, use dev_err() with the freq. >
> > - Mani
> > 
> I think use dev_err() here does not make sense:
> 
> 1. This print is not an error message , just an information print. Using
> dev_err() reduces the readability of this code.

Then why it was dev_err() in the first place?

> 2. This prints will be print very frequent, I afraid it will increase the
> latency of clock scaling.
> 

First you need to decide whether this print should warn user or not. It is
telling users that the OPP table supplied a frequency that doesn't match the
gear speed. This can happen if there is a discrepancy between DT and the driver.
In that case, the users *should* be warned to fix the driver/DT. If you bury it
with dev_dbg(), no one will notice it.

If your concern is with the frequency of logs, then use dev_err_ratelimited().

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 4/8] scsi: ufs: qcom: Implement the freq_to_gear_speed() vops

[Ziqi Chen]

On 1/24/2025 1:35 PM, Manivannan Sadhasivam wrote:
> On Tue, Jan 21, 2025 at 11:52:42AM +0800, Ziqi Chen wrote:
>>
>>
>> On 1/20/2025 11:41 PM, Manivannan Sadhasivam wrote:
>>> On Mon, Jan 20, 2025 at 08:07:07PM +0800, Ziqi Chen wrote:
>>>> Hi Mani，
>>>>
>>>> Thanks for your comments~
>>>>
>>>> On 1/19/2025 3:30 PM, Manivannan Sadhasivam wrote:
>>>>> On Thu, Jan 16, 2025 at 05:11:45PM +0800, Ziqi Chen wrote:
>>>>>> From: Can Guo <quic_cang@quicinc.com>
>>>>>>
>>>>>> Implement the freq_to_gear_speed() vops to map the unipro core clock
>>>>>> frequency to the corresponding maximum supported gear speed.
>>>>>>
>>>>>> Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>>>>>> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>>>>>> Signed-off-by: Can Guo <quic_cang@quicinc.com>
>>>>>> ---
>>>>>>     drivers/ufs/host/ufs-qcom.c | 32 ++++++++++++++++++++++++++++++++
>>>>>>     1 file changed, 32 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>>>>>> index 1e8a23eb8c13..64263fa884f5 100644
>>>>>> --- a/drivers/ufs/host/ufs-qcom.c
>>>>>> +++ b/drivers/ufs/host/ufs-qcom.c
>>>>>> @@ -1803,6 +1803,37 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba)
>>>>>>     	return ret;
>>>>>>     }
>>>>>> +static int ufs_qcom_freq_to_gear_speed(struct ufs_hba *hba, unsigned long freq, u32 *gear)
>>>>>> +{
>>>>>> +	int ret = 0 >
>>>>> Please do not initialize ret with 0. Return the actual value directly.
>>>>>
>>>>
>>>> If we don't initialize ret here, for the cases of freq matched in the table,
>>>> it will return an unknown ret value. It is not make sense, right?
>>>>
>>>> Or you may want to say we don't need “ret” , just need to return gear value?
>>>> But we need this "ret" to check whether the freq is invalid.
>>>>
>>>
>>> I meant to say that you can just return 0 directly in success case and -EINVAL
>>> in the case of error.
>>>
>> Hi Mani,
>>
>> If we don't print freq here , I think your suggestion is very good. If we
>> print freq in this function , using "ret" to indicate success case and
>> failure case and print freq an the end of this function is the way to avoid
>> code bloat.
>>
>> How do you think about it?
>>
> 
> I don't understand how code bloat comes into picture here. I'm just asking for
> this:
> 
> static int ufs_qcom_freq_to_gear_speed(struct ufs_hba *hba, unsigned long freq, u32 *gear)
> {
> 	switch (freq) {
> 	case 403000000:
> 		*gear = UFS_HS_G5;
> 		break;
> 	...
> 
> 	default:
> 		dev_err(hba->dev, "Unsupported clock freq: %ld\n", freq);
> 		return -EINVAL;
> 	}
> 	
> 	return 0;
> }
> 
>>>>>> +
>>>>>> +	switch (freq) {
>>>>>> +	case 403000000:
>>>>>> +		*gear = UFS_HS_G5;
>>>>>> +		break;
>>>>>> +	case 300000000:
>>>>>> +		*gear = UFS_HS_G4;
>>>>>> +		break;
>>>>>> +	case 201500000:
>>>>>> +		*gear = UFS_HS_G3;
>>>>>> +		break;
>>>>>> +	case 150000000:
>>>>>> +	case 100000000:
>>>>>> +		*gear = UFS_HS_G2;
>>>>>> +		break;
>>>>>> +	case 75000000:
>>>>>> +	case 37500000:
>>>>>> +		*gear = UFS_HS_G1;
>>>>>> +		break;
>>>>>> +	default:
>>>>>> +		ret = -EINVAL;
>>>>>> +		dev_err(hba->dev, "Unsupported clock freq\n");
>>>>>
>>>>> Print the freq.
>>>>
>>>> Ok, thank for your suggestion, we can print freq with dev_dbg() in next
>>>> version.
>>>>
>>>
>>> No, use dev_err() with the freq. >
>>> - Mani
>>>
>> I think use dev_err() here does not make sense:
>>
>> 1. This print is not an error message , just an information print. Using
>> dev_err() reduces the readability of this code.
> 
> Then why it was dev_err() in the first place?
> 
>> 2. This prints will be print very frequent, I afraid it will increase the
>> latency of clock scaling.
>>
> 
> First you need to decide whether this print should warn user or not. It is
> telling users that the OPP table supplied a frequency that doesn't match the
> gear speed. This can happen if there is a discrepancy between DT and the driver.
> In that case, the users *should* be warned to fix the driver/DT. If you bury it
> with dev_dbg(), no one will notice it.
> 
> If your concern is with the frequency of logs, then use dev_err_ratelimited().
> 
> - Mani
> 
I misunderstand your point Mani, I thought you want me to print freq for 
all cases...  if you mean that print failure case, I already added it in 
patch V2.

-Ziqi

[PATCH 5/8] scsi: ufs: core: Enable multi-level gear scaling

[Ziqi Chen]

From: Can Guo <quic_cang@quicinc.com>

With OPP V2 enabled, devfreq can scale clocks amongst multiple frequency
plans. However, the gear speed is only toggled between min and max during
clock scaling. Enable multi-level gear scaling by mapping clock frequencies
to gear speeds, so that when devfreq scales clock frequencies we can put
the UFS link at the appropraite gear speeds accordingly.

Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
Signed-off-by: Can Guo <quic_cang@quicinc.com>
---
 drivers/ufs/core/ufshcd.c | 46 ++++++++++++++++++++++++++++++---------
 1 file changed, 36 insertions(+), 10 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 8d295cc827cc..839bc23aeda0 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -1308,16 +1308,28 @@ static int ufshcd_wait_for_doorbell_clr(struct ufs_hba *hba,
 /**
  * ufshcd_scale_gear - scale up/down UFS gear
  * @hba: per adapter instance
+ * @target_gear: target gear to scale to
  * @scale_up: True for scaling up gear and false for scaling down
  *
  * Return: 0 for success; -EBUSY if scaling can't happen at this time;
  * non-zero for any other errors.
  */
-static int ufshcd_scale_gear(struct ufs_hba *hba, bool scale_up)
+static int ufshcd_scale_gear(struct ufs_hba *hba, u32 target_gear, bool scale_up)
 {
 	int ret = 0;
 	struct ufs_pa_layer_attr new_pwr_info;
 
+	if (target_gear) {
+		memcpy(&new_pwr_info, &hba->pwr_info,
+		       sizeof(struct ufs_pa_layer_attr));
+
+		new_pwr_info.gear_tx = target_gear;
+		new_pwr_info.gear_rx = target_gear;
+
+		goto do_pmc;
+	}
+
+	/* Legacy gear scaling, in case vops_freq_to_gear_speed() is not implemented */
 	if (scale_up) {
 		memcpy(&new_pwr_info, &hba->clk_scaling.saved_pwr_info,
 		       sizeof(struct ufs_pa_layer_attr));
@@ -1338,6 +1350,7 @@ static int ufshcd_scale_gear(struct ufs_hba *hba, bool scale_up)
 		}
 	}
 
+do_pmc:
 	/* check if the power mode needs to be changed or not? */
 	ret = ufshcd_config_pwr_mode(hba, &new_pwr_info);
 	if (ret)
@@ -1408,15 +1421,19 @@ static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, int err, bool sc
 static int ufshcd_devfreq_scale(struct ufs_hba *hba, unsigned long freq,
 				bool scale_up)
 {
+	u32 old_gear = hba->pwr_info.gear_rx;
+	u32 new_gear = 0;
 	int ret = 0;
 
+	ufshcd_vops_freq_to_gear_speed(hba, freq, &new_gear);
+
 	ret = ufshcd_clock_scaling_prepare(hba, 1 * USEC_PER_SEC);
 	if (ret)
 		return ret;
 
 	/* scale down the gear before scaling down clocks */
 	if (!scale_up) {
-		ret = ufshcd_scale_gear(hba, false);
+		ret = ufshcd_scale_gear(hba, new_gear, false);
 		if (ret)
 			goto out_unprepare;
 	}
@@ -1424,13 +1441,13 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, unsigned long freq,
 	ret = ufshcd_scale_clks(hba, freq, scale_up);
 	if (ret) {
 		if (!scale_up)
-			ufshcd_scale_gear(hba, true);
+			ufshcd_scale_gear(hba, old_gear, true);
 		goto out_unprepare;
 	}
 
 	/* scale up the gear after scaling up clocks */
 	if (scale_up) {
-		ret = ufshcd_scale_gear(hba, true);
+		ret = ufshcd_scale_gear(hba, new_gear, true);
 		if (ret) {
 			ufshcd_scale_clks(hba, hba->devfreq->previous_freq,
 					  false);
@@ -1723,6 +1740,8 @@ static ssize_t ufshcd_clkscale_enable_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t count)
 {
 	struct ufs_hba *hba = dev_get_drvdata(dev);
+	struct ufs_clk_info *clki;
+	unsigned long freq;
 	u32 value;
 	int err = 0;
 
@@ -1746,14 +1765,21 @@ static ssize_t ufshcd_clkscale_enable_store(struct device *dev,
 
 	if (value) {
 		ufshcd_resume_clkscaling(hba);
-	} else {
-		ufshcd_suspend_clkscaling(hba);
-		err = ufshcd_devfreq_scale(hba, ULONG_MAX, true);
-		if (err)
-			dev_err(hba->dev, "%s: failed to scale clocks up %d\n",
-					__func__, err);
+		goto out_rel;
 	}
 
+	clki = list_first_entry(&hba->clk_list_head, struct ufs_clk_info, list);
+	freq = clki->max_freq;
+
+	ufshcd_suspend_clkscaling(hba);
+	err = ufshcd_devfreq_scale(hba, freq, true);
+	if (err)
+		dev_err(hba->dev, "%s: failed to scale clocks up %d\n",
+				__func__, err);
+	else
+		hba->clk_scaling.target_freq = freq;
+
+out_rel:
 	ufshcd_release(hba);
 	ufshcd_rpm_put_sync(hba);
 out:
-- 
2.34.1

Re: [PATCH 5/8] scsi: ufs: core: Enable multi-level gear scaling

[Manivannan Sadhasivam]

On Thu, Jan 16, 2025 at 05:11:46PM +0800, Ziqi Chen wrote:
> From: Can Guo <quic_cang@quicinc.com>
> 
> With OPP V2 enabled, devfreq can scale clocks amongst multiple frequency
> plans. However, the gear speed is only toggled between min and max during
> clock scaling. Enable multi-level gear scaling by mapping clock frequencies
> to gear speeds, so that when devfreq scales clock frequencies we can put
> the UFS link at the appropraite gear speeds accordingly.
> 
> Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> Signed-off-by: Can Guo <quic_cang@quicinc.com>
> ---
>  drivers/ufs/core/ufshcd.c | 46 ++++++++++++++++++++++++++++++---------
>  1 file changed, 36 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 8d295cc827cc..839bc23aeda0 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -1308,16 +1308,28 @@ static int ufshcd_wait_for_doorbell_clr(struct ufs_hba *hba,
>  /**
>   * ufshcd_scale_gear - scale up/down UFS gear
>   * @hba: per adapter instance
> + * @target_gear: target gear to scale to
>   * @scale_up: True for scaling up gear and false for scaling down
>   *
>   * Return: 0 for success; -EBUSY if scaling can't happen at this time;
>   * non-zero for any other errors.
>   */
> -static int ufshcd_scale_gear(struct ufs_hba *hba, bool scale_up)
> +static int ufshcd_scale_gear(struct ufs_hba *hba, u32 target_gear, bool scale_up)
>  {
>  	int ret = 0;
>  	struct ufs_pa_layer_attr new_pwr_info;
>  
> +	if (target_gear) {
> +		memcpy(&new_pwr_info, &hba->pwr_info,
> +		       sizeof(struct ufs_pa_layer_attr));
> +
> +		new_pwr_info.gear_tx = target_gear;
> +		new_pwr_info.gear_rx = target_gear;
> +
> +		goto do_pmc;

goto config_pwr_mode;

> +	}
> +
> +	/* Legacy gear scaling, in case vops_freq_to_gear_speed() is not implemented */
>  	if (scale_up) {
>  		memcpy(&new_pwr_info, &hba->clk_scaling.saved_pwr_info,
>  		       sizeof(struct ufs_pa_layer_attr));
> @@ -1338,6 +1350,7 @@ static int ufshcd_scale_gear(struct ufs_hba *hba, bool scale_up)
>  		}
>  	}
>  
> +do_pmc:
>  	/* check if the power mode needs to be changed or not? */
>  	ret = ufshcd_config_pwr_mode(hba, &new_pwr_info);
>  	if (ret)
> @@ -1408,15 +1421,19 @@ static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, int err, bool sc
>  static int ufshcd_devfreq_scale(struct ufs_hba *hba, unsigned long freq,
>  				bool scale_up)
>  {
> +	u32 old_gear = hba->pwr_info.gear_rx;
> +	u32 new_gear = 0;
>  	int ret = 0;
>  
> +	ufshcd_vops_freq_to_gear_speed(hba, freq, &new_gear);
> +
>  	ret = ufshcd_clock_scaling_prepare(hba, 1 * USEC_PER_SEC);
>  	if (ret)
>  		return ret;
>  
>  	/* scale down the gear before scaling down clocks */
>  	if (!scale_up) {
> -		ret = ufshcd_scale_gear(hba, false);
> +		ret = ufshcd_scale_gear(hba, new_gear, false);
>  		if (ret)
>  			goto out_unprepare;
>  	}
> @@ -1424,13 +1441,13 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, unsigned long freq,
>  	ret = ufshcd_scale_clks(hba, freq, scale_up);
>  	if (ret) {
>  		if (!scale_up)
> -			ufshcd_scale_gear(hba, true);
> +			ufshcd_scale_gear(hba, old_gear, true);
>  		goto out_unprepare;
>  	}
>  
>  	/* scale up the gear after scaling up clocks */
>  	if (scale_up) {
> -		ret = ufshcd_scale_gear(hba, true);
> +		ret = ufshcd_scale_gear(hba, new_gear, true);
>  		if (ret) {
>  			ufshcd_scale_clks(hba, hba->devfreq->previous_freq,
>  					  false);
> @@ -1723,6 +1740,8 @@ static ssize_t ufshcd_clkscale_enable_store(struct device *dev,
>  		struct device_attribute *attr, const char *buf, size_t count)
>  {
>  	struct ufs_hba *hba = dev_get_drvdata(dev);
> +	struct ufs_clk_info *clki;
> +	unsigned long freq;
>  	u32 value;
>  	int err = 0;
>  
> @@ -1746,14 +1765,21 @@ static ssize_t ufshcd_clkscale_enable_store(struct device *dev,
>  
>  	if (value) {
>  		ufshcd_resume_clkscaling(hba);
> -	} else {
> -		ufshcd_suspend_clkscaling(hba);
> -		err = ufshcd_devfreq_scale(hba, ULONG_MAX, true);
> -		if (err)
> -			dev_err(hba->dev, "%s: failed to scale clocks up %d\n",
> -					__func__, err);
> +		goto out_rel;
>  	}
>  
> +	clki = list_first_entry(&hba->clk_list_head, struct ufs_clk_info, list);
> +	freq = clki->max_freq;
> +
> +	ufshcd_suspend_clkscaling(hba);
> +	err = ufshcd_devfreq_scale(hba, freq, true);
> +	if (err)
> +		dev_err(hba->dev, "%s: failed to scale clocks up %d\n",
> +				__func__, err);
> +	else
> +		hba->clk_scaling.target_freq = freq;
> +

Just noticed that the 'clkscale_enable', 'clkgate_{delay_ms}/enable' sysfs
attributes have no ABI documentation. Please add them also in a separate patch.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 5/8] scsi: ufs: core: Enable multi-level gear scaling

[Ziqi Chen]

Hi Mani,

Thanks for your comment~

On 1/19/2025 3:48 PM, Manivannan Sadhasivam wrote:
> On Thu, Jan 16, 2025 at 05:11:46PM +0800, Ziqi Chen wrote:
>> From: Can Guo <quic_cang@quicinc.com>
>>
>> With OPP V2 enabled, devfreq can scale clocks amongst multiple frequency
>> plans. However, the gear speed is only toggled between min and max during
>> clock scaling. Enable multi-level gear scaling by mapping clock frequencies
>> to gear speeds, so that when devfreq scales clock frequencies we can put
>> the UFS link at the appropraite gear speeds accordingly.
>>
>> Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>> Signed-off-by: Can Guo <quic_cang@quicinc.com>
>> ---
>>   drivers/ufs/core/ufshcd.c | 46 ++++++++++++++++++++++++++++++---------
>>   1 file changed, 36 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>> index 8d295cc827cc..839bc23aeda0 100644
>> --- a/drivers/ufs/core/ufshcd.c
>> +++ b/drivers/ufs/core/ufshcd.c
>> @@ -1308,16 +1308,28 @@ static int ufshcd_wait_for_doorbell_clr(struct ufs_hba *hba,
>>   /**
>>    * ufshcd_scale_gear - scale up/down UFS gear
>>    * @hba: per adapter instance
>> + * @target_gear: target gear to scale to
>>    * @scale_up: True for scaling up gear and false for scaling down
>>    *
>>    * Return: 0 for success; -EBUSY if scaling can't happen at this time;
>>    * non-zero for any other errors.
>>    */
>> -static int ufshcd_scale_gear(struct ufs_hba *hba, bool scale_up)
>> +static int ufshcd_scale_gear(struct ufs_hba *hba, u32 target_gear, bool scale_up)
>>   {
>>   	int ret = 0;
>>   	struct ufs_pa_layer_attr new_pwr_info;
>>   
>> +	if (target_gear) {
>> +		memcpy(&new_pwr_info, &hba->pwr_info,
>> +		       sizeof(struct ufs_pa_layer_attr));
>> +
>> +		new_pwr_info.gear_tx = target_gear;
>> +		new_pwr_info.gear_rx = target_gear;
>> +
>> +		goto do_pmc;
> 
> goto config_pwr_mode;
>

Ok, Thanks for your suggestion, I will change it in next version.

>> +	}
>> +
>> +	/* Legacy gear scaling, in case vops_freq_to_gear_speed() is not implemented */
>>   	if (scale_up) {
>>   		memcpy(&new_pwr_info, &hba->clk_scaling.saved_pwr_info,
>>   		       sizeof(struct ufs_pa_layer_attr));
>> @@ -1338,6 +1350,7 @@ static int ufshcd_scale_gear(struct ufs_hba *hba, bool scale_up)
>>   		}
>>   	}
>>   
>> +do_pmc:
>>   	/* check if the power mode needs to be changed or not? */
>>   	ret = ufshcd_config_pwr_mode(hba, &new_pwr_info);
>>   	if (ret)
>> @@ -1408,15 +1421,19 @@ static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, int err, bool sc
>>   static int ufshcd_devfreq_scale(struct ufs_hba *hba, unsigned long freq,
>>   				bool scale_up)
>>   {
>> +	u32 old_gear = hba->pwr_info.gear_rx;
>> +	u32 new_gear = 0;
>>   	int ret = 0;
>>   
>> +	ufshcd_vops_freq_to_gear_speed(hba, freq, &new_gear);
>> +
>>   	ret = ufshcd_clock_scaling_prepare(hba, 1 * USEC_PER_SEC);
>>   	if (ret)
>>   		return ret;
>>   
>>   	/* scale down the gear before scaling down clocks */
>>   	if (!scale_up) {
>> -		ret = ufshcd_scale_gear(hba, false);
>> +		ret = ufshcd_scale_gear(hba, new_gear, false);
>>   		if (ret)
>>   			goto out_unprepare;
>>   	}
>> @@ -1424,13 +1441,13 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, unsigned long freq,
>>   	ret = ufshcd_scale_clks(hba, freq, scale_up);
>>   	if (ret) {
>>   		if (!scale_up)
>> -			ufshcd_scale_gear(hba, true);
>> +			ufshcd_scale_gear(hba, old_gear, true);
>>   		goto out_unprepare;
>>   	}
>>   
>>   	/* scale up the gear after scaling up clocks */
>>   	if (scale_up) {
>> -		ret = ufshcd_scale_gear(hba, true);
>> +		ret = ufshcd_scale_gear(hba, new_gear, true);
>>   		if (ret) {
>>   			ufshcd_scale_clks(hba, hba->devfreq->previous_freq,
>>   					  false);
>> @@ -1723,6 +1740,8 @@ static ssize_t ufshcd_clkscale_enable_store(struct device *dev,
>>   		struct device_attribute *attr, const char *buf, size_t count)
>>   {
>>   	struct ufs_hba *hba = dev_get_drvdata(dev);
>> +	struct ufs_clk_info *clki;
>> +	unsigned long freq;
>>   	u32 value;
>>   	int err = 0;
>>   
>> @@ -1746,14 +1765,21 @@ static ssize_t ufshcd_clkscale_enable_store(struct device *dev,
>>   
>>   	if (value) {
>>   		ufshcd_resume_clkscaling(hba);
>> -	} else {
>> -		ufshcd_suspend_clkscaling(hba);
>> -		err = ufshcd_devfreq_scale(hba, ULONG_MAX, true);
>> -		if (err)
>> -			dev_err(hba->dev, "%s: failed to scale clocks up %d\n",
>> -					__func__, err);
>> +		goto out_rel;
>>   	}
>>   
>> +	clki = list_first_entry(&hba->clk_list_head, struct ufs_clk_info, list);
>> +	freq = clki->max_freq;
>> +
>> +	ufshcd_suspend_clkscaling(hba);
>> +	err = ufshcd_devfreq_scale(hba, freq, true);
>> +	if (err)
>> +		dev_err(hba->dev, "%s: failed to scale clocks up %d\n",
>> +				__func__, err);
>> +	else
>> +		hba->clk_scaling.target_freq = freq;
>> +
> 
> Just noticed that the 'clkscale_enable', 'clkgate_{delay_ms}/enable' sysfs
> attributes have no ABI documentation. Please add them also in a separate patch.

Ok, I will update it in next version.

>  > - Mani
> 

-Ziqi

[PATCH 6/8] scsi: ufs: core: Check if scaling up is required when disable clkscale

[Ziqi Chen]

When disabling clkscale via the clkscale_enable sysfs entry, UFS driver
shall perform scaling up once regardless. Check if scaling up is required
or not first to avoid repetitive work.

Co-developed-by: Can Guo <quic_cang@quicinc.com>
Signed-off-by: Can Guo <quic_cang@quicinc.com>
Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
---
 drivers/ufs/core/ufshcd.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 839bc23aeda0..721bf9d1a356 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -1772,6 +1772,10 @@ static ssize_t ufshcd_clkscale_enable_store(struct device *dev,
 	freq = clki->max_freq;
 
 	ufshcd_suspend_clkscaling(hba);
+
+	if (!ufshcd_is_devfreq_scaling_required(hba, freq, true))
+		goto out_rel;
+
 	err = ufshcd_devfreq_scale(hba, freq, true);
 	if (err)
 		dev_err(hba->dev, "%s: failed to scale clocks up %d\n",
-- 
2.34.1

[PATCH 7/8] scsi: ufs: core: Toggle Write Booster during clock scaling base on gear speed

[Ziqi Chen]

From: Can Guo <quic_cang@quicinc.com>

During clock scaling, Write Booster is toggled on or off based on
whether the clock is scaled up or down. However, with OPP V2 powered
multi-level gear scaling, the gear can be scaled amongst multiple gear
speeds, e.g., it may scale down from G5 to G4, or from G4 to G2. To provide
flexibilities, add a new field for clock scaling such that during clock
scaling Write Booster can be enabled or disabled based on gear speeds but
not based on scaling up or down.

Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
Signed-off-by: Can Guo <quic_cang@quicinc.com>
---
 drivers/ufs/core/ufshcd.c | 17 ++++++++++++-----
 include/ufs/ufshcd.h      |  3 +++
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 721bf9d1a356..31ebf267135b 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -1395,13 +1395,17 @@ static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba, u64 timeout_us)
 	return ret;
 }
 
-static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, int err, bool scale_up)
+static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, int err)
 {
 	up_write(&hba->clk_scaling_lock);
 
-	/* Enable Write Booster if we have scaled up else disable it */
-	if (ufshcd_enable_wb_if_scaling_up(hba) && !err)
-		ufshcd_wb_toggle(hba, scale_up);
+	/* Enable Write Booster if current gear requires it else disable it */
+	if (ufshcd_enable_wb_if_scaling_up(hba) && !err) {
+		bool wb_en;
+
+		wb_en = hba->pwr_info.gear_rx >= hba->clk_scaling.wb_gear ? true : false;
+		ufshcd_wb_toggle(hba, wb_en);
+	}
 
 	mutex_unlock(&hba->wb_mutex);
 
@@ -1456,7 +1460,7 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, unsigned long freq,
 	}
 
 out_unprepare:
-	ufshcd_clock_scaling_unprepare(hba, ret, scale_up);
+	ufshcd_clock_scaling_unprepare(hba, ret);
 	return ret;
 }
 
@@ -1816,6 +1820,9 @@ static void ufshcd_init_clk_scaling(struct ufs_hba *hba)
 	if (!hba->clk_scaling.min_gear)
 		hba->clk_scaling.min_gear = UFS_HS_G1;
 
+	if (!hba->clk_scaling.wb_gear)
+		hba->clk_scaling.wb_gear = UFS_HS_G3;
+
 	INIT_WORK(&hba->clk_scaling.suspend_work,
 		  ufshcd_clk_scaling_suspend_work);
 	INIT_WORK(&hba->clk_scaling.resume_work,
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 8c7c497d63d3..8e6c2eb68011 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -448,6 +448,8 @@ struct ufs_clk_gating {
  * @resume_work: worker to resume devfreq
  * @target_freq: frequency requested by devfreq framework
  * @min_gear: lowest HS gear to scale down to
+ * @wb_gear: enable Write Booster when HS gear scales above or equal to it, else
+ *		disable Write Booster
  * @is_enabled: tracks if scaling is currently enabled or not, controlled by
  *		clkscale_enable sysfs node
  * @is_allowed: tracks if scaling is currently allowed or not, used to block
@@ -468,6 +470,7 @@ struct ufs_clk_scaling {
 	struct work_struct resume_work;
 	unsigned long target_freq;
 	u32 min_gear;
+	u32 wb_gear;
 	bool is_enabled;
 	bool is_allowed;
 	bool is_initialized;
-- 
2.34.1

RE: [PATCH 7/8] scsi: ufs: core: Toggle Write Booster during clock scaling base on gear speed

[Avri Altman]

> 
> From: Can Guo <quic_cang@quicinc.com>
> 
> During clock scaling, Write Booster is toggled on or off based on whether the
> clock is scaled up or down. However, with OPP V2 powered multi-level gear
> scaling, the gear can be scaled amongst multiple gear speeds, e.g., it may
> scale down from G5 to G4, or from G4 to G2. To provide flexibilities, add a
> new field for clock scaling such that during clock scaling Write Booster can be
> enabled or disabled based on gear speeds but not based on scaling up or
> down.
> 
> Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> Signed-off-by: Can Guo <quic_cang@quicinc.com>
> ---
>  drivers/ufs/core/ufshcd.c | 17 ++++++++++++-----
>  include/ufs/ufshcd.h      |  3 +++
>  2 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index
> 721bf9d1a356..31ebf267135b 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -1395,13 +1395,17 @@ static int ufshcd_clock_scaling_prepare(struct
> ufs_hba *hba, u64 timeout_us)
>         return ret;
>  }
> 
> -static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, int err, bool
> scale_up)
> +static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, int
> +err)
>  {
>         up_write(&hba->clk_scaling_lock);
> 
> -       /* Enable Write Booster if we have scaled up else disable it */
> -       if (ufshcd_enable_wb_if_scaling_up(hba) && !err)
> -               ufshcd_wb_toggle(hba, scale_up);
> +       /* Enable Write Booster if current gear requires it else disable it */
> +       if (ufshcd_enable_wb_if_scaling_up(hba) && !err) {
> +               bool wb_en;
Can be initialized?

> +
> +               wb_en = hba->pwr_info.gear_rx >= hba->clk_scaling.wb_gear ? true
> : false;

If (wb != hba->dev_info.wb_enabled)
> +               ufshcd_wb_toggle(hba, wb_en);
> +       }
Wouldn't it make sense to move the wb toggling to ufshcd_scale_gear ?
This way you'll be able to leave the legacy on/off toggling?

> 
>         mutex_unlock(&hba->wb_mutex);
> 
> @@ -1456,7 +1460,7 @@ static int ufshcd_devfreq_scale(struct ufs_hba
> *hba, unsigned long freq,
>         }
> 
>  out_unprepare:
> -       ufshcd_clock_scaling_unprepare(hba, ret, scale_up);
> +       ufshcd_clock_scaling_unprepare(hba, ret);
>         return ret;
>  }
> 
> @@ -1816,6 +1820,9 @@ static void ufshcd_init_clk_scaling(struct ufs_hba
> *hba)
>         if (!hba->clk_scaling.min_gear)
>                 hba->clk_scaling.min_gear = UFS_HS_G1;
> 
> +       if (!hba->clk_scaling.wb_gear)
> +               hba->clk_scaling.wb_gear = UFS_HS_G3;
So you will toggle wb off on init (pwm) and on sporadic writes.
I guess there is no harm done.

Thanks,
Avri

> +
>         INIT_WORK(&hba->clk_scaling.suspend_work,
>                   ufshcd_clk_scaling_suspend_work);
>         INIT_WORK(&hba->clk_scaling.resume_work,
> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h index
> 8c7c497d63d3..8e6c2eb68011 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -448,6 +448,8 @@ struct ufs_clk_gating {
>   * @resume_work: worker to resume devfreq
>   * @target_freq: frequency requested by devfreq framework
>   * @min_gear: lowest HS gear to scale down to
> + * @wb_gear: enable Write Booster when HS gear scales above or equal to
> it, else
> + *             disable Write Booster
>   * @is_enabled: tracks if scaling is currently enabled or not, controlled by
>   *             clkscale_enable sysfs node
>   * @is_allowed: tracks if scaling is currently allowed or not, used to block
> @@ -468,6 +470,7 @@ struct ufs_clk_scaling {
>         struct work_struct resume_work;
>         unsigned long target_freq;
>         u32 min_gear;
> +       u32 wb_gear;
>         bool is_enabled;
>         bool is_allowed;
>         bool is_initialized;
> --
> 2.34.1

Re: [PATCH 7/8] scsi: ufs: core: Toggle Write Booster during clock scaling base on gear speed

[Ziqi Chen]

Hi  Avri,

Thanks for your comment.

On 1/16/2025 9:27 PM, Avri Altman wrote:
>>
>> From: Can Guo <quic_cang@quicinc.com>
>>
>> During clock scaling, Write Booster is toggled on or off based on whether the
>> clock is scaled up or down. However, with OPP V2 powered multi-level gear
>> scaling, the gear can be scaled amongst multiple gear speeds, e.g., it may
>> scale down from G5 to G4, or from G4 to G2. To provide flexibilities, add a
>> new field for clock scaling such that during clock scaling Write Booster can be
>> enabled or disabled based on gear speeds but not based on scaling up or
>> down.
>>
>> Co-developed-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>> Signed-off-by: Can Guo <quic_cang@quicinc.com>
>> ---
>>   drivers/ufs/core/ufshcd.c | 17 ++++++++++++-----
>>   include/ufs/ufshcd.h      |  3 +++
>>   2 files changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index
>> 721bf9d1a356..31ebf267135b 100644
>> --- a/drivers/ufs/core/ufshcd.c
>> +++ b/drivers/ufs/core/ufshcd.c
>> @@ -1395,13 +1395,17 @@ static int ufshcd_clock_scaling_prepare(struct
>> ufs_hba *hba, u64 timeout_us)
>>          return ret;
>>   }
>>
>> -static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, int err, bool
>> scale_up)
>> +static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, int
>> +err)
>>   {
>>          up_write(&hba->clk_scaling_lock);
>>
>> -       /* Enable Write Booster if we have scaled up else disable it */
>> -       if (ufshcd_enable_wb_if_scaling_up(hba) && !err)
>> -               ufshcd_wb_toggle(hba, scale_up);
>> +       /* Enable Write Booster if current gear requires it else disable it */
>> +       if (ufshcd_enable_wb_if_scaling_up(hba) && !err) {
>> +               bool wb_en;
> Can be initialized?
>

In this code, the wb_en variable does not need to be explicitly 
initialized. This is because within the conditional statement, wb_en is 
assigned a value based on the comparison between hba->pwr_info.gear_rx 
and hba->clk_scaling.wb_gear. Therefore, regardless of the condition, 
wb_en will be assigned either true or false.

However, it is a good practice to ensure that wb_en is correctly 
assigned in all possible code paths.  Thanks for your suggestion, I will 
initialize it as "False" in next version.

>> +
>> +               wb_en = hba->pwr_info.gear_rx >= hba->clk_scaling.wb_gear ? true
>> : false;
> 
> If (wb != hba->dev_info.wb_enabled)
>> +               ufshcd_wb_toggle(hba, wb_en);
>> +       }
> Wouldn't it make sense to move the wb toggling to ufshcd_scale_gear ?
> This way you'll be able to leave the legacy on/off toggling?
>

We don't need legacy wb on/off any more. Regarding the logic of this 
series, even if gear scale go to legacy branch , current wb toggle logic 
can also cover it.

for example, for legacy gear scale , it only has 2 possible gear speed 
mode, scal up to max gear or scale down to G1, we choose G3 as the 
wb_gear toggle gear can cover legacy WB toggle.

Any more , some customer may want the WB keep ON or OFF, they can set 
the wb_gear to G0 or max_gear to meet their requirement.

>>
>>          mutex_unlock(&hba->wb_mutex);
>>
>> @@ -1456,7 +1460,7 @@ static int ufshcd_devfreq_scale(struct ufs_hba
>> *hba, unsigned long freq,
>>          }
>>
>>   out_unprepare:
>> -       ufshcd_clock_scaling_unprepare(hba, ret, scale_up);
>> +       ufshcd_clock_scaling_unprepare(hba, ret);
>>          return ret;
>>   }
>>
>> @@ -1816,6 +1820,9 @@ static void ufshcd_init_clk_scaling(struct ufs_hba
>> *hba)
>>          if (!hba->clk_scaling.min_gear)
>>                  hba->clk_scaling.min_gear = UFS_HS_G1;
>>
>> +       if (!hba->clk_scaling.wb_gear)
>> +               hba->clk_scaling.wb_gear = UFS_HS_G3;
> So you will toggle wb off on init (pwm) and on sporadic writes.
> I guess there is no harm done.
>

No , it won't toggle wb off on init. As per UFS hba probe sequence, the 
timing of devfreq init is very late.  The WB will be turn ON at the 
early init and complete whole init sequence with high gear speed mode.
Not worry about WB would be turn OFF during init.


> Thanks,
> Avri

-Ziqi

> 
>> +
>>          INIT_WORK(&hba->clk_scaling.suspend_work,
>>                    ufshcd_clk_scaling_suspend_work);
>>          INIT_WORK(&hba->clk_scaling.resume_work,
>> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h index
>> 8c7c497d63d3..8e6c2eb68011 100644
>> --- a/include/ufs/ufshcd.h
>> +++ b/include/ufs/ufshcd.h
>> @@ -448,6 +448,8 @@ struct ufs_clk_gating {
>>    * @resume_work: worker to resume devfreq
>>    * @target_freq: frequency requested by devfreq framework
>>    * @min_gear: lowest HS gear to scale down to
>> + * @wb_gear: enable Write Booster when HS gear scales above or equal to
>> it, else
>> + *             disable Write Booster
>>    * @is_enabled: tracks if scaling is currently enabled or not, controlled by
>>    *             clkscale_enable sysfs node
>>    * @is_allowed: tracks if scaling is currently allowed or not, used to block
>> @@ -468,6 +470,7 @@ struct ufs_clk_scaling {
>>          struct work_struct resume_work;
>>          unsigned long target_freq;
>>          u32 min_gear;
>> +       u32 wb_gear;
>>          bool is_enabled;
>>          bool is_allowed;
>>          bool is_initialized;
>> --
>> 2.34.1
> 

[PATCH 8/8] ARM: dts: msm: Use Operation Points V2 for UFS on SM8650

[Ziqi Chen]

Use Operation Points V2 for UFS on SM8650 so that multi-level
clock/gear scaling can be possible.

Co-developed-by: Can Guo <quic_cang@quicinc.com>
Signed-off-by: Can Guo <quic_cang@quicinc.com>
Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sm8650.dtsi | 51 +++++++++++++++++++++++-----
 1 file changed, 43 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
index 01ac3769ffa6..5466f1217f64 100644
--- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
@@ -2557,18 +2557,11 @@ ufs_mem_hc: ufs@1d84000 {
 				      "tx_lane0_sync_clk",
 				      "rx_lane0_sync_clk",
 				      "rx_lane1_sync_clk";
-			freq-table-hz = <100000000 403000000>,
-					<0 0>,
-					<0 0>,
-					<100000000 403000000>,
-					<100000000 403000000>,
-					<0 0>,
-					<0 0>,
-					<0 0>;
 
 			resets = <&gcc GCC_UFS_PHY_BCR>;
 			reset-names = "rst";
 
+			operating-points-v2 = <&ufs_opp_table>;
 			interconnects = <&aggre1_noc MASTER_UFS_MEM QCOM_ICC_TAG_ALWAYS
 					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
 					<&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS
@@ -2590,6 +2583,48 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
 			#reset-cells = <1>;
 
 			status = "disabled";
+
+			ufs_opp_table: opp-table {
+					   compatible = "operating-points-v2";
+					   // LOW_SVS
+					   opp-100000000 {
+							   opp-hz = /bits/ 64 <100000000>,
+									   /bits/ 64 <0>,
+									   /bits/ 64 <0>,
+									   /bits/ 64 <100000000>,
+									   /bits/ 64 <0>,
+									   /bits/ 64 <0>,
+									   /bits/ 64 <0>,
+									   /bits/ 64 <0>;
+							   required-opps = <&rpmhpd_opp_low_svs>;
+					   };
+
+					   // SVS
+					   opp-201500000 {
+							   opp-hz = /bits/ 64 <201500000>,
+									   /bits/ 64 <0>,
+									   /bits/ 64 <0>,
+									   /bits/ 64 <201500000>,
+									   /bits/ 64 <0>,
+									   /bits/ 64 <0>,
+									   /bits/ 64 <0>,
+									   /bits/ 64 <0>;
+							   required-opps = <&rpmhpd_opp_svs>;
+					   };
+
+					   // NOM/TURBO
+					   opp-403000000 {
+							   opp-hz = /bits/ 64 <403000000>,
+									   /bits/ 64 <0>,
+									   /bits/ 64 <0>,
+									   /bits/ 64 <403000000>,
+									   /bits/ 64 <0>,
+									   /bits/ 64 <0>,
+									   /bits/ 64 <0>,
+									   /bits/ 64 <0>;
+							   required-opps = <&rpmhpd_opp_nom>;
+					   };
+			   };
 		};
 
 		ice: crypto@1d88000 {
-- 
2.34.1

Re: [PATCH 8/8] ARM: dts: msm: Use Operation Points V2 for UFS on SM8650

[Krzysztof Kozlowski]

On 16/01/2025 10:11, Ziqi Chen wrote:
> Use Operation Points V2 for UFS on SM8650 so that multi-level
> clock/gear scaling can be possible.
> 
> Co-developed-by: Can Guo <quic_cang@quicinc.com>
> Signed-off-by: Can Guo <quic_cang@quicinc.com>
> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>

Please don't send downstream code directly, but fix it first. Actually -
rework it 100%.

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

> ---
>  arch/arm64/boot/dts/qcom/sm8650.dtsi | 51 +++++++++++++++++++++++-----
>  1 file changed, 43 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> index 01ac3769ffa6..5466f1217f64 100644
> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> @@ -2557,18 +2557,11 @@ ufs_mem_hc: ufs@1d84000 {
>  				      "tx_lane0_sync_clk",
>  				      "rx_lane0_sync_clk",
>  				      "rx_lane1_sync_clk";
> -			freq-table-hz = <100000000 403000000>,
> -					<0 0>,
> -					<0 0>,
> -					<100000000 403000000>,
> -					<100000000 403000000>,
> -					<0 0>,
> -					<0 0>,
> -					<0 0>;
>  
>  			resets = <&gcc GCC_UFS_PHY_BCR>;
>  			reset-names = "rst";
>  
> +			operating-points-v2 = <&ufs_opp_table>;
>  			interconnects = <&aggre1_noc MASTER_UFS_MEM QCOM_ICC_TAG_ALWAYS
>  					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
>  					<&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS
> @@ -2590,6 +2583,48 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
>  			#reset-cells = <1>;
>  
>  			status = "disabled";
> +
> +			ufs_opp_table: opp-table {
> +					   compatible = "operating-points-v2";


Messed indentation.

> +					   // LOW_SVS


Drop

> +					   opp-100000000 {
> +							   opp-hz = /bits/ 64 <100000000>,
> +									   /bits/ 64 <0>,

Messed alignment.

> +									   /bits/ 64 <0>,
> +									   /bits/ 64 <100000000>,
> +									   /bits/ 64 <0>,
> +									   /bits/ 64 <0>,
> +									   /bits/ 64 <0>,




Best regards,
Krzysztof

Re: [PATCH 8/8] ARM: dts: msm: Use Operation Points V2 for UFS on SM8650

[Ziqi Chen]

Hi Krzysztof,

Thanks for review and comments~

As Neil has submitted a similar patch:
https://lore.kernel.org/all/20250115-topic-sm8x50-upstream-dt-icc-update-v1-10-eaa8b10e2af7@linaro.org/ 

I will withdraw this patch in next version.

-Ziqi

On 1/16/2025 5:22 PM, Krzysztof Kozlowski wrote:
> On 16/01/2025 10:11, Ziqi Chen wrote:
>> Use Operation Points V2 for UFS on SM8650 so that multi-level
>> clock/gear scaling can be possible.
>>
>> Co-developed-by: Can Guo <quic_cang@quicinc.com>
>> Signed-off-by: Can Guo <quic_cang@quicinc.com>
>> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> 
> Please don't send downstream code directly, but fix it first. Actually -
> rework it 100%.
> 
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching. For bindings, the preferred subjects are
> explained here:
> https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
> >> ---
>>   arch/arm64/boot/dts/qcom/sm8650.dtsi | 51 +++++++++++++++++++++++-----
>>   1 file changed, 43 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
>> index 01ac3769ffa6..5466f1217f64 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
>> @@ -2557,18 +2557,11 @@ ufs_mem_hc: ufs@1d84000 {
>>   				      "tx_lane0_sync_clk",
>>   				      "rx_lane0_sync_clk",
>>   				      "rx_lane1_sync_clk";
>> -			freq-table-hz = <100000000 403000000>,
>> -					<0 0>,
>> -					<0 0>,
>> -					<100000000 403000000>,
>> -					<100000000 403000000>,
>> -					<0 0>,
>> -					<0 0>,
>> -					<0 0>;
>>   
>>   			resets = <&gcc GCC_UFS_PHY_BCR>;
>>   			reset-names = "rst";
>>   
>> +			operating-points-v2 = <&ufs_opp_table>;
>>   			interconnects = <&aggre1_noc MASTER_UFS_MEM QCOM_ICC_TAG_ALWAYS
>>   					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
>>   					<&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS
>> @@ -2590,6 +2583,48 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
>>   			#reset-cells = <1>;
>>   
>>   			status = "disabled";
>> +
>> +			ufs_opp_table: opp-table {
>> +					   compatible = "operating-points-v2";
> 
> 
> Messed indentation.
> 
>> +					   // LOW_SVS
> 
> 
> Drop
> 
>> +					   opp-100000000 {
>> +							   opp-hz = /bits/ 64 <100000000>,
>> +									   /bits/ 64 <0>,
> 
> Messed alignment.
> 
>> +									   /bits/ 64 <0>,
>> +									   /bits/ 64 <100000000>,
>> +									   /bits/ 64 <0>,
>> +									   /bits/ 64 <0>,
>> +									   /bits/ 64 <0>,
> 
> 
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH 8/8] ARM: dts: msm: Use Operation Points V2 for UFS on SM8650

[neil.armstrong]

Hi,

On 16/01/2025 10:11, Ziqi Chen wrote:
> Use Operation Points V2 for UFS on SM8650 so that multi-level
> clock/gear scaling can be possible.


I've already sent a similar one at https://lore.kernel.org/all/20250115-topic-sm8x50-upstream-dt-icc-update-v1-10-eaa8b10e2af7@linaro.org/

Neil

> 
> Co-developed-by: Can Guo <quic_cang@quicinc.com>
> Signed-off-by: Can Guo <quic_cang@quicinc.com>
> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> ---
>   arch/arm64/boot/dts/qcom/sm8650.dtsi | 51 +++++++++++++++++++++++-----
>   1 file changed, 43 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> index 01ac3769ffa6..5466f1217f64 100644
> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> @@ -2557,18 +2557,11 @@ ufs_mem_hc: ufs@1d84000 {
>   				      "tx_lane0_sync_clk",
>   				      "rx_lane0_sync_clk",
>   				      "rx_lane1_sync_clk";
> -			freq-table-hz = <100000000 403000000>,
> -					<0 0>,
> -					<0 0>,
> -					<100000000 403000000>,
> -					<100000000 403000000>,
> -					<0 0>,
> -					<0 0>,
> -					<0 0>;
>   
>   			resets = <&gcc GCC_UFS_PHY_BCR>;
>   			reset-names = "rst";
>   
> +			operating-points-v2 = <&ufs_opp_table>;
>   			interconnects = <&aggre1_noc MASTER_UFS_MEM QCOM_ICC_TAG_ALWAYS
>   					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
>   					<&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS
> @@ -2590,6 +2583,48 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
>   			#reset-cells = <1>;
>   
>   			status = "disabled";
> +
> +			ufs_opp_table: opp-table {
> +					   compatible = "operating-points-v2";
> +					   // LOW_SVS
> +					   opp-100000000 {
> +							   opp-hz = /bits/ 64 <100000000>,
> +									   /bits/ 64 <0>,
> +									   /bits/ 64 <0>,
> +									   /bits/ 64 <100000000>,
> +									   /bits/ 64 <0>,
> +									   /bits/ 64 <0>,
> +									   /bits/ 64 <0>,
> +									   /bits/ 64 <0>;
> +							   required-opps = <&rpmhpd_opp_low_svs>;
> +					   };
> +
> +					   // SVS
> +					   opp-201500000 {
> +							   opp-hz = /bits/ 64 <201500000>,
> +									   /bits/ 64 <0>,
> +									   /bits/ 64 <0>,
> +									   /bits/ 64 <201500000>,
> +									   /bits/ 64 <0>,
> +									   /bits/ 64 <0>,
> +									   /bits/ 64 <0>,
> +									   /bits/ 64 <0>;
> +							   required-opps = <&rpmhpd_opp_svs>;
> +					   };
> +
> +					   // NOM/TURBO
> +					   opp-403000000 {
> +							   opp-hz = /bits/ 64 <403000000>,
> +									   /bits/ 64 <0>,
> +									   /bits/ 64 <0>,
> +									   /bits/ 64 <403000000>,
> +									   /bits/ 64 <0>,
> +									   /bits/ 64 <0>,
> +									   /bits/ 64 <0>,
> +									   /bits/ 64 <0>;
> +							   required-opps = <&rpmhpd_opp_nom>;
> +					   };
> +			   };
>   		};
>   
>   		ice: crypto@1d88000 {

Re: [PATCH 8/8] ARM: dts: msm: Use Operation Points V2 for UFS on SM8650

[Ziqi Chen]

On 1/16/2025 5:24 PM, neil.armstrong@linaro.org wrote:
> Hi,
> 
> On 16/01/2025 10:11, Ziqi Chen wrote:
>> Use Operation Points V2 for UFS on SM8650 so that multi-level
>> clock/gear scaling can be possible.
> 
> 
> I've already sent a similar one at 
> https://lore.kernel.org/all/20250115-topic-sm8x50-upstream-dt-icc-update-v1-10-eaa8b10e2af7@linaro.org/
> 
> Neil
> 

Hi Neil,

Thank you for reminder , I will withdraw this patch in next version.

- Ziqi

>>
>> Co-developed-by: Can Guo <quic_cang@quicinc.com>
>> Signed-off-by: Can Guo <quic_cang@quicinc.com>
>> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/sm8650.dtsi | 51 +++++++++++++++++++++++-----
>>   1 file changed, 43 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi 
>> b/arch/arm64/boot/dts/qcom/sm8650.dtsi
>> index 01ac3769ffa6..5466f1217f64 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
>> @@ -2557,18 +2557,11 @@ ufs_mem_hc: ufs@1d84000 {
>>                         "tx_lane0_sync_clk",
>>                         "rx_lane0_sync_clk",
>>                         "rx_lane1_sync_clk";
>> -            freq-table-hz = <100000000 403000000>,
>> -                    <0 0>,
>> -                    <0 0>,
>> -                    <100000000 403000000>,
>> -                    <100000000 403000000>,
>> -                    <0 0>,
>> -                    <0 0>,
>> -                    <0 0>;
>>               resets = <&gcc GCC_UFS_PHY_BCR>;
>>               reset-names = "rst";
>> +            operating-points-v2 = <&ufs_opp_table>;
>>               interconnects = <&aggre1_noc MASTER_UFS_MEM 
>> QCOM_ICC_TAG_ALWAYS
>>                        &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
>>                       <&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS
>> @@ -2590,6 +2583,48 @@ &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
>>               #reset-cells = <1>;
>>               status = "disabled";
>> +
>> +            ufs_opp_table: opp-table {
>> +                       compatible = "operating-points-v2";
>> +                       // LOW_SVS
>> +                       opp-100000000 {
>> +                               opp-hz = /bits/ 64 <100000000>,
>> +                                       /bits/ 64 <0>,
>> +                                       /bits/ 64 <0>,
>> +                                       /bits/ 64 <100000000>,
>> +                                       /bits/ 64 <0>,
>> +                                       /bits/ 64 <0>,
>> +                                       /bits/ 64 <0>,
>> +                                       /bits/ 64 <0>;
>> +                               required-opps = <&rpmhpd_opp_low_svs>;
>> +                       };
>> +
>> +                       // SVS
>> +                       opp-201500000 {
>> +                               opp-hz = /bits/ 64 <201500000>,
>> +                                       /bits/ 64 <0>,
>> +                                       /bits/ 64 <0>,
>> +                                       /bits/ 64 <201500000>,
>> +                                       /bits/ 64 <0>,
>> +                                       /bits/ 64 <0>,
>> +                                       /bits/ 64 <0>,
>> +                                       /bits/ 64 <0>;
>> +                               required-opps = <&rpmhpd_opp_svs>;
>> +                       };
>> +
>> +                       // NOM/TURBO
>> +                       opp-403000000 {
>> +                               opp-hz = /bits/ 64 <403000000>,
>> +                                       /bits/ 64 <0>,
>> +                                       /bits/ 64 <0>,
>> +                                       /bits/ 64 <403000000>,
>> +                                       /bits/ 64 <0>,
>> +                                       /bits/ 64 <0>,
>> +                                       /bits/ 64 <0>,
>> +                                       /bits/ 64 <0>;
>> +                               required-opps = <&rpmhpd_opp_nom>;
>> +                       };
>> +               };
>>           };
>>           ice: crypto@1d88000 {
> 

Re: [PATCH 0/8] Support Multi-frequency scale for UFS

[Neil Armstrong]

Hi,

[+linux-arm-msm@vger.kernel.org]

On 16/01/2025 10:11, Ziqi Chen wrote:
> With OPP V2 enabled, devfreq can scale clocks amongst multiple frequency
> plans. However, the gear speed is only toggled between min and max during
> clock scaling. Enable multi-level gear scaling by mapping clock frequencies
> to gear speeds, so that when devfreq scales clock frequencies we can put
> the UFS link at the appropraite gear speeds accordingly.
> 
> This series has been tested on below platforms -
> SM8650 + UFS3.1

Which board did you use ? the MTP ?

> SM8750 + UFS4.0

Did you alse test it on SM8550 ? this platform is also concerned.
And perhaps SM8450 should be also converted to the OPP table & tested.

Please Cc linux-arm-msm on all patches since we're directly concerned by
the whole changeset.

Thanks,
Neil

> 
> 
> Can Guo (6):
>    scsi: ufs: core: Pass target_freq to clk_scale_notify() vops
>    scsi: ufs: qcom: Pass target_freq to clk scale pre and post change
>    scsi: ufs: core: Add a vops to map clock frequency to gear speed
>    scsi: ufs: qcom: Implement the freq_to_gear_speed() vops
>    scsi: ufs: core: Enable multi-level gear scaling
>    scsi: ufs: core: Toggle Write Booster during clock scaling base on
>      gear speed
> 
> Ziqi Chen (2):
>    scsi: ufs: core: Check if scaling up is required when disable clkscale
>    ARM: dts: msm: Use Operation Points V2 for UFS on SM8650
> 
>   arch/arm64/boot/dts/qcom/sm8650.dtsi | 51 ++++++++++++++++----
>   drivers/ufs/core/ufshcd-priv.h       | 17 +++++--
>   drivers/ufs/core/ufshcd.c            | 71 +++++++++++++++++++++-------
>   drivers/ufs/host/ufs-mediatek.c      |  1 +
>   drivers/ufs/host/ufs-qcom.c          | 60 ++++++++++++++++++-----
>   include/ufs/ufshcd.h                 |  8 +++-
>   6 files changed, 166 insertions(+), 42 deletions(-)
> 

Re: [PATCH 0/8] Support Multi-frequency scale for UFS

[Ziqi Chen]

Hi Neil,

Thanks for your comment.

On 1/16/2025 5:28 PM, Neil Armstrong wrote:
> Hi,
> 
> [+linux-arm-msm@vger.kernel.org]
> 
> On 16/01/2025 10:11, Ziqi Chen wrote:
>> With OPP V2 enabled, devfreq can scale clocks amongst multiple frequency
>> plans. However, the gear speed is only toggled between min and max during
>> clock scaling. Enable multi-level gear scaling by mapping clock 
>> frequencies
>> to gear speeds, so that when devfreq scales clock frequencies we can put
>> the UFS link at the appropraite gear speeds accordingly.
>>
>> This series has been tested on below platforms -
>> SM8650 + UFS3.1
> 
> Which board did you use ? the MTP ? >

We tested on MTP.

>> SM8750 + UFS4.0
> 
> Did you alse test it on SM8550 ? this platform is also concerned.
> And perhaps SM8450 should be also converted to the OPP table & tested.
>
We didn't test in on SM8550 before, but now we already tested it on 
SM8550 once see you this comment. It works fine on SM8550 as well.
I will update this information in next version.

> Please Cc linux-arm-msm on all patches since we're directly concerned by
> the whole changeset.

Sure , Thank you for reminder, I will CC this group from next version.

> 
> Thanks,
> Neil
>

-Ziqi

>>
>>
>> Can Guo (6):
>>    scsi: ufs: core: Pass target_freq to clk_scale_notify() vops
>>    scsi: ufs: qcom: Pass target_freq to clk scale pre and post change
>>    scsi: ufs: core: Add a vops to map clock frequency to gear speed
>>    scsi: ufs: qcom: Implement the freq_to_gear_speed() vops
>>    scsi: ufs: core: Enable multi-level gear scaling
>>    scsi: ufs: core: Toggle Write Booster during clock scaling base on
>>      gear speed
>>
>> Ziqi Chen (2):
>>    scsi: ufs: core: Check if scaling up is required when disable clkscale
>>    ARM: dts: msm: Use Operation Points V2 for UFS on SM8650
>>
>>   arch/arm64/boot/dts/qcom/sm8650.dtsi | 51 ++++++++++++++++----
>>   drivers/ufs/core/ufshcd-priv.h       | 17 +++++--
>>   drivers/ufs/core/ufshcd.c            | 71 +++++++++++++++++++++-------
>>   drivers/ufs/host/ufs-mediatek.c      |  1 +
>>   drivers/ufs/host/ufs-qcom.c          | 60 ++++++++++++++++++-----
>>   include/ufs/ufshcd.h                 |  8 +++-
>>   6 files changed, 166 insertions(+), 42 deletions(-)
>>
> 

Re: [PATCH 0/8] Support Multi-frequency scale for UFS

[Manivannan Sadhasivam]

On Thu, Jan 16, 2025 at 05:11:41PM +0800, Ziqi Chen wrote:

You missed CCing linux-arm-msm mailing list to the cover letter.

> With OPP V2 enabled, devfreq can scale clocks amongst multiple frequency
> plans. However, the gear speed is only toggled between min and max during
> clock scaling. Enable multi-level gear scaling by mapping clock frequencies
> to gear speeds, so that when devfreq scales clock frequencies we can put
> the UFS link at the appropraite gear speeds accordingly.
> 

But the UFSHC PHY settings are not updated for each gear speed, isn't it? Then
I'm wondering how much we get out of this 'multi-level gear scaling'.

- Mani

> This series has been tested on below platforms -
> SM8650 + UFS3.1
> SM8750 + UFS4.0
> 
> 
> Can Guo (6):
>   scsi: ufs: core: Pass target_freq to clk_scale_notify() vops
>   scsi: ufs: qcom: Pass target_freq to clk scale pre and post change
>   scsi: ufs: core: Add a vops to map clock frequency to gear speed
>   scsi: ufs: qcom: Implement the freq_to_gear_speed() vops
>   scsi: ufs: core: Enable multi-level gear scaling
>   scsi: ufs: core: Toggle Write Booster during clock scaling base on
>     gear speed
> 
> Ziqi Chen (2):
>   scsi: ufs: core: Check if scaling up is required when disable clkscale
>   ARM: dts: msm: Use Operation Points V2 for UFS on SM8650
> 
>  arch/arm64/boot/dts/qcom/sm8650.dtsi | 51 ++++++++++++++++----
>  drivers/ufs/core/ufshcd-priv.h       | 17 +++++--
>  drivers/ufs/core/ufshcd.c            | 71 +++++++++++++++++++++-------
>  drivers/ufs/host/ufs-mediatek.c      |  1 +
>  drivers/ufs/host/ufs-qcom.c          | 60 ++++++++++++++++++-----
>  include/ufs/ufshcd.h                 |  8 +++-
>  6 files changed, 166 insertions(+), 42 deletions(-)
> 
> -- 
> 2.34.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 0/8] Support Multi-frequency scale for UFS

[Ziqi Chen]

Hi Mani,

Thanks for you review~

On 1/19/2025 3:57 PM, Manivannan Sadhasivam wrote:
> On Thu, Jan 16, 2025 at 05:11:41PM +0800, Ziqi Chen wrote:
> 
> You missed CCing linux-arm-msm mailing list to the cover letter.
>
Thank you for reminder, I will cc this group in next patch version.

>> With OPP V2 enabled, devfreq can scale clocks amongst multiple frequency
>> plans. However, the gear speed is only toggled between min and max during
>> clock scaling. Enable multi-level gear scaling by mapping clock frequencies
>> to gear speeds, so that when devfreq scales clock frequencies we can put
>> the UFS link at the appropraite gear speeds accordingly.
>>
> 
> But the UFSHC PHY settings are not updated for each gear speed, isn't it? Then
> I'm wondering how much we get out of this 'multi-level gear scaling'.

Per design, we don't need to update any PHY setting for each gear speed 
mode.

> 
> - Mani
> 

-Ziqi

>> This series has been tested on below platforms -
>> SM8650 + UFS3.1
>> SM8750 + UFS4.0
>>
>>
>> Can Guo (6):
>>    scsi: ufs: core: Pass target_freq to clk_scale_notify() vops
>>    scsi: ufs: qcom: Pass target_freq to clk scale pre and post change
>>    scsi: ufs: core: Add a vops to map clock frequency to gear speed
>>    scsi: ufs: qcom: Implement the freq_to_gear_speed() vops
>>    scsi: ufs: core: Enable multi-level gear scaling
>>    scsi: ufs: core: Toggle Write Booster during clock scaling base on
>>      gear speed
>>
>> Ziqi Chen (2):
>>    scsi: ufs: core: Check if scaling up is required when disable clkscale
>>    ARM: dts: msm: Use Operation Points V2 for UFS on SM8650
>>
>>   arch/arm64/boot/dts/qcom/sm8650.dtsi | 51 ++++++++++++++++----
>>   drivers/ufs/core/ufshcd-priv.h       | 17 +++++--
>>   drivers/ufs/core/ufshcd.c            | 71 +++++++++++++++++++++-------
>>   drivers/ufs/host/ufs-mediatek.c      |  1 +
>>   drivers/ufs/host/ufs-qcom.c          | 60 ++++++++++++++++++-----
>>   include/ufs/ufshcd.h                 |  8 +++-
>>   6 files changed, 166 insertions(+), 42 deletions(-)
>>
>> -- 
>> 2.34.1
>>
>