[PATCH v5 0/9] Support PPTT for ARM64

[PATCH v5 0/9] Support PPTT for ARM64

[Jeremy Linton]

This patch set is dependent on "[14/15] ACPICA: ACPI 6.2: Additional
PPTT flags" https://patchwork.kernel.org/patch/10064191/

ACPI 6.2 adds the Processor Properties Topology Table (PPTT), which is
used to describe the processor and cache topology. Ideally it is
used to extend/override information provided by the hardware, but
right now ARM64 is entirely dependent on firmware provided tables.

This patch parses the table for the cache topology and CPU topology.
When we enable ACPI/PPTT for arm64 we map the physical_id to the
PPTT node flagged as the physical package by the firmware.
This results in topologies that match what the remainder of the
system expects.

For example on juno:
[root@mammon-juno-rh topology]# lstopo-no-graphics
  Package L#0
    L2 L#0 (1024KB)
      L1d L#0 (32KB) + L1i L#0 (32KB) + Core L#0 + PU L#0 (P#0)
      L1d L#1 (32KB) + L1i L#1 (32KB) + Core L#1 + PU L#1 (P#1)
      L1d L#2 (32KB) + L1i L#2 (32KB) + Core L#2 + PU L#2 (P#2)
      L1d L#3 (32KB) + L1i L#3 (32KB) + Core L#3 + PU L#3 (P#3)
    L2 L#1 (2048KB)
      L1d L#4 (32KB) + L1i L#4 (48KB) + Core L#4 + PU L#4 (P#4)
      L1d L#5 (32KB) + L1i L#5 (48KB) + Core L#5 + PU L#5 (P#5)
  HostBridge L#0
    PCIBridge
      PCIBridge
        PCIBridge
          PCI 1095:3132
            Block(Disk) L#0 "sda"
        PCIBridge
          PCI 1002:68f9
            GPU L#1 "renderD128"
            GPU L#2 "card0"
            GPU L#3 "controlD64"
        PCIBridge
          PCI 11ab:4380
            Net L#4 "enp8s0"


Git tree at:
http://linux-arm.org/git?p=linux-jlinton.git
branch: pptt_v5

v4->v5:
Update the cache type from NOCACHE to UNIFIED when all the cache
  attributes we update are valid. This fixes a problem where caches
  which are entirely created by the PPTT don't show up in lstopo.

Give the PPTT its own firmware_node in the cache structure instead of
  sharing it with the of_node.

Move some pieces around between patches.

v3->v4:
Suppress the "Found duplicate cache level/type..." message if the
  duplicate cache entry is actually a duplicate node. This allows cases
  like L1I and L1D nodes that point at the same L2 node to be accepted
  without the warning.
  
Remove cluster/physical split code. Add a patch to rename cluster_id
  so that its clear the topology may not be referring to a cluster.
  
Add additional ACPICA patch for the PPTT cache properties. This matches
  an outstanding ACPICA pull that should be merged in the near future.
  
Replace a number of (struct*)((u8*)ptr+offset) constructs with ACPI_ADD_PTR

Split out the topology parsing into an additional patch.

Tweak the cpu topology code to terminate on either a level, or a flag.
  Add an additional function which retrives the physical package id
  for a given cpu.

Various other comments/tweaks.

v2->v3:

Remove valid bit check on leaf nodes. Now simply being a leaf node
  is sufficient to verify the processor id against the ACPI
  processor ids (gotten from MADT). 

Use the acpi processor for the "level 0" Id. This makes the /sys
  visible core/thread ids more human readable if the firmware uses
  small consecutive values for processor ids.

Added PPTT to the list of injectable ACPI tables.

Fix bug which kept the code from using the processor node as intended
  in v2, caused by misuse of git rebase/fixup.

v1->v2:

The parser keys off the acpi_pptt_processor node to determine
  unique cache's rather than the acpi_pptt_cache referenced by the
  processor node. This allows PPTT tables which "share" cache nodes
  across cpu nodes despite not being a shared cache.

Jeremy Linton (9):
  arm64/acpi: Create arch specific cpu to acpi id helper
  ACPI/PPTT: Add Processor Properties Topology Table parsing
  ACPI: Enable PPTT support on ARM64
  drivers: base cacheinfo: Add support for ACPI based firmware tables
  arm64: Add support for ACPI based firmware tables
  ACPI/PPTT: Add topology parsing code
  arm64: Topology, rename cluster_id
  arm64: topology: Enable ACPI/PPTT based CPU topology.
  ACPI: Add PPTT to injectable table list

 arch/arm64/Kconfig                |   1 +
 arch/arm64/include/asm/acpi.h     |   4 +
 arch/arm64/include/asm/topology.h |   4 +-
 arch/arm64/kernel/cacheinfo.c     |  15 +-
 arch/arm64/kernel/topology.c      |  74 ++++-
 drivers/acpi/Kconfig              |   3 +
 drivers/acpi/Makefile             |   1 +
 drivers/acpi/pptt.c               | 592 ++++++++++++++++++++++++++++++++++++++
 drivers/acpi/tables.c             |   3 +-
 drivers/base/cacheinfo.c          |  20 +-
 include/linux/cacheinfo.h         |  13 +-
 include/linux/topology.h          |   2 +
 12 files changed, 703 insertions(+), 29 deletions(-)
 create mode 100644 drivers/acpi/pptt.c

-- 
2.13.5

[PATCH v5 1/9] arm64/acpi: Create arch specific cpu to acpi id helper

[Jeremy Linton]

Its helpful to be able to lookup the acpi_processor_id associated
with a logical cpu. Provide an arm64 helper to do this.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 arch/arm64/include/asm/acpi.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index 32f465a80e4e..0db62a4cbce2 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -86,6 +86,10 @@ static inline bool acpi_has_cpu_in_madt(void)
 }
 
 struct acpi_madt_generic_interrupt *acpi_cpu_get_madt_gicc(int cpu);
+static inline u32 get_acpi_id_for_cpu(unsigned int cpu)
+{
+	return	acpi_cpu_get_madt_gicc(cpu)->uid;
+}
 
 static inline void arch_fix_phys_package_id(int num, u32 slot) { }
 void __init acpi_init_cpus(void);
-- 
2.13.5

[PATCH v5 2/9] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Jeremy Linton]

ACPI 6.2 adds a new table, which describes how processing units
are related to each other in tree like fashion. Caches are
also sprinkled throughout the tree and describe the properties
of the caches in relation to other caches and processing units.

Add the code to parse the cache hierarchy and report the total
number of levels of cache for a given core using
acpi_find_last_cache_level() as well as fill out the individual
cores cache information with cache_setup_acpi() once the
cpu_cacheinfo structure has been populated by the arch specific
code.

An additional patch later in the set adds the ability to report
peers in the topology using find_acpi_cpu_topology()
to report a unique ID for each processing unit at a given level
in the tree. These unique id's can then be used to match related
processing units which exist as threads, COD (clusters
on die), within a given package, etc.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 drivers/acpi/pptt.c | 476 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 476 insertions(+)
 create mode 100644 drivers/acpi/pptt.c

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
new file mode 100644
index 000000000000..0f8a1631af33
--- /dev/null
+++ b/drivers/acpi/pptt.c
@@ -0,0 +1,476 @@
+/*
+ * Copyright (C) 2017, ARM
+ *
+ * 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.
+ *
+ * This file implements parsing of Processor Properties Topology Table (PPTT)
+ * which is optionally used to describe the processor and cache topology.
+ * Due to the relative pointers used throughout the table, this doesn't
+ * leverage the existing subtable parsing in the kernel.
+ *
+ * The PPTT structure is an inverted tree, with each node potentially
+ * holding one or two inverted tree data structures describing
+ * the caches available at that level. Each cache structure optionally
+ * contains properties describing the cache at a given level which can be
+ * used to override hardware probed values.
+ */
+#define pr_fmt(fmt) "ACPI PPTT: " fmt
+
+#include <linux/acpi.h>
+#include <linux/cacheinfo.h>
+#include <acpi/processor.h>
+
+/* total number of attributes checked by the properties code */
+#define PPTT_CHECKED_ATTRIBUTES 6
+
+/*
+ * Given the PPTT table, find and verify that the subtable entry
+ * is located within the table
+ */
+static struct acpi_subtable_header *fetch_pptt_subtable(
+	struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+	struct acpi_subtable_header *entry;
+
+	/* there isn't a subtable at reference 0 */
+	if (pptt_ref < sizeof(struct acpi_subtable_header))
+		return NULL;
+
+	if (pptt_ref + sizeof(struct acpi_subtable_header) > table_hdr->length)
+		return NULL;
+
+	entry = ACPI_ADD_PTR(struct acpi_subtable_header, table_hdr, pptt_ref);
+
+	if (pptt_ref + entry->length > table_hdr->length)
+		return NULL;
+
+	return entry;
+}
+
+static struct acpi_pptt_processor *fetch_pptt_node(
+	struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+	return (struct acpi_pptt_processor *)fetch_pptt_subtable(table_hdr,
+								 pptt_ref);
+}
+
+static struct acpi_pptt_cache *fetch_pptt_cache(
+	struct acpi_table_header *table_hdr, u32 pptt_ref)
+{
+	return (struct acpi_pptt_cache *)fetch_pptt_subtable(table_hdr,
+							     pptt_ref);
+}
+
+static struct acpi_subtable_header *acpi_get_pptt_resource(
+	struct acpi_table_header *table_hdr,
+	struct acpi_pptt_processor *node, int resource)
+{
+	u32 *ref;
+
+	if (resource >= node->number_of_priv_resources)
+		return NULL;
+
+	ref = ACPI_ADD_PTR(u32, node, sizeof(struct acpi_pptt_processor));
+	ref += resource;
+
+	return fetch_pptt_subtable(table_hdr, *ref);
+}
+
+/*
+ * Attempt to find a given cache level, while counting the max number
+ * of cache levels for the cache node.
+ *
+ * Given a pptt resource, verify that it is a cache node, then walk
+ * down each level of caches, counting how many levels are found
+ * as well as checking the cache type (icache, dcache, unified). If a
+ * level & type match, then we set found, and continue the search.
+ * Once the entire cache branch has been walked return its max
+ * depth.
+ */
+static int acpi_pptt_walk_cache(struct acpi_table_header *table_hdr,
+				int local_level,
+				struct acpi_subtable_header *res,
+				struct acpi_pptt_cache **found,
+				int level, int type)
+{
+	struct acpi_pptt_cache *cache;
+
+	if (res->type != ACPI_PPTT_TYPE_CACHE)
+		return 0;
+
+	cache = (struct acpi_pptt_cache *) res;
+	while (cache) {
+		local_level++;
+
+		if ((local_level == level) &&
+		    (cache->flags & ACPI_PPTT_CACHE_TYPE_VALID) &&
+		    ((cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) == type)) {
+			if ((*found != NULL) && (cache != *found))
+				pr_err("Found duplicate cache level/type unable to determine uniqueness\n");
+
+			pr_debug("Found cache @ level %d\n", level);
+			*found = cache;
+			/*
+			 * continue looking at this node's resource list
+			 * to verify that we don't find a duplicate
+			 * cache node.
+			 */
+		}
+		cache = fetch_pptt_cache(table_hdr, cache->next_level_of_cache);
+	}
+	return local_level;
+}
+
+/*
+ * Given a CPU node look for cache levels that exist at this level, and then
+ * for each cache node, count how many levels exist below (logically above) it.
+ * If a level and type are specified, and we find that level/type, abort
+ * processing and return the acpi_pptt_cache structure.
+ */
+static struct acpi_pptt_cache *acpi_find_cache_level(
+	struct acpi_table_header *table_hdr,
+	struct acpi_pptt_processor *cpu_node,
+	int *starting_level, int level, int type)
+{
+	struct acpi_subtable_header *res;
+	int number_of_levels = *starting_level;
+	int resource = 0;
+	struct acpi_pptt_cache *ret = NULL;
+	int local_level;
+
+	/* walk down from processor node */
+	while ((res = acpi_get_pptt_resource(table_hdr, cpu_node, resource))) {
+		resource++;
+
+		local_level = acpi_pptt_walk_cache(table_hdr, *starting_level,
+						   res, &ret, level, type);
+		/*
+		 * we are looking for the max depth. Since its potentially
+		 * possible for a given node to have resources with differing
+		 * depths verify that the depth we have found is the largest.
+		 */
+		if (number_of_levels < local_level)
+			number_of_levels = local_level;
+	}
+	if (number_of_levels > *starting_level)
+		*starting_level = number_of_levels;
+
+	return ret;
+}
+
+/*
+ * Given a processor node containing a processing unit, walk into it and count
+ * how many levels exist solely for it, and then walk up each level until we hit
+ * the root node (ignore the package level because it may be possible to have
+ * caches that exist across packages). Count the number of cache levels that
+ * exist at each level on the way up.
+ */
+static int acpi_process_node(struct acpi_table_header *table_hdr,
+			     struct acpi_pptt_processor *cpu_node)
+{
+	int total_levels = 0;
+
+	do {
+		acpi_find_cache_level(table_hdr, cpu_node, &total_levels, 0, 0);
+		cpu_node = fetch_pptt_node(table_hdr, cpu_node->parent);
+	} while (cpu_node);
+
+	return total_levels;
+}
+
+/*
+ * Determine if the *node parameter is a leaf node by iterating the
+ * PPTT table, looking for nodes which reference it.
+ * Return 0 if we find a node referencing the passed node,
+ * or 1 if we don't.
+ */
+static int acpi_pptt_leaf_node(struct acpi_table_header *table_hdr,
+			       struct acpi_pptt_processor *node)
+{
+	struct acpi_subtable_header *entry;
+	unsigned long table_end;
+	u32 node_entry;
+	struct acpi_pptt_processor *cpu_node;
+
+	table_end = (unsigned long)table_hdr + table_hdr->length;
+	node_entry = ACPI_PTR_DIFF(node, table_hdr);
+	entry = ACPI_ADD_PTR(struct acpi_subtable_header, table_hdr,
+			     sizeof(struct acpi_table_pptt));
+
+	while ((unsigned long)(entry + 1) < table_end) {
+		cpu_node = (struct acpi_pptt_processor *)entry;
+		if ((entry->type == ACPI_PPTT_TYPE_PROCESSOR) &&
+		    (cpu_node->parent == node_entry))
+			return 0;
+		entry = ACPI_ADD_PTR(struct acpi_subtable_header, entry,
+				     entry->length);
+	}
+	return 1;
+}
+
+/*
+ * Find the subtable entry describing the provided processor.
+ * This is done by iterating the PPTT table looking for processor nodes
+ * which have an acpi_processor_id that matches the acpi_cpu_id parameter
+ * passed into the function. If we find a node that matches this criteria
+ * we verify that its a leaf node in the topology rather than depending
+ * on the valid flag, which doesn't need to be set for leaf nodes.
+ */
+static struct acpi_pptt_processor *acpi_find_processor_node(
+	struct acpi_table_header *table_hdr,
+	u32 acpi_cpu_id)
+{
+	struct acpi_subtable_header *entry;
+	unsigned long table_end;
+	struct acpi_pptt_processor *cpu_node;
+
+	table_end = (unsigned long)table_hdr + table_hdr->length;
+	entry = ACPI_ADD_PTR(struct acpi_subtable_header, table_hdr,
+			     sizeof(struct acpi_table_pptt));
+
+	/* find the processor structure associated with this cpuid */
+	while ((unsigned long)(entry + 1) < table_end) {
+		cpu_node = (struct acpi_pptt_processor *)entry;
+
+		if (entry->length == 0) {
+			pr_err("Invalid zero length subtable\n");
+			break;
+		}
+		if ((entry->type == ACPI_PPTT_TYPE_PROCESSOR) &&
+		    (acpi_cpu_id == cpu_node->acpi_processor_id) &&
+		     acpi_pptt_leaf_node(table_hdr, cpu_node)) {
+			return (struct acpi_pptt_processor *)entry;
+		}
+
+		entry = ACPI_ADD_PTR(struct acpi_subtable_header, entry,
+				     entry->length);
+	}
+
+	return NULL;
+}
+
+static int acpi_find_cache_levels(struct acpi_table_header *table_hdr,
+				  u32 acpi_cpu_id)
+{
+	int number_of_levels = 0;
+	struct acpi_pptt_processor *cpu;
+
+	cpu = acpi_find_processor_node(table_hdr, acpi_cpu_id);
+	if (cpu)
+		number_of_levels = acpi_process_node(table_hdr, cpu);
+
+	return number_of_levels;
+}
+
+/* Convert the linux cache_type to a ACPI PPTT cache type value */
+static u8 acpi_cache_type(enum cache_type type)
+{
+	switch (type) {
+	case CACHE_TYPE_DATA:
+		pr_debug("Looking for data cache\n");
+		return ACPI_PPTT_CACHE_TYPE_DATA;
+	case CACHE_TYPE_INST:
+		pr_debug("Looking for instruction cache\n");
+		return ACPI_PPTT_CACHE_TYPE_INSTR;
+	default:
+	case CACHE_TYPE_UNIFIED:
+		pr_debug("Looking for unified cache\n");
+		/*
+		 * It is important that ACPI_PPTT_CACHE_TYPE_UNIFIED
+		 * contains the bit pattern that will match both
+		 * ACPI unified bit patterns because we use it later
+		 * to match both cases.
+		 */
+		return ACPI_PPTT_CACHE_TYPE_UNIFIED;
+	}
+}
+
+/* find the ACPI node describing the cache type/level for the given CPU */
+static struct acpi_pptt_cache *acpi_find_cache_node(
+	struct acpi_table_header *table_hdr, u32 acpi_cpu_id,
+	enum cache_type type, unsigned int level,
+	struct acpi_pptt_processor **node)
+{
+	int total_levels = 0;
+	struct acpi_pptt_cache *found = NULL;
+	struct acpi_pptt_processor *cpu_node;
+	u8 acpi_type = acpi_cache_type(type);
+
+	pr_debug("Looking for CPU %d's level %d cache type %d\n",
+		 acpi_cpu_id, level, acpi_type);
+
+	cpu_node = acpi_find_processor_node(table_hdr, acpi_cpu_id);
+
+	while ((cpu_node) && (!found)) {
+		found = acpi_find_cache_level(table_hdr, cpu_node,
+					      &total_levels, level, acpi_type);
+		*node = cpu_node;
+		cpu_node = fetch_pptt_node(table_hdr, cpu_node->parent);
+	}
+
+	return found;
+}
+
+/*
+ * The ACPI spec implies that the fields in the cache structures are used to
+ * extend and correct the information probed from the hardware. In the case
+ * of arm64 the CCSIDR probing has been removed because it might be incorrect.
+ */
+static void update_cache_properties(struct cacheinfo *this_leaf,
+				    struct acpi_pptt_cache *found_cache,
+				    struct acpi_pptt_processor *cpu_node)
+{
+	int valid_flags = 0;
+
+	if (found_cache->flags & ACPI_PPTT_SIZE_PROPERTY_VALID) {
+		this_leaf->size = found_cache->size;
+		valid_flags++;
+	}
+	if (found_cache->flags & ACPI_PPTT_LINE_SIZE_VALID) {
+		this_leaf->coherency_line_size = found_cache->line_size;
+		valid_flags++;
+	}
+	if (found_cache->flags & ACPI_PPTT_NUMBER_OF_SETS_VALID) {
+		this_leaf->number_of_sets = found_cache->number_of_sets;
+		valid_flags++;
+	}
+	if (found_cache->flags & ACPI_PPTT_ASSOCIATIVITY_VALID) {
+		this_leaf->ways_of_associativity = found_cache->associativity;
+		valid_flags++;
+	}
+	if (found_cache->flags & ACPI_PPTT_WRITE_POLICY_VALID) {
+		switch (found_cache->attributes & ACPI_PPTT_MASK_WRITE_POLICY) {
+		case ACPI_PPTT_CACHE_POLICY_WT:
+			this_leaf->attributes = CACHE_WRITE_THROUGH;
+			break;
+		case ACPI_PPTT_CACHE_POLICY_WB:
+			this_leaf->attributes = CACHE_WRITE_BACK;
+			break;
+		}
+		valid_flags++;
+	}
+	if (found_cache->flags & ACPI_PPTT_ALLOCATION_TYPE_VALID) {
+		switch (found_cache->attributes & ACPI_PPTT_MASK_ALLOCATION_TYPE) {
+		case ACPI_PPTT_CACHE_READ_ALLOCATE:
+			this_leaf->attributes |= CACHE_READ_ALLOCATE;
+			break;
+		case ACPI_PPTT_CACHE_WRITE_ALLOCATE:
+			this_leaf->attributes |= CACHE_WRITE_ALLOCATE;
+			break;
+		case ACPI_PPTT_CACHE_RW_ALLOCATE:
+		case ACPI_PPTT_CACHE_RW_ALLOCATE_ALT:
+			this_leaf->attributes |=
+				CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE;
+			break;
+		}
+		valid_flags++;
+	}
+	/*
+	 * If all the above flags are valid, and the cache type is NOCACHE
+	 * update the cache type as well.
+	 */
+	if ((this_leaf->type == CACHE_TYPE_NOCACHE) &&
+	    (valid_flags == PPTT_CHECKED_ATTRIBUTES))
+		this_leaf->type = CACHE_TYPE_UNIFIED;
+}
+
+/*
+ * Update the kernel cache information for each level of cache
+ * associated with the given acpi cpu.
+ */
+static void cache_setup_acpi_cpu(struct acpi_table_header *table,
+				 unsigned int cpu)
+{
+	struct acpi_pptt_cache *found_cache;
+	struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
+	u32 acpi_cpu_id = get_acpi_id_for_cpu(cpu);
+	struct cacheinfo *this_leaf;
+	unsigned int index = 0;
+	struct acpi_pptt_processor *cpu_node = NULL;
+
+	while (index < get_cpu_cacheinfo(cpu)->num_leaves) {
+		this_leaf = this_cpu_ci->info_list + index;
+		found_cache = acpi_find_cache_node(table, acpi_cpu_id,
+						   this_leaf->type,
+						   this_leaf->level,
+						   &cpu_node);
+		pr_debug("found = %p %p\n", found_cache, cpu_node);
+		if (found_cache)
+			update_cache_properties(this_leaf,
+						found_cache,
+						cpu_node);
+
+		index++;
+	}
+}
+
+/**
+ * acpi_find_last_cache_level() - Determines the number of cache levels for a PE
+ * @cpu: Kernel logical cpu number
+ *
+ * Given a logical cpu number, returns the number of levels of cache represented
+ * in the PPTT. Errors caused by lack of a PPTT table, or otherwise, return 0
+ * indicating we didn't find any cache levels.
+ *
+ * Return: Cache levels visible to this core.
+ */
+int acpi_find_last_cache_level(unsigned int cpu)
+{
+	u32 acpi_cpu_id;
+	struct acpi_table_header *table;
+	int number_of_levels = 0;
+	acpi_status status;
+
+	pr_debug("Cache Setup find last level cpu=%d\n", cpu);
+
+	acpi_cpu_id = get_acpi_id_for_cpu(cpu);
+	status = acpi_get_table(ACPI_SIG_PPTT, 0, &table);
+	if (ACPI_FAILURE(status)) {
+		pr_err_once("No PPTT table found, cache topology may be inaccurate\n");
+	} else {
+		number_of_levels = acpi_find_cache_levels(table, acpi_cpu_id);
+		acpi_put_table(table);
+	}
+	pr_debug("Cache Setup find last level level=%d\n", number_of_levels);
+
+	return number_of_levels;
+}
+
+/**
+ * cache_setup_acpi() - Override CPU cache topology with data from the PPTT
+ * @cpu: Kernel logical cpu number
+ *
+ * Updates the global cache info provided by cpu_get_cacheinfo()
+ * when there are valid properties in the acpi_pptt_cache nodes. A
+ * successful parse may not result in any updates if none of the
+ * cache levels have any valid flags set.  Futher, a unique value is
+ * associated with each known CPU cache entry. This unique value
+ * can be used to determine whether caches are shared between cpus.
+ *
+ * Return: -ENOENT on failure to find table, or 0 on success
+ */
+int cache_setup_acpi(unsigned int cpu)
+{
+	struct acpi_table_header *table;
+	acpi_status status;
+
+	pr_debug("Cache Setup ACPI cpu %d\n", cpu);
+
+	status = acpi_get_table(ACPI_SIG_PPTT, 0, &table);
+	if (ACPI_FAILURE(status)) {
+		pr_err_once("No PPTT table found, cache topology may be inaccurate\n");
+		return -ENOENT;
+	}
+
+	cache_setup_acpi_cpu(table, cpu);
+	acpi_put_table(table);
+
+	return status;
+}
-- 
2.13.5

[PATCH v5 3/9] ACPI: Enable PPTT support on ARM64

[Jeremy Linton]

Now that we have a PPTT parser, in preparation for its use
on arm64, lets build it.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 arch/arm64/Kconfig    | 1 +
 drivers/acpi/Kconfig  | 3 +++
 drivers/acpi/Makefile | 1 +
 3 files changed, 5 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index a93339f5178f..e62fd1e08c1f 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -7,6 +7,7 @@ config ARM64
 	select ACPI_REDUCED_HARDWARE_ONLY if ACPI
 	select ACPI_MCFG if ACPI
 	select ACPI_SPCR_TABLE if ACPI
+	select ACPI_PPTT if ACPI
 	select ARCH_CLOCKSOURCE_DATA
 	select ARCH_HAS_DEBUG_VIRTUAL
 	select ARCH_HAS_DEVMEM_IS_ALLOWED
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 46505396869e..df7aebf0af0e 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -545,6 +545,9 @@ config ACPI_CONFIGFS
 
 if ARM64
 source "drivers/acpi/arm64/Kconfig"
+
+config ACPI_PPTT
+	bool
 endif
 
 config TPS68470_PMIC_OPREGION
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index 41954a601989..b6056b566df4 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -87,6 +87,7 @@ obj-$(CONFIG_ACPI_BGRT)		+= bgrt.o
 obj-$(CONFIG_ACPI_CPPC_LIB)	+= cppc_acpi.o
 obj-$(CONFIG_ACPI_SPCR_TABLE)	+= spcr.o
 obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o
+obj-$(CONFIG_ACPI_PPTT) 	+= pptt.o
 
 # processor has its own "processor." module_param namespace
 processor-y			:= processor_driver.o
-- 
2.13.5

[PATCH v5 4/9] drivers: base cacheinfo: Add support for ACPI based firmware tables

[Jeremy Linton]

Add a entry to to struct cacheinfo to maintain a reference to the PPTT
node which can be used to match identical caches across cores. Also
stub out cache_setup_acpi() so that individual architectures can
enable ACPI topology parsing.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 drivers/acpi/pptt.c       |  1 +
 drivers/base/cacheinfo.c  | 20 ++++++++++++++------
 include/linux/cacheinfo.h | 13 ++++++++++++-
 3 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
index 0f8a1631af33..a35e457cefb7 100644
--- a/drivers/acpi/pptt.c
+++ b/drivers/acpi/pptt.c
@@ -329,6 +329,7 @@ static void update_cache_properties(struct cacheinfo *this_leaf,
 {
 	int valid_flags = 0;
 
+	this_leaf->firmware_node = cpu_node;
 	if (found_cache->flags & ACPI_PPTT_SIZE_PROPERTY_VALID) {
 		this_leaf->size = found_cache->size;
 		valid_flags++;
diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
index eb3af2739537..ba89f9310e6f 100644
--- a/drivers/base/cacheinfo.c
+++ b/drivers/base/cacheinfo.c
@@ -86,7 +86,10 @@ static int cache_setup_of_node(unsigned int cpu)
 static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf,
 					   struct cacheinfo *sib_leaf)
 {
-	return sib_leaf->of_node == this_leaf->of_node;
+	if (acpi_disabled)
+		return sib_leaf->of_node == this_leaf->of_node;
+	else
+		return sib_leaf->firmware_node == this_leaf->firmware_node;
 }
 
 /* OF properties to query for a given cache type */
@@ -215,6 +218,11 @@ static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf,
 }
 #endif
 
+int __weak cache_setup_acpi(unsigned int cpu)
+{
+	return -ENOTSUPP;
+}
+
 static int cache_shared_cpu_map_setup(unsigned int cpu)
 {
 	struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
@@ -225,11 +233,11 @@ static int cache_shared_cpu_map_setup(unsigned int cpu)
 	if (this_cpu_ci->cpu_map_populated)
 		return 0;
 
-	if (of_have_populated_dt())
+	if (!acpi_disabled)
+		ret = cache_setup_acpi(cpu);
+	else if (of_have_populated_dt())
 		ret = cache_setup_of_node(cpu);
-	else if (!acpi_disabled)
-		/* No cache property/hierarchy support yet in ACPI */
-		ret = -ENOTSUPP;
+
 	if (ret)
 		return ret;
 
@@ -286,7 +294,7 @@ static void cache_shared_cpu_map_remove(unsigned int cpu)
 
 static void cache_override_properties(unsigned int cpu)
 {
-	if (of_have_populated_dt())
+	if (acpi_disabled && of_have_populated_dt())
 		return cache_of_override_properties(cpu);
 }
 
diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
index 3d9805297cda..7ebff157ae6c 100644
--- a/include/linux/cacheinfo.h
+++ b/include/linux/cacheinfo.h
@@ -37,6 +37,8 @@ enum cache_type {
  * @of_node: if devicetree is used, this represents either the cpu node in
  *	case there's no explicit cache node or the cache node itself in the
  *	device tree
+ * @firmware_node: When not using DT, this may contain pointers to other
+ *	firmware based values. Particularly ACPI/PPTT unique values.
  * @disable_sysfs: indicates whether this node is visible to the user via
  *	sysfs or not
  * @priv: pointer to any private data structure specific to particular
@@ -65,8 +67,8 @@ struct cacheinfo {
 #define CACHE_ALLOCATE_POLICY_MASK	\
 	(CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE)
 #define CACHE_ID		BIT(4)
-
 	struct device_node *of_node;
+	void *firmware_node;
 	bool disable_sysfs;
 	void *priv;
 };
@@ -99,6 +101,15 @@ int func(unsigned int cpu)					\
 struct cpu_cacheinfo *get_cpu_cacheinfo(unsigned int cpu);
 int init_cache_level(unsigned int cpu);
 int populate_cache_leaves(unsigned int cpu);
+int cache_setup_acpi(unsigned int cpu);
+int acpi_find_last_cache_level(unsigned int cpu);
+#ifndef CONFIG_ACPI
+int acpi_find_last_cache_level(unsigned int cpu)
+{
+	/*ACPI kernels should be built with PPTT support*/
+	return 0;
+}
+#endif
 
 const struct attribute_group *cache_get_priv_group(struct cacheinfo *this_leaf);
 
-- 
2.13.5

[PATCH v5 5/9] arm64: Add support for ACPI based firmware tables

[Jeremy Linton]

The /sys cache entries should support ACPI/PPTT generated cache
topology information. Lets detect ACPI systems and call
an arch specific cache_setup_acpi() routine to update the hardware
probed cache topology.

For arm64, if ACPI is enabled, determine the max number of cache
levels and populate them using the PPTT table if one is available.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 arch/arm64/kernel/cacheinfo.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kernel/cacheinfo.c b/arch/arm64/kernel/cacheinfo.c
index 380f2e2fbed5..0bf0a835122f 100644
--- a/arch/arm64/kernel/cacheinfo.c
+++ b/arch/arm64/kernel/cacheinfo.c
@@ -17,6 +17,7 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <linux/acpi.h>
 #include <linux/cacheinfo.h>
 #include <linux/of.h>
 
@@ -46,7 +47,7 @@ static void ci_leaf_init(struct cacheinfo *this_leaf,
 
 static int __init_cache_level(unsigned int cpu)
 {
-	unsigned int ctype, level, leaves, of_level;
+	unsigned int ctype, level, leaves, fw_level;
 	struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
 
 	for (level = 1, leaves = 0; level <= MAX_CACHE_LEVEL; level++) {
@@ -59,15 +60,19 @@ static int __init_cache_level(unsigned int cpu)
 		leaves += (ctype == CACHE_TYPE_SEPARATE) ? 2 : 1;
 	}
 
-	of_level = of_find_last_cache_level(cpu);
-	if (level < of_level) {
+	if (acpi_disabled)
+		fw_level = of_find_last_cache_level(cpu);
+	else
+		fw_level = acpi_find_last_cache_level(cpu);
+
+	if (level < fw_level) {
 		/*
 		 * some external caches not specified in CLIDR_EL1
 		 * the information may be available in the device tree
 		 * only unified external caches are considered here
 		 */
-		leaves += (of_level - level);
-		level = of_level;
+		leaves += (fw_level - level);
+		level = fw_level;
 	}
 
 	this_cpu_ci->num_levels = level;
-- 
2.13.5

[PATCH v5 6/9] ACPI/PPTT: Add topology parsing code

[Jeremy Linton]

The PPTT can be used to determine the groupings of CPU's at
given levels in the system. Lets add a few routines to the PPTT
parsing code to return a unique id for each unique level in the
processor hierarchy. This can then be matched to build
thread/core/cluster/die/package/etc mappings for each processing
element in the system.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 drivers/acpi/pptt.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 115 insertions(+)

diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
index a35e457cefb7..b9b7b8b8ad21 100644
--- a/drivers/acpi/pptt.c
+++ b/drivers/acpi/pptt.c
@@ -412,6 +412,79 @@ static void cache_setup_acpi_cpu(struct acpi_table_header *table,
 	}
 }
 
+/* Passing level values greater than this will result in search termination */
+#define PPTT_ABORT_PACKAGE 0xFF
+
+/*
+ * Given an acpi_pptt_processor node, walk up until we identify the
+ * package that the node is associated with, or we run out of levels
+ * to request or the search is terminated with a flag match
+ * The level parameter also serves to limit possible loops within the tree.
+ */
+static struct acpi_pptt_processor *acpi_find_processor_package_id(
+	struct acpi_table_header *table_hdr,
+	struct acpi_pptt_processor *cpu,
+	int level, int flag)
+{
+	struct acpi_pptt_processor *prev_node;
+
+	while (cpu && level) {
+		if (cpu->flags & flag)
+			break;
+		pr_debug("level %d\n", level);
+		prev_node = fetch_pptt_node(table_hdr, cpu->parent);
+		if (prev_node == NULL)
+			break;
+		cpu = prev_node;
+		level--;
+	}
+	return cpu;
+}
+
+/*
+ * Get a unique value given a cpu, and a topology level, that can be
+ * matched to determine which cpus share common topological features
+ * at that level.
+ */
+static int topology_get_acpi_cpu_tag(struct acpi_table_header *table,
+				     unsigned int cpu, int level, int flag)
+{
+	struct acpi_pptt_processor *cpu_node;
+	u32 acpi_cpu_id = get_acpi_id_for_cpu(cpu);
+
+	cpu_node = acpi_find_processor_node(table, acpi_cpu_id);
+	if (cpu_node) {
+		cpu_node = acpi_find_processor_package_id(table, cpu_node,
+							  level, flag);
+		/* Only the first level has a guaranteed id */
+		if (level == 0)
+			return cpu_node->acpi_processor_id;
+		return (int)((u8 *)cpu_node - (u8 *)table);
+	}
+	pr_err_once("PPTT table found, but unable to locate core for %d\n",
+		    cpu);
+	return -ENOENT;
+}
+
+static int find_acpi_cpu_topology_tag(unsigned int cpu, int level, int flag)
+{
+	struct acpi_table_header *table;
+	acpi_status status;
+	int retval;
+
+	status = acpi_get_table(ACPI_SIG_PPTT, 0, &table);
+	if (ACPI_FAILURE(status)) {
+		pr_err_once("No PPTT table found, cpu topology may be inaccurate\n");
+		return -ENOENT;
+	}
+	retval = topology_get_acpi_cpu_tag(table, cpu, level, flag);
+	pr_debug("Topology Setup ACPI cpu %d, level %d ret = %d\n",
+		 cpu, level, retval);
+	acpi_put_table(table);
+
+	return retval;
+}
+
 /**
  * acpi_find_last_cache_level() - Determines the number of cache levels for a PE
  * @cpu: Kernel logical cpu number
@@ -475,3 +548,45 @@ int cache_setup_acpi(unsigned int cpu)
 
 	return status;
 }
+
+/**
+ * find_acpi_cpu_topology() - Determine a unique topology value for a given cpu
+ * @cpu: Kernel logical cpu number
+ * @level: The topological level for which we would like a unique ID
+ *
+ * Determine a topology unique ID for each thread/core/cluster/mc_grouping
+ * /socket/etc. This ID can then be used to group peers, which will have
+ * matching ids.
+ *
+ * The search terminates when either the requested level is found or
+ * we reach a root node. Levels beyond the termination point will return the
+ * same unique ID. The unique id for level 0 is the acpi processor id. All
+ * other levels beyond this use a generated value to uniquely identify
+ * a topological feature.
+ *
+ * Return: -ENOENT if the PPTT doesn't exist, or the cpu cannot be found.
+ * Otherwise returns a value which represents a unique topological feature.
+ */
+int find_acpi_cpu_topology(unsigned int cpu, int level)
+{
+	return find_acpi_cpu_topology_tag(cpu, level, 0);
+}
+
+/**
+ * find_acpi_cpu_topology_package() - Determine a unique cpu package value
+ * @cpu: Kernel logical cpu number
+ *
+ * Determine a topology unique package ID for the given cpu.
+ * This ID can then be used to group peers, which will have matching ids.
+ *
+ * The search terminates when either a level is found with the PHYSICAL_PACKAGE
+ * flag set or we reach a root node.
+ *
+ * Return: -ENOENT if the PPTT doesn't exist, or the cpu cannot be found.
+ * Otherwise returns a value which represents the package for this cpu.
+ */
+int find_acpi_cpu_topology_package(unsigned int cpu)
+{
+	return find_acpi_cpu_topology_tag(cpu, PPTT_ABORT_PACKAGE,
+					  ACPI_PPTT_PHYSICAL_PACKAGE);
+}
-- 
2.13.5

[PATCH v5 7/9] arm64: Topology, rename cluster_id

[Jeremy Linton]

Lets match the name of the arm64 topology field
to the kernel macro that uses it.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 arch/arm64/include/asm/topology.h |  4 ++--
 arch/arm64/kernel/topology.c      | 27 ++++++++++++++-------------
 2 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
index c4f2d50491eb..118136268f66 100644
--- a/arch/arm64/include/asm/topology.h
+++ b/arch/arm64/include/asm/topology.h
@@ -7,14 +7,14 @@
 struct cpu_topology {
 	int thread_id;
 	int core_id;
-	int cluster_id;
+	int physical_id;
 	cpumask_t thread_sibling;
 	cpumask_t core_sibling;
 };
 
 extern struct cpu_topology cpu_topology[NR_CPUS];
 
-#define topology_physical_package_id(cpu)	(cpu_topology[cpu].cluster_id)
+#define topology_physical_package_id(cpu)	(cpu_topology[cpu].physical_id)
 #define topology_core_id(cpu)		(cpu_topology[cpu].core_id)
 #define topology_core_cpumask(cpu)	(&cpu_topology[cpu].core_sibling)
 #define topology_sibling_cpumask(cpu)	(&cpu_topology[cpu].thread_sibling)
diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index 8d48b233e6ce..74a8a5173a35 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -51,7 +51,7 @@ static int __init get_cpu_for_node(struct device_node *node)
 	return -1;
 }
 
-static int __init parse_core(struct device_node *core, int cluster_id,
+static int __init parse_core(struct device_node *core, int physical_id,
 			     int core_id)
 {
 	char name[10];
@@ -67,7 +67,7 @@ static int __init parse_core(struct device_node *core, int cluster_id,
 			leaf = false;
 			cpu = get_cpu_for_node(t);
 			if (cpu >= 0) {
-				cpu_topology[cpu].cluster_id = cluster_id;
+				cpu_topology[cpu].physical_id = physical_id;
 				cpu_topology[cpu].core_id = core_id;
 				cpu_topology[cpu].thread_id = i;
 			} else {
@@ -89,7 +89,7 @@ static int __init parse_core(struct device_node *core, int cluster_id,
 			return -EINVAL;
 		}
 
-		cpu_topology[cpu].cluster_id = cluster_id;
+		cpu_topology[cpu].physical_id = physical_id;
 		cpu_topology[cpu].core_id = core_id;
 	} else if (leaf) {
 		pr_err("%pOF: Can't get CPU for leaf core\n", core);
@@ -105,7 +105,7 @@ static int __init parse_cluster(struct device_node *cluster, int depth)
 	bool leaf = true;
 	bool has_cores = false;
 	struct device_node *c;
-	static int cluster_id __initdata;
+	static int physical_id __initdata;
 	int core_id = 0;
 	int i, ret;
 
@@ -144,7 +144,7 @@ static int __init parse_cluster(struct device_node *cluster, int depth)
 			}
 
 			if (leaf) {
-				ret = parse_core(c, cluster_id, core_id++);
+				ret = parse_core(c, physical_id, core_id++);
 			} else {
 				pr_err("%pOF: Non-leaf cluster with core %s\n",
 				       cluster, name);
@@ -162,7 +162,7 @@ static int __init parse_cluster(struct device_node *cluster, int depth)
 		pr_warn("%pOF: empty cluster\n", cluster);
 
 	if (leaf)
-		cluster_id++;
+		physical_id++;
 
 	return 0;
 }
@@ -198,7 +198,7 @@ static int __init parse_dt_topology(void)
 	 * only mark cores described in the DT as possible.
 	 */
 	for_each_possible_cpu(cpu)
-		if (cpu_topology[cpu].cluster_id == -1)
+		if (cpu_topology[cpu].physical_id == -1)
 			ret = -EINVAL;
 
 out_map:
@@ -228,7 +228,7 @@ static void update_siblings_masks(unsigned int cpuid)
 	for_each_possible_cpu(cpu) {
 		cpu_topo = &cpu_topology[cpu];
 
-		if (cpuid_topo->cluster_id != cpu_topo->cluster_id)
+		if (cpuid_topo->physical_id != cpu_topo->physical_id)
 			continue;
 
 		cpumask_set_cpu(cpuid, &cpu_topo->core_sibling);
@@ -249,7 +249,7 @@ void store_cpu_topology(unsigned int cpuid)
 	struct cpu_topology *cpuid_topo = &cpu_topology[cpuid];
 	u64 mpidr;
 
-	if (cpuid_topo->cluster_id != -1)
+	if (cpuid_topo->physical_id != -1)
 		goto topology_populated;
 
 	mpidr = read_cpuid_mpidr();
@@ -263,19 +263,19 @@ void store_cpu_topology(unsigned int cpuid)
 		/* Multiprocessor system : Multi-threads per core */
 		cpuid_topo->thread_id  = MPIDR_AFFINITY_LEVEL(mpidr, 0);
 		cpuid_topo->core_id    = MPIDR_AFFINITY_LEVEL(mpidr, 1);
-		cpuid_topo->cluster_id = MPIDR_AFFINITY_LEVEL(mpidr, 2) |
+		cpuid_topo->physical_id = MPIDR_AFFINITY_LEVEL(mpidr, 2) |
 					 MPIDR_AFFINITY_LEVEL(mpidr, 3) << 8;
 	} else {
 		/* Multiprocessor system : Single-thread per core */
 		cpuid_topo->thread_id  = -1;
 		cpuid_topo->core_id    = MPIDR_AFFINITY_LEVEL(mpidr, 0);
-		cpuid_topo->cluster_id = MPIDR_AFFINITY_LEVEL(mpidr, 1) |
+		cpuid_topo->physical_id = MPIDR_AFFINITY_LEVEL(mpidr, 1) |
 					 MPIDR_AFFINITY_LEVEL(mpidr, 2) << 8 |
 					 MPIDR_AFFINITY_LEVEL(mpidr, 3) << 16;
 	}
 
 	pr_debug("CPU%u: cluster %d core %d thread %d mpidr %#016llx\n",
-		 cpuid, cpuid_topo->cluster_id, cpuid_topo->core_id,
+		 cpuid, cpuid_topo->physical_id, cpuid_topo->core_id,
 		 cpuid_topo->thread_id, mpidr);
 
 topology_populated:
@@ -291,7 +291,7 @@ static void __init reset_cpu_topology(void)
 
 		cpu_topo->thread_id = -1;
 		cpu_topo->core_id = 0;
-		cpu_topo->cluster_id = -1;
+		cpu_topo->physical_id = -1;
 
 		cpumask_clear(&cpu_topo->core_sibling);
 		cpumask_set_cpu(cpu, &cpu_topo->core_sibling);
@@ -300,6 +300,7 @@ static void __init reset_cpu_topology(void)
 	}
 }
 
+
 void __init init_cpu_topology(void)
 {
 	reset_cpu_topology();
-- 
2.13.5

[PATCH v5 8/9] arm64: topology: Enable ACPI/PPTT based CPU topology.

[Jeremy Linton]

Propagate the topology information from the PPTT tree to the
cpu_topology array. We can get the thread id, core_id and
cluster_id by assuming certain levels of the PPTT tree correspond
to those concepts. The package_id is flagged in the tree and can be
found by calling find_acpi_cpu_topology_package() which terminates
its search when it finds an ACPI node flagged as the physical
package. If the tree doesn't contain enough levels to represent
all of the requested levels then the root node will be returned
for all subsequent levels.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 arch/arm64/kernel/topology.c | 47 +++++++++++++++++++++++++++++++++++++++++++-
 include/linux/topology.h     |  2 ++
 2 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index 74a8a5173a35..198714aca9e8 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -11,6 +11,7 @@
  * for more details.
  */
 
+#include <linux/acpi.h>
 #include <linux/arch_topology.h>
 #include <linux/cpu.h>
 #include <linux/cpumask.h>
@@ -22,6 +23,7 @@
 #include <linux/sched.h>
 #include <linux/sched/topology.h>
 #include <linux/slab.h>
+#include <linux/smp.h>
 #include <linux/string.h>
 
 #include <asm/cpu.h>
@@ -300,6 +302,47 @@ static void __init reset_cpu_topology(void)
 	}
 }
 
+#ifdef CONFIG_ACPI
+/*
+ * Propagate the topology information of the processor_topology_node tree to the
+ * cpu_topology array.
+ */
+static int __init parse_acpi_topology(void)
+{
+	u64 is_threaded;
+	int cpu;
+	int topology_id;
+
+	is_threaded = read_cpuid_mpidr() & MPIDR_MT_BITMASK;
+
+	for_each_possible_cpu(cpu) {
+		topology_id = find_acpi_cpu_topology(cpu, 0);
+		if (topology_id < 0)
+			return topology_id;
+
+		if (is_threaded) {
+			cpu_topology[cpu].thread_id = topology_id;
+			topology_id = find_acpi_cpu_topology(cpu, 1);
+			cpu_topology[cpu].core_id   = topology_id;
+			topology_id = find_acpi_cpu_topology_package(cpu);
+			cpu_topology[cpu].physical_id = topology_id;
+		} else {
+			cpu_topology[cpu].thread_id  = -1;
+			cpu_topology[cpu].core_id    = topology_id;
+			topology_id = find_acpi_cpu_topology_package(cpu);
+			cpu_topology[cpu].physical_id = topology_id;
+		}
+	}
+	return 0;
+}
+
+#else
+static int __init parse_acpi_topology(void)
+{
+	/*ACPI kernels should be built with PPTT support*/
+	return -EINVAL;
+}
+#endif
 
 void __init init_cpu_topology(void)
 {
@@ -309,6 +352,8 @@ void __init init_cpu_topology(void)
 	 * Discard anything that was parsed if we hit an error so we
 	 * don't use partial information.
 	 */
-	if (of_have_populated_dt() && parse_dt_topology())
+	if ((!acpi_disabled) && parse_acpi_topology())
+		reset_cpu_topology();
+	else if (of_have_populated_dt() && parse_dt_topology())
 		reset_cpu_topology();
 }
diff --git a/include/linux/topology.h b/include/linux/topology.h
index cb0775e1ee4b..170ce87edd88 100644
--- a/include/linux/topology.h
+++ b/include/linux/topology.h
@@ -43,6 +43,8 @@
 		if (nr_cpus_node(node))
 
 int arch_update_cpu_topology(void);
+int find_acpi_cpu_topology(unsigned int cpu, int level);
+int find_acpi_cpu_topology_package(unsigned int cpu);
 
 /* Conform to ACPI 2.0 SLIT distance definitions */
 #define LOCAL_DISTANCE		10
-- 
2.13.5

[PATCH v5 9/9] ACPI: Add PPTT to injectable table list

[Jeremy Linton]

Add ACPI_SIG_PPTT to the table so initrd's can override the
system topology.

Signed-off-by: Geoffrey Blake <geoffrey.blake@arm.com>
Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 drivers/acpi/tables.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 80ce2a7d224b..6d254450115b 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -456,7 +456,8 @@ static const char * const table_sigs[] = {
 	ACPI_SIG_SLIC, ACPI_SIG_SPCR, ACPI_SIG_SPMI, ACPI_SIG_TCPA,
 	ACPI_SIG_UEFI, ACPI_SIG_WAET, ACPI_SIG_WDAT, ACPI_SIG_WDDT,
 	ACPI_SIG_WDRT, ACPI_SIG_DSDT, ACPI_SIG_FADT, ACPI_SIG_PSDT,
-	ACPI_SIG_RSDT, ACPI_SIG_XSDT, ACPI_SIG_SSDT, NULL };
+	ACPI_SIG_RSDT, ACPI_SIG_XSDT, ACPI_SIG_SSDT, ACPI_SIG_PPTT,
+	NULL };
 
 #define ACPI_HEADER_SIZE sizeof(struct acpi_table_header)
 
-- 
2.13.5

Re: [PATCH v5 2/9] ACPI/PPTT: Add Processor Properties Topology Table parsing

[Rafael J. Wysocki]

On Friday, December 1, 2017 11:23:23 PM CET Jeremy Linton wrote:
> ACPI 6.2 adds a new table, which describes how processing units
> are related to each other in tree like fashion. Caches are
> also sprinkled throughout the tree and describe the properties
> of the caches in relation to other caches and processing units.
> 
> Add the code to parse the cache hierarchy and report the total
> number of levels of cache for a given core using
> acpi_find_last_cache_level() as well as fill out the individual
> cores cache information with cache_setup_acpi() once the
> cpu_cacheinfo structure has been populated by the arch specific
> code.
> 
> An additional patch later in the set adds the ability to report
> peers in the topology using find_acpi_cpu_topology()
> to report a unique ID for each processing unit at a given level
> in the tree. These unique id's can then be used to match related
> processing units which exist as threads, COD (clusters
> on die), within a given package, etc.
> 
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

This is only going to be used by ARM64 for the time being, so I need
someone from that camp to review this.

Sudeep, Hanjun, Lorenzo?

Thanks,
Rafael

Re: [PATCH v5 4/9] drivers: base cacheinfo: Add support for ACPI based firmware tables

[Rafael J. Wysocki]

On Friday, December 1, 2017 11:23:25 PM CET Jeremy Linton wrote:
> Add a entry to to struct cacheinfo to maintain a reference to the PPTT
> node which can be used to match identical caches across cores. Also
> stub out cache_setup_acpi() so that individual architectures can
> enable ACPI topology parsing.
> 
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
>  drivers/acpi/pptt.c       |  1 +
>  drivers/base/cacheinfo.c  | 20 ++++++++++++++------
>  include/linux/cacheinfo.h | 13 ++++++++++++-
>  3 files changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
> index 0f8a1631af33..a35e457cefb7 100644
> --- a/drivers/acpi/pptt.c
> +++ b/drivers/acpi/pptt.c
> @@ -329,6 +329,7 @@ static void update_cache_properties(struct cacheinfo *this_leaf,
>  {
>  	int valid_flags = 0;
>  
> +	this_leaf->firmware_node = cpu_node;
>  	if (found_cache->flags & ACPI_PPTT_SIZE_PROPERTY_VALID) {
>  		this_leaf->size = found_cache->size;
>  		valid_flags++;
> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
> index eb3af2739537..ba89f9310e6f 100644
> --- a/drivers/base/cacheinfo.c
> +++ b/drivers/base/cacheinfo.c
> @@ -86,7 +86,10 @@ static int cache_setup_of_node(unsigned int cpu)
>  static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf,
>  					   struct cacheinfo *sib_leaf)
>  {
> -	return sib_leaf->of_node == this_leaf->of_node;
> +	if (acpi_disabled)
> +		return sib_leaf->of_node == this_leaf->of_node;
> +	else
> +		return sib_leaf->firmware_node == this_leaf->firmware_node;
>  }
>  
>  /* OF properties to query for a given cache type */
> @@ -215,6 +218,11 @@ static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf,
>  }
>  #endif
>  
> +int __weak cache_setup_acpi(unsigned int cpu)
> +{
> +	return -ENOTSUPP;
> +}
> +
>  static int cache_shared_cpu_map_setup(unsigned int cpu)
>  {
>  	struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
> @@ -225,11 +233,11 @@ static int cache_shared_cpu_map_setup(unsigned int cpu)
>  	if (this_cpu_ci->cpu_map_populated)
>  		return 0;
>  
> -	if (of_have_populated_dt())
> +	if (!acpi_disabled)
> +		ret = cache_setup_acpi(cpu);
> +	else if (of_have_populated_dt())
>  		ret = cache_setup_of_node(cpu);
> -	else if (!acpi_disabled)
> -		/* No cache property/hierarchy support yet in ACPI */
> -		ret = -ENOTSUPP;
> +
>  	if (ret)
>  		return ret;
>  
> @@ -286,7 +294,7 @@ static void cache_shared_cpu_map_remove(unsigned int cpu)
>  
>  static void cache_override_properties(unsigned int cpu)
>  {
> -	if (of_have_populated_dt())
> +	if (acpi_disabled && of_have_populated_dt())
>  		return cache_of_override_properties(cpu);
>  }
>  
> diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
> index 3d9805297cda..7ebff157ae6c 100644
> --- a/include/linux/cacheinfo.h
> +++ b/include/linux/cacheinfo.h
> @@ -37,6 +37,8 @@ enum cache_type {
>   * @of_node: if devicetree is used, this represents either the cpu node in
>   *	case there's no explicit cache node or the cache node itself in the
>   *	device tree
> + * @firmware_node: When not using DT, this may contain pointers to other
> + *	firmware based values. Particularly ACPI/PPTT unique values.
>   * @disable_sysfs: indicates whether this node is visible to the user via
>   *	sysfs or not
>   * @priv: pointer to any private data structure specific to particular
> @@ -65,8 +67,8 @@ struct cacheinfo {
>  #define CACHE_ALLOCATE_POLICY_MASK	\
>  	(CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE)
>  #define CACHE_ID		BIT(4)
> -
>  	struct device_node *of_node;
> +	void *firmware_node;

What about converting this to using struct fwnode instead of adding
fields to it?

>  	bool disable_sysfs;
>  	void *priv;
>  };
> @@ -99,6 +101,15 @@ int func(unsigned int cpu)					\
>  struct cpu_cacheinfo *get_cpu_cacheinfo(unsigned int cpu);
>  int init_cache_level(unsigned int cpu);
>  int populate_cache_leaves(unsigned int cpu);
> +int cache_setup_acpi(unsigned int cpu);
> +int acpi_find_last_cache_level(unsigned int cpu);
> +#ifndef CONFIG_ACPI
> +int acpi_find_last_cache_level(unsigned int cpu)
> +{
> +	/*ACPI kernels should be built with PPTT support*/
> +	return 0;
> +}
> +#endif
>  
>  const struct attribute_group *cache_get_priv_group(struct cacheinfo *this_leaf);
>  
> 

Thanks,
Rafael

Re: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code

[Rafael J. Wysocki]

On Friday, December 1, 2017 11:23:27 PM CET Jeremy Linton wrote:
> The PPTT can be used to determine the groupings of CPU's at
> given levels in the system. Lets add a few routines to the PPTT
> parsing code to return a unique id for each unique level in the
> processor hierarchy. This can then be matched to build
> thread/core/cluster/die/package/etc mappings for each processing
> element in the system.
> 
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>

Why can't this be folded into patch [2/9]?

Thanks,
Rafael

Re: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code

[Jeremy Linton]

Hi,

First, thanks for taking a look at this.

On 12/11/2017 07:12 PM, Rafael J. Wysocki wrote:
> On Friday, December 1, 2017 11:23:27 PM CET Jeremy Linton wrote:
>> The PPTT can be used to determine the groupings of CPU's at
>> given levels in the system. Lets add a few routines to the PPTT
>> parsing code to return a unique id for each unique level in the
>> processor hierarchy. This can then be matched to build
>> thread/core/cluster/die/package/etc mappings for each processing
>> element in the system.
>>
>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> 
> Why can't this be folded into patch [2/9]?

It can, and I will be happy squash it.

It was requested that the topology portion of the parser be split out 
back in v3.

https://www.spinics.net/lists/linux-acpi/msg78487.html

Re: [PATCH v5 4/9] drivers: base cacheinfo: Add support for ACPI based firmware tables

[Jeremy Linton]

Hi,

On 12/11/2017 07:11 PM, Rafael J. Wysocki wrote:
> On Friday, December 1, 2017 11:23:25 PM CET Jeremy Linton wrote:
>> Add a entry to to struct cacheinfo to maintain a reference to the PPTT
>> node which can be used to match identical caches across cores. Also
>> stub out cache_setup_acpi() so that individual architectures can
>> enable ACPI topology parsing.
>>
>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>> ---
>>   drivers/acpi/pptt.c       |  1 +
>>   drivers/base/cacheinfo.c  | 20 ++++++++++++++------
>>   include/linux/cacheinfo.h | 13 ++++++++++++-
>>   3 files changed, 27 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
>> index 0f8a1631af33..a35e457cefb7 100644
>> --- a/drivers/acpi/pptt.c
>> +++ b/drivers/acpi/pptt.c
>> @@ -329,6 +329,7 @@ static void update_cache_properties(struct cacheinfo *this_leaf,
>>   {
>>   	int valid_flags = 0;
>>   
>> +	this_leaf->firmware_node = cpu_node;
>>   	if (found_cache->flags & ACPI_PPTT_SIZE_PROPERTY_VALID) {
>>   		this_leaf->size = found_cache->size;
>>   		valid_flags++;
>> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
>> index eb3af2739537..ba89f9310e6f 100644
>> --- a/drivers/base/cacheinfo.c
>> +++ b/drivers/base/cacheinfo.c
>> @@ -86,7 +86,10 @@ static int cache_setup_of_node(unsigned int cpu)
>>   static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf,
>>   					   struct cacheinfo *sib_leaf)
>>   {
>> -	return sib_leaf->of_node == this_leaf->of_node;
>> +	if (acpi_disabled)
>> +		return sib_leaf->of_node == this_leaf->of_node;
>> +	else
>> +		return sib_leaf->firmware_node == this_leaf->firmware_node;
>>   }
>>   
>>   /* OF properties to query for a given cache type */
>> @@ -215,6 +218,11 @@ static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf,
>>   }
>>   #endif
>>   
>> +int __weak cache_setup_acpi(unsigned int cpu)
>> +{
>> +	return -ENOTSUPP;
>> +}
>> +
>>   static int cache_shared_cpu_map_setup(unsigned int cpu)
>>   {
>>   	struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
>> @@ -225,11 +233,11 @@ static int cache_shared_cpu_map_setup(unsigned int cpu)
>>   	if (this_cpu_ci->cpu_map_populated)
>>   		return 0;
>>   
>> -	if (of_have_populated_dt())
>> +	if (!acpi_disabled)
>> +		ret = cache_setup_acpi(cpu);
>> +	else if (of_have_populated_dt())
>>   		ret = cache_setup_of_node(cpu);
>> -	else if (!acpi_disabled)
>> -		/* No cache property/hierarchy support yet in ACPI */
>> -		ret = -ENOTSUPP;
>> +
>>   	if (ret)
>>   		return ret;
>>   
>> @@ -286,7 +294,7 @@ static void cache_shared_cpu_map_remove(unsigned int cpu)
>>   
>>   static void cache_override_properties(unsigned int cpu)
>>   {
>> -	if (of_have_populated_dt())
>> +	if (acpi_disabled && of_have_populated_dt())
>>   		return cache_of_override_properties(cpu);
>>   }
>>   
>> diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
>> index 3d9805297cda..7ebff157ae6c 100644
>> --- a/include/linux/cacheinfo.h
>> +++ b/include/linux/cacheinfo.h
>> @@ -37,6 +37,8 @@ enum cache_type {
>>    * @of_node: if devicetree is used, this represents either the cpu node in
>>    *	case there's no explicit cache node or the cache node itself in the
>>    *	device tree
>> + * @firmware_node: When not using DT, this may contain pointers to other
>> + *	firmware based values. Particularly ACPI/PPTT unique values.
>>    * @disable_sysfs: indicates whether this node is visible to the user via
>>    *	sysfs or not
>>    * @priv: pointer to any private data structure specific to particular
>> @@ -65,8 +67,8 @@ struct cacheinfo {
>>   #define CACHE_ALLOCATE_POLICY_MASK	\
>>   	(CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE)
>>   #define CACHE_ID		BIT(4)
>> -
>>   	struct device_node *of_node;
>> +	void *firmware_node;
> 
> What about converting this to using struct fwnode instead of adding
> fields to it?

I didn't really want to add another field here, but I've also pointed 
out how I thought converting it to a fwnode wasn't a good choice.

https://lkml.org/lkml/2017/11/20/502

Mostly because IMHO its even more misleading (lacking any 
fwnode_operations) than misusing the of_node as a void *.

Given that I'm in the minority thinking this, how far down the fwnode 
path on the ACPI side do we want to go? Is simply treating it as a void 
pointer sufficient for the ACPI side, considering all the PPTT code 
needs is a unique token?


> 
>>   	bool disable_sysfs;
>>   	void *priv;
>>   };
>> @@ -99,6 +101,15 @@ int func(unsigned int cpu)					\
>>   struct cpu_cacheinfo *get_cpu_cacheinfo(unsigned int cpu);
>>   int init_cache_level(unsigned int cpu);
>>   int populate_cache_leaves(unsigned int cpu);
>> +int cache_setup_acpi(unsigned int cpu);
>> +int acpi_find_last_cache_level(unsigned int cpu);
>> +#ifndef CONFIG_ACPI
>> +int acpi_find_last_cache_level(unsigned int cpu)
>> +{
>> +	/*ACPI kernels should be built with PPTT support*/
>> +	return 0;
>> +}
>> +#endif
>>   
>>   const struct attribute_group *cache_get_priv_group(struct cacheinfo *this_leaf);
>>   
>>
> 
> Thanks,
> Rafael
> 

Re: [PATCH v5 4/9] drivers: base cacheinfo: Add support for ACPI based firmware tables

[Rafael J. Wysocki]

On Tue, Dec 12, 2017 at 6:03 PM, Jeremy Linton <jeremy.linton@arm.com> wrote:
> Hi,
>
>
> On 12/11/2017 07:11 PM, Rafael J. Wysocki wrote:
>>
>> On Friday, December 1, 2017 11:23:25 PM CET Jeremy Linton wrote:
>>>
>>> Add a entry to to struct cacheinfo to maintain a reference to the PPTT
>>> node which can be used to match identical caches across cores. Also
>>> stub out cache_setup_acpi() so that individual architectures can
>>> enable ACPI topology parsing.
>>>
>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>>> ---
>>>   drivers/acpi/pptt.c       |  1 +
>>>   drivers/base/cacheinfo.c  | 20 ++++++++++++++------
>>>   include/linux/cacheinfo.h | 13 ++++++++++++-
>>>   3 files changed, 27 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
>>> index 0f8a1631af33..a35e457cefb7 100644
>>> --- a/drivers/acpi/pptt.c
>>> +++ b/drivers/acpi/pptt.c
>>> @@ -329,6 +329,7 @@ static void update_cache_properties(struct cacheinfo
>>> *this_leaf,
>>>   {
>>>         int valid_flags = 0;
>>>   +     this_leaf->firmware_node = cpu_node;
>>>         if (found_cache->flags & ACPI_PPTT_SIZE_PROPERTY_VALID) {
>>>                 this_leaf->size = found_cache->size;
>>>                 valid_flags++;
>>> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
>>> index eb3af2739537..ba89f9310e6f 100644
>>> --- a/drivers/base/cacheinfo.c
>>> +++ b/drivers/base/cacheinfo.c
>>> @@ -86,7 +86,10 @@ static int cache_setup_of_node(unsigned int cpu)
>>>   static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf,
>>>                                            struct cacheinfo *sib_leaf)
>>>   {
>>> -       return sib_leaf->of_node == this_leaf->of_node;
>>> +       if (acpi_disabled)
>>> +               return sib_leaf->of_node == this_leaf->of_node;
>>> +       else
>>> +               return sib_leaf->firmware_node ==
>>> this_leaf->firmware_node;
>>>   }
>>>     /* OF properties to query for a given cache type */
>>> @@ -215,6 +218,11 @@ static inline bool cache_leaves_are_shared(struct
>>> cacheinfo *this_leaf,
>>>   }
>>>   #endif
>>>   +int __weak cache_setup_acpi(unsigned int cpu)
>>> +{
>>> +       return -ENOTSUPP;
>>> +}
>>> +
>>>   static int cache_shared_cpu_map_setup(unsigned int cpu)
>>>   {
>>>         struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
>>> @@ -225,11 +233,11 @@ static int cache_shared_cpu_map_setup(unsigned int
>>> cpu)
>>>         if (this_cpu_ci->cpu_map_populated)
>>>                 return 0;
>>>   -     if (of_have_populated_dt())
>>> +       if (!acpi_disabled)
>>> +               ret = cache_setup_acpi(cpu);
>>> +       else if (of_have_populated_dt())
>>>                 ret = cache_setup_of_node(cpu);
>>> -       else if (!acpi_disabled)
>>> -               /* No cache property/hierarchy support yet in ACPI */
>>> -               ret = -ENOTSUPP;
>>> +
>>>         if (ret)
>>>                 return ret;
>>>   @@ -286,7 +294,7 @@ static void cache_shared_cpu_map_remove(unsigned
>>> int cpu)
>>>     static void cache_override_properties(unsigned int cpu)
>>>   {
>>> -       if (of_have_populated_dt())
>>> +       if (acpi_disabled && of_have_populated_dt())
>>>                 return cache_of_override_properties(cpu);
>>>   }
>>>   diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
>>> index 3d9805297cda..7ebff157ae6c 100644
>>> --- a/include/linux/cacheinfo.h
>>> +++ b/include/linux/cacheinfo.h
>>> @@ -37,6 +37,8 @@ enum cache_type {
>>>    * @of_node: if devicetree is used, this represents either the cpu node
>>> in
>>>    *    case there's no explicit cache node or the cache node itself in
>>> the
>>>    *    device tree
>>> + * @firmware_node: When not using DT, this may contain pointers to other
>>> + *     firmware based values. Particularly ACPI/PPTT unique values.
>>>    * @disable_sysfs: indicates whether this node is visible to the user
>>> via
>>>    *    sysfs or not
>>>    * @priv: pointer to any private data structure specific to particular
>>> @@ -65,8 +67,8 @@ struct cacheinfo {
>>>   #define CACHE_ALLOCATE_POLICY_MASK    \
>>>         (CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE)
>>>   #define CACHE_ID              BIT(4)
>>> -
>>>         struct device_node *of_node;
>>> +       void *firmware_node;
>>
>>
>> What about converting this to using struct fwnode instead of adding
>> fields to it?
>
>
> I didn't really want to add another field here, but I've also pointed out
> how I thought converting it to a fwnode wasn't a good choice.
>
> https://lkml.org/lkml/2017/11/20/502
>
> Mostly because IMHO its even more misleading (lacking any fwnode_operations)
> than misusing the of_node as a void *.

I'm not sure what you mean.

Anyway, the idea is to have one pointer in there instead of two that
cannot be used at the same time and there's no reason why of_node
should be special.

of_node should just be one of multiple choices.

> Given that I'm in the minority thinking this, how far down the fwnode path
> on the ACPI side do we want to go?

I have no idea. :-)

> Is simply treating it as a void pointer
> sufficient for the ACPI side, considering all the PPTT code needs is a
> unique token?

I guess you can think about it as of_node under a different name, but
whether or not this is sufficient depends on what you need it for.

Thanks,
Rafael

Re: [PATCH v5 4/9] drivers: base cacheinfo: Add support for ACPI based firmware tables

[Jeremy Linton]

Hi,

On 12/12/2017 11:25 AM, Rafael J. Wysocki wrote:
> On Tue, Dec 12, 2017 at 6:03 PM, Jeremy Linton <jeremy.linton@arm.com> wrote:
>> Hi,
>>
>>
>> On 12/11/2017 07:11 PM, Rafael J. Wysocki wrote:
>>>
>>> On Friday, December 1, 2017 11:23:25 PM CET Jeremy Linton wrote:
>>>>
>>>> Add a entry to to struct cacheinfo to maintain a reference to the PPTT
>>>> node which can be used to match identical caches across cores. Also
>>>> stub out cache_setup_acpi() so that individual architectures can
>>>> enable ACPI topology parsing.
>>>>
>>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>>>> ---
>>>>    drivers/acpi/pptt.c       |  1 +
>>>>    drivers/base/cacheinfo.c  | 20 ++++++++++++++------
>>>>    include/linux/cacheinfo.h | 13 ++++++++++++-
>>>>    3 files changed, 27 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
>>>> index 0f8a1631af33..a35e457cefb7 100644
>>>> --- a/drivers/acpi/pptt.c
>>>> +++ b/drivers/acpi/pptt.c
>>>> @@ -329,6 +329,7 @@ static void update_cache_properties(struct cacheinfo
>>>> *this_leaf,
>>>>    {
>>>>          int valid_flags = 0;
>>>>    +     this_leaf->firmware_node = cpu_node;
>>>>          if (found_cache->flags & ACPI_PPTT_SIZE_PROPERTY_VALID) {
>>>>                  this_leaf->size = found_cache->size;
>>>>                  valid_flags++;
>>>> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c
>>>> index eb3af2739537..ba89f9310e6f 100644
>>>> --- a/drivers/base/cacheinfo.c
>>>> +++ b/drivers/base/cacheinfo.c
>>>> @@ -86,7 +86,10 @@ static int cache_setup_of_node(unsigned int cpu)
>>>>    static inline bool cache_leaves_are_shared(struct cacheinfo *this_leaf,
>>>>                                             struct cacheinfo *sib_leaf)
>>>>    {
>>>> -       return sib_leaf->of_node == this_leaf->of_node;
>>>> +       if (acpi_disabled)
>>>> +               return sib_leaf->of_node == this_leaf->of_node;
>>>> +       else
>>>> +               return sib_leaf->firmware_node ==
>>>> this_leaf->firmware_node;
>>>>    }
>>>>      /* OF properties to query for a given cache type */
>>>> @@ -215,6 +218,11 @@ static inline bool cache_leaves_are_shared(struct
>>>> cacheinfo *this_leaf,
>>>>    }
>>>>    #endif
>>>>    +int __weak cache_setup_acpi(unsigned int cpu)
>>>> +{
>>>> +       return -ENOTSUPP;
>>>> +}
>>>> +
>>>>    static int cache_shared_cpu_map_setup(unsigned int cpu)
>>>>    {
>>>>          struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
>>>> @@ -225,11 +233,11 @@ static int cache_shared_cpu_map_setup(unsigned int
>>>> cpu)
>>>>          if (this_cpu_ci->cpu_map_populated)
>>>>                  return 0;
>>>>    -     if (of_have_populated_dt())
>>>> +       if (!acpi_disabled)
>>>> +               ret = cache_setup_acpi(cpu);
>>>> +       else if (of_have_populated_dt())
>>>>                  ret = cache_setup_of_node(cpu);
>>>> -       else if (!acpi_disabled)
>>>> -               /* No cache property/hierarchy support yet in ACPI */
>>>> -               ret = -ENOTSUPP;
>>>> +
>>>>          if (ret)
>>>>                  return ret;
>>>>    @@ -286,7 +294,7 @@ static void cache_shared_cpu_map_remove(unsigned
>>>> int cpu)
>>>>      static void cache_override_properties(unsigned int cpu)
>>>>    {
>>>> -       if (of_have_populated_dt())
>>>> +       if (acpi_disabled && of_have_populated_dt())
>>>>                  return cache_of_override_properties(cpu);
>>>>    }
>>>>    diff --git a/include/linux/cacheinfo.h b/include/linux/cacheinfo.h
>>>> index 3d9805297cda..7ebff157ae6c 100644
>>>> --- a/include/linux/cacheinfo.h
>>>> +++ b/include/linux/cacheinfo.h
>>>> @@ -37,6 +37,8 @@ enum cache_type {
>>>>     * @of_node: if devicetree is used, this represents either the cpu node
>>>> in
>>>>     *    case there's no explicit cache node or the cache node itself in
>>>> the
>>>>     *    device tree
>>>> + * @firmware_node: When not using DT, this may contain pointers to other
>>>> + *     firmware based values. Particularly ACPI/PPTT unique values.
>>>>     * @disable_sysfs: indicates whether this node is visible to the user
>>>> via
>>>>     *    sysfs or not
>>>>     * @priv: pointer to any private data structure specific to particular
>>>> @@ -65,8 +67,8 @@ struct cacheinfo {
>>>>    #define CACHE_ALLOCATE_POLICY_MASK    \
>>>>          (CACHE_READ_ALLOCATE | CACHE_WRITE_ALLOCATE)
>>>>    #define CACHE_ID              BIT(4)
>>>> -
>>>>          struct device_node *of_node;
>>>> +       void *firmware_node;
>>>
>>>
>>> What about converting this to using struct fwnode instead of adding
>>> fields to it?
>>
>>
>> I didn't really want to add another field here, but I've also pointed out
>> how I thought converting it to a fwnode wasn't a good choice.
>>
>> https://lkml.org/lkml/2017/11/20/502
>>
>> Mostly because IMHO its even more misleading (lacking any fwnode_operations)
>> than misusing the of_node as a void *.
> 
> I'm not sure what you mean.

Converting the DT drivers/cacheinfo.c code to use a fwnode_handle is 
straightforward. But IMHO it doesn't solve the readability problem of 
either casting the ACPI/PPTT token directly to the resulting 
fwnode_handle *, or alternatively an actual fwnode_handle with bogus 
fwnode_operations to wrap that token.


> 
> Anyway, the idea is to have one pointer in there instead of two that
> cannot be used at the same time and there's no reason why of_node
> should be special.

	Avoid two pointers for size, or readability? Because the last version 
had a union with of_node, which isn't strictly necessary as I can just 
cast the pptt token to of_node. There is exactly one line of code after 
that which uses the token and it doesn't care about type.

(in cache_leaves_are_shared())

> 
> of_node should just be one of multiple choices.
> 
>> Given that I'm in the minority thinking this, how far down the fwnode path
>> on the ACPI side do we want to go?
> 
> I have no idea. :-)
> 
>> Is simply treating it as a void pointer
>> sufficient for the ACPI side, considering all the PPTT code needs is a
>> unique token?
> 
> I guess you can think about it as of_node under a different name, but
> whether or not this is sufficient depends on what you need it for.

> 
> Thanks,
> Rafael
> 

Re: [PATCH v5 4/9] drivers: base cacheinfo: Add support for ACPI based firmware tables

[Rafael J. Wysocki]

On Tue, Dec 12, 2017 at 11:55 PM, Jeremy Linton <jeremy.linton@arm.com> wrote:
> Hi,
>
>
> On 12/12/2017 11:25 AM, Rafael J. Wysocki wrote:
>>

[cut]

>>>>
>>>>
>>>>
>>>> What about converting this to using struct fwnode instead of adding
>>>> fields to it?
>>>
>>>
>>>
>>> I didn't really want to add another field here, but I've also pointed out
>>> how I thought converting it to a fwnode wasn't a good choice.
>>>
>>> https://lkml.org/lkml/2017/11/20/502
>>>
>>> Mostly because IMHO its even more misleading (lacking any
>>> fwnode_operations)
>>> than misusing the of_node as a void *.
>>
>>
>> I'm not sure what you mean.
>
>
> Converting the DT drivers/cacheinfo.c code to use a fwnode_handle is
> straightforward. But IMHO it doesn't solve the readability problem of either
> casting the ACPI/PPTT token directly to the resulting fwnode_handle *, or
> alternatively an actual fwnode_handle with bogus fwnode_operations to wrap
> that token.

I'm not talking about that at all.

>>
>> Anyway, the idea is to have one pointer in there instead of two that
>> cannot be used at the same time and there's no reason why of_node
>> should be special.
>
>
>         Avoid two pointers for size, or readability? Because the last
> version had a union with of_node, which isn't strictly necessary as I can
> just cast the pptt token to of_node. There is exactly one line of code after
> that which uses the token and it doesn't care about type.

So call this field "token" or similar.  Don't call it "of_node" and
don't introduce another "firmware_node" thing in addition to that.
That just is a mess, sorry.

Thanks,
Rafael

Re: [PATCH v5 4/9] drivers: base cacheinfo: Add support for ACPI based firmware tables

[Jeremy Linton]

On 12/12/2017 05:02 PM, Rafael J. Wysocki wrote:
> On Tue, Dec 12, 2017 at 11:55 PM, Jeremy Linton <jeremy.linton@arm.com> wrote:
>> Hi,
>>
>>
>> On 12/12/2017 11:25 AM, Rafael J. Wysocki wrote:
>>>
> 
> [cut]

(trimming list)

> 
>>>>>
>>>>>
>>>>>
>>>>> What about converting this to using struct fwnode instead of adding
>>>>> fields to it?
>>>>
>>>>
>>>>
>>>> I didn't really want to add another field here, but I've also pointed out
>>>> how I thought converting it to a fwnode wasn't a good choice.
>>>>
>>>> https://lkml.org/lkml/2017/11/20/502
>>>>
>>>> Mostly because IMHO its even more misleading (lacking any
>>>> fwnode_operations)
>>>> than misusing the of_node as a void *.
>>>
>>>
>>> I'm not sure what you mean.
>>
>>
>> Converting the DT drivers/cacheinfo.c code to use a fwnode_handle is
>> straightforward. But IMHO it doesn't solve the readability problem of either
>> casting the ACPI/PPTT token directly to the resulting fwnode_handle *, or
>> alternatively an actual fwnode_handle with bogus fwnode_operations to wrap
>> that token.
> 
> I'm not talking about that at all.
> 
>>>
>>> Anyway, the idea is to have one pointer in there instead of two that
>>> cannot be used at the same time and there's no reason why of_node
>>> should be special.
>>
>>
>>          Avoid two pointers for size, or readability? Because the last
>> version had a union with of_node, which isn't strictly necessary as I can
>> just cast the pptt token to of_node. There is exactly one line of code after
>> that which uses the token and it doesn't care about type.
> 
> So call this field "token" or similar.  Don't call it "of_node" and
> don't introduce another "firmware_node" thing in addition to that.
> That just is a mess, sorry.

I sort of agree, I think I can just change the whole of_node to a 
generic 'void *firmware_unique' which works fine for the PPTT code, it 
should also work for the DT code in cache_leaves_are_shared().

The slight gocha is there is a bit of DT code which initially runs 
earlier that uses of_node as an indirect parameter to a couple functions 
(by just passing the cacheinfo). Let me see if I can tweak that a bit.

Frankly, If I understood completely all the *priv cases I suspect it 
might be possible to collapse *of_node into that as well. That is as 
long as no one decides to flush out DT on x86, or PPTT on x86.

Re: [PATCH v5 4/9] drivers: base cacheinfo: Add support for ACPI based firmware tables

[Rafael J. Wysocki]

On Wed, Dec 13, 2017 at 12:37 AM, Jeremy Linton <jeremy.linton@arm.com> wrote:
> On 12/12/2017 05:02 PM, Rafael J. Wysocki wrote:
>>
>> On Tue, Dec 12, 2017 at 11:55 PM, Jeremy Linton <jeremy.linton@arm.com>
>> wrote:
>>>
>>> Hi,
>>>
>>>
>>> On 12/12/2017 11:25 AM, Rafael J. Wysocki wrote:
>>>>
>>>>
>>
>> [cut]
>
>
> (trimming list)
>
>
>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> What about converting this to using struct fwnode instead of adding
>>>>>> fields to it?
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> I didn't really want to add another field here, but I've also pointed
>>>>> out
>>>>> how I thought converting it to a fwnode wasn't a good choice.
>>>>>
>>>>> https://lkml.org/lkml/2017/11/20/502
>>>>>
>>>>> Mostly because IMHO its even more misleading (lacking any
>>>>> fwnode_operations)
>>>>> than misusing the of_node as a void *.
>>>>
>>>>
>>>>
>>>> I'm not sure what you mean.
>>>
>>>
>>>
>>> Converting the DT drivers/cacheinfo.c code to use a fwnode_handle is
>>> straightforward. But IMHO it doesn't solve the readability problem of
>>> either
>>> casting the ACPI/PPTT token directly to the resulting fwnode_handle *, or
>>> alternatively an actual fwnode_handle with bogus fwnode_operations to
>>> wrap
>>> that token.
>>
>>
>> I'm not talking about that at all.
>>
>>>>
>>>> Anyway, the idea is to have one pointer in there instead of two that
>>>> cannot be used at the same time and there's no reason why of_node
>>>> should be special.
>>>
>>>
>>>
>>>          Avoid two pointers for size, or readability? Because the last
>>> version had a union with of_node, which isn't strictly necessary as I can
>>> just cast the pptt token to of_node. There is exactly one line of code
>>> after
>>> that which uses the token and it doesn't care about type.
>>
>>
>> So call this field "token" or similar.  Don't call it "of_node" and
>> don't introduce another "firmware_node" thing in addition to that.
>> That just is a mess, sorry.
>
>
> I sort of agree, I think I can just change the whole of_node to a generic
> 'void *firmware_unique' which works fine for the PPTT code, it should also
> work for the DT code in cache_leaves_are_shared().
>
> The slight gocha is there is a bit of DT code which initially runs earlier
> that uses of_node as an indirect parameter to a couple functions (by just
> passing the cacheinfo). Let me see if I can tweak that a bit.
>
> Frankly, If I understood completely all the *priv cases I suspect it might
> be possible to collapse *of_node into that as well. That is as long as no
> one decides to flush out DT on x86, or PPTT on x86.

I'm not aware of any plans to go in that direction.

Anyway, that would be a worry of whoever wanted to do that.  No need
to worry about it upfront.

Re: [PATCH v5 3/9] ACPI: Enable PPTT support on ARM64

[Lorenzo Pieralisi]

On Fri, Dec 01, 2017 at 04:23:24PM -0600, Jeremy Linton wrote:
> Now that we have a PPTT parser, in preparation for its use
> on arm64, lets build it.
> 
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
>  arch/arm64/Kconfig    | 1 +
>  drivers/acpi/Kconfig  | 3 +++
>  drivers/acpi/Makefile | 1 +
>  3 files changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index a93339f5178f..e62fd1e08c1f 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -7,6 +7,7 @@ config ARM64
>  	select ACPI_REDUCED_HARDWARE_ONLY if ACPI
>  	select ACPI_MCFG if ACPI
>  	select ACPI_SPCR_TABLE if ACPI
> +	select ACPI_PPTT if ACPI
>  	select ARCH_CLOCKSOURCE_DATA
>  	select ARCH_HAS_DEBUG_VIRTUAL
>  	select ARCH_HAS_DEVMEM_IS_ALLOWED
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 46505396869e..df7aebf0af0e 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -545,6 +545,9 @@ config ACPI_CONFIGFS
>  
>  if ARM64
>  source "drivers/acpi/arm64/Kconfig"
> +
> +config ACPI_PPTT
> +	bool

We need to make a choice here. Either PPTT is considered ARM64 only and
we move code and config to drivers/acpi/arm64/Kconfig or we leave it in
drivers/acpi/pptt.c and we add a Kconfig entry in drivers/acpi/Kconfig
(and we make pptt.c compile on !ARM64 - which is what it should be given
that there is nothing ARM64 specific in it).

Lorenzo

>  endif
>  
>  config TPS68470_PMIC_OPREGION
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 41954a601989..b6056b566df4 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -87,6 +87,7 @@ obj-$(CONFIG_ACPI_BGRT)		+= bgrt.o
>  obj-$(CONFIG_ACPI_CPPC_LIB)	+= cppc_acpi.o
>  obj-$(CONFIG_ACPI_SPCR_TABLE)	+= spcr.o
>  obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o
> +obj-$(CONFIG_ACPI_PPTT) 	+= pptt.o
>  
>  # processor has its own "processor." module_param namespace
>  processor-y			:= processor_driver.o
> -- 
> 2.13.5
> 

Re: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code

[Lorenzo Pieralisi]

On Tue, Dec 12, 2017 at 10:13:08AM -0600, Jeremy Linton wrote:
> Hi,
> 
> First, thanks for taking a look at this.
> 
> On 12/11/2017 07:12 PM, Rafael J. Wysocki wrote:
> >On Friday, December 1, 2017 11:23:27 PM CET Jeremy Linton wrote:
> >>The PPTT can be used to determine the groupings of CPU's at
> >>given levels in the system. Lets add a few routines to the PPTT
> >>parsing code to return a unique id for each unique level in the
> >>processor hierarchy. This can then be matched to build
> >>thread/core/cluster/die/package/etc mappings for each processing
> >>element in the system.
> >>
> >>Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> >
> >Why can't this be folded into patch [2/9]?
> 
> It can, and I will be happy squash it.
> 
> It was requested that the topology portion of the parser be split
> out back in v3.
> 
> https://www.spinics.net/lists/linux-acpi/msg78487.html

I asked to split cache/topology since I am not familiar with cache
code and Sudeep - who looks after the cache code - won't be able
to review this series in time for v4.16.

Lorenzo

Re: [PATCH v5 7/9] arm64: Topology, rename cluster_id

[Lorenzo Pieralisi]

[+Morten, Dietmar]

$SUBJECT should be:

arm64: topology: rename cluster_id

On Fri, Dec 01, 2017 at 04:23:28PM -0600, Jeremy Linton wrote:
> Lets match the name of the arm64 topology field
> to the kernel macro that uses it.
> 
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
>  arch/arm64/include/asm/topology.h |  4 ++--
>  arch/arm64/kernel/topology.c      | 27 ++++++++++++++-------------
>  2 files changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
> index c4f2d50491eb..118136268f66 100644
> --- a/arch/arm64/include/asm/topology.h
> +++ b/arch/arm64/include/asm/topology.h
> @@ -7,14 +7,14 @@
>  struct cpu_topology {
>  	int thread_id;
>  	int core_id;
> -	int cluster_id;
> +	int physical_id;

package_id ?

It has been debated before, I know. Should we keep the cluster_id too
(even if it would be 1:1 mapped to package_id - for now) ?

There is also arch/arm to take into account, again, this patch is
just renaming (as it should have named since the beginning) a
topology level but we should consider everything from a legacy
perspective.

Lorenzo

>  	cpumask_t thread_sibling;
>  	cpumask_t core_sibling;
>  };
>  
>  extern struct cpu_topology cpu_topology[NR_CPUS];
>  
> -#define topology_physical_package_id(cpu)	(cpu_topology[cpu].cluster_id)
> +#define topology_physical_package_id(cpu)	(cpu_topology[cpu].physical_id)
>  #define topology_core_id(cpu)		(cpu_topology[cpu].core_id)
>  #define topology_core_cpumask(cpu)	(&cpu_topology[cpu].core_sibling)
>  #define topology_sibling_cpumask(cpu)	(&cpu_topology[cpu].thread_sibling)
> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> index 8d48b233e6ce..74a8a5173a35 100644
> --- a/arch/arm64/kernel/topology.c
> +++ b/arch/arm64/kernel/topology.c
> @@ -51,7 +51,7 @@ static int __init get_cpu_for_node(struct device_node *node)
>  	return -1;
>  }
>  
> -static int __init parse_core(struct device_node *core, int cluster_id,
> +static int __init parse_core(struct device_node *core, int physical_id,
>  			     int core_id)
>  {
>  	char name[10];
> @@ -67,7 +67,7 @@ static int __init parse_core(struct device_node *core, int cluster_id,
>  			leaf = false;
>  			cpu = get_cpu_for_node(t);
>  			if (cpu >= 0) {
> -				cpu_topology[cpu].cluster_id = cluster_id;
> +				cpu_topology[cpu].physical_id = physical_id;
>  				cpu_topology[cpu].core_id = core_id;
>  				cpu_topology[cpu].thread_id = i;
>  			} else {
> @@ -89,7 +89,7 @@ static int __init parse_core(struct device_node *core, int cluster_id,
>  			return -EINVAL;
>  		}
>  
> -		cpu_topology[cpu].cluster_id = cluster_id;
> +		cpu_topology[cpu].physical_id = physical_id;
>  		cpu_topology[cpu].core_id = core_id;
>  	} else if (leaf) {
>  		pr_err("%pOF: Can't get CPU for leaf core\n", core);
> @@ -105,7 +105,7 @@ static int __init parse_cluster(struct device_node *cluster, int depth)
>  	bool leaf = true;
>  	bool has_cores = false;
>  	struct device_node *c;
> -	static int cluster_id __initdata;
> +	static int physical_id __initdata;
>  	int core_id = 0;
>  	int i, ret;
>  
> @@ -144,7 +144,7 @@ static int __init parse_cluster(struct device_node *cluster, int depth)
>  			}
>  
>  			if (leaf) {
> -				ret = parse_core(c, cluster_id, core_id++);
> +				ret = parse_core(c, physical_id, core_id++);
>  			} else {
>  				pr_err("%pOF: Non-leaf cluster with core %s\n",
>  				       cluster, name);
> @@ -162,7 +162,7 @@ static int __init parse_cluster(struct device_node *cluster, int depth)
>  		pr_warn("%pOF: empty cluster\n", cluster);
>  
>  	if (leaf)
> -		cluster_id++;
> +		physical_id++;
>  
>  	return 0;
>  }
> @@ -198,7 +198,7 @@ static int __init parse_dt_topology(void)
>  	 * only mark cores described in the DT as possible.
>  	 */
>  	for_each_possible_cpu(cpu)
> -		if (cpu_topology[cpu].cluster_id == -1)
> +		if (cpu_topology[cpu].physical_id == -1)
>  			ret = -EINVAL;
>  
>  out_map:
> @@ -228,7 +228,7 @@ static void update_siblings_masks(unsigned int cpuid)
>  	for_each_possible_cpu(cpu) {
>  		cpu_topo = &cpu_topology[cpu];
>  
> -		if (cpuid_topo->cluster_id != cpu_topo->cluster_id)
> +		if (cpuid_topo->physical_id != cpu_topo->physical_id)
>  			continue;
>  
>  		cpumask_set_cpu(cpuid, &cpu_topo->core_sibling);
> @@ -249,7 +249,7 @@ void store_cpu_topology(unsigned int cpuid)
>  	struct cpu_topology *cpuid_topo = &cpu_topology[cpuid];
>  	u64 mpidr;
>  
> -	if (cpuid_topo->cluster_id != -1)
> +	if (cpuid_topo->physical_id != -1)
>  		goto topology_populated;
>  
>  	mpidr = read_cpuid_mpidr();
> @@ -263,19 +263,19 @@ void store_cpu_topology(unsigned int cpuid)
>  		/* Multiprocessor system : Multi-threads per core */
>  		cpuid_topo->thread_id  = MPIDR_AFFINITY_LEVEL(mpidr, 0);
>  		cpuid_topo->core_id    = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> -		cpuid_topo->cluster_id = MPIDR_AFFINITY_LEVEL(mpidr, 2) |
> +		cpuid_topo->physical_id = MPIDR_AFFINITY_LEVEL(mpidr, 2) |
>  					 MPIDR_AFFINITY_LEVEL(mpidr, 3) << 8;
>  	} else {
>  		/* Multiprocessor system : Single-thread per core */
>  		cpuid_topo->thread_id  = -1;
>  		cpuid_topo->core_id    = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> -		cpuid_topo->cluster_id = MPIDR_AFFINITY_LEVEL(mpidr, 1) |
> +		cpuid_topo->physical_id = MPIDR_AFFINITY_LEVEL(mpidr, 1) |
>  					 MPIDR_AFFINITY_LEVEL(mpidr, 2) << 8 |
>  					 MPIDR_AFFINITY_LEVEL(mpidr, 3) << 16;
>  	}
>  
>  	pr_debug("CPU%u: cluster %d core %d thread %d mpidr %#016llx\n",
> -		 cpuid, cpuid_topo->cluster_id, cpuid_topo->core_id,
> +		 cpuid, cpuid_topo->physical_id, cpuid_topo->core_id,
>  		 cpuid_topo->thread_id, mpidr);
>  
>  topology_populated:
> @@ -291,7 +291,7 @@ static void __init reset_cpu_topology(void)
>  
>  		cpu_topo->thread_id = -1;
>  		cpu_topo->core_id = 0;
> -		cpu_topo->cluster_id = -1;
> +		cpu_topo->physical_id = -1;
>  
>  		cpumask_clear(&cpu_topo->core_sibling);
>  		cpumask_set_cpu(cpu, &cpu_topo->core_sibling);
> @@ -300,6 +300,7 @@ static void __init reset_cpu_topology(void)
>  	}
>  }
>  
> +
>  void __init init_cpu_topology(void)
>  {
>  	reset_cpu_topology();
> -- 
> 2.13.5
> 

Re: [PATCH v5 8/9] arm64: topology: Enable ACPI/PPTT based CPU topology.

[Lorenzo Pieralisi]

Nit: remove the period in $SUBJECT and capitalize with a coherent
policy for the patches touching the same code.

On Fri, Dec 01, 2017 at 04:23:29PM -0600, Jeremy Linton wrote:
> Propagate the topology information from the PPTT tree to the
> cpu_topology array. We can get the thread id, core_id and
> cluster_id by assuming certain levels of the PPTT tree correspond
> to those concepts. The package_id is flagged in the tree and can be
> found by calling find_acpi_cpu_topology_package() which terminates
> its search when it finds an ACPI node flagged as the physical
> package. If the tree doesn't contain enough levels to represent
> all of the requested levels then the root node will be returned
> for all subsequent levels.
> 
> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> ---
>  arch/arm64/kernel/topology.c | 47 +++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/topology.h     |  2 ++
>  2 files changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> index 74a8a5173a35..198714aca9e8 100644
> --- a/arch/arm64/kernel/topology.c
> +++ b/arch/arm64/kernel/topology.c
> @@ -11,6 +11,7 @@
>   * for more details.
>   */
>  
> +#include <linux/acpi.h>
>  #include <linux/arch_topology.h>
>  #include <linux/cpu.h>
>  #include <linux/cpumask.h>
> @@ -22,6 +23,7 @@
>  #include <linux/sched.h>
>  #include <linux/sched/topology.h>
>  #include <linux/slab.h>
> +#include <linux/smp.h>
>  #include <linux/string.h>
>  
>  #include <asm/cpu.h>
> @@ -300,6 +302,47 @@ static void __init reset_cpu_topology(void)
>  	}
>  }
>  
> +#ifdef CONFIG_ACPI
> +/*
> + * Propagate the topology information of the processor_topology_node tree to the
> + * cpu_topology array.
> + */
> +static int __init parse_acpi_topology(void)
> +{
> +	u64 is_threaded;

Nit: a bool would be preferable.

> +	int cpu;
> +	int topology_id;

int cpu, topology_id;

> +	is_threaded = read_cpuid_mpidr() & MPIDR_MT_BITMASK;

> +	for_each_possible_cpu(cpu) {
> +		topology_id = find_acpi_cpu_topology(cpu, 0);
> +		if (topology_id < 0)
> +			return topology_id;
> +
> +		if (is_threaded) {
> +			cpu_topology[cpu].thread_id = topology_id;
> +			topology_id = find_acpi_cpu_topology(cpu, 1);
> +			cpu_topology[cpu].core_id   = topology_id;
> +			topology_id = find_acpi_cpu_topology_package(cpu);
> +			cpu_topology[cpu].physical_id = topology_id;
> +		} else {
> +			cpu_topology[cpu].thread_id  = -1;
> +			cpu_topology[cpu].core_id    = topology_id;
> +			topology_id = find_acpi_cpu_topology_package(cpu);
> +			cpu_topology[cpu].physical_id = topology_id;
> +		}
> +	}

Add a space.

It is probably my fault so apologies if that's the case. The

find_acpi_cpu_topology()

API is a bit strange since it behaves differently according to the
level passed in.

I think it is better to define two calls (it might well have been like
that in one of the previous series versions):

- find_acpi_cpu_package_level() (returns: package level if success, <0 on
  failure)
- acpi_cpu_topology_id()

It would even be better to lump the two calls together but you do not
know how many topology levels are there so it becomes a bit complicated
to handle.

> +	return 0;
> +}
> +
> +#else
> +static int __init parse_acpi_topology(void)

static inline ?

> +{
> +	/*ACPI kernels should be built with PPTT support*/

I think you can remove this comment.

> +	return -EINVAL;
> +}
> +#endif
>  
>  void __init init_cpu_topology(void)
>  {
> @@ -309,6 +352,8 @@ void __init init_cpu_topology(void)
>  	 * Discard anything that was parsed if we hit an error so we
>  	 * don't use partial information.
>  	 */
> -	if (of_have_populated_dt() && parse_dt_topology())
> +	if ((!acpi_disabled) && parse_acpi_topology())
> +		reset_cpu_topology();
> +	else if (of_have_populated_dt() && parse_dt_topology())
>  		reset_cpu_topology();
>  }
> diff --git a/include/linux/topology.h b/include/linux/topology.h
> index cb0775e1ee4b..170ce87edd88 100644
> --- a/include/linux/topology.h
> +++ b/include/linux/topology.h
> @@ -43,6 +43,8 @@
>  		if (nr_cpus_node(node))
>  
>  int arch_update_cpu_topology(void);
> +int find_acpi_cpu_topology(unsigned int cpu, int level);
> +int find_acpi_cpu_topology_package(unsigned int cpu);

I do not think these two declarations:

a) belong in this patch
b) belong in include/linux/topology.h (should be acpi.h)

Lorenzo

Re: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code

[Rafael J. Wysocki]

On Wed, Dec 13, 2017 at 6:38 PM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Tue, Dec 12, 2017 at 10:13:08AM -0600, Jeremy Linton wrote:
>> Hi,
>>
>> First, thanks for taking a look at this.
>>
>> On 12/11/2017 07:12 PM, Rafael J. Wysocki wrote:
>> >On Friday, December 1, 2017 11:23:27 PM CET Jeremy Linton wrote:
>> >>The PPTT can be used to determine the groupings of CPU's at
>> >>given levels in the system. Lets add a few routines to the PPTT
>> >>parsing code to return a unique id for each unique level in the
>> >>processor hierarchy. This can then be matched to build
>> >>thread/core/cluster/die/package/etc mappings for each processing
>> >>element in the system.
>> >>
>> >>Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>> >
>> >Why can't this be folded into patch [2/9]?
>>
>> It can, and I will be happy squash it.
>>
>> It was requested that the topology portion of the parser be split
>> out back in v3.
>>
>> https://www.spinics.net/lists/linux-acpi/msg78487.html
>
> I asked to split cache/topology since I am not familiar with cache
> code and Sudeep - who looks after the cache code - won't be able
> to review this series in time for v4.16.

OK, so why do we need it in 4.16?

Re: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code

[Jeremy Linton]

Hi,

On 12/13/2017 04:28 PM, Rafael J. Wysocki wrote:
> On Wed, Dec 13, 2017 at 6:38 PM, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
>> On Tue, Dec 12, 2017 at 10:13:08AM -0600, Jeremy Linton wrote:
>>> Hi,
>>>
>>> First, thanks for taking a look at this.
>>>
>>> On 12/11/2017 07:12 PM, Rafael J. Wysocki wrote:
>>>> On Friday, December 1, 2017 11:23:27 PM CET Jeremy Linton wrote:
>>>>> The PPTT can be used to determine the groupings of CPU's at
>>>>> given levels in the system. Lets add a few routines to the PPTT
>>>>> parsing code to return a unique id for each unique level in the
>>>>> processor hierarchy. This can then be matched to build
>>>>> thread/core/cluster/die/package/etc mappings for each processing
>>>>> element in the system.
>>>>>
>>>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>>>>
>>>> Why can't this be folded into patch [2/9]?
>>>
>>> It can, and I will be happy squash it.
>>>
>>> It was requested that the topology portion of the parser be split
>>> out back in v3.
>>>
>>> https://www.spinics.net/lists/linux-acpi/msg78487.html
>>
>> I asked to split cache/topology since I am not familiar with cache
>> code and Sudeep - who looks after the cache code - won't be able
>> to review this series in time for v4.16.
> 
> OK, so why do we need it in 4.16?

I think its more case of as soon as possible. That is because there are 
machines where the topology is completely incorrect due to assumptions 
the kernel makes based on registers that aren't defined for that purpose 
(say describing which cores are in a physical socket, or LLC's attached 
to interconnects or memory controllers).

This incorrect topology information is reported to things like the 
kernel scheduler, which then makes poor scheduling decisions resulting 
in sub-optimal system performance.


This patchset (and ACPI 6.2) clears up a lot of those problems.

Thanks,

Re: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code

[Rafael J. Wysocki]

On Thu, Dec 14, 2017 at 12:06 AM, Jeremy Linton <jeremy.linton@arm.com> wrote:
> Hi,
>
>
> On 12/13/2017 04:28 PM, Rafael J. Wysocki wrote:
>>
>> On Wed, Dec 13, 2017 at 6:38 PM, Lorenzo Pieralisi
>> <lorenzo.pieralisi@arm.com> wrote:
>>>
>>> On Tue, Dec 12, 2017 at 10:13:08AM -0600, Jeremy Linton wrote:
>>>>
>>>> Hi,
>>>>
>>>> First, thanks for taking a look at this.
>>>>
>>>> On 12/11/2017 07:12 PM, Rafael J. Wysocki wrote:
>>>>>
>>>>> On Friday, December 1, 2017 11:23:27 PM CET Jeremy Linton wrote:
>>>>>>
>>>>>> The PPTT can be used to determine the groupings of CPU's at
>>>>>> given levels in the system. Lets add a few routines to the PPTT
>>>>>> parsing code to return a unique id for each unique level in the
>>>>>> processor hierarchy. This can then be matched to build
>>>>>> thread/core/cluster/die/package/etc mappings for each processing
>>>>>> element in the system.
>>>>>>
>>>>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>>>>>
>>>>>
>>>>> Why can't this be folded into patch [2/9]?
>>>>
>>>>
>>>> It can, and I will be happy squash it.
>>>>
>>>> It was requested that the topology portion of the parser be split
>>>> out back in v3.
>>>>
>>>> https://www.spinics.net/lists/linux-acpi/msg78487.html
>>>
>>>
>>> I asked to split cache/topology since I am not familiar with cache
>>> code and Sudeep - who looks after the cache code - won't be able
>>> to review this series in time for v4.16.
>>
>>
>> OK, so why do we need it in 4.16?
>
>
> I think its more case of as soon as possible. That is because there are
> machines where the topology is completely incorrect due to assumptions the
> kernel makes based on registers that aren't defined for that purpose (say
> describing which cores are in a physical socket, or LLC's attached to
> interconnects or memory controllers).
>
> This incorrect topology information is reported to things like the kernel
> scheduler, which then makes poor scheduling decisions resulting in
> sub-optimal system performance.
>
> This patchset (and ACPI 6.2) clears up a lot of those problems.

As long as the ACPI tables are as expected that is, I suppose?

Anyway, fair enough, but I don't want to rush it in.

Thanks,
Rafael

Re: [PATCH v5 7/9] arm64: Topology, rename cluster_id

[Jeremy Linton]

Hi,

On 12/13/2017 12:02 PM, Lorenzo Pieralisi wrote:
> [+Morten, Dietmar]
> 
> $SUBJECT should be:
> 
> arm64: topology: rename cluster_id

Sure..

> 
> On Fri, Dec 01, 2017 at 04:23:28PM -0600, Jeremy Linton wrote:
>> Lets match the name of the arm64 topology field
>> to the kernel macro that uses it.
>>
>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>> ---
>>   arch/arm64/include/asm/topology.h |  4 ++--
>>   arch/arm64/kernel/topology.c      | 27 ++++++++++++++-------------
>>   2 files changed, 16 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
>> index c4f2d50491eb..118136268f66 100644
>> --- a/arch/arm64/include/asm/topology.h
>> +++ b/arch/arm64/include/asm/topology.h
>> @@ -7,14 +7,14 @@
>>   struct cpu_topology {
>>   	int thread_id;
>>   	int core_id;
>> -	int cluster_id;
>> +	int physical_id;
> 
> package_id ?

Given the macro is topology_physical_package_id, either makes sense to 
me. <shrug> I will change it in the next set.


> 
> It has been debated before, I know. Should we keep the cluster_id too
> (even if it would be 1:1 mapped to package_id - for now) ?

Well given that this patch replaces the patch that did that at your 
request..

I was hoping someone else would comment here, but my take at this point 
is that it doesn't really matter in a functional sense at the moment.
Like the chiplet discussion it can be the subject of a future patch 
along with the patches which tweak the scheduler to understand the split.

BTW, given that i'm OoO next week, and the following that are the 
holidays, I don't intend to repost this for a couple weeks. I don't 
think there are any issues with this set.

> 
> There is also arch/arm to take into account, again, this patch is
> just renaming (as it should have named since the beginning) a
> topology level but we should consider everything from a legacy
> perspective.
> 
> Lorenzo
> 
>>   	cpumask_t thread_sibling;
>>   	cpumask_t core_sibling;
>>   };
>>   
>>   extern struct cpu_topology cpu_topology[NR_CPUS];
>>   
>> -#define topology_physical_package_id(cpu)	(cpu_topology[cpu].cluster_id)
>> +#define topology_physical_package_id(cpu)	(cpu_topology[cpu].physical_id)
>>   #define topology_core_id(cpu)		(cpu_topology[cpu].core_id)
>>   #define topology_core_cpumask(cpu)	(&cpu_topology[cpu].core_sibling)
>>   #define topology_sibling_cpumask(cpu)	(&cpu_topology[cpu].thread_sibling)
>> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
>> index 8d48b233e6ce..74a8a5173a35 100644
>> --- a/arch/arm64/kernel/topology.c
>> +++ b/arch/arm64/kernel/topology.c
>> @@ -51,7 +51,7 @@ static int __init get_cpu_for_node(struct device_node *node)
>>   	return -1;
>>   }
>>   
>> -static int __init parse_core(struct device_node *core, int cluster_id,
>> +static int __init parse_core(struct device_node *core, int physical_id,
>>   			     int core_id)
>>   {
>>   	char name[10];
>> @@ -67,7 +67,7 @@ static int __init parse_core(struct device_node *core, int cluster_id,
>>   			leaf = false;
>>   			cpu = get_cpu_for_node(t);
>>   			if (cpu >= 0) {
>> -				cpu_topology[cpu].cluster_id = cluster_id;
>> +				cpu_topology[cpu].physical_id = physical_id;
>>   				cpu_topology[cpu].core_id = core_id;
>>   				cpu_topology[cpu].thread_id = i;
>>   			} else {
>> @@ -89,7 +89,7 @@ static int __init parse_core(struct device_node *core, int cluster_id,
>>   			return -EINVAL;
>>   		}
>>   
>> -		cpu_topology[cpu].cluster_id = cluster_id;
>> +		cpu_topology[cpu].physical_id = physical_id;
>>   		cpu_topology[cpu].core_id = core_id;
>>   	} else if (leaf) {
>>   		pr_err("%pOF: Can't get CPU for leaf core\n", core);
>> @@ -105,7 +105,7 @@ static int __init parse_cluster(struct device_node *cluster, int depth)
>>   	bool leaf = true;
>>   	bool has_cores = false;
>>   	struct device_node *c;
>> -	static int cluster_id __initdata;
>> +	static int physical_id __initdata;
>>   	int core_id = 0;
>>   	int i, ret;
>>   
>> @@ -144,7 +144,7 @@ static int __init parse_cluster(struct device_node *cluster, int depth)
>>   			}
>>   
>>   			if (leaf) {
>> -				ret = parse_core(c, cluster_id, core_id++);
>> +				ret = parse_core(c, physical_id, core_id++);
>>   			} else {
>>   				pr_err("%pOF: Non-leaf cluster with core %s\n",
>>   				       cluster, name);
>> @@ -162,7 +162,7 @@ static int __init parse_cluster(struct device_node *cluster, int depth)
>>   		pr_warn("%pOF: empty cluster\n", cluster);
>>   
>>   	if (leaf)
>> -		cluster_id++;
>> +		physical_id++;
>>   
>>   	return 0;
>>   }
>> @@ -198,7 +198,7 @@ static int __init parse_dt_topology(void)
>>   	 * only mark cores described in the DT as possible.
>>   	 */
>>   	for_each_possible_cpu(cpu)
>> -		if (cpu_topology[cpu].cluster_id == -1)
>> +		if (cpu_topology[cpu].physical_id == -1)
>>   			ret = -EINVAL;
>>   
>>   out_map:
>> @@ -228,7 +228,7 @@ static void update_siblings_masks(unsigned int cpuid)
>>   	for_each_possible_cpu(cpu) {
>>   		cpu_topo = &cpu_topology[cpu];
>>   
>> -		if (cpuid_topo->cluster_id != cpu_topo->cluster_id)
>> +		if (cpuid_topo->physical_id != cpu_topo->physical_id)
>>   			continue;
>>   
>>   		cpumask_set_cpu(cpuid, &cpu_topo->core_sibling);
>> @@ -249,7 +249,7 @@ void store_cpu_topology(unsigned int cpuid)
>>   	struct cpu_topology *cpuid_topo = &cpu_topology[cpuid];
>>   	u64 mpidr;
>>   
>> -	if (cpuid_topo->cluster_id != -1)
>> +	if (cpuid_topo->physical_id != -1)
>>   		goto topology_populated;
>>   
>>   	mpidr = read_cpuid_mpidr();
>> @@ -263,19 +263,19 @@ void store_cpu_topology(unsigned int cpuid)
>>   		/* Multiprocessor system : Multi-threads per core */
>>   		cpuid_topo->thread_id  = MPIDR_AFFINITY_LEVEL(mpidr, 0);
>>   		cpuid_topo->core_id    = MPIDR_AFFINITY_LEVEL(mpidr, 1);
>> -		cpuid_topo->cluster_id = MPIDR_AFFINITY_LEVEL(mpidr, 2) |
>> +		cpuid_topo->physical_id = MPIDR_AFFINITY_LEVEL(mpidr, 2) |
>>   					 MPIDR_AFFINITY_LEVEL(mpidr, 3) << 8;
>>   	} else {
>>   		/* Multiprocessor system : Single-thread per core */
>>   		cpuid_topo->thread_id  = -1;
>>   		cpuid_topo->core_id    = MPIDR_AFFINITY_LEVEL(mpidr, 0);
>> -		cpuid_topo->cluster_id = MPIDR_AFFINITY_LEVEL(mpidr, 1) |
>> +		cpuid_topo->physical_id = MPIDR_AFFINITY_LEVEL(mpidr, 1) |
>>   					 MPIDR_AFFINITY_LEVEL(mpidr, 2) << 8 |
>>   					 MPIDR_AFFINITY_LEVEL(mpidr, 3) << 16;
>>   	}
>>   
>>   	pr_debug("CPU%u: cluster %d core %d thread %d mpidr %#016llx\n",
>> -		 cpuid, cpuid_topo->cluster_id, cpuid_topo->core_id,
>> +		 cpuid, cpuid_topo->physical_id, cpuid_topo->core_id,
>>   		 cpuid_topo->thread_id, mpidr);
>>   
>>   topology_populated:
>> @@ -291,7 +291,7 @@ static void __init reset_cpu_topology(void)
>>   
>>   		cpu_topo->thread_id = -1;
>>   		cpu_topo->core_id = 0;
>> -		cpu_topo->cluster_id = -1;
>> +		cpu_topo->physical_id = -1;
>>   
>>   		cpumask_clear(&cpu_topo->core_sibling);
>>   		cpumask_set_cpu(cpu, &cpu_topo->core_sibling);
>> @@ -300,6 +300,7 @@ static void __init reset_cpu_topology(void)
>>   	}
>>   }
>>   
>> +
>>   void __init init_cpu_topology(void)
>>   {
>>   	reset_cpu_topology();
>> -- 
>> 2.13.5
>>

Re: [PATCH v5 8/9] arm64: topology: Enable ACPI/PPTT based CPU topology.

[Jeremy Linton]

Hi,

On 12/13/2017 12:22 PM, Lorenzo Pieralisi wrote:
> Nit: remove the period in $SUBJECT and capitalize with a coherent
> policy for the patches touching the same code.

Ok, thanks.

> 
> On Fri, Dec 01, 2017 at 04:23:29PM -0600, Jeremy Linton wrote:
>> Propagate the topology information from the PPTT tree to the
>> cpu_topology array. We can get the thread id, core_id and
>> cluster_id by assuming certain levels of the PPTT tree correspond
>> to those concepts. The package_id is flagged in the tree and can be
>> found by calling find_acpi_cpu_topology_package() which terminates
>> its search when it finds an ACPI node flagged as the physical
>> package. If the tree doesn't contain enough levels to represent
>> all of the requested levels then the root node will be returned
>> for all subsequent levels.
>>
>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>> ---
>>   arch/arm64/kernel/topology.c | 47 +++++++++++++++++++++++++++++++++++++++++++-
>>   include/linux/topology.h     |  2 ++
>>   2 files changed, 48 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
>> index 74a8a5173a35..198714aca9e8 100644
>> --- a/arch/arm64/kernel/topology.c
>> +++ b/arch/arm64/kernel/topology.c
>> @@ -11,6 +11,7 @@
>>    * for more details.
>>    */
>>   
>> +#include <linux/acpi.h>
>>   #include <linux/arch_topology.h>
>>   #include <linux/cpu.h>
>>   #include <linux/cpumask.h>
>> @@ -22,6 +23,7 @@
>>   #include <linux/sched.h>
>>   #include <linux/sched/topology.h>
>>   #include <linux/slab.h>
>> +#include <linux/smp.h>
>>   #include <linux/string.h>
>>   
>>   #include <asm/cpu.h>
>> @@ -300,6 +302,47 @@ static void __init reset_cpu_topology(void)
>>   	}
>>   }
>>   
>> +#ifdef CONFIG_ACPI
>> +/*
>> + * Propagate the topology information of the processor_topology_node tree to the
>> + * cpu_topology array.
>> + */
>> +static int __init parse_acpi_topology(void)
>> +{
>> +	u64 is_threaded;
> 
> Nit: a bool would be preferable.
> 
>> +	int cpu;
>> +	int topology_id;
> 
> int cpu, topology_id;
> 
>> +	is_threaded = read_cpuid_mpidr() & MPIDR_MT_BITMASK;
> 
>> +	for_each_possible_cpu(cpu) {
>> +		topology_id = find_acpi_cpu_topology(cpu, 0);
>> +		if (topology_id < 0)
>> +			return topology_id;
>> +
>> +		if (is_threaded) {
>> +			cpu_topology[cpu].thread_id = topology_id;
>> +			topology_id = find_acpi_cpu_topology(cpu, 1);
>> +			cpu_topology[cpu].core_id   = topology_id;
>> +			topology_id = find_acpi_cpu_topology_package(cpu);
>> +			cpu_topology[cpu].physical_id = topology_id;
>> +		} else {
>> +			cpu_topology[cpu].thread_id  = -1;
>> +			cpu_topology[cpu].core_id    = topology_id;
>> +			topology_id = find_acpi_cpu_topology_package(cpu);
>> +			cpu_topology[cpu].physical_id = topology_id;
>> +		}
>> +	}
> 
> Add a space.
> 
> It is probably my fault so apologies if that's the case. The
> 
> find_acpi_cpu_topology()
> 
> API is a bit strange since it behaves differently according to the
> level passed in.

? In the sense that the id is more directly defined by the firmware for 
level0? Not sure this really matters, particularly if at some future 
point we come up with a way to standardize an id for the sockets/etc (or 
we just renumber the nodes).

AKA, I don't see a problem as we aren't making any guarantees about what 
the return id represents other than its unique for siblings at a given 
level.

> 
> I think it is better to define two calls (it might well have been like
> that in one of the previous series versions):
> 
> - find_acpi_cpu_package_level() (returns: package level if success, <0 on
>    failure)
> - acpi_cpu_topology_id()

So, knowing the absolute level a given flag is set at allows you to 
query a node relative to that level. That is a good idea if you wanted 
to identify say a numa in package level (say package-1). But its 
complicated by that fact that package-1 may be meaningless in a lot of 
cases (maybe package-1 is just a core). The alternative here for finding 
a numa proximity domain is to try to find a PPTT node with a subset of 
cores which happens to match an proximity domain. But given there is 
currently nothing in the specification which requires a 1:1 mapping 
between a given set of PPTT nodes and a proximity domain (given the PPTT 
is focused on cache nodes) I tend to think we want further flags in the 
PPTT and language that makes it clear when this happens. So the 
interface should be more generic find_acpi_cpu_flag_level(cpu, flag). 
That way we avoid the complexity of trying to guess from a relative 
level if a particular topological feature is appropriate.


> 
> It would even be better to lump the two calls together but you do not
> know how many topology levels are there so it becomes a bit complicated
> to handle.

Right, which is basically what the underlying 
find_acpi_cpu_topology_tag() is doing at the moment. My tendency here is 
just to export that call and let the PPTT parsing code wrap the decision 
about what to do if the flag can't be found. Which means the whole 
discussion above about letting the higher level code try to infer 
relative levels is a last resort. I'm more for creating a couple 
additional flags (FLAG_GIVE_ME_LEVEL_WHERE_NUMA_STARTS) and feeding it 
to acpi_cpu_topology_tag() with some additional logic which says 
something to the affect, return the NUMA level in the PPTT described by 
some future standardized flag, otherwise return the socket level, and if 
that doesn't exist return the root node level (or maybe if we get 
desperate a node which seems to match a SRAT proximity domain). That 
keeps the PPTT interface fairly simple, and keeps the level selection 
out of the common topology code.


There are also some other possible future directions which don't fit any 
of these models. So in that regard I think we want to keep the inteface 
as simple as possible, and transform it in the future when we have a 
direct need for it.
	
Thanks,

Re: [PATCH v5 7/9] arm64: Topology, rename cluster_id

[Morten Rasmussen]

On Fri, Dec 15, 2017 at 10:36:35AM -0600, Jeremy Linton wrote:
> Hi,
> 
> On 12/13/2017 12:02 PM, Lorenzo Pieralisi wrote:
> >[+Morten, Dietmar]
> >
> >$SUBJECT should be:
> >
> >arm64: topology: rename cluster_id
> 
> Sure..
> 
> >
> >On Fri, Dec 01, 2017 at 04:23:28PM -0600, Jeremy Linton wrote:
> >>Lets match the name of the arm64 topology field
> >>to the kernel macro that uses it.
> >>
> >>Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> >>---
> >>  arch/arm64/include/asm/topology.h |  4 ++--
> >>  arch/arm64/kernel/topology.c      | 27 ++++++++++++++-------------
> >>  2 files changed, 16 insertions(+), 15 deletions(-)
> >>
> >>diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
> >>index c4f2d50491eb..118136268f66 100644
> >>--- a/arch/arm64/include/asm/topology.h
> >>+++ b/arch/arm64/include/asm/topology.h
> >>@@ -7,14 +7,14 @@
> >>  struct cpu_topology {
> >>  	int thread_id;
> >>  	int core_id;
> >>-	int cluster_id;
> >>+	int physical_id;
> >
> >package_id ?
> 
> Given the macro is topology_physical_package_id, either makes sense to me.
> <shrug> I will change it in the next set.

I would vote for package_id too. arch/arm has 'socket_id' though.

> >
> >It has been debated before, I know. Should we keep the cluster_id too
> >(even if it would be 1:1 mapped to package_id - for now) ?
> 
> Well given that this patch replaces the patch that did that at your
> request..
> 
> I was hoping someone else would comment here, but my take at this point is
> that it doesn't really matter in a functional sense at the moment.
> Like the chiplet discussion it can be the subject of a future patch along
> with the patches which tweak the scheduler to understand the split.
> 
> BTW, given that i'm OoO next week, and the following that are the holidays,
> I don't intend to repost this for a couple weeks. I don't think there are
> any issues with this set.
> 
> >
> >There is also arch/arm to take into account, again, this patch is
> >just renaming (as it should have named since the beginning) a
> >topology level but we should consider everything from a legacy
> >perspective.

arch/arm has gone for thread/core/socket for the three topology levels
it supports.

I'm not sure what short term value keeping cluster_id has? Isn't it just
about where we make the package = cluster assignment? Currently it is in
the definition of topology_physical_package_id. If we keep cluster_id
and add package_id, it gets moved into the MPIDR/DT parsing code. 

Keeping cluster_id and introducing a topology_cluster_id function could
help cleaning up some of the users of topology_physical_package_id that
currently assumes package_id == cluster_id though.

Morten

Re: [PATCH v5 7/9] arm64: Topology, rename cluster_id

[Lorenzo Pieralisi]

On Mon, Dec 18, 2017 at 12:42:29PM +0000, Morten Rasmussen wrote:
> On Fri, Dec 15, 2017 at 10:36:35AM -0600, Jeremy Linton wrote:
> > Hi,
> > 
> > On 12/13/2017 12:02 PM, Lorenzo Pieralisi wrote:
> > >[+Morten, Dietmar]
> > >
> > >$SUBJECT should be:
> > >
> > >arm64: topology: rename cluster_id
> > 
> > Sure..
> > 
> > >
> > >On Fri, Dec 01, 2017 at 04:23:28PM -0600, Jeremy Linton wrote:
> > >>Lets match the name of the arm64 topology field
> > >>to the kernel macro that uses it.
> > >>
> > >>Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> > >>---
> > >>  arch/arm64/include/asm/topology.h |  4 ++--
> > >>  arch/arm64/kernel/topology.c      | 27 ++++++++++++++-------------
> > >>  2 files changed, 16 insertions(+), 15 deletions(-)
> > >>
> > >>diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
> > >>index c4f2d50491eb..118136268f66 100644
> > >>--- a/arch/arm64/include/asm/topology.h
> > >>+++ b/arch/arm64/include/asm/topology.h
> > >>@@ -7,14 +7,14 @@
> > >>  struct cpu_topology {
> > >>  	int thread_id;
> > >>  	int core_id;
> > >>-	int cluster_id;
> > >>+	int physical_id;
> > >
> > >package_id ?
> > 
> > Given the macro is topology_physical_package_id, either makes sense to me.
> > <shrug> I will change it in the next set.
> 
> I would vote for package_id too. arch/arm has 'socket_id' though.
> 
> > >
> > >It has been debated before, I know. Should we keep the cluster_id too
> > >(even if it would be 1:1 mapped to package_id - for now) ?
> > 
> > Well given that this patch replaces the patch that did that at your
> > request..
> > 
> > I was hoping someone else would comment here, but my take at this point is
> > that it doesn't really matter in a functional sense at the moment.
> > Like the chiplet discussion it can be the subject of a future patch along
> > with the patches which tweak the scheduler to understand the split.
> > 
> > BTW, given that i'm OoO next week, and the following that are the holidays,
> > I don't intend to repost this for a couple weeks. I don't think there are
> > any issues with this set.
> > 
> > >
> > >There is also arch/arm to take into account, again, this patch is
> > >just renaming (as it should have named since the beginning) a
> > >topology level but we should consider everything from a legacy
> > >perspective.
> 
> arch/arm has gone for thread/core/socket for the three topology levels
> it supports.
> 
> I'm not sure what short term value keeping cluster_id has? Isn't it just
> about where we make the package = cluster assignment? Currently it is in
> the definition of topology_physical_package_id. If we keep cluster_id
> and add package_id, it gets moved into the MPIDR/DT parsing code. 
> 
> Keeping cluster_id and introducing a topology_cluster_id function could
> help cleaning up some of the users of topology_physical_package_id that
> currently assumes package_id == cluster_id though.

I think we should settle for a name (eg package_id), remove cluster_id
and convert arch/arm socket_id to the same naming used on arm64 (for
consistency - it is just a variable rename).

This leaves us with the naming "cluster" only in DT topology bindings,
which should be fine, given that "cluster" in that context is just a
"processor-container" - yes we could have chosen a better naming in
the first place but that's what it is.

We should nuke the existing users of topology_physical_package_id()
to identify clusters, I would not add another function for that purpose,
let's nip it in the bud.

Lorenzo

Re: [PATCH v5 7/9] arm64: Topology, rename cluster_id

[Morten Rasmussen]

On Mon, Dec 18, 2017 at 03:47:03PM +0000, Lorenzo Pieralisi wrote:
> On Mon, Dec 18, 2017 at 12:42:29PM +0000, Morten Rasmussen wrote:
> > On Fri, Dec 15, 2017 at 10:36:35AM -0600, Jeremy Linton wrote:
> > > Hi,
> > > 
> > > On 12/13/2017 12:02 PM, Lorenzo Pieralisi wrote:
> > > >[+Morten, Dietmar]
> > > >
> > > >$SUBJECT should be:
> > > >
> > > >arm64: topology: rename cluster_id
> > > 
> > > Sure..
> > > 
> > > >
> > > >On Fri, Dec 01, 2017 at 04:23:28PM -0600, Jeremy Linton wrote:
> > > >>Lets match the name of the arm64 topology field
> > > >>to the kernel macro that uses it.
> > > >>
> > > >>Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> > > >>---
> > > >>  arch/arm64/include/asm/topology.h |  4 ++--
> > > >>  arch/arm64/kernel/topology.c      | 27 ++++++++++++++-------------
> > > >>  2 files changed, 16 insertions(+), 15 deletions(-)
> > > >>
> > > >>diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
> > > >>index c4f2d50491eb..118136268f66 100644
> > > >>--- a/arch/arm64/include/asm/topology.h
> > > >>+++ b/arch/arm64/include/asm/topology.h
> > > >>@@ -7,14 +7,14 @@
> > > >>  struct cpu_topology {
> > > >>  	int thread_id;
> > > >>  	int core_id;
> > > >>-	int cluster_id;
> > > >>+	int physical_id;
> > > >
> > > >package_id ?
> > > 
> > > Given the macro is topology_physical_package_id, either makes sense to me.
> > > <shrug> I will change it in the next set.
> > 
> > I would vote for package_id too. arch/arm has 'socket_id' though.
> > 
> > > >
> > > >It has been debated before, I know. Should we keep the cluster_id too
> > > >(even if it would be 1:1 mapped to package_id - for now) ?
> > > 
> > > Well given that this patch replaces the patch that did that at your
> > > request..
> > > 
> > > I was hoping someone else would comment here, but my take at this point is
> > > that it doesn't really matter in a functional sense at the moment.
> > > Like the chiplet discussion it can be the subject of a future patch along
> > > with the patches which tweak the scheduler to understand the split.
> > > 
> > > BTW, given that i'm OoO next week, and the following that are the holidays,
> > > I don't intend to repost this for a couple weeks. I don't think there are
> > > any issues with this set.
> > > 
> > > >
> > > >There is also arch/arm to take into account, again, this patch is
> > > >just renaming (as it should have named since the beginning) a
> > > >topology level but we should consider everything from a legacy
> > > >perspective.
> > 
> > arch/arm has gone for thread/core/socket for the three topology levels
> > it supports.
> > 
> > I'm not sure what short term value keeping cluster_id has? Isn't it just
> > about where we make the package = cluster assignment? Currently it is in
> > the definition of topology_physical_package_id. If we keep cluster_id
> > and add package_id, it gets moved into the MPIDR/DT parsing code. 
> > 
> > Keeping cluster_id and introducing a topology_cluster_id function could
> > help cleaning up some of the users of topology_physical_package_id that
> > currently assumes package_id == cluster_id though.
> 
> I think we should settle for a name (eg package_id), remove cluster_id
> and convert arch/arm socket_id to the same naming used on arm64 (for
> consistency - it is just a variable rename).

Agreed. 

> This leaves us with the naming "cluster" only in DT topology bindings,
> which should be fine, given that "cluster" in that context is just a
> "processor-container" - yes we could have chosen a better naming in
> the first place but that's what it is.

I think having "clusters" in DT is fine as it represent the actual
hardware topology and uses the actual "Arm" term to describe it. The
default topology in Linux just doesn't have an equivalent topology
level, but that can be fixed. DT is however missing a notion of package.

> We should nuke the existing users of topology_physical_package_id()
> to identify clusters, I would not add another function for that purpose,
> let's nip it in the bud.

Even better if that can be pulled of.

Morten

Re: [PATCH v5 7/9] arm64: Topology, rename cluster_id

[Xiongfeng Wang]

Hi,

On 2017/12/18 20:42, Morten Rasmussen wrote:
> On Fri, Dec 15, 2017 at 10:36:35AM -0600, Jeremy Linton wrote:
>> Hi,
>>
>> On 12/13/2017 12:02 PM, Lorenzo Pieralisi wrote:
>>> [+Morten, Dietmar]
>>>
>>> $SUBJECT should be:
>>>
>>> arm64: topology: rename cluster_id
>>
[cut]
>>
>> I was hoping someone else would comment here, but my take at this point is
>> that it doesn't really matter in a functional sense at the moment.
>> Like the chiplet discussion it can be the subject of a future patch along
>> with the patches which tweak the scheduler to understand the split.
>>
>> BTW, given that i'm OoO next week, and the following that are the holidays,
>> I don't intend to repost this for a couple weeks. I don't think there are
>> any issues with this set.
>>
>>>
>>> There is also arch/arm to take into account, again, this patch is
>>> just renaming (as it should have named since the beginning) a
>>> topology level but we should consider everything from a legacy
>>> perspective.
> 
> arch/arm has gone for thread/core/socket for the three topology levels
> it supports.
> 
> I'm not sure what short term value keeping cluster_id has? Isn't it just
> about where we make the package = cluster assignment? Currently it is in
> the definition of topology_physical_package_id. If we keep cluster_id
> and add package_id, it gets moved into the MPIDR/DT parsing code. 
> 
> Keeping cluster_id and introducing a topology_cluster_id function could
> help cleaning up some of the users of topology_physical_package_id that
> currently assumes package_id == cluster_id though.

I think we still need the information describing which cores are in one cluster.
Many arm64 chips have the architecture core/cluster/socket. Cores in one cluster may
share a same L2 cache. That information can be used to build the sched_domain. If we put
cores in one cluster in one sched_domain, the performance will be better.(please see
kernel/sched/topology.c:1197, cpu_coregroup_mask() uses 'core_sibling' to build a multi-core
sched_domain)
So I think we still need variable to record which cores are in one sched_domain for future use.

Thanks,
Xiongfeng

> 
> Morten
> 
> .
> 

Re: [PATCH v5 7/9] arm64: Topology, rename cluster_id

[Morten Rasmussen]

On Tue, Jan 02, 2018 at 10:29:35AM +0800, Xiongfeng Wang wrote:
> Hi,
> 
> On 2017/12/18 20:42, Morten Rasmussen wrote:
> > On Fri, Dec 15, 2017 at 10:36:35AM -0600, Jeremy Linton wrote:
> >> Hi,
> >>
> >> On 12/13/2017 12:02 PM, Lorenzo Pieralisi wrote:
> >>> [+Morten, Dietmar]
> >>>
> >>> $SUBJECT should be:
> >>>
> >>> arm64: topology: rename cluster_id
> >>
> [cut]
> >>
> >> I was hoping someone else would comment here, but my take at this point is
> >> that it doesn't really matter in a functional sense at the moment.
> >> Like the chiplet discussion it can be the subject of a future patch along
> >> with the patches which tweak the scheduler to understand the split.
> >>
> >> BTW, given that i'm OoO next week, and the following that are the holidays,
> >> I don't intend to repost this for a couple weeks. I don't think there are
> >> any issues with this set.
> >>
> >>>
> >>> There is also arch/arm to take into account, again, this patch is
> >>> just renaming (as it should have named since the beginning) a
> >>> topology level but we should consider everything from a legacy
> >>> perspective.
> > 
> > arch/arm has gone for thread/core/socket for the three topology levels
> > it supports.
> > 
> > I'm not sure what short term value keeping cluster_id has? Isn't it just
> > about where we make the package = cluster assignment? Currently it is in
> > the definition of topology_physical_package_id. If we keep cluster_id
> > and add package_id, it gets moved into the MPIDR/DT parsing code. 
> > 
> > Keeping cluster_id and introducing a topology_cluster_id function could
> > help cleaning up some of the users of topology_physical_package_id that
> > currently assumes package_id == cluster_id though.
> 
> I think we still need the information describing which cores are in one cluster.
> Many arm64 chips have the architecture core/cluster/socket. Cores in one cluster may
> share a same L2 cache. That information can be used to build the sched_domain. If we put
> cores in one cluster in one sched_domain, the performance will be better.(please see
> kernel/sched/topology.c:1197, cpu_coregroup_mask() uses 'core_sibling' to build a multi-core
> sched_domain)
> So I think we still need variable to record which cores are in one sched_domain for future use.

I agree that information about clusters is useful. The problem is that
we currently treat clusters as sockets (also known as dies or
physical_package_id depending on which bit of code you are looking at). We
don't handle the core/cluster/socket topology you mention as three
'levels' of grouping of cores. We currently create one sched_domain
levels for cores (in a cluster) and a parent sched_domain level of all
clusters in the system ignoring if those are in different sockets and
tell the scheduler that those clusters are all separate sockets. This
doesn't work very well for systems with many clusters and/or multiple
sockets.

How to solve that problem is a longer discussion which I'm happy to
have. This patch is just preparing to get rid of the assumption that
clusters are sockets as this is not in line with what other
architectures does and the assumptions made by the scheduler.

Morten

RE: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code

[vkilari]

Hi Jeremy,

 Sorry, I don't have your previous patch emails to reply on right patch
context.
So commenting on top of this patch.

AFAIU, the PPTT v5 patches still rely on CLIDR_EL1 register to know the type
of 
Caches enabled/available on the platform. With PPTT, it should not rely on
architecture
registers. There can be platforms which can report cache availability in
PPTT but not in 
architecture registers.

The following code snippet shows usage of CLIDR_EL1

In arch/arm64/kernel/cacheinfo.c

static inline enum cache_type get_cache_type(int level)
{
         u64 clidr;
 
         if (level > MAX_CACHE_LEVEL)
                 return CACHE_TYPE_NOCACHE;
         clidr = read_sysreg(clidr_el1);
         return CLIDR_CTYPE(clidr, level);
}

static int __populate_cache_leaves(unsigned int cpu)
{
          unsigned int level, idx;
          enum cache_type type;
          struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
          struct cacheinfo *this_leaf = this_cpu_ci->info_list;
  
          for (idx = 0, level = 1; level <= this_cpu_ci->num_levels &&
               idx < this_cpu_ci->num_leaves; idx++, level++) {
                  type = get_cache_type(level);
                  if (type == CACHE_TYPE_SEPARATE) {
                          ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level);
                          ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level);
                  } else {
                          ci_leaf_init(this_leaf++, type, level);
                  }
         }
          return 0;
 }

In populate_cache_leaves() the cache type is read from CLIDR_EL1 register.
If CLIDR_EL1 reports CACHE_TYPE_NOCACHE for a particular level then sysfs
entry
/sys/devices/system/cpu/cpu0/index<n>/type is not created and hence
userspace tools
like lstopo will not report this cache level.

Regards
Vijay

> -----Original Message-----
> From: linux-arm-kernel
[mailto:linux-arm-kernel-bounces@lists.infradead.org]
> On Behalf Of Rafael J. Wysocki
> Sent: Thursday, December 14, 2017 4:40 AM
> To: Jeremy Linton <jeremy.linton@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>; Jonathan.Zhang@cavium.com;
> Jayachandran.Nair@cavium.com; Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com>; Catalin Marinas <catalin.marinas@arm.com>;
> Rafael J. Wysocki <rafael@kernel.org>; jhugo@codeaurora.org; Will Deacon
> <will.deacon@arm.com>; Linux PM <linux-pm@vger.kernel.org>; Rafael J.
> Wysocki <rjw@rjwysocki.net>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; ACPI Devel Maling List
<linux-acpi@vger.kernel.org>;
> Viresh Kumar <viresh.kumar@linaro.org>; Hanjun Guo
> <hanjun.guo@linaro.org>; Al Stone <ahs3@redhat.com>; Sudeep Holla
> <sudeep.holla@arm.com>; austinwc@codeaurora.org;
> wangxiongfeng2@huawei.com; linux-arm-kernel@lists.infradead.org; Len
> Brown <lenb@kernel.org>
> Subject: Re: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code
> 
> On Thu, Dec 14, 2017 at 12:06 AM, Jeremy Linton <jeremy.linton@arm.com>
> wrote:
> > Hi,
> >
> >
> > On 12/13/2017 04:28 PM, Rafael J. Wysocki wrote:
> >>
> >> On Wed, Dec 13, 2017 at 6:38 PM, Lorenzo Pieralisi
> >> <lorenzo.pieralisi@arm.com> wrote:
> >>>
> >>> On Tue, Dec 12, 2017 at 10:13:08AM -0600, Jeremy Linton wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> First, thanks for taking a look at this.
> >>>>
> >>>> On 12/11/2017 07:12 PM, Rafael J. Wysocki wrote:
> >>>>>
> >>>>> On Friday, December 1, 2017 11:23:27 PM CET Jeremy Linton wrote:
> >>>>>>
> >>>>>> The PPTT can be used to determine the groupings of CPU's at given
> >>>>>> levels in the system. Lets add a few routines to the PPTT parsing
> >>>>>> code to return a unique id for each unique level in the processor
> >>>>>> hierarchy. This can then be matched to build
> >>>>>> thread/core/cluster/die/package/etc mappings for each processing
> >>>>>> element in the system.
> >>>>>>
> >>>>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> >>>>>
> >>>>>
> >>>>> Why can't this be folded into patch [2/9]?
> >>>>
> >>>>
> >>>> It can, and I will be happy squash it.
> >>>>
> >>>> It was requested that the topology portion of the parser be split
> >>>> out back in v3.
> >>>>
> >>>> https://www.spinics.net/lists/linux-acpi/msg78487.html
> >>>
> >>>
> >>> I asked to split cache/topology since I am not familiar with cache
> >>> code and Sudeep - who looks after the cache code - won't be able to
> >>> review this series in time for v4.16.
> >>
> >>
> >> OK, so why do we need it in 4.16?
> >
> >
> > I think its more case of as soon as possible. That is because there
> > are machines where the topology is completely incorrect due to
> > assumptions the kernel makes based on registers that aren't defined
> > for that purpose (say describing which cores are in a physical socket,
> > or LLC's attached to interconnects or memory controllers).
> >
> > This incorrect topology information is reported to things like the
> > kernel scheduler, which then makes poor scheduling decisions resulting
> > in sub-optimal system performance.
> >
> > This patchset (and ACPI 6.2) clears up a lot of those problems.
> 
> As long as the ACPI tables are as expected that is, I suppose?
> 
> Anyway, fair enough, but I don't want to rush it in.
> 
> Thanks,
> Rafael
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 4/9] drivers: base cacheinfo: Add support for ACPI based firmware tables

[Sudeep Holla]

(Sorry for the delay, just returning from vacation)

On 12/12/17 23:37, Jeremy Linton wrote:
> On 12/12/2017 05:02 PM, Rafael J. Wysocki wrote:

[...]

>>
>> So call this field "token" or similar.  Don't call it "of_node" and
>> don't introduce another "firmware_node" thing in addition to that.
>> That just is a mess, sorry.

I completely agree. Both me and Lorenzo pointed that out in previous
revisions and fair enough you have a valid concern it's use with PPTT.

> 
> I sort of agree, I think I can just change the whole of_node to a
> generic 'void *firmware_unique' which works fine for the PPTT code, it
> should also work for the DT code in cache_leaves_are_shared().
>

Should be fine.

> The slight gocha is there is a bit of DT code which initially runs
> earlier that uses of_node as an indirect parameter to a couple functions
> (by just passing the cacheinfo). Let me see if I can tweak that a bit.
>

May be use a simple inline wrapper functions to convert, might help if
we diverge too.

> Frankly, If I understood completely all the *priv cases I suspect it
> might be possible to collapse *of_node into that as well. That is as
> long as no one decides to flush out DT on x86, or PPTT on x86.
> 

priv is used to save architecture/cache specific details that can't be
generalized. I doubt if this of_node or PPTT pointer/offset falls in
that category.


-- 
Regards,
Sudeep

Re: [PATCH v5 7/9] arm64: Topology, rename cluster_id

[Sudeep Holla]

On 02/01/18 02:29, Xiongfeng Wang wrote:
> Hi,
> 
> On 2017/12/18 20:42, Morten Rasmussen wrote:
>> On Fri, Dec 15, 2017 at 10:36:35AM -0600, Jeremy Linton wrote:
>>> Hi,
>>>
>>> On 12/13/2017 12:02 PM, Lorenzo Pieralisi wrote:
>>>> [+Morten, Dietmar]
>>>>
>>>> $SUBJECT should be:
>>>>
>>>> arm64: topology: rename cluster_id
>>>
> [cut]
>>>
> I think we still need the information describing which cores are in one
> cluster. Many arm64 chips have the architecture core/cluster/socket. Cores
> in one cluster may share a same L2 cache. That information can be used to
> build the sched_domain. If we put cores in one cluster in one sched_domain,
> the performance will be better.(please see kernel/sched/topology.c:1197,
> cpu_coregroup_mask() uses 'core_sibling' to build a multi-core
> sched_domain).

We get all the cache information from DT/ACPI PPTT(mainly topology) and now
even the geometry. So ideally, the sharing information must come from that.
Any other solution might end up in conflict if DT/PPTT and that mismatch.

> So I think we still need variable to record which cores are in one
> sched_domain for future use.

I tend to say no, at-least not as is.
-- 
Regards,
Sudeep

Re: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code

[Jeremy Linton]

Hi,

On 01/03/2018 02:49 AM, vkilari@codeaurora.org wrote:
> Hi Jeremy,
> 
>   Sorry, I don't have your previous patch emails to reply on right patch
> context.
> So commenting on top of this patch.
> 
> AFAIU, the PPTT v5 patches still rely on CLIDR_EL1 register to know the type
> of
> Caches enabled/available on the platform. With PPTT, it should not rely on
> architecture
> registers. There can be platforms which can report cache availability in
> PPTT but not in
> architecture registers.
> 
> The following code snippet shows usage of CLIDR_EL1
> 
> In arch/arm64/kernel/cacheinfo.c
> 
> static inline enum cache_type get_cache_type(int level)
> {
>           u64 clidr;
>   
>           if (level > MAX_CACHE_LEVEL)
>                   return CACHE_TYPE_NOCACHE;
>           clidr = read_sysreg(clidr_el1);
>           return CLIDR_CTYPE(clidr, level);
> }
> 
> static int __populate_cache_leaves(unsigned int cpu)
> {
>            unsigned int level, idx;
>            enum cache_type type;
>            struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
>            struct cacheinfo *this_leaf = this_cpu_ci->info_list;
>    
>            for (idx = 0, level = 1; level <= this_cpu_ci->num_levels &&
>                 idx < this_cpu_ci->num_leaves; idx++, level++) {
>                    type = get_cache_type(level);
>                    if (type == CACHE_TYPE_SEPARATE) {
>                            ci_leaf_init(this_leaf++, CACHE_TYPE_DATA, level);
>                            ci_leaf_init(this_leaf++, CACHE_TYPE_INST, level);
>                    } else {
>                            ci_leaf_init(this_leaf++, type, level);
>                    }
>           }
>            return 0;
>   }
> 
> In populate_cache_leaves() the cache type is read from CLIDR_EL1 register.
> If CLIDR_EL1 reports CACHE_TYPE_NOCACHE for a particular level then sysfs
> entry
> /sys/devices/system/cpu/cpu0/index<n>/type is not created and hence
> userspace tools
> like lstopo will not report this cache level.


This sounds suspiciously like one of things tweaked between v4->v5. If 
you look at update_cache_properties() in patch 2/9, you will see that we 
only update/find NOCACHE nodes and convert them to UNIFIED when all the 
attributes in the node are supplied.

This means that if the node has an incomplete set of attributes we won't 
update it. Can you verify that you have all those attributes set for 
nodes which aren't being described by the hardware?

Thanks,


> 
> Regards
> Vijay
> 
>> -----Original Message-----
>> From: linux-arm-kernel
> [mailto:linux-arm-kernel-bounces@lists.infradead.org]
>> On Behalf Of Rafael J. Wysocki
>> Sent: Thursday, December 14, 2017 4:40 AM
>> To: Jeremy Linton <jeremy.linton@arm.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>; Jonathan.Zhang@cavium.com;
>> Jayachandran.Nair@cavium.com; Lorenzo Pieralisi
>> <lorenzo.pieralisi@arm.com>; Catalin Marinas <catalin.marinas@arm.com>;
>> Rafael J. Wysocki <rafael@kernel.org>; jhugo@codeaurora.org; Will Deacon
>> <will.deacon@arm.com>; Linux PM <linux-pm@vger.kernel.org>; Rafael J.
>> Wysocki <rjw@rjwysocki.net>; Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org>; Linux Kernel Mailing List <linux-
>> kernel@vger.kernel.org>; ACPI Devel Maling List
> <linux-acpi@vger.kernel.org>;
>> Viresh Kumar <viresh.kumar@linaro.org>; Hanjun Guo
>> <hanjun.guo@linaro.org>; Al Stone <ahs3@redhat.com>; Sudeep Holla
>> <sudeep.holla@arm.com>; austinwc@codeaurora.org;
>> wangxiongfeng2@huawei.com; linux-arm-kernel@lists.infradead.org; Len
>> Brown <lenb@kernel.org>
>> Subject: Re: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code
>>
>> On Thu, Dec 14, 2017 at 12:06 AM, Jeremy Linton <jeremy.linton@arm.com>
>> wrote:
>>> Hi,
>>>
>>>
>>> On 12/13/2017 04:28 PM, Rafael J. Wysocki wrote:
>>>>
>>>> On Wed, Dec 13, 2017 at 6:38 PM, Lorenzo Pieralisi
>>>> <lorenzo.pieralisi@arm.com> wrote:
>>>>>
>>>>> On Tue, Dec 12, 2017 at 10:13:08AM -0600, Jeremy Linton wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> First, thanks for taking a look at this.
>>>>>>
>>>>>> On 12/11/2017 07:12 PM, Rafael J. Wysocki wrote:
>>>>>>>
>>>>>>> On Friday, December 1, 2017 11:23:27 PM CET Jeremy Linton wrote:
>>>>>>>>
>>>>>>>> The PPTT can be used to determine the groupings of CPU's at given
>>>>>>>> levels in the system. Lets add a few routines to the PPTT parsing
>>>>>>>> code to return a unique id for each unique level in the processor
>>>>>>>> hierarchy. This can then be matched to build
>>>>>>>> thread/core/cluster/die/package/etc mappings for each processing
>>>>>>>> element in the system.
>>>>>>>>
>>>>>>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>>>>>>>
>>>>>>>
>>>>>>> Why can't this be folded into patch [2/9]?
>>>>>>
>>>>>>
>>>>>> It can, and I will be happy squash it.
>>>>>>
>>>>>> It was requested that the topology portion of the parser be split
>>>>>> out back in v3.
>>>>>>
>>>>>> https://www.spinics.net/lists/linux-acpi/msg78487.html
>>>>>
>>>>>
>>>>> I asked to split cache/topology since I am not familiar with cache
>>>>> code and Sudeep - who looks after the cache code - won't be able to
>>>>> review this series in time for v4.16.
>>>>
>>>>
>>>> OK, so why do we need it in 4.16?
>>>
>>>
>>> I think its more case of as soon as possible. That is because there
>>> are machines where the topology is completely incorrect due to
>>> assumptions the kernel makes based on registers that aren't defined
>>> for that purpose (say describing which cores are in a physical socket,
>>> or LLC's attached to interconnects or memory controllers).
>>>
>>> This incorrect topology information is reported to things like the
>>> kernel scheduler, which then makes poor scheduling decisions resulting
>>> in sub-optimal system performance.
>>>
>>> This patchset (and ACPI 6.2) clears up a lot of those problems.
>>
>> As long as the ACPI tables are as expected that is, I suppose?
>>
>> Anyway, fair enough, but I don't want to rush it in.
>>
>> Thanks,
>> Rafael
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH v5 7/9] arm64: Topology, rename cluster_id

[Jeremy Linton]

Hi,

On 01/03/2018 08:29 AM, Sudeep Holla wrote:
> 
> On 02/01/18 02:29, Xiongfeng Wang wrote:
>> Hi,
>>
>> On 2017/12/18 20:42, Morten Rasmussen wrote:
>>> On Fri, Dec 15, 2017 at 10:36:35AM -0600, Jeremy Linton wrote:
>>>> Hi,
>>>>
>>>> On 12/13/2017 12:02 PM, Lorenzo Pieralisi wrote:
>>>>> [+Morten, Dietmar]
>>>>>
>>>>> $SUBJECT should be:
>>>>>
>>>>> arm64: topology: rename cluster_id
>>>>
>> [cut]
>>>>
>> I think we still need the information describing which cores are in one
>> cluster. Many arm64 chips have the architecture core/cluster/socket. Cores
>> in one cluster may share a same L2 cache. That information can be used to
>> build the sched_domain. If we put cores in one cluster in one sched_domain,
>> the performance will be better.(please see kernel/sched/topology.c:1197,
>> cpu_coregroup_mask() uses 'core_sibling' to build a multi-core
>> sched_domain).
> 
> We get all the cache information from DT/ACPI PPTT(mainly topology) and now
> even the geometry. So ideally, the sharing information must come from that.
> Any other solution might end up in conflict if DT/PPTT and that mismatch.
> 
>> So I think we still need variable to record which cores are in one
>> sched_domain for future use.
> 
> I tend to say no, at-least not as is.
> 

Well, either way, with DynamiQ (and a55/a75) the cores have private 
L2's, which means that the cluster sharing is happening at what is then 
the L3 level. So, the code I had in earlier versions would have needed 
tweaks to deal with that anyway.

IMHO, if we want to detect this kind of sharing for future scheduling 
domains, it should probably be done independent of PPTT/DT/MIPDR by 
picking out shared cache levels from struct cacheinfo *. Which makes 
that change unrelated to the basic population of cacheinfo and 
cpu_topology in this patchset.

Re: [PATCH v5 7/9] arm64: Topology, rename cluster_id

[Sudeep Holla]

On Wed, Jan 03, 2018 at 11:32:00AM -0600, Jeremy Linton wrote:
> Hi,
> 
> On 01/03/2018 08:29 AM, Sudeep Holla wrote:
> >
> >On 02/01/18 02:29, Xiongfeng Wang wrote:
> >>Hi,
> >>
> >>On 2017/12/18 20:42, Morten Rasmussen wrote:
> >>>On Fri, Dec 15, 2017 at 10:36:35AM -0600, Jeremy Linton wrote:
> >>>>Hi,
> >>>>
> >>>>On 12/13/2017 12:02 PM, Lorenzo Pieralisi wrote:
> >>>>>[+Morten, Dietmar]
> >>>>>
> >>>>>$SUBJECT should be:
> >>>>>
> >>>>>arm64: topology: rename cluster_id
> >>>>
> >>[cut]
> >>>>
> >>I think we still need the information describing which cores are in one
> >>cluster. Many arm64 chips have the architecture core/cluster/socket. Cores
> >>in one cluster may share a same L2 cache. That information can be used to
> >>build the sched_domain. If we put cores in one cluster in one sched_domain,
> >>the performance will be better.(please see kernel/sched/topology.c:1197,
> >>cpu_coregroup_mask() uses 'core_sibling' to build a multi-core
> >>sched_domain).
> >
> >We get all the cache information from DT/ACPI PPTT(mainly topology) and now
> >even the geometry. So ideally, the sharing information must come from that.
> >Any other solution might end up in conflict if DT/PPTT and that mismatch.
> >
> >>So I think we still need variable to record which cores are in one
> >>sched_domain for future use.
> >
> >I tend to say no, at-least not as is.
> >
> 
> Well, either way, with DynamiQ (and a55/a75) the cores have private L2's,
> which means that the cluster sharing is happening at what is then the L3
> level. So, the code I had in earlier versions would have needed tweaks to
> deal with that anyway.
>

Indeed.

> IMHO, if we want to detect this kind of sharing for future scheduling
> domains, it should probably be done independent of PPTT/DT/MIPDR by picking
> out shared cache levels from struct cacheinfo *. Which makes that change
> unrelated to the basic population of cacheinfo and cpu_topology in this
> patchset.
>

Sure, that's what I meant above. i.e. we need to depend on firmware(DT/ACPI)
rather than architected way(which doesn't exist anyways). Since cacheinfo
abstracts DT/ACPI, it sounds right to use that to fetch any information
on cache topology.

--
Regards,
Sudeep

Re: [PATCH v5 7/9] arm64: Topology, rename cluster_id

[Xiongfeng Wang]

On 2018/1/4 1:32, Jeremy Linton wrote:
> Hi,
> 
> On 01/03/2018 08:29 AM, Sudeep Holla wrote:
>>
>> On 02/01/18 02:29, Xiongfeng Wang wrote:
>>> Hi,
>>>
>>> On 2017/12/18 20:42, Morten Rasmussen wrote:
>>>> On Fri, Dec 15, 2017 at 10:36:35AM -0600, Jeremy Linton wrote:
>>>>> Hi,
>>>>>
>>>>> On 12/13/2017 12:02 PM, Lorenzo Pieralisi wrote:
>>>>>> [+Morten, Dietmar]
>>>>>>
>>>>>> $SUBJECT should be:
>>>>>>
>>>>>> arm64: topology: rename cluster_id
>>>>>
>>> [cut]
>>>>>
>>> I think we still need the information describing which cores are in one
>>> cluster. Many arm64 chips have the architecture core/cluster/socket. Cores
>>> in one cluster may share a same L2 cache. That information can be used to
>>> build the sched_domain. If we put cores in one cluster in one sched_domain,
>>> the performance will be better.(please see kernel/sched/topology.c:1197,
>>> cpu_coregroup_mask() uses 'core_sibling' to build a multi-core
>>> sched_domain).
>>
>> We get all the cache information from DT/ACPI PPTT(mainly topology) and now
>> even the geometry. So ideally, the sharing information must come from that.
>> Any other solution might end up in conflict if DT/PPTT and that mismatch.
>>
>>> So I think we still need variable to record which cores are in one
>>> sched_domain for future use.
>>
>> I tend to say no, at-least not as is.
>>
> 
> Well, either way, with DynamiQ (and a55/a75) the cores have private L2's, which means that the cluster sharing is happening at what is then the L3 level. So, the code I had in earlier versions would have needed tweaks to deal with that anyway.
> 
> IMHO, if we want to detect this kind of sharing for future scheduling domains, it should probably be done independent of PPTT/DT/MIPDR by picking out shared cache levels from struct cacheinfo *. Which makes that change unrelated to the basic population of cacheinfo and cpu_topology in this patchset.
> 
I think we need to build scheduling domains not only on the cache-sharing information,
but also some other information, such as which cores use the same cache coherent interconnect
(I don't know the detail, I just guess)

I think PPTT is used to report the cores topology, which cores are more related to each other.
They may share the same cache, or use the same CCI, or are physically near to each other.
I think we should use this information to build  MC(multi-cores) scheduling domains.

Or maybe  we can just discard the MC scheduling domain and handle this scheduling-domain-building
task to the NUMA subsystem entirely, I don't know if it is proper.

Thanks,
Xiongfeng

> 
> .
> 

Re: [PATCH v5 7/9] arm64: Topology, rename cluster_id

[Xiongfeng Wang]

On 2018/1/4 1:32, Jeremy Linton wrote:
> Hi,
> 
> On 01/03/2018 08:29 AM, Sudeep Holla wrote:
>>
>> On 02/01/18 02:29, Xiongfeng Wang wrote:
>>> Hi,
>>>
>>> On 2017/12/18 20:42, Morten Rasmussen wrote:
>>>> On Fri, Dec 15, 2017 at 10:36:35AM -0600, Jeremy Linton wrote:
>>>>> Hi,
>>>>>
>>>>> On 12/13/2017 12:02 PM, Lorenzo Pieralisi wrote:
>>>>>> [+Morten, Dietmar]
>>>>>>
>>>>>> $SUBJECT should be:
>>>>>>
>>>>>> arm64: topology: rename cluster_id
>>>>>
>>> [cut]
>>>>>
>>> I think we still need the information describing which cores are in one
>>> cluster. Many arm64 chips have the architecture core/cluster/socket. Cores
>>> in one cluster may share a same L2 cache. That information can be used to
>>> build the sched_domain. If we put cores in one cluster in one sched_domain,
>>> the performance will be better.(please see kernel/sched/topology.c:1197,
>>> cpu_coregroup_mask() uses 'core_sibling' to build a multi-core
>>> sched_domain).
>>
>> We get all the cache information from DT/ACPI PPTT(mainly topology) and now
>> even the geometry. So ideally, the sharing information must come from that.
>> Any other solution might end up in conflict if DT/PPTT and that mismatch.

Sorry, I didn't express myself clearly. There may be some misunderstanding above.
I mean that PPTT report the cores topology, such as a level of the topology tree maybe  cores in one cluster,
another level maybe cores in one package.
We not only need variable in 'struct topology' to record which cores are in one package,
but also need variable to record which cores are in one cluster.
>>
>>> So I think we still need variable to record which cores are in one
>>> sched_domain for future use.
>>
>> I tend to say no, at-least not as is.
>>
> 
> Well, either way, with DynamiQ (and a55/a75) the cores have private L2's, which means that the cluster sharing is happening at what is then the L3 level. So, the code I had in earlier versions would have needed tweaks to deal with that anyway.
> 
> IMHO, if we want to detect this kind of sharing for future scheduling domains, it should probably be done independent of PPTT/DT/MIPDR by picking out shared cache levels from struct cacheinfo *. Which makes that change unrelated to the basic population of cacheinfo and cpu_topology in this patchset.
> 
> 
> .
> 

RE: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code

[vkilari]

Hi Jeremy

> -----Original Message-----
> From: linux-arm-kernel
[mailto:linux-arm-kernel-bounces@lists.infradead.org]
> On Behalf Of Jeremy Linton
> Sent: Wednesday, January 3, 2018 10:28 PM
> To: vkilari@codeaurora.org
> Cc: 'Mark Rutland' <mark.rutland@arm.com>; Jonathan.Zhang@cavium.com;
> Jayachandran.Nair@cavium.com; 'Lorenzo Pieralisi'
> <lorenzo.pieralisi@arm.com>; austinwc@codeaurora.org; 'Linux PM' <linux-
> pm@vger.kernel.org>; jhugo@codeaurora.org; 'Catalin Marinas'
> <catalin.marinas@arm.com>; 'Sudeep Holla' <sudeep.holla@arm.com>; 'Will
> Deacon' <will.deacon@arm.com>; 'Linux Kernel Mailing List' <linux-
> kernel@vger.kernel.org>; wangxiongfeng2@huawei.com; 'ACPI Devel Maling
> List' <linux-acpi@vger.kernel.org>; 'Viresh Kumar'
<viresh.kumar@linaro.org>;
> 'Rafael J. Wysocki' <rjw@rjwysocki.net>; 'Hanjun Guo'
> <hanjun.guo@linaro.org>; 'Greg Kroah-Hartman'
> <gregkh@linuxfoundation.org>; 'Rafael J. Wysocki' <rafael@kernel.org>; 'Al
> Stone' <ahs3@redhat.com>; linux-arm-kernel@lists.infradead.org; 'Len
Brown'
> <lenb@kernel.org>
> Subject: Re: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code
> 
> Hi,
> 
> On 01/03/2018 02:49 AM, vkilari@codeaurora.org wrote:
> > Hi Jeremy,
> >
> >   Sorry, I don't have your previous patch emails to reply on right
> > patch context.
> > So commenting on top of this patch.
> >
> > AFAIU, the PPTT v5 patches still rely on CLIDR_EL1 register to know
> > the type of Caches enabled/available on the platform. With PPTT, it
> > should not rely on architecture registers. There can be platforms
> > which can report cache availability in PPTT but not in architecture
> > registers.
> >
> > The following code snippet shows usage of CLIDR_EL1
> >
> > In arch/arm64/kernel/cacheinfo.c
> >
> > static inline enum cache_type get_cache_type(int level) {
> >           u64 clidr;
> >
> >           if (level > MAX_CACHE_LEVEL)
> >                   return CACHE_TYPE_NOCACHE;
> >           clidr = read_sysreg(clidr_el1);
> >           return CLIDR_CTYPE(clidr, level); }
> >
> > static int __populate_cache_leaves(unsigned int cpu) {
> >            unsigned int level, idx;
> >            enum cache_type type;
> >            struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
> >            struct cacheinfo *this_leaf = this_cpu_ci->info_list;
> >
> >            for (idx = 0, level = 1; level <= this_cpu_ci->num_levels &&
> >                 idx < this_cpu_ci->num_leaves; idx++, level++) {
> >                    type = get_cache_type(level);
> >                    if (type == CACHE_TYPE_SEPARATE) {
> >                            ci_leaf_init(this_leaf++, CACHE_TYPE_DATA,
level);
> >                            ci_leaf_init(this_leaf++, CACHE_TYPE_INST,
level);
> >                    } else {
> >                            ci_leaf_init(this_leaf++, type, level);
> >                    }
> >           }
> >            return 0;
> >   }
> >
> > In populate_cache_leaves() the cache type is read from CLIDR_EL1
register.
> > If CLIDR_EL1 reports CACHE_TYPE_NOCACHE for a particular level then
> > sysfs entry /sys/devices/system/cpu/cpu0/index<n>/type is not created
> > and hence userspace tools like lstopo will not report this cache
> > level.
> 
> 
> This sounds suspiciously like one of things tweaked between v4->v5. If you
look
> at update_cache_properties() in patch 2/9, you will see that we only
> update/find NOCACHE nodes and convert them to UNIFIED when all the
> attributes in the node are supplied.
> 
> This means that if the node has an incomplete set of attributes we won't
update
> it. Can you verify that you have all those attributes set for nodes which
aren't
> being described by the hardware?

Thanks for pointing out.
Why do we need to check for set of attributes and decide it as UNIFIED
cache.?
We can get cache type from attributes bits[3:2] if cache type valid flag is
set
irrespective of other attributes. If cache type valid flag is not set then
we can assume
it as NOCACHE type as neither architecture register nor in PPTT has valid
cache type.

> 
> Thanks,
> 
> 
> >
> > Regards
> > Vijay
> >
> >> -----Original Message-----
> >> From: linux-arm-kernel
> > [mailto:linux-arm-kernel-bounces@lists.infradead.org]
> >> On Behalf Of Rafael J. Wysocki
> >> Sent: Thursday, December 14, 2017 4:40 AM
> >> To: Jeremy Linton <jeremy.linton@arm.com>
> >> Cc: Mark Rutland <mark.rutland@arm.com>; Jonathan.Zhang@cavium.com;
> >> Jayachandran.Nair@cavium.com; Lorenzo Pieralisi
> >> <lorenzo.pieralisi@arm.com>; Catalin Marinas
> >> <catalin.marinas@arm.com>; Rafael J. Wysocki <rafael@kernel.org>;
> >> jhugo@codeaurora.org; Will Deacon <will.deacon@arm.com>; Linux PM
> <linux-pm@vger.kernel.org>; Rafael J.
> >> Wysocki <rjw@rjwysocki.net>; Greg Kroah-Hartman
> >> <gregkh@linuxfoundation.org>; Linux Kernel Mailing List <linux-
> >> kernel@vger.kernel.org>; ACPI Devel Maling List
> > <linux-acpi@vger.kernel.org>;
> >> Viresh Kumar <viresh.kumar@linaro.org>; Hanjun Guo
> >> <hanjun.guo@linaro.org>; Al Stone <ahs3@redhat.com>; Sudeep Holla
> >> <sudeep.holla@arm.com>; austinwc@codeaurora.org;
> >> wangxiongfeng2@huawei.com; linux-arm-kernel@lists.infradead.org; Len
> >> Brown <lenb@kernel.org>
> >> Subject: Re: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code
> >>
> >> On Thu, Dec 14, 2017 at 12:06 AM, Jeremy Linton
> >> <jeremy.linton@arm.com>
> >> wrote:
> >>> Hi,
> >>>
> >>>
> >>> On 12/13/2017 04:28 PM, Rafael J. Wysocki wrote:
> >>>>
> >>>> On Wed, Dec 13, 2017 at 6:38 PM, Lorenzo Pieralisi
> >>>> <lorenzo.pieralisi@arm.com> wrote:
> >>>>>
> >>>>> On Tue, Dec 12, 2017 at 10:13:08AM -0600, Jeremy Linton wrote:
> >>>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>> First, thanks for taking a look at this.
> >>>>>>
> >>>>>> On 12/11/2017 07:12 PM, Rafael J. Wysocki wrote:
> >>>>>>>
> >>>>>>> On Friday, December 1, 2017 11:23:27 PM CET Jeremy Linton wrote:
> >>>>>>>>
> >>>>>>>> The PPTT can be used to determine the groupings of CPU's at
> >>>>>>>> given levels in the system. Lets add a few routines to the PPTT
> >>>>>>>> parsing code to return a unique id for each unique level in the
> >>>>>>>> processor hierarchy. This can then be matched to build
> >>>>>>>> thread/core/cluster/die/package/etc mappings for each
> >>>>>>>> processing element in the system.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
> >>>>>>>
> >>>>>>>
> >>>>>>> Why can't this be folded into patch [2/9]?
> >>>>>>
> >>>>>>
> >>>>>> It can, and I will be happy squash it.
> >>>>>>
> >>>>>> It was requested that the topology portion of the parser be split
> >>>>>> out back in v3.
> >>>>>>
> >>>>>> https://www.spinics.net/lists/linux-acpi/msg78487.html
> >>>>>
> >>>>>
> >>>>> I asked to split cache/topology since I am not familiar with cache
> >>>>> code and Sudeep - who looks after the cache code - won't be able
> >>>>> to review this series in time for v4.16.
> >>>>
> >>>>
> >>>> OK, so why do we need it in 4.16?
> >>>
> >>>
> >>> I think its more case of as soon as possible. That is because there
> >>> are machines where the topology is completely incorrect due to
> >>> assumptions the kernel makes based on registers that aren't defined
> >>> for that purpose (say describing which cores are in a physical
> >>> socket, or LLC's attached to interconnects or memory controllers).
> >>>
> >>> This incorrect topology information is reported to things like the
> >>> kernel scheduler, which then makes poor scheduling decisions
> >>> resulting in sub-optimal system performance.
> >>>
> >>> This patchset (and ACPI 6.2) clears up a lot of those problems.
> >>
> >> As long as the ACPI tables are as expected that is, I suppose?
> >>
> >> Anyway, fair enough, but I don't want to rush it in.
> >>
> >> Thanks,
> >> Rafael
> >>
> >> _______________________________________________
> >> linux-arm-kernel mailing list
> >> linux-arm-kernel@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 4/9] drivers: base cacheinfo: Add support for ACPI based firmware tables

[Sudeep Holla]

On 03/01/18 14:21, Sudeep Holla wrote:
> On 12/12/17 23:37, Jeremy Linton wrote:
>> On 12/12/2017 05:02 PM, Rafael J. Wysocki wrote:
> 
> [...]
> 
>>>
>>> So call this field "token" or similar.  Don't call it "of_node" and
>>> don't introduce another "firmware_node" thing in addition to that.
>>> That just is a mess, sorry.
> 
> I completely agree. Both me and Lorenzo pointed that out in previous
> revisions and fair enough you have a valid concern it's use with PPTT.
> 
>>
>> I sort of agree, I think I can just change the whole of_node to a
>> generic 'void *firmware_unique' which works fine for the PPTT code, it
>> should also work for the DT code in cache_leaves_are_shared().
>>
> 
> Should be fine.
> 

Just to let you know, I don't have much to add. I will wait for the
patches that replace of_node with firmware cookie or something similar.
-- 
Regards,
Sudeep

Re: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code

[Jeremy Linton]

Hi,

On 01/04/2018 12:48 AM, vkilari@codeaurora.org wrote:
> Hi Jeremy
> 
>> -----Original Message-----
>> From: linux-arm-kernel
> [mailto:linux-arm-kernel-bounces@lists.infradead.org]
>> On Behalf Of Jeremy Linton
>> Sent: Wednesday, January 3, 2018 10:28 PM
>> To: vkilari@codeaurora.org
>> Cc: 'Mark Rutland' <mark.rutland@arm.com>; Jonathan.Zhang@cavium.com;
>> Jayachandran.Nair@cavium.com; 'Lorenzo Pieralisi'
>> <lorenzo.pieralisi@arm.com>; austinwc@codeaurora.org; 'Linux PM' <linux-
>> pm@vger.kernel.org>; jhugo@codeaurora.org; 'Catalin Marinas'
>> <catalin.marinas@arm.com>; 'Sudeep Holla' <sudeep.holla@arm.com>; 'Will
>> Deacon' <will.deacon@arm.com>; 'Linux Kernel Mailing List' <linux-
>> kernel@vger.kernel.org>; wangxiongfeng2@huawei.com; 'ACPI Devel Maling
>> List' <linux-acpi@vger.kernel.org>; 'Viresh Kumar'
> <viresh.kumar@linaro.org>;
>> 'Rafael J. Wysocki' <rjw@rjwysocki.net>; 'Hanjun Guo'
>> <hanjun.guo@linaro.org>; 'Greg Kroah-Hartman'
>> <gregkh@linuxfoundation.org>; 'Rafael J. Wysocki' <rafael@kernel.org>; 'Al
>> Stone' <ahs3@redhat.com>; linux-arm-kernel@lists.infradead.org; 'Len
> Brown'
>> <lenb@kernel.org>
>> Subject: Re: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code
>>
>> Hi,
>>
>> On 01/03/2018 02:49 AM, vkilari@codeaurora.org wrote:
>>> Hi Jeremy,
>>>
>>>    Sorry, I don't have your previous patch emails to reply on right
>>> patch context.
>>> So commenting on top of this patch.
>>>
>>> AFAIU, the PPTT v5 patches still rely on CLIDR_EL1 register to know
>>> the type of Caches enabled/available on the platform. With PPTT, it
>>> should not rely on architecture registers. There can be platforms
>>> which can report cache availability in PPTT but not in architecture
>>> registers.
>>>
>>> The following code snippet shows usage of CLIDR_EL1
>>>
>>> In arch/arm64/kernel/cacheinfo.c
>>>
>>> static inline enum cache_type get_cache_type(int level) {
>>>            u64 clidr;
>>>
>>>            if (level > MAX_CACHE_LEVEL)
>>>                    return CACHE_TYPE_NOCACHE;
>>>            clidr = read_sysreg(clidr_el1);
>>>            return CLIDR_CTYPE(clidr, level); }
>>>
>>> static int __populate_cache_leaves(unsigned int cpu) {
>>>             unsigned int level, idx;
>>>             enum cache_type type;
>>>             struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
>>>             struct cacheinfo *this_leaf = this_cpu_ci->info_list;
>>>
>>>             for (idx = 0, level = 1; level <= this_cpu_ci->num_levels &&
>>>                  idx < this_cpu_ci->num_leaves; idx++, level++) {
>>>                     type = get_cache_type(level);
>>>                     if (type == CACHE_TYPE_SEPARATE) {
>>>                             ci_leaf_init(this_leaf++, CACHE_TYPE_DATA,
> level);
>>>                             ci_leaf_init(this_leaf++, CACHE_TYPE_INST,
> level);
>>>                     } else {
>>>                             ci_leaf_init(this_leaf++, type, level);
>>>                     }
>>>            }
>>>             return 0;
>>>    }
>>>
>>> In populate_cache_leaves() the cache type is read from CLIDR_EL1
> register.
>>> If CLIDR_EL1 reports CACHE_TYPE_NOCACHE for a particular level then
>>> sysfs entry /sys/devices/system/cpu/cpu0/index<n>/type is not created
>>> and hence userspace tools like lstopo will not report this cache
>>> level.
>>
>>
>> This sounds suspiciously like one of things tweaked between v4->v5. If you
> look
>> at update_cache_properties() in patch 2/9, you will see that we only
>> update/find NOCACHE nodes and convert them to UNIFIED when all the
>> attributes in the node are supplied.
>>
>> This means that if the node has an incomplete set of attributes we won't
> update
>> it. Can you verify that you have all those attributes set for nodes which
> aren't
>> being described by the hardware?
> 
> Thanks for pointing out.
> Why do we need to check for set of attributes and decide it as UNIFIED
> cache.?
> We can get cache type from attributes bits[3:2] if cache type valid flag is
> set
> irrespective of other attributes. If cache type valid flag is not set then
> we can assume
> it as NOCACHE type as neither architecture register nor in PPTT has valid
> cache type.

To answer the first question, in a strict sense we don't need to check 
any of the attributes in order to override the cache type. That said, 
initially I was going to trigger the override only when important 
attributes were set to assure that we weren't exporting meaningless 
nodes into sysfs. Then while picking which attributes I considered 
important, I came to the conclusion that it was simply better to assure 
that they were all set for nodes entirely generated by the PPTT. AKA, I 
don't want to see L3 cache nodes with their size or associativity unset, 
its better in that case that they remain hidden.

Per, the cache type valid bit. The code is written with the assumption 
that it is overriding probed values (despite that not being true at the 
moment for arm64) in the spirit of the standard. This informs/restricts 
how the code works because we aren't simply generating the entire 
cacheinfo directly from PPTT walks. Instead we are merging the PPTT 
information with anything previously probed, meaning we need a way to 
match existing cacheinfo structures with PPTT nodes.

So, the logic finding/matching an existing probed cache node requires 
that the cache type is valid because the cache level, and type is used 
as the match key. If the PPTT cache node doesn't have the cache type 
valid set, then the match logic won't find the node, and the PPTT code 
won't make any updates. That may also be what your seeing.. Basically 
what is happening is that cacheinfo NOCACHE nodes that happen to match 
valid PPTT UNIFIED nodes, can have their cache types overridden, but 
only if we determine the remainder of the PPTT node has sufficient 
information that we aren't exporting cacheinfo structures without useful 
information. Currently, the only time this can happen is for nodes which 
are entirely PPTT generated, so I think its fair the PPTT contain enough 
information to make those nodes useful.

Thanks,



> 
>>
>> Thanks,
>>
>>
>>>
>>> Regards
>>> Vijay
>>>
>>>> -----Original Message-----
>>>> From: linux-arm-kernel
>>> [mailto:linux-arm-kernel-bounces@lists.infradead.org]
>>>> On Behalf Of Rafael J. Wysocki
>>>> Sent: Thursday, December 14, 2017 4:40 AM
>>>> To: Jeremy Linton <jeremy.linton@arm.com>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>; Jonathan.Zhang@cavium.com;
>>>> Jayachandran.Nair@cavium.com; Lorenzo Pieralisi
>>>> <lorenzo.pieralisi@arm.com>; Catalin Marinas
>>>> <catalin.marinas@arm.com>; Rafael J. Wysocki <rafael@kernel.org>;
>>>> jhugo@codeaurora.org; Will Deacon <will.deacon@arm.com>; Linux PM
>> <linux-pm@vger.kernel.org>; Rafael J.
>>>> Wysocki <rjw@rjwysocki.net>; Greg Kroah-Hartman
>>>> <gregkh@linuxfoundation.org>; Linux Kernel Mailing List <linux-
>>>> kernel@vger.kernel.org>; ACPI Devel Maling List
>>> <linux-acpi@vger.kernel.org>;
>>>> Viresh Kumar <viresh.kumar@linaro.org>; Hanjun Guo
>>>> <hanjun.guo@linaro.org>; Al Stone <ahs3@redhat.com>; Sudeep Holla
>>>> <sudeep.holla@arm.com>; austinwc@codeaurora.org;
>>>> wangxiongfeng2@huawei.com; linux-arm-kernel@lists.infradead.org; Len
>>>> Brown <lenb@kernel.org>
>>>> Subject: Re: [PATCH v5 6/9] ACPI/PPTT: Add topology parsing code
>>>>
>>>> On Thu, Dec 14, 2017 at 12:06 AM, Jeremy Linton
>>>> <jeremy.linton@arm.com>
>>>> wrote:
>>>>> Hi,
>>>>>
>>>>>
>>>>> On 12/13/2017 04:28 PM, Rafael J. Wysocki wrote:
>>>>>>
>>>>>> On Wed, Dec 13, 2017 at 6:38 PM, Lorenzo Pieralisi
>>>>>> <lorenzo.pieralisi@arm.com> wrote:
>>>>>>>
>>>>>>> On Tue, Dec 12, 2017 at 10:13:08AM -0600, Jeremy Linton wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> First, thanks for taking a look at this.
>>>>>>>>
>>>>>>>> On 12/11/2017 07:12 PM, Rafael J. Wysocki wrote:
>>>>>>>>>
>>>>>>>>> On Friday, December 1, 2017 11:23:27 PM CET Jeremy Linton wrote:
>>>>>>>>>>
>>>>>>>>>> The PPTT can be used to determine the groupings of CPU's at
>>>>>>>>>> given levels in the system. Lets add a few routines to the PPTT
>>>>>>>>>> parsing code to return a unique id for each unique level in the
>>>>>>>>>> processor hierarchy. This can then be matched to build
>>>>>>>>>> thread/core/cluster/die/package/etc mappings for each
>>>>>>>>>> processing element in the system.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Why can't this be folded into patch [2/9]?
>>>>>>>>
>>>>>>>>
>>>>>>>> It can, and I will be happy squash it.
>>>>>>>>
>>>>>>>> It was requested that the topology portion of the parser be split
>>>>>>>> out back in v3.
>>>>>>>>
>>>>>>>> https://www.spinics.net/lists/linux-acpi/msg78487.html
>>>>>>>
>>>>>>>
>>>>>>> I asked to split cache/topology since I am not familiar with cache
>>>>>>> code and Sudeep - who looks after the cache code - won't be able
>>>>>>> to review this series in time for v4.16.
>>>>>>
>>>>>>
>>>>>> OK, so why do we need it in 4.16?
>>>>>
>>>>>
>>>>> I think its more case of as soon as possible. That is because there
>>>>> are machines where the topology is completely incorrect due to
>>>>> assumptions the kernel makes based on registers that aren't defined
>>>>> for that purpose (say describing which cores are in a physical
>>>>> socket, or LLC's attached to interconnects or memory controllers).
>>>>>
>>>>> This incorrect topology information is reported to things like the
>>>>> kernel scheduler, which then makes poor scheduling decisions
>>>>> resulting in sub-optimal system performance.
>>>>>
>>>>> This patchset (and ACPI 6.2) clears up a lot of those problems.
>>>>
>>>> As long as the ACPI tables are as expected that is, I suppose?
>>>>
>>>> Anyway, fair enough, but I don't want to rush it in.
>>>>
>>>> Thanks,
>>>> Rafael
>>>>
>>>> _______________________________________________
>>>> linux-arm-kernel mailing list
>>>> linux-arm-kernel@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH v5 7/9] arm64: Topology, rename cluster_id

[Jeremy Linton]

Hi,

On 01/03/2018 09:59 PM, Xiongfeng Wang wrote:
> 
> 
> On 2018/1/4 1:32, Jeremy Linton wrote:
>> Hi,
>>
>> On 01/03/2018 08:29 AM, Sudeep Holla wrote:
>>>
>>> On 02/01/18 02:29, Xiongfeng Wang wrote:
>>>> Hi,
>>>>
>>>> On 2017/12/18 20:42, Morten Rasmussen wrote:
>>>>> On Fri, Dec 15, 2017 at 10:36:35AM -0600, Jeremy Linton wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 12/13/2017 12:02 PM, Lorenzo Pieralisi wrote:
>>>>>>> [+Morten, Dietmar]
>>>>>>>
>>>>>>> $SUBJECT should be:
>>>>>>>
>>>>>>> arm64: topology: rename cluster_id
>>>>>>
>>>> [cut]
>>>>>>
>>>> I think we still need the information describing which cores are in one
>>>> cluster. Many arm64 chips have the architecture core/cluster/socket. Cores
>>>> in one cluster may share a same L2 cache. That information can be used to
>>>> build the sched_domain. If we put cores in one cluster in one sched_domain,
>>>> the performance will be better.(please see kernel/sched/topology.c:1197,
>>>> cpu_coregroup_mask() uses 'core_sibling' to build a multi-core
>>>> sched_domain).
>>>
>>> We get all the cache information from DT/ACPI PPTT(mainly topology) and now
>>> even the geometry. So ideally, the sharing information must come from that.
>>> Any other solution might end up in conflict if DT/PPTT and that mismatch.
>>>
>>>> So I think we still need variable to record which cores are in one
>>>> sched_domain for future use.
>>>
>>> I tend to say no, at-least not as is.
>>>
>>
>> Well, either way, with DynamiQ (and a55/a75) the cores have private L2's, which means that the cluster sharing is happening at what is then the L3 level. So, the code I had in earlier versions would have needed tweaks to deal with that anyway.
>>
>> IMHO, if we want to detect this kind of sharing for future scheduling domains, it should probably be done independent of PPTT/DT/MIPDR by picking out shared cache levels from struct cacheinfo *. Which makes that change unrelated to the basic population of cacheinfo and cpu_topology in this patchset.
>>
> I think we need to build scheduling domains not only on the cache-sharing information,
> but also some other information, such as which cores use the same cache coherent interconnect
> (I don't know the detail, I just guess)
> 
> I think PPTT is used to report the cores topology, which cores are more related to each other.
> They may share the same cache, or use the same CCI, or are physically near to each other.
> I think we should use this information to build  MC(multi-cores) scheduling domains.
> 
> Or maybe  we can just discard the MC scheduling domain and handle this scheduling-domain-building
> task to the NUMA subsystem entirely, I don't know if it is proper.


For the immediate future what I would like is a way to identify where in 
the PPTT topology the NUMA domains begin (rather than assuming socket, 
which is the current plan). That allows the manufactures of systems 
(with say say MCM based topologies) to dictate at which level in the 
cpu/cache topology they want to start describing the topology with the 
SLIT/SRAT tables. I think that moves us in the direction you are 
indicating while still leaving the door open for something like a 
cluster level scheduling domain (based on cores sharing caches) or a 
split LLC domain (also based on cores sharing caches) that happens to be 
on die...

Re: [PATCH v5 3/9] ACPI: Enable PPTT support on ARM64

[Jeremy Linton]

Hi,

On 12/13/2017 11:26 AM, Lorenzo Pieralisi wrote:
> On Fri, Dec 01, 2017 at 04:23:24PM -0600, Jeremy Linton wrote:
>> Now that we have a PPTT parser, in preparation for its use
>> on arm64, lets build it.
>>
>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>> ---
>>   arch/arm64/Kconfig    | 1 +
>>   drivers/acpi/Kconfig  | 3 +++
>>   drivers/acpi/Makefile | 1 +
>>   3 files changed, 5 insertions(+)
>>
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index a93339f5178f..e62fd1e08c1f 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -7,6 +7,7 @@ config ARM64
>>   	select ACPI_REDUCED_HARDWARE_ONLY if ACPI
>>   	select ACPI_MCFG if ACPI
>>   	select ACPI_SPCR_TABLE if ACPI
>> +	select ACPI_PPTT if ACPI
>>   	select ARCH_CLOCKSOURCE_DATA
>>   	select ARCH_HAS_DEBUG_VIRTUAL
>>   	select ARCH_HAS_DEVMEM_IS_ALLOWED
>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>> index 46505396869e..df7aebf0af0e 100644
>> --- a/drivers/acpi/Kconfig
>> +++ b/drivers/acpi/Kconfig
>> @@ -545,6 +545,9 @@ config ACPI_CONFIGFS
>>   
>>   if ARM64
>>   source "drivers/acpi/arm64/Kconfig"
>> +
>> +config ACPI_PPTT
>> +	bool
> 
> We need to make a choice here. Either PPTT is considered ARM64 only and
> we move code and config to drivers/acpi/arm64/Kconfig or we leave it in
> drivers/acpi/pptt.c and we add a Kconfig entry in drivers/acpi/Kconfig
> (and we make pptt.c compile on !ARM64 - which is what it should be given
> that there is nothing ARM64 specific in it).

No one else has expressed and opinion about this here..

So i'm not sure what to think.

OTOH a lot of people didn't like it when I had it in the arm64 
directory, which was my original opinion. It seems other people thought 
that at some point in the future other ACPI platforms would want to use 
it so putting it in the arm64 directory was a mistake.

So, I'm leaning towards leaving it like it is, under the assumption that 
when someone puts in the effort to verify it on another ACPI platform 
they can move the config option up 6 lines. In the meantime I don't 
think it should be enabled on platforms where it hasn't been tested or 
is basically blocked (cpu_cacheinfo->cpu_map_populated) from executing 
or there isn't currently any benefit.

Hence I'm planning on leaving it as is until I get further clarity.


> 
> Lorenzo
> 
>>   endif
>>   
>>   config TPS68470_PMIC_OPREGION
>> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
>> index 41954a601989..b6056b566df4 100644
>> --- a/drivers/acpi/Makefile
>> +++ b/drivers/acpi/Makefile
>> @@ -87,6 +87,7 @@ obj-$(CONFIG_ACPI_BGRT)		+= bgrt.o
>>   obj-$(CONFIG_ACPI_CPPC_LIB)	+= cppc_acpi.o
>>   obj-$(CONFIG_ACPI_SPCR_TABLE)	+= spcr.o
>>   obj-$(CONFIG_ACPI_DEBUGGER_USER) += acpi_dbg.o
>> +obj-$(CONFIG_ACPI_PPTT) 	+= pptt.o
>>   
>>   # processor has its own "processor." module_param namespace
>>   processor-y			:= processor_driver.o
>> -- 
>> 2.13.5
>>

Re: [PATCH v5 3/9] ACPI: Enable PPTT support on ARM64

[Rafael J. Wysocki]

On Fri, Jan 5, 2018 at 10:58 PM, Jeremy Linton <jeremy.linton@arm.com> wrote:
> Hi,
>
>
> On 12/13/2017 11:26 AM, Lorenzo Pieralisi wrote:
>>
>> On Fri, Dec 01, 2017 at 04:23:24PM -0600, Jeremy Linton wrote:
>>>
>>> Now that we have a PPTT parser, in preparation for its use
>>> on arm64, lets build it.
>>>
>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>>> ---
>>>   arch/arm64/Kconfig    | 1 +
>>>   drivers/acpi/Kconfig  | 3 +++
>>>   drivers/acpi/Makefile | 1 +
>>>   3 files changed, 5 insertions(+)
>>>
>>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>>> index a93339f5178f..e62fd1e08c1f 100644
>>> --- a/arch/arm64/Kconfig
>>> +++ b/arch/arm64/Kconfig
>>> @@ -7,6 +7,7 @@ config ARM64
>>>         select ACPI_REDUCED_HARDWARE_ONLY if ACPI
>>>         select ACPI_MCFG if ACPI
>>>         select ACPI_SPCR_TABLE if ACPI
>>> +       select ACPI_PPTT if ACPI
>>>         select ARCH_CLOCKSOURCE_DATA
>>>         select ARCH_HAS_DEBUG_VIRTUAL
>>>         select ARCH_HAS_DEVMEM_IS_ALLOWED
>>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>>> index 46505396869e..df7aebf0af0e 100644
>>> --- a/drivers/acpi/Kconfig
>>> +++ b/drivers/acpi/Kconfig
>>> @@ -545,6 +545,9 @@ config ACPI_CONFIGFS
>>>     if ARM64
>>>   source "drivers/acpi/arm64/Kconfig"
>>> +
>>> +config ACPI_PPTT
>>> +       bool
>>
>>
>> We need to make a choice here. Either PPTT is considered ARM64 only and
>> we move code and config to drivers/acpi/arm64/Kconfig or we leave it in
>> drivers/acpi/pptt.c and we add a Kconfig entry in drivers/acpi/Kconfig
>> (and we make pptt.c compile on !ARM64 - which is what it should be given
>> that there is nothing ARM64 specific in it).
>
>
> No one else has expressed and opinion about this here..
>
> So i'm not sure what to think.
>
> OTOH a lot of people didn't like it when I had it in the arm64 directory,
> which was my original opinion. It seems other people thought that at some
> point in the future other ACPI platforms would want to use it so putting it
> in the arm64 directory was a mistake.

In my view it may go directly into drivers/acpi/ for the time being.

That said, going forward it may be useful to add a special
subdirectory under drivers/acpi/ for these "table drivers" as we seem
to be acquiring them at alarming rate.

> So, I'm leaning towards leaving it like it is, under the assumption that
> when someone puts in the effort to verify it on another ACPI platform they
> can move the config option up 6 lines. In the meantime I don't think it
> should be enabled on platforms where it hasn't been tested or is basically
> blocked (cpu_cacheinfo->cpu_map_populated) from executing or there isn't
> currently any benefit.

Agreed.

Thanks,
Rafael