Re: [PATCH V1 1/2] scsi: ufs: ufs-qcom: Add specified gear support for multi gear scaling

[Manivannan Sadhasivam]

On Wed, Sep 09, 2026 at 10:34:58AM +0530, Nitin Rawat wrote:
> 
> 
> On 9/2/2026 9:32 PM, Manivannan Sadhasivam wrote:
> > On Sat, Aug 29, 2026 at 01:13:54PM +0530, Nitin Rawat wrote:
> > > From: Ziqi Chen <ziqi.chen@xxxxxxxxxxxxxxxx>
> > > 
> > > The UFS clock frequency and gear speed do not necessarily have a strict
> > > one-to-one correspondence on all platforms. Introduce a device tree
> > > based configuration interface that allows specifying the HS gear
> > > speed for each supported operating frequency via the "opp-level"
> > > property in the OPP table. When this property is not configured, the
> > > driver falls back to the default frequency-to-gear mapping table.
> > > 
> > > Signed-off-by: Ziqi Chen <ziqi.chen@xxxxxxxxxxxxxxxx>
> > > Signed-off-by: Nitin Rawat <nitin.rawat@xxxxxxxxxxxxxxxx>
> > > ---
> > >   drivers/ufs/host/ufs-qcom.c | 30 ++++++++++++++++++++++++++----
> > >   1 file changed, 26 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> > > index 0f2e083b04fd..aa2ac2cd2b69 100644
> > > --- a/drivers/ufs/host/ufs-qcom.c
> > > +++ b/drivers/ufs/host/ufs-qcom.c
> > > @@ -2460,8 +2460,9 @@ static unsigned long ufs_qcom_opp_freq_to_clk_freq(struct ufs_hba *hba,
> > >   	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);
> > > +	if (IS_ERR_OR_NULL(opp)) {
> > > +		dev_err(hba->dev, "%s: Failed to find OPP for exact frequency %lu\n",
> > > +			__func__, freq);
> > 
> > Don't bring back the '__func__' marking please...
> Sure, will take in next patchset
> 
> 
> > 
> > >   		return 0;
> > >   	}
> > > 
> > > @@ -2489,12 +2490,32 @@ static unsigned long ufs_qcom_opp_freq_to_clk_freq(struct ufs_hba *hba,
> > > 
> > >   static u32 ufs_qcom_freq_to_gear_speed(struct ufs_hba *hba, unsigned long freq)
> > >   {
> > > -	u32 gear = UFS_HS_DONT_CHANGE;
> > > +	struct dev_pm_opp *opp;
> > >   	unsigned long unipro_freq;
> > > +	u32 gear = UFS_HS_DONT_CHANGE;
> > 
> > Nit: Preserve reverse Xmas order.
> Sure, will take in next patchset
> 
> > 
> > > 
> > >   	if (!hba->use_pm_opp)
> > >   		return gear;
> > > 
> > > +	opp = dev_pm_opp_find_freq_exact_indexed(hba->dev, freq, 0, true);
> > > +	if (IS_ERR_OR_NULL(opp)) {
> > > +		dev_err(hba->dev, "%s: Failed to find OPP for exact frequency %lu\n",
> > > +			__func__, freq);
> > 
> > Drop '__func__' here and below.
> Sure, will take in next patchset
> 
> > 
> > > +		return gear;
> > > +	}
> > > +
> > > +	/* Get HS gear speed from 'opp-level' */
> > > +	gear = dev_pm_opp_get_level(opp);
> > > +	dev_pm_opp_put(opp);
> > > +
> > > +	/*
> > > +	 * Greater than max gear means that there is no specified gear configured in DT
> > > +	 * or the specified gear is invalid.
> > > +	 */
> > > +	if (gear <= hba->max_pwr_info.info.gear_rx)
> > > +		return gear;
> > 
> > Sashiko pointed out a valid concern with this check, please take a look.
> 
> I reviewed the bot's comment. The concern raised applies to the case where
> the gear value provided through the device tree is higher (for example, 5)
> than what a UFS 3.x device can support. After link startup and negotiation,
> hba->max_pwr_info.info.gear_rx would be 4. In this scenario, the condition
> below evaluates to false:
> 
> 
> > +	if (gear <= hba->max_pwr_info.info.gear_rx)
> > +		return gear;
> 
> Execution then falls back to the switch-case logic. If the current frequency
> does not match any of the predefined entries, the function returns
> UFS_HS_DONT_CHANGE, which effectively maps to the minimum gear. Shahiko's
> suggestion is to instead return the device's maximum negotiated gear using:
> 
> A couple of points to note:
> 
> 1. If the current frequency does not match any of the expected frequency
> entries, that is already an existing issue. In such a case, simply returning
> the maximum negotiated gear may not be correct because we do not know the
> actual gear corresponding to the currently programmed frequency.
> 
> 2. The patch under review does not change this existing behavior. It only
> addresses the handling of gear values that are within the negotiated device
> capabilities and does not alter the fallback path when the frequency lookup
> fails.
> 
> 
> Considering the above points, I believe no changes are required for this
> patch at this time.
> We can revisit this behavior separately and evaluate potential optimizations
> in a future patch if needed.
> 

Fair enough.

- Mani

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