Skip to contentSkip to conversation

ARM: EXYNOS: add cpuidle-exynos.max_states kernel parameter

PATCH New Bartlomiej Zolnierkiewicz 2 participants 9 messages Aug 30, 2013 – Sep 2, 2013

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 2/2 ARM: EXYNOS: add cpuidle-exynos.max_states kernel parameter

Add "cpuidle-exynos.max_states=" parameter to allow user to specify the maximum of allowed CPU idle states for ARM EXYNOS cpuidle driver.

This change is needed because C1 state (AFTR mode) is often not able to work properly due to incompatibility with some bootloader versions.

Usage examples:

"cpuidle-exynos.max_states=1" disables C1 state (AFTR mode).

"cpuidle-exynos.max_states=0" disables the driver completely.

---
 arch/arm/mach-exynos/cpuidle.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
arch/arm/mach-exynos/cpuidle.c +19 -0
index 8e881e0..ead0f71 100644
@@ -16,6 +16,7 @@
 #include <linux/export.h>
 #include <linux/time.h>
 #include <linux/platform_device.h>
+#include <linux/moduleparam.h>
 
 #include <asm/proc-fns.h>
 #include <asm/smp_scu.h>
@@ -30,6 +31,9 @@
 
 #include "common.h"
 
+#define MODULE_PARAM_PREFIX	"cpuidle-exynos."
+#define PREFIX			"cpuidle-exynos: "
+
 #define REG_DIRECTGO_ADDR	(samsung_rev() == EXYNOS4210_REV_1_1 ? \
 			S5P_INFORM7 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
 			(S5P_VA_SYSRAM + 0x24) : S5P_INFORM0))
@@ -63,6 +67,9 @@ static struct cpuidle_driver exynos4_idle_driver = {
 	.safe_state_index = 0,
 };
 
+/* cpuidle-exynos.max_states=0 disables driver */
+static int max_states = CPUIDLE_STATE_MAX;
+
 /* Ext-GIC nIRQ/nFIQ is the only wakeup source in AFTR */
 static void exynos4_set_wakeupmask(void)
 {
@@ -198,6 +205,16 @@ static int __init exynos_cpuidle_probe(struct platform_device *pdev)
 	int cpu_id, ret;
 	struct cpuidle_device *device;
 
+	if (max_states == 0) {
+		pr_info(PREFIX "disabled\n");
+		return 0;
+	}
+
+	if (max_states < exynos4_idle_driver.state_count) {
+		pr_info(PREFIX "limiting to %d state(s)\n", max_states);
+		exynos4_idle_driver.state_count = max_states;
+	}
+
 	if (soc_is_exynos5250())
 		exynos5_core_down_clk();
 
@@ -234,3 +251,5 @@ static struct platform_driver exynos_cpuidle_driver = {
 };
 
 module_platform_driver(exynos_cpuidle_driver);
+
+module_param(max_states, int, 0444);
Signature · 1 line
1.8.2.3
#1 lore raw

PATCH 2/2 ARM: EXYNOS: add cpuidle-exynos.max_states kernel parameter

Quoted 8 lines · Bartlomiej Zolnierkiewicz

Add "cpuidle-exynos.max_states=" parameter to allow user to specify the maximum of allowed CPU idle states for ARM EXYNOS cpuidle driver.

This change is needed because C1 state (AFTR mode) is often not able to work properly due to incompatibility with some bootloader versions.

Usage examples:

Quoted 8 lines

"cpuidle-exynos.max_states=1" disables C1 state (AFTR mode).

"cpuidle-exynos.max_states=0" disables the driver completely.

There is a max_cstate option for acpi and intel idle. There is also the cpuidle.off=1 option. As the semantic is the same, I think adding a common cpuidle option usable for all the drivers is better.

Thanks
-- Daniel

Signature · 5 lines
 <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
#2 lore raw

PATCH 2/2 ARM: EXYNOS: add cpuidle-exynos.max_states kernel parameter

Quoted 13 lines · Daniel Lezcano
Quoted 12 lines · Bartlomiej Zolnierkiewicz

Add "cpuidle-exynos.max_states=" parameter to allow user to specify the maximum of allowed CPU idle states for ARM EXYNOS cpuidle driver.

This change is needed because C1 state (AFTR mode) is often not able to work properly due to incompatibility with some bootloader versions.

Usage examples:

"cpuidle-exynos.max_states=1" disables C1 state (AFTR mode).

"cpuidle-exynos.max_states=0" disables the driver completely.

Quoted 8 lines
Quoted 4 lines

There is a max_cstate option for acpi and intel idle. There is also the cpuidle.off=1 option. As the semantic is the same, I think adding a common cpuidle option usable for all the drivers is better.

I thought about making the option common for all cpuidle drivers first but due to support for multiple cpuidle drivers on one machine (i.e. big.LITTLE), per-driver option looked like a better approach.

Should I make the option common and not worry about multiple drivers on one machine support?

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

#3 lore raw

PATCH 2/2 ARM: EXYNOS: add cpuidle-exynos.max_states kernel parameter

Quoted 23 lines · Bartlomiej Zolnierkiewicz
Quoted 13 lines · Daniel Lezcano
Quoted 12 lines · Bartlomiej Zolnierkiewicz

Add "cpuidle-exynos.max_states=" parameter to allow user to specify the maximum of allowed CPU idle states for ARM EXYNOS cpuidle driver.

This change is needed because C1 state (AFTR mode) is often not able to work properly due to incompatibility with some bootloader versions.

Usage examples:

"cpuidle-exynos.max_states=1" disables C1 state (AFTR mode).

"cpuidle-exynos.max_states=0" disables the driver completely.

Quoted 8 lines
Quoted 4 lines

There is a max_cstate option for acpi and intel idle. There is also the cpuidle.off=1 option. As the semantic is the same, I think adding a common cpuidle option usable for all the drivers is better.

Quoted 6 lines

I thought about making the option common for all cpuidle drivers first but due to support for multiple cpuidle drivers on one machine (i.e. big.LITTLE), per-driver option looked like a better approach.

Should I make the option common and not worry about multiple drivers on one machine support?

Mmh, that's a good point.

I am not in favor of multiple options spread across the different drivers. Furthermore the max_cstate is used in the intel platform to 'discover' what states the firmware supports which is not the case of the cpuidle ARM drivers (except new PSCI based). This option does not really fits well here.

There is the kernel parameter 'cpuidle.off', so disabling the driver is ok.

You converted the cpuidle driver to a platform driver. Isn't possible to pass information in the platform data field at boot time to tell AFTR is not supported and then act on the 'disabled' field of this state ?

Signature · 5 lines
 <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
#4 lore raw

PATCH 2/2 ARM: EXYNOS: add cpuidle-exynos.max_states kernel parameter

Quoted 39 lines · Daniel Lezcano
Quoted 23 lines · Bartlomiej Zolnierkiewicz
Quoted 13 lines · Daniel Lezcano
Quoted 12 lines · Bartlomiej Zolnierkiewicz

Add "cpuidle-exynos.max_states=" parameter to allow user to specify the maximum of allowed CPU idle states for ARM EXYNOS cpuidle driver.

This change is needed because C1 state (AFTR mode) is often not able to work properly due to incompatibility with some bootloader versions.

Usage examples:

"cpuidle-exynos.max_states=1" disables C1 state (AFTR mode).

"cpuidle-exynos.max_states=0" disables the driver completely.

Quoted 8 lines
Quoted 4 lines

There is a max_cstate option for acpi and intel idle. There is also the cpuidle.off=1 option. As the semantic is the same, I think adding a common cpuidle option usable for all the drivers is better.

Quoted 6 lines

I thought about making the option common for all cpuidle drivers first but due to support for multiple cpuidle drivers on one machine (i.e. big.LITTLE), per-driver option looked like a better approach.

Should I make the option common and not worry about multiple drivers on one machine support?

Mmh, that's a good point.

I am not in favor of multiple options spread across the different drivers. Furthermore the max_cstate is used in the intel platform to 'discover' what states the firmware supports which is not the case of the cpuidle ARM drivers (except new PSCI based). This option does not really fits well here.

Quoted 5 lines

There is the kernel parameter 'cpuidle.off', so disabling the driver is ok.

You converted the cpuidle driver to a platform driver. Isn't possible to pass information in the platform data field at boot time to tell AFTR is not supported and then act on the 'disabled' field of this state ?

It might be possible but I don't know where the source of this data would be, platform specific kernel parameter? It sounds just like moving the code around and adding superfluous platform->driver code because the similar kernel parameter to disable just AFTR can be added in cpuidle-exynos driver as well.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

#5 lore raw

PATCH 2/2 ARM: EXYNOS: add cpuidle-exynos.max_states kernel parameter

Quoted 46 lines · Bartlomiej Zolnierkiewicz
Quoted 39 lines · Daniel Lezcano
Quoted 23 lines · Bartlomiej Zolnierkiewicz
Quoted 13 lines · Daniel Lezcano
Quoted 12 lines · Bartlomiej Zolnierkiewicz

Add "cpuidle-exynos.max_states=" parameter to allow user to specify the maximum of allowed CPU idle states for ARM EXYNOS cpuidle driver.

This change is needed because C1 state (AFTR mode) is often not able to work properly due to incompatibility with some bootloader versions.

Usage examples:

"cpuidle-exynos.max_states=1" disables C1 state (AFTR mode).

"cpuidle-exynos.max_states=0" disables the driver completely.

Quoted 8 lines
Quoted 4 lines

There is a max_cstate option for acpi and intel idle. There is also the cpuidle.off=1 option. As the semantic is the same, I think adding a common cpuidle option usable for all the drivers is better.

Quoted 6 lines

I thought about making the option common for all cpuidle drivers first but due to support for multiple cpuidle drivers on one machine (i.e. big.LITTLE), per-driver option looked like a better approach.

Should I make the option common and not worry about multiple drivers on one machine support?

Mmh, that's a good point.

I am not in favor of multiple options spread across the different drivers. Furthermore the max_cstate is used in the intel platform to 'discover' what states the firmware supports which is not the case of the cpuidle ARM drivers (except new PSCI based). This option does not really fits well here.

Quoted 5 lines

There is the kernel parameter 'cpuidle.off', so disabling the driver is ok.

You converted the cpuidle driver to a platform driver. Isn't possible to pass information in the platform data field at boot time to tell AFTR is not supported and then act on the 'disabled' field of this state ?

Quoted 5 lines

It might be possible but I don't know where the source of this data would be, platform specific kernel parameter? It sounds just like moving the code around and adding superfluous platform->driver code because the similar kernel parameter to disable just AFTR can be added in cpuidle-exynos driver as well.

It is to prevent to add a new kernel parameter (with the documentation) for a single driver which has a bogus idle state. If that could be handled internally that would be cleaner.

Can you shortly describe what happens with the bootloader and AFTR ?

I guess you are not interested in cpuidle.off=1 because you want cpuidle statistics for WFI, right ?

Signature · 5 lines
 <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
#6 lore raw

PATCH 2/2 ARM: EXYNOS: add cpuidle-exynos.max_states kernel parameter

Quoted 53 lines · Daniel Lezcano
Quoted 46 lines · Bartlomiej Zolnierkiewicz
Quoted 39 lines · Daniel Lezcano
Quoted 23 lines · Bartlomiej Zolnierkiewicz
Quoted 13 lines · Daniel Lezcano
Quoted 12 lines · Bartlomiej Zolnierkiewicz

Add "cpuidle-exynos.max_states=" parameter to allow user to specify the maximum of allowed CPU idle states for ARM EXYNOS cpuidle driver.

This change is needed because C1 state (AFTR mode) is often not able to work properly due to incompatibility with some bootloader versions.

Usage examples:

"cpuidle-exynos.max_states=1" disables C1 state (AFTR mode).

"cpuidle-exynos.max_states=0" disables the driver completely.

Quoted 8 lines
Quoted 4 lines

There is a max_cstate option for acpi and intel idle. There is also the cpuidle.off=1 option. As the semantic is the same, I think adding a common cpuidle option usable for all the drivers is better.

Quoted 6 lines

I thought about making the option common for all cpuidle drivers first but due to support for multiple cpuidle drivers on one machine (i.e. big.LITTLE), per-driver option looked like a better approach.

Should I make the option common and not worry about multiple drivers on one machine support?

Mmh, that's a good point.

I am not in favor of multiple options spread across the different drivers. Furthermore the max_cstate is used in the intel platform to 'discover' what states the firmware supports which is not the case of the cpuidle ARM drivers (except new PSCI based). This option does not really fits well here.

Quoted 5 lines

There is the kernel parameter 'cpuidle.off', so disabling the driver is ok.

You converted the cpuidle driver to a platform driver. Isn't possible to pass information in the platform data field at boot time to tell AFTR is not supported and then act on the 'disabled' field of this state ?

Quoted 5 lines

It might be possible but I don't know where the source of this data would be, platform specific kernel parameter? It sounds just like moving the code around and adding superfluous platform->driver code because the similar kernel parameter to disable just AFTR can be added in cpuidle-exynos driver as well.

Quoted 3 lines

It is to prevent to add a new kernel parameter (with the documentation) for a single driver which has a bogus idle state. If that could be handled internally that would be cleaner.

If I believed that it could be handled internally I wouldn't be trying to add a kernel parameter to handle it.. Would I? ;)

Quoted 1 line

Can you shortly describe what happens with the bootloader and AFTR ?

AFTR just doesn't work with the custom U-Boot version that we are using (attempts to go into AFTR mode result in lockup) and using the upstream version of U-Boot is not an option since it doesn't support the hardware that we are using AFAIK. I also don't know exactly why it doesn't work (I just suspect that it reuses INFORM registers for some other purposes).

Quoted 2 lines

I guess you are not interested in cpuidle.off=1 because you want cpuidle statistics for WFI, right ?

Right. :)

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

