[PATCH v1 0/3] PM / AVS: add Rockchip cpu avs

[PATCH v1 0/3] PM / AVS: add Rockchip cpu avs

[Finlye Xiao]

From: Finley Xiao <finley.xiao@rock-chips.com>

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.

Finley Xiao (3):
  nvmem: rockchip-efuse: Change initcall to subsys
  of: Add support for reading a s32 from a multi-value property.
  PM / AVS: rockchip-cpu-avs: add driver handling Rockchip cpu avs

 .../devicetree/bindings/power/rockchip-cpu-avs.txt |  37 +++
 drivers/nvmem/rockchip-efuse.c                     |   9 +-
 drivers/of/base.c                                  |  23 ++
 drivers/power/avs/Kconfig                          |   8 +
 drivers/power/avs/Makefile                         |   1 +
 drivers/power/avs/rockchip-cpu-avs.c               | 314 +++++++++++++++++++++
 include/linux/of.h                                 |  10 +
 7 files changed, 401 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/power/rockchip-cpu-avs.txt
 create mode 100644 drivers/power/avs/rockchip-cpu-avs.c

-- 
1.9.1

[PATCH v1 1/3] nvmem: rockchip-efuse: Change initcall to subsys

[Finlye Xiao]

From: Finley Xiao <finley.xiao@rock-chips.com>

We will register a cpufreq notifier for adjusting opp's voltage, and it
need to fetch cpu's leakage from efuse in the notifier_call. so the efuse
driver should probe before cpufreq driver.

Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com>
---
 drivers/nvmem/rockchip-efuse.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/nvmem/rockchip-efuse.c b/drivers/nvmem/rockchip-efuse.c
index 4d3f391..378993d 100644
--- a/drivers/nvmem/rockchip-efuse.c
+++ b/drivers/nvmem/rockchip-efuse.c
@@ -144,6 +144,13 @@ static struct platform_driver rockchip_efuse_driver = {
 	},
 };
 
-module_platform_driver(rockchip_efuse_driver);
+static int __init rockchip_efuse_module_init(void)
+{
+	return platform_driver_probe(&rockchip_efuse_driver,
+				     rockchip_efuse_probe);
+}
+
+subsys_initcall(rockchip_efuse_module_init);
+
 MODULE_DESCRIPTION("rockchip_efuse driver");
 MODULE_LICENSE("GPL v2");
-- 
1.9.1

[PATCH v1 2/3] of: Add support for reading a s32 from a multi-value property.

[Finlye Xiao]

From: Finley Xiao <finley.xiao@rock-chips.com>

