| Message ID | 1470351902-43103-16-git-send-email-lina.iyer@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/08/16 00:05, Lina Iyer wrote: > Define power domain and the power states for the domain as defined by > the PSCI firmware. > The 8916 firmware supports OS initiated method of > powering off the CPU clusters. How is that related to the this DTS change, more details below ? > > Cc: <devicetree@vger.kernel.org> > Signed-off-by: Lina Iyer <lina.iyer@linaro.org> > --- > arch/arm64/boot/dts/qcom/msm8916.dtsi | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi > index 3029773..eb0aaed 100644 > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi > @@ -64,6 +64,7 @@ > next-level-cache = <&L2_0>; > enable-method = "psci"; > cpu-idle-states = <&CPU_SPC>; > + power-domains = <&CPU_PD>; This is really messy. We need to have idle state information at one place. I prefer to have a hierarchal representation of power-domains for CPU with idle-states at each level. > }; > > CPU1: cpu@1 { > @@ -73,6 +74,7 @@ > next-level-cache = <&L2_0>; > enable-method = "psci"; > cpu-idle-states = <&CPU_SPC>; > + power-domains = <&CPU_PD>; > }; > > CPU2: cpu@2 { > @@ -82,6 +84,7 @@ > next-level-cache = <&L2_0>; > enable-method = "psci"; > cpu-idle-states = <&CPU_SPC>; > + power-domains = <&CPU_PD>; > }; > > CPU3: cpu@3 { > @@ -91,6 +94,7 @@ > next-level-cache = <&L2_0>; > enable-method = "psci"; > cpu-idle-states = <&CPU_SPC>; > + power-domains = <&CPU_PD>; > }; > > L2_0: l2-cache { > @@ -113,6 +117,29 @@ > psci { > compatible = "arm,psci-1.0"; > method = "smc"; Why is it inside PSCI node ? I don't see a need for that. If it needs to be here, then amend the binding document. > + > + CPU_PD: cpu-pd@0 { > + #power-domain-cells = <0>; > + domain-idle-states = <&CLUSTER_RET>, <&CLUSTER_PWR_DWN>; > + }; > + > + domain-states { > + CLUSTER_RET: domain_ret { > + compatible = "arm,idle-state"; > + arm,psci-suspend-param = <0x1000010>; > + entry-latency-us = <500>; > + exit-latency-us = <500>; > + min-residency-us = <2000>; > + }; > + > + CLUSTER_PWR_DWN: domain_gdhs { > + compatible = "arm,idle-state"; > + arm,psci-suspend-param = <0x1000030>; > + entry-latency-us = <2000>; > + exit-latency-us = <2000>; > + min-residency-us = <6000>; > + }; > + }; So how do you collapse these states into the cpu level states ? We should be able to cope up with platform co-ordinated mode of idle. For me, this binding and the representation here is designed only to address OS co-ordinated mode of idle support but it should be other way around. Design the bindings that can cater any mode (platform and OS co-ordinated)
On Wed, Aug 10 2016 at 09:27 -0600, Sudeep Holla wrote: > > >On 05/08/16 00:05, Lina Iyer wrote: >>Define power domain and the power states for the domain as defined by >>the PSCI firmware. > >>The 8916 firmware supports OS initiated method of >>powering off the CPU clusters. > >How is that related to the this DTS change, more details below ? > >> >>Cc: <devicetree@vger.kernel.org> >>Signed-off-by: Lina Iyer <lina.iyer@linaro.org> >>--- >> arch/arm64/boot/dts/qcom/msm8916.dtsi | 27 +++++++++++++++++++++++++++ >> 1 file changed, 27 insertions(+) >> >>diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi >>index 3029773..eb0aaed 100644 >>--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi >>+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi >>@@ -64,6 +64,7 @@ >> next-level-cache = <&L2_0>; >> enable-method = "psci"; >> cpu-idle-states = <&CPU_SPC>; >>+ power-domains = <&CPU_PD>; > >This is really messy. We need to have idle state information at one >place. I prefer to have a hierarchal representation of power-domains >for CPU with idle-states at each level. > >> }; I see where are going with that. We need to then isolate idle states from all devices (including CPU) and put them under the umberella of the domain/parent idle states. We also need to remember that domain idle states are not just for CPU domains. There are generic PM domains that also define their idle states. For some, that hierarchy may not make sense. So forcing it on all domains is not correct as well. >> >> CPU1: cpu@1 { >>@@ -73,6 +74,7 @@ >> next-level-cache = <&L2_0>; >> enable-method = "psci"; >> cpu-idle-states = <&CPU_SPC>; >>+ power-domains = <&CPU_PD>; >> }; >> >> CPU2: cpu@2 { >>@@ -82,6 +84,7 @@ >> next-level-cache = <&L2_0>; >> enable-method = "psci"; >> cpu-idle-states = <&CPU_SPC>; >>+ power-domains = <&CPU_PD>; >> }; >> >> CPU3: cpu@3 { >>@@ -91,6 +94,7 @@ >> next-level-cache = <&L2_0>; >> enable-method = "psci"; >> cpu-idle-states = <&CPU_SPC>; >>+ power-domains = <&CPU_PD>; >> }; >> >> L2_0: l2-cache { >>@@ -113,6 +117,29 @@ >> psci { >> compatible = "arm,psci-1.0"; >> method = "smc"; > >Why is it inside PSCI node ? I don't see a need for that. >If it needs to be here, then amend the binding document. > It is described in patch 13/15. It is inside PSCI node, because PSCI has the domain controller. >>+ >>+ CPU_PD: cpu-pd@0 { >>+ #power-domain-cells = <0>; >>+ domain-idle-states = <&CLUSTER_RET>, <&CLUSTER_PWR_DWN>; >>+ }; >>+ >>+ domain-states { >>+ CLUSTER_RET: domain_ret { >>+ compatible = "arm,idle-state"; >>+ arm,psci-suspend-param = <0x1000010>; >>+ entry-latency-us = <500>; >>+ exit-latency-us = <500>; >>+ min-residency-us = <2000>; >>+ }; >>+ >>+ CLUSTER_PWR_DWN: domain_gdhs { >>+ compatible = "arm,idle-state"; >>+ arm,psci-suspend-param = <0x1000030>; >>+ entry-latency-us = <2000>; >>+ exit-latency-us = <2000>; >>+ min-residency-us = <6000>; >>+ }; >>+ }; > >So how do you collapse these states into the cpu level states ? Why do you have to collapse? >We should be able to cope up with platform co-ordinated mode of idle. >For me, this binding and the representation here is designed only to >address OS co-ordinated mode of idle support but it should be other way >around. Design the bindings that can cater any mode (platform and OS >co-ordinated) As explained to Brendan on the version2 of the series, OSI and PC are orthagonal to each other. The idle state definition in the devicetree exactly matches the unique approaches of these 2 modes. In platform coordinated, the CPU determines the idle state of the domain and selects the state, hence it makes sense to extend the cpu-idle-states to cover those domain states. In OSI, the CPUs only determine their idle states. When they are done with their idle state, they bubble up and let the domain choose its idle state and therefore the domain-idle-states is part of the domain controller. With this addition platform coordinated representation is not broken. If your SoC supports both platform and os modes, then you can specify the idle states of both of them in the DT. The clause in firmware/psci.c will however, choose OSI if its available. I am not sure we want to dynamically switch betweeen OSI and PC at runtime. Thanks, Lina
On 10/08/16 18:35, Lina Iyer wrote: > On Wed, Aug 10 2016 at 09:27 -0600, Sudeep Holla wrote: >> >> >> On 05/08/16 00:05, Lina Iyer wrote: >>> Define power domain and the power states for the domain as defined by >>> the PSCI firmware. >> >>> The 8916 firmware supports OS initiated method of >>> powering off the CPU clusters. >> >> How is that related to the this DTS change, more details below ? >> >>> >>> Cc: <devicetree@vger.kernel.org> >>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org> >>> --- >>> arch/arm64/boot/dts/qcom/msm8916.dtsi | 27 +++++++++++++++++++++++++++ >>> 1 file changed, 27 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi >>> b/arch/arm64/boot/dts/qcom/msm8916.dtsi >>> index 3029773..eb0aaed 100644 >>> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi >>> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi >>> @@ -64,6 +64,7 @@ >>> next-level-cache = <&L2_0>; >>> enable-method = "psci"; >>> cpu-idle-states = <&CPU_SPC>; >>> + power-domains = <&CPU_PD>; >> >> This is really messy. We need to have idle state information at one >> place. I prefer to have a hierarchal representation of power-domains >> for CPU with idle-states at each level. >> >>> }; > I see where are going with that. We need to then isolate idle states > from all devices (including CPU) and put them under the umberella of the > domain/parent idle states. > > We also need to remember that domain idle states are not just for CPU > domains. There are generic PM domains that also define their idle > states. For some, that hierarchy may not make sense. So forcing it on > all domains is not correct as well. > Yes again I absolutely agree with that. But just represent the hierarchy present in the hardware in the DT. If it doesn't exist in the hardware for some device, then it won't be represented. >>> >>> CPU1: cpu@1 { >>> @@ -73,6 +74,7 @@ >>> next-level-cache = <&L2_0>; >>> enable-method = "psci"; >>> cpu-idle-states = <&CPU_SPC>; >>> + power-domains = <&CPU_PD>; >>> }; >>> >>> CPU2: cpu@2 { >>> @@ -82,6 +84,7 @@ >>> next-level-cache = <&L2_0>; >>> enable-method = "psci"; >>> cpu-idle-states = <&CPU_SPC>; >>> + power-domains = <&CPU_PD>; >>> }; >>> >>> CPU3: cpu@3 { >>> @@ -91,6 +94,7 @@ >>> next-level-cache = <&L2_0>; >>> enable-method = "psci"; >>> cpu-idle-states = <&CPU_SPC>; >>> + power-domains = <&CPU_PD>; >>> }; >>> >>> L2_0: l2-cache { >>> @@ -113,6 +117,29 @@ >>> psci { >>> compatible = "arm,psci-1.0"; >>> method = "smc"; >> >> Why is it inside PSCI node ? I don't see a need for that. >> If it needs to be here, then amend the binding document. >> > It is described in patch 13/15. > > It is inside PSCI node, because PSCI has the domain controller. > OK, I haven't gone through all the patches, I was just interested in the binding. Sorry for that. >>> + >>> + CPU_PD: cpu-pd@0 { >>> + #power-domain-cells = <0>; >>> + domain-idle-states = <&CLUSTER_RET>, <&CLUSTER_PWR_DWN>; >>> + }; >>> + >>> + domain-states { >>> + CLUSTER_RET: domain_ret { >>> + compatible = "arm,idle-state"; >>> + arm,psci-suspend-param = <0x1000010>; >>> + entry-latency-us = <500>; >>> + exit-latency-us = <500>; >>> + min-residency-us = <2000>; >>> + }; >>> + >>> + CLUSTER_PWR_DWN: domain_gdhs { >>> + compatible = "arm,idle-state"; >>> + arm,psci-suspend-param = <0x1000030>; >>> + entry-latency-us = <2000>; >>> + exit-latency-us = <2000>; >>> + min-residency-us = <6000>; >>> + }; >>> + }; >> >> So how do you collapse these states into the cpu level states ? > Why do you have to collapse? > To deal with platform co-ordinated mode of cpu idle support. I think I gave an example in the other email. >> We should be able to cope up with platform co-ordinated mode of idle. >> For me, this binding and the representation here is designed only to >> address OS co-ordinated mode of idle support but it should be other way >> around. Design the bindings that can cater any mode (platform and OS >> co-ordinated) > As explained to Brendan on the version2 of the series, OSI and PC are > orthagonal to each other. The idle state definition in the devicetree > exactly matches the unique approaches of these 2 modes. > We don't design bindings that depend on the software approaches or configurations. It should represent the hardware. > In platform coordinated, the CPU determines the idle state of the domain > and selects the state, hence it makes sense to extend the > cpu-idle-states to cover those domain states. > So, are you saying that old bindings are for platform co-ordinated mode and these new ones are for OS co-ordinated mode. Make it clear in the binding document and get explicit approval from the DT maintainers for that. I really don't like that approach. > In OSI, the CPUs only determine their idle states. When they are done > with their idle state, they bubble up and let the domain choose its idle > state and therefore the domain-idle-states is part of the domain > controller. > I understand that, but I don't like the approach taken to define the DT bindings. DT should have one or the other form and any mode can be used as long as firmware is queried and put into appropriate mode of operation. > With this addition platform coordinated representation is not broken. If > your SoC supports both platform and os modes, then you can specify the > idle states of both of them in the DT. The clause in firmware/psci.c > will however, choose OSI if its available. I am not sure we want to > dynamically switch betweeen OSI and PC at runtime. > How ? cpu-idle-states in CPU node can't have both: 1. just it's idles states, and 2. complete list of flattened idle states as it' is present today.
diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi index 3029773..eb0aaed 100644 --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi @@ -64,6 +64,7 @@ next-level-cache = <&L2_0>; enable-method = "psci"; cpu-idle-states = <&CPU_SPC>; + power-domains = <&CPU_PD>; }; CPU1: cpu@1 { @@ -73,6 +74,7 @@ next-level-cache = <&L2_0>; enable-method = "psci"; cpu-idle-states = <&CPU_SPC>; + power-domains = <&CPU_PD>; }; CPU2: cpu@2 { @@ -82,6 +84,7 @@ next-level-cache = <&L2_0>; enable-method = "psci"; cpu-idle-states = <&CPU_SPC>; + power-domains = <&CPU_PD>; }; CPU3: cpu@3 { @@ -91,6 +94,7 @@ next-level-cache = <&L2_0>; enable-method = "psci"; cpu-idle-states = <&CPU_SPC>; + power-domains = <&CPU_PD>; }; L2_0: l2-cache { @@ -113,6 +117,29 @@ psci { compatible = "arm,psci-1.0"; method = "smc"; + + CPU_PD: cpu-pd@0 { + #power-domain-cells = <0>; + domain-idle-states = <&CLUSTER_RET>, <&CLUSTER_PWR_DWN>; + }; + + domain-states { + CLUSTER_RET: domain_ret { + compatible = "arm,idle-state"; + arm,psci-suspend-param = <0x1000010>; + entry-latency-us = <500>; + exit-latency-us = <500>; + min-residency-us = <2000>; + }; + + CLUSTER_PWR_DWN: domain_gdhs { + compatible = "arm,idle-state"; + arm,psci-suspend-param = <0x1000030>; + entry-latency-us = <2000>; + exit-latency-us = <2000>; + min-residency-us = <6000>; + }; + }; }; timer {
Define power domain and the power states for the domain as defined by the PSCI firmware. The 8916 firmware supports OS initiated method of powering off the CPU clusters. Cc: <devicetree@vger.kernel.org> Signed-off-by: Lina Iyer <lina.iyer@linaro.org> --- arch/arm64/boot/dts/qcom/msm8916.dtsi | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)