[PATCH 3/3] xen/arm: Add support for Broadcom 7445D0 A15 platform.

[PATCH 3/3] xen/arm: Add support for Broadcom 7445D0 A15 platform.

[Jon Fraser]

This code supports the Broadcom 7445D0 32-bit A15 based platform.

Signed-off-by: Jon Fraser <jfraser@broadcom.com>
---
 xen/arch/arm/platforms/Makefile |   1 +
 xen/arch/arm/platforms/brcm.c   | 326 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 327 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 680364f..3b92dde 100644
--- a/xen/arch/arm/platforms/Makefile
+++ b/xen/arch/arm/platforms/Makefile
@@ -1,4 +1,5 @@
 obj-y += vexpress.o
+obj-$(CONFIG_ARM_32) += brcm.o
 obj-$(CONFIG_ARM_32) += exynos5.o
 obj-$(CONFIG_ARM_32) += midway.o
 obj-$(CONFIG_ARM_32) += omap5.o
diff --git a/xen/arch/arm/platforms/brcm.c b/xen/arch/arm/platforms/brcm.c
new file mode 100644
index 0000000..7c9d888
--- /dev/null
+++ b/xen/arch/arm/platforms/brcm.c
@@ -0,0 +1,326 @@
+/*
+ * xen/arch/arm/platforms/brcm.c
+ *
+ * Broadcom Platform startup.
+ *
+ * Jon Fraser  <jfraser@broadcom.com>
+ * Copyright (c) 2013-2014 Broadcom Corporation
+ *
+ * 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>
+#include <asm/io.h>
+#include <xen/delay.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;
+    uint32_t    scratch_reg;
+};
+
+static u32 brcm_boot_continuation_pc;
+
+static struct brcm_plat_regs regs;
+
+static int brcm_get_dt_node(char *compat_str, struct dt_device_node **dn,
+                             u32 *reg_base)
+{
+    struct dt_device_node *node;
+    u64 reg_base_64;
+    u64 size;
+    int rc;
+
+    node = dt_find_compatible_node(NULL, NULL, compat_str);
+    if ( !node )
+    {
+        dprintk(XENLOG_ERR, "%s: missing \"%s\" node\n", __func__, compat_str);
+        return -ENOENT;
+    }
+
+    rc = dt_device_get_address(node, 0, &reg_base_64, &size);
+    if ( rc )
+    {
+        dprintk(XENLOG_ERR, "%s: missing \"reg\" prop\n", __func__);
+        return rc;
+    }
+
+    if ( dn )
+        *dn = node;
+
+    if ( reg_base )
+        *reg_base = (u32)(reg_base_64 & 0xffffffff);
+
+    return 0;
+}
+
+static int brcm_populate_plat_regs(void)
+{
+    int rc, failed;
+    struct dt_device_node *node;
+    u32 reg_base;
+    u32 val;
+
+    rc = brcm_get_dt_node("brcm,brcmstb-cpu-biu-ctrl", &node, &reg_base);
+    if ( rc )
+        return rc;
+
+    failed = 0;
+
+    if ( dt_property_read_u32(node, "cpu-reset-config-reg", &val) )
+        regs.hif_cpu_reset_config = reg_base + val;
+    else
+    {
+        dprintk(XENLOG_ERR, "Missing property \"cpu-reset-config-reg\"\n");
+        failed = 1;
+    }
+
+    if ( dt_property_read_u32(node, "cpu0-pwr-zone-ctrl-reg", &val) )
+        regs.cpu0_pwr_zone_ctrl = reg_base + val;
+    else
+    {
+        dprintk(XENLOG_ERR, "Missing property \"cpu0-pwr-zone-ctrl-reg\"\n");
+        failed = 1;
+    }
+
+    if ( dt_property_read_u32(node, "scratch-reg", &val) )
+        regs.scratch_reg = reg_base + val;
+    else
+    {
+        dprintk(XENLOG_ERR, "Missing property \"scratch-reg\"\n");
+        failed = 1;
+    }
+
+    rc = brcm_get_dt_node("brcm,brcmstb-hif-continuation", &node, &reg_base);
+    if ( rc )
+        return rc;
+
+    regs.hif_boot_continuation = reg_base;
+
+    if ( failed )
+        return -ENOENT;
+
+    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);
+    dprintk(XENLOG_INFO, "scratch_reg : %08xh\n",
+                    regs.scratch_reg);
+
+    return 0;
+}
+
+#define ZONE_PWR_UP_REQ   (1 << 10)
+#define ZONE_PWR_ON_STATE (1 << 26)
+
+static int brcm_cpu_power_on(int cpu)
+{
+    u32 tmp;
+    void __iomem *va, *pwr_ctl;
+    unsigned int timeout;
+
+    ASSERT ( cpu < NR_CPUS );
+    dprintk(XENLOG_ERR, "%s: Power on cpu %d\n", __func__, cpu);
+
+    va = ioremap_nocache(regs.cpu0_pwr_zone_ctrl, NR_CPUS * sizeof(u32));
+
+    if ( !va )
+    {
+        dprintk(XENLOG_ERR, "%s: Unable to map \"cpu0_pwr_zone_ctrl\"\n",
+                        __func__);
+        return -EFAULT;
+    }
+
+    pwr_ctl = va + (cpu * sizeof(u32) );
+
+    /* request core power on */
+    tmp = readl(pwr_ctl);
+    tmp |= ZONE_PWR_UP_REQ;
+    writel(tmp, pwr_ctl);
+
+    /*
+     * Wait for the cpu to power on.
+     * Wait a max of 10 msec.
+     */
+    timeout = 10;
+    tmp = readl(pwr_ctl);
+
+    while ( !(tmp & ZONE_PWR_ON_STATE) )
+    {
+        if ( timeout-- == 0 )
+            break;
+
+        mdelay(1);
+    }
+
+    iounmap(va);
+
+    if ( timeout == 0 )
+    {
+        dprintk(XENLOG_ERR, "CPU%d power enable failed", cpu);
+        return -ETIMEDOUT;
+    }
+
+    return 0;
+}
+
+static int brcm_cpu_release(u32 cpu)
+{
+    u32 tmp;
+    u32 __iomem *reg;
+
+    dprintk(XENLOG_INFO, "%s: Taking cpu %d out of reset \n", __func__, cpu);
+
+    reg = ioremap_nocache(regs.hif_cpu_reset_config, sizeof(u32));
+    if ( !reg )
+    {
+        dprintk(XENLOG_ERR, "%s: Unable to map \"hif_cpu_reset_config\"\n",
+                __func__);
+        return -EFAULT;
+    }
+
+    /* now take the cpu out of reset */
+    tmp = readl(reg);
+    tmp &= ~(1 << cpu);
+    writel(tmp, reg);
+
+    iounmap(reg);
+
+    return 0;
+}
+
+static int brcm_set_boot_continuation(u32 cpu, u32 pc)
+{
+    u32 __iomem *reg, index;
+    dprintk(XENLOG_INFO, "%s: cpu %d pc 0x%x\n", __func__, cpu, pc);
+
+    ASSERT (cpu < NR_CPUS);
+
+    reg = ioremap_nocache(regs.hif_boot_continuation,
+                          NR_CPUS * 2 * sizeof(u32));
+    if ( !reg )
+    {
+        dprintk(XENLOG_ERR, "%s: Unable to map \"hif_boot_continuation\"\n",
+                __func__);
+        return -EFAULT;
+    }
+
+    index = cpu * 2;
+
+    writel(0, reg + index);
+    writel(pc, reg + index + 1);
+
+    iounmap(reg);
+
+    return 0;
+}
+
+static int brcm_cpu_up(int cpu)
+{
+    u32  rc;
+
+    ASSERT (cpu < NR_CPUS);
+
+    rc = brcm_cpu_power_on(cpu);
+    if ( rc )
+        return rc;
+
+    rc = brcm_set_boot_continuation(cpu, brcm_boot_continuation_pc);
+    if ( rc )
+        return rc;
+
+   return brcm_cpu_release(cpu);
+}
+
+static int __init brcm_smp_init(void)
+{
+    u32 __iomem *scratch;
+    u32 target_pc;
+
+    scratch = ioremap_nocache(regs.scratch_reg, sizeof(u32));
+
+    if ( !scratch )
+    {
+        dprintk(XENLOG_ERR, "%s: Unable to map \"scratch_reg\"\n", __func__);
+        return -EFAULT;
+    }
+    /*
+     * The HIF CPU BIU CTRL Scratch Register is used to pass
+     * addresses between this code in xen and the boot helper.
+     * The helper puts its own entry point in the scratch register.
+     * That address is written to the cpu boot continuation registers.
+     * The helper expects xen to put xen's entry point back in the register.
+     * The helper will jump to that address.
+     * The helper is in SRAM, which will always be a 32 bit address.
+     */
+
+    brcm_boot_continuation_pc = readl(scratch);
+
+    target_pc = __pa(init_secondary);
+    writel(target_pc, scratch);
+
+    iounmap(scratch);
+
+    dprintk(XENLOG_INFO, "%s: target_pc 0x%x boot continuation pc 0x%x\n",
+        __func__, target_pc, brcm_boot_continuation_pc);
+
+    return 0;
+}
+
+static int brcm_init(void)
+{
+    return brcm_populate_plat_regs();
+}
+
+static int brcm_specific_mapping(struct domain *d)
+{
+    return 0;
+}
+
+static void brcm_reset(void)
+{
+}
+
+static uint32_t brcm_quirks(void)
+{
+    return 0;
+}
+
+static const char const *brcm_dt_compat[] __initconst =
+{
+    "brcm,bcm7445d0",
+    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:
+ */
-- 
1.9.0.138.g2de3478

Re: [PATCH 3/3] xen/arm: Add support for Broadcom 7445D0 A15 platform.

[Julien Grall]

Hello Jon,

On 09/30/2014 11:57 PM, Jon Fraser wrote:
> +static u32 brcm_boot_continuation_pc;
> +
> +static struct brcm_plat_regs regs;
> +
> +static int brcm_get_dt_node(char *compat_str, struct dt_device_node **dn,
> +                             u32 *reg_base)
> +{
> +    struct dt_device_node *node;

You don't modify the node so:

const struct dt_device_node *node;

> +    u64 reg_base_64;
> +    u64 size;
> +    int rc;
> +
> +    node = dt_find_compatible_node(NULL, NULL, compat_str);
> +    if ( !node )
> +    {
> +        dprintk(XENLOG_ERR, "%s: missing \"%s\" node\n", __func__, compat_str);
> +        return -ENOENT;
> +    }
> +
> +    rc = dt_device_get_address(node, 0, &reg_base_64, &size);

You can pass NULL instead of &size as you don't use the variable within
the function and dt_device_get_address is able to cope with NULL.

> +    if ( rc )
> +    {
> +        dprintk(XENLOG_ERR, "%s: missing \"reg\" prop\n", __func__);
> +        return rc;
> +    }
> +
> +    if ( dn )
> +        *dn = node;
> +
> +    if ( reg_base )
> +        *reg_base = (u32)(reg_base_64 & 0xffffffff);

I don't think the cast is useful here.

> +
> +    return 0;
> +}
> +
> +static int brcm_populate_plat_regs(void)
> +{
> +    int rc, failed;
> +    struct dt_device_node *node;

const struct dt_device_node *node;


> +    u32 reg_base;
> +    u32 val;
> +
> +    rc = brcm_get_dt_node("brcm,brcmstb-cpu-biu-ctrl", &node, &reg_base);
> +    if ( rc )
> +        return rc;
> +
> +    failed = 0;
> +
> +    if ( dt_property_read_u32(node, "cpu-reset-config-reg", &val) )
> +        regs.hif_cpu_reset_config = reg_base + val;
> +    else
> +    {
> +        dprintk(XENLOG_ERR, "Missing property \"cpu-reset-config-reg\"\n");
> +        failed = 1;
> +    }

I would invert the way to do it:

if ( !dt_property_read_u32(...) )
{
   dprintk(XENLOG_ERR, ...);
   goto err; /* Or return -ENOENT */
}

regs.hif_cpu_reset_config = reg_base + val;

It's easier to read as we know this is an error case and we don't need
to continue checking the other properties.

> +
> +    if ( dt_property_read_u32(node, "cpu0-pwr-zone-ctrl-reg", &val) )
> +        regs.cpu0_pwr_zone_ctrl = reg_base + val;
> +    else
> +    {
> +        dprintk(XENLOG_ERR, "Missing property \"cpu0-pwr-zone-ctrl-reg\"\n");
> +        failed = 1;
> +    }

ditto

> +    if ( dt_property_read_u32(node, "scratch-reg", &val) )
> +        regs.scratch_reg = reg_base + val;
> +    else
> +    {
> +        dprintk(XENLOG_ERR, "Missing property \"scratch-reg\"\n");
> +        failed = 1;
> +    }

ditto

> +    rc = brcm_get_dt_node("brcm,brcmstb-hif-continuation", &node, &reg_base);

You doesn't seem to use the variable "node" within the function. I would
either drop the argument in brcm_get_dt_node or pass NULL.

> +    if ( rc )
> +        return rc;
> +
> +    regs.hif_boot_continuation = reg_base;
> +
> +    if ( failed )
> +        return -ENOENT;
> +
> +    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);
> +    dprintk(XENLOG_INFO, "scratch_reg : %08xh\n",
> +                    regs.scratch_reg);
> +
> +    return 0;
> +}
> +
> +#define ZONE_PWR_UP_REQ   (1 << 10)
> +#define ZONE_PWR_ON_STATE (1 << 26)
> +
> +static int brcm_cpu_power_on(int cpu)
> +{
> +    u32 tmp;
> +    void __iomem *va, *pwr_ctl;
> +    unsigned int timeout;
> +
> +    ASSERT ( cpu < NR_CPUS );

This ASSERT is not useful, cpu will always be < NR_CPUS and we already
have a check earlier (see smp_init_cpus).

> +    dprintk(XENLOG_ERR, "%s: Power on cpu %d\n", __func__, cpu);
> +
> +    va = ioremap_nocache(regs.cpu0_pwr_zone_ctrl, NR_CPUS * sizeof(u32));

Using NR_CPUS is buggy here, the value could be very big as Xen is
compiled for multiple platform. Don't you need to map only 1 CPUs?

You could do smth like:

ioremap_nocache(regs.cpu0_ ... + (cpu * sizeof(u32)), sizeof(u32));

> +
> +    if ( !va )
> +    {
> +        dprintk(XENLOG_ERR, "%s: Unable to map \"cpu0_pwr_zone_ctrl\"\n",
> +                        __func__);
> +        return -EFAULT;
> +    }
> +
> +    pwr_ctl = va + (cpu * sizeof(u32) );

spurious white space before the closing brace.

> +
> +    /* request core power on */
> +    tmp = readl(pwr_ctl);
> +    tmp |= ZONE_PWR_UP_REQ;
> +    writel(tmp, pwr_ctl);
> +
> +    /*
> +     * Wait for the cpu to power on.
> +     * Wait a max of 10 msec.
> +     */
> +    timeout = 10;
> +    tmp = readl(pwr_ctl);
> +
> +    while ( !(tmp & ZONE_PWR_ON_STATE) )
> +    {
> +        if ( timeout-- == 0 )
> +            break;
> +
> +        mdelay(1);
> +    }

Hmmm, you read the value once before the loop and you never read again.
Didn't you forget the readl in the loop?

[..]

> +static int brcm_set_boot_continuation(u32 cpu, u32 pc)
> +{
> +    u32 __iomem *reg, index;
> +    dprintk(XENLOG_INFO, "%s: cpu %d pc 0x%x\n", __func__, cpu, pc);
> +
> +    ASSERT (cpu < NR_CPUS);

ditto for the ASSERT.

> +
> +    reg = ioremap_nocache(regs.hif_boot_continuation,
> +                          NR_CPUS * 2 * sizeof(u32));

dittor for NR_CPUS.

> +    if ( !reg )
> +    {
> +        dprintk(XENLOG_ERR, "%s: Unable to map \"hif_boot_continuation\"\n",
> +                __func__);
> +        return -EFAULT;
> +    }
> +
> +    index = cpu * 2;

Did you miss a * sizeof(u32) here?

> +    writel(0, reg + index);
> +    writel(pc, reg + index + 1);
> +
> +    iounmap(reg);
> +
> +    return 0;
> +}
> +
> +static int brcm_cpu_up(int cpu)
> +{
> +    u32  rc;

int rc;

> +
> +    ASSERT (cpu < NR_CPUS);

ditto for the ASSERT.

> +static int __init brcm_smp_init(void)
> +{

[..]

> +    dprintk(XENLOG_INFO, "%s: target_pc 0x%x boot continuation pc 0x%x\n",
> +        __func__, target_pc, brcm_boot_continuation_pc);

NIT: The second line is not correctly aligned.

> +
> +    return 0;
> +}
> +
> +static int brcm_init(void)

This callback is only used during Xen initialization so:

static __init ...

> +{
> +    return brcm_populate_plat_regs();

I guess this would be the same for brcm_populate_plat_regs?

> +}
> +
> +static int brcm_specific_mapping(struct domain *d)
> +{
> +    return 0;
> +}
> +
> +static void brcm_reset(void)
> +{
> +}
> +
> +static uint32_t brcm_quirks(void)
> +{
> +    return 0;
> +}

[..]

> +PLATFORM_START(brcm, "Broadcom B15")

[..]

> +    .quirks         = brcm_quirks,
> +    .reset          = brcm_reset,
> +    .specific_mapping    = brcm_specific_mapping,

Those functions don't contain code. You can remove them for the platform
description.

> +PLATFORM_END

Regards,


-- 
Julien Grall