| Message ID | 1470351902-43103-3-git-send-email-lina.iyer@linaro.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, Aug 04, 2016 at 05:04:49PM -0600, Lina Iyer wrote: > From: Axel Haslam <ahaslam+renesas@baylibre.com> > > Update DT bindings to describe idle states of PM domains. > > Cc: <devicetree@vger.kernel.org> > Signed-off-by: Marc Titinger <mtitinger+renesas@baylibre.com> > Signed-off-by: Lina Iyer <lina.iyer@linaro.org> > [Lina: Added state properties, removed state names, wakeup-latency, > added of_pm_genpd_init() API, pruned commit text] > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > [Ulf: Moved around code to make it compile properly, rebased on top of multiple state support] This change doesn't make sense given this is just a binding doc. > --- > .../devicetree/bindings/power/power_domain.txt | 57 ++++++++++++++++++++++ > 1 file changed, 57 insertions(+) Acked-by: Rob Herring <robh@kernel.org> -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Lina, I have few concerns mainly due to the lack of description and not the binding per say. On 05/08/16 00:04, Lina Iyer wrote: > From: Axel Haslam <ahaslam+renesas@baylibre.com> > > Update DT bindings to describe idle states of PM domains. > > Cc: <devicetree@vger.kernel.org> > Signed-off-by: Marc Titinger <mtitinger+renesas@baylibre.com> > Signed-off-by: Lina Iyer <lina.iyer@linaro.org> > [Lina: Added state properties, removed state names, wakeup-latency, > added of_pm_genpd_init() API, pruned commit text] > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > [Ulf: Moved around code to make it compile properly, rebased on top of multiple state support] > --- > .../devicetree/bindings/power/power_domain.txt | 57 ++++++++++++++++++++++ > 1 file changed, 57 insertions(+) > > diff --git a/Documentation/devicetree/bindings/power/power_domain.txt b/Documentation/devicetree/bindings/power/power_domain.txt > index 025b5e7..4960486 100644 > --- a/Documentation/devicetree/bindings/power/power_domain.txt > +++ b/Documentation/devicetree/bindings/power/power_domain.txt > @@ -29,6 +29,10 @@ Optional properties: > specified by this binding. More details about power domain specifier are > available in the next section. > > +- domain-idle-states : A phandle of an idle-state that shall be soaked into a > + generic domain power state. The idle state definitions are > + compatible with arm,idle-state specified in [1]. > + So I assume these can be used for the genpd states. Either we rename it domain-power-states or make it clear that these domain-idle-states can also represent the power-states for normal devices. > Example: > > power: power-controller@12340000 { > @@ -59,6 +63,57 @@ The nodes above define two power controllers: 'parent' and 'child'. > Domains created by the 'child' power controller are subdomains of '0' power > domain provided by the 'parent' power controller. > > +Example 3: ARM v7 style CPU PM domains (Linux domain controller) > + > + cpus { > + #address-cells = <1>; > + #size-cells = <0>; > + > + CPU0: cpu@0 { > + device_type = "cpu"; > + compatible = "arm,cortex-a7", "arm,armv7"; > + reg = <0x0>; > + power-domains = <&a7_pd>; This example doesn't consider how do we deal with the presence off cpu-idle-states property in CPU nodes. IMO we need move even the cpu/core level idle states into its own power domain. It also helps to solve other usecases like PMU, debug/coresight devices attached to the core power domain(in most of the cases) while they may be in separate PD like PMUs on OMAP. That will help OS whether to save/restore the states on idle-entry. In [PATCH v3 15/15] ARM64: dts: Define CPU power domain for MSM8916, the idle-states are split across the cpu cpu-idle-states and pd domain-idle-states property. That looks like a really mess to me. We need to have all the idle state information at one place and in this case PD seems more appropriate instead of splitting them across. We can also keep the code clean and not break compatibility. Whenever both PD and CPU contains idle-states, PD must take precedence. Also these needs to be documented clearly in the binding. > + }; > + > + CPU1: cpu@1 { > + device_type = "cpu"; > + compatible = "arm,cortex-a15", "arm,armv7"; > + reg = <0x0>; > + power-domains = <&a15_pd>; > + }; > + }; > + > + pm-domains { > + a15_pd: a15_pd { > + /* will have A15 platform ARM_PD_METHOD_OF_DECLARE*/ the above comment make no sense in the binding document, remove it
Hi Sudeep, On Wed, Aug 10 2016 at 09:15 -0600, Sudeep Holla wrote: >Hi Lina, > >I have few concerns mainly due to the lack of description and not the >binding per say. > >On 05/08/16 00:04, Lina Iyer wrote: >>From: Axel Haslam <ahaslam+renesas@baylibre.com> >> >>Update DT bindings to describe idle states of PM domains. >> >>Cc: <devicetree@vger.kernel.org> >>Signed-off-by: Marc Titinger <mtitinger+renesas@baylibre.com> >>Signed-off-by: Lina Iyer <lina.iyer@linaro.org> >>[Lina: Added state properties, removed state names, wakeup-latency, >>added of_pm_genpd_init() API, pruned commit text] >>Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >>[Ulf: Moved around code to make it compile properly, rebased on top of multiple state support] >>--- >> .../devicetree/bindings/power/power_domain.txt | 57 ++++++++++++++++++++++ >> 1 file changed, 57 insertions(+) >> >>diff --git a/Documentation/devicetree/bindings/power/power_domain.txt b/Documentation/devicetree/bindings/power/power_domain.txt >>index 025b5e7..4960486 100644 >>--- a/Documentation/devicetree/bindings/power/power_domain.txt >>+++ b/Documentation/devicetree/bindings/power/power_domain.txt >>@@ -29,6 +29,10 @@ Optional properties: >> specified by this binding. More details about power domain specifier are >> available in the next section. >> >>+- domain-idle-states : A phandle of an idle-state that shall be soaked into a >>+ generic domain power state. The idle state definitions are >>+ compatible with arm,idle-state specified in [1]. >>+ > >So I assume these can be used for the genpd states. Either we rename >it domain-power-states or make it clear that these domain-idle-states >can also represent the power-states for normal devices. > These are the domains' idle states. These states are only used when the domain goes into idle, not when the domain is active. These are not power states that the domain can operate on either. Hence the idle-state moniker. Also, the bindings to describe the state are the same as arm,idle-state. It made sense to call these domain idle states instead of domain-power-states. >> Example: >> >> power: power-controller@12340000 { >>@@ -59,6 +63,57 @@ The nodes above define two power controllers: 'parent' and 'child'. >> Domains created by the 'child' power controller are subdomains of '0' power >> domain provided by the 'parent' power controller. >> >>+Example 3: ARM v7 style CPU PM domains (Linux domain controller) >>+ >>+ cpus { >>+ #address-cells = <1>; >>+ #size-cells = <0>; >>+ >>+ CPU0: cpu@0 { >>+ device_type = "cpu"; >>+ compatible = "arm,cortex-a7", "arm,armv7"; >>+ reg = <0x0>; >>+ power-domains = <&a7_pd>; > >This example doesn't consider how do we deal with the presence off >cpu-idle-states property in CPU nodes. > I can amend the example. But to answer your underlying question, they will exist as separate properties. >IMO we need move even the cpu/core level idle states into its own power >domain. It also helps to solve other usecases like PMU, debug/coresight >devices attached to the core power domain(in most of the cases) while >they may be in separate PD like PMUs on OMAP. That will help OS whether >to save/restore the states on idle-entry. > This idea was brought up by Kevin earlier in the discussions, but we shelved it for a later date. >In [PATCH v3 15/15] ARM64: dts: Define CPU power domain for MSM8916, the >idle-states are split across the cpu cpu-idle-states and pd >domain-idle-states property. That looks like a really mess to me. > It is pretty clear that CPUs cannot not define the domain idle states. Domains define their own idle states. Just as you mention above. CPU is just a single component in its domain. There may be other devices like PMUs, Coresights etc that also may have a say in the idle state the domain may be put in, when the devices are idle. As such, adding domain idle states to the CPU's idle state property is not appropriate. Our kernel has runtime PM for devices and then there is CPUidle, both are diverging without one knowing about the other. We have to start unifying them inorder to have better holistic power management in the SoC. To that regard, we have to start imagining CPUs as just another device, albeit a special device. But for our purposes in determining domain idle state, it will just be a device attached to the domain. >We need to have all the idle state information at one place and in this >case PD seems more appropriate instead of splitting them across. > That approach isn't correct. Where will we put the idle states of other devices that are also part of the domain? We are thinking about a model, where every device defines its own idle states and we define relationships between those idle states and their parents' idle states. Ofcourse, devices don't have idle states today, but that is something we have been pondering over. >We can also keep the code clean and not break compatibility. Whenever >both PD and CPU contains idle-states, PD must take precedence. > Why? The CPU and PD states are orthogonal. While the PD state is dependent on the CPU state, the latter is not true. Devices determine their own states. Based on the individual device states, we then determine the state of the parent and bubble up on the hierarchy. >Also these needs to be documented clearly in the binding. > >>+ }; >>+ >>+ CPU1: cpu@1 { >>+ device_type = "cpu"; >>+ compatible = "arm,cortex-a15", "arm,armv7"; >>+ reg = <0x0>; >>+ power-domains = <&a15_pd>; >>+ }; >>+ }; >>+ >>+ pm-domains { >>+ a15_pd: a15_pd { >>+ /* will have A15 platform ARM_PD_METHOD_OF_DECLARE*/ > >the above comment make no sense in the binding document, remove it > Yup. Will remove. Thanks, Lina -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/08/16 17:40, Lina Iyer wrote: > Hi Sudeep, > > On Wed, Aug 10 2016 at 09:15 -0600, Sudeep Holla wrote: >> Hi Lina, >> >> I have few concerns mainly due to the lack of description and not the >> binding per say. [...] >>> +- domain-idle-states : A phandle of an idle-state that shall be >>> soaked into a >>> + generic domain power state. The idle state >>> definitions are >>> + compatible with arm,idle-state specified in [1]. >>> + >> >> So I assume these can be used for the genpd states. Either we rename >> it domain-power-states or make it clear that these domain-idle-states >> can also represent the power-states for normal devices. >> > These are the domains' idle states. These states are only used when the > domain goes into idle, not when the domain is active. These are not > power states that the domain can operate on either. Hence the idle-state > moniker. I am not sure if we can tell that the device is running in all it's power states. E.g. in ACPI IIUC, only D0 state represent running state, while D{1,2,3} are power states which consume less power than D0/running state. I think genpd is designed on those lines. So I was thinking if these idle-states can also if use from non-CPU devices w.r.t binding, it will serve as D-state equivalent in ACPI > Also, the bindings to describe the state are the same as arm,idle-state. > It made sense to call these domain idle states instead of > domain-power-states. > I am fine with that, but we have idle states compatible to distinguish it from normal device idle/power states. >>> Example: >>> >>> power: power-controller@12340000 { >>> @@ -59,6 +63,57 @@ The nodes above define two power controllers: >>> 'parent' and 'child'. >>> Domains created by the 'child' power controller are subdomains of '0' >>> power >>> domain provided by the 'parent' power controller. >>> >>> +Example 3: ARM v7 style CPU PM domains (Linux domain controller) >>> + >>> + cpus { >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + >>> + CPU0: cpu@0 { >>> + device_type = "cpu"; >>> + compatible = "arm,cortex-a7", "arm,armv7"; >>> + reg = <0x0>; >>> + power-domains = <&a7_pd>; >> >> This example doesn't consider how do we deal with the presence off >> cpu-idle-states property in CPU nodes. >> > I can amend the example. But to answer your underlying question, they > will exist as separate properties. > Which is asking for trouble IMO. One should take precedence over other if both are present. Ideally new DTs can just have PD, we will continue to support cpu-idle-states for old DT. >> IMO we need move even the cpu/core level idle states into its own power >> domain. It also helps to solve other usecases like PMU, debug/coresight >> devices attached to the core power domain(in most of the cases) while >> they may be in separate PD like PMUs on OMAP. That will help OS whether >> to save/restore the states on idle-entry. >> > This idea was brought up by Kevin earlier in the discussions, but we > shelved it for a later date. > Any particular reasons ? I will try to dig up. I would do in one shot especially with respect to bindings. Implementation wise, it's fine we can take up in stages. I don't see any issue adding it in first go. This binding is good, you just make it hierarchical and add more description. >> In [PATCH v3 15/15] ARM64: dts: Define CPU power domain for MSM8916, the >> idle-states are split across the cpu cpu-idle-states and pd >> domain-idle-states property. That looks like a really mess to me. >> > It is pretty clear that CPUs cannot not define the domain idle states. > Domains define their own idle states. Just as you mention above. CPU is > just a single component in its domain. There may be other devices like > PMUs, Coresights etc that also may have a say in the idle state the > domain may be put in, when the devices are idle. As such, adding domain > idle states to the CPU's idle state property is not appropriate. > No I am not saying we need to add domain idle states to the CPU's idle state property. I am saying we need to remove cpu-idle-states or ignore it when PD is present. And get all the idle state information for PD. I am objecting the split we are creating across CPU and higher level power domains. And this binding document is incomplete as it skips all those details. We just need PD handle in CPU and no idle state information there. Create PD hierarchy and have all idle state information at one place. > Our kernel has runtime PM for devices and then there is CPUidle, both > are diverging without one knowing about the other. We have to start > unifying them inorder to have better holistic power management in the > SoC. To that regard, we have to start imagining CPUs as just another > device, albeit a special device. But for our purposes in determining > domain idle state, it will just be a device attached to the domain. > Absolutely agree on that. No arguments. I am asking to go a step ahead to include even cpu/core level power domains not just cluster/higher level domains. >> We need to have all the idle state information at one place and in this >> case PD seems more appropriate instead of splitting them across. >> > That approach isn't correct. Where will we put the idle states of other > devices that are also part of the domain? We are thinking about a model, > where every device defines its own idle states and we define > relationships between those idle states and their parents' idle states. Yes I understand. You confused me here. Won't that be one-to-one relationship ? If not, how is that dealt in the current bindings ? > Ofcourse, devices don't have idle states today, but that is something we > have been pondering over. > Yes we these binding should be easily extensible, I don't see any issue. >> We can also keep the code clean and not break compatibility. Whenever >> both PD and CPU contains idle-states, PD must take precedence. >> > Why? > The CPU and PD states are orthogonal. While the PD state is dependent on > the CPU state, the latter is not true. Devices determine their own > states. Based on the individual device states, we then determine the > state of the parent and bubble up on the hierarchy. > I may be missing something. Now with your example in the binding, if another device shares the cluster PD, can it have different idle states? If so how does it map ? In general whatever binding we come up must not just address OS coordinated mode. Also I was thinking to have better coverage in the description by having a bit more complex system like: cluster0 CLUSTER_RET(Retention) CLUSTER_PG(Power Gate) core0 CORE_RET CORE_PG core1 CORE_RET CORE_PG cluster1 CLUSTER_RET CLUSTER_PG core0 CORE_RET CORE_PG core1 CORE_RET CORE_PG Platform Co-ordinate supports the following states and we should be able to determine that from the binding: CORE_RET CORE_PG CORE_RET + CLUSTER_RET CORE_PG + CLUSTER_RET CORE_PG + CLUSTER_PG
On 10/08/16 19:09, Sudeep Holla wrote: > > > On 10/08/16 17:40, Lina Iyer wrote: >> Hi Sudeep, >> >> On Wed, Aug 10 2016 at 09:15 -0600, Sudeep Holla wrote: >>> Hi Lina, >>> >>> I have few concerns mainly due to the lack of description and not the >>> binding per say. > > [...] > >>>> +- domain-idle-states : A phandle of an idle-state that shall be >>>> soaked into a >>>> + generic domain power state. The idle state >>>> definitions are >>>> + compatible with arm,idle-state specified in [1]. >>>> + >>> >>> So I assume these can be used for the genpd states. Either we rename >>> it domain-power-states or make it clear that these domain-idle-states >>> can also represent the power-states for normal devices. >>> >> These are the domains' idle states. These states are only used when the >> domain goes into idle, not when the domain is active. These are not >> power states that the domain can operate on either. Hence the idle-state >> moniker. > > I am not sure if we can tell that the device is running in all it's > power states. E.g. in ACPI IIUC, only D0 state represent running state, > while D{1,2,3} are power states which consume less power than D0/running > state. I think genpd is designed on those lines. > I didn't complete this section earlier. Just to clarify we can label them as idle states or whatever. Strictly speaking today even cpu-idle-states have power off state, so the term is not important. What I intended to say is that it should be able to describe even device idle(power states minus the active running) states.
On Wed, Aug 10 2016 at 12:09 -0600, Sudeep Holla wrote: > > >On 10/08/16 17:40, Lina Iyer wrote: >>Hi Sudeep, >> >>On Wed, Aug 10 2016 at 09:15 -0600, Sudeep Holla wrote: >>>Hi Lina, >>> >>>I have few concerns mainly due to the lack of description and not the >>>binding per say. > >[...] > >>It is pretty clear that CPUs cannot not define the domain idle states. >>Domains define their own idle states. Just as you mention above. CPU is >>just a single component in its domain. There may be other devices like >>PMUs, Coresights etc that also may have a say in the idle state the >>domain may be put in, when the devices are idle. As such, adding domain >>idle states to the CPU's idle state property is not appropriate. >> > >No I am not saying we need to add domain idle states to the CPU's idle >state property. I am saying we need to remove cpu-idle-states or ignore >it when PD is present. And get all the idle state information for PD. > >I am objecting the split we are creating across CPU and higher level >power domains. And this binding document is incomplete as it skips all >those details. We just need PD handle in CPU and no idle state >information there. Create PD hierarchy and have all idle state >information at one place. > Let me think about this a bit and see what I can come up with. >>Our kernel has runtime PM for devices and then there is CPUidle, both >>are diverging without one knowing about the other. We have to start >>unifying them inorder to have better holistic power management in the >>SoC. To that regard, we have to start imagining CPUs as just another >>device, albeit a special device. But for our purposes in determining >>domain idle state, it will just be a device attached to the domain. >> > >Absolutely agree on that. No arguments. I am asking to go a step ahead >to include even cpu/core level power domains not just cluster/higher >level domains. > >>>We need to have all the idle state information at one place and in this >>>case PD seems more appropriate instead of splitting them across. >>> >>That approach isn't correct. Where will we put the idle states of other >>devices that are also part of the domain? We are thinking about a model, >>where every device defines its own idle states and we define >>relationships between those idle states and their parents' idle states. > >Yes I understand. You confused me here. Won't that be one-to-one >relationship ? If not, how is that dealt in the current bindings ? > >>Ofcourse, devices don't have idle states today, but that is something we >>have been pondering over. >> > >Yes we these binding should be easily extensible, I don't see any issue. > >>>We can also keep the code clean and not break compatibility. Whenever >>>both PD and CPU contains idle-states, PD must take precedence. >>> >>Why? >>The CPU and PD states are orthogonal. While the PD state is dependent on >>the CPU state, the latter is not true. Devices determine their own >>states. Based on the individual device states, we then determine the >>state of the parent and bubble up on the hierarchy. >> > >I may be missing something. Now with your example in the binding, if >another device shares the cluster PD, can it have different idle states? >If so how does it map ? > > >In general whatever binding we come up must not just address OS >coordinated mode. Also I was thinking to have better coverage in the >description by having a bit more complex system like: > >cluster0 > CLUSTER_RET(Retention) > CLUSTER_PG(Power Gate) > core0 > CORE_RET > CORE_PG > core1 > CORE_RET > CORE_PG > >cluster1 > CLUSTER_RET > CLUSTER_PG > core0 > CORE_RET > CORE_PG > core1 > CORE_RET > CORE_PG > >Platform Co-ordinate supports the following states and we should be >able to determine that from the binding: > >CORE_RET >CORE_PG >CORE_RET + CLUSTER_RET The problem that we have to sove here is knowing that CORE_RET + CLUSTER_PG (hypothetically) an invalid combination. Kevin and I debated it in the earlier RFC and we dont have a good way to solve this generically for all devices. >CORE_PG + CLUSTER_RET >CORE_PG + CLUSTER_PG > > Thanks, Lina -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Lina On 11/08/16 22:10, Lina Iyer wrote: > On Wed, Aug 10 2016 at 12:09 -0600, Sudeep Holla wrote: >> >> >> On 10/08/16 17:40, Lina Iyer wrote: >>> Hi Sudeep, >>> >>> On Wed, Aug 10 2016 at 09:15 -0600, Sudeep Holla wrote: >>>> Hi Lina, >>>> >>>> I have few concerns mainly due to the lack of description and not the >>>> binding per say. >> >> [...] >> >>> It is pretty clear that CPUs cannot not define the domain idle states. >>> Domains define their own idle states. Just as you mention above. CPU is >>> just a single component in its domain. There may be other devices like >>> PMUs, Coresights etc that also may have a say in the idle state the >>> domain may be put in, when the devices are idle. As such, adding domain >>> idle states to the CPU's idle state property is not appropriate. >>> >> >> No I am not saying we need to add domain idle states to the CPU's idle >> state property. I am saying we need to remove cpu-idle-states or ignore >> it when PD is present. And get all the idle state information for PD. >> >> I am objecting the split we are creating across CPU and higher level >> power domains. And this binding document is incomplete as it skips all >> those details. We just need PD handle in CPU and no idle state >> information there. Create PD hierarchy and have all idle state >> information at one place. >> > Let me think about this a bit and see what I can come up with. > >>> Our kernel has runtime PM for devices and then there is CPUidle, both >>> are diverging without one knowing about the other. We have to start >>> unifying them inorder to have better holistic power management in the >>> SoC. To that regard, we have to start imagining CPUs as just another >>> device, albeit a special device. But for our purposes in determining >>> domain idle state, it will just be a device attached to the domain. >>> >> >> Absolutely agree on that. No arguments. I am asking to go a step ahead >> to include even cpu/core level power domains not just cluster/higher >> level domains. >> >>>> We need to have all the idle state information at one place and in this >>>> case PD seems more appropriate instead of splitting them across. >>>> >>> That approach isn't correct. Where will we put the idle states of other >>> devices that are also part of the domain? We are thinking about a model, >>> where every device defines its own idle states and we define >>> relationships between those idle states and their parents' idle states. >> >> Yes I understand. You confused me here. Won't that be one-to-one >> relationship ? If not, how is that dealt in the current bindings ? >> >>> Ofcourse, devices don't have idle states today, but that is something we >>> have been pondering over. >>> >> >> Yes we these binding should be easily extensible, I don't see any issue. >> >>>> We can also keep the code clean and not break compatibility. Whenever >>>> both PD and CPU contains idle-states, PD must take precedence. >>>> >>> Why? >>> The CPU and PD states are orthogonal. While the PD state is dependent on >>> the CPU state, the latter is not true. Devices determine their own >>> states. Based on the individual device states, we then determine the >>> state of the parent and bubble up on the hierarchy. >>> >> >> I may be missing something. Now with your example in the binding, if >> another device shares the cluster PD, can it have different idle states? >> If so how does it map ? >> >> >> In general whatever binding we come up must not just address OS >> coordinated mode. Also I was thinking to have better coverage in the >> description by having a bit more complex system like: >> >> cluster0 >> CLUSTER_RET(Retention) >> CLUSTER_PG(Power Gate) >> core0 >> CORE_RET >> CORE_PG >> core1 >> CORE_RET >> CORE_PG >> >> cluster1 >> CLUSTER_RET >> CLUSTER_PG >> core0 >> CORE_RET >> CORE_PG >> core1 >> CORE_RET >> CORE_PG >> >> Platform Co-ordinate supports the following states and we should be >> able to determine that from the binding: >> >> CORE_RET >> CORE_PG >> CORE_RET + CLUSTER_RET > > The problem that we have to sove here is knowing that CORE_RET + > CLUSTER_PG (hypothetically) an invalid combination. Kevin and > I debated it in the earlier RFC and we dont have a good way to solve > this generically for all devices. > This is interesting. I had been working on the assumption that a parent power domain cannot enter any idle state until its children were all in their deepest idle state. I now realise that it's easy to imagine platforms where this isn't the case. However, I don't understand how your current bindings solve this issue and why using domain-power-states for all states (i.e. ignoring cpu-idle-states and putting CPU idle states in the domain-idle-states of a per-CPU power domain - I believe this is what Sudeep is suggesting) makes it any more difficult. Could you link to this previous discussion you mentioned? I'm having trouble finding it (R.I.P Gmane). >> CORE_PG + CLUSTER_RET >> CORE_PG + CLUSTER_PG >> >> > Thanks, > Lina > Cheers, Brendan IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/08/16 22:10, Lina Iyer wrote: > On Wed, Aug 10 2016 at 12:09 -0600, Sudeep Holla wrote: >> [...] >> cluster0 >> CLUSTER_RET(Retention) >> CLUSTER_PG(Power Gate) >> core0 >> CORE_RET >> CORE_PG >> core1 >> CORE_RET >> CORE_PG >> >> cluster1 >> CLUSTER_RET >> CLUSTER_PG >> core0 >> CORE_RET >> CORE_PG >> core1 >> CORE_RET >> CORE_PG >> >> Platform Co-ordinate supports the following states and we should be >> able to determine that from the binding: >> >> CORE_RET >> CORE_PG >> CORE_RET + CLUSTER_RET > > The problem that we have to sove here is knowing that CORE_RET + > CLUSTER_PG (hypothetically) an invalid combination. Kevin and > I debated it in the earlier RFC and we dont have a good way to solve > this generically for all devices. > Yes, I agree it's complex. But that needs to be solved IMO. I can think of 2 possible solutions: 1. Index the states(which people have not liked, but as along as we don't use it in the code as it for any other purpose, it should be fine) and then have each state mentioning what parent state can be entered at this child state(i.e. starting index and all states below it) 2. Something similar to (1) but without index instead phandles. Again these are just thoughts, others may think of some better solution(s). Sorry I haven't followed all the previous threads in detail.
Hi Lina, Apologies, I sent this reply before and automatically included an "IMPORTANT NOTICE" footer, please disregard that email, here's the same thing without the footer. On Thu, Aug 11, 2016 at 03:10:23PM -0600, Lina Iyer wrote: > On Wed, Aug 10 2016 at 12:09 -0600, Sudeep Holla wrote: > > > > > >On 10/08/16 17:40, Lina Iyer wrote: > >>Hi Sudeep, > >> > >>On Wed, Aug 10 2016 at 09:15 -0600, Sudeep Holla wrote: > >>>Hi Lina, > >>> > >>>I have few concerns mainly due to the lack of description and not the > >>>binding per say. > > > >[...] > > > >>It is pretty clear that CPUs cannot not define the domain idle states. > >>Domains define their own idle states. Just as you mention above. CPU is > >>just a single component in its domain. There may be other devices like > >>PMUs, Coresights etc that also may have a say in the idle state the > >>domain may be put in, when the devices are idle. As such, adding domain > >>idle states to the CPU's idle state property is not appropriate. > >> > > > >No I am not saying we need to add domain idle states to the CPU's idle > >state property. I am saying we need to remove cpu-idle-states or ignore > >it when PD is present. And get all the idle state information for PD. > > > >I am objecting the split we are creating across CPU and higher level > >power domains. And this binding document is incomplete as it skips all > >those details. We just need PD handle in CPU and no idle state > >information there. Create PD hierarchy and have all idle state > >information at one place. > > > Let me think about this a bit and see what I can come up with. > > >>Our kernel has runtime PM for devices and then there is CPUidle, both > >>are diverging without one knowing about the other. We have to start > >>unifying them inorder to have better holistic power management in the > >>SoC. To that regard, we have to start imagining CPUs as just another > >>device, albeit a special device. But for our purposes in determining > >>domain idle state, it will just be a device attached to the domain. > >> > > > >Absolutely agree on that. No arguments. I am asking to go a step ahead > >to include even cpu/core level power domains not just cluster/higher > >level domains. > > > >>>We need to have all the idle state information at one place and in this > >>>case PD seems more appropriate instead of splitting them across. > >>> > >>That approach isn't correct. Where will we put the idle states of other > >>devices that are also part of the domain? We are thinking about a model, > >>where every device defines its own idle states and we define > >>relationships between those idle states and their parents' idle states. > > > >Yes I understand. You confused me here. Won't that be one-to-one > >relationship ? If not, how is that dealt in the current bindings ? > > > >>Ofcourse, devices don't have idle states today, but that is something we > >>have been pondering over. > >> > > > >Yes we these binding should be easily extensible, I don't see any issue. > > > >>>We can also keep the code clean and not break compatibility. Whenever > >>>both PD and CPU contains idle-states, PD must take precedence. > >>> > >>Why? > >>The CPU and PD states are orthogonal. While the PD state is dependent on > >>the CPU state, the latter is not true. Devices determine their own > >>states. Based on the individual device states, we then determine the > >>state of the parent and bubble up on the hierarchy. > >> > > > >I may be missing something. Now with your example in the binding, if > >another device shares the cluster PD, can it have different idle states? > >If so how does it map ? > > > > > >In general whatever binding we come up must not just address OS > >coordinated mode. Also I was thinking to have better coverage in > >the description by having a bit more complex system like: > > > >cluster0 > > CLUSTER_RET(Retention) > > CLUSTER_PG(Power Gate) > > core0 > > CORE_RET > > CORE_PG > > core1 > > CORE_RET > > CORE_PG > > > >cluster1 > > CLUSTER_RET > > CLUSTER_PG > > core0 > > CORE_RET > > CORE_PG > > core1 > > CORE_RET > > CORE_PG > > > >Platform Co-ordinate supports the following states and we should > >be able to determine that from the binding: > > > >CORE_RET > >CORE_PG > >CORE_RET + CLUSTER_RET > > The problem that we have to sove here is knowing that CORE_RET + > CLUSTER_PG (hypothetically) an invalid combination. Kevin and > I debated it in the earlier RFC and we dont have a good way to solve > this generically for all devices. > This is interesting. I had been working on the assumption that a parent power domain cannot enter any idle state until its children were all in their deepest idle state. I now realise that it's easy to imagine platforms where this isn't the case. However, I don't understand how your current bindings solve this issue and why using domain-power-states for all states (i.e. ignoring cpu-idle-states and putting CPU idle states in the domain-idle-states of a per-CPU power domain - I believe this is what Sudeep is suggesting) makes it any more difficult. Could you link to this previous discussion you mentioned? I'm having trouble finding it (R.I.P Gmane). > >CORE_PG + CLUSTER_RET > >CORE_PG + CLUSTER_PG > > Cheers, Brendan -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 12 2016 at 06:35 -0600, Brendan Jackman wrote: >> >In general whatever binding we come up must not just address OS >> >coordinated mode. Also I was thinking to have better coverage in >> >the description by having a bit more complex system like: >> > >> >cluster0 >> > CLUSTER_RET(Retention) >> > CLUSTER_PG(Power Gate) >> > core0 >> > CORE_RET >> > CORE_PG >> > core1 >> > CORE_RET >> > CORE_PG >> > >> >cluster1 >> > CLUSTER_RET >> > CLUSTER_PG >> > core0 >> > CORE_RET >> > CORE_PG >> > core1 >> > CORE_RET >> > CORE_PG >> > >> >Platform Co-ordinate supports the following states and we should >> >be able to determine that from the binding: >> > >> >CORE_RET >> >CORE_PG >> >CORE_RET + CLUSTER_RET >> >> The problem that we have to sove here is knowing that CORE_RET + >> CLUSTER_PG (hypothetically) an invalid combination. Kevin and >> I debated it in the earlier RFC and we dont have a good way to solve >> this generically for all devices. >> > > >This is interesting. I had been working on the assumption that a parent >power domain cannot enter any idle state until its children were all in >their deepest idle state. I now realise that it's easy to imagine >platforms where this isn't the case. > >However, I don't understand how your current bindings solve this issue >and why using domain-power-states for all states (i.e. ignoring >cpu-idle-states and putting CPU idle states in the domain-idle-states of >a per-CPU power domain - I believe this is what Sudeep is suggesting) >makes it any more difficult. > You are right, my current bindings don't solve it. I imagined one would solve it by writing their own CPU PM Domain governor. In the context of platform coordinated, we dont have a choice in Linux. May be the firmware can assert that intelligence in not choosing those states. So, we may have states added to cpuidle that are invalid and never get chosen by the firmware. I am not sure, but may be that is acceptable. >Could you link to this previous discussion you mentioned? I'm having >trouble finding it (R.I.P Gmane). > Sigh. So hard to search. Let me see where it is, if it in mail or IRC communication. Thanks, Lina >> >CORE_PG + CLUSTER_RET >> >CORE_PG + CLUSTER_PG >> > > >Cheers, >Brendan -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 12 2016 at 04:08 -0600, Sudeep Holla wrote: > > >On 11/08/16 22:10, Lina Iyer wrote: >>On Wed, Aug 10 2016 at 12:09 -0600, Sudeep Holla wrote: >>> > >[...] > >>>cluster0 >>> CLUSTER_RET(Retention) >>> CLUSTER_PG(Power Gate) >>> core0 >>> CORE_RET >>> CORE_PG >>> core1 >>> CORE_RET >>> CORE_PG >>> >>>cluster1 >>> CLUSTER_RET >>> CLUSTER_PG >>> core0 >>> CORE_RET >>> CORE_PG >>> core1 >>> CORE_RET >>> CORE_PG >>> >>>Platform Co-ordinate supports the following states and we should be >>>able to determine that from the binding: >>> >>>CORE_RET >>>CORE_PG >>>CORE_RET + CLUSTER_RET >> >>The problem that we have to sove here is knowing that CORE_RET + >>CLUSTER_PG (hypothetically) an invalid combination. Kevin and >>I debated it in the earlier RFC and we dont have a good way to solve >>this generically for all devices. >> > >Yes, I agree it's complex. But that needs to be solved IMO. > >I can think of 2 possible solutions: > >1. Index the states(which people have not liked, but as along as we > don't use it in the code as it for any other purpose, it should be > fine) and then have each state mentioning what parent state can be > entered at this child state(i.e. starting index and all states below > it) > This is how QCOM solved it downstream. >2. Something similar to (1) but without index instead phandles. > The problem is when you have non-CPU devices in the device tree and since they do not have a way to represent states like CPU, we did not have a clear path to that. Hence we punted that to later. Whatever we do, we should solve it for a generic PM domain, not just CPU domains. Thanks, Lina >Again these are just thoughts, others may think of some better >solution(s). Sorry I haven't followed all the previous threads in detail. > >-- >Regards, >Sudeep -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15/08/16 17:08, Lina Iyer wrote: > On Fri, Aug 12 2016 at 04:08 -0600, Sudeep Holla wrote: >> >> >> On 11/08/16 22:10, Lina Iyer wrote: >>> On Wed, Aug 10 2016 at 12:09 -0600, Sudeep Holla wrote: >>>> >> >> [...] >> >>>> cluster0 >>>> CLUSTER_RET(Retention) >>>> CLUSTER_PG(Power Gate) >>>> core0 >>>> CORE_RET >>>> CORE_PG >>>> core1 >>>> CORE_RET >>>> CORE_PG >>>> >>>> cluster1 >>>> CLUSTER_RET >>>> CLUSTER_PG >>>> core0 >>>> CORE_RET >>>> CORE_PG >>>> core1 >>>> CORE_RET >>>> CORE_PG >>>> >>>> Platform Co-ordinate supports the following states and we should be >>>> able to determine that from the binding: >>>> >>>> CORE_RET >>>> CORE_PG >>>> CORE_RET + CLUSTER_RET >>> >>> The problem that we have to sove here is knowing that CORE_RET + >>> CLUSTER_PG (hypothetically) an invalid combination. Kevin and >>> I debated it in the earlier RFC and we dont have a good way to solve >>> this generically for all devices. >>> >> >> Yes, I agree it's complex. But that needs to be solved IMO. >> >> I can think of 2 possible solutions: >> >> 1. Index the states(which people have not liked, but as along as we >> don't use it in the code as it for any other purpose, it should be >> fine) and then have each state mentioning what parent state can be >> entered at this child state(i.e. starting index and all states below >> it) >> > This is how QCOM solved it downstream. > Yes even ACPI has indices to solve this. >> 2. Something similar to (1) but without index instead phandles. >> > > The problem is when you have non-CPU devices in the device tree and > since they do not have a way to represent states like CPU, we did not > have a clear path to that. Hence we punted that to later. Whatever we > do, we should solve it for a generic PM domain, not just CPU domains. > Yes bindings defined here should be applicable for devices to, but only CPU's will have this hierarchy while the devices need not bother about hierarchy. However the parent power domain can ever the state which is least common denominator of all it's children power domain. That's my understanding. No ?
On Mon, Aug 15 2016 at 10:14 -0600, Sudeep Holla wrote: > > >On 15/08/16 17:08, Lina Iyer wrote: >>On Fri, Aug 12 2016 at 04:08 -0600, Sudeep Holla wrote: >>> >>> >>>On 11/08/16 22:10, Lina Iyer wrote: >>>>On Wed, Aug 10 2016 at 12:09 -0600, Sudeep Holla wrote: >>>>> >>> >>>[...] >>> >>>>>cluster0 >>>>> CLUSTER_RET(Retention) >>>>> CLUSTER_PG(Power Gate) >>>>> core0 >>>>> CORE_RET >>>>> CORE_PG >>>>> core1 >>>>> CORE_RET >>>>> CORE_PG >>>>> >>>>>cluster1 >>>>> CLUSTER_RET >>>>> CLUSTER_PG >>>>> core0 >>>>> CORE_RET >>>>> CORE_PG >>>>> core1 >>>>> CORE_RET >>>>> CORE_PG >>>>> >>>>>Platform Co-ordinate supports the following states and we should be >>>>>able to determine that from the binding: >>>>> >>>>>CORE_RET >>>>>CORE_PG >>>>>CORE_RET + CLUSTER_RET >>>> >>>>The problem that we have to sove here is knowing that CORE_RET + >>>>CLUSTER_PG (hypothetically) an invalid combination. Kevin and >>>>I debated it in the earlier RFC and we dont have a good way to solve >>>>this generically for all devices. >>>> >>> >>>Yes, I agree it's complex. But that needs to be solved IMO. >>> >>>I can think of 2 possible solutions: >>> >>>1. Index the states(which people have not liked, but as along as we >>> don't use it in the code as it for any other purpose, it should be >>> fine) and then have each state mentioning what parent state can be >>> entered at this child state(i.e. starting index and all states below >>> it) >>> >>This is how QCOM solved it downstream. >> > >Yes even ACPI has indices to solve this. > >>>2. Something similar to (1) but without index instead phandles. >>> >> >>The problem is when you have non-CPU devices in the device tree and >>since they do not have a way to represent states like CPU, we did not >>have a clear path to that. Hence we punted that to later. Whatever we >>do, we should solve it for a generic PM domain, not just CPU domains. >> > >Yes bindings defined here should be applicable for devices to, but only >CPU's will have this hierarchy while the devices need not bother about >hierarchy. However the parent power domain can ever the state which is >least common denominator of all it's children power domain. That's my >understanding. No ? > That is correct. But say if all the CPUs choose CORE_RET + CLUSTER_PG, which is invalid and the firmware has to ignore it and does CORE_RET + CLUSTER_RET instead, then Linux may have an inconsistent view of the state selection. Thanks, Lina >-- >Regards, >Sudeep -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 15/08/16 23:40, Lina Iyer wrote: > On Mon, Aug 15 2016 at 10:14 -0600, Sudeep Holla wrote: >> >> >> On 15/08/16 17:08, Lina Iyer wrote: >>> On Fri, Aug 12 2016 at 04:08 -0600, Sudeep Holla wrote: >>>> >>>> >>>> On 11/08/16 22:10, Lina Iyer wrote: >>>>> On Wed, Aug 10 2016 at 12:09 -0600, Sudeep Holla wrote: >>>>>> >>>> >>>> [...] >>>> >>>>>> cluster0 >>>>>> CLUSTER_RET(Retention) >>>>>> CLUSTER_PG(Power Gate) >>>>>> core0 >>>>>> CORE_RET >>>>>> CORE_PG >>>>>> core1 >>>>>> CORE_RET >>>>>> CORE_PG >>>>>> >>>>>> cluster1 >>>>>> CLUSTER_RET >>>>>> CLUSTER_PG >>>>>> core0 >>>>>> CORE_RET >>>>>> CORE_PG >>>>>> core1 >>>>>> CORE_RET >>>>>> CORE_PG >>>>>> >>>>>> Platform Co-ordinate supports the following states and we should be >>>>>> able to determine that from the binding: >>>>>> >>>>>> CORE_RET >>>>>> CORE_PG >>>>>> CORE_RET + CLUSTER_RET >>>>> >>>>> The problem that we have to sove here is knowing that CORE_RET + >>>>> CLUSTER_PG (hypothetically) an invalid combination. Kevin and >>>>> I debated it in the earlier RFC and we dont have a good way to solve >>>>> this generically for all devices. >>>>> >>>> >>>> Yes, I agree it's complex. But that needs to be solved IMO. >>>> >>>> I can think of 2 possible solutions: >>>> >>>> 1. Index the states(which people have not liked, but as along as we >>>> don't use it in the code as it for any other purpose, it should be >>>> fine) and then have each state mentioning what parent state can be >>>> entered at this child state(i.e. starting index and all states below >>>> it) >>>> >>> This is how QCOM solved it downstream. >>> >> >> Yes even ACPI has indices to solve this. >> >>>> 2. Something similar to (1) but without index instead phandles. >>>> >>> >>> The problem is when you have non-CPU devices in the device tree and >>> since they do not have a way to represent states like CPU, we did not >>> have a clear path to that. Hence we punted that to later. Whatever we >>> do, we should solve it for a generic PM domain, not just CPU domains. >>> >> >> Yes bindings defined here should be applicable for devices to, but only >> CPU's will have this hierarchy while the devices need not bother about >> hierarchy. However the parent power domain can ever the state which is >> least common denominator of all it's children power domain. That's my >> understanding. No ? >> Are you saying that the parent can enter the shallowest idle state that all its children are in (I.e if all its children are in "retention" then it can enter "retention")? I don't know what the reality is on existing platforms but it doesn't sound like 100% safe assumption to make. Also I don't think you can necessarily correlate idle states at different domain levels - i.e. here we've matched up the idea of "retention" at core level with that of "retention" at cluster level. I may have misunderstood you there.. > That is correct. But say if all the CPUs choose CORE_RET + CLUSTER_PG, > which is invalid and the firmware has to ignore it and does CORE_RET + > CLUSTER_RET instead, then Linux may have an inconsistent view of the > state selection. > Perhaps a better starting point would be to go with the assumption that a parent PD can only enter any idle state once its children are in their deepest idle states. That is, we'd miss out on CORE_RET + CLUSTER_RET but have no invalid ones. Then a later addition to the bindings as discussed above could enable the possibility of those combinations to be expressed. > Thanks, > Lina > >> -- >> Regards, >> Sudeep > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Lina, Agh, sorry, sent with the "IMPORTANT NOTICE" again, still getting used to mailing lists.. here's the message again without it. On Mon, Aug 15, 2016 at 04:40:14PM -0600, Lina Iyer wrote: > On Mon, Aug 15 2016 at 10:14 -0600, Sudeep Holla wrote: > > > > > >On 15/08/16 17:08, Lina Iyer wrote: > >>On Fri, Aug 12 2016 at 04:08 -0600, Sudeep Holla wrote: > >>> > >>> > >>>On 11/08/16 22:10, Lina Iyer wrote: > >>>>On Wed, Aug 10 2016 at 12:09 -0600, Sudeep Holla wrote: > >>>>> > >>> > >>>[...] > >>> > >>>>>cluster0 > >>>>> CLUSTER_RET(Retention) > >>>>> CLUSTER_PG(Power Gate) > >>>>> core0 > >>>>> CORE_RET > >>>>> CORE_PG > >>>>> core1 > >>>>> CORE_RET > >>>>> CORE_PG > >>>>> > >>>>>cluster1 > >>>>> CLUSTER_RET > >>>>> CLUSTER_PG > >>>>> core0 > >>>>> CORE_RET > >>>>> CORE_PG > >>>>> core1 > >>>>> CORE_RET > >>>>> CORE_PG > >>>>> > >>>>>Platform Co-ordinate supports the following states and we should be > >>>>>able to determine that from the binding: > >>>>> > >>>>>CORE_RET > >>>>>CORE_PG > >>>>>CORE_RET + CLUSTER_RET > >>>> > >>>>The problem that we have to sove here is knowing that CORE_RET + > >>>>CLUSTER_PG (hypothetically) an invalid combination. Kevin and > >>>>I debated it in the earlier RFC and we dont have a good way to solve > >>>>this generically for all devices. > >>>> > >>> > >>>Yes, I agree it's complex. But that needs to be solved IMO. > >>> > >>>I can think of 2 possible solutions: > >>> > >>>1. Index the states(which people have not liked, but as along as we > >>> don't use it in the code as it for any other purpose, it should be > >>> fine) and then have each state mentioning what parent state can be > >>> entered at this child state(i.e. starting index and all states below > >>> it) > >>> > >>This is how QCOM solved it downstream. > >> > > > >Yes even ACPI has indices to solve this. > > > >>>2. Something similar to (1) but without index instead phandles. > >>> > >> > >>The problem is when you have non-CPU devices in the device tree and > >>since they do not have a way to represent states like CPU, we did not > >>have a clear path to that. Hence we punted that to later. Whatever we > >>do, we should solve it for a generic PM domain, not just CPU domains. > >> > > > >Yes bindings defined here should be applicable for devices to, but only > >CPU's will have this hierarchy while the devices need not bother about > >hierarchy. However the parent power domain can ever the state which is > >least common denominator of all it's children power domain. That's my > >understanding. No? > > Are you saying that the parent can enter the shallowest idle state that all its children are in (I.e if all its children are in "retention" then it can enter "retention")? I don't know what the reality is on existing platforms but it doesn't sound like 100% safe assumption to make. Also I don't think you can necessarily correlate idle states at different domain levels - i.e. here we've matched up the idea of "retention" at core level with that of "retention" at cluster level. I may have misunderstood you there.. > That is correct. But say if all the CPUs choose CORE_RET + CLUSTER_PG, > which is invalid and the firmware has to ignore it and does CORE_RET + > CLUSTER_RET instead, then Linux may have an inconsistent view of the > state selection. > Perhaps a better starting point would be to go with the assumption that a parent PD can only enter any idle state once its children are in their deepest idle states. So in the example above we'd end up with CORE_RET CORE_PG CORE_PG + CLUSTER_RET CORE_PG + CLUSTER_PG (Missing out on CORE_RET + CLUSTER_RET, even though that's a valid combination from the hardware's perspective) Then a later addition to the bindings as discussed above could enable the possibility of those combinations to be expressed. > Thanks, > Lina > > >-- > >Regards, > >Sudeep > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 16/08/16 09:41, Brendan Jackman wrote: > Hi Lina, > On Mon, Aug 15, 2016 at 04:40:14PM -0600, Lina Iyer wrote: >> On Mon, Aug 15 2016 at 10:14 -0600, Sudeep Holla wrote: [,,,] >>> >>> Yes even ACPI has indices to solve this. >>> >>>>> 2. Something similar to (1) but without index instead phandles. >>>>> >>>> >>>> The problem is when you have non-CPU devices in the device tree and >>>> since they do not have a way to represent states like CPU, we did not >>>> have a clear path to that. Hence we punted that to later. Whatever we >>>> do, we should solve it for a generic PM domain, not just CPU domains. >>>> >>> >>> Yes bindings defined here should be applicable for devices to, but only >>> CPU's will have this hierarchy while the devices need not bother about >>> hierarchy. However the parent power domain can ever the state which is >>> least common denominator of all it's children power domain. That's my >>> understanding. No? >>> > > > Are you saying that the parent can enter the shallowest idle state that all its > children are in (I.e if all its children are in "retention" then it can enter > "retention")? I don't know what the reality is on existing platforms but it > doesn't sound like 100% safe assumption to make. I was referring to non-CPU/device power states above. For CPU we do need a mechanism in place to indicate the dependency. > Also I don't think you can > necessarily correlate idle states at different domain levels - i.e. here we've > matched up the idea of "retention" at core level with that of "retention" at > cluster level. I may have misunderstood you there.. > Correct for CPUs. For normal devices and their power domains, it could straight forward. E.g if many devices are at-least at state D1(few may be at state D2 or above), the parent can enter D1.(D0-runnning and D1-D3 are low power states in the above example) >> That is correct. But say if all the CPUs choose CORE_RET + CLUSTER_PG, >> which is invalid and the firmware has to ignore it and does CORE_RET + >> CLUSTER_RET instead, then Linux may have an inconsistent view of the >> state selection. >> 1. First, CORE_RET + CLUSTER_PG should not be registered as valid idle state. 2. We do have inconsistent view already for platform co-ordinated idle In-fact it could happen even with OSC mode I believe. Platform can always demote the state, so OS can never get the exact view unless it queries the firmware for that explicitly(e.g. PSCI_STATS) > > Perhaps a better starting point would be to go with the assumption that a parent > PD can only enter any idle state once its children are in their deepest idle > states. > > So in the example above we'd end up with > > CORE_RET > CORE_PG > CORE_PG + CLUSTER_RET > CORE_PG + CLUSTER_PG > Yes this assumption seems good enough to me. At-least no invalid combination is ensured. > (Missing out on CORE_RET + CLUSTER_RET, even though that's a valid combination > from the hardware's perspective) > Yes, if it's a real issue then we need proper bindings to deal with that. Otherwise we can manage without the extra information. > Then a later addition to the bindings as discussed above could enable the > possibility of those combinations to be expressed. > Seems feasible solution to me, but better to make this explicit in the binding and check with few others. It looks fair enough assumption IMO.
Sudeep Holla <sudeep.holla@arm.com> writes: [...] > In general whatever binding we come up must not just address OS > coordinated mode. Also I was thinking to have better coverage in the > description by having a bit more complex system like: > > cluster0 > CLUSTER_RET(Retention) > CLUSTER_PG(Power Gate) > core0 > CORE_RET > CORE_PG > core1 > CORE_RET > CORE_PG Also, remember that a power domain may contain more than just CPUs, so this will also need to handle things like: device0..N DEV_CLK_GATE DEV_RET DEV_PG So, as (I think) Lina was trying to say, including CPU idle states inside domain idles states doesn't really scale well because it would also imply domain states would also include device idle states. IMO, the device-specific states belong in the device nodes, and that includes CPUs. It's up to the domain (genpd) governor to look at *all* devices in the domain, check their state and make a domain-wide decision. The tricky part remains, IMO, the mapping between device/CPU states and allowable domain states. As was suggested earlier, a good potential starting point would be that all devices/CPUs would need to be in their deepest state before the domain would make any decisions. While that leaves soem power savings on the table, it maps well to how genpd works today with only on/off states and could be extended with more complicated governors down the road. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 19/08/16 19:10, Kevin Hilman wrote: > Sudeep Holla <sudeep.holla@arm.com> writes: > > [...] > >> In general whatever binding we come up must not just address OS >> coordinated mode. Also I was thinking to have better coverage in the >> description by having a bit more complex system like: >> >> cluster0 >> CLUSTER_RET(Retention) >> CLUSTER_PG(Power Gate) >> core0 >> CORE_RET >> CORE_PG >> core1 >> CORE_RET >> CORE_PG > > Also, remember that a power domain may contain more than just CPUs, so > this will also need to handle things like: > > device0..N > DEV_CLK_GATE > DEV_RET > DEV_PG > > So, as (I think) Lina was trying to say, including CPU idle states > inside domain idles states doesn't really scale well because it would > also imply domain states would also include device idle states. > > IMO, the device-specific states belong in the device nodes, and that > includes CPUs. > OK, IIUC we don't have device idle states binding today, so we are not breaking anything there. Can you elaborate on the issue you see if we just have domain idle-states ? Is it because we currently create genpd domain for each entry ? If a CPU/Device can be enter idle-state(s) it means that it is in a power domain on its own, so I don't see any issue in such representation. > It's up to the domain (genpd) governor to look at *all* devices in the > domain, check their state and make a domain-wide decision. > Lets not mix the current genpd implementation in the kernel into this discussion for simplicity. How is the implementation in the kernel today and what can be done is a separate topic. What this discussion should aim at is to present the idle states in the system in the device tree so that it address the issues we have currently and extensible in near future with any compatibility issues. > The tricky part remains, IMO, the mapping between device/CPU states and > allowable domain states. > > As was suggested earlier, a good potential starting point would be that > all devices/CPUs would need to be in their deepest state before the > domain would make any decisions. While that leaves soem power savings > on the table, it maps well to how genpd works today with only on/off > states and could be extended with more complicated governors down the > road. > Agreed. Some example below for discussion, feel free to add more cases. -- Regards, Sudeep --->8 1. Dual cluster with 2 CPUs in each cluster with powerdown at both CPU and cluster level idle-states { CPU_SLEEP_0: cpu-sleep-0 { ... entry-latency-us = <300>; ... }; CLUSTER_SLEEP_0: cluster-sleep-0 { ... entry-latency-us = <300>; ... }; }; cpu@0 { ... /* * implentation may ignore cpu-idle-states if power-domains * has idle-states, DT's may have both for backward compatibility */ cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>; power-domains = <&CPU_0_1_PD>; ... }; cpu@1 { ... cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>; power-domains = <&CPU_0_1_PD>; ... }; cpu@100 { ... cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>; power-domains = <&CPU_1_0_PD>; ... }; cpu@101 { ... cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>; power-domains = <&CPU_1_1_PD>; ... }; power-domains { CLUSTER_0_PD: cluster-0-pd { #power-domain-cells = <0>; domain-idle-states = <&CLUSTER_SLEEP_0>; }; CPU_0_0_PD: cpu-0-0-pd@0 { #power-domain-cells = <0>; domain-idle-states = <&CPU_SLEEP_0>; power-domains = <&CLUSTER_0_PD>; }; CPU_0_1_PD: cpu-0-1-pd@1 { #power-domain-cells = <0>; domain-idle-states = <&CPU_SLEEP_0>; power-domains = <&CLUSTER_0_PD>; }; CLUSTER_1_PD: cluster-1-pd { #power-domain-cells = <0>; domain-idle-states = <&CLUSTER_SLEEP_0>; }; CPU_1_0_PD: cpu-1-0-pd@0 { #power-domain-cells = <0>; domain-idle-states = <&CPU_SLEEP_0>; power-domains = <&CLUSTER_1_PD>; }; CPU_1_1_PD: cpu-1-1-pd@1 { #power-domain-cells = <0>; domain-idle-states = <&CPU_SLEEP_0>; power-domains = <&CLUSTER_1_PD>; }; }; 2. Dual cluster with 2 CPUs in each cluster with retention and powerdown at both CPU and cluster level idle-states { CPU_SLEEP_0: cpu-sleep-0 { /* Retention */ ... entry-latency-us = <100>; ... }; CPU_SLEEP_1: cpu-sleep-1 { /* Power-down */ ... entry-latency-us = <500>; ... }; CLUSTER_SLEEP_0: cluster-sleep-0 { /* Retention */ ... entry-latency-us = <300>; ... }; CLUSTER_SLEEP_1: cluster-sleep-1 {/* Power-down */ ... entry-latency-us = <1000>; ... }; }; cpu@0 { ... power-domains = <&CPU_0_1_PD>; ... }; cpu@1 { ... power-domains = <&CPU_0_1_PD>; ... }; cpu@100 { ... power-domains = <&CPU_1_0_PD>; ... }; cpu@101 { ... power-domains = <&CPU_1_1_PD>; ... }; power-domains { /* * Each cluster/core PD may point to different idle states, * it's all same here in the example to keep it short and * simple */ CLUSTER_0_PD: cluster-0-pd { #power-domain-cells = <0>; domain-idle-states = <&CLUSTER_SLEEP_0 &CLUSTER_SLEEP_1>; }; CPU_0_0_PD: cpu-0-0-pd@0 { #power-domain-cells = <0>; domain-idle-states = <&CPU_SLEEP_0 &CPU_SLEEP_1>; power-domains = <&CLUSTER_0_PD>; }; CPU_0_1_PD: cpu-0-1-pd@1 { #power-domain-cells = <0>; domain-idle-states = <&CPU_SLEEP_0 &CPU_SLEEP_1>; power-domains = <&CLUSTER_0_PD>; }; CLUSTER_1_PD: cluster-1-pd { #power-domain-cells = <0>; domain-idle-states = <&CLUSTER_SLEEP_0 &CLUSTER_SLEEP_1>; }; CPU_1_0_PD: cpu-1-0-pd@0 { #power-domain-cells = <0>; domain-idle-states = <&CPU_SLEEP_0 &CPU_SLEEP_1>; power-domains = <&CLUSTER_1_PD>; }; CPU_1_1_PD: cpu-1-1-pd@1 { #power-domain-cells = <0>; domain-idle-states = <&CPU_SLEEP_0 &CPU_SLEEP_1>; power-domains = <&CLUSTER_1_PD>; }; }; 3. Dual cluster with 2 CPUs in each cluster with retention and powerdown at just cluster level idle-states { CLUSTER_SLEEP_0: cluster-sleep-0 { /* Retention */ ... entry-latency-us = <300>; ... }; CLUSTER_SLEEP_1: cluster-sleep-1 {/* Power-down */ ... entry-latency-us = <1000>; ... }; }; cpu@0 { ... power-domains = <&CLUSTER_0_PD>; ... }; cpu@1 { ... power-domains = <&CLUSTER_0_PD>; ... }; cpu@100 { ... power-domains = <&CLUSTER_1_PD>; ... }; cpu@101 { ... power-domains = <&CLUSTER_1_PD>; ... }; power-domains { CLUSTER_0_PD: cluster-0-pd { #power-domain-cells = <0>; domain-idle-states = <&CLUSTER_SLEEP_0 &CLUSTER_SLEEP_1>; }; CLUSTER_1_PD: cluster-1-pd { #power-domain-cells = <0>; domain-idle-states = <&CLUSTER_SLEEP_0 &CLUSTER_SLEEP_1>; }; }; 4. 4 devices sharing the power domain. idle-states { /* * Device idle states may differ from CPU idle states in terms * of the list of properties */ DEVPD_SLEEP_0: devpd-sleep-0 { /* Retention */ ... entry-latency-us = <300>; ... }; DEVPD_SLEEP_1: devpd-sleep-1 {/* Power-down */ ... entry-latency-us = <1000>; ... }; }; dev@0 { ... power-domains = <&DEV_PD_0>; ... }; dev@1 { ... power-domains = <&DEV_PD_0>; ... }; dev@2 { ... power-domains = <&DEV_PD_0>; ... }; dev@3 { ... power-domains = <&DEV_PD_0>; ... }; power-domains { DEV_PD_0: device-pd-0 { #power-domain-cells = <0>; domain-idle-states = <&DEVPD_SLEEP_0 &DEVPD_SLEEP_1>; }; }; 5. 4 devices sharing the power domain + another device sharing the power domain but has it's own sub-domain idle-states { DEVPD_0_SLEEP_0: devpd-sleep-0 { /* Retention */ ... entry-latency-us = <300>; ... }; DEVPD_0_SLEEP_1: devpd-sleep-1 {/* Power-down */ ... entry-latency-us = <1000>; ... }; DEVPD_1_SLEEP_0: devpd-sleep-0 { /* Retention */ ... entry-latency-us = <300>; ... }; DEVPD_1_SLEEP_1: devpd-sleep-1 {/* Power-down */ ... entry-latency-us = <1000>; ... }; }; dev@0 { ... power-domains = <&DEV_PD_0>; ... }; dev@1 { ... power-domains = <&DEV_PD_0>; ... }; dev@2 { ... power-domains = <&DEV_PD_0>; ... }; dev@3 { ... power-domains = <&DEV_PD_0>; ... }; dev@4 { ... power-domains = <&DEV_PD_1>; ... }; power-domains { DEV_PD_0: device-pd-0 { #power-domain-cells = <0>; domain-idle-states = <&DEVPD_0_SLEEP_0 &DEVPD_0_SLEEP_1>; }; DEV_PD_1: device-pd-1 { #power-domain-cells = <0>; power-domains = <&DEV_PD_0>; domain-idle-states = <&DEVPD_1_SLEEP_0 &DEVPD_1_SLEEP_1>; }; }; -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/power/power_domain.txt b/Documentation/devicetree/bindings/power/power_domain.txt index 025b5e7..4960486 100644 --- a/Documentation/devicetree/bindings/power/power_domain.txt +++ b/Documentation/devicetree/bindings/power/power_domain.txt @@ -29,6 +29,10 @@ Optional properties: specified by this binding. More details about power domain specifier are available in the next section. +- domain-idle-states : A phandle of an idle-state that shall be soaked into a + generic domain power state. The idle state definitions are + compatible with arm,idle-state specified in [1]. + Example: power: power-controller@12340000 { @@ -59,6 +63,57 @@ The nodes above define two power controllers: 'parent' and 'child'. Domains created by the 'child' power controller are subdomains of '0' power domain provided by the 'parent' power controller. +Example 3: ARM v7 style CPU PM domains (Linux domain controller) + + cpus { + #address-cells = <1>; + #size-cells = <0>; + + CPU0: cpu@0 { + device_type = "cpu"; + compatible = "arm,cortex-a7", "arm,armv7"; + reg = <0x0>; + power-domains = <&a7_pd>; + }; + + CPU1: cpu@1 { + device_type = "cpu"; + compatible = "arm,cortex-a15", "arm,armv7"; + reg = <0x0>; + power-domains = <&a15_pd>; + }; + }; + + pm-domains { + a15_pd: a15_pd { + /* will have A15 platform ARM_PD_METHOD_OF_DECLARE*/ + compatible = "arm,cortex-a15"; + #power-domain-cells = <0>; + domain-idle-states = <&CLUSTER_SLEEP_0>; + }; + + a7_pd: a7_pd { + /* will have a A7 platform ARM_PD_METHOD_OF_DECLARE*/ + compatible = "arm,cortex-a7"; + #power-domain-cells = <0>; + domain-idle-states = <&CLUSTER_SLEEP_0>, <&CLUSTER_SLEEP_1>; + }; + + CLUSTER_SLEEP_0: state0 { + compatible = "arm,idle-state"; + entry-latency-us = <1000>; + exit-latency-us = <2000>; + min-residency-us = <10000>; + }; + + CLUSTER_SLEEP_1: state1 { + compatible = "arm,idle-state"; + entry-latency-us = <5000>; + exit-latency-us = <5000>; + min-residency-us = <100000>; + }; + }; + ==PM domain consumers== Required properties: @@ -76,3 +131,5 @@ Example: The node above defines a typical PM domain consumer device, which is located inside a PM domain with index 0 of a power controller represented by a node with the label "power". + +[1]. Documentation/devicetree/bindings/arm/idle-states.txt