| Message ID | 1366948645-12060-1-git-send-email-shawn.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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
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
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
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
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
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 = {
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(+)