[PATCH v3] ARM: mvebu: Disable CPU Idle on Armada 38x

[PATCH v3] ARM: mvebu: Disable CPU Idle on Armada 38x

[Gregory CLEMENT]

On Armada 38x SoCs, under heavy I/O load, the system hangs when CPU
Idle is enabled. Waiting for a solution to this issue, this patch
disables the CPU Idle support for this SoC.

As CPU Hot plug support also uses some of the CPU Idle functions it is
also affected by the same issue. This patch disables it also for the
Armada 38x SoCs.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
Tested-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: <stable@vger.kernel.org> # v3.17 +
---
Hi,

In this version I removed the unneeded initialization of the ret
variable, I also fixed the warning message, and finally I added the
Tested-by from Thomas.

Gregory

 arch/arm/mach-mvebu/pmsu.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c
index 8b9f5e202ccf..4f4e22206ae5 100644
--- a/arch/arm/mach-mvebu/pmsu.c
+++ b/arch/arm/mach-mvebu/pmsu.c
@@ -415,6 +415,9 @@ static __init int armada_38x_cpuidle_init(void)
 	void __iomem *mpsoc_base;
 	u32 reg;
 
+	pr_warn("CPU idle is currently broken on Armada 38x: disabling");
+	return 0;
+
 	np = of_find_compatible_node(NULL, NULL,
 				     "marvell,armada-380-coherency-fabric");
 	if (!np)
@@ -476,6 +479,16 @@ static int __init mvebu_v7_cpu_pm_init(void)
 		return 0;
 	of_node_put(np);
 
+	/*
+	 * Currently the CPU idle support for Armada 38x is broken, as
+	 * the CPU hotplug uses some of the CPU idle functions it is
+	 * broken too, so let's disable it
+	 */
+	if (of_machine_is_compatible("marvell,armada380")) {
+		cpu_hotplug_disable();
+		pr_warn("CPU hotplug support is currently broken on Armada 38x: disabling");
+	}
+
 	if (of_machine_is_compatible("marvell,armadaxp"))
 		ret = armada_xp_cpuidle_init();
 	else if (of_machine_is_compatible("marvell,armada370"))
@@ -489,7 +502,8 @@ static int __init mvebu_v7_cpu_pm_init(void)
 		return ret;
 
 	mvebu_v7_pmsu_enable_l2_powerdown_onidle();
-	platform_device_register(&mvebu_v7_cpuidle_device);
+	if (mvebu_v7_cpuidle_device.name)
+		platform_device_register(&mvebu_v7_cpuidle_device);
 	cpu_pm_register_notifier(&mvebu_v7_cpu_pm_notifier);
 
 	return 0;
-- 
2.1.0

Re: [PATCH v3] ARM: mvebu: Disable CPU Idle on Armada 38x

[Russell King - ARM Linux]

On Tue, Mar 31, 2015 at 06:46:18PM +0200, Gregory CLEMENT wrote:
> On Armada 38x SoCs, under heavy I/O load, the system hangs when CPU
> Idle is enabled. Waiting for a solution to this issue, this patch
> disables the CPU Idle support for this SoC.
> 
> As CPU Hot plug support also uses some of the CPU Idle functions it is
> also affected by the same issue. This patch disables it also for the
> Armada 38x SoCs.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> Tested-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: <stable@vger.kernel.org> # v3.17 +
> ---
> Hi,
> 
> In this version I removed the unneeded initialization of the ret
> variable, I also fixed the warning message, and finally I added the
> Tested-by from Thomas.

In order not to lose the results of my testing on Armada 388, and to
trigger a discussion, I'll reply to this mail.

With the Armada 388 board I have, running mainline 4.3-rc3, I see the
following board current and power consumption:

CPU idle state					Current	Power
Patch unreverted				0.33A	3.9W
Patch reverted					0.42A	5W
Patch reverted, state 1 disabled on one CPU	0.37A	4.4W
Patch reverted, state 1 disabled on both CPUs	0.33A	3.9W
Patch reverted, state 1 disabled on both CPUs,	0.37A	4.4W
CPU 1 hot-unplugged
Marvell Vendor-based kernel with CPU idle off	0.37A	4.4W
(I don't have a vendor kernel with CPU idle enabled)

This is with a mainline 4.3-rc3 kernel with my own patch set on top,
which includes things like EEE support giving a 360mW saving compared
to kernels without.

The result is a 30% _increase_ in power consumption when CPU idle is
enabled.  For reference, when placing both CPUs into a loop via
while :; do :; done in bash, the consumption goes up to 0.5A / 6W.

Thomas suggested to increase the CPU idle thresholds in a similar manner
to that done for Armada XP in ce6031c89a35 ("cpuidle: mvebu: Update
cpuidle thresholds for Armada XP SOCs"), so I made the following change:

@@ -92,9 +92,9 @@ static struct cpuidle_driver armada38x_idle_driver = {
-               .exit_latency           = 10,
+               .exit_latency           = 100,
-               .target_residency       = 100,
+               .target_residency       = 1000,

This had no effect on the power usage.

This brings up the obvious question, is there a bug somewhere, resulting
in suspending the CPU not resulting in power savings?

I don't think so.  I've fixed the failure path in the mvebu CPU idle code
as per the RFC I sent out earlier this morning, which means if we fail to
suspend the CPU, time will not be accounted to CPU idle state 1 - this
means we can be sure that the statistics for state 1 won't be incorrectly
incremented.  The CPU idle statistics indicate that we are hitting the
lower power state 1, and spending some time there for each call:

/sys/devices/system/cpu/cpu0/cpuidle/state0/time:24726278
/sys/devices/system/cpu/cpu0/cpuidle/state0/usage:29991
/sys/devices/system/cpu/cpu0/cpuidle/state1/time:290135931
/sys/devices/system/cpu/cpu0/cpuidle/state1/usage:74951
/sys/devices/system/cpu/cpu1/cpuidle/state0/time:41096682
/sys/devices/system/cpu/cpu1/cpuidle/state0/usage:141538
/sys/devices/system/cpu/cpu1/cpuidle/state1/time:270641329
/sys/devices/system/cpu/cpu1/cpuidle/state1/usage:47845

So we're probably _not_ looping trying to enter state 1 and immediately
coming back out.  In other words, we are suspending the CPU, and the CPU
is staying suspended for a period of time.

I think we have to conclude that we are successfully suspending the CPU,
and that it is resuming through armada_38x_cpu_resume() and cpu_resume()
as we expect, and the result if placing the CPU into suspend actually
results in higher power consumption than placing the CPU into WFI mode.

It could be that some register isn't set correctly to allow the CPU
power domain to be properly shut down.  It could also be that the SoC
doesn't implement the isolation barrier between the CPU and surrounding
circuitry, resulting in higher leakage when the CPU is placed into
suspend mode.  (The SCU power register is always present, but the
implementation of the isolation barriers to allow individual CPUs to be
independently powered down are optional.)  Either way, without
documentation, there's no way to know.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH v3] ARM: mvebu: Disable CPU Idle on Armada 38x

[Russell King - ARM Linux]

On Tue, Mar 31, 2015 at 06:46:18PM +0200, Gregory CLEMENT wrote:
> On Armada 38x SoCs, under heavy I/O load, the system hangs when CPU
> Idle is enabled. Waiting for a solution to this issue, this patch
> disables the CPU Idle support for this SoC.
> 
> As CPU Hot plug support also uses some of the CPU Idle functions it is
> also affected by the same issue. This patch disables it also for the
> Armada 38x SoCs.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> Tested-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: <stable@vger.kernel.org> # v3.17 +
> ---
> Hi,
> 
> In this version I removed the unneeded initialization of the ret
> variable, I also fixed the warning message, and finally I added the
> Tested-by from Thomas.

In order not to lose the results of my testing on Armada 388, and to
trigger a discussion, I'll reply to this mail.

With the Armada 388 board I have, running mainline 4.3-rc3, I see the
following board current and power consumption:

CPU idle state					Current	Power
Patch unreverted				0.33A	3.9W
Patch reverted					0.42A	5W
Patch reverted, state 1 disabled on one CPU	0.37A	4.4W
Patch reverted, state 1 disabled on both CPUs	0.33A	3.9W
Patch reverted, state 1 disabled on both CPUs,	0.37A	4.4W
CPU 1 hot-unplugged
Marvell Vendor-based kernel with CPU idle off	0.37A	4.4W
(I don't have a vendor kernel with CPU idle enabled)

This is with a mainline 4.3-rc3 kernel with my own patch set on top,
which includes things like EEE support giving a 360mW saving compared
to kernels without.

The result is a 30% _increase_ in power consumption when CPU idle is
enabled.  For reference, when placing both CPUs into a loop via
while :; do :; done in bash, the consumption goes up to 0.5A / 6W.

Thomas suggested to increase the CPU idle thresholds in a similar manner
to that done for Armada XP in ce6031c89a35 ("cpuidle: mvebu: Update
cpuidle thresholds for Armada XP SOCs"), so I made the following change:

@@ -92,9 +92,9 @@ static struct cpuidle_driver armada38x_idle_driver = {
-               .exit_latency           = 10,
+               .exit_latency           = 100,
-               .target_residency       = 100,
+               .target_residency       = 1000,

This had no effect on the power usage.

This brings up the obvious question, is there a bug somewhere, resulting
in suspending the CPU not resulting in power savings?

I don't think so.  I've fixed the failure path in the mvebu CPU idle code
as per the RFC I sent out earlier this morning, which means if we fail to
suspend the CPU, time will not be accounted to CPU idle state 1 - this
means we can be sure that the statistics for state 1 won't be incorrectly
incremented.  The CPU idle statistics indicate that we are hitting the
lower power state 1, and spending some time there for each call:

/sys/devices/system/cpu/cpu0/cpuidle/state0/time:24726278
/sys/devices/system/cpu/cpu0/cpuidle/state0/usage:29991
/sys/devices/system/cpu/cpu0/cpuidle/state1/time:290135931
/sys/devices/system/cpu/cpu0/cpuidle/state1/usage:74951
/sys/devices/system/cpu/cpu1/cpuidle/state0/time:41096682
/sys/devices/system/cpu/cpu1/cpuidle/state0/usage:141538
/sys/devices/system/cpu/cpu1/cpuidle/state1/time:270641329
/sys/devices/system/cpu/cpu1/cpuidle/state1/usage:47845

So we're probably _not_ looping trying to enter state 1 and immediately
coming back out.  In other words, we are suspending the CPU, and the CPU
is staying suspended for a period of time.

I think we have to conclude that we are successfully suspending the CPU,
and that it is resuming through armada_38x_cpu_resume() and cpu_resume()
as we expect, and the result if placing the CPU into suspend actually
results in higher power consumption than placing the CPU into WFI mode.

It could be that some register isn't set correctly to allow the CPU
power domain to be properly shut down.  It could also be that the SoC
doesn't implement the isolation barrier between the CPU and surrounding
circuitry, resulting in higher leakage when the CPU is placed into
suspend mode.  (The SCU power register is always present, but the
implementation of the isolation barriers to allow individual CPUs to be
independently powered down are optional.)  Either way, without
documentation, there's no way to know.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v3] ARM: mvebu: Disable CPU Idle on Armada 38x

[Gregory CLEMENT]

Hi Russell King,
 
 On sam., oct. 03 2015, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Tue, Mar 31, 2015 at 06:46:18PM +0200, Gregory CLEMENT wrote:
>> On Armada 38x SoCs, under heavy I/O load, the system hangs when CPU
>> Idle is enabled. Waiting for a solution to this issue, this patch
>> disables the CPU Idle support for this SoC.
>> 
>> As CPU Hot plug support also uses some of the CPU Idle functions it is
>> also affected by the same issue. This patch disables it also for the
>> Armada 38x SoCs.
>> 
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> Tested-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
>> Cc: <stable@vger.kernel.org> # v3.17 +
>> ---
>> Hi,
>> 
>> In this version I removed the unneeded initialization of the ret
>> variable, I also fixed the warning message, and finally I added the
>> Tested-by from Thomas.
>
> In order not to lose the results of my testing on Armada 388, and to
> trigger a discussion, I'll reply to this mail.
>
> With the Armada 388 board I have, running mainline 4.3-rc3, I see the
> following board current and power consumption:
>
> CPU idle state					Current	Power
> Patch unreverted				0.33A	3.9W
> Patch reverted					0.42A	5W
> Patch reverted, state 1 disabled on one CPU	0.37A	4.4W
> Patch reverted, state 1 disabled on both CPUs	0.33A	3.9W
> Patch reverted, state 1 disabled on both CPUs,	0.37A	4.4W
> CPU 1 hot-unplugged
> Marvell Vendor-based kernel with CPU idle off	0.37A	4.4W
> (I don't have a vendor kernel with CPU idle enabled)
>
> This is with a mainline 4.3-rc3 kernel with my own patch set on top,
> which includes things like EEE support giving a 360mW saving compared
> to kernels without.
>
> The result is a 30% _increase_ in power consumption when CPU idle is
> enabled.  For reference, when placing both CPUs into a loop via
> while :; do :; done in bash, the consumption goes up to 0.5A / 6W.
>
> Thomas suggested to increase the CPU idle thresholds in a similar manner
> to that done for Armada XP in ce6031c89a35 ("cpuidle: mvebu: Update
> cpuidle thresholds for Armada XP SOCs"), so I made the following change:
>
> @@ -92,9 +92,9 @@ static struct cpuidle_driver armada38x_idle_driver = {
> -               .exit_latency           = 10,
> +               .exit_latency           = 100,
> -               .target_residency       = 100,
> +               .target_residency       = 1000,
>
> This had no effect on the power usage.
>
> This brings up the obvious question, is there a bug somewhere, resulting
> in suspending the CPU not resulting in power savings?
>
> I don't think so.  I've fixed the failure path in the mvebu CPU idle code
> as per the RFC I sent out earlier this morning, which means if we fail to
> suspend the CPU, time will not be accounted to CPU idle state 1 - this
> means we can be sure that the statistics for state 1 won't be incorrectly
> incremented.  The CPU idle statistics indicate that we are hitting the
> lower power state 1, and spending some time there for each call:
>
> /sys/devices/system/cpu/cpu0/cpuidle/state0/time:24726278
> /sys/devices/system/cpu/cpu0/cpuidle/state0/usage:29991
> /sys/devices/system/cpu/cpu0/cpuidle/state1/time:290135931
> /sys/devices/system/cpu/cpu0/cpuidle/state1/usage:74951
> /sys/devices/system/cpu/cpu1/cpuidle/state0/time:41096682
> /sys/devices/system/cpu/cpu1/cpuidle/state0/usage:141538
> /sys/devices/system/cpu/cpu1/cpuidle/state1/time:270641329
> /sys/devices/system/cpu/cpu1/cpuidle/state1/usage:47845
>
> So we're probably _not_ looping trying to enter state 1 and immediately
> coming back out.  In other words, we are suspending the CPU, and the CPU
> is staying suspended for a period of time.
>
> I think we have to conclude that we are successfully suspending the CPU,
> and that it is resuming through armada_38x_cpu_resume() and cpu_resume()
> as we expect, and the result if placing the CPU into suspend actually
> results in higher power consumption than placing the CPU into WFI
> mode.

I finally managed to measure the power consumption on my Armada 388 GP
board and I confirm what you saw. With CPU idle disable in pmsu.c it
consumes 0.43A@12V and when it is enable it consumes 0.53A@12V so around
30% more.

If I disable the state1 on a CPU it goes down 0.49@12, and if I disable
the state1 opn both CPU, then it comes back to 0.43A@12V.

>
> It could be that some register isn't set correctly to allow the CPU
> power domain to be properly shut down.  It could also be that the SoC
> doesn't implement the isolation barrier between the CPU and surrounding
> circuitry, resulting in higher leakage when the CPU is placed into
> suspend mode.  (The SCU power register is always present, but the
> implementation of the isolation barriers to allow individual CPUs to be
> independently powered down are optional.)  Either way, without
> documentation, there's no way to know.

Now that I can reproduce it, I will investigate to find what we do
wrong.

Thanks,

Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH v3] ARM: mvebu: Disable CPU Idle on Armada 38x

[Gregory CLEMENT]

Hi Russell King,
 
 On sam., oct. 03 2015, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Tue, Mar 31, 2015 at 06:46:18PM +0200, Gregory CLEMENT wrote:
>> On Armada 38x SoCs, under heavy I/O load, the system hangs when CPU
>> Idle is enabled. Waiting for a solution to this issue, this patch
>> disables the CPU Idle support for this SoC.
>> 
>> As CPU Hot plug support also uses some of the CPU Idle functions it is
>> also affected by the same issue. This patch disables it also for the
>> Armada 38x SoCs.
>> 
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> Tested-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
>> Cc: <stable@vger.kernel.org> # v3.17 +
>> ---
>> Hi,
>> 
>> In this version I removed the unneeded initialization of the ret
>> variable, I also fixed the warning message, and finally I added the
>> Tested-by from Thomas.
>
> In order not to lose the results of my testing on Armada 388, and to
> trigger a discussion, I'll reply to this mail.
>
> With the Armada 388 board I have, running mainline 4.3-rc3, I see the
> following board current and power consumption:
>
> CPU idle state					Current	Power
> Patch unreverted				0.33A	3.9W
> Patch reverted					0.42A	5W
> Patch reverted, state 1 disabled on one CPU	0.37A	4.4W
> Patch reverted, state 1 disabled on both CPUs	0.33A	3.9W
> Patch reverted, state 1 disabled on both CPUs,	0.37A	4.4W
> CPU 1 hot-unplugged
> Marvell Vendor-based kernel with CPU idle off	0.37A	4.4W
> (I don't have a vendor kernel with CPU idle enabled)
>
> This is with a mainline 4.3-rc3 kernel with my own patch set on top,
> which includes things like EEE support giving a 360mW saving compared
> to kernels without.
>
> The result is a 30% _increase_ in power consumption when CPU idle is
> enabled.  For reference, when placing both CPUs into a loop via
> while :; do :; done in bash, the consumption goes up to 0.5A / 6W.
>
> Thomas suggested to increase the CPU idle thresholds in a similar manner
> to that done for Armada XP in ce6031c89a35 ("cpuidle: mvebu: Update
> cpuidle thresholds for Armada XP SOCs"), so I made the following change:
>
> @@ -92,9 +92,9 @@ static struct cpuidle_driver armada38x_idle_driver = {
> -               .exit_latency           = 10,
> +               .exit_latency           = 100,
> -               .target_residency       = 100,
> +               .target_residency       = 1000,
>
> This had no effect on the power usage.
>
> This brings up the obvious question, is there a bug somewhere, resulting
> in suspending the CPU not resulting in power savings?
>
> I don't think so.  I've fixed the failure path in the mvebu CPU idle code
> as per the RFC I sent out earlier this morning, which means if we fail to
> suspend the CPU, time will not be accounted to CPU idle state 1 - this
> means we can be sure that the statistics for state 1 won't be incorrectly
> incremented.  The CPU idle statistics indicate that we are hitting the
> lower power state 1, and spending some time there for each call:
>
> /sys/devices/system/cpu/cpu0/cpuidle/state0/time:24726278
> /sys/devices/system/cpu/cpu0/cpuidle/state0/usage:29991
> /sys/devices/system/cpu/cpu0/cpuidle/state1/time:290135931
> /sys/devices/system/cpu/cpu0/cpuidle/state1/usage:74951
> /sys/devices/system/cpu/cpu1/cpuidle/state0/time:41096682
> /sys/devices/system/cpu/cpu1/cpuidle/state0/usage:141538
> /sys/devices/system/cpu/cpu1/cpuidle/state1/time:270641329
> /sys/devices/system/cpu/cpu1/cpuidle/state1/usage:47845
>
> So we're probably _not_ looping trying to enter state 1 and immediately
> coming back out.  In other words, we are suspending the CPU, and the CPU
> is staying suspended for a period of time.
>
> I think we have to conclude that we are successfully suspending the CPU,
> and that it is resuming through armada_38x_cpu_resume() and cpu_resume()
> as we expect, and the result if placing the CPU into suspend actually
> results in higher power consumption than placing the CPU into WFI
> mode.

I finally managed to measure the power consumption on my Armada 388 GP
board and I confirm what you saw. With CPU idle disable in pmsu.c it
consumes 0.43A at 12V and when it is enable it consumes 0.53A at 12V so around
30% more.

If I disable the state1 on a CPU it goes down 0.49 at 12, and if I disable
the state1 opn both CPU, then it comes back to 0.43A at 12V.

>
> It could be that some register isn't set correctly to allow the CPU
> power domain to be properly shut down.  It could also be that the SoC
> doesn't implement the isolation barrier between the CPU and surrounding
> circuitry, resulting in higher leakage when the CPU is placed into
> suspend mode.  (The SCU power register is always present, but the
> implementation of the isolation barriers to allow individual CPUs to be
> independently powered down are optional.)  Either way, without
> documentation, there's no way to know.

Now that I can reproduce it, I will investigate to find what we do
wrong.

Thanks,

Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com