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@xxxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxxx>
> > > > Co-developed-by: Ziqi Chen <quic_ziqichen@xxxxxxxxxxx>
> > > > Signed-off-by: Ziqi Chen <quic_ziqichen@xxxxxxxxxxx>
> > > > ---
> > > >    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