From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [Patch v2 1/1] Add Odroid-XU (Exynos 5410) Date: Sat, 26 Jul 2014 23:17:10 +0100 Message-ID: <53D428E6.7040304@linaro.org> References: <1406332945-27713-1-git-send-email-suriyan.r@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1406332945-27713-1-git-send-email-suriyan.r@gmail.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Suriyan Ramasami , xen-devel@lists.xen.org Cc: keir@xen.org, ian.jackson@eu.citrix.com, ian.campbell@citrix.com, jbeulich@suse.com, tim@xen.org List-Id: xen-devel@lists.xenproject.org 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 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 > #include > #include > +#include > > 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