2011-04-03 21:01:05

by John Linn

[permalink] [raw]
Subject: [PATCH 2/3] ARM: Xilinx: add SMP specific support files

These files are the core processing for SMP which are similar
to most other platforms SMP. The biggest difference is the
way the 2nd CPU is started.

Signed-off-by: John Linn <[email protected]>
---
arch/arm/mach-xilinx/common.h | 2 +
arch/arm/mach-xilinx/headsmp.S | 59 ++++++++++++
arch/arm/mach-xilinx/include/mach/smp.h | 30 ++++++
arch/arm/mach-xilinx/platsmp.c | 159 +++++++++++++++++++++++++++++++
4 files changed, 250 insertions(+), 0 deletions(-)
create mode 100644 arch/arm/mach-xilinx/headsmp.S
create mode 100644 arch/arm/mach-xilinx/include/mach/smp.h
create mode 100644 arch/arm/mach-xilinx/platsmp.c

diff --git a/arch/arm/mach-xilinx/common.h b/arch/arm/mach-xilinx/common.h
index 71f4ebc..ecd8d65 100644
--- a/arch/arm/mach-xilinx/common.h
+++ b/arch/arm/mach-xilinx/common.h
@@ -25,6 +25,8 @@ void __init xilinx_system_init(void);
void __init xilinx_irq_init(void);
void __init xilinx_map_io(void);

+void xilinx_secondary_startup(void);
+
extern struct sys_timer xttcpss_sys_timer;

#endif
diff --git a/arch/arm/mach-xilinx/headsmp.S b/arch/arm/mach-xilinx/headsmp.S
new file mode 100644
index 0000000..bdf9a16
--- /dev/null
+++ b/arch/arm/mach-xilinx/headsmp.S
@@ -0,0 +1,59 @@
+/*
+ * arch/arm/mach-xilinx/headsmp.S
+ *
+ * Secondary CPU startup routine source file.
+ *
+ * Copyright (C) 2011 Xilinx, Inc.
+ *
+ * This file is based on arm omap smp platform file and arm
+ * realview smp platform.
+ *
+ * Copyright (C) 2009 Texas Instruments, Inc.
+ * Copyright (c) 2003 ARM Limited.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/linkage.h>
+#include <linux/init.h>
+#include <mach/xilinx_soc.h>
+#include <mach/io.h>
+
+ __INIT
+
+/*
+ * Xilinx specific entry point for secondary CPU to jump from ROM
+ * code. This routine provides a wait loop in which secondary core
+ * is held until we're ready for it to initialise. The primary core
+ * will update the boot lock with the boot key when it's ready for
+ * the secondary CPU to boot.
+ *
+ * The WFE must be in the loop when using this code with a probe
+ * because any operations cause debug events which can wake up CPU1
+ * falsely.
+ */
+ENTRY(xilinx_secondary_startup)
+ mov r0, #0
+ ldr r1, =(OCM_HIGH_PHYS + BOOT_LOCK_OFFSET)
+ ldr r0, =BOOT_LOCK_KEY
+ mov r3, #0
+hold:
+ wfe
+ add r3, #1 @ only track number of wakeups
+ @ for diagnostics
+ ldr r2, [r1]
+ cmp r2, r0
+ bne hold
+
+ /*
+ * CPU0 released CPU1 by writing the key to the lock, go ahead
+ * and start CPU1 running the kernel
+ */
+ b secondary_startup
diff --git a/arch/arm/mach-xilinx/include/mach/smp.h b/arch/arm/mach-xilinx/include/mach/smp.h
new file mode 100644
index 0000000..c3aae15
--- /dev/null
+++ b/arch/arm/mach-xilinx/include/mach/smp.h
@@ -0,0 +1,30 @@
+/* arch/arm/mach-xilinx/include/mach/smp.h
+ *
+ * Copyright (C) 2011 Xilinx
+ *
+ * based on arch/arm/mach-realview/include/mach/smp.h
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __MACH_SMP_H__
+#define __MACH_SMP_H__
+
+#include <asm/hardware/gic.h>
+
+/*
+ * We use IRQ1 as the IPI
+ */
+static inline void smp_cross_call(const struct cpumask *mask, int ipi)
+{
+ gic_raise_softirq(mask, ipi);
+}
+
+#endif
diff --git a/arch/arm/mach-xilinx/platsmp.c b/arch/arm/mach-xilinx/platsmp.c
new file mode 100644
index 0000000..be2d55c
--- /dev/null
+++ b/arch/arm/mach-xilinx/platsmp.c
@@ -0,0 +1,159 @@
+/*
+ * arch/arm/mach-xilinx/platsmp.c
+ *
+ * This file contains Xilinx specific SMP code, used to start up
+ * the second processor.
+ *
+ * Copyright (C) 2011 Xilinx
+ *
+ * based on linux/arch/arm/mach-realview/platsmp.c
+ *
+ * Copyright (C) 2002 ARM Ltd.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+#include <linux/jiffies.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <asm/cacheflush.h>
+#include <asm/smp_scu.h>
+#include <mach/xilinx_soc.h>
+#include <mach/smp.h>
+#include "common.h"
+
+static void __iomem *scu_base = SCU_PERIPH_BASE;
+
+static DEFINE_SPINLOCK(boot_lock);
+
+/* Secondary CPU kernel startup is a 2 phase process.
+ * 1st phase is transition from a boot loader to the kernel, but
+ * then wait not starting the kernel yet. 2nd phase starts the
+ * the kernel. In both phases, the secondary CPU waits for an
+ * event before it continues.
+ */
+
+void __cpuinit platform_secondary_init(unsigned int cpu)
+{
+ /*
+ * if any interrupts are already enabled for the primary
+ * core (e.g. timer irq), then they will not have been enabled
+ * for us: do so
+ */
+ gic_secondary_init(0);
+
+ /*
+ * Synchronise with the boot thread.
+ */
+ spin_lock(&boot_lock);
+ spin_unlock(&boot_lock);
+}
+
+int __cpuinit 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);
+
+ printk(KERN_INFO "Xilinx SMP: booting CPU1 now\n");
+
+ /*
+ * Update boot lock register with the boot key to allow the
+ * secondary processor to start the kernel after an SEV.
+ */
+ __raw_writel(BOOT_LOCK_KEY, OCM_HIGH_BASE + BOOT_LOCK_OFFSET);
+
+ /* Flush the kernel cache to ensure that the page tables are
+ * available for the secondary CPU to use and make sure that
+ * the write buffer is drained before doing an SEV.
+ */
+ flush_cache_all();
+ smp_wmb();
+
+ /*
+ * Send an event to wake the secondary core from WFE state.
+ */
+ sev();
+
+ timeout = jiffies + (1 * HZ);
+ while (time_before(jiffies, timeout))
+ ;
+
+ /*
+ * now the secondary core is starting up let it run its
+ * calibrations, then wait for it to finish
+ */
+ spin_unlock(&boot_lock);
+
+ return 0;
+}
+
+/*
+ * Initialise the CPU possible map early - this describes the CPUs
+ * which may be present or become present in the system.
+ */
+void __init smp_init_cpus(void)
+{
+ unsigned int i, ncores;
+
+ ncores = scu_base ? scu_get_core_count(scu_base) : 1;
+
+ /* sanity check */
+ if (ncores > NR_CPUS) {
+ printk(KERN_WARNING
+ "Realview: no. of cores (%d) greater than configured "
+ "maximum of %d - clipping\n",
+ ncores, NR_CPUS);
+ ncores = NR_CPUS;
+ }
+
+ for (i = 0; i < ncores; i++)
+ set_cpu_possible(i, true);
+}
+
+void __init platform_smp_prepare_cpus(unsigned int max_cpus)
+{
+ int i;
+
+ /*
+ * Initialise the present map, which describes the set of CPUs
+ * actually populated at the present time.
+ */
+ for (i = 0; i < max_cpus; i++)
+ set_cpu_present(i, true);
+
+ scu_enable(scu_base);
+
+ /* Initialize the boot lock register to prevent CPU1 from
+ starting the kernel before CPU0 is ready for that.
+ */
+ __raw_writel(0, OCM_HIGH_BASE + BOOT_LOCK_OFFSET);
+
+ /*
+ * Write the address of secondary startup routine into the
+ * boot address. The secondary CPU will use this value
+ * to get into the kernel after it's awake from WFE state.
+ *
+ * Note the physical address is needed as the secondary CPU
+ * will not have the MMU on yet. A barrier is added to ensure
+ * that write buffer is drained.
+ */
+ __raw_writel(virt_to_phys(xilinx_secondary_startup),
+ OCM_HIGH_BASE + BOOT_ADDR_OFFSET);
+ smp_wmb();
+
+ /*
+ * Send an event to wake the secondary core from WFE state.
+ */
+ sev();
+}
--
1.5.4.7



