This patchset does some cleanup. It could have been folded in a single patch but the review would have been less clean than splitting it into small and trivial patches.
The main purpose of this patch is to remove the usage of the driver_data field from the state_usage structure. Len Brown is doing this cleanup in the intel_idle.c file. With this patchset, the processor_idle.c file will be the last user of this field.
Also, the patchset simplify the code and makes it a bit more clear to read.
I don't have this hardware, the code is not tested.
Daniel Lezcano (4): davinci: cpuidle - use global variable for ddr2 flag davinci: cpuidle - move code to prevent forward declaration davinci: cpuidle - remove the ops davinci: cpuidle - remove useless initialization
arch/arm/mach-davinci/cpuidle.c | 84 ++++++++++++--------------------------- 1 file changed, 25 insertions(+), 59 deletions(-)
Replace the flag by a simple global boolean in the cpuidle.c. That will allow to cleanup the rest of the code right after, because the ops won't make sense anymore.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-davinci/cpuidle.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/arch/arm/mach-davinci/cpuidle.c b/arch/arm/mach-davinci/cpuidle.c index 9107691..ebb8808 100644 --- a/arch/arm/mach-davinci/cpuidle.c +++ b/arch/arm/mach-davinci/cpuidle.c @@ -26,8 +26,8 @@ #define DAVINCI_CPUIDLE_MAX_STATES 2
struct davinci_ops { - void (*enter) (u32 flags); - void (*exit) (u32 flags); + void (*enter) (void); + void (*exit) (void); u32 flags; };
@@ -40,20 +40,17 @@ static int davinci_enter_idle(struct cpuidle_device *dev, struct davinci_ops *ops = cpuidle_get_statedata(state_usage);
if (ops && ops->enter) - ops->enter(ops->flags); + ops->enter();
index = cpuidle_wrap_enter(dev, drv, index, arm_cpuidle_simple_enter);
if (ops && ops->exit) - ops->exit(ops->flags); + ops->exit();
return index; }
-/* fields in davinci_ops.flags */ -#define DAVINCI_CPUIDLE_FLAGS_DDR2_PWDN BIT(0) - static struct cpuidle_driver davinci_idle_driver = { .name = "cpuidle-davinci", .owner = THIS_MODULE, @@ -72,6 +69,7 @@ static struct cpuidle_driver davinci_idle_driver = {
static DEFINE_PER_CPU(struct cpuidle_device, davinci_cpuidle_device); static void __iomem *ddr2_reg_base; +static bool ddr2_pdown = false;
static void davinci_save_ddr_power(int enter, bool pdown) { @@ -92,14 +90,14 @@ static void davinci_save_ddr_power(int enter, bool pdown) __raw_writel(val, ddr2_reg_base + DDR2_SDRCR_OFFSET); }
-static void davinci_c2state_enter(u32 flags) +static void davinci_c2state_enter(void) { - davinci_save_ddr_power(1, !!(flags & DAVINCI_CPUIDLE_FLAGS_DDR2_PWDN)); + davinci_save_ddr_power(1, ddr2_pdown); }
-static void davinci_c2state_exit(u32 flags) +static void davinci_c2state_exit(void) { - davinci_save_ddr_power(0, !!(flags & DAVINCI_CPUIDLE_FLAGS_DDR2_PWDN)); + davinci_save_ddr_power(0, ddr2_pdown); }
static struct davinci_ops davinci_states[DAVINCI_CPUIDLE_MAX_STATES] = { @@ -124,8 +122,7 @@ static int __init davinci_cpuidle_probe(struct platform_device *pdev)
ddr2_reg_base = pdata->ddr2_ctlr_base;
- if (pdata->ddr2_pdown) - davinci_states[1].flags |= DAVINCI_CPUIDLE_FLAGS_DDR2_PWDN; + ddr2_pdown = pdata->ddr2_pdown; cpuidle_set_statedata(&device->states_usage[1], &davinci_states[1]);
device->state_count = DAVINCI_CPUIDLE_MAX_STATES;
The patch is mindless, it just moves the idle function below in the file in order to prevent forward declaration in the next patch.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-davinci/cpuidle.c | 72 +++++++++++++++++++-------------------- 1 file changed, 36 insertions(+), 36 deletions(-)
diff --git a/arch/arm/mach-davinci/cpuidle.c b/arch/arm/mach-davinci/cpuidle.c index ebb8808..9438672 100644 --- a/arch/arm/mach-davinci/cpuidle.c +++ b/arch/arm/mach-davinci/cpuidle.c @@ -31,42 +31,6 @@ struct davinci_ops { u32 flags; };
-/* Actual code that puts the SoC in different idle states */ -static int davinci_enter_idle(struct cpuidle_device *dev, - struct cpuidle_driver *drv, - int index) -{ - struct cpuidle_state_usage *state_usage = &dev->states_usage[index]; - struct davinci_ops *ops = cpuidle_get_statedata(state_usage); - - if (ops && ops->enter) - ops->enter(); - - index = cpuidle_wrap_enter(dev, drv, index, - arm_cpuidle_simple_enter); - - if (ops && ops->exit) - ops->exit(); - - return index; -} - -static struct cpuidle_driver davinci_idle_driver = { - .name = "cpuidle-davinci", - .owner = THIS_MODULE, - .en_core_tk_irqen = 1, - .states[0] = ARM_CPUIDLE_WFI_STATE, - .states[1] = { - .enter = davinci_enter_idle, - .exit_latency = 10, - .target_residency = 100000, - .flags = CPUIDLE_FLAG_TIME_VALID, - .name = "DDR SR", - .desc = "WFI and DDR Self Refresh", - }, - .state_count = DAVINCI_CPUIDLE_MAX_STATES, -}; - static DEFINE_PER_CPU(struct cpuidle_device, davinci_cpuidle_device); static void __iomem *ddr2_reg_base; static bool ddr2_pdown = false; @@ -107,6 +71,42 @@ static struct davinci_ops davinci_states[DAVINCI_CPUIDLE_MAX_STATES] = { }, };
+/* Actual code that puts the SoC in different idle states */ +static int davinci_enter_idle(struct cpuidle_device *dev, + struct cpuidle_driver *drv, + int index) +{ + struct cpuidle_state_usage *state_usage = &dev->states_usage[index]; + struct davinci_ops *ops = cpuidle_get_statedata(state_usage); + + if (ops && ops->enter) + ops->enter(); + + index = cpuidle_wrap_enter(dev, drv, index, + arm_cpuidle_simple_enter); + + if (ops && ops->exit) + ops->exit(); + + return index; +} + +static struct cpuidle_driver davinci_idle_driver = { + .name = "cpuidle-davinci", + .owner = THIS_MODULE, + .en_core_tk_irqen = 1, + .states[0] = ARM_CPUIDLE_WFI_STATE, + .states[1] = { + .enter = davinci_enter_idle, + .exit_latency = 10, + .target_residency = 100000, + .flags = CPUIDLE_FLAG_TIME_VALID, + .name = "DDR SR", + .desc = "WFI and DDR Self Refresh", + }, + .state_count = DAVINCI_CPUIDLE_MAX_STATES, +}; + static int __init davinci_cpuidle_probe(struct platform_device *pdev) { int ret;
With one function handling the idle state and a single variable, the usage of the davinci_ops is overkill.
This patch removes these ops and simplify the code.
Furthermore, the 'driver_data' field is no longer used, we have 1 of the 3 remaining user of this field removed.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-davinci/cpuidle.c | 33 ++------------------------------- 1 file changed, 2 insertions(+), 31 deletions(-)
diff --git a/arch/arm/mach-davinci/cpuidle.c b/arch/arm/mach-davinci/cpuidle.c index 9438672..28cc8e8 100644 --- a/arch/arm/mach-davinci/cpuidle.c +++ b/arch/arm/mach-davinci/cpuidle.c @@ -25,12 +25,6 @@
#define DAVINCI_CPUIDLE_MAX_STATES 2
-struct davinci_ops { - void (*enter) (void); - void (*exit) (void); - u32 flags; -}; - static DEFINE_PER_CPU(struct cpuidle_device, davinci_cpuidle_device); static void __iomem *ddr2_reg_base; static bool ddr2_pdown = false; @@ -54,39 +48,17 @@ static void davinci_save_ddr_power(int enter, bool pdown) __raw_writel(val, ddr2_reg_base + DDR2_SDRCR_OFFSET); }
-static void davinci_c2state_enter(void) -{ - davinci_save_ddr_power(1, ddr2_pdown); -} - -static void davinci_c2state_exit(void) -{ - davinci_save_ddr_power(0, ddr2_pdown); -} - -static struct davinci_ops davinci_states[DAVINCI_CPUIDLE_MAX_STATES] = { - [1] = { - .enter = davinci_c2state_enter, - .exit = davinci_c2state_exit, - }, -}; - /* Actual code that puts the SoC in different idle states */ static int davinci_enter_idle(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index) { - struct cpuidle_state_usage *state_usage = &dev->states_usage[index]; - struct davinci_ops *ops = cpuidle_get_statedata(state_usage); - - if (ops && ops->enter) - ops->enter(); + davinci_save_ddr_power(1, ddr2_pdown);
index = cpuidle_wrap_enter(dev, drv, index, arm_cpuidle_simple_enter);
- if (ops && ops->exit) - ops->exit(); + davinci_save_ddr_power(0, ddr2_pdown);
return index; } @@ -123,7 +95,6 @@ static int __init davinci_cpuidle_probe(struct platform_device *pdev) ddr2_reg_base = pdata->ddr2_ctlr_base;
ddr2_pdown = pdata->ddr2_pdown; - cpuidle_set_statedata(&device->states_usage[1], &davinci_states[1]);
device->state_count = DAVINCI_CPUIDLE_MAX_STATES;
The device->state_count is initialized in the cpuidle_register_device function.
Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org --- arch/arm/mach-davinci/cpuidle.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/arch/arm/mach-davinci/cpuidle.c b/arch/arm/mach-davinci/cpuidle.c index 28cc8e8..144839b 100644 --- a/arch/arm/mach-davinci/cpuidle.c +++ b/arch/arm/mach-davinci/cpuidle.c @@ -96,8 +96,6 @@ static int __init davinci_cpuidle_probe(struct platform_device *pdev)
ddr2_pdown = pdata->ddr2_pdown;
- device->state_count = DAVINCI_CPUIDLE_MAX_STATES; - ret = cpuidle_register_driver(&davinci_idle_driver); if (ret) { dev_err(&pdev->dev, "failed to register driver\n");
On 02/01/2013 08:48 AM, Daniel Lezcano wrote:
This patchset does some cleanup. It could have been folded in a single patch but the review would have been less clean than splitting it into small and trivial patches.
The main purpose of this patch is to remove the usage of the driver_data field from the state_usage structure. Len Brown is doing this cleanup in the intel_idle.c file. With this patchset, the processor_idle.c file will be the last user of this field.
Daniel, Thanks for this cleanup. Hopefully we can hear from somebody with davinci HW who can test it?
Looks like it is now up to me to address processor_idle.c and finish the job of expunging driver_data.
thanks, -Len Brown
Intel Open Source Technology Center
Also, the patchset simplify the code and makes it a bit more clear to read.
I don't have this hardware, the code is not tested.
Daniel Lezcano (4): davinci: cpuidle - use global variable for ddr2 flag davinci: cpuidle - move code to prevent forward declaration davinci: cpuidle - remove the ops davinci: cpuidle - remove useless initialization
arch/arm/mach-davinci/cpuidle.c | 84 ++++++++++++--------------------------- 1 file changed, 25 insertions(+), 59 deletions(-)
On 2/2/2013 12:19 AM, Len Brown wrote:
On 02/01/2013 08:48 AM, Daniel Lezcano wrote:
This patchset does some cleanup. It could have been folded in a single patch but the review would have been less clean than splitting it into small and trivial patches.
The main purpose of this patch is to remove the usage of the driver_data field from the state_usage structure. Len Brown is doing this cleanup in the intel_idle.c file. With this patchset, the processor_idle.c file will be the last user of this field.
Daniel, Thanks for this cleanup. Hopefully we can hear from somebody with davinci HW who can test it?
Can someone forward the patches to davinci-linux-open-source@linux.davincidsp.com? I can help verify.
Thanks, Sekhar
On 02/03/2013 12:54 PM, Sekhar Nori wrote:
On 2/2/2013 12:19 AM, Len Brown wrote:
On 02/01/2013 08:48 AM, Daniel Lezcano wrote:
This patchset does some cleanup. It could have been folded in a single patch but the review would have been less clean than splitting it into small and trivial patches.
The main purpose of this patch is to remove the usage of the driver_data field from the state_usage structure. Len Brown is doing this cleanup in the intel_idle.c file. With this patchset, the processor_idle.c file will be the last user of this field.
Daniel, Thanks for this cleanup. Hopefully we can hear from somebody with davinci HW who can test it?
Can someone forward the patches to davinci-linux-open-source@linux.davincidsp.com? I can help verify.
I cc'ed this mailing list but I have not subscribed to it. The mailing list description says "moderated for non-subscribers" in the MAINTAINERS file.
I assumed the moderator will accept the patchset.
Shall I subscribe and resend ?
Thanks -- Daniel
On 2/3/2013 5:52 PM, Daniel Lezcano wrote:
On 02/03/2013 12:54 PM, Sekhar Nori wrote:
On 2/2/2013 12:19 AM, Len Brown wrote:
On 02/01/2013 08:48 AM, Daniel Lezcano wrote:
This patchset does some cleanup. It could have been folded in a single patch but the review would have been less clean than splitting it into small and trivial patches.
The main purpose of this patch is to remove the usage of the driver_data field from the state_usage structure. Len Brown is doing this cleanup in the intel_idle.c file. With this patchset, the processor_idle.c file will be the last user of this field.
Daniel, Thanks for this cleanup. Hopefully we can hear from somebody with davinci HW who can test it?
Can someone forward the patches to davinci-linux-open-source@linux.davincidsp.com? I can help verify.
I cc'ed this mailing list but I have not subscribed to it. The mailing list description says "moderated for non-subscribers" in the MAINTAINERS file.
I assumed the moderator will accept the patchset.
Shall I subscribe and resend ?
No need. I see the patches on the list. I will test and get back.
Thanks, Sekhar
On 02/03/2013 02:00 PM, Sekhar Nori wrote:
On 2/3/2013 5:52 PM, Daniel Lezcano wrote:
On 02/03/2013 12:54 PM, Sekhar Nori wrote:
On 2/2/2013 12:19 AM, Len Brown wrote:
On 02/01/2013 08:48 AM, Daniel Lezcano wrote:
This patchset does some cleanup. It could have been folded in a single patch but the review would have been less clean than splitting it into small and trivial patches.
The main purpose of this patch is to remove the usage of the driver_data field from the state_usage structure. Len Brown is doing this cleanup in the intel_idle.c file. With this patchset, the processor_idle.c file will be the last user of this field.
Daniel, Thanks for this cleanup. Hopefully we can hear from somebody with davinci HW who can test it?
Can someone forward the patches to davinci-linux-open-source@linux.davincidsp.com? I can help verify.
I cc'ed this mailing list but I have not subscribed to it. The mailing list description says "moderated for non-subscribers" in the MAINTAINERS file.
I assumed the moderator will accept the patchset.
Shall I subscribe and resend ?
No need. I see the patches on the list. I will test and get back.
Cool ! Thanks !
-- Daniel
On 2/1/2013 7:18 PM, Daniel Lezcano wrote:
+static bool ddr2_pdown = false;
checkpatch.pl warned against zero initialization of static variable here. Other than that the series looks good. I checked that both state0 and state1 are entered on da850 evm. Making power measurements requires more elaborate setup so I left that out.
Target residency of state1 seems high and it is entered very infrequently. But that's not something introduced by this series. So for all the patches, you can add:
Acked-by: Sekhar Nori nsekhar@ti.com
Thanks, Sekhar
On 02/01/2013 07:49 PM, Len Brown wrote:
On 02/01/2013 08:48 AM, Daniel Lezcano wrote:
This patchset does some cleanup. It could have been folded in a single patch but the review would have been less clean than splitting it into small and trivial patches.
The main purpose of this patch is to remove the usage of the driver_data field from the state_usage structure. Len Brown is doing this cleanup in the intel_idle.c file. With this patchset, the processor_idle.c file will be the last user of this field.
Daniel, Thanks for this cleanup. Hopefully we can hear from somebody with davinci HW who can test it?
Looks like it is now up to me to address processor_idle.c and finish the job of expunging driver_data.
Are you ok if I take care of that ?
-- Daniel
-
Daniel Lezcano -
Len Brown -
Sekhar Nori