[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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?

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

I now wonder the same thing about device_tree_type_matches too...

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

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

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

> +        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;
> +}
> +
>  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);
>  
>      return 0;
>  }
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 4d010c0..a0e3a97 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -26,8 +26,16 @@ struct dt_mem_info {
>      struct membank bank[NR_MEM_BANKS];
>  };
>  
> +struct dt_gic_info {
> +    paddr_t gic_dist_addr;
> +    paddr_t gic_cpu_addr;
> +    paddr_t gic_hyp_addr;
> +    paddr_t gic_vcpu_addr;
> +};
> +
>  struct dt_early_info {
>      struct dt_mem_info mem;
> +    struct dt_gic_info gic;
>  };
>  
>  typedef int (*device_tree_node_func)(const void *fdt,



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.