From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [XEN/ARM PATCH v2 1/1] Add support for Exynos secure firmware Date: Fri, 12 Sep 2014 16:52:42 -0700 Message-ID: <5413874A.5070001@linaro.org> References: <1410562915-16761-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: <1410562915-16761-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: ian.jackson@citrix.com, keir@xen.org, ian.campbell@citrix.com, jbeulich@suse.com, tim@xen.org List-Id: xen-devel@lists.xenproject.org Hi Suriyan, On 12/09/14 16:01, Suriyan Ramasami wrote: > +static int __init exynos5_smp_init(void) > +{ > + void __iomem *sysram; > + u64 sysram_addr; > + u64 size; > + u64 sysram_offset; > + int rc; > + > + rc = exynos_smp_init_getbasesizeoffset(&sysram_addr, &size, &sysram_offset); The function name is odd. As you call the function only here, couldn't you inline it? > + if ( rc ) > + return rc; > + > + dprintk(XENLOG_INFO, "sysram_addr: %016llx size: %016llx offset: %016llx\n", > + sysram_addr, size, sysram_offset); > + > + sysram = ioremap_nocache(sysram_addr, size); > if ( !sysram ) > { > dprintk(XENLOG_ERR, "Unable to map exynos5 MMIO\n"); > @@ -125,7 +158,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); > + writel(__pa(init_secondary), sysram + sysram_offset); > > iounmap(sysram); > > @@ -135,7 +168,7 @@ 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; > + S5P_CORE_LOCAL_PWR_EN; Why this change? > } > > static void exynos_cpu_power_up(void __iomem *power, int cpu) > @@ -171,8 +204,7 @@ static int exynos5_cpu_power_up(void __iomem *power, int cpu) > return 0; > } > > -static int exynos5_get_pmu_base_addr(u64 *power_base_addr) { > - u64 size; > +static int exynos5_get_pmu_baseandsize(u64 *power_base_addr, u64 *size) { The Xen coding style is static int foo(...) { > struct dt_device_node *node; > int rc; > static const struct dt_device_match exynos_dt_pmu_matches[] __initconst = > @@ -190,7 +222,7 @@ static int exynos5_get_pmu_base_addr(u64 *power_base_addr) { > return -ENXIO; > } > > - rc = dt_device_get_address(node, 0, power_base_addr, &size); > + rc = dt_device_get_address(node, 0, power_base_addr, size); > if ( rc ) > { > dprintk(XENLOG_ERR, "Error in \"samsung,exynos5XXX-pmu\"\n"); > @@ -198,23 +230,31 @@ static int exynos5_get_pmu_base_addr(u64 *power_base_addr) { > } > > dprintk(XENLOG_DEBUG, "power_base_addr: %016llx size: %016llx\n", > - *power_base_addr, size); > + *power_base_addr, *size); > > return 0; > } > > +static void exynos_smc(u32 cmd, u32 arg1, u32 arg2, u32 arg3) > +{ > + asm( > + "dsb;" > + "smc #0;" > + ); > +} >+ The compiler may decide to inline the function. This will end up to the command register not in register r0. Give a look to __invoke_psci_fn_smc in xen/arch/arm/psci.c. It might be worth to introduce an SMC helper. > static int exynos5_cpu_up(int cpu) > { > u64 power_base_addr; > + u64 size; > void __iomem *power; > int rc; > > - rc = exynos5_get_pmu_base_addr(&power_base_addr); > + rc = exynos5_get_pmu_baseandsize(&power_base_addr, &size); > if ( rc ) > return rc; > > - power = ioremap_nocache(power_base_addr + > - EXYNOS_ARM_CORE0_CONFIG, PAGE_SIZE); > + power = ioremap_nocache(power_base_addr, size); > if ( !power ) > { > dprintk(XENLOG_ERR, "Unable to map power MMIO\n"); > @@ -230,22 +270,23 @@ static int exynos5_cpu_up(int cpu) > > iounmap(power); > > + exynos_smc(SMC_CMD_CPU1BOOT, cpu, 0, 0); > + The call is not done unconditionally on Linux. It's only done when the secure firmware is present. > return cpu_up_send_sgi(cpu); > } > > static void exynos5_reset(void) > { > u64 power_base_addr; > + u64 size; > void __iomem *pmu; > int rc; > > - BUILD_BUG_ON(EXYNOS5_SWRESET >= PAGE_SIZE); > - > - rc = exynos5_get_pmu_base_addr(&power_base_addr); > + rc = exynos5_get_pmu_baseandsize(&power_base_addr, &size); > if ( rc ) > return; > > - pmu = ioremap_nocache(power_base_addr, PAGE_SIZE); > + pmu = ioremap_nocache(power_base_addr, size); > if ( !pmu ) > { > dprintk(XENLOG_ERR, "Unable to map PMU\n"); > @@ -264,6 +305,7 @@ static const struct dt_device_match exynos5_blacklist_dev[] __initconst = > * This is result to random freeze. > */ > DT_MATCH_COMPATIBLE("samsung,exynos4210-mct"), > + DT_MATCH_COMPATIBLE("samsung,secure-firmware"), Can you add a comment explaining why we blacklist the secure firmware? Regards, -- Julien Grall