On 2014-1-18 0:00, Bjorn Helgaas wrote:

[+cc Jens, James, Ben, Fujita (authors of PCI_DMA_BUS_IS_PHYS usage)]

On Fri, Jan 17, 2014 at 5:24 AM, Hanjun Guo hanjun.guo@linaro.org wrote:

...

Not all the ARM64 targets that are using ACPI have PCI, so introduce some stub functions to make PCI optional for ACPI, and make ACPI core run without CONFIG_PCI on ARM64.

pcibios_penalize_isa_irq() is arch dependent, introduce asm/pci.h to include it.

Since ACPI on X86 and IA64 depends on PCI, it will not break X86 and IA64 with this patch.

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

---

arch/arm64/include/asm/pci.h | 20 ++++++++++++++++++++ drivers/acpi/Makefile | 2 +- drivers/acpi/internal.h | 7 +++++++ drivers/acpi/osl.c | 3 ++- include/linux/pci.h | 33 ++++++++++++++++++++++++--------- 5 files changed, 54 insertions(+), 11 deletions(-) create mode 100644 arch/arm64/include/asm/pci.h

diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h new file mode 100644 index 0000000..455909d --- /dev/null +++ b/arch/arm64/include/asm/pci.h @@ -0,0 +1,20 @@ +#ifndef __ASMARM64_PCI_H +#define __ASMARM64_PCI_H

+#ifdef __KERNEL__

+static inline void pcibios_penalize_isa_irq(int irq, int active) +{

•   /* We don't do dynamic PCI IRQ allocation */
  

+}

+/*

• • The PCI address space does equal the physical memory address space.
• • The networking and block device layers use this boolean for bounce
• • buffer decisions.
• */

+#define PCI_DMA_BUS_IS_PHYS (1)

I'm not sure this is accurate. The arm code uses pci_add_resource_offset() with non-zero offsets, which means a physical memory address is not the same as a PCI bus address. I don't know what arm64 does for PCI, but I suspect it is similar.

I'm not sure either. I copied this code from arm/asm/pci.h and I thought it will be the same for ARM64.

I think PCI_DMA_BUS_IS_PHYS is due for some overhaul. I'm dubious that it is working as intended anymore. At the very least, it seems like something that is dependent on the device in question.

...

diff --git a/include/linux/pci.h b/include/linux/pci.h index a13d682..726cf2a 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h ... +static inline int raw_pci_read(unsigned int domain, unsigned int bus,

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

+{ return -EINVAL; }

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

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

+{return -EINVAL; }

Fix the spacing here (missing a space before "return").

With that, this include/linux/pci.h change is:

Acked-by: Bjorn Helgaas bhelgaas@google.com

Thanks!

Hanjun

On 17/01/14 12:24, Hanjun Guo wrote:

ACPI requires a cpu.h, add a dummy one copied from arm. This will need updated or replaced as ACPI based cpu hotplug or cpu topology for armv8 is worked out.

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

---

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

diff --git a/arch/arm64/include/asm/cpu.h b/arch/arm64/include/asm/cpu.h new file mode 100644 index 0000000..8625eb1 --- /dev/null +++ b/arch/arm64/include/asm/cpu.h @@ -0,0 +1,25 @@ +/*

• • Copyright (C) 2004-2005 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_ARM_CPU_H +#define __ASM_ARM_CPU_H

+#include <linux/percpu.h> +#include <linux/cpu.h> +#include <linux/topology.h>

+struct cpuinfo_arm {

• struct cpu cpu;
• u64 cpuid;

+#ifdef CONFIG_SMP

• unsigned int loops_per_jiffy;

IIUC we don't have loop-based delay support in ARM64. It's always timer based and in which case we don't need loops_per_jiffy

Regards, Sudeep

On Fri, Jan 17, 2014 at 12:24:56PM +0000, Hanjun Guo wrote:

ACPI requires a cpu.h, add a dummy one copied from arm. This will need updated or replaced as ACPI based cpu hotplug or cpu topology for armv8 is worked out.

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

---

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

diff --git a/arch/arm64/include/asm/cpu.h b/arch/arm64/include/asm/cpu.h new file mode 100644 index 0000000..8625eb1 --- /dev/null +++ b/arch/arm64/include/asm/cpu.h @@ -0,0 +1,25 @@ +/*

• • Copyright (C) 2004-2005 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_ARM_CPU_H +#define __ASM_ARM_CPU_H

+#include <linux/percpu.h> +#include <linux/cpu.h> +#include <linux/topology.h>

+struct cpuinfo_arm {

• struct cpu cpu;
• u64 cpuid;

+#ifdef CONFIG_SMP

• unsigned int loops_per_jiffy;

+#endif +};

+DECLARE_PER_CPU(struct cpuinfo_arm, cpu_data);

+#endif

Could you not leave this file empty until you add the code that actually makes use of cpu_data?

On 2014-1-17 22:25, Sudeep Holla wrote:

On 17/01/14 12:24, Hanjun Guo 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.

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 | 32 ++++++++++++++++++++++++++++++++ arch/arm64/include/asm/processor.h | 2 ++ arch/arm64/kernel/process.c | 3 +++ 3 files 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..cf19dc6 --- /dev/null +++ b/arch/arm64/include/asm/acpi.h @@ -0,0 +1,32 @@ +/*

• • Copyright (C) 2013, Al Stone al.stone@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.
• */

+#ifndef _ASM_ARM64_ACPI_H +#define _ASM_ARM64_ACPI_H

+static inline bool arch_has_acpi_pdc(void) +{

• return false; /* always false for now */

+}

+static inline void arch_acpi_set_pdc_bits(u32 *buf) +{

• return;

+}

+#endif /*_ASM_ARM64_ACPI_H*/ diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h index 45b20cd..50ce951 100644 --- a/arch/arm64/include/asm/processor.h +++ b/arch/arm64/include/asm/processor.h @@ -162,6 +162,8 @@ static inline void spin_lock_prefetch(const void *x) #define HAVE_ARCH_PICK_MMAP_LAYOUT +extern unsigned long boot_option_idle_override;

#endif #endif /* __ASM_PROCESSOR_H */ diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c index de17c89..13d3d7f 100644 --- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c @@ -89,6 +89,9 @@ void arch_cpu_idle_prepare(void) local_fiq_enable(); } +unsigned long boot_option_idle_override = IDLE_NO_OVERRIDE; +EXPORT_SYMBOL(boot_option_idle_override);

This is what I mentioned in other email. Do we really foresee use of this in ARM64 or it's just added to avoid build issues ?

Just avoid build issues, can not foresee use of this in ARM64 :)

Thanks Hanjun

On 2014年01月24日 00:19, Catalin Marinas wrote:

On Mon, Jan 20, 2014 at 09:20:38AM +0000, Hanjun Guo wrote:

...

On 2014-1-17 22:25, Sudeep Holla wrote:

...

On 17/01/14 12:24, Hanjun Guo wrote:

...

--- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c @@ -89,6 +89,9 @@ void arch_cpu_idle_prepare(void) local_fiq_enable(); } +unsigned long boot_option_idle_override = IDLE_NO_OVERRIDE; +EXPORT_SYMBOL(boot_option_idle_override);

This is what I mentioned in other email. Do we really foresee use of this in ARM64 or it's just added to avoid build issues ?

Just avoid build issues, can not foresee use of this in ARM64 :)

So ideally we should look for a better solution here.

Ok, I will try my best.

Thanks Hanjun

Hi Catalin,

On 2014年01月24日 02:03, Catalin Marinas wrote:

On Fri, Jan 17, 2014 at 12:24:57PM +0000, Hanjun Guo wrote:

...

--- /dev/null +++ b/arch/arm64/include/asm/acpi.h @@ -0,0 +1,32 @@ +/*

• • Copyright (C) 2013, Al Stone al.stone@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.
• */

+#ifndef _ASM_ARM64_ACPI_H +#define _ASM_ARM64_ACPI_H

Nitpick: please use __ASM_ACPI_H as a guard for consistency (the same for all the other patches introducing new header files).

ok, will update this patch.

Hanjun

On Fri, Jan 17, 2014 at 12:24:58PM +0000, Hanjun Guo wrote:

Introduce arm_core.c and its related head file, after this patch, we can get ACPI tables from firmware on ARM64 now.

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

[...]

diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index bd9bbd0..2210353 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/cputype.h> #include <asm/elf.h> @@ -225,6 +226,11 @@ void __init setup_arch(char **cmdline_p) arm64_memblock_init();

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

Do we really need *three* back-to-back calls for ACPI to initialise?

Will

On 17/01/14 12:24, Hanjun Guo wrote:

Introduce arm_core.c and its related head file, after this patch, we can get ACPI tables from firmware on ARM64 now.

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 | 57 +++++++++++ arch/arm64/kernel/setup.c | 6 ++ drivers/acpi/Makefile | 2 + drivers/acpi/plat/Makefile | 1 + drivers/acpi/plat/arm-core.c | 209 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 275 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 cf19dc6..908d71b 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -19,6 +19,43 @@ #ifndef _ASM_ARM64_ACPI_H #define _ASM_ARM64_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) { return false; /* always false for now */ @@ -29,4 +66,24 @@ 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();

+}

+/* FIXME: this function should be moved to topology.h when it's ready */ +void arch_fix_phys_package_id(int num, u32 slot);

+/* temperally define -1 to make acpi core compilerable */ +#define cpu_physical_id(cpu) -1

I assume `cpu` here is logical cpu id in which case you can define it to be same as cpu_logical_map ?

Regards, Sudeep

On Fri, Jan 17, 2014 at 12:24:58PM +0000, Hanjun Guo wrote:

[...]

diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index bd9bbd0..2210353 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/cputype.h> #include <asm/elf.h> @@ -225,6 +226,11 @@ void __init setup_arch(char **cmdline_p) arm64_memblock_init();

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

Can I ask you please why we need to parse ACPI tables before paging_init() ?

[...]

+/*

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

Again, why is this needed ? What's needed before paging_init() from ACPI ?

[...]

+/*

• • acpi_boot_table_init() and acpi_boot_init()
• • called from setup_arch(), always.
• • 1. checksums all tables
• • 2. enumerates lapics
• • 3. enumerates io-apics
• • acpi_table_init() is separated to allow reading SRAT without
• • other side effects.
• */

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

• }

+}

+int __init early_acpi_boot_init(void) +{

• /*

• * If acpi_disabled, bail out
  

• */
  

• if (acpi_disabled)

• return -ENODEV;
  

• /*

• * Process the Multiple APIC Description Table (MADT), if present
  

• */
  

• early_acpi_process_madt();
• return 0;

+}

+int __init acpi_boot_init(void) +{

• /*

• * If acpi_disabled, bail out
  

• */
  

• if (acpi_disabled)

• return -ENODEV;
  

• acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt);
• /*

• * Process the Multiple APIC Description Table (MADT), if present
  

• */
  

• acpi_process_madt();
• return 0;

+}

Well, apart from having three init calls, one returning void and two returning proper values, do not understand why, and do not understand why we need three calls in the first place...why should we process MADT twice in two separate calls ? What is supposed to change in between that prevents you from merging the two together ?

Thanks, Lorenzo

On 2014年01月23日 23:56, Tomasz Nowicki wrote:

Hi Lorenzo,

W dniu 22.01.2014 12:54, Lorenzo Pieralisi pisze:

...

On Fri, Jan 17, 2014 at 12:24:58PM +0000, Hanjun Guo wrote:

[...]

...

diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index bd9bbd0..2210353 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/cputype.h> #include <asm/elf.h> @@ -225,6 +226,11 @@ void __init setup_arch(char **cmdline_p)

arm64_memblock_init();

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

paging_init();

Can I ask you please why we need to parse ACPI tables before paging_init() ?

This is for future usage and because of couple of reasons. Mainly SRAT table parsing should be done (before paging_init()) for proper NUMA initialization and then paging_init().

Yes, I agree, thanks for Tomasz's clarification.

...

[...]

...

+/*

• • __acpi_map_table() will be called before page_init(), so

early_ioremap()

• • or early_memremap() should be called here.

Again, why is this needed ? What's needed before paging_init() from ACPI ?

[...]

...

+/*

• • acpi_boot_table_init() and acpi_boot_init()
• • called from setup_arch(), always.
• • 1. checksums all tables
• • 2. enumerates lapics
• • 3. enumerates io-apics
• • acpi_table_init() is separated to allow reading SRAT without
• • other side effects.
• */

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

+}

+int __init early_acpi_boot_init(void) +{

• /*
• • If acpi_disabled, bail out
• */
• if (acpi_disabled)
• return -ENODEV;
• /*
• • Process the Multiple APIC Description Table (MADT), if present
• */
• early_acpi_process_madt();
• return 0;

+}

+int __init acpi_boot_init(void) +{

• /*
• • If acpi_disabled, bail out
• */
• if (acpi_disabled)
• return -ENODEV;
• acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt);
• /*
• • Process the Multiple APIC Description Table (MADT), if present
• */
• acpi_process_madt();
• return 0;

+}