This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.


2011-04-04 15:13:27

by Catalin Marinas

[permalink] [raw]
Subject: Re: [PATCH 2/3] ARM: Xilinx: add SMP specific support files

John,

A few suggestions on barriers below.

On 3 April 2011 22:00, John Linn <[email protected]> wrote:
> diff --git a/arch/arm/mach-xilinx/platsmp.c b/arch/arm/mach-xilinx/platsmp.c
> new file mode 100644
> index 0000000..be2d55c
> --- /dev/null
> +++ b/arch/arm/mach-xilinx/platsmp.c
...
> +int __cpuinit 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);
> +
> +       printk(KERN_INFO "Xilinx SMP: booting CPU1 now\n");
> +
> +       /*
> +        * Update boot lock register with the boot key to allow the
> +        * secondary processor to start the kernel after an SEV.
> +        */
> +       __raw_writel(BOOT_LOCK_KEY, OCM_HIGH_BASE + BOOT_LOCK_OFFSET);
> +
> +       /* Flush the kernel cache to ensure that the page tables are
> +        * available for the secondary CPU to use and make sure that
> +        * the write buffer is drained before doing an SEV.
> +        */
> +       flush_cache_all();
> +       smp_wmb();
> +
> +       /*
> +        * Send an event to wake the secondary core from WFE state.
> +        */
> +       sev();

You could flush the cache before writing the boot lock register in
case the secondary wakes up from WFE. You also need a wmb() rather
than the smp_wmb(). The latter only works in the inner shareable
domain but the secondary CPU hasn't initialised the MMU yet.

Something like below:

flush_cache_all();
__raw_writel(BOOT_LOCK_KEY, OCM_HIGH_BASE + BOOT_LOCK_OFFSET);
mb();
sev();

> +void __init platform_smp_prepare_cpus(unsigned int max_cpus)
> +{
> +       int i;
> +
> +       /*
> +        * Initialise the present map, which describes the set of CPUs
> +        * actually populated at the present time.
> +        */
> +       for (i = 0; i < max_cpus; i++)
> +               set_cpu_present(i, true);
> +
> +       scu_enable(scu_base);
> +
> +       /* Initialize the boot lock register to prevent CPU1 from
> +          starting the kernel before CPU0 is ready for that.
> +       */
> +       __raw_writel(0, OCM_HIGH_BASE + BOOT_LOCK_OFFSET);
> +
> +       /*
> +        * Write the address of secondary startup routine into the
> +        * boot address. The secondary CPU will use this value
> +        * to get into the kernel after it's awake from WFE state.
> +        *
> +        * Note the physical address is needed as the secondary CPU
> +        * will not have the MMU on yet. A barrier is added to ensure
> +        * that write buffer is drained.
> +        */
> +       __raw_writel(virt_to_phys(xilinx_secondary_startup),
> +                                       OCM_HIGH_BASE + BOOT_ADDR_OFFSET);
> +       smp_wmb();

Same here, use a wmb() or mb() before the sev().

--
Catalin

2011-04-04 16:38:21

by John Linn

[permalink] [raw]
Subject: RE: [PATCH 2/3] ARM: Xilinx: add SMP specific support files

> -----Original Message-----
> From: [email protected] [mailto:[email protected]] On
> Behalf Of Catalin Marinas
> Sent: Monday, April 04, 2011 9:13 AM
> To: John Linn
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH 2/3] ARM: Xilinx: add SMP specific support files
>
> John,
>
> A few suggestions on barriers below.
>
> On 3 April 2011 22:00, John Linn <[email protected]> wrote:
> > diff --git a/arch/arm/mach-xilinx/platsmp.c b/arch/arm/mach-
> xilinx/platsmp.c
> > new file mode 100644
> > index 0000000..be2d55c
> > --- /dev/null
> > +++ b/arch/arm/mach-xilinx/platsmp.c
> ...
> > +int __cpuinit 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);
> > +
> > +       printk(KERN_INFO "Xilinx SMP: booting CPU1 now\n");
> > +
> > +       /*
> > +        * Update boot lock register with the boot key to allow the
> > +        * secondary processor to start the kernel after an SEV.
> > +        */
> > +       __raw_writel(BOOT_LOCK_KEY, OCM_HIGH_BASE +
> BOOT_LOCK_OFFSET);
> > +
> > +       /* Flush the kernel cache to ensure that the page tables are
> > +        * available for the secondary CPU to use and make sure that
> > +        * the write buffer is drained before doing an SEV.
> > +        */
> > +       flush_cache_all();
> > +       smp_wmb();
> > +
> > +       /*
> > +        * Send an event to wake the secondary core from WFE state.
> > +        */
> > +       sev();
>
> You could flush the cache before writing the boot lock register in
> case the secondary wakes up from WFE.

