[coreboot-gerrit] [M] Change in coreboot[main]: soc/mediatek/mt8196: Initialize MCUPM

[Paul Menzel (Code Review)]

Attention is currently required from: Hung-Te Lin, Jarried Lin, agogo.

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

The change is no longer submittable: All-Comments-Resolved is unsatisfied now.

Change subject: soc/mediatek/mt8196: Initialize MCUPM
......................................................................

Patch Set 6:

(9 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/85888/comment/96ba839d_e2acb043?usp... :
PS6, Line 17: TEST=Build pass and we can see the mcupm logs after reset releases.
Please paste the new logs.

File src/soc/mediatek/mt8196/Kconfig:

https://review.coreboot.org/c/coreboot/+/85888/comment/18ff0ec3_bacf3f18?usp... :
PS5, Line 64: 	  The file name of the MediaTek MCUPM firmware.
> Paul, please see the MCUPM README in https://review.coreboot. […]

Thank you for the pointer. In my opinion it still should go into the commit message.

Additionally the commit also has the init sequence and it says something about secure master and so on. In my opinion this should be also described.

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

https://review.coreboot.org/c/coreboot/+/85888/comment/4530da61_914762fb?usp... :
PS6, Line 56: MCUPM_SRAM_SIZE
I’d append _KB.

https://review.coreboot.org/c/coreboot/+/85888/comment/efaed8ec_d940ae4e?usp... :
PS6, Line 69: POLLING_MCU_TIME
Please append the unit to the name.

https://review.coreboot.org/c/coreboot/+/85888/comment/2de02e7b_50860a2a?usp... :
PS6, Line 79: 0x4
Hex notation not necessary?

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

https://review.coreboot.org/c/coreboot/+/85888/comment/1e9ff732_152b767a?usp... :
PS6, Line 17: POLLING_MCU_TIME
retry takes number of attempts as first argument. Maybe use `wait_…()` from the stopwatch() framework `src/include/timer.h`.

https://review.coreboot.org/c/coreboot/+/85888/comment/9c2d8dc0_84427239?usp... :
PS6, Line 21: 		       "[EB_SPMC] Polling MCU_PORT_SET_R0_0 timeout, %#x\n",
Please also print the timeout value.

https://review.coreboot.org/c/coreboot/+/85888/comment/69919cbc_efa5fb7b?usp... :
PS6, Line 72: 	int ret = 0;
Does not need to be initialized.

https://review.coreboot.org/c/coreboot/+/85888/comment/2b73118c_4b989120?usp... :
PS6, Line 80: 		ret = eb_spmc_spm();
Return right away?