[PATCH 00/03] ARM: shmobile: Use shmobile_init_delay() on r8a7791

[PATCH 00/03] ARM: shmobile: Use shmobile_init_delay() on r8a7791

[Magnus Damm]

ARM: shmobile: Use shmobile_init_delay() on r8a7791

[PATCH 01/03] ARM: shmobile: Use r8a7791 DT CPU Frequency in common case	
[PATCH 02/03] ARM: shmobile: Use r8a7791 DT CPU Frequency for Koelsch
[PATCH 03/03] ARM: shmobile: Remove unused r8a7791_init_early()

Convert r8a7791 to rely on shmobile_init_delay() instead of using a per-SoC
delay setup function.

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

 Written against renesas-devel-v3.15-rc5-20140511

 arch/arm/mach-shmobile/board-koelsch-reference.c |    2 +-
 arch/arm/mach-shmobile/board-koelsch.c           |    2 +-
 arch/arm/mach-shmobile/include/mach/r8a7791.h    |    1 -
 arch/arm/mach-shmobile/setup-r8a7791.c           |    9 +--------
 4 files changed, 3 insertions(+), 11 deletions(-)

[PATCH 01/03] ARM: shmobile: Use r8a7791 DT CPU Frequency in common case

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Convert the common C-code-less r8a7791 DT board support
to use shmobile_init_delay() to be able to migrate away
from per-SoC delay setup functions.

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

 arch/arm/mach-shmobile/setup-r8a7791.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- 0001/arch/arm/mach-shmobile/setup-r8a7791.c
+++ work/arch/arm/mach-shmobile/setup-r8a7791.c	2014-05-12 08:02:27.000000000 +0900
@@ -222,7 +222,7 @@ static const char *r8a7791_boards_compat
 
 DT_MACHINE_START(R8A7791_DT, "Generic R8A7791 (Flattened Device Tree)")
 	.smp		= smp_ops(r8a7791_smp_ops),
-	.init_early	= r8a7791_init_early,
+	.init_early	= shmobile_init_delay,
 	.init_time	= rcar_gen2_timer_init,
 	.dt_compat	= r8a7791_boards_compat_dt,
 MACHINE_END

[PATCH 02/03] ARM: shmobile: Use r8a7791 DT CPU Frequency for Koelsch

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Convert the Koelsch board support to use shmobile_init_delay()
to be able to migrate away from per-SoC delay setup functions.

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

 arch/arm/mach-shmobile/board-koelsch-reference.c |    2 +-
 arch/arm/mach-shmobile/board-koelsch.c           |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

--- 0001/arch/arm/mach-shmobile/board-koelsch-reference.c
+++ work/arch/arm/mach-shmobile/board-koelsch-reference.c	2014-05-12 08:04:07.000000000 +0900
@@ -139,7 +139,7 @@ static const char * const koelsch_boards
 
 DT_MACHINE_START(KOELSCH_DT, "koelsch")
 	.smp		= smp_ops(r8a7791_smp_ops),
-	.init_early	= r8a7791_init_early,
+	.init_early	= shmobile_init_delay,
 	.init_time	= rcar_gen2_timer_init,
 	.init_machine	= koelsch_add_standard_devices,
 	.init_late	= shmobile_init_late,
--- 0001/arch/arm/mach-shmobile/board-koelsch.c
+++ work/arch/arm/mach-shmobile/board-koelsch.c	2014-05-12 08:03:53.000000000 +0900
@@ -522,7 +522,7 @@ static const char * const koelsch_boards
 
 DT_MACHINE_START(KOELSCH_DT, "koelsch")
 	.smp		= smp_ops(r8a7791_smp_ops),
-	.init_early	= r8a7791_init_early,
+	.init_early	= shmobile_init_delay,
 	.init_time	= rcar_gen2_timer_init,
 	.init_machine	= koelsch_init,
 	.init_late	= shmobile_init_late,

[PATCH 03/03] ARM: shmobile: Remove unused r8a7791_init_early()

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Remove the now unused r8a7791_init_early() function.

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

 arch/arm/mach-shmobile/include/mach/r8a7791.h |    1 -
 arch/arm/mach-shmobile/setup-r8a7791.c        |    7 -------
 2 files changed, 8 deletions(-)

--- 0001/arch/arm/mach-shmobile/include/mach/r8a7791.h
+++ work/arch/arm/mach-shmobile/include/mach/r8a7791.h	2014-05-12 08:05:58.000000000 +0900
@@ -5,7 +5,6 @@ void r8a7791_add_standard_devices(void);
 void r8a7791_add_dt_devices(void);
 void r8a7791_clock_init(void);
 void r8a7791_pinmux_init(void);