Understood. Since we control when it gets into the kernel completely I guess I wasn't worried about it, maybe I should have been. Easy enough to change, not a big deal.

> You also need a wmb() rather
> than the smp_wmb(). The latter only works in the inner shareable
> domain but the secondary CPU hasn't initialised the MMU yet.

Ok, great, appreciate the help there.

>
> Something like below:
>
> flush_cache_all();
> __raw_writel(BOOT_LOCK_KEY, OCM_HIGH_BASE + BOOT_LOCK_OFFSET);
> mb();
> sev();
>
> > +void __init platform_smp_prepare_cpus(unsigned int max_cpus)
> > +{
> > +       int i;
> > +
> > +       /*
> > +        * Initialise the present map, which describes the set of
> CPUs
> > +        * actually populated at the present time.
> > +        */
> > +       for (i = 0; i < max_cpus; i++)
> > +               set_cpu_present(i, true);
> > +
> > +       scu_enable(scu_base);
> > +
> > +       /* Initialize the boot lock register to prevent CPU1 from
> > +          starting the kernel before CPU0 is ready for that.
> > +       */
> > +       __raw_writel(0, OCM_HIGH_BASE + BOOT_LOCK_OFFSET);
> > +
> > +       /*
> > +        * Write the address of secondary startup routine into the
> > +        * boot address. The secondary CPU will use this value
> > +        * to get into the kernel after it's awake from WFE state.
> > +        *
> > +        * Note the physical address is needed as the secondary CPU
> > +        * will not have the MMU on yet. A barrier is added to ensure
> > +        * that write buffer is drained.
> > +        */
> > +       __raw_writel(virt_to_phys(xilinx_secondary_startup),
> > +                                       OCM_HIGH_BASE +
> BOOT_ADDR_OFFSET);
> > +       smp_wmb();
>
> Same here, use a wmb() or mb() before the sev().
>

Yes.

Thanks for the input and taking time to review. I'll wait a bit to see if any
other input before spinning a new patch set.

-- John

> --
> Catalin


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?

2011-04-04 16:50:31

by Russell King - ARM Linux

[permalink] [raw]
Subject: Re: [PATCH 2/3] ARM: Xilinx: add SMP specific support files

On Sun, Apr 03, 2011 at 03:00:17PM -0600, John Linn wrote:
> +#include <linux/jiffies.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <asm/cacheflush.h>
> +#include <asm/smp_scu.h>
> +#include <mach/xilinx_soc.h>
> +#include <mach/smp.h>
> +#include "common.h"
> +
> +static void __iomem *scu_base = SCU_PERIPH_BASE;

You don't need this if its known at build time.

> +
> +static DEFINE_SPINLOCK(boot_lock);
> +
> +/* Secondary CPU kernel startup is a 2 phase process.
> + * 1st phase is transition from a boot loader to the kernel, but
> + * then wait not starting the kernel yet. 2nd phase starts the
> + * the kernel. In both phases, the secondary CPU waits for an
> + * event before it continues.
> + */
> +
> +void __cpuinit platform_secondary_init(unsigned int cpu)
> +{
> + /*
> + * if any interrupts are already enabled for the primary
> + * core (e.g. timer irq), then they will not have been enabled
> + * for us: do so
> + */
> + gic_secondary_init(0);
> +
> + /*
> + * Synchronise with the boot thread.
> + */
> + spin_lock(&boot_lock);
> + spin_unlock(&boot_lock);
> +}
> +
> +int __cpuinit 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);
> +
> + printk(KERN_INFO "Xilinx SMP: booting CPU1 now\n");

Aren't the messages which are already printed enough?

> +
> + /*
> + * Update boot lock register with the boot key to allow the
> + * secondary processor to start the kernel after an SEV.
> + */
> + __raw_writel(BOOT_LOCK_KEY, OCM_HIGH_BASE + BOOT_LOCK_OFFSET);
> +
> + /* Flush the kernel cache to ensure that the page tables are
> + * available for the secondary CPU to use and make sure that
> + * the write buffer is drained before doing an SEV.
> + */
> + flush_cache_all();
> + smp_wmb();

