Re: [PATCH] xen/arm: midway: implement SMP

[Ian Campbell <ian.campbell@citrix.com>]

On Tue, 2013-11-19 at 16:24 +0100, Andre Przywara wrote:
> On 11/18/2013 02:27 PM, Julien Grall wrote:
> > Signed-off-by: Julien Grall <julien.grall@linaro.org>
> > ---
> >   xen/arch/arm/platforms/midway.c | 22 ++++++++++++++++++++++
> >   1 file changed, 22 insertions(+)
> >
> > diff --git a/xen/arch/arm/platforms/midway.c b/xen/arch/arm/platforms/midway.c
> > index 399056b..3b9fcfc 100644
> > --- a/xen/arch/arm/platforms/midway.c
> > +++ b/xen/arch/arm/platforms/midway.c
> > @@ -47,6 +47,27 @@ static uint32_t midway_quirks(void)
> >       return PLATFORM_QUIRK_DOM0_MAPPING_11;
> >   }
> >
> > +static int __init midway_cpu_up(int cpu)
> 
> Wouldn't it make more sense to do this initialization in smp_init() 
> instead of here per CPU? init_secondary() is fixed and thus we will not 
> write different values for each core.
> I guess it does not really matter, I was just wondering whether this 
> would be a saner approach (and I think your first version was this way, 
> right?)

This means that we use the core codes CPU enumeration and don't need to
repeat it in smp_init.

> P.S. BTW: Any reason we instantiate cpu_up() for all platforms where 
> they are actually NOPs?

When I initially started building this infrastructure there was stuff
there, but it went away during development and so far it seems to have
turned out to be unnecessary. This is largely because on all the
platforms we supported so far the generic sev suffices to kick acpu out
of either the firmware or Xen's holding pen.

This hook will be needed for PSCI, hopefully sooner rather than later.

>  I think the platform code only calls a function 
> if it is non-NULL, so we could just skip all of this: cpu_up() {return 
> 0;} definition.

Could do.

> 
> > +{
> > +    void __iomem *pens;
> > +
> > +    pens = ioremap_nocache(0, PAGE_SIZE);
> > +    if ( !pens )
> > +    {
> > +        dprintk(XENLOG_ERR, "Unable to map midway pens MMIO\n");
> > +        return -EFAULT;
> > +    }
> > +
> > +    printk("Set cpu pen %u to %"PRIpaddr" (%p)\n",
> > +           cpu, __pa(init_secondary), init_secondary);
> > +
> > +    writel(__pa(init_secondary), pens + 0x40 + cpu * 0x10);
> > +
> > +    iounmap(pens);
> > +
> > +    return 0;
> > +}
> > +
> >   static const char * const midway_dt_compat[] __initconst =
> >   {
> >       "calxeda,ecx-2000",
> > @@ -57,6 +78,7 @@ PLATFORM_START(midway, "CALXEDA MIDWAY")
> >       .compatible = midway_dt_compat,
> >       .reset = midway_reset,
> >       .quirks = midway_quirks,
> > +    .cpu_up = midway_cpu_up,
> >   PLATFORM_END
> >
> >   /*
> >
>