-void r8a7791_init_early(void);
 extern struct smp_operations r8a7791_smp_ops;
 
 #endif /* __ASM_R8A7791_H__ */
--- 0006/arch/arm/mach-shmobile/setup-r8a7791.c
+++ work/arch/arm/mach-shmobile/setup-r8a7791.c	2014-05-12 08:05:22.000000000 +0900
@@ -207,13 +207,6 @@ void __init r8a7791_add_standard_devices
 	r8a7791_register_thermal();
 }
 
-void __init r8a7791_init_early(void)
-{
-#ifndef CONFIG_ARM_ARCH_TIMER
-	shmobile_setup_delay(1500, 2, 4); /* Cortex-A15 @ 1500MHz */
-#endif
-}
-
 #ifdef CONFIG_USE_OF
 static const char *r8a7791_boards_compat_dt[] __initdata = {
 	"renesas,r8a7791",

Re: [PATCH 00/03] ARM: shmobile: Use shmobile_init_delay() on r8a7791

[Simon Horman]

On Mon, May 12, 2014 at 08:25:08AM +0900, Magnus Damm wrote:
> ARM: shmobile: Use shmobile_init_delay() on r8a7791
> 
> [PATCH 01/03] ARM: shmobile: Use r8a7791 DT CPU Frequency in common case	
> [PATCH 02/03] ARM: shmobile: Use r8a7791 DT CPU Frequency for Koelsch
> [PATCH 03/03] ARM: shmobile: Remove unused r8a7791_init_early()
> 
> Convert r8a7791 to rely on shmobile_init_delay() instead of using a per-SoC
> delay setup function.

Thanks, I will queue this up.

Re: [PATCH 00/03] ARM: shmobile: Use shmobile_init_delay() on r8a7791

[Laurent Pinchart]

Hi Magnus,

On Monday 12 May 2014 08:25:08 Magnus Damm wrote:
> ARM: shmobile: Use shmobile_init_delay() on r8a7791
> 
> [PATCH 01/03] ARM: shmobile: Use r8a7791 DT CPU Frequency in common case
> [PATCH 02/03] ARM: shmobile: Use r8a7791 DT CPU Frequency for Koelsch
> [PATCH 03/03] ARM: shmobile: Remove unused r8a7791_init_early()
> 
> Convert r8a7791 to rely on shmobile_init_delay() instead of using a per-SoC
> delay setup function.

I'm afraid this patch set causes a boot time and CPU load regression on 
Koelsch. With Simon's latest devel branch which has the patches applied, the 
kernel boot time (using koelsch_defconfig) up to the point where the kernel 
tries to connect to the DHCP server is 17.898437s. The CPU load (as reported 
by top) on an idle system is then around 33%, spent in [kworker/0:1]. If I 
revert the patches the same time goes down to 1.921875s and the CPU load on an 
idle system is close to 0.

> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> ---
> 
>  Written against renesas-devel-v3.15-rc5-20140511
> 
>  arch/arm/mach-shmobile/board-koelsch-reference.c |    2 +-
>  arch/arm/mach-shmobile/board-koelsch.c           |    2 +-
>  arch/arm/mach-shmobile/include/mach/r8a7791.h    |    1 -
>  arch/arm/mach-shmobile/setup-r8a7791.c           |    9 +--------
>  4 files changed, 3 insertions(+), 11 deletions(-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 00/03] ARM: shmobile: Use shmobile_init_delay() on r8a7791

[Simon Horman]

On Mon, May 12, 2014 at 10:00:08PM +0200, Laurent Pinchart wrote:
> Hi Magnus,
> 
> On Monday 12 May 2014 08:25:08 Magnus Damm wrote:
> > ARM: shmobile: Use shmobile_init_delay() on r8a7791
> > 
> > [PATCH 01/03] ARM: shmobile: Use r8a7791 DT CPU Frequency in common case
> > [PATCH 02/03] ARM: shmobile: Use r8a7791 DT CPU Frequency for Koelsch
> > [PATCH 03/03] ARM: shmobile: Remove unused r8a7791_init_early()
> > 
> > Convert r8a7791 to rely on shmobile_init_delay() instead of using a per-SoC
> > delay setup function.
> 
> I'm afraid this patch set causes a boot time and CPU load regression on 
> Koelsch. With Simon's latest devel branch which has the patches applied, the 
> kernel boot time (using koelsch_defconfig) up to the point where the kernel 
> tries to connect to the DHCP server is 17.898437s. The CPU load (as reported 
> by top) on an idle system is then around 33%, spent in [kworker/0:1]. If I 
> revert the patches the same time goes down to 1.921875s and the CPU load on an 
> idle system is close to 0.

Thanks for checking this Laurent.
I have this and the (related to my eyes) r8a7740 changes in their own
branch. For now I will leave it in next but refrain from sending a
pull-request. I will drop it from next if the problem hasn't been resolved
in time for rc6 (which seems likely).

> > Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> > ---
> > 
> >  Written against renesas-devel-v3.15-rc5-20140511
> > 
> >  arch/arm/mach-shmobile/board-koelsch-reference.c |    2 +-
> >  arch/arm/mach-shmobile/board-koelsch.c           |    2 +-
> >  arch/arm/mach-shmobile/include/mach/r8a7791.h    |    1 -
> >  arch/arm/mach-shmobile/setup-r8a7791.c           |    9 +--------
> >  4 files changed, 3 insertions(+), 11 deletions(-)
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

Re: [PATCH 00/03] ARM: shmobile: Use shmobile_init_delay() on r8a7791

[Simon Horman]

On Tue, May 13, 2014 at 09:59:36AM +0900, Simon Horman wrote:
> On Mon, May 12, 2014 at 10:00:08PM +0200, Laurent Pinchart wrote:
> > Hi Magnus,
> > 
> > On Monday 12 May 2014 08:25:08 Magnus Damm wrote:
> > > ARM: shmobile: Use shmobile_init_delay() on r8a7791
> > > 
> > > [PATCH 01/03] ARM: shmobile: Use r8a7791 DT CPU Frequency in common case
> > > [PATCH 02/03] ARM: shmobile: Use r8a7791 DT CPU Frequency for Koelsch
> > > [PATCH 03/03] ARM: shmobile: Remove unused r8a7791_init_early()
> > > 
> > > Convert r8a7791 to rely on shmobile_init_delay() instead of using a per-SoC
> > > delay setup function.
> > 
> > I'm afraid this patch set causes a boot time and CPU load regression on 
> > Koelsch. With Simon's latest devel branch which has the patches applied, the 
> > kernel boot time (using koelsch_defconfig) up to the point where the kernel 
> > tries to connect to the DHCP server is 17.898437s. The CPU load (as reported 
> > by top) on an idle system is then around 33%, spent in [kworker/0:1]. If I 
> > revert the patches the same time goes down to 1.921875s and the CPU load on an 
> > idle system is close to 0.
> 
> Thanks for checking this Laurent.
> I have this and the (related to my eyes) r8a7740 changes in their own
> branch. For now I will leave it in next but refrain from sending a
> pull-request. I will drop it from next if the problem hasn't been resolved
> in time for rc6 (which seems likely).

Hi Laurent, Hi Magnus,

I have done some further investigation and I believe that the problem
is that "clock-frequency" node values are in Hz while shmobile_setup_delay
expects an argument in MHz.

I propose the following:

From: Simon Horman <horms+renesas@verge.net.au>

[PATCH] ARM: shmobile: Set clock frequency in HZ from OF nodes

shmobile_init_delay() looks for OF "clock-frequency" to determine
the delay which is set by calling shmobile_setup_delay().

Unfortunately this seems to be incorrect in detail as
"clock-frequency" node values are in HZ whereas the frequency
argument to shmobile_setup_delay() is in MHz.

Provide a variant of shmobile_setup_delay() that accepts HZ to
correct this problem.

Signed-off-by: Simon Horman <horms+renesas@verge.net.au>

diff --git a/arch/arm/mach-shmobile/timer.c b/arch/arm/mach-shmobile/timer.c
index ccecde9..8d80a33 100644
--- a/arch/arm/mach-shmobile/timer.c
+++ b/arch/arm/mach-shmobile/timer.c
@@ -23,21 +23,24 @@
 #include <linux/delay.h>
 #include <linux/of_address.h>
 
-void __init shmobile_setup_delay(unsigned int max_cpu_core_mhz,
-				 unsigned int mult, unsigned int div)
+void __init shmobile_setup_delay_hz(unsigned int max_cpu_core_hz,
+				    unsigned int mult, unsigned int div)
 {
 	/* calculate a worst-case loops-per-jiffy value
-	 * based on maximum cpu core mhz setting and the
+	 * based on maximum cpu core hz setting and the
 	 * __delay() implementation in arch/arm/lib/delay.S
 	 *
 	 * this will result in a longer delay than expected
 	 * when the cpu core runs on lower frequencies.
 	 */
-
-	unsigned int value = (1000000 * mult) / (HZ * div);
-
 	if (!preset_lpj)
-		preset_lpj = max_cpu_core_mhz * value;
+		preset_lpj = max_cpu_core_hz * mult / (HZ * div);
+}
+
+void __init shmobile_setup_delay(unsigned int max_cpu_core_mhz,
+				 unsigned int mult, unsigned int div)
+{
+	shmobile_setup_delay_hz(max_cpu_core_mhz, mult * 100000, div);
 }
 
 void __init shmobile_init_delay(void)
@@ -58,12 +61,12 @@ void __init shmobile_init_delay(void)
 
 	if (max_freq) {
 		if (of_find_compatible_node(NULL, NULL, "arm,cortex-a8"))
-			shmobile_setup_delay(max_freq, 1, 3);
+			shmobile_setup_delay_hz(max_freq, 1, 3);
 		else if (of_find_compatible_node(NULL, NULL, "arm,cortex-a9"))
-			shmobile_setup_delay(max_freq, 1, 3);
+			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(max_freq, 2, 4);
+				shmobile_setup_delay_hz(max_freq, 2, 4);
 	}
 }
 

Re: [PATCH 00/03] ARM: shmobile: Use shmobile_init_delay() on r8a7791

[Geert Uytterhoeven]

Hi Simon,

On Tue, May 13, 2014 at 9:12 AM, Simon Horman <horms@verge.net.au> wrote:
> From: Simon Horman <horms+renesas@verge.net.au>
>
> [PATCH] ARM: shmobile: Set clock frequency in HZ from OF nodes
>
> shmobile_init_delay() looks for OF "clock-frequency" to determine
> the delay which is set by calling shmobile_setup_delay().
>
> Unfortunately this seems to be incorrect in detail as
> "clock-frequency" node values are in HZ whereas the frequency
> argument to shmobile_setup_delay() is in MHz.

Indeed. Nice catch.

> Provide a variant of shmobile_setup_delay() that accepts HZ to
> correct this problem.
>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
>
> diff --git a/arch/arm/mach-shmobile/timer.c b/arch/arm/mach-shmobile/timer.c
> index ccecde9..8d80a33 100644
> --- a/arch/arm/mach-shmobile/timer.c
> +++ b/arch/arm/mach-shmobile/timer.c
> @@ -23,21 +23,24 @@
>  #include <linux/delay.h>
>  #include <linux/of_address.h>
>
> -void __init shmobile_setup_delay(unsigned int max_cpu_core_mhz,
> -                                unsigned int mult, unsigned int div)
> +void __init shmobile_setup_delay_hz(unsigned int max_cpu_core_hz,
> +                                   unsigned int mult, unsigned int div)
>  {
>         /* calculate a worst-case loops-per-jiffy value
> -        * based on maximum cpu core mhz setting and the
> +        * based on maximum cpu core hz setting and the
>          * __delay() implementation in arch/arm/lib/delay.S
>          *
>          * this will result in a longer delay than expected
>          * when the cpu core runs on lower frequencies.
>          */
> -
> -       unsigned int value = (1000000 * mult) / (HZ * div);
> -
>         if (!preset_lpj)
> -               preset_lpj = max_cpu_core_mhz * value;
> +               preset_lpj = max_cpu_core_hz * mult / (HZ * div);

I believe it was written this way to avoid integer overflow.
As soon as we get a 2.2 GHz A15 (mult = 2), "max_cpu_core_hz * mult"
will overflow.

When using Hz instead of MHz, we probably want to switch from
multiplying a small number (in MHz) to dividing a large number (in Hz):

        preset_lpj = max_cpu_core_hz / value;

So value should be:

        value = HZ * div / mult;

which is OK as mult is really small (1 or 2).
Does this look right? Haven't had my coffee yet ;-)

All of this will still overflow when clock-frequency no longer fits in u32,
and we have to revise the bindings ;-)

> +}
> +
> +void __init shmobile_setup_delay(unsigned int max_cpu_core_mhz,
> +                                unsigned int mult, unsigned int div)
> +{
> +       shmobile_setup_delay_hz(max_cpu_core_mhz, mult * 100000, div);

While this works (for now --- no longer with my proposed modifications
above), didn't you intend this?

    shmobile_setup_delay_hz(max_cpu_core_mhz * 1000000, mult, div);

>  }
>
>  void __init shmobile_init_delay(void)
> @@ -58,12 +61,12 @@ void __init shmobile_init_delay(void)
>
>         if (max_freq) {
>                 if (of_find_compatible_node(NULL, NULL, "arm,cortex-a8"))
> -                       shmobile_setup_delay(max_freq, 1, 3);
> +                       shmobile_setup_delay_hz(max_freq, 1, 3);
>                 else if (of_find_compatible_node(NULL, NULL, "arm,cortex-a9"))
> -                       shmobile_setup_delay(max_freq, 1, 3);
> +                       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(max_freq, 2, 4);
> +                               shmobile_setup_delay_hz(max_freq, 2, 4);

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 00/03] ARM: shmobile: Use shmobile_init_delay() on r8a7791

[Simon Horman]

On Tue, May 13, 2014 at 09:33:45AM +0200, Geert Uytterhoeven wrote:
> Hi Simon,
> 
> On Tue, May 13, 2014 at 9:12 AM, Simon Horman <horms@verge.net.au> wrote:
> > From: Simon Horman <horms+renesas@verge.net.au>
> >
> > [PATCH] ARM: shmobile: Set clock frequency in HZ from OF nodes
> >
> > shmobile_init_delay() looks for OF "clock-frequency" to determine
> > the delay which is set by calling shmobile_setup_delay().
> >
> > Unfortunately this seems to be incorrect in detail as
> > "clock-frequency" node values are in HZ whereas the frequency
> > argument to shmobile_setup_delay() is in MHz.
> 
> Indeed. Nice catch.
> 
> > Provide a variant of shmobile_setup_delay() that accepts HZ to
> > correct this problem.
> >
> > Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> >
> > diff --git a/arch/arm/mach-shmobile/timer.c b/arch/arm/mach-shmobile/timer.c
> > index ccecde9..8d80a33 100644
> > --- a/arch/arm/mach-shmobile/timer.c
> > +++ b/arch/arm/mach-shmobile/timer.c
> > @@ -23,21 +23,24 @@
> >  #include <linux/delay.h>
> >  #include <linux/of_address.h>
> >
> > -void __init shmobile_setup_delay(unsigned int max_cpu_core_mhz,
> > -                                unsigned int mult, unsigned int div)
> > +void __init shmobile_setup_delay_hz(unsigned int max_cpu_core_hz,
> > +                                   unsigned int mult, unsigned int div)
> >  {
> >         /* calculate a worst-case loops-per-jiffy value
> > -        * based on maximum cpu core mhz setting and the
> > +        * based on maximum cpu core hz setting and the
> >          * __delay() implementation in arch/arm/lib/delay.S
> >          *
> >          * this will result in a longer delay than expected
> >          * when the cpu core runs on lower frequencies.
> >          */
> > -
> > -       unsigned int value = (1000000 * mult) / (HZ * div);
> > -
> >         if (!preset_lpj)
> > -               preset_lpj = max_cpu_core_mhz * value;
> > +               preset_lpj = max_cpu_core_hz * mult / (HZ * div);
> 
> I believe it was written this way to avoid integer overflow.
> As soon as we get a 2.2 GHz A15 (mult = 2), "max_cpu_core_hz * mult"
> will overflow.

Yes, I expect so.

The purpose of my patch was mainly to illustrate the problem
rather than to provide a diffinitive fix..

> When using Hz instead of MHz, we probably want to switch from
> multiplying a small number (in MHz) to dividing a large number (in Hz):
> 
>         preset_lpj = max_cpu_core_hz / value;
> 
> So value should be:
> 
>         value = HZ * div / mult;
> 
> which is OK as mult is really small (1 or 2).
> Does this look right? Haven't had my coffee yet ;-)

It appears to work :)

