On 17/08/15 14:23, Hanjun Guo wrote:
Hi Marc,
Tomasz and I reworked this patch set to address your comments, mainly about the self-probe infrastructure and the way to init GIC redistributor in GICC structures.
For the self-probe infrastructure, we introduce a new file in drivers/acpi/ which can be reused for ioapic in the future;
For init GIC redistributor in GICC structures, I introduced a flag in struct redist_region just as you suggested, and get the redist base from GICC strucutes if no redistributor regions are available from GICR strucutes. I think the naming of the flag and functions are not good, please comment on them :)
For the proper namespace of domain token for ACPI, I think we can use the Interrupt Controller Structure Types as the token, (just some ideas, not implemented in the patch, trying to discuss with you that if it makes sense to you), for Structure Types, we got: 1 - I/O APIC 0xB - GICC CPU Interface Structure 0xC - GICD GIC Distributor Structure 0xD - GICv2m MSI Frame 0xE - GICR Redistributor Structure 0xF - ITS Structure
so except the GICD 0xC both for GICv2/3, others can used as proper namespace for different domain tokens, what do you think?
Please comments on if we are on right direction, thanks.
And after my earlier rant, I did what I should have done the first place, which is to do it myself. I started from your first two patches, and cleaned it up.
Hardly any code left at all. Tested on Juno.
From e80ce48c7449429f3191000244d6b94ab92b7f40 Mon Sep 17 00:00:00 2001
From: Marc Zyngier marc.zyngier@arm.com Date: Fri, 28 Aug 2015 18:48:19 +0100 Subject: [PATCH] irqchip/acpi: Fix the bloody mess
Signed-off-by: Marc Zyngier marc.zyngier@arm.com --- drivers/acpi/Makefile | 3 -- drivers/acpi/irq.c | 110 ---------------------------------------- drivers/irqchip/irq-gic.c | 73 +++++++++++++------------- drivers/irqchip/irqchip.c | 35 ++++++++++++- include/linux/acpi.h | 32 +++++++++--- include/linux/acpi_irq.h | 12 ----- include/linux/irqchip.h | 11 ++-- include/linux/mod_devicetable.h | 9 ---- 8 files changed, 100 insertions(+), 185 deletions(-) delete mode 100644 drivers/acpi/irq.c delete mode 100644 include/linux/acpi_irq.h
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index 599f1df..a742a83 100644 --- a/drivers/acpi/Makefile +++ b/drivers/acpi/Makefile @@ -27,9 +27,6 @@ acpi-$(CONFIG_ACPI_SYSTEM_POWER_STATES_SUPPORT) += sleep.o acpi-y += device_pm.o acpi-$(CONFIG_ACPI_SLEEP) += proc.o
-# IRQ controller probe -acpi-y += irq.o - # # ACPI Bus and Device Drivers # diff --git a/drivers/acpi/irq.c b/drivers/acpi/irq.c deleted file mode 100644 index cb23cb2..0000000 --- a/drivers/acpi/irq.c +++ /dev/null @@ -1,110 +0,0 @@ -/* - * Copyright (C) 2015, Linaro Ltd. - * Author: Tomasz Nowicki tomasz.nowicki@linaro.org - * Author: Hanjun Guo hanjun.guo@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. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with this program. If not, see http://www.gnu.org/licenses/. - */ - -#include <linux/acpi.h> -#include <linux/init.h> - -/* - * This special acpi_table_id is the sentinel at the end of the - * acpi_table_id[] array of all irqchips. It is automatically placed at - * the end of the array by the linker, thanks to being part of a - * special section. - */ -static const struct acpi_table_id -irqchip_acpi_match_end __used __section(__irqchip_acpi_table_end); -extern struct acpi_table_id __irqchip_acpi_table[]; -static struct acpi_table_id *iterator; - -static int __init -acpi_match_gic_redist(struct acpi_subtable_header *header, - const unsigned long end) -{ - return 0; -} - -static bool __init -acpi_gic_redist_is_present(void) -{ - int count; - - count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, - acpi_match_gic_redist, 0); - return count > 0; -} - -static int __init -acpi_match_madt_subtable(struct acpi_subtable_header *header, - const unsigned long end) -{ - struct acpi_madt_generic_distributor *dist; - u8 gic_version = ACPI_MADT_GIC_VERSION_NONE; - - /* Found appropriated subtable, now try to do additional matching */ - switch (header->type) { - case ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR: - - dist = (struct acpi_madt_generic_distributor *)header; - gic_version = dist->version; - - /* - * This is for backward compatibility with ACPI 5.1, - * which has no gic_version field. - */ - if (gic_version == ACPI_MADT_GIC_VERSION_NONE) { - /* It is GICv3/v4 if redistributor is present */ - if (acpi_gic_redist_is_present()) - gic_version = ACPI_MADT_GIC_VERSION_V3; - else - gic_version = ACPI_MADT_GIC_VERSION_V2; - - return 0; - } - - /* - * GICv4 has meaning to KVM, for host IRQ controller we can - * treat it as GICv3 to avoid another IRQCHIP_ACPI_DECLARE - * entry. - */ - if (gic_version == ACPI_MADT_GIC_VERSION_V4) - gic_version = ACPI_MADT_GIC_VERSION_V3; - - if (gic_version == iterator->driver_data) - return 0; - - return -AE_NOT_FOUND; - } - - /* No additional matching for the rest of subtable types for now */ - return 0; -} - -void __init acpi_irq_init(void) -{ - - if (acpi_disabled) - return; - - for (iterator = __irqchip_acpi_table; iterator->id[0]; iterator++) { - if (acpi_table_parse_madt(iterator->type, - acpi_match_madt_subtable, 0) <= 0) - continue; /* No match or invalid subtables */ - - acpi_table_parse(ACPI_SIG_MADT, - (acpi_tbl_table_handler)iterator->handler); - } -} diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index 03adec8..5a9f586 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -1069,21 +1069,6 @@ gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header, 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; - return 0; -} - static int gic_acpi_gsi_desc_populate(struct acpi_gsi_descriptor *data, u32 gsi, unsigned int irq_type) { @@ -1109,37 +1094,47 @@ static int gic_acpi_gsi_desc_populate(struct acpi_gsi_descriptor *data, return 0; }
-static int __init -gic_v2_acpi_init(struct acpi_table_header *table) +static int __init acpi_match_gic_redist(struct acpi_subtable_header *header, + const unsigned long end) { + return 0; +} + +static bool __init acpi_gic_redist_is_present(void) +{ + return acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR, + acpi_match_gic_redist, 0) > 0; +} + +static bool __init gic_validate_dist(struct acpi_subtable_header *header, + struct acpi_table_id *id) +{ + struct acpi_madt_generic_distributor *dist; + dist = (struct acpi_madt_generic_distributor *)header; + + return (dist->version == id->driver_data); +} + +static int __init gic_v2_acpi_init(struct acpi_subtable_header *header, + const unsigned long end) +{ + struct acpi_madt_generic_distributor *dist; void __iomem *cpu_base, *dist_base; int count;
+ if (acpi_gic_redist_is_present()) + return -EINVAL; + /* Collect CPU base addresses */ - count = acpi_parse_entries(ACPI_SIG_MADT, - sizeof(struct acpi_table_madt), - gic_acpi_parse_madt_cpu, table, - ACPI_MADT_TYPE_GENERIC_INTERRUPT, 0); + count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT, + gic_acpi_parse_madt_cpu, 0); if (count <= 0) { pr_err("No valid GICC entries exist\n"); return -EINVAL; }
- /* - * 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(ACPI_SIG_MADT, - sizeof(struct acpi_table_madt), - gic_acpi_parse_madt_distributor, table, - ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, 0); - if (count <= 0) { - pr_err("No valid GICD entries exist\n"); - return -EINVAL; - } else if (count > 1) { - pr_err("More than one GICD entry detected\n"); - return -EINVAL; - } + dist = (struct acpi_madt_generic_distributor *)header; + dist_phy_base = dist->base_address;
cpu_base = ioremap(cpu_phy_base, ACPI_GIC_CPU_IF_MEM_SIZE); if (!cpu_base) { @@ -1164,5 +1159,9 @@ gic_v2_acpi_init(struct acpi_table_header *table) return 0; } IRQCHIP_ACPI_DECLARE(gic_v2, ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, - ACPI_MADT_GIC_VERSION_V2, gic_v2_acpi_init); + gic_validate_dist, ACPI_MADT_GIC_VERSION_V2, + gic_v2_acpi_init); +IRQCHIP_ACPI_DECLARE(gic_v2_maybe, ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, + gic_validate_dist, ACPI_MADT_GIC_VERSION_NONE, + gic_v2_acpi_init); #endif diff --git a/drivers/irqchip/irqchip.c b/drivers/irqchip/irqchip.c index afd1af3..8a49385 100644 --- a/drivers/irqchip/irqchip.c +++ b/drivers/irqchip/irqchip.c @@ -8,7 +8,7 @@ * warranty of any kind, whether express or implied. */
-#include <linux/acpi_irq.h> +#include <linux/acpi.h> #include <linux/init.h> #include <linux/of_irq.h> #include <linux/irqchip.h> @@ -24,9 +24,40 @@ irqchip_of_match_end __used __section(__irqchip_of_table_end);
extern struct of_device_id __irqchip_of_table[];
+#ifdef CONFIG_ACPI +/* Same dance for ACPI */ +static const struct acpi_table_id +irqchip_acpi_match_end __used __section(__irqchip_acpi_table_end); +extern struct acpi_table_id __irqchip_acpi_table[]; +static struct acpi_table_id *iterator; + +static int __init acpi_match_irqchip(struct acpi_subtable_header *header, + const unsigned long end) +{ + acpi_table_id_validate validate = iterator->validate; + acpi_tbl_entry_handler handler = iterator->handler; + + if (validate && !validate(header, iterator)) + return 0; + + handler(header, end); + return 0; +} + +static void __init acpi_irq_init(void) +{ + if (!acpi_disabled) + for (iterator = __irqchip_acpi_table; + iterator->id[0]; iterator++) + acpi_table_parse_madt(iterator->type, + acpi_match_irqchip, 0); +} +#else +#define acpi_irq_init() do {} while(0) +#endif + void __init irqchip_init(void) { of_irq_init(__irqchip_of_table); - acpi_irq_init(); } diff --git a/include/linux/acpi.h b/include/linux/acpi.h index a30b969..f2f4887 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -830,21 +830,37 @@ static inline struct acpi_device *acpi_get_next_child(struct device *dev, #endif
#ifdef CONFIG_ACPI -#define ACPI_DECLARE(table, name, table_id, subtable, data, fn) \ +struct acpi_table_id; +typedef bool (*acpi_table_id_validate)(struct acpi_subtable_header *, + struct acpi_table_id *); + +#define ACPI_TABLE_ID_LEN 5 + +struct acpi_table_id { + __u8 id[ACPI_TABLE_ID_LEN]; + __u8 type; + acpi_tbl_entry_handler handler; + acpi_table_id_validate validate; + kernel_ulong_t driver_data; +}; + +#define ACPI_DECLARE(table, name, table_id, subtable, valid, data, fn) \ static const struct acpi_table_id __acpi_table_##name \ __used __section(__##table##_acpi_table) \ = { .id = table_id, \ .type = subtable, \ - .handler = (void *)fn, \ + .validate = valid, \ + .handler = fn, \ .driver_data = data } #else -#define ACPI_DECLARE(table, name, table_id, subtable, data, fn) \ - static const struct acpi_table_id __acpi_table_##name \ +#define ACPI_DECLARE(table, name, table_id, subtable, validate, data, fn) \ + static const void * __acpi_table_##name[] \ __attribute__((unused)) \ - = { .id = table_id, \ - .type = subtable, \ - .handler = (void *)fn, \ - .driver_data = data } + = { (void *) table_id, \ + (void *) subtable, \ + (void *) valid, \ + (void *) fn, \ + (void *) data } #endif
#endif /*_LINUX_ACPI_H*/ diff --git a/include/linux/acpi_irq.h b/include/linux/acpi_irq.h deleted file mode 100644 index 4c0e108..0000000 --- a/include/linux/acpi_irq.h +++ /dev/null @@ -1,12 +0,0 @@ -#ifndef _LINUX_ACPI_IRQ_H -#define _LINUX_ACPI_IRQ_H - -#include <linux/irq.h> - -#ifdef CONFIG_ACPI -void acpi_irq_init(void); -#else -static inline void acpi_irq_init(void) { } -#endif - -#endif /* _LINUX_ACPI_IRQ_H */ diff --git a/include/linux/irqchip.h b/include/linux/irqchip.h index a7573fd..7e956d0 100644 --- a/include/linux/irqchip.h +++ b/include/linux/irqchip.h @@ -27,16 +27,19 @@ #define IRQCHIP_DECLARE(name, compat, fn) OF_DECLARE_2(irqchip, name, compat, fn)
/* - * This macro must be used by the different ARM GIC drivers to declare + * This macro must be used by the different irqchip drivers to declare * the association between their version and their initialization function. * * @name: name that must be unique accross all IRQCHIP_ACPI_DECLARE of the * same file. - * @gic_version: version of GIC + * @subtable: Subtable to be identified in MADT + * @validate: Function to be called on that subtable to check its validity. + * Can be NULL. + * @data: data to be checked by the validate function. * @fn: initialization function */ -#define IRQCHIP_ACPI_DECLARE(name, subtable, version, fn) \ - ACPI_DECLARE(irqchip, name, ACPI_SIG_MADT, subtable, version, fn) +#define IRQCHIP_ACPI_DECLARE(name, subtable, validate, data, fn) \ + ACPI_DECLARE(irqchip, name, ACPI_SIG_MADT, subtable, validate, data, fn)
#ifdef CONFIG_IRQCHIP void irqchip_init(void); diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h index ebedaa7..34f25b7 100644 --- a/include/linux/mod_devicetable.h +++ b/include/linux/mod_devicetable.h @@ -193,15 +193,6 @@ struct acpi_device_id { __u32 cls_msk; };
-#define ACPI_TABLE_ID_LEN 5 - -struct acpi_table_id { - __u8 id[ACPI_TABLE_ID_LEN]; - __u8 type; - const void *handler; - kernel_ulong_t driver_data; -}; - #define PNP_ID_LEN 8 #define PNP_MAX_DEVICES 8