Re: [PATCH] xen/arm: Initial Broadcom A15 platform support

[Julien Grall <julien.grall@linaro.org>]

On 11/08/2013 11:37 PM, Jon Fraser wrote:
> Initial platform support for Broadcom A15/B15 platforms.
>
> Signed-off-by: Jon Fraser <jfraser@broadcom.com>

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 <asm/platform.h>
> +#include <xen/mm.h>
> +#include <xen/vmap.h>
> +
> +
> +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, &reg_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, &reg_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, &reg_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