Attention is currently required from: Hung-Te Lin, Jarried Lin, Paul Menzel, Yidi Lin, Yu-Ping Wu.
Zhaoqing Jiu has posted comments on this change by Jarried Lin. ( https://review.coreboot.org/c/coreboot/+/86017?usp=email )
Change subject: soc/mediatek/mt8196: Add thermal driver ......................................................................
Patch Set 10:
(14 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/86017/comment/00160cf8_15adc8c2?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. […]
The other SOC also have such driver, but only have kernel part. For MT8196, moved driver port of driver initialization to coreboot.
https://review.coreboot.org/c/coreboot/+/86017/comment/b521367d_1fc916bd?usp... : PS8, Line 10:
Why is a separate SRAM driver needed?
SRAM is used to communicate with micro process, set default value in coreboot.
https://review.coreboot.org/c/coreboot/+/86017/comment/b9f78edc_13bf70e0?usp... : PS8, Line 11: TEST=Check temperatures read from each sensors.
With the command `sensors`?
Just check the coreboot log
Commit Message:
https://review.coreboot.org/c/coreboot/+/86017/comment/21f09105_bee02247?usp... : PS9, Line 12: [0m
Remove these control codes.
Done
File src/soc/mediatek/mt8196/include/soc/thermal_internal.h:
https://review.coreboot.org/c/coreboot/+/86017/comment/12c3b40d_e7458fc1?usp... : PS8, Line 15: /*
Add a blank line above?
Done
https://review.coreboot.org/c/coreboot/+/86017/comment/42b5ff33_3f5b7537?usp... : PS8, Line 16: * module LVTS Plan
Move *LVTS Plan* more to the right?
Done
File src/soc/mediatek/mt8196/thermal.c:
https://review.coreboot.org/c/coreboot/+/86017/comment/5ddb0fc9_dc428dca?usp... : PS8, Line 38: */
Please remove the duplicate comments in thermal_internal.h.
Done
https://review.coreboot.org/c/coreboot/+/86017/comment/aae3870e_3f11449d?usp... : PS8, Line 163: printk(BIOS_INFO, "%s: msr_raw=%u, temp_mc=%d\n", __func__, msr_raw, temp_mc);
Please change to `BIOS_DEBUG`
Done
https://review.coreboot.org/c/coreboot/+/86017/comment/5db01075_d6715396?usp... : PS8, Line 170: uint32_t i;
I'd prefer using `int` for array index. `size_t` is for representing the size of a C object. […]
Done
https://review.coreboot.org/c/coreboot/+/86017/comment/a026aa92_ef554e80?usp... : PS8, Line 271: printk(BIOS_INFO, "%s\n", __func__);
Can we remove this log?
Done
https://review.coreboot.org/c/coreboot/+/86017/comment/351f0f8c_34a26abf?usp... : PS8, Line 375: * to wait HW init done,
to wait *until* HW init *is* done,
Done
https://review.coreboot.org/c/coreboot/+/86017/comment/dacbd75b_4f581852?usp... : PS8, Line 376: translates
translate
Done
https://review.coreboot.org/c/coreboot/+/86017/comment/75658676_cad07b30?usp... : PS8, Line 379: finish
finishes
Done
https://review.coreboot.org/c/coreboot/+/86017/comment/cb464aec_08fa7d6a?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?
Done
-
Zhaoqing Jiu (Code Review)