arm64: dts: Add idle-states for Juno
Subsystems (2)
PATCH 1/1 arm64: dts: Add idle-states for Juno
From: Jon Medhurst <[email protected]>
---
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(+)
arch/arm64/boot/dts/arm/juno.dts +28 -0
@@ -34,12 +34,35 @@ #address-cells = <2>; #size-cells = <0>; + idle-states { + entry-method = "arm,psci"; + + CPU_SLEEP_0: cpu-sleep-0 { + compatible = "arm,idle-state"; + arm,psci-suspend-param = <0x0010000>; + local-timer-stop; + entry-latency-us = <100>; + exit-latency-us = <250>; + min-residency-us = <2000>; + }; + + CLUSTER_SLEEP_0: cluster-sleep-0 { + compatible = "arm,idle-state"; + arm,psci-suspend-param = <0x1010000>; + local-timer-stop; + entry-latency-us = <800>; + exit-latency-us = <700>; + min-residency-us = <2500>; + }; + }; + A57_0: cpu at 0 { compatible = "arm,cortex-a57","arm,armv8"; reg = <0x0 0x0>; device_type = "cpu"; enable-method = "psci"; next-level-cache = <&A57_L2>; + cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>; }; A57_1: cpu at 1 {
@@ -48,6 +71,7 @@ device_type = "cpu"; enable-method = "psci"; next-level-cache = <&A57_L2>; + cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>; }; A53_0: cpu at 100 {
@@ -56,6 +80,7 @@ device_type = "cpu"; enable-method = "psci"; next-level-cache = <&A53_L2>; + cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>; }; A53_1: cpu at 101 {
@@ -64,6 +89,7 @@ device_type = "cpu"; enable-method = "psci"; next-level-cache = <&A53_L2>; + cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>; }; A53_2: cpu at 102 {
@@ -72,6 +98,7 @@ device_type = "cpu"; enable-method = "psci"; next-level-cache = <&A53_L2>; + cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>; }; A53_3: cpu at 103 {
@@ -80,6 +107,7 @@ device_type = "cpu"; enable-method = "psci"; next-level-cache = <&A53_L2>; + cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>; }; A57_L2: l2-cache0 {
Signature · 1 line
2.1.4
Liviu Dudau
maintainerPATCH 1/1 arm64: dts: Add idle-states for Juno
Quoted 8 lines · Jon Medhurst (Tixy)
From: Jon Medhurst <[email protected]>
Signed-off-by Jon Medhurst <[email protected]>---
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
Quoted 89 lines
arch/arm64/boot/dts/arm/juno.dts | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)arch/arm64/boot/dts/arm/juno.dts +28
-0@@ -34,12 +34,35 @@ #address-cells = <2>; #size-cells = <0>; + idle-states { + entry-method = "arm,psci"; + + CPU_SLEEP_0: cpu-sleep-0 { + compatible = "arm,idle-state"; + arm,psci-suspend-param = <0x0010000>; + local-timer-stop; + entry-latency-us = <100>; + exit-latency-us = <250>; + min-residency-us = <2000>; + }; + + CLUSTER_SLEEP_0: cluster-sleep-0 { + compatible = "arm,idle-state"; + arm,psci-suspend-param = <0x1010000>; + local-timer-stop; + entry-latency-us = <800>; + exit-latency-us = <700>; + min-residency-us = <2500>; + }; + }; + A57_0: cpu at 0 { compatible = "arm,cortex-a57","arm,armv8"; reg = <0x0 0x0>; device_type = "cpu"; enable-method = "psci"; next-level-cache = <&A57_L2>; + cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>; }; A57_1: cpu at 1 {@@ -48,6 +71,7 @@ device_type = "cpu"; enable-method = "psci"; next-level-cache = <&A57_L2>; + cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>; }; A53_0: cpu at 100 {@@ -56,6 +80,7 @@ device_type = "cpu"; enable-method = "psci"; next-level-cache = <&A53_L2>; + cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>; }; A53_1: cpu at 101 {@@ -64,6 +89,7 @@ device_type = "cpu"; enable-method = "psci"; next-level-cache = <&A53_L2>; + cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>; }; A53_2: cpu at 102 {@@ -72,6 +98,7 @@ device_type = "cpu"; enable-method = "psci"; next-level-cache = <&A53_L2>; + cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>; }; A53_3: cpu at 103 {@@ -80,6 +107,7 @@ device_type = "cpu"; enable-method = "psci"; next-level-cache = <&A53_L2>; + cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>; }; A57_L2: l2-cache0 {Signature · 1 line
2.1.4
Signature · 8 lines
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
?\_(?)_/?PATCH 1/1 arm64: dts: Add idle-states for Juno
Quoted 2 lines · Liviu Dudau
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
Signature · 1 line
Tixy
PATCH 1/1 arm64: dts: Add idle-states for Juno
In reply to Jon Medhurst (Tixy)Quoted 7 lines · Jon Medhurst (Tixy)
From: Jon Medhurst <[email protected]>
Signed-off-by Jon Medhurst <[email protected]>---
These have been kicking around out of tree for ages, any reason they shouldn't be in mainline?
One possible reason could be that these values are not tuned(e.g. latency values, can they be same for both clusters ?) Though these reasons are not blocking and this patch will not cause any functionality break even if is merged as is.
Regards,
Sudeep
PATCH 1/1 arm64: dts: Add idle-states for Juno
Quoted 4 lines · Sudeep Holla
Quoted 2 lines · Jon Medhurst (Tixy)
From: Jon Medhurst <[email protected]>
Quoted 8 lines
Quoted 5 lines
Signed-off-by Jon Medhurst <[email protected]>---
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.
Quoted 3 lines
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 ;-)
Signature · 1 line
Tixy
PATCH 1/1 arm64: dts: Add idle-states for Juno
Quoted 20 lines · Jon Medhurst (Tixy)
Quoted 4 lines · Sudeep Holla
Quoted 2 lines · Jon Medhurst (Tixy)
From: Jon Medhurst <[email protected]>
Quoted 8 lines
Quoted 5 lines
Signed-off-by Jon Medhurst <[email protected]>---
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.
Quoted 3 lines
Though these
reasons are not blocking and this patch will not cause any functionality break even if is merged as is.
Quoted 8 lines
My main purpose with trying to get this merged is so that people using Juno for general testing and validation will actually have cpuidle running and so potentially find more bugs.
If we wait until ARM have finished tweaking their firmware and tuning benchmark runs to get 'optimum' values for their main usecases then I suspect we'll be waiting a long time ;-)
Agreed, I am not blocking the patch, it can be merged in its current state. Since we had always seen stability issues with firmware until recently, we were hesitant to push this change. Now that we have established that it's stable enough, it's ready to go ;)
Regards,
Sudeep
PATCH 1/1 arm64: dts: Add idle-states for Juno
In reply to Jon Medhurst (Tixy)Quoted 16 lines · Jon Medhurst (Tixy)
Quoted 4 lines · Sudeep Holla
Quoted 2 lines · Jon Medhurst (Tixy)
From: Jon Medhurst <[email protected]>
Quoted 8 lines
Quoted 5 lines
Signed-off-by Jon Medhurst <[email protected]>---
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.
Quoted 7 lines
Quoted 3 lines
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
Quoted 8 lines
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 ;-)
Signature · 1 line
Tixy
PATCH 1/1 arm64: dts: Add idle-states for Juno
Quoted 25 lines · Lorenzo Pieralisi
Quoted 16 lines · Jon Medhurst (Tixy)
Quoted 4 lines · Sudeep Holla
Quoted 2 lines · Jon Medhurst (Tixy)
From: Jon Medhurst <[email protected]>
Quoted 8 lines
Quoted 5 lines
Signed-off-by Jon Medhurst <[email protected]>---
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.
Quoted 7 lines
Quoted 3 lines
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.
Quoted 5 lines
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.
Signature · 1 line
Tixy
PATCH 1/1 arm64: dts: Add idle-states for Juno
Quoted 26 lines · Jon Medhurst (Tixy)
Quoted 25 lines · Lorenzo Pieralisi
Quoted 23 lines · Jon Medhurst (Tixy)
Quoted 4 lines · Sudeep Holla
Quoted 2 lines · Jon Medhurst (Tixy)
From: Jon Medhurst <[email protected]>
Quoted 8 lines
Quoted 5 lines
Signed-off-by Jon Medhurst <[email protected]>---
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.
Quoted 3 lines
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.
Quoted 9 lines
Quoted 5 lines
I am reluctant to enable idle states in the default Juno dts, they will affect latencies and performance tests significantly. I should find a way to disable them by default and possibly have a DT property to enabled them explicitly, we can't merge the dts as it is we have to change the CPUidle code first.
Presumably the same argument goes for cpufreq? That'll upset people doing performance benchmarks, so shouldn't be enabled by default?
Or we can have cpufreq enabled with performance governor as default ;)
Regards,
Sudeep
PATCH 1/1 arm64: dts: Add idle-states for Juno
Quoted 34 lines · Sudeep Holla
Quoted 32 lines · Jon Medhurst (Tixy)
Quoted 30 lines · Lorenzo Pieralisi
Quoted 16 lines · Jon Medhurst (Tixy)
Quoted 4 lines · Sudeep Holla
Quoted 2 lines · Jon Medhurst (Tixy)
From: Jon Medhurst <[email protected]>
Quoted 8 lines
Quoted 5 lines
Signed-off-by Jon Medhurst <[email protected]>---
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.
Quoted 7 lines
Quoted 3 lines
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.
Quoted 5 lines
Quoted 3 lines
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.
Signature · 1 line
Tixy
PATCH 1/1 arm64: dts: Add idle-states for Juno
In reply to Lorenzo PieralisiQuoted 20 lines · Lorenzo Pieralisi
Quoted 19 lines · Jon Medhurst (Tixy)
Quoted 11 lines · Sudeep Holla
Quoted 7 lines · Jon Medhurst (Tixy)
From: Jon Medhurst <[email protected]>
Signed-off-by Jon Medhurst <[email protected]>---
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.
Quoted 3 lines
Though these
reasons are not blocking and this patch will not cause any functionality break even if is merged as is.
Quoted 6 lines
Quoted 3 lines
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.
Signature · 1 line
Catalin
PATCH 1/1 arm64: dts: Add idle-states for Juno
Quoted 25 lines · Catalin Marinas
Quoted 24 lines · Lorenzo Pieralisi
Quoted 22 lines · Jon Medhurst (Tixy)
Quoted 11 lines · Sudeep Holla
Quoted 7 lines · Jon Medhurst (Tixy)
From: Jon Medhurst <[email protected]>
Signed-off-by Jon Medhurst <[email protected]>---
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.
Quoted 3 lines
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.
Quoted 6 lines
Quoted 2 lines
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
Liviu Dudau
maintainerPATCH 1/1 arm64: dts: Add idle-states for Juno
Quoted 33 lines · Lorenzo Pieralisi
Quoted 25 lines · Catalin Marinas
Quoted 24 lines · Lorenzo Pieralisi
Quoted 22 lines · Jon Medhurst (Tixy)
Quoted 11 lines · Sudeep Holla
Quoted 7 lines · Jon Medhurst (Tixy)
From: Jon Medhurst <[email protected]>
Signed-off-by Jon Medhurst <[email protected]>---
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.
Quoted 3 lines
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.
Quoted 6 lines
Quoted 2 lines
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.
Quoted 8 lines
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
Quoted 3 lines
Lorenzo
Signature · 8 lines
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
?\_(?)_/?PATCH 1/1 arm64: dts: Add idle-states for Juno
Quoted 43 lines · Liviu Dudau
Quoted 33 lines · Lorenzo Pieralisi
Quoted 25 lines · Catalin Marinas
Quoted 24 lines · Lorenzo Pieralisi
Quoted 22 lines · Jon Medhurst (Tixy)
Quoted 11 lines · Sudeep Holla
Quoted 7 lines · Jon Medhurst (Tixy)
From: Jon Medhurst <[email protected]>
Signed-off-by Jon Medhurst <[email protected]>---
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.
Quoted 3 lines
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.
Quoted 6 lines
Quoted 2 lines
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.
Quoted 8 lines
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.
Quoted 5 lines
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
Quoted 16 lines
Best regards,
LiviuQuoted 3 lines
Lorenzo
Signature · 8 lines
==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ??\_(???)_/??
PATCH 1/1 arm64: dts: Add idle-states for Juno
In reply to Lorenzo PieralisiQuoted 29 lines · Lorenzo Pieralisi
Quoted 28 lines · Catalin Marinas
Quoted 26 lines · Lorenzo Pieralisi
Quoted 15 lines · Jon Medhurst (Tixy)
Quoted 11 lines · Sudeep Holla
Quoted 7 lines · Jon Medhurst (Tixy)
From: Jon Medhurst <[email protected]>
Signed-off-by Jon Medhurst <[email protected]>---
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.
Quoted 7 lines
Quoted 3 lines
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.
Quoted 7 lines
Quoted 3 lines
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.
Quoted 4 lines
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?
Signature · 1 line
Catalin
PATCH 1/1 arm64: dts: Add idle-states for Juno
Quoted 34 lines · Catalin Marinas
Quoted 33 lines · Lorenzo Pieralisi
Quoted 31 lines · Catalin Marinas
Quoted 20 lines · Lorenzo Pieralisi
Quoted 19 lines · Jon Medhurst (Tixy)
Quoted 11 lines · Sudeep Holla
Quoted 7 lines · Jon Medhurst (Tixy)
From: Jon Medhurst <[email protected]>
Signed-off-by Jon Medhurst <[email protected]>---
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.
Quoted 3 lines
Though these
reasons are not blocking and this patch will not cause any functionality break even if is merged as is.Quoted 6 lines
Quoted 3 lines
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.
Quoted 8 lines
Quoted 3 lines
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.
Quoted 7 lines
Quoted 4 lines
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.
Quoted 3 lines
BTW, cannot the cpuidle governor be made smart enough (unless it is already) to figure out when the next timer is going to happen so that the CPU doesn't go in a deep sleep state with a high latency?
The menu governor already detects if/when core can sleep for long enough to break even in terms of energy and to avoid breaking latency constraints (which are tunable, with lax default values though).
My concern is the time required to exit the idle states (and the cache refills required on power-up), we can certainly tune the DT residency parameters (and user space can prevent idle by forcing latency constraints, that's what eg cyclic test does by default), and that's what I am going to do. And then there are IRQs that the governor can't predict, if a cpu is powered off it will take a latency hit, that can only be prevented by disabling the idle states.
Anyway, with all of the above in mind, I am fine with enabling the idle states, I need to review and test the idle states DT data in the patch first though.
Lorenzo
PATCH 1/1 arm64: dts: Add idle-states for Juno
In reply to Jon Medhurst (Tixy)Quoted 17 lines · Jon Medhurst (Tixy)
From: Jon Medhurst <[email protected]>
Signed-off-by Jon Medhurst <[email protected]>---
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(+)arch/arm64/boot/dts/arm/juno.dts +0
-0
arch/arm64/boot/dts/arm/juno.dts line 43
Quoted 10 lines
arch/arm64/boot/dts/arm/juno.dts +7
-0@@ -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?
Quoted 65 lines
arch/arm64/boot/dts/arm/juno.dts +21
-0+ entry-latency-us = <100>; + exit-latency-us = <250>; + min-residency-us = <2000>; + }; + + CLUSTER_SLEEP_0: cluster-sleep-0 { + compatible = "arm,idle-state"; + arm,psci-suspend-param = <0x1010000>; + local-timer-stop; + entry-latency-us = <800>; + exit-latency-us = <700>; + min-residency-us = <2500>; + }; + }; + A57_0: cpu at 0 { compatible = "arm,cortex-a57","arm,armv8"; reg = <0x0 0x0>; device_type = "cpu"; enable-method = "psci"; next-level-cache = <&A57_L2>; + cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>; }; A57_1: cpu at 1 {@@ -48,6 +71,7 @@ device_type = "cpu"; enable-method = "psci"; next-level-cache = <&A57_L2>; + cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>; }; A53_0: cpu at 100 {@@ -56,6 +80,7 @@ device_type = "cpu"; enable-method = "psci"; next-level-cache = <&A53_L2>; + cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>; }; A53_1: cpu at 101 {@@ -64,6 +89,7 @@ device_type = "cpu"; enable-method = "psci"; next-level-cache = <&A53_L2>; + cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>; }; A53_2: cpu at 102 {@@ -72,6 +98,7 @@ device_type = "cpu"; enable-method = "psci"; next-level-cache = <&A53_L2>; + cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>; }; A53_3: cpu at 103 {@@ -80,6 +107,7 @@ device_type = "cpu"; enable-method = "psci"; next-level-cache = <&A53_L2>; + cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>; }; A57_L2: l2-cache0 {
PATCH 1/1 arm64: dts: Add idle-states for Juno
On Fri, 2015-05-01 at 09:55 +0800, Leo Yan wrote:
[...]
Quoted 11 lines
Quoted 11 lines
arch/arm64/boot/dts/arm/juno.dts | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)arch/arm64/boot/dts/arm/juno.dts +0
-0@@ -34,12 +34,35 @@ #address-cells = <2>; #size-cells = <0>;
Quoted 11 lines
arch/arm64/boot/dts/arm/juno.dts line 43
Quoted 7 lines
arch/arm64/boot/dts/arm/juno.dts +7
-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.
Quoted 3 lines
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).
Signature · 1 line
Tixy
PATCH 1/1 arm64: dts: Add idle-states for Juno
Quoted 34 lines · Jon Medhurst (Tixy)
On Fri, 2015-05-01 at 09:55 +0800, Leo Yan wrote:
[...]Quoted 11 lines
Quoted 11 lines
arch/arm64/boot/dts/arm/juno.dts | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)arch/arm64/boot/dts/arm/juno.dts +0
-0@@ -34,12 +34,35 @@ #address-cells = <2>; #size-cells = <0>;Quoted 11 lines
arch/arm64/boot/dts/arm/juno.dts line 43
Quoted 7 lines
arch/arm64/boot/dts/arm/juno.dts +7
-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.
Quoted 3 lines
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?
Quoted 6 lines
Do you mean that if the local timer is not lost (and so we should not have local-timer-stop above), then we should have another identical idle state except that it _does_ specify local-timer-stop to force the broadcast time to be used? If so, would that second state ever be more power efficient than the first? (I don't know the answer to that, this whole area is pretty new to me).
Ok, to start with, if we want the generic CPUidle driver to work on bL systems, we should get this patch merged:
http://www.spinics.net/lists/arm-kernel/msg412790.html
Architected timer is always lost on power down, unless we are talking about a logic retention state (or if we are talking about local timers that are not the architected ones). Anyway, with the patch above, different cpus can have different idle states, so different behaviours when it comes to the local timer behaviour. If, say, on a A53/A57 system, A53 cpus lose the local timer on idle state entry and A57 cpus do not (that's a non-existing example though), those cpus will have different idle states so different local-timer-stop flags.
I hope this helps.
Lorenzo
PATCH 1/1 arm64: dts: Add idle-states for Juno
Quoted 42 lines · Lorenzo Pieralisi
Quoted 34 lines · Jon Medhurst (Tixy)
On Fri, 2015-05-01 at 09:55 +0800, Leo Yan wrote:
[...]Quoted 11 lines
Quoted 11 lines
arch/arm64/boot/dts/arm/juno.dts | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)arch/arm64/boot/dts/arm/juno.dts +0
-0@@ -34,12 +34,35 @@ #address-cells = <2>; #size-cells = <0>;Quoted 11 lines
arch/arm64/boot/dts/arm/juno.dts line 43
Quoted 7 lines
arch/arm64/boot/dts/arm/juno.dts +7
-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.
Quoted 3 lines
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?Quoted 6 lines
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).
Quoted 4 lines
Ok, to start with, if we want the generic CPUidle driver to work on bL systems, we should get this patch merged:
Thanks pointing out this.
Quoted 8 lines
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.
Quoted 1 line
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
Liviu Dudau
maintainerPATCH 1/1 arm64: dts: Add idle-states for Juno
Quoted 59 lines · Leo Yan
Quoted 42 lines · Lorenzo Pieralisi
Quoted 34 lines · Jon Medhurst (Tixy)
On Fri, 2015-05-01 at 09:55 +0800, Leo Yan wrote:
[...]Quoted 11 lines
Quoted 11 lines
arch/arm64/boot/dts/arm/juno.dts | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)arch/arm64/boot/dts/arm/juno.dts +0
-0@@ -34,12 +34,35 @@ #address-cells = <2>; #size-cells = <0>;Quoted 11 lines
arch/arm64/boot/dts/arm/juno.dts line 43
Quoted 7 lines
arch/arm64/boot/dts/arm/juno.dts +7
-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.
Quoted 3 lines
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?Quoted 6 lines
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).
Quoted 4 lines
Ok, to start with, if we want the generic CPUidle driver to work on bL systems, we should get this patch merged:
Thanks pointing out this.
Quoted 8 lines
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.
Quoted 8 lines
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
Quoted 4 lines
Thanks,
Leo Yan
Signature · 8 lines
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
?\_(?)_/?PATCH 1/1 arm64: dts: Add idle-states for Juno
Quoted 69 lines · Liviu Dudau
Quoted 59 lines · Leo Yan
Quoted 42 lines · Lorenzo Pieralisi
Quoted 34 lines · Jon Medhurst (Tixy)
On Fri, 2015-05-01 at 09:55 +0800, Leo Yan wrote:
[...]Quoted 11 lines
Quoted 11 lines
arch/arm64/boot/dts/arm/juno.dts | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)arch/arm64/boot/dts/arm/juno.dts +0
-0@@ -34,12 +34,35 @@ #address-cells = <2>; #size-cells = <0>;Quoted 11 lines
arch/arm64/boot/dts/arm/juno.dts line 43
Quoted 7 lines
arch/arm64/boot/dts/arm/juno.dts +7
-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.
Quoted 3 lines
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?Quoted 6 lines
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).
Quoted 4 lines
Ok, to start with, if we want the generic CPUidle driver to work on bL systems, we should get this patch merged:
Thanks pointing out this.
Quoted 8 lines
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.
Quoted 8 lines
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?
Quoted 4 lines
Not quite. Behind the architected timers there is a system shared counter that is used as a basis for the local CPU's counter. So when a CPU is being powered off the system counter still counts and the CPU's copy gets updated on power on by the firmware (hopefully).
There is nothing to be updated (ie CPUidle will never shut off the PDSOC power domain), the system counter still counts on cpu and cluster power off, but the logic implementing the arch timer in the cpu (ie comparators) is lost on power off, so the arch timer has to be offloaded to the broadcast device.
HTH,
Lorenzo
PATCH 1/1 arm64: dts: Add idle-states for Juno
Quoted 75 lines · Lorenzo Pieralisi
Quoted 69 lines · Liviu Dudau
Quoted 59 lines · Leo Yan
Quoted 42 lines · Lorenzo Pieralisi
Quoted 34 lines · Jon Medhurst (Tixy)
On Fri, 2015-05-01 at 09:55 +0800, Leo Yan wrote:
[...]Quoted 11 lines
Quoted 11 lines
arch/arm64/boot/dts/arm/juno.dts | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)arch/arm64/boot/dts/arm/juno.dts +0
-0@@ -34,12 +34,35 @@ #address-cells = <2>; #size-cells = <0>;Quoted 11 lines
arch/arm64/boot/dts/arm/juno.dts line 43
Quoted 7 lines
arch/arm64/boot/dts/arm/juno.dts +7
-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.
Quoted 3 lines
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?Quoted 6 lines
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).
Quoted 4 lines
Ok, to start with, if we want the generic CPUidle driver to work on bL systems, we should get this patch merged:
Thanks pointing out this.
Quoted 8 lines
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.
Quoted 8 lines
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?
Quoted 4 lines
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).
Quoted 4 lines
There is nothing to be updated (ie CPUidle will never shut off the PDSOC power domain), the system counter still counts on cpu and cluster power off, but the logic implementing the arch timer in the cpu (ie comparators) is lost on power off, so the arch timer has to be offloaded to the broadcast device.
Thanks for confirmation. This will be helpful for later enabling cpuidle on hikey board (eight CA53 cores)...
Thanks,
Leo Yan
PATCH 1/1 arm64: dts: Add idle-states for Juno
In reply to Jon Medhurst (Tixy)Quoted 34 lines · Jon Medhurst (Tixy)
On Fri, 2015-05-01 at 09:55 +0800, Leo Yan wrote:
[...]Quoted 11 lines
Quoted 11 lines
arch/arm64/boot/dts/arm/juno.dts | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)arch/arm64/boot/dts/arm/juno.dts +0
-0@@ -34,12 +34,35 @@ #address-cells = <2>; #size-cells = <0>;Quoted 11 lines
arch/arm64/boot/dts/arm/juno.dts line 43
Quoted 7 lines
arch/arm64/boot/dts/arm/juno.dts +7
-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.
Quoted 3 lines
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?
Quoted 6 lines
Do you mean that if the local timer is not lost (and so we should not have local-timer-stop above), then we should have another identical idle state except that it _does_ specify local-timer-stop to force the broadcast time to be used? If so, would that second state ever be more power efficient than the first? (I don't know the answer to that, this whole area is pretty new to me).
Yes, the second state will have better power efficient. But if the arch timer can keep working after the cpu is powered off, we also can get benefit for cpuidle's latency, this is because s/w don't need migrate the cpu timers to broad cast timer list anymore.
Thanks,
Leo Yan
PATCH 1/1 arm64: dts: Add idle-states for Juno
In reply to Jon Medhurst (Tixy)Quoted 3 lines · Jon Medhurst (Tixy)
From: Jon Medhurst <[email protected]>
Signed-off-by Jon Medhurst <[email protected]>
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?
Quoted 89 lines
---
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(+)arch/arm64/boot/dts/arm/juno.dts +28
-0@@ -34,12 +34,35 @@ #address-cells = <2>; #size-cells = <0>; + idle-states { + entry-method = "arm,psci"; + + CPU_SLEEP_0: cpu-sleep-0 { + compatible = "arm,idle-state"; + arm,psci-suspend-param = <0x0010000>; + local-timer-stop; + entry-latency-us = <100>; + exit-latency-us = <250>; + min-residency-us = <2000>; + }; + + CLUSTER_SLEEP_0: cluster-sleep-0 { + compatible = "arm,idle-state"; + arm,psci-suspend-param = <0x1010000>; + local-timer-stop; + entry-latency-us = <800>; + exit-latency-us = <700>; + min-residency-us = <2500>; + }; + }; + A57_0: cpu at 0 { compatible = "arm,cortex-a57","arm,armv8"; reg = <0x0 0x0>; device_type = "cpu"; enable-method = "psci"; next-level-cache = <&A57_L2>; + cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>; }; A57_1: cpu at 1 {@@ -48,6 +71,7 @@ device_type = "cpu"; enable-method = "psci"; next-level-cache = <&A57_L2>; + cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>; }; A53_0: cpu at 100 {@@ -56,6 +80,7 @@ device_type = "cpu"; enable-method = "psci"; next-level-cache = <&A53_L2>; + cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>; }; A53_1: cpu at 101 {@@ -64,6 +89,7 @@ device_type = "cpu"; enable-method = "psci"; next-level-cache = <&A53_L2>; + cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>; }; A53_2: cpu at 102 {@@ -72,6 +98,7 @@ device_type = "cpu"; enable-method = "psci"; next-level-cache = <&A53_L2>; + cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>; }; A53_3: cpu at 103 {@@ -80,6 +107,7 @@ device_type = "cpu"; enable-method = "psci"; next-level-cache = <&A53_L2>; + cpu-idle-states = <&CPU_SLEEP_0 &CLUSTER_SLEEP_0>; }; A57_L2: l2-cache0 {
PATCH 1/1 arm64: dts: Add idle-states for Juno
Quoted 6 lines · Punit Agrawal
Quoted 3 lines · Jon Medhurst (Tixy)
From: Jon Medhurst <[email protected]>
Signed-off-by Jon Medhurst <[email protected]>
Quoted 7 lines
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/)
Signature · 1 line
Tixy
PATCH 1/1 arm64: dts: Add idle-states for Juno
Quoted 23 lines · Jon Medhurst (Tixy)
Quoted 6 lines · Punit Agrawal
Quoted 3 lines · Jon Medhurst (Tixy)
From: Jon Medhurst <[email protected]>
Signed-off-by Jon Medhurst <[email protected]>Quoted 7 lines
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?).
Quoted 6 lines
In an effort to forestall that I would say: does anyone actually care if the values are optimal? Juno is a reference platform and powered off mains, so tuning for the optimum power consumption is pretty pointless. But because it _is_ used as a reference by people it should at least have these features enabled, to serve as an example, and for test coverage.
I agree with you here, let me check the entry/exit latencies again to make sure they are reasonably set-up, it is 4.5 material anyway.
Thanks,
Lorenzo
PATCH 1/1 arm64: dts: Add idle-states for Juno
Quoted 31 lines · Lorenzo Pieralisi
Quoted 23 lines · Jon Medhurst (Tixy)
Quoted 6 lines · Punit Agrawal
Quoted 3 lines · Jon Medhurst (Tixy)
From: Jon Medhurst <[email protected]>
Signed-off-by Jon Medhurst <[email protected]>Quoted 7 lines
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?).
Quoted 6 lines
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.
Quoted 2 lines
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
Quoted 8 lines
Thanks,
Lorenzo_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH 1/1 arm64: dts: Add idle-states for Juno
Quoted 33 lines · Punit Agrawal
Quoted 31 lines · Lorenzo Pieralisi
Quoted 29 lines · Jon Medhurst (Tixy)
Quoted 6 lines · Punit Agrawal
Quoted 3 lines · Jon Medhurst (Tixy)
From: Jon Medhurst <[email protected]>
Signed-off-by Jon Medhurst <[email protected]>Quoted 7 lines
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.
Quoted 7 lines
Quoted 2 lines
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:
PATCH 1/1 arm64: dts: Add idle-states for Juno
Quoted 50 lines · Lorenzo Pieralisi
Quoted 33 lines · Punit Agrawal
Quoted 31 lines · Lorenzo Pieralisi
Quoted 29 lines · Jon Medhurst (Tixy)
Quoted 6 lines · Punit Agrawal
Quoted 3 lines · Jon Medhurst (Tixy)
From: Jon Medhurst <[email protected]>
Signed-off-by Jon Medhurst <[email protected]>Quoted 7 lines
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.
Quoted 7 lines
Quoted 2 lines
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
Quoted 7 lines
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:
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.
Quoted 5 lines
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel