[PATCH 0/2] ARM: renesas: Enable SMP on R-Car E2

[PATCH 0/2] ARM: renesas: Enable SMP on R-Car E2

[Geert Uytterhoeven]

	Hi Magnus, Mar[ck], Sergei, Simon,

This patch series enables SMP on R-Car E2 (r8a7794).

  - The first patch initializes CNTVOFF for secondary CPU cores, like is
    already done for the boot CPU core.  Without this, the ARM arch
    timer doesn't work on secondary CPU cores.
  - The second patch adds the required infrastructure (APMU device node
    and corresponding enable-method) to DT.
    This must not be applied on a branch that doesn't have the first
    patch!

According to the original comments, the CNTVOFF initialization is needed
on Cortex-A7 only, hence affects R-Car E2, which has a dual Cortex-A7
cluster.
Apparently it's not needed on other R-Car Gen2 members that have (only)
Cortex-A15 clusters.
Perhaps it's also needed on R-Car H2, which has a big.LITTLE
quad Cortex-A15 + quad Cortex-A7 configuration, when running Linux on
the Cortex-A7 cores?  How can I test that?

Thanks for your comments!

Geert Uytterhoeven (1):
  ARM: shmobile: Add CA7 arch_timer initialization for secondary CPUs

Sergei Shtylyov (1):
  ARM: dts: r8a7794: Add SMP support

 arch/arm/boot/dts/r8a7794.dtsi        |  7 +++++++
 arch/arm/mach-shmobile/Makefile       |  1 +
 arch/arm/mach-shmobile/common.h       |  1 +
 arch/arm/mach-shmobile/headsmp-apmu.S | 38 +++++++++++++++++++++++++++++++++++
 arch/arm/mach-shmobile/platsmp-apmu.c |  2 +-
 5 files changed, 48 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/mach-shmobile/headsmp-apmu.S

-- 
2.7.4

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] ARM: shmobile: Add CA7 arch_timer initialization for secondary CPUs

[Geert Uytterhoeven]

On Cortex-A7, the arch timer CNTVOFF register is uninitialized.
Hence when enabling SMP on r8a7794, the kernel log is spammed with:

    WARNING: Underflow in clocksource 'arch_sys_counter' observed, time update ignored.
	     Please report this, consider using a different clocksource, if possible.
	     Your kernel is probably still fine.

To fix this, wrap the standard secondary_startup routine inside a
routine which initializes CNTVOFF when running on a Cortex-A7.
As the only possibilities are Cortex-A7 or Cortex-A15, checking the low
nibble of the Primary Part Number is sufficient.

The initialization is identical to what is already done for the boot CPU
since commit 9ce3fa6816c2fb59 ("ARM: shmobile: rcar-gen2: Add CA7
arch_timer initialization for r8a7794").

Based on patches by Hisashi Nakamura in the BSP.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 arch/arm/mach-shmobile/Makefile       |  1 +
 arch/arm/mach-shmobile/common.h       |  1 +
 arch/arm/mach-shmobile/headsmp-apmu.S | 38 +++++++++++++++++++++++++++++++++++
 arch/arm/mach-shmobile/platsmp-apmu.c |  2 +-
 4 files changed, 41 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/mach-shmobile/headsmp-apmu.S

diff --git a/arch/arm/mach-shmobile/Makefile b/arch/arm/mach-shmobile/Makefile
index 64611a1b4276517b..de735444e9f715fd 100644
--- a/arch/arm/mach-shmobile/Makefile
+++ b/arch/arm/mach-shmobile/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_ARCH_R8A7793)	+= regulator-quirk-rcar-gen2.o
 
 # SMP objects
 smp-y				:= $(cpu-y)
+smp-$(CONFIG_ARCH_RCAR_GEN2)	+= headsmp-apmu.o
 smp-$(CONFIG_ARCH_SH73A0)	+= smp-sh73a0.o headsmp-scu.o platsmp-scu.o
 smp-$(CONFIG_ARCH_R8A7779)	+= smp-r8a7779.o headsmp-scu.o platsmp-scu.o
 smp-$(CONFIG_ARCH_R8A7790)	+= smp-r8a7790.o
diff --git a/arch/arm/mach-shmobile/common.h b/arch/arm/mach-shmobile/common.h
index 1a8f7b3ab449db56..6792a07bce761aab 100644
--- a/arch/arm/mach-shmobile/common.h
+++ b/arch/arm/mach-shmobile/common.h
@@ -11,6 +11,7 @@ extern void shmobile_smp_hook(unsigned int cpu, unsigned long fn,
 			      unsigned long arg);
 extern bool shmobile_smp_cpu_can_disable(unsigned int cpu);
 extern bool shmobile_smp_init_fallback_ops(void);
+extern void shmobile_boot_apmu(void);
 extern void shmobile_boot_scu(void);
 extern void shmobile_smp_scu_prepare_cpus(phys_addr_t scu_base_phys,
 					  unsigned int max_cpus);
diff --git a/arch/arm/mach-shmobile/headsmp-apmu.S b/arch/arm/mach-shmobile/headsmp-apmu.S
new file mode 100644
index 0000000000000000..52516d81ce98384c
--- /dev/null
+++ b/arch/arm/mach-shmobile/headsmp-apmu.S
@@ -0,0 +1,38 @@
+/*
+ * SMP support for APMU based systems
+ *
+ * Copyright (C) 2014  Renesas Electronics Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/linkage.h>
+
+ENTRY(shmobile_boot_apmu)
+	mrc	p15, 0, r0, c0, c0, 0		/* Get Main ID */
+	ubfx	r1, r0, #4, #4			/* r1=Lo 4bit of Primary Part */
+	cmp	r1, #0x7			/* 0x7 = CA7, 0xf = CA15 */
+	bne	1f
+	/*
+	 * CA7 setup
+	 * CNTVOFF has to be initialized either from non-secure Hypervisor
+	 * mode or secure Monitor mode with SCR.NS==1. If TrustZone is enabled
+	 * then it should be handled by the secure code
+	 */
+	cps	0x16
+	mrc	p15, 0, r1, c1, c1, 0		/* get Secure Config */
+	orr	r0, r1, #1
+	mcr	p15, 0, r0, c1, c1, 0		/* Set Non Secure bit */
+	isb
+	mov	r0, #0
+	mcrr	p15, 4, r0, r0, c14		/* CNTVOFF = 0 */
+	isb
+	mcr	p15, 0, r1, c1, c1, 0		/* Set Secure bit */
+	isb
+	cps	0x13
+1:
+
+	b	secondary_startup
+ENDPROC(shmobile_boot_apmu)
diff --git a/arch/arm/mach-shmobile/platsmp-apmu.c b/arch/arm/mach-shmobile/platsmp-apmu.c
index 3ca2c13346f0cbc3..4422b615a6ee6045 100644
--- a/arch/arm/mach-shmobile/platsmp-apmu.c
+++ b/arch/arm/mach-shmobile/platsmp-apmu.c
@@ -204,7 +204,7 @@ void __init shmobile_smp_apmu_prepare_cpus(unsigned int max_cpus,
 int shmobile_smp_apmu_boot_secondary(unsigned int cpu, struct task_struct *idle)
 {
 	/* For this particular CPU register boot vector */
-	shmobile_smp_hook(cpu, __pa_symbol(secondary_startup), 0);
+	shmobile_smp_hook(cpu, __pa_symbol(shmobile_boot_apmu), 0);
 
 	return apmu_wrap(cpu, apmu_power_on);
 }
-- 
2.7.4

[PATCH 2/2] ARM: dts: r8a7794: Add SMP support

[Geert Uytterhoeven]

From: Sergei Shtylyov <sergei.shtylyov-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org>

Add the device tree node for the Advanced Power Management Unit (APMU).
Use the "enable-method" prop to  point out that the APMU should be used
for the SMP support.

Signed-off-by: Sergei Shtylyov <sergei.shtylyov-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org>
Signed-off-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
---
 arch/arm/boot/dts/r8a7794.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/r8a7794.dtsi b/arch/arm/boot/dts/r8a7794.dtsi
index f32b458e93285adc..8afd04339c84c65e 100644
--- a/arch/arm/boot/dts/r8a7794.dtsi
+++ b/arch/arm/boot/dts/r8a7794.dtsi
@@ -37,6 +37,7 @@
 	cpus {
 		#address-cells = <1>;
 		#size-cells = <0>;
+		enable-method = "renesas,apmu";
 
 		cpu0: cpu@0 {
 			device_type = "cpu";
@@ -65,6 +66,12 @@
 		};
 	};
 
+	apmu@e6151000 {
+		compatible = "renesas,r8a7794-apmu", "renesas,apmu";
+		reg = <0 0xe6151000 0 0x188>;
+		cpus = <&cpu0 &cpu1>;
+	};
+
 	gic: interrupt-controller@f1001000 {
 		compatible = "arm,gic-400";
 		#interrupt-cells = <3>;
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ARM: shmobile: Add CA7 arch_timer initialization for secondary CPUs

[Marc Zyngier]

Hi Geert,

On 04/07/17 18:02, Geert Uytterhoeven wrote:
> On Cortex-A7, the arch timer CNTVOFF register is uninitialized.
> Hence when enabling SMP on r8a7794, the kernel log is spammed with:
> 
>     WARNING: Underflow in clocksource 'arch_sys_counter' observed, time update ignored.
> 	     Please report this, consider using a different clocksource, if possible.
> 	     Your kernel is probably still fine.
> 
> To fix this, wrap the standard secondary_startup routine inside a
> routine which initializes CNTVOFF when running on a Cortex-A7.
> As the only possibilities are Cortex-A7 or Cortex-A15, checking the low
> nibble of the Primary Part Number is sufficient.
> 
> The initialization is identical to what is already done for the boot CPU
> since commit 9ce3fa6816c2fb59 ("ARM: shmobile: rcar-gen2: Add CA7
> arch_timer initialization for r8a7794").

Humfff... Pretty horrible. Comments below.

> 
> Based on patches by Hisashi Nakamura in the BSP.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>
> ---
>  arch/arm/mach-shmobile/Makefile       |  1 +
>  arch/arm/mach-shmobile/common.h       |  1 +
>  arch/arm/mach-shmobile/headsmp-apmu.S | 38 +++++++++++++++++++++++++++++++++++
>  arch/arm/mach-shmobile/platsmp-apmu.c |  2 +-
>  4 files changed, 41 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm/mach-shmobile/headsmp-apmu.S
> 
> diff --git a/arch/arm/mach-shmobile/Makefile b/arch/arm/mach-shmobile/Makefile
> index 64611a1b4276517b..de735444e9f715fd 100644
> --- a/arch/arm/mach-shmobile/Makefile
> +++ b/arch/arm/mach-shmobile/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_ARCH_R8A7793)	+= regulator-quirk-rcar-gen2.o
>  
>  # SMP objects
>  smp-y				:= $(cpu-y)
> +smp-$(CONFIG_ARCH_RCAR_GEN2)	+= headsmp-apmu.o
>  smp-$(CONFIG_ARCH_SH73A0)	+= smp-sh73a0.o headsmp-scu.o platsmp-scu.o
>  smp-$(CONFIG_ARCH_R8A7779)	+= smp-r8a7779.o headsmp-scu.o platsmp-scu.o
>  smp-$(CONFIG_ARCH_R8A7790)	+= smp-r8a7790.o
> diff --git a/arch/arm/mach-shmobile/common.h b/arch/arm/mach-shmobile/common.h
> index 1a8f7b3ab449db56..6792a07bce761aab 100644
> --- a/arch/arm/mach-shmobile/common.h
> +++ b/arch/arm/mach-shmobile/common.h
> @@ -11,6 +11,7 @@ extern void shmobile_smp_hook(unsigned int cpu, unsigned long fn,
>  			      unsigned long arg);
>  extern bool shmobile_smp_cpu_can_disable(unsigned int cpu);
>  extern bool shmobile_smp_init_fallback_ops(void);
> +extern void shmobile_boot_apmu(void);
>  extern void shmobile_boot_scu(void);
>  extern void shmobile_smp_scu_prepare_cpus(phys_addr_t scu_base_phys,
>  					  unsigned int max_cpus);
> diff --git a/arch/arm/mach-shmobile/headsmp-apmu.S b/arch/arm/mach-shmobile/headsmp-apmu.S
> new file mode 100644
> index 0000000000000000..52516d81ce98384c
> --- /dev/null
> +++ b/arch/arm/mach-shmobile/headsmp-apmu.S
> @@ -0,0 +1,38 @@
> +/*
> + * SMP support for APMU based systems
> + *
> + * Copyright (C) 2014  Renesas Electronics Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/linkage.h>
> +
> +ENTRY(shmobile_boot_apmu)
> +	mrc	p15, 0, r0, c0, c0, 0		/* Get Main ID */
> +	ubfx	r1, r0, #4, #4			/* r1=Lo 4bit of Primary Part */
> +	cmp	r1, #0x7			/* 0x7 = CA7, 0xf = CA15 */
> +	bne	1f

Why don't you deal with the A15 parts as well? And TBH, you'd be better
off checking ID_PFR1 for both Generic Timers and Virtualization Extensions.

> +	/*
> +	 * CA7 setup
> +	 * CNTVOFF has to be initialized either from non-secure Hypervisor
> +	 * mode or secure Monitor mode with SCR.NS==1. If TrustZone is enabled
> +	 * then it should be handled by the secure code
> +	 */
> +	cps	0x16

It'd be worth adding a MONITOR_MODE macro instead of this raw value.

> +	mrc	p15, 0, r1, c1, c1, 0		/* get Secure Config */
> +	orr	r0, r1, #1
> +	mcr	p15, 0, r0, c1, c1, 0		/* Set Non Secure bit */
> +	isb
> +	mov	r0, #0
> +	mcrr	p15, 4, r0, r0, c14		/* CNTVOFF = 0 */
> +	isb
> +	mcr	p15, 0, r1, c1, c1, 0		/* Set Secure bit */
> +	isb
> +	cps	0x13

and use SVC_MODE here.

> +1:
> +
> +	b	secondary_startup
> +ENDPROC(shmobile_boot_apmu)
> diff --git a/arch/arm/mach-shmobile/platsmp-apmu.c b/arch/arm/mach-shmobile/platsmp-apmu.c
> index 3ca2c13346f0cbc3..4422b615a6ee6045 100644
> --- a/arch/arm/mach-shmobile/platsmp-apmu.c
> +++ b/arch/arm/mach-shmobile/platsmp-apmu.c
> @@ -204,7 +204,7 @@ void __init shmobile_smp_apmu_prepare_cpus(unsigned int max_cpus,
>  int shmobile_smp_apmu_boot_secondary(unsigned int cpu, struct task_struct *idle)
>  {
>  	/* For this particular CPU register boot vector */
> -	shmobile_smp_hook(cpu, __pa_symbol(secondary_startup), 0);
> +	shmobile_smp_hook(cpu, __pa_symbol(shmobile_boot_apmu), 0);
>  
>  	return apmu_wrap(cpu, apmu_power_on);
>  }
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ARM: shmobile: Add CA7 arch_timer initialization for secondary CPUs

[Geert Uytterhoeven]

Hi Marc,

On Tue, Jul 4, 2017 at 7:32 PM, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
> On 04/07/17 18:02, Geert Uytterhoeven wrote:
>> On Cortex-A7, the arch timer CNTVOFF register is uninitialized.
>> Hence when enabling SMP on r8a7794, the kernel log is spammed with:
>>
>>     WARNING: Underflow in clocksource 'arch_sys_counter' observed, time update ignored.
>>            Please report this, consider using a different clocksource, if possible.
>>            Your kernel is probably still fine.
>>
>> To fix this, wrap the standard secondary_startup routine inside a
>> routine which initializes CNTVOFF when running on a Cortex-A7.
>> As the only possibilities are Cortex-A7 or Cortex-A15, checking the low
>> nibble of the Primary Part Number is sufficient.
>>
>> The initialization is identical to what is already done for the boot CPU
>> since commit 9ce3fa6816c2fb59 ("ARM: shmobile: rcar-gen2: Add CA7
>> arch_timer initialization for r8a7794").
>
> Humfff... Pretty horrible. Comments below.

I know.  I suppose this should be done by the firmware/boot loader?
But we have to live with firmware/boot loaders in the wild...

>> --- /dev/null
>> +++ b/arch/arm/mach-shmobile/headsmp-apmu.S
>> @@ -0,0 +1,38 @@
>> +/*
>> + * SMP support for APMU based systems
>> + *
>> + * Copyright (C) 2014  Renesas Electronics Corporation
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include <linux/linkage.h>
>> +
>> +ENTRY(shmobile_boot_apmu)
>> +     mrc     p15, 0, r0, c0, c0, 0           /* Get Main ID */
>> +     ubfx    r1, r0, #4, #4                  /* r1=Lo 4bit of Primary Part */
>> +     cmp     r1, #0x7                        /* 0x7 = CA7, 0xf = CA15 */
>> +     bne     1f
>
> Why don't you deal with the A15 parts as well? And TBH, you'd be better
> off checking ID_PFR1 for both Generic Timers and Virtualization Extensions.

Do we have to deal with the A15 parts here?
We're not having issues with the A15 parts, so that's why I left it out.

I'm trying to understand what's different between A15 and A7, and why A7
needs this code, while A15 apparently doesn't.
I'm not seeing any obvious differences in the U-Boot sources handling
e.g. r8a7791/koelsch (dual A15) vs. r8a7794/alt (dual A7).

> off checking ID_PFR1 for both Generic Timers and Virtualization Extensions.

Thank, I'll have a look at that...

>> +     /*
>> +      * CA7 setup
>> +      * CNTVOFF has to be initialized either from non-secure Hypervisor
>> +      * mode or secure Monitor mode with SCR.NS==1. If TrustZone is enabled
>> +      * then it should be handled by the secure code
>> +      */
>> +     cps     0x16
>
> It'd be worth adding a MONITOR_MODE macro instead of this raw value.

>> +     cps     0x13
>
> and use SVC_MODE here.

Thanks, I'll have a look at those, too.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ARM: shmobile: Add CA7 arch_timer initialization for secondary CPUs

[Marc Zyngier]

On 04/07/17 19:20, Geert Uytterhoeven wrote:
> Hi Marc,
> 
> On Tue, Jul 4, 2017 at 7:32 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> On 04/07/17 18:02, Geert Uytterhoeven wrote:
>>> On Cortex-A7, the arch timer CNTVOFF register is uninitialized.
>>> Hence when enabling SMP on r8a7794, the kernel log is spammed with:
>>>
>>>     WARNING: Underflow in clocksource 'arch_sys_counter' observed, time update ignored.
>>>            Please report this, consider using a different clocksource, if possible.
>>>            Your kernel is probably still fine.
>>>
>>> To fix this, wrap the standard secondary_startup routine inside a
>>> routine which initializes CNTVOFF when running on a Cortex-A7.
>>> As the only possibilities are Cortex-A7 or Cortex-A15, checking the low
>>> nibble of the Primary Part Number is sufficient.
>>>
>>> The initialization is identical to what is already done for the boot CPU
>>> since commit 9ce3fa6816c2fb59 ("ARM: shmobile: rcar-gen2: Add CA7
>>> arch_timer initialization for r8a7794").
>>
>> Humfff... Pretty horrible. Comments below.
> 
> I know.  I suppose this should be done by the firmware/boot loader?

That's what is normally expected.

> But we have to live with firmware/boot loaders in the wild...

Yeah, I can tell. It is a shame that firmware people didn't realize that
just because the old firmware seems to work on a new revision of the
architecture, it doesn't mean it does everything right for that
particular revision... Oh well.

>>> --- /dev/null
>>> +++ b/arch/arm/mach-shmobile/headsmp-apmu.S
>>> @@ -0,0 +1,38 @@
>>> +/*
>>> + * SMP support for APMU based systems
>>> + *
>>> + * Copyright (C) 2014  Renesas Electronics Corporation
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + */
>>> +
>>> +#include <linux/linkage.h>
>>> +
>>> +ENTRY(shmobile_boot_apmu)
>>> +     mrc     p15, 0, r0, c0, c0, 0           /* Get Main ID */
>>> +     ubfx    r1, r0, #4, #4                  /* r1=Lo 4bit of Primary Part */
>>> +     cmp     r1, #0x7                        /* 0x7 = CA7, 0xf = CA15 */
>>> +     bne     1f
>>
>> Why don't you deal with the A15 parts as well? And TBH, you'd be better
>> off checking ID_PFR1 for both Generic Timers and Virtualization Extensions.
> 
> Do we have to deal with the A15 parts here?
> We're not having issues with the A15 parts, so that's why I left it out.

Well, A15 has the exact same features as A7 when it comes to the timer,
and CNTVOFF definitely resets as UNKNOWN.

> 
> I'm trying to understand what's different between A15 and A7, and why A7
> needs this code, while A15 apparently doesn't.
> I'm not seeing any obvious differences in the U-Boot sources handling
> e.g. r8a7791/koelsch (dual A15) vs. r8a7794/alt (dual A7).

The UNKNOWN above might well be 0 on A15, but I wouldn't bet on it. If
nothing sets CNTVOFF on these systems, consider yourself lucky!

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/2] ARM: shmobile: Add CA7 arch_timer initialization for secondary CPUs

[Geert Uytterhoeven]

Hi Marc,

On Wed, Jul 5, 2017 at 12:07 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 04/07/17 19:20, Geert Uytterhoeven wrote:
>> On Tue, Jul 4, 2017 at 7:32 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> On 04/07/17 18:02, Geert Uytterhoeven wrote:
>>>> On Cortex-A7, the arch timer CNTVOFF register is uninitialized.
>>>> Hence when enabling SMP on r8a7794, the kernel log is spammed with:
>>>>
>>>>     WARNING: Underflow in clocksource 'arch_sys_counter' observed, time update ignored.
>>>>            Please report this, consider using a different clocksource, if possible.
>>>>            Your kernel is probably still fine.
>>>>
>>>> To fix this, wrap the standard secondary_startup routine inside a
>>>> routine which initializes CNTVOFF when running on a Cortex-A7.
>>>> As the only possibilities are Cortex-A7 or Cortex-A15, checking the low
>>>> nibble of the Primary Part Number is sufficient.
>>>>
>>>> The initialization is identical to what is already done for the boot CPU
>>>> since commit 9ce3fa6816c2fb59 ("ARM: shmobile: rcar-gen2: Add CA7
>>>> arch_timer initialization for r8a7794").
>>>
>>> Humfff... Pretty horrible. Comments below.
>>
>> I know.  I suppose this should be done by the firmware/boot loader?
>
> That's what is normally expected.
>
>> But we have to live with firmware/boot loaders in the wild...
>
> Yeah, I can tell. It is a shame that firmware people didn't realize that
> just because the old firmware seems to work on a new revision of the
> architecture, it doesn't mean it does everything right for that
> particular revision... Oh well.
>
>>>> --- /dev/null
>>>> +++ b/arch/arm/mach-shmobile/headsmp-apmu.S
>>>> @@ -0,0 +1,38 @@
>>>> +/*
>>>> + * SMP support for APMU based systems
>>>> + *
>>>> + * Copyright (C) 2014  Renesas Electronics Corporation
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify
>>>> + * it under the terms of the GNU General Public License version 2 as
>>>> + * published by the Free Software Foundation.
>>>> + */
>>>> +
>>>> +#include <linux/linkage.h>
>>>> +
>>>> +ENTRY(shmobile_boot_apmu)
>>>> +     mrc     p15, 0, r0, c0, c0, 0           /* Get Main ID */
>>>> +     ubfx    r1, r0, #4, #4                  /* r1=Lo 4bit of Primary Part */
>>>> +     cmp     r1, #0x7                        /* 0x7 = CA7, 0xf = CA15 */
>>>> +     bne     1f
>>>
>>> Why don't you deal with the A15 parts as well? And TBH, you'd be better
>>> off checking ID_PFR1 for both Generic Timers and Virtualization Extensions.
>>
>> Do we have to deal with the A15 parts here?
>> We're not having issues with the A15 parts, so that's why I left it out.
>
> Well, A15 has the exact same features as A7 when it comes to the timer,
> and CNTVOFF definitely resets as UNKNOWN.

IC.

>> I'm trying to understand what's different between A15 and A7, and why A7
>> needs this code, while A15 apparently doesn't.
>> I'm not seeing any obvious differences in the U-Boot sources handling
>> e.g. r8a7791/koelsch (dual A15) vs. r8a7794/alt (dual A7).
>
> The UNKNOWN above might well be 0 on A15, but I wouldn't bet on it. If
> nothing sets CNTVOFF on these systems, consider yourself lucky!

OK.  That means I can just drop the Primary Part check, as this code path
is exercised only on systems with Cortex-A15 and/or A7 CPU cores.

Thanks!

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