Re: [RFC PATCH 0/2] thermal: Add generic devfreq cooling device

[Chanwoo Choi <cw00.choi@samsung.com>]

Hi Punit,

On 07/20/2015 11:43 PM, Punit Agrawal wrote:
> Hi Chanwoo,
> 
> Chanwoo Choi <cw00.choi@samsung.com> writes:
> 
>> Hi Punit,
>>
>> On 07/17/2015 07:53 PM, Punit Agrawal wrote:
>>> Hi Chanwoo,
>>>
>>> Chanwoo Choi <cw00.choi@samsung.com> writes:
>>>
>>>> This patchset introduce the generic devfreq cooling device for generic thermal
>>>> framework. The devfreq devices are used ad cooling device to reduce the
>>>> overheating temperature. This patch is based on drivers/thermal/cpu_cooling.c.
>>>> The devfreq cooling device can change the ragne of the frequency table of
>>>> devfreq device according to cooling level in device tree file.
>>>>
>>>
>>> Have you had a look at the devfreq cooling patches from Javi[0][1]? How
>>> is the current patchset different?
>>
>> I didn't see Javi's patchset before. Thanks for your information.
>>
>> I reviewed ths Javi's patchset. Both Javi's patchset and my patchset 
>> has same concept except for applying the power allocator thermal governor
>> as you below comment.
>>
>> But, there are some difference.
>>
>> First,
>> I don't add new devfreq API (devfreq_set_max() / devfreq_set_min()).
>> The my patchset used existing update_devfreq() to update the
>> maximum frequency of devfreq device.
>>
> 
> Ok.
> 
>> Second,
>> In my patchset, the devfreq cooling device will be operated
>> as existing cpu cooling device. If sensor measure the overheating
>> temperature, devfreq cooling device will limit the maximum frequency
>> of devfreq device.
> 
> Javi's patchset behaves exactly as you describe here.
> 
>> As below example, the devicetree file includes
>> the overheating temperature information of each trip-point.
>> - Javi's patchset used the static power value calculated by
>> devfreq_cooling_gen_power_table() instead of temperature.
>>
> 
> You've got this wrong.
> 
> The power table is used to model device power consumption which allows
> the device to be used with the power_allocator governor. Refer to the
> power_allocator documentation[2] to understand how it is used.
> 
> This doesn't change the behaviour when used with other governors.
> 
> [2] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/thermal/power_allocator.txt?id=refs/tags/v4.2-rc3

OK. I'll check it.

> 
>> Third,
>> Javi's patchset used the same string of type when calling
>> the thermal_of_cooling_device_register()
>> - Javi's patchset used always the same "devfreq" string.
>> - My patchset used the different "thermal-devfreq-%d" string
>> according to each devfreq cooling device.
>>
> 
> Except for this difference (which needs to be fixed), the current
> patchset is a subset of the functionality proposed in [1].

> 
> With the merging of the power_allocator governor in v4.2, it makes sense
> for the devfreq cooling device to also include support for it -
> especially when the functionality is already under discussion on the
> list.
> 
> As such, it would be great if you could provide feedback on that thread.

First of all, I'm not opposite to apply power_allocator
to cooling device (CPU, Devfreq). I think that thermal framework
may need the basic patch-set for devfreq cooling device
as cpu cooling device without power allocator.

And then the additional patch-set(e.g., to apply power_allocator)
will be implemented on basic patchset. The your proposed
patch-set[1] include the two features with power_allocator. 

Lastly, I want to simplify the devfreq cooling device driver
without unneed/additional functions as I explained on previous reply.

Thanks,
Chanwoo Choi

