Exynos5422 odroidxu3 pwm-fan control using thermal sensors

Exynos5422 odroidxu3 pwm-fan control using thermal sensors

[Anand Moon]

This work depeds upon work done by Lukasz Majewski <l.majewski@samsung.com>
 and Sjoerd Simons <sjoerd.simons@collabora.co.uk> regarding the pwm-fan.

-Anand Moon

[PATCH 1/6] ARM: dts :exynos5422-odroidxu3 Add pwm-fan node to the Odroid-XU3 board.

[Anand Moon]

Add pwm-fan node to the OdroidXU3 board.

Tested on OdroidXU3 board.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 arch/arm/boot/dts/exynos5422-odroidxu3.dts | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5422-odroidxu3.dts b/arch/arm/boot/dts/exynos5422-odroidxu3.dts
index a519c86..eaec006 100644
--- a/arch/arm/boot/dts/exynos5422-odroidxu3.dts
+++ b/arch/arm/boot/dts/exynos5422-odroidxu3.dts
@@ -278,6 +278,15 @@
 	rtc@101E0000 {
 		status = "okay";
 	};
+
+	fan0: pwm-fan {
+		compatible = "pwm-fan";
+		pwms = <&pwm 0 20972 0>;
+		cooling-min-state = <0>;
+		cooling-max-state = <3>;
+		#cooling-cells = <2>;
+		cooling-levels = <0 90 130 170 230>;
+	};
 };
 
 &hdmi {
@@ -369,3 +378,10 @@
 		shunt-resistor = <10000>;
 	};
 };
+
+&pwm {
+	pinctrl-0 = <&pwm0_out>;
+	pinctrl-names = "default";
+	samsung,pwm-outputs = <0>;
+	status = "okay";
+};
-- 
1.9.1

Re: [PATCH 1/6] ARM: dts :exynos5422-odroidxu3 Add pwm-fan node to the Odroid-XU3 board.

[Lukasz Majewski]

Hi Anand,

> Add pwm-fan node to the OdroidXU3 board.
> 
> Tested on OdroidXU3 board.
> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>  arch/arm/boot/dts/exynos5422-odroidxu3.dts | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/exynos5422-odroidxu3.dts
> b/arch/arm/boot/dts/exynos5422-odroidxu3.dts index a519c86..eaec006
> 100644 --- a/arch/arm/boot/dts/exynos5422-odroidxu3.dts
> +++ b/arch/arm/boot/dts/exynos5422-odroidxu3.dts
> @@ -278,6 +278,15 @@
>  	rtc@101E0000 {
>  		status = "okay";
>  	};
> +
> +	fan0: pwm-fan {
> +		compatible = "pwm-fan";
> +		pwms = <&pwm 0 20972 0>;
> +		cooling-min-state = <0>;
> +		cooling-max-state = <3>;
> +		#cooling-cells = <2>;
> +		cooling-levels = <0 90 130 170 230>;
> +	};
>  };
>  
>  &hdmi {
> @@ -369,3 +378,10 @@
>  		shunt-resistor = <10000>;
>  	};
>  };
> +
> +&pwm {
> +	pinctrl-0 = <&pwm0_out>;
> +	pinctrl-names = "default";
> +	samsung,pwm-outputs = <0>;
> +	status = "okay";
> +};

Acked-by: Lukasz Majewski <l.majewski@samsung.com>

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[PATCH 2/6] ARM: dts exynos5420 update the cooling cells for core cpu0

[Anand Moon]

update the cooling level for cpu0 to avoid following message.

root@odroidxu3:~# dmesg | grep ther
[    0.241511] /thermal-zones/cpu-thermal/cooling-maps/map0:
			 could not get #cooling-cells for /cpus/cpu@0

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 arch/arm/boot/dts/exynos5420.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
index c0e98cf..6b49f3c 100644
--- a/arch/arm/boot/dts/exynos5420.dtsi
+++ b/arch/arm/boot/dts/exynos5420.dtsi
@@ -61,6 +61,10 @@
 			reg = <0x0>;
 			clock-frequency = <1800000000>;
 			cci-control-port = <&cci_control1>;
+
+			cooling-min-level = <10>;
+			cooling-max-level = <7>;
+			#cooling-cells = <2>; /* min followed by max */
 		};
 
 		cpu1: cpu@1 {
-- 
1.9.1

Re: [PATCH 2/6] ARM: dts exynos5420 update the cooling cells for core cpu0

[Lukasz Majewski]

Hi Anand,

> update the cooling level for cpu0 to avoid following message.
> 
> root@odroidxu3:~# dmesg | grep ther
> [    0.241511] /thermal-zones/cpu-thermal/cooling-maps/map0:
> 			 could not get #cooling-cells for /cpus/cpu@0
> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>  arch/arm/boot/dts/exynos5420.dtsi | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/exynos5420.dtsi
> b/arch/arm/boot/dts/exynos5420.dtsi index c0e98cf..6b49f3c 100644
> --- a/arch/arm/boot/dts/exynos5420.dtsi
> +++ b/arch/arm/boot/dts/exynos5420.dtsi
> @@ -61,6 +61,10 @@
>  			reg = <0x0>;
>  			clock-frequency = <1800000000>;
>  			cci-control-port = <&cci_control1>;
> +
> +			cooling-min-level = <10>;
> +			cooling-max-level = <7>;
> +			#cooling-cells = <2>; /* min followed by max
> */ };
>  
>  		cpu1: cpu@1 {

Acked-by: Lukasz Majewski <l.majewski@samsung.com>

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[PATCH 3/6] ARM:dts exynos5422 Update the thermal sensor for tmu_cpu0

[Anand Moon]

Move the registration of thermal sensors for tmu_cpu0 from exynos5420.dtsi
to exynos5-cpu-thermal.dtsi, to avoid duplicate registration of the sensors.

Tested on OdroidXU3 board.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 arch/arm/boot/dts/exynos5-cpu-thermal.dtsi | 58 ++++++++++++++++++++++++++++++
 arch/arm/boot/dts/exynos5420.dtsi          |  4 ---
 arch/arm/boot/dts/exynos5422-odroidxu3.dts |  1 +
 3 files changed, 59 insertions(+), 4 deletions(-)
 create mode 100644 arch/arm/boot/dts/exynos5-cpu-thermal.dtsi

diff --git a/arch/arm/boot/dts/exynos5-cpu-thermal.dtsi b/arch/arm/boot/dts/exynos5-cpu-thermal.dtsi
new file mode 100644
index 0000000..8fede70
--- /dev/null
+++ b/arch/arm/boot/dts/exynos5-cpu-thermal.dtsi
@@ -0,0 +1,58 @@
+/*
+ * Device tree sources for Exynos5 thermal zone
+ *
+ * Copyright (c) 2014 Lukasz Majewski <l.majewski@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <dt-bindings/thermal/thermal.h>
+
+/ {
+	thermal-zones {
+		cpu0_thermal: cpu0-thermal {
+			thermal-sensors = <&tmu_cpu0>;
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
+			trips {
+				cpu_alert0: cpu-alert-0 {
+					temperature = <48000>; /* ms */
+					hysteresis = <3000>; /* ms */
+					type = "active";
+				};
+				cpu_alert1: cpu-alert-1 {
+					temperature = <53000>; /* ms */
+					hysteresis = <3000>; /* ms */
+					type = "active";
+				};
+				cpu_alert2: cpu-alert-2 {
+					temperature = <59000>; /* ms */
+					hysteresis = <3000>; /* ms */
+					type = "active";
+				};
+				cpu_crit0: cpu-crit-0 {
+					temperature = <75000>; /* ms */
+					hysteresis = <0>; /* ms */
+					type = "critical";
+				};
+			};
+			cooling-maps {
+				map0 {
+				     trip = <&cpu_alert0>;
+				     cooling-device = <&fan0 0 1>;
+				};
+				map1 {
+				     trip = <&cpu_alert1>;
+				     cooling-device = <&fan0 1 2>;
+				};
+				map2 {
+				     trip = <&cpu_alert2>;
+				     cooling-device = <&fan0 2 3>;
+				};
+			};
+		};
+	};
+};
diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
index 6b49f3c..eb0f16c 100644
--- a/arch/arm/boot/dts/exynos5420.dtsi
+++ b/arch/arm/boot/dts/exynos5420.dtsi
@@ -827,10 +827,6 @@
 	};
 
 	thermal-zones {
-		cpu0_thermal: cpu0-thermal {
-			thermal-sensors = <&tmu_cpu0>;
-			#include "exynos5420-trip-points.dtsi"
-		};
 		cpu1_thermal: cpu1-thermal {
 		       thermal-sensors = <&tmu_cpu1>;
 		       #include "exynos5420-trip-points.dtsi"
diff --git a/arch/arm/boot/dts/exynos5422-odroidxu3.dts b/arch/arm/boot/dts/exynos5422-odroidxu3.dts
index eaec006..c8b3e3e 100644
--- a/arch/arm/boot/dts/exynos5422-odroidxu3.dts
+++ b/arch/arm/boot/dts/exynos5422-odroidxu3.dts
@@ -12,6 +12,7 @@
 
 /dts-v1/;
 #include "exynos5800.dtsi"
+#include "exynos5-cpu-thermal.dtsi"
 
 / {
 	model = "Hardkernel Odroid XU3";
-- 
1.9.1

Re: [PATCH 3/6] ARM:dts exynos5422 Update the thermal sensor for tmu_cpu0

[Lukasz Majewski]

Hi Anand,

> Move the registration of thermal sensors for tmu_cpu0 from
> exynos5420.dtsi to exynos5-cpu-thermal.dtsi, to avoid duplicate
> registration of the sensors.
> 
> Tested on OdroidXU3 board.
> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>  arch/arm/boot/dts/exynos5-cpu-thermal.dtsi | 58
> ++++++++++++++++++++++++++++++
> arch/arm/boot/dts/exynos5420.dtsi          |  4 ---
> arch/arm/boot/dts/exynos5422-odroidxu3.dts |  1 + 3 files changed, 59
> insertions(+), 4 deletions(-) create mode 100644
> arch/arm/boot/dts/exynos5-cpu-thermal.dtsi
> 
> diff --git a/arch/arm/boot/dts/exynos5-cpu-thermal.dtsi
> b/arch/arm/boot/dts/exynos5-cpu-thermal.dtsi new file mode 100644
> index 0000000..8fede70
> --- /dev/null
> +++ b/arch/arm/boot/dts/exynos5-cpu-thermal.dtsi
> @@ -0,0 +1,58 @@
> +/*
> + * Device tree sources for Exynos5 thermal zone
> + *
> + * Copyright (c) 2014 Lukasz Majewski <l.majewski@samsung.com>

			^^^^^^^^^^^^^^^^ Could you update this
			line :-). I'm just reviewer here :-)

> + *
> + * This program is free software; you can redistribute it and/or
> modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#include <dt-bindings/thermal/thermal.h>
> +
> +/ {
> +	thermal-zones {
> +		cpu0_thermal: cpu0-thermal {
> +			thermal-sensors = <&tmu_cpu0>;
> +			polling-delay-passive = <0>;
> +			polling-delay = <0>;
> +			trips {
> +				cpu_alert0: cpu-alert-0 {
> +					temperature = <48000>; /* ms
> */
> +					hysteresis = <3000>; /* ms */
							    ^^^^^^^^^
							We already have
							"millicelsius"
							instead od ms
> +					type = "active";
> +				};
> +				cpu_alert1: cpu-alert-1 {
> +					temperature = <53000>; /* ms
> */
> +					hysteresis = <3000>; /* ms */
> +					type = "active";
> +				};
> +				cpu_alert2: cpu-alert-2 {
> +					temperature = <59000>; /* ms
> */
> +					hysteresis = <3000>; /* ms */
> +					type = "active";
> +				};
> +				cpu_crit0: cpu-crit-0 {
> +					temperature = <75000>; /* ms
> */
> +					hysteresis = <0>; /* ms */
> +					type = "critical";

		Is there any special reasons why we need special values
		for cpu0_thermal sensor at XU3? Is something wrong with
		default values defined at exynos4-cpu-thermal.dtsi?

		If this is Odroid XU3 specific, then IMHO it should be
		added to exynos5422-odroidxu3.dts

> +				};
> +			};
> +			cooling-maps {
> +				map0 {
> +				     trip = <&cpu_alert0>;
> +				     cooling-device = <&fan0 0 1>;
> +				};
> +				map1 {
> +				     trip = <&cpu_alert1>;
> +				     cooling-device = <&fan0 1 2>;
> +				};
> +				map2 {
> +				     trip = <&cpu_alert2>;
> +				     cooling-device = <&fan0 2 3>;
> +				};
> +			};
> +		};
> +	};
> +};
> diff --git a/arch/arm/boot/dts/exynos5420.dtsi
> b/arch/arm/boot/dts/exynos5420.dtsi index 6b49f3c..eb0f16c 100644
> --- a/arch/arm/boot/dts/exynos5420.dtsi
> +++ b/arch/arm/boot/dts/exynos5420.dtsi
> @@ -827,10 +827,6 @@
>  	};
>  
>  	thermal-zones {
> -		cpu0_thermal: cpu0-thermal {
> -			thermal-sensors = <&tmu_cpu0>;
> -			#include "exynos5420-trip-points.dtsi"
			^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [1]

> -		};
>  		cpu1_thermal: cpu1-thermal {
>  		       thermal-sensors = <&tmu_cpu1>;
>  		       #include "exynos5420-trip-points.dtsi"
> diff --git a/arch/arm/boot/dts/exynos5422-odroidxu3.dts
> b/arch/arm/boot/dts/exynos5422-odroidxu3.dts index eaec006..c8b3e3e
> 100644 --- a/arch/arm/boot/dts/exynos5422-odroidxu3.dts
> +++ b/arch/arm/boot/dts/exynos5422-odroidxu3.dts
> @@ -12,6 +12,7 @@
>  
>  /dts-v1/;
>  #include "exynos5800.dtsi"
> +#include "exynos5-cpu-thermal.dtsi"

I would prefer to stick to convention proposed at [1]. It keeps
the locality of the include with respective thermal zone.

>  
>  / {
>  	model = "Hardkernel Odroid XU3";

DTS changes should be added to Samsung tree by Samsung maintainer - Mr.
Kukjin Kim.

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH 3/6] ARM:dts exynos5422 Update the thermal sensor for tmu_cpu0

[Anand Moon]

Hi Lukasz,

On Odroidxu3 board critical temp is 85.0 C. Below is the sensors output.

therm_zone0-virtual-0
Adapter: Virtual device
temp1: +39.0▒ၰC (crit = +85.0▒°C)
temp2: +38.0▒°C (crit = +85.0▒°C)
temp3: +40.0▒°C (crit = +85.0▒°C)
temp4: +49.0▒°C (crit = +85.0▒°C)
temp5: +29.0▒°C (crit = +85.0▒°C)

I have observed cpu shutdown as we do test pm-qa thermal testing.

So I propose the temperature values to be 50000, 65000 ,80000 and
85000 Critical temperature.

Please share your thoughts.

-Anand Moon

On 8 April 2015 at 13:32, Lukasz Majewski <l.majewski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> Hi Anand,
>
>> Move the registration of thermal sensors for tmu_cpu0 from
>> exynos5420.dtsi to exynos5-cpu-thermal.dtsi, to avoid duplicate
>> registration of the sensors.
>>
>> Tested on OdroidXU3 board.
>>
>> Signed-off-by: Anand Moon <linux.amoon-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  arch/arm/boot/dts/exynos5-cpu-thermal.dtsi | 58
>> ++++++++++++++++++++++++++++++
>> arch/arm/boot/dts/exynos5420.dtsi          |  4 ---
>> arch/arm/boot/dts/exynos5422-odroidxu3.dts |  1 + 3 files changed, 59
>> insertions(+), 4 deletions(-) create mode 100644
>> arch/arm/boot/dts/exynos5-cpu-thermal.dtsi
>>
>> diff --git a/arch/arm/boot/dts/exynos5-cpu-thermal.dtsi
>> b/arch/arm/boot/dts/exynos5-cpu-thermal.dtsi new file mode 100644
>> index 0000000..8fede70
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/exynos5-cpu-thermal.dtsi
>> @@ -0,0 +1,58 @@
>> +/*
>> + * Device tree sources for Exynos5 thermal zone
>> + *
>> + * Copyright (c) 2014 Lukasz Majewski <l.majewski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>
>                         ^^^^^^^^^^^^^^^^ Could you update this
>                         line :-). I'm just reviewer here :-)
>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + */
>> +
>> +#include <dt-bindings/thermal/thermal.h>
>> +
>> +/ {
>> +     thermal-zones {
>> +             cpu0_thermal: cpu0-thermal {
>> +                     thermal-sensors = <&tmu_cpu0>;
>> +                     polling-delay-passive = <0>;
>> +                     polling-delay = <0>;
>> +                     trips {
>> +                             cpu_alert0: cpu-alert-0 {
>> +                                     temperature = <48000>; /* ms
>> */
>> +                                     hysteresis = <3000>; /* ms */
>                                                             ^^^^^^^^^
>                                                         We already have
>                                                         "millicelsius"
>                                                         instead od ms
>> +                                     type = "active";
>> +                             };
>> +                             cpu_alert1: cpu-alert-1 {
>> +                                     temperature = <53000>; /* ms
>> */
>> +                                     hysteresis = <3000>; /* ms */
>> +                                     type = "active";
>> +                             };
>> +                             cpu_alert2: cpu-alert-2 {
>> +                                     temperature = <59000>; /* ms
>> */
>> +                                     hysteresis = <3000>; /* ms */
>> +                                     type = "active";
>> +                             };
>> +                             cpu_crit0: cpu-crit-0 {
>> +                                     temperature = <75000>; /* ms
>> */
>> +                                     hysteresis = <0>; /* ms */
>> +                                     type = "critical";
>
>                 Is there any special reasons why we need special values
>                 for cpu0_thermal sensor at XU3? Is something wrong with
>                 default values defined at exynos4-cpu-thermal.dtsi?
>
>                 If this is Odroid XU3 specific, then IMHO it should be
>                 added to exynos5422-odroidxu3.dts
>
>> +                             };
>> +                     };
>> +                     cooling-maps {
>> +                             map0 {
>> +                                  trip = <&cpu_alert0>;
>> +                                  cooling-device = <&fan0 0 1>;
>> +                             };
>> +                             map1 {
>> +                                  trip = <&cpu_alert1>;
>> +                                  cooling-device = <&fan0 1 2>;
>> +                             };
>> +                             map2 {
>> +                                  trip = <&cpu_alert2>;
>> +                                  cooling-device = <&fan0 2 3>;
>> +                             };
>> +                     };
>> +             };
>> +     };
>> +};
>> diff --git a/arch/arm/boot/dts/exynos5420.dtsi
>> b/arch/arm/boot/dts/exynos5420.dtsi index 6b49f3c..eb0f16c 100644
>> --- a/arch/arm/boot/dts/exynos5420.dtsi
>> +++ b/arch/arm/boot/dts/exynos5420.dtsi
>> @@ -827,10 +827,6 @@
>>       };
>>
>>       thermal-zones {
>> -             cpu0_thermal: cpu0-thermal {
>> -                     thermal-sensors = <&tmu_cpu0>;
>> -                     #include "exynos5420-trip-points.dtsi"
>                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [1]
>
>> -             };
>>               cpu1_thermal: cpu1-thermal {
>>                      thermal-sensors = <&tmu_cpu1>;
>>                      #include "exynos5420-trip-points.dtsi"
>> diff --git a/arch/arm/boot/dts/exynos5422-odroidxu3.dts
>> b/arch/arm/boot/dts/exynos5422-odroidxu3.dts index eaec006..c8b3e3e
>> 100644 --- a/arch/arm/boot/dts/exynos5422-odroidxu3.dts
>> +++ b/arch/arm/boot/dts/exynos5422-odroidxu3.dts
>> @@ -12,6 +12,7 @@
>>
>>  /dts-v1/;
>>  #include "exynos5800.dtsi"
>> +#include "exynos5-cpu-thermal.dtsi"
>
> I would prefer to stick to convention proposed at [1]. It keeps
> the locality of the include with respective thermal zone.
>
>>
>>  / {
>>       model = "Hardkernel Odroid XU3";
>
> DTS changes should be added to Samsung tree by Samsung maintainer - Mr.
> Kukjin Kim.
>
> --
> Best regards,
>
> Lukasz Majewski
>
> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
--
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

Re: [PATCH 3/6] ARM:dts exynos5422 Update the thermal sensor for tmu_cpu0

[Lukasz Majewski]

Hi Anand,

> Hi Lukasz,
> 
> On Odroidxu3 board critical temp is 85.0 C. Below is the sensors
> output.

Do you have XU3 with a fan?

> 
> therm_zone0-virtual-0
> Adapter: Virtual device
> temp1: +39.0▒ၰC (crit = +85.0▒°C)
> temp2: +38.0▒°C (crit = +85.0▒°C)
> temp3: +40.0▒°C (crit = +85.0▒°C)
> temp4: +49.0▒°C (crit = +85.0▒°C)
> temp5: +29.0▒°C (crit = +85.0▒°C)
> 
> I have observed cpu shutdown as we do test pm-qa thermal testing.
> 
> So I propose the temperature values to be 50000, 65000 ,80000 and
> 85000 Critical temperature.
> 

This seems strange - since other Samsung's SoC have higher work
temperatures (both exynos5 and exynos4).

I will try to check those thresholds on my XU3. Have anybody else
experienced overheating at XU3?

Sjoerd? Kukjin?

> Please share your thoughts.
> 
> -Anand Moon
> 
> On 8 April 2015 at 13:32, Lukasz Majewski <l.majewski@samsung.com>
> wrote:
> > Hi Anand,
> >
> >> Move the registration of thermal sensors for tmu_cpu0 from
> >> exynos5420.dtsi to exynos5-cpu-thermal.dtsi, to avoid duplicate
> >> registration of the sensors.
> >>
> >> Tested on OdroidXU3 board.
> >>
> >> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> >> ---
> >>  arch/arm/boot/dts/exynos5-cpu-thermal.dtsi | 58
> >> ++++++++++++++++++++++++++++++
> >> arch/arm/boot/dts/exynos5420.dtsi          |  4 ---
> >> arch/arm/boot/dts/exynos5422-odroidxu3.dts |  1 + 3 files changed,
> >> 59 insertions(+), 4 deletions(-) create mode 100644
> >> arch/arm/boot/dts/exynos5-cpu-thermal.dtsi
> >>
> >> diff --git a/arch/arm/boot/dts/exynos5-cpu-thermal.dtsi
> >> b/arch/arm/boot/dts/exynos5-cpu-thermal.dtsi new file mode 100644
> >> index 0000000..8fede70
> >> --- /dev/null
> >> +++ b/arch/arm/boot/dts/exynos5-cpu-thermal.dtsi
> >> @@ -0,0 +1,58 @@
> >> +/*
> >> + * Device tree sources for Exynos5 thermal zone
> >> + *
> >> + * Copyright (c) 2014 Lukasz Majewski <l.majewski@samsung.com>
> >
> >                         ^^^^^^^^^^^^^^^^ Could you update this
> >                         line :-). I'm just reviewer here :-)
> >
> >> + *
> >> + * This program is free software; you can redistribute it and/or
> >> modify
> >> + * it under the terms of the GNU General Public License version 2
> >> as
> >> + * published by the Free Software Foundation.
> >> + *
> >> + */
> >> +
> >> +#include <dt-bindings/thermal/thermal.h>
> >> +
> >> +/ {
> >> +     thermal-zones {
> >> +             cpu0_thermal: cpu0-thermal {
> >> +                     thermal-sensors = <&tmu_cpu0>;
> >> +                     polling-delay-passive = <0>;
> >> +                     polling-delay = <0>;
> >> +                     trips {
> >> +                             cpu_alert0: cpu-alert-0 {
> >> +                                     temperature = <48000>; /* ms
> >> */
> >> +                                     hysteresis = <3000>; /* ms */
> >                                                             ^^^^^^^^^
> >                                                         We already
> > have "millicelsius"
> >                                                         instead od
> > ms
> >> +                                     type = "active";
> >> +                             };
> >> +                             cpu_alert1: cpu-alert-1 {
> >> +                                     temperature = <53000>; /* ms
> >> */
> >> +                                     hysteresis = <3000>; /* ms */
> >> +                                     type = "active";
> >> +                             };
> >> +                             cpu_alert2: cpu-alert-2 {
> >> +                                     temperature = <59000>; /* ms
> >> */
> >> +                                     hysteresis = <3000>; /* ms */
> >> +                                     type = "active";
> >> +                             };
> >> +                             cpu_crit0: cpu-crit-0 {
> >> +                                     temperature = <75000>; /* ms
> >> */
> >> +                                     hysteresis = <0>; /* ms */
> >> +                                     type = "critical";
> >
> >                 Is there any special reasons why we need special
> > values for cpu0_thermal sensor at XU3? Is something wrong with
> >                 default values defined at exynos4-cpu-thermal.dtsi?
> >
> >                 If this is Odroid XU3 specific, then IMHO it should
> > be added to exynos5422-odroidxu3.dts
> >
> >> +                             };
> >> +                     };
> >> +                     cooling-maps {
> >> +                             map0 {
> >> +                                  trip = <&cpu_alert0>;
> >> +                                  cooling-device = <&fan0 0 1>;
> >> +                             };
> >> +                             map1 {
> >> +                                  trip = <&cpu_alert1>;
> >> +                                  cooling-device = <&fan0 1 2>;
> >> +                             };
> >> +                             map2 {
> >> +                                  trip = <&cpu_alert2>;
> >> +                                  cooling-device = <&fan0 2 3>;
> >> +                             };
> >> +                     };
> >> +             };
> >> +     };
> >> +};
> >> diff --git a/arch/arm/boot/dts/exynos5420.dtsi
> >> b/arch/arm/boot/dts/exynos5420.dtsi index 6b49f3c..eb0f16c 100644
> >> --- a/arch/arm/boot/dts/exynos5420.dtsi
> >> +++ b/arch/arm/boot/dts/exynos5420.dtsi
> >> @@ -827,10 +827,6 @@
> >>       };
> >>
> >>       thermal-zones {
> >> -             cpu0_thermal: cpu0-thermal {
> >> -                     thermal-sensors = <&tmu_cpu0>;
> >> -                     #include "exynos5420-trip-points.dtsi"
> >                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [1]
> >
> >> -             };
> >>               cpu1_thermal: cpu1-thermal {
> >>                      thermal-sensors = <&tmu_cpu1>;
> >>                      #include "exynos5420-trip-points.dtsi"
> >> diff --git a/arch/arm/boot/dts/exynos5422-odroidxu3.dts
> >> b/arch/arm/boot/dts/exynos5422-odroidxu3.dts index eaec006..c8b3e3e
> >> 100644 --- a/arch/arm/boot/dts/exynos5422-odroidxu3.dts
> >> +++ b/arch/arm/boot/dts/exynos5422-odroidxu3.dts
> >> @@ -12,6 +12,7 @@
> >>
> >>  /dts-v1/;
> >>  #include "exynos5800.dtsi"
> >> +#include "exynos5-cpu-thermal.dtsi"
> >
> > I would prefer to stick to convention proposed at [1]. It keeps
> > the locality of the include with respective thermal zone.
> >
> >>
> >>  / {
> >>       model = "Hardkernel Odroid XU3";
> >
> > DTS changes should be added to Samsung tree by Samsung maintainer -
> > Mr. Kukjin Kim.
> >
> > --
> > Best regards,
> >
> > Lukasz Majewski
> >
> > Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH 3/6] ARM:dts exynos5422 Update the thermal sensor for tmu_cpu0

[Anand Moon]

Hi Lukasz,

I have Odroidxu3 board with fan. Odroid xu3 board and run on high temperature.

As per my understating as the sensors temperature cross the first
alert it triggers fan to run.
I was doing some stress testing to trigger this overheating with my changes.

-Anand Moon

On 8 April 2015 at 15:00, Lukasz Majewski <l.majewski@samsung.com> wrote:
> Hi Anand,
>
>> Hi Lukasz,
>>
>> On Odroidxu3 board critical temp is 85.0 C. Below is the sensors
>> output.
>
> Do you have XU3 with a fan?
>
>>
>> therm_zone0-virtual-0
>> Adapter: Virtual device
>> temp1: +39.0▒ၰC (crit = +85.0▒°C)
>> temp2: +38.0▒°C (crit = +85.0▒°C)
>> temp3: +40.0▒°C (crit = +85.0▒°C)
>> temp4: +49.0▒°C (crit = +85.0▒°C)
>> temp5: +29.0▒°C (crit = +85.0▒°C)
>>
>> I have observed cpu shutdown as we do test pm-qa thermal testing.
>>
>> So I propose the temperature values to be 50000, 65000 ,80000 and
>> 85000 Critical temperature.
>>
>
> This seems strange - since other Samsung's SoC have higher work
> temperatures (both exynos5 and exynos4).
>
> I will try to check those thresholds on my XU3. Have anybody else
> experienced overheating at XU3?
>
> Sjoerd? Kukjin?
>
>> Please share your thoughts.
>>
>> -Anand Moon
>>
>> On 8 April 2015 at 13:32, Lukasz Majewski <l.majewski@samsung.com>
>> wrote:
>> > Hi Anand,
>> >
>> >> Move the registration of thermal sensors for tmu_cpu0 from
>> >> exynos5420.dtsi to exynos5-cpu-thermal.dtsi, to avoid duplicate
>> >> registration of the sensors.
>> >>
>> >> Tested on OdroidXU3 board.
>> >>
>> >> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>> >> ---
>> >>  arch/arm/boot/dts/exynos5-cpu-thermal.dtsi | 58
>> >> ++++++++++++++++++++++++++++++
>> >> arch/arm/boot/dts/exynos5420.dtsi          |  4 ---
>> >> arch/arm/boot/dts/exynos5422-odroidxu3.dts |  1 + 3 files changed,
>> >> 59 insertions(+), 4 deletions(-) create mode 100644
>> >> arch/arm/boot/dts/exynos5-cpu-thermal.dtsi
>> >>
>> >> diff --git a/arch/arm/boot/dts/exynos5-cpu-thermal.dtsi
>> >> b/arch/arm/boot/dts/exynos5-cpu-thermal.dtsi new file mode 100644
>> >> index 0000000..8fede70
>> >> --- /dev/null
>> >> +++ b/arch/arm/boot/dts/exynos5-cpu-thermal.dtsi
>> >> @@ -0,0 +1,58 @@
>> >> +/*
>> >> + * Device tree sources for Exynos5 thermal zone
>> >> + *
>> >> + * Copyright (c) 2014 Lukasz Majewski <l.majewski@samsung.com>
>> >
>> >                         ^^^^^^^^^^^^^^^^ Could you update this
>> >                         line :-). I'm just reviewer here :-)
>> >
>> >> + *
>> >> + * This program is free software; you can redistribute it and/or
>> >> modify
>> >> + * it under the terms of the GNU General Public License version 2
>> >> as
>> >> + * published by the Free Software Foundation.
>> >> + *
>> >> + */
>> >> +
>> >> +#include <dt-bindings/thermal/thermal.h>
>> >> +
>> >> +/ {
>> >> +     thermal-zones {
>> >> +             cpu0_thermal: cpu0-thermal {
>> >> +                     thermal-sensors = <&tmu_cpu0>;
>> >> +                     polling-delay-passive = <0>;
>> >> +                     polling-delay = <0>;
>> >> +                     trips {
>> >> +                             cpu_alert0: cpu-alert-0 {
>> >> +                                     temperature = <48000>; /* ms
>> >> */
>> >> +                                     hysteresis = <3000>; /* ms */
>> >                                                             ^^^^^^^^^
>> >                                                         We already
>> > have "millicelsius"
>> >                                                         instead od
>> > ms
>> >> +                                     type = "active";
>> >> +                             };
>> >> +                             cpu_alert1: cpu-alert-1 {
>> >> +                                     temperature = <53000>; /* ms
>> >> */
>> >> +                                     hysteresis = <3000>; /* ms */
>> >> +                                     type = "active";
>> >> +                             };
>> >> +                             cpu_alert2: cpu-alert-2 {
>> >> +                                     temperature = <59000>; /* ms
>> >> */
>> >> +                                     hysteresis = <3000>; /* ms */
>> >> +                                     type = "active";
>> >> +                             };
>> >> +                             cpu_crit0: cpu-crit-0 {
>> >> +                                     temperature = <75000>; /* ms
>> >> */
>> >> +                                     hysteresis = <0>; /* ms */
>> >> +                                     type = "critical";
>> >
>> >                 Is there any special reasons why we need special
>> > values for cpu0_thermal sensor at XU3? Is something wrong with
>> >                 default values defined at exynos4-cpu-thermal.dtsi?
>> >
>> >                 If this is Odroid XU3 specific, then IMHO it should
>> > be added to exynos5422-odroidxu3.dts
>> >
>> >> +                             };
>> >> +                     };
>> >> +                     cooling-maps {
>> >> +                             map0 {
>> >> +                                  trip = <&cpu_alert0>;
>> >> +                                  cooling-device = <&fan0 0 1>;
>> >> +                             };
>> >> +                             map1 {
>> >> +                                  trip = <&cpu_alert1>;
>> >> +                                  cooling-device = <&fan0 1 2>;
>> >> +                             };
>> >> +                             map2 {
>> >> +                                  trip = <&cpu_alert2>;
>> >> +                                  cooling-device = <&fan0 2 3>;
>> >> +                             };
>> >> +                     };
>> >> +             };
>> >> +     };
>> >> +};
>> >> diff --git a/arch/arm/boot/dts/exynos5420.dtsi
>> >> b/arch/arm/boot/dts/exynos5420.dtsi index 6b49f3c..eb0f16c 100644
>> >> --- a/arch/arm/boot/dts/exynos5420.dtsi
>> >> +++ b/arch/arm/boot/dts/exynos5420.dtsi
>> >> @@ -827,10 +827,6 @@
>> >>       };
>> >>
>> >>       thermal-zones {
>> >> -             cpu0_thermal: cpu0-thermal {
>> >> -                     thermal-sensors = <&tmu_cpu0>;
>> >> -                     #include "exynos5420-trip-points.dtsi"
>> >                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [1]
>> >
>> >> -             };
>> >>               cpu1_thermal: cpu1-thermal {
>> >>                      thermal-sensors = <&tmu_cpu1>;
>> >>                      #include "exynos5420-trip-points.dtsi"
>> >> diff --git a/arch/arm/boot/dts/exynos5422-odroidxu3.dts
>> >> b/arch/arm/boot/dts/exynos5422-odroidxu3.dts index eaec006..c8b3e3e
>> >> 100644 --- a/arch/arm/boot/dts/exynos5422-odroidxu3.dts
>> >> +++ b/arch/arm/boot/dts/exynos5422-odroidxu3.dts
>> >> @@ -12,6 +12,7 @@
>> >>
>> >>  /dts-v1/;
>> >>  #include "exynos5800.dtsi"
>> >> +#include "exynos5-cpu-thermal.dtsi"
>> >
>> > I would prefer to stick to convention proposed at [1]. It keeps
>> > the locality of the include with respective thermal zone.
>> >
>> >>
>> >>  / {
>> >>       model = "Hardkernel Odroid XU3";
>> >
>> > DTS changes should be added to Samsung tree by Samsung maintainer -
>> > Mr. Kukjin Kim.
>> >
>> > --
>> > Best regards,
>> >
>> > Lukasz Majewski
>> >
>> > Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
>
>
> --
> Best regards,
>
> Lukasz Majewski
>
> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[PATCH 4/6] ARM: dts: OdroidXU3: Enable TMU at Exynos5422 base.

[Anand Moon]

This commit enables TMU IP block on the Exynos5422 OdroidXU3
device.

Tested on OdroidXU3 board.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 arch/arm/boot/dts/exynos5422-odroidxu3.dts | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5422-odroidxu3.dts b/arch/arm/boot/dts/exynos5422-odroidxu3.dts
index c8b3e3e..e1635b5 100644
--- a/arch/arm/boot/dts/exynos5422-odroidxu3.dts
+++ b/arch/arm/boot/dts/exynos5422-odroidxu3.dts
@@ -288,6 +288,31 @@
 		#cooling-cells = <2>;
 		cooling-levels = <0 90 130 170 230>;
 	};
+
+	tmu@10060000 {
+		vtmu-supply = <&ldo10_reg>;
+		status = "okay";
+	};
+
+	tmu@10064000 {
+		vtmu-supply = <&ldo10_reg>;
+		status = "okay";
+	};
+
+	tmu@10068000 {
+		vtmu-supply = <&ldo10_reg>;
+		status = "okay";
+	};
+
+	tmu@1006c000 {
+		vtmu-supply = <&ldo10_reg>;
+		status = "okay";
+	};
+
+	tmu@100a0000 {
+		vtmu-supply = <&ldo10_reg>;
+		status = "okay";
+	};
 };
 
 &hdmi {
-- 
1.9.1

Re: [PATCH 4/6] ARM: dts: OdroidXU3: Enable TMU at Exynos5422 base.

[Lukasz Majewski]

Hi Anand,

> This commit enables TMU IP block on the Exynos5422 OdroidXU3
> device.
> 
> Tested on OdroidXU3 board.
> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>  arch/arm/boot/dts/exynos5422-odroidxu3.dts | 25
> +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/exynos5422-odroidxu3.dts
> b/arch/arm/boot/dts/exynos5422-odroidxu3.dts index c8b3e3e..e1635b5
> 100644 --- a/arch/arm/boot/dts/exynos5422-odroidxu3.dts
> +++ b/arch/arm/boot/dts/exynos5422-odroidxu3.dts
> @@ -288,6 +288,31 @@
>  		#cooling-cells = <2>;
>  		cooling-levels = <0 90 130 170 230>;
>  	};
> +
> +	tmu@10060000 {
> +		vtmu-supply = <&ldo10_reg>;
> +		status = "okay";
> +	};
> +
> +	tmu@10064000 {
> +		vtmu-supply = <&ldo10_reg>;
> +		status = "okay";
> +	};
> +
> +	tmu@10068000 {
> +		vtmu-supply = <&ldo10_reg>;
> +		status = "okay";
> +	};
> +
> +	tmu@1006c000 {
> +		vtmu-supply = <&ldo10_reg>;
> +		status = "okay";
> +	};
> +
> +	tmu@100a0000 {
> +		vtmu-supply = <&ldo10_reg>;
> +		status = "okay";
> +	};
>  };
>  
>  &hdmi {

This seems correct. However please consider my comment to the previous
patch.

Acked-by: Lukasz Majewski <l.majewski@samsung.com>

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

[PATCH 5/6] pwm: samsung: Fix output race on disabling

[Anand Moon]

From: Sjoerd Simons <sjoerd.simons@collabora.co.uk>

When disabling the samsung PWM the output state remains at the level it
was in the end of a pwm cycle. In other words, calling pwm_disable when
at 100% duty will keep the output active, while at all other setting the
output will go/stay inactive. On top of that the samsung PWM settings are
double-buffered, which means the new settings only get applied at the
start of a new PWM cycle.

This results in a race if the PWM is at 100% duty and a driver calls:
  pwm_config (pwm, 0, period);
  pwm_disable (pwm);

In this case the PWMs output will unexpectedly stay active, unless a new
PWM cycle happened to start between the register writes in _config and
_disable. As far as i can tell this is a regression introduced by 3bdf878,
before that a call to pwm_config would call pwm_samsung_enable which,
while heavy-handed, made sure the expected settings were live.

To resolve this, while not re-introducing the issues 3bdf878 (flickering
as the PWM got reset while in a PWM cycle). Only force an update of the
settings when at 100% duty, which shouldn't have a noticeable effect on
the output but is enough to ensure the behaviour is as expected on
disable.

Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 drivers/pwm/pwm-samsung.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c
index 3e9b583..649f6c4 100644
--- a/drivers/pwm/pwm-samsung.c
+++ b/drivers/pwm/pwm-samsung.c
@@ -269,12 +269,31 @@ static void pwm_samsung_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	spin_unlock_irqrestore(&samsung_pwm_lock, flags);
 }
 
+static void pwm_samsung_manual_update(struct samsung_pwm_chip *chip,
+				      struct pwm_device *pwm)
+{
+	unsigned int tcon_chan = to_tcon_channel(pwm->hwpwm);
+	u32 tcon;
+	unsigned long flags;
+
+	spin_lock_irqsave(&samsung_pwm_lock, flags);
+
+	tcon = readl(chip->base + REG_TCON);
+	tcon |= TCON_MANUALUPDATE(tcon_chan);
+	writel(tcon, chip->base + REG_TCON);
+
+	tcon &= ~TCON_MANUALUPDATE(tcon_chan);
+	writel(tcon, chip->base + REG_TCON);
+
+	spin_unlock_irqrestore(&samsung_pwm_lock, flags);
+}
+
 static int pwm_samsung_config(struct pwm_chip *chip, struct pwm_device *pwm,
 			      int duty_ns, int period_ns)
 {
 	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
 	struct samsung_pwm_channel *chan = pwm_get_chip_data(pwm);
-	u32 tin_ns = chan->tin_ns, tcnt, tcmp;
+	u32 tin_ns = chan->tin_ns, tcnt, tcmp, oldtcmp;
 
 	/*
 	 * We currently avoid using 64bit arithmetic by using the
@@ -288,6 +307,7 @@ static int pwm_samsung_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		return 0;
 
 	tcnt = readl(our_chip->base + REG_TCNTB(pwm->hwpwm));
+	oldtcmp = readl(our_chip->base + REG_TCMPB(pwm->hwpwm));
 
 	/* We need tick count for calculation, not last tick. */
 	++tcnt;
@@ -335,6 +355,15 @@ static int pwm_samsung_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	writel(tcnt, our_chip->base + REG_TCNTB(pwm->hwpwm));
 	writel(tcmp, our_chip->base + REG_TCMPB(pwm->hwpwm));
 
+	/* In case the PWM is currently at 100% duty, force a manual update
+	 * to prevent the signal staying high in the pwm is disabled shortly
+	 * afer this update (before it autoreloaded the new values) .
+	 */
+	if (oldtcmp == (u32) -1) {
+		dev_dbg(our_chip->chip.dev, "Forcing manual update");
+		pwm_samsung_manual_update(our_chip, pwm);
+	}
+
 	chan->period_ns = period_ns;
 	chan->tin_ns = tin_ns;
 	chan->duty_ns = duty_ns;
-- 
1.9.1

Re: [PATCH 5/6] pwm: samsung: Fix output race on disabling

[Lukasz Majewski]

Hi Anand,

> From: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> 
> When disabling the samsung PWM the output state remains at the level
> it was in the end of a pwm cycle. In other words, calling pwm_disable
> when at 100% duty will keep the output active, while at all other
> setting the output will go/stay inactive. On top of that the samsung
> PWM settings are double-buffered, which means the new settings only
> get applied at the start of a new PWM cycle.
> 
> This results in a race if the PWM is at 100% duty and a driver calls:
>   pwm_config (pwm, 0, period);
>   pwm_disable (pwm);
> 
> In this case the PWMs output will unexpectedly stay active, unless a
> new PWM cycle happened to start between the register writes in
> _config and _disable. As far as i can tell this is a regression
> introduced by 3bdf878, before that a call to pwm_config would call
> pwm_samsung_enable which, while heavy-handed, made sure the expected
> settings were live.
> 
> To resolve this, while not re-introducing the issues 3bdf878
> (flickering as the PWM got reset while in a PWM cycle). Only force an
> update of the settings when at 100% duty, which shouldn't have a
> noticeable effect on the output but is enough to ensure the behaviour
> is as expected on disable.
> 
> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>  drivers/pwm/pwm-samsung.c | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c
> index 3e9b583..649f6c4 100644
> --- a/drivers/pwm/pwm-samsung.c
> +++ b/drivers/pwm/pwm-samsung.c
> @@ -269,12 +269,31 @@ static void pwm_samsung_disable(struct pwm_chip
> *chip, struct pwm_device *pwm)
> spin_unlock_irqrestore(&samsung_pwm_lock, flags); }
>  
> +static void pwm_samsung_manual_update(struct samsung_pwm_chip *chip,
> +				      struct pwm_device *pwm)
> +{
> +	unsigned int tcon_chan = to_tcon_channel(pwm->hwpwm);
> +	u32 tcon;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&samsung_pwm_lock, flags);
> +
> +	tcon = readl(chip->base + REG_TCON);
> +	tcon |= TCON_MANUALUPDATE(tcon_chan);
> +	writel(tcon, chip->base + REG_TCON);
> +
> +	tcon &= ~TCON_MANUALUPDATE(tcon_chan);
> +	writel(tcon, chip->base + REG_TCON);
> +
> +	spin_unlock_irqrestore(&samsung_pwm_lock, flags);
> +}
> +
>  static int pwm_samsung_config(struct pwm_chip *chip, struct
> pwm_device *pwm, int duty_ns, int period_ns)
>  {
>  	struct samsung_pwm_chip *our_chip =
> to_samsung_pwm_chip(chip); struct samsung_pwm_channel *chan =
> pwm_get_chip_data(pwm);
> -	u32 tin_ns = chan->tin_ns, tcnt, tcmp;
> +	u32 tin_ns = chan->tin_ns, tcnt, tcmp, oldtcmp;
>  
>  	/*
>  	 * We currently avoid using 64bit arithmetic by using the
> @@ -288,6 +307,7 @@ static int pwm_samsung_config(struct pwm_chip
> *chip, struct pwm_device *pwm, return 0;
>  
>  	tcnt = readl(our_chip->base + REG_TCNTB(pwm->hwpwm));
> +	oldtcmp = readl(our_chip->base + REG_TCMPB(pwm->hwpwm));
>  
>  	/* We need tick count for calculation, not last tick. */
>  	++tcnt;
> @@ -335,6 +355,15 @@ static int pwm_samsung_config(struct pwm_chip
> *chip, struct pwm_device *pwm, writel(tcnt, our_chip->base +
> REG_TCNTB(pwm->hwpwm)); writel(tcmp, our_chip->base +
> REG_TCMPB(pwm->hwpwm)); 
> +	/* In case the PWM is currently at 100% duty, force a manual
> update
	
	Cosmetic comment:

	Wasn't checkpatch complaining about this comment style?
	/* .....
         * .....

	instead of:
	/*
	 * .....
	 * .....

> +	 * to prevent the signal staying high in the pwm is disabled
> shortly
> +	 * afer this update (before it autoreloaded the new values) .
> +	 */
> +	if (oldtcmp == (u32) -1) {
> +		dev_dbg(our_chip->chip.dev, "Forcing manual update");
> +		pwm_samsung_manual_update(our_chip, pwm);
> +	}
> +
>  	chan->period_ns = period_ns;
>  	chan->tin_ns = tin_ns;
>  	chan->duty_ns = duty_ns;

Despite the above,

Acked-by: Lukasz Majewski <l.majewski@samsung.com>

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH 5/6] pwm: samsung: Fix output race on disabling

[Sjoerd Simons]

On Wed, 2015-04-08 at 10:28 +0200, Lukasz Majewski wrote:
> Hi Anand,
> 
> > From: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> > When disabling the samsung PWM the output state remains at the level
> > it was in the end of a pwm cycle. In other words, calling pwm_disable
> > when at 100% duty will keep the output active, while at all other
> > setting the output will go/stay inactive. On top of that the samsung
> > PWM settings are double-buffered, which means the new settings only
> > get applied at the start of a new PWM cycle.

This patch is already in the linux-pwm for-next tree so should probably
be dropped form this patchset to prevent conflicts.

-- 
Sjoerd Simons <sjoerd.simons@collabora.co.uk>
Collabora Ltd.

Re: [PATCH 5/6] pwm: samsung: Fix output race on disabling

[Anand Moon]

Hi Sjoerd,

Correct. Will do so. I just included in this series. As it relevant to
my changes and testing.

-Anand Moon

On 8 April 2015 at 14:12, Sjoerd Simons <sjoerd.simons@collabora.co.uk> wrote:
>
>
> On Wed, 2015-04-08 at 10:28 +0200, Lukasz Majewski wrote:
>> Hi Anand,
>>
>> > From: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
>> > When disabling the samsung PWM the output state remains at the level
>> > it was in the end of a pwm cycle. In other words, calling pwm_disable
>> > when at 100% duty will keep the output active, while at all other
>> > setting the output will go/stay inactive. On top of that the samsung
>> > PWM settings are double-buffered, which means the new settings only
>> > get applied at the start of a new PWM cycle.
>
> This patch is already in the linux-pwm for-next tree so should probably
> be dropped form this patchset to prevent conflicts.
>
> --
> Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> Collabora Ltd.

[PATCH 6/6] hwmon: pwm-fan: Update the duty cycle inorder to control the pwm-fan

[Anand Moon]

Below changes depend on following patch.
https://patchwork.kernel.org/patch/5944061/

Update the pwm_config with duty then update the pwm_disable
to poweroff the cpu fan.

Tested on OdroidXU3 board.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 drivers/hwmon/pwm-fan.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 7c83dc4..f25c841 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -44,26 +44,24 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
 	int ret = 0;
 
 	mutex_lock(&ctx->lock);
+
 	if (ctx->pwm_value == pwm)
 		goto exit_set_pwm_err;
 
-	if (pwm == 0) {
-		pwm_disable(ctx->pwm);
-		goto exit_set_pwm;
-	}
-
 	duty = DIV_ROUND_UP(pwm * (ctx->pwm->period - 1), MAX_PWM);
 	ret = pwm_config(ctx->pwm, duty, ctx->pwm->period);
 	if (ret)
 		goto exit_set_pwm_err;
 
+	if (pwm == 0)
+		pwm_disable(ctx->pwm);
+
 	if (ctx->pwm_value == 0) {
 		ret = pwm_enable(ctx->pwm);
 		if (ret)
 			goto exit_set_pwm_err;
 	}
 
-exit_set_pwm:
 	ctx->pwm_value = pwm;
 exit_set_pwm_err:
 	mutex_unlock(&ctx->lock);
-- 
1.9.1

Re: [PATCH 6/6] hwmon: pwm-fan: Update the duty cycle inorder to control the pwm-fan

[Lukasz Majewski]

Hi Anand,

> Below changes depend on following patch.
> https://patchwork.kernel.org/patch/5944061/
> 
> Update the pwm_config with duty then update the pwm_disable
> to poweroff the cpu fan.
> 
> Tested on OdroidXU3 board.
> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>  drivers/hwmon/pwm-fan.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index 7c83dc4..f25c841 100644
> --- a/drivers/hwmon/pwm-fan.c
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -44,26 +44,24 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx,
> unsigned long pwm) int ret = 0;
>  
>  	mutex_lock(&ctx->lock);
> +
>  	if (ctx->pwm_value == pwm)
>  		goto exit_set_pwm_err;
>  
> -	if (pwm == 0) {
> -		pwm_disable(ctx->pwm);
> -		goto exit_set_pwm;
> -	}
> -
>  	duty = DIV_ROUND_UP(pwm * (ctx->pwm->period - 1), MAX_PWM);
>  	ret = pwm_config(ctx->pwm, duty, ctx->pwm->period);
>  	if (ret)
>  		goto exit_set_pwm_err;
>  
> +	if (pwm == 0)
> +		pwm_disable(ctx->pwm);
> +
>  	if (ctx->pwm_value == 0) {
>  		ret = pwm_enable(ctx->pwm);
>  		if (ret)
>  			goto exit_set_pwm_err;
>  	}
>  
> -exit_set_pwm:
>  	ctx->pwm_value = pwm;
>  exit_set_pwm_err:
>  	mutex_unlock(&ctx->lock);

Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>

BTW: I've added Guenter to CC.

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH 6/6] hwmon: pwm-fan: Update the duty cycle inorder to control the pwm-fan

[Guenter Roeck]

On 04/08/2015 01:44 AM, Lukasz Majewski wrote:
> Hi Anand,
>
>> Below changes depend on following patch.
>> https://patchwork.kernel.org/patch/5944061/
>>
>> Update the pwm_config with duty then update the pwm_disable
>> to poweroff the cpu fan.
>>
>> Tested on OdroidXU3 board.
>>
>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>> ---
>>   drivers/hwmon/pwm-fan.c | 10 ++++------
>>   1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
>> index 7c83dc4..f25c841 100644
>> --- a/drivers/hwmon/pwm-fan.c
>> +++ b/drivers/hwmon/pwm-fan.c
>> @@ -44,26 +44,24 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx,
>> unsigned long pwm) int ret = 0;
>>
>>   	mutex_lock(&ctx->lock);
>> +
>>   	if (ctx->pwm_value == pwm)
>>   		goto exit_set_pwm_err;
>>
>> -	if (pwm == 0) {
>> -		pwm_disable(ctx->pwm);
>> -		goto exit_set_pwm;
>> -	}
>> -
>>   	duty = DIV_ROUND_UP(pwm * (ctx->pwm->period - 1), MAX_PWM);
>>   	ret = pwm_config(ctx->pwm, duty, ctx->pwm->period);
>>   	if (ret)
>>   		goto exit_set_pwm_err;
>>
>> +	if (pwm == 0)
>> +		pwm_disable(ctx->pwm);
>> +
>>   	if (ctx->pwm_value == 0) {
>>   		ret = pwm_enable(ctx->pwm);
>>   		if (ret)
>>   			goto exit_set_pwm_err;
>>   	}
>>
>> -exit_set_pwm:
>>   	ctx->pwm_value = pwm;
>>   exit_set_pwm_err:
>>   	mutex_unlock(&ctx->lock);
>
> Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>
>
> BTW: I've added Guenter to CC.
>
I don't have a copy of the original patch, so I can't apply it.

Guenter

Re: [PATCH 6/6] hwmon: pwm-fan: Update the duty cycle inorder to control the pwm-fan

[Guenter Roeck]

On Wed, Apr 08, 2015 at 10:44:15AM +0200, Lukasz Majewski wrote:
> Hi Anand,
> 
> > Below changes depend on following patch.
> > https://patchwork.kernel.org/patch/5944061/
> > 
> > Update the pwm_config with duty then update the pwm_disable
> > to poweroff the cpu fan.
> > 

Unfortunately, the patch does not include an explanation why it is needed.

The original code presumably did not update the duty cycle because
pwm was about to be disabled anyway. That kind of made sense to me.
Updating the duty cycle to 0 just to disable the pwm channel right
afterwards does not immediately make sense.

Given that, I would expect to see a rationale here. Why is this patch needed ?
Does it fix a bug ? If yes, pelase describe the bug. If not, what is the
purpose of this patch ?

Maybe that is all explained in patch 0/6, which I was not copied on. Even
if so, the reationale will be needed in the changelog to explain to future
developers why this change was made.

Thanks,
Guenter

> > Tested on OdroidXU3 board.
> > 
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> >  drivers/hwmon/pwm-fan.c | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> > index 7c83dc4..f25c841 100644
> > --- a/drivers/hwmon/pwm-fan.c
> > +++ b/drivers/hwmon/pwm-fan.c
> > @@ -44,26 +44,24 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx,
> > unsigned long pwm) int ret = 0;
> >  
> >  	mutex_lock(&ctx->lock);
> > +

[ please refrain from unnecessary whitespace changes ]

> >  	if (ctx->pwm_value == pwm)
> >  		goto exit_set_pwm_err;
> >  
> > -	if (pwm == 0) {
> > -		pwm_disable(ctx->pwm);
> > -		goto exit_set_pwm;
> > -	}
> > -
> >  	duty = DIV_ROUND_UP(pwm * (ctx->pwm->period - 1), MAX_PWM);
> >  	ret = pwm_config(ctx->pwm, duty, ctx->pwm->period);
> >  	if (ret)
> >  		goto exit_set_pwm_err;
> >  
> > +	if (pwm == 0)
> > +		pwm_disable(ctx->pwm);
> > +
> >  	if (ctx->pwm_value == 0) {
> >  		ret = pwm_enable(ctx->pwm);
> >  		if (ret)
> >  			goto exit_set_pwm_err;
> >  	}
> >  
> > -exit_set_pwm:
> >  	ctx->pwm_value = pwm;
> >  exit_set_pwm_err:
> >  	mutex_unlock(&ctx->lock);
> 
> Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>
> 
> BTW: I've added Guenter to CC.
> 
> -- 
> Best regards,
> 
> Lukasz Majewski
> 
> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH 6/6] hwmon: pwm-fan: Update the duty cycle inorder to control the pwm-fan

[Anand Moon]

Hi Guenter,

Initially the board bootup the cooling level state is 0.
So update the duty cycle and this power off the fan.
As their is no state change the fan will not spin.

Once the temperature sensor is reached to alert temperature it changes state.
With the state change the fan cools the CPU and then stop's

I have observed this state change with tmon utility in linux/tools/thermal/tmon/

-Anand Moon

On 8 April 2015 at 21:02, Guenter Roeck <linux@roeck-us.net> wrote:
> On Wed, Apr 08, 2015 at 10:44:15AM +0200, Lukasz Majewski wrote:
>> Hi Anand,
>>
>> > Below changes depend on following patch.
>> > https://patchwork.kernel.org/patch/5944061/
>> >
>> > Update the pwm_config with duty then update the pwm_disable
>> > to poweroff the cpu fan.
>> >
>
> Unfortunately, the patch does not include an explanation why it is needed.
>
> The original code presumably did not update the duty cycle because
> pwm was about to be disabled anyway. That kind of made sense to me.
> Updating the duty cycle to 0 just to disable the pwm channel right
> afterwards does not immediately make sense.
>
> Given that, I would expect to see a rationale here. Why is this patch needed ?
> Does it fix a bug ? If yes, pelase describe the bug. If not, what is the
> purpose of this patch ?
>
> Maybe that is all explained in patch 0/6, which I was not copied on. Even
> if so, the reationale will be needed in the changelog to explain to future
> developers why this change was made.
>
> Thanks,
> Guenter
>
>> > Tested on OdroidXU3 board.
>> >
>> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>> > ---
>> >  drivers/hwmon/pwm-fan.c | 10 ++++------
>> >  1 file changed, 4 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
>> > index 7c83dc4..f25c841 100644
>> > --- a/drivers/hwmon/pwm-fan.c
>> > +++ b/drivers/hwmon/pwm-fan.c
>> > @@ -44,26 +44,24 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx,
>> > unsigned long pwm) int ret = 0;
>> >
>> >     mutex_lock(&ctx->lock);
>> > +
>
> [ please refrain from unnecessary whitespace changes ]
>
>> >     if (ctx->pwm_value == pwm)
>> >             goto exit_set_pwm_err;
>> >
>> > -   if (pwm == 0) {
>> > -           pwm_disable(ctx->pwm);
>> > -           goto exit_set_pwm;
>> > -   }
>> > -
>> >     duty = DIV_ROUND_UP(pwm * (ctx->pwm->period - 1), MAX_PWM);
>> >     ret = pwm_config(ctx->pwm, duty, ctx->pwm->period);
>> >     if (ret)
>> >             goto exit_set_pwm_err;
>> >
>> > +   if (pwm == 0)
>> > +           pwm_disable(ctx->pwm);
>> > +
>> >     if (ctx->pwm_value == 0) {
>> >             ret = pwm_enable(ctx->pwm);
>> >             if (ret)
>> >                     goto exit_set_pwm_err;
>> >     }
>> >
>> > -exit_set_pwm:
>> >     ctx->pwm_value = pwm;
>> >  exit_set_pwm_err:
>> >     mutex_unlock(&ctx->lock);
>>
>> Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>
>>
>> BTW: I've added Guenter to CC.
>>
>> --
>> Best regards,
>>
>> Lukasz Majewski
>>
>> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH 6/6] hwmon: pwm-fan: Update the duty cycle inorder to control the pwm-fan

[Guenter Roeck]

On Wed, Apr 08, 2015 at 09:32:05PM +0530, Anand Moon wrote:
> Hi Guenter,
> 
> Initially the board bootup the cooling level state is 0.
> So update the duty cycle and this power off the fan.
> As their is no state change the fan will not spin.
> 
> Once the temperature sensor is reached to alert temperature it changes state.
> With the state change the fan cools the CPU and then stop's
> 
> I have observed this state change with tmon utility in linux/tools/thermal/tmon/
> 
Sorry, I am missing something. I still don't see what problem you are fixing
with this patch. What behavior is wrong with the current code, and how does your
patch fix it ?

Guenter

> -Anand Moon
> 
> On 8 April 2015 at 21:02, Guenter Roeck <linux@roeck-us.net> wrote:
> > On Wed, Apr 08, 2015 at 10:44:15AM +0200, Lukasz Majewski wrote:
> >> Hi Anand,
> >>
> >> > Below changes depend on following patch.
> >> > https://patchwork.kernel.org/patch/5944061/
> >> >
> >> > Update the pwm_config with duty then update the pwm_disable
> >> > to poweroff the cpu fan.
> >> >
> >
> > Unfortunately, the patch does not include an explanation why it is needed.
> >
> > The original code presumably did not update the duty cycle because
> > pwm was about to be disabled anyway. That kind of made sense to me.
> > Updating the duty cycle to 0 just to disable the pwm channel right
> > afterwards does not immediately make sense.
> >
> > Given that, I would expect to see a rationale here. Why is this patch needed ?
> > Does it fix a bug ? If yes, pelase describe the bug. If not, what is the
> > purpose of this patch ?
> >
> > Maybe that is all explained in patch 0/6, which I was not copied on. Even
> > if so, the reationale will be needed in the changelog to explain to future
> > developers why this change was made.
> >
> > Thanks,
> > Guenter
> >
> >> > Tested on OdroidXU3 board.
> >> >
> >> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> >> > ---
> >> >  drivers/hwmon/pwm-fan.c | 10 ++++------
> >> >  1 file changed, 4 insertions(+), 6 deletions(-)
> >> >
> >> > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> >> > index 7c83dc4..f25c841 100644
> >> > --- a/drivers/hwmon/pwm-fan.c
> >> > +++ b/drivers/hwmon/pwm-fan.c
> >> > @@ -44,26 +44,24 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx,
> >> > unsigned long pwm) int ret = 0;
> >> >
> >> >     mutex_lock(&ctx->lock);
> >> > +
> >
> > [ please refrain from unnecessary whitespace changes ]
> >
> >> >     if (ctx->pwm_value == pwm)
> >> >             goto exit_set_pwm_err;
> >> >
> >> > -   if (pwm == 0) {
> >> > -           pwm_disable(ctx->pwm);
> >> > -           goto exit_set_pwm;
> >> > -   }
> >> > -
> >> >     duty = DIV_ROUND_UP(pwm * (ctx->pwm->period - 1), MAX_PWM);
> >> >     ret = pwm_config(ctx->pwm, duty, ctx->pwm->period);
> >> >     if (ret)
> >> >             goto exit_set_pwm_err;
> >> >
> >> > +   if (pwm == 0)
> >> > +           pwm_disable(ctx->pwm);
> >> > +
> >> >     if (ctx->pwm_value == 0) {
> >> >             ret = pwm_enable(ctx->pwm);
> >> >             if (ret)
> >> >                     goto exit_set_pwm_err;
> >> >     }
> >> >
> >> > -exit_set_pwm:
> >> >     ctx->pwm_value = pwm;
> >> >  exit_set_pwm_err:
> >> >     mutex_unlock(&ctx->lock);
> >>
> >> Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>
> >>
> >> BTW: I've added Guenter to CC.
> >>
> >> --
> >> Best regards,
> >>
> >> Lukasz Majewski
> >>
> >> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH 6/6] hwmon: pwm-fan: Update the duty cycle inorder to control the pwm-fan

[Anand Moon]

Hi Guenter,

Sorry my blunder mistake. Sorry for the noise.

I just tested with spiking this patch and my observation and testing
were wrong we can skip this patch.

I will send an v2 patch series removing the patch 5 and patch 6.

With correct dts changes.

Thanks for pointing my mistake.

-Anand Moon

On 8 April 2015 at 22:23, Guenter Roeck <linux@roeck-us.net> wrote:
> On Wed, Apr 08, 2015 at 09:32:05PM +0530, Anand Moon wrote:
>> Hi Guenter,
>>
>> Initially the board bootup the cooling level state is 0.
>> So update the duty cycle and this power off the fan.
>> As their is no state change the fan will not spin.
>>
>> Once the temperature sensor is reached to alert temperature it changes state.
>> With the state change the fan cools the CPU and then stop's
>>
>> I have observed this state change with tmon utility in linux/tools/thermal/tmon/
>>
> Sorry, I am missing something. I still don't see what problem you are fixing
> with this patch. What behavior is wrong with the current code, and how does your
> patch fix it ?
>
> Guenter
>
>> -Anand Moon
>>
>> On 8 April 2015 at 21:02, Guenter Roeck <linux@roeck-us.net> wrote:
>> > On Wed, Apr 08, 2015 at 10:44:15AM +0200, Lukasz Majewski wrote:
>> >> Hi Anand,
>> >>
>> >> > Below changes depend on following patch.
>> >> > https://patchwork.kernel.org/patch/5944061/
>> >> >
>> >> > Update the pwm_config with duty then update the pwm_disable
>> >> > to poweroff the cpu fan.
>> >> >
>> >
>> > Unfortunately, the patch does not include an explanation why it is needed.
>> >
>> > The original code presumably did not update the duty cycle because
>> > pwm was about to be disabled anyway. That kind of made sense to me.
>> > Updating the duty cycle to 0 just to disable the pwm channel right
>> > afterwards does not immediately make sense.
>> >
>> > Given that, I would expect to see a rationale here. Why is this patch needed ?
>> > Does it fix a bug ? If yes, pelase describe the bug. If not, what is the
>> > purpose of this patch ?
>> >
>> > Maybe that is all explained in patch 0/6, which I was not copied on. Even
>> > if so, the reationale will be needed in the changelog to explain to future
>> > developers why this change was made.
>> >
>> > Thanks,
>> > Guenter
>> >
>> >> > Tested on OdroidXU3 board.
>> >> >
>> >> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>> >> > ---
>> >> >  drivers/hwmon/pwm-fan.c | 10 ++++------
>> >> >  1 file changed, 4 insertions(+), 6 deletions(-)
>> >> >
>> >> > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
>> >> > index 7c83dc4..f25c841 100644
>> >> > --- a/drivers/hwmon/pwm-fan.c
>> >> > +++ b/drivers/hwmon/pwm-fan.c
>> >> > @@ -44,26 +44,24 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx,
>> >> > unsigned long pwm) int ret = 0;
>> >> >
>> >> >     mutex_lock(&ctx->lock);
>> >> > +
>> >
>> > [ please refrain from unnecessary whitespace changes ]
>> >
>> >> >     if (ctx->pwm_value == pwm)
>> >> >             goto exit_set_pwm_err;
>> >> >
>> >> > -   if (pwm == 0) {
>> >> > -           pwm_disable(ctx->pwm);
>> >> > -           goto exit_set_pwm;
>> >> > -   }
>> >> > -
>> >> >     duty = DIV_ROUND_UP(pwm * (ctx->pwm->period - 1), MAX_PWM);
>> >> >     ret = pwm_config(ctx->pwm, duty, ctx->pwm->period);
>> >> >     if (ret)
>> >> >             goto exit_set_pwm_err;
>> >> >
>> >> > +   if (pwm == 0)
>> >> > +           pwm_disable(ctx->pwm);
>> >> > +
>> >> >     if (ctx->pwm_value == 0) {
>> >> >             ret = pwm_enable(ctx->pwm);
>> >> >             if (ret)
>> >> >                     goto exit_set_pwm_err;
>> >> >     }
>> >> >
>> >> > -exit_set_pwm:
>> >> >     ctx->pwm_value = pwm;
>> >> >  exit_set_pwm_err:
>> >> >     mutex_unlock(&ctx->lock);
>> >>
>> >> Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>
>> >>
>> >> BTW: I've added Guenter to CC.
>> >>
>> >> --
>> >> Best regards,
>> >>
>> >> Lukasz Majewski
>> >>
>> >> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH 6/6] hwmon: pwm-fan: Update the duty cycle inorder to control the pwm-fan

[Anand Moon]

Hi Guenter/Lukasz,

Earlier I send v2 version of the patch spiking this one.

Markus Riechl came back to me with below mail.
So This patch confirms fixes the bug.

I will send v3 version of the patch. Earlier I was in delima about the bug.

-Anand Moon
-------------------------------------------
Hi Anand,

I tested your patch.

After booting the fan is spinning despite only 44°C.

/sys/class/thermal/cooling_device0/curstate is 0.
/sys/class/hwmon/hwmon4/pwm1 is 0

when I echo 1 > cur_state and then echo 0 > cur_state again,
the fan switches to off and behaves as expected.

It looks like there is a bug in initializing the pwm output
immediately after booting.

Best Regards,

--
Markus Reichl

On 8 April 2015 at 23:19, Anand Moon <linux.amoon@gmail.com> wrote:
> Hi Guenter,
>
> Sorry my blunder mistake. Sorry for the noise.
>
> I just tested with spiking this patch and my observation and testing
> were wrong we can skip this patch.
>
> I will send an v2 patch series removing the patch 5 and patch 6.
>
> With correct dts changes.
>
> Thanks for pointing my mistake.
>
> -Anand Moon
>
> On 8 April 2015 at 22:23, Guenter Roeck <linux@roeck-us.net> wrote:
>> On Wed, Apr 08, 2015 at 09:32:05PM +0530, Anand Moon wrote:
>>> Hi Guenter,
>>>
>>> Initially the board bootup the cooling level state is 0.
>>> So update the duty cycle and this power off the fan.
>>> As their is no state change the fan will not spin.
>>>
>>> Once the temperature sensor is reached to alert temperature it changes state.
>>> With the state change the fan cools the CPU and then stop's
>>>
>>> I have observed this state change with tmon utility in linux/tools/thermal/tmon/
>>>
>> Sorry, I am missing something. I still don't see what problem you are fixing
>> with this patch. What behavior is wrong with the current code, and how does your
>> patch fix it ?
>>
>> Guenter
>>
>>> -Anand Moon
>>>
>>> On 8 April 2015 at 21:02, Guenter Roeck <linux@roeck-us.net> wrote:
>>> > On Wed, Apr 08, 2015 at 10:44:15AM +0200, Lukasz Majewski wrote:
>>> >> Hi Anand,
>>> >>
>>> >> > Below changes depend on following patch.
>>> >> > https://patchwork.kernel.org/patch/5944061/
>>> >> >
>>> >> > Update the pwm_config with duty then update the pwm_disable
>>> >> > to poweroff the cpu fan.
>>> >> >
>>> >
>>> > Unfortunately, the patch does not include an explanation why it is needed.
>>> >
>>> > The original code presumably did not update the duty cycle because
>>> > pwm was about to be disabled anyway. That kind of made sense to me.
>>> > Updating the duty cycle to 0 just to disable the pwm channel right
>>> > afterwards does not immediately make sense.
>>> >
>>> > Given that, I would expect to see a rationale here. Why is this patch needed ?
>>> > Does it fix a bug ? If yes, pelase describe the bug. If not, what is the
>>> > purpose of this patch ?
>>> >
>>> > Maybe that is all explained in patch 0/6, which I was not copied on. Even
>>> > if so, the reationale will be needed in the changelog to explain to future
>>> > developers why this change was made.
>>> >
>>> > Thanks,
>>> > Guenter
>>> >
>>> >> > Tested on OdroidXU3 board.
>>> >> >
>>> >> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>> >> > ---
>>> >> >  drivers/hwmon/pwm-fan.c | 10 ++++------
>>> >> >  1 file changed, 4 insertions(+), 6 deletions(-)
>>> >> >
>>> >> > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
>>> >> > index 7c83dc4..f25c841 100644
>>> >> > --- a/drivers/hwmon/pwm-fan.c
>>> >> > +++ b/drivers/hwmon/pwm-fan.c
>>> >> > @@ -44,26 +44,24 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx,
>>> >> > unsigned long pwm) int ret = 0;
>>> >> >
>>> >> >     mutex_lock(&ctx->lock);
>>> >> > +
>>> >
>>> > [ please refrain from unnecessary whitespace changes ]
>>> >
>>> >> >     if (ctx->pwm_value == pwm)
>>> >> >             goto exit_set_pwm_err;
>>> >> >
>>> >> > -   if (pwm == 0) {
>>> >> > -           pwm_disable(ctx->pwm);
>>> >> > -           goto exit_set_pwm;
>>> >> > -   }
>>> >> > -
>>> >> >     duty = DIV_ROUND_UP(pwm * (ctx->pwm->period - 1), MAX_PWM);
>>> >> >     ret = pwm_config(ctx->pwm, duty, ctx->pwm->period);
>>> >> >     if (ret)
>>> >> >             goto exit_set_pwm_err;
>>> >> >
>>> >> > +   if (pwm == 0)
>>> >> > +           pwm_disable(ctx->pwm);
>>> >> > +
>>> >> >     if (ctx->pwm_value == 0) {
>>> >> >             ret = pwm_enable(ctx->pwm);
>>> >> >             if (ret)
>>> >> >                     goto exit_set_pwm_err;
>>> >> >     }
>>> >> >
>>> >> > -exit_set_pwm:
>>> >> >     ctx->pwm_value = pwm;
>>> >> >  exit_set_pwm_err:
>>> >> >     mutex_unlock(&ctx->lock);
>>> >>
>>> >> Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>
>>> >>
>>> >> BTW: I've added Guenter to CC.
>>> >>
>>> >> --
>>> >> Best regards,
>>> >>
>>> >> Lukasz Majewski
>>> >>
>>> >> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH 6/6] hwmon: pwm-fan: Update the duty cycle inorder to control the pwm-fan

[Sjoerd Simons]

Hey Anand,

On Fri, 2015-04-10 at 16:58 +0530, Anand Moon wrote:
> Hi Guenter/Lukasz,
> 
> Earlier I send v2 version of the patch spiking this one.
> 
> Markus Riechl came back to me with below mail.
> So This patch confirms fixes the bug.
> 
> I will send v3 version of the patch. Earlier I was in delima about the bug.
> 
> -Anand Moon
> -------------------------------------------
> Hi Anand,
> 
> I tested your patch.
> 
> After booting the fan is spinning despite only 44°C.
> 
> /sys/class/thermal/cooling_device0/curstate is 0.
> /sys/class/hwmon/hwmon4/pwm1 is 0
> 
> when I echo 1 > cur_state and then echo 0 > cur_state again,
> the fan switches to off and behaves as expected.
> 
> It looks like there is a bug in initializing the pwm output
> immediately after booting.

The problem here will be that at boot the PWM runs at full duty. With
the current exynos PWM drive if you disable the PWM it will stop pulsing
but remain high if it was at 100% duty. My patch on which you depend
upon fixed a race  where disabling the pwm right after changing the duty
cycle (e.g. to 0%) also kept the signal high. 

From looking at other PWM users at the time it seemed that most if not
all always first set to duty to 0% and then disable the pwm. Which
should work fine on exynos now. However iirc Thierry recently clarified
that the expected result of pwm_disable is not just that the modulation
stops but also that the output signal goes low, although that's not very
explicit in the current pwm documentation.. The exynos PWM driver will
need another fix tweak to make that true.



> Best Regards,



> --
> Markus Reichl
> 
> On 8 April 2015 at 23:19, Anand Moon <linux.amoon@gmail.com> wrote:
> > Hi Guenter,
> >
> > Sorry my blunder mistake. Sorry for the noise.
> >
> > I just tested with spiking this patch and my observation and testing
> > were wrong we can skip this patch.
> >
> > I will send an v2 patch series removing the patch 5 and patch 6.
> >
> > With correct dts changes.
> >
> > Thanks for pointing my mistake.
> >
> > -Anand Moon
> >
> > On 8 April 2015 at 22:23, Guenter Roeck <linux@roeck-us.net> wrote:
> >> On Wed, Apr 08, 2015 at 09:32:05PM +0530, Anand Moon wrote:
> >>> Hi Guenter,
> >>>
> >>> Initially the board bootup the cooling level state is 0.
> >>> So update the duty cycle and this power off the fan.
> >>> As their is no state change the fan will not spin.
> >>>
> >>> Once the temperature sensor is reached to alert temperature it changes state.
> >>> With the state change the fan cools the CPU and then stop's
> >>>
> >>> I have observed this state change with tmon utility in linux/tools/thermal/tmon/
> >>>
> >> Sorry, I am missing something. I still don't see what problem you are fixing
> >> with this patch. What behavior is wrong with the current code, and how does your
> >> patch fix it ?
> >>
> >> Guenter
> >>
> >>> -Anand Moon
> >>>
> >>> On 8 April 2015 at 21:02, Guenter Roeck <linux@roeck-us.net> wrote:
> >>> > On Wed, Apr 08, 2015 at 10:44:15AM +0200, Lukasz Majewski wrote:
> >>> >> Hi Anand,
> >>> >>
> >>> >> > Below changes depend on following patch.
> >>> >> > https://patchwork.kernel.org/patch/5944061/
> >>> >> >
> >>> >> > Update the pwm_config with duty then update the pwm_disable
> >>> >> > to poweroff the cpu fan.
> >>> >> >
> >>> >
> >>> > Unfortunately, the patch does not include an explanation why it is needed.
> >>> >
> >>> > The original code presumably did not update the duty cycle because
> >>> > pwm was about to be disabled anyway. That kind of made sense to me.
> >>> > Updating the duty cycle to 0 just to disable the pwm channel right
> >>> > afterwards does not immediately make sense.
> >>> >
> >>> > Given that, I would expect to see a rationale here. Why is this patch needed ?
> >>> > Does it fix a bug ? If yes, pelase describe the bug. If not, what is the
> >>> > purpose of this patch ?
> >>> >
> >>> > Maybe that is all explained in patch 0/6, which I was not copied on. Even
> >>> > if so, the reationale will be needed in the changelog to explain to future
> >>> > developers why this change was made.
> >>> >
> >>> > Thanks,
> >>> > Guenter
> >>> >
> >>> >> > Tested on OdroidXU3 board.
> >>> >> >
> >>> >> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> >>> >> > ---
> >>> >> >  drivers/hwmon/pwm-fan.c | 10 ++++------
> >>> >> >  1 file changed, 4 insertions(+), 6 deletions(-)
> >>> >> >
> >>> >> > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> >>> >> > index 7c83dc4..f25c841 100644
> >>> >> > --- a/drivers/hwmon/pwm-fan.c
> >>> >> > +++ b/drivers/hwmon/pwm-fan.c
> >>> >> > @@ -44,26 +44,24 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx,
> >>> >> > unsigned long pwm) int ret = 0;
> >>> >> >
> >>> >> >     mutex_lock(&ctx->lock);
> >>> >> > +
> >>> >
> >>> > [ please refrain from unnecessary whitespace changes ]
> >>> >
> >>> >> >     if (ctx->pwm_value == pwm)
> >>> >> >             goto exit_set_pwm_err;
> >>> >> >
> >>> >> > -   if (pwm == 0) {
> >>> >> > -           pwm_disable(ctx->pwm);
> >>> >> > -           goto exit_set_pwm;
> >>> >> > -   }
> >>> >> > -
> >>> >> >     duty = DIV_ROUND_UP(pwm * (ctx->pwm->period - 1), MAX_PWM);
> >>> >> >     ret = pwm_config(ctx->pwm, duty, ctx->pwm->period);
> >>> >> >     if (ret)
> >>> >> >             goto exit_set_pwm_err;
> >>> >> >
> >>> >> > +   if (pwm == 0)
> >>> >> > +           pwm_disable(ctx->pwm);
> >>> >> > +
> >>> >> >     if (ctx->pwm_value == 0) {
> >>> >> >             ret = pwm_enable(ctx->pwm);
> >>> >> >             if (ret)
> >>> >> >                     goto exit_set_pwm_err;
> >>> >> >     }
> >>> >> >
> >>> >> > -exit_set_pwm:
> >>> >> >     ctx->pwm_value = pwm;
> >>> >> >  exit_set_pwm_err:
> >>> >> >     mutex_unlock(&ctx->lock);
> >>> >>
> >>> >> Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>
> >>> >>
> >>> >> BTW: I've added Guenter to CC.
> >>> >>
> >>> >> --
> >>> >> Best regards,
> >>> >>
> >>> >> Lukasz Majewski
> >>> >>
> >>> >> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Re: [PATCH 6/6] hwmon: pwm-fan: Update the duty cycle inorder to control the pwm-fan

[Anand Moon]

Hi Sjoerd,

I don't much advance knowledge on internal signaling of pwm-samsung module.

So do I need to send this patch again ?

-Anand Moon


On 10 April 2015 at 17:30, Sjoerd Simons <sjoerd.simons-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org> wrote:
> Hey Anand,
>
> On Fri, 2015-04-10 at 16:58 +0530, Anand Moon wrote:
>> Hi Guenter/Lukasz,
>>
>> Earlier I send v2 version of the patch spiking this one.
>>
>> Markus Riechl came back to me with below mail.
>> So This patch confirms fixes the bug.
>>
>> I will send v3 version of the patch. Earlier I was in delima about the bug.
>>
>> -Anand Moon
>> -------------------------------------------
>> Hi Anand,
>>
>> I tested your patch.
>>
>> After booting the fan is spinning despite only 44°C.
>>
>> /sys/class/thermal/cooling_device0/curstate is 0.
>> /sys/class/hwmon/hwmon4/pwm1 is 0
>>
>> when I echo 1 > cur_state and then echo 0 > cur_state again,
>> the fan switches to off and behaves as expected.
>>
>> It looks like there is a bug in initializing the pwm output
>> immediately after booting.
>
> The problem here will be that at boot the PWM runs at full duty. With
> the current exynos PWM drive if you disable the PWM it will stop pulsing
> but remain high if it was at 100% duty. My patch on which you depend
> upon fixed a race  where disabling the pwm right after changing the duty
> cycle (e.g. to 0%) also kept the signal high.
>
> From looking at other PWM users at the time it seemed that most if not
> all always first set to duty to 0% and then disable the pwm. Which
> should work fine on exynos now. However iirc Thierry recently clarified
> that the expected result of pwm_disable is not just that the modulation
> stops but also that the output signal goes low, although that's not very
> explicit in the current pwm documentation.. The exynos PWM driver will
> need another fix tweak to make that true.
>
>
>
>> Best Regards,
>
>
>
>> --
>> Markus Reichl
>>
>> On 8 April 2015 at 23:19, Anand Moon <linux.amoon-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> > Hi Guenter,
>> >
>> > Sorry my blunder mistake. Sorry for the noise.
>> >
>> > I just tested with spiking this patch and my observation and testing
>> > were wrong we can skip this patch.
>> >
>> > I will send an v2 patch series removing the patch 5 and patch 6.
>> >
>> > With correct dts changes.
>> >
>> > Thanks for pointing my mistake.
>> >
>> > -Anand Moon
>> >
>> > On 8 April 2015 at 22:23, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
>> >> On Wed, Apr 08, 2015 at 09:32:05PM +0530, Anand Moon wrote:
>> >>> Hi Guenter,
>> >>>
>> >>> Initially the board bootup the cooling level state is 0.
>> >>> So update the duty cycle and this power off the fan.
>> >>> As their is no state change the fan will not spin.
>> >>>
>> >>> Once the temperature sensor is reached to alert temperature it changes state.
>> >>> With the state change the fan cools the CPU and then stop's
>> >>>
>> >>> I have observed this state change with tmon utility in linux/tools/thermal/tmon/
>> >>>
>> >> Sorry, I am missing something. I still don't see what problem you are fixing
>> >> with this patch. What behavior is wrong with the current code, and how does your
>> >> patch fix it ?
>> >>
>> >> Guenter
>> >>
>> >>> -Anand Moon
>> >>>
>> >>> On 8 April 2015 at 21:02, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
>> >>> > On Wed, Apr 08, 2015 at 10:44:15AM +0200, Lukasz Majewski wrote:
>> >>> >> Hi Anand,
>> >>> >>
>> >>> >> > Below changes depend on following patch.
>> >>> >> > https://patchwork.kernel.org/patch/5944061/
>> >>> >> >
>> >>> >> > Update the pwm_config with duty then update the pwm_disable
>> >>> >> > to poweroff the cpu fan.
>> >>> >> >
>> >>> >
>> >>> > Unfortunately, the patch does not include an explanation why it is needed.
>> >>> >
>> >>> > The original code presumably did not update the duty cycle because
>> >>> > pwm was about to be disabled anyway. That kind of made sense to me.
>> >>> > Updating the duty cycle to 0 just to disable the pwm channel right
>> >>> > afterwards does not immediately make sense.
>> >>> >
>> >>> > Given that, I would expect to see a rationale here. Why is this patch needed ?
>> >>> > Does it fix a bug ? If yes, pelase describe the bug. If not, what is the
>> >>> > purpose of this patch ?
>> >>> >
>> >>> > Maybe that is all explained in patch 0/6, which I was not copied on. Even
>> >>> > if so, the reationale will be needed in the changelog to explain to future
>> >>> > developers why this change was made.
>> >>> >
>> >>> > Thanks,
>> >>> > Guenter
>> >>> >
>> >>> >> > Tested on OdroidXU3 board.
>> >>> >> >
>> >>> >> > Signed-off-by: Anand Moon <linux.amoon-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> >>> >> > ---
>> >>> >> >  drivers/hwmon/pwm-fan.c | 10 ++++------
>> >>> >> >  1 file changed, 4 insertions(+), 6 deletions(-)
>> >>> >> >
>> >>> >> > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
>> >>> >> > index 7c83dc4..f25c841 100644
>> >>> >> > --- a/drivers/hwmon/pwm-fan.c
>> >>> >> > +++ b/drivers/hwmon/pwm-fan.c
>> >>> >> > @@ -44,26 +44,24 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx,
>> >>> >> > unsigned long pwm) int ret = 0;
>> >>> >> >
>> >>> >> >     mutex_lock(&ctx->lock);
>> >>> >> > +
>> >>> >
>> >>> > [ please refrain from unnecessary whitespace changes ]
>> >>> >
>> >>> >> >     if (ctx->pwm_value == pwm)
>> >>> >> >             goto exit_set_pwm_err;
>> >>> >> >
>> >>> >> > -   if (pwm == 0) {
>> >>> >> > -           pwm_disable(ctx->pwm);
>> >>> >> > -           goto exit_set_pwm;
>> >>> >> > -   }
>> >>> >> > -
>> >>> >> >     duty = DIV_ROUND_UP(pwm * (ctx->pwm->period - 1), MAX_PWM);
>> >>> >> >     ret = pwm_config(ctx->pwm, duty, ctx->pwm->period);
>> >>> >> >     if (ret)
>> >>> >> >             goto exit_set_pwm_err;
>> >>> >> >
>> >>> >> > +   if (pwm == 0)
>> >>> >> > +           pwm_disable(ctx->pwm);
>> >>> >> > +
>> >>> >> >     if (ctx->pwm_value == 0) {
>> >>> >> >             ret = pwm_enable(ctx->pwm);
>> >>> >> >             if (ret)
>> >>> >> >                     goto exit_set_pwm_err;
>> >>> >> >     }
>> >>> >> >
>> >>> >> > -exit_set_pwm:
>> >>> >> >     ctx->pwm_value = pwm;
>> >>> >> >  exit_set_pwm_err:
>> >>> >> >     mutex_unlock(&ctx->lock);
>> >>> >>
>> >>> >> Reviewed-by: Lukasz Majewski <l.majewski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> >>> >>
>> >>> >> BTW: I've added Guenter to CC.
>> >>> >>
>> >>> >> --
>> >>> >> Best regards,
>> >>> >>
>> >>> >> Lukasz Majewski
>> >>> >>
>> >>> >> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
>
>
--
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

Re: [PATCH 6/6] hwmon: pwm-fan: Update the duty cycle inorder to control the pwm-fan

[Guenter Roeck]

On 04/10/2015 05:59 AM, Anand Moon wrote:
> Hi Sjoerd,
>
> I don't much advance knowledge on internal signaling of pwm-samsung module.
>
> So do I need to send this patch again ?
>

 From the context, it seems that the fix in hwmon would only paint
over a problem in the actual pwm driver, correct ?

If you resubmit the patch I would expect you to explain this in the
commit log.

Guenter

> -Anand Moon
>
>
> On 10 April 2015 at 17:30, Sjoerd Simons <sjoerd.simons@collabora.co.uk> wrote:
>> Hey Anand,
>>
>> On Fri, 2015-04-10 at 16:58 +0530, Anand Moon wrote:
>>> Hi Guenter/Lukasz,
>>>
>>> Earlier I send v2 version of the patch spiking this one.
>>>
>>> Markus Riechl came back to me with below mail.
>>> So This patch confirms fixes the bug.
>>>
>>> I will send v3 version of the patch. Earlier I was in delima about the bug.
>>>
>>> -Anand Moon
>>> -------------------------------------------
>>> Hi Anand,
>>>
>>> I tested your patch.
>>>
>>> After booting the fan is spinning despite only 44°C.
>>>
>>> /sys/class/thermal/cooling_device0/curstate is 0.
>>> /sys/class/hwmon/hwmon4/pwm1 is 0
>>>
>>> when I echo 1 > cur_state and then echo 0 > cur_state again,
>>> the fan switches to off and behaves as expected.
>>>
>>> It looks like there is a bug in initializing the pwm output
>>> immediately after booting.
>>
>> The problem here will be that at boot the PWM runs at full duty. With
>> the current exynos PWM drive if you disable the PWM it will stop pulsing
>> but remain high if it was at 100% duty. My patch on which you depend
>> upon fixed a race  where disabling the pwm right after changing the duty
>> cycle (e.g. to 0%) also kept the signal high.
>>
>>  From looking at other PWM users at the time it seemed that most if not
>> all always first set to duty to 0% and then disable the pwm. Which
>> should work fine on exynos now. However iirc Thierry recently clarified
>> that the expected result of pwm_disable is not just that the modulation
>> stops but also that the output signal goes low, although that's not very
>> explicit in the current pwm documentation.. The exynos PWM driver will
>> need another fix tweak to make that true.
>>
>>
>>
>>> Best Regards,
>>
>>
>>
>>> --
>>> Markus Reichl
>>>
>>> On 8 April 2015 at 23:19, Anand Moon <linux.amoon@gmail.com> wrote:
>>>> Hi Guenter,
>>>>
>>>> Sorry my blunder mistake. Sorry for the noise.
>>>>
>>>> I just tested with spiking this patch and my observation and testing
>>>> were wrong we can skip this patch.
>>>>
>>>> I will send an v2 patch series removing the patch 5 and patch 6.
>>>>
>>>> With correct dts changes.
>>>>
>>>> Thanks for pointing my mistake.
>>>>
>>>> -Anand Moon
>>>>
>>>> On 8 April 2015 at 22:23, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>> On Wed, Apr 08, 2015 at 09:32:05PM +0530, Anand Moon wrote:
>>>>>> Hi Guenter,
>>>>>>
>>>>>> Initially the board bootup the cooling level state is 0.
>>>>>> So update the duty cycle and this power off the fan.
>>>>>> As their is no state change the fan will not spin.
>>>>>>
>>>>>> Once the temperature sensor is reached to alert temperature it changes state.
>>>>>> With the state change the fan cools the CPU and then stop's
>>>>>>
>>>>>> I have observed this state change with tmon utility in linux/tools/thermal/tmon/
>>>>>>
>>>>> Sorry, I am missing something. I still don't see what problem you are fixing
>>>>> with this patch. What behavior is wrong with the current code, and how does your
>>>>> patch fix it ?
>>>>>
>>>>> Guenter
>>>>>
>>>>>> -Anand Moon
>>>>>>
>>>>>> On 8 April 2015 at 21:02, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>>>> On Wed, Apr 08, 2015 at 10:44:15AM +0200, Lukasz Majewski wrote:
>>>>>>>> Hi Anand,
>>>>>>>>
>>>>>>>>> Below changes depend on following patch.
>>>>>>>>> https://patchwork.kernel.org/patch/5944061/
>>>>>>>>>
>>>>>>>>> Update the pwm_config with duty then update the pwm_disable
>>>>>>>>> to poweroff the cpu fan.
>>>>>>>>>
>>>>>>>
>>>>>>> Unfortunately, the patch does not include an explanation why it is needed.
>>>>>>>
>>>>>>> The original code presumably did not update the duty cycle because
>>>>>>> pwm was about to be disabled anyway. That kind of made sense to me.
>>>>>>> Updating the duty cycle to 0 just to disable the pwm channel right
>>>>>>> afterwards does not immediately make sense.
>>>>>>>
>>>>>>> Given that, I would expect to see a rationale here. Why is this patch needed ?
>>>>>>> Does it fix a bug ? If yes, pelase describe the bug. If not, what is the
>>>>>>> purpose of this patch ?
>>>>>>>
>>>>>>> Maybe that is all explained in patch 0/6, which I was not copied on. Even
>>>>>>> if so, the reationale will be needed in the changelog to explain to future
>>>>>>> developers why this change was made.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Guenter
>>>>>>>
>>>>>>>>> Tested on OdroidXU3 board.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>>>>>>>> ---
>>>>>>>>>   drivers/hwmon/pwm-fan.c | 10 ++++------
>>>>>>>>>   1 file changed, 4 insertions(+), 6 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
>>>>>>>>> index 7c83dc4..f25c841 100644
>>>>>>>>> --- a/drivers/hwmon/pwm-fan.c
>>>>>>>>> +++ b/drivers/hwmon/pwm-fan.c
>>>>>>>>> @@ -44,26 +44,24 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx,
>>>>>>>>> unsigned long pwm) int ret = 0;
>>>>>>>>>
>>>>>>>>>      mutex_lock(&ctx->lock);
>>>>>>>>> +
>>>>>>>
>>>>>>> [ please refrain from unnecessary whitespace changes ]
>>>>>>>
>>>>>>>>>      if (ctx->pwm_value == pwm)
>>>>>>>>>              goto exit_set_pwm_err;
>>>>>>>>>
>>>>>>>>> -   if (pwm == 0) {
>>>>>>>>> -           pwm_disable(ctx->pwm);
>>>>>>>>> -           goto exit_set_pwm;
>>>>>>>>> -   }
>>>>>>>>> -
>>>>>>>>>      duty = DIV_ROUND_UP(pwm * (ctx->pwm->period - 1), MAX_PWM);
>>>>>>>>>      ret = pwm_config(ctx->pwm, duty, ctx->pwm->period);
>>>>>>>>>      if (ret)
>>>>>>>>>              goto exit_set_pwm_err;
>>>>>>>>>
>>>>>>>>> +   if (pwm == 0)
>>>>>>>>> +           pwm_disable(ctx->pwm);
>>>>>>>>> +
>>>>>>>>>      if (ctx->pwm_value == 0) {
>>>>>>>>>              ret = pwm_enable(ctx->pwm);
>>>>>>>>>              if (ret)
>>>>>>>>>                      goto exit_set_pwm_err;
>>>>>>>>>      }
>>>>>>>>>
>>>>>>>>> -exit_set_pwm:
>>>>>>>>>      ctx->pwm_value = pwm;
>>>>>>>>>   exit_set_pwm_err:
>>>>>>>>>      mutex_unlock(&ctx->lock);
>>>>>>>>
>>>>>>>> Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>
>>>>>>>>
>>>>>>>> BTW: I've added Guenter to CC.
>>>>>>>>
>>>>>>>> --
>>>>>>>> Best regards,
>>>>>>>>
>>>>>>>> Lukasz Majewski
>>>>>>>>
>>>>>>>> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
>>
>>
>

Re: [PATCH 6/6] hwmon: pwm-fan: Update the duty cycle inorder to control the pwm-fan

[Anand Moon]

Hi Guenter,

I will do so.

-Anand Moon

On 10 April 2015 at 18:39, Guenter Roeck <linux@roeck-us.net> wrote:
> On 04/10/2015 05:59 AM, Anand Moon wrote:
>>
>> Hi Sjoerd,
>>
>> I don't much advance knowledge on internal signaling of pwm-samsung
>> module.
>>
>> So do I need to send this patch again ?
>>
>
> From the context, it seems that the fix in hwmon would only paint
> over a problem in the actual pwm driver, correct ?
>
> If you resubmit the patch I would expect you to explain this in the
> commit log.
>
> Guenter
>
>
>> -Anand Moon
>>
>>
>> On 10 April 2015 at 17:30, Sjoerd Simons <sjoerd.simons@collabora.co.uk>
>> wrote:
>>>
>>> Hey Anand,
>>>
>>> On Fri, 2015-04-10 at 16:58 +0530, Anand Moon wrote:
>>>>
>>>> Hi Guenter/Lukasz,
>>>>
>>>> Earlier I send v2 version of the patch spiking this one.
>>>>
>>>> Markus Riechl came back to me with below mail.
>>>> So This patch confirms fixes the bug.
>>>>
>>>> I will send v3 version of the patch. Earlier I was in delima about the
>>>> bug.
>>>>
>>>> -Anand Moon
>>>> -------------------------------------------
>>>> Hi Anand,
>>>>
>>>> I tested your patch.
>>>>
>>>> After booting the fan is spinning despite only 44°C.
>>>>
>>>> /sys/class/thermal/cooling_device0/curstate is 0.
>>>> /sys/class/hwmon/hwmon4/pwm1 is 0
>>>>
>>>> when I echo 1 > cur_state and then echo 0 > cur_state again,
>>>> the fan switches to off and behaves as expected.
>>>>
>>>> It looks like there is a bug in initializing the pwm output
>>>> immediately after booting.
>>>
>>>
>>> The problem here will be that at boot the PWM runs at full duty. With
>>> the current exynos PWM drive if you disable the PWM it will stop pulsing
>>> but remain high if it was at 100% duty. My patch on which you depend
>>> upon fixed a race  where disabling the pwm right after changing the duty
>>> cycle (e.g. to 0%) also kept the signal high.
>>>
>>>  From looking at other PWM users at the time it seemed that most if not
>>> all always first set to duty to 0% and then disable the pwm. Which
>>> should work fine on exynos now. However iirc Thierry recently clarified
>>> that the expected result of pwm_disable is not just that the modulation
>>> stops but also that the output signal goes low, although that's not very
>>> explicit in the current pwm documentation.. The exynos PWM driver will
>>> need another fix tweak to make that true.
>>>
>>>
>>>
>>>> Best Regards,
>>>
>>>
>>>
>>>
>>>> --
>>>> Markus Reichl
>>>>
>>>> On 8 April 2015 at 23:19, Anand Moon <linux.amoon@gmail.com> wrote:
>>>>>
>>>>> Hi Guenter,
>>>>>
>>>>> Sorry my blunder mistake. Sorry for the noise.
>>>>>
>>>>> I just tested with spiking this patch and my observation and testing
>>>>> were wrong we can skip this patch.
>>>>>
>>>>> I will send an v2 patch series removing the patch 5 and patch 6.
>>>>>
>>>>> With correct dts changes.
>>>>>
>>>>> Thanks for pointing my mistake.
>>>>>
>>>>> -Anand Moon
>>>>>
>>>>> On 8 April 2015 at 22:23, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>>>
>>>>>> On Wed, Apr 08, 2015 at 09:32:05PM +0530, Anand Moon wrote:
>>>>>>>
>>>>>>> Hi Guenter,
>>>>>>>
>>>>>>> Initially the board bootup the cooling level state is 0.
>>>>>>> So update the duty cycle and this power off the fan.
>>>>>>> As their is no state change the fan will not spin.
>>>>>>>
>>>>>>> Once the temperature sensor is reached to alert temperature it
>>>>>>> changes state.
>>>>>>> With the state change the fan cools the CPU and then stop's
>>>>>>>
>>>>>>> I have observed this state change with tmon utility in
>>>>>>> linux/tools/thermal/tmon/
>>>>>>>
>>>>>> Sorry, I am missing something. I still don't see what problem you are
>>>>>> fixing
>>>>>> with this patch. What behavior is wrong with the current code, and how
>>>>>> does your
>>>>>> patch fix it ?
>>>>>>
>>>>>> Guenter
>>>>>>
>>>>>>> -Anand Moon
>>>>>>>
>>>>>>> On 8 April 2015 at 21:02, Guenter Roeck <linux@roeck-us.net> wrote:
>>>>>>>>
>>>>>>>> On Wed, Apr 08, 2015 at 10:44:15AM +0200, Lukasz Majewski wrote:
>>>>>>>>>
>>>>>>>>> Hi Anand,
>>>>>>>>>
>>>>>>>>>> Below changes depend on following patch.
>>>>>>>>>> https://patchwork.kernel.org/patch/5944061/
>>>>>>>>>>
>>>>>>>>>> Update the pwm_config with duty then update the pwm_disable
>>>>>>>>>> to poweroff the cpu fan.
>>>>>>>>>>
>>>>>>>>
>>>>>>>> Unfortunately, the patch does not include an explanation why it is
>>>>>>>> needed.
>>>>>>>>
>>>>>>>> The original code presumably did not update the duty cycle because
>>>>>>>> pwm was about to be disabled anyway. That kind of made sense to me.
>>>>>>>> Updating the duty cycle to 0 just to disable the pwm channel right
>>>>>>>> afterwards does not immediately make sense.
>>>>>>>>
>>>>>>>> Given that, I would expect to see a rationale here. Why is this
>>>>>>>> patch needed ?
>>>>>>>> Does it fix a bug ? If yes, pelase describe the bug. If not, what is
>>>>>>>> the
>>>>>>>> purpose of this patch ?
>>>>>>>>
>>>>>>>> Maybe that is all explained in patch 0/6, which I was not copied on.
>>>>>>>> Even
>>>>>>>> if so, the reationale will be needed in the changelog to explain to
>>>>>>>> future
>>>>>>>> developers why this change was made.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Guenter
>>>>>>>>
>>>>>>>>>> Tested on OdroidXU3 board.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>>>>>>>>> ---
>>>>>>>>>>   drivers/hwmon/pwm-fan.c | 10 ++++------
>>>>>>>>>>   1 file changed, 4 insertions(+), 6 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
>>>>>>>>>> index 7c83dc4..f25c841 100644
>>>>>>>>>> --- a/drivers/hwmon/pwm-fan.c
>>>>>>>>>> +++ b/drivers/hwmon/pwm-fan.c
>>>>>>>>>> @@ -44,26 +44,24 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx,
>>>>>>>>>> unsigned long pwm) int ret = 0;
>>>>>>>>>>
>>>>>>>>>>      mutex_lock(&ctx->lock);
>>>>>>>>>> +
>>>>>>>>
>>>>>>>>
>>>>>>>> [ please refrain from unnecessary whitespace changes ]
>>>>>>>>
>>>>>>>>>>      if (ctx->pwm_value == pwm)
>>>>>>>>>>              goto exit_set_pwm_err;
>>>>>>>>>>
>>>>>>>>>> -   if (pwm == 0) {
>>>>>>>>>> -           pwm_disable(ctx->pwm);
>>>>>>>>>> -           goto exit_set_pwm;
>>>>>>>>>> -   }
>>>>>>>>>> -
>>>>>>>>>>      duty = DIV_ROUND_UP(pwm * (ctx->pwm->period - 1), MAX_PWM);
>>>>>>>>>>      ret = pwm_config(ctx->pwm, duty, ctx->pwm->period);
>>>>>>>>>>      if (ret)
>>>>>>>>>>              goto exit_set_pwm_err;
>>>>>>>>>>
>>>>>>>>>> +   if (pwm == 0)
>>>>>>>>>> +           pwm_disable(ctx->pwm);
>>>>>>>>>> +
>>>>>>>>>>      if (ctx->pwm_value == 0) {
>>>>>>>>>>              ret = pwm_enable(ctx->pwm);
>>>>>>>>>>              if (ret)
>>>>>>>>>>                      goto exit_set_pwm_err;
>>>>>>>>>>      }
>>>>>>>>>>
>>>>>>>>>> -exit_set_pwm:
>>>>>>>>>>      ctx->pwm_value = pwm;
>>>>>>>>>>   exit_set_pwm_err:
>>>>>>>>>>      mutex_unlock(&ctx->lock);
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>
>>>>>>>>>
>>>>>>>>> BTW: I've added Guenter to CC.
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> Best regards,
>>>>>>>>>
>>>>>>>>> Lukasz Majewski
>>>>>>>>>
>>>>>>>>> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
>>>
>>>
>>>
>>
>

Re: [PATCH 6/6] hwmon: pwm-fan: Update the duty cycle inorder to control the pwm-fan

[Sjoerd Simons]

On Fri, 2015-04-10 at 06:09 -0700, Guenter Roeck wrote:
> On 04/10/2015 05:59 AM, Anand Moon wrote:
> > Hi Sjoerd,
> >
> > I don't much advance knowledge on internal signaling of pwm-samsung module.
> >
> > So do I need to send this patch again ?
> >
> 
>  From the context, it seems that the fix in hwmon would only paint
> over a problem in the actual pwm driver, correct ?

Yes/no/maybe :). Imho this is something to clarify in the pwm API
documentation. As currently all it says is: 
  "pwm_disable - stop a PWM output toggling", 

Which is what the exynos driver does. 

Thierry, could you clearify what the intention is here? I'm happy to
prepare a pwm driver patch if needed to solve this?

> If you resubmit the patch I would expect you to explain this in the
> commit log.
> 
> Guenter
> 
> > -Anand Moon
> >
> >
> > On 10 April 2015 at 17:30, Sjoerd Simons <sjoerd.simons@collabora.co.uk> wrote:
> >> Hey Anand,
> >>
> >> On Fri, 2015-04-10 at 16:58 +0530, Anand Moon wrote:
> >>> Hi Guenter/Lukasz,
> >>>
> >>> Earlier I send v2 version of the patch spiking this one.
> >>>
> >>> Markus Riechl came back to me with below mail.
> >>> So This patch confirms fixes the bug.
> >>>
> >>> I will send v3 version of the patch. Earlier I was in delima about the bug.
> >>>
> >>> -Anand Moon
> >>> -------------------------------------------
> >>> Hi Anand,
> >>>
> >>> I tested your patch.
> >>>
> >>> After booting the fan is spinning despite only 44°C.
> >>>
> >>> /sys/class/thermal/cooling_device0/curstate is 0.
> >>> /sys/class/hwmon/hwmon4/pwm1 is 0
> >>>
> >>> when I echo 1 > cur_state and then echo 0 > cur_state again,
> >>> the fan switches to off and behaves as expected.
> >>>
> >>> It looks like there is a bug in initializing the pwm output
> >>> immediately after booting.
> >>
> >> The problem here will be that at boot the PWM runs at full duty. With
> >> the current exynos PWM drive if you disable the PWM it will stop pulsing
> >> but remain high if it was at 100% duty. My patch on which you depend
> >> upon fixed a race  where disabling the pwm right after changing the duty
> >> cycle (e.g. to 0%) also kept the signal high.
> >>
> >>  From looking at other PWM users at the time it seemed that most if not
> >> all always first set to duty to 0% and then disable the pwm. Which
> >> should work fine on exynos now. However iirc Thierry recently clarified
> >> that the expected result of pwm_disable is not just that the modulation
> >> stops but also that the output signal goes low, although that's not very
> >> explicit in the current pwm documentation.. The exynos PWM driver will
> >> need another fix tweak to make that true.
> >>
> >>
> >>
> >>> Best Regards,
> >>
> >>
> >>
> >>> --
> >>> Markus Reichl
> >>>
> >>> On 8 April 2015 at 23:19, Anand Moon <linux.amoon@gmail.com> wrote:
> >>>> Hi Guenter,
> >>>>
> >>>> Sorry my blunder mistake. Sorry for the noise.
> >>>>
> >>>> I just tested with spiking this patch and my observation and testing
> >>>> were wrong we can skip this patch.
> >>>>
> >>>> I will send an v2 patch series removing the patch 5 and patch 6.
> >>>>
> >>>> With correct dts changes.
> >>>>
> >>>> Thanks for pointing my mistake.
> >>>>
> >>>> -Anand Moon
> >>>>
> >>>> On 8 April 2015 at 22:23, Guenter Roeck <linux@roeck-us.net> wrote:
> >>>>> On Wed, Apr 08, 2015 at 09:32:05PM +0530, Anand Moon wrote:
> >>>>>> Hi Guenter,
> >>>>>>
> >>>>>> Initially the board bootup the cooling level state is 0.
> >>>>>> So update the duty cycle and this power off the fan.
> >>>>>> As their is no state change the fan will not spin.
> >>>>>>
> >>>>>> Once the temperature sensor is reached to alert temperature it changes state.
> >>>>>> With the state change the fan cools the CPU and then stop's
> >>>>>>
> >>>>>> I have observed this state change with tmon utility in linux/tools/thermal/tmon/
> >>>>>>
> >>>>> Sorry, I am missing something. I still don't see what problem you are fixing
> >>>>> with this patch. What behavior is wrong with the current code, and how does your
> >>>>> patch fix it ?
> >>>>>
> >>>>> Guenter
> >>>>>
> >>>>>> -Anand Moon
> >>>>>>
> >>>>>> On 8 April 2015 at 21:02, Guenter Roeck <linux@roeck-us.net> wrote:
> >>>>>>> On Wed, Apr 08, 2015 at 10:44:15AM +0200, Lukasz Majewski wrote:
> >>>>>>>> Hi Anand,
> >>>>>>>>
> >>>>>>>>> Below changes depend on following patch.
> >>>>>>>>> https://patchwork.kernel.org/patch/5944061/
> >>>>>>>>>
> >>>>>>>>> Update the pwm_config with duty then update the pwm_disable
> >>>>>>>>> to poweroff the cpu fan.
> >>>>>>>>>
> >>>>>>>
> >>>>>>> Unfortunately, the patch does not include an explanation why it is needed.
> >>>>>>>
> >>>>>>> The original code presumably did not update the duty cycle because
> >>>>>>> pwm was about to be disabled anyway. That kind of made sense to me.
> >>>>>>> Updating the duty cycle to 0 just to disable the pwm channel right
> >>>>>>> afterwards does not immediately make sense.
> >>>>>>>
> >>>>>>> Given that, I would expect to see a rationale here. Why is this patch needed ?
> >>>>>>> Does it fix a bug ? If yes, pelase describe the bug. If not, what is the
> >>>>>>> purpose of this patch ?
> >>>>>>>
> >>>>>>> Maybe that is all explained in patch 0/6, which I was not copied on. Even
> >>>>>>> if so, the reationale will be needed in the changelog to explain to future
> >>>>>>> developers why this change was made.
> >>>>>>>
> >>>>>>> Thanks,
> >>>>>>> Guenter
> >>>>>>>
> >>>>>>>>> Tested on OdroidXU3 board.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> >>>>>>>>> ---
> >>>>>>>>>   drivers/hwmon/pwm-fan.c | 10 ++++------
> >>>>>>>>>   1 file changed, 4 insertions(+), 6 deletions(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> >>>>>>>>> index 7c83dc4..f25c841 100644
> >>>>>>>>> --- a/drivers/hwmon/pwm-fan.c
> >>>>>>>>> +++ b/drivers/hwmon/pwm-fan.c
> >>>>>>>>> @@ -44,26 +44,24 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx,
> >>>>>>>>> unsigned long pwm) int ret = 0;
> >>>>>>>>>
> >>>>>>>>>      mutex_lock(&ctx->lock);
> >>>>>>>>> +
> >>>>>>>
> >>>>>>> [ please refrain from unnecessary whitespace changes ]
> >>>>>>>
> >>>>>>>>>      if (ctx->pwm_value == pwm)
> >>>>>>>>>              goto exit_set_pwm_err;
> >>>>>>>>>
> >>>>>>>>> -   if (pwm == 0) {
> >>>>>>>>> -           pwm_disable(ctx->pwm);
> >>>>>>>>> -           goto exit_set_pwm;
> >>>>>>>>> -   }
> >>>>>>>>> -
> >>>>>>>>>      duty = DIV_ROUND_UP(pwm * (ctx->pwm->period - 1), MAX_PWM);
> >>>>>>>>>      ret = pwm_config(ctx->pwm, duty, ctx->pwm->period);
> >>>>>>>>>      if (ret)
> >>>>>>>>>              goto exit_set_pwm_err;
> >>>>>>>>>
> >>>>>>>>> +   if (pwm == 0)
> >>>>>>>>> +           pwm_disable(ctx->pwm);
> >>>>>>>>> +
> >>>>>>>>>      if (ctx->pwm_value == 0) {
> >>>>>>>>>              ret = pwm_enable(ctx->pwm);
> >>>>>>>>>              if (ret)
> >>>>>>>>>                      goto exit_set_pwm_err;
> >>>>>>>>>      }
> >>>>>>>>>
> >>>>>>>>> -exit_set_pwm:
> >>>>>>>>>      ctx->pwm_value = pwm;
> >>>>>>>>>   exit_set_pwm_err:
> >>>>>>>>>      mutex_unlock(&ctx->lock);
> >>>>>>>>
> >>>>>>>> Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>
> >>>>>>>>
> >>>>>>>> BTW: I've added Guenter to CC.
> >>>>>>>>
> >>>>>>>> --
> >>>>>>>> Best regards,
> >>>>>>>>
> >>>>>>>> Lukasz Majewski
> >>>>>>>>
> >>>>>>>> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
> >>
> >>
> >
> 

Re: [PATCH 6/6] hwmon: pwm-fan: Update the duty cycle inorder to control the pwm-fan

[Thierry Reding]

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

On Fri, Apr 10, 2015 at 03:30:01PM +0200, Sjoerd Simons wrote:
> On Fri, 2015-04-10 at 06:09 -0700, Guenter Roeck wrote:
> > On 04/10/2015 05:59 AM, Anand Moon wrote:
> > > Hi Sjoerd,
> > >
> > > I don't much advance knowledge on internal signaling of pwm-samsung module.
> > >
> > > So do I need to send this patch again ?
> > >
> > 
> >  From the context, it seems that the fix in hwmon would only paint
> > over a problem in the actual pwm driver, correct ?
> 
> Yes/no/maybe :). Imho this is something to clarify in the pwm API
> documentation. As currently all it says is: 
>   "pwm_disable - stop a PWM output toggling", 
> 
> Which is what the exynos driver does. 
> 
> Thierry, could you clearify what the intention is here? I'm happy to
> prepare a pwm driver patch if needed to solve this?

I think the safest thing to do is for users to do both. You call
pwm_config() with a zero duty cycle to make it clear what the status is
that you want. Then you call pwm_disable() to state that you don't need
the output signal anymore, so that any clocks needed by the PWM can be
stopped. Doing so gives the driver the most information and should make
the user more resilient against any possible quirks in drivers.

Drivers that on pwm_disable() still invert the signal should still be
fixed, though. In the cases that I'm aware of the sequence of setting
the duty cycle to 0 and calling pwm_disable() won't actually paper over
any bugs. It's precisely that sequence that triggers the bug. Merely
calling pwm_disable() should trigger it as well, pwm_config(0) without
pwm_disable() would hide it.

Thierry

> > If you resubmit the patch I would expect you to explain this in the
> > commit log.
> > 
> > Guenter
> > 
> > > -Anand Moon
> > >
> > >
> > > On 10 April 2015 at 17:30, Sjoerd Simons <sjoerd.simons@collabora.co.uk> wrote:
> > >> Hey Anand,
> > >>
> > >> On Fri, 2015-04-10 at 16:58 +0530, Anand Moon wrote:
> > >>> Hi Guenter/Lukasz,
> > >>>
> > >>> Earlier I send v2 version of the patch spiking this one.
> > >>>
> > >>> Markus Riechl came back to me with below mail.
> > >>> So This patch confirms fixes the bug.
> > >>>
> > >>> I will send v3 version of the patch. Earlier I was in delima about the bug.
> > >>>
> > >>> -Anand Moon
> > >>> -------------------------------------------
> > >>> Hi Anand,
> > >>>
> > >>> I tested your patch.
> > >>>
> > >>> After booting the fan is spinning despite only 44°C.
> > >>>
> > >>> /sys/class/thermal/cooling_device0/curstate is 0.
> > >>> /sys/class/hwmon/hwmon4/pwm1 is 0
> > >>>
> > >>> when I echo 1 > cur_state and then echo 0 > cur_state again,
> > >>> the fan switches to off and behaves as expected.
> > >>>
> > >>> It looks like there is a bug in initializing the pwm output
> > >>> immediately after booting.
> > >>
> > >> The problem here will be that at boot the PWM runs at full duty. With
> > >> the current exynos PWM drive if you disable the PWM it will stop pulsing
> > >> but remain high if it was at 100% duty. My patch on which you depend
> > >> upon fixed a race  where disabling the pwm right after changing the duty
> > >> cycle (e.g. to 0%) also kept the signal high.
> > >>
> > >>  From looking at other PWM users at the time it seemed that most if not
> > >> all always first set to duty to 0% and then disable the pwm. Which
> > >> should work fine on exynos now. However iirc Thierry recently clarified
> > >> that the expected result of pwm_disable is not just that the modulation
> > >> stops but also that the output signal goes low, although that's not very
> > >> explicit in the current pwm documentation.. The exynos PWM driver will
> > >> need another fix tweak to make that true.
> > >>
> > >>
> > >>
> > >>> Best Regards,
> > >>
> > >>
> > >>
> > >>> --
> > >>> Markus Reichl
> > >>>
> > >>> On 8 April 2015 at 23:19, Anand Moon <linux.amoon@gmail.com> wrote:
> > >>>> Hi Guenter,
> > >>>>
> > >>>> Sorry my blunder mistake. Sorry for the noise.
> > >>>>
> > >>>> I just tested with spiking this patch and my observation and testing
> > >>>> were wrong we can skip this patch.
> > >>>>
> > >>>> I will send an v2 patch series removing the patch 5 and patch 6.
> > >>>>
> > >>>> With correct dts changes.
> > >>>>
> > >>>> Thanks for pointing my mistake.
> > >>>>
> > >>>> -Anand Moon
> > >>>>
> > >>>> On 8 April 2015 at 22:23, Guenter Roeck <linux@roeck-us.net> wrote:
> > >>>>> On Wed, Apr 08, 2015 at 09:32:05PM +0530, Anand Moon wrote:
> > >>>>>> Hi Guenter,
> > >>>>>>
> > >>>>>> Initially the board bootup the cooling level state is 0.
> > >>>>>> So update the duty cycle and this power off the fan.
> > >>>>>> As their is no state change the fan will not spin.
> > >>>>>>
> > >>>>>> Once the temperature sensor is reached to alert temperature it changes state.
> > >>>>>> With the state change the fan cools the CPU and then stop's
> > >>>>>>
> > >>>>>> I have observed this state change with tmon utility in linux/tools/thermal/tmon/
> > >>>>>>
> > >>>>> Sorry, I am missing something. I still don't see what problem you are fixing
> > >>>>> with this patch. What behavior is wrong with the current code, and how does your
> > >>>>> patch fix it ?
> > >>>>>
> > >>>>> Guenter
> > >>>>>
> > >>>>>> -Anand Moon
> > >>>>>>
> > >>>>>> On 8 April 2015 at 21:02, Guenter Roeck <linux@roeck-us.net> wrote:
> > >>>>>>> On Wed, Apr 08, 2015 at 10:44:15AM +0200, Lukasz Majewski wrote:
> > >>>>>>>> Hi Anand,
> > >>>>>>>>
> > >>>>>>>>> Below changes depend on following patch.
> > >>>>>>>>> https://patchwork.kernel.org/patch/5944061/
> > >>>>>>>>>
> > >>>>>>>>> Update the pwm_config with duty then update the pwm_disable
> > >>>>>>>>> to poweroff the cpu fan.
> > >>>>>>>>>
> > >>>>>>>
> > >>>>>>> Unfortunately, the patch does not include an explanation why it is needed.
> > >>>>>>>
> > >>>>>>> The original code presumably did not update the duty cycle because
> > >>>>>>> pwm was about to be disabled anyway. That kind of made sense to me.
> > >>>>>>> Updating the duty cycle to 0 just to disable the pwm channel right
> > >>>>>>> afterwards does not immediately make sense.
> > >>>>>>>
> > >>>>>>> Given that, I would expect to see a rationale here. Why is this patch needed ?
> > >>>>>>> Does it fix a bug ? If yes, pelase describe the bug. If not, what is the
> > >>>>>>> purpose of this patch ?
> > >>>>>>>
> > >>>>>>> Maybe that is all explained in patch 0/6, which I was not copied on. Even
> > >>>>>>> if so, the reationale will be needed in the changelog to explain to future
> > >>>>>>> developers why this change was made.
> > >>>>>>>
> > >>>>>>> Thanks,
> > >>>>>>> Guenter
> > >>>>>>>
> > >>>>>>>>> Tested on OdroidXU3 board.
> > >>>>>>>>>
> > >>>>>>>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > >>>>>>>>> ---
> > >>>>>>>>>   drivers/hwmon/pwm-fan.c | 10 ++++------
> > >>>>>>>>>   1 file changed, 4 insertions(+), 6 deletions(-)
> > >>>>>>>>>
> > >>>>>>>>> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> > >>>>>>>>> index 7c83dc4..f25c841 100644
> > >>>>>>>>> --- a/drivers/hwmon/pwm-fan.c
> > >>>>>>>>> +++ b/drivers/hwmon/pwm-fan.c
> > >>>>>>>>> @@ -44,26 +44,24 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx,
> > >>>>>>>>> unsigned long pwm) int ret = 0;
> > >>>>>>>>>
> > >>>>>>>>>      mutex_lock(&ctx->lock);
> > >>>>>>>>> +
> > >>>>>>>
> > >>>>>>> [ please refrain from unnecessary whitespace changes ]
> > >>>>>>>
> > >>>>>>>>>      if (ctx->pwm_value == pwm)
> > >>>>>>>>>              goto exit_set_pwm_err;
> > >>>>>>>>>
> > >>>>>>>>> -   if (pwm == 0) {
> > >>>>>>>>> -           pwm_disable(ctx->pwm);
> > >>>>>>>>> -           goto exit_set_pwm;
> > >>>>>>>>> -   }
> > >>>>>>>>> -
> > >>>>>>>>>      duty = DIV_ROUND_UP(pwm * (ctx->pwm->period - 1), MAX_PWM);
> > >>>>>>>>>      ret = pwm_config(ctx->pwm, duty, ctx->pwm->period);
> > >>>>>>>>>      if (ret)
> > >>>>>>>>>              goto exit_set_pwm_err;
> > >>>>>>>>>
> > >>>>>>>>> +   if (pwm == 0)
> > >>>>>>>>> +           pwm_disable(ctx->pwm);
> > >>>>>>>>> +
> > >>>>>>>>>      if (ctx->pwm_value == 0) {
> > >>>>>>>>>              ret = pwm_enable(ctx->pwm);
> > >>>>>>>>>              if (ret)
> > >>>>>>>>>                      goto exit_set_pwm_err;
> > >>>>>>>>>      }
> > >>>>>>>>>
> > >>>>>>>>> -exit_set_pwm:
> > >>>>>>>>>      ctx->pwm_value = pwm;
> > >>>>>>>>>   exit_set_pwm_err:
> > >>>>>>>>>      mutex_unlock(&ctx->lock);
> > >>>>>>>>
> > >>>>>>>> Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>
> > >>>>>>>>
> > >>>>>>>> BTW: I've added Guenter to CC.
> > >>>>>>>>
> > >>>>>>>> --
> > >>>>>>>> Best regards,
> > >>>>>>>>
> > >>>>>>>> Lukasz Majewski
> > >>>>>>>>
> > >>>>>>>> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
> > >>
> > >>
> > >
> > 
> 
> 

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 6/6] hwmon: pwm-fan: Update the duty cycle inorder to control the pwm-fan

[Ben Gamari]

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

Thierry Reding <thierry.reding@gmail.com> writes:

> On Fri, Apr 10, 2015 at 03:30:01PM +0200, Sjoerd Simons wrote:
>> 
>> Yes/no/maybe :). Imho this is something to clarify in the pwm API
>> documentation. As currently all it says is: 
>>   "pwm_disable - stop a PWM output toggling", 
>> 
>> Which is what the exynos driver does. 
>> 
>> Thierry, could you clearify what the intention is here? I'm happy to
>> prepare a pwm driver patch if needed to solve this?
>
> I think the safest thing to do is for users to do both. You call
> pwm_config() with a zero duty cycle to make it clear what the status is
> that you want. Then you call pwm_disable() to state that you don't need
> the output signal anymore, so that any clocks needed by the PWM can be
> stopped. Doing so gives the driver the most information and should make
> the user more resilient against any possible quirks in drivers.
>
It would be great if the documentation were more clear on this matter
regardless. This is something I can imagine having to spend substantial
amounts of time Googling whereas a simple note in the documentation would
have removed all ambiguity.

Cheers,

- Ben

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]

Re: [PATCH 6/6] hwmon: pwm-fan: Update the duty cycle inorder to control the pwm-fan

[Guenter Roeck]

On Fri, Apr 10, 2015 at 01:25:52PM -0400, Ben Gamari wrote:
> Thierry Reding <thierry.reding@gmail.com> writes:
> 
> > On Fri, Apr 10, 2015 at 03:30:01PM +0200, Sjoerd Simons wrote:
> >> 
> >> Yes/no/maybe :). Imho this is something to clarify in the pwm API
> >> documentation. As currently all it says is: 
> >>   "pwm_disable - stop a PWM output toggling", 
> >> 
> >> Which is what the exynos driver does. 
> >> 
> >> Thierry, could you clearify what the intention is here? I'm happy to
> >> prepare a pwm driver patch if needed to solve this?
> >
> > I think the safest thing to do is for users to do both. You call
> > pwm_config() with a zero duty cycle to make it clear what the status is
> > that you want. Then you call pwm_disable() to state that you don't need
> > the output signal anymore, so that any clocks needed by the PWM can be
> > stopped. Doing so gives the driver the most information and should make
> > the user more resilient against any possible quirks in drivers.
> >
> It would be great if the documentation were more clear on this matter
> regardless. This is something I can imagine having to spend substantial
> amounts of time Googling whereas a simple note in the documentation would
> have removed all ambiguity.
> 
Especially since, in this case, the output signal _is_ still needed.
It appears that pwm_disable() is only expected to stop the clock, not
the signal itself.

Guenter

Re: Exynos5422 odroidxu3 pwm-fan control using thermal sensors

[Markus Reichl]

Am Freitag, 27. März 2015, 03:09:09 schrieb Anand Moon:
> This work depeds upon work done by Lukasz Majewski <l.majewski@samsung.com>
>  and Sjoerd Simons <sjoerd.simons@collabora.co.uk> regarding the pwm-fan.
> 
> -Anand Moon
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Tested-by: Markus Reichl <m.reichl@fivetechno.de>

Best Regards,
---
Markus Reichl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Exynos5422 odroidxu3 pwm-fan control using thermal sensors

[Markus Reichl]

Am Freitag, 27. März 2015, 03:09:09 schrieb Anand Moon:
> This work depeds upon work done by Lukasz Majewski <l.majewski@samsung.com>
>  and Sjoerd Simons <sjoerd.simons@collabora.co.uk> regarding the pwm-fan.
> 
> -Anand Moon
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

MarkusTested-by:  Markus Reichl <m.reichl@fivetechno.de>

Best Regards,
---
Markus Reichl