[PATCH v4 00/18] Introduce ACPI for ARM64 based on ACPI 5.1

[PATCH v4 00/18] Introduce ACPI for ARM64 based on ACPI 5.1

[Hanjun Guo]

ACPI 5.1 has been released and now be freely available for
download [1]. It fixed some major gaps to run ACPI on ARM,
this patch just follow the ACPI 5.1 spec and prepare the
code to run ACPI on ARM64.

ACPI 5.1 has some major changes for the following tables and
method which are essential for ARM platforms:
1) MADT table updates.
2) FADT updates for PSCI
3) GTDT

This patch set is the ARM64 ACPI core patches covered MADT, FADT
and GTDT, platform board specific drivers are not covered by this
patch set, but we provided drivers for Juno to boot with ACPI only
for review purpose.

We first introduce acpi.c and its related head file which are needed
by ACPI core, and then get RSDP to extract all the ACPI boot-time tables.
When all the boot-time tables (FADT, MADT, GTDT) are ready, then
parse them to init the sytem when booted. Specifically, 
a) we use FADT to init PSCI and use PSCI to boot SMP;
b) Use MADT for GIC init and SMP init;
c) GTDT for arch timer init.

This patch set is based on 3.17-rc4 and was tested by Graeme on Juno
and FVP base model boot with ACPI only OK, if you want to test them,
you can pull from acpi-5.1-v4 branch in leg/acpi repo:
git://git.linaro.org/leg/acpi/acpi.git

Updates since v3:
 - Compile out sleep.c on ARM64 when ACPI enabled
 - refactor the GIC init code to address the comments from Marc and
   Arnd
 - refactor the SMP init code to fix some logic problem when PSCI is
   not present, also address some of Grant and Lorenzo's comments
 - reorder the patch series and move ACPI table changes to the front
   of patch set
 - rebase on top of 3.17-rc4

Updates since v2:
 - Refactor the code to make SMP/PSCI init with less sperated init
   path by Tomasz
 - make ACPI depend on EXPERT
 - Address lots of comments from Catalin, Sudeep, Geoff
 - Add Juno device ACPI driver patches for review

Updates since v1:
 - Set ACPI default off on ARM64 suggested by Olof;
 - Rebase the patch set on top of linux-next branch/linux-pm tree which
   includes the ACPICA for full ACPI 5.1 support.
 - Update the document as suggested;
 - Adress lots of comments from Mark, Sudeep, Randy, Naresh, Olof, Geoff
   and more...

[1]: http://www.uefi.org/sites/default/files/resources/ACPI_5_1release.pdf

Al Stone (3):
  ARM64 / ACPI: Get RSDP and ACPI boot-time tables
  ARM64 / ACPI: Introduce early_param for "acpi"
  ARM64 / ACPI: Select ACPI_REDUCED_HARDWARE_ONLY if ACPI is enabled on
    ARM64

Ashwin Chaugule (1):
  ACPI / table: Add new function to get table entries

Graeme Gregory (4):
  ARM64 / ACPI: Introduce sleep-arm.c
  ARM64 / ACPI: If we chose to boot from acpi then disable FDT
  ARM64 / ACPI: Enable ARM64 in Kconfig
  Documentation: ACPI for ARM64

Hanjun Guo (8):
  ARM64: Move the init of cpu_logical_map(0) before
    unflatten_device_tree()
  ARM64 / ACPI: Make PCI optional for ACPI on ARM64
  ARM64 / ACPI: Parse FADT table to get PSCI flags for PSCI init
  ACPI / table: Print GIC information when MADT is parsed
  ARM64 / ACPI: Parse MADT for SMP initialization
  ACPI / processor: Make it possible to get CPU hardware ID via GICC
  ARM64 / ACPI: Introduce ACPI_IRQ_MODEL_GIC and register device's gsi
  ARM64 / ACPI: Parse GTDT to initialize arch timer

Tomasz Nowicki (2):
  ACPI / table: Count matched and successfully parsed entries without
    specifying max entries
  ARM64 / ACPI: Add GICv2 specific ACPI boot support

 Documentation/arm64/arm-acpi.txt     |  218 +++++++++++++++++++++
 Documentation/kernel-parameters.txt  |    3 +-
 arch/arm64/Kconfig                   |    3 +
 arch/arm64/include/asm/acenv.h       |   18 ++
 arch/arm64/include/asm/acpi.h        |   96 ++++++++++
 arch/arm64/include/asm/cpu_ops.h     |    1 +
 arch/arm64/include/asm/pci.h         |   11 ++
 arch/arm64/include/asm/psci.h        |    3 +-
 arch/arm64/include/asm/smp.h         |    5 +-
 arch/arm64/kernel/Makefile           |    1 +
 arch/arm64/kernel/acpi.c             |  351 ++++++++++++++++++++++++++++++++++
 arch/arm64/kernel/cpu_ops.c          |    4 +-
 arch/arm64/kernel/psci.c             |   78 +++++---
 arch/arm64/kernel/setup.c            |   23 ++-
 arch/arm64/kernel/smp.c              |    2 +-
 arch/arm64/kernel/time.c             |    7 +
 drivers/acpi/Kconfig                 |    6 +-
 drivers/acpi/Makefile                |    6 +-
 drivers/acpi/bus.c                   |    3 +
 drivers/acpi/internal.h              |    5 +
 drivers/acpi/processor_core.c        |   37 ++++
 drivers/acpi/sleep-arm.c             |   28 +++
 drivers/acpi/tables.c                |  115 +++++++++--
 drivers/clocksource/arm_arch_timer.c |  117 ++++++++++--
 drivers/irqchip/irq-gic.c            |  106 ++++++++++
 drivers/irqchip/irqchip.c            |    3 +
 include/linux/acpi.h                 |    5 +
 include/linux/clocksource.h          |    6 +
 include/linux/irqchip/arm-gic-acpi.h |   31 +++
 include/linux/pci.h                  |   37 +++-
 30 files changed, 1238 insertions(+), 91 deletions(-)
 create mode 100644 Documentation/arm64/arm-acpi.txt
 create mode 100644 arch/arm64/include/asm/acenv.h
 create mode 100644 arch/arm64/include/asm/acpi.h
 create mode 100644 arch/arm64/include/asm/pci.h
 create mode 100644 arch/arm64/kernel/acpi.c
 create mode 100644 drivers/acpi/sleep-arm.c
 create mode 100644 include/linux/irqchip/arm-gic-acpi.h

-- 
1.7.9.5

[PATCH v4 01/18] ARM64: Move the init of cpu_logical_map(0) before unflatten_device_tree()

[Hanjun Guo]

It always make sense to initialize CPU0's logical map entry from the
hardware values, so move the initialization of cpu_logical_map(0)
before unflatten_device_tree() which is needed by ACPI code later.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 arch/arm64/kernel/setup.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index edb146d..8f33f72 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -393,11 +393,11 @@ void __init setup_arch(char **cmdline_p)
 
 	efi_idmap_init();
 
+	cpu_logical_map(0) = read_cpuid_mpidr() & MPIDR_HWID_BITMASK;
 	unflatten_device_tree();
 
 	psci_init();
 
-	cpu_logical_map(0) = read_cpuid_mpidr() & MPIDR_HWID_BITMASK;
 	cpu_read_bootcpu_ops();
 #ifdef CONFIG_SMP
 	smp_init_cpus();
-- 
1.7.9.5

Re: [PATCH v4 01/18] ARM64: Move the init of cpu_logical_map(0) before unflatten_device_tree()

[Jon Masters]

On 09/12/2014 09:59 AM, Hanjun Guo wrote:
> It always make sense to initialize CPU0's logical map entry from the
> hardware values, so move the initialization of cpu_logical_map(0)
> before unflatten_device_tree() which is needed by ACPI code later.
> 
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> ---
>  arch/arm64/kernel/setup.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index edb146d..8f33f72 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -393,11 +393,11 @@ void __init setup_arch(char **cmdline_p)
>  
>  	efi_idmap_init();
>  
> +	cpu_logical_map(0) = read_cpuid_mpidr() & MPIDR_HWID_BITMASK;
>  	unflatten_device_tree();
>  
>  	psci_init();
>  
> -	cpu_logical_map(0) = read_cpuid_mpidr() & MPIDR_HWID_BITMASK;
>  	cpu_read_bootcpu_ops();
>  #ifdef CONFIG_SMP
>  	smp_init_cpus();

Thanks - this is a good fix to ensure we're always dealing with the
masked value of the MPIDR Aff bits that we care about.

Jon.

Re: [PATCH v4 01/18] ARM64: Move the init of cpu_logical_map(0) before unflatten_device_tree()

[Olof Johansson]

On Fri, Sep 12, 2014 at 09:59:59PM +0800, Hanjun Guo wrote:
> It always make sense to initialize CPU0's logical map entry from the
> hardware values, so move the initialization of cpu_logical_map(0)
> before unflatten_device_tree() which is needed by ACPI code later.
> 
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>

Acked-by: Olof Johansson <olof@lixom.net>

With or without needs for ACPI, I don't think there's any reason for
this code to not go in now.


-Olof

Re: [PATCH v4 01/18] ARM64: Move the init of cpu_logical_map(0) before unflatten_device_tree()

[Mark Rutland]

On Mon, Sep 15, 2014 at 07:16:32AM +0100, Olof Johansson wrote:
> On Fri, Sep 12, 2014 at 09:59:59PM +0800, Hanjun Guo wrote:
> > It always make sense to initialize CPU0's logical map entry from the
> > hardware values, so move the initialization of cpu_logical_map(0)
> > before unflatten_device_tree() which is needed by ACPI code later.
> > 
> > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> 
> Acked-by: Olof Johansson <olof@lixom.net>
> 
> With or without needs for ACPI, I don't think there's any reason for
> this code to not go in now.

I can see this being of use to psci_init() where I'd like to check
MIGRATE_INFO_TYPE and MIGRATE_INFO_UP_CPU early on for systems with PSCI
0.2+. So moving it at least that early sounds good to me.

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

[PATCH v4 02/18] ACPI / table: Add new function to get table entries

[Hanjun Guo]

From: Ashwin Chaugule <ashwin.chaugule@linaro.org>

The acpi_table_parse() function has a callback that
passes a pointer to a table_header. Add a new function
which takes this pointer and parses its entries. This
eliminates the need to re-traverse all the tables for
each call. e.g. as in acpi_table_parse_madt() which is
normally called after acpi_table_parse().

Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 drivers/acpi/tables.c |   67 ++++++++++++++++++++++++++++++++++---------------
 include/linux/acpi.h  |    4 +++
 2 files changed, 51 insertions(+), 20 deletions(-)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 6d5a6cd..21ae521 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -192,17 +192,14 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
 
 
 int __init
-acpi_table_parse_entries(char *id,
-			     unsigned long table_size,
-			     int entry_id,
-			     acpi_tbl_entry_handler handler,
-			     unsigned int max_entries)
+acpi_parse_entries(unsigned long table_size,
+		acpi_tbl_entry_handler handler,
+		struct acpi_table_header *table_header,
+		int entry_id, unsigned int max_entries)
 {
-	struct acpi_table_header *table_header = NULL;
 	struct acpi_subtable_header *entry;
-	unsigned int count = 0;
+	int count = 0;
 	unsigned long table_end;
-	acpi_size tbl_size;
 
 	if (acpi_disabled)
 		return -ENODEV;
@@ -210,13 +207,11 @@ acpi_table_parse_entries(char *id,
 	if (!handler)
 		return -EINVAL;
 
-	if (strncmp(id, ACPI_SIG_MADT, 4) == 0)
-		acpi_get_table_with_size(id, acpi_apic_instance, &table_header, &tbl_size);
-	else
-		acpi_get_table_with_size(id, 0, &table_header, &tbl_size);
+	if (!table_size)
+		return -EINVAL;
 
 	if (!table_header) {
-		pr_warn("%4.4s not present\n", id);
+		pr_warn("Table header not present\n");
 		return -ENODEV;
 	}
 
@@ -232,30 +227,62 @@ acpi_table_parse_entries(char *id,
 		if (entry->type == entry_id
 		    && (!max_entries || count++ < max_entries))
 			if (handler(entry, table_end))
-				goto err;
+				return -EINVAL;
 
 		/*
 		 * If entry->length is 0, break from this loop to avoid
 		 * infinite loop.
 		 */
 		if (entry->length == 0) {
-			pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, entry_id);
-			goto err;
+			pr_err("[0x%02x] Invalid zero length\n", entry_id);
+			return -EINVAL;
 		}
 
 		entry = (struct acpi_subtable_header *)
 		    ((unsigned long)entry + entry->length);
 	}
+
 	if (max_entries && count > max_entries) {
 		pr_warn("[%4.4s:0x%02x] ignored %i entries of %i found\n",
-			id, entry_id, count - max_entries, count);
+			table_header->signature, entry_id, count - max_entries,
+			count);
 	}
 
-	early_acpi_os_unmap_memory((char *)table_header, tbl_size);
 	return count;
-err:
+}
+
+int __init
+acpi_table_parse_entries(char *id,
+			 unsigned long table_size,
+			 int entry_id,
+			 acpi_tbl_entry_handler handler,
+			 unsigned int max_entries)
+{
+	struct acpi_table_header *table_header = NULL;
+	acpi_size tbl_size;
+	int count;
+
+	if (acpi_disabled)
+		return -ENODEV;
+
+	if (!handler)
+		return -EINVAL;
+
+	if (strncmp(id, ACPI_SIG_MADT, 4) == 0)
+		acpi_get_table_with_size(id, acpi_apic_instance, &table_header, &tbl_size);
+	else
+		acpi_get_table_with_size(id, 0, &table_header, &tbl_size);
+
+	if (!table_header) {
+		pr_warn("%4.4s not present\n", id);
+		return -ENODEV;
+	}
+
+	count = acpi_parse_entries(table_size, handler, table_header,
+			entry_id, max_entries);
+
 	early_acpi_os_unmap_memory((char *)table_header, tbl_size);
-	return -EINVAL;
+	return count;
 }
 
 int __init
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 807cbc4..36d5012 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -123,6 +123,10 @@ int acpi_numa_init (void);
 
 int acpi_table_init (void);
 int acpi_table_parse(char *id, acpi_tbl_table_handler handler);
+int __init acpi_parse_entries(unsigned long table_size,
+			      acpi_tbl_entry_handler handler,
+			      struct acpi_table_header *table_header,
+			      int entry_id, unsigned int max_entries);
 int __init acpi_table_parse_entries(char *id, unsigned long table_size,
 				    int entry_id,
 				    acpi_tbl_entry_handler handler,
-- 
1.7.9.5

Re: [PATCH v4 02/18] ACPI / table: Add new function to get table entries

[Grant Likely]

On Fri, 12 Sep 2014 22:00:00 +0800, Hanjun Guo <hanjun.guo@linaro.org> wrote:
> From: Ashwin Chaugule <ashwin.chaugule@linaro.org>
> 
> The acpi_table_parse() function has a callback that
> passes a pointer to a table_header. Add a new function
> which takes this pointer and parses its entries. This
> eliminates the need to re-traverse all the tables for
> each call. e.g. as in acpi_table_parse_madt() which is
> normally called after acpi_table_parse().
> 
> Signed-off-by: Ashwin Chaugule <ashwin.chaugule@linaro.org>
> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>

Makes sense.

Acked-by: Grant Likely <grant.likely@linaro.org>

g.

> ---
>  drivers/acpi/tables.c |   67 ++++++++++++++++++++++++++++++++++---------------
>  include/linux/acpi.h  |    4 +++
>  2 files changed, 51 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> index 6d5a6cd..21ae521 100644
> --- a/drivers/acpi/tables.c
> +++ b/drivers/acpi/tables.c
> @@ -192,17 +192,14 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
>  
>  
>  int __init
> -acpi_table_parse_entries(char *id,
> -			     unsigned long table_size,
> -			     int entry_id,
> -			     acpi_tbl_entry_handler handler,
> -			     unsigned int max_entries)
> +acpi_parse_entries(unsigned long table_size,
> +		acpi_tbl_entry_handler handler,
> +		struct acpi_table_header *table_header,
> +		int entry_id, unsigned int max_entries)
>  {
> -	struct acpi_table_header *table_header = NULL;
>  	struct acpi_subtable_header *entry;
> -	unsigned int count = 0;
> +	int count = 0;
>  	unsigned long table_end;
> -	acpi_size tbl_size;
>  
>  	if (acpi_disabled)
>  		return -ENODEV;
> @@ -210,13 +207,11 @@ acpi_table_parse_entries(char *id,
>  	if (!handler)
>  		return -EINVAL;
>  
> -	if (strncmp(id, ACPI_SIG_MADT, 4) == 0)
> -		acpi_get_table_with_size(id, acpi_apic_instance, &table_header, &tbl_size);
> -	else
> -		acpi_get_table_with_size(id, 0, &table_header, &tbl_size);
> +	if (!table_size)
> +		return -EINVAL;
>  
>  	if (!table_header) {
> -		pr_warn("%4.4s not present\n", id);
> +		pr_warn("Table header not present\n");
>  		return -ENODEV;
>  	}
>  
> @@ -232,30 +227,62 @@ acpi_table_parse_entries(char *id,
>  		if (entry->type == entry_id
>  		    && (!max_entries || count++ < max_entries))
>  			if (handler(entry, table_end))
> -				goto err;
> +				return -EINVAL;
>  
>  		/*
>  		 * If entry->length is 0, break from this loop to avoid
>  		 * infinite loop.
>  		 */
>  		if (entry->length == 0) {
> -			pr_err("[%4.4s:0x%02x] Invalid zero length\n", id, entry_id);
> -			goto err;
> +			pr_err("[0x%02x] Invalid zero length\n", entry_id);
> +			return -EINVAL;
>  		}
>  
>  		entry = (struct acpi_subtable_header *)
>  		    ((unsigned long)entry + entry->length);
>  	}
> +
>  	if (max_entries && count > max_entries) {
>  		pr_warn("[%4.4s:0x%02x] ignored %i entries of %i found\n",
> -			id, entry_id, count - max_entries, count);
> +			table_header->signature, entry_id, count - max_entries,
> +			count);
>  	}
>  
> -	early_acpi_os_unmap_memory((char *)table_header, tbl_size);
>  	return count;
> -err:
> +}
> +
> +int __init
> +acpi_table_parse_entries(char *id,
> +			 unsigned long table_size,
> +			 int entry_id,
> +			 acpi_tbl_entry_handler handler,
> +			 unsigned int max_entries)
> +{
> +	struct acpi_table_header *table_header = NULL;
> +	acpi_size tbl_size;
> +	int count;
> +
> +	if (acpi_disabled)
> +		return -ENODEV;
> +
> +	if (!handler)
> +		return -EINVAL;
> +
> +	if (strncmp(id, ACPI_SIG_MADT, 4) == 0)
> +		acpi_get_table_with_size(id, acpi_apic_instance, &table_header, &tbl_size);
> +	else
> +		acpi_get_table_with_size(id, 0, &table_header, &tbl_size);
> +
> +	if (!table_header) {
> +		pr_warn("%4.4s not present\n", id);
> +		return -ENODEV;
> +	}
> +
> +	count = acpi_parse_entries(table_size, handler, table_header,
> +			entry_id, max_entries);
> +
>  	early_acpi_os_unmap_memory((char *)table_header, tbl_size);
> -	return -EINVAL;
> +	return count;
>  }
>  
>  int __init
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 807cbc4..36d5012 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -123,6 +123,10 @@ int acpi_numa_init (void);
>  
>  int acpi_table_init (void);
>  int acpi_table_parse(char *id, acpi_tbl_table_handler handler);
> +int __init acpi_parse_entries(unsigned long table_size,
> +			      acpi_tbl_entry_handler handler,
> +			      struct acpi_table_header *table_header,
> +			      int entry_id, unsigned int max_entries);
>  int __init acpi_table_parse_entries(char *id, unsigned long table_size,
>  				    int entry_id,
>  				    acpi_tbl_entry_handler handler,
> -- 
> 1.7.9.5
> 

[PATCH v4 03/18] ACPI / table: Count matched and successfully parsed entries without specifying max entries

[Hanjun Guo]

From: Tomasz Nowicki <tomasz.nowicki@linaro.org>

It is very useful to traverse all available table entries without max
number of expected entries type. Current acpi_parse_entries()
implementation gives that feature but it does not count those entries,
it returns 0 instead, so fix it to count matched and successfully
entries and return it.

NOTE: This change has no impact to x86 and ia64 archs since existing code
checks for error occurrence only (acpi_parse_entries(...,0) < 0).

Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 drivers/acpi/tables.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 21ae521..b18e45e 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -225,10 +225,13 @@ acpi_parse_entries(unsigned long table_size,
 	while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
 	       table_end) {
 		if (entry->type == entry_id
-		    && (!max_entries || count++ < max_entries))
+		    && (!max_entries || count < max_entries)) {
 			if (handler(entry, table_end))
 				return -EINVAL;
 
+			count++;
+		}
+
 		/*
 		 * If entry->length is 0, break from this loop to avoid
 		 * infinite loop.
-- 
1.7.9.5

Re: [PATCH v4 03/18] ACPI / table: Count matched and successfully parsed entries without specifying max entries

[Grant Likely]

On Fri, 12 Sep 2014 22:00:01 +0800, Hanjun Guo <hanjun.guo@linaro.org> wrote:
> From: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> 
> It is very useful to traverse all available table entries without max
> number of expected entries type. Current acpi_parse_entries()
> implementation gives that feature but it does not count those entries,
> it returns 0 instead, so fix it to count matched and successfully
> entries and return it.
> 
> NOTE: This change has no impact to x86 and ia64 archs since existing code
> checks for error occurrence only (acpi_parse_entries(...,0) < 0).
> 
> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>

Acked-by: Grant Likely <grant.likely@linaro.org>

> ---
>  drivers/acpi/tables.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
> index 21ae521..b18e45e 100644
> --- a/drivers/acpi/tables.c
> +++ b/drivers/acpi/tables.c
> @@ -225,10 +225,13 @@ acpi_parse_entries(unsigned long table_size,
>  	while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
>  	       table_end) {
>  		if (entry->type == entry_id
> -		    && (!max_entries || count++ < max_entries))
> +		    && (!max_entries || count < max_entries)) {
>  			if (handler(entry, table_end))
>  				return -EINVAL;
>  
> +			count++;
> +		}
> +
>  		/*
>  		 * If entry->length is 0, break from this loop to avoid
>  		 * infinite loop.
> -- 
> 1.7.9.5
> 

[PATCH v4 04/18] ARM64 / ACPI: Get RSDP and ACPI boot-time tables

[Hanjun Guo]

From: Al Stone <al.stone@linaro.org>

As we want to get ACPI tables to parse and then use the information
for system initialization, we should get the RSDP (Root System
Description Pointer) first, it then locates Extended Root Description
Table (XSDT) which contains all the 64-bit physical address that
pointer to other boot-time tables.

Introduce acpi.c and its related head file in this patch to provide
fundamental needs of extern variables and functions for ACPI core,
and then get boot-time tables as needed.
  - asm/acenv.h for arch specific ACPICA environments and
    implementation, It is needed unconditionally by ACPI core;
  - asm/acpi.h for arch specific variables and functions needed by
    ACPI driver core;
  - acpi.c for ARM64 related ACPI implementation for ACPI driver
    core;

acpi_boot_table_init() is introduced to get RSDP and boot-time tables,
it will be called in setup_arch() before paging_init(), so we should
use eary_memremap() mechanism here to get the RSDP and all the table
pointers.

Signed-off-by: Al Stone <al.stone@linaro.org>
Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 arch/arm64/include/asm/acenv.h |   18 +++++++++++
 arch/arm64/include/asm/acpi.h  |   45 ++++++++++++++++++++++++++
 arch/arm64/kernel/Makefile     |    1 +
 arch/arm64/kernel/acpi.c       |   69 ++++++++++++++++++++++++++++++++++++++++
 arch/arm64/kernel/setup.c      |    4 +++
 5 files changed, 137 insertions(+)
 create mode 100644 arch/arm64/include/asm/acenv.h
 create mode 100644 arch/arm64/include/asm/acpi.h
 create mode 100644 arch/arm64/kernel/acpi.c

diff --git a/arch/arm64/include/asm/acenv.h b/arch/arm64/include/asm/acenv.h
new file mode 100644
index 0000000..b49166f
--- /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
+
+/* It is required unconditionally by ACPI core, update it when needed. */
+
+#endif /* _ASM_ACENV_H */
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
new file mode 100644
index 0000000..8b837ab
--- /dev/null
+++ b/arch/arm64/include/asm/acpi.h
@@ -0,0 +1,45 @@
+/*
+ *  Copyright (C) 2013-2014, Linaro Ltd.
+ *	Author: Al Stone <al.stone@linaro.org>
+ *	Author: Graeme Gregory <graeme.gregory@linaro.org>
+ *	Author: Hanjun Guo <hanjun.guo@linaro.org>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation;
+ */
+
+#ifndef _ASM_ACPI_H
+#define _ASM_ACPI_H
+
+/* Basic configuration for ACPI */
+#ifdef	CONFIG_ACPI
+#define acpi_strict 1	/* No out-of-spec workarounds on ARM64 */
+extern int acpi_disabled;
+extern int acpi_noirq;
+extern int acpi_pci_disabled;
+
+static inline void disable_acpi(void)
+{
+	acpi_disabled = 1;
+	acpi_pci_disabled = 1;
+	acpi_noirq = 1;
+}
+
+/*
+ * 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
+ * obtained from GICC with 0 and not print some error message as well.
+ * Since MADT must provide at least one GICC structure for GIC
+ * initialization, CPU will be always available in MADT on ARM64.
+ */
+static inline bool acpi_has_cpu_in_madt(void)
+{
+	return true;
+}
+
+static inline void arch_fix_phys_package_id(int num, u32 slot) { }
+
+#endif /* CONFIG_ACPI */
+
+#endif /*_ASM_ACPI_H*/
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index df7ef87..29ea7d6 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -29,6 +29,7 @@ arm64-obj-$(CONFIG_ARM64_CPU_SUSPEND)	+= sleep.o suspend.o
 arm64-obj-$(CONFIG_JUMP_LABEL)		+= jump_label.o
 arm64-obj-$(CONFIG_KGDB)		+= kgdb.o
 arm64-obj-$(CONFIG_EFI)			+= efi.o efi-stub.o efi-entry.o
+arm64-obj-$(CONFIG_ACPI)		+= acpi.o
 
 obj-y					+= $(arm64-obj-y) vdso/
 obj-m					+= $(arm64-obj-m)
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
@@ -0,0 +1,69 @@
+/*
+ *  ARM64 Specific Low-Level ACPI Boot Support
+ *
+ *  Copyright (C) 2013-2014, Linaro Ltd.
+ *	Author: Al Stone <al.stone@linaro.org>
+ *	Author: Graeme Gregory <graeme.gregory@linaro.org>
+ *	Author: Hanjun Guo <hanjun.guo@linaro.org>
+ *	Author: Tomasz Nowicki <tomasz.nowicki@linaro.org>
+ *	Author: Naresh Bhat <naresh.bhat@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.
+ */
+
+#include <linux/init.h>
+#include <linux/acpi.h>
+#include <linux/cpumask.h>
+#include <linux/memblock.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/bootmem.h>
+#include <linux/smp.h>
+
+int acpi_noirq;			/* skip ACPI IRQ initialization */
+int acpi_disabled;
+EXPORT_SYMBOL(acpi_disabled);
+
+int acpi_pci_disabled;		/* skip ACPI PCI scan and IRQ initialization */
+EXPORT_SYMBOL(acpi_pci_disabled);
+
+/*
+ * __acpi_map_table() will be called before page_init(), so early_ioremap()
+ * or early_memremap() should be called here to for ACPI table mapping.
+ */
+char *__init __acpi_map_table(unsigned long phys, unsigned long size)
+{
+	if (!phys || !size)
+		return NULL;
+
+	return early_memremap(phys, size);
+}
+
+void __init __acpi_unmap_table(char *map, unsigned long size)
+{
+	if (!map || !size)
+		return;
+
+	early_memunmap(map, size);
+}
+
+/*
+ * 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;
+
+	/* Initialize the ACPI boot-time table parser. */
+	if (acpi_table_init())
+		disable_acpi();
+}
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 8f33f72..5b05227 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>
@@ -388,6 +389,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();
+
 	paging_init();
 	request_standard_resources();
 
-- 
1.7.9.5

[PATCH v4 05/18] ARM64 / ACPI: Introduce sleep-arm.c

[Hanjun Guo]

From: Graeme Gregory <graeme.gregory@linaro.org>

ACPI 5.1 does not currently support S states for ARM64 hardware but
ACPI code will call acpi_target_system_state() for device power
managment, so introduce sleep-arm.c to allow other drivers to function
until S states are defined.

Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 drivers/acpi/Makefile    |    4 ++++
 drivers/acpi/sleep-arm.c |   28 ++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)
 create mode 100644 drivers/acpi/sleep-arm.c

diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 505d4d7..89837f8 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -23,7 +23,11 @@ acpi-y				+= nvs.o
 
 # Power management related files
 acpi-y				+= wakeup.o
+ifeq ($(ARCH), arm64)
+acpi-y				+= sleep-arm.o
+else # X86, IA64
 acpi-y				+= sleep.o
+endif
 acpi-y				+= device_pm.o
 acpi-$(CONFIG_ACPI_SLEEP)	+= proc.o
 
diff --git a/drivers/acpi/sleep-arm.c b/drivers/acpi/sleep-arm.c
new file mode 100644
index 0000000..54578ef
--- /dev/null
+++ b/drivers/acpi/sleep-arm.c
@@ -0,0 +1,28 @@
+/*
+ *  ARM64 Specific Sleep Functionality
+ *
+ *  Copyright (C) 2013-2014, Linaro Ltd.
+ *      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.
+ */
+
+#include <linux/acpi.h>
+
+/*
+ * Currently the ACPI 5.1 standard does not define S states in a
+ * manner which is usable for ARM64. These two stubs are sufficient
+ * that system initialises and device PM works.
+ */
+u32 acpi_target_system_state(void)
+{
+	return ACPI_STATE_S0;
+}
+EXPORT_SYMBOL_GPL(acpi_target_system_state);
+
+int __init acpi_sleep_init(void)
+{
+	return -ENOSYS;
+}
-- 
1.7.9.5

Re: [PATCH v4 05/18] ARM64 / ACPI: Introduce sleep-arm.c

[Catalin Marinas]

On Fri, Sep 12, 2014 at 03:00:03PM +0100, Hanjun Guo wrote:
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -23,7 +23,11 @@ acpi-y				+= nvs.o
>  
>  # Power management related files
>  acpi-y				+= wakeup.o
> +ifeq ($(ARCH), arm64)
> +acpi-y				+= sleep-arm.o
> +else # X86, IA64
>  acpi-y				+= sleep.o
> +endif
>  acpi-y				+= device_pm.o
>  acpi-$(CONFIG_ACPI_SLEEP)	+= proc.o

What about:

-acpi-y				+= sleep.o
+acpi-$(CONFIG_ACPI_SLEEP)	+= sleep.o

and make CONFIG_ACPI_SLEEP depend on !ARM64? Does the kernel compile if
you disable CONFIG_ACPI_SLEEP?

-- 
Catalin

Re: [PATCH v4 05/18] ARM64 / ACPI: Introduce sleep-arm.c

[Graeme Gregory]

On Fri, Sep 12, 2014 at 03:51:02PM +0100, Catalin Marinas wrote:
> On Fri, Sep 12, 2014 at 03:00:03PM +0100, Hanjun Guo wrote:
> > --- a/drivers/acpi/Makefile
> > +++ b/drivers/acpi/Makefile
> > @@ -23,7 +23,11 @@ acpi-y				+= nvs.o
> >  
> >  # Power management related files
> >  acpi-y				+= wakeup.o
> > +ifeq ($(ARCH), arm64)
> > +acpi-y				+= sleep-arm.o
> > +else # X86, IA64
> >  acpi-y				+= sleep.o
> > +endif
> >  acpi-y				+= device_pm.o
> >  acpi-$(CONFIG_ACPI_SLEEP)	+= proc.o
> 
> What about:
> 
> -acpi-y				+= sleep.o
> +acpi-$(CONFIG_ACPI_SLEEP)	+= sleep.o
> 
> and make CONFIG_ACPI_SLEEP depend on !ARM64? Does the kernel compile if
> you disable CONFIG_ACPI_SLEEP?
> 
sleep.c is still needed in the case CONFIG_ACPI_SLEEP=n

basically the two functions I stubbed are always called by bus.c

Graeme

Re: [PATCH v4 05/18] ARM64 / ACPI: Introduce sleep-arm.c

[Catalin Marinas]

On Fri, Sep 12, 2014 at 04:18:51PM +0100, Graeme Gregory wrote:
> On Fri, Sep 12, 2014 at 03:51:02PM +0100, Catalin Marinas wrote:
> > On Fri, Sep 12, 2014 at 03:00:03PM +0100, Hanjun Guo wrote:
> > > --- a/drivers/acpi/Makefile
> > > +++ b/drivers/acpi/Makefile
> > > @@ -23,7 +23,11 @@ acpi-y				+= nvs.o
> > >  
> > >  # Power management related files
> > >  acpi-y				+= wakeup.o
> > > +ifeq ($(ARCH), arm64)
> > > +acpi-y				+= sleep-arm.o
> > > +else # X86, IA64
> > >  acpi-y				+= sleep.o
> > > +endif
> > >  acpi-y				+= device_pm.o
> > >  acpi-$(CONFIG_ACPI_SLEEP)	+= proc.o
> > 
> > What about:
> > 
> > -acpi-y				+= sleep.o
> > +acpi-$(CONFIG_ACPI_SLEEP)	+= sleep.o
> > 
> > and make CONFIG_ACPI_SLEEP depend on !ARM64? Does the kernel compile if
> > you disable CONFIG_ACPI_SLEEP?
> 
> sleep.c is still needed in the case CONFIG_ACPI_SLEEP=n
> 
> basically the two functions I stubbed are always called by bus.c

So the two functions are:

acpi_target_system_state() - already defined in acpi_bus.h when
CONFIG_ACPI_SLEEP is off.

acpi_sleep_init() - this one doesn't have a dummy implementation for
!ACPI_SLEEP.

Now, for x86, do we need acpi_sleep_init() when CONFIG_ACPI_SLEEP is
off? If not, we could simply add a dummy acpi_sleep_init() function when
!ACPI_SLEEP and don't enable the config option for arm64.

-- 
Catalin

Re: [PATCH v4 05/18] ARM64 / ACPI: Introduce sleep-arm.c

[Graeme Gregory]

On Fri, Sep 12, 2014 at 04:49:03PM +0100, Catalin Marinas wrote:
> On Fri, Sep 12, 2014 at 04:18:51PM +0100, Graeme Gregory wrote:
> > On Fri, Sep 12, 2014 at 03:51:02PM +0100, Catalin Marinas wrote:
> > > On Fri, Sep 12, 2014 at 03:00:03PM +0100, Hanjun Guo wrote:
> > > > --- a/drivers/acpi/Makefile
> > > > +++ b/drivers/acpi/Makefile
> > > > @@ -23,7 +23,11 @@ acpi-y				+= nvs.o
> > > >  
> > > >  # Power management related files
> > > >  acpi-y				+= wakeup.o
> > > > +ifeq ($(ARCH), arm64)
> > > > +acpi-y				+= sleep-arm.o
> > > > +else # X86, IA64
> > > >  acpi-y				+= sleep.o
> > > > +endif
> > > >  acpi-y				+= device_pm.o
> > > >  acpi-$(CONFIG_ACPI_SLEEP)	+= proc.o
> > > 
> > > What about:
> > > 
> > > -acpi-y				+= sleep.o
> > > +acpi-$(CONFIG_ACPI_SLEEP)	+= sleep.o
> > > 
> > > and make CONFIG_ACPI_SLEEP depend on !ARM64? Does the kernel compile if
> > > you disable CONFIG_ACPI_SLEEP?
> > 
> > sleep.c is still needed in the case CONFIG_ACPI_SLEEP=n
> > 
> > basically the two functions I stubbed are always called by bus.c
> 
> So the two functions are:
> 
> acpi_target_system_state() - already defined in acpi_bus.h when
> CONFIG_ACPI_SLEEP is off.
> 
> acpi_sleep_init() - this one doesn't have a dummy implementation for
> !ACPI_SLEEP.
> 
> Now, for x86, do we need acpi_sleep_init() when CONFIG_ACPI_SLEEP is
> off? If not, we could simply add a dummy acpi_sleep_init() function when
> !ACPI_SLEEP and don't enable the config option for arm64.
> 
It looks to me that it still does processing for the S5 state which is
soft off.

It sets up the operations to be called when entering this state and also
sets up a notifier.

Graeme

Re: [PATCH v4 05/18] ARM64 / ACPI: Introduce sleep-arm.c

[Matthew Garrett]

On Fri, Sep 12, 2014 at 04:49:03PM +0100, Catalin Marinas wrote:

> Now, for x86, do we need acpi_sleep_init() when CONFIG_ACPI_SLEEP is
> off? If not, we could simply add a dummy acpi_sleep_init() function when
> !ACPI_SLEEP and don't enable the config option for arm64.

The !ACPI_SLEEP case still uses the ACPI code for powering the system 
down. I'd recommend adding a new CONFIG_ACPI_POWER_OFF option, wrapping 
the remaining code in sleep.c and disabling that on ARM.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH v4 05/18] ARM64 / ACPI: Introduce sleep-arm.c

[Jon Masters]

On 09/12/2014 10:00 AM, Hanjun Guo wrote:
> From: Graeme Gregory <graeme.gregory@linaro.org>
> 
> ACPI 5.1 does not currently support S states for ARM64 hardware but
> ACPI code will call acpi_target_system_state() for device power
> managment, so introduce sleep-arm.c to allow other drivers to function
> until S states are defined.

Aside: ACPI5.1 does define the Platform Communication Channel and CPC
(Collaborative Processor Performance Control). Some details need to be
fleshed out there for practical 64-bit ARMv8 server systems, but the
underpinnings and mechanisms are in place in the 5.1 specification that
can be built upon over time to achieve practical S-State equivalence.

Jon.

Re: [PATCH v4 05/18] ARM64 / ACPI: Introduce sleep-arm.c

[Graeme Gregory]

On Fri, Sep 12, 2014 at 03:43:36PM -0400, Jon Masters wrote:
> On 09/12/2014 10:00 AM, Hanjun Guo wrote:
> > From: Graeme Gregory <graeme.gregory@linaro.org>
> > 
> > ACPI 5.1 does not currently support S states for ARM64 hardware but
> > ACPI code will call acpi_target_system_state() for device power
> > managment, so introduce sleep-arm.c to allow other drivers to function
> > until S states are defined.
> 
> Aside: ACPI5.1 does define the Platform Communication Channel and CPC
> (Collaborative Processor Performance Control). Some details need to be
> fleshed out there for practical 64-bit ARMv8 server systems, but the
> underpinnings and mechanisms are in place in the 5.1 specification that
> can be built upon over time to achieve practical S-State equivalence.
> 
Disagree, S-States are about going to sleep or hibernating, that is
nothing to do with CPU Performance.

Graeme

Re: [PATCH v4 05/18] ARM64 / ACPI: Introduce sleep-arm.c

[Jon Masters]

On 09/12/2014 04:03 PM, Graeme Gregory wrote:
> On Fri, Sep 12, 2014 at 03:43:36PM -0400, Jon Masters wrote:
>> On 09/12/2014 10:00 AM, Hanjun Guo wrote:
>>> From: Graeme Gregory <graeme.gregory@linaro.org>
>>>
>>> ACPI 5.1 does not currently support S states for ARM64 hardware but
>>> ACPI code will call acpi_target_system_state() for device power
>>> managment, so introduce sleep-arm.c to allow other drivers to function
>>> until S states are defined.
>>
>> Aside: ACPI5.1 does define the Platform Communication Channel and CPC
>> (Collaborative Processor Performance Control). Some details need to be
>> fleshed out there for practical 64-bit ARMv8 server systems, but the
>> underpinnings and mechanisms are in place in the 5.1 specification that
>> can be built upon over time to achieve practical S-State equivalence.
                                                    ^ meant C/P sorry

> Disagree, S-States are about going to sleep or hibernating, that is
> nothing to do with CPU Performance.

Correct. In replying to the above, I was thinking about power and
performance management of components  - CPU (C/P), and device (D)
specific power/performance states - not the overall system (S) state,
and I went off on a tangent. That was off-topic. For the actual topic at
hand, that of global sleep, you are correct that only G0 (S0) and S5 are
really defined today. Though I do think that the PCC might be used to
actually implement overall system sleep state transitions later.

Jon.

[PATCH v4 06/18] ARM64 / ACPI: Introduce early_param for "acpi"

[Hanjun Guo]

From: Al Stone <al.stone@linaro.org>

Introduce one early parameters "off" for "acpi" to disable ACPI on
ARM64.

This ensures the kernel uses the DT on a platform that provides both
ACPI tables and DT.

Signed-off-by: Al Stone <al.stone@linaro.org>
Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 Documentation/kernel-parameters.txt |    3 ++-
 arch/arm64/kernel/acpi.c            |   15 +++++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 5ae8608..9dfb1d8 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -165,7 +165,7 @@ multipliers 'Kilo', 'Mega', and 'Giga', equalling 2^10, 2^20, and 2^30
 bytes respectively. Such letter suffixes can also be entirely omitted.
 
 
-	acpi=		[HW,ACPI,X86]
+	acpi=		[HW,ACPI,X86,ARM]
 			Advanced Configuration and Power Interface
 			Format: { force | off | strict | noirq | rsdt }
 			force -- enable ACPI if default was off
@@ -175,6 +175,7 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 				strictly ACPI specification compliant.
 			rsdt -- prefer RSDT over (default) XSDT
 			copy_dsdt -- copy DSDT to memory
+			For ARM64, ONLY "acpi=off" is available.
 
 			See also Documentation/power/runtime_pm.txt, pci=noacpi
 
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 9252f72..693da7f 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -67,3 +67,18 @@ void __init acpi_boot_table_init(void)
 	if (acpi_table_init())
 		disable_acpi();
 }
+
+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);
-- 
1.7.9.5

Re: [PATCH v4 06/18] ARM64 / ACPI: Introduce early_param for "acpi"

[Olof Johansson]

On Fri, Sep 12, 2014 at 10:00:04PM +0800, Hanjun Guo wrote:
> From: Al Stone <al.stone@linaro.org>
> 
> Introduce one early parameters "off" for "acpi" to disable ACPI on
> ARM64.
> 
> This ensures the kernel uses the DT on a platform that provides both
> ACPI tables and DT.
> 
> Signed-off-by: Al Stone <al.stone@linaro.org>
> Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> ---
>  Documentation/kernel-parameters.txt |    3 ++-
>  arch/arm64/kernel/acpi.c            |   15 +++++++++++++++
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 5ae8608..9dfb1d8 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -165,7 +165,7 @@ multipliers 'Kilo', 'Mega', and 'Giga', equalling 2^10, 2^20, and 2^30
>  bytes respectively. Such letter suffixes can also be entirely omitted.
>  
>  
> -	acpi=		[HW,ACPI,X86]
> +	acpi=		[HW,ACPI,X86,ARM]

ARM64, not ARM.

>  			Advanced Configuration and Power Interface
>  			Format: { force | off | strict | noirq | rsdt }
>  			force -- enable ACPI if default was off
> @@ -175,6 +175,7 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  				strictly ACPI specification compliant.
>  			rsdt -- prefer RSDT over (default) XSDT
>  			copy_dsdt -- copy DSDT to memory
> +			For ARM64, ONLY "acpi=off" is available.

Both force and off should be made available, since they're the two main switches here.

>  			See also Documentation/power/runtime_pm.txt, pci=noacpi
>  
> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> index 9252f72..693da7f 100644
> --- a/arch/arm64/kernel/acpi.c
> +++ b/arch/arm64/kernel/acpi.c
> @@ -67,3 +67,18 @@ void __init acpi_boot_table_init(void)
>  	if (acpi_table_init())
>  		disable_acpi();
>  }
> +
> +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 */

For symmetry, you should handle acpi=force here too (i.e. if someone
disables by default, which I expect we'll ask for when this code is
first merged).

> +
> +	return 0;
> +}
> +early_param("acpi", parse_acpi);
> -- 
> 1.7.9.5
> 

Re: [PATCH v4 06/18] ARM64 / ACPI: Introduce early_param for "acpi"

[Catalin Marinas]

On Mon, Sep 15, 2014 at 07:28:24AM +0100, Olof Johansson wrote:
> On Fri, Sep 12, 2014 at 10:00:04PM +0800, Hanjun Guo wrote:
> >  			See also Documentation/power/runtime_pm.txt, pci=noacpi
> >  
> > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> > index 9252f72..693da7f 100644
> > --- a/arch/arm64/kernel/acpi.c
> > +++ b/arch/arm64/kernel/acpi.c
> > @@ -67,3 +67,18 @@ void __init acpi_boot_table_init(void)
> >  	if (acpi_table_init())
> >  		disable_acpi();
> >  }
> > +
> > +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 */
> 
> For symmetry, you should handle acpi=force here too (i.e. if someone
> disables by default, which I expect we'll ask for when this code is
> first merged).

Does this mean you want another Kconfig option - ACPI_DEFAULT_OFF (which
would be set by default)?

-- 
Catalin

Re: [PATCH v4 06/18] ARM64 / ACPI: Introduce early_param for "acpi"

[Olof Johansson]

On Mon, Sep 15, 2014 at 03:51:07PM +0100, Catalin Marinas wrote:
> On Mon, Sep 15, 2014 at 07:28:24AM +0100, Olof Johansson wrote:
> > On Fri, Sep 12, 2014 at 10:00:04PM +0800, Hanjun Guo wrote:
> > >  			See also Documentation/power/runtime_pm.txt, pci=noacpi
> > >  
> > > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> > > index 9252f72..693da7f 100644
> > > --- a/arch/arm64/kernel/acpi.c
> > > +++ b/arch/arm64/kernel/acpi.c
> > > @@ -67,3 +67,18 @@ void __init acpi_boot_table_init(void)
> > >  	if (acpi_table_init())
> > >  		disable_acpi();
> > >  }
> > > +
> > > +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 */
> > 
> > For symmetry, you should handle acpi=force here too (i.e. if someone
> > disables by default, which I expect we'll ask for when this code is
> > first merged).
> 
> Does this mean you want another Kconfig option - ACPI_DEFAULT_OFF (which
> would be set by default)?

No, adding another config option isn't going to help anyone. Instead,
this gives us the full symmetry and option to just always by default
keep ACPI off, even if it is merged.

My argument on when ACPI should be merged is not new (i.e. once we've
seen more than just the simple system parts solved with it and solved
successfully), then we're still likely to have a transition period when
it's in tree but not enabled by default. That's what acpi=force is for,
for those who want to test with it during that time.

So, my suggestion is just there now to make sure that whichever way
we go, things will just work as expected (and the same way as on x86,
which is a goal for some).


-Olof

Re: [PATCH v4 06/18] ARM64 / ACPI: Introduce early_param for "acpi"

[Jon Masters]

On 09/15/2014 10:51 AM, Catalin Marinas wrote:
> On Mon, Sep 15, 2014 at 07:28:24AM +0100, Olof Johansson wrote:
>> On Fri, Sep 12, 2014 at 10:00:04PM +0800, Hanjun Guo wrote:
>>>  			See also Documentation/power/runtime_pm.txt, pci=noacpi
>>>  
>>> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
>>> index 9252f72..693da7f 100644
>>> --- a/arch/arm64/kernel/acpi.c
>>> +++ b/arch/arm64/kernel/acpi.c
>>> @@ -67,3 +67,18 @@ void __init acpi_boot_table_init(void)
>>>  	if (acpi_table_init())
>>>  		disable_acpi();
>>>  }
>>> +
>>> +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 */
>>
>> For symmetry, you should handle acpi=force here too (i.e. if someone
>> disables by default, which I expect we'll ask for when this code is
>> first merged).
> 
> Does this mean you want another Kconfig option - ACPI_DEFAULT_OFF (which
> would be set by default)?

This would make sense. I mentioned this before since it gives distros an
opportunity to set this differently for their shipping preference
without the upstream having to default to being on.

Jon.

Re: [PATCH v4 06/18] ARM64 / ACPI: Introduce early_param for "acpi"

[Hanjun Guo]

On 2014年09月15日 14:28, Olof Johansson wrote:
> On Fri, Sep 12, 2014 at 10:00:04PM +0800, Hanjun Guo wrote:
>> From: Al Stone <al.stone@linaro.org>
>>
>> Introduce one early parameters "off" for "acpi" to disable ACPI on
>> ARM64.
>>
>> This ensures the kernel uses the DT on a platform that provides both
>> ACPI tables and DT.
>>
>> Signed-off-by: Al Stone <al.stone@linaro.org>
>> Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>> ---
>>  Documentation/kernel-parameters.txt |    3 ++-
>>  arch/arm64/kernel/acpi.c            |   15 +++++++++++++++
>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
>> index 5ae8608..9dfb1d8 100644
>> --- a/Documentation/kernel-parameters.txt
>> +++ b/Documentation/kernel-parameters.txt
>> @@ -165,7 +165,7 @@ multipliers 'Kilo', 'Mega', and 'Giga', equalling 2^10, 2^20, and 2^30
>>  bytes respectively. Such letter suffixes can also be entirely omitted.
>>  
>>  
>> -	acpi=		[HW,ACPI,X86]
>> +	acpi=		[HW,ACPI,X86,ARM]
> ARM64, not ARM.

I will update it.

