On 2014-8-19 2:30, Sudeep Holla wrote:

On 04/08/14 16:28, Hanjun Guo wrote:

...

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/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;

This can go away now, as it's already present in v3.17-rc1

Thanks for the reminding, I will rebase this patch set on top of 3.17-rc1 and remove the outdated description.

...

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

[...]

...

+/*

• • Checking for the posibility that the CPU can be initialized from the MADT.
• • It's used from ACPI core in crash kernel case where boot CPU is not
• • necessarily CPU0 like spec demands. Since MADT must provide at least one

Do you mean ACPI spec demands CPU0 to be boot cpu ?

ACPI spec demands CPU0 (BSP) should be the first entry in MADT.

IIUC it's basically to handle UP system with SMP kernel. IWO if all the ACPI resources are reused from SMP system to boot UP system with SMP kernel, with this check the ACPI core won't override the CPU index obtained from GIC/APIC mapping to zero.

Yes, you are right, I will update the comments.

...

• • GICC structure for GIC initialization, CPU will be always available in
• • MADT on ARM64.
• */

+static inline bool acpi_has_cpu_in_madt(void) +{

• return 1;

probably true instead of 1

ok.

...

+}

+static inline void arch_fix_phys_package_id(int num, u32 slot) { }

+#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..395778c --- /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
• • 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 FADT, 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()) {

Can't we call acpi_table_init directly in setup ? IIUC x86 has acpi_boot_table_init as it parses Boot Flags table(ACPI_SIG_BOOT)

Yes, we can. But I think it not a big deal to dot that, if that makes the code more readable, I will update it.

...

•    disable_acpi();
  

•    return;
  

• }

+} diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index fc50461..85c6326 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/cputype.h> @@ -385,6 +386,9 @@ void __init setup_arch(char **cmdline_p) efi_init(); arm64_memblock_init();

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

• paging_init();
  request_standard_resources();
  

Thanks Hanjun

On 2014-8-18 22:27, Catalin Marinas wrote:

On Mon, Aug 04, 2014 at 04:28:12PM +0100, Hanjun Guo wrote:

...

There are two flags: PSCI_COMPLIANT and PSCI_USE_HVC. When set, the former signals to the OS that the hardware is PSCI compliant.

Actually it signals that the firmware is PSCI compliant. The hardware doesn't care much.

Right, I will update it.

...

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index 6400312..6e04868 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);

+}

Do we actually need !! here? Shouldn't the compiler figure out conversion to bool automatically?

I thought !! will explicitly show that it's a bool value and improve the readability of the code, but I'm ok to remove !! here.

...

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 9cf9127..69a315d 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -11,6 +11,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> @@ -47,6 +49,26 @@ 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 version,
  

• * and there is a minor version of FADT which was introduced
  

• * by ACPI 5.1, we only deal with ACPI 5.1 or higher version
  

• * to get arm boot flags, or we will disable ACPI.
  

• */
  

• if (table->revision < 5 || fadt->minor_revision < 1) {

If we ever get revision 6.0, this would trigger.

Yes, good catch, actually I already fixed that in my local git repo,

+ if (table->revision > 5 || + (table->revision == 5 && fadt->minor_revision >= 1)) { + return 0; + } else { + pr_info("FADT revision is %d.%d, no PSCI support, should be 5.1 or higher\n", + table->revision, fadt->minor_revision); + disable_acpi(); + return -EINVAL; + }

...

diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index 85c6326..dfc4e4f3 100644 --- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -395,6 +395,8 @@ void __init setup_arch(char **cmdline_p) efi_idmap_init(); cpu_logical_map(0) = read_cpuid_mpidr() & MPIDR_HWID_BITMASK;

• acpi_boot_init();
• unflatten_device_tree();

Unless that's changed in a subsequent patch, do we still need to call unflatten_device_tree() if ACPI was successful?

No, we don't. in [PATCH v2 16/18], we will not call unflatten_device_tree() if ACPI is successful. Since the CONFIG_ACPI is not enabled for ARM64 (will enable it in the last patch), so acpi_boot_init() is a stub empty function here.

...

psci_init();

I would also rename this to something like psci_dt_init() and move the acpi_disabled check here rather than in the callee.

thanks for the suggestion, I will update my patch :)

Thanks Hanjun

On 2014-8-19 19:10, Mark Rutland wrote:

...

...

...

@@ -47,6 +49,26 @@ 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 version,
  

• * and there is a minor version of FADT which was introduced
  

• * by ACPI 5.1, we only deal with ACPI 5.1 or higher version
  

• * to get arm boot flags, or we will disable ACPI.
  

• */
  

• if (table->revision < 5 || fadt->minor_revision < 1) {

If we ever get revision 6.0, this would trigger.

Yes, good catch, actually I already fixed that in my local git repo,

•   if (table->revision > 5 ||
  

•       (table->revision == 5 && fadt->minor_revision >= 1)) {
  

•           return 0;
  

•   } else {
  

•           pr_info("FADT revision is %d.%d, no PSCI support, should be 5.1
  

or higher\n",

•                   table->revision, fadt->minor_revision);
  

•           disable_acpi();
  

•           return -EINVAL;
  

•   }
  

Given you return in the first path, you don't need the remaining code to live in an else block.

Agreed, I will update it, and move disable_acpi() outside this function and keep it in one place as Sudeep suggested.

Thanks Hanjun

Hi Bob,

On 2014-8-20 6:55, Moore, Robert wrote:

I should warn you that FADT version numbers are notoriously unreliable; In fact, in ACPICA we were eventually forced to abandon them entirely. We use the actual size of the FADT instead.

Yes, I heard that story, thanks for the reminding. But I also see that the revision number is used on x86 and ia64 now in parsing FADT, it is a firmware bug not to comply with the spec, and on ARM, only ACPI 5.1 or higher can be used in Linux, Major.Minor revision was introduced in ACPI 5.1 and it was a major change for it, so I think firmware should comply with that, if firmware just copy some code from somewhere else and leave the revision number unchanged, we will not boot (disable ACPI).

The size of FADT for 5.0 and 5.1 is no difference, it is pretty hard to use that to identify the version of FADT, did I miss something?

Thanks Hanjun

Bob

...

-----Original Message----- From: Hanjun Guo [mailto:hanjun.guo@linaro.org] Sent: Tuesday, August 19, 2014 5:14 AM To: Mark Rutland Cc: Catalin Marinas; Rafael J. Wysocki; graeme.gregory@linaro.org; Arnd Bergmann; Olof Johansson; grant.likely@linaro.org; Sudeep Holla; Will Deacon; Jason Cooper; Marc Zyngier; Bjorn Helgaas; Daniel Lezcano; Mark Brown; Rob Herring; Robert Richter; Zheng, Lv; Moore, Robert; Lorenzo Pieralisi; Liviu Dudau; Randy Dunlap; Charles Garcia-Tobin; linux- acpi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- kernel@vger.kernel.org; linaro-acpi@lists.linaro.org Subject: Re: [PATCH v2 05/18] ARM64 / ACPI: Parse FADT table to get PSCI flags for PSCI init

On 2014-8-19 19:10, Mark Rutland wrote:

...

...

...

...

@@ -47,6 +49,26 @@ 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 version,
  

• * and there is a minor version of FADT which was introduced
  

• * by ACPI 5.1, we only deal with ACPI 5.1 or higher version
  

• * to get arm boot flags, or we will disable ACPI.
  

• */
  

• if (table->revision < 5 || fadt->minor_revision < 1) {

If we ever get revision 6.0, this would trigger.

Yes, good catch, actually I already fixed that in my local git repo,

•   if (table->revision > 5 ||
  

•       (table->revision == 5 && fadt->minor_revision >= 1)) {
  

•           return 0;
  

•   } else {
  

•           pr_info("FADT revision is %d.%d, no PSCI support,
  

• should be 5.1

or higher\n",

•                   table->revision, fadt->minor_revision);
  

•           disable_acpi();
  

•           return -EINVAL;
  

•   }
  

Given you return in the first path, you don't need the remaining code to live in an else block.

Agreed, I will update it, and move disable_acpi() outside this function and keep it in one place as Sudeep suggested.

Thanks Hanjun

On 2014-8-19 2:32, Sudeep Holla wrote:

On 04/08/14 16:28, Hanjun Guo wrote:

...

There are two flags: PSCI_COMPLIANT and PSCI_USE_HVC. When set, the former signals to the OS that the hardware 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 bring up secondery CPUs, so disable ACPI if we get an FADT table with version less that 5.1.

[...]

...

+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 version,
  

• * and there is a minor version of FADT which was introduced
  

• * by ACPI 5.1, we only deal with ACPI 5.1 or higher version
  

• * to get arm boot flags, or we will disable ACPI.
  

• */
  

• if (table->revision < 5 || fadt->minor_revision < 1) {

            &&

Catalin also pointed out this bug :), but && is not enough, for example, this would not trigger of revision 4.1, although 4.1 is not exist, but the code will still confusing people, so I updated the code as:

+ if (table->revision > 5 || + (table->revision == 5 && fadt->minor_revision >= 1)) { + return 0; + } else { + pr_info("FADT revision is %d.%d, no PSCI support, should be 5.1 or higher\n", + table->revision, fadt->minor_revision); + disable_acpi(); + return -EINVAL; + }

...

•    pr_info("FADT version is %d.%d, no PSCI support, should be 5.1 or
  

higher\n",

•        table->revision, fadt->minor_revision);
  

•    disable_acpi();
  

Instead of disabling ACPI at multiple places in the boot sequence, can't it be done once if acpi_boot_init failed ?

...

•    return -EINVAL;
  

• }
• return 0;

+}

• /*
  • acpi_boot_table_init() called from setup_arch(), always.
  • 1. find RSDP and get its address, and then find XSDT

@@ -68,6 +90,21 @@ void __init acpi_boot_table_init(void) } }

+int __init acpi_boot_init(void) +{

• int err = 0;
• /* If acpi_disabled, bail out */
• if (acpi_disabled)

•    return -ENODEV;
  

• err = acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt);
• if (err)

•    pr_err("Can't find FADT\n");
  

• return err;

+}

• /*
  • acpi_suspend_lowlevel() - save kernel state and suspend.

diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c index 9e9798f..2ad2084 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> @@ -231,6 +232,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;

+}

Nit: IMO may be you can move these refactoring as separate patch before ACPI related changes ?

I'm ok with it, will format another patch and place it in the beginning of this patch set.

...

/*

• PSCI Function IDs for v0.2+ are well defined so use
• standard values.

@@ -264,29 +292,7 @@ static int 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);

@@ -333,6 +339,40 @@ out_put_node: return err; }

+#ifdef CONFIG_ACPI +static int get_set_conduit_method_acpi(void) +{

• 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;
  

• return 0;

+}

+/* We use PSCI 0.2+ when ACPI is deployed */ +static int psci_0_2_init_acpi(void)

May be this can be renamed psci_acpi_init and called from setup directly instead of calling from DT function with acpi_disabled check.

Also suggested by Catalin, will do it :)

...

+{

• if (!acpi_psci_present()) {

•    pr_info("is not implemented in ACPI.\n");
  

•    return -EOPNOTSUPP;
  

• }
• get_set_conduit_method_acpi();
• psci_0_2_set_functions();
• return 0;

+} +#else +static inline int psci_0_2_init_acpi(void) +{

• return -ENODEV;

+} +#endif

• static const struct of_device_id psci_of_match[] __initconst = { { .compatible = "arm,psci", .data = psci_0_1_init}, { .compatible = "arm,psci-0.2", .data = psci_0_2_init},

@@ -345,6 +385,9 @@ int __init psci_init(void) const struct of_device_id *matched_np; psci_initcall_t init_fn;

• if (!acpi_disabled)

•    return psci_0_2_init_acpi();
  

As mentioned above you need not modify this.

...

  np = of_find_matching_node_and_match(NULL, psci_of_match, &matched_np);

  if (!np)

diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index 85c6326..dfc4e4f3 100644 --- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -395,6 +395,8 @@ void __init setup_arch(char **cmdline_p) efi_idmap_init();

  cpu_logical_map(0) = read_cpuid_mpidr() & MPIDR_HWID_BITMASK;

• acpi_boot_init();

Check for the return error here and disable ACPI once rather than at multiple places all over.

ok.

Thanks Hanjun

On 2014-8-18 22:27, Catalin Marinas wrote:

On Mon, Aug 04, 2014 at 04:28:13PM +0100, Hanjun Guo wrote:

...

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index 6e04868..e877967 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -64,6 +64,8 @@ static inline void arch_fix_phys_package_id(int num, u32 slot) { } extern int (*acpi_suspend_lowlevel)(void); #define acpi_wakeup_address 0 +#define MAX_GIC_CPU_INTERFACE 65535

Does this need to be more than NR_CPUS?

Sometimes yes, CPU structure entries in MADT just like CPU nodes in device tree, the number of them may more than NR_CPUS.

...

+/**

• • acpi_register_gic_cpu_interface - register a gic cpu interface and
• • generates a logical cpu number
• • @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 the gic cpu interface
• */

+static int acpi_register_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;
  

• }
• /* If it is the first CPU, no need to check duplicate MPIDRs */
• if (!enabled_cpus)

• goto skip_mpidr_check;
  

• /*

• * 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_present_cpu(cpu) {

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

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

• 	       mpidr);
  

• 	return -EINVAL;
  

• }
  

• }

+skip_mpidr_check:

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

• /*
  

•  * boot_cpu_init() already hold bit 0 in cpu_present_mask
  

•  * for BSP, no need to allocate again.
  

•  */
  

• cpu = 0;
  

• } else {

• cpu = cpumask_next_zero(-1, cpu_present_mask);
  

• }
• /* map the logical cpu id to cpu MPIDR */
• cpu_logical_map(cpu) = mpidr;
• set_cpu_possible(cpu, true);
• set_cpu_present(cpu, true);
• return cpu;

+}

Why not only set the cpu possible here (with the additional mpidr validity checks changed to for_each_possible_cpu)...

...

diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c index 40f38f4..8f1d37c 100644 --- a/arch/arm64/kernel/smp.c +++ b/arch/arm64/kernel/smp.c @@ -36,6 +36,7 @@ #include <linux/completion.h> #include <linux/of.h> #include <linux/irq_work.h> +#include <linux/acpi.h> #include <asm/atomic.h> #include <asm/cacheflush.h> @@ -458,7 +459,14 @@ void __init smp_prepare_cpus(unsigned int max_cpus) if (err) continue;

• set_cpu_present(cpu, true);
  

• /*
  

•  * In ACPI mode, cpu_present_map was initialised when
  

•  * MADT table was parsed which before this function
  

•  * is called.
  

•  */
  

• if (acpi_disabled)
  

• 	set_cpu_present(cpu, true);
  

• max_cpus--;

... and don't touch this at all.

the original intent was to support CPU hotplug, I will update the patch as you suggested and introduce the change when CPU hotplug patch is upstreamed, then it will make the patch less confusing.

Thanks Hanjun

On 2014-8-20 22:38, Catalin Marinas wrote:

On Tue, Aug 19, 2014 at 08:36:46AM +0100, Hanjun Guo wrote:

...

On 2014-8-18 22:27, Catalin Marinas wrote:

...

On Mon, Aug 04, 2014 at 04:28:13PM +0100, Hanjun Guo wrote:

...

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index 6e04868..e877967 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -64,6 +64,8 @@ static inline void arch_fix_phys_package_id(int num, u32 slot) { } extern int (*acpi_suspend_lowlevel)(void); #define acpi_wakeup_address 0 +#define MAX_GIC_CPU_INTERFACE 65535

Does this need to be more than NR_CPUS?

Sometimes yes, CPU structure entries in MADT just like CPU nodes in device tree, the number of them may more than NR_CPUS.

I have a more general question here. In ACPI, is MADT the only way to build a CPU topology?

Unfortunately yes as far as I can tell.

It looks weird that we use GIC properties to create the cpu_logical_map().

Actually information in GICC structures in MADT will both used for GIC init and SMP init, GICC structures represents CPUs in the system.

A side-effect is that the GIC-related functions are now scattered all over the kernel rather than being contained in the GIC driver itself.

As patch 12/18 shows, all the GIC related code all contained in the GIC driver, GICC structure is more than GIC-related but also CPUs in the system (I have to admit that the name of GICC in ACPI is confusing).

Thanks Hanjun

Hi Sudeep,

On 2014-8-19 2:33, Sudeep Holla wrote:

On 04/08/14 16:28, 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 and cpu_present_map.

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/kernel/acpi.c | 129 ++++++++++++++++++++++++++++++++++++++++- arch/arm64/kernel/smp.c | 10 +++- 3 files changed, 138 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index 6e04868..e877967 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -64,6 +64,8 @@ static inline void arch_fix_phys_package_id(int num, u32 slot) { } extern int (*acpi_suspend_lowlevel)(void); #define acpi_wakeup_address 0

+#define MAX_GIC_CPU_INTERFACE 65535

Did you get this information from GIC specification ?

Actually not. since MAX_GIC_CPU_INTERFACE represents the maximum GIC CPU interfaces (CPUs) entries in MADT, so I use a number big enough to represent max CPUs in the system.

IIUC you are trying to represent max number of interrupt controller entries MADT can possibly have. So, I had suggested to change the name like MAX_MADT_INTERRUPT_CONTROLLER_ENTRIES so that it is not GIC specific.

Will INTERRUPT_CONTROLLER confuse people? There is only one GIC redistributor (some people regard it as interrupt controller) in ARM system, if we use INTERRUPT_CONTROLLER people will regard it as multi-redistributors in the system, I think GIC_CPU_INTERFACE would be better, what do you think?

...

#endif /* CONFIG_ACPI */

#endif /*_ASM_ACPI_H*/ diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 69a315d..9e07d99 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -22,6 +22,9 @@ #include <linux/bootmem.h> #include <linux/smp.h>

+#include <asm/smp_plat.h> +#include <asm/cputype.h>

• int acpi_noirq; /* skip ACPI IRQ initialization */ int acpi_disabled; EXPORT_SYMBOL(acpi_disabled);

@@ -29,6 +32,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.

@@ -49,6 +54,122 @@ void __init __acpi_unmap_table(char *map, unsigned long size) early_memunmap(map, size); }

+/**

• • acpi_register_gic_cpu_interface - register a gic cpu interface and
• • generates a logical cpu number
• • @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 the gic cpu interface
• */

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

IMO the function name gives me a wrong idea that you are registering something with GIC. How about acpi_map_gic_cpu_interface ?

Great, acpi_map_gic_cpu_interface is better.

...

• 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;
  

• }
• /* If it is the first CPU, no need to check duplicate MPIDRs */
• if (!enabled_cpus)

•    goto skip_mpidr_check;
  

• /*

• * 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_present_cpu(cpu) {

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

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

•               mpidr);
  

•        return -EINVAL;
  

•    }
  

• }

+skip_mpidr_check:

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

•    /*
  

•     * boot_cpu_init() already hold bit 0 in cpu_present_mask
  

•     * for BSP, no need to allocate again.
  

•     */
  

•    cpu = 0;
  

• } else {

•    cpu = cpumask_next_zero(-1, cpu_present_mask);
  

• }
• /* map the logical cpu id to cpu MPIDR */
• cpu_logical_map(cpu) = mpidr;
• set_cpu_possible(cpu, true);

IMO it better to keep all these updates to cpumasks contained in the smp.c as I had mentioned before. I think you can refactor smp_init_cpus to achieve that.

Could you give me more hints? smp_init_cpus() use the CPU node in device tree to map cpu, but in ACPI, they are table entries, if you can give me more hints about how to refactor it, I will appreciate a lot.

...

• set_cpu_present(cpu, true);

This is totally wrong ? What would you do if the cpu failed to initialize ? I don't see that handled.

...

• 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_register_gic_cpu_interface(processor->arm_mpidr,

•    processor->flags & ACPI_MADT_ENABLED);
  

• return 0;

+}

+/*

• • Parse GIC cpu interface related entries in MADT
• • returns 0 on success, < 0 on error
• */

+static int __init acpi_parse_madt_gic_cpu_interface_entries(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, MAX_GIC_CPU_INTERFACE);
  

• if (!count) {

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

•    return -ENODEV;
  

• } else if (count < 0) {

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

•    return count;
  

• }
• /* Make boot-up look pretty */
• pr_info("%d CPUs enabled, %d CPUs total\n", enabled_cpus, total_cpus);

Ideally just setup cpu_logical_map in acpi_parse_gic_cpu_interface and setup cpumasks in smp_init_cpus.

...

• return 0;

+}

• static int __init acpi_parse_fadt(struct acpi_table_header *table) { struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table;

@@ -99,8 +220,12 @@ int __init acpi_boot_init(void) return -ENODEV;

  err = acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt);

• if (err)

•    pr_err("Can't find FADT\n");
  

• if (err) {

•    pr_err("Can't find FADT or error happened during parsing FADT\n");
  

•    return err;
  

• }

• err = acpi_parse_madt_gic_cpu_interface_entries();

  return err; }

diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c index 40f38f4..8f1d37c 100644 --- a/arch/arm64/kernel/smp.c +++ b/arch/arm64/kernel/smp.c @@ -36,6 +36,7 @@ #include <linux/completion.h> #include <linux/of.h> #include <linux/irq_work.h> +#include <linux/acpi.h>

#include <asm/atomic.h> #include <asm/cacheflush.h> @@ -458,7 +459,14 @@ void __init smp_prepare_cpus(unsigned int max_cpus) if (err) continue;

•    set_cpu_present(cpu, true);
  

•    /*
  

•     * In ACPI mode, cpu_present_map was initialised when
  

•     * MADT table was parsed which before this function
  

•     * is called.
  

•     */
  

•    if (acpi_disabled)
  

•        set_cpu_present(cpu, true);
  

This is the right place to set the cpu present based on the return value of cpu_prepare.

Yes, as I replied in previous email, I will update the patch and consider CPU hotplug in the future.

Thanks Hanjun

Hi,

From: Hanjun Guo [mailto:hanjun.guo@linaro.org] Sent: Monday, August 04, 2014 11:28 PM To: Catalin Marinas; Rafael J. Wysocki; Mark Rutland

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 | 39 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index 6d5a6cd..77f8bb1 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -183,6 +183,44 @@ 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[0x%08llx] MPDIR[0x%llx] %s)\n",
  

• 		p->uid, p->base_address, p->arm_mpidr,
  

Just reminders. The acpi_id looks vague, is it a UID for the processor? Should 0x%08llx for base_address be %p?

• 		(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[0x%08llx] gsi_base[%d])\n",
  

• 		p->gic_id, p->base_address, p->global_irq_base);
  

• }
  

• break;
  

Same question for base_address.

• 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[0x%08llx])\n",
  

• 		p->msi_frame_id, p->base_address);
  

Ditto.

• }
  

• break;
  

• case ACPI_MADT_TYPE_GENERIC_REDISTRIBUTOR:

• {
  

• 	struct acpi_madt_generic_redistributor *p =
  

• 		(struct acpi_madt_generic_redistributor *)header;
  

• 	pr_info("GIC Redistributor (address[0x%08llx] region_size[0x%x])\n",
  

• 		p->base_address, p->length);
  

Ditto.

• }
  

• break;
  

• default: pr_warn("Found unsupported MADT entry (type = 0x%x)\n", header->type);

@@ -190,7 +228,6 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) } }

int __init acpi_table_parse_entries(char *id, unsigned long table_size,

This block shouldn't be a part of this patch.

Thanks and best regards -Lv

-- 1.7.9.5

On 2014-8-18 22:27, Catalin Marinas wrote:

On Mon, Aug 04, 2014 at 04:28:15PM +0100, Hanjun Guo wrote:

...

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index e877967..022f4ad 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 +/*

• • 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.
• */

+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);

Since this is not a static function and ACPI-specific, could you prefix it with something like acpi (or arm64_acpi_ unless it gets too long)?

...

diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c index d62d12f..05bc314 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; @@ -49,12 +51,44 @@ static const struct cpu_operations * __init cpu_get_ops(const char *name) return NULL; } +#ifdef CONFIG_ACPI +/*

• • Get a cpu's boot method in the ACPI way.
• */

+static char * __init acpi_get_cpu_boot_method(void) +{

• /*

• * For ACPI 5.1, only two kind of methods are provided,
  

• * Parking protocol and PSCI, but Parking protocol is
  

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

• * for SMP initialization before the ACPI spec or Parking
  

• * protocol spec is updated.
  

• */
  

• switch (smp_boot_protocol()) {
• case ACPI_SMP_BOOT_PSCI:

• return "psci";
  

• case ACPI_SMP_BOOT_PARKING_PROTOCOL:
• default:

• return NULL;
  

• }

+}

Actually, do we even need to define smp_boot_protocol()? Is it used anywhere else apart from this patch (I still haven't gone through all patches)?

It is just used in this patch. I think we can make the ACPI boot protocol scalable in this way, if we support another boot protocol in ACPI in the future, we can easily update the function to support it, does it make sense?

...

+#else +static inline char * __init acpi_get_cpu_boot_method(void) { return NULL; } +#endif

/*

• • Read a cpu's enable method from the device tree and record it in cpu_ops.

• • Read a cpu's enable method and record it in cpu_ops.
  */

int __init cpu_read_ops(struct device_node *dn, int cpu) {

• const char *enable_method = of_get_property(dn, "enable-method", NULL);

• const char *enable_method;
• if (!acpi_disabled) {

• enable_method = acpi_get_cpu_boot_method();
  

• goto get_ops;
  

Are we always guaranteed an enable_method here? You should have an if/else of ACPI vs DT and keep the if (!enable_method) check for both cases.

ok, I will fix it.

Thanks Hanjun

On 2014-8-19 2:34, Sudeep Holla wrote:

On 04/08/14 16:28, Hanjun Guo wrote:

[...]

...

/* Basic configuration for ACPI */ #ifdef CONFIG_ACPI +/*

• • 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.
• */

+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);

[...]

...

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

+}

Which do you need this here ? Can't you use acpi_psci_present directly in acpi_get_cpu_boot_method ?

My intent was to make the code scalable if we introduce another (or more) boot protocol in ACPI, does it make sense to you?

...

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..05bc314 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; @@ -49,12 +51,44 @@ static const struct cpu_operations * __init cpu_get_ops(const char *name) return NULL; }

+#ifdef CONFIG_ACPI +/*

• • Get a cpu's boot method in the ACPI way.
• */

+static char * __init acpi_get_cpu_boot_method(void) +{

• /*

• * For ACPI 5.1, only two kind of methods are provided,
  

• * Parking protocol and PSCI, but Parking protocol is
  

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

• * for SMP initialization before the ACPI spec or Parking
  

• * protocol spec is updated.
  

• */
  

• switch (smp_boot_protocol()) {
• case ACPI_SMP_BOOT_PSCI:

•    return "psci";
  

• case ACPI_SMP_BOOT_PARKING_PROTOCOL:
• default:

•    return NULL;
  

• }

Use acpi_psci_present as mentioned above.

...

+} +#else +static inline char * __init acpi_get_cpu_boot_method(void) { return NULL; } +#endif

• /*

• • Read a cpu's enable method from the device tree and record it in cpu_ops.

• • Read a cpu's enable method and record it in cpu_ops.
  */ int __init cpu_read_ops(struct device_node *dn, int cpu) {

• const char *enable_method = of_get_property(dn, "enable-method", NULL);

• const char *enable_method;
• if (!acpi_disabled) {

•    enable_method = acpi_get_cpu_boot_method();
  

•    goto get_ops;
  

• }
• enable_method = of_get_property(dn, "enable-method", NULL); if (!enable_method) { /* * The boot CPU may not have an enable method (e.g. when

@@ -66,10 +100,17 @@ int __init cpu_read_ops(struct device_node *dn, int cpu) return -ENOENT; }

+get_ops: cpu_ops[cpu] = cpu_get_ops(enable_method); if (!cpu_ops[cpu]) {

•    pr_warn("%s: unsupported enable-method property: %s\n",
  

•        dn->full_name, enable_method);
  

•    if (acpi_disabled) {
  

•        pr_warn("%s: unsupported enable-method property: %s\n",
  

•            dn->full_name, enable_method);
  

•    } else {
  

•        pr_warn("CPU %d: boot protocol unsupported or unknown\n",
  

•            cpu);
  

•    }
  

•     return -EOPNOTSUPP;
  }
  
  

@@ -78,7 +119,14 @@ int __init cpu_read_ops(struct device_node *dn, int cpu)

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_read_ops(NULL, 0);
  

•    return;
  

• }

Again not good to mix ACPI in DT functions forcing you to pass device_node ptr as NULL, better to separate this.

I separate them in the first version, and combine tham as Geoff suggested for scalable reasons.

Once you gather all this !acpi_disabled case, you can create appropriate abstractions to be used in setup.c

E.g. here you check !acpi_disabled and pass NULL for DT node to cpu_read_ops and hence again you check for !acpi_disabled in cpu_read_ops. So you need first identify all these checks and put in one place to understand well how you can refactor existing code to avoid these multiple checks.

I will remove the multiple acpi_disabled checks and refactor the code.

Thanks Hanjun

On 2014-8-19 2:56, Geoff Levand wrote:

Hi Hanjun,

Hi Geoff,

On Mon, 2014-08-04 at 23:28 +0800, Hanjun Guo wrote:

...

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

By having this preprocessor conditional in the header leads to a proliferation of preprocessor conditionals since any code that includes this header will also need to have preprocessor conditionals. Another down side of having this is that this code will not get a build test for builds with CONFIG_ACPI=n.

I will move some definitions out of preprocessor conditional and introduce some stub function when CONFIG_ACPI is disabled, then I think I can remove all the preprocessor conditionals in .c file.

...

+/*

• • 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.
• */

+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);

The name smp_boot_protocol() seems like it would be a generic routine, but it is acpi specific. Maybe:

enum acpi_boot_protocol_type {...};

enum acpi_boot_protocol_type acpi_boot_protocol(void);

Agreed.

...

--- a/arch/arm64/kernel/cpu_ops.c +++ b/arch/arm64/kernel/cpu_ops.c @@ -49,12 +51,44 @@ static const struct cpu_operations * __init cpu_get_ops(const char *name) return NULL; } +#ifdef CONFIG_ACPI +/*

• • Get a cpu's boot method in the ACPI way.
• */

+static char * __init acpi_get_cpu_boot_method(void) +{

• /*

• * For ACPI 5.1, only two kind of methods are provided,
  

• * Parking protocol and PSCI, but Parking protocol is
  

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

• * for SMP initialization before the ACPI spec or Parking
  

• * protocol spec is updated.
  

• */
  

• switch (smp_boot_protocol()) {
• case ACPI_SMP_BOOT_PSCI:

• return "psci";
  

• case ACPI_SMP_BOOT_PARKING_PROTOCOL:
• default:

• return NULL;
  

• }

+} +#else +static inline char * __init acpi_get_cpu_boot_method(void) { return NULL; } +#endif

Since this is inside a C source file, the inline keyword isn't needed since the optimizer will inline regardless.

With that said, I think it would be cleaner to have this as:

static char * __init acpi_get_cpu_boot_method(void) { #ifdef CONFIG_ACPI return NULL; #else ... #endif }

Or better to make smp_boot_protocol() callable regardless of CONFIG_ACPI and then no preprocessor conditionals at all would be needed.

...

/*

• • Read a cpu's enable method from the device tree and record it in cpu_ops.

• • Read a cpu's enable method and record it in cpu_ops.
  */

int __init cpu_read_ops(struct device_node *dn, int cpu) {

• const char *enable_method = of_get_property(dn, "enable-method", NULL);

• const char *enable_method;
• if (!acpi_disabled) {

• enable_method = acpi_get_cpu_boot_method();
  

• goto get_ops;
  

• }
• enable_method = of_get_property(dn, "enable-method", NULL); if (!enable_method) { /*
  • The boot CPU may not have an enable method (e.g. when

@@ -66,10 +100,17 @@ int __init cpu_read_ops(struct device_node *dn, int cpu) return -ENOENT; } +get_ops: cpu_ops[cpu] = cpu_get_ops(enable_method); if (!cpu_ops[cpu]) {

• pr_warn("%s: unsupported enable-method property: %s\n",
  

• 	dn->full_name, enable_method);
  

• if (acpi_disabled) {
  

• 	pr_warn("%s: unsupported enable-method property: %s\n",
  

• 		dn->full_name, enable_method);
  

• } else {
  

• 	pr_warn("CPU %d: boot protocol unsupported or unknown\n",
  

• 		cpu);
  

• }
  

Can't we have this more integrated, maybe something like this?

enable_method = acpi_disabled ? of_get_property(dn, "enable-method", NULL) : acpi_get_cpu_boot_method();

I like this :)

message = acpi_disabled ? dn->full_name : "";

... pr_warn("CPU %d: %s unsupported enable-method property: %s\n", cpu, message, enable_method)

In ACPI, there is no enable-method property, it is a term from, so I think the message printed can be separated.

Thanks Hanjun

On Mon, Aug 04, 2014 at 04:28:16PM +0100, Hanjun Guo wrote:

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index 022f4ad..a81898d 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 /* @@ -59,6 +61,18 @@ static inline void disable_acpi(void) acpi_noirq = 1; } +u32 pack_mpidr_into_32_bits(u64 mpidr);

I would define this as a static inline function here.

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;
  

You can use pack_mpidr_into_32_bits().

On Tue, Aug 19, 2014 at 09:37:34AM +0100, Hanjun Guo wrote:

On 2014-8-18 22:27, Catalin Marinas wrote:

...

On Mon, Aug 04, 2014 at 04:28:16PM +0100, Hanjun Guo wrote:

...

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;
  

You can use pack_mpidr_into_32_bits().

processor_core.c will be used by x86 and ia64 too, it will cause compile error on !ARM64 platforms.

Oh. So we do we have an ARM-specific function in core ACPI code?

On 04/08/14 16:28, 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 | 14 ++++++++++++++ arch/arm64/kernel/acpi.c | 21 ++++++++++++++++++++- drivers/acpi/processor_core.c | 37 +++++++++++++++++++++++++++++++++++++ 3 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index 022f4ad..a81898d 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 /*

@@ -59,6 +61,18 @@ static inline void disable_acpi(void) acpi_noirq = 1; }

+u32 pack_mpidr_into_32_bits(u64 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.

You need to specify this is packed version of cpu_logical_map or better is to move pack_mpidr_into_32_bits here along with the description you have so that it's easy to understand.

• */

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

• /*
  • Checking for the posibility that the CPU can be initialized from the MADT.
  • It's used from ACPI core in crash kernel case where boot CPU is not

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 8a54b4e..ac7ab34 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -22,7 +22,6 @@ #include <linux/bootmem.h> #include <linux/smp.h>

-#include <asm/smp_plat.h> #include <asm/cputype.h>

int acpi_noirq; /* skip ACPI IRQ initialization */ @@ -239,6 +238,26 @@ int __init acpi_boot_init(void) return err; }

+/* 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.
• */

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

+}

• /*
  • acpi_suspend_lowlevel() - save kernel state and suspend.

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;
  

Reuse pack_mpidr_into_32_bits

• 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:

On 2014-8-19 2:34, Sudeep Holla wrote:

On 04/08/14 16:28, Hanjun Guo wrote:

...

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

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

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

---

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

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index ac7ab34..621ced8 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -34,6 +34,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.

*/ @@ -169,6 +175,73 @@ static int __init acpi_parse_madt_gic_cpu_interface_entries(void) return 0; }

+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);

Not sure if acpi_gsi_to_irq and acpi_{,un}register_gsi belong here. These are GIC specific and should belong to GIC driver IMO.

I think we can keep them here as GSI is ACPI specific and they are not GIC init related.

Thanks Hanjun

On Mon, Aug 04, 2014 at 04:28:24PM +0100, Hanjun Guo wrote:

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index 3f5f745..f6b6791 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

depends on (ARM64 && EXPERT)? (together with default on if CONFIG_ACPI is enabled).

On 2014-8-5 4:48, Randy Dunlap wrote:

On 08/04/14 08:28, 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 | 215 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 215 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..a6e14a2 --- /dev/null +++ b/Documentation/arm64/arm-acpi.txt @@ -0,0 +1,215 @@ +ACPI on ARMv8 Servers +---------------------

...

...

+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 not being set,

                                      If CONFIG_ACPI is not set,

Good catch, We will update it.

Thanks Hanjun