| Message ID | ce360ef6ce1a39efb6690aed621af5fb1d83d377.1367188423.git.julien.grall@linaro.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Mon, 2013-04-29 at 00:01 +0100, Julien Grall wrote: > - Remove early parsing for GIC addresses > - Remove hard coded maintenance IRQ number At last, the payoff! > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > --- > xen/arch/arm/gic.c | 63 ++++++++++++++++++++++++++++------------- > xen/common/device_tree.c | 42 --------------------------- I like this line! > @@ -464,7 +486,7 @@ void gic_route_ppis(void) > { > /* XXX should get these from DT */ > /* GIC maintenance */ > - gic_route_irq(25, 1, 1u << smp_processor_id(), 0xa0); > + gic_route_dt_irq(&gic.maintenance, 1u << smp_processor_id(), 0xa0); > /* Hypervisor Timer */ > gic_route_irq(26, 1, 1u << smp_processor_id(), 0xa0); > /* Virtual Timer */ > @@ -813,7 +835,8 @@ void gic_dump_info(struct vcpu *v) > > void __cpuinit init_maintenance_interrupt(void) > { > - request_irq(25, maintenance_interrupt, 0, "irq-maintenance", NULL); > + request_irq(gic.maintenance.irq, maintenance_interrupt, > + 0, "irq-maintenance", NULL); Would a dt_request_irq be useful anywhere other than here? Ian.
On 04/29/2013 04:35 PM, Ian Campbell wrote: > On Mon, 2013-04-29 at 00:01 +0100, Julien Grall wrote: >> - Remove early parsing for GIC addresses >> - Remove hard coded maintenance IRQ number > > At last, the payoff! > >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> --- >> xen/arch/arm/gic.c | 63 ++++++++++++++++++++++++++++------------- >> xen/common/device_tree.c | 42 --------------------------- > > I like this line! > >> @@ -464,7 +486,7 @@ void gic_route_ppis(void) >> { >> /* XXX should get these from DT */ >> /* GIC maintenance */ >> - gic_route_irq(25, 1, 1u << smp_processor_id(), 0xa0); >> + gic_route_dt_irq(&gic.maintenance, 1u << smp_processor_id(), 0xa0); >> /* Hypervisor Timer */ >> gic_route_irq(26, 1, 1u << smp_processor_id(), 0xa0); >> /* Virtual Timer */ >> @@ -813,7 +835,8 @@ void gic_dump_info(struct vcpu *v) >> >> void __cpuinit init_maintenance_interrupt(void) >> { >> - request_irq(25, maintenance_interrupt, 0, "irq-maintenance", NULL); >> + request_irq(gic.maintenance.irq, maintenance_interrupt, >> + 0, "irq-maintenance", NULL); > > Would a dt_request_irq be useful anywhere other than here? > Yes. Nearly everywhere the IRQ is retrieved from the device tree (ie: UART, timer...). I will create dt_request_irq.
On 04/29/2013 04:35 PM, Ian Campbell wrote: > On Mon, 2013-04-29 at 00:01 +0100, Julien Grall wrote: >> - Remove early parsing for GIC addresses >> - Remove hard coded maintenance IRQ number > > At last, the payoff! > >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> --- >> xen/arch/arm/gic.c | 63 ++++++++++++++++++++++++++++------------- >> xen/common/device_tree.c | 42 --------------------------- > > I like this line! > >> @@ -464,7 +486,7 @@ void gic_route_ppis(void) >> { >> /* XXX should get these from DT */ >> /* GIC maintenance */ >> - gic_route_irq(25, 1, 1u << smp_processor_id(), 0xa0); >> + gic_route_dt_irq(&gic.maintenance, 1u << smp_processor_id(), 0xa0); >> /* Hypervisor Timer */ >> gic_route_irq(26, 1, 1u << smp_processor_id(), 0xa0); >> /* Virtual Timer */ >> @@ -813,7 +835,8 @@ void gic_dump_info(struct vcpu *v) >> >> void __cpuinit init_maintenance_interrupt(void) >> { >> - request_irq(25, maintenance_interrupt, 0, "irq-maintenance", NULL); >> + request_irq(gic.maintenance.irq, maintenance_interrupt, >> + 0, "irq-maintenance", NULL); > > Would a dt_request_irq be useful anywhere other than here? As all the interrupts should be retrieved from the device_tree could we remove request_irq for ARM (ie move request_irq definition to asm-x86/irq.h)? It's also a safe guard for developper to avoid hardcoded IRQ. Then we can: 1) modify irq argument type 2) rename the function in request_dt_irq I'm not sure the latter is usefull.
On Mon, 2013-04-29 at 21:42 +0100, Julien Grall wrote: > On 04/29/2013 04:35 PM, Ian Campbell wrote: > > > On Mon, 2013-04-29 at 00:01 +0100, Julien Grall wrote: > >> - Remove early parsing for GIC addresses > >> - Remove hard coded maintenance IRQ number > > > > At last, the payoff! > > > >> > >> Signed-off-by: Julien Grall <julien.grall@linaro.org> > >> --- > >> xen/arch/arm/gic.c | 63 ++++++++++++++++++++++++++++------------- > >> xen/common/device_tree.c | 42 --------------------------- > > > > I like this line! > > > >> @@ -464,7 +486,7 @@ void gic_route_ppis(void) > >> { > >> /* XXX should get these from DT */ > >> /* GIC maintenance */ > >> - gic_route_irq(25, 1, 1u << smp_processor_id(), 0xa0); > >> + gic_route_dt_irq(&gic.maintenance, 1u << smp_processor_id(), 0xa0); > >> /* Hypervisor Timer */ > >> gic_route_irq(26, 1, 1u << smp_processor_id(), 0xa0); > >> /* Virtual Timer */ > >> @@ -813,7 +835,8 @@ void gic_dump_info(struct vcpu *v) > >> > >> void __cpuinit init_maintenance_interrupt(void) > >> { > >> - request_irq(25, maintenance_interrupt, 0, "irq-maintenance", NULL); > >> + request_irq(gic.maintenance.irq, maintenance_interrupt, > >> + 0, "irq-maintenance", NULL); > > > > Would a dt_request_irq be useful anywhere other than here? > > As all the interrupts should be retrieved from the device_tree could we > remove request_irq for ARM (ie move request_irq definition to > asm-x86/irq.h)? It's also a safe guard for developper to avoid hardcoded > IRQ. Might be something to consider for 4.4, needs discussion with the x86 chaps and Keir? Since request_irq is implerment in arch code we could just skip it, then link errors would do the rest. > Then we can: > 1) modify irq argument type > 2) rename the function in request_dt_irq > > I'm not sure the latter is usefull. >
On 04/30/2013 10:34 AM, Ian Campbell wrote: > On Mon, 2013-04-29 at 21:42 +0100, Julien Grall wrote: >> On 04/29/2013 04:35 PM, Ian Campbell wrote: >> >>> On Mon, 2013-04-29 at 00:01 +0100, Julien Grall wrote: >>>> - Remove early parsing for GIC addresses >>>> - Remove hard coded maintenance IRQ number >>> >>> At last, the payoff! >>> >>>> >>>> Signed-off-by: Julien Grall <julien.grall@linaro.org> >>>> --- >>>> xen/arch/arm/gic.c | 63 ++++++++++++++++++++++++++++------------- >>>> xen/common/device_tree.c | 42 --------------------------- >>> >>> I like this line! >>> >>>> @@ -464,7 +486,7 @@ void gic_route_ppis(void) >>>> { >>>> /* XXX should get these from DT */ >>>> /* GIC maintenance */ >>>> - gic_route_irq(25, 1, 1u << smp_processor_id(), 0xa0); >>>> + gic_route_dt_irq(&gic.maintenance, 1u << smp_processor_id(), 0xa0); >>>> /* Hypervisor Timer */ >>>> gic_route_irq(26, 1, 1u << smp_processor_id(), 0xa0); >>>> /* Virtual Timer */ >>>> @@ -813,7 +835,8 @@ void gic_dump_info(struct vcpu *v) >>>> >>>> void __cpuinit init_maintenance_interrupt(void) >>>> { >>>> - request_irq(25, maintenance_interrupt, 0, "irq-maintenance", NULL); >>>> + request_irq(gic.maintenance.irq, maintenance_interrupt, >>>> + 0, "irq-maintenance", NULL); >>> >>> Would a dt_request_irq be useful anywhere other than here? >> >> As all the interrupts should be retrieved from the device_tree could we >> remove request_irq for ARM (ie move request_irq definition to >> asm-x86/irq.h)? It's also a safe guard for developper to avoid hardcoded >> IRQ. > > Might be something to consider for 4.4, needs discussion with the x86 > chaps and Keir? > > Since request_irq is implerment in arch code we could just skip it, then > link errors would do the rest. What do you mean by "implement in arch code"? Except on UART driver (pl011 and exynos4210) I don't see any usage in common code. I have also notice that I should create dt_setup_irq. The setup_irq is used in UART driver. >> Then we can: >> 1) modify irq argument type >> 2) rename the function in request_dt_irq >> >> I'm not sure the latter is usefull. >> > >
> >>> Would a dt_request_irq be useful anywhere other than here? > >> > >> As all the interrupts should be retrieved from the device_tree could we > >> remove request_irq for ARM (ie move request_irq definition to > >> asm-x86/irq.h)? It's also a safe guard for developper to avoid hardcoded > >> IRQ. > > > > Might be something to consider for 4.4, needs discussion with the x86 > > chaps and Keir? > > > > Since request_irq is implerment in arch code we could just skip it, then > > link errors would do the rest. > > What do you mean by "implement in arch code"? request_irq is implemented in xen/arch/{arm,x86}/irq.c. We could just omit ARMs version (in favour of dt_request_irq), any stray users of request_irq would trigger a linker error... > Except on UART driver > (pl011 and exynos4210) I don't see any usage in common code. > I have also notice that I should create dt_setup_irq. The setup_irq is > used in UART driver. Sounds wise. Ian.
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 1f44fea..9c8049e 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -45,6 +45,7 @@ static struct { paddr_t hbase; /* Address of virtual interface registers */ paddr_t vbase; /* Address of virtual cpu interface registers */ unsigned int lines; + struct dt_irq maintenance; /* IRQ maintenance */ unsigned int cpus; spinlock_t lock; } gic; @@ -352,28 +353,49 @@ int gic_irq_xlate(const u32 *intspec, unsigned int intsize, /* Set up the GIC */ void __init gic_init(void) { + struct dt_device_node *node; + int res; + + node = dt_find_interrupt_controller("arm,cortex-a15-gic"); + if ( !node ) + panic("Unable to find compatible GIC in the device tree\n"); + + dt_device_set_used_by(node, DT_USED_BY_XEN); + + res = dt_device_get_address(node, 0, &gic.dbase, NULL); + if ( res || !gic.dbase || (gic.dbase & ~PAGE_MASK) ) + panic("GIC: Cannot find a valid address for the distributor\n"); + + res = dt_device_get_address(node, 1, &gic.cbase, NULL); + if ( res || !gic.cbase || (gic.cbase & ~PAGE_MASK) ) + panic("GIC: Cannot find a valid address for the CPU\n"); + + res = dt_device_get_address(node, 2, &gic.hbase, NULL); + if ( res || !gic.hbase || (gic.hbase & ~PAGE_MASK) ) + panic("GIC: Cannot find a valid address for the hypervisor\n"); + + res = dt_device_get_address(node, 3, &gic.vbase, NULL); + if ( res || !gic.vbase || (gic.vbase & ~PAGE_MASK) ) + panic("GIC: Cannot find a valid address for the virtual CPU\n"); + + res = dt_device_get_irq(node, 0, &gic.maintenance); + if ( res ) + panic("GIC: Cannot find the maintenance IRQ\n"); + + /* Set the GIC as the primary interrupt controller */ + dt_interrupt_controller = node; + + /* TODO: Add check on distributor, cpu size */ + printk("GIC initialization:\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 || - !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"); - 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("GIC interfaces not page aligned.\n"); - - gic.dbase = early_info.gic.gic_dist_addr; - gic.cbase = early_info.gic.gic_cpu_addr; - gic.hbase = early_info.gic.gic_hyp_addr; - gic.vbase = early_info.gic.gic_vcpu_addr; + " gic_vcpu_addr=%"PRIpaddr"\n" + " gic_maintenance_irq=%u\n", + gic.dbase, gic.cbase, gic.hbase, gic.vbase, + gic.maintenance.irq); + set_fixmap(FIXMAP_GICD, gic.dbase >> PAGE_SHIFT, DEV_SHARED); BUILD_BUG_ON(FIXMAP_ADDR(FIXMAP_GICC1) != FIXMAP_ADDR(FIXMAP_GICC2)-PAGE_SIZE); @@ -464,7 +486,7 @@ void gic_route_ppis(void) { /* XXX should get these from DT */ /* GIC maintenance */ - gic_route_irq(25, 1, 1u << smp_processor_id(), 0xa0); + gic_route_dt_irq(&gic.maintenance, 1u << smp_processor_id(), 0xa0); /* Hypervisor Timer */ gic_route_irq(26, 1, 1u << smp_processor_id(), 0xa0); /* Virtual Timer */ @@ -813,7 +835,8 @@ void gic_dump_info(struct vcpu *v) void __cpuinit init_maintenance_interrupt(void) { - request_irq(25, maintenance_interrupt, 0, "irq-maintenance", NULL); + request_irq(gic.maintenance.irq, maintenance_interrupt, + 0, "irq-maintenance", NULL); } /* diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c index c623fe2..284b574 100644 --- a/xen/common/device_tree.c +++ b/xen/common/device_tree.c @@ -422,46 +422,6 @@ 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; - } - - 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; - interfaces = device_tree_nr_reg_ranges(prop, address_cells, size_cells); - if ( interfaces < 4 ) - { - early_printk("fdt: node `%s': not enough ranges\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; -} - static void __init process_multiboot_node(const void *fdt, int node, const char *name, u32 address_cells, u32 size_cells) @@ -513,8 +473,6 @@ 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); else if ( device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) ) process_multiboot_node(fdt, node, name, address_cells, size_cells); diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h index c897eab..a3502e0 100644 --- a/xen/include/xen/device_tree.h +++ b/xen/include/xen/device_tree.h @@ -31,13 +31,6 @@ 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_mb_module { paddr_t start; paddr_t size; @@ -52,7 +45,6 @@ struct dt_module_info { struct dt_early_info { struct dt_mem_info mem; - struct dt_gic_info gic; struct dt_module_info modules; };
- Remove early parsing for GIC addresses - Remove hard coded maintenance IRQ number Signed-off-by: Julien Grall <julien.grall@linaro.org> --- xen/arch/arm/gic.c | 63 ++++++++++++++++++++++++++++------------- xen/common/device_tree.c | 42 --------------------------- xen/include/xen/device_tree.h | 8 ------ 3 files changed, 43 insertions(+), 70 deletions(-)