Re: [ARM:PATCH v1 1/1] Add Odroid-XU (Exynos5410) support

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

Hi Suriyan,

On 07/24/2014 11:47 PM, Suriyan Ramasami wrote:
>    XEN/ARM: Add Odroid-XU support
> 
>    The Odroid-XU from hardkernel is an Exynos5410 based board.
>    This patch adds support for the above said board.
> 
> Signed-off-by: Suriyan Ramasami <suriyan.r@gmail.com>
> ---
>  xen/arch/arm/platforms/exynos5.c        | 52 +++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/platforms/exynos5.h |  7 +++++
>  2 files changed, 59 insertions(+)
> 
> diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c
> index 65e584f..a210404 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)
>  {
> @@ -85,6 +86,46 @@ static int __init exynos5_smp_init(void)
>      return 0;
>  }
>  
> +static int __init exynos5410_smp_init(void)
> +{
> +    void __iomem *sysram;
> +    void __iomem *power;
> +    char *c;
> +    int i;
> +
> +    /* Power the secondary cores. */
> +    for (i = 1; i < EXYNOS5410_NUM_CPUS; i++) {

Can we avoid hardcoding the number of CPUs here and rely on the device tree?

> +       power = ioremap_nocache(EXYNOS5410_POWER_CPU_BASE +
> +                               i * EXYNOS5410_POWER_CPU_OFFSET, PAGE_SIZE);

You forgot to check the return of ioremap_nocache.

> +       c = (char *) power;
> +       dprintk(XENLOG_INFO, "Power: %x status: %x\n", c[0], c[4]);
> +       c[0] = EXYNOS5410_POWER_ENABLE;

You have to use read*, write* helpers when the MMIO is mapped via ioremap_*.

Otherwise the compiler may reorder the access to the region and the
behavior would be undefined.

The remark is the same everywhere in this function.

> +       dprintk(XENLOG_INFO, "Waiting for power status to change to %d\n",
> +               EXYNOS5410_POWER_ENABLE);
> +       while (c[4] != EXYNOS5410_POWER_ENABLE) {

The Xen coding style request the "{" to be on a newline.

> +           udelay(1);
> +       }
> +       dprintk(XENLOG_INFO, "Power status changed to %d!\n",
> +               EXYNOS5410_POWER_ENABLE);
> +       iounmap(power);
> +
> +       sysram = ioremap_nocache(EXYNOS5410_PA_SYSRAM, PAGE_SIZE);
> +       if ( !sysram )
> +       {
> +           dprintk(XENLOG_ERR, "Unable to map exynos5 MMIO\n");
> +           return -EFAULT;
> +       }
> +
> +       printk("Set SYSRAM to %"PRIpaddr" (%p)\n",
> +              __pa(init_secondary), init_secondary);
> +       writel(__pa(init_secondary), sysram);
> +
> +       iounmap(sysram);
> +
> +    }
> +    return 0;
> +}
> +

Regards,

-- 
Julien Grall