The page tables and everything else required should already be visible
to the secondary CPU - if not that points to a bug in the generic code.
All that you should need to do here is to make sure your write above
is visible to the secondary CPU (and the address for it to boot from).

> +
> + /*
> + * Send an event to wake the secondary core from WFE state.
> + */
> + sev();
> +
> + timeout = jiffies + (1 * HZ);
> + while (time_before(jiffies, timeout))
> + ;

Have you no way to determine that the secondary CPU has started?

> +void __init smp_init_cpus(void)
> +{
> + unsigned int i, ncores;
> +
> + ncores = scu_base ? scu_get_core_count(scu_base) : 1;

unsigned int i, ncores = scu_get_core_count(SCU_PERIPH_BASE);

> +
> + /* sanity check */
> + if (ncores > NR_CPUS) {
> + printk(KERN_WARNING
> + "Realview: no. of cores (%d) greater than configured "
> + "maximum of %d - clipping\n",
> + ncores, NR_CPUS);
> + ncores = NR_CPUS;
> + }

Firstly, you're not Realview. Secondly, I think this kind of check should
be inside scu_get_core_count() if it has to exist.

> +
> + for (i = 0; i < ncores; i++)
> + set_cpu_possible(i, true);
> +}
> +
> +void __init platform_smp_prepare_cpus(unsigned int max_cpus)
> +{
> + int i;
> +
> + /*
> + * Initialise the present map, which describes the set of CPUs
> + * actually populated at the present time.
> + */
> + for (i = 0; i < max_cpus; i++)
> + set_cpu_present(i, true);
> +
> + scu_enable(scu_base);
> +
> + /* Initialize the boot lock register to prevent CPU1 from
> + starting the kernel before CPU0 is ready for that.
> + */
> + __raw_writel(0, OCM_HIGH_BASE + BOOT_LOCK_OFFSET);
> +
> + /*
> + * Write the address of secondary startup routine into the
> + * boot address. The secondary CPU will use this value
> + * to get into the kernel after it's awake from WFE state.
> + *
> + * Note the physical address is needed as the secondary CPU
> + * will not have the MMU on yet. A barrier is added to ensure
> + * that write buffer is drained.
> + */
> + __raw_writel(virt_to_phys(xilinx_secondary_startup),
> + OCM_HIGH_BASE + BOOT_ADDR_OFFSET);
> + smp_wmb();
> +
> + /*
> + * Send an event to wake the secondary core from WFE state.
> + */
> + sev();

This looks like it shouldn't be necessary - you send an event in the
boot_secondary() path which should be enough to get the CPU going.

2011-04-04 17:11:25

by John Linn

[permalink] [raw]
Subject: RE: [PATCH 2/3] ARM: Xilinx: add SMP specific support files

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:[email protected]]
> Sent: Monday, April 04, 2011 10:50 AM
> To: John Linn
> Cc: [email protected];
[email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH 2/3] ARM: Xilinx: add SMP specific support files
>
> On Sun, Apr 03, 2011 at 03:00:17PM -0600, John Linn wrote:
> > +#include <linux/jiffies.h>
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <asm/cacheflush.h>
> > +#include <asm/smp_scu.h>
> > +#include <mach/xilinx_soc.h>
> > +#include <mach/smp.h>
> > +#include "common.h"
> > +
> > +static void __iomem *scu_base = SCU_PERIPH_BASE;
>
> You don't need this if its known at build time.

Ok.

>
> > +
> > +static DEFINE_SPINLOCK(boot_lock);
> > +
> > +/* Secondary CPU kernel startup is a 2 phase process.
> > + * 1st phase is transition from a boot loader to the kernel, but
> > + * then wait not starting the kernel yet. 2nd phase starts the
> > + * the kernel. In both phases, the secondary CPU waits for an
> > + * event before it continues.
> > + */
> > +
> > +void __cpuinit platform_secondary_init(unsigned int cpu)
> > +{
> > + /*
> > + * if any interrupts are already enabled for the primary
> > + * core (e.g. timer irq), then they will not have been enabled
> > + * for us: do so
> > + */
> > + gic_secondary_init(0);
> > +
> > + /*
> > + * Synchronise with the boot thread.
> > + */
> > + spin_lock(&boot_lock);
> > + spin_unlock(&boot_lock);
> > +}
> > +
> > +int __cpuinit 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);
> > +
> > + printk(KERN_INFO "Xilinx SMP: booting CPU1 now\n");
>
> Aren't the messages which are already printed enough?

