| Message ID | 1430402268.2868.20.camel@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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 ;-)
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.
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 {
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.
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).
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
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
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
"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 {
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
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
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
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 {