RE: [PATCH v2] arm64: dts: ls1088a: add one more thermal zone node

[Andy Tang]

> -----Original Message-----
> From: Shawn Guo <shawnguo@xxxxxxxxxx>
> Sent: 2019年3月4日 14:21
> To: Andy Tang <andy.tang@xxxxxxx>
> Cc: Leo Li <leoyang.li@xxxxxxx>; robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; linux-pm@xxxxxxxxxxxxxxx;
> daniel.lezcano@xxxxxxxxxx; rui.zhang@xxxxxxxxx; edubezval@xxxxxxxxx
> Subject: Re: [PATCH v2] arm64: dts: ls1088a: add one more thermal zone node
> 
> On Mon, Mar 04, 2019 at 11:21:11AM +0800, Yuantian Tang wrote:
> > Ls1088a has 2 thermal sensors, core cluster and SoC platform. Core
> > cluster sensor is used to monitor the temperature of core and SoC
> > platform is for platform. The current dts only support the first sensor.
> > This patch adds the second sensor node to dts to enable it.
> >
> > Signed-off-by: Yuantian Tang <andy.tang@xxxxxxx>
> > ---
> > v2:
> > 	- Add more information about sensors to description
> > PS: In order to keep consistency to the first thermal-zone node, there
> > will be "WARNING: line over 80 characters" warnings.
> >
> >  arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi |   43
> +++++++++++++++++++++--
> >  1 files changed, 39 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi
> > b/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi
> > index 661137f..9f52bc9 100644
> > --- a/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi
> > @@ -129,19 +129,19 @@
> >  	};
> >
> >  	thermal-zones {
> > -		cpu_thermal: cpu-thermal {
> > +		ccu {
> 
> Is this change really necessary?  What does 'ccu' stand for?
I think so. ccu stands for core cluster unit. cpu is too general.
On some platforms, there are more than one core clusters.
At least we should change it to "core cluster" if short form is not appropriate.

> 
> >  			polling-delay-passive = <1000>;
> >  			polling-delay = <5000>;
> >  			thermal-sensors = <&tmu 0>;
> >
> >  			trips {
> > -				cpu_alert: cpu-alert {
> > +				ccu_alert: ccu-alert {
> >  					temperature = <85000>;
> >  					hysteresis = <2000>;
> >  					type = "passive";
> >  				};
> >
> > -				cpu_crit: cpu-crit {
> > +				ccu_crit: ccu-crit {
> >  					temperature = <95000>;
> >  					hysteresis = <2000>;
> >  					type = "critical";
> > @@ -150,7 +150,42 @@
> >
> >  			cooling-maps {
> >  				map0 {
> > -					trip = <&cpu_alert>;
> > +					trip = <&ccu_alert>;
> > +					cooling-device =
> > +						<&cpu0 THERMAL_NO_LIMIT
> THERMAL_NO_LIMIT>,
> > +						<&cpu1 THERMAL_NO_LIMIT
> THERMAL_NO_LIMIT>,
> > +						<&cpu2 THERMAL_NO_LIMIT
> THERMAL_NO_LIMIT>,
> > +						<&cpu3 THERMAL_NO_LIMIT
> THERMAL_NO_LIMIT>,
> > +						<&cpu4 THERMAL_NO_LIMIT
> THERMAL_NO_LIMIT>,
> > +						<&cpu5 THERMAL_NO_LIMIT
> THERMAL_NO_LIMIT>,
> > +						<&cpu6 THERMAL_NO_LIMIT
> THERMAL_NO_LIMIT>,
> > +						<&cpu7 THERMAL_NO_LIMIT
> THERMAL_NO_LIMIT>;
> > +				};
> > +			};
> > +		};
> > +
> > +		plt {
> 
> What about 'platform-thermal' for node name, platform-alert and platform-crit
> for trip nodes below?
OK, will use long name form.

BR,
Andy
> 
> Shawn
> 
> > +			polling-delay-passive = <1000>;
> > +			polling-delay = <5000>;
> > +			thermal-sensors = <&tmu 1>;
> > +
> > +			trips {
> > +				plt_alert: plt-alert {
> > +					temperature = <85000>;
> > +					hysteresis = <2000>;
> > +					type = "passive";
> > +				};
> > +
> > +				plt_crit: plt-crit {
> > +					temperature = <95000>;
> > +					hysteresis = <2000>;
> > +					type = "critical";
> > +				};
> > +			};
> > +
> > +			cooling-maps {
> > +				map0 {
> > +					trip = <&plt_alert>;
> >  					cooling-device =
> >  						<&cpu0 THERMAL_NO_LIMIT
> THERMAL_NO_LIMIT>,
> >  						<&cpu1 THERMAL_NO_LIMIT
> THERMAL_NO_LIMIT>,
> > --
> > 1.7.1
> >