Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: boot up sspm ......................................................................
Patch Set 4:
(6 comments)
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/Kconfig File src/mainboard/google/kukui/Kconfig:
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/Kconfig@6... PS4, Line 68: config SSPM_BIN_FILE Still think this shouldn't be a Kconfig...
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/Makefile.... File src/mainboard/google/kukui/Makefile.inc:
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/Makefile.... PS4, Line 32: sspm-compression := $(CBFS_COMPRESS_FLAG) Odd that you said you weren't compressing it anymore now? Because this definitely looks like you do (and you should, compression is almost certainly a much bigger speed gain than what a memcpy or two would cost).
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/mainboard... File src/mainboard/google/kukui/mainboard.c:
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/mainboard... PS4, Line 41: static void configure_sspm(void) nit: You don't really need single line functions, can just put sspm_init() into mainboard_init().
https://review.coreboot.org/#/c/31516/4/src/soc/mediatek/mt8183/sspm.c File src/soc/mediatek/mt8183/sspm.c:
https://review.coreboot.org/#/c/31516/4/src/soc/mediatek/mt8183/sspm.c@23 PS4, Line 23: void
Make that bool?
Probably better to just die() if something goes wrong, assuming the board cannot really boot without this anyway?
https://review.coreboot.org/#/c/31516/4/src/soc/mediatek/mt8183/sspm.c@35 PS4, Line 35: BIOS_DEBUG
Make that INFO?
I'd say at least ERR, but probably a die()
https://review.coreboot.org/#/c/31516/4/src/soc/mediatek/mt8183/sspm.c@38 PS4, Line 38: "sspm[0]=%#x, [%zd]=%#x\n", nit: I'm not sure this is useful enough to keep in here after you're done debugging?