> 
> Thanks,
> Punit
> 
>> In my patchset, devfreq cooling device uses the same method
>> to determine the throttling situation as existing cpu cooling
>> device. It is just my opinion.
>>
>>>
>>> At first glance, it seems that you are not implementing the extensions
>>> that allow devfreq cooling devices to be used with power_allocator
>>> thermal governor that got merged in v4.2-rc1.
>>>
>>> Thanks,
>>> Punit
>>>
>>> [0] http://article.gmane.org/gmane.linux.power-management.general/61936
>>> [1] http://article.gmane.org/gmane.linux.power-management.general/62417
>>
>> Thanks,
>> Chanwoo Choi
>>
>>>
>>>
>>>> To verify the devfreq cooling device driver, I testd it with following platform:
>>>>
>>>> For example,
>>>> - The Mali GPU of Exynos5433 SoC uses the devfreq framework to support the DVFS
>>>> feature and Exynos5433 contains the G3D (GPU) thermal sensor. Following example
>>>> explain the correlation between mali dt node and thermal sensor/zone.
>>>> : thermal sensor : G3D sensor of Samsung Exynos5433 [1][2]
>>>> : devfreq cooling device : Mali GPU [3]
>>>>
>>>> According to the temperature of g3d thermal sensor inclued in Exynos5433,
>>>> devfreq cooling device can change the maximum frequency of Mali GPU.
>>>>
>>>> 1. In Exynos5433-based board dts file, Mali GPU dt node uses the devfreq
>>>> framework to suppot the DVFS feature. Following dt node includes the
>>>> both 'cooling-cells' and 'operating-points' which means the supported
>>>> frequency entries:
>>>>
>>>> 	mali: mali@14AC0000 {
>>>> 		compatible = "arm,mali-midgard";
>>>> 		reg = <0x14AC0000 0x5000>;
>>>> 		interrupts = <0 282 0>, <0 283 0>, <0 281 0>;
>>>> 		interrupt-names = "JOB", "MMU", "GPU";
>>>> 		clocks = <&cmu_g3d CLK_ACLK_G3D>;
>>>> 		clock-names = "clk_mali";
>>>> 		power-domains = <&pd_g3d>;
>>>> 		status = "disabled";
>>>>
>>>> 		#cooling-cells = <2>;
>>>>
>>>> 		operating-points = <
>>>> 			700000 1150000
>>>> 			600000 1150000
>>>> 			550000 1125000
>>>> 			500000 1075000
>>>> 			420000 1025000
>>>> 			350000 1025000
>>>> 			266000 1000000
>>>> 			160000 1000000
>>>> 		>;
>>>> 	};
>>>>
>>>> 2. In exynos5433.dtsi, G3D thermal sensor measure the temperature of Mali GPU:
>>>>
>>>> 	tmu_g3d: tmu@10070000 {
>>>> 		compatible = "samsung,exynos5433-tmu";
>>>> 		reg = <0x10070000 0x200>;
>>>> 		interrupts = <0 99 0>;
>>>> 		clocks = <&cmu_peris CLK_PCLK_TMU1_APBIF>,
>>>> 			 <&cmu_peris CLK_SCLK_TMU1>;
>>>> 		clock-names = "tmu_apbif", "tmu_sclk";
>>>> 		#include "exynos5433-tmu-sensor-conf.dtsi"
>>>> 		status = "disabled";
>>>> 	};
>>>>
>>>> 3. In exynos5433-tmu.dtsi, thermal-zones includes both trip points and
>>>> cooling-maps of g3d thermal sensor. Following cooling-maps show the match
>>>> between each trip point and each cooling device (devfreq device of mali):
>>>>
>>>> 	thermal-zones {
>>>> 		/* ...... */
>>>> 		g3d_thermal: g3d-thermal {
>>>> 			thermal-sensors = <&tmu_g3d>;
>>>> 			polling-delay-passive = <0>;
>>>> 			polling-delay = <0>;
>>>> 			trips {
>>>> 				g3d_alert_0: g3d-alert-0 {
>>>> 					temperature = <30000>;	/* millicelsius */
>>>> 					hysteresis = <10000>;	/* millicelsius */
>>>> 					type = "active";
>>>> 				};
>>>> 				g3d_alert_1: g3d-alert-1 {
>>>> 					temperature = <40000>;	/* millicelsius */
>>>> 					hysteresis = <10000>;	/* millicelsius */
>>>> 					type = "active";
>>>> 				};
>>>>
>>>> 				/* ...... */
>>>> 			};
>>>>
>>>> 			cooling-maps {
>>>> 				map0 {
>>>> 					/* Set maximum frequency as 550MHz  */
>>>> 					trip = <&g3d_alert_0>;
>>>> 					cooling-device = <&mali 2 2>;
>>>> 				};
>>>> 				map1 {
>>>> 					/* Set maximum frequency as 420MHz  */
>>>> 					trip = <&g3d_alert_1>;
>>>> 					cooling-device = <&mali 4 4>;
>>>> 				};
>>>>
>>>> 				/* ...... */
>>>> 			};
>>>> 		};
>>>>
>>>> 		......
>>>> 	};
>>>>
>>>> [1] https://git.kernel.org/cgit/linux/kernel/git/kgene/linux-samsung.git/commit/?h=v4.3-next/dt64-samsung&id=ac008f6b537703bb9a6fcc3882ca4af3331aa24f
>>>> [2] https://git.kernel.org/cgit/linux/kernel/git/kgene/linux-samsung.git/commit/?h=v4.3-next/dt64-samsung&id=bcddc3a84e49ca1c646cf2081687a544a15f9218
>>>> [3] malideveloper.arm.com/downloads/drivers/TX041/r5p0-06rel0/TX041-SW-99002-r5p0-06rel0.tgz
>>>>
>>>> Chanwoo Choi (2):
>>>>   PM: devfreq: Add the prototype of update_devfreq() to export
>>>>   thermal: devfreq_cooling: Add generic devfreq cooling device implementaion
>>>>
>>>>  .../devicetree/bindings/thermal/thermal.txt        |   8 +-
>>>>  drivers/devfreq/devfreq.c                          |  22 +-
>>>>  drivers/thermal/Kconfig                            |  11 +
>>>>  drivers/thermal/Makefile                           |   3 +
>>>>  drivers/thermal/devfreq-cooling.c                  | 309 +++++++++++++++++++++
>>>>  include/linux/devfreq-cooling.h                    |  80 ++++++
>>>>  include/linux/devfreq.h                            |   7 +
>>>>  7 files changed, 425 insertions(+), 15 deletions(-)
>>>>  create mode 100644 drivers/thermal/devfreq-cooling.c
>>>>  create mode 100644 include/linux/devfreq-cooling.h
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>