linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/2] thermal: mediatek: Add cpu power cooling model
@ 2015-10-22 12:02 Dawei Chien
  2015-10-22 12:02 ` [PATCH v3 1/2] " Dawei Chien
                   ` (2 more replies)
  0 siblings, 3 replies; 23+ messages in thread
From: Dawei Chien @ 2015-10-22 12:02 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rafael J. Wysocki, Rob Herring, Pawel Moll, Mark Rutland,
	Ian Campbell, Kumar Gala, Matthias Brugger, Daniel Kurtz,
	Sascha Hauer, Daniel Lezcano, Dawei Chien, devicetree,
	linux-arm-kernel, linux-kernel, linux-pm, linux-mediatek,
	srv_heupstream, Sascha Hauer

Use Intelligent Power Allocation (IPA) technical to add static/dynamic power model for binding CPU thermal zone.
The power allocator governor allocates power budget to control CPU temperature.

Power Allocator governor is able to keep SOC temperature within a defined temperature range to avoid SOC overheat and keep it's performance. mt8173-cpufreq.c need to register its' own power model with power allocator thermal governor, so that power allocator governor can allocates suitable power budget to control CPU temperature.

PATCH1 is base on
https://patchwork.kernel.org/patch/7034601/

PATCH2 is base on Sascha's thermal driver V9
https://patchwork.kernel.org/patch/7249821/
https://patchwork.kernel.org/patch/7249861/
https://patchwork.kernel.org/patch/7249891/

Change since V1:
include mt8171.h and sort header file for mt8173.dtsi

Change since V2:
Move dynamic/static power model in device tree

Dawei.Chien (2):
  thermal: mediatek: Add cpu power cooling model.
  arm64: dts: mt8173: Add thermal zone node for mt8173.

 arch/arm64/boot/dts/mediatek/mt8173.dtsi |   44 ++++++++++++++
 drivers/cpufreq/mt8173-cpufreq.c         |   97 ++++++++++++++++++++++++++----
 2 files changed, 130 insertions(+), 11 deletions(-)

--
1.7.9.5

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v3 1/2] thermal: mediatek: Add cpu power cooling model.
  2015-10-22 12:02 [PATCH v3 0/2] thermal: mediatek: Add cpu power cooling model Dawei Chien
@ 2015-10-22 12:02 ` Dawei Chien
  2015-11-04 19:40   ` Eduardo Valentin
  2015-10-22 12:02 ` [PATCH v3 2/2] arm64: dts: mt8173: Add thermal zone node for mt8173 Dawei Chien
  2015-10-28 15:44 ` [PATCH v3 0/2] thermal: mediatek: Add cpu power cooling model Viresh Kumar
  2 siblings, 1 reply; 23+ messages in thread
From: Dawei Chien @ 2015-10-22 12:02 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rafael J. Wysocki, Rob Herring, Pawel Moll, Mark Rutland,
	Ian Campbell, Kumar Gala, Matthias Brugger, Daniel Kurtz,
	Sascha Hauer, Daniel Lezcano, Dawei Chien, devicetree,
	linux-arm-kernel, linux-kernel, linux-pm, linux-mediatek,
	srv_heupstream, Sascha Hauer

This power model is base on Intelligent Power Allocation (IPA)
technical, requires that the operating-points of the CPUs are
registered using the kernel's opp library and the
`cpufreq_frequency_table` is assigned to the `struct device`
of the cpu MT8173.

Signed-off-by: Dawei.Chien <dawei.chien@mediatek.com>
---
This patch is base on
https://patchwork.kernel.org/patch/7034601/
---
 drivers/cpufreq/mt8173-cpufreq.c |  152 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 144 insertions(+), 8 deletions(-)

diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
index 49caed2..23c19c5 100644
--- a/drivers/cpufreq/mt8173-cpufreq.c
+++ b/drivers/cpufreq/mt8173-cpufreq.c
@@ -29,6 +29,16 @@
 #define MAX_VOLT_LIMIT		(1150000)
 #define VOLT_TOL		(10000)
 
+struct mtk_cpu_static_power {
+	unsigned long voltage;
+	unsigned int power;
+};
+
+static struct mtk_cpu_static_power *mtk_ca53_static_power_table;
+static struct mtk_cpu_static_power *mtk_ca57_static_power_table;
+static int mtk_ca53_static_table_length;
+static int mtk_ca57_static_table_length;
+
 /*
  * The struct mtk_cpu_dvfs_info holds necessary information for doing CPU DVFS
  * on each CPU power/clock domain of Mediatek SoCs. Each CPU cluster in
@@ -51,6 +61,110 @@ struct mtk_cpu_dvfs_info {
 	bool need_voltage_tracking;
 };
 
+unsigned int mtk_cpufreq_lookup_power(const struct mtk_cpu_static_power *table,
+		unsigned int count, unsigned long voltage)
+{
+	int i;
+
+	for (i = 0; i < count; i++) {
+		if (voltage <= table[i].voltage)
+			return table[i].power;
+	}
+
+	return table[count - 1].power;
+}
+
+int mtk_cpufreq_get_static(cpumask_t *cpumask, int interval,
+		unsigned long voltage, u32 *power)
+{
+	int nr_cpus = cpumask_weight(cpumask);
+
+	*power = 0;
+
+	if (nr_cpus) {
+		if (cpumask_test_cpu(0, cpumask))
+			*power += mtk_cpufreq_lookup_power(
+					mtk_ca53_static_power_table,
+					mtk_ca53_static_table_length,
+					voltage);
+
+		if (cpumask_test_cpu(2, cpumask))
+			*power += mtk_cpufreq_lookup_power(
+					mtk_ca57_static_power_table,
+					mtk_ca57_static_table_length,
+					voltage);
+	}
+
+	return 0;
+}
+
+unsigned int mtk_get_power_table_info(struct cpufreq_policy *policy,
+		struct device_node *np, const char *node_name)
+{
+	int mtk_static_table_length;
+	const struct property *prop;
+	struct mtk_cpu_dvfs_info *info = policy->driver_data;
+	struct device *cpu_dev = info->cpu_dev;
+	const __be32 *val;
+	int nr, i;
+
+	prop = of_find_property(np, node_name, NULL);
+
+	if (!prop) {
+		pr_err("failed to get static-power-points\n");
+		return -ENODEV;
+	}
+
+	if (!prop->value) {
+		pr_err("failed to get static power array data\n");
+		return -EINVAL;
+	}
+
+	nr = prop->length / sizeof(u32);
+
+	if (nr % 2) {
+		pr_err("Invalid OPP list\n");
+		return -EINVAL;
+	}
+
+	mtk_static_table_length = nr / 2;
+
+	if (cpumask_test_cpu(0, policy->related_cpus)) {
+		mtk_ca53_static_table_length = mtk_static_table_length;
+		mtk_ca53_static_power_table = devm_kcalloc(cpu_dev,
+					mtk_static_table_length,
+					sizeof(*mtk_ca53_static_power_table),
+					GFP_KERNEL);
+
+		val = prop->value;
+		for (i = 0; i < mtk_static_table_length; ++i) {
+			unsigned long voltage = be32_to_cpup(val++);
+			unsigned int power = be32_to_cpup(val++);
+
+			mtk_ca53_static_power_table[i].voltage = voltage;
+			mtk_ca53_static_power_table[i].power = power;
+			pr_info("volt:%ld uv, power:%d mW\n", voltage, power);
+		}
+	} else {
+                mtk_ca57_static_table_length = mtk_static_table_length;
+		mtk_ca57_static_power_table = devm_kcalloc(cpu_dev,
+					mtk_static_table_length,
+					sizeof(*mtk_ca57_static_power_table),
+					GFP_KERNEL);
+		val = prop->value;
+		for (i = 0; i < mtk_static_table_length; ++i) {
+			unsigned long voltage = be32_to_cpup(val++);
+			unsigned int power = be32_to_cpup(val++);
+
+			mtk_ca57_static_power_table[i].voltage = voltage;
+			mtk_ca57_static_power_table[i].power = power;
+			pr_info("volt:%ld uv, power:%d mW\n", voltage, power);
+		}
+	}
+
+	return	0;
+}
+
 static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
 					int new_vproc)
 {
@@ -267,20 +381,40 @@ static void mtk_cpufreq_ready(struct cpufreq_policy *policy)
 {
 	struct mtk_cpu_dvfs_info *info = policy->driver_data;
 	struct device_node *np = of_node_get(info->cpu_dev->of_node);
+	u32 capacitance;
+	int ret;
 
 	if (WARN_ON(!np))
 		return;
 
 	if (of_find_property(np, "#cooling-cells", NULL)) {
-		info->cdev = of_cpufreq_cooling_register(np,
-							 policy->related_cpus);
 
-		if (IS_ERR(info->cdev)) {
-			dev_err(info->cpu_dev,
-				"running cpufreq without cooling device: %ld\n",
-				PTR_ERR(info->cdev));
+		if (!info->cdev) {
+
+			of_property_read_u32(np,
+					"dynamic-power-coefficient",
+					&capacitance);
+
+			ret = mtk_get_power_table_info(policy, np,
+						"static-power-points");
+			if (ret) {
+                                dev_err(info->cpu_dev,
+                                        "cpufreq without static-points: %d\n",
+                                        ret);
+			}
+
+			info->cdev = of_cpufreq_power_cooling_register(np,
+				policy->related_cpus,
+				capacitance,
+				mtk_cpufreq_get_static);
+
+			if (IS_ERR(info->cdev)) {
+				dev_err(info->cpu_dev,
+					"cpufreq without cdev: %ld\n",
+					PTR_ERR(info->cdev));
+					info->cdev = NULL;
+			}
 
-			info->cdev = NULL;
 		}
 	}
 
@@ -460,7 +594,9 @@ static int mtk_cpufreq_exit(struct cpufreq_policy *policy)
 {
 	struct mtk_cpu_dvfs_info *info = policy->driver_data;
 
-	cpufreq_cooling_unregister(info->cdev);
+	if (info->cdev)
+		cpufreq_cooling_unregister(info->cdev);
+
 	dev_pm_opp_free_cpufreq_table(info->cpu_dev, &policy->freq_table);
 	mtk_cpu_dvfs_info_release(info);
 	kfree(info);
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v3 2/2] arm64: dts: mt8173: Add thermal zone node for mt8173.
  2015-10-22 12:02 [PATCH v3 0/2] thermal: mediatek: Add cpu power cooling model Dawei Chien
  2015-10-22 12:02 ` [PATCH v3 1/2] " Dawei Chien
@ 2015-10-22 12:02 ` Dawei Chien
  2015-10-28 15:39   ` Viresh Kumar
  2015-11-04 19:41   ` Eduardo Valentin
  2015-10-28 15:44 ` [PATCH v3 0/2] thermal: mediatek: Add cpu power cooling model Viresh Kumar
  2 siblings, 2 replies; 23+ messages in thread
From: Dawei Chien @ 2015-10-22 12:02 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rafael J. Wysocki, Rob Herring, Pawel Moll, Mark Rutland,
	Ian Campbell, Kumar Gala, Matthias Brugger, Daniel Kurtz,
	Sascha Hauer, Daniel Lezcano, Dawei Chien, devicetree,
	linux-arm-kernel, linux-kernel, linux-pm, linux-mediatek,
	srv_heupstream, Sascha Hauer

Add thermal zone node to mt8173.dtsi.

Signed-off-by: Dawei Chien <dawei.chien@mediatek.com>
---
This patch is base on
https://patchwork.kernel.org/patch/7249821/
https://patchwork.kernel.org/patch/7249861/
https://patchwork.kernel.org/patch/7249891/
---
 arch/arm64/boot/dts/mediatek/mt8173.dtsi |   90 ++++++++++++++++++++++++++++++
 1 file changed, 90 insertions(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
index 3b18f37..eaf12bf 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
@@ -16,6 +16,7 @@
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/power/mt8173-power.h>
 #include <dt-bindings/reset-controller/mt8173-resets.h>
+#include <dt-bindings/thermal/mt8173.h>
 #include "mt8173-pinfunc.h"
 
 / {
@@ -54,6 +55,18 @@
 			reg = <0x000>;
 			enable-method = "psci";
 			cpu-idle-states = <&CPU_SLEEP_0>;
+			static-power-points = <
+				859000  43
+				908000  52
+				983000  86
+				1009000 123
+				1028000 138
+				1083000 172
+				1110900 180
+				1125000 192
+			>;
+			dynamic-power-coefficient = <263>;
+			#cooling-cells = <2>;
 		};
 
 		cpu1: cpu@1 {
@@ -62,6 +75,17 @@
 			reg = <0x001>;
 			enable-method = "psci";
 			cpu-idle-states = <&CPU_SLEEP_0>;
+			static-power-points = <
+				859000  43
+				908000  52
+				983000  86
+				1009000 123
+				1028000 138
+				1083000 172
+				1110900 180
+				1125000 192
+			>;
+			dynamic-power-coefficient = <263>;
 		};
 
 		cpu2: cpu@100 {
@@ -70,6 +94,18 @@
 			reg = <0x100>;
 			enable-method = "psci";
 			cpu-idle-states = <&CPU_SLEEP_0>;
+			static-power-points = <
+				828000  72
+				867000  90
+				927000  156
+				968000  181
+				1007000 298
+				1049000 435
+				1089900 533
+				1125000 533
+			>;
+			dynamic-power-coefficient = <530>;
+			#cooling-cells = <2>;
 		};
 
 		cpu3: cpu@101 {
@@ -78,6 +114,17 @@
 			reg = <0x101>;
 			enable-method = "psci";
 			cpu-idle-states = <&CPU_SLEEP_0>;
+			static-power-points = <
+				828000  72
+				867000  90
+				927000  156
+				968000  181
+				1007000 298
+				1049000 435
+				1089900 533
+				1125000 533
+			>;
+			dynamic-power-coefficient = <530>;
 		};
 
 		idle-states {
@@ -116,6 +163,49 @@
 		clock-output-names = "clk32k";
 	};
 
+	thermal-zones {
+		cpu_thermal: cpu_thermal {
+			polling-delay-passive = <1000>; /* milliseconds */
+			polling-delay = <1000>; /* milliseconds */
+
+			thermal-sensors = <&thermal MT8173_THERMAL_ZONE_CA57>;
+			sustainable-power = <1500>;
+
+			trips {
+				threshold: trip-point@0 {
+					temperature = <68000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+
+				target: trip-point@1 {
+					temperature = <85000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+
+				cpu_crit: cpu_crit@0 {
+					temperature = <115000>;
+					hysteresis = <2000>;
+					type = "critical";
+				};
+			};
+
+			cooling-maps {
+				map@0 {
+					trip = <&target>;
+					cooling-device = <&cpu0 0 0>;
+					contribution = <1024>;
+				};
+				map@1 {
+					trip = <&target>;
+					cooling-device = <&cpu2 0 0>;
+					contribution = <2048>;
+				};
+			};
+		};
+	};
+
 	timer {
 		compatible = "arm,armv8-timer";
 		interrupt-parent = <&gic>;
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 2/2] arm64: dts: mt8173: Add thermal zone node for mt8173.
  2015-10-22 12:02 ` [PATCH v3 2/2] arm64: dts: mt8173: Add thermal zone node for mt8173 Dawei Chien
@ 2015-10-28 15:39   ` Viresh Kumar
  2015-11-02 10:51     ` dawei chien
  2015-11-04 19:41   ` Eduardo Valentin
  1 sibling, 1 reply; 23+ messages in thread
