On Mon, Jul 28, 2014 at 09:23:40AM -0700, Olof Johansson wrote:
> On Mon, Jul 28, 2014 at 09:42:57AM +0100, Graeme Gregory wrote:

> > >>+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 passed in the appropriate _DSM (ACPI Section 9.14.1) method or
> > >>+_DSD (ACPI Section 6.2.5). This data should be rare and not OS specific.

...

> > >I see these two sentences as contradictory, given that the _DSD doc
> > >worst case turn into quite a mess.

> > >Given that ACPI can present completely different data based on what OS
> > >is running, it's quite common to indeed have OS specific data in
> > >there. How does that relate to this document and these practices?

> > OS specific data has traditionally not worked out well for ACPI, I
> > would like to "persuade" people not to use it on ARM.

> It hasn't? I think Microsoft disagrees. It's also how vendors have been able to
> present an older machine description to keep their newer hardware compatible
> with older software, isn't it? How do you expect to handle that if you can
> only present one table? It's the same challenge that DT has.

It seems sensible to recommend against using OS specifics if possible if
only from the point of view of improving the robustness of the system -
the less paths there are to test in the BIOS the more likely it is that
the active path is one that's been well tested. It's legal in the spec
and you can do it but encouraging people not to do it will hopefully
make life easier down the line. Similarly encouraging people to put as
little as possible in there should reduce the opportunities they have to
get things wrong.

The best use case for OS testing is to enable a non-default workaround
for older versions of the OS but in the case of Linux that's a bit
tricky since we don't have clear versions to test against - even with
the kernel version number it's never clear if it's been patched by a
distro or something. Windows is a much more fixed target here.

Attachments:

(No filename) (2.05 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2014-07-28 18:28:28

by Sudeep Holla

[permalink] [raw]

Subject: Re: [PATCH 03/19] ARM64 / ACPI: Introduce lowlevel suspend function

On 24/07/14 14:00, Hanjun Guo wrote:
> From: Graeme Gregory <[email protected]>
>
> acpi_wakeup_address is used on x86 as the address bios jumps into
> when machine wakes up from suspend. As arm64 does not have such a
> bios this mechanism will be provided by other means. But the define
> is still required inside the acpi core.
>
> Introduce a null stub for acpi_suspend_lowlevel as this is also
> required by core. This will be filled in when standards are
> defined for arm64 ACPI global power states.
>
> Reviewed-by: Grant Likely <[email protected]>
> Signed-off-by: Graeme Gregory <[email protected]>
> Signed-off-by: Hanjun Guo <[email protected]>
> ---
> arch/arm64/include/asm/acpi.h | 12 ++++++++++++
> arch/arm64/kernel/acpi.c | 7 +++++++
> 2 files changed, 19 insertions(+)
>
> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> index e8581ea..44b617f 100644
> --- a/arch/arm64/include/asm/acpi.h
> +++ b/arch/arm64/include/asm/acpi.h
> @@ -36,6 +36,18 @@ static inline bool acpi_has_cpu_in_madt(void)
> return 1;
> }
>
> +/* Low-level suspend routine.
> + *
> + * ACPI S-states for ARM64 have to be defined
> + * and approved before doing anything else, maybe
> + * we need update the ACPI spec, here we
> + * just introduce function and macro needed by
> + * ACPI core as IA64 did, and revisit them when
> + * the spec is ready.
> + */
> +extern int (*acpi_suspend_lowlevel)(void);
> +#define acpi_wakeup_address 0
> +

While I understand that this is temporary solution, but will this be
sufficient to make sure we don't enter acpi_suspend_enter ?

A brief look at acpi_suspend_enter showed access to write to
ACPI_BITREG_SCI_ENABLE which might just explode. But if you are sure that
it will not be executed, then it should be fine for now.

Regards,
Sudeep

2014-07-28 18:29:00

by Sudeep Holla

[permalink] [raw]

Subject: Re: [PATCH 01/19] ARM64 / ACPI: Get RSDP and ACPI boot-time tables

On 24/07/14 14:00, Hanjun Guo wrote:
> From: Al Stone <[email protected]>
>
> 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/cpu.h need for ACPI core and will be updated in the future to
> add definitions for arch_(un)register_cpu which are required for
> ACPI based physical CPU hotplug;
> - asm/acenv.h for arch specific ACPICA environments and
> implementation;
> - 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 <[email protected]>
> Signed-off-by: Graeme Gregory <[email protected]>
> Signed-off-by: Tomasz Nowicki <[email protected]>
> Signed-off-by: Hanjun Guo <[email protected]>
> ---
> arch/arm64/include/asm/acenv.h | 18 ++++++++++
> arch/arm64/include/asm/acpi.h | 41 ++++++++++++++++++++++
> arch/arm64/include/asm/cpu.h | 11 ++++++
> arch/arm64/kernel/Makefile | 1 +
> arch/arm64/kernel/acpi.c | 76 ++++++++++++++++++++++++++++++++++++++++
> arch/arm64/kernel/setup.c | 4 +++
> 6 files changed, 151 insertions(+)
> 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/cpu.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..3899ee6
> --- /dev/null
> +++ b/arch/arm64/include/asm/acenv.h
> @@ -0,0 +1,18 @@
> +/*
> + * ARM64 specific ACPICA environments and implementation
> + *
> + * Copyright (C) 2014, Linaro Ltd.
> + * Author: Hanjun Guo <[email protected]>
> + * Author: Graeme Gregory <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef _ASM_ACENV_H
> +#define _ASM_ACENV_H
> +
> +#define ACPI_FLUSH_CPU_CACHE() WARN_ONCE(1, "Not currently supported on ARM64")
> +
> +#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..e8581ea
> --- /dev/null
> +++ b/arch/arm64/include/asm/acpi.h
> @@ -0,0 +1,41 @@
> +/*
> + * Copyright (C) 2013-2014, Linaro Ltd.
> + * Author: Al Stone <[email protected]>
> + * Author: Graeme Gregory <[email protected]>
> + * Author: Hanjun Guo <[email protected]>
> + *
> + * 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
> +extern int acpi_disabled;
> +extern int acpi_noirq;
> +extern int acpi_pci_disabled;
> +extern int acpi_strict;

Until we need to enable workarounds, we should have acpi_strict
as macro rather than variable similar to IA64.

> +
> +static inline void disable_acpi(void)
> +{
> + acpi_disabled = 1;
> + acpi_pci_disabled = 1;
> + acpi_noirq = 1;
> +}
> +
> +/*
> + * MADT must provide at least one GICC structure
> + * for GIC initialization, so CPU will be
> + * always available in MADT on ARM64
> + */
> +static inline bool acpi_has_cpu_in_madt(void)
> +{
> + return 1;
> +}
> +

I don't see the user of this function in the series, it's better to
remove. Moreover the comment is so misleading. We will have CPU UID
not the CPU itself in MADT.

> +#endif /* CONFIG_ACPI */
> +
> +#endif /*_ASM_ACPI_H*/
> diff --git a/arch/arm64/include/asm/cpu.h b/arch/arm64/include/asm/cpu.h
> new file mode 100644
> index 0000000..cee7d3f
> --- /dev/null
> +++ b/arch/arm64/include/asm/cpu.h
> @@ -0,0 +1,11 @@
> +/*
> + * Copyright (C) 2013-2014 ARM Ltd.
> + *
> + * 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_CPU_H
> +#define __ASM_CPU_H
> +
> +#endif
> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> index cdaedad..b568c26 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..c62fce6
> --- /dev/null
> +++ b/arch/arm64/kernel/acpi.c
> @@ -0,0 +1,76 @@
> +/*
> + * ARM64 Specific Low-Level ACPI Boot Support
> + *
> + * Copyright (C) 2013-2014, Linaro Ltd.
> + * Author: Al Stone <[email protected]>
> + * Author: Graeme Gregory <[email protected]>
> + * Author: Hanjun Guo <[email protected]>
> + *
> + * 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>
> +
> +/*
> + * We never plan to use RSDT on arm/arm64 as its deprecated in spec but this
> + * variable is still required by the ACPI core
> + */
> +u32 acpi_rsdt_forced;

This looks like yet another stray variable not used anywhere including
core ACPI

> +
> +int acpi_noirq; /* skip ACPI IRQ initialization */
> +int acpi_strict;
> +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_iounmap(map, size);

Why not early_memunmap for consistency ?
Also now there's generic implementation of early mappings does it make
sense to move this also to acpi core ?

Regards,
Sudeep

2014-07-28 18:29:37

by Sudeep Holla

[permalink] [raw]

Hi Hanjun,

On Thu, 2014-07-24 at 21:00 +0800, Hanjun Guo wrote:
> ACPI 5.1 only has two explicit methods to boot up SMP,
> PSCI and Parking protocol, but the Parking protocol is
> only suitable 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 <[email protected]>
> Signed-off-by: Tomasz Nowicki <[email protected]>
> ---
> arch/arm64/include/asm/acpi.h | 21 +++++++++++++++
> arch/arm64/include/asm/cpu_ops.h | 9 ++++++-
> arch/arm64/include/asm/smp.h | 2 +-
> arch/arm64/kernel/acpi.c | 9 +++++++
> arch/arm64/kernel/cpu_ops.c | 52 ++++++++++++++++++++++++++++++++++----
> arch/arm64/kernel/smp.c | 29 +++++++++++++++++++--
> 6 files changed, 113 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> index 5ce85f8..6240327 100644
> --- a/arch/arm64/include/asm/acpi.h
> +++ b/arch/arm64/include/asm/acpi.h
> @@ -14,6 +14,27 @@
>
> /* Basic configuration for ACPI */
> #ifdef CONFIG_ACPI

^^ This seems to be a tab (\t) character here, which is a strange thing
for me to see...

> +/*
> + * ACPI 5.1 only has two explicit methods to
> + * boot up SMP, PSCI and Parking protocol,
> + * but the Parking protocol is only defined
> + * 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.
> + *
> + * This enum is intend to make the boot method
> + * scalable when above updates are happended,
> + * which NOT means to support all of them.
> + */

This comment will become out of date soon (I hope), and it is often the
case that these short term comments are not removed, so I think it
better to put this kind of note into the commit message, not the code.

> +enum acpi_smp_boot_protocol {
> + ACPI_SMP_BOOT_PSCI,
> + ACPI_SMP_BOOT_PARKING_PROTOCOL,
> + ACPI_SMP_BOOT_PROTOCOL_MAX
> +};
> +
> +enum acpi_smp_boot_protocol smp_boot_protocol(void);
> +
> extern int acpi_disabled;
> extern int acpi_noirq;
> extern int acpi_pci_disabled;
> diff --git a/arch/arm64/include/asm/cpu_ops.h b/arch/arm64/include/asm/cpu_ops.h
> index d7b4b38..2a7c6fd 100644
> --- a/arch/arm64/include/asm/cpu_ops.h
> +++ b/arch/arm64/include/asm/cpu_ops.h
> @@ -61,7 +61,14 @@ struct cpu_operations {
> };
>
> extern const struct cpu_operations *cpu_ops[NR_CPUS];
> -extern int __init cpu_read_ops(struct device_node *dn, int cpu);
> +extern int __init cpu_of_read_ops(struct device_node *dn, int cpu);
> +
> +#ifdef CONFIG_ACPI
> +extern int __init cpu_acpi_read_ops(int cpu);
> +#else
> +static inline int __init cpu_acpi_read_ops(int cpu) { return -ENODEV; }
> +#endif

This looks messy and not scalable for new enable methods. It
seems a better way is to retain cpu_read_ops() and its functionality,
which is to return the proper enable method for that cpu in a generic
way.

Is there some reason you can't integrate acpi into the existing
cpu_ops and need to make this completely parallel method?

> extern void __init cpu_read_bootcpu_ops(void);
>
> #endif /* ifndef __ASM_CPU_OPS_H */
> diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h
> index a498f2c..a5cea56 100644
> --- a/arch/arm64/include/asm/smp.h
> +++ b/arch/arm64/include/asm/smp.h
> @@ -39,7 +39,7 @@ 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)
> + * Platform specific SMP operations
> */
> extern void smp_init_cpus(void);
>
> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> index ff0f6a0..2af6662 100644
> --- a/arch/arm64/kernel/acpi.c
> +++ b/arch/arm64/kernel/acpi.c
> @@ -184,6 +184,15 @@ static int __init acpi_parse_madt_gic_cpu_interface_entries(void)
> return 0;
> }
>
> +/* Protocol to bring up secondary CPUs */
> +enum acpi_smp_boot_protocol smp_boot_protocol(void)
> +{
> + if (acpi_psci_present)
> + return ACPI_SMP_BOOT_PSCI;
> + else
> + return ACPI_SMP_BOOT_PARKING_PROTOCOL;
> +}
> +
> static int __init acpi_parse_fadt(struct acpi_table_header *table)
> {
> struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table;
> diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
> index d62d12f..4d9b3cf 100644
> --- a/arch/arm64/kernel/cpu_ops.c
> +++ b/arch/arm64/kernel/cpu_ops.c
> @@ -16,11 +16,13 @@
> * along with this program. If not, see <http://www.gnu.org/licenses/>.
> */
>
> -#include <asm/cpu_ops.h>
> -#include <asm/smp_plat.h>
> #include <linux/errno.h>
> #include <linux/of.h>
> #include <linux/string.h>
> +#include <linux/acpi.h>
> +
> +#include <asm/cpu_ops.h>
> +#include <asm/smp_plat.h>
>
> extern const struct cpu_operations smp_spin_table_ops;
> extern const struct cpu_operations cpu_psci_ops;
> @@ -52,7 +54,7 @@ static const struct cpu_operations * __init cpu_get_ops(const char *name)
> /*
> * Read a cpu's enable method from the device tree and record it in cpu_ops.
> */
> -int __init cpu_read_ops(struct device_node *dn, int cpu)
> +int __init cpu_of_read_ops(struct device_node *dn, int cpu)
> {
> const char *enable_method = of_get_property(dn, "enable-method", NULL);
> if (!enable_method) {
> @@ -76,12 +78,52 @@ int __init cpu_read_ops(struct device_node *dn, int cpu)
> return 0;
> }
>
> +#ifdef CONFIG_ACPI
> +/*
> + * Read a cpu's enable method in the ACPI way and record it in cpu_ops.
> + */
> +int __init cpu_acpi_read_ops(int cpu)
> +{
> + /*
> + * For ACPI 5.1, only two kind of methods are provided,
> + * Parking protocol and PSCI, but Parking protocol is
> + * used on ARMv7 only, so make PSCI as the only method
> + * for SMP initialization before the ACPI spec or Parking
> + * protocol spec is updated.
> + */

Again, this comment will get old fast (I hope).

> + switch (smp_boot_protocol()) {
> + case ACPI_SMP_BOOT_PSCI:
> + cpu_ops[cpu] = cpu_get_ops("psci");
> + break;
> + case ACPI_SMP_BOOT_PARKING_PROTOCOL:
> + default:
> + cpu_ops[cpu] = NULL;
> + break;
> + }
> +
> + if (!cpu_ops[cpu]) {
> + pr_warn("CPU %d: unsupported enable-method, only PSCI is supported\n",
> + cpu);
> + return -EOPNOTSUPP;
> + }
> +
> + return 0;
> +}
> +#endif
> +
> void __init cpu_read_bootcpu_ops(void)
> {
> - struct device_node *dn = of_get_cpu_node(0, NULL);
> + struct device_node *dn;
> +
> + if (!acpi_disabled) {
> + cpu_acpi_read_ops(0);
> + return;
> + }
> +
> + dn = of_get_cpu_node(0, NULL);
> if (!dn) {
> pr_err("Failed to find device node for boot cpu\n");
> return;
> }
> - cpu_read_ops(dn, 0);
> + cpu_of_read_ops(dn, 0);
> }
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index 8f1d37c..cb71662 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -315,7 +315,7 @@ static void (*smp_cross_call)(const struct cpumask *, unsigned int);
> * 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)
> +static void __init of_smp_init_cpus(void)
> {
> struct device_node *dn = NULL;
> unsigned int i, cpu = 1;
> @@ -387,7 +387,7 @@ void __init smp_init_cpus(void)
> if (cpu >= NR_CPUS)
> goto next;
>
> - if (cpu_read_ops(dn, cpu) != 0)
> + if (cpu_of_read_ops(dn, cpu) != 0)
> goto next;
>
> if (cpu_ops[cpu]->cpu_init(dn, cpu))
> @@ -418,6 +418,31 @@ next:
> set_cpu_possible(i, true);
> }
>
> +/*
> + * In ACPI mode, the cpu possible map was enumerated before SMP
> + * initialization when MADT table was parsed, so we can get the
> + * possible map here to initialize CPUs.
> + */
> +static void __init acpi_smp_init_cpus(void)
> +{
> + int cpu;
> +
> + for_each_possible_cpu(cpu) {
> + if (cpu_acpi_read_ops(cpu) != 0)
> + continue;
> +
> + cpu_ops[cpu]->cpu_init(NULL, cpu);
> + }
> +}
> +
> +void __init smp_init_cpus(void)
> +{
> + if (acpi_disabled)
> + of_smp_init_cpus();
> + else
> + acpi_smp_init_cpus();
> +}

This is the same as cpu_ops, is acpi so special we need a completely
parallel method of initializing secondary cpus?

-Geoff