Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34545 )
Change subject: SPM: mt8183: support spm suspend in coreboot ......................................................................
Patch Set 2:
(5 comments)
Please add a more elaborate commit message for such a huge diff stat. What is SPM and why is it needed? Why does it have to be set up by the firmware, and it’s not enough that the OS sets it up? From what datasheet did you take the macro definitions?
Also, please add a follow-up commit with additional time-stamps (or stopwatch messages) so the time of this can be measured.
What is the boot time penalty of this commit?
https://review.coreboot.org/c/coreboot/+/34545/2/src/soc/mediatek/mt8183/spm... File src/soc/mediatek/mt8183/spm.c:
https://review.coreboot.org/c/coreboot/+/34545/2/src/soc/mediatek/mt8183/spm... PS2, Line 225: CBFS_TYPE_RAW); Please use the full text width.
https://review.coreboot.org/c/coreboot/+/34545/2/src/soc/mediatek/mt8183/spm... PS2, Line 243: BIOS_INFO Probably more debug level?
https://review.coreboot.org/c/coreboot/+/34545/2/src/soc/mediatek/mt8183/spm... PS2, Line 244: (unsigned int)(unsigned long) Why the double cast?
https://review.coreboot.org/c/coreboot/+/34545/2/src/soc/mediatek/mt8183/spm... PS2, Line 305: spm_load_firmware(index); Did you add time-stamps for this? I thinks these thinks should be measured for how long the take.
https://review.coreboot.org/c/coreboot/+/34545/2/src/soc/mediatek/mt8183/spm... PS2, Line 310: printk(BIOS_ERR, "SPM: firmware is not ready!!!\n"); As this is a user visible message, please add more information, what the user should do.