From: Viresh Kumar @ 2015-10-28 15:39 UTC (permalink / raw)
  To: Dawei Chien
  Cc: Rafael J. Wysocki, Rob Herring, Pawel Moll, Mark Rutland,
	Ian Campbell, Kumar Gala, Matthias Brugger, Daniel Kurtz,
	Sascha Hauer, Daniel Lezcano, devicetree, linux-arm-kernel,
	linux-kernel, linux-pm, linux-mediatek, srv_heupstream,
	Sascha Hauer

On 22-10-15, 20:02, Dawei Chien wrote:
> Add thermal zone node to mt8173.dtsi.
> 
> Signed-off-by: Dawei Chien <dawei.chien@mediatek.com>
> ---
> This patch is base on
> https://patchwork.kernel.org/patch/7249821/
> https://patchwork.kernel.org/patch/7249861/
> https://patchwork.kernel.org/patch/7249891/
> ---
>  arch/arm64/boot/dts/mediatek/mt8173.dtsi |   90 ++++++++++++++++++++++++++++++
>  1 file changed, 90 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> index 3b18f37..eaf12bf 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> @@ -16,6 +16,7 @@
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>  #include <dt-bindings/power/mt8173-power.h>
>  #include <dt-bindings/reset-controller/mt8173-resets.h>
> +#include <dt-bindings/thermal/mt8173.h>
>  #include "mt8173-pinfunc.h"
>  
>  / {
> @@ -54,6 +55,18 @@
>  			reg = <0x000>;
>  			enable-method = "psci";
>  			cpu-idle-states = <&CPU_SLEEP_0>;
> +			static-power-points = <
> +				859000  43
> +				908000  52
> +				983000  86
> +				1009000 123
> +				1028000 138
> +				1083000 172
> +				1110900 180
> +				1125000 192

What's the unit of power here? Is this power accurate? Or just a
number representing the power ?

> +			>;
> +			dynamic-power-coefficient = <263>;
> +			#cooling-cells = <2>;
>  		};

@Rob: Looks like another good candidate for the OPP-v2 table? Power.

-- 
viresh

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 0/2] thermal: mediatek: Add cpu power cooling model
  2015-10-22 12:02 [PATCH v3 0/2] thermal: mediatek: Add cpu power cooling model Dawei Chien
  2015-10-22 12:02 ` [PATCH v3 1/2] " Dawei Chien
  2015-10-22 12:02 ` [PATCH v3 2/2] arm64: dts: mt8173: Add thermal zone node for mt8173 Dawei Chien
@ 2015-10-28 15:44 ` Viresh Kumar
  2015-11-02 10:46   ` dawei chien
  2015-11-02 15:53   ` Punit Agrawal
  2 siblings, 2 replies; 23+ messages in thread
From: Viresh Kumar @ 2015-10-28 15:44 UTC (permalink / raw)
  To: Dawei Chien
  Cc: Rafael J. Wysocki, Rob Herring, Pawel Moll, Mark Rutland,
	Ian Campbell, Kumar Gala, Matthias Brugger, Daniel Kurtz,
	Sascha Hauer, Daniel Lezcano, devicetree, linux-arm-kernel,
	linux-kernel, linux-pm, linux-mediatek, srv_heupstream,
	Sascha Hauer

On 22-10-15, 20:02, Dawei Chien wrote:
> Use Intelligent Power Allocation (IPA) technical to add static/dynamic power model for binding CPU thermal zone.
> The power allocator governor allocates power budget to control CPU temperature.
> 
> Power Allocator governor is able to keep SOC temperature within a defined temperature range to avoid SOC overheat and keep it's performance. mt8173-cpufreq.c need to register its' own power model with power allocator thermal governor, so that power allocator governor can allocates suitable power budget to control CPU temperature.
> 
> PATCH1 is base on
> https://patchwork.kernel.org/patch/7034601/
> 
> PATCH2 is base on Sascha's thermal driver V9
> https://patchwork.kernel.org/patch/7249821/
> https://patchwork.kernel.org/patch/7249861/
> https://patchwork.kernel.org/patch/7249891/
> 
> Change since V1:
> include mt8171.h and sort header file for mt8173.dtsi
> 
> Change since V2:
> Move dynamic/static power model in device tree
> 
> Dawei.Chien (2):
>   thermal: mediatek: Add cpu power cooling model.
>   arm64: dts: mt8173: Add thermal zone node for mt8173.

Sorry for being extremely late in reviewing this stuff. You are
already on v3 and I haven't reviewed it once. Mostly due to bad timing
of my holidays and other work pressure.

Now, there are few things that I feel are not properly addressed here,
and I may be wrong:
- Where are the bindings for static-power-points and
  dynamic-power-coefficient. Sorry I failed to see them in this or
  other series you mentioned.
- Even then, why should we be adding another table into DT for
  voltage/power ? And not reuse and extend the opp-v2 stuff which is
  already mainlined now.
- There are few issues with the code as well, but I want to see where
  the bindings should go first. And then only discuss the code
  further.

-- 
viresh

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 0/2] thermal: mediatek: Add cpu power cooling model
  2015-10-28 15:44 ` [PATCH v3 0/2] thermal: mediatek: Add cpu power cooling model Viresh Kumar
@ 2015-11-02 10:46   ` dawei chien
  2015-11-02 12:10     ` Viresh Kumar
  2015-11-02 15:53   ` Punit Agrawal
  1 sibling, 1 reply; 23+ messages in thread
From: dawei chien @ 2015-11-02 10:46 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rafael J. Wysocki, Rob Herring, Pawel Moll, Mark Rutland,
	Ian Campbell, Kumar Gala, Matthias Brugger, Daniel Kurtz,
	Sascha Hauer, Daniel Lezcano, devicetree, linux-arm-kernel,
	linux-kernel, linux-pm, linux-mediatek, srv_heupstream,
	Sascha Hauer

Hi Viresh,
On Wed, 2015-10-28 at 21:14 +0530, Viresh Kumar wrote:
> On 22-10-15, 20:02, Dawei Chien wrote:
> > Use Intelligent Power Allocation (IPA) technical to add static/dynamic power model for binding CPU thermal zone.
> > The power allocator governor allocates power budget to control CPU temperature.
> > 
> > Power Allocator governor is able to keep SOC temperature within a defined temperature range to avoid SOC overheat and keep it's performance. mt8173-cpufreq.c need to register its' own power model with power allocator thermal governor, so that power allocator governor can allocates suitable power budget to control CPU temperature.
> > 
> > PATCH1 is base on
> > https://patchwork.kernel.org/patch/7034601/
> > 
> > PATCH2 is base on Sascha's thermal driver V9
> > https://patchwork.kernel.org/patch/7249821/
> > https://patchwork.kernel.org/patch/7249861/
> > https://patchwork.kernel.org/patch/7249891/
> > 
> > Change since V1:
> > include mt8171.h and sort header file for mt8173.dtsi
> > 
> > Change since V2:
> > Move dynamic/static power model in device tree
> > 
> > Dawei.Chien (2):
> >   thermal: mediatek: Add cpu power cooling model.
> >   arm64: dts: mt8173: Add thermal zone node for mt8173.
> 
> Sorry for being extremely late in reviewing this stuff. You are
> already on v3 and I haven't reviewed it once. Mostly due to bad timing
> of my holidays and other work pressure.

You're welcome, truly thank you for your kindly reviewing

> Now, there are few things that I feel are not properly addressed here,
> and I may be wrong:
> - Where are the bindings for static-power-points and
>   dynamic-power-coefficient. Sorry I failed to see them in this or
>   other series you mentioned.

Please refer to following document (2-1,2-2) for dynamic-power &
static-power in detail. Besides, do I need to add another document for
our own MT8173 IC.
http://lxr.free-electrons.com/source/Documentation/thermal/cpu-cooling-api.txt

> - Even then, why should we be adding another table into DT for
>   voltage/power ? And not reuse and extend the opp-v2 stuff which is
>   already mainlined now.

We could reuse opp-v2 for static power points after OPPV2 back port to
our currently branch.
However, as far as I know, there is no "power" in opp.c (suck like
opp-hz) as far, so I need to add something in opp.c for my purpose, suck
like add power in _opp_add_static_v2, and add something for return
"power", right? I may be wrong, please kindly give me your suggestion,
thank you.

Actually, I am considering to remove the part of static power point
since it is optional for Power Model. Could you agree with this?
> - There are few issues with the code as well, but I want to see where
>   the bindings should go first. And then only discuss the code
>   further.
> 



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 2/2] arm64: dts: mt8173: Add thermal zone node for mt8173.
  2015-10-28 15:39   ` Viresh Kumar
@ 2015-11-02 10:51     ` dawei chien
  0 siblings, 0 replies; 23+ messages in thread
From: dawei chien @ 2015-11-02 10:51 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rafael J. Wysocki, Rob Herring, Pawel Moll, Mark Rutland,
	Ian Campbell, Kumar Gala, Matthias Brugger, Daniel Kurtz,
	Sascha Hauer, Daniel Lezcano, devicetree, linux-arm-kernel,
	linux-kernel, linux-pm, linux-mediatek, srv_heupstream,
	Sascha Hauer

On Wed, 2015-10-28 at 21:09 +0530, Viresh Kumar wrote:
> On 22-10-15, 20:02, Dawei Chien wrote:
> > Add thermal zone node to mt8173.dtsi.
> > 
> > Signed-off-by: Dawei Chien <dawei.chien@mediatek.com>
> > ---
> > This patch is base on
> > https://patchwork.kernel.org/patch/7249821/
> > https://patchwork.kernel.org/patch/7249861/
> > https://patchwork.kernel.org/patch/7249891/
> > ---
> >  arch/arm64/boot/dts/mediatek/mt8173.dtsi |   90 ++++++++++++++++++++++++++++++
> >  1 file changed, 90 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > index 3b18f37..eaf12bf 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > @@ -16,6 +16,7 @@
> >  #include <dt-bindings/interrupt-controller/arm-gic.h>
> >  #include <dt-bindings/power/mt8173-power.h>
> >  #include <dt-bindings/reset-controller/mt8173-resets.h>
> > +#include <dt-bindings/thermal/mt8173.h>
> >  #include "mt8173-pinfunc.h"
> >  
> >  / {
> > @@ -54,6 +55,18 @@
> >  			reg = <0x000>;
> >  			enable-method = "psci";
> >  			cpu-idle-states = <&CPU_SLEEP_0>;
> > +			static-power-points = <
> > +				859000  43
> > +				908000  52
> > +				983000  86
> > +				1009000 123
> > +				1028000 138
> > +				1083000 172
> > +				1110900 180
> > +				1125000 192
> 
> What's the unit of power here? Is this power accurate? Or just a
> number representing the power ?

The unit is mW. This power was measured by tools, but it may be not
accurate enough since this tool has a error range.

> > +			>;
> > +			dynamic-power-coefficient = <263>;
> > +			#cooling-cells = <2>;
> >  		};
> 
> @Rob: Looks like another good candidate for the OPP-v2 table? Power.
> 



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 0/2] thermal: mediatek: Add cpu power cooling model
  2015-11-02 10:46   ` dawei chien
@ 2015-11-02 12:10     ` Viresh Kumar
  2015-11-05 11:09       ` dawei chien
  0 siblings, 1 reply; 23+ messages in thread
From: Viresh Kumar @ 2015-11-02 12:10 UTC (permalink / raw)
  To: dawei chien
  Cc: Rafael J. Wysocki, Rob Herring, Pawel Moll, Mark Rutland,
	Ian Campbell, Kumar Gala, Matthias Brugger, Daniel Kurtz,
	Sascha Hauer, Daniel Lezcano, devicetree, linux-arm-kernel,
	linux-kernel, linux-pm, linux-mediatek, srv_heupstream,
	Sascha Hauer

On 02-11-15, 18:46, dawei chien wrote:
> On Wed, 2015-10-28 at 21:14 +0530, Viresh Kumar wrote:
> > Sorry for being extremely late in reviewing this stuff. You are
> > already on v3 and I haven't reviewed it once. Mostly due to bad timing
> > of my holidays and other work pressure.
> 
> You're welcome, truly thank you for your kindly reviewing

Thanks for understanding.

> > Now, there are few things that I feel are not properly addressed here,
> > and I may be wrong:
> > - Where are the bindings for static-power-points and
> >   dynamic-power-coefficient. Sorry I failed to see them in this or
> >   other series you mentioned.
> 
> Please refer to following document (2-1,2-2) for dynamic-power &
> static-power in detail. Besides, do I need to add another document for
> our own MT8173 IC.
> http://lxr.free-electrons.com/source/Documentation/thermal/cpu-cooling-api.txt

That's about the power-API, but I am talking about the Device Tree
bindings here. So, when you add any new DT bindings (Or a new property
in device tree blobs), you need to add its documentation in
Documentation/devicetree/bindings/... and get it approved by DT
maintainers as well. You perhaps missed that completely, otherwise you
would have been told really early that the new bindings aren't going
to help.

> > - Even then, why should we be adding another table into DT for
> >   voltage/power ? And not reuse and extend the opp-v2 stuff which is
> >   already mainlined now.
> 
> We could reuse opp-v2 for static power points after OPPV2 back port to
> our currently branch.

Your current branch doesn't matter to us. All that matters here is
mainline, that's where you are adding code to. And you must test your
stuff on the latest upstream branch only, not on some old kernel
release. You can include other dependency patches though, that are
required to make it work and mention them in cover-letter.

> However, as far as I know, there is no "power" in opp.c (suck like

s/suck/such ?

> opp-hz) as far, so I need to add something in opp.c for my purpose, suck
> like add power in _opp_add_static_v2, and add something for return
> "power", right? I may be wrong, please kindly give me your suggestion,
> thank you.

You first need to propose a change in DT bindings for OPPs:
Documentation/devicetree/bindings/opp/opp.txt

And then we can change the code properly.

> Actually, I am considering to remove the part of static power point
> since it is optional for Power Model. Could you agree with this?

If its not important for your platform, then I don't have any issues
with that..

-- 
viresh

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 0/2] thermal: mediatek: Add cpu power cooling model
  2015-10-28 15:44 ` [PATCH v3 0/2] thermal: mediatek: Add cpu power cooling model Viresh Kumar
  2015-11-02 10:46   ` dawei chien
@ 2015-11-02 15:53   ` Punit Agrawal
  2015-11-02 16:10     ` Viresh Kumar
  2015-11-04 19:36     ` Eduardo Valentin
  1 sibling, 2 replies; 23+ messages in thread
From: Punit Agrawal @ 2015-11-02 15:53 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Dawei Chien, Rafael J. Wysocki, Rob Herring, Pawel Moll,
	Mark Rutland, Ian Campbell, Kumar Gala, Matthias Brugger,
	Daniel Kurtz, Sascha Hauer, Daniel Lezcano, devicetree,
	linux-arm-kernel, linux-kernel, linux-pm, linux-mediatek,
	srv_heupstream, Sascha Hauer

Viresh Kumar <viresh.kumar@linaro.org> writes:

> On 22-10-15, 20:02, Dawei Chien wrote:
>> Use Intelligent Power Allocation (IPA) technical to add static/dynamic power model for binding CPU thermal zone.
>> The power allocator governor allocates power budget to control CPU temperature.
>> 
>> Power Allocator governor is able to keep SOC temperature within a
>> defined temperature range to avoid SOC overheat and keep it's
>> performance. mt8173-cpufreq.c need to register its' own power model
>> with power allocator thermal governor, so that power allocator
>> governor can allocates suitable power budget to control CPU
>> temperature.
>> 
>> PATCH1 is base on
>> https://patchwork.kernel.org/patch/7034601/
>> 
>> PATCH2 is base on Sascha's thermal driver V9
>> https://patchwork.kernel.org/patch/7249821/
>> https://patchwork.kernel.org/patch/7249861/
>> https://patchwork.kernel.org/patch/7249891/
>> 
>> Change since V1:
>> include mt8171.h and sort header file for mt8173.dtsi
>> 
>> Change since V2:
>> Move dynamic/static power model in device tree
>> 
>> Dawei.Chien (2):
>>   thermal: mediatek: Add cpu power cooling model.
>>   arm64: dts: mt8173: Add thermal zone node for mt8173.
>
> Sorry for being extremely late in reviewing this stuff. You are
> already on v3 and I haven't reviewed it once. Mostly due to bad timing
> of my holidays and other work pressure.
>
> Now, there are few things that I feel are not properly addressed here,
> and I may be wrong:
> - Where are the bindings for static-power-points and
>   dynamic-power-coefficient. Sorry I failed to see them in this or
>   other series you mentioned.

For dynamic power, I had posted some patches[0][1][2] introducing the
binding as well as updating cooling device registration via cpufreq
driver. Now that the SCPI hwmon driver is merged, I should re-send the
remaining patches.

[0] http://lkml.iu.edu/hypermail/linux/kernel/1508.0/01020.html
[1] http://lkml.iu.edu/hypermail/linux/kernel/1508.0/01022.html
[3] http://lkml.iu.edu/hypermail/linux/kernel/1508.0/01031.html

> - Even then, why should we be adding another table into DT for
>   voltage/power ? And not reuse and extend the opp-v2 stuff which is
>   already mainlined now.
> - There are few issues with the code as well, but I want to see where
>   the bindings should go first. And then only discuss the code
>   further.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 0/2] thermal: mediatek: Add cpu power cooling model
  2015-11-02 15:53   ` Punit Agrawal
@ 2015-11-02 16:10     ` Viresh Kumar
  2015-11-04 19:36     ` Eduardo Valentin
  1 sibling, 0 replies; 23+ messages in thread
From: Viresh Kumar @ 2015-11-02 16:10 UTC (permalink / raw)
  To: Punit Agrawal
  Cc: Dawei Chien, Rafael J. Wysocki, Rob Herring, Pawel Moll,
	Mark Rutland, Ian Campbell, Kumar Gala, Matthias Brugger,
	Daniel Kurtz, Sascha Hauer, Daniel Lezcano, devicetree,
	linux-arm-kernel, linux-kernel, linux-pm, linux-mediatek,
	srv_heupstream, Sascha Hauer

