| Message ID | 1409583475-6978-14-git-send-email-hanjun.guo@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/09/14 15:57, Hanjun Guo wrote: > From: Tomasz Nowicki <tomasz.nowicki@linaro.org> > > ACPI kernel uses MADT table for proper GIC initialization. It needs to > parse GIC related subtables, collect CPU interface and distributor > addresses and call driver initialization function (which is hardware > abstraction agnostic). In a similar way, FDT initialize GICv1/2. > > NOTE: This commit allow to initialize GICv1/2 only. I cannot help but notice that there is no support for KVM here. It'd be good to add a note to that effect, so that people do not expect virtualization support to be working when booting with ACPI. > Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org> > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > --- > arch/arm64/include/asm/acpi.h | 2 - > arch/arm64/kernel/acpi.c | 23 +++++++ > arch/arm64/kernel/irq.c | 5 ++ > drivers/irqchip/irq-gic.c | 114 ++++++++++++++++++++++++++++++++++ > include/linux/irqchip/arm-gic-acpi.h | 33 ++++++++++ > 5 files changed, 175 insertions(+), 2 deletions(-) > create mode 100644 include/linux/irqchip/arm-gic-acpi.h > > diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h > index a867467..5d2ab63 100644 > --- a/arch/arm64/include/asm/acpi.h > +++ b/arch/arm64/include/asm/acpi.h > @@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void); > extern int (*acpi_suspend_lowlevel)(void); > #define acpi_wakeup_address 0 > > -#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535 > - > #else > > static inline bool acpi_psci_present(void) { return false; } > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c > index 354b912..b3b82b0 100644 > --- a/arch/arm64/kernel/acpi.c > +++ b/arch/arm64/kernel/acpi.c > @@ -23,6 +23,7 @@ > #include <linux/irqdomain.h> > #include <linux/bootmem.h> > #include <linux/smp.h> > +#include <linux/irqchip/arm-gic-acpi.h> > > #include <asm/cputype.h> > #include <asm/cpu_ops.h> > @@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void) > pr_err("Can't find FADT or error happened during parsing FADT\n"); > } > > +void __init acpi_gic_init(void) > +{ > + struct acpi_table_header *table; > + acpi_status status; > + acpi_size tbl_size; > + int err; > + > + status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, &table, &tbl_size); > + if (ACPI_FAILURE(status)) { > + const char *msg = acpi_format_exception(status); > + > + pr_err("Failed to get MADT table, %s\n", msg); > + return; > + } > + > + err = gic_v2_acpi_init(table); > + if (err) > + pr_err("Failed to initialize GIC IRQ controller"); What will happen when you get to implement GICv3 support? Another entry like this? Why isn't this entirely contained in the GIC driver? Do I sound like a stuck record? > + > + early_acpi_os_unmap_memory((char *)table, tbl_size); > +} > + > /* > * acpi_suspend_lowlevel() - save kernel state and suspend. > * > diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c > index 0f08dfd..c074d60 100644 > --- a/arch/arm64/kernel/irq.c > +++ b/arch/arm64/kernel/irq.c > @@ -28,6 +28,7 @@ > #include <linux/irqchip.h> > #include <linux/seq_file.h> > #include <linux/ratelimit.h> > +#include <linux/irqchip/arm-gic-acpi.h> > > unsigned long irq_err_count; > > @@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs *)) > void __init init_IRQ(void) > { > irqchip_init(); > + > + if (!handle_arch_irq) > + acpi_gic_init(); > + Why isn't this called from irqchip_init? It would seem like the logical spot to probe an interrupt controller. > if (!handle_arch_irq) > panic("No interrupt controller found."); > } > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > index 4b959e6..85cbf43 100644 > --- a/drivers/irqchip/irq-gic.c > +++ b/drivers/irqchip/irq-gic.c > @@ -33,12 +33,14 @@ > #include <linux/of.h> > #include <linux/of_address.h> > #include <linux/of_irq.h> > +#include <linux/acpi.h> > #include <linux/irqdomain.h> > #include <linux/interrupt.h> > #include <linux/percpu.h> > #include <linux/slab.h> > #include <linux/irqchip/chained_irq.h> > #include <linux/irqchip/arm-gic.h> > +#include <linux/irqchip/arm-gic-acpi.h> > > #include <asm/cputype.h> > #include <asm/irq.h> > @@ -1029,3 +1031,115 @@ IRQCHIP_DECLARE(msm_8660_qgic, "qcom,msm-8660-qgic", gic_of_init); > IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init); > > #endif > + > +#ifdef CONFIG_ACPI > +static u64 dist_phy_base, cpu_phy_base = ULONG_MAX; Please use phys_addr_t for physical addresses. The use of ULONG_MAX looks dodgy. Please have a proper symbol to flag the fact that it hasn't been assigned yet. > + > +static int __init > +gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header, > + const unsigned long end) > +{ > + struct acpi_madt_generic_interrupt *processor; > + u64 gic_cpu_base; phys_addr_t > + processor = (struct acpi_madt_generic_interrupt *)header; > + > + if (BAD_MADT_ENTRY(processor, end)) > + return -EINVAL; > + > + gic_cpu_base = processor->base_address; > + if (!gic_cpu_base) > + return -EFAULT; Is zero an invalid address? > + > + /* > + * There is no support for non-banked GICv1/2 register in ACPI spec. > + * All CPU interface addresses have to be the same. > + */ > + if (cpu_phy_base != ULONG_MAX && gic_cpu_base != cpu_phy_base) > + return -EFAULT; > + > + cpu_phy_base = gic_cpu_base; > + return 0; > +} > + > +static int __init > +gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header, > + const unsigned long end) > +{ > + struct acpi_madt_generic_distributor *dist; > + > + dist = (struct acpi_madt_generic_distributor *)header; > + > + if (BAD_MADT_ENTRY(dist, end)) > + return -EINVAL; > + > + dist_phy_base = dist->base_address; > + if (!dist_phy_base) > + return -EFAULT; Same question about zero. > + > + return 0; > +} > + > +int __init > +gic_v2_acpi_init(struct acpi_table_header *table) > +{ > + void __iomem *cpu_base, *dist_base; > + int count; > + > + /* Collect CPU base addresses */ > + count = acpi_parse_entries(sizeof(struct acpi_table_madt), > + gic_acpi_parse_madt_cpu, table, > + ACPI_MADT_TYPE_GENERIC_INTERRUPT, > + ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES); > + if (count < 0) { > + pr_err("Error during GICC entries parsing\n"); > + return -EFAULT; > + } else if (!count) { > + /* No GICC entries provided, use address from MADT header */ > + struct acpi_table_madt *madt = (struct acpi_table_madt *)table; > + > + if (!madt->address) > + return -EFAULT; > + > + cpu_phy_base = (u64)madt->address; > + } > + > + /* > + * Find distributor base address. We expect one distributor entry since > + * ACPI 5.1 spec neither support multi-GIC instances nor GIC cascade. > + */ > + count = acpi_parse_entries(sizeof(struct acpi_table_madt), > + gic_acpi_parse_madt_distributor, table, > + ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, > + ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES); > + if (count <= 0) { > + pr_err("Error during GICD entries parsing\n"); > + return -EFAULT; > + } else if (count > 1) { > + pr_err("More than one GICD entry detected\n"); > + return -EINVAL; > + } > + > + cpu_base = ioremap(cpu_phy_base, ACPI_GIC_CPU_IF_MEM_SIZE); > + if (!cpu_base) { > + pr_err("Unable to map GICC registers\n"); > + return -ENOMEM; > + } > + > + dist_base = ioremap(dist_phy_base, ACPI_GIC_DIST_MEM_SIZE); > + if (!dist_base) { > + pr_err("Unable to map GICD registers\n"); > + iounmap(cpu_base); > + 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); > + return 0; > +} > +#endif > diff --git a/include/linux/irqchip/arm-gic-acpi.h b/include/linux/irqchip/arm-gic-acpi.h > new file mode 100644 > index 0000000..ce2ae1a8 > --- /dev/null > +++ b/include/linux/irqchip/arm-gic-acpi.h > @@ -0,0 +1,33 @@ > +/* > + * Copyright (C) 2014, Linaro Ltd. > + * Author: Tomasz Nowicki <tomasz.nowicki@linaro.org> > + * > + * 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 > + * published by the Free Software Foundation. > + */ > + > +#ifndef ARM_GIC_ACPI_H_ > +#define ARM_GIC_ACPI_H_ > + > +#include <linux/acpi.h> Do we need linux/acpi.h here? You could have a separate forward declaration of struct acpi_table_header, specially in the light of my last remark below. > + > +#ifdef CONFIG_ACPI > +#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535 With GICv2? I doubt it. > +#define ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES 1 > + > +/* > + * Hard code here, we can not get memory size from MADT (but FDT does), > + * Actually no need to do that, because this size can be inferred > + * from GIC spec. > + */ > +#define ACPI_GIC_DIST_MEM_SIZE (SZ_64K) I don't know which version of the spec you're looking at, but my version of the GICv2 spec has a 4kB distributor. Also, it would be good to make obvious which GIC version this define is about. > +#define ACPI_GIC_CPU_IF_MEM_SIZE (SZ_8K) > + > +void acpi_gic_init(void); > +int gic_v2_acpi_init(struct acpi_table_header *table); > +#else > +static inline void acpi_gic_init(void) { } > +#endif > + > +#endif /* ARM_GIC_ACPI_H_ */ > In the end, why do we need a separate file for this? I cannot see anything that prevents it from being merged with arm-gic.h. Thanks, M.
On 1 September 2014 19:35, Marc Zyngier <marc.zyngier@arm.com> wrote: > > On 01/09/14 15:57, Hanjun Guo wrote: > > From: Tomasz Nowicki <tomasz.nowicki@linaro.org> > > > > ACPI kernel uses MADT table for proper GIC initialization. It needs to > > parse GIC related subtables, collect CPU interface and distributor > > addresses and call driver initialization function (which is hardware > > abstraction agnostic). In a similar way, FDT initialize GICv1/2. > > > > NOTE: This commit allow to initialize GICv1/2 only. > > I cannot help but notice that there is no support for KVM here. It'd be > good to add a note to that effect, so that people do not expect > virtualization support to be working when booting with ACPI. Just a heads up for this, I run up to this problem too while testing and by forcing some values for vgic and arch_timers I could boot a QEMU guest without problems. I'll try and see if it will be easy to handle this more properly instead of hardcoded values. Regards.
On 01.09.2014 19:35, Marc Zyngier wrote: > On 01/09/14 15:57, Hanjun Guo wrote: >> From: Tomasz Nowicki <tomasz.nowicki@linaro.org> >> >> ACPI kernel uses MADT table for proper GIC initialization. It needs to >> parse GIC related subtables, collect CPU interface and distributor >> addresses and call driver initialization function (which is hardware >> abstraction agnostic). In a similar way, FDT initialize GICv1/2. >> >> NOTE: This commit allow to initialize GICv1/2 only. > > I cannot help but notice that there is no support for KVM here. It'd be > good to add a note to that effect, so that people do not expect > virtualization support to be working when booting with ACPI. yes, it is worth mentioning! > >> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org> >> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >> --- >> arch/arm64/include/asm/acpi.h | 2 - >> arch/arm64/kernel/acpi.c | 23 +++++++ >> arch/arm64/kernel/irq.c | 5 ++ >> drivers/irqchip/irq-gic.c | 114 ++++++++++++++++++++++++++++++++++ >> include/linux/irqchip/arm-gic-acpi.h | 33 ++++++++++ >> 5 files changed, 175 insertions(+), 2 deletions(-) >> create mode 100644 include/linux/irqchip/arm-gic-acpi.h >> >> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h >> index a867467..5d2ab63 100644 >> --- a/arch/arm64/include/asm/acpi.h >> +++ b/arch/arm64/include/asm/acpi.h >> @@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void); >> extern int (*acpi_suspend_lowlevel)(void); >> #define acpi_wakeup_address 0 >> >> -#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535 >> - >> #else >> >> static inline bool acpi_psci_present(void) { return false; } >> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c >> index 354b912..b3b82b0 100644 >> --- a/arch/arm64/kernel/acpi.c >> +++ b/arch/arm64/kernel/acpi.c >> @@ -23,6 +23,7 @@ >> #include <linux/irqdomain.h> >> #include <linux/bootmem.h> >> #include <linux/smp.h> >> +#include <linux/irqchip/arm-gic-acpi.h> >> >> #include <asm/cputype.h> >> #include <asm/cpu_ops.h> >> @@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void) >> pr_err("Can't find FADT or error happened during parsing FADT\n"); >> } >> >> +void __init acpi_gic_init(void) >> +{ >> + struct acpi_table_header *table; >> + acpi_status status; >> + acpi_size tbl_size; >> + int err; >> + >> + status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, &table, &tbl_size); >> + if (ACPI_FAILURE(status)) { >> + const char *msg = acpi_format_exception(status); >> + >> + pr_err("Failed to get MADT table, %s\n", msg); >> + return; >> + } >> + >> + err = gic_v2_acpi_init(table); >> + if (err) >> + pr_err("Failed to initialize GIC IRQ controller"); > > What will happen when you get to implement GICv3 support? Another entry > like this? Why isn't this entirely contained in the GIC driver? Do I > sound like a stuck record? There will be another call to GICv3 init: [...] err = gic_v3_acpi_init(table); if (err) err = gic_v2_acpi_init(table); if (err) pr_err("Failed to initialize GIC IRQ controller"); [...] This is the main reason I put common code here. > >> + >> + early_acpi_os_unmap_memory((char *)table, tbl_size); >> +} >> + >> /* >> * acpi_suspend_lowlevel() - save kernel state and suspend. >> * >> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c >> index 0f08dfd..c074d60 100644 >> --- a/arch/arm64/kernel/irq.c >> +++ b/arch/arm64/kernel/irq.c >> @@ -28,6 +28,7 @@ >> #include <linux/irqchip.h> >> #include <linux/seq_file.h> >> #include <linux/ratelimit.h> >> +#include <linux/irqchip/arm-gic-acpi.h> >> >> unsigned long irq_err_count; >> >> @@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs *)) >> void __init init_IRQ(void) >> { >> irqchip_init(); >> + >> + if (!handle_arch_irq) >> + acpi_gic_init(); >> + > > Why isn't this called from irqchip_init? It would seem like the logical > spot to probe an interrupt controller. irqchip.c is OF dependent, I want to decouple these from the very beginning. > >> if (!handle_arch_irq) >> panic("No interrupt controller found."); >> } >> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >> index 4b959e6..85cbf43 100644 >> --- a/drivers/irqchip/irq-gic.c >> +++ b/drivers/irqchip/irq-gic.c >> @@ -33,12 +33,14 @@ >> #include <linux/of.h> >> #include <linux/of_address.h> >> #include <linux/of_irq.h> >> +#include <linux/acpi.h> >> #include <linux/irqdomain.h> >> #include <linux/interrupt.h> >> #include <linux/percpu.h> >> #include <linux/slab.h> >> #include <linux/irqchip/chained_irq.h> >> #include <linux/irqchip/arm-gic.h> >> +#include <linux/irqchip/arm-gic-acpi.h> >> >> #include <asm/cputype.h> >> #include <asm/irq.h> >> @@ -1029,3 +1031,115 @@ IRQCHIP_DECLARE(msm_8660_qgic, "qcom,msm-8660-qgic", gic_of_init); >> IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init); >> >> #endif >> + >> +#ifdef CONFIG_ACPI >> +static u64 dist_phy_base, cpu_phy_base = ULONG_MAX; > > Please use phys_addr_t for physical addresses. The use of ULONG_MAX > looks dodgy. Please have a proper symbol to flag the fact that it hasn't > been assigned yet. Sure, will do. > >> + >> +static int __init >> +gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header, >> + const unsigned long end) >> +{ >> + struct acpi_madt_generic_interrupt *processor; >> + u64 gic_cpu_base; > > phys_addr_t > >> + processor = (struct acpi_madt_generic_interrupt *)header; >> + >> + if (BAD_MADT_ENTRY(processor, end)) >> + return -EINVAL; >> + >> + gic_cpu_base = processor->base_address; >> + if (!gic_cpu_base) >> + return -EFAULT; > > Is zero an invalid address? Yeah, good point. > >> + >> + /* >> + * There is no support for non-banked GICv1/2 register in ACPI spec. >> + * All CPU interface addresses have to be the same. >> + */ >> + if (cpu_phy_base != ULONG_MAX && gic_cpu_base != cpu_phy_base) >> + return -EFAULT; >> + >> + cpu_phy_base = gic_cpu_base; >> + return 0; >> +} >> + >> +static int __init >> +gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header, >> + const unsigned long end) >> +{ >> + struct acpi_madt_generic_distributor *dist; >> + >> + dist = (struct acpi_madt_generic_distributor *)header; >> + >> + if (BAD_MADT_ENTRY(dist, end)) >> + return -EINVAL; >> + >> + dist_phy_base = dist->base_address; >> + if (!dist_phy_base) >> + return -EFAULT; > > Same question about zero. > >> + >> + return 0; >> +} >> + >> +int __init >> +gic_v2_acpi_init(struct acpi_table_header *table) >> +{ >> + void __iomem *cpu_base, *dist_base; >> + int count; >> + >> + /* Collect CPU base addresses */ >> + count = acpi_parse_entries(sizeof(struct acpi_table_madt), >> + gic_acpi_parse_madt_cpu, table, >> + ACPI_MADT_TYPE_GENERIC_INTERRUPT, >> + ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES); >> + if (count < 0) { >> + pr_err("Error during GICC entries parsing\n"); >> + return -EFAULT; >> + } else if (!count) { >> + /* No GICC entries provided, use address from MADT header */ >> + struct acpi_table_madt *madt = (struct acpi_table_madt *)table; >> + >> + if (!madt->address) >> + return -EFAULT; >> + >> + cpu_phy_base = (u64)madt->address; >> + } >> + >> + /* >> + * Find distributor base address. We expect one distributor entry since >> + * ACPI 5.1 spec neither support multi-GIC instances nor GIC cascade. >> + */ >> + count = acpi_parse_entries(sizeof(struct acpi_table_madt), >> + gic_acpi_parse_madt_distributor, table, >> + ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, >> + ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES); >> + if (count <= 0) { >> + pr_err("Error during GICD entries parsing\n"); >> + return -EFAULT; >> + } else if (count > 1) { >> + pr_err("More than one GICD entry detected\n"); >> + return -EINVAL; >> + } >> + >> + cpu_base = ioremap(cpu_phy_base, ACPI_GIC_CPU_IF_MEM_SIZE); >> + if (!cpu_base) { >> + pr_err("Unable to map GICC registers\n"); >> + return -ENOMEM; >> + } >> + >> + dist_base = ioremap(dist_phy_base, ACPI_GIC_DIST_MEM_SIZE); >> + if (!dist_base) { >> + pr_err("Unable to map GICD registers\n"); >> + iounmap(cpu_base); >> + 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); >> + return 0; >> +} >> +#endif >> diff --git a/include/linux/irqchip/arm-gic-acpi.h b/include/linux/irqchip/arm-gic-acpi.h >> new file mode 100644 >> index 0000000..ce2ae1a8 >> --- /dev/null >> +++ b/include/linux/irqchip/arm-gic-acpi.h >> @@ -0,0 +1,33 @@ >> +/* >> + * Copyright (C) 2014, Linaro Ltd. >> + * Author: Tomasz Nowicki <tomasz.nowicki@linaro.org> >> + * >> + * 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 >> + * published by the Free Software Foundation. >> + */ >> + >> +#ifndef ARM_GIC_ACPI_H_ >> +#define ARM_GIC_ACPI_H_ >> + >> +#include <linux/acpi.h> > > Do we need linux/acpi.h here? You could have a separate forward > declaration of struct acpi_table_header, specially in the light of my > last remark below. Indeed, we can do forward declaration instead of #include <linux/acpi.h>. Thanks! > >> + >> +#ifdef CONFIG_ACPI >> +#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535 > > With GICv2? I doubt it. I will create macro for each GIC driver: #define ACPI_MAX_GICV2_CPU_INTERFACE_ENTRIES 8 #define ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES 65535 > >> +#define ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES 1 >> + >> +/* >> + * Hard code here, we can not get memory size from MADT (but FDT does), >> + * Actually no need to do that, because this size can be inferred >> + * from GIC spec. >> + */ >> +#define ACPI_GIC_DIST_MEM_SIZE (SZ_64K) > > I don't know which version of the spec you're looking at, but my version > of the GICv2 spec has a 4kB distributor. Also, it would be good to make > obvious which GIC version this define is about. OK > >> +#define ACPI_GIC_CPU_IF_MEM_SIZE (SZ_8K) >> + >> +void acpi_gic_init(void); >> +int gic_v2_acpi_init(struct acpi_table_header *table); >> +#else >> +static inline void acpi_gic_init(void) { } >> +#endif >> + >> +#endif /* ARM_GIC_ACPI_H_ */ >> > > In the end, why do we need a separate file for this? I cannot see > anything that prevents it from being merged with arm-gic.h. > > Thanks, > > M. > Having only GICv2, it would work. Considering we would do the same for GICv3 (arm-gic-v3.h) there will be register name conflicts for both headers inclusion: [...] #include <linux/irqchip/arm-gic.h> #include <linux/irqchip/arm-gic-v3.h> [...] err = gic_v3_acpi_init(table); if (err) err = gic_v2_acpi_init(table); if (err) pr_err("Failed to initialize GIC IRQ controller"); [...] So instead of changing register names prefix, I choose new header will be less painfully. Regards, Tomasz
On 02/09/14 12:48, Tomasz Nowicki wrote: > On 01.09.2014 19:35, Marc Zyngier wrote: >> On 01/09/14 15:57, Hanjun Guo wrote: >>> From: Tomasz Nowicki <tomasz.nowicki@linaro.org> >>> >>> ACPI kernel uses MADT table for proper GIC initialization. It needs to >>> parse GIC related subtables, collect CPU interface and distributor >>> addresses and call driver initialization function (which is hardware >>> abstraction agnostic). In a similar way, FDT initialize GICv1/2. >>> >>> NOTE: This commit allow to initialize GICv1/2 only. >> >> I cannot help but notice that there is no support for KVM here. It'd be >> good to add a note to that effect, so that people do not expect >> virtualization support to be working when booting with ACPI. > > yes, it is worth mentioning! > >> >>> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org> >>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >>> --- >>> arch/arm64/include/asm/acpi.h | 2 - >>> arch/arm64/kernel/acpi.c | 23 +++++++ >>> arch/arm64/kernel/irq.c | 5 ++ >>> drivers/irqchip/irq-gic.c | 114 ++++++++++++++++++++++++++++++++++ >>> include/linux/irqchip/arm-gic-acpi.h | 33 ++++++++++ >>> 5 files changed, 175 insertions(+), 2 deletions(-) >>> create mode 100644 include/linux/irqchip/arm-gic-acpi.h >>> >>> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h >>> index a867467..5d2ab63 100644 >>> --- a/arch/arm64/include/asm/acpi.h >>> +++ b/arch/arm64/include/asm/acpi.h >>> @@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void); >>> extern int (*acpi_suspend_lowlevel)(void); >>> #define acpi_wakeup_address 0 >>> >>> -#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535 >>> - >>> #else >>> >>> static inline bool acpi_psci_present(void) { return false; } >>> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c >>> index 354b912..b3b82b0 100644 >>> --- a/arch/arm64/kernel/acpi.c >>> +++ b/arch/arm64/kernel/acpi.c >>> @@ -23,6 +23,7 @@ >>> #include <linux/irqdomain.h> >>> #include <linux/bootmem.h> >>> #include <linux/smp.h> >>> +#include <linux/irqchip/arm-gic-acpi.h> >>> >>> #include <asm/cputype.h> >>> #include <asm/cpu_ops.h> >>> @@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void) >>> pr_err("Can't find FADT or error happened during parsing FADT\n"); >>> } >>> >>> +void __init acpi_gic_init(void) >>> +{ >>> + struct acpi_table_header *table; >>> + acpi_status status; >>> + acpi_size tbl_size; >>> + int err; >>> + >>> + status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, &table, &tbl_size); >>> + if (ACPI_FAILURE(status)) { >>> + const char *msg = acpi_format_exception(status); >>> + >>> + pr_err("Failed to get MADT table, %s\n", msg); >>> + return; >>> + } >>> + >>> + err = gic_v2_acpi_init(table); >>> + if (err) >>> + pr_err("Failed to initialize GIC IRQ controller"); >> >> What will happen when you get to implement GICv3 support? Another entry >> like this? Why isn't this entirely contained in the GIC driver? Do I >> sound like a stuck record? > > There will be another call to GICv3 init: > [...] > err = gic_v3_acpi_init(table); > if (err) > err = gic_v2_acpi_init(table); > if (err) > pr_err("Failed to initialize GIC IRQ controller"); > [...] > This is the main reason I put common code here. > >> >>> + >>> + early_acpi_os_unmap_memory((char *)table, tbl_size); >>> +} >>> + >>> /* >>> * acpi_suspend_lowlevel() - save kernel state and suspend. >>> * >>> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c >>> index 0f08dfd..c074d60 100644 >>> --- a/arch/arm64/kernel/irq.c >>> +++ b/arch/arm64/kernel/irq.c >>> @@ -28,6 +28,7 @@ >>> #include <linux/irqchip.h> >>> #include <linux/seq_file.h> >>> #include <linux/ratelimit.h> >>> +#include <linux/irqchip/arm-gic-acpi.h> >>> >>> unsigned long irq_err_count; >>> >>> @@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs *)) >>> void __init init_IRQ(void) >>> { >>> irqchip_init(); >>> + >>> + if (!handle_arch_irq) >>> + acpi_gic_init(); >>> + >> >> Why isn't this called from irqchip_init? It would seem like the logical >> spot to probe an interrupt controller. > > irqchip.c is OF dependent, I want to decouple these from the very > beginning. No. irqchip.c is not OF dependent, it is just that DT is the only thing we support so far. I don't think duplicating the kernel infrastructure "because we're different" is the right way. There is no reason for your probing structure to be artificially different (you're parsing the same information, at the same time). Just put in place a similar probing mechanism, and this will look a lot better. >> >>> if (!handle_arch_irq) >>> panic("No interrupt controller found."); >>> } >>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >>> index 4b959e6..85cbf43 100644 >>> --- a/drivers/irqchip/irq-gic.c >>> +++ b/drivers/irqchip/irq-gic.c >>> @@ -33,12 +33,14 @@ >>> #include <linux/of.h> >>> #include <linux/of_address.h> >>> #include <linux/of_irq.h> >>> +#include <linux/acpi.h> >>> #include <linux/irqdomain.h> >>> #include <linux/interrupt.h> >>> #include <linux/percpu.h> >>> #include <linux/slab.h> >>> #include <linux/irqchip/chained_irq.h> >>> #include <linux/irqchip/arm-gic.h> >>> +#include <linux/irqchip/arm-gic-acpi.h> >>> >>> #include <asm/cputype.h> >>> #include <asm/irq.h> >>> @@ -1029,3 +1031,115 @@ IRQCHIP_DECLARE(msm_8660_qgic, "qcom,msm-8660-qgic", gic_of_init); >>> IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init); >>> >>> #endif >>> + >>> +#ifdef CONFIG_ACPI >>> +static u64 dist_phy_base, cpu_phy_base = ULONG_MAX; >> >> Please use phys_addr_t for physical addresses. The use of ULONG_MAX >> looks dodgy. Please have a proper symbol to flag the fact that it hasn't >> been assigned yet. > Sure, will do. > >> >>> + >>> +static int __init >>> +gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header, >>> + const unsigned long end) >>> +{ >>> + struct acpi_madt_generic_interrupt *processor; >>> + u64 gic_cpu_base; >> >> phys_addr_t >> >>> + processor = (struct acpi_madt_generic_interrupt *)header; >>> + >>> + if (BAD_MADT_ENTRY(processor, end)) >>> + return -EINVAL; >>> + >>> + gic_cpu_base = processor->base_address; >>> + if (!gic_cpu_base) >>> + return -EFAULT; >> >> Is zero an invalid address? > Yeah, good point. >> >>> + >>> + /* >>> + * There is no support for non-banked GICv1/2 register in ACPI spec. >>> + * All CPU interface addresses have to be the same. >>> + */ >>> + if (cpu_phy_base != ULONG_MAX && gic_cpu_base != cpu_phy_base) >>> + return -EFAULT; >>> + >>> + cpu_phy_base = gic_cpu_base; >>> + return 0; >>> +} >>> + >>> +static int __init >>> +gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header, >>> + const unsigned long end) >>> +{ >>> + struct acpi_madt_generic_distributor *dist; >>> + >>> + dist = (struct acpi_madt_generic_distributor *)header; >>> + >>> + if (BAD_MADT_ENTRY(dist, end)) >>> + return -EINVAL; >>> + >>> + dist_phy_base = dist->base_address; >>> + if (!dist_phy_base) >>> + return -EFAULT; >> >> Same question about zero. >> >>> + >>> + return 0; >>> +} >>> + >>> +int __init >>> +gic_v2_acpi_init(struct acpi_table_header *table) >>> +{ >>> + void __iomem *cpu_base, *dist_base; >>> + int count; >>> + >>> + /* Collect CPU base addresses */ >>> + count = acpi_parse_entries(sizeof(struct acpi_table_madt), >>> + gic_acpi_parse_madt_cpu, table, >>> + ACPI_MADT_TYPE_GENERIC_INTERRUPT, >>> + ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES); >>> + if (count < 0) { >>> + pr_err("Error during GICC entries parsing\n"); >>> + return -EFAULT; >>> + } else if (!count) { >>> + /* No GICC entries provided, use address from MADT header */ >>> + struct acpi_table_madt *madt = (struct acpi_table_madt *)table; >>> + >>> + if (!madt->address) >>> + return -EFAULT; >>> + >>> + cpu_phy_base = (u64)madt->address; >>> + } >>> + >>> + /* >>> + * Find distributor base address. We expect one distributor entry since >>> + * ACPI 5.1 spec neither support multi-GIC instances nor GIC cascade. >>> + */ >>> + count = acpi_parse_entries(sizeof(struct acpi_table_madt), >>> + gic_acpi_parse_madt_distributor, table, >>> + ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, >>> + ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES); >>> + if (count <= 0) { >>> + pr_err("Error during GICD entries parsing\n"); >>> + return -EFAULT; >>> + } else if (count > 1) { >>> + pr_err("More than one GICD entry detected\n"); >>> + return -EINVAL; >>> + } >>> + >>> + cpu_base = ioremap(cpu_phy_base, ACPI_GIC_CPU_IF_MEM_SIZE); >>> + if (!cpu_base) { >>> + pr_err("Unable to map GICC registers\n"); >>> + return -ENOMEM; >>> + } >>> + >>> + dist_base = ioremap(dist_phy_base, ACPI_GIC_DIST_MEM_SIZE); >>> + if (!dist_base) { >>> + pr_err("Unable to map GICD registers\n"); >>> + iounmap(cpu_base); >>> + 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); >>> + return 0; >>> +} >>> +#endif >>> diff --git a/include/linux/irqchip/arm-gic-acpi.h b/include/linux/irqchip/arm-gic-acpi.h >>> new file mode 100644 >>> index 0000000..ce2ae1a8 >>> --- /dev/null >>> +++ b/include/linux/irqchip/arm-gic-acpi.h >>> @@ -0,0 +1,33 @@ >>> +/* >>> + * Copyright (C) 2014, Linaro Ltd. >>> + * Author: Tomasz Nowicki <tomasz.nowicki@linaro.org> >>> + * >>> + * 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 >>> + * published by the Free Software Foundation. >>> + */ >>> + >>> +#ifndef ARM_GIC_ACPI_H_ >>> +#define ARM_GIC_ACPI_H_ >>> + >>> +#include <linux/acpi.h> >> >> Do we need linux/acpi.h here? You could have a separate forward >> declaration of struct acpi_table_header, specially in the light of my >> last remark below. > Indeed, we can do forward declaration instead of #include > <linux/acpi.h>. Thanks! > >> >>> + >>> +#ifdef CONFIG_ACPI >>> +#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535 >> >> With GICv2? I doubt it. > I will create macro for each GIC driver: > #define ACPI_MAX_GICV2_CPU_INTERFACE_ENTRIES 8 > #define ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES 65535 Where do you get this value (ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES) from? >> >>> +#define ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES 1 >>> + >>> +/* >>> + * Hard code here, we can not get memory size from MADT (but FDT does), >>> + * Actually no need to do that, because this size can be inferred >>> + * from GIC spec. >>> + */ >>> +#define ACPI_GIC_DIST_MEM_SIZE (SZ_64K) >> >> I don't know which version of the spec you're looking at, but my version >> of the GICv2 spec has a 4kB distributor. Also, it would be good to make >> obvious which GIC version this define is about. > OK > >> >>> +#define ACPI_GIC_CPU_IF_MEM_SIZE (SZ_8K) >>> + >>> +void acpi_gic_init(void); >>> +int gic_v2_acpi_init(struct acpi_table_header *table); >>> +#else >>> +static inline void acpi_gic_init(void) { } >>> +#endif >>> + >>> +#endif /* ARM_GIC_ACPI_H_ */ >>> >> >> In the end, why do we need a separate file for this? I cannot see >> anything that prevents it from being merged with arm-gic.h. >> >> Thanks, >> >> M. >> > Having only GICv2, it would work. Considering we would do the same for > GICv3 (arm-gic-v3.h) there will be register name conflicts for both > headers inclusion: > > [...] > #include <linux/irqchip/arm-gic.h> > #include <linux/irqchip/arm-gic-v3.h> > [...] > err = gic_v3_acpi_init(table); > if (err) > err = gic_v2_acpi_init(table); > if (err) > pr_err("Failed to initialize GIC IRQ controller"); > [...] > So instead of changing register names prefix, I choose new header will > be less painfully. Yes, and this is exactly why I pushed back on that last time. I'll continue saying that interrupt controllers should be self-probing, with ACPI as they are with DT. Even with the restrictions of ACPI and SBSA, we end-up with at least 2 main families of interrupt controllers (GICv2 and GICv3), both with a number of "interesting" variations (GICv2m and GICv4, to only mention those I'm directly involved with). I can safely predict that the above will become a tangled mess within 18 months, and the idea of littering the arch code with a bunch of hardcoded "if (blah())" doesn't fill me with joy and confidence. In summary: we have the infrastructure already, just use it. Thanks, M.
On 2014?09?02? 21:02, Marc Zyngier wrote: > On 02/09/14 12:48, Tomasz Nowicki wrote: >> On 01.09.2014 19:35, Marc Zyngier wrote: >>> On 01/09/14 15:57, Hanjun Guo wrote: >>>> From: Tomasz Nowicki <tomasz.nowicki@linaro.org> >>>> >>>> ACPI kernel uses MADT table for proper GIC initialization. It needs to >>>> parse GIC related subtables, collect CPU interface and distributor >>>> addresses and call driver initialization function (which is hardware >>>> abstraction agnostic). In a similar way, FDT initialize GICv1/2. >>>> >>>> NOTE: This commit allow to initialize GICv1/2 only. >>> I cannot help but notice that there is no support for KVM here. It'd be >>> good to add a note to that effect, so that people do not expect >>> virtualization support to be working when booting with ACPI. >> yes, it is worth mentioning! >> >>>> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org> >>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >>>> --- >>>> arch/arm64/include/asm/acpi.h | 2 - >>>> arch/arm64/kernel/acpi.c | 23 +++++++ >>>> arch/arm64/kernel/irq.c | 5 ++ >>>> drivers/irqchip/irq-gic.c | 114 ++++++++++++++++++++++++++++++++++ >>>> include/linux/irqchip/arm-gic-acpi.h | 33 ++++++++++ >>>> 5 files changed, 175 insertions(+), 2 deletions(-) >>>> create mode 100644 include/linux/irqchip/arm-gic-acpi.h >>>> >>>> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h >>>> index a867467..5d2ab63 100644 >>>> --- a/arch/arm64/include/asm/acpi.h >>>> +++ b/arch/arm64/include/asm/acpi.h >>>> @@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void); >>>> extern int (*acpi_suspend_lowlevel)(void); >>>> #define acpi_wakeup_address 0 >>>> >>>> -#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535 >>>> - >>>> #else >>>> >>>> static inline bool acpi_psci_present(void) { return false; } >>>> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c >>>> index 354b912..b3b82b0 100644 >>>> --- a/arch/arm64/kernel/acpi.c >>>> +++ b/arch/arm64/kernel/acpi.c >>>> @@ -23,6 +23,7 @@ >>>> #include <linux/irqdomain.h> >>>> #include <linux/bootmem.h> >>>> #include <linux/smp.h> >>>> +#include <linux/irqchip/arm-gic-acpi.h> >>>> >>>> #include <asm/cputype.h> >>>> #include <asm/cpu_ops.h> >>>> @@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void) >>>> pr_err("Can't find FADT or error happened during parsing FADT\n"); >>>> } >>>> >>>> +void __init acpi_gic_init(void) >>>> +{ >>>> + struct acpi_table_header *table; >>>> + acpi_status status; >>>> + acpi_size tbl_size; >>>> + int err; >>>> + >>>> + status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, &table, &tbl_size); >>>> + if (ACPI_FAILURE(status)) { >>>> + const char *msg = acpi_format_exception(status); >>>> + >>>> + pr_err("Failed to get MADT table, %s\n", msg); >>>> + return; >>>> + } >>>> + >>>> + err = gic_v2_acpi_init(table); >>>> + if (err) >>>> + pr_err("Failed to initialize GIC IRQ controller"); >>> What will happen when you get to implement GICv3 support? Another entry >>> like this? Why isn't this entirely contained in the GIC driver? Do I >>> sound like a stuck record? >> There will be another call to GICv3 init: >> [...] >> err = gic_v3_acpi_init(table); >> if (err) >> err = gic_v2_acpi_init(table); >> if (err) >> pr_err("Failed to initialize GIC IRQ controller"); >> [...] >> This is the main reason I put common code here. >> >>>> + >>>> + early_acpi_os_unmap_memory((char *)table, tbl_size); >>>> +} >>>> + >>>> /* >>>> * acpi_suspend_lowlevel() - save kernel state and suspend. >>>> * >>>> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c >>>> index 0f08dfd..c074d60 100644 >>>> --- a/arch/arm64/kernel/irq.c >>>> +++ b/arch/arm64/kernel/irq.c >>>> @@ -28,6 +28,7 @@ >>>> #include <linux/irqchip.h> >>>> #include <linux/seq_file.h> >>>> #include <linux/ratelimit.h> >>>> +#include <linux/irqchip/arm-gic-acpi.h> >>>> >>>> unsigned long irq_err_count; >>>> >>>> @@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs *)) >>>> void __init init_IRQ(void) >>>> { >>>> irqchip_init(); >>>> + >>>> + if (!handle_arch_irq) >>>> + acpi_gic_init(); >>>> + >>> Why isn't this called from irqchip_init? It would seem like the logical >>> spot to probe an interrupt controller. >> irqchip.c is OF dependent, I want to decouple these from the very >> beginning. > No. irqchip.c is not OF dependent, it is just that DT is the only thing > we support so far. I don't think duplicating the kernel infrastructure > "because we're different" is the right way. > > There is no reason for your probing structure to be artificially > different (you're parsing the same information, at the same time). Just > put in place a similar probing mechanism, and this will look a lot better. > >>>> if (!handle_arch_irq) >>>> panic("No interrupt controller found."); >>>> } >>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >>>> index 4b959e6..85cbf43 100644 >>>> --- a/drivers/irqchip/irq-gic.c >>>> +++ b/drivers/irqchip/irq-gic.c >>>> @@ -33,12 +33,14 @@ >>>> #include <linux/of.h> >>>> #include <linux/of_address.h> >>>> #include <linux/of_irq.h> >>>> +#include <linux/acpi.h> >>>> #include <linux/irqdomain.h> >>>> #include <linux/interrupt.h> >>>> #include <linux/percpu.h> >>>> #include <linux/slab.h> >>>> #include <linux/irqchip/chained_irq.h> >>>> #include <linux/irqchip/arm-gic.h> >>>> +#include <linux/irqchip/arm-gic-acpi.h> >>>> >>>> #include <asm/cputype.h> >>>> #include <asm/irq.h> >>>> @@ -1029,3 +1031,115 @@ IRQCHIP_DECLARE(msm_8660_qgic, "qcom,msm-8660-qgic", gic_of_init); >>>> IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init); >>>> >>>> #endif >>>> + >>>> +#ifdef CONFIG_ACPI >>>> +static u64 dist_phy_base, cpu_phy_base = ULONG_MAX; >>> Please use phys_addr_t for physical addresses. The use of ULONG_MAX >>> looks dodgy. Please have a proper symbol to flag the fact that it hasn't >>> been assigned yet. >> Sure, will do. >> >>>> + >>>> +static int __init >>>> +gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header, >>>> + const unsigned long end) >>>> +{ >>>> + struct acpi_madt_generic_interrupt *processor; >>>> + u64 gic_cpu_base; >>> phys_addr_t >>> >>>> + processor = (struct acpi_madt_generic_interrupt *)header; >>>> + >>>> + if (BAD_MADT_ENTRY(processor, end)) >>>> + return -EINVAL; >>>> + >>>> + gic_cpu_base = processor->base_address; >>>> + if (!gic_cpu_base) >>>> + return -EFAULT; >>> Is zero an invalid address? >> Yeah, good point. >>>> + >>>> + /* >>>> + * There is no support for non-banked GICv1/2 register in ACPI spec. >>>> + * All CPU interface addresses have to be the same. >>>> + */ >>>> + if (cpu_phy_base != ULONG_MAX && gic_cpu_base != cpu_phy_base) >>>> + return -EFAULT; >>>> + >>>> + cpu_phy_base = gic_cpu_base; >>>> + return 0; >>>> +} >>>> + >>>> +static int __init >>>> +gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header, >>>> + const unsigned long end) >>>> +{ >>>> + struct acpi_madt_generic_distributor *dist; >>>> + >>>> + dist = (struct acpi_madt_generic_distributor *)header; >>>> + >>>> + if (BAD_MADT_ENTRY(dist, end)) >>>> + return -EINVAL; >>>> + >>>> + dist_phy_base = dist->base_address; >>>> + if (!dist_phy_base) >>>> + return -EFAULT; >>> Same question about zero. >>> >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +int __init >>>> +gic_v2_acpi_init(struct acpi_table_header *table) >>>> +{ >>>> + void __iomem *cpu_base, *dist_base; >>>> + int count; >>>> + >>>> + /* Collect CPU base addresses */ >>>> + count = acpi_parse_entries(sizeof(struct acpi_table_madt), >>>> + gic_acpi_parse_madt_cpu, table, >>>> + ACPI_MADT_TYPE_GENERIC_INTERRUPT, >>>> + ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES); >>>> + if (count < 0) { >>>> + pr_err("Error during GICC entries parsing\n"); >>>> + return -EFAULT; >>>> + } else if (!count) { >>>> + /* No GICC entries provided, use address from MADT header */ >>>> + struct acpi_table_madt *madt = (struct acpi_table_madt *)table; >>>> + >>>> + if (!madt->address) >>>> + return -EFAULT; >>>> + >>>> + cpu_phy_base = (u64)madt->address; >>>> + } >>>> + >>>> + /* >>>> + * Find distributor base address. We expect one distributor entry since >>>> + * ACPI 5.1 spec neither support multi-GIC instances nor GIC cascade. >>>> + */ >>>> + count = acpi_parse_entries(sizeof(struct acpi_table_madt), >>>> + gic_acpi_parse_madt_distributor, table, >>>> + ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, >>>> + ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES); >>>> + if (count <= 0) { >>>> + pr_err("Error during GICD entries parsing\n"); >>>> + return -EFAULT; >>>> + } else if (count > 1) { >>>> + pr_err("More than one GICD entry detected\n"); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + cpu_base = ioremap(cpu_phy_base, ACPI_GIC_CPU_IF_MEM_SIZE); >>>> + if (!cpu_base) { >>>> + pr_err("Unable to map GICC registers\n"); >>>> + return -ENOMEM; >>>> + } >>>> + >>>> + dist_base = ioremap(dist_phy_base, ACPI_GIC_DIST_MEM_SIZE); >>>> + if (!dist_base) { >>>> + pr_err("Unable to map GICD registers\n"); >>>> + iounmap(cpu_base); >>>> + 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); >>>> + return 0; >>>> +} >>>> +#endif >>>> diff --git a/include/linux/irqchip/arm-gic-acpi.h b/include/linux/irqchip/arm-gic-acpi.h >>>> new file mode 100644 >>>> index 0000000..ce2ae1a8 >>>> --- /dev/null >>>> +++ b/include/linux/irqchip/arm-gic-acpi.h >>>> @@ -0,0 +1,33 @@ >>>> +/* >>>> + * Copyright (C) 2014, Linaro Ltd. >>>> + * Author: Tomasz Nowicki <tomasz.nowicki@linaro.org> >>>> + * >>>> + * 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 >>>> + * published by the Free Software Foundation. >>>> + */ >>>> + >>>> +#ifndef ARM_GIC_ACPI_H_ >>>> +#define ARM_GIC_ACPI_H_ >>>> + >>>> +#include <linux/acpi.h> >>> Do we need linux/acpi.h here? You could have a separate forward >>> declaration of struct acpi_table_header, specially in the light of my >>> last remark below. >> Indeed, we can do forward declaration instead of #include >> <linux/acpi.h>. Thanks! >> >>>> + >>>> +#ifdef CONFIG_ACPI >>>> +#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535 >>> With GICv2? I doubt it. >> I will create macro for each GIC driver: >> #define ACPI_MAX_GICV2_CPU_INTERFACE_ENTRIES 8 >> #define ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES 65535 > Where do you get this value (ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES) from? This value is for max processors entries in MADT, and we will use it to scan MADT for SMP/GIC Init, I just make it big enough for GICv3/4. since ACPI core will stop scan MADT if the real numbers of processors entries are reached no matter how big ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES is, I think we can just define a number big enough then it will work (x86 and ia64 did the same thing). Thanks Hanjun
On 02/09/14 16:45, Hanjun Guo wrote: > On 2014?09?02? 21:02, Marc Zyngier wrote: >> On 02/09/14 12:48, Tomasz Nowicki wrote: >>> On 01.09.2014 19:35, Marc Zyngier wrote: >>>> On 01/09/14 15:57, Hanjun Guo wrote: >>>>> From: Tomasz Nowicki <tomasz.nowicki@linaro.org> >>>>> >>>>> ACPI kernel uses MADT table for proper GIC initialization. It needs to >>>>> parse GIC related subtables, collect CPU interface and distributor >>>>> addresses and call driver initialization function (which is hardware >>>>> abstraction agnostic). In a similar way, FDT initialize GICv1/2. >>>>> >>>>> NOTE: This commit allow to initialize GICv1/2 only. >>>> I cannot help but notice that there is no support for KVM here. It'd be >>>> good to add a note to that effect, so that people do not expect >>>> virtualization support to be working when booting with ACPI. >>> yes, it is worth mentioning! >>> >>>>> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org> >>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >>>>> --- >>>>> arch/arm64/include/asm/acpi.h | 2 - >>>>> arch/arm64/kernel/acpi.c | 23 +++++++ >>>>> arch/arm64/kernel/irq.c | 5 ++ >>>>> drivers/irqchip/irq-gic.c | 114 ++++++++++++++++++++++++++++++++++ >>>>> include/linux/irqchip/arm-gic-acpi.h | 33 ++++++++++ >>>>> 5 files changed, 175 insertions(+), 2 deletions(-) >>>>> create mode 100644 include/linux/irqchip/arm-gic-acpi.h >>>>> >>>>> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h >>>>> index a867467..5d2ab63 100644 >>>>> --- a/arch/arm64/include/asm/acpi.h >>>>> +++ b/arch/arm64/include/asm/acpi.h >>>>> @@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void); >>>>> extern int (*acpi_suspend_lowlevel)(void); >>>>> #define acpi_wakeup_address 0 >>>>> >>>>> -#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535 >>>>> - >>>>> #else >>>>> >>>>> static inline bool acpi_psci_present(void) { return false; } >>>>> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c >>>>> index 354b912..b3b82b0 100644 >>>>> --- a/arch/arm64/kernel/acpi.c >>>>> +++ b/arch/arm64/kernel/acpi.c >>>>> @@ -23,6 +23,7 @@ >>>>> #include <linux/irqdomain.h> >>>>> #include <linux/bootmem.h> >>>>> #include <linux/smp.h> >>>>> +#include <linux/irqchip/arm-gic-acpi.h> >>>>> >>>>> #include <asm/cputype.h> >>>>> #include <asm/cpu_ops.h> >>>>> @@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void) >>>>> pr_err("Can't find FADT or error happened during parsing FADT\n"); >>>>> } >>>>> >>>>> +void __init acpi_gic_init(void) >>>>> +{ >>>>> + struct acpi_table_header *table; >>>>> + acpi_status status; >>>>> + acpi_size tbl_size; >>>>> + int err; >>>>> + >>>>> + status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, &table, &tbl_size); >>>>> + if (ACPI_FAILURE(status)) { >>>>> + const char *msg = acpi_format_exception(status); >>>>> + >>>>> + pr_err("Failed to get MADT table, %s\n", msg); >>>>> + return; >>>>> + } >>>>> + >>>>> + err = gic_v2_acpi_init(table); >>>>> + if (err) >>>>> + pr_err("Failed to initialize GIC IRQ controller"); >>>> What will happen when you get to implement GICv3 support? Another entry >>>> like this? Why isn't this entirely contained in the GIC driver? Do I >>>> sound like a stuck record? >>> There will be another call to GICv3 init: >>> [...] >>> err = gic_v3_acpi_init(table); >>> if (err) >>> err = gic_v2_acpi_init(table); >>> if (err) >>> pr_err("Failed to initialize GIC IRQ controller"); >>> [...] >>> This is the main reason I put common code here. >>> >>>>> + >>>>> + early_acpi_os_unmap_memory((char *)table, tbl_size); >>>>> +} >>>>> + >>>>> /* >>>>> * acpi_suspend_lowlevel() - save kernel state and suspend. >>>>> * >>>>> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c >>>>> index 0f08dfd..c074d60 100644 >>>>> --- a/arch/arm64/kernel/irq.c >>>>> +++ b/arch/arm64/kernel/irq.c >>>>> @@ -28,6 +28,7 @@ >>>>> #include <linux/irqchip.h> >>>>> #include <linux/seq_file.h> >>>>> #include <linux/ratelimit.h> >>>>> +#include <linux/irqchip/arm-gic-acpi.h> >>>>> >>>>> unsigned long irq_err_count; >>>>> >>>>> @@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs *)) >>>>> void __init init_IRQ(void) >>>>> { >>>>> irqchip_init(); >>>>> + >>>>> + if (!handle_arch_irq) >>>>> + acpi_gic_init(); >>>>> + >>>> Why isn't this called from irqchip_init? It would seem like the logical >>>> spot to probe an interrupt controller. >>> irqchip.c is OF dependent, I want to decouple these from the very >>> beginning. >> No. irqchip.c is not OF dependent, it is just that DT is the only thing >> we support so far. I don't think duplicating the kernel infrastructure >> "because we're different" is the right way. >> >> There is no reason for your probing structure to be artificially >> different (you're parsing the same information, at the same time). Just >> put in place a similar probing mechanism, and this will look a lot better. >> >>>>> if (!handle_arch_irq) >>>>> panic("No interrupt controller found."); >>>>> } >>>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >>>>> index 4b959e6..85cbf43 100644 >>>>> --- a/drivers/irqchip/irq-gic.c >>>>> +++ b/drivers/irqchip/irq-gic.c >>>>> @@ -33,12 +33,14 @@ >>>>> #include <linux/of.h> >>>>> #include <linux/of_address.h> >>>>> #include <linux/of_irq.h> >>>>> +#include <linux/acpi.h> >>>>> #include <linux/irqdomain.h> >>>>> #include <linux/interrupt.h> >>>>> #include <linux/percpu.h> >>>>> #include <linux/slab.h> >>>>> #include <linux/irqchip/chained_irq.h> >>>>> #include <linux/irqchip/arm-gic.h> >>>>> +#include <linux/irqchip/arm-gic-acpi.h> >>>>> >>>>> #include <asm/cputype.h> >>>>> #include <asm/irq.h> >>>>> @@ -1029,3 +1031,115 @@ IRQCHIP_DECLARE(msm_8660_qgic, "qcom,msm-8660-qgic", gic_of_init); >>>>> IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init); >>>>> >>>>> #endif >>>>> + >>>>> +#ifdef CONFIG_ACPI >>>>> +static u64 dist_phy_base, cpu_phy_base = ULONG_MAX; >>>> Please use phys_addr_t for physical addresses. The use of ULONG_MAX >>>> looks dodgy. Please have a proper symbol to flag the fact that it hasn't >>>> been assigned yet. >>> Sure, will do. >>> >>>>> + >>>>> +static int __init >>>>> +gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header, >>>>> + const unsigned long end) >>>>> +{ >>>>> + struct acpi_madt_generic_interrupt *processor; >>>>> + u64 gic_cpu_base; >>>> phys_addr_t >>>> >>>>> + processor = (struct acpi_madt_generic_interrupt *)header; >>>>> + >>>>> + if (BAD_MADT_ENTRY(processor, end)) >>>>> + return -EINVAL; >>>>> + >>>>> + gic_cpu_base = processor->base_address; >>>>> + if (!gic_cpu_base) >>>>> + return -EFAULT; >>>> Is zero an invalid address? >>> Yeah, good point. >>>>> + >>>>> + /* >>>>> + * There is no support for non-banked GICv1/2 register in ACPI spec. >>>>> + * All CPU interface addresses have to be the same. >>>>> + */ >>>>> + if (cpu_phy_base != ULONG_MAX && gic_cpu_base != cpu_phy_base) >>>>> + return -EFAULT; >>>>> + >>>>> + cpu_phy_base = gic_cpu_base; >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int __init >>>>> +gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header, >>>>> + const unsigned long end) >>>>> +{ >>>>> + struct acpi_madt_generic_distributor *dist; >>>>> + >>>>> + dist = (struct acpi_madt_generic_distributor *)header; >>>>> + >>>>> + if (BAD_MADT_ENTRY(dist, end)) >>>>> + return -EINVAL; >>>>> + >>>>> + dist_phy_base = dist->base_address; >>>>> + if (!dist_phy_base) >>>>> + return -EFAULT; >>>> Same question about zero. >>>> >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +int __init >>>>> +gic_v2_acpi_init(struct acpi_table_header *table) >>>>> +{ >>>>> + void __iomem *cpu_base, *dist_base; >>>>> + int count; >>>>> + >>>>> + /* Collect CPU base addresses */ >>>>> + count = acpi_parse_entries(sizeof(struct acpi_table_madt), >>>>> + gic_acpi_parse_madt_cpu, table, >>>>> + ACPI_MADT_TYPE_GENERIC_INTERRUPT, >>>>> + ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES); >>>>> + if (count < 0) { >>>>> + pr_err("Error during GICC entries parsing\n"); >>>>> + return -EFAULT; >>>>> + } else if (!count) { >>>>> + /* No GICC entries provided, use address from MADT header */ >>>>> + struct acpi_table_madt *madt = (struct acpi_table_madt *)table; >>>>> + >>>>> + if (!madt->address) >>>>> + return -EFAULT; >>>>> + >>>>> + cpu_phy_base = (u64)madt->address; >>>>> + } >>>>> + >>>>> + /* >>>>> + * Find distributor base address. We expect one distributor entry since >>>>> + * ACPI 5.1 spec neither support multi-GIC instances nor GIC cascade. >>>>> + */ >>>>> + count = acpi_parse_entries(sizeof(struct acpi_table_madt), >>>>> + gic_acpi_parse_madt_distributor, table, >>>>> + ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, >>>>> + ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES); >>>>> + if (count <= 0) { >>>>> + pr_err("Error during GICD entries parsing\n"); >>>>> + return -EFAULT; >>>>> + } else if (count > 1) { >>>>> + pr_err("More than one GICD entry detected\n"); >>>>> + return -EINVAL; >>>>> + } >>>>> + >>>>> + cpu_base = ioremap(cpu_phy_base, ACPI_GIC_CPU_IF_MEM_SIZE); >>>>> + if (!cpu_base) { >>>>> + pr_err("Unable to map GICC registers\n"); >>>>> + return -ENOMEM; >>>>> + } >>>>> + >>>>> + dist_base = ioremap(dist_phy_base, ACPI_GIC_DIST_MEM_SIZE); >>>>> + if (!dist_base) { >>>>> + pr_err("Unable to map GICD registers\n"); >>>>> + iounmap(cpu_base); >>>>> + 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); >>>>> + return 0; >>>>> +} >>>>> +#endif >>>>> diff --git a/include/linux/irqchip/arm-gic-acpi.h b/include/linux/irqchip/arm-gic-acpi.h >>>>> new file mode 100644 >>>>> index 0000000..ce2ae1a8 >>>>> --- /dev/null >>>>> +++ b/include/linux/irqchip/arm-gic-acpi.h >>>>> @@ -0,0 +1,33 @@ >>>>> +/* >>>>> + * Copyright (C) 2014, Linaro Ltd. >>>>> + * Author: Tomasz Nowicki <tomasz.nowicki@linaro.org> >>>>> + * >>>>> + * 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 >>>>> + * published by the Free Software Foundation. >>>>> + */ >>>>> + >>>>> +#ifndef ARM_GIC_ACPI_H_ >>>>> +#define ARM_GIC_ACPI_H_ >>>>> + >>>>> +#include <linux/acpi.h> >>>> Do we need linux/acpi.h here? You could have a separate forward >>>> declaration of struct acpi_table_header, specially in the light of my >>>> last remark below. >>> Indeed, we can do forward declaration instead of #include >>> <linux/acpi.h>. Thanks! >>> >>>>> + >>>>> +#ifdef CONFIG_ACPI >>>>> +#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535 >>>> With GICv2? I doubt it. >>> I will create macro for each GIC driver: >>> #define ACPI_MAX_GICV2_CPU_INTERFACE_ENTRIES 8 >>> #define ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES 65535 >> Where do you get this value (ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES) from? > > This value is for max processors entries in MADT, and we will use it to scan MADT > for SMP/GIC Init, I just make it big enough for GICv3/4. since ACPI core will stop > scan MADT if the real numbers of processors entries are reached no matter > how big ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES is, I think we can just > define a number big enough then it will work (x86 and ia64 did the same thing). Then it is worth mentioning that this is an arbitrary limit, unrelated to what the architecture describes. Thanks, M.
On 02/09/14 16:45, Hanjun Guo wrote: > On 2014?09?02? 21:02, Marc Zyngier wrote: >> On 02/09/14 12:48, Tomasz Nowicki wrote: >>> On 01.09.2014 19:35, Marc Zyngier wrote: >>>> On 01/09/14 15:57, Hanjun Guo wrote: >>>>> From: Tomasz Nowicki <tomasz.nowicki@linaro.org> >>>>> >>>>> ACPI kernel uses MADT table for proper GIC initialization. It needs to >>>>> parse GIC related subtables, collect CPU interface and distributor >>>>> addresses and call driver initialization function (which is hardware >>>>> abstraction agnostic). In a similar way, FDT initialize GICv1/2. >>>>> >>>>> NOTE: This commit allow to initialize GICv1/2 only. >>>> I cannot help but notice that there is no support for KVM here. It'd be >>>> good to add a note to that effect, so that people do not expect >>>> virtualization support to be working when booting with ACPI. >>> yes, it is worth mentioning! >>> >>>>> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org> >>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >>>>> --- >>>>> arch/arm64/include/asm/acpi.h | 2 - >>>>> arch/arm64/kernel/acpi.c | 23 +++++++ >>>>> arch/arm64/kernel/irq.c | 5 ++ >>>>> drivers/irqchip/irq-gic.c | 114 ++++++++++++++++++++++++++++++++++ >>>>> include/linux/irqchip/arm-gic-acpi.h | 33 ++++++++++ >>>>> 5 files changed, 175 insertions(+), 2 deletions(-) >>>>> create mode 100644 include/linux/irqchip/arm-gic-acpi.h >>>>> >>>>> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h >>>>> index a867467..5d2ab63 100644 >>>>> --- a/arch/arm64/include/asm/acpi.h >>>>> +++ b/arch/arm64/include/asm/acpi.h >>>>> @@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void); >>>>> extern int (*acpi_suspend_lowlevel)(void); >>>>> #define acpi_wakeup_address 0 >>>>> >>>>> -#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535 >>>>> - >>>>> #else >>>>> >>>>> static inline bool acpi_psci_present(void) { return false; } >>>>> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c >>>>> index 354b912..b3b82b0 100644 >>>>> --- a/arch/arm64/kernel/acpi.c >>>>> +++ b/arch/arm64/kernel/acpi.c >>>>> @@ -23,6 +23,7 @@ >>>>> #include <linux/irqdomain.h> >>>>> #include <linux/bootmem.h> >>>>> #include <linux/smp.h> >>>>> +#include <linux/irqchip/arm-gic-acpi.h> >>>>> >>>>> #include <asm/cputype.h> >>>>> #include <asm/cpu_ops.h> >>>>> @@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void) >>>>> pr_err("Can't find FADT or error happened during parsing FADT\n"); >>>>> } >>>>> >>>>> +void __init acpi_gic_init(void) >>>>> +{ >>>>> + struct acpi_table_header *table; >>>>> + acpi_status status; >>>>> + acpi_size tbl_size; >>>>> + int err; >>>>> + >>>>> + status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, &table, &tbl_size); >>>>> + if (ACPI_FAILURE(status)) { >>>>> + const char *msg = acpi_format_exception(status); >>>>> + >>>>> + pr_err("Failed to get MADT table, %s\n", msg); >>>>> + return; >>>>> + } >>>>> + >>>>> + err = gic_v2_acpi_init(table); >>>>> + if (err) >>>>> + pr_err("Failed to initialize GIC IRQ controller"); >>>> What will happen when you get to implement GICv3 support? Another entry >>>> like this? Why isn't this entirely contained in the GIC driver? Do I >>>> sound like a stuck record? >>> There will be another call to GICv3 init: >>> [...] >>> err = gic_v3_acpi_init(table); >>> if (err) >>> err = gic_v2_acpi_init(table); >>> if (err) >>> pr_err("Failed to initialize GIC IRQ controller"); >>> [...] >>> This is the main reason I put common code here. >>> >>>>> + >>>>> + early_acpi_os_unmap_memory((char *)table, tbl_size); >>>>> +} >>>>> + >>>>> /* >>>>> * acpi_suspend_lowlevel() - save kernel state and suspend. >>>>> * >>>>> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c >>>>> index 0f08dfd..c074d60 100644 >>>>> --- a/arch/arm64/kernel/irq.c >>>>> +++ b/arch/arm64/kernel/irq.c >>>>> @@ -28,6 +28,7 @@ >>>>> #include <linux/irqchip.h> >>>>> #include <linux/seq_file.h> >>>>> #include <linux/ratelimit.h> >>>>> +#include <linux/irqchip/arm-gic-acpi.h> >>>>> >>>>> unsigned long irq_err_count; >>>>> >>>>> @@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs *)) >>>>> void __init init_IRQ(void) >>>>> { >>>>> irqchip_init(); >>>>> + >>>>> + if (!handle_arch_irq) >>>>> + acpi_gic_init(); >>>>> + >>>> Why isn't this called from irqchip_init? It would seem like the logical >>>> spot to probe an interrupt controller. >>> irqchip.c is OF dependent, I want to decouple these from the very >>> beginning. >> No. irqchip.c is not OF dependent, it is just that DT is the only thing >> we support so far. I don't think duplicating the kernel infrastructure >> "because we're different" is the right way. >> >> There is no reason for your probing structure to be artificially >> different (you're parsing the same information, at the same time). Just >> put in place a similar probing mechanism, and this will look a lot better. >> >>>>> if (!handle_arch_irq) >>>>> panic("No interrupt controller found."); >>>>> } >>>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >>>>> index 4b959e6..85cbf43 100644 >>>>> --- a/drivers/irqchip/irq-gic.c >>>>> +++ b/drivers/irqchip/irq-gic.c >>>>> @@ -33,12 +33,14 @@ >>>>> #include <linux/of.h> >>>>> #include <linux/of_address.h> >>>>> #include <linux/of_irq.h> >>>>> +#include <linux/acpi.h> >>>>> #include <linux/irqdomain.h> >>>>> #include <linux/interrupt.h> >>>>> #include <linux/percpu.h> >>>>> #include <linux/slab.h> >>>>> #include <linux/irqchip/chained_irq.h> >>>>> #include <linux/irqchip/arm-gic.h> >>>>> +#include <linux/irqchip/arm-gic-acpi.h> >>>>> >>>>> #include <asm/cputype.h> >>>>> #include <asm/irq.h> >>>>> @@ -1029,3 +1031,115 @@ IRQCHIP_DECLARE(msm_8660_qgic, "qcom,msm-8660-qgic", gic_of_init); >>>>> IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init); >>>>> >>>>> #endif >>>>> + >>>>> +#ifdef CONFIG_ACPI >>>>> +static u64 dist_phy_base, cpu_phy_base = ULONG_MAX; >>>> Please use phys_addr_t for physical addresses. The use of ULONG_MAX >>>> looks dodgy. Please have a proper symbol to flag the fact that it hasn't >>>> been assigned yet. >>> Sure, will do. >>> >>>>> + >>>>> +static int __init >>>>> +gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header, >>>>> + const unsigned long end) >>>>> +{ >>>>> + struct acpi_madt_generic_interrupt *processor; >>>>> + u64 gic_cpu_base; >>>> phys_addr_t >>>> >>>>> + processor = (struct acpi_madt_generic_interrupt *)header; >>>>> + >>>>> + if (BAD_MADT_ENTRY(processor, end)) >>>>> + return -EINVAL; >>>>> + >>>>> + gic_cpu_base = processor->base_address; >>>>> + if (!gic_cpu_base) >>>>> + return -EFAULT; >>>> Is zero an invalid address? >>> Yeah, good point. >>>>> + >>>>> + /* >>>>> + * There is no support for non-banked GICv1/2 register in ACPI spec. >>>>> + * All CPU interface addresses have to be the same. >>>>> + */ >>>>> + if (cpu_phy_base != ULONG_MAX && gic_cpu_base != cpu_phy_base) >>>>> + return -EFAULT; >>>>> + >>>>> + cpu_phy_base = gic_cpu_base; >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int __init >>>>> +gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header, >>>>> + const unsigned long end) >>>>> +{ >>>>> + struct acpi_madt_generic_distributor *dist; >>>>> + >>>>> + dist = (struct acpi_madt_generic_distributor *)header; >>>>> + >>>>> + if (BAD_MADT_ENTRY(dist, end)) >>>>> + return -EINVAL; >>>>> + >>>>> + dist_phy_base = dist->base_address; >>>>> + if (!dist_phy_base) >>>>> + return -EFAULT; >>>> Same question about zero. >>>> >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +int __init >>>>> +gic_v2_acpi_init(struct acpi_table_header *table) >>>>> +{ >>>>> + void __iomem *cpu_base, *dist_base; >>>>> + int count; >>>>> + >>>>> + /* Collect CPU base addresses */ >>>>> + count = acpi_parse_entries(sizeof(struct acpi_table_madt), >>>>> + gic_acpi_parse_madt_cpu, table, >>>>> + ACPI_MADT_TYPE_GENERIC_INTERRUPT, >>>>> + ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES); >>>>> + if (count < 0) { >>>>> + pr_err("Error during GICC entries parsing\n"); >>>>> + return -EFAULT; >>>>> + } else if (!count) { >>>>> + /* No GICC entries provided, use address from MADT header */ >>>>> + struct acpi_table_madt *madt = (struct acpi_table_madt *)table; >>>>> + >>>>> + if (!madt->address) >>>>> + return -EFAULT; >>>>> + >>>>> + cpu_phy_base = (u64)madt->address; >>>>> + } >>>>> + >>>>> + /* >>>>> + * Find distributor base address. We expect one distributor entry since >>>>> + * ACPI 5.1 spec neither support multi-GIC instances nor GIC cascade. >>>>> + */ >>>>> + count = acpi_parse_entries(sizeof(struct acpi_table_madt), >>>>> + gic_acpi_parse_madt_distributor, table, >>>>> + ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, >>>>> + ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES); >>>>> + if (count <= 0) { >>>>> + pr_err("Error during GICD entries parsing\n"); >>>>> + return -EFAULT; >>>>> + } else if (count > 1) { >>>>> + pr_err("More than one GICD entry detected\n"); >>>>> + return -EINVAL; >>>>> + } >>>>> + >>>>> + cpu_base = ioremap(cpu_phy_base, ACPI_GIC_CPU_IF_MEM_SIZE); >>>>> + if (!cpu_base) { >>>>> + pr_err("Unable to map GICC registers\n"); >>>>> + return -ENOMEM; >>>>> + } >>>>> + >>>>> + dist_base = ioremap(dist_phy_base, ACPI_GIC_DIST_MEM_SIZE); >>>>> + if (!dist_base) { >>>>> + pr_err("Unable to map GICD registers\n"); >>>>> + iounmap(cpu_base); >>>>> + 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); >>>>> + return 0; >>>>> +} >>>>> +#endif >>>>> diff --git a/include/linux/irqchip/arm-gic-acpi.h b/include/linux/irqchip/arm-gic-acpi.h >>>>> new file mode 100644 >>>>> index 0000000..ce2ae1a8 >>>>> --- /dev/null >>>>> +++ b/include/linux/irqchip/arm-gic-acpi.h >>>>> @@ -0,0 +1,33 @@ >>>>> +/* >>>>> + * Copyright (C) 2014, Linaro Ltd. >>>>> + * Author: Tomasz Nowicki <tomasz.nowicki@linaro.org> >>>>> + * >>>>> + * 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 >>>>> + * published by the Free Software Foundation. >>>>> + */ >>>>> + >>>>> +#ifndef ARM_GIC_ACPI_H_ >>>>> +#define ARM_GIC_ACPI_H_ >>>>> + >>>>> +#include <linux/acpi.h> >>>> Do we need linux/acpi.h here? You could have a separate forward >>>> declaration of struct acpi_table_header, specially in the light of my >>>> last remark below. >>> Indeed, we can do forward declaration instead of #include >>> <linux/acpi.h>. Thanks! >>> >>>>> + >>>>> +#ifdef CONFIG_ACPI >>>>> +#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535 >>>> With GICv2? I doubt it. >>> I will create macro for each GIC driver: >>> #define ACPI_MAX_GICV2_CPU_INTERFACE_ENTRIES 8 >>> #define ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES 65535 >> Where do you get this value (ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES) from? > > This value is for max processors entries in MADT, and we will use it to scan MADT > for SMP/GIC Init, I just make it big enough for GICv3/4. since ACPI core will stop > scan MADT if the real numbers of processors entries are reached no matter > how big ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES is, I think we can just > define a number big enough then it will work (x86 and ia64 did the same thing). > This is the exact reason I kept mentioning *not to link it with GIC architecture* in my previous reviews. It's just *max possible entries in MADT*. Regards, Sudeep
On Tue, Sep 02, 2014 at 12:48:37PM +0100, Tomasz Nowicki wrote: > On 01.09.2014 19:35, Marc Zyngier wrote: > > On 01/09/14 15:57, Hanjun Guo wrote: > >> From: Tomasz Nowicki <tomasz.nowicki@linaro.org> > >> > >> ACPI kernel uses MADT table for proper GIC initialization. It needs to > >> parse GIC related subtables, collect CPU interface and distributor > >> addresses and call driver initialization function (which is hardware > >> abstraction agnostic). In a similar way, FDT initialize GICv1/2. > >> > >> NOTE: This commit allow to initialize GICv1/2 only. > > > > I cannot help but notice that there is no support for KVM here. It'd be > > good to add a note to that effect, so that people do not expect > > virtualization support to be working when booting with ACPI. > > yes, it is worth mentioning! It's also worth mentioning if there are any plans to fix this ;).
On 02.09.2014 15:02, Marc Zyngier wrote: > On 02/09/14 12:48, Tomasz Nowicki wrote: >> On 01.09.2014 19:35, Marc Zyngier wrote: >>> On 01/09/14 15:57, Hanjun Guo wrote: >>>> From: Tomasz Nowicki <tomasz.nowicki@linaro.org> >>>> >>>> ACPI kernel uses MADT table for proper GIC initialization. It needs to >>>> parse GIC related subtables, collect CPU interface and distributor >>>> addresses and call driver initialization function (which is hardware >>>> abstraction agnostic). In a similar way, FDT initialize GICv1/2. >>>> >>>> NOTE: This commit allow to initialize GICv1/2 only. >>> >>> I cannot help but notice that there is no support for KVM here. It'd be >>> good to add a note to that effect, so that people do not expect >>> virtualization support to be working when booting with ACPI. >> >> yes, it is worth mentioning! >> >>> >>>> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org> >>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >>>> --- >>>> arch/arm64/include/asm/acpi.h | 2 - >>>> arch/arm64/kernel/acpi.c | 23 +++++++ >>>> arch/arm64/kernel/irq.c | 5 ++ >>>> drivers/irqchip/irq-gic.c | 114 ++++++++++++++++++++++++++++++++++ >>>> include/linux/irqchip/arm-gic-acpi.h | 33 ++++++++++ >>>> 5 files changed, 175 insertions(+), 2 deletions(-) >>>> create mode 100644 include/linux/irqchip/arm-gic-acpi.h >>>> >>>> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h >>>> index a867467..5d2ab63 100644 >>>> --- a/arch/arm64/include/asm/acpi.h >>>> +++ b/arch/arm64/include/asm/acpi.h >>>> @@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void); >>>> extern int (*acpi_suspend_lowlevel)(void); >>>> #define acpi_wakeup_address 0 >>>> >>>> -#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535 >>>> - >>>> #else >>>> >>>> static inline bool acpi_psci_present(void) { return false; } >>>> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c >>>> index 354b912..b3b82b0 100644 >>>> --- a/arch/arm64/kernel/acpi.c >>>> +++ b/arch/arm64/kernel/acpi.c >>>> @@ -23,6 +23,7 @@ >>>> #include <linux/irqdomain.h> >>>> #include <linux/bootmem.h> >>>> #include <linux/smp.h> >>>> +#include <linux/irqchip/arm-gic-acpi.h> >>>> >>>> #include <asm/cputype.h> >>>> #include <asm/cpu_ops.h> >>>> @@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void) >>>> pr_err("Can't find FADT or error happened during parsing FADT\n"); >>>> } >>>> >>>> +void __init acpi_gic_init(void) >>>> +{ >>>> + struct acpi_table_header *table; >>>> + acpi_status status; >>>> + acpi_size tbl_size; >>>> + int err; >>>> + >>>> + status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, &table, &tbl_size); >>>> + if (ACPI_FAILURE(status)) { >>>> + const char *msg = acpi_format_exception(status); >>>> + >>>> + pr_err("Failed to get MADT table, %s\n", msg); >>>> + return; >>>> + } >>>> + >>>> + err = gic_v2_acpi_init(table); >>>> + if (err) >>>> + pr_err("Failed to initialize GIC IRQ controller"); >>> >>> What will happen when you get to implement GICv3 support? Another entry >>> like this? Why isn't this entirely contained in the GIC driver? Do I >>> sound like a stuck record? >> >> There will be another call to GICv3 init: >> [...] >> err = gic_v3_acpi_init(table); >> if (err) >> err = gic_v2_acpi_init(table); >> if (err) >> pr_err("Failed to initialize GIC IRQ controller"); >> [...] >> This is the main reason I put common code here. >> >>> >>>> + >>>> + early_acpi_os_unmap_memory((char *)table, tbl_size); >>>> +} >>>> + >>>> /* >>>> * acpi_suspend_lowlevel() - save kernel state and suspend. >>>> * >>>> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c >>>> index 0f08dfd..c074d60 100644 >>>> --- a/arch/arm64/kernel/irq.c >>>> +++ b/arch/arm64/kernel/irq.c >>>> @@ -28,6 +28,7 @@ >>>> #include <linux/irqchip.h> >>>> #include <linux/seq_file.h> >>>> #include <linux/ratelimit.h> >>>> +#include <linux/irqchip/arm-gic-acpi.h> >>>> >>>> unsigned long irq_err_count; >>>> >>>> @@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs *)) >>>> void __init init_IRQ(void) >>>> { >>>> irqchip_init(); >>>> + >>>> + if (!handle_arch_irq) >>>> + acpi_gic_init(); >>>> + >>> >>> Why isn't this called from irqchip_init? It would seem like the logical >>> spot to probe an interrupt controller. >> >> irqchip.c is OF dependent, I want to decouple these from the very >> beginning. > > No. irqchip.c is not OF dependent, it is just that DT is the only thing > we support so far. I don't think duplicating the kernel infrastructure > "because we're different" is the right way. > > There is no reason for your probing structure to be artificially > different (you're parsing the same information, at the same time). Just > put in place a similar probing mechanism, and this will look a lot better. > >>> >>>> if (!handle_arch_irq) >>>> panic("No interrupt controller found."); >>>> } >>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >>>> index 4b959e6..85cbf43 100644 >>>> --- a/drivers/irqchip/irq-gic.c >>>> +++ b/drivers/irqchip/irq-gic.c >>>> @@ -33,12 +33,14 @@ >>>> #include <linux/of.h> >>>> #include <linux/of_address.h> >>>> #include <linux/of_irq.h> >>>> +#include <linux/acpi.h> >>>> #include <linux/irqdomain.h> >>>> #include <linux/interrupt.h> >>>> #include <linux/percpu.h> >>>> #include <linux/slab.h> >>>> #include <linux/irqchip/chained_irq.h> >>>> #include <linux/irqchip/arm-gic.h> >>>> +#include <linux/irqchip/arm-gic-acpi.h> >>>> >>>> #include <asm/cputype.h> >>>> #include <asm/irq.h> >>>> @@ -1029,3 +1031,115 @@ IRQCHIP_DECLARE(msm_8660_qgic, "qcom,msm-8660-qgic", gic_of_init); >>>> IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init); >>>> >>>> #endif >>>> + >>>> +#ifdef CONFIG_ACPI >>>> +static u64 dist_phy_base, cpu_phy_base = ULONG_MAX; >>> >>> Please use phys_addr_t for physical addresses. The use of ULONG_MAX >>> looks dodgy. Please have a proper symbol to flag the fact that it hasn't >>> been assigned yet. >> Sure, will do. >> >>> >>>> + >>>> +static int __init >>>> +gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header, >>>> + const unsigned long end) >>>> +{ >>>> + struct acpi_madt_generic_interrupt *processor; >>>> + u64 gic_cpu_base; >>> >>> phys_addr_t >>> >>>> + processor = (struct acpi_madt_generic_interrupt *)header; >>>> + >>>> + if (BAD_MADT_ENTRY(processor, end)) >>>> + return -EINVAL; >>>> + >>>> + gic_cpu_base = processor->base_address; >>>> + if (!gic_cpu_base) >>>> + return -EFAULT; >>> >>> Is zero an invalid address? >> Yeah, good point. >>> >>>> + >>>> + /* >>>> + * There is no support for non-banked GICv1/2 register in ACPI spec. >>>> + * All CPU interface addresses have to be the same. >>>> + */ >>>> + if (cpu_phy_base != ULONG_MAX && gic_cpu_base != cpu_phy_base) >>>> + return -EFAULT; >>>> + >>>> + cpu_phy_base = gic_cpu_base; >>>> + return 0; >>>> +} >>>> + >>>> +static int __init >>>> +gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header, >>>> + const unsigned long end) >>>> +{ >>>> + struct acpi_madt_generic_distributor *dist; >>>> + >>>> + dist = (struct acpi_madt_generic_distributor *)header; >>>> + >>>> + if (BAD_MADT_ENTRY(dist, end)) >>>> + return -EINVAL; >>>> + >>>> + dist_phy_base = dist->base_address; >>>> + if (!dist_phy_base) >>>> + return -EFAULT; >>> >>> Same question about zero. >>> >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +int __init >>>> +gic_v2_acpi_init(struct acpi_table_header *table) >>>> +{ >>>> + void __iomem *cpu_base, *dist_base; >>>> + int count; >>>> + >>>> + /* Collect CPU base addresses */ >>>> + count = acpi_parse_entries(sizeof(struct acpi_table_madt), >>>> + gic_acpi_parse_madt_cpu, table, >>>> + ACPI_MADT_TYPE_GENERIC_INTERRUPT, >>>> + ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES); >>>> + if (count < 0) { >>>> + pr_err("Error during GICC entries parsing\n"); >>>> + return -EFAULT; >>>> + } else if (!count) { >>>> + /* No GICC entries provided, use address from MADT header */ >>>> + struct acpi_table_madt *madt = (struct acpi_table_madt *)table; >>>> + >>>> + if (!madt->address) >>>> + return -EFAULT; >>>> + >>>> + cpu_phy_base = (u64)madt->address; >>>> + } >>>> + >>>> + /* >>>> + * Find distributor base address. We expect one distributor entry since >>>> + * ACPI 5.1 spec neither support multi-GIC instances nor GIC cascade. >>>> + */ >>>> + count = acpi_parse_entries(sizeof(struct acpi_table_madt), >>>> + gic_acpi_parse_madt_distributor, table, >>>> + ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, >>>> + ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES); >>>> + if (count <= 0) { >>>> + pr_err("Error during GICD entries parsing\n"); >>>> + return -EFAULT; >>>> + } else if (count > 1) { >>>> + pr_err("More than one GICD entry detected\n"); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + cpu_base = ioremap(cpu_phy_base, ACPI_GIC_CPU_IF_MEM_SIZE); >>>> + if (!cpu_base) { >>>> + pr_err("Unable to map GICC registers\n"); >>>> + return -ENOMEM; >>>> + } >>>> + >>>> + dist_base = ioremap(dist_phy_base, ACPI_GIC_DIST_MEM_SIZE); >>>> + if (!dist_base) { >>>> + pr_err("Unable to map GICD registers\n"); >>>> + iounmap(cpu_base); >>>> + 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); >>>> + return 0; >>>> +} >>>> +#endif >>>> diff --git a/include/linux/irqchip/arm-gic-acpi.h b/include/linux/irqchip/arm-gic-acpi.h >>>> new file mode 100644 >>>> index 0000000..ce2ae1a8 >>>> --- /dev/null >>>> +++ b/include/linux/irqchip/arm-gic-acpi.h >>>> @@ -0,0 +1,33 @@ >>>> +/* >>>> + * Copyright (C) 2014, Linaro Ltd. >>>> + * Author: Tomasz Nowicki <tomasz.nowicki@linaro.org> >>>> + * >>>> + * 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 >>>> + * published by the Free Software Foundation. >>>> + */ >>>> + >>>> +#ifndef ARM_GIC_ACPI_H_ >>>> +#define ARM_GIC_ACPI_H_ >>>> + >>>> +#include <linux/acpi.h> >>> >>> Do we need linux/acpi.h here? You could have a separate forward >>> declaration of struct acpi_table_header, specially in the light of my >>> last remark below. >> Indeed, we can do forward declaration instead of #include >> <linux/acpi.h>. Thanks! >> >>> >>>> + >>>> +#ifdef CONFIG_ACPI >>>> +#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535 >>> >>> With GICv2? I doubt it. >> I will create macro for each GIC driver: >> #define ACPI_MAX_GICV2_CPU_INTERFACE_ENTRIES 8 >> #define ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES 65535 > > Where do you get this value (ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES) from? > >>> >>>> +#define ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES 1 >>>> + >>>> +/* >>>> + * Hard code here, we can not get memory size from MADT (but FDT does), >>>> + * Actually no need to do that, because this size can be inferred >>>> + * from GIC spec. >>>> + */ >>>> +#define ACPI_GIC_DIST_MEM_SIZE (SZ_64K) >>> >>> I don't know which version of the spec you're looking at, but my version >>> of the GICv2 spec has a 4kB distributor. Also, it would be good to make >>> obvious which GIC version this define is about. >> OK >> >>> >>>> +#define ACPI_GIC_CPU_IF_MEM_SIZE (SZ_8K) >>>> + >>>> +void acpi_gic_init(void); >>>> +int gic_v2_acpi_init(struct acpi_table_header *table); >>>> +#else >>>> +static inline void acpi_gic_init(void) { } >>>> +#endif >>>> + >>>> +#endif /* ARM_GIC_ACPI_H_ */ >>>> >>> >>> In the end, why do we need a separate file for this? I cannot see >>> anything that prevents it from being merged with arm-gic.h. >>> >>> Thanks, >>> >>> M. >>> >> Having only GICv2, it would work. Considering we would do the same for >> GICv3 (arm-gic-v3.h) there will be register name conflicts for both >> headers inclusion: >> >> [...] >> #include <linux/irqchip/arm-gic.h> >> #include <linux/irqchip/arm-gic-v3.h> >> [...] >> err = gic_v3_acpi_init(table); >> if (err) >> err = gic_v2_acpi_init(table); >> if (err) >> pr_err("Failed to initialize GIC IRQ controller"); >> [...] >> So instead of changing register names prefix, I choose new header will >> be less painfully. > > Yes, and this is exactly why I pushed back on that last time. I'll > continue saying that interrupt controllers should be self-probing, with > ACPI as they are with DT. > > Even with the restrictions of ACPI and SBSA, we end-up with at least 2 > main families of interrupt controllers (GICv2 and GICv3), both with a > number of "interesting" variations (GICv2m and GICv4, to only mention > those I'm directly involved with). > > I can safely predict that the above will become a tangled mess within 18 > months, and the idea of littering the arch code with a bunch of > hardcoded "if (blah())" doesn't fill me with joy and confidence. > > In summary: we have the infrastructure already, just use it. We had that discussion but I see we still don't have consensus here. It would be good to know our direction before we prepare next patch version. Arnd any comments on this from you side? Tomasz
On 02/09/14 16:45, Hanjun Guo wrote: > On 2014?09?02? 21:02, Marc Zyngier wrote: >> On 02/09/14 12:48, Tomasz Nowicki wrote: >>> On 01.09.2014 19:35, Marc Zyngier wrote: >>>> On 01/09/14 15:57, Hanjun Guo wrote: >>>>> From: Tomasz Nowicki <tomasz.nowicki@linaro.org> >>>>> >>>>> ACPI kernel uses MADT table for proper GIC initialization. It needs to >>>>> parse GIC related subtables, collect CPU interface and distributor >>>>> addresses and call driver initialization function (which is hardware >>>>> abstraction agnostic). In a similar way, FDT initialize GICv1/2. >>>>> >>>>> NOTE: This commit allow to initialize GICv1/2 only. >>>> I cannot help but notice that there is no support for KVM here. It'd be >>>> good to add a note to that effect, so that people do not expect >>>> virtualization support to be working when booting with ACPI. >>> yes, it is worth mentioning! >>> >>>>> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org> >>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >>>>> --- >>>>> arch/arm64/include/asm/acpi.h | 2 - >>>>> arch/arm64/kernel/acpi.c | 23 +++++++ >>>>> arch/arm64/kernel/irq.c | 5 ++ >>>>> drivers/irqchip/irq-gic.c | 114 ++++++++++++++++++++++++++++++++++ >>>>> include/linux/irqchip/arm-gic-acpi.h | 33 ++++++++++ >>>>> 5 files changed, 175 insertions(+), 2 deletions(-) >>>>> create mode 100644 include/linux/irqchip/arm-gic-acpi.h >>>>> >>>>> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h >>>>> index a867467..5d2ab63 100644 >>>>> --- a/arch/arm64/include/asm/acpi.h >>>>> +++ b/arch/arm64/include/asm/acpi.h >>>>> @@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void); >>>>> extern int (*acpi_suspend_lowlevel)(void); >>>>> #define acpi_wakeup_address 0 >>>>> >>>>> -#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535 >>>>> - >>>>> #else >>>>> >>>>> static inline bool acpi_psci_present(void) { return false; } >>>>> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c >>>>> index 354b912..b3b82b0 100644 >>>>> --- a/arch/arm64/kernel/acpi.c >>>>> +++ b/arch/arm64/kernel/acpi.c >>>>> @@ -23,6 +23,7 @@ >>>>> #include <linux/irqdomain.h> >>>>> #include <linux/bootmem.h> >>>>> #include <linux/smp.h> >>>>> +#include <linux/irqchip/arm-gic-acpi.h> >>>>> >>>>> #include <asm/cputype.h> >>>>> #include <asm/cpu_ops.h> >>>>> @@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void) >>>>> pr_err("Can't find FADT or error happened during parsing FADT\n"); >>>>> } >>>>> >>>>> +void __init acpi_gic_init(void) >>>>> +{ >>>>> + struct acpi_table_header *table; >>>>> + acpi_status status; >>>>> + acpi_size tbl_size; >>>>> + int err; >>>>> + >>>>> + status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, &table, &tbl_size); >>>>> + if (ACPI_FAILURE(status)) { >>>>> + const char *msg = acpi_format_exception(status); >>>>> + >>>>> + pr_err("Failed to get MADT table, %s\n", msg); >>>>> + return; >>>>> + } >>>>> + >>>>> + err = gic_v2_acpi_init(table); >>>>> + if (err) >>>>> + pr_err("Failed to initialize GIC IRQ controller"); >>>> What will happen when you get to implement GICv3 support? Another entry >>>> like this? Why isn't this entirely contained in the GIC driver? Do I >>>> sound like a stuck record? >>> There will be another call to GICv3 init: >>> [...] >>> err = gic_v3_acpi_init(table); >>> if (err) >>> err = gic_v2_acpi_init(table); >>> if (err) >>> pr_err("Failed to initialize GIC IRQ controller"); >>> [...] >>> This is the main reason I put common code here. >>> >>>>> + >>>>> + early_acpi_os_unmap_memory((char *)table, tbl_size); >>>>> +} >>>>> + >>>>> /* >>>>> * acpi_suspend_lowlevel() - save kernel state and suspend. >>>>> * >>>>> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c >>>>> index 0f08dfd..c074d60 100644 >>>>> --- a/arch/arm64/kernel/irq.c >>>>> +++ b/arch/arm64/kernel/irq.c >>>>> @@ -28,6 +28,7 @@ >>>>> #include <linux/irqchip.h> >>>>> #include <linux/seq_file.h> >>>>> #include <linux/ratelimit.h> >>>>> +#include <linux/irqchip/arm-gic-acpi.h> >>>>> >>>>> unsigned long irq_err_count; >>>>> >>>>> @@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs *)) >>>>> void __init init_IRQ(void) >>>>> { >>>>> irqchip_init(); >>>>> + >>>>> + if (!handle_arch_irq) >>>>> + acpi_gic_init(); >>>>> + >>>> Why isn't this called from irqchip_init? It would seem like the logical >>>> spot to probe an interrupt controller. >>> irqchip.c is OF dependent, I want to decouple these from the very >>> beginning. >> No. irqchip.c is not OF dependent, it is just that DT is the only thing >> we support so far. I don't think duplicating the kernel infrastructure >> "because we're different" is the right way. >> >> There is no reason for your probing structure to be artificially >> different (you're parsing the same information, at the same time). Just >> put in place a similar probing mechanism, and this will look a lot better. >> >>>>> if (!handle_arch_irq) >>>>> panic("No interrupt controller found."); >>>>> } >>>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c >>>>> index 4b959e6..85cbf43 100644 >>>>> --- a/drivers/irqchip/irq-gic.c >>>>> +++ b/drivers/irqchip/irq-gic.c >>>>> @@ -33,12 +33,14 @@ >>>>> #include <linux/of.h> >>>>> #include <linux/of_address.h> >>>>> #include <linux/of_irq.h> >>>>> +#include <linux/acpi.h> >>>>> #include <linux/irqdomain.h> >>>>> #include <linux/interrupt.h> >>>>> #include <linux/percpu.h> >>>>> #include <linux/slab.h> >>>>> #include <linux/irqchip/chained_irq.h> >>>>> #include <linux/irqchip/arm-gic.h> >>>>> +#include <linux/irqchip/arm-gic-acpi.h> >>>>> >>>>> #include <asm/cputype.h> >>>>> #include <asm/irq.h> >>>>> @@ -1029,3 +1031,115 @@ IRQCHIP_DECLARE(msm_8660_qgic, "qcom,msm-8660-qgic", gic_of_init); >>>>> IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init); >>>>> >>>>> #endif >>>>> + >>>>> +#ifdef CONFIG_ACPI >>>>> +static u64 dist_phy_base, cpu_phy_base = ULONG_MAX; >>>> Please use phys_addr_t for physical addresses. The use of ULONG_MAX >>>> looks dodgy. Please have a proper symbol to flag the fact that it hasn't >>>> been assigned yet. >>> Sure, will do. >>> >>>>> + >>>>> +static int __init >>>>> +gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header, >>>>> + const unsigned long end) >>>>> +{ >>>>> + struct acpi_madt_generic_interrupt *processor; >>>>> + u64 gic_cpu_base; >>>> phys_addr_t >>>> >>>>> + processor = (struct acpi_madt_generic_interrupt *)header; >>>>> + >>>>> + if (BAD_MADT_ENTRY(processor, end)) >>>>> + return -EINVAL; >>>>> + >>>>> + gic_cpu_base = processor->base_address; >>>>> + if (!gic_cpu_base) >>>>> + return -EFAULT; >>>> Is zero an invalid address? >>> Yeah, good point. >>>>> + >>>>> + /* >>>>> + * There is no support for non-banked GICv1/2 register in ACPI spec. >>>>> + * All CPU interface addresses have to be the same. >>>>> + */ >>>>> + if (cpu_phy_base != ULONG_MAX && gic_cpu_base != cpu_phy_base) >>>>> + return -EFAULT; >>>>> + >>>>> + cpu_phy_base = gic_cpu_base; >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int __init >>>>> +gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header, >>>>> + const unsigned long end) >>>>> +{ >>>>> + struct acpi_madt_generic_distributor *dist; >>>>> + >>>>> + dist = (struct acpi_madt_generic_distributor *)header; >>>>> + >>>>> + if (BAD_MADT_ENTRY(dist, end)) >>>>> + return -EINVAL; >>>>> + >>>>> + dist_phy_base = dist->base_address; >>>>> + if (!dist_phy_base) >>>>> + return -EFAULT; >>>> Same question about zero. >>>> >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +int __init >>>>> +gic_v2_acpi_init(struct acpi_table_header *table) >>>>> +{ >>>>> + void __iomem *cpu_base, *dist_base; >>>>> + int count; >>>>> + >>>>> + /* Collect CPU base addresses */ >>>>> + count = acpi_parse_entries(sizeof(struct acpi_table_madt), >>>>> + gic_acpi_parse_madt_cpu, table, >>>>> + ACPI_MADT_TYPE_GENERIC_INTERRUPT, >>>>> + ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES); >>>>> + if (count < 0) { >>>>> + pr_err("Error during GICC entries parsing\n"); >>>>> + return -EFAULT; >>>>> + } else if (!count) { >>>>> + /* No GICC entries provided, use address from MADT header */ >>>>> + struct acpi_table_madt *madt = (struct acpi_table_madt *)table; >>>>> + >>>>> + if (!madt->address) >>>>> + return -EFAULT; >>>>> + >>>>> + cpu_phy_base = (u64)madt->address; >>>>> + } >>>>> + >>>>> + /* >>>>> + * Find distributor base address. We expect one distributor entry since >>>>> + * ACPI 5.1 spec neither support multi-GIC instances nor GIC cascade. >>>>> + */ >>>>> + count = acpi_parse_entries(sizeof(struct acpi_table_madt), >>>>> + gic_acpi_parse_madt_distributor, table, >>>>> + ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, >>>>> + ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES); >>>>> + if (count <= 0) { >>>>> + pr_err("Error during GICD entries parsing\n"); >>>>> + return -EFAULT; >>>>> + } else if (count > 1) { >>>>> + pr_err("More than one GICD entry detected\n"); >>>>> + return -EINVAL; >>>>> + } >>>>> + >>>>> + cpu_base = ioremap(cpu_phy_base, ACPI_GIC_CPU_IF_MEM_SIZE); >>>>> + if (!cpu_base) { >>>>> + pr_err("Unable to map GICC registers\n"); >>>>> + return -ENOMEM; >>>>> + } >>>>> + >>>>> + dist_base = ioremap(dist_phy_base, ACPI_GIC_DIST_MEM_SIZE); >>>>> + if (!dist_base) { >>>>> + pr_err("Unable to map GICD registers\n"); >>>>> + iounmap(cpu_base); >>>>> + 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); >>>>> + return 0; >>>>> +} >>>>> +#endif >>>>> diff --git a/include/linux/irqchip/arm-gic-acpi.h b/include/linux/irqchip/arm-gic-acpi.h >>>>> new file mode 100644 >>>>> index 0000000..ce2ae1a8 >>>>> --- /dev/null >>>>> +++ b/include/linux/irqchip/arm-gic-acpi.h >>>>> @@ -0,0 +1,33 @@ >>>>> +/* >>>>> + * Copyright (C) 2014, Linaro Ltd. >>>>> + * Author: Tomasz Nowicki <tomasz.nowicki@linaro.org> >>>>> + * >>>>> + * 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 >>>>> + * published by the Free Software Foundation. >>>>> + */ >>>>> + >>>>> +#ifndef ARM_GIC_ACPI_H_ >>>>> +#define ARM_GIC_ACPI_H_ >>>>> + >>>>> +#include <linux/acpi.h> >>>> Do we need linux/acpi.h here? You could have a separate forward >>>> declaration of struct acpi_table_header, specially in the light of my >>>> last remark below. >>> Indeed, we can do forward declaration instead of #include >>> <linux/acpi.h>. Thanks! >>> >>>>> + >>>>> +#ifdef CONFIG_ACPI >>>>> +#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535 >>>> With GICv2? I doubt it. >>> I will create macro for each GIC driver: >>> #define ACPI_MAX_GICV2_CPU_INTERFACE_ENTRIES 8 >>> #define ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES 65535 >> Where do you get this value (ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES) from? > > This value is for max processors entries in MADT, and we will use it to scan MADT > for SMP/GIC Init, I just make it big enough for GICv3/4. since ACPI core will stop > scan MADT if the real numbers of processors entries are reached no matter > how big ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES is, I think we can just > define a number big enough then it will work (x86 and ia64 did the same thing). Also, with GICv3++, there is no such thing as a memory-mapped CPU interface anymore. What you get is a bunch of redistributors (one per CPU). I assume what you have here actually describe the redistributors, and its name should reflect that. Thanks, M.
>>>>>> + >>>>>> +#ifdef CONFIG_ACPI >>>>>> +#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535 >>>>> With GICv2? I doubt it. >>>> I will create macro for each GIC driver: >>>> #define ACPI_MAX_GICV2_CPU_INTERFACE_ENTRIES 8 >>>> #define ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES 65535 >>> Where do you get this value (ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES) from? >> This value is for max processors entries in MADT, and we will use it to scan MADT >> for SMP/GIC Init, I just make it big enough for GICv3/4. since ACPI core will stop >> scan MADT if the real numbers of processors entries are reached no matter >> how big ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES is, I think we can just >> define a number big enough then it will work (x86 and ia64 did the same thing). > Also, with GICv3++, there is no such thing as a memory-mapped CPU > interface anymore. What you get is a bunch of redistributors (one per > CPU). I assume what you have here actually describe the redistributors, > and its name should reflect that. As Sudeep said, it is not to link to GIC architecture, so I think we can keep it stick with ACPI spec, in ACPI spec, it called "GICC structure" (section 5.2.12.14 in ACPI 5.1), so we can name it as ACPI_MAX_GICC_STRUCTURE_ENTRIES no matter GICv2 or GICv3/4 (with GICv2, it may have more than 8 entries with some disabled ones, will no more than 8 enabled entries). What do you think? Thanks Hanjun
On Wednesday 03 September 2014 11:26:14 Tomasz Nowicki wrote: > On 02.09.2014 15:02, Marc Zyngier wrote: > > On 02/09/14 12:48, Tomasz Nowicki wrote: > >> On 01.09.2014 19:35, Marc Zyngier wrote: > >>>> @@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs *)) > >>>> void __init init_IRQ(void) > >>>> { > >>>> irqchip_init(); > >>>> + > >>>> + if (!handle_arch_irq) > >>>> + acpi_gic_init(); > >>>> + > >>> > >>> Why isn't this called from irqchip_init? It would seem like the logical > >>> spot to probe an interrupt controller. > >> > >> irqchip.c is OF dependent, I want to decouple these from the very > >> beginning. > > > > No. irqchip.c is not OF dependent, it is just that DT is the only thing > > we support so far. I don't think duplicating the kernel infrastructure > > "because we're different" is the right way. > > > > There is no reason for your probing structure to be artificially > > different (you're parsing the same information, at the same time). Just > > put in place a similar probing mechanism, and this will look a lot better. > >> Having only GICv2, it would work. Considering we would do the same for > >> GICv3 (arm-gic-v3.h) there will be register name conflicts for both > >> headers inclusion: > >> > >> [...] > >> #include <linux/irqchip/arm-gic.h> > >> #include <linux/irqchip/arm-gic-v3.h> > >> [...] > >> err = gic_v3_acpi_init(table); > >> if (err) > >> err = gic_v2_acpi_init(table); > >> if (err) > >> pr_err("Failed to initialize GIC IRQ controller"); > >> [...] > >> So instead of changing register names prefix, I choose new header will > >> be less painfully. > > > > Yes, and this is exactly why I pushed back on that last time. I'll > > continue saying that interrupt controllers should be self-probing, with > > ACPI as they are with DT. > > > > Even with the restrictions of ACPI and SBSA, we end-up with at least 2 > > main families of interrupt controllers (GICv2 and GICv3), both with a > > number of "interesting" variations (GICv2m and GICv4, to only mention > > those I'm directly involved with). > > > > I can safely predict that the above will become a tangled mess within 18 > > months, and the idea of littering the arch code with a bunch of > > hardcoded "if (blah())" doesn't fill me with joy and confidence. > > > > In summary: we have the infrastructure already, just use it. > > We had that discussion but I see we still don't have consensus here. It > would be good to know our direction before we prepare next patch > version. Arnd any comments on this from you side? I still prefer being explicit here for the same reason I mentioned earlier: I want it to be very clear that we don't support arbitrary irqchips other than the ones in the APCI specification. The infrastructure exists on DT because we have to support a large number of incompatible irqchips. In particular, the ACPI tables describing the irqchip have no way to identify the GIC at all, if I read the spec correctly, you have to parse the tables, ioremap the registers and then read the ID to know if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any "device" for the GIC that a hypothetical probe function would be based on. It does seem wrong to parse the tables in the irq-gic.c file though: that part can well be common across the various gic versions and then call into either irq-gic.c or irq-gic-v3.c for the version specific parts. Whether we put that common code into drivers/irqchip/irqchip.c, drivers/irqchip/gic-common.c, drivers/irqchip/irq-acpi-gic.c or drivers/acpi/irq-gic.c I don't care at all. Arnd
On Monday 01 September 2014 22:57:51 Hanjun Guo wrote: > + /* Collect CPU base addresses */ > + count = acpi_parse_entries(sizeof(struct acpi_table_madt), > + gic_acpi_parse_madt_cpu, table, > + ACPI_MADT_TYPE_GENERIC_INTERRUPT, > + ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES); > + if (count < 0) { > + pr_err("Error during GICC entries parsing\n"); > + return -EFAULT; > + } else if (!count) { > + /* No GICC entries provided, use address from MADT header */ > + struct acpi_table_madt *madt = (struct acpi_table_madt *)table; > + > + if (!madt->address) > + return -EFAULT; > + > + cpu_phy_base = (u64)madt->address; > + } After I read through ACPI-5.1 section 5.2.12.14, I wonder if this is the best way to treat a missing ACPI_MADT_TYPE_GENERIC_INTERRUPT table. Do we expect to see those in practice? It seems like using the x86 local APIC address as a fallback for the GIC address is not something we should do unless we absolutely have to support a system that doesn't have the GIC table. Arnd
On 03.09.2014 20:42, Arnd Bergmann wrote: > On Monday 01 September 2014 22:57:51 Hanjun Guo wrote: >> + /* Collect CPU base addresses */ >> + count = acpi_parse_entries(sizeof(struct acpi_table_madt), >> + gic_acpi_parse_madt_cpu, table, >> + ACPI_MADT_TYPE_GENERIC_INTERRUPT, >> + ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES); >> + if (count < 0) { >> + pr_err("Error during GICC entries parsing\n"); >> + return -EFAULT; >> + } else if (!count) { >> + /* No GICC entries provided, use address from MADT header */ >> + struct acpi_table_madt *madt = (struct acpi_table_madt *)table; >> + >> + if (!madt->address) >> + return -EFAULT; >> + >> + cpu_phy_base = (u64)madt->address; >> + } > > After I read through ACPI-5.1 section 5.2.12.14, I wonder if this is the > best way to treat a missing ACPI_MADT_TYPE_GENERIC_INTERRUPT table. > > Do we expect to see those in practice? It seems like using the x86 local > APIC address as a fallback for the GIC address is not something we > should do unless we absolutely have to support a system that doesn't > have the GIC table. No, we do not expect and hopefully there will be no such :) But, we are trying to be as much as possible inline with 5.1 spec, 5.2.12.14 says: [...] If provided here (CPU physical base address), the "Local Interrupt Controller Address" field in the MADT must be ignored by the OSPM. [...] Regards, Tomasz
On Thursday 04 September 2014 12:10:28 Tomasz Nowicki wrote: > On 03.09.2014 20:42, Arnd Bergmann wrote: > > On Monday 01 September 2014 22:57:51 Hanjun Guo wrote: > >> + /* Collect CPU base addresses */ > >> + count = acpi_parse_entries(sizeof(struct acpi_table_madt), > >> + gic_acpi_parse_madt_cpu, table, > >> + ACPI_MADT_TYPE_GENERIC_INTERRUPT, > >> + ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES); > >> + if (count < 0) { > >> + pr_err("Error during GICC entries parsing\n"); > >> + return -EFAULT; > >> + } else if (!count) { > >> + /* No GICC entries provided, use address from MADT header */ > >> + struct acpi_table_madt *madt = (struct acpi_table_madt *)table; > >> + > >> + if (!madt->address) > >> + return -EFAULT; > >> + > >> + cpu_phy_base = (u64)madt->address; > >> + } > > > > After I read through ACPI-5.1 section 5.2.12.14, I wonder if this is the > > best way to treat a missing ACPI_MADT_TYPE_GENERIC_INTERRUPT table. > > > > Do we expect to see those in practice? It seems like using the x86 local > > APIC address as a fallback for the GIC address is not something we > > should do unless we absolutely have to support a system that doesn't > > have the GIC table. > > No, we do not expect and hopefully there will be no such > > But, we are trying to be as much as possible inline with 5.1 spec, > 5.2.12.14 says: > [...] > If provided here (CPU physical base address), the "Local Interrupt > Controller Address" field in the MADT must be ignored by the OSPM. > [...] > Yes, that's what I saw. So ignoring it all the time is fine, right? Presumably the madt->address field is only referenced here because some pre-5.1 implementations used to do that. Arnd
On 04.09.2014 12:14, Arnd Bergmann wrote: > On Thursday 04 September 2014 12:10:28 Tomasz Nowicki wrote: >> On 03.09.2014 20:42, Arnd Bergmann wrote: >>> On Monday 01 September 2014 22:57:51 Hanjun Guo wrote: >>>> + /* Collect CPU base addresses */ >>>> + count = acpi_parse_entries(sizeof(struct acpi_table_madt), >>>> + gic_acpi_parse_madt_cpu, table, >>>> + ACPI_MADT_TYPE_GENERIC_INTERRUPT, >>>> + ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES); >>>> + if (count < 0) { >>>> + pr_err("Error during GICC entries parsing\n"); >>>> + return -EFAULT; >>>> + } else if (!count) { >>>> + /* No GICC entries provided, use address from MADT header */ >>>> + struct acpi_table_madt *madt = (struct acpi_table_madt *)table; >>>> + >>>> + if (!madt->address) >>>> + return -EFAULT; >>>> + >>>> + cpu_phy_base = (u64)madt->address; >>>> + } >>> >>> After I read through ACPI-5.1 section 5.2.12.14, I wonder if this is the >>> best way to treat a missing ACPI_MADT_TYPE_GENERIC_INTERRUPT table. >>> >>> Do we expect to see those in practice? It seems like using the x86 local >>> APIC address as a fallback for the GIC address is not something we >>> should do unless we absolutely have to support a system that doesn't >>> have the GIC table. >> >> No, we do not expect and hopefully there will be no such >> >> But, we are trying to be as much as possible inline with 5.1 spec, >> 5.2.12.14 says: >> [...] >> If provided here (CPU physical base address), the "Local Interrupt >> Controller Address" field in the MADT must be ignored by the OSPM. >> [...] >> > > Yes, that's what I saw. So ignoring it all the time is fine, right? > Presumably the madt->address field is only referenced here because > some pre-5.1 implementations used to do that. So this is very vague statement. On the one hand it would make sense to take madt->address if we have no GICC entries. On the other hand we do not support non-banked GIC cpu registers. So all of then need to have the same cpu_base_address. What if one has null address? Should the rest take madt->address? I think you are right, I will remove madt->address fallback and simplify the code. Thanks, Tomasz
On 2014?09?03? 19:17, Hanjun Guo wrote: >>>>>>> + >>>>>>> +#ifdef CONFIG_ACPI >>>>>>> +#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535 >>>>>> With GICv2? I doubt it. >>>>> I will create macro for each GIC driver: >>>>> #define ACPI_MAX_GICV2_CPU_INTERFACE_ENTRIES 8 >>>>> #define ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES 65535 >>>> Where do you get this value (ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES) from? >>> This value is for max processors entries in MADT, and we will use it to scan MADT >>> for SMP/GIC Init, I just make it big enough for GICv3/4. since ACPI core will stop >>> scan MADT if the real numbers of processors entries are reached no matter >>> how big ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES is, I think we can just >>> define a number big enough then it will work (x86 and ia64 did the same thing). >> Also, with GICv3++, there is no such thing as a memory-mapped CPU >> interface anymore. What you get is a bunch of redistributors (one per >> CPU). I assume what you have here actually describe the redistributors, >> and its name should reflect that. > As Sudeep said, it is not to link to GIC architecture, so I think we can keep > it stick with ACPI spec, in ACPI spec, it called "GICC structure" (section 5.2.12.14 > in ACPI 5.1), so we can name it as ACPI_MAX_GICC_STRUCTURE_ENTRIES no matter > GICv2 or GICv3/4 (with GICv2, it may have more than 8 entries with some disabled > ones, will no more than 8 enabled entries). > > What do you think? After more consideration on this, we think that we can remove those macros which introduce confusions, and just pass 0 to ACPI core for the max entries of GICC structure or GICR structure, ACPI core will continue scan all the entries in MADT with 0 passed. With that, we can avoid such name confusions for all GIC related structures in MADT no matter GICv2 or GICv3/4. Thanks Hanjun
On 03.09.2014 16:57, Arnd Bergmann wrote: > On Wednesday 03 September 2014 11:26:14 Tomasz Nowicki wrote: >> On 02.09.2014 15:02, Marc Zyngier wrote: >>> On 02/09/14 12:48, Tomasz Nowicki wrote: >>>> On 01.09.2014 19:35, Marc Zyngier wrote: >>>>>> @@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs *)) >>>>>> void __init init_IRQ(void) >>>>>> { >>>>>> irqchip_init(); >>>>>> + >>>>>> + if (!handle_arch_irq) >>>>>> + acpi_gic_init(); >>>>>> + >>>>> >>>>> Why isn't this called from irqchip_init? It would seem like the logical >>>>> spot to probe an interrupt controller. >>>> >>>> irqchip.c is OF dependent, I want to decouple these from the very >>>> beginning. >>> >>> No. irqchip.c is not OF dependent, it is just that DT is the only thing >>> we support so far. I don't think duplicating the kernel infrastructure >>> "because we're different" is the right way. >>> >>> There is no reason for your probing structure to be artificially >>> different (you're parsing the same information, at the same time). Just >>> put in place a similar probing mechanism, and this will look a lot better. > > >>>> Having only GICv2, it would work. Considering we would do the same for >>>> GICv3 (arm-gic-v3.h) there will be register name conflicts for both >>>> headers inclusion: >>>> >>>> [...] >>>> #include <linux/irqchip/arm-gic.h> >>>> #include <linux/irqchip/arm-gic-v3.h> >>>> [...] >>>> err = gic_v3_acpi_init(table); >>>> if (err) >>>> err = gic_v2_acpi_init(table); >>>> if (err) >>>> pr_err("Failed to initialize GIC IRQ controller"); >>>> [...] >>>> So instead of changing register names prefix, I choose new header will >>>> be less painfully. >>> >>> Yes, and this is exactly why I pushed back on that last time. I'll >>> continue saying that interrupt controllers should be self-probing, with >>> ACPI as they are with DT. >>> >>> Even with the restrictions of ACPI and SBSA, we end-up with at least 2 >>> main families of interrupt controllers (GICv2 and GICv3), both with a >>> number of "interesting" variations (GICv2m and GICv4, to only mention >>> those I'm directly involved with). >>> >>> I can safely predict that the above will become a tangled mess within 18 >>> months, and the idea of littering the arch code with a bunch of >>> hardcoded "if (blah())" doesn't fill me with joy and confidence. >>> >>> In summary: we have the infrastructure already, just use it. >> >> We had that discussion but I see we still don't have consensus here. It >> would be good to know our direction before we prepare next patch >> version. Arnd any comments on this from you side? > > I still prefer being explicit here for the same reason I mentioned earlier: > I want it to be very clear that we don't support arbitrary irqchips other > than the ones in the APCI specification. The infrastructure exists on DT > because we have to support a large number of incompatible irqchips. > > In particular, the ACPI tables describing the irqchip have no way to > identify the GIC at all, if I read the spec correctly, you have to > parse the tables, ioremap the registers and then read the ID to know > if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any "device" > for the GIC that a hypothetical probe function would be based on. Yes, it is just one table with bunch of different subtables types. Since GIC identification register is at different offset across GICv1/2 and GICv3, the probe logic seems to be slightly different. IMO, we should look into our MADT and try GICv3 fist and then GICv2, that means presence of redistributor entries suggests GICv3/4. Its absence means we need to look for GICC subtables and then call GICv2 init. > > It does seem wrong to parse the tables in the irq-gic.c file though: > that part can well be common across the various gic versions and then > call into either irq-gic.c or irq-gic-v3.c for the version specific > parts. Whether we put that common code into drivers/irqchip/irqchip.c, > drivers/irqchip/gic-common.c, drivers/irqchip/irq-acpi-gic.c or > drivers/acpi/irq-gic.c I don't care at all. We already had tried making MADT parser common for all GICs in the separate file (outside GIC driver), it did not end up well. In the light of above comment, the logic will be complex and some GIC local strutures need to be exported: 1. Check redistributor entries. 2. If none exist, as fallback, check GICC entries, there are redistributor base address assigned to CPU. Unfortunately it has no register set size so ioremap only two pages (RD + SGI) check if we support VLPI and extend ioremap if true. We cannot operate on GIC register outside GIC driver so that requires another exported GICv3 function. 3. Jump to redistributor part if we are OK so far, if not, then we start playing with GICC enries and look for cpu base addresses. Honestly, apart from distributor parsing logic, there is no common code. As you can see, current gic_v2_acpi_init() logic is quite easy to follow. And gic_v3_acpi_init() inside of GICv3 driver is easy too. I might not see other solutions but I am open to other suggestions, so please correct me in that case. Regards, Tomasz
On 03/09/14 15:57, Arnd Bergmann wrote: > On Wednesday 03 September 2014 11:26:14 Tomasz Nowicki wrote: >> On 02.09.2014 15:02, Marc Zyngier wrote: >>> On 02/09/14 12:48, Tomasz Nowicki wrote: >>>> On 01.09.2014 19:35, Marc Zyngier wrote: >>>>>> @@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs *)) >>>>>> void __init init_IRQ(void) >>>>>> { >>>>>> irqchip_init(); >>>>>> + >>>>>> + if (!handle_arch_irq) >>>>>> + acpi_gic_init(); >>>>>> + >>>>> >>>>> Why isn't this called from irqchip_init? It would seem like the logical >>>>> spot to probe an interrupt controller. >>>> >>>> irqchip.c is OF dependent, I want to decouple these from the very >>>> beginning. >>> >>> No. irqchip.c is not OF dependent, it is just that DT is the only thing >>> we support so far. I don't think duplicating the kernel infrastructure >>> "because we're different" is the right way. >>> >>> There is no reason for your probing structure to be artificially >>> different (you're parsing the same information, at the same time). Just >>> put in place a similar probing mechanism, and this will look a lot better. > > >>>> Having only GICv2, it would work. Considering we would do the same for >>>> GICv3 (arm-gic-v3.h) there will be register name conflicts for both >>>> headers inclusion: >>>> >>>> [...] >>>> #include <linux/irqchip/arm-gic.h> >>>> #include <linux/irqchip/arm-gic-v3.h> >>>> [...] >>>> err = gic_v3_acpi_init(table); >>>> if (err) >>>> err = gic_v2_acpi_init(table); >>>> if (err) >>>> pr_err("Failed to initialize GIC IRQ controller"); >>>> [...] >>>> So instead of changing register names prefix, I choose new header will >>>> be less painfully. >>> >>> Yes, and this is exactly why I pushed back on that last time. I'll >>> continue saying that interrupt controllers should be self-probing, with >>> ACPI as they are with DT. >>> >>> Even with the restrictions of ACPI and SBSA, we end-up with at least 2 >>> main families of interrupt controllers (GICv2 and GICv3), both with a >>> number of "interesting" variations (GICv2m and GICv4, to only mention >>> those I'm directly involved with). >>> >>> I can safely predict that the above will become a tangled mess within 18 >>> months, and the idea of littering the arch code with a bunch of >>> hardcoded "if (blah())" doesn't fill me with joy and confidence. >>> >>> In summary: we have the infrastructure already, just use it. >> >> We had that discussion but I see we still don't have consensus here. It >> would be good to know our direction before we prepare next patch >> version. Arnd any comments on this from you side? > > I still prefer being explicit here for the same reason I mentioned earlier: > I want it to be very clear that we don't support arbitrary irqchips other > than the ones in the APCI specification. The infrastructure exists on DT > because we have to support a large number of incompatible irqchips. I'm not suggesting that we should support more than the ACPI spec says. And that's certainly the whole point of a spec, isn't it? ACPI says what we support, and we're not going any further. I'm just saying that we shouldn't make the maintenance burden heavier, and the code nothing short of disgusting. Using our current infrastructure doesn't mean we're going to support GIcv2.37. > In particular, the ACPI tables describing the irqchip have no way to > identify the GIC at all, if I read the spec correctly, you have to > parse the tables, ioremap the registers and then read the ID to know > if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any "device" > for the GIC that a hypothetical probe function would be based on. This is not the way I read the spec. Table 5-46 (Interrupt Controller Structure) tells you if you have a CPU interface (GICv1/v2) or a redistributor (GICv3/v4). That's enough to know whether or not you should carry on probing a particular controller. The various GIC versions don't really have a unified memory map anyway (well, none that you can rely on), and you really have to rely on ACPI to tell you what you have. > It does seem wrong to parse the tables in the irq-gic.c file though: > that part can well be common across the various gic versions and then > call into either irq-gic.c or irq-gic-v3.c for the version specific > parts. Whether we put that common code into drivers/irqchip/irqchip.c, > drivers/irqchip/gic-common.c, drivers/irqchip/irq-acpi-gic.c or > drivers/acpi/irq-gic.c I don't care at all. I don't think so you can make that common code very easily. The information required by both drivers is organized differently. If it was, I'd have done that for the DT binding. Thanks, M.
On Friday 05 September 2014 10:47:30 Marc Zyngier wrote: > > > > I still prefer being explicit here for the same reason I mentioned earlier: > > I want it to be very clear that we don't support arbitrary irqchips other > > than the ones in the APCI specification. The infrastructure exists on DT > > because we have to support a large number of incompatible irqchips. > > I'm not suggesting that we should support more than the ACPI spec says. > And that's certainly the whole point of a spec, isn't it? ACPI says what > we support, and we're not going any further. I'm just saying that we > shouldn't make the maintenance burden heavier, and the code nothing > short of disgusting. Using our current infrastructure doesn't mean we're > going to support GIcv2.37. Ok > > In particular, the ACPI tables describing the irqchip have no way to > > identify the GIC at all, if I read the spec correctly, you have to > > parse the tables, ioremap the registers and then read the ID to know > > if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any "device" > > for the GIC that a hypothetical probe function would be based on. > > This is not the way I read the spec. Table 5-46 (Interrupt Controller > Structure) tells you if you have a CPU interface (GICv1/v2) or a > redistributor (GICv3/v4). That's enough to know whether or not you > should carry on probing a particular controller. Ah, good. I missed that. > The various GIC versions don't really have a unified memory map anyway > (well, none that you can rely on), and you really have to rely on ACPI > to tell you what you have. So we are back to needing to support two different irqchip drivers (v1/v2/v2m and v3/v4), instead of five or more, right? > > It does seem wrong to parse the tables in the irq-gic.c file though: > > that part can well be common across the various gic versions and then > > call into either irq-gic.c or irq-gic-v3.c for the version specific > > parts. Whether we put that common code into drivers/irqchip/irqchip.c, > > drivers/irqchip/gic-common.c, drivers/irqchip/irq-acpi-gic.c or > > drivers/acpi/irq-gic.c I don't care at all. > > I don't think so you can make that common code very easily. The > information required by both drivers is organized differently. > If it was, I'd have done that for the DT binding. I see, and that's also what Tomasz just explained. So can we just have one an irqchip_init() function doing this:? if (dt) of_irq_init(__irqchip_of_table); else if (acpi) { read cpu-interface and redistributor address from acpi tables if (cpu-interface) gic_v2_acpi_init(table); else if(redistributor) gic_v3_acpi_init(table) } Arnd
On 05.09.2014 12:13, Arnd Bergmann wrote: > On Friday 05 September 2014 10:47:30 Marc Zyngier wrote: >>> >>> I still prefer being explicit here for the same reason I mentioned earlier: >>> I want it to be very clear that we don't support arbitrary irqchips other >>> than the ones in the APCI specification. The infrastructure exists on DT >>> because we have to support a large number of incompatible irqchips. >> >> I'm not suggesting that we should support more than the ACPI spec says. >> And that's certainly the whole point of a spec, isn't it? ACPI says what >> we support, and we're not going any further. I'm just saying that we >> shouldn't make the maintenance burden heavier, and the code nothing >> short of disgusting. Using our current infrastructure doesn't mean we're >> going to support GIcv2.37. > > Ok > >>> In particular, the ACPI tables describing the irqchip have no way to >>> identify the GIC at all, if I read the spec correctly, you have to >>> parse the tables, ioremap the registers and then read the ID to know >>> if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any "device" >>> for the GIC that a hypothetical probe function would be based on. >> >> This is not the way I read the spec. Table 5-46 (Interrupt Controller >> Structure) tells you if you have a CPU interface (GICv1/v2) or a >> redistributor (GICv3/v4). That's enough to know whether or not you >> should carry on probing a particular controller. > > Ah, good. I missed that. > >> The various GIC versions don't really have a unified memory map anyway >> (well, none that you can rely on), and you really have to rely on ACPI >> to tell you what you have. > > So we are back to needing to support two different irqchip drivers > (v1/v2/v2m and v3/v4), instead of five or more, right? > >>> It does seem wrong to parse the tables in the irq-gic.c file though: >>> that part can well be common across the various gic versions and then >>> call into either irq-gic.c or irq-gic-v3.c for the version specific >>> parts. Whether we put that common code into drivers/irqchip/irqchip.c, >>> drivers/irqchip/gic-common.c, drivers/irqchip/irq-acpi-gic.c or >>> drivers/acpi/irq-gic.c I don't care at all. >> >> I don't think so you can make that common code very easily. The >> information required by both drivers is organized differently. >> If it was, I'd have done that for the DT binding. > > I see, and that's also what Tomasz just explained. So can we just > have one an irqchip_init() function doing this:? > > if (dt) > of_irq_init(__irqchip_of_table); > else if (acpi) { > read cpu-interface and redistributor address from acpi tables > if (cpu-interface) > gic_v2_acpi_init(table); > else if(redistributor) > gic_v3_acpi_init(table) > } > It is pretty much the same as: if (dt) of_irq_init(__irqchip_of_table); else if (acpi) { err = gic_v3_acpi_init(table) { read redistributor address from acpi tables [...] } if (err) { err = gic_v2_acpi_init(table) { read cpu-interface address from acpi tables [...] } } } What basically is my current implementation (well, without GICv3 support but that will be next step). Tomasz
On 05/09/14 11:13, Arnd Bergmann wrote: > On Friday 05 September 2014 10:47:30 Marc Zyngier wrote: >>> >>> I still prefer being explicit here for the same reason I mentioned earlier: >>> I want it to be very clear that we don't support arbitrary irqchips other >>> than the ones in the APCI specification. The infrastructure exists on DT >>> because we have to support a large number of incompatible irqchips. >> >> I'm not suggesting that we should support more than the ACPI spec says. >> And that's certainly the whole point of a spec, isn't it? ACPI says what >> we support, and we're not going any further. I'm just saying that we >> shouldn't make the maintenance burden heavier, and the code nothing >> short of disgusting. Using our current infrastructure doesn't mean we're >> going to support GIcv2.37. > > Ok > >>> In particular, the ACPI tables describing the irqchip have no way to >>> identify the GIC at all, if I read the spec correctly, you have to >>> parse the tables, ioremap the registers and then read the ID to know >>> if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any "device" >>> for the GIC that a hypothetical probe function would be based on. >> >> This is not the way I read the spec. Table 5-46 (Interrupt Controller >> Structure) tells you if you have a CPU interface (GICv1/v2) or a >> redistributor (GICv3/v4). That's enough to know whether or not you >> should carry on probing a particular controller. > > Ah, good. I missed that. > >> The various GIC versions don't really have a unified memory map anyway >> (well, none that you can rely on), and you really have to rely on ACPI >> to tell you what you have. > > So we are back to needing to support two different irqchip drivers > (v1/v2/v2m and v3/v4), instead of five or more, right? As long as we make sure that the variants are directly probed from the "canonical" driver (v2m from v2, ITS and v4 from v3), then we're OK. >>> It does seem wrong to parse the tables in the irq-gic.c file though: >>> that part can well be common across the various gic versions and then >>> call into either irq-gic.c or irq-gic-v3.c for the version specific >>> parts. Whether we put that common code into drivers/irqchip/irqchip.c, >>> drivers/irqchip/gic-common.c, drivers/irqchip/irq-acpi-gic.c or >>> drivers/acpi/irq-gic.c I don't care at all. >> >> I don't think so you can make that common code very easily. The >> information required by both drivers is organized differently. >> If it was, I'd have done that for the DT binding. > > I see, and that's also what Tomasz just explained. So can we just > have one an irqchip_init() function doing this:? > > if (dt) > of_irq_init(__irqchip_of_table); > else if (acpi) { > read cpu-interface and redistributor address from acpi tables > if (cpu-interface) > gic_v2_acpi_init(table); > else if(redistributor) > gic_v3_acpi_init(table) > } That'd be better, but that only considers the basic architecture. GICv2m and GICv3's ITS bring additional complexity (and their own table parsing). At that stage, I'm not sure how much commonality we're left with. Thanks, M.
On 05.09.2014 12:39, Marc Zyngier wrote: > On 05/09/14 11:13, Arnd Bergmann wrote: >> On Friday 05 September 2014 10:47:30 Marc Zyngier wrote: >>>> >>>> I still prefer being explicit here for the same reason I mentioned earlier: >>>> I want it to be very clear that we don't support arbitrary irqchips other >>>> than the ones in the APCI specification. The infrastructure exists on DT >>>> because we have to support a large number of incompatible irqchips. >>> >>> I'm not suggesting that we should support more than the ACPI spec says. >>> And that's certainly the whole point of a spec, isn't it? ACPI says what >>> we support, and we're not going any further. I'm just saying that we >>> shouldn't make the maintenance burden heavier, and the code nothing >>> short of disgusting. Using our current infrastructure doesn't mean we're >>> going to support GIcv2.37. >> >> Ok >> >>>> In particular, the ACPI tables describing the irqchip have no way to >>>> identify the GIC at all, if I read the spec correctly, you have to >>>> parse the tables, ioremap the registers and then read the ID to know >>>> if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any "device" >>>> for the GIC that a hypothetical probe function would be based on. >>> >>> This is not the way I read the spec. Table 5-46 (Interrupt Controller >>> Structure) tells you if you have a CPU interface (GICv1/v2) or a >>> redistributor (GICv3/v4). That's enough to know whether or not you >>> should carry on probing a particular controller. >> >> Ah, good. I missed that. >> >>> The various GIC versions don't really have a unified memory map anyway >>> (well, none that you can rely on), and you really have to rely on ACPI >>> to tell you what you have. >> >> So we are back to needing to support two different irqchip drivers >> (v1/v2/v2m and v3/v4), instead of five or more, right? > > As long as we make sure that the variants are directly probed from the > "canonical" driver (v2m from v2, ITS and v4 from v3), then we're OK. Yes, that is our intention. > >>>> It does seem wrong to parse the tables in the irq-gic.c file though: >>>> that part can well be common across the various gic versions and then >>>> call into either irq-gic.c or irq-gic-v3.c for the version specific >>>> parts. Whether we put that common code into drivers/irqchip/irqchip.c, >>>> drivers/irqchip/gic-common.c, drivers/irqchip/irq-acpi-gic.c or >>>> drivers/acpi/irq-gic.c I don't care at all. >>> >>> I don't think so you can make that common code very easily. The >>> information required by both drivers is organized differently. >>> If it was, I'd have done that for the DT binding. >> >> I see, and that's also what Tomasz just explained. So can we just >> have one an irqchip_init() function doing this:? >> >> if (dt) >> of_irq_init(__irqchip_of_table); >> else if (acpi) { >> read cpu-interface and redistributor address from acpi tables >> if (cpu-interface) >> gic_v2_acpi_init(table); >> else if(redistributor) >> gic_v3_acpi_init(table) >> } > > That'd be better, but that only considers the basic architecture. GICv2m > and GICv3's ITS bring additional complexity (and their own table > parsing). At that stage, I'm not sure how much commonality we're left with. Right, my local ACPI GICv3 support assumes to probe ITS from gic_v3_acpi_init() body. Tomasz
On 09/01/2014 01:35 PM, Marc Zyngier wrote: > On 01/09/14 15:57, Hanjun Guo wrote: >> From: Tomasz Nowicki <tomasz.nowicki@linaro.org> >> >> ACPI kernel uses MADT table for proper GIC initialization. It needs to >> parse GIC related subtables, collect CPU interface and distributor >> addresses and call driver initialization function (which is hardware >> abstraction agnostic). In a similar way, FDT initialize GICv1/2. >> >> NOTE: This commit allow to initialize GICv1/2 only. > > I cannot help but notice that there is no support for KVM here. It'd be > good to add a note to that effect, so that people do not expect > virtualization support to be working when booting with ACPI. Wei Huang within Red Hat is currently beginning the process of bringing up KVM support under ACPI on real hardware. This will be fixed asap. Jon.
On 09/03/2014 06:30 AM, Marc Zyngier wrote: > On 02/09/14 16:45, Hanjun Guo wrote: >> This value is for max processors entries in MADT, and we will use it to scan MADT >> for SMP/GIC Init, I just make it big enough for GICv3/4. since ACPI core will stop >> scan MADT if the real numbers of processors entries are reached no matter >> how big ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES is, I think we can just >> define a number big enough then it will work (x86 and ia64 did the same thing). > > Also, with GICv3++, there is no such thing as a memory-mapped CPU > interface anymore. What you get is a bunch of redistributors (one per > CPU). I assume what you have here actually describe the redistributors, > and its name should reflect that. (though you could have a GICv3/v4 system providing a legacy GICv2(m) compatibility mode having the CPU memory interfaces still defined) Jon.
On 09/03/2014 10:57 AM, Arnd Bergmann wrote: > On Wednesday 03 September 2014 11:26:14 Tomasz Nowicki wrote: > In particular, the ACPI tables describing the irqchip have no way to > identify the GIC at all, if I read the spec correctly, you have to > parse the tables, ioremap the registers and then read the ID to know > if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any "device" > for the GIC that a hypothetical probe function would be based on. (aside) I have already noticed this and am separately raising a request to have this dealt with in the specification at a later time. I believe it's fairly contrived, since I don't think it'll be at all common to have a GICv3/v4 IP implementation that has a CPU interface defined. The problem (not now, but in later implementations that actually have GICv3/v4 hardware is making assumptions since even a GICv3 or GICv4 system could implement a legacy compatibility mode in which the memory register interface is defined and mappable so you would be valid in having defined memory addresses in the MADT linked structures then. Jon.
On 09/03/2014 02:42 PM, Arnd Bergmann wrote: > On Monday 01 September 2014 22:57:51 Hanjun Guo wrote: >> + /* Collect CPU base addresses */ >> + count = acpi_parse_entries(sizeof(struct acpi_table_madt), >> + gic_acpi_parse_madt_cpu, table, >> + ACPI_MADT_TYPE_GENERIC_INTERRUPT, >> + ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES); >> + if (count < 0) { >> + pr_err("Error during GICC entries parsing\n"); >> + return -EFAULT; >> + } else if (!count) { >> + /* No GICC entries provided, use address from MADT header */ >> + struct acpi_table_madt *madt = (struct acpi_table_madt *)table; >> + >> + if (!madt->address) >> + return -EFAULT; >> + >> + cpu_phy_base = (u64)madt->address; >> + } > > After I read through ACPI-5.1 section 5.2.12.14, I wonder if this is the > best way to treat a missing ACPI_MADT_TYPE_GENERIC_INTERRUPT table. > > Do we expect to see those in practice? It seems like using the x86 local > APIC address as a fallback for the GIC address is not something we > should do unless we absolutely have to support a system that doesn't > have the GIC table. All GICv2 based ACPI systems should define GICCs for the CPU interfaces. Jon.
On Tue, 02 Sep 2014 13:48:37 +0200, Tomasz Nowicki <tomasz.nowicki@linaro.org> wrote: > On 01.09.2014 19:35, Marc Zyngier wrote: > > On 01/09/14 15:57, Hanjun Guo wrote: > >> From: Tomasz Nowicki <tomasz.nowicki@linaro.org> > >> > >> ACPI kernel uses MADT table for proper GIC initialization. It needs to > >> parse GIC related subtables, collect CPU interface and distributor > >> addresses and call driver initialization function (which is hardware > >> abstraction agnostic). In a similar way, FDT initialize GICv1/2. > >> > >> NOTE: This commit allow to initialize GICv1/2 only. > > > > I cannot help but notice that there is no support for KVM here. It'd be > > good to add a note to that effect, so that people do not expect > > virtualization support to be working when booting with ACPI. > > yes, it is worth mentioning! > > > > >> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org> > >> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > >> --- > >> arch/arm64/include/asm/acpi.h | 2 - > >> arch/arm64/kernel/acpi.c | 23 +++++++ > >> arch/arm64/kernel/irq.c | 5 ++ > >> drivers/irqchip/irq-gic.c | 114 ++++++++++++++++++++++++++++++++++ > >> include/linux/irqchip/arm-gic-acpi.h | 33 ++++++++++ > >> 5 files changed, 175 insertions(+), 2 deletions(-) > >> create mode 100644 include/linux/irqchip/arm-gic-acpi.h > >> > >> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h > >> index a867467..5d2ab63 100644 > >> --- a/arch/arm64/include/asm/acpi.h > >> +++ b/arch/arm64/include/asm/acpi.h > >> @@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void); > >> extern int (*acpi_suspend_lowlevel)(void); > >> #define acpi_wakeup_address 0 > >> > >> -#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535 > >> - > >> #else > >> > >> static inline bool acpi_psci_present(void) { return false; } > >> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c > >> index 354b912..b3b82b0 100644 > >> --- a/arch/arm64/kernel/acpi.c > >> +++ b/arch/arm64/kernel/acpi.c > >> @@ -23,6 +23,7 @@ > >> #include <linux/irqdomain.h> > >> #include <linux/bootmem.h> > >> #include <linux/smp.h> > >> +#include <linux/irqchip/arm-gic-acpi.h> > >> > >> #include <asm/cputype.h> > >> #include <asm/cpu_ops.h> > >> @@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void) > >> pr_err("Can't find FADT or error happened during parsing FADT\n"); > >> } > >> > >> +void __init acpi_gic_init(void) > >> +{ > >> + struct acpi_table_header *table; > >> + acpi_status status; > >> + acpi_size tbl_size; > >> + int err; > >> + > >> + status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, &table, &tbl_size); > >> + if (ACPI_FAILURE(status)) { > >> + const char *msg = acpi_format_exception(status); > >> + > >> + pr_err("Failed to get MADT table, %s\n", msg); > >> + return; > >> + } > >> + > >> + err = gic_v2_acpi_init(table); > >> + if (err) > >> + pr_err("Failed to initialize GIC IRQ controller"); > > > > What will happen when you get to implement GICv3 support? Another entry > > like this? Why isn't this entirely contained in the GIC driver? Do I > > sound like a stuck record? > > There will be another call to GICv3 init: > [...] > err = gic_v3_acpi_init(table); > if (err) > err = gic_v2_acpi_init(table); > if (err) > pr_err("Failed to initialize GIC IRQ controller"); > [...] > This is the main reason I put common code here. > > > > >> + > >> + early_acpi_os_unmap_memory((char *)table, tbl_size); > >> +} > >> + > >> /* > >> * acpi_suspend_lowlevel() - save kernel state and suspend. > >> * > >> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c > >> index 0f08dfd..c074d60 100644 > >> --- a/arch/arm64/kernel/irq.c > >> +++ b/arch/arm64/kernel/irq.c > >> @@ -28,6 +28,7 @@ > >> #include <linux/irqchip.h> > >> #include <linux/seq_file.h> > >> #include <linux/ratelimit.h> > >> +#include <linux/irqchip/arm-gic-acpi.h> > >> > >> unsigned long irq_err_count; > >> > >> @@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs *)) > >> void __init init_IRQ(void) > >> { > >> irqchip_init(); > >> + > >> + if (!handle_arch_irq) > >> + acpi_gic_init(); > >> + > > > > Why isn't this called from irqchip_init? It would seem like the logical > > spot to probe an interrupt controller. > > irqchip.c is OF dependent, I want to decouple these from the very > beginning. It doesn't have to be that way, but given that ARM64 is the only platform in the foreseeable future that will use ACPI irq setup, it doesn't make sense to put it in there. g.
Hi Grant, On 11/09/14 12:48, Grant Likely wrote: > On Tue, 02 Sep 2014 13:48:37 +0200, Tomasz Nowicki <tomasz.nowicki@linaro.org> wrote: >> On 01.09.2014 19:35, Marc Zyngier wrote: >>> On 01/09/14 15:57, Hanjun Guo wrote: >>>> From: Tomasz Nowicki <tomasz.nowicki@linaro.org> >>>> >>>> ACPI kernel uses MADT table for proper GIC initialization. It needs to >>>> parse GIC related subtables, collect CPU interface and distributor >>>> addresses and call driver initialization function (which is hardware >>>> abstraction agnostic). In a similar way, FDT initialize GICv1/2. >>>> >>>> NOTE: This commit allow to initialize GICv1/2 only. >>> >>> I cannot help but notice that there is no support for KVM here. It'd be >>> good to add a note to that effect, so that people do not expect >>> virtualization support to be working when booting with ACPI. >> >> yes, it is worth mentioning! >> >>> >>>> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org> >>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >>>> --- >>>> arch/arm64/include/asm/acpi.h | 2 - >>>> arch/arm64/kernel/acpi.c | 23 +++++++ >>>> arch/arm64/kernel/irq.c | 5 ++ >>>> drivers/irqchip/irq-gic.c | 114 ++++++++++++++++++++++++++++++++++ >>>> include/linux/irqchip/arm-gic-acpi.h | 33 ++++++++++ >>>> 5 files changed, 175 insertions(+), 2 deletions(-) >>>> create mode 100644 include/linux/irqchip/arm-gic-acpi.h >>>> >>>> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h >>>> index a867467..5d2ab63 100644 >>>> --- a/arch/arm64/include/asm/acpi.h >>>> +++ b/arch/arm64/include/asm/acpi.h >>>> @@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void); >>>> extern int (*acpi_suspend_lowlevel)(void); >>>> #define acpi_wakeup_address 0 >>>> >>>> -#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535 >>>> - >>>> #else >>>> >>>> static inline bool acpi_psci_present(void) { return false; } >>>> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c >>>> index 354b912..b3b82b0 100644 >>>> --- a/arch/arm64/kernel/acpi.c >>>> +++ b/arch/arm64/kernel/acpi.c >>>> @@ -23,6 +23,7 @@ >>>> #include <linux/irqdomain.h> >>>> #include <linux/bootmem.h> >>>> #include <linux/smp.h> >>>> +#include <linux/irqchip/arm-gic-acpi.h> >>>> >>>> #include <asm/cputype.h> >>>> #include <asm/cpu_ops.h> >>>> @@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void) >>>> pr_err("Can't find FADT or error happened during parsing FADT\n"); >>>> } >>>> >>>> +void __init acpi_gic_init(void) >>>> +{ >>>> + struct acpi_table_header *table; >>>> + acpi_status status; >>>> + acpi_size tbl_size; >>>> + int err; >>>> + >>>> + status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, &table, &tbl_size); >>>> + if (ACPI_FAILURE(status)) { >>>> + const char *msg = acpi_format_exception(status); >>>> + >>>> + pr_err("Failed to get MADT table, %s\n", msg); >>>> + return; >>>> + } >>>> + >>>> + err = gic_v2_acpi_init(table); >>>> + if (err) >>>> + pr_err("Failed to initialize GIC IRQ controller"); >>> >>> What will happen when you get to implement GICv3 support? Another entry >>> like this? Why isn't this entirely contained in the GIC driver? Do I >>> sound like a stuck record? >> >> There will be another call to GICv3 init: >> [...] >> err = gic_v3_acpi_init(table); >> if (err) >> err = gic_v2_acpi_init(table); >> if (err) >> pr_err("Failed to initialize GIC IRQ controller"); >> [...] >> This is the main reason I put common code here. >> >>> >>>> + >>>> + early_acpi_os_unmap_memory((char *)table, tbl_size); >>>> +} >>>> + >>>> /* >>>> * acpi_suspend_lowlevel() - save kernel state and suspend. >>>> * >>>> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c >>>> index 0f08dfd..c074d60 100644 >>>> --- a/arch/arm64/kernel/irq.c >>>> +++ b/arch/arm64/kernel/irq.c >>>> @@ -28,6 +28,7 @@ >>>> #include <linux/irqchip.h> >>>> #include <linux/seq_file.h> >>>> #include <linux/ratelimit.h> >>>> +#include <linux/irqchip/arm-gic-acpi.h> >>>> >>>> unsigned long irq_err_count; >>>> >>>> @@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs *)) >>>> void __init init_IRQ(void) >>>> { >>>> irqchip_init(); >>>> + >>>> + if (!handle_arch_irq) >>>> + acpi_gic_init(); >>>> + >>> >>> Why isn't this called from irqchip_init? It would seem like the logical >>> spot to probe an interrupt controller. >> >> irqchip.c is OF dependent, I want to decouple these from the very >> beginning. > > It doesn't have to be that way, but given that ARM64 is the only > platform in the foreseeable future that will use ACPI irq setup, it > doesn't make sense to put it in there. I have a different perspective. There is no reason to pollute the arch code with something that is essentially platform specific. irqchip_init is the logical place to probe for an irqchip, and I fail to see the point of sticking this code somewhere else. Why would ACPI be so special that it requires additional hooks in the arch code? Thanks, M.
On Tue, 02 Sep 2014 14:02:31 +0100, Marc Zyngier <marc.zyngier@arm.com> wrote: > On 02/09/14 12:48, Tomasz Nowicki wrote: > > On 01.09.2014 19:35, Marc Zyngier wrote: > >> On 01/09/14 15:57, Hanjun Guo wrote: > >>> From: Tomasz Nowicki <tomasz.nowicki@linaro.org> > >>> > >>> ACPI kernel uses MADT table for proper GIC initialization. It needs to > >>> parse GIC related subtables, collect CPU interface and distributor > >>> addresses and call driver initialization function (which is hardware > >>> abstraction agnostic). In a similar way, FDT initialize GICv1/2. > >>> > >>> NOTE: This commit allow to initialize GICv1/2 only. > >> > >> I cannot help but notice that there is no support for KVM here. It'd be > >> good to add a note to that effect, so that people do not expect > >> virtualization support to be working when booting with ACPI. > > > > yes, it is worth mentioning! > > > >> > >>> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org> > >>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > >>> --- > >>> arch/arm64/include/asm/acpi.h | 2 - > >>> arch/arm64/kernel/acpi.c | 23 +++++++ > >>> arch/arm64/kernel/irq.c | 5 ++ > >>> drivers/irqchip/irq-gic.c | 114 ++++++++++++++++++++++++++++++++++ > >>> include/linux/irqchip/arm-gic-acpi.h | 33 ++++++++++ > >>> 5 files changed, 175 insertions(+), 2 deletions(-) > >>> create mode 100644 include/linux/irqchip/arm-gic-acpi.h > >>> > >>> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h > >>> index a867467..5d2ab63 100644 > >>> --- a/arch/arm64/include/asm/acpi.h > >>> +++ b/arch/arm64/include/asm/acpi.h > >>> @@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void); > >>> extern int (*acpi_suspend_lowlevel)(void); > >>> #define acpi_wakeup_address 0 > >>> > >>> -#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535 > >>> - > >>> #else > >>> > >>> static inline bool acpi_psci_present(void) { return false; } > >>> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c > >>> index 354b912..b3b82b0 100644 > >>> --- a/arch/arm64/kernel/acpi.c > >>> +++ b/arch/arm64/kernel/acpi.c > >>> @@ -23,6 +23,7 @@ > >>> #include <linux/irqdomain.h> > >>> #include <linux/bootmem.h> > >>> #include <linux/smp.h> > >>> +#include <linux/irqchip/arm-gic-acpi.h> > >>> > >>> #include <asm/cputype.h> > >>> #include <asm/cpu_ops.h> > >>> @@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void) > >>> pr_err("Can't find FADT or error happened during parsing FADT\n"); > >>> } > >>> > >>> +void __init acpi_gic_init(void) > >>> +{ > >>> + struct acpi_table_header *table; > >>> + acpi_status status; > >>> + acpi_size tbl_size; > >>> + int err; > >>> + > >>> + status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, &table, &tbl_size); > >>> + if (ACPI_FAILURE(status)) { > >>> + const char *msg = acpi_format_exception(status); > >>> + > >>> + pr_err("Failed to get MADT table, %s\n", msg); > >>> + return; > >>> + } > >>> + > >>> + err = gic_v2_acpi_init(table); > >>> + if (err) > >>> + pr_err("Failed to initialize GIC IRQ controller"); > >> > >> What will happen when you get to implement GICv3 support? Another entry > >> like this? Why isn't this entirely contained in the GIC driver? Do I > >> sound like a stuck record? > > > > There will be another call to GICv3 init: > > [...] > > err = gic_v3_acpi_init(table); > > if (err) > > err = gic_v2_acpi_init(table); > > if (err) > > pr_err("Failed to initialize GIC IRQ controller"); > > [...] > > This is the main reason I put common code here. > > > >> > >>> + > >>> + early_acpi_os_unmap_memory((char *)table, tbl_size); > >>> +} > >>> + > >>> /* > >>> * acpi_suspend_lowlevel() - save kernel state and suspend. > >>> * > >>> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c > >>> index 0f08dfd..c074d60 100644 > >>> --- a/arch/arm64/kernel/irq.c > >>> +++ b/arch/arm64/kernel/irq.c > >>> @@ -28,6 +28,7 @@ > >>> #include <linux/irqchip.h> > >>> #include <linux/seq_file.h> > >>> #include <linux/ratelimit.h> > >>> +#include <linux/irqchip/arm-gic-acpi.h> > >>> > >>> unsigned long irq_err_count; > >>> > >>> @@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs *)) > >>> void __init init_IRQ(void) > >>> { > >>> irqchip_init(); > >>> + > >>> + if (!handle_arch_irq) > >>> + acpi_gic_init(); > >>> + > >> > >> Why isn't this called from irqchip_init? It would seem like the logical > >> spot to probe an interrupt controller. > > > > irqchip.c is OF dependent, I want to decouple these from the very > > beginning. > > No. irqchip.c is not OF dependent, it is just that DT is the only thing > we support so far. I don't think duplicating the kernel infrastructure > "because we're different" is the right way. > > There is no reason for your probing structure to be artificially > different (you're parsing the same information, at the same time). Just > put in place a similar probing mechanism, and this will look a lot better. > > >> > >>> if (!handle_arch_irq) > >>> panic("No interrupt controller found."); > >>> } > >>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > >>> index 4b959e6..85cbf43 100644 > >>> --- a/drivers/irqchip/irq-gic.c > >>> +++ b/drivers/irqchip/irq-gic.c > >>> @@ -33,12 +33,14 @@ > >>> #include <linux/of.h> > >>> #include <linux/of_address.h> > >>> #include <linux/of_irq.h> > >>> +#include <linux/acpi.h> > >>> #include <linux/irqdomain.h> > >>> #include <linux/interrupt.h> > >>> #include <linux/percpu.h> > >>> #include <linux/slab.h> > >>> #include <linux/irqchip/chained_irq.h> > >>> #include <linux/irqchip/arm-gic.h> > >>> +#include <linux/irqchip/arm-gic-acpi.h> > >>> > >>> #include <asm/cputype.h> > >>> #include <asm/irq.h> > >>> @@ -1029,3 +1031,115 @@ IRQCHIP_DECLARE(msm_8660_qgic, "qcom,msm-8660-qgic", gic_of_init); > >>> IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init); > >>> > >>> #endif > >>> + > >>> +#ifdef CONFIG_ACPI > >>> +static u64 dist_phy_base, cpu_phy_base = ULONG_MAX; > >> > >> Please use phys_addr_t for physical addresses. The use of ULONG_MAX > >> looks dodgy. Please have a proper symbol to flag the fact that it hasn't > >> been assigned yet. > > Sure, will do. > > > >> > >>> + > >>> +static int __init > >>> +gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header, > >>> + const unsigned long end) > >>> +{ > >>> + struct acpi_madt_generic_interrupt *processor; > >>> + u64 gic_cpu_base; > >> > >> phys_addr_t > >> > >>> + processor = (struct acpi_madt_generic_interrupt *)header; > >>> + > >>> + if (BAD_MADT_ENTRY(processor, end)) > >>> + return -EINVAL; > >>> + > >>> + gic_cpu_base = processor->base_address; > >>> + if (!gic_cpu_base) > >>> + return -EFAULT; > >> > >> Is zero an invalid address? > > Yeah, good point. > >> > >>> + > >>> + /* > >>> + * There is no support for non-banked GICv1/2 register in ACPI spec. > >>> + * All CPU interface addresses have to be the same. > >>> + */ > >>> + if (cpu_phy_base != ULONG_MAX && gic_cpu_base != cpu_phy_base) > >>> + return -EFAULT; > >>> + > >>> + cpu_phy_base = gic_cpu_base; > >>> + return 0; > >>> +} > >>> + > >>> +static int __init > >>> +gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header, > >>> + const unsigned long end) > >>> +{ > >>> + struct acpi_madt_generic_distributor *dist; > >>> + > >>> + dist = (struct acpi_madt_generic_distributor *)header; > >>> + > >>> + if (BAD_MADT_ENTRY(dist, end)) > >>> + return -EINVAL; > >>> + > >>> + dist_phy_base = dist->base_address; > >>> + if (!dist_phy_base) > >>> + return -EFAULT; > >> > >> Same question about zero. > >> > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +int __init > >>> +gic_v2_acpi_init(struct acpi_table_header *table) > >>> +{ > >>> + void __iomem *cpu_base, *dist_base; > >>> + int count; > >>> + > >>> + /* Collect CPU base addresses */ > >>> + count = acpi_parse_entries(sizeof(struct acpi_table_madt), > >>> + gic_acpi_parse_madt_cpu, table, > >>> + ACPI_MADT_TYPE_GENERIC_INTERRUPT, > >>> + ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES); > >>> + if (count < 0) { > >>> + pr_err("Error during GICC entries parsing\n"); > >>> + return -EFAULT; > >>> + } else if (!count) { > >>> + /* No GICC entries provided, use address from MADT header */ > >>> + struct acpi_table_madt *madt = (struct acpi_table_madt *)table; > >>> + > >>> + if (!madt->address) > >>> + return -EFAULT; > >>> + > >>> + cpu_phy_base = (u64)madt->address; > >>> + } > >>> + > >>> + /* > >>> + * Find distributor base address. We expect one distributor entry since > >>> + * ACPI 5.1 spec neither support multi-GIC instances nor GIC cascade. > >>> + */ > >>> + count = acpi_parse_entries(sizeof(struct acpi_table_madt), > >>> + gic_acpi_parse_madt_distributor, table, > >>> + ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, > >>> + ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES); > >>> + if (count <= 0) { > >>> + pr_err("Error during GICD entries parsing\n"); > >>> + return -EFAULT; > >>> + } else if (count > 1) { > >>> + pr_err("More than one GICD entry detected\n"); > >>> + return -EINVAL; > >>> + } > >>> + > >>> + cpu_base = ioremap(cpu_phy_base, ACPI_GIC_CPU_IF_MEM_SIZE); > >>> + if (!cpu_base) { > >>> + pr_err("Unable to map GICC registers\n"); > >>> + return -ENOMEM; > >>> + } > >>> + > >>> + dist_base = ioremap(dist_phy_base, ACPI_GIC_DIST_MEM_SIZE); > >>> + if (!dist_base) { > >>> + pr_err("Unable to map GICD registers\n"); > >>> + iounmap(cpu_base); > >>> + 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); > >>> + return 0; > >>> +} > >>> +#endif > >>> diff --git a/include/linux/irqchip/arm-gic-acpi.h b/include/linux/irqchip/arm-gic-acpi.h > >>> new file mode 100644 > >>> index 0000000..ce2ae1a8 > >>> --- /dev/null > >>> +++ b/include/linux/irqchip/arm-gic-acpi.h > >>> @@ -0,0 +1,33 @@ > >>> +/* > >>> + * Copyright (C) 2014, Linaro Ltd. > >>> + * Author: Tomasz Nowicki <tomasz.nowicki@linaro.org> > >>> + * > >>> + * 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 > >>> + * published by the Free Software Foundation. > >>> + */ > >>> + > >>> +#ifndef ARM_GIC_ACPI_H_ > >>> +#define ARM_GIC_ACPI_H_ > >>> + > >>> +#include <linux/acpi.h> > >> > >> Do we need linux/acpi.h here? You could have a separate forward > >> declaration of struct acpi_table_header, specially in the light of my > >> last remark below. > > Indeed, we can do forward declaration instead of #include > > <linux/acpi.h>. Thanks! > > > >> > >>> + > >>> +#ifdef CONFIG_ACPI > >>> +#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535 > >> > >> With GICv2? I doubt it. > > I will create macro for each GIC driver: > > #define ACPI_MAX_GICV2_CPU_INTERFACE_ENTRIES 8 > > #define ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES 65535 > > Where do you get this value (ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES) from? > > >> > >>> +#define ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES 1 > >>> + > >>> +/* > >>> + * Hard code here, we can not get memory size from MADT (but FDT does), > >>> + * Actually no need to do that, because this size can be inferred > >>> + * from GIC spec. > >>> + */ > >>> +#define ACPI_GIC_DIST_MEM_SIZE (SZ_64K) > >> > >> I don't know which version of the spec you're looking at, but my version > >> of the GICv2 spec has a 4kB distributor. Also, it would be good to make > >> obvious which GIC version this define is about. > > OK > > > >> > >>> +#define ACPI_GIC_CPU_IF_MEM_SIZE (SZ_8K) > >>> + > >>> +void acpi_gic_init(void); > >>> +int gic_v2_acpi_init(struct acpi_table_header *table); > >>> +#else > >>> +static inline void acpi_gic_init(void) { } > >>> +#endif > >>> + > >>> +#endif /* ARM_GIC_ACPI_H_ */ > >>> > >> > >> In the end, why do we need a separate file for this? I cannot see > >> anything that prevents it from being merged with arm-gic.h. > >> > >> Thanks, > >> > >> M. > >> > > Having only GICv2, it would work. Considering we would do the same for > > GICv3 (arm-gic-v3.h) there will be register name conflicts for both > > headers inclusion: > > > > [...] > > #include <linux/irqchip/arm-gic.h> > > #include <linux/irqchip/arm-gic-v3.h> > > [...] > > err = gic_v3_acpi_init(table); > > if (err) > > err = gic_v2_acpi_init(table); > > if (err) > > pr_err("Failed to initialize GIC IRQ controller"); > > [...] > > So instead of changing register names prefix, I choose new header will > > be less painfully. > > Yes, and this is exactly why I pushed back on that last time. I'll > continue saying that interrupt controllers should be self-probing, with > ACPI as they are with DT. We are in a completely different situation with ACPI compared to DT. In ACPI there is currently exactly one interrupt controller that is allowed (GICv2). In the near future there will be a second one (GICv3). There is zero need to have a infrastructure to abstract a pluggable interface for registering interrupt controllers. With DT we've got a tonne of controllers that we need to support. The pluggable infrastructure was created because we needed it for maintenance. All we're really talking about here is a table of registered irq controllers vs. a few lines of if/else if/else initialization code. The moment it gets unwieldy, we will refactor it. Until then, the if/else block is better code. As for putting the ACPI hook into irqchip_init(), I don't doing so buys us anything. Common code is good when it is common, but in this case there is no other platform is going to use this ACPI irq initialization. Not x86, not ia64. It is ARM64 specific code and moving it out from arm64 doesn't make sense. I'm not going to grump and complain if Hanjun decides to move it into irqchip_init() anyway, but, I don't think this is an issue that should block the patch series. g. > > Even with the restrictions of ACPI and SBSA, we end-up with at least 2 > main families of interrupt controllers (GICv2 and GICv3), both with a > number of "interesting" variations (GICv2m and GICv4, to only mention > those I'm directly involved with). > > I can safely predict that the above will become a tangled mess within 18 > months, and the idea of littering the arch code with a bunch of > hardcoded "if (blah())" doesn't fill me with joy and confidence. > > In summary: we have the infrastructure already, just use it. > > Thanks, > > M. > -- > Jazz is not dead. It just smells funny...
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index a867467..5d2ab63 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void); extern int (*acpi_suspend_lowlevel)(void); #define acpi_wakeup_address 0 -#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535 - #else static inline bool acpi_psci_present(void) { return false; } diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 354b912..b3b82b0 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -23,6 +23,7 @@ #include <linux/irqdomain.h> #include <linux/bootmem.h> #include <linux/smp.h> +#include <linux/irqchip/arm-gic-acpi.h> #include <asm/cputype.h> #include <asm/cpu_ops.h> @@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void) pr_err("Can't find FADT or error happened during parsing FADT\n"); } +void __init acpi_gic_init(void) +{ + struct acpi_table_header *table; + acpi_status status; + acpi_size tbl_size; + int err; + + status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, &table, &tbl_size); + if (ACPI_FAILURE(status)) { + const char *msg = acpi_format_exception(status); + + pr_err("Failed to get MADT table, %s\n", msg); + return; + } + + err = gic_v2_acpi_init(table); + if (err) + pr_err("Failed to initialize GIC IRQ controller"); + + early_acpi_os_unmap_memory((char *)table, tbl_size); +} + /* * acpi_suspend_lowlevel() - save kernel state and suspend. * diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c index 0f08dfd..c074d60 100644 --- a/arch/arm64/kernel/irq.c +++ b/arch/arm64/kernel/irq.c @@ -28,6 +28,7 @@ #include <linux/irqchip.h> #include <linux/seq_file.h> #include <linux/ratelimit.h> +#include <linux/irqchip/arm-gic-acpi.h> unsigned long irq_err_count; @@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs *)) void __init init_IRQ(void) { irqchip_init(); + + if (!handle_arch_irq) + acpi_gic_init(); + if (!handle_arch_irq) panic("No interrupt controller found."); } diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index 4b959e6..85cbf43 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -33,12 +33,14 @@ #include <linux/of.h> #include <linux/of_address.h> #include <linux/of_irq.h> +#include <linux/acpi.h> #include <linux/irqdomain.h> #include <linux/interrupt.h> #include <linux/percpu.h> #include <linux/slab.h> #include <linux/irqchip/chained_irq.h> #include <linux/irqchip/arm-gic.h> +#include <linux/irqchip/arm-gic-acpi.h> #include <asm/cputype.h> #include <asm/irq.h> @@ -1029,3 +1031,115 @@ IRQCHIP_DECLARE(msm_8660_qgic, "qcom,msm-8660-qgic", gic_of_init); IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init); #endif + +#ifdef CONFIG_ACPI +static u64 dist_phy_base, cpu_phy_base = ULONG_MAX; + +static int __init +gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header, + const unsigned long end) +{ + struct acpi_madt_generic_interrupt *processor; + u64 gic_cpu_base; + + processor = (struct acpi_madt_generic_interrupt *)header; + + if (BAD_MADT_ENTRY(processor, end)) + return -EINVAL; + + gic_cpu_base = processor->base_address; + if (!gic_cpu_base) + return -EFAULT; + + /* + * There is no support for non-banked GICv1/2 register in ACPI spec. + * All CPU interface addresses have to be the same. + */ + if (cpu_phy_base != ULONG_MAX && gic_cpu_base != cpu_phy_base) + return -EFAULT; + + cpu_phy_base = gic_cpu_base; + return 0; +} + +static int __init +gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header, + const unsigned long end) +{ + struct acpi_madt_generic_distributor *dist; + + dist = (struct acpi_madt_generic_distributor *)header; + + if (BAD_MADT_ENTRY(dist, end)) + return -EINVAL; + + dist_phy_base = dist->base_address; + if (!dist_phy_base) + return -EFAULT; + + return 0; +} + +int __init +gic_v2_acpi_init(struct acpi_table_header *table) +{ + void __iomem *cpu_base, *dist_base; + int count; + + /* Collect CPU base addresses */ + count = acpi_parse_entries(sizeof(struct acpi_table_madt), + gic_acpi_parse_madt_cpu, table, + ACPI_MADT_TYPE_GENERIC_INTERRUPT, + ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES); + if (count < 0) { + pr_err("Error during GICC entries parsing\n"); + return -EFAULT; + } else if (!count) { + /* No GICC entries provided, use address from MADT header */ + struct acpi_table_madt *madt = (struct acpi_table_madt *)table; + + if (!madt->address) + return -EFAULT; + + cpu_phy_base = (u64)madt->address; + } + + /* + * Find distributor base address. We expect one distributor entry since + * ACPI 5.1 spec neither support multi-GIC instances nor GIC cascade. + */ + count = acpi_parse_entries(sizeof(struct acpi_table_madt), + gic_acpi_parse_madt_distributor, table, + ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, + ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES); + if (count <= 0) { + pr_err("Error during GICD entries parsing\n"); + return -EFAULT; + } else if (count > 1) { + pr_err("More than one GICD entry detected\n"); + return -EINVAL; + } + + cpu_base = ioremap(cpu_phy_base, ACPI_GIC_CPU_IF_MEM_SIZE); + if (!cpu_base) { + pr_err("Unable to map GICC registers\n"); + return -ENOMEM; + } + + dist_base = ioremap(dist_phy_base, ACPI_GIC_DIST_MEM_SIZE); + if (!dist_base) { + pr_err("Unable to map GICD registers\n"); + iounmap(cpu_base); + 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); + return 0; +} +#endif diff --git a/include/linux/irqchip/arm-gic-acpi.h b/include/linux/irqchip/arm-gic-acpi.h new file mode 100644 index 0000000..ce2ae1a8 --- /dev/null +++ b/include/linux/irqchip/arm-gic-acpi.h @@ -0,0 +1,33 @@ +/* + * Copyright (C) 2014, Linaro Ltd. + * Author: Tomasz Nowicki <tomasz.nowicki@linaro.org> + * + * 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 + * published by the Free Software Foundation. + */ + +#ifndef ARM_GIC_ACPI_H_ +#define ARM_GIC_ACPI_H_ + +#include <linux/acpi.h> + +#ifdef CONFIG_ACPI +#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535 +#define ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES 1 + +/* + * Hard code here, we can not get memory size from MADT (but FDT does), + * Actually no need to do that, because this size can be inferred + * from GIC spec. + */ +#define ACPI_GIC_DIST_MEM_SIZE (SZ_64K) +#define ACPI_GIC_CPU_IF_MEM_SIZE (SZ_8K) + +void acpi_gic_init(void); +int gic_v2_acpi_init(struct acpi_table_header *table); +#else +static inline void acpi_gic_init(void) { } +#endif + +#endif /* ARM_GIC_ACPI_H_ */