Message-ID: <5c9dab8d-99d3-f50d-22ab-9547e07fec05@rock-chips.com>
Date:	Wed, 17 Aug 2016 21:59:44 +0800
From:	Finley Xiao <finley.xiao@...k-chips.com>
To:	Heiko Stübner <heiko@...ech.de>
Cc:	srinivas.kandagatla@...aro.org, maxime.ripard@...e-electrons.com,
	robh+dt@...nel.org, frowand.list@...il.com, sre@...nel.org,
	dbaryshkov@...il.com, dwmw2@...radead.org, mark.rutland@....com,
	khilman@...nel.org, nm@...com, rjw@...ysocki.net,
	viresh.kumar@...aro.org, sboyd@...eaurora.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-rockchip@...ts.infradead.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
	wxt@...k-chips.com, jay.xu@...k-chips.com,
	rocky.hao@...k-chips.com, tim.chen@...k-chips.com,
	tony.xie@...k-chips.com, ulysses.huang@...k-chips.com,
	lin.huang@...k-chips.com
Subject: Re: [PATCH v1 3/3] PM / AVS: rockchip-cpu-avs: add driver handling
 Rockchip cpu avs



在 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@...k-chips.com>
>>
>> This patch supports adjusting opp's voltage according to leakage
>>
>> Signed-off-by: Finley Xiao <finley.xiao@...k-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@...k-chips.com>");
>> +MODULE_LICENSE("GPL v2");
>
>
>

-- 
Finley

Powered by blists - more mailing lists