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

[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/+/85654?usp=email )

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

Patch Set 2:

(34 comments)

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

https://review.coreboot.org/c/coreboot/+/85654/comment/12abaa43_79f9e193?usp... :
PS2, Line 14: #include <halt.h>
Replace `BIOS_CRIT logs + halt()` with `die(...)` with an error message.

https://review.coreboot.org/c/coreboot/+/85654/comment/8ec3c471_3d293107?usp... :
PS2, Line 193: gpueb_parsing_fw
`parse_fw`

https://review.coreboot.org/c/coreboot/+/85654/comment/f9df3080_57e6688a?usp... :
PS2, Line 193: imgsize
img_size

https://review.coreboot.org/c/coreboot/+/85654/comment/795739a4_5f7886f9?usp... :
PS2, Line 193: void *buf_addr
`const void *buf`

https://review.coreboot.org/c/coreboot/+/85654/comment/34c5cead_13e20235?usp... :
PS2, Line 197: ptimg_hdr_t
Rename to `gpueb_pt_hdr`. No `_t`.

https://review.coreboot.org/c/coreboot/+/85654/comment/d0f59b7e_fe7f170c?usp... :
PS2, Line 201: 	while (hdr->magic == PT_MAGIC) {
We should also pass `size_t buf_size` to this function, and make sure we never parse beyond that (i.e. `(uintptr_t)hdr + sizeof(*hdr) <= (uintptr_t)buf + buf_size`).

https://review.coreboot.org/c/coreboot/+/85654/comment/646b935a_3b5e5ca5?usp... :
PS2, Line 201: PT
Can we rename this to `GPUEB_PT_MAGIC`?

https://review.coreboot.org/c/coreboot/+/85654/comment/43c16b2f_875f2ebf?usp... :
PS2, Line 202: (
remove

https://review.coreboot.org/c/coreboot/+/85654/comment/943cb821_74314923?usp... :
PS2, Line 204: 		if (hdr->id == 0 || hdr->id >= PT_FW_NUM) {
Is this case expected? If so, can't we remove these unused partitions from the gpueb blob? If not, we should print a warning log here.

https://review.coreboot.org/c/coreboot/+/85654/comment/30216dce_9024ab39?usp... :
PS2, Line 209: 
Check `(uintptr_t)img + hdr->img_size <= (uintptr_t)buf + buf_size` here.

https://review.coreboot.org/c/coreboot/+/85654/comment/918936aa_a7ba2a11?usp... :
PS2, Line 213: auto_dma_fw[PT_FW_IDX_GPUEB_FW].fw = img;
             : 			auto_dma_fw[PT_FW_IDX_GPUEB_FW].fw_size = hdr->img_size;
Since PT_FW_IDX_GPUEB_FW is NOT auto dma fw (if I understand this correctly), we don't need to store these into the `auto_dma_fw` array. In the check below (line #225), skip `PT_FW_IDX_GPUEB_FW`. Then, check `gpueb_img` and `gpueb_img_size` at the end of this function.

https://review.coreboot.org/c/coreboot/+/85654/comment/c87ae1c9_46e0ebea?usp... :
PS2, Line 235: 	memcpy((void *)(GPUEB_SRAM_BASE + auto_dma_fw[pt_id].fw_addr),
We should check `fw_addr` against a max value for safety. Is `assert(fw_addr + fw_size <= GPUEB_SRAM_SIZE)` correct?

https://review.coreboot.org/c/coreboot/+/85654/comment/b8d0df23_94dff19a?usp... :
PS2, Line 264: (
remove

https://review.coreboot.org/c/coreboot/+/85654/comment/7586c4ac_e8593163?usp... :
PS2, Line 265: CRIT
ERR

https://review.coreboot.org/c/coreboot/+/85654/comment/cf0a5a60_0250a26b?usp... :
PS2, Line 271: CRIT
ERR

https://review.coreboot.org/c/coreboot/+/85654/comment/37b65979_13ef66ad?usp... :
PS2, Line 289: imgsize
img_size

https://review.coreboot.org/c/coreboot/+/85654/comment/1a9ebb5f_62df937f?usp... :
PS2, Line 294: Invalid parameters returned
Failed to parse gpueb fw

https://review.coreboot.org/c/coreboot/+/85654/comment/d76fee09_08d06ade?usp... :
PS2, Line 300: 	memset((void *)GPUEB_SRAM_BASE, 0, GPUEB_SRAM_SIZE);
Should check `gpueb_imgsize <= GPUEB_SRAM_SIZE`.

https://review.coreboot.org/c/coreboot/+/85654/comment/26e6fb63_48281c7e?usp... :
PS2, Line 352: 	.reset = gpueb_reset
nit: Add a trailing `,`

https://review.coreboot.org/c/coreboot/+/85654/comment/023dc7d7_1cf815c7?usp... :
PS2, Line 357: (
remove

https://review.coreboot.org/c/coreboot/+/85654/comment/508986eb_1c21e0aa?usp... :
PS2, Line 358: die
Is the gpueb init flow critical for the device to boot up? If not, I wonder if we can print an error message and then return, instead of calling `die()` everywhere.

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

https://review.coreboot.org/c/coreboot/+/85654/comment/7c12cb1b_ba003d55?usp... :
PS2, Line 188: IO_PHYS + 0x38500000
Rewrite as `MFGSYS_BASE + 0x08500000`. Please rebase onto CB:85740.

https://review.coreboot.org/c/coreboot/+/85654/comment/945103a3_a43fb32d?usp... :
PS2, Line 189: GPU_EB_RPC_BASE
Rename to `MFG_GPUEB_RPC_BASE`.

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

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

`MFG_RPC_BASE` already exists there. Please move `GPUEB_BASE` there and rename to `MFG_GPUEB_BASE`.

https://review.coreboot.org/c/coreboot/+/85654/comment/04b11b5e_38f396de?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 ?

The bases are the same. `MFG_RPC_BASE` should be removed here.

https://review.coreboot.org/c/coreboot/+/85654/comment/a495d7ed_b1177936?usp... :
PS2, Line 71: 0xA0000
Write `(640 * KiB)`, and the comment can be removed.

https://review.coreboot.org/c/coreboot/+/85654/comment/e53cace2_5d4b0b70?usp... :
PS2, Line 73: _4B
Remove `_4B`

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

Why do we need to do the subtraction? Can't we define this using `0x0009FD00` directly?

https://review.coreboot.org/c/coreboot/+/85654/comment/79de1cb2_85e0b8db?usp... :
PS2, Line 77: #define GPUEB_GPR0				GPR_BASE_ADDR(0)
            : #define GPUEB_GPR1				GPR_BASE_ADDR(1)
            : #define GPUEB_GPR2				GPR_BASE_ADDR(2)
            : #define GPUEB_GPR6				GPR_BASE_ADDR(6)
            : #define GPUEB_GPR17				GPR_BASE_ADDR(17)
            : #define GPUEB_GPR18				GPR_BASE_ADDR(18)
            : #define GPUEB_GPR23				GPR_BASE_ADDR(23)
            : #define GPUEB_GPR30				GPR_BASE_ADDR(30)
Remove these. Use `GPR_BASE_ADDR(x)` directly.

https://review.coreboot.org/c/coreboot/+/85654/comment/86a32472_a28de526?usp... :
PS2, Line 138: GPUEB_SRAM_GPR25
Remove this.

https://review.coreboot.org/c/coreboot/+/85654/comment/4c09b8a8_71ee2786?usp... :
PS2, Line 152: SWITCH_MUX_WAIT_TIME
`SWITCH_MUX_WAIT_US`

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

Use `BIOS_DEBUG` instead of `BIOS_CRIT`.

https://review.coreboot.org/c/coreboot/+/85654/comment/e246b1ba_3a5c16ec?usp... :
PS2, Line 186: void
const

https://review.coreboot.org/c/coreboot/+/85654/comment/18e84fcd_18822b02?usp... :
PS2, Line 185: struct auto_dma_fw_header {
             : 	void *fw;
             : 	size_t fw_size;
             : 	uint64_t fw_addr;
             : };
Move to gpueb.c and rename to `struct auto_dma_fw`, as this isn't a header.