Well, apart from having three init calls, one returning void and two returning proper values, do not understand why, and do not understand why we need three calls in the first place...why should we process MADT twice in two separate calls ? What is supposed to change in between that prevents you from merging the two together ?

Thanks for pointing this out. I can merge acpi_boot_table_init() and early_acpi_boot_init() together, but can not merge early_acpi_boot_init() and acpi_boot_init() together.

early_acpi_boot_init() and acpi_boot_init() was separated intentionally for memory hotplug reasons. memory allocated in this stage can not be migrated and cause memory hot-remove failed, in order to keep memory allocated at base node (general NUMA node 0 in the system) at boot stage, we should parse SRAT first before CPU is enumerated, does this make sense to you?

Thanks Hanjun

Hi Lorenzo,

W dniu 24.01.2014 13:53, Lorenzo Pieralisi pisze:

Hi Hanjun,

On Fri, Jan 24, 2014 at 09:09:40AM +0000, Hanjun Guo wrote:

...

On 2014?01?23? 23:56, Tomasz Nowicki wrote:

...

Hi Lorenzo,

W dniu 22.01.2014 12:54, Lorenzo Pieralisi pisze:

...

On Fri, Jan 17, 2014 at 12:24:58PM +0000, Hanjun Guo wrote:

[...]

...

diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c index bd9bbd0..2210353 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/cputype.h> #include <asm/elf.h> @@ -225,6 +226,11 @@ void __init setup_arch(char **cmdline_p)

arm64_memblock_init();

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

paging_init();

Can I ask you please why we need to parse ACPI tables before paging_init() ?

This is for future usage and because of couple of reasons. Mainly SRAT table parsing should be done (before paging_init()) for proper NUMA initialization and then paging_init().

Thank you for the explanation. I still have some questions:

1. What are the other reasons ?

If we agreed that we need SRAT parsing before paging_init() the second reason is all implications related to that. I mean if we want to make ACPI tables parseable before paging_init(), we need early_ioremap mechanism and __acpi_map_table() fixes. In that case, IMHO, it is better to please it in the right place now. early_ioremap is object of UEFI support patch set.

2. NUMA is not supported at the moment and I reckon SRAT needs updating since the only way to associate a CPU to a memory node is through a local APIC id that is non-existent on ARM and at least deserves a new entry.

Right, that requires further work on SRAT.

I am still not sure that providing a hook for parsing the ACPI tables before paging_init() is the main focus at the moment, it is probably best, as we've mentioned manifold times, to make sure that the infrastructure to detect ACPI vs DT at run-time is in place in the kernel and allows us to boot either with ACPI or DT, in a mutual exclusive way (same binary kernel supporting both, runtime detection/decision on what data to use, ACPI tables vs DT nodes, detection made once for all and NOT on a per property basis).

I will have a look at SRAT and how it is used on x86, and get back to you on this.

[...]

...

...

...

...

• • acpi_boot_table_init() and acpi_boot_init()
• • called from setup_arch(), always.
• • 1. checksums all tables
• • 2. enumerates lapics
• • 3. enumerates io-apics
• • acpi_table_init() is separated to allow reading SRAT without
• • other side effects.
• */

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

+}

+int __init early_acpi_boot_init(void) +{

• /*
• • If acpi_disabled, bail out
• */
• if (acpi_disabled)
• return -ENODEV;
• /*
• • Process the Multiple APIC Description Table (MADT), if present
• */
• early_acpi_process_madt();
• return 0;

+}

+int __init acpi_boot_init(void) +{

• /*
• • If acpi_disabled, bail out
• */
• if (acpi_disabled)
• return -ENODEV;
• acpi_table_parse(ACPI_SIG_FADT, acpi_parse_fadt);
• /*
• • Process the Multiple APIC Description Table (MADT), if present
• */
• acpi_process_madt();
• return 0;

+}

Well, apart from having three init calls, one returning void and two returning proper values, do not understand why, and do not understand why we need three calls in the first place...why should we process MADT twice in two separate calls ? What is supposed to change in between that prevents you from merging the two together ?

Thanks for pointing this out. I can merge acpi_boot_table_init() and early_acpi_boot_init() together, but can not merge early_acpi_boot_init() and acpi_boot_init() together.

early_acpi_boot_init() and acpi_boot_init() was separated intentionally for memory hotplug reasons. memory allocated in this stage can not be migrated and cause memory hot-remove failed, in order to keep memory allocated at base node (general NUMA node 0 in the system) at boot stage, we should parse SRAT first before CPU is enumerated, does this make sense to you?

Are you parsing the SRAT in this series to get memory info or memory is still initialized by DT even when system is supposed to be booted with ACPI (ie there is a valid ACPI root table ?)

I have a hunch the latter is what's happening (and that's wrong, because memory information when kernel is booted through ACPI must be retrieved from UEFI - at least that's what has been defined), so I still see an early hook to initialize ACPI tables before paging_init() that has no use as the current patchset stands, please correct me if I am wrong.

Yes, currently memory is initialized by DT but getting memory map from UEFI is the matter of time (pending upstream process). Also, SRAT table is not parsing in this series but it will, and I think it plays the main role to put it before paging_init.

Regards, Tomasz

On 2014-1-17 22:04, Arnd Bergmann wrote:

On Friday 17 January 2014, Hanjun Guo wrote:

...

+++ b/arch/arm64/pci/Makefile @@ -0,0 +1 @@ +obj-y += pci.o diff --git a/arch/arm64/pci/pci.c b/arch/arm64/pci/pci.c new file mode 100644 index 0000000..4e46790 --- /dev/null +++ b/arch/arm64/pci/pci.c @@ -0,0 +1,33 @@ +#include <linux/acpi.h> +#include <linux/types.h> +#include <linux/kernel.h> +#include <linux/pci.h>

+/**

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

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

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

+{

• return -EINVAL;

+}

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

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

+{

• return -EINVAL;

+}

I'd rather not see __weak functions here. Just provide them unconditionally so that we can add a proper implementation when needed. You could also define these as 'static inline' in a header file to keep them from consuming space in the object code.

Ok, I will remove __weak in next version.

...

diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c index 3c8521d..1835b21 100644 --- a/drivers/acpi/plat/arm-core.c +++ b/drivers/acpi/plat/arm-core.c @@ -100,6 +100,25 @@ int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) } EXPORT_SYMBOL_GPL(acpi_gsi_to_irq); +int acpi_isa_irq_to_gsi(unsigned isa_irq, u32 *gsi) +{

• return -1;

+}

+int acpi_register_ioapic(acpi_handle handle, u64 phys_addr, u32 gsi_base) +{

• /* TBD */
• return -EINVAL;

+} +EXPORT_SYMBOL(acpi_register_ioapic);

+int acpi_unregister_ioapic(acpi_handle handle, u32 gsi_base) +{

• /* TBD */
• return -EINVAL;

+} +EXPORT_SYMBOL(acpi_unregister_ioapic);

My feeling is that these are better handled in the ACPI code by not calling them on architectures that have no ISA or no IOAPIC support.

We have configuration symbols for both, so you don't have to make it depend on CONFIG_ARM64 or CONFIG_X86.

Do you mean introduce a stub function when there is no ISA support?

acpi_register_ioapic()/acpi_unregister_ioapic() will be used for IOAPIC hotplug and GIC distributor is something like IOAPIC on x86, so I think these two functions can be reserved for future use.

Thanks Hanjun

On 2014年01月20日 16:20, Arnd Bergmann wrote:

On Monday 20 January 2014 16:08:01 Hanjun Guo wrote:

...

...

...

diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c index 3c8521d..1835b21 100644 --- a/drivers/acpi/plat/arm-core.c +++ b/drivers/acpi/plat/arm-core.c @@ -100,6 +100,25 @@ int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) } EXPORT_SYMBOL_GPL(acpi_gsi_to_irq); +int acpi_isa_irq_to_gsi(unsigned isa_irq, u32 *gsi) +{

• return -1;

+}

+int acpi_register_ioapic(acpi_handle handle, u64 phys_addr, u32 gsi_base) +{

• /* TBD */
• return -EINVAL;

+} +EXPORT_SYMBOL(acpi_register_ioapic);

+int acpi_unregister_ioapic(acpi_handle handle, u32 gsi_base) +{

• /* TBD */
• return -EINVAL;

+} +EXPORT_SYMBOL(acpi_unregister_ioapic);

My feeling is that these are better handled in the ACPI code by not calling them on architectures that have no ISA or no IOAPIC support.

We have configuration symbols for both, so you don't have to make it depend on CONFIG_ARM64 or CONFIG_X86.

Do you mean introduce a stub function when there is no ISA support?

Do you anticipate ISA devices on ARM64? I hope not ;-)

me too :)

My guess is that whatever code calls this function should be disabled in reduced hw mode.

ok, that would be make sense, will update it in next version.

...

acpi_register_ioapic()/acpi_unregister_ioapic() will be used for IOAPIC hotplug and GIC distributor is something like IOAPIC on x86, so I think these two functions can be reserved for future use.

But GIC is not hotplugged, is it? It still sounds x86 specific to me.

Well, if we want to do physical CPU hotplug on ARM/ARM64 (maybe years later?), then GIC add/remove is needed because we have to remove GIC on the SoC too when we remove the physical CPU.

Thanks Hanjun

On 17/01/14 12:25, Hanjun Guo wrote:

Add Kconfigs to build ACPI on ARM64, and make ACPI runable on ARM64.

acpi_idle driver is x86/IA64 dependent now, so make CONFIG_ACPI_PROCESSOR depends on X86 || IA64, and implement it on ARM/ARM64 in the furture.

In order to make arm-core.c can both run on ARM and ARM64, introduce CONFIG_ACPI_ARM to support it.

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

---

arch/arm64/Kconfig | 2 ++ drivers/acpi/Kconfig | 11 ++++++++--- drivers/acpi/plat/Makefile | 2 +- 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 6d4dd22..2b1fb1d 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -279,6 +279,8 @@ source "net/Kconfig" source "drivers/Kconfig" +source "drivers/acpi/Kconfig"

source "fs/Kconfig" source "arch/arm64/kvm/Kconfig" diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index 4770de5..cae5dc9 100644 --- a/drivers/acpi/Kconfig +++ b/drivers/acpi/Kconfig @@ -2,13 +2,16 @@ # ACPI Configuration # +config ACPI_ARM

• bool

menuconfig ACPI bool "ACPI (Advanced Configuration and Power Interface) Support" depends on !IA64_HP_SIM

• depends on IA64 || X86
• depends on PCI

• depends on ((IA64 || X86) && PCI) || ARM64 select PNP

• default y

• select ACPI_ARM if ARM64
• default y if !ARM64 help Advanced Configuration and Power Interface (ACPI) support for Linux requires an ACPI-compliant platform (hardware/firmware),

@@ -149,6 +152,7 @@ config ACPI_PROCESSOR tristate "Processor" select THERMAL select CPU_IDLE

• depends on X86 || IA64

This doesn't look correct to me. You are disabling Perf/Idle/Thermal/Throttling support for ARM64. Any particular reason ?

Also this is conflicting with other change where you are adding/exporting boot_option_idle_override which is mainly used in processor_idle and that's disabled for ARM64 here.

Regards, Sudeep

On Fri, Jan 17, 2014 at 12:25:01PM +0000, Hanjun Guo wrote:

Add Kconfigs to build ACPI on ARM64, and make ACPI runable on ARM64.

acpi_idle driver is x86/IA64 dependent now, so make CONFIG_ACPI_PROCESSOR depends on X86 || IA64, and implement it on ARM/ARM64 in the furture.

In order to make arm-core.c can both run on ARM and ARM64, introduce CONFIG_ACPI_ARM to support it.

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

---

arch/arm64/Kconfig | 2 ++ drivers/acpi/Kconfig | 11 ++++++++--- drivers/acpi/plat/Makefile | 2 +- 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 6d4dd22..2b1fb1d 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -279,6 +279,8 @@ source "net/Kconfig" source "drivers/Kconfig" +source "drivers/acpi/Kconfig"

source "fs/Kconfig" source "arch/arm64/kvm/Kconfig" diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index 4770de5..cae5dc9 100644 --- a/drivers/acpi/Kconfig +++ b/drivers/acpi/Kconfig @@ -2,13 +2,16 @@ # ACPI Configuration # +config ACPI_ARM

• bool

Could be better as def_bool ARM64

menuconfig ACPI bool "ACPI (Advanced Configuration and Power Interface) Support" depends on !IA64_HP_SIM

• depends on IA64 || X86
• depends on PCI

• depends on ((IA64 || X86) && PCI) || ARM64 select PNP

• default y

• select ACPI_ARM if ARM64

And remove this select here.

Hi Hanjun,

On 17/01/14 12:25, Hanjun Guo wrote:

Implement core functions for parsing MADT table to get the information about GIC cpu interface and GIC distributor to prepare for SMP and GIC initialization.

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

arch/arm64/include/asm/acpi.h | 3 + drivers/acpi/plat/arm-core.c | 139 ++++++++++++++++++++++++++++++++++++++++- drivers/acpi/tables.c | 21 +++++++ 3 files changed, 162 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index e108d9c..c335c6d 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -83,6 +83,9 @@ void arch_fix_phys_package_id(int num, u32 slot); extern int (*acpi_suspend_lowlevel)(void); #define acpi_wakeup_address (0) +#define MAX_GIC_CPU_INTERFACE 256

I'll bite. Where on Earth is this value coming from? If that's for GICv2, 8 is the maximum. For GICv3+, that's incredibly low, and should be probed probed at runtime anyway.

+#define MAX_GIC_DISTRIBUTOR 1 /* should be the same as MAX_GIC_NR */

No support for cascaded GICs?

#else /* !CONFIG_ACPI */ #define acpi_disabled 1 /* ACPI sometimes enabled on ARM */ #define acpi_noirq 1 /* ACPI sometimes enabled on ARM */ diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c index 1835b21..8ba3e6f 100644 --- a/drivers/acpi/plat/arm-core.c +++ b/drivers/acpi/plat/arm-core.c @@ -46,6 +46,16 @@ EXPORT_SYMBOL(acpi_disabled); int acpi_pci_disabled; /* skip ACPI PCI scan and IRQ initialization */ EXPORT_SYMBOL(acpi_pci_disabled); +/*

• • Local interrupt controller address,
• • GIC cpu interface base address on ARM/ARM64
• */

+static u64 acpi_lapic_addr __initdata;

If that's a GIC address, why not call it as such?

+#define BAD_MADT_ENTRY(entry, end) ( \

• (!entry) || (unsigned long)entry + sizeof(*entry) > end || \
• ((struct acpi_subtable_header *)entry)->length < sizeof(*entry))

#define PREFIX "ACPI: "

Just do: #define pr_fmt(fmt) "ACPI: " fmt

and remove all the occurrences of PREFIX.

/* FIXME: this function should be moved to topology.c when it is ready */ @@ -92,6 +102,115 @@ void __init __acpi_unmap_table(char *map, unsigned long size) return; } +static int __init acpi_parse_madt(struct acpi_table_header *table) +{

• struct acpi_table_madt *madt = NULL;

No need to initialize this to NULL, you're doing an assignment at the next line...

• madt = (struct acpi_table_madt *)table;
• if (!madt) {

• pr_warn(PREFIX "Unable to map MADT\n");
  

There is no mapping here, please fix the message accordingly.

• return -ENODEV;
  

• }
• if (madt->address) {

• acpi_lapic_addr = (u64) madt->address;
  

So you're updating this static variable, for the distributor and each CPU interface? /me puzzled...

• pr_info(PREFIX "Local APIC address 0x%08x\n", madt->address);
  

Away with this APIC madness. GICC and GICD are the concepts we're all familiar with here, and using the proper terminology would certainly help reviewing these patches...

• }
• return 0;

+}

+/*

• • GIC structures on ARM are somthing like Local APIC structures on x86,
• • which means GIC cpu interfaces for GICv2/v3. Every GIC structure in
• • MADT table represents a cpu in the system.

And what do you do when your GICv3 doesn't have a memory-mapped interface, but only uses system registers?

• • GIC distributor structures are somthing like IOAPIC on x86. GIC can
• • be initialized with information in this structure.
• • Please refer to chapter5.2.12.14/15 of ACPI 5.0

A pointer to that documentation?

• */

+static int __init +acpi_parse_gic(struct acpi_subtable_header *header, const unsigned long end) +{

• struct acpi_madt_generic_interrupt *processor = NULL;
• processor = (struct acpi_madt_generic_interrupt *)header;
• if (BAD_MADT_ENTRY(processor, end))

• return -EINVAL;
  

• acpi_table_print_madt_entry(header);
• return 0;

+}

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

• 		const unsigned long end)
  

+{

• struct acpi_madt_generic_distributor *distributor = NULL;
• distributor = (struct acpi_madt_generic_distributor *)header;
• if (BAD_MADT_ENTRY(distributor, end))

• return -EINVAL;
  

• acpi_table_print_madt_entry(header);
• return 0;

+}

+/*

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

+static int __init acpi_parse_madt_gic_entries(void) +{

• int count;
• /*

• * do a partial walk of MADT to determine how many CPUs
  

• * we have including disabled CPUs
  

• */
  

• count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT,

• 		acpi_parse_gic, MAX_GIC_CPU_INTERFACE);
  

• if (!count) {

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

• return -ENODEV;
  

• } else if (count < 0) {

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

• return count;
  

• }

So you do a lot of parsing to count stuff, and then discard the number of counted objects... You might as well check that there is at least one valid object and stop there.

• return 0;

+}

+/*

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

+static int __init acpi_parse_madt_gic_distributor_entries(void) +{

• int count;
• count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR,

• 	acpi_parse_gic_distributor, MAX_GIC_DISTRIBUTOR);
  

• if (!count) {

• pr_err(PREFIX "No GIC distributor entries present\n");
  

• return -ENODEV;
  

• } else if (count < 0) {

• pr_err(PREFIX "Error parsing GIC distributor entry\n");
  

• return count;
  

• }
• return 0;

+}

int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) { *irq = gsi_to_irq(gsi); @@ -141,11 +260,29 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table) static void __init early_acpi_process_madt(void) {

• return;

• acpi_table_parse(ACPI_SIG_MADT, acpi_parse_madt);

} static void __init acpi_process_madt(void) {

• int error;
• if (!acpi_table_parse(ACPI_SIG_MADT, acpi_parse_madt)) {

How many times are you going to parse the same table? Surely you can stash whatever information you need and be done with it?

• /*
  

•  * Parse MADT GIC cpu interface entries
  

•  */
  

• error = acpi_parse_madt_gic_entries();
  

• if (!error) {
  

• 	/*
  

• 	 * Parse MADT GIC distributor entries
  

• 	 */
  

• 	acpi_parse_madt_gic_distributor_entries();
  

• }
  

• }
• pr_info("Using ACPI for processor (GIC) configuration information\n");
• return;

} diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index d67a1fe..b3e4615 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -191,6 +191,27 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) } break;

• case ACPI_MADT_TYPE_GENERIC_INTERRUPT:

