Nicolas Boichat has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46389 )
Change subject: mediatek/mt8192: add spmfw loader ......................................................................
Patch Set 30:
(4 comments)
A few more comments, since I fought with that code for a bit ,-)
https://review.coreboot.org/c/coreboot/+/46389/18/src/soc/mediatek/mt8192/sp... File src/soc/mediatek/mt8192/spm.c:
https://review.coreboot.org/c/coreboot/+/46389/18/src/soc/mediatek/mt8192/sp... PS18, Line 599: Do you also want to check that firmware_size == pcm->desc.total_words?
(old comment that I forgot to submit)
https://review.coreboot.org/c/coreboot/+/46389/30/src/soc/mediatek/mt8192/sp... File src/soc/mediatek/mt8192/spm.c:
https://review.coreboot.org/c/coreboot/+/46389/30/src/soc/mediatek/mt8192/sp... PS30, Line 14: static struct pwr_ctrl spm_init_ctrl = { do you want to add a const here, so that the compiler can hopefully optimize some of the code below?
(we should probably look at the generated assembly to confirm: the functions below can shrink to very little code if the compiler is able to figure that out)
https://review.coreboot.org/c/coreboot/+/46389/30/src/soc/mediatek/mt8192/sp... PS30, Line 471: /* Kick IM to fetch (only toggle IM_KICK) */ I think I commented on an earlier version, this comment does not seem to match the code? (kick sounds a bit different than clock enable)
https://review.coreboot.org/c/coreboot/+/46389/30/src/soc/mediatek/mt8192/sp... PS30, Line 472: con0 = read32(&mtk_spm->pcm_con0); : write32(&mtk_spm->pcm_con0, con0 | SPM_REGWR_CFG_KEY | PCM_CK_EN_LSB); setbits32(&mtk_spm->pcm_con0, SPM_REGWR_CFG_KEY | PCM_CK_EN_LSB);