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

[Zhaoqing Jiu (Code Review)]

Attention is currently required from: Hung-Te Lin, Jarried Lin, 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 3:

(18 comments)

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

PS2:
> Rename to thermal.c.

Done

https://review.coreboot.org/c/coreboot/+/86017/comment/90518e79_e1551146?usp... :
PS2, Line 13: TIMES
> RETRY_CNT

Done

https://review.coreboot.org/c/coreboot/+/86017/comment/0d2504c4_a7a5222c?usp... :
PS2, Line 18: TIMES
> RETRY_CNT […]

Done

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

Done

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

Done

https://review.coreboot.org/c/coreboot/+/86017/comment/713ca1ad_ce10c645?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 acceptab […]

Done

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

Done

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

Done

https://review.coreboot.org/c/coreboot/+/86017/comment/9256e68f_9ea946cc?usp... :
PS2, Line 133: INFO
> ERR

Done

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

Done

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

PS2:
> Rename to thermal_sram.c.

Done

https://review.coreboot.org/c/coreboot/+/86017/comment/1fe88cc5_6891afd6?usp... :
PS2, Line 8: _sram + 0x00014000
> Better to use `_mcufw_reserved + 0x1000`. […]

Done

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

Done

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

Done

https://review.coreboot.org/c/coreboot/+/86017/comment/22f444ba_6381911e?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, […]

Done

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

Done

https://review.coreboot.org/c/coreboot/+/86017/comment/38abaa9e_f416b1f2?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'm fine either way. I was just providing an alternative to the current code.

Done

https://review.coreboot.org/c/coreboot/+/86017/comment/f9cea59f_1fa3b037?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++;
            : 	}
> > Would it be better to align the same style with thermal_cls_sram() function? […]

Just keep it same as thermal_stat_cls_sram()