Re: [PATCH] arm64: dts: exynos7: add support for cpuidle core power down

[Chander Kashyap]

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@xxxxxxx> 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@xxxxxxx> 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@xxxxxxxxxxx>
>> >> ---
>> >> 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@xxxxxxxxxxxxxxxxxxx
>> > 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@xxxxxxxxxxxxxxx
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/