| Message ID | 6e7f063528064751438ffa12fea9c8f5229be78d.1367979526.git.julien.grall@linaro.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Wed, 2013-05-08 at 03:33 +0100, Julien Grall wrote: > - gic_route_irq_to_guest takes a dt_irq instead of an IRQ number > - remove hardcoded address/IRQ > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > > Changes in v2: > - Use the new function dt_irq_is_level_trigger > - Disable DEBUG_DT by default > - Rename parse_device_tree to map_device_from_device_tree Should be map_devices_... > + /** Javadoc! > + * Don't map IRQ that have no physical meaning > + * ie: IRQ whose controller is not the GIC > + */ > + if ( rirq.controller != dt_interrupt_controller ) > + { > + DPRINT("irq %u skipped it controller (%s)\n", you might mean "skipped its controller" ? But I think a clearer message would be "irq %u not connected to primary controller" or something. > + i, dt_node_full_name(rirq.controller)); > + continue; > + } > + > + res = dt_irq_translate(&rirq, &irq); > + if ( res ) > + { > + printk(XENLOG_ERR "Unable to translate irq %u for %s\n", > + i, dt_node_full_name(dev)); > + return res; > + } > + > + DPRINT("irq %u = %u type = %#x\n", i, irq.irq, irq.type); The # format specifier is required by $STANDARD to not be all that sensible and/or consistent when when the value is 0, i.e. printf("%#x\n", 0) => "0" printf("%#x\n", 1) => "0x1" Worse if you use widths then: printf("%#02x\n", 0); => "00" printf("%#02x\n", 1); => "0x1" printf("%#04x\n", 0); => "0000" printf("%#04x\n", 1); => "0x01" For this reason we tend to avoid # and just use "0x%...", assuming irq==0 is a possibility, and likewise below addr==0 it's probably better to avoid #. > + /* Don't check return because the IRQ can be use by multiple device */ "used by" > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index f7b9889..ddad0c8 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -692,13 +692,14 @@ void gic_inject(void) > gic_inject_irq_start(); > } > > -int gic_route_irq_to_guest(struct domain *d, unsigned int irq, > +int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq, > const char * devname) > { > struct irqaction *action; > - struct irq_desc *desc = irq_to_desc(irq); > + struct irq_desc *desc = irq_to_desc(irq->irq); > unsigned long flags; > int retval; > + bool_t level; > > action = xmalloc(struct irqaction); > if (!action) > @@ -706,6 +707,7 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int irq, > > action->dev_id = d; > action->name = devname; > + action->free_on_release = 1; > > spin_lock_irqsave(&desc->lock, flags); > spin_lock(&gic.lock); > @@ -713,9 +715,11 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int irq, > desc->handler = &gic_guest_irq_type; > desc->status |= IRQ_GUEST; > > - gic_set_irq_properties(irq, 1, 1u << smp_processor_id(), 0xa0); > + level = dt_irq_is_level_trigger(irq); > + > + gic_set_irq_properties(irq->irq, level, 1u << smp_processor_id(), 0xa0); By the end of this series are all callers going through the above dance? git_set_irq_properties could take a dt_irq? Ian.
On 05/08/2013 04:28 PM, Ian Campbell wrote: > On Wed, 2013-05-08 at 03:33 +0100, Julien Grall wrote: >> - gic_route_irq_to_guest takes a dt_irq instead of an IRQ number >> - remove hardcoded address/IRQ >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> >> Changes in v2: >> - Use the new function dt_irq_is_level_trigger >> - Disable DEBUG_DT by default >> - Rename parse_device_tree to map_device_from_device_tree > > Should be map_devices_... Will be fix on the next patch series. >> + /** > > Javadoc! > >> + * Don't map IRQ that have no physical meaning >> + * ie: IRQ whose controller is not the GIC >> + */ >> + if ( rirq.controller != dt_interrupt_controller ) >> + { >> + DPRINT("irq %u skipped it controller (%s)\n", > > you might mean "skipped its controller" ? But I think a clearer message > would be "irq %u not connected to primary controller" or something. > >> + i, dt_node_full_name(rirq.controller)); >> + continue; >> + } >> + >> + res = dt_irq_translate(&rirq, &irq); >> + if ( res ) >> + { >> + printk(XENLOG_ERR "Unable to translate irq %u for %s\n", >> + i, dt_node_full_name(dev)); >> + return res; >> + } >> + >> + DPRINT("irq %u = %u type = %#x\n", i, irq.irq, irq.type); > > The # format specifier is required by $STANDARD to not be all that > sensible and/or consistent when when the value is 0, i.e. > printf("%#x\n", 0) => "0" > printf("%#x\n", 1) => "0x1" > Worse if you use widths then: > printf("%#02x\n", 0); => "00" > printf("%#02x\n", 1); => "0x1" > printf("%#04x\n", 0); => "0000" > printf("%#04x\n", 1); => "0x01" > > For this reason we tend to avoid # and just use "0x%...", assuming > irq==0 is a possibility, and likewise below addr==0 it's probably better > to avoid #. Thanks for this hint. I will replace all my %#x by 0x%x. >> + /* Don't check return because the IRQ can be use by multiple device */ > > "used by" > >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >> index f7b9889..ddad0c8 100644 >> --- a/xen/arch/arm/gic.c >> +++ b/xen/arch/arm/gic.c >> @@ -692,13 +692,14 @@ void gic_inject(void) >> gic_inject_irq_start(); >> } >> >> -int gic_route_irq_to_guest(struct domain *d, unsigned int irq, >> +int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq, >> const char * devname) >> { >> struct irqaction *action; >> - struct irq_desc *desc = irq_to_desc(irq); >> + struct irq_desc *desc = irq_to_desc(irq->irq); >> unsigned long flags; >> int retval; >> + bool_t level; >> >> action = xmalloc(struct irqaction); >> if (!action) >> @@ -706,6 +707,7 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int irq, >> >> action->dev_id = d; >> action->name = devname; >> + action->free_on_release = 1; >> >> spin_lock_irqsave(&desc->lock, flags); >> spin_lock(&gic.lock); >> @@ -713,9 +715,11 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int irq, >> desc->handler = &gic_guest_irq_type; >> desc->status |= IRQ_GUEST; >> >> - gic_set_irq_properties(irq, 1, 1u << smp_processor_id(), 0xa0); >> + level = dt_irq_is_level_trigger(irq); >> + >> + gic_set_irq_properties(irq->irq, level, 1u << smp_processor_id(), 0xa0); > > By the end of this series are all callers going through the above dance? > git_set_irq_properties could take a dt_irq? No. I can add patch to use a dt_irq.
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 6581492..0b762a9 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -14,6 +14,7 @@ #include <asm/setup.h> #include <asm/gic.h> +#include <xen/irq.h> #include "kernel.h" static unsigned int __initdata opt_dom0_max_vcpus; @@ -30,6 +31,14 @@ static void __init parse_dom0_mem(const char *s) } custom_param("dom0_mem", parse_dom0_mem); +//#define DEBUG_DT + +#ifdef DEBUG_DT +# define DPRINT(fmt, args...) printk(XENLOG_DEBUG fmt, ##args) +#else +# define DPRINT(fmt, args...) do {} while ( 0 ) +#endif + /* * Amount of extra space required to dom0's device tree. No new nodes * are added (yet) but one terminating reserve map entry (16 bytes) is @@ -303,6 +312,122 @@ static int write_nodes(struct domain *d, struct kernel_info *kinfo, return 0; } +/* Map the device in the domain */ +static int map_device(struct domain *d, const struct dt_device_node *dev) +{ + unsigned int nirq; + unsigned int naddr; + unsigned int i; + int res; + struct dt_irq irq; + struct dt_raw_irq rirq; + u64 addr, size; + + nirq = dt_number_of_irq(dev); + naddr = dt_number_of_address(dev); + + DPRINT("%s nirq = %d naddr = %u\n", dt_node_full_name(dev), nirq, naddr); + + /* Map IRQs */ + for ( i = 0; i < nirq; i++ ) + { + res = dt_device_get_raw_irq(dev, i, &rirq); + if ( res ) + { + printk(XENLOG_ERR "Unable to retrieve irq %u for %s\n", + i, dt_node_full_name(dev)); + return res; + } + + /** + * Don't map IRQ that have no physical meaning + * ie: IRQ whose controller is not the GIC + */ + if ( rirq.controller != dt_interrupt_controller ) + { + DPRINT("irq %u skipped it controller (%s)\n", + i, dt_node_full_name(rirq.controller)); + continue; + } + + res = dt_irq_translate(&rirq, &irq); + if ( res ) + { + printk(XENLOG_ERR "Unable to translate irq %u for %s\n", + i, dt_node_full_name(dev)); + return res; + } + + DPRINT("irq %u = %u type = %#x\n", i, irq.irq, irq.type); + /* Don't check return because the IRQ can be use by multiple device */ + gic_route_irq_to_guest(d, &irq, dt_node_name(dev)); + } + + /* Map the address ranges */ + for ( i = 0; i < naddr; i++ ) + { + res = dt_device_get_address(dev, i, &addr, &size); + if ( res ) + { + printk(XENLOG_ERR "Unable to retrieve address %u for %s\n", + i, dt_node_full_name(dev)); + return res; + } + + DPRINT("addr %u = %#"PRIx64" - %#"PRIx64"\n", i, addr, addr + size - 1); + + res = map_mmio_regions(d, addr & PAGE_MASK, + PAGE_ALIGN(addr + size) - 1, + addr & PAGE_MASK); + if ( res ) + { + printk(XENLOG_ERR "Unable to map %#"PRIx64" - %#"PRIx64" in dom0\n", + addr & PAGE_MASK, PAGE_ALIGN(addr + size) - 1); + return res; + } + } + + return 0; +} + +static int handle_node(struct domain *d, const struct dt_device_node *np) +{ + const struct dt_device_node *child; + int res; + + DPRINT("handle %s\n", dt_node_full_name(np)); + + /* Skip theses nodes and the sub-nodes */ + if ( dt_device_is_compatible(np, "xen,xen") || + dt_device_type_is_equal(np, "memory") || + !strcmp("/chosen", dt_node_full_name(np)) ) + return 0; + + if ( dt_device_used_by(np) != DT_USED_BY_XEN ) + { + res = map_device(d, np); + + if ( res ) + return res; + } + + for ( child = np->child; child != NULL; child = child->sibling ) + { + res = handle_node(d, child); + if ( res ) + return res; + } + + return 0; +} + +static int map_device_from_device_tree(struct domain *d) +{ + ASSERT(dt_host && (dt_host->sibling == NULL)); + + return handle_node(d, dt_host); +} + static int prepare_dtb(struct domain *d, struct kernel_info *kinfo) { void *fdt; @@ -385,24 +510,7 @@ int construct_dom0(struct domain *d) if ( rc < 0 ) return rc; - printk("Map CS2 MMIO regions 1:1 in the P2M %#llx->%#llx\n", 0x18000000ULL, 0x1BFFFFFFULL); - map_mmio_regions(d, 0x18000000, 0x1BFFFFFF, 0x18000000); - printk("Map CS3 MMIO regions 1:1 in the P2M %#llx->%#llx\n", 0x1C000000ULL, 0x1FFFFFFFULL); - map_mmio_regions(d, 0x1C000000, 0x1FFFFFFF, 0x1C000000); - - printk("Routing peripheral interrupts to guest\n"); - /* TODO Get from device tree */ - gic_route_irq_to_guest(d, 34, "timer0"); - /*gic_route_irq_to_guest(d, 37, "uart0"); -- XXX used by Xen*/ - gic_route_irq_to_guest(d, 38, "uart1"); - gic_route_irq_to_guest(d, 39, "uart2"); - gic_route_irq_to_guest(d, 40, "uart3"); - gic_route_irq_to_guest(d, 41, "mmc0-1"); - gic_route_irq_to_guest(d, 42, "mmc0-2"); - gic_route_irq_to_guest(d, 44, "keyboard"); - gic_route_irq_to_guest(d, 45, "mouse"); - gic_route_irq_to_guest(d, 46, "lcd"); - gic_route_irq_to_guest(d, 47, "eth"); + map_device_from_device_tree(d); /* The following loads use the domain's p2m */ p2m_load_VTTBR(d); diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index f7b9889..ddad0c8 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -692,13 +692,14 @@ void gic_inject(void) gic_inject_irq_start(); } -int gic_route_irq_to_guest(struct domain *d, unsigned int irq, +int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq, const char * devname) { struct irqaction *action; - struct irq_desc *desc = irq_to_desc(irq); + struct irq_desc *desc = irq_to_desc(irq->irq); unsigned long flags; int retval; + bool_t level; action = xmalloc(struct irqaction); if (!action) @@ -706,6 +707,7 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int irq, action->dev_id = d; action->name = devname; + action->free_on_release = 1; spin_lock_irqsave(&desc->lock, flags); spin_lock(&gic.lock); @@ -713,9 +715,11 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int irq, desc->handler = &gic_guest_irq_type; desc->status |= IRQ_GUEST; - gic_set_irq_properties(irq, 1, 1u << smp_processor_id(), 0xa0); + level = dt_irq_is_level_trigger(irq); + + gic_set_irq_properties(irq->irq, level, 1u << smp_processor_id(), 0xa0); - retval = __setup_irq(desc, irq, action); + retval = __setup_irq(desc, irq->irq, action); if (retval) { xfree(action); goto out; diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h index e7608dc..513c1fc 100644 --- a/xen/include/asm-arm/gic.h +++ b/xen/include/asm-arm/gic.h @@ -157,7 +157,8 @@ extern int gic_events_need_delivery(void); extern void __cpuinit init_maintenance_interrupt(void); extern void gic_set_guest_irq(struct vcpu *v, unsigned int irq, unsigned int state, unsigned int priority); -extern int gic_route_irq_to_guest(struct domain *d, unsigned int irq, +extern int gic_route_irq_to_guest(struct domain *d, + const struct dt_irq *irq, const char * devname); /* Accept an interrupt from the GIC and dispatch its handler */
- gic_route_irq_to_guest takes a dt_irq instead of an IRQ number - remove hardcoded address/IRQ Signed-off-by: Julien Grall <julien.grall@linaro.org> Changes in v2: - Use the new function dt_irq_is_level_trigger - Disable DEBUG_DT by default - Rename parse_device_tree to map_device_from_device_tree --- xen/arch/arm/domain_build.c | 144 +++++++++++++++++++++++++++++++++++++------ xen/arch/arm/gic.c | 12 ++-- xen/include/asm-arm/gic.h | 3 +- 3 files changed, 136 insertions(+), 23 deletions(-)