| Message ID | 1437473280-11431-6-git-send-email-marc.zyngier@arm.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Tue, Jul 21, 2015 at 11:08:00AM +0100, Marc Zyngier wrote: > Now that the basic ACPI GSI code is irq domain aware, make sure > that the ACPI support in the GIC doesn't pointlessly deviate from > the DT path. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > drivers/irqchip/irq-gic.c | 17 ++++++----------- > include/linux/irqchip/arm-gic.h | 2 +- > 2 files changed, 7 insertions(+), 12 deletions(-) > > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > index b41ccf5..f5d365d 100644 > --- a/drivers/irqchip/irq-gic.c > +++ b/drivers/irqchip/irq-gic.c > @@ -813,8 +813,6 @@ static int gic_irq_domain_xlate(struct irq_domain *d, > { > unsigned long ret = 0; > > - if (irq_domain_get_of_node(d) != controller) > - return -EINVAL; This change seems to have nothing to do with the description. Graeme > if (intsize < 3) > return -EINVAL; > > @@ -887,7 +885,7 @@ void gic_set_irqchip_flags(unsigned long flags) > > void __init gic_init_bases(unsigned int gic_nr, int irq_start, > void __iomem *dist_base, void __iomem *cpu_base, > - u32 percpu_offset, struct device_node *node) > + u32 percpu_offset, void *domain_token) > { > irq_hw_number_t hwirq_base; > struct gic_chip_data *gic; > @@ -946,8 +944,8 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, > gic_irqs = 1020; > gic->gic_irqs = gic_irqs; > > - if (node) { /* DT case */ > - gic->domain = irq_domain_add_linear(node, gic_irqs, > + if (domain_token) { /* DT/ACPI case */ > + gic->domain = irq_domain_add_linear(domain_token, gic_irqs, > &gic_irq_domain_hierarchy_ops, > gic); > } else { /* Non-DT case */ > @@ -973,7 +971,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, > irq_base = irq_start; > } > > - gic->domain = irq_domain_add_legacy(node, gic_irqs, irq_base, > + gic->domain = irq_domain_add_legacy(NULL, gic_irqs, irq_base, > hwirq_base, &gic_irq_domain_ops, gic); > } > > @@ -1132,12 +1130,9 @@ gic_v2_acpi_init(struct acpi_table_header *table) > } > > /* > - * 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()). > + * Initialize zero GIC instance (no multi-GIC support). > */ > - gic_init_bases(0, -1, dist_base, cpu_base, 0, NULL); > - irq_set_default_host(gic_data[0].domain); > + gic_init_bases(0, -1, dist_base, cpu_base, 0, (void *)ACPI_IRQ_MODEL_GIC); > > acpi_irq_model = ACPI_IRQ_MODEL_GIC; > return 0; > diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h > index 9de976b..97799b7 100644 > --- a/include/linux/irqchip/arm-gic.h > +++ b/include/linux/irqchip/arm-gic.h > @@ -97,7 +97,7 @@ struct device_node; > > void gic_set_irqchip_flags(unsigned long flags); > void gic_init_bases(unsigned int, int, void __iomem *, void __iomem *, > - u32 offset, struct device_node *); > + u32 offset, void *); > void gic_cascade_irq(unsigned int gic_nr, unsigned int irq); > void gic_cpu_if_down(void); > > -- > 2.1.4 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 21 Jul 2015 13:34:08 +0100 Graeme Gregory <graeme@xora.org.uk> wrote: > On Tue, Jul 21, 2015 at 11:08:00AM +0100, Marc Zyngier wrote: > > Now that the basic ACPI GSI code is irq domain aware, make sure > > that the ACPI support in the GIC doesn't pointlessly deviate from > > the DT path. > > > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > > --- > > drivers/irqchip/irq-gic.c | 17 ++++++----------- > > include/linux/irqchip/arm-gic.h | 2 +- > > 2 files changed, 7 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > > index b41ccf5..f5d365d 100644 > > --- a/drivers/irqchip/irq-gic.c > > +++ b/drivers/irqchip/irq-gic.c > > @@ -813,8 +813,6 @@ static int gic_irq_domain_xlate(struct irq_domain *d, > > { > > unsigned long ret = 0; > > > > - if (irq_domain_get_of_node(d) != controller) > > - return -EINVAL; > This change seems to have nothing to do with the description. It has everything to do with making this function usable in the context of ACPI ;-). This is another ugly aspect of the irqdomain part, where "controller" is actually the device_node extracted from of_phandle_args. This will actually be the domain_token, and this comparison would fail with ACPI. I may add another patch for that. On DT, this is actually pretty useless, as we're always registering the GIC domain with its device_node, so this is really guaranteed to match. Thanks, M.
On Tue, Jul 21, 2015 at 11:08:00AM +0100, Marc Zyngier wrote: > Now that the basic ACPI GSI code is irq domain aware, make sure > that the ACPI support in the GIC doesn't pointlessly deviate from > the DT path. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > drivers/irqchip/irq-gic.c | 17 ++++++----------- > include/linux/irqchip/arm-gic.h | 2 +- > 2 files changed, 7 insertions(+), 12 deletions(-) > > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > index b41ccf5..f5d365d 100644 > --- a/drivers/irqchip/irq-gic.c > +++ b/drivers/irqchip/irq-gic.c > @@ -813,8 +813,6 @@ static int gic_irq_domain_xlate(struct irq_domain *d, > { > unsigned long ret = 0; > > - if (irq_domain_get_of_node(d) != controller) > - return -EINVAL; > if (intsize < 3) > return -EINVAL; > > @@ -887,7 +885,7 @@ void gic_set_irqchip_flags(unsigned long flags) > > void __init gic_init_bases(unsigned int gic_nr, int irq_start, > void __iomem *dist_base, void __iomem *cpu_base, > - u32 percpu_offset, struct device_node *node) > + u32 percpu_offset, void *domain_token) > { > irq_hw_number_t hwirq_base; > struct gic_chip_data *gic; > @@ -946,8 +944,8 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, > gic_irqs = 1020; > gic->gic_irqs = gic_irqs; > > - if (node) { /* DT case */ > - gic->domain = irq_domain_add_linear(node, gic_irqs, > + if (domain_token) { /* DT/ACPI case */ > + gic->domain = irq_domain_add_linear(domain_token, gic_irqs, > &gic_irq_domain_hierarchy_ops, > gic); > } else { /* Non-DT case */ > @@ -973,7 +971,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, > irq_base = irq_start; > } > > - gic->domain = irq_domain_add_legacy(node, gic_irqs, irq_base, > + gic->domain = irq_domain_add_legacy(NULL, gic_irqs, irq_base, > hwirq_base, &gic_irq_domain_ops, gic); > } > > @@ -1132,12 +1130,9 @@ gic_v2_acpi_init(struct acpi_table_header *table) > } > > /* > - * 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()). > + * Initialize zero GIC instance (no multi-GIC support). > */ > - gic_init_bases(0, -1, dist_base, cpu_base, 0, NULL); > - irq_set_default_host(gic_data[0].domain); > + gic_init_bases(0, -1, dist_base, cpu_base, 0, (void *)ACPI_IRQ_MODEL_GIC); Nit: the acpi_irq_model_id enum starts from 0, I do not think we will use the IRQ domain look-up for the ACPI_IRQ_MODEL_PIC but we have to be careful anyway. Lorenzo > > acpi_irq_model = ACPI_IRQ_MODEL_GIC; > return 0; > diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h > index 9de976b..97799b7 100644 > --- a/include/linux/irqchip/arm-gic.h > +++ b/include/linux/irqchip/arm-gic.h > @@ -97,7 +97,7 @@ struct device_node; > > void gic_set_irqchip_flags(unsigned long flags); > void gic_init_bases(unsigned int, int, void __iomem *, void __iomem *, > - u32 offset, struct device_node *); > + u32 offset, void *); > void gic_cascade_irq(unsigned int gic_nr, unsigned int irq); > void gic_cpu_if_down(void); > > -- > 2.1.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 21/07/15 19:05, Lorenzo Pieralisi wrote: > On Tue, Jul 21, 2015 at 11:08:00AM +0100, Marc Zyngier wrote: >> Now that the basic ACPI GSI code is irq domain aware, make sure >> that the ACPI support in the GIC doesn't pointlessly deviate from >> the DT path. >> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >> --- >> drivers/irqchip/irq-gic.c | 17 ++++++----------- >> include/linux/irqchip/arm-gic.h | 2 +- >> 2 files changed, 7 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >> index b41ccf5..f5d365d 100644 >> --- a/drivers/irqchip/irq-gic.c >> +++ b/drivers/irqchip/irq-gic.c >> @@ -813,8 +813,6 @@ static int gic_irq_domain_xlate(struct irq_domain *d, >> { >> unsigned long ret = 0; >> >> - if (irq_domain_get_of_node(d) != controller) >> - return -EINVAL; >> if (intsize < 3) >> return -EINVAL; >> >> @@ -887,7 +885,7 @@ void gic_set_irqchip_flags(unsigned long flags) >> >> void __init gic_init_bases(unsigned int gic_nr, int irq_start, >> void __iomem *dist_base, void __iomem *cpu_base, >> - u32 percpu_offset, struct device_node *node) >> + u32 percpu_offset, void *domain_token) >> { >> irq_hw_number_t hwirq_base; >> struct gic_chip_data *gic; >> @@ -946,8 +944,8 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, >> gic_irqs = 1020; >> gic->gic_irqs = gic_irqs; >> >> - if (node) { /* DT case */ >> - gic->domain = irq_domain_add_linear(node, gic_irqs, >> + if (domain_token) { /* DT/ACPI case */ >> + gic->domain = irq_domain_add_linear(domain_token, gic_irqs, >> &gic_irq_domain_hierarchy_ops, >> gic); >> } else { /* Non-DT case */ >> @@ -973,7 +971,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, >> irq_base = irq_start; >> } >> >> - gic->domain = irq_domain_add_legacy(node, gic_irqs, irq_base, >> + gic->domain = irq_domain_add_legacy(NULL, gic_irqs, irq_base, >> hwirq_base, &gic_irq_domain_ops, gic); >> } >> >> @@ -1132,12 +1130,9 @@ gic_v2_acpi_init(struct acpi_table_header *table) >> } >> >> /* >> - * 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()). >> + * Initialize zero GIC instance (no multi-GIC support). >> */ >> - gic_init_bases(0, -1, dist_base, cpu_base, 0, NULL); >> - irq_set_default_host(gic_data[0].domain); >> + gic_init_bases(0, -1, dist_base, cpu_base, 0, (void *)ACPI_IRQ_MODEL_GIC); > > Nit: the acpi_irq_model_id enum starts from 0, I do not think we will > use the IRQ domain look-up for the ACPI_IRQ_MODEL_PIC but we have > to be careful anyway. Yeah, I noticed that one too, but couldn't imagine the PIC being migrated to that model just yet. It looks like it would be pretty harmless to set ACPI_IRQ_MODEL_PIC to 1, and introduce ACPI_IRQ_MODEL_ILLEGAL as zero. Thanks, M.
On 07/22/2015 02:12 AM, Marc Zyngier wrote: > On 21/07/15 19:05, Lorenzo Pieralisi wrote: >> On Tue, Jul 21, 2015 at 11:08:00AM +0100, Marc Zyngier wrote: >>> Now that the basic ACPI GSI code is irq domain aware, make sure >>> that the ACPI support in the GIC doesn't pointlessly deviate from >>> the DT path. >>> >>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >>> --- >>> drivers/irqchip/irq-gic.c | 17 ++++++----------- >>> include/linux/irqchip/arm-gic.h | 2 +- >>> 2 files changed, 7 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >>> index b41ccf5..f5d365d 100644 >>> --- a/drivers/irqchip/irq-gic.c >>> +++ b/drivers/irqchip/irq-gic.c >>> @@ -813,8 +813,6 @@ static int gic_irq_domain_xlate(struct irq_domain *d, >>> { >>> unsigned long ret = 0; >>> >>> - if (irq_domain_get_of_node(d) != controller) >>> - return -EINVAL; >>> if (intsize < 3) >>> return -EINVAL; >>> >>> @@ -887,7 +885,7 @@ void gic_set_irqchip_flags(unsigned long flags) >>> >>> void __init gic_init_bases(unsigned int gic_nr, int irq_start, >>> void __iomem *dist_base, void __iomem *cpu_base, >>> - u32 percpu_offset, struct device_node *node) >>> + u32 percpu_offset, void *domain_token) >>> { >>> irq_hw_number_t hwirq_base; >>> struct gic_chip_data *gic; >>> @@ -946,8 +944,8 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, >>> gic_irqs = 1020; >>> gic->gic_irqs = gic_irqs; >>> >>> - if (node) { /* DT case */ >>> - gic->domain = irq_domain_add_linear(node, gic_irqs, >>> + if (domain_token) { /* DT/ACPI case */ >>> + gic->domain = irq_domain_add_linear(domain_token, gic_irqs, >>> &gic_irq_domain_hierarchy_ops, >>> gic); >>> } else { /* Non-DT case */ >>> @@ -973,7 +971,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, >>> irq_base = irq_start; >>> } >>> >>> - gic->domain = irq_domain_add_legacy(node, gic_irqs, irq_base, >>> + gic->domain = irq_domain_add_legacy(NULL, gic_irqs, irq_base, >>> hwirq_base, &gic_irq_domain_ops, gic); >>> } >>> >>> @@ -1132,12 +1130,9 @@ gic_v2_acpi_init(struct acpi_table_header *table) >>> } >>> >>> /* >>> - * 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()). >>> + * Initialize zero GIC instance (no multi-GIC support). >>> */ >>> - gic_init_bases(0, -1, dist_base, cpu_base, 0, NULL); >>> - irq_set_default_host(gic_data[0].domain); >>> + gic_init_bases(0, -1, dist_base, cpu_base, 0, (void *)ACPI_IRQ_MODEL_GIC); >> >> Nit: the acpi_irq_model_id enum starts from 0, I do not think we will >> use the IRQ domain look-up for the ACPI_IRQ_MODEL_PIC but we have >> to be careful anyway. > > Yeah, I noticed that one too, but couldn't imagine the PIC being > migrated to that model just yet. It looks like it would be pretty > harmless to set ACPI_IRQ_MODEL_PIC to 1, and introduce > ACPI_IRQ_MODEL_ILLEGAL as zero. I think this will be a problem, because acpi_irq_model_id enum actually is defined by the ACPI spec, and the value is used to report to BIOS the current interrupt model used by OS, see section 5.8.1 _PIC Method in ACPI 6.0: 0 – PIC mode 1 – APIC mode 2 – SAPIC mode Other values –Reserved so we can't set ACPI_IRQ_MODEL_PIC to 1 as it may break the firmware, also _PIC method actually is not needed for ARM platform at all, we don't need to report to firmware the interrupt model used by OS on ARM, it only used by legacy IA platforms, actually I'm planning to remove acpi_irq_model_id on ARM64. So to me acpi_irq_model_id is suitable for the token, can we use another one as the token? how about the GIC ID in the MADT table? and this also can be used for x86 (IOAPIC IDs) too. Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/22/2015 04:35 PM, Hanjun Guo wrote: > On 07/22/2015 02:12 AM, Marc Zyngier wrote: >> On 21/07/15 19:05, Lorenzo Pieralisi wrote: >>> On Tue, Jul 21, 2015 at 11:08:00AM +0100, Marc Zyngier wrote: >>>> Now that the basic ACPI GSI code is irq domain aware, make sure >>>> that the ACPI support in the GIC doesn't pointlessly deviate from >>>> the DT path. >>>> >>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >>>> --- >>>> drivers/irqchip/irq-gic.c | 17 ++++++----------- >>>> include/linux/irqchip/arm-gic.h | 2 +- >>>> 2 files changed, 7 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >>>> index b41ccf5..f5d365d 100644 >>>> --- a/drivers/irqchip/irq-gic.c >>>> +++ b/drivers/irqchip/irq-gic.c >>>> @@ -813,8 +813,6 @@ static int gic_irq_domain_xlate(struct >>>> irq_domain *d, >>>> { >>>> unsigned long ret = 0; >>>> >>>> - if (irq_domain_get_of_node(d) != controller) >>>> - return -EINVAL; >>>> if (intsize < 3) >>>> return -EINVAL; >>>> >>>> @@ -887,7 +885,7 @@ void gic_set_irqchip_flags(unsigned long flags) >>>> >>>> void __init gic_init_bases(unsigned int gic_nr, int irq_start, >>>> void __iomem *dist_base, void __iomem *cpu_base, >>>> - u32 percpu_offset, struct device_node *node) >>>> + u32 percpu_offset, void *domain_token) >>>> { >>>> irq_hw_number_t hwirq_base; >>>> struct gic_chip_data *gic; >>>> @@ -946,8 +944,8 @@ void __init gic_init_bases(unsigned int gic_nr, >>>> int irq_start, >>>> gic_irqs = 1020; >>>> gic->gic_irqs = gic_irqs; >>>> >>>> - if (node) { /* DT case */ >>>> - gic->domain = irq_domain_add_linear(node, gic_irqs, >>>> + if (domain_token) { /* DT/ACPI case */ >>>> + gic->domain = irq_domain_add_linear(domain_token, gic_irqs, >>>> &gic_irq_domain_hierarchy_ops, >>>> gic); >>>> } else { /* Non-DT case */ >>>> @@ -973,7 +971,7 @@ void __init gic_init_bases(unsigned int gic_nr, >>>> int irq_start, >>>> irq_base = irq_start; >>>> } >>>> >>>> - gic->domain = irq_domain_add_legacy(node, gic_irqs, irq_base, >>>> + gic->domain = irq_domain_add_legacy(NULL, gic_irqs, irq_base, >>>> hwirq_base, &gic_irq_domain_ops, gic); >>>> } >>>> >>>> @@ -1132,12 +1130,9 @@ gic_v2_acpi_init(struct acpi_table_header >>>> *table) >>>> } >>>> >>>> /* >>>> - * 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()). >>>> + * Initialize zero GIC instance (no multi-GIC support). >>>> */ >>>> - gic_init_bases(0, -1, dist_base, cpu_base, 0, NULL); >>>> - irq_set_default_host(gic_data[0].domain); >>>> + gic_init_bases(0, -1, dist_base, cpu_base, 0, (void >>>> *)ACPI_IRQ_MODEL_GIC); >>> >>> Nit: the acpi_irq_model_id enum starts from 0, I do not think we will >>> use the IRQ domain look-up for the ACPI_IRQ_MODEL_PIC but we have >>> to be careful anyway. >> >> Yeah, I noticed that one too, but couldn't imagine the PIC being >> migrated to that model just yet. It looks like it would be pretty >> harmless to set ACPI_IRQ_MODEL_PIC to 1, and introduce >> ACPI_IRQ_MODEL_ILLEGAL as zero. > > I think this will be a problem, because acpi_irq_model_id enum actually > is defined by the ACPI spec, and the value is used to report to BIOS > the current interrupt model used by OS, see section 5.8.1 _PIC Method > in ACPI 6.0: > > 0 – PIC mode > 1 – APIC mode > 2 – SAPIC mode > Other values –Reserved > > so we can't set ACPI_IRQ_MODEL_PIC to 1 as it may break the firmware, > also _PIC method actually is not needed for ARM platform at all, we > don't need to report to firmware the interrupt model used by OS on > ARM, it only used by legacy IA platforms, actually I'm planning to > remove acpi_irq_model_id on ARM64. > > So to me acpi_irq_model_id is suitable for the token, can we use > another one as the token? how about the GIC ID in the MADT table? > and this also can be used for x86 (IOAPIC IDs) too. or just introduce a similar enum as acpi_irq_model for this purpose, as acpi_irq_model is for _PIC method. Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22/07/15 09:35, Hanjun Guo wrote: > On 07/22/2015 02:12 AM, Marc Zyngier wrote: >> On 21/07/15 19:05, Lorenzo Pieralisi wrote: >>> On Tue, Jul 21, 2015 at 11:08:00AM +0100, Marc Zyngier wrote: >>>> Now that the basic ACPI GSI code is irq domain aware, make sure >>>> that the ACPI support in the GIC doesn't pointlessly deviate from >>>> the DT path. >>>> >>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >>>> --- >>>> drivers/irqchip/irq-gic.c | 17 ++++++----------- >>>> include/linux/irqchip/arm-gic.h | 2 +- >>>> 2 files changed, 7 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >>>> index b41ccf5..f5d365d 100644 >>>> --- a/drivers/irqchip/irq-gic.c >>>> +++ b/drivers/irqchip/irq-gic.c >>>> @@ -813,8 +813,6 @@ static int gic_irq_domain_xlate(struct irq_domain *d, >>>> { >>>> unsigned long ret = 0; >>>> >>>> - if (irq_domain_get_of_node(d) != controller) >>>> - return -EINVAL; >>>> if (intsize < 3) >>>> return -EINVAL; >>>> >>>> @@ -887,7 +885,7 @@ void gic_set_irqchip_flags(unsigned long flags) >>>> >>>> void __init gic_init_bases(unsigned int gic_nr, int irq_start, >>>> void __iomem *dist_base, void __iomem *cpu_base, >>>> - u32 percpu_offset, struct device_node *node) >>>> + u32 percpu_offset, void *domain_token) >>>> { >>>> irq_hw_number_t hwirq_base; >>>> struct gic_chip_data *gic; >>>> @@ -946,8 +944,8 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, >>>> gic_irqs = 1020; >>>> gic->gic_irqs = gic_irqs; >>>> >>>> - if (node) { /* DT case */ >>>> - gic->domain = irq_domain_add_linear(node, gic_irqs, >>>> + if (domain_token) { /* DT/ACPI case */ >>>> + gic->domain = irq_domain_add_linear(domain_token, gic_irqs, >>>> &gic_irq_domain_hierarchy_ops, >>>> gic); >>>> } else { /* Non-DT case */ >>>> @@ -973,7 +971,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, >>>> irq_base = irq_start; >>>> } >>>> >>>> - gic->domain = irq_domain_add_legacy(node, gic_irqs, irq_base, >>>> + gic->domain = irq_domain_add_legacy(NULL, gic_irqs, irq_base, >>>> hwirq_base, &gic_irq_domain_ops, gic); >>>> } >>>> >>>> @@ -1132,12 +1130,9 @@ gic_v2_acpi_init(struct acpi_table_header *table) >>>> } >>>> >>>> /* >>>> - * 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()). >>>> + * Initialize zero GIC instance (no multi-GIC support). >>>> */ >>>> - gic_init_bases(0, -1, dist_base, cpu_base, 0, NULL); >>>> - irq_set_default_host(gic_data[0].domain); >>>> + gic_init_bases(0, -1, dist_base, cpu_base, 0, (void *)ACPI_IRQ_MODEL_GIC); >>> >>> Nit: the acpi_irq_model_id enum starts from 0, I do not think we will >>> use the IRQ domain look-up for the ACPI_IRQ_MODEL_PIC but we have >>> to be careful anyway. >> >> Yeah, I noticed that one too, but couldn't imagine the PIC being >> migrated to that model just yet. It looks like it would be pretty >> harmless to set ACPI_IRQ_MODEL_PIC to 1, and introduce >> ACPI_IRQ_MODEL_ILLEGAL as zero. > > I think this will be a problem, because acpi_irq_model_id enum actually > is defined by the ACPI spec, and the value is used to report to BIOS > the current interrupt model used by OS, see section 5.8.1 _PIC Method > in ACPI 6.0: > > 0 – PIC mode > 1 – APIC mode > 2 – SAPIC mode > Other values –Reserved Ah, right. > so we can't set ACPI_IRQ_MODEL_PIC to 1 as it may break the firmware, > also _PIC method actually is not needed for ARM platform at all, we > don't need to report to firmware the interrupt model used by OS on > ARM, it only used by legacy IA platforms, actually I'm planning to > remove acpi_irq_model_id on ARM64. Remove? I don't get it. Either ACPI_IRQ_MODEL_GIC is a leval value, and you can't remove it, or it is not, and I wonder how it ended up here the first place. > So to me acpi_irq_model_id is suitable for the token, can we use is *not* ? > another one as the token? how about the GIC ID in the MADT table? > and this also can be used for x86 (IOAPIC IDs) too. You can use whatever you want, just not a pointer. I'll add a token parameter to the acpi_set_irq_model function that I mentioned in my reply to Lorenzo. Thanks, M.
On 07/22/2015 04:53 PM, Marc Zyngier wrote: > On 22/07/15 09:35, Hanjun Guo wrote: >> On 07/22/2015 02:12 AM, Marc Zyngier wrote: >>> On 21/07/15 19:05, Lorenzo Pieralisi wrote: >>>> On Tue, Jul 21, 2015 at 11:08:00AM +0100, Marc Zyngier wrote: >>>>> Now that the basic ACPI GSI code is irq domain aware, make sure >>>>> that the ACPI support in the GIC doesn't pointlessly deviate from >>>>> the DT path. >>>>> >>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >>>>> --- >>>>> drivers/irqchip/irq-gic.c | 17 ++++++----------- >>>>> include/linux/irqchip/arm-gic.h | 2 +- >>>>> 2 files changed, 7 insertions(+), 12 deletions(-) >>>>> >>>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >>>>> index b41ccf5..f5d365d 100644 >>>>> --- a/drivers/irqchip/irq-gic.c >>>>> +++ b/drivers/irqchip/irq-gic.c >>>>> @@ -813,8 +813,6 @@ static int gic_irq_domain_xlate(struct irq_domain *d, >>>>> { >>>>> unsigned long ret = 0; >>>>> >>>>> - if (irq_domain_get_of_node(d) != controller) >>>>> - return -EINVAL; >>>>> if (intsize < 3) >>>>> return -EINVAL; >>>>> >>>>> @@ -887,7 +885,7 @@ void gic_set_irqchip_flags(unsigned long flags) >>>>> >>>>> void __init gic_init_bases(unsigned int gic_nr, int irq_start, >>>>> void __iomem *dist_base, void __iomem *cpu_base, >>>>> - u32 percpu_offset, struct device_node *node) >>>>> + u32 percpu_offset, void *domain_token) >>>>> { >>>>> irq_hw_number_t hwirq_base; >>>>> struct gic_chip_data *gic; >>>>> @@ -946,8 +944,8 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, >>>>> gic_irqs = 1020; >>>>> gic->gic_irqs = gic_irqs; >>>>> >>>>> - if (node) { /* DT case */ >>>>> - gic->domain = irq_domain_add_linear(node, gic_irqs, >>>>> + if (domain_token) { /* DT/ACPI case */ >>>>> + gic->domain = irq_domain_add_linear(domain_token, gic_irqs, >>>>> &gic_irq_domain_hierarchy_ops, >>>>> gic); >>>>> } else { /* Non-DT case */ >>>>> @@ -973,7 +971,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, >>>>> irq_base = irq_start; >>>>> } >>>>> >>>>> - gic->domain = irq_domain_add_legacy(node, gic_irqs, irq_base, >>>>> + gic->domain = irq_domain_add_legacy(NULL, gic_irqs, irq_base, >>>>> hwirq_base, &gic_irq_domain_ops, gic); >>>>> } >>>>> >>>>> @@ -1132,12 +1130,9 @@ gic_v2_acpi_init(struct acpi_table_header *table) >>>>> } >>>>> >>>>> /* >>>>> - * 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()). >>>>> + * Initialize zero GIC instance (no multi-GIC support). >>>>> */ >>>>> - gic_init_bases(0, -1, dist_base, cpu_base, 0, NULL); >>>>> - irq_set_default_host(gic_data[0].domain); >>>>> + gic_init_bases(0, -1, dist_base, cpu_base, 0, (void *)ACPI_IRQ_MODEL_GIC); >>>> >>>> Nit: the acpi_irq_model_id enum starts from 0, I do not think we will >>>> use the IRQ domain look-up for the ACPI_IRQ_MODEL_PIC but we have >>>> to be careful anyway. >>> >>> Yeah, I noticed that one too, but couldn't imagine the PIC being >>> migrated to that model just yet. It looks like it would be pretty >>> harmless to set ACPI_IRQ_MODEL_PIC to 1, and introduce >>> ACPI_IRQ_MODEL_ILLEGAL as zero. >> >> I think this will be a problem, because acpi_irq_model_id enum actually >> is defined by the ACPI spec, and the value is used to report to BIOS >> the current interrupt model used by OS, see section 5.8.1 _PIC Method >> in ACPI 6.0: >> >> 0 – PIC mode >> 1 – APIC mode >> 2 – SAPIC mode >> Other values –Reserved > > Ah, right. > >> so we can't set ACPI_IRQ_MODEL_PIC to 1 as it may break the firmware, >> also _PIC method actually is not needed for ARM platform at all, we >> don't need to report to firmware the interrupt model used by OS on >> ARM, it only used by legacy IA platforms, actually I'm planning to >> remove acpi_irq_model_id on ARM64. > > Remove? I don't get it. Either ACPI_IRQ_MODEL_GIC is a leval value, and > you can't remove it, or it is not, and I wonder how it ended up here the > first place. The best solution is that we don't call _PIC method on ARM at all (acpi_irq_model is used for _PIC method), but we can live with it since there is no harm if ARM firmware don't care about the value or ARM firmware don't define _PIC method in DSDT. > >> So to me acpi_irq_model_id is suitable for the token, can we use > is *not* ? >> another one as the token? how about the GIC ID in the MADT table? >> and this also can be used for x86 (IOAPIC IDs) too. > > You can use whatever you want, just not a pointer. I'll add a token > parameter to the acpi_set_irq_model function that I mentioned in my > reply to Lorenzo. Thank you, I will wait for the v2 to rebase my ACPI GICv3 patches. Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index b41ccf5..f5d365d 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -813,8 +813,6 @@ static int gic_irq_domain_xlate(struct irq_domain *d, { unsigned long ret = 0; - if (irq_domain_get_of_node(d) != controller) - return -EINVAL; if (intsize < 3) return -EINVAL; @@ -887,7 +885,7 @@ void gic_set_irqchip_flags(unsigned long flags) void __init gic_init_bases(unsigned int gic_nr, int irq_start, void __iomem *dist_base, void __iomem *cpu_base, - u32 percpu_offset, struct device_node *node) + u32 percpu_offset, void *domain_token) { irq_hw_number_t hwirq_base; struct gic_chip_data *gic; @@ -946,8 +944,8 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, gic_irqs = 1020; gic->gic_irqs = gic_irqs; - if (node) { /* DT case */ - gic->domain = irq_domain_add_linear(node, gic_irqs, + if (domain_token) { /* DT/ACPI case */ + gic->domain = irq_domain_add_linear(domain_token, gic_irqs, &gic_irq_domain_hierarchy_ops, gic); } else { /* Non-DT case */ @@ -973,7 +971,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start, irq_base = irq_start; } - gic->domain = irq_domain_add_legacy(node, gic_irqs, irq_base, + gic->domain = irq_domain_add_legacy(NULL, gic_irqs, irq_base, hwirq_base, &gic_irq_domain_ops, gic); } @@ -1132,12 +1130,9 @@ gic_v2_acpi_init(struct acpi_table_header *table) } /* - * 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()). + * Initialize zero GIC instance (no multi-GIC support). */ - gic_init_bases(0, -1, dist_base, cpu_base, 0, NULL); - irq_set_default_host(gic_data[0].domain); + gic_init_bases(0, -1, dist_base, cpu_base, 0, (void *)ACPI_IRQ_MODEL_GIC); acpi_irq_model = ACPI_IRQ_MODEL_GIC; return 0; diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h index 9de976b..97799b7 100644 --- a/include/linux/irqchip/arm-gic.h +++ b/include/linux/irqchip/arm-gic.h @@ -97,7 +97,7 @@ struct device_node; void gic_set_irqchip_flags(unsigned long flags); void gic_init_bases(unsigned int, int, void __iomem *, void __iomem *, - u32 offset, struct device_node *); + u32 offset, void *); void gic_cascade_irq(unsigned int gic_nr, unsigned int irq); void gic_cpu_if_down(void);
Now that the basic ACPI GSI code is irq domain aware, make sure that the ACPI support in the GIC doesn't pointlessly deviate from the DT path. Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- drivers/irqchip/irq-gic.c | 17 ++++++----------- include/linux/irqchip/arm-gic.h | 2 +- 2 files changed, 7 insertions(+), 12 deletions(-)