Thanks
Hanjun

[PATCH v4 07/18] ARM64 / ACPI: If we chose to boot from acpi then disable FDT

[Hanjun Guo]

From: Graeme Gregory <graeme.gregory@linaro.org>

If the early boot methods of acpi are happy that we have valid ACPI
tables and acpi=off has not been passed. Then do not unflat
devicetree effectively disabling further hardware probing from DT.

Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 arch/arm64/kernel/setup.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 5b05227..5796734 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -398,7 +398,8 @@ void __init setup_arch(char **cmdline_p)
 	efi_idmap_init();
 
 	cpu_logical_map(0) = read_cpuid_mpidr() & MPIDR_HWID_BITMASK;
-	unflatten_device_tree();
+	if (acpi_disabled)
+		unflatten_device_tree();
 
 	psci_init();
 
-- 
1.7.9.5

[PATCH v4 08/18] ARM64 / ACPI: Make PCI optional for ACPI on ARM64

[Hanjun Guo]

As PCI for ARM64 is not ready, so introduce some stub functions
to make PCI optional for ACPI, and make ACPI core run without
CONFIG_PCI on ARM64.

Since ACPI on X86 and IA64 depends on PCI and this patch only makes
PCI optional for ARM64, it will not break anything on X86 and IA64.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 arch/arm64/include/asm/pci.h |   11 +++++++++++
 drivers/acpi/Makefile        |    2 +-
 drivers/acpi/internal.h      |    5 +++++
 include/linux/pci.h          |   37 +++++++++++++++++++++++++++----------
 4 files changed, 44 insertions(+), 11 deletions(-)
 create mode 100644 arch/arm64/include/asm/pci.h

diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h
new file mode 100644
index 0000000..250cd24
--- /dev/null
+++ b/arch/arm64/include/asm/pci.h
@@ -0,0 +1,11 @@
+#ifndef __ASM_PCI_H
+#define __ASM_PCI_H
+#ifdef __KERNEL__
+
+/*
+ * PCI address space differs from physical memory address space
+ */
+#define PCI_DMA_BUS_IS_PHYS	(0)
+
+#endif  /* __KERNEL__ */
+#endif  /* __ASM_PCI_H */
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 89837f8..6f3a74d 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -43,7 +43,7 @@ acpi-y				+= processor_core.o
 acpi-$(CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC) += processor_pdc.o
 acpi-y				+= ec.o
 acpi-$(CONFIG_ACPI_DOCK)	+= dock.o
-acpi-y				+= pci_root.o pci_link.o pci_irq.o
+acpi-$(CONFIG_PCI)		+= pci_root.o pci_link.o pci_irq.o
 acpi-y				+= acpi_lpss.o
 acpi-y				+= acpi_platform.o
 acpi-y				+= acpi_pnp.o
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 4c5cf77..e1e6487 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -26,8 +26,13 @@
 acpi_status acpi_os_initialize1(void);
 int init_acpi_device_notify(void);
 int acpi_scan_init(void);
+#ifdef CONFIG_PCI
 void acpi_pci_root_init(void);
 void acpi_pci_link_init(void);
+#else
+static inline void acpi_pci_root_init(void) {}
+static inline void acpi_pci_link_init(void) {}
+#endif
 void acpi_processor_init(void);
 void acpi_platform_init(void);
 void acpi_pnp_init(void);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 61978a4..50fa750 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -558,15 +558,6 @@ struct pci_ops {
 	int (*write)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 val);
 };
 
-/*
- * ACPI needs to be able to access PCI config space before we've done a
- * PCI bus scan and created pci_bus structures.
- */
-int raw_pci_read(unsigned int domain, unsigned int bus, unsigned int devfn,
-		 int reg, int len, u32 *val);
-int raw_pci_write(unsigned int domain, unsigned int bus, unsigned int devfn,
-		  int reg, int len, u32 val);
-
 struct pci_bus_region {
 	dma_addr_t start;
 	dma_addr_t end;
@@ -1293,6 +1284,16 @@ typedef int (*arch_set_vga_state_t)(struct pci_dev *pdev, bool decode,
 		      unsigned int command_bits, u32 flags);
 void pci_register_set_vga_state(arch_set_vga_state_t func);
 
+/*
+ * ACPI needs to be able to access PCI config space before we've done a
+ * PCI bus scan and created pci_bus structures.
+ */
+int raw_pci_read(unsigned int domain, unsigned int bus, unsigned int devfn,
+		 int reg, int len, u32 *val);
+int raw_pci_write(unsigned int domain, unsigned int bus, unsigned int devfn,
+		  int reg, int len, u32 val);
+void pcibios_penalize_isa_irq(int irq, int active);
+
 #else /* CONFIG_PCI is not enabled */
 
 /*
@@ -1394,6 +1395,23 @@ static inline struct pci_dev *pci_get_bus_and_slot(unsigned int bus,
 						unsigned int devfn)
 { return NULL; }
 
+static inline struct pci_bus *pci_find_bus(int domain, int busnr)
+{ return NULL; }
+
+static inline int pci_bus_write_config_byte(struct pci_bus *bus,
+				unsigned int devfn, int where, u8 val)
+{ return -ENOSYS; }
+
+static inline int raw_pci_read(unsigned int domain, unsigned int bus,
+			unsigned int devfn, int reg, int len, u32 *val)
+{ return -ENOSYS; }
+
+static inline int raw_pci_write(unsigned int domain, unsigned int bus,
+			unsigned int devfn, int reg, int len, u32 val)
+{ return -ENOSYS; }
+
+static inline void pcibios_penalize_isa_irq(int irq, int active) { }
+
 static inline int pci_domain_nr(struct pci_bus *bus) { return 0; }
 static inline struct pci_dev *pci_dev_get(struct pci_dev *dev) { return NULL; }
 
@@ -1607,7 +1625,6 @@ int pcibios_set_pcie_reset_state(struct pci_dev *dev,
 				 enum pcie_reset_state state);
 int pcibios_add_device(struct pci_dev *dev);
 void pcibios_release_device(struct pci_dev *dev);
-void pcibios_penalize_isa_irq(int irq, int active);
 
 #ifdef CONFIG_HIBERNATE_CALLBACKS
 extern struct dev_pm_ops pcibios_pm_ops;
-- 
1.7.9.5

[PATCH v4 09/18] ARM64 / ACPI: Parse FADT table to get PSCI flags for PSCI init

[Hanjun Guo]

There are two flags: PSCI_COMPLIANT and PSCI_USE_HVC. When set,
the former signals to the OS that the firmware is PSCI compliant.
The latter selects the appropriate conduit for PSCI calls by
toggling between Hypervisor Calls (HVC) and Secure Monitor Calls
(SMC).

FADT table contains such information, parse FADT to get the flags
for PSCI init. Since ACPI 5.1 doesn't support self defined PSCI
function IDs, which means that only PSCI 0.2+ is supported in ACPI.

At the same time, only ACPI 5.1 or higher verison supports PSCI,
and FADT Major.Minor version was introduced in ACPI 5.1, so we
will check the version and only parse FADT table with version >= 5.1.

If firmware provides ACPI tables with ACPI version less than 5.1,
OS will be messed up with those information and have no way to init
smp and GIC, so disable ACPI if we get an FADT table with version
less that 5.1.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
---
 arch/arm64/include/asm/acpi.h |   17 +++++++++
 arch/arm64/include/asm/psci.h |    3 +-
 arch/arm64/kernel/acpi.c      |   31 +++++++++++++++-
 arch/arm64/kernel/psci.c      |   78 ++++++++++++++++++++++++++++-------------
 arch/arm64/kernel/setup.c     |    8 +++--
 5 files changed, 108 insertions(+), 29 deletions(-)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index 8b837ab..ecba671 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -19,6 +19,18 @@ extern int acpi_disabled;
 extern int acpi_noirq;
 extern int acpi_pci_disabled;
 
+/* 1 to indicate PSCI 0.2+ is implemented */
+static inline bool acpi_psci_present(void)
+{
+	return acpi_gbl_FADT.arm_boot_flags & ACPI_FADT_PSCI_COMPLIANT;
+}
+
+/* 1 to indicate HVC must be used instead of SMC as the PSCI conduit */
+static inline bool acpi_psci_use_hvc(void)
+{
+	return acpi_gbl_FADT.arm_boot_flags & ACPI_FADT_PSCI_USE_HVC;
+}
+
 static inline void disable_acpi(void)
 {
 	acpi_disabled = 1;
@@ -40,6 +52,11 @@ static inline bool acpi_has_cpu_in_madt(void)
 
 static inline void arch_fix_phys_package_id(int num, u32 slot) { }
 
+#else
+
+static inline bool acpi_psci_present(void) { return false; }
+static inline bool acpi_psci_use_hvc(void) { return false; }
+
 #endif /* CONFIG_ACPI */
 
 #endif /*_ASM_ACPI_H*/
diff --git a/arch/arm64/include/asm/psci.h b/arch/arm64/include/asm/psci.h
index e5312ea..2454bc5 100644
--- a/arch/arm64/include/asm/psci.h
+++ b/arch/arm64/include/asm/psci.h
@@ -14,6 +14,7 @@
 #ifndef __ASM_PSCI_H
 #define __ASM_PSCI_H
 
-int psci_init(void);
+int psci_dt_init(void);
+int psci_acpi_init(void);
 
 #endif /* __ASM_PSCI_H */
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 693da7f..9e1ae37 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -13,6 +13,8 @@
  *  published by the Free Software Foundation.
  */
 
+#define pr_fmt(fmt) "ACPI: " fmt
+
 #include <linux/init.h>
 #include <linux/acpi.h>
 #include <linux/cpumask.h>
@@ -49,10 +51,32 @@ void __init __acpi_unmap_table(char *map, unsigned long size)
 	early_memunmap(map, size);
 }
 
+static int __init acpi_parse_fadt(struct acpi_table_header *table)
+{
+	struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table;
+
+	/*
+	 * Revision in table header is the FADT Major revision,
+	 * and there is a minor revision of FADT which was introduced
+	 * by ACPI 5.1, we only deal with ACPI 5.1 or higher revision
+	 * to get arm boot flags, or we will disable ACPI.
+	 */
+	if (table->revision > 5 ||
+	    (table->revision == 5 && fadt->minor_revision >= 1))
+		return 0;
+
+	pr_warn("Unsupported FADT revision %d.%d, should be 5.1+, will disable ACPI\n",
+		table->revision, fadt->minor_revision);
+	disable_acpi();
+
+	return -EINVAL;
+}
+
 /*
  * 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
+ *	3. check ACPI FADT revisoin
  *
  * We can parse ACPI boot-time tables such as MADT after
  * this function is called.
@@ -64,8 +88,13 @@ void __init acpi_boot_table_init(void)
 		return;
 
 	/* Initialize the ACPI boot-time table parser. */
-	if (acpi_table_init())
+	if (acpi_table_init()) {
 		disable_acpi();
+		return;
+	}
+
+	if (acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt))
+		pr_err("Can't find FADT or error happened during parsing FADT\n");
 }
 
 static int __init parse_acpi(char *arg)
diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c
index 5539547..15ba470 100644
--- a/arch/arm64/kernel/psci.c
+++ b/arch/arm64/kernel/psci.c
@@ -15,6 +15,7 @@
 
 #define pr_fmt(fmt) "psci: " fmt
 
+#include <linux/acpi.h>
 #include <linux/init.h>
 #include <linux/of.h>
 #include <linux/smp.h>
@@ -23,6 +24,7 @@
 #include <linux/delay.h>
 #include <uapi/linux/psci.h>
 
+#include <asm/acpi.h>
 #include <asm/compiler.h>
 #include <asm/cpu_ops.h>
 #include <asm/errno.h>
@@ -231,6 +233,33 @@ static void psci_sys_poweroff(void)
 	invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
 }
 
+static void psci_0_2_set_functions(void)
+{
+	pr_info("Using standard PSCI v0.2 function IDs\n");
+	psci_function_id[PSCI_FN_CPU_SUSPEND] = PSCI_0_2_FN64_CPU_SUSPEND;
+	psci_ops.cpu_suspend = psci_cpu_suspend;
+
+	psci_function_id[PSCI_FN_CPU_OFF] = PSCI_0_2_FN_CPU_OFF;
+	psci_ops.cpu_off = psci_cpu_off;
+
+	psci_function_id[PSCI_FN_CPU_ON] = PSCI_0_2_FN64_CPU_ON;
+	psci_ops.cpu_on = psci_cpu_on;
+
+	psci_function_id[PSCI_FN_MIGRATE] = PSCI_0_2_FN64_MIGRATE;
+	psci_ops.migrate = psci_migrate;
+
+	psci_function_id[PSCI_FN_AFFINITY_INFO] = PSCI_0_2_FN64_AFFINITY_INFO;
+	psci_ops.affinity_info = psci_affinity_info;
+
+	psci_function_id[PSCI_FN_MIGRATE_INFO_TYPE] =
+		PSCI_0_2_FN_MIGRATE_INFO_TYPE;
+	psci_ops.migrate_info_type = psci_migrate_info_type;
+
+	arm_pm_restart = psci_sys_reset;
+
+	pm_power_off = psci_sys_poweroff;
+}
+
 /*
  * PSCI Function IDs for v0.2+ are well defined so use
  * standard values.
@@ -264,29 +293,7 @@ static int __init psci_0_2_init(struct device_node *np)
 		}
 	}
 
-	pr_info("Using standard PSCI v0.2 function IDs\n");
-	psci_function_id[PSCI_FN_CPU_SUSPEND] = PSCI_0_2_FN64_CPU_SUSPEND;
-	psci_ops.cpu_suspend = psci_cpu_suspend;
-
-	psci_function_id[PSCI_FN_CPU_OFF] = PSCI_0_2_FN_CPU_OFF;
-	psci_ops.cpu_off = psci_cpu_off;
-
-	psci_function_id[PSCI_FN_CPU_ON] = PSCI_0_2_FN64_CPU_ON;
-	psci_ops.cpu_on = psci_cpu_on;
-
-	psci_function_id[PSCI_FN_MIGRATE] = PSCI_0_2_FN64_MIGRATE;
-	psci_ops.migrate = psci_migrate;
-
-	psci_function_id[PSCI_FN_AFFINITY_INFO] = PSCI_0_2_FN64_AFFINITY_INFO;
-	psci_ops.affinity_info = psci_affinity_info;
-
-	psci_function_id[PSCI_FN_MIGRATE_INFO_TYPE] =
-		PSCI_0_2_FN_MIGRATE_INFO_TYPE;
-	psci_ops.migrate_info_type = psci_migrate_info_type;
-
-	arm_pm_restart = psci_sys_reset;
-
-	pm_power_off = psci_sys_poweroff;
+	psci_0_2_set_functions();
 
 out_put_node:
 	of_node_put(np);
@@ -339,7 +346,7 @@ static const struct of_device_id psci_of_match[] __initconst = {
 	{},
 };
 
-int __init psci_init(void)
+int __init psci_dt_init(void)
 {
 	struct device_node *np;
 	const struct of_device_id *matched_np;
@@ -354,6 +361,29 @@ int __init psci_init(void)
 	return init_fn(np);
 }
 
+/*
+ * We use PSCI 0.2+ when ACPI is deployed on ARM64 and it's
+ * explicitly clarified in SBBR
+ */
+int __init psci_acpi_init(void)
+{
+	if (!acpi_psci_present()) {
+		pr_info("is not implemented in ACPI.\n");
+		return -EOPNOTSUPP;
+	}
+
+	pr_info("probing for conduit method from ACPI.\n");
+
+	if (acpi_psci_use_hvc())
+		invoke_psci_fn = __invoke_psci_fn_hvc;
+	else
+		invoke_psci_fn = __invoke_psci_fn_smc;
+
+	psci_0_2_set_functions();
+
+	return 0;
+}
+
 #ifdef CONFIG_SMP
 
 static int __init cpu_psci_cpu_init(struct device_node *dn, unsigned int cpu)
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 5796734..7ba20d4 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -398,10 +398,12 @@ void __init setup_arch(char **cmdline_p)
 	efi_idmap_init();
 
 	cpu_logical_map(0) = read_cpuid_mpidr() & MPIDR_HWID_BITMASK;
-	if (acpi_disabled)
+	if (acpi_disabled) {
 		unflatten_device_tree();
-
-	psci_init();
+		psci_dt_init();
+	} else {
+		psci_acpi_init();
+	}
 
 	cpu_read_bootcpu_ops();
 #ifdef CONFIG_SMP
-- 
1.7.9.5

Re: [PATCH v4 09/18] ARM64 / ACPI: Parse FADT table to get PSCI flags for PSCI init

[Olof Johansson]

