Re: [PATCH V1] scsi: ufs: core: store min and max clk freq from OPP table

[Nitin Rawat]

On 12/6/2023 1:24 PM, Manivannan Sadhasivam wrote:
> On Wed, Dec 06, 2023 at 11:06:28AM +0530, Nitin Rawat wrote:
> > OPP support will make use of OPP table in device tree and removes
> > freq-table-hz property from device tree.
> >
> > With OPP enabled in devicetree, clki->min_freq and clki->maxfreq
> > currently is not getting updated and the value is set to 0.
> >
> > Soc vendors like qcom, mediatek uses clki->minfreq and clki->maxfreq
> > in vendor specific file. These frequencies values are used to update
> > vendor specific configurations. Since the value is 0, it is causing
> > functional issue.
>
> How about,
>
> "OPP support added by commit 72208ebe181e ("scsi: ufs: core: Add support
> for parsing OPP") doesn't update the min_freq and max_freq of each clocks
> in 'struct ufs_clk_info'.
>
> But these values are used by the vendor host drivers internally for controller
> configuration. When the OPP support is enabled in devicetree, these values will
> be 0, causing boot issues on the respective platforms.
>
> So let's parse the min_freq and max_freq of all clocks while parsing the OPP
> table."
>
> > Add code to store the min and max ufs clk frequency from OPP table.

Sure. Will update in next patchset.

> > Fixes: 72208ebe181e ("scsi: ufs: core: Add support for parsing OPP")
> > Co-developed-by: Manish Pandey <quic_mapa@xxxxxxxxxxx>
> > Signed-off-by: Manish Pandey <quic_mapa@xxxxxxxxxxx>
> > Signed-off-by: Nitin Rawat <quic_nitirawa@xxxxxxxxxxx>
> > ---
> >   drivers/ufs/host/ufshcd-pltfrm.c | 56 ++++++++++++++++++++++++++++++++
> >   1 file changed, 56 insertions(+)
> >
> > diff --git a/drivers/ufs/host/ufshcd-pltfrm.c b/drivers/ufs/host/ufshcd-pltfrm.c
> > index da2558e274b4..12fa6f7d6a97 100644
> > --- a/drivers/ufs/host/ufshcd-pltfrm.c
> > +++ b/drivers/ufs/host/ufshcd-pltfrm.c
> > @@ -13,6 +13,7 @@
> >   #include <linux/pm_opp.h>
> >   #include <linux/pm_runtime.h>
> >   #include <linux/of.h>
> > +#include <linux/clk.h>
>
> Sort includes alphabetically.

Sure. Will update in next patchset

> >   #include <ufs/ufshcd.h>
> >   #include "ufshcd-pltfrm.h"
> > @@ -213,6 +214,55 @@ static void ufshcd_init_lanes_per_dir(struct ufs_hba *hba)
> >   	}
> >   }
> >
> > +/**
> > + * ufshcd_config_min_max_clk_freq - update min and max freq
>
> "ufshcd_parse_clock_min_max_freq - Parse MIN and MAX frequencies of clocks"
>
> > + * @hba: per adapter instance
> > + *
> > + * This function store min and max freq for all the clocks.
> > + *
>
> "This function parses MIN and MAX frequencies of all clocks required by the
> vendor host drivers."
>
> > + * Returns 0 for success and non-zero for failure
> > + */
> > +static int ufshcd_config_min_max_clk_freq(struct ufs_hba *hba)
> > +{
> > +	struct list_head *head = &hba->clk_list_head;
> > +	struct dev_pm_opp *opp;
> > +	struct ufs_clk_info *clki;
>
> Please maintain reverse Xmas tree order. It's not a rule for this driver, but my
> own preference.
>
> > +	unsigned long freq;
> > +	u8 idx = 0;
> > +	int ret;
>
> This won't be needed if all the return values are directly returned as I shared
> below.


Will Addressed all ret comments in next patchset.
> > +
> > +	list_for_each_entry(clki, head, list) {
> > +		if (!clki->name)
> > +			continue;
> > +
> > +		clki->clk = devm_clk_get(hba->dev, clki->name);
> > +		if (!IS_ERR_OR_NULL(clki->clk)) {
>
> This function won't return NULL, so IS_ERR() is sufficient.
>
> > +			/* Find Max Freq */
> > +			freq = ULONG_MAX;
> > +			opp = dev_pm_opp_find_freq_floor_indexed(hba->dev, &freq, idx);
>
> Use idx++ and get rid of the increment at the end of the 'if' condition.

If we increment idx++ here, dev_pm_opp_find_freq_ceil_indexed will use 
incremented idx which is not correct. Hence i added at end after both 
the call.

> > +			if (IS_ERR(opp)) {
> > +				dev_err(hba->dev, "failed to find dev_pm_opp\n");
>
> "Failed to find OPP for MAX frequency"
>
> > +				ret = PTR_ERR(opp);
>
> return PTR_ERR(opp);
>
> > +				return ret;
> > +			}
> > +			clki->max_freq = dev_pm_opp_get_freq_indexed(opp, idx);
> > +
>
> Missing dev_pm_opp_put()

Thanks. Will update in next patchset.

> > +			/* Find Min Freq */
> > +			freq = 0;
> > +			opp = dev_pm_opp_find_freq_ceil_indexed(hba->dev, &freq, idx);
> > +			if (IS_ERR(opp)) {
> > +				dev_err(hba->dev, "failed to find dev_pm_opp\n");
>
> "Failed to find OPP for MIN frequency"
>
> > +				ret = PTR_ERR(opp);
>
> return PTR_ERR(opp);
>
> > +				return ret;
> > +			}
> > +			clki->min_freq = dev_pm_opp_get_freq_indexed(opp, idx);
>
> Missing dev_pm_opp_put()
>
> > +			idx++;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >   static int ufshcd_parse_operating_points(struct ufs_hba *hba)
> >   {
> >   	struct device *dev = hba->dev;
> > @@ -279,6 +329,12 @@ static int ufshcd_parse_operating_points(struct ufs_hba *hba)
> >   		return ret;
> >   	}
> >
> > +	ret = ufshcd_config_min_max_clk_freq(hba);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to get min max freq: %d\n", ret);
>
> Since we already print error message inside the function, no need to do the same
> here.

Sure. Will update in next patchset

> - Mani
>
> > +		return ret;
> > +	}
> > +
> >   	hba->use_pm_opp = true;
> >
> >   	return 0;
> > --
> > 2.17.1

Thanks,
Nitin