[PATCH 0/5] ACPI probing infrastructure

[PATCH 0/5] ACPI probing infrastructure

[Marc Zyngier]

IRQ controllers and timers are the two types of device the kernel
requires before being able to use the device driver model.

ACPI so far lacks a proper probing infrastructure similar to the one
we have with DT, where we're able to declare IRQ chips and
clocksources inside the driver code, and let the core code pick it up
and call us back on a match. This leads to all kind of really ugly
hacks all over the arm64 code and even in the ACPI layer.

It turns out that providing such a probing infrastructure is rather
easy, and provides a much deserved cleanup in both the arch code, the
GIC driver, and the architected timer driver.

I'm sure there is some more code to be deleted, and one can only
wonder why this wasn't done before the arm64 code was initially merged
(the diffstat says it all...).

Patches are against v4.2, and a branch is available at

git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git acpi/device-probing

Marc Zyngier (5):
  acpi: Add basic device probing infrastructure
  irqchip/acpi: Add probing infrastructure for ACPI-based irqchips
  irqchip/gic: Convert the GIC driver to ACPI probing
  clocksource/acpi: Add probing infrastructure for ACPI-based
    clocksources
  clocksource/arm_arch_timer: Convert to ACPI probing

 arch/arm64/include/asm/acpi.h        |  1 -
 arch/arm64/include/asm/irq.h         | 13 -------
 arch/arm64/kernel/acpi.c             | 25 -------------
 arch/arm64/kernel/time.c             |  6 ----
 drivers/acpi/scan.c                  | 41 +++++++++++++++++++++
 drivers/clocksource/arm_arch_timer.c | 10 +-----
 drivers/clocksource/clksrc-of.c      |  4 +++
 drivers/irqchip/irq-gic.c            | 69 ++++++++++++++++++------------------
 drivers/irqchip/irqchip.c            |  5 ++-
 include/asm-generic/vmlinux.lds.h    | 11 ++++++
 include/linux/acpi.h                 | 56 +++++++++++++++++++++++++++++
 include/linux/acpi_irq.h             | 10 ------
 include/linux/clocksource.h          |  6 ----
 include/linux/irqchip.h              | 16 +++++++++
 include/linux/irqchip/arm-gic-acpi.h | 31 ----------------
 15 files changed, 166 insertions(+), 138 deletions(-)
 delete mode 100644 include/linux/acpi_irq.h
 delete mode 100644 include/linux/irqchip/arm-gic-acpi.h

-- 
2.1.4

[PATCH 1/5] acpi: Add basic device probing infrastructure

[Marc Zyngier]

IRQ controllers and timers are the two types of device the kernel
requires before being able to use the device driver model.

ACPI so far lacks a proper probing infrastructure similar to the one
we have with DT, where we're able to declare IRQ chips and
clocksources inside the driver code, and let the core code pick it up
and call us back on a match. This leads to all kind of really ugly
hacks all over the arm64 code and even in the ACPI layer.

In order to allow some basic probing based on the ACPI tables,
introduce "struct acpi_probe_entry" which contains just enough
data and callbacks to match a table, an optional subtable, and
call a probe function. A driver can, at build time, register itself
and expect being called if the right entry exists in the ACPI
table.

A acpi_probe_device_init() is provided, taking an ACPI table
identifier, and iterating over the registered entries.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/acpi/scan.c               | 41 ++++++++++++++++++++++++++++
 include/asm-generic/vmlinux.lds.h | 11 ++++++++
 include/linux/acpi.h              | 56 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 108 insertions(+)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index ec25635..9e920ec 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -2793,3 +2793,44 @@ int __init acpi_scan_init(void)
 	mutex_unlock(&acpi_scan_lock);
 	return result;
 }
+
+static const struct acpi_probe_entry device_acpi_probe_end
+		__used __section(__device_acpi_probe_table_end);
+extern struct acpi_probe_entry __device_acpi_probe_table[];
+static struct acpi_probe_entry *ape;
+static int acpi_probe_count;
+static DEFINE_SPINLOCK(acpi_probe_lock);
+
+static int __init acpi_match_madt(struct acpi_subtable_header *header,
+				  const unsigned long end)
+{
+	if (!ape->validate_subtbl || ape->validate_subtbl(header, ape))
+		if (!ape->probe_subtbl(header, end))
+			acpi_probe_count++;
+
+	return 0;
+}
+
+int __init acpi_probe_device_table(const char *id)
+{
+	int count = 0;
+
+	if (acpi_disabled)
+		return 0;
+
+	spin_lock(&acpi_probe_lock);
+	for (ape = __device_acpi_probe_table; ape->probe_table; ape++) {
+		if (!ACPI_COMPARE_NAME(id, ape->id))
+			continue;
+		if (ACPI_COMPARE_NAME(ACPI_SIG_MADT, ape->id)) {
+			acpi_probe_count = 0;
+			acpi_table_parse_madt(ape->type, acpi_match_madt, 0);
+			count += acpi_probe_count;
+		} else {
+			count = acpi_table_parse(ape->id, ape->probe_table);
+		}
+	}
+	spin_unlock(&acpi_probe_lock);
+
+	return count;
+}
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 8bd374d..875397a 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -181,6 +181,16 @@
 #define CPUIDLE_METHOD_OF_TABLES() OF_TABLE(CONFIG_CPU_IDLE, cpuidle_method)
 #define EARLYCON_OF_TABLES()	OF_TABLE(CONFIG_SERIAL_EARLYCON, earlycon)
 
+#ifdef CONFIG_ACPI
+#define ACPI_PROBE_TABLE(name)						\
+	. = ALIGN(8);							\
+	VMLINUX_SYMBOL(__##name##_acpi_probe_table) = .;		\
+	*(__##name##_acpi_probe_table)					\
+	*(__##name##_acpi_probe_table_end)
+#else
+#define ACPI_PROBE_TABLE(name)
+#endif
+
 #define KERNEL_DTB()							\
 	STRUCT_ALIGN();							\
 	VMLINUX_SYMBOL(__dtb_start) = .;				\
@@ -516,6 +526,7 @@
 	CPUIDLE_METHOD_OF_TABLES()					\
 	KERNEL_DTB()							\
 	IRQCHIP_OF_MATCH_TABLE()					\
+	ACPI_PROBE_TABLE(device)					\
 	EARLYCON_TABLE()						\
 	EARLYCON_OF_TABLES()
 
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index d2445fa..bf6dfb9 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -763,6 +763,52 @@ int acpi_dev_prop_read(struct acpi_device *adev, const char *propname,
 
 struct acpi_device *acpi_get_next_child(struct device *dev,
 					struct acpi_device *child);
+
+struct acpi_probe_entry;
+typedef bool (*acpi_probe_entry_validate_subtbl)(struct acpi_subtable_header *,
+						 struct acpi_probe_entry *);
+
+#define ACPI_TABLE_ID_LEN	5
+
+/**
+ * struct acpi_probe_entry - boot-time probing entry
+ * @id:			ACPI table name
+ * @type:		Optional subtable type to match
+ *			(if @id contains subtables)
+ * @validate_subtbl:	Optional callback to check the validity of
+ *			the subtable
+ * @probe_table:	Callback to the driver being probed when table
+ *			match is successful
+ * @probe_subtbl:	Callback to the driver being probed when table and
+ *			subtable match (and optionnal callback is successful)
+ * @driver_data:	Sideband data provided back to the driver
+ */
+struct acpi_probe_entry {
+	__u8 id[ACPI_TABLE_ID_LEN];
+	__u8 type;
+	acpi_probe_entry_validate_subtbl validate_subtbl;
+	union {
+		acpi_tbl_table_handler probe_table;
+		acpi_tbl_entry_handler probe_subtbl;
+	};
+	kernel_ulong_t driver_data;
+};
+
+#define ACPI_DECLARE(table, name, table_id, subtable, valid, data, fn)	\
+	static const struct acpi_probe_entry __acpi_probe_##name	\
+		__used __section(__##table##_acpi_probe_table)		\
+		 = {							\
+			.id = table_id,					\
+			.type = subtable,				\
+			.validate_subtbl = valid,			\
+			.probe_table = (acpi_tbl_table_handler)fn,	\
+			.driver_data = data, 				\
+		   }
+
+#define DEVICE_ACPI_DECLARE(name, table_id, fn)				\
+	ACPI_DECLARE(device, name, table_id, 0, NULL, 0, fn)
+
+int acpi_probe_device_table(const char *id);
 #else
 static inline int acpi_dev_get_property(struct acpi_device *adev,
 					const char *name, acpi_object_type type,
@@ -813,6 +859,16 @@ static inline struct acpi_device *acpi_get_next_child(struct device *dev,
 	return NULL;
 }
 
+#define ACPI_DECLARE(table, name, table_id, subtable, validate, data, fn) \
+	static const void * __acpi_table_##name[]			\
+		__attribute__((unused))					\
+		 = { (void *) table_id,					\
+		     (void *) subtable,					\
+		     (void *) valid,					\
+		     (void *) fn,					\
+		     (void *) data }
+
+static inline int acpi_probe_device_table(const char *id) { return 0; }
 #endif
 
 #endif	/*_LINUX_ACPI_H*/
-- 
2.1.4

[PATCH 2/5] irqchip/acpi: Add probing infrastructure for ACPI-based irqchips

[Marc Zyngier]

DT enjoys a rather nice probing infrastructure for irqchips, while
ACPI is so far stuck into a very distant past.

This patch introduces a declarative API, allowing irqchips to be
self-contained and be called when a particular entry is matched
in the MADT table.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/irq.h | 11 -----------
 drivers/irqchip/irqchip.c    |  8 +++++---
 include/linux/acpi_irq.h     | 10 ----------
 include/linux/irqchip.h      | 16 ++++++++++++++++
 4 files changed, 21 insertions(+), 24 deletions(-)
 delete mode 100644 include/linux/acpi_irq.h

diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
index bbb251b..1a1037a 100644
--- a/arch/arm64/include/asm/irq.h
+++ b/arch/arm64/include/asm/irq.h
@@ -10,15 +10,4 @@ struct pt_regs;
 extern void migrate_irqs(void);
 extern void set_handle_irq(void (*handle_irq)(struct pt_regs *));
 
-static inline void acpi_irq_init(void)
-{
-	/*
-	 * Hardcode ACPI IRQ chip initialization to GICv2 for now.
-	 * Proper irqchip infrastructure will be implemented along with
-	 * incoming  GICv2m|GICv3|ITS bits.
-	 */
-	acpi_gic_init();
-}
-#define acpi_irq_init acpi_irq_init
-
 #endif
diff --git a/drivers/irqchip/irqchip.c b/drivers/irqchip/irqchip.c
index afd1af3..0644d17 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>
@@ -27,6 +27,8 @@ extern struct of_device_id __irqchip_of_table[];
 void __init irqchip_init(void)
 {
 	of_irq_init(__irqchip_of_table);
-
-	acpi_irq_init();
+#if defined(CONFIG_ARM64) && defined(CONFIG_ACPI)
+	acpi_gic_init();	/* Temporary hack */
+#endif
+	acpi_probe_device_table(ACPI_SIG_MADT);
 }
diff --git a/include/linux/acpi_irq.h b/include/linux/acpi_irq.h
deleted file mode 100644
index f10c872..0000000
--- a/include/linux/acpi_irq.h
+++ /dev/null
@@ -1,10 +0,0 @@
-#ifndef _LINUX_ACPI_IRQ_H
-#define _LINUX_ACPI_IRQ_H
-
-#include <linux/irq.h>
-
-#ifndef acpi_irq_init
-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 6388873..1df3123 100644
--- a/include/linux/irqchip.h
+++ b/include/linux/irqchip.h
@@ -11,6 +11,7 @@
 #ifndef _LINUX_IRQCHIP_H
 #define _LINUX_IRQCHIP_H
 
+#include <linux/acpi.h>
 #include <linux/of.h>
 
 /*
@@ -25,6 +26,21 @@
  */
 #define IRQCHIP_DECLARE(name, compat, fn) OF_DECLARE_2(irqchip, name, compat, fn)
 
+/*
+ * 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.
+ * @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, validate, data, fn)	\
+	ACPI_DECLARE(device, name, ACPI_SIG_MADT, subtable, validate, data, fn)
+
 #ifdef CONFIG_IRQCHIP
 void irqchip_init(void);
 #else
-- 
2.1.4

[PATCH 3/5] irqchip/gic: Convert the GIC driver to ACPI probing

[Marc Zyngier]

Now that we have a basic infrastructure to register irqchips and
call them on discovery of a matching entry in MADT, convert the
GIC driver to this new probing method.

It ends up being a code deletion party, which is a rather good thing.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/acpi.h        |  1 -
 arch/arm64/include/asm/irq.h         |  2 --
 arch/arm64/kernel/acpi.c             | 25 -------------
 drivers/irqchip/irq-gic.c            | 69 ++++++++++++++++++------------------
 drivers/irqchip/irqchip.c            |  3 --
 include/linux/irqchip/arm-gic-acpi.h | 31 ----------------
 6 files changed, 35 insertions(+), 96 deletions(-)
 delete 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 406485e..de97359 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -13,7 +13,6 @@
 #define _ASM_ACPI_H
 
 #include <linux/mm.h>
-#include <linux/irqchip/arm-gic-acpi.h>
 
 #include <asm/cputype.h>
 #include <asm/psci.h>
diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
index 1a1037a..94c5367 100644
--- a/arch/arm64/include/asm/irq.h
+++ b/arch/arm64/include/asm/irq.h
@@ -1,8 +1,6 @@
 #ifndef __ASM_IRQ_H
 #define __ASM_IRQ_H
 
-#include <linux/irqchip/arm-gic-acpi.h>
-
 #include <asm-generic/irq.h>
 
 struct pt_regs;
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 19de753..d6463bb 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -205,28 +205,3 @@ void __init acpi_boot_table_init(void)
 			disable_acpi();
 	}
 }
-
-void __init acpi_gic_init(void)
-{
-	struct acpi_table_header *table;
-	acpi_status status;
-	acpi_size tbl_size;
-	int err;
-
-	if (acpi_disabled)
-		return;
-
-	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);
-}
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 4dd8826..5570f5c 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -40,7 +40,6 @@
 #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>
@@ -1043,7 +1042,7 @@ IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init);
 #endif
 
 #ifdef CONFIG_ACPI
-static phys_addr_t dist_phy_base, cpu_phy_base __initdata;
+static phys_addr_t cpu_phy_base __initdata;
 
 static int __init
 gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
@@ -1071,60 +1070,56 @@ 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)
+/* The things you have to do to just *count* something... */
+static int __init acpi_dummy_func(struct acpi_subtable_header *header,
+				  const unsigned long end)
 {
-	struct acpi_madt_generic_distributor *dist;
+	return 0;
+}
 
-	dist = (struct acpi_madt_generic_distributor *)header;
+static bool __init acpi_gic_redist_is_present(void)
+{
+	return acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR,
+				     acpi_dummy_func, 0) > 0;
+}
 
-	if (BAD_MADT_ENTRY(dist, end))
-		return -EINVAL;
+static bool __init gic_validate_dist(struct acpi_subtable_header *header,
+				     struct acpi_probe_entry *ape)
+{
+	struct acpi_madt_generic_distributor *dist;
+	dist = (struct acpi_madt_generic_distributor *)header;
 
-	dist_phy_base = dist->base_address;
-	return 0;
+	return (dist->version == ape->driver_data &&
+		(dist->version != ACPI_MADT_GIC_VERSION_NONE ||
+		 !acpi_gic_redist_is_present()));
 }
 
-int __init
-gic_v2_acpi_init(struct acpi_table_header *table)
+#define ACPI_GICV2_DIST_MEM_SIZE	(SZ_4K)
+#define ACPI_GIC_CPU_IF_MEM_SIZE	(SZ_8K)
+
+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;
 
 	/* 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;
-	}
-
 	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_GICV2_DIST_MEM_SIZE);
+	dist = (struct acpi_madt_generic_distributor *)header;
+	dist_base = ioremap(dist->base_address, ACPI_GICV2_DIST_MEM_SIZE);
 	if (!dist_base) {
 		pr_err("Unable to map GICD registers\n");
 		iounmap(cpu_base);
@@ -1142,4 +1137,10 @@ gic_v2_acpi_init(struct acpi_table_header *table)
 	acpi_irq_model = ACPI_IRQ_MODEL_GIC;
 	return 0;
 }
+IRQCHIP_ACPI_DECLARE(gic_v2, ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR,
+		     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 0644d17..639fd32 100644
--- a/drivers/irqchip/irqchip.c
+++ b/drivers/irqchip/irqchip.c
@@ -27,8 +27,5 @@ extern struct of_device_id __irqchip_of_table[];
 void __init irqchip_init(void)
 {
 	of_irq_init(__irqchip_of_table);
-#if defined(CONFIG_ARM64) && defined(CONFIG_ACPI)
-	acpi_gic_init();	/* Temporary hack */
-#endif
 	acpi_probe_device_table(ACPI_SIG_MADT);
 }
diff --git a/include/linux/irqchip/arm-gic-acpi.h b/include/linux/irqchip/arm-gic-acpi.h
deleted file mode 100644
index de3419e..0000000
--- a/include/linux/irqchip/arm-gic-acpi.h
+++ /dev/null
@@ -1,31 +0,0 @@
-/*
- * 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_
-
-#ifdef CONFIG_ACPI
-
-/*
- * 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_GICV2_DIST_MEM_SIZE	(SZ_4K)
-#define ACPI_GIC_CPU_IF_MEM_SIZE	(SZ_8K)
-
-struct acpi_table_header;
-
-int gic_v2_acpi_init(struct acpi_table_header *table);
-void acpi_gic_init(void);
-#else
-static inline void acpi_gic_init(void) { }
-#endif
-
-#endif /* ARM_GIC_ACPI_H_ */
-- 
2.1.4

[PATCH 4/5] clocksource/acpi: Add probing infrastructure for ACPI-based clocksources

[Marc Zyngier]

DT enjoys a rather nice probing infrastructure for clocksources,
while ACPI is so far stuck into a very distant past.

This patch introduces a declarative API, allowing clocksources
to be self-contained and be called when parsing the GTDT table.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/clocksource/clksrc-of.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/clocksource/clksrc-of.c b/drivers/clocksource/clksrc-of.c
index 0093a8e..b351b1e 100644
--- a/drivers/clocksource/clksrc-of.c
+++ b/drivers/clocksource/clksrc-of.c
@@ -14,6 +14,7 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <linux/acpi.h>
 #include <linux/init.h>
 #include <linux/of.h>
 #include <linux/clocksource.h>
@@ -38,6 +39,9 @@ void __init clocksource_of_init(void)
 		init_func(np);
 		clocksources++;
 	}
+
+	clocksources += acpi_probe_device_table(ACPI_SIG_GTDT);
+
 	if (!clocksources)
 		pr_crit("%s: no matching clocksources found\n", __func__);
 }
-- 
2.1.4

[PATCH 5/5] clocksource/arm_arch_timer: Convert to ACPI probing

[Marc Zyngier]

It is now absolutely trivial to convert the arch timer driver to
use ACPI probing, just like its DT counterpart.

Let's enjoy another crapectomy.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kernel/time.c             |  6 ------
 drivers/clocksource/arm_arch_timer.c | 10 +---------
 include/linux/clocksource.h          |  6 ------
 3 files changed, 1 insertion(+), 21 deletions(-)

diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c
index 42f9195..4e56739 100644
--- a/arch/arm64/kernel/time.c
+++ b/arch/arm64/kernel/time.c
@@ -73,12 +73,6 @@ void __init time_init(void)
 
 	tick_setup_hrtimer_broadcast();
 
-	/*
-	 * Since ACPI or FDT will only one be available in the system,
-	 * we can use acpi_generic_timer_init() here safely
-	 */
-	acpi_generic_timer_init();
-
 	arch_timer_rate = arch_timer_get_rate();
 	if (!arch_timer_rate)
 		panic("Unable to initialise architected timer.\n");
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 0aa135d..ba4e6f2 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -872,13 +872,5 @@ static int __init arch_timer_acpi_init(struct acpi_table_header *table)
 	arch_timer_init();
 	return 0;
 }
-
-/* Initialize all the generic timers presented in GTDT */
-void __init acpi_generic_timer_init(void)
-{
-	if (acpi_disabled)
-		return;
-
-	acpi_table_parse(ACPI_SIG_GTDT, arch_timer_acpi_init);
-}
+DEVICE_ACPI_DECLARE(arch_timer, ACPI_SIG_GTDT, arch_timer_acpi_init);
 #endif
diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index 278dd27..a25fc6e 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -252,10 +252,4 @@ extern void clocksource_of_init(void);
 static inline void clocksource_of_init(void) {}
 #endif
 
-#ifdef CONFIG_ACPI
-void acpi_generic_timer_init(void);
-#else
-static inline void acpi_generic_timer_init(void) { }
-#endif
-
 #endif /* _LINUX_CLOCKSOURCE_H */
-- 
2.1.4

Re: [PATCH 0/5] ACPI probing infrastructure

[Tomasz Nowicki]

Hi Marc,

On 04.09.2015 19:06, Marc Zyngier wrote:
> IRQ controllers and timers are the two types of device the kernel
> requires before being able to use the device driver model.
>
> ACPI so far lacks a proper probing infrastructure similar to the one
> we have with DT, where we're able to declare IRQ chips and
> clocksources inside the driver code, and let the core code pick it up
> and call us back on a match. This leads to all kind of really ugly
> hacks all over the arm64 code and even in the ACPI layer.
>
> It turns out that providing such a probing infrastructure is rather
> easy, and provides a much deserved cleanup in both the arch code, the
> GIC driver, and the architected timer driver.
>
> I'm sure there is some more code to be deleted, and one can only
> wonder why this wasn't done before the arm64 code was initially merged
> (the diffstat says it all...).
>
> Patches are against v4.2, and a branch is available at
>
> git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git acpi/device-probing
>
> Marc Zyngier (5):
>    acpi: Add basic device probing infrastructure
>    irqchip/acpi: Add probing infrastructure for ACPI-based irqchips
>    irqchip/gic: Convert the GIC driver to ACPI probing
>    clocksource/acpi: Add probing infrastructure for ACPI-based
>      clocksources
>    clocksource/arm_arch_timer: Convert to ACPI probing
>
>   arch/arm64/include/asm/acpi.h        |  1 -
>   arch/arm64/include/asm/irq.h         | 13 -------
>   arch/arm64/kernel/acpi.c             | 25 -------------
>   arch/arm64/kernel/time.c             |  6 ----
>   drivers/acpi/scan.c                  | 41 +++++++++++++++++++++
>   drivers/clocksource/arm_arch_timer.c | 10 +-----
>   drivers/clocksource/clksrc-of.c      |  4 +++
>   drivers/irqchip/irq-gic.c            | 69 ++++++++++++++++++------------------
>   drivers/irqchip/irqchip.c            |  5 ++-
>   include/asm-generic/vmlinux.lds.h    | 11 ++++++
>   include/linux/acpi.h                 | 56 +++++++++++++++++++++++++++++
>   include/linux/acpi_irq.h             | 10 ------
>   include/linux/clocksource.h          |  6 ----
>   include/linux/irqchip.h              | 16 +++++++++
>   include/linux/irqchip/arm-gic-acpi.h | 31 ----------------
>   15 files changed, 166 insertions(+), 138 deletions(-)
>   delete mode 100644 include/linux/acpi_irq.h
>   delete mode 100644 include/linux/irqchip/arm-gic-acpi.h
>

Thanks for cleaning this up!

For the whole series:
Acked-by: Tomasz Nowicki <tn@semihalf.com>

Re: [PATCH 1/5] acpi: Add basic device probing infrastructure

[Lorenzo Pieralisi]

[+M.Salter]

On Fri, Sep 04, 2015 at 06:06:48PM +0100, Marc Zyngier wrote:
> IRQ controllers and timers are the two types of device the kernel
> requires before being able to use the device driver model.
> 
> ACPI so far lacks a proper probing infrastructure similar to the one
> we have with DT, where we're able to declare IRQ chips and
> clocksources inside the driver code, and let the core code pick it up
> and call us back on a match. This leads to all kind of really ugly
> hacks all over the arm64 code and even in the ACPI layer.
> 
> In order to allow some basic probing based on the ACPI tables,
> introduce "struct acpi_probe_entry" which contains just enough
> data and callbacks to match a table, an optional subtable, and
> call a probe function. A driver can, at build time, register itself
> and expect being called if the right entry exists in the ACPI
> table.
> 
> A acpi_probe_device_init() is provided, taking an ACPI table
> identifier, and iterating over the registered entries.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  drivers/acpi/scan.c               | 41 ++++++++++++++++++++++++++++
>  include/asm-generic/vmlinux.lds.h | 11 ++++++++
>  include/linux/acpi.h              | 56 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 108 insertions(+)
> 
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index ec25635..9e920ec 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -2793,3 +2793,44 @@ int __init acpi_scan_init(void)
>  	mutex_unlock(&acpi_scan_lock);
>  	return result;
>  }
> +
> +static const struct acpi_probe_entry device_acpi_probe_end
> +		__used __section(__device_acpi_probe_table_end);
> +extern struct acpi_probe_entry __device_acpi_probe_table[];
> +static struct acpi_probe_entry *ape;
> +static int acpi_probe_count;
> +static DEFINE_SPINLOCK(acpi_probe_lock);
> +
> +static int __init acpi_match_madt(struct acpi_subtable_header *header,
> +				  const unsigned long end)
> +{
> +	if (!ape->validate_subtbl || ape->validate_subtbl(header, ape))
> +		if (!ape->probe_subtbl(header, end))
> +			acpi_probe_count++;
> +
> +	return 0;
> +}
> +
> +int __init acpi_probe_device_table(const char *id)
> +{
> +	int count = 0;
> +
> +	if (acpi_disabled)
> +		return 0;
> +
> +	spin_lock(&acpi_probe_lock);
> +	for (ape = __device_acpi_probe_table; ape->probe_table; ape++) {
> +		if (!ACPI_COMPARE_NAME(id, ape->id))
> +			continue;
> +		if (ACPI_COMPARE_NAME(ACPI_SIG_MADT, ape->id)) {
> +			acpi_probe_count = 0;
> +			acpi_table_parse_madt(ape->type, acpi_match_madt, 0);
> +			count += acpi_probe_count;
> +		} else {
> +			count = acpi_table_parse(ape->id, ape->probe_table);
> +		}
> +	}
> +	spin_unlock(&acpi_probe_lock);
> +
> +	return count;
> +}

