Message-ID: <Ys2ZVeBHyhrUBC4m@hovoldconsulting.com>
Date:   Tue, 12 Jul 2022 17:55:01 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Viresh Kumar <viresh.kumar@...aro.org>
Cc:     Bjorn Andersson <bjorn.andersson@...aro.org>,
        Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
        Chanwoo Choi <cw00.choi@...sung.com>,
        Dmitry Osipenko <digetx@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Kyungmin Park <kyungmin.park@...sung.com>,
        MyungJoo Ham <myungjoo.ham@...sung.com>,
        Nishanth Menon <nm@...com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Stephen Boyd <sboyd@...nel.org>,
        Thierry Reding <thierry.reding@...il.com>,
        Viresh Kumar <vireshk@...nel.org>, linux-pm@...r.kernel.org,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        devicetree@...r.kernel.org,
        Dmitry Osipenko <dmitry.osipenko@...labora.com>,
        linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org,
        Rob Herring <robh@...nel.org>
Subject: Re: [PATCH V2 00/13] OPP: Add support for multiple clocks*

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

Powered by blists - more mailing lists