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 2:
(18 comments)
File src/soc/mediatek/mt8196/gpueb.c:
https://review.coreboot.org/c/coreboot/+/85654/comment/9c9a277d_762bdad6?usp... : PS1, Line 96: uint32_t
align with first param
Done
https://review.coreboot.org/c/coreboot/+/85654/comment/96c5f81a_64479273?usp... : PS1, Line 126: 50
define this value
Done
https://review.coreboot.org/c/coreboot/+/85654/comment/04af92ab_853c3d42?usp... : PS1, Line 131: 1000
define this value
Done
https://review.coreboot.org/c/coreboot/+/85654/comment/aa8b5e53_3a614fd9?usp... : PS1, Line 187: 500
define
Done
https://review.coreboot.org/c/coreboot/+/85654/comment/9e458bdf_e0f1abe6?usp... : PS1, Line 198: mfg0_rpc_ctrl_shutdown
why do we pass `POWER_ON` to a function named with `_shutdown` ?
This name is indeed easily misleading. The function has been renamed to mfg0_power_on to improve readability.
https://review.coreboot.org/c/coreboot/+/85654/comment/727cbea4_dbe49419?usp... : PS1, Line 235: tmp_addr
`buf_addr` or just `buf`
Done
https://review.coreboot.org/c/coreboot/+/85654/comment/f83ad74c_3e530447?usp... : PS1, Line 253: need include
no need to
Done
https://review.coreboot.org/c/coreboot/+/85654/comment/b3e1cd60_cbaa4bf7?usp... : PS1, Line 276: int pt_id
`enum pt_fw_idx id`
Done
https://review.coreboot.org/c/coreboot/+/85654/comment/034df533_10179d60?usp... : PS1, Line 279: a
align with 2nd `(`
Done
https://review.coreboot.org/c/coreboot/+/85654/comment/44584877_d05f5db2?usp... : PS1, Line 339: .
remove tailing `. […]
Done
https://review.coreboot.org/c/coreboot/+/85654/comment/224a156a_0ad1af8e?usp... : PS1, Line 369: wait_us(BOOT_TIMEOUT_US, is_boot_up_complete()) == 0
if (!wait_us(BOOT_TIMEOUT_US, is_boot_up_complete()))
Done
File src/soc/mediatek/mt8196/include/soc/gpueb_priv.h:
https://review.coreboot.org/c/coreboot/+/85654/comment/d778ab31_9e8367e4?usp... : PS1, Line 21: define MFG_RPC_MFG1_PWR_CON (GPU_EB_RPC_BASE + 0x0500) /* 0x4B800500 */ : #define MFG_RPC_MFG0_PWR_CON (GPU_EB_RPC_BASE + 0x0504) /* 0x4B800504 */
just want to confirm that the ordering is correct.
Done
https://review.coreboot.org/c/coreboot/+/85654/comment/9bed7fed_d0a210b2?usp... : PS1, Line 104: 0x00000064
100
Done
https://review.coreboot.org/c/coreboot/+/85654/comment/d2fff0da_d43bf97b?usp... : PS1, Line 117: SW
align with above `SW`
Done
https://review.coreboot.org/c/coreboot/+/85654/comment/d3ed489c_bfbff454?usp... : PS1, Line 124: PMIC_ACK_SYNC
align
Done
https://review.coreboot.org/c/coreboot/+/85654/comment/0dc8850a_51578a9d?usp... : PS1, Line 129: MFG_RPC_MFG0_PWR_ACK_2ND_BIT
align
Done
https://review.coreboot.org/c/coreboot/+/85654/comment/35030d49_82dae120?usp... : PS1, Line 160: PT_FW_IDX
lower case
Done
File src/soc/mediatek/mt8196/soc.c:
https://review.coreboot.org/c/coreboot/+/85654/comment/cd3954a8_7510a2d7?usp... : PS1, Line 24: (uint64_t)
no need.
Done