We should add a mechanism to prevent re-parsing the same entries
multiple times (in case this function is called with the same
signature multiple times). We could create a separate table of device
entries, per-subsystem, that we want to parse (irqchip specific table,
timers, etc.) instead of adding all the devices to the same table (ie
linker section), you can do this already with the current patchset by
just choosing different table names as DT does.

We may also want to extend this set so that it can be used to parse the
same table, same subtype multiple times at different stages in the boot
path (but let's first see if it is a) really needed b) feasible).

Basically it is to avoid parsing the MADT multiple times:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-May/340267.html

Those can be extensions to the current patchset (because basically
they are not real issues at present), it is just a heads-up.

Thanks for putting it together !
Lorenzo

> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index 8bd374d..875397a 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -181,6 +181,16 @@
>  #define CPUIDLE_METHOD_OF_TABLES() OF_TABLE(CONFIG_CPU_IDLE, cpuidle_method)
>  #define EARLYCON_OF_TABLES()	OF_TABLE(CONFIG_SERIAL_EARLYCON, earlycon)
>  
> +#ifdef CONFIG_ACPI
> +#define ACPI_PROBE_TABLE(name)						\
> +	. = ALIGN(8);							\
> +	VMLINUX_SYMBOL(__##name##_acpi_probe_table) = .;		\
> +	*(__##name##_acpi_probe_table)					\
> +	*(__##name##_acpi_probe_table_end)
> +#else
> +#define ACPI_PROBE_TABLE(name)
> +#endif
> +
>  #define KERNEL_DTB()							\
>  	STRUCT_ALIGN();							\
>  	VMLINUX_SYMBOL(__dtb_start) = .;				\
> @@ -516,6 +526,7 @@
>  	CPUIDLE_METHOD_OF_TABLES()					\
>  	KERNEL_DTB()							\
>  	IRQCHIP_OF_MATCH_TABLE()					\
> +	ACPI_PROBE_TABLE(device)					\
>  	EARLYCON_TABLE()						\
>  	EARLYCON_OF_TABLES()
>  
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index d2445fa..bf6dfb9 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -763,6 +763,52 @@ int acpi_dev_prop_read(struct acpi_device *adev, const char *propname,
>  
>  struct acpi_device *acpi_get_next_child(struct device *dev,
>  					struct acpi_device *child);
> +
> +struct acpi_probe_entry;
> +typedef bool (*acpi_probe_entry_validate_subtbl)(struct acpi_subtable_header *,
> +						 struct acpi_probe_entry *);
> +
> +#define ACPI_TABLE_ID_LEN	5
> +
> +/**
> + * struct acpi_probe_entry - boot-time probing entry
> + * @id:			ACPI table name
> + * @type:		Optional subtable type to match
> + *			(if @id contains subtables)
> + * @validate_subtbl:	Optional callback to check the validity of
> + *			the subtable
> + * @probe_table:	Callback to the driver being probed when table
> + *			match is successful
> + * @probe_subtbl:	Callback to the driver being probed when table and
> + *			subtable match (and optionnal callback is successful)
> + * @driver_data:	Sideband data provided back to the driver
> + */
> +struct acpi_probe_entry {
> +	__u8 id[ACPI_TABLE_ID_LEN];
> +	__u8 type;
> +	acpi_probe_entry_validate_subtbl validate_subtbl;
> +	union {
> +		acpi_tbl_table_handler probe_table;
> +		acpi_tbl_entry_handler probe_subtbl;
> +	};
> +	kernel_ulong_t driver_data;
> +};
> +
> +#define ACPI_DECLARE(table, name, table_id, subtable, valid, data, fn)	\
> +	static const struct acpi_probe_entry __acpi_probe_##name	\
> +		__used __section(__##table##_acpi_probe_table)		\
> +		 = {							\
> +			.id = table_id,					\
> +			.type = subtable,				\
> +			.validate_subtbl = valid,			\
> +			.probe_table = (acpi_tbl_table_handler)fn,	\
> +			.driver_data = data, 				\
> +		   }
> +
> +#define DEVICE_ACPI_DECLARE(name, table_id, fn)				\
> +	ACPI_DECLARE(device, name, table_id, 0, NULL, 0, fn)
> +
> +int acpi_probe_device_table(const char *id);
>  #else
>  static inline int acpi_dev_get_property(struct acpi_device *adev,
>  					const char *name, acpi_object_type type,
> @@ -813,6 +859,16 @@ static inline struct acpi_device *acpi_get_next_child(struct device *dev,
>  	return NULL;
>  }
>  
> +#define ACPI_DECLARE(table, name, table_id, subtable, validate, data, fn) \
> +	static const void * __acpi_table_##name[]			\
> +		__attribute__((unused))					\
> +		 = { (void *) table_id,					\
> +		     (void *) subtable,					\
> +		     (void *) valid,					\
> +		     (void *) fn,					\
> +		     (void *) data }
> +
> +static inline int acpi_probe_device_table(const char *id) { return 0; }
>  #endif
>  
>  #endif	/*_LINUX_ACPI_H*/
> -- 
> 2.1.4
> 

Re: [PATCH 1/5] acpi: Add basic device probing infrastructure

[Marc Zyngier]

On 07/09/15 17:00, Lorenzo Pieralisi wrote:
> [+M.Salter]
> 
> On Fri, Sep 04, 2015 at 06:06:48PM +0100, Marc Zyngier wrote:
>> IRQ controllers and timers are the two types of device the kernel
>> requires before being able to use the device driver model.
>>
>> ACPI so far lacks a proper probing infrastructure similar to the one
>> we have with DT, where we're able to declare IRQ chips and
>> clocksources inside the driver code, and let the core code pick it up
>> and call us back on a match. This leads to all kind of really ugly
>> hacks all over the arm64 code and even in the ACPI layer.
>>
>> In order to allow some basic probing based on the ACPI tables,
>> introduce "struct acpi_probe_entry" which contains just enough
>> data and callbacks to match a table, an optional subtable, and
>> call a probe function. A driver can, at build time, register itself
>> and expect being called if the right entry exists in the ACPI
>> table.
>>
>> A acpi_probe_device_init() is provided, taking an ACPI table
>> identifier, and iterating over the registered entries.
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  drivers/acpi/scan.c               | 41 ++++++++++++++++++++++++++++
>>  include/asm-generic/vmlinux.lds.h | 11 ++++++++
>>  include/linux/acpi.h              | 56 +++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 108 insertions(+)
>>
>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>> index ec25635..9e920ec 100644
>> --- a/drivers/acpi/scan.c
>> +++ b/drivers/acpi/scan.c
>> @@ -2793,3 +2793,44 @@ int __init acpi_scan_init(void)
>>  	mutex_unlock(&acpi_scan_lock);
>>  	return result;
>>  }
>> +
>> +static const struct acpi_probe_entry device_acpi_probe_end
>> +		__used __section(__device_acpi_probe_table_end);
>> +extern struct acpi_probe_entry __device_acpi_probe_table[];
>> +static struct acpi_probe_entry *ape;
>> +static int acpi_probe_count;
>> +static DEFINE_SPINLOCK(acpi_probe_lock);
>> +
>> +static int __init acpi_match_madt(struct acpi_subtable_header *header,
>> +				  const unsigned long end)
>> +{
>> +	if (!ape->validate_subtbl || ape->validate_subtbl(header, ape))
>> +		if (!ape->probe_subtbl(header, end))
>> +			acpi_probe_count++;
>> +
>> +	return 0;
>> +}
>> +
>> +int __init acpi_probe_device_table(const char *id)
>> +{
>> +	int count = 0;
>> +
>> +	if (acpi_disabled)
>> +		return 0;
>> +
>> +	spin_lock(&acpi_probe_lock);
>> +	for (ape = __device_acpi_probe_table; ape->probe_table; ape++) {
>> +		if (!ACPI_COMPARE_NAME(id, ape->id))
>> +			continue;
>> +		if (ACPI_COMPARE_NAME(ACPI_SIG_MADT, ape->id)) {
>> +			acpi_probe_count = 0;
>> +			acpi_table_parse_madt(ape->type, acpi_match_madt, 0);
>> +			count += acpi_probe_count;
>> +		} else {
>> +			count = acpi_table_parse(ape->id, ape->probe_table);
>> +		}
>> +	}
>> +	spin_unlock(&acpi_probe_lock);
>> +
>> +	return count;
>> +}
> 
> We should add a mechanism to prevent re-parsing the same entries
> multiple times (in case this function is called with the same
> signature multiple times). We could create a separate table of device
> entries, per-subsystem, that we want to parse (irqchip specific table,
> timers, etc.) instead of adding all the devices to the same table (ie
> linker section), you can do this already with the current patchset by
> just choosing different table names as DT does.

Yeah, my initial approach was to have multiple tables, but I ended up
deciding against it because nothing required it so far, and I wanted to
avoid the over-engineered syndrome.

Also, it could be useful to flag entries that have been successfully
probed to ensure we don't try them again. Though having separate tables
would probably greatly reduce the usefulness of this.

> We may also want to extend this set so that it can be used to parse the
> same table, same subtype multiple times at different stages in the boot
> path (but let's first see if it is a) really needed b) feasible).
> 
> Basically it is to avoid parsing the MADT multiple times:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-May/340267.html
> 
> Those can be extensions to the current patchset (because basically
> they are not real issues at present), it is just a heads-up.

I'll see if I can whip that up by the end of the week - the PMU stuff is
definitely interesting.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 0/5] ACPI probing infrastructure

[Rafael J. Wysocki]

On Friday, September 04, 2015 06:06:47 PM Marc Zyngier wrote:
> IRQ controllers and timers are the two types of device the kernel
> requires before being able to use the device driver model.
> 
> ACPI so far lacks a proper probing infrastructure similar to the one
> we have with DT, where we're able to declare IRQ chips and
> clocksources inside the driver code, and let the core code pick it up
> and call us back on a match. This leads to all kind of really ugly
> hacks all over the arm64 code and even in the ACPI layer.
> 
> It turns out that providing such a probing infrastructure is rather
> easy, and provides a much deserved cleanup in both the arch code, the
> GIC driver, and the architected timer driver.

Since I'm not familiar with the DT probing infrastructure mentioned above,
can you please explain to me (possibly at a high level), how it is supposed
to work in the ACPI case?

Thanks,
Rafael

Re: [PATCH 1/5] acpi: Add basic device probing infrastructure

[Rafael J. Wysocki]

On Friday, September 04, 2015 06:06:48 PM Marc Zyngier wrote:
> IRQ controllers and timers are the two types of device the kernel
> requires before being able to use the device driver model.
> 
> ACPI so far lacks a proper probing infrastructure similar to the one
> we have with DT, where we're able to declare IRQ chips and
> clocksources inside the driver code, and let the core code pick it up
> and call us back on a match. This leads to all kind of really ugly
> hacks all over the arm64 code and even in the ACPI layer.
> 
> In order to allow some basic probing based on the ACPI tables,
> introduce "struct acpi_probe_entry" which contains just enough
> data and callbacks to match a table, an optional subtable, and
> call a probe function. A driver can, at build time, register itself
> and expect being called if the right entry exists in the ACPI
> table.
> 
> A acpi_probe_device_init() is provided, taking an ACPI table
> identifier, and iterating over the registered entries.

What about things that are provided by the ACPI namespace (eg. via _MAT) rather
than in static tables?

Thanks,
Rafael

Re: [PATCH 0/5] ACPI probing infrastructure

[Marc Zyngier]

On 07/09/15 22:26, Rafael J. Wysocki wrote:
> On Friday, September 04, 2015 06:06:47 PM Marc Zyngier wrote:
>> IRQ controllers and timers are the two types of device the kernel
>> requires before being able to use the device driver model.
>>
>> ACPI so far lacks a proper probing infrastructure similar to the one
>> we have with DT, where we're able to declare IRQ chips and
>> clocksources inside the driver code, and let the core code pick it up
>> and call us back on a match. This leads to all kind of really ugly
>> hacks all over the arm64 code and even in the ACPI layer.
>>
>> It turns out that providing such a probing infrastructure is rather
>> easy, and provides a much deserved cleanup in both the arch code, the
>> GIC driver, and the architected timer driver.
> 
> Since I'm not familiar with the DT probing infrastructure mentioned above,
> can you please explain to me (possibly at a high level), how it is supposed
> to work in the ACPI case?

So let's start with DT. Each interrupt controller driver has at least
one entry like this:

IRQCHIP_DECLARE(gic_400, "arm,gic-400", gic_of_init);

which says: if you find a node having "arm,gic-400" as a compatible
string in the device tree, then call gic_of_init with this node as a
parameter. The probing itself is done by the OF layer when the
architecture code calls of_irq_init() (usually via irqchip_init).

This has a number of benefits:

- The irqchip code is self-contained. No architecture specific entry
point, no exposed symbols. Just a standard interface.

- The low-level architecture code doesn't have to know about which
interrupt controller is present. It just calls into the firmware
interface (of_irq_init) which is going to sort things out.

Similar infrastructure is provided for the timers/clock sources. Note
that this is not a replacement for the device model, but acts as a
probing infrastructure for things that are required too early for the
device infrastructure to be available.

What I'm aiming for is to introduce the same level of abstraction for
ACPI, or at least for the few bits that are required before a full blown
ACPI/device model can be used. For this, I introduce something  vaguely
similar:

IRQCHIP_ACPI_DECLARE(gic_v2, ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR,
		     gic_validate_dist, ACPI_MADT_GIC_VERSION_V2,
		     gic_v2_acpi_init);

which says: if you find a ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR entry in
MADT (implied by the macro), and that entry is of type
ACPI_MADT_GIC_VERSION_V2 (as checked by gic_validate_dist), then call
gic_v2_acpi_init with the entry as a parameter. A bit more convoluted,
but still without any special entry point.

The various interrupt controller drivers can then implement the above,
and the arch code can use a firmware-specific call to get the probing
done, still being oblivious of what interrupt controller is being used.
It also makes the adaptation of a DT driver to ACPI easier.

Does this help?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/5] acpi: Add basic device probing infrastructure

[Marc Zyngier]

On 07/09/15 22:29, Rafael J. Wysocki wrote:
> On Friday, September 04, 2015 06:06:48 PM Marc Zyngier wrote:
>> IRQ controllers and timers are the two types of device the kernel
>> requires before being able to use the device driver model.
>>
>> ACPI so far lacks a proper probing infrastructure similar to the one
>> we have with DT, where we're able to declare IRQ chips and
>> clocksources inside the driver code, and let the core code pick it up
>> and call us back on a match. This leads to all kind of really ugly
>> hacks all over the arm64 code and even in the ACPI layer.
>>
>> In order to allow some basic probing based on the ACPI tables,
>> introduce "struct acpi_probe_entry" which contains just enough
>> data and callbacks to match a table, an optional subtable, and
>> call a probe function. A driver can, at build time, register itself
>> and expect being called if the right entry exists in the ACPI
>> table.
>>
>> A acpi_probe_device_init() is provided, taking an ACPI table
>> identifier, and iterating over the registered entries.
> 
> What about things that are provided by the ACPI namespace (eg. via _MAT) rather
> than in static tables?

By the time we get to process non-static tables, the whole probing
infrastructure (including the ACPI interpreter) should be up and
running. I'm not seeing this stuff as a replacement for more dynamic
things - quite the opposite. It is only to be used for early bring-up.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/5] acpi: Add basic device probing infrastructure

[Hanjun Guo]

On 09/08/2015 05:57 PM, Marc Zyngier wrote:
> On 07/09/15 22:29, Rafael J. Wysocki wrote:
>> On Friday, September 04, 2015 06:06:48 PM Marc Zyngier wrote:
>>> IRQ controllers and timers are the two types of device the kernel
>>> requires before being able to use the device driver model.
>>>
>>> ACPI so far lacks a proper probing infrastructure similar to the one
>>> we have with DT, where we're able to declare IRQ chips and
>>> clocksources inside the driver code, and let the core code pick it up
>>> and call us back on a match. This leads to all kind of really ugly
>>> hacks all over the arm64 code and even in the ACPI layer.
>>>
>>> In order to allow some basic probing based on the ACPI tables,
>>> introduce "struct acpi_probe_entry" which contains just enough
>>> data and callbacks to match a table, an optional subtable, and
>>> call a probe function. A driver can, at build time, register itself
>>> and expect being called if the right entry exists in the ACPI
>>> table.
>>>
>>> A acpi_probe_device_init() is provided, taking an ACPI table
>>> identifier, and iterating over the registered entries.
>>
>> What about things that are provided by the ACPI namespace (eg. via _MAT) rather
>> than in static tables?
>
> By the time we get to process non-static tables, the whole probing
> infrastructure (including the ACPI interpreter) should be up and
> running. I'm not seeing this stuff as a replacement for more dynamic
> things - quite the opposite. It is only to be used for early bring-up.

Yes, this framework is for static tables and used at boot time,
sometimes quite early, which is before acpi_early_init().

But for _MAT (which is used for dynamic device configuration), it's
really a good question, I think _MAT is mainly for CPU hotplug, and
it's not related to this framework (for GIC init and clock source).
To hot add/remove a whole ARM SoC with _MAT, I think we need more
time to make the spec ready first, that's long term work, and agian
it's nothing to do with this infrastructure if I understand correctly :)

Thanks
Hanjun

Re: [PATCH 0/5] ACPI probing infrastructure

[Hanjun Guo]

Hi Marc,

Sorry for the late response for quite a while...

On 09/05/2015 01:06 AM, Marc Zyngier wrote:
> IRQ controllers and timers are the two types of device the kernel
> requires before being able to use the device driver model.
>
> ACPI so far lacks a proper probing infrastructure similar to the one
> we have with DT, where we're able to declare IRQ chips and
> clocksources inside the driver code, and let the core code pick it up
> and call us back on a match. This leads to all kind of really ugly
> hacks all over the arm64 code and even in the ACPI layer.
>
> It turns out that providing such a probing infrastructure is rather
> easy, and provides a much deserved cleanup in both the arch code, the
> GIC driver, and the architected timer driver.
>
> I'm sure there is some more code to be deleted, and one can only
> wonder why this wasn't done before the arm64 code was initially merged
> (the diffstat says it all...).
>
> Patches are against v4.2, and a branch is available at
>
> git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git acpi/device-probing

Great thanks to cleanup these stuff, I will test
this patch set and review it, will get back to you
if I get anything.

Thanks
Hanjun

Re: [PATCH 0/5] ACPI probing infrastructure

[Tomasz Nowicki]

On 07.09.2015 08:58, Tomasz Nowicki wrote:
> Hi Marc,
>
> On 04.09.2015 19:06, Marc Zyngier wrote:
>> IRQ controllers and timers are the two types of device the kernel
>> requires before being able to use the device driver model.
>>
>> ACPI so far lacks a proper probing infrastructure similar to the one
>> we have with DT, where we're able to declare IRQ chips and
>> clocksources inside the driver code, and let the core code pick it up
>> and call us back on a match. This leads to all kind of really ugly
>> hacks all over the arm64 code and even in the ACPI layer.
>>
>> It turns out that providing such a probing infrastructure is rather
>> easy, and provides a much deserved cleanup in both the arch code, the
>> GIC driver, and the architected timer driver.
>>
>> I'm sure there is some more code to be deleted, and one can only
>> wonder why this wasn't done before the arm64 code was initially merged
>> (the diffstat says it all...).
>>
>> Patches are against v4.2, and a branch is available at
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git
>> acpi/device-probing
>>
>> Marc Zyngier (5):
>>    acpi: Add basic device probing infrastructure
>>    irqchip/acpi: Add probing infrastructure for ACPI-based irqchips
>>    irqchip/gic: Convert the GIC driver to ACPI probing
>>    clocksource/acpi: Add probing infrastructure for ACPI-based
>>      clocksources
>>    clocksource/arm_arch_timer: Convert to ACPI probing
>>
>>   arch/arm64/include/asm/acpi.h        |  1 -
>>   arch/arm64/include/asm/irq.h         | 13 -------
>>   arch/arm64/kernel/acpi.c             | 25 -------------
>>   arch/arm64/kernel/time.c             |  6 ----
>>   drivers/acpi/scan.c                  | 41 +++++++++++++++++++++
>>   drivers/clocksource/arm_arch_timer.c | 10 +-----
>>   drivers/clocksource/clksrc-of.c      |  4 +++
>>   drivers/irqchip/irq-gic.c            | 69
>> ++++++++++++++++++------------------
>>   drivers/irqchip/irqchip.c            |  5 ++-
>>   include/asm-generic/vmlinux.lds.h    | 11 ++++++
>>   include/linux/acpi.h                 | 56 +++++++++++++++++++++++++++++
>>   include/linux/acpi_irq.h             | 10 ------
>>   include/linux/clocksource.h          |  6 ----
>>   include/linux/irqchip.h              | 16 +++++++++
>>   include/linux/irqchip/arm-gic-acpi.h | 31 ----------------
>>   15 files changed, 166 insertions(+), 138 deletions(-)
>>   delete mode 100644 include/linux/acpi_irq.h
>>   delete mode 100644 include/linux/irqchip/arm-gic-acpi.h
>>
>
> Thanks for cleaning this up!
>
> For the whole series:
> Acked-by: Tomasz Nowicki <tn@semihalf.com>

s/Acked-by/Reviewed-by

Tomasz

Re: [PATCH 0/5] ACPI probing infrastructure

[Rafael J. Wysocki]

On Tuesday, September 08, 2015 10:45:58 AM Marc Zyngier wrote:
> On 07/09/15 22:26, Rafael J. Wysocki wrote:
> > On Friday, September 04, 2015 06:06:47 PM Marc Zyngier wrote:
> >> IRQ controllers and timers are the two types of device the kernel
> >> requires before being able to use the device driver model.
> >>
> >> ACPI so far lacks a proper probing infrastructure similar to the one
> >> we have with DT, where we're able to declare IRQ chips and
> >> clocksources inside the driver code, and let the core code pick it up
> >> and call us back on a match. This leads to all kind of really ugly
> >> hacks all over the arm64 code and even in the ACPI layer.
> >>
> >> It turns out that providing such a probing infrastructure is rather
> >> easy, and provides a much deserved cleanup in both the arch code, the
> >> GIC driver, and the architected timer driver.
> > 
> > Since I'm not familiar with the DT probing infrastructure mentioned above,
> > can you please explain to me (possibly at a high level), how it is supposed
> > to work in the ACPI case?
> 
> So let's start with DT. Each interrupt controller driver has at least
> one entry like this:
> 
> IRQCHIP_DECLARE(gic_400, "arm,gic-400", gic_of_init);
> 
> which says: if you find a node having "arm,gic-400" as a compatible
> string in the device tree, then call gic_of_init with this node as a
> parameter. The probing itself is done by the OF layer when the
> architecture code calls of_irq_init() (usually via irqchip_init).
> 
> This has a number of benefits:
> 
> - The irqchip code is self-contained. No architecture specific entry
> point, no exposed symbols. Just a standard interface.
> 
> - The low-level architecture code doesn't have to know about which
> interrupt controller is present. It just calls into the firmware
> interface (of_irq_init) which is going to sort things out.
> 
> Similar infrastructure is provided for the timers/clock sources. Note
> that this is not a replacement for the device model, but acts as a
> probing infrastructure for things that are required too early for the
> device infrastructure to be available.
> 
> What I'm aiming for is to introduce the same level of abstraction for
> ACPI, or at least for the few bits that are required before a full blown
> ACPI/device model can be used. For this, I introduce something  vaguely
> similar:
> 
> IRQCHIP_ACPI_DECLARE(gic_v2, ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR,
> 		     gic_validate_dist, ACPI_MADT_GIC_VERSION_V2,
> 		     gic_v2_acpi_init);
> 
> which says: if you find a ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR entry in
> MADT (implied by the macro), and that entry is of type
> ACPI_MADT_GIC_VERSION_V2 (as checked by gic_validate_dist), then call
> gic_v2_acpi_init with the entry as a parameter. A bit more convoluted,
> but still without any special entry point.
> 
> The various interrupt controller drivers can then implement the above,
> and the arch code can use a firmware-specific call to get the probing
> done, still being oblivious of what interrupt controller is being used.
> It also makes the adaptation of a DT driver to ACPI easier.
> 
> Does this help?

Yes it does, thanks!

Rafael

Re: [PATCH 0/5] ACPI probing infrastructure

[Hanjun Guo]

On 09/08/2015 09:19 PM, Hanjun Guo wrote:
> Hi Marc,
>
> Sorry for the late response for quite a while...
>
> On 09/05/2015 01:06 AM, Marc Zyngier wrote:
>> IRQ controllers and timers are the two types of device the kernel
>> requires before being able to use the device driver model.
>>
>> ACPI so far lacks a proper probing infrastructure similar to the one
>> we have with DT, where we're able to declare IRQ chips and
>> clocksources inside the driver code, and let the core code pick it up
>> and call us back on a match. This leads to all kind of really ugly
>> hacks all over the arm64 code and even in the ACPI layer.
>>
>> It turns out that providing such a probing infrastructure is rather
>> easy, and provides a much deserved cleanup in both the arch code, the
>> GIC driver, and the architected timer driver.
>>
>> I'm sure there is some more code to be deleted, and one can only
>> wonder why this wasn't done before the arm64 code was initially merged
>> (the diffstat says it all...).
>>
>> Patches are against v4.2, and a branch is available at
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git
>> acpi/device-probing

I pulled, and noticed that there is v2, it's cleaner than v1 for the
patch 1/5 to me :)

>
> Great thanks to cleanup these stuff, I will test
> this patch set and review it, will get back to you
> if I get anything.

I tested the v2 and it boots pretty happy as before, if
you post to maillist,

Tested-by: Hanjun Guo <hanjun.guo@linaro.org>
Reviewed-by: Hanjun Guo <hanjun.guo@linaro.org>

Thanks
Hanjun

Re: [PATCH 0/5] ACPI probing infrastructure

[Marc Zyngier]

On 10/09/15 10:03, Hanjun Guo wrote:
> On 09/08/2015 09:19 PM, Hanjun Guo wrote:
>> Hi Marc,
>>
>> Sorry for the late response for quite a while...
>>
>> On 09/05/2015 01:06 AM, Marc Zyngier wrote:
>>> IRQ controllers and timers are the two types of device the kernel
>>> requires before being able to use the device driver model.
>>>
>>> ACPI so far lacks a proper probing infrastructure similar to the one
>>> we have with DT, where we're able to declare IRQ chips and
>>> clocksources inside the driver code, and let the core code pick it up
>>> and call us back on a match. This leads to all kind of really ugly
>>> hacks all over the arm64 code and even in the ACPI layer.
>>>
>>> It turns out that providing such a probing infrastructure is rather
>>> easy, and provides a much deserved cleanup in both the arch code, the
>>> GIC driver, and the architected timer driver.
>>>
>>> I'm sure there is some more code to be deleted, and one can only
>>> wonder why this wasn't done before the arm64 code was initially merged
>>> (the diffstat says it all...).
>>>
>>> Patches are against v4.2, and a branch is available at
>>>
>>> git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git
>>> acpi/device-probing
> 
> I pulled, and noticed that there is v2, it's cleaner than v1 for the
> patch 1/5 to me :)
> 
>>
>> Great thanks to cleanup these stuff, I will test
>> this patch set and review it, will get back to you
>> if I get anything.
> 
> I tested the v2 and it boots pretty happy as before, if
> you post to maillist,
> 
> Tested-by: Hanjun Guo <hanjun.guo@linaro.org>
> Reviewed-by: Hanjun Guo <hanjun.guo@linaro.org>

I'm still working on v2, so I'm afraid this is still a bit premature.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...