Message-ID: <20220712151045.vn4tpat4c4cplndo@vireshk-i7>
Date:   Tue, 12 Jul 2022 20:40:45 +0530
From:   Viresh Kumar <viresh.kumar@...aro.org>
To:     Johan Hovold <johan@...nel.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 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.

> 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!
> [  +0.000004] debugfs: Directory 'opp:0' with parent 'cx' already present!
> [  +0.000004] debugfs: Directory 'opp:0' with parent 'cx' already present!
> [  +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present!
> [  +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present!
> [  +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present!
> [  +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present!
> [  +0.000003] debugfs: Directory 'opp:0' with parent 'cx' already present!
> [  +0.000003] 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);

I have merged this into:

commit 341df9889277 ("OPP: Allow multiple clocks for a device")

and pushed out for linux-next.


Bjorn, Mani,

It would be really good if we can find a way to make following work on
Qcom:

        clk_get(cpu_dev, NULL or "xo")

If that happens, we can handle the special case just at the consumer
driver (qcom-cpufreq-hw) and not in the core.

-- 
viresh

Powered by blists - more mailing lists