| Message ID | 1434703572-26221-7-git-send-email-hanjun.guo@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jun 19, 2015 at 09:46:09AM +0100, Hanjun Guo wrote: > Introduce acpi_irq_domain for GICv2 core domain instead of referring > to the irq_default_domain, based on that, pass gsi as the argument and > get the gsi in gic_irq_domain_alloc() to add stacked irqdomain support > for ACPI based GICv2 init. This commit log does not parse and we can read the code, there is no point in trying to rewrite it here. > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > --- > drivers/acpi/gsi.c | 28 +++++++++++++--------------- > drivers/irqchip/irq-gic.c | 32 +++++++++++++++++--------------- > include/linux/irqchip/arm-gic-acpi.h | 2 ++ > 3 files changed, 32 insertions(+), 30 deletions(-) > > diff --git a/drivers/acpi/gsi.c b/drivers/acpi/gsi.c > index 38208f2..55b5f31 100644 > --- a/drivers/acpi/gsi.c > +++ b/drivers/acpi/gsi.c > @@ -3,6 +3,7 @@ > * > * Copyright (C) 2015 ARM Ltd. > * Author: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > + * Hanjun Guo <hanjun.guo@linaro.org> for stacked irqdomains support > * > * This program is free software; you can redistribute it and/or modify > * it under the terms of the GNU General Public License version 2 as > @@ -13,6 +14,8 @@ > #include <linux/irqdomain.h> > > enum acpi_irq_model_id acpi_irq_model; > +/* ACPI core domian pointing to GICv2/3 core domain */ This code is not GIC specific and does not have to be. > +struct irq_domain *acpi_irq_domain __read_mostly; Make it static and create helpers to set it. > static unsigned int acpi_gsi_get_irq_type(int trigger, int polarity) > { > @@ -45,12 +48,7 @@ static unsigned int acpi_gsi_get_irq_type(int trigger, int polarity) > */ > int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) > { > - /* > - * Only default domain is supported at present, always find > - * the mapping corresponding to default domain by passing NULL > - * as irq_domain parameter > - */ > - *irq = irq_find_mapping(NULL, gsi); > + *irq = irq_find_mapping(acpi_irq_domain, gsi); > /* > * *irq == 0 means no mapping, that should > * be reported as a failure > @@ -72,16 +70,16 @@ EXPORT_SYMBOL_GPL(acpi_gsi_to_irq); > int acpi_register_gsi(struct device *dev, u32 gsi, int trigger, > int polarity) > { > - unsigned int irq; > + int irq; > unsigned int irq_type = acpi_gsi_get_irq_type(trigger, polarity); > > - /* > - * There is no way at present to look-up the IRQ domain on ACPI, > - * hence always create mapping referring to the default domain > - * by passing NULL as irq_domain parameter > - */ > - irq = irq_create_mapping(NULL, gsi); > - if (!irq) > + irq = irq_find_mapping(acpi_irq_domain, gsi); > + if (irq > 0) > + return irq; > + > + irq = irq_domain_alloc_irqs(acpi_irq_domain, 1, dev_to_node(dev), > + &gsi); > + if (irq <= 0) > return -EINVAL; > > /* Set irq type if specified and different than the current one */ > @@ -98,7 +96,7 @@ EXPORT_SYMBOL_GPL(acpi_register_gsi); > */ > void acpi_unregister_gsi(u32 gsi) > { > - int irq = irq_find_mapping(NULL, gsi); > + int irq = irq_find_mapping(acpi_irq_domain, gsi); > > irq_dispose_mapping(irq); > } > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > index 8fc67bc..d1b2131 100644 > --- a/drivers/irqchip/irq-gic.c > +++ b/drivers/irqchip/irq-gic.c > @@ -851,15 +851,22 @@ static struct notifier_block gic_cpu_notifier = { > static int gic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, > unsigned int nr_irqs, void *arg) > { > - int i, ret; > + int i; > irq_hw_number_t hwirq; > - unsigned int type = IRQ_TYPE_NONE; > - struct of_phandle_args *irq_data = arg; > > - ret = gic_irq_domain_xlate(domain, irq_data->np, irq_data->args, > - irq_data->args_count, &hwirq, &type); > - if (ret) > - return ret; > + if (domain->of_node) { /* DT case */ > + int ret; > + unsigned int type = IRQ_TYPE_NONE; > + struct of_phandle_args *irq_data = arg; > + > + ret = gic_irq_domain_xlate(domain, irq_data->np, > + irq_data->args, > + irq_data->args_count, &hwirq, &type); > + if (ret) > + return ret; > + } else { /* ACPI case */ > + hwirq = (irq_hw_number_t)*(u32 *)arg; > + } If domain->of_node is NULL and system booted with DT the code above does not fail (and if it fails almost certainly that won't be graceful) but it should. > for (i = 0; i < nr_irqs; i++) > gic_irq_domain_map(domain, virq + i, hwirq + i); > @@ -945,11 +952,11 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, > gic_irqs = 1020; > gic->gic_irqs = gic_irqs; > > - if (node) { /* DT case */ > + if (node || !acpi_disabled) { /* DT or ACPI case */ > gic->domain = irq_domain_add_linear(node, gic_irqs, > &gic_irq_domain_hierarchy_ops, > gic); > - } else { /* Non-DT case */ > + } else { /* Non-DT and ACPI case */ > /* > * For primary GICs, skip over SGIs. > * For secondary GICs, skip over PPIs, too. > @@ -1130,13 +1137,8 @@ gic_v2_acpi_init(struct acpi_table_header *table) > return -ENOMEM; > } > > - /* > - * Initialize zero GIC instance (no multi-GIC support). Also, set GIC > - * as default IRQ domain to allow for GSI registration and GSI to IRQ > - * number translation (see acpi_register_gsi() and acpi_gsi_to_irq()). > - */ > gic_init_bases(0, -1, dist_base, cpu_base, 0, NULL); > - irq_set_default_host(gic_data[0].domain); > + acpi_irq_domain = gic_data[0].domain; > > acpi_irq_model = ACPI_IRQ_MODEL_GIC; > return 0; > diff --git a/include/linux/irqchip/arm-gic-acpi.h b/include/linux/irqchip/arm-gic-acpi.h > index 56cd82c..a4a5edb 100644 > --- a/include/linux/irqchip/arm-gic-acpi.h > +++ b/include/linux/irqchip/arm-gic-acpi.h > @@ -21,5 +21,7 @@ > #define ACPI_GIC_CPU_IF_MEM_SIZE (SZ_8K) > #define ACPI_GICV3_DIST_MEM_SIZE (SZ_64K) > > +extern struct irq_domain *acpi_irq_domain; > + This declaration does not belong here, see above. Lorenzo
On 06/23/2015 01:20 AM, Lorenzo Pieralisi wrote: > On Fri, Jun 19, 2015 at 09:46:09AM +0100, Hanjun Guo wrote: >> Introduce acpi_irq_domain for GICv2 core domain instead of referring >> to the irq_default_domain, based on that, pass gsi as the argument and >> get the gsi in gic_irq_domain_alloc() to add stacked irqdomain support >> for ACPI based GICv2 init. > > This commit log does not parse and we can read the code, there is no > point in trying to rewrite it here. > >> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >> --- >> drivers/acpi/gsi.c | 28 +++++++++++++--------------- >> drivers/irqchip/irq-gic.c | 32 +++++++++++++++++--------------- >> include/linux/irqchip/arm-gic-acpi.h | 2 ++ >> 3 files changed, 32 insertions(+), 30 deletions(-) >> >> diff --git a/drivers/acpi/gsi.c b/drivers/acpi/gsi.c >> index 38208f2..55b5f31 100644 >> --- a/drivers/acpi/gsi.c >> +++ b/drivers/acpi/gsi.c >> @@ -3,6 +3,7 @@ >> * >> * Copyright (C) 2015 ARM Ltd. >> * Author: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> >> + * Hanjun Guo <hanjun.guo@linaro.org> for stacked irqdomains support >> * >> * This program is free software; you can redistribute it and/or modify >> * it under the terms of the GNU General Public License version 2 as >> @@ -13,6 +14,8 @@ >> #include <linux/irqdomain.h> >> >> enum acpi_irq_model_id acpi_irq_model; >> +/* ACPI core domian pointing to GICv2/3 core domain */ > > This code is not GIC specific and does not have to be. > >> +struct irq_domain *acpi_irq_domain __read_mostly; > > Make it static and create helpers to set it. > >> static unsigned int acpi_gsi_get_irq_type(int trigger, int polarity) >> { >> @@ -45,12 +48,7 @@ static unsigned int acpi_gsi_get_irq_type(int trigger, int polarity) >> */ >> int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) >> { >> - /* >> - * Only default domain is supported at present, always find >> - * the mapping corresponding to default domain by passing NULL >> - * as irq_domain parameter >> - */ >> - *irq = irq_find_mapping(NULL, gsi); >> + *irq = irq_find_mapping(acpi_irq_domain, gsi); >> /* >> * *irq == 0 means no mapping, that should >> * be reported as a failure >> @@ -72,16 +70,16 @@ EXPORT_SYMBOL_GPL(acpi_gsi_to_irq); >> int acpi_register_gsi(struct device *dev, u32 gsi, int trigger, >> int polarity) >> { >> - unsigned int irq; >> + int irq; >> unsigned int irq_type = acpi_gsi_get_irq_type(trigger, polarity); >> >> - /* >> - * There is no way at present to look-up the IRQ domain on ACPI, >> - * hence always create mapping referring to the default domain >> - * by passing NULL as irq_domain parameter >> - */ >> - irq = irq_create_mapping(NULL, gsi); >> - if (!irq) >> + irq = irq_find_mapping(acpi_irq_domain, gsi); >> + if (irq > 0) >> + return irq; >> + >> + irq = irq_domain_alloc_irqs(acpi_irq_domain, 1, dev_to_node(dev), >> + &gsi); >> + if (irq <= 0) >> return -EINVAL; >> >> /* Set irq type if specified and different than the current one */ >> @@ -98,7 +96,7 @@ EXPORT_SYMBOL_GPL(acpi_register_gsi); >> */ >> void acpi_unregister_gsi(u32 gsi) >> { >> - int irq = irq_find_mapping(NULL, gsi); >> + int irq = irq_find_mapping(acpi_irq_domain, gsi); >> >> irq_dispose_mapping(irq); >> } >> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >> index 8fc67bc..d1b2131 100644 >> --- a/drivers/irqchip/irq-gic.c >> +++ b/drivers/irqchip/irq-gic.c >> @@ -851,15 +851,22 @@ static struct notifier_block gic_cpu_notifier = { >> static int gic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, >> unsigned int nr_irqs, void *arg) >> { >> - int i, ret; >> + int i; >> irq_hw_number_t hwirq; >> - unsigned int type = IRQ_TYPE_NONE; >> - struct of_phandle_args *irq_data = arg; >> >> - ret = gic_irq_domain_xlate(domain, irq_data->np, irq_data->args, >> - irq_data->args_count, &hwirq, &type); >> - if (ret) >> - return ret; >> + if (domain->of_node) { /* DT case */ >> + int ret; >> + unsigned int type = IRQ_TYPE_NONE; >> + struct of_phandle_args *irq_data = arg; >> + >> + ret = gic_irq_domain_xlate(domain, irq_data->np, >> + irq_data->args, >> + irq_data->args_count, &hwirq, &type); >> + if (ret) >> + return ret; >> + } else { /* ACPI case */ >> + hwirq = (irq_hw_number_t)*(u32 *)arg; >> + } > > If domain->of_node is NULL and system booted with DT the code above > does not fail (and if it fails almost certainly that won't be graceful) > but it should. how about the following logic? if (!domain->of_node && acpi_disabled) return -ENODEV; else if (domain->of_node) dt related code; else ACPI related code; > >> for (i = 0; i < nr_irqs; i++) >> gic_irq_domain_map(domain, virq + i, hwirq + i); >> @@ -945,11 +952,11 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, >> gic_irqs = 1020; >> gic->gic_irqs = gic_irqs; >> >> - if (node) { /* DT case */ >> + if (node || !acpi_disabled) { /* DT or ACPI case */ >> gic->domain = irq_domain_add_linear(node, gic_irqs, >> &gic_irq_domain_hierarchy_ops, >> gic); >> - } else { /* Non-DT case */ >> + } else { /* Non-DT and ACPI case */ >> /* >> * For primary GICs, skip over SGIs. >> * For secondary GICs, skip over PPIs, too. >> @@ -1130,13 +1137,8 @@ gic_v2_acpi_init(struct acpi_table_header *table) >> return -ENOMEM; >> } >> >> - /* >> - * Initialize zero GIC instance (no multi-GIC support). Also, set GIC >> - * as default IRQ domain to allow for GSI registration and GSI to IRQ >> - * number translation (see acpi_register_gsi() and acpi_gsi_to_irq()). >> - */ >> gic_init_bases(0, -1, dist_base, cpu_base, 0, NULL); >> - irq_set_default_host(gic_data[0].domain); >> + acpi_irq_domain = gic_data[0].domain; >> >> acpi_irq_model = ACPI_IRQ_MODEL_GIC; >> return 0; >> diff --git a/include/linux/irqchip/arm-gic-acpi.h b/include/linux/irqchip/arm-gic-acpi.h >> index 56cd82c..a4a5edb 100644 >> --- a/include/linux/irqchip/arm-gic-acpi.h >> +++ b/include/linux/irqchip/arm-gic-acpi.h >> @@ -21,5 +21,7 @@ >> #define ACPI_GIC_CPU_IF_MEM_SIZE (SZ_8K) >> #define ACPI_GICV3_DIST_MEM_SIZE (SZ_64K) >> >> +extern struct irq_domain *acpi_irq_domain; >> + > > This declaration does not belong here, see above. will update in next version. Thanks Hanjun
On Tue, Jun 23, 2015 at 04:11:38PM +0100, Hanjun Guo wrote: [...] > >> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > >> index 8fc67bc..d1b2131 100644 > >> --- a/drivers/irqchip/irq-gic.c > >> +++ b/drivers/irqchip/irq-gic.c > >> @@ -851,15 +851,22 @@ static struct notifier_block gic_cpu_notifier = { > >> static int gic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, > >> unsigned int nr_irqs, void *arg) > >> { > >> - int i, ret; > >> + int i; > >> irq_hw_number_t hwirq; > >> - unsigned int type = IRQ_TYPE_NONE; > >> - struct of_phandle_args *irq_data = arg; > >> > >> - ret = gic_irq_domain_xlate(domain, irq_data->np, irq_data->args, > >> - irq_data->args_count, &hwirq, &type); > >> - if (ret) > >> - return ret; > >> + if (domain->of_node) { /* DT case */ > >> + int ret; > >> + unsigned int type = IRQ_TYPE_NONE; > >> + struct of_phandle_args *irq_data = arg; > >> + > >> + ret = gic_irq_domain_xlate(domain, irq_data->np, > >> + irq_data->args, > >> + irq_data->args_count, &hwirq, &type); > >> + if (ret) > >> + return ret; > >> + } else { /* ACPI case */ > >> + hwirq = (irq_hw_number_t)*(u32 *)arg; > >> + } > > > > If domain->of_node is NULL and system booted with DT the code above > > does not fail (and if it fails almost certainly that won't be graceful) > > but it should. > > how about the following logic? > > if (!domain->of_node && acpi_disabled) > return -ENODEV; > else if (domain->of_node) > dt related code; > else > ACPI related code; Code is not checking the node at present so: if (acpi_disabled) dt code; else ACPI code; would do, but that's a nit. > >> for (i = 0; i < nr_irqs; i++) > >> gic_irq_domain_map(domain, virq + i, hwirq + i); > >> @@ -945,11 +952,11 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, > >> gic_irqs = 1020; > >> gic->gic_irqs = gic_irqs; > >> > >> - if (node) { /* DT case */ > >> + if (node || !acpi_disabled) { /* DT or ACPI case */ > >> gic->domain = irq_domain_add_linear(node, gic_irqs, > >> &gic_irq_domain_hierarchy_ops, > >> gic); I think this is a bit more worrying, I mean passing a NULL node pointer to the irqdomain layer which basically means you are booting out of ACPI (for you, if that's true for the irq_domain_add_linear implementation that's another story), the node pointer should be optional but you need feedback from IRQ layer maintainers here. Lorenzo
On 06/24/2015 01:38 AM, Lorenzo Pieralisi wrote: > On Tue, Jun 23, 2015 at 04:11:38PM +0100, Hanjun Guo wrote: > > [...] > >>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >>>> index 8fc67bc..d1b2131 100644 >>>> --- a/drivers/irqchip/irq-gic.c >>>> +++ b/drivers/irqchip/irq-gic.c >>>> @@ -851,15 +851,22 @@ static struct notifier_block gic_cpu_notifier = { >>>> static int gic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, >>>> unsigned int nr_irqs, void *arg) >>>> { >>>> - int i, ret; >>>> + int i; >>>> irq_hw_number_t hwirq; >>>> - unsigned int type = IRQ_TYPE_NONE; >>>> - struct of_phandle_args *irq_data = arg; >>>> >>>> - ret = gic_irq_domain_xlate(domain, irq_data->np, irq_data->args, >>>> - irq_data->args_count, &hwirq, &type); >>>> - if (ret) >>>> - return ret; >>>> + if (domain->of_node) { /* DT case */ >>>> + int ret; >>>> + unsigned int type = IRQ_TYPE_NONE; >>>> + struct of_phandle_args *irq_data = arg; >>>> + >>>> + ret = gic_irq_domain_xlate(domain, irq_data->np, >>>> + irq_data->args, >>>> + irq_data->args_count, &hwirq, &type); >>>> + if (ret) >>>> + return ret; >>>> + } else { /* ACPI case */ >>>> + hwirq = (irq_hw_number_t)*(u32 *)arg; >>>> + } >>> >>> If domain->of_node is NULL and system booted with DT the code above >>> does not fail (and if it fails almost certainly that won't be graceful) >>> but it should. >> >> how about the following logic? >> >> if (!domain->of_node && acpi_disabled) >> return -ENODEV; >> else if (domain->of_node) >> dt related code; >> else >> ACPI related code; > > Code is not checking the node at present so: > > if (acpi_disabled) > dt code; > else > ACPI code; > > would do, but that's a nit. > >>>> for (i = 0; i < nr_irqs; i++) >>>> gic_irq_domain_map(domain, virq + i, hwirq + i); >>>> @@ -945,11 +952,11 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, >>>> gic_irqs = 1020; >>>> gic->gic_irqs = gic_irqs; >>>> >>>> - if (node) { /* DT case */ >>>> + if (node || !acpi_disabled) { /* DT or ACPI case */ >>>> gic->domain = irq_domain_add_linear(node, gic_irqs, >>>> &gic_irq_domain_hierarchy_ops, >>>> gic); > > I think this is a bit more worrying, I mean passing a NULL node pointer to > the irqdomain layer which basically means you are booting out of ACPI I'm little confused here, would you mind explaining more for your worrying? To me, node pointer is optional and it's ok for ACPI case. > (for you, if that's true for the irq_domain_add_linear implementation > that's another story), the node pointer should be optional but you > need feedback from IRQ layer maintainers here. Sure. Thanks Hanjun
On 27/06/15 04:52, Hanjun Guo wrote: > On 06/24/2015 01:38 AM, Lorenzo Pieralisi wrote: >> On Tue, Jun 23, 2015 at 04:11:38PM +0100, Hanjun Guo wrote: >> >> [...] >> >>>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >>>>> index 8fc67bc..d1b2131 100644 >>>>> --- a/drivers/irqchip/irq-gic.c >>>>> +++ b/drivers/irqchip/irq-gic.c >>>>> @@ -851,15 +851,22 @@ static struct notifier_block gic_cpu_notifier = { >>>>> static int gic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, >>>>> unsigned int nr_irqs, void *arg) >>>>> { >>>>> - int i, ret; >>>>> + int i; >>>>> irq_hw_number_t hwirq; >>>>> - unsigned int type = IRQ_TYPE_NONE; >>>>> - struct of_phandle_args *irq_data = arg; >>>>> >>>>> - ret = gic_irq_domain_xlate(domain, irq_data->np, irq_data->args, >>>>> - irq_data->args_count, &hwirq, &type); >>>>> - if (ret) >>>>> - return ret; >>>>> + if (domain->of_node) { /* DT case */ >>>>> + int ret; >>>>> + unsigned int type = IRQ_TYPE_NONE; >>>>> + struct of_phandle_args *irq_data = arg; >>>>> + >>>>> + ret = gic_irq_domain_xlate(domain, irq_data->np, >>>>> + irq_data->args, >>>>> + irq_data->args_count, &hwirq, &type); >>>>> + if (ret) >>>>> + return ret; >>>>> + } else { /* ACPI case */ >>>>> + hwirq = (irq_hw_number_t)*(u32 *)arg; >>>>> + } >>>> >>>> If domain->of_node is NULL and system booted with DT the code above >>>> does not fail (and if it fails almost certainly that won't be graceful) >>>> but it should. >>> >>> how about the following logic? >>> >>> if (!domain->of_node && acpi_disabled) >>> return -ENODEV; >>> else if (domain->of_node) >>> dt related code; >>> else >>> ACPI related code; >> >> Code is not checking the node at present so: >> >> if (acpi_disabled) >> dt code; >> else >> ACPI code; >> >> would do, but that's a nit. >> >>>>> for (i = 0; i < nr_irqs; i++) >>>>> gic_irq_domain_map(domain, virq + i, hwirq + i); >>>>> @@ -945,11 +952,11 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, >>>>> gic_irqs = 1020; >>>>> gic->gic_irqs = gic_irqs; >>>>> >>>>> - if (node) { /* DT case */ >>>>> + if (node || !acpi_disabled) { /* DT or ACPI case */ >>>>> gic->domain = irq_domain_add_linear(node, gic_irqs, >>>>> &gic_irq_domain_hierarchy_ops, >>>>> gic); >> >> I think this is a bit more worrying, I mean passing a NULL node pointer to >> the irqdomain layer which basically means you are booting out of ACPI > > I'm little confused here, would you mind explaining more for your > worrying? To me, node pointer is optional and it's ok for ACPI > case. > >> (for you, if that's true for the irq_domain_add_linear implementation >> that's another story), the node pointer should be optional but you >> need feedback from IRQ layer maintainers here. > > Sure. Frankly, I'd really like to see ACPI using the "node" parameter for something useful. This would save having to cache pointers all over the place, will make find_irq_host() work as expected... etc. See the comment at the top of linux/irqdomain.h : "... This code could thus be used on other architectures by replacing those two by some sort of arch-specific void * "token" used to identify interrupt controllers." Maybe it is time to bite the bullet. Thanks, M.
Hi Marc, On 06/29/2015 04:39 PM, Marc Zyngier wrote: > On 27/06/15 04:52, Hanjun Guo wrote: >> On 06/24/2015 01:38 AM, Lorenzo Pieralisi wrote: >>> On Tue, Jun 23, 2015 at 04:11:38PM +0100, Hanjun Guo wrote: [...] >>> >>>>>> for (i = 0; i < nr_irqs; i++) >>>>>> gic_irq_domain_map(domain, virq + i, hwirq + i); >>>>>> @@ -945,11 +952,11 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, >>>>>> gic_irqs = 1020; >>>>>> gic->gic_irqs = gic_irqs; >>>>>> >>>>>> - if (node) { /* DT case */ >>>>>> + if (node || !acpi_disabled) { /* DT or ACPI case */ >>>>>> gic->domain = irq_domain_add_linear(node, gic_irqs, >>>>>> &gic_irq_domain_hierarchy_ops, >>>>>> gic); >>> >>> I think this is a bit more worrying, I mean passing a NULL node pointer to >>> the irqdomain layer which basically means you are booting out of ACPI >> >> I'm little confused here, would you mind explaining more for your >> worrying? To me, node pointer is optional and it's ok for ACPI >> case. >> >>> (for you, if that's true for the irq_domain_add_linear implementation >>> that's another story), the node pointer should be optional but you >>> need feedback from IRQ layer maintainers here. >> >> Sure. > > Frankly, I'd really like to see ACPI using the "node" parameter for > something useful. This would save having to cache pointers all over the > place, will make find_irq_host() work as expected... etc. > > See the comment at the top of linux/irqdomain.h : > > "... This code could thus be used on other architectures by replacing > those two by some sort of arch-specific void * "token" used to identify > interrupt controllers." To init GIC in ACPI, we can only use the table entry pointer as the token, but the ACPI static tables are early mem/io remapped memory at boot stage, and it will be not available after boot, also we need muti types of MADT enties to init GIC (GICC and GICD for GICv2, GICC or GICR and GICD for GICv3), not as DT, just one single node to include all the information needed to init the GIC. We use ACPI handle for devices as node for DT when the namespace is available, but that's pretty late in the boot stage which GIC, SMP and timers were already initialized, so ACPI handle can not use as the token too. I see multi places just pass NULL as the pointer directly for irq_domain_add_linear() which works fine, and for ACPI, we tested this patch and also it works. > > Maybe it is time to bite the bullet. Hope comment above explains :) Thanks Hanjun
On 30/06/15 12:50, Hanjun Guo wrote: > Hi Marc, > > On 06/29/2015 04:39 PM, Marc Zyngier wrote: >> On 27/06/15 04:52, Hanjun Guo wrote: >>> On 06/24/2015 01:38 AM, Lorenzo Pieralisi wrote: >>>> On Tue, Jun 23, 2015 at 04:11:38PM +0100, Hanjun Guo wrote: > [...] >>>> >>>>>>> for (i = 0; i < nr_irqs; i++) >>>>>>> gic_irq_domain_map(domain, virq + i, hwirq + i); >>>>>>> @@ -945,11 +952,11 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, >>>>>>> gic_irqs = 1020; >>>>>>> gic->gic_irqs = gic_irqs; >>>>>>> >>>>>>> - if (node) { /* DT case */ >>>>>>> + if (node || !acpi_disabled) { /* DT or ACPI case */ >>>>>>> gic->domain = irq_domain_add_linear(node, gic_irqs, >>>>>>> &gic_irq_domain_hierarchy_ops, >>>>>>> gic); >>>> >>>> I think this is a bit more worrying, I mean passing a NULL node pointer to >>>> the irqdomain layer which basically means you are booting out of ACPI >>> >>> I'm little confused here, would you mind explaining more for your >>> worrying? To me, node pointer is optional and it's ok for ACPI >>> case. >>> >>>> (for you, if that's true for the irq_domain_add_linear implementation >>>> that's another story), the node pointer should be optional but you >>>> need feedback from IRQ layer maintainers here. >>> >>> Sure. >> >> Frankly, I'd really like to see ACPI using the "node" parameter for >> something useful. This would save having to cache pointers all over the >> place, will make find_irq_host() work as expected... etc. >> >> See the comment at the top of linux/irqdomain.h : >> >> "... This code could thus be used on other architectures by replacing >> those two by some sort of arch-specific void * "token" used to identify >> interrupt controllers." > > To init GIC in ACPI, we can only use the table entry pointer as > the token, but the ACPI static tables are early mem/io remapped > memory at boot stage, and it will be not available after boot, > also we need muti types of MADT enties to init GIC (GICC and GICD > for GICv2, GICC or GICR and GICD for GICv3), not as DT, just > one single node to include all the information needed to init > the GIC. A single pointer would be enough, you don't need all of them. > We use ACPI handle for devices as node for DT when the namespace > is available, but that's pretty late in the boot stage which GIC, > SMP and timers were already initialized, so ACPI handle can not > use as the token too. > > I see multi places just pass NULL as the pointer directly for > irq_domain_add_linear() which works fine, and for ACPI, we tested > this patch and also it works. Yes it works. But you're reinventing the wheel by keeping references outside of the normal framework, which is simply going to make the code more difficult to maintain in the long run. Putting NULL as the device_node parameter really means "this is a domain I don't need to look up later". In your case, you will have to lookup that domain, all the time. You're just doing it in your own little corner, which is what bothers me. M.
On Tue, Jun 30, 2015 at 01:17:14PM +0100, Marc Zyngier wrote: > On 30/06/15 12:50, Hanjun Guo wrote: > > Hi Marc, > > > > On 06/29/2015 04:39 PM, Marc Zyngier wrote: > >> On 27/06/15 04:52, Hanjun Guo wrote: > >>> On 06/24/2015 01:38 AM, Lorenzo Pieralisi wrote: > >>>> On Tue, Jun 23, 2015 at 04:11:38PM +0100, Hanjun Guo wrote: > > [...] > >>>> > >>>>>>> for (i = 0; i < nr_irqs; i++) > >>>>>>> gic_irq_domain_map(domain, virq + i, hwirq + i); > >>>>>>> @@ -945,11 +952,11 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, > >>>>>>> gic_irqs = 1020; > >>>>>>> gic->gic_irqs = gic_irqs; > >>>>>>> > >>>>>>> - if (node) { /* DT case */ > >>>>>>> + if (node || !acpi_disabled) { /* DT or ACPI case */ > >>>>>>> gic->domain = irq_domain_add_linear(node, gic_irqs, > >>>>>>> &gic_irq_domain_hierarchy_ops, > >>>>>>> gic); > >>>> > >>>> I think this is a bit more worrying, I mean passing a NULL node pointer to > >>>> the irqdomain layer which basically means you are booting out of ACPI > >>> > >>> I'm little confused here, would you mind explaining more for your > >>> worrying? To me, node pointer is optional and it's ok for ACPI > >>> case. > >>> > >>>> (for you, if that's true for the irq_domain_add_linear implementation > >>>> that's another story), the node pointer should be optional but you > >>>> need feedback from IRQ layer maintainers here. > >>> > >>> Sure. > >> > >> Frankly, I'd really like to see ACPI using the "node" parameter for > >> something useful. This would save having to cache pointers all over the > >> place, will make find_irq_host() work as expected... etc. > >> > >> See the comment at the top of linux/irqdomain.h : > >> > >> "... This code could thus be used on other architectures by replacing > >> those two by some sort of arch-specific void * "token" used to identify > >> interrupt controllers." > > > > To init GIC in ACPI, we can only use the table entry pointer as > > the token, but the ACPI static tables are early mem/io remapped > > memory at boot stage, and it will be not available after boot, > > also we need muti types of MADT enties to init GIC (GICC and GICD > > for GICv2, GICC or GICR and GICD for GICv3), not as DT, just > > one single node to include all the information needed to init > > the GIC. > > A single pointer would be enough, you don't need all of them. > > > We use ACPI handle for devices as node for DT when the namespace > > is available, but that's pretty late in the boot stage which GIC, > > SMP and timers were already initialized, so ACPI handle can not > > use as the token too. > > > > I see multi places just pass NULL as the pointer directly for > > irq_domain_add_linear() which works fine, and for ACPI, we tested > > this patch and also it works. > > Yes it works. But you're reinventing the wheel by keeping references > outside of the normal framework, which is simply going to make the code > more difficult to maintain in the long run. > > Putting NULL as the device_node parameter really means "this is a domain > I don't need to look up later". In your case, you will have to lookup > that domain, all the time. You're just doing it in your own little > corner, which is what bothers me. > Hanjun, I think it should be possible that instead of looking up the domains in our own bit of code. We can instead use a ptr to the appropriate information as the token instead. I don't think we have to replicate the behaviour of node, in the DT case, but just do what is sensible for ACPI in this case. Graeme
On 06/30/2015 11:07 PM, Graeme Gregory wrote: > On Tue, Jun 30, 2015 at 01:17:14PM +0100, Marc Zyngier wrote: >> On 30/06/15 12:50, Hanjun Guo wrote: >>> Hi Marc, >>> >>> On 06/29/2015 04:39 PM, Marc Zyngier wrote: >>>> On 27/06/15 04:52, Hanjun Guo wrote: >>>>> On 06/24/2015 01:38 AM, Lorenzo Pieralisi wrote: >>>>>> On Tue, Jun 23, 2015 at 04:11:38PM +0100, Hanjun Guo wrote: >>> [...] >>>>>> >>>>>>>>> for (i = 0; i < nr_irqs; i++) >>>>>>>>> gic_irq_domain_map(domain, virq + i, hwirq + i); >>>>>>>>> @@ -945,11 +952,11 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, >>>>>>>>> gic_irqs = 1020; >>>>>>>>> gic->gic_irqs = gic_irqs; >>>>>>>>> >>>>>>>>> - if (node) { /* DT case */ >>>>>>>>> + if (node || !acpi_disabled) { /* DT or ACPI case */ >>>>>>>>> gic->domain = irq_domain_add_linear(node, gic_irqs, >>>>>>>>> &gic_irq_domain_hierarchy_ops, >>>>>>>>> gic); >>>>>> >>>>>> I think this is a bit more worrying, I mean passing a NULL node pointer to >>>>>> the irqdomain layer which basically means you are booting out of ACPI >>>>> >>>>> I'm little confused here, would you mind explaining more for your >>>>> worrying? To me, node pointer is optional and it's ok for ACPI >>>>> case. >>>>> >>>>>> (for you, if that's true for the irq_domain_add_linear implementation >>>>>> that's another story), the node pointer should be optional but you >>>>>> need feedback from IRQ layer maintainers here. >>>>> >>>>> Sure. >>>> >>>> Frankly, I'd really like to see ACPI using the "node" parameter for >>>> something useful. This would save having to cache pointers all over the >>>> place, will make find_irq_host() work as expected... etc. >>>> >>>> See the comment at the top of linux/irqdomain.h : >>>> >>>> "... This code could thus be used on other architectures by replacing >>>> those two by some sort of arch-specific void * "token" used to identify >>>> interrupt controllers." >>> >>> To init GIC in ACPI, we can only use the table entry pointer as >>> the token, but the ACPI static tables are early mem/io remapped >>> memory at boot stage, and it will be not available after boot, >>> also we need muti types of MADT enties to init GIC (GICC and GICD >>> for GICv2, GICC or GICR and GICD for GICv3), not as DT, just >>> one single node to include all the information needed to init >>> the GIC. >> >> A single pointer would be enough, you don't need all of them. >> >>> We use ACPI handle for devices as node for DT when the namespace >>> is available, but that's pretty late in the boot stage which GIC, >>> SMP and timers were already initialized, so ACPI handle can not >>> use as the token too. >>> >>> I see multi places just pass NULL as the pointer directly for >>> irq_domain_add_linear() which works fine, and for ACPI, we tested >>> this patch and also it works. >> >> Yes it works. But you're reinventing the wheel by keeping references >> outside of the normal framework, which is simply going to make the code >> more difficult to maintain in the long run. >> >> Putting NULL as the device_node parameter really means "this is a domain >> I don't need to look up later". In your case, you will have to lookup >> that domain, all the time. You're just doing it in your own little >> corner, which is what bothers me. >> > Hanjun, I think it should be possible that instead of looking up the > domains in our own bit of code. We can instead use a ptr to the > appropriate information as the token instead. > > I don't think we have to replicate the behaviour of node, in the DT > case, but just do what is sensible for ACPI in this case. I'm trying to introduce a pointer which point to the GICD when GIC is initialized, this pointer will be used to match the irqdomain. In driver code, I will find this pointer with the gsi, then find the irqdomain match the pointer, then I hope I can remove acpi_irq_domain in the end. Marc, beyond this issue, do you have any more comments about this patch set? the framework of init GIC? and the code to init GICV3? if yes, I can address it along with this one together. Thanks Hanjun
diff --git a/drivers/acpi/gsi.c b/drivers/acpi/gsi.c index 38208f2..55b5f31 100644 --- a/drivers/acpi/gsi.c +++ b/drivers/acpi/gsi.c @@ -3,6 +3,7 @@ * * Copyright (C) 2015 ARM Ltd. * Author: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> + * Hanjun Guo <hanjun.guo@linaro.org> for stacked irqdomains support * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 as @@ -13,6 +14,8 @@ #include <linux/irqdomain.h> enum acpi_irq_model_id acpi_irq_model; +/* ACPI core domian pointing to GICv2/3 core domain */ +struct irq_domain *acpi_irq_domain __read_mostly; static unsigned int acpi_gsi_get_irq_type(int trigger, int polarity) { @@ -45,12 +48,7 @@ static unsigned int acpi_gsi_get_irq_type(int trigger, int polarity) */ int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) { - /* - * Only default domain is supported at present, always find - * the mapping corresponding to default domain by passing NULL - * as irq_domain parameter - */ - *irq = irq_find_mapping(NULL, gsi); + *irq = irq_find_mapping(acpi_irq_domain, gsi); /* * *irq == 0 means no mapping, that should * be reported as a failure @@ -72,16 +70,16 @@ EXPORT_SYMBOL_GPL(acpi_gsi_to_irq); int acpi_register_gsi(struct device *dev, u32 gsi, int trigger, int polarity) { - unsigned int irq; + int irq; unsigned int irq_type = acpi_gsi_get_irq_type(trigger, polarity); - /* - * There is no way at present to look-up the IRQ domain on ACPI, - * hence always create mapping referring to the default domain - * by passing NULL as irq_domain parameter - */ - irq = irq_create_mapping(NULL, gsi); - if (!irq) + irq = irq_find_mapping(acpi_irq_domain, gsi); + if (irq > 0) + return irq; + + irq = irq_domain_alloc_irqs(acpi_irq_domain, 1, dev_to_node(dev), + &gsi); + if (irq <= 0) return -EINVAL; /* Set irq type if specified and different than the current one */ @@ -98,7 +96,7 @@ EXPORT_SYMBOL_GPL(acpi_register_gsi); */ void acpi_unregister_gsi(u32 gsi) { - int irq = irq_find_mapping(NULL, gsi); + int irq = irq_find_mapping(acpi_irq_domain, gsi); irq_dispose_mapping(irq); } diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index 8fc67bc..d1b2131 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -851,15 +851,22 @@ static struct notifier_block gic_cpu_notifier = { static int gic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, unsigned int nr_irqs, void *arg) { - int i, ret; + int i; irq_hw_number_t hwirq; - unsigned int type = IRQ_TYPE_NONE; - struct of_phandle_args *irq_data = arg; - ret = gic_irq_domain_xlate(domain, irq_data->np, irq_data->args, - irq_data->args_count, &hwirq, &type); - if (ret) - return ret; + if (domain->of_node) { /* DT case */ + int ret; + unsigned int type = IRQ_TYPE_NONE; + struct of_phandle_args *irq_data = arg; + + ret = gic_irq_domain_xlate(domain, irq_data->np, + irq_data->args, + irq_data->args_count, &hwirq, &type); + if (ret) + return ret; + } else { /* ACPI case */ + hwirq = (irq_hw_number_t)*(u32 *)arg; + } for (i = 0; i < nr_irqs; i++) gic_irq_domain_map(domain, virq + i, hwirq + i); @@ -945,11 +952,11 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, gic_irqs = 1020; gic->gic_irqs = gic_irqs; - if (node) { /* DT case */ + if (node || !acpi_disabled) { /* DT or ACPI case */ gic->domain = irq_domain_add_linear(node, gic_irqs, &gic_irq_domain_hierarchy_ops, gic); - } else { /* Non-DT case */ + } else { /* Non-DT and ACPI case */ /* * For primary GICs, skip over SGIs. * For secondary GICs, skip over PPIs, too. @@ -1130,13 +1137,8 @@ gic_v2_acpi_init(struct acpi_table_header *table) return -ENOMEM; } - /* - * Initialize zero GIC instance (no multi-GIC support). Also, set GIC - * as default IRQ domain to allow for GSI registration and GSI to IRQ - * number translation (see acpi_register_gsi() and acpi_gsi_to_irq()). - */ gic_init_bases(0, -1, dist_base, cpu_base, 0, NULL); - irq_set_default_host(gic_data[0].domain); + acpi_irq_domain = gic_data[0].domain; acpi_irq_model = ACPI_IRQ_MODEL_GIC; return 0; diff --git a/include/linux/irqchip/arm-gic-acpi.h b/include/linux/irqchip/arm-gic-acpi.h index 56cd82c..a4a5edb 100644 --- a/include/linux/irqchip/arm-gic-acpi.h +++ b/include/linux/irqchip/arm-gic-acpi.h @@ -21,5 +21,7 @@ #define ACPI_GIC_CPU_IF_MEM_SIZE (SZ_8K) #define ACPI_GICV3_DIST_MEM_SIZE (SZ_64K) +extern struct irq_domain *acpi_irq_domain; + #endif /* CONFIG_ACPI */ #endif /* ARM_GIC_ACPI_H_ */
Introduce acpi_irq_domain for GICv2 core domain instead of referring to the irq_default_domain, based on that, pass gsi as the argument and get the gsi in gic_irq_domain_alloc() to add stacked irqdomain support for ACPI based GICv2 init. Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> --- drivers/acpi/gsi.c | 28 +++++++++++++--------------- drivers/irqchip/irq-gic.c | 32 +++++++++++++++++--------------- include/linux/irqchip/arm-gic-acpi.h | 2 ++ 3 files changed, 32 insertions(+), 30 deletions(-)