On Fri, 25 Apr 2014 21:20:07 +0800, Hanjun Guo hanjun.guo@linaro.org wrote:

_PDC related stuff in processor_core.c is little bit X86/IA64 dependent, macros of ACPI_PDC_* are _PDC bit definitions for Intel processors, if we use these macros in processor_core.c, we will meet compile error when ACPI is enabled on ARM64.

This patch reworks the code to make it more arch-independent, moving Intel related _PDC bits into architecture directory, no functional change.

Cc: Tony Luck tony.luck@intel.com Cc: Fenghua Yu fenghua.yu@intel.com Cc: Thomas Gleixner tglx@linutronix.de Cc: "H. Peter Anvin" hpa@zytor.com Cc: x86@kernel.org Cc: linux-ia64@vger.kernel.org Signed-off-by: Hanjun Guo hanjun.guo@linaro.org Signed-off-by: Graeme Gregory graeme.gregory@linaro.org

---

arch/ia64/include/asm/acpi.h | 5 +---- arch/ia64/kernel/acpi.c | 15 +++++++++++++++ arch/x86/include/asm/acpi.h | 19 +------------------ arch/x86/kernel/acpi/cstate.c | 27 +++++++++++++++++++++++++++ drivers/acpi/processor_core.c | 19 +------------------ 5 files changed, 45 insertions(+), 40 deletions(-)

diff --git a/arch/ia64/include/asm/acpi.h b/arch/ia64/include/asm/acpi.h index d651102..d2b8b9d 100644 --- a/arch/ia64/include/asm/acpi.h +++ b/arch/ia64/include/asm/acpi.h @@ -152,10 +152,7 @@ extern int __initdata nid_to_pxm_map[MAX_NUMNODES]; #endif static inline bool arch_has_acpi_pdc(void) { return true; } -static inline void arch_acpi_set_pdc_bits(u32 *buf) -{

• buf[2] |= ACPI_PDC_EST_CAPABILITY_SMP;

-} +extern void arch_acpi_set_pdc_bits(u32 *buf); #define acpi_unlazy_tlb(x) diff --git a/arch/ia64/kernel/acpi.c b/arch/ia64/kernel/acpi.c index 0d407b3..23e9b40 100644 --- a/arch/ia64/kernel/acpi.c +++ b/arch/ia64/kernel/acpi.c @@ -996,3 +996,18 @@ EXPORT_SYMBOL(acpi_unregister_ioapic);

• TBD when when IA64 starts to support suspend...

*/ int acpi_suspend_lowlevel(void) { return 0; }

+void arch_acpi_set_pdc_bits(u32 *buf) +{

• /* Enable coordination with firmware's _TSD info */
• buf[2] |= ACPI_PDC_SMP_T_SWCOORD | ACPI_PDC_EST_CAPABILITY_SMP;
• if (boot_option_idle_override == IDLE_NOMWAIT) {

• /*
  

•  * If mwait is disabled for CPU C-states, the C2C3_FFH access
  

•  * mode will be disabled in the parameter of _PDC object.
  

•  * Of course C1_FFH access mode will also be disabled.
  

•  */
  

• buf[2] &= ~(ACPI_PDC_C_C2C3_FFH | ACPI_PDC_C_C1_FFH);
  

• }

+}

The commit text makes no comment about why this function is being moved from a static inline to an extern in the acpi.c file. I assume it is because it needs access to the boot_option_idle_override global variable, but it isn't immediately obvious, and should be described in the commit text.

Otherwise, the patch looks sane.

Reviewed-by: Grant Likely grant.likely@linaro.org

diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h index c8c1e70..e9f71bc 100644 --- a/arch/x86/include/asm/acpi.h +++ b/arch/x86/include/asm/acpi.h @@ -147,24 +147,7 @@ static inline bool arch_has_acpi_pdc(void) c->x86_vendor == X86_VENDOR_CENTAUR); } -static inline void arch_acpi_set_pdc_bits(u32 *buf) -{

• struct cpuinfo_x86 *c = &cpu_data(0);
• buf[2] |= ACPI_PDC_C_CAPABILITY_SMP;
• if (cpu_has(c, X86_FEATURE_EST))

• buf[2] |= ACPI_PDC_EST_CAPABILITY_SWSMP;
  

• if (cpu_has(c, X86_FEATURE_ACPI))

• buf[2] |= ACPI_PDC_T_FFH;
  

• /*

• * If mwait/monitor is unsupported, C2/C3_FFH will be disabled
  

• */
  

• if (!cpu_has(c, X86_FEATURE_MWAIT))

• buf[2] &= ~(ACPI_PDC_C_C2C3_FFH);
  

-} +extern void arch_acpi_set_pdc_bits(u32 *buf); #else /* !CONFIG_ACPI */ diff --git a/arch/x86/kernel/acpi/cstate.c b/arch/x86/kernel/acpi/cstate.c index 4b28159..95127d0 100644 --- a/arch/x86/kernel/acpi/cstate.c +++ b/arch/x86/kernel/acpi/cstate.c @@ -16,6 +16,33 @@ #include <asm/mwait.h> #include <asm/special_insns.h> +void arch_acpi_set_pdc_bits(u32 *buf) +{

