ARM: BCM5301X: Implement SMP support
Subsystems (4)
RFC PATCH 1/1 ARM: BCM5301X: Implement SMP support
--- arch/arm/boot/dts/bcm4708.dtsi | 1 + arch/arm/mach-bcm/Makefile | 3 + arch/arm/mach-bcm/bcm5301x_headsmp.S | 108 ++++++++++++++++++++ arch/arm/mach-bcm/bcm5301x_smp.c | 185 +++++++++++++++++++++++++++++++++++ 4 files changed, 297 insertions(+) create mode 100644 arch/arm/mach-bcm/bcm5301x_headsmp.S create mode 100644 arch/arm/mach-bcm/bcm5301x_smp.c
arch/arm/boot/dts/bcm4708.dtsi +1 -0
@@ -15,6 +15,7 @@ cpus { #address-cells = <1>; #size-cells = <0>; + enable-method = "brcm,bcm4708-smp"; cpu at 0 { device_type = "cpu";
arch/arm/mach-bcm/Makefile +3 -0
@@ -36,6 +36,9 @@ obj-$(CONFIG_ARCH_BCM2835) += board_bcm2835.o # BCM5301X obj-$(CONFIG_ARCH_BCM_5301X) += bcm_5301x.o +ifeq ($(CONFIG_SMP),y) +obj-$(CONFIG_ARCH_BCM_5301X) += bcm5301x_smp.o bcm5301x_headsmp.o +endif # BCM63XXx obj-$(CONFIG_ARCH_BCM_63XX) := bcm63xx.o
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +108 -0
108 lines not on this page · raw message
arch/arm/mach-bcm/bcm5301x_smp.c new file +185 -0
185 lines not on this page · raw message
Signature · 1 line
1.8.4.5
RFC PATCH 1/1 ARM: BCM5301X: Implement SMP support
Thanks for working on this.
Someone else with more knowledge about arm cortex A9 SMP stuff should look at this patch.
Did you had a look at mach-rockchip/platsmp.c ? While I was looking at SMP stuff this code looked clean to me and they are also using a Cortex A9.
There are some comments in the code.
Hauke
Quoted 89 lines · Rafa? Mi?ecki
Signed-off-by Rafa? Mi?ecki <[email protected]>--- arch/arm/boot/dts/bcm4708.dtsi | 1 + arch/arm/mach-bcm/Makefile | 3 + arch/arm/mach-bcm/bcm5301x_headsmp.S | 108 ++++++++++++++++++++ arch/arm/mach-bcm/bcm5301x_smp.c | 185 +++++++++++++++++++++++++++++++++++ 4 files changed, 297 insertions(+) create mode 100644 arch/arm/mach-bcm/bcm5301x_headsmp.S create mode 100644 arch/arm/mach-bcm/bcm5301x_smp.carch/arm/boot/dts/bcm4708.dtsi +1
-0@@ -15,6 +15,7 @@ cpus { #address-cells = <1>; #size-cells = <0>; + enable-method = "brcm,bcm4708-smp"; cpu at 0 { device_type = "cpu";arch/arm/mach-bcm/Makefile +3
-0@@ -36,6 +36,9 @@ obj-$(CONFIG_ARCH_BCM2835) += board_bcm2835.o # BCM5301X obj-$(CONFIG_ARCH_BCM_5301X) += bcm_5301x.o +ifeq ($(CONFIG_SMP),y) +obj-$(CONFIG_ARCH_BCM_5301X) += bcm5301x_smp.o bcm5301x_headsmp.o +endif # BCM63XXx obj-$(CONFIG_ARCH_BCM_63XX) := bcm63xx.oarch/arm/mach-bcm/bcm5301x_headsmp.S new file +47
-0@@ -0,0 +1,108 @@ +/* + * Broadcom BCM470X / BCM5301X ARM platform code. + * + * Copyright 2003 - 2008 Broadcom Corporation + * + * Licensed under the GNU/GPL. See COPYING for details. + */ + +#include <asm/memory.h> + +#include <linux/linkage.h> +#include <linux/init.h> + +#define __virt_to_phys(x) ((x) - PAGE_OFFSET) + +/* + * v7_l1_cache_invalidate + * + * Invalidate contents of L1 cache without flushing its contents + * into outer cache and memory. This is needed when the contents + * of the cache are unpredictable after power-up. + * + * corrupts r0-r6 + */ +ENTRY(v7_l1_cache_invalidate) + mov r0, #0 + mcr p15, 2, r0, c0, c0, 0 @ set cache level to 1 + mrc p15, 1, r0, c0, c0, 0 @ read CLIDR + + ldr r1, =0x7fff + and r2, r1, r0, lsr #13 @ get max # of index size + + ldr r1, =0x3ff + and r3, r1, r0, lsr #3 @ NumWays - 1 + add r2, r2, #1 @ NumSets + + and r0, r0, #0x7 + add r0, r0, #4 @ SetShift + + clz r1, r3 @ WayShift + add r4, r3, #1 @ NumWays +1: sub r2, r2, #1 @ NumSets-- + mov r3, r4 @ Temp = NumWays +2: subs r3, r3, #1 @ Temp-- + mov r5, r3, lsl r1 + mov r6, r2, lsl r0 + orr r5, r5, r6 @ Reg = (Temp<<WayShift)|(NumSets<<SetShift)
arch/arm/mach-bcm/bcm5301x_headsmp.S line 57
Quoted 10 lines
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +10
-0+ mcr p15, 0, r5, c7, c6, 2 @ Invalidate line + bgt 2b + cmp r2, #0 + bgt 1b + dsb + mov r0,#0 + mcr p15,0,r0,c7,c5,0 /* Invalidate icache */ + isb + mov pc, lr +ENDPROC(v7_l1_cache_invalidate)
This function looks similar to v7_invalidate_l1 and v7_flush_icache_all in arch/arm/mm/cache-v7.S if it is different and it is intended could you please point it out in the comment.
Quoted 67 lines
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +50
-0+/* + * Platform specific entry point for secondary CPUs. This + * provides a "holding pen" into which all secondary cores are held + * until we're ready for them to initialise. + */ + __CPUINIT +ENTRY(bcm5301x_secondary_startup) + /* + * Get hardware CPU id of ours + */ + mrc p15, 0, r0, c0, c0, 5 + and r0, r0, #15 + /* + * Wait on <pen_release> variable by physical address + * to contain our hardware CPU id + */ +#ifdef CONFIG_SPARSEMEM + ldr r2, =(PAGE_OFFSET+SZ_128M) + ldr r1, =pen_release + cmp r1, r2 + bge 1f + ldr r2, =PAGE_OFFSET + sub r6, r1, r2 + b 2f +1: + sub r1, r1, r2 + ldr r2, =PHYS_OFFSET2 + add r6, r1, r2 +2: +#else + ldr r6, =__virt_to_phys(pen_release) +#endif +pen: ldr r7, [r6] + cmp r7, r0 + bne pen + nop + /* + * In case L1 cache has unpredictable contents at power-up + * clean its contents without flushing. + */ + bl v7_l1_cache_invalidate + nop + /* + * we've been released from the holding pen: secondary_stack + * should now contain the SVC stack for this core + */ + b secondary_startup + +ENDPROC(bcm5301x_secondary_startup) + .ltorgarch/arm/mach-bcm/bcm5301x_smp.c new file +11
-0@@ -0,0 +1,185 @@ +/* +* Broadcom BCM470X / BCM5301X ARM platform code. +* +* Copyright (C) 2002 ARM Ltd. +* Copyright (C) 2015 Rafa? Mi?ecki <[email protected]> +* +* Licensed under the GNU/GPL. See COPYING for details. +*/ + +#include <asm/cacheflush.h> +#include <asm/delay.h>
arch/arm/mach-bcm/bcm5301x_smp.c line 21
Quoted 10 lines
arch/arm/mach-bcm/bcm5301x_smp.c new file +10
-0+#include <asm/smp_scu.h> + +#include <linux/clockchips.h> + +/* +* There is a 1KB LUT located at 0xFFFF0400-0xFFFFFFFF, and its first entry +* is where the secondary entry point needs to be written +*/ +#define SOC_ROM_BASE_PA 0xffff0000 +#define SOC_ROM_LUT_OFF 0x400
Shouldn't this be given through device tree?
arch/arm/mach-bcm/bcm5301x_smp.c line 24
Quoted 3 lines
arch/arm/mach-bcm/bcm5301x_smp.c new file +3
-0+ +/* ENTRY in bcm5301x_headsmp.S */ +extern void bcm5301x_secondary_startup (void );
Shouldn't this go into some common header? I think we do not have such a header so it is not that of a problem for now.
Quoted 162 lines
arch/arm/mach-bcm/bcm5301x_smp.c new file +161
-0+ +static DEFINE_SPINLOCK (boot_lock); + +static void __cpuinit write_pen_release (int val) +{ + pen_release = val; +/* Make sure this store is visible to other CPUs */ +smp_wmb (); +__cpuc_flush_dcache_area ((void *)&pen_release,sizeof (pen_release)); +outer_clean_range (__pa (&pen_release),__pa (&pen_release +1 )); +} + +static void __init bcm5301x_smp_secondary_set_entry (void (*entry_point)(void )) +{ +void __iomem *rombase =NULL ; +phys_addr_t lut_pa; +u32 offset, mask; +u32 val; + + mask = (1UL <<PAGE_SHIFT ) -1 ; + + lut_pa =SOC_ROM_BASE_PA & ~mask; + offset =SOC_ROM_BASE_PA & mask; + offset +=SOC_ROM_LUT_OFF ; + + rombase =ioremap (lut_pa,PAGE_SIZE ); +if (!rombase) +return ; + val =virt_to_phys (entry_point); + +writel (val, rombase + offset); + +smp_wmb ();/* probably not needed - io regs are not cached */ +dsb_sev ();/* Exit WFI */ +mb (); + +iounmap (rombase); +} + +static void __init bcm5301x_smp_prepare_cpus (unsigned int max_cpus) +{ +void __iomem *scu_base; +unsigned int ncores; + +if (!scu_a9_has_base ()) { +pr_warn ("Unknown SCU base\n" ); +return ; + } + + scu_base =ioremap ((phys_addr_t )scu_a9_get_base (),SZ_256 ); +if (!scu_base) { +pr_err ("Failed to remap SCU\n" ); +return ; + } + + ncores =scu_get_core_count (scu_base); +if (max_cpus > ncores) { +unsigned int i; + +pr_warn ("Possible CPU mask exceeds available cores, reducing to %u\n" , + ncores); +for (i = ncores -1 ; i < max_cpus; i++) +set_cpu_present (i, false); + max_cpus = ncores; + } + +if (max_cpus >1 ) { +/* nobody is to be released from the pen yet */ + pen_release =-1 ; + +/* Initialise the SCU */ +scu_enable (scu_base); + +/* Let CPUs know where to start */ +bcm5301x_smp_secondary_set_entry (bcm5301x_secondary_startup); + } + +iounmap (scu_base); +} + +static void __cpuinit bcm5301x_smp_secondary_init (unsigned int cpu) +{ +trace_hardirqs_off (); + +/* +* let the primary processor know we're out of the +* pen, then head off into the C entry point +*/ +write_pen_release (-1 ); + +/* +* Synchronise with the boot thread. +*/ +spin_lock (&boot_lock); +spin_unlock (&boot_lock); +} + +static int __cpuinit bcm5301x_smp_boot_secondary (unsigned int cpu, +struct task_struct *idle) +{ +unsigned long timeout; + +/* +* set synchronisation state between this boot processor +* and the secondary one +*/ +spin_lock (&boot_lock); + +/* +* The secondary processor is waiting to be released from +* the holding pen - release it, then wait for it to flag +* that it has been released by resetting pen_release. +* +* Note that "pen_release" is the hardware CPU ID, whereas +* "cpu" is Linux's internal ID. +*/ +write_pen_release (cpu); + +dsb_sev (); + +/* +* Timeout set on purpose in jiffies so that on slow processors +* that must also have low HZ it will wait longer. +*/ + timeout = jiffies + (HZ *10 ); + +udelay (100 ); + +/* +* If the secondary CPU was waiting on WFE, it should +* be already watching <pen_release>, or it could be +* waiting in WFI, send it an IPI to be sure it wakes. +*/ +if (pen_release !=-1 ) +tick_broadcast (cpumask_of (cpu)); + +while (time_before (jiffies, timeout)) { +smp_rmb (); +if (pen_release ==-1 ) +break ; + +udelay (10 ); + } + +/* +* now the secondary core is starting up let it run its +* calibrations, then wait for it to finish +*/ +spin_unlock (&boot_lock); + +return pen_release !=-1 ? -ENOSYS :0 ; +} + +static struct smp_operations bcm5301x_smp_ops__initdata = { + .smp_prepare_cpus = bcm5301x_smp_prepare_cpus, + .smp_secondary_init = bcm5301x_smp_secondary_init, + .smp_boot_secondary = bcm5301x_smp_boot_secondary, +}; + +CPU_METHOD_OF_DECLARE (bcm5301x_smp,"brcm,bcm4708-smp" , + &bcm5301x_smp_ops);
RFC PATCH 1/1 ARM: BCM5301X: Implement SMP support
In reply to Rafał MiłeckiQuoted 15 lines · Rafa? Mi?ecki
Signed-off-by Rafa? Mi?ecki <[email protected]>--- arch/arm/boot/dts/bcm4708.dtsi | 1 + arch/arm/mach-bcm/Makefile | 3 + arch/arm/mach-bcm/bcm5301x_headsmp.S | 108 ++++++++++++++++++++ arch/arm/mach-bcm/bcm5301x_smp.c | 185 +++++++++++++++++++++++++++++++++++ 4 files changed, 297 insertions(+) create mode 100644 arch/arm/mach-bcm/bcm5301x_headsmp.S create mode 100644 arch/arm/mach-bcm/bcm5301x_smp.carch/arm/boot/dts/bcm4708.dtsi +0
-0
arch/arm/boot/dts/bcm4708.dtsi line 18
Quoted 4 lines
arch/arm/boot/dts/bcm4708.dtsi +1
-0@@ -15,6 +15,7 @@ cpus { #address-cells = <1>; #size-cells = <0>; + enable-method = "brcm,bcm4708-smp";
This must be documented.
We really should be getting to the point where we have a small number of standard(ish) enable methods rather than just adding a load of new IMP DEF methods with pointless differences.
Quoted 27 lines
arch/arm/boot/dts/bcm4708.dtsi +0
-0cpu at 0 { device_type = "cpu";arch/arm/mach-bcm/Makefile +3
-0@@ -36,6 +36,9 @@ obj-$(CONFIG_ARCH_BCM2835) += board_bcm2835.o # BCM5301X obj-$(CONFIG_ARCH_BCM_5301X) += bcm_5301x.o +ifeq ($(CONFIG_SMP),y) +obj-$(CONFIG_ARCH_BCM_5301X) += bcm5301x_smp.o bcm5301x_headsmp.o +endif # BCM63XXx obj-$(CONFIG_ARCH_BCM_63XX) := bcm63xx.oarch/arm/mach-bcm/bcm5301x_headsmp.S new file +4
-0@@ -0,0 +1,108 @@ +/* + * Broadcom BCM470X / BCM5301X ARM platform code. + * + * Copyright 2003 - 2008 Broadcom Corporation
arch/arm/mach-bcm/bcm5301x_headsmp.S line 14
Quoted 10 lines
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +10
-0+ * + * Licensed under the GNU/GPL. See COPYING for details. + */ + +#include <asm/memory.h> + +#include <linux/linkage.h> +#include <linux/init.h> + +#define __virt_to_phys(x) ((x) - PAGE_OFFSET)
This does not looks like something there should be a custom implementation of.
Quoted 4 lines
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +4
-0+ +/* + * v7_l1_cache_invalidate + *
arch/arm/mach-bcm/bcm5301x_headsmp.S line 28
Quoted 10 lines
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +10
-0+ * Invalidate contents of L1 cache without flushing its contents + * into outer cache and memory. This is needed when the contents + * of the cache are unpredictable after power-up. + * + * corrupts r0-r6 + */ +ENTRY(v7_l1_cache_invalidate) + mov r0, #0 + mcr p15, 2, r0, c0, c0, 0 @ set cache level to 1 + mrc p15, 1, r0, c0, c0, 0 @ read CLIDR
Isn't that the CCSIDR, not the CLIDR?
You need an ISB between CSSELR writes and CCSIDR reads.
Quoted 17 lines
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +17
-0+ + ldr r1, =0x7fff + and r2, r1, r0, lsr #13 @ get max # of index size + + ldr r1, =0x3ff + and r3, r1, r0, lsr #3 @ NumWays - 1 + add r2, r2, #1 @ NumSets + + and r0, r0, #0x7 + add r0, r0, #4 @ SetShift + + clz r1, r3 @ WayShift + add r4, r3, #1 @ NumWays +1: sub r2, r2, #1 @ NumSets-- + mov r3, r4 @ Temp = NumWays +2: subs r3, r3, #1 @ Temp-- + mov r5, r3, lsl r1
arch/arm/mach-bcm/bcm5301x_headsmp.S line 55
Quoted 10 lines
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +10
-0+ mov r6, r2, lsl r0 + orr r5, r5, r6 @ Reg = (Temp<<WayShift)|(NumSets<<SetShift) + mcr p15, 0, r5, c7, c6, 2 @ Invalidate line + bgt 2b + cmp r2, #0 + bgt 1b + dsb + mov r0,#0 + mcr p15,0,r0,c7,c5,0 /* Invalidate icache */ + isb
SUrely you're missing a dsb after the i-cache maintenance?
arch/arm/mach-bcm/bcm5301x_headsmp.S line 57
Quoted 2 lines
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +2
-0+ mov pc, lr +ENDPROC(v7_l1_cache_invalidate)
This looks like a total mess. If you _really_ need this, factor it out of the existing cache flush infrastructure. We don't need more broken copies.
Do you have a guarantee that the CPU won't write back any of this naturally before the invalidate is complete?
Is the CPU coherent at this point?
arch/arm/mach-bcm/bcm5301x_headsmp.S line 70
Quoted 13 lines
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +13
-0+ +/* + * Platform specific entry point for secondary CPUs. This + * provides a "holding pen" into which all secondary cores are held + * until we're ready for them to initialise. + */ + __CPUINIT +ENTRY(bcm5301x_secondary_startup) + /* + * Get hardware CPU id of ours + */ + mrc p15, 0, r0, c0, c0, 5 + and r0, r0, #15
Test all of the MPIDR.Aff* bits, please.
Quoted 7 lines
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +7
-0+ /* + * Wait on <pen_release> variable by physical address + * to contain our hardware CPU id + */ +#ifdef CONFIG_SPARSEMEM + ldr r2, =(PAGE_OFFSET+SZ_128M) + ldr r1, =pen_release
arch/arm/mach-bcm/bcm5301x_headsmp.S line 87
Quoted 10 lines
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +10
-0+ cmp r1, r2 + bge 1f + ldr r2, =PAGE_OFFSET + sub r6, r1, r2 + b 2f +1: + sub r1, r1, r2 + ldr r2, =PHYS_OFFSET2 + add r6, r1, r2 +2:
Huh? We really shouldn't have to care about SPARSEMEM in this kind of code. I assume the fundamental issue here is your custom __virt_to_phys implementation.
arch/arm/mach-bcm/bcm5301x_headsmp.S line 94
Quoted 7 lines
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +7
-0+#else + ldr r6, =__virt_to_phys(pen_release) +#endif +pen: ldr r7, [r6] + cmp r7, r0 + bne pen + nop
Pointless nop?
arch/arm/mach-bcm/bcm5301x_headsmp.S line 100
Quoted 6 lines
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +6
-0+ /* + * In case L1 cache has unpredictable contents at power-up + * clean its contents without flushing. + */ + bl v7_l1_cache_invalidate + nop
Another pointless nop?
Quoted 25 lines
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +8
-0+ /* + * we've been released from the holding pen: secondary_stack + * should now contain the SVC stack for this core + */ + b secondary_startup + +ENDPROC(bcm5301x_secondary_startup) + .ltorgarch/arm/mach-bcm/bcm5301x_smp.c new file +11
-0@@ -0,0 +1,185 @@ +/* +* Broadcom BCM470X / BCM5301X ARM platform code. +* +* Copyright (C) 2002 ARM Ltd. +* Copyright (C) 2015 Rafa? Mi?ecki <[email protected]> +* +* Licensed under the GNU/GPL. See COPYING for details. +*/ + +#include <asm/cacheflush.h> +#include <asm/delay.h>
arch/arm/mach-bcm/bcm5301x_smp.c line 21
Quoted 10 lines
arch/arm/mach-bcm/bcm5301x_smp.c new file +10
-0+#include <asm/smp_scu.h> + +#include <linux/clockchips.h> + +/* +* There is a 1KB LUT located at 0xFFFF0400-0xFFFFFFFF, and its first entry +* is where the secondary entry point needs to be written +*/ +#define SOC_ROM_BASE_PA 0xffff0000 +#define SOC_ROM_LUT_OFF 0x400
We shouldn't be hard-coding physical addresses; those should come from the DT.
arch/arm/mach-bcm/bcm5301x_smp.c line 34
Quoted 13 lines
arch/arm/mach-bcm/bcm5301x_smp.c new file +13
-0+ +/* ENTRY in bcm5301x_headsmp.S */ +extern void bcm5301x_secondary_startup (void ); + +static DEFINE_SPINLOCK (boot_lock); + +static void __cpuinit write_pen_release (int val) +{ + pen_release = val; +/* Make sure this store is visible to other CPUs */ +smp_wmb (); +__cpuc_flush_dcache_area ((void *)&pen_release,sizeof (pen_release)); +outer_clean_range (__pa (&pen_release),__pa (&pen_release +1 ));
Surely we have some common infrastructure to perform this sort of maintenance?
Quoted 13 lines
arch/arm/mach-bcm/bcm5301x_smp.c new file +13
-0+} + +static void __init bcm5301x_smp_secondary_set_entry (void (*entry_point)(void )) +{ +void __iomem *rombase =NULL ; +phys_addr_t lut_pa; +u32 offset, mask; +u32 val; + + mask = (1UL <<PAGE_SHIFT ) -1 ; + + lut_pa =SOC_ROM_BASE_PA & ~mask; + offset =SOC_ROM_BASE_PA & mask;
arch/arm/mach-bcm/bcm5301x_smp.c line 57
Quoted 10 lines
arch/arm/mach-bcm/bcm5301x_smp.c new file +10
-0+ offset +=SOC_ROM_LUT_OFF ; + + rombase =ioremap (lut_pa,PAGE_SIZE ); +if (!rombase) +return ; + val =virt_to_phys (entry_point); + +writel (val, rombase + offset); + +smp_wmb ();/* probably not needed - io regs are not cached */
Surely the following DSB is sufficient?
arch/arm/mach-bcm/bcm5301x_smp.c line 59
Quoted 2 lines
arch/arm/mach-bcm/bcm5301x_smp.c new file +2
-0+dsb_sev ();/* Exit WFI */ +mb ();
What's the mb for?
Quoted 11 lines
arch/arm/mach-bcm/bcm5301x_smp.c new file +11
-0+ +iounmap (rombase); +} + +static void __init bcm5301x_smp_prepare_cpus (unsigned int max_cpus) +{ +void __iomem *scu_base; +unsigned int ncores; + +if (!scu_a9_has_base ()) { +pr_warn ("Unknown SCU base\n" );
arch/arm/mach-bcm/bcm5301x_smp.c line 80
Quoted 10 lines
arch/arm/mach-bcm/bcm5301x_smp.c new file +10
-0+return ; + } + + scu_base =ioremap ((phys_addr_t )scu_a9_get_base (),SZ_256 ); +if (!scu_base) { +pr_err ("Failed to remap SCU\n" ); +return ; + } + + ncores =scu_get_core_count (scu_base);
Just read this from the DT as we do elsewhere.
Quoted 23 lines
arch/arm/mach-bcm/bcm5301x_smp.c new file +23
-0+if (max_cpus > ncores) { +unsigned int i; + +pr_warn ("Possible CPU mask exceeds available cores, reducing to %u\n" , + ncores); +for (i = ncores -1 ; i < max_cpus; i++) +set_cpu_present (i, false); + max_cpus = ncores; + } + +if (max_cpus >1 ) { +/* nobody is to be released from the pen yet */ + pen_release =-1 ; + +/* Initialise the SCU */ +scu_enable (scu_base); + +/* Let CPUs know where to start */ +bcm5301x_smp_secondary_set_entry (bcm5301x_secondary_startup); + } + +iounmap (scu_base); +}
[...]
Quoted 10 lines
arch/arm/mach-bcm/bcm5301x_smp.c new file +10
-0+static int __cpuinit bcm5301x_smp_boot_secondary (unsigned int cpu, +struct task_struct *idle) +{ +unsigned long timeout; + +/* +* set synchronisation state between this boot processor +* and the secondary one +*/ +spin_lock (&boot_lock);
arch/arm/mach-bcm/bcm5301x_smp.c line 141
Quoted 10 lines
arch/arm/mach-bcm/bcm5301x_smp.c new file +10
-0+ +/* +* The secondary processor is waiting to be released from +* the holding pen - release it, then wait for it to flag +* that it has been released by resetting pen_release. +* +* Note that "pen_release" is the hardware CPU ID, whereas +* "cpu" is Linux's internal ID. +*/ +write_pen_release (cpu);
As far as I can tell you're relying on the logical ID being equivalent to MPDR.Aff0, which isn't necessarily true. Either use the physical ID or use the actual logical ID.
Quoted 8 lines
arch/arm/mach-bcm/bcm5301x_smp.c new file +8
-0+ +dsb_sev (); + +/* +* Timeout set on purpose in jiffies so that on slow processors +* that must also have low HZ it will wait longer. +*/ + timeout = jiffies + (HZ *10 );
arch/arm/mach-bcm/bcm5301x_smp.c line 159
Quoted 10 lines
arch/arm/mach-bcm/bcm5301x_smp.c new file +10
-0+ +udelay (100 ); + +/* +* If the secondary CPU was waiting on WFE, it should +* be already watching <pen_release>, or it could be +* waiting in WFI, send it an IPI to be sure it wakes. +*/ +if (pen_release !=-1 ) +tick_broadcast (cpumask_of (cpu));
NAK. This is not what tick_broadcast is intended for.
If you need an IPI then send an IPI, don't piggyback on the timekeeping infrastructure.
Mark.
PATCH 1/1 ARM: BCM5301X: Implement SMP support
In reply to Rafał Miłecki--- Documentation/devicetree/bindings/arm/bcm4708.txt | 17 +++ Documentation/devicetree/bindings/arm/cpus.txt | 1 + arch/arm/boot/dts/bcm4708.dtsi | 5 + arch/arm/mach-bcm/Makefile | 3 + arch/arm/mach-bcm/bcm5301x_headsmp.S | 46 +++++++ arch/arm/mach-bcm/bcm5301x_smp.c | 160 ++++++++++++++++++++++ 6 files changed, 232 insertions(+) create mode 100644 arch/arm/mach-bcm/bcm5301x_headsmp.S create mode 100644 arch/arm/mach-bcm/bcm5301x_smp.c
Documentation/devicetree/bindings/arm/bcm4708.txt +17 -0
@@ -6,3 +6,20 @@ Boards with the BCM4708 SoC shall have the following properties: Required root node property: compatible = "brcm,bcm4708"; + +Optional sub-node properties: + +compatible = "brcm,bcm4708-sysram" SYSRAM for SMP bringup. + SMP-capable SoCs use part of the SYSRAM for storing + location of code to be executed by the extra cores. + +Example: + + { + compatible = "brcm,bcm4708"; + + smp-sysram { + compatible = "brcm,bcm4708-sysram"; + reg = <0xffff0000 0x1000>; + }; + };
Documentation/devicetree/bindings/arm/cpus.txt +1 -0
@@ -188,6 +188,7 @@ nodes to be present and contain the properties described below. can be one of: "allwinner,sun6i-a31" "arm,psci" + "brcm,bcm4708-smp" "brcm,brahma-b15" "marvell,armada-375-smp" "marvell,armada-380-smp"
arch/arm/boot/dts/bcm4708.dtsi +5 -0
@@ -15,6 +15,7 @@ cpus { #address-cells = <1>; #size-cells = <0>; + enable-method = "brcm,bcm4708-smp"; cpu at 0 { device_type = "cpu";
@@ -31,4 +32,8 @@ }; }; + smp-sysram { + compatible = "brcm,bcm4708-sysram"; + reg = <0xffff0000 0x1000>; + }; };
arch/arm/mach-bcm/Makefile +3 -0
@@ -36,6 +36,9 @@ obj-$(CONFIG_ARCH_BCM2835) += board_bcm2835.o # BCM5301X obj-$(CONFIG_ARCH_BCM_5301X) += bcm_5301x.o +ifeq ($(CONFIG_SMP),y) +obj-$(CONFIG_ARCH_BCM_5301X) += bcm5301x_smp.o bcm5301x_headsmp.o +endif # BCM63XXx obj-$(CONFIG_ARCH_BCM_63XX) := bcm63xx.o
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +46 -0
46 lines not on this page · raw message
arch/arm/mach-bcm/bcm5301x_smp.c new file +160 -0
160 lines not on this page · raw message
Signature · 1 line
1.8.4.5
PATCH 1/1 ARM: BCM5301X: Implement SMP support
Quoted 1 line · Rafa? Mi?ecki
Signed-off-by Rafa? Mi?ecki <[email protected]>
So does it look good enough to send it in pull request? :)
PATCH 1/1 ARM: BCM5301X: Implement SMP support
In reply to Rafał MiłeckiQuoted 2 lines · Rafa? Mi?ecki
Signed-off-by Rafa? Mi?ecki <[email protected]>---
[snip]
Documentation/devicetree/bindings/arm/bcm4708.txt Boards with the BCM4708 SoC shall have the following properties: · line 14
Quoted 5 lines
Documentation/devicetree/bindings/arm/bcm4708.txt +5
-0+Optional sub-node properties: + +compatible = "brcm,bcm4708-sysram" SYSRAM for SMP bringup. + SMP-capable SoCs use part of the SYSRAM for storing + location of code to be executed by the extra cores.
Is this a regular kind of SRAM? If so, can you use "mmio-sram" as a compatible fallback?
[snip]
arch/arm/mach-bcm/bcm5301x_headsmp.S line 29
Quoted 6 lines
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +6
-0+ + /* + * In case L1 cache has unpredictable contents at power-up + * clean its contents without flushing. + */ + /* bl v7_l1_cache_invalidate */
You can remove that comment now
arch/arm/mach-bcm/bcm5301x_headsmp.S line 33
Quoted 4 lines
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +4
-0+ bl v7_invalidate_l1 + + mov r0,#0 + mcr p15,0,r0,c7,c5,0 /* Invalidate icache */
For consistency with the previous lines, you would want to space operands.
Quoted 51 lines
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +13
-0+ dsb + isb + + /* + * we've been released from the holding pen: secondary_stack + * should now contain the SVC stack for this core + */ + b secondary_startup +ENDPROC(bcm5301x_secondary_startup) + + .align 2 +1: .long . + .long pen_releasearch/arm/mach-bcm/bcm5301x_smp.c new file +32
-0@@ -0,0 +1,160 @@ +/* +* Broadcom BCM470X / BCM5301X ARM platform code. +* +* Copyright (C) 2002 ARM Ltd. +* Copyright (C) 2015 Rafa? Mi?ecki <[email protected]> +* +* Licensed under the GNU/GPL. See COPYING for details. +*/ + +#include <asm/cacheflush.h> +#include <asm/delay.h> +#include <asm/smp_plat.h> +#include <asm/smp_scu.h> + +#include <linux/clockchips.h> +#include <linux/of.h> +#include <linux/of_address.h> + +#define SOC_ROM_LUT_OFF 0x400 + +extern void bcm5301x_secondary_startup (void ); + +static void __cpuinit write_pen_release (int val) +{ + pen_release = val; +smp_wmb (); +sync_cache_w (&pen_release); +} + +static DEFINE_SPINLOCK (boot_lock); + +static void __init bcm5301x_smp_secondary_set_entry (void (*entry_point)(void ))
arch/arm/mach-bcm/bcm5301x_smp.c line 42
Quoted 10 lines
arch/arm/mach-bcm/bcm5301x_smp.c new file +10
-0+{ +void __iomem *sysram_base_addr =NULL ; +struct device_node *node; + +for_each_compatible_node (node,NULL ,"brcm,bcm4708-sysram" ) { +if (!of_device_is_available (node)) +continue ; + sysram_base_addr =of_iomap (node,0 ); +break ; + }
How about of_find_compatible()? Do you expect more than one sysram node in a DTS?
Signature · 1 line
Florian
PATCH v2 1/1 ARM: BCM5301X: Implement SMP support
In reply to Rafał Miłecki---
V2: Change code after receiving Florian's comments:
1) Use "mmio-sram"
2) Remove commented out ASM call
3) Fix coding style in ASM
4) Simplify finding OF node
--- Documentation/devicetree/bindings/arm/bcm4708.txt | 24 ++++ Documentation/devicetree/bindings/arm/cpus.txt | 1 + arch/arm/boot/dts/bcm4708.dtsi | 13 ++ arch/arm/mach-bcm/Makefile | 3 + arch/arm/mach-bcm/bcm5301x_headsmp.S | 45 ++++++ arch/arm/mach-bcm/bcm5301x_smp.c | 158 ++++++++++++++++++++++ 6 files changed, 244 insertions(+) create mode 100644 arch/arm/mach-bcm/bcm5301x_headsmp.S create mode 100644 arch/arm/mach-bcm/bcm5301x_smp.c
Documentation/devicetree/bindings/arm/bcm4708.txt +24 -0
@@ -6,3 +6,27 @@ Boards with the BCM4708 SoC shall have the following properties: Required root node property: compatible = "brcm,bcm4708"; + +Optional sub-node properties: + +compatible = "mmio-sram" for SRAM access with IO memory region + This is needed for SMP-capable SoCs which use part of + SRAM for storing location of code to be executed by the + extra cores. + SMP support requires another sub-node with compatible + property "brcm,bcm4708-sysram". + +Example: + + sysram at ffff0000 { + compatible = "mmio-sram"; + reg = <0xffff0000 0x10000>; + #address-cells = <1>; + #size-cells = <1>; + ranges = <0 0xffff0000 0x10000>; + + smp-sysram at 0 { + compatible = "brcm,bcm4708-sysram"; + reg = <0x0 0x1000>; + }; + };
Documentation/devicetree/bindings/arm/cpus.txt +1 -0
@@ -189,6 +189,7 @@ nodes to be present and contain the properties described below. can be one of: "allwinner,sun6i-a31" "arm,psci" + "brcm,bcm4708-smp" "brcm,brahma-b15" "marvell,armada-375-smp" "marvell,armada-380-smp"
arch/arm/boot/dts/bcm4708.dtsi +13 -0
@@ -15,6 +15,7 @@ cpus { #address-cells = <1>; #size-cells = <0>; + enable-method = "brcm,bcm4708-smp"; cpu at 0 { device_type = "cpu";
@@ -31,4 +32,16 @@ }; }; + sysram at ffff0000 { + compatible = "mmio-sram"; + reg = <0xffff0000 0x10000>; + #address-cells = <1>; + #size-cells = <1>; + ranges = <0 0xffff0000 0x10000>; + + smp-sysram at 0 { + compatible = "brcm,bcm4708-sysram"; + reg = <0x0 0x1000>; + }; + }; };
arch/arm/mach-bcm/Makefile +3 -0
@@ -36,6 +36,9 @@ obj-$(CONFIG_ARCH_BCM2835) += board_bcm2835.o # BCM5301X obj-$(CONFIG_ARCH_BCM_5301X) += bcm_5301x.o +ifeq ($(CONFIG_SMP),y) +obj-$(CONFIG_ARCH_BCM_5301X) += bcm5301x_smp.o bcm5301x_headsmp.o +endif # BCM63XXx obj-$(CONFIG_ARCH_BCM_63XX) := bcm63xx.o
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +45 -0
45 lines not on this page · raw message
arch/arm/mach-bcm/bcm5301x_smp.c new file +158 -0
158 lines not on this page · raw message
Signature · 1 line
1.8.4.5
PATCH v2 1/1 ARM: BCM5301X: Implement SMP support
Quoted 14 lines · Rafa? Mi?ecki
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +14
-0+/* + * BCM5301X specific entry point for secondary CPUs. + */ +ENTRY(bcm5301x_secondary_startup) + mrc p15, 0, r0, c0, c0, 5 + and r0, r0, #15 + adr r4, 1f + ldmia r4, {r5, r6} + sub r4, r4, r5 + add r6, r6, r4 +pen: ldr r7, [r6] + cmp r7, r0 + bne pen +
arch/arm/mach-bcm/bcm5301x_headsmp.S line 34
Quoted 10 lines
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +10
-0+ /* + * In case L1 cache has unpredictable contents at power-up + * clean its contents without flushing. + */ + bl v7_invalidate_l1 + + mov r0, #0 + mcr p15, 0, r0, c7, c5, 0 /* Invalidate icache */ + dsb + isb
So if your I-cache contains unpredictable contents, how do you execute the code to this point? Shouldn't the I-cache invalidate be the very first instruction you execute followed by the dsb and isb (oh, and iirc it ignores the value in the register).
In the case where a CPU has unpredictable contents at power up, the ARM ARM requires that an implementation specific sequence is followed to initialise the caches. I doubt that such a sequence includes testing a pen value.
arch/arm/mach-bcm/bcm5301x_smp.c line 49
Quoted 9 lines
arch/arm/mach-bcm/bcm5301x_smp.c new file +9
-0+ sysram_base_addr =of_iomap (node,0 ); +if (!sysram_base_addr) { +pr_warn ("Failed to map sysram\n" ); +return ; + } + +writel (virt_to_phys (entry_point), sysram_base_addr +SOC_ROM_LUT_OFF ); + +dsb_sev ();/* Exit WFI */
Which WFI? This seems to imply that you have some kind of initial firmware. If so, that should be taking care of the cache initialisation, not the kernel.
arch/arm/mach-bcm/bcm5301x_smp.c line 50
Quoted 1 line
arch/arm/mach-bcm/bcm5301x_smp.c new file +1
-0+mb ();/* make sure write buffer is drained */
writel() already ensures that.
arch/arm/mach-bcm/bcm5301x_smp.c line 118
Quoted 12 lines
arch/arm/mach-bcm/bcm5301x_smp.c new file +12
-0+/* +* The secondary processor is waiting to be released from +* the holding pen - release it, then wait for it to flag +* that it has been released by resetting pen_release. +* +* Note that "pen_release" is the hardware CPU ID, whereas +* "cpu" is Linux's internal ID. +*/ +write_pen_release (cpu_logical_map (cpu)); + +/* Send the secondary CPU SEV */ +dsb_sev ();
If you even need any of the pen code, if you're having to send a SEV here, wouldn't having a WFE in the pen assembly loop above be a good idea?
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.
PATCH v2 1/1 ARM: BCM5301X: Implement SMP support
Hi,
I tested this patch on my device now.
What does the loader do before Linux gets started on the second CPU and what is ensured?
Quoted 31 lines · Russell King - ARM Linux
Quoted 14 lines · Rafa? Mi?ecki
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +14
-0+/* + * BCM5301X specific entry point for secondary CPUs. + */ +ENTRY(bcm5301x_secondary_startup) + mrc p15, 0, r0, c0, c0, 5 + and r0, r0, #15 + adr r4, 1f + ldmia r4, {r5, r6} + sub r4, r4, r5 + add r6, r6, r4 +pen: ldr r7, [r6] + cmp r7, r0 + bne pen +arch/arm/mach-bcm/bcm5301x_headsmp.S line 34
Quoted 10 lines
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +10
-0+ /* + * In case L1 cache has unpredictable contents at power-up + * clean its contents without flushing. + */ + bl v7_invalidate_l1 + + mov r0, #0 + mcr p15, 0, r0, c7, c5, 0 /* Invalidate icache */ + dsb + isbSo if your I-cache contains unpredictable contents, how do you execute the code to this point? Shouldn't the I-cache invalidate be the very first instruction you execute followed by the dsb and isb (oh, and iirc it ignores the value in the register).
Quoted 4 lines
In the case where a CPU has unpredictable contents at power up, the ARM ARM requires that an implementation specific sequence is followed to initialise the caches. I doubt that such a sequence includes testing a pen value.
When I remove the test for the pen value the CPU does not come up any more, I get this log output:
[ 0.132292] CPU: Testing write buffer coherency: ok [ 0.137635] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000 [ 0.143675] Setting up static identity map for 0x82a0 - 0x82d4 [ 10.149786] CPU1: failed to boot: -38 [ 10.153651] Brought up 1 CPUs
This was caused just by removing the "cmp r7, r0" and "bne pen" instructions.
With these instructions are added it works and I get this:
[ 0.132329] CPU: Testing write buffer coherency: ok [ 0.137682] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000 [ 0.143708] Setting up static identity map for 0x82a0 - 0x82d4 [ 0.189788] CPU1: thread -1, cpu 1, socket 0, mpidr 80000001 [ 0.189892] Brought up 2 CPUs [ 0.198889] SMP: Total of 2 processors activated (3188.32 BogoMIPS).
Currently this is 100% reproducible.
Could it be that the second CPU needs some time till it is synchronised correctly?
I do not know if and why the cache clearing is needed, I do not have access to the SoC documentation or the ASIC/firmware developer.
Quoted 19 lines
arch/arm/mach-bcm/bcm5301x_smp.c line 49
Quoted 10 lines
arch/arm/mach-bcm/bcm5301x_smp.c new file +9
-0+ sysram_base_addr =of_iomap (node,0 ); +if (!sysram_base_addr) { +pr_warn ("Failed to map sysram\n" ); +return ; + } + +writel (virt_to_phys (entry_point), sysram_base_addr +SOC_ROM_LUT_OFF ); + +dsb_sev ();/* Exit WFI */ Which WFI? This seems to imply that you have some kind of initial firmware. If so, that should be taking care of the cache initialisation, not the kernel.
arch/arm/mach-bcm/bcm5301x_smp.c line 50
Quoted 1 line
arch/arm/mach-bcm/bcm5301x_smp.c new file +1
-0+mb ();/* make sure write buffer is drained */
writel()already ensures that.
Quoted 16 lines
arch/arm/mach-bcm/bcm5301x_smp.c line 118
Quoted 12 lines
arch/arm/mach-bcm/bcm5301x_smp.c new file +12
-0+/* +* The secondary processor is waiting to be released from +* the holding pen - release it, then wait for it to flag +* that it has been released by resetting pen_release. +* +* Note that "pen_release" is the hardware CPU ID, whereas +* "cpu" is Linux's internal ID. +*/ +write_pen_release (cpu_logical_map (cpu)); + +/* Send the secondary CPU SEV */ +dsb_sev ();If you even need any of the pen code, if you're having to send a SEV here, wouldn't having a WFE in the pen assembly loop above be a good idea?
I have to read more on how WFE and co works.
Hauke
Ray Jui
maintainerPATCH v2 1/1 ARM: BCM5301X: Implement SMP support
+ bcm-kernel-feedback-list.
Kapil, you might want to take a look at this. Not sure how this is related to your SMP patches for NSP.
Quoted 115 lines · Hauke Mehrtens
Hi,
I tested this patch on my device now.
What does the loader do before Linux gets started on the second CPU and what is ensured?
Quoted 31 lines · Russell King - ARM Linux
Quoted 14 lines · Rafa? Mi?ecki
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +14
-0+/* + * BCM5301X specific entry point for secondary CPUs. + */ +ENTRY(bcm5301x_secondary_startup) + mrc p15, 0, r0, c0, c0, 5 + and r0, r0, #15 + adr r4, 1f + ldmia r4, {r5, r6} + sub r4, r4, r5 + add r6, r6, r4 +pen: ldr r7, [r6] + cmp r7, r0 + bne pen +arch/arm/mach-bcm/bcm5301x_headsmp.S line 34
Quoted 10 lines
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +10
-0+ /* + * In case L1 cache has unpredictable contents at power-up + * clean its contents without flushing. + */ + bl v7_invalidate_l1 + + mov r0, #0 + mcr p15, 0, r0, c7, c5, 0 /* Invalidate icache */ + dsb + isbSo if your I-cache contains unpredictable contents, how do you execute the code to this point? Shouldn't the I-cache invalidate be the very first instruction you execute followed by the dsb and isb (oh, and iirc it ignores the value in the register).
Quoted 4 lines
In the case where a CPU has unpredictable contents at power up, the ARM ARM requires that an implementation specific sequence is followed to initialise the caches. I doubt that such a sequence includes testing a pen value.
When I remove the test for the pen value the CPU does not come up any more, I get this log output:
[ 0.132292] CPU: Testing write buffer coherency: ok [ 0.137635] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000 [ 0.143675] Setting up static identity map for 0x82a0 - 0x82d4 [ 10.149786] CPU1: failed to boot: -38 [ 10.153651] Brought up 1 CPUsThis was caused just by removing the "cmp r7, r0" and "bne pen" instructions.
With these instructions are added it works and I get this:
[ 0.132329] CPU: Testing write buffer coherency: ok [ 0.137682] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000 [ 0.143708] Setting up static identity map for 0x82a0 - 0x82d4 [ 0.189788] CPU1: thread -1, cpu 1, socket 0, mpidr 80000001 [ 0.189892] Brought up 2 CPUs [ 0.198889] SMP: Total of 2 processors activated (3188.32 BogoMIPS).Currently this is 100% reproducible.
Could it be that the second CPU needs some time till it is synchronised correctly?
I do not know if and why the cache clearing is needed, I do not have access to the SoC documentation or the ASIC/firmware developer.
Quoted 19 lines
arch/arm/mach-bcm/bcm5301x_smp.c line 49
Quoted 10 lines
arch/arm/mach-bcm/bcm5301x_smp.c new file +9
-0+ sysram_base_addr =of_iomap (node,0 ); +if (!sysram_base_addr) { +pr_warn ("Failed to map sysram\n" ); +return ; + } + +writel (virt_to_phys (entry_point), sysram_base_addr +SOC_ROM_LUT_OFF ); + +dsb_sev ();/* Exit WFI */ Which WFI? This seems to imply that you have some kind of initial firmware. If so, that should be taking care of the cache initialisation, not the kernel.
arch/arm/mach-bcm/bcm5301x_smp.c line 50
Quoted 1 line
arch/arm/mach-bcm/bcm5301x_smp.c new file +1
-0+mb ();/* make sure write buffer is drained */
writel()already ensures that.Quoted 16 lines
arch/arm/mach-bcm/bcm5301x_smp.c line 118
Quoted 12 lines
arch/arm/mach-bcm/bcm5301x_smp.c new file +12
-0+/* +* The secondary processor is waiting to be released from +* the holding pen - release it, then wait for it to flag +* that it has been released by resetting pen_release. +* +* Note that "pen_release" is the hardware CPU ID, whereas +* "cpu" is Linux's internal ID. +*/ +write_pen_release (cpu_logical_map (cpu)); + +/* Send the secondary CPU SEV */ +dsb_sev ();If you even need any of the pen code, if you're having to send a SEV here, wouldn't having a WFE in the pen assembly loop above be a good idea?
I have to read more on how WFE and co works.
Hauke
PATCH v2 1/1 ARM: BCM5301X: Implement SMP support
Quoted 4 lines · Ray Jui
+ bcm-kernel-feedback-list.
Kapil, you might want to take a look at this. Not sure how this is related to your SMP patches for NSP.
Ray, I don't have complete/other patch sets for this change. It would be good if I get those patch sets as well or complete e-mail thread. I think if we have a cleaner solutions for SMP, we can consolidate the change required for NS and NSP. I have few points to add, which are inline in this e-mail.
Quoted 41 lines · Hauke Mehrtens
Quoted 39 lines
Hi,
I tested this patch on my device now.
What does the loader do before Linux gets started on the second CPU and what is ensured?
Quoted 31 lines · Russell King - ARM Linux
Quoted 14 lines · Rafa? Mi?ecki
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +14
-0+/* + * BCM5301X specific entry point for secondary CPUs. + */ +ENTRY(bcm5301x_secondary_startup) + mrc p15, 0, r0, c0, c0, 5 + and r0, r0, #15 + adr r4, 1f + ldmia r4, {r5, r6} + sub r4, r4, r5 + add r6, r6, r4 +pen: ldr r7, [r6] + cmp r7, r0 + bne pen +arch/arm/mach-bcm/bcm5301x_headsmp.S line 34
Quoted 10 lines
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +10
-0+ /* + * In case L1 cache has unpredictable contents at power-up + * clean its contents without flushing. + */ + bl v7_invalidate_l1 + + mov r0, #0 + mcr p15, 0, r0, c7, c5, 0 /* Invalidate icache */ + dsb + isbSo if your I-cache contains unpredictable contents, how do you execute the code to this point? Shouldn't the I-cache invalidate be the very first instruction you execute followed by the dsb and isb (oh, and iirc it ignores the value in the register).
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
In the case where a CPU has unpredictable contents at power up, the ARM ARM requires that an implementation specific sequence is followed to initialise the caches. I doubt that such a sequence includes testing a pen value.
Are you sure this is an issue of unpredictable L1 cache contents at power-up? AFAIK, 5301X had an issue with secondary core initialization. Secondary core which waits on WFE would let it out of the pen as soon as the first spin_*lock executes. This was because of a BOOTROM bug in NS, so the work around was to reset the address for the secondary processor to go back and wait for the signal from the primary core. This vector fixup is required so that the secondary core doesn't start executing kernel instructions until we've patched its jump address during wakeup_secondary().
Also, v7 setup function should invalidate L1 cache and we should remove all v7_invalidate_l1 calls in all headsmp.S in platform specific directories.
Quoted 68 lines
Quoted 68 lines
When I remove the test for the pen value the CPU does not come up any more, I get this log output:
[ 0.132292] CPU: Testing write buffer coherency: ok [ 0.137635] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000 [ 0.143675] Setting up static identity map for 0x82a0 - 0x82d4 [ 10.149786] CPU1: failed to boot: -38 [ 10.153651] Brought up 1 CPUsThis was caused just by removing the "cmp r7, r0" and "bne pen" instructions.
With these instructions are added it works and I get this:
[ 0.132329] CPU: Testing write buffer coherency: ok [ 0.137682] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000 [ 0.143708] Setting up static identity map for 0x82a0 - 0x82d4 [ 0.189788] CPU1: thread -1, cpu 1, socket 0, mpidr 80000001 [ 0.189892] Brought up 2 CPUs [ 0.198889] SMP: Total of 2 processors activated (3188.32 BogoMIPS).Currently this is 100% reproducible.
Could it be that the second CPU needs some time till it is synchronised correctly?
I do not know if and why the cache clearing is needed, I do not have access to the SoC documentation or the ASIC/firmware developer.
Quoted 35 lines
arch/arm/mach-bcm/bcm5301x_smp.c line 49
Quoted 10 lines
arch/arm/mach-bcm/bcm5301x_smp.c new file +9
-0+ sysram_base_addr =of_iomap (node,0 ); +if (!sysram_base_addr) { +pr_warn ("Failed to map sysram\n" ); +return ; + } + +writel (virt_to_phys (entry_point), sysram_base_addr +SOC_ROM_LUT_OFF ); + +dsb_sev ();/* Exit WFI */ Which WFI? This seems to imply that you have some kind of initial firmware. If so, that should be taking care of the cache initialisation, not the kernel.
arch/arm/mach-bcm/bcm5301x_smp.c line 50
Quoted 1 line
arch/arm/mach-bcm/bcm5301x_smp.c new file +1
-0+mb ();/* make sure write buffer is drained */
writel()already ensures that.arch/arm/mach-bcm/bcm5301x_smp.c line 118
Quoted 12 lines
arch/arm/mach-bcm/bcm5301x_smp.c new file +12
-0+/* +* The secondary processor is waiting to be released from +* the holding pen - release it, then wait for it to flag +* that it has been released by resetting pen_release. +* +* Note that "pen_release" is the hardware CPU ID, whereas +* "cpu" is Linux's internal ID. +*/ +write_pen_release (cpu_logical_map (cpu)); + +/* Send the secondary CPU SEV */ +dsb_sev ();If you even need any of the pen code, if you're having to send a SEV here, wouldn't having a WFE in the pen assembly loop above be a good idea?
Quoted 4 lines
Quoted 4 lines
I have to read more on how WFE and co works.
Hauke
Thanks,
Kapil
PATCH v2 1/1 ARM: BCM5301X: Implement SMP support
Quoted 5 lines · Kapil Hali
Quoted 2 lines · Ray Jui
+ bcm-kernel-feedback-list.
Quoted 8 lines
Quoted 2 lines
Kapil, you might want to take a look at this. Not sure how this is related to your SMP patches for NSP.
Ray, I don't have complete/other patch sets for this change. It would be good if I get those patch sets as well or complete e-mail thread. I think if we have a cleaner solutions for SMP, we can consolidate the change required for NS and NSP. I have few points to add, which are inline in this e-mail.
Hi Kapil,
the patch was posted on the arm mailing list:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/332690.html
but you can also find it in OpenWrt here:
https://dev.openwrt.org/browser/trunk/target/linux/bcm53xx/patches-4.1/131-ARM-BCM5301X-Implement-SMP-support.patch
It looks very similar to your approach so I assume we can use the same SMP code for NS and NSP. I will look into your patches later on.
Hauke
Quoted 135 lines
Quoted 42 lines · Hauke Mehrtens
Quoted 39 lines
Hi,
I tested this patch on my device now.
What does the loader do before Linux gets started on the second CPU and what is ensured?
Quoted 31 lines · Russell King - ARM Linux
Quoted 14 lines · Rafa? Mi?ecki
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +14
-0+/* + * BCM5301X specific entry point for secondary CPUs. + */ +ENTRY(bcm5301x_secondary_startup) + mrc p15, 0, r0, c0, c0, 5 + and r0, r0, #15 + adr r4, 1f + ldmia r4, {r5, r6} + sub r4, r4, r5 + add r6, r6, r4 +pen: ldr r7, [r6] + cmp r7, r0 + bne pen +arch/arm/mach-bcm/bcm5301x_headsmp.S line 34
Quoted 10 lines
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +10
-0+ /* + * In case L1 cache has unpredictable contents at power-up + * clean its contents without flushing. + */ + bl v7_invalidate_l1 + + mov r0, #0 + mcr p15, 0, r0, c7, c5, 0 /* Invalidate icache */ + dsb + isbSo if your I-cache contains unpredictable contents, how do you execute the code to this point? Shouldn't the I-cache invalidate be the very first instruction you execute followed by the dsb and isb (oh, and iirc it ignores the value in the register).
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
In the case where a CPU has unpredictable contents at power up, the ARM ARM requires that an implementation specific sequence is followed to initialise the caches. I doubt that such a sequence includes testing a pen value.
Are you sure this is an issue of unpredictable L1 cache contents at power-up? AFAIK, 5301X had an issue with secondary core initialization. Secondary core which waits on WFE would let it out of the pen as soon as the first spin_*lock executes. This was because of a BOOTROM bug in NS, so the work around was to reset the address for the secondary processor to go back and wait for the signal from the primary core. This vector fixup is required so that the secondary core doesn't start executing kernel instructions until we've patched its jump address during
wakeup_secondary().Also, v7 setup function should invalidate L1 cache and we should remove all
v7_invalidate_l1calls in allheadsmp.Sin platform specific directories.Quoted 68 lines
Quoted 68 lines
When I remove the test for the pen value the CPU does not come up any more, I get this log output:
[ 0.132292] CPU: Testing write buffer coherency: ok [ 0.137635] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000 [ 0.143675] Setting up static identity map for 0x82a0 - 0x82d4 [ 10.149786] CPU1: failed to boot: -38 [ 10.153651] Brought up 1 CPUsThis was caused just by removing the "cmp r7, r0" and "bne pen" instructions.
With these instructions are added it works and I get this:
[ 0.132329] CPU: Testing write buffer coherency: ok [ 0.137682] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000 [ 0.143708] Setting up static identity map for 0x82a0 - 0x82d4 [ 0.189788] CPU1: thread -1, cpu 1, socket 0, mpidr 80000001 [ 0.189892] Brought up 2 CPUs [ 0.198889] SMP: Total of 2 processors activated (3188.32 BogoMIPS).Currently this is 100% reproducible.
Could it be that the second CPU needs some time till it is synchronised correctly?
I do not know if and why the cache clearing is needed, I do not have access to the SoC documentation or the ASIC/firmware developer.
Quoted 35 lines
arch/arm/mach-bcm/bcm5301x_smp.c line 49
Quoted 10 lines
arch/arm/mach-bcm/bcm5301x_smp.c new file +9
-0+ sysram_base_addr =of_iomap (node,0 ); +if (!sysram_base_addr) { +pr_warn ("Failed to map sysram\n" ); +return ; + } + +writel (virt_to_phys (entry_point), sysram_base_addr +SOC_ROM_LUT_OFF ); + +dsb_sev ();/* Exit WFI */ Which WFI? This seems to imply that you have some kind of initial firmware. If so, that should be taking care of the cache initialisation, not the kernel.
arch/arm/mach-bcm/bcm5301x_smp.c line 50
Quoted 1 line
arch/arm/mach-bcm/bcm5301x_smp.c new file +1
-0+mb ();/* make sure write buffer is drained */
writel()already ensures that.arch/arm/mach-bcm/bcm5301x_smp.c line 118
Quoted 12 lines
arch/arm/mach-bcm/bcm5301x_smp.c new file +12
-0+/* +* The secondary processor is waiting to be released from +* the holding pen - release it, then wait for it to flag +* that it has been released by resetting pen_release. +* +* Note that "pen_release" is the hardware CPU ID, whereas +* "cpu" is Linux's internal ID. +*/ +write_pen_release (cpu_logical_map (cpu)); + +/* Send the secondary CPU SEV */ +dsb_sev ();If you even need any of the pen code, if you're having to send a SEV here, wouldn't having a WFE in the pen assembly loop above be a good idea?
Quoted 4 lines
Quoted 4 lines
I have to read more on how WFE and co works.
Hauke
Thanks,
Kapil
PATCH v2 1/1 ARM: BCM5301X: Implement SMP support
Quoted 15 lines · Hauke Mehrtens
Quoted 5 lines · Kapil Hali
Quoted 2 lines · Ray Jui
+ bcm-kernel-feedback-list.
Quoted 8 lines
Quoted 2 lines
Kapil, you might want to take a look at this. Not sure how this is related to your SMP patches for NSP.
Ray, I don't have complete/other patch sets for this change. It would be good if I get those patch sets as well or complete e-mail thread. I think if we have a cleaner solutions for SMP, we can consolidate the change required for NS and NSP. I have few points to add, which are inline in this e-mail.
Quoted 8 lines
Hi Kapil,
the patch was posted on the arm mailing list:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/332690.html
but you can also find it in OpenWrt here:
https://dev.openwrt.org/browser/trunk/target/linux/bcm53xx/patches-4.1/131-ARM-BCM5301X-Implement-SMP-support.patch
It looks very similar to your approach so I assume we can use the same SMP code for NS and NSP. I will look into your patches later on.
You are right, we can use the common SMP code for NS and NSP, however, I am only concerned about the secondary_startup() function in case of NS as is being discussed in the e-mail chain.
As there are multiple versions of NS SoC, and AFAIK, some of those have anomalous BOOTROM. We should have a cleaner generic solution which can be used for both NS and NSP.
Quoted 139 lines
Hauke
Quoted 135 lines
Quoted 42 lines · Hauke Mehrtens
Quoted 39 lines
Hi,
I tested this patch on my device now.
What does the loader do before Linux gets started on the second CPU and what is ensured?
Quoted 31 lines · Russell King - ARM Linux
Quoted 14 lines · Rafa? Mi?ecki
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +14
-0+/* + * BCM5301X specific entry point for secondary CPUs. + */ +ENTRY(bcm5301x_secondary_startup) + mrc p15, 0, r0, c0, c0, 5 + and r0, r0, #15 + adr r4, 1f + ldmia r4, {r5, r6} + sub r4, r4, r5 + add r6, r6, r4 +pen: ldr r7, [r6] + cmp r7, r0 + bne pen +arch/arm/mach-bcm/bcm5301x_headsmp.S line 34
Quoted 10 lines
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +10
-0+ /* + * In case L1 cache has unpredictable contents at power-up + * clean its contents without flushing. + */ + bl v7_invalidate_l1 + + mov r0, #0 + mcr p15, 0, r0, c7, c5, 0 /* Invalidate icache */ + dsb + isbSo if your I-cache contains unpredictable contents, how do you execute the code to this point? Shouldn't the I-cache invalidate be the very first instruction you execute followed by the dsb and isb (oh, and iirc it ignores the value in the register).
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
In the case where a CPU has unpredictable contents at power up, the ARM ARM requires that an implementation specific sequence is followed to initialise the caches. I doubt that such a sequence includes testing a pen value.
Are you sure this is an issue of unpredictable L1 cache contents at power-up? AFAIK, 5301X had an issue with secondary core initialization. Secondary core which waits on WFE would let it out of the pen as soon as the first spin_*lock executes. This was because of a BOOTROM bug in NS, so the work around was to reset the address for the secondary processor to go back and wait for the signal from the primary core. This vector fixup is required so that the secondary core doesn't start executing kernel instructions until we've patched its jump address during
wakeup_secondary().Also, v7 setup function should invalidate L1 cache and we should remove all
v7_invalidate_l1calls in allheadsmp.Sin platform specific directories.Quoted 68 lines
Quoted 68 lines
When I remove the test for the pen value the CPU does not come up any more, I get this log output:
[ 0.132292] CPU: Testing write buffer coherency: ok [ 0.137635] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000 [ 0.143675] Setting up static identity map for 0x82a0 - 0x82d4 [ 10.149786] CPU1: failed to boot: -38 [ 10.153651] Brought up 1 CPUsThis was caused just by removing the "cmp r7, r0" and "bne pen" instructions.
With these instructions are added it works and I get this:
[ 0.132329] CPU: Testing write buffer coherency: ok [ 0.137682] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000 [ 0.143708] Setting up static identity map for 0x82a0 - 0x82d4 [ 0.189788] CPU1: thread -1, cpu 1, socket 0, mpidr 80000001 [ 0.189892] Brought up 2 CPUs [ 0.198889] SMP: Total of 2 processors activated (3188.32 BogoMIPS).Currently this is 100% reproducible.
Could it be that the second CPU needs some time till it is synchronised correctly?
I do not know if and why the cache clearing is needed, I do not have access to the SoC documentation or the ASIC/firmware developer.
Quoted 35 lines
arch/arm/mach-bcm/bcm5301x_smp.c line 49
Quoted 10 lines
arch/arm/mach-bcm/bcm5301x_smp.c new file +9
-0+ sysram_base_addr =of_iomap (node,0 ); +if (!sysram_base_addr) { +pr_warn ("Failed to map sysram\n" ); +return ; + } + +writel (virt_to_phys (entry_point), sysram_base_addr +SOC_ROM_LUT_OFF ); + +dsb_sev ();/* Exit WFI */ Which WFI? This seems to imply that you have some kind of initial firmware. If so, that should be taking care of the cache initialisation, not the kernel.
arch/arm/mach-bcm/bcm5301x_smp.c line 50
Quoted 1 line
arch/arm/mach-bcm/bcm5301x_smp.c new file +1
-0+mb ();/* make sure write buffer is drained */
writel()already ensures that.arch/arm/mach-bcm/bcm5301x_smp.c line 118
Quoted 12 lines
arch/arm/mach-bcm/bcm5301x_smp.c new file +12
-0+/* +* The secondary processor is waiting to be released from +* the holding pen - release it, then wait for it to flag +* that it has been released by resetting pen_release. +* +* Note that "pen_release" is the hardware CPU ID, whereas +* "cpu" is Linux's internal ID. +*/ +write_pen_release (cpu_logical_map (cpu)); + +/* Send the secondary CPU SEV */ +dsb_sev ();If you even need any of the pen code, if you're having to send a SEV here, wouldn't having a WFE in the pen assembly loop above be a good idea?
Quoted 4 lines
Quoted 4 lines
I have to read more on how WFE and co works.
Hauke
Thanks,
Kapil
Thanks,
Kapil
PATCH v2 1/1 ARM: BCM5301X: Implement SMP support
In reply to Hauke MehrtensQuoted 15 lines · Hauke Mehrtens
Quoted 5 lines · Kapil Hali
Quoted 2 lines · Ray Jui
+ bcm-kernel-feedback-list.
Quoted 8 lines
Quoted 2 lines
Kapil, you might want to take a look at this. Not sure how this is related to your SMP patches for NSP.
Ray, I don't have complete/other patch sets for this change. It would be good if I get those patch sets as well or complete e-mail thread. I think if we have a cleaner solutions for SMP, we can consolidate the change required for NS and NSP. I have few points to add, which are inline in this e-mail.
Quoted 8 lines
Hi Kapil,
the patch was posted on the arm mailing list:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/332690.html
but you can also find it in OpenWrt here:
https://dev.openwrt.org/browser/trunk/target/linux/bcm53xx/patches-4.1/131-ARM-BCM5301X-Implement-SMP-support.patch
It looks very similar to your approach so I assume we can use the same SMP code for NS and NSP. I will look into your patches later on.
Hello Hauke,
With my last patch (https://lkml.org/lkml/2015/10/15/690), SMP on 4708 is working with the NSP SMP patch
(https://lkml.org/lkml/2015/10/14/769). That patch does not have the issues that Russell King was mentioning in this patch series (and supports more SoCs). Have you had a chance to verify that it works for you?
Thanks,
Jon
Quoted 139 lines
Hauke
Quoted 135 lines
Quoted 42 lines · Hauke Mehrtens
Quoted 39 lines
Hi,
I tested this patch on my device now.
What does the loader do before Linux gets started on the second CPU and what is ensured?
Quoted 31 lines · Russell King - ARM Linux
Quoted 14 lines · Rafa? Mi?ecki
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +14
-0+/* + * BCM5301X specific entry point for secondary CPUs. + */ +ENTRY(bcm5301x_secondary_startup) + mrc p15, 0, r0, c0, c0, 5 + and r0, r0, #15 + adr r4, 1f + ldmia r4, {r5, r6} + sub r4, r4, r5 + add r6, r6, r4 +pen: ldr r7, [r6] + cmp r7, r0 + bne pen +arch/arm/mach-bcm/bcm5301x_headsmp.S line 34
Quoted 10 lines
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +10
-0+ /* + * In case L1 cache has unpredictable contents at power-up + * clean its contents without flushing. + */ + bl v7_invalidate_l1 + + mov r0, #0 + mcr p15, 0, r0, c7, c5, 0 /* Invalidate icache */ + dsb + isbSo if your I-cache contains unpredictable contents, how do you execute the code to this point? Shouldn't the I-cache invalidate be the very first instruction you execute followed by the dsb and isb (oh, and iirc it ignores the value in the register).
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
In the case where a CPU has unpredictable contents at power up, the ARM ARM requires that an implementation specific sequence is followed to initialise the caches. I doubt that such a sequence includes testing a pen value.
Are you sure this is an issue of unpredictable L1 cache contents at power-up? AFAIK, 5301X had an issue with secondary core initialization. Secondary core which waits on WFE would let it out of the pen as soon as the first spin_*lock executes. This was because of a BOOTROM bug in NS, so the work around was to reset the address for the secondary processor to go back and wait for the signal from the primary core. This vector fixup is required so that the secondary core doesn't start executing kernel instructions until we've patched its jump address during
wakeup_secondary().Also, v7 setup function should invalidate L1 cache and we should remove all
v7_invalidate_l1calls in allheadsmp.Sin platform specific directories.Quoted 68 lines
Quoted 68 lines
When I remove the test for the pen value the CPU does not come up any more, I get this log output:
[ 0.132292] CPU: Testing write buffer coherency: ok [ 0.137635] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000 [ 0.143675] Setting up static identity map for 0x82a0 - 0x82d4 [ 10.149786] CPU1: failed to boot: -38 [ 10.153651] Brought up 1 CPUsThis was caused just by removing the "cmp r7, r0" and "bne pen" instructions.
With these instructions are added it works and I get this:
[ 0.132329] CPU: Testing write buffer coherency: ok [ 0.137682] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000 [ 0.143708] Setting up static identity map for 0x82a0 - 0x82d4 [ 0.189788] CPU1: thread -1, cpu 1, socket 0, mpidr 80000001 [ 0.189892] Brought up 2 CPUs [ 0.198889] SMP: Total of 2 processors activated (3188.32 BogoMIPS).Currently this is 100% reproducible.
Could it be that the second CPU needs some time till it is synchronised correctly?
I do not know if and why the cache clearing is needed, I do not have access to the SoC documentation or the ASIC/firmware developer.
Quoted 35 lines
arch/arm/mach-bcm/bcm5301x_smp.c line 49
Quoted 10 lines
arch/arm/mach-bcm/bcm5301x_smp.c new file +9
-0+ sysram_base_addr =of_iomap (node,0 ); +if (!sysram_base_addr) { +pr_warn ("Failed to map sysram\n" ); +return ; + } + +writel (virt_to_phys (entry_point), sysram_base_addr +SOC_ROM_LUT_OFF ); + +dsb_sev ();/* Exit WFI */ Which WFI? This seems to imply that you have some kind of initial firmware. If so, that should be taking care of the cache initialisation, not the kernel.
arch/arm/mach-bcm/bcm5301x_smp.c line 50
Quoted 1 line
arch/arm/mach-bcm/bcm5301x_smp.c new file +1
-0+mb ();/* make sure write buffer is drained */
writel()already ensures that.arch/arm/mach-bcm/bcm5301x_smp.c line 118
Quoted 12 lines
arch/arm/mach-bcm/bcm5301x_smp.c new file +12
-0+/* +* The secondary processor is waiting to be released from +* the holding pen - release it, then wait for it to flag +* that it has been released by resetting pen_release. +* +* Note that "pen_release" is the hardware CPU ID, whereas +* "cpu" is Linux's internal ID. +*/ +write_pen_release (cpu_logical_map (cpu)); + +/* Send the secondary CPU SEV */ +dsb_sev ();If you even need any of the pen code, if you're having to send a SEV here, wouldn't having a WFE in the pen assembly loop above be a good idea?
Quoted 4 lines
Quoted 4 lines
I have to read more on how WFE and co works.
Hauke
Thanks,
Kapil
PATCH v2 1/1 ARM: BCM5301X: Implement SMP support
Quoted 25 lines · Jon Mason
Quoted 15 lines · Hauke Mehrtens
Quoted 5 lines · Kapil Hali
Quoted 2 lines · Ray Jui
+ bcm-kernel-feedback-list.
Quoted 8 lines
Quoted 2 lines
Kapil, you might want to take a look at this. Not sure how this is related to your SMP patches for NSP.
Ray, I don't have complete/other patch sets for this change. It would be good if I get those patch sets as well or complete e-mail thread. I think if we have a cleaner solutions for SMP, we can consolidate the change required for NS and NSP. I have few points to add, which are inline in this e-mail.
Quoted 8 lines
Hi Kapil,
the patch was posted on the arm mailing list:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/332690.html
but you can also find it in OpenWrt here:
https://dev.openwrt.org/browser/trunk/target/linux/bcm53xx/patches-4.1/131-ARM-BCM5301X-Implement-SMP-support.patch
It looks very similar to your approach so I assume we can use the same SMP code for NS and NSP. I will look into your patches later on.
Quoted 7 lines
Hello Hauke,
With my last patch (https://lkml.org/lkml/2015/10/15/690), SMP on 4708 is working with the NSP SMP patch
(https://lkml.org/lkml/2015/10/14/769). That patch does not have the issues that Russell King was mentioning in this patch series (and supports more SoCs). Have you had a chance to verify that it works for you?
Yes I tried it and it works for me. I have already added it to OpenWrt:
https://dev.openwrt.org/changeset/47247
Hauke
Quoted 143 lines
Thanks,
JonQuoted 139 lines
Hauke
Quoted 135 lines
Quoted 42 lines · Hauke Mehrtens
Quoted 39 lines
Hi,
I tested this patch on my device now.
What does the loader do before Linux gets started on the second CPU and what is ensured?
Quoted 31 lines · Russell King - ARM Linux
Quoted 14 lines · Rafa? Mi?ecki
(quoted diff) +14
-0+/* + * BCM5301X specific entry point for secondary CPUs. + */ +ENTRY(bcm5301x_secondary_startup) + mrc p15, 0, r0, c0, c0, 5 + and r0, r0, #15 + adr r4, 1f + ldmia r4, {r5, r6} + sub r4, r4, r5 + add r6, r6, r4 +pen: ldr r7, [r6] + cmp r7, r0 + bne pen +Review
Quoted 10 lines
(quoted diff) +10
-0+ /* + * In case L1 cache has unpredictable contents at power-up + * clean its contents without flushing. + */ + bl v7_invalidate_l1 + + mov r0, #0 + mcr p15, 0, r0, c7, c5, 0 /* Invalidate icache */ + dsb + isbSo if your I-cache contains unpredictable contents, how do you execute the code to this point? Shouldn't the I-cache invalidate be the very first instruction you execute followed by the dsb and isb (oh, and iirc it ignores the value in the register).
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
In the case where a CPU has unpredictable contents at power up, the ARM ARM requires that an implementation specific sequence is followed to initialise the caches. I doubt that such a sequence includes testing a pen value.
Are you sure this is an issue of unpredictable L1 cache contents at power-up? AFAIK, 5301X had an issue with secondary core initialization. Secondary core which waits on WFE would let it out of the pen as soon as the first spin_*lock executes. This was because of a BOOTROM bug in NS, so the work around was to reset the address for the secondary processor to go back and wait for the signal from the primary core. This vector fixup is required so that the secondary core doesn't start executing kernel instructions until we've patched its jump address during
wakeup_secondary().Also, v7 setup function should invalidate L1 cache and we should remove all
v7_invalidate_l1calls in allheadsmp.Sin platform specific directories.Quoted 68 lines
Quoted 68 lines
When I remove the test for the pen value the CPU does not come up any more, I get this log output:
[ 0.132292] CPU: Testing write buffer coherency: ok [ 0.137635] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000 [ 0.143675] Setting up static identity map for 0x82a0 - 0x82d4 [ 10.149786] CPU1: failed to boot: -38 [ 10.153651] Brought up 1 CPUsThis was caused just by removing the "cmp r7, r0" and "bne pen" instructions.
With these instructions are added it works and I get this:
[ 0.132329] CPU: Testing write buffer coherency: ok [ 0.137682] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000 [ 0.143708] Setting up static identity map for 0x82a0 - 0x82d4 [ 0.189788] CPU1: thread -1, cpu 1, socket 0, mpidr 80000001 [ 0.189892] Brought up 2 CPUs [ 0.198889] SMP: Total of 2 processors activated (3188.32 BogoMIPS).Currently this is 100% reproducible.
Could it be that the second CPU needs some time till it is synchronised correctly?
I do not know if and why the cache clearing is needed, I do not have access to the SoC documentation or the ASIC/firmware developer.
Quoted 35 lines
Review
Quoted 10 lines
(quoted diff) +9
-0+ sysram_base_addr = of_iomap(node, 0); + if (!sysram_base_addr) { + pr_warn("Failed to map sysram\n"); + return; + } + + writel(virt_to_phys(entry_point), sysram_base_addr + SOC_ROM_LUT_OFF); + + dsb_sev(); /* Exit WFI */Which WFI? This seems to imply that you have some kind of initial firmware. If so, that should be taking care of the cache initialisation, not the kernel.
Review
Quoted 1 line
(quoted diff) +1
-0+ mb(); /* make sure write buffer is drained */
writel()already ensures that.Review
Quoted 12 lines
(quoted diff) +12
-0+ /* + * The secondary processor is waiting to be released from + * the holding pen - release it, then wait for it to flag + * that it has been released by resetting pen_release. + * + * Note that "pen_release" is the hardware CPU ID, whereas + * "cpu" is Linux's internal ID. + */ + write_pen_release(cpu_logical_map(cpu)); + + /* Send the secondary CPU SEV */ + dsb_sev();If you even need any of the pen code, if you're having to send a SEV here, wouldn't having a WFE in the pen assembly loop above be a good idea?
Quoted 4 lines
Quoted 4 lines
I have to read more on how WFE and co works.
Hauke
Thanks,
Kapil
PATCH v2 1/1 ARM: BCM5301X: Implement SMP support
In reply to Hauke MehrtensQuoted 43 lines · Hauke Mehrtens
Hi,
I tested this patch on my device now.
What does the loader do before Linux gets started on the second CPU and what is ensured?
Quoted 35 lines · Russell King - ARM Linux
Quoted 14 lines · Rafa? Mi?ecki
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +14
-0+/* + * BCM5301X specific entry point for secondary CPUs. + */ +ENTRY(bcm5301x_secondary_startup) + mrc p15, 0, r0, c0, c0, 5 + and r0, r0, #15 + adr r4, 1f + ldmia r4, {r5, r6} + sub r4, r4, r5 + add r6, r6, r4 +pen: ldr r7, [r6] + cmp r7, r0 + bne pen +arch/arm/mach-bcm/bcm5301x_headsmp.S line 34
Quoted 10 lines
arch/arm/mach-bcm/bcm5301x_headsmp.S new file +10
-0+ /* + * In case L1 cache has unpredictable contents at power-up + * clean its contents without flushing. + */ + bl v7_invalidate_l1 + + mov r0, #0 + mcr p15, 0, r0, c7, c5, 0 /* Invalidate icache */ + dsb + isbSo if your I-cache contains unpredictable contents, how do you execute the code to this point? Shouldn't the I-cache invalidate be the very first instruction you execute followed by the dsb and isb (oh, and iirc it ignores the value in the register).
In the case where a CPU has unpredictable contents at power up, the ARM ARM requires that an implementation specific sequence is followed to initialise the caches. I doubt that such a sequence includes testing a pen value.
Things have changed in this area: ARMv7 CPU support now conforms with other Linux CPU support, and we invalidate the caches prior to enabling them.
However, I still come back to the point I said above, which you have not addressed. If the L1 cache contains unpredictable contents at power up, how can you guarnatee that any code the CPU executes _is read by the CPU_ rather than the random contents of the L1 cache.
This isn't about the pen stuff. It's about whether you can execute any code reliably at this point.
There are several possibilities that the ARM ARM allows:
* the caches are reset at CPU reset and contain no valid or dirty lines
* the caches are not reset at CPU reset, but are not searched until the enable bits are set
* the caches are not reset at CPU reset, but are searched
The last case is the "special" case that requires a implementation specific code to initialise them safely - but anyone implementing that would be really silly to do so, so I doubt this is what you have.
So, I'd just get rid of that unnecessary cache flushing, especially as I've said above, the ARMv7 CPU entry has been fixed to invalidate caches, rather than flush dirty data (hence potentially random data in the second case above) out to memory.
Signature · 2 lines
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.