Re: [PATCH] arm64: dts: exynos7: add support for cpuidle core power down
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
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
>> 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 10
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
>> >> ---
>> >> 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-su
Re: [PATCH] arm64: dts: exynos7: add support for cpuidle core power down
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
> 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
> >> ---
> >> 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 = <0x001>;
> >> + 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
> >
> >> + };
> >> +
Re: [PATCH] arm64: dts: exynos7: add support for cpuidle core power down
Hi Lorenzo,
On Wed, Oct 15, 2014 at 2:30 PM, Lorenzo Pieralisi
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
>> ---
>> 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 = <0x001>;
>> + 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-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] arm64: dts: exynos7: add support for cpuidle core power down
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 = <0x001>;
> > > + 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] arm64: dts: exynos7: add support for cpuidle core power down
> > + CPU_SLEEP: cpu-sleep {
> > + compatible = "arm,idle-state";
> > + local-timer-stop;
> > + arm,psci-suspend-param = <0x001>;
> > + 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] arm64: dts: exynos7: add support for cpuidle core power down
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
> ---
> 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 = <0x001>;
> + 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
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