> All of this will still overflow when clock-frequency no longer fits in u32,
> and we have to revise the bindings ;-)

Awesome!

> 
> > +}
> > +
> > +void __init shmobile_setup_delay(unsigned int max_cpu_core_mhz,
> > +                                unsigned int mult, unsigned int div)
> > +{
> > +       shmobile_setup_delay_hz(max_cpu_core_mhz, mult * 100000, div);
> 
> While this works (for now --- no longer with my proposed modifications
> above), didn't you intend this?
> 
>     shmobile_setup_delay_hz(max_cpu_core_mhz * 1000000, mult, div);

Yes :)


I wonder if the following is any good.


From: Simon Horman <horms+renesas@verge.net.au>

[PATCH] ARM: shmobile: Set clock frequency in HZ from OF nodes

shmobile_init_delay() looks for OF "clock-frequency" to determine
the delay which is set by calling shmobile_setup_delay().

Unfortunately this seems to be incorrect in detail as
"clock-frequency" node values are in HZ whereas the frequency
argument to shmobile_setup_delay() is in MHz.

Provide a variant of shmobile_setup_delay() that accepts HZ to
correct this problem.

Signed-off-by: Simon Horman <horms+renesas@verge.net.au>

---
v2
* As suggested by Geert Uytterhoeven
  - Don't ignore the potential for integer overflow.
    To this end I have taken Geert's suggestion of dividing by
    a large "value" in the HZ case.
    And I have let the original function unchanged for the MHz case:
    it should eventually be removed anyway.