On Fri, Sep 12, 2014 at 10:00:07PM +0800, Hanjun Guo wrote:
> diff --git a/arch/arm64/include/asm/psci.h b/arch/arm64/include/asm/psci.h
> index e5312ea..2454bc5 100644
> --- a/arch/arm64/include/asm/psci.h
> +++ b/arch/arm64/include/asm/psci.h
> @@ -14,6 +14,7 @@
>  #ifndef __ASM_PSCI_H
>  #define __ASM_PSCI_H
>  
> -int psci_init(void);
> +int psci_dt_init(void);
> +int psci_acpi_init(void);
>  
>  #endif /* __ASM_PSCI_H */
> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> index 693da7f..9e1ae37 100644
> --- a/arch/arm64/kernel/acpi.c
> +++ b/arch/arm64/kernel/acpi.c
> @@ -13,6 +13,8 @@
>   *  published by the Free Software Foundation.
>   */
>  
> +#define pr_fmt(fmt) "ACPI: " fmt
> +
>  #include <linux/init.h>
>  #include <linux/acpi.h>
>  #include <linux/cpumask.h>
> @@ -49,10 +51,32 @@ void __init __acpi_unmap_table(char *map, unsigned long size)
>  	early_memunmap(map, size);
>  }
>  
> +static int __init acpi_parse_fadt(struct acpi_table_header *table)
> +{
> +	struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table;
> +
> +	/*
> +	 * Revision in table header is the FADT Major revision,
> +	 * and there is a minor revision of FADT which was introduced
> +	 * by ACPI 5.1, we only deal with ACPI 5.1 or higher revision
> +	 * to get arm boot flags, or we will disable ACPI.
> +	 */
> +	if (table->revision > 5 ||
> +	    (table->revision == 5 && fadt->minor_revision >= 1))
> +		return 0;

The commend and code disagree. The code says 5.1 or newer, while the comment say
_only_ 5.1. One or the other needs to change.

[...]

> diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c
> index 5539547..15ba470 100644
> --- a/arch/arm64/kernel/psci.c
> +++ b/arch/arm64/kernel/psci.c
> @@ -15,6 +15,7 @@
>  
>  #define pr_fmt(fmt) "psci: " fmt
>  
> +#include <linux/acpi.h>
>  #include <linux/init.h>
>  #include <linux/of.h>
>  #include <linux/smp.h>
> @@ -23,6 +24,7 @@
>  #include <linux/delay.h>
>  #include <uapi/linux/psci.h>
>  
> +#include <asm/acpi.h>
>  #include <asm/compiler.h>
>  #include <asm/cpu_ops.h>
>  #include <asm/errno.h>
> @@ -231,6 +233,33 @@ static void psci_sys_poweroff(void)
>  	invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
>  }
>  
> +static void psci_0_2_set_functions(void)
> +{
> +	pr_info("Using standard PSCI v0.2 function IDs\n");
> +	psci_function_id[PSCI_FN_CPU_SUSPEND] = PSCI_0_2_FN64_CPU_SUSPEND;
> +	psci_ops.cpu_suspend = psci_cpu_suspend;
> +
> +	psci_function_id[PSCI_FN_CPU_OFF] = PSCI_0_2_FN_CPU_OFF;
> +	psci_ops.cpu_off = psci_cpu_off;
> +
> +	psci_function_id[PSCI_FN_CPU_ON] = PSCI_0_2_FN64_CPU_ON;
> +	psci_ops.cpu_on = psci_cpu_on;
> +
> +	psci_function_id[PSCI_FN_MIGRATE] = PSCI_0_2_FN64_MIGRATE;
> +	psci_ops.migrate = psci_migrate;
> +
> +	psci_function_id[PSCI_FN_AFFINITY_INFO] = PSCI_0_2_FN64_AFFINITY_INFO;
> +	psci_ops.affinity_info = psci_affinity_info;
> +
> +	psci_function_id[PSCI_FN_MIGRATE_INFO_TYPE] =
> +		PSCI_0_2_FN_MIGRATE_INFO_TYPE;
> +	psci_ops.migrate_info_type = psci_migrate_info_type;
> +
> +	arm_pm_restart = psci_sys_reset;
> +
> +	pm_power_off = psci_sys_poweroff;
> +}
> +
>  /*
>   * PSCI Function IDs for v0.2+ are well defined so use
>   * standard values.
> @@ -264,29 +293,7 @@ static int __init psci_0_2_init(struct device_node *np)
>  		}
>  	}
>  
> -	pr_info("Using standard PSCI v0.2 function IDs\n");
> -	psci_function_id[PSCI_FN_CPU_SUSPEND] = PSCI_0_2_FN64_CPU_SUSPEND;
> -	psci_ops.cpu_suspend = psci_cpu_suspend;
> -
> -	psci_function_id[PSCI_FN_CPU_OFF] = PSCI_0_2_FN_CPU_OFF;
> -	psci_ops.cpu_off = psci_cpu_off;
> -
> -	psci_function_id[PSCI_FN_CPU_ON] = PSCI_0_2_FN64_CPU_ON;
> -	psci_ops.cpu_on = psci_cpu_on;
> -
> -	psci_function_id[PSCI_FN_MIGRATE] = PSCI_0_2_FN64_MIGRATE;
> -	psci_ops.migrate = psci_migrate;
> -
> -	psci_function_id[PSCI_FN_AFFINITY_INFO] = PSCI_0_2_FN64_AFFINITY_INFO;
> -	psci_ops.affinity_info = psci_affinity_info;
> -
> -	psci_function_id[PSCI_FN_MIGRATE_INFO_TYPE] =
> -		PSCI_0_2_FN_MIGRATE_INFO_TYPE;
> -	psci_ops.migrate_info_type = psci_migrate_info_type;
> -
> -	arm_pm_restart = psci_sys_reset;
> -
> -	pm_power_off = psci_sys_poweroff;
> +	psci_0_2_set_functions();
>  
>  out_put_node:
>  	of_node_put(np);
> @@ -339,7 +346,7 @@ static const struct of_device_id psci_of_match[] __initconst = {
>  	{},
>  };
>  
> -int __init psci_init(void)
> +int __init psci_dt_init(void)
>  {
>  	struct device_node *np;
>  	const struct of_device_id *matched_np;
> @@ -354,6 +361,29 @@ int __init psci_init(void)
>  	return init_fn(np);
>  }
>  
> +/*
> + * We use PSCI 0.2+ when ACPI is deployed on ARM64 and it's
> + * explicitly clarified in SBBR
> + */
> +int __init psci_acpi_init(void)
> +{
> +	if (!acpi_psci_present()) {
> +		pr_info("is not implemented in ACPI.\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	pr_info("probing for conduit method from ACPI.\n");
> +
> +	if (acpi_psci_use_hvc())
> +		invoke_psci_fn = __invoke_psci_fn_hvc;
> +	else
> +		invoke_psci_fn = __invoke_psci_fn_smc;
> +
> +	psci_0_2_set_functions();
> +
> +	return 0;
> +}
> +
>  #ifdef CONFIG_SMP
>  
>  static int __init cpu_psci_cpu_init(struct device_node *dn, unsigned int cpu)
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index 5796734..7ba20d4 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -398,10 +398,12 @@ void __init setup_arch(char **cmdline_p)
>  	efi_idmap_init();
>  
>  	cpu_logical_map(0) = read_cpuid_mpidr() & MPIDR_HWID_BITMASK;
> -	if (acpi_disabled)
> +	if (acpi_disabled) {
>  		unflatten_device_tree();
> -
> -	psci_init();
> +		psci_dt_init();
> +	} else {
> +		psci_acpi_init();
> +	}

This is slightly nitty now, but I think it'd be slightly easier to follow
boot path if the switch for acpi disabled/enabled was in psci.c. It'd also allow
psci_dt_init() and psci_acpi_init() to be static.

I.e. keep a psci_init() and just make it call one or the other.


-Olof

Re: [PATCH v4 09/18] ARM64 / ACPI: Parse FADT table to get PSCI flags for PSCI init

[Catalin Marinas]

On Mon, Sep 15, 2014 at 07:38:08AM +0100, Olof Johansson wrote:
> On Fri, Sep 12, 2014 at 10:00:07PM +0800, Hanjun Guo wrote:
> > +static int __init acpi_parse_fadt(struct acpi_table_header *table)
> > +{
> > +	struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table;
> > +
> > +	/*
> > +	 * Revision in table header is the FADT Major revision,
> > +	 * and there is a minor revision of FADT which was introduced
> > +	 * by ACPI 5.1, we only deal with ACPI 5.1 or higher revision
> > +	 * to get arm boot flags, or we will disable ACPI.
> > +	 */
> > +	if (table->revision > 5 ||
> > +	    (table->revision == 5 && fadt->minor_revision >= 1))
> > +		return 0;
> 
> The commend and code disagree. The code says 5.1 or newer, while the comment say
> _only_ 5.1. One or the other needs to change.

I don't think the comment and code disagree, it says "we only deal with
ACPI 5.1 _or_ higher revision".

But since you mention it and following my other thread with Grant, I
think at least for now we should make this check 5.1 only.

-- 
Catalin

Re: [PATCH v4 09/18] ARM64 / ACPI: Parse FADT table to get PSCI flags for PSCI init

[Jon Masters]

On 09/15/2014 10:54 AM, Catalin Marinas wrote:
> On Mon, Sep 15, 2014 at 07:38:08AM +0100, Olof Johansson wrote:
>> On Fri, Sep 12, 2014 at 10:00:07PM +0800, Hanjun Guo wrote:
>>> +static int __init acpi_parse_fadt(struct acpi_table_header *table)
>>> +{
>>> +	struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table;
>>> +
>>> +	/*
>>> +	 * Revision in table header is the FADT Major revision,
>>> +	 * and there is a minor revision of FADT which was introduced
>>> +	 * by ACPI 5.1, we only deal with ACPI 5.1 or higher revision
>>> +	 * to get arm boot flags, or we will disable ACPI.
>>> +	 */
>>> +	if (table->revision > 5 ||
>>> +	    (table->revision == 5 && fadt->minor_revision >= 1))
>>> +		return 0;
>>
>> The commend and code disagree. The code says 5.1 or newer, while the comment say
>> _only_ 5.1. One or the other needs to change.
> 
> I don't think the comment and code disagree, it says "we only deal with
> ACPI 5.1 _or_ higher revision".
> 
> But since you mention it and following my other thread with Grant, I
> think at least for now we should make this check 5.1 only.

The problem with making this 5.1 only is that the specification will
move forward. The way the specification works is that the version
increments, structures grow new members, but additions are done in a
backward-compatible way. Therefore there's no reason why an ACPI6.0
system (in theory) should not be able to boot a 5.1 kernel[0]. By
artificially restricting this compatibility, users are worse off.

Jon.

[0] There will be examples where this is not true - let's say there's a
GICvX specification that's incompatible with GICv3+ and GICvX folks
don't necessarily choose to implement backward compatibility for GICv3.
If there's a new MADT subtype record handling GICvX then the OS would
need to support minimally the ACPI rev that adds this type.

Re: [PATCH v4 09/18] ARM64 / ACPI: Parse FADT table to get PSCI flags for PSCI init

[Hanjun Guo]

On 2014年09月15日 14:38, Olof Johansson wrote:
> On Fri, Sep 12, 2014 at 10:00:07PM +0800, Hanjun Guo wrote:
>> diff --git a/arch/arm64/include/asm/psci.h b/arch/arm64/include/asm/psci.h
>> index e5312ea..2454bc5 100644
>> --- a/arch/arm64/include/asm/psci.h
>> +++ b/arch/arm64/include/asm/psci.h
>> @@ -14,6 +14,7 @@
>>  #ifndef __ASM_PSCI_H
>>  #define __ASM_PSCI_H
>>  
>> -int psci_init(void);
>> +int psci_dt_init(void);
>> +int psci_acpi_init(void);
>>  
>>  #endif /* __ASM_PSCI_H */
>> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
>> index 693da7f..9e1ae37 100644
>> --- a/arch/arm64/kernel/acpi.c
>> +++ b/arch/arm64/kernel/acpi.c
>> @@ -13,6 +13,8 @@
>>   *  published by the Free Software Foundation.
>>   */
>>  
>> +#define pr_fmt(fmt) "ACPI: " fmt
>> +
>>  #include <linux/init.h>
>>  #include <linux/acpi.h>
>>  #include <linux/cpumask.h>
>> @@ -49,10 +51,32 @@ void __init __acpi_unmap_table(char *map, unsigned long size)
>>  	early_memunmap(map, size);
>>  }
>>  
>> +static int __init acpi_parse_fadt(struct acpi_table_header *table)
>> +{
>> +	struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table;
>> +
>> +	/*
>> +	 * Revision in table header is the FADT Major revision,
>> +	 * and there is a minor revision of FADT which was introduced
>> +	 * by ACPI 5.1, we only deal with ACPI 5.1 or higher revision
>> +	 * to get arm boot flags, or we will disable ACPI.
>> +	 */
>> +	if (table->revision > 5 ||
>> +	    (table->revision == 5 && fadt->minor_revision >= 1))
>> +		return 0;
> The commend and code disagree. The code says 5.1 or newer, while the comment say
> _only_ 5.1. One or the other needs to change.

I describe it in the comments "ACPI 5.1 or higher revision", maybe
"newer revision" will be more explicit.

>
> [...]
>
>> diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c
>> index 5539547..15ba470 100644
>> --- a/arch/arm64/kernel/psci.c
>> +++ b/arch/arm64/kernel/psci.c
>> @@ -15,6 +15,7 @@
>>  
>>  #define pr_fmt(fmt) "psci: " fmt
>>  
>> +#include <linux/acpi.h>
>>  #include <linux/init.h>
>>  #include <linux/of.h>
>>  #include <linux/smp.h>
>> @@ -23,6 +24,7 @@
>>  #include <linux/delay.h>
>>  #include <uapi/linux/psci.h>
>>  
>> +#include <asm/acpi.h>
>>  #include <asm/compiler.h>
>>  #include <asm/cpu_ops.h>
>>  #include <asm/errno.h>
>> @@ -231,6 +233,33 @@ static void psci_sys_poweroff(void)
>>  	invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
>>  }
>>  
>> +static void psci_0_2_set_functions(void)
>> +{
>> +	pr_info("Using standard PSCI v0.2 function IDs\n");
>> +	psci_function_id[PSCI_FN_CPU_SUSPEND] = PSCI_0_2_FN64_CPU_SUSPEND;
>> +	psci_ops.cpu_suspend = psci_cpu_suspend;
>> +
>> +	psci_function_id[PSCI_FN_CPU_OFF] = PSCI_0_2_FN_CPU_OFF;
>> +	psci_ops.cpu_off = psci_cpu_off;
>> +
>> +	psci_function_id[PSCI_FN_CPU_ON] = PSCI_0_2_FN64_CPU_ON;
>> +	psci_ops.cpu_on = psci_cpu_on;
>> +
>> +	psci_function_id[PSCI_FN_MIGRATE] = PSCI_0_2_FN64_MIGRATE;
>> +	psci_ops.migrate = psci_migrate;
>> +
>> +	psci_function_id[PSCI_FN_AFFINITY_INFO] = PSCI_0_2_FN64_AFFINITY_INFO;
>> +	psci_ops.affinity_info = psci_affinity_info;
>> +
>> +	psci_function_id[PSCI_FN_MIGRATE_INFO_TYPE] =
>> +		PSCI_0_2_FN_MIGRATE_INFO_TYPE;
>> +	psci_ops.migrate_info_type = psci_migrate_info_type;
>> +
>> +	arm_pm_restart = psci_sys_reset;
>> +
>> +	pm_power_off = psci_sys_poweroff;
>> +}
>> +
>>  /*
>>   * PSCI Function IDs for v0.2+ are well defined so use
>>   * standard values.
>> @@ -264,29 +293,7 @@ static int __init psci_0_2_init(struct device_node *np)
>>  		}
>>  	}
>>  
>> -	pr_info("Using standard PSCI v0.2 function IDs\n");
>> -	psci_function_id[PSCI_FN_CPU_SUSPEND] = PSCI_0_2_FN64_CPU_SUSPEND;
>> -	psci_ops.cpu_suspend = psci_cpu_suspend;
>> -
>> -	psci_function_id[PSCI_FN_CPU_OFF] = PSCI_0_2_FN_CPU_OFF;
>> -	psci_ops.cpu_off = psci_cpu_off;
>> -
>> -	psci_function_id[PSCI_FN_CPU_ON] = PSCI_0_2_FN64_CPU_ON;
>> -	psci_ops.cpu_on = psci_cpu_on;
>> -
>> -	psci_function_id[PSCI_FN_MIGRATE] = PSCI_0_2_FN64_MIGRATE;
>> -	psci_ops.migrate = psci_migrate;
>> -
>> -	psci_function_id[PSCI_FN_AFFINITY_INFO] = PSCI_0_2_FN64_AFFINITY_INFO;
>> -	psci_ops.affinity_info = psci_affinity_info;
>> -
>> -	psci_function_id[PSCI_FN_MIGRATE_INFO_TYPE] =
>> -		PSCI_0_2_FN_MIGRATE_INFO_TYPE;
>> -	psci_ops.migrate_info_type = psci_migrate_info_type;
>> -
>> -	arm_pm_restart = psci_sys_reset;
>> -
>> -	pm_power_off = psci_sys_poweroff;
>> +	psci_0_2_set_functions();
>>  
>>  out_put_node:
>>  	of_node_put(np);
>> @@ -339,7 +346,7 @@ static const struct of_device_id psci_of_match[] __initconst = {
>>  	{},
>>  };
>>  
>> -int __init psci_init(void)
>> +int __init psci_dt_init(void)
>>  {
>>  	struct device_node *np;
>>  	const struct of_device_id *matched_np;
>> @@ -354,6 +361,29 @@ int __init psci_init(void)
>>  	return init_fn(np);
>>  }
>>  
>> +/*
>> + * We use PSCI 0.2+ when ACPI is deployed on ARM64 and it's
>> + * explicitly clarified in SBBR
>> + */
>> +int __init psci_acpi_init(void)
>> +{
>> +	if (!acpi_psci_present()) {
>> +		pr_info("is not implemented in ACPI.\n");
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	pr_info("probing for conduit method from ACPI.\n");
>> +
>> +	if (acpi_psci_use_hvc())
>> +		invoke_psci_fn = __invoke_psci_fn_hvc;
>> +	else
>> +		invoke_psci_fn = __invoke_psci_fn_smc;
>> +
>> +	psci_0_2_set_functions();
>> +
>> +	return 0;
>> +}
>> +
>>  #ifdef CONFIG_SMP
>>  
>>  static int __init cpu_psci_cpu_init(struct device_node *dn, unsigned int cpu)
>> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
>> index 5796734..7ba20d4 100644
>> --- a/arch/arm64/kernel/setup.c
>> +++ b/arch/arm64/kernel/setup.c
>> @@ -398,10 +398,12 @@ void __init setup_arch(char **cmdline_p)
>>  	efi_idmap_init();
>>  
>>  	cpu_logical_map(0) = read_cpuid_mpidr() & MPIDR_HWID_BITMASK;
>> -	if (acpi_disabled)
>> +	if (acpi_disabled) {
>>  		unflatten_device_tree();
>> -
>> -	psci_init();
>> +		psci_dt_init();
>> +	} else {
>> +		psci_acpi_init();
>> +	}
> This is slightly nitty now, but I think it'd be slightly easier to follow
> boot path if the switch for acpi disabled/enabled was in psci.c. It'd also allow
> psci_dt_init() and psci_acpi_init() to be static.
>
> I.e. keep a psci_init() and just make it call one or the other.

Do you mean something like blow in psci.c?

int __init psci_init(void)
{
if (acpi_disabled)
return psci_dt_init();
else
return psci_acpi_init();
}

Thanks
Hanjun

[PATCH v4 10/18] ACPI / table: Print GIC information when MADT is parsed

[Hanjun Guo]

When MADT is parsed, print GIC information to make the boot
log look pretty.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
---
 drivers/acpi/tables.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index b18e45e..47f36d4 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -183,6 +183,49 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
 		}
 		break;
 
+	case ACPI_MADT_TYPE_GENERIC_INTERRUPT:
+		{
+			struct acpi_madt_generic_interrupt *p =
+				(struct acpi_madt_generic_interrupt *)header;
+			pr_info("GICC (acpi_id[0x%04x] address[%p] MPDIR[0x%llx] %s)\n",
+				p->uid, (void *)(unsigned long)p->base_address,
+				p->arm_mpidr,
+				(p->flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
+
+		}
+		break;
+
+	case ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR:
+		{
+			struct acpi_madt_generic_distributor *p =
+				(struct acpi_madt_generic_distributor *)header;
+			pr_info("GIC Distributor (gic_id[0x%04x] address[%p] gsi_base[%d])\n",
+				p->gic_id,
+				(void *)(unsigned long)p->base_address,
+				p->global_irq_base);
+		}
+		break;
+
+	case ACPI_MADT_TYPE_GENERIC_MSI_FRAME:
+		{
+			struct acpi_madt_generic_msi_frame *p =
+				(struct acpi_madt_generic_msi_frame *)header;
+			pr_info("GIC MSI Frame (msi_fame_id[%d] address[%p])\n",
+				p->msi_frame_id,
+				(void *)(unsigned long)p->base_address);
+		}
+		break;
+
+	case ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR:
+		{
+			struct acpi_madt_generic_redistributor *p =
+				(struct acpi_madt_generic_redistributor *)header;
+			pr_info("GIC Redistributor (address[%p] region_size[0x%x])\n",
+				(void *)(unsigned long)p->base_address,
+				p->length);
+		}
+		break;
+
 	default:
 		pr_warn("Found unsupported MADT entry (type = 0x%x)\n",
 			header->type);
-- 
1.7.9.5

Re: [PATCH v4 10/18] ACPI / table: Print GIC information when MADT is parsed

[Olof Johansson]

On Fri, Sep 12, 2014 at 10:00:08PM +0800, Hanjun Guo wrote:
> When MADT is parsed, print GIC information to make the boot
> log look pretty.

If you do this for prettiness, having an example output in the commit
message would be a nice touch.

The question is why it's needed though? Do we really need all this verbose
information in dmesg when it could be found out in other ways instead?

x86 is notoriously verbose in some of these aspects, I'm not sure it's
something we need to mimic here?


-Olof

Re: [PATCH v4 10/18] ACPI / table: Print GIC information when MADT is parsed

[Jon Masters]

Hi Olof,

How about having it verbose in the initial implementation (in case there are any fun times bringing up early systems) and then revisiting this? x86 is very verbose but it can be helpful.

Jon.

-- 
Computer Architect | Sent from my #ARM Powered Mobile device

On Sep 15, 2014 12:34 AM, Olof Johansson <olof@lixom.net> wrote:
>
> On Fri, Sep 12, 2014 at 10:00:08PM +0800, Hanjun Guo wrote: 
> > When MADT is parsed, print GIC information to On Fri, Sep 12, 2014 at 10:00:08PM +0800, Hanjun Guo wrote:
> When MADT is parsed, print GIC information to make the boot
> log look pretty.

If you do this for prettiness, having an example output in the commit
message would be a nice touch.

The question is why it's needed though? Do we really need all this verbose
information in dmesg when it could be found out in other ways instead?

x86 is notoriously verbose in some of these aspects, I'm not sure it's
something we need to mimic here?


-Olof

Re: [PATCH v4 10/18] ACPI / table: Print GIC information when MADT is parsed

[Grant Likely]

On Sun, 14 Sep 2014 23:40:10 -0700, Olof Johansson <olof@lixom.net> wrote:
> On Fri, Sep 12, 2014 at 10:00:08PM +0800, Hanjun Guo wrote:
> > When MADT is parsed, print GIC information to make the boot
> > log look pretty.
> 
> If you do this for prettiness, having an example output in the commit
> message would be a nice touch.

Agreed.

> The question is why it's needed though? Do we really need all this verbose
> information in dmesg when it could be found out in other ways instead?
> 
> x86 is notoriously verbose in some of these aspects, I'm not sure it's
> something we need to mimic here?

For consistency, it does make sense to have the common x86 and ARM64
code outputing information in the same way, so I think the patch still
makes sense. All of them could be moved to KERN_DEBUG as a separate
patch if we want to cull early boot output. I would split that off and
discuss it separately.

g.

Re: [PATCH v4 10/18] ACPI / table: Print GIC information when MADT is parsed

[Olof Johansson]

On Mon, Sep 15, 2014 at 10:52 AM, Grant Likely <grant.likely@linaro.org> wrote:
> On Sun, 14 Sep 2014 23:40:10 -0700, Olof Johansson <olof@lixom.net> wrote:
>> On Fri, Sep 12, 2014 at 10:00:08PM +0800, Hanjun Guo wrote:
>> > When MADT is parsed, print GIC information to make the boot
>> > log look pretty.
>>
>> If you do this for prettiness, having an example output in the commit
>> message would be a nice touch.
>
> Agreed.
>
>> The question is why it's needed though? Do we really need all this verbose
>> information in dmesg when it could be found out in other ways instead?
>>
>> x86 is notoriously verbose in some of these aspects, I'm not sure it's
>> something we need to mimic here?
>
> For consistency, it does make sense to have the common x86 and ARM64
> code outputing information in the same way, so I think the patch still
> makes sense. All of them could be moved to KERN_DEBUG as a separate
> patch if we want to cull early boot output. I would split that off and
> discuss it separately.

Yeah, pr_debug() makes sense here -- in particular if you're going to
have a potentially slow serial console to print all this to at info
level.

I disagree on order though, there's no reason to hold off fixing this
and doing it as a separate patch.


-Olof

Re: [PATCH v4 10/18] ACPI / table: Print GIC information when MADT is parsed

[Jon Masters]

On 09/15/2014 02:01 PM, Olof Johansson wrote:
> On Mon, Sep 15, 2014 at 10:52 AM, Grant Likely <grant.likely@linaro.org> wrote:
>> On Sun, 14 Sep 2014 23:40:10 -0700, Olof Johansson <olof@lixom.net> wrote:
>>> On Fri, Sep 12, 2014 at 10:00:08PM +0800, Hanjun Guo wrote:
>>>> When MADT is parsed, print GIC information to make the boot
>>>> log look pretty.
>>>
>>> If you do this for prettiness, having an example output in the commit
>>> message would be a nice touch.
>>
>> Agreed.

Here's example output from a hardware platform (just as an example in
this thread, mostly for archiving purposes):

[    0.000000] ACPI: GICC (acpi_id[0x0000] address[00000000e112f000]
MPDIR[0x0] enabled)
[    0.000000] ACPI: GICC (acpi_id[0x0001] address[00000000e112f000]
MPDIR[0x1] enabled)
[    0.000000] ACPI: GICC (acpi_id[0x0100] address[00000000e112f000]
MPDIR[0x100] enabled)
[    0.000000] ACPI: GICC (acpi_id[0x0101] address[00000000e112f000]
MPDIR[0x101] enabled)
[    0.000000] ACPI: GICC (acpi_id[0x0200] address[00000000e112f000]
MPDIR[0x200] enabled)
[    0.000000] ACPI: GICC (acpi_id[0x0201] address[00000000e112f000]
MPDIR[0x201] enabled)

Jon.

[PATCH v4 11/18] ARM64 / ACPI: Parse MADT for SMP initialization

[Hanjun Guo]

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.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
---
 arch/arm64/include/asm/acpi.h    |    2 +
 arch/arm64/include/asm/cpu_ops.h |    1 +
 arch/arm64/include/asm/smp.h     |    5 +-
 arch/arm64/kernel/acpi.c         |  147 +++++++++++++++++++++++++++++++++++++-
 arch/arm64/kernel/cpu_ops.c      |    4 +-
 arch/arm64/kernel/setup.c        |    8 ++-
 arch/arm64/kernel/smp.c          |    2 +-
 7 files changed, 160 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index ecba671..da8f74a 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -51,11 +51,13 @@ static inline bool acpi_has_cpu_in_madt(void)
 }
 
 static inline void arch_fix_phys_package_id(int num, u32 slot) { }
+void __init acpi_smp_init_cpus(void);
 
 #else
 
 static inline bool acpi_psci_present(void) { return false; }
 static inline bool acpi_psci_use_hvc(void) { return false; }
+static inline void acpi_smp_init_cpus(void) { }
 
 #endif /* CONFIG_ACPI */
 
diff --git a/arch/arm64/include/asm/cpu_ops.h b/arch/arm64/include/asm/cpu_ops.h
index d7b4b38..d149580 100644
--- a/arch/arm64/include/asm/cpu_ops.h
+++ b/arch/arm64/include/asm/cpu_ops.h
@@ -61,6 +61,7 @@ struct cpu_operations {
 };
 
 extern const struct cpu_operations *cpu_ops[NR_CPUS];
+const struct cpu_operations *cpu_get_ops(const char *name);
 extern int __init cpu_read_ops(struct device_node *dn, int cpu);
 extern void __init cpu_read_bootcpu_ops(void);
 
diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h
index a498f2c..c877adc 100644
--- a/arch/arm64/include/asm/smp.h
+++ b/arch/arm64/include/asm/smp.h
@@ -39,9 +39,10 @@ extern void show_ipi_list(struct seq_file *p, int prec);
 extern void handle_IPI(int ipinr, struct pt_regs *regs);
 
 /*
- * Setup the set of possible CPUs (via set_cpu_possible)
+ * Discover the set of possible CPUs and determine their
+ * SMP operations.
  */
-extern void smp_init_cpus(void);
+extern void of_smp_init_cpus(void);
 
 /*
  * Provide a function to raise an IPI cross call on CPUs in callmap.
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 9e1ae37..644b8b8 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -24,6 +24,10 @@
 #include <linux/bootmem.h>
 #include <linux/smp.h>
 
+#include <asm/smp_plat.h>
+#include <asm/cputype.h>
+#include <asm/cpu_ops.h>
+
 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 */
+
 /*
  * __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,131 @@ 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++;
+	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;
+			}
+		}
+		
+		/* allocate a logical cpu id for the new comer */
+		cpu = cpumask_next_zero(-1, cpu_possible_mask);
+	} else {
+		/* First GICC entry must be BSP as ACPI spec said */
+		if  (cpu_logical_map(0) != mpidr) {
+			pr_err("First GICC entry with MPIDR 0x%llx is not BSP\n",
+			       mpidr);
+			return -EINVAL;
+		}
+
+		/*
+		 * boot_cpu_init() already hold bit 0 in cpu_present_mask
+		 * for BSP, no need to allocate again.
+		 */
+		cpu = 0;
+	}
+
+	/* CPU 0 was already initialized */
+	if (cpu) {
+		cpu_ops[cpu] = cpu_get_ops(acpi_psci_present() ? "psci" : NULL);
+		if (!cpu_ops[cpu])
+			return -EINVAL;
+
+		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);
+	} else {
+		/* get cpu0's ops, no need to return if ops is null */
+		cpu_ops[0] = cpu_get_ops(acpi_psci_present() ? "psci" : NULL);
+	}
+
+	enabled_cpus++;
+	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 & MPIDR_HWID_BITMASK,
+		processor->flags & ACPI_MADT_ENABLED);
+
+	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, 0);
+
+	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;
+	}
+
+	/* Make boot-up look pretty */
+	pr_info("%d CPUs enabled, %d CPUs total\n", enabled_cpus, total_cpus);
+}
+
 static int __init acpi_parse_fadt(struct acpi_table_header *table)
 {
 	struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table;
@@ -62,8 +193,20 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table)
 	 * to get arm boot flags, or we will disable ACPI.
 	 */
 	if (table->revision > 5 ||
-	    (table->revision == 5 && fadt->minor_revision >= 1))
-		return 0;
+	    (table->revision == 5 && fadt->minor_revision >= 1)) {
+		/*
+		 * 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())
+			return 0;
+
+		pr_warn("has no PSCI support, will not bring up secondary CPUs\n");
+		return -EOPNOTSUPP;
+	}
 
 	pr_warn("Unsupported FADT revision %d.%d, should be 5.1+, will disable ACPI\n",
 		table->revision, fadt->minor_revision);
diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
index cce9524..1a04deb 100644
--- a/arch/arm64/kernel/cpu_ops.c
+++ b/arch/arm64/kernel/cpu_ops.c
@@ -27,7 +27,7 @@ extern const struct cpu_operations cpu_psci_ops;
 
 const struct cpu_operations *cpu_ops[NR_CPUS];
 
-static const struct cpu_operations *supported_cpu_ops[] __initconst = {
+static const struct cpu_operations *supported_cpu_ops[] = {
 #ifdef CONFIG_SMP
 	&smp_spin_table_ops,
 #endif
@@ -35,7 +35,7 @@ static const struct cpu_operations *supported_cpu_ops[] __initconst = {
 	NULL,
 };
 
-static const struct cpu_operations * __init cpu_get_ops(const char *name)
+const struct cpu_operations *cpu_get_ops(const char *name)
 {
 	const struct cpu_operations **ops = supported_cpu_ops;
 
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 7ba20d4..281fa34 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -60,6 +60,7 @@
 #include <asm/memblock.h>
 #include <asm/psci.h>
 #include <asm/efi.h>
+#include <asm/acpi.h>
 
 unsigned int processor_id;
 EXPORT_SYMBOL(processor_id);
@@ -401,13 +402,16 @@ void __init setup_arch(char **cmdline_p)
 	if (acpi_disabled) {
 		unflatten_device_tree();
 		psci_dt_init();
+		cpu_read_bootcpu_ops();
+#ifdef CONFIG_SMP
+		of_smp_init_cpus();
+#endif
 	} else {
 		psci_acpi_init();
+		acpi_smp_init_cpus();
 	}
 
-	cpu_read_bootcpu_ops();
 #ifdef CONFIG_SMP
-	smp_init_cpus();
 	smp_build_mpidr_hash();
 #endif
 
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 4743397..4e390ac 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -321,7 +321,7 @@ void __init smp_prepare_boot_cpu(void)
  * cpu logical map array containing MPIDR values related to logical
  * cpus. Assumes that cpu_logical_map(0) has already been initialized.
  */
-void __init smp_init_cpus(void)
+void __init of_smp_init_cpus(void)
 {
 	struct device_node *dn = NULL;
 	unsigned int i, cpu = 1;
-- 
1.7.9.5

Re: [PATCH v4 11/18] ARM64 / ACPI: Parse MADT for SMP initialization

[Jon Masters]

On 09/12/2014 10:00 AM, 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.

As an aside, initial support for the Parking Protocol is working on test
systems within Red Hat at this time. Additional patches for that will be
posted separately, however Parking Protocol is not the preferred option
on ARMv8 (PSCI should be the default). Consequently, support for those
limited number of systems doing Parking Protocol can follow after the
initial support for longer term PSCI is enabled.

Jon.

Re: [PATCH v4 11/18] ARM64 / ACPI: Parse MADT for SMP initialization

[Catalin Marinas]

On Fri, Sep 12, 2014 at 08:47:00PM +0100, Jon Masters wrote:
> On 09/12/2014 10:00 AM, 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.
> 
> As an aside, initial support for the Parking Protocol is working on test
> systems within Red Hat at this time.

BTW, have the details of the parking protocol been updated for ARMv8
yet?

-- 
Catalin

Re: [PATCH v4 11/18] ARM64 / ACPI: Parse MADT for SMP initialization

[Jon Masters]

On 09/15/2014 10:56 AM, Catalin Marinas wrote:
> On Fri, Sep 12, 2014 at 08:47:00PM +0100, Jon Masters wrote:
>> On 09/12/2014 10:00 AM, 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.
>>
>> As an aside, initial support for the Parking Protocol is working on test
>> systems within Red Hat at this time.
> 
> BTW, have the details of the parking protocol been updated for ARMv8
> yet?

There's an interim specification for which initial (test, in
development) patches are currently in the fedorahosted development
kernel. Mark is planning to followup on that once he's had chance to
polish and clean things up. But the parking protocol is not required for
initial ACPI enablement (we really want everyone to be doing PSCI
anyway, it's only a small subset that will ever do parking protocol).

Jon.

Re: [PATCH v4 11/18] ARM64 / ACPI: Parse MADT for SMP initialization

[Olof Johansson]

On Fri, Sep 12, 2014 at 10:00:09PM +0800, 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.
> 
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
> ---
>  arch/arm64/include/asm/acpi.h    |    2 +
>  arch/arm64/include/asm/cpu_ops.h |    1 +
>  arch/arm64/include/asm/smp.h     |    5 +-
>  arch/arm64/kernel/acpi.c         |  147 +++++++++++++++++++++++++++++++++++++-
>  arch/arm64/kernel/cpu_ops.c      |    4 +-
>  arch/arm64/kernel/setup.c        |    8 ++-
>  arch/arm64/kernel/smp.c          |    2 +-
>  7 files changed, 160 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> index ecba671..da8f74a 100644
> --- a/arch/arm64/include/asm/acpi.h
> +++ b/arch/arm64/include/asm/acpi.h
> @@ -51,11 +51,13 @@ static inline bool acpi_has_cpu_in_madt(void)
>  }
>  
>  static inline void arch_fix_phys_package_id(int num, u32 slot) { }
> +void __init acpi_smp_init_cpus(void);
>  
>  #else
>  
>  static inline bool acpi_psci_present(void) { return false; }
>  static inline bool acpi_psci_use_hvc(void) { return false; }
> +static inline void acpi_smp_init_cpus(void) { }
>  
>  #endif /* CONFIG_ACPI */
>  
> diff --git a/arch/arm64/include/asm/cpu_ops.h b/arch/arm64/include/asm/cpu_ops.h
> index d7b4b38..d149580 100644
> --- a/arch/arm64/include/asm/cpu_ops.h
> +++ b/arch/arm64/include/asm/cpu_ops.h
> @@ -61,6 +61,7 @@ struct cpu_operations {
>  };
>  
>  extern const struct cpu_operations *cpu_ops[NR_CPUS];
> +const struct cpu_operations *cpu_get_ops(const char *name);
>  extern int __init cpu_read_ops(struct device_node *dn, int cpu);
>  extern void __init cpu_read_bootcpu_ops(void);
>  
> diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h
> index a498f2c..c877adc 100644
> --- a/arch/arm64/include/asm/smp.h
> +++ b/arch/arm64/include/asm/smp.h
> @@ -39,9 +39,10 @@ extern void show_ipi_list(struct seq_file *p, int prec);
>  extern void handle_IPI(int ipinr, struct pt_regs *regs);
>  
>  /*
> - * Setup the set of possible CPUs (via set_cpu_possible)
> + * Discover the set of possible CPUs and determine their
> + * SMP operations.
>   */
> -extern void smp_init_cpus(void);
> +extern void of_smp_init_cpus(void);
>  
>  /*
>   * Provide a function to raise an IPI cross call on CPUs in callmap.
> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> index 9e1ae37..644b8b8 100644
> --- a/arch/arm64/kernel/acpi.c
> +++ b/arch/arm64/kernel/acpi.c
> @@ -24,6 +24,10 @@
>  #include <linux/bootmem.h>
>  #include <linux/smp.h>
>  
> +#include <asm/smp_plat.h>
> +#include <asm/cputype.h>
> +#include <asm/cpu_ops.h>
> +
>  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 */
> +
>  /*
>   * __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,131 @@ 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");

This message, when showing up in dmesg, will probably mostly just make
someone scratch their head. Something slightly more descriptive would
be a good idea.

> +		return -EINVAL;
> +	}
> +
> +	total_cpus++;
> +	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.
> +		 */

But is it this entry or the other one that should be ignored? Is
this expected to be something that firmware vendors get wrong all the
time? Would it be better to abort SMP alltogether?

> +		for_each_possible_cpu(cpu) {
> +			if (cpu_logical_map(cpu) == mpidr) {
> +				pr_err("Firmware bug, duplicate CPU MPIDR: 0x%llx in MADT\n",
> +				mpidr);

Misindented.

> +				return -EINVAL;
> +			}
> +		}
> +		
> +		/* allocate a logical cpu id for the new comer */
> +		cpu = cpumask_next_zero(-1, cpu_possible_mask);
> +	} else {
> +		/* First GICC entry must be BSP as ACPI spec said */

"spec said", would be nice to have a chapter/section reference.

> +		if  (cpu_logical_map(0) != mpidr) {
> +			pr_err("First GICC entry with MPIDR 0x%llx is not BSP\n",
> +			       mpidr);

Same thing here about usefulness of error message. What is a user to do with this?

> +			return -EINVAL;
> +		}
> +
> +		/*
> +		 * boot_cpu_init() already hold bit 0 in cpu_present_mask
> +		 * for BSP, no need to allocate again.
> +		 */
> +		cpu = 0;
> +	}
> +
> +	/* CPU 0 was already initialized */
> +	if (cpu) {
> +		cpu_ops[cpu] = cpu_get_ops(acpi_psci_present() ? "psci" : NULL);
> +		if (!cpu_ops[cpu])
> +			return -EINVAL;
> +
> +		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);
> +	} else {
> +		/* get cpu0's ops, no need to return if ops is null */
> +		cpu_ops[0] = cpu_get_ops(acpi_psci_present() ? "psci" : NULL);
> +	}
> +
> +	enabled_cpus++;
> +	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 & MPIDR_HWID_BITMASK,
> +		processor->flags & ACPI_MADT_ENABLED);
> +
> +	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, 0);
> +
> +	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;
> +	}
> +
> +	/* Make boot-up look pretty */
> +	pr_info("%d CPUs enabled, %d CPUs total\n", enabled_cpus, total_cpus);
> +}
> +
>  static int __init acpi_parse_fadt(struct acpi_table_header *table)
>  {
>  	struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table;
> @@ -62,8 +193,20 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table)
>  	 * to get arm boot flags, or we will disable ACPI.
>  	 */
>  	if (table->revision > 5 ||
> -	    (table->revision == 5 && fadt->minor_revision >= 1))
> -		return 0;
> +	    (table->revision == 5 && fadt->minor_revision >= 1)) {
> +		/*
> +		 * 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())
> +			return 0;
> +
> +		pr_warn("has no PSCI support, will not bring up secondary CPUs\n");

s/has//

> +		return -EOPNOTSUPP;
> +	}
>  
>  	pr_warn("Unsupported FADT revision %d.%d, should be 5.1+, will disable ACPI\n",
>  		table->revision, fadt->minor_revision);
> diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
> index cce9524..1a04deb 100644
> --- a/arch/arm64/kernel/cpu_ops.c
> +++ b/arch/arm64/kernel/cpu_ops.c
> @@ -27,7 +27,7 @@ extern const struct cpu_operations cpu_psci_ops;
>  
>  const struct cpu_operations *cpu_ops[NR_CPUS];
>  
> -static const struct cpu_operations *supported_cpu_ops[] __initconst = {
> +static const struct cpu_operations *supported_cpu_ops[] = {
>  #ifdef CONFIG_SMP
>  	&smp_spin_table_ops,
>  #endif
> @@ -35,7 +35,7 @@ static const struct cpu_operations *supported_cpu_ops[] __initconst = {
>  	NULL,
>  };
>  
> -static const struct cpu_operations * __init cpu_get_ops(const char *name)
> +const struct cpu_operations *cpu_get_ops(const char *name)
>  {
>  	const struct cpu_operations **ops = supported_cpu_ops;
>  
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index 7ba20d4..281fa34 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -60,6 +60,7 @@
>  #include <asm/memblock.h>
>  #include <asm/psci.h>
>  #include <asm/efi.h>
> +#include <asm/acpi.h>
>  
>  unsigned int processor_id;
>  EXPORT_SYMBOL(processor_id);
> @@ -401,13 +402,16 @@ void __init setup_arch(char **cmdline_p)
>  	if (acpi_disabled) {
>  		unflatten_device_tree();
>  		psci_dt_init();
> +		cpu_read_bootcpu_ops();
> +#ifdef CONFIG_SMP
> +		of_smp_init_cpus();
> +#endif

Please create a !SMP stub so you can do without the ifdef here, just
like you did for the ACPI case.

>  	} else {
>  		psci_acpi_init();
> +		acpi_smp_init_cpus();
>  	}
>  
> -	cpu_read_bootcpu_ops();
>  #ifdef CONFIG_SMP
> -	smp_init_cpus();
>  	smp_build_mpidr_hash();
>  #endif
>  
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index 4743397..4e390ac 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -321,7 +321,7 @@ void __init smp_prepare_boot_cpu(void)
>   * cpu logical map array containing MPIDR values related to logical
>   * cpus. Assumes that cpu_logical_map(0) has already been initialized.
>   */
> -void __init smp_init_cpus(void)
> +void __init of_smp_init_cpus(void)
>  {
>  	struct device_node *dn = NULL;
>  	unsigned int i, cpu = 1;
> -- 
> 1.7.9.5
> 


-Olof

Re: [PATCH v4 11/18] ARM64 / ACPI: Parse MADT for SMP initialization

[Hanjun Guo]

On 2014年09月15日 15:00, Olof Johansson wrote:
> On Fri, Sep 12, 2014 at 10:00:09PM +0800, 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.
>>
>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>> Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
>> ---
>>  arch/arm64/include/asm/acpi.h    |    2 +
>>  arch/arm64/include/asm/cpu_ops.h |    1 +
>>  arch/arm64/include/asm/smp.h     |    5 +-
>>  arch/arm64/kernel/acpi.c         |  147 +++++++++++++++++++++++++++++++++++++-
>>  arch/arm64/kernel/cpu_ops.c      |    4 +-
>>  arch/arm64/kernel/setup.c        |    8 ++-
>>  arch/arm64/kernel/smp.c          |    2 +-
>>  7 files changed, 160 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
>> index ecba671..da8f74a 100644
>> --- a/arch/arm64/include/asm/acpi.h
>> +++ b/arch/arm64/include/asm/acpi.h
>> @@ -51,11 +51,13 @@ static inline bool acpi_has_cpu_in_madt(void)
>>  }
>>  
>>  static inline void arch_fix_phys_package_id(int num, u32 slot) { }
>> +void __init acpi_smp_init_cpus(void);
>>  
>>  #else
>>  
>>  static inline bool acpi_psci_present(void) { return false; }
>>  static inline bool acpi_psci_use_hvc(void) { return false; }
>> +static inline void acpi_smp_init_cpus(void) { }
>>  
>>  #endif /* CONFIG_ACPI */
>>  
>> diff --git a/arch/arm64/include/asm/cpu_ops.h b/arch/arm64/include/asm/cpu_ops.h
>> index d7b4b38..d149580 100644
>> --- a/arch/arm64/include/asm/cpu_ops.h
>> +++ b/arch/arm64/include/asm/cpu_ops.h
>> @@ -61,6 +61,7 @@ struct cpu_operations {
>>  };
>>  
>>  extern const struct cpu_operations *cpu_ops[NR_CPUS];
>> +const struct cpu_operations *cpu_get_ops(const char *name);
>>  extern int __init cpu_read_ops(struct device_node *dn, int cpu);
>>  extern void __init cpu_read_bootcpu_ops(void);
>>  
>> diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h
>> index a498f2c..c877adc 100644
>> --- a/arch/arm64/include/asm/smp.h
>> +++ b/arch/arm64/include/asm/smp.h
>> @@ -39,9 +39,10 @@ extern void show_ipi_list(struct seq_file *p, int prec);
>>  extern void handle_IPI(int ipinr, struct pt_regs *regs);
>>  
>>  /*
>> - * Setup the set of possible CPUs (via set_cpu_possible)
>> + * Discover the set of possible CPUs and determine their
>> + * SMP operations.
>>   */
>> -extern void smp_init_cpus(void);
>> +extern void of_smp_init_cpus(void);
>>  
>>  /*
>>   * Provide a function to raise an IPI cross call on CPUs in callmap.
>> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
>> index 9e1ae37..644b8b8 100644
>> --- a/arch/arm64/kernel/acpi.c
>> +++ b/arch/arm64/kernel/acpi.c
>> @@ -24,6 +24,10 @@
>>  #include <linux/bootmem.h>
>>  #include <linux/smp.h>
>>  
>> +#include <asm/smp_plat.h>
>> +#include <asm/cputype.h>
>> +#include <asm/cpu_ops.h>
>> +
>>  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 */
>> +
>>  /*
>>   * __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,131 @@ 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");
> This message, when showing up in dmesg, will probably mostly just make
> someone scratch their head. Something slightly more descriptive would
> be a good idea.

I agree. how about "Skip MADT cpu entry with invalid MPIDR" ?

>
>> +		return -EINVAL;
>> +	}
>> +
>> +	total_cpus++;
>> +	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.
>> +		 */
> But is it this entry or the other one that should be ignored? 

Only this entry will be ignored, I did the same thing as DT parsing
CPU nodes.

> Is
> this expected to be something that firmware vendors get wrong all the
> time? Would it be better to abort SMP alltogether?

I think we can still boot other CPUs with correct MPIDRs, and ignore
the wrong ones.

>
>> +		for_each_possible_cpu(cpu) {
>> +			if (cpu_logical_map(cpu) == mpidr) {
>> +				pr_err("Firmware bug, duplicate CPU MPIDR: 0x%llx in MADT\n",
>> +				mpidr);
> Misindented.

Will update.

>
>> +				return -EINVAL;
>> +			}
>> +		}
>> +		
>> +		/* allocate a logical cpu id for the new comer */
>> +		cpu = cpumask_next_zero(-1, cpu_possible_mask);
>> +	} else {
>> +		/* First GICC entry must be BSP as ACPI spec said */
> "spec said", would be nice to have a chapter/section reference.

ok.

>
>> +		if  (cpu_logical_map(0) != mpidr) {
>> +			pr_err("First GICC entry with MPIDR 0x%llx is not BSP\n",
>> +			       mpidr);
> Same thing here about usefulness of error message. What is a user to do with this?

I'm using this to debug the MADT table in UEFI, and correct them if this printed.

>
>> +			return -EINVAL;
>> +		}
>> +
>> +		/*
>> +		 * boot_cpu_init() already hold bit 0 in cpu_present_mask
>> +		 * for BSP, no need to allocate again.
>> +		 */
>> +		cpu = 0;
>> +	}
>> +
>> +	/* CPU 0 was already initialized */
>> +	if (cpu) {
>> +		cpu_ops[cpu] = cpu_get_ops(acpi_psci_present() ? "psci" : NULL);
>> +		if (!cpu_ops[cpu])
>> +			return -EINVAL;
>> +
>> +		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);
>> +	} else {
>> +		/* get cpu0's ops, no need to return if ops is null */
>> +		cpu_ops[0] = cpu_get_ops(acpi_psci_present() ? "psci" : NULL);
>> +	}
>> +
>> +	enabled_cpus++;
>> +	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 & MPIDR_HWID_BITMASK,
>> +		processor->flags & ACPI_MADT_ENABLED);
>> +
>> +	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, 0);
>> +
>> +	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;
>> +	}
>> +
>> +	/* Make boot-up look pretty */
>> +	pr_info("%d CPUs enabled, %d CPUs total\n", enabled_cpus, total_cpus);
>> +}
>> +
>>  static int __init acpi_parse_fadt(struct acpi_table_header *table)
>>  {
>>  	struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table;
>> @@ -62,8 +193,20 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table)
>>  	 * to get arm boot flags, or we will disable ACPI.
>>  	 */
>>  	if (table->revision > 5 ||
>> -	    (table->revision == 5 && fadt->minor_revision >= 1))
>> -		return 0;
>> +	    (table->revision == 5 && fadt->minor_revision >= 1)) {
>> +		/*
>> +		 * 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())
>> +			return 0;
>> +
>> +		pr_warn("has no PSCI support, will not bring up secondary CPUs\n");
> s/has//
>
>> +		return -EOPNOTSUPP;
>> +	}
>>  
>>  	pr_warn("Unsupported FADT revision %d.%d, should be 5.1+, will disable ACPI\n",
>>  		table->revision, fadt->minor_revision);
>> diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
>> index cce9524..1a04deb 100644
>> --- a/arch/arm64/kernel/cpu_ops.c
>> +++ b/arch/arm64/kernel/cpu_ops.c
>> @@ -27,7 +27,7 @@ extern const struct cpu_operations cpu_psci_ops;
>>  
>>  const struct cpu_operations *cpu_ops[NR_CPUS];
>>  
>> -static const struct cpu_operations *supported_cpu_ops[] __initconst = {
>> +static const struct cpu_operations *supported_cpu_ops[] = {
>>  #ifdef CONFIG_SMP
>>  	&smp_spin_table_ops,
>>  #endif
>> @@ -35,7 +35,7 @@ static const struct cpu_operations *supported_cpu_ops[] __initconst = {
>>  	NULL,
>>  };
>>  
>> -static const struct cpu_operations * __init cpu_get_ops(const char *name)
>> +const struct cpu_operations *cpu_get_ops(const char *name)
>>  {
>>  	const struct cpu_operations **ops = supported_cpu_ops;
>>  
>> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
>> index 7ba20d4..281fa34 100644
>> --- a/arch/arm64/kernel/setup.c
>> +++ b/arch/arm64/kernel/setup.c
>> @@ -60,6 +60,7 @@
>>  #include <asm/memblock.h>
>>  #include <asm/psci.h>
>>  #include <asm/efi.h>
>> +#include <asm/acpi.h>
>>  
>>  unsigned int processor_id;
>>  EXPORT_SYMBOL(processor_id);
>> @@ -401,13 +402,16 @@ void __init setup_arch(char **cmdline_p)
>>  	if (acpi_disabled) {
>>  		unflatten_device_tree();
>>  		psci_dt_init();
>> +		cpu_read_bootcpu_ops();
>> +#ifdef CONFIG_SMP
>> +		of_smp_init_cpus();
>> +#endif
> Please create a !SMP stub so you can do without the ifdef here, just
> like you did for the ACPI case.

I like this, will update the patch. :)

Thanks
Hanjun

[PATCH v4 12/18] ACPI / processor: Make it possible to get CPU hardware ID via GICC

[Hanjun Guo]

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 da8f74a..2c48f70e 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.
+ */
+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 644b8b8..2ef8721 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

Re: [PATCH v4 12/18] ACPI / processor: Make it possible to get CPU hardware ID via GICC

[Olof Johansson]

On Fri, Sep 12, 2014 at 10:00:10PM +0800, 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 da8f74a..2c48f70e 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.
> + */

This is a pretty run-on comment.

I'd much rather value something like:

Existing apic_id is 32-bit, to conform to the same datatype we need to repack the GICC
structure MPIDR.

The two formats are:

<show how the 64-bit format is laid out>
<show how the 32-bit format is laid out>


> +static inline u32 pack_mpidr_into_32_bits(u64 mpidr)

That's a pretty long name for an exported function.

> +{
> +	/*
> +	 * Bits [0:7] Aff0;
> +	 * Bits [8:15] Aff1;
> +	 * Bits [16:23] Aff2;
> +	 * Bits [32:39] Aff3;
> +	 */

With the format description above added, then this comment can be removed.

> +	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 644b8b8..2ef8721 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.
> +		 */

Here's duplicated information again. Reusing the same macro as in the header file
would help overall readability, I think.

> +		*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
> 

Re: [PATCH v4 12/18] ACPI / processor: Make it possible to get CPU hardware ID via GICC

[Hanjun Guo]

On 2014年09月15日 15:05, Olof Johansson wrote:
> On Fri, Sep 12, 2014 at 10:00:10PM +0800, 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 da8f74a..2c48f70e 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.
>> + */
> This is a pretty run-on comment.
>
> I'd much rather value something like:
>
> Existing apic_id is 32-bit, to conform to the same datatype we need to repack the GICC
> structure MPIDR.
>
> The two formats are:
>
> <show how the 64-bit format is laid out>
> <show how the 32-bit format is laid out>
>
>
>> +static inline u32 pack_mpidr_into_32_bits(u64 mpidr)
> That's a pretty long name for an exported function.
>
>> +{
>> +	/*
>> +	 * Bits [0:7] Aff0;
>> +	 * Bits [8:15] Aff1;
>> +	 * Bits [16:23] Aff2;
>> +	 * Bits [32:39] Aff3;
>> +	 */
> With the format description above added, then this comment can be removed.

ok, I will update it.

>
>> +	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 644b8b8..2ef8721 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.
>> +		 */
> Here's duplicated information again. Reusing the same macro as in the header file
> would help overall readability, I think.

The problem is that it will cause compile error on x86 and ia64 platform,
it is ARM64 specific only.

>
>> +		*mpidr = ((gicc->arm_mpidr & 0xff00000000) >> 8)
>> +			 | gicc->arm_mpidr;
>> +		return 0;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +

[PATCH v4 13/18] ARM64 / ACPI: Introduce ACPI_IRQ_MODEL_GIC and register device's gsi

[Hanjun Guo]

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 2ef8721..5b3546b 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.
  */
@@ -181,6 +187,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);
+
+/*
+ * 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];
+	 */
+
+	if (trigger == ACPI_EDGE_SENSITIVE &&
+				polarity == ACPI_ACTIVE_LOW)
+		irq_type = IRQ_TYPE_EDGE_FALLING;
+	else if (trigger == ACPI_EDGE_SENSITIVE &&
+				polarity == ACPI_ACTIVE_HIGH)
+		irq_type = IRQ_TYPE_EDGE_RISING;
+	else if (trigger == ACPI_LEVEL_SENSITIVE &&
+				polarity == ACPI_ACTIVE_LOW)
+		irq_type = IRQ_TYPE_LEVEL_LOW;
+	else if (trigger == ACPI_LEVEL_SENSITIVE &&
+				polarity == ACPI_ACTIVE_HIGH)
+		irq_type = IRQ_TYPE_LEVEL_HIGH;
+	else
+		irq_type = IRQ_TYPE_NONE;
+
+	/*
+	 * Since only one GIC is supported in ACPI 5.0, we can
+	 * create mapping refer to the default domain
+	 */
+	irq = irq_create_mapping(NULL, gsi);
+	if (!irq)
+		return irq;
+
+	/* Set irq type if specified and different than the current one */
+	if (irq_type != IRQ_TYPE_NONE &&
+		irq_type != irq_get_trigger_type(irq))
+		irq_set_irq_type(irq, irq_type);
+	return irq;
+}
+EXPORT_SYMBOL_GPL(acpi_register_gsi);
+
+void acpi_unregister_gsi(u32 gsi)
+{
+}
+EXPORT_SYMBOL_GPL(acpi_unregister_gsi);
+
 static int __init acpi_parse_fadt(struct acpi_table_header *table)
 {
 	struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table;
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 8581f5b..d132c1b 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -458,6 +458,9 @@ static int __init acpi_bus_init_irq(void)
 	case ACPI_IRQ_MODEL_IOSAPIC:
 		message = "IOSAPIC";
 		break;
+	case ACPI_IRQ_MODEL_GIC:
+		message = "GIC";
+		break;
 	case ACPI_IRQ_MODEL_PLATFORM:
 		message = "platform specific model";
 		break;
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 36d5012..4615eb1 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -71,6 +71,7 @@ enum acpi_irq_model_id {
 	ACPI_IRQ_MODEL_IOAPIC,
 	ACPI_IRQ_MODEL_IOSAPIC,
 	ACPI_IRQ_MODEL_PLATFORM,
+	ACPI_IRQ_MODEL_GIC,
 	ACPI_IRQ_MODEL_COUNT
 };
 
-- 
1.7.9.5

[PATCH v4 14/18] ARM64 / ACPI: Add GICv2 specific ACPI boot support

[Hanjun Guo]

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 basic functionality.
GICv2 vitalization extension, GICv3/4 and ITS are considered as next
steps.

Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 arch/arm64/kernel/acpi.c             |   23 ++++++++
 drivers/irqchip/irq-gic.c            |  106 ++++++++++++++++++++++++++++++++++
 drivers/irqchip/irqchip.c            |    3 +
 include/linux/irqchip/arm-gic-acpi.h |   31 ++++++++++
 4 files changed, 163 insertions(+)
 create mode 100644 include/linux/irqchip/arm-gic-acpi.h

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 5b3546b..9869377 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>
@@ -312,6 +313,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);
+}
+
 static int __init parse_acpi(char *arg)
 {
 	if (!arg)
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 4b959e6..8aba096 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,107 @@ 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 phys_addr_t dist_phy_base, cpu_phy_base;
+static int cpu_base_assigned;
+
+static int __init
+gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
+			const unsigned long end)
+{
+	struct acpi_madt_generic_interrupt *processor;
+	phys_addr_t gic_cpu_base;
+
+	processor = (struct acpi_madt_generic_interrupt *)header;
+
+	if (BAD_MADT_ENTRY(processor, end))
+		return -EINVAL;
+
+	/*
+	 * There is no support for non-banked GICv1/2 register in ACPI spec.
+	 * All CPU interface addresses have to be the same.
+	 */
+	gic_cpu_base = processor->base_address;
+	if (cpu_base_assigned && gic_cpu_base != cpu_phy_base)
+		return -EFAULT;
+
+	cpu_phy_base = gic_cpu_base;
+	cpu_base_assigned = 1;
+	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;
+	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, 0);
+	if (count < 0) {
+		pr_err("Error during GICC entries parsing\n");
+		return -EFAULT;
+	} else if (!count) {
+		pr_err("No valid GICC entries exist\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Find distributor base address. We expect one distributor entry since
+	 * ACPI 5.1 spec neither support multi-GIC instances nor GIC cascade.
+	 */
+	count = acpi_parse_entries(sizeof(struct acpi_table_madt),
+				   gic_acpi_parse_madt_distributor, table,
+				   ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, 0);
+	if (count <= 0) {
+		pr_err("Error during GICD entries parsing\n");
+		return -EFAULT;
+	} else if (!count) {
+		pr_err("No valid GICD entries exist\n");
+		return -EINVAL;
+	} else if (count > 1) {
+		pr_err("More than one GICD entry detected\n");
+		return -EINVAL;
+	}
+
+	cpu_base = ioremap(cpu_phy_base, ACPI_GIC_CPU_IF_MEM_SIZE);
+	if (!cpu_base) {
+		pr_err("Unable to map GICC registers\n");
+		return -ENOMEM;
+	}
+
+	dist_base = ioremap(dist_phy_base, ACPI_GICV2_DIST_MEM_SIZE);
+	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/drivers/irqchip/irqchip.c b/drivers/irqchip/irqchip.c
index 0fe2f71..9106c6d 100644
--- a/drivers/irqchip/irqchip.c
+++ b/drivers/irqchip/irqchip.c
@@ -11,6 +11,7 @@
 #include <linux/init.h>
 #include <linux/of_irq.h>
 #include <linux/irqchip.h>
+#include <linux/irqchip/arm-gic-acpi.h>
 
 /*
  * This special of_device_id is the sentinel at the end of the
@@ -26,4 +27,6 @@ extern struct of_device_id __irqchip_of_table[];
 void __init irqchip_init(void)
 {
 	of_irq_init(__irqchip_of_table);
+
+	acpi_gic_init();
 }
diff --git a/include/linux/irqchip/arm-gic-acpi.h b/include/linux/irqchip/arm-gic-acpi.h
new file mode 100644
index 0000000..ad5b577
--- /dev/null
+++ b/include/linux/irqchip/arm-gic-acpi.h
@@ -0,0 +1,31 @@
+/*
+ * Copyright (C) 2014, Linaro Ltd.
+ *	Author: Tomasz Nowicki <tomasz.nowicki@linaro.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef ARM_GIC_ACPI_H_
+#define ARM_GIC_ACPI_H_
+
+#ifdef CONFIG_ACPI
+
+/*
+ * Hard code here, we can not get memory size from MADT (but FDT does),
+ * Actually no need to do that, because this size can be inferred
+ * from GIC spec.
+ */
+#define ACPI_GICV2_DIST_MEM_SIZE	(SZ_4K)
+#define ACPI_GIC_CPU_IF_MEM_SIZE	(SZ_8K)
+
+struct acpi_table_header;
+
+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_ */
-- 
1.7.9.5

Re: [PATCH v4 14/18] ARM64 / ACPI: Add GICv2 specific ACPI boot support

[Jon Masters]

On 09/12/2014 10:00 AM, 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 basic functionality.
> GICv2 vitalization extension, GICv3/4 and ITS are considered as next
> steps.

Aside: A request to specifically update the flags in ACPI to indicate
GICv3/v4 systems has been filed for a future spec revision.

Jon.

Re: [PATCH v4 14/18] ARM64 / ACPI: Add GICv2 specific ACPI boot support

[Catalin Marinas]

On Fri, Sep 12, 2014 at 03:00:12PM +0100, Hanjun Guo wrote:
> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> index 5b3546b..9869377 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>
> @@ -312,6 +313,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);
> +}

Maybe this was discussed already but why does this function need to live
under arch/arm64? Isn't the driver code more appropriate?

-- 
Catalin

Re: [PATCH v4 14/18] ARM64 / ACPI: Add GICv2 specific ACPI boot support

[Jon Masters]

On 09/15/2014 11:01 AM, Catalin Marinas wrote:
> On Fri, Sep 12, 2014 at 03:00:12PM +0100, Hanjun Guo wrote:
>> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
>> index 5b3546b..9869377 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>
>> @@ -312,6 +313,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);
>> +}
> 
> Maybe this was discussed already but why does this function need to live
> under arch/arm64? Isn't the driver code more appropriate?

Well there's two halves to this, right? There's the MADT parsing/setup,
which is architecture specific, and then there's the GIC irqchip
initialization which lives under drivers.

Jon.

Re: [PATCH v4 14/18] ARM64 / ACPI: Add GICv2 specific ACPI boot support

[Catalin Marinas]

On Mon, Sep 15, 2014 at 05:16:21PM +0100, Jon Masters wrote:
> On 09/15/2014 11:01 AM, Catalin Marinas wrote:
> > On Fri, Sep 12, 2014 at 03:00:12PM +0100, Hanjun Guo wrote:
> >> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> >> index 5b3546b..9869377 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>
> >> @@ -312,6 +313,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);
> >> +}
> > 
> > Maybe this was discussed already but why does this function need to live
> > under arch/arm64? Isn't the driver code more appropriate?
> 
> Well there's two halves to this, right? There's the MADT parsing/setup,
> which is architecture specific, and then there's the GIC irqchip
> initialization which lives under drivers.

I think it gets worse, this function is called from irqchip_init(). I
would have been slightly happier if it was called from the arm64
init_IRQ(). But putting an ARM specific GIC initialisation call in a
generic irqchip_init() just looks weird. Can we do anything better here?

> 
> Jon.
> 
> 

-- 
Catalin

Re: [PATCH v4 14/18] ARM64 / ACPI: Add GICv2 specific ACPI boot support

[Tomasz Nowicki]

On 15.09.2014 18:42, Catalin Marinas wrote:
> On Mon, Sep 15, 2014 at 05:16:21PM +0100, Jon Masters wrote:
>> On 09/15/2014 11:01 AM, Catalin Marinas wrote:
>>> On Fri, Sep 12, 2014 at 03:00:12PM +0100, Hanjun Guo wrote:
>>>> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
>>>> index 5b3546b..9869377 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>
>>>> @@ -312,6 +313,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);
>>>> +}
>>>
>>> Maybe this was discussed already but why does this function need to live
>>> under arch/arm64? Isn't the driver code more appropriate?

There will be another call here for GICv3 so this function will merge 
common logic for them. As Jon pointed out, we are planning to add ACPI 
flag which indicates GICv3 or GICv2(m) IRQ controller in advance.

>>
>> Well there's two halves to this, right? There's the MADT parsing/setup,
>> which is architecture specific, and then there's the GIC irqchip
>> initialization which lives under drivers.
>
> I think it gets worse, this function is called from irqchip_init(). I
> would have been slightly happier if it was called from the arm64
> init_IRQ(). But putting an ARM specific GIC initialisation call in a
> generic irqchip_init() just looks weird. Can we do anything better here?

Yes this was discussed, please have a look at: 
https://lkml.org/lkml/2014/9/1/555
We had this in init_IRQ() in previous patch set, then we got feedback 
irqchip_init() is more appropriate. We can move it back to init_IRQ() 
and I am sold on this.

Tomasz

Re: [PATCH v4 14/18] ARM64 / ACPI: Add GICv2 specific ACPI boot support

[Catalin Marinas]

On Wed, Sep 17, 2014 at 08:40:08AM +0100, Tomasz Nowicki wrote:
> On 15.09.2014 18:42, Catalin Marinas wrote:
> > On Mon, Sep 15, 2014 at 05:16:21PM +0100, Jon Masters wrote:
> >> On 09/15/2014 11:01 AM, Catalin Marinas wrote:
> >>> On Fri, Sep 12, 2014 at 03:00:12PM +0100, Hanjun Guo wrote:
> >>>> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> >>>> index 5b3546b..9869377 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>
> >>>> @@ -312,6 +313,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);
> >>>> +}
> >>>
> >>> Maybe this was discussed already but why does this function need to live
> >>> under arch/arm64? Isn't the driver code more appropriate?
> 
> There will be another call here for GICv3 so this function will merge 
> common logic for them. As Jon pointed out, we are planning to add ACPI 
> flag which indicates GICv3 or GICv2(m) IRQ controller in advance.

We have GIC stuff for ACPI scattered around the kernel
(arch/arm64/kernel/acpi.c, drivers/acpi/processor_core.c,
drivers/irqchip/). It would be nicer if we find some common place for
them, even if that is something under drivers/acpi/ or drivers/irqchip/
(we even have an irq-gic-common.c).

> >> Well there's two halves to this, right? There's the MADT parsing/setup,
> >> which is architecture specific, and then there's the GIC irqchip
> >> initialization which lives under drivers.
> >
> > I think it gets worse, this function is called from irqchip_init(). I
> > would have been slightly happier if it was called from the arm64
> > init_IRQ(). But putting an ARM specific GIC initialisation call in a
> > generic irqchip_init() just looks weird. Can we do anything better here?
> 
> Yes this was discussed, please have a look at: 
> https://lkml.org/lkml/2014/9/1/555
> We had this in init_IRQ() in previous patch set, then we got feedback 
> irqchip_init() is more appropriate. We can move it back to init_IRQ() 
> and I am sold on this.

The irqchip_init() is indeed the place to call other interrupt
controller initialisation functions but what I don't particularly like
is calling the GIC one directly while the OF ones are checked against a
match string. For GICv3 and later, do you plan to use the same
acpi_gic_init() functions? Otherwise we could do something like
ACPI_IRQCHIP_DECLARE() similar to the OF ones and a common function that
probes whatever is built into the kernel.

-- 
Catalin

Re: [PATCH v4 14/18] ARM64 / ACPI: Add GICv2 specific ACPI boot support

[Arnd Bergmann]

On Wednesday 17 September 2014, Catalin Marinas wrote:
> > > I think it gets worse, this function is called from irqchip_init(). I
> > > would have been slightly happier if it was called from the arm64
> > > init_IRQ(). But putting an ARM specific GIC initialisation call in a
> > > generic irqchip_init() just looks weird. Can we do anything better here?
> > 
> > Yes this was discussed, please have a look at: 
> > https://lkml.org/lkml/2014/9/1/555
> > We had this in init_IRQ() in previous patch set, then we got feedback 
> > irqchip_init() is more appropriate. We can move it back to init_IRQ() 
> > and I am sold on this.
> 
> The irqchip_init() is indeed the place to call other interrupt
> controller initialisation functions but what I don't particularly like
> is calling the GIC one directly while the OF ones are checked against a
> match string. For GICv3 and later, do you plan to use the same
> acpi_gic_init() functions? Otherwise we could do something like
> ACPI_IRQCHIP_DECLARE() similar to the OF ones and a common function that
> probes whatever is built into the kernel.

I talked abouto this with Marc Z the other day, and I think it really
comes down to how we expect this to develop in the future:

If this is going to stay with the GICv2/v3/v4 line of interrupt controllers,
I see the ACPI_IRQCHIP_DECLARE() as total overdesign, since we wouldn't
ever need more than two entry points.

Doing ACPI_IRQCHIP_DECLARE() makes it possible to deal with other
incompatible irqchips as they come along, but also seems to invite
those.

Marc believes that it's inevitable that people will add lots of crazy
interrupt controllers to systems using ACPI and at that point I agree
it would be the right way to deal with it. However, I also think that
as long as people expect to be able to add lots of crazy interrupt
controller drivers, we are not ready to merge ACPI in the first place,
because it must first be clear to everybody that we are not going to
allow those nonstandard controller drivers to get merged.

	Arnd

Re: [PATCH v4 14/18] ARM64 / ACPI: Add GICv2 specific ACPI boot support

[Catalin Marinas]

On Thu, Sep 18, 2014 at 03:25:29AM +0100, Arnd Bergmann wrote:
> On Wednesday 17 September 2014, Catalin Marinas wrote:
> > > > I think it gets worse, this function is called from irqchip_init(). I
> > > > would have been slightly happier if it was called from the arm64
> > > > init_IRQ(). But putting an ARM specific GIC initialisation call in a
> > > > generic irqchip_init() just looks weird. Can we do anything better here?
> > > 
> > > Yes this was discussed, please have a look at: 
> > > https://lkml.org/lkml/2014/9/1/555
> > > We had this in init_IRQ() in previous patch set, then we got feedback 
> > > irqchip_init() is more appropriate. We can move it back to init_IRQ() 
> > > and I am sold on this.
> > 
> > The irqchip_init() is indeed the place to call other interrupt
> > controller initialisation functions but what I don't particularly like
> > is calling the GIC one directly while the OF ones are checked against a
> > match string. For GICv3 and later, do you plan to use the same
> > acpi_gic_init() functions? Otherwise we could do something like
> > ACPI_IRQCHIP_DECLARE() similar to the OF ones and a common function that
> > probes whatever is built into the kernel.
> 
> I talked abouto this with Marc Z the other day, and I think it really
> comes down to how we expect this to develop in the future:
> 
> If this is going to stay with the GICv2/v3/v4 line of interrupt controllers,
> I see the ACPI_IRQCHIP_DECLARE() as total overdesign, since we wouldn't
> ever need more than two entry points.

I agree, if we are going to have a single acpi_gic_init() function
handling all versions of GIC then a macro isn't needed.

> Doing ACPI_IRQCHIP_DECLARE() makes it possible to deal with other
> incompatible irqchips as they come along, but also seems to invite
> those.
> 
> Marc believes that it's inevitable that people will add lots of crazy
> interrupt controllers to systems using ACPI and at that point I agree
> it would be the right way to deal with it. However, I also think that
> as long as people expect to be able to add lots of crazy interrupt
> controller drivers, we are not ready to merge ACPI in the first place,
> because it must first be clear to everybody that we are not going to
> allow those nonstandard controller drivers to get merged.

Sounds fine to me. So we leave this call in irqchip_init() and ignore
ACPI for platforms with non-standard interrupt controllers.

-- 
Catalin

[PATCH v4 15/18] ARM64 / ACPI: Parse GTDT to initialize arch timer

[Hanjun Guo]

Using the information presented by GTDT to initialize the arch
timer (not memory-mapped).

Originally-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 arch/arm64/kernel/time.c             |    7 ++
 drivers/clocksource/arm_arch_timer.c |  117 ++++++++++++++++++++++++++++------
 include/linux/clocksource.h          |    6 ++
 3 files changed, 111 insertions(+), 19 deletions(-)

diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c
index 1a7125c..42f9195 100644
--- a/arch/arm64/kernel/time.c
+++ b/arch/arm64/kernel/time.c
@@ -35,6 +35,7 @@
 #include <linux/delay.h>
 #include <linux/clocksource.h>
 #include <linux/clk-provider.h>
+#include <linux/acpi.h>
 
 #include <clocksource/arm_arch_timer.h>
 
@@ -72,6 +73,12 @@ void __init time_init(void)
 
 	tick_setup_hrtimer_broadcast();
 
+	/*
+	 * Since ACPI or FDT will only one be available in the system,
+	 * we can use acpi_generic_timer_init() here safely
+	 */
+	acpi_generic_timer_init();
+
 	arch_timer_rate = arch_timer_get_rate();
 	if (!arch_timer_rate)
 		panic("Unable to initialise architected timer.\n");
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 5163ec1..397783f 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -21,6 +21,7 @@
 #include <linux/io.h>
 #include <linux/slab.h>
 #include <linux/sched_clock.h>
+#include <linux/acpi.h>
 
 #include <asm/arch_timer.h>
 #include <asm/virt.h>
@@ -338,8 +339,12 @@ arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
 	if (arch_timer_rate)
 		return;
 