• {
  

• 	struct acpi_madt_generic_interrupt *p =
  

• 		(struct acpi_madt_generic_interrupt *)header;
  

• 	printk(KERN_INFO PREFIX
  

Use pr_info

• 	       "GIC (acpi_id[0x%04x] gic_id[0x%04x] %s)\n",
  

• 	       p->uid, p->gic_id,
  

• 	       (p->flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
  

• }
  

• break;
  

• case ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR:

• {
  

• 	struct acpi_madt_generic_distributor *p =
  

• 		(struct acpi_madt_generic_distributor *)header;
  

• 	printk(KERN_INFO PREFIX
  

• 	       "GIC Distributor (id[0x%04x] address[0x%08llx] gsi_base[%d])\n",
  

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

• }
  

• break;
  

• default: printk(KERN_WARNING PREFIX "Found unsupported MADT entry (type = 0x%x)\n",

Most of that code seems to be repeatedly parsing and printing stuff, and I fail to see what it actually does.

M.

On Friday 24 January 2014, Hanjun Guo wrote:

...

...

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index e108d9c..c335c6d 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -83,6 +83,9 @@ void arch_fix_phys_package_id(int num, u32 slot); extern int (*acpi_suspend_lowlevel)(void); #define acpi_wakeup_address (0) +#define MAX_GIC_CPU_INTERFACE 256

I'll bite. Where on Earth is this value coming from?

I just thought 256 is big enough for now :( Yes, should be a larger number for GICv3.

Could this just be set to NR_CPUS? That way it will be large enough for any system you can actually run on.

Arnd

On 17/01/14 12:25, Hanjun Guo wrote:

When boot the kernel with MADT, the cpu possible and present sets should be enumerated for later smp initialization.

The logic cpu id maps to APIC id (GIC id) is also implemented, it is needed for acpi processor drivers.

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

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

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h index c335c6d..7edd39e 100644 --- a/arch/arm64/include/asm/acpi.h +++ b/arch/arm64/include/asm/acpi.h @@ -76,9 +76,6 @@ static inline void acpi_disable_pci(void) /* FIXME: this function should be moved to topology.h when it's ready */ void arch_fix_phys_package_id(int num, u32 slot); -/* temperally define -1 to make acpi core compilerable */ -#define cpu_physical_id(cpu) -1

/* Low-level suspend routine. */ extern int (*acpi_suspend_lowlevel)(void); #define acpi_wakeup_address (0) @@ -86,6 +83,10 @@ extern int (*acpi_suspend_lowlevel)(void); #define MAX_GIC_CPU_INTERFACE 256 #define MAX_GIC_DISTRIBUTOR 1 /* should be the same as MAX_GIC_NR */ +/* map logic cpu id to physical GIC id */ +extern int arm_cpu_to_apicid[NR_CPUS]; +#define cpu_physical_id(cpu) arm_cpu_to_apicid[cpu]

You refer arm_cpu_to_apicid as a mapping from logical cpu id to GIC id, then why is it assigned to cpu_physical_id. cpu_physical_id must be same as cpu_logical_map which is from logical cpu id to MPIDRs.

[...]

@@ -321,6 +401,9 @@ int __init early_acpi_boot_init(void) if (acpi_disabled) return -ENODEV;

• /* Get the boot CPU's GIC cpu interface id before MADT parsing */
• boot_cpu_apic_id = read_cpuid_mpidr() & MPIDR_HWID_BITMASK;

Code contradicts your comment. You need GIC cpu interface ID(referred as GIC ID in the MADT), but you are assigning the MPIDR(physical CPU ID on ARM) to boot_cpu_apic_id. I think you are assuming GIC ID and MPIDR to be same which is wrong.

With ACPI5.0, IIUC the only possible way to manage mapping from GIC CPU interface to MPIDR is to assume(in a way enforce on ARM) ACPI Processor UID in MADT and CPU Device definition match MPIDR.

Regards, Sudeep

On Fri, Jan 17, 2014 at 12:25:04PM +0000, Hanjun Guo wrote:

[...]

+/* map logic cpu id to physical GIC id */ +extern int arm_cpu_to_apicid[NR_CPUS]; +#define cpu_physical_id(cpu) arm_cpu_to_apicid[cpu]

Sudeep already commented on this, please update it accordingly.

#else /* !CONFIG_ACPI */ #define acpi_disabled 1 /* ACPI sometimes enabled on ARM */ #define acpi_noirq 1 /* ACPI sometimes enabled on ARM */ diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c index 8ba3e6f..1d9b789 100644 --- a/drivers/acpi/plat/arm-core.c +++ b/drivers/acpi/plat/arm-core.c @@ -31,6 +31,7 @@ #include <linux/smp.h> #include <asm/pgtable.h> +#include <asm/cputype.h> /*

• We never plan to use RSDT on arm/arm64 as its deprecated in spec but this

@@ -52,6 +53,13 @@ EXPORT_SYMBOL(acpi_pci_disabled); */ static u64 acpi_lapic_addr __initdata;

Is this variable actually needed ?

+/* available_cpus here means enabled cpu in MADT */ +static int available_cpus;

Ditto.

+/* Map logic cpu id to physical GIC id (physical CPU id). */ +int arm_cpu_to_apicid[NR_CPUS] = { [0 ... NR_CPUS-1] = -1 }; +static int boot_cpu_apic_id = -1;

Do we need all these variables ? I think we should reuse cpu_logical_map data structures for that, it looks suspiciously familiar.

#define BAD_MADT_ENTRY(entry, end) ( \ (!entry) || (unsigned long)entry + sizeof(*entry) > end || \ ((struct acpi_subtable_header *)entry)->length < sizeof(*entry)) @@ -132,6 +140,55 @@ static int __init acpi_parse_madt(struct acpi_table_header *table)

• Please refer to chapter5.2.12.14/15 of ACPI 5.0

*/ +/**

• • acpi_register_gic_cpu_interface - register a gic cpu interface and
• • generates a logic cpu number
• • @id: gic cpu interface id to register
• • @enabled: this cpu is enabled or not
• • Returns the logic cpu number which maps to the gic cpu interface
• */

+static int acpi_register_gic_cpu_interface(int id, u8 enabled) +{

• int cpu;
• if (id >= MAX_GIC_CPU_INTERFACE) {

• pr_info(PREFIX "skipped apicid that is too big\n");
  

• return -EINVAL;
  

• }
• total_cpus++;
• if (!enabled)

• return -EINVAL;
  

• if (available_cpus >= NR_CPUS) {

• pr_warn(PREFIX "NR_CPUS limit of %d reached,"
  

• " Processor %d/0x%x ignored.\n", NR_CPUS, total_cpus, id);
  

• return -EINVAL;
  

• }

Hmm...what if you are missing the boot CPU ? It is a worthy check. Have a look at smp_init_cpus(), it does not bail out on cpu>= NR_CPUS because you do want to make sure that the DT contains the boot CPU node. Same logic applies.

• available_cpus++;

Is available_cpus != num_possible_cpus() ? It does not look like hence available_cpus can go.

• /* allocate a logic cpu id for the new comer */
• if (boot_cpu_apic_id == id) {

• /*
  

•  * boot_cpu_init() already hold bit 0 in cpu_present_mask
  

•  * for BSP, no need to allocte again.
  

•  */
  

• cpu = 0;
  

• } else {

• cpu = cpumask_next_zero(-1, cpu_present_mask);
  

• }
• /* map the logic cpu id to APIC id */
• arm_cpu_to_apicid[cpu] = id;
• set_cpu_present(cpu, true);
• set_cpu_possible(cpu, true);

This is getting nasty. Before adding this patch and previous ones we need to put in place a method for the kernel to make a definite choice between ACPI and DT and stick to that. We can't initialize the logical map twice (which will happen if your DT has valid cpu nodes and a chosen node pointing to proper ACPI tables) or even having some entries initialized from DT and others by ACPI. It is a big fat no-no, please update the series accordingly.

• return cpu;

+}

static int __init acpi_parse_gic(struct acpi_subtable_header *header, const unsigned long end) { @@ -144,6 +201,16 @@ acpi_parse_gic(struct acpi_subtable_header *header, const unsigned long end) acpi_table_print_madt_entry(header);

• /*

• * We need to register disabled CPU as well to permit
  

• * counting disabled CPUs. This allows us to size
  

• * cpus_possible_map more accurately, to permit
  

• * to not preallocating memory for all NR_CPUS
  

• * when we use CPU hotplug.
  

• */
  

• acpi_register_gic_cpu_interface(processor->gic_id,

• 	processor->flags & ACPI_MADT_ENABLED);
  

• return 0;

} @@ -186,6 +253,19 @@ static int __init acpi_parse_madt_gic_entries(void) return count; } +#ifdef CONFIG_SMP

• if (available_cpus == 0) {

• pr_info(PREFIX "Found 0 CPUs; assuming 1\n");
  

• arm_cpu_to_apicid[available_cpus] =
  

• 	read_cpuid_mpidr() & MPIDR_HWID_BITMASK;
  

• available_cpus = 1;	/* We've got at least one of these */
  

• }

I'd rather check the MADT for at least the boot cpu to present, if it is not ACPI tables are horribly buggy and the kernel should barf on that.

+#endif

• /* Make boot-up look pretty */
• pr_info("%d CPUs available, %d CPUs total\n", available_cpus,

• total_cpus);
  

Ok, now, how can we use the "disabled" CPUs == (total_cpus - available_cpus) ?

Are we keeping track of them in the kernel at all ? It does not look like, so I wonder whether we need this bit of info. I do not see why it is pretty to know that there are disabled, aka unusable CPUs.

return 0; } @@ -321,6 +401,9 @@ int __init early_acpi_boot_init(void) if (acpi_disabled) return -ENODEV;

• /* Get the boot CPU's GIC cpu interface id before MADT parsing */
• boot_cpu_apic_id = read_cpuid_mpidr() & MPIDR_HWID_BITMASK;

See Sudeep's comment.

Thanks, Lorenzo

On Fri, Jan 24, 2014 at 02:37:28PM +0000, Hanjun Guo wrote:

Hi Lorenzo,

On 2014?01?22? 23:53, Lorenzo Pieralisi wrote:

...

On Fri, Jan 17, 2014 at 12:25:04PM +0000, Hanjun Guo wrote:

[...]

...

+/* map logic cpu id to physical GIC id */ +extern int arm_cpu_to_apicid[NR_CPUS]; +#define cpu_physical_id(cpu) arm_cpu_to_apicid[cpu]

Sudeep already commented on this, please update it accordingly.

Actually after some careful review of the ACPI code, I can't update it as MPIDR here.

MPIDR can be the ACPI uid and NOT the GIC id, the mapping of them are something like this in ACPI driver now:

logic cpu id <---> APIC Id (GIC ID) <---> ACPI uid (MPIDR on ARM) but not logic cpu id <---> ACPI uid directly, you can refer to the code of processor_core.c

So here I can only map GIC id to logical cpu id.

On ARM platforms GIC CPU IF id is probeable, you do not need to parse it (ie it is not information that you will find in DT). Please have a look at drivers/irqchip/irq-gic.c.

We have to understand what's really required and when in ACPI, or to put it differently, why cpu_physical_id(cpu) is required and at what time at boot, I will have a look on my side too.

...

...

• #else /* !CONFIG_ACPI */ #define acpi_disabled 1 /* ACPI sometimes enabled on ARM */ #define acpi_noirq 1 /* ACPI sometimes enabled on ARM */

diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c index 8ba3e6f..1d9b789 100644 --- a/drivers/acpi/plat/arm-core.c +++ b/drivers/acpi/plat/arm-core.c @@ -31,6 +31,7 @@ #include <linux/smp.h> #include <asm/pgtable.h> +#include <asm/cputype.h> /*

• We never plan to use RSDT on arm/arm64 as its deprecated in spec but this

@@ -52,6 +53,13 @@ EXPORT_SYMBOL(acpi_pci_disabled); */ static u64 acpi_lapic_addr __initdata;

Is this variable actually needed ?

Yes, needed for GIC initialization.

...

...

+/* available_cpus here means enabled cpu in MADT */ +static int available_cpus;

Ditto.

...

+/* Map logic cpu id to physical GIC id (physical CPU id). */ +int arm_cpu_to_apicid[NR_CPUS] = { [0 ... NR_CPUS-1] = -1 }; +static int boot_cpu_apic_id = -1;

Do we need all these variables ? I think we should reuse cpu_logical_map data structures for that, it looks suspiciously familiar.

MPIDR is the different part. if we use MPIDR as GIC id, i think we can reuse cpu_logical_map, but Sudeep suggested not use MPIDR as GIC id.

It is not about *reusing* cpu_logical_map, it is about setting it up properly. cpu_logical_map must be initialized by ACPI for the spin table method to work properly (and PSCI too).

And yes, cpu_physical_id(cpu) is expected to be the GIC CPU IF id on ARM, at least it looks like, I had a look too. But this does not change anything as far as cpu_logical_map is concerned, it must contain a list of MPIDRs in the system and must be retrieved via ACPI, not DT CPU nodes when ACPI is used for booting.

I will have a further look, since this discrepancy is annoying.

[...]

...

...

• available_cpus++;

Is available_cpus != num_possible_cpus() ? It does not look like hence available_cpus can go.

No, possible cpus include available cpus and disabled cpus this is useful for ACPI based CPU hot-plug features.

...

...

• /* allocate a logic cpu id for the new comer */
• if (boot_cpu_apic_id == id) {

• /*
  

•  * boot_cpu_init() already hold bit 0 in cpu_present_mask
  

•  * for BSP, no need to allocte again.
  

•  */
  

• cpu = 0;
  

• } else {

• cpu = cpumask_next_zero(-1, cpu_present_mask);
  

• }
• /* map the logic cpu id to APIC id */
• arm_cpu_to_apicid[cpu] = id;
• set_cpu_present(cpu, true);
• set_cpu_possible(cpu, true);

This is getting nasty. Before adding this patch and previous ones we need to put in place a method for the kernel to make a definite choice between ACPI and DT and stick to that. We can't initialize the logical map twice (which will happen if your DT has valid cpu nodes and a chosen node pointing to proper ACPI tables) or even having some entries initialized from DT and others by ACPI. It is a big fat no-no, please update the series accordingly.

really good catch here :) so the problem here is that should we use both ACPI and DT in one system?

...

...

• return cpu;

+}

• static int __init acpi_parse_gic(struct acpi_subtable_header *header, const unsigned long end) {

@@ -144,6 +201,16 @@ acpi_parse_gic(struct acpi_subtable_header *header, const unsigned long end) acpi_table_print_madt_entry(header);

• /*

• * We need to register disabled CPU as well to permit
  

• * counting disabled CPUs. This allows us to size
  

• * cpus_possible_map more accurately, to permit
  

• * to not preallocating memory for all NR_CPUS
  

• * when we use CPU hotplug.
  

• */
  

• acpi_register_gic_cpu_interface(processor->gic_id,

• 	processor->flags & ACPI_MADT_ENABLED);
  

• return 0; }

@@ -186,6 +253,19 @@ static int __init acpi_parse_madt_gic_entries(void) return count; } +#ifdef CONFIG_SMP

• if (available_cpus == 0) {

• pr_info(PREFIX "Found 0 CPUs; assuming 1\n");
  

• arm_cpu_to_apicid[available_cpus] =
  

• 	read_cpuid_mpidr() & MPIDR_HWID_BITMASK;
  

• available_cpus = 1;	/* We've got at least one of these */
  

• }

I'd rather check the MADT for at least the boot cpu to present, if it is not ACPI tables are horribly buggy and the kernel should barf on that.

...

+#endif

• /* Make boot-up look pretty */
• pr_info("%d CPUs available, %d CPUs total\n", available_cpus,

• total_cpus);
  

Ok, now, how can we use the "disabled" CPUs == (total_cpus - available_cpus) ?

For cpus can be hot-added later when system is running.

I do not see any usage in the patchset and certainly those variables are not used in this patch, apart from printing messages whose usefulness is debatable. If, as you say, you are using those variables for something else, please add code in the patch where they are introduced for it to be self-contained and to simplify the review.

Thanks, Lorenzo

Hi Catalin,

On 2014年01月24日 01:50, Catalin Marinas wrote:

On Fri, Jan 17, 2014 at 12:25:05PM +0000, Hanjun Guo wrote:

...

--- a/arch/arm64/kernel/smp.c +++ b/arch/arm64/kernel/smp.c @@ -48,6 +48,7 @@ #include <asm/sections.h> #include <asm/tlbflush.h> #include <asm/ptrace.h> +#include <asm/acpi.h> /*

• as from 2.5, kernels no longer have an init_tasks structure

@@ -280,7 +281,7 @@ static void (*smp_cross_call)(const struct cpumask *, unsigned int);

• cpu logical map array containing MPIDR values related to logical
• cpus. Assumes that cpu_logical_map(0) has already been initialized.

*/ -void __init smp_init_cpus(void) +static int __init of_smp_init_cpus(void) { struct device_node *dn = NULL; unsigned int i, cpu = 1; @@ -364,6 +365,10 @@ next: cpu++; }

• /* No device tree or no CPU node in DT */
• if (cpu == 1 && !bootcpu_valid)

• return -ENODEV;
  

• /* sanity check */ if (cpu > NR_CPUS) pr_warning("no. of cores (%d) greater than configured maximum of %d - clipping\n",

@@ -371,7 +376,7 @@ next: if (!bootcpu_valid) { pr_err("DT missing boot CPU MPIDR, not enabling secondaries\n");

• return;
  

• return -EINVAL;
  

  }

/* @@ -381,6 +386,39 @@ next: for (i = 0; i < NR_CPUS; i++) if (cpu_logical_map(i) != INVALID_HWID) set_cpu_possible(i, true);

• return 0;

+}

+/*

• • In ACPI mode, the cpu possible map was enumerated before SMP
• • initialization when MADT table was parsed, so we can get the
• • possible map here to initialize CPUs.
• */

+static void __init acpi_smp_init_cpus(void) +{

• int cpu;
• const char *enable_method;
• for_each_possible_cpu(cpu) {

• enable_method = acpi_get_enable_method(cpu);
  

• if (!enable_method)
  

• 	continue;
  

• cpu_ops[cpu] = cpu_get_ops(enable_method);
  

• if (!cpu_ops[cpu])
  

• 	continue;
  

• cpu_ops[cpu]->cpu_init(NULL, cpu);
  

• }

+}

+void __init smp_init_cpus(void) +{

• if (!of_smp_init_cpus())

• return;
  

• acpi_smp_init_cpus(); }

With DT we initialise the cpu_ops[0] via cpu_read_bootcpu_ops() called from setup_arch(). This is needed because with PSCI we use cpu_ops for power management even if it's a UP system. Do you get a some kernel warning about device node for the boot cpu not found?

Thanks for pointing this out, actually we didn't find a dts file with spin-table method for SMP initialization, and this patch is not fully tested (only spin-table method is supported in ACPI now), we are working on that and get this patch fully tested.

I will review the code carefully in your comments, and update the code accordingly.

...

--- a/drivers/acpi/plat/arm-core.c +++ b/drivers/acpi/plat/arm-core.c @@ -367,6 +367,32 @@ static void __init acpi_process_madt(void) } /*

• • To see PCSI is enabled or not.
• • PSCI is not available for ACPI 5.0, return FALSE for now.
• • FIXME: should we introduce early_param("psci", func) for test purpose?
• */

+static bool acpi_psci_smp_available(int cpu) +{

• return FALSE;

+}

+static const char *enable_method[] = {

• "psci",
• "spin-table",
• NULL

+};

+const char *acpi_get_enable_method(int cpu) +{

• if (acpi_psci_smp_available(cpu))

• return enable_method[0];
  

• else

• return enable_method[1];
  

+}

You could just use literal strings here, actually even ignoring PSCI until available.

Ok.

Thanks Hanjun