Re: [Xen-devel] [Patch v2 1/1] Add Odroid-XU (Exynos 5410)

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Sat, 2014-07-26 at 23:17 +0100, Julien Grall wrote:
> > @@ -67,8 +68,19 @@ static int exynos5_specific_mapping(struct domain *d)
> >   static int __init exynos5_smp_init(void)
> >   {
> >       void __iomem *sysram;
> > +    unsigned long pa_sysram;
> >
> > -    sysram = ioremap_nocache(S5P_PA_SYSRAM, PAGE_SIZE);
> > +    if ( dt_machine_is_compatible(EXYNOS5250_COMPAT) )
> > +        pa_sysram = S5P_PA_SYSRAM;
> > +    else if ( dt_machine_is_compatible(EXYNOS5410_COMPAT) )
> > +        pa_sysram = EXYNOS5410_PA_SYSRAM;
> 
> I dislike this solution. We've introduced the platform API to avoid 
> checking the compatible string every time we call callbacks and make 
> them very simply.
> 
> Rather than if/else stuff I would prefer if you define a different 
> platform, and create a common function which will be called with the 
> right parameters.

I disagree. For platforms which are very similar and differ only in
small details a single platform and some small details a single platform
and a few compatible checks are IMHO a perfectly fine approach.

For example in this case the smp_init is identical apart from the load
address, and a single platform+callback and detecting the load address
via is_compatible is absolutely fine.

> On another side, Linux has introduced recently a new bindings to 
> retrieve the sysram from the device tree. See commit b3205dea
> "ARM: EXYNOS: Map SYSRAM through generic DT bindings" in the Linux 
> upstream tree.
> 
> I would definitely prefer to use this way on Xen to make simpler adding 
> a new exynos support. The drawback is we don't support older kernel on 
> Xen for those processors.

I think it would be reasonable to use SYSRAM if present but have a
fallback to something based on the compat string for backwards compat
(which I presume is what the kernel must do too).

> For odroid XU (aka exynos 5410) it's perfectly fine as we don't have yet 
> a support.
> For the exynos5250, I think it's arguable. I don't think this board is 
> used in production but we may want to keep binding compatibility support 
> with Xen 4.4. I will let the ARM maintainers decide on this point.
> 
> If we decide to keep compatibility, then I still would prefer a clean 
> support for Odroid Xu. Even if it's mean to rework the current exynos 
> platform code.
> 
> > +    else
> > +    {
> > +        dprintk(XENLOG_ERR, "Machine not compatible!\n");
> > +        return -EFAULT;
> > +    }

This can be a BUG() since the compat list only contains two options and
you've considered both of them.

> > +
> > +    sysram = ioremap_nocache(pa_sysram, PAGE_SIZE);
> >       if ( !sysram )
> >       {
> >           dprintk(XENLOG_ERR, "Unable to map exynos5 MMIO\n");
> > @@ -84,6 +96,48 @@ static int __init exynos5_smp_init(void)
> >       return 0;
> >   }
> >
> > +static int __init exynos5_cpu_up(int cpu)
> > +{
> > +    void __iomem *power;
> > +    char byte0, byte4;
> > +    int rc;
> > +
> > +    if ( dt_machine_is_compatible(EXYNOS5250_COMPAT) ) {
> > +        rc = cpu_up_send_sgi(cpu);
> > +        return rc;
> > +    }
> 
> Introducing 2 differents platform (one for the exynos5250 and the other 
> for the 5410) would permit to have a separate callbacks for the cpu up 
> and avoid again the if/else.

I'd be happy if this was structured:

        if ( dt_machine_is_compatible(....5410) )
        {
            /* EXYNOS5410: Power the secondary core */
                ...
        }

        return cpu_up_send_sgi(cpu);

I'm also not entirely sure about the use of the #defines for the compat
string, since everything is contained in this one file I don't think it
is strictly necessary.

As an aside I'd quite like to see xen/include/asm-arm/platforms/ getting
slowly removed in favour of local headers in xen/arch/arm/platforms or
even defines in the C files themselves, since this platform stuff should
be completely self contained these days and not of any interest to the
rest of the code. No need to shave that yakk in this patch though.

> 
> > +    /* EXYNOS5410: Power the secondary core */
> > +    power = ioremap_nocache(EXYNOS5410_POWER_CPU_BASE +
> > +                            cpu * EXYNOS5410_POWER_CPU_OFFSET, PAGE_SIZE);
> 
> Is it really exynos5410 specific? Linux upstream seems to have a generic 
> way to bring up secondary CPUs. It would be better if we have a similar 
> one. So bring up a new exynos platform will be more easier.

Certainly if this could be applied to all exynos (even the 5250) it
would be much preferable.

> 
> > +    if ( !power )
> > +    {
> > +        dprintk(XENLOG_ERR, "Unable to map exynos5410 MMIO\n");
> > +        return -EFAULT;
> > +    }
> > +
> > +    byte0 = readb(power);
> > +    byte4 = readb(power + 4);
> > +    dprintk(XENLOG_INFO, "Power: %x status: %x\n", byte0, byte4);
> > +
> > +    writeb(EXYNOS5410_POWER_ENABLE, power);
> > +    dprintk(XENLOG_INFO, "Waiting for power status to change to %x\n",
> > +            EXYNOS5410_POWER_ENABLE);
> > +
> > +    do
> > +    {
> > +        udelay(1);
> > +        byte4 = readb(power + 4);
> 
> Xen will go in an infinite loop, if the CPU has not been correctly 
> enabled. I think you should add a timeout there.

Yes, please.

If you can find more meaningful names than byte0 and byte4 that would be
good too.

> > @@ -122,7 +177,7 @@ PLATFORM_START(exynos5, "SAMSUNG EXYNOS5")
> >       .init_time = exynos5_init_time,
> >       .specific_mapping = exynos5_specific_mapping,
> 
> I'm not sure if those specific mapping are relevant for the odroid XU... 
> Hence IIRC, they are now defined in the device tree.

A difference in this *would* require a separate platform, since this is
only a list not a function.

Is it possible that the 52xx DTBs have caught up and these are no longer
required even there though?



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel