[PATCH v3] ARM: imx: replicate the diagnostic register of boot cpu into secondary cores

[PATCH v3] ARM: imx: replicate the diagnostic register of boot cpu into secondary cores

[Shawn Guo]

The diagnostic register holds the errata bits.  Mostly bootloader
does not bring up secondary cores, so that when errata bits are set
in bootloader, they are set only for boot cpu.  But on a SMP
configuration, it should be equally done on every single core.
Set up the diagnostic register for secondary cores by replicating
the register from boot cpu.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
Changes since v2:
 * Setting secondary cores' diagnostic register before MMU is enabled.

 arch/arm/mach-imx/headsmp.S |   12 ++++++++++++
 arch/arm/mach-imx/platsmp.c |   14 ++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/arch/arm/mach-imx/headsmp.S b/arch/arm/mach-imx/headsmp.S
index a58c8b0..cd87970 100644
--- a/arch/arm/mach-imx/headsmp.S
+++ b/arch/arm/mach-imx/headsmp.S
@@ -18,8 +18,20 @@
 	.section ".text.head", "ax"
 
 #ifdef CONFIG_SMP
+diag_reg_offset:
+	.word	g_diag_reg - .
+
+	.macro	set_diag_reg
+	adr	r0, diag_reg_offset
+	ldr	r1, [r0]
+	add	r1, r1, r0		@ r1 = physical &g_diag_reg
+	ldr	r0, [r1]
+	mcr	p15, 0, r0, c15, c0, 1	@ write diagnostic register
+	.endm
+
 ENTRY(v7_secondary_startup)
 	bl	v7_invalidate_l1
+	set_diag_reg
 	b	secondary_startup
 ENDPROC(v7_secondary_startup)
 #endif
diff --git a/arch/arm/mach-imx/platsmp.c b/arch/arm/mach-imx/platsmp.c
index 7061bde..abe9d95 100644
--- a/arch/arm/mach-imx/platsmp.c
+++ b/arch/arm/mach-imx/platsmp.c
@@ -13,6 +13,7 @@
 #include <linux/init.h>
 #include <linux/smp.h>
 #include <linux/irqchip/arm-gic.h>
+#include <asm/cacheflush.h>
 #include <asm/page.h>
 #include <asm/smp_scu.h>
 #include <asm/mach/map.h>
@@ -22,6 +23,7 @@
 
 #define SCU_STANDBY_ENABLE	(1 << 5)
 
+u32 g_diag_reg;
 static void __iomem *scu_base;
 
 static struct map_desc scu_io_desc __initdata = {
@@ -91,6 +93,18 @@ void imx_smp_prepare(void)
 static void __init imx_smp_prepare_cpus(unsigned int max_cpus)
 {
 	imx_smp_prepare();
+
+	/*
+	 * The diagnostic register holds the errata bits.  Mostly bootloader
+	 * does not bring up secondary cores, so that when errata bits are set
+	 * in bootloader, they are set only for boot cpu.  But on a SMP
+	 * configuration, it should be equally done on every single core.
+	 * Read the register from boot cpu here, and will replicate it into
+	 * secondary cores when booting them.
+	 */
+	asm("mrc p15, 0, %0, c15, c0, 1" : "=r" (g_diag_reg) : : "cc");
+	__cpuc_flush_dcache_area(&g_diag_reg, sizeof(g_diag_reg));
+	outer_clean_range(__pa(&g_diag_reg), __pa(&g_diag_reg + 1));
 }
 
 struct smp_operations  imx_smp_ops __initdata = {
-- 
1.7.9.5

[PATCH v3] ARM: imx: replicate the diagnostic register of boot cpu into secondary cores

[Will Deacon]

Hi Shawn,

Thanks for respinning this without the MMU enabled.

On Fri, Apr 26, 2013 at 04:57:25AM +0100, Shawn Guo wrote:
> The diagnostic register holds the errata bits.  Mostly bootloader
> does not bring up secondary cores, so that when errata bits are set
> in bootloader, they are set only for boot cpu.  But on a SMP
> configuration, it should be equally done on every single core.
> Set up the diagnostic register for secondary cores by replicating
> the register from boot cpu.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
> Changes since v2:
>  * Setting secondary cores' diagnostic register before MMU is enabled.
> 
>  arch/arm/mach-imx/headsmp.S |   12 ++++++++++++
>  arch/arm/mach-imx/platsmp.c |   14 ++++++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/arch/arm/mach-imx/headsmp.S b/arch/arm/mach-imx/headsmp.S
> index a58c8b0..cd87970 100644
> --- a/arch/arm/mach-imx/headsmp.S
> +++ b/arch/arm/mach-imx/headsmp.S
> @@ -18,8 +18,20 @@
>  	.section ".text.head", "ax"
>  
>  #ifdef CONFIG_SMP
> +diag_reg_offset:
> +	.word	g_diag_reg - .
> +
> +	.macro	set_diag_reg
> +	adr	r0, diag_reg_offset
> +	ldr	r1, [r0]
> +	add	r1, r1, r0		@ r1 = physical &g_diag_reg
> +	ldr	r0, [r1]
> +	mcr	p15, 0, r0, c15, c0, 1	@ write diagnostic register
> +	.endm

Why not re-read the register directly, rather than mrc on the current core?
I feel uneasy about blindly copying one CPU's diagnostic bits into another.
It would also make the code simpler.

Will

[PATCH v3] ARM: imx: replicate the diagnostic register of boot cpu into secondary cores

[Shawn Guo]

On Fri, Apr 26, 2013 at 02:21:30PM +0100, Will Deacon wrote:
> > diff --git a/arch/arm/mach-imx/headsmp.S b/arch/arm/mach-imx/headsmp.S
> > index a58c8b0..cd87970 100644
> > --- a/arch/arm/mach-imx/headsmp.S
> > +++ b/arch/arm/mach-imx/headsmp.S
> > @@ -18,8 +18,20 @@
> >  	.section ".text.head", "ax"
> >  
> >  #ifdef CONFIG_SMP
> > +diag_reg_offset:
> > +	.word	g_diag_reg - .
> > +
> > +	.macro	set_diag_reg
> > +	adr	r0, diag_reg_offset
> > +	ldr	r1, [r0]
> > +	add	r1, r1, r0		@ r1 = physical &g_diag_reg
> > +	ldr	r0, [r1]
> > +	mcr	p15, 0, r0, c15, c0, 1	@ write diagnostic register
> > +	.endm
> 
> Why not re-read the register directly, rather than mrc on the current core?

I do not understand the comment.  What do you mean by "re-read"?  Read
the register on secondary cores will only return us 0, as bootloader
only sets up the register on cpu0.  The macro set_diag_reg only runs on
secondary cores to copy cpu0's diagnostic bits.  I think it's safe on
SMP.

Shawn

> I feel uneasy about blindly copying one CPU's diagnostic bits into another.
> It would also make the code simpler.

[PATCH v3] ARM: imx: replicate the diagnostic register of boot cpu into secondary cores

[Dirk Behme]

Am 26.04.2013 15:47, schrieb Shawn Guo:
> On Fri, Apr 26, 2013 at 02:21:30PM +0100, Will Deacon wrote:
>>> diff --git a/arch/arm/mach-imx/headsmp.S b/arch/arm/mach-imx/headsmp.S
>>> index a58c8b0..cd87970 100644
>>> --- a/arch/arm/mach-imx/headsmp.S
>>> +++ b/arch/arm/mach-imx/headsmp.S
>>> @@ -18,8 +18,20 @@
>>>   	.section ".text.head", "ax"
>>>
>>>   #ifdef CONFIG_SMP
>>> +diag_reg_offset:
>>> +	.word	g_diag_reg - .
>>> +
>>> +	.macro	set_diag_reg
>>> +	adr	r0, diag_reg_offset
>>> +	ldr	r1, [r0]
>>> +	add	r1, r1, r0		@ r1 = physical &g_diag_reg
>>> +	ldr	r0, [r1]
>>> +	mcr	p15, 0, r0, c15, c0, 1	@ write diagnostic register
>>> +	.endm
>>
>> Why not re-read the register directly, rather than mrc on the current core?
>
> I do not understand the comment.  What do you mean by "re-read"?  Read
> the register on secondary cores will only return us 0, as bootloader
> only sets up the register on cpu0.  The macro set_diag_reg only runs on
> secondary cores to copy cpu0's diagnostic bits.  I think it's safe on
> SMP.

Any news on this?

Thanks

Dirk

[PATCH v3] ARM: imx: replicate the diagnostic register of boot cpu into secondary cores

[Will Deacon]

On Wed, May 01, 2013 at 04:32:14PM +0100, Dirk Behme wrote:
> Am 26.04.2013 15:47, schrieb Shawn Guo:
> > On Fri, Apr 26, 2013 at 02:21:30PM +0100, Will Deacon wrote:
> >>> diff --git a/arch/arm/mach-imx/headsmp.S b/arch/arm/mach-imx/headsmp.S
> >>> index a58c8b0..cd87970 100644
> >>> --- a/arch/arm/mach-imx/headsmp.S
> >>> +++ b/arch/arm/mach-imx/headsmp.S
> >>> @@ -18,8 +18,20 @@
> >>>   	.section ".text.head", "ax"
> >>>
> >>>   #ifdef CONFIG_SMP
> >>> +diag_reg_offset:
> >>> +	.word	g_diag_reg - .
> >>> +
> >>> +	.macro	set_diag_reg
> >>> +	adr	r0, diag_reg_offset
> >>> +	ldr	r1, [r0]
> >>> +	add	r1, r1, r0		@ r1 = physical &g_diag_reg
> >>> +	ldr	r0, [r1]
> >>> +	mcr	p15, 0, r0, c15, c0, 1	@ write diagnostic register
> >>> +	.endm
> >>
> >> Why not re-read the register directly, rather than mrc on the current core?
> >
> > I do not understand the comment.  What do you mean by "re-read"?  Read
> > the register on secondary cores will only return us 0, as bootloader
> > only sets up the register on cpu0.  The macro set_diag_reg only runs on
> > secondary cores to copy cpu0's diagnostic bits.  I think it's safe on
> > SMP.
> 
> Any news on this?

Well, I don't like copying the register value over from CPU0. I'd rather we
orr'd in the bits we actually need, like we would in __v7_setup. Perhaps the
code there should be factored out into arm_errata_xxxx asm macros, so
platforms can invoke them if they need to without breaking non-secure boot
for multi-platform kernels.

Will

[PATCH v3] ARM: imx: replicate the diagnostic register of boot cpu into secondary cores

[Dirk Behme]

Am 01.05.2013 17:38, schrieb Will Deacon:
> On Wed, May 01, 2013 at 04:32:14PM +0100, Dirk Behme wrote:
>> Am 26.04.2013 15:47, schrieb Shawn Guo:
>>> On Fri, Apr 26, 2013 at 02:21:30PM +0100, Will Deacon wrote:
>>>>> diff --git a/arch/arm/mach-imx/headsmp.S b/arch/arm/mach-imx/headsmp.S
>>>>> index a58c8b0..cd87970 100644
>>>>> --- a/arch/arm/mach-imx/headsmp.S
>>>>> +++ b/arch/arm/mach-imx/headsmp.S
>>>>> @@ -18,8 +18,20 @@
>>>>>    	.section ".text.head", "ax"
>>>>>
>>>>>    #ifdef CONFIG_SMP
>>>>> +diag_reg_offset:
>>>>> +	.word	g_diag_reg - .
>>>>> +
>>>>> +	.macro	set_diag_reg
>>>>> +	adr	r0, diag_reg_offset
>>>>> +	ldr	r1, [r0]
>>>>> +	add	r1, r1, r0		@ r1 = physical &g_diag_reg
>>>>> +	ldr	r0, [r1]
>>>>> +	mcr	p15, 0, r0, c15, c0, 1	@ write diagnostic register
>>>>> +	.endm
>>>>
>>>> Why not re-read the register directly, rather than mrc on the current core?
>>>
>>> I do not understand the comment.  What do you mean by "re-read"?  Read
>>> the register on secondary cores will only return us 0, as bootloader
>>> only sets up the register on cpu0.  The macro set_diag_reg only runs on
>>> secondary cores to copy cpu0's diagnostic bits.  I think it's safe on
>>> SMP.
>>
>> Any news on this?
>
> Well, I don't like copying the register value over from CPU0.

Just for a better understanding: What's wrong with using the same 
register values on all CPUx? I.e. with copying them from CPU0? 
Shouldn't all CPUx use the same setting? Or at least the same errata 
bits enabled?

> I'd rather we
> orr'd in the bits we actually need, like we would in __v7_setup. Perhaps the
> code there should be factored out into arm_errata_xxxx asm macros,

I'm not as much in the details as Shawn and you, but it's my 
understanding that we moved the errata handling recently from the 
kernel to the boot loader? After this, we now find that the boot 
loader does the errata handling only for CPU0, and now we start to 
discuss to move it back to the kernel?

Sorry if my rough understanding is wrong ;)

Thanks for your help and best regards

Dirk

[PATCH v3] ARM: imx: replicate the diagnostic register of boot cpu into secondary cores

[Will Deacon]

On Wed, May 01, 2013 at 05:19:24PM +0100, Dirk Behme wrote:
> Am 01.05.2013 17:38, schrieb Will Deacon:
> > On Wed, May 01, 2013 at 04:32:14PM +0100, Dirk Behme wrote:
> Just for a better understanding: What's wrong with using the same 
> register values on all CPUx? I.e. with copying them from CPU0? 
> Shouldn't all CPUx use the same setting? Or at least the same errata 
> bits enabled?

There's this thing called big.LITTLE... In this case, we're all A9s, but it
still feels horrible.

> > I'd rather we
> > orr'd in the bits we actually need, like we would in __v7_setup. Perhaps the
> > code there should be factored out into arm_errata_xxxx asm macros,
> 
> I'm not as much in the details as Shawn and you, but it's my 
> understanding that we moved the errata handling recently from the 
> kernel to the boot loader? After this, we now find that the boot 
> loader does the errata handling only for CPU0, and now we start to 
> discuss to move it back to the kernel?

In Shawn's case, I don't think the secondary CPUs execute any code prior to
kernel text, but he can correct me if I'm wrong.

Will

[PATCH v3] ARM: imx: replicate the diagnostic register of boot cpu into secondary cores

[Shawn Guo]

On Wed, May 01, 2013 at 05:38:17PM +0100, Will Deacon wrote:
> On Wed, May 01, 2013 at 05:19:24PM +0100, Dirk Behme wrote:
> > Am 01.05.2013 17:38, schrieb Will Deacon:
> > > On Wed, May 01, 2013 at 04:32:14PM +0100, Dirk Behme wrote:
> > Just for a better understanding: What's wrong with using the same 
> > register values on all CPUx? I.e. with copying them from CPU0? 
> > Shouldn't all CPUx use the same setting? Or at least the same errata 
> > bits enabled?
> 
> There's this thing called big.LITTLE... In this case, we're all A9s, but it
> still feels horrible.

It sounds more like a feeling instead of any issue in real.  Unless I
hear some convincing technical explanation, I will send the patch for
-rc to fix the broken reboot on imx6q.

Shawn

[PATCH v3] ARM: imx: replicate the diagnostic register of boot cpu into secondary cores

[Dirk Behme]

On 02.05.2013 04:21, Shawn Guo wrote:
> On Wed, May 01, 2013 at 05:38:17PM +0100, Will Deacon wrote:
>> On Wed, May 01, 2013 at 05:19:24PM +0100, Dirk Behme wrote:
>>> Am 01.05.2013 17:38, schrieb Will Deacon:
>>>> On Wed, May 01, 2013 at 04:32:14PM +0100, Dirk Behme wrote:
>>> Just for a better understanding: What's wrong with using the same
>>> register values on all CPUx? I.e. with copying them from CPU0?
>>> Shouldn't all CPUx use the same setting? Or at least the same errata
>>> bits enabled?
>>
>> There's this thing called big.LITTLE... In this case, we're all A9s, but it
>> still feels horrible.
>
> It sounds more like a feeling instead of any issue in real.  Unless I
> hear some convincing technical explanation, I will send the patch for
> -rc to fix the broken reboot on imx6q.

As v3 of this patch touches only i.MX specific code, we know that i.MX6 
isn't big.LITTLE and all CPUx are the same, I agree with Shawn that it's 
ok to do it this way to fix a real bug.

Acked-by: Dirk Behme <dirk.behme@de.bosch.com>

Thanks and best regards

Dirk