mediatek: mt7623: fix thermal zone

[anonimou0]

As discussed on #9396 , using @VA1DER patch posted there and the tests done, I'm trying to create this pull request.

Text from @VA1DER:

The CPU will be throttled all the way down to 98MHz if the temperature rises even a degree above the trip point, and I have further discovered that if the internal temperature of the device is above the first trip point temperature when it boots,

The patch deletes the first two trip points and cooling maps. The throttling temperature will then be at 87°C, which is still a low enough temperature for ARM devices to not be in the real danger zone, and gives some operational headroom.

[dangowrt]

Both active and passive trip points are desirable to have. Simply dropping them is not an option. Instead, we should swap the order (ie. first active == fan, then passive == throttling) and maybe raise the trip-point temperatures a little if needed.
On the formal side this PR also lacks a meaningful commit message as well as Signed-off-by: line.

[anonimou0]

Followed your comment, @dangowrt , and swapped both active and passive trip points

Also raised 10oC on the active trip point and 30oC on the passive trip point.

Tested on UniElec U7623-02 eMMC (512M RAM)

[dangowrt]

Please squash the 3 commits into a single one (eg. using git rebase -i origin/master and then using the fixup or squash commands in the editor) and also write a meaningful commit message including Signed-off-by line (also in the commit for openwrt.git, not just the in the patch file you are adding).
The commit title should start with mediatek: mt7623: ....

See https://openwrt.org/submitting-patches

[anonimou0]

Sorry for bothering and thanks for the guidance, @dangowrt .

I believe I successfully made the changes requested.

[VA1DER]

Hi guys, I apologize for not following this more closely, I had recent eye surgery which prevents me from spending too much time at a screen.

Both active and passive trip points are desirable to have. Simply dropping them is not an option.

I think you are falling into the same trap I originally did. I assumed that since active and passive trip points existed and were named as such that they were doing what they are supposed to do. I thought reversing them would help. This is not the case. What I failed to do is look at what those trip points were actually tripping. The three trip points link to three cooling maps, map0, map1, and map2. All those cooling maps are 100% identical. This means that despite the name "passive", "active" and "hot", these trip points are doing the same thing by triggering identical cooling maps.

Therefore two of the three maps are doing exactly nothing. Having them in is just fooling people into thinking they are there "for a reason".

Until and unless the three cooling maps are changed to actually do something different from each other, the best course of action is to simply delete two of the trip points and delete two of the cooling maps, and have the remaining trip point trip at a reasonable temperature.

[robimarko]

Unless there is an active cooling device like a fan then active and passive are both gonna just end up throttling and that's it.

[dangowrt]

@robimarko Both, the U7623 and the BPi-R2 do have a connector for a PWM-driven fan, so active cooling could be setup and make sense on those devices.

[robimarko]

That's a different thing then, those trip points need to be much higher though

[VA1DER]

Both, the U7623 and the BPi-R2 do have a connector for a PWM-driven fan

Where does the U7623 even have a fan connector? And on the R2, has anyone ever got the fan PWM to work?

In any case, there is certainly no way of activating a fan that is common to both the U7623 and the BPI-R2. I don't believe the appropriate place to have .dts entries for U7623/BPI-R2 fans is in their common platform .dtsi. If it were appropriate to have all three trip points in the .dtsi, it still wouldn't be appropriate to have them all tied to identical cooling maps.

I still suggest the appropriate course of action is to remove two of the trip points and their associated (identical) cooling maps from the .dtsi, and if we get the PWM's working with fans, incorporate what we need into the individual device .dts's

[dangowrt]

@VA1DER Ok, agreed, let's just remove the lowest trip point for now and have new first trippoint at 77 deg C called passive with CPU as cooling device, because that makes most sense.

[anonimou0]

Ok, agreed, let's just remove the lowest trip point for now and have new first trippoint at 77 deg C called passive with CPU as cooling device, because that makes most sense.

Unless I got something wrong, the current push should provide that.

[frank-w]

• It seems you're touching 2 files - ah sorry, it is for 5.10 and 5.15...missed this on mobile phone
• Please squash commits into 1 as this is a single change
• signed off tag have to use real name

[anonimou0]

I will, eventually, get this right, with the rebases.... Sorry for the noise.

Does this look good now?

[anonimou0]

Hello.

Just checking if any more changes are required.

Thanks

[anonimou0]

Author name (anonimou) need to be your real name 'firstname lastname'
Commit subject line seems ok (mediatek: mt7623: thermal zone fix The fix was proposed by @VA1DER at issue 9396.)
Signed-off-by is missing or doesn't match author (should be 'Signed-off-by: anonimou <anonimou_eu@hotmail.com>')

Should this be the right one? Should I change the github name to match what I signed off?

[frank-w]

Change your local name is enough. Look in your ~/.gitconfig

[anonimou0]

Pushed with the right .gitconfig, now?

Sorry for causing all this issue. squashing the commits are being harder than I could figure, hence the force push.

[frank-w]

Imho commit title is too long, usally should take max 75 chars like all lines.

You could change it like the title of this issue

"mediatek: mt7623: Thermal zone fix"

And put your current title down in the description with linebreaks after max 75 chars.

[anonimou0]

Sorry for the long delay on this.

Following @frank-w suggestion, changed the commit messages.

[Ansuel]

@anonimou0 yes if in doubt just post the full patch on pastebin and post the link here... anyway you should not modify the output from git format-patch

[anonimou0]

I pushed with the output of git format-patch already

[Ansuel]

@anonimou0 REALLY NICE! now just /target/linux/refresh push the final version and we are good to go !

[anonimou0]

@Ansuel there is no /target/linux/refresh file and make target/linux/refresh and make target/linux/update both fail without a recipe

[Ansuel]

what do you mean without a recipe?

[anonimou0]

what do you mean without a recipe?

My bad. I was running the wrong command.

Should I finally push just the patch file or all of them?

	modified:   target/linux/generic/backport-5.15/005-v5.17-01-Kbuild-use-Wdeclaration-after-statement.patch
	modified:   target/linux/generic/backport-5.15/005-v5.17-02-Kbuild-move-to-std-gnu11.patch
	modified:   target/linux/generic/backport-5.15/005-v5.17-03-Kbuild-use-std-gnu11-for-KBUILD_USERCFLAGS.patch
	modified:   target/linux/generic/backport-5.15/011-kbuild-export-SUBARCH.patch
	modified:   target/linux/generic/hack-5.15/204-module_strip.patch
	modified:   target/linux/mediatek/patches-5.15/193-dts-mt7623-thermal_zone_fix.patch

[Ansuel]

@anonimou0 only the relevant patch pls

[anonimou0]

Done? Maybe?

[Ansuel]

@anonimou0 it was a long journey but we manage to make it. Thanks for following instructions and not getting annoyed by me ahahah.

[anonimou0]

@Ansuel, no annoyance at all. Actually, I felt the other way, annoying you guys. Thanks for the patience. Learned a lot on the way.

Thanks @Ansuel , @dangowrt , @frank-w , @VA1DER