[PATCH] arm64: dts: Add idle-states for Juno

[PATCH] arm64: dts: Add idle-states for Juno

[Jon Medhurst (Tixy)]

From: Jon Medhurst <tixy@linaro.org>

Signed-off-by: Jon Medhurst <tixy@linaro.org>
---

These have been kicking around out of tree for ages, any reason they
shouldn't be in mainline? Also, was unsure of what to put in commit
message.

 arch/arm64/boot/dts/arm/juno.dts | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
index 133ee59..7a9a449 100644
--- a/arch/arm64/boot/dts/arm/juno.dts
+++ b/arch/arm64/boot/dts/arm/juno.dts
@@ -34,12 +34,35 @@
 		#address-cells = <2>;
 		#size-cells = <0>;
 
+		idle-states {
+			entry-method = "arm,psci";
+
+			CPU_SLEEP_0: cpu-sleep-0 {
+				compatible = "arm,idle-state";
+				arm,psci-suspend-param = <0x0010000>;
+				local-timer-stop;
+				entry-latency-us = <100>;
+				exit-latency-us = <250>;
+				min-residency-us = <2000>;
+			};
+
+			CLUSTER_SLEEP_0: cluster-sleep-0 {
+				compatible = "arm,idle-state";
+				arm,psci-suspend-param = <0x1010000>;
+				local-timer-stop;
+				entry-latency-us = <800>;
+				exit-latency-us = <700>;
+				min-residency-us = <2500>;
+			};
+		};
+
 		A57_0: cpu at 0 {
 			compatible = "arm,cortex-a57","arm,armv8";
 			reg = <0x0 0x0>;
 			device_type = "cpu";
 			enable-method = "psci";
 			next-level-cache = <&A57_L2>;
+			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
 		};
 
 		A57_1: cpu at 1 {
@@ -48,6 +71,7 @@
 			device_type = "cpu";
 			enable-method = "psci";
 			next-level-cache = <&A57_L2>;
+			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
 		};
 
 		A53_0: cpu at 100 {
@@ -56,6 +80,7 @@
 			device_type = "cpu";
 			enable-method = "psci";
 			next-level-cache = <&A53_L2>;
+			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
 		};
 
 		A53_1: cpu at 101 {
@@ -64,6 +89,7 @@
 			device_type = "cpu";
 			enable-method = "psci";
 			next-level-cache = <&A53_L2>;
+			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
 		};
 
 		A53_2: cpu at 102 {
@@ -72,6 +98,7 @@
 			device_type = "cpu";
 			enable-method = "psci";
 			next-level-cache = <&A53_L2>;
+			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
 		};
 
 		A53_3: cpu at 103 {
@@ -80,6 +107,7 @@
 			device_type = "cpu";
 			enable-method = "psci";
 			next-level-cache = <&A53_L2>;
+			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
 		};
 
 		A57_L2: l2-cache0 {
-- 
2.1.4

[PATCH] arm64: dts: Add idle-states for Juno

[Liviu Dudau]

On Thu, Apr 30, 2015 at 02:57:48PM +0100, Jon Medhurst (Tixy) wrote:
> From: Jon Medhurst <tixy@linaro.org>
> 
> Signed-off-by: Jon Medhurst <tixy@linaro.org>
> ---
> 
> These have been kicking around out of tree for ages, any reason they
> shouldn't be in mainline? Also, was unsure of what to put in commit
> message.

Hi Tixy,

Re: commit message: maybe a note on the firmware version and kernel
version where this has been tested on?

Best regards,
Liviu

> 
>  arch/arm64/boot/dts/arm/juno.dts | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
> index 133ee59..7a9a449 100644
> --- a/arch/arm64/boot/dts/arm/juno.dts
> +++ b/arch/arm64/boot/dts/arm/juno.dts
> @@ -34,12 +34,35 @@
>  		#address-cells = <2>;
>  		#size-cells = <0>;
>  
> +		idle-states {
> +			entry-method = "arm,psci";
> +
> +			CPU_SLEEP_0: cpu-sleep-0 {
> +				compatible = "arm,idle-state";
> +				arm,psci-suspend-param = <0x0010000>;
> +				local-timer-stop;
> +				entry-latency-us = <100>;
> +				exit-latency-us = <250>;
> +				min-residency-us = <2000>;
> +			};
> +
> +			CLUSTER_SLEEP_0: cluster-sleep-0 {
> +				compatible = "arm,idle-state";
> +				arm,psci-suspend-param = <0x1010000>;
> +				local-timer-stop;
> +				entry-latency-us = <800>;
> +				exit-latency-us = <700>;
> +				min-residency-us = <2500>;
> +			};
> +		};
> +
>  		A57_0: cpu at 0 {
>  			compatible = "arm,cortex-a57","arm,armv8";
>  			reg = <0x0 0x0>;
>  			device_type = "cpu";
>  			enable-method = "psci";
>  			next-level-cache = <&A57_L2>;
> +			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
>  		};
>  
>  		A57_1: cpu at 1 {
> @@ -48,6 +71,7 @@
>  			device_type = "cpu";
>  			enable-method = "psci";
>  			next-level-cache = <&A57_L2>;
> +			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
>  		};
>  
>  		A53_0: cpu at 100 {
> @@ -56,6 +80,7 @@
>  			device_type = "cpu";
>  			enable-method = "psci";
>  			next-level-cache = <&A53_L2>;
> +			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
>  		};
>  
>  		A53_1: cpu at 101 {
> @@ -64,6 +89,7 @@
>  			device_type = "cpu";
>  			enable-method = "psci";
>  			next-level-cache = <&A53_L2>;
> +			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
>  		};
>  
>  		A53_2: cpu at 102 {
> @@ -72,6 +98,7 @@
>  			device_type = "cpu";
>  			enable-method = "psci";
>  			next-level-cache = <&A53_L2>;
> +			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
>  		};
>  
>  		A53_3: cpu at 103 {
> @@ -80,6 +107,7 @@
>  			device_type = "cpu";
>  			enable-method = "psci";
>  			next-level-cache = <&A53_L2>;
> +			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
>  		};
>  
>  		A57_L2: l2-cache0 {
> -- 
> 2.1.4
> 
> 
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ?\_(?)_/?

[PATCH] arm64: dts: Add idle-states for Juno

[Sudeep Holla]

On 30/04/15 14:57, Jon Medhurst (Tixy) wrote:
> From: Jon Medhurst <tixy@linaro.org>
>
> Signed-off-by: Jon Medhurst <tixy@linaro.org>
> ---
>
> These have been kicking around out of tree for ages, any reason they
> shouldn't be in mainline?

One possible reason could be that these values are not tuned(e.g.
latency values, can they be same for both clusters ?) Though these
reasons are not blocking and this patch will not cause any
functionality break even if is merged as is.

Regards,
Sudeep

[PATCH] arm64: dts: Add idle-states for Juno

[Jon Medhurst (Tixy)]

On Thu, 2015-04-30 at 16:42 +0100, Liviu Dudau wrote:
> Re: commit message: maybe a note on the firmware version and kernel
> version where this has been tested on?

Sure, I'll update it with that. I.e. ...

Tested on Linux 4.1-rc1 using firmware from version 0.10.1 of the
Juno board recovery image:
https://releases.linaro.org/14.12/members/arm/android/images/armv8-android-juno-lsk/board_recovery_image-0.10.1-linaro1.tar.bz2

-- 
Tixy

[PATCH] arm64: dts: Add idle-states for Juno

[Jon Medhurst (Tixy)]

On Thu, 2015-04-30 at 17:00 +0100, Sudeep Holla wrote:
> 
> On 30/04/15 14:57, Jon Medhurst (Tixy) wrote:
> > From: Jon Medhurst <tixy@linaro.org>
> >
> > Signed-off-by: Jon Medhurst <tixy@linaro.org>
> > ---
> >
> > These have been kicking around out of tree for ages, any reason they
> > shouldn't be in mainline?
> 
> One possible reason could be that these values are not tuned(e.g.
> latency values, can they be same for both clusters ?)

I thought that both clusters being the same was questionable.

>  Though these
> reasons are not blocking and this patch will not cause any
> functionality break even if is merged as is.

My main purpose with trying to get this merged is so that people using
Juno for general testing and validation will actually have cpuidle
running and so potentially find more bugs.

If we wait until ARM have finished tweaking their firmware and tuning
benchmark runs to get 'optimum' values for their main usecases then I
suspect we'll be waiting a long time ;-)

-- 
Tixy

[PATCH] arm64: dts: Add idle-states for Juno

[Sudeep Holla]

On 30/04/15 17:40, Jon Medhurst (Tixy) wrote:
> On Thu, 2015-04-30 at 17:00 +0100, Sudeep Holla wrote:
>>
>> On 30/04/15 14:57, Jon Medhurst (Tixy) wrote:
>>> From: Jon Medhurst <tixy@linaro.org>
>>>
>>> Signed-off-by: Jon Medhurst <tixy@linaro.org>
>>> ---
>>>
>>> These have been kicking around out of tree for ages, any reason they
>>> shouldn't be in mainline?
>>
>> One possible reason could be that these values are not tuned(e.g.
>> latency values, can they be same for both clusters ?)
>
> I thought that both clusters being the same was questionable.
>
>>   Though these
>> reasons are not blocking and this patch will not cause any
>> functionality break even if is merged as is.
>
> My main purpose with trying to get this merged is so that people using
> Juno for general testing and validation will actually have cpuidle
> running and so potentially find more bugs.
>
> If we wait until ARM have finished tweaking their firmware and tuning
> benchmark runs to get 'optimum' values for their main usecases then I
> suspect we'll be waiting a long time ;-)
>

Agreed, I am not blocking the patch, it can be merged in its current
state. Since we had always seen stability issues with firmware until
recently, we were hesitant to push this change. Now that we have
established that it's stable enough, it's ready to go ;)

Regards,
Sudeep

[PATCH] arm64: dts: Add idle-states for Juno

[Lorenzo Pieralisi]

On Thu, Apr 30, 2015 at 05:40:35PM +0100, Jon Medhurst (Tixy) wrote:
> On Thu, 2015-04-30 at 17:00 +0100, Sudeep Holla wrote:
> > 
> > On 30/04/15 14:57, Jon Medhurst (Tixy) wrote:
> > > From: Jon Medhurst <tixy@linaro.org>
> > >
> > > Signed-off-by: Jon Medhurst <tixy@linaro.org>
> > > ---
> > >
> > > These have been kicking around out of tree for ages, any reason they
> > > shouldn't be in mainline?
> > 
> > One possible reason could be that these values are not tuned(e.g.
> > latency values, can they be same for both clusters ?)
> 
> I thought that both clusters being the same was questionable.
> 
> >  Though these
> > reasons are not blocking and this patch will not cause any
> > functionality break even if is merged as is.
> 
> My main purpose with trying to get this merged is so that people using
> Juno for general testing and validation will actually have cpuidle
> running and so potentially find more bugs.

I am reluctant to enable idle states in the default Juno dts, they
will affect latencies and performance tests significantly. I should
find a way to disable them by default and possibly have a DT property
to enabled them explicitly, we can't merge the dts as it is we have
to change the CPUidle code first.

Lorenzo

> 
> If we wait until ARM have finished tweaking their firmware and tuning
> benchmark runs to get 'optimum' values for their main usecases then I
> suspect we'll be waiting a long time ;-)
> 
> -- 
> Tixy
> 

[PATCH] arm64: dts: Add idle-states for Juno

[Jon Medhurst (Tixy)]

On Thu, 2015-04-30 at 18:17 +0100, Lorenzo Pieralisi wrote:
> On Thu, Apr 30, 2015 at 05:40:35PM +0100, Jon Medhurst (Tixy) wrote:
> > On Thu, 2015-04-30 at 17:00 +0100, Sudeep Holla wrote:
> > > 
> > > On 30/04/15 14:57, Jon Medhurst (Tixy) wrote:
> > > > From: Jon Medhurst <tixy@linaro.org>
> > > >
> > > > Signed-off-by: Jon Medhurst <tixy@linaro.org>
> > > > ---
> > > >
> > > > These have been kicking around out of tree for ages, any reason they
> > > > shouldn't be in mainline?
> > > 
> > > One possible reason could be that these values are not tuned(e.g.
> > > latency values, can they be same for both clusters ?)
> > 
> > I thought that both clusters being the same was questionable.
> > 
> > >  Though these
> > > reasons are not blocking and this patch will not cause any
> > > functionality break even if is merged as is.
> > 
> > My main purpose with trying to get this merged is so that people using
> > Juno for general testing and validation will actually have cpuidle
> > running and so potentially find more bugs.
> 
> I am reluctant to enable idle states in the default Juno dts, they
> will affect latencies and performance tests significantly. I should
> find a way to disable them by default and possibly have a DT property
> to enabled them explicitly, we can't merge the dts as it is we have
> to change the CPUidle code first.

Presumably the same argument goes for cpufreq? That'll upset people
doing performance benchmarks, so shouldn't be enabled by default?

Anyway, I'll just step back and let you ARM guys decide what and when
any upstream kernel support should be merged.

-- 
Tixy

[PATCH] arm64: dts: Add idle-states for Juno

[Sudeep Holla]

On 30/04/15 18:36, Jon Medhurst (Tixy) wrote:
> On Thu, 2015-04-30 at 18:17 +0100, Lorenzo Pieralisi wrote:
>> On Thu, Apr 30, 2015 at 05:40:35PM +0100, Jon Medhurst (Tixy) wrote:
>>> On Thu, 2015-04-30 at 17:00 +0100, Sudeep Holla wrote:
>>>>
>>>> On 30/04/15 14:57, Jon Medhurst (Tixy) wrote:
>>>>> From: Jon Medhurst <tixy@linaro.org>
>>>>>
>>>>> Signed-off-by: Jon Medhurst <tixy@linaro.org>
>>>>> ---
>>>>>
>>>>> These have been kicking around out of tree for ages, any reason they
>>>>> shouldn't be in mainline?
>>>>
>>>> One possible reason could be that these values are not tuned(e.g.
>>>> latency values, can they be same for both clusters ?)
>>>
>>> I thought that both clusters being the same was questionable.
>>>
>>>>   Though these
>>>> reasons are not blocking and this patch will not cause any
>>>> functionality break even if is merged as is.
>>>
>>> My main purpose with trying to get this merged is so that people using
>>> Juno for general testing and validation will actually have cpuidle
>>> running and so potentially find more bugs.
>>
>> I am reluctant to enable idle states in the default Juno dts, they
>> will affect latencies and performance tests significantly. I should
>> find a way to disable them by default and possibly have a DT property
>> to enabled them explicitly, we can't merge the dts as it is we have
>> to change the CPUidle code first.
>
> Presumably the same argument goes for cpufreq? That'll upset people
> doing performance benchmarks, so shouldn't be enabled by default?
>

Or we can have cpufreq enabled with performance governor as default ;)

Regards,
Sudeep

[PATCH] arm64: dts: Add idle-states for Juno

[Leo Yan]

On Thu, Apr 30, 2015 at 02:57:48PM +0100, Jon Medhurst (Tixy) wrote:
> From: Jon Medhurst <tixy@linaro.org>
> 
> Signed-off-by: Jon Medhurst <tixy@linaro.org>
> ---
> 
> These have been kicking around out of tree for ages, any reason they
> shouldn't be in mainline? Also, was unsure of what to put in commit
> message.
> 
>  arch/arm64/boot/dts/arm/juno.dts | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
> index 133ee59..7a9a449 100644
> --- a/arch/arm64/boot/dts/arm/juno.dts
> +++ b/arch/arm64/boot/dts/arm/juno.dts
> @@ -34,12 +34,35 @@
>  		#address-cells = <2>;
>  		#size-cells = <0>;
>  
> +		idle-states {
> +			entry-method = "arm,psci";
> +
> +			CPU_SLEEP_0: cpu-sleep-0 {
> +				compatible = "arm,idle-state";
> +				arm,psci-suspend-param = <0x0010000>;
> +				local-timer-stop;

Just want to figure out the best way for big.LITTLE system; so have
one question: CA53 and CA57 have different power domain for arch
timer, right? If this is the case, should we define two kinds of cpu
sleep states, one of them will not migrate to broadcast timer and
keep using arch timer after cpu has been powered down?

> +				entry-latency-us = <100>;
> +				exit-latency-us = <250>;
> +				min-residency-us = <2000>;
> +			};
> +
> +			CLUSTER_SLEEP_0: cluster-sleep-0 {
> +				compatible = "arm,idle-state";
> +				arm,psci-suspend-param = <0x1010000>;
> +				local-timer-stop;
> +				entry-latency-us = <800>;
> +				exit-latency-us = <700>;
> +				min-residency-us = <2500>;
> +			};
> +		};
> +
>  		A57_0: cpu at 0 {
>  			compatible = "arm,cortex-a57","arm,armv8";
>  			reg = <0x0 0x0>;
>  			device_type = "cpu";
>  			enable-method = "psci";
>  			next-level-cache = <&A57_L2>;
> +			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
>  		};
>  
>  		A57_1: cpu at 1 {
> @@ -48,6 +71,7 @@
>  			device_type = "cpu";
>  			enable-method = "psci";
>  			next-level-cache = <&A57_L2>;
> +			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
>  		};
>  
>  		A53_0: cpu at 100 {
> @@ -56,6 +80,7 @@
>  			device_type = "cpu";
>  			enable-method = "psci";
>  			next-level-cache = <&A53_L2>;
> +			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
>  		};
>  
>  		A53_1: cpu at 101 {
> @@ -64,6 +89,7 @@
>  			device_type = "cpu";
>  			enable-method = "psci";
>  			next-level-cache = <&A53_L2>;
> +			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
>  		};
>  
>  		A53_2: cpu at 102 {
> @@ -72,6 +98,7 @@
>  			device_type = "cpu";
>  			enable-method = "psci";
>  			next-level-cache = <&A53_L2>;
> +			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
>  		};
>  
>  		A53_3: cpu at 103 {
> @@ -80,6 +107,7 @@
>  			device_type = "cpu";
>  			enable-method = "psci";
>  			next-level-cache = <&A53_L2>;
> +			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
>  		};
>  
>  		A57_L2: l2-cache0 {

[PATCH] arm64: dts: Add idle-states for Juno

[Jon Medhurst (Tixy)]

On Thu, 2015-04-30 at 18:40 +0100, Sudeep Holla wrote:
> 
> On 30/04/15 18:36, Jon Medhurst (Tixy) wrote:
> > On Thu, 2015-04-30 at 18:17 +0100, Lorenzo Pieralisi wrote:
> >> On Thu, Apr 30, 2015 at 05:40:35PM +0100, Jon Medhurst (Tixy) wrote:
> >>> On Thu, 2015-04-30 at 17:00 +0100, Sudeep Holla wrote:
> >>>>
> >>>> On 30/04/15 14:57, Jon Medhurst (Tixy) wrote:
> >>>>> From: Jon Medhurst <tixy@linaro.org>
> >>>>>
> >>>>> Signed-off-by: Jon Medhurst <tixy@linaro.org>
> >>>>> ---
> >>>>>
> >>>>> These have been kicking around out of tree for ages, any reason they
> >>>>> shouldn't be in mainline?
> >>>>
> >>>> One possible reason could be that these values are not tuned(e.g.
> >>>> latency values, can they be same for both clusters ?)
> >>>
> >>> I thought that both clusters being the same was questionable.
> >>>
> >>>>   Though these
> >>>> reasons are not blocking and this patch will not cause any
> >>>> functionality break even if is merged as is.
> >>>
> >>> My main purpose with trying to get this merged is so that people using
> >>> Juno for general testing and validation will actually have cpuidle
> >>> running and so potentially find more bugs.
> >>
> >> I am reluctant to enable idle states in the default Juno dts, they
> >> will affect latencies and performance tests significantly. I should
> >> find a way to disable them by default and possibly have a DT property
> >> to enabled them explicitly, we can't merge the dts as it is we have
> >> to change the CPUidle code first.
> >
> > Presumably the same argument goes for cpufreq? That'll upset people
> > doing performance benchmarks, so shouldn't be enabled by default?
> >
> 
> Or we can have cpufreq enabled with performance governor as default ;)

Which will crank the cpu clock up to max (1.1GHz on the A57s), which is
higher than what you get without cpufreq in the kernel (850MHz) so
everyone's performance measurements will suddenly go faster.

I don't know if that is a problem for the usecase's Lorenzo wants the
default Juno setup to satisfy.

-- 
Tixy

[PATCH] arm64: dts: Add idle-states for Juno

[Catalin Marinas]

On Thu, Apr 30, 2015 at 06:17:01PM +0100, Lorenzo Pieralisi wrote:
> On Thu, Apr 30, 2015 at 05:40:35PM +0100, Jon Medhurst (Tixy) wrote:
> > On Thu, 2015-04-30 at 17:00 +0100, Sudeep Holla wrote:
> > > On 30/04/15 14:57, Jon Medhurst (Tixy) wrote:
> > > > From: Jon Medhurst <tixy@linaro.org>
> > > >
> > > > Signed-off-by: Jon Medhurst <tixy@linaro.org>
> > > > ---
> > > >
> > > > These have been kicking around out of tree for ages, any reason they
> > > > shouldn't be in mainline?
> > > 
> > > One possible reason could be that these values are not tuned(e.g.
> > > latency values, can they be same for both clusters ?)
> > 
> > I thought that both clusters being the same was questionable.
> > 
> > >  Though these
> > > reasons are not blocking and this patch will not cause any
> > > functionality break even if is merged as is.
> > 
> > My main purpose with trying to get this merged is so that people using
> > Juno for general testing and validation will actually have cpuidle
> > running and so potentially find more bugs.
> 
> I am reluctant to enable idle states in the default Juno dts, they
> will affect latencies and performance tests significantly.

OTOH, I guess they will improve the power benchmarks. IMO, we should
place in the DT whatever the hardware and firmware supports. It's up to
those doing benchmarks to disable CPU suspend.

-- 
Catalin

[PATCH] arm64: dts: Add idle-states for Juno

[Lorenzo Pieralisi]

On Fri, May 01, 2015 at 10:02:02AM +0100, Catalin Marinas wrote:
> On Thu, Apr 30, 2015 at 06:17:01PM +0100, Lorenzo Pieralisi wrote:
> > On Thu, Apr 30, 2015 at 05:40:35PM +0100, Jon Medhurst (Tixy) wrote:
> > > On Thu, 2015-04-30 at 17:00 +0100, Sudeep Holla wrote:
> > > > On 30/04/15 14:57, Jon Medhurst (Tixy) wrote:
> > > > > From: Jon Medhurst <tixy@linaro.org>
> > > > >
> > > > > Signed-off-by: Jon Medhurst <tixy@linaro.org>
> > > > > ---
> > > > >
> > > > > These have been kicking around out of tree for ages, any reason they
> > > > > shouldn't be in mainline?
> > > > 
> > > > One possible reason could be that these values are not tuned(e.g.
> > > > latency values, can they be same for both clusters ?)
> > > 
> > > I thought that both clusters being the same was questionable.
> > > 
> > > >  Though these
> > > > reasons are not blocking and this patch will not cause any
> > > > functionality break even if is merged as is.
> > > 
> > > My main purpose with trying to get this merged is so that people using
> > > Juno for general testing and validation will actually have cpuidle
> > > running and so potentially find more bugs.
> > 
> > I am reluctant to enable idle states in the default Juno dts, they
> > will affect latencies and performance tests significantly.
> 
> OTOH, I guess they will improve the power benchmarks. IMO, we should
> place in the DT whatever the hardware and firmware supports. It's up to
> those doing benchmarks to disable CPU suspend.

I am ok with DT defining whatever HW and FW support, the question is
whether we want CPUidle (and CPUfreq) enabled by default in the
defconfig then.

This will certainly trigger mainline regressions from a latency/performance
standpoint (true, some tests disable idle states by default ie cyclic
test), unless we disable CPUidle via command line parameter or we
force people carrying out tests to disable idle states through sysfs knobs.

Lorenzo

[PATCH] arm64: dts: Add idle-states for Juno

[Liviu Dudau]

On Fri, May 01, 2015 at 11:12:11AM +0100, Lorenzo Pieralisi wrote:
> On Fri, May 01, 2015 at 10:02:02AM +0100, Catalin Marinas wrote:
> > On Thu, Apr 30, 2015 at 06:17:01PM +0100, Lorenzo Pieralisi wrote:
> > > On Thu, Apr 30, 2015 at 05:40:35PM +0100, Jon Medhurst (Tixy) wrote:
> > > > On Thu, 2015-04-30 at 17:00 +0100, Sudeep Holla wrote:
> > > > > On 30/04/15 14:57, Jon Medhurst (Tixy) wrote:
> > > > > > From: Jon Medhurst <tixy@linaro.org>
> > > > > >
> > > > > > Signed-off-by: Jon Medhurst <tixy@linaro.org>
> > > > > > ---
> > > > > >
> > > > > > These have been kicking around out of tree for ages, any reason they
> > > > > > shouldn't be in mainline?
> > > > > 
> > > > > One possible reason could be that these values are not tuned(e.g.
> > > > > latency values, can they be same for both clusters ?)
> > > > 
> > > > I thought that both clusters being the same was questionable.
> > > > 
> > > > >  Though these
> > > > > reasons are not blocking and this patch will not cause any
> > > > > functionality break even if is merged as is.
> > > > 
> > > > My main purpose with trying to get this merged is so that people using
> > > > Juno for general testing and validation will actually have cpuidle
> > > > running and so potentially find more bugs.
> > > 
> > > I am reluctant to enable idle states in the default Juno dts, they
> > > will affect latencies and performance tests significantly.
> > 
> > OTOH, I guess they will improve the power benchmarks. IMO, we should
> > place in the DT whatever the hardware and firmware supports. It's up to
> > those doing benchmarks to disable CPU suspend.
> 
> I am ok with DT defining whatever HW and FW support, the question is
> whether we want CPUidle (and CPUfreq) enabled by default in the
> defconfig then.
> 
> This will certainly trigger mainline regressions from a latency/performance
> standpoint (true, some tests disable idle states by default ie cyclic
> test), unless we disable CPUidle via command line parameter or we
> force people carrying out tests to disable idle states through sysfs knobs.

Hi Lorenzo,

Do you have any specific tests in mind that you are afraid of breaking? Would
enabling the performance CPUfreq driver by default not be enough to cover those
tests, like Sudeep suggested?

Best regards,
Liviu

> 
> Lorenzo
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ?\_(?)_/?

[PATCH] arm64: dts: Add idle-states for Juno

[Jon Medhurst (Tixy)]

