* Re: [PATCH] ARM: shmobile: Fix device node reference leakage in shmobile_init_delay
2014-05-21 13:31 [PATCH] ARM: shmobile: Fix device node reference leakage in shmobile_init_delay Laurent Pinchart
@ 2014-05-21 13:56 ` Geert Uytterhoeven
2014-05-21 16:13 ` Laurent Pinchart
` (7 subsequent siblings)
8 siblings, 0 replies; 11+ messages in thread
From: Geert Uytterhoeven @ 2014-05-21 13:56 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] ARM: shmobile: Fix device node reference leakage in shmobile_init_delay
2014-05-21 13:31 [PATCH] ARM: shmobile: Fix device node reference leakage in shmobile_init_delay Laurent Pinchart
2014-05-21 13:56 ` Geert Uytterhoeven
@ 2014-05-21 16:13 ` Laurent Pinchart
2014-05-22 0:24 ` Magnus Damm
` (6 subsequent siblings)
8 siblings, 0 replies; 11+ messages in thread
From: Laurent Pinchart @ 2014-05-21 16:13 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] ARM: shmobile: Fix device node reference leakage in shmobile_init_delay
2014-05-21 13:31 [PATCH] ARM: shmobile: Fix device node reference leakage in shmobile_init_delay Laurent Pinchart
2014-05-21 13:56 ` Geert Uytterhoeven
2014-05-21 16:13 ` Laurent Pinchart
@ 2014-05-22 0:24 ` Magnus Damm
2014-05-22 0:50 ` Laurent Pinchart
` (5 subsequent siblings)
8 siblings, 0 replies; 11+ messages in thread
From: Magnus Damm @ 2014-05-22 0:24 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] ARM: shmobile: Fix device node reference leakage in shmobile_init_delay
2014-05-21 13:31 [PATCH] ARM: shmobile: Fix device node reference leakage in shmobile_init_delay Laurent Pinchart
` (2 preceding siblings ...)
2014-05-22 0:24 ` Magnus Damm
@ 2014-05-22 0:50 ` Laurent Pinchart
2014-05-22 3:10 ` Magnus Damm
` (4 subsequent siblings)
8 siblings, 0 replies; 11+ messages in thread
From: Laurent Pinchart @ 2014-05-22 0:50 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] ARM: shmobile: Fix device node reference leakage in shmobile_init_delay
2014-05-21 13:31 [PATCH] ARM: shmobile: Fix device node reference leakage in shmobile_init_delay Laurent Pinchart
` (3 preceding siblings ...)
2014-05-22 0:50 ` Laurent Pinchart
@ 2014-05-22 3:10 ` Magnus Damm
2014-05-22 9:42 ` Laurent Pinchart
` (3 subsequent siblings)
8 siblings, 0 replies; 11+ messages in thread
From: Magnus Damm @ 2014-05-22 3:10 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] ARM: shmobile: Fix device node reference leakage in shmobile_init_delay
2014-05-21 13:31 [PATCH] ARM: shmobile: Fix device node reference leakage in shmobile_init_delay Laurent Pinchart
` (4 preceding siblings ...)
2014-05-22 3:10 ` Magnus Damm
@ 2014-05-22 9:42 ` Laurent Pinchart
2014-05-22 11:35 ` Magnus Damm
` (2 subsequent siblings)
8 siblings, 0 replies; 11+ messages in thread
From: Laurent Pinchart @ 2014-05-22 9:42 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] ARM: shmobile: Fix device node reference leakage in shmobile_init_delay
2014-05-21 13:31 [PATCH] ARM: shmobile: Fix device node reference leakage in shmobile_init_delay Laurent Pinchart
` (5 preceding siblings ...)
2014-05-22 9:42 ` Laurent Pinchart
@ 2014-05-22 11:35 ` Magnus Damm
2014-05-22 11:37 ` Magnus Damm
2014-05-26 1:56 ` Simon Horman
8 siblings, 0 replies; 11+ messages in thread
From: Magnus Damm @ 2014-05-22 11:35 UTC (permalink / raw)
To: linux-sh
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>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] ARM: shmobile: Fix device node reference leakage in shmobile_init_delay
2014-05-21 13:31 [PATCH] ARM: shmobile: Fix device node reference leakage in shmobile_init_delay Laurent Pinchart
` (6 preceding siblings ...)
2014-05-22 11:35 ` Magnus Damm
@ 2014-05-22 11:37 ` Magnus Damm
2014-05-26 1:56 ` Simon Horman
8 siblings, 0 replies; 11+ messages in thread
From: Magnus Damm @ 2014-05-22 11:37 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] ARM: shmobile: Fix device node reference leakage in shmobile_init_delay
2014-05-21 13:31 [PATCH] ARM: shmobile: Fix device node reference leakage in shmobile_init_delay Laurent Pinchart
` (7 preceding siblings ...)
2014-05-22 11:37 ` Magnus Damm
@ 2014-05-26 1:56 ` Simon Horman
8 siblings, 0 replies; 11+ messages in thread
From: Simon Horman @ 2014-05-26 1:56 UTC (permalink / raw)
To: linux-sh
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.
^ permalink raw reply [flat|nested] 11+ messages in thread