[PATCH] ARM: shmobile: Fix device node reference leakage in shmobile_init_delay

[PATCH] ARM: shmobile: Fix device node reference leakage in shmobile_init_delay

[Laurent Pinchart]

The of_find_compatible_node() function returns a new reference to the
found node. Instead of just adding of_node_put() calls, simplify the
code by moving the CPU identification logic inside the loop over cpu
nodes, in order to lower complexity from O(n) to O(1) by replacing
of_find_compatible_node() calls with of_device_is_compatible().

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 arch/arm/mach-shmobile/timer.c | 50 ++++++++++++++++++++++++------------------
 1 file changed, 29 insertions(+), 21 deletions(-)

Based on top of Simon's latest devel branch.

diff --git a/arch/arm/mach-shmobile/timer.c b/arch/arm/mach-shmobile/timer.c
index 68bc0b8..942efdc 100644
--- a/arch/arm/mach-shmobile/timer.c
+++ b/arch/arm/mach-shmobile/timer.c
@@ -59,29 +59,37 @@ void __init shmobile_setup_delay(unsigned int max_cpu_core_mhz,
 
 void __init shmobile_init_delay(void)
 {
-	struct device_node *np, *parent;
-	u32 max_freq, freq;
-
-	max_freq = 0;
-
-	parent = of_find_node_by_path("/cpus");
-	if (parent) {
-		for_each_child_of_node(parent, np) {
-			if (!of_property_read_u32(np, "clock-frequency", &freq))
-				max_freq = max(max_freq, freq);
-		}
-		of_node_put(parent);
-	}
+	struct device_node *np, *cpus;
+	bool is_a8_a9 = false;
+	bool is_a15 = false;
+	u32 max_freq = 0;
+
+	cpus = of_find_node_by_path("/cpus");
+	if (!cpus)
+		return;
+
+	for_each_child_of_node(cpus, np) {
+		u32 freq;
+
+		if (!of_property_read_u32(np, "clock-frequency", &freq))
+			max_freq = max(max_freq, freq);
 
-	if (max_freq) {
-		if (of_find_compatible_node(NULL, NULL, "arm,cortex-a8"))
-			shmobile_setup_delay_hz(max_freq, 1, 3);
-		else if (of_find_compatible_node(NULL, NULL, "arm,cortex-a9"))
-			shmobile_setup_delay_hz(max_freq, 1, 3);
-		else if (of_find_compatible_node(NULL, NULL, "arm,cortex-a15"))
-			if (!IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
-				shmobile_setup_delay_hz(max_freq, 2, 4);
+		if (of_device_is_compatible(np, "arm,cortex-a8") ||
+		    of_device_is_compatible(np, "arm,cortex-a9"))
+			is_a8_a9 = true;
+		else if (of_device_is_compatible(np, "arm,cortex-a15"))
+			is_a15 = true;
 	}
+
+	of_node_put(cpus);
+
+	if (!max_freq)
+		return;
+
+	if (is_a8_a9)
+		shmobile_setup_delay_hz(max_freq, 1, 3);
+	else if (is_a15 && !IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
+		shmobile_setup_delay_hz(max_freq, 2, 4);
 }
 
 static void __init shmobile_late_time_init(void)
-- 
Regards,

Laurent Pinchart

Re: [PATCH] ARM: shmobile: Fix device node reference leakage in shmobile_init_delay

[Geert Uytterhoeven]

Hi Laurent, Magnus,

On Wed, May 21, 2014 at 3:31 PM, Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:
> +       bool is_a8_a9 = false;
> +       bool is_a15 = false;
> +       u32 max_freq = 0;
> +
> +       cpus = of_find_node_by_path("/cpus");
> +       if (!cpus)
> +               return;
> +
> +       for_each_child_of_node(cpus, np) {
> +               u32 freq;
> +
> +               if (!of_property_read_u32(np, "clock-frequency", &freq))
> +                       max_freq = max(max_freq, freq);
>
> -       if (max_freq) {
> -               if (of_find_compatible_node(NULL, NULL, "arm,cortex-a8"))
> -                       shmobile_setup_delay_hz(max_freq, 1, 3);
> -               else if (of_find_compatible_node(NULL, NULL, "arm,cortex-a9"))
> -                       shmobile_setup_delay_hz(max_freq, 1, 3);
> -               else if (of_find_compatible_node(NULL, NULL, "arm,cortex-a15"))
> -                       if (!IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
> -                               shmobile_setup_delay_hz(max_freq, 2, 4);
> +               if (of_device_is_compatible(np, "arm,cortex-a8") ||
> +                   of_device_is_compatible(np, "arm,cortex-a9"))
> +                       is_a8_a9 = true;

So now we have to care about CPU families in two places: here and below,
when calling shmobile_setup_delay_hz().

IIUC, "shmobile_setup_delay_hz(max_freq, 2, 4)" is (ignoring rounding)
equivalent to "shmobile_setup_delay_hz(max_freq, 1, 2)"?
Do we actually have cases where mult needs to be different from 1?
If not, we can kill the "mult" parameter, and replace the "is_a8_a9 = true"
by:

div = 3;

> +               else if (of_device_is_compatible(np, "arm,cortex-a15"))
> +                       is_a15 = true;

and

else if (of_device_is_compatible(np, "arm,cortex-a15") &&
         !IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
        div = 2;

>         }
> +
> +       of_node_put(cpus);
> +
> +       if (!max_freq)
> +               return;
> +
> +       if (is_a8_a9)
> +               shmobile_setup_delay_hz(max_freq, 1, 3);
> +       else if (is_a15 && !IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
> +               shmobile_setup_delay_hz(max_freq, 2, 4);

and this just becomes:

if (div)
    shmobile_setup_delay_hz(max_freq, div);

Am I missing something?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] ARM: shmobile: Fix device node reference leakage in shmobile_init_delay

[Laurent Pinchart]

Hi Geert,

On Wednesday 21 May 2014 15:56:20 Geert Uytterhoeven wrote:
> Hi Laurent, Magnus,
> 
> On Wed, May 21, 2014 at 3:31 PM, Laurent Pinchart wrote:
> > +       bool is_a8_a9 = false;
> > +       bool is_a15 = false;
> > +       u32 max_freq = 0;
> > +
> > +       cpus = of_find_node_by_path("/cpus");
> > +       if (!cpus)
> > +               return;
> > +
> > +       for_each_child_of_node(cpus, np) {
> > +               u32 freq;
> > +
> > +               if (!of_property_read_u32(np, "clock-frequency", &freq))
> > +                       max_freq = max(max_freq, freq);
> > 
> > -       if (max_freq) {
> > -               if (of_find_compatible_node(NULL, NULL, "arm,cortex-a8"))
> > -                       shmobile_setup_delay_hz(max_freq, 1, 3);
> > -               else if (of_find_compatible_node(NULL, NULL,
> > "arm,cortex-a9")) -                      
> > shmobile_setup_delay_hz(max_freq, 1, 3);
> > -               else if (of_find_compatible_node(NULL, NULL,
> > "arm,cortex-a15")) -                       if
> > (!IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
> > -                               shmobile_setup_delay_hz(max_freq, 2, 4);
> > +               if (of_device_is_compatible(np, "arm,cortex-a8") ||
> > +                   of_device_is_compatible(np, "arm,cortex-a9"))
> > +                       is_a8_a9 = true;
> 
> So now we have to care about CPU families in two places: here and below,
> when calling shmobile_setup_delay_hz().
> 
> IIUC, "shmobile_setup_delay_hz(max_freq, 2, 4)" is (ignoring rounding)
> equivalent to "shmobile_setup_delay_hz(max_freq, 1, 2)"?
> Do we actually have cases where mult needs to be different from 1?
> If not, we can kill the "mult" parameter, and replace the "is_a8_a9 = true"
> by:
> 
> div = 3;
> 
> > +               else if (of_device_is_compatible(np, "arm,cortex-a15"))
> > +                       is_a15 = true;
> 
> and
> 
> else if (of_device_is_compatible(np, "arm,cortex-a15") &&
>          !IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
>         div = 2;
> 
> >         }
> > 
> > +
> > +       of_node_put(cpus);
> > +
> > +       if (!max_freq)
> > +               return;
> > +
> > +       if (is_a8_a9)
> > +               shmobile_setup_delay_hz(max_freq, 1, 3);
> > +       else if (is_a15 && !IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
> > +               shmobile_setup_delay_hz(max_freq, 2, 4);
> 
> and this just becomes:
> 
> if (div)
>     shmobile_setup_delay_hz(max_freq, div);
> 
> Am I missing something?

That's an option as well, but I find the code flow to slightly be less clear 
in that case. I have no strong preference so I'll let Magnus decide.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] ARM: shmobile: Fix device node reference leakage in shmobile_init_delay

[Magnus Damm]

Hi Laurent and Geert,

On Thu, May 22, 2014 at 1:13 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Geert,
>
> On Wednesday 21 May 2014 15:56:20 Geert Uytterhoeven wrote:
>> Hi Laurent, Magnus,
>>
>> On Wed, May 21, 2014 at 3:31 PM, Laurent Pinchart wrote:
>> > +       bool is_a8_a9 = false;
>> > +       bool is_a15 = false;
>> > +       u32 max_freq = 0;
>> > +
>> > +       cpus = of_find_node_by_path("/cpus");
>> > +       if (!cpus)
>> > +               return;
>> > +
>> > +       for_each_child_of_node(cpus, np) {
>> > +               u32 freq;
>> > +
>> > +               if (!of_property_read_u32(np, "clock-frequency", &freq))
>> > +                       max_freq = max(max_freq, freq);
>> >
>> > -       if (max_freq) {
>> > -               if (of_find_compatible_node(NULL, NULL, "arm,cortex-a8"))
>> > -                       shmobile_setup_delay_hz(max_freq, 1, 3);
>> > -               else if (of_find_compatible_node(NULL, NULL,
>> > "arm,cortex-a9")) -
>> > shmobile_setup_delay_hz(max_freq, 1, 3);
>> > -               else if (of_find_compatible_node(NULL, NULL,
>> > "arm,cortex-a15")) -                       if
>> > (!IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
>> > -                               shmobile_setup_delay_hz(max_freq, 2, 4);
>> > +               if (of_device_is_compatible(np, "arm,cortex-a8") ||
>> > +                   of_device_is_compatible(np, "arm,cortex-a9"))
>> > +                       is_a8_a9 = true;
>>
>> So now we have to care about CPU families in two places: here and below,
>> when calling shmobile_setup_delay_hz().
>>
>> IIUC, "shmobile_setup_delay_hz(max_freq, 2, 4)" is (ignoring rounding)
>> equivalent to "shmobile_setup_delay_hz(max_freq, 1, 2)"?
>> Do we actually have cases where mult needs to be different from 1?
>> If not, we can kill the "mult" parameter, and replace the "is_a8_a9 = true"
>> by:
>>
>> div = 3;
>>
>> > +               else if (of_device_is_compatible(np, "arm,cortex-a15"))
>> > +                       is_a15 = true;
>>
>> and
>>
>> else if (of_device_is_compatible(np, "arm,cortex-a15") &&
>>          !IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
>>         div = 2;
>>
>> >         }
>> >
>> > +
>> > +       of_node_put(cpus);
>> > +
>> > +       if (!max_freq)
>> > +               return;
>> > +
>> > +       if (is_a8_a9)
>> > +               shmobile_setup_delay_hz(max_freq, 1, 3);
>> > +       else if (is_a15 && !IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
>> > +               shmobile_setup_delay_hz(max_freq, 2, 4);
>>
>> and this just becomes:
>>
>> if (div)
>>     shmobile_setup_delay_hz(max_freq, div);
>>
>> Am I missing something?
>
> That's an option as well, but I find the code flow to slightly be less clear
> in that case. I have no strong preference so I'll let Magnus decide.

Ouch, I will have to decide? =)

If we consider upcoming R-Car E2 that is Cortex-A7 only, i wonder
which way to go to also support that easily?

Thanks,

/ magnus

Re: [PATCH] ARM: shmobile: Fix device node reference leakage in shmobile_init_delay

[Laurent Pinchart]

On Thursday 22 May 2014 09:24:47 Magnus Damm wrote:
> On Thu, May 22, 2014 at 1:13 AM, Laurent Pinchart wrote:
> > On Wednesday 21 May 2014 15:56:20 Geert Uytterhoeven wrote:
> >> On Wed, May 21, 2014 at 3:31 PM, Laurent Pinchart wrote:
> >> > +       bool is_a8_a9 = false;
> >> > +       bool is_a15 = false;
> >> > +       u32 max_freq = 0;
> >> > +
> >> > +       cpus = of_find_node_by_path("/cpus");
> >> > +       if (!cpus)
> >> > +               return;
> >> > +
> >> > +       for_each_child_of_node(cpus, np) {
> >> > +               u32 freq;
> >> > +
> >> > +               if (!of_property_read_u32(np, "clock-frequency",
> >> > &freq))
> >> > +                       max_freq = max(max_freq, freq);
> >> > 
> >> > -       if (max_freq) {
> >> > -               if (of_find_compatible_node(NULL, NULL,
> >> > "arm,cortex-a8"))
> >> > -                       shmobile_setup_delay_hz(max_freq, 1, 3);
> >> > -               else if (of_find_compatible_node(NULL, NULL,
> >> > "arm,cortex-a9")) -
> >> > shmobile_setup_delay_hz(max_freq, 1, 3);
> >> > -               else if (of_find_compatible_node(NULL, NULL,
> >> > "arm,cortex-a15")) -                       if
> >> > (!IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
> >> > -                               shmobile_setup_delay_hz(max_freq, 2,
> >> > 4);
> >> > +               if (of_device_is_compatible(np, "arm,cortex-a8") ||
> >> > +                   of_device_is_compatible(np, "arm,cortex-a9"))
> >> > +                       is_a8_a9 = true;
> >> 
> >> So now we have to care about CPU families in two places: here and below,
> >> when calling shmobile_setup_delay_hz().
> >> 
> >> IIUC, "shmobile_setup_delay_hz(max_freq, 2, 4)" is (ignoring rounding)
> >> equivalent to "shmobile_setup_delay_hz(max_freq, 1, 2)"?
> >> Do we actually have cases where mult needs to be different from 1?
> >> If not, we can kill the "mult" parameter, and replace the "is_a8_a9 > >> true"
> >> by:
> >> 
> >> div = 3;
> >> 
> >> > +               else if (of_device_is_compatible(np, "arm,cortex-a15"))
> >> > +                       is_a15 = true;
> >> 
> >> and
> >> 
> >> else if (of_device_is_compatible(np, "arm,cortex-a15") &&
> >> 
> >>          !IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
> >>         
> >>         div = 2;
> >>         
> >> >         }
> >> > 
> >> > +
> >> > +       of_node_put(cpus);
> >> > +
> >> > +       if (!max_freq)
> >> > +               return;
> >> > +
> >> > +       if (is_a8_a9)
> >> > +               shmobile_setup_delay_hz(max_freq, 1, 3);
> >> > +       else if (is_a15 && !IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
> >> > +               shmobile_setup_delay_hz(max_freq, 2, 4);
> >> 
> >> and this just becomes:
> >> 
> >> if (div)
> >> 
> >>     shmobile_setup_delay_hz(max_freq, div);
> >> 
> >> Am I missing something?
> > 
> > That's an option as well, but I find the code flow to slightly be less
> > clear in that case. I have no strong preference so I'll let Magnus
> > decide.
>
> Ouch, I will have to decide? =)
> 
> If we consider upcoming R-Car E2 that is Cortex-A7 only, i wonder
> which way to go to also support that easily?

Depends, what do we need to do on that platform ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH] ARM: shmobile: Fix device node reference leakage in shmobile_init_delay

[Magnus Damm]

On Thu, May 22, 2014 at 9:50 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Thursday 22 May 2014 09:24:47 Magnus Damm wrote:
>> On Thu, May 22, 2014 at 1:13 AM, Laurent Pinchart wrote:
>> > On Wednesday 21 May 2014 15:56:20 Geert Uytterhoeven wrote:
>> >> On Wed, May 21, 2014 at 3:31 PM, Laurent Pinchart wrote:
>> >> > +       bool is_a8_a9 = false;
>> >> > +       bool is_a15 = false;
>> >> > +       u32 max_freq = 0;
>> >> > +
>> >> > +       cpus = of_find_node_by_path("/cpus");
>> >> > +       if (!cpus)
>> >> > +               return;
>> >> > +
>> >> > +       for_each_child_of_node(cpus, np) {
>> >> > +               u32 freq;
>> >> > +
>> >> > +               if (!of_property_read_u32(np, "clock-frequency",
>> >> > &freq))
>> >> > +                       max_freq = max(max_freq, freq);
>> >> >
>> >> > -       if (max_freq) {
>> >> > -               if (of_find_compatible_node(NULL, NULL,
>> >> > "arm,cortex-a8"))
>> >> > -                       shmobile_setup_delay_hz(max_freq, 1, 3);
>> >> > -               else if (of_find_compatible_node(NULL, NULL,
>> >> > "arm,cortex-a9")) -
>> >> > shmobile_setup_delay_hz(max_freq, 1, 3);
>> >> > -               else if (of_find_compatible_node(NULL, NULL,
>> >> > "arm,cortex-a15")) -                       if
>> >> > (!IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
>> >> > -                               shmobile_setup_delay_hz(max_freq, 2,
>> >> > 4);
>> >> > +               if (of_device_is_compatible(np, "arm,cortex-a8") ||
>> >> > +                   of_device_is_compatible(np, "arm,cortex-a9"))
>> >> > +                       is_a8_a9 = true;
>> >>
>> >> So now we have to care about CPU families in two places: here and below,
>> >> when calling shmobile_setup_delay_hz().
>> >>
>> >> IIUC, "shmobile_setup_delay_hz(max_freq, 2, 4)" is (ignoring rounding)
>> >> equivalent to "shmobile_setup_delay_hz(max_freq, 1, 2)"?
>> >> Do we actually have cases where mult needs to be different from 1?
>> >> If not, we can kill the "mult" parameter, and replace the "is_a8_a9 >> >> true"
>> >> by:
>> >>
>> >> div = 3;
>> >>
>> >> > +               else if (of_device_is_compatible(np, "arm,cortex-a15"))
>> >> > +                       is_a15 = true;
>> >>
>> >> and
>> >>
>> >> else if (of_device_is_compatible(np, "arm,cortex-a15") &&
>> >>
>> >>          !IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
>> >>
>> >>         div = 2;
>> >>
>> >> >         }
>> >> >
>> >> > +
>> >> > +       of_node_put(cpus);
>> >> > +
>> >> > +       if (!max_freq)
>> >> > +               return;
>> >> > +
>> >> > +       if (is_a8_a9)
>> >> > +               shmobile_setup_delay_hz(max_freq, 1, 3);
>> >> > +       else if (is_a15 && !IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
>> >> > +               shmobile_setup_delay_hz(max_freq, 2, 4);
>> >>
>> >> and this just becomes:
>> >>
>> >> if (div)
>> >>
>> >>     shmobile_setup_delay_hz(max_freq, div);
>> >>
>> >> Am I missing something?
>> >
>> > That's an option as well, but I find the code flow to slightly be less
>> > clear in that case. I have no strong preference so I'll let Magnus
>> > decide.
>>
>> Ouch, I will have to decide? =)
>>
>> If we consider upcoming R-Car E2 that is Cortex-A7 only, i wonder
>> which way to go to also support that easily?
>
> Depends, what do we need to do on that platform ?

Like on other SoCs - make sure the delay timings are correct! What
makes it special is that it is our first CA7-only platform.

Not sure how different the CA7 would be from say CA15 though. And the
existing values may be far from perfect. So I guess someone needs to
go through the delay cycles in general and figure out what the correct
settings would be.

Thanks,

/ magnus

Re: [PATCH] ARM: shmobile: Fix device node reference leakage in shmobile_init_delay

[Laurent Pinchart]

Hi Magnus,

On Thursday 22 May 2014 12:10:06 Magnus Damm wrote:
> On Thu, May 22, 2014 at 9:50 AM, Laurent Pinchart wrote:
> > On Thursday 22 May 2014 09:24:47 Magnus Damm wrote:
> >> On Thu, May 22, 2014 at 1:13 AM, Laurent Pinchart wrote:
> >> > On Wednesday 21 May 2014 15:56:20 Geert Uytterhoeven wrote:
> >> >> On Wed, May 21, 2014 at 3:31 PM, Laurent Pinchart wrote:
> >> >> > +       bool is_a8_a9 = false;
> >> >> > +       bool is_a15 = false;
> >> >> > +       u32 max_freq = 0;
> >> >> > +
> >> >> > +       cpus = of_find_node_by_path("/cpus");
> >> >> > +       if (!cpus)
> >> >> > +               return;
> >> >> > +
> >> >> > +       for_each_child_of_node(cpus, np) {
> >> >> > +               u32 freq;
> >> >> > +
> >> >> > +               if (!of_property_read_u32(np, "clock-frequency",
> >> >> > &freq))
> >> >> > +                       max_freq = max(max_freq, freq);
> >> >> > 
> >> >> > -       if (max_freq) {
> >> >> > -               if (of_find_compatible_node(NULL, NULL,
> >> >> > "arm,cortex-a8"))
> >> >> > -                       shmobile_setup_delay_hz(max_freq, 1, 3);
> >> >> > -               else if (of_find_compatible_node(NULL, NULL,
> >> >> > "arm,cortex-a9")) -
> >> >> > shmobile_setup_delay_hz(max_freq, 1, 3);
> >> >> > -               else if (of_find_compatible_node(NULL, NULL,
> >> >> > "arm,cortex-a15")) -                       if
> >> >> > (!IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
> >> >> > -                               shmobile_setup_delay_hz(max_freq, 2,
> >> >> > 4);
> >> >> > +               if (of_device_is_compatible(np, "arm,cortex-a8") ||
> >> >> > +                   of_device_is_compatible(np, "arm,cortex-a9"))
> >> >> > +                       is_a8_a9 = true;
> >> >> 
> >> >> So now we have to care about CPU families in two places: here and
> >> >> below, when calling shmobile_setup_delay_hz().
> >> >> 
> >> >> IIUC, "shmobile_setup_delay_hz(max_freq, 2, 4)" is (ignoring rounding)
> >> >> equivalent to "shmobile_setup_delay_hz(max_freq, 1, 2)"?
> >> >> Do we actually have cases where mult needs to be different from 1?
> >> >> If not, we can kill the "mult" parameter, and replace the "is_a8_a9 > >> >> true" by:
> >> >> 
> >> >> div = 3;
> >> >> 
> >> >> > +               else if (of_device_is_compatible(np,
> >> >> > "arm,cortex-a15"))
> >> >> > +                       is_a15 = true;
> >> >> 
> >> >> and
> >> >> 
> >> >> else if (of_device_is_compatible(np, "arm,cortex-a15") &&
> >> >>          !IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
> >> >>         div = 2;
> >> >>         
> >> >> >         }
> >> >> > 
> >> >> > +
> >> >> > +       of_node_put(cpus);
> >> >> > +
> >> >> > +       if (!max_freq)
> >> >> > +               return;
> >> >> > +
> >> >> > +       if (is_a8_a9)
> >> >> > +               shmobile_setup_delay_hz(max_freq, 1, 3);
> >> >> > +       else if (is_a15 && !IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
> >> >> > +               shmobile_setup_delay_hz(max_freq, 2, 4);
> >> >> 
> >> >> and this just becomes:
> >> >> 
> >> >> if (div)
> >> >> 
> >> >>     shmobile_setup_delay_hz(max_freq, div);
> >> >> 
> >> >> Am I missing something?
> >> > 
> >> > That's an option as well, but I find the code flow to slightly be less
> >> > clear in that case. I have no strong preference so I'll let Magnus
> >> > decide.
> >> 
> >> Ouch, I will have to decide? =)
> >> 
> >> If we consider upcoming R-Car E2 that is Cortex-A7 only, i wonder
> >> which way to go to also support that easily?
> > 
> > Depends, what do we need to do on that platform ?
> 
> Like on other SoCs - make sure the delay timings are correct!

Of course, but what does that look like for E2 in terms of div and mult 
values, and how that relates to the architected timer being available or not 
(and possibly other parameters) ?

As we don't seem to have that information at the moment I would vote for 
ignoring E2 for now and revisiting the code later. It's a pretty simple 
function anyway.

So, back to square one, what's your preferred implementation ? :-)

> What makes it special is that it is our first CA7-only platform.
> 
> Not sure how different the CA7 would be from say CA15 though. And the
> existing values may be far from perfect. So I guess someone needs to
> go through the delay cycles in general and figure out what the correct
> settings would be.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] ARM: shmobile: Fix device node reference leakage in shmobile_init_delay

[Magnus Damm]

Hi Laurent,

On Wed, May 21, 2014 at 10:31 PM, Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:
> The of_find_compatible_node() function returns a new reference to the
> found node. Instead of just adding of_node_put() calls, simplify the
> code by moving the CPU identification logic inside the loop over cpu
> nodes, in order to lower complexity from O(n) to O(1) by replacing
> of_find_compatible_node() calls with of_device_is_compatible().
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  arch/arm/mach-shmobile/timer.c | 50 ++++++++++++++++++++++++------------------
>  1 file changed, 29 insertions(+), 21 deletions(-)

Thanks for fixing this! Looks good to me.

Acked-by: Magnus Damm <damm+renesas@opensource.se>

Re: [PATCH] ARM: shmobile: Fix device node reference leakage in shmobile_init_delay

[Magnus Damm]

Hi Geert,

On Wed, May 21, 2014 at 10:56 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Laurent, Magnus,
>
> On Wed, May 21, 2014 at 3:31 PM, Laurent Pinchart
> <laurent.pinchart+renesas@ideasonboard.com> wrote:
>> +       bool is_a8_a9 = false;
>> +       bool is_a15 = false;
>> +       u32 max_freq = 0;
>> +
>> +       cpus = of_find_node_by_path("/cpus");
>> +       if (!cpus)
>> +               return;
>> +
>> +       for_each_child_of_node(cpus, np) {
>> +               u32 freq;
>> +
>> +               if (!of_property_read_u32(np, "clock-frequency", &freq))
>> +                       max_freq = max(max_freq, freq);
>>
>> -       if (max_freq) {
>> -               if (of_find_compatible_node(NULL, NULL, "arm,cortex-a8"))
>> -                       shmobile_setup_delay_hz(max_freq, 1, 3);
>> -               else if (of_find_compatible_node(NULL, NULL, "arm,cortex-a9"))
>> -                       shmobile_setup_delay_hz(max_freq, 1, 3);
>> -               else if (of_find_compatible_node(NULL, NULL, "arm,cortex-a15"))
>> -                       if (!IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
>> -                               shmobile_setup_delay_hz(max_freq, 2, 4);
>> +               if (of_device_is_compatible(np, "arm,cortex-a8") ||
>> +                   of_device_is_compatible(np, "arm,cortex-a9"))
>> +                       is_a8_a9 = true;
>
> So now we have to care about CPU families in two places: here and below,
> when calling shmobile_setup_delay_hz().
>
> IIUC, "shmobile_setup_delay_hz(max_freq, 2, 4)" is (ignoring rounding)
> equivalent to "shmobile_setup_delay_hz(max_freq, 1, 2)"?
> Do we actually have cases where mult needs to be different from 1?
> If not, we can kill the "mult" parameter, and replace the "is_a8_a9 = true"
> by:
>
> div = 3;
>
>> +               else if (of_device_is_compatible(np, "arm,cortex-a15"))
>> +                       is_a15 = true;
>
> and
>
> else if (of_device_is_compatible(np, "arm,cortex-a15") &&
>          !IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
>         div = 2;
>
>>         }
>> +
>> +       of_node_put(cpus);
>> +
>> +       if (!max_freq)
>> +               return;
>> +
>> +       if (is_a8_a9)
>> +               shmobile_setup_delay_hz(max_freq, 1, 3);
>> +       else if (is_a15 && !IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
>> +               shmobile_setup_delay_hz(max_freq, 2, 4);
>
> and this just becomes:
>
> if (div)
>     shmobile_setup_delay_hz(max_freq, div);
>
> Am I missing something?

Your logic sounds sane to me, but I think this should be handled after
we figure out how to deal with CA7.

Incremental patches are always welcome! And the CA7 behavior can be
tested on Lager already.

Thanks,

/ magnus

Re: [PATCH] ARM: shmobile: Fix device node reference leakage in shmobile_init_delay

[Simon Horman]

On Thu, May 22, 2014 at 08:35:51PM +0900, Magnus Damm wrote:
> Hi Laurent,
> 
> On Wed, May 21, 2014 at 10:31 PM, Laurent Pinchart
> <laurent.pinchart+renesas@ideasonboard.com> wrote:
> > The of_find_compatible_node() function returns a new reference to the
> > found node. Instead of just adding of_node_put() calls, simplify the
> > code by moving the CPU identification logic inside the loop over cpu
> > nodes, in order to lower complexity from O(n) to O(1) by replacing
> > of_find_compatible_node() calls with of_device_is_compatible().
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  arch/arm/mach-shmobile/timer.c | 50 ++++++++++++++++++++++++------------------
> >  1 file changed, 29 insertions(+), 21 deletions(-)
> 
> Thanks for fixing this! Looks good to me.
> 
> Acked-by: Magnus Damm <damm+renesas@opensource.se>

Thanks, I have queued this up.

[PATCH] ARM: shmobile: Fix device node reference leakage in shmobile_init_delay

[Simon Horman]

From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

The of_find_compatible_node() function returns a new reference to the
found node. Instead of just adding of_node_put() calls, simplify the
code by moving the CPU identification logic inside the loop over cpu
nodes, in order to lower complexity from O(n) to O(1) by replacing
of_find_compatible_node() calls with of_device_is_compatible().

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Acked-by: Magnus Damm <damm+renesas@opensource.se>
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
 arch/arm/mach-shmobile/timer.c | 50 ++++++++++++++++++++++++------------------
 1 file changed, 29 insertions(+), 21 deletions(-)

diff --git a/arch/arm/mach-shmobile/timer.c b/arch/arm/mach-shmobile/timer.c
index 68bc0b8..942efdc 100644
--- a/arch/arm/mach-shmobile/timer.c
+++ b/arch/arm/mach-shmobile/timer.c
@@ -59,29 +59,37 @@ void __init shmobile_setup_delay(unsigned int max_cpu_core_mhz,
 
 void __init shmobile_init_delay(void)
 {
-	struct device_node *np, *parent;
-	u32 max_freq, freq;
-
-	max_freq = 0;
-
-	parent = of_find_node_by_path("/cpus");
-	if (parent) {
-		for_each_child_of_node(parent, np) {
-			if (!of_property_read_u32(np, "clock-frequency", &freq))
-				max_freq = max(max_freq, freq);
-		}
-		of_node_put(parent);
-	}
+	struct device_node *np, *cpus;
+	bool is_a8_a9 = false;
+	bool is_a15 = false;
+	u32 max_freq = 0;
+
+	cpus = of_find_node_by_path("/cpus");
+	if (!cpus)
+		return;
+
+	for_each_child_of_node(cpus, np) {
+		u32 freq;
+
+		if (!of_property_read_u32(np, "clock-frequency", &freq))
+			max_freq = max(max_freq, freq);
 
-	if (max_freq) {
-		if (of_find_compatible_node(NULL, NULL, "arm,cortex-a8"))
-			shmobile_setup_delay_hz(max_freq, 1, 3);
-		else if (of_find_compatible_node(NULL, NULL, "arm,cortex-a9"))
-			shmobile_setup_delay_hz(max_freq, 1, 3);
-		else if (of_find_compatible_node(NULL, NULL, "arm,cortex-a15"))
-			if (!IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
-				shmobile_setup_delay_hz(max_freq, 2, 4);
+		if (of_device_is_compatible(np, "arm,cortex-a8") ||
+		    of_device_is_compatible(np, "arm,cortex-a9"))
+			is_a8_a9 = true;
+		else if (of_device_is_compatible(np, "arm,cortex-a15"))
+			is_a15 = true;
 	}
+
+	of_node_put(cpus);
+
+	if (!max_freq)
+		return;
+
+	if (is_a8_a9)
+		shmobile_setup_delay_hz(max_freq, 1, 3);
+	else if (is_a15 && !IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
+		shmobile_setup_delay_hz(max_freq, 2, 4);
 }
 
 static void __init shmobile_late_time_init(void)
-- 
2.0.0.rc2