From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [XEN/ARM PATCH v1 1/1] Unbreak Arndale XEN boot Date: Thu, 11 Sep 2014 11:49:38 -0700 Message-ID: <5411EEC2.7050108@linaro.org> References: <1410456310-31415-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: <1410456310-31415-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.campbell@citrix.com, tim@xen.org, jbeulich@suse.com, ian.jackson@citrix.com, julien.grall@linaro.com List-Id: xen-devel@lists.xenproject.org Hello Suriyan, My email address is @linaro.org not @linaro.com. I nearly miss this patch because of this. Depending if Ian apply his patch (see the conversation on "Odroid-XU..."), I would update the title and the message. On 11/09/14 10:25, Suriyan Ramasami wrote: > diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c > index bc9ae15..334650c 100644 > --- a/xen/arch/arm/platforms/exynos5.c > +++ b/xen/arch/arm/platforms/exynos5.c > @@ -28,6 +28,9 @@ > #include > #include > > +/* This corresponds to CONFIG_NR_CPUS in linux config */ > +#define EXYNOS_CONFIG_NR_CPUS 0x08 > + > #define EXYNOS_ARM_CORE0_CONFIG 0x2000 > #define EXYNOS_ARM_CORE_CONFIG(_nr) (0x80 * (_nr)) > #define EXYNOS_ARM_CORE_STATUS(_nr) (EXYNOS_ARM_CORE_CONFIG(_nr) + 0x4) > @@ -42,8 +45,6 @@ static int exynos5_init_time(void) > u64 mct_base_addr; > u64 size; > > - BUILD_BUG_ON(EXYNOS5_MCT_G_TCON >= PAGE_SIZE); > - > node = dt_find_compatible_node(NULL, NULL, "samsung,exynos4210-mct"); > if ( !node ) > { > @@ -61,7 +62,14 @@ static int exynos5_init_time(void) > dprintk(XENLOG_INFO, "mct_base_addr: %016llx size: %016llx\n", > mct_base_addr, size); > > - mct = ioremap_attr(mct_base_addr, PAGE_SIZE, PAGE_HYPERVISOR_NOCACHE); > + if ( size <= EXYNOS5_MCT_G_TCON ) Honestly, I don't think this check is very useful. The device tree should always contains valid size. > + { > + dprintk(XENLOG_ERR, "EXYNOS5_MCT_G_TCON: %d is >= size\n", > + EXYNOS5_MCT_G_TCON); > + return -EINVAL; > + } > + > + mct = ioremap_attr(mct_base_addr, size, PAGE_HYPERVISOR_NOCACHE); > if ( !mct ) > { > dprintk(XENLOG_ERR, "Unable to map MCT\n"); > @@ -91,14 +99,36 @@ static int exynos5250_specific_mapping(struct domain *d) > return 0; > } > > -static int __init exynos5_smp_init(void) > +static int exynos_smp_init_getbaseandoffset(u64 *sysram_addr, > + u64 *sysram_offset) > { This function contains nearly twice the same code. Except the compatible string and the offset which differs, everything is the same. I would do smth like: node = dt_find_compatible_node(NULL, NULL, "samsung,secure-firmare"); if ( !node ) { compatible = "samsung,exynos4210-sysram"; *sysram_offset = 0; } else { compatible "samsung,exynos4210-sysram-ns"; *sysram_offset = 0x1c; } node = dt_find_compatible_node(NULL, NULL, compatible); if ( !node ) .... [..] > +static int __init exynos5_smp_init(void) > +{ > + void __iomem *sysram; > + u64 sysram_addr; > + u64 sysram_offset; > + int rc; > + > + rc = exynos_smp_init_getbaseandoffset(&sysram_addr, &sysram_offset); > + if ( rc ) > + return rc; > > - sysram = ioremap_nocache(sysram_ns_base_addr, PAGE_SIZE); > + dprintk(XENLOG_INFO, "sysram_addr: %016llx offset: %016llx\n", > + sysram_addr, sysram_offset); > + > + if ( sysram_offset >= PAGE_SIZE ) > + { > + dprintk(XENLOG_ERR, "sysram_offset is >= PAGE_SIZE\n"); > + return -EINVAL; > + } As the previous check for the MCT. I don't think it's useful. Also why don't do get the size from the device tree? > + > + sysram = ioremap_nocache(sysram_addr, PAGE_SIZE); > if ( !sysram ) > { > dprintk(XENLOG_ERR, "Unable to map exynos5 MMIO\n"); > @@ -125,7 +176,7 @@ static int __init exynos5_smp_init(void) > > printk("Set SYSRAM to %"PRIpaddr" (%p)\n", > __pa(init_secondary), init_secondary); > - writel(__pa(init_secondary), sysram + 0x1c); > + writel(__pa(init_secondary), sysram + sysram_offset); > > iounmap(sysram); > > @@ -134,14 +185,14 @@ static int __init exynos5_smp_init(void) > > static int exynos_cpu_power_state(void __iomem *power, int cpu) > { > - return __raw_readl(power + EXYNOS_ARM_CORE_STATUS(cpu)) & > - S5P_CORE_LOCAL_PWR_EN; > + return __raw_readl(power + EXYNOS_ARM_CORE0_CONFIG + > + EXYNOS_ARM_CORE_STATUS(cpu)) & S5P_CORE_LOCAL_PWR_EN; Linux has the EXYNOS_ARM_CORE0_CONFIG included in the macro EXYNOS_CORE_CONFIGURATION. I would do the same here. [..] > - power = ioremap_nocache(power_base_addr + > - EXYNOS_ARM_CORE0_CONFIG, PAGE_SIZE); > + if ( size <= EXYNOS_ARM_CORE0_CONFIG + > + EXYNOS_ARM_CORE_STATUS(EXYNOS_CONFIG_NR_CPUS) ) > + { > + dprintk(XENLOG_ERR, "CORE registers fall outside of pmu range.\n"); > + return -EINVAL; > + } > + Same remark as before for the check. [..] > - pmu = ioremap_nocache(power_base_addr, PAGE_SIZE); > + if ( size <= EXYNOS5_SWRESET ) > + { > + dprintk(XENLOG_ERR, "EXYNOS5_SWRESET: %d is >= size\n", > + EXYNOS5_SWRESET); > + return; > + } > + Same here too. Regards, -- Julien Grall