[PATCH v5 0/5] Prepare for running ACPI on !x86 and !ia64

[PATCH v5 0/5] Prepare for running ACPI on !x86 and !ia64

[Hanjun Guo]

Some of the ACPI code is arch-dependent and make the code can't be
compiled on !x86 or !ia64, the first two patches just do some rework
on the idle_boot_override and _PDC related stuff to make the ACPI
code more arch-independent.

The third patch just introduce map_gic_id() for ACPI processor core
followed the ACPI 5.0 spec. Last two patches are just some cleanups.

I have compiled the kernel successfully after appling this patch set
on x86 and ia64.

Changs for v5:
Realign the additional lines to the open parenthesis
pr_info(xxxx);
	xxxx;

Changes for v4:
a) remove preprocessor directives in function bodies;
b) add another two patches for cleanups.

Changes for v3:
Fix the issues pointed out by Lan Tianyu and add the Reviewed-by for
first patch.

Changes for v2:
a) add #if defined(CONFIG_X86) || defined(CONFIG_IA64) for
   idle_boot_override related code;
b) Rebased on 3.14-rc1.

Changes since the RFC version:
a) Remove the RFC tag;
b) Move idle_boot_override out of the arch directory suggested
   by Alan;
c) Make these 3 patches as a separate patch set since there are
   not not related to the ARM/ARM64 platform.

Hanjun Guo (5):
  ACPI / idle: Make idle_boot_override depend on x86 and ia64
  ACPI / processor_core: Rework _PDC related stuff to make it more
    arch-independent
  ACPI / processor: Introduce map_gic_id() to get apic id from MADT or
    _MAT method
  ACPI: Move BAD_MADT_ENTRY() to linux/acpi.h
  ACPI: Replace printk with pr_* in tables.c

 arch/ia64/include/asm/acpi.h  |    5 +-
 arch/ia64/kernel/acpi.c       |   18 ++++--
 arch/x86/include/asm/acpi.h   |   19 +------
 arch/x86/kernel/acpi/boot.c   |    4 --
 arch/x86/kernel/acpi/cstate.c |   29 ++++++++++
 drivers/acpi/processor_core.c |   92 ++++++++++++++++++------------
 drivers/acpi/tables.c         |  124 ++++++++++++++++++-----------------------
 include/linux/acpi.h          |    4 ++
 8 files changed, 158 insertions(+), 137 deletions(-)

-- 
1.7.9.5

[PATCH v5 1/5] ACPI / idle: Make idle_boot_override depend on x86 and ia64

[Hanjun Guo]

idle_boot_override depends on x86 and ia64 now, and we can not
foresee it will be used on ARM or ARM64, so move the code into
CONFIG_X86 and CONFIG_IA64 #ifdefs to make processor_core.c
can be compiled on ARM64.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 drivers/acpi/processor_core.c |   47 ++++++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 19 deletions(-)

diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
index 7c50a4c..4d91b32 100644
--- a/drivers/acpi/processor_core.c
+++ b/drivers/acpi/processor_core.c
@@ -19,24 +19,6 @@
 #define _COMPONENT		ACPI_PROCESSOR_COMPONENT
 ACPI_MODULE_NAME("processor_core");
 
-static int __init set_no_mwait(const struct dmi_system_id *id)
-{
-	printk(KERN_NOTICE PREFIX "%s detected - "
-		"disabling mwait for CPU C-states\n", id->ident);
-	boot_option_idle_override = IDLE_NOMWAIT;
-	return 0;
-}
-
-static struct dmi_system_id processor_idle_dmi_table[] __initdata = {
-	{
-	set_no_mwait, "Extensa 5220", {
-	DMI_MATCH(DMI_BIOS_VENDOR, "Phoenix Technologies LTD"),
-	DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
-	DMI_MATCH(DMI_PRODUCT_VERSION, "0100"),
-	DMI_MATCH(DMI_BOARD_NAME, "Columbia") }, NULL},
-	{},
-};
-
 static int map_lapic_id(struct acpi_subtable_header *entry,
 		 u32 acpi_id, int *apic_id)
 {
@@ -379,13 +361,40 @@ early_init_pdc(acpi_handle handle, u32 lvl, void *context, void **rv)
 	return AE_OK;
 }
 
-void __init acpi_early_processor_set_pdc(void)
+#if defined(CONFIG_X86) || defined(CONFIG_IA64)
+static int __init set_no_mwait(const struct dmi_system_id *id)
+{
+	pr_notice(PREFIX "%s detected - disabling mwait for CPU C-states\n",
+		  id->ident);
+	boot_option_idle_override = IDLE_NOMWAIT;
+	return 0;
+}
+
+static struct dmi_system_id processor_idle_dmi_table[] __initdata = {
+	{
+	set_no_mwait, "Extensa 5220", {
+	DMI_MATCH(DMI_BIOS_VENDOR, "Phoenix Technologies LTD"),
+	DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
+	DMI_MATCH(DMI_PRODUCT_VERSION, "0100"),
+	DMI_MATCH(DMI_BOARD_NAME, "Columbia") }, NULL},
+	{},
+};
+
+static void __init processor_dmi_check(void)
 {
 	/*
 	 * Check whether the system is DMI table. If yes, OSPM
 	 * should not use mwait for CPU-states.
 	 */
 	dmi_check_system(processor_idle_dmi_table);
+}
+#else
+static inline void processor_dmi_check(void) {}
+#endif
+
+void __init acpi_early_processor_set_pdc(void)
+{
+	processor_dmi_check();
 
 	acpi_walk_namespace(ACPI_TYPE_PROCESSOR, ACPI_ROOT_OBJECT,
 			    ACPI_UINT32_MAX,
-- 
1.7.9.5

[PATCH v5 2/5] ACPI / processor_core: Rework _PDC related stuff to make it more arch-independent

[Hanjun Guo]

_PDC related stuff in processor_core.c is little bit X86/IA64 dependent,
rework the code to make it more arch-independent, no functional change
in this patch.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
---
 arch/ia64/include/asm/acpi.h  |    5 +----
 arch/ia64/kernel/acpi.c       |   14 ++++++++++++++
 arch/x86/include/asm/acpi.h   |   19 +------------------
 arch/x86/kernel/acpi/cstate.c |   29 +++++++++++++++++++++++++++++
 drivers/acpi/processor_core.c |   19 +------------------
 5 files changed, 46 insertions(+), 40 deletions(-)

diff --git a/arch/ia64/include/asm/acpi.h b/arch/ia64/include/asm/acpi.h
index d651102..d2b8b9d 100644
--- a/arch/ia64/include/asm/acpi.h
+++ b/arch/ia64/include/asm/acpi.h
@@ -152,10 +152,7 @@ extern int __initdata nid_to_pxm_map[MAX_NUMNODES];
 #endif
 
 static inline bool arch_has_acpi_pdc(void) { return true; }
-static inline void arch_acpi_set_pdc_bits(u32 *buf)
-{
-	buf[2] |= ACPI_PDC_EST_CAPABILITY_SMP;
-}
+extern void arch_acpi_set_pdc_bits(u32 *buf);
 
 #define acpi_unlazy_tlb(x)
 
diff --git a/arch/ia64/kernel/acpi.c b/arch/ia64/kernel/acpi.c
index 07d209c..af9d9e4 100644
--- a/arch/ia64/kernel/acpi.c
+++ b/arch/ia64/kernel/acpi.c
@@ -1014,3 +1014,17 @@ EXPORT_SYMBOL(acpi_unregister_ioapic);
  * TBD when when IA64 starts to support suspend...
  */
 int acpi_suspend_lowlevel(void) { return 0; }
+
+void arch_acpi_set_pdc_bits(u32 *buf)
+{
+	/* Enable coordination with firmware's _TSD info */
+	buf[2] |= ACPI_PDC_SMP_T_SWCOORD | ACPI_PDC_EST_CAPABILITY_SMP;
+	if (boot_option_idle_override == IDLE_NOMWAIT) {
+		/*
+		 * If mwait is disabled for CPU C-states, the C2C3_FFH access
+		 * mode will be disabled in the parameter of _PDC object.
+		 * Of course C1_FFH access mode will also be disabled.
+		 */
+		buf[2] &= ~(ACPI_PDC_C_C2C3_FFH | ACPI_PDC_C_C1_FFH);
+	}
+}
diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
index c8c1e70..e9f71bc 100644
--- a/arch/x86/include/asm/acpi.h
+++ b/arch/x86/include/asm/acpi.h
@@ -147,24 +147,7 @@ static inline bool arch_has_acpi_pdc(void)
 		c->x86_vendor == X86_VENDOR_CENTAUR);
 }
 
-static inline void arch_acpi_set_pdc_bits(u32 *buf)
-{
-	struct cpuinfo_x86 *c = &cpu_data(0);
-
-	buf[2] |= ACPI_PDC_C_CAPABILITY_SMP;
-
-	if (cpu_has(c, X86_FEATURE_EST))
-		buf[2] |= ACPI_PDC_EST_CAPABILITY_SWSMP;
-
-	if (cpu_has(c, X86_FEATURE_ACPI))
-		buf[2] |= ACPI_PDC_T_FFH;
-
-	/*
-	 * If mwait/monitor is unsupported, C2/C3_FFH will be disabled
-	 */
-	if (!cpu_has(c, X86_FEATURE_MWAIT))
-		buf[2] &= ~(ACPI_PDC_C_C2C3_FFH);
-}
+extern void arch_acpi_set_pdc_bits(u32 *buf);
 
 #else /* !CONFIG_ACPI */
 
diff --git a/arch/x86/kernel/acpi/cstate.c b/arch/x86/kernel/acpi/cstate.c
index e69182f..a36638f 100644
--- a/arch/x86/kernel/acpi/cstate.c
+++ b/arch/x86/kernel/acpi/cstate.c
@@ -16,6 +16,35 @@
 #include <asm/mwait.h>
 #include <asm/special_insns.h>
 
+void arch_acpi_set_pdc_bits(u32 *buf)
+{
+	struct cpuinfo_x86 *c = &cpu_data(0);
+
+	/* Enable coordination with firmware's _TSD info */
+	buf[2] |= ACPI_PDC_SMP_T_SWCOORD | ACPI_PDC_C_CAPABILITY_SMP;
+
+	if (cpu_has(c, X86_FEATURE_EST))
+		buf[2] |= ACPI_PDC_EST_CAPABILITY_SWSMP;
+
+	if (cpu_has(c, X86_FEATURE_ACPI))
+		buf[2] |= ACPI_PDC_T_FFH;
+
+	/*
+	 * If mwait/monitor is unsupported, C2/C3_FFH will be disabled
+	 */
+	if (!cpu_has(c, X86_FEATURE_MWAIT))
+		buf[2] &= ~(ACPI_PDC_C_C2C3_FFH);
+
+	if (boot_option_idle_override == IDLE_NOMWAIT) {
+		/*
+		 * If mwait is disabled for CPU C-states, the C2C3_FFH access
+		 * mode will be disabled in the parameter of _PDC object.
+		 * Of course C1_FFH access mode will also be disabled.
+		 */
+		buf[2] &= ~(ACPI_PDC_C_C2C3_FFH | ACPI_PDC_C_C1_FFH);
+	}
+}
+
 /*
  * Initialize bm_flags based on the CPU cache properties
  * On SMP it depends on cache configuration
diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
index 4d91b32..4dcf776 100644
--- a/drivers/acpi/processor_core.c
+++ b/drivers/acpi/processor_core.c
@@ -255,9 +255,6 @@ static void acpi_set_pdc_bits(u32 *buf)
 	buf[0] = ACPI_PDC_REVISION_ID;
 	buf[1] = 1;
 
-	/* Enable coordination with firmware's _TSD info */
-	buf[2] = ACPI_PDC_SMP_T_SWCOORD;
-
 	/* Twiddle arch-specific bits needed for _PDC */
 	arch_acpi_set_pdc_bits(buf);
 }
@@ -282,7 +279,7 @@ static struct acpi_object_list *acpi_processor_alloc_pdc(void)
 		return NULL;
 	}
 
-	buf = kmalloc(12, GFP_KERNEL);
+	buf = kzalloc(12, GFP_KERNEL);
 	if (!buf) {
 		printk(KERN_ERR "Memory allocation error\n");
 		kfree(obj);
@@ -310,20 +307,6 @@ acpi_processor_eval_pdc(acpi_handle handle, struct acpi_object_list *pdc_in)
 {
 	acpi_status status = AE_OK;
 
-	if (boot_option_idle_override == IDLE_NOMWAIT) {
-		/*
-		 * If mwait is disabled for CPU C-states, the C2C3_FFH access
-		 * mode will be disabled in the parameter of _PDC object.
-		 * Of course C1_FFH access mode will also be disabled.
-		 */
-		union acpi_object *obj;
-		u32 *buffer = NULL;
-
-		obj = pdc_in->pointer;
-		buffer = (u32 *)(obj->buffer.pointer);
-		buffer[2] &= ~(ACPI_PDC_C_C2C3_FFH | ACPI_PDC_C_C1_FFH);
-
-	}
 	status = acpi_evaluate_object(handle, "_PDC", pdc_in, NULL);
 
 	if (ACPI_FAILURE(status))
-- 
1.7.9.5

[PATCH v5 3/5] ACPI / processor: Introduce map_gic_id() to get apic id from MADT or _MAT method

[Hanjun Guo]

Get apic id from MADT or _MAT method is not implemented on arm/arm64,
and ACPI 5.0 introduces GIC Structure for it, so this patch introduces
map_gic_id() to get apic id followed the ACPI 5.0 spec.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 drivers/acpi/processor_core.c |   26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
index 4dcf776..d316d9b 100644
--- a/drivers/acpi/processor_core.c
+++ b/drivers/acpi/processor_core.c
@@ -71,6 +71,27 @@ static int map_lsapic_id(struct acpi_subtable_header *entry,
 	return 0;
 }
 
+static int map_gic_id(struct acpi_subtable_header *entry,
+		int device_declaration, u32 acpi_id, int *apic_id)
+{
+	struct acpi_madt_generic_interrupt *gic =
+		(struct acpi_madt_generic_interrupt *)entry;
+
+	if (!(gic->flags & ACPI_MADT_ENABLED))
+		return -ENODEV;
+
+	/* In the GIC interrupt model, logical processors are
+	 * required to have a Processor Device object in the DSDT,
+	 * so we should check device_declaration here
+	 */
+	if (device_declaration && (gic->uid == acpi_id)) {
+		*apic_id = gic->gic_id;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
 static int map_madt_entry(int type, u32 acpi_id)
 {
 	unsigned long madt_end, entry;
@@ -106,6 +127,9 @@ static int map_madt_entry(int type, u32 acpi_id)
 		} else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC) {
 			if (!map_lsapic_id(header, type, acpi_id, &apic_id))
 				break;
+		} else if (header->type == ACPI_MADT_TYPE_GENERIC_INTERRUPT) {
+			if (!map_gic_id(header, type, acpi_id, &apic_id))
+				break;
 		}
 		entry += header->length;
 	}
@@ -136,6 +160,8 @@ static int map_mat_entry(acpi_handle handle, int type, u32 acpi_id)
 		map_lapic_id(header, acpi_id, &apic_id);
 	} else if (header->type == ACPI_MADT_TYPE_LOCAL_SAPIC) {
 		map_lsapic_id(header, type, acpi_id, &apic_id);
+	} else if (header->type == ACPI_MADT_TYPE_GENERIC_INTERRUPT) {
+		map_gic_id(header, type, acpi_id, &apic_id);
 	}
 
 exit:
-- 
1.7.9.5

[PATCH v5 4/5] ACPI: Move BAD_MADT_ENTRY() to linux/acpi.h

[Hanjun Guo]

BAD_MADT_ENTRY() is arch independent and will be used for all
archs which parsing MADT table, so move it to linux/acpi.h to
simply the code.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 arch/ia64/kernel/acpi.c     |    4 ----
 arch/x86/kernel/acpi/boot.c |    4 ----
 include/linux/acpi.h        |    4 ++++
 3 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/arch/ia64/kernel/acpi.c b/arch/ia64/kernel/acpi.c
index af9d9e4..f7bd074 100644
--- a/arch/ia64/kernel/acpi.c
+++ b/arch/ia64/kernel/acpi.c
@@ -54,10 +54,6 @@
 #include <asm/sal.h>
 #include <asm/cyclone.h>
 
-#define BAD_MADT_ENTRY(entry, end) (                                        \
-		(!entry) || (unsigned long)entry + sizeof(*entry) > end ||  \
-		((struct acpi_subtable_header *)entry)->length < sizeof(*entry))
-
 #define PREFIX			"ACPI: "
 
 unsigned int acpi_cpei_override;
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 1dac942..123f9e3 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -53,10 +53,6 @@ EXPORT_SYMBOL(acpi_disabled);
 # include <asm/proto.h>
 #endif				/* X86 */
 
-#define BAD_MADT_ENTRY(entry, end) (					    \
-		(!entry) || (unsigned long)entry + sizeof(*entry) > end ||  \
-		((struct acpi_subtable_header *)entry)->length < sizeof(*entry))
-
 #define PREFIX			"ACPI: "
 
 int acpi_noirq;				/* skip ACPI IRQ initialization */
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 1151a1d..6a15ddd 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -108,6 +108,10 @@ static inline void acpi_initrd_override(void *data, size_t size)
 }
 #endif
 
+#define BAD_MADT_ENTRY(entry, end) (					    \
+		(!entry) || (unsigned long)entry + sizeof(*entry) > end ||  \
+		((struct acpi_subtable_header *)entry)->length < sizeof(*entry))
+
 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);
-- 
1.7.9.5

[PATCH v5 5/5] ACPI: Replace printk with pr_* in tables.c

[Hanjun Guo]

This patch just do some clean up to replace printk with pr_*,
no functional change.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 drivers/acpi/tables.c |  124 +++++++++++++++++++++----------------------------
 1 file changed, 54 insertions(+), 70 deletions(-)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 5837f85..fd95a76 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -55,10 +55,9 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
 		{
 			struct acpi_madt_local_apic *p =
 			    (struct acpi_madt_local_apic *)header;
-			printk(KERN_INFO PREFIX
-			       "LAPIC (acpi_id[0x%02x] lapic_id[0x%02x] %s)\n",
-			       p->processor_id, p->id,
-			       (p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
+			pr_info(PREFIX "LAPIC (acpi_id[0x%02x] lapic_id[0x%02x] %s)\n",
+				p->processor_id, p->id,
+				(p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
 		}
 		break;
 
@@ -66,11 +65,9 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
 		{
 			struct acpi_madt_local_x2apic *p =
 			    (struct acpi_madt_local_x2apic *)header;
-			printk(KERN_INFO PREFIX
-			       "X2APIC (apic_id[0x%02x] uid[0x%02x] %s)\n",
-			       p->local_apic_id, p->uid,
-			       (p->lapic_flags & ACPI_MADT_ENABLED) ?
-			       "enabled" : "disabled");
+			pr_info(PREFIX "X2APIC (apic_id[0x%02x] uid[0x%02x] %s)\n",
+				p->local_apic_id, p->uid,
+				(p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
 		}
 		break;
 
@@ -78,9 +75,8 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
 		{
 			struct acpi_madt_io_apic *p =
 			    (struct acpi_madt_io_apic *)header;
-			printk(KERN_INFO PREFIX
-			       "IOAPIC (id[0x%02x] address[0x%08x] gsi_base[%d])\n",
-			       p->id, p->address, p->global_irq_base);
+			pr_info(PREFIX "IOAPIC (id[0x%02x] address[0x%08x] gsi_base[%d])\n",
+				p->id, p->address, p->global_irq_base);
 		}
 		break;
 
@@ -88,18 +84,15 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
 		{
 			struct acpi_madt_interrupt_override *p =
 			    (struct acpi_madt_interrupt_override *)header;
-			printk(KERN_INFO PREFIX
-			       "INT_SRC_OVR (bus %d bus_irq %d global_irq %d %s %s)\n",
-			       p->bus, p->source_irq, p->global_irq,
-			       mps_inti_flags_polarity[p->inti_flags & ACPI_MADT_POLARITY_MASK],
-			       mps_inti_flags_trigger[(p->inti_flags & ACPI_MADT_TRIGGER_MASK) >> 2]);
+			pr_info(PREFIX "INT_SRC_OVR (bus %d bus_irq %d global_irq %d %s %s)\n",
+				p->bus, p->source_irq, p->global_irq,
+				mps_inti_flags_polarity[p->inti_flags & ACPI_MADT_POLARITY_MASK],
+				mps_inti_flags_trigger[(p->inti_flags & ACPI_MADT_TRIGGER_MASK) >> 2]);
 			if (p->inti_flags  &
 			    ~(ACPI_MADT_POLARITY_MASK | ACPI_MADT_TRIGGER_MASK))
-				printk(KERN_INFO PREFIX
-				       "INT_SRC_OVR unexpected reserved flags: 0x%x\n",
-				       p->inti_flags  &
+				pr_info(PREFIX "INT_SRC_OVR unexpected reserved flags: 0x%x\n",
+					p->inti_flags  &
 					~(ACPI_MADT_POLARITY_MASK | ACPI_MADT_TRIGGER_MASK));
-
 		}
 		break;
 
@@ -107,11 +100,10 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
 		{
 			struct acpi_madt_nmi_source *p =
 			    (struct acpi_madt_nmi_source *)header;
-			printk(KERN_INFO PREFIX
-			       "NMI_SRC (%s %s global_irq %d)\n",
-			       mps_inti_flags_polarity[p->inti_flags & ACPI_MADT_POLARITY_MASK],
-			       mps_inti_flags_trigger[(p->inti_flags & ACPI_MADT_TRIGGER_MASK) >> 2],
-			       p->global_irq);
+			pr_info(PREFIX "NMI_SRC (%s %s global_irq %d)\n",
+				mps_inti_flags_polarity[p->inti_flags & ACPI_MADT_POLARITY_MASK],
+				mps_inti_flags_trigger[(p->inti_flags & ACPI_MADT_TRIGGER_MASK) >> 2],
+				p->global_irq);
 		}
 		break;
 
@@ -119,12 +111,11 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
 		{
 			struct acpi_madt_local_apic_nmi *p =
 			    (struct acpi_madt_local_apic_nmi *)header;
-			printk(KERN_INFO PREFIX
-			       "LAPIC_NMI (acpi_id[0x%02x] %s %s lint[0x%x])\n",
-			       p->processor_id,
-			       mps_inti_flags_polarity[p->inti_flags & ACPI_MADT_POLARITY_MASK	],
-			       mps_inti_flags_trigger[(p->inti_flags & ACPI_MADT_TRIGGER_MASK) >> 2],
-			       p->lint);
+			pr_info(PREFIX "LAPIC_NMI (acpi_id[0x%02x] %s %s lint[0x%x])\n",
+				p->processor_id,
+				mps_inti_flags_polarity[p->inti_flags & ACPI_MADT_POLARITY_MASK	],
+				mps_inti_flags_trigger[(p->inti_flags & ACPI_MADT_TRIGGER_MASK) >> 2],
+				p->lint);
 		}
 		break;
 
@@ -137,12 +128,11 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
 			polarity = p->inti_flags & ACPI_MADT_POLARITY_MASK;
 			trigger = (p->inti_flags & ACPI_MADT_TRIGGER_MASK) >> 2;
 
-			printk(KERN_INFO PREFIX
-			       "X2APIC_NMI (uid[0x%02x] %s %s lint[0x%x])\n",
-			       p->uid,
-			       mps_inti_flags_polarity[polarity],
-			       mps_inti_flags_trigger[trigger],
-			       p->lint);
+			pr_info(PREFIX "X2APIC_NMI (uid[0x%02x] %s %s lint[0x%x])\n",
+				p->uid,
+				mps_inti_flags_polarity[polarity],
+				mps_inti_flags_trigger[trigger],
+				p->lint);
 		}
 		break;
 
@@ -150,9 +140,8 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
 		{
 			struct acpi_madt_local_apic_override *p =
 			    (struct acpi_madt_local_apic_override *)header;
-			printk(KERN_INFO PREFIX
-			       "LAPIC_ADDR_OVR (address[%p])\n",
-			       (void *)(unsigned long)p->address);
+			pr_info(PREFIX "LAPIC_ADDR_OVR (address[%p])\n",
+				(void *)(unsigned long)p->address);
 		}
 		break;
 
@@ -160,10 +149,9 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
 		{
 			struct acpi_madt_io_sapic *p =
 			    (struct acpi_madt_io_sapic *)header;
-			printk(KERN_INFO PREFIX
-			       "IOSAPIC (id[0x%x] address[%p] gsi_base[%d])\n",
-			       p->id, (void *)(unsigned long)p->address,
-			       p->global_irq_base);
+			pr_info(PREFIX "IOSAPIC (id[0x%x] address[%p] gsi_base[%d])\n",
+				p->id, (void *)(unsigned long)p->address,
+				p->global_irq_base);
 		}
 		break;
 
@@ -171,10 +159,10 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
 		{
 			struct acpi_madt_local_sapic *p =
 			    (struct acpi_madt_local_sapic *)header;
-			printk(KERN_INFO PREFIX
-			       "LSAPIC (acpi_id[0x%02x] lsapic_id[0x%02x] lsapic_eid[0x%02x] %s)\n",
-			       p->processor_id, p->id, p->eid,
-			       (p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
+			pr_info(PREFIX
+				"LSAPIC (acpi_id[0x%02x] lsapic_id[0x%02x] lsapic_eid[0x%02x] %s)\n",
+				p->processor_id, p->id, p->eid,
+				(p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
 		}
 		break;
 
@@ -182,19 +170,18 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
 		{
 			struct acpi_madt_interrupt_source *p =
 			    (struct acpi_madt_interrupt_source *)header;
-			printk(KERN_INFO PREFIX
-			       "PLAT_INT_SRC (%s %s type[0x%x] id[0x%04x] eid[0x%x] iosapic_vector[0x%x] global_irq[0x%x]\n",
-			       mps_inti_flags_polarity[p->inti_flags & ACPI_MADT_POLARITY_MASK],
-			       mps_inti_flags_trigger[(p->inti_flags & ACPI_MADT_TRIGGER_MASK) >> 2],
-			       p->type, p->id, p->eid, p->io_sapic_vector,
-			       p->global_irq);
+			pr_info(PREFIX
+				"PLAT_INT_SRC (%s %s type[0x%x] id[0x%04x] eid[0x%x] iosapic_vector[0x%x] global_irq[0x%x]\n",
+				mps_inti_flags_polarity[p->inti_flags & ACPI_MADT_POLARITY_MASK],
+				mps_inti_flags_trigger[(p->inti_flags & ACPI_MADT_TRIGGER_MASK) >> 2],
+				p->type, p->id, p->eid, p->io_sapic_vector,
+				p->global_irq);
 		}
 		break;
 
 	default:
-		printk(KERN_WARNING PREFIX
-		       "Found unsupported MADT entry (type = 0x%x)\n",
-		       header->type);
+		pr_warn(PREFIX "Found unsupported MADT entry (type = 0x%x)\n",
+			header->type);
 		break;
 	}
 }
@@ -225,7 +212,7 @@ acpi_table_parse_entries(char *id,
 		acpi_get_table_with_size(id, 0, &table_header, &tbl_size);
 
 	if (!table_header) {
-		printk(KERN_WARNING PREFIX "%4.4s not present\n", id);
+		pr_warn(PREFIX "%4.4s not present\n", id);
 		return -ENODEV;
 	}
 
@@ -256,8 +243,8 @@ acpi_table_parse_entries(char *id,
 		    ((unsigned long)entry + entry->length);
 	}
 	if (max_entries && count > max_entries) {
-		printk(KERN_WARNING PREFIX "[%4.4s:0x%02x] ignored %i entries of "
-		       "%i found\n", id, entry_id, count - max_entries, count);
+		pr_warn(PREFIX "[%4.4s:0x%02x] ignored %i entries of %i found\n",
+			id, entry_id, count - max_entries, count);
 	}
 
 	early_acpi_os_unmap_memory((char *)table_header, tbl_size);
@@ -322,13 +309,11 @@ static void __init check_multiple_madt(void)
 
 	acpi_get_table_with_size(ACPI_SIG_MADT, 2, &table, &tbl_size);
 	if (table) {
-		printk(KERN_WARNING PREFIX
-		       "BIOS bug: multiple APIC/MADT found,"
-		       " using %d\n", acpi_apic_instance);
-		printk(KERN_WARNING PREFIX
-		       "If \"acpi_apic_instance=%d\" works better, "
-		       "notify linux-acpi at vger.kernel.org\n",
-		       acpi_apic_instance ? 0 : 2);
+		pr_warn(PREFIX "BIOS bug: multiple APIC/MADT found, using %d\n",
+			acpi_apic_instance);
+		pr_warn(PREFIX "If \"acpi_apic_instance=%d\" works better, "
+			"notify linux-acpi at vger.kernel.org\n",
+			acpi_apic_instance ? 0 : 2);
 		early_acpi_os_unmap_memory(table, tbl_size);
 
 	} else
@@ -365,8 +350,7 @@ static int __init acpi_parse_apic_instance(char *str)
 
 	acpi_apic_instance = simple_strtoul(str, NULL, 0);
 
-	printk(KERN_NOTICE PREFIX "Shall use APIC/MADT table %d\n",
-	       acpi_apic_instance);
+	pr_notice(PREFIX "Shall use APIC/MADT table %d\n", acpi_apic_instance);
 
 	return 0;
 }
-- 
1.7.9.5

[PATCH v5 2/5] ACPI / processor_core: Rework _PDC related stuff to make it more arch-independent

[Rafael J. Wysocki]

On Wednesday, February 19, 2014 12:23:55 AM Hanjun Guo wrote:
> _PDC related stuff in processor_core.c is little bit X86/IA64 dependent,
> rework the code to make it more arch-independent, no functional change
> in this patch.
> 
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>

I've queued up patches [1,3-5/5] from this series for 3.15 (modulo changelog
modifications), but this one should be CCed to the x86 and ia64 maintainers.

Thanks!

> ---
>  arch/ia64/include/asm/acpi.h  |    5 +----
>  arch/ia64/kernel/acpi.c       |   14 ++++++++++++++
>  arch/x86/include/asm/acpi.h   |   19 +------------------
>  arch/x86/kernel/acpi/cstate.c |   29 +++++++++++++++++++++++++++++
>  drivers/acpi/processor_core.c |   19 +------------------
>  5 files changed, 46 insertions(+), 40 deletions(-)
> 
> diff --git a/arch/ia64/include/asm/acpi.h b/arch/ia64/include/asm/acpi.h
> index d651102..d2b8b9d 100644
> --- a/arch/ia64/include/asm/acpi.h
> +++ b/arch/ia64/include/asm/acpi.h
> @@ -152,10 +152,7 @@ extern int __initdata nid_to_pxm_map[MAX_NUMNODES];
>  #endif
>  
>  static inline bool arch_has_acpi_pdc(void) { return true; }
> -static inline void arch_acpi_set_pdc_bits(u32 *buf)
> -{
> -	buf[2] |= ACPI_PDC_EST_CAPABILITY_SMP;
> -}
> +extern void arch_acpi_set_pdc_bits(u32 *buf);
>  
>  #define acpi_unlazy_tlb(x)
>  
> diff --git a/arch/ia64/kernel/acpi.c b/arch/ia64/kernel/acpi.c
> index 07d209c..af9d9e4 100644
> --- a/arch/ia64/kernel/acpi.c
> +++ b/arch/ia64/kernel/acpi.c
> @@ -1014,3 +1014,17 @@ EXPORT_SYMBOL(acpi_unregister_ioapic);
>   * TBD when when IA64 starts to support suspend...
>   */
>  int acpi_suspend_lowlevel(void) { return 0; }
> +
> +void arch_acpi_set_pdc_bits(u32 *buf)
> +{
> +	/* Enable coordination with firmware's _TSD info */
> +	buf[2] |= ACPI_PDC_SMP_T_SWCOORD | ACPI_PDC_EST_CAPABILITY_SMP;
> +	if (boot_option_idle_override == IDLE_NOMWAIT) {
> +		/*
> +		 * If mwait is disabled for CPU C-states, the C2C3_FFH access
> +		 * mode will be disabled in the parameter of _PDC object.
> +		 * Of course C1_FFH access mode will also be disabled.
> +		 */
> +		buf[2] &= ~(ACPI_PDC_C_C2C3_FFH | ACPI_PDC_C_C1_FFH);
> +	}
> +}
> diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
> index c8c1e70..e9f71bc 100644
> --- a/arch/x86/include/asm/acpi.h
> +++ b/arch/x86/include/asm/acpi.h
> @@ -147,24 +147,7 @@ static inline bool arch_has_acpi_pdc(void)
>  		c->x86_vendor == X86_VENDOR_CENTAUR);
>  }
>  
> -static inline void arch_acpi_set_pdc_bits(u32 *buf)
> -{
> -	struct cpuinfo_x86 *c = &cpu_data(0);
> -
> -	buf[2] |= ACPI_PDC_C_CAPABILITY_SMP;
> -
> -	if (cpu_has(c, X86_FEATURE_EST))
> -		buf[2] |= ACPI_PDC_EST_CAPABILITY_SWSMP;
> -
> -	if (cpu_has(c, X86_FEATURE_ACPI))
> -		buf[2] |= ACPI_PDC_T_FFH;
> -
> -	/*
> -	 * If mwait/monitor is unsupported, C2/C3_FFH will be disabled
> -	 */
> -	if (!cpu_has(c, X86_FEATURE_MWAIT))
> -		buf[2] &= ~(ACPI_PDC_C_C2C3_FFH);
> -}
> +extern void arch_acpi_set_pdc_bits(u32 *buf);
>  
>  #else /* !CONFIG_ACPI */
>  
> diff --git a/arch/x86/kernel/acpi/cstate.c b/arch/x86/kernel/acpi/cstate.c
> index e69182f..a36638f 100644
> --- a/arch/x86/kernel/acpi/cstate.c
> +++ b/arch/x86/kernel/acpi/cstate.c
> @@ -16,6 +16,35 @@
>  #include <asm/mwait.h>
>  #include <asm/special_insns.h>
>  
> +void arch_acpi_set_pdc_bits(u32 *buf)
> +{
> +	struct cpuinfo_x86 *c = &cpu_data(0);
> +
> +	/* Enable coordination with firmware's _TSD info */
> +	buf[2] |= ACPI_PDC_SMP_T_SWCOORD | ACPI_PDC_C_CAPABILITY_SMP;
> +
> +	if (cpu_has(c, X86_FEATURE_EST))
> +		buf[2] |= ACPI_PDC_EST_CAPABILITY_SWSMP;
> +
> +	if (cpu_has(c, X86_FEATURE_ACPI))
> +		buf[2] |= ACPI_PDC_T_FFH;
> +
> +	/*
> +	 * If mwait/monitor is unsupported, C2/C3_FFH will be disabled
> +	 */
> +	if (!cpu_has(c, X86_FEATURE_MWAIT))
> +		buf[2] &= ~(ACPI_PDC_C_C2C3_FFH);
> +
> +	if (boot_option_idle_override == IDLE_NOMWAIT) {
> +		/*
> +		 * If mwait is disabled for CPU C-states, the C2C3_FFH access
> +		 * mode will be disabled in the parameter of _PDC object.
> +		 * Of course C1_FFH access mode will also be disabled.
> +		 */
> +		buf[2] &= ~(ACPI_PDC_C_C2C3_FFH | ACPI_PDC_C_C1_FFH);
> +	}
> +}
> +
>  /*
>   * Initialize bm_flags based on the CPU cache properties
>   * On SMP it depends on cache configuration
> diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
> index 4d91b32..4dcf776 100644
> --- a/drivers/acpi/processor_core.c
> +++ b/drivers/acpi/processor_core.c
> @@ -255,9 +255,6 @@ static void acpi_set_pdc_bits(u32 *buf)
>  	buf[0] = ACPI_PDC_REVISION_ID;
>  	buf[1] = 1;
>  
> -	/* Enable coordination with firmware's _TSD info */
> -	buf[2] = ACPI_PDC_SMP_T_SWCOORD;
> -
>  	/* Twiddle arch-specific bits needed for _PDC */
>  	arch_acpi_set_pdc_bits(buf);
>  }
> @@ -282,7 +279,7 @@ static struct acpi_object_list *acpi_processor_alloc_pdc(void)
>  		return NULL;
>  	}
>  
> -	buf = kmalloc(12, GFP_KERNEL);
> +	buf = kzalloc(12, GFP_KERNEL);
>  	if (!buf) {
>  		printk(KERN_ERR "Memory allocation error\n");
>  		kfree(obj);
> @@ -310,20 +307,6 @@ acpi_processor_eval_pdc(acpi_handle handle, struct acpi_object_list *pdc_in)
>  {
>  	acpi_status status = AE_OK;
>  
> -	if (boot_option_idle_override == IDLE_NOMWAIT) {
> -		/*
> -		 * If mwait is disabled for CPU C-states, the C2C3_FFH access
> -		 * mode will be disabled in the parameter of _PDC object.
> -		 * Of course C1_FFH access mode will also be disabled.
> -		 */
> -		union acpi_object *obj;
> -		u32 *buffer = NULL;
> -
> -		obj = pdc_in->pointer;
> -		buffer = (u32 *)(obj->buffer.pointer);
> -		buffer[2] &= ~(ACPI_PDC_C_C2C3_FFH | ACPI_PDC_C_C1_FFH);
> -
> -	}
>  	status = acpi_evaluate_object(handle, "_PDC", pdc_in, NULL);
>  
>  	if (ACPI_FAILURE(status))
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH v5 2/5] ACPI / processor_core: Rework _PDC related stuff to make it more arch-independent

[Hanjun Guo]

On 2014?02?19? 08:50, Rafael J. Wysocki wrote:
> On Wednesday, February 19, 2014 12:23:55 AM Hanjun Guo wrote:
>> _PDC related stuff in processor_core.c is little bit X86/IA64 dependent,
>> rework the code to make it more arch-independent, no functional change
>> in this patch.
>>
>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>> Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
> I've queued up patches [1,3-5/5] from this series for 3.15 (modulo changelog
> modifications), but this one should be CCed to the x86 and ia64 maintainers.

Thanks! I will resend this patch with Tony and Thomas in cc list, so is 
there any chance for this
patch to be merged into 3.15 if I get acked from them?

Best regards
Hanjun

[PATCH v5 3/5] ACPI / processor: Introduce map_gic_id() to get apic id from MADT or _MAT method

[Sudeep Holla]

Hi Hanjun,

On 18/02/14 16:23, Hanjun Guo wrote:
> Get apic id from MADT or _MAT method is not implemented on arm/arm64,
> and ACPI 5.0 introduces GIC Structure for it, so this patch introduces
> map_gic_id() to get apic id followed the ACPI 5.0 spec.
> 
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> ---
>  drivers/acpi/processor_core.c |   26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
> index 4dcf776..d316d9b 100644
> --- a/drivers/acpi/processor_core.c
> +++ b/drivers/acpi/processor_core.c
> @@ -71,6 +71,27 @@ static int map_lsapic_id(struct acpi_subtable_header *entry,
>  	return 0;
>  }
>  
> +static int map_gic_id(struct acpi_subtable_header *entry,
> +		int device_declaration, u32 acpi_id, int *apic_id)
> +{
> +	struct acpi_madt_generic_interrupt *gic =
> +		(struct acpi_madt_generic_interrupt *)entry;
> +
> +	if (!(gic->flags & ACPI_MADT_ENABLED))
> +		return -ENODEV;
> +
> +	/* In the GIC interrupt model, logical processors are
> +	 * required to have a Processor Device object in the DSDT,
> +	 * so we should check device_declaration here
> +	 */
> +	if (device_declaration && (gic->uid == acpi_id)) {
> +		*apic_id = gic->gic_id;

I have mentioned this earlier, it's not clear yet to me how does this work ?
It needs more clarity in the form of comment here at-least as the ACPIv5.0 is
also not so clear or explicit on how to handle this.

Here you are expecting gic->uid = acpi_id which is fine, while acpi_map_cpuid
matches apic_id with cpu_physical_id(which must be MPIDR in ARM{32,64}). The
latter imposes restriction that gic->gic_id has to be MPIDR. Does that mean we
are imposing restriction on GIC ID to be MPIDR ? If so please document it here
and please explain the reason behind that choice.

I would expect _UID to be MPIDR rather than GIC ID but you may have some reasons
for this choice.

Regards,
Sudeep

[PATCH v5 2/5] ACPI / processor_core: Rework _PDC related stuff to make it more arch-independent

[Rafael J. Wysocki]

On Wednesday, February 19, 2014 10:16:16 AM Hanjun Guo wrote:
> On 2014?02?19? 08:50, Rafael J. Wysocki wrote:
> > On Wednesday, February 19, 2014 12:23:55 AM Hanjun Guo wrote:
> >> _PDC related stuff in processor_core.c is little bit X86/IA64 dependent,
> >> rework the code to make it more arch-independent, no functional change
> >> in this patch.
> >>
> >> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> >> Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
> > I've queued up patches [1,3-5/5] from this series for 3.15 (modulo changelog
> > modifications), but this one should be CCed to the x86 and ia64 maintainers.
> 
> Thanks! I will resend this patch with Tony and Thomas in cc list, so is 
> there any chance for this
> patch to be merged into 3.15 if I get acked from them?

If they ack it, then yes, it can.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH v5 3/5] ACPI / processor: Introduce map_gic_id() to get apic id from MADT or _MAT method

[Hanjun Guo]

Hi Sudeep,

Thanks for your comments, please refer to the replies below. :)

On 2014?02?19? 22:33, Sudeep Holla wrote:
> Hi Hanjun,
>
> On 18/02/14 16:23, Hanjun Guo wrote:
>> Get apic id from MADT or _MAT method is not implemented on arm/arm64,
>> and ACPI 5.0 introduces GIC Structure for it, so this patch introduces
>> map_gic_id() to get apic id followed the ACPI 5.0 spec.
>>
>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>> ---
>>   drivers/acpi/processor_core.c |   26 ++++++++++++++++++++++++++
>>   1 file changed, 26 insertions(+)
>>
>> diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
>> index 4dcf776..d316d9b 100644
>> --- a/drivers/acpi/processor_core.c
>> +++ b/drivers/acpi/processor_core.c
>> @@ -71,6 +71,27 @@ static int map_lsapic_id(struct acpi_subtable_header *entry,
>>   	return 0;
>>   }
>>   
>> +static int map_gic_id(struct acpi_subtable_header *entry,
>> +		int device_declaration, u32 acpi_id, int *apic_id)
>> +{
>> +	struct acpi_madt_generic_interrupt *gic =
>> +		(struct acpi_madt_generic_interrupt *)entry;
>> +
>> +	if (!(gic->flags & ACPI_MADT_ENABLED))
>> +		return -ENODEV;
>> +
>> +	/* In the GIC interrupt model, logical processors are
>> +	 * required to have a Processor Device object in the DSDT,
>> +	 * so we should check device_declaration here
>> +	 */
>> +	if (device_declaration && (gic->uid == acpi_id)) {
>> +		*apic_id = gic->gic_id;
> I have mentioned this earlier, it's not clear yet to me how does this work ?
> It needs more clarity in the form of comment here at-least as the ACPIv5.0 is
> also not so clear or explicit on how to handle this.

Yes, I noticed your comments and had a reply for that, after a
long consideration for this, I would withdraw my previous comments
before, please refer to the comments below.

>
> Here you are expecting gic->uid = acpi_id which is fine, while acpi_map_cpuid
> matches apic_id with cpu_physical_id(which must be MPIDR in ARM{32,64}). The
> latter imposes restriction that gic->gic_id has to be MPIDR. Does that mean we
> are imposing restriction on GIC ID to be MPIDR ? If so please document it here
> and please explain the reason behind that choice.

On x86 and IA64, APIC/SAPIC ID is the hardware id of the logical
processor, and UID is just a unique ID to identify the processor in DSDT, it
can be any value, and even can be strings defined in ASL if I remember
that correctly.

The processor driver also follows that guidance now, and GIC structure 
in MADT
actually represents a processor (GICC) in the system, so I would let
gic->gic_id as MPIDR(processor hardware ID) for now to avoid confusions
and keep consistency with current ACPI driver.

UID can be any value and it depends on implementation, so it can be MPIDR
also, it will not conflict with GIC ID and can works fine in acpi processor
driver now.

>
> I would expect _UID to be MPIDR rather than GIC ID but you may have some reasons
> for this choice.

I think they both can be MPIDR for now, is this ok to you?

we can update the code when the new ACPI spec is coming out if there
is a clear explanation for GIC ID and UID in GIC structures, does this make
sense to you?

Thanks
Hanjun

[PATCH v5 3/5] ACPI / processor: Introduce map_gic_id() to get apic id from MADT or _MAT method

[Sudeep Holla]

Hi Hanjun,

(Adding MarcZ for his views on GIC)

On 20/02/14 03:59, Hanjun Guo wrote:
> Hi Sudeep,
> 
> Thanks for your comments, please refer to the replies below. :)
> 
> On 2014?02?19? 22:33, Sudeep Holla wrote:
>> Hi Hanjun,
>>
>> On 18/02/14 16:23, Hanjun Guo wrote:
>>> Get apic id from MADT or _MAT method is not implemented on arm/arm64,
>>> and ACPI 5.0 introduces GIC Structure for it, so this patch introduces
>>> map_gic_id() to get apic id followed the ACPI 5.0 spec.
>>>
>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>> ---
>>>   drivers/acpi/processor_core.c |   26 ++++++++++++++++++++++++++
>>>   1 file changed, 26 insertions(+)
>>>
>>> diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
>>> index 4dcf776..d316d9b 100644
>>> --- a/drivers/acpi/processor_core.c
>>> +++ b/drivers/acpi/processor_core.c
>>> @@ -71,6 +71,27 @@ static int map_lsapic_id(struct acpi_subtable_header *entry,
>>>   	return 0;
>>>   }
>>>   
>>> +static int map_gic_id(struct acpi_subtable_header *entry,
>>> +		int device_declaration, u32 acpi_id, int *apic_id)
>>> +{
>>> +	struct acpi_madt_generic_interrupt *gic =
>>> +		(struct acpi_madt_generic_interrupt *)entry;
>>> +
>>> +	if (!(gic->flags & ACPI_MADT_ENABLED))
>>> +		return -ENODEV;
>>> +
>>> +	/* In the GIC interrupt model, logical processors are
>>> +	 * required to have a Processor Device object in the DSDT,
>>> +	 * so we should check device_declaration here
>>> +	 */
>>> +	if (device_declaration && (gic->uid == acpi_id)) {
>>> +		*apic_id = gic->gic_id;
>> I have mentioned this earlier, it's not clear yet to me how does this work ?
>> It needs more clarity in the form of comment here at-least as the ACPIv5.0 is
>> also not so clear or explicit on how to handle this.
> 
> Yes, I noticed your comments and had a reply for that, after a
> long consideration for this, I would withdraw my previous comments
> before, please refer to the comments below.
> 
>>
>> Here you are expecting gic->uid = acpi_id which is fine, while acpi_map_cpuid
>> matches apic_id with cpu_physical_id(which must be MPIDR in ARM{32,64}). The
>> latter imposes restriction that gic->gic_id has to be MPIDR. Does that mean we
>> are imposing restriction on GIC ID to be MPIDR ? If so please document it here
>> and please explain the reason behind that choice.
> 
> On x86 and IA64, APIC/SAPIC ID is the hardware id of the logical
> processor, and UID is just a unique ID to identify the processor in DSDT, it
> can be any value, and even can be strings defined in ASL if I remember
> that correctly.
> 
OK, but that's not the case on ARM{32,64}. My main concern here is if we don't
make this definitions clear enough, the vendors might produce ACPI tables with
whatever suits them and we may end up supporting them. Since we are starting
with clean slate, we can avoid getting into such situations. I will be to be
more elaborate this time.

The GIC ID is referred as the local GIC?s hardware ID in ACPIv5.0.
IIUC, since GICC is per-cpu entry, it has to GIC CPU interface ID.

Now how does it differ from MPIDR ? e.g. ARM TC2(multi cluster system)
	GIC ID		MPIDR			Comment
	0		0x000			CA15_0
	1		0x001			CA15_1
	2		0x100			CA7_0
	3		0x101			CA7_1
	4		0x102			CA7_2

> The processor driver also follows that guidance now, and GIC structure 
> in MADT
> actually represents a processor (GICC) in the system, so I would let
> gic->gic_id as MPIDR(processor hardware ID) for now to avoid confusions
> and keep consistency with current ACPI driver.
> 

Yes that's my worry "... so I would let gic->gic_id as MPIDR(processor hardware
ID)" as it contradicts the definition.

1. Yes today GIC CPU ID is enumerated in the code and we don't need it, but
   that doesn't mean the GICC contain whatever(of vendor choice). IMO it should
   be CPU interface ID as per the definition.

2. It's better you post this patch as part of ARM64 port for easier
   understanding on how this gets used as its ARM specific anyway.
   That gives better/complete picture at least for review purposes.

> UID can be any value and it depends on implementation, so it can be MPIDR
> also, it will not conflict with GIC ID and can works fine in acpi processor
> driver now.
> 

Agreed UID can be anything, but your initial ARM64 APCI patches were assuming it
to be MPIDR. Since MPIDR is not specified anywhere else in the ACPI tables and
it is required for some boot methods like PSCI and may be others, I am for
mandating _UID in processor object to be MPIDR. If you agree please make it
explicit in the comments.

>>
>> I would expect _UID to be MPIDR rather than GIC ID but you may have some reasons
>> for this choice.
> 
> I think they both can be MPIDR for now, is this ok to you?
> 

Yes for _UID and no for GIC ID, they need to represent the hardware. We can see
how to manage in this in ACPI. Initially I went by the name "cpu_physical_id"
in acpi_map_cpuid but it looks like it's cpuid->apicid mapping on x86. So that
should not be problem.

> we can update the code when the new ACPI spec is coming out if there
> is a clear explanation for GIC ID and UID in GIC structures, does this make
> sense to you?

Yes, that's for next version of ACPI but we don't know that yet. IIUC you are
trying to implement ACPI port for ARM64 based on ACPIv5.0 through these patches
right ? Hence I pushing you to be more explicit through comments as ACPIv5.0 is
not clear enough.

Regards,
Sudeep

[PATCH v5 2/5] ACPI / processor_core: Rework _PDC related stuff to make it more arch-independent

[Catalin Marinas]

Hi Rafael,

On Wed, Feb 19, 2014 at 01:50:22AM +0100, Rafael J. Wysocki wrote:
> On Wednesday, February 19, 2014 12:23:55 AM Hanjun Guo wrote:
> > _PDC related stuff in processor_core.c is little bit X86/IA64 dependent,
> > rework the code to make it more arch-independent, no functional change
> > in this patch.
> > 
> > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> > Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
> 
> I've queued up patches [1,3-5/5] from this series for 3.15 (modulo changelog
> modifications), but this one should be CCed to the x86 and ia64 maintainers.

Thanks for taking these patches. I would however hold onto patch 3/5 as
this is still under discussion. Basically for patches specific to ARM
ACPI I would really like to see more acks before being merged as that's
a new thing for us.

Thanks.

-- 
Catalin

[PATCH v5 2/5] ACPI / processor_core: Rework _PDC related stuff to make it more arch-independent

[Rafael J. Wysocki]

On Friday, February 21, 2014 06:24:24 PM Catalin Marinas wrote:
> Hi Rafael,
> 
> On Wed, Feb 19, 2014 at 01:50:22AM +0100, Rafael J. Wysocki wrote:
> > On Wednesday, February 19, 2014 12:23:55 AM Hanjun Guo wrote:
> > > _PDC related stuff in processor_core.c is little bit X86/IA64 dependent,
> > > rework the code to make it more arch-independent, no functional change
> > > in this patch.
> > > 
> > > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> > > Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
> > 
> > I've queued up patches [1,3-5/5] from this series for 3.15 (modulo changelog
> > modifications), but this one should be CCed to the x86 and ia64 maintainers.
> 
> Thanks for taking these patches. I would however hold onto patch 3/5 as
> this is still under discussion. Basically for patches specific to ARM
> ACPI I would really like to see more acks before being merged as that's
> a new thing for us.

OK, I'll drop [3/5] for now, then.

I'm wondering, though, whose ACKs I should be waiting for before applying those
patches?

Rafael

[PATCH v5 3/5] ACPI / processor: Introduce map_gic_id() to get apic id from MADT or _MAT method

[Hanjun Guo]

On 2014-2-21 20:37, Sudeep Holla wrote:
> Hi Hanjun,
> 
> (Adding MarcZ for his views on GIC)
> 
> On 20/02/14 03:59, Hanjun Guo wrote:
>> Hi Sudeep,
>>
>> Thanks for your comments, please refer to the replies below. :)
>>
>> On 2014?02?19? 22:33, Sudeep Holla wrote:
>>> Hi Hanjun,
>>>
>>> On 18/02/14 16:23, Hanjun Guo wrote:
>>>> Get apic id from MADT or _MAT method is not implemented on arm/arm64,
>>>> and ACPI 5.0 introduces GIC Structure for it, so this patch introduces
>>>> map_gic_id() to get apic id followed the ACPI 5.0 spec.
>>>>
>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>> ---
>>>>   drivers/acpi/processor_core.c |   26 ++++++++++++++++++++++++++
>>>>   1 file changed, 26 insertions(+)
>>>>
>>>> diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
>>>> index 4dcf776..d316d9b 100644
>>>> --- a/drivers/acpi/processor_core.c
>>>> +++ b/drivers/acpi/processor_core.c
>>>> @@ -71,6 +71,27 @@ static int map_lsapic_id(struct acpi_subtable_header *entry,
>>>>   	return 0;
>>>>   }
>>>>   
>>>> +static int map_gic_id(struct acpi_subtable_header *entry,
>>>> +		int device_declaration, u32 acpi_id, int *apic_id)
>>>> +{
>>>> +	struct acpi_madt_generic_interrupt *gic =
>>>> +		(struct acpi_madt_generic_interrupt *)entry;
>>>> +
>>>> +	if (!(gic->flags & ACPI_MADT_ENABLED))
>>>> +		return -ENODEV;
>>>> +
>>>> +	/* In the GIC interrupt model, logical processors are
>>>> +	 * required to have a Processor Device object in the DSDT,
>>>> +	 * so we should check device_declaration here
>>>> +	 */
>>>> +	if (device_declaration && (gic->uid == acpi_id)) {
>>>> +		*apic_id = gic->gic_id;
>>> I have mentioned this earlier, it's not clear yet to me how does this work ?
>>> It needs more clarity in the form of comment here at-least as the ACPIv5.0 is
>>> also not so clear or explicit on how to handle this.
>>
>> Yes, I noticed your comments and had a reply for that, after a
>> long consideration for this, I would withdraw my previous comments
>> before, please refer to the comments below.
>>
>>>
>>> Here you are expecting gic->uid = acpi_id which is fine, while acpi_map_cpuid
>>> matches apic_id with cpu_physical_id(which must be MPIDR in ARM{32,64}). The
>>> latter imposes restriction that gic->gic_id has to be MPIDR. Does that mean we
>>> are imposing restriction on GIC ID to be MPIDR ? If so please document it here
>>> and please explain the reason behind that choice.
>>
>> On x86 and IA64, APIC/SAPIC ID is the hardware id of the logical
>> processor, and UID is just a unique ID to identify the processor in DSDT, it
>> can be any value, and even can be strings defined in ASL if I remember
>> that correctly.
>>
> OK, but that's not the case on ARM{32,64}. My main concern here is if we don't
> make this definitions clear enough, the vendors might produce ACPI tables with
> whatever suits them and we may end up supporting them. Since we are starting
> with clean slate, we can avoid getting into such situations. I will be to be
> more elaborate this time.

I agree.

> 
> The GIC ID is referred as the local GIC?s hardware ID in ACPIv5.0.
> IIUC, since GICC is per-cpu entry, it has to GIC CPU interface ID.
> 
> Now how does it differ from MPIDR ? e.g. ARM TC2(multi cluster system)
> 	GIC ID		MPIDR			Comment
> 	0		0x000			CA15_0
> 	1		0x001			CA15_1
> 	2		0x100			CA7_0
> 	3		0x101			CA7_1
> 	4		0x102			CA7_2

Yes, obvious different. I know GIC ID can matche the bit index of the associated processor
in the distributor's GICD_ITARGETSR register, and it a clear statement in GICv1/GICv2, my
question is that is this consistent in GICv3/v4 too? this will have some impact on the
code implementation.

> 
>> The processor driver also follows that guidance now, and GIC structure 
>> in MADT
>> actually represents a processor (GICC) in the system, so I would let
>> gic->gic_id as MPIDR(processor hardware ID) for now to avoid confusions
>> and keep consistency with current ACPI driver.
>>
> 
> Yes that's my worry "... so I would let gic->gic_id as MPIDR(processor hardware
> ID)" as it contradicts the definition.
> 
> 1. Yes today GIC CPU ID is enumerated in the code and we don't need it, but
>    that doesn't mean the GICC contain whatever(of vendor choice). IMO it should
>    be CPU interface ID as per the definition.
> 
> 2. It's better you post this patch as part of ARM64 port for easier
>    understanding on how this gets used as its ARM specific anyway.
>    That gives better/complete picture at least for review purposes.

It make sense to me, since Rafael had dropped this patch, I will resend
it in ARM64 ACPI core patche set.

> 
>> UID can be any value and it depends on implementation, so it can be MPIDR
>> also, it will not conflict with GIC ID and can works fine in acpi processor
>> driver now.
>>
> 
> Agreed UID can be anything, but your initial ARM64 APCI patches were assuming it
> to be MPIDR. Since MPIDR is not specified anywhere else in the ACPI tables and
> it is required for some boot methods like PSCI and may be others, I am for
> mandating _UID in processor object to be MPIDR. If you agree please make it
> explicit in the comments.
> 
>>>
>>> I would expect _UID to be MPIDR rather than GIC ID but you may have some reasons
>>> for this choice.
>>
>> I think they both can be MPIDR for now, is this ok to you?
>>
> 
> Yes for _UID and no for GIC ID, they need to represent the hardware. We can see
> how to manage in this in ACPI. Initially I went by the name "cpu_physical_id"
> in acpi_map_cpuid but it looks like it's cpuid->apicid mapping on x86. So that
> should not be problem.

Yes, from the ACPI side, it is ok. but we still need the mappings from logical
processor id to MPIDR value for SMP initialization, just as cpu_logical_map()
in ARM64 did, so I will address your comments and figure out a better solution.

> 
>> we can update the code when the new ACPI spec is coming out if there
>> is a clear explanation for GIC ID and UID in GIC structures, does this make
>> sense to you?
> 
> Yes, that's for next version of ACPI but we don't know that yet. IIUC you are
> trying to implement ACPI port for ARM64 based on ACPIv5.0 through these patches
> right ? Hence I pushing you to be more explicit through comments as ACPIv5.0 is
> not clear enough.

I agree. I will resend this patch as part of ARM64 ACPI core patch set (it is still
need some time though), please help me to review the code and let's discuss it at
that time, is that make sense to you?

Thanks
Hanjun

[PATCH v5 2/5] ACPI / processor_core: Rework _PDC related stuff to make it more arch-independent

[Catalin Marinas]

On 21 Feb 2014, at 23:35, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Friday, February 21, 2014 06:24:24 PM Catalin Marinas wrote:
>> On Wed, Feb 19, 2014 at 01:50:22AM +0100, Rafael J. Wysocki wrote:
>>> On Wednesday, February 19, 2014 12:23:55 AM Hanjun Guo wrote:
>>>> _PDC related stuff in processor_core.c is little bit X86/IA64 dependent,
>>>> rework the code to make it more arch-independent, no functional change
>>>> in this patch.
>>>> 
>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>> Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
>>> 
>>> I've queued up patches [1,3-5/5] from this series for 3.15 (modulo changelog
>>> modifications), but this one should be CCed to the x86 and ia64 maintainers.
>> 
>> Thanks for taking these patches. I would however hold onto patch 3/5 as
>> this is still under discussion. Basically for patches specific to ARM
>> ACPI I would really like to see more acks before being merged as that's
>> a new thing for us.
> 
> OK, I'll drop [3/5] for now, then.

Thanks (it?s only temporary ;)).

> I'm wondering, though, whose ACKs I should be waiting for before applying those
> patches?

Good question ;). In this particular case, there is an ongoing
discussion between Hanjun and Sudeep. While there isn?t anything
major, I would like to see some agreement and potentially an Ack from
the other party involved in the discussion (Sudeep).

There are other patches that are not specific to ARM, so it?s
really your decision. As for the general ARM(64) ACPI case, I don?t
think we have anyone in charge with deciding what?s correct or not
(BTW, who are the people active both in the _ARM_ Linux kernel community
and the ACPI standardisation forum?).

In the mean-time, I would like to see at least an ack from the arm-soc
team (Arnd and Olof) or them collecting those ARM-specific patches and
sending pull request to you. My biggest worry is how the ACPI standard
is interpreted by vendors and how (non-standard) features get into the 
Linux kernel.

Anyway, I?ll meet the Linaro guys in a week time and we?ll agree on
a way forward.

Thanks,

Catalin

[PATCH v5 3/5] ACPI / processor: Introduce map_gic_id() to get apic id from MADT or _MAT method

[Marc Zyngier]

On 2014-02-22 10:21, Hanjun Guo wrote:
> On 2014-2-21 20:37, Sudeep Holla wrote:
>> Hi Hanjun,
>>
>> (Adding MarcZ for his views on GIC)
>>
>> On 20/02/14 03:59, Hanjun Guo wrote:
>>> Hi Sudeep,
>>>
>>> Thanks for your comments, please refer to the replies below. :)
>>>
>>> On 2014?02?19? 22:33, Sudeep Holla wrote:
>>>> Hi Hanjun,
>>>>
>>>> On 18/02/14 16:23, Hanjun Guo wrote:
>>>>> Get apic id from MADT or _MAT method is not implemented on 
>>>>> arm/arm64,
>>>>> and ACPI 5.0 introduces GIC Structure for it, so this patch 
>>>>> introduces
>>>>> map_gic_id() to get apic id followed the ACPI 5.0 spec.
>>>>>
>>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>>> ---
>>>>>   drivers/acpi/processor_core.c |   26 ++++++++++++++++++++++++++
>>>>>   1 file changed, 26 insertions(+)
>>>>>
>>>>> diff --git a/drivers/acpi/processor_core.c 
>>>>> b/drivers/acpi/processor_core.c
>>>>> index 4dcf776..d316d9b 100644
>>>>> --- a/drivers/acpi/processor_core.c
>>>>> +++ b/drivers/acpi/processor_core.c
>>>>> @@ -71,6 +71,27 @@ static int map_lsapic_id(struct 
>>>>> acpi_subtable_header *entry,
>>>>>   	return 0;
>>>>>   }
>>>>>
>>>>> +static int map_gic_id(struct acpi_subtable_header *entry,
>>>>> +		int device_declaration, u32 acpi_id, int *apic_id)
>>>>> +{
>>>>> +	struct acpi_madt_generic_interrupt *gic =
>>>>> +		(struct acpi_madt_generic_interrupt *)entry;
>>>>> +
>>>>> +	if (!(gic->flags & ACPI_MADT_ENABLED))
>>>>> +		return -ENODEV;
>>>>> +
>>>>> +	/* In the GIC interrupt model, logical processors are
>>>>> +	 * required to have a Processor Device object in the DSDT,
>>>>> +	 * so we should check device_declaration here
>>>>> +	 */
>>>>> +	if (device_declaration && (gic->uid == acpi_id)) {
>>>>> +		*apic_id = gic->gic_id;
>>>> I have mentioned this earlier, it's not clear yet to me how does 
>>>> this work ?
>>>> It needs more clarity in the form of comment here at-least as the 
>>>> ACPIv5.0 is
>>>> also not so clear or explicit on how to handle this.
>>>
>>> Yes, I noticed your comments and had a reply for that, after a
>>> long consideration for this, I would withdraw my previous comments
>>> before, please refer to the comments below.
>>>
>>>>
>>>> Here you are expecting gic->uid = acpi_id which is fine, while 
>>>> acpi_map_cpuid
>>>> matches apic_id with cpu_physical_id(which must be MPIDR in 
>>>> ARM{32,64}). The
>>>> latter imposes restriction that gic->gic_id has to be MPIDR. Does 
>>>> that mean we
>>>> are imposing restriction on GIC ID to be MPIDR ? If so please 
>>>> document it here
>>>> and please explain the reason behind that choice.
>>>
>>> On x86 and IA64, APIC/SAPIC ID is the hardware id of the logical
>>> processor, and UID is just a unique ID to identify the processor in 
>>> DSDT, it
>>> can be any value, and even can be strings defined in ASL if I 
>>> remember
>>> that correctly.
>>>
>> OK, but that's not the case on ARM{32,64}. My main concern here is 
>> if we don't
>> make this definitions clear enough, the vendors might produce ACPI 
>> tables with
>> whatever suits them and we may end up supporting them. Since we are 
>> starting
>> with clean slate, we can avoid getting into such situations. I will 
>> be to be
>> more elaborate this time.
>
> I agree.
>
>>
>> The GIC ID is referred as the local GIC?s hardware ID in ACPIv5.0.
>> IIUC, since GICC is per-cpu entry, it has to GIC CPU interface ID.
>>
>> Now how does it differ from MPIDR ? e.g. ARM TC2(multi cluster 
>> system)
>> 	GIC ID		MPIDR			Comment
>> 	0		0x000			CA15_0
>> 	1		0x001			CA15_1
>> 	2		0x100			CA7_0
>> 	3		0x101			CA7_1
>> 	4		0x102			CA7_2
>
> Yes, obvious different. I know GIC ID can matche the bit index of the
> associated processor
> in the distributor's GICD_ITARGETSR register, and it a clear
> statement in GICv1/GICv2, my
> question is that is this consistent in GICv3/v4 too? this will have
> some impact on the
> code implementation.

For GICv3/v4, the only way you can match a CPU with its local 
redistributor is by using the CPU MPIDR. The GIC CPU ID is an 
implementation choice that may not exist (it doesn't in a distributed 
implementation), so anything that relies on a GIC CPU ID is broken for 
GICv3.

         M.
-- 
Fast, cheap, reliable. Pick two.

[PATCH v5 3/5] ACPI / processor: Introduce map_gic_id() to get apic id from MADT or _MAT method

[Hanjun Guo]

On 2014-2-22 19:30, Marc Zyngier wrote:
> On 2014-02-22 10:21, Hanjun Guo wrote:
>> On 2014-2-21 20:37, Sudeep Holla wrote:
>>> Hi Hanjun,
>>>
>>> (Adding MarcZ for his views on GIC)
>>>
>>> On 20/02/14 03:59, Hanjun Guo wrote:
>>>> Hi Sudeep,
>>>>
>>>> Thanks for your comments, please refer to the replies below. :)
>>>>
>>>> On 2014?02?19? 22:33, Sudeep Holla wrote:
>>>>> Hi Hanjun,
>>>>>
>>>>> On 18/02/14 16:23, Hanjun Guo wrote:
>>>>>> Get apic id from MADT or _MAT method is not implemented on arm/arm64,
>>>>>> and ACPI 5.0 introduces GIC Structure for it, so this patch introduces
>>>>>> map_gic_id() to get apic id followed the ACPI 5.0 spec.
>>>>>>
>>>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>>>> ---
>>>>>>   drivers/acpi/processor_core.c |   26 ++++++++++++++++++++++++++
>>>>>>   1 file changed, 26 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/acpi/processor_core.c b/drivers/acpi/processor_core.c
>>>>>> index 4dcf776..d316d9b 100644
>>>>>> --- a/drivers/acpi/processor_core.c
>>>>>> +++ b/drivers/acpi/processor_core.c
>>>>>> @@ -71,6 +71,27 @@ static int map_lsapic_id(struct acpi_subtable_header *entry,
>>>>>>       return 0;
>>>>>>   }
>>>>>>
>>>>>> +static int map_gic_id(struct acpi_subtable_header *entry,
>>>>>> +        int device_declaration, u32 acpi_id, int *apic_id)
>>>>>> +{
>>>>>> +    struct acpi_madt_generic_interrupt *gic =
>>>>>> +        (struct acpi_madt_generic_interrupt *)entry;
>>>>>> +
>>>>>> +    if (!(gic->flags & ACPI_MADT_ENABLED))
>>>>>> +        return -ENODEV;
>>>>>> +
>>>>>> +    /* In the GIC interrupt model, logical processors are
>>>>>> +     * required to have a Processor Device object in the DSDT,
>>>>>> +     * so we should check device_declaration here
>>>>>> +     */
>>>>>> +    if (device_declaration && (gic->uid == acpi_id)) {
>>>>>> +        *apic_id = gic->gic_id;
>>>>> I have mentioned this earlier, it's not clear yet to me how does this work ?
>>>>> It needs more clarity in the form of comment here at-least as the ACPIv5.0 is
>>>>> also not so clear or explicit on how to handle this.
>>>>
>>>> Yes, I noticed your comments and had a reply for that, after a
>>>> long consideration for this, I would withdraw my previous comments
>>>> before, please refer to the comments below.
>>>>
>>>>>
>>>>> Here you are expecting gic->uid = acpi_id which is fine, while acpi_map_cpuid
>>>>> matches apic_id with cpu_physical_id(which must be MPIDR in ARM{32,64}). The
>>>>> latter imposes restriction that gic->gic_id has to be MPIDR. Does that mean we
>>>>> are imposing restriction on GIC ID to be MPIDR ? If so please document it here
>>>>> and please explain the reason behind that choice.
>>>>
>>>> On x86 and IA64, APIC/SAPIC ID is the hardware id of the logical
>>>> processor, and UID is just a unique ID to identify the processor in DSDT, it
>>>> can be any value, and even can be strings defined in ASL if I remember
>>>> that correctly.
>>>>
>>> OK, but that's not the case on ARM{32,64}. My main concern here is if we don't
>>> make this definitions clear enough, the vendors might produce ACPI tables with
>>> whatever suits them and we may end up supporting them. Since we are starting
>>> with clean slate, we can avoid getting into such situations. I will be to be
>>> more elaborate this time.
>>
>> I agree.
>>
>>>
>>> The GIC ID is referred as the local GIC?s hardware ID in ACPIv5.0.
>>> IIUC, since GICC is per-cpu entry, it has to GIC CPU interface ID.
>>>
>>> Now how does it differ from MPIDR ? e.g. ARM TC2(multi cluster system)
>>>     GIC ID        MPIDR            Comment
>>>     0        0x000            CA15_0
>>>     1        0x001            CA15_1
>>>     2        0x100            CA7_0
>>>     3        0x101            CA7_1
>>>     4        0x102            CA7_2
>>
>> Yes, obvious different. I know GIC ID can matche the bit index of the
>> associated processor
>> in the distributor's GICD_ITARGETSR register, and it a clear
>> statement in GICv1/GICv2, my
>> question is that is this consistent in GICv3/v4 too? this will have
>> some impact on the
>> code implementation.
> 
> For GICv3/v4, the only way you can match a CPU with its local redistributor is by using the CPU MPIDR. The GIC CPU ID is an implementation choice that may not exist (it doesn't in a distributed
> implementation), so anything that relies on a GIC CPU ID is broken for GICv3.

So that's the broken part, it seems that GIC ID is useless both
in GICv1/v2 and GICv3/v4 if it represents GIC CPU interface ID,
the only reliable one is the MPIDR.

I agree UID should be MPIDR for future usage, so how about this solution:

+static int map_gic_id(struct acpi_subtable_header *entry,
+		int device_declaration, u32 acpi_id, int *apic_id)
+{
+	struct acpi_madt_generic_interrupt *gic =
+		(struct acpi_madt_generic_interrupt *)entry;
+
+	if (!(gic->flags & ACPI_MADT_ENABLED))
+		return -ENODEV;
+
+	/* In the GIC interrupt model, logical processors are
+	 * required to have a Processor Device object in the DSDT,
+	 * so we should check device_declaration here
+	 */
+	if (device_declaration && (gic->uid == acpi_id)) {
		/*
		 * this is the special case for ARM/ARM64, gic->uid
		 * should be MPIDR which represents the CPU hardware
		 * id, so use gic->uid instead of gic->gic_id here
		 * to get the physical processor ID. (...)
		 */
+		*apic_id = gic->uid;
+		return 0;
+	}
+
+	return -EINVAL;
+}
+

Thanks
Hanjun

[PATCH v5 2/5] ACPI / processor_core: Rework _PDC related stuff to make it more arch-independent

[Hanjun Guo]

On 2014-2-22 18:33, Catalin Marinas wrote:
> On 21 Feb 2014, at 23:35, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Friday, February 21, 2014 06:24:24 PM Catalin Marinas wrote:
>>> On Wed, Feb 19, 2014 at 01:50:22AM +0100, Rafael J. Wysocki wrote:
>>>> On Wednesday, February 19, 2014 12:23:55 AM Hanjun Guo wrote:
>>>>> _PDC related stuff in processor_core.c is little bit X86/IA64 dependent,
>>>>> rework the code to make it more arch-independent, no functional change
>>>>> in this patch.
>>>>>
>>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>>> Signed-off-by: Graeme Gregory <graeme.gregory@linaro.org>
>>>>
>>>> I've queued up patches [1,3-5/5] from this series for 3.15 (modulo changelog
>>>> modifications), but this one should be CCed to the x86 and ia64 maintainers.
>>>
>>> Thanks for taking these patches. I would however hold onto patch 3/5 as
>>> this is still under discussion. Basically for patches specific to ARM
>>> ACPI I would really like to see more acks before being merged as that's
>>> a new thing for us.
>>
>> OK, I'll drop [3/5] for now, then.
> 
> Thanks (it?s only temporary ;)).
> 
>> I'm wondering, though, whose ACKs I should be waiting for before applying those
>> patches?
> 
> Good question ;). In this particular case, there is an ongoing
> discussion between Hanjun and Sudeep. While there isn?t anything
> major, I would like to see some agreement and potentially an Ack from
> the other party involved in the discussion (Sudeep).
> 
> There are other patches that are not specific to ARM, so it?s
> really your decision. As for the general ARM(64) ACPI case, I don?t
> think we have anyone in charge with deciding what?s correct or not
> (BTW, who are the people active both in the _ARM_ Linux kernel community
> and the ACPI standardisation forum?).

I'm in ASWG (ACPI spec working group) under UEFI, and Al Stone and Charles
(+cc Charles) are also in this forum.

Thanks
Hanjun