---
 arch/arm/mach-shmobile/timer.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-shmobile/timer.c b/arch/arm/mach-shmobile/timer.c
index ccecde9..68bc0b8 100644
--- a/arch/arm/mach-shmobile/timer.c
+++ b/arch/arm/mach-shmobile/timer.c
@@ -23,6 +23,23 @@
 #include <linux/delay.h>
 #include <linux/of_address.h>
 
+void __init shmobile_setup_delay_hz(unsigned int max_cpu_core_hz,
+				    unsigned int mult, unsigned int div)
+{
+	/* calculate a worst-case loops-per-jiffy value
+	 * based on maximum cpu core hz setting and the
+	 * __delay() implementation in arch/arm/lib/delay.S
+	 *
+	 * this will result in a longer delay than expected
+	 * when the cpu core runs on lower frequencies.
+	 */
+
+	unsigned int value = HZ * div / mult;
+
+	if (!preset_lpj)
+		preset_lpj = max_cpu_core_hz / value;
+}
+
 void __init shmobile_setup_delay(unsigned int max_cpu_core_mhz,
 				 unsigned int mult, unsigned int div)
 {
@@ -58,12 +75,12 @@ void __init shmobile_init_delay(void)
 
 	if (max_freq) {
 		if (of_find_compatible_node(NULL, NULL, "arm,cortex-a8"))
-			shmobile_setup_delay(max_freq, 1, 3);
+			shmobile_setup_delay_hz(max_freq, 1, 3);
 		else if (of_find_compatible_node(NULL, NULL, "arm,cortex-a9"))
-			shmobile_setup_delay(max_freq, 1, 3);
+			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(max_freq, 2, 4);
+				shmobile_setup_delay_hz(max_freq, 2, 4);
 	}
 }
 
-- 
1.8.5.2

Re: [PATCH 00/03] ARM: shmobile: Use shmobile_init_delay() on r8a7791

[Magnus Damm]

On Wed, May 14, 2014 at 12:28 PM, Simon Horman <horms@verge.net.au> wrote:
> I wonder if the following is any good.
>
> From: Simon Horman <horms+renesas@verge.net.au>
>
> [PATCH] ARM: shmobile: Set clock frequency in HZ from OF nodes
>
> shmobile_init_delay() looks for OF "clock-frequency" to determine
> the delay which is set by calling shmobile_setup_delay().
>
> Unfortunately this seems to be incorrect in detail as
> "clock-frequency" node values are in HZ whereas the frequency
> argument to shmobile_setup_delay() is in MHz.
>
> Provide a variant of shmobile_setup_delay() that accepts HZ to
> correct this problem.
>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
>
> ---
> v2
> * As suggested by Geert Uytterhoeven
>   - Don't ignore the potential for integer overflow.
>     To this end I have taken Geert's suggestion of dividing by
>     a large "value" in the HZ case.
>     And I have let the original function unchanged for the MHz case:
>     it should eventually be removed anyway.

Hi Simon and Geert,

Thanks for dealing with the fallout of my cosmetic changes. Nice to
see that you managed to track it down!

I think this patch looks perfect, please merge unless anyone objects.

Cheers,

/ magnus

Re: [PATCH 00/03] ARM: shmobile: Use shmobile_init_delay() on r8a7791

[Simon Horman]

