Re: [PATCH] xen: get GIC addresses from DT

[David Vrabel <david.vrabel@citrix.com>]

On 23/11/12 15:21, Stefano Stabellini wrote:
> Get the address of the GIC distributor, cpu, virtual and virtual cpu
> interfaces registers from device tree.

The original intention of the early DTB parsing was to get the minimum
of info needed before the DTB could be translated into a more useful
form (similar to what Linux does).

Is this something that is still being considered in the long term?  If
so, should the GIC be initialized from this translated form, instead of
using the early parsing?

David


> 
> 
> Note: I couldn't completely get rid of GIC_BASE_ADDRESS, GIC_DR_OFFSET
> and friends because we are using them from mode_switch.S, that is
> executed before device tree has been parsed. But at least mode_switch.S
> is known to contain vexpress specific code anyway.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
[...]
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index da0af77..79b1dd1 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -54,6 +54,27 @@ bool_t device_tree_type_matches(const void *fdt, int node, const char *match)
>      return !strncmp(prop, match, len);
>  }
>  
> +bool_t device_tree_node_compatible(const void *fdt, int node, const char *match)
> +{
> +    int len, l;
> +    const void *prop;
> +
> +    prop = fdt_getprop(fdt, node, "compatible", &len);
> +    if ( prop == NULL )
> +        return 0;
> +
> +    while ( len > 0 ) {
> +        if ( !strncmp(prop, match, strlen(match)) )
> +            return 1;
> +        l = strlen(prop) + 1;
> +        prop += l;
> +        len -= l;
> +    }

This doesn't look right.  Don't you need to split the prop string on
comma boundaries?

> +
> +    return 0;
> +}
> +
> +
>  static void __init get_val(const u32 **cell, u32 cells, u64 *val)
>  {
>      *val = 0;
> @@ -270,6 +291,50 @@ static void __init process_cpu_node(const void *fdt, int node,
>      cpumask_set_cpu(start, &cpu_possible_map);
>  }
>  
> +static void __init process_gic_node(const void *fdt, int node,
> +                                    const char *name,
> +                                    u32 address_cells, u32 size_cells)
> +{
> +    const struct fdt_property *prop;
> +    const u32 *cell;
> +    paddr_t start, size;
> +    int reg_cells, interfaces;
> +
> +    if ( address_cells < 1 || size_cells < 1 )
> +    {
> +        early_printk("fdt: node `%s': invalid #address-cells or #size-cells",
> +                     name);
> +        return;
> +    }
> +
> +    prop = fdt_get_property(fdt, node, "reg", NULL);
> +    if ( !prop )
> +    {
> +        early_printk("fdt: node `%s': missing `reg' property\n", name);
> +        return;
> +    }
> +
> +    cell = (const u32 *)prop->data;
> +    device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);

Is this needed?  This cell is reread below.

> +
> +    cell = (const u32 *)prop->data;
> +    reg_cells = address_cells + size_cells;
> +    interfaces = fdt32_to_cpu(prop->len) / (reg_cells * sizeof(u32));

Perhaps wrap this in a helper function?  Look like it might be useful
elsewhere.

> +    if ( interfaces < 4 )
> +    {
> +        early_printk("fdt: node `%s': invalid `reg' property\n", name);
> +        return;
> +    }
> +    device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
> +    early_info.gic.gic_dist_addr = start;
> +    device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
> +    early_info.gic.gic_cpu_addr = start;
> +    device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
> +    early_info.gic.gic_hyp_addr = start;
> +    device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
> +    early_info.gic.gic_vcpu_addr = start;

Is the GIC driver still hardcoding the register region sizes?  Or does
it not need the size?

> +}
> +
>  static int __init early_scan_node(const void *fdt,
>                                    int node, const char *name, int depth,
>                                    u32 address_cells, u32 size_cells,
> @@ -279,6 +344,8 @@ static int __init early_scan_node(const void *fdt,
>          process_memory_node(fdt, node, name, address_cells, size_cells);
>      else if ( device_tree_type_matches(fdt, node, "cpu") )
>          process_cpu_node(fdt, node, name, address_cells, size_cells);
> +    else if ( device_tree_node_compatible(fdt, node, "arm,cortex-a15-gic") )
> +        process_gic_node(fdt, node, name, address_cells, size_cells);

Long term, I don't think you want a long list of all the different
interrupt controllers here.

David