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

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

[Suriyan Ramasami]

   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.

   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>
---
 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;
+    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;
+    }
+
+    /* EXYNOS5410: Power the secondary core */
+    power = ioremap_nocache(EXYNOS5410_POWER_CPU_BASE +
+                            cpu * EXYNOS5410_POWER_CPU_OFFSET, PAGE_SIZE);
+    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);
+    } 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,
     .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 */
 /*
-- 
1.9.1

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

[Julien Grall]

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

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

[Ian Campbell]

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?

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

[Julien Grall]

Hi Ian,

On 07/28/2014 10:03 AM, Ian Campbell wrote:
> 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.

While it's perfectly fine when the platform code support only 2
different version processor. The if/else will become very hard to read
when new board will be added (such as the Arndale octa).

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

Hence, at every dt_machine_is_compatible you have to think: "Is this my
machine?". Having 2 platform would be easier to read such as:

exynos5_smp_init(paddr_t sysram)
{
}

exynos5250_smp_init(void)
{
   return exynos_smp_init(sysram_exynos5250);
}

exynos5410_smp_init(void)
{
   return exynos_smp_init(sysram_exynos5410);
}

No need to check the compatible every time and save us from expensive
checking.

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

The Linux kernel is tight to a specific device tree. While the fallback
is perfectly fine for the Arndale (because we already supported it), the
support of the Odroid xu in Linux will use the bindings by default.

We should use the same things and avoid if/else as much as possible and
use the device tree for this purpose. Hence the binding is very simple.

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

So you add an indentation for about 50 lines just because we want to
keep a single platform structure...

Less than 5% of the code is shared. It's perfectly fine to have 2
different platforms

PLATFORM_EXYNOS5250
	.cpu_up = cpu_up_send_sgi

PLATFORM_EXYNOS5410
	.cpu_up = exynos5410_cpu_up

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

Hrm... the specific mapping is a function. You are mixing with the
blacklist field.


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

The chipid is not defined in the device tree bindings. IIRC, it's not
the case for the PWM region. I will have to check the code.

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Mon, 2014-07-28 at 12:32 +0100, Julien Grall wrote:

> Less than 5% of the code is shared. It's perfectly fine to have 2
> different platforms

To be clear, I'm fine with *either* approach rather than having a strong
preference for one over the other where the differences are relatively
minor (as they are here IMHO).

> >>> @@ -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.
> > 
> 
> Hrm... the specific mapping is a function. You are mixing with the
> blacklist field.

Indeed I was.

> > Is it possible that the 52xx DTBs have caught up and these are no longer
> > required even there though?
> 
> The chipid is not defined in the device tree bindings. IIRC, it's not
> the case for the PWM region. I will have to check the code.
> 
> Regards,
>