[PATCH 00/20] Make ACPI core running on ARM64

[PATCH 00/20] Make ACPI core running on ARM64

[Hanjun Guo]

This patch set trys to make ACPI core running on ARM64.

1. Why ACPI is needed ?
ACPI provides standard interfaces for power managment, themal
control, device enumeration and configuration and etc. So ACPI
can make OS and hardware decoupling and make people upgrade
software or hardware more easily.

ACPI is more than that. It can provide standard error report
interface called ACPI Platform Error Interface (APEI) for
RAS features, and provide standard interface for dynamic
system device reconfigurations such as CPU/memory/computer
node hot-add and hot-remove, this is especially useful
for servers.

2. Does ACPI run propably on ARM64 ?
ACPI spec 5.0 introduces hardware reduced mode, GICC/GICD,
GTDT for arch timer and etc for Socs.

This makes it possible to run ACPI on ARM/ARM64. Some
company already did that (UEFI+ACPI) on ARM platforms.
ACPI spec for ARM/ARM64 is still need to be improved and
people are working on that, but for now, we can run ACPI
propably except for some new features such as PSCI and
GICv3.

3. What we did in this patch set
patch 1-8(Hanjun Guo) will make ACPI core running on ARM64 and
make the compilation is ok on ARM64.

patch 9-12 will enumeration cpus presented by MADT table.
since PSCI is not available for ACPI 5.0, we use spin-table
method for SMP initialization.

patch 13-16 will initialize GIC in the system. ACPI only
support one GIC and only report information about GICC and
GICD, so there will some limitations for GICv3 and GICH
for GICv2 for virtualization.

patch 17-19 and patch from Amit will finish the arch timer
initialization. We get informatiom from GTDT
(Generic Timer Description Table) to initialize arch timer
and introduce a macro CLOCKSOURCE_ACPI_DECLARE to do the
same job as CLOCKSOURCE_OF_DECLARE to make it scalable for
other timers.

All the ACPI tables are available with BSD license at:
git://git.linaro.org/arm/acpi/acpi-asl.git

Changs since last RFC version:
1) Introduce some PCI functions when PCI is enabled
2) Select ACPI_REDUCED_HARDWARE_ONLY if ACPI is enabled
3) Remove ACPI based CPU hotplug code
4) Use spin-table method for SMP initialization
5) Add macro CLOCKSOURCE_ACPI_DECLARE for ACPI timer init
6) Address lots of comments and suggestions in last RFC version
7) Remove RFC tag and make all the patches seperated as two
   parts, one is "ACPI: Some patches to prepare for running ACPI
   on !x86 and !ia64", and another is this one.
8) Rebased on 3.13-rc8.

Amit Daniel Kachhap (1):
  clocksource / acpi: Add macro CLOCKSOURCE_ACPI_DECLARE

Hanjun Guo (19):
  ARM64 / ACPI: Make PCI optional for ACPI on ARM64
  ARM64 : Add dummy asm/cpu.h
  ARM64 / ACPI: Introduce the skeleton of _PDC related for ARM64
  ARM64 / ACPI: Introduce arm_core.c and its related head file
  ARM64 / ACPI: Introduce lowlevel suspend function
  ARM64 / ACPI: Introduce some PCI functions when PCI is enabled
  ARM64 / ACPI: Enable ARM64 in Kconfig
  ARM64 / ACPI: Select ACPI_REDUCED_HARDWARE_ONLY if ACPI is enabled on
    ARM64
  ARM64 / ACPI: Implement core functions for parsing MADT table
  ARM64 / ACPI: Enumerate possible/present CPU set and map logical cpu
    id to APIC id
  ARM64 / ACPI: Get the enable method for SMP initialization
  ARM64 / ACPI: Use Parked Address in GIC structure for spin table SMP
    initialisation
  ARM64 / ACPI: Define ACPI_IRQ_MODEL_GIC needed for arm
  Irqchip / gic: Set as default domain so we can access from ACPI
  ACPI / ARM64: Update acpi_register_gsi to register with the core IRQ
    subsystem
  ACPI / GIC: Initialize GIC using the information in MADT
  clocksource / arch_timer: Use ACPI GTDT table to initialize arch
    timer
  clocksource / ACPI: Introduce clocksource_acpi_init() using
    CLOCKSOURCE_ACPI_DECLARE
  ARM64 / clocksource: Use clocksource_acpi_init()

 arch/arm64/Kconfig                   |    3 +
 arch/arm64/Makefile                  |    1 +
 arch/arm64/include/asm/acpi.h        |  112 +++++++
 arch/arm64/include/asm/cpu.h         |   25 ++
 arch/arm64/include/asm/cpu_ops.h     |    1 +
 arch/arm64/include/asm/pci.h         |   20 ++
 arch/arm64/include/asm/processor.h   |    2 +
 arch/arm64/include/asm/smp.h         |    2 +-
 arch/arm64/kernel/cpu_ops.c          |    2 +-
 arch/arm64/kernel/irq.c              |    6 +
 arch/arm64/kernel/process.c          |    3 +
 arch/arm64/kernel/setup.c            |    6 +
 arch/arm64/kernel/smp.c              |   42 ++-
 arch/arm64/kernel/smp_spin_table.c   |   12 +-
 arch/arm64/kernel/time.c             |    6 +
 arch/arm64/pci/Makefile              |    1 +
 arch/arm64/pci/pci.c                 |   33 ++
 drivers/acpi/Kconfig                 |   11 +-
 drivers/acpi/Makefile                |    4 +-
 drivers/acpi/bus.c                   |    3 +
 drivers/acpi/internal.h              |    7 +
 drivers/acpi/osl.c                   |    3 +-
 drivers/acpi/plat/Makefile           |    1 +
 drivers/acpi/plat/arm-core.c         |  599 ++++++++++++++++++++++++++++++++++
 drivers/acpi/tables.c                |   21 ++
 drivers/clocksource/Makefile         |    1 +
 drivers/clocksource/arm_arch_timer.c |  101 +++++-
 drivers/clocksource/clksrc-acpi.c    |   36 ++
 drivers/irqchip/irq-gic.c            |    7 +
 include/asm-generic/vmlinux.lds.h    |   10 +
 include/linux/acpi.h                 |    7 +
 include/linux/clocksource.h          |   15 +
 include/linux/pci.h                  |   33 +-
 33 files changed, 1099 insertions(+), 37 deletions(-)
 create mode 100644 arch/arm64/include/asm/acpi.h
 create mode 100644 arch/arm64/include/asm/cpu.h
 create mode 100644 arch/arm64/include/asm/pci.h
 create mode 100644 arch/arm64/pci/Makefile
 create mode 100644 arch/arm64/pci/pci.c
 create mode 100644 drivers/acpi/plat/Makefile
 create mode 100644 drivers/acpi/plat/arm-core.c
 create mode 100644 drivers/clocksource/clksrc-acpi.c

-- 
1.7.9.5

[PATCH 01/20] ARM64 / ACPI: Make PCI optional for ACPI on ARM64

[Hanjun Guo]

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)
+
+#endif /* __KERNEL__ */
+
+#endif /* __ASMARM64_PCI_H */
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 0331f91..d8cebe3 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -38,7 +38,7 @@ acpi-y				+= acpi_processor.o
 acpi-y				+= processor_core.o
 acpi-y				+= ec.o
 acpi-$(CONFIG_ACPI_DOCK)	+= dock.o
-acpi-y				+= pci_root.o pci_link.o pci_irq.o
+acpi-$(CONFIG_PCI)		+= pci_root.o pci_link.o pci_irq.o
 acpi-$(CONFIG_X86_INTEL_LPSS)	+= acpi_lpss.o
 acpi-y				+= acpi_platform.o
 acpi-y				+= power.o
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index a29739c..52dff47 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -26,9 +26,16 @@
 acpi_status acpi_os_initialize1(void);
 int init_acpi_device_notify(void);
 int acpi_scan_init(void);
+#ifdef CONFIG_PCI
 void acpi_pci_root_init(void);
 void acpi_pci_link_init(void);
 void acpi_pci_root_hp_init(void);
+#else
+static inline void acpi_pci_root_init(void) {}
+static inline void acpi_pci_link_init(void) {}
+static inline void acpi_pci_root_hp_init(void) {}
+#endif /* CONFIG_PCI */
+
 void acpi_processor_init(void);
 void acpi_platform_init(void);
 int acpi_sysfs_init(void);
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 54a20ff..14ee6fc 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -1050,7 +1050,8 @@ acpi_os_read_pci_configuration(struct acpi_pci_id * pci_id, u32 reg,
 	result = raw_pci_read(pci_id->segment, pci_id->bus,
 				PCI_DEVFN(pci_id->device, pci_id->function),
 				reg, size, &value32);
-	*value = value32;
+	if (!result)
+		*value = value32;
 
 	return (result ? AE_ERROR : AE_OK);
 }
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
@@ -541,15 +541,6 @@ struct pci_ops {
 	int (*write)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 val);
 };
 
-/*
- * ACPI needs to be able to access PCI config space before we've done a
- * PCI bus scan and created pci_bus structures.
- */
-int raw_pci_read(unsigned int domain, unsigned int bus, unsigned int devfn,
-		 int reg, int len, u32 *val);
-int raw_pci_write(unsigned int domain, unsigned int bus, unsigned int devfn,
-		  int reg, int len, u32 val);
-
 struct pci_bus_region {
 	resource_size_t start;
 	resource_size_t end;
@@ -1281,6 +1272,15 @@ typedef int (*arch_set_vga_state_t)(struct pci_dev *pdev, bool decode,
 		      unsigned int command_bits, u32 flags);
 void pci_register_set_vga_state(arch_set_vga_state_t func);
 
+/*
+ * ACPI needs to be able to access PCI config space before we've done a
+ * PCI bus scan and created pci_bus structures.
+ */
+int raw_pci_read(unsigned int domain, unsigned int bus, unsigned int devfn,
+		 int reg, int len, u32 *val);
+int raw_pci_write(unsigned int domain, unsigned int bus, unsigned int devfn,
+		  int reg, int len, u32 val);
+
 #else /* CONFIG_PCI is not enabled */
 
 /*
@@ -1477,6 +1477,21 @@ static inline int pci_domain_nr(struct pci_bus *bus)
 static inline struct pci_dev *pci_dev_get(struct pci_dev *dev)
 { return NULL; }
 
+static inline struct pci_bus *pci_find_bus(int domain, int busnr)
+{ return NULL; }
+
+static inline int pci_bus_write_config_byte(struct pci_bus *bus,
+			unsigned int devfn, int where, u8 val)
+{ return -ENODEV; }
+
+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; }
+
 #define dev_is_pci(d) (false)
 #define dev_is_pf(d) (false)
 #define dev_num_vf(d) (0)
-- 
1.7.9.5

[PATCH 02/20] ARM64 : Add dummy asm/cpu.h

[Hanjun Guo]

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

[PATCH 03/20] ARM64 / ACPI: Introduce the skeleton of _PDC related for ARM64

[Hanjun Guo]

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 our default idle handler.
  */
-- 
1.7.9.5

[PATCH 04/20] ARM64 / ACPI: Introduce arm_core.c and its related head file

[Hanjun Guo]

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
+
+#else	/* !CONFIG_ACPI */
+#define acpi_disabled 1		/* ACPI sometimes enabled on ARM */
+#define acpi_noirq 1		/* ACPI sometimes enabled on ARM */
+#define acpi_pci_disabled 1	/* ACPI PCI sometimes enabled on ARM */
+#define acpi_strict 1		/* no ACPI spec workarounds on ARM */
+#endif
+
 #endif /*_ASM_ARM64_ACPI_H*/
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();
 	request_standard_resources();
 
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index d8cebe3..9fbba50 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..162a1ab
--- /dev/null
+++ b/drivers/acpi/plat/arm-core.c
@@ -0,0 +1,209 @@
+/*
+ *  ARM/ARM64 Specific Low-Level ACPI Boot Support
+ *
+ *  Copyright (C) 2013, Al Stone <al.stone@linaro.org>
+ *  Copyright (C) 2013, Graeme Gregory <graeme.gregory@linaro.org>
+ *  Copyright (C) 2013, 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);
+
+#define PREFIX			"ACPI: "
+
+/* FIXME: this function should be moved to topology.c when it is ready */
+void arch_fix_phys_package_id(int num, u32 slot)
+{
+	return;
+}
+EXPORT_SYMBOL_GPL(arch_fix_phys_package_id);
+
+enum acpi_irq_model_id acpi_irq_model = ACPI_IRQ_MODEL_PLATFORM;
+
+static unsigned int gsi_to_irq(unsigned int gsi)
+{
+	int irq = irq_create_mapping(NULL, gsi);
+
+	return irq;
+}
+
+/*
+ * __acpi_map_table() will be called before page_init(), so early_ioremap()
+ * or early_memremap() should be called here.
+ *
+ * FIXME: early_io/memremap()/early_iounmap() are not upstream yet on ARM64,
+ * just wait for Mark Salter's patchset accepted by mainline
+ */
+char *__init __acpi_map_table(unsigned long phys, unsigned long size)
+{
+	if (!phys || !size)
+		return NULL;
+
+	/*
+	 * temporarily use phys_to_virt(),
+	 * should be early_memremap(phys, size) here
+	 */
+	return phys_to_virt(phys);
+}
+
+void __init __acpi_unmap_table(char *map, unsigned long size)
+{
+	if (!map || !size)
+		return;
+
+	/* should be early_iounmap(map, size); */
+	return;
+}
+
+int acpi_gsi_to_irq(u32 gsi, unsigned int *irq)
+{
+	*irq = gsi_to_irq(gsi);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(acpi_gsi_to_irq);
+
+/*
+ * success: return IRQ number (>0)
+ * failure: return =< 0
+ */
+int acpi_register_gsi(struct device *dev, u32 gsi, int trigger, int polarity)
+{
+	return -1;
+}
+EXPORT_SYMBOL_GPL(acpi_register_gsi);
+
+void acpi_unregister_gsi(u32 gsi)
+{
+}
+EXPORT_SYMBOL_GPL(acpi_unregister_gsi);
+
+static int __init acpi_parse_fadt(struct acpi_table_header *table)
+{
+	return 0;
+}
+
+static void __init early_acpi_process_madt(void)
+{
+	return;
+}
+
+static void __init acpi_process_madt(void)
+{
+	return;
+}
+
+/*
+ * 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;
+}
+
+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);
-- 
1.7.9.5

[PATCH 05/20] ARM64 / ACPI: Introduce lowlevel suspend function

[Hanjun Guo]

Lowlevel suspend function is needed for ACPI based suspend/resume,
introduce ARM related lowlevel function in this patch.

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 908d71b..e108d9c 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -79,6 +79,10 @@ 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)
+
 #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 162a1ab..3c8521d 100644
--- a/drivers/acpi/plat/arm-core.c
+++ b/drivers/acpi/plat/arm-core.c
@@ -207,3 +207,10 @@ static int __init parse_acpi(char *arg)
 	return 0;
 }
 early_param("acpi", parse_acpi);
+
+/*
+ * acpi_suspend_lowlevel() - save kernel state and suspend.
+ *
+ * TBD when ARM/ARM64 starts to support suspend...
+ */
+int (*acpi_suspend_lowlevel)(void);
-- 
1.7.9.5

[PATCH 06/20] ARM64 / ACPI: Introduce some PCI functions when PCI is enabled

[Hanjun Guo]

Introduce some PCI functions to make ACPI can be compiled when
CONFIG_PCI is enabled, these functions should be revisited when
implemented on ARM64.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 arch/arm64/Makefile          |    1 +
 arch/arm64/pci/Makefile      |    1 +
 arch/arm64/pci/pci.c         |   33 +++++++++++++++++++++++++++++++++
 drivers/acpi/plat/arm-core.c |   19 +++++++++++++++++++
 4 files changed, 54 insertions(+)
 create mode 100644 arch/arm64/pci/Makefile
 create mode 100644 arch/arm64/pci/pci.c

diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
index 2fceb71..fcaa58e 100644
--- a/arch/arm64/Makefile
+++ b/arch/arm64/Makefile
@@ -45,6 +45,7 @@ export	TEXT_OFFSET GZFLAGS
 core-y		+= arch/arm64/kernel/ arch/arm64/mm/
 core-$(CONFIG_KVM) += arch/arm64/kvm/
 core-$(CONFIG_XEN) += arch/arm64/xen/
+drivers-$(CONFIG_PCI)	+= arch/arm64/pci/
 libs-y		:= arch/arm64/lib/ $(libs-y)
 libs-y		+= $(LIBGCC)
 
diff --git a/arch/arm64/pci/Makefile b/arch/arm64/pci/Makefile
new file mode 100644
index 0000000..b8d5dbd
--- /dev/null
+++ 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;
+}
+
+/* Root bridge scanning */
+struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
+{
+	return NULL;
+}
+
+void __init pci_acpi_crs_quirks(void)
+{
+	return;
+}
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);
+
 /*
  * success: return IRQ number (>0)
  * failure: return =< 0
-- 
1.7.9.5

[PATCH 07/20] ARM64 / ACPI: Enable ARM64 in Kconfig

[Hanjun Guo]

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
 	default y
 	help
 	  This driver installs ACPI as the idle handler for Linux and uses
@@ -250,6 +254,7 @@ config ACPI_DEBUG
 config ACPI_PCI_SLOT
 	bool "PCI slot detection driver"
 	depends on SYSFS
+	depends on PCI
 	default n
 	help
 	  This driver creates entries in /sys/bus/pci/slots/ for all PCI
diff --git a/drivers/acpi/plat/Makefile b/drivers/acpi/plat/Makefile
index 46bc65e..3a61176 100644
--- a/drivers/acpi/plat/Makefile
+++ b/drivers/acpi/plat/Makefile
@@ -1 +1 @@
-obj-$(CONFIG_ARM64)	+= arm-core.o
+obj-$(CONFIG_ACPI_ARM)	+= arm-core.o
-- 
1.7.9.5

[PATCH 08/20] ARM64 / ACPI: Select ACPI_REDUCED_HARDWARE_ONLY if ACPI is enabled on ARM64

[Hanjun Guo]

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

Signed-off-by: Al Stone <al.stone@linaro.org>
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 arch/arm64/Kconfig |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 2b1fb1d..741692c 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1,5 +1,6 @@
 config ARM64
 	def_bool y
+	select ACPI_REDUCED_HARDWARE_ONLY if ACPI
 	select ARCH_HAS_ATOMIC64_DEC_IF_POSITIVE
 	select ARCH_USE_CMPXCHG_LOCKREF
 	select ARCH_WANT_OPTIONAL_GPIOLIB
-- 
1.7.9.5

[PATCH 09/20] ARM64 / ACPI: Implement core functions for parsing MADT table

[Hanjun Guo]

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
+#define MAX_GIC_DISTRIBUTOR   1		/* should be the same as MAX_GIC_NR */
+
 #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;
+
+#define BAD_MADT_ENTRY(entry, end) (					\
+	(!entry) || (unsigned long)entry + sizeof(*entry) > end ||	\
+	((struct acpi_subtable_header *)entry)->length < sizeof(*entry))
+
 #define PREFIX			"ACPI: "
 
 /* 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;
+
+	madt = (struct acpi_table_madt *)table;
+	if (!madt) {
+		pr_warn(PREFIX "Unable to map MADT\n");
+		return -ENODEV;
+	}
+
+	if (madt->address) {
+		acpi_lapic_addr = (u64) madt->address;
+
+		pr_info(PREFIX "Local APIC address 0x%08x\n", madt->address);
+	}
+
+	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.
+ *
+ * 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
+ */
+
+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;
+	}
+
+	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)) {
+
+		/*
+		 * 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
+			       "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",
-- 
1.7.9.5

[PATCH 10/20] ARM64 / ACPI: Enumerate possible/present CPU set and map logical cpu id to APIC id

[Hanjun Guo]

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]
+
 #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;
 
+/* available_cpus here means enabled cpu in MADT */
+static int available_cpus;
+
+/* 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;
+
 #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;
+	}
+
+	available_cpus++;
+
+	/* 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);
+
+	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 */
+	}
+#endif
+
+	/* Make boot-up look pretty */
+	pr_info("%d CPUs available, %d CPUs total\n", available_cpus,
+		total_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;
+
 	/*
 	 * Process the Multiple APIC Description Table (MADT), if present
 	 */
-- 
1.7.9.5

[PATCH 11/20] ARM64 / ACPI: Get the enable method for SMP initialization

[Hanjun Guo]

ACPI has no flag to indicate different enable methods for SMP
initialization, but it indicates that spin-table is supported
at now because of the Parked Protocol shows that there is
parked address in GIC structure.

In order to boot the system with ACPI if DT is not available,
we set the default enable method as spin-table since PSCI is
not available in ACPI spec at now.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 arch/arm64/include/asm/acpi.h    |    8 ++++++++
 arch/arm64/include/asm/cpu_ops.h |    1 +
 arch/arm64/include/asm/smp.h     |    2 +-
 arch/arm64/kernel/cpu_ops.c      |    2 +-
 arch/arm64/kernel/smp.c          |   42 ++++++++++++++++++++++++++++++++++++--
 drivers/acpi/plat/arm-core.c     |   26 +++++++++++++++++++++++
 6 files changed, 77 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index 7edd39e..097394c 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -87,11 +87,19 @@ extern int (*acpi_suspend_lowlevel)(void);
 extern int arm_cpu_to_apicid[NR_CPUS];
 #define cpu_physical_id(cpu) arm_cpu_to_apicid[cpu]
 
+extern const char *acpi_get_enable_method(int cpu);
+
 #else	/* !CONFIG_ACPI */
 #define acpi_disabled 1		/* ACPI sometimes enabled on ARM */
 #define acpi_noirq 1		/* ACPI sometimes enabled on ARM */
 #define acpi_pci_disabled 1	/* ACPI PCI sometimes enabled on ARM */
 #define acpi_strict 1		/* no ACPI spec workarounds on ARM */
+
+static inline const char *acpi_get_enable_method(int cpu)
+{
+	return NULL;
+}
+
 #endif
 
 #endif /*_ASM_ARM64_ACPI_H*/
diff --git a/arch/arm64/include/asm/cpu_ops.h b/arch/arm64/include/asm/cpu_ops.h
index c4cdb5e..08c8f07 100644
--- a/arch/arm64/include/asm/cpu_ops.h
+++ b/arch/arm64/include/asm/cpu_ops.h
@@ -53,6 +53,7 @@ struct cpu_operations {
 };
 
 extern const struct cpu_operations *cpu_ops[NR_CPUS];
+extern const struct cpu_operations * __init cpu_get_ops(const char *name);
 extern int __init cpu_read_ops(struct device_node *dn, int cpu);
 extern void __init cpu_read_bootcpu_ops(void);
 
diff --git a/arch/arm64/include/asm/smp.h b/arch/arm64/include/asm/smp.h
index a498f2c..a5cea56 100644
--- a/arch/arm64/include/asm/smp.h
+++ b/arch/arm64/include/asm/smp.h
@@ -39,7 +39,7 @@ extern void show_ipi_list(struct seq_file *p, int prec);
 extern void handle_IPI(int ipinr, struct pt_regs *regs);
 
 /*
- * Setup the set of possible CPUs (via set_cpu_possible)
+ * Platform specific SMP operations
  */
 extern void smp_init_cpus(void);
 
diff --git a/arch/arm64/kernel/cpu_ops.c b/arch/arm64/kernel/cpu_ops.c
index d62d12f..531f2cc 100644
--- a/arch/arm64/kernel/cpu_ops.c
+++ b/arch/arm64/kernel/cpu_ops.c
@@ -35,7 +35,7 @@ static const struct cpu_operations *supported_cpu_ops[] __initconst = {
 	NULL,
 };
 
-static const struct cpu_operations * __init cpu_get_ops(const char *name)
+const struct cpu_operations * __init cpu_get_ops(const char *name)
 {
 	const struct cpu_operations **ops = supported_cpu_ops;
 
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index a0c2ca6..7719d46 100644
--- 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();
 }
 
 void __init smp_prepare_cpus(unsigned int max_cpus)
diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c
index 1d9b789..2704633 100644
--- 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];
+}
+
+/*
  * acpi_boot_table_init() and acpi_boot_init()
  *  called from setup_arch(), always.
  *	1. checksums all tables
-- 
1.7.9.5

[PATCH 12/20] ARM64 / ACPI: Use Parked Address in GIC structure for spin table SMP initialisation

[Hanjun Guo]

Parked Address in GIC structure can be used as cpu release address
for spin table SMP initialisation.

This patch gets parked address from MADT and use it for SMP
initialisation when DT is not available.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 arch/arm64/include/asm/acpi.h      |    7 +++++++
 arch/arm64/kernel/smp_spin_table.c |   12 ++++++++----
 drivers/acpi/plat/arm-core.c       |   24 +++++++++++++++++++++++-
 3 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index 097394c..856a2e7 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -89,6 +89,8 @@ extern int arm_cpu_to_apicid[NR_CPUS];
 
 extern const char *acpi_get_enable_method(int cpu);
 
+extern int acpi_get_cpu_release_address(int cpu, u64 *release_address);
+
 #else	/* !CONFIG_ACPI */
 #define acpi_disabled 1		/* ACPI sometimes enabled on ARM */
 #define acpi_noirq 1		/* ACPI sometimes enabled on ARM */
@@ -100,6 +102,11 @@ static inline const char *acpi_get_enable_method(int cpu)
 	return NULL;
 }
 
+static inline int acpi_get_cpu_release_address(int cpu, u64 *release_address)
+{
+	return -ENODEV;
+}
+
 #endif
 
 #endif /*_ASM_ARM64_ACPI_H*/
diff --git a/arch/arm64/kernel/smp_spin_table.c b/arch/arm64/kernel/smp_spin_table.c
index 44c2280..ce86189 100644
--- a/arch/arm64/kernel/smp_spin_table.c
+++ b/arch/arm64/kernel/smp_spin_table.c
@@ -25,6 +25,7 @@
 #include <asm/cpu_ops.h>
 #include <asm/cputype.h>
 #include <asm/smp_plat.h>