This patch adds an of_property_read_s32_index() function to allow
reading a single indexed s32 value from a property containing multiple
s32 values.

Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com>
---
 drivers/of/base.c  | 23 +++++++++++++++++++++++
 include/linux/of.h | 10 ++++++++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 7792266..346457d 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1200,6 +1200,29 @@ int of_property_read_u32_index(const struct device_node *np,
 EXPORT_SYMBOL_GPL(of_property_read_u32_index);
 
 /**
+ * of_property_read_s32_index - Find and read a s32 from a multi-value property.
+ *
+ * @np:		device node from which the property value is to be read.
+ * @propname:	name of the property to be searched.
+ * @index:	index of the u32 in the list of values
+ * @out_value:	pointer to return value, modified only if no error.
+ *
+ * Search for a property in a device node and read nth 32-bit value from
+ * it. Returns 0 on success, -EINVAL if the property does not exist,
+ * -ENODATA if property does not have a value, and -EOVERFLOW if the
+ * property data isn't large enough.
+ *
+ * The out_value is modified only if a valid s32 value can be decoded.
+ */
+int of_property_read_s32_index(const struct device_node *np,
+			       const char *propname, u32 index, s32 *out_value)
+{
+	return of_property_read_u32_index(np, propname, index,
+		(u32 *)out_value);
+}
+EXPORT_SYMBOL_GPL(of_property_read_s32_index);
+
+/**
  * of_property_read_u8_array - Find and read an array of u8 from a property.
  *
  * @np:		device node from which the property value is to be read.
diff --git a/include/linux/of.h b/include/linux/of.h
index 3d9ff8e..cb9a627 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -291,6 +291,9 @@ extern int of_property_count_elems_of_size(const struct device_node *np,
 extern int of_property_read_u32_index(const struct device_node *np,
 				       const char *propname,
 				       u32 index, u32 *out_value);
+extern int of_property_read_s32_index(const struct device_node *np,
+				      const char *propname,
+				      u32 index, s32 *out_value);
 extern int of_property_read_u8_array(const struct device_node *np,
 			const char *propname, u8 *out_values, size_t sz);
 extern int of_property_read_u16_array(const struct device_node *np,
@@ -536,6 +539,13 @@ static inline int of_property_read_u32_index(const struct device_node *np,
 	return -ENOSYS;
 }
 
+static inline int of_property_read_s32_index(const struct device_node *np,
+					     const char *propname,
+					     u32 index, s32 *out_value)
+{
+	return -ENOSYS;
+}
+
 static inline int of_property_read_u8_array(const struct device_node *np,
 			const char *propname, u8 *out_values, size_t sz)
 {
-- 
1.9.1

[PATCH v1 3/3] PM / AVS: rockchip-cpu-avs: add driver handling Rockchip cpu avs

[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>
---
 .../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.
+
+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");
-- 
1.9.1

Re: [PATCH v1 3/3] PM / AVS: rockchip-cpu-avs: add driver handling Rockchip cpu avs

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1888 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 58506 bytes --]

Re: [PATCH v1 2/3] of: Add support for reading a s32 from a multi-value property.

[Heiko Stübner]

Hi Finley,

Am Dienstag, 16. August 2016, 10:38:58 schrieb Finlye Xiao:
> From: Finley Xiao <finley.xiao@rock-chips.com>
> 
> This patch adds an of_property_read_s32_index() function to allow
> reading a single indexed s32 value from a property containing multiple
> s32 values.
>
> Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com>

for people reading along, this simply duplicates what of_property_read_s32 
already does, only for indexed properties.

But I do have one issue below.

> ---
>  drivers/of/base.c  | 23 +++++++++++++++++++++++
>  include/linux/of.h | 10 ++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 7792266..346457d 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1200,6 +1200,29 @@ int of_property_read_u32_index(const struct
> device_node *np, EXPORT_SYMBOL_GPL(of_property_read_u32_index);
> 
>  /**
> + * of_property_read_s32_index - Find and read a s32 from a multi-value
> property. + *
> + * @np:		device node from which the property value is to be read.
> + * @propname:	name of the property to be searched.
> + * @index:	index of the u32 in the list of values
> + * @out_value:	pointer to return value, modified only if no error.
> + *
> + * Search for a property in a device node and read nth 32-bit value from
> + * it. Returns 0 on success, -EINVAL if the property does not exist,
> + * -ENODATA if property does not have a value, and -EOVERFLOW if the
> + * property data isn't large enough.
> + *
> + * The out_value is modified only if a valid s32 value can be decoded.
> + */
> +int of_property_read_s32_index(const struct device_node *np,
> +			       const char *propname, u32 index, s32 *out_value)
> +{
> +	return of_property_read_u32_index(np, propname, index,
> +		(u32 *)out_value);
> +}
> +EXPORT_SYMBOL_GPL(of_property_read_s32_index);

I think this could very well live directly in the of.h header below 
of_property_read_s32 or so.

As this also only calls the more generic u32 variant, it can be just a static 
inline there and does not need to duplicate the whole stub voodoo, as the 
of_property_read_u32_index will also generate the -ENOSYS error in the !OF 
case.


Heiko

Re: [PATCH v1 3/3] PM / AVS: rockchip-cpu-avs: add driver handling Rockchip cpu avs

[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.

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");

Re: [PATCH v1 3/3] PM / AVS: rockchip-cpu-avs: add driver handling Rockchip cpu avs

[Finley Xiao]

在 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");
>
>
>

-- 
Finley

Re: [PATCH v1 3/3] PM / AVS: rockchip-cpu-avs: add driver handling Rockchip cpu avs

[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?

> +
> +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

Re: [PATCH v1 2/3] of: Add support for reading a s32 from a multi-value property.

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 869 bytes --]

On Tue, 2016-08-16 at 10:38 +0800, Finlye Xiao wrote:
> From: Finley Xiao <finley.xiao@rock-chips.com>
> 
> This patch adds an of_property_read_s32_index() function to allow
> reading a single indexed s32 value from a property containing multiple
> s32 values.
> 
> Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com>

NAK.

Nobody should be using the old of_property_* functions any more anyway.
You should be using the generic device_propery_* functions which work
regardless of where the information comes from (actual DT vs. ACPI
_DSD).

So no, don't *add* any more of these functions. Only add the generic
version. And if your driver isn't using the generic property
functions... fix it.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5760 bytes --]

Re: [PATCH v1 2/3] of: Add support for reading a s32 from a multi-value property.

[Heiko Stuebner]

Am Freitag, 19. August 2016, 15:15:19 CEST schrieb David Woodhouse:
> On Tue, 2016-08-16 at 10:38 +0800, Finlye Xiao wrote:
> > From: Finley Xiao <finley.xiao@rock-chips.com>
> > 
> > This patch adds an of_property_read_s32_index() function to allow
> > reading a single indexed s32 value from a property containing multiple
> > s32 values.
> > 
> > Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com>
> 
> NAK.
> 
> Nobody should be using the old of_property_* functions any more anyway.
> You should be using the generic device_propery_* functions which work
> regardless of where the information comes from (actual DT vs. ACPI
> _DSD).
>
> So no, don't *add* any more of these functions. Only add the generic
> version. And if your driver isn't using the generic property
> functions... fix it.

As far as I can see, all the device_property_* functions are grounded on their
of_property_*, acpi_property_* etc counterparts and functions reading specific
elements (the _index variants) are currently not available at all.

drivers/base/property.c:
#define OF_DEV_PROP_READ_ARRAY(node, propname, type, val, nval)				\
	(val) ? of_property_read_##type##_array((node), (propname), (val), (nval))	\
	      : of_property_count_elems_of_size((node), (propname), sizeof(type))

So even if you're using the device_property_* functions you'd still need
a match in the underlying functions or am I missing something?

Re: [PATCH v1 2/3] of: Add support for reading a s32 from a multi-value property.

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1175 bytes --]

On Fri, 2016-08-19 at 22:41 +0200, Heiko Stuebner wrote:
> 
> > So no, don't *add* any more of these functions. Only add the generic
> > version. And if your driver isn't using the generic property
> > functions... fix it.
> 
> As far as I can see, all the device_property_* functions are grounded on their
> of_property_*, acpi_property_* etc counterparts and functions reading specific
> elements (the _index variants) are currently not available at all.
> 
> drivers/base/property.c:
> #define OF_DEV_PROP_READ_ARRAY(node, propname, type, val, nval)                         \
>         (val) ? of_property_read_##type##_array((node), (propname), (val), (nval))      \
>               : of_property_count_elems_of_size((node), (propname), sizeof(type))
> 
> So even if you're using the device_property_* functions you'd still need
> a match in the underlying functions or am I missing something?

Yes, but the underlying function should never be used directly by
drivers. And should probably be prefixed with __ or marked deprecated
(with an override in its one genuine call site).

-- 
dwmw2



[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5760 bytes --]

Re: [PATCH v1 2/3] of: Add support for reading a s32 from a multi-value property.

[Finley Xiao]

在 2016/8/20 4:47, David Woodhouse 写道:
> On Fri, 2016-08-19 at 22:41 +0200, Heiko Stuebner wrote:
>>> So no, don't *add* any more of these functions. Only add the generic
>>> version. And if your driver isn't using the generic property
>>> functions... fix it.
>> As far as I can see, all the device_property_* functions are grounded on their
>> of_property_*, acpi_property_* etc counterparts and functions reading specific
>> elements (the _index variants) are currently not available at all.
>>
>> drivers/base/property.c:
>> #define OF_DEV_PROP_READ_ARRAY(node, propname, type, val, nval)                         \
>>          (val) ? of_property_read_##type##_array((node), (propname), (val), (nval))      \
>>                : of_property_count_elems_of_size((node), (propname), sizeof(type))
>>
>> So even if you're using the device_property_* functions you'd still need
>> a match in the underlying functions or am I missing something?
> Yes, but the underlying function should never be used directly by
> drivers. And should probably be prefixed with __ or marked deprecated
> (with an override in its one genuine call site).
>

So can I add a device_property_read_s32_array function in property.h?

--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -108,6 +108,13 @@ static inline int device_property_read_u32(struct 
device *dev,
         return device_property_read_u32_array(dev, propname, val, 1);
  }

+static inline int device_property_read_s32_array(struct device *dev, 
const char *propname,
+                                  s32 *val, size_t nval)
+{
+       return device_property_read_u32_array(struct device *dev, const 
char *propname,
+                                  (u32 *)val, size_t nval);
+}

-- 
Finley

Re: [PATCH v1 2/3] of: Add support for reading a s32 from a multi-value property.

[Mark Rutland]

On Fri, Aug 19, 2016 at 09:47:34PM +0100, David Woodhouse wrote:
> On Fri, 2016-08-19 at 22:41 +0200, Heiko Stuebner wrote:
> > 
> > > So no, don't *add* any more of these functions. Only add the generic
> > > version. And if your driver isn't using the generic property
> > > functions... fix it.
> > 
> > As far as I can see, all the device_property_* functions are grounded on their
> > of_property_*, acpi_property_* etc counterparts and functions reading specific
> > elements (the _index variants) are currently not available at all.
> > 
> > drivers/base/property.c:
> > #define OF_DEV_PROP_READ_ARRAY(node, propname, type, val, nval)                         \
> >         (val) ? of_property_read_##type##_array((node), (propname), (val), (nval))      \
> >               : of_property_count_elems_of_size((node), (propname), sizeof(type))
> > 
> > So even if you're using the device_property_* functions you'd still need
> > a match in the underlying functions or am I missing something?
> 
> Yes, but the underlying function should never be used directly by
> drivers. 

There are properties which never make sense with ACPI, and there are
drivers which shouldn't exist in the case of ACPI (due to clashes with
core ACPI functionality). So using of_property_* for those, and not
matching any ACPI tables is perfectly fine.

IMO, an of_property_read_s32_index() function is perfectly fine (as is
an ACPI-specific variant, or a unified helper, if that's useful to some
driver).

There is no need to force drivers to superficially appear to support
ACPI when they have not been designed with ACPI in mind, and are
unlikely to function in an ACPI environment.

Thanks,
Mark.

Re: [PATCH v1 3/3] PM / AVS: rockchip-cpu-avs: add driver handling Rockchip cpu avs

[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.

>> +
>> +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
>
>
>

-- 
Finley

Re: [PATCH v1 3/3] PM / AVS: rockchip-cpu-avs: add driver handling Rockchip cpu avs

[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.

Re: [PATCH v1 3/3] PM / AVS: rockchip-cpu-avs: add driver handling Rockchip cpu avs

[Finley Xiao]

在 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.

>
>
>
>
>

-- 
Finley

Re: [PATCH v1 3/3] PM / AVS: rockchip-cpu-avs: add driver handling Rockchip cpu avs

[Rob Herring]

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