DAWEI CHIEN has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34545 )
Change subject: mediatek/mt8183: Init SPM driver ......................................................................
Patch Set 17:
(7 comments)
https://review.coreboot.org/c/coreboot/+/34545/17/src/mainboard/google/kukui... File src/mainboard/google/kukui/mainboard.c:
https://review.coreboot.org/c/coreboot/+/34545/17/src/mainboard/google/kukui... PS17, Line 161: SPM initial fail!!!
SPM initialization failed, suspend/resume may fail.
ok, would change it on next version.
https://review.coreboot.org/c/coreboot/+/34545/17/src/soc/mediatek/mt8183/Ma... File src/soc/mediatek/mt8183/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/34545/17/src/soc/mediatek/mt8183/Ma... PS17, Line 72: 3rdparty/blobs/soc/mediatek/mt8183/pcm_allinone_lp4_3200.bin
let's try to refactor this with the sspm one. […]
ok, would change it on next version.
https://review.coreboot.org/c/coreboot/+/34545/17/src/soc/mediatek/mt8183/sp... File src/soc/mediatek/mt8183/spm.c:
https://review.coreboot.org/c/coreboot/+/34545/17/src/soc/mediatek/mt8183/sp... PS17, Line 80: write32(&mtk_spm->spare_ack_mask, : (read32(&mtk_spm->spare_ack_mask) & ~SPARE_ACK_MASK_B_BIT1) | : SPARE_ACK_MASK_B_BIT0);
Is this equivalent to: […]
Yes, would change it on next version.
https://review.coreboot.org/c/coreboot/+/34545/17/src/soc/mediatek/mt8183/sp... PS17, Line 88: write32
setbits32 ?
Yes, would change it on next version.
https://review.coreboot.org/c/coreboot/+/34545/17/src/soc/mediatek/mt8183/sp... PS17, Line 130: read32(&mtk_spm->pcm_reg0_data)
wrong indent - this should be aligned to (
ok, would change it on next version.
https://review.coreboot.org/c/coreboot/+/34545/17/src/soc/mediatek/mt8183/sp... PS17, Line 135: write32
clrsetbits32?
ok, would change it on next version.
https://review.coreboot.org/c/coreboot/+/34545/17/src/soc/mediatek/mt8183/sp... PS17, Line 309: while
should we do a wait_us or wait_ms here?
In my opinion, even usually spm don't take more time to switch status, we still need give SPM time until spm change new status to avoid SPM in wrong status and make sure spm could run pcm code. If there is any issue, I would rather stay to debug in this while loop than print a error message.