+#include <asm/acpi.h>
 
 extern void secondary_holding_pen(void);
 volatile unsigned long secondary_holding_pen_release = INVALID_HWID;
@@ -47,7 +48,6 @@ static void write_pen_release(u64 val)
 	__flush_dcache_area(start, size);
 }
 
-
 static int smp_spin_table_cpu_init(struct device_node *dn, unsigned int cpu)
 {
 	/*
@@ -55,10 +55,14 @@ static int smp_spin_table_cpu_init(struct device_node *dn, unsigned int cpu)
 	 */
 	if (of_property_read_u64(dn, "cpu-release-addr",
 				 &cpu_release_addr[cpu])) {
-		pr_err("CPU %d: missing or invalid cpu-release-addr property\n",
-		       cpu);
 
-		return -1;
+		/* try ACPI way */
+		if (acpi_get_cpu_release_address(cpu, &cpu_release_addr[cpu])) {
+			pr_err("CPU %d: missing or invalid cpu-release-addr property\n",
+				cpu);
+
+			return -1;
+		}
 	}
 
 	return 0;
diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c
index 2704633..7f1d709 100644
--- a/drivers/acpi/plat/arm-core.c
+++ b/drivers/acpi/plat/arm-core.c
@@ -60,6 +60,9 @@ static int available_cpus;
 int arm_cpu_to_apicid[NR_CPUS] = { [0 ... NR_CPUS-1] = -1 };
 static int boot_cpu_apic_id = -1;
 
+/* Parked Address in ACPI GIC structure */
+static u64 parked_address[NR_CPUS];
+
 #define BAD_MADT_ENTRY(entry, end) (					\
 	(!entry) || (unsigned long)entry + sizeof(*entry) > end ||	\
 	((struct acpi_subtable_header *)entry)->length < sizeof(*entry))
@@ -193,6 +196,7 @@ static int __init
 acpi_parse_gic(struct acpi_subtable_header *header, const unsigned long end)
 {
 	struct acpi_madt_generic_interrupt *processor = NULL;
+	int cpu;
 
 	processor = (struct acpi_madt_generic_interrupt *)header;
 
@@ -208,9 +212,16 @@ acpi_parse_gic(struct acpi_subtable_header *header, const unsigned long end)
 	 * to not preallocating memory for all NR_CPUS
 	 * when we use CPU hotplug.
 	 */
-	acpi_register_gic_cpu_interface(processor->gic_id,
+	cpu = acpi_register_gic_cpu_interface(processor->gic_id,
 			processor->flags & ACPI_MADT_ENABLED);
 
+	/*
+	 * We need the parked address for SMP initialization with
+	 * spin-table enable method
+	 */
+	if (cpu >= 0 && processor->parked_address)
+		parked_address[cpu] = processor->parked_address;
+
 	return 0;
 }
 
@@ -269,6 +280,17 @@ static int __init acpi_parse_madt_gic_entries(void)
 	return 0;
 }
 
+/* Parked Address in ACPI GIC structure can be used as cpu release addr */
+int acpi_get_cpu_release_address(int cpu, u64 *release_address)
+{
+	if (!release_address || !parked_address[cpu])
+		return -EINVAL;
+
+	*release_address = parked_address[cpu];
+
+	return 0;
+}
+
 /*
  * Parse GIC distributor related entries in MADT
  * returns 0 on success, < 0 on error
-- 
1.7.9.5

[PATCH 13/20] ARM64 / ACPI: Define ACPI_IRQ_MODEL_GIC needed for arm

[Hanjun Guo]

Needed because arm uses GIC which is defined in ACPI 5.0 spec.

Signed-off-by: Al Stone <al.stone@linaro.org>
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 drivers/acpi/bus.c           |    3 +++
 drivers/acpi/plat/arm-core.c |    6 +++++-
 include/linux/acpi.h         |    1 +
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 0710004..246b40c 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -487,6 +487,9 @@ static int __init acpi_bus_init_irq(void)
 	case ACPI_IRQ_MODEL_IOSAPIC:
 		message = "IOSAPIC";
 		break;
+	case ACPI_IRQ_MODEL_GIC:
+		message = "GIC";
+		break;
 	case ACPI_IRQ_MODEL_PLATFORM:
 		message = "platform specific model";
 		break;
diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c
index 7f1d709..2ff00b0 100644
--- a/drivers/acpi/plat/arm-core.c
+++ b/drivers/acpi/plat/arm-core.c
@@ -76,7 +76,11 @@ void arch_fix_phys_package_id(int num, u32 slot)
 }
 EXPORT_SYMBOL_GPL(arch_fix_phys_package_id);
 
-enum acpi_irq_model_id acpi_irq_model = ACPI_IRQ_MODEL_PLATFORM;
+/*
+ * Since we're on ARM, the default interrupt routing model
+ * clearly has to be GIC.
+ */
+enum acpi_irq_model_id acpi_irq_model = ACPI_IRQ_MODEL_GIC;
 
 static unsigned int gsi_to_irq(unsigned int gsi)
 {
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index d9099b1..da55da2 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -63,6 +63,7 @@ enum acpi_irq_model_id {
 	ACPI_IRQ_MODEL_IOAPIC,
 	ACPI_IRQ_MODEL_IOSAPIC,
 	ACPI_IRQ_MODEL_PLATFORM,
+	ACPI_IRQ_MODEL_GIC,
 	ACPI_IRQ_MODEL_COUNT
 };
 
-- 
1.7.9.5

[PATCH 14/20] Irqchip / gic: Set as default domain so we can access from ACPI

[Hanjun Guo]

Only one GIC is supported in ACPI 5.0, even cascade GIC is not supported.

So if we set the GIC as the default domain then we can access it for IRQ
mapping within the ACPI code.

Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 drivers/irqchip/irq-gic.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 341c601..40fde7b 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -963,6 +963,13 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
 		register_cpu_notifier(&gic_cpu_notifier);
 #endif
 		set_handle_irq(gic_handle_irq);
+
+		/*
+		 * do not set default host for GIC domain multi-times.
+		 * FIXME: This probably needs revisited when multi GICs
+		 * supported
+		 */
+		irq_set_default_host(gic->domain);
 	}
 
 	gic_chip.flags |= gic_arch_extn.flags;
-- 
1.7.9.5

[PATCH 15/20] ACPI / ARM64: Update acpi_register_gsi to register with the core IRQ subsystem

[Hanjun Guo]

This API is similar to DT based irq_of_parse_and_map but does link
parent/child IRQ controllers. This is tested for primary GIC PPI and GIC SPI
interrupts and not for secondary child irq controllers.

Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
Signed-off-by: Tomasz Nowicki <tomasz.nowicki@linaro.org>
---
 drivers/acpi/plat/arm-core.c |   48 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 46 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c
index 2ff00b0..22c8235 100644
--- a/drivers/acpi/plat/arm-core.c
+++ b/drivers/acpi/plat/arm-core.c
@@ -84,7 +84,7 @@ enum acpi_irq_model_id acpi_irq_model = ACPI_IRQ_MODEL_GIC;
 
 static unsigned int gsi_to_irq(unsigned int gsi)
 {
-	int irq = irq_create_mapping(NULL, gsi);
+	int irq = irq_find_mapping(NULL, gsi);
 
 	return irq;
 }
@@ -350,7 +350,51 @@ EXPORT_SYMBOL(acpi_unregister_ioapic);
  */
 int acpi_register_gsi(struct device *dev, u32 gsi, int trigger, int polarity)
 {
-	return -1;
+	unsigned int irq;
+	unsigned int irq_type;
+
+	/*
+	 * ACPI have no bindings to indicate SPI or PPI, so we
+	 * use different mappings from DT in ACPI.
+	 *
+	 * For FDT
+	 * PPI interrupt: in the range [0, 15];
+	 * SPI interrupt: in the range [0, 987];
+	 *
+	 * For ACPI, GSI should be unique so using
+	 * identity mapping for hwirq:
+	 * PPI interrupt: in the range [16, 31];
+	 * SPI interrupt: in the range [32, 1019];
+	 */
+
+	if (trigger == ACPI_EDGE_SENSITIVE &&
+				polarity == ACPI_ACTIVE_LOW)
+		irq_type = IRQ_TYPE_EDGE_FALLING;
+	else if (trigger == ACPI_EDGE_SENSITIVE &&
+				polarity == ACPI_ACTIVE_HIGH)
+		irq_type = IRQ_TYPE_EDGE_RISING;
+	else if (trigger == ACPI_LEVEL_SENSITIVE &&
+				polarity == ACPI_ACTIVE_LOW)
+		irq_type = IRQ_TYPE_LEVEL_LOW;
+	else if (trigger == ACPI_LEVEL_SENSITIVE &&
+				polarity == ACPI_ACTIVE_HIGH)
+		irq_type = IRQ_TYPE_LEVEL_HIGH;
+	else
+		irq_type = IRQ_TYPE_NONE;
+
+	/*
+	 * Since only one GIC is supported in ACPI 5.0, we can
+	 * create mapping refer to the default domain
+	 */
+	irq = irq_create_mapping(NULL, gsi);
+	if (!irq)
+		return irq;
+
+	/* Set irq type if specified and different than the current one */
+	if (irq_type != IRQ_TYPE_NONE &&
+		irq_type != irq_get_trigger_type(irq))
+		irq_set_irq_type(irq, irq_type);
+	return irq;
 }
 EXPORT_SYMBOL_GPL(acpi_register_gsi);
 
-- 
1.7.9.5

=?yes?q?=5BPATCH=2016/20=5D=20ACPI=20/=20GIC=3A=20Initialize=20GIC=20using=20the=20information=20in=20MADT?=

[Hanjun Guo]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=yes, Size: 5023 bytes --]

In MADT table, there are GIC cpu interface base address and
GIC distributor base address, use them to convert GIC to ACPI.

Only GICC and GICD are described in ACPI 5.0, and only one GIC
is supported at now.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 arch/arm64/kernel/irq.c      |    6 ++++
 drivers/acpi/plat/arm-core.c |   64 ++++++++++++++++++++++++++++++++++++------
 include/linux/acpi.h         |    6 ++++
 3 files changed, 68 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index 473e5db..490d971 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -25,6 +25,7 @@
 #include <linux/irq.h>
 #include <linux/smp.h>
 #include <linux/init.h>
+#include <linux/acpi.h>
 #include <linux/irqchip.h>
 #include <linux/seq_file.h>
 #include <linux/ratelimit.h>
@@ -78,6 +79,11 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs *))
 void __init init_IRQ(void)
 {
 	irqchip_init();
+
+	/* FIXME: need to fix GIC hardcoded here and find a scalable way */
+	if (!handle_arch_irq)
+		acpi_gic_init();
+
 	if (!handle_arch_irq)
 		panic("No interrupt controller found.");
 }
diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c
index 22c8235..c4a7327 100644
--- a/drivers/acpi/plat/arm-core.c
+++ b/drivers/acpi/plat/arm-core.c
@@ -27,6 +27,7 @@
 #include <linux/module.h>
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
+#include <linux/irqchip/arm-gic.h>
 #include <linux/bootmem.h>
 #include <linux/smp.h>
 
@@ -210,6 +211,15 @@ acpi_parse_gic(struct acpi_subtable_header *header, const unsigned long end)
 	acpi_table_print_madt_entry(header);
 
 	/*
+	 * As ACPI 5.0 said, the 64-bit physical address in GIC struct at
+	 * which the processor can access this GIC. If provided, the
+	 * “Local Interruptp controller Address” field in  the MADT
+	 * is ignored by OSPM.
+	 */
+	if (processor->base_address)
+		acpi_lapic_addr = processor->base_address;
+
+	/*
 	 * 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
@@ -229,11 +239,26 @@ acpi_parse_gic(struct acpi_subtable_header *header, const unsigned long end)
 	return 0;
 }
 
+/*
+ * Hard code here, we can not get memory size from MADT (but FDT does),
+ * this size can be refered from GICv2 spec
+ */
+#define GIC_DISTRIBUTOR_MEMORY_SIZE (SZ_4K)
+#define GIC_CPU_INTERFACE_MEMORY_SIZE (SZ_8K)
+
+/*
+ * ACPI 5.0 only provides information of GICC and GICD, use them
+ * to initialize GIC
+ */
 static int __init
 acpi_parse_gic_distributor(struct acpi_subtable_header *header,
 				const unsigned long end)
 {
 	struct acpi_madt_generic_distributor *distributor = NULL;
+	void __iomem *dist_base, *cpu_base;
+
+	if (!IS_ENABLED(CONFIG_ARM_GIC))
+		return -ENODEV;
 
 	distributor = (struct acpi_madt_generic_distributor *)header;
 
@@ -242,6 +267,27 @@ acpi_parse_gic_distributor(struct acpi_subtable_header *header,
 
 	acpi_table_print_madt_entry(header);
 
+	/* GIC is initialised after page_init(), no need for early_ioremap */
+	dist_base = ioremap(distributor->base_address,
+				GIC_DISTRIBUTOR_MEMORY_SIZE);
+	if (!dist_base) {
+		pr_warn(PREFIX "unable to map gic dist registers\n");
+		return -ENOMEM;
+	}
+
+	/*
+	 * acpi_lapic_addr is stored in acpi_parse_madt() or acpi_parse_gic(),
+	 * so we can use it here for GIC init
+	 */
+	cpu_base = ioremap(acpi_lapic_addr, GIC_CPU_INTERFACE_MEMORY_SIZE);
+	if (!cpu_base) {
+		iounmap(dist_base);
+		pr_warn(PREFIX "unable to map gic cpu registers\n");
+		return -ENOMEM;
+	}
+
+	gic_init(distributor->gic_id, -1, dist_base, cpu_base);
+
 	return 0;
 }
 
@@ -423,19 +469,21 @@ static void __init acpi_process_madt(void)
 		 * 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();
-		}
+		if (!error)
+			pr_info("Using ACPI for processor (GIC) configuration information\n");
 	}
 
-	pr_info("Using ACPI for processor (GIC) configuration information\n");
-
 	return;
 }
 
+int __init acpi_gic_init(void)
+{
+	/*
+	 * Parse MADT GIC distributor entries
+	 */
+	return acpi_parse_madt_gic_distributor_entries();
+}
+
 /*
  * To see PCSI is enabled or not.
  *
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index da55da2..a2dbe67 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -106,6 +106,7 @@ char * __acpi_map_table (unsigned long phys_addr, unsigned long size);
 void __acpi_unmap_table(char *map, unsigned long size);
 int early_acpi_boot_init(void);
 int acpi_boot_init (void);
+int acpi_gic_init(void);
 void acpi_boot_table_init (void);
 int acpi_mps_check (void);
 int acpi_numa_init (void);
@@ -436,6 +437,11 @@ static inline int acpi_boot_init(void)
 	return 0;
 }
 
+static inline int acpi_gic_init(void)
+{
+	return -ENODEV;
+}
+
 static inline void acpi_boot_table_init(void)
 {
 	return;
-- 
1.7.9.5

[PATCH 17/20] clocksource / arch_timer: Use ACPI GTDT table to initialize arch timer

[Hanjun Guo]

ACPI GTDT (Generic Timer Description Table) contains information for
arch timer initialization, this patch use this table to probe arm timer.

GTDT table is used for ARM/ARM64 only, please refer to chapter 5.2.24
of ACPI 5.0 spec for detailed inforamtion

Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 drivers/clocksource/arm_arch_timer.c |  100 +++++++++++++++++++++++++++++-----
 1 file changed, 85 insertions(+), 15 deletions(-)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 95fb944..1fa5f67 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -21,6 +21,7 @@
 #include <linux/io.h>
 #include <linux/slab.h>
 #include <linux/sched_clock.h>
+#include <linux/acpi.h>
 
 #include <asm/arch_timer.h>
 #include <asm/virt.h>
@@ -632,20 +633,8 @@ static void __init arch_timer_common_init(void)
 	arch_timer_arch_init();
 }
 
-static void __init arch_timer_init(struct device_node *np)
+static void __init arch_timer_init(void)
 {
-	int i;
-
-	if (arch_timers_present & ARCH_CP15_TIMER) {
-		pr_warn("arch_timer: multiple nodes in dt, skipping\n");
-		return;
-	}
-
-	arch_timers_present |= ARCH_CP15_TIMER;
-	for (i = PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++)
-		arch_timer_ppi[i] = irq_of_parse_and_map(np, i);
-	arch_timer_detect_rate(NULL, np);
-
 	/*
 	 * If HYP mode is available, we know that the physical timer
 	 * has been configured to be accessible from PL1. Use it, so
@@ -667,8 +656,89 @@ static void __init arch_timer_init(struct device_node *np)
 	arch_timer_register();
 	arch_timer_common_init();
 }
-CLOCKSOURCE_OF_DECLARE(armv7_arch_timer, "arm,armv7-timer", arch_timer_init);
-CLOCKSOURCE_OF_DECLARE(armv8_arch_timer, "arm,armv8-timer", arch_timer_init);
+
+static void __init arch_timer_of_init(struct device_node *np)
+{
+	int i;
+
+	if (arch_timers_present & ARCH_CP15_TIMER) {
+		pr_warn("arch_timer: multiple nodes in dt, skipping\n");
+		return;
+	}
+
+	arch_timers_present |= ARCH_CP15_TIMER;
+	for (i = PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++)
+		arch_timer_ppi[i] = irq_of_parse_and_map(np, i);
+	arch_timer_detect_rate(NULL, np);
+
+	arch_timer_init();
+}
+CLOCKSOURCE_OF_DECLARE(armv7_arch_timer, "arm,armv7-timer", arch_timer_of_init);
+CLOCKSOURCE_OF_DECLARE(armv8_arch_timer, "arm,armv8-timer", arch_timer_of_init);
+
+#ifdef CONFIG_ACPI
+static void __init register_arch_interrupt(u32 interrupt, u32 flags,
+					int *arch_timer_ppi)
+{
+	int trigger, polarity;
+
+	if (!interrupt || !arch_timer_ppi)
+		return;
+
+	trigger = (flags & ACPI_GTDT_INTERRUPT_MODE) ? ACPI_EDGE_SENSITIVE
+			: ACPI_LEVEL_SENSITIVE;
+
+	polarity = (flags & ACPI_GTDT_INTERRUPT_POLARITY) ? ACPI_ACTIVE_LOW
+			: ACPI_ACTIVE_HIGH;
+
+	*arch_timer_ppi = acpi_register_gsi(NULL, interrupt, trigger,
+						polarity);
+}
+
+static int __init acpi_parse_gtdt(struct acpi_table_header *table)
+{
+	struct acpi_table_gtdt *gtdt;
+
+	gtdt = (struct acpi_table_gtdt *)table;
+	if (!gtdt)
+		return -EINVAL;
+
+	arch_timer_rate = arch_timer_get_cntfrq();
+
+	if (!arch_timer_rate) {
+		pr_warn("arch_timer: Could not get frequency from CNTFREG\n");
+		return -EINVAL;
+	}
+
+	register_arch_interrupt(gtdt->secure_pl1_interrupt,
+		gtdt->secure_pl1_flags, &arch_timer_ppi[PHYS_SECURE_PPI]);
+
+	register_arch_interrupt(gtdt->non_secure_pl1_interrupt,
+		gtdt->non_secure_pl1_flags, &arch_timer_ppi[PHYS_NONSECURE_PPI]);
+
+	register_arch_interrupt(gtdt->virtual_timer_interrupt,
+		gtdt->virtual_timer_flags, &arch_timer_ppi[VIRT_PPI]);
+
+	register_arch_interrupt(gtdt->non_secure_pl2_interrupt,
+		gtdt->non_secure_pl2_flags, &arch_timer_ppi[HYP_PPI]);
+
+	return 0;
+}
+
+void __init arch_timer_acpi_init(struct acpi_table_header *table)
+{
+	if (arch_timers_present & ARCH_CP15_TIMER) {
+		pr_warn("arch_timer: already initialized, skipping\n");
+		return;
+	}
+
+	if (acpi_parse_gtdt(table))
+		return;
+
+	arch_timers_present |= ARCH_CP15_TIMER;
+	arch_timer_init();
+}
+#endif
 
 static void __init arch_timer_mem_init(struct device_node *np)
 {
-- 
1.7.9.5

[PATCH 18/20] clocksource / acpi: Add macro CLOCKSOURCE_ACPI_DECLARE

[Hanjun Guo]

From: Amit Daniel Kachhap <amit.daniel@samsung.com>

This macro does the same job as CLOCKSOURCE_OF_DECLARE. The device
name from the ACPI timer table is matched with all the registered
timer controllers and matching initialisation routine is invoked.

Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 include/asm-generic/vmlinux.lds.h |   10 ++++++++++
 include/linux/clocksource.h       |   12 ++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index bc2121f..3216bee 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -148,6 +148,15 @@
 #define CLKSRC_OF_TABLES()
 #endif
 
+#ifdef CONFIG_ACPI
+#define CLKSRC_ACPI_TABLES() . = ALIGN(8);				\
+			   VMLINUX_SYMBOL(__clksrc_acpi_table) = .;	\
+			   *(__clksrc_acpi_table)			\
+			   *(__clksrc_acpi_table_end)
+#else
+#define CLKSRC_ACPI_TABLES()
+#endif
+
 #ifdef CONFIG_IRQCHIP
 #define IRQCHIP_OF_MATCH_TABLE()					\
 	. = ALIGN(8);							\
@@ -491,6 +500,7 @@
 	MEM_DISCARD(init.rodata)					\
 	CLK_OF_TABLES()							\
 	CLKSRC_OF_TABLES()						\
+	CLKSRC_ACPI_TABLES()						\
 	KERNEL_DTB()							\
 	IRQCHIP_OF_MATCH_TABLE()
 
diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index 67301a4..1d500cf 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -341,6 +341,8 @@ extern int clocksource_i8253_init(void);
 
 struct device_node;
 typedef void(*clocksource_of_init_fn)(struct device_node *);
+typedef void(*clocksource_acpi_init_fn)(void);
+
 #ifdef CONFIG_CLKSRC_OF
 extern void clocksource_of_init(void);
 
@@ -358,4 +360,14 @@ static inline void clocksource_of_init(void) {}
 		     .data = (fn == (clocksource_of_init_fn)NULL) ? fn : fn }
 #endif
 
+#ifdef CONFIG_ACPI
+#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn)			\
+	static const struct acpi_device_id __clksrc_acpi_table_##name	\
+		__used __section(__clksrc_acpi_table)			\
+		 = { .id = compat,				\
+		     .driver_data = (kernel_ulong_t)fn }
+#else
+#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn)
+#endif
+
 #endif /* _LINUX_CLOCKSOURCE_H */
-- 
1.7.9.5

[PATCH 19/20] clocksource / ACPI: Introduce clocksource_acpi_init() using CLOCKSOURCE_ACPI_DECLARE

[Hanjun Guo]

acpi_table_parse() will find table with table id and run handler
on it. So we use CLOCKSOURCE_ACPI_DECLARE to decalare timer tables
and glue its handler, then initialize them in clocksource_acpi_init().

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
---
 drivers/clocksource/Makefile         |    1 +
 drivers/clocksource/arm_arch_timer.c |    1 +
 drivers/clocksource/clksrc-acpi.c    |   36 ++++++++++++++++++++++++++++++++++
 include/linux/clocksource.h          |    3 +++
 4 files changed, 41 insertions(+)
 create mode 100644 drivers/clocksource/clksrc-acpi.c

diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 33621ef..2304c99 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -1,4 +1,5 @@
 obj-$(CONFIG_CLKSRC_OF)	+= clksrc-of.o
+obj-$(CONFIG_ACPI)	+= clksrc-acpi.o
 obj-$(CONFIG_ATMEL_TCB_CLKSRC)	+= tcb_clksrc.o
 obj-$(CONFIG_X86_CYCLONE_TIMER)	+= cyclone.o
 obj-$(CONFIG_X86_PM_TIMER)	+= acpi_pm.o
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 1fa5f67..6489037 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -738,6 +738,7 @@ void __init arch_timer_acpi_init(struct acpi_table_header *table)
 	arch_timers_present |= ARCH_CP15_TIMER;
 	arch_timer_init();
 }
+CLOCKSOURCE_ACPI_DECLARE(armv8_arch_timer, "GTDT", arch_timer_acpi_init);
 #endif
 
 static void __init arch_timer_mem_init(struct device_node *np)
diff --git a/drivers/clocksource/clksrc-acpi.c b/drivers/clocksource/clksrc-acpi.c
new file mode 100644
index 0000000..41efc83
--- /dev/null
+++ b/drivers/clocksource/clksrc-acpi.c
@@ -0,0 +1,36 @@
+/*
+ * Copyright (c) 2014, Hanjun Guo <hanjun.guo@linaro.org>
+ * Copyright (c) 2014, Amit Daniel Kachhap <amit.daniel@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/init.h>
+#include <linux/acpi.h>
+#include <linux/clocksource.h>
+
+extern struct acpi_device_id __clksrc_acpi_table[];
+
+static const struct acpi_device_id __clksrc_acpi_table_sentinel
+	__used __section(__clksrc_acpi_table_end);
+
+void __init clocksource_acpi_init(void)
+{
+	const struct acpi_device_id *id;
+	acpi_tbl_table_handler init_func;
+
+	for (id = __clksrc_acpi_table; id->id[0]; id++) {
+		init_func = (acpi_tbl_table_handler)id->driver_data;
+		acpi_table_parse(id->id, init_func);
+	}
+}
diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index 1d500cf..9bee8b3 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -361,12 +361,15 @@ static inline void clocksource_of_init(void) {}
 #endif
 
 #ifdef CONFIG_ACPI
+extern void clocksource_acpi_init(void);
+
 #define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn)			\
 	static const struct acpi_device_id __clksrc_acpi_table_##name	\
 		__used __section(__clksrc_acpi_table)			\
 		 = { .id = compat,				\
 		     .driver_data = (kernel_ulong_t)fn }
 #else
+static inline void clocksource_acpi_init(void) { return; }
 #define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn)
 #endif
 
-- 
1.7.9.5

[PATCH 20/20] ARM64 / clocksource: Use clocksource_acpi_init()

[Hanjun Guo]

Use clocksource_acpi_init() on ARM64 to initialise timers
in ACPI way when DT is not available.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 arch/arm64/kernel/time.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c
index 29c39d5..6e453ac 100644
--- a/arch/arm64/kernel/time.c
+++ b/arch/arm64/kernel/time.c
@@ -67,6 +67,12 @@ void __init time_init(void)
 
 	clocksource_of_init();
 
+	/*
+	 * Since ACPI or FDT will only one be available in the system,
+	 * we can use clocksource_acpi_init() here safely
+	 */
+	clocksource_acpi_init();
+
 	arch_timer_rate = arch_timer_get_rate();
 	if (!arch_timer_rate)
 		panic("Unable to initialise architected timer.\n");
-- 
1.7.9.5

Re: [PATCH 06/20] ARM64 / ACPI: Introduce some PCI functions when PCI is enabled

[Arnd Bergmann]

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.

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

	Arnd

Re: [PATCH 04/20] ARM64 / ACPI: Introduce arm_core.c and its related head file

[Will Deacon]

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

Re: [PATCH 09/20] ARM64 / ACPI: Implement core functions for parsing MADT table

[Arnd Bergmann]

On Friday 17 January 2014, Hanjun Guo wrote:
>  
> +/*
> + * Local interrupt controller address,
> + * GIC cpu interface base address on ARM/ARM64
> + */
> +static u64 acpi_lapic_addr __initdata;

If it's cpu local, don't you need more than one address to support SMP?
Also, the variable appears to be write-only.

> +#define BAD_MADT_ENTRY(entry, end) (					\
> +	(!entry) || (unsigned long)entry + sizeof(*entry) > end ||	\
> +	((struct acpi_subtable_header *)entry)->length < sizeof(*entry))
> +

Better make this an inline function.

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

You don't need the initialization in the first line when you write to the
variable before reading it. Same in the other functions.

	Arnd

Re: [PATCH 12/20] ARM64 / ACPI: Use Parked Address in GIC structure for spin table SMP initialisation

[Arnd Bergmann]

On Friday 17 January 2014, Hanjun Guo wrote:
> +/* Parked Address in ACPI GIC structure */
> +static u64 parked_address[NR_CPUS];

Please use the per_cpu infrastructure rather than NR_CPUS long arrays.

	Arnd

Re: [PATCH 18/20] clocksource / acpi: Add macro CLOCKSOURCE_ACPI_DECLARE

[Arnd Bergmann]

On Friday 17 January 2014, Hanjun Guo wrote:
> 
> From: Amit Daniel Kachhap <amit.daniel@samsung.com>
> 
> This macro does the same job as CLOCKSOURCE_OF_DECLARE. The device
> name from the ACPI timer table is matched with all the registered
> timer controllers and matching initialisation routine is invoked.

I wouldn't anticipate this infrastructure to be required. Shouldn't all
ARMv8 machines have an architected timer?

	Arnd

Re: [PATCH 02/20] ARM64 : Add dummy asm/cpu.h

[Sudeep Holla]

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

Re: [PATCH 12/20] ARM64 / ACPI: Use Parked Address in GIC structure for spin table SMP initialisation

[Tomasz Nowicki]

On 01/17/14 15:15, Arnd Bergmann wrote:
> On Friday 17 January 2014, Hanjun Guo wrote:
>> +/* Parked Address in ACPI GIC structure */
>> +static u64 parked_address[NR_CPUS];
>
> Please use the per_cpu infrastructure rather than NR_CPUS long arrays.
Actually, per_cpu areas are initialized a bit later. If we really want 
to use here per_cpu, then we need to implement early_per_cpu 
Frankenstein like x86 does.

Tomasz

Re: [PATCH 01/20] ARM64 / ACPI: Make PCI optional for ACPI on ARM64

[Bjorn Helgaas]

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

Re: [PATCH 10/20] ARM64 / ACPI: Enumerate possible/present CPU set and map logical cpu id to APIC id

[Sudeep Holla]

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

Re: [PATCH 04/20] ARM64 / ACPI: Introduce arm_core.c and its related head file

[Hanjun Guo]

On 2014-1-17 22:12, Will Deacon wrote:
> 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?

Sorry, my colleague Graeme had integrate them as one function but I
forgot to merge them in this patch, my bad, will update it in next
version.

Thanks
Hanjun

Re: [PATCH 06/20] ARM64 / ACPI: Introduce some PCI functions when PCI is enabled

[Hanjun Guo]

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

Re: [PATCH 06/20] ARM64 / ACPI: Introduce some PCI functions when PCI is enabled

[Arnd Bergmann]

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

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

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

	Arnd

Re: [PATCH 09/20] ARM64 / ACPI: Implement core functions for parsing MADT table

[Hanjun Guo]

On 2014-1-17 22:12, Arnd Bergmann wrote:
> On Friday 17 January 2014, Hanjun Guo wrote:
>>  
>> +/*
>> + * Local interrupt controller address,
>> + * GIC cpu interface base address on ARM/ARM64
>> + */
>> +static u64 acpi_lapic_addr __initdata;
> 
> If it's cpu local, don't you need more than one address to support SMP?

Good point, thanks for pointing this out. I have a question, do we really
have some SoCs without banked registers?

I ask this question because we can do something for GIC cpu interface
with per cpu offset, but ACPI do NOT support per cpu offset for GIC
distributor.

> Also, the variable appears to be write-only.

Actually not, it will be used for GIC initialization.

> 
>> +#define BAD_MADT_ENTRY(entry, end) (					\
>> +	(!entry) || (unsigned long)entry + sizeof(*entry) > end ||	\
>> +	((struct acpi_subtable_header *)entry)->length < sizeof(*entry))
>> +
> 
> Better make this an inline function.
> 
>> +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;
> 
> You don't need the initialization in the first line when you write to the
> variable before reading it. Same in the other functions.

Ok, I will update them all in next version.

Thanks
hanjun

Re: [PATCH 02/20] ARM64 : Add dummy asm/cpu.h

[Hanjun Guo]

On 2014-1-17 22:22, Sudeep Holla wrote:
> 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

Ok, will remove it.

Thanks
Hanjun

Re: [PATCH 18/20] clocksource / acpi: Add macro CLOCKSOURCE_ACPI_DECLARE

[Hanjun Guo]

On 2014-1-17 22:21, Arnd Bergmann wrote:
> On Friday 17 January 2014, Hanjun Guo wrote:
>>
>> From: Amit Daniel Kachhap <amit.daniel@samsung.com>
>>
>> This macro does the same job as CLOCKSOURCE_OF_DECLARE. The device
>> name from the ACPI timer table is matched with all the registered
>> timer controllers and matching initialisation routine is invoked.
> 
> I wouldn't anticipate this infrastructure to be required. Shouldn't all
> ARMv8 machines have an architected timer?

I not sure of this, could anyone can give some guidance? if only arch
timer is available for ARM64, this will make thing very simple.

Thanks
Hanjun

Re: [PATCH 03/20] ARM64 / ACPI: Introduce the skeleton of _PDC related for ARM64

[Hanjun Guo]

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

Re: [PATCH 07/20] ARM64 / ACPI: Enable ARM64 in Kconfig

[Hanjun Guo]

On 2014-1-17 22:34, Sudeep Holla wrote:
> 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 ?

As I said in the change log, acpi_idle driver is x86/IA64 dependent now,
we should revisit it later for these drivers on ARM64.

Since this is the ARM ACPI core patches, I think we can implement it later.

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

boot_option_idle_override is used in processor_core.c and it is the common
file used by every processor drivers including the processor_idle driver,
if I didn't introduce boot_option_idle_override there will be compile errors.

Thanks
Hanjun

Re: [PATCH 01/20] ARM64 / ACPI: Make PCI optional for ACPI on ARM64

[Hanjun Guo]

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

Re: [PATCH 04/20] ARM64 / ACPI: Introduce arm_core.c and its related head file

[Hanjun Guo]

On 2014-1-18 0:56, Sudeep Holla wrote:
> 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 ?

Yes. it is about the hardware id of CPU and may refer to MPIDR in ARM/ARM64.

Thanks
Hanjun

Re: [PATCH 10/20] ARM64 / ACPI: Enumerate possible/present CPU set and map logical cpu id to APIC id

[Hanjun Guo]

On 2014年01月18日 01:37, Sudeep Holla wrote:
> 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.

yes, agree with you. I mixed up the GIC Id and UID, as you said, UID can be
MPIDR, and then use as hardware id to map to the logical id, it is not the
same as x86 and ia64, will update this patch.

Thanks
Hanjun

Re: [PATCH 06/20] ARM64 / ACPI: Introduce some PCI functions when PCI is enabled

[Hanjun Guo]

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

Re: [PATCH 06/20] ARM64 / ACPI: Introduce some PCI functions when PCI is enabled

[Arnd Bergmann]

On Monday 20 January 2014, Hanjun Guo wrote:
> >> 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.

In general, I recommend not planning for the future in kernel code when you
don't know what is going to happen. It's always easy enough to change
things once you get there, as long as no stable ABI is involved.

I just looked at the caller of these functions, and found a self-contained
PCI driver in drivers/pci/ioapic.c, which uses two sepate PCI classes for
ioapic and ioxapic. I think it's a safe assumption to say that even if we
get ARM CPU+GIC hotplug, that would not use the same ioapic driver. This
driver is currently marked X86-only, and that should probably stay this way,
so you won't need the hooks.

	Arnd

Re: [PATCH 06/20] ARM64 / ACPI: Introduce some PCI functions when PCI is enabled

[Hanjun Guo]

On 2014-1-21 2:39, Arnd Bergmann wrote:
> On Monday 20 January 2014, Hanjun Guo wrote:
>>>> 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.
> 
> In general, I recommend not planning for the future in kernel code when you
> don't know what is going to happen. It's always easy enough to change
> things once you get there, as long as no stable ABI is involved.

Ok, I agree with you.

> 
> I just looked at the caller of these functions, and found a self-contained
> PCI driver in drivers/pci/ioapic.c, which uses two sepate PCI classes for
> ioapic and ioxapic. I think it's a safe assumption to say that even if we
> get ARM CPU+GIC hotplug, that would not use the same ioapic driver. This
> driver is currently marked X86-only, and that should probably stay this way,
> so you won't need the hooks.

Will find a suitable way to fix that in next version, thanks for you comments :)

Hanjun

Re: [PATCH 18/20] clocksource / acpi: Add macro CLOCKSOURCE_ACPI_DECLARE

[Linus Walleij]

On Fri, Jan 17, 2014 at 1:25 PM, Hanjun Guo <hanjun.guo@linaro.org> wrote:

> From: Amit Daniel Kachhap <amit.daniel@samsung.com>
>
> This macro does the same job as CLOCKSOURCE_OF_DECLARE. The device
> name from the ACPI timer table is matched with all the registered
> timer controllers and matching initialisation routine is invoked.
>
> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>

Actually I have a fat patch renaming CLOCKSOURCE_OF_DECLARE()
to TIMER_OF_DECLARE() and I think this macro, if needed, should
be named TIMER_ACPI_DECLARE().

The reason is that "clocksource" is a Linux-internal name and this
macro pertains to the hardware name in respective system
description type.

> +#ifdef CONFIG_ACPI
> +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn)                     \
> +       static const struct acpi_device_id __clksrc_acpi_table_##name   \
> +               __used __section(__clksrc_acpi_table)                   \
> +                = { .id = compat,                              \
> +                    .driver_data = (kernel_ulong_t)fn }
> +#else
> +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn)
> +#endif

This hammers down the world to compile one binary for ACPI
and one binary for device tree. Maybe that's fine, I don't know.

Yours,
Linus Walleij

Re: [PATCH 18/20] clocksource / acpi: Add macro CLOCKSOURCE_ACPI_DECLARE

[Mark Rutland]

On Wed, Jan 22, 2014 at 08:26:50AM +0000, Linus Walleij wrote:
> On Fri, Jan 17, 2014 at 1:25 PM, Hanjun Guo <hanjun.guo@linaro.org> wrote:
> 
> > From: Amit Daniel Kachhap <amit.daniel@samsung.com>
> >
> > This macro does the same job as CLOCKSOURCE_OF_DECLARE. The device
> > name from the ACPI timer table is matched with all the registered
> > timer controllers and matching initialisation routine is invoked.
> >
> > Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
> > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> 
> Actually I have a fat patch renaming CLOCKSOURCE_OF_DECLARE()
> to TIMER_OF_DECLARE() and I think this macro, if needed, should
> be named TIMER_ACPI_DECLARE().
> 
> The reason is that "clocksource" is a Linux-internal name and this
> macro pertains to the hardware name in respective system
> description type.
> 
> > +#ifdef CONFIG_ACPI
> > +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn)                     \
> > +       static const struct acpi_device_id __clksrc_acpi_table_##name   \
> > +               __used __section(__clksrc_acpi_table)                   \
> > +                = { .id = compat,                              \
> > +                    .driver_data = (kernel_ulong_t)fn }
> > +#else
> > +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn)
> > +#endif
> 
> This hammers down the world to compile one binary for ACPI
> and one binary for device tree. Maybe that's fine, I don't know.

How does it do that?

As far as I could tell CONFIG_ACPI and CONFIG_OF are not mutually
exclusive, and this just means that we only build the datastructures for
matching from ACPI when CONFIG_ACPI is enabled.

Have I missed something?

I definitely don't want to see mutually exclusive ACPI and DT support.

Cheers,
Mark.

Re: [PATCH 18/20] clocksource / acpi: Add macro CLOCKSOURCE_ACPI_DECLARE

[Mark Rutland]

On Mon, Jan 20, 2014 at 09:08:32AM +0000, Hanjun Guo wrote:
> On 2014-1-17 22:21, Arnd Bergmann wrote:
> > On Friday 17 January 2014, Hanjun Guo wrote:
> >>
> >> From: Amit Daniel Kachhap <amit.daniel@samsung.com>
> >>
> >> This macro does the same job as CLOCKSOURCE_OF_DECLARE. The device
> >> name from the ACPI timer table is matched with all the registered
> >> timer controllers and matching initialisation routine is invoked.
> > 
> > I wouldn't anticipate this infrastructure to be required. Shouldn't all
> > ARMv8 machines have an architected timer?
> 
> I not sure of this, could anyone can give some guidance? if only arch
> timer is available for ARM64, this will make thing very simple.

All ARMv8 systems should have an architected timer.

However, they may also have other timers (e.g. global timers for use
when CPUs are in low power states and their local architected timers
aren't active).

Thanks,
Mark.

Re: [PATCH 04/20] ARM64 / ACPI: Introduce arm_core.c and its related head file

[Lorenzo Pieralisi]

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

Re: [PATCH 18/20] clocksource / acpi: Add macro CLOCKSOURCE_ACPI_DECLARE

[Linus Walleij]

On Wed, Jan 22, 2014 at 12:45 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Jan 22, 2014 at 08:26:50AM +0000, Linus Walleij wrote:

>> This hammers down the world to compile one binary for ACPI
>> and one binary for device tree. Maybe that's fine, I don't know.
>
> How does it do that?

Bah it doesn't I misread the patch. Forget that comment.

Yours,
Linus Walleij

Re: [PATCH 18/20] clocksource / acpi: Add macro CLOCKSOURCE_ACPI_DECLARE

[Arnd Bergmann]

On Wednesday 22 January 2014 11:46:16 Mark Rutland wrote:
> On Mon, Jan 20, 2014 at 09:08:32AM +0000, Hanjun Guo wrote:
> > On 2014-1-17 22:21, Arnd Bergmann wrote:
> > > On Friday 17 January 2014, Hanjun Guo wrote:
> > >>
> > >> From: Amit Daniel Kachhap <amit.daniel@samsung.com>
> > >>
> > >> This macro does the same job as CLOCKSOURCE_OF_DECLARE. The device
> > >> name from the ACPI timer table is matched with all the registered
> > >> timer controllers and matching initialisation routine is invoked.
> > > 
> > > I wouldn't anticipate this infrastructure to be required. Shouldn't all
> > > ARMv8 machines have an architected timer?
> > 
> > I not sure of this, could anyone can give some guidance? if only arch
> > timer is available for ARM64, this will make thing very simple.
> 
> All ARMv8 systems should have an architected timer.
> 
> However, they may also have other timers (e.g. global timers for use
> when CPUs are in low power states and their local architected timers
> aren't active).

But all other timers could be regular platform drivers I suppose. No
need for special infrastructure for those.

	Arnd

Re: [PATCH 18/20] clocksource / acpi: Add macro CLOCKSOURCE_ACPI_DECLARE

[Mark Rutland]

On Wed, Jan 22, 2014 at 02:56:05PM +0000, Arnd Bergmann wrote:
> On Wednesday 22 January 2014 11:46:16 Mark Rutland wrote:
> > On Mon, Jan 20, 2014 at 09:08:32AM +0000, Hanjun Guo wrote:
> > > On 2014-1-17 22:21, Arnd Bergmann wrote:
> > > > On Friday 17 January 2014, Hanjun Guo wrote:
> > > >>
> > > >> From: Amit Daniel Kachhap <amit.daniel@samsung.com>
> > > >>
> > > >> This macro does the same job as CLOCKSOURCE_OF_DECLARE. The device
> > > >> name from the ACPI timer table is matched with all the registered
> > > >> timer controllers and matching initialisation routine is invoked.
> > > > 
> > > > I wouldn't anticipate this infrastructure to be required. Shouldn't all
> > > > ARMv8 machines have an architected timer?
> > > 
> > > I not sure of this, could anyone can give some guidance? if only arch
> > > timer is available for ARM64, this will make thing very simple.
> > 
> > All ARMv8 systems should have an architected timer.
> > 
> > However, they may also have other timers (e.g. global timers for use
> > when CPUs are in low power states and their local architected timers
> > aren't active).
> 
> But all other timers could be regular platform drivers I suppose. No
> need for special infrastructure for those.

Except for the fact that some timers / clocksources that we already have
in 32-bit land will likely be reused in 64-bit SoC designs. People will
want to use the same driver for both 32-bit and 64-bit kernels, and thus
we need CLOCKSOURCE_OF_DECLARE in 64-bit kernels.

Those platforms which will have ACPI will likely reuse existing timer IP
blocks, and will want to make minimal changes to the driver, which will
likely be using CLOCKSOURCE_OF_DECLARE. We can't change these to
platform drivers or we can break existing systems because the timers
will be registered too late.

I don't see how we can share drivers between 32-bit and 64-bit kernels
without sharing a common driver model, and I think it makes sense to
have some uniformity across drivers (i.e. always use
CLOCKSOURCE_*_DECLARE rather than sometimes using platform drivers).

Thanks,
Mark.

Re: [PATCH 18/20] clocksource / acpi: Add macro CLOCKSOURCE_ACPI_DECLARE

[Arnd Bergmann]

On Wednesday 22 January 2014 15:17:49 Mark Rutland wrote:
> Except for the fact that some timers / clocksources that we already have
> in 32-bit land will likely be reused in 64-bit SoC designs. People will
> want to use the same driver for both 32-bit and 64-bit kernels, and thus
> we need CLOCKSOURCE_OF_DECLARE in 64-bit kernels.
> 
> Those platforms which will have ACPI will likely reuse existing timer IP
> blocks, and will want to make minimal changes to the driver, which will
> likely be using CLOCKSOURCE_OF_DECLARE. We can't change these to
> platform drivers or we can break existing systems because the timers
> will be registered too late.
> 
> I don't see how we can share drivers between 32-bit and 64-bit kernels
> without sharing a common driver model, and I think it makes sense to
> have some uniformity across drivers (i.e. always use
> CLOCKSOURCE_*_DECLARE rather than sometimes using platform drivers).

This still sounds like speculation. I would defer this change until
we actually have a platform that needs it.

The platforms that would reuse a lot of IP blocks are most likely
embedded systems and /not/ server hardware following some strict
specification, so they wouldn't use ACPI.

	Arnd

Re: [PATCH 10/20] ARM64 / ACPI: Enumerate possible/present CPU set and map logical cpu id to APIC id

[Lorenzo Pieralisi]

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

Re: [Linaro-acpi] [PATCH 04/20] ARM64 / ACPI: Introduce arm_core.c and its related head file

[Tomasz Nowicki]

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().

Regards,
Tomasz
>
> [...]
>
>> +/*
>> + * __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
>
>
> _______________________________________________
> Linaro-acpi mailing list
> Linaro-acpi@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-acpi
>

Re: [PATCH 02/20] ARM64 : Add dummy asm/cpu.h

[Catalin Marinas]

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?

-- 
Catalin

Re: [PATCH 03/20] ARM64 / ACPI: Introduce the skeleton of _PDC related for ARM64

[Catalin Marinas]

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.

-- 
Catalin

Re: [PATCH 07/20] ARM64 / ACPI: Enable ARM64 in Kconfig

[Catalin Marinas]

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.

-- 
Catalin

Re: [PATCH 11/20] ARM64 / ACPI: Get the enable method for SMP initialization

[Catalin Marinas]

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?

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

-- 
Catalin

Re: [PATCH 09/20] ARM64 / ACPI: Implement core functions for parsing MADT table

[Marc Zyngier]

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.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 03/20] ARM64 / ACPI: Introduce the skeleton of _PDC related for ARM64

[Catalin Marinas]

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

-- 
Catalin

Re: [PATCH 18/20] clocksource / acpi: Add macro CLOCKSOURCE_ACPI_DECLARE

[Hanjun Guo]

Hi Linus,

Sorry for the late reply.

On 2014年01月22日 16:26, Linus Walleij wrote:
> On Fri, Jan 17, 2014 at 1:25 PM, Hanjun Guo <hanjun.guo@linaro.org> wrote:
>
>> From: Amit Daniel Kachhap <amit.daniel@samsung.com>
>>
>> This macro does the same job as CLOCKSOURCE_OF_DECLARE. The device
>> name from the ACPI timer table is matched with all the registered
>> timer controllers and matching initialisation routine is invoked.
>>
>> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> Actually I have a fat patch renaming CLOCKSOURCE_OF_DECLARE()
> to TIMER_OF_DECLARE() and I think this macro, if needed, should
> be named TIMER_ACPI_DECLARE().
>
> The reason is that "clocksource" is a Linux-internal name and this
> macro pertains to the hardware name in respective system
> description type.

That make sense to me too, I will update in next version if
this patch is still needed.

>
>> +#ifdef CONFIG_ACPI
>> +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn)                     \
>> +       static const struct acpi_device_id __clksrc_acpi_table_##name   \
>> +               __used __section(__clksrc_acpi_table)                   \
>> +                = { .id = compat,                              \
>> +                    .driver_data = (kernel_ulong_t)fn }
>> +#else
>> +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn)
>> +#endif
> This hammers down the world to compile one binary for ACPI
> and one binary for device tree. Maybe that's fine, I don't know.

This is a problem we can have some discussion on it.
I prefer mutually exclusive ACPI and DT support.

Thanks
Hanjun

Re: [PATCH 18/20] clocksource / acpi: Add macro CLOCKSOURCE_ACPI_DECLARE

[Hanjun Guo]

On 2014年01月22日 19:45, Mark Rutland wrote:
> On Wed, Jan 22, 2014 at 08:26:50AM +0000, Linus Walleij wrote:
>> On Fri, Jan 17, 2014 at 1:25 PM, Hanjun Guo <hanjun.guo@linaro.org> wrote:
>>
>>> From: Amit Daniel Kachhap <amit.daniel@samsung.com>
>>>
>>> This macro does the same job as CLOCKSOURCE_OF_DECLARE. The device
>>> name from the ACPI timer table is matched with all the registered
>>> timer controllers and matching initialisation routine is invoked.
>>>
>>> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>> Actually I have a fat patch renaming CLOCKSOURCE_OF_DECLARE()
>> to TIMER_OF_DECLARE() and I think this macro, if needed, should
>> be named TIMER_ACPI_DECLARE().
>>
>> The reason is that "clocksource" is a Linux-internal name and this
>> macro pertains to the hardware name in respective system
>> description type.
>>
>>> +#ifdef CONFIG_ACPI
>>> +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn)                     \
>>> +       static const struct acpi_device_id __clksrc_acpi_table_##name   \
>>> +               __used __section(__clksrc_acpi_table)                   \
>>> +                = { .id = compat,                              \
>>> +                    .driver_data = (kernel_ulong_t)fn }
>>> +#else
>>> +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn)
>>> +#endif
>> This hammers down the world to compile one binary for ACPI
>> and one binary for device tree. Maybe that's fine, I don't know.
> How does it do that?
>
> As far as I could tell CONFIG_ACPI and CONFIG_OF are not mutually
> exclusive, and this just means that we only build the datastructures for
> matching from ACPI when CONFIG_ACPI is enabled.
>
> Have I missed something?
>
> I definitely don't want to see mutually exclusive ACPI and DT support.

ACPI and DT did the same job so I think they should mutually exclusive.
if we enable both DT and ACPI in one system, this will leading confusions.

Further more, firmware guys will be happy to present all the devices
in one way, not both of them.

Thanks
Hanjun

Re: [PATCH 18/20] clocksource / acpi: Add macro CLOCKSOURCE_ACPI_DECLARE

[Hanjun Guo]

On 2014年01月22日 19:46, Mark Rutland wrote:
> On Mon, Jan 20, 2014 at 09:08:32AM +0000, Hanjun Guo wrote:
>> On 2014-1-17 22:21, Arnd Bergmann wrote:
>>> On Friday 17 January 2014, Hanjun Guo wrote:
>>>> From: Amit Daniel Kachhap <amit.daniel@samsung.com>
>>>>
>>>> This macro does the same job as CLOCKSOURCE_OF_DECLARE. The device
>>>> name from the ACPI timer table is matched with all the registered
>>>> timer controllers and matching initialisation routine is invoked.
>>> I wouldn't anticipate this infrastructure to be required. Shouldn't all
>>> ARMv8 machines have an architected timer?
>> I not sure of this, could anyone can give some guidance? if only arch
>> timer is available for ARM64, this will make thing very simple.
> All ARMv8 systems should have an architected timer.

Thanks for the clarification :)

>
> However, they may also have other timers (e.g. global timers for use
> when CPUs are in low power states and their local architected timers
> aren't active).

Only arch timer (generic timer) is defined as a table in ACPI 5.0
spec at now, so other timers will not described as table and will
described as device object in DSDT table, so do you think we need
this framework or not?

Thanks
Hanjun

Re: [Linaro-acpi] [PATCH 04/20] ARM64 / ACPI: Introduce arm_core.c and its related head file

[Hanjun Guo]

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

Re: [PATCH 18/20] clocksource / acpi: Add macro CLOCKSOURCE_ACPI_DECLARE

[Hanjun Guo]

On 2014年01月22日 22:56, Arnd Bergmann wrote:
> On Wednesday 22 January 2014 11:46:16 Mark Rutland wrote:
>> On Mon, Jan 20, 2014 at 09:08:32AM +0000, Hanjun Guo wrote:
>>> On 2014-1-17 22:21, Arnd Bergmann wrote:
>>>> On Friday 17 January 2014, Hanjun Guo wrote:
>>>>> From: Amit Daniel Kachhap <amit.daniel@samsung.com>
>>>>>
>>>>> This macro does the same job as CLOCKSOURCE_OF_DECLARE. The device
>>>>> name from the ACPI timer table is matched with all the registered
>>>>> timer controllers and matching initialisation routine is invoked.
>>>> I wouldn't anticipate this infrastructure to be required. Shouldn't all
>>>> ARMv8 machines have an architected timer?
>>> I not sure of this, could anyone can give some guidance? if only arch
>>> timer is available for ARM64, this will make thing very simple.
>> All ARMv8 systems should have an architected timer.
>>
>> However, they may also have other timers (e.g. global timers for use
>> when CPUs are in low power states and their local architected timers
>> aren't active).
> But all other timers could be regular platform drivers I suppose. No
> need for special infrastructure for those.

One more information, acpi_early_init() is run before timekeeping_init()
now, and that patch was merged into 3.14-rc1. So we can initialize
timers for scanning the DSDT table, and it also means that timer defined
in DSDT can be initialized early enough.

Thanks
Hanjun

Re: [PATCH 18/20] clocksource / acpi: Add macro CLOCKSOURCE_ACPI_DECLARE

[Mark Rutland]

On Fri, Jan 24, 2014 at 12:20:46AM +0000, Hanjun Guo wrote:
> On 2014年01月22日 19:45, Mark Rutland wrote:
> > On Wed, Jan 22, 2014 at 08:26:50AM +0000, Linus Walleij wrote:
> >> On Fri, Jan 17, 2014 at 1:25 PM, Hanjun Guo <hanjun.guo@linaro.org> wrote:
> >>
> >>> From: Amit Daniel Kachhap <amit.daniel@samsung.com>
> >>>
> >>> This macro does the same job as CLOCKSOURCE_OF_DECLARE. The device
> >>> name from the ACPI timer table is matched with all the registered
> >>> timer controllers and matching initialisation routine is invoked.
> >>>
> >>> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
> >>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> >> Actually I have a fat patch renaming CLOCKSOURCE_OF_DECLARE()
> >> to TIMER_OF_DECLARE() and I think this macro, if needed, should
> >> be named TIMER_ACPI_DECLARE().
> >>
> >> The reason is that "clocksource" is a Linux-internal name and this
> >> macro pertains to the hardware name in respective system
> >> description type.
> >>
> >>> +#ifdef CONFIG_ACPI
> >>> +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn)                     \
> >>> +       static const struct acpi_device_id __clksrc_acpi_table_##name   \
> >>> +               __used __section(__clksrc_acpi_table)                   \
> >>> +                = { .id = compat,                              \
> >>> +                    .driver_data = (kernel_ulong_t)fn }
> >>> +#else
> >>> +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn)
> >>> +#endif
> >> This hammers down the world to compile one binary for ACPI
> >> and one binary for device tree. Maybe that's fine, I don't know.
> > How does it do that?
> >
> > As far as I could tell CONFIG_ACPI and CONFIG_OF are not mutually
> > exclusive, and this just means that we only build the datastructures for
> > matching from ACPI when CONFIG_ACPI is enabled.
> >
> > Have I missed something?
> >
> > I definitely don't want to see mutually exclusive ACPI and DT support.
> 
> ACPI and DT did the same job so I think they should mutually exclusive.
> if we enable both DT and ACPI in one system, this will leading confusions.

ACPI and DT do similar jobs, and we should be mutually exclusive at
runtime. However, they should not be mutually exclusive at compile-time.

Being mutually exclusive at compile-time is just broken. It creates more
work for distributions (who need to ship double the number of kernels),
it increases the number of configurations requiring testing, and it
makes it easier for bugs to be introduced. It's just painful, and
there's no reason for it.

At boot time the kernel needs to decide which to use for hardware
description, and completely ignore the other (which should not be
present, but lets not assume that or inevitably someone will break that
assumption for a quick hack).

The same kernel should boot on a system that has a DTB or a system that
has ACPI tables. On a system that's provided both it should use one or
the other, but not both.

Thanks,
Mark.

Re: [PATCH 18/20] clocksource / acpi: Add macro CLOCKSOURCE_ACPI_DECLARE

[Mark Rutland]

On Fri, Jan 24, 2014 at 12:12:24AM +0000, Hanjun Guo wrote:
> Hi Linus,
> 
> Sorry for the late reply.
> 
> On 2014年01月22日 16:26, Linus Walleij wrote:
> > On Fri, Jan 17, 2014 at 1:25 PM, Hanjun Guo <hanjun.guo@linaro.org> wrote:
> >
> >> From: Amit Daniel Kachhap <amit.daniel@samsung.com>
> >>
> >> This macro does the same job as CLOCKSOURCE_OF_DECLARE. The device
> >> name from the ACPI timer table is matched with all the registered
> >> timer controllers and matching initialisation routine is invoked.
> >>
> >> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
> >> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> > Actually I have a fat patch renaming CLOCKSOURCE_OF_DECLARE()
> > to TIMER_OF_DECLARE() and I think this macro, if needed, should
> > be named TIMER_ACPI_DECLARE().
> >
> > The reason is that "clocksource" is a Linux-internal name and this
> > macro pertains to the hardware name in respective system
> > description type.
> 
> That make sense to me too, I will update in next version if
> this patch is still needed.
> 
> >
> >> +#ifdef CONFIG_ACPI
> >> +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn)                     \
> >> +       static const struct acpi_device_id __clksrc_acpi_table_##name   \
> >> +               __used __section(__clksrc_acpi_table)                   \
> >> +                = { .id = compat,                              \
> >> +                    .driver_data = (kernel_ulong_t)fn }
> >> +#else
> >> +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn)
> >> +#endif
> > This hammers down the world to compile one binary for ACPI
> > and one binary for device tree. Maybe that's fine, I don't know.
> 
> This is a problem we can have some discussion on it.
> I prefer mutually exclusive ACPI and DT support.

A lot of work has been put into making a single kernel boot everywhere.
It's forced duplicated code to be factored out, and it's made the kernel
more flexible. While it has been painful, it's forced a far higher
quality standard across the board(s).

Having a separate ACPI-capable or DT-capable kernels goes completely
against that, and it's completely broken:

* It doubles the testing effort required for a particular kernel. I can
  guarantee that we will miss bugs (even amazingly bad build bugs)
  because no-one will be able to test a full suite of kernels.

* It introduces the possibility of completely pointles arbitrary
  differences between the two. How long until we see the first bug-fix
  that only works in one configuration?

* It creates additional work for distributions, which need to build more
  kernels test them, distribute them, and document which platforms which
  kernels are supported on. This creates more pain for end-users too.
  
Eventually we _will_ get fed up with all of those, and we'll have to do
painful invasive work to make the kernel decide at runtime.

Having separate kernels is a lazy shortcut. It's painful for everyone,
leads to a greater maintenance overhead, it's not what we want now and
not what we want in future.

No thanks.

Either the kernel figures out whether or not to deal with ACPI at
runtime, or it doesn't deal with it at all.

Thanks,
Mark.

Re: [Linaro-acpi] [PATCH 04/20] ARM64 / ACPI: Introduce arm_core.c and its related head file

[Lorenzo Pieralisi]

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

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.

Thank you,
Lorenzo

Re: [PATCH 10/20] ARM64 / ACPI: Enumerate possible/present CPU set and map logical cpu id to APIC id

[Hanjun Guo]

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.

>
>> +
>>   #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.

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

Thanks, I will review he code you mentioned and find a solution
for ACPI part.

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

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

I will rework this part to get the GIC cpu interface id, not the MPIDR here.

Thanks
Hanjun

Re: [PATCH 02/20] ARM64 : Add dummy asm/cpu.h

[Hanjun Guo]

On 2014年01月24日 00:15, Catalin Marinas wrote:
> 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?

Of course, I will update in next version :)

Thanks
Hanjun

Re: [PATCH 03/20] ARM64 / ACPI: Introduce the skeleton of _PDC related for ARM64

[Hanjun Guo]

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

Re: [PATCH 07/20] ARM64 / ACPI: Enable ARM64 in Kconfig

[Hanjun Guo]

On 2014年01月24日 00:39, Catalin Marinas wrote:
> 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.

Thanks for the guidance, will update in next version.

>

Re: [PATCH 11/20] ARM64 / ACPI: Get the enable method for SMP initialization

[Hanjun Guo]

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

Re: [PATCH 18/20] clocksource / acpi: Add macro CLOCKSOURCE_ACPI_DECLARE

[Catalin Marinas]

On Fri, Jan 24, 2014 at 12:08:15PM +0000, Mark Rutland wrote:
> On Fri, Jan 24, 2014 at 12:20:46AM +0000, Hanjun Guo wrote:
> > On 2014年01月22日 19:45, Mark Rutland wrote:
> > > On Wed, Jan 22, 2014 at 08:26:50AM +0000, Linus Walleij wrote:
> > >> On Fri, Jan 17, 2014 at 1:25 PM, Hanjun Guo <hanjun.guo@linaro.org> wrote:
> > >>
> > >>> From: Amit Daniel Kachhap <amit.daniel@samsung.com>
> > >>>
> > >>> This macro does the same job as CLOCKSOURCE_OF_DECLARE. The device
> > >>> name from the ACPI timer table is matched with all the registered
> > >>> timer controllers and matching initialisation routine is invoked.
> > >>>
> > >>> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
> > >>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> > >> Actually I have a fat patch renaming CLOCKSOURCE_OF_DECLARE()
> > >> to TIMER_OF_DECLARE() and I think this macro, if needed, should
> > >> be named TIMER_ACPI_DECLARE().
> > >>
> > >> The reason is that "clocksource" is a Linux-internal name and this
> > >> macro pertains to the hardware name in respective system
> > >> description type.
> > >>
> > >>> +#ifdef CONFIG_ACPI
> > >>> +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn)                     \
> > >>> +       static const struct acpi_device_id __clksrc_acpi_table_##name   \
> > >>> +               __used __section(__clksrc_acpi_table)                   \
> > >>> +                = { .id = compat,                              \
> > >>> +                    .driver_data = (kernel_ulong_t)fn }
> > >>> +#else
> > >>> +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn)
> > >>> +#endif
> > >> This hammers down the world to compile one binary for ACPI
> > >> and one binary for device tree. Maybe that's fine, I don't know.
> > > How does it do that?
> > >
> > > As far as I could tell CONFIG_ACPI and CONFIG_OF are not mutually
> > > exclusive, and this just means that we only build the datastructures for
> > > matching from ACPI when CONFIG_ACPI is enabled.
> > >
> > > Have I missed something?
> > >
> > > I definitely don't want to see mutually exclusive ACPI and DT support.
> > 
> > ACPI and DT did the same job so I think they should mutually exclusive.
> > if we enable both DT and ACPI in one system, this will leading confusions.
> 
> ACPI and DT do similar jobs, and we should be mutually exclusive at
> runtime. However, they should not be mutually exclusive at compile-time.
> 
> Being mutually exclusive at compile-time is just broken. It creates more
> work for distributions (who need to ship double the number of kernels),
> it increases the number of configurations requiring testing, and it
> makes it easier for bugs to be introduced. It's just painful, and
> there's no reason for it.

I fully agree (IOW, I'll NAK patches that break this assumption; we want
single kernel image whether it uses DT or ACPI).

> At boot time the kernel needs to decide which to use for hardware
> description, and completely ignore the other (which should not be
> present, but lets not assume that or inevitably someone will break that
> assumption for a quick hack).
> 
> The same kernel should boot on a system that has a DTB or a system that
> has ACPI tables. On a system that's provided both it should use one or
> the other, but not both.

Do we still need the chosen node to be passed via DT for command line,
even if the kernel uses ACPI?

-- 
Catalin

Re: [PATCH 09/20] ARM64 / ACPI: Implement core functions for parsing MADT table

[Hanjun Guo]

Hi Marc,

On 2014年01月24日 01:54, Marc Zyngier wrote:
> 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?

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

> If that's for
> GICv2, 8 is the maximum. For GICv3+, that's incredibly low, and should
> be probed probed at runtime anyway.

I would prefer to do that, but this value is used to
probe CPUs in MADT :)

>
>> +#define MAX_GIC_DISTRIBUTOR   1		/* should be the same as MAX_GIC_NR */
> No support for cascaded GICs?

Yes, no cascade GICs in ACPI at now.

>
>> +
>>   #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?

thanks for the suggesting, I will update.

>
>> +#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.

Ok, I will address your comments above in next version.
>
>> +		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...

Good catch. So I have a question: do we really have some SoCs
without banked registers on ARM64? if not , I think we can use
a single static variable is ok.

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

That make sense to me too, will update.

>
>> +	}
>> +
>> +	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?

Please refer to http://www.acpi.info/

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

good catch, we already addressed this problem, and will
update in next version.

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

this function use printk always, should change all of them?

>
>> +			       "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.

yes, just print some information when booting.

Thank you very much for the comments.

Hanjun

Re: [PATCH 03/20] ARM64 / ACPI: Introduce the skeleton of _PDC related for ARM64

[Hanjun Guo]

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
>

Re: [PATCH 10/20] ARM64 / ACPI: Enumerate possible/present CPU set and map logical cpu id to APIC id

[Lorenzo Pieralisi]

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

Re: [PATCH 18/20] clocksource / acpi: Add macro CLOCKSOURCE_ACPI_DECLARE

[Mark Rutland]

On Fri, Jan 24, 2014 at 03:15:13PM +0000, Catalin Marinas wrote:
> Do we still need the chosen node to be passed via DT for command line,
> even if the kernel uses ACPI?

It depends on how the kernel is booted.

If not booted as a UEFI application, then the user would have to provide
a dtb to pass the command line.

If booted as a UEFI application, the kernel can retrieve the commandline
from UEFI and handle it appropriately (this _might_ involve the EFI stub
building a dtb with a chosen node and pasing that to the kernel, but
that could change as it's within the kernel). In that case, the user
need not build a dtb manually.

However, I'd expect that we use a restricted dtb with only a /chosen
node for passing additional data as an equivalent to x86's bootargs
(which would contain the location of ACPI tables and possibly other
stuff like initrd location if not passed on the command line). That
makes the very early logic in the kernel much simpler, and it gives us
an extensible format that should limit headaches in future.

So if booted with ACPI we _might_ have a restricted chosen-only dtb,
which _might_ be passed to the kernel explicitly by the user.

Thanks,
Mark.

Re: [PATCH 18/20] clocksource / acpi: Add macro CLOCKSOURCE_ACPI_DECLARE

[Hanjun Guo]

Hi Mark,

On 2014年01月24日 20:32, Mark Rutland wrote:
> On Fri, Jan 24, 2014 at 12:12:24AM +0000, Hanjun Guo wrote:
>> Hi Linus,
>>
>> Sorry for the late reply.
>>
>> On 2014年01月22日 16:26, Linus Walleij wrote:
>>> On Fri, Jan 17, 2014 at 1:25 PM, Hanjun Guo <hanjun.guo@linaro.org> wrote:
>>>
>>>> From: Amit Daniel Kachhap <amit.daniel@samsung.com>
>>>>
>>>> This macro does the same job as CLOCKSOURCE_OF_DECLARE. The device
>>>> name from the ACPI timer table is matched with all the registered
>>>> timer controllers and matching initialisation routine is invoked.
>>>>
>>>> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>> Actually I have a fat patch renaming CLOCKSOURCE_OF_DECLARE()
>>> to TIMER_OF_DECLARE() and I think this macro, if needed, should
>>> be named TIMER_ACPI_DECLARE().
>>>
>>> The reason is that "clocksource" is a Linux-internal name and this
>>> macro pertains to the hardware name in respective system
>>> description type.
>> That make sense to me too, I will update in next version if
>> this patch is still needed.
>>
>>>> +#ifdef CONFIG_ACPI
>>>> +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn)                     \
>>>> +       static const struct acpi_device_id __clksrc_acpi_table_##name   \
>>>> +               __used __section(__clksrc_acpi_table)                   \
>>>> +                = { .id = compat,                              \
>>>> +                    .driver_data = (kernel_ulong_t)fn }
>>>> +#else
>>>> +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn)
>>>> +#endif
>>> This hammers down the world to compile one binary for ACPI
>>> and one binary for device tree. Maybe that's fine, I don't know.
>> This is a problem we can have some discussion on it.
>> I prefer mutually exclusive ACPI and DT support.
> A lot of work has been put into making a single kernel boot everywhere.
> It's forced duplicated code to be factored out, and it's made the kernel
> more flexible. While it has been painful, it's forced a far higher
> quality standard across the board(s).
>
> Having a separate ACPI-capable or DT-capable kernels goes completely
> against that, and it's completely broken:
>
> * It doubles the testing effort required for a particular kernel. I can
>    guarantee that we will miss bugs (even amazingly bad build bugs)
>    because no-one will be able to test a full suite of kernels.
>
> * It introduces the possibility of completely pointles arbitrary
>    differences between the two. How long until we see the first bug-fix
>    that only works in one configuration?
>
> * It creates additional work for distributions, which need to build more
>    kernels test them, distribute them, and document which platforms which
>    kernels are supported on. This creates more pain for end-users too.
>    
> Eventually we _will_ get fed up with all of those, and we'll have to do
> painful invasive work to make the kernel decide at runtime.
>
> Having separate kernels is a lazy shortcut. It's painful for everyone,
> leads to a greater maintenance overhead, it's not what we want now and
> not what we want in future.
>
> No thanks.
>
> Either the kernel figures out whether or not to deal with ACPI at
> runtime, or it doesn't deal with it at all.

I fully agree with you for the single kernel image, I didn't notice this 
before, sorry for
my noise about the exclusive ACPI and DT support.

Thank you very much to let things much more clearer :)

Hanjun

Re: [PATCH 18/20] clocksource / acpi: Add macro CLOCKSOURCE_ACPI_DECLARE

[Hanjun Guo]

On 2014年01月24日 23:15, Catalin Marinas wrote:
> On Fri, Jan 24, 2014 at 12:08:15PM +0000, Mark Rutland wrote:
>> On Fri, Jan 24, 2014 at 12:20:46AM +0000, Hanjun Guo wrote:
>>> On 2014年01月22日 19:45, Mark Rutland wrote:
>>>> On Wed, Jan 22, 2014 at 08:26:50AM +0000, Linus Walleij wrote:
>>>>> On Fri, Jan 17, 2014 at 1:25 PM, Hanjun Guo <hanjun.guo@linaro.org> wrote:
>>>>>
>>>>>> From: Amit Daniel Kachhap <amit.daniel@samsung.com>
>>>>>>
>>>>>> This macro does the same job as CLOCKSOURCE_OF_DECLARE. The device
>>>>>> name from the ACPI timer table is matched with all the registered
>>>>>> timer controllers and matching initialisation routine is invoked.
>>>>>>
>>>>>> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
>>>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>>> Actually I have a fat patch renaming CLOCKSOURCE_OF_DECLARE()
>>>>> to TIMER_OF_DECLARE() and I think this macro, if needed, should
>>>>> be named TIMER_ACPI_DECLARE().
>>>>>
>>>>> The reason is that "clocksource" is a Linux-internal name and this
>>>>> macro pertains to the hardware name in respective system
>>>>> description type.
>>>>>
>>>>>> +#ifdef CONFIG_ACPI
>>>>>> +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn)                     \
>>>>>> +       static const struct acpi_device_id __clksrc_acpi_table_##name   \
>>>>>> +               __used __section(__clksrc_acpi_table)                   \
>>>>>> +                = { .id = compat,                              \
>>>>>> +                    .driver_data = (kernel_ulong_t)fn }
>>>>>> +#else
>>>>>> +#define CLOCKSOURCE_ACPI_DECLARE(name, compat, fn)
>>>>>> +#endif
>>>>> This hammers down the world to compile one binary for ACPI
>>>>> and one binary for device tree. Maybe that's fine, I don't know.
>>>> How does it do that?
>>>>
>>>> As far as I could tell CONFIG_ACPI and CONFIG_OF are not mutually
>>>> exclusive, and this just means that we only build the datastructures for
>>>> matching from ACPI when CONFIG_ACPI is enabled.
>>>>
>>>> Have I missed something?
>>>>
>>>> I definitely don't want to see mutually exclusive ACPI and DT support.
>>> ACPI and DT did the same job so I think they should mutually exclusive.
>>> if we enable both DT and ACPI in one system, this will leading confusions.
>> ACPI and DT do similar jobs, and we should be mutually exclusive at
>> runtime. However, they should not be mutually exclusive at compile-time.
>>
>> Being mutually exclusive at compile-time is just broken. It creates more
>> work for distributions (who need to ship double the number of kernels),
>> it increases the number of configurations requiring testing, and it
>> makes it easier for bugs to be introduced. It's just painful, and
>> there's no reason for it.
> I fully agree (IOW, I'll NAK patches that break this assumption; we want
> single kernel image whether it uses DT or ACPI).

I will not break this in next version, because I totally agree with Mark 
too :)

>
>> At boot time the kernel needs to decide which to use for hardware
>> description, and completely ignore the other (which should not be
>> present, but lets not assume that or inevitably someone will break that
>> assumption for a quick hack).
>>
>> The same kernel should boot on a system that has a DTB or a system that
>> has ACPI tables. On a system that's provided both it should use one or
>> the other, but not both.
> Do we still need the chosen node to be passed via DT for command line,
> even if the kernel uses ACPI?

It depends, but I would prefer not. I prefer UEFI+ACPI and then we don't 
need

the chosen node to be passed via DT.

Thanks
Hanjun

Re: [PATCH 10/20] ARM64 / ACPI: Enumerate possible/present CPU set and map logical cpu id to APIC id

[Hanjun Guo]

On 2014年01月24日 23:35, Lorenzo Pieralisi wrote:
> 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.

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

Thank you for doing this, I will look that too.

>
> [...]
>
>>>> +
>>>> +	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?

I think Mark had a clear answer about this :) (Answer for my self)

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

ah, yes. although my ACPI based CPU hot-plug patch is ready, but need
this patch set goes in first and then send them out.

I agree with you, will try to update this patch.

Thanks
Hanjun

Re: [Linaro-acpi] [PATCH 04/20] ARM64 / ACPI: Introduce arm_core.c and its related head file

[Tomasz Nowicki]

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

Re: [PATCH 09/20] ARM64 / ACPI: Implement core functions for parsing MADT table

[Arnd Bergmann]

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

Re: [PATCH 17/20] clocksource / arch_timer: Use ACPI GTDT table to initialize arch timer

[Mark Rutland]

On Fri, Jan 17, 2014 at 12:25:11PM +0000, Hanjun Guo wrote:
> ACPI GTDT (Generic Timer Description Table) contains information for
> arch timer initialization, this patch use this table to probe arm timer.
> 
> GTDT table is used for ARM/ARM64 only, please refer to chapter 5.2.24
> of ACPI 5.0 spec for detailed inforamtion
> 
> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> ---
>  drivers/clocksource/arm_arch_timer.c |  100 +++++++++++++++++++++++++++++-----
>  1 file changed, 85 insertions(+), 15 deletions(-)

[...]

> +static void __init register_arch_interrupt(u32 interrupt, u32 flags,
> +					int *arch_timer_ppi)
> +{
> +	int trigger, polarity;
> +
> +	if (!interrupt || !arch_timer_ppi)
> +		return;
> +
> +	trigger = (flags & ACPI_GTDT_INTERRUPT_MODE) ? ACPI_EDGE_SENSITIVE
> +			: ACPI_LEVEL_SENSITIVE;
> +
> +	polarity = (flags & ACPI_GTDT_INTERRUPT_POLARITY) ? ACPI_ACTIVE_LOW
> +			: ACPI_ACTIVE_HIGH;
> +
> +	*arch_timer_ppi = acpi_register_gsi(NULL, interrupt, trigger,
> +						polarity);
> +}

Why does this take a pointer to the irq rather than returning the irq
(as with irq_of_parse_and_map)?

This looks awfully generic. Are the timer interrupts encoded specially
or is this useful for parsing other interrupts?

> +
> +static int __init acpi_parse_gtdt(struct acpi_table_header *table)
> +{
> +	struct acpi_table_gtdt *gtdt;
> +
> +	gtdt = (struct acpi_table_gtdt *)table;
> +	if (!gtdt)
> +		return -EINVAL;
> +
> +	arch_timer_rate = arch_timer_get_cntfrq();
> +
> +	if (!arch_timer_rate) {
> +		pr_warn("arch_timer: Could not get frequency from CNTFREG\n");

s/CNTFREG/CNTFREQ/

This is probably worth a pr_err at least, the system is unlikely to get
very far if the timers don't work.

Thanks,
Mark.