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

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

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