On Mon, Sep 01, 2014 at 03:57:40PM +0100, Hanjun Guo wrote:

diff --git a/arch/arm64/include/asm/acenv.h b/arch/arm64/include/asm/acenv.h new file mode 100644 index 0000000..3899ee6 --- /dev/null +++ b/arch/arm64/include/asm/acenv.h @@ -0,0 +1,18 @@ +/*

• • ARM64 specific ACPICA environments and implementation
• • Copyright (C) 2014, Linaro Ltd.
• • Author: Hanjun Guo hanjun.guo@linaro.org
• • Author: Graeme Gregory graeme.gregory@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 _ASM_ACENV_H +#define _ASM_ACENV_H

+#define ACPI_FLUSH_CPU_CACHE() WARN_ONCE(1, "Not currently supported on ARM64")

Does this mean that it will be supported at some point? Looking at the places where this function is called, I don't really see how this would ever work on ARM. Which means that we add such macro just to be able to compile code that would never be used on arm64. I would rather see the relevant ACPI files only compiled on x86/IA-64 rather than arm64.

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c new file mode 100644 index 0000000..9252f72 --- /dev/null +++ b/arch/arm64/kernel/acpi.c

[...]

+/*

• • acpi_boot_table_init() called from setup_arch(), always.
• • 1. find RSDP and get its address, and then find XSDT
• • 2. extract all tables and checksums them all
• • We can parse ACPI boot-time tables such as MADT after
• • this function is called.
• */

+void __init acpi_boot_table_init(void) +{

• /* If acpi_disabled, bail out */
• if (acpi_disabled)

• return;
  

So at this point, there wouldn't be anything yet to set acpi_disabled to 1, unless !CONFIG_ACPI.

• /* Initialize the ACPI boot-time table parser. */
• if (acpi_table_init())

• disable_acpi();
  

Do you need anything more to add to this function later? Otherwise, just call it in setup_arch() directly (maybe dependent on IS_ENABLED(CONFIG_ACPI) or just create an equivalent dummy acpi_table_init() when not configured).

diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index c96172a..fb7cc0e 100644 --- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -43,6 +43,7 @@ #include <linux/of_fdt.h> #include <linux/of_platform.h> #include <linux/efi.h> +#include <linux/acpi.h> #include <asm/fixmap.h> #include <asm/cpu.h> @@ -385,6 +386,9 @@ void __init setup_arch(char **cmdline_p) efi_init(); arm64_memblock_init();

• /* Parse the ACPI tables for possible boot-time configuration */
• acpi_boot_table_init();

Is there any dummy acpi_boot_table_init() when !CONFIG_ACPI? I couldn't see one in this patch, so I don't see how this would compile when ACPI is disabled.

On 2014/9/10 0:26, Catalin Marinas wrote:

On Mon, Sep 01, 2014 at 03:57:40PM +0100, Hanjun Guo wrote:

...

diff --git a/arch/arm64/include/asm/acenv.h b/arch/arm64/include/asm/acenv.h new file mode 100644 index 0000000..3899ee6 --- /dev/null +++ b/arch/arm64/include/asm/acenv.h @@ -0,0 +1,18 @@ +/*

• • ARM64 specific ACPICA environments and implementation
• • Copyright (C) 2014, Linaro Ltd.
• • Author: Hanjun Guo hanjun.guo@linaro.org
• • Author: Graeme Gregory graeme.gregory@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 _ASM_ACENV_H +#define _ASM_ACENV_H

+#define ACPI_FLUSH_CPU_CACHE() WARN_ONCE(1, "Not currently supported on ARM64")

Does this mean that it will be supported at some point? Looking at the places where this function is called, I don't really see how this would ever work on ARM. Which means that we add such macro just to be able to compile code that would never be used on arm64. I would rather see the relevant ACPI files only compiled on x86/IA-64 rather than arm64.

...

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c new file mode 100644 index 0000000..9252f72 --- /dev/null +++ b/arch/arm64/kernel/acpi.c

[...]

...

+/*

• • acpi_boot_table_init() called from setup_arch(), always.
• • 1. find RSDP and get its address, and then find XSDT
• • 2. extract all tables and checksums them all
• • We can parse ACPI boot-time tables such as MADT after
• • this function is called.
• */

+void __init acpi_boot_table_init(void) +{

• /* If acpi_disabled, bail out */
• if (acpi_disabled)

• return;
  

So at this point, there wouldn't be anything yet to set acpi_disabled to 1, unless !CONFIG_ACPI.

...

• /* Initialize the ACPI boot-time table parser. */
• if (acpi_table_init())

• disable_acpi();
  

Do you need anything more to add to this function later? Otherwise, just call it in setup_arch() directly (maybe dependent on IS_ENABLED(CONFIG_ACPI) or just create an equivalent dummy acpi_table_init() when not configured).

We add FADT parse function to this function later.

...

diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index c96172a..fb7cc0e 100644 --- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -43,6 +43,7 @@ #include <linux/of_fdt.h> #include <linux/of_platform.h> #include <linux/efi.h> +#include <linux/acpi.h> #include <asm/fixmap.h> #include <asm/cpu.h> @@ -385,6 +386,9 @@ void __init setup_arch(char **cmdline_p) efi_init(); arm64_memblock_init();

• /* Parse the ACPI tables for possible boot-time configuration */
• acpi_boot_table_init();

Is there any dummy acpi_boot_table_init() when !CONFIG_ACPI? I couldn't see one in this patch, so I don't see how this would compile when ACPI is disabled.

There is a stub function in linux/acpi.h when ACPI is disabled.

Thanks Hanjun

On Mon, Sep 01, 2014 at 03:57:42PM +0100, Hanjun Guo wrote:

--- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -74,3 +74,18 @@ void __init acpi_boot_table_init(void)

• TBD when ARM/ARM64 starts to support suspend...

*/ int (*acpi_suspend_lowlevel)(void) = NULL;

+static int __init parse_acpi(char *arg) +{

• if (!arg)

• return -EINVAL;
  

• /* "acpi=off" disables both ACPI table parsing and interpreter */
• if (strcmp(arg, "off") == 0)

• disable_acpi();
  

• else

• return -EINVAL;	/* Core will print when we return error */
  

• return 0;

+} +early_param("acpi", parse_acpi);

I forgot about early param, so there is a way to set acpi_disabled to 1 before populating the tables.

Hi Lorenzo,

On 2014年09月04日 01:21, Lorenzo Pieralisi wrote:

On Mon, Sep 01, 2014 at 03:57:47PM +0100, Hanjun Guo wrote:

...

MADT contains the information for MPIDR which is essential for SMP initialization, parse the GIC cpu interface structures to get the MPIDR value and map it to cpu_logical_map(), and add enabled cpu with valid MPIDR into cpu_possible_map.

ACPI 5.1 only has two explicit methods to boot up SMP, PSCI and Parking protocol, but the Parking protocol is only specified for ARMv7 now, so make PSCI as the only way for the SMP boot protocol before some updates for the ACPI spec or the Parking protocol spec.

[...]

...

int acpi_noirq; /* skip ACPI IRQ initialization */ int acpi_disabled; EXPORT_SYMBOL(acpi_disabled); @@ -31,6 +35,8 @@ EXPORT_SYMBOL(acpi_disabled); int acpi_pci_disabled; /* skip ACPI PCI scan and IRQ initialization */ EXPORT_SYMBOL(acpi_pci_disabled); +static int enabled_cpus; /* Processors (GICC) with enabled flag in MADT */

Will this be ever different from (num_possible_cpus() - 1) ?

Yes, num_possible_cpus() will much more than enabled cpus in MADT, when ACPI based CPU hot plug is introduced, you can refer to the code in x86.

...

/*

• __acpi_map_table() will be called before page_init(), so early_ioremap()
• or early_memremap() should be called here to for ACPI table mapping.

@@ -51,6 +57,144 @@ void __init __acpi_unmap_table(char *map, unsigned long size) early_memunmap(map, size); } +/**

• • acpi_map_gic_cpu_interface - generates a logical cpu number
• • and map to MPIDR represented by GICC structure
• • @mpidr: CPU's hardware id to register, MPIDR represented in MADT
• • @enabled: this cpu is enabled or not
• • Returns the logical cpu number which maps to MPIDR
• */

+static int acpi_map_gic_cpu_interface(u64 mpidr, u8 enabled) +{

• int cpu;
• if (mpidr == INVALID_HWID) {

• pr_info("Skip invalid cpu hardware ID\n");
  

• return -EINVAL;
  

• }
• total_cpus++;

What's this used for ?

It is for all the CPU entries in MADT table, it is used to let people know how many CPUs in MADT (enabled and disabled).

...

• if (!enabled)

• return -EINVAL;
  

• if (enabled_cpus >= NR_CPUS) {

• pr_warn("NR_CPUS limit of %d reached, Processor %d/0x%llx ignored.\n",
  

• 	NR_CPUS, total_cpus, mpidr);
  

• return -EINVAL;
  

• }
• /* No need to check duplicate MPIDRs for the first CPU */
• if (enabled_cpus) {

• /*
  

•  * Duplicate MPIDRs are a recipe for disaster. Scan
  

•  * all initialized entries and check for
  

•  * duplicates. If any is found just ignore the CPU.
  

•  */
  

• for_each_possible_cpu(cpu) {
  

• 	if (cpu_logical_map(cpu) == mpidr) {
  

• 		pr_err("Firmware bug, duplicate CPU MPIDR: 0x%llx in MADT\n",
  

• 		mpidr);
  

• 		return -EINVAL;
  

• 	}
  

• }
  

• } else {

• /* Fist GICC entry must be BSP as ACPI spec said */
  

s/Fist/First/

...

• if  (cpu_logical_map(0) != mpidr) {
  

• 	pr_err("First GICC entry is not BSP for MPIDR 0x%llx\n",
  

• 	       mpidr);
  

• 	return -EINVAL;
  

• }
  

Interesting, this means that if I want to change the boot CPU I have to recompile the ACPI tables. Is that really true ?

No, you needn't. there is a logic problem here, we just need to print some message here and continue, OS will still ok with that.

...

• }
• /* allocate a logical cpu id for the new comer */
• if (cpu_logical_map(0) == mpidr) {

• /*
  

•  * boot_cpu_init() already hold bit 0 in cpu_present_mask
  

•  * for BSP, no need to allocate again.
  

•  */
  

• cpu = 0;
  

• } else {

• cpu = cpumask_next_zero(-1, cpu_possible_mask);
  

• }

You may use a ternary operator, more compact and clearer.

BTW you seem to be contradicting yourself. On one hand you keep a counter for enabled_cpus, and then use cpu_possible_mask to allocate a logical cpu id. Make a decision, either you use a counter or you use cpu_possible_mask and its bitweight.

ok.

...

• /*

• * ACPI 5.1 only has two explicit methods to boot up SMP,
  

• * PSCI and Parking protocol, but the Parking protocol is
  

• * only specified for ARMv7 now, so make PSCI as the only
  

• * way for the SMP boot protocol before some updates for
  

• * the ACPI spec or the Parking protocol spec.
  

• */
  

• if (!acpi_psci_present()) {

• pr_warn("CPU %d has no PSCI support, will not boot\n", cpu);
  

• return -EOPNOTSUPP;
  

• }

This check really does not belong here. You do not even start parsing the gic cpu interfaces if psci is missing or I am missing something myself. Anyway, this check must not be in this function.

I agree with you, i will update the patch.

...

• /* Get cpu_ops include the boot CPU */
• cpu_ops[cpu] = cpu_get_ops("psci");
• if (!cpu_ops[cpu])

• return -EINVAL;
  

• /* CPU 0 was already initialized */
• if (cpu) {

• if (cpu_ops[cpu]->cpu_init(NULL, cpu))
  

• 	return -EOPNOTSUPP;
  

• /* map the logical cpu id to cpu MPIDR */
  

• cpu_logical_map(cpu) = mpidr;
  

• set_cpu_possible(cpu, true);
  

• }
• enabled_cpus++;

See above to me enabled_cpus and (num_possible_cpus() - 1) are identical.

I think I need to remove all the CPU hotplug related code and make this function as simple as possible and introduce them when needed.

...

• return cpu;

+}

+static int __init +acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header,

• 		const unsigned long end)
  

+{

• struct acpi_madt_generic_interrupt *processor;
• processor = (struct acpi_madt_generic_interrupt *)header;
• if (BAD_MADT_ENTRY(processor, end))

• return -EINVAL;
  

• acpi_table_print_madt_entry(header);
• acpi_map_gic_cpu_interface(processor->arm_mpidr,

• processor->flags & ACPI_MADT_ENABLED);
  

Ehm. You must check the return value here right (and return an error if that's an error, otherwise the count value below can be botched ?!).

Or you do not consider a parsing error as an error and want to keep parsing remaining GIC CPU IF entries ?

yes, this is my intension. we can skip the error ones and boot other CPUs which have no errors.

...

• return 0;

+}

+/* Parse GIC cpu interface entries in MADT for SMP init */ +void __init acpi_smp_init_cpus(void) +{

• int count;
• /*

• * do a partial walk of MADT to determine how many CPUs
  

• * we have including disabled CPUs, and get information
  

• * we need for SMP init
  

• */
  

• count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT,

• 	acpi_parse_gic_cpu_interface,
  

• 	ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES);
  

• if (!count) {

• pr_err("No GIC CPU interface entries present\n");
  

• return;
  

• } else if (count < 0) {

• pr_err("Error parsing GIC CPU interface entry\n");
  

• return;
  

• }

What would you consider an error ? A single GIC CPU IF entry error ?

could you please explain it in detail? I can't catch up with you, my apologizes.

Thanks Hanjun

Hi Hanjun,

On Mon, Sep 01, 2014 at 03:57:48PM +0100, Hanjun Guo wrote:

Introduce a new function map_gicc_mpidr() to allow MPIDRs to be obtained from the GICC Structure introduced by ACPI 5.1.

MPIDR is the CPU hardware ID as local APIC ID on x86 platform, so we use MPIDR not the GIC CPU interface ID to identify CPUs.

Signed-off-by: Hanjun Guo hanjun.guo@linaro.org

arch/arm64/include/asm/acpi.h | 32 ++++++++++++++++++++++++++++++++ arch/arm64/kernel/acpi.c | 1 - drivers/acpi/processor_core.c | 37 +++++++++++++++++++++++++++++++++++++ 3 files changed, 69 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index e013dbb..a867467 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -12,6 +12,8 @@ #ifndef _ASM_ACPI_H #define _ASM_ACPI_H +#include <asm/smp_plat.h>

/* Basic configuration for ACPI */ #ifdef CONFIG_ACPI #define acpi_strict 1 /* No out-of-spec workarounds on ARM64 */ @@ -38,6 +40,36 @@ static inline void disable_acpi(void) acpi_noirq = 1; } +/* MPIDR value provided in GICC structure is 64 bits, but

• • the acpi processor driver use the 32 bits cpu hardware
• • ID (apic_id on intel platform) everywhere, it is pretty
• • hard to modify the acpi processor driver to accept the
• • 64 bits MPIDR value, at the same time, only 32 bits of
• • the MPIDR is used in the 64 bits MPIDR, just pack the
• • Affx fields into a single 32 bit identifier to accommodate
• • the acpi processor drivers.
• */

I have comments on the code in this patch, but they are not the most important point. What I am really worried about, it is that as ARM, I do not want to know what an apic_id is. This code is *supposed* to be generic and yet it is chock-full of x86 specific stuff and you are trying to make ARM HW concepts fit with x86 ones, and I am not happy with that.

To be clearer, why does not this look-up of:

logical-cpu-index -> physical-cpu-index

is not carried out using the acpi_id ? Every architecture will have to add arch specific code to carry out the reverse look-up:

acpi_id -> apic_id (x86) acpi_id -> mpidr_el1 (arm64)

and the code would end up being split in a nice way. On top of that, I wonder why ACPI structures like eg struct acpi_processor contain x86 specific data (ie apic_id). I know it is a HW identifier as the MPIDR_EL1 is on arm64, but I do not want to deal with that in generic ACPI code because that's not generic at ALL.

What if another architecture wants to use ACPI ? Are we going to map its HW CPU identifier to an apic_id only because that's what x86 requires ?

I am sorry I do not like that. I understand it is easier to map ARM code to existing ACPI structures but I feel we will run into issues very soon because of that.

Is it that complex to remove the apic_id dependency in *generic* ACPI code and replace it with functions that hook into arch specific code to carry out the logical to physical cpu mappings ?

I understand this is harder to do, but it will make your life easier in the long run. I am thinking of other pieces of code like the supposedly generic ACPI CPUidle driver, where we *still* depend on the apic to detect idle states, this is not going to fly, I am sorry, we need to have code that has a chance to be generic from the beginning not as an afterthought.

Lorenzo

+static inline u32 pack_mpidr_into_32_bits(u64 mpidr) +{

• /*

• * Bits [0:7] Aff0;
  

• * Bits [8:15] Aff1;
  

• * Bits [16:23] Aff2;
  

• * Bits [32:39] Aff3;
  

• */
  

• return (u32) ((mpidr & 0xff00000000) >> 8) | mpidr;

+}

+/*

• • The ACPI processor driver for ACPI core code needs this macro
• • to find out this cpu was already mapped (mapping from CPU hardware
• • ID to CPU logical ID) or not.
• • cpu_logical_map(cpu) is the mapping of MPIDR and the logical cpu,
• • and MPIDR is the cpu hardware ID we needed to pack.
• */

+#define cpu_physical_id(cpu) pack_mpidr_into_32_bits(cpu_logical_map(cpu))

/*

• It's used from ACPI core in kdump to boot UP system with SMP kernel,
• with this check the ACPI core will not override the CPU index

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index fbaaf01..35dff11 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -24,7 +24,6 @@ #include <linux/bootmem.h> #include <linux/smp.h> -#include <asm/smp_plat.h> #include <asm/cputype.h> #include <asm/cpu_ops.h> diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c index e32321c..4007313 100644 --- a/drivers/acpi/processor_core.c +++ b/drivers/acpi/processor_core.c @@ -64,6 +64,38 @@ static int map_lsapic_id(struct acpi_subtable_header *entry, return 0; } +/*

• • On ARM platform, MPIDR value is the hardware ID as apic ID
• • on Intel platforms
• */

+static int map_gicc_mpidr(struct acpi_subtable_header *entry,

• int device_declaration, u32 acpi_id, int *mpidr)
  

+{

• struct acpi_madt_generic_interrupt *gicc =

•    container_of(entry, struct acpi_madt_generic_interrupt, header);
  

• if (!(gicc->flags & ACPI_MADT_ENABLED))

• return -ENODEV;
  

• /* In the GIC interrupt model, logical processors are

• * required to have a Processor Device object in the DSDT,
  

• * so we should check device_declaration here
  

• */
  

• if (device_declaration && (gicc->uid == acpi_id)) {

• /*
  

•  * Only bits [0:7] Aff0, bits [8:15] Aff1, bits [16:23] Aff2
  

•  * and bits [32:39] Aff3 are meaningful, so pack the Affx
  

•  * fields into a single 32 bit identifier to accommodate the
  

•  * acpi processor drivers.
  

•  */
  

• *mpidr = ((gicc->arm_mpidr & 0xff00000000) >> 8)
  

• 	 | gicc->arm_mpidr;
  

• return 0;
  

• }
• return -EINVAL;

+}

static int map_madt_entry(int type, u32 acpi_id) { unsigned long madt_end, entry; @@ -99,6 +131,9 @@ static int map_madt_entry(int type, u32 acpi_id) } else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC) { if (!map_lsapic_id(header, type, acpi_id, &apic_id)) break;

• } else if (header->type == ACPI_MADT_TYPE_GENERIC_INTERRUPT) {
  

• 	if (!map_gicc_mpidr(header, type, acpi_id, &apic_id))
  

• 		break;
  

  } entry += header->length; }

