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

[Julien Grall <julien.grall@linaro.org>]

Hi Suriyan,

Thank you for the patch.

On 26/07/14 01:02, Suriyan Ramasami wrote:
>     XEN/ARM: Add Odroid-XU support
>
>     The Odroid-Xu from hardkernel is an Exynos 5410 based board.
>     This patch addds support to the above said board.

We usually separate the commit message and the change log via "---" and 
a newline.

So when one of the committers will apply the patch, everything after the 
"---" will be stripped.

>     v2:
>        a. Set startup address in exynos5_smp_init() only once.
>        b. Turn on power to each core in exynos5_cpu_up().
>        c. Create single PLATFORM with smp_init, and cpu_up  which dispatches
>           to correct code.
>        d. Check return code of io_remap for power.
>        e. Use read* write* calls for MMIO mapped addresses.
>        f. Xen coding style changes.
>
>
>     v1: Add Odroid-XU board support
>
> Signed-off-by: Suriyan Ramasami <suriyan.r@gmail.com>

Of course, you will have to move you signed-off-by before the "---" :).

> ---
>   xen/arch/arm/platforms/exynos5.c        | 61 +++++++++++++++++++++++++++++++--
>   xen/include/asm-arm/platforms/exynos5.h |  8 +++++
>   2 files changed, 66 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c
> index b65c2c2..e61d9be 100644
> --- a/xen/arch/arm/platforms/exynos5.c
> +++ b/xen/arch/arm/platforms/exynos5.c
> @@ -26,6 +26,7 @@
>   #include <asm/platforms/exynos5.h>
>   #include <asm/platform.h>
>   #include <asm/io.h>
> +#include <asm/delay.h>
>
>   static int exynos5_init_time(void)
>   {
> @@ -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.

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.
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;
> +    }
> +
> +    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.

> +    /* 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.

> +    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.

> +    } while ( byte4 != EXYNOS5410_POWER_ENABLE );
> +
> +    dprintk(XENLOG_INFO, "Power status changed to %x!\n", byte4);
> +
> +    iounmap(power);
> +
> +    rc = cpu_up_send_sgi(cpu);
> +    return rc;
> +}
> +
>   static void exynos5_reset(void)
>   {
>       void __iomem *pmu;
> @@ -103,7 +157,8 @@ static void exynos5_reset(void)
>
>   static const char * const exynos5_dt_compat[] __initconst =
>   {
> -    "samsung,exynos5250",
> +    EXYNOS5250_COMPAT,
> +    EXYNOS5410_COMPAT,
>       NULL
>   };
>
> @@ -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.

Regards,

>       .smp_init = exynos5_smp_init,
> -    .cpu_up = cpu_up_send_sgi,
> +    .cpu_up = exynos5_cpu_up,
>       .reset = exynos5_reset,
>       .blacklist_dev = exynos5_blacklist_dev,
>   PLATFORM_END
> diff --git a/xen/include/asm-arm/platforms/exynos5.h b/xen/include/asm-arm/platforms/exynos5.h
> index ea941e7..4d84fe8 100644
> --- a/xen/include/asm-arm/platforms/exynos5.h
> +++ b/xen/include/asm-arm/platforms/exynos5.h
> @@ -13,6 +13,14 @@
>   #define EXYNOS5_SWRESET             0x0400      /* Relative to PA_PMU */
>
>   #define S5P_PA_SYSRAM   0x02020000
> +#define EXYNOS5250_COMPAT           "samsung,exynos5250"
> +
> +/* Exynos5410 specific */
> +#define EXYNOS5410_PA_SYSRAM        0x0207301c
> +#define EXYNOS5410_POWER_CPU_BASE   (0x10040000 + 0x2000)
> +#define EXYNOS5410_POWER_CPU_OFFSET (0x80)
> +#define EXYNOS5410_POWER_ENABLE     (0x03)
> +#define EXYNOS5410_COMPAT           "samsung,exynos5410"
>
>   #endif /* __ASM_ARM_PLATFORMS_EXYNOS5_H */
>   /*
>

-- 
Julien Grall