| Message ID | 1471315139-28285-4-git-send-email-finley.xiao@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Finley,
[auto build test ERROR on battery/master]
[also build test ERROR on v4.8-rc2 next-20160815]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Finlye-Xiao/PM-AVS-add-Rockchip-cpu-avs/20160816-105228
base: git://git.infradead.org/battery-2.6.git master
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
All error/warnings (new ones prefixed by >>):
drivers/power/avs/rockchip-cpu-avs.c: In function 'rockchip_cpu_avs_notifier':
>> drivers/power/avs/rockchip-cpu-avs.c:230:10: error: implicit declaration of function 'cpufreq_frequency_get_table' [-Werror=implicit-function-declaration]
table = cpufreq_frequency_get_table(policy->cpu);
^
>> drivers/power/avs/rockchip-cpu-avs.c:230:8: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
table = cpufreq_frequency_get_table(policy->cpu);
^
cc1: some warnings being treated as errors
vim +/cpufreq_frequency_get_table +230 drivers/power/avs/rockchip-cpu-avs.c
224 dev = get_cpu_device(policy->cpu);
225 if (!dev) {
226 pr_err("cpu%d Failed to get device\n", policy->cpu);
227 goto out;
228 }
229
> 230 table = cpufreq_frequency_get_table(policy->cpu);
231 if (!table) {
232 pr_err("cpu%d CPUFreq table not found\n", policy->cpu);
233 goto out;
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Finley, Am Dienstag, 16. August 2016, 10:38:59 schrieb Finlye Xiao: > From: Finley Xiao <finley.xiao@rock-chips.com> > > This patch supports adjusting opp's voltage according to leakage > > Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com> we of course talked about this before and it generally looks good, I just found a bunch of smaller issues below. > --- > .../devicetree/bindings/power/rockchip-cpu-avs.txt | 37 +++ > drivers/power/avs/Kconfig | 8 + > drivers/power/avs/Makefile | 1 + > drivers/power/avs/rockchip-cpu-avs.c | 314 > +++++++++++++++++++++ 4 files changed, 360 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt create mode > 100644 drivers/power/avs/rockchip-cpu-avs.c > > diff --git a/Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt > b/Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt new file > mode 100644 > index 0000000..90f6b08 > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt the binding should generally be a separate patch, so that Rob and other devicetree-maintainers can spot them better, so name it something like dt-bindings: add binding document for Rockchip cpu avs or similar. > @@ -0,0 +1,37 @@ > +Rockchip cpu avs device tree bindings > +------------------------------------- > + > +Under the same frequency, the operating voltage tends to decrease with > +increasing leakage. so it is necessary to adjust opp's voltage according > +to leakage for power. > + > + > +Required properties: > +- compatible: Should be one of the following. > + - "rockchip,rk3399-cpu-avs" - for RK3399 SoCs. > +- leakage-volt-<name>: Named leakage-volt property. At runtime, the > + platform can find a cpu's cluster_id according to it's cpu_id and match > + leakage-volt-<name> property. The property is an array of 3-tuples > + items, and each item consists of leakage and voltage like > + <min-leakage-mA max-leakage-mA vol-uV>. vol -> volt ? > + min-leakage: minimum leakage in mA. > + max-leakage: maximum leakage in mA. > + vol: voltage in microvolt. vol -> volt? maybe also make that volt: voltage offset in mV to apply to the opp table entries > + > +Example: > + > + cpu_avs: cpu-avs { > + compatible = "rockchip,rk3399-cpu-avs"; > + leakage-volt-cluster0 = < > + /* mA mA uV*/ > + 0 100 0 > + 101 200 (-25000) > + 201 300 (-50000) > + >; > + leakage-volt-cluster1 = < > + /* mA mA uV*/ > + 0 100 0 > + 101 200 (-25000) > + 201 300 (-50000) > + >; > + }; > diff --git a/drivers/power/avs/Kconfig b/drivers/power/avs/Kconfig > index a67eeac..c8f2d09 100644 > --- a/drivers/power/avs/Kconfig > +++ b/drivers/power/avs/Kconfig > @@ -18,3 +18,11 @@ config ROCKCHIP_IODOMAIN > Say y here to enable support io domains on Rockchip SoCs. It is > necessary for the io domain setting of the SoC to match the > voltage supplied by the regulators. > + > +config ROCKCHIP_CPU_AVS > + bool "Rockchip CPU AVS support" > + depends on POWER_AVS && ARCH_ROCKCHIP && OF as the kbuild-robot found out, you'll need as well a depends on CPU_FREQ also, I don't know if this also applies to the rk3288 and before (arm32), but if so and with your generic depends on ARCH_ROCKCHIP, you'd also need a depends on ARM_CPU_TOPOLOGY if ARM as the topology-stuff is not always active on arm32. > + help > + Say y here to enable support CPU AVS on Rockchip SoCs. > + The cpu's operating voltage is adapted depending on leakage > + or pvtm. > diff --git a/drivers/power/avs/Makefile b/drivers/power/avs/Makefile > index ba4c7bc..11ce242 100644 > --- a/drivers/power/avs/Makefile > +++ b/drivers/power/avs/Makefile > @@ -1,2 +1,3 @@ > obj-$(CONFIG_POWER_AVS_OMAP) += smartreflex.o > obj-$(CONFIG_ROCKCHIP_IODOMAIN) += rockchip-io-domain.o > +obj-$(CONFIG_ROCKCHIP_CPU_AVS) += rockchip-cpu-avs.o > diff --git a/drivers/power/avs/rockchip-cpu-avs.c > b/drivers/power/avs/rockchip-cpu-avs.c new file mode 100644 > index 0000000..8266c02 > --- /dev/null > +++ b/drivers/power/avs/rockchip-cpu-avs.c > @@ -0,0 +1,314 @@ > +/* > + * Rockchip CPU AVS support. > + * > + * Copyright (c) 2016 ROCKCHIP, Co. Ltd. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt copy'n'pasted from somewhere? Should probably not be here > +#include <linux/cpu.h> > +#include <linux/cpufreq.h> > +#include <linux/cpumask.h> > +#include <linux/err.h> > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/nvmem-consumer.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/pm_opp.h> > +#include <linux/slab.h> > +#include <linux/types.h> > +#include "../../base/power/opp/opp.h" > + > +#define MAX_NAME_LEN 22 > +#define LEAKAGE_TABLE_END ~1 > +#define INVALID_VALUE 0xff > + > +struct leakage_volt_table { > + int min; > + int max; > + int volt; > +}; > + > +struct leakage_volt_table *leakage_volt_table; you have the leakage_volt_table nicely in the rockchip_cpu_avs struct, so that looks like something forgotten? > +struct rockchip_cpu_avs { > + struct leakage_volt_table **volt_table; > + struct notifier_block cpufreq_notify; > +}; > + > +#define notifier_to_avs(_n) container_of(_n, struct rockchip_cpu_avs, \ > + cpufreq_notify) > + > +static unsigned char rockchip_fetch_leakage(struct device *dev) > +{ > + struct nvmem_cell *cell; > + unsigned char *buf; > + size_t len; > + unsigned char leakage = INVALID_VALUE; hmm, I don't trust that INVALID_VALUE to much - as it blocks the 255 value from the efuse. Can you tell what is the value range for the leakage in the efuse? Especially, as the range in your conversion-table is reaching 300, 255 does look like it might be a valid value on some chip. So I'd suggest to convert "leakage" and the function return type to an int. That way you can easily keep the full value range and also put the actual errors (as negative values) into it so that callers can act accordingly. > + > + cell = nvmem_cell_get(dev, "cpu_leakage"); > + if (IS_ERR(cell)) { > + pr_err("failed to get cpu_leakage cell\n"); > + return INVALID_VALUE; > + } > + > + buf = (unsigned char *)nvmem_cell_read(cell, &len); > + > + nvmem_cell_put(cell); > + > + if (IS_ERR(buf)) { > + pr_err("failed to read nvmem cell\n"); > + return INVALID_VALUE; > + } > + leakage = buf[0]; > + kfree(buf); > + > + return leakage; > +} > + > +static int rockchip_fetch_leakage_volt_table( > + struct device_node *np, > + struct leakage_volt_table **table, > + const char *name) > +{ > + struct leakage_volt_table *volt_table = NULL; > + const struct property *prop; > + int count, i; > + > + prop = of_find_property(np, name, NULL); > + if (!prop) { > + pr_err("failed to find prop %s\n", name); > + return -EINVAL; > + } > + if (!prop->value) { > + pr_err("%s value is NULL\n", name); > + return -ENODATA; > + } > + > + count = of_property_count_u32_elems(np, name); > + if (count < 0) { > + pr_err("Invalid %s property (%d)\n", name, count); > + return -EINVAL; > + } > + if (count % 3) { > + pr_err("Invalid number of elements in %s property (%d)\n", > + name, count); > + return -EINVAL; > + } > + > + volt_table = kzalloc(sizeof(*table) * (count / 3 + 1), GFP_KERNEL); > + if (!volt_table) > + return -ENOMEM; > + > + if (volt_table) { > + for (i = 0; i < count / 3; i++) { > + of_property_read_s32_index(np, name, 3 * i, > + &volt_table[i].min); > + of_property_read_s32_index(np, name, 3 * i + 1, > + &volt_table[i].max); > + of_property_read_s32_index(np, name, 3 * i + 2, > + &volt_table[i].volt); > + } > + volt_table[i].min = 0; > + volt_table[i].max = 0; > + volt_table[i].volt = LEAKAGE_TABLE_END; > + } > + > + *table = volt_table; > + > + return 0; > +} > + > +static int rockchip_parse_leakage_volt(unsigned char leakage, > + unsigned int cpu, > + struct rockchip_cpu_avs *avs) > +{ > + struct leakage_volt_table *table; > + unsigned int i, j, id; > + int volt; > + > + id = topology_physical_package_id(cpu); > + if (id < 0) > + id = 0; looks strange. If the cluster cannot be determined, shouldn't this just return the error and not work with possible wrong assumptions? > + table = avs->volt_table[id]; > + if (!table) > + return 0; > + > + for (i = 0; table[i].volt != LEAKAGE_TABLE_END; i++) { > + if (leakage >= table[i].min) > + j = i; > + } > + > + volt = table[j].volt; > + > + return volt; why not directly "return table[j].volt"? No need to assign and then return. > +} > + > +static void rockchip_adjust_opp_table(struct device *dev, > + struct cpufreq_frequency_table *table, > + int volt) > +{ > + struct opp_table *opp_table; > + struct cpufreq_frequency_table *pos; > + struct dev_pm_opp *opp; > + int ret; > + > + rcu_read_lock(); > + > + opp_table = _find_opp_table(dev); > + if (IS_ERR(opp_table)) { > + pr_err("failed to find OPP table\n"); > + rcu_read_unlock(); > + return; > + } > + > + cpufreq_for_each_valid_entry(pos, table) { > + opp = dev_pm_opp_find_freq_exact(dev, pos->frequency * 1000, > + true); > + if (IS_ERR(opp)) { > + pr_err("failed to find OPP for freq %d (%d)\n", > + pos->frequency, ret); > + continue; > + } > + opp->u_volt += volt; > + opp->u_volt_min += volt; > + opp->u_volt_max += volt; > + } > + > + rcu_read_unlock(); > +} > + > +static void rockchip_adjust_volt_by_leakage( > + struct device *dev, > + struct cpufreq_frequency_table *table, > + unsigned int cpu, > + struct rockchip_cpu_avs *avs) > +{ > + unsigned char leakage; > + int volt; > + > + /* fetch leakage from efuse */ > + leakage = rockchip_fetch_leakage(dev); > + if (leakage == INVALID_VALUE) { > + pr_err("cpu%d leakage invalid\n", cpu); > + return; > + } > + > + /* fetch adjust volt from table */ > + volt = rockchip_parse_leakage_volt(leakage, cpu, avs); > + if (volt) > + rockchip_adjust_opp_table(dev, table, volt); > + > + pr_debug("cpu%d, leakage=%d, adjust_volt=%d\n", cpu, leakage, volt); > +} > + > +static int rockchip_cpu_avs_notifier(struct notifier_block *nb, > + unsigned long event, void *data) > +{ > + struct rockchip_cpu_avs *avs = notifier_to_avs(nb); > + struct cpufreq_policy *policy = data; > + struct device *dev; > + > + struct cpufreq_frequency_table *table; > + > + if (event != CPUFREQ_START) > + goto out; > + > + dev = get_cpu_device(policy->cpu); > + if (!dev) { > + pr_err("cpu%d Failed to get device\n", policy->cpu); > + goto out; > + } > + > + table = cpufreq_frequency_get_table(policy->cpu); > + if (!table) { > + pr_err("cpu%d CPUFreq table not found\n", policy->cpu); > + goto out; > + } > + > + rockchip_adjust_volt_by_leakage(dev, table, policy->cpu, avs); > + > +out: > + > + return NOTIFY_OK; > +} > + > +static const struct of_device_id rockchip_cpu_avs_match[] = { > + { > + .compatible = "rockchip,rk3399-cpu-avs", > + }, > + { /* sentinel */ }, > +}; > +MODULE_DEVICE_TABLE(of, rockchip_cpu_avs_match); > + > +static int rockchip_cpu_avs_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + struct rockchip_cpu_avs *avs; > + char name[MAX_NAME_LEN]; > + int i, ret, cpu, id; > + int last_id = -1; > + int cluster_num = 0; > + > + avs = devm_kzalloc(&pdev->dev, sizeof(struct rockchip_cpu_avs), > + GFP_KERNEL); > + if (!avs) > + return -ENOMEM; > + > + avs->cpufreq_notify.notifier_call = rockchip_cpu_avs_notifier; > + > + for_each_online_cpu(cpu) { > + id = topology_physical_package_id(cpu); > + if (id < 0) > + id = 0; same as above > + if (id != last_id) { > + last_id = id; > + cluster_num++; > + } > + } > + > + avs->volt_table = devm_kzalloc(&pdev->dev, > + sizeof(struct leakage_volt_table) * cluster_num, GFP_KERNEL); > + if (!avs->volt_table) > + return -ENOMEM; > + > + for (i = 0; i < cluster_num; i++) { > + snprintf(name, MAX_NAME_LEN, "leakage-volt-cluster%d", i); > + ret = rockchip_fetch_leakage_volt_table(np, &avs->volt_table[i], > + name); > + if (ret) > + continue; > + } > + > + return cpufreq_register_notifier(&avs->cpufreq_notify, > + CPUFREQ_POLICY_NOTIFIER); > +} > + > +static struct platform_driver rockchip_cpu_avs_driver = { > + .probe = rockchip_cpu_avs_probe, > + .driver = { > + .name = "rockchip-cpu-avs", > + .of_match_table = rockchip_cpu_avs_match, > + .suppress_bind_attrs = true, > + }, > +}; > + > +static int __init rockchip_cpu_avs_module_init(void) > +{ > + return platform_driver_probe(&rockchip_cpu_avs_driver, > + rockchip_cpu_avs_probe); > +} > + > +subsys_initcall(rockchip_cpu_avs_module_init); > + > +MODULE_DESCRIPTION("Rockchip CPU AVS driver"); > +MODULE_AUTHOR("Finley Xiao <finley.xiao@rock-chips.com>"); > +MODULE_LICENSE("GPL v2");
在 2016/8/17 1:24, Heiko Stübner 写道: > Hi Finley, > > Am Dienstag, 16. August 2016, 10:38:59 schrieb Finlye Xiao: >> From: Finley Xiao <finley.xiao@rock-chips.com> >> >> This patch supports adjusting opp's voltage according to leakage >> >> Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com> > we of course talked about this before and it generally looks good, I just > found a bunch of smaller issues below. > > >> --- >> .../devicetree/bindings/power/rockchip-cpu-avs.txt | 37 +++ >> drivers/power/avs/Kconfig | 8 + >> drivers/power/avs/Makefile | 1 + >> drivers/power/avs/rockchip-cpu-avs.c | 314 >> +++++++++++++++++++++ 4 files changed, 360 insertions(+) >> create mode 100644 >> Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt create mode >> 100644 drivers/power/avs/rockchip-cpu-avs.c >> >> diff --git a/Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt >> b/Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt new file >> mode 100644 >> index 0000000..90f6b08 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt > the binding should generally be a separate patch, so that Rob and other > devicetree-maintainers can spot them better, so name it something like > dt-bindings: add binding document for Rockchip cpu avs > or similar. > > >> @@ -0,0 +1,37 @@ >> +Rockchip cpu avs device tree bindings >> +------------------------------------- >> + >> +Under the same frequency, the operating voltage tends to decrease with >> +increasing leakage. so it is necessary to adjust opp's voltage according >> +to leakage for power. >> + >> + >> +Required properties: >> +- compatible: Should be one of the following. >> + - "rockchip,rk3399-cpu-avs" - for RK3399 SoCs. >> +- leakage-volt-<name>: Named leakage-volt property. At runtime, the >> + platform can find a cpu's cluster_id according to it's cpu_id and match >> + leakage-volt-<name> property. The property is an array of 3-tuples >> + items, and each item consists of leakage and voltage like >> + <min-leakage-mA max-leakage-mA vol-uV>. > vol -> volt ? > >> + min-leakage: minimum leakage in mA. >> + max-leakage: maximum leakage in mA. >> + vol: voltage in microvolt. > vol -> volt? > > maybe also make that > volt: voltage offset in mV to apply to the opp table entries > > >> + >> +Example: >> + >> + cpu_avs: cpu-avs { >> + compatible = "rockchip,rk3399-cpu-avs"; >> + leakage-volt-cluster0 = < >> + /* mA mA uV*/ >> + 0 100 0 >> + 101 200 (-25000) >> + 201 300 (-50000) >> + >; >> + leakage-volt-cluster1 = < >> + /* mA mA uV*/ >> + 0 100 0 >> + 101 200 (-25000) >> + 201 300 (-50000) >> + >; >> + }; >> diff --git a/drivers/power/avs/Kconfig b/drivers/power/avs/Kconfig >> index a67eeac..c8f2d09 100644 >> --- a/drivers/power/avs/Kconfig >> +++ b/drivers/power/avs/Kconfig >> @@ -18,3 +18,11 @@ config ROCKCHIP_IODOMAIN >> Say y here to enable support io domains on Rockchip SoCs. It is >> necessary for the io domain setting of the SoC to match the >> voltage supplied by the regulators. >> + >> +config ROCKCHIP_CPU_AVS >> + bool "Rockchip CPU AVS support" >> + depends on POWER_AVS && ARCH_ROCKCHIP && OF > as the kbuild-robot found out, you'll need as well a > depends on CPU_FREQ > > also, I don't know if this also applies to the rk3288 and before (arm32), but > if so and with your generic depends on ARCH_ROCKCHIP, you'd also need a > depends on ARM_CPU_TOPOLOGY if ARM > > as the topology-stuff is not always active on arm32. > > >> + help >> + Say y here to enable support CPU AVS on Rockchip SoCs. >> + The cpu's operating voltage is adapted depending on leakage >> + or pvtm. >> diff --git a/drivers/power/avs/Makefile b/drivers/power/avs/Makefile >> index ba4c7bc..11ce242 100644 >> --- a/drivers/power/avs/Makefile >> +++ b/drivers/power/avs/Makefile >> @@ -1,2 +1,3 @@ >> obj-$(CONFIG_POWER_AVS_OMAP) += smartreflex.o >> obj-$(CONFIG_ROCKCHIP_IODOMAIN) += rockchip-io-domain.o >> +obj-$(CONFIG_ROCKCHIP_CPU_AVS) += rockchip-cpu-avs.o >> diff --git a/drivers/power/avs/rockchip-cpu-avs.c >> b/drivers/power/avs/rockchip-cpu-avs.c new file mode 100644 >> index 0000000..8266c02 >> --- /dev/null >> +++ b/drivers/power/avs/rockchip-cpu-avs.c >> @@ -0,0 +1,314 @@ >> +/* >> + * Rockchip CPU AVS support. >> + * >> + * Copyright (c) 2016 ROCKCHIP, Co. Ltd. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + */ >> + >> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > copy'n'pasted from somewhere? Should probably not be here > > >> +#include <linux/cpu.h> >> +#include <linux/cpufreq.h> >> +#include <linux/cpumask.h> >> +#include <linux/err.h> >> +#include <linux/init.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/nvmem-consumer.h> >> +#include <linux/of.h> >> +#include <linux/platform_device.h> >> +#include <linux/pm_opp.h> >> +#include <linux/slab.h> >> +#include <linux/types.h> >> +#include "../../base/power/opp/opp.h" >> + >> +#define MAX_NAME_LEN 22 >> +#define LEAKAGE_TABLE_END ~1 >> +#define INVALID_VALUE 0xff >> + >> +struct leakage_volt_table { >> + int min; >> + int max; >> + int volt; >> +}; >> + >> +struct leakage_volt_table *leakage_volt_table; > you have the leakage_volt_table nicely in the rockchip_cpu_avs struct, so that > looks like something forgotten? > > >> +struct rockchip_cpu_avs { >> + struct leakage_volt_table **volt_table; >> + struct notifier_block cpufreq_notify; >> +}; >> + >> +#define notifier_to_avs(_n) container_of(_n, struct rockchip_cpu_avs, \ >> + cpufreq_notify) >> + >> +static unsigned char rockchip_fetch_leakage(struct device *dev) >> +{ >> + struct nvmem_cell *cell; >> + unsigned char *buf; >> + size_t len; >> + unsigned char leakage = INVALID_VALUE; > hmm, I don't trust that INVALID_VALUE to much - as it blocks the 255 value > from the efuse. Can you tell what is the value range for the leakage in the > efuse? Especially, as the range in your conversion-table is reaching 300, 255 > does look like it might be a valid value on some chip. According to the internal document, the cpu leakage ranges from 0 to 254, 0xff is considered to be an invalid value on Rockchip SoCs. I will modify the conversion-table. > > So I'd suggest to convert "leakage" and the function return type to an int. > That way you can easily keep the full value range and also put the actual > errors (as negative values) into it so that callers can act accordingly. > > >> + >> + cell = nvmem_cell_get(dev, "cpu_leakage"); >> + if (IS_ERR(cell)) { >> + pr_err("failed to get cpu_leakage cell\n"); >> + return INVALID_VALUE; >> + } >> + >> + buf = (unsigned char *)nvmem_cell_read(cell, &len); >> + >> + nvmem_cell_put(cell); >> + >> + if (IS_ERR(buf)) { >> + pr_err("failed to read nvmem cell\n"); >> + return INVALID_VALUE; >> + } >> + leakage = buf[0]; >> + kfree(buf); >> + >> + return leakage; >> +} >> + >> +static int rockchip_fetch_leakage_volt_table( >> + struct device_node *np, >> + struct leakage_volt_table **table, >> + const char *name) >> +{ >> + struct leakage_volt_table *volt_table = NULL; >> + const struct property *prop; >> + int count, i; >> + >> + prop = of_find_property(np, name, NULL); >> + if (!prop) { >> + pr_err("failed to find prop %s\n", name); >> + return -EINVAL; >> + } >> + if (!prop->value) { >> + pr_err("%s value is NULL\n", name); >> + return -ENODATA; >> + } >> + >> + count = of_property_count_u32_elems(np, name); >> + if (count < 0) { >> + pr_err("Invalid %s property (%d)\n", name, count); >> + return -EINVAL; >> + } >> + if (count % 3) { >> + pr_err("Invalid number of elements in %s property (%d)\n", >> + name, count); >> + return -EINVAL; >> + } >> + >> + volt_table = kzalloc(sizeof(*table) * (count / 3 + 1), GFP_KERNEL); >> + if (!volt_table) >> + return -ENOMEM; >> + >> + if (volt_table) { >> + for (i = 0; i < count / 3; i++) { >> + of_property_read_s32_index(np, name, 3 * i, >> + &volt_table[i].min); >> + of_property_read_s32_index(np, name, 3 * i + 1, >> + &volt_table[i].max); >> + of_property_read_s32_index(np, name, 3 * i + 2, >> + &volt_table[i].volt); >> + } >> + volt_table[i].min = 0; >> + volt_table[i].max = 0; >> + volt_table[i].volt = LEAKAGE_TABLE_END; >> + } >> + >> + *table = volt_table; >> + >> + return 0; >> +} >> + >> +static int rockchip_parse_leakage_volt(unsigned char leakage, >> + unsigned int cpu, >> + struct rockchip_cpu_avs *avs) >> +{ >> + struct leakage_volt_table *table; >> + unsigned int i, j, id; >> + int volt; >> + >> + id = topology_physical_package_id(cpu); >> + if (id < 0) >> + id = 0; > looks strange. If the cluster cannot be determined, shouldn't this just return > the error and not work with possible wrong assumptions? > > >> + table = avs->volt_table[id]; >> + if (!table) >> + return 0; >> + >> + for (i = 0; table[i].volt != LEAKAGE_TABLE_END; i++) { >> + if (leakage >= table[i].min) >> + j = i; >> + } >> + >> + volt = table[j].volt; >> + >> + return volt; > why not directly "return table[j].volt"? No need to assign and then return. > > >> +} >> + >> +static void rockchip_adjust_opp_table(struct device *dev, >> + struct cpufreq_frequency_table *table, >> + int volt) >> +{ >> + struct opp_table *opp_table; >> + struct cpufreq_frequency_table *pos; >> + struct dev_pm_opp *opp; >> + int ret; >> + >> + rcu_read_lock(); >> + >> + opp_table = _find_opp_table(dev); >> + if (IS_ERR(opp_table)) { >> + pr_err("failed to find OPP table\n"); >> + rcu_read_unlock(); >> + return; >> + } >> + >> + cpufreq_for_each_valid_entry(pos, table) { >> + opp = dev_pm_opp_find_freq_exact(dev, pos->frequency * 1000, >> + true); >> + if (IS_ERR(opp)) { >> + pr_err("failed to find OPP for freq %d (%d)\n", >> + pos->frequency, ret); >> + continue; >> + } >> + opp->u_volt += volt; >> + opp->u_volt_min += volt; >> + opp->u_volt_max += volt; >> + } >> + >> + rcu_read_unlock(); >> +} >> + >> +static void rockchip_adjust_volt_by_leakage( >> + struct device *dev, >> + struct cpufreq_frequency_table *table, >> + unsigned int cpu, >> + struct rockchip_cpu_avs *avs) >> +{ >> + unsigned char leakage; >> + int volt; >> + >> + /* fetch leakage from efuse */ >> + leakage = rockchip_fetch_leakage(dev); >> + if (leakage == INVALID_VALUE) { >> + pr_err("cpu%d leakage invalid\n", cpu); >> + return; >> + } >> + >> + /* fetch adjust volt from table */ >> + volt = rockchip_parse_leakage_volt(leakage, cpu, avs); >> + if (volt) >> + rockchip_adjust_opp_table(dev, table, volt); >> + >> + pr_debug("cpu%d, leakage=%d, adjust_volt=%d\n", cpu, leakage, volt); >> +} >> + >> +static int rockchip_cpu_avs_notifier(struct notifier_block *nb, >> + unsigned long event, void *data) >> +{ >> + struct rockchip_cpu_avs *avs = notifier_to_avs(nb); >> + struct cpufreq_policy *policy = data; >> + struct device *dev; >> + >> + struct cpufreq_frequency_table *table; >> + >> + if (event != CPUFREQ_START) >> + goto out; >> + >> + dev = get_cpu_device(policy->cpu); >> + if (!dev) { >> + pr_err("cpu%d Failed to get device\n", policy->cpu); >> + goto out; >> + } >> + >> + table = cpufreq_frequency_get_table(policy->cpu); >> + if (!table) { >> + pr_err("cpu%d CPUFreq table not found\n", policy->cpu); >> + goto out; >> + } >> + >> + rockchip_adjust_volt_by_leakage(dev, table, policy->cpu, avs); >> + >> +out: >> + >> + return NOTIFY_OK; >> +} >> + >> +static const struct of_device_id rockchip_cpu_avs_match[] = { >> + { >> + .compatible = "rockchip,rk3399-cpu-avs", >> + }, >> + { /* sentinel */ }, >> +}; >> +MODULE_DEVICE_TABLE(of, rockchip_cpu_avs_match); >> + >> +static int rockchip_cpu_avs_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct device_node *np = dev->of_node; >> + struct rockchip_cpu_avs *avs; >> + char name[MAX_NAME_LEN]; >> + int i, ret, cpu, id; >> + int last_id = -1; >> + int cluster_num = 0; >> + >> + avs = devm_kzalloc(&pdev->dev, sizeof(struct rockchip_cpu_avs), >> + GFP_KERNEL); >> + if (!avs) >> + return -ENOMEM; >> + >> + avs->cpufreq_notify.notifier_call = rockchip_cpu_avs_notifier; >> + >> + for_each_online_cpu(cpu) { >> + id = topology_physical_package_id(cpu); >> + if (id < 0) >> + id = 0; > same as above > > >> + if (id != last_id) { >> + last_id = id; >> + cluster_num++; >> + } >> + } >> + >> + avs->volt_table = devm_kzalloc(&pdev->dev, >> + sizeof(struct leakage_volt_table) * cluster_num, GFP_KERNEL); >> + if (!avs->volt_table) >> + return -ENOMEM; >> + >> + for (i = 0; i < cluster_num; i++) { >> + snprintf(name, MAX_NAME_LEN, "leakage-volt-cluster%d", i); >> + ret = rockchip_fetch_leakage_volt_table(np, &avs->volt_table[i], >> + name); >> + if (ret) >> + continue; >> + } >> + >> + return cpufreq_register_notifier(&avs->cpufreq_notify, >> + CPUFREQ_POLICY_NOTIFIER); >> +} >> + >> +static struct platform_driver rockchip_cpu_avs_driver = { >> + .probe = rockchip_cpu_avs_probe, >> + .driver = { >> + .name = "rockchip-cpu-avs", >> + .of_match_table = rockchip_cpu_avs_match, >> + .suppress_bind_attrs = true, >> + }, >> +}; >> + >> +static int __init rockchip_cpu_avs_module_init(void) >> +{ >> + return platform_driver_probe(&rockchip_cpu_avs_driver, >> + rockchip_cpu_avs_probe); >> +} >> + >> +subsys_initcall(rockchip_cpu_avs_module_init); >> + >> +MODULE_DESCRIPTION("Rockchip CPU AVS driver"); >> +MODULE_AUTHOR("Finley Xiao <finley.xiao@rock-chips.com>"); >> +MODULE_LICENSE("GPL v2"); > > >
On Tue, Aug 16, 2016 at 10:38:59AM +0800, Finlye Xiao wrote: > From: Finley Xiao <finley.xiao@rock-chips.com> > > This patch supports adjusting opp's voltage according to leakage > > Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com> > --- > .../devicetree/bindings/power/rockchip-cpu-avs.txt | 37 +++ > drivers/power/avs/Kconfig | 8 + > drivers/power/avs/Makefile | 1 + > drivers/power/avs/rockchip-cpu-avs.c | 314 +++++++++++++++++++++ > 4 files changed, 360 insertions(+) > create mode 100644 Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt > create mode 100644 drivers/power/avs/rockchip-cpu-avs.c > > diff --git a/Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt b/Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt > new file mode 100644 > index 0000000..90f6b08 > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt > @@ -0,0 +1,37 @@ > +Rockchip cpu avs device tree bindings > +------------------------------------- > + > +Under the same frequency, the operating voltage tends to decrease with > +increasing leakage. so it is necessary to adjust opp's voltage according > +to leakage for power. > + > + > +Required properties: > +- compatible: Should be one of the following. > + - "rockchip,rk3399-cpu-avs" - for RK3399 SoCs. > +- leakage-volt-<name>: Named leakage-volt property. At runtime, the > + platform can find a cpu's cluster_id according to it's cpu_id and match > + leakage-volt-<name> property. The property is an array of 3-tuples > + items, and each item consists of leakage and voltage like > + <min-leakage-mA max-leakage-mA vol-uV>. > + min-leakage: minimum leakage in mA. > + max-leakage: maximum leakage in mA. > + vol: voltage in microvolt. How do you determine these values? When do they vary? > + > +Example: > + > + cpu_avs: cpu-avs { > + compatible = "rockchip,rk3399-cpu-avs"; This isn't really a hardware block. For the same reasons we don't have cpufreq nodes. So I don't think this belongs in DT. Rob
在 2016/8/19 21:36, Rob Herring 写道: > On Tue, Aug 16, 2016 at 10:38:59AM +0800, Finlye Xiao wrote: >> From: Finley Xiao <finley.xiao@rock-chips.com> >> >> This patch supports adjusting opp's voltage according to leakage >> >> Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com> >> --- >> .../devicetree/bindings/power/rockchip-cpu-avs.txt | 37 +++ >> drivers/power/avs/Kconfig | 8 + >> drivers/power/avs/Makefile | 1 + >> drivers/power/avs/rockchip-cpu-avs.c | 314 +++++++++++++++++++++ >> 4 files changed, 360 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt >> create mode 100644 drivers/power/avs/rockchip-cpu-avs.c >> >> diff --git a/Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt b/Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt >> new file mode 100644 >> index 0000000..90f6b08 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt >> @@ -0,0 +1,37 @@ >> +Rockchip cpu avs device tree bindings >> +------------------------------------- >> + >> +Under the same frequency, the operating voltage tends to decrease with >> +increasing leakage. so it is necessary to adjust opp's voltage according >> +to leakage for power. >> + >> + >> +Required properties: >> +- compatible: Should be one of the following. >> + - "rockchip,rk3399-cpu-avs" - for RK3399 SoCs. >> +- leakage-volt-<name>: Named leakage-volt property. At runtime, the >> + platform can find a cpu's cluster_id according to it's cpu_id and match >> + leakage-volt-<name> property. The property is an array of 3-tuples >> + items, and each item consists of leakage and voltage like >> + <min-leakage-mA max-leakage-mA vol-uV>. >> + min-leakage: minimum leakage in mA. >> + max-leakage: maximum leakage in mA. >> + vol: voltage in microvolt. > How do you determine these values? When do they vary? How do you determine these values? run antutu-benchmark leakage freq min_volt 30mA 1608MHz 1025mV 40mA 1608MHz 1000mV 50mA 1608MHz 975mV From the table, we see the min_volt decrease with increasing leakage. So we can set the default opp-microvolt 1025mV for 1608MHz, and add an leakage_volt_cluster0 property as follows. leakage_volt_cluster0 = < /* min(mA) max(mA) volt(uV)*/ 0 40 0 41 50 (-25000) 51 254 (-50000) >; If the leakage is between 41mA and 50mA，the opp-microvolt will subtract 25mV。 If the leakage is between 41mA and 50mA，the opp-microvolt will subtract 50mV。 When do they vary? From the code, cpufreq_online --cpufreq_driver->init(policy); /* add new opp table */ --blocking_notifier_call_chain(&cpufreq_policy_notifier_list, CPUFREQ_START, policy); --cpufreq_init_policy ----cpufreq_set_policy --------__cpufreq_governor(policy, CPUFREQ_GOV_START); The cpu's opp table is added in the init function(cpufreq_init)，and I will register a cpufreq notifier, once the first cpu of cluster is onlined, my notifer will be called, and if the event is CPUFREQ_START，it will modify the opp-microvolt according to leakage_volt_cluster0. >> + >> +Example: >> + >> + cpu_avs: cpu-avs { >> + compatible = "rockchip,rk3399-cpu-avs"; > This isn't really a hardware block. For the same reasons we don't have > cpufreq nodes. So I don't think this belongs in DT. if I delete the compatible property like the thermal-zones node, is it allowed? cpu_avs: cpu-avs { leakage_volt_cluster0 = < /* min(mA) max(mA) volt(uV)*/ 0 40 0 41 50 (-25000) 51 254 (-50000) >; leakage_volt_cluster1 = < /* min(mA) max(mA) volt(uV)*/ 0 40 0 41 50 (-25000) 51 254 (-50000) >; } > > Rob > > >
Hi Finley, Am Dienstag, 23. August 2016, 12:10:27 schrieb Finley Xiao: > 在 2016/8/19 21:36, Rob Herring 写道: > > On Tue, Aug 16, 2016 at 10:38:59AM +0800, Finlye Xiao wrote: > >> From: Finley Xiao <finley.xiao@rock-chips.com> > >> > >> This patch supports adjusting opp's voltage according to leakage > >> > >> Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com> > >> --- > >> > >> .../devicetree/bindings/power/rockchip-cpu-avs.txt | 37 +++ > >> drivers/power/avs/Kconfig | 8 + > >> drivers/power/avs/Makefile | 1 + > >> drivers/power/avs/rockchip-cpu-avs.c | 314 > >> +++++++++++++++++++++ 4 files changed, 360 insertions(+) > >> create mode 100644 > >> Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt create > >> mode 100644 drivers/power/avs/rockchip-cpu-avs.c > >> > >> diff --git a/Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt > >> b/Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt new file > >> mode 100644 > >> index 0000000..90f6b08 > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt > >> @@ -0,0 +1,37 @@ > >> +Rockchip cpu avs device tree bindings > >> +------------------------------------- > >> + > >> +Under the same frequency, the operating voltage tends to decrease with > >> +increasing leakage. so it is necessary to adjust opp's voltage according > >> +to leakage for power. > >> + > >> + > >> +Required properties: > >> +- compatible: Should be one of the following. > >> + - "rockchip,rk3399-cpu-avs" - for RK3399 SoCs. > >> +- leakage-volt-<name>: Named leakage-volt property. At runtime, the > >> + platform can find a cpu's cluster_id according to it's cpu_id and > >> match > >> + leakage-volt-<name> property. The property is an array of 3-tuples > >> + items, and each item consists of leakage and voltage like > >> + <min-leakage-mA max-leakage-mA vol-uV>. > >> + min-leakage: minimum leakage in mA. > >> + max-leakage: maximum leakage in mA. > >> + vol: voltage in microvolt. > > > > How do you determine these values? When do they vary? > > How do you determine these values? > > run antutu-benchmark > leakage freq min_volt > 30mA 1608MHz 1025mV > 40mA 1608MHz 1000mV > 50mA 1608MHz 975mV > > From the table, we see the min_volt decrease with increasing leakage. > So we can set the default opp-microvolt 1025mV for 1608MHz, > and add an leakage_volt_cluster0 property as follows. > leakage_volt_cluster0 = < > /* min(mA) max(mA) volt(uV)*/ > 0 40 0 > 41 50 (-25000) > 51 254 (-50000) > > >; > > If the leakage is between 41mA and 50mA，the opp-microvolt will subtract > 25mV。 If the leakage is between 41mA and 50mA，the opp-microvolt will > subtract 50mV。 > > When do they vary? > > From the code, > cpufreq_online > --cpufreq_driver->init(policy); /* add new opp table */ > --blocking_notifier_call_chain(&cpufreq_policy_notifier_list, > CPUFREQ_START, policy); > --cpufreq_init_policy > ----cpufreq_set_policy > --------__cpufreq_governor(policy, CPUFREQ_GOV_START); > > The cpu's opp table is added in the init function(cpufreq_init)，and I will > register a cpufreq notifier, once the first cpu of cluster is onlined, > my notifer will be called, and if the event is CPUFREQ_START，it will modify > the opp-microvolt according to leakage_volt_cluster0. I think what Rob was asking with "when do they vary" is, are these leakage values the same for each and every rk3399 soc or are they determined separately for each board-type using the rk3399.
在 2016/8/23 17:03, Heiko Stübner 写道: > Hi Finley, > > Am Dienstag, 23. August 2016, 12:10:27 schrieb Finley Xiao: >> 在 2016/8/19 21:36, Rob Herring 写道: >>> On Tue, Aug 16, 2016 at 10:38:59AM +0800, Finlye Xiao wrote: >>>> From: Finley Xiao <finley.xiao@rock-chips.com> >>>> >>>> This patch supports adjusting opp's voltage according to leakage >>>> >>>> Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com> >>>> --- >>>> >>>> .../devicetree/bindings/power/rockchip-cpu-avs.txt | 37 +++ >>>> drivers/power/avs/Kconfig | 8 + >>>> drivers/power/avs/Makefile | 1 + >>>> drivers/power/avs/rockchip-cpu-avs.c | 314 >>>> +++++++++++++++++++++ 4 files changed, 360 insertions(+) >>>> create mode 100644 >>>> Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt create >>>> mode 100644 drivers/power/avs/rockchip-cpu-avs.c >>>> >>>> diff --git a/Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt >>>> b/Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt new file >>>> mode 100644 >>>> index 0000000..90f6b08 >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt >>>> @@ -0,0 +1,37 @@ >>>> +Rockchip cpu avs device tree bindings >>>> +------------------------------------- >>>> + >>>> +Under the same frequency, the operating voltage tends to decrease with >>>> +increasing leakage. so it is necessary to adjust opp's voltage according >>>> +to leakage for power. >>>> + >>>> + >>>> +Required properties: >>>> +- compatible: Should be one of the following. >>>> + - "rockchip,rk3399-cpu-avs" - for RK3399 SoCs. >>>> +- leakage-volt-<name>: Named leakage-volt property. At runtime, the >>>> + platform can find a cpu's cluster_id according to it's cpu_id and >>>> match >>>> + leakage-volt-<name> property. The property is an array of 3-tuples >>>> + items, and each item consists of leakage and voltage like >>>> + <min-leakage-mA max-leakage-mA vol-uV>. >>>> + min-leakage: minimum leakage in mA. >>>> + max-leakage: maximum leakage in mA. >>>> + vol: voltage in microvolt. >>> How do you determine these values? When do they vary? >> How do you determine these values? >> >> run antutu-benchmark >> leakage freq min_volt >> 30mA 1608MHz 1025mV >> 40mA 1608MHz 1000mV >> 50mA 1608MHz 975mV >> >> From the table, we see the min_volt decrease with increasing leakage. >> So we can set the default opp-microvolt 1025mV for 1608MHz, >> and add an leakage_volt_cluster0 property as follows. >> leakage_volt_cluster0 = < >> /* min(mA) max(mA) volt(uV)*/ >> 0 40 0 >> 41 50 (-25000) >> 51 254 (-50000) >> >>> ; >> If the leakage is between 41mA and 50mA，the opp-microvolt will subtract >> 25mV。 If the leakage is between 41mA and 50mA，the opp-microvolt will >> subtract 50mV。 >> >> When do they vary? >> >> From the code, >> cpufreq_online >> --cpufreq_driver->init(policy); /* add new opp table */ >> --blocking_notifier_call_chain(&cpufreq_policy_notifier_list, >> CPUFREQ_START, policy); >> --cpufreq_init_policy >> ----cpufreq_set_policy >> --------__cpufreq_governor(policy, CPUFREQ_GOV_START); >> >> The cpu's opp table is added in the init function(cpufreq_init)，and I will >> register a cpufreq notifier, once the first cpu of cluster is onlined, >> my notifer will be called, and if the event is CPUFREQ_START，it will modify >> the opp-microvolt according to leakage_volt_cluster0. > I think what Rob was asking with "when do they vary" is, are these leakage > values the same for each and every rk3399 soc or are they determined > separately for each board-type using the rk3399. Oh, sorry, I misunderstood. They are the same for every rk3399 soc. > > > > >
On Tue, Aug 23, 2016 at 05:29:16PM +0800, Finley Xiao wrote: > > > 在 2016/8/23 17:03, Heiko Stübner 写道: > >Hi Finley, > > > >Am Dienstag, 23. August 2016, 12:10:27 schrieb Finley Xiao: > >>在 2016/8/19 21:36, Rob Herring 写道: > >>>On Tue, Aug 16, 2016 at 10:38:59AM +0800, Finlye Xiao wrote: > >>>>From: Finley Xiao <finley.xiao@rock-chips.com> > >>>> > >>>>This patch supports adjusting opp's voltage according to leakage > >>>> > >>>>Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com> > >>>>--- > >>>> > >>>> .../devicetree/bindings/power/rockchip-cpu-avs.txt | 37 +++ > >>>> drivers/power/avs/Kconfig | 8 + > >>>> drivers/power/avs/Makefile | 1 + > >>>> drivers/power/avs/rockchip-cpu-avs.c | 314 > >>>> +++++++++++++++++++++ 4 files changed, 360 insertions(+) > >>>> create mode 100644 > >>>> Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt create > >>>> mode 100644 drivers/power/avs/rockchip-cpu-avs.c > >>>> > >>>>diff --git a/Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt > >>>>b/Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt new file > >>>>mode 100644 > >>>>index 0000000..90f6b08 > >>>>--- /dev/null > >>>>+++ b/Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt > >>>>@@ -0,0 +1,37 @@ > >>>>+Rockchip cpu avs device tree bindings > >>>>+------------------------------------- > >>>>+ > >>>>+Under the same frequency, the operating voltage tends to decrease with > >>>>+increasing leakage. so it is necessary to adjust opp's voltage according > >>>>+to leakage for power. > >>>>+ > >>>>+ > >>>>+Required properties: > >>>>+- compatible: Should be one of the following. > >>>>+ - "rockchip,rk3399-cpu-avs" - for RK3399 SoCs. > >>>>+- leakage-volt-<name>: Named leakage-volt property. At runtime, the > >>>>+ platform can find a cpu's cluster_id according to it's cpu_id and > >>>>match > >>>>+ leakage-volt-<name> property. The property is an array of 3-tuples > >>>>+ items, and each item consists of leakage and voltage like > >>>>+ <min-leakage-mA max-leakage-mA vol-uV>. > >>>>+ min-leakage: minimum leakage in mA. > >>>>+ max-leakage: maximum leakage in mA. > >>>>+ vol: voltage in microvolt. > >>>How do you determine these values? When do they vary? > >>How do you determine these values? > >> > >>run antutu-benchmark > >>leakage freq min_volt > >>30mA 1608MHz 1025mV > >>40mA 1608MHz 1000mV > >>50mA 1608MHz 975mV > >> > >> From the table, we see the min_volt decrease with increasing leakage. > >>So we can set the default opp-microvolt 1025mV for 1608MHz, > >>and add an leakage_volt_cluster0 property as follows. > >>leakage_volt_cluster0 = < > >> /* min(mA) max(mA) volt(uV)*/ > >> 0 40 0 > >> 41 50 (-25000) > >> 51 254 (-50000) > >> > >>>; > >>If the leakage is between 41mA and 50mA，the opp-microvolt will subtract > >>25mV。 If the leakage is between 41mA and 50mA，the opp-microvolt will > >>subtract 50mV。 > >> > >>When do they vary? > >> > >> From the code, > >>cpufreq_online > >>--cpufreq_driver->init(policy); /* add new opp table */ > >>--blocking_notifier_call_chain(&cpufreq_policy_notifier_list, > >>CPUFREQ_START, policy); > >>--cpufreq_init_policy > >>----cpufreq_set_policy > >>--------__cpufreq_governor(policy, CPUFREQ_GOV_START); > >> > >>The cpu's opp table is added in the init function(cpufreq_init)，and I will > >>register a cpufreq notifier, once the first cpu of cluster is onlined, > >>my notifer will be called, and if the event is CPUFREQ_START，it will modify > >>the opp-microvolt according to leakage_volt_cluster0. > >I think what Rob was asking with "when do they vary" is, are these leakage > >values the same for each and every rk3399 soc or are they determined > >separately for each board-type using the rk3399. > > Oh, sorry, I misunderstood. > They are the same for every rk3399 soc. Then I think the values should go in the kernel at least until we define a common way to handle this. Rob
diff --git a/Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt b/Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt new file mode 100644 index 0000000..90f6b08 --- /dev/null +++ b/Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt @@ -0,0 +1,37 @@ +Rockchip cpu avs device tree bindings +------------------------------------- + +Under the same frequency, the operating voltage tends to decrease with +increasing leakage. so it is necessary to adjust opp's voltage according +to leakage for power. + + +Required properties: +- compatible: Should be one of the following. + - "rockchip,rk3399-cpu-avs" - for RK3399 SoCs. +- leakage-volt-<name>: Named leakage-volt property. At runtime, the + platform can find a cpu's cluster_id according to it's cpu_id and match + leakage-volt-<name> property. The property is an array of 3-tuples + items, and each item consists of leakage and voltage like + <min-leakage-mA max-leakage-mA vol-uV>. + min-leakage: minimum leakage in mA. + max-leakage: maximum leakage in mA. + vol: voltage in microvolt. + +Example: + + cpu_avs: cpu-avs { + compatible = "rockchip,rk3399-cpu-avs"; + leakage-volt-cluster0 = < + /* mA mA uV*/ + 0 100 0 + 101 200 (-25000) + 201 300 (-50000) + >; + leakage-volt-cluster1 = < + /* mA mA uV*/ + 0 100 0 + 101 200 (-25000) + 201 300 (-50000) + >; + }; diff --git a/drivers/power/avs/Kconfig b/drivers/power/avs/Kconfig index a67eeac..c8f2d09 100644 --- a/drivers/power/avs/Kconfig +++ b/drivers/power/avs/Kconfig @@ -18,3 +18,11 @@ config ROCKCHIP_IODOMAIN Say y here to enable support io domains on Rockchip SoCs. It is necessary for the io domain setting of the SoC to match the voltage supplied by the regulators. + +config ROCKCHIP_CPU_AVS + bool "Rockchip CPU AVS support" + depends on POWER_AVS && ARCH_ROCKCHIP && OF + help + Say y here to enable support CPU AVS on Rockchip SoCs. + The cpu's operating voltage is adapted depending on leakage + or pvtm. diff --git a/drivers/power/avs/Makefile b/drivers/power/avs/Makefile index ba4c7bc..11ce242 100644 --- a/drivers/power/avs/Makefile +++ b/drivers/power/avs/Makefile @@ -1,2 +1,3 @@ obj-$(CONFIG_POWER_AVS_OMAP) += smartreflex.o obj-$(CONFIG_ROCKCHIP_IODOMAIN) += rockchip-io-domain.o +obj-$(CONFIG_ROCKCHIP_CPU_AVS) += rockchip-cpu-avs.o diff --git a/drivers/power/avs/rockchip-cpu-avs.c b/drivers/power/avs/rockchip-cpu-avs.c new file mode 100644 index 0000000..8266c02 --- /dev/null +++ b/drivers/power/avs/rockchip-cpu-avs.c @@ -0,0 +1,314 @@ +/* + * Rockchip CPU AVS support. + * + * Copyright (c) 2016 ROCKCHIP, Co. Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/cpu.h> +#include <linux/cpufreq.h> +#include <linux/cpumask.h> +#include <linux/err.h> +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/nvmem-consumer.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pm_opp.h> +#include <linux/slab.h> +#include <linux/types.h> +#include "../../base/power/opp/opp.h" + +#define MAX_NAME_LEN 22 +#define LEAKAGE_TABLE_END ~1 +#define INVALID_VALUE 0xff + +struct leakage_volt_table { + int min; + int max; + int volt; +}; + +struct leakage_volt_table *leakage_volt_table; + +struct rockchip_cpu_avs { + struct leakage_volt_table **volt_table; + struct notifier_block cpufreq_notify; +}; + +#define notifier_to_avs(_n) container_of(_n, struct rockchip_cpu_avs, \ + cpufreq_notify) + +static unsigned char rockchip_fetch_leakage(struct device *dev) +{ + struct nvmem_cell *cell; + unsigned char *buf; + size_t len; + unsigned char leakage = INVALID_VALUE; + + cell = nvmem_cell_get(dev, "cpu_leakage"); + if (IS_ERR(cell)) { + pr_err("failed to get cpu_leakage cell\n"); + return INVALID_VALUE; + } + + buf = (unsigned char *)nvmem_cell_read(cell, &len); + + nvmem_cell_put(cell); + + if (IS_ERR(buf)) { + pr_err("failed to read nvmem cell\n"); + return INVALID_VALUE; + } + leakage = buf[0]; + kfree(buf); + + return leakage; +} + +static int rockchip_fetch_leakage_volt_table( + struct device_node *np, + struct leakage_volt_table **table, + const char *name) +{ + struct leakage_volt_table *volt_table = NULL; + const struct property *prop; + int count, i; + + prop = of_find_property(np, name, NULL); + if (!prop) { + pr_err("failed to find prop %s\n", name); + return -EINVAL; + } + if (!prop->value) { + pr_err("%s value is NULL\n", name); + return -ENODATA; + } + + count = of_property_count_u32_elems(np, name); + if (count < 0) { + pr_err("Invalid %s property (%d)\n", name, count); + return -EINVAL; + } + if (count % 3) { + pr_err("Invalid number of elements in %s property (%d)\n", + name, count); + return -EINVAL; + } + + volt_table = kzalloc(sizeof(*table) * (count / 3 + 1), GFP_KERNEL); + if (!volt_table) + return -ENOMEM; + + if (volt_table) { + for (i = 0; i < count / 3; i++) { + of_property_read_s32_index(np, name, 3 * i, + &volt_table[i].min); + of_property_read_s32_index(np, name, 3 * i + 1, + &volt_table[i].max); + of_property_read_s32_index(np, name, 3 * i + 2, + &volt_table[i].volt); + } + volt_table[i].min = 0; + volt_table[i].max = 0; + volt_table[i].volt = LEAKAGE_TABLE_END; + } + + *table = volt_table; + + return 0; +} + +static int rockchip_parse_leakage_volt(unsigned char leakage, + unsigned int cpu, + struct rockchip_cpu_avs *avs) +{ + struct leakage_volt_table *table; + unsigned int i, j, id; + int volt; + + id = topology_physical_package_id(cpu); + if (id < 0) + id = 0; + + table = avs->volt_table[id]; + if (!table) + return 0; + + for (i = 0; table[i].volt != LEAKAGE_TABLE_END; i++) { + if (leakage >= table[i].min) + j = i; + } + + volt = table[j].volt; + + return volt; +} + +static void rockchip_adjust_opp_table(struct device *dev, + struct cpufreq_frequency_table *table, + int volt) +{ + struct opp_table *opp_table; + struct cpufreq_frequency_table *pos; + struct dev_pm_opp *opp; + int ret; + + rcu_read_lock(); + + opp_table = _find_opp_table(dev); + if (IS_ERR(opp_table)) { + pr_err("failed to find OPP table\n"); + rcu_read_unlock(); + return; + } + + cpufreq_for_each_valid_entry(pos, table) { + opp = dev_pm_opp_find_freq_exact(dev, pos->frequency * 1000, + true); + if (IS_ERR(opp)) { + pr_err("failed to find OPP for freq %d (%d)\n", + pos->frequency, ret); + continue; + } + opp->u_volt += volt; + opp->u_volt_min += volt; + opp->u_volt_max += volt; + } + + rcu_read_unlock(); +} + +static void rockchip_adjust_volt_by_leakage( + struct device *dev, + struct cpufreq_frequency_table *table, + unsigned int cpu, + struct rockchip_cpu_avs *avs) +{ + unsigned char leakage; + int volt; + + /* fetch leakage from efuse */ + leakage = rockchip_fetch_leakage(dev); + if (leakage == INVALID_VALUE) { + pr_err("cpu%d leakage invalid\n", cpu); + return; + } + + /* fetch adjust volt from table */ + volt = rockchip_parse_leakage_volt(leakage, cpu, avs); + if (volt) + rockchip_adjust_opp_table(dev, table, volt); + + pr_debug("cpu%d, leakage=%d, adjust_volt=%d\n", cpu, leakage, volt); +} + +static int rockchip_cpu_avs_notifier(struct notifier_block *nb, + unsigned long event, void *data) +{ + struct rockchip_cpu_avs *avs = notifier_to_avs(nb); + struct cpufreq_policy *policy = data; + struct device *dev; + + struct cpufreq_frequency_table *table; + + if (event != CPUFREQ_START) + goto out; + + dev = get_cpu_device(policy->cpu); + if (!dev) { + pr_err("cpu%d Failed to get device\n", policy->cpu); + goto out; + } + + table = cpufreq_frequency_get_table(policy->cpu); + if (!table) { + pr_err("cpu%d CPUFreq table not found\n", policy->cpu); + goto out; + } + + rockchip_adjust_volt_by_leakage(dev, table, policy->cpu, avs); + +out: + + return NOTIFY_OK; +} + +static const struct of_device_id rockchip_cpu_avs_match[] = { + { + .compatible = "rockchip,rk3399-cpu-avs", + }, + { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE(of, rockchip_cpu_avs_match); + +static int rockchip_cpu_avs_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct device_node *np = dev->of_node; + struct rockchip_cpu_avs *avs; + char name[MAX_NAME_LEN]; + int i, ret, cpu, id; + int last_id = -1; + int cluster_num = 0; + + avs = devm_kzalloc(&pdev->dev, sizeof(struct rockchip_cpu_avs), + GFP_KERNEL); + if (!avs) + return -ENOMEM; + + avs->cpufreq_notify.notifier_call = rockchip_cpu_avs_notifier; + + for_each_online_cpu(cpu) { + id = topology_physical_package_id(cpu); + if (id < 0) + id = 0; + if (id != last_id) { + last_id = id; + cluster_num++; + } + } + + avs->volt_table = devm_kzalloc(&pdev->dev, + sizeof(struct leakage_volt_table) * cluster_num, GFP_KERNEL); + if (!avs->volt_table) + return -ENOMEM; + + for (i = 0; i < cluster_num; i++) { + snprintf(name, MAX_NAME_LEN, "leakage-volt-cluster%d", i); + ret = rockchip_fetch_leakage_volt_table(np, &avs->volt_table[i], + name); + if (ret) + continue; + } + + return cpufreq_register_notifier(&avs->cpufreq_notify, + CPUFREQ_POLICY_NOTIFIER); +} + +static struct platform_driver rockchip_cpu_avs_driver = { + .probe = rockchip_cpu_avs_probe, + .driver = { + .name = "rockchip-cpu-avs", + .of_match_table = rockchip_cpu_avs_match, + .suppress_bind_attrs = true, + }, +}; + +static int __init rockchip_cpu_avs_module_init(void) +{ + return platform_driver_probe(&rockchip_cpu_avs_driver, + rockchip_cpu_avs_probe); +} + +subsys_initcall(rockchip_cpu_avs_module_init); + +MODULE_DESCRIPTION("Rockchip CPU AVS driver"); +MODULE_AUTHOR("Finley Xiao <finley.xiao@rock-chips.com>"); +MODULE_LICENSE("GPL v2");