devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/3] thermal: mediatek: Add cpu power cooling model
@ 2015-11-27  9:32 Dawei Chien
       [not found] ` <1448616727-29367-1-git-send-email-dawei.chien-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
  2015-11-27  9:32 ` [PATCH v4 2/3] arm64: dts: mt8173: Add dynamic power node Dawei Chien
  0 siblings, 2 replies; 10+ messages in thread
From: Dawei Chien @ 2015-11-27  9:32 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-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-pm-u79uwXL29TY76Z2rM5mHXA,
	linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	srv_heupstream-NuS5LvNUpcJWk0Htik3J/w, Sascha Hauer,
	Punit Agrawal

Use Intelligent Power Allocation (IPA) technical to add 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.

Binging document is refer to this patchset
https://lkml.org/lkml/2015/11/17/251

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

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

Change since V3:
1. Remove static power model.
2. Split V3's device tree in two for thermal zones and dynamic power models respectively.

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

 arch/arm64/boot/dts/mediatek/mt8173.dtsi |   47 ++++++++++++++++++++++++++++++
 drivers/cpufreq/mt8173-cpufreq.c         |   28 +++++++++++++-----
 2 files changed, 67 insertions(+), 8 deletions(-)

--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

* [PATCH v4 1/3] arm64: dts: mt8173: Add thermal zone node.
       [not found] ` <1448616727-29367-1-git-send-email-dawei.chien-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
@ 2015-11-27  9:32   ` Dawei Chien
  2015-11-27  9:32   ` [PATCH v4 3/3] thermal: mediatek: Add cpu dynamic power cooling model Dawei Chien
  1 sibling, 0 replies; 10+ messages in thread
From: Dawei Chien @ 2015-11-27  9:32 UTC (permalink / raw)
  To: Viresh Kumar
  Cc: Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA, Punit Agrawal,
	srv_heupstream-NuS5LvNUpcJWk0Htik3J/w, Pawel Moll, Ian Campbell,
	Dawei Chien, Sascha Hauer, Daniel Lezcano, Rafael J. Wysocki,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Rob Herring,
	linux-pm-u79uwXL29TY76Z2rM5mHXA, Sascha Hauer, Kumar Gala,
	Matthias Brugger, linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r

This adds thermal zone node to Mediatek MT8173 dtsi file.

Signed-off-by: Dawei Chien <dawei.chien-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
---
This patch is based on patchset:
https://lkml.org/lkml/2015/11/18/84
---
 arch/arm64/boot/dts/mediatek/mt8173.dtsi |   43 ++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
index fda805d..4114cee 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
@@ -188,6 +188,49 @@
 		clock-output-names = "cpum_ck";
 	};
 
+	thermal-zones {
+		cpu_thermal: cpu_thermal {
+			polling-delay-passive = <1000>; /* milliseconds */
+			polling-delay = <1000>; /* milliseconds */
+
+			thermal-sensors = <&thermal 0>;
+			sustainable-power = <1500>; /* milliwatts */
+
+			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] 10+ messages in thread

* [PATCH v4 2/3] arm64: dts: mt8173: Add dynamic power node.
  2015-11-27  9:32 [PATCH v4 0/3] thermal: mediatek: Add cpu power cooling model Dawei Chien
       [not found] ` <1448616727-29367-1-git-send-email-dawei.chien-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
@ 2015-11-27  9:32 ` Dawei Chien
  1 sibling, 0 replies; 10+ messages in thread
From: Dawei Chien @ 2015-11-27  9:32 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, Punit Agrawal

This device node is for calculating dynamic power in mW.
Since mt8173 has two clusters, there are two dynamic power
coefficient as well.

Signed-off-by: Dawei Chien <dawei.chien@mediatek.com>
---
This patch is base on patchset:
https://lkml.org/lkml/2015/11/17/251
---
 arch/arm64/boot/dts/mediatek/mt8173.dtsi |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
index 4114cee..33fabe4 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
@@ -71,6 +71,7 @@
 			#cooling-cells = <2>;
 			#cooling-min-level = <0>;
 			#cooling-max-level = <7>;
+			dynamic-power-coefficient = <263>;
 		};
 
 		cpu1: cpu@1 {
@@ -95,6 +96,7 @@
 			#cooling-cells = <2>;
 			#cooling-min-level = <0>;
 			#cooling-max-level = <7>;
+			dynamic-power-coefficient = <263>;
 		};
 
 		cpu2: cpu@100 {
@@ -119,6 +121,7 @@
 			#cooling-cells = <2>;
 			#cooling-min-level = <0>;
 			#cooling-max-level = <7>;
+			dynamic-power-coefficient = <530>;
 		};
 
 		cpu3: cpu@101 {
@@ -143,6 +146,7 @@
 			#cooling-cells = <2>;
 			#cooling-min-level = <0>;
 			#cooling-max-level = <7>;
+			dynamic-power-coefficient = <530>;
 		};
 
 		idle-states {
-- 
1.7.9.5


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

* [PATCH v4 3/3] thermal: mediatek: Add cpu dynamic power cooling model.
       [not found] ` <1448616727-29367-1-git-send-email-dawei.chien-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
  2015-11-27  9:32   ` [PATCH v4 1/3] arm64: dts: mt8173: Add thermal zone node Dawei Chien
@ 2015-11-27  9:32   ` Dawei Chien
  2015-11-30  5:38     ` Viresh Kumar
  1 sibling, 1 reply; 10+ messages in thread
From: Dawei Chien @ 2015-11-27  9:32 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-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-pm-u79uwXL29TY76Z2rM5mHXA,
	linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	srv_heupstream-NuS5LvNUpcJWk0Htik3J/w, Sascha Hauer,
	Punit Agrawal

MT8173 cpufreq driver use of_cpufreq_power_cooling_register registering
cooling devices with dynamic power coefficient.

Signed-off-by: Dawei Chien <dawei.chien-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
---
This patch is base on patchset:
https://lkml.org/lkml/2015/11/17/251
---
 drivers/cpufreq/mt8173-cpufreq.c |   28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
index 83001dc..4d39468 100644
--- a/drivers/cpufreq/mt8173-cpufreq.c
+++ b/drivers/cpufreq/mt8173-cpufreq.c
@@ -263,24 +263,34 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
 	return 0;
 }
 
+#define DYNAMIC_POWER "dynamic-power-coefficient"
+
 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;
 
 	if (WARN_ON(!np))
 		return;
 
 	if (of_find_property(np, "#cooling-cells", NULL)) {
-		info->cdev = of_cpufreq_cooling_register(np,
-							 policy->related_cpus);
+		if (!info->cdev) {
+			of_property_read_u32(np, DYNAMIC_POWER, &capacitance);
+
+			info->cdev = of_cpufreq_power_cooling_register(np,
+							policy->related_cpus,
+							capacitance,
+							NULL);
 
-		if (IS_ERR(info->cdev)) {
-			dev_err(info->cpu_dev,
-				"running cpufreq without cooling device: %ld\n",
-				PTR_ERR(info->cdev));
+			if (IS_ERR(info->cdev)) {
+				dev_err(info->cpu_dev,
+					"running cpufreq without cooling device: %ld\n",
+					PTR_ERR(info->cdev));
 
-			info->cdev = NULL;
+				info->cdev = NULL;
+			}
 		}
 	}
 
@@ -460,7 +470,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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

* Re: [PATCH v4 3/3] thermal: mediatek: Add cpu dynamic power cooling model.
  2015-11-27  9:32   ` [PATCH v4 3/3] thermal: mediatek: Add cpu dynamic power cooling model Dawei Chien
@ 2015-11-30  5:38     ` Viresh Kumar
  2015-11-30  9:26       ` dawei chien
  0 siblings, 1 reply; 10+ messages in thread
From: Viresh Kumar @ 2015-11-30  5:38 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, Punit Agrawal

On 27-11-15, 17:32, Dawei Chien wrote:
> MT8173 cpufreq driver use of_cpufreq_power_cooling_register registering
> cooling devices with dynamic power coefficient.
> 
> Signed-off-by: Dawei Chien <dawei.chien@mediatek.com>
> ---
> This patch is base on patchset:
> https://lkml.org/lkml/2015/11/17/251
> ---
>  drivers/cpufreq/mt8173-cpufreq.c |   28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
> index 83001dc..4d39468 100644
> --- a/drivers/cpufreq/mt8173-cpufreq.c
> +++ b/drivers/cpufreq/mt8173-cpufreq.c
> @@ -263,24 +263,34 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
>  	return 0;
>  }
>  
> +#define DYNAMIC_POWER "dynamic-power-coefficient"
> +
>  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;
>  
>  	if (WARN_ON(!np))
>  		return;
>  
>  	if (of_find_property(np, "#cooling-cells", NULL)) {
> -		info->cdev = of_cpufreq_cooling_register(np,
> -							 policy->related_cpus);
> +		if (!info->cdev) {

Why will info->cdev be non-NULL here ?

> +			of_property_read_u32(np, DYNAMIC_POWER, &capacitance);

This can fail, in which case capacitance will be used uninitialized.
Fix that by initializing it with 0 at the beginning of this routine.

> +			info->cdev = of_cpufreq_power_cooling_register(np,
> +							policy->related_cpus,
> +							capacitance,
> +							NULL);
>  
> -		if (IS_ERR(info->cdev)) {
> -			dev_err(info->cpu_dev,
> -				"running cpufreq without cooling device: %ld\n",
> -				PTR_ERR(info->cdev));
> +			if (IS_ERR(info->cdev)) {
> +				dev_err(info->cpu_dev,
> +					"running cpufreq without cooling device: %ld\n",
> +					PTR_ERR(info->cdev));
>  
> -			info->cdev = NULL;
> +				info->cdev = NULL;
> +			}
>  		}
>  	}
>  
> @@ -460,7 +470,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);
> +

Why do you need to update this?

>  	dev_pm_opp_free_cpufreq_table(info->cpu_dev, &policy->freq_table);
>  	mtk_cpu_dvfs_info_release(info);
>  	kfree(info);
> -- 
> 1.7.9.5

-- 
viresh

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

* Re: [PATCH v4 3/3] thermal: mediatek: Add cpu dynamic power cooling model.
  2015-11-30  5:38     ` Viresh Kumar
@ 2015-11-30  9:26       ` dawei chien
  2015-11-30  9:30         ` Viresh Kumar
  0 siblings, 1 reply; 10+ messages in thread
From: dawei chien @ 2015-11-30  9:26 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, Punit Agrawal

On Mon, 2015-11-30 at 11:08 +0530, Viresh Kumar wrote:
> On 27-11-15, 17:32, Dawei Chien wrote:
> > MT8173 cpufreq driver use of_cpufreq_power_cooling_register registering
> > cooling devices with dynamic power coefficient.
> > 
> > Signed-off-by: Dawei Chien <dawei.chien@mediatek.com>
> > ---
> > This patch is base on patchset:
> > https://lkml.org/lkml/2015/11/17/251
> > ---
> >  drivers/cpufreq/mt8173-cpufreq.c |   28 ++++++++++++++++++++--------
> >  1 file changed, 20 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
> > index 83001dc..4d39468 100644
> > --- a/drivers/cpufreq/mt8173-cpufreq.c
> > +++ b/drivers/cpufreq/mt8173-cpufreq.c
> > @@ -263,24 +263,34 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
> >  	return 0;
> >  }
> >  
> > +#define DYNAMIC_POWER "dynamic-power-coefficient"
> > +
> >  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;
> >  
> >  	if (WARN_ON(!np))
> >  		return;
> >  
> >  	if (of_find_property(np, "#cooling-cells", NULL)) {
> > -		info->cdev = of_cpufreq_cooling_register(np,
> > -							 policy->related_cpus);
> > +		if (!info->cdev) {
> 
> Why will info->cdev be non-NULL here ?

This is a error-checking to avoid user or any script by command line hotplug CPU
more than two times, we don't need to register cooling device on this case.

I will remove it if you don't agree it.

> > +			of_property_read_u32(np, DYNAMIC_POWER, &capacitance);
> 
> This can fail, in which case capacitance will be used uninitialized.
> Fix that by initializing it with 0 at the beginning of this routine.

Thank you, I will follow your comment to fix it on next version.

> > +			info->cdev = of_cpufreq_power_cooling_register(np,
> > +							policy->related_cpus,
> > +							capacitance,
> > +							NULL);
> >  
> > -		if (IS_ERR(info->cdev)) {
> > -			dev_err(info->cpu_dev,
> > -				"running cpufreq without cooling device: %ld\n",
> > -				PTR_ERR(info->cdev));
> > +			if (IS_ERR(info->cdev)) {
> > +				dev_err(info->cpu_dev,
> > +					"running cpufreq without cooling device: %ld\n",
> > +					PTR_ERR(info->cdev));
> >  
> > -			info->cdev = NULL;
> > +				info->cdev = NULL;
> > +			}
> >  		}
> >  	}
> >  
> > @@ -460,7 +470,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);
> > +
> 
> Why do you need to update this?

This is a error-checking to avoid user or any script by command line
unplug CPU more than two times, we don't need to unregister cooling
device on this case.

I will remove it if you don't agree it.

> >  	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	[flat|nested] 10+ messages in thread

* Re: [PATCH v4 3/3] thermal: mediatek: Add cpu dynamic power cooling model.
  2015-11-30  9:26       ` dawei chien
