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@xxxxxxx> > Suggested-by: Mark Rutland <mark.rutland@xxxxxxx> > Suggested-by: Catalin Marinas <catalin.marinas@xxxxxxx> > Cc: Russell King <linux@xxxxxxxxxxxxxxx> > Cc: Catalin Marinas <catalin.marinas@xxxxxxx> > Cc: Will Deacon <will.deacon@xxxxxxx> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> > Signed-off-by: Juri Lelli <juri.lelli@xxxxxxx> > --- > > 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html