On 02-11-15, 15:53, Punit Agrawal wrote:
> For dynamic power, I had posted some patches[0][1][2] introducing the
> binding as well as updating cooling device registration via cpufreq
> driver. Now that the SCPI hwmon driver is merged, I should re-send the
> remaining patches.
> 
> [0] http://lkml.iu.edu/hypermail/linux/kernel/1508.0/01020.html
> [1] http://lkml.iu.edu/hypermail/linux/kernel/1508.0/01022.html
> [3] http://lkml.iu.edu/hypermail/linux/kernel/1508.0/01031.html

Sure. Just that whatever can be merged with opp-v2 should be merged.

-- 
viresh

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 0/2] thermal: mediatek: Add cpu power cooling model
  2015-11-02 15:53   ` Punit Agrawal
  2015-11-02 16:10     ` Viresh Kumar
@ 2015-11-04 19:36     ` Eduardo Valentin
  1 sibling, 0 replies; 23+ messages in thread
From: Eduardo Valentin @ 2015-11-04 19:36 UTC (permalink / raw)
  To: Punit Agrawal
  Cc: Viresh Kumar, Dawei Chien, Rafael J. Wysocki, Rob Herring,
	Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
	Matthias Brugger, Daniel Kurtz, Sascha Hauer, Daniel Lezcano,
	devicetree, linux-arm-kernel, linux-kernel, linux-pm,
	linux-mediatek, srv_heupstream, Sascha Hauer

On Mon, Nov 02, 2015 at 03:53:33PM +0000, Punit Agrawal wrote:
> Viresh Kumar <viresh.kumar@linaro.org> writes:
> 
> > On 22-10-15, 20:02, Dawei Chien wrote:
> >> Use Intelligent Power Allocation (IPA) technical to add static/dynamic power model for binding CPU thermal zone.
> >> The power allocator governor allocates power budget to control CPU temperature.
> >> 
> >> Power Allocator governor is able to keep SOC temperature within a
> >> defined temperature range to avoid SOC overheat and keep it's
> >> performance. mt8173-cpufreq.c need to register its' own power model
> >> with power allocator thermal governor, so that power allocator
> >> governor can allocates suitable power budget to control CPU
> >> temperature.
> >> 
> >> PATCH1 is base on
> >> https://patchwork.kernel.org/patch/7034601/
> >> 
> >> PATCH2 is base on Sascha's thermal driver V9
> >> https://patchwork.kernel.org/patch/7249821/
> >> https://patchwork.kernel.org/patch/7249861/
> >> https://patchwork.kernel.org/patch/7249891/
> >> 
> >> Change since V1:
> >> include mt8171.h and sort header file for mt8173.dtsi
> >> 
> >> Change since V2:
> >> Move dynamic/static power model in device tree
> >> 
> >> Dawei.Chien (2):
> >>   thermal: mediatek: Add cpu power cooling model.
> >>   arm64: dts: mt8173: Add thermal zone node for mt8173.
> >
> > Sorry for being extremely late in reviewing this stuff. You are
> > already on v3 and I haven't reviewed it once. Mostly due to bad timing
> > of my holidays and other work pressure.
> >
> > Now, there are few things that I feel are not properly addressed here,
> > and I may be wrong:
> > - Where are the bindings for static-power-points and
> >   dynamic-power-coefficient. Sorry I failed to see them in this or
> >   other series you mentioned.
> 
> For dynamic power, I had posted some patches[0][1][2] introducing the
> binding as well as updating cooling device registration via cpufreq
> driver. Now that the SCPI hwmon driver is merged, I should re-send the
> remaining patches.
> 
> [0] http://lkml.iu.edu/hypermail/linux/kernel/1508.0/01020.html

Are you sure this binding is applicable only to ARM cpus?

> [1] http://lkml.iu.edu/hypermail/linux/kernel/1508.0/01022.html
> [3] http://lkml.iu.edu/hypermail/linux/kernel/1508.0/01031.html
> 
> > - Even then, why should we be adding another table into DT for
> >   voltage/power ? And not reuse and extend the opp-v2 stuff which is
> >   already mainlined now.
> > - There are few issues with the code as well, but I want to see where
> >   the bindings should go first. And then only discuss the code
> >   further.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 1/2] thermal: mediatek: Add cpu power cooling model.
  2015-10-22 12:02 ` [PATCH v3 1/2] " Dawei Chien
@ 2015-11-04 19:40   ` Eduardo Valentin
  2015-11-05 11:10     ` dawei chien
  0 siblings, 1 reply; 23+ messages in thread
From: Eduardo Valentin @ 2015-11-04 19:40 UTC (permalink / raw)
  To: Dawei Chien
  Cc: Viresh Kumar, Rafael J. Wysocki, Rob Herring, Pawel Moll,
	Mark Rutland, Ian Campbell, Kumar Gala, Matthias Brugger,
	Daniel Kurtz, Sascha Hauer, Daniel Lezcano, devicetree,
	linux-arm-kernel, linux-kernel, linux-pm, linux-mediatek,
	srv_heupstream, Sascha Hauer

On Thu, Oct 22, 2015 at 08:02:38PM +0800, Dawei Chien wrote:
> This power model is base on Intelligent Power Allocation (IPA)
> technical, requires that the operating-points of the CPUs are
> registered using the kernel's opp library and the
> `cpufreq_frequency_table` is assigned to the `struct device`
> of the cpu MT8173.
> 
> Signed-off-by: Dawei.Chien <dawei.chien@mediatek.com>
> ---
> This patch is base on
> https://patchwork.kernel.org/patch/7034601/
> ---
>  drivers/cpufreq/mt8173-cpufreq.c |  152 ++++++++++++++++++++++++++++++++++++--

Given that you are proposing this on top a DT binding, why reading a
table of static power model applicable only to mt8173 cpufreq driver?

I have not seen anything specific (formula etc) that prevents this code
to be generalized to other CPUs.

Can you please help me to understand?

BR,

