On Tue, Oct 29, 2019 at 01:34:24PM +0800, Sudeep Holla wrote:
> On Mon, Oct 28, 2019 at 10:45:22AM +0100, Ulf Hansson wrote:
> > + Niklas
> >
> > On Mon, 28 Oct 2019 at 08:49, Sudeep Holla <sudeep.holla@xxxxxxx> wrote:
> > >
> > > On Mon, Oct 28, 2019 at 08:35:55AM +0100, Ulf Hansson wrote:
> > > > On Sun, 27 Oct 2019 at 03:30, Sudeep Holla <sudeep.holla@xxxxxxx> wrote:
> > > > >
> > > > > On Thu, Oct 24, 2019 at 06:47:43PM +0200, Ulf Hansson wrote:
> > > > > > On Thu, 24 Oct 2019 at 18:31, Sudeep Holla <sudeep.holla@xxxxxxx> wrote:
> > > > > > >
> > > > > > > On Thu, Oct 10, 2019 at 01:39:34PM +0200, Ulf Hansson wrote:
> > > > > > > > Introduce a PSCI DT helper function, psci_dt_attach_cpu(), which takes a
> > > > > > > > CPU number as an in-parameter and tries to attach the CPU's struct device
> > > > > > > > to its corresponding PM domain.
> > > > > > > >
> > > > > > > > Let's makes use of dev_pm_domain_attach_by_name(), as it allows us to
> > > > > > > > specify "psci" as the "name" of the PM domain to attach to. Additionally,
> > > > > > > > let's also prepare the attached device to be power managed via runtime PM.
> > > > > > > >
> > > > > > > > Signed-off-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> > > > > > > > ---
> > > > > > > > drivers/cpuidle/cpuidle-psci-domain.c | 21 +++++++++++++++++++++
> > > > > > > > drivers/cpuidle/cpuidle-psci.h | 6 ++++++
> > > > > > > > 2 files changed, 27 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
> > > > > > > > index 3f5143ccc3e0..7429fd7626a1 100644
> > > > > > > > --- a/drivers/cpuidle/cpuidle-psci-domain.c
> > > > > > > > +++ b/drivers/cpuidle/cpuidle-psci-domain.c
> > > > > > > > @@ -9,9 +9,11 @@
> > > > > > > >
> > > > > > > > #define pr_fmt(fmt) "CPUidle PSCI: " fmt
> > > > > > > >
> > > > > > > > +#include <linux/cpu.h>
> > > > > > > > #include <linux/device.h>
> > > > > > > > #include <linux/kernel.h>
> > > > > > > > #include <linux/pm_domain.h>
> > > > > > > > +#include <linux/pm_runtime.h>
> > > > > > > > #include <linux/psci.h>
> > > > > > > > #include <linux/slab.h>
> > > > > > > > #include <linux/string.h>
> > > > > > > > @@ -279,3 +281,22 @@ static int __init psci_idle_init_domains(void)
> > > > > > > > return ret;
> > > > > > > > }
> > > > > > > > subsys_initcall(psci_idle_init_domains);
> > > > > > > > +
> > > > > > > > +struct device *psci_dt_attach_cpu(int cpu)
> > > > > > > > +{
> > > > > > > > + struct device *dev;
> > > > > > > > +
> > > > > > > > + /* Currently limit the hierarchical topology to be used in OSI mode. */
> > > > > > > > + if (!psci_has_osi_support())
> > > > > > > > + return NULL;
> > > > > > > > +
> > > > > > > > + dev = dev_pm_domain_attach_by_name(get_cpu_device(cpu), "psci");
> > > > > > >
> > > > > > > This clarifies the need for the fixed name. But why not just go by index 0
> > > > > > > as the consumer of these psci power-domains will have only one power domain
> > > > > > > entry. Why do we need this name compulsory ?
> > > > > >
> > > > > > The idea is to be future proof. If I recall correctly, the CPU node on
> > > > > > some QCOM SoCs may also have "CPR" PM domain specified, thus
> > > > > > "multiple" power-domains could be specified.
> > > > > >
> > > > >
> > > > > I am sure we don't want to mx-n-match any power domain provider with
> > > > > psci. And also I expect in these above mentioned cases, there won't be any
> > > > > psci power domains.
> > > > >
> > > > > > In any case, using "psci" doesn't really hurt, right?
> > > > > >
> > > > >
> > > > > Doesn't but I don't see need for one as only one should exist, as mentioned
> > > > > above we don't want mix-n-match with psci ever.
> > > >
> > > > Not sure I get your point, sorry.
> > > >
> > > > The CPU could very well be attached to more than one power-domain. Of
> > > > course not multiple "PSCI power-domains". One could be an PSCI power
> > > > domain and another one could be the QCOM CPR (Core power reduction)
> > > > power domain.
> > > >
> > >
> > > And who controls QCOM CPR ? If it's OSPM, this model is broken.
> > > I mean OSPM can vote, but the control *has* to be in PSCI firmware to
> > > change any CPU power state.
> > >
> > > If it's firmware controlled, then there's no need to explicitly specify
> > > both to OSPM.
> >
> > This is about OPP and CPUFreq, so it has nothing to do with PSCI.
> >
> > >
> > > > Have a look at these binding, there are already upstream, perhaps that
> > > > clarifies this?
> > > > Documentation/devicetree/bindings/opp/qcom-nvmem-cpufreq.txt
> > > >
> > >
> > > OK, I will have a look.
> >
> > Great.
> >
> > I have looped in Niklas Casell, he should be able to answer any more
> > detailed questions in regards to QCOM CPR, if that is needed.
> >
>
> So had a look at the DT bindings and standalone it looks fine.
> But when it's mixed like the way you describe: yikes!
>
> Why does a power(oh wait it's actually performance domain!) is combined
> with a device whose actual power is controlled by only by PSCI/firmware
> is associated along with another power(again actally performance)
> domain.
>
> This whole representation of performance domain as power domain in the
> bindings is a *mess*. If Linux kernel chose to implement it as part
> of genpd, that's fine. But we should have had a separate binding for
> that.
>
> > In any case, we are discussing whether we should require a
> > power-domain-names set to "psci" for the CPU node - and I don't see
> > how that could hurt. Right?
> >
>
> Honestly I don't like this, but we don't have any choice I think.
> So yes, but you need to update the binding. Hope new platform move
> all these performance domain control part into firmware and have single
> control from kernel unlike the present generation which OPP through
> clock or cpufreq and the voltage/performance comtrol via genpd.
FWIW, in newer generation Qualcomm SoCs like SDM845,
the voltage/performance control is done in firmware,
by the OSM (drivers/cpufreq/qcom-cpufreq-hw.c).
Kind regards,
Niklas
