| Message ID | 1430402268.2868.20.camel@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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 { > -- > 2.1.4 > > >
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
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 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
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 >
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 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
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 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.
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
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 >
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). 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
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! / > --------------- > ??\_(???)_/??
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 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?
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 >
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
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
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/)
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
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
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>
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
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 {