Re: [PATCH 5/5] powerpc/microwatt: Add SMP support

[Michael Ellerman <mpe@ellerman.id.au>]

Paul Mackerras <paulus@ozlabs.org> writes:
> On Wed, Jan 29, 2025 at 04:21:26PM +1000, Nicholas Piggin wrote:
>> On Wed Jan 29, 2025 at 8:55 AM AEST, Paul Mackerras wrote:
>> > This adds support for Microwatt systems with more than one core, and
>> > updates the device tree for a 2-core version.  (This does not prevent
>> > the kernel from running on a single-core system.)
>> >
>> > Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
>> 
>> Well, I'm impressed you added SMP :)
>> 
>> What happens with a 1 CPU system? Do we time out waiting for secondaries
>> and continue, or is there something more graceful?
>
> There's a field in the SYSCON register which tells you how many cores
> there are.  microwatt_init_smp() looks at that field and only starts
> the CPUs that are there.
>
> Oops, sorry, I see that I forgot to do 'git add' on
> arch/powerpc/platforms/microwatt/smp.c.  Here it is (I'll include it
> properly in v2, of course):
>
> // SPDX-License-Identifier: GPL-2.0-or-later
>
> /*
>  * SMP support functions for Microwatt
>  * Copyright 2025 Paul Mackerras <paulus@ozlabs.org>
>  */
>
> #include <linux/kernel.h>
> #include <linux/smp.h>
> #include <linux/io.h>
> #include <asm/early_ioremap.h>
> #include <asm/xics.h>
>
> #include "microwatt.h"
>
> static void __init microwatt_smp_probe(void)
> {
> 	xics_smp_probe();
> }
>
> static void microwatt_smp_setup_cpu(int cpu)
> {
> 	if (cpu != 0)
> 		xics_setup_cpu();
> }
>
> static struct smp_ops_t microwatt_smp_ops = {
> 	.probe		= microwatt_smp_probe,
> 	.message_pass	= NULL,		/* Use smp_muxed_ipi_message_pass */
> 	.kick_cpu	= smp_generic_kick_cpu,
> 	.setup_cpu	= microwatt_smp_setup_cpu,
> };
>
> /* XXX get from device tree */
> #define SYSCON_BASE	0xc0000000
>
> #define SYSCON_CPU_CTRL	0x58
>
> void __init microwatt_init_smp(void)
> {
> 	volatile unsigned char __iomem *syscon;
> 	int ncpus;
> 	int timeout;
>
> 	syscon = early_ioremap(SYSCON_BASE, 0x100);
> 	if (syscon == NULL) {
> 		pr_err("Failed to map SYSCON\n");
> 		return;
> 	}
> 	ncpus = (readl(syscon + SYSCON_CPU_CTRL) >> 8) & 0xff;
> 	if (ncpus < 2)
> 		goto out;
>
> 	smp_ops = &microwatt_smp_ops;
>
> 	/*
> 	 * Write two instructions at location 0:
> 	 * mfspr r3, PIR
> 	 * b __secondary_hold
> 	 */
> 	*(unsigned int *)KERNELBASE = 0x7c7ffaa6;
> 	*(unsigned int *)(KERNELBASE+4) = 0x4800005c;
 
There's macros that would make these a little nicer, ie. PPC_RAW_MFSPR()
and PPC_RAW_BRANCH().

> 	/* enable the other CPUs, they start at location 0 */
> 	writel((1ul << ncpus) - 1, syscon + SYSCON_CPU_CTRL);
>
> 	timeout = 10000;
> 	while (!__secondary_hold_acknowledge) {
> 		if (--timeout == 0)
> 			break;
> 		barrier();
> 	}

I assume CPU 0 always boots first?

Is the loop actually necessary? It only waits for a single non-zero CPU
to come up after all, not all of them.

cheers