SMP support for ZEDBOARD

[Ian Lepore]

On Tue, 2014-04-29 at 18:07 +0000, Wojciech A. Koszek wrote:
> On Mon, Apr 28, 2014 at 04:58:10PM -0700, Thomas Skibo wrote:
> > 
> > Hi, All.
> > 
> > I continue to test FreeBSD on the Zedboard and SMP seems very solid. Can 
> > I get someone to commit SMP support to head (diffs attached)?
> > 
> > I also have some bug-fixes too, mainly for the ethernet driver.  Should 
> > I just file bugs and provide the patches?
> > 
> 
> Thomas
> 
> This certainly looks good. Small style nit below.
> 
> > Index: sys/arm/xilinx/zy7_mp.c
> > ===================================================================
> > --- /dev/null	2014-04-09 08:33:00.000000000 -0700
> > +++ sys/arm/xilinx/zy7_mp.c	2014-04-05 11:28:29.000000000 -0700
> > @@ -0,0 +1,98 @@
> > +/*-
> > + * Copyright (c) 2013 Thomas Skibo.  All rights reserved.
> > + *
> > + * Redistribution and use in source and binary forms, with or without
> > + * modification, are permitted provided that the following conditions
> > + * are met:
> > + * 1. Redistributions of source code must retain the above copyright
> > + *    notice, this list of conditions and the following disclaimer.
> > + * 2. Redistributions in binary form must reproduce the above copyright
> > + *    notice, this list of conditions and the following disclaimer in the
> > + *    documentation and/or other materials provided with the distribution.
> > + *
> > + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
> > + * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
> > + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
> > + * IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
> > + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
> > + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> > + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> > + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> > + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > + */
> > +
> > +#include <sys/cdefs.h>
> > +__FBSDID("$FreeBSD$");
> 
> You need \n here.
> 
> > +#include <sys/param.h>
> > +#include <sys/systm.h>
> > +#include <sys/bus.h>
> > +#include <sys/lock.h>
> > +#include <sys/mutex.h>
> > +#include <sys/smp.h>
> > +
> > +#include <machine/smp.h>
> > +#include <machine/fdt.h>
> > +#include <machine/intr.h>
> > +
> > +#include <arm/xilinx/zy7_reg.h>
> > +
> > +#define ZYNQ7_CPU1_ENTRY	0xfffffff0
> > +
> > +void
> > +platform_mp_init_secondary(void)
> > +{
> > +
> > +	gic_init_secondary();
> > +}
> > +
> > +void
> > +platform_mp_setmaxid(void)
> > +{
> > +
> > +     mp_maxid = 1;
> 
> You have spaces here instead of TAB for alignment.
> 
> > +}
> > +
> > +int
> > +platform_mp_probe(void)
> > +{
> > +
> > +	mp_ncpus = 2;
> 
> Why does ARM port of FreeBSD need that? Why isn't number of CPUs read from
> the SCU register? This method sounds redundant (at least across ARM Cortex
> family)
> 
> > +	return (1);
> > +}
> > +
> > +void    
> > +platform_mp_start_ap(void)
> > +{
> > +	bus_space_handle_t ocm_handle;
> > +
> > +	/* Map in magic location to give entry address to CPU1. */
> > +	if (bus_space_map(fdtbus_bs_tag, ZYNQ7_CPU1_ENTRY, 4,
> > +			  0, &ocm_handle) != 0)
> > +		panic("platform_mp_start_ap: Couldn't map OCM\n");
> > +
> > +	/* Write start address for CPU1. */
> > +	bus_space_write_4(fdtbus_bs_tag, ocm_handle, 0,
> > +			  pmap_kextract((vm_offset_t)mpentry));
> > +
> > +	/* The SCU is enabled by the BOOTROM but I think the second CPU
> 
> You probably need to start comments with:
> 
> 	/*
> 	 * ...
> 
> (missing \n)
> 
> > +	 * doesn't turn on filtering until after the wake-up below.
> > +	 * I think that's why things don't work if I don't put these
> > +	 * cache ops here.  Also, the magic location, 0xfffffff0, isn't
> > +	 * in the SCU's filtering range so it needs a write-back too.
> > +	 */
> > +	cpu_idcache_wbinv_all();
> > +	cpu_l2cache_wbinv_all();
> > +
> > +	/* Wake up CPU1. */
> > +	armv7_sev();
> > +
> > +	bus_space_unmap(fdtbus_bs_tag, ocm_handle, 4);
> > +}
> > +
> > +void
> > +platform_ipi_send(cpuset_t cpus, u_int ipi)
> > +{
> > +
> > +	pic_ipi_send(cpus, ipi);
> > +}
> > Index: sys/arm/conf/ZEDBOARD
> > ===================================================================
> > --- sys/arm/conf/ZEDBOARD	(revision 264766)
> > +++ sys/arm/conf/ZEDBOARD	(working copy)
> > @@ -56,6 +56,7 @@
> >  options 	_KPOSIX_PRIORITY_SCHEDULING # Posix P1003_1B real-time extensions
> >  options 	FREEBSD_BOOT_LOADER
> >  options 	VFP			# vfp/neon
> > +options		SMP			# Symmetric MultiProcessor Kernel
> 
> I think we have \t\t instead of just \t here.
> 
> >  
> >  # Debugging
> >  makeoptions	DEBUG=-g
> > Index: sys/arm/xilinx/files.zynq7
> > ===================================================================
> > --- sys/arm/xilinx/files.zynq7	(revision 264766)
> > +++ sys/arm/xilinx/files.zynq7	(working copy)
> > @@ -21,6 +21,7 @@
> >  arm/xilinx/zy7_bus_space.c			standard
> >  arm/xilinx/zy7_slcr.c				standard
> >  arm/xilinx/zy7_devcfg.c				standard
> > +arm/xilinx/zy7_mp.c				optional smp
> >  
> >  dev/cadence/if_cgem.c				optional if_cgem
> >  dev/sdhci/sdhci_fdt.c				optional sdhci
> > Index: sys/arm/xilinx/std.zynq7
> > ===================================================================
> > --- sys/arm/xilinx/std.zynq7	(revision 264766)
> > +++ sys/arm/xilinx/std.zynq7	(working copy)
> > @@ -20,3 +20,5 @@
> >  
> >  options		ARM_L2_PIPT
> >  
> > +options		IPI_IRQ_START=0
> > +options		IPI_IRQ_END=15
> > 
> 
> Run with older U-Boot, does it just work?
> 

I tidied up the whitespace nits you noted when I did the commit.

About the platform_mp_probe() and multicore startup code in general,
I've been thinking we should have generic armv7/mpcore code for doing
almost everything except the actual launching of the AP cores, which
does tend to be wildly different amongst the various SoCs.

-- Ian