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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2014-07-24 at 15:47 -0700, Suriyan Ramasami wrote:
> 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++) {
> +       power = ioremap_nocache(EXYNOS5410_POWER_CPU_BASE +
> +                               i * EXYNOS5410_POWER_CPU_OFFSET, PAGE_SIZE);
> +       c = (char *) power;
> +       dprintk(XENLOG_INFO, "Power: %x status: %x\n", c[0], c[4]);
> +       c[0] = EXYNOS5410_POWER_ENABLE;
> +       dprintk(XENLOG_INFO, "Waiting for power status to change to %d\n",
> +               EXYNOS5410_POWER_ENABLE);
> +       while (c[4] != EXYNOS5410_POWER_ENABLE) {
> +           udelay(1);
> +       }
> +       dprintk(XENLOG_INFO, "Power status changed to %d!\n",
> +               EXYNOS5410_POWER_ENABLE);
> +       iounmap(power);

Doesn't this turn on the core power before setting the strartup address (below)?

> +
> +       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);

You do this for every CPU even thought the address doesn't differ, is that 
right?

> +    }
> +    return 0;
> +}
> 
> @@ -105,6 +146,7 @@ static void exynos5_reset(void)
>  static const char * const exynos5_dt_compat[] __initconst =
>  {
>      "samsung,exynos5250",
> +    "samsung,exynos5410",
>      NULL
>  };
>  
> @@ -128,6 +170,16 @@ PLATFORM_START(exynos5, "SAMSUNG EXYNOS5")
>      .blacklist_dev = exynos5_blacklist_dev,
>  PLATFORM_END
>  
> +PLATFORM_START(exynos5410, "SAMSUNG EXYNOS5410")

Both of the PLATFORM_START's now refer to the same compatible list, so
both of them will match. I don't know which one will win. I suppose it
must be your new one or else you wouldn't have been able to test.


You need to either:

create two compatible lists, one for each PLATFORM block

or

create a single PLATFORM with an smp_init which dispatches to the
correct code based on the compatible ID of the platform.

Looking at your new smp_init can you do

        Write SYSRAM

        if (! cmpat 5410 ) 
                return
                
    for each cpu
                enable power.

?

If you do end up with two smp init functions please also rename the
existing exynos5_smp_init to be 5250 specific.

It's also possible that you should do the per-cpu power on in the cpu_up
hook rather than smp_init. In which case smp_init would stay as it is
and cpu_up would become:

        if ( 5410 ) 
                map and write power enable
                
        cpu_up_send_sgi

or something like that.

Ian.


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