[PATCH v2 1/3] scsi: ufs: qcom: Check gear against max gear in vop freq_to_gear()

[PATCH v2 1/3] scsi: ufs: qcom: Check gear against max gear in vop freq_to_gear()

[Ziqi Chen]

The vop freq_to_gear() may return a gear greater than the negotiated max
gear, return the negotiated max gear if the mapped gear is greater than it.

Tested-by: Neil Armstrong <neil.armstrong@linaro.org>
Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>

---
v1 - > v2:
1. Instead of return 'gear', return '0' directly if didn't find mapped
   gear
2. Derectly return min_t(gear,max_gear) instead assign to 'gear' then
   return it.
---
 drivers/ufs/host/ufs-qcom.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 790da25cbaf3..7f10926100a5 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -2105,10 +2105,10 @@ static u32 ufs_qcom_freq_to_gear_speed(struct ufs_hba *hba, unsigned long freq)
 		break;
 	default:
 		dev_err(hba->dev, "%s: Unsupported clock freq : %lu\n", __func__, freq);
-		break;
+		return 0;
 	}
 
-	return gear;
+	return min_t(u32, gear, hba->max_pwr_info.info.gear_rx);
 }
 
 /*
-- 
2.34.1

Re: [PATCH v2 1/3] scsi: ufs: qcom: Check gear against max gear in vop freq_to_gear()

[Bean Huo]

On Wed, 2025-05-07 at 15:44 +0800, Ziqi Chen wrote:
> The vop freq_to_gear() may return a gear greater than the negotiated
> max
> gear, return the negotiated max gear if the mapped gear is greater
> than it.
> 
> Tested-by: Neil Armstrong <neil.armstrong@linaro.org>
> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>

Reviewed-by: Bean Huo <beanhuo@micron.com>

Re: [PATCH v2 1/3] scsi: ufs: qcom: Check gear against max gear in vop freq_to_gear()

[Konrad Dybcio]

On 5/7/25 9:44 AM, Ziqi Chen wrote:
> The vop freq_to_gear() may return a gear greater than the negotiated max
> gear, return the negotiated max gear if the mapped gear is greater than it.
> 
> Tested-by: Neil Armstrong <neil.armstrong@linaro.org>
> Signed-off-by: Ziqi Chen <quic_ziqichen@quicinc.com>
> 
> ---
> v1 - > v2:
> 1. Instead of return 'gear', return '0' directly if didn't find mapped
>    gear
> 2. Derectly return min_t(gear,max_gear) instead assign to 'gear' then
>    return it.
> ---
>  drivers/ufs/host/ufs-qcom.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 790da25cbaf3..7f10926100a5 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -2105,10 +2105,10 @@ static u32 ufs_qcom_freq_to_gear_speed(struct ufs_hba *hba, unsigned long freq)
>  		break;
>  	default:
>  		dev_err(hba->dev, "%s: Unsupported clock freq : %lu\n", __func__, freq);
> -		break;
> +		return 0;

That's UFS_PWM_DONT_CHANGE, please it so that the reader can more easily
make sense of the code.. Actually, perhaps this function could be tagged
to return an entry of this enum specifically

Konrad

>  	}
>  
> -	return gear;
> +	return min_t(u32, gear, hba->max_pwr_info.info.gear_rx);
>  }
>  
>  /*

Re: [PATCH v2 1/3] scsi: ufs: qcom: Check gear against max gear in vop freq_to_gear()

[Ziqi Chen]

On 5/8/2025 11:30 PM, Konrad Dybcio wrote:
> That's UFS_PWM_DONT_CHANGE, please it so that the reader can more easily
> make sense of the code.. Actually, perhaps this function could be tagged
> to return an entry of this enum specifically

Sure , Thanks Konrad .

BRs,
Ziqi

[PATCH v2 2/3] scsi: ufs: qcom: Map devfreq OPP freq to UniPro Core Clock freq

[Ziqi Chen]

From: Can Guo <quic_cang@quicinc.com>

On some platforms, the devfreq OPP freq may be different than the unipro
core clock freq. Implement ufs_qcom_opp_freq_to_clk_freq() and use it to
find the unipro core clk freq.

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>
---
 drivers/ufs/host/ufs-qcom.c | 81 ++++++++++++++++++++++++++++++++-----
 1 file changed, 71 insertions(+), 10 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 7f10926100a5..804c8ccd8d03 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -122,7 +122,9 @@ static const struct {
 };
 
 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, unsigned long freq);
+static unsigned long ufs_qcom_opp_freq_to_clk_freq(struct ufs_hba *hba, unsigned long freq,
+												   char *name);
+static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up, unsigned long freq);
 
 static struct ufs_qcom_host *rcdev_to_ufs_host(struct reset_controller_dev *rcd)
 {
@@ -656,7 +658,7 @@ static int ufs_qcom_link_startup_notify(struct ufs_hba *hba,
 			return -EINVAL;
 		}
 
-		err = ufs_qcom_set_core_clk_ctrl(hba, ULONG_MAX);
+		err = ufs_qcom_set_core_clk_ctrl(hba, true, ULONG_MAX);
 		if (err)
 			dev_err(hba->dev, "cfg core clk ctrl failed\n");
 		/*
@@ -1414,29 +1416,46 @@ 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, unsigned long freq)
+static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up, unsigned long freq)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
 	struct list_head *head = &hba->clk_list_head;
 	struct ufs_clk_info *clki;
 	u32 cycles_in_1us = 0;
 	u32 core_clk_ctrl_reg;
+	unsigned long clk_freq;
 	int err;
 
+	if (hba->use_pm_opp) {
+		clk_freq = ufs_qcom_opp_freq_to_clk_freq(hba, freq, "core_clk_unipro");
+		if (clk_freq) {
+			cycles_in_1us = ceil(clk_freq, HZ_PER_MHZ);
+			goto set_core_clk_ctrl;
+		}
+	}
+
 	list_for_each_entry(clki, head, list) {
 		if (!IS_ERR_OR_NULL(clki->clk) &&
 		    !strcmp(clki->name, "core_clk_unipro")) {
-			if (!clki->max_freq)
+			if (!clki->max_freq) {
 				cycles_in_1us = 150; /* default for backwards compatibility */
-			else if (freq == ULONG_MAX)
+				break;
+			}
+
+			if (freq == ULONG_MAX) {
 				cycles_in_1us = ceil(clki->max_freq, HZ_PER_MHZ);
-			else
-				cycles_in_1us = ceil(freq, HZ_PER_MHZ);
+				break;
+			}
 
+			if (is_scale_up)
+				cycles_in_1us = ceil(clki->max_freq, HZ_PER_MHZ);
+			else
+				cycles_in_1us = ceil(clk_get_rate(clki->clk), HZ_PER_MHZ);
 			break;
 		}
 	}
 
+set_core_clk_ctrl:
 	err = ufshcd_dme_get(hba,
 			    UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL),
 			    &core_clk_ctrl_reg);
@@ -1479,7 +1498,7 @@ static int ufs_qcom_clk_scale_up_pre_change(struct ufs_hba *hba, unsigned long f
 		return ret;
 	}
 	/* set unipro core clock attributes and clear clock divider */
-	return ufs_qcom_set_core_clk_ctrl(hba, freq);
+	return ufs_qcom_set_core_clk_ctrl(hba, true, freq);
 }
 
 static int ufs_qcom_clk_scale_up_post_change(struct ufs_hba *hba)
@@ -1511,7 +1530,7 @@ static int ufs_qcom_clk_scale_down_pre_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, freq);
+	return ufs_qcom_set_core_clk_ctrl(hba, false, freq);
 }
 
 static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, bool scale_up,
@@ -2081,11 +2100,53 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba)
 	return ret;
 }
 
+static unsigned long ufs_qcom_opp_freq_to_clk_freq(struct ufs_hba *hba,
+												   unsigned long freq, char *name)
+{
+	struct ufs_clk_info *clki;
+	struct dev_pm_opp *opp;
+	unsigned long clk_freq;
+	int idx = 0;
+	bool found = false;
+
+	opp = dev_pm_opp_find_freq_exact_indexed(hba->dev, freq, 0, true);
+	if (IS_ERR(opp)) {
+		dev_err(hba->dev, "Failed to find OPP for exact frequency %lu\n", freq);
+		return 0;
+	}
+
+	list_for_each_entry(clki, &hba->clk_list_head, list) {
+		if (!strcmp(clki->name, name)) {
+			found = true;
+			break;
+		}
+
+		idx++;
+	}
+
+	if (!found) {
+		dev_err(hba->dev, "Failed to find clock '%s' in clk list\n", name);
+		dev_pm_opp_put(opp);
+		return 0;
+	}
+
+	clk_freq = dev_pm_opp_get_freq_indexed(opp, idx);
+
+	dev_pm_opp_put(opp);
+
+	return clk_freq;
+}
+
 static u32 ufs_qcom_freq_to_gear_speed(struct ufs_hba *hba, unsigned long freq)
 {
 	u32 gear = 0;
+	unsigned long unipro_freq;
+
+	if (!hba->use_pm_opp)
+		return gear;
 
-	switch (freq) {
+	unipro_freq = ufs_qcom_opp_freq_to_clk_freq(hba, freq, "core_clk_unipro");
+	switch (unipro_freq) {
 	case 403000000:
 		gear = UFS_HS_G5;
 		break;
-- 
2.34.1

Re: [PATCH v2 2/3] scsi: ufs: qcom: Map devfreq OPP freq to UniPro Core Clock freq

[Luca Weiss]

Hi Ziqi,

On Wed May 7, 2025 at 9:44 AM CEST, Ziqi Chen wrote:
> From: Can Guo <quic_cang@quicinc.com>
>
> On some platforms, the devfreq OPP freq may be different than the unipro
> core clock freq. Implement ufs_qcom_opp_freq_to_clk_freq() and use it to
> find the unipro core clk freq.
>
> 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>
> ---
>  drivers/ufs/host/ufs-qcom.c | 81 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 71 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 7f10926100a5..804c8ccd8d03 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -122,7 +122,9 @@ static const struct {
>  };
>  
>  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, unsigned long freq);
> +static unsigned long ufs_qcom_opp_freq_to_clk_freq(struct ufs_hba *hba, unsigned long freq,
> +												   char *name);
> +static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up, unsigned long freq);
>  
>  static struct ufs_qcom_host *rcdev_to_ufs_host(struct reset_controller_dev *rcd)
>  {
> @@ -656,7 +658,7 @@ static int ufs_qcom_link_startup_notify(struct ufs_hba *hba,
>  			return -EINVAL;
>  		}
>  
> -		err = ufs_qcom_set_core_clk_ctrl(hba, ULONG_MAX);
> +		err = ufs_qcom_set_core_clk_ctrl(hba, true, ULONG_MAX);
>  		if (err)
>  			dev_err(hba->dev, "cfg core clk ctrl failed\n");
>  		/*
> @@ -1414,29 +1416,46 @@ 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, unsigned long freq)
> +static int ufs_qcom_set_core_clk_ctrl(struct ufs_hba *hba, bool is_scale_up, unsigned long freq)
>  {
>  	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
>  	struct list_head *head = &hba->clk_list_head;
>  	struct ufs_clk_info *clki;
>  	u32 cycles_in_1us = 0;
>  	u32 core_clk_ctrl_reg;
> +	unsigned long clk_freq;
>  	int err;
>  
> +	if (hba->use_pm_opp) {
> +		clk_freq = ufs_qcom_opp_freq_to_clk_freq(hba, freq, "core_clk_unipro");
> +		if (clk_freq) {
> +			cycles_in_1us = ceil(clk_freq, HZ_PER_MHZ);
> +			goto set_core_clk_ctrl;
> +		}
> +	}
> +
>  	list_for_each_entry(clki, head, list) {
>  		if (!IS_ERR_OR_NULL(clki->clk) &&
>  		    !strcmp(clki->name, "core_clk_unipro")) {
> -			if (!clki->max_freq)
> +			if (!clki->max_freq) {
>  				cycles_in_1us = 150; /* default for backwards compatibility */
> -			else if (freq == ULONG_MAX)
> +				break;
> +			}
> +
> +			if (freq == ULONG_MAX) {
>  				cycles_in_1us = ceil(clki->max_freq, HZ_PER_MHZ);
> -			else
> -				cycles_in_1us = ceil(freq, HZ_PER_MHZ);
> +				break;
> +			}
>  
> +			if (is_scale_up)
> +				cycles_in_1us = ceil(clki->max_freq, HZ_PER_MHZ);
> +			else
> +				cycles_in_1us = ceil(clk_get_rate(clki->clk), HZ_PER_MHZ);
>  			break;
>  		}
>  	}
>  
> +set_core_clk_ctrl:
>  	err = ufshcd_dme_get(hba,
>  			    UIC_ARG_MIB(DME_VS_CORE_CLK_CTRL),
>  			    &core_clk_ctrl_reg);
> @@ -1479,7 +1498,7 @@ static int ufs_qcom_clk_scale_up_pre_change(struct ufs_hba *hba, unsigned long f
>  		return ret;
>  	}
>  	/* set unipro core clock attributes and clear clock divider */
> -	return ufs_qcom_set_core_clk_ctrl(hba, freq);
> +	return ufs_qcom_set_core_clk_ctrl(hba, true, freq);
>  }
>  
>  static int ufs_qcom_clk_scale_up_post_change(struct ufs_hba *hba)
> @@ -1511,7 +1530,7 @@ static int ufs_qcom_clk_scale_down_pre_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, freq);
> +	return ufs_qcom_set_core_clk_ctrl(hba, false, freq);
>  }
>  
>  static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, bool scale_up,
> @@ -2081,11 +2100,53 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba)
>  	return ret;
>  }
>  
> +static unsigned long ufs_qcom_opp_freq_to_clk_freq(struct ufs_hba *hba,
> +												   unsigned long freq, char *name)
> +{
> +	struct ufs_clk_info *clki;
> +	struct dev_pm_opp *opp;
> +	unsigned long clk_freq;
> +	int idx = 0;
> +	bool found = false;
> +
> +	opp = dev_pm_opp_find_freq_exact_indexed(hba->dev, freq, 0, true);
> +	if (IS_ERR(opp)) {
> +		dev_err(hba->dev, "Failed to find OPP for exact frequency %lu\n", freq);

I'm hitting this print on bootup:

[    0.512515] ufshcd-qcom 1d84000.ufshc: Failed to find OPP for exact frequency 18446744073709551615
[    0.512571] ufshcd-qcom 1d84000.ufshc: Failed to find OPP for exact frequency 18446744073709551615

Doesn't look like it's intended? The number is (2^64 - 1)

Regards
Luca

> +		return 0;
> +	}
> +
> +	list_for_each_entry(clki, &hba->clk_list_head, list) {
> +		if (!strcmp(clki->name, name)) {
> +			found = true;
> +			break;
> +		}
> +
> +		idx++;
> +	}
> +
> +	if (!found) {
> +		dev_err(hba->dev, "Failed to find clock '%s' in clk list\n", name);
> +		dev_pm_opp_put(opp);
> +		return 0;
> +	}
> +
> +	clk_freq = dev_pm_opp_get_freq_indexed(opp, idx);
> +
> +	dev_pm_opp_put(opp);
> +
> +	return clk_freq;
> +}
> +
>  static u32 ufs_qcom_freq_to_gear_speed(struct ufs_hba *hba, unsigned long freq)
>  {
>  	u32 gear = 0;
> +	unsigned long unipro_freq;
> +
> +	if (!hba->use_pm_opp)
> +		return gear;
>  
> -	switch (freq) {
> +	unipro_freq = ufs_qcom_opp_freq_to_clk_freq(hba, freq, "core_clk_unipro");
> +	switch (unipro_freq) {
>  	case 403000000:
>  		gear = UFS_HS_G5;
>  		break;

Re: [PATCH v2 2/3] scsi: ufs: qcom: Map devfreq OPP freq to UniPro Core Clock freq

[Ziqi Chen]

Hi Luca,

On 5/7/2025 4:19 PM, Luca Weiss wrote:
> Hi Ziqi,
> 
> On Wed May 7, 2025 at 9:44 AM CEST, Ziqi Chen wrote:
>> From: Can Guo <quic_cang@quicinc.com>
>>
>> On some platforms, the devfreq OPP freq may be different than the unipro
>> core clock freq. Implement ufs_qcom_opp_freq_to_clk_freq() and use it to
>> find the unipro core clk freq.
>>
>> 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>
>> ---
>>   drivers/ufs/host/ufs-qcom.c | 81 ++++++++++++++++++++++++++++++++-----
>>   1 file changed, 71 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>> index 7f10926100a5..804c8ccd8d03 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>>   
>> +static unsigned long ufs_qcom_opp_freq_to_clk_freq(struct ufs_hba *hba,
>> +												   unsigned long freq, char *name)
>> +{
>> +	struct ufs_clk_info *clki;
>> +	struct dev_pm_opp *opp;
>> +	unsigned long clk_freq;
>> +	int idx = 0;
>> +	bool found = false;
>> +
>> +	opp = dev_pm_opp_find_freq_exact_indexed(hba->dev, freq, 0, true);
>> +	if (IS_ERR(opp)) {
>> +		dev_err(hba->dev, "Failed to find OPP for exact frequency %lu\n", freq);
> 
> I'm hitting this print on bootup:
> 
> [    0.512515] ufshcd-qcom 1d84000.ufshc: Failed to find OPP for exact frequency 18446744073709551615
> [    0.512571] ufshcd-qcom 1d84000.ufshc: Failed to find OPP for exact frequency 18446744073709551615
> 
> Doesn't look like it's intended? The number is (2^64 - 1)
> 
Yes, this is expected. During link startup, the frequency
ULONG_MAX will be passed to ufs_qcom_set_core_clk_ctrl() and
ufs_qcom_cfg_timer(). This frequency cannot be found through the API
dev_pm_opp_find_freq_exact_indexed(). Therefore, we handle the
frequency ULONG_MAX separately within Ufs_qcom_set_core_clk_ctrl()
and ufs_qcom_cfg_timer().

This print only be print twice during link startup. If you think print
such print during bootup is not make sense, I can improve the code and
update a new vwesion.

BRs.
Ziqi

> Regards
> Luca
> 

Re: [PATCH v2 2/3] scsi: ufs: qcom: Map devfreq OPP freq to UniPro Core Clock freq

[Luca Weiss]

On Wed May 7, 2025 at 11:09 AM CEST, Ziqi Chen wrote:
> Hi Luca,
>
> On 5/7/2025 4:19 PM, Luca Weiss wrote:
>> Hi Ziqi,
>> 
>> On Wed May 7, 2025 at 9:44 AM CEST, Ziqi Chen wrote:
>>> From: Can Guo <quic_cang@quicinc.com>
>>>
>>> On some platforms, the devfreq OPP freq may be different than the unipro
>>> core clock freq. Implement ufs_qcom_opp_freq_to_clk_freq() and use it to
>>> find the unipro core clk freq.
>>>
>>> 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>
>>> ---
>>>   drivers/ufs/host/ufs-qcom.c | 81 ++++++++++++++++++++++++++++++++-----
>>>   1 file changed, 71 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>>> index 7f10926100a5..804c8ccd8d03 100644
>>> --- a/drivers/ufs/host/ufs-qcom.c
>>> +++ b/drivers/ufs/host/ufs-qcom.c
>>>   
>>> +static unsigned long ufs_qcom_opp_freq_to_clk_freq(struct ufs_hba *hba,
>>> +												   unsigned long freq, char *name)
>>> +{
>>> +	struct ufs_clk_info *clki;
>>> +	struct dev_pm_opp *opp;
>>> +	unsigned long clk_freq;
>>> +	int idx = 0;
>>> +	bool found = false;
>>> +
>>> +	opp = dev_pm_opp_find_freq_exact_indexed(hba->dev, freq, 0, true);
>>> +	if (IS_ERR(opp)) {
>>> +		dev_err(hba->dev, "Failed to find OPP for exact frequency %lu\n", freq);
>> 
>> I'm hitting this print on bootup:
>> 
>> [    0.512515] ufshcd-qcom 1d84000.ufshc: Failed to find OPP for exact frequency 18446744073709551615
>> [    0.512571] ufshcd-qcom 1d84000.ufshc: Failed to find OPP for exact frequency 18446744073709551615
>> 
>> Doesn't look like it's intended? The number is (2^64 - 1)
>> 
> Yes, this is expected. During link startup, the frequency
> ULONG_MAX will be passed to ufs_qcom_set_core_clk_ctrl() and
> ufs_qcom_cfg_timer(). This frequency cannot be found through the API
> dev_pm_opp_find_freq_exact_indexed(). Therefore, we handle the
> frequency ULONG_MAX separately within Ufs_qcom_set_core_clk_ctrl()
> and ufs_qcom_cfg_timer().
>
> This print only be print twice during link startup. If you think print
> such print during bootup is not make sense, I can improve the code and
> update a new vwesion.

I'll let others comment on what should happen but certainly this large
number looks more like a mistake, like an integer overflow, if you don't
dig into what this number is supposed to represent.

Perhaps an idea could be to just skip the print (or even more code) for
ULONG_MAX since an opp for that is not supposed to exist anyways?

I didn't check the code now but for other frequencies this would be an
actual error I imagine where it should be visible.

Regards
Luca

>
> BRs.
> Ziqi
>
>> Regards
>> Luca
>> 

Re: [PATCH v2 2/3] scsi: ufs: qcom: Map devfreq OPP freq to UniPro Core Clock freq

[Ziqi Chen]

On 5/7/2025 5:59 PM, Luca Weiss wrote:
> On Wed May 7, 2025 at 11:09 AM CEST, Ziqi Chen wrote:
>> Hi Luca,
>>
>> On 5/7/2025 4:19 PM, Luca Weiss wrote:
>>> Hi Ziqi,
>>>
>>> On Wed May 7, 2025 at 9:44 AM CEST, Ziqi Chen wrote:
>>>> From: Can Guo <quic_cang@quicinc.com>
>>>>
>>>> On some platforms, the devfreq OPP freq may be different than the unipro
>>>> core clock freq. Implement ufs_qcom_opp_freq_to_clk_freq() and use it to
>>>> find the unipro core clk freq.
>>>>
>>>> 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>
>>>> ---
>>>>    drivers/ufs/host/ufs-qcom.c | 81 ++++++++++++++++++++++++++++++++-----
>>>>    1 file changed, 71 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>>>> index 7f10926100a5..804c8ccd8d03 100644
>>>> --- a/drivers/ufs/host/ufs-qcom.c
>>>> +++ b/drivers/ufs/host/ufs-qcom.c
>>>>    
>>>> +static unsigned long ufs_qcom_opp_freq_to_clk_freq(struct ufs_hba *hba,
>>>> +												   unsigned long freq, char *name)
>>>> +{
>>>> +	struct ufs_clk_info *clki;
>>>> +	struct dev_pm_opp *opp;
>>>> +	unsigned long clk_freq;
>>>> +	int idx = 0;
>>>> +	bool found = false;
>>>> +
>>>> +	opp = dev_pm_opp_find_freq_exact_indexed(hba->dev, freq, 0, true);
>>>> +	if (IS_ERR(opp)) {
>>>> +		dev_err(hba->dev, "Failed to find OPP for exact frequency %lu\n", freq);
>>>
>>> I'm hitting this print on bootup:
>>>
>>> [    0.512515] ufshcd-qcom 1d84000.ufshc: Failed to find OPP for exact frequency 18446744073709551615
>>> [    0.512571] ufshcd-qcom 1d84000.ufshc: Failed to find OPP for exact frequency 18446744073709551615
>>>
>>> Doesn't look like it's intended? The number is (2^64 - 1)
>>>
>> Yes, this is expected. During link startup, the frequency
>> ULONG_MAX will be passed to ufs_qcom_set_core_clk_ctrl() and
>> ufs_qcom_cfg_timer(). This frequency cannot be found through the API
>> dev_pm_opp_find_freq_exact_indexed(). Therefore, we handle the
>> frequency ULONG_MAX separately within Ufs_qcom_set_core_clk_ctrl()
>> and ufs_qcom_cfg_timer().
>>
>> This print only be print twice during link startup. If you think print
>> such print during bootup is not make sense, I can improve the code and
>> update a new vwesion.
> 
> I'll let others comment on what should happen but certainly this large
> number looks more like a mistake, like an integer overflow, if you don't
> dig into what this number is supposed to represent.
> 
> Perhaps an idea could be to just skip the print (or even more code) for
> ULONG_MAX since an opp for that is not supposed to exist anyways?
> 
> I didn't check the code now but for other frequencies this would be an
> actual error I imagine where it should be visible.
> 
sure, let me clear away this prints in next version.

BRs,
Ziqi

> Regards
> Luca
> 
>>
>> BRs.
>> Ziqi
>>
>>> Regards
>>> Luca
>>>
> 

Re: [PATCH v2 2/3] scsi: ufs: qcom: Map devfreq OPP freq to UniPro Core Clock freq

[neil.armstrong]

On 07/05/2025 11:09, Ziqi Chen wrote:
> Hi Luca,
> 
> On 5/7/2025 4:19 PM, Luca Weiss wrote:
>> Hi Ziqi,
>>
>> On Wed May 7, 2025 at 9:44 AM CEST, Ziqi Chen wrote:
>>> From: Can Guo <quic_cang@quicinc.com>
>>>
>>> On some platforms, the devfreq OPP freq may be different than the unipro
>>> core clock freq. Implement ufs_qcom_opp_freq_to_clk_freq() and use it to
>>> find the unipro core clk freq.
>>>
>>> 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>
>>> ---
>>>   drivers/ufs/host/ufs-qcom.c | 81 ++++++++++++++++++++++++++++++++-----
>>>   1 file changed, 71 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>>> index 7f10926100a5..804c8ccd8d03 100644
>>> --- a/drivers/ufs/host/ufs-qcom.c
>>> +++ b/drivers/ufs/host/ufs-qcom.c
>>> +static unsigned long ufs_qcom_opp_freq_to_clk_freq(struct ufs_hba *hba,
>>> +                                                   unsigned long freq, char *name)
>>> +{
>>> +    struct ufs_clk_info *clki;
>>> +    struct dev_pm_opp *opp;
>>> +    unsigned long clk_freq;
>>> +    int idx = 0;
>>> +    bool found = false;
>>> +
>>> +    opp = dev_pm_opp_find_freq_exact_indexed(hba->dev, freq, 0, true);
>>> +    if (IS_ERR(opp)) {
>>> +        dev_err(hba->dev, "Failed to find OPP for exact frequency %lu\n", freq);
>>
>> I'm hitting this print on bootup:
>>
>> [    0.512515] ufshcd-qcom 1d84000.ufshc: Failed to find OPP for exact frequency 18446744073709551615
>> [    0.512571] ufshcd-qcom 1d84000.ufshc: Failed to find OPP for exact frequency 18446744073709551615
>>
>> Doesn't look like it's intended? The number is (2^64 - 1)
>>
> Yes, this is expected. During link startup, the frequency
> ULONG_MAX will be passed to ufs_qcom_set_core_clk_ctrl() and
> ufs_qcom_cfg_timer(). This frequency cannot be found through the API
> dev_pm_opp_find_freq_exact_indexed(). Therefore, we handle the
> frequency ULONG_MAX separately within Ufs_qcom_set_core_clk_ctrl()
> and ufs_qcom_cfg_timer().
> 
> This print only be print twice during link startup. If you think print
> such print during bootup is not make sense, I can improve the code and
> update a new vwesion.

I think just don't call ufs_qcom_opp_freq_to_clk_freq() if freq==ULONG_MAX

Neil

> 
> BRs.
> Ziqi
> 
>> Regards
>> Luca
>>
> 
> 

Re: [PATCH v2 2/3] scsi: ufs: qcom: Map devfreq OPP freq to UniPro Core Clock freq

[Ziqi Chen]

On 5/7/2025 7:59 PM, neil.armstrong@linaro.org wrote:
> On 07/05/2025 11:09, Ziqi Chen wrote:
>> Hi Luca,
>>
>> On 5/7/2025 4:19 PM, Luca Weiss wrote:
>>> Hi Ziqi,
>>>
>>> On Wed May 7, 2025 at 9:44 AM CEST, Ziqi Chen wrote:
>>>> From: Can Guo <quic_cang@quicinc.com>
>>>>
>>>> On some platforms, the devfreq OPP freq may be different than the 
>>>> unipro
>>>> core clock freq. Implement ufs_qcom_opp_freq_to_clk_freq() and use 
>>>> it to
>>>> find the unipro core clk freq.
>>>>
>>>> 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>
>>>> ---
>>>>   drivers/ufs/host/ufs-qcom.c | 81 +++++++++++++++++++++++++++++++ 
>>>> +-----
>>>>   1 file changed, 71 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>>>> index 7f10926100a5..804c8ccd8d03 100644
>>>> --- a/drivers/ufs/host/ufs-qcom.c
>>>> +++ b/drivers/ufs/host/ufs-qcom.c
>>>> +static unsigned long ufs_qcom_opp_freq_to_clk_freq(struct ufs_hba 
>>>> *hba,
>>>> +                                                   unsigned long 
>>>> freq, char *name)
>>>> +{
>>>> +    struct ufs_clk_info *clki;
>>>> +    struct dev_pm_opp *opp;
>>>> +    unsigned long clk_freq;
>>>> +    int idx = 0;
>>>> +    bool found = false;
>>>> +
>>>> +    opp = dev_pm_opp_find_freq_exact_indexed(hba->dev, freq, 0, true);
>>>> +    if (IS_ERR(opp)) {
>>>> +        dev_err(hba->dev, "Failed to find OPP for exact frequency 
>>>> %lu\n", freq);
>>>
>>> I'm hitting this print on bootup:
>>>
>>> [    0.512515] ufshcd-qcom 1d84000.ufshc: Failed to find OPP for 
>>> exact frequency 18446744073709551615
>>> [    0.512571] ufshcd-qcom 1d84000.ufshc: Failed to find OPP for 
>>> exact frequency 18446744073709551615
>>>
>>> Doesn't look like it's intended? The number is (2^64 - 1)
>>>
>> Yes, this is expected. During link startup, the frequency
>> ULONG_MAX will be passed to ufs_qcom_set_core_clk_ctrl() and
>> ufs_qcom_cfg_timer(). This frequency cannot be found through the API
>> dev_pm_opp_find_freq_exact_indexed(). Therefore, we handle the
>> frequency ULONG_MAX separately within Ufs_qcom_set_core_clk_ctrl()
>> and ufs_qcom_cfg_timer().
>>
>> This print only be print twice during link startup. If you think print
>> such print during bootup is not make sense, I can improve the code and
>> update a new vwesion.
> 
> I think just don't call ufs_qcom_opp_freq_to_clk_freq() if freq==ULONG_MAX
> 
Thanks Neil,  yes, it is a way , let me discuss internally and update 
new version.

BRs,
Ziqi

> Neil
> 
>>
>> BRs.
>> Ziqi
>>
>>> Regards
>>> Luca
>>>
>>
>>
> 

Re: [PATCH v2 2/3] scsi: ufs: qcom: Map devfreq OPP freq to UniPro Core Clock freq

[Bean Huo]

On Wed, 2025-05-07 at 15:44 +0800, Ziqi Chen wrote:
> -       return ufs_qcom_set_core_clk_ctrl(hba, freq);
> +       return ufs_qcom_set_core_clk_ctrl(hba, false, freq);
>  }
>  
>  static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, bool scale_up,
> @@ -2081,11 +2100,53 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba)
>         return ret;
>  }
>  
> +static unsigned long ufs_qcom_opp_freq_to_clk_freq(struct ufs_hba *hba,
> +                                                                                                  unsigned long freq, char *name)


This tab indentation is strange!


> +{
> +       struct ufs_clk_info *clki

Re: [PATCH v2 2/3] scsi: ufs: qcom: Map devfreq OPP freq to UniPro Core Clock freq

[Ziqi Chen]

On 5/8/2025 5:21 PM, Bean Huo wrote:
> On Wed, 2025-05-07 at 15:44 +0800, Ziqi Chen wrote:
>> -       return ufs_qcom_set_core_clk_ctrl(hba, freq);
>> +       return ufs_qcom_set_core_clk_ctrl(hba, false, freq);
>>   }
>>   
>>   static int ufs_qcom_clk_scale_notify(struct ufs_hba *hba, bool scale_up,
>> @@ -2081,11 +2100,53 @@ static int ufs_qcom_config_esi(struct ufs_hba *hba)
>>          return ret;
>>   }
>>   
>> +static unsigned long ufs_qcom_opp_freq_to_clk_freq(struct ufs_hba *hba,
>> +                                                                                                  unsigned long freq, char *name)
> 
> 
> This tab indentation is strange!
> 
Thanks Bean, it is my fault , I didn't change My IDE Tab indentation
size to 8 characters, it is still 4 . let me update.

BRs,
Ziqi
> 
>> +{
>> +       struct ufs_clk_info *clki
> 

[PATCH v2 3/3] scsi: ufs: qcom: Call ufs_qcom_cfg_timers() in clock scaling path

[Ziqi Chen]

From: Can Guo <quic_cang@quicinc.com>

ufs_qcom_cfg_timers() is clock freq dependent like ufs_qcom_set_core_clk_
ctrl(), hence move ufs_qcom_cfg_timers() call to clock scaling path. In
addition, do not assume the devfreq OPP freq is always the 'core_clock'
freq although 'core_clock' is the first clock phandle in device tree, use
ufs_qcom_opp_freq_to_clk_freq() to find the core clk freq.

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>
---
 drivers/ufs/host/ufs-qcom.c | 49 ++++++++++++++++++++++---------------
 1 file changed, 29 insertions(+), 20 deletions(-)

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 804c8ccd8d03..e8fabf4a6d8a 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -599,13 +599,14 @@ static int ufs_qcom_hce_enable_notify(struct ufs_hba *hba,
  *
  * @hba: host controller instance
  * @is_pre_scale_up: flag to check if pre scale up condition.
+ * @freq: target opp freq
  * Return: zero for success and non-zero in case of a failure.
  */
-static int ufs_qcom_cfg_timers(struct ufs_hba *hba, bool is_pre_scale_up)
+static int ufs_qcom_cfg_timers(struct ufs_hba *hba, bool is_pre_scale_up, unsigned long freq)
 {
 	struct ufs_qcom_host *host = ufshcd_get_variant(hba);
 	struct ufs_clk_info *clki;
-	unsigned long core_clk_rate = 0;
+	unsigned long clk_freq = 0;
 	u32 core_clk_cycles_per_us;
 
 	/*
@@ -617,22 +618,34 @@ static int ufs_qcom_cfg_timers(struct ufs_hba *hba, bool is_pre_scale_up)
 	if (host->hw_ver.major < 4 && !ufshcd_is_intr_aggr_allowed(hba))
 		return 0;
 
+	if (hba->use_pm_opp) {
+		clk_freq = ufs_qcom_opp_freq_to_clk_freq(hba, freq, "core_clk");
+		if (clk_freq)
+			goto cfg_timers;
+	}
+
 	list_for_each_entry(clki, &hba->clk_list_head, list) {
 		if (!strcmp(clki->name, "core_clk")) {
+			if (freq == ULONG_MAX) {
+				clk_freq = clki->max_freq;
+				break;
+			}
+
 			if (is_pre_scale_up)
-				core_clk_rate = clki->max_freq;
+				clk_freq = clki->max_freq;
 			else
-				core_clk_rate = clk_get_rate(clki->clk);
+				clk_freq = clk_get_rate(clki->clk);
 			break;
 		}
 
 	}
 
+cfg_timers:
 	/* If frequency is smaller than 1MHz, set to 1MHz */
-	if (core_clk_rate < DEFAULT_CLK_RATE_HZ)
-		core_clk_rate = DEFAULT_CLK_RATE_HZ;
+	if (clk_freq < DEFAULT_CLK_RATE_HZ)
+		clk_freq = DEFAULT_CLK_RATE_HZ;
 
-	core_clk_cycles_per_us = core_clk_rate / USEC_PER_SEC;
+	core_clk_cycles_per_us = clk_freq / USEC_PER_SEC;
 	if (ufshcd_readl(hba, REG_UFS_SYS1CLK_1US) != core_clk_cycles_per_us) {
 		ufshcd_writel(hba, core_clk_cycles_per_us, REG_UFS_SYS1CLK_1US);
 		/*
@@ -652,7 +665,7 @@ static int ufs_qcom_link_startup_notify(struct ufs_hba *hba,
 
 	switch (status) {
 	case PRE_CHANGE:
-		if (ufs_qcom_cfg_timers(hba, false)) {
+		if (ufs_qcom_cfg_timers(hba, false, ULONG_MAX)) {
 			dev_err(hba->dev, "%s: ufs_qcom_cfg_timers() failed\n",
 				__func__);
 			return -EINVAL;
@@ -930,17 +943,6 @@ static int ufs_qcom_pwr_change_notify(struct ufs_hba *hba,
 
 		break;
 	case POST_CHANGE:
-		if (ufs_qcom_cfg_timers(hba, false)) {
-			dev_err(hba->dev, "%s: ufs_qcom_cfg_timers() failed\n",
-				__func__);
-			/*
-			 * we return error code at the end of the routine,
-			 * but continue to configure UFS_PHY_TX_LANE_ENABLE
-			 * and bus voting as usual
-			 */
-			ret = -EINVAL;
-		}
-
 		/* cache the power mode parameters to use internally */
 		memcpy(&host->dev_req_params,
 				dev_req_params, sizeof(*dev_req_params));
@@ -1492,7 +1494,7 @@ static int ufs_qcom_clk_scale_up_pre_change(struct ufs_hba *hba, unsigned long f
 {
 	int ret;
 
-	ret = ufs_qcom_cfg_timers(hba, true);
+	ret = ufs_qcom_cfg_timers(hba, true, freq);
 	if (ret) {
 		dev_err(hba->dev, "%s ufs cfg timer failed\n", __func__);
 		return ret;
@@ -1529,6 +1531,13 @@ static int ufs_qcom_clk_scale_down_pre_change(struct ufs_hba *hba)
 
 static int ufs_qcom_clk_scale_down_post_change(struct ufs_hba *hba, unsigned long freq)
 {
+	int ret;
+
+	ret = ufs_qcom_cfg_timers(hba, false, freq);
+	if (ret) {
+		dev_err(hba->dev, "%s: ufs_qcom_cfg_timers() failed\n",	__func__);
+		return ret;
+	}
 	/* set unipro core clock attributes and clear clock divider */
 	return ufs_qcom_set_core_clk_ctrl(hba, false, freq);
 }
-- 
2.34.1