From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [Patch v3 1/1] Add Odroid-XU (Exynos5410) Date: Mon, 18 Aug 2014 14:07:38 -0500 Message-ID: <53F24EFA.8020409@linaro.org> References: <1407985978-13997-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: <1407985978-13997-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 Hello Suriyan, On 13/08/14 22:12, Suriyan Ramasami wrote: > -static int __init exynos5_smp_init(void) > +static int __init exynos5250_cpu_up(int cpu) > +{ > + return cpu_up_send_sgi(cpu); > +} > + This is not necessary. You can directly assign cpu_up_send_sgi to the cpu_up callback of the platform structure. > +static int __init exynos_smp_init(unsigned long pa_sysram) paddr_t pa_sysram > { > void __iomem *sysram; > > - sysram = ioremap_nocache(S5P_PA_SYSRAM, PAGE_SIZE); > + sysram = ioremap_nocache(pa_sysram, PAGE_SIZE); > if ( !sysram ) > { > dprintk(XENLOG_ERR, "Unable to map exynos5 MMIO\n"); > @@ -84,6 +95,121 @@ static int __init exynos5_smp_init(void) > return 0; > } > > +static int __init exynos5250_smp_init(void) > +{ > + return exynos_smp_init(S5P_PA_SYSRAM); > +} I would rename S5P_PA_SYSRAM and EXYNOS5_* to something specific to exynos 5250. It would make clear that new platform should use the device tree to get information. > +static int __init exynos5_smp_init(void) > +{ > + struct dt_device_node *node; > + u64 sysram_ns_base_addr = 0; > + u64 size; > + int rc; > + > + node = dt_find_compatible_node(NULL, NULL, "samsung,exynos4210-sysram-ns"); > + if ( !node ) { Coding style: if ( !node ) { > + dprintk(XENLOG_ERR, "samsung,exynos4210-sysram-ns missing in DT\n"); The coding style doesn't allow to use hard tab. > + return -ENXIO; > + } > + > + rc = dt_device_get_address(node, 0, &sysram_ns_base_addr, &size); > + if (rc) { Coding style: if ( rc ) { > + dprintk(XENLOG_ERR, "Error in \"samsung,exynos4210-sysram-ns\"\n"); > + return -ENXIO; > + } > + > + dprintk(XENLOG_INFO, "sysram_ns_base_addr: %08x size: %08x\n", > + (unsigned int) sysram_ns_base_addr, (unsigned int) size); Why do you cast to unsigned int rather than right printf format? > + > + return exynos_smp_init(sysram_ns_base_addr + 0x1c); > +} > + > +static int exynos_cpu_power_state(void __iomem *power, int cpu) > +{ > + return readl(power + EXYNOS_ARM_CORE_STATUS(cpu)) & > + S5P_CORE_LOCAL_PWR_EN; Please correctly align the second line to the open branch. I.e: return readl(power .... S5P_CORE_... Also, why did you replace the __raw_readl by a readl? The former one doesn't use barrier while the latter does. As Linux is using the former one, I don't understand why we would require barrier on Xen. > +} > + > +static void exynos_cpu_set_power_up(void __iomem *power, int cpu) > +{ > + writel(S5P_CORE_LOCAL_PWR_EN, > + power + EXYNOS_ARM_CORE_CONFIG(cpu)); Same here for both coding style and writel. > +} > + > +static int exynos_cpu_power_up(void __iomem *power, int cpu) > +{ > + int timeout; unsigned int. > + > + if ( !exynos_cpu_power_state(power, cpu) ) { Coding style: if ( ... ) { > + exynos_cpu_set_power_up(power, cpu); I would keep the same name as Linux ie exynos_cpu_power_up to avoid confusion and make backporting easier. > + timeout = 10; > + > + /* wait max 10 ms until cpu is on */ > + while (exynos_cpu_power_state(power, cpu) != S5P_CORE_LOCAL_PWR_EN) { Coding style: while ( ... ) { > + if (timeout-- == 0) > + break; Coding style: if ( ... ) > + > + mdelay(1); > + } > + > + if (timeout == 0) { Coding style: if ( ... ) { > + dprintk(XENLOG_ERR, "CPU%d power enable failed", cpu); > + return -ETIMEDOUT; > + } > + } > + return 0; > +} > + > +static int exynos5_cpu_up(int cpu) > +{ > + static const struct dt_device_match exynos_dt_pmu_matches[] __initconst = > + { [..] > + DT_MATCH_COMPATIBLE("samsung,exynos5422-pmu"), Where does this compatible come from? I don't find any reference in Linux upstream documentation (Documentation/devicetree/bindings/arm/samsung/pmu.txt). > + { /*sentinel*/ }, > + }; > + void __iomem *power; > + u64 power_base_addr = 0; > + u64 size; > + struct dt_device_node *node; > + int rc; > + > + node = dt_find_matching_node(NULL, exynos_dt_pmu_matches); > + if ( !node ) { Coding style: if ( !node ) { > + dprintk(XENLOG_ERR, "samsung,exynos5XXX-pmu missing in DT\n"); > + return -ENXIO; > + } > + > + rc = dt_device_get_address(node, 0, &power_base_addr, &size); > + if ( rc ) { Coding style if ( rc ) { > + dprintk(XENLOG_ERR, "Error in \"samsung,exynos5XXXX-pmu\"\n"); > + return -ENXIO; > + } > + > + dprintk(XENLOG_INFO, "power_base_addr: %08x size: %08x\n", I would use XENLOG_DEBUG > + (unsigned int) power_base_addr, (unsigned int) size); > + > + power = ioremap_nocache(power_base_addr + > + EXYNOS_ARM_CORE0_CONFIG, PAGE_SIZE); > + if ( !power ) > + { > + dprintk(XENLOG_ERR, "Unable to map power MMIO\n"); > + return -EFAULT; > + } > + > + rc = exynos_cpu_power_up(power, cpu); > + if ( rc ) { > + return -ETIMEDOUT; If exynos_cpu_power_up is failing you never unmap the power region. > +PLATFORM_START(exynos5410, "SAMSUNG EXYNOS5410") Nothing looks 5410 specific in structure. I would rename it to exynos5, "SAMSUNG EXYNOS5". So if someone want to add a new exynos5 (such as the octa) it wouldn't have to rename this platform. Regards, -- Julien Grall