Messages in this thread | | | Date | Thu, 05 Dec 2013 21:41:48 +0800 | From | Hanjun Guo <> | Subject | Re: [RFC part2 PATCH 9/9] ACPI / GIC: Initialize GIC using the information in MADT |
| |
On 2013年12月04日 23:50, Marc Zyngier wrote: > On 04/12/13 15:32, Hanjun Guo wrote: >> On 2013年12月04日 01:26, Marc Zyngier wrote: >>> Hi Hanjun, >>> >>> On 03/12/13 16:39, Hanjun Guo wrote: >>>> In MADT table, there are GIC cpu interface base address and >>>> GIC distributor base address, use them to convert GIC to ACPI. >>>> >>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> >>>> --- >>>> arch/arm64/kernel/irq.c | 5 ++++ >>>> drivers/acpi/plat/arm-core.c | 66 ++++++++++++++++++++++++++++++++++++------ >>>> include/linux/acpi.h | 6 ++++ >>>> 3 files changed, 68 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c >>>> index 473e5db..a9e68bf 100644 >>>> --- a/arch/arm64/kernel/irq.c >>>> +++ b/arch/arm64/kernel/irq.c >>>> @@ -25,6 +25,7 @@ >>>> #include <linux/irq.h> >>>> #include <linux/smp.h> >>>> #include <linux/init.h> >>>> +#include <linux/acpi.h> >>>> #include <linux/irqchip.h> >>>> #include <linux/seq_file.h> >>>> #include <linux/ratelimit.h> >>>> @@ -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 is the GIC hardcoded? >> Very good question, thanks. I considered GIC only in my patch set. >> I have no idea how to handle the GIC hardcoded problem here for >> now, but I will figure it out later. >> >> If any suggestion, I will appreciate a lot. >> >>> How are you going to support other interrupt >>> controllers? >> ACPI 5.0 supports GICv2 only for now, if we want to >> support other interrupt controller, we should introduce >> some OEM table and parsing it, and it will not covered >> by this patch set. >> >>>> if (!handle_arch_irq) >>>> panic("No interrupt controller found."); >>>> } >>>> diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c >>>> index 17c99e1..509b847 100644 >>>> --- a/drivers/acpi/plat/arm-core.c >>>> +++ b/drivers/acpi/plat/arm-core.c >>>> @@ -29,6 +29,7 @@ >>>> #include <linux/module.h> >>>> #include <linux/irq.h> >>>> #include <linux/irqdomain.h> >>>> +#include <linux/irqchip/arm-gic.h> >>>> #include <linux/slab.h> >>>> #include <linux/bootmem.h> >>>> #include <linux/ioport.h> >>>> @@ -211,11 +212,21 @@ acpi_parse_gic(struct acpi_subtable_header *header, const unsigned long end) >>>> return 0; >>>> } >>>> >>>> +#ifdef CONFIG_ARM_GIC >>>> +/* >>>> + * Hard code here, we can not get memory size from MADT (but FDT does), >>>> + * this size is described in ARMv8 foudation model's User Guide >>>> + */ >>>> +#define GIC_DISTRIBUTOR_MEMORY_SIZE (SZ_8K) >>>> +#define GIC_CPU_INTERFACE_MEMORY_SIZE (SZ_4K) >>> Aside from the incorrect sizes, how do you plan to address the other >>> regions that the GICv2 specification describes? >> Did these regions have the same base address? I mean the same >> as GIC distributor base address and GIC cpu interface base address. >> >> if yes, since the base address is stored in gic_init(), it can be for >> furture >> use. if I misunderstood your question, please let me know. > Look at the VGIC implementation for KVM in virt/kvm/arm. It does its own > probing of the additional regions used for virtualization. > > The GIC and VGIC code are completely separate, and you'll need to find > an acceptable solution for that too.
Ok, will review the VGIC code for KVM, thanks for the guidance.
>>>> static int __init >>>> acpi_parse_gic_distributor(struct acpi_subtable_header *header, >>>> const unsigned long end) >>>> { >>>> struct acpi_madt_generic_distributor *distributor = NULL; >>>> + void __iomem *dist_base = NULL; >>>> + void __iomem *cpu_base = NULL; >>>> >>>> distributor = (struct acpi_madt_generic_distributor *)header; >>>> >>>> @@ -224,8 +235,43 @@ acpi_parse_gic_distributor(struct acpi_subtable_header *header, >>>> >>>> acpi_table_print_madt_entry(header); >>>> >>>> + /* GIC is initialised after page_init(), no need for early_ioremap */ >>>> + dist_base = ioremap(distributor->base_address, >>>> + GIC_CPU_INTERFACE_MEMORY_SIZE); >>>> + if (!dist_base) { >>>> + pr_warn(PREFIX "unable to map gic dist registers\n"); >>>> + return -ENOMEM; >>>> + } >>>> + >>>> + /* >>>> + * acpi_lapic_addr is stored in acpi_parse_madt(), >>>> + * so we can use it here for GIC init >>>> + */ >>>> + if (acpi_lapic_addr) { >>>> + iounmap(dist_base); >>>> + pr_warn(PREFIX "Invalid GIC cpu interface base address\n"); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + cpu_base = ioremap(acpi_lapic_addr, GIC_CPU_INTERFACE_MEMORY_SIZE); >>>> + if (!cpu_base) { >>>> + iounmap(dist_base); >>>> + pr_warn(PREFIX "unable to map gic cpu registers\n"); >>>> + return -ENOMEM; >>>> + } >>>> + >>>> + gic_init(distributor->gic_id, -1, dist_base, cpu_base); >>>> + >>>> return 0; >>>> } >>>> +#else >>>> +static int __init >>>> +acpi_parse_gic_distributor(struct acpi_subtable_header *header, >>>> + const unsigned long end) >>>> +{ >>>> + return -ENODEV; >>>> +} >>>> +#endif /* CONFIG_ARM_GIC */ >>>> >>>> /* >>>> * Parse GIC cpu interface related entries in MADT >>>> @@ -234,7 +280,7 @@ acpi_parse_gic_distributor(struct acpi_subtable_header *header, >>>> static int __init acpi_parse_madt_gic_entries(void) >>>> { >>>> int count; >>>> - >>>> + >>>> /* >>>> * do a partial walk of MADT to determine how many CPUs >>>> * we have including disabled CPUs >>>> @@ -468,19 +514,21 @@ static void __init acpi_process_madt(void) >>>> * Parse MADT GIC cpu interface entries >>>> */ >>>> error = acpi_parse_madt_gic_entries(); >>>> - if (!error) { >>>> - /* >>>> - * Parse MADT GIC distributor entries >>>> - */ >>>> - acpi_parse_madt_gic_distributor_entries(); >>>> - } >>>> + if (!error) >>>> + pr_info("Using ACPI for processor (GIC) configuration information\n"); >>>> } >>>> >>>> - pr_info("Using ACPI for processor (GIC) configuration information\n"); >>>> - >>>> return; >>>> } >>>> >>>> +int __init acpi_gic_init(void) >>>> +{ >>>> + /* >>>> + * Parse MADT GIC distributor entries >>>> + */ >>>> + return acpi_parse_madt_gic_distributor_entries(); >>>> +} >>>> + >>> Why can't you do the GIC init in the GIC code? We've tried hard to make >>> interrupt controllers discoverable and self contained. >> thanks for your suggestion, Rob also had the same suggestion, >> will try to update it in next version. >> >>> What are you >>> going to do when ACPI adds GICv3 to the mix? I don't really think this >>> model (shoving everything into the core ACPI code) is sustainable in the >>> long run... >> Since GICv3 related ACPI proposal is not public and not goes into ACPI >> spec, my suggestion is that we implement GICv2 only for now and post >> another patches for GICv3 when the new ACPI spec is available. > Certainly. But I think you should aim for a scalable solution right > away, instead of starting with something that we already know won't work > for stuff that is already around the corner (which is what I infer from > your "non public" statement).
yes, sure I will, thanks for your comments.
Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |