Re: [Xen-devel] [PATCH] xen: get GIC addresses from DT

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 30 Nov 2012, David Vrabel wrote:
> 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?

Regardless of the final form of DT parsing in Xen, I think that CPU,
memory and GIC are the three things that should be parsed early.


> > 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@xxxxxxxxxxxxx>
> > 
> [...]
> > 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?

Nope, the compatible string to match is, for example, the entirety of
"arm,cortex-a15-gic", or alternatively "arm,cortex-a9-gic". Trying to
match just "cortex-a15-gic" would be incorrect.
This is what Linux does too.


> > +    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.

Yes, because device_tree_get_reg increments the cell pointer

> > +
> > +    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.

OK


> > +    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?

Yes, it is. However we do know the size of all the GIC interfaces
because they are specified in the GIC docs.


> > +}
> > +
> >  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.

Well, in practice there is just one interrupt controller in the ARMv7
and ARMv8 worlds right now, that is the GIC. I don't think that's going
to change anytime soon.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel