Re: [PATCH v2 08/13] cpuidle: psci: Add a helper to attach a CPU to its PM domain

[Niklas Cassel <niklas.cassel@linaro.org>]

On Thu, Nov 07, 2019 at 11:22:24AM +0100, Ulf Hansson wrote:
> On Thu, 7 Nov 2019 at 10:13, Niklas Cassel <niklas.cassel@linaro.org> wrote:
> >
> > On Tue, Oct 29, 2019 at 05:44:33PM +0100, 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.
> > >
> > > Note that, the implementation of the new helper function is in a new
> > > separate c-file, which may seems a bit too much at this point. However,
> > > subsequent changes that implements the remaining part of the PM domain
> > > support for cpuidle-psci, helps to justify this split.
> > >
> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > ---
> > >
> > > Changes in v2:
> > >       - Reorder patch to be the first one that starts adding the PM domain
> > >         support.
> > >       - Rebased.
> > >
> > > ---
> > >  drivers/cpuidle/Makefile              |  4 ++-
> > >  drivers/cpuidle/cpuidle-psci-domain.c | 36 +++++++++++++++++++++++++++
> > >  drivers/cpuidle/cpuidle-psci.h        | 12 +++++++++
> > >  3 files changed, 51 insertions(+), 1 deletion(-)
> > >  create mode 100644 drivers/cpuidle/cpuidle-psci-domain.c
> > >  create mode 100644 drivers/cpuidle/cpuidle-psci.h
> > >
> > > diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
> > > index ee70d5cc5b99..cc8c769d7fa9 100644
> > > --- a/drivers/cpuidle/Makefile
> > > +++ b/drivers/cpuidle/Makefile
> > > @@ -21,7 +21,9 @@ obj-$(CONFIG_ARM_U8500_CPUIDLE)         += cpuidle-ux500.o
> > >  obj-$(CONFIG_ARM_AT91_CPUIDLE)          += cpuidle-at91.o
> > >  obj-$(CONFIG_ARM_EXYNOS_CPUIDLE)        += cpuidle-exynos.o
> > >  obj-$(CONFIG_ARM_CPUIDLE)            += cpuidle-arm.o
> > > -obj-$(CONFIG_ARM_PSCI_CPUIDLE)               += cpuidle-psci.o
> > > +obj-$(CONFIG_ARM_PSCI_CPUIDLE)               += cpuidle_psci.o
> > > +cpuidle_psci-y                               := cpuidle-psci.o
> > > +cpuidle_psci-$(CONFIG_PM_GENERIC_DOMAINS_OF) += cpuidle-psci-domain.o
> > >
> > >  ###############################################################################
> > >  # MIPS drivers
> > > diff --git a/drivers/cpuidle/cpuidle-psci-domain.c b/drivers/cpuidle/cpuidle-psci-domain.c
> > > new file mode 100644
> > > index 000000000000..bc7df4dc0686
> > > --- /dev/null
> > > +++ b/drivers/cpuidle/cpuidle-psci-domain.c
> > > @@ -0,0 +1,36 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * PM domains for CPUs via genpd - managed by cpuidle-psci.
> > > + *
> > > + * Copyright (C) 2019 Linaro Ltd.
> > > + * Author: Ulf Hansson <ulf.hansson@linaro.org>
> > > + *
> > > + */
> > > +
> > > +#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 "cpuidle-psci.h"
> > > +
> > > +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");
> >
> > Hello Ulf,
> >
> > here you use dev_pm_domain_attach_by_name(), which will call
> > genpd_dev_pm_attach_by_name(), which will call genpd_dev_pm_attach_by_id(),
> > which will call __genpd_dev_pm_attach(virt_dev, dev, index, false);
> > the last argument is power_on, which here is always set to false.
> >
> > In older versions of your patch series, psci_dt_attach_cpu() called
> > dev_pm_domain_attach(dev, true), where the last argument is power_on.
> > Interestingly enough (for the non-ACPI case), dev_pm_domain_attach()
> > ignores the power_on parameter, and simply calls genpd_dev_pm_attach(dev);
> > which will call __genpd_dev_pm_attach(dev, dev, 0, true);
> > the last argument is power_on, which here is always set to true.
> >
> > In other words, your previous patch series always powered on the power
> > domain, while the newer versions do not. Is this change intentional?
> 
> Wow, thanks for an in-depth review!
> 
> Yes, the change is intentional.
> 
> If the device is attached via dev_pm_domain_attach(), genpd needs to
> power on the PM domain, due to legacy reasons (from behaviours by
> drivers/subsystem).
> 
> This isn't the case when the device is attached through
> dev_pm_domain_attach_by_*(), as there is no legacy to care about.
> 
> > Perhaps psci_dt_attach_cpu() should call dev_to_genpd(dev)->power_on()
> > after attaching the power domain? (In order to be consistent with the
> > previous behavior of this patch series.)
> 
> After calling dev_pm_domain_attach_by_name, I am calling
> pm_runtime_get_sync() if the cpu is online, which means the
> corresponding genpd will be powered on - but then, only when actually
> needed.
> 
> The old behaviour actually relied on the late_initcall
> genpd_power_off_unused(), to power off the genpd, in case the CPU
> device was offline.
> 
> In other words, the new behaviour is even slightly better. Does it make sense?

Yes, thank you!

pm_runtime_get_sync() will call genpd_runtime_resume(),
which calls genpd_power_on().


Kind regards,
Niklas

> > > +     if (IS_ERR_OR_NULL(dev))
> > > +             return dev;
> > > +
> > > +     pm_runtime_irq_safe(dev);
> > > +     if (cpu_online(cpu))
> > > +             pm_runtime_get_sync(dev);
> > > +
> > > +     return dev;
> > > +}
> > > diff --git a/drivers/cpuidle/cpuidle-psci.h b/drivers/cpuidle/cpuidle-psci.h
> > > new file mode 100644
> > > index 000000000000..0cadbb71dc55
> > > --- /dev/null
> > > +++ b/drivers/cpuidle/cpuidle-psci.h
> > > @@ -0,0 +1,12 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +
> > > +#ifndef __CPUIDLE_PSCI_H
> > > +#define __CPUIDLE_PSCI_H
> > > +
> > > +#ifdef CONFIG_PM_GENERIC_DOMAINS_OF
> > > +struct device *psci_dt_attach_cpu(int cpu);
> > > +#else
> > > +static inline struct device *psci_dt_attach_cpu(int cpu) { return NULL; }
> > > +#endif
> > > +
> > > +#endif /* __CPUIDLE_PSCI_H */
> > > --
> > > 2.17.1
> > >