Roger Lu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46389 )
Change subject: mediatek/mt8192: add spmfw loader ......................................................................
Patch Set 29:
(3 comments)
https://review.coreboot.org/c/coreboot/+/46389/17/src/soc/mediatek/mt8192/sp... File src/soc/mediatek/mt8192/spm.c:
https://review.coreboot.org/c/coreboot/+/46389/17/src/soc/mediatek/mt8192/sp... PS17, Line 945: write32(&mtk_spm->md32pcm_dma0_start, MD32PCM_DMA0_START_VAL);
Update?
write32(&mtk_spm->md32pcm_dma0_start, 0); => It is not necessary and can be removed.
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... File src/soc/mediatek/mt8192/spm.c:
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 566: write32(&mtk_spm->pcm_con1, SPM_REGWR_CFG_KEY | : (read32(&mtk_spm->pcm_con1) & ~RG_PCM_TIMER_EN_LSB));
Forgot this?
It is fixed in https://review.coreboot.org/c/coreboot/+/46389/29/src/soc/mediatek/mt8192/sp....
https://review.coreboot.org/c/coreboot/+/46389/29/src/soc/mediatek/mt8192/sp... File src/soc/mediatek/mt8192/spm.c:
https://review.coreboot.org/c/coreboot/+/46389/29/src/soc/mediatek/mt8192/sp... PS29, Line 421: con1 = read32(&mtk_spm->pcm_con1) & RG_PCM_WDT_WAKE_LSB; : write32(&mtk_spm->pcm_con1, : con1 | SPM_REGWR_CFG_KEY | REG_EVENT_LOCK_EN_LSB | : REG_SPM_SRAM_ISOINT_B_LSB | RG_AHBMIF_APBEN_LSB | : REG_MD32_APB_INTERNAL_EN_LSB);
Use clrsetbits32?
The original code keeps PCM WDT setting from con1 only and assign new bits to con1 register. So, it cannot be replaced with clrsetbits32.