Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46389 )
Change subject: mediatek/mt8192: add spmfw loader ......................................................................
Patch Set 26:
(33 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 like this. I'd suggest having consistent naming.
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.c?
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.
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.
For "(30 * 32768)" however, we need to keep the parentheses.
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/in... PS26, Line 90: ( Same.
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/in... PS26, Line 109: ( Same.
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/in... PS26, Line 639: 0 Be consistent and remove the leading zeros?
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.
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 322: SPM_REGWR_CFG_KEY Same.
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 325: SPM_REGWR_CFG_KEY Same.
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?
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?
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?
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 387: ( No need for parentheses.
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?
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 418: SPM_REGWR_CFG_KEY Move to next line.
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 420: SPM_REGWR_CFG_KEY Same.
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 423: ( No need for parentheses.
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 424: con1 Same.
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 450: ( No need for parentheses. Same below.
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 455: t T
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 460: Remove one space.
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 461: Remove one space.
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?
https://review.coreboot.org/c/coreboot/+/46389/26/src/soc/mediatek/mt8192/sp... PS26, Line 474: k K
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?
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?
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)); Use SET32_BITFIELDS?
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?
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?
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?
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?
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?