[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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.

       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@xxxxxxxxx>

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",

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


      .smp_init = exynos5_smp_init,
-    .cpu_up = cpu_up_send_sgi,
+    .cpu_up = exynos5_cpu_up,
      .reset = exynos5_reset,
      .blacklist_dev = exynos5_blacklist_dev,
diff --git a/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

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.