diff mbox

[v2,3/9] irqchip / GIC: Add GIC version support in ACPI MADT

Message ID 1434703572-26221-4-git-send-email-hanjun.guo@linaro.org
State New
Headers show

Commit Message

Hanjun Guo June 19, 2015, 8:46 a.m. UTC 
There is a field added in ACPI MADT table to indicate the
GIC version, so parse the table to get its value for later
use.

If GIC version presented in MADT is 0, we need to fallback
to hardware discovery to get the GIC version.

In ACPI MADT table there is no compatible strings to indicate
various irqchips and also ACPI doesn't support irqchips which
are not compatible with ARM GIC spec, so GIC version can be used
to load different GIC drivers which is needed for the later patch.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 arch/arm64/Kconfig                   |   1 +
 drivers/irqchip/Kconfig              |   3 +
 drivers/irqchip/Makefile             |   1 +
 drivers/irqchip/irq-gic-acpi.c       | 112 +++++++++++++++++++++++++++++++++++
 include/linux/irqchip/arm-gic-acpi.h |   1 +
 5 files changed, 118 insertions(+)
 create mode 100644 drivers/irqchip/irq-gic-acpi.c

Comments

Hanjun Guo June 23, 2015, 11:18 a.m. UTC | #1 
On 06/23/2015 12:45 AM, Lorenzo Pieralisi wrote:
> On Fri, Jun 19, 2015 at 09:46:06AM +0100, Hanjun Guo wrote:
>
> [...]
>
>> +
>> +static int __init
>> +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;
>> +
>> +	/* scan MADT table to find if we have redistributor entries */
>> +	count  =  acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR,
>> +					match_gic_redist, 0);
>> +
>> +	/* has at least one GIC redistributor entry */
>> +	if (count > 0)
>> +		return true;
>> +	else
>> +		return false;
>> +}
>
> return count > 0;
>
> What about systems where the redistributor data is in the GICC subtable ? Do
> you treat them as GIC V2 :) ?

Good catch :)

It's in my TODO list and I didn't put some notes here, my bad.

If we want to support that feature, it seems we need to refactor
the GICv3 code for rdist too, in gic_populate_rdist(), it use
redistributor regions and jump to each cpu's redistributor using
the stride, but with the redistributor data is in the GICC subtable,
we only have the base address for each cpu's redistributor, have
no regions.

Another way to do that we can repack all the base address for
redistributors into regions then no need to refactor the code,
but all the CPUs in MADT have to be listed in order which is
insane.

>
> On a side note, having to define an empty function like match_gic_redist is
> horrible.
>
> I wonder whether it is not better to refactor map_madt_entry(), create
> a MADT subtable iterator out of it and make that code generic, instead
> of being forced to add these useless MADT handlers just to count
> entries, it is not the first I noticed.

I agree, I also think that parse GICC for each driver (SMP, GIC) is
duplicate, trying to consolidate them, but I haven't figure out a
good way to do that.

>
>> +static int __init acpi_gic_version_init(void)
>> +{
>> +	int count;
>> +	void __iomem *dist_base;
>> +	u32 reg;
>> +
>> +	count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR,
>> +					acpi_gic_parse_distributor, 0);
>> +
>> +	if (count <= 0) {
>> +		pr_err("No valid GIC distributor entry exists\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (gic_version >= ACPI_MADT_GIC_VERSION_RESERVED) {
>> +		pr_err("Invalid GIC version %d in MADT\n", gic_version);
>> +		return -EINVAL;
>> +	}
>> +
>> +	/*
>> +	 * when the GIC version is 0, we fallback to hardware discovery.
>> +	 * this is also needed to keep compatiable with ACPI 5.1,
>> +	 * which has no gic_version field in distributor structure and
>> +	 * reserved as 0.
>> +	 *
>> +	 * For hardware discovery, the offset for GICv1/2 and GICv3/4 to
>> +	 * get the GIC version is different (0xFE8 for GICv1/2 and 0xFFE8
>> +	 * for GICv3/4), so we need to handle it separately.
>
>> +	 */
>> +	if (gic_version	== ACPI_MADT_GIC_VERSION_NONE) {
>> +		/* it's GICv3/v4 if redistributor is present */
>> +		if (acpi_gic_redist_is_present()) {
>
> See above, IIUC you might have systems with GIC v3 where this call
> would fail, and we do not want to fall back to GIC v2.

Yes, but we need to find a way to refactor the GICv3 code for
populating redistributors first.

Thanks
Hanjun
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hanjun Guo June 27, 2015, 6:07 a.m. UTC | #2 
On 06/23/2015 12:45 AM, Lorenzo Pieralisi wrote:
> On Fri, Jun 19, 2015 at 09:46:06AM +0100, Hanjun Guo wrote:
>
> [...]
>
>> +
>> +static int __init
>> +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;
>> +
>> +	/* scan MADT table to find if we have redistributor entries */
>> +	count  =  acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR,
>> +					match_gic_redist, 0);
>> +
>> +	/* has at least one GIC redistributor entry */
>> +	if (count > 0)
>> +		return true;
>> +	else
>> +		return false;
>> +}
>
> return count > 0;
>
> What about systems where the redistributor data is in the GICC subtable ? Do
> you treat them as GIC V2 :) ?
>
> On a side note, having to define an empty function like match_gic_redist is
> horrible.
>
> I wonder whether it is not better to refactor map_madt_entry(), create
> a MADT subtable iterator out of it and make that code generic, instead
> of being forced to add these useless MADT handlers just to count
> entries, it is not the first I noticed.

After digging into the code, it seems that we need more discussion for
this comment, you suggested that refactor map_madt_entry() and create
a MADT subtable iterator out of it, but we still need a handler
to handle each subtable entry, right? then it will become another
version of acpi_parse_entries() in drivers/acpi/table.c, and no
improvement to code, did I miss something?