>  1 file changed, 144 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
> index 49caed2..23c19c5 100644
> --- a/drivers/cpufreq/mt8173-cpufreq.c
> +++ b/drivers/cpufreq/mt8173-cpufreq.c
> @@ -29,6 +29,16 @@
>  #define MAX_VOLT_LIMIT		(1150000)
>  #define VOLT_TOL		(10000)
>  
> +struct mtk_cpu_static_power {
> +	unsigned long voltage;
> +	unsigned int power;
> +};
> +
> +static struct mtk_cpu_static_power *mtk_ca53_static_power_table;
> +static struct mtk_cpu_static_power *mtk_ca57_static_power_table;
> +static int mtk_ca53_static_table_length;
> +static int mtk_ca57_static_table_length;
> +
>  /*
>   * The struct mtk_cpu_dvfs_info holds necessary information for doing CPU DVFS
>   * on each CPU power/clock domain of Mediatek SoCs. Each CPU cluster in
> @@ -51,6 +61,110 @@ struct mtk_cpu_dvfs_info {
>  	bool need_voltage_tracking;
>  };
>  
> +unsigned int mtk_cpufreq_lookup_power(const struct mtk_cpu_static_power *table,
> +		unsigned int count, unsigned long voltage)
> +{
> +	int i;
> +
> +	for (i = 0; i < count; i++) {
> +		if (voltage <= table[i].voltage)
> +			return table[i].power;
> +	}
> +
> +	return table[count - 1].power;
> +}
> +
> +int mtk_cpufreq_get_static(cpumask_t *cpumask, int interval,
> +		unsigned long voltage, u32 *power)
> +{
> +	int nr_cpus = cpumask_weight(cpumask);
> +
> +	*power = 0;
> +
> +	if (nr_cpus) {
> +		if (cpumask_test_cpu(0, cpumask))
> +			*power += mtk_cpufreq_lookup_power(
> +					mtk_ca53_static_power_table,
> +					mtk_ca53_static_table_length,
> +					voltage);
> +
> +		if (cpumask_test_cpu(2, cpumask))
> +			*power += mtk_cpufreq_lookup_power(
> +					mtk_ca57_static_power_table,
> +					mtk_ca57_static_table_length,
> +					voltage);
> +	}
> +
> +	return 0;
> +}
> +
> +unsigned int mtk_get_power_table_info(struct cpufreq_policy *policy,
> +		struct device_node *np, const char *node_name)
> +{
> +	int mtk_static_table_length;
> +	const struct property *prop;
> +	struct mtk_cpu_dvfs_info *info = policy->driver_data;
> +	struct device *cpu_dev = info->cpu_dev;
> +	const __be32 *val;
> +	int nr, i;
> +
> +	prop = of_find_property(np, node_name, NULL);
> +
> +	if (!prop) {
> +		pr_err("failed to get static-power-points\n");
> +		return -ENODEV;
> +	}
> +
> +	if (!prop->value) {
> +		pr_err("failed to get static power array data\n");
> +		return -EINVAL;
> +	}
> +
> +	nr = prop->length / sizeof(u32);
> +
> +	if (nr % 2) {
> +		pr_err("Invalid OPP list\n");
> +		return -EINVAL;
> +	}
> +
> +	mtk_static_table_length = nr / 2;
> +
> +	if (cpumask_test_cpu(0, policy->related_cpus)) {
> +		mtk_ca53_static_table_length = mtk_static_table_length;
> +		mtk_ca53_static_power_table = devm_kcalloc(cpu_dev,
> +					mtk_static_table_length,
> +					sizeof(*mtk_ca53_static_power_table),
> +					GFP_KERNEL);
> +
> +		val = prop->value;
> +		for (i = 0; i < mtk_static_table_length; ++i) {
> +			unsigned long voltage = be32_to_cpup(val++);
> +			unsigned int power = be32_to_cpup(val++);
> +
> +			mtk_ca53_static_power_table[i].voltage = voltage;
> +			mtk_ca53_static_power_table[i].power = power;
> +			pr_info("volt:%ld uv, power:%d mW\n", voltage, power);
> +		}
> +	} else {
> +                mtk_ca57_static_table_length = mtk_static_table_length;
> +		mtk_ca57_static_power_table = devm_kcalloc(cpu_dev,
> +					mtk_static_table_length,
> +					sizeof(*mtk_ca57_static_power_table),
> +					GFP_KERNEL);
> +		val = prop->value;
> +		for (i = 0; i < mtk_static_table_length; ++i) {
> +			unsigned long voltage = be32_to_cpup(val++);
> +			unsigned int power = be32_to_cpup(val++);
> +
> +			mtk_ca57_static_power_table[i].voltage = voltage;
> +			mtk_ca57_static_power_table[i].power = power;
> +			pr_info("volt:%ld uv, power:%d mW\n", voltage, power);
> +		}
> +	}
> +
> +	return	0;
> +}
> +
>  static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
>  					int new_vproc)
>  {
> @@ -267,20 +381,40 @@ static void mtk_cpufreq_ready(struct cpufreq_policy *policy)
>  {
>  	struct mtk_cpu_dvfs_info *info = policy->driver_data;
>  	struct device_node *np = of_node_get(info->cpu_dev->of_node);
> +	u32 capacitance;
> +	int ret;
>  
>  	if (WARN_ON(!np))
>  		return;
>  
>  	if (of_find_property(np, "#cooling-cells", NULL)) {
> -		info->cdev = of_cpufreq_cooling_register(np,
> -							 policy->related_cpus);
>  
> -		if (IS_ERR(info->cdev)) {
> -			dev_err(info->cpu_dev,
> -				"running cpufreq without cooling device: %ld\n",
> -				PTR_ERR(info->cdev));
> +		if (!info->cdev) {
> +
> +			of_property_read_u32(np,
> +					"dynamic-power-coefficient",
> +					&capacitance);
> +
> +			ret = mtk_get_power_table_info(policy, np,
> +						"static-power-points");
> +			if (ret) {
> +                                dev_err(info->cpu_dev,
> +                                        "cpufreq without static-points: %d\n",
> +                                        ret);
> +			}
> +
> +			info->cdev = of_cpufreq_power_cooling_register(np,
> +				policy->related_cpus,
> +				capacitance,
> +				mtk_cpufreq_get_static);
> +
> +			if (IS_ERR(info->cdev)) {
> +				dev_err(info->cpu_dev,
> +					"cpufreq without cdev: %ld\n",
> +					PTR_ERR(info->cdev));
> +					info->cdev = NULL;
> +			}
>  
> -			info->cdev = NULL;
>  		}
>  	}
>  
> @@ -460,7 +594,9 @@ static int mtk_cpufreq_exit(struct cpufreq_policy *policy)
>  {
>  	struct mtk_cpu_dvfs_info *info = policy->driver_data;
>  
> -	cpufreq_cooling_unregister(info->cdev);
> +	if (info->cdev)
> +		cpufreq_cooling_unregister(info->cdev);
> +
>  	dev_pm_opp_free_cpufreq_table(info->cpu_dev, &policy->freq_table);
>  	mtk_cpu_dvfs_info_release(info);
>  	kfree(info);
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 2/2] arm64: dts: mt8173: Add thermal zone node for mt8173.
  2015-10-22 12:02 ` [PATCH v3 2/2] arm64: dts: mt8173: Add thermal zone node for mt8173 Dawei Chien
  2015-10-28 15:39   ` Viresh Kumar
@ 2015-11-04 19:41   ` Eduardo Valentin
  2015-11-05 11:10     ` dawei chien
  1 sibling, 1 reply; 23+ messages in thread
From: Eduardo Valentin @ 2015-11-04 19:41 UTC (permalink / raw)
  To: Dawei Chien
  Cc: Viresh Kumar, Rafael J. Wysocki, Rob Herring, Pawel Moll,
	Mark Rutland, Ian Campbell, Kumar Gala, Matthias Brugger,
	Daniel Kurtz, Sascha Hauer, Daniel Lezcano, devicetree,
	linux-arm-kernel, linux-kernel, linux-pm, linux-mediatek,
	srv_heupstream, Sascha Hauer

On Thu, Oct 22, 2015 at 08:02:39PM +0800, Dawei Chien wrote:
> Add thermal zone node to mt8173.dtsi.
> 
> Signed-off-by: Dawei Chien <dawei.chien@mediatek.com>
> ---
> This patch is base on
> https://patchwork.kernel.org/patch/7249821/
> https://patchwork.kernel.org/patch/7249861/
> https://patchwork.kernel.org/patch/7249891/
> ---
>  arch/arm64/boot/dts/mediatek/mt8173.dtsi |   90 ++++++++++++++++++++++++++++++

This patch adds also the static / dynamic power models too. Can you
please split this patch in two: one for the thermal zones, one for the
models.

>  1 file changed, 90 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> index 3b18f37..eaf12bf 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> @@ -16,6 +16,7 @@
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>  #include <dt-bindings/power/mt8173-power.h>
>  #include <dt-bindings/reset-controller/mt8173-resets.h>
> +#include <dt-bindings/thermal/mt8173.h>
>  #include "mt8173-pinfunc.h"
>  
>  / {
> @@ -54,6 +55,18 @@
>  			reg = <0x000>;
>  			enable-method = "psci";
>  			cpu-idle-states = <&CPU_SLEEP_0>;
> +			static-power-points = <
> +				859000  43
> +				908000  52
> +				983000  86
> +				1009000 123
> +				1028000 138
> +				1083000 172
> +				1110900 180
> +				1125000 192
> +			>;
> +			dynamic-power-coefficient = <263>;
> +			#cooling-cells = <2>;
>  		};
>  
>  		cpu1: cpu@1 {
> @@ -62,6 +75,17 @@
>  			reg = <0x001>;
>  			enable-method = "psci";
>  			cpu-idle-states = <&CPU_SLEEP_0>;
> +			static-power-points = <
> +				859000  43
> +				908000  52
> +				983000  86
> +				1009000 123
> +				1028000 138
> +				1083000 172
> +				1110900 180
> +				1125000 192
> +			>;
> +			dynamic-power-coefficient = <263>;
>  		};
>  
>  		cpu2: cpu@100 {
> @@ -70,6 +94,18 @@
>  			reg = <0x100>;
>  			enable-method = "psci";
>  			cpu-idle-states = <&CPU_SLEEP_0>;
> +			static-power-points = <
> +				828000  72
> +				867000  90
> +				927000  156
> +				968000  181
> +				1007000 298
> +				1049000 435
> +				1089900 533
> +				1125000 533
> +			>;
> +			dynamic-power-coefficient = <530>;
> +			#cooling-cells = <2>;
>  		};
>  
>  		cpu3: cpu@101 {
> @@ -78,6 +114,17 @@
>  			reg = <0x101>;
>  			enable-method = "psci";
>  			cpu-idle-states = <&CPU_SLEEP_0>;
> +			static-power-points = <
> +				828000  72
> +				867000  90
> +				927000  156
> +				968000  181
> +				1007000 298
> +				1049000 435
> +				1089900 533
> +				1125000 533
> +			>;
> +			dynamic-power-coefficient = <530>;
>  		};
>  
>  		idle-states {
> @@ -116,6 +163,49 @@
>  		clock-output-names = "clk32k";
>  	};
>  
> +	thermal-zones {
> +		cpu_thermal: cpu_thermal {
> +			polling-delay-passive = <1000>; /* milliseconds */
> +			polling-delay = <1000>; /* milliseconds */
> +
> +			thermal-sensors = <&thermal MT8173_THERMAL_ZONE_CA57>;
> +			sustainable-power = <1500>;
> +
> +			trips {
> +				threshold: trip-point@0 {
> +					temperature = <68000>;
> +					hysteresis = <2000>;
> +					type = "passive";
> +				};
> +
> +				target: trip-point@1 {
> +					temperature = <85000>;
> +					hysteresis = <2000>;
> +					type = "passive";
> +				};
> +
> +				cpu_crit: cpu_crit@0 {
> +					temperature = <115000>;
> +					hysteresis = <2000>;
> +					type = "critical";
> +				};
> +			};
> +
> +			cooling-maps {
> +				map@0 {
> +					trip = <&target>;
> +					cooling-device = <&cpu0 0 0>;
> +					contribution = <1024>;
> +				};
> +				map@1 {
> +					trip = <&target>;
> +					cooling-device = <&cpu2 0 0>;
> +					contribution = <2048>;
> +				};
> +			};
> +		};
> +	};
> +
>  	timer {
>  		compatible = "arm,armv8-timer";
>  		interrupt-parent = <&gic>;
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 0/2] thermal: mediatek: Add cpu power cooling model
  2015-11-02 12:10     ` Viresh Kumar
@ 2015-11-05 11:09       ` dawei chien
  2015-11-06  3:20         ` Viresh Kumar
  0 siblings, 1 reply; 23+ messages in thread
From: dawei chien @ 2015-11-05 11:09 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rafael J. Wysocki, Rob Herring, Pawel Moll, Mark Rutland,
	Ian Campbell, Kumar Gala, Matthias Brugger, Daniel Kurtz,
	Sascha Hauer, Daniel Lezcano, devicetree, linux-arm-kernel,
	linux-kernel, linux-pm, linux-mediatek, srv_heupstream,
	Sascha Hauer

On Mon, 2015-11-02 at 17:40 +0530, Viresh Kumar wrote:
> On 02-11-15, 18:46, dawei chien wrote:
> > On Wed, 2015-10-28 at 21:14 +0530, Viresh Kumar wrote:
> > > Sorry for being extremely late in reviewing this stuff. You are
> > > already on v3 and I haven't reviewed it once. Mostly due to bad timing
> > > of my holidays and other work pressure.
> > 
> > You're welcome, truly thank you for your kindly reviewing
> 
> Thanks for understanding.
> 
> > > Now, there are few things that I feel are not properly addressed here,
> > > and I may be wrong:
> > > - Where are the bindings for static-power-points and
> > >   dynamic-power-coefficient. Sorry I failed to see them in this or
> > >   other series you mentioned.
> > 
> > Please refer to following document (2-1,2-2) for dynamic-power &
> > static-power in detail. Besides, do I need to add another document for
> > our own MT8173 IC.
> > http://lxr.free-electrons.com/source/Documentation/thermal/cpu-cooling-api.txt
> 
> That's about the power-API, but I am talking about the Device Tree
> bindings here. So, when you add any new DT bindings (Or a new property
> in device tree blobs), you need to add its documentation in
> Documentation/devicetree/bindings/... and get it approved by DT
> maintainers as well. You perhaps missed that completely, otherwise you
> would have been told really early that the new bindings aren't going
> to help.

Thank you for your kindly explaining, now I could understand what I
miss, I will send device tree binding on next version such like
following description.

--- a/Documentation/devicetree/bindings/clock/mt8173-cpu-dvfs.txt
+++ b/Documentation/devicetree/bindings/clock/mt8173-cpu-dvfs.txt
@@ -10,6 +10,17 @@ Required properties:
        Please refer to
Documentation/devicetree/bindings/clk/clock-bindings.txt for
        generic clock consumer properties.
 - proc-supply: Regulator for Vproc of CPU cluster.
+- dynamic-power-coefficient:
+       Usage: optional
+       Value type: <prop-encoded-array>
+       Definition: A u32 value that represents an indicative
+       running time dynamic power coefficient in
+       fundamental units of mW/MHz/uVolt^2.
+       The dynamic energy consumption of the CPU
+       is proportional to the square of the
+       Voltage (V) and the clock frequency (f).
+       Pdyn = dynamic-power-coefficient * V^2 * f
+       where voltage is in uV, frequency is in MHz.

> > > - Even then, why should we be adding another table into DT for
> > >   voltage/power ? And not reuse and extend the opp-v2 stuff which is
> > >   already mainlined now.
> > 
> > We could reuse opp-v2 for static power points after OPPV2 back port to
> > our currently branch.
> 
> Your current branch doesn't matter to us. All that matters here is
> mainline, that's where you are adding code to. And you must test your
> stuff on the latest upstream branch only, not on some old kernel
> release. You can include other dependency patches though, that are
> required to make it work and mention them in cover-letter.

Thank you for your kindly explaining, Now I know I should develop and
test on mainline branch since this is where I try to add code.

However, please understanding currently mt8173_cpufreq.c is not ready
for OPPV2 in mainline as far, that's the reason why currently I can't
reuse OPPV2 and extend for static power table. My propose is for adding
CPU cooling device for our own product.

Actually, I would like to remove static power table on next patch since
static power is optional and dynamic power is much more than static
power.

> > However, as far as I know, there is no "power" in opp.c (suck like
> 
> s/suck/such ?
> 
> > opp-hz) as far, so I need to add something in opp.c for my purpose, suck
> > like add power in _opp_add_static_v2, and add something for return
> > "power", right? I may be wrong, please kindly give me your suggestion,
> > thank you.
> 
> You first need to propose a change in DT bindings for OPPs:
> Documentation/devicetree/bindings/opp/opp.txt
> 
> And then we can change the code properly.
> 
> > Actually, I am considering to remove the part of static power point
> > since it is optional for Power Model. Could you agree with this?
> 
> If its not important for your platform, then I don't have any issues
> with that..
> 



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 1/2] thermal: mediatek: Add cpu power cooling model.
  2015-11-04 19:40   ` Eduardo Valentin
