Roger Lu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46389 )
Change subject: mediatek/mt8192: add spmfw loader ......................................................................
Patch Set 28:
(32 comments)
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/common/mt... File src/soc/mediatek/common/mtcmos.c:
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/common/mt... PS26, Line 26: poweron_config_en
MT8183 uses poweron_config_set, while MT8192 has poweron_config_en, so we can't simply change it lik […]
Done
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/Ma... File src/soc/mediatek/mt8192/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/Ma... PS26, Line 42: ramstage-y += spm.c
Put this after soc. […]
Done
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/in... File src/soc/mediatek/mt8192/include/soc/spm.h:
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/in... PS26, Line 11: (
No need for parentheses.
Done
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/in... PS26, Line 59: (
No need for parentheses for a single number. Same below. […]
Done
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/in... PS26, Line 90: (
Same.
Done
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/in... PS26, Line 109: (
Same.
Done
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/in... PS26, Line 639: 0
Be consistent and remove the leading zeros?
Done
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 314: SPM_REGWR_CFG_KEY
Move this to the next line for readability.
Done
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 322: SPM_REGWR_CFG_KEY
Same.
Done
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 325: SPM_REGWR_CFG_KEY
Same.
Done
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 330: write32(&mtk_spm->spm_clk_con, read32(&mtk_spm->spm_clk_con) | : REG_SYSCLK1_SRC_MD2_SRCCLKENA);
Can we use clrsetbits32 or SET32_BITFIELDS?
Done
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 361: write32(&mtk_spm->spm_dvfs_misc, (read32(&mtk_spm->spm_dvfs_misc) & : ~(SPM_DVFS_FORCE_ENABLE_LSB)) | (SPM_DVFSRC_ENABLE_LSB));
Can we use clrsetbits32 or SET32_BITFIELDS?
Done
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 370: reg = read32(&mtk_spm->spm_ack_chk_con_3); : reg |= (SPM_ACK_CHK_3_CON_HW_MODE_TRIG | SPM_ACK_CHK_3_CON_CLR_ALL); : reg &= ~(SPM_ACK_CHK_3_CON_EN); : write32(&mtk_spm->spm_ack_chk_con_3, reg);
Can we use clrsetbits32 or SET32_BITFIELDS?
Done
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 387: (
No need for parentheses.
Done
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 414: write32(&mtk_spm->pcm_con1, SPM_REGWR_CFG_KEY | : (read32(&mtk_spm->pcm_con1) & ~RG_PCM_TIMER_EN_LSB));
Use SET32_BITFIELDS?
Done
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 418: SPM_REGWR_CFG_KEY
Move to next line.
Done
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 420: SPM_REGWR_CFG_KEY
Same.
No exceed 80 lines. Therefore, Keep the same line.
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 423: (
No need for parentheses.
miss this one, will fix in the next patch.
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 424: con1
Same.
Done
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 450: (
No need for parentheses. Same below.
Done
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 455: t
T
Done
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 460:
Remove one space.
miss this one, will fix in the next patch.
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 461:
Remove one space.
miss this one, will fix in the next patch.
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 470: write32(&mtk_spm->pcm_con1, read32(&mtk_spm->pcm_con1) | : SPM_REGWR_CFG_KEY | RG_IM_SLAVE_LSB);
Use SET32_BITFIELDS?
Done
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 474: k
K
Done
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 551: write32(&mtk_spm->pcm_con1, read32(&mtk_spm->pcm_con1) | : SPM_REGWR_CFG_KEY | SPM_EVENT_COUNTER_CLR_LSB);
Use SET32_BITFIELDS?
Done
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 555: write32(&mtk_spm->sys_timer_con, read32(&mtk_spm->sys_timer_con) | : SYS_TIMER_START_EN_LSB);
Use SET32_BITFIELDS?
Done
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 581: isr = read32(&mtk_spm->spm_irq_mask); : write32(&mtk_spm->spm_irq_mask, isr | ISRM_RET_IRQ_AUX);
Use SET32_BITFIELDS?
Done
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 585: write32(&mtk_spm->pcm_con1, SPM_REGWR_CFG_KEY | : (read32(&mtk_spm->pcm_con1) & ~SPM_EVENT_COUNTER_CLR_LSB));
Use SET32_BITFIELDS?
Done
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 589: write32(&mtk_spm->sys_timer_con, read32(&mtk_spm->sys_timer_con) & : ~SYS_TIMER_START_EN_LSB);
Use SET32_BITFIELDS?
Done
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 628: con0 = read32(&mtk_spm->pcm_con0); : write32(&mtk_spm->pcm_con0, con0 | SPM_REGWR_CFG_KEY | PCM_CK_EN_LSB);
Use SET32_BITFIELDS?
Done
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 632: rstn = read32(&mtk_spm->md32pcm_cfgreg_sw_rstn); : write32(&mtk_spm->md32pcm_cfgreg_sw_rstn, : rstn | MD32PCM_CFGREG_SW_RSTN_RESET);
Use SET32_BITFIELDS?
Done