Powering off idle CPUs saves about 33 mW compared to using WFI only.
Additional power savings are possible by idling the L2 and downclocking
the cluster when all CPUs are idle.
Entry and exit latency were measured using a logic analyzer, with GPIO
pins toggled in Linux after the calls to trace_cpu_idle() in
cpuidle_enter_state(), and in the power management firmware after CPU
power-off completes and immediately after detecting an interrupt.
800 us and 1500 us are worst-case values, largely driven by the fact
that the power management firmware is single threaded. It can only
handle commands to power off CPUs one at a time, and it cannot process
any commands while powering on a CPU in response to an interrupt.
The cluster suspend process reliably takes 36 us; I rounded this up to
50 us. If all CPUs enter the cluster idle state at the same time, exit
latency is actually reduced, because there is no contention in that
case. However, if only some CPUs enter the cluster idle state, behavior
is the same as for CPU idle.
Polling delay for the power management firmware to detect a pending
interrupt is insignificant; it is less than 20 us.
min-residency was chosen as the point where enabling the idle state
consumed no more average power than disabling the idle state at a
variety of interrupt rates.
Signed-off-by: Samuel Holland <[email protected]>
---
I'm sending this patch as an RFC because it raises questions about how
we handle firmware versioning. How far back does (or should) our support
for old TF-A and Crust versions go?
cpuidle has a problem that without working firmware support, CPUs will
enter idle states and be unable to wake up. As a result, the system will
hang at some point during boot, usually before getting to userspace.
For over a year[0], TF-A has exposed the PSCI CPU_SUSPEND function when
a SCPI implementation is present[1]. Implementing CPU_SUSPEND is
required for implementing SYSTEM_SUSPEND[2], even if CPU_SUSPEND is not
itself used for anything.
However, there was no code to actually wake up a CPU once it called the
CPU_SUSPEND function, because I could not find the register providing
the necessary information. The fact that CPU_SUSPEND was broken affected
nobody, because nothing ever called it -- there were no idle states in
the DTS. In hindsight, what I should have done was always return failure
from sunxi_validate_power_state(), but that ship has long sailed.
I finally found the elusive register and implemented the wakeup code
earlier this month[3]. So now, CPU_SUSPEND actually works, if all of
your firmware is up to date, and cpuidle works if you add the states in
your device tree.
Unfortunately, there is currently nothing verifying that compatibility.
So you can get into four possible scenarios:
1) No idle states in DTS, any firmware => Linux works, with baseline
power consumption.
2) Idle states added to DTS, no Crust/SCPI => Linux works, but every
attempt to enter an idle state is rejected because CPU_SUSPEND is
not hooked up. So power consumption increases by a sizable amount.
3) Idle states added to DTS, "old" Crust/SCPI (before [3]) => Linux
fails to boot, because CPUs never return from idle states.
4) Idle states added to DTS, "new" Crust/SCPI (after [3]) => Linux
works, with improved power consumption compared to the baseline.
Obviously, we want to prevent scenario 3 if possible.
Enter the current patch: I chose the arm,psci-suspend-param values
specifically so they would be _rejected_ by the current TF-A code. This
makes scenario 3 behave like scenario 2. I then have some follow-up TF-A
patches (not yet submitted) to switch to the new parameter encoding[4].
This brings me back to my original question. Once the TF-A patches in
[4] are merged, scenario 3 (with an updated TF-A but an old Crust) would
fail to boot again. Do we care?
Should I implement some kind of runtime version checking, so TF-A can
disable CPU_SUSPEND if it would be broken? Or instead, should we wait
some amount of time to merge this patch (or the patches at [4]) and
assume people have upgraded?
Where would people expect this sort of possibly-breaking change to be
documented?
Separately, since I assume most A64/H5 users (outside of LibreELEC and
the PinePhone) are not using Crust, scenario 2 would be very common. If
merging this patch increases their idle power draw by 500 mW, is that an
acceptable cost for decreasing other users' idle power draw by 50 mW?
Sorry for the wall of text,
Samuel
[0]: https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/commit/plat/allwinner/common/sunxi_pm.c?id=e382c88e2a26995099bb931d49e754dcaebc5593
[1]: https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/plat/allwinner/common/sunxi_scpi_pm.c?id=2e0e51f42586826a1f6f6c1e532f90e6df642cf5#n190
[2]: https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/lib/psci/psci_setup.c?id=2e0e51f42586826a1f6f6c1e532f90e6df642cf5#n251
[3]: https://github.com/crust-firmware/crust/commits/85944467c804
[4]: https://github.com/crust-firmware/arm-trusted-firmware/commits/d6ebf5dab2da
---
arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 26 +++++++++++++++++++
arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi | 26 +++++++++++++++++++
2 files changed, 52 insertions(+)
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index 57786fc120c3..2b1b5b36098c 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -54,6 +54,7 @@ cpu0: cpu@0 {
clocks = <&ccu CLK_CPUX>;
clock-names = "cpu";
#cooling-cells = <2>;
+ cpu-idle-states = <&cpu_sleep>, <&cluster_sleep>;
};
cpu1: cpu@1 {
@@ -65,6 +66,7 @@ cpu1: cpu@1 {
clocks = <&ccu CLK_CPUX>;
clock-names = "cpu";
#cooling-cells = <2>;
+ cpu-idle-states = <&cpu_sleep>, <&cluster_sleep>;
};
cpu2: cpu@2 {
@@ -76,6 +78,7 @@ cpu2: cpu@2 {
clocks = <&ccu CLK_CPUX>;
clock-names = "cpu";
#cooling-cells = <2>;
+ cpu-idle-states = <&cpu_sleep>, <&cluster_sleep>;
};
cpu3: cpu@3 {
@@ -87,6 +90,29 @@ cpu3: cpu@3 {
clocks = <&ccu CLK_CPUX>;
clock-names = "cpu";
#cooling-cells = <2>;
+ cpu-idle-states = <&cpu_sleep>, <&cluster_sleep>;
+ };
+
+ idle-states {
+ entry-method = "psci";
+
+ cpu_sleep: cpu-sleep {
+ compatible = "arm,idle-state";
+ local-timer-stop;
+ entry-latency-us = <800>;
+ exit-latency-us = <1500>;
+ min-residency-us = <25000>;
+ arm,psci-suspend-param = <0x00010003>;
+ };
+
+ cluster_sleep: cluster-sleep {
+ compatible = "arm,idle-state";
+ local-timer-stop;
+ entry-latency-us = <850>;
+ exit-latency-us = <1500>;
+ min-residency-us = <50000>;
+ arm,psci-suspend-param = <0x01010013>;
+ };
};
L2: l2-cache {
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi
index 578a63dedf46..1c416f648c58 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi
@@ -18,6 +18,7 @@ cpu0: cpu@0 {
clocks = <&ccu CLK_CPUX>;
clock-latency-ns = <244144>; /* 8 32k periods */
#cooling-cells = <2>;
+ cpu-idle-states = <&cpu_sleep>, <&cluster_sleep>;
};
cpu1: cpu@1 {
@@ -28,6 +29,7 @@ cpu1: cpu@1 {
clocks = <&ccu CLK_CPUX>;
clock-latency-ns = <244144>; /* 8 32k periods */
#cooling-cells = <2>;
+ cpu-idle-states = <&cpu_sleep>, <&cluster_sleep>;
};
cpu2: cpu@2 {
@@ -38,6 +40,7 @@ cpu2: cpu@2 {
clocks = <&ccu CLK_CPUX>;
clock-latency-ns = <244144>; /* 8 32k periods */
#cooling-cells = <2>;
+ cpu-idle-states = <&cpu_sleep>, <&cluster_sleep>;
};
cpu3: cpu@3 {
@@ -48,6 +51,29 @@ cpu3: cpu@3 {
clocks = <&ccu CLK_CPUX>;
clock-latency-ns = <244144>; /* 8 32k periods */
#cooling-cells = <2>;
+ cpu-idle-states = <&cpu_sleep>, <&cluster_sleep>;
+ };
+
+ idle-states {
+ entry-method = "psci";
+
+ cpu_sleep: cpu-sleep {
+ compatible = "arm,idle-state";
+ local-timer-stop;
+ entry-latency-us = <800>;
+ exit-latency-us = <1500>;
+ min-residency-us = <25000>;
+ arm,psci-suspend-param = <0x00010003>;
+ };
+
+ cluster_sleep: cluster-sleep {
+ compatible = "arm,idle-state";
+ local-timer-stop;
+ entry-latency-us = <850>;
+ exit-latency-us = <1500>;
+ min-residency-us = <50000>;
+ arm,psci-suspend-param = <0x01010013>;
+ };
};
};
--
2.26.2
On Mon, 22 Mar 2021 01:25:14 -0500
Samuel Holland <[email protected]> wrote:
Hi,
> Powering off idle CPUs saves about 33 mW compared to using WFI only.
> Additional power savings are possible by idling the L2 and downclocking
> the cluster when all CPUs are idle.
>
> Entry and exit latency were measured using a logic analyzer, with GPIO
> pins toggled in Linux after the calls to trace_cpu_idle() in
> cpuidle_enter_state(), and in the power management firmware after CPU
> power-off completes and immediately after detecting an interrupt.
>
> 800 us and 1500 us are worst-case values, largely driven by the fact
> that the power management firmware is single threaded. It can only
> handle commands to power off CPUs one at a time, and it cannot process
> any commands while powering on a CPU in response to an interrupt.
>
> The cluster suspend process reliably takes 36 us; I rounded this up to
> 50 us. If all CPUs enter the cluster idle state at the same time, exit
> latency is actually reduced, because there is no contention in that
> case. However, if only some CPUs enter the cluster idle state, behavior
> is the same as for CPU idle.
>
> Polling delay for the power management firmware to detect a pending
> interrupt is insignificant; it is less than 20 us.
>
> min-residency was chosen as the point where enabling the idle state
> consumed no more average power than disabling the idle state at a
> variety of interrupt rates.
>
> Signed-off-by: Samuel Holland <[email protected]>
> ---
>
> I'm sending this patch as an RFC because it raises questions about how
> we handle firmware versioning. How far back does (or should) our support
> for old TF-A and Crust versions go?
>
> cpuidle has a problem that without working firmware support, CPUs will
> enter idle states and be unable to wake up. As a result, the system will
> hang at some point during boot, usually before getting to userspace.
>
> For over a year[0], TF-A has exposed the PSCI CPU_SUSPEND function when
> a SCPI implementation is present[1]. Implementing CPU_SUSPEND is
> required for implementing SYSTEM_SUSPEND[2], even if CPU_SUSPEND is not
> itself used for anything.
>
> However, there was no code to actually wake up a CPU once it called the
> CPU_SUSPEND function, because I could not find the register providing
> the necessary information. The fact that CPU_SUSPEND was broken affected
> nobody, because nothing ever called it -- there were no idle states in
> the DTS. In hindsight, what I should have done was always return failure
> from sunxi_validate_power_state(), but that ship has long sailed.
>
> I finally found the elusive register and implemented the wakeup code
> earlier this month[3]. So now, CPU_SUSPEND actually works, if all of
> your firmware is up to date, and cpuidle works if you add the states in
> your device tree.
>
> Unfortunately, there is currently nothing verifying that compatibility.
> So you can get into four possible scenarios:
> 1) No idle states in DTS, any firmware => Linux works, with baseline
> power consumption.
> 2) Idle states added to DTS, no Crust/SCPI => Linux works, but every
> attempt to enter an idle state is rejected because CPU_SUSPEND is
> not hooked up. So power consumption increases by a sizable amount.
> 3) Idle states added to DTS, "old" Crust/SCPI (before [3]) => Linux
> fails to boot, because CPUs never return from idle states.
> 4) Idle states added to DTS, "new" Crust/SCPI (after [3]) => Linux
> works, with improved power consumption compared to the baseline.
>
> Obviously, we want to prevent scenario 3 if possible.
So I think the core of the problem is that the DT describes some
firmware feature, but we have the DT bundled with the kernel, not the
firmware.
So is there any way we can detect an older crust version in U-Boot,
then remove any potential idle states from the DT?
Granted, this requires recent U-Boot as well, but at least we could try
to mitigate the worst case a bit?
A better solution could be to only *add* the idle states if the rest of
the firmware is deemed worthy. So the mainline DTs would not carry the
properties in the first place, and only U-Boot adds them, on detecting
a capable firmware?
Admittedly this changes the "flow" of the DT, where the kernel is the
authority, but it might help to solve this problem?
Or any other way, which involves U-Boot patching the DTB? (This would
apply to the DTB passed to the kernel, regardless of where and when
it's loaded from)
Any opinions?
Cheers,
Andre
> Enter the current patch: I chose the arm,psci-suspend-param values
> specifically so they would be _rejected_ by the current TF-A code. This
> makes scenario 3 behave like scenario 2. I then have some follow-up TF-A
> patches (not yet submitted) to switch to the new parameter encoding[4].
>
> This brings me back to my original question. Once the TF-A patches in
> [4] are merged, scenario 3 (with an updated TF-A but an old Crust) would
> fail to boot again. Do we care?
>
> Should I implement some kind of runtime version checking, so TF-A can
> disable CPU_SUSPEND if it would be broken? Or instead, should we wait
> some amount of time to merge this patch (or the patches at [4]) and
> assume people have upgraded?
>
> Where would people expect this sort of possibly-breaking change to be
> documented?
>
> Separately, since I assume most A64/H5 users (outside of LibreELEC and
> the PinePhone) are not using Crust, scenario 2 would be very common. If
> merging this patch increases their idle power draw by 500 mW, is that an
> acceptable cost for decreasing other users' idle power draw by 50 mW?
>
> Sorry for the wall of text,
> Samuel
>
> [0]: https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/commit/plat/allwinner/common/sunxi_pm.c?id=e382c88e2a26995099bb931d49e754dcaebc5593
> [1]: https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/plat/allwinner/common/sunxi_scpi_pm.c?id=2e0e51f42586826a1f6f6c1e532f90e6df642cf5#n190
> [2]: https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/lib/psci/psci_setup.c?id=2e0e51f42586826a1f6f6c1e532f90e6df642cf5#n251
> [3]: https://github.com/crust-firmware/crust/commits/85944467c804
> [4]: https://github.com/crust-firmware/arm-trusted-firmware/commits/d6ebf5dab2da
>
> ---
>
> arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 26 +++++++++++++++++++
> arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi | 26 +++++++++++++++++++
> 2 files changed, 52 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> index 57786fc120c3..2b1b5b36098c 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> @@ -54,6 +54,7 @@ cpu0: cpu@0 {
> clocks = <&ccu CLK_CPUX>;
> clock-names = "cpu";
> #cooling-cells = <2>;
> + cpu-idle-states = <&cpu_sleep>, <&cluster_sleep>;
> };
>
> cpu1: cpu@1 {
> @@ -65,6 +66,7 @@ cpu1: cpu@1 {
> clocks = <&ccu CLK_CPUX>;
> clock-names = "cpu";
> #cooling-cells = <2>;
> + cpu-idle-states = <&cpu_sleep>, <&cluster_sleep>;
> };
>
> cpu2: cpu@2 {
> @@ -76,6 +78,7 @@ cpu2: cpu@2 {
> clocks = <&ccu CLK_CPUX>;
> clock-names = "cpu";
> #cooling-cells = <2>;
> + cpu-idle-states = <&cpu_sleep>, <&cluster_sleep>;
> };
>
> cpu3: cpu@3 {
> @@ -87,6 +90,29 @@ cpu3: cpu@3 {
> clocks = <&ccu CLK_CPUX>;
> clock-names = "cpu";
> #cooling-cells = <2>;
> + cpu-idle-states = <&cpu_sleep>, <&cluster_sleep>;
> + };
> +
> + idle-states {
> + entry-method = "psci";
> +
> + cpu_sleep: cpu-sleep {
> + compatible = "arm,idle-state";
> + local-timer-stop;
> + entry-latency-us = <800>;
> + exit-latency-us = <1500>;
> + min-residency-us = <25000>;
> + arm,psci-suspend-param = <0x00010003>;
> + };
> +
> + cluster_sleep: cluster-sleep {
> + compatible = "arm,idle-state";
> + local-timer-stop;
> + entry-latency-us = <850>;
> + exit-latency-us = <1500>;
> + min-residency-us = <50000>;
> + arm,psci-suspend-param = <0x01010013>;
> + };
> };
>
> L2: l2-cache {
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi
> index 578a63dedf46..1c416f648c58 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi
> @@ -18,6 +18,7 @@ cpu0: cpu@0 {
> clocks = <&ccu CLK_CPUX>;
> clock-latency-ns = <244144>; /* 8 32k periods */
> #cooling-cells = <2>;
> + cpu-idle-states = <&cpu_sleep>, <&cluster_sleep>;
> };
>
> cpu1: cpu@1 {
> @@ -28,6 +29,7 @@ cpu1: cpu@1 {
> clocks = <&ccu CLK_CPUX>;
> clock-latency-ns = <244144>; /* 8 32k periods */
> #cooling-cells = <2>;
> + cpu-idle-states = <&cpu_sleep>, <&cluster_sleep>;
> };
>
> cpu2: cpu@2 {
> @@ -38,6 +40,7 @@ cpu2: cpu@2 {
> clocks = <&ccu CLK_CPUX>;
> clock-latency-ns = <244144>; /* 8 32k periods */
> #cooling-cells = <2>;
> + cpu-idle-states = <&cpu_sleep>, <&cluster_sleep>;
> };
>
> cpu3: cpu@3 {
> @@ -48,6 +51,29 @@ cpu3: cpu@3 {
> clocks = <&ccu CLK_CPUX>;
> clock-latency-ns = <244144>; /* 8 32k periods */
> #cooling-cells = <2>;
> + cpu-idle-states = <&cpu_sleep>, <&cluster_sleep>;
> + };
> +
> + idle-states {
> + entry-method = "psci";
> +
> + cpu_sleep: cpu-sleep {
> + compatible = "arm,idle-state";
> + local-timer-stop;
> + entry-latency-us = <800>;
> + exit-latency-us = <1500>;
> + min-residency-us = <25000>;
> + arm,psci-suspend-param = <0x00010003>;
> + };
> +
> + cluster_sleep: cluster-sleep {
> + compatible = "arm,idle-state";
> + local-timer-stop;
> + entry-latency-us = <850>;
> + exit-latency-us = <1500>;
> + min-residency-us = <50000>;
> + arm,psci-suspend-param = <0x01010013>;
> + };
> };
> };
>
On 3/22/21 8:56 PM, Andre Przywara wrote:
>> I'm sending this patch as an RFC because it raises questions about how
>> we handle firmware versioning. How far back does (or should) our support
>> for old TF-A and Crust versions go?
>>
>> cpuidle has a problem that without working firmware support, CPUs will
>> enter idle states and be unable to wake up. As a result, the system will
>> hang at some point during boot, usually before getting to userspace.
>>
>> For over a year[0], TF-A has exposed the PSCI CPU_SUSPEND function when
>> a SCPI implementation is present[1]. Implementing CPU_SUSPEND is
>> required for implementing SYSTEM_SUSPEND[2], even if CPU_SUSPEND is not
>> itself used for anything.
>>
>> However, there was no code to actually wake up a CPU once it called the
>> CPU_SUSPEND function, because I could not find the register providing
>> the necessary information. The fact that CPU_SUSPEND was broken affected
>> nobody, because nothing ever called it -- there were no idle states in
>> the DTS. In hindsight, what I should have done was always return failure
>> from sunxi_validate_power_state(), but that ship has long sailed.
>>
>> I finally found the elusive register and implemented the wakeup code
>> earlier this month[3]. So now, CPU_SUSPEND actually works, if all of
>> your firmware is up to date, and cpuidle works if you add the states in
>> your device tree.
>>
>> Unfortunately, there is currently nothing verifying that compatibility.
>> So you can get into four possible scenarios:
>> 1) No idle states in DTS, any firmware => Linux works, with baseline
>> power consumption.
>> 2) Idle states added to DTS, no Crust/SCPI => Linux works, but every
>> attempt to enter an idle state is rejected because CPU_SUSPEND is
>> not hooked up. So power consumption increases by a sizable amount.
>> 3) Idle states added to DTS, "old" Crust/SCPI (before [3]) => Linux
>> fails to boot, because CPUs never return from idle states.
>> 4) Idle states added to DTS, "new" Crust/SCPI (after [3]) => Linux
>> works, with improved power consumption compared to the baseline.
>>
>> Obviously, we want to prevent scenario 3 if possible.
>
> So I think the core of the problem is that the DT describes some
> firmware feature, but we have the DT bundled with the kernel, not the
> firmware.
I would say the core problem is that the firmware lies about supporting
PSCI CPU_SUSPEND. Linux shouldn't be calling CPU_SUSPEND if the firmware
declares it as unavailable, regardless of what is in the DTS.
(Technically, per the PSCI standard, CPU_SUSPEND is a mandatory
function, but a quick survey of the TF-A platforms shows it is far from
universally implemented.)
> So is there any way we can detect an older crust version in U-Boot,
> then remove any potential idle states from the DT?
Let's assume that we are using a functioning SoC (H3) or the secure fuse
is blown (A64) and therefore U-Boot cannot access SRAM A2. I can think
of three ways it can learn about crust:
a) PSCI_FEATURES (e.g. is CPU_SUSPEND supported)
b) Metadata in the FIT image
c) Custom SMCs
TF-A has some additional methods available:
d) The SCPI-reported firmware version
e) The magic number at the beginning of the firmware binary
> Granted, this requires recent U-Boot as well, but at least we could try
> to mitigate the worst case a bit?
If we're okay with modifying firmware to solve this problem, then I
propose the following solution:
1) Version bump crust or change its magic number.
2) Modify TF-A to only report CPU_SUSPEND as available if it detects the
new crust version. This would involve conditionally setting
sunxi_scpi_psci_ops.validate_power_state, and updating psci_setup.c
to also check for .validate_power_state when setting psci_caps.
3) Modify the Linux PSCI client to respect PSCI_FEATURES when setting
psci_ops.cpu_suspend. cpuidle-psci checks for this function before
setting up idle states.
4) Finally, after some time, add the idle states to the DTS.
In fact, this solution solves both scenarios 2 and 3, because it also
takes care of the native PM implementation, which doesn't implement
CPU_SUSPEND at all.
Does that sound workable?
Regards,
Samuel
> A better solution could be to only *add* the idle states if the rest of
> the firmware is deemed worthy. So the mainline DTs would not carry the
> properties in the first place, and only U-Boot adds them, on detecting
> a capable firmware?
> Admittedly this changes the "flow" of the DT, where the kernel is the
> authority, but it might help to solve this problem?
>
> Or any other way, which involves U-Boot patching the DTB? (This would
> apply to the DTB passed to the kernel, regardless of where and when
> it's loaded from)
>
> Any opinions?
>
> Cheers,
> Andre
>
>> Enter the current patch: I chose the arm,psci-suspend-param values
>> specifically so they would be _rejected_ by the current TF-A code. This
>> makes scenario 3 behave like scenario 2. I then have some follow-up TF-A
>> patches (not yet submitted) to switch to the new parameter encoding[4].
>>
>> This brings me back to my original question. Once the TF-A patches in
>> [4] are merged, scenario 3 (with an updated TF-A but an old Crust) would
>> fail to boot again. Do we care?
>>
>> Should I implement some kind of runtime version checking, so TF-A can
>> disable CPU_SUSPEND if it would be broken? Or instead, should we wait
>> some amount of time to merge this patch (or the patches at [4]) and
>> assume people have upgraded?
>>
>> Where would people expect this sort of possibly-breaking change to be
>> documented?
>>
>> Separately, since I assume most A64/H5 users (outside of LibreELEC and
>> the PinePhone) are not using Crust, scenario 2 would be very common. If
>> merging this patch increases their idle power draw by 500 mW, is that an
>> acceptable cost for decreasing other users' idle power draw by 50 mW?
>>
>> Sorry for the wall of text,
>> Samuel
>>
>> [0]: https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/commit/plat/allwinner/common/sunxi_pm.c?id=e382c88e2a26995099bb931d49e754dcaebc5593
>> [1]: https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/plat/allwinner/common/sunxi_scpi_pm.c?id=2e0e51f42586826a1f6f6c1e532f90e6df642cf5#n190
>> [2]: https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/lib/psci/psci_setup.c?id=2e0e51f42586826a1f6f6c1e532f90e6df642cf5#n251
>> [3]: https://github.com/crust-firmware/crust/commits/85944467c804
>> [4]: https://github.com/crust-firmware/arm-trusted-firmware/commits/d6ebf5dab2da
>>
>> ---
>>
>> arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 26 +++++++++++++++++++
>> arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi | 26 +++++++++++++++++++
>> 2 files changed, 52 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>> index 57786fc120c3..2b1b5b36098c 100644
>> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>> @@ -54,6 +54,7 @@ cpu0: cpu@0 {
>> clocks = <&ccu CLK_CPUX>;
>> clock-names = "cpu";
>> #cooling-cells = <2>;
>> + cpu-idle-states = <&cpu_sleep>, <&cluster_sleep>;
>> };
>>
>> cpu1: cpu@1 {
>> @@ -65,6 +66,7 @@ cpu1: cpu@1 {
>> clocks = <&ccu CLK_CPUX>;
>> clock-names = "cpu";
>> #cooling-cells = <2>;
>> + cpu-idle-states = <&cpu_sleep>, <&cluster_sleep>;
>> };
>>
>> cpu2: cpu@2 {
>> @@ -76,6 +78,7 @@ cpu2: cpu@2 {
>> clocks = <&ccu CLK_CPUX>;
>> clock-names = "cpu";
>> #cooling-cells = <2>;
>> + cpu-idle-states = <&cpu_sleep>, <&cluster_sleep>;
>> };
>>
>> cpu3: cpu@3 {
>> @@ -87,6 +90,29 @@ cpu3: cpu@3 {
>> clocks = <&ccu CLK_CPUX>;
>> clock-names = "cpu";
>> #cooling-cells = <2>;
>> + cpu-idle-states = <&cpu_sleep>, <&cluster_sleep>;
>> + };
>> +
>> + idle-states {
>> + entry-method = "psci";
>> +
>> + cpu_sleep: cpu-sleep {
>> + compatible = "arm,idle-state";
>> + local-timer-stop;
>> + entry-latency-us = <800>;
>> + exit-latency-us = <1500>;
>> + min-residency-us = <25000>;
>> + arm,psci-suspend-param = <0x00010003>;
>> + };
>> +
>> + cluster_sleep: cluster-sleep {
>> + compatible = "arm,idle-state";
>> + local-timer-stop;
>> + entry-latency-us = <850>;
>> + exit-latency-us = <1500>;
>> + min-residency-us = <50000>;
>> + arm,psci-suspend-param = <0x01010013>;
>> + };
>> };
>>
>> L2: l2-cache {
>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi
>> index 578a63dedf46..1c416f648c58 100644
>> --- a/arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi
>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h5.dtsi
>> @@ -18,6 +18,7 @@ cpu0: cpu@0 {
>> clocks = <&ccu CLK_CPUX>;
>> clock-latency-ns = <244144>; /* 8 32k periods */
>> #cooling-cells = <2>;
>> + cpu-idle-states = <&cpu_sleep>, <&cluster_sleep>;
>> };
>>
>> cpu1: cpu@1 {
>> @@ -28,6 +29,7 @@ cpu1: cpu@1 {
>> clocks = <&ccu CLK_CPUX>;
>> clock-latency-ns = <244144>; /* 8 32k periods */
>> #cooling-cells = <2>;
>> + cpu-idle-states = <&cpu_sleep>, <&cluster_sleep>;
>> };
>>
>> cpu2: cpu@2 {
>> @@ -38,6 +40,7 @@ cpu2: cpu@2 {
>> clocks = <&ccu CLK_CPUX>;
>> clock-latency-ns = <244144>; /* 8 32k periods */
>> #cooling-cells = <2>;
>> + cpu-idle-states = <&cpu_sleep>, <&cluster_sleep>;
>> };
>>
>> cpu3: cpu@3 {
>> @@ -48,6 +51,29 @@ cpu3: cpu@3 {
>> clocks = <&ccu CLK_CPUX>;
>> clock-latency-ns = <244144>; /* 8 32k periods */
>> #cooling-cells = <2>;
>> + cpu-idle-states = <&cpu_sleep>, <&cluster_sleep>;
>> + };
>> +
>> + idle-states {
>> + entry-method = "psci";
>> +
>> + cpu_sleep: cpu-sleep {
>> + compatible = "arm,idle-state";
>> + local-timer-stop;
>> + entry-latency-us = <800>;
>> + exit-latency-us = <1500>;
>> + min-residency-us = <25000>;
>> + arm,psci-suspend-param = <0x00010003>;
>> + };
>> +
>> + cluster_sleep: cluster-sleep {
>> + compatible = "arm,idle-state";
>> + local-timer-stop;
>> + entry-latency-us = <850>;
>> + exit-latency-us = <1500>;
>> + min-residency-us = <50000>;
>> + arm,psci-suspend-param = <0x01010013>;
>> + };
>> };
>> };
>>
>
Hi Samuel,
On Tue, Mar 23, 2021 at 11:44:50PM -0500, Samuel Holland wrote:
> On 3/22/21 8:56 PM, Andre Przywara wrote:
> >> I'm sending this patch as an RFC because it raises questions about how
> >> we handle firmware versioning. How far back does (or should) our support
> >> for old TF-A and Crust versions go?
> >>
> >> cpuidle has a problem that without working firmware support, CPUs will
> >> enter idle states and be unable to wake up. As a result, the system will
> >> hang at some point during boot, usually before getting to userspace.
> >>
> >> For over a year[0], TF-A has exposed the PSCI CPU_SUSPEND function when
> >> a SCPI implementation is present[1]. Implementing CPU_SUSPEND is
> >> required for implementing SYSTEM_SUSPEND[2], even if CPU_SUSPEND is not
> >> itself used for anything.
> >>
> >> However, there was no code to actually wake up a CPU once it called the
> >> CPU_SUSPEND function, because I could not find the register providing
> >> the necessary information. The fact that CPU_SUSPEND was broken affected
> >> nobody, because nothing ever called it -- there were no idle states in
> >> the DTS. In hindsight, what I should have done was always return failure
> >> from sunxi_validate_power_state(), but that ship has long sailed.
> >>
> >> I finally found the elusive register and implemented the wakeup code
> >> earlier this month[3]. So now, CPU_SUSPEND actually works, if all of
> >> your firmware is up to date, and cpuidle works if you add the states in
> >> your device tree.
> >>
> >> Unfortunately, there is currently nothing verifying that compatibility.
> >> So you can get into four possible scenarios:
> >> 1) No idle states in DTS, any firmware => Linux works, with baseline
> >> power consumption.
> >> 2) Idle states added to DTS, no Crust/SCPI => Linux works, but every
> >> attempt to enter an idle state is rejected because CPU_SUSPEND is
> >> not hooked up. So power consumption increases by a sizable amount.
> >> 3) Idle states added to DTS, "old" Crust/SCPI (before [3]) => Linux
> >> fails to boot, because CPUs never return from idle states.
> >> 4) Idle states added to DTS, "new" Crust/SCPI (after [3]) => Linux
> >> works, with improved power consumption compared to the baseline.
> >>
> >> Obviously, we want to prevent scenario 3 if possible.
> >
> > So I think the core of the problem is that the DT describes some
> > firmware feature, but we have the DT bundled with the kernel, not the
> > firmware.
>
> I would say the core problem is that the firmware lies about supporting
> PSCI CPU_SUSPEND. Linux shouldn't be calling CPU_SUSPEND if the firmware
> declares it as unavailable, regardless of what is in the DTS.
I would say we have two core problems :)
> (Technically, per the PSCI standard, CPU_SUSPEND is a mandatory
> function, but a quick survey of the TF-A platforms shows it is far from
> universally implemented.)
U-boot also implements CPU_SUSPEND but will return -1 if you don't have
an implementation. I guess that at the moment we shouldn't be reporting
it as supported if we don't
But I also agree with Andre here, we shouldn't list cpuidles
capabilities in the DTS if we don't always have them either.
> > So is there any way we can detect an older crust version in U-Boot,
> > then remove any potential idle states from the DT?
>
> Let's assume that we are using a functioning SoC (H3) or the secure fuse
> is blown (A64) and therefore U-Boot cannot access SRAM A2. I can think
> of three ways it can learn about crust:
>
> a) PSCI_FEATURES (e.g. is CPU_SUSPEND supported)
> b) Metadata in the FIT image
> c) Custom SMCs
>
> TF-A has some additional methods available:
>
> d) The SCPI-reported firmware version
> e) The magic number at the beginning of the firmware binary
>
> > Granted, this requires recent U-Boot as well, but at least we could try
> > to mitigate the worst case a bit?
>
> If we're okay with modifying firmware to solve this problem, then I
> propose the following solution:
>
> 1) Version bump crust or change its magic number.
> 2) Modify TF-A to only report CPU_SUSPEND as available if it detects the
> new crust version. This would involve conditionally setting
> sunxi_scpi_psci_ops.validate_power_state, and updating psci_setup.c
> to also check for .validate_power_state when setting psci_caps.
> 3) Modify the Linux PSCI client to respect PSCI_FEATURES when setting
> psci_ops.cpu_suspend. cpuidle-psci checks for this function before
> setting up idle states.
> 4) Finally, after some time, add the idle states to the DTS.
>
> In fact, this solution solves both scenarios 2 and 3, because it also
> takes care of the native PM implementation, which doesn't implement
> CPU_SUSPEND at all.
>
> Does that sound workable?
If we can add the DT node at boot in crust (or in TF-A), then we don't
really need PSCI_FEATURES, and it would magically work once the firmware
is updated. It looks like a solid plan otherwise
Maxime
On Tue, Mar 23, 2021 at 11:44:50PM -0500, Samuel Holland wrote:
> 3) Modify the Linux PSCI client to respect PSCI_FEATURES when setting
> psci_ops.cpu_suspend. cpuidle-psci checks for this function before
> setting up idle states.
We don't call PSCI_FEATURES on mandatory functions as it may not return
anything meaningful for such mandatory functions(as that is not mandated
in the spec). In short NACK to add PSCI_FEATURES check for mandatory
functions.
--
Regards,
Sudeep