#7 lore raw

PATCH 2/2 ARM: EXYNOS: add cpuidle-exynos.max_states kernel parameter

Quoted 61 lines · Bartlomiej Zolnierkiewicz
Quoted 53 lines · Daniel Lezcano
Quoted 46 lines · Bartlomiej Zolnierkiewicz
Quoted 39 lines · Daniel Lezcano
Quoted 23 lines · Bartlomiej Zolnierkiewicz
Quoted 13 lines · Daniel Lezcano
Quoted 12 lines · Bartlomiej Zolnierkiewicz

Add "cpuidle-exynos.max_states=" parameter to allow user to specify the maximum of allowed CPU idle states for ARM EXYNOS cpuidle driver.

This change is needed because C1 state (AFTR mode) is often not able to work properly due to incompatibility with some bootloader versions.

Usage examples:

"cpuidle-exynos.max_states=1" disables C1 state (AFTR mode).

"cpuidle-exynos.max_states=0" disables the driver completely.

Quoted 8 lines
Quoted 4 lines

There is a max_cstate option for acpi and intel idle. There is also the cpuidle.off=1 option. As the semantic is the same, I think adding a common cpuidle option usable for all the drivers is better.

Quoted 6 lines

I thought about making the option common for all cpuidle drivers first but due to support for multiple cpuidle drivers on one machine (i.e. big.LITTLE), per-driver option looked like a better approach.