-	/* Try to determine the frequency from the device tree or CNTFRQ */
-	if (of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
+	/*
+	 * Try to determine the frequency from the device tree or CNTFRQ,
+	 * if ACPI is enabled, get the frequency from CNTFRQ ONLY.
+	 */
+	if (!acpi_disabled ||
+	    of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
 		if (cntbase)
 			arch_timer_rate = readl_relaxed(cntbase + CNTFRQ);
 		else
@@ -635,20 +640,8 @@ static void __init arch_timer_common_init(void)
 	arch_timer_arch_init();
 }
 
-static void __init arch_timer_init(struct device_node *np)
+static void __init arch_timer_init(void)
 {
-	int i;
-
-	if (arch_timers_present & ARCH_CP15_TIMER) {
-		pr_warn("arch_timer: multiple nodes in dt, skipping\n");
-		return;
-	}
-
-	arch_timers_present |= ARCH_CP15_TIMER;
-	for (i = PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++)
-		arch_timer_ppi[i] = irq_of_parse_and_map(np, i);
-	arch_timer_detect_rate(NULL, np);
-
 	/*
 	 * If HYP mode is available, we know that the physical timer
 	 * has been configured to be accessible from PL1. Use it, so
@@ -667,13 +660,31 @@ static void __init arch_timer_init(struct device_node *np)
 		}
 	}
 
-	arch_timer_c3stop = !of_property_read_bool(np, "always-on");
-
 	arch_timer_register();
 	arch_timer_common_init();
 }
-CLOCKSOURCE_OF_DECLARE(armv7_arch_timer, "arm,armv7-timer", arch_timer_init);
-CLOCKSOURCE_OF_DECLARE(armv8_arch_timer, "arm,armv8-timer", arch_timer_init);
+
+static void __init arch_timer_of_init(struct device_node *np)
+{
+	int i;
+
+	if (arch_timers_present & ARCH_CP15_TIMER) {
+		pr_warn("arch_timer: multiple nodes in dt, skipping\n");
+		return;
+	}
+
+	arch_timers_present |= ARCH_CP15_TIMER;
+	for (i = PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++)
+		arch_timer_ppi[i] = irq_of_parse_and_map(np, i);
+
+	arch_timer_detect_rate(NULL, np);
+
+	arch_timer_c3stop = !of_property_read_bool(np, "always-on");
+
+	arch_timer_init();
+}
+CLOCKSOURCE_OF_DECLARE(armv7_arch_timer, "arm,armv7-timer", arch_timer_of_init);
+CLOCKSOURCE_OF_DECLARE(armv8_arch_timer, "arm,armv8-timer", arch_timer_of_init);
 
 static void __init arch_timer_mem_init(struct device_node *np)
 {
@@ -740,3 +751,71 @@ static void __init arch_timer_mem_init(struct device_node *np)
 }
 CLOCKSOURCE_OF_DECLARE(armv7_arch_timer_mem, "arm,armv7-timer-mem",
 		       arch_timer_mem_init);
+
+#ifdef CONFIG_ACPI
+static int __init
+map_generic_timer_interrupt(u32 interrupt, u32 flags)
+{
+	int trigger, polarity;
+
+	if (!interrupt)
+		return 0;
+
+	trigger = (flags & ACPI_GTDT_INTERRUPT_MODE) ? ACPI_EDGE_SENSITIVE
+			: ACPI_LEVEL_SENSITIVE;
+
+	polarity = (flags & ACPI_GTDT_INTERRUPT_POLARITY) ? ACPI_ACTIVE_LOW
+			: ACPI_ACTIVE_HIGH;
+
+	return acpi_register_gsi(NULL, interrupt, trigger, polarity);
+}
+
+/* Initialize per-processor generic timer */
+static int __init arch_timer_acpi_init(struct acpi_table_header *table)
+{
+	struct acpi_table_gtdt *gtdt;
+
+	if (arch_timers_present & ARCH_CP15_TIMER) {
+		pr_warn("arch_timer: already initialized, skipping\n");
+		return -EINVAL;
+	}
+
+	gtdt = container_of(table, struct acpi_table_gtdt, header);
+
+	arch_timers_present |= ARCH_CP15_TIMER;
+
+	arch_timer_ppi[PHYS_SECURE_PPI] =
+		map_generic_timer_interrupt(gtdt->secure_el1_interrupt,
+		gtdt->secure_el1_flags);
+
+	arch_timer_ppi[PHYS_NONSECURE_PPI] =
+		map_generic_timer_interrupt(gtdt->non_secure_el1_interrupt,
+		gtdt->non_secure_el1_flags);
+
+	arch_timer_ppi[VIRT_PPI] =
+		map_generic_timer_interrupt(gtdt->virtual_timer_interrupt,
+		gtdt->virtual_timer_flags);
+
+	arch_timer_ppi[HYP_PPI] =
+		map_generic_timer_interrupt(gtdt->non_secure_el2_interrupt,
+		gtdt->non_secure_el2_flags);
+
+	/* Get the frequency from CNTFRQ */
+	arch_timer_detect_rate(NULL, NULL);
+
+	/* Always-on capability */
+	arch_timer_c3stop = !(gtdt->non_secure_el1_flags & ACPI_GTDT_ALWAYS_ON);
+
+	arch_timer_init();
+	return 0;
+}
+
+/* Initialize all the generic timers presented in GTDT */
+void __init acpi_generic_timer_init(void)
+{
+	if (acpi_disabled)
+		return;
+
+	acpi_table_parse(ACPI_SIG_GTDT, arch_timer_acpi_init);
+}
+#endif
diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index 653f0e2..5839f98 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -346,4 +346,10 @@ extern void clocksource_of_init(void);
 static inline void clocksource_of_init(void) {}
 #endif
 
+#ifdef CONFIG_ACPI
+void acpi_generic_timer_init(void);
+#else
+static inline void acpi_generic_timer_init(void) {}
+#endif
+
 #endif /* _LINUX_CLOCKSOURCE_H */
-- 
1.7.9.5

Re: [PATCH v4 15/18] ARM64 / ACPI: Parse GTDT to initialize arch timer

[Olof Johansson]

On Fri, Sep 12, 2014 at 10:00:13PM +0800, Hanjun Guo wrote:
> Using the information presented by GTDT to initialize the arch
> timer (not memory-mapped).
> 
> Originally-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> ---
>  arch/arm64/kernel/time.c             |    7 ++
>  drivers/clocksource/arm_arch_timer.c |  117 ++++++++++++++++++++++++++++------
>  include/linux/clocksource.h          |    6 ++
>  3 files changed, 111 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c
> index 1a7125c..42f9195 100644
> --- a/arch/arm64/kernel/time.c
> +++ b/arch/arm64/kernel/time.c
> @@ -35,6 +35,7 @@
>  #include <linux/delay.h>
>  #include <linux/clocksource.h>
>  #include <linux/clk-provider.h>
> +#include <linux/acpi.h>
>  
>  #include <clocksource/arm_arch_timer.h>
>  
> @@ -72,6 +73,12 @@ void __init time_init(void)
>  
>  	tick_setup_hrtimer_broadcast();
>  
> +	/*
> +	 * Since ACPI or FDT will only one be available in the system,
> +	 * we can use acpi_generic_timer_init() here safely
> +	 */
> +	acpi_generic_timer_init();
> +
>  	arch_timer_rate = arch_timer_get_rate();
>  	if (!arch_timer_rate)
>  		panic("Unable to initialise architected timer.\n");
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 5163ec1..397783f 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -21,6 +21,7 @@
>  #include <linux/io.h>
>  #include <linux/slab.h>
>  #include <linux/sched_clock.h>
> +#include <linux/acpi.h>
>  
>  #include <asm/arch_timer.h>
>  #include <asm/virt.h>
> @@ -338,8 +339,12 @@ arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
>  	if (arch_timer_rate)
>  		return;
>  
> -	/* Try to determine the frequency from the device tree or CNTFRQ */
> -	if (of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
> +	/*
> +	 * Try to determine the frequency from the device tree or CNTFRQ,
> +	 * if ACPI is enabled, get the frequency from CNTFRQ ONLY.
> +	 */

If ACPI is enabled, the tree won't have been unflattened and there wouldn't be
a property to match. So I don't think the extra check is needed here.

If anything, add a check to make sure "np" is non-null instead.

> +	if (!acpi_disabled ||
> +	    of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
>  		if (cntbase)
>  			arch_timer_rate = readl_relaxed(cntbase + CNTFRQ);
>  		else


-Olof

[PATCH v4 16/18] ARM64 / ACPI: Select ACPI_REDUCED_HARDWARE_ONLY if ACPI is enabled on ARM64

[Hanjun Guo]

From: Al Stone <al.stone@linaro.org>

ACPI reduced hardware mode is disabled by default, but ARM64
can only run properly in ACPI hardware reduced mode, so select
ACPI_REDUCED_HARDWARE_ONLY if ACPI is enabled on ARM64.

Reviewed-by: Grant Likely <grant.likely@linaro.org>
Signed-off-by: Al Stone <al.stone@linaro.org>
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 arch/arm64/Kconfig |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index fd4e81a..065438c 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1,5 +1,6 @@
 config ARM64
 	def_bool y
+	select ACPI_REDUCED_HARDWARE_ONLY if ACPI
 	select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
 	select ARCH_HAS_SG_CHAIN
 	select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
-- 
1.7.9.5

[PATCH v4 17/18] ARM64 / ACPI: Enable ARM64 in Kconfig

[Hanjun Guo]

From: Graeme Gregory <graeme.gregory@linaro.org>

Add Kconfigs to build ACPI on ARM64, and make ACPI available on ARM64.

acpi_idle driver is x86/IA64 dependent now, so make CONFIG_ACPI_PROCESSOR
depend on X86 || IA64, and implement it on ARM64 in the future.

Reviewed-by: Grant Likely <grant.likely@linaro.org>
Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
Signed-off-by: Al Stone <al.stone@linaro.org>
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 arch/arm64/Kconfig   |    2 ++
 drivers/acpi/Kconfig |    6 +++---
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 065438c..8dabaeb 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -422,6 +422,8 @@ source "drivers/Kconfig"
 
 source "drivers/firmware/Kconfig"
 
+source "drivers/acpi/Kconfig"
+
 source "fs/Kconfig"
 
 source "arch/arm64/kvm/Kconfig"
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index d0f3265..f935d2e 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -5,8 +5,7 @@
 menuconfig ACPI
 	bool "ACPI (Advanced Configuration and Power Interface) Support"
 	depends on !IA64_HP_SIM
-	depends on IA64 || X86
-	depends on PCI
+	depends on ((IA64 || X86) && PCI) || (ARM64 && EXPERT)
 	select PNP
 	default y
 	help
@@ -163,6 +162,7 @@ config ACPI_PROCESSOR
 	tristate "Processor"
 	select THERMAL
 	select CPU_IDLE
+	depends on X86 || IA64
 	default y
 	help
 	  This driver installs ACPI as the idle handler for Linux and uses
@@ -263,7 +263,7 @@ config ACPI_DEBUG
 
 config ACPI_PCI_SLOT
 	bool "PCI slot detection driver"
-	depends on SYSFS
+	depends on SYSFS && PCI
 	default n
 	help
 	  This driver creates entries in /sys/bus/pci/slots/ for all PCI
-- 
1.7.9.5

[PATCH v4 18/18] Documentation: ACPI for ARM64

[Hanjun Guo]

From: Graeme Gregory <graeme.gregory@linaro.org>

Add documentation for the guidelines of how to use ACPI
on ARM64.

Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 Documentation/arm64/arm-acpi.txt |  218 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 218 insertions(+)
 create mode 100644 Documentation/arm64/arm-acpi.txt

diff --git a/Documentation/arm64/arm-acpi.txt b/Documentation/arm64/arm-acpi.txt
new file mode 100644
index 0000000..704a9e0
--- /dev/null
+++ b/Documentation/arm64/arm-acpi.txt
@@ -0,0 +1,218 @@
+ACPI on ARMv8 Servers
+---------------------
+
+ACPI can be used for ARMv8 general purpose servers designed to follow
+the SBSA specification (currently available to people with an ARM login at
+http://silver.arm.com).
+
+The kernel will implement minimum ACPI version is 5.1 + errata as released by
+the UEFI Forum, which is available at <http://www.uefi.org/acpi/specs>.
+
+If the machine does not meet the requirements of the SBSA, or cannot be
+described in the required ACPI specifications then it is likely that Device Tree
+(DT) is more suitable for the hardware.
+
+Relationship with Device Tree
+-----------------------------
+
+ACPI support in drivers and subsystems for ARMv8 should never be mutually
+exclusive with DT support at compile time.
+
+At boot time the kernel will only use one description method depending on
+parameters passed from the bootloader (including kernel bootargs).
+
+Regardless of whether DT or ACPI is used, the kernel must always be capable
+of booting with either scheme (in kernels with both schemes enabled at compile
+time).
+
+When booting using ACPI tables the /chosen node in DT will still be parsed
+to extract the kernel command line and initrd path. No other section of
+the DT will be used.
+
+Booting using ACPI tables
+-------------------------
+
+Currently, the only defined method to pass ACPI tables to the kernel on ARMv8
+is via the UEFI system configuration table.
+
+The UEFI implementation MUST set the ACPI_20_TABLE_GUID to point to the
+RSDP table (the table with the ACPI signature "RSD PTR ").
+
+The pointer to the RSDP table will be retrieved from EFI by the ACPI core.
+
+Processing of ACPI tables may be disabled by passing acpi=off on the kernel
+command line.
+
+DO use an XSDT; RSDTs are deprecated and should not be used on arm64. They
+only allow for 32-bit addresses.
+
+DO NOT use the 32-bit address fields in the FADT; they are deprecated. The
+64-bit alternatives MUST be used.
+
+The minimum set of tables MUST include RSDP, XSDT, FACS, FADT, DSDT, MADT
+and GTDT. If PCI is used the MCFG table MUST also be present.
+
+ACPI Detection
+--------------
+
+Drivers should determine their probe() type by checking for ACPI_HANDLE,
+or .of_node, or other information in the device structure. This is
+detailed further in the "Driver Recommendations" section.
+
+In non-driver code If the presence of ACPI needs to be detected at runtime,
+then check the value of acpi_disabled. If CONFIG_ACPI is not set,
+acpi_disabled will always be 1.
+
+Device Enumeration
+------------------
+
+Device descriptions in ACPI should use standard recognized ACPI interfaces.
+These are far simpler than the information provided via Device Tree. Drivers
+should take into account this simplicity and work with sensible defaults.
+
+On no account should a Device Tree attempt to be replicated in ASL using such
+constructs as Name(KEY0, "Value1") type constructs. Additional driver specific
+data should be represented with the appropriate _DSD (ACPI Section 6.2.5)
+structure. _DSM (ACPI Section 9.14.1) should only be used if _DSD cannot
+represent the data required.
+
+This data should be rare and not OS specific. For x86 ACPI has taken to
+identifying itself as Windows because it was found that only one path was
+routinely tested. For ARMv8 it would be preferable to have only one well
+tested path.
+
+_DSD covers more than the generic server case and care should be taken not to
+replicate highly specific embedded behaviour from DT into generic servers.
+
+Common _DSD bindings should be submitted to ASWG to be included in the
+document :-
+
+http://www.uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel.htm
+
+If these bindings are mirrored from DT care should be taken to ensure they are
+reviewed as DT bindings before submission to limit divergance in bindings.
+
+Programmable Power Control Resources
+------------------------------------
+
+Programmable power control resources include such resources as voltage/current
+providers (regulators) and clock sources.
+
+For power control of these resources they should be represented with Power
+Resource Objects (ACPI Section 7.1). The ACPI core will then handle correctly
+enabling/disabling of resources as they are needed.
+
+The ACPI 5.1 specification does not contain any standard binding for these
+objects to enable programmable levels or rates so this should be avoided if
+possible and the resources set to appropriate levels by the firmware. If this is
+not possible then any manipulation should be abstracted in ASL.
+
+Each device in ACPI has D-states and these can be controlled through
+the optional methods _PS0..._PS3 where _PS0 is full on and _PS3 is full off.
+
+If either _PS0 or _PS3 is implemented, then the other method must also be
+implemented.
+
+If a device requires usage or setup of a power resource when on, the ASL
+should organize that it is allocated/enabled using the _PS0 method.
+
+Resources allocated/enabled in the _PS0 method should be disabled/de-allocated
+in the _PS3 method.
+
+Such code in _PS? methods will of course be very platform specific but
+should allow the driver to operate the device without special non-standard
+values being read from ASL. Further, abstracting the use of these resources
+allows hardware revisions without requiring updates to the kernel.
+
+Clocks
+------
+
+Like clocks that are part of the power resources there is no standard way
+to represent a clock tree in ACPI 5.1 in a similar manner to how it is
+described in DT.
+
+Devices affected by this include things like UARTs, SoC driven LCD displays,
+etc.
+
+The firmware (for example, UEFI) should initialize these clocks to fixed working
+values before the kernel is executed.
+
+Driver Recommendations
+----------------------
+
+DO NOT remove any FDT handling when adding ACPI support for a driver. Different
+systems may use the same device.
+
+DO try and keep complex sections of ACPI and DT functionality separate. This
+may mean a patch to break out some complex DT to another function before
+the patch to add ACPI. This may happen in other functions but is most likely
+in probe function. This gives a clearer flow of data for reviewing driver
+source.
+
+probe() :-
+
+static int device_probe_dt(struct platform_device *pdev)
+{
+	/* DT specific functionality */
+	...
+}
+
+static int device_probe_acpi(struct platform_device *pdev)
+{
+	/* ACPI specific functionality */
+	...
+}
+
+static int device_probe(stuct platform_device *pdev)
+{
+	...
+	struct device_node node = pdev->dev.of_node;
+	...
+
+	if (node)
+		ret = device_probe_dt(pdev);
+	else if (ACPI_HANDLE(&pdev->dev))
+		ret = device_probe_acpi(pdev);
+	else
+		/* other initialization */
+		...
+	/* Continue with any generic probe operations */
+	...
+}
+
+DO keep the MODULE_DEVICE_TABLE entries together in the driver to make it clear
+the different names the driver is probed for, both from DT and from ACPI.
+
+module device tables :-
+
+static struct of_device_id virtio_mmio_match[] = {
+        { .compatible = "virtio,mmio", },
+        { }
+};
+MODULE_DEVICE_TABLE(of, virtio_mmio_match);
+
+static const struct acpi_device_id virtio_mmio_acpi_match[] = {
+        { "LNRO0005", },
+        { }
+};
+MODULE_DEVICE_TABLE(acpi, virtio_mmio_acpi_match);
+
+ASWG
+----
+
+The following areas are not yet well defined for ARM in the current ACPI
+specification and are expected to be worked through in the UEFI ACPI
+Specification Working Group (ASWG) <http://www.uefi.org/workinggroups>.
+Participation in this group is open to all UEFI members.
+
+	- ACPI based CPU topology
+	- ACPI based Power management
+	- CPU idle control based on PSCI
+	- CPU performance control (CPPC)
+	- ACPI based SMMU
+	- ITS support for GIC in MADT
+
+No code shall be accepted into the kernel unless it complies with the released
+standards from UEFI ASWG. If there are features missing from ACPI to make it
+function on a platform, ECRs should be submitted to ASWG and go through the
+approval process.
-- 
1.7.9.5

Re: [PATCH v4 18/18] Documentation: ACPI for ARM64

[Olof Johansson]

On Fri, Sep 12, 2014 at 10:00:16PM +0800, Hanjun Guo wrote:
> From: Graeme Gregory <graeme.gregory@linaro.org>
> 
> Add documentation for the guidelines of how to use ACPI
> on ARM64.
> 
> Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> ---
>  Documentation/arm64/arm-acpi.txt |  218 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 218 insertions(+)
>  create mode 100644 Documentation/arm64/arm-acpi.txt
> 
> diff --git a/Documentation/arm64/arm-acpi.txt b/Documentation/arm64/arm-acpi.txt
> new file mode 100644
> index 0000000..704a9e0
> --- /dev/null
> +++ b/Documentation/arm64/arm-acpi.txt
> @@ -0,0 +1,218 @@
> +ACPI on ARMv8 Servers
> +---------------------
> +
> +ACPI can be used for ARMv8 general purpose servers designed to follow
> +the SBSA specification (currently available to people with an ARM login at
> +http://silver.arm.com).
> +
> +The kernel will implement minimum ACPI version is 5.1 + errata as released by
> +the UEFI Forum, which is available at <http://www.uefi.org/acpi/specs>.

"implements", not "will implement" (assuming the rest of the patch set
is merged at the same time as this patch).

> +If the machine does not meet the requirements of the SBSA, or cannot be
> +described in the required ACPI specifications then it is likely that Device Tree
> +(DT) is more suitable for the hardware.
> +
> +Relationship with Device Tree
> +-----------------------------
> +
> +ACPI support in drivers and subsystems for ARMv8 should never be mutually
> +exclusive with DT support at compile time.
> +
> +At boot time the kernel will only use one description method depending on
> +parameters passed from the bootloader (including kernel bootargs).
> +
> +Regardless of whether DT or ACPI is used, the kernel must always be capable
> +of booting with either scheme (in kernels with both schemes enabled at compile
> +time).
> +
> +When booting using ACPI tables the /chosen node in DT will still be parsed
> +to extract the kernel command line and initrd path. No other section of
> +the DT will be used.
> +
> +Booting using ACPI tables
> +-------------------------
> +
> +Currently, the only defined method to pass ACPI tables to the kernel on ARMv8
> +is via the UEFI system configuration table.
> +
> +The UEFI implementation MUST set the ACPI_20_TABLE_GUID to point to the
> +RSDP table (the table with the ACPI signature "RSD PTR ").
> +
> +The pointer to the RSDP table will be retrieved from EFI by the ACPI core.
> +
> +Processing of ACPI tables may be disabled by passing acpi=off on the kernel
> +command line.
> +
> +DO use an XSDT; RSDTs are deprecated and should not be used on arm64. They
> +only allow for 32-bit addresses.
> +
> +DO NOT use the 32-bit address fields in the FADT; they are deprecated. The
> +64-bit alternatives MUST be used.
> +
> +The minimum set of tables MUST include RSDP, XSDT, FACS, FADT, DSDT, MADT
> +and GTDT. If PCI is used the MCFG table MUST also be present.

This wording (DO NOT, MUST, etc) doesn't make sense in this doc. You're
not instructing a firmware vendor how to implement their firmware here,
you're documenting what the kernel expects from it.

> +
> +ACPI Detection
> +--------------
> +
> +Drivers should determine their probe() type by checking for ACPI_HANDLE,
> +or .of_node, or other information in the device structure. This is
> +detailed further in the "Driver Recommendations" section.
> +
> +In non-driver code If the presence of ACPI needs to be detected at runtime,

"code If" looks odd here. 

> +then check the value of acpi_disabled. If CONFIG_ACPI is not set,
> +acpi_disabled will always be 1.
> +
> +Device Enumeration
> +------------------
> +
> +Device descriptions in ACPI should use standard recognized ACPI interfaces.
> +These are far simpler than the information provided via Device Tree. Drivers
> +should take into account this simplicity and work with sensible defaults.

s/simpler/contains less information/

This is a key point in this doc (use sensible defaults), and I expect it's
one of the major ones that people will get wrong. A bit of elaboration
would be good.

> +
> +On no account should a Device Tree attempt to be replicated in ASL using such
> +constructs as Name(KEY0, "Value1") type constructs. Additional driver specific
> +data should be represented with the appropriate _DSD (ACPI Section 6.2.5)
> +structure. _DSM (ACPI Section 9.14.1) should only be used if _DSD cannot
> +represent the data required.
> +
> +This data should be rare and not OS specific. For x86 ACPI has taken to
> +identifying itself as Windows because it was found that only one path was
> +routinely tested. For ARMv8 it would be preferable to have only one well
> +tested path.
> +
> +_DSD covers more than the generic server case and care should be taken not to
> +replicate highly specific embedded behaviour from DT into generic servers.
> +
> +Common _DSD bindings should be submitted to ASWG to be included in the
> +document :-
> +
> +http://www.uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel.htm
> +
> +If these bindings are mirrored from DT care should be taken to ensure they are
> +reviewed as DT bindings before submission to limit divergance in bindings.

Some of the above is again not suitable language for this document. And
again, this is likely the wrong place to educate system firware
developers on how to describe their hardware.

> +
> +Programmable Power Control Resources
> +------------------------------------
> +
> +Programmable power control resources include such resources as voltage/current
> +providers (regulators) and clock sources.
> +
> +For power control of these resources they should be represented with Power
> +Resource Objects (ACPI Section 7.1). The ACPI core will then handle correctly
> +enabling/disabling of resources as they are needed.
> +
> +The ACPI 5.1 specification does not contain any standard binding for these
> +objects to enable programmable levels or rates so this should be avoided if
> +possible and the resources set to appropriate levels by the firmware. If this is
> +not possible then any manipulation should be abstracted in ASL.
> +
> +Each device in ACPI has D-states and these can be controlled through
> +the optional methods _PS0..._PS3 where _PS0 is full on and _PS3 is full off.
> +
> +If either _PS0 or _PS3 is implemented, then the other method must also be
> +implemented.
> +
> +If a device requires usage or setup of a power resource when on, the ASL
> +should organize that it is allocated/enabled using the _PS0 method.

Again, some of this seems like the wrong place to mandate how firmware works.

If anything, it can be rephrased as "the kernel assumes that ASL will
handle power resource management/enablement through the _PS0 method", etc.

> +Resources allocated/enabled in the _PS0 method should be disabled/de-allocated
> +in the _PS3 method.
> +
> +Such code in _PS? methods will of course be very platform specific but
> +should allow the driver to operate the device without special non-standard
> +values being read from ASL. Further, abstracting the use of these resources
> +allows hardware revisions without requiring updates to the kernel.
> +
> +Clocks
> +------
> +
> +Like clocks that are part of the power resources there is no standard way
> +to represent a clock tree in ACPI 5.1 in a similar manner to how it is
> +described in DT.
> +
> +Devices affected by this include things like UARTs, SoC driven LCD displays,
> +etc.

The devices aren't affected by the lack of clock tree representation, but they are affected
by a possibly variable input clock to their blocks. That's quite a big difference.

> +The firmware (for example, UEFI) should initialize these clocks to fixed working
> +values before the kernel is executed.

Add:

", such that the driver can be hardcoded to assume those frequencies". :-(


> +
> +Driver Recommendations
> +----------------------
> +
> +DO NOT remove any FDT handling when adding ACPI support for a driver. Different
> +systems may use the same device.

s/FDT/DT/

> +
> +DO try and keep complex sections of ACPI and DT functionality separate. This
> +may mean a patch to break out some complex DT to another function before
> +the patch to add ACPI. This may happen in other functions but is most likely
> +in probe function. This gives a clearer flow of data for reviewing driver
> +source.

The above can be read as we're preferring to split the driver in two with
as much code path divergence as possible. Instead, splitting probing
in two but making the rest of the driver mostly execute off of data
(i.e. setup internal per-device state in a struct based on defaults +
what you discover at probe) is a much better approach, and it's what
we have been pushing for in the platform code as well.

> +ASWG
> +----
> +
> +The following areas are not yet well defined for ARM in the current ACPI
> +specification and are expected to be worked through in the UEFI ACPI

Since docs go stale all the time, please refer to 5.1 instead of "current" here.

> +Specification Working Group (ASWG) <http://www.uefi.org/workinggroups>.
> +Participation in this group is open to all UEFI members.
> +
> +	- ACPI based CPU topology
> +	- ACPI based Power management
> +	- CPU idle control based on PSCI
> +	- CPU performance control (CPPC)
> +	- ACPI based SMMU
> +	- ITS support for GIC in MADT
> +
> +No code shall be accepted into the kernel unless it complies with the released
> +standards from UEFI ASWG. If there are features missing from ACPI to make it
> +function on a platform, ECRs should be submitted to ASWG and go through the
> +approval process.

Technically this is untrue and should at least be rephrased. We're likely
to at some point need to deal with bad tables that need quirks and fixups,
just like we've had to do on other systems.


-Olof

Re: [PATCH v4 18/18] Documentation: ACPI for ARM64

[Matthew Garrett]

On Fri, Sep 12, 2014 at 10:00:16PM +0800, Hanjun Guo wrote:

> +Common _DSD bindings should be submitted to ASWG to be included in the
> +document :-
> +
> +http://www.uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel.htm

How are individuals/companies who aren't UEFI members supposed to do 
this? Aren't there IP issues involved in taking submissions from 
non-members?

> +No code shall be accepted into the kernel unless it complies with the released
> +standards from UEFI ASWG. If there are features missing from ACPI to make it
> +function on a platform, ECRs should be submitted to ASWG and go through the
> +approval process.

Similar question here. Is the expectation that all ARM vendors will 
become UEFI contributors?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH v4 18/18] Documentation: ACPI for ARM64

[Matthew Garrett]

On Wed, Sep 17, 2014 at 02:44:10AM +0100, Matthew Garrett wrote:
> On Fri, Sep 12, 2014 at 10:00:16PM +0800, Hanjun Guo wrote:
> > +No code shall be accepted into the kernel unless it complies with the released
> > +standards from UEFI ASWG. If there are features missing from ACPI to make it
> > +function on a platform, ECRs should be submitted to ASWG and go through the
> > +approval process.
> 
> Similar question here. Is the expectation that all ARM vendors will 
> become UEFI contributors?

I should also add that this is inconsistent with how we've managed 
things in the x86 world - there's a great deal of ACPI functionality 
implemented that's not covered in the spec.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH v4 18/18] Documentation: ACPI for ARM64

[Jon Masters]

On 09/16/2014 09:44 PM, Matthew Garrett wrote:
> On Fri, Sep 12, 2014 at 10:00:16PM +0800, Hanjun Guo wrote:
> 
>> +Common _DSD bindings should be submitted to ASWG to be included in the
>> +document :-
>> +
>> +http://www.uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel.htm
> 
> How are individuals/companies who aren't UEFI members supposed to do 
> this? Aren't there IP issues involved in taking submissions from 
> non-members?

Obviously, I'm not a lawyer, but I would note that the _DSD bindings
aren't going into the core ACPI document. I think the specifics for non
members of the UEFI to potentially contribute bindings need to be
addressed. That said, at this point, the population of the UEFI forum is
very well aligned with initial players (that this is the case is not
entirely an accident, I went around to a few folks a year or so ago and
made sure that they had become members in anticipation of this issue).

Jon.

Re: [PATCH v4 18/18] Documentation: ACPI for ARM64

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1187 bytes --]

On Wed, Sep 17, 2014 at 04:58:10AM -0400, Jon Masters wrote:
> On 09/16/2014 09:44 PM, Matthew Garrett wrote:

> >> +http://www.uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel.htm

> > How are individuals/companies who aren't UEFI members supposed to do 
> > this? Aren't there IP issues involved in taking submissions from 
> > non-members?

> Obviously, I'm not a lawyer, but I would note that the _DSD bindings
> aren't going into the core ACPI document. I think the specifics for non
> members of the UEFI to potentially contribute bindings need to be
> addressed. That said, at this point, the population of the UEFI forum is
> very well aligned with initial players (that this is the case is not
> entirely an accident, I went around to a few folks a year or so ago and
> made sure that they had become members in anticipation of this issue).

Are you sure you've covered all the vendors of off-SoC components who
might be involved?  That's certainly already an issue with people doing
x86 machines (who aren't being bound by this spec but as a subsystem
maintainer how am I supposed to tell if the target is an ARM or x86
system?).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v4 18/18] Documentation: ACPI for ARM64

[Graeme Gregory]

On Wed, Sep 17, 2014 at 02:44:10AM +0100, Matthew Garrett wrote:
> On Fri, Sep 12, 2014 at 10:00:16PM +0800, Hanjun Guo wrote:
> 
> > +Common _DSD bindings should be submitted to ASWG to be included in the
> > +document :-
> > +
> > +http://www.uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel.htm
> 
> How are individuals/companies who aren't UEFI members supposed to do 
> this? Aren't there IP issues involved in taking submissions from 
> non-members?
> 

This section needs to be changed anyway, it comes out of a
misunderstanding I had of the _DSD spec.

It sounds like from the discussions in other threads that ARM64 should
be following x86 and re-using DT bindings here. In which case there is
not need to submit things to UEFI organisation.

What I got a little lost in has there been a formal decision about DT
bindings in _DSD?

Graeme

Re: [PATCH v4 18/18] Documentation: ACPI for ARM64

[Arnd Bergmann]

On Wednesday 17 September 2014, Graeme Gregory wrote:
> It sounds like from the discussions in other threads that ARM64 should
> be following x86 and re-using DT bindings here. In which case there is
> not need to submit things to UEFI organisation.
> 
> What I got a little lost in has there been a formal decision about DT
> bindings in _DSD?

I think this is a discussion that still needs to happen: either we should
recommend everyone to use _DSD in favor of the alternatives, or we
should prohibit the use of _DSD. I have heard arguments both ways, but
hopefully we can find an easy answer.

	Arnd

Re: [PATCH v4 18/18] Documentation: ACPI for ARM64

[Graeme Gregory]

On Thu, Sep 18, 2014 at 01:22:10AM +0200, Arnd Bergmann wrote:
> On Wednesday 17 September 2014, Graeme Gregory wrote:
> > It sounds like from the discussions in other threads that ARM64 should
> > be following x86 and re-using DT bindings here. In which case there is
> > not need to submit things to UEFI organisation.
> > 
> > What I got a little lost in has there been a formal decision about DT
> > bindings in _DSD?
> 
> I think this is a discussion that still needs to happen: either we should
> recommend everyone to use _DSD in favor of the alternatives, or we
> should prohibit the use of _DSD. I have heard arguments both ways, but
> hopefully we can find an easy answer.
> 

This discussion is just not going to happen until people at @redhat.com
and people who have currently announced/released hardware are actually
willing to start talking about it.

Id love to be able to put my foot down and ban the use of _DSD for
servers but I suspect that will not happen.

Graeme

Re: [PATCH v4 18/18] Documentation: ACPI for ARM64

[Catalin Marinas]

On Thu, Sep 18, 2014 at 12:40:36AM +0100, Graeme Gregory wrote:
> On Thu, Sep 18, 2014 at 01:22:10AM +0200, Arnd Bergmann wrote:
> > On Wednesday 17 September 2014, Graeme Gregory wrote:
> > > It sounds like from the discussions in other threads that ARM64 should
> > > be following x86 and re-using DT bindings here. In which case there is
> > > not need to submit things to UEFI organisation.
> > > 
> > > What I got a little lost in has there been a formal decision about DT
> > > bindings in _DSD?
> > 
> > I think this is a discussion that still needs to happen: either we should
> > recommend everyone to use _DSD in favor of the alternatives, or we
> > should prohibit the use of _DSD. I have heard arguments both ways, but
> > hopefully we can find an easy answer.
> 
> This discussion is just not going to happen until people at @redhat.com
> and people who have currently announced/released hardware are actually
> willing to start talking about it.

OTOH, there doesn't seem to be any urgency for merging arm64 ACPI
support, so this discussion can wait ;).

> Id love to be able to put my foot down and ban the use of _DSD for
> servers but I suspect that will not happen.

And the alternative would be hard-coded platform specific configuration
(or we assume that such configuration isn't needed)?

-- 
Catalin

Re: [PATCH v4 18/18] Documentation: ACPI for ARM64

[Rafael J. Wysocki]

On Wednesday, September 17, 2014 04:40:36 PM Graeme Gregory wrote:
> On Thu, Sep 18, 2014 at 01:22:10AM +0200, Arnd Bergmann wrote:
> > On Wednesday 17 September 2014, Graeme Gregory wrote:
> > > It sounds like from the discussions in other threads that ARM64 should
> > > be following x86 and re-using DT bindings here. In which case there is
> > > not need to submit things to UEFI organisation.
> > > 
> > > What I got a little lost in has there been a formal decision about DT
> > > bindings in _DSD?
> > 
> > I think this is a discussion that still needs to happen: either we should
> > recommend everyone to use _DSD in favor of the alternatives, or we
> > should prohibit the use of _DSD. I have heard arguments both ways, but
> > hopefully we can find an easy answer.
> > 
> 
> This discussion is just not going to happen until people at @redhat.com
> and people who have currently announced/released hardware are actually
> willing to start talking about it.
> 
> Id love to be able to put my foot down and ban the use of _DSD for
> servers but I suspect that will not happen.

I'll probably should stay away from this discussion, but I can't resist. :-)

Please imagine the situation in which the same IP block is included in an ARM64
SoC and in an x86 SoC that ships with ACPI tables and a _DSD for that device in
them.  What benefit would be there from disallowing systems based on the ARM64
SoC in question to ship the same _DSD in their ACPI tables?

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v4 18/18] Documentation: ACPI for ARM64

[Jon Masters]

On 09/18/2014 07:20 PM, Rafael J. Wysocki wrote:
> On Wednesday, September 17, 2014 04:40:36 PM Graeme Gregory wrote:
>> On Thu, Sep 18, 2014 at 01:22:10AM +0200, Arnd Bergmann wrote:
>>> On Wednesday 17 September 2014, Graeme Gregory wrote:
>>>> It sounds like from the discussions in other threads that ARM64 should
>>>> be following x86 and re-using DT bindings here. In which case there is
>>>> not need to submit things to UEFI organisation.
>>>>
>>>> What I got a little lost in has there been a formal decision about DT
>>>> bindings in _DSD?
>>>
>>> I think this is a discussion that still needs to happen: either we should
>>> recommend everyone to use _DSD in favor of the alternatives, or we
>>> should prohibit the use of _DSD. I have heard arguments both ways, but
>>> hopefully we can find an easy answer.
>>>
>>
>> This discussion is just not going to happen until people at @redhat.com
>> and people who have currently announced/released hardware are actually
>> willing to start talking about it.
>>
>> Id love to be able to put my foot down and ban the use of _DSD for
>> servers but I suspect that will not happen.
> 
> I'll probably should stay away from this discussion, but I can't resist. :-)
> 
> Please imagine the situation in which the same IP block is included in an ARM64
> SoC and in an x86 SoC that ships with ACPI tables and a _DSD for that device in
> them.  What benefit would be there from disallowing systems based on the ARM64
> SoC in question to ship the same _DSD in their ACPI tables?

"Disallowing" is a strong word in any case, because vendors own the
platform and will ship _DSD properties to describe those devices. So the
only "disallowing" Linux can do is to ignore entities present in ACPI
tables that have already been shipped by vendors.

Anyway. I think we all don't want a runaway frenzy with _DSD key/values
(generally there ought to be as few as possible, and additions should
only happen carefully). Broadly, there are three levels I see here:

0). Devices that are part of the core ACPI specification. None today
need key/value pairs, and I want to avoid this from growing.
1). Devices containing _DSD key/value pairs for a specific device but of
a common industry type, such as a network device. In this case, the 4-6
properties that might need to be specified (MAC address, PHY address,
PHY type, etc.) should be as minimal as possible and then standardized
into a common binding that vendors agree to support, and which is owned
and controlled by a neutral group such as ASWG.
2). Devices containining a specific ACPI ID that is unique to a given
vendor where that device implements value-add/offload/something non core
that can be owned entirely within a driver for one device. In that case,
maybe a vendor would define a minimal set of _DSD key/values and be on
the hook to maintain compatibility themselves.

