Re: [RFC PATCH 4/9] opp: core: Don't warn if required OPP device does not exist

[Ulf Hansson]

On Thu, 14 Oct 2021 at 09:23, Hector Martin <marcan@xxxxxxxxx> wrote:
>
> On 14/10/2021 16.03, Hector Martin wrote:
> > On 14/10/2021 15.56, Viresh Kumar wrote:
> >>> +   /*
> >>> +    * Attach the CPU device to its genpd domain (if any), to allow OPP
> >>> +    * dependencies to be satisfied.
> >>> +    */
> >>> +   ret = genpd_dev_pm_attach(cpu_dev);
> >>> +   if (ret <= 0) {
> >>> +           dev_err(cpu_dev, "Failed to attach CPU device to genpd\n");
> >>> +           goto out;
> >>> +   }
> >>> +
> >>
> >> Other platform do this from some other place I think.
> >>
> >> Ulf, where should this code be moved ? cpu-clk driver ?
> >>
> >
> > I see one driver that does this is drivers/clk/qcom/apcs-sdx55.c (via
> > dev_pm_domain_attach). Though it only does it for CPU#0; we need to do
> > it for all CPUs.
>
> Looking into this further, I'm not sure I like the idea of doing this in
> the clocks driver. There might be locking issues since it gets
> instantiated twice and yet doesn't really itself know what subset of
> CPUs it applies to.

I agree. I suggest you look into using a genpd provider and hook up
all CPU's devices to it. I think that is what Viresh also suggested
earlier - and this makes most sense to me.

As a reference you may have a look at some Qcom platforms that already use this:

arch/arm64/boot/dts/qcom/qcs404.dtsi

drivers/cpufreq/qcom-cpufreq-nvmem.c:
To hook up CPU devices to their PM domains (genpds) - it calls
dev_pm_opp_attach_genpd(), which is a kind of wrapper for
dev_pm_domain_attach_by_name().

drivers/soc/qcom/cpr.c
Registers the genpd provider that is capable of dealing with
performance states/OPPs for CPUs.

>
> There's another driver that does this:
> drivers/cpuidle/cpuidle-psci-domain.c. That one specifically looks for a
> power domain called "psci". Perhaps it would make sense to make this
> generic in cpufreq-dt as per my prior patch, but explicitly request a
> "cpufreq" domain? That way only devicetrees that opt in to having this
> handled by cpufreq by naming it that way would get this behavior.

That sounds like an idea that is worth exploring. In this way, the
only thing that needs to be implemented for new cases would be the
genpd provider driver.

BTW, as you will figure out by looking at the above references, for
the qcom case we are using "cpr" as the domain name for cpufreq. Of
course, that doesn't mean we can use "cpufreq" (or whatever name that
makes sense) going forward for new cases.

>
> --
> Hector Martin (marcan@xxxxxxxxx)
> Public Key: https://mrcn.st/pub

Kind regards
Uffe