[L] Change in coreboot[main]: soc/mediatek/mt8196: Add GPUEB support

[Andy Hsu (Code Review)]

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

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

Change subject: soc/mediatek/mt8196: Add GPUEB support
......................................................................

Patch Set 3:

(10 comments)

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

https://review.coreboot.org/c/coreboot/+/85654/comment/4554f51b_9ad7d19f?usp... :
PS2, Line 123: 
             : 	if (read32p(MFG_RPC_MFG0_PWR_CON) == MFG0_PWR_ON_VALUE)
             : 		return;
             : 
             : 	if (!(read32p(MFG_RPC_MFG0_PWR_CON) & MFG0_PWR_SRAM_PDN))
             : 		return;
> Please add comments why the setting can be omitted in these two conditions.

Done

https://review.coreboot.org/c/coreboot/+/85654/comment/d1239284_6a928120?usp... :
PS2, Line 225: 1
> PT_FW_IDX_GPUEB_FW

Done

https://review.coreboot.org/c/coreboot/+/85654/comment/8ffa006c_2cf09b13?usp... :
PS2, Line 236: aut
> align with `(void *)`

Done

https://review.coreboot.org/c/coreboot/+/85654/comment/9a38684b_286db84e?usp... :
PS2, Line 305: 	write32p(GPUEB_INFO, (uintptr_t)mcu->load_buffer);
             : 	write32p(GPUEB_ABNORMAL_BOOT, 0);
             : 	write32p(GPUEB_WARM_BOOT, 0);
             : 	write32p(GPUMPU_RSV_ADDR, (uintptr_t)_resv_mem_gpueb >> PAGE_SHIFT);
> Can you explain this part ? I don't quite understand why it writes `mcu->load_buffer` to `GPUEB_INFO […]

Upon confirmation, this section is identified as legacy code and is not needed in the current process. It can be removed.

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

https://review.coreboot.org/c/coreboot/+/85654/comment/72258711_b90ae8e5?usp... :
PS2, Line 10: #define SPM2GPUPM_CON			(SPM_BASE + 0x0410)		/* 0x1C004410 */
            : #define SPM_REQ_STA_9			(SPM_BASE + 0x0884)		/* 0x1C004884 */
            : #define SPM_MFG0_PWR_CON		(SPM_BASE + 0x0EA8)
> remove

Done

https://review.coreboot.org/c/coreboot/+/85654/comment/a573ca4e_68d6ce3b?usp... :
PS2, Line 13: define GPUEB_BASE			(MFGSYS_BASE + 0x0B000000)	/* 0x4B000000 */
            : #define MFG_RPC_BASE			(MFGSYS_BASE + 0x0B800000)
> move to soc/addressmap. […]

Done

https://review.coreboot.org/c/coreboot/+/85654/comment/3d2a1f33_d17cc3f4?usp... :
PS2, Line 57: #define MFG_RPC_GHPM_CFG13_CON		(GPU_EB_RPC_BASE + 0x0834)	/* 0x4B800834 */
            : #define MFG_GHPM_RO2_CON		(MFG_RPC_BASE + 0x09AC)		/* 0x4B8009AC */
> why are they using different bases ?

Done

https://review.coreboot.org/c/coreboot/+/85654/comment/051c32cc_753372a4?usp... :
PS2, Line 75:  GPUEB_GPR_SIZE + \
> move to the above line.

Done

https://review.coreboot.org/c/coreboot/+/85654/comment/f17af5ef_c7785546?usp... :
PS2, Line 157: #define DUMP_REG(reg)	printk(BIOS_CRIT, #reg "(%#x) = %#x\n", (reg), read32p((reg)))
> move to gpueb. […]

Done

https://review.coreboot.org/c/coreboot/+/85654/comment/9cf5e934_3e3b7fb6?usp... :
PS2, Line 197: memory addr; current setting is within the 4GB memory
             : 				 * region
             : 				 */
> The FW address is in the 4GB range.

Done