| Message ID | 4002844.4ZqqJexTv4@amdc1032 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/30/2013 12:21 PM, Bartlomiej Zolnierkiewicz wrote: > 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. > > Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > Cc: Tomasz Figa <t.figa@samsung.com> > Cc: Amit Daniel Kachhap <amit.daniel@samsung.com> 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
On Monday, September 02, 2013 10:54:17 AM Daniel Lezcano wrote: > On 08/30/2013 12:21 PM, Bartlomiej Zolnierkiewicz wrote: > > 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. > > > > Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > > Cc: Tomasz Figa <t.figa@samsung.com> > > Cc: Amit Daniel Kachhap <amit.daniel@samsung.com> > > 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
On 09/02/2013 11:41 AM, Bartlomiej Zolnierkiewicz wrote: > On Monday, September 02, 2013 10:54:17 AM Daniel Lezcano wrote: >> On 08/30/2013 12:21 PM, Bartlomiej Zolnierkiewicz wrote: >>> 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. >>> >>> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> >>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> >>> Cc: Tomasz Figa <t.figa@samsung.com> >>> Cc: Amit Daniel Kachhap <amit.daniel@samsung.com> >> >> 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? 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 ?
On Monday, September 02, 2013 03:18:51 PM Daniel Lezcano wrote: > On 09/02/2013 11:41 AM, Bartlomiej Zolnierkiewicz wrote: > > On Monday, September 02, 2013 10:54:17 AM Daniel Lezcano wrote: > >> On 08/30/2013 12:21 PM, Bartlomiej Zolnierkiewicz wrote: > >>> 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. > >>> > >>> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > >>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > >>> Cc: Tomasz Figa <t.figa@samsung.com> > >>> Cc: Amit Daniel Kachhap <amit.daniel@samsung.com> > >> > >> 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? > > 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 ? 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
On 09/02/2013 03:48 PM, Bartlomiej Zolnierkiewicz wrote: > On Monday, September 02, 2013 03:18:51 PM Daniel Lezcano wrote: >> On 09/02/2013 11:41 AM, Bartlomiej Zolnierkiewicz wrote: >>> On Monday, September 02, 2013 10:54:17 AM Daniel Lezcano wrote: >>>> On 08/30/2013 12:21 PM, Bartlomiej Zolnierkiewicz wrote: >>>>> 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. >>>>> >>>>> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> >>>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> >>>>> Cc: Tomasz Figa <t.figa@samsung.com> >>>>> Cc: Amit Daniel Kachhap <amit.daniel@samsung.com> >>>> >>>> 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? >> >> 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 ? > > 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 ?
On Monday, September 02, 2013 04:24:23 PM Daniel Lezcano wrote: > On 09/02/2013 03:48 PM, Bartlomiej Zolnierkiewicz wrote: > > On Monday, September 02, 2013 03:18:51 PM Daniel Lezcano wrote: > >> On 09/02/2013 11:41 AM, Bartlomiej Zolnierkiewicz wrote: > >>> On Monday, September 02, 2013 10:54:17 AM Daniel Lezcano wrote: > >>>> On 08/30/2013 12:21 PM, Bartlomiej Zolnierkiewicz wrote: > >>>>> 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. > >>>>> > >>>>> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > >>>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > >>>>> Cc: Tomasz Figa <t.figa@samsung.com> > >>>>> Cc: Amit Daniel Kachhap <amit.daniel@samsung.com> > >>>> > >>>> 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? > >> > >> 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 ? > > > > 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. If I believed that it could be handled internally I wouldn't be trying to add a kernel parameter to handle it.. Would I? ;) > 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). > 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
On 09/02/2013 04:43 PM, Bartlomiej Zolnierkiewicz wrote: > On Monday, September 02, 2013 04:24:23 PM Daniel Lezcano wrote: >> On 09/02/2013 03:48 PM, Bartlomiej Zolnierkiewicz wrote: >>> On Monday, September 02, 2013 03:18:51 PM Daniel Lezcano wrote: >>>> On 09/02/2013 11:41 AM, Bartlomiej Zolnierkiewicz wrote: >>>>> On Monday, September 02, 2013 10:54:17 AM Daniel Lezcano wrote: >>>>>> On 08/30/2013 12:21 PM, Bartlomiej Zolnierkiewicz wrote: >>>>>>> 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. >>>>>>> >>>>>>> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> >>>>>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> >>>>>>> Cc: Tomasz Figa <t.figa@samsung.com> >>>>>>> Cc: Amit Daniel Kachhap <amit.daniel@samsung.com> >>>>>> >>>>>> 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? >>>> >>>> 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 ? >>> >>> 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. > > If I believed that it could be handled internally I wouldn't be trying to > add a kernel parameter to handle it.. Would I? ;) > >> 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 :) >> I guess you are not interested in cpuidle.off=1 because you want cpuidle >> statistics for WFI, right ? > > Right. :)
On Monday, September 02, 2013 05:52:15 PM Daniel Lezcano wrote: > On 09/02/2013 04:43 PM, Bartlomiej Zolnierkiewicz wrote: > > On Monday, September 02, 2013 04:24:23 PM Daniel Lezcano wrote: > >> On 09/02/2013 03:48 PM, Bartlomiej Zolnierkiewicz wrote: > >>> On Monday, September 02, 2013 03:18:51 PM Daniel Lezcano wrote: > >>>> On 09/02/2013 11:41 AM, Bartlomiej Zolnierkiewicz wrote: > >>>>> On Monday, September 02, 2013 10:54:17 AM Daniel Lezcano wrote: > >>>>>> On 08/30/2013 12:21 PM, Bartlomiej Zolnierkiewicz wrote: > >>>>>>> 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. > >>>>>>> > >>>>>>> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > >>>>>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > >>>>>>> Cc: Tomasz Figa <t.figa@samsung.com> > >>>>>>> Cc: Amit Daniel Kachhap <amit.daniel@samsung.com> > >>>>>> > >>>>>> 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? > >>>> > >>>> 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 ? > >>> > >>> 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. > > > > If I believed that it could be handled internally I wouldn't be trying to > > add a kernel parameter to handle it.. Would I? ;) > > > >> 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 :) 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). > >> 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
diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c index 8e881e0..ead0f71 100644 --- a/arch/arm/mach-exynos/cpuidle.c +++ b/arch/arm/mach-exynos/cpuidle.c @@ -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);