diff mbox

[v3,13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support

Message ID 1409583475-6978-14-git-send-email-hanjun.guo@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Hanjun Guo Sept. 1, 2014, 2:57 p.m. UTC
From: Tomasz Nowicki <tomasz.nowicki@linaro.org>

ACPI kernel uses MADT table for proper GIC initialization. It needs to
parse GIC related subtables, collect CPU interface and distributor
addresses and call driver initialization function (which is hardware
abstraction agnostic). In a similar way, FDT initialize GICv1/2.

NOTE: This commit allow to initialize GICv1/2 only.

Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 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

Comments

Marc Zyngier Sept. 1, 2014, 5:35 p.m. UTC | #1
On 01/09/14 15:57, Hanjun Guo wrote:
> From: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> 
> ACPI kernel uses MADT table for proper GIC initialization. It needs to
> parse GIC related subtables, collect CPU interface and distributor
> addresses and call driver initialization function (which is hardware
> 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.

> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> ---
>  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/asm/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 <linux/irqdomain.h>
>  #include <linux/bootmem.h>
>  #include <linux/smp.h>
> +#include <linux/irqchip/arm-gic-acpi.h>
>  
>  #include <asm/cputype.h>
>  #include <asm/cpu_ops.h>
> @@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
>  		pr_err("Can't find FADT or error happened during parsing FADT\n");
>  }
>  
> +void __init acpi_gic_init(void)
> +{
> +	struct acpi_table_header *table;
> +	acpi_status status;
> +	acpi_size tbl_size;
> +	int err;
> +
> +	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");

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?

> +
> +	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 <linux/irqchip.h>
>  #include <linux/seq_file.h>
>  #include <linux/ratelimit.h>
> +#include <linux/irqchip/arm-gic-acpi.h>
>  
>  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 logical
spot to probe an interrupt controller.

>  	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 <linux/of.h>
>  #include <linux/of_address.h>
>  #include <linux/of_irq.h>
> +#include <linux/acpi.h>
>  #include <linux/irqdomain.h>
>  #include <linux/interrupt.h>
>  #include <linux/percpu.h>
>  #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>
> @@ -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 = ULONG_MAX;

Please use phys_addr_t for physical addresses. The use of ULONG_MAX
looks dodgy. Please have a proper symbol to flag the fact that it hasn't
been assigned yet.

> +
> +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 = (struct acpi_madt_generic_interrupt *)header;
> +
> +	if (BAD_MADT_ENTRY(processor, end))
> +		return -EINVAL;
> +
> +	gic_cpu_base = processor->base_address;
> +	if (!gic_cpu_base)
> +		return -EFAULT;

Is zero an invalid address?

> +
> +	/*
> +	 * There is no support for non-banked GICv1/2 register in ACPI spec.
> +	 * All CPU interface addresses have to be the same.
> +	 */
> +	if (cpu_phy_base != ULONG_MAX && gic_cpu_base != cpu_phy_base)
> +		return -EFAULT;
> +
> +	cpu_phy_base = gic_cpu_base;
> +	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;
> +	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 = acpi_parse_entries(sizeof(struct acpi_table_madt),
> +				   gic_acpi_parse_madt_cpu, table,
> +				   ACPI_MADT_TYPE_GENERIC_INTERRUPT,
> +				   ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES);
> +	if (count < 0) {
> +		pr_err("Error during GICC entries parsing\n");
> +		return -EFAULT;
> +	} else if (!count) {
> +		/* No GICC entries provided, use address from MADT header */
> +		struct acpi_table_madt *madt = (struct acpi_table_madt *)table;
> +
> +		if (!madt->address)
> +			return -EFAULT;
> +
> +		cpu_phy_base = (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 = acpi_parse_entries(sizeof(struct acpi_table_madt),
> +				   gic_acpi_parse_madt_distributor, table,
> +				   ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR,
> +				   ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES);
> +	if (count <= 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 = 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_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 GSI 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 <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_
> +
> +#include <linux/acpi.h>

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.

> +
> +#ifdef CONFIG_ACPI
> +#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES	65535

With GICv2? I doubt it.

> +#define ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES	1
> +
> +/*
> + * 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_GIC_DIST_MEM_SIZE		(SZ_64K)

I don't know which version of the spec you're looking at, but my version
of the GICv2 spec has a 4kB distributor. Also, it would be good to make
obvious which GIC version this define is about.

> +#define ACPI_GIC_CPU_IF_MEM_SIZE	(SZ_8K)
> +
> +void acpi_gic_init(void);
> +int gic_v2_acpi_init(struct acpi_table_header *table);
> +#else
> +static inline void acpi_gic_init(void) { }
> +#endif
> +
> +#endif /* ARM_GIC_ACPI_H_ */
> 

In the end, why do we need a separate file for this? I cannot see
anything that prevents it from being merged with arm-gic.h.

Thanks,

	M.
Alexander Spyridakis Sept. 2, 2014, 8:28 a.m. UTC | #2
On 1 September 2014 19:35, Marc Zyngier <marc.zyngier@arm.com> wrote:
>
> On 01/09/14 15:57, Hanjun Guo wrote:
> > From: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> >
> > ACPI kernel uses MADT table for proper GIC initialization. It needs to
> > parse GIC related subtables, collect CPU interface and distributor
> > addresses and call driver initialization function (which is hardware
> > 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.

Just a heads up for this, I run up to this problem too while testing
and by forcing some values for vgic and arch_timers I could boot a
QEMU guest without problems.

I'll try and see if it will be easy to handle this more properly
instead of hardcoded values.

Regards.
Tomasz Nowicki Sept. 2, 2014, 11:48 a.m. UTC | #3
On 01.09.2014 19:35, Marc Zyngier wrote:
> On 01/09/14 15:57, Hanjun Guo wrote:
>> From: Tomasz Nowicki <tomasz.nowicki@linaro.org>
>>
>> ACPI kernel uses MADT table for proper GIC initialization. It needs to
>> parse GIC related subtables, collect CPU interface and distributor
>> addresses and call driver initialization function (which is hardware
>> 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 <tomasz.nowicki@linaro.org>
>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>> ---
>>   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/asm/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 <linux/irqdomain.h>
>>   #include <linux/bootmem.h>
>>   #include <linux/smp.h>
>> +#include <linux/irqchip/arm-gic-acpi.h>
>>
>>   #include <asm/cputype.h>
>>   #include <asm/cpu_ops.h>
>> @@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
>>   		pr_err("Can't find FADT or error happened during parsing FADT\n");
>>   }
>>
>> +void __init acpi_gic_init(void)
>> +{
>> +	struct acpi_table_header *table;
>> +	acpi_status status;
>> +	acpi_size tbl_size;
>> +	int err;
>> +
>> +	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");
>
> 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 = gic_v3_acpi_init(table);
	if (err)
		err = 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 <linux/irqchip.h>
>>   #include <linux/seq_file.h>
>>   #include <linux/ratelimit.h>
>> +#include <linux/irqchip/arm-gic-acpi.h>
>>
>>   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 logical
> spot to probe an interrupt controller.

irqchip.c is OF dependent, I want to decouple these from the very 
beginning.

>
>>   	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 <linux/of.h>
>>   #include <linux/of_address.h>
>>   #include <linux/of_irq.h>
>> +#include <linux/acpi.h>
>>   #include <linux/irqdomain.h>
>>   #include <linux/interrupt.h>
>>   #include <linux/percpu.h>
>>   #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>
>> @@ -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 = ULONG_MAX;
>
> Please use phys_addr_t for physical addresses. The use of ULONG_MAX
> 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 = (struct acpi_madt_generic_interrupt *)header;
>> +
>> +	if (BAD_MADT_ENTRY(processor, end))
>> +		return -EINVAL;
>> +
>> +	gic_cpu_base = 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 ACPI spec.
>> +	 * All CPU interface addresses have to be the same.
>> +	 */
>> +	if (cpu_phy_base != ULONG_MAX && gic_cpu_base != cpu_phy_base)
>> +		return -EFAULT;
>> +
>> +	cpu_phy_base = gic_cpu_base;
>> +	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;
>> +	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 = acpi_parse_entries(sizeof(struct acpi_table_madt),
>> +				   gic_acpi_parse_madt_cpu, table,
>> +				   ACPI_MADT_TYPE_GENERIC_INTERRUPT,
>> +				   ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES);
>> +	if (count < 0) {
>> +		pr_err("Error during GICC entries parsing\n");
>> +		return -EFAULT;
>> +	} else if (!count) {
>> +		/* No GICC entries provided, use address from MADT header */
>> +		struct acpi_table_madt *madt = (struct acpi_table_madt *)table;
>> +
>> +		if (!madt->address)
>> +			return -EFAULT;
>> +
>> +		cpu_phy_base = (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 = acpi_parse_entries(sizeof(struct acpi_table_madt),
>> +				   gic_acpi_parse_madt_distributor, table,
>> +				   ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR,
>> +				   ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES);
>> +	if (count <= 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 = 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_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 GSI 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 <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_
>> +
>> +#include <linux/acpi.h>
>
> 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 
<linux/acpi.h>. 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

>
>> +#define ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES	1
>> +
>> +/*
>> + * 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_GIC_DIST_MEM_SIZE		(SZ_64K)
>
> I don't know which version of the spec you're looking at, but my version
> of the GICv2 spec has a 4kB distributor. Also, it would be good to make
> obvious which GIC version this define is about.
OK

>
>> +#define ACPI_GIC_CPU_IF_MEM_SIZE	(SZ_8K)
>> +
>> +void acpi_gic_init(void);
>> +int gic_v2_acpi_init(struct acpi_table_header *table);
>> +#else
>> +static inline void acpi_gic_init(void) { }
>> +#endif
>> +
>> +#endif /* ARM_GIC_ACPI_H_ */
>>
>
> In the end, why do we need a separate file for this? I cannot see
> anything that prevents it from being merged with arm-gic.h.
>
> Thanks,
>
> 	M.
>
Having only GICv2, it would work. Considering we would do the same for 
GICv3 (arm-gic-v3.h) there will be register name conflicts for both 
headers inclusion:

[...]
#include <linux/irqchip/arm-gic.h>
#include <linux/irqchip/arm-gic-v3.h>
[...]
	err = gic_v3_acpi_init(table);
	if (err)
		err = gic_v2_acpi_init(table);
	if (err)
		pr_err("Failed to initialize GIC IRQ controller");
[...]
So instead of changing register names prefix, I choose new header will 
be less painfully.

Regards,
Tomasz
Marc Zyngier Sept. 2, 2014, 1:02 p.m. UTC | #4
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 <tomasz.nowicki@linaro.org>
>>>
>>> ACPI kernel uses MADT table for proper GIC initialization. It needs to
>>> parse GIC related subtables, collect CPU interface and distributor
>>> addresses and call driver initialization function (which is hardware
>>> 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 <tomasz.nowicki@linaro.org>
>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>> ---
>>>   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/asm/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 <linux/irqdomain.h>
>>>   #include <linux/bootmem.h>
>>>   #include <linux/smp.h>
>>> +#include <linux/irqchip/arm-gic-acpi.h>
>>>
>>>   #include <asm/cputype.h>
>>>   #include <asm/cpu_ops.h>
>>> @@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
>>>              pr_err("Can't find FADT or error happened during parsing FADT\n");
>>>   }
>>>
>>> +void __init acpi_gic_init(void)
>>> +{
>>> +    struct acpi_table_header *table;
>>> +    acpi_status status;
>>> +    acpi_size tbl_size;
>>> +    int err;
>>> +
>>> +    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");
>>
>> 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 = gic_v3_acpi_init(table);
>         if (err)
>                 err = 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 <linux/irqchip.h>
>>>   #include <linux/seq_file.h>
>>>   #include <linux/ratelimit.h>
>>> +#include <linux/irqchip/arm-gic-acpi.h>
>>>
>>>   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 logical
>> 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 thing
we support so far. I don't think duplicating the kernel infrastructure
"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). Just
put in place a similar probing mechanism, and this will look a lot better.

>>
>>>      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 <linux/of.h>
>>>   #include <linux/of_address.h>
>>>   #include <linux/of_irq.h>
>>> +#include <linux/acpi.h>
>>>   #include <linux/irqdomain.h>
>>>   #include <linux/interrupt.h>
>>>   #include <linux/percpu.h>
>>>   #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>
>>> @@ -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 = ULONG_MAX;
>>
>> Please use phys_addr_t for physical addresses. The use of ULONG_MAX
>> 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 = (struct acpi_madt_generic_interrupt *)header;
>>> +
>>> +    if (BAD_MADT_ENTRY(processor, end))
>>> +            return -EINVAL;
>>> +
>>> +    gic_cpu_base = 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 ACPI spec.
>>> +     * All CPU interface addresses have to be the same.
>>> +     */
>>> +    if (cpu_phy_base != ULONG_MAX && gic_cpu_base != cpu_phy_base)
>>> +            return -EFAULT;
>>> +
>>> +    cpu_phy_base = gic_cpu_base;
>>> +    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;
>>> +    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 = acpi_parse_entries(sizeof(struct acpi_table_madt),
>>> +                               gic_acpi_parse_madt_cpu, table,
>>> +                               ACPI_MADT_TYPE_GENERIC_INTERRUPT,
>>> +                               ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES);
>>> +    if (count < 0) {
>>> +            pr_err("Error during GICC entries parsing\n");
>>> +            return -EFAULT;
>>> +    } else if (!count) {
>>> +            /* No GICC entries provided, use address from MADT header */
>>> +            struct acpi_table_madt *madt = (struct acpi_table_madt *)table;
>>> +
>>> +            if (!madt->address)
>>> +                    return -EFAULT;
>>> +
>>> +            cpu_phy_base = (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 = acpi_parse_entries(sizeof(struct acpi_table_madt),
>>> +                               gic_acpi_parse_madt_distributor, table,
>>> +                               ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR,
>>> +                               ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES);
>>> +    if (count <= 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 = 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_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 GSI 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 <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_
>>> +
>>> +#include <linux/acpi.h>
>>
>> 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
> <linux/acpi.h>. 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) from?

>>
>>> +#define ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES    1
>>> +
>>> +/*
>>> + * 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_GIC_DIST_MEM_SIZE              (SZ_64K)
>>
>> I don't know which version of the spec you're looking at, but my version
>> of the GICv2 spec has a 4kB distributor. Also, it would be good to make
>> obvious which GIC version this define is about.
> OK
> 
>>
>>> +#define ACPI_GIC_CPU_IF_MEM_SIZE    (SZ_8K)
>>> +
>>> +void acpi_gic_init(void);
>>> +int gic_v2_acpi_init(struct acpi_table_header *table);
>>> +#else
>>> +static inline void acpi_gic_init(void) { }
>>> +#endif
>>> +
>>> +#endif /* ARM_GIC_ACPI_H_ */
>>>
>>
>> In the end, why do we need a separate file for this? I cannot see
>> anything that prevents it from being merged with arm-gic.h.
>>
>> Thanks,
>>
>>       M.
>>
> Having only GICv2, it would work. Considering we would do the same for
> GICv3 (arm-gic-v3.h) there will be register name conflicts for both
> headers inclusion:
> 
> [...]
> #include <linux/irqchip/arm-gic.h>
> #include <linux/irqchip/arm-gic-v3.h>
> [...]
>         err = gic_v3_acpi_init(table);
>         if (err)
>                 err = gic_v2_acpi_init(table);
>         if (err)
>                 pr_err("Failed to initialize GIC IRQ controller");
> [...]
> So instead of changing register names prefix, I choose new header will
> be less painfully.

Yes, and this is exactly why I pushed back on that last time. I'll
continue saying that interrupt controllers should be self-probing, with
ACPI as they are with DT.

Even with the restrictions of ACPI and SBSA, we end-up with at least 2
main families of interrupt controllers (GICv2 and GICv3), both with a
number of "interesting" variations (GICv2m and GICv4, to only mention
those I'm directly involved with).

I can safely predict that the above will become a tangled mess within 18
months, and the idea of littering the arch code with a bunch of
hardcoded "if (blah())" doesn't fill me with joy and confidence.

In summary: we have the infrastructure already, just use it.

Thanks,

	M.
Hanjun Guo Sept. 2, 2014, 3:45 p.m. UTC | #5
On 2014?09?02? 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 <tomasz.nowicki@linaro.org>
>>>>
>>>> ACPI kernel uses MADT table for proper GIC initialization. It needs to
>>>> parse GIC related subtables, collect CPU interface and distributor
>>>> addresses and call driver initialization function (which is hardware
>>>> 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 <tomasz.nowicki@linaro.org>
>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>> ---
>>>>   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/asm/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 <linux/irqdomain.h>
>>>>   #include <linux/bootmem.h>
>>>>   #include <linux/smp.h>
>>>> +#include <linux/irqchip/arm-gic-acpi.h>
>>>>
>>>>   #include <asm/cputype.h>
>>>>   #include <asm/cpu_ops.h>
>>>> @@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
>>>>              pr_err("Can't find FADT or error happened during parsing FADT\n");
>>>>   }
>>>>
>>>> +void __init acpi_gic_init(void)
>>>> +{
>>>> +    struct acpi_table_header *table;
>>>> +    acpi_status status;
>>>> +    acpi_size tbl_size;
>>>> +    int err;
>>>> +
>>>> +    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");
>>> 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 = gic_v3_acpi_init(table);
>>         if (err)
>>                 err = 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 <linux/irqchip.h>
>>>>   #include <linux/seq_file.h>
>>>>   #include <linux/ratelimit.h>
>>>> +#include <linux/irqchip/arm-gic-acpi.h>
>>>>
>>>>   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 logical
>>> 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 thing
> we support so far. I don't think duplicating the kernel infrastructure
> "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). Just
> put in place a similar probing mechanism, and this will look a lot better.
>
>>>>      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 <linux/of.h>
>>>>   #include <linux/of_address.h>
>>>>   #include <linux/of_irq.h>
>>>> +#include <linux/acpi.h>
>>>>   #include <linux/irqdomain.h>
>>>>   #include <linux/interrupt.h>
>>>>   #include <linux/percpu.h>
>>>>   #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>
>>>> @@ -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 = ULONG_MAX;
>>> Please use phys_addr_t for physical addresses. The use of ULONG_MAX
>>> 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 = (struct acpi_madt_generic_interrupt *)header;
>>>> +
>>>> +    if (BAD_MADT_ENTRY(processor, end))
>>>> +            return -EINVAL;
>>>> +
>>>> +    gic_cpu_base = 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 ACPI spec.
>>>> +     * All CPU interface addresses have to be the same.
>>>> +     */
>>>> +    if (cpu_phy_base != ULONG_MAX && gic_cpu_base != cpu_phy_base)
>>>> +            return -EFAULT;
>>>> +
>>>> +    cpu_phy_base = gic_cpu_base;
>>>> +    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;
>>>> +    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 = acpi_parse_entries(sizeof(struct acpi_table_madt),
>>>> +                               gic_acpi_parse_madt_cpu, table,
>>>> +                               ACPI_MADT_TYPE_GENERIC_INTERRUPT,
>>>> +                               ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES);
>>>> +    if (count < 0) {
>>>> +            pr_err("Error during GICC entries parsing\n");
>>>> +            return -EFAULT;
>>>> +    } else if (!count) {
>>>> +            /* No GICC entries provided, use address from MADT header */
>>>> +            struct acpi_table_madt *madt = (struct acpi_table_madt *)table;
>>>> +
>>>> +            if (!madt->address)
>>>> +                    return -EFAULT;
>>>> +
>>>> +            cpu_phy_base = (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 = acpi_parse_entries(sizeof(struct acpi_table_madt),
>>>> +                               gic_acpi_parse_madt_distributor, table,
>>>> +                               ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR,
>>>> +                               ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES);
>>>> +    if (count <= 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 = 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_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 GSI 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 <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_
>>>> +
>>>> +#include <linux/acpi.h>
>>> 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
>> <linux/acpi.h>. 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) from?

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 core will stop
scan MADT if the real numbers of processors entries are reached no matter
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 same thing).

Thanks
Hanjun
Marc Zyngier Sept. 2, 2014, 3:59 p.m. UTC | #6
On 02/09/14 16:45, Hanjun Guo wrote:
> On 2014?09?02? 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 <tomasz.nowicki@linaro.org>
>>>>>
>>>>> ACPI kernel uses MADT table for proper GIC initialization. It needs to
>>>>> parse GIC related subtables, collect CPU interface and distributor
>>>>> addresses and call driver initialization function (which is hardware
>>>>> 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 <tomasz.nowicki@linaro.org>
>>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>>> ---
>>>>>   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/asm/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 <linux/irqdomain.h>
>>>>>   #include <linux/bootmem.h>
>>>>>   #include <linux/smp.h>
>>>>> +#include <linux/irqchip/arm-gic-acpi.h>
>>>>>
>>>>>   #include <asm/cputype.h>
>>>>>   #include <asm/cpu_ops.h>
>>>>> @@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
>>>>>              pr_err("Can't find FADT or error happened during parsing FADT\n");
>>>>>   }
>>>>>
>>>>> +void __init acpi_gic_init(void)
>>>>> +{
>>>>> +    struct acpi_table_header *table;
>>>>> +    acpi_status status;
>>>>> +    acpi_size tbl_size;
>>>>> +    int err;
>>>>> +
>>>>> +    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");
>>>> 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 = gic_v3_acpi_init(table);
>>>         if (err)
>>>                 err = 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 <linux/irqchip.h>
>>>>>   #include <linux/seq_file.h>
>>>>>   #include <linux/ratelimit.h>
>>>>> +#include <linux/irqchip/arm-gic-acpi.h>
>>>>>
>>>>>   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 logical
>>>> 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 thing
>> we support so far. I don't think duplicating the kernel infrastructure
>> "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). Just
>> put in place a similar probing mechanism, and this will look a lot better.
>>
>>>>>      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 <linux/of.h>
>>>>>   #include <linux/of_address.h>
>>>>>   #include <linux/of_irq.h>
>>>>> +#include <linux/acpi.h>
>>>>>   #include <linux/irqdomain.h>
>>>>>   #include <linux/interrupt.h>
>>>>>   #include <linux/percpu.h>
>>>>>   #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>
>>>>> @@ -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 = ULONG_MAX;
>>>> Please use phys_addr_t for physical addresses. The use of ULONG_MAX
>>>> 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 = (struct acpi_madt_generic_interrupt *)header;
>>>>> +
>>>>> +    if (BAD_MADT_ENTRY(processor, end))
>>>>> +            return -EINVAL;
>>>>> +
>>>>> +    gic_cpu_base = 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 ACPI spec.
>>>>> +     * All CPU interface addresses have to be the same.
>>>>> +     */
>>>>> +    if (cpu_phy_base != ULONG_MAX && gic_cpu_base != cpu_phy_base)
>>>>> +            return -EFAULT;
>>>>> +
>>>>> +    cpu_phy_base = gic_cpu_base;
>>>>> +    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;
>>>>> +    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 = acpi_parse_entries(sizeof(struct acpi_table_madt),
>>>>> +                               gic_acpi_parse_madt_cpu, table,
>>>>> +                               ACPI_MADT_TYPE_GENERIC_INTERRUPT,
>>>>> +                               ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES);
>>>>> +    if (count < 0) {
>>>>> +            pr_err("Error during GICC entries parsing\n");
>>>>> +            return -EFAULT;
>>>>> +    } else if (!count) {
>>>>> +            /* No GICC entries provided, use address from MADT header */
>>>>> +            struct acpi_table_madt *madt = (struct acpi_table_madt *)table;
>>>>> +
>>>>> +            if (!madt->address)
>>>>> +                    return -EFAULT;
>>>>> +
>>>>> +            cpu_phy_base = (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 = acpi_parse_entries(sizeof(struct acpi_table_madt),
>>>>> +                               gic_acpi_parse_madt_distributor, table,
>>>>> +                               ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR,
>>>>> +                               ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES);
>>>>> +    if (count <= 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 = 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_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 GSI 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 <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_
>>>>> +
>>>>> +#include <linux/acpi.h>
>>>> 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
>>> <linux/acpi.h>. 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) from?
> 
> 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 core will stop
> scan MADT if the real numbers of processors entries are reached no matter
> 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 same thing).

Then it is worth mentioning that this is an arbitrary limit, unrelated
to what the architecture describes.

Thanks,

	M.
Sudeep Holla Sept. 2, 2014, 4:11 p.m. UTC | #7
On 02/09/14 16:45, Hanjun Guo wrote:
> On 2014?09?02? 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 <tomasz.nowicki@linaro.org>
>>>>>
>>>>> ACPI kernel uses MADT table for proper GIC initialization. It needs to
>>>>> parse GIC related subtables, collect CPU interface and distributor
>>>>> addresses and call driver initialization function (which is hardware
>>>>> 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 <tomasz.nowicki@linaro.org>
>>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>>> ---
>>>>>    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/asm/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 <linux/irqdomain.h>
>>>>>    #include <linux/bootmem.h>
>>>>>    #include <linux/smp.h>
>>>>> +#include <linux/irqchip/arm-gic-acpi.h>
>>>>>
>>>>>    #include <asm/cputype.h>
>>>>>    #include <asm/cpu_ops.h>
>>>>> @@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
>>>>>               pr_err("Can't find FADT or error happened during parsing FADT\n");
>>>>>    }
>>>>>
>>>>> +void __init acpi_gic_init(void)
>>>>> +{
>>>>> +    struct acpi_table_header *table;
>>>>> +    acpi_status status;
>>>>> +    acpi_size tbl_size;
>>>>> +    int err;
>>>>> +
>>>>> +    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");
>>>> 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 = gic_v3_acpi_init(table);
>>>          if (err)
>>>                  err = 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 <linux/irqchip.h>
>>>>>    #include <linux/seq_file.h>
>>>>>    #include <linux/ratelimit.h>
>>>>> +#include <linux/irqchip/arm-gic-acpi.h>
>>>>>
>>>>>    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 logical
>>>> 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 thing
>> we support so far. I don't think duplicating the kernel infrastructure
>> "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). Just
>> put in place a similar probing mechanism, and this will look a lot better.
>>
>>>>>       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 <linux/of.h>
>>>>>    #include <linux/of_address.h>
>>>>>    #include <linux/of_irq.h>
>>>>> +#include <linux/acpi.h>
>>>>>    #include <linux/irqdomain.h>
>>>>>    #include <linux/interrupt.h>
>>>>>    #include <linux/percpu.h>
>>>>>    #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>
>>>>> @@ -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 = ULONG_MAX;
>>>> Please use phys_addr_t for physical addresses. The use of ULONG_MAX
>>>> 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 = (struct acpi_madt_generic_interrupt *)header;
>>>>> +
>>>>> +    if (BAD_MADT_ENTRY(processor, end))
>>>>> +            return -EINVAL;
>>>>> +
>>>>> +    gic_cpu_base = 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 ACPI spec.
>>>>> +     * All CPU interface addresses have to be the same.
>>>>> +     */
>>>>> +    if (cpu_phy_base != ULONG_MAX && gic_cpu_base != cpu_phy_base)
>>>>> +            return -EFAULT;
>>>>> +
>>>>> +    cpu_phy_base = gic_cpu_base;
>>>>> +    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;
>>>>> +    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 = acpi_parse_entries(sizeof(struct acpi_table_madt),
>>>>> +                               gic_acpi_parse_madt_cpu, table,
>>>>> +                               ACPI_MADT_TYPE_GENERIC_INTERRUPT,
>>>>> +                               ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES);
>>>>> +    if (count < 0) {
>>>>> +            pr_err("Error during GICC entries parsing\n");
>>>>> +            return -EFAULT;
>>>>> +    } else if (!count) {
>>>>> +            /* No GICC entries provided, use address from MADT header */
>>>>> +            struct acpi_table_madt *madt = (struct acpi_table_madt *)table;
>>>>> +
>>>>> +            if (!madt->address)
>>>>> +                    return -EFAULT;
>>>>> +
>>>>> +            cpu_phy_base = (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 = acpi_parse_entries(sizeof(struct acpi_table_madt),
>>>>> +                               gic_acpi_parse_madt_distributor, table,
>>>>> +                               ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR,
>>>>> +                               ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES);
>>>>> +    if (count <= 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 = 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_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 GSI 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 <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_
>>>>> +
>>>>> +#include <linux/acpi.h>
>>>> 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
>>> <linux/acpi.h>. 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) from?
>
> 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 core will stop
> scan MADT if the real numbers of processors entries are reached no matter
> 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 same thing).
>

This is the exact reason I kept mentioning *not to link it with GIC
architecture* in my previous reviews. It's just *max possible entries in
MADT*.

Regards,
Sudeep
Catalin Marinas Sept. 2, 2014, 4:34 p.m. UTC | #8
On Tue, Sep 02, 2014 at 12:48:37PM +0100, Tomasz Nowicki wrote:
> On 01.09.2014 19:35, Marc Zyngier wrote:
> > On 01/09/14 15:57, Hanjun Guo wrote:
> >> From: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> >>
> >> ACPI kernel uses MADT table for proper GIC initialization. It needs to
> >> parse GIC related subtables, collect CPU interface and distributor
> >> addresses and call driver initialization function (which is hardware
> >> 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!

It's also worth mentioning if there are any plans to fix this ;).
Tomasz Nowicki Sept. 3, 2014, 9:26 a.m. UTC | #9
On 02.09.2014 15: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 <tomasz.nowicki@linaro.org>
>>>>
>>>> ACPI kernel uses MADT table for proper GIC initialization. It needs to
>>>> parse GIC related subtables, collect CPU interface and distributor
>>>> addresses and call driver initialization function (which is hardware
>>>> 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 <tomasz.nowicki@linaro.org>
>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>> ---
>>>>    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/asm/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 <linux/irqdomain.h>
>>>>    #include <linux/bootmem.h>
>>>>    #include <linux/smp.h>
>>>> +#include <linux/irqchip/arm-gic-acpi.h>
>>>>
>>>>    #include <asm/cputype.h>
>>>>    #include <asm/cpu_ops.h>
>>>> @@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
>>>>               pr_err("Can't find FADT or error happened during parsing FADT\n");
>>>>    }
>>>>
>>>> +void __init acpi_gic_init(void)
>>>> +{
>>>> +    struct acpi_table_header *table;
>>>> +    acpi_status status;
>>>> +    acpi_size tbl_size;
>>>> +    int err;
>>>> +
>>>> +    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");
>>>
>>> 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 = gic_v3_acpi_init(table);
>>          if (err)
>>                  err = 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 <linux/irqchip.h>
>>>>    #include <linux/seq_file.h>
>>>>    #include <linux/ratelimit.h>
>>>> +#include <linux/irqchip/arm-gic-acpi.h>
>>>>
>>>>    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 logical
>>> 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 thing
> we support so far. I don't think duplicating the kernel infrastructure
> "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). Just
> put in place a similar probing mechanism, and this will look a lot better.
>
>>>
>>>>       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 <linux/of.h>
>>>>    #include <linux/of_address.h>
>>>>    #include <linux/of_irq.h>
>>>> +#include <linux/acpi.h>
>>>>    #include <linux/irqdomain.h>
>>>>    #include <linux/interrupt.h>
>>>>    #include <linux/percpu.h>
>>>>    #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>
>>>> @@ -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 = ULONG_MAX;
>>>
>>> Please use phys_addr_t for physical addresses. The use of ULONG_MAX
>>> 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 = (struct acpi_madt_generic_interrupt *)header;
>>>> +
>>>> +    if (BAD_MADT_ENTRY(processor, end))
>>>> +            return -EINVAL;
>>>> +
>>>> +    gic_cpu_base = 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 ACPI spec.
>>>> +     * All CPU interface addresses have to be the same.
>>>> +     */
>>>> +    if (cpu_phy_base != ULONG_MAX && gic_cpu_base != cpu_phy_base)
>>>> +            return -EFAULT;
>>>> +
>>>> +    cpu_phy_base = gic_cpu_base;
>>>> +    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;
>>>> +    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 = acpi_parse_entries(sizeof(struct acpi_table_madt),
>>>> +                               gic_acpi_parse_madt_cpu, table,
>>>> +                               ACPI_MADT_TYPE_GENERIC_INTERRUPT,
>>>> +                               ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES);
>>>> +    if (count < 0) {
>>>> +            pr_err("Error during GICC entries parsing\n");
>>>> +            return -EFAULT;
>>>> +    } else if (!count) {
>>>> +            /* No GICC entries provided, use address from MADT header */
>>>> +            struct acpi_table_madt *madt = (struct acpi_table_madt *)table;
>>>> +
>>>> +            if (!madt->address)
>>>> +                    return -EFAULT;
>>>> +
>>>> +            cpu_phy_base = (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 = acpi_parse_entries(sizeof(struct acpi_table_madt),
>>>> +                               gic_acpi_parse_madt_distributor, table,
>>>> +                               ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR,
>>>> +                               ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES);
>>>> +    if (count <= 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 = 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_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 GSI 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 <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_
>>>> +
>>>> +#include <linux/acpi.h>
>>>
>>> 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
>> <linux/acpi.h>. 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) from?
>
>>>
>>>> +#define ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES    1
>>>> +
>>>> +/*
>>>> + * 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_GIC_DIST_MEM_SIZE              (SZ_64K)
>>>
>>> I don't know which version of the spec you're looking at, but my version
>>> of the GICv2 spec has a 4kB distributor. Also, it would be good to make
>>> obvious which GIC version this define is about.
>> OK
>>
>>>
>>>> +#define ACPI_GIC_CPU_IF_MEM_SIZE    (SZ_8K)
>>>> +
>>>> +void acpi_gic_init(void);
>>>> +int gic_v2_acpi_init(struct acpi_table_header *table);
>>>> +#else
>>>> +static inline void acpi_gic_init(void) { }
>>>> +#endif
>>>> +
>>>> +#endif /* ARM_GIC_ACPI_H_ */
>>>>
>>>
>>> In the end, why do we need a separate file for this? I cannot see
>>> anything that prevents it from being merged with arm-gic.h.
>>>
>>> Thanks,
>>>
>>>        M.
>>>
>> Having only GICv2, it would work. Considering we would do the same for
>> GICv3 (arm-gic-v3.h) there will be register name conflicts for both
>> headers inclusion:
>>
>> [...]
>> #include <linux/irqchip/arm-gic.h>
>> #include <linux/irqchip/arm-gic-v3.h>
>> [...]
>>          err = gic_v3_acpi_init(table);
>>          if (err)
>>                  err = gic_v2_acpi_init(table);
>>          if (err)
>>                  pr_err("Failed to initialize GIC IRQ controller");
>> [...]
>> So instead of changing register names prefix, I choose new header will
>> be less painfully.
>
> Yes, and this is exactly why I pushed back on that last time. I'll
> continue saying that interrupt controllers should be self-probing, with
> ACPI as they are with DT.
>
> Even with the restrictions of ACPI and SBSA, we end-up with at least 2
> main families of interrupt controllers (GICv2 and GICv3), both with a
> number of "interesting" variations (GICv2m and GICv4, to only mention
> those I'm directly involved with).
>
> I can safely predict that the above will become a tangled mess within 18
> months, and the idea of littering the arch code with a bunch of
> hardcoded "if (blah())" doesn't fill me with joy and confidence.
>
> In summary: we have the infrastructure already, just use it.

We had that discussion but I see we still don't have consensus here. It 
would be good to know our direction before we prepare next patch 
version. Arnd any comments on this from you side?

Tomasz
Marc Zyngier Sept. 3, 2014, 10:30 a.m. UTC | #10
On 02/09/14 16:45, Hanjun Guo wrote:
> On 2014?09?02? 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 <tomasz.nowicki@linaro.org>
>>>>>
>>>>> ACPI kernel uses MADT table for proper GIC initialization. It needs to
>>>>> parse GIC related subtables, collect CPU interface and distributor
>>>>> addresses and call driver initialization function (which is hardware
>>>>> 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 <tomasz.nowicki@linaro.org>
>>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>>> ---
>>>>>   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/asm/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 <linux/irqdomain.h>
>>>>>   #include <linux/bootmem.h>
>>>>>   #include <linux/smp.h>
>>>>> +#include <linux/irqchip/arm-gic-acpi.h>
>>>>>
>>>>>   #include <asm/cputype.h>
>>>>>   #include <asm/cpu_ops.h>
>>>>> @@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
>>>>>              pr_err("Can't find FADT or error happened during parsing FADT\n");
>>>>>   }
>>>>>
>>>>> +void __init acpi_gic_init(void)
>>>>> +{
>>>>> +    struct acpi_table_header *table;
>>>>> +    acpi_status status;
>>>>> +    acpi_size tbl_size;
>>>>> +    int err;
>>>>> +
>>>>> +    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");
>>>> 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 = gic_v3_acpi_init(table);
>>>         if (err)
>>>                 err = 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 <linux/irqchip.h>
>>>>>   #include <linux/seq_file.h>
>>>>>   #include <linux/ratelimit.h>
>>>>> +#include <linux/irqchip/arm-gic-acpi.h>
>>>>>
>>>>>   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 logical
>>>> 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 thing
>> we support so far. I don't think duplicating the kernel infrastructure
>> "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). Just
>> put in place a similar probing mechanism, and this will look a lot better.
>>
>>>>>      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 <linux/of.h>
>>>>>   #include <linux/of_address.h>
>>>>>   #include <linux/of_irq.h>
>>>>> +#include <linux/acpi.h>
>>>>>   #include <linux/irqdomain.h>
>>>>>   #include <linux/interrupt.h>
>>>>>   #include <linux/percpu.h>
>>>>>   #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>
>>>>> @@ -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 = ULONG_MAX;
>>>> Please use phys_addr_t for physical addresses. The use of ULONG_MAX
>>>> 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 = (struct acpi_madt_generic_interrupt *)header;
>>>>> +
>>>>> +    if (BAD_MADT_ENTRY(processor, end))
>>>>> +            return -EINVAL;
>>>>> +
>>>>> +    gic_cpu_base = 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 ACPI spec.
>>>>> +     * All CPU interface addresses have to be the same.
>>>>> +     */
>>>>> +    if (cpu_phy_base != ULONG_MAX && gic_cpu_base != cpu_phy_base)
>>>>> +            return -EFAULT;
>>>>> +
>>>>> +    cpu_phy_base = gic_cpu_base;
>>>>> +    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;
>>>>> +    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 = acpi_parse_entries(sizeof(struct acpi_table_madt),
>>>>> +                               gic_acpi_parse_madt_cpu, table,
>>>>> +                               ACPI_MADT_TYPE_GENERIC_INTERRUPT,
>>>>> +                               ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES);
>>>>> +    if (count < 0) {
>>>>> +            pr_err("Error during GICC entries parsing\n");
>>>>> +            return -EFAULT;
>>>>> +    } else if (!count) {
>>>>> +            /* No GICC entries provided, use address from MADT header */
>>>>> +            struct acpi_table_madt *madt = (struct acpi_table_madt *)table;
>>>>> +
>>>>> +            if (!madt->address)
>>>>> +                    return -EFAULT;
>>>>> +
>>>>> +            cpu_phy_base = (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 = acpi_parse_entries(sizeof(struct acpi_table_madt),
>>>>> +                               gic_acpi_parse_madt_distributor, table,
>>>>> +                               ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR,
>>>>> +                               ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES);
>>>>> +    if (count <= 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 = 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_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 GSI 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 <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_
>>>>> +
>>>>> +#include <linux/acpi.h>
>>>> 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
>>> <linux/acpi.h>. 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) from?
> 
> 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 core will stop
> scan MADT if the real numbers of processors entries are reached no matter
> 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 same thing).

Also, with GICv3++, there is no such thing as a memory-mapped CPU
interface anymore. What you get is a bunch of redistributors (one per
CPU). I assume what you have here actually describe the redistributors,
and its name should reflect that.

Thanks,

	M.
Hanjun Guo Sept. 3, 2014, 11:17 a.m. UTC | #11
>>>>>> +
>>>>>> +#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) from?
>> 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 core will stop
>> scan MADT if the real numbers of processors entries are reached no matter
>> 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 same thing).
> Also, with GICv3++, there is no such thing as a memory-mapped CPU
> interface anymore. What you get is a bunch of redistributors (one per
> CPU). I assume what you have here actually describe the redistributors,
> and its name should reflect that.

As Sudeep said, it is not to link to GIC architecture, so I think we can keep
it stick with ACPI spec, in ACPI spec, it called "GICC structure" (section 5.2.12.14
in ACPI 5.1), so we can name it as ACPI_MAX_GICC_STRUCTURE_ENTRIES no matter
GICv2 or GICv3/4 (with GICv2, it may have more than 8 entries with some disabled
ones, will no more than 8 enabled entries).

What do you think?

Thanks
Hanjun
Arnd Bergmann Sept. 3, 2014, 2:57 p.m. UTC | #12
On Wednesday 03 September 2014 11:26:14 Tomasz Nowicki wrote:
> On 02.09.2014 15:02, Marc Zyngier wrote:
> > On 02/09/14 12:48, Tomasz Nowicki wrote:
> >> On 01.09.2014 19:35, Marc Zyngier wrote:
> >>>> @@ -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 logical
> >>> 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 thing
> > we support so far. I don't think duplicating the kernel infrastructure
> > "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). Just
> > put in place a similar probing mechanism, and this will look a lot better.


> >> Having only GICv2, it would work. Considering we would do the same for
> >> GICv3 (arm-gic-v3.h) there will be register name conflicts for both
> >> headers inclusion:
> >>
> >> [...]
> >> #include <linux/irqchip/arm-gic.h>
> >> #include <linux/irqchip/arm-gic-v3.h>
> >> [...]
> >>          err = gic_v3_acpi_init(table);
> >>          if (err)
> >>                  err = gic_v2_acpi_init(table);
> >>          if (err)
> >>                  pr_err("Failed to initialize GIC IRQ controller");
> >> [...]
> >> So instead of changing register names prefix, I choose new header will
> >> be less painfully.
> >
> > Yes, and this is exactly why I pushed back on that last time. I'll
> > continue saying that interrupt controllers should be self-probing, with
> > ACPI as they are with DT.
> >
> > Even with the restrictions of ACPI and SBSA, we end-up with at least 2
> > main families of interrupt controllers (GICv2 and GICv3), both with a
> > number of "interesting" variations (GICv2m and GICv4, to only mention
> > those I'm directly involved with).
> >
> > I can safely predict that the above will become a tangled mess within 18
> > months, and the idea of littering the arch code with a bunch of
> > hardcoded "if (blah())" doesn't fill me with joy and confidence.
> >
> > In summary: we have the infrastructure already, just use it.
> 
> We had that discussion but I see we still don't have consensus here. It 
> would be good to know our direction before we prepare next patch 
> version. Arnd any comments on this from you side?

I still prefer being explicit here for the same reason I mentioned earlier:
I want it to be very clear that we don't support arbitrary irqchips other
than the ones in the APCI specification. The infrastructure exists on DT
because we have to support a large number of incompatible irqchips.

In particular, the ACPI tables describing the irqchip have no way to
identify the GIC at all, if I read the spec correctly, you have to
parse the tables, ioremap the registers and then read the ID to know
if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any "device"
for the GIC that a hypothetical probe function would be based on.

It does seem wrong to parse the tables in the irq-gic.c file though:
that part can well be common across the various gic versions and then
call into either irq-gic.c or irq-gic-v3.c for the version specific
parts. Whether we put that common code into drivers/irqchip/irqchip.c,
drivers/irqchip/gic-common.c, drivers/irqchip/irq-acpi-gic.c or
drivers/acpi/irq-gic.c I don't care at all.

	Arnd
Arnd Bergmann Sept. 3, 2014, 6:42 p.m. UTC | #13
On Monday 01 September 2014 22:57:51 Hanjun Guo wrote:
> +       /* Collect CPU base addresses */
> +       count = acpi_parse_entries(sizeof(struct acpi_table_madt),
> +                                  gic_acpi_parse_madt_cpu, table,
> +                                  ACPI_MADT_TYPE_GENERIC_INTERRUPT,
> +                                  ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES);
> +       if (count < 0) {
> +               pr_err("Error during GICC entries parsing\n");
> +               return -EFAULT;
> +       } else if (!count) {
> +               /* No GICC entries provided, use address from MADT header */
> +               struct acpi_table_madt *madt = (struct acpi_table_madt *)table;
> +
> +               if (!madt->address)
> +                       return -EFAULT;
> +
> +               cpu_phy_base = (u64)madt->address;
> +       }

After I read through ACPI-5.1 section 5.2.12.14, I wonder if this is the
best way to treat a missing ACPI_MADT_TYPE_GENERIC_INTERRUPT table.

Do we expect to see those in practice? It seems like using the x86 local
APIC address as a fallback for the GIC address is not something we
should do unless we absolutely have to support a system that doesn't
have the GIC table.

	Arnd
Tomasz Nowicki Sept. 4, 2014, 10:10 a.m. UTC | #14
On 03.09.2014 20:42, Arnd Bergmann wrote:
> On Monday 01 September 2014 22:57:51 Hanjun Guo wrote:
>> +       /* Collect CPU base addresses */
>> +       count = acpi_parse_entries(sizeof(struct acpi_table_madt),
>> +                                  gic_acpi_parse_madt_cpu, table,
>> +                                  ACPI_MADT_TYPE_GENERIC_INTERRUPT,
>> +                                  ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES);
>> +       if (count < 0) {
>> +               pr_err("Error during GICC entries parsing\n");
>> +               return -EFAULT;
>> +       } else if (!count) {
>> +               /* No GICC entries provided, use address from MADT header */
>> +               struct acpi_table_madt *madt = (struct acpi_table_madt *)table;
>> +
>> +               if (!madt->address)
>> +                       return -EFAULT;
>> +
>> +               cpu_phy_base = (u64)madt->address;
>> +       }
>
> After I read through ACPI-5.1 section 5.2.12.14, I wonder if this is the
> best way to treat a missing ACPI_MADT_TYPE_GENERIC_INTERRUPT table.
>
> Do we expect to see those in practice? It seems like using the x86 local
> APIC address as a fallback for the GIC address is not something we
> should do unless we absolutely have to support a system that doesn't
> have the GIC table.

No, we do not expect and hopefully there will be no such :)

But, we are trying to be as much as possible inline with 5.1 spec, 
5.2.12.14 says:
[...]
If provided here (CPU physical base address), the "Local Interrupt 
Controller Address" field in the MADT must be ignored by the OSPM.
[...]

Regards,
Tomasz
Arnd Bergmann Sept. 4, 2014, 10:14 a.m. UTC | #15
On Thursday 04 September 2014 12:10:28 Tomasz Nowicki wrote:
> On 03.09.2014 20:42, Arnd Bergmann wrote:
> > On Monday 01 September 2014 22:57:51 Hanjun Guo wrote:
> >> +       /* Collect CPU base addresses */
> >> +       count = acpi_parse_entries(sizeof(struct acpi_table_madt),
> >> +                                  gic_acpi_parse_madt_cpu, table,
> >> +                                  ACPI_MADT_TYPE_GENERIC_INTERRUPT,
> >> +                                  ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES);
> >> +       if (count < 0) {
> >> +               pr_err("Error during GICC entries parsing\n");
> >> +               return -EFAULT;
> >> +       } else if (!count) {
> >> +               /* No GICC entries provided, use address from MADT header */
> >> +               struct acpi_table_madt *madt = (struct acpi_table_madt *)table;
> >> +
> >> +               if (!madt->address)
> >> +                       return -EFAULT;
> >> +
> >> +               cpu_phy_base = (u64)madt->address;
> >> +       }
> >
> > After I read through ACPI-5.1 section 5.2.12.14, I wonder if this is the
> > best way to treat a missing ACPI_MADT_TYPE_GENERIC_INTERRUPT table.
> >
> > Do we expect to see those in practice? It seems like using the x86 local
> > APIC address as a fallback for the GIC address is not something we
> > should do unless we absolutely have to support a system that doesn't
> > have the GIC table.
> 
> No, we do not expect and hopefully there will be no such 
> 
> But, we are trying to be as much as possible inline with 5.1 spec, 
> 5.2.12.14 says:
> [...]
> If provided here (CPU physical base address), the "Local Interrupt 
> Controller Address" field in the MADT must be ignored by the OSPM.
> [...]
> 

Yes, that's what I saw. So ignoring it all the time is fine, right?
Presumably the madt->address field is only referenced here because
some pre-5.1 implementations used to do that.

	Arnd
Tomasz Nowicki Sept. 4, 2014, 10:39 a.m. UTC | #16
On 04.09.2014 12:14, Arnd Bergmann wrote:
> On Thursday 04 September 2014 12:10:28 Tomasz Nowicki wrote:
>> On 03.09.2014 20:42, Arnd Bergmann wrote:
>>> On Monday 01 September 2014 22:57:51 Hanjun Guo wrote:
>>>> +       /* Collect CPU base addresses */
>>>> +       count = acpi_parse_entries(sizeof(struct acpi_table_madt),
>>>> +                                  gic_acpi_parse_madt_cpu, table,
>>>> +                                  ACPI_MADT_TYPE_GENERIC_INTERRUPT,
>>>> +                                  ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES);
>>>> +       if (count < 0) {
>>>> +               pr_err("Error during GICC entries parsing\n");
>>>> +               return -EFAULT;
>>>> +       } else if (!count) {
>>>> +               /* No GICC entries provided, use address from MADT header */
>>>> +               struct acpi_table_madt *madt = (struct acpi_table_madt *)table;
>>>> +
>>>> +               if (!madt->address)
>>>> +                       return -EFAULT;
>>>> +
>>>> +               cpu_phy_base = (u64)madt->address;
>>>> +       }
>>>
>>> After I read through ACPI-5.1 section 5.2.12.14, I wonder if this is the
>>> best way to treat a missing ACPI_MADT_TYPE_GENERIC_INTERRUPT table.
>>>
>>> Do we expect to see those in practice? It seems like using the x86 local
>>> APIC address as a fallback for the GIC address is not something we
>>> should do unless we absolutely have to support a system that doesn't
>>> have the GIC table.
>>
>> No, we do not expect and hopefully there will be no such
>>
>> But, we are trying to be as much as possible inline with 5.1 spec,
>> 5.2.12.14 says:
>> [...]
>> If provided here (CPU physical base address), the "Local Interrupt
>> Controller Address" field in the MADT must be ignored by the OSPM.
>> [...]
>>
>
> Yes, that's what I saw. So ignoring it all the time is fine, right?
> Presumably the madt->address field is only referenced here because
> some pre-5.1 implementations used to do that.

So this is very vague statement. On the one hand it would make sense to 
take madt->address if we have no GICC entries. On the other hand we do 
not support non-banked GIC cpu registers. So all of then need to have 
the same cpu_base_address. What if one has null address? Should the rest 
take madt->address? I think you are right, I will remove madt->address 
fallback and simplify the code.

Thanks,
Tomasz
Hanjun Guo Sept. 4, 2014, 2:03 p.m. UTC | #17
On 2014?09?03? 19:17, Hanjun Guo wrote:
>>>>>>> +
>>>>>>> +#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) from?
>>> 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 core will stop
>>> scan MADT if the real numbers of processors entries are reached no matter
>>> 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 same thing).
>> Also, with GICv3++, there is no such thing as a memory-mapped CPU
>> interface anymore. What you get is a bunch of redistributors (one per
>> CPU). I assume what you have here actually describe the redistributors,
>> and its name should reflect that.
> As Sudeep said, it is not to link to GIC architecture, so I think we can keep
> it stick with ACPI spec, in ACPI spec, it called "GICC structure" (section 5.2.12.14
> in ACPI 5.1), so we can name it as ACPI_MAX_GICC_STRUCTURE_ENTRIES no matter
> GICv2 or GICv3/4 (with GICv2, it may have more than 8 entries with some disabled
> ones, will no more than 8 enabled entries).
>
> What do you think?

After more consideration on this, we think that we can remove those macros which
introduce confusions, and just pass 0 to ACPI core for the max entries of GICC structure
or GICR structure, ACPI core will continue scan all the entries in MADT with 0 passed.
With that, we can avoid such name confusions for all GIC related structures in MADT
no matter GICv2 or GICv3/4.

Thanks
Hanjun
Tomasz Nowicki Sept. 5, 2014, 8:52 a.m. UTC | #18
On 03.09.2014 16:57, Arnd Bergmann wrote:
> On Wednesday 03 September 2014 11:26:14 Tomasz Nowicki wrote:
>> On 02.09.2014 15:02, Marc Zyngier wrote:
>>> On 02/09/14 12:48, Tomasz Nowicki wrote:
>>>> On 01.09.2014 19:35, Marc Zyngier wrote:
>>>>>> @@ -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 logical
>>>>> 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 thing
>>> we support so far. I don't think duplicating the kernel infrastructure
>>> "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). Just
>>> put in place a similar probing mechanism, and this will look a lot better.
>
>
>>>> Having only GICv2, it would work. Considering we would do the same for
>>>> GICv3 (arm-gic-v3.h) there will be register name conflicts for both
>>>> headers inclusion:
>>>>
>>>> [...]
>>>> #include <linux/irqchip/arm-gic.h>
>>>> #include <linux/irqchip/arm-gic-v3.h>
>>>> [...]
>>>>           err = gic_v3_acpi_init(table);
>>>>           if (err)
>>>>                   err = gic_v2_acpi_init(table);
>>>>           if (err)
>>>>                   pr_err("Failed to initialize GIC IRQ controller");
>>>> [...]
>>>> So instead of changing register names prefix, I choose new header will
>>>> be less painfully.
>>>
>>> Yes, and this is exactly why I pushed back on that last time. I'll
>>> continue saying that interrupt controllers should be self-probing, with
>>> ACPI as they are with DT.
>>>
>>> Even with the restrictions of ACPI and SBSA, we end-up with at least 2
>>> main families of interrupt controllers (GICv2 and GICv3), both with a
>>> number of "interesting" variations (GICv2m and GICv4, to only mention
>>> those I'm directly involved with).
>>>
>>> I can safely predict that the above will become a tangled mess within 18
>>> months, and the idea of littering the arch code with a bunch of
>>> hardcoded "if (blah())" doesn't fill me with joy and confidence.
>>>
>>> In summary: we have the infrastructure already, just use it.
>>
>> We had that discussion but I see we still don't have consensus here. It
>> would be good to know our direction before we prepare next patch
>> version. Arnd any comments on this from you side?
>
> I still prefer being explicit here for the same reason I mentioned earlier:
> I want it to be very clear that we don't support arbitrary irqchips other
> than the ones in the APCI specification. The infrastructure exists on DT
> because we have to support a large number of incompatible irqchips.
>
> In particular, the ACPI tables describing the irqchip have no way to
> identify the GIC at all, if I read the spec correctly, you have to
> parse the tables, ioremap the registers and then read the ID to know
> if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any "device"
> for the GIC that a hypothetical probe function would be based on.
Yes, it is just one table with bunch of different subtables types. Since 
GIC identification register is at different offset across GICv1/2 and 
GICv3, the probe logic seems to be slightly different. IMO, we should 
look into our MADT and try GICv3 fist and then GICv2, that means 
presence of redistributor entries suggests GICv3/4. Its absence means we 
need to look for GICC subtables and then call GICv2 init.

>
> It does seem wrong to parse the tables in the irq-gic.c file though:
> that part can well be common across the various gic versions and then
> call into either irq-gic.c or irq-gic-v3.c for the version specific
> parts. Whether we put that common code into drivers/irqchip/irqchip.c,
> drivers/irqchip/gic-common.c, drivers/irqchip/irq-acpi-gic.c or
> drivers/acpi/irq-gic.c I don't care at all.

We already had tried making MADT parser common for all GICs in the 
separate file (outside GIC driver), it did not end up well. In the light 
of above comment, the logic will be complex and some GIC local strutures 
need to be exported:
1. Check redistributor entries.
2. If none exist, as fallback, check GICC entries, there are 
redistributor base address assigned to CPU. Unfortunately it has no 
register set size so ioremap only two pages (RD + SGI) check if we 
support VLPI and extend ioremap if true. We cannot operate on GIC 
register outside GIC driver so that requires another exported GICv3 
function.
3. Jump to redistributor part if we are OK so far, if not, then we start 
playing with GICC enries and look for cpu base addresses.

Honestly, apart from distributor parsing logic, there is no common code.

As you can see, current gic_v2_acpi_init() logic is quite easy to 
follow. And gic_v3_acpi_init() inside of GICv3 driver is easy too. I 
might not see other solutions but I am open to other suggestions, so 
please correct me in that case.

Regards,
Tomasz
Marc Zyngier Sept. 5, 2014, 9:47 a.m. UTC | #19
On 03/09/14 15:57, Arnd Bergmann wrote:
> On Wednesday 03 September 2014 11:26:14 Tomasz Nowicki wrote:
>> On 02.09.2014 15:02, Marc Zyngier wrote:
>>> On 02/09/14 12:48, Tomasz Nowicki wrote:
>>>> On 01.09.2014 19:35, Marc Zyngier wrote:
>>>>>> @@ -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 logical
>>>>> 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 thing
>>> we support so far. I don't think duplicating the kernel infrastructure
>>> "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). Just
>>> put in place a similar probing mechanism, and this will look a lot better.
> 
> 
>>>> Having only GICv2, it would work. Considering we would do the same for
>>>> GICv3 (arm-gic-v3.h) there will be register name conflicts for both
>>>> headers inclusion:
>>>>
>>>> [...]
>>>> #include <linux/irqchip/arm-gic.h>
>>>> #include <linux/irqchip/arm-gic-v3.h>
>>>> [...]
>>>>          err = gic_v3_acpi_init(table);
>>>>          if (err)
>>>>                  err = gic_v2_acpi_init(table);
>>>>          if (err)
>>>>                  pr_err("Failed to initialize GIC IRQ controller");
>>>> [...]
>>>> So instead of changing register names prefix, I choose new header will
>>>> be less painfully.
>>>
>>> Yes, and this is exactly why I pushed back on that last time. I'll
>>> continue saying that interrupt controllers should be self-probing, with
>>> ACPI as they are with DT.
>>>
>>> Even with the restrictions of ACPI and SBSA, we end-up with at least 2
>>> main families of interrupt controllers (GICv2 and GICv3), both with a
>>> number of "interesting" variations (GICv2m and GICv4, to only mention
>>> those I'm directly involved with).
>>>
>>> I can safely predict that the above will become a tangled mess within 18
>>> months, and the idea of littering the arch code with a bunch of
>>> hardcoded "if (blah())" doesn't fill me with joy and confidence.
>>>
>>> In summary: we have the infrastructure already, just use it.
>>
>> We had that discussion but I see we still don't have consensus here. It 
>> would be good to know our direction before we prepare next patch 
>> version. Arnd any comments on this from you side?
> 
> I still prefer being explicit here for the same reason I mentioned earlier:
> I want it to be very clear that we don't support arbitrary irqchips other
> than the ones in the APCI specification. The infrastructure exists on DT
> because we have to support a large number of incompatible irqchips.

I'm not suggesting that we should support more than the ACPI spec says.
And that's certainly the whole point of a spec, isn't it? ACPI says what
we support, and we're not going any further. I'm just saying that we
shouldn't make the maintenance burden heavier, and the code nothing
short of disgusting. Using our current infrastructure doesn't mean we're
going to support GIcv2.37.

> In particular, the ACPI tables describing the irqchip have no way to
> identify the GIC at all, if I read the spec correctly, you have to
> parse the tables, ioremap the registers and then read the ID to know
> if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any "device"
> for the GIC that a hypothetical probe function would be based on.

This is not the way I read the spec. Table 5-46 (Interrupt Controller
Structure) tells you if you have a CPU interface (GICv1/v2) or a
redistributor (GICv3/v4). That's enough to know whether or not you
should carry on probing a particular controller.

The various GIC versions don't really have a unified memory map anyway
(well, none that you can rely on), and you really have to rely on ACPI
to tell you what you have.

> It does seem wrong to parse the tables in the irq-gic.c file though:
> that part can well be common across the various gic versions and then
> call into either irq-gic.c or irq-gic-v3.c for the version specific
> parts. Whether we put that common code into drivers/irqchip/irqchip.c,
> drivers/irqchip/gic-common.c, drivers/irqchip/irq-acpi-gic.c or
> drivers/acpi/irq-gic.c I don't care at all.

I don't think so you can make that common code very easily. The
information required by both drivers is organized differently.
If it was, I'd have done that for the DT binding.

Thanks,

	M.
Arnd Bergmann Sept. 5, 2014, 10:13 a.m. UTC | #20
On Friday 05 September 2014 10:47:30 Marc Zyngier wrote:
> > 
> > I still prefer being explicit here for the same reason I mentioned earlier:
> > I want it to be very clear that we don't support arbitrary irqchips other
> > than the ones in the APCI specification. The infrastructure exists on DT
> > because we have to support a large number of incompatible irqchips.
> 
> I'm not suggesting that we should support more than the ACPI spec says.
> And that's certainly the whole point of a spec, isn't it? ACPI says what
> we support, and we're not going any further. I'm just saying that we
> shouldn't make the maintenance burden heavier, and the code nothing
> short of disgusting. Using our current infrastructure doesn't mean we're
> going to support GIcv2.37.

Ok

> > In particular, the ACPI tables describing the irqchip have no way to
> > identify the GIC at all, if I read the spec correctly, you have to
> > parse the tables, ioremap the registers and then read the ID to know
> > if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any "device"
> > for the GIC that a hypothetical probe function would be based on.
> 
> This is not the way I read the spec. Table 5-46 (Interrupt Controller
> Structure) tells you if you have a CPU interface (GICv1/v2) or a
> redistributor (GICv3/v4). That's enough to know whether or not you
> should carry on probing a particular controller.

Ah, good. I missed that.

> The various GIC versions don't really have a unified memory map anyway
> (well, none that you can rely on), and you really have to rely on ACPI
> to tell you what you have.

So we are back to needing to support two different irqchip drivers
(v1/v2/v2m and v3/v4), instead of five or more, right?

> > It does seem wrong to parse the tables in the irq-gic.c file though:
> > that part can well be common across the various gic versions and then
> > call into either irq-gic.c or irq-gic-v3.c for the version specific
> > parts. Whether we put that common code into drivers/irqchip/irqchip.c,
> > drivers/irqchip/gic-common.c, drivers/irqchip/irq-acpi-gic.c or
> > drivers/acpi/irq-gic.c I don't care at all.
> 
> I don't think so you can make that common code very easily. The
> information required by both drivers is organized differently.
> If it was, I'd have done that for the DT binding.

I see, and that's also what Tomasz just explained. So can we just
have one an irqchip_init() function doing this:?

if (dt)
	of_irq_init(__irqchip_of_table);
else if (acpi) {
	read cpu-interface and redistributor address from acpi tables
	if (cpu-interface)
		gic_v2_acpi_init(table);
	else if(redistributor)
		gic_v3_acpi_init(table)
}

	Arnd
Tomasz Nowicki Sept. 5, 2014, 10:36 a.m. UTC | #21
On 05.09.2014 12:13, Arnd Bergmann wrote:
> On Friday 05 September 2014 10:47:30 Marc Zyngier wrote:
>>>
>>> I still prefer being explicit here for the same reason I mentioned earlier:
>>> I want it to be very clear that we don't support arbitrary irqchips other
>>> than the ones in the APCI specification. The infrastructure exists on DT
>>> because we have to support a large number of incompatible irqchips.
>>
>> I'm not suggesting that we should support more than the ACPI spec says.
>> And that's certainly the whole point of a spec, isn't it? ACPI says what
>> we support, and we're not going any further. I'm just saying that we
>> shouldn't make the maintenance burden heavier, and the code nothing
>> short of disgusting. Using our current infrastructure doesn't mean we're
>> going to support GIcv2.37.
>
> Ok
>
>>> In particular, the ACPI tables describing the irqchip have no way to
>>> identify the GIC at all, if I read the spec correctly, you have to
>>> parse the tables, ioremap the registers and then read the ID to know
>>> if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any "device"
>>> for the GIC that a hypothetical probe function would be based on.
>>
>> This is not the way I read the spec. Table 5-46 (Interrupt Controller
>> Structure) tells you if you have a CPU interface (GICv1/v2) or a
>> redistributor (GICv3/v4). That's enough to know whether or not you
>> should carry on probing a particular controller.
>
> Ah, good. I missed that.
>
>> The various GIC versions don't really have a unified memory map anyway
>> (well, none that you can rely on), and you really have to rely on ACPI
>> to tell you what you have.
>
> So we are back to needing to support two different irqchip drivers
> (v1/v2/v2m and v3/v4), instead of five or more, right?
>
>>> It does seem wrong to parse the tables in the irq-gic.c file though:
>>> that part can well be common across the various gic versions and then
>>> call into either irq-gic.c or irq-gic-v3.c for the version specific
>>> parts. Whether we put that common code into drivers/irqchip/irqchip.c,
>>> drivers/irqchip/gic-common.c, drivers/irqchip/irq-acpi-gic.c or
>>> drivers/acpi/irq-gic.c I don't care at all.
>>
>> I don't think so you can make that common code very easily. The
>> information required by both drivers is organized differently.
>> If it was, I'd have done that for the DT binding.
>
> I see, and that's also what Tomasz just explained. So can we just
> have one an irqchip_init() function doing this:?
>
> if (dt)
> 	of_irq_init(__irqchip_of_table);
> else if (acpi) {
> 	read cpu-interface and redistributor address from acpi tables
> 	if (cpu-interface)
> 		gic_v2_acpi_init(table);
> 	else if(redistributor)
> 		gic_v3_acpi_init(table)
> }
>

It is pretty much the same as:
if (dt)
	of_irq_init(__irqchip_of_table);
else if (acpi) {
	err = gic_v3_acpi_init(table) {
		read redistributor address from acpi tables
		[...]
	}
	if (err) {
		err = gic_v2_acpi_init(table) {
		read cpu-interface address from acpi tables
		[...]
		}
	}
}
What basically is my current implementation (well, without GICv3 support 
but that will be next step).

Tomasz
Marc Zyngier Sept. 5, 2014, 10:39 a.m. UTC | #22
On 05/09/14 11:13, Arnd Bergmann wrote:
> On Friday 05 September 2014 10:47:30 Marc Zyngier wrote:
>>>
>>> I still prefer being explicit here for the same reason I mentioned earlier:
>>> I want it to be very clear that we don't support arbitrary irqchips other
>>> than the ones in the APCI specification. The infrastructure exists on DT
>>> because we have to support a large number of incompatible irqchips.
>>
>> I'm not suggesting that we should support more than the ACPI spec says.
>> And that's certainly the whole point of a spec, isn't it? ACPI says what
>> we support, and we're not going any further. I'm just saying that we
>> shouldn't make the maintenance burden heavier, and the code nothing
>> short of disgusting. Using our current infrastructure doesn't mean we're
>> going to support GIcv2.37.
> 
> Ok
> 
>>> In particular, the ACPI tables describing the irqchip have no way to
>>> identify the GIC at all, if I read the spec correctly, you have to
>>> parse the tables, ioremap the registers and then read the ID to know
>>> if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any "device"
>>> for the GIC that a hypothetical probe function would be based on.
>>
>> This is not the way I read the spec. Table 5-46 (Interrupt Controller
>> Structure) tells you if you have a CPU interface (GICv1/v2) or a
>> redistributor (GICv3/v4). That's enough to know whether or not you
>> should carry on probing a particular controller.
> 
> Ah, good. I missed that.
> 
>> The various GIC versions don't really have a unified memory map anyway
>> (well, none that you can rely on), and you really have to rely on ACPI
>> to tell you what you have.
> 
> So we are back to needing to support two different irqchip drivers
> (v1/v2/v2m and v3/v4), instead of five or more, right?

As long as we make sure that the variants are directly probed from the
"canonical" driver (v2m from v2, ITS and v4 from v3), then we're OK.

>>> It does seem wrong to parse the tables in the irq-gic.c file though:
>>> that part can well be common across the various gic versions and then
>>> call into either irq-gic.c or irq-gic-v3.c for the version specific
>>> parts. Whether we put that common code into drivers/irqchip/irqchip.c,
>>> drivers/irqchip/gic-common.c, drivers/irqchip/irq-acpi-gic.c or
>>> drivers/acpi/irq-gic.c I don't care at all.
>>
>> I don't think so you can make that common code very easily. The
>> information required by both drivers is organized differently.
>> If it was, I'd have done that for the DT binding.
> 
> I see, and that's also what Tomasz just explained. So can we just
> have one an irqchip_init() function doing this:?
> 
> if (dt)
> 	of_irq_init(__irqchip_of_table);
> else if (acpi) {
> 	read cpu-interface and redistributor address from acpi tables
> 	if (cpu-interface)
> 		gic_v2_acpi_init(table);
> 	else if(redistributor)
> 		gic_v3_acpi_init(table)
> }

That'd be better, but that only considers the basic architecture. GICv2m
and GICv3's ITS bring additional complexity (and their own table
parsing). At that stage, I'm not sure how much commonality we're left with.

Thanks,

	M.
Tomasz Nowicki Sept. 5, 2014, 10:49 a.m. UTC | #23
On 05.09.2014 12:39, Marc Zyngier wrote:
> On 05/09/14 11:13, Arnd Bergmann wrote:
>> On Friday 05 September 2014 10:47:30 Marc Zyngier wrote:
>>>>
>>>> I still prefer being explicit here for the same reason I mentioned earlier:
>>>> I want it to be very clear that we don't support arbitrary irqchips other
>>>> than the ones in the APCI specification. The infrastructure exists on DT
>>>> because we have to support a large number of incompatible irqchips.
>>>
>>> I'm not suggesting that we should support more than the ACPI spec says.
>>> And that's certainly the whole point of a spec, isn't it? ACPI says what
>>> we support, and we're not going any further. I'm just saying that we
>>> shouldn't make the maintenance burden heavier, and the code nothing
>>> short of disgusting. Using our current infrastructure doesn't mean we're
>>> going to support GIcv2.37.
>>
>> Ok
>>
>>>> In particular, the ACPI tables describing the irqchip have no way to
>>>> identify the GIC at all, if I read the spec correctly, you have to
>>>> parse the tables, ioremap the registers and then read the ID to know
>>>> if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any "device"
>>>> for the GIC that a hypothetical probe function would be based on.
>>>
>>> This is not the way I read the spec. Table 5-46 (Interrupt Controller
>>> Structure) tells you if you have a CPU interface (GICv1/v2) or a
>>> redistributor (GICv3/v4). That's enough to know whether or not you
>>> should carry on probing a particular controller.
>>
>> Ah, good. I missed that.
>>
>>> The various GIC versions don't really have a unified memory map anyway
>>> (well, none that you can rely on), and you really have to rely on ACPI
>>> to tell you what you have.
>>
>> So we are back to needing to support two different irqchip drivers
>> (v1/v2/v2m and v3/v4), instead of five or more, right?
>
> As long as we make sure that the variants are directly probed from the
> "canonical" driver (v2m from v2, ITS and v4 from v3), then we're OK.
Yes, that is our intention.

>
>>>> It does seem wrong to parse the tables in the irq-gic.c file though:
>>>> that part can well be common across the various gic versions and then
>>>> call into either irq-gic.c or irq-gic-v3.c for the version specific
>>>> parts. Whether we put that common code into drivers/irqchip/irqchip.c,
>>>> drivers/irqchip/gic-common.c, drivers/irqchip/irq-acpi-gic.c or
>>>> drivers/acpi/irq-gic.c I don't care at all.
>>>
>>> I don't think so you can make that common code very easily. The
>>> information required by both drivers is organized differently.
>>> If it was, I'd have done that for the DT binding.
>>
>> I see, and that's also what Tomasz just explained. So can we just
>> have one an irqchip_init() function doing this:?
>>
>> if (dt)
>> 	of_irq_init(__irqchip_of_table);
>> else if (acpi) {
>> 	read cpu-interface and redistributor address from acpi tables
>> 	if (cpu-interface)
>> 		gic_v2_acpi_init(table);
>> 	else if(redistributor)
>> 		gic_v3_acpi_init(table)
>> }
>
> That'd be better, but that only considers the basic architecture. GICv2m
> and GICv3's ITS bring additional complexity (and their own table
> parsing). At that stage, I'm not sure how much commonality we're left with.

Right, my local ACPI GICv3 support assumes to probe ITS from 
gic_v3_acpi_init() body.

Tomasz
Jon Masters Sept. 9, 2014, 6:14 a.m. UTC | #24
On 09/01/2014 01:35 PM, Marc Zyngier wrote:
> On 01/09/14 15:57, Hanjun Guo wrote:
>> From: Tomasz Nowicki <tomasz.nowicki@linaro.org>
>>
>> ACPI kernel uses MADT table for proper GIC initialization. It needs to
>> parse GIC related subtables, collect CPU interface and distributor
>> addresses and call driver initialization function (which is hardware
>> 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.

Wei Huang within Red Hat is currently beginning the process of bringing
up KVM support under ACPI on real hardware. This will be fixed asap.

Jon.
Jon Masters Sept. 9, 2014, 6:21 a.m. UTC | #25
On 09/03/2014 06:30 AM, Marc Zyngier wrote:
> On 02/09/14 16:45, Hanjun Guo wrote:

>> 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 core will stop
>> scan MADT if the real numbers of processors entries are reached no matter
>> 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 same thing).
> 
> Also, with GICv3++, there is no such thing as a memory-mapped CPU
> interface anymore. What you get is a bunch of redistributors (one per
> CPU). I assume what you have here actually describe the redistributors,
> and its name should reflect that.

(though you could have a GICv3/v4 system providing a legacy GICv2(m)
compatibility mode having the CPU memory interfaces still defined)

Jon.
Jon Masters Sept. 9, 2014, 6:27 a.m. UTC | #26
On 09/03/2014 10:57 AM, Arnd Bergmann wrote:
> On Wednesday 03 September 2014 11:26:14 Tomasz Nowicki wrote:

> In particular, the ACPI tables describing the irqchip have no way to
> identify the GIC at all, if I read the spec correctly, you have to
> parse the tables, ioremap the registers and then read the ID to know
> if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any "device"
> for the GIC that a hypothetical probe function would be based on.

(aside) I have already noticed this and am separately raising a request
to have this dealt with in the specification at a later time. I believe
it's fairly contrived, since I don't think it'll be at all common to
have a GICv3/v4 IP implementation that has a CPU interface defined.

The problem (not now, but in later implementations that actually have
GICv3/v4 hardware is making assumptions since even a GICv3 or GICv4
system could implement a legacy compatibility mode in which the memory
register interface is defined and mappable so you would be valid in
having defined memory addresses in the MADT linked structures then.

Jon.
Jon Masters Sept. 9, 2014, 6:35 a.m. UTC | #27
On 09/03/2014 02:42 PM, Arnd Bergmann wrote:
> On Monday 01 September 2014 22:57:51 Hanjun Guo wrote:
>> +       /* Collect CPU base addresses */
>> +       count = acpi_parse_entries(sizeof(struct acpi_table_madt),
>> +                                  gic_acpi_parse_madt_cpu, table,
>> +                                  ACPI_MADT_TYPE_GENERIC_INTERRUPT,
>> +                                  ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES);
>> +       if (count < 0) {
>> +               pr_err("Error during GICC entries parsing\n");
>> +               return -EFAULT;
>> +       } else if (!count) {
>> +               /* No GICC entries provided, use address from MADT header */
>> +               struct acpi_table_madt *madt = (struct acpi_table_madt *)table;
>> +
>> +               if (!madt->address)
>> +                       return -EFAULT;
>> +
>> +               cpu_phy_base = (u64)madt->address;
>> +       }
> 
> After I read through ACPI-5.1 section 5.2.12.14, I wonder if this is the
> best way to treat a missing ACPI_MADT_TYPE_GENERIC_INTERRUPT table.
> 
> Do we expect to see those in practice? It seems like using the x86 local
> APIC address as a fallback for the GIC address is not something we
> should do unless we absolutely have to support a system that doesn't
> have the GIC table.

All GICv2 based ACPI systems should define GICCs for the CPU interfaces.

Jon.
Grant Likely Sept. 11, 2014, 11:48 a.m. UTC | #28
On Tue, 02 Sep 2014 13:48:37 +0200, Tomasz Nowicki <tomasz.nowicki@linaro.org> wrote:
> On 01.09.2014 19:35, Marc Zyngier wrote:
> > On 01/09/14 15:57, Hanjun Guo wrote:
> >> From: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> >>
> >> ACPI kernel uses MADT table for proper GIC initialization. It needs to
> >> parse GIC related subtables, collect CPU interface and distributor
> >> addresses and call driver initialization function (which is hardware
> >> 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 <tomasz.nowicki@linaro.org>
> >> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> >> ---
> >>   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/asm/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 <linux/irqdomain.h>
> >>   #include <linux/bootmem.h>
> >>   #include <linux/smp.h>
> >> +#include <linux/irqchip/arm-gic-acpi.h>
> >>
> >>   #include <asm/cputype.h>
> >>   #include <asm/cpu_ops.h>
> >> @@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
> >>   		pr_err("Can't find FADT or error happened during parsing FADT\n");
> >>   }
> >>
> >> +void __init acpi_gic_init(void)
> >> +{
> >> +	struct acpi_table_header *table;
> >> +	acpi_status status;
> >> +	acpi_size tbl_size;
> >> +	int err;
> >> +
> >> +	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");
> >
> > 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 = gic_v3_acpi_init(table);
> 	if (err)
> 		err = 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 <linux/irqchip.h>
> >>   #include <linux/seq_file.h>
> >>   #include <linux/ratelimit.h>
> >> +#include <linux/irqchip/arm-gic-acpi.h>
> >>
> >>   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 logical
> > spot to probe an interrupt controller.
> 
> irqchip.c is OF dependent, I want to decouple these from the very 
> beginning.

It doesn't have to be that way, but given that ARM64 is the only
platform in the foreseeable future that will use ACPI irq setup, it
doesn't make sense to put it in there.

g.
Marc Zyngier Sept. 11, 2014, 12:01 p.m. UTC | #29
Hi Grant,

On 11/09/14 12:48, Grant Likely wrote:
> On Tue, 02 Sep 2014 13:48:37 +0200, Tomasz Nowicki <tomasz.nowicki@linaro.org> wrote:
>> On 01.09.2014 19:35, Marc Zyngier wrote:
>>> On 01/09/14 15:57, Hanjun Guo wrote:
>>>> From: Tomasz Nowicki <tomasz.nowicki@linaro.org>
>>>>
>>>> ACPI kernel uses MADT table for proper GIC initialization. It needs to
>>>> parse GIC related subtables, collect CPU interface and distributor
>>>> addresses and call driver initialization function (which is hardware
>>>> 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 <tomasz.nowicki@linaro.org>
>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>> ---
>>>>   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/asm/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 <linux/irqdomain.h>
>>>>   #include <linux/bootmem.h>
>>>>   #include <linux/smp.h>
>>>> +#include <linux/irqchip/arm-gic-acpi.h>
>>>>
>>>>   #include <asm/cputype.h>
>>>>   #include <asm/cpu_ops.h>
>>>> @@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
>>>>   		pr_err("Can't find FADT or error happened during parsing FADT\n");
>>>>   }
>>>>
>>>> +void __init acpi_gic_init(void)
>>>> +{
>>>> +	struct acpi_table_header *table;
>>>> +	acpi_status status;
>>>> +	acpi_size tbl_size;
>>>> +	int err;
>>>> +
>>>> +	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");
>>>
>>> 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 = gic_v3_acpi_init(table);
>> 	if (err)
>> 		err = 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 <linux/irqchip.h>
>>>>   #include <linux/seq_file.h>
>>>>   #include <linux/ratelimit.h>
>>>> +#include <linux/irqchip/arm-gic-acpi.h>
>>>>
>>>>   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 logical
>>> spot to probe an interrupt controller.
>>
>> irqchip.c is OF dependent, I want to decouple these from the very 
>> beginning.
> 
> It doesn't have to be that way, but given that ARM64 is the only
> platform in the foreseeable future that will use ACPI irq setup, it
> doesn't make sense to put it in there.

I have a different perspective. There is no reason to pollute the arch
code with something that is essentially platform specific.

irqchip_init is the logical place to probe for an irqchip, and I fail to
see the point of sticking this code somewhere else. Why would ACPI be so
special that it requires additional hooks in the arch code?

Thanks,

	M.
Grant Likely Sept. 11, 2014, 1:43 p.m. UTC | #30
On Tue, 02 Sep 2014 14:02:31 +0100, Marc Zyngier <marc.zyngier@arm.com> 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 <tomasz.nowicki@linaro.org>
> >>>
> >>> ACPI kernel uses MADT table for proper GIC initialization. It needs to
> >>> parse GIC related subtables, collect CPU interface and distributor
> >>> addresses and call driver initialization function (which is hardware
> >>> 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 <tomasz.nowicki@linaro.org>
> >>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> >>> ---
> >>>   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/asm/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 <linux/irqdomain.h>
> >>>   #include <linux/bootmem.h>
> >>>   #include <linux/smp.h>
> >>> +#include <linux/irqchip/arm-gic-acpi.h>
> >>>
> >>>   #include <asm/cputype.h>
> >>>   #include <asm/cpu_ops.h>
> >>> @@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
> >>>              pr_err("Can't find FADT or error happened during parsing FADT\n");
> >>>   }
> >>>
> >>> +void __init acpi_gic_init(void)
> >>> +{
> >>> +    struct acpi_table_header *table;
> >>> +    acpi_status status;
> >>> +    acpi_size tbl_size;
> >>> +    int err;
> >>> +
> >>> +    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");
> >>
> >> 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 = gic_v3_acpi_init(table);
> >         if (err)
> >                 err = 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 <linux/irqchip.h>
> >>>   #include <linux/seq_file.h>
> >>>   #include <linux/ratelimit.h>
> >>> +#include <linux/irqchip/arm-gic-acpi.h>
> >>>
> >>>   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 logical
> >> 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 thing
> we support so far. I don't think duplicating the kernel infrastructure
> "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). Just
> put in place a similar probing mechanism, and this will look a lot better.
> 
> >>
> >>>      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 <linux/of.h>
> >>>   #include <linux/of_address.h>
> >>>   #include <linux/of_irq.h>
> >>> +#include <linux/acpi.h>
> >>>   #include <linux/irqdomain.h>
> >>>   #include <linux/interrupt.h>
> >>>   #include <linux/percpu.h>
> >>>   #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>
> >>> @@ -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 = ULONG_MAX;
> >>
> >> Please use phys_addr_t for physical addresses. The use of ULONG_MAX
> >> 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 = (struct acpi_madt_generic_interrupt *)header;
> >>> +
> >>> +    if (BAD_MADT_ENTRY(processor, end))
> >>> +            return -EINVAL;
> >>> +
> >>> +    gic_cpu_base = 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 ACPI spec.
> >>> +     * All CPU interface addresses have to be the same.
> >>> +     */
> >>> +    if (cpu_phy_base != ULONG_MAX && gic_cpu_base != cpu_phy_base)
> >>> +            return -EFAULT;
> >>> +
> >>> +    cpu_phy_base = gic_cpu_base;
> >>> +    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;
> >>> +    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 = acpi_parse_entries(sizeof(struct acpi_table_madt),
> >>> +                               gic_acpi_parse_madt_cpu, table,
> >>> +                               ACPI_MADT_TYPE_GENERIC_INTERRUPT,
> >>> +                               ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES);
> >>> +    if (count < 0) {
> >>> +            pr_err("Error during GICC entries parsing\n");
> >>> +            return -EFAULT;
> >>> +    } else if (!count) {
> >>> +            /* No GICC entries provided, use address from MADT header */
> >>> +            struct acpi_table_madt *madt = (struct acpi_table_madt *)table;
> >>> +
> >>> +            if (!madt->address)
> >>> +                    return -EFAULT;
> >>> +
> >>> +            cpu_phy_base = (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 = acpi_parse_entries(sizeof(struct acpi_table_madt),
> >>> +                               gic_acpi_parse_madt_distributor, table,
> >>> +                               ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR,
> >>> +                               ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES);
> >>> +    if (count <= 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 = 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_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 GSI 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 <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_
> >>> +
> >>> +#include <linux/acpi.h>
> >>
> >> 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
> > <linux/acpi.h>. 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) from?
> 
> >>
> >>> +#define ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES    1
> >>> +
> >>> +/*
> >>> + * 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_GIC_DIST_MEM_SIZE              (SZ_64K)
> >>
> >> I don't know which version of the spec you're looking at, but my version
> >> of the GICv2 spec has a 4kB distributor. Also, it would be good to make
> >> obvious which GIC version this define is about.
> > OK
> > 
> >>
> >>> +#define ACPI_GIC_CPU_IF_MEM_SIZE    (SZ_8K)
> >>> +
> >>> +void acpi_gic_init(void);
> >>> +int gic_v2_acpi_init(struct acpi_table_header *table);
> >>> +#else
> >>> +static inline void acpi_gic_init(void) { }
> >>> +#endif
> >>> +
> >>> +#endif /* ARM_GIC_ACPI_H_ */
> >>>
> >>
> >> In the end, why do we need a separate file for this? I cannot see
> >> anything that prevents it from being merged with arm-gic.h.
> >>
> >> Thanks,
> >>
> >>       M.
> >>
> > Having only GICv2, it would work. Considering we would do the same for
> > GICv3 (arm-gic-v3.h) there will be register name conflicts for both
> > headers inclusion:
> > 
> > [...]
> > #include <linux/irqchip/arm-gic.h>
> > #include <linux/irqchip/arm-gic-v3.h>
> > [...]
> >         err = gic_v3_acpi_init(table);
> >         if (err)
> >                 err = gic_v2_acpi_init(table);
> >         if (err)
> >                 pr_err("Failed to initialize GIC IRQ controller");
> > [...]
> > So instead of changing register names prefix, I choose new header will
> > be less painfully.
> 
> Yes, and this is exactly why I pushed back on that last time. I'll
> continue saying that interrupt controllers should be self-probing, with
> ACPI as they are with DT.

We are in a completely different situation with ACPI compared to DT. In
ACPI there is currently exactly one interrupt controller that is
allowed (GICv2). In the near future there will be a second one (GICv3).
There is zero need to have a infrastructure to abstract a pluggable
interface for registering interrupt controllers.

With DT we've got a tonne of controllers that we need to support. The
pluggable infrastructure was created because we needed it for
maintenance.

All we're really talking about here is a table of registered irq
controllers vs. a few lines of if/else if/else initialization code. The
moment it gets unwieldy, we will refactor it. Until then, the if/else
block is better code.

As for putting the ACPI hook into irqchip_init(), I don't doing so buys
us anything. Common code is good when it is common, but in this case
there is no other platform is going to use this ACPI irq initialization.
Not x86, not ia64. It is ARM64 specific code and moving it out from
arm64 doesn't make sense.

I'm not going to grump and complain if Hanjun decides to move it into
irqchip_init() anyway, but, I don't think this is an issue that should
block the patch series.

g.

> 
> Even with the restrictions of ACPI and SBSA, we end-up with at least 2
> main families of interrupt controllers (GICv2 and GICv3), both with a
> number of "interesting" variations (GICv2m and GICv4, to only mention
> those I'm directly involved with).
> 
> I can safely predict that the above will become a tangled mess within 18
> months, and the idea of littering the arch code with a bunch of
> hardcoded "if (blah())" doesn't fill me with joy and confidence.
> 
> In summary: we have the infrastructure already, just use it.
> 
> Thanks,
> 
> 	M.
> -- 
> Jazz is not dead. It just smells funny...
diff mbox

Patch

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/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 <linux/irqdomain.h>
 #include <linux/bootmem.h>
 #include <linux/smp.h>
+#include <linux/irqchip/arm-gic-acpi.h>
 
 #include <asm/cputype.h>
 #include <asm/cpu_ops.h>
@@ -313,6 +314,28 @@  void __init acpi_boot_table_init(void)
 		pr_err("Can't find FADT or error happened during parsing FADT\n");
 }
 
+void __init acpi_gic_init(void)
+{
+	struct acpi_table_header *table;
+	acpi_status status;
+	acpi_size tbl_size;
+	int err;
+
+	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);
+}
+
 /*
  * 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 <linux/irqchip.h>
 #include <linux/seq_file.h>
 #include <linux/ratelimit.h>
+#include <linux/irqchip/arm-gic-acpi.h>
 
 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();
+
 	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 <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
+#include <linux/acpi.h>
 #include <linux/irqdomain.h>
 #include <linux/interrupt.h>
 #include <linux/percpu.h>
 #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>
@@ -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 = ULONG_MAX;
+
+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;
+
+	processor = (struct acpi_madt_generic_interrupt *)header;
+
+	if (BAD_MADT_ENTRY(processor, end))
+		return -EINVAL;
+
+	gic_cpu_base = processor->base_address;
+	if (!gic_cpu_base)
+		return -EFAULT;
+
+	/*
+	 * There is no support for non-banked GICv1/2 register in ACPI spec.
+	 * All CPU interface addresses have to be the same.
+	 */
+	if (cpu_phy_base != ULONG_MAX && gic_cpu_base != cpu_phy_base)
+		return -EFAULT;
+
+	cpu_phy_base = gic_cpu_base;
+	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;
+	if (!dist_phy_base)
+		return -EFAULT;
+
+	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 = acpi_parse_entries(sizeof(struct acpi_table_madt),
+				   gic_acpi_parse_madt_cpu, table,
+				   ACPI_MADT_TYPE_GENERIC_INTERRUPT,
+				   ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES);
+	if (count < 0) {
+		pr_err("Error during GICC entries parsing\n");
+		return -EFAULT;
+	} else if (!count) {
+		/* No GICC entries provided, use address from MADT header */
+		struct acpi_table_madt *madt = (struct acpi_table_madt *)table;
+
+		if (!madt->address)
+			return -EFAULT;
+
+		cpu_phy_base = (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 = acpi_parse_entries(sizeof(struct acpi_table_madt),
+				   gic_acpi_parse_madt_distributor, table,
+				   ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR,
+				   ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES);
+	if (count <= 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 = 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_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 GSI 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 <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_
+
+#include <linux/acpi.h>
+
+#ifdef CONFIG_ACPI
+#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES	65535
+#define ACPI_MAX_GIC_DISTRIBUTOR_ENTRIES	1
+
+/*
+ * 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_GIC_DIST_MEM_SIZE		(SZ_64K)
+#define ACPI_GIC_CPU_IF_MEM_SIZE	(SZ_8K)
+
+void acpi_gic_init(void);
+int gic_v2_acpi_init(struct acpi_table_header *table);
+#else
+static inline void acpi_gic_init(void) { }
+#endif
+
+#endif /* ARM_GIC_ACPI_H_ */