[PATCH 0/4] davinci: cpuidle - some cleanups

[PATCH 0/4] davinci: cpuidle - some cleanups

[Daniel Lezcano]

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(-)

-- 
1.7.9.5

[PATCH 1/4] davinci: cpuidle - use global variable for ddr2 flag

[Daniel Lezcano]

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;
-- 
1.7.9.5

[PATCH 2/4] davinci: cpuidle - move code to prevent forward declaration

[Daniel Lezcano]

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;
-- 
1.7.9.5

[PATCH 3/4] davinci: cpuidle - remove the ops

[Daniel Lezcano]

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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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;
 
-- 
1.7.9.5

[PATCH 4/4] davinci: cpuidle - remove useless initialization

[Daniel Lezcano]

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");
-- 
1.7.9.5

Re: [PATCH 0/4] davinci: cpuidle - some cleanups

[Len Brown]

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(-)
> 

Re: [PATCH 0/4] davinci: cpuidle - some cleanups

[Sekhar Nori]

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

Re: [PATCH 0/4] davinci: cpuidle - some cleanups

[Daniel Lezcano]

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


-- 
 <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

Re: [PATCH 0/4] davinci: cpuidle - some cleanups

[Sekhar Nori]

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

Re: [PATCH 0/4] davinci: cpuidle - some cleanups

[Daniel Lezcano]

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

-- 
 <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

Re: [PATCH 1/4] davinci: cpuidle - use global variable for ddr2 flag

[Sekhar Nori]

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

Re: [PATCH 0/4] davinci: cpuidle - some cleanups

[Daniel Lezcano]

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


-- 
 <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


_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev