From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH] xen/arm: Initial Broadcom A15 platform support Date: Mon, 11 Nov 2013 20:37:06 +0000 Message-ID: <52813FF2.2000804@linaro.org> References: <1383953844-31566-1-git-send-email-jfraser@broadcom.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1383953844-31566-1-git-send-email-jfraser@broadcom.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: Jon Fraser Cc: ian.campbell@citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 11/08/2013 11:37 PM, Jon Fraser wrote: > Initial platform support for Broadcom A15/B15 platforms. > > Signed-off-by: Jon Fraser Hello, Thanks to add support for broadcom processor in Xen. It seems you use the wrong coding style for this patch. You can read CODING_STYLES at the root of the xen repository. > --- > xen/arch/arm/platforms/Makefile | 1 + > xen/arch/arm/platforms/brcm.c | 249 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 250 insertions(+) > create mode 100644 xen/arch/arm/platforms/brcm.c > > diff --git a/xen/arch/arm/platforms/Makefile b/xen/arch/arm/platforms/Makefile > index f0dd72c..60635b4 100644 > --- a/xen/arch/arm/platforms/Makefile > +++ b/xen/arch/arm/platforms/Makefile > @@ -3,3 +3,4 @@ obj-$(CONFIG_ARM_32) += exynos5.o > obj-$(CONFIG_ARM_32) += midway.o > obj-$(CONFIG_ARM_32) += omap5.o > obj-$(CONFIG_ARM_32) += sunxi.o > +obj-$(CONFIG_ARM_32) += brcm.o > diff --git a/xen/arch/arm/platforms/brcm.c b/xen/arch/arm/platforms/brcm.c > new file mode 100644 > index 0000000..b8d8527 > --- /dev/null > +++ b/xen/arch/arm/platforms/brcm.c > @@ -0,0 +1,249 @@ > +/* > + * xen/arch/arm/platform/brcm.c > + * > + * Broadcom specific settings > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include > +#include > +#include > + > + > +struct brcm_plat_regs { > + uint32_t hif_mask; > + uint32_t hif_cpu_reset_config; > + uint32_t hif_boot_continuation; > + uint32_t cpu0_pwr_zone_ctrl; > +}; > + > +static struct brcm_plat_regs regs; > + > +static int brcm_get_dt_node(char *compat_str, struct dt_device_node **dn, > + u32 *reg_base) spurious space before u32. > +{ > + struct dt_device_node *dt_dn; > + u64 reg_base_64; > + u64 size; > + int rc; > + > + dt_dn = dt_find_compatible_node(NULL, NULL, compat_str); > + if (!dt_dn) { if ( ... ) { > + dprintk(XENLOG_ERR, "%s: missing \"%s\" node\n", __func__, compat_str); > + return(-EIO); > + } > + > + rc = dt_device_get_address(dt_dn, 0, ®_base_64, &size); > + if (rc) { > + dprintk(XENLOG_ERR, "%s: missing \"reg\" prop\n", __func__); > + return(rc); > + } > + > + if (dn) if ( ... ) > + *dn = dt_dn; > + > + if (reg_base) > + *reg_base = (u32)(reg_base_64 & 0xffffffff); I think you don't need to use both cast and mask. You can only use one of theses solutions. > + > + return(0); > +} > + > +static int brcm_populate_plat_regs(void) > +{ > + int rc; > + struct dt_device_node *dt_dn; As you don't modify the node, you should use const. > + u32 reg_base; > + u32 val; > + char *compat_str; Same here. > + > + compat_str = "brcm,brcmstb-cpu-biu-ctrl"; > + rc = brcm_get_dt_node(compat_str, &dt_dn, ®_base); > + if (rc) > + return(rc); > + > + dt_property_read_u32(dt_dn, "cpu-reset-config-reg", &val); You should check the return of dt_property_read_u32 > + regs.hif_cpu_reset_config = reg_base + val; > + > + dt_property_read_u32(dt_dn, "cpu0-pwr-zone-ctrl-reg", &val); Same here. > + regs.cpu0_pwr_zone_ctrl = reg_base + val; > + > + compat_str = "brcm,brcmstb-hif-continuation"; > + rc = brcm_get_dt_node(compat_str, &dt_dn, ®_base); > + if (rc) > + return(rc); > + > + dt_property_read_u32(dt_dn, "stb-boot-hi-addr0-reg", &val); Same here. > + regs.hif_boot_continuation = reg_base + val; > + > + dprintk(XENLOG_INFO, "hif_cpu_reset_config : %08xh\n", > + regs.hif_cpu_reset_config); > + dprintk(XENLOG_INFO, "cpu0_pwr_zone_ctrl : %08xh\n", > + regs.cpu0_pwr_zone_ctrl); > + dprintk(XENLOG_INFO, "hif_boot_continuation : %08xh\n", > + regs.hif_boot_continuation); > + > + return(0); > +} > + > +#define ZONE_PWR_UP_REQ (1 << 10) > +#define ZONE_PWR_ON_STATE (1 << 26) > + > +static int brcm_cpu_power_on(void __iomem *va, int cpu) > +{ > + int rc = 0; > + volatile u32 *reg; > + u32 tmp; > + > + reg = (volatile u32 *)(va + (regs.cpu0_pwr_zone_ctrl & ~PAGE_MASK) + > + (cpu * sizeof(u32))); > + > + /* request core power on */ > + *reg = ZONE_PWR_UP_REQ; > + dsb(); You should use writel/readl wrapper to read/write memory mapped with ioremap_*. It will take care of dsb. > + > + /* wait for power to be applied to cire */ > + do { > + tmp = *reg; Same here. > + } while (!(tmp & ZONE_PWR_ON_STATE)); > + > + return rc; > +} > + > +static int brcm_cpu_power_on_all(void) > +{ > + int rc = 0; > + int cpu; > + void __iomem *va; > + > + va = ioremap_nocache(regs.cpu0_pwr_zone_ctrl & PAGE_MASK, PAGE_SIZE); ioremap_* is able to cooperate with non aligned address. You don't need to align cpu0_pwr_zone_ctrl. You should also check that va is not NULL (it's an error). > + > + for (cpu = 1; cpu < 4; cpu++) { > + rc = brcm_cpu_power_on(va, cpu); > + if (rc) > + break; > + } I don't like the for loop on the cores. In the future, it's possible that Xen decides to only use 2 cores, so you will power unused core. What about enabling power in brcm_cpu_up, if it's not too late? > + > + iounmap(va); > + > + return rc; > +} > + > +static int __init brcm_smp_init(void) > +{ > + void __iomem *va; > + volatile u32 *reg; > + u32 target_pc; > + dprintk(XENLOG_INFO, "%s\n",__func__); > + > + if (regs.cpu0_pwr_zone_ctrl) { > + dprintk(XENLOG_INFO, "applying power to remaining CPU cores\n"); > + brcm_cpu_power_on_all(); You already call brcm_cpu_power_on_all in brcm_init. Why do you need to call it again? > + } > + > + va = ioremap_nocache(regs.hif_boot_continuation & PAGE_MASK, PAGE_SIZE); > + reg = (volatile u32 *)(va + (regs.hif_boot_continuation & ~PAGE_MASK)); > + > + > + /* set boot continuation registers */ > + dprintk(XENLOG_INFO, "%s: HIF CR at 0x%p\n", __func__, reg); > + target_pc =__pa(init_secondary); > + reg[3] = target_pc; > + reg[5] = target_pc; > + reg[7] = target_pc; > + > + iounmap(va); > + > + va = ioremap_nocache(regs.hif_cpu_reset_config & PAGE_MASK, PAGE_SIZE); Some comment as the previous ioremap_* > + reg = (volatile u32 *)(va + (regs.hif_cpu_reset_config & ~PAGE_MASK)); > + > + /* now take the cpus out of reset */ > + reg[0] = 0; writel? > + > + iounmap(va); > + > + return 0; > +} > + > + > +static int brcm_init(void) > +{ > + int rc; > + > + rc = brcm_populate_plat_regs(); > + if (rc) > + return(rc); > + > + /* > + * All newer 28nm chips will require poking the BPCM prior to starting > + * the cores. > + */ > + if (regs.cpu0_pwr_zone_ctrl) { > + dprintk(XENLOG_INFO, "applying power to remaining CPU cores\n"); > + brcm_cpu_power_on_all(); > + } > + > + return(0); > +} > +static int __init brcm_cpu_up(int cpu) > +{ > + /* > + * Nothing to do here, the generic sev() will suffice to kick CPUs > + * out of either the firmware or our own smp_up_cpu gate, > + * depending on where they have ended up. > + */ > + > + return 0; > +} > + > +static int brcm_specific_mapping(struct domain *d) > +{ > + /* Map the entire GISB register space 1:1 */ > + map_mmio_regions(d, 0xf0000000, 0xf0ffffff, > + 0xf0000000); > + > + return 0; > +} > + > +static void brcm_reset(void) > +{ > +} > + > +static uint32_t brcm_quirks(void) > +{ > + return PLATFORM_QUIRK_DOM0_MAPPING_11; > +} > + > +static const char const *brcm_dt_compat[] __initdata = const char * const ... __initconst > +{ > + "arm,brcm", > + "brcm,brcmstb", > + NULL > +}; > + > +PLATFORM_START(brcm, "Broadcom B15") > + .compatible = brcm_dt_compat, > + .init = brcm_init, > + .smp_init = brcm_smp_init, > + .cpu_up = brcm_cpu_up, > + .quirks = brcm_quirks, > + .reset = brcm_reset, > + .specific_mapping = brcm_specific_mapping, > +PLATFORM_END > + > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * indent-tabs-mode: nil > + * End: > + */ > -- Julien Grall