Re: [PATCH v3] arm64: dts: mediatek: mt8183: disable thermal zones without trips.

[Daniel Lezcano]

On 06/10/2022 13:08, Amjad Ouled-Ameur wrote:
> Hi Daniel,
>
> Thank you for your feedback.
>
> On 10/4/22 12:47, Daniel Lezcano wrote:
> > Hi Amjad,
> >
> > On 04/10/2022 12:11, Amjad Ouled-Ameur wrote:
> > > Thermal zones without trip point are not registered by thermal core.
> > >
> > > tzts1 ~ tzts6 zones of mt8183 were intially introduced for test-purpose
> > > only.
> > >
> > > Disable the zones above and keep only cpu_thermal enabled.
> >
> > It does not make sense to disable the thermal zones. Either the 
> > thermal zones are needed or they are not. Keeping them for debug 
> > purpose is not desired.
> As Matthias Brugger mentioned in previous versions, DTS should describe 
> the HW as it is, the sensors are in the HW.

Yes, it is here:

		thermal: thermal@1100b000 {
                        #thermal-sensor-cells = <1>;
                        compatible = "mediatek,mt8183-thermal";
                        reg = <0 0x1100b000 0 0x1000>;
                        clocks = <&infracfg CLK_INFRA_THERM>,
                                 <&infracfg CLK_INFRA_AUXADC>;
	                clock-names = "therm", "auxadc";
                        resets = <&infracfg 
MT8183_INFRACFG_AO_THERM_SW_RST>;
                        interrupts = <0 76 IRQ_TYPE_LEVEL_LOW>;
                        mediatek,auxadc = <&auxadc>;
		        mediatek,apmixedsys = <&apmixedsys>;
                        nvmem-cells = <&thermal_calibration>;
			nvmem-cell-names = "calibration-data";
		};

> > Alternatively to removal, you can:
> >
> >  - remove 'sustainable-power'
> >  - add a passive trip point, optionally a hot trip point and a 
> > critical trip point
>
> Why removing "sustainable-power" instead of simply disabling the device 
> ? 

Because sustainable-power is wrong. It is probably coming from a 
copy-paste. It does not make sense to have this.

The sustainable-power is for the power allocator governor which is 
limited to CPU and GPU. Here the thermal zones are for different devices.

Especially that, if a user needs to use the sensor

If the thermal zone is disabled, how can it use the sensor?

> in the future, they might not be able to find the right 
> sustainable-power ; thus I think it should remain the way it is.
>
> As to adding tripping points, MediaTek does not have ones to add for now 
> for those sensors.

Please do this:

Add:

trips {
       threshold : trip1 {
           	temperature = <50000>;
                type = "passive";
          };
};

And remove all the empty cooling maps and the sustainable power properties.


> > The passive trip point will allow the userspace to set a value in 
> > order to get notified about the devices temperature (writable trip 
> > point). The hot temperature will send a notification to userspace so 
> > it can take a last chance decision to drop the temperature before the 
> > critical temperature.
> >
> > The passive trip point temperature could be a high temperature.
> >
> > The mitigation is also managed from userspace as a whole.
> >
> >
> > > Signed-off-by: Amjad Ouled-Ameur <aouledameur@xxxxxxxxxxxx>
> > > ---
> > >   arch/arm64/boot/dts/mediatek/mt8183.dtsi | 6 ++++++
> > >   1 file changed, 6 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi 
> > > b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> > > index 9d32871973a2..53f7a0fbaa88 100644
> > > --- a/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> > > +++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> > > @@ -1191,6 +1191,7 @@ tzts1: tzts1 {
> > >                   polling-delay = <0>;
> > >                   thermal-sensors = <&thermal 1>;
> > >                   sustainable-power = <5000>;
> > > +                status = "disabled";
> > >                   trips {};
> > >                   cooling-maps {};
> > >               };
> > > @@ -1200,6 +1201,7 @@ tzts2: tzts2 {
> > >                   polling-delay = <0>;
> > >                   thermal-sensors = <&thermal 2>;
> > >                   sustainable-power = <5000>;
> > > +                status = "disabled";
> > >                   trips {};
> > >                   cooling-maps {};
> > >               };
> > > @@ -1209,6 +1211,7 @@ tzts3: tzts3 {
> > >                   polling-delay = <0>;
> > >                   thermal-sensors = <&thermal 3>;
> > >                   sustainable-power = <5000>;
> > > +                status = "disabled";
> > >                   trips {};
> > >                   cooling-maps {};
> > >               };
> > > @@ -1218,6 +1221,7 @@ tzts4: tzts4 {
> > >                   polling-delay = <0>;
> > >                   thermal-sensors = <&thermal 4>;
> > >                   sustainable-power = <5000>;
> > > +                status = "disabled";
> > >                   trips {};
> > >                   cooling-maps {};
> > >               };
> > > @@ -1227,6 +1231,7 @@ tzts5: tzts5 {
> > >                   polling-delay = <0>;
> > >                   thermal-sensors = <&thermal 5>;
> > >                   sustainable-power = <5000>;
> > > +                status = "disabled";
> > >                   trips {};
> > >                   cooling-maps {};
> > >               };
> > > @@ -1236,6 +1241,7 @@ tztsABB: tztsABB {
> > >                   polling-delay = <0>;
> > >                   thermal-sensors = <&thermal 6>;
> > >                   sustainable-power = <5000>;
> > > +                status = "disabled";
> > >                   trips {};
> > >                   cooling-maps {};
> > >               };


--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog