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@xxxxxxxxxxx>
> > > > >
> > > > > 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@xxxxxxxxxxx>
> > > > > Signed-off-by: Ziqi Chen <quic_ziqichen@xxxxxxxxxxx>
> > > > > Signed-off-by: Can Guo <quic_cang@xxxxxxxxxxx>
> > > > > ---
> > > > > 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
--
மணிவண்ணன் சதாசிவம்
