Re: [PATCH 01/10] xen/arm: Implement hip04-d01 platform

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

Hi Frediano,

On 11/03/2014 10:11 AM, Frediano Ziglio wrote:
> Add this new platform to Xen.
> This platform require specific code to initialize CPUs.

s/require/requires/

I guess your platform doesn't support PSCI?

> 
> Signed-off-by: Frediano Ziglio <frediano.ziglio@huawei.com>
> Signed-off-by: Zoltan Kiss <zoltan.kiss@huawei.com>
> ---

[..]

> diff --git a/xen/arch/arm/platforms/hip04.c b/xen/arch/arm/platforms/hip04.c
> new file mode 100644
> index 0000000..bf38c23
> --- /dev/null
> +++ b/xen/arch/arm/platforms/hip04.c
> @@ -0,0 +1,258 @@

[..]

> +
> +struct hip04_secondary_cpu_data {

coding style:

struct hip04_secondary_cpu_data
{

> +    u32 bootwrapper_phys;
> +    u32 bootwrapper_size;
> +    u32 bootwrapper_magic;
> +    u32 relocation_entry;
> +    u32 relocation_size;
> +};
> +
> +static void __iomem *relocation, *sysctrl, *fabric;
> +static int hip04_cpu_table[HIP04_MAX_CLUSTERS][HIP04_MAX_CPUS_PER_CLUSTER];
> +static struct hip04_secondary_cpu_data hip04_boot;
> +
> +static void hip04_reset(void)
> +{
> +    /* TODO */

Why did you implement the reset in a separate patch rather than here?

> +}
> +
> +static void hip04_set_snoop_filter(unsigned int cluster, unsigned int on)
> +{
> +    unsigned long data;
> +
> +    if (!fabric)

Coding style:

if ( !fabric )

> +        return;
> +    data = readl_relaxed(fabric + FAB_SF_MODE);
> +    if (on)

if ( on )

> +        data |= 1 << cluster;
> +    else
> +        data &= ~(1 << cluster);
> +    writel_relaxed(data, fabric + FAB_SF_MODE);
> +    while (1) {

while ( 1 )
{

> +        if (data == readl_relaxed(fabric + FAB_SF_MODE))

if ( ... )

> +            break;
> +    }

The loop is turning in an infinite loop if the reading value is never
correct.

> +}
> +
> +static bool __init hip04_cpu_table_init(void)
> +{
> +    unsigned int mpidr, cpu, cluster;
> +
> +    mpidr = cpu_logical_map(smp_processor_id());
> +    cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> +    cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> +
> +    if (cluster >= HIP04_MAX_CLUSTERS ||
> +        cpu >= HIP04_MAX_CPUS_PER_CLUSTER) {

if ( ...
     ... )
{

> +        printk(XENLOG_ERR "%s: boot CPU is out of bound!\n", __func__);
> +        return false;
> +    }
> +    hip04_set_snoop_filter(cluster, 1);
> +    hip04_cpu_table[cluster][cpu] = 1;

Missing blank line

> +    return true;
> +}
> +
> +static bool hip04_cluster_down(unsigned int cluster)
> +{
> +    int i;
> +
> +    for (i = 0; i < HIP04_MAX_CPUS_PER_CLUSTER; i++)

for ( ... )

> +        if (hip04_cpu_table[cluster][i])

if ( ... )

> +            return false;
> +    return true;
> +}
> +
> +static void hip04_cluster_up(unsigned int cluster)
> +{
> +    unsigned long data, mask;
> +
> +    if ( hip04_cluster_down(cluster) ) {

Wouldn't it be easier to return if the cluster is up? It will one layer
of indentation.

> +        data = CLUSTER_L2_RESET_BIT | CLUSTER_DEBUG_RESET_BIT;
> +        writel_relaxed(data, sysctrl + SC_CPU_RESET_DREQ(cluster));
> +        do {
> +            mask = CLUSTER_L2_RESET_STATUS | \
> +                   CLUSTER_DEBUG_RESET_STATUS;
> +            data = readl_relaxed(sysctrl + \
> +                         SC_CPU_RESET_STATUS(cluster));
> +        } while (data & mask);
> +        hip04_set_snoop_filter(cluster, 1);
> +    }
> +}
> +
> +static int __init hip04_smp_init(void)
> +{
> +    struct dt_device_node *np, *np_fab;

The device node are not modified so:

const struct

[..]

> +    writel_relaxed(hip04_boot.bootwrapper_phys, relocation);
> +    writel_relaxed(hip04_boot.bootwrapper_magic, relocation + 4);
> +    writel_relaxed(__pa(init_secondary), relocation + 8);
> +    writel_relaxed(0, relocation + 12);
> +
> +    return 0;
> +
> +err:

For consistence, you should unmap everything you mapped with ioremap_*

> +    printk("%s", msg);
> +    return -ENXIO;
> +}
> +
> +static int hip04_cpu_up(int cpu)
> +{
> +    unsigned int cluster = cpu / 4;

The number 4 is confusing here, why not using the define you've created
above? Such as  HIP04_MAX_CPUS_PER_CLUSTER

> +    unsigned long data;

The coding style requires a blank line after the declarations block.

> +    cpu %= 4;

Ditto for the number 4.

Regards,

-- 
Julien Grall