• struct cpuinfo_x86 *c = &cpu_data(0);
• /* Enable coordination with firmware's _TSD info */
• buf[2] |= ACPI_PDC_SMP_T_SWCOORD | ACPI_PDC_C_CAPABILITY_SMP;
• if (cpu_has(c, X86_FEATURE_EST))

• buf[2] |= ACPI_PDC_EST_CAPABILITY_SWSMP;
  

• if (cpu_has(c, X86_FEATURE_ACPI))

• buf[2] |= ACPI_PDC_T_FFH;
  

• /* If mwait/monitor is unsupported, C2/C3_FFH will be disabled */
• if (!cpu_has(c, X86_FEATURE_MWAIT))

• buf[2] &= ~(ACPI_PDC_C_C2C3_FFH);
  

• if (boot_option_idle_override == IDLE_NOMWAIT) {

• /*
  

•  * If mwait is disabled for CPU C-states, the C2C3_FFH access
  

•  * mode will be disabled in the parameter of _PDC object.
  

•  * Of course C1_FFH access mode will also be disabled.
  

•  */
  

• buf[2] &= ~(ACPI_PDC_C_C2C3_FFH | ACPI_PDC_C_C1_FFH);
  

• }

+}

/*

• Initialize bm_flags based on the CPU cache properties
• On SMP it depends on cache configuration

diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c index 71e2065..5250327 100644 --- a/drivers/acpi/processor_core.c +++ b/drivers/acpi/processor_core.c @@ -254,9 +254,6 @@ static void acpi_set_pdc_bits(u32 *buf) buf[0] = ACPI_PDC_REVISION_ID; buf[1] = 1;

• /* Enable coordination with firmware's _TSD info */
• buf[2] = ACPI_PDC_SMP_T_SWCOORD;
• /* Twiddle arch-specific bits needed for _PDC */ arch_acpi_set_pdc_bits(buf);

} @@ -281,7 +278,7 @@ static struct acpi_object_list *acpi_processor_alloc_pdc(void) return NULL; }

• buf = kmalloc(12, GFP_KERNEL);

• buf = kzalloc(12, GFP_KERNEL); if (!buf) { printk(KERN_ERR "Memory allocation error\n"); kfree(obj);

@@ -309,20 +306,6 @@ acpi_processor_eval_pdc(acpi_handle handle, struct acpi_object_list *pdc_in) { acpi_status status = AE_OK;

• if (boot_option_idle_override == IDLE_NOMWAIT) {

• /*
  

•  * If mwait is disabled for CPU C-states, the C2C3_FFH access
  

•  * mode will be disabled in the parameter of _PDC object.
  

•  * Of course C1_FFH access mode will also be disabled.
  

•  */
  

• union acpi_object *obj;
  

• u32 *buffer = NULL;
  

• obj = pdc_in->pointer;
  

• buffer = (u32 *)(obj->buffer.pointer);
  

• buffer[2] &= ~(ACPI_PDC_C_C2C3_FFH | ACPI_PDC_C_C1_FFH);
  

• } status = acpi_evaluate_object(handle, "_PDC", pdc_in, NULL);

if (ACPI_FAILURE(status)) -- 1.7.9.5

On 2014-4-29 17:39, Grant Likely wrote:

On Fri, 25 Apr 2014 21:20:08 +0800, Hanjun Guo hanjun.guo@linaro.org wrote:

...

The _PDC (Processor Driver Capabilities) object provides OSPM a mechanism to convey to the platform the capabilities supported by OSPM for processor power management.

OSPM evaluates _PDC prior to evaluating any other processor power management objects returning configuration information.

This patch introduces the skeleton of _PDC related file to make ACPI core can be compiled on ARM64, and should be revisited when implementing CPU power control.

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

---

arch/arm64/include/asm/acpi.h | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 arch/arm64/include/asm/acpi.h

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h new file mode 100644 index 0000000..e3e990e --- /dev/null +++ b/arch/arm64/include/asm/acpi.h @@ -0,0 +1,37 @@ +/*

• • Copyright (C) 2013~2014, Al Stone al.stone@linaro.org

Not really a correct copyright assignement. The copyright is held by Linaro, but the author is Al. It should instead read:

• Copyright (C0 2013-2014, Linaro Ltd.
• Author: Al Stone al.stone@linaro.org

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

We will update this patch accordingly.

Thanks Hanjun

On Fri, 25 Apr 2014 21:20:09 +0800, Hanjun Guo hanjun.guo@linaro.org wrote:

ACPI core requires a cpu.h, add a dummy one here temporarily. This file will be updated in the future to add definitions for arch_(un)register_cpu which are required for ACPI based physical CPU hotplug when these standards have be ratified by the ASWG.

Signed-off-by: Graeme Gregory graeme.gregory@linaro.org Signed-off-by: Hanjun Guo hanjun.guo@linaro.org

---

arch/arm64/include/asm/cpu.h | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 arch/arm64/include/asm/cpu.h

I think we're trying to get away from these empty header stubs by putting them in asm-generic. Arnd, care to comment here?

g.

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

1.7.9.5

On Fri, Apr 25, 2014 at 04:51:47PM +0100, Mark Rutland wrote:

Hi Hanjun,

On Fri, Apr 25, 2014 at 02:20:10PM +0100, Hanjun Guo wrote:

...

ACPI core need lots extern variables and functions which should be provided by arch dependent code to make itself compilable. so introduce arm_core.c and its related header file here.

acpi_boot_table_init() will be called in setup_arch() before paging_init(), so we should use eary_ioremap() mechanism here to get the RSDP and all the table pointers, with this patch, we can get ACPI boot-time tables from firmware on ARM64.

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: Tomasz Nowicki tomasz.nowicki@linaro.org

---

arch/arm64/include/asm/acpi.h | 53 +++++++++++++++++++ arch/arm64/kernel/setup.c | 4 ++ drivers/acpi/Makefile | 2 + drivers/acpi/plat/Makefile | 1 + drivers/acpi/plat/arm-core.c | 113 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 173 insertions(+) create mode 100644 drivers/acpi/plat/Makefile create mode 100644 drivers/acpi/plat/arm-core.c

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index e3e990e..3ac9dfb 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -19,6 +19,43 @@ #ifndef _ASM_ACPI_H #define _ASM_ACPI_H +#include <asm/cacheflush.h>

+#include <linux/init.h>

+#define COMPILER_DEPENDENT_INT64 s64 +#define COMPILER_DEPENDENT_UINT64 u64

Is there any reason this can't be in a common ACPI header shared be ia64 and x86 too? Given we already have generic types for this it seems pointless to define this in each architecture.

It looks like include/acpi/actypes.h tries to do that already...

Yes I think we can replace that with uint64_t and int64_t types.

...

+/*

• • Calling conventions:
• • ACPI_SYSTEM_XFACE - Interfaces to host OS (handlers, threads)
• • ACPI_EXTERNAL_XFACE - External ACPI interfaces
• • ACPI_INTERNAL_XFACE - Internal ACPI interfaces
• • ACPI_INTERNAL_VAR_XFACE - Internal variable-parameter list interfaces
• */

+#define ACPI_SYSTEM_XFACE +#define ACPI_EXTERNAL_XFACE +#define ACPI_INTERNAL_XFACE +#define ACPI_INTERNAL_VAR_XFACE

+/* Asm macros */ +#define ACPI_FLUSH_CPU_CACHE() flush_cache_all()

This almost certainly does not do what you think it does.

flush_cache_all walks the architected levels of cache visible to the current CPU (i.e. those in CLIDR_EL1), and walks over each cache line at that level, cleaning and evicting it. It also flushes the I-cache (which I don't think you care about here).

This is NOT safe if the cache is enabled. Lines can migrate between levels in the middle of the sequence.

In an SMP system this does NOT guarantee that data is evicted to memory, even if the cache is disabled. Other CPUs with caches enabled can acquire a cacheline (even if dirty) and it can sit in their cache.

In a UP system or an SMP system where all other architected caches are disabled (and flushed) this does NOT guarantee that data hits memory. In the presence of a system-level cache this will simply flush the data out to said system-level rather than memory.

I believe the intent here is to have something analogous to WBINVD for use in idle. Unfortunately there simply isn't anything analogous. Luckily in the presence of PSCI, the PSCI implementation should do all of the cache maintenance required to prevent any data loss and/or corruption, and anything we need to have visible to noncacheable accesses (i.e. flushed out to memory) we should be able to flush by VA.

This maintenance is unsafe, and shouldn't be necessary on any sane system. Please get rid of it. I would very much like to get rid of flush_cache_all() before its misuse spreads further.

Thanks for explanation Mark, you are correct on x86 it is defined as wbinvd().

I think looking at where it is actually used we can make this an empty macro on arm64 for now. Where it used are areas we don't currently execute and need arm64 replacements or refactorising to remove x86isms.

...

+/* Basic configuration for ACPI */ +#ifdef CONFIG_ACPI +extern int acpi_disabled; +extern int acpi_noirq; +extern int acpi_pci_disabled; +extern int acpi_strict;

This looks very odd. Why are these prototypes not coming from a header? If they're defined in the same place, why not move the disable_acpi function there?

This is a header :-)

I think this is a peculiarity of how acpica is incorporated into linux but will check.

...

+static inline void disable_acpi(void) +{

• acpi_disabled = 1;
• acpi_pci_disabled = 1;
• acpi_noirq = 1;

+}

[...]

...

+/*

• • __acpi_map_table() will be called before page_init(), so early_ioremap()
• • or early_memremap() should be called here.
• */

+char *__init __acpi_map_table(unsigned long phys, unsigned long size) +{

• if (!phys || !size)

• return NULL;
  

Is there any reason that tables can't exist at physical address 0? It's entirely valid to have memory there.

[...]

On ARM64 there is not, we can fix this.

...

+int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) +{

• *irq = -1;
• return 0;

+} +EXPORT_SYMBOL_GPL(acpi_gsi_to_irq);

This appears to be missing a giant warning that it does nothing useful.

I was under the impression that we were meant to use 0 to represent the lack of an interrupt these days, too...

We can fix this.

Graeme

On Fri, Apr 25, 2014 at 07:38:48PM +0100, Mark Rutland wrote:

On Fri, Apr 25, 2014 at 05:53:20PM +0100, Graeme Gregory wrote:

...

On Fri, Apr 25, 2014 at 04:51:47PM +0100, Mark Rutland wrote:

...

Hi Hanjun,

On Fri, Apr 25, 2014 at 02:20:10PM +0100, Hanjun Guo wrote:

...

ACPI core need lots extern variables and functions which should be provided by arch dependent code to make itself compilable. so introduce arm_core.c and its related header file here.

acpi_boot_table_init() will be called in setup_arch() before paging_init(), so we should use eary_ioremap() mechanism here to get the RSDP and all the table pointers, with this patch, we can get ACPI boot-time tables from firmware on ARM64.

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: Tomasz Nowicki tomasz.nowicki@linaro.org

---

arch/arm64/include/asm/acpi.h | 53 +++++++++++++++++++ arch/arm64/kernel/setup.c | 4 ++ drivers/acpi/Makefile | 2 + drivers/acpi/plat/Makefile | 1 + drivers/acpi/plat/arm-core.c | 113 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 173 insertions(+) create mode 100644 drivers/acpi/plat/Makefile create mode 100644 drivers/acpi/plat/arm-core.c

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index e3e990e..3ac9dfb 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -19,6 +19,43 @@ #ifndef _ASM_ACPI_H #define _ASM_ACPI_H +#include <asm/cacheflush.h>

+#include <linux/init.h>

+#define COMPILER_DEPENDENT_INT64 s64 +#define COMPILER_DEPENDENT_UINT64 u64

Is there any reason this can't be in a common ACPI header shared be ia64 and x86 too? Given we already have generic types for this it seems pointless to define this in each architecture.

It looks like include/acpi/actypes.h tries to do that already...

Yes I think we can replace that with uint64_t and int64_t types.

I have dug into this deeper and since include/acpi/platform/aclinux.h defines ACPI_USE_SYSTEM_INTTYPES then these defines should not be used at all and it should be safe for us to just not have them in arm64

I guess they are historic in x86/ia64

...

...

...

+/*

• • Calling conventions:
• • ACPI_SYSTEM_XFACE - Interfaces to host OS (handlers, threads)
• • ACPI_EXTERNAL_XFACE - External ACPI interfaces
• • ACPI_INTERNAL_XFACE - Internal ACPI interfaces
• • ACPI_INTERNAL_VAR_XFACE - Internal variable-parameter list interfaces
• */

+#define ACPI_SYSTEM_XFACE +#define ACPI_EXTERNAL_XFACE +#define ACPI_INTERNAL_XFACE +#define ACPI_INTERNAL_VAR_XFACE

+/* Asm macros */ +#define ACPI_FLUSH_CPU_CACHE() flush_cache_all()

This almost certainly does not do what you think it does.

flush_cache_all walks the architected levels of cache visible to the current CPU (i.e. those in CLIDR_EL1), and walks over each cache line at that level, cleaning and evicting it. It also flushes the I-cache (which I don't think you care about here).

This is NOT safe if the cache is enabled. Lines can migrate between levels in the middle of the sequence.

In an SMP system this does NOT guarantee that data is evicted to memory, even if the cache is disabled. Other CPUs with caches enabled can acquire a cacheline (even if dirty) and it can sit in their cache.

In a UP system or an SMP system where all other architected caches are disabled (and flushed) this does NOT guarantee that data hits memory. In the presence of a system-level cache this will simply flush the data out to said system-level rather than memory.

I believe the intent here is to have something analogous to WBINVD for use in idle. Unfortunately there simply isn't anything analogous. Luckily in the presence of PSCI, the PSCI implementation should do all of the cache maintenance required to prevent any data loss and/or corruption, and anything we need to have visible to noncacheable accesses (i.e. flushed out to memory) we should be able to flush by VA.

This maintenance is unsafe, and shouldn't be necessary on any sane system. Please get rid of it. I would very much like to get rid of flush_cache_all() before its misuse spreads further.

Thanks for explanation Mark, you are correct on x86 it is defined as wbinvd().

I think looking at where it is actually used we can make this an empty macro on arm64 for now. Where it used are areas we don't currently execute and need arm64 replacements or refactorising to remove x86isms.

That sounds good. Is it worth putting a warn or similar there just in case?

...

...

...

+/* Basic configuration for ACPI */ +#ifdef CONFIG_ACPI +extern int acpi_disabled; +extern int acpi_noirq; +extern int acpi_pci_disabled; +extern int acpi_strict;

This looks very odd. Why are these prototypes not coming from a header? If they're defined in the same place, why not move the disable_acpi function there?

This is a header :-)

True; I must get my eyes tested. :)

Are these variables expected to be used by needed by other code, or are they just for the benefit of the static inlines in this header?

...

I think this is a peculiarity of how acpica is incorporated into linux but will check.

Ok.

...

...

...

+static inline void disable_acpi(void) +{

• acpi_disabled = 1;
• acpi_pci_disabled = 1;
• acpi_noirq = 1;

+}

[...]

...

+/*

• • __acpi_map_table() will be called before page_init(), so early_ioremap()
• • or early_memremap() should be called here.
• */

+char *__init __acpi_map_table(unsigned long phys, unsigned long size) +{

• if (!phys || !size)

• return NULL;
  

Is there any reason that tables can't exist at physical address 0? It's entirely valid to have memory there.

[...]

On ARM64 there is not, we can fix this.

...

...

+int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) +{

• *irq = -1;
• return 0;

+} +EXPORT_SYMBOL_GPL(acpi_gsi_to_irq);

This appears to be missing a giant warning that it does nothing useful.

I was under the impression that we were meant to use 0 to represent the lack of an interrupt these days, too...

We can fix this.

Sounds good!

Cheers, Mark.

Thanks

Graeme

Hi,

From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Hanjun Guo Sent: Friday, April 25, 2014 9:20 PM

ACPI core need lots extern variables and functions which should be provided by arch dependent code to make itself compilable. so introduce arm_core.c and its related header file here.

acpi_boot_table_init() will be called in setup_arch() before paging_init(), so we should use eary_ioremap() mechanism here to get the RSDP and all the table pointers, with this patch, we can get ACPI boot-time tables from firmware on ARM64.

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: Tomasz Nowicki tomasz.nowicki@linaro.org

---

arch/arm64/include/asm/acpi.h | 53 +++++++++++++++++++ arch/arm64/kernel/setup.c | 4 ++ drivers/acpi/Makefile | 2 + drivers/acpi/plat/Makefile | 1 + drivers/acpi/plat/arm-core.c | 113 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 173 insertions(+) create mode 100644 drivers/acpi/plat/Makefile create mode 100644 drivers/acpi/plat/arm-core.c

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index e3e990e..3ac9dfb 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -19,6 +19,43 @@ #ifndef _ASM_ACPI_H #define _ASM_ACPI_H

+#include <asm/cacheflush.h>

+#include <linux/init.h>

+#define COMPILER_DEPENDENT_INT64 s64 +#define COMPILER_DEPENDENT_UINT64 u64

+/*

• • Calling conventions:
• • ACPI_SYSTEM_XFACE - Interfaces to host OS (handlers, threads)
• • ACPI_EXTERNAL_XFACE - External ACPI interfaces
• • ACPI_INTERNAL_XFACE - Internal ACPI interfaces
• • ACPI_INTERNAL_VAR_XFACE - Internal variable-parameter list interfaces
• */

+#define ACPI_SYSTEM_XFACE +#define ACPI_EXTERNAL_XFACE +#define ACPI_INTERNAL_XFACE +#define ACPI_INTERNAL_VAR_XFACE

Just a reminder. I'm going to sort such default definitions out. The story can be found here: https://lkml.org/lkml/2014/4/8/57 The patch to achieve this can be found here: https://lkml.org/lkml/2014/4/23/71

If your series is merged after the above series, you don't need the above lines.

+/* Asm macros */ +#define ACPI_FLUSH_CPU_CACHE() flush_cache_all()

Another reminder. I'm going to split this into <asm/acenv.h> in the same story. The patch to achieve this can be found here: https://lkml.org/lkml/2014/4/23/75 If your series is merged after the above series, you need to put this line into arch/arm/include/asm/acenv.h.

Thanks and best regards -Lv

+/* Basic configuration for ACPI */ +#ifdef CONFIG_ACPI +extern int acpi_disabled; +extern int acpi_noirq; +extern int acpi_pci_disabled; +extern int acpi_strict;

+static inline void disable_acpi(void) +{

• acpi_disabled = 1;
• acpi_pci_disabled = 1;
• acpi_noirq = 1;

+}

static inline bool arch_has_acpi_pdc(void) { /* @@ -34,4 +71,20 @@ static inline void arch_acpi_set_pdc_bits(u32 *buf) return; }

+static inline void acpi_noirq_set(void) { acpi_noirq = 1; } +static inline void acpi_disable_pci(void) +{

• acpi_pci_disabled = 1;
• acpi_noirq_set();

+}

+/*

• • The ACPI processor driver for ACPI core code needs this macro.
• • Temporarily define it to -1 so that the ACPI core can compile,
• • but update it when we get the hardware ID from the MADT table.
• */

+#define cpu_physical_id(cpu) -1

+#endif /* CONFIG_ACPI */

#endif /*_ASM_ACPI_H*/ diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index 720853f..70658e1 100644 --- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -41,6 +41,7 @@ #include <linux/memblock.h> #include <linux/of_fdt.h> #include <linux/of_platform.h> +#include <linux/acpi.h>

#include <asm/fixmap.h> #include <asm/cputype.h> @@ -368,6 +369,9 @@ void __init setup_arch(char **cmdline_p)

arm64_memblock_init();

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

diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index 0331f91..8521c97 100644 --- a/drivers/acpi/Makefile +++ b/drivers/acpi/Makefile @@ -83,3 +83,5 @@ obj-$(CONFIG_ACPI_PROCESSOR_AGGREGATOR) += acpi_pad.o obj-$(CONFIG_ACPI_APEI) += apei/

obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o

+obj-y += plat/ diff --git a/drivers/acpi/plat/Makefile b/drivers/acpi/plat/Makefile new file mode 100644 index 0000000..46bc65e --- /dev/null +++ b/drivers/acpi/plat/Makefile @@ -0,0 +1 @@ +obj-$(CONFIG_ARM64) += arm-core.o diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c new file mode 100644 index 0000000..47b5d9b --- /dev/null +++ b/drivers/acpi/plat/arm-core.c @@ -0,0 +1,113 @@ +/*

• • ARM/ARM64 Specific Low-Level ACPI Boot Support
• • Copyright (C) 2013~2014, Al Stone al.stone@linaro.org
• • Copyright (C) 2013~2014, Graeme Gregory graeme.gregory@linaro.org
• • Copyright (C) 2013~2014, 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 as published by
• • the Free Software Foundation; either version 2 of the License, or
• • (at your option) any later version.
• • This program is distributed in the hope that it will be useful,
• • but WITHOUT ANY WARRANTY; without even the implied warranty of
• • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
• • GNU General Public License for more details.
• */

+#include <linux/init.h> +#include <linux/acpi.h> +#include <linux/cpumask.h> +#include <linux/memblock.h> +#include <linux/module.h> +#include <linux/irq.h> +#include <linux/irqdomain.h> +#include <linux/bootmem.h> +#include <linux/smp.h>

+#include <asm/pgtable.h>

+/*

• • We never plan to use RSDT on arm/arm64 as its deprecated in spec but this
• • variable is still required by the ACPI core
• */

+u32 acpi_rsdt_forced;

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

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

+enum acpi_irq_model_id acpi_irq_model = ACPI_IRQ_MODEL_PLATFORM;

+/*

• • __acpi_map_table() will be called before page_init(), so early_ioremap()
• • or early_memremap() should be called here.
• */

+char *__init __acpi_map_table(unsigned long phys, unsigned long size) +{

• if (!phys || !size)

• return NULL;
  

• return early_ioremap(phys, size);

+}

+void __init __acpi_unmap_table(char *map, unsigned long size) +{

• if (!map || !size)

• return;
  

• early_iounmap(map, size);

+}

+int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) +{

• *irq = -1;
• 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) +{

• return -1;

+} +EXPORT_SYMBOL_GPL(acpi_register_gsi);

+void acpi_unregister_gsi(u32 gsi) +{ +} +EXPORT_SYMBOL_GPL(acpi_unregister_gsi);

+/*

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

• disable_acpi();
  

• return;
  

• }

+}

1.7.9.5

-- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html

On Fri, 25 Apr 2014 21:20:10 +0800, Hanjun Guo hanjun.guo@linaro.org wrote:

ACPI core need lots extern variables and functions which should be provided by arch dependent code to make itself compilable. so introduce arm_core.c and its related header file here.

acpi_boot_table_init() will be called in setup_arch() before paging_init(), so we should use eary_ioremap() mechanism here to get the RSDP and all the table pointers, with this patch, we can get ACPI boot-time tables from firmware on ARM64.

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: Tomasz Nowicki tomasz.nowicki@linaro.org

Looks okay to me.

Reviewed-by: Grant Likely grant.likely@linaro.org

---

arch/arm64/include/asm/acpi.h | 53 +++++++++++++++++++ arch/arm64/kernel/setup.c | 4 ++ drivers/acpi/Makefile | 2 + drivers/acpi/plat/Makefile | 1 + drivers/acpi/plat/arm-core.c | 113 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 173 insertions(+) create mode 100644 drivers/acpi/plat/Makefile create mode 100644 drivers/acpi/plat/arm-core.c

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index e3e990e..3ac9dfb 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -19,6 +19,43 @@ #ifndef _ASM_ACPI_H #define _ASM_ACPI_H +#include <asm/cacheflush.h>

+#include <linux/init.h>

+#define COMPILER_DEPENDENT_INT64 s64 +#define COMPILER_DEPENDENT_UINT64 u64

+/*

• • Calling conventions:
• • ACPI_SYSTEM_XFACE - Interfaces to host OS (handlers, threads)
• • ACPI_EXTERNAL_XFACE - External ACPI interfaces
• • ACPI_INTERNAL_XFACE - Internal ACPI interfaces
• • ACPI_INTERNAL_VAR_XFACE - Internal variable-parameter list interfaces
• */

+#define ACPI_SYSTEM_XFACE +#define ACPI_EXTERNAL_XFACE +#define ACPI_INTERNAL_XFACE +#define ACPI_INTERNAL_VAR_XFACE

+/* Asm macros */ +#define ACPI_FLUSH_CPU_CACHE() flush_cache_all()

+/* Basic configuration for ACPI */ +#ifdef CONFIG_ACPI +extern int acpi_disabled; +extern int acpi_noirq; +extern int acpi_pci_disabled; +extern int acpi_strict;

+static inline void disable_acpi(void) +{

• acpi_disabled = 1;
• acpi_pci_disabled = 1;
• acpi_noirq = 1;

+}

static inline bool arch_has_acpi_pdc(void) { /* @@ -34,4 +71,20 @@ static inline void arch_acpi_set_pdc_bits(u32 *buf) return; } +static inline void acpi_noirq_set(void) { acpi_noirq = 1; } +static inline void acpi_disable_pci(void) +{

• acpi_pci_disabled = 1;
• acpi_noirq_set();

+}

+/*

• • The ACPI processor driver for ACPI core code needs this macro.
• • Temporarily define it to -1 so that the ACPI core can compile,
• • but update it when we get the hardware ID from the MADT table.
• */

+#define cpu_physical_id(cpu) -1

+#endif /* CONFIG_ACPI */

#endif /*_ASM_ACPI_H*/ diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index 720853f..70658e1 100644 --- a/arch/arm64/kernel/setup.c +++ b/arch/arm64/kernel/setup.c @@ -41,6 +41,7 @@ #include <linux/memblock.h> #include <linux/of_fdt.h> #include <linux/of_platform.h> +#include <linux/acpi.h> #include <asm/fixmap.h> #include <asm/cputype.h> @@ -368,6 +369,9 @@ void __init setup_arch(char **cmdline_p) arm64_memblock_init();

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

diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index 0331f91..8521c97 100644 --- a/drivers/acpi/Makefile +++ b/drivers/acpi/Makefile @@ -83,3 +83,5 @@ obj-$(CONFIG_ACPI_PROCESSOR_AGGREGATOR) += acpi_pad.o obj-$(CONFIG_ACPI_APEI) += apei/ obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o

+obj-y += plat/ diff --git a/drivers/acpi/plat/Makefile b/drivers/acpi/plat/Makefile new file mode 100644 index 0000000..46bc65e --- /dev/null +++ b/drivers/acpi/plat/Makefile @@ -0,0 +1 @@ +obj-$(CONFIG_ARM64) += arm-core.o diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c new file mode 100644 index 0000000..47b5d9b --- /dev/null +++ b/drivers/acpi/plat/arm-core.c @@ -0,0 +1,113 @@ +/*

• • ARM/ARM64 Specific Low-Level ACPI Boot Support
• • Copyright (C) 2013~2014, Al Stone al.stone@linaro.org
• • Copyright (C) 2013~2014, Graeme Gregory graeme.gregory@linaro.org
• • Copyright (C) 2013~2014, 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 as published by
• • the Free Software Foundation; either version 2 of the License, or
• • (at your option) any later version.
• • This program is distributed in the hope that it will be useful,
• • but WITHOUT ANY WARRANTY; without even the implied warranty of
• • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
• • GNU General Public License for more details.
• */

+#include <linux/init.h> +#include <linux/acpi.h> +#include <linux/cpumask.h> +#include <linux/memblock.h> +#include <linux/module.h> +#include <linux/irq.h> +#include <linux/irqdomain.h> +#include <linux/bootmem.h> +#include <linux/smp.h>

+#include <asm/pgtable.h>

+/*

• • We never plan to use RSDT on arm/arm64 as its deprecated in spec but this
• • variable is still required by the ACPI core
• */

+u32 acpi_rsdt_forced;

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

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

+enum acpi_irq_model_id acpi_irq_model = ACPI_IRQ_MODEL_PLATFORM;

+/*

• • __acpi_map_table() will be called before page_init(), so early_ioremap()
• • or early_memremap() should be called here.
• */

+char *__init __acpi_map_table(unsigned long phys, unsigned long size) +{

• if (!phys || !size)

• return NULL;
  

• return early_ioremap(phys, size);

+}

+void __init __acpi_unmap_table(char *map, unsigned long size) +{

• if (!map || !size)

• return;
  

• early_iounmap(map, size);

+}

+int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) +{

• *irq = -1;
• 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) +{

• return -1;

+} +EXPORT_SYMBOL_GPL(acpi_register_gsi);

+void acpi_unregister_gsi(u32 gsi) +{ +} +EXPORT_SYMBOL_GPL(acpi_unregister_gsi);

+/*

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

• disable_acpi();
  

• return;
  

• }

+}

1.7.9.5

On Fri, 25 Apr 2014 21:20:11 +0800, Hanjun Guo hanjun.guo@linaro.org wrote:

Introduce two early parameters for "acpi", one is the parameter to disable ACPI on ARM64 and another one is acpi=strict to disable out-of-spec workarounds.

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

---

drivers/acpi/plat/arm-core.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)

diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c index 47b5d9b..e4846fd 100644 --- a/drivers/acpi/plat/arm-core.c +++ b/drivers/acpi/plat/arm-core.c @@ -111,3 +111,21 @@ void __init acpi_boot_table_init(void) return; } }

+static int __init parse_acpi(char *arg) +{

• if (!arg)

• return -EINVAL;
  

• /* "acpi=off" disables both ACPI table parsing and interpreter */
• if (strcmp(arg, "off") == 0) {

• disable_acpi();
  

• }
• /* acpi=strict disables out-of-spec workarounds */
• else if (strcmp(arg, "strict") == 0) {

• acpi_strict = 1;
  

• }
• return 0;

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

This is a cut down version of arch/x86/kernel/acpi/boot.c. Both the 'off' and 'strict' options appear useful regardless of platform. I would consider refactoring this to have a common parse_acpi() and a call out to another function for arch specific bits.

Otherwise:

Reviewed-by: Grant Likely grant.likely@linaro.org

-- 1.7.9.5

On Fri, Apr 25, 2014 at 02:20:12PM +0100, Hanjun Guo wrote:

acpi_wakeup_address is used on x86 as the address bios jumps into when machine wakes up from suspend. As arm64 does not have such a bios this mechanism will be provided by other means. But the define is still required inside the acpi core.

Introduce a null stub for acpi_suspend_lowlevel as this is also required by core. This will be filled in when standards are defined for arm64 ACPI global power states.

Signed-off-by: Graeme Gregory graeme.gregory@linaro.org Signed-off-by: Hanjun Guo hanjun.guo@linaro.org

---

arch/arm64/include/asm/acpi.h | 4 ++++ drivers/acpi/plat/arm-core.c | 7 +++++++ 2 files changed, 11 insertions(+)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index 3ac9dfb..60e3a72 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -85,6 +85,10 @@ static inline void acpi_disable_pci(void) */ #define cpu_physical_id(cpu) -1 +/* Low-level suspend routine. */ +extern int (*acpi_suspend_lowlevel)(void); +#define acpi_wakeup_address (0)

I understand you want code to compile, but as I mentioned in another thread, I would focus more on understanding if these macros/hooks are really needed for ARM rather than defining them _just_ to make code compile. The fact that you are forced to define them makes me think there is something to be changed in ACPI core code instead of adding empty and probably useless macros/hooks for ARM which I have no idea whatsoever whether will ever be needed or not.

Lorenzo

Hi Arnd,

On 2014-4-28 21:49, Arnd Bergmann wrote:

On Friday 25 April 2014, Hanjun Guo wrote:

...

CONFIG_ACPI depends CONFIG_PCI now, and ACPI provides ACPI based PCI hotplug and PCI host bridge hotplug, introduce some PCI functions to make ACPI core can be compiled, some of the functions should be revisited when implemented on ARM64.

...

diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h index d93576f..0aa3607 100644 --- a/arch/arm64/include/asm/pci.h +++ b/arch/arm64/include/asm/pci.h @@ -21,6 +21,17 @@ struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus); #define pcibios_assign_all_busses() \ (pci_has_flag(PCI_REASSIGN_ALL_BUS)) +static inline void pcibios_penalize_isa_irq(int irq, int active) +{

• /* we don't do dynamic pci irq allocation */

+}

+static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel) +{

• /* no legacy IRQ on arm64 */
• return -ENODEV;

+}

I think these would be better addressed in the caller. From what I can tell, they are only called by the ISAPNP code path for doing IRQ resource allocation, while the ACPIPNP code uses hardwired IRQ resources.

I agree. pcibios_penalize_isa_irq() is only used by x86 and I will send out a patch to make pcibios_penalize_isa_irq() as __weak in PCI core and remove all the stub function out except x86. For pci_get_legacy_ide_irq(), I think we can fix it in the same way, does this make sense to you?

...

@@ -171,3 +172,36 @@ unsigned long pci_ioremap_io(const struct resource *res, phys_addr_t phys_addr) /* return io_offset */ return start * PAGE_SIZE - res->start; }

+/*

• • raw_pci_read - Platform-specific PCI config space access.
• • Default empty implementation. Replace with an architecture-specific setup
• • routine, if necessary.
• */

+int raw_pci_read(unsigned int domain, unsigned int bus,

•   unsigned int devfn, int reg, int len, u32 *val)
  

+{

• return -EINVAL;

+}

+int raw_pci_write(unsigned int domain, unsigned int bus,

•   unsigned int devfn, int reg, int len, u32 val)
  

+{

• return -EINVAL;

+}

+#ifdef CONFIG_ACPI +/* Root bridge scanning */ +struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) +{

• /* TODO: Should be revisited when implementing PCI on ACPI */
• return NULL;

+}

+void __init pci_acpi_crs_quirks(void) +{

• /* Do nothing on ARM64 */
• return;

+} +#endif

And these probably don't need to be done at the architecture level. I expect we will only have to worry about SBSA compliant PCI hosts, so this can be done in the host controller driver for compliant devices, which is probably the one that Will Deacon is working on already.

Note that we are aiming for an empty PCI implementation in ARM64, doing everything in either the PCI core or in the individual host drivers.

Ok, I will review Will Deacon's patch to find out the solution for pci_acpi_scan_root().

For pci_acpi_crs_quirks(), it's probably enough to stub out the declaration and to make it x86 specific. ia64 already doesn't use it, and we can hope we won't need it for arm64 either.

Yes, I agree, thanks a lot for the great suggestion :) I will send out a patch to address this according to your advice.

For raw_pci_{read,write} we can have a trivial generic implementation in the PCI core, like

int __weak raw_pci_read(unsigned int domain, unsigned int bus, unsigned int devfn, int reg, int len, u32 *val) { struct pci_bus *bus = pci_find_bus(domain, bus); if (!bus) return -ENODEV;

return bus->ops->read(bus, devfn, reg, len, val); }

which won't work on x86 or ia64, but should be fine everywhere else. Alternatively, you can change the ACPI code to do the above manually and call the architecture independent interfaces, either bus->ops->read, or pci_bus_read_config_{byte,word,dword}, which would actually simplify the ACPI code.

This may not work. ACPI needs to be able to access PCI config space before we've done a PCI bus scan and created pci_bus structures, so the pointer of bus will be NULL.

Thanks Hanjun

On 2014-4-29 17:56, Grant Likely wrote:

On Fri, 25 Apr 2014 21:20:17 +0800, Hanjun Guo hanjun.guo@linaro.org wrote:

...

Since EC (Embedded controller) needs SCI for the interupt which is not available on ARM64, make it depend on X86 || IA64 only in the Kconfig file.

Signed-off-by: Hanjun Guo hanjun.guo@linaro.org

drivers/acpi/Kconfig | 1 + 1 file changed, 1 insertion(+)

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index ae45088..391325c 100644 --- a/drivers/acpi/Kconfig +++ b/drivers/acpi/Kconfig @@ -49,6 +49,7 @@ config ACPI_SLEEP config ACPI_EC_DEBUGFS tristate "EC read/write access through /sys/kernel/debug/ec"

• depends on X86 || IA64

Nit: This patch and patch 10 should be applied before patch 9 which adds the option to enable ACPI, but otherwise this series looks reasonable to me and aside from the comments made by others is pretty much ready to be merged. Go ahead and add my reviewed-by tag to the whole series.

Reviewed-by: Grant Likely grant.likely@linaro.org

Great, we will address all the comments and send out another version.

Thanks Hanjun