Re: [PATCH v4 01/10] irqchip / GIC: Add GIC version support in ACPI MADT

[Hanjun Guo <hanjun.guo@linaro.org>]

On 08/05/2015 08:57 PM, Marc Zyngier wrote:
> On 05/08/15 13:40, Hanjun Guo wrote:
>> On 08/04/2015 08:06 PM, Marc Zyngier wrote:
>>> On 29/07/15 11:08, Hanjun Guo wrote:
>>>> There is a field added in ACPI 6.0 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       | 109 +++++++++++++++++++++++++++++++++++
>>>>    include/linux/irqchip/arm-gic-acpi.h |   1 +
>>>>    5 files changed, 115 insertions(+)
>>>>    create mode 100644 drivers/irqchip/irq-gic-acpi.c
>>>>
>>>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>>>> index 318175f..f2ff61f 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 120d815..557ec2f 100644
>>>> --- a/drivers/irqchip/Kconfig
>>>> +++ b/drivers/irqchip/Kconfig
>>>> @@ -47,6 +47,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 11d08c9..383f421 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 irq-gic-v3-its-pci-msi.o irq-gic-v3-its-platform-msi.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..6537b43
>>>> --- /dev/null
>>>> +++ b/drivers/irqchip/irq-gic-acpi.c
>>>> @@ -0,0 +1,109 @@
>>>> +/*
>>>> + * 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);
>>>> +
>>>> +	/* that's true if we have at least one GIC redistributor entry */
>>>> +	return count > 0;
>>>> +}
>>>> +
>>>> +static int __init acpi_gic_version_init(void)
>>>> +{
>>>> +	int count;
>>>> +	u32 reg;
>>>> +	void __iomem *dist_base;
>>>> +
>>>> +	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;
>>>
>>> Frankly, why should we care? If there is no redistributor, this is a V2.
>>> If there are redistributors, then it is a V3/V4, and it shouldn't matter
>>> which one this is as the GICv3 driver can find out all by itself. Also,
>>> the GICv4 feature only matter for KVM, which can probe this on its own.
>>
>> For hardware discovery, it works.
>>
>> I use the gic_version in later patch to ioremap physical base address of
>> GICR (which 2 or 4 64k pages), but I can get it from register in later
>> patch then remove the code above.
>>
>> For gic version in GICD subtable, we must consider V4 as firmware may
>> just present ACPI_MADT_GIC_VERSION_V4 as the GIC version, that's why
>> we need to match V4 in the GICv3 driver.
>
> My point is: the GICv3 driver doesn't give a damn. At all. And it
> shouldn't! The *only* difference is the size of the redistributor, and
> this can be worked out *inside the GIC driver* by looking at:
>
> - GICR_PIDR2.ArchRev (and not GICD_PIDR2)
> - GICR_TYPER.VLPIS
>
> Whatever the firmware says is absolutely irrelevant, and even if we
> needed to override what the HW reports (because it is broken), we can
> implement it as a quirk inside the driver itself, without any of this
> ioremap dance that doesn't even do the right thing.
>
> So to "we must consider V4", my answer is no. Certainly not at this
> location.

Sorry, I didn't make it clear. what I mean is that when GIC version
is presented as V4, we can match the GICv3 driver with it, and
everything will be the same in the driver itself.

Thanks
Hanjun