Should I make the option common and not worry about multiple drivers on one machine support?

Mmh, that's a good point.

I am not in favor of multiple options spread across the different drivers. Furthermore the max_cstate is used in the intel platform to 'discover' what states the firmware supports which is not the case of the cpuidle ARM drivers (except new PSCI based). This option does not really fits well here.

Quoted 5 lines

There is the kernel parameter 'cpuidle.off', so disabling the driver is ok.

You converted the cpuidle driver to a platform driver. Isn't possible to pass information in the platform data field at boot time to tell AFTR is not supported and then act on the 'disabled' field of this state ?

Quoted 5 lines

It might be possible but I don't know where the source of this data would be, platform specific kernel parameter? It sounds just like moving the code around and adding superfluous platform->driver code because the similar kernel parameter to disable just AFTR can be added in cpuidle-exynos driver as well.

Quoted 3 lines

It is to prevent to add a new kernel parameter (with the documentation) for a single driver which has a bogus idle state. If that could be handled internally that would be cleaner.

If I believed that it could be handled internally I wouldn't be trying to add a kernel parameter to handle it.. Would I? ;)

Quoted 7 lines
Quoted 1 line

Can you shortly describe what happens with the bootloader and AFTR ?

AFTR just doesn't work with the custom U-Boot version that we are using (attempts to go into AFTR mode result in lockup) and using the upstream version of U-Boot is not an option since it doesn't support the hardware that we are using AFAIK. I also don't know exactly why it doesn't work (I just suspect that it reuses INFORM registers for some other purposes).

You want to add a kernel option as a work around for a bug in U-Boot ?

IMO, you should drop the hot potato to the u-boot guys :)

Quoted 4 lines
Quoted 2 lines

I guess you are not interested in cpuidle.off=1 because you want cpuidle statistics for WFI, right ?

Right. :)

Signature · 5 lines
 <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
#8 lore raw

PATCH 2/2 ARM: EXYNOS: add cpuidle-exynos.max_states kernel parameter

Quoted 70 lines · Daniel Lezcano
Quoted 61 lines · Bartlomiej Zolnierkiewicz
Quoted 53 lines · Daniel Lezcano
Quoted 46 lines · Bartlomiej Zolnierkiewicz
Quoted 39 lines · Daniel Lezcano
Quoted 23 lines · Bartlomiej Zolnierkiewicz
Quoted 13 lines · Daniel Lezcano
Quoted 12 lines · Bartlomiej Zolnierkiewicz

Add "cpuidle-exynos.max_states=" parameter to allow user to specify the maximum of allowed CPU idle states for ARM EXYNOS cpuidle driver.

This change is needed because C1 state (AFTR mode) is often not able to work properly due to incompatibility with some bootloader versions.

Usage examples:

"cpuidle-exynos.max_states=1" disables C1 state (AFTR mode).

"cpuidle-exynos.max_states=0" disables the driver completely.

Quoted 8 lines
Quoted 4 lines

There is a max_cstate option for acpi and intel idle. There is also the cpuidle.off=1 option. As the semantic is the same, I think adding a common cpuidle option usable for all the drivers is better.

Quoted 6 lines

I thought about making the option common for all cpuidle drivers first but due to support for multiple cpuidle drivers on one machine (i.e. big.LITTLE), per-driver option looked like a better approach.

Should I make the option common and not worry about multiple drivers on one machine support?

Mmh, that's a good point.

I am not in favor of multiple options spread across the different drivers. Furthermore the max_cstate is used in the intel platform to 'discover' what states the firmware supports which is not the case of the cpuidle ARM drivers (except new PSCI based). This option does not really fits well here.

Quoted 5 lines

There is the kernel parameter 'cpuidle.off', so disabling the driver is ok.

You converted the cpuidle driver to a platform driver. Isn't possible to pass information in the platform data field at boot time to tell AFTR is not supported and then act on the 'disabled' field of this state ?

Quoted 5 lines

It might be possible but I don't know where the source of this data would be, platform specific kernel parameter? It sounds just like moving the code around and adding superfluous platform->driver code because the similar kernel parameter to disable just AFTR can be added in cpuidle-exynos driver as well.

Quoted 3 lines

It is to prevent to add a new kernel parameter (with the documentation) for a single driver which has a bogus idle state. If that could be handled internally that would be cleaner.

If I believed that it could be handled internally I wouldn't be trying to add a kernel parameter to handle it.. Would I? ;)

Quoted 7 lines
Quoted 1 line

Can you shortly describe what happens with the bootloader and AFTR ?

AFTR just doesn't work with the custom U-Boot version that we are using (attempts to go into AFTR mode result in lockup) and using the upstream version of U-Boot is not an option since it doesn't support the hardware that we are using AFAIK. I also don't know exactly why it doesn't work (I just suspect that it reuses INFORM registers for some other purposes).

Quoted 3 lines

You want to add a kernel option as a work around for a bug in U-Boot ?

IMO, you should drop the hot potato to the u-boot guys :)

Well, if it only was so simple. :)

[ Unfortunately, they have higher priority tasks so this is unlikely to get
fixed in the foreseeable future. ]

Problems with AFTR are also not limited to our U-Boot problem (i.e. issue with Hypervisor enabled on EXYNOS5 or problem with CONFIG_THUMB2_KERNEL=y compiled kernels) so it would be nice to have a way to disable it without the need for recompiling the kernel.

Limiting maximum allowed cpuidle state is also useful for testing purposes (this is not limited to EXYNOS driver but a generally useful thing).

Quoted 4 lines
Quoted 4 lines
Quoted 2 lines

I guess you are not interested in cpuidle.off=1 because you want cpuidle statistics for WFI, right ?

Right. :)

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics