From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marc Zyngier Subject: Re: [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support Date: Tue, 02 Sep 2014 16:59:59 +0100 Message-ID: <5405E97F.3080207@arm.com> References: <1409583475-6978-1-git-send-email-hanjun.guo@linaro.org> <1409583475-6978-14-git-send-email-hanjun.guo@linaro.org> <5404AE56.80801@arm.com> <5405AE95.1020201@linaro.org> <5405BFE7.5060005@arm.com> <5405E626.4090306@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from fw-tnat.austin.arm.com ([217.140.110.23]:39956 "EHLO collaborate-mta1.arm.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1754624AbaIBQAI (ORCPT ); Tue, 2 Sep 2014 12:00:08 -0400 In-Reply-To: <5405E626.4090306@linaro.org> Sender: linux-acpi-owner@vger.kernel.org List-Id: linux-acpi@vger.kernel.org To: Hanjun Guo Cc: Tomasz Nowicki , Catalin Marinas , "Rafael J. Wysocki" , Mark Rutland , Olof Johansson , "grant.likely@linaro.org" , "graeme.gregory@linaro.org" , Arnd Bergmann , Sudeep Holla , Will Deacon , Jason Cooper , Bjorn Helgaas , Daniel Lezcano , Mark Brown , Rob Herring , Robert Richter , Lv Zheng , Robert Moore , Lorenzo Pieralisi , Liviu Dudau , Randy Dunlap , Charles Garcia-Tobin , linu On 02/09/14 16:45, Hanjun Guo wrote: > On 2014=E5=B9=B409=E6=9C=8802=E6=97=A5 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 >>>>> >>>>> ACPI kernel uses MADT table for proper GIC initialization. It nee= ds to >>>>> parse GIC related subtables, collect CPU interface and distributo= r >>>>> addresses and call driver initialization function (which is hardw= are >>>>> 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 >>>>> Signed-off-by: Hanjun Guo >>>>> --- >>>>> 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/a= sm/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 >>>>> #include >>>>> #include >>>>> +#include >>>>> >>>>> #include >>>>> #include >>>>> @@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void) >>>>> pr_err("Can't find FADT or error happened during par= sing FADT\n"); >>>>> } >>>>> >>>>> +void __init acpi_gic_init(void) >>>>> +{ >>>>> + struct acpi_table_header *table; >>>>> + acpi_status status; >>>>> + acpi_size tbl_size; >>>>> + int err; >>>>> + >>>>> + status =3D acpi_get_table_with_size(ACPI_SIG_MADT, 0, &table= , &tbl_size); >>>>> + if (ACPI_FAILURE(status)) { >>>>> + const char *msg =3D acpi_format_exception(status); >>>>> + >>>>> + pr_err("Failed to get MADT table, %s\n", msg); >>>>> + return; >>>>> + } >>>>> + >>>>> + err =3D 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 =3D gic_v3_acpi_init(table); >>> if (err) >>> err =3D 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 >>>>> #include >>>>> #include >>>>> +#include >>>>> >>>>> 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 lo= gical >>>> 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 th= ing >> we support so far. I don't think duplicating the kernel infrastructu= re >> "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). J= ust >> put in place a similar probing mechanism, and this will look a lot b= etter. >> >>>>> 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 >>>>> #include >>>>> #include >>>>> +#include >>>>> #include >>>>> #include >>>>> #include >>>>> #include >>>>> #include >>>>> #include >>>>> +#include >>>>> >>>>> #include >>>>> #include >>>>> @@ -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 =3D ULONG_MAX; >>>> Please use phys_addr_t for physical addresses. The use of ULONG_MA= X >>>> 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 =3D (struct acpi_madt_generic_interrupt *)header; >>>>> + >>>>> + if (BAD_MADT_ENTRY(processor, end)) >>>>> + return -EINVAL; >>>>> + >>>>> + gic_cpu_base =3D 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 AC= PI spec. >>>>> + * All CPU interface addresses have to be the same. >>>>> + */ >>>>> + if (cpu_phy_base !=3D ULONG_MAX && gic_cpu_base !=3D cpu_phy= _base) >>>>> + return -EFAULT; >>>>> + >>>>> + cpu_phy_base =3D gic_cpu_base; >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int __init >>>>> +gic_acpi_parse_madt_distributor(struct acpi_subtable_header *hea= der, >>>>> + const unsigned long end) >>>>> +{ >>>>> + struct acpi_madt_generic_distributor *dist; >>>>> + >>>>> + dist =3D (struct acpi_madt_generic_distributor *)header; >>>>> + >>>>> + if (BAD_MADT_ENTRY(dist, end)) >>>>> + return -EINVAL; >>>>> + >>>>> + dist_phy_base =3D 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 =3D acpi_parse_entries(sizeof(struct acpi_table_madt), >>>>> + gic_acpi_parse_madt_cpu, table, >>>>> + ACPI_MADT_TYPE_GENERIC_INTERRUPT, >>>>> + ACPI_MAX_GIC_CPU_INTERFACE_ENTRIE= S); >>>>> + if (count < 0) { >>>>> + pr_err("Error during GICC entries parsing\n"); >>>>> + return -EFAULT; >>>>> + } else if (!count) { >>>>> + /* No GICC entries provided, use address from MADT h= eader */ >>>>> + struct acpi_table_madt *madt =3D (struct acpi_table_= madt *)table; >>>>> + >>>>> + if (!madt->address) >>>>> + return -EFAULT; >>>>> + >>>>> + cpu_phy_base =3D (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 =3D acpi_parse_entries(sizeof(struct acpi_table_madt), >>>>> + gic_acpi_parse_madt_distributor, = table, >>>>> + ACPI_MADT_TYPE_GENERIC_DISTRIBUTO= R, >>>>> + ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES)= ; >>>>> + if (count <=3D 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 =3D 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 =3D 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 G= SI 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 >>>>> + * >>>>> + * 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 >>>> 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 >>> . 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) f= rom? >=20 > 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 c= ore will stop > scan MADT if the real numbers of processors entries are reached no ma= tter > 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 sa= me thing). Then it is worth mentioning that this is an arbitrary limit, unrelated to what the architecture describes. Thanks, M. --=20 Jazz is not dead. It just smells funny... -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" i= n the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html