diff mbox series

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

Message ID 20191029164438.17012-9-ulf.hansson@linaro.org (mailing list archive)
State Superseded
Headers show
Series cpuidle: psci: Support hierarchical CPU arrangement | expand

Commit Message

Ulf Hansson Oct. 29, 2019, 4:44 p.m. UTC 
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

Comments

Niklas Cassel Nov. 7, 2019, 9:13 a.m. UTC | #1 
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?
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.)


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
>
Ulf Hansson Nov. 7, 2019, 10:22 a.m. UTC | #2 
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?

Kind regards
Uffe

>
>
> 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
> >
Niklas Cassel Nov. 11, 2019, 11:36 a.m. UTC | #3 
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
> > >
Sudeep Holla Nov. 15, 2019, 5:13 p.m. UTC | #4 
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

This was super confusing for a minute until I noticed the difference
between _ and - used here. I know such pattern is used in the kernel,
just that it's difficult to notice on first go :)

>
>  ###############################################################################
>  # 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");
> +	if (IS_ERR_OR_NULL(dev))
> +		return dev;
> +
> +	pm_runtime_irq_safe(dev);
> +	if (cpu_online(cpu))
> +		pm_runtime_get_sync(dev);

I probably have to wait till I see the user of this, but until then I
assume we have some way to deal with CPU HP machinery for this.

Other than that, it looks fine. I will get back to this to ack or with
more questions as I review further.

--
Regards,
Sudeep
Ulf Hansson Nov. 18, 2019, 12:28 p.m. UTC | #5 
On Fri, 15 Nov 2019 at 18:13, Sudeep Holla <sudeep.holla@arm.com> 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
>
> This was super confusing for a minute until I noticed the difference
> between _ and - used here. I know such pattern is used in the kernel,
> just that it's difficult to notice on first go :)
>
> >
> >  ###############################################################################
> >  # 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");
> > +     if (IS_ERR_OR_NULL(dev))
> > +             return dev;
> > +
> > +     pm_runtime_irq_safe(dev);
> > +     if (cpu_online(cpu))
> > +             pm_runtime_get_sync(dev);
>
> I probably have to wait till I see the user of this, but until then I
> assume we have some way to deal with CPU HP machinery for this.

Yes, I discussed this with Lorenzo at LPC as well. I did not include a
patch in the series using a CPU HP, simply because I am targeting to
land the basic support first.

For now, this means that the "cluster" will remain on even if there
are CPUs being put offline.

>
> Other than that, it looks fine. I will get back to this to ack or with
> more questions as I review further.

Great, thanks!

Kind regards
Uffe
diff mbox series

Patch

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");
+	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 */