On Fri, 2015-05-01 at 09:55 +0800, Leo Yan wrote:
[...]
> >  arch/arm64/boot/dts/arm/juno.dts | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
> > index 133ee59..7a9a449 100644
> > --- a/arch/arm64/boot/dts/arm/juno.dts
> > +++ b/arch/arm64/boot/dts/arm/juno.dts
> > @@ -34,12 +34,35 @@
> >  		#address-cells = <2>;
> >  		#size-cells = <0>;
> >  
> > +		idle-states {
> > +			entry-method = "arm,psci";
> > +
> > +			CPU_SLEEP_0: cpu-sleep-0 {
> > +				compatible = "arm,idle-state";
> > +				arm,psci-suspend-param = <0x0010000>;
> > +				local-timer-stop;
> 
> Just want to figure out the best way for big.LITTLE system; so have
> one question: CA53 and CA57 have different power domain for arch
> timer, right?

I'm not sure of the answer to that. The documentation I have does seem
to state the timer is lost on cluster power down, which would imply that
it's not when just powering down a cpu, but I'm not at all clear on the
matter.

>  If this is the case, should we define two kinds of cpu
> sleep states, one of them will not migrate to broadcast timer and
> keep using arch timer after cpu has been powered down?

Do you mean that if the local timer is not lost (and so we should not
have local-timer-stop above), then we should have another identical idle
state except that it _does_ specify local-timer-stop to force the
broadcast time to be used? If so, would that second state ever be more
power efficient than the first? (I don't know the answer to that, this
whole area is pretty new to me).

-- 
Tixy

[PATCH] arm64: dts: Add idle-states for Juno

[Lorenzo Pieralisi]

On Fri, May 01, 2015 at 11:22:31AM +0100, Jon Medhurst (Tixy) wrote:
> On Fri, 2015-05-01 at 09:55 +0800, Leo Yan wrote:
> [...]
> > >  arch/arm64/boot/dts/arm/juno.dts | 28 ++++++++++++++++++++++++++++
> > >  1 file changed, 28 insertions(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
> > > index 133ee59..7a9a449 100644
> > > --- a/arch/arm64/boot/dts/arm/juno.dts
> > > +++ b/arch/arm64/boot/dts/arm/juno.dts
> > > @@ -34,12 +34,35 @@
> > >  		#address-cells = <2>;
> > >  		#size-cells = <0>;
> > >  
> > > +		idle-states {
> > > +			entry-method = "arm,psci";
> > > +
> > > +			CPU_SLEEP_0: cpu-sleep-0 {
> > > +				compatible = "arm,idle-state";
> > > +				arm,psci-suspend-param = <0x0010000>;
> > > +				local-timer-stop;
> > 
> > Just want to figure out the best way for big.LITTLE system; so have
> > one question: CA53 and CA57 have different power domain for arch
> > timer, right?
> 
> I'm not sure of the answer to that. The documentation I have does seem
> to state the timer is lost on cluster power down, which would imply that
> it's not when just powering down a cpu, but I'm not at all clear on the
> matter.
> 
> >  If this is the case, should we define two kinds of cpu
> > sleep states, one of them will not migrate to broadcast timer and
> > keep using arch timer after cpu has been powered down?
> 
> Do you mean that if the local timer is not lost (and so we should not
> have local-timer-stop above), then we should have another identical idle
> state except that it _does_ specify local-timer-stop to force the
> broadcast time to be used? If so, would that second state ever be more
> power efficient than the first? (I don't know the answer to that, this
> whole area is pretty new to me).

Ok, to start with, if we want the generic CPUidle driver to work on bL
systems, we should get this patch merged:

http://www.spinics.net/lists/arm-kernel/msg412790.html

Architected timer is always lost on power down, unless we are talking
about a logic retention state (or if we are talking about local timers
that are not the architected ones). Anyway, with the patch above, different
cpus can have different idle states, so different behaviours when it
comes to the local timer behaviour. If, say, on a A53/A57 system, A53
cpus lose the local timer on idle state entry and A57 cpus do not
(that's a non-existing example though), those cpus will have different
idle states so different local-timer-stop flags.

I hope this helps.

Lorenzo

[PATCH] arm64: dts: Add idle-states for Juno

[Lorenzo Pieralisi]

On Fri, May 01, 2015 at 11:22:02AM +0100, Liviu Dudau wrote:
> On Fri, May 01, 2015 at 11:12:11AM +0100, Lorenzo Pieralisi wrote:
> > On Fri, May 01, 2015 at 10:02:02AM +0100, Catalin Marinas wrote:
> > > On Thu, Apr 30, 2015 at 06:17:01PM +0100, Lorenzo Pieralisi wrote:
> > > > On Thu, Apr 30, 2015 at 05:40:35PM +0100, Jon Medhurst (Tixy) wrote:
> > > > > On Thu, 2015-04-30 at 17:00 +0100, Sudeep Holla wrote:
> > > > > > On 30/04/15 14:57, Jon Medhurst (Tixy) wrote:
> > > > > > > From: Jon Medhurst <tixy@linaro.org>
> > > > > > >
> > > > > > > Signed-off-by: Jon Medhurst <tixy@linaro.org>
> > > > > > > ---
> > > > > > >
> > > > > > > These have been kicking around out of tree for ages, any reason they
> > > > > > > shouldn't be in mainline?
> > > > > > 
> > > > > > One possible reason could be that these values are not tuned(e.g.
> > > > > > latency values, can they be same for both clusters ?)
> > > > > 
> > > > > I thought that both clusters being the same was questionable.
> > > > > 
> > > > > >  Though these
> > > > > > reasons are not blocking and this patch will not cause any
> > > > > > functionality break even if is merged as is.
> > > > > 
> > > > > My main purpose with trying to get this merged is so that people using
> > > > > Juno for general testing and validation will actually have cpuidle
> > > > > running and so potentially find more bugs.
> > > > 
> > > > I am reluctant to enable idle states in the default Juno dts, they
> > > > will affect latencies and performance tests significantly.
> > > 
> > > OTOH, I guess they will improve the power benchmarks. IMO, we should
> > > place in the DT whatever the hardware and firmware supports. It's up to
> > > those doing benchmarks to disable CPU suspend.
> > 
> > I am ok with DT defining whatever HW and FW support, the question is
> > whether we want CPUidle (and CPUfreq) enabled by default in the
> > defconfig then.
> > 
> > This will certainly trigger mainline regressions from a latency/performance
> > standpoint (true, some tests disable idle states by default ie cyclic
> > test), unless we disable CPUidle via command line parameter or we
> > force people carrying out tests to disable idle states through sysfs knobs.
> 
> Hi Lorenzo,
> 
> Do you have any specific tests in mind that you are afraid of breaking? Would
> enabling the performance CPUfreq driver by default not be enough to cover those
> tests, like Sudeep suggested?

We are talking about separate things here. Every test/benchmark affected by
latency will regress if we enable idle states, regardless of CPUfreq.

If we agree that it is left to people carrying out performance tests to
disable those features in the kernel, that's fine by me, I am pretty sure it
won't take us long to notice by the time we merge this patch in the
mainline, I just wanted to avoid forcing features upon users who may not
find energy savings a useful feature, that's it.

I will review the idle states dts content then.

Lorenzo

> Best regards,
> Liviu
> 
> > 
> > Lorenzo
> > 
> 
> -- 
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ??\_(???)_/??

[PATCH] arm64: dts: Add idle-states for Juno

[Leo Yan]

On Fri, May 01, 2015 at 11:22:31AM +0100, Jon Medhurst (Tixy) wrote:
> On Fri, 2015-05-01 at 09:55 +0800, Leo Yan wrote:
> [...]
> > >  arch/arm64/boot/dts/arm/juno.dts | 28 ++++++++++++++++++++++++++++
> > >  1 file changed, 28 insertions(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
> > > index 133ee59..7a9a449 100644
> > > --- a/arch/arm64/boot/dts/arm/juno.dts
> > > +++ b/arch/arm64/boot/dts/arm/juno.dts
> > > @@ -34,12 +34,35 @@
> > >  		#address-cells = <2>;
> > >  		#size-cells = <0>;
> > >  
> > > +		idle-states {
> > > +			entry-method = "arm,psci";
> > > +
> > > +			CPU_SLEEP_0: cpu-sleep-0 {
> > > +				compatible = "arm,idle-state";
> > > +				arm,psci-suspend-param = <0x0010000>;
> > > +				local-timer-stop;
> > 
> > Just want to figure out the best way for big.LITTLE system; so have
> > one question: CA53 and CA57 have different power domain for arch
> > timer, right?
> 
> I'm not sure of the answer to that. The documentation I have does seem
> to state the timer is lost on cluster power down, which would imply that
> it's not when just powering down a cpu, but I'm not at all clear on the
> matter.
> 
> >  If this is the case, should we define two kinds of cpu
> > sleep states, one of them will not migrate to broadcast timer and
> > keep using arch timer after cpu has been powered down?
> 
> Do you mean that if the local timer is not lost (and so we should not
> have local-timer-stop above), then we should have another identical idle
> state except that it _does_ specify local-timer-stop to force the
> broadcast time to be used? If so, would that second state ever be more
> power efficient than the first? (I don't know the answer to that, this
> whole area is pretty new to me).

Yes, the second state will have better power efficient. But if the
arch timer can keep working after the cpu is powered off, we also
can get benefit for cpuidle's latency, this is because s/w don't need
migrate the cpu timers to broad cast timer list anymore.

Thanks,
Leo Yan

[PATCH] arm64: dts: Add idle-states for Juno

[Leo Yan]

On Fri, May 01, 2015 at 11:45:06AM +0100, Lorenzo Pieralisi wrote:
> On Fri, May 01, 2015 at 11:22:31AM +0100, Jon Medhurst (Tixy) wrote:
> > On Fri, 2015-05-01 at 09:55 +0800, Leo Yan wrote:
> > [...]
> > > >  arch/arm64/boot/dts/arm/juno.dts | 28 ++++++++++++++++++++++++++++
> > > >  1 file changed, 28 insertions(+)
> > > > 
> > > > diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
> > > > index 133ee59..7a9a449 100644
> > > > --- a/arch/arm64/boot/dts/arm/juno.dts
> > > > +++ b/arch/arm64/boot/dts/arm/juno.dts
> > > > @@ -34,12 +34,35 @@
> > > >  		#address-cells = <2>;
> > > >  		#size-cells = <0>;
> > > >  
> > > > +		idle-states {
> > > > +			entry-method = "arm,psci";
> > > > +
> > > > +			CPU_SLEEP_0: cpu-sleep-0 {
> > > > +				compatible = "arm,idle-state";
> > > > +				arm,psci-suspend-param = <0x0010000>;
> > > > +				local-timer-stop;
> > > 
> > > Just want to figure out the best way for big.LITTLE system; so have
> > > one question: CA53 and CA57 have different power domain for arch
> > > timer, right?
> > 
> > I'm not sure of the answer to that. The documentation I have does seem
> > to state the timer is lost on cluster power down, which would imply that
> > it's not when just powering down a cpu, but I'm not at all clear on the
> > matter.
> > 
> > >  If this is the case, should we define two kinds of cpu
> > > sleep states, one of them will not migrate to broadcast timer and
> > > keep using arch timer after cpu has been powered down?
> > 
> > Do you mean that if the local timer is not lost (and so we should not
> > have local-timer-stop above), then we should have another identical idle
> > state except that it _does_ specify local-timer-stop to force the
> > broadcast time to be used? If so, would that second state ever be more
> > power efficient than the first? (I don't know the answer to that, this
> > whole area is pretty new to me).
> 
> Ok, to start with, if we want the generic CPUidle driver to work on bL
> systems, we should get this patch merged:
> 
> http://www.spinics.net/lists/arm-kernel/msg412790.html

Thanks pointing out this.

> Architected timer is always lost on power down, unless we are talking
> about a logic retention state (or if we are talking about local timers
> that are not the architected ones). Anyway, with the patch above, different
> cpus can have different idle states, so different behaviours when it
> comes to the local timer behaviour. If, say, on a A53/A57 system, A53
> cpus lose the local timer on idle state entry and A57 cpus do not
> (that's a non-existing example though), those cpus will have different
> idle states so different local-timer-stop flags.

For A57, not sure if arch timer will be powered off if cpu has been
powered off.

>From A53's TRM figure 2-1 Cortex-A53 processor block diagram, the arch
timer is not within CPU power domain, so likey arch timer still will
work even cpu has been powered off (PDCPU/PDADVSIMD both have been off).
This looks like is not aligning w/t your upper description. Could u
confirm this understanding is right?

Thanks,
Leo Yan

[PATCH] arm64: dts: Add idle-states for Juno

[Catalin Marinas]

On Fri, May 01, 2015 at 11:12:11AM +0100, Lorenzo Pieralisi wrote:
> On Fri, May 01, 2015 at 10:02:02AM +0100, Catalin Marinas wrote:
> > On Thu, Apr 30, 2015 at 06:17:01PM +0100, Lorenzo Pieralisi wrote:
> > > On Thu, Apr 30, 2015 at 05:40:35PM +0100, Jon Medhurst (Tixy) wrote:
> > > > On Thu, 2015-04-30 at 17:00 +0100, Sudeep Holla wrote:
> > > > > On 30/04/15 14:57, Jon Medhurst (Tixy) wrote:
> > > > > > From: Jon Medhurst <tixy@linaro.org>
> > > > > >
> > > > > > Signed-off-by: Jon Medhurst <tixy@linaro.org>
> > > > > > ---
> > > > > >
> > > > > > These have been kicking around out of tree for ages, any reason they
> > > > > > shouldn't be in mainline?
> > > > > 
> > > > > One possible reason could be that these values are not tuned(e.g.
> > > > > latency values, can they be same for both clusters ?)
> > > > 
> > > > I thought that both clusters being the same was questionable.
> > > > 
> > > > >  Though these
> > > > > reasons are not blocking and this patch will not cause any
> > > > > functionality break even if is merged as is.
> > > > 
> > > > My main purpose with trying to get this merged is so that people using
> > > > Juno for general testing and validation will actually have cpuidle
> > > > running and so potentially find more bugs.
> > > 
> > > I am reluctant to enable idle states in the default Juno dts, they
> > > will affect latencies and performance tests significantly.
> > 
> > OTOH, I guess they will improve the power benchmarks. IMO, we should
> > place in the DT whatever the hardware and firmware supports. It's up to
> > those doing benchmarks to disable CPU suspend.
> 
> I am ok with DT defining whatever HW and FW support, the question is
> whether we want CPUidle (and CPUfreq) enabled by default in the
> defconfig then.

It's functionality we support, so I think it should be enabled in
defconfig. This config is meant as a point of reference for what's
supported by the arm64 kernel and not tuned for some specific
benchmarks.

> This will certainly trigger mainline regressions from a latency/performance
> standpoint (true, some tests disable idle states by default ie cyclic
> test), unless we disable CPUidle via command line parameter or we
> force people carrying out tests to disable idle states through sysfs knobs.

It may affect performance for specific benchmarks but what if others
test the power consumption?

BTW, cannot the cpuidle governor be made smart enough (unless it is
already) to figure out when the next timer is going to happen so that
the CPU doesn't go in a deep sleep state with a high latency?

-- 
Catalin

[PATCH] arm64: dts: Add idle-states for Juno

[Liviu Dudau]

On Fri, May 01, 2015 at 12:55:07PM +0100, Leo Yan wrote:
> On Fri, May 01, 2015 at 11:45:06AM +0100, Lorenzo Pieralisi wrote:
> > On Fri, May 01, 2015 at 11:22:31AM +0100, Jon Medhurst (Tixy) wrote:
> > > On Fri, 2015-05-01 at 09:55 +0800, Leo Yan wrote:
> > > [...]
> > > > >  arch/arm64/boot/dts/arm/juno.dts | 28 ++++++++++++++++++++++++++++
> > > > >  1 file changed, 28 insertions(+)
> > > > > 
> > > > > diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
> > > > > index 133ee59..7a9a449 100644
> > > > > --- a/arch/arm64/boot/dts/arm/juno.dts
> > > > > +++ b/arch/arm64/boot/dts/arm/juno.dts
> > > > > @@ -34,12 +34,35 @@
> > > > >  		#address-cells = <2>;
> > > > >  		#size-cells = <0>;
> > > > >  
> > > > > +		idle-states {
> > > > > +			entry-method = "arm,psci";
> > > > > +
> > > > > +			CPU_SLEEP_0: cpu-sleep-0 {
> > > > > +				compatible = "arm,idle-state";
> > > > > +				arm,psci-suspend-param = <0x0010000>;
> > > > > +				local-timer-stop;
> > > > 
> > > > Just want to figure out the best way for big.LITTLE system; so have
> > > > one question: CA53 and CA57 have different power domain for arch
> > > > timer, right?
> > > 
> > > I'm not sure of the answer to that. The documentation I have does seem
> > > to state the timer is lost on cluster power down, which would imply that
> > > it's not when just powering down a cpu, but I'm not at all clear on the
> > > matter.
> > > 
> > > >  If this is the case, should we define two kinds of cpu
> > > > sleep states, one of them will not migrate to broadcast timer and
> > > > keep using arch timer after cpu has been powered down?
> > > 
> > > Do you mean that if the local timer is not lost (and so we should not
> > > have local-timer-stop above), then we should have another identical idle
> > > state except that it _does_ specify local-timer-stop to force the
> > > broadcast time to be used? If so, would that second state ever be more
> > > power efficient than the first? (I don't know the answer to that, this
> > > whole area is pretty new to me).
> > 
> > Ok, to start with, if we want the generic CPUidle driver to work on bL
> > systems, we should get this patch merged:
> > 
> > http://www.spinics.net/lists/arm-kernel/msg412790.html
> 
> Thanks pointing out this.
> 
> > Architected timer is always lost on power down, unless we are talking
> > about a logic retention state (or if we are talking about local timers
> > that are not the architected ones). Anyway, with the patch above, different
> > cpus can have different idle states, so different behaviours when it
> > comes to the local timer behaviour. If, say, on a A53/A57 system, A53
> > cpus lose the local timer on idle state entry and A57 cpus do not
> > (that's a non-existing example though), those cpus will have different
> > idle states so different local-timer-stop flags.
> 
> For A57, not sure if arch timer will be powered off if cpu has been
> powered off.
> 
> From A53's TRM figure 2-1 Cortex-A53 processor block diagram, the arch
> timer is not within CPU power domain, so likey arch timer still will
> work even cpu has been powered off (PDCPU/PDADVSIMD both have been off).
> This looks like is not aligning w/t your upper description. Could u
> confirm this understanding is right?

Not quite. Behind the architected timers there is a system shared counter
that is used as a basis for the local CPU's counter. So when a CPU is being
powered off the system counter still counts and the CPU's copy gets updated
on power on by the firmware (hopefully).

Best regards,
Liviu

> 
> Thanks,
> Leo Yan
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ?\_(?)_/?

[PATCH] arm64: dts: Add idle-states for Juno

[Lorenzo Pieralisi]

On Fri, May 01, 2015 at 02:30:13PM +0100, Catalin Marinas wrote:
> On Fri, May 01, 2015 at 11:12:11AM +0100, Lorenzo Pieralisi wrote:
> > On Fri, May 01, 2015 at 10:02:02AM +0100, Catalin Marinas wrote:
> > > On Thu, Apr 30, 2015 at 06:17:01PM +0100, Lorenzo Pieralisi wrote:
> > > > On Thu, Apr 30, 2015 at 05:40:35PM +0100, Jon Medhurst (Tixy) wrote:
> > > > > On Thu, 2015-04-30 at 17:00 +0100, Sudeep Holla wrote:
> > > > > > On 30/04/15 14:57, Jon Medhurst (Tixy) wrote:
> > > > > > > From: Jon Medhurst <tixy@linaro.org>
> > > > > > >
> > > > > > > Signed-off-by: Jon Medhurst <tixy@linaro.org>
> > > > > > > ---
> > > > > > >
> > > > > > > These have been kicking around out of tree for ages, any reason they
> > > > > > > shouldn't be in mainline?
> > > > > > 
> > > > > > One possible reason could be that these values are not tuned(e.g.
> > > > > > latency values, can they be same for both clusters ?)
> > > > > 
> > > > > I thought that both clusters being the same was questionable.
> > > > > 
> > > > > >  Though these
> > > > > > reasons are not blocking and this patch will not cause any
> > > > > > functionality break even if is merged as is.
> > > > > 
> > > > > My main purpose with trying to get this merged is so that people using
> > > > > Juno for general testing and validation will actually have cpuidle
> > > > > running and so potentially find more bugs.
> > > > 
> > > > I am reluctant to enable idle states in the default Juno dts, they
> > > > will affect latencies and performance tests significantly.
> > > 
> > > OTOH, I guess they will improve the power benchmarks. IMO, we should
> > > place in the DT whatever the hardware and firmware supports. It's up to
> > > those doing benchmarks to disable CPU suspend.
> > 
> > I am ok with DT defining whatever HW and FW support, the question is
> > whether we want CPUidle (and CPUfreq) enabled by default in the
> > defconfig then.
> 
> It's functionality we support, so I think it should be enabled in
> defconfig. This config is meant as a point of reference for what's
> supported by the arm64 kernel and not tuned for some specific
> benchmarks.

Point raised and taken, it makes sense so I am happy with this.

> > This will certainly trigger mainline regressions from a latency/performance
> > standpoint (true, some tests disable idle states by default ie cyclic
> > test), unless we disable CPUidle via command line parameter or we
> > force people carrying out tests to disable idle states through sysfs knobs.
> 
> It may affect performance for specific benchmarks but what if others
> test the power consumption?

See above.

> BTW, cannot the cpuidle governor be made smart enough (unless it is
> already) to figure out when the next timer is going to happen so that
> the CPU doesn't go in a deep sleep state with a high latency?

The menu governor already detects if/when core can sleep for long
enough to break even in terms of energy and to avoid breaking latency
constraints (which are tunable, with lax default values though).

My concern is the time required to exit the idle states (and the
cache refills required on power-up), we can certainly tune the DT residency
parameters (and user space can prevent idle by forcing latency constraints,
that's what eg cyclic test does by default), and that's what I am going
to do. And then there are IRQs that the governor can't predict, if a cpu
is powered off it will take a latency hit, that can only be prevented
by disabling the idle states.

Anyway, with all of the above in mind, I am fine with enabling the idle
states, I need to review and test the idle states DT data in the patch
first though.

Lorenzo

[PATCH] arm64: dts: Add idle-states for Juno

[Lorenzo Pieralisi]

On Fri, May 01, 2015 at 02:58:11PM +0100, Liviu Dudau wrote:
> On Fri, May 01, 2015 at 12:55:07PM +0100, Leo Yan wrote:
> > On Fri, May 01, 2015 at 11:45:06AM +0100, Lorenzo Pieralisi wrote:
> > > On Fri, May 01, 2015 at 11:22:31AM +0100, Jon Medhurst (Tixy) wrote:
> > > > On Fri, 2015-05-01 at 09:55 +0800, Leo Yan wrote:
> > > > [...]
> > > > > >  arch/arm64/boot/dts/arm/juno.dts | 28 ++++++++++++++++++++++++++++
> > > > > >  1 file changed, 28 insertions(+)
> > > > > > 
> > > > > > diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
> > > > > > index 133ee59..7a9a449 100644
> > > > > > --- a/arch/arm64/boot/dts/arm/juno.dts
> > > > > > +++ b/arch/arm64/boot/dts/arm/juno.dts
> > > > > > @@ -34,12 +34,35 @@
> > > > > >  		#address-cells = <2>;
> > > > > >  		#size-cells = <0>;
> > > > > >  
> > > > > > +		idle-states {
> > > > > > +			entry-method = "arm,psci";
> > > > > > +
> > > > > > +			CPU_SLEEP_0: cpu-sleep-0 {
> > > > > > +				compatible = "arm,idle-state";
> > > > > > +				arm,psci-suspend-param = <0x0010000>;
> > > > > > +				local-timer-stop;
> > > > > 
> > > > > Just want to figure out the best way for big.LITTLE system; so have
> > > > > one question: CA53 and CA57 have different power domain for arch
> > > > > timer, right?
> > > > 
> > > > I'm not sure of the answer to that. The documentation I have does seem
> > > > to state the timer is lost on cluster power down, which would imply that
> > > > it's not when just powering down a cpu, but I'm not at all clear on the
> > > > matter.
> > > > 
> > > > >  If this is the case, should we define two kinds of cpu
> > > > > sleep states, one of them will not migrate to broadcast timer and
> > > > > keep using arch timer after cpu has been powered down?
> > > > 
> > > > Do you mean that if the local timer is not lost (and so we should not
> > > > have local-timer-stop above), then we should have another identical idle
> > > > state except that it _does_ specify local-timer-stop to force the
> > > > broadcast time to be used? If so, would that second state ever be more
> > > > power efficient than the first? (I don't know the answer to that, this
> > > > whole area is pretty new to me).
> > > 
> > > Ok, to start with, if we want the generic CPUidle driver to work on bL
> > > systems, we should get this patch merged:
> > > 
> > > http://www.spinics.net/lists/arm-kernel/msg412790.html
> > 
> > Thanks pointing out this.
> > 
> > > Architected timer is always lost on power down, unless we are talking
> > > about a logic retention state (or if we are talking about local timers
> > > that are not the architected ones). Anyway, with the patch above, different
> > > cpus can have different idle states, so different behaviours when it
> > > comes to the local timer behaviour. If, say, on a A53/A57 system, A53
> > > cpus lose the local timer on idle state entry and A57 cpus do not
> > > (that's a non-existing example though), those cpus will have different
> > > idle states so different local-timer-stop flags.
> > 
> > For A57, not sure if arch timer will be powered off if cpu has been
> > powered off.
> > 
> > From A53's TRM figure 2-1 Cortex-A53 processor block diagram, the arch
> > timer is not within CPU power domain, so likey arch timer still will
> > work even cpu has been powered off (PDCPU/PDADVSIMD both have been off).
> > This looks like is not aligning w/t your upper description. Could u
> > confirm this understanding is right?
> 
> Not quite. Behind the architected timers there is a system shared counter
> that is used as a basis for the local CPU's counter. So when a CPU is being
> powered off the system counter still counts and the CPU's copy gets updated
> on power on by the firmware (hopefully).

There is nothing to be updated (ie CPUidle will never shut off the PDSOC
power domain), the system counter still counts on cpu and cluster power off,
but the logic implementing the arch timer in the cpu (ie comparators) is lost
on power off, so the arch timer has to be offloaded to the broadcast device.

HTH,
Lorenzo

[PATCH] arm64: dts: Add idle-states for Juno

[Leo Yan]

On Thu, May 07, 2015 at 01:40:29PM +0100, Lorenzo Pieralisi wrote:
> On Fri, May 01, 2015 at 02:58:11PM +0100, Liviu Dudau wrote:
> > On Fri, May 01, 2015 at 12:55:07PM +0100, Leo Yan wrote:
> > > On Fri, May 01, 2015 at 11:45:06AM +0100, Lorenzo Pieralisi wrote:
> > > > On Fri, May 01, 2015 at 11:22:31AM +0100, Jon Medhurst (Tixy) wrote:
> > > > > On Fri, 2015-05-01 at 09:55 +0800, Leo Yan wrote:
> > > > > [...]
> > > > > > >  arch/arm64/boot/dts/arm/juno.dts | 28 ++++++++++++++++++++++++++++
> > > > > > >  1 file changed, 28 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
> > > > > > > index 133ee59..7a9a449 100644
> > > > > > > --- a/arch/arm64/boot/dts/arm/juno.dts
> > > > > > > +++ b/arch/arm64/boot/dts/arm/juno.dts
> > > > > > > @@ -34,12 +34,35 @@
> > > > > > >  		#address-cells = <2>;
> > > > > > >  		#size-cells = <0>;
> > > > > > >  
> > > > > > > +		idle-states {
> > > > > > > +			entry-method = "arm,psci";
> > > > > > > +
> > > > > > > +			CPU_SLEEP_0: cpu-sleep-0 {
> > > > > > > +				compatible = "arm,idle-state";
> > > > > > > +				arm,psci-suspend-param = <0x0010000>;
> > > > > > > +				local-timer-stop;
> > > > > > 
> > > > > > Just want to figure out the best way for big.LITTLE system; so have
> > > > > > one question: CA53 and CA57 have different power domain for arch
> > > > > > timer, right?
> > > > > 
> > > > > I'm not sure of the answer to that. The documentation I have does seem
> > > > > to state the timer is lost on cluster power down, which would imply that
> > > > > it's not when just powering down a cpu, but I'm not at all clear on the
> > > > > matter.
> > > > > 
> > > > > >  If this is the case, should we define two kinds of cpu
> > > > > > sleep states, one of them will not migrate to broadcast timer and
> > > > > > keep using arch timer after cpu has been powered down?
> > > > > 
> > > > > Do you mean that if the local timer is not lost (and so we should not
> > > > > have local-timer-stop above), then we should have another identical idle
> > > > > state except that it _does_ specify local-timer-stop to force the
> > > > > broadcast time to be used? If so, would that second state ever be more
> > > > > power efficient than the first? (I don't know the answer to that, this
> > > > > whole area is pretty new to me).
> > > > 
> > > > Ok, to start with, if we want the generic CPUidle driver to work on bL
> > > > systems, we should get this patch merged:
> > > > 
> > > > http://www.spinics.net/lists/arm-kernel/msg412790.html
> > > 
> > > Thanks pointing out this.
> > > 
> > > > Architected timer is always lost on power down, unless we are talking
> > > > about a logic retention state (or if we are talking about local timers
> > > > that are not the architected ones). Anyway, with the patch above, different
> > > > cpus can have different idle states, so different behaviours when it
> > > > comes to the local timer behaviour. If, say, on a A53/A57 system, A53
> > > > cpus lose the local timer on idle state entry and A57 cpus do not
> > > > (that's a non-existing example though), those cpus will have different
> > > > idle states so different local-timer-stop flags.
> > > 
> > > For A57, not sure if arch timer will be powered off if cpu has been
> > > powered off.
> > > 
> > > From A53's TRM figure 2-1 Cortex-A53 processor block diagram, the arch
> > > timer is not within CPU power domain, so likey arch timer still will
> > > work even cpu has been powered off (PDCPU/PDADVSIMD both have been off).
> > > This looks like is not aligning w/t your upper description. Could u
> > > confirm this understanding is right?
> > 
> > Not quite. Behind the architected timers there is a system shared counter
> > that is used as a basis for the local CPU's counter. So when a CPU is being
> > powered off the system counter still counts and the CPU's copy gets updated
> > on power on by the firmware (hopefully).
> 
> There is nothing to be updated (ie CPUidle will never shut off the PDSOC
> power domain), the system counter still counts on cpu and cluster power off,
> but the logic implementing the arch timer in the cpu (ie comparators) is lost
> on power off, so the arch timer has to be offloaded to the broadcast device.

Thanks for confirmation. This will be helpful for later enabling
cpuidle on hikey board (eight CA53 cores)...

Thanks,
Leo Yan

[PATCH] arm64: dts: Add idle-states for Juno

[Punit Agrawal]

"Jon Medhurst (Tixy)" <tixy@linaro.org> writes:

> From: Jon Medhurst <tixy@linaro.org>
>
> Signed-off-by: Jon Medhurst <tixy@linaro.org>

Apologies for resurrecting an old thread.

Following the discussion on this thread, even though certain concerns
were raised, there wasn't any objection to $SUBJECT being merged.

I don't see this patch in any tree; perhaps it's slipped through the
cracks.

Could this be merged please?

> ---
>
> These have been kicking around out of tree for ages, any reason they
> shouldn't be in mainline? Also, was unsure of what to put in commit
> message.
>
>  arch/arm64/boot/dts/arm/juno.dts | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
> index 133ee59..7a9a449 100644
> --- a/arch/arm64/boot/dts/arm/juno.dts
> +++ b/arch/arm64/boot/dts/arm/juno.dts
> @@ -34,12 +34,35 @@
>  		#address-cells = <2>;
>  		#size-cells = <0>;
>  
> +		idle-states {
> +			entry-method = "arm,psci";
> +
> +			CPU_SLEEP_0: cpu-sleep-0 {
> +				compatible = "arm,idle-state";
> +				arm,psci-suspend-param = <0x0010000>;
> +				local-timer-stop;
> +				entry-latency-us = <100>;
> +				exit-latency-us = <250>;
> +				min-residency-us = <2000>;
> +			};
> +
> +			CLUSTER_SLEEP_0: cluster-sleep-0 {
> +				compatible = "arm,idle-state";
> +				arm,psci-suspend-param = <0x1010000>;
> +				local-timer-stop;
> +				entry-latency-us = <800>;
> +				exit-latency-us = <700>;
> +				min-residency-us = <2500>;
> +			};
> +		};
> +
>  		A57_0: cpu at 0 {
>  			compatible = "arm,cortex-a57","arm,armv8";
>  			reg = <0x0 0x0>;
>  			device_type = "cpu";
>  			enable-method = "psci";
>  			next-level-cache = <&A57_L2>;
> +			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
>  		};
>  
>  		A57_1: cpu at 1 {
> @@ -48,6 +71,7 @@
>  			device_type = "cpu";
>  			enable-method = "psci";
>  			next-level-cache = <&A57_L2>;
> +			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
>  		};
>  
>  		A53_0: cpu at 100 {
> @@ -56,6 +80,7 @@
>  			device_type = "cpu";
>  			enable-method = "psci";
>  			next-level-cache = <&A53_L2>;
> +			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
>  		};
>  
>  		A53_1: cpu at 101 {
> @@ -64,6 +89,7 @@
>  			device_type = "cpu";
>  			enable-method = "psci";
>  			next-level-cache = <&A53_L2>;
> +			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
>  		};
>  
>  		A53_2: cpu at 102 {
> @@ -72,6 +98,7 @@
>  			device_type = "cpu";
>  			enable-method = "psci";
>  			next-level-cache = <&A53_L2>;
> +			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
>  		};
>  
>  		A53_3: cpu at 103 {
> @@ -80,6 +107,7 @@
>  			device_type = "cpu";
>  			enable-method = "psci";
>  			next-level-cache = <&A53_L2>;
> +			cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>;
>  		};
>  
>  		A57_L2: l2-cache0 {

[PATCH] arm64: dts: Add idle-states for Juno

[Jon Medhurst (Tixy)]

On Thu, 2015-10-22 at 14:22 +0100, Punit Agrawal wrote:
> "Jon Medhurst (Tixy)" <tixy@linaro.org> writes:
> 
> > From: Jon Medhurst <tixy@linaro.org>
> >
> > Signed-off-by: Jon Medhurst <tixy@linaro.org>
> 
> Apologies for resurrecting an old thread.
> 
> Following the discussion on this thread, even though certain concerns
> were raised, there wasn't any objection to $SUBJECT being merged.
> 
> I don't see this patch in any tree; perhaps it's slipped through the
> cracks.

It did slip through the cracks. Lorenzo's last comment was "I am fine
with enabling the idle states, I need to review and test the idle states
DT data in the patch first though." and I didn't chase things up.

The patch will need refreshing to add idle for Juno r1. Which will then
probably resurrect the discussion about where the numbers come from for
residency times, and are the same ones for r0 valid on r1 (and r2?).

In an effort to forestall that I would say: does anyone actually care if
the values are optimal? Juno is a reference platform and powered off
mains, so tuning for the optimum power consumption is pretty pointless.
But because it _is_ used as a reference by people it should at least
have these features enabled, to serve as an example, and for test
coverage.

(And we can all pretend to ignore the elephant in the room
http://lwn.net/Articles/659347/)

-- 
Tixy

[PATCH] arm64: dts: Add idle-states for Juno

[Lorenzo Pieralisi]

On Mon, Oct 26, 2015 at 02:12:31PM +0000, Jon Medhurst (Tixy) wrote:
> On Thu, 2015-10-22 at 14:22 +0100, Punit Agrawal wrote:
> > "Jon Medhurst (Tixy)" <tixy@linaro.org> writes:
> > 
> > > From: Jon Medhurst <tixy@linaro.org>
> > >
> > > Signed-off-by: Jon Medhurst <tixy@linaro.org>
> > 
> > Apologies for resurrecting an old thread.
> > 
> > Following the discussion on this thread, even though certain concerns
> > were raised, there wasn't any objection to $SUBJECT being merged.
> > 
> > I don't see this patch in any tree; perhaps it's slipped through the
> > cracks.
> 
> It did slip through the cracks. Lorenzo's last comment was "I am fine
> with enabling the idle states, I need to review and test the idle states
> DT data in the patch first though." and I didn't chase things up.
> 
> The patch will need refreshing to add idle for Juno r1. Which will then
> probably resurrect the discussion about where the numbers come from for
> residency times, and are the same ones for r0 valid on r1 (and r2?).
> 
> In an effort to forestall that I would say: does anyone actually care if
> the values are optimal? Juno is a reference platform and powered off
> mains, so tuning for the optimum power consumption is pretty pointless.
> But because it _is_ used as a reference by people it should at least
> have these features enabled, to serve as an example, and for test
> coverage.

I agree with you here, let me check the entry/exit latencies again
to make sure they are reasonably set-up, it is 4.5 material anyway.

Thanks,
Lorenzo

[PATCH] arm64: dts: Add idle-states for Juno

[Punit Agrawal]

Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> writes:

> On Mon, Oct 26, 2015 at 02:12:31PM +0000, Jon Medhurst (Tixy) wrote:
>> On Thu, 2015-10-22 at 14:22 +0100, Punit Agrawal wrote:
>> > "Jon Medhurst (Tixy)" <tixy@linaro.org> writes:
>> > 
>> > > From: Jon Medhurst <tixy@linaro.org>
>> > >
>> > > Signed-off-by: Jon Medhurst <tixy@linaro.org>
>> > 
>> > Apologies for resurrecting an old thread.
>> > 
>> > Following the discussion on this thread, even though certain concerns
>> > were raised, there wasn't any objection to $SUBJECT being merged.
>> > 
>> > I don't see this patch in any tree; perhaps it's slipped through the
>> > cracks.
>> 
>> It did slip through the cracks. Lorenzo's last comment was "I am fine
>> with enabling the idle states, I need to review and test the idle states
>> DT data in the patch first though." and I didn't chase things up.
>> 
>> The patch will need refreshing to add idle for Juno r1. Which will then
>> probably resurrect the discussion about where the numbers come from for
>> residency times, and are the same ones for r0 valid on r1 (and r2?).
>> 
>> In an effort to forestall that I would say: does anyone actually care if
>> the values are optimal? Juno is a reference platform and powered off
>> mains, so tuning for the optimum power consumption is pretty pointless.
>> But because it _is_ used as a reference by people it should at least
>> have these features enabled, to serve as an example, and for test
>> coverage.
>
> I agree with you here, let me check the entry/exit latencies again
> to make sure they are reasonably set-up, it is 4.5 material anyway.

Hi Lorenzo,

Have you had a chance to sanity check the values we've got here? I
didn't want to miss this merge window if possible.

Thanks,
Punit

>
> Thanks,
> Lorenzo
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] arm64: dts: Add idle-states for Juno

[Lorenzo Pieralisi]

On Mon, Nov 23, 2015 at 05:45:10PM +0000, Punit Agrawal wrote:
> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> writes:
> 
> > On Mon, Oct 26, 2015 at 02:12:31PM +0000, Jon Medhurst (Tixy) wrote:
> >> On Thu, 2015-10-22 at 14:22 +0100, Punit Agrawal wrote:
> >> > "Jon Medhurst (Tixy)" <tixy@linaro.org> writes:
> >> > 
> >> > > From: Jon Medhurst <tixy@linaro.org>
> >> > >
> >> > > Signed-off-by: Jon Medhurst <tixy@linaro.org>
> >> > 
> >> > Apologies for resurrecting an old thread.
> >> > 
> >> > Following the discussion on this thread, even though certain concerns
> >> > were raised, there wasn't any objection to $SUBJECT being merged.
> >> > 
> >> > I don't see this patch in any tree; perhaps it's slipped through the
> >> > cracks.
> >> 
> >> It did slip through the cracks. Lorenzo's last comment was "I am fine
> >> with enabling the idle states, I need to review and test the idle states
> >> DT data in the patch first though." and I didn't chase things up.
> >> 
> >> The patch will need refreshing to add idle for Juno r1. Which will then
> >> probably resurrect the discussion about where the numbers come from for
> >> residency times, and are the same ones for r0 valid on r1 (and r2?).
> >> 
> >> In an effort to forestall that I would say: does anyone actually care if
> >> the values are optimal? Juno is a reference platform and powered off
> >> mains, so tuning for the optimum power consumption is pretty pointless.
> >> But because it _is_ used as a reference by people it should at least
> >> have these features enabled, to serve as an example, and for test
> >> coverage.
> >
> > I agree with you here, let me check the entry/exit latencies again
> > to make sure they are reasonably set-up, it is 4.5 material anyway.
> 
> Hi Lorenzo,
> 
> Have you had a chance to sanity check the values we've got here? I
> didn't want to miss this merge window if possible.

Yes and they need updating. This data is really really FW dependent,
that's the reason why I still think that these values should be provided
by FW and not shoved into the kernel. Anyway, here we go (I assume the
worst case and that's the only safe option I can see):

- the entry-latency should be set to 300us for _both_ idle states
- the exit-latency should be set to 1.2ms for _both_ idle states

The min-residency values looks reasonable, a tad optimistic (but again
that's workload and FW dependent so either we settle for the worst
possible case or we do not merge this at all).

With the changes above:

Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

[PATCH] arm64: dts: Add idle-states for Juno

[Punit Agrawal]

Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> writes:

> On Mon, Nov 23, 2015 at 05:45:10PM +0000, Punit Agrawal wrote:
>> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> writes:
>> 
>> > On Mon, Oct 26, 2015 at 02:12:31PM +0000, Jon Medhurst (Tixy) wrote:
>> >> On Thu, 2015-10-22 at 14:22 +0100, Punit Agrawal wrote:
>> >> > "Jon Medhurst (Tixy)" <tixy@linaro.org> writes:
>> >> > 
>> >> > > From: Jon Medhurst <tixy@linaro.org>
>> >> > >
>> >> > > Signed-off-by: Jon Medhurst <tixy@linaro.org>
>> >> > 
>> >> > Apologies for resurrecting an old thread.
>> >> > 
>> >> > Following the discussion on this thread, even though certain concerns
>> >> > were raised, there wasn't any objection to $SUBJECT being merged.
>> >> > 
>> >> > I don't see this patch in any tree; perhaps it's slipped through the
>> >> > cracks.
>> >> 
>> >> It did slip through the cracks. Lorenzo's last comment was "I am fine
>> >> with enabling the idle states, I need to review and test the idle states
>> >> DT data in the patch first though." and I didn't chase things up.
>> >> 
>> >> The patch will need refreshing to add idle for Juno r1. Which will then
>> >> probably resurrect the discussion about where the numbers come from for
>> >> residency times, and are the same ones for r0 valid on r1 (and r2?).
>> >> 
>> >> In an effort to forestall that I would say: does anyone actually care if
>> >> the values are optimal? Juno is a reference platform and powered off
>> >> mains, so tuning for the optimum power consumption is pretty pointless.
>> >> But because it _is_ used as a reference by people it should at least
>> >> have these features enabled, to serve as an example, and for test
>> >> coverage.
>> >
>> > I agree with you here, let me check the entry/exit latencies again
>> > to make sure they are reasonably set-up, it is 4.5 material anyway.
>> 
>> Hi Lorenzo,
>> 
>> Have you had a chance to sanity check the values we've got here? I
>> didn't want to miss this merge window if possible.
>
> Yes and they need updating. This data is really really FW dependent,
> that's the reason why I still think that these values should be provided
> by FW and not shoved into the kernel. Anyway, here we go (I assume the
> worst case and that's the only safe option I can see):
>
> - the entry-latency should be set to 300us for _both_ idle states
> - the exit-latency should be set to 1.2ms for _both_ idle states
>
> The min-residency values looks reasonable, a tad optimistic (but again
> that's workload and FW dependent so either we settle for the worst
> possible case or we do not merge this at all).
>
> With the changes above:
>
> Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

Tixy,

Could you please refresh this patch with the changes suggested by
Lorenzo? We might still be able to get it in for 4.4.

Thanks,
Punit

ps: Apologies if I've missed a new posting with these changes.

>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel