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

[Alvin Liu (Code Review)]

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

Alvin Liu 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 4:

(34 comments)

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

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

Done

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

Done

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

Done

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

Done

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

Done

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

Done

https://review.coreboot.org/c/coreboot/+/85654/comment/d023565e_92e75bd7?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. […]

Done

https://review.coreboot.org/c/coreboot/+/85654/comment/0afd8f5b_2c2f3bc1?usp... :
PS2, Line 202: (
> remove

Done

https://review.coreboot.org/c/coreboot/+/85654/comment/fbeb01b0_cae2130e?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, w […]

Done

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

Done

https://review.coreboot.org/c/coreboot/+/85654/comment/1b9e0fc3_145875ff?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 […]

Done

https://review.coreboot.org/c/coreboot/+/85654/comment/f6c38876_7be8e823?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. […]

Done

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

Done

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

Done

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

Done

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

Done

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

Done

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

Done

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

Done

https://review.coreboot.org/c/coreboot/+/85654/comment/65242889_22a81536?usp... :
PS2, Line 357: (
> remove

Done

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

https://review.coreboot.org/c/coreboot/+/85654/comment/d82ef5cf_ab0b9b84?usp... :
PS3, Line 20: BIOS_CRIT
> BIOS_DEBUG

Done

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

https://review.coreboot.org/c/coreboot/+/85654/comment/b44158b9_6787e99b?usp... :
PS3, Line 197: GPUEB_BASE
> MFG_GPUEB_BASE

Done

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

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

Done

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

Done

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

https://review.coreboot.org/c/coreboot/+/85654/comment/efaf4495_fa9835d5?usp... :
PS2, Line 13: define GPUEB_BASE			(MFGSYS_BASE + 0x0B000000)	/* 0x4B000000 */
            : #define MFG_RPC_BASE			(MFGSYS_BASE + 0x0B800000)
> `MFG_RPC_BASE` already exists there. Please move `GPUEB_BASE` there and rename to `MFG_GPUEB_BASE`.

Done

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

Done

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

Done

https://review.coreboot.org/c/coreboot/+/85654/comment/f7ae9454_2b1fbd00?usp... :
PS2, Line 75:  GPUEB_GPR_SIZE + \
> Why do we need to do the subtraction? Can't we define this using `0x0009FD00` directly?

Done

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

Done

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

Done

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

Done

https://review.coreboot.org/c/coreboot/+/85654/comment/ecf91a49_d9606777?usp... :
PS2, Line 157: #define DUMP_REG(reg)	printk(BIOS_CRIT, #reg "(%#x) = %#x\n", (reg), read32p((reg)))
> Use `BIOS_DEBUG` instead of `BIOS_CRIT`.

Done

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

Done

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

Done