| Message ID | 20191010113937.15962-1-ulf.hansson@linaro.org |
|---|---|
| Headers | show |
| Series | cpuidle: psci: Support hierarchical CPU arrangement | expand |
On Thu, 10 Oct 2019 at 13:40, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > This series enables initial support for hierarchical CPU arrangement, managed > by PSCI and its corresponding cpuidle driver. It's based on using the generic > PM domain (genpd), which nowadays also supports devices belonging to CPUs. > > The last DTS patch enables the hierarchical topology to be used for the Qcom > 410c Dragonboard, which supports the PSCI OS-initiated mode. > > Do note, most of the code in this series have been posted earlier, but from the > latest version being reviewed, we agreed on that it was better to re-work the > PSCI backend driver as a first step, simply to get a clean interface towards the > cpuidle driver. > > Summary of the main-changes since the last submission [1]: > > - Moved implementation from the psci FW dricer into cpuidle-psci. > > - Re-requesting review of the DT bindings, as we have moved to yaml. No > changes as such, but tried to clarify a few things in the text. > > - Drop the patch enabling support for CPU hotplug, postponing that to the next > step. > > - Respect the hierarchical topology in DT only when OSI mode is supported. > This is to start simple and we can always extend the support on top. > > The series is also available at: > git.linaro.org/people/ulf.hansson/linux-pm.git next > > Kind regards > Ulf Hansson > > [1] > https://lwn.net/Articles/788306/ > > > Lina Iyer (1): > cpuidle: dt: Support hierarchical CPU idle states > > Ulf Hansson (12): > cpuidle: psci: Fix potential access to unmapped memory > dt: psci: Update DT bindings to support hierarchical PSCI states > firmware: psci: Export functions to manage the OSI mode > of: base: Add of_get_cpu_state_node() to get idle states for a CPU > node > cpuidle: psci: Simplify OF parsing of CPU idle state nodes > cpuidle: psci: Support hierarchical CPU idle states > cpuidle: psci: Prepare to use OS initiated suspend mode via PM domains > cpuidle: psci: Add support for PM domains by using genpd > cpuidle: psci: Add a helper to attach a CPU to its PM domain > cpuidle: psci: Attach CPU devices to their PM domains > cpuidle: psci: Manage runtime PM in the idle path > arm64: dts: Convert to the hierarchical CPU topology layout for > MSM8916 > > .../devicetree/bindings/arm/psci.yaml | 153 +++++++++ > arch/arm64/boot/dts/qcom/msm8916.dtsi | 57 +++- > drivers/cpuidle/Makefile | 4 +- > drivers/cpuidle/cpuidle-psci-domain.c | 302 ++++++++++++++++++ > drivers/cpuidle/cpuidle-psci.c | 106 ++++-- > drivers/cpuidle/cpuidle-psci.h | 17 + > drivers/cpuidle/dt_idle_states.c | 5 +- > drivers/firmware/psci/psci.c | 18 +- > drivers/of/base.c | 36 +++ > include/linux/of.h | 8 + > include/linux/psci.h | 2 + > 11 files changed, 673 insertions(+), 35 deletions(-) > create mode 100644 drivers/cpuidle/cpuidle-psci-domain.c > create mode 100644 drivers/cpuidle/cpuidle-psci.h > > -- > 2.17.1 > Sudeep, Lorenzo, Just wanted to give you a gentle ping about this series, especially patch1 is kind of urgent. Kind regards Uffe
On Thu, Oct 10, 2019 at 01:39:31PM +0200, Ulf Hansson wrote: > Currently CPU's idle states are represented using the flattened model. > Let's add support for the hierarchical layout, via converting to use > of_get_cpu_state_node(). > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > drivers/cpuidle/cpuidle-psci.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c > index 1195a1056139..5c30f23a8a7b 100644 > --- a/drivers/cpuidle/cpuidle-psci.c > +++ b/drivers/cpuidle/cpuidle-psci.c > @@ -85,7 +85,7 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node, > return -ENOMEM; > > for (i = 0; i < state_nodes; i++) { > - state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i); > + state_node = of_get_cpu_state_node(cpu_node, i); Ah, here we go. Sorry, ignore my comment in previous patch then. Reviewed-by: Sudeep Holla <sudeep.holla@arm.com> -- Regards, Sudeep
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@arm.com> 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@linaro.org> > > > --- > > > 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. > > Further, it's specified as > > optional in the generic binding, do we make it "required" for this psci > > idle states binding anywhere that I missed ? > > Good point! Unless you tell me differently, I will update the DT doc > to clarify this is "required". > No but why is my question ? We don't have to. If firmware/DT wants to specify the name, sure. But it must remain optional IMO. -- Regards, Sudeep
On Sun, 27 Oct 2019 at 03:30, Sudeep Holla <sudeep.holla@arm.com> 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@arm.com> 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@linaro.org> > > > > --- > > > > 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. Have a look at these binding, there are already upstream, perhaps that clarifies this? Documentation/devicetree/bindings/opp/qcom-nvmem-cpufreq.txt > > > > Further, it's specified as > > > optional in the generic binding, do we make it "required" for this psci > > > idle states binding anywhere that I missed ? > > > > Good point! Unless you tell me differently, I will update the DT doc > > to clarify this is "required". > > > > No but why is my question ? We don't have to. If firmware/DT wants to > specify the name, sure. But it must remain optional IMO. According the QCOM CPR case, we need a way to distinguish what power domain we should attach the CPU to. If we don't use power-domain-names to do that, what else should we use? Kind regards Uffe
+ Niklas On Mon, 28 Oct 2019 at 08:49, Sudeep Holla <sudeep.holla@arm.com> 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@arm.com> 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@arm.com> 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@linaro.org> > > > > > > --- > > > > > > 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. 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? Kind regards Uffe
This series enables initial support for hierarchical CPU arrangement, managed by PSCI and its corresponding cpuidle driver. It's based on using the generic PM domain (genpd), which nowadays also supports devices belonging to CPUs. The last DTS patch enables the hierarchical topology to be used for the Qcom 410c Dragonboard, which supports the PSCI OS-initiated mode. Do note, most of the code in this series have been posted earlier, but from the latest version being reviewed, we agreed on that it was better to re-work the PSCI backend driver as a first step, simply to get a clean interface towards the cpuidle driver. Summary of the main-changes since the last submission [1]: - Moved implementation from the psci FW dricer into cpuidle-psci. - Re-requesting review of the DT bindings, as we have moved to yaml. No changes as such, but tried to clarify a few things in the text. - Drop the patch enabling support for CPU hotplug, postponing that to the next step. - Respect the hierarchical topology in DT only when OSI mode is supported. This is to start simple and we can always extend the support on top. The series is also available at: git.linaro.org/people/ulf.hansson/linux-pm.git next Kind regards Ulf Hansson [1] https://lwn.net/Articles/788306/ Lina Iyer (1): cpuidle: dt: Support hierarchical CPU idle states Ulf Hansson (12): cpuidle: psci: Fix potential access to unmapped memory dt: psci: Update DT bindings to support hierarchical PSCI states firmware: psci: Export functions to manage the OSI mode of: base: Add of_get_cpu_state_node() to get idle states for a CPU node cpuidle: psci: Simplify OF parsing of CPU idle state nodes cpuidle: psci: Support hierarchical CPU idle states cpuidle: psci: Prepare to use OS initiated suspend mode via PM domains cpuidle: psci: Add support for PM domains by using genpd cpuidle: psci: Add a helper to attach a CPU to its PM domain cpuidle: psci: Attach CPU devices to their PM domains cpuidle: psci: Manage runtime PM in the idle path arm64: dts: Convert to the hierarchical CPU topology layout for MSM8916 .../devicetree/bindings/arm/psci.yaml | 153 +++++++++ arch/arm64/boot/dts/qcom/msm8916.dtsi | 57 +++- drivers/cpuidle/Makefile | 4 +- drivers/cpuidle/cpuidle-psci-domain.c | 302 ++++++++++++++++++ drivers/cpuidle/cpuidle-psci.c | 106 ++++-- drivers/cpuidle/cpuidle-psci.h | 17 + drivers/cpuidle/dt_idle_states.c | 5 +- drivers/firmware/psci/psci.c | 18 +- drivers/of/base.c | 36 +++ include/linux/of.h | 8 + include/linux/psci.h | 2 + 11 files changed, 673 insertions(+), 35 deletions(-) create mode 100644 drivers/cpuidle/cpuidle-psci-domain.c create mode 100644 drivers/cpuidle/cpuidle-psci.h -- 2.17.1