| Message ID | 1413354920-20165-1-git-send-email-k.chander@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Oct 15, 2014 at 07:35:20AM +0100, Chander Kashyap wrote: > Exynos7 has core power down state where cores can be powered off independently. > This patch adds support for this state. Please tell us more about the idle-state values you are adding, in particular entry, exit latencies and min-residency values. > Signed-off-by: Chander Kashyap <k.chander@samsung.com> > --- > This patch has following dependencies: > - [PATCH v5 0/8] arch: arm64: Enable support for Samsung Exynos7 SoC > http://www.spinics.net/lists/linux-samsung-soc/msg37047.html > - [PATCH v9 0/8] ARM generic idle states > http://permalink.gmane.org/gmane.linux.power-management.general/49224 Series above was merged, so dependency is stale. > arch/arm64/boot/dts/exynos/exynos7.dtsi | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/arch/arm64/boot/dts/exynos/exynos7.dtsi b/arch/arm64/boot/dts/exynos/exynos7.dtsi > index ce221ac..8e0a034 100644 > --- a/arch/arm64/boot/dts/exynos/exynos7.dtsi > +++ b/arch/arm64/boot/dts/exynos/exynos7.dtsi > @@ -36,6 +36,7 @@ > device_type = "cpu"; > compatible = "arm,cortex-a57", "arm,armv8"; > enable-method = "psci"; > + cpu-idle-states = <&CPU_SLEEP>; I would add cpu-idle-states phandle after the reg property, as defined in the idle states bindings. > reg = <0x0>; > }; > > @@ -43,6 +44,7 @@ > device_type = "cpu"; > compatible = "arm,cortex-a57", "arm,armv8"; > enable-method = "psci"; > + cpu-idle-states = <&CPU_SLEEP>; > reg = <0x1>; > }; > > @@ -50,6 +52,7 @@ > device_type = "cpu"; > compatible = "arm,cortex-a57", "arm,armv8"; > enable-method = "psci"; > + cpu-idle-states = <&CPU_SLEEP>; > reg = <0x2>; > }; > > @@ -57,8 +60,23 @@ > device_type = "cpu"; > compatible = "arm,cortex-a57", "arm,armv8"; > enable-method = "psci"; > + cpu-idle-states = <&CPU_SLEEP>; > reg = <0x3>; > }; > + > + idle-states { > + entry-method = "arm,psci"; > + > + CPU_SLEEP: cpu-sleep { > + compatible = "arm,idle-state"; > + local-timer-stop; > + arm,psci-suspend-param = <0x0010000>; > + entry-latency-us = <20>; > + exit-latency-us = <150>; > + min-residency-us = <2100>; > + status = "enabled"; status ? This is not a documented property. If you need it please explain why, define its bindings and we can see how to accommodate it. Thank you, Lorenzo > + }; > + }; > }; > > psci { > -- > 1.7.9.5 > >
> > + CPU_SLEEP: cpu-sleep { > > + compatible = "arm,idle-state"; > > + local-timer-stop; > > + arm,psci-suspend-param = <0x0010000>; > > + entry-latency-us = <20>; > > + exit-latency-us = <150>; > > + min-residency-us = <2100>; > > + status = "enabled"; While status is a relatively standard property, it's absence implies everything is OK. There no need for it here as-is. Additionally, the canonical value is "okay", not "enabled", so this would fail were we to use of_device_is_available in the idle states parsing. > status ? This is not a documented property. If you need it please explain > why, define its bindings and we can see how to accommodate it. Do we expect that some idle states won't be available on some boards built from the same platform? Mark.
On Wed, Oct 15, 2014 at 02:02:18PM +0100, Mark Rutland wrote: > > > + CPU_SLEEP: cpu-sleep { > > > + compatible = "arm,idle-state"; > > > + local-timer-stop; > > > + arm,psci-suspend-param = <0x0010000>; > > > + entry-latency-us = <20>; > > > + exit-latency-us = <150>; > > > + min-residency-us = <2100>; > > > + status = "enabled"; > > While status is a relatively standard property, it's absence implies > everything is OK. There no need for it here as-is. > > Additionally, the canonical value is "okay", not "enabled", so this > would fail were we to use of_device_is_available in the idle states > parsing. Good point. I still want it documented in the bindings, keeping in mind your remark above. > > status ? This is not a documented property. If you need it please explain > > why, define its bindings and we can see how to accommodate it. > > Do we expect that some idle states won't be available on some boards > built from the same platform? I think it is something we should expect and be able to cope with that. I will add status to idle-states bindings updates for this cycle and patch DT parsing code accordingly. Lorenzo
Hi Lorenzo, On Wed, Oct 15, 2014 at 2:30 PM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > On Wed, Oct 15, 2014 at 07:35:20AM +0100, Chander Kashyap wrote: >> Exynos7 has core power down state where cores can be powered off independently. >> This patch adds support for this state. > > Please tell us more about the idle-state values you are adding, in particular > entry, exit latencies and min-residency values. Entry latency: This value is calculated as follows: On entry to arm64_enter_idle_state: timestamp1 = ktimeget(); after returning from cpu_suspend() timestamp2 = ktimeget(); latency = timestamp2-timestamp1; Cpu is not allowed to enter core powerdown by skipping wfi instruction at end. Hence calculated time contains entry time + failure exit time. Regarding exit-latency and target-residency time, got these values from HW team. I am using these as initial values and I will be working on optimizing these values with further experiments. If you could suggest any formal method of deriving these values, i can try those methods as well. > >> Signed-off-by: Chander Kashyap <k.chander@samsung.com> >> --- >> This patch has following dependencies: >> - [PATCH v5 0/8] arch: arm64: Enable support for Samsung Exynos7 SoC >> http://www.spinics.net/lists/linux-samsung-soc/msg37047.html >> - [PATCH v9 0/8] ARM generic idle states >> http://permalink.gmane.org/gmane.linux.power-management.general/49224 > > Series above was merged, so dependency is stale. i will remove this > >> arch/arm64/boot/dts/exynos/exynos7.dtsi | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+)dont >> >> diff --git a/arch/arm64/boot/dts/exynos/exynos7.dtsi b/arch/arm64/boot/dts/exynos/exynos7.dtsi >> index ce221ac..8e0a034 100644 >> --- a/arch/arm64/boot/dts/exynos/exynos7.dtsi >> +++ b/arch/arm64/boot/dts/exynos/exynos7.dtsi >> @@ -36,6 +36,7 @@ >> device_type = "cpu"; >> compatible = "arm,cortex-a57", "arm,armv8"; >> enable-method = "psci"; >> + cpu-idle-states = <&CPU_SLEEP>; > > I would add cpu-idle-states phandle after the reg property, as defined > in the idle states bindings. i will move this after reg property. > >> reg = <0x0>; >> }; >> >> @@ -43,6 +44,7 @@ >> device_type = "cpu"; >> compatible = "arm,cortex-a57", "arm,armv8"; >> enable-method = "psci"; >> + cpu-idle-states = <&CPU_SLEEP>; >> reg = <0x1>; >> }; >> >> @@ -50,6 +52,7 @@ >> device_type = "cpu"; >> compatible = "arm,cortex-a57", "arm,armv8"; >> enable-method = "psci"; >> + cpu-idle-states = <&CPU_SLEEP>; >> reg = <0x2>; >> }; >> >> @@ -57,8 +60,23 @@ >> device_type = "cpu"; >> compatible = "arm,cortex-a57", "arm,armv8"; >> enable-method = "psci"; >> + cpu-idle-states = <&CPU_SLEEP>; >> reg = <0x3>; >> }; >> + >> + idle-states { >> + entry-method = "arm,psci"; >> + >> + CPU_SLEEP: cpu-sleep { >> + compatible = "arm,idle-state"; >> + local-timer-stop; >> + arm,psci-suspend-param = <0x0010000>; >> + entry-latency-us = <20>; >> + exit-latency-us = <150>; >> + min-residency-us = <2100>; >> + status = "enabled"; > > status ? This is not a documented property. If you need it please explain > why, define its bindings and we can see how to accommodate it. I will add okay for status property. As per the bindings posted by you. regards, > > Thank you, > Lorenzo > >> + }; >> + }; >> }; >> >> psci { >> -- >> 1.7.9.5 >> >> > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, Oct 17, 2014 at 10:43:59AM +0100, Chander Kashyap wrote: > Hi Lorenzo, > > On Wed, Oct 15, 2014 at 2:30 PM, Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com> wrote: > > On Wed, Oct 15, 2014 at 07:35:20AM +0100, Chander Kashyap wrote: > >> Exynos7 has core power down state where cores can be powered off independently. > >> This patch adds support for this state. > > > > Please tell us more about the idle-state values you are adding, in particular > > entry, exit latencies and min-residency values. > > Entry latency: This value is calculated as follows: > > On entry to arm64_enter_idle_state: > timestamp1 = ktimeget(); > > after returning from cpu_suspend() > > timestamp2 = ktimeget(); > > latency = timestamp2-timestamp1; > > Cpu is not allowed to enter core powerdown by skipping wfi instruction at end. This may not be the worst case (because the worst case depends on the state of the cache in the core unless the latency is power down command dominated, so at the cost of being pedantic, please make sure that's what you are measuring and document it in the commit log). > Hence calculated time contains entry time + failure exit time. > > > Regarding > exit-latency and target-residency time, got these values from HW team. > > I am using these as initial values and I will be working on optimizing > these values with further experiments. > If you could suggest any formal method of deriving these values, i can > try those methods as well. Well, you have to set the core/cluster in worst case scenario and compute the break-even residency against wfi (since you have two states); it certainly has a dependency on PSCI implementation too among other things. exit-latency should come from HW design even though there is a cache refill factor to be considered too and should be factored in. Lorenzo > > > > >> Signed-off-by: Chander Kashyap <k.chander@samsung.com> > >> --- > >> This patch has following dependencies: > >> - [PATCH v5 0/8] arch: arm64: Enable support for Samsung Exynos7 SoC > >> http://www.spinics.net/lists/linux-samsung-soc/msg37047.html > >> - [PATCH v9 0/8] ARM generic idle states > >> http://permalink.gmane.org/gmane.linux.power-management.general/49224 > > > > Series above was merged, so dependency is stale. > > i will remove this > > > > >> arch/arm64/boot/dts/exynos/exynos7.dtsi | 18 ++++++++++++++++++ > >> 1 file changed, 18 insertions(+)dont > >> > >> diff --git a/arch/arm64/boot/dts/exynos/exynos7.dtsi b/arch/arm64/boot/dts/exynos/exynos7.dtsi > >> index ce221ac..8e0a034 100644 > >> --- a/arch/arm64/boot/dts/exynos/exynos7.dtsi > >> +++ b/arch/arm64/boot/dts/exynos/exynos7.dtsi > >> @@ -36,6 +36,7 @@ > >> device_type = "cpu"; > >> compatible = "arm,cortex-a57", "arm,armv8"; > >> enable-method = "psci"; > >> + cpu-idle-states = <&CPU_SLEEP>; > > > > I would add cpu-idle-states phandle after the reg property, as defined > > in the idle states bindings. > > i will move this after reg property. > > > > >> reg = <0x0>; > >> }; > >> > >> @@ -43,6 +44,7 @@ > >> device_type = "cpu"; > >> compatible = "arm,cortex-a57", "arm,armv8"; > >> enable-method = "psci"; > >> + cpu-idle-states = <&CPU_SLEEP>; > >> reg = <0x1>; > >> }; > >> > >> @@ -50,6 +52,7 @@ > >> device_type = "cpu"; > >> compatible = "arm,cortex-a57", "arm,armv8"; > >> enable-method = "psci"; > >> + cpu-idle-states = <&CPU_SLEEP>; > >> reg = <0x2>; > >> }; > >> > >> @@ -57,8 +60,23 @@ > >> device_type = "cpu"; > >> compatible = "arm,cortex-a57", "arm,armv8"; > >> enable-method = "psci"; > >> + cpu-idle-states = <&CPU_SLEEP>; > >> reg = <0x3>; > >> }; > >> + > >> + idle-states { > >> + entry-method = "arm,psci"; > >> + > >> + CPU_SLEEP: cpu-sleep { > >> + compatible = "arm,idle-state"; > >> + local-timer-stop; > >> + arm,psci-suspend-param = <0x0010000>; > >> + entry-latency-us = <20>; > >> + exit-latency-us = <150>; > >> + min-residency-us = <2100>; > >> + status = "enabled"; > > > > status ? This is not a documented property. If you need it please explain > > why, define its bindings and we can see how to accommodate it. > > I will add okay for status property. As per the bindings posted by you. > > regards, > > > > Thank you, > > Lorenzo > > > >> + }; > >> + }; > >> }; > >> > >> psci { > >> -- > >> 1.7.9.5 > >> > >> > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > -- > 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 >
Sorry for very late response. As i was on vacation so couldn’t reply. On Tue, Oct 21, 2014 at 10:03 PM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > On Fri, Oct 17, 2014 at 10:43:59AM +0100, Chander Kashyap wrote: >> Hi Lorenzo, >> >> On Wed, Oct 15, 2014 at 2:30 PM, Lorenzo Pieralisi >> <lorenzo.pieralisi@arm.com> wrote: >> > On Wed, Oct 15, 2014 at 07:35:20AM +0100, Chander Kashyap wrote: >> >> Exynos7 has core power down state where cores can be powered off independently. >> >> This patch adds support for this state. >> > >> > Please tell us more about the idle-state values you are adding, in particular >> > entry, exit latencies and min-residency values. >> >> Entry latency: This value is calculated as follows: >> >> On entry to arm64_enter_idle_state: >> timestamp1 = ktimeget(); >> >> after returning from cpu_suspend() >> >> timestamp2 = ktimeget(); >> >> latency = timestamp2-timestamp1; >> >> Cpu is not allowed to enter core powerdown by skipping wfi instruction at end. > This may not be the worst case (because the worst case depends on the state > of the cache in the core unless the latency is power down command dominated, > so at the cost of being pedantic, please make sure that's what you are > measuring and document it in the commit log). > If i understood correctly you are referring to cache flush time. The measured entry latency time is averaged time for 100000 transactions with varying load. I will document entry latency calculation in the commit message. >> Hence calculated time contains entry time + failure exit time. >> >> >> Regarding >> exit-latency and target-residency time, got these values from HW team. >> >> I am using these as initial values and I will be working on optimizing >> these values with further experiments. >> If you could suggest any formal method of deriving these values, i can >> try those methods as well. > > Well, you have to set the core/cluster in worst case scenario and > compute the break-even residency against wfi (since you have two > states); it certainly has a dependency on PSCI implementation too among > other things. > > exit-latency should come from HW design even though there is a cache > refill factor to be considered too and should be factored in. Exit and target residency are provided by HW team. I will post the V3 with changed commit message. > > Lorenzo > >> >> > >> >> Signed-off-by: Chander Kashyap <k.chander@samsung.com> >> >> --- >> >> This patch has following dependencies: >> >> - [PATCH v5 0/8] arch: arm64: Enable support for Samsung Exynos7 SoC >> >> http://www.spinics.net/lists/linux-samsung-soc/msg37047.html >> >> - [PATCH v9 0/8] ARM generic idle states >> >> http://permalink.gmane.org/gmane.linux.power-management.general/49224 >> > >> > Series above was merged, so dependency is stale. >> >> i will remove this >> >> > >> >> arch/arm64/boot/dts/exynos/exynos7.dtsi | 18 ++++++++++++++++++ >> >> 1 file changed, 18 insertions(+)dont >> >> >> >> diff --git a/arch/arm64/boot/dts/exynos/exynos7.dtsi b/arch/arm64/boot/dts/exynos/exynos7.dtsi >> >> index ce221ac..8e0a034 100644 >> >> --- a/arch/arm64/boot/dts/exynos/exynos7.dtsi >> >> +++ b/arch/arm64/boot/dts/exynos/exynos7.dtsi >> >> @@ -36,6 +36,7 @@ >> >> device_type = "cpu"; >> >> compatible = "arm,cortex-a57", "arm,armv8"; >> >> enable-method = "psci"; >> >> + cpu-idle-states = <&CPU_SLEEP>; >> > >> > I would add cpu-idle-states phandle after the reg property, as defined >> > in the idle states bindings. >> >> i will move this after reg property. >> >> > >> >> reg = <0x0>; >> >> }; >> >> >> >> @@ -43,6 +44,7 @@ >> >> device_type = "cpu"; >> >> compatible = "arm,cortex-a57", "arm,armv8"; >> >> enable-method = "psci"; >> >> + cpu-idle-states = <&CPU_SLEEP>; >> >> reg = <0x1>; >> >> }; >> >> >> >> @@ -50,6 +52,7 @@ >> >> device_type = "cpu"; >> >> compatible = "arm,cortex-a57", "arm,armv8"; >> >> enable-method = "psci"; >> >> + cpu-idle-states = <&CPU_SLEEP>; >> >> reg = <0x2>; >> >> }; >> >> >> >> @@ -57,8 +60,23 @@ >> >> device_type = "cpu"; >> >> compatible = "arm,cortex-a57", "arm,armv8"; >> >> enable-method = "psci"; >> >> + cpu-idle-states = <&CPU_SLEEP>; >> >> reg = <0x3>; >> >> }; >> >> + >> >> + idle-states { >> >> + entry-method = "arm,psci"; >> >> + >> >> + CPU_SLEEP: cpu-sleep { >> >> + compatible = "arm,idle-state"; >> >> + local-timer-stop; >> >> + arm,psci-suspend-param = <0x0010000>; >> >> + entry-latency-us = <20>; >> >> + exit-latency-us = <150>; >> >> + min-residency-us = <2100>; >> >> + status = "enabled"; >> > >> > status ? This is not a documented property. If you need it please explain >> > why, define its bindings and we can see how to accommodate it. >> >> I will add okay for status property. As per the bindings posted by you. >> >> regards, >> > >> > Thank you, >> > Lorenzo >> > >> >> + }; >> >> + }; >> >> }; >> >> >> >> psci { >> >> -- >> >> 1.7.9.5 >> >> >> >> >> > >> > >> > _______________________________________________ >> > linux-arm-kernel mailing list >> > linux-arm-kernel@lists.infradead.org >> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >> -- >> 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 >> > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/arch/arm64/boot/dts/exynos/exynos7.dtsi b/arch/arm64/boot/dts/exynos/exynos7.dtsi index ce221ac..8e0a034 100644 --- a/arch/arm64/boot/dts/exynos/exynos7.dtsi +++ b/arch/arm64/boot/dts/exynos/exynos7.dtsi @@ -36,6 +36,7 @@ device_type = "cpu"; compatible = "arm,cortex-a57", "arm,armv8"; enable-method = "psci"; + cpu-idle-states = <&CPU_SLEEP>; reg = <0x0>; }; @@ -43,6 +44,7 @@ device_type = "cpu"; compatible = "arm,cortex-a57", "arm,armv8"; enable-method = "psci"; + cpu-idle-states = <&CPU_SLEEP>; reg = <0x1>; }; @@ -50,6 +52,7 @@ device_type = "cpu"; compatible = "arm,cortex-a57", "arm,armv8"; enable-method = "psci"; + cpu-idle-states = <&CPU_SLEEP>; reg = <0x2>; }; @@ -57,8 +60,23 @@ device_type = "cpu"; compatible = "arm,cortex-a57", "arm,armv8"; enable-method = "psci"; + cpu-idle-states = <&CPU_SLEEP>; reg = <0x3>; }; + + idle-states { + entry-method = "arm,psci"; + + CPU_SLEEP: cpu-sleep { + compatible = "arm,idle-state"; + local-timer-stop; + arm,psci-suspend-param = <0x0010000>; + entry-latency-us = <20>; + exit-latency-us = <150>; + min-residency-us = <2100>; + status = "enabled"; + }; + }; }; psci {
Exynos7 has core power down state where cores can be powered off independently. This patch adds support for this state. Signed-off-by: Chander Kashyap <k.chander@samsung.com> --- This patch has following dependencies: - [PATCH v5 0/8] arch: arm64: Enable support for Samsung Exynos7 SoC http://www.spinics.net/lists/linux-samsung-soc/msg37047.html - [PATCH v9 0/8] ARM generic idle states http://permalink.gmane.org/gmane.linux.power-management.general/49224 arch/arm64/boot/dts/exynos/exynos7.dtsi | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)