On Wed, May 14, 2014 at 12:46:27PM +0900, Magnus Damm wrote:
> On Wed, May 14, 2014 at 12:28 PM, Simon Horman <horms@verge.net.au> wrote:
> > I wonder if the following is any good.
> >
> > From: Simon Horman <horms+renesas@verge.net.au>
> >
> > [PATCH] ARM: shmobile: Set clock frequency in HZ from OF nodes
> >
> > shmobile_init_delay() looks for OF "clock-frequency" to determine
> > the delay which is set by calling shmobile_setup_delay().
> >
> > Unfortunately this seems to be incorrect in detail as
> > "clock-frequency" node values are in HZ whereas the frequency
> > argument to shmobile_setup_delay() is in MHz.
> >
> > Provide a variant of shmobile_setup_delay() that accepts HZ to
> > correct this problem.
> >
> > Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> >
> > ---
> > v2
> > * As suggested by Geert Uytterhoeven
> >   - Don't ignore the potential for integer overflow.
> >     To this end I have taken Geert's suggestion of dividing by
> >     a large "value" in the HZ case.
> >     And I have let the original function unchanged for the MHz case:
> >     it should eventually be removed anyway.
> 
> Hi Simon and Geert,
> 
> Thanks for dealing with the fallout of my cosmetic changes. Nice to
> see that you managed to track it down!
> 
> I think this patch looks perfect, please merge unless anyone objects.

Thanks, I will go ahead and merge it.

Re: [PATCH 00/03] ARM: shmobile: Use shmobile_init_delay() on r8a7791

[Magnus Damm]

Hi Laurent,

On Tue, May 13, 2014 at 5:00 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> On Monday 12 May 2014 08:25:08 Magnus Damm wrote:
>> ARM: shmobile: Use shmobile_init_delay() on r8a7791
>>
>> [PATCH 01/03] ARM: shmobile: Use r8a7791 DT CPU Frequency in common case
>> [PATCH 02/03] ARM: shmobile: Use r8a7791 DT CPU Frequency for Koelsch
>> [PATCH 03/03] ARM: shmobile: Remove unused r8a7791_init_early()
>>
>> Convert r8a7791 to rely on shmobile_init_delay() instead of using a per-SoC
>> delay setup function.
>
> I'm afraid this patch set causes a boot time and CPU load regression on
> Koelsch. With Simon's latest devel branch which has the patches applied, the
> kernel boot time (using koelsch_defconfig) up to the point where the kernel
> tries to connect to the DHCP server is 17.898437s. The CPU load (as reported
> by top) on an idle system is then around 33%, spent in [kworker/0:1]. If I
> revert the patches the same time goes down to 1.921875s and the CPU load on an
> idle system is close to 0.

Thanks for testing and reporting this! I believe Simon has posted a
fix for this issue which is related to MHz vs Hz bug introduced by me
in function shmobile_init_delay(). Things should be in order now.

Cheers,

/ magnus

[PATCH 00/03] ARM: shmobile: Use shmobile_init_delay() on r8a7790

[Magnus Damm]

ARM: shmobile: Use shmobile_init_delay() on r8a7790

[PATCH 01/03] ARM: shmobile: Use r8a7790 DT CPU Frequency in common case	
[PATCH 02/03] ARM: shmobile: Use r8a7790 DT CPU Frequency for Lager
[PATCH 03/03] ARM: shmobile: Remove unused r8a7790_init_early()

Convert r8a7790 to rely on shmobile_init_delay() instead of using a per-SoC
delay setup function. Very similar to the r8a7791 implementation.

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

 Written against renesas-devel-v3.15-rc5-20140517

 arch/arm/mach-shmobile/board-lager-reference.c |    2 +-
 arch/arm/mach-shmobile/board-lager.c           |    2 +-
 arch/arm/mach-shmobile/include/mach/r8a7790.h  |    1 -
 arch/arm/mach-shmobile/setup-r8a7790.c         |    9 +--------
 4 files changed, 3 insertions(+), 11 deletions(-)

[PATCH 01/03] ARM: shmobile: Use r8a7790 DT CPU Frequency in common case

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Convert the common C-code-less r8a7790 DT board support
to use shmobile_init_delay() to be able to migrate away
from per-SoC delay setup functions.

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

 arch/arm/mach-shmobile/setup-r8a7790.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- 0001/arch/arm/mach-shmobile/setup-r8a7790.c
+++ work/arch/arm/mach-shmobile/setup-r8a7790.c	2014-05-19 22:24:38.000000000 +0900
@@ -323,7 +323,7 @@ static const char * const r8a7790_boards
 
 DT_MACHINE_START(R8A7790_DT, "Generic R8A7790 (Flattened Device Tree)")
 	.smp		= smp_ops(r8a7790_smp_ops),
-	.init_early	= r8a7790_init_early,
+	.init_early	= shmobile_init_delay,
 	.init_time	= rcar_gen2_timer_init,
 	.dt_compat	= r8a7790_boards_compat_dt,
 MACHINE_END

[PATCH 02/03] ARM: shmobile: Use r8a7790 DT CPU Frequency for Lager

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Convert the Lager board support to use shmobile_init_delay()
to be able to migrate away from per-SoC delay setup functions.

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

 arch/arm/mach-shmobile/board-lager-reference.c |    2 +-
 arch/arm/mach-shmobile/board-lager.c           |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

