|
[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 |