* [PATCH 2/2] ARM: EXYNOS: add cpuidle-exynos.max_states kernel parameter
@ 2013-08-30 10:21 Bartlomiej Zolnierkiewicz
2013-09-02 8:54 ` Daniel Lezcano
0 siblings, 1 reply; 9+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2013-08-30 10:21 UTC (permalink / raw)
To: linux-arm-kernel
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>
---
arch/arm/mach-exynos/cpuidle.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
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);
--
1.8.2.3
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 2/2] ARM: EXYNOS: add cpuidle-exynos.max_states kernel parameter
2013-08-30 10:21 [PATCH 2/2] ARM: EXYNOS: add cpuidle-exynos.max_states kernel parameter Bartlomiej Zolnierkiewicz
@ 2013-09-02 8:54 ` Daniel Lezcano
2013-09-02 9:41 ` Bartlomiej Zolnierkiewicz
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Lezcano @ 2013-09-02 8:54 UTC (permalink / raw)
To: linux-arm-kernel
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
--
<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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] ARM: EXYNOS: add cpuidle-exynos.max_states kernel parameter
2013-09-02 8:54 ` Daniel Lezcano
@ 2013-09-02 9:41 ` Bartlomiej Zolnierkiewicz
2013-09-02 13:18 ` Daniel Lezcano
0 siblings, 1 reply; 9+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2013-09-02 9:41 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] ARM: EXYNOS: add cpuidle-exynos.max_states kernel parameter
2013-09-02 9:41 ` Bartlomiej Zolnierkiewicz
@ 2013-09-02 13:18 ` Daniel Lezcano
2013-09-02 13:48 ` Bartlomiej Zolnierkiewicz
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Lezcano @ 2013-09-02 13:18 UTC (permalink / raw)
To: linux-arm-kernel
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 ?
--
<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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] ARM: EXYNOS: add cpuidle-exynos.max_states kernel parameter
2013-09-02 13:18 ` Daniel Lezcano
@ 2013-09-02 13:48 ` Bartlomiej Zolnierkiewicz
2013-09-02 14:24 ` Daniel Lezcano
0 siblings, 1 reply; 9+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2013-09-02 13:48 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] ARM: EXYNOS: add cpuidle-exynos.max_states kernel parameter
2013-09-02 13:48 ` Bartlomiej Zolnierkiewicz
@ 2013-09-02 14:24 ` Daniel Lezcano
2013-09-02 14:43 ` Bartlomiej Zolnierkiewicz
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Lezcano @ 2013-09-02 14:24 UTC (permalink / raw)
To: linux-arm-kernel
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 ?
--
<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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] ARM: EXYNOS: add cpuidle-exynos.max_states kernel parameter
2013-09-02 14:24 ` Daniel Lezcano
@ 2013-09-02 14:43 ` Bartlomiej Zolnierkiewicz
2013-09-02 15:52 ` Daniel Lezcano
0 siblings, 1 reply; 9+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2013-09-02 14:43 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] ARM: EXYNOS: add cpuidle-exynos.max_states kernel parameter
2013-09-02 14:43 ` Bartlomiej Zolnierkiewicz
@ 2013-09-02 15:52 ` Daniel Lezcano
2013-09-02 16:13 ` Bartlomiej Zolnierkiewicz
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Lezcano @ 2013-09-02 15:52 UTC (permalink / raw)
To: linux-arm-kernel
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. :)
--
<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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] ARM: EXYNOS: add cpuidle-exynos.max_states kernel parameter
2013-09-02 15:52 ` Daniel Lezcano
@ 2013-09-02 16:13 ` Bartlomiej Zolnierkiewicz
0 siblings, 0 replies; 9+ messages in thread
From: Bartlomiej Zolnierkiewicz @ 2013-09-02 16:13 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-09-02 16:13 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-08-30 10:21 [PATCH 2/2] ARM: EXYNOS: add cpuidle-exynos.max_states kernel parameter Bartlomiej Zolnierkiewicz
2013-09-02 8:54 ` Daniel Lezcano
2013-09-02 9:41 ` Bartlomiej Zolnierkiewicz
2013-09-02 13:18 ` Daniel Lezcano
2013-09-02 13:48 ` Bartlomiej Zolnierkiewicz
2013-09-02 14:24 ` Daniel Lezcano
2013-09-02 14:43 ` Bartlomiej Zolnierkiewicz
2013-09-02 15:52 ` Daniel Lezcano
2013-09-02 16:13 ` Bartlomiej Zolnierkiewicz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).