[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2] xen: get GIC addresses from DT
On Tue, 4 Dec 2012, Ian Campbell wrote: > On Fri, 2012-11-30 at 14:28 +0000, Stefano Stabellini wrote: > > Get the address of the GIC distributor, cpu, virtual and virtual cpu > > interfaces registers from device tree. > > > > 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. > > > > > > Changes in v2: > > - remove 2 superflous lines from process_gic_node; > > - introduce device_tree_get_reg_ranges; > > - add a check for uninitialized GIC interface addresses; > > - add a check for non-page aligned GIC interface addresses; > > - remove the code to deal with non-page aligned addresses from GICC and > > GICH. > > > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx> > > > > > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > > index 0c6fab9..2b29e7e 100644 > > --- a/xen/arch/arm/gic.c > > +++ b/xen/arch/arm/gic.c > > @@ -26,6 +26,7 @@ > > #include <xen/errno.h> > > #include <xen/softirq.h> > > #include <xen/list.h> > > +#include <xen/device_tree.h> > > #include <asm/p2m.h> > > #include <asm/domain.h> > > > > @@ -33,10 +34,8 @@ > > > > /* Access to the GIC Distributor registers through the fixmap */ > > #define GICD ((volatile uint32_t *) FIXMAP_ADDR(FIXMAP_GICD)) > > -#define GICC ((volatile uint32_t *) (FIXMAP_ADDR(FIXMAP_GICC1) \ > > - + (GIC_CR_OFFSET & 0xfff))) > > -#define GICH ((volatile uint32_t *) (FIXMAP_ADDR(FIXMAP_GICH) \ > > - + (GIC_HR_OFFSET & 0xfff))) > > +#define GICC ((volatile uint32_t *) FIXMAP_ADDR(FIXMAP_GICC1)) > > +#define GICH ((volatile uint32_t *) FIXMAP_ADDR(FIXMAP_GICH)) > > static void gic_restore_pending_irqs(struct vcpu *v); > > > > /* Global state */ > > @@ -44,6 +43,7 @@ static struct { > > paddr_t dbase; /* Address of distributor registers */ > > paddr_t cbase; /* Address of CPU interface registers */ > > paddr_t hbase; /* Address of virtual interface registers */ > > + paddr_t vbase; /* Address of virtual cpu interface registers */ > > unsigned int lines; > > unsigned int cpus; > > spinlock_t lock; > > @@ -306,10 +306,27 @@ static void __cpuinit gic_hyp_disable(void) > > /* Set up the GIC */ > > void __init gic_init(void) > > { > > - /* XXX FIXME get this from devicetree */ > > - gic.dbase = GIC_BASE_ADDRESS + GIC_DR_OFFSET; > > - gic.cbase = GIC_BASE_ADDRESS + GIC_CR_OFFSET; > > - gic.hbase = GIC_BASE_ADDRESS + GIC_HR_OFFSET; > > + if ( !early_info.gic.gic_dist_addr || > > + !early_info.gic.gic_cpu_addr || > > + !early_info.gic.gic_hyp_addr || > > + !early_info.gic.gic_vcpu_addr ) > > + panic("the physical address of one of the GIC interfaces is > > missing:\n" > > + " gic_dist_addr=%"PRIpaddr"\n" > > + " gic_cpu_addr=%"PRIpaddr"\n" > > + " gic_hyp_addr=%"PRIpaddr"\n" > > + " gic_vcpu_addr=%"PRIpaddr"\n", > > + early_info.gic.gic_dist_addr, early_info.gic.gic_cpu_addr, > > + early_info.gic.gic_hyp_addr, early_info.gic.gic_vcpu_addr); > > + if ( (early_info.gic.gic_dist_addr & ~PAGE_MASK) || > > + (early_info.gic.gic_cpu_addr & ~PAGE_MASK) || > > + (early_info.gic.gic_hyp_addr & ~PAGE_MASK) || > > + (early_info.gic.gic_vcpu_addr & ~PAGE_MASK) ) > > + panic("error: GIC interfaces not page aligned.\n"); > > Oh, maybe I should have skipped "arm: add few checks to gic_init" and > come straight here instead? Yep. I am going to address the other comments too. > > static void maintenance_interrupt(int irq, void *dev_id, struct > > cpu_user_regs *regs) > > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c > > index da0af77..8d5b6b0 100644 > > --- a/xen/common/device_tree.c > > +++ b/xen/common/device_tree.c > > @@ -54,6 +54,33 @@ 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; > > This will decide that match="foo-bar" is compatible with a node which > contains compatible = "foo-bar-baz". Is that deliberate? I thought the > DT way would be to have compatible = "foo-bar-baz", "foo-bar" ? > > Perhaps this is just due to cut-n-paste from device_tree_node_matches > (where it makes sense, I think)? You are right, I think that we should have a perfect match for the compatible string. > I now wonder the same thing about device_tree_type_matches too... I think we should use strcmp in both cases. I'll send another patch for that. > > + l = strlen(prop) + 1; > > + prop += l; > > + len -= l; > > + } > > + > > + return 0; > > +} > > + > > +static void device_tree_get_reg_ranges(const struct fdt_property *prop, > > "nr" in the name somewhere? I thought at first this was somehow > returning the ranges themselves. OK > > + u32 address_cells, u32 size_cells, int *ranges) > > +{ > > + u32 reg_cells = address_cells + size_cells; > > + *ranges = fdt32_to_cpu(prop->len) / (reg_cells * sizeof(u32)); > > +} > > + > > static void __init get_val(const u32 **cell, u32 cells, u64 *val) > > { > > *val = 0; > > @@ -209,7 +236,6 @@ static void __init process_memory_node(const void *fdt, > > int node, > > u32 address_cells, u32 size_cells) > > { > > const struct fdt_property *prop; > > - size_t reg_cells; > > int i; > > int banks; > > const u32 *cell; > > @@ -230,8 +256,7 @@ static void __init process_memory_node(const void *fdt, > > int node, > > } > > > > cell = (const u32 *)prop->data; > > - reg_cells = address_cells + size_cells; > > - banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof(u32)); > > + device_tree_get_reg_ranges(prop, address_cells, size_cells, &banks); > > > > for ( i = 0; i < banks && early_info.mem.nr_banks < NR_MEM_BANKS; i++ ) > > { > > @@ -270,6 +295,46 @@ 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 interfaces; > > + > > + if ( address_cells < 1 || size_cells < 1 ) > > + { > > + early_printk("fdt: node `%s': invalid #address-cells or > > #size-cells", > > + name); > > + return; > > + } > > Is this the sort of check which the generic DT walker helper thing ought > to include? No, sometimes address_cells < 1 or size_cells < 1 are completely acceptable, see process_cpu_node for example. > > + > > + 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_ranges(prop, address_cells, size_cells, > > &interfaces); > > + if ( interfaces < 4 ) > > + { > > + early_printk("fdt: node `%s': invalid `reg' property\n", name); > > A more specific message would be "not enough ranges in ..." OK _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |