On Wed, Mar 11, 2015 at 12:39:32PM +0000, Hanjun Guo wrote:

From: Graeme Gregory graeme.gregory@linaro.org

ACPI 5.1 does not currently support S states for ARM64 hardware but ACPI code will call acpi_target_system_state() and acpi_sleep_init() for device power management, so introduce CONFIG_ACPI_GENERIC_SLEEP and select it for x86 and ia64 only to make sleep functions available, and also introduce stub function to allow other drivers to function until S states are defined for ARM64.

It will be no functional change for x86 and IA64.

CC: Rafael J. Wysocki rjw@rjwysocki.net Suggested-by: Rafael J. Wysocki rafael.j.wysocki@intel.com Acked-by: Robert Richter rrichter@cavium.com Signed-off-by: Graeme Gregory graeme.gregory@linaro.org Signed-off-by: Tomasz Nowicki tomasz.nowicki@linaro.org Signed-off-by: Hanjun Guo hanjun.guo@linaro.org

It looks ok to me:

Acked-by: Lorenzo Pieralisi lorenzo.pieralisi@arm.com

---

arch/ia64/Kconfig | 1 + arch/x86/Kconfig | 1 + drivers/acpi/Kconfig | 4 ++++ drivers/acpi/Makefile | 2 +- drivers/acpi/internal.h | 4 ++++ 5 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig index 074e52b..e8728d7 100644 --- a/arch/ia64/Kconfig +++ b/arch/ia64/Kconfig @@ -10,6 +10,7 @@ config IA64 select ARCH_MIGHT_HAVE_PC_SERIO select PCI if (!IA64_HP_SIM) select ACPI if (!IA64_HP_SIM)

• select ACPI_GENERIC_SLEEP if ACPI select ARCH_MIGHT_HAVE_ACPI_PDC if ACPI select HAVE_UNSTABLE_SCHED_CLOCK select HAVE_IDE

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index b7d31ca..9804431 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -22,6 +22,7 @@ config X86_64 ### Arch settings config X86 def_bool y

• select ACPI_GENERIC_SLEEP if ACPI select ARCH_MIGHT_HAVE_ACPI_PDC if ACPI select ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS select ARCH_HAS_FAST_MULTIPLIER

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index e6c3ddd..a7b9120 100644 --- a/drivers/acpi/Kconfig +++ b/drivers/acpi/Kconfig @@ -48,9 +48,13 @@ config ACPI_LEGACY_TABLES_LOOKUP config ARCH_MIGHT_HAVE_ACPI_PDC bool +config ACPI_GENERIC_SLEEP

• bool

config ACPI_SLEEP bool depends on SUSPEND || HIBERNATION

• depends on ACPI_GENERIC_SLEEP default y

config ACPI_PROCFS_POWER diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index 623b117..2397822 100644 --- a/drivers/acpi/Makefile +++ b/drivers/acpi/Makefile @@ -23,7 +23,7 @@ acpi-y += nvs.o # Power management related files acpi-y += wakeup.o -acpi-y += sleep.o +acpi-$(CONFIG_ACPI_GENERIC_SLEEP) += sleep.o acpi-y += device_pm.o acpi-$(CONFIG_ACPI_SLEEP) += proc.o diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h index 56b321a..b5eef4c 100644 --- a/drivers/acpi/internal.h +++ b/drivers/acpi/internal.h @@ -161,7 +161,11 @@ void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit); /*-------------------------------------------------------------------------- Suspend/Resume -------------------------------------------------------------------------- */ +#ifdef CONFIG_ACPI_GENERIC_SLEEP extern int acpi_sleep_init(void); +#else +static inline int acpi_sleep_init(void) { return -ENOSYS; } +#endif #ifdef CONFIG_ACPI_SLEEP int acpi_sleep_proc_init(void); -- 1.9.1

On 2015/3/13 6:57, Rafael J. Wysocki wrote:

On Wednesday, March 11, 2015 08:39:32 PM Hanjun Guo wrote:

...

From: Graeme Gregory graeme.gregory@linaro.org

ACPI 5.1 does not currently support S states for ARM64 hardware but ACPI code will call acpi_target_system_state() and acpi_sleep_init() for device power management, so introduce CONFIG_ACPI_GENERIC_SLEEP and select it for x86 and ia64 only to make sleep functions available, and also introduce stub function to allow other drivers to function until S states are defined for ARM64.

It will be no functional change for x86 and IA64.

CC: Rafael J. Wysocki rjw@rjwysocki.net Suggested-by: Rafael J. Wysocki rafael.j.wysocki@intel.com Acked-by: Robert Richter rrichter@cavium.com Signed-off-by: Graeme Gregory graeme.gregory@linaro.org Signed-off-by: Tomasz Nowicki tomasz.nowicki@linaro.org Signed-off-by: Hanjun Guo hanjun.guo@linaro.org

---

arch/ia64/Kconfig | 1 + arch/x86/Kconfig | 1 + drivers/acpi/Kconfig | 4 ++++ drivers/acpi/Makefile | 2 +- drivers/acpi/internal.h | 4 ++++ 5 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig index 074e52b..e8728d7 100644 --- a/arch/ia64/Kconfig +++ b/arch/ia64/Kconfig @@ -10,6 +10,7 @@ config IA64 select ARCH_MIGHT_HAVE_PC_SERIO select PCI if (!IA64_HP_SIM) select ACPI if (!IA64_HP_SIM)

• select ACPI_GENERIC_SLEEP if ACPI select ARCH_MIGHT_HAVE_ACPI_PDC if ACPI select HAVE_UNSTABLE_SCHED_CLOCK select HAVE_IDE

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index b7d31ca..9804431 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -22,6 +22,7 @@ config X86_64 ### Arch settings config X86 def_bool y

• select ACPI_GENERIC_SLEEP if ACPI select ARCH_MIGHT_HAVE_ACPI_PDC if ACPI select ARCH_HAS_DEBUG_STRICT_USER_COPY_CHECKS select ARCH_HAS_FAST_MULTIPLIER

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index e6c3ddd..a7b9120 100644 --- a/drivers/acpi/Kconfig +++ b/drivers/acpi/Kconfig @@ -48,9 +48,13 @@ config ACPI_LEGACY_TABLES_LOOKUP config ARCH_MIGHT_HAVE_ACPI_PDC bool +config ACPI_GENERIC_SLEEP

• bool

config ACPI_SLEEP bool depends on SUSPEND || HIBERNATION

• depends on ACPI_GENERIC_SLEEP default y

config ACPI_PROCFS_POWER diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index 623b117..2397822 100644 --- a/drivers/acpi/Makefile +++ b/drivers/acpi/Makefile @@ -23,7 +23,7 @@ acpi-y += nvs.o # Power management related files acpi-y += wakeup.o -acpi-y += sleep.o +acpi-$(CONFIG_ACPI_GENERIC_SLEEP) += sleep.o acpi-y += device_pm.o acpi-$(CONFIG_ACPI_SLEEP) += proc.o diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h index 56b321a..b5eef4c 100644 --- a/drivers/acpi/internal.h +++ b/drivers/acpi/internal.h @@ -161,7 +161,11 @@ void acpi_ec_remove_query_handler(struct acpi_ec *ec, u8 query_bit); /*-------------------------------------------------------------------------- Suspend/Resume -------------------------------------------------------------------------- */ +#ifdef CONFIG_ACPI_GENERIC_SLEEP extern int acpi_sleep_init(void); +#else +static inline int acpi_sleep_init(void) { return -ENOSYS; }

Please change the return value here, preferably to -ENXIO.

-ENOSYS *specifically* means "system call not implemented" and should only be used to indicate this particular condition.

Oops, you mentioned this for another patch, I will send a updated patch for this one.

Thanks Hanjun

On 18 March 2015 at 12:35, Lorenzo Pieralisi lorenzo.pieralisi@arm.com wrote:

On Wed, Mar 11, 2015 at 12:39:34PM +0000, Hanjun Guo wrote:

...

From: Al Stone al.stone@linaro.org

This implements the following policy to decide whether ACPI should be used to boot the system:

• acpi=off: ACPI will not be used to boot the system, even if there is no alternative available (e.g., device tree is empty)
• acpi=force: only ACPI will be used to boot the system; if that fails, there will be no fallback to alternative methods (such as device tree)

I think this comment is stale. acpi=force enables ACPI and tries to init the ACPI tables without even checking DT, but it does fall back to DT if ACPI table init fails (by disabling ACPI and unflattening the FDT).

Am I wrong ?

No, you're right. But I would suggest that we fix the code, not the comment.

I think we are all in agreement on the policy, we only need to make disable_acpi() conditional on whether acpi_param_force is set

...

• otherwise, ACPI will be used as a fallback if the device tree turns out to lack a platform description; the heuristic to decide this is whether /chosen is the only node present at depth 1

CC: Catalin Marinas catalin.marinas@arm.com CC: Will Deacon will.deacon@arm.com CC: Rafael J. Wysocki rjw@rjwysocki.net Acked-by: Olof Johansson olof@lixom.net Acked-by: Grant Likely grant.likely@linaro.org Tested-by: Timur Tabi timur@codeaurora.org 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 Signed-off-by: Ard Biesheuvel ard.biesheuvel@linaro.org

---

Documentation/kernel-parameters.txt | 3 ++- arch/arm64/include/asm/acpi.h | 7 +++++ arch/arm64/kernel/acpi.c | 52 +++++++++++++++++++++++++++++++++---- 3 files changed, 56 insertions(+), 6 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt index bfcb1a6..d6c35a7 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,ARM64]
                  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" or "acpi=force" are available
  
                  See also Documentation/power/runtime_pm.txt, pci=noacpi
  
  

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index 40e0924..c5a9b97 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -39,6 +39,13 @@ static inline void disable_acpi(void) acpi_noirq = 1; }

+static inline void enable_acpi(void) +{

• acpi_disabled = 0;
  

• acpi_pci_disabled = 0;
  

• acpi_noirq = 0;
  

+}

/*

• 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 7abac24..2269e30 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -22,15 +22,49 @@ #include <linux/irq.h> #include <linux/irqdomain.h> #include <linux/memblock.h> +#include <linux/of_fdt.h> #include <linux/smp.h>

-int acpi_noirq; /* skip ACPI IRQ initialization */ -int acpi_disabled; +int acpi_noirq = 1; /* skip ACPI IRQ initialization */ +int acpi_disabled = 1; EXPORT_SYMBOL(acpi_disabled);

-int acpi_pci_disabled; /* skip ACPI PCI scan and IRQ initialization */ +int acpi_pci_disabled = 1; /* skip ACPI PCI scan and IRQ initialization */ EXPORT_SYMBOL(acpi_pci_disabled);

+static bool param_acpi_off __initdata; +static bool param_acpi_force __initdata;

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

•         param_acpi_off = true;
  

• else if (strcmp(arg, "force") == 0) /* force ACPI to be enabled */
  

•         param_acpi_force = true;
  

• else
  

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

• return 0;
  

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

+static int __init dt_scan_depth1_nodes(unsigned long node,

•                                const char *uname, int depth,
  

•                                void *data)
  

+{

• /*
  

•  * Return 1 as soon as we encounter a node at depth 1 that is
  

•  * not the /chosen node.
  

•  */
  

• if (depth == 1 && (strcmp(uname, "chosen") != 0))
  

•         return 1;
  

• return 0;
  

+}

/*

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

@@ -83,10 +117,18 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table) */ void __init acpi_boot_table_init(void) {

• /* If acpi_disabled, bail out */
  

• if (acpi_disabled)
  

• /*
  

•  * Enable ACPI instead of device tree unless
  

•  * - ACPI has been disabled explicitly (acpi=off), or
  

•  * - the device tree is not empty (it has more than just a /chosen node)
  

•  *   and ACPI has not been force enabled (acpi=force)
  

•  */
  

• if (param_acpi_off ||
  

•     (!param_acpi_force && of_scan_flat_dt(dt_scan_depth1_nodes, NULL)))
          return;
  
  

• enable_acpi();
  

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

-- 1.9.1

On Wed, Mar 18, 2015 at 08:07:09PM +0000, Ard Biesheuvel wrote:

On 18 March 2015 at 12:35, Lorenzo Pieralisi lorenzo.pieralisi@arm.com wrote:

...

On Wed, Mar 11, 2015 at 12:39:34PM +0000, Hanjun Guo wrote:

...

From: Al Stone al.stone@linaro.org

This implements the following policy to decide whether ACPI should be used to boot the system:

• acpi=off: ACPI will not be used to boot the system, even if there is no alternative available (e.g., device tree is empty)
• acpi=force: only ACPI will be used to boot the system; if that fails, there will be no fallback to alternative methods (such as device tree)

I think this comment is stale. acpi=force enables ACPI and tries to init the ACPI tables without even checking DT, but it does fall back to DT if ACPI table init fails (by disabling ACPI and unflattening the FDT).

Am I wrong ?

No, you're right. But I would suggest that we fix the code, not the comment.

So would I. I flagged this up on the comment since I was not able to follow the thread on arm64 acpi=force and thought I was missing something.

I think we are all in agreement on the policy, we only need to make disable_acpi() conditional on whether acpi_param_force is set

Either this or Catalin's fix, in actual terms the end result should be the same on arm64, leaving acpi_disabled will save us some pointless parsing IMO.

Nit: "acpi" kernel parameter description defines

"force -- enable ACPI if default was off"

which is not what we do on arm64 if we leave ACPI disabled when acpi=force and ACPI fails to init.

I do not think we should care, if anyone disagrees manifest yourselves.

Lorenzo

...

...

• otherwise, ACPI will be used as a fallback if the device tree turns out to lack a platform description; the heuristic to decide this is whether /chosen is the only node present at depth 1

CC: Catalin Marinas catalin.marinas@arm.com CC: Will Deacon will.deacon@arm.com CC: Rafael J. Wysocki rjw@rjwysocki.net Acked-by: Olof Johansson olof@lixom.net Acked-by: Grant Likely grant.likely@linaro.org Tested-by: Timur Tabi timur@codeaurora.org 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 Signed-off-by: Ard Biesheuvel ard.biesheuvel@linaro.org

---

Documentation/kernel-parameters.txt | 3 ++- arch/arm64/include/asm/acpi.h | 7 +++++ arch/arm64/kernel/acpi.c | 52 +++++++++++++++++++++++++++++++++---- 3 files changed, 56 insertions(+), 6 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt index bfcb1a6..d6c35a7 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,ARM64]
                  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" or "acpi=force" are available
  
                  See also Documentation/power/runtime_pm.txt, pci=noacpi
  
  

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index 40e0924..c5a9b97 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -39,6 +39,13 @@ static inline void disable_acpi(void) acpi_noirq = 1; }

+static inline void enable_acpi(void) +{

• acpi_disabled = 0;
  

• acpi_pci_disabled = 0;
  

• acpi_noirq = 0;
  

+}

/*

• 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 7abac24..2269e30 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -22,15 +22,49 @@ #include <linux/irq.h> #include <linux/irqdomain.h> #include <linux/memblock.h> +#include <linux/of_fdt.h> #include <linux/smp.h>

-int acpi_noirq; /* skip ACPI IRQ initialization */ -int acpi_disabled; +int acpi_noirq = 1; /* skip ACPI IRQ initialization */ +int acpi_disabled = 1; EXPORT_SYMBOL(acpi_disabled);

-int acpi_pci_disabled; /* skip ACPI PCI scan and IRQ initialization */ +int acpi_pci_disabled = 1; /* skip ACPI PCI scan and IRQ initialization */ EXPORT_SYMBOL(acpi_pci_disabled);

+static bool param_acpi_off __initdata; +static bool param_acpi_force __initdata;

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

•         param_acpi_off = true;
  

• else if (strcmp(arg, "force") == 0) /* force ACPI to be enabled */
  

•         param_acpi_force = true;
  

• else
  

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

• return 0;
  

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

+static int __init dt_scan_depth1_nodes(unsigned long node,

•                                const char *uname, int depth,
  

•                                void *data)
  

+{

• /*
  

•  * Return 1 as soon as we encounter a node at depth 1 that is
  

•  * not the /chosen node.
  

•  */
  

• if (depth == 1 && (strcmp(uname, "chosen") != 0))
  

•         return 1;
  

• return 0;
  

+}

/*

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

@@ -83,10 +117,18 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table) */ void __init acpi_boot_table_init(void) {

• /* If acpi_disabled, bail out */
  

• if (acpi_disabled)
  

• /*
  

•  * Enable ACPI instead of device tree unless
  

•  * - ACPI has been disabled explicitly (acpi=off), or
  

•  * - the device tree is not empty (it has more than just a /chosen node)
  

•  *   and ACPI has not been force enabled (acpi=force)
  

•  */
  

• if (param_acpi_off ||
  

•     (!param_acpi_force && of_scan_flat_dt(dt_scan_depth1_nodes, NULL)))
          return;
  
  

• enable_acpi();
  

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

-- 1.9.1

On Wed, Mar 11, 2015 at 08:39:35PM +0800, Hanjun Guo wrote:

From: Graeme Gregory graeme.gregory@linaro.org

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

CC: Catalin Marinas catalin.marinas@arm.com CC: Will Deacon will.deacon@arm.com Tested-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Tested-by: Yijing Wang wangyijing@huawei.com Tested-by: Mark Langsdorf mlangsdo@redhat.com Tested-by: Jon Masters jcm@redhat.com Tested-by: Timur Tabi timur@codeaurora.org Tested-by: Robert Richter rrichter@cavium.com Acked-by: Robert Richter rrichter@cavium.com Acked-by: Olof Johansson olof@lixom.net Acked-by: Grant Likely grant.likely@linaro.org Signed-off-by: Graeme Gregory graeme.gregory@linaro.org Signed-off-by: Hanjun Guo hanjun.guo@linaro.org

---

arch/arm64/kernel/setup.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index 4f1a014..e8c7000 100644 --- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -390,7 +390,8 @@ void __init setup_arch(char **cmdline_p) early_ioremap_reset();

• unflatten_device_tree();

• if (acpi_disabled)

• unflatten_device_tree();
  

Lorenzo commented on the previous patch that acpi=force still allows the fall back to DT. I guess you may want something like:

if (acpi_disabled && !param_acpi_force) unflatten_device_tree();

On Wed, Mar 11, 2015 at 12:39:36PM +0000, Hanjun Guo wrote:

[...]

+static void __init 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.

@@ -306,29 +335,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();

You should have factored out the firmware version probing too, that's the only way we can detect the PSCI firmware version when booting through ACPI. You can end up initializing pointers for v0.2+ with a mismatching version implemented in PSCI firmware, eg 0.1.

We should do that incrementally, I will put together a patch to factor out the FW version probing first, you can rebase on top of it.

out_put_node: of_node_put(np); @@ -381,7 +388,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; @@ -396,6 +403,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;
  

• }

If PSCI is not present, that's a problem related to SMP init, right ? That's where a warning should be printed if any, not here, the SBBR mandates PSCI as secondaries bring up method, warn otherwise.

Lorenzo

On Mon, Mar 16, 2015 at 11:45:25AM +0000, Hanjun Guo wrote:

On 2015年03月13日 22:51, Lorenzo Pieralisi wrote:

...

On Wed, Mar 11, 2015 at 12:39:36PM +0000, Hanjun Guo wrote:

[...]

...

+static void __init 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.

@@ -306,29 +335,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();

You should have factored out the firmware version probing too, that's the only way we can detect the PSCI firmware version when booting through ACPI. You can end up initializing pointers for v0.2+ with a mismatching version implemented in PSCI firmware, eg 0.1.

We should do that incrementally, I will put together a patch to factor out the FW version probing first, you can rebase on top of it.

Incrementally patches on top of this patch set? I think v10 of this patch set is ready for merge, but I'm open for suggestions if we will not miss the merge window for Catalin.

I gave you my suggestion, I will try to get the prerequisite patch queued asap, it is not a big deal but that's something that should be fixed otherwise I would not have flagged this up.

I will post the patch asap, if we fail to get that in we will see what to do, I do not expect this to be a blocking point.

...

...

out_put_node: of_node_put(np); @@ -381,7 +388,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; @@ -396,6 +403,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;
  

• }

If PSCI is not present, that's a problem related to SMP init, right ? That's where a warning should be printed if any, not here, the SBBR mandates PSCI as secondaries bring up method, warn otherwise.

The SBBR is also said that if PSCI is not available, Parking protocol will be used as secondaries bring up method, so I said that it is ok to me that we don't print warn message for no PSCI support when parsing FADT.

So maybe we can go back to the previous solution, print some warning message if no PSCI when parsing FADT?

You answered your own question. It is not what it is mandated, but if a platform boots with parking protocol, do you think the information you are printing in:

if (!acpi_psci_present()) { pr_info("is not implemented in ACPI.\n"); ^^^

is useful to them ?

What should be flagged up is a missing boot method for secondaries, a missing PSCI is not per-se an error, that's why I said it should be done when preparing CPUs for SMP init.

No big deal at all, but I would remove the pr_info above.

Lorenzo

On Wed, Mar 11, 2015 at 12:39:39PM +0000, Hanjun Guo wrote:

CPU hardware ID (phys_id) is defined as u32 in structure acpi_processor, but phys_id is used as int in acpi processor driver, so it will lead to some inconsistence for the drivers.

Furthermore, to cater for ACPI arch ports that implement 64 bits CPU ids a generic CPU physical id type is required.

So introduce typedef u32 phys_cpuid_t for x86 and ia64, and introduce a macro CPU_PHYS_ID_INVALID as (u32)(-1), use phys_cpuid_t when phys_id defined in acpi processor driver, and replace CPU_PHYS_ID_INVALID as -1 for phys_id, this will solve the inconsistence in acpi processor driver, and will prepare for the ACPI on ARM64 for the 64 bit CPU hardware ID in the following patch.

Nit: Commit log above is stale (since you moved the phys_cpuid_t to common declaration for all archs that do not define PHYS_CPUID_INVALID and the macro name itself changed) so you should rewrite it before asking to merge it. Furthermore you should not describe how you implemented the change, you should describe why.

Other than that:

Acked-by: Lorenzo Pieralisi lorenzo.pieralisi@arm.com

CC: Rafael J Wysocki rjw@rjwysocki.net Suggested-by: Lorenzo Pieralisi lorenzo.pieralisi@arm.com Reviewed-by: Grant Likely grant.likely@linaro.org Acked-by: Sudeep Holla sudeep.holla@arm.com Signed-off-by: Catalin Marinas catalin.marinas@arm.com [hj: reworked cpu physid map return codes] Signed-off-by: Hanjun Guo hanjun.guo@linaro.org

---

arch/ia64/kernel/acpi.c | 2 +- arch/x86/kernel/acpi/boot.c | 2 +- drivers/acpi/acpi_processor.c | 7 ++++--- drivers/acpi/processor_core.c | 30 +++++++++++++++--------------- include/acpi/processor.h | 6 +++--- include/linux/acpi.h | 7 ++++++- 6 files changed, 30 insertions(+), 24 deletions(-)

diff --git a/arch/ia64/kernel/acpi.c b/arch/ia64/kernel/acpi.c index 2c44989..067ef44 100644 --- a/arch/ia64/kernel/acpi.c +++ b/arch/ia64/kernel/acpi.c @@ -887,7 +887,7 @@ static int _acpi_map_lsapic(acpi_handle handle, int physid, int *pcpu) } /* wrapper to silence section mismatch warning */ -int __ref acpi_map_cpu(acpi_handle handle, int physid, int *pcpu) +int __ref acpi_map_cpu(acpi_handle handle, phys_cpuid_t physid, int *pcpu) { return _acpi_map_lsapic(handle, physid, pcpu); } diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c index 3d525c6..e4f8582 100644 --- a/arch/x86/kernel/acpi/boot.c +++ b/arch/x86/kernel/acpi/boot.c @@ -757,7 +757,7 @@ static int _acpi_map_lsapic(acpi_handle handle, int physid, int *pcpu) } /* wrapper to silence section mismatch warning */ -int __ref acpi_map_cpu(acpi_handle handle, int physid, int *pcpu) +int __ref acpi_map_cpu(acpi_handle handle, phys_cpuid_t physid, int *pcpu) { return _acpi_map_lsapic(handle, physid, pcpu); } diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c index 1020b1b..58f335c 100644 --- a/drivers/acpi/acpi_processor.c +++ b/drivers/acpi/acpi_processor.c @@ -170,7 +170,7 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr) acpi_status status; int ret;

• if (pr->phys_id == -1)

• if (pr->phys_id == PHYS_CPUID_INVALID) return -ENODEV;

status = acpi_evaluate_integer(pr->handle, "_STA", NULL, &sta); @@ -215,7 +215,8 @@ static int acpi_processor_get_info(struct acpi_device *device) union acpi_object object = { 0 }; struct acpi_buffer buffer = { sizeof(union acpi_object), &object }; struct acpi_processor *pr = acpi_driver_data(device);

• int phys_id, cpu_index, device_declaration = 0;

• phys_cpuid_t phys_id;
• int cpu_index, device_declaration = 0; acpi_status status = AE_OK; static int cpu0_initialized; unsigned long long value;

@@ -263,7 +264,7 @@ static int acpi_processor_get_info(struct acpi_device *device) } phys_id = acpi_get_phys_id(pr->handle, device_declaration, pr->acpi_id);

• if (phys_id < 0)

• if (phys_id == PHYS_CPUID_INVALID) acpi_handle_debug(pr->handle, "failed to get CPU physical ID.\n"); pr->phys_id = phys_id;

diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c index 7962651..51cc299 100644 --- a/drivers/acpi/processor_core.c +++ b/drivers/acpi/processor_core.c @@ -32,7 +32,7 @@ static struct acpi_table_madt *get_madt_table(void) } static int map_lapic_id(struct acpi_subtable_header *entry,

•  u32 acpi_id, int *apic_id)
  

•  u32 acpi_id, phys_cpuid_t *apic_id)
  

{ struct acpi_madt_local_apic *lapic = container_of(entry, struct acpi_madt_local_apic, header); @@ -48,7 +48,7 @@ static int map_lapic_id(struct acpi_subtable_header *entry, } static int map_x2apic_id(struct acpi_subtable_header *entry,

• 	 int device_declaration, u32 acpi_id, int *apic_id)
  

• int device_declaration, u32 acpi_id, phys_cpuid_t *apic_id)
  

{ struct acpi_madt_local_x2apic *apic = container_of(entry, struct acpi_madt_local_x2apic, header); @@ -65,7 +65,7 @@ static int map_x2apic_id(struct acpi_subtable_header *entry, } static int map_lsapic_id(struct acpi_subtable_header *entry,

• int device_declaration, u32 acpi_id, int *apic_id)
  

• int device_declaration, u32 acpi_id, phys_cpuid_t *apic_id)
  

{ struct acpi_madt_local_sapic *lsapic = container_of(entry, struct acpi_madt_local_sapic, header); @@ -83,10 +83,10 @@ static int map_lsapic_id(struct acpi_subtable_header *entry, return 0; } -static int map_madt_entry(int type, u32 acpi_id) +static phys_cpuid_t map_madt_entry(int type, u32 acpi_id) { unsigned long madt_end, entry;

• int phys_id = -1; /* CPU hardware ID */

• phys_cpuid_t phys_id = PHYS_CPUID_INVALID; /* CPU hardware ID */ struct acpi_table_madt *madt;

madt = get_madt_table(); @@ -117,12 +117,12 @@ static int map_madt_entry(int type, u32 acpi_id) return phys_id; } -static int map_mat_entry(acpi_handle handle, int type, u32 acpi_id) +static phys_cpuid_t map_mat_entry(acpi_handle handle, int type, u32 acpi_id) { struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; union acpi_object *obj; struct acpi_subtable_header *header;

• int phys_id = -1;

• phys_cpuid_t phys_id = PHYS_CPUID_INVALID;

if (ACPI_FAILURE(acpi_evaluate_object(handle, "_MAT", NULL, &buffer))) goto exit; @@ -149,27 +149,27 @@ exit: return phys_id; } -int acpi_get_phys_id(acpi_handle handle, int type, u32 acpi_id) +phys_cpuid_t acpi_get_phys_id(acpi_handle handle, int type, u32 acpi_id) {

• int phys_id;

• phys_cpuid_t phys_id;

phys_id = map_mat_entry(handle, type, acpi_id);

• if (phys_id == -1)

• if (phys_id == PHYS_CPUID_INVALID) phys_id = map_madt_entry(type, acpi_id);

return phys_id; } -int acpi_map_cpuid(int phys_id, u32 acpi_id) +int acpi_map_cpuid(phys_cpuid_t phys_id, u32 acpi_id) { #ifdef CONFIG_SMP int i; #endif

• if (phys_id == -1) {

• if (phys_id == PHYS_CPUID_INVALID) { /*
  • On UP processor, there is no _MAT or MADT table.

•  * So above phys_id is always set to -1.
  

•  * So above phys_id is always set to PHYS_CPUID_INVALID.
  

  • BIOS may define multiple CPU handles even for UP processor.
  • For example,

@@ -190,7 +190,7 @@ int acpi_map_cpuid(int phys_id, u32 acpi_id) if (nr_cpu_ids <= 1 && acpi_id == 0) return acpi_id; else

• 	return phys_id;
  

• 	return -1;
  

  }

#ifdef CONFIG_SMP @@ -208,7 +208,7 @@ int acpi_map_cpuid(int phys_id, u32 acpi_id) int acpi_get_cpuid(acpi_handle handle, int type, u32 acpi_id) {

• int phys_id;

• phys_cpuid_t phys_id;

phys_id = acpi_get_phys_id(handle, type, acpi_id); diff --git a/include/acpi/processor.h b/include/acpi/processor.h index b95dc32..4188a4d 100644 --- a/include/acpi/processor.h +++ b/include/acpi/processor.h @@ -196,7 +196,7 @@ struct acpi_processor_flags { struct acpi_processor { acpi_handle handle; u32 acpi_id;

• u32 phys_id; /* CPU hardware ID such as APIC ID for x86 */

• phys_cpuid_t phys_id; /* CPU hardware ID such as APIC ID for x86 */ u32 id; /* CPU logical ID allocated by OS */ u32 pblk; int performance_platform_limit;

@@ -310,8 +310,8 @@ static inline int acpi_processor_get_bios_limit(int cpu, unsigned int *limit) #endif /* CONFIG_CPU_FREQ */ /* in processor_core.c */ -int acpi_get_phys_id(acpi_handle, int type, u32 acpi_id); -int acpi_map_cpuid(int phys_id, u32 acpi_id); +phys_cpuid_t acpi_get_phys_id(acpi_handle, int type, u32 acpi_id); +int acpi_map_cpuid(phys_cpuid_t phys_id, u32 acpi_id); int acpi_get_cpuid(acpi_handle, int type, u32 acpi_id); /* in processor_pdc.c */ diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 24c7aa8..6ec33c5 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -146,9 +146,14 @@ void acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa); int acpi_numa_memory_affinity_init (struct acpi_srat_mem_affinity *ma); void acpi_numa_arch_fixup(void); +#ifndef PHYS_CPUID_INVALID +typedef u32 phys_cpuid_t; +#define PHYS_CPUID_INVALID (phys_cpuid_t)(-1) +#endif

#ifdef CONFIG_ACPI_HOTPLUG_CPU /* Arch dependent functions for cpu hotplug support */ -int acpi_map_cpu(acpi_handle handle, int physid, int *pcpu); +int acpi_map_cpu(acpi_handle handle, phys_cpuid_t physid, int *pcpu); int acpi_unmap_cpu(int cpu); #endif /* CONFIG_ACPI_HOTPLUG_CPU */ -- 1.9.1

On Wednesday, March 11, 2015 08:39:40 PM 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, since MPIDR for ARM64 is 64-bit, so typedef u64 for phys_cpuid_t.

The ARM architecture defines the MPIDR register as the CPU hardware identifier. This patch adds the code infrastructure to retrieve the MPIDR values from the ARM ACPI GICC structure in order to look-up the kernel CPU hardware ids required by the ACPI core code to identify CPUs.

CC: Rafael J. Wysocki rjw@rjwysocki.net CC: Catalin Marinas catalin.marinas@arm.com CC: Will Deacon will.deacon@arm.com Tested-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Tested-by: Yijing Wang wangyijing@huawei.com Tested-by: Mark Langsdorf mlangsdo@redhat.com Tested-by: Jon Masters jcm@redhat.com Tested-by: Timur Tabi timur@codeaurora.org Tested-by: Robert Richter rrichter@cavium.com Acked-by: Robert Richter rrichter@cavium.com Reviewed-by: Grant Likely grant.likely@linaro.org Signed-off-by: Hanjun Guo hanjun.guo@linaro.org

Acked-by: Rafael J. Wysocki rafael.j.wysocki@intel.com

---

arch/arm64/include/asm/acpi.h | 12 ++++++++++++ drivers/acpi/processor_core.c | 30 ++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index 9719921..eea0bc3 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -13,6 +13,8 @@ #define _ASM_ACPI_H #include <linux/mm.h> +#include <asm/cputype.h> +#include <asm/smp_plat.h> /* Basic configuration for ACPI */ #ifdef CONFIG_ACPI @@ -27,6 +29,9 @@ static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys, } #define acpi_os_ioremap acpi_os_ioremap +typedef u64 phys_cpuid_t; +#define PHYS_CPUID_INVALID INVALID_HWID

#define acpi_strict 1 /* No out-of-spec workarounds on ARM64 */ extern int acpi_disabled; extern int acpi_noirq; @@ -59,6 +64,13 @@ static inline void enable_acpi(void) } /*

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

+#define cpu_physical_id(cpu) 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
• obtained from GICC with 0 and not print some error message as well.

diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c index 51cc299..b1ec78b 100644 --- a/drivers/acpi/processor_core.c +++ b/drivers/acpi/processor_core.c @@ -83,6 +83,31 @@ static int map_lsapic_id(struct acpi_subtable_header *entry, return 0; } +/*

• • Retrieve the ARM CPU physical identifier (MPIDR)
• */

+static int map_gicc_mpidr(struct acpi_subtable_header *entry,

• int device_declaration, u32 acpi_id, phys_cpuid_t *mpidr)
  

+{

• struct acpi_madt_generic_interrupt *gicc =

•    container_of(entry, struct acpi_madt_generic_interrupt, header);
  

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

• return -ENODEV;
  

• /* device_declaration means Device object in DSDT, 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)) {

• *mpidr = gicc->arm_mpidr;
  

• return 0;
  

• }
• return -EINVAL;

+}

static phys_cpuid_t map_madt_entry(int type, u32 acpi_id) { unsigned long madt_end, entry; @@ -111,6 +136,9 @@ static phys_cpuid_t 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, &phys_id)) break;

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

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

• 		break;
  

  } entry += header->length; }

@@ -143,6 +171,8 @@ static phys_cpuid_t map_mat_entry(acpi_handle handle, int type, u32 acpi_id) map_lsapic_id(header, type, acpi_id, &phys_id); else if (header->type == ACPI_MADT_TYPE_LOCAL_X2APIC) map_x2apic_id(header, type, acpi_id, &phys_id);

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

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

exit: kfree(buffer.pointer);

On Wed, Mar 11, 2015 at 12:39:41PM +0000, 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.

We are going to implement stacked domains when GICv2m, GICv3, ITS support are added.

CC: Marc Zyngier marc.zyngier@arm.com Originally-by: Amit Daniel Kachhap amit.daniel@samsung.com Tested-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Tested-by: Yijing Wang wangyijing@huawei.com Tested-by: Mark Langsdorf mlangsdo@redhat.com Tested-by: Jon Masters jcm@redhat.com Tested-by: Timur Tabi timur@codeaurora.org Tested-by: Robert Richter rrichter@cavium.com Acked-by: Robert Richter rrichter@cavium.com Acked-by: Rafael J. Wysocki rafael.j.wysocki@intel.com Reviewed-by: Grant Likely grant.likely@linaro.org 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 c9203c0..dec6f8a 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -76,6 +76,12 @@ static int __init dt_scan_depth1_nodes(unsigned long node, } /*

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

*/ @@ -218,6 +224,73 @@ void __init acpi_init_cpus(void) pr_info("%d CPUs enabled, %d CPUs total\n", enabled_cpus, total_cpus); } +int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) +{

• *irq = irq_find_mapping(NULL, gsi);
• return 0;

+} +EXPORT_SYMBOL_GPL(acpi_gsi_to_irq);

+/*

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

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

• unsigned int irq;
• unsigned int irq_type;
• /*

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

• * use different mappings from DT in ACPI.
  

• *
  

• * For FDT
  

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

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

• *
  

• * For ACPI, GSI should be unique so using
  

• * the hwirq directly for the mapping:
  

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

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

• */
  

• if (trigger == ACPI_EDGE_SENSITIVE &&

• 		polarity == ACPI_ACTIVE_LOW)
  

• irq_type = IRQ_TYPE_EDGE_FALLING;
  

• else if (trigger == ACPI_EDGE_SENSITIVE &&

• 		polarity == ACPI_ACTIVE_HIGH)
  

• irq_type = IRQ_TYPE_EDGE_RISING;
  

• else if (trigger == ACPI_LEVEL_SENSITIVE &&

• 		polarity == ACPI_ACTIVE_LOW)
  

• irq_type = IRQ_TYPE_LEVEL_LOW;
  

• else if (trigger == ACPI_LEVEL_SENSITIVE &&

• 		polarity == ACPI_ACTIVE_HIGH)
  

• irq_type = IRQ_TYPE_LEVEL_HIGH;
  

• else

• irq_type = IRQ_TYPE_NONE;
  

• /*

• * Since only one GIC is supported in ACPI 5.0, we can
  

• * create mapping refer to the default domain
  

• */
  

• irq = irq_create_mapping(NULL, gsi);
• if (!irq)

• return irq;
  

• /* Set irq type if specified and different than the current one */
• if (irq_type != IRQ_TYPE_NONE &&

• irq_type != irq_get_trigger_type(irq))
  

• irq_set_irq_type(irq, irq_type);
  

• return irq;

+} +EXPORT_SYMBOL_GPL(acpi_register_gsi);

I see you've still got this buried in the arch code. Is there any plan to move it out, as I moaned about this in the last version of the series and nothing seems to have changed?

Will

On 2015/3/20 3:37, Will Deacon wrote:

On Thu, Mar 19, 2015 at 10:12:05AM +0000, Lorenzo Pieralisi wrote:

...

On Thu, Mar 19, 2015 at 03:45:35AM +0000, Hanjun Guo wrote:

...

...

...

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

I see you've still got this buried in the arch code. Is there any plan to move it out, as I moaned about this in the last version of the series and nothing seems to have changed?

Ah, sorry. Last time when I was in Hongkong for LCA this Feb, I discussed with Lorenzo and he had a look into that too, he also met some obstacles to do that, so Lorenzo said that he will talk to you about this (Lorenzo, correct me if I'm wrong due to hearing problems of much noise in that room where we were talking).

Anyway, if we move those functions to core code, such as irqdomain code, which will be compiled for x86 too, we can only set those functions as _weak, or we guard with them as #ifdef CONFIG_ARM64 ... #endif, so for me, it's really not a big deal to move those code out of arch/arm64, but I'm still open for suggestions if you can do that in a proper way.

You heard me clear and sound in HK, Will has a point and I looked into this. Code is generic but not enough to be useful on other arches at the moment, I need more time to look into this and see if we can move this code to acpi core in a way that makes sense, to have, as you say, a "default" implementation.

Yeah, just something guarded by a CONFIG option (probably not ARM64 though) would be enough, I think. Nothing too fancy.

Hi Will,

It is ARM64 related code and ACPI specific, I can come up with following code:

arch/arm64/kernel/acpi.c | 67 --------------------------------------------- kernel/irq/irqdomain.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 67 deletions(-)

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 5819ef7..d207544 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -224,73 +224,6 @@ void __init acpi_init_cpus(void) pr_info("%d CPUs enabled, %d CPUs total\n", enabled_cpus, total_cpus); }

-int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) -{ - *irq = irq_find_mapping(NULL, gsi); - - return 0; -} -EXPORT_SYMBOL_GPL(acpi_gsi_to_irq); - -/* - * success: return IRQ number (>0) - * failure: return =< 0 - */ -int acpi_register_gsi(struct device *dev, u32 gsi, int trigger, int polarity) -{ - unsigned int irq; - unsigned int irq_type; - - /* - * ACPI have no bindings to indicate SPI or PPI, so we - * use different mappings from DT in ACPI. - * - * For FDT - * PPI interrupt: in the range [0, 15]; - * SPI interrupt: in the range [0, 987]; - * - * For ACPI, GSI should be unique so using - * the hwirq directly for the mapping: - * PPI interrupt: in the range [16, 31]; - * SPI interrupt: in the range [32, 1019]; - */ - - if (trigger == ACPI_EDGE_SENSITIVE && - polarity == ACPI_ACTIVE_LOW) - irq_type = IRQ_TYPE_EDGE_FALLING; - else if (trigger == ACPI_EDGE_SENSITIVE && - polarity == ACPI_ACTIVE_HIGH) - irq_type = IRQ_TYPE_EDGE_RISING; - else if (trigger == ACPI_LEVEL_SENSITIVE && - polarity == ACPI_ACTIVE_LOW) - irq_type = IRQ_TYPE_LEVEL_LOW; - else if (trigger == ACPI_LEVEL_SENSITIVE && - polarity == ACPI_ACTIVE_HIGH) - irq_type = IRQ_TYPE_LEVEL_HIGH; - else - irq_type = IRQ_TYPE_NONE; - - /* - * Since only one GIC is supported in ACPI 5.0, we can - * create mapping refer to the default domain - */ - irq = irq_create_mapping(NULL, gsi); - if (!irq) - return irq; - - /* Set irq type if specified and different than the current one */ - if (irq_type != IRQ_TYPE_NONE && - irq_type != irq_get_trigger_type(irq)) - irq_set_irq_type(irq, irq_type); - return irq; -} -EXPORT_SYMBOL_GPL(acpi_register_gsi); - -void acpi_unregister_gsi(u32 gsi) -{ -} -EXPORT_SYMBOL_GPL(acpi_unregister_gsi); - static int __init acpi_parse_fadt(struct acpi_table_header *table) { struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table; diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c index 7fac311..5a614bc 100644 --- a/kernel/irq/irqdomain.c +++ b/kernel/irq/irqdomain.c @@ -1,5 +1,6 @@ #define pr_fmt(fmt) "irq: " fmt

+#include <linux/acpi.h> #include <linux/debugfs.h> #include <linux/hardirq.h> #include <linux/interrupt.h> @@ -568,6 +569,75 @@ unsigned int irq_find_mapping(struct irq_domain *domain, } EXPORT_SYMBOL_GPL(irq_find_mapping);

+#if defined(CONFIG_ARM64) && defined(CONFIG_ACPI) +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.1, we can + * create mapping refer to the default domain + */ + irq = irq_create_mapping(NULL, gsi); + if (!irq) + return irq; + + /* Set irq type if specified and different than the current one */ + if (irq_type != IRQ_TYPE_NONE && + irq_type != irq_get_trigger_type(irq)) + irq_set_irq_type(irq, irq_type); + return irq; +} +EXPORT_SYMBOL_GPL(acpi_register_gsi); + +void acpi_unregister_gsi(u32 gsi) +{ +} +EXPORT_SYMBOL_GPL(acpi_unregister_gsi); +#endif /* CONFIG_ARM64 && CONFIG_ACPI */ + #ifdef CONFIG_IRQ_DOMAIN_DEBUG static int virq_debug_show(struct seq_file *m, void *private) {

is this the way you prefered?

Thanks Hanjun

On 11 Mar 2015 12:42, "Hanjun Guo" hanjun.guo@linaro.org wrote:

From: Tomasz Nowicki tomasz.nowicki@linaro.org

ACPI kernel uses MADT table for proper GIC initialization. It needs to parse GIC related subtables, collect CPU interface and distributor addresses and call driver initialization function (which is hardware abstraction agnostic). In a similar way, FDT initialize GICv1/2.

NOTE: This commit allow to initialize GICv1/2 basic functionality. While now simple GICv2 init call is used, any further GIC features require generic infrastructure for proper ACPI irqchip initialization. That mechanism and stacked irqdomains to support GICv2 MSI/virtualization extension, GICv3/4 and its ITS are considered as next steps.

CC: Jason Cooper jason@lakedaemon.net CC: Marc Zyngier marc.zyngier@arm.com CC: Thomas Gleixner tglx@linutronix.de

BTW, Thomas is taking a bit of a break, do he is unlikely to give an ack here in a timely manner. I've not heard from Jason. Personally, I think we can proceed without their ack if everything else is in order (heck, I used to help with the irq subsystem, use me as an ack of you want). The patch is low impact and only had effect for ARM ACPI builds.

g.

Tested-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Tested-by: Yijing Wang wangyijing@huawei.com Tested-by: Mark Langsdorf mlangsdo@redhat.com Tested-by: Jon Masters jcm@redhat.com Tested-by: Timur Tabi timur@codeaurora.org Tested-by: Robert Richter rrichter@cavium.com Acked-by: Robert Richter rrichter@cavium.com Reviewed-by: Grant Likely grant.likely@linaro.org Signed-off-by: Tomasz Nowicki tomasz.nowicki@linaro.org Signed-off-by: Hanjun Guo hanjun.guo@linaro.org

---

arch/arm64/include/asm/acpi.h | 2 + arch/arm64/include/asm/irq.h | 13 +++++ arch/arm64/kernel/acpi.c | 25 +++++++++ drivers/irqchip/irq-gic.c | 102

+++++++++++++++++++++++++++++++++++

drivers/irqchip/irqchip.c | 3 ++ include/linux/acpi_irq.h | 10 ++++ include/linux/irqchip/arm-gic-acpi.h | 31 +++++++++++ 7 files changed, 186 insertions(+) create mode 100644 include/linux/acpi_irq.h create mode 100644 include/linux/irqchip/arm-gic-acpi.h

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index eea0bc3..59c05d8 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -13,6 +13,8 @@ #define _ASM_ACPI_H

#include <linux/mm.h> +#include <linux/irqchip/arm-gic-acpi.h>

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

diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h index 94c5367..bbb251b 100644 --- a/arch/arm64/include/asm/irq.h +++ b/arch/arm64/include/asm/irq.h @@ -1,6 +1,8 @@ #ifndef __ASM_IRQ_H #define __ASM_IRQ_H

+#include <linux/irqchip/arm-gic-acpi.h>

#include <asm-generic/irq.h>

struct pt_regs; @@ -8,4 +10,15 @@ struct pt_regs; extern void migrate_irqs(void); extern void set_handle_irq(void (*handle_irq)(struct pt_regs *));

+static inline void acpi_irq_init(void) +{

•   /*
  

•    * Hardcode ACPI IRQ chip initialization to GICv2 for now.
  

•    * Proper irqchip infrastructure will be implemented along with
  

•    * incoming  GICv2m|GICv3|ITS bits.
  

•    */
  

•   acpi_gic_init();
  

+} +#define acpi_irq_init acpi_irq_init

#endif diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index dec6f8a..6468f88 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -359,3 +359,28 @@ void __init acpi_boot_table_init(void) pr_err("Can't find FADT\n"); } }

+void __init acpi_gic_init(void) +{

•   struct acpi_table_header *table;
  

•   acpi_status status;
  

•   acpi_size tbl_size;
  

•   int err;
  

•   if (acpi_disabled)
  

•           return;
  

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

+} diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index 4634cf7..929d668 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -33,12 +33,14 @@ #include <linux/of.h> #include <linux/of_address.h> #include <linux/of_irq.h> +#include <linux/acpi.h> #include <linux/irqdomain.h> #include <linux/interrupt.h> #include <linux/percpu.h> #include <linux/slab.h> #include <linux/irqchip/chained_irq.h> #include <linux/irqchip/arm-gic.h> +#include <linux/irqchip/arm-gic-acpi.h>

#include <asm/cputype.h> #include <asm/irq.h> @@ -1086,3 +1088,103 @@ IRQCHIP_DECLARE(msm_8660_qgic,

"qcom,msm-8660-qgic", gic_of_init);

IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init);

#endif

+#ifdef CONFIG_ACPI +static phys_addr_t dist_phy_base, cpu_phy_base __initdata;

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

•                   const unsigned long end)
  

+{

•   struct acpi_madt_generic_interrupt *processor;
  

•   phys_addr_t gic_cpu_base;
  

•   static int cpu_base_assigned;
  

•   processor = (struct acpi_madt_generic_interrupt *)header;
  

•   if (BAD_MADT_ENTRY(processor, end))
  

•           return -EINVAL;
  

•   /*
  

•    * There is no support for non-banked GICv1/2 register in ACPI
  

spec.

•    * All CPU interface addresses have to be the same.
  

•    */
  

•   gic_cpu_base = processor->base_address;
  

•   if (cpu_base_assigned && gic_cpu_base != cpu_phy_base)
  

•           return -EINVAL;
  

•   cpu_phy_base = gic_cpu_base;
  

•   cpu_base_assigned = 1;
  

•   return 0;
  

+}

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

•                           const unsigned long end)
  

+{

•   struct acpi_madt_generic_distributor *dist;
  

•   dist = (struct acpi_madt_generic_distributor *)header;
  

•   if (BAD_MADT_ENTRY(dist, end))
  

•           return -EINVAL;
  

•   dist_phy_base = dist->base_address;
  

•   return 0;
  

+}

+int __init +gic_v2_acpi_init(struct acpi_table_header *table) +{

•   void __iomem *cpu_base, *dist_base;
  

•   int count;
  

•   /* Collect CPU base addresses */
  

•   count = acpi_parse_entries(ACPI_SIG_MADT,
  

•                              sizeof(struct acpi_table_madt),
  

•                              gic_acpi_parse_madt_cpu, table,
  

•                              ACPI_MADT_TYPE_GENERIC_INTERRUPT, 0);
  

•   if (count <= 0) {
  

•           pr_err("No valid GICC entries exist\n");
  

•           return -EINVAL;
  

•   }
  

•   /*
  

•    * Find distributor base address. We expect one distributor entry
  

since

•    * ACPI 5.1 spec neither support multi-GIC instances nor GIC
  

cascade.

•    */
  

•   count = acpi_parse_entries(ACPI_SIG_MADT,
  

•                              sizeof(struct acpi_table_madt),
  

•                              gic_acpi_parse_madt_distributor, table,
  

•                              ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR, 0);
  

•   if (count <= 0) {
  

•           pr_err("No valid GICD entries exist\n");
  

•           return -EINVAL;
  

•   } else if (count > 1) {
  

•           pr_err("More than one GICD entry detected\n");
  

•           return -EINVAL;
  

•   }
  

•   cpu_base = ioremap(cpu_phy_base, ACPI_GIC_CPU_IF_MEM_SIZE);
  

•   if (!cpu_base) {
  

•           pr_err("Unable to map GICC registers\n");
  

•           return -ENOMEM;
  

•   }
  

•   dist_base = ioremap(dist_phy_base, ACPI_GICV2_DIST_MEM_SIZE);
  

•   if (!dist_base) {
  

•           pr_err("Unable to map GICD registers\n");
  

•           iounmap(cpu_base);
  

•           return -ENOMEM;
  

•   }
  

•   /*
  

•    * Initialize zero GIC instance (no multi-GIC support). Also, set
  

GIC

•    * as default IRQ domain to allow for GSI registration and GSI to
  

IRQ

•    * number translation (see acpi_register_gsi() and
  

acpi_gsi_to_irq()).

•    */
  

•   gic_init_bases(0, -1, dist_base, cpu_base, 0, NULL);
  

•   irq_set_default_host(gic_data[0].domain);
  

•   return 0;
  

+} +#endif diff --git a/drivers/irqchip/irqchip.c b/drivers/irqchip/irqchip.c index 0fe2f71..afd1af3 100644 --- a/drivers/irqchip/irqchip.c +++ b/drivers/irqchip/irqchip.c @@ -8,6 +8,7 @@

• warranty of any kind, whether express or implied.

*/

+#include <linux/acpi_irq.h> #include <linux/init.h> #include <linux/of_irq.h> #include <linux/irqchip.h> @@ -26,4 +27,6 @@ extern struct of_device_id __irqchip_of_table[]; void __init irqchip_init(void) { of_irq_init(__irqchip_of_table);

•   acpi_irq_init();
  

} diff --git a/include/linux/acpi_irq.h b/include/linux/acpi_irq.h new file mode 100644 index 0000000..e4e8a81 --- /dev/null +++ b/include/linux/acpi_irq.h @@ -0,0 +1,10 @@ +#ifndef _LINUX_ACPI_IRQ_H +#define _LINUX_ACPI_IRQ_H

+#include <asm/irq.h>

+#ifndef acpi_irq_init +static inline void acpi_irq_init(void) { } +#endif

+#endif /* _LINUX_ACPI_IRQ_H */ diff --git a/include/linux/irqchip/arm-gic-acpi.h

b/include/linux/irqchip/arm-gic-acpi.h

new file mode 100644 index 0000000..de3419e --- /dev/null +++ b/include/linux/irqchip/arm-gic-acpi.h @@ -0,0 +1,31 @@ +/*

• • Copyright (C) 2014, Linaro Ltd.

• • Author: Tomasz Nowicki <tomasz.nowicki@linaro.org>
    

• • This program is free software; you can redistribute it and/or modify
• • it under the terms of the GNU General Public License version 2 as
• • published by the Free Software Foundation.
• */

+#ifndef ARM_GIC_ACPI_H_ +#define ARM_GIC_ACPI_H_

+#ifdef CONFIG_ACPI

+/*

• • Hard code here, we can not get memory size from MADT (but FDT does),
• • Actually no need to do that, because this size can be inferred
• • from GIC spec.
• */

+#define ACPI_GICV2_DIST_MEM_SIZE (SZ_4K) +#define ACPI_GIC_CPU_IF_MEM_SIZE (SZ_8K)

+struct acpi_table_header;

+int gic_v2_acpi_init(struct acpi_table_header *table); +void acpi_gic_init(void); +#else +static inline void acpi_gic_init(void) { } +#endif

+#endif /* ARM_GIC_ACPI_H_ */

1.9.1

On 2015/3/12 7:11, Jason Cooper wrote:

Hey Grant,

On Wed, Mar 11, 2015 at 06:04:50PM +0000, Grant Likely wrote:

...

On 11 Mar 2015 12:42, "Hanjun Guo" hanjun.guo@linaro.org wrote:

...

From: Tomasz Nowicki tomasz.nowicki@linaro.org

ACPI kernel uses MADT table for proper GIC initialization. It needs to parse GIC related subtables, collect CPU interface and distributor addresses and call driver initialization function (which is hardware abstraction agnostic). In a similar way, FDT initialize GICv1/2.

NOTE: This commit allow to initialize GICv1/2 basic functionality. While now simple GICv2 init call is used, any further GIC features require generic infrastructure for proper ACPI irqchip initialization. That mechanism and stacked irqdomains to support GICv2 MSI/virtualization extension, GICv3/4 and its ITS are considered as next steps.

CC: Jason Cooper jason@lakedaemon.net CC: Marc Zyngier marc.zyngier@arm.com CC: Thomas Gleixner tglx@linutronix.de

BTW, Thomas is taking a bit of a break, do he is unlikely to give an ack here in a timely manner. I've not heard from Jason. Personally, I think we can proceed without their ack if everything else is in order (heck, I used to help with the irq subsystem, use me as an ack of you want). The patch is low impact and only had effect for ARM ACPI builds.

I'm not talking much, but I am tracking and collecting everything for irqchip. We do have some other changes in this driver this time around. So it'd be nice if I could take this.

I had reached out to Olof for his thoughts on this and he hasn't had enough cycles to look at it. iirc, Marc reviewed a previous version and was happy with the changes. My only question I had for Olof I'll put below:

Please allow me to explain a little bit before Olof's confirmation, please don't mind if any offended.

...

...

diff --git a/drivers/irqchip/irqchip.c b/drivers/irqchip/irqchip.c index 0fe2f71..afd1af3 100644 --- a/drivers/irqchip/irqchip.c +++ b/drivers/irqchip/irqchip.c @@ -8,6 +8,7 @@

• warranty of any kind, whether express or implied.

*/

+#include <linux/acpi_irq.h> #include <linux/init.h> #include <linux/of_irq.h> #include <linux/irqchip.h> @@ -26,4 +27,6 @@ extern struct of_device_id __irqchip_of_table[]; void __init irqchip_init(void) { of_irq_init(__irqchip_of_table);

•   acpi_irq_init();
  

}

Is this in line with Olof's idea that providing a dtb would override ACPI?

Yes, it will. Since ACPI is default OFF (disabled), if a dtb provided, and no acpi=force passed in the early command line, dtb will be used as system configuration for boot (dtb is always the prior one for now) [1]. In acpi_gic_init() which called by acpi_irq_init(), it will return immediately if acpi disabled, so it will not parse any ACPI table for device configuration.

[1]: [patch 08/21] ARM64 / ACPI: Introduce early_param "acpi=" to enable/disable ACPI

Thanks Hanjun

On 2015/3/12 13:12, Jason Cooper wrote:

On Thu, Mar 12, 2015 at 09:46:39AM +0800, Hanjun Guo wrote:

...

On 2015/3/12 7:11, Jason Cooper wrote:

...

Hey Grant,

On Wed, Mar 11, 2015 at 06:04:50PM +0000, Grant Likely wrote:

...

On 11 Mar 2015 12:42, "Hanjun Guo" hanjun.guo@linaro.org wrote:

[...]

...

...

...

...

diff --git a/drivers/irqchip/irqchip.c b/drivers/irqchip/irqchip.c index 0fe2f71..afd1af3 100644 --- a/drivers/irqchip/irqchip.c +++ b/drivers/irqchip/irqchip.c @@ -8,6 +8,7 @@

• warranty of any kind, whether express or implied.

*/

+#include <linux/acpi_irq.h> #include <linux/init.h> #include <linux/of_irq.h> #include <linux/irqchip.h> @@ -26,4 +27,6 @@ extern struct of_device_id __irqchip_of_table[]; void __init irqchip_init(void) { of_irq_init(__irqchip_of_table);

•   acpi_irq_init();
  

}

Is this in line with Olof's idea that providing a dtb would override ACPI?

Yes, it will. Since ACPI is default OFF (disabled), if a dtb provided, and no acpi=force passed in the early command line, dtb will be used as system configuration for boot (dtb is always the prior one for now) [1]. In acpi_gic_init() which called by acpi_irq_init(), it will return immediately if acpi disabled, so it will not parse any ACPI table for device configuration.

Ok, that matches my recollection. Thanks for refreshing my memory. I'll apply this on a topic branch for irqchip/gic when I return from travel. Most likely Friday or over the weekend.

Thank you very much! But this patch can't be applied without previous ones in this patch set, how about you ack this patch and Catalin takes it via ARM64 tree? I'm not sure for this, it depends on your decision.

Thanks Hanjun

On Fri, Mar 13, 2015 at 5:15 PM, Jason Cooper jason@lakedaemon.net wrote:

Hanjun, Catalin,

On Thu, Mar 12, 2015 at 03:31:57PM +0800, Hanjun Guo wrote:

...

On 2015/3/12 13:12, Jason Cooper wrote:

...

On Thu, Mar 12, 2015 at 09:46:39AM +0800, Hanjun Guo wrote:

...

On 2015/3/12 7:11, Jason Cooper wrote:

...

Hey Grant,

On Wed, Mar 11, 2015 at 06:04:50PM +0000, Grant Likely wrote:

...

On 11 Mar 2015 12:42, "Hanjun Guo" hanjun.guo@linaro.org wrote:

[...]

...

...

...

...

> diff --git a/drivers/irqchip/irqchip.c b/drivers/irqchip/irqchip.c > index 0fe2f71..afd1af3 100644 > --- a/drivers/irqchip/irqchip.c > +++ b/drivers/irqchip/irqchip.c > @@ -8,6 +8,7 @@ > * warranty of any kind, whether express or implied. > */ > > +#include <linux/acpi_irq.h> > #include <linux/init.h> > #include <linux/of_irq.h> > #include <linux/irqchip.h> > @@ -26,4 +27,6 @@ extern struct of_device_id __irqchip_of_table[]; > void __init irqchip_init(void) > { > of_irq_init(__irqchip_of_table); > + > + acpi_irq_init(); > }

Is this in line with Olof's idea that providing a dtb would override ACPI?

Yes, it will. Since ACPI is default OFF (disabled), if a dtb provided, and no acpi=force passed in the early command line, dtb will be used as system configuration for boot (dtb is always the prior one for now) [1]. In acpi_gic_init() which called by acpi_irq_init(), it will return immediately if acpi disabled, so it will not parse any ACPI table for device configuration.

Ok, that matches my recollection. Thanks for refreshing my memory. I'll apply this on a topic branch for irqchip/gic when I return from travel. Most likely Friday or over the weekend.

Thank you very much! But this patch can't be applied without previous ones in this patch set, how about you ack this patch and Catalin takes it via ARM64 tree? I'm not sure for this, it depends on your decision.

Is this a build dependency or a boot dependency? I only received this patch in the series and I apologize, I'm a bit swamped atm. Catalin, would an immutable irqchip/gic topic branch with this in it work for you?

Jason,

For a series like this I strongly recommend you provide an ack and let the whole series go in via a single branch. Trying to split it up only to reassemble it again creates more work for everyone. There is also very little likelyhood that this will create a complex conflict with your tree.

g.

On Wed, Mar 11, 2015 at 12:39:44PM +0000, Hanjun Guo wrote:

From: Al Stone al.stone@linaro.org

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

If the firmware is not using hardware reduced ACPI mode, we will disable ACPI to avoid nightmare such as accessing some registers which are not available on ARM64.

CC: Catalin Marinas catalin.marinas@arm.com CC: Will Deacon will.deacon@arm.com Reviewed-by: Grant Likely grant.likely@linaro.org Tested-by: Suravee Suthikulpanit Suravee.Suthikulpanit@amd.com Tested-by: Yijing Wang wangyijing@huawei.com Tested-by: Mark Langsdorf mlangsdo@redhat.com Tested-by: Jon Masters jcm@redhat.com Tested-by: Timur Tabi timur@codeaurora.org Tested-by: Robert Richter rrichter@cavium.com Acked-by: Robert Richter rrichter@cavium.com Signed-off-by: Al Stone al.stone@linaro.org Signed-off-by: Hanjun Guo hanjun.guo@linaro.org

---

arch/arm64/Kconfig | 1 + arch/arm64/kernel/acpi.c | 8 +++++++- 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 1b8e973..d00ab9a 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -1,5 +1,6 @@ config ARM64 def_bool y

• select ACPI_REDUCED_HARDWARE_ONLY if ACPI select ARCH_BINFMT_ELF_RANDOMIZE_PIE select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE select ARCH_HAS_GCOV_PROFILE_ALL

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 6468f88..5819ef7 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -303,6 +303,11 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table) */ if (table->revision > 5 || (table->revision == 5 && fadt->minor_revision >= 1)) {

• if (!acpi_gbl_reduced_hardware) {
  

• 	pr_err("Not hardware reduced ACPI mode, will not be supported\n");
  

• 	goto disable_acpi;
  

• }
  

I reviewed the code and found that acpi_parse_fadt has become very complex to read and understand. On top of that, I still do not understand why you check PSCI presence in there (to print a warning ?) and as it was raised before disable_acpi() is scattered all over the place. I do not understand why we enable ACPI to disable it again if one of the checks fails, IMHO it is better to leave it disabled, carry out the checks and enable ACPI if all of them pass.

I came up with the patch attached on top of your series, which should be split, tested on Juno, please test, let me know your opinion and shout if you spot something wrong, it should simplify things a lot.

I wonder if acpi_get_table_with_size() usage is frowned upon, but you use it anyway and it helps us remove the FADT parsing function that in my opinion is useless, since its return value is dumped by ACPI core.

Thanks, Lorenzo

--- arch/arm64/kernel/acpi.c | 92 ++++++++++++++++++++++++------------------------ 1 file changed, 46 insertions(+), 46 deletions(-)

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 5819ef7..b7497fa 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -291,56 +291,23 @@ void acpi_unregister_gsi(u32 gsi) } EXPORT_SYMBOL_GPL(acpi_unregister_gsi);

-static int __init acpi_parse_fadt(struct acpi_table_header *table) -{ - struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table; - - /* - * 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 newer revision to get GIC and SMP - * boot protocol configuration data, or we will disable ACPI. - */ - if (table->revision > 5 || - (table->revision == 5 && fadt->minor_revision >= 1)) { - if (!acpi_gbl_reduced_hardware) { - pr_err("Not hardware reduced ACPI mode, will not be supported\n"); - goto disable_acpi; - } - - /* - * 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 Parking protocol spec. - */ - if (acpi_psci_present()) - return 0; - - pr_warn("No PSCI support, will not bring up secondary CPUs\n"); - return -EOPNOTSUPP; - } - - pr_warn("Unsupported FADT revision %d.%d, should be 5.1+, will disable ACPI\n", - table->revision, fadt->minor_revision); - -disable_acpi: - disable_acpi(); - return -EINVAL; -} - /* * acpi_boot_table_init() called from setup_arch(), always. * 1. find RSDP and get its address, and then find XSDT * 2. extract all tables and checksums them all * 3. check ACPI FADT revision + * 4. check ACPI FADT HW reduced flag * * We can parse ACPI boot-time tables such as MADT after * this function is called. */ void __init acpi_boot_table_init(void) { + struct acpi_table_header *table; + struct acpi_table_fadt *fadt; + acpi_status status; + acpi_size tbl_size; + /* * Enable ACPI instead of device tree unless * - ACPI has been disabled explicitly (acpi=off), or @@ -351,19 +318,52 @@ void __init acpi_boot_table_init(void) (!param_acpi_force && of_scan_flat_dt(dt_scan_depth1_nodes, NULL))) return;

- enable_acpi(); - /* Initialize the ACPI boot-time table parser. */ if (acpi_table_init()) { - disable_acpi(); + pr_err("Failed to init ACPI tables\n"); return; }

- if (acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt)) { - /* disable ACPI if no FADT is found */ - disable_acpi(); - pr_err("Can't find FADT\n"); + /* + * FADT is required on arm64; retrieve it to check its presence + * and carry out revision and ACPI HW reduced compliancy tests + */ + status = acpi_get_table_with_size(ACPI_SIG_FADT, 0, &table, &tbl_size); + if (ACPI_FAILURE(status)) { + const char *msg = acpi_format_exception(status); + + pr_err("Failed to get FADT table, %s\n", msg); + return; } + + 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 newer revision to get GIC and SMP + * boot protocol configuration data. + */ + if (table->revision < 5 || + (table->revision == 5 && fadt->minor_revision < 1)) { + pr_err("Unsupported FADT revision %d.%d, should be 5.1+\n", + table->revision, fadt->minor_revision); + goto out_unmap; + } + + if (!(fadt->flags & ACPI_FADT_HW_REDUCED)) { + pr_err("FADT not ACPI hardware reduced compliant\n"); + goto out_unmap; + } + + enable_acpi(); + + /* + * acpi_get_table_with_size() creates FADT table mapping that + * should be released after parsing and before resuming boot + */ +out_unmap: + early_acpi_os_unmap_memory((char *)table, tbl_size); }

void __init acpi_gic_init(void)

On Fri, Mar 13, 2015 at 03:28:45AM +0000, Hanjun Guo wrote:

[...]

...

/*

• acpi_boot_table_init() called from setup_arch(), always.
• 1. find RSDP and get its address, and then find XSDT
• 2. extract all tables and checksums them all
• 3. check ACPI FADT revision

• • 4. check ACPI FADT HW reduced flag
  • We can parse ACPI boot-time tables such as MADT after
  • this function is called.
  */

void __init acpi_boot_table_init(void) {

• struct acpi_table_header *table;
• struct acpi_table_fadt *fadt;
• acpi_status status;
• acpi_size tbl_size;
• /*
  • Enable ACPI instead of device tree unless
  • • ACPI has been disabled explicitly (acpi=off), or

@@ -351,19 +318,52 @@ void __init acpi_boot_table_init(void) (!param_acpi_force && of_scan_flat_dt(dt_scan_depth1_nodes, NULL))) return;

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

Since we disable ACPI in default, it is a bit strange for me to init all the ACPI tables and parse FADT when ACPI is disabled, could you put some comments here to clarify the purpose? other than that, it is looks good to me.

Ok, the purpose was to make things simpler, but I think that given current code it is not 100% safe to init ACPI tables with acpi_disabled == 1.

To me having to enable ACPI to parse the tables and check *if* ACPI tables are there is a bit crazy, but I agree with you that given current code it is safer.

Patch rewritten, here below, please have a look, test it and rework bits as needed, I added comments where I thought they were needed but please add to that if you feel it is worth it.

It should be easy to split, let me know if you want an incremental version.

Thanks ! Lorenzo

--- arch/arm64/kernel/acpi.c | 103 +++++++++++++++++++++++++++++++---------------- 1 file changed, 69 insertions(+), 34 deletions(-)

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 5819ef7..e5ee4d4 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -291,43 +291,60 @@ void acpi_unregister_gsi(u32 gsi) } EXPORT_SYMBOL_GPL(acpi_unregister_gsi);

-static int __init acpi_parse_fadt(struct acpi_table_header *table) +/* + * acpi_fadt_sanity_check() - Check FADT presence and carry out sanity + * checks on it + * + * Return 0 on success, <0 on failure + */ +static int __init acpi_fadt_sanity_check(void) { - struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table; + struct acpi_table_header *table; + struct acpi_table_fadt *fadt; + acpi_status status; + acpi_size tbl_size; + int ret = 0; + + /* + * FADT is required on arm64; retrieve it to check its presence + * and carry out revision and ACPI HW reduced compliancy tests + */ + status = acpi_get_table_with_size(ACPI_SIG_FADT, 0, &table, &tbl_size); + if (ACPI_FAILURE(status)) { + const char *msg = acpi_format_exception(status); + + pr_err("Failed to get FADT table, %s\n", msg); + return -ENODEV; + } + + 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 newer revision to get GIC and SMP - * boot protocol configuration data, or we will disable ACPI. + * boot protocol configuration data. */ - if (table->revision > 5 || - (table->revision == 5 && fadt->minor_revision >= 1)) { - if (!acpi_gbl_reduced_hardware) { - pr_err("Not hardware reduced ACPI mode, will not be supported\n"); - goto disable_acpi; - } - - /* - * 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 Parking protocol spec. - */ - if (acpi_psci_present()) - return 0; - - pr_warn("No PSCI support, will not bring up secondary CPUs\n"); - return -EOPNOTSUPP; + if (table->revision < 5 || + (table->revision == 5 && fadt->minor_revision < 1)) { + pr_err("Unsupported FADT revision %d.%d, should be 5.1+\n", + table->revision, fadt->minor_revision); + ret = -EINVAL; + goto out; }

- pr_warn("Unsupported FADT revision %d.%d, should be 5.1+, will disable ACPI\n", - table->revision, fadt->minor_revision); + if (!(fadt->flags & ACPI_FADT_HW_REDUCED)) { + pr_err("FADT not ACPI hardware reduced compliant\n"); + ret = -EINVAL; + }

-disable_acpi: - disable_acpi(); - return -EINVAL; +out: + /* + * acpi_get_table_with_size() creates FADT table mapping that + * should be released after parsing and before resuming boot + */ + early_acpi_os_unmap_memory((char *)table, tbl_size); + return ret; }

/* @@ -335,12 +352,17 @@ disable_acpi: * 1. find RSDP and get its address, and then find XSDT * 2. extract all tables and checksums them all * 3. check ACPI FADT revision + * 4. check ACPI FADT HW reduced flag * * We can parse ACPI boot-time tables such as MADT after * this function is called. + * + * ACPI is enabled on return if ACPI tables initialized and sanity checks + * passed, disabled otherwise */ void __init acpi_boot_table_init(void) { + /* * Enable ACPI instead of device tree unless * - ACPI has been disabled explicitly (acpi=off), or @@ -351,19 +373,32 @@ void __init acpi_boot_table_init(void) (!param_acpi_force && of_scan_flat_dt(dt_scan_depth1_nodes, NULL))) return;

+ /* + * ACPI is disabled at this point. Enable it in order to parse + * the ACPI tables and carry out sanity checks + */ enable_acpi();

/* Initialize the ACPI boot-time table parser. */ if (acpi_table_init()) { - disable_acpi(); - return; + pr_err("Failed to init ACPI tables\n"); + goto init_failed; }

- if (acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt)) { - /* disable ACPI if no FADT is found */ - disable_acpi(); - pr_err("Can't find FADT\n"); - } + /* + * Check FADT presence and carry out FADT sanity checks + */ + if (acpi_fadt_sanity_check() < 0) + goto init_failed; + + /* + * ACPI tables initialized and FADT sanity checks passed, leave + * ACPI enabled and carry on booting + */ + return; + +init_failed: + disable_acpi(); }

void __init acpi_gic_init(void)

On Mon, Mar 16, 2015 at 11:33:29AM +0000, Hanjun Guo wrote:

[...]

...

Patch rewritten, here below, please have a look, test it and rework bits as needed, I added comments where I thought they were needed but please add to that if you feel it is worth it.

It should be easy to split, let me know if you want an incremental version.

This one is much better, pretty fine to me, thanks!

I assume that this patch is cleanup patch on top of ARM64 ACPI core patches, right?

Well, it is a clean-up patch on top of your series, I can split it in the respective patches and send you the patches in your series updated with this code.

Thanks, Lorenzo

Thanks Hanjun

...

Thanks ! Lorenzo

---

arch/arm64/kernel/acpi.c | 103 +++++++++++++++++++++++++++++++---------------- 1 file changed, 69 insertions(+), 34 deletions(-)

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index 5819ef7..e5ee4d4 100644 --- a/arch/arm64/kernel/acpi.c +++ b/arch/arm64/kernel/acpi.c @@ -291,43 +291,60 @@ void acpi_unregister_gsi(u32 gsi) } EXPORT_SYMBOL_GPL(acpi_unregister_gsi);

-static int __init acpi_parse_fadt(struct acpi_table_header *table) +/*

• • acpi_fadt_sanity_check() - Check FADT presence and carry out sanity

• • 	      checks on it
    

• • Return 0 on success, <0 on failure
• */

+static int __init acpi_fadt_sanity_check(void) {

• struct acpi_table_fadt *fadt = (struct acpi_table_fadt *)table;

• struct acpi_table_header *table;

• struct acpi_table_fadt *fadt;

• acpi_status status;

• acpi_size tbl_size;

• int ret = 0;

• /*

• * FADT is required on arm64; retrieve it to check its presence
  

• * and carry out revision and ACPI HW reduced compliancy tests
  

• */
  

• status = acpi_get_table_with_size(ACPI_SIG_FADT, 0, &table, &tbl_size);

• if (ACPI_FAILURE(status)) {

• const char *msg = acpi_format_exception(status);
  

• pr_err("Failed to get FADT table, %s\n", msg);
  

• return -ENODEV;
  

• }

• 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 newer revision to get GIC and SMP

• * boot protocol configuration data, or we will disable ACPI.
  

• * boot protocol configuration data.
  

  */

• if (table->revision > 5 ||

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

• if (!acpi_gbl_reduced_hardware) {
  

• 	pr_err("Not hardware reduced ACPI mode, will not be supported\n");
  

• 	goto disable_acpi;
  

• }
  

• /*
  

•  * 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 Parking protocol spec.
  

•  */
  

• if (acpi_psci_present())
  

• 	return 0;
  

• pr_warn("No PSCI support, will not bring up secondary CPUs\n");
  

• return -EOPNOTSUPP;
  

• if (table->revision < 5 ||

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

• pr_err("Unsupported FADT revision %d.%d, should be 5.1+\n",
  

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

• ret = -EINVAL;
  

• goto out;
  

  }

• pr_warn("Unsupported FADT revision %d.%d, should be 5.1+, will disable ACPI\n",

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

• if (!(fadt->flags & ACPI_FADT_HW_REDUCED)) {

• pr_err("FADT not ACPI hardware reduced compliant\n");
  

• ret = -EINVAL;
  

• }

-disable_acpi:

• disable_acpi();
• return -EINVAL;

+out:

• /*

• * acpi_get_table_with_size() creates FADT table mapping that
  

• * should be released after parsing and before resuming boot
  

• */
  

• early_acpi_os_unmap_memory((char *)table, tbl_size);

• return ret; }

  /*

@@ -335,12 +352,17 @@ disable_acpi:

• 1. find RSDP and get its address, and then find XSDT
• 2. extract all tables and checksums them all
• 3. check ACPI FADT revision

• • 4. check ACPI FADT HW reduced flag
  • We can parse ACPI boot-time tables such as MADT after
  • this function is called.
• • ACPI is enabled on return if ACPI tables initialized and sanity checks
• • passed, disabled otherwise
  */ void __init acpi_boot_table_init(void) {
• /*
  • Enable ACPI instead of device tree unless
  • • ACPI has been disabled explicitly (acpi=off), or

@@ -351,19 +373,32 @@ void __init acpi_boot_table_init(void) (!param_acpi_force && of_scan_flat_dt(dt_scan_depth1_nodes, NULL))) return;

• /*

• * ACPI is disabled at this point. Enable it in order to parse
  

• * the ACPI tables and carry out sanity checks
  

• */
  

  enable_acpi();

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

• disable_acpi();
  

• return;
  

• pr_err("Failed to init ACPI tables\n");
  

• goto init_failed;
  

  }

• if (acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt)) {

• /* disable ACPI if no FADT is found */
  

• disable_acpi();
  

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

• }

• /*

• * Check FADT presence and carry out FADT sanity checks
  

• */
  

• if (acpi_fadt_sanity_check() < 0)

• goto init_failed;
  

• /*

• * ACPI tables initialized and FADT sanity checks passed, leave
  

• * ACPI enabled and carry on booting
  

• */
  

• return;

+init_failed:

• disable_acpi(); }

  void __init acpi_gic_init(void)

On Wednesday, March 18, 2015 09:18:20 AM Lorenzo Pieralisi wrote:

On Mon, Mar 16, 2015 at 11:33:29AM +0000, Hanjun Guo wrote:

...

On 2015年03月13日 19:04, Lorenzo Pieralisi wrote:

...

On Fri, Mar 13, 2015 at 03:28:45AM +0000, Hanjun Guo wrote:

[...]

...

...

/*

• acpi_boot_table_init() called from setup_arch(), always.
• 1. find RSDP and get its address, and then find XSDT
• 2. extract all tables and checksums them all
• 3. check ACPI FADT revision

• • 4. check ACPI FADT HW reduced flag
  • We can parse ACPI boot-time tables such as MADT after
  • this function is called.
  */ void __init acpi_boot_table_init(void) {
• struct acpi_table_header *table;
• struct acpi_table_fadt *fadt;
• acpi_status status;
• acpi_size tbl_size;
• /*
  • Enable ACPI instead of device tree unless
  • • ACPI has been disabled explicitly (acpi=off), or

@@ -351,19 +318,52 @@ void __init acpi_boot_table_init(void) (!param_acpi_force && of_scan_flat_dt(dt_scan_depth1_nodes, NULL))) return;

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

Since we disable ACPI in default, it is a bit strange for me to init all the ACPI tables and parse FADT when ACPI is disabled, could you put some comments here to clarify the purpose? other than that, it is looks good to me.

Ok, the purpose was to make things simpler, but I think that given current code it is not 100% safe to init ACPI tables with acpi_disabled == 1.

To me having to enable ACPI to parse the tables and check *if* ACPI tables are there is a bit crazy, but I agree with you that given current code it is safer.

Patch rewritten, here below, please have a look, test it and rework bits as needed, I added comments where I thought they were needed but please add to that if you feel it is worth it.

It should be easy to split, let me know if you want an incremental version.

This one is much better, pretty fine to me, thanks!

I assume that this patch is cleanup patch on top of ARM64 ACPI core patches, right?

For the records, I created a branch with my patch split over some of your patches, even though I think to make things simpler it is best to apply it as a single patch on top of your series,

I agree. In particular, the patches that have already been reviewed and ACKed won't need to be reviewed again this way which seems to be crucially important to me.

I will prepare the commit log for the single patch version too.

Thanks!

Hanjun, please add this one to the series when ready.