@ 2015-11-30  9:30         ` Viresh Kumar
  2015-11-30 10:21           ` dawei chien
  0 siblings, 1 reply; 10+ messages in thread
From: Viresh Kumar @ 2015-11-30  9:30 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, Punit Agrawal

On 30-11-15, 17:26, dawei chien wrote:
> On Mon, 2015-11-30 at 11:08 +0530, Viresh Kumar wrote:
> > On 27-11-15, 17:32, Dawei Chien wrote:
> > > MT8173 cpufreq driver use of_cpufreq_power_cooling_register registering
> > > cooling devices with dynamic power coefficient.
> > > 
> > > Signed-off-by: Dawei Chien <dawei.chien@mediatek.com>
> > > ---
> > > This patch is base on patchset:
> > > https://lkml.org/lkml/2015/11/17/251
> > > ---
> > >  drivers/cpufreq/mt8173-cpufreq.c |   28 ++++++++++++++++++++--------
> > >  1 file changed, 20 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
> > > index 83001dc..4d39468 100644
> > > --- a/drivers/cpufreq/mt8173-cpufreq.c
> > > +++ b/drivers/cpufreq/mt8173-cpufreq.c
> > > @@ -263,24 +263,34 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
> > >  	return 0;
> > >  }
> > >  
> > > +#define DYNAMIC_POWER "dynamic-power-coefficient"
> > > +
> > >  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;
> > >  
> > >  	if (WARN_ON(!np))
> > >  		return;
> > >  
> > >  	if (of_find_property(np, "#cooling-cells", NULL)) {
> > > -		info->cdev = of_cpufreq_cooling_register(np,
> > > -							 policy->related_cpus);
> > > +		if (!info->cdev) {
> > 
> > Why will info->cdev be non-NULL here ?
> 
> This is a error-checking to avoid user or any script by command line hotplug CPU
> more than two times, we don't need to register cooling device on this case.

Why?

> I will remove it if you don't agree it.

No, my agreeing or not doesn't matter. If what you are doing is useful
(and I am not able to understand it), then you should make me
understand that and don't change your code.

But, I really do not get the reasoning behind the logic. Please
elaborate that step by step.

> > > @@ -460,7 +470,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);
> > > +
> > 
> > Why do you need to update this?
> 
> This is a error-checking to avoid user or any script by command line
> unplug CPU more than two times, we don't need to unregister cooling
> device on this case.
> 
> I will remove it if you don't agree it.

Same here.

-- 
viresh

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

* Re: [PATCH v4 3/3] thermal: mediatek: Add cpu dynamic power cooling model.
  2015-11-30  9:30         ` Viresh Kumar
@ 2015-11-30 10:21           ` dawei chien
  2015-11-30 10:34             ` Viresh Kumar
  0 siblings, 1 reply; 10+ messages in thread
From: dawei chien @ 2015-11-30 10:21 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-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-pm-u79uwXL29TY76Z2rM5mHXA,
	linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	srv_heupstream-NuS5LvNUpcJWk0Htik3J/w, Sascha Hauer,
	Punit Agrawal

On Mon, 2015-11-30 at 15:00 +0530, Viresh Kumar wrote:
> On 30-11-15, 17:26, dawei chien wrote:
> > On Mon, 2015-11-30 at 11:08 +0530, Viresh Kumar wrote:
> > > On 27-11-15, 17:32, Dawei Chien wrote:
> > > > MT8173 cpufreq driver use of_cpufreq_power_cooling_register registering
> > > > cooling devices with dynamic power coefficient.
> > > > 
> > > > Signed-off-by: Dawei Chien <dawei.chien-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > > > ---
> > > > This patch is base on patchset:
> > > > https://lkml.org/lkml/2015/11/17/251
> > > > ---
> > > >  drivers/cpufreq/mt8173-cpufreq.c |   28 ++++++++++++++++++++--------
> > > >  1 file changed, 20 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
> > > > index 83001dc..4d39468 100644
> > > > --- a/drivers/cpufreq/mt8173-cpufreq.c
> > > > +++ b/drivers/cpufreq/mt8173-cpufreq.c
> > > > @@ -263,24 +263,34 @@ static int mtk_cpufreq_set_target(struct cpufreq_policy *policy,
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +#define DYNAMIC_POWER "dynamic-power-coefficient"
> > > > +
> > > >  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;
> > > >  
> > > >  	if (WARN_ON(!np))
> > > >  		return;
> > > >  
> > > >  	if (of_find_property(np, "#cooling-cells", NULL)) {
> > > > -		info->cdev = of_cpufreq_cooling_register(np,
> > > > -							 policy->related_cpus);
> > > > +		if (!info->cdev) {
> > > 
> > > Why will info->cdev be non-NULL here ?
> > 
> > This is a error-checking to avoid user or any script by command line hotplug CPU
> > more than two times, we don't need to register cooling device on this case.
> 
> Why?
As far as I know, user or shell script has the right for using command online/offline cpu.
Either user by console or shell script could make cpu2 online even cpu2 already onlined.
That could cause of_cpufreq_cooling_register execute many times for the
same cooling device.

"echo 1 > /sys/devices/system/cpu/cpu2/online"

With above hotplug command, mtk_cpufreq_ready will register cooling
device again as well, but fail since the cooling device already created,
so we might need not register cooling device again on this case.

> > I will remove it if you don't agree it.
> 
> No, my agreeing or not doesn't matter. If what you are doing is useful
> (and I am not able to understand it), then you should make me
> understand that and don't change your code.
> 
> But, I really do not get the reasoning behind the logic. Please
> elaborate that step by step.

Please refer to above explaining, thank you.

> > > > @@ -460,7 +470,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);
> > > > +
> > > 
> > > Why do you need to update this?
> > 
> > This is a error-checking to avoid user or any script by command line
> > unplug CPU more than two times, we don't need to unregister cooling
> > device on this case.
> > 
> > I will remove it if you don't agree it.
> 
> Same here.
Please refer to the same reason as above. thank you.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

* Re: [PATCH v4 3/3] thermal: mediatek: Add cpu dynamic power cooling model.
  2015-11-30 10:21           ` dawei chien
@ 2015-11-30 10:34             ` Viresh Kumar
  2015-11-30 12:16               ` dawei chien
  0 siblings, 1 reply; 10+ messages in thread
From: Viresh Kumar @ 2015-11-30 10:34 UTC (permalink / raw)
  To: dawei chien
  Cc: Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA, Punit Agrawal,
	srv_heupstream-NuS5LvNUpcJWk0Htik3J/w, Pawel Moll, Ian Campbell,
	Sascha Hauer, Daniel Lezcano, Rafael J. Wysocki,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Rob Herring,
	linux-pm-u79uwXL29TY76Z2rM5mHXA, Sascha Hauer, Kumar Gala,
	Matthias Brugger, linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r

On 30-11-15, 18:21, dawei chien wrote:
> As far as I know, user or shell script has the right for using command online/offline cpu.

Right.

> Either user by console or shell script could make cpu2 online even cpu2 already onlined.

Hey, no. You can't online that is already online. You can write '1' to
the file /sys/devices/system/cpu/cpuX/online, but it wouldn't have any
affect.

> That could cause of_cpufreq_cooling_register execute many times for the
> same cooling device.

No.

> "echo 1 > /sys/devices/system/cpu/cpu2/online"
> 
> With above hotplug command, mtk_cpufreq_ready will register cooling
> device again as well, but fail since the cooling device already created,
> so we might need not register cooling device again on this case.

Have you tested this yourself ?

-- 
viresh

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

* Re: [PATCH v4 3/3] thermal: mediatek: Add cpu dynamic power cooling model.
  2015-11-30 10:34             ` Viresh Kumar
@ 2015-11-30 12:16               ` dawei chien
  0 siblings, 0 replies; 10+ messages in thread
From: dawei chien @ 2015-11-30 12:16 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-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-pm-u79uwXL29TY76Z2rM5mHXA,
	linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	srv_heupstream-NuS5LvNUpcJWk0Htik3J/w, Sascha Hauer,
	Punit Agrawal

On Mon, 2015-11-30 at 16:04 +0530, Viresh Kumar wrote:
> On 30-11-15, 18:21, dawei chien wrote:
> > As far as I know, user or shell script has the right for using command online/offline cpu.
> 
> Right.
> 
> > Either user by console or shell script could make cpu2 online even cpu2 already onlined.
> 
> Hey, no. You can't online that is already online. You can write '1' to
> the file /sys/devices/system/cpu/cpuX/online, but it wouldn't have any
> affect.
> 
> > That could cause of_cpufreq_cooling_register execute many times for the
> > same cooling device.
> 
> No.
You are right. certainly no, it's my misunderstand.

> > "echo 1 > /sys/devices/system/cpu/cpu2/online"
> > 
> > With above hotplug command, mtk_cpufreq_ready will register cooling
> > device again as well, but fail since the cooling device already created,
> > so we might need not register cooling device again on this case.
> 
> Have you tested this yourself ?

Truly sorry for misunderstanding, so we don't need to add error checking
code there.

(It seems whole of cluster offline, the cooling device unregister as
well. If just one cpu online, cooling device register as well)

Three months ago, Once I used following command, cooling device can't
create any more. err message will show once I echo online command.
However, this bug already fixed in mainline by you. 

echo 0 > /sys/devices/system/cpu/cpu2/online
echo 0 > /sys/devices/system/cpu/cpu3/online
echo 1 > /sys/devices/system/cpu/cpu2/online

commit 528464eaa46ae1bd319882e4dd3495802e55b8c4
Author: Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Date:   Thu Jul 23 14:32:32 2015 +0530

    thermal: remove dangling 'weight_attr' device file

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

end of thread, other threads:[~2015-11-30 12:16 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-11-27  9:32 [PATCH v4 0/3] thermal: mediatek: Add cpu power cooling model Dawei Chien
     [not found] ` <1448616727-29367-1-git-send-email-dawei.chien-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
2015-11-27  9:32   ` [PATCH v4 1/3] arm64: dts: mt8173: Add thermal zone node Dawei Chien
2015-11-27  9:32   ` [PATCH v4 3/3] thermal: mediatek: Add cpu dynamic power cooling model Dawei Chien
2015-11-30  5:38     ` Viresh Kumar
2015-11-30  9:26       ` dawei chien
2015-11-30  9:30         ` Viresh Kumar
2015-11-30 10:21           ` dawei chien
2015-11-30 10:34             ` Viresh Kumar
2015-11-30 12:16               ` dawei chien
2015-11-27  9:32 ` [PATCH v4 2/3] arm64: dts: mt8173: Add dynamic power node Dawei Chien

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