On Tue, Jul 12, 2022 at 08:40:45PM +0530, Viresh Kumar wrote:
> On 12-07-22, 16:29, Johan Hovold wrote:
> > On Tue, Jul 12, 2022 at 01:22:40PM +0530, Viresh Kumar wrote:
> > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > > index 666e1ebf91d1..4f4a285886fa 100644
> > > --- a/drivers/opp/core.c
> > > +++ b/drivers/opp/core.c
> > > @@ -1384,6 +1384,20 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,
> > > }
> > >
> > > if (ret == -ENOENT) {
> > > + /*
> > > + * There are few platforms which don't want the OPP core to
> > > + * manage device's clock settings. In such cases neither the
> > > + * platform provides the clks explicitly to us, nor the DT
> > > + * contains a valid clk entry. The OPP nodes in DT may still
> > > + * contain "opp-hz" property though, which we need to parse and
> > > + * allow the platform to find an OPP based on freq later on.
> > > + *
> > > + * This is a simple solution to take care of such corner cases,
> > > + * i.e. make the clk_count 1, which lets us allocate space for
> > > + * frequency in opp->rates and also parse the entries in DT.
> > > + */
> > > + opp_table->clk_count = 1;
> > > +
> > > dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__, ret);
> > > return opp_table;
> > > }
> >
> > This looks like a hack.
>
> Yeah, a bit. Initially I wanted to solve it in a cleaner way, like it
> is done for Tegra, where you will pass the right clock name to the OPP
> core, so it can verify that the clk is there and parse the table. And
> then tell the OPP core not to configure the clk from
> dev_pm_opp_set_opp(), which is possible now. This would have done the
> things in the right way.
>
> The problem with Qcom's DT is that the CPU node have the OPP table but
> doesn't contain the clocks, which are available with the
> qcom,cpufreq-hw node instead. Because of this, I couldn't pass the
> real clocks name to the OPP core, "xo", for the CPU device.
>
> I really tried to avoid adding the above code for Tegra and found a
> better and cleaner way out. But I couldn't do the same here and
> figured it may be more generic of a problem, which is fine as well.
>
> The OPP core does two things currently:
>
> 1) Parse the DT and provide helpers to find the right OPP, etc.
>
> 2) Provide generic helper to configure all resources related to the
> OPP.
>
> It is fine if some platforms only want to have the first and not the
> second. To have the second though, you need to have the first as well.
>
> The clk is required only for the second case, and the OPP core should
> parse the DT anyways, irrespective of the availability of the clock.
> Because of this reason, making the above change looked reasonable
> (this is what was happening before my new patches came in anyway). The
> clock isn't there, but there is "opp-hz" present in the DT, which
> needs to be parsed.
Ok, thanks for the details. I'd still look into if there's some way to
avoid setting clk_count when there are no clocks as it sounds like an
anti-pattern that will just make the code harder to understand and
maintain.
> > And it also triggers a bunch of new warning when
> > opp is trying to create debugfs entries for an entirely different table
> > which now gets clk_count set to 1:
> >
> > [ +0.000979] cx: _update_opp_table_clk: Couldn't find clock: -2
> > [ +0.000022] debugfs: Directory 'opp:0' with parent 'cx' already present!
> > This is for the rpmhpd whose opp table does not have either opp-hz or
> > clocks (just opp-level).
>
> Ahh, I missed switching back to the earlier code here. i.e. not use
> the frequency for OPP directory's name, when it is 0.
>
> This will fix it.
>
> diff --git a/drivers/opp/debugfs.c b/drivers/opp/debugfs.c
> index 402c507edac7..96a30a032c5f 100644
> --- a/drivers/opp/debugfs.c
> +++ b/drivers/opp/debugfs.c
> @@ -138,7 +138,7 @@ void opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
> * - For some devices rate isn't available or there are multiple, use
> * index instead for them.
> */
> - if (likely(opp_table->clk_count == 1))
> + if (likely(opp_table->clk_count == 1 && opp->rates[0]))
> id = opp->rates[0];
> else
> id = _get_opp_count(opp_table);
Indeed it does, thanks.
> I have merged this into:
>
> commit 341df9889277 ("OPP: Allow multiple clocks for a device")
>
> and pushed out for linux-next.
Thanks for addressing this quickly. With the two patches above applied,
the issues I noticed are gone.
Johan