Thanks
Hanjun
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index c273e4a..9d62bbb 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -16,6 +16,7 @@  config ARM64
 	select ARM_AMBA
 	select ARM_ARCH_TIMER
 	select ARM_GIC
+	select ARM_GIC_ACPI if ACPI
 	select AUDIT_ARCH_COMPAT_GENERIC
 	select ARM_GIC_V2M if PCI_MSI
 	select ARM_GIC_V3
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 6de62a9..0dd64c5 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -46,6 +46,9 @@  config ARM_VIC_NR
 	  The maximum number of VICs available in the system, for
 	  power management.
 
+config ARM_GIC_ACPI
+	bool
+
 config ATMEL_AIC_IRQ
 	bool
 	select GENERIC_IRQ_CHIP
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index dda4927..0bd8e49 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -23,6 +23,7 @@  obj-$(CONFIG_ARM_GIC)			+= irq-gic.o irq-gic-common.o
 obj-$(CONFIG_ARM_GIC_V2M)		+= irq-gic-v2m.o
 obj-$(CONFIG_ARM_GIC_V3)		+= irq-gic-v3.o irq-gic-common.o
 obj-$(CONFIG_ARM_GIC_V3_ITS)		+= irq-gic-v3-its.o
+obj-$(CONFIG_ARM_GIC_ACPI)		+= irq-gic-acpi.o
 obj-$(CONFIG_ARM_NVIC)			+= irq-nvic.o
 obj-$(CONFIG_ARM_VIC)			+= irq-vic.o
 obj-$(CONFIG_ATMEL_AIC_IRQ)		+= irq-atmel-aic-common.o irq-atmel-aic.o
diff --git a/drivers/irqchip/irq-gic-acpi.c b/drivers/irqchip/irq-gic-acpi.c
new file mode 100644
index 0000000..58f7831
--- /dev/null
+++ b/drivers/irqchip/irq-gic-acpi.c
@@ -0,0 +1,112 @@ 
+/*
+ * ACPI based support for ARM GIC init
+ *
+ * Copyright (C) 2015, Linaro Ltd.
+ * 	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.
+ */
+
+#define pr_fmt(fmt) "ACPI: GIC: " fmt
+
+#include <linux/acpi.h>
+#include <linux/init.h>
+#include <linux/irqchip/arm-gic-acpi.h>
+#include <linux/irqchip/arm-gic-v3.h>
+
+/* GIC version presented in MADT GIC distributor structure */
+static u8 gic_version __initdata = ACPI_MADT_GIC_VERSION_NONE;
+
+static phys_addr_t dist_phy_base __initdata;
+
+static int __init
+acpi_gic_parse_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;
+
+	gic_version = dist->version;
+	dist_phy_base = dist->base_address;
+	return 0;
+}
+
+static int __init
+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;
+
+	/* scan MADT table to find if we have redistributor entries */
+	count  =  acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR,
+					match_gic_redist, 0);
+
+	/* has at least one GIC redistributor entry */
+	if (count > 0)
+		return true;
+	else
+		return false;
+}
+
+static int __init acpi_gic_version_init(void)
+{
+	int count;
+	void __iomem *dist_base;
+	u32 reg;
+
+	count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR,
+					acpi_gic_parse_distributor, 0);
+
+	if (count <= 0) {
+		pr_err("No valid GIC distributor entry exists\n");
+		return -ENODEV;
+	}
+
+	if (gic_version >= ACPI_MADT_GIC_VERSION_RESERVED) {
+		pr_err("Invalid GIC version %d in MADT\n", gic_version);
+		return -EINVAL;
+	}
+
+	/*
+	 * when the GIC version is 0, we fallback to hardware discovery.
+	 * this is also needed to keep compatiable with ACPI 5.1,
+	 * which has no gic_version field in distributor structure and
+	 * reserved as 0.
+	 *
+	 * For hardware discovery, the offset for GICv1/2 and GICv3/4 to
+	 * get the GIC version is different (0xFE8 for GICv1/2 and 0xFFE8
+	 * for GICv3/4), so we need to handle it separately.
+	 */
+	if (gic_version	== ACPI_MADT_GIC_VERSION_NONE) {
+		/* it's GICv3/v4 if redistributor is present */
+		if (acpi_gic_redist_is_present()) {
+			dist_base = ioremap(dist_phy_base,
+					    ACPI_GICV3_DIST_MEM_SIZE);
+			if (!dist_base)
+				return -ENOMEM;
+
+			reg = readl_relaxed(dist_base + GICD_PIDR2) &
+					    GIC_PIDR2_ARCH_MASK;
+			if (reg == GIC_PIDR2_ARCH_GICv3)
+				gic_version = ACPI_MADT_GIC_VERSION_V3;
+			else
+				gic_version = ACPI_MADT_GIC_VERSION_V4;
+
+			iounmap(dist_base);
+		} else {
+			gic_version = ACPI_MADT_GIC_VERSION_V2;
+		}
+	}
+
+	return 0;
+}
diff --git a/include/linux/irqchip/arm-gic-acpi.h b/include/linux/irqchip/arm-gic-acpi.h
index de3419e..13bc676 100644
--- a/include/linux/irqchip/arm-gic-acpi.h
+++ b/include/linux/irqchip/arm-gic-acpi.h
@@ -19,6 +19,7 @@ 
  */
 #define ACPI_GICV2_DIST_MEM_SIZE	(SZ_4K)
 #define ACPI_GIC_CPU_IF_MEM_SIZE	(SZ_8K)
+#define ACPI_GICV3_DIST_MEM_SIZE	(SZ_64K)
 
 struct acpi_table_header;