This was helpful, but can be deleted too, not a big deal.

>
> > +
> > + /*
> > + * Update boot lock register with the boot key to allow the
> > + * secondary processor to start the kernel after an SEV.
> > + */
> > + __raw_writel(BOOT_LOCK_KEY, OCM_HIGH_BASE + BOOT_LOCK_OFFSET);
> > +
> > + /* Flush the kernel cache to ensure that the page tables are
> > + * available for the secondary CPU to use and make sure that
> > + * the write buffer is drained before doing an SEV.
> > + */
> > + flush_cache_all();
> > + smp_wmb();
>
> The page tables and everything else required should already be visible
> to the secondary CPU - if not that points to a bug in the generic
code.
> All that you should need to do here is to make sure your write above
> is visible to the secondary CPU (and the address for it to boot from).

I didn't have the flush in early during debug and found it was needed.
I haven't tried it for quite some time without, I'll give it a try as
maybe some generic code was updated to fix a problem since then.

I noticed it in other platforms was the reason I thought it was really
needed.

>
> > +
> > + /*
> > + * Send an event to wake the secondary core from WFE state.
> > + */
> > + sev();
> > +
> > + timeout = jiffies + (1 * HZ);
> > + while (time_before(jiffies, timeout))
> > + ;
>
> Have you no way to determine that the secondary CPU has started?

Not right now. I can look at the ability to handshake back so that
it's not based on time. I'll to see what some other platforms do with
that.

I can see that it would help boot time not to do it this way.

It was based it off of some platforms doing it like this and it may
not have been the best pattern to use.

>
> > +void __init smp_init_cpus(void)
> > +{
> > + unsigned int i, ncores;
> > +
> > + ncores = scu_base ? scu_get_core_count(scu_base) : 1;
>
> unsigned int i, ncores = scu_get_core_count(SCU_PERIPH_BASE);
>
> > +
> > + /* sanity check */
> > + if (ncores > NR_CPUS) {
> > + printk(KERN_WARNING
> > + "Realview: no. of cores (%d) greater than
configured
> "
> > + "maximum of %d - clipping\n",
> > + ncores, NR_CPUS);
> > + ncores = NR_CPUS;
> > + }
>
> Firstly, you're not Realview. Secondly, I think this kind of check
> should
> be inside scu_get_core_count() if it has to exist.
>

I should have caught that. Ok it can be moved inside.

The NR_CPUS thing is not real clear to me so I'll have to investigate
more as we
only have 2 CPUs, yet the default in the kernel configs is generally 4.

I know there has been a big deal about defconfigs so I was trying not to
add more
problems by adding one for Xilinx platform, but maybe I have
misunderstood that issue.

> > +
> > + for (i = 0; i < ncores; i++)
> > + set_cpu_possible(i, true);
> > +}
> > +
> > +void __init platform_smp_prepare_cpus(unsigned int max_cpus)
> > +{
> > + int i;
> > +
> > + /*
> > + * Initialise the present map, which describes the set of CPUs
> > + * actually populated at the present time.
> > + */
> > + for (i = 0; i < max_cpus; i++)
> > + set_cpu_present(i, true);
> > +
> > + scu_enable(scu_base);
> > +
> > + /* Initialize the boot lock register to prevent CPU1 from
> > + starting the kernel before CPU0 is ready for that.
> > + */
> > + __raw_writel(0, OCM_HIGH_BASE + BOOT_LOCK_OFFSET);
> > +
> > + /*
> > + * Write the address of secondary startup routine into the
> > + * boot address. The secondary CPU will use this value
> > + * to get into the kernel after it's awake from WFE state.
> > + *
> > + * Note the physical address is needed as the secondary CPU
> > + * will not have the MMU on yet. A barrier is added to ensure
> > + * that write buffer is drained.
> > + */
> > + __raw_writel(virt_to_phys(xilinx_secondary_startup),
> > + OCM_HIGH_BASE +
BOOT_ADDR_OFFSET);
> > + smp_wmb();
> > +
> > + /*
> > + * Send an event to wake the secondary core from WFE state.
> > + */
> > + sev();
>
> This looks like it shouldn't be necessary - you send an event in the
> boot_secondary() path which should be enough to get the CPU going.

It's a 2 stage process, the 1st SEV gets the 2nd CPU out of the boot rom
and into the kernel, the 2nd SEV lets it start running the kernel.

At some point this was based on a similar pattern that I saw in other
platforms.

Thanks for your time and input,
John

This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.