[XL] Change in coreboot[main]: soc/mediatek/mt8196: Add thermal driver

[Paul Menzel (Code Review)]

Attention is currently required from: Hung-Te Lin, Jarried Lin, Yu-Ping Wu.

Paul Menzel has posted comments on this change by Jarried Lin. ( https://review.coreboot.org/c/coreboot/+/86017?usp=email )

The change is no longer submittable: All-Comments-Resolved is unsatisfied now.

Change subject: soc/mediatek/mt8196: Add thermal driver
......................................................................

Patch Set 8: Code-Review+1

(17 comments)

Patchset:

PS8: 
Looks good overall. It’d be great if the log messages could be improved, and also some variable types be changed to use non-fixed length types where possible.

Commit Message:

https://review.coreboot.org/c/coreboot/+/86017/comment/63cefa58_a7fa6d67?usp... :
PS8, Line 9: Add thermal driver to support LVTS (Low Voltage Thermal Sensor).
This seems new for MT8196. The other SOC do not have such a driver. Could you please add some more background what changed?

https://review.coreboot.org/c/coreboot/+/86017/comment/58e00223_ba08c7c6?usp... :
PS8, Line 10: 
Why is a separate SRAM driver needed?

https://review.coreboot.org/c/coreboot/+/86017/comment/618f79e4_418b5fa1?usp... :
PS8, Line 11: TEST=Check temperatures read from each sensors.
Please paste the new log messages.

https://review.coreboot.org/c/coreboot/+/86017/comment/f072efcd_6e9cd9cf?usp... :
PS8, Line 11: TEST=Check temperatures read from each sensors.
With the command `sensors`?

File src/soc/mediatek/mt8196/include/soc/thermal_internal.h:

https://review.coreboot.org/c/coreboot/+/86017/comment/fdb51304_5a50f55c?usp... :
PS8, Line 15: /*
Add a blank line above?

https://review.coreboot.org/c/coreboot/+/86017/comment/c82854bc_d9720d02?usp... :
PS8, Line 16:  * module            LVTS Plan
Move *LVTS Plan* more to the right?

File src/soc/mediatek/mt8196/thermal.c:

https://review.coreboot.org/c/coreboot/+/86017/comment/fa136c70_f2e54360?usp... :
PS8, Line 38:  */
To avoid duplication, the comment should just be in one file?

https://review.coreboot.org/c/coreboot/+/86017/comment/7cd5ba3c_4c5bc135?usp... :
PS8, Line 163: 	printk(BIOS_INFO, "%s: msr_raw=%u, temp_mc=%d\n", __func__, msr_raw, temp_mc);
Should be a debug message, or rewritten?

https://review.coreboot.org/c/coreboot/+/86017/comment/31214c9a_3343f33f?usp... :
PS8, Line 170: 	uint32_t i;
size_t?

https://review.coreboot.org/c/coreboot/+/86017/comment/f3ddb0d3_1786d9cc?usp... :
PS8, Line 234: 	int i;
size_t

https://review.coreboot.org/c/coreboot/+/86017/comment/c0e175bd_5f23357a?usp... :
PS8, Line 268: 	int i;
size_t

https://review.coreboot.org/c/coreboot/+/86017/comment/2e97dde9_3478b79b?usp... :
PS8, Line 271: 	printk(BIOS_INFO, "%s\n", __func__);
Looks like a debug message.

https://review.coreboot.org/c/coreboot/+/86017/comment/6def15ad_a7dc4f69?usp... :
PS8, Line 375: 		 * to wait HW init done,
to wait *until* HW init *is* done,

https://review.coreboot.org/c/coreboot/+/86017/comment/f1a3f67a_20a695ec?usp... :
PS8, Line 376: translates
translate

https://review.coreboot.org/c/coreboot/+/86017/comment/09854128_b894e73a?usp... :
PS8, Line 379: finish
finishes

https://review.coreboot.org/c/coreboot/+/86017/comment/bccf043f_318b9406?usp... :
PS8, Line 373: * 26111 is magic num
             : 		 * this is to keep system alive for a while
             : 		 * to wait HW init done,
             : 		 * because 0 msr raw will translates to 28x'C
             : 		 * and then 28x'C will trigger a SW reset.
             : 		 *
             : 		 * if HW init finish, this msr raw will not be 0,
             : 		 * system can report normal temperature.
             : 		 * if wait over 60 times zero, this means something
             : 		 * wrong with HW, must trigger BUG on and dump useful
             : 		 * register for debug.
Re-flow the text?