ARM: shmobile: Use shmobile_init_delay() on r8a7791

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.

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 <[email protected]>

---

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

Hi Magnus,

Quoted 8 lines · 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.

Quoted 10 lines

Signed-off-by Magnus Damm <[email protected]>

---

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

Signature · 3 lines

Regards,

Laurent Pinchart

Quoted 12 lines · Laurent Pinchart

Hi Magnus,

Quoted 8 lines · 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.

Quoted 7 lines

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

From: Simon Horman <[email protected]>

[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 <[email protected]>

arch/arm/mach-shmobile/timer.c +3 -3

index ccecde9..8d80a33 100644
@@ -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

arch/arm/mach-shmobile/timer.c +0 -0

 }
 
 void __init shmobile_init_delay(void)

@@ -58,12 +61,12 @@ void __init shmobile_init_delay(void)
 
        if (max_freq) {

Hi Simon,

Quoted 10 lines · Simon Horman

From: Simon Horman <[email protected]>

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

Quoted 28 lines

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

Signed-off-by Simon Horman <[email protected]>

arch/arm/mach-shmobile/timer.c +3 -5

index ccecde9..8d80a33 100644
@@ -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);

arch/arm/mach-shmobile/timer.c line 37

Quoted 4 lines

arch/arm/mach-shmobile/timer.c +1 -2

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

arch/arm/mach-shmobile/timer.c line 43

Quoted 7 lines

arch/arm/mach-shmobile/timer.c +6 -0

+}
+
+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);

I wonder if the following is any good.

From: Simon Horman <[email protected]>

[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 <[email protected]>

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

Quoted 18 lines · Simon Horman

I wonder if the following is any good.

From: Simon Horman <[email protected]>

[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 <[email protected]>

Quoted 8 lines

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

Hi Magnus,

Quoted 8 lines · 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.