--- 0001/arch/arm/mach-shmobile/board-lager-reference.c
+++ work/arch/arm/mach-shmobile/board-lager-reference.c	2014-05-19 22:21:00.000000000 +0900
@@ -115,7 +115,7 @@ static const char *lager_boards_compat_d
 
 DT_MACHINE_START(LAGER_DT, "lager")
 	.smp		= smp_ops(r8a7790_smp_ops),
-	.init_early	= r8a7790_init_early,
+	.init_early	= shmobile_init_delay,
 	.init_time	= rcar_gen2_timer_init,
 	.init_machine	= lager_add_standard_devices,
 	.init_late	= shmobile_init_late,
--- 0001/arch/arm/mach-shmobile/board-lager.c
+++ work/arch/arm/mach-shmobile/board-lager.c	2014-05-19 22:21:20.000000000 +0900
@@ -886,7 +886,7 @@ static const char * const lager_boards_c
 
 DT_MACHINE_START(LAGER_DT, "lager")
 	.smp		= smp_ops(r8a7790_smp_ops),
-	.init_early	= r8a7790_init_early,
+	.init_early	= shmobile_init_delay,
 	.init_time	= rcar_gen2_timer_init,
 	.init_machine	= lager_init,
 	.init_late	= shmobile_init_late,

[PATCH 03/03] ARM: shmobile: Remove unused r8a7790_init_early()

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Remove the now unused r8a7790_init_early() function.

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

 arch/arm/mach-shmobile/include/mach/r8a7790.h |    1 -
 arch/arm/mach-shmobile/setup-r8a7790.c        |    7 -------
 2 files changed, 8 deletions(-)

--- 0001/arch/arm/mach-shmobile/include/mach/r8a7790.h
+++ work/arch/arm/mach-shmobile/include/mach/r8a7790.h	2014-05-19 22:26:16.000000000 +0900
@@ -33,7 +33,6 @@ void r8a7790_add_dt_devices(void);
 void r8a7790_clock_init(void);
 void r8a7790_pinmux_init(void);
 void r8a7790_pm_init(void);
-void r8a7790_init_early(void);
 extern struct smp_operations r8a7790_smp_ops;
 
 #endif /* __ASM_R8A7790_H__ */
--- 0003/arch/arm/mach-shmobile/setup-r8a7790.c
+++ work/arch/arm/mach-shmobile/setup-r8a7790.c	2014-05-19 22:25:49.000000000 +0900
@@ -307,13 +307,6 @@ void __init r8a7790_add_standard_devices
 	r8a7790_register_audio_dmac(1);
 }
 
-void __init r8a7790_init_early(void)
-{
-#ifndef CONFIG_ARM_ARCH_TIMER
-	shmobile_setup_delay(1300, 2, 4); /* Cortex-A15 @ 1300MHz */
-#endif
-}
-
 #ifdef CONFIG_USE_OF
 
 static const char * const r8a7790_boards_compat_dt[] __initconst = {

Re: [PATCH 00/03] ARM: shmobile: Use shmobile_init_delay() on r8a7790

[Geert Uytterhoeven]

On Tue, May 20, 2014 at 1:37 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
> [PATCH 01/03] ARM: shmobile: Use r8a7790 DT CPU Frequency in common case
> [PATCH 02/03] ARM: shmobile: Use r8a7790 DT CPU Frequency for Lager
> [PATCH 03/03] ARM: shmobile: Remove unused r8a7790_init_early()
>
> Convert r8a7790 to rely on shmobile_init_delay() instead of using a per-SoC
> delay setup function. Very similar to the r8a7791 implementation.
>
> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>

Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>

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 00/03] ARM: shmobile: Use shmobile_init_delay() on r8a7790

[Simon Horman]

On Tue, May 20, 2014 at 09:48:08AM +0200, Geert Uytterhoeven wrote:
> On Tue, May 20, 2014 at 1:37 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
> > [PATCH 01/03] ARM: shmobile: Use r8a7790 DT CPU Frequency in common case
> > [PATCH 02/03] ARM: shmobile: Use r8a7790 DT CPU Frequency for Lager
> > [PATCH 03/03] ARM: shmobile: Remove unused r8a7790_init_early()
> >
> > Convert r8a7790 to rely on shmobile_init_delay() instead of using a per-SoC
> > delay setup function. Very similar to the r8a7791 implementation.
> >
> > Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> 
> Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>

Thanks I will queue these up.