@@ -131,6 +166,8 @@ static int map_mat_entry(acpi_handle handle, int type, u32 acpi_id) map_lsapic_id(header, type, acpi_id, &apic_id); } else if (header->type == ACPI_MADT_TYPE_LOCAL_X2APIC) { map_x2apic_id(header, type, acpi_id, &apic_id);

• } else if (header->type == ACPI_MADT_TYPE_GENERIC_INTERRUPT) {

• map_gicc_mpidr(header, type, acpi_id, &apic_id);
  

  }

exit: -- 1.7.9.5

On Thu, Sep 11, 2014 at 12:08 PM, Grant Likely grant.likely@linaro.org wrote:

On Mon, 1 Sep 2014 22:57:49 +0800, Hanjun Guo hanjun.guo@linaro.org wrote:

...

Introduce ACPI_IRQ_MODEL_GIC which is needed for ARM64 as GIC is used, and then register device's gsi with the core IRQ subsystem.

acpi_register_gsi() is similar to DT based irq_of_parse_and_map(), since gsi is unique in the system, so use hwirq number directly for the mapping.

Originally-by: Amit Daniel Kachhap amit.daniel@samsung.com Signed-off-by: Hanjun Guo hanjun.guo@linaro.org

---

arch/arm64/kernel/acpi.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++ drivers/acpi/bus.c | 3 ++ include/linux/acpi.h | 1 + 3 files changed, 77 insertions(+)

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 35dff11..354b912 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -37,6 +37,12 @@ EXPORT_SYMBOL(acpi_pci_disabled); static int enabled_cpus; /* Processors (GICC) with enabled flag in MADT */

/*

• • Since we're on ARM, the default interrupt routing model
• • clearly has to be GIC.
• */

+enum acpi_irq_model_id acpi_irq_model = ACPI_IRQ_MODEL_GIC;

+/*

• __acpi_map_table() will be called before page_init(), so early_ioremap()
• or early_memremap() should be called here to for ACPI table mapping.

*/ @@ -194,6 +200,73 @@ void __init acpi_smp_init_cpus(void) pr_info("%d CPUs enabled, %d CPUs total\n", enabled_cpus, total_cpus); }

+int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) +{

• *irq = irq_find_mapping(NULL, gsi);
  

• return 0;
  

+} +EXPORT_SYMBOL_GPL(acpi_gsi_to_irq);

Why is this exported? x86 exports it, but ia64 does not. There aren't very many callers, and none of them can be built as a module AFAICS.

...

+/*

• • success: return IRQ number (>0)
• • failure: return =< 0
• */

+int acpi_register_gsi(struct device *dev, u32 gsi, int trigger, int polarity) +{

• unsigned int irq;
  

• unsigned int irq_type;
  

• /*
  

•  * ACPI have no bindings to indicate SPI or PPI, so we
  

•  * use different mappings from DT in ACPI.
  

•  *
  

•  * For FDT
  

•  * PPI interrupt: in the range [0, 15];
  

•  * SPI interrupt: in the range [0, 987];
  

•  *
  

•  * For ACPI, GSI should be unique so using
  

•  * the hwirq directly for the mapping:
  

•  * PPI interrupt: in the range [16, 31];
  

•  * SPI interrupt: in the range [32, 1019];
  

•  */
  

Hmmm, so doing it this way means that DT systems will have a different irq_domain setup compared with ACPI systems. I'm not convinced we want to do that, but I need to look at the code that sets up the new domains before I comment further...

Okay, nevermind. I looked at the setup code. This isn't an issue of the irq domain being set up differently, but rather the binding translation operates differently between DT and ACPI. ACPI used a single integer encapsulating PPI and SPI which just happens to line up with the hwirq numbers, whereas DT uses a [type,number] tuple that needs translating into the hwirq. This code is fine.

g.

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.

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

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

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

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

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