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

[Manivannan Sadhasivam]

On Wed, Dec 06, 2023 at 04:35:24PM +0530, Nitin Rawat wrote:
> 
> 
> 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.

Ah, right. Please increment it in dev_pm_opp_find_freq_ceil_indexed() instead.

- Mani

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