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

[Yu-Ping Wu (Code Review)]

Attention is currently required from: Hung-Te Lin, Jarried Lin, Yidi Lin.

Yu-Ping Wu 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 2:

(18 comments)

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

PS2: 
Rename to thermal.c.

https://review.coreboot.org/c/coreboot/+/86017/comment/79f42a38_02359730?usp... :
PS2, Line 13: TIMES
RETRY_CNT

https://review.coreboot.org/c/coreboot/+/86017/comment/0cab9070_dba60d0f?usp... :
PS2, Line 18: TIMES
RETRY_CNT

Align all values with `0x13121110` in line #21.

https://review.coreboot.org/c/coreboot/+/86017/comment/5bf5ebb3_5c35e261?usp... :
PS2, Line 105: /*=============================================================
             :  * Local variable definition
             :  *=============================================================
             :  */
Remove.

https://review.coreboot.org/c/coreboot/+/86017/comment/8a121d6b_f36addcb?usp... :
PS2, Line 109: g_
Drop `g_` for global variables.

https://review.coreboot.org/c/coreboot/+/86017/comment/8625d214_1bd9ac8c?usp... :
PS2, Line 110: g_count_r[L_TS_LVTS_NUM];
             : static uint8_t g_op_cali[LVTS_CONTROLLER_NUM];
For these 2 global variables `count_r` (what does "r" mean?) and `op_cali` (I guess this is acceptable, if you cannot come up with a better name), can we use more descriptive names?

https://review.coreboot.org/c/coreboot/+/86017/comment/aac37f39_c5fba01b?usp... :
PS2, Line 132: read32(&tc->regs->lvts_config_0) & DEVICE_ACCESS_START_BIT
`!(read32(&tc->regs->lvts_config_0) & DEVICE_ACCESS_START_BIT)`

`retry(attempts, condition)` waits for `condition` to become true. Please double check.

https://review.coreboot.org/c/coreboot/+/86017/comment/3f00fa4e_9601ccd0?usp... :
PS2, Line 133: INFO
ERR

https://review.coreboot.org/c/coreboot/+/86017/comment/2a49cc18_a1d10dd3?usp... :
PS2, Line 133: write err: 
remove

https://review.coreboot.org/c/coreboot/+/86017/comment/aa7011b1_c2001dd8?usp... :
PS2, Line 167: >=
`>`

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

PS2: 
Rename to thermal_sram.c.

https://review.coreboot.org/c/coreboot/+/86017/comment/af808a83_6c7540be?usp... :
PS2, Line 8: _sram + 0x00014000
Better to use `_mcufw_reserved + 0x1000`. You'll need to add `DECLARE_REGION(_mcufw_reserved)` in src/soc/mediatek/common/include/soc/symbols.h.

https://review.coreboot.org/c/coreboot/+/86017/comment/2b999980_62d409ea?usp... :
PS2, Line 16: 	
const

https://review.coreboot.org/c/coreboot/+/86017/comment/beb4f2f2_63f3997c?usp... :
PS2, Line 16: 0x27bc86aa
upper case

https://review.coreboot.org/c/coreboot/+/86017/comment/1b6f3be6_55f79f98?usp... :
PS2, Line 17: THERMAL_SRAM_BASE
Is this supposed to be `THERMAL_ATC_SRAM_BASE`? We're writing the pattern to the whole thermal SRAM, but only clear and invalidate dcache for ATC SRAM?

https://review.coreboot.org/c/coreboot/+/86017/comment/19104ba5_0451ccb5?usp... :
PS2, Line 18: 4
`sizeof(*buff)`

https://review.coreboot.org/c/coreboot/+/86017/comment/78eb1fa5_30098e0b?usp... :
PS2, Line 32: int i = 0;
            : 	uint32_t pattern = 0xffffffff;
            : 	uint32_t *buff = (uint32_t *)THERMAL_STAT_SRAM_BASE;
            : 	for (i = 0; i < THERMAL_STAT_SRAM_LEN / 4; i++) {
            : 		*buff = pattern;
            : 		buff++;
            : 	}
I assume `memset((void *)THERMAL_STAT_SRAM_BASE, 0xFF, THERMAL_STAT_SRAM_LEN)` would be faster.

https://review.coreboot.org/c/coreboot/+/86017/comment/79bc567c_0d9f4899?usp... :
PS2, Line 48: int i = 0;
            : 	uint32_t pattern = 0xffffffff;
            : 	uint32_t *buff = (uint32_t *)GPU_THERMAL_STAT_SRAM_BASE;
            : 	for (i = 0; i < GPU_THERMAL_STAT_SRAM_LEN / 4; i++) {
            : 		*buff = pattern;
            : 		buff++;
            : 	}
Use memset?