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.

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

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

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

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

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

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

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

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

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

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;

+}

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?

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

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-...

+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