I've chatted with a few people, and there will be a nice proposal
presented to ASWG/UEFI on how to provide an official process for
defining key/value pairs that are shared between common device types and
managed by such a forum, as in the cases 0 and 1 above. In the
meanwhile, there is only one _DSD use case in the early ACPI patches for
ARM servers (in the network MAC, to pass in the mac address and a couple
of PHY address/ID bits) and I've connected the vendors together asking
them to come up with the initial first example covering that.

None of this is core to ACPI enablement. It's specific to a few drivers
where also on x86 there will be _DSD properties.

Jon.

Re: [PATCH v4 18/18] Documentation: ACPI for ARM64

[Rafael J. Wysocki]

On Wednesday, September 17, 2014 02:44:10 AM Matthew Garrett wrote:
> On Fri, Sep 12, 2014 at 10:00:16PM +0800, Hanjun Guo wrote:
> 
> > +Common _DSD bindings should be submitted to ASWG to be included in the
> > +document :-
> > +
> > +http://www.uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel.htm

Well, the intention here is different.

The linked document is supposed to define the format of data to be returned
by _DSD and the one at:

http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf

does it specifically for the UUID associated with the DT bindings format.

There are no implied IP issues with using the information there I know of and
if there's any fine print anywhere that may suggest so, please let me know.

That said, those documents are not intended to be libraries of actual bindings.

A common place for the binding and a process for submitting/accepting them is
needed, but we are yet to agree on how to do that.

Rafael

Re: [PATCH v4 18/18] Documentation: ACPI for ARM64

[Matthew Garrett]

On Wed, Sep 17, 2014 at 09:37:42PM +0200, Rafael J. Wysocki wrote:

> There are no implied IP issues with using the information there I know of and
> if there's any fine print anywhere that may suggest so, please let me know.

Using the information should be fine, but my understanding of the UEFI 
forum rules is that any submissions to UEFI specs must be from UEFI 
forum members - there are concerns around accidentally including 
patented material. The easy way around this is just for the bindings to 
be managed outside UEFI.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH v4 18/18] Documentation: ACPI for ARM64

[Jon Masters]

On 09/17/2014 03:22 PM, Matthew Garrett wrote:
> On Wed, Sep 17, 2014 at 09:37:42PM +0200, Rafael J. Wysocki wrote:
> 
>> There are no implied IP issues with using the information there I know of and
>> if there's any fine print anywhere that may suggest so, please let me know.
> 
> Using the information should be fine, but my understanding of the UEFI 
> forum rules is that any submissions to UEFI specs must be from UEFI 
> forum members - there are concerns around accidentally including 
> patented material. The easy way around this is just for the bindings to 
> be managed outside UEFI.

I've raised this with Dong in person this morning and I'll ask him to
followup on UEFI policy.

Jon.

Re: [PATCH v4 18/18] Documentation: ACPI for ARM64

[Rafael J. Wysocki]

On Wednesday, September 17, 2014 08:22:59 PM Matthew Garrett wrote:
> On Wed, Sep 17, 2014 at 09:37:42PM +0200, Rafael J. Wysocki wrote:
> 
> > There are no implied IP issues with using the information there I know of and
> > if there's any fine print anywhere that may suggest so, please let me know.
> 
> Using the information should be fine, but my understanding of the UEFI 
> forum rules is that any submissions to UEFI specs must be from UEFI 
> forum members - there are concerns around accidentally including 
> patented material.

The documents in question are not regarded as UEFI specs, however.  They
are just hosted by UEFI.

> The easy way around this is just for the bindings to 
> be managed outside UEFI.

Again, there's a difference between UEFI material and the UEFI hosting
something (but not maintaining it).

In principle, the bindings could be hosted by UEFI, but maintained by
community members.

Rafael

Re: [PATCH v4 18/18] Documentation: ACPI for ARM64

[Matthew Garrett]

On Wed, Sep 17, 2014 at 10:11:13PM +0200, Rafael J. Wysocki wrote:
> On Wednesday, September 17, 2014 08:22:59 PM Matthew Garrett wrote:
> > Using the information should be fine, but my understanding of the UEFI 
> > forum rules is that any submissions to UEFI specs must be from UEFI 
> > forum members - there are concerns around accidentally including 
> > patented material.
> 
> The documents in question are not regarded as UEFI specs, however.  They
> are just hosted by UEFI.

There's arguably some precedent for that - the ACPI vendor IDs and EFI 
System Partition name repositories are hosted rather than specs. But it 
seems like there's some qualitative difference between that kind of raw 
data and something that actually defines behaviour. But if the UEFI 
people feel comfortable with this, it's not a problem.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH v4 18/18] Documentation: ACPI for ARM64

[Hanjun Guo]

On 2014年09月18日 04:11, Rafael J. Wysocki wrote:
> On Wednesday, September 17, 2014 08:22:59 PM Matthew Garrett wrote:
>> On Wed, Sep 17, 2014 at 09:37:42PM +0200, Rafael J. Wysocki wrote:
>>
>>> There are no implied IP issues with using the information there I know of and
>>> if there's any fine print anywhere that may suggest so, please let me know.
>> Using the information should be fine, but my understanding of the UEFI 
>> forum rules is that any submissions to UEFI specs must be from UEFI 
>> forum members - there are concerns around accidentally including 
>> patented material.
> The documents in question are not regarded as UEFI specs, however.  They
> are just hosted by UEFI.
>
>> The easy way around this is just for the bindings to 
>> be managed outside UEFI.
> Again, there's a difference between UEFI material and the UEFI hosting
> something (but not maintaining it).
>
> In principle, the bindings could be hosted by UEFI, but maintained by
> community members.

Thanks for the clarify, I totally agree with you, that's how things work now for such doc hosted
by UEFI.

For now,  _DSD will use the same binding as DT, is there any chance that _DSD will introduce
new bindings? if yes, how can we handle it?

Thanks
Hanjun

Re: [PATCH v4 18/18] Documentation: ACPI for ARM64

[Pavel Machek]

On Fri 2014-09-12 22:00:16, Hanjun Guo wrote:
> From: Graeme Gregory <graeme.gregory@linaro.org>
> 
> Add documentation for the guidelines of how to use ACPI
> on ARM64.

> +No code shall be accepted into the kernel unless it complies with the released
> +standards from UEFI ASWG. If there are features missing from ACPI to make it
> +function on a platform, ECRs should be submitted to ASWG and go through the
> +approval process.

Surely this should be narrowed down somehow? Or is reading all the
released standards from ASWG mandatory before patching the kernel now?

Spelling out wtf ECR is would be nice, too.

And if you expect all kernel contributors to comply with your
favourite standards, you should put an URL where the standards can be
downloaded here.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v4 18/18] Documentation: ACPI for ARM64

[Matthew Garrett]

On Mon, Sep 22, 2014 at 09:48:41PM +0200, Pavel Machek wrote:
> On Fri 2014-09-12 22:00:16, Hanjun Guo wrote:
> > +No code shall be accepted into the kernel unless it complies with the released
> > +standards from UEFI ASWG. If there are features missing from ACPI to make it
> > +function on a platform, ECRs should be submitted to ASWG and go through the
> > +approval process.
> 
> Surely this should be narrowed down somehow? Or is reading all the
> released standards from ASWG mandatory before patching the kernel now?
> 
> Spelling out wtf ECR is would be nice, too.

Explicit Change Request. These can only be filed by paid-up members of 
the UEFI Forum, so I suspect this requirement is going to be unworkable 
(there's plenty of ACPI support code for large x86 vendors which isn't 
part of any ACPI spec)

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH v4 18/18] Documentation: ACPI for ARM64

[Rafael J. Wysocki]

On Monday, September 22, 2014 09:31:36 PM Matthew Garrett wrote:
> On Mon, Sep 22, 2014 at 09:48:41PM +0200, Pavel Machek wrote:
> > On Fri 2014-09-12 22:00:16, Hanjun Guo wrote:
> > > +No code shall be accepted into the kernel unless it complies with the released
> > > +standards from UEFI ASWG. If there are features missing from ACPI to make it
> > > +function on a platform, ECRs should be submitted to ASWG and go through the
> > > +approval process.
> > 
> > Surely this should be narrowed down somehow? Or is reading all the
> > released standards from ASWG mandatory before patching the kernel now?
> > 
> > Spelling out wtf ECR is would be nice, too.
> 
> Explicit Change Request. These can only be filed by paid-up members of 
> the UEFI Forum, so I suspect this requirement is going to be unworkable 
> (there's plenty of ACPI support code for large x86 vendors which isn't 
> part of any ACPI spec).

Why do you think so?

Linux Foundation can do that on behalf of the community if no one else.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v4 18/18] Documentation: ACPI for ARM64

[Matthew Garrett]

On Tue, Sep 23, 2014 at 12:46:24AM +0200, Rafael J. Wysocki wrote:
> On Monday, September 22, 2014 09:31:36 PM Matthew Garrett wrote:
> > Explicit Change Request. These can only be filed by paid-up members of 
> > the UEFI Forum, so I suspect this requirement is going to be unworkable 
> > (there's plenty of ACPI support code for large x86 vendors which isn't 
> > part of any ACPI spec).
> 
> Why do you think so?

The IP rules in the membership agreements.

> Linux Foundation can do that on behalf of the community if no one else.

Unfortunately not - any work contributed must be the work of the 
contributing member. There can't be any public discussion.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH v4 18/18] Documentation: ACPI for ARM64

[Hanjun Guo]

On Sep 23, 2014, 06:28AM, Matthew Garrett wrote:
> On Tue, Sep 23, 2014 at 12:46:24AM +0200, Rafael J. Wysocki wrote:
>> On Monday, September 22, 2014 09:31:36 PM Matthew Garrett wrote:
>>> Explicit Change Request. These can only be filed by paid-up members of 
>>> the UEFI Forum, so I suspect this requirement is going to be unworkable 
>>> (there's plenty of ACPI support code for large x86 vendors which isn't 
>>> part of any ACPI spec).
>> Why do you think so?
> The IP rules in the membership agreements.

If I'm not mistaken, I think there is no IP issues for the _DSD bindings,
it just some key value pairs.

Thanks
Hanjun

Re: [PATCH v4 18/18] Documentation: ACPI for ARM64

[Matthew Garrett]

On Tue, Sep 23, 2014 at 06:34:42AM +0800, Hanjun Guo wrote:
> On Sep 23, 2014, 06:28AM, Matthew Garrett wrote:
> > On Tue, Sep 23, 2014 at 12:46:24AM +0200, Rafael J. Wysocki wrote:
> >> On Monday, September 22, 2014 09:31:36 PM Matthew Garrett wrote:
> >>> Explicit Change Request. These can only be filed by paid-up members of 
> >>> the UEFI Forum, so I suspect this requirement is going to be unworkable 
> >>> (there's plenty of ACPI support code for large x86 vendors which isn't 
> >>> part of any ACPI spec).
> >> Why do you think so?
> > The IP rules in the membership agreements.
> 
> If I'm not mistaken, I think there is no IP issues for the _DSD bindings,
> it just some key value pairs.

That paragraph is talking about ACPI support in general, not just the 
_DSD code.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH v4 18/18] Documentation: ACPI for ARM64

[Rafael J. Wysocki]

On Tuesday, September 23, 2014 06:34:42 AM Hanjun Guo wrote:
> On Sep 23, 2014, 06:28AM, Matthew Garrett wrote:
> > On Tue, Sep 23, 2014 at 12:46:24AM +0200, Rafael J. Wysocki wrote:
> >> On Monday, September 22, 2014 09:31:36 PM Matthew Garrett wrote:
> >>> Explicit Change Request. These can only be filed by paid-up members of 
> >>> the UEFI Forum, so I suspect this requirement is going to be unworkable 
> >>> (there's plenty of ACPI support code for large x86 vendors which isn't 
> >>> part of any ACPI spec).
> >> Why do you think so?
> > The IP rules in the membership agreements.
> 
> If I'm not mistaken, I think there is no IP issues for the _DSD bindings,
> it just some key value pairs.

Well, if we are talking about the bindings themselves only, then as I said
previously that would be a hosted document rather than a UEFI specification
anyway, so I don't see how the "IP rules in the membership agreements" would
apply to that in any case.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v4 18/18] Documentation: ACPI for ARM64

[Al Stone]

On 09/22/2014 04:46 PM, Rafael J. Wysocki wrote:
> On Monday, September 22, 2014 09:31:36 PM Matthew Garrett wrote:
>> On Mon, Sep 22, 2014 at 09:48:41PM +0200, Pavel Machek wrote:
>>> On Fri 2014-09-12 22:00:16, Hanjun Guo wrote:
>>>> +No code shall be accepted into the kernel unless it complies with the released
>>>> +standards from UEFI ASWG. If there are features missing from ACPI to make it
>>>> +function on a platform, ECRs should be submitted to ASWG and go through the
>>>> +approval process.
>>>
>>> Surely this should be narrowed down somehow? Or is reading all the
>>> released standards from ASWG mandatory before patching the kernel now?
>>>
>>> Spelling out wtf ECR is would be nice, too.
>>
>> Explicit Change Request.

Actually, it's "Engineering Change Request" -- a standard document
format used by several of the UEFI working groups, including the ASWG.

 These can only be filed by paid-up members of
>> the UEFI Forum, so I suspect this requirement is going to be unworkable 
>> (there's plenty of ACPI support code for large x86 vendors which isn't 
>> part of any ACPI spec).
> 
> Why do you think so?
> 
> Linux Foundation can do that on behalf of the community if no one else.
> 

Exactly so.  Or, collaborate with the hardware vendor, or a distro
or anyone else that is a Promoter or Contributor as defined by UEFI
[0].  The only thing to keep clear when doing so is who owns the
intellectual property for any proposed change; this is one of the
reasons the UEFI Forum has paid membership levels -- to pay for the
legal assistance to make sure that the specs can be freely used.  As
someone who is part of the ASWG, I'd personally be glad to help out
however I can in this regard.

I'm also curious as to what's being referred to as ACPI support
code for large x86 vendors which is not part of the spec; I *think*
I know what's being described but a specific example would really
help me understand better.

Thanks.



[0] http://www.uefi.org/join

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

Re: [PATCH v4 18/18] Documentation: ACPI for ARM64

[Matthew Garrett]

On Mon, Sep 22, 2014 at 04:55:48PM -0600, Al Stone wrote:

> Exactly so.  Or, collaborate with the hardware vendor, or a distro
> or anyone else that is a Promoter or Contributor as defined by UEFI
> [0].  The only thing to keep clear when doing so is who owns the
> intellectual property for any proposed change; this is one of the
> reasons the UEFI Forum has paid membership levels -- to pay for the
> legal assistance to make sure that the specs can be freely used.  As
> someone who is part of the ASWG, I'd personally be glad to help out
> however I can in this regard.

No, it's not about IP ownership, it's about whether those contributing 
the IP have waived patent rights. All contributors to a UEFI spec must 
be members - it's not acceptable for a member to contribute material on 
behalf of a non-member.

> I'm also curious as to what's being referred to as ACPI support
> code for large x86 vendors which is not part of the spec; I *think*
> I know what's being described but a specific example would really
> help me understand better.

Almost everything in drivers/platform/x86, the ACPI support code under 
drivers/gpu, the PCC code for HP servers, some of the USB-ACPI glue 
(defined by a Microsoft spec), some of the ACPI/TPM integration (defined 
by TCG), some hwmon code, probably a few other bits and pieces.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH v4 18/18] Documentation: ACPI for ARM64

[Al Stone]

On 09/22/2014 01:48 PM, Pavel Machek wrote:
> On Fri 2014-09-12 22:00:16, Hanjun Guo wrote:
>> From: Graeme Gregory <graeme.gregory@linaro.org>
>>
>> Add documentation for the guidelines of how to use ACPI
>> on ARM64.
> 
>> +No code shall be accepted into the kernel unless it complies with the released
>> +standards from UEFI ASWG. If there are features missing from ACPI to make it
>> +function on a platform, ECRs should be submitted to ASWG and go through the
>> +approval process.
> 
> Surely this should be narrowed down somehow? Or is reading all the
> released standards from ASWG mandatory before patching the kernel now?

Asking someone to read the ACPI standard when implementing ACPI
functionality is no different than doing the same thing for PCI,
SCSI, TCP/IP or any of a bazillion others that are used by the
kernel.  Yes, the wording of this sentence is awkward; that's a
very different problem and easily fixed.

> Spelling out wtf ECR is would be nice, too.

ECR stands for "Engineering Change Request".  It is simply a
recommended document format for requesting changes to be made to
the ACPI specification (and is also used by other UEFI working
groups).  The text can easily be fixed to make that clear.

> And if you expect all kernel contributors to comply with your
> favourite standards, you should put an URL where the standards can be
> downloaded here.
> 

Easily enough corrected; until the patch is updated, one can find
the specs at the oh so very cleverly named URL:

    http://www.uefi.org/specifications


-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------