Date: Tue, 29 Apr 2014 13:05:13 -0600 From: Ian Lepore <ian@FreeBSD.org> To: "Wojciech A. Koszek" <wkoszek@FreeBSD.org> Cc: freebsd-arm <freebsd-arm@FreeBSD.org>, Thomas Skibo <ThomasSkibo@sbcglobal.net> Subject: Re: SMP support for ZEDBOARD Message-ID: <1398798313.22079.31.camel@revolution.hippie.lan> In-Reply-To: <20140429180756.GP91461@FreeBSD.org> References: <535EEB12.2050704@sbcglobal.net> <20140429180756.GP91461@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1398798313.22079.31.camel>