arm64: dts: Add idle-states for Juno

Message ID 1430402268.2868.20.camel@linaro.org
State New
Headers show

Commit Message

Jon Medhurst (Tixy) April 30, 2015, 1:57 p.m.
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(+)

Comments

Jon Medhurst (Tixy) April 30, 2015, 4:28 p.m. | #1
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
Jon Medhurst (Tixy) April 30, 2015, 4:40 p.m. | #2
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 ;-)
Jon Medhurst (Tixy) April 30, 2015, 5:36 p.m. | #3
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.
Leo Yan May 1, 2015, 1:55 a.m. | #4
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@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@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@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@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@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@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 {
Jon Medhurst (Tixy) May 1, 2015, 8:52 a.m. | #5
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.
Jon Medhurst (Tixy) May 1, 2015, 10:22 a.m. | #6
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).
Leo Yan May 1, 2015, 11:39 a.m. | #7
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
Leo Yan May 1, 2015, 11:55 a.m. | #8
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
Leo Yan May 7, 2015, 2:32 p.m. | #9
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
Punit Agrawal Oct. 22, 2015, 1:22 p.m. | #10
"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@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@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@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@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@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@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 {
Jon Medhurst (Tixy) Oct. 26, 2015, 2:12 p.m. | #11
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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Punit Agrawal Nov. 23, 2015, 5:45 p.m. | #12
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@lists.infradead.org

> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Punit Agrawal Dec. 7, 2015, 4:59 p.m. | #13
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@lists.infradead.org

> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel


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

Patch hide | download patch | download mbox

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@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@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@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@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@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@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 {