diff mbox series

Aw: Re: Re: [PATCH] Fix mt7622.dtsi thermal cpu

Message ID trinity-2eb7c0ac-d9dc-446c-8907-69b5f4df6838-1624618996538@3c-app-gmx-bs66 (mailing list archive)
State New, archived
Headers show
Series Aw: Re: Re: [PATCH] Fix mt7622.dtsi thermal cpu | expand

Commit Message

Frank Wunderlich June 25, 2021, 11:03 a.m. UTC
Hi

> Gesendet: Freitag, 25. Juni 2021 um 11:57 Uhr
> Von: "Daniel Lezcano" <daniel.lezcano@linaro.org>

> You should not add the fan in the mt7622.dtsi itself but in the board
> specific file where there is a fan output on it. mt7622.dtsi is supposed
> to be the SoC itself AFAICT.
>
> For instance:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399-sapphire.dtsi#n39
>
> https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399-sapphire.dtsi#n164

> > @@ -170,14 +177,12 @@
> >  			cooling-maps {
> >  				map0 {
> >  					trip = <&cpu_passive>;
> > -					cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> > -							 <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> > +					cooling-device = <&fan0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> >  				};
>
> fan == active trip point
>
> This is referring to the passive trip point. So it should point to the
> CPU as it is now. Note the order of mitigation is inverted regarding the
> proposal description.

but we need to disable the passive trip as cpu-trotteling starts there...the higher temperature trips are currently not reached

summary

moving fan and cpu_thermal-override to bananapi-r64.dts

passive-trip: cooling-device = <&cpu0/1 0 0> as in erics Patch
active trip: cooling-device = <&fan0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
the other 2 unchanged

but i suggest changing the temperature points in mt7622 dtsi as this is SoC specific

so basicly:

Comments

Eric Woudstra June 25, 2021, 11:07 a.m. UTC | #1
I choose "hot* with CPU, because it was the best temperature. But it should really be passive only with the cooling device CPU but with a much higher temperature. For me 87 degrees is fine and tested. But for mainline we would better ask Mediatek for the correct maximum temperature.

⁣Get BlueMail for Android ​

On Jun 25, 2021, 1:03 PM, at 1:03 PM, Frank Wunderlich <frank-w@public-files.de> wrote:
>Hi
>
>> Gesendet: Freitag, 25. Juni 2021 um 11:57 Uhr
>> Von: "Daniel Lezcano" <daniel.lezcano@linaro.org>
>
>> You should not add the fan in the mt7622.dtsi itself but in the board
>> specific file where there is a fan output on it. mt7622.dtsi is
>supposed
>> to be the SoC itself AFAICT.
>>
>> For instance:
>>
>>
>https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399-sapphire.dtsi#n39
>>
>>
>https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399-sapphire.dtsi#n164
>
>> > @@ -170,14 +177,12 @@
>> >  			cooling-maps {
>> >  				map0 {
>> >  					trip = <&cpu_passive>;
>> > -					cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
>> > -							 <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
>> > +					cooling-device = <&fan0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
>> >  				};
>>
>> fan == active trip point
>>
>> This is referring to the passive trip point. So it should point to
>the
>> CPU as it is now. Note the order of mitigation is inverted regarding
>the
>> proposal description.
>
>but we need to disable the passive trip as cpu-trotteling starts
>there...the higher temperature trips are currently not reached
>
>summary
>
>moving fan and cpu_thermal-override to bananapi-r64.dts
>
>passive-trip: cooling-device = <&cpu0/1 0 0> as in erics Patch
>active trip: cooling-device = <&fan0 THERMAL_NO_LIMIT
>THERMAL_NO_LIMIT>;
>the other 2 unchanged
>
>but i suggest changing the temperature points in mt7622 dtsi as this is
>SoC specific
>
>so basicly:
>
>--- a/arch/arm64/boot/dts/mediatek/mt7622.dtsi
>+++ b/arch/arm64/boot/dts/mediatek/mt7622.dtsi
>@@ -143,13 +143,13 @@ cpu_thermal: cpu-thermal {
>
>                        trips {
>                                cpu_passive: cpu-passive {
>-                                       temperature = <47000>;
>+                                       temperature = <70000>;
>                                        hysteresis = <2000>;
>                                        type = "passive";
>                                };
>
>                                cpu_active: cpu-active {
>-                                       temperature = <67000>;
>+                                       temperature = <80000>;
>                                        hysteresis = <2000>;
>                                        type = "active";
>                                };
>@@ -170,8 +170,8 @@ cpu-crit {
>                        cooling-maps {
>                                map0 {
>                                        trip = <&cpu_passive>;
>-                                       cooling-device = <&cpu0
>THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
>-                                                        <&cpu1
>THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
>+                                       cooling-device = <&cpu0 0 0>,
>+                                                        <&cpu1 0 0>;
>                                };
>
>                                map1 {
>@@ -428,6 +428,7 @@ uart3: serial@11005000 {
>        pwm: pwm@11006000 {
>                compatible = "mediatek,mt7622-pwm";
>                reg = <0 0x11006000 0 0x1000>;
>+               #pwm-cells = <3>;
>                interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_LOW>;
>                clocks = <&topckgen CLK_TOP_PWM_SEL>,
>                         <&pericfg CLK_PERI_PWM_PD>,
>
>--- a/arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts
>+++ b/arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts
>@@ -37,6 +37,13 @@ cpu@1 {
>                };
>        };
>
>+       fan0: pwm-fan {
>+               compatible = "pwm-fan";
>+               #cooling-cells = <2>;
>+               pwms = <&pwm 2 10000 0>;
>+               cooling-levels = <0 102 170 230>;
>+       };
>+
>        gpio-keys {
>                compatible = "gpio-keys";
>
>@@ -582,6 +589,29 @@ &u3phy {
>        status = "okay";
> };
>
>+&cpu_thermal {
>+       cooling-maps {
>+               map1 {
>+                       trip = <&cpu_active>;
>+                       cooling-device = <&fan0 THERMAL_NO_LIMIT
>THERMAL_NO_LIMIT>;
>+               };
>+       };
>+};
>+
> &uart0 {
>        pinctrl-names = "default";
>        pinctrl-0 = <&uart0_pins>;
Daniel Lezcano June 25, 2021, 11:47 a.m. UTC | #2
On 25/06/2021 13:03, Frank Wunderlich wrote:
> Hi
> 
>> Gesendet: Freitag, 25. Juni 2021 um 11:57 Uhr
>> Von: "Daniel Lezcano" <daniel.lezcano@linaro.org>
> 
>> You should not add the fan in the mt7622.dtsi itself but in the board
>> specific file where there is a fan output on it. mt7622.dtsi is supposed
>> to be the SoC itself AFAICT.
>>
>> For instance:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399-sapphire.dtsi#n39
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399-sapphire.dtsi#n164
> 
>>> @@ -170,14 +177,12 @@
>>>  			cooling-maps {
>>>  				map0 {
>>>  					trip = <&cpu_passive>;
>>> -					cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
>>> -							 <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
>>> +					cooling-device = <&fan0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
>>>  				};
>>
>> fan == active trip point
>>
>> This is referring to the passive trip point. So it should point to the
>> CPU as it is now. Note the order of mitigation is inverted regarding the
>> proposal description.
> 
> but we need to disable the passive trip as cpu-trotteling starts there...the higher temperature trips are currently not reached

Sorry, can you rephrase it ? I'm not getting the point.


> summary
> 
> moving fan and cpu_thermal-override to bananapi-r64.dts
> 
> passive-trip: cooling-device = <&cpu0/1 0 0> as in erics Patch
> active trip: cooling-device = <&fan0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> the other 2 unchanged
> 
> but i suggest changing the temperature points in mt7622 dtsi as this is SoC specific
> 
> so basicly:
> 
> --- a/arch/arm64/boot/dts/mediatek/mt7622.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt7622.dtsi
> @@ -143,13 +143,13 @@ cpu_thermal: cpu-thermal {
> 
>                         trips {
>                                 cpu_passive: cpu-passive {
> -                                       temperature = <47000>;
> +                                       temperature = <70000>;

May be increase the passive temp to 75°C.

>                                         hysteresis = <2000>;
>                                         type = "passive";
>                                 };
> 
>                                 cpu_active: cpu-active {
> -                                       temperature = <67000>;
> +                                       temperature = <80000>;
>                                         hysteresis = <2000>;
>                                         type = "active";
>                                 };

Move the active trip 'cpu_active' to mt7622-bananapi-bpi-r64.dts. and
set it to 70°C in the mt7622-bananapi-bpi-r64.dts, so the fan will act
before the cpu throttling.

The behavior should be the following: The temperature reaches 70°C, the
fan will start, if the temperature continues to increase, it will
increase the speed. If the temperature reaches 75°C, the fan is still
rotating at full speed but the cpu begins to be throttled.

AFAIU, it is a Cortex-A53 running @1.35GH, so except the board is in a
black metal box under the sun, I don't see how we can reach this thermal
limits.

> @@ -170,8 +170,8 @@ cpu-crit {
>                         cooling-maps {
>                                 map0 {
>                                         trip = <&cpu_passive>;
> -                                       cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> -                                                        <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> +                                       cooling-device = <&cpu0 0 0>,
> +                                                        <&cpu1 0 0>;

You should keep it untouched.

>                                 };
> 
>                                 map1 {
> @@ -428,6 +428,7 @@ uart3: serial@11005000 {
>         pwm: pwm@11006000 {
>                 compatible = "mediatek,mt7622-pwm";
>                 reg = <0 0x11006000 0 0x1000>;
> +               #pwm-cells = <3>;
>                 interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_LOW>;
>                 clocks = <&topckgen CLK_TOP_PWM_SEL>,
>                          <&pericfg CLK_PERI_PWM_PD>,
> 
> --- a/arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts
> +++ b/arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts
> @@ -37,6 +37,13 @@ cpu@1 {
>                 };
>         };
> 
> +       fan0: pwm-fan {
> +               compatible = "pwm-fan";
> +               #cooling-cells = <2>;
> +               pwms = <&pwm 2 10000 0>;
> +               cooling-levels = <0 102 170 230>;
> +       };
> +
>         gpio-keys {
>                 compatible = "gpio-keys";
> 
> @@ -582,6 +589,29 @@ &u3phy {
>         status = "okay";
>  };
> 
> +&cpu_thermal {
> +       cooling-maps {
> +               map1 {
> +                       trip = <&cpu_active>;
> +                       cooling-device = <&fan0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> +               };
> +       };
> +};
> +
>  &uart0 {
>         pinctrl-names = "default";
>         pinctrl-0 = <&uart0_pins>;
>
Frank Wunderlich June 25, 2021, 12:28 p.m. UTC | #3
> Gesendet: Freitag, 25. Juni 2021 um 13:47 Uhr
> Von: "Daniel Lezcano" <daniel.lezcano@linaro.org>

> > but we need to disable the passive trip as cpu-trotteling starts there...the higher temperature trips are currently not reached
> 
> Sorry, can you rephrase it ? I'm not getting the point.

the problem currently is that passive is at 47degress Celsius and trottles cpu, active (67°C) and hot points are never reached this way. so at least we need to change temperatures in dtsi, and maybe disable cpu-trotteling on passive trip. imho fan will never start if it is in active and cpu is trottled before in passive

> > summary
> > 
> > moving fan and cpu_thermal-override to bananapi-r64.dts
> > 
> > passive-trip: cooling-device = <&cpu0/1 0 0> as in erics Patch
> > active trip: cooling-device = <&fan0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> > the other 2 unchanged
> > 
> > but i suggest changing the temperature points in mt7622 dtsi as this is SoC specific
> > 
> > so basicly:
> > 
> > --- a/arch/arm64/boot/dts/mediatek/mt7622.dtsi
> > +++ b/arch/arm64/boot/dts/mediatek/mt7622.dtsi
> > @@ -143,13 +143,13 @@ cpu_thermal: cpu-thermal {
> > 
> >                         trips {
> >                                 cpu_passive: cpu-passive {
> > -                                       temperature = <47000>;
> > +                                       temperature = <70000>;
> 
> May be increase the passive temp to 75°C.
> 
> >                                         hysteresis = <2000>;
> >                                         type = "passive";
> >                                 };
> > 
> >                                 cpu_active: cpu-active {
> > -                                       temperature = <67000>;
> > +                                       temperature = <80000>;
> >                                         hysteresis = <2000>;
> >                                         type = "active";
> >                                 };
> 
> Move the active trip 'cpu_active' to mt7622-bananapi-bpi-r64.dts. and
> set it to 70°C in the mt7622-bananapi-bpi-r64.dts, so the fan will act
> before the cpu throttling.
> 
> The behavior should be the following: The temperature reaches 70°C, the
> fan will start, if the temperature continues to increase, it will
> increase the speed. If the temperature reaches 75°C, the fan is still
> rotating at full speed but the cpu begins to be throttled.

passive to 75 and active lower to 70? is this as intended that active comes before passive?

mt7622-bananapi-bpi-r64.dts:

&cpu_thermal {
	trips {
		cpu_passive: cpu-passive {
			temperature = <75000>;
			hysteresis = <2000>;
			type = "passive";
		};

		cpu_active: cpu-active {
			temperature = <70000>;
			hysteresis = <2000>;
			type = "active";
		};
	};

	cooling-maps {
		map1 {
			trip = <&cpu_active>;
			cooling-device = <&fan0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
		};
	};
};


> AFAIU, it is a Cortex-A53 running @1.35GH, so except the board is in a
> black metal box under the sun, I don't see how we can reach this thermal
> limits.
> 
> > @@ -170,8 +170,8 @@ cpu-crit {
> >                         cooling-maps {
> >                                 map0 {
> >                                         trip = <&cpu_passive>;
> > -                                       cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> > -                                                        <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> > +                                       cooling-device = <&cpu0 0 0>,
> > +                                                        <&cpu1 0 0>;
> 
> You should keep it untouched.

then cpu is trottled at passive point (currently 47°C) and imho fan does not start at active

> >                                 };
Daniel Lezcano June 25, 2021, 12:50 p.m. UTC | #4
On 25/06/2021 14:28, Frank Wunderlich wrote:
>> Gesendet: Freitag, 25. Juni 2021 um 13:47 Uhr
>> Von: "Daniel Lezcano" <daniel.lezcano@linaro.org>
> 
>>> but we need to disable the passive trip as cpu-trotteling starts there...the higher temperature trips are currently not reached
>>
>> Sorry, can you rephrase it ? I'm not getting the point.
> 
> the problem currently is that passive is at 47degress Celsius and
> trottles cpu, active (67°C) and hot points are never reached this way.
> so at least we need to change temperatures in dtsi, and maybe disable
> cpu-trotteling on passive trip. imho fan will never start if it is in
> active and cpu is trottled before in passive

Ok, thanks for the clarification.

>>> summary
>>>
>>> moving fan and cpu_thermal-override to bananapi-r64.dts
>>>
>>> passive-trip: cooling-device = <&cpu0/1 0 0> as in erics Patch
>>> active trip: cooling-device = <&fan0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
>>> the other 2 unchanged
>>>
>>> but i suggest changing the temperature points in mt7622 dtsi as this is SoC specific
>>>
>>> so basicly:
>>>
>>> --- a/arch/arm64/boot/dts/mediatek/mt7622.dtsi
>>> +++ b/arch/arm64/boot/dts/mediatek/mt7622.dtsi
>>> @@ -143,13 +143,13 @@ cpu_thermal: cpu-thermal {
>>>
>>>                         trips {
>>>                                 cpu_passive: cpu-passive {
>>> -                                       temperature = <47000>;
>>> +                                       temperature = <70000>;
>>
>> May be increase the passive temp to 75°C.
>>
>>>                                         hysteresis = <2000>;
>>>                                         type = "passive";
>>>                                 };
>>>
>>>                                 cpu_active: cpu-active {
>>> -                                       temperature = <67000>;
>>> +                                       temperature = <80000>;
>>>                                         hysteresis = <2000>;
>>>                                         type = "active";
>>>                                 };
>>
>> Move the active trip 'cpu_active' to mt7622-bananapi-bpi-r64.dts. and
>> set it to 70°C in the mt7622-bananapi-bpi-r64.dts, so the fan will act
>> before the cpu throttling.
>>
>> The behavior should be the following: The temperature reaches 70°C, the
>> fan will start, if the temperature continues to increase, it will
>> increase the speed. If the temperature reaches 75°C, the fan is still
>> rotating at full speed but the cpu begins to be throttled.
> 
> passive to 75 and active lower to 70? is this as intended that active comes before passive?

Yes. So there is a default passive mitigation temp for the SoC at 75°C.
And the bpi has a setup with a fan mitigating before the cpu throttling.

> mt7622-bananapi-bpi-r64.dts:
> 
> &cpu_thermal {
> 	trips {
> 		cpu_passive: cpu-passive {
> 			temperature = <75000>;
> 			hysteresis = <2000>;
> 			type = "passive";
> 		};

No need to add this trip point, it should be changed to 75°C in SoC DT
mt7622.dtsi. This fragment of DT will concatenate with the previous one.

> 		cpu_active: cpu-active {
> 			temperature = <70000>;
> 			hysteresis = <2000>;
> 			type = "active";
> 		};
> 	};
> 
> 	cooling-maps {
> 		map1 {
> 			trip = <&cpu_active>;
> 			cooling-device = <&fan0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> 		};
> 	};
> };
> 
> 
>> AFAIU, it is a Cortex-A53 running @1.35GH, so except the board is in a
>> black metal box under the sun, I don't see how we can reach this thermal
>> limits.
>>
>>> @@ -170,8 +170,8 @@ cpu-crit {
>>>                         cooling-maps {
>>>                                 map0 {
>>>                                         trip = <&cpu_passive>;
>>> -                                       cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
>>> -                                                        <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
>>> +                                       cooling-device = <&cpu0 0 0>,
>>> +                                                        <&cpu1 0 0>;
>>
>> You should keep it untouched.
> 
> then cpu is trottled at passive point (currently 47°C) and imho fan does not start at active
> 
>>>                                 };
>
diff mbox series

Patch

--- a/arch/arm64/boot/dts/mediatek/mt7622.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt7622.dtsi
@@ -143,13 +143,13 @@  cpu_thermal: cpu-thermal {

                        trips {
                                cpu_passive: cpu-passive {
-                                       temperature = <47000>;
+                                       temperature = <70000>;
                                        hysteresis = <2000>;
                                        type = "passive";
                                };

                                cpu_active: cpu-active {
-                                       temperature = <67000>;
+                                       temperature = <80000>;
                                        hysteresis = <2000>;
                                        type = "active";
                                };
@@ -170,8 +170,8 @@  cpu-crit {
                        cooling-maps {
                                map0 {
                                        trip = <&cpu_passive>;
-                                       cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-                                                        <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+                                       cooling-device = <&cpu0 0 0>,
+                                                        <&cpu1 0 0>;
                                };

                                map1 {
@@ -428,6 +428,7 @@  uart3: serial@11005000 {
        pwm: pwm@11006000 {
                compatible = "mediatek,mt7622-pwm";
                reg = <0 0x11006000 0 0x1000>;
+               #pwm-cells = <3>;
                interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_LOW>;
                clocks = <&topckgen CLK_TOP_PWM_SEL>,
                         <&pericfg CLK_PERI_PWM_PD>,

--- a/arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts
+++ b/arch/arm64/boot/dts/mediatek/mt7622-bananapi-bpi-r64.dts
@@ -37,6 +37,13 @@  cpu@1 {
                };
        };

+       fan0: pwm-fan {
+               compatible = "pwm-fan";
+               #cooling-cells = <2>;
+               pwms = <&pwm 2 10000 0>;
+               cooling-levels = <0 102 170 230>;
+       };
+
        gpio-keys {
                compatible = "gpio-keys";

@@ -582,6 +589,29 @@  &u3phy {
        status = "okay";
 };

+&cpu_thermal {
+       cooling-maps {
+               map1 {
+                       trip = <&cpu_active>;
+                       cooling-device = <&fan0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+               };
+       };
+};
+
 &uart0 {
        pinctrl-names = "default";
        pinctrl-0 = <&uart0_pins>;


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek