Erin Lo has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: Boot up SSPM ......................................................................
Patch Set 7:
(12 comments)
https://review.coreboot.org/#/c/31516/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31516/4//COMMIT_MSG@7 PS4, Line 7: boot up sspm
Boot up SSPM
Done
https://review.coreboot.org/#/c/31516/4//COMMIT_MSG@9 PS4, Line 9: enables
enable
Done
https://review.coreboot.org/#/c/31516/4//COMMIT_MSG@14 PS4, Line 14: worng
wrong
Removed it
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...
Removed it
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/Kconfig@7... PS4, Line 71: ---help---
That syntax seems new.
Removed it
https://review.coreboot.org/#/c/31516/4/src/mainboard/google/kukui/Kconfig@7... PS4, Line 72: SSPM is Secure System Power Manager for power control in secure domain.
Please add a dot at the end.
Removed it
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)
Oh... yes, actually, that's the cause of your problem. […]
Done
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().
Done
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
Probably better to just die() if something goes wrong, assuming the board cannot really boot without […]
Done
https://review.coreboot.org/#/c/31516/4/src/soc/mediatek/mt8183/sspm.c@35 PS4, Line 35: printk(BIOS_DEBUG, "no sspm\n");
No SSPM firmware(?) found.
Done
https://review.coreboot.org/#/c/31516/4/src/soc/mediatek/mt8183/sspm.c@35 PS4, Line 35: BIOS_DEBUG
I'd say at least ERR, but probably a die()
Done
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?
Done