[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] xen: get GIC addresses from DT
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |