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
--
மணிவண்ணன் சதாசிவம்
