From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Campbell <Ian.Campbell@citrix.com>
Subject: Re: [ARM:PATCH v1 1/1] Add Odroid-XU (Exynos5410)
	support
Date: Fri, 25 Jul 2014 09:18:42 +0100
Message-ID: <1406276322.29480.77.camel@dagon.hellion.org.uk>
References: <1406242066-7001-1-git-send-email-suriyan.r@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1406242066-7001-1-git-send-email-suriyan.r@gmail.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Suriyan Ramasami <suriyan.r@gmail.com>
Cc: keir@xen.org, ian.jackson@eu.citrix.com, julien.grall@linaro.org, tim@xen.org, xen-devel@lists.xen.org, jbeulich@suse.com
List-Id: xen-devel@lists.xenproject.org

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.