[PATCH v2 0/9] Fix issues and factorize arm/arm64 capacity information code

[PATCH v2 0/9] Fix issues and factorize arm/arm64 capacity information code

[Juri Lelli]

Hi,

arm and arm64 topology.c share a lot of code related to parsing of capacity
information. This is v2 of a solution [1] (based on Will's, Catalin's and
Mark's off-line suggestions) to move such common code in a single place:
drivers/base/arch_topology.c (by creating such file and conditionally compiling
it for arm and arm64 only).

First 5 patches are actually fixes for the current code.

Patch 6 is the actual refactoring.

Patch 7 removes one of the extern symbols by changing a bit the now common
code.

Patch 8 removes the remaining externs (as required by Russell during v1 review)
by creating a new header file include/linux/arch_topology.h and including that
from arm, arm64 and drivers.

Last patch addresses Dietmar's comments to v1 and adds a 'atd_' prefix to
interfaces exported by drivers code and used by arch (and potentially others in
the future).

Changes from v1:

 - rebase on top of 4.10-rc7
 - fix licence issue as pointed out by Russell
 - propose a solution for removing all remaining externs
 - addressed Dietmar's comments regarding better namespaces

The set is based on top of linux/master (4.10-rc7 d5adbfcd5f7b) and it is also
available from:

 git://linux-arm.org/linux-jl.git upstream/default_caps_factorize-v2

Best,

- Juri

[1] v1 - https://marc.info/?l=linux-kernel&m=148483680119355&w=2

Juri Lelli (9):
  Documentation: arm: fix wrong reference number in DT definition
  Documentation/ABI: add information about cpu_capacity
  arm: fix return value of parse_cpu_capacity
  arm: remove wrong CONFIG_PROC_SYSCTL ifdef
  arm64: remove wrong CONFIG_PROC_SYSCTL ifdef
  arm, arm64: factorize common cpu capacity default code
  arm,arm64,drivers: reduce scope of cap_parsing_failed
  arm,arm64,drivers: move externs in a new header file
  arm,arm64,drivers: add a prefix to drivers arch_topology interfaces

 Documentation/ABI/testing/sysfs-devices-system-cpu |   7 +
 Documentation/devicetree/bindings/arm/cpus.txt     |   4 +-
 arch/arm/Kconfig                                   |   1 +
 arch/arm/kernel/topology.c                         | 221 +------------------
 arch/arm64/Kconfig                                 |   1 +
 arch/arm64/kernel/topology.c                       | 228 +-------------------
 drivers/base/Kconfig                               |   8 +
 drivers/base/Makefile                              |   1 +
 drivers/base/arch_topology.c                       | 238 +++++++++++++++++++++
 include/linux/arch_topology.h                      |  17 ++
 10 files changed, 285 insertions(+), 441 deletions(-)
 create mode 100644 drivers/base/arch_topology.c
 create mode 100644 include/linux/arch_topology.h

-- 
2.10.0

[PATCH v2 1/9] Documentation: arm: fix wrong reference number in DT definition

[Juri Lelli]

Reference to cpu capacity binding has a wrong number. Fix it.

Reported-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Juri Lelli <juri.lelli@arm.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/arm/cpus.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
index a1bcfeed5f24..c27376a27a92 100644
--- a/Documentation/devicetree/bindings/arm/cpus.txt
+++ b/Documentation/devicetree/bindings/arm/cpus.txt
@@ -246,7 +246,7 @@ nodes to be present and contain the properties described below.
 		Usage: Optional
 		Value type: <u32>
 		Definition:
-			# u32 value representing CPU capacity [3] in
+			# u32 value representing CPU capacity [4] in
 			  DMIPS/MHz, relative to highest capacity-dmips-mhz
 			  in the system.
 
@@ -473,5 +473,5 @@ cpus {
 [2] arm/msm/qcom,kpss-acc.txt
 [3] ARM Linux kernel documentation - idle states bindings
     Documentation/devicetree/bindings/arm/idle-states.txt
-[3] ARM Linux kernel documentation - cpu capacity bindings
+[4] ARM Linux kernel documentation - cpu capacity bindings
     Documentation/devicetree/bindings/arm/cpu-capacity.txt
-- 
2.10.0

[PATCH v2 2/9] Documentation/ABI: add information about cpu_capacity

[Juri Lelli]

/sys/devices/system/cpu/cpu#/cpu_capacity describe information about
CPUs heterogeneity (ref. to Documentation/devicetree/bindings/arm/
cpu-capacity.txt).

Add such description.

Signed-off-by: Juri Lelli <juri.lelli@arm.com>
---
 Documentation/ABI/testing/sysfs-devices-system-cpu | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-devices-system-cpu b/Documentation/ABI/testing/sysfs-devices-system-cpu
index 2a4a423d08e0..f3d5817c4ef0 100644
--- a/Documentation/ABI/testing/sysfs-devices-system-cpu
+++ b/Documentation/ABI/testing/sysfs-devices-system-cpu
@@ -366,3 +366,10 @@ Contact:	Linux ARM Kernel Mailing list <linux-arm-kernel@lists.infradead.org>
 Description:	AArch64 CPU registers
 		'identification' directory exposes the CPU ID registers for
 		 identifying model and revision of the CPU.
+
+What:		/sys/devices/system/cpu/cpu#/cpu_capacity
+Date:		December 2016
+Contact:	Linux kernel mailing list <linux-kernel@vger.kernel.org>
+Description:	information about CPUs heterogeneity.
+
+		cpu_capacity: capacity of cpu#.
-- 
2.10.0

[PATCH v2 3/9] arm: fix return value of parse_cpu_capacity

[Juri Lelli]

parse_cpu_capacity() has to return 0 on failure, but it currently returns
1 instead if raw_capacity kcalloc failed.

Fix it by removing the negation of the return value.

Cc: Russell King <linux@arm.linux.org.uk>
Reported-by: Morten Rasmussen <morten.rasmussen@arm.com>
Fixes: 06073ee26775 ('ARM: 8621/3: parse cpu capacity-dmips-mhz from DT')
Signed-off-by: Juri Lelli <juri.lelli@arm.com>
---
 arch/arm/kernel/topology.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
index ebf47d91b804..b439f7fff86b 100644
--- a/arch/arm/kernel/topology.c
+++ b/arch/arm/kernel/topology.c
@@ -165,7 +165,7 @@ static int __init parse_cpu_capacity(struct device_node *cpu_node, int cpu)
 			if (!raw_capacity) {
 				pr_err("cpu_capacity: failed to allocate memory for raw capacities\n");
 				cap_parsing_failed = true;
-				return !ret;
+				return ret;
 			}
 		}
 		capacity_scale = max(cpu_capacity, capacity_scale);
-- 
2.10.0

[PATCH v2 4/9] arm: remove wrong CONFIG_PROC_SYSCTL ifdef

[Juri Lelli]

The sysfs cpu_capacity entry for each CPU has nothing to do with
PROC_FS, nor it's in /proc/sys path.

Remove such ifdef.

Cc: Russell King <linux@arm.linux.org.uk>
Reported-and-suggested-by: Sudeep Holla <sudeep.holla@arm.com>
Fixes: 7e5930aaef5d ('ARM: 8622/3: add sysfs cpu_capacity attribute')
Signed-off-by: Juri Lelli <juri.lelli@arm.com>
---
 arch/arm/kernel/topology.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
index b439f7fff86b..c760a321935b 100644
--- a/arch/arm/kernel/topology.c
+++ b/arch/arm/kernel/topology.c
@@ -56,7 +56,6 @@ static void set_capacity_scale(unsigned int cpu, unsigned long capacity)
 	per_cpu(cpu_scale, cpu) = capacity;
 }
 
-#ifdef CONFIG_PROC_SYSCTL
 static ssize_t cpu_capacity_show(struct device *dev,
 				 struct device_attribute *attr,
 				 char *buf)
@@ -113,7 +112,6 @@ static int register_cpu_capacity_sysctl(void)
 	return 0;
 }
 subsys_initcall(register_cpu_capacity_sysctl);
-#endif
 
 #ifdef CONFIG_OF
 struct cpu_efficiency {
-- 
2.10.0

[PATCH v2 5/9] arm64: remove wrong CONFIG_PROC_SYSCTL ifdef

[Juri Lelli]

The sysfs cpu_capacity entry for each CPU has nothing to do with
PROC_FS, nor it's in /proc/sys path.

Remove such ifdef.

Cc: Will Deacon <will.deacon@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Reported-and-suggested-by: Sudeep Holla <sudeep.holla@arm.com>
Fixes: be8f185d8af4 ('arm64: add sysfs cpu_capacity attribute')
Signed-off-by: Juri Lelli <juri.lelli@arm.com>
---
 arch/arm64/kernel/topology.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index 655e65f38f31..565dd69888cc 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -41,7 +41,6 @@ static void set_capacity_scale(unsigned int cpu, unsigned long capacity)
 	per_cpu(cpu_scale, cpu) = capacity;
 }
 
-#ifdef CONFIG_PROC_SYSCTL
 static ssize_t cpu_capacity_show(struct device *dev,
 				 struct device_attribute *attr,
 				 char *buf)
@@ -98,7 +97,6 @@ static int register_cpu_capacity_sysctl(void)
 	return 0;
 }
 subsys_initcall(register_cpu_capacity_sysctl);
-#endif
 
 static u32 capacity_scale;
 static u32 *raw_capacity;
-- 
2.10.0

[PATCH v2 6/9] arm, arm64: factorize common cpu capacity default code

[Juri Lelli]

arm and arm64 share lot of code relative to parsing CPU capacity
information from DT, using that information for appropriate scaling and
exposing a sysfs interface for chaging such values at runtime.

Factorize such code in a common place (driver/base/arch_topology.c) in
preparation for further additions.

Suggested-by: Will Deacon <will.deacon@arm.com>
Suggested-by: Mark Rutland <mark.rutland@arm.com>
Suggested-by: Catalin Marinas <catalin.marinas@arm.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Juri Lelli <juri.lelli@arm.com>
---

Changes from v1:
 - keep the original GPLv2 header
---
 arch/arm/Kconfig             |   1 +
 arch/arm/kernel/topology.c   | 213 ++------------------------------------
 arch/arm64/Kconfig           |   1 +
 arch/arm64/kernel/topology.c | 219 +--------------------------------------
 drivers/base/Kconfig         |   8 ++
 drivers/base/Makefile        |   1 +
 drivers/base/arch_topology.c | 237 +++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 257 insertions(+), 423 deletions(-)
 create mode 100644 drivers/base/arch_topology.c

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 186c4c214e0a..6dd5736c1e3c 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -19,6 +19,7 @@ config ARM
 	select EDAC_SUPPORT
 	select EDAC_ATOMIC_SCRUB
 	select GENERIC_ALLOCATOR
+	select GENERIC_ARCH_TOPOLOGY if ARM_CPU_TOPOLOGY
 	select GENERIC_ATOMIC64 if (CPU_V7M || CPU_V6 || !CPU_32v6K || !AEABI)
 	select GENERIC_CLOCKEVENTS_BROADCAST if SMP
 	select GENERIC_EARLY_IOREMAP
diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
index c760a321935b..51e9ed6439f1 100644
--- a/arch/arm/kernel/topology.c
+++ b/arch/arm/kernel/topology.c
@@ -43,75 +43,10 @@
  * to run the rebalance_domains for all idle cores and the cpu_capacity can be
  * updated during this sequence.
  */
-static DEFINE_PER_CPU(unsigned long, cpu_scale) = SCHED_CAPACITY_SCALE;
-static DEFINE_MUTEX(cpu_scale_mutex);
 
-unsigned long arch_scale_cpu_capacity(struct sched_domain *sd, int cpu)
-{
-	return per_cpu(cpu_scale, cpu);
-}
-
-static void set_capacity_scale(unsigned int cpu, unsigned long capacity)
-{
-	per_cpu(cpu_scale, cpu) = capacity;
-}
-
-static ssize_t cpu_capacity_show(struct device *dev,
-				 struct device_attribute *attr,
-				 char *buf)
-{
-	struct cpu *cpu = container_of(dev, struct cpu, dev);
-
-	return sprintf(buf, "%lu\n",
-			arch_scale_cpu_capacity(NULL, cpu->dev.id));
-}
-
-static ssize_t cpu_capacity_store(struct device *dev,
-				  struct device_attribute *attr,
-				  const char *buf,
-				  size_t count)
-{
-	struct cpu *cpu = container_of(dev, struct cpu, dev);
-	int this_cpu = cpu->dev.id, i;
-	unsigned long new_capacity;
-	ssize_t ret;
-
-	if (count) {
-		ret = kstrtoul(buf, 0, &new_capacity);
-		if (ret)
-			return ret;
-		if (new_capacity > SCHED_CAPACITY_SCALE)
-			return -EINVAL;
-
-		mutex_lock(&cpu_scale_mutex);
-		for_each_cpu(i, &cpu_topology[this_cpu].core_sibling)
-			set_capacity_scale(i, new_capacity);
-		mutex_unlock(&cpu_scale_mutex);
-	}
-
-	return count;
-}
-
-static DEVICE_ATTR_RW(cpu_capacity);
-
-static int register_cpu_capacity_sysctl(void)
-{
-	int i;
-	struct device *cpu;
-
-	for_each_possible_cpu(i) {
-		cpu = get_cpu_device(i);
-		if (!cpu) {
-			pr_err("%s: too early to get CPU%d device!\n",
-			       __func__, i);
-			continue;
-		}
-		device_create_file(cpu, &dev_attr_cpu_capacity);
-	}
-
-	return 0;
-}
-subsys_initcall(register_cpu_capacity_sysctl);
+extern unsigned long
+arch_scale_cpu_capacity(struct sched_domain *sd, int cpu);
+extern void set_capacity_scale(unsigned int cpu, unsigned long capacity);
 
 #ifdef CONFIG_OF
 struct cpu_efficiency {
@@ -140,145 +75,9 @@ static unsigned long *__cpu_capacity;
 
 static unsigned long middle_capacity = 1;
 static bool cap_from_dt = true;
-static u32 *raw_capacity;
-static bool cap_parsing_failed;
-static u32 capacity_scale;
-
-static int __init parse_cpu_capacity(struct device_node *cpu_node, int cpu)
-{
-	int ret = 1;
-	u32 cpu_capacity;
-
-	if (cap_parsing_failed)
-		return !ret;
-
-	ret = of_property_read_u32(cpu_node,
-				   "capacity-dmips-mhz",
-				   &cpu_capacity);
-	if (!ret) {
-		if (!raw_capacity) {
-			raw_capacity = kcalloc(num_possible_cpus(),
-					       sizeof(*raw_capacity),
-					       GFP_KERNEL);
-			if (!raw_capacity) {
-				pr_err("cpu_capacity: failed to allocate memory for raw capacities\n");
-				cap_parsing_failed = true;
-				return ret;
-			}
-		}
-		capacity_scale = max(cpu_capacity, capacity_scale);
-		raw_capacity[cpu] = cpu_capacity;
-		pr_debug("cpu_capacity: %s cpu_capacity=%u (raw)\n",
-			cpu_node->full_name, raw_capacity[cpu]);
-	} else {
-		if (raw_capacity) {
-			pr_err("cpu_capacity: missing %s raw capacity\n",
-				cpu_node->full_name);
-			pr_err("cpu_capacity: partial information: fallback to 1024 for all CPUs\n");
-		}
-		cap_parsing_failed = true;
-		kfree(raw_capacity);
-	}
-
-	return !ret;
-}
-
-static void normalize_cpu_capacity(void)
-{
-	u64 capacity;
-	int cpu;
-
-	if (!raw_capacity || cap_parsing_failed)
-		return;
-
-	pr_debug("cpu_capacity: capacity_scale=%u\n", capacity_scale);
-	mutex_lock(&cpu_scale_mutex);
-	for_each_possible_cpu(cpu) {
-		capacity = (raw_capacity[cpu] << SCHED_CAPACITY_SHIFT)
-			/ capacity_scale;
-		set_capacity_scale(cpu, capacity);
-		pr_debug("cpu_capacity: CPU%d cpu_capacity=%lu\n",
-			cpu, arch_scale_cpu_capacity(NULL, cpu));
-	}
-	mutex_unlock(&cpu_scale_mutex);
-}
-
-#ifdef CONFIG_CPU_FREQ
-static cpumask_var_t cpus_to_visit;
-static bool cap_parsing_done;
-static void parsing_done_workfn(struct work_struct *work);
-static DECLARE_WORK(parsing_done_work, parsing_done_workfn);
-
-static int
-init_cpu_capacity_callback(struct notifier_block *nb,
-			   unsigned long val,
-			   void *data)
-{
-	struct cpufreq_policy *policy = data;
-	int cpu;
-
-	if (cap_parsing_failed || cap_parsing_done)
-		return 0;
-
-	switch (val) {
-	case CPUFREQ_NOTIFY:
-		pr_debug("cpu_capacity: init cpu capacity for CPUs [%*pbl] (to_visit=%*pbl)\n",
-				cpumask_pr_args(policy->related_cpus),
-				cpumask_pr_args(cpus_to_visit));
-		cpumask_andnot(cpus_to_visit,
-			       cpus_to_visit,
-			       policy->related_cpus);
-		for_each_cpu(cpu, policy->related_cpus) {
-			raw_capacity[cpu] = arch_scale_cpu_capacity(NULL, cpu) *
-					    policy->cpuinfo.max_freq / 1000UL;
-			capacity_scale = max(raw_capacity[cpu], capacity_scale);
-		}
-		if (cpumask_empty(cpus_to_visit)) {
-			normalize_cpu_capacity();
-			kfree(raw_capacity);
-			pr_debug("cpu_capacity: parsing done\n");
-			cap_parsing_done = true;
-			schedule_work(&parsing_done_work);
-		}
-	}
-	return 0;
-}
-
-static struct notifier_block init_cpu_capacity_notifier = {
-	.notifier_call = init_cpu_capacity_callback,
-};
-
-static int __init register_cpufreq_notifier(void)
-{
-	if (cap_parsing_failed)
-		return -EINVAL;
-
-	if (!alloc_cpumask_var(&cpus_to_visit, GFP_KERNEL)) {
-		pr_err("cpu_capacity: failed to allocate memory for cpus_to_visit\n");
-		return -ENOMEM;
-	}
-	cpumask_copy(cpus_to_visit, cpu_possible_mask);
-
-	return cpufreq_register_notifier(&init_cpu_capacity_notifier,
-					 CPUFREQ_POLICY_NOTIFIER);
-}
-core_initcall(register_cpufreq_notifier);
-
-static void parsing_done_workfn(struct work_struct *work)
-{
-	cpufreq_unregister_notifier(&init_cpu_capacity_notifier,
-					 CPUFREQ_POLICY_NOTIFIER);
-}
-
-#else
-static int __init free_raw_capacity(void)
-{
-	kfree(raw_capacity);
-
-	return 0;
-}
-core_initcall(free_raw_capacity);
-#endif
+extern bool cap_parsing_failed;
+extern void normalize_cpu_capacity(void);
+extern int __init parse_cpu_capacity(struct device_node *cpu_node, int cpu);
 
 /*
  * Iterate all CPUs' descriptor in DT and compute the efficiency
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 111742126897..7534bb41ee09 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -36,6 +36,7 @@ config ARM64
 	select EDAC_SUPPORT
 	select FRAME_POINTER
 	select GENERIC_ALLOCATOR
+	select GENERIC_ARCH_TOPOLOGY
 	select GENERIC_CLOCKEVENTS
 	select GENERIC_CLOCKEVENTS_BROADCAST
 	select GENERIC_CPU_AUTOPROBE
diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index 565dd69888cc..f629f7524d65 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -11,7 +11,6 @@
  * for more details.
  */
 
-#include <linux/acpi.h>
 #include <linux/cpu.h>
 #include <linux/cpumask.h>
 #include <linux/init.h>
@@ -22,226 +21,14 @@
 #include <linux/sched.h>
 #include <linux/slab.h>
 #include <linux/string.h>
-#include <linux/cpufreq.h>
 
 #include <asm/cpu.h>
 #include <asm/cputype.h>
 #include <asm/topology.h>
 
-static DEFINE_PER_CPU(unsigned long, cpu_scale) = SCHED_CAPACITY_SCALE;
-static DEFINE_MUTEX(cpu_scale_mutex);
-
-unsigned long arch_scale_cpu_capacity(struct sched_domain *sd, int cpu)
-{
-	return per_cpu(cpu_scale, cpu);
-}
-
-static void set_capacity_scale(unsigned int cpu, unsigned long capacity)
-{
-	per_cpu(cpu_scale, cpu) = capacity;
-}
-
-static ssize_t cpu_capacity_show(struct device *dev,
-				 struct device_attribute *attr,
-				 char *buf)
-{
-	struct cpu *cpu = container_of(dev, struct cpu, dev);
-
-	return sprintf(buf, "%lu\n",
-			arch_scale_cpu_capacity(NULL, cpu->dev.id));
-}
-
-static ssize_t cpu_capacity_store(struct device *dev,
-				  struct device_attribute *attr,
-				  const char *buf,
-				  size_t count)
-{
-	struct cpu *cpu = container_of(dev, struct cpu, dev);
-	int this_cpu = cpu->dev.id, i;
-	unsigned long new_capacity;
-	ssize_t ret;
-
-	if (count) {
-		ret = kstrtoul(buf, 0, &new_capacity);
-		if (ret)
-			return ret;
-		if (new_capacity > SCHED_CAPACITY_SCALE)
-			return -EINVAL;
-
-		mutex_lock(&cpu_scale_mutex);
-		for_each_cpu(i, &cpu_topology[this_cpu].core_sibling)
-			set_capacity_scale(i, new_capacity);
-		mutex_unlock(&cpu_scale_mutex);
-	}
-
-	return count;
-}
-
-static DEVICE_ATTR_RW(cpu_capacity);
-
-static int register_cpu_capacity_sysctl(void)
-{
-	int i;
-	struct device *cpu;
-
-	for_each_possible_cpu(i) {
-		cpu = get_cpu_device(i);
-		if (!cpu) {
-			pr_err("%s: too early to get CPU%d device!\n",
-			       __func__, i);
-			continue;
-		}
-		device_create_file(cpu, &dev_attr_cpu_capacity);
-	}
-
-	return 0;
-}
-subsys_initcall(register_cpu_capacity_sysctl);
-
-static u32 capacity_scale;
-static u32 *raw_capacity;
-static bool cap_parsing_failed;
-
-static void __init parse_cpu_capacity(struct device_node *cpu_node, int cpu)
-{
-	int ret;
-	u32 cpu_capacity;
-
-	if (cap_parsing_failed)
-		return;
-
-	ret = of_property_read_u32(cpu_node,
-				   "capacity-dmips-mhz",
-				   &cpu_capacity);
-	if (!ret) {
-		if (!raw_capacity) {
-			raw_capacity = kcalloc(num_possible_cpus(),
-					       sizeof(*raw_capacity),
-					       GFP_KERNEL);
-			if (!raw_capacity) {
-				pr_err("cpu_capacity: failed to allocate memory for raw capacities\n");
-				cap_parsing_failed = true;
-				return;
-			}
-		}
-		capacity_scale = max(cpu_capacity, capacity_scale);
-		raw_capacity[cpu] = cpu_capacity;
-		pr_debug("cpu_capacity: %s cpu_capacity=%u (raw)\n",
-			cpu_node->full_name, raw_capacity[cpu]);
-	} else {
-		if (raw_capacity) {
-			pr_err("cpu_capacity: missing %s raw capacity\n",
-				cpu_node->full_name);
-			pr_err("cpu_capacity: partial information: fallback to 1024 for all CPUs\n");
-		}
-		cap_parsing_failed = true;
-		kfree(raw_capacity);
-	}
-}
-
-static void normalize_cpu_capacity(void)
-{
-	u64 capacity;
-	int cpu;
-
-	if (!raw_capacity || cap_parsing_failed)
-		return;
-
-	pr_debug("cpu_capacity: capacity_scale=%u\n", capacity_scale);
-	mutex_lock(&cpu_scale_mutex);
-	for_each_possible_cpu(cpu) {
-		pr_debug("cpu_capacity: cpu=%d raw_capacity=%u\n",
-			 cpu, raw_capacity[cpu]);
-		capacity = (raw_capacity[cpu] << SCHED_CAPACITY_SHIFT)
-			/ capacity_scale;
-		set_capacity_scale(cpu, capacity);
-		pr_debug("cpu_capacity: CPU%d cpu_capacity=%lu\n",
-			cpu, arch_scale_cpu_capacity(NULL, cpu));
-	}
-	mutex_unlock(&cpu_scale_mutex);
-}
-
-#ifdef CONFIG_CPU_FREQ
-static cpumask_var_t cpus_to_visit;
-static bool cap_parsing_done;
-static void parsing_done_workfn(struct work_struct *work);
-static DECLARE_WORK(parsing_done_work, parsing_done_workfn);
-
-static int
-init_cpu_capacity_callback(struct notifier_block *nb,
-			   unsigned long val,
-			   void *data)
-{
-	struct cpufreq_policy *policy = data;
-	int cpu;
-
-	if (cap_parsing_failed || cap_parsing_done)
-		return 0;
-
-	switch (val) {
-	case CPUFREQ_NOTIFY:
-		pr_debug("cpu_capacity: init cpu capacity for CPUs [%*pbl] (to_visit=%*pbl)\n",
-				cpumask_pr_args(policy->related_cpus),
-				cpumask_pr_args(cpus_to_visit));
-		cpumask_andnot(cpus_to_visit,
-			       cpus_to_visit,
-			       policy->related_cpus);
-		for_each_cpu(cpu, policy->related_cpus) {
-			raw_capacity[cpu] = arch_scale_cpu_capacity(NULL, cpu) *
-					    policy->cpuinfo.max_freq / 1000UL;
-			capacity_scale = max(raw_capacity[cpu], capacity_scale);
-		}
-		if (cpumask_empty(cpus_to_visit)) {
-			normalize_cpu_capacity();
-			kfree(raw_capacity);
-			pr_debug("cpu_capacity: parsing done\n");
-			cap_parsing_done = true;
-			schedule_work(&parsing_done_work);
-		}
-	}
-	return 0;
-}
-
-static struct notifier_block init_cpu_capacity_notifier = {
-	.notifier_call = init_cpu_capacity_callback,
-};
-
-static int __init register_cpufreq_notifier(void)
-{
-	/*
-	 * on ACPI-based systems we need to use the default cpu capacity
-	 * until we have the necessary code to parse the cpu capacity, so
-	 * skip registering cpufreq notifier.
-	 */
-	if (!acpi_disabled || cap_parsing_failed)
-		return -EINVAL;
-
-	if (!alloc_cpumask_var(&cpus_to_visit, GFP_KERNEL)) {
-		pr_err("cpu_capacity: failed to allocate memory for cpus_to_visit\n");
-		return -ENOMEM;
-	}
-	cpumask_copy(cpus_to_visit, cpu_possible_mask);
-
-	return cpufreq_register_notifier(&init_cpu_capacity_notifier,
-					 CPUFREQ_POLICY_NOTIFIER);
-}
-core_initcall(register_cpufreq_notifier);
-
-static void parsing_done_workfn(struct work_struct *work)
-{
-	cpufreq_unregister_notifier(&init_cpu_capacity_notifier,
-					 CPUFREQ_POLICY_NOTIFIER);
-}
-
-#else
-static int __init free_raw_capacity(void)
-{
-	kfree(raw_capacity);
-
-	return 0;
-}
-core_initcall(free_raw_capacity);
-#endif
+extern bool cap_parsing_failed;
+extern void normalize_cpu_capacity(void);
+extern int __init parse_cpu_capacity(struct device_node *cpu_node, int cpu);
 
 static int __init get_cpu_for_node(struct device_node *node)
 {
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index d718ae4b907a..307ea31187dd 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -339,4 +339,12 @@ config CMA_ALIGNMENT
 
 endif
 
+config GENERIC_ARCH_TOPOLOGY
+	bool
+	help
+	  Enable support for architectures common topology code: e.g., parsing
+	  CPU capacity information from DT, usage of such information for
+	  appropriate scaling, sysfs interface for changing capacity values at
+          runtime.
+
 endmenu
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index f2816f6ff76a..397e5c344e6a 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_SOC_BUS) += soc.o
 obj-$(CONFIG_PINCTRL) += pinctrl.o
 obj-$(CONFIG_DEV_COREDUMP) += devcoredump.o
 obj-$(CONFIG_GENERIC_MSI_IRQ_DOMAIN) += platform-msi.o
+obj-$(CONFIG_GENERIC_ARCH_TOPOLOGY) += arch_topology.o
 
 obj-y			+= test/
 
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
new file mode 100644
index 000000000000..c1dd430adad2
--- /dev/null
+++ b/drivers/base/arch_topology.c
@@ -0,0 +1,237 @@
+/*
+ * driver/base/arch_topology.c - Arch specific cpu topology information
+ *
+ * Copyright (C) 2016, ARM Ltd.
+ * Written by: Juri Lelli, ARM Ltd.
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive
+ * for more details.
+ */
+
+#include <linux/acpi.h>
+#include <linux/cpu.h>
+#include <linux/cpufreq.h>
+#include <linux/device.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/topology.h>
+
+static DEFINE_MUTEX(cpu_scale_mutex);
+static DEFINE_PER_CPU(unsigned long, cpu_scale) = SCHED_CAPACITY_SCALE;
+
+unsigned long arch_scale_cpu_capacity(struct sched_domain *sd, int cpu)
+{
+	return per_cpu(cpu_scale, cpu);
+}
+
+void set_capacity_scale(unsigned int cpu, unsigned long capacity)
+{
+	per_cpu(cpu_scale, cpu) = capacity;
+}
+
+static ssize_t cpu_capacity_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct cpu *cpu = container_of(dev, struct cpu, dev);
+
+	return sprintf(buf, "%lu\n",
+			arch_scale_cpu_capacity(NULL, cpu->dev.id));
+}
+
+static ssize_t cpu_capacity_store(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf,
+				  size_t count)
+{
+	struct cpu *cpu = container_of(dev, struct cpu, dev);
+	int this_cpu = cpu->dev.id, i;
+	unsigned long new_capacity;
+	ssize_t ret;
+
+	if (count) {
+		ret = kstrtoul(buf, 0, &new_capacity);
+		if (ret)
+			return ret;
+		if (new_capacity > SCHED_CAPACITY_SCALE)
+			return -EINVAL;
+
+		mutex_lock(&cpu_scale_mutex);
+		for_each_cpu(i, &cpu_topology[this_cpu].core_sibling)
+			set_capacity_scale(i, new_capacity);
+		mutex_unlock(&cpu_scale_mutex);
+	}
+
+	return count;
+}
+
+static DEVICE_ATTR_RW(cpu_capacity);
+
+static int register_cpu_capacity_sysctl(void)
+{
+	int i;
+	struct device *cpu;
+
+	for_each_possible_cpu(i) {
+		cpu = get_cpu_device(i);
+		if (!cpu) {
+			pr_err("%s: too early to get CPU%d device!\n",
+			       __func__, i);
+			continue;
+		}
+		device_create_file(cpu, &dev_attr_cpu_capacity);
+	}
+
+	return 0;
+}
+subsys_initcall(register_cpu_capacity_sysctl);
+
+u32 capacity_scale;
+u32 *raw_capacity;
+bool cap_parsing_failed;
+
+void normalize_cpu_capacity(void)
+{
+	u64 capacity;
+	int cpu;
+
+	if (!raw_capacity || cap_parsing_failed)
+		return;
+
+	pr_debug("cpu_capacity: capacity_scale=%u\n", capacity_scale);
+	mutex_lock(&cpu_scale_mutex);
+	for_each_possible_cpu(cpu) {
+		pr_debug("cpu_capacity: cpu=%d raw_capacity=%u\n",
+			 cpu, raw_capacity[cpu]);
+		capacity = (raw_capacity[cpu] << SCHED_CAPACITY_SHIFT)
+			/ capacity_scale;
+		set_capacity_scale(cpu, capacity);
+		pr_debug("cpu_capacity: CPU%d cpu_capacity=%lu\n",
+			cpu, arch_scale_cpu_capacity(NULL, cpu));
+	}
+	mutex_unlock(&cpu_scale_mutex);
+}
+
+int __init parse_cpu_capacity(struct device_node *cpu_node, int cpu)
+{
+	int ret = 1;
+	u32 cpu_capacity;
+
+	if (cap_parsing_failed)
+		return !ret;
+
+	ret = of_property_read_u32(cpu_node,
+				   "capacity-dmips-mhz",
+				   &cpu_capacity);
+	if (!ret) {
+		if (!raw_capacity) {
+			raw_capacity = kcalloc(num_possible_cpus(),
+					       sizeof(*raw_capacity),
+					       GFP_KERNEL);
+			if (!raw_capacity) {
+				pr_err("cpu_capacity: failed to allocate memory for raw capacities\n");
+				cap_parsing_failed = true;
+				return ret;
+			}
+		}
+		capacity_scale = max(cpu_capacity, capacity_scale);
+		raw_capacity[cpu] = cpu_capacity;
+		pr_debug("cpu_capacity: %s cpu_capacity=%u (raw)\n",
+			cpu_node->full_name, raw_capacity[cpu]);
+	} else {
+		if (raw_capacity) {
+			pr_err("cpu_capacity: missing %s raw capacity\n",
+				cpu_node->full_name);
+			pr_err("cpu_capacity: partial information: fallback to 1024 for all CPUs\n");
+		}
+		cap_parsing_failed = true;
+		kfree(raw_capacity);
+	}
+
+	return !ret;
+}
+
+#ifdef CONFIG_CPU_FREQ
+static cpumask_var_t cpus_to_visit;
+static bool cap_parsing_done;
+static void parsing_done_workfn(struct work_struct *work);
+static DECLARE_WORK(parsing_done_work, parsing_done_workfn);
+
+static int
+init_cpu_capacity_callback(struct notifier_block *nb,
+			   unsigned long val,
+			   void *data)
+{
+	struct cpufreq_policy *policy = data;
+	int cpu;
+
+	if (cap_parsing_failed || cap_parsing_done)
+		return 0;
+
+	switch (val) {
+	case CPUFREQ_NOTIFY:
+		pr_debug("cpu_capacity: init cpu capacity for CPUs [%*pbl] (to_visit=%*pbl)\n",
+				cpumask_pr_args(policy->related_cpus),
+				cpumask_pr_args(cpus_to_visit));
+		cpumask_andnot(cpus_to_visit,
+			       cpus_to_visit,
+			       policy->related_cpus);
+		for_each_cpu(cpu, policy->related_cpus) {
+			raw_capacity[cpu] = arch_scale_cpu_capacity(NULL, cpu) *
+					    policy->cpuinfo.max_freq / 1000UL;
+			capacity_scale = max(raw_capacity[cpu], capacity_scale);
+		}
+		if (cpumask_empty(cpus_to_visit)) {
+			normalize_cpu_capacity();
+			kfree(raw_capacity);
+			pr_debug("cpu_capacity: parsing done\n");
+			cap_parsing_done = true;
+			schedule_work(&parsing_done_work);
+		}
+	}
+	return 0;
+}
+
+static struct notifier_block init_cpu_capacity_notifier = {
+	.notifier_call = init_cpu_capacity_callback,
+};
+
+static int __init register_cpufreq_notifier(void)
+{
+	/*
+	 * on ACPI-based systems we need to use the default cpu capacity
+	 * until we have the necessary code to parse the cpu capacity, so
+	 * skip registering cpufreq notifier.
+	 */
+	if (!acpi_disabled || cap_parsing_failed)
+		return -EINVAL;
+
+	if (!alloc_cpumask_var(&cpus_to_visit, GFP_KERNEL)) {
+		pr_err("cpu_capacity: failed to allocate memory for cpus_to_visit\n");
+		return -ENOMEM;
+	}
+
+	cpumask_copy(cpus_to_visit, cpu_possible_mask);
+
+	return cpufreq_register_notifier(&init_cpu_capacity_notifier,
+					 CPUFREQ_POLICY_NOTIFIER);
+}
+core_initcall(register_cpufreq_notifier);
+
+static void parsing_done_workfn(struct work_struct *work)
+{
+	cpufreq_unregister_notifier(&init_cpu_capacity_notifier,
+					 CPUFREQ_POLICY_NOTIFIER);
+}
+
+#else
+static int __init free_raw_capacity(void)
+{
+	kfree(raw_capacity);
+
+	return 0;
+}
+core_initcall(free_raw_capacity);
+#endif
-- 
2.10.0

[PATCH v2 7/9] arm,arm64,drivers: reduce scope of cap_parsing_failed

[Juri Lelli]

Reduce the scope of cap_parsing_failed (making it static in
drivers/base/arch_topology.c) by slightly changing {arm,arm64} DT
parsing code.

For arm checking for !cap_parsing_failed before calling normalize_
cpu_capacity() is superfluous, as returning an error from parse_
cpu_capacity() (above) means cap_from _dt is set to false.

For arm64 we can simply check if raw_capacity points to something,
which is not if capacity parsing has failed.

Suggested-by: Morten Rasmussen <morten.rasmussen@arm.com>
Signed-off-by: Juri Lelli <juri.lelli@arm.com>
---
 arch/arm/kernel/topology.c   | 3 +--
 arch/arm64/kernel/topology.c | 5 +----
 drivers/base/arch_topology.c | 4 ++--
 3 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
index 51e9ed6439f1..5d4679a5418b 100644
--- a/arch/arm/kernel/topology.c
+++ b/arch/arm/kernel/topology.c
@@ -75,7 +75,6 @@ static unsigned long *__cpu_capacity;
 
 static unsigned long middle_capacity = 1;
 static bool cap_from_dt = true;
-extern bool cap_parsing_failed;
 extern void normalize_cpu_capacity(void);
 extern int __init parse_cpu_capacity(struct device_node *cpu_node, int cpu);
 
@@ -164,7 +163,7 @@ static void __init parse_dt_topology(void)
 		middle_capacity = ((max_capacity / 3)
 				>> (SCHED_CAPACITY_SHIFT-1)) + 1;
 
-	if (cap_from_dt && !cap_parsing_failed)
+	if (cap_from_dt)
 		normalize_cpu_capacity();
 }
 
diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index f629f7524d65..deb5ebc1bdfe 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -26,7 +26,6 @@
 #include <asm/cputype.h>
 #include <asm/topology.h>
 
-extern bool cap_parsing_failed;
 extern void normalize_cpu_capacity(void);
 extern int __init parse_cpu_capacity(struct device_node *cpu_node, int cpu);
 
@@ -186,10 +185,8 @@ static int __init parse_dt_topology(void)
 	 * cluster with restricted subnodes.
 	 */
 	map = of_get_child_by_name(cn, "cpu-map");
-	if (!map) {
-		cap_parsing_failed = true;
+	if (!map)
 		goto out;
-	}
 
 	ret = parse_cluster(map, 0);
 	if (ret != 0)
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index c1dd430adad2..c75fa5ab732b 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -90,7 +90,7 @@ subsys_initcall(register_cpu_capacity_sysctl);
 
 u32 capacity_scale;
 u32 *raw_capacity;
-bool cap_parsing_failed;
+static bool cap_parsing_failed;
 
 void normalize_cpu_capacity(void)
 {
@@ -205,7 +205,7 @@ static int __init register_cpufreq_notifier(void)
 	 * until we have the necessary code to parse the cpu capacity, so
 	 * skip registering cpufreq notifier.
 	 */
-	if (!acpi_disabled || cap_parsing_failed)
+	if (!acpi_disabled || !raw_capacity)
 		return -EINVAL;
 
 	if (!alloc_cpumask_var(&cpus_to_visit, GFP_KERNEL)) {
-- 
2.10.0

[PATCH v2 8/9] arm,arm64,drivers: move externs in a new header file

[Juri Lelli]

Create a new header file (include/linux/arch_topology.h) and put there
declarations of interfaces used by arm, arm64 and drivers code.

Signed-off-by: Juri Lelli <juri.lelli@arm.com>
---
 arch/arm/kernel/topology.c    |  7 +------
 arch/arm64/kernel/topology.c  |  4 +---
 drivers/base/arch_topology.c  |  1 +
 include/linux/arch_topology.h | 17 +++++++++++++++++
 4 files changed, 20 insertions(+), 9 deletions(-)
 create mode 100644 include/linux/arch_topology.h

diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
index 5d4679a5418b..1855632105a4 100644
--- a/arch/arm/kernel/topology.c
+++ b/arch/arm/kernel/topology.c
@@ -11,6 +11,7 @@
  * for more details.
  */
 
+#include <linux/arch_topology.h>
 #include <linux/cpu.h>
 #include <linux/cpufreq.h>
 #include <linux/cpumask.h>
@@ -44,10 +45,6 @@
  * updated during this sequence.
  */
 
-extern unsigned long
-arch_scale_cpu_capacity(struct sched_domain *sd, int cpu);
-extern void set_capacity_scale(unsigned int cpu, unsigned long capacity);
-
 #ifdef CONFIG_OF
 struct cpu_efficiency {
 	const char *compatible;
@@ -75,8 +72,6 @@ static unsigned long *__cpu_capacity;
 
 static unsigned long middle_capacity = 1;
 static bool cap_from_dt = true;
-extern void normalize_cpu_capacity(void);
-extern int __init parse_cpu_capacity(struct device_node *cpu_node, int cpu);
 
 /*
  * Iterate all CPUs' descriptor in DT and compute the efficiency
diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index deb5ebc1bdfe..6d2af4d9479f 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -11,6 +11,7 @@
  * for more details.
  */
 
+#include <linux/arch_topology.h>
 #include <linux/cpu.h>
 #include <linux/cpumask.h>
 #include <linux/init.h>
@@ -26,9 +27,6 @@
 #include <asm/cputype.h>
 #include <asm/topology.h>
 
-extern void normalize_cpu_capacity(void);
-extern int __init parse_cpu_capacity(struct device_node *cpu_node, int cpu);
-
 static int __init get_cpu_for_node(struct device_node *node)
 {
 	struct device_node *cpu_node;
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index c75fa5ab732b..409a8bb613f2 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -10,6 +10,7 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/arch_topology.h>
 #include <linux/cpu.h>
 #include <linux/cpufreq.h>
 #include <linux/device.h>
diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h
new file mode 100644
index 000000000000..4edae9fe8cdd
--- /dev/null
+++ b/include/linux/arch_topology.h
@@ -0,0 +1,17 @@
+/*
+ * include/linux/arch_topology.h - arch specific cpu topology information
+ */
+#ifndef _LINUX_ARCH_TOPOLOGY_H_
+#define _LINUX_ARCH_TOPOLOGY_H_
+
+void normalize_cpu_capacity(void);
+
+struct device_node;
+int parse_cpu_capacity(struct device_node *cpu_node, int cpu);
+
+struct sched_domain;
+unsigned long arch_scale_cpu_capacity(struct sched_domain *sd, int cpu);
+
+void set_capacity_scale(unsigned int cpu, unsigned long capacity);
+
+#endif /* _LINUX_ARCH_TOPOLOGY_H_ */
-- 
2.10.0

[PATCH v2 9/9] arm,arm64,drivers: add a prefix to drivers arch_topology interfaces

[Juri Lelli]

Now that some functions that deal with arch topology information live
under drivers, there is a clash of naming that might create confusion.

Tidy things up by creating a drivers namespace for interfaces used by
arch code; achieve this by prepending a 'atd_' (arch topology driver)
prefix to driver interfaces.

Signed-off-by: Juri Lelli <juri.lelli@arm.com>
---
 arch/arm/kernel/topology.c    |  8 ++++----
 arch/arm64/kernel/topology.c  |  4 ++--
 drivers/base/arch_topology.c  | 20 ++++++++++----------
 include/linux/arch_topology.h |  8 ++++----
 4 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
index 1855632105a4..a33e0891fee2 100644
--- a/arch/arm/kernel/topology.c
+++ b/arch/arm/kernel/topology.c
@@ -110,7 +110,7 @@ static void __init parse_dt_topology(void)
 			continue;
 		}
 
-		if (parse_cpu_capacity(cn, cpu)) {
+		if (atd_parse_cpu_capacity(cn, cpu)) {
 			of_node_put(cn);
 			continue;
 		}
@@ -159,7 +159,7 @@ static void __init parse_dt_topology(void)
 				>> (SCHED_CAPACITY_SHIFT-1)) + 1;
 
 	if (cap_from_dt)
-		normalize_cpu_capacity();
+		atd_normalize_cpu_capacity();
 }
 
 /*
@@ -172,10 +172,10 @@ static void update_cpu_capacity(unsigned int cpu)
 	if (!cpu_capacity(cpu) || cap_from_dt)
 		return;
 
-	set_capacity_scale(cpu, cpu_capacity(cpu) / middle_capacity);
+	atd_set_capacity_scale(cpu, cpu_capacity(cpu) / middle_capacity);
 
 	pr_info("CPU%u: update cpu_capacity %lu\n",
-		cpu, arch_scale_cpu_capacity(NULL, cpu));
+		cpu, atd_scale_cpu_capacity(NULL, cpu));
 }
 
 #else
diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index 6d2af4d9479f..1f6b34b6b359 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -38,7 +38,7 @@ static int __init get_cpu_for_node(struct device_node *node)
 
 	for_each_possible_cpu(cpu) {
 		if (of_get_cpu_node(cpu, NULL) == cpu_node) {
-			parse_cpu_capacity(cpu_node, cpu);
+			atd_parse_cpu_capacity(cpu_node, cpu);
 			of_node_put(cpu_node);
 			return cpu;
 		}
@@ -190,7 +190,7 @@ static int __init parse_dt_topology(void)
 	if (ret != 0)
 		goto out_map;
 
-	normalize_cpu_capacity();
+	atd_normalize_cpu_capacity();
 
 	/*
 	 * Check that all cores are in the topology; the SMP code will
diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 409a8bb613f2..5c20280bb746 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -22,12 +22,12 @@
 static DEFINE_MUTEX(cpu_scale_mutex);
 static DEFINE_PER_CPU(unsigned long, cpu_scale) = SCHED_CAPACITY_SCALE;
 
-unsigned long arch_scale_cpu_capacity(struct sched_domain *sd, int cpu)
+unsigned long atd_scale_cpu_capacity(struct sched_domain *sd, int cpu)
 {
 	return per_cpu(cpu_scale, cpu);
 }
 
-void set_capacity_scale(unsigned int cpu, unsigned long capacity)
+void atd_set_capacity_scale(unsigned int cpu, unsigned long capacity)
 {
 	per_cpu(cpu_scale, cpu) = capacity;
 }
@@ -39,7 +39,7 @@ static ssize_t cpu_capacity_show(struct device *dev,
 	struct cpu *cpu = container_of(dev, struct cpu, dev);
 
 	return sprintf(buf, "%lu\n",
-			arch_scale_cpu_capacity(NULL, cpu->dev.id));
+			atd_scale_cpu_capacity(NULL, cpu->dev.id));
 }
 
 static ssize_t cpu_capacity_store(struct device *dev,
@@ -61,7 +61,7 @@ static ssize_t cpu_capacity_store(struct device *dev,
 
 		mutex_lock(&cpu_scale_mutex);
 		for_each_cpu(i, &cpu_topology[this_cpu].core_sibling)
-			set_capacity_scale(i, new_capacity);
+			atd_set_capacity_scale(i, new_capacity);
 		mutex_unlock(&cpu_scale_mutex);
 	}
 
@@ -93,7 +93,7 @@ u32 capacity_scale;
 u32 *raw_capacity;
 static bool cap_parsing_failed;
 
-void normalize_cpu_capacity(void)
+void atd_normalize_cpu_capacity(void)
 {
 	u64 capacity;
 	int cpu;
@@ -108,14 +108,14 @@ void normalize_cpu_capacity(void)
 			 cpu, raw_capacity[cpu]);
 		capacity = (raw_capacity[cpu] << SCHED_CAPACITY_SHIFT)
 			/ capacity_scale;
-		set_capacity_scale(cpu, capacity);
+		atd_set_capacity_scale(cpu, capacity);
 		pr_debug("cpu_capacity: CPU%d cpu_capacity=%lu\n",
-			cpu, arch_scale_cpu_capacity(NULL, cpu));
+			cpu, atd_scale_cpu_capacity(NULL, cpu));
 	}
 	mutex_unlock(&cpu_scale_mutex);
 }
 
-int __init parse_cpu_capacity(struct device_node *cpu_node, int cpu)
+int __init atd_parse_cpu_capacity(struct device_node *cpu_node, int cpu)
 {
 	int ret = 1;
 	u32 cpu_capacity;
@@ -180,12 +180,12 @@ init_cpu_capacity_callback(struct notifier_block *nb,
 			       cpus_to_visit,
 			       policy->related_cpus);
 		for_each_cpu(cpu, policy->related_cpus) {
-			raw_capacity[cpu] = arch_scale_cpu_capacity(NULL, cpu) *
+			raw_capacity[cpu] = atd_scale_cpu_capacity(NULL, cpu) *
 					    policy->cpuinfo.max_freq / 1000UL;
 			capacity_scale = max(raw_capacity[cpu], capacity_scale);
 		}
 		if (cpumask_empty(cpus_to_visit)) {
-			normalize_cpu_capacity();
+			atd_normalize_cpu_capacity();
 			kfree(raw_capacity);
 			pr_debug("cpu_capacity: parsing done\n");
 			cap_parsing_done = true;
diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h
index 4edae9fe8cdd..e25458d7ee9a 100644
--- a/include/linux/arch_topology.h
+++ b/include/linux/arch_topology.h
@@ -4,14 +4,14 @@
 #ifndef _LINUX_ARCH_TOPOLOGY_H_
 #define _LINUX_ARCH_TOPOLOGY_H_
 
-void normalize_cpu_capacity(void);
+void atd_normalize_cpu_capacity(void);
 
 struct device_node;
-int parse_cpu_capacity(struct device_node *cpu_node, int cpu);
+int atd_parse_cpu_capacity(struct device_node *cpu_node, int cpu);
 
 struct sched_domain;
-unsigned long arch_scale_cpu_capacity(struct sched_domain *sd, int cpu);
+unsigned long atd_scale_cpu_capacity(struct sched_domain *sd, int cpu);
 
-void set_capacity_scale(unsigned int cpu, unsigned long capacity);
+void atd_set_capacity_scale(unsigned int cpu, unsigned long capacity);
 
 #endif /* _LINUX_ARCH_TOPOLOGY_H_ */
-- 
2.10.0

Re: [PATCH v2 6/9] arm, arm64: factorize common cpu capacity default code

[Greg KH]

On Thu, Feb 09, 2017 at 09:25:22AM +0000, Juri Lelli wrote:
> arm and arm64 share lot of code relative to parsing CPU capacity
> information from DT, using that information for appropriate scaling and
> exposing a sysfs interface for chaging such values at runtime.
> 
> Factorize such code in a common place (driver/base/arch_topology.c) in
> preparation for further additions.
> 
> Suggested-by: Will Deacon <will.deacon@arm.com>
> Suggested-by: Mark Rutland <mark.rutland@arm.com>
> Suggested-by: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Juri Lelli <juri.lelli@arm.com>
> ---
> 
> Changes from v1:
>  - keep the original GPLv2 header
> ---
>  arch/arm/Kconfig             |   1 +
>  arch/arm/kernel/topology.c   | 213 ++------------------------------------
>  arch/arm64/Kconfig           |   1 +
>  arch/arm64/kernel/topology.c | 219 +--------------------------------------
>  drivers/base/Kconfig         |   8 ++
>  drivers/base/Makefile        |   1 +
>  drivers/base/arch_topology.c | 237 +++++++++++++++++++++++++++++++++++++++++++
>  7 files changed, 257 insertions(+), 423 deletions(-)
>  create mode 100644 drivers/base/arch_topology.c

Ah, so you want _me_ to maintain this, ok, I better review it...

> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -339,4 +339,12 @@ config CMA_ALIGNMENT
>  
>  endif
>  
> +config GENERIC_ARCH_TOPOLOGY
> +	bool
> +	help
> +	  Enable support for architectures common topology code: e.g., parsing
> +	  CPU capacity information from DT, usage of such information for
> +	  appropriate scaling, sysfs interface for changing capacity values at
> +          runtime.

Mix of spaces and tabs :(

> +
>  endmenu
> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> index f2816f6ff76a..397e5c344e6a 100644
> --- a/drivers/base/Makefile
> +++ b/drivers/base/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_SOC_BUS) += soc.o
>  obj-$(CONFIG_PINCTRL) += pinctrl.o
>  obj-$(CONFIG_DEV_COREDUMP) += devcoredump.o
>  obj-$(CONFIG_GENERIC_MSI_IRQ_DOMAIN) += platform-msi.o
> +obj-$(CONFIG_GENERIC_ARCH_TOPOLOGY) += arch_topology.o
>  
>  obj-y			+= test/
>  
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> new file mode 100644
> index 000000000000..c1dd430adad2
> --- /dev/null
> +++ b/drivers/base/arch_topology.c
> @@ -0,0 +1,237 @@
> +/*
> + * driver/base/arch_topology.c - Arch specific cpu topology information

No need to keep the filename in the file, you know what it is called :)

> + *
> + * Copyright (C) 2016, ARM Ltd.
> + * Written by: Juri Lelli, ARM Ltd.
> + *
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License.  See the file "COPYING" in the main directory of this archive
> + * for more details.

So, v2 only?  Please be specific.  Even better yet, use a SPDX header if
you want to, those are always nice.

> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/cpu.h>
> +#include <linux/cpufreq.h>
> +#include <linux/device.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/topology.h>
> +
> +static DEFINE_MUTEX(cpu_scale_mutex);
> +static DEFINE_PER_CPU(unsigned long, cpu_scale) = SCHED_CAPACITY_SCALE;
> +
> +unsigned long arch_scale_cpu_capacity(struct sched_domain *sd, int cpu)

Why do you have sd here?  You never use it:

> +{
> +	return per_cpu(cpu_scale, cpu);

See?  What am I missing?

> +}
> +
> +void set_capacity_scale(unsigned int cpu, unsigned long capacity)
> +{
> +	per_cpu(cpu_scale, cpu) = capacity;
> +}
> +
> +static ssize_t cpu_capacity_show(struct device *dev,
> +				 struct device_attribute *attr,
> +				 char *buf)
> +{
> +	struct cpu *cpu = container_of(dev, struct cpu, dev);
> +
> +	return sprintf(buf, "%lu\n",
> +			arch_scale_cpu_capacity(NULL, cpu->dev.id));
> +}
> +
> +static ssize_t cpu_capacity_store(struct device *dev,
> +				  struct device_attribute *attr,
> +				  const char *buf,
> +				  size_t count)
> +{
> +	struct cpu *cpu = container_of(dev, struct cpu, dev);
> +	int this_cpu = cpu->dev.id, i;

new line for:
	int i;
please.

> +	unsigned long new_capacity;
> +	ssize_t ret;
> +
> +	if (count) {

	if (!count)
		return 0;

then you can get on with the rest of the logic.  Don't indent if you
don't have to.

> +		ret = kstrtoul(buf, 0, &new_capacity);
> +		if (ret)
> +			return ret;
> +		if (new_capacity > SCHED_CAPACITY_SCALE)
> +			return -EINVAL;
> +
> +		mutex_lock(&cpu_scale_mutex);
> +		for_each_cpu(i, &cpu_topology[this_cpu].core_sibling)
> +			set_capacity_scale(i, new_capacity);
> +		mutex_unlock(&cpu_scale_mutex);
> +	}
> +
> +	return count;
> +}

No documentation for these sysfs file?  Not good :(

> +
> +static DEVICE_ATTR_RW(cpu_capacity);
> +
> +static int register_cpu_capacity_sysctl(void)
> +{
> +	int i;
> +	struct device *cpu;
> +
> +	for_each_possible_cpu(i) {
> +		cpu = get_cpu_device(i);
> +		if (!cpu) {
> +			pr_err("%s: too early to get CPU%d device!\n",
> +			       __func__, i);

What is this going to help with?

> +			continue;
> +		}
> +		device_create_file(cpu, &dev_attr_cpu_capacity);

You realize you just raced userspace, right?  Why do it this way and not
register the files when the CPU device is created/removed?

> +	}
> +
> +	return 0;
> +}
> +subsys_initcall(register_cpu_capacity_sysctl);
> +
> +u32 capacity_scale;
> +u32 *raw_capacity;
> +bool cap_parsing_failed;

globals?  really?  That's bold :(

> +
> +void normalize_cpu_capacity(void)

naming is hard, but try to put a good, descriptive, prefix on everything
you are exporting in the same file, the same prefix.

cpu_capacity_normalize()?
cpu_capacity_register_sysctl()?

and so on.

> +{
> +	u64 capacity;
> +	int cpu;
> +
> +	if (!raw_capacity || cap_parsing_failed)
> +		return;
> +
> +	pr_debug("cpu_capacity: capacity_scale=%u\n", capacity_scale);
> +	mutex_lock(&cpu_scale_mutex);
> +	for_each_possible_cpu(cpu) {
> +		pr_debug("cpu_capacity: cpu=%d raw_capacity=%u\n",
> +			 cpu, raw_capacity[cpu]);
> +		capacity = (raw_capacity[cpu] << SCHED_CAPACITY_SHIFT)
> +			/ capacity_scale;
> +		set_capacity_scale(cpu, capacity);
> +		pr_debug("cpu_capacity: CPU%d cpu_capacity=%lu\n",
> +			cpu, arch_scale_cpu_capacity(NULL, cpu));
> +	}
> +	mutex_unlock(&cpu_scale_mutex);
> +}
> +
> +int __init parse_cpu_capacity(struct device_node *cpu_node, int cpu)

cpu_capacity_parse()?

thanks,

greg k-h

Re: [PATCH v2 6/9] arm, arm64: factorize common cpu capacity default code

[Juri Lelli]

Hi Greg,

On 10/02/17 15:28, Greg KH wrote:
> On Thu, Feb 09, 2017 at 09:25:22AM +0000, Juri Lelli wrote:
> > arm and arm64 share lot of code relative to parsing CPU capacity
> > information from DT, using that information for appropriate scaling and
> > exposing a sysfs interface for chaging such values at runtime.
> > 
> > Factorize such code in a common place (driver/base/arch_topology.c) in
> > preparation for further additions.
> > 
> > Suggested-by: Will Deacon <will.deacon@arm.com>
> > Suggested-by: Mark Rutland <mark.rutland@arm.com>
> > Suggested-by: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Russell King <linux@armlinux.org.uk>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Juri Lelli <juri.lelli@arm.com>
> > ---
> > 
> > Changes from v1:
> >  - keep the original GPLv2 header
> > ---
> >  arch/arm/Kconfig             |   1 +
> >  arch/arm/kernel/topology.c   | 213 ++------------------------------------
> >  arch/arm64/Kconfig           |   1 +
> >  arch/arm64/kernel/topology.c | 219 +--------------------------------------
> >  drivers/base/Kconfig         |   8 ++
> >  drivers/base/Makefile        |   1 +
> >  drivers/base/arch_topology.c | 237 +++++++++++++++++++++++++++++++++++++++++++
> >  7 files changed, 257 insertions(+), 423 deletions(-)
> >  create mode 100644 drivers/base/arch_topology.c
> 
> Ah, so you want _me_ to maintain this, ok, I better review it...
> 

This has been suggested as a possible way to stop replicating code between arm
and arm64 (and possibly other archs in the future). Are you in principle OK
with it?

Thanks a lot for your comments, please find my answers below.

> > --- a/drivers/base/Kconfig
> > +++ b/drivers/base/Kconfig
> > @@ -339,4 +339,12 @@ config CMA_ALIGNMENT
> >  
> >  endif
> >  
> > +config GENERIC_ARCH_TOPOLOGY
> > +	bool
> > +	help
> > +	  Enable support for architectures common topology code: e.g., parsing
> > +	  CPU capacity information from DT, usage of such information for
> > +	  appropriate scaling, sysfs interface for changing capacity values at
> > +          runtime.
> 
> Mix of spaces and tabs :(
> 

Argh. :(

> > +
> >  endmenu
> > diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> > index f2816f6ff76a..397e5c344e6a 100644
> > --- a/drivers/base/Makefile
> > +++ b/drivers/base/Makefile
> > @@ -23,6 +23,7 @@ obj-$(CONFIG_SOC_BUS) += soc.o
> >  obj-$(CONFIG_PINCTRL) += pinctrl.o
> >  obj-$(CONFIG_DEV_COREDUMP) += devcoredump.o
> >  obj-$(CONFIG_GENERIC_MSI_IRQ_DOMAIN) += platform-msi.o
> > +obj-$(CONFIG_GENERIC_ARCH_TOPOLOGY) += arch_topology.o
> >  
> >  obj-y			+= test/
> >  
> > diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> > new file mode 100644
> > index 000000000000..c1dd430adad2
> > --- /dev/null
> > +++ b/drivers/base/arch_topology.c
> > @@ -0,0 +1,237 @@
> > +/*
> > + * driver/base/arch_topology.c - Arch specific cpu topology information
> 
> No need to keep the filename in the file, you know what it is called :)
> 

OK, removed.

> > + *
> > + * Copyright (C) 2016, ARM Ltd.
> > + * Written by: Juri Lelli, ARM Ltd.
> > + *
> > + * This file is subject to the terms and conditions of the GNU General Public
> > + * License.  See the file "COPYING" in the main directory of this archive
> > + * for more details.
> 
> So, v2 only?  Please be specific.  Even better yet, use a SPDX header if
> you want to, those are always nice.
> 

Yes, v2 only.

  * for more details.                                                                                                                                                                                                                    
+ *                                                                                                                                                                                                                                      
+ * Released under the GPLv2 only.                                                                                                                                                                                                       
+ * SPDX-License-Identifier: GPL-2.0 

Would do, right?

> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/cpu.h>
> > +#include <linux/cpufreq.h>
> > +#include <linux/device.h>
> > +#include <linux/of.h>
> > +#include <linux/slab.h>
> > +#include <linux/string.h>
> > +#include <linux/topology.h>
> > +
> > +static DEFINE_MUTEX(cpu_scale_mutex);
> > +static DEFINE_PER_CPU(unsigned long, cpu_scale) = SCHED_CAPACITY_SCALE;
> > +
> > +unsigned long arch_scale_cpu_capacity(struct sched_domain *sd, int cpu)
> 
> Why do you have sd here?  You never use it:
> 
> > +{
> > +	return per_cpu(cpu_scale, cpu);
> 
> See?  What am I missing?
> 

This is how this function is defined in kernel/sched/sched.h:

#ifndef arch_scale_cpu_capacity
static __always_inline
unsigned long arch_scale_cpu_capacity(struct sched_domain *sd, int cpu)
{
	if (sd && (sd->flags & SD_SHARE_CPUCAPACITY) && (sd->span_weight > 1))
		return sd->smt_gain / sd->span_weight;

	return SCHED_CAPACITY_SCALE;
}
#endif

and in this case the sd argument is used: there is a call site in fair.c
that passes a non NULL sd, updated_cpu_capacity().

A following set of patches will re-define the function so that the
drivers one gets used by the kernel (only arm and arm64 will currently
want this), with something like this in arch code

#define arch_scale_cpu_capacity atd_scale_cpu_capacity

Please note that last patch of this set renames this function atd_scale_
cpu_capacity, to (hopefully) make this approach more clear.

Does it make more sense to you?

> > +}
> > +
> > +void set_capacity_scale(unsigned int cpu, unsigned long capacity)
> > +{
> > +	per_cpu(cpu_scale, cpu) = capacity;
> > +}
> > +
> > +static ssize_t cpu_capacity_show(struct device *dev,
> > +				 struct device_attribute *attr,
> > +				 char *buf)
> > +{
> > +	struct cpu *cpu = container_of(dev, struct cpu, dev);
> > +
> > +	return sprintf(buf, "%lu\n",
> > +			arch_scale_cpu_capacity(NULL, cpu->dev.id));
> > +}
> > +
> > +static ssize_t cpu_capacity_store(struct device *dev,
> > +				  struct device_attribute *attr,
> > +				  const char *buf,
> > +				  size_t count)
> > +{
> > +	struct cpu *cpu = container_of(dev, struct cpu, dev);
> > +	int this_cpu = cpu->dev.id, i;
> 
> new line for:
> 	int i;
> please.
> 

Sure.

> > +	unsigned long new_capacity;
> > +	ssize_t ret;
> > +
> > +	if (count) {
> 
> 	if (!count)
> 		return 0;
> 
> then you can get on with the rest of the logic.  Don't indent if you
> don't have to.
> 

Right.

> > +		ret = kstrtoul(buf, 0, &new_capacity);
> > +		if (ret)
> > +			return ret;
> > +		if (new_capacity > SCHED_CAPACITY_SCALE)
> > +			return -EINVAL;
> > +
> > +		mutex_lock(&cpu_scale_mutex);
> > +		for_each_cpu(i, &cpu_topology[this_cpu].core_sibling)
> > +			set_capacity_scale(i, new_capacity);
> > +		mutex_unlock(&cpu_scale_mutex);
> > +	}
> > +
> > +	return count;
> > +}
> 
> No documentation for these sysfs file?  Not good :(
> 

Patch 2/9 introduces some documentation. There is already more in
Documentation/devicetree/bindings/arm/cpu-capacity.txt.

Do you think I should improve further?

> > +
> > +static DEVICE_ATTR_RW(cpu_capacity);
> > +
> > +static int register_cpu_capacity_sysctl(void)
> > +{
> > +	int i;
> > +	struct device *cpu;
> > +
> > +	for_each_possible_cpu(i) {
> > +		cpu = get_cpu_device(i);
> > +		if (!cpu) {
> > +			pr_err("%s: too early to get CPU%d device!\n",
> > +			       __func__, i);
> 
> What is this going to help with?
> 

Not much I guess, I can remove it.

> > +			continue;
> > +		}
> > +		device_create_file(cpu, &dev_attr_cpu_capacity);
> 
> You realize you just raced userspace, right?  Why do it this way and not
> register the files when the CPU device is created/removed?
> 

Humm, my intention for doing it this way is that I'd like to make all
the code dealing with cpu_capacity confined in a single place (this
file), without the need to modify other files.

> > +	}
> > +
> > +	return 0;
> > +}
> > +subsys_initcall(register_cpu_capacity_sysctl);

AFAIU, for both arm and arm64 CPU device is registered with a
subsys_initcall(topology_init), so I'm doing the same. Other archs seem to do
similar things. Could you explain a little more why this is a problem?

> > +
> > +u32 capacity_scale;
> > +u32 *raw_capacity;
> > +bool cap_parsing_failed;
> 
> globals?  really?  That's bold :(
> 

Yeah, ugly. However, patch 7/9 is making cap_parsing_failed static. The other
two can be made static already, I should have done that in the first place. :(

BTW, with this set I'm trying to incrementally fix things (after moving code in
the new place), does it look reasonable to you or would you prefer to squash
intermediate steps?

> > +
> > +void normalize_cpu_capacity(void)
> 
> naming is hard, but try to put a good, descriptive, prefix on everything
> you are exporting in the same file, the same prefix.
> 
> cpu_capacity_normalize()?
> cpu_capacity_register_sysctl()?
> 
> and so on.
> 
> > +{
> > +	u64 capacity;
> > +	int cpu;
> > +
> > +	if (!raw_capacity || cap_parsing_failed)
> > +		return;
> > +
> > +	pr_debug("cpu_capacity: capacity_scale=%u\n", capacity_scale);
> > +	mutex_lock(&cpu_scale_mutex);
> > +	for_each_possible_cpu(cpu) {
> > +		pr_debug("cpu_capacity: cpu=%d raw_capacity=%u\n",
> > +			 cpu, raw_capacity[cpu]);
> > +		capacity = (raw_capacity[cpu] << SCHED_CAPACITY_SHIFT)
> > +			/ capacity_scale;
> > +		set_capacity_scale(cpu, capacity);
> > +		pr_debug("cpu_capacity: CPU%d cpu_capacity=%lu\n",
> > +			cpu, arch_scale_cpu_capacity(NULL, cpu));
> > +	}
> > +	mutex_unlock(&cpu_scale_mutex);
> > +}
> > +
> > +int __init parse_cpu_capacity(struct device_node *cpu_node, int cpu)
> 
> cpu_capacity_parse()?
> 

OK, I'll try to fix the naming as you suggest. Thanks!

Best,

- Juri

Re: [PATCH v2 6/9] arm, arm64: factorize common cpu capacity default code

[Rob Herring]

On Fri, Feb 10, 2017 at 8:28 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Feb 09, 2017 at 09:25:22AM +0000, Juri Lelli wrote:
>> arm and arm64 share lot of code relative to parsing CPU capacity
>> information from DT, using that information for appropriate scaling and
>> exposing a sysfs interface for chaging such values at runtime.
>>
>> Factorize such code in a common place (driver/base/arch_topology.c) in
>> preparation for further additions.
>>
>> Suggested-by: Will Deacon <will.deacon@arm.com>
>> Suggested-by: Mark Rutland <mark.rutland@arm.com>
>> Suggested-by: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Russell King <linux@armlinux.org.uk>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Juri Lelli <juri.lelli@arm.com>
>> ---
>>
>> Changes from v1:
>>  - keep the original GPLv2 header
>> ---
>>  arch/arm/Kconfig             |   1 +
>>  arch/arm/kernel/topology.c   | 213 ++------------------------------------
>>  arch/arm64/Kconfig           |   1 +
>>  arch/arm64/kernel/topology.c | 219 +--------------------------------------
>>  drivers/base/Kconfig         |   8 ++
>>  drivers/base/Makefile        |   1 +
>>  drivers/base/arch_topology.c | 237 +++++++++++++++++++++++++++++++++++++++++++
>>  7 files changed, 257 insertions(+), 423 deletions(-)
>>  create mode 100644 drivers/base/arch_topology.c
>
> Ah, so you want _me_ to maintain this, ok, I better review it...
>
>> --- a/drivers/base/Kconfig
>> +++ b/drivers/base/Kconfig
>> @@ -339,4 +339,12 @@ config CMA_ALIGNMENT
>>
>>  endif
>>
>> +config GENERIC_ARCH_TOPOLOGY
>> +     bool
>> +     help
>> +       Enable support for architectures common topology code: e.g., parsing
>> +       CPU capacity information from DT, usage of such information for
>> +       appropriate scaling, sysfs interface for changing capacity values at
>> +          runtime.
>
> Mix of spaces and tabs :(
>
>> +
>>  endmenu
>> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
>> index f2816f6ff76a..397e5c344e6a 100644
>> --- a/drivers/base/Makefile
>> +++ b/drivers/base/Makefile
>> @@ -23,6 +23,7 @@ obj-$(CONFIG_SOC_BUS) += soc.o
>>  obj-$(CONFIG_PINCTRL) += pinctrl.o
>>  obj-$(CONFIG_DEV_COREDUMP) += devcoredump.o
>>  obj-$(CONFIG_GENERIC_MSI_IRQ_DOMAIN) += platform-msi.o
>> +obj-$(CONFIG_GENERIC_ARCH_TOPOLOGY) += arch_topology.o
>>
>>  obj-y                        += test/
>>
>> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
>> new file mode 100644
>> index 000000000000..c1dd430adad2
>> --- /dev/null
>> +++ b/drivers/base/arch_topology.c
>> @@ -0,0 +1,237 @@
>> +/*
>> + * driver/base/arch_topology.c - Arch specific cpu topology information
>
> No need to keep the filename in the file, you know what it is called :)
>
>> + *
>> + * Copyright (C) 2016, ARM Ltd.
>> + * Written by: Juri Lelli, ARM Ltd.
>> + *
>> + * This file is subject to the terms and conditions of the GNU General Public
>> + * License.  See the file "COPYING" in the main directory of this archive
>> + * for more details.
>
> So, v2 only?  Please be specific.  Even better yet, use a SPDX header if
> you want to, those are always nice.

Sorry to hijack this thread, but you're recommending SPDX now? You
seemed pretty negative on it last time it came up[1]. Or was that just
in context of the churn of converting existing files? Personally, I
like the use of SPDX tags over free form license text and would like
to encourage it for dts files.

Rob

[1] https://lkml.org/lkml/2015/2/5/65

Re: [PATCH v2 6/9] arm, arm64: factorize common cpu capacity default code

[Greg KH]

On Wed, Feb 15, 2017 at 05:17:05PM -0600, Rob Herring wrote:
> On Fri, Feb 10, 2017 at 8:28 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> >> + *
> >> + * Copyright (C) 2016, ARM Ltd.
> >> + * Written by: Juri Lelli, ARM Ltd.
> >> + *
> >> + * This file is subject to the terms and conditions of the GNU General Public
> >> + * License.  See the file "COPYING" in the main directory of this archive
> >> + * for more details.
> >
> > So, v2 only?  Please be specific.  Even better yet, use a SPDX header if
> > you want to, those are always nice.
> 
> Sorry to hijack this thread, but you're recommending SPDX now? You
> seemed pretty negative on it last time it came up[1]. Or was that just
> in context of the churn of converting existing files?

It was in the context of someone trying to tell someone else to do the
work for them of converting all of the existing files to use SPDX.  I've
never refused a patch from someone adding SPDX identifiers to the
kernel, in fact, _I'm_ the only one that has ever used such an
identifier on a kernel file :)

> Personally, I like the use of SPDX tags over free form license text
> and would like to encourage it for dts files.

Sure, go ahead, I'd encourage it.

thanks,

greg k-h

Re: [PATCH v2 6/9] arm, arm64: factorize common cpu capacity default code

[Juri Lelli]

Hi Greg,

did you have a chance to have a look at my replies below?

It would be really helpful to understand from you how to move forward
with this set.

Best Regards,

- Juri

On 13/02/17 15:09, Juri Lelli wrote:
> Hi Greg,
> 
> On 10/02/17 15:28, Greg KH wrote:
> > On Thu, Feb 09, 2017 at 09:25:22AM +0000, Juri Lelli wrote:
> > > arm and arm64 share lot of code relative to parsing CPU capacity
> > > information from DT, using that information for appropriate scaling and
> > > exposing a sysfs interface for chaging such values at runtime.
> > > 
> > > Factorize such code in a common place (driver/base/arch_topology.c) in
> > > preparation for further additions.
> > > 
> > > Suggested-by: Will Deacon <will.deacon@arm.com>
> > > Suggested-by: Mark Rutland <mark.rutland@arm.com>
> > > Suggested-by: Catalin Marinas <catalin.marinas@arm.com>
> > > Cc: Russell King <linux@armlinux.org.uk>
> > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > Cc: Will Deacon <will.deacon@arm.com>
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Signed-off-by: Juri Lelli <juri.lelli@arm.com>
> > > ---
> > > 
> > > Changes from v1:
> > >  - keep the original GPLv2 header
> > > ---
> > >  arch/arm/Kconfig             |   1 +
> > >  arch/arm/kernel/topology.c   | 213 ++------------------------------------
> > >  arch/arm64/Kconfig           |   1 +
> > >  arch/arm64/kernel/topology.c | 219 +--------------------------------------
> > >  drivers/base/Kconfig         |   8 ++
> > >  drivers/base/Makefile        |   1 +
> > >  drivers/base/arch_topology.c | 237 +++++++++++++++++++++++++++++++++++++++++++
> > >  7 files changed, 257 insertions(+), 423 deletions(-)
> > >  create mode 100644 drivers/base/arch_topology.c
> > 
> > Ah, so you want _me_ to maintain this, ok, I better review it...
> > 
> 
> This has been suggested as a possible way to stop replicating code between arm
> and arm64 (and possibly other archs in the future). Are you in principle OK
> with it?
> 
> Thanks a lot for your comments, please find my answers below.
> 
> > > --- a/drivers/base/Kconfig
> > > +++ b/drivers/base/Kconfig
> > > @@ -339,4 +339,12 @@ config CMA_ALIGNMENT
> > >  
> > >  endif
> > >  
> > > +config GENERIC_ARCH_TOPOLOGY
> > > +	bool
> > > +	help
> > > +	  Enable support for architectures common topology code: e.g., parsing
> > > +	  CPU capacity information from DT, usage of such information for
> > > +	  appropriate scaling, sysfs interface for changing capacity values at
> > > +          runtime.
> > 
> > Mix of spaces and tabs :(
> > 
> 
> Argh. :(
> 
> > > +
> > >  endmenu
> > > diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> > > index f2816f6ff76a..397e5c344e6a 100644
> > > --- a/drivers/base/Makefile
> > > +++ b/drivers/base/Makefile
> > > @@ -23,6 +23,7 @@ obj-$(CONFIG_SOC_BUS) += soc.o
> > >  obj-$(CONFIG_PINCTRL) += pinctrl.o
> > >  obj-$(CONFIG_DEV_COREDUMP) += devcoredump.o
> > >  obj-$(CONFIG_GENERIC_MSI_IRQ_DOMAIN) += platform-msi.o
> > > +obj-$(CONFIG_GENERIC_ARCH_TOPOLOGY) += arch_topology.o
> > >  
> > >  obj-y			+= test/
> > >  
> > > diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> > > new file mode 100644
> > > index 000000000000..c1dd430adad2
> > > --- /dev/null
> > > +++ b/drivers/base/arch_topology.c
> > > @@ -0,0 +1,237 @@
> > > +/*
> > > + * driver/base/arch_topology.c - Arch specific cpu topology information
> > 
> > No need to keep the filename in the file, you know what it is called :)
> > 
> 
> OK, removed.
> 
> > > + *
> > > + * Copyright (C) 2016, ARM Ltd.
> > > + * Written by: Juri Lelli, ARM Ltd.
> > > + *
> > > + * This file is subject to the terms and conditions of the GNU General Public
> > > + * License.  See the file "COPYING" in the main directory of this archive
> > > + * for more details.
> > 
> > So, v2 only?  Please be specific.  Even better yet, use a SPDX header if
> > you want to, those are always nice.
> > 
> 
> Yes, v2 only.
> 
>   * for more details.                                                                                                                                                                                                                    
> + *                                                                                                                                                                                                                                      
> + * Released under the GPLv2 only.                                                                                                                                                                                                       
> + * SPDX-License-Identifier: GPL-2.0 
> 
> Would do, right?
> 
> > > + */
> > > +
> > > +#include <linux/acpi.h>
> > > +#include <linux/cpu.h>
> > > +#include <linux/cpufreq.h>
> > > +#include <linux/device.h>
> > > +#include <linux/of.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/string.h>
> > > +#include <linux/topology.h>
> > > +
> > > +static DEFINE_MUTEX(cpu_scale_mutex);
> > > +static DEFINE_PER_CPU(unsigned long, cpu_scale) = SCHED_CAPACITY_SCALE;
> > > +
> > > +unsigned long arch_scale_cpu_capacity(struct sched_domain *sd, int cpu)
> > 
> > Why do you have sd here?  You never use it:
> > 
> > > +{
> > > +	return per_cpu(cpu_scale, cpu);
> > 
> > See?  What am I missing?
> > 
> 
> This is how this function is defined in kernel/sched/sched.h:
> 
> #ifndef arch_scale_cpu_capacity
> static __always_inline
> unsigned long arch_scale_cpu_capacity(struct sched_domain *sd, int cpu)
> {
> 	if (sd && (sd->flags & SD_SHARE_CPUCAPACITY) && (sd->span_weight > 1))
> 		return sd->smt_gain / sd->span_weight;
> 
> 	return SCHED_CAPACITY_SCALE;
> }
> #endif
> 
> and in this case the sd argument is used: there is a call site in fair.c
> that passes a non NULL sd, updated_cpu_capacity().
> 
> A following set of patches will re-define the function so that the
> drivers one gets used by the kernel (only arm and arm64 will currently
> want this), with something like this in arch code
> 
> #define arch_scale_cpu_capacity atd_scale_cpu_capacity
> 
> Please note that last patch of this set renames this function atd_scale_
> cpu_capacity, to (hopefully) make this approach more clear.
> 
> Does it make more sense to you?
> 
> > > +}
> > > +
> > > +void set_capacity_scale(unsigned int cpu, unsigned long capacity)
> > > +{
> > > +	per_cpu(cpu_scale, cpu) = capacity;
> > > +}
> > > +
> > > +static ssize_t cpu_capacity_show(struct device *dev,
> > > +				 struct device_attribute *attr,
> > > +				 char *buf)
> > > +{
> > > +	struct cpu *cpu = container_of(dev, struct cpu, dev);
> > > +
> > > +	return sprintf(buf, "%lu\n",
> > > +			arch_scale_cpu_capacity(NULL, cpu->dev.id));
> > > +}
> > > +
> > > +static ssize_t cpu_capacity_store(struct device *dev,
> > > +				  struct device_attribute *attr,
> > > +				  const char *buf,
> > > +				  size_t count)
> > > +{
> > > +	struct cpu *cpu = container_of(dev, struct cpu, dev);
> > > +	int this_cpu = cpu->dev.id, i;
> > 
> > new line for:
> > 	int i;
> > please.
> > 
> 
> Sure.
> 
> > > +	unsigned long new_capacity;
> > > +	ssize_t ret;
> > > +
> > > +	if (count) {
> > 
> > 	if (!count)
> > 		return 0;
> > 
> > then you can get on with the rest of the logic.  Don't indent if you
> > don't have to.
> > 
> 
> Right.
> 
> > > +		ret = kstrtoul(buf, 0, &new_capacity);
> > > +		if (ret)
> > > +			return ret;
> > > +		if (new_capacity > SCHED_CAPACITY_SCALE)
> > > +			return -EINVAL;
> > > +
> > > +		mutex_lock(&cpu_scale_mutex);
> > > +		for_each_cpu(i, &cpu_topology[this_cpu].core_sibling)
> > > +			set_capacity_scale(i, new_capacity);
> > > +		mutex_unlock(&cpu_scale_mutex);
> > > +	}
> > > +
> > > +	return count;
> > > +}
> > 
> > No documentation for these sysfs file?  Not good :(
> > 
> 
> Patch 2/9 introduces some documentation. There is already more in
> Documentation/devicetree/bindings/arm/cpu-capacity.txt.
> 
> Do you think I should improve further?
> 
> > > +
> > > +static DEVICE_ATTR_RW(cpu_capacity);
> > > +
> > > +static int register_cpu_capacity_sysctl(void)
> > > +{
> > > +	int i;
> > > +	struct device *cpu;
> > > +
> > > +	for_each_possible_cpu(i) {
> > > +		cpu = get_cpu_device(i);
> > > +		if (!cpu) {
> > > +			pr_err("%s: too early to get CPU%d device!\n",
> > > +			       __func__, i);
> > 
> > What is this going to help with?
> > 
> 
> Not much I guess, I can remove it.
> 
> > > +			continue;
> > > +		}
> > > +		device_create_file(cpu, &dev_attr_cpu_capacity);
> > 
> > You realize you just raced userspace, right?  Why do it this way and not
> > register the files when the CPU device is created/removed?
> > 
> 
> Humm, my intention for doing it this way is that I'd like to make all
> the code dealing with cpu_capacity confined in a single place (this
> file), without the need to modify other files.
> 
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +subsys_initcall(register_cpu_capacity_sysctl);
> 
> AFAIU, for both arm and arm64 CPU device is registered with a
> subsys_initcall(topology_init), so I'm doing the same. Other archs seem to do
> similar things. Could you explain a little more why this is a problem?
> 
> > > +
> > > +u32 capacity_scale;
> > > +u32 *raw_capacity;
> > > +bool cap_parsing_failed;
> > 
> > globals?  really?  That's bold :(
> > 
> 
> Yeah, ugly. However, patch 7/9 is making cap_parsing_failed static. The other
> two can be made static already, I should have done that in the first place. :(
> 
> BTW, with this set I'm trying to incrementally fix things (after moving code in
> the new place), does it look reasonable to you or would you prefer to squash
> intermediate steps?
> 
> > > +
> > > +void normalize_cpu_capacity(void)
> > 
> > naming is hard, but try to put a good, descriptive, prefix on everything
> > you are exporting in the same file, the same prefix.
> > 
> > cpu_capacity_normalize()?
> > cpu_capacity_register_sysctl()?
> > 
> > and so on.
> > 
> > > +{
> > > +	u64 capacity;
> > > +	int cpu;
> > > +
> > > +	if (!raw_capacity || cap_parsing_failed)
> > > +		return;
> > > +
> > > +	pr_debug("cpu_capacity: capacity_scale=%u\n", capacity_scale);
> > > +	mutex_lock(&cpu_scale_mutex);
> > > +	for_each_possible_cpu(cpu) {
> > > +		pr_debug("cpu_capacity: cpu=%d raw_capacity=%u\n",
> > > +			 cpu, raw_capacity[cpu]);
> > > +		capacity = (raw_capacity[cpu] << SCHED_CAPACITY_SHIFT)
> > > +			/ capacity_scale;
> > > +		set_capacity_scale(cpu, capacity);
> > > +		pr_debug("cpu_capacity: CPU%d cpu_capacity=%lu\n",
> > > +			cpu, arch_scale_cpu_capacity(NULL, cpu));
> > > +	}
> > > +	mutex_unlock(&cpu_scale_mutex);
> > > +}
> > > +
> > > +int __init parse_cpu_capacity(struct device_node *cpu_node, int cpu)
> > 
> > cpu_capacity_parse()?
> > 
> 
> OK, I'll try to fix the naming as you suggest. Thanks!
> 
> Best,
> 
> - Juri