@ 2015-11-05 11:10     ` dawei chien
  2015-11-06  3:24       ` Viresh Kumar
  0 siblings, 1 reply; 23+ messages in thread
From: dawei chien @ 2015-11-05 11:10 UTC (permalink / raw)
  To: Eduardo Valentin
  Cc: Viresh Kumar, Rafael J. Wysocki, Rob Herring, Pawel Moll,
	Mark Rutland, Ian Campbell, Kumar Gala, Matthias Brugger,
	Daniel Kurtz, Sascha Hauer, Daniel Lezcano, devicetree,
	linux-arm-kernel, linux-kernel, linux-pm, linux-mediatek,
	srv_heupstream, Sascha Hauer

On Wed, 2015-11-04 at 11:40 -0800, Eduardo Valentin wrote:
> On Thu, Oct 22, 2015 at 08:02:38PM +0800, Dawei Chien wrote:
> > This power model is base on Intelligent Power Allocation (IPA)
> > technical, requires that the operating-points of the CPUs are
> > registered using the kernel's opp library and the
> > `cpufreq_frequency_table` is assigned to the `struct device`
> > of the cpu MT8173.
> > 
> > Signed-off-by: Dawei.Chien <dawei.chien@mediatek.com>
> > ---
> > This patch is base on
> > https://patchwork.kernel.org/patch/7034601/
> > ---
> >  drivers/cpufreq/mt8173-cpufreq.c |  152 ++++++++++++++++++++++++++++++++++++--
> 
> Given that you are proposing this on top a DT binding, why reading a
> table of static power model applicable only to mt8173 cpufreq driver?
> 
> I have not seen anything specific (formula etc) that prevents this code
> to be generalized to other CPUs.
> 
> Can you please help me to understand?
> 
> BR,
This is because our platform currently only support mt8173_cpufreq.c, so
that I only add static power model for our owner IC.

Please understanding that I wouldn't give a DT binding document since I
will remove static power table on next version, but I can try to explain
it.

As far as I know, static power is somewhat leakage of CPU clusters, so
that we hardly to find a formula, which can suitable all kinds of CPUs
since leakage is different. In ARM IPA framework, static power only need
to be defined by who register cpufreq_power_cooling_register. The
voltage/power table is just one way to present leakage power of CPUs.

Actually, static power is optional since dynamic power is much more than
static power.

> >  1 file changed, 144 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
> > index 49caed2..23c19c5 100644
> > --- a/drivers/cpufreq/mt8173-cpufreq.c
> > +++ b/drivers/cpufreq/mt8173-cpufreq.c
> > @@ -29,6 +29,16 @@
> >  #define MAX_VOLT_LIMIT		(1150000)
> >  #define VOLT_TOL		(10000)
> >  
> > +struct mtk_cpu_static_power {
> > +	unsigned long voltage;
> > +	unsigned int power;
> > +};
> > +
> > +static struct mtk_cpu_static_power *mtk_ca53_static_power_table;
> > +static struct mtk_cpu_static_power *mtk_ca57_static_power_table;
> > +static int mtk_ca53_static_table_length;
> > +static int mtk_ca57_static_table_length;
> > +
> >  /*
> >   * The struct mtk_cpu_dvfs_info holds necessary information for doing CPU DVFS
> >   * on each CPU power/clock domain of Mediatek SoCs. Each CPU cluster in
> > @@ -51,6 +61,110 @@ struct mtk_cpu_dvfs_info {
> >  	bool need_voltage_tracking;
> >  };
> >  
> > +unsigned int mtk_cpufreq_lookup_power(const struct mtk_cpu_static_power *table,
> > +		unsigned int count, unsigned long voltage)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < count; i++) {
> > +		if (voltage <= table[i].voltage)
> > +			return table[i].power;
> > +	}
> > +
> > +	return table[count - 1].power;
> > +}
> > +
> > +int mtk_cpufreq_get_static(cpumask_t *cpumask, int interval,
> > +		unsigned long voltage, u32 *power)
> > +{
> > +	int nr_cpus = cpumask_weight(cpumask);
> > +
> > +	*power = 0;
> > +
> > +	if (nr_cpus) {
> > +		if (cpumask_test_cpu(0, cpumask))
> > +			*power += mtk_cpufreq_lookup_power(
> > +					mtk_ca53_static_power_table,
> > +					mtk_ca53_static_table_length,
> > +					voltage);
> > +
> > +		if (cpumask_test_cpu(2, cpumask))
> > +			*power += mtk_cpufreq_lookup_power(
> > +					mtk_ca57_static_power_table,
> > +					mtk_ca57_static_table_length,
> > +					voltage);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +unsigned int mtk_get_power_table_info(struct cpufreq_policy *policy,
> > +		struct device_node *np, const char *node_name)
> > +{
> > +	int mtk_static_table_length;
> > +	const struct property *prop;
> > +	struct mtk_cpu_dvfs_info *info = policy->driver_data;
> > +	struct device *cpu_dev = info->cpu_dev;
> > +	const __be32 *val;
> > +	int nr, i;
> > +
> > +	prop = of_find_property(np, node_name, NULL);
> > +
> > +	if (!prop) {
> > +		pr_err("failed to get static-power-points\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	if (!prop->value) {
> > +		pr_err("failed to get static power array data\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	nr = prop->length / sizeof(u32);
> > +
> > +	if (nr % 2) {
> > +		pr_err("Invalid OPP list\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	mtk_static_table_length = nr / 2;
> > +
> > +	if (cpumask_test_cpu(0, policy->related_cpus)) {
> > +		mtk_ca53_static_table_length = mtk_static_table_length;
> > +		mtk_ca53_static_power_table = devm_kcalloc(cpu_dev,
> > +					mtk_static_table_length,
> > +					sizeof(*mtk_ca53_static_power_table),
> > +					GFP_KERNEL);
> > +
> > +		val = prop->value;
> > +		for (i = 0; i < mtk_static_table_length; ++i) {
> > +			unsigned long voltage = be32_to_cpup(val++);
> > +			unsigned int power = be32_to_cpup(val++);
> > +
> > +			mtk_ca53_static_power_table[i].voltage = voltage;
> > +			mtk_ca53_static_power_table[i].power = power;
> > +			pr_info("volt:%ld uv, power:%d mW\n", voltage, power);
> > +		}
> > +	} else {
> > +                mtk_ca57_static_table_length = mtk_static_table_length;
> > +		mtk_ca57_static_power_table = devm_kcalloc(cpu_dev,
> > +					mtk_static_table_length,
> > +					sizeof(*mtk_ca57_static_power_table),
> > +					GFP_KERNEL);
> > +		val = prop->value;
> > +		for (i = 0; i < mtk_static_table_length; ++i) {
> > +			unsigned long voltage = be32_to_cpup(val++);
> > +			unsigned int power = be32_to_cpup(val++);
> > +
> > +			mtk_ca57_static_power_table[i].voltage = voltage;
> > +			mtk_ca57_static_power_table[i].power = power;
> > +			pr_info("volt:%ld uv, power:%d mW\n", voltage, power);
> > +		}
> > +	}
> > +
> > +	return	0;
> > +}
> > +
> >  static int mtk_cpufreq_voltage_tracking(struct mtk_cpu_dvfs_info *info,
> >  					int new_vproc)
> >  {
> > @@ -267,20 +381,40 @@ static void mtk_cpufreq_ready(struct cpufreq_policy *policy)
> >  {
> >  	struct mtk_cpu_dvfs_info *info = policy->driver_data;
> >  	struct device_node *np = of_node_get(info->cpu_dev->of_node);
> > +	u32 capacitance;
> > +	int ret;
> >  
> >  	if (WARN_ON(!np))
> >  		return;
> >  
> >  	if (of_find_property(np, "#cooling-cells", NULL)) {
> > -		info->cdev = of_cpufreq_cooling_register(np,
> > -							 policy->related_cpus);
> >  
> > -		if (IS_ERR(info->cdev)) {
> > -			dev_err(info->cpu_dev,
> > -				"running cpufreq without cooling device: %ld\n",
> > -				PTR_ERR(info->cdev));
> > +		if (!info->cdev) {
> > +
> > +			of_property_read_u32(np,
> > +					"dynamic-power-coefficient",
> > +					&capacitance);
> > +
> > +			ret = mtk_get_power_table_info(policy, np,
> > +						"static-power-points");
> > +			if (ret) {
> > +                                dev_err(info->cpu_dev,
> > +                                        "cpufreq without static-points: %d\n",
> > +                                        ret);
> > +			}
> > +
> > +			info->cdev = of_cpufreq_power_cooling_register(np,
> > +				policy->related_cpus,
> > +				capacitance,
> > +				mtk_cpufreq_get_static);
> > +
> > +			if (IS_ERR(info->cdev)) {
> > +				dev_err(info->cpu_dev,
> > +					"cpufreq without cdev: %ld\n",
> > +					PTR_ERR(info->cdev));
> > +					info->cdev = NULL;
> > +			}
> >  
> > -			info->cdev = NULL;
> >  		}
> >  	}
> >  
> > @@ -460,7 +594,9 @@ static int mtk_cpufreq_exit(struct cpufreq_policy *policy)
> >  {
> >  	struct mtk_cpu_dvfs_info *info = policy->driver_data;
> >  
> > -	cpufreq_cooling_unregister(info->cdev);
> > +	if (info->cdev)
> > +		cpufreq_cooling_unregister(info->cdev);
> > +
> >  	dev_pm_opp_free_cpufreq_table(info->cpu_dev, &policy->freq_table);
> >  	mtk_cpu_dvfs_info_release(info);
> >  	kfree(info);
> > -- 
> > 1.7.9.5
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 2/2] arm64: dts: mt8173: Add thermal zone node for mt8173.
  2015-11-04 19:41   ` Eduardo Valentin
@ 2015-11-05 11:10     ` dawei chien
  0 siblings, 0 replies; 23+ messages in thread
From: dawei chien @ 2015-11-05 11:10 UTC (permalink / raw)
  To: Eduardo Valentin
  Cc: Viresh Kumar, Rafael J. Wysocki, Rob Herring, Pawel Moll,
	Mark Rutland, Ian Campbell, Kumar Gala, Matthias Brugger,
	Daniel Kurtz, Sascha Hauer, Daniel Lezcano, devicetree,
	linux-arm-kernel, linux-kernel, linux-pm, linux-mediatek,
	srv_heupstream, Sascha Hauer

On Wed, 2015-11-04 at 11:41 -0800, Eduardo Valentin wrote:
> On Thu, Oct 22, 2015 at 08:02:39PM +0800, Dawei Chien wrote:
> > Add thermal zone node to mt8173.dtsi.
> > 
> > Signed-off-by: Dawei Chien <dawei.chien@mediatek.com>
> > ---
> > This patch is base on
> > https://patchwork.kernel.org/patch/7249821/
> > https://patchwork.kernel.org/patch/7249861/
> > https://patchwork.kernel.org/patch/7249891/
> > ---
> >  arch/arm64/boot/dts/mediatek/mt8173.dtsi |   90 ++++++++++++++++++++++++++++++
> 
> This patch adds also the static / dynamic power models too. Can you
> please split this patch in two: one for the thermal zones, one for the
> models.

Thank you for reviewing, I will split this patch in two and resend
again.

> >  1 file changed, 90 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > index 3b18f37..eaf12bf 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> > @@ -16,6 +16,7 @@
> >  #include <dt-bindings/interrupt-controller/arm-gic.h>
> >  #include <dt-bindings/power/mt8173-power.h>
> >  #include <dt-bindings/reset-controller/mt8173-resets.h>
> > +#include <dt-bindings/thermal/mt8173.h>
> >  #include "mt8173-pinfunc.h"
> >  
> >  / {
> > @@ -54,6 +55,18 @@
> >  			reg = <0x000>;
> >  			enable-method = "psci";
> >  			cpu-idle-states = <&CPU_SLEEP_0>;
> > +			static-power-points = <
> > +				859000  43
> > +				908000  52
> > +				983000  86
> > +				1009000 123
> > +				1028000 138
> > +				1083000 172
> > +				1110900 180
> > +				1125000 192
> > +			>;
> > +			dynamic-power-coefficient = <263>;
> > +			#cooling-cells = <2>;
> >  		};
> >  
> >  		cpu1: cpu@1 {
> > @@ -62,6 +75,17 @@
> >  			reg = <0x001>;
> >  			enable-method = "psci";
> >  			cpu-idle-states = <&CPU_SLEEP_0>;
> > +			static-power-points = <
> > +				859000  43
> > +				908000  52
> > +				983000  86
> > +				1009000 123
> > +				1028000 138
> > +				1083000 172
> > +				1110900 180
> > +				1125000 192
> > +			>;
> > +			dynamic-power-coefficient = <263>;
> >  		};
> >  
> >  		cpu2: cpu@100 {
> > @@ -70,6 +94,18 @@
> >  			reg = <0x100>;
> >  			enable-method = "psci";
> >  			cpu-idle-states = <&CPU_SLEEP_0>;
> > +			static-power-points = <
> > +				828000  72
> > +				867000  90
> > +				927000  156
> > +				968000  181
> > +				1007000 298
> > +				1049000 435
> > +				1089900 533
> > +				1125000 533
> > +			>;
> > +			dynamic-power-coefficient = <530>;
> > +			#cooling-cells = <2>;
> >  		};
> >  
> >  		cpu3: cpu@101 {
> > @@ -78,6 +114,17 @@
> >  			reg = <0x101>;
> >  			enable-method = "psci";
> >  			cpu-idle-states = <&CPU_SLEEP_0>;
> > +			static-power-points = <
> > +				828000  72
> > +				867000  90
> > +				927000  156
> > +				968000  181
> > +				1007000 298
> > +				1049000 435
> > +				1089900 533
> > +				1125000 533
> > +			>;
> > +			dynamic-power-coefficient = <530>;
> >  		};
> >  
> >  		idle-states {
> > @@ -116,6 +163,49 @@
> >  		clock-output-names = "clk32k";
> >  	};
> >  
> > +	thermal-zones {
> > +		cpu_thermal: cpu_thermal {
> > +			polling-delay-passive = <1000>; /* milliseconds */
> > +			polling-delay = <1000>; /* milliseconds */
> > +
> > +			thermal-sensors = <&thermal MT8173_THERMAL_ZONE_CA57>;
> > +			sustainable-power = <1500>;
> > +
> > +			trips {
> > +				threshold: trip-point@0 {
> > +					temperature = <68000>;
> > +					hysteresis = <2000>;
> > +					type = "passive";
> > +				};
> > +
> > +				target: trip-point@1 {
> > +					temperature = <85000>;
> > +					hysteresis = <2000>;
> > +					type = "passive";
> > +				};
> > +
> > +				cpu_crit: cpu_crit@0 {
> > +					temperature = <115000>;
> > +					hysteresis = <2000>;
> > +					type = "critical";
> > +				};
> > +			};
> > +
> > +			cooling-maps {
> > +				map@0 {
> > +					trip = <&target>;
> > +					cooling-device = <&cpu0 0 0>;
> > +					contribution = <1024>;
> > +				};
> > +				map@1 {
> > +					trip = <&target>;
> > +					cooling-device = <&cpu2 0 0>;
> > +					contribution = <2048>;
> > +				};
> > +			};
> > +		};
> > +	};
> > +
> >  	timer {
> >  		compatible = "arm,armv8-timer";
> >  		interrupt-parent = <&gic>;
> > -- 
> > 1.7.9.5
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 0/2] thermal: mediatek: Add cpu power cooling model
  2015-11-05 11:09       ` dawei chien
@ 2015-11-06  3:20         ` Viresh Kumar
  2015-11-11 12:00           ` dawei chien
  0 siblings, 1 reply; 23+ messages in thread
From: Viresh Kumar @ 2015-11-06  3:20 UTC (permalink / raw)
  To: dawei chien
  Cc: Rafael J. Wysocki, Rob Herring, Pawel Moll, Mark Rutland,
	Ian Campbell, Kumar Gala, Matthias Brugger, Daniel Kurtz,
	Sascha Hauer, Daniel Lezcano, devicetree, linux-arm-kernel,
	linux-kernel, linux-pm, linux-mediatek, srv_heupstream,
	Sascha Hauer

On 05-11-15, 19:09, dawei chien wrote:
> Thank you for your kindly explaining, now I could understand what I
> miss, I will send device tree binding on next version such like
> following description.
> 
> --- a/Documentation/devicetree/bindings/clock/mt8173-cpu-dvfs.txt
> +++ b/Documentation/devicetree/bindings/clock/mt8173-cpu-dvfs.txt
> @@ -10,6 +10,17 @@ Required properties:
>         Please refer to
> Documentation/devicetree/bindings/clk/clock-bindings.txt for
>         generic clock consumer properties.
>  - proc-supply: Regulator for Vproc of CPU cluster.
> +- dynamic-power-coefficient:
> +       Usage: optional
> +       Value type: <prop-encoded-array>
> +       Definition: A u32 value that represents an indicative
> +       running time dynamic power coefficient in
> +       fundamental units of mW/MHz/uVolt^2.
> +       The dynamic energy consumption of the CPU
> +       is proportional to the square of the
> +       Voltage (V) and the clock frequency (f).
> +       Pdyn = dynamic-power-coefficient * V^2 * f
> +       where voltage is in uV, frequency is in MHz.

Please check with Punit if he is planning to add the same..

> Thank you for your kindly explaining, Now I know I should develop and
> test on mainline branch since this is where I try to add code.
> 
> However, please understanding currently mt8173_cpufreq.c is not ready
> for OPPV2 in mainline as far, that's the reason why currently I can't
> reuse OPPV2 and extend for static power table. My propose is for adding
> CPU cooling device for our own product.

Firstly, we don't care. You are pushing something to mainline, you
have to get it tested someway on mainline.

Secondly, there are *almost* no changes required to the mtk cpufreq
driver for OPPV2. Just update your DT in a similar way it is done for
one of the exynos platforms and it should just work fine.

-- 
viresh

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 1/2] thermal: mediatek: Add cpu power cooling model.
  2015-11-05 11:10     ` dawei chien
@ 2015-11-06  3:24       ` Viresh Kumar
  2015-11-10 11:20         ` Javi Merino
  0 siblings, 1 reply; 23+ messages in thread
From: Viresh Kumar @ 2015-11-06  3:24 UTC (permalink / raw)
  To: dawei chien, Javi Merino
  Cc: Eduardo Valentin, Rafael J. Wysocki, Rob Herring, Pawel Moll,
	Mark Rutland, Ian Campbell, Kumar Gala, Matthias Brugger,
	Daniel Kurtz, Sascha Hauer, Daniel Lezcano, devicetree,
	linux-arm-kernel, linux-kernel, linux-pm, linux-mediatek,
	srv_heupstream, Sascha Hauer

Cc'ing Javi (which you should have as he wrote the power-thing for
cpu-cooling).

On 05-11-15, 19:10, dawei chien wrote:
> This is because our platform currently only support mt8173_cpufreq.c, so
> that I only add static power model for our owner IC.

Bindings are (normally) supposed to be general than a platform
specific.

> Please understanding that I wouldn't give a DT binding document since I
> will remove static power table on next version, but I can try to explain
> it.

Then just don't add things in the first place.

> As far as I know, static power is somewhat leakage of CPU clusters, so
> that we hardly to find a formula, which can suitable all kinds of CPUs
> since leakage is different. In ARM IPA framework, static power only need
> to be defined by who register cpufreq_power_cooling_register. The
> voltage/power table is just one way to present leakage power of CPUs.

The bindings don't fix the values for static power, but just provides
a field for platforms to use. Everyone can then send its own power
figures. Why do you thing it can't be generalized?

> Actually, static power is optional since dynamic power is much more than
> static power.

Maybe, we should still capture it.

@Javi ?

-- 
viresh

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 1/2] thermal: mediatek: Add cpu power cooling model.
  2015-11-06  3:24       ` Viresh Kumar
@ 2015-11-10 11:20         ` Javi Merino
  2015-11-10 18:41           ` Eduardo Valentin
  2015-11-13  5:02           ` Viresh Kumar
  0 siblings, 2 replies; 23+ messages in thread
From: Javi Merino @ 2015-11-10 11:20 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: dawei chien, Eduardo Valentin, Rafael J. Wysocki, Rob Herring,
	Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
	Matthias Brugger, Daniel Kurtz, Sascha Hauer, Daniel Lezcano,
	devicetree, linux-arm-kernel, linux-kernel, linux-pm,
	linux-mediatek, srv_heupstream, Sascha Hauer

On Fri, Nov 06, 2015 at 08:54:33AM +0530, Viresh Kumar wrote:
> Cc'ing Javi (which you should have as he wrote the power-thing for
> cpu-cooling).
> 
> On 05-11-15, 19:10, dawei chien wrote:
> > This is because our platform currently only support mt8173_cpufreq.c, so
> > that I only add static power model for our owner IC.
> 
> Bindings are (normally) supposed to be general than a platform
> specific.
> 
> > Please understanding that I wouldn't give a DT binding document since I
> > will remove static power table on next version, but I can try to explain
> > it.
> 
> Then just don't add things in the first place.
> 
> > As far as I know, static power is somewhat leakage of CPU clusters, so
> > that we hardly to find a formula, which can suitable all kinds of CPUs
> > since leakage is different. In ARM IPA framework, static power only need
> > to be defined by who register cpufreq_power_cooling_register. The
> > voltage/power table is just one way to present leakage power of CPUs.
> 
> The bindings don't fix the values for static power, but just provides
> a field for platforms to use. Everyone can then send its own power
> figures. Why do you thing it can't be generalized?

The way they are described here is useful only for this platform, but
it's not generic.  It only takes into account voltage as (I assume)
it's the only variable that affects it in this implementation.  A
generalized version of the static power should take into account the
temperature and the idle state.

> > Actually, static power is optional since dynamic power is much more than
> > static power.
> 
> Maybe, we should still capture it.
> 
> @Javi ?

It really depends on the platform.  If dawei says that for their
platform static power is negligible then it is ok to not capture it.

Cheers,
Javi

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 1/2] thermal: mediatek: Add cpu power cooling model.
  2015-11-10 11:20         ` Javi Merino
@ 2015-11-10 18:41           ` Eduardo Valentin
  2015-11-11  9:36             ` Javi Merino
  2015-11-13  5:02           ` Viresh Kumar
  1 sibling, 1 reply; 23+ messages in thread
From: Eduardo Valentin @ 2015-11-10 18:41 UTC (permalink / raw)
  To: Javi Merino
  Cc: Viresh Kumar, dawei chien, Rafael J. Wysocki, Rob Herring,
	Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
	Matthias Brugger, Daniel Kurtz, Sascha Hauer, Daniel Lezcano,
	devicetree, linux-arm-kernel, linux-kernel, linux-pm,
	linux-mediatek, srv_heupstream, Sascha Hauer

On Tue, Nov 10, 2015 at 11:20:18AM +0000, Javi Merino wrote:
> On Fri, Nov 06, 2015 at 08:54:33AM +0530, Viresh Kumar wrote:
> > Cc'ing Javi (which you should have as he wrote the power-thing for
> > cpu-cooling).
> > 
> > On 05-11-15, 19:10, dawei chien wrote:
> > > This is because our platform currently only support mt8173_cpufreq.c, so
> > > that I only add static power model for our owner IC.
> > 
> > Bindings are (normally) supposed to be general than a platform
> > specific.
> > 
> > > Please understanding that I wouldn't give a DT binding document since I
> > > will remove static power table on next version, but I can try to explain
> > > it.
> > 
> > Then just don't add things in the first place.
> > 
> > > As far as I know, static power is somewhat leakage of CPU clusters, so
> > > that we hardly to find a formula, which can suitable all kinds of CPUs
> > > since leakage is different. In ARM IPA framework, static power only need
> > > to be defined by who register cpufreq_power_cooling_register. The
> > > voltage/power table is just one way to present leakage power of CPUs.
> > 
> > The bindings don't fix the values for static power, but just provides
> > a field for platforms to use. Everyone can then send its own power
> > figures. Why do you thing it can't be generalized?
> 
> The way they are described here is useful only for this platform, but
> it's not generic.  It only takes into account voltage as (I assume)
> it's the only variable that affects it in this implementation.  A
> generalized version of the static power should take into account the
> temperature and the idle state.

Still, why would we have one binding to describe static power per platform?

I would prefer we go towards a generalized binding description.

If temperature is not needed on all platforms, make it an optional
property.

BR,

Eduardo Valentin

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 1/2] thermal: mediatek: Add cpu power cooling model.
  2015-11-10 18:41           ` Eduardo Valentin
@ 2015-11-11  9:36             ` Javi Merino
  0 siblings, 0 replies; 23+ messages in thread
From: Javi Merino @ 2015-11-11  9:36 UTC (permalink / raw)
  To: Eduardo Valentin
  Cc: Viresh Kumar, dawei chien, Rafael J. Wysocki, Rob Herring,
	Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
	Matthias Brugger, Daniel Kurtz, Sascha Hauer, Daniel Lezcano,
	devicetree, linux-arm-kernel, linux-kernel, linux-pm,
	linux-mediatek, srv_heupstream, Sascha Hauer

On Tue, Nov 10, 2015 at 10:41:22AM -0800, Eduardo Valentin wrote:
> On Tue, Nov 10, 2015 at 11:20:18AM +0000, Javi Merino wrote:
> > On Fri, Nov 06, 2015 at 08:54:33AM +0530, Viresh Kumar wrote:
> > > Cc'ing Javi (which you should have as he wrote the power-thing for
> > > cpu-cooling).
> > > 
> > > On 05-11-15, 19:10, dawei chien wrote:
> > > > This is because our platform currently only support mt8173_cpufreq.c, so
> > > > that I only add static power model for our owner IC.
> > > 
> > > Bindings are (normally) supposed to be general than a platform
> > > specific.
> > > 
> > > > Please understanding that I wouldn't give a DT binding document since I
> > > > will remove static power table on next version, but I can try to explain
> > > > it.
> > > 
> > > Then just don't add things in the first place.
> > > 
> > > > As far as I know, static power is somewhat leakage of CPU clusters, so
> > > > that we hardly to find a formula, which can suitable all kinds of CPUs
> > > > since leakage is different. In ARM IPA framework, static power only need
> > > > to be defined by who register cpufreq_power_cooling_register. The
> > > > voltage/power table is just one way to present leakage power of CPUs.
> > > 
> > > The bindings don't fix the values for static power, but just provides
> > > a field for platforms to use. Everyone can then send its own power
> > > figures. Why do you thing it can't be generalized?
> > 
> > The way they are described here is useful only for this platform, but
> > it's not generic.  It only takes into account voltage as (I assume)
> > it's the only variable that affects it in this implementation.  A
> > generalized version of the static power should take into account the
> > temperature and the idle state.
> 
> Still, why would we have one binding to describe static power per platform?
> 
> I would prefer we go towards a generalized binding description.

Sure, I wasn't saying that we want one binding per platform.  I was
just saying that this binding can't be made generic because it's not.

Cheers,
Javi

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 0/2] thermal: mediatek: Add cpu power cooling model
  2015-11-06  3:20         ` Viresh Kumar
@ 2015-11-11 12:00           ` dawei chien
  0 siblings, 0 replies; 23+ messages in thread
From: dawei chien @ 2015-11-11 12:00 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Rafael J. Wysocki, Rob Herring, Pawel Moll, Mark Rutland,
	Ian Campbell, Kumar Gala, Matthias Brugger, Daniel Kurtz,
	Sascha Hauer, Daniel Lezcano, devicetree, linux-arm-kernel,
	linux-kernel, linux-pm, linux-mediatek, srv_heupstream,
	Sascha Hauer

On Fri, 2015-11-06 at 08:50 +0530, Viresh Kumar wrote:
> On 05-11-15, 19:09, dawei chien wrote:
> > Thank you for your kindly explaining, now I could understand what I
> > miss, I will send device tree binding on next version such like
> > following description.
> > 
> > --- a/Documentation/devicetree/bindings/clock/mt8173-cpu-dvfs.txt
> > +++ b/Documentation/devicetree/bindings/clock/mt8173-cpu-dvfs.txt
> > @@ -10,6 +10,17 @@ Required properties:
> >         Please refer to
> > Documentation/devicetree/bindings/clk/clock-bindings.txt for
> >         generic clock consumer properties.
> >  - proc-supply: Regulator for Vproc of CPU cluster.
> > +- dynamic-power-coefficient:
> > +       Usage: optional
> > +       Value type: <prop-encoded-array>
> > +       Definition: A u32 value that represents an indicative
> > +       running time dynamic power coefficient in
> > +       fundamental units of mW/MHz/uVolt^2.
> > +       The dynamic energy consumption of the CPU
> > +       is proportional to the square of the
> > +       Voltage (V) and the clock frequency (f).
> > +       Pdyn = dynamic-power-coefficient * V^2 * f
> > +       where voltage is in uV, frequency is in MHz.
> 
> Please check with Punit if he is planning to add the same.
Punit just sent the patch for this binding[1] yesterday, so I will re-send next version once his patch has been reviewed.
> > Thank you for your kindly explaining, Now I know I should develop and
> > test on mainline branch since this is where I try to add code.
> > 
> > However, please understanding currently mt8173_cpufreq.c is not ready
> > for OPPV2 in mainline as far, that's the reason why currently I can't
> > reuse OPPV2 and extend for static power table. My propose is for adding
> > CPU cooling device for our own product.
> 
> Firstly, we don't care. You are pushing something to mainline, you
> have to get it tested someway on mainline.
> 
> Secondly, there are *almost* no changes required to the mtk cpufreq
> driver for OPPV2. Just update your DT in a similar way it is done for
> one of the exynos platforms and it should just work fine.
> 
In our platform, thermal throttling is good enough with dynamic power
only, so my plan is to send dynamic power model first in next version.

Regarding static power model, we will continue discussing with ARM to
find a better solution.

Thanks.

[1] https://lkml.org/lkml/2015/11/9/542

BR,
Dawei



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 1/2] thermal: mediatek: Add cpu power cooling model.
  2015-11-10 11:20         ` Javi Merino
  2015-11-10 18:41           ` Eduardo Valentin
@ 2015-11-13  5:02           ` Viresh Kumar
  1 sibling, 0 replies; 23+ messages in thread
From: Viresh Kumar @ 2015-11-13  5:02 UTC (permalink / raw)
  To: Javi Merino
  Cc: dawei chien, Eduardo Valentin, Rafael J. Wysocki, Rob Herring,
	Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
	Matthias Brugger, Daniel Kurtz, Sascha Hauer, Daniel Lezcano,
	devicetree, linux-arm-kernel, linux-kernel, linux-pm,
	linux-mediatek, srv_heupstream, Sascha Hauer

On 10-11-15, 11:20, Javi Merino wrote:
> The way they are described here is useful only for this platform, but
> it's not generic.  It only takes into account voltage as (I assume)
> it's the only variable that affects it in this implementation.  A
> generalized version of the static power should take into account the
> temperature and the idle state.

yeah, but I thought we are talking about the final static power being
present in the DT. How the platform gets it, doesn't matter at all.
They may consider just the voltage or temperature and idle state as
well.

-- 
viresh

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2015-11-13  5:02 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-22 12:02 [PATCH v3 0/2] thermal: mediatek: Add cpu power cooling model Dawei Chien
2015-10-22 12:02 ` [PATCH v3 1/2] " Dawei Chien
2015-11-04 19:40   ` Eduardo Valentin
2015-11-05 11:10     ` dawei chien
2015-11-06  3:24       ` Viresh Kumar
2015-11-10 11:20         ` Javi Merino
2015-11-10 18:41           ` Eduardo Valentin
2015-11-11  9:36             ` Javi Merino
2015-11-13  5:02           ` Viresh Kumar
2015-10-22 12:02 ` [PATCH v3 2/2] arm64: dts: mt8173: Add thermal zone node for mt8173 Dawei Chien
2015-10-28 15:39   ` Viresh Kumar
2015-11-02 10:51     ` dawei chien
2015-11-04 19:41   ` Eduardo Valentin
2015-11-05 11:10     ` dawei chien
2015-10-28 15:44 ` [PATCH v3 0/2] thermal: mediatek: Add cpu power cooling model Viresh Kumar
2015-11-02 10:46   ` dawei chien
2015-11-02 12:10     ` Viresh Kumar
2015-11-05 11:09       ` dawei chien
2015-11-06  3:20         ` Viresh Kumar
2015-11-11 12:00           ` dawei chien
2015-11-02 15:53   ` Punit Agrawal
2015-11-02 16:10     ` Viresh Kumar
2015-11-04 19:36     ` Eduardo Valentin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).