Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34545 )
Change subject: mediatek/mt8183: Init SPM driver ......................................................................
Patch Set 25:
(24 comments)
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/in... File src/soc/mediatek/mt8183/include/soc/spm.h:
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/in... PS14, Line 50: u32 pcm_event_vector0; : u32 pcm_event_vector1; : u32 pcm_event_vector2; : u32 pcm_event_vector3; : u32 pcm_event_vector4; : u32 pcm_event_vector5; : u32 pcm_event_vector6; : u32 pcm_event_vector7; : u32 pcm_event_vector8; : u32 pcm_event_vector9; : u32 pcm_event_vector10; : u32 pcm_event_vector11; : u32 pcm_event_vector12; : u32 pcm_event_vector13; : u32 pcm_event_vector14; : u32 pcm_event_vector15;
it is ok to me
Done
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/in... PS14, Line 476: check_member(mtk_spm_regs, poweron_config_set, 0x0000); : check_member(mtk_spm_regs, spm_power_on_val0, 0x0004); : check_member(mtk_spm_regs, spm_power_on_val1, 0x0008); : check_member(mtk_spm_regs, spm_clk_con, 0x000c); : check_member(mtk_spm_regs, pcm_con0, 0x0018); : check_member(mtk_spm_regs, pcm_con1, 0x001c); : check_member(mtk_spm_regs, pcm_im_ptr, 0x0020); : check_member(mtk_spm_regs, pcm_im_len, 0x0024); : check_member(mtk_spm_regs, pcm_reg_data_ini, 0x0028); : check_member(mtk_spm_regs, pcm_pwr_io_en, 0x002c); : check_member(mtk_spm_regs, pcm_im_host_rw_ptr, 0x0038); : check_member(mtk_spm_regs, pcm_im_host_rw_dat, 0x003c); : check_member(mtk_spm_regs, pcm_event_vector0, 0x0040); : check_member(mtk_spm_regs, pcm_event_vector1, 0x0044); : check_member(mtk_spm_regs, pcm_event_vector2, 0x0048); : check_member(mtk_spm_regs, pcm_event_vector3, 0x004c); : check_member(mtk_spm_regs, pcm_event_vector4, 0x0050); : check_member(mtk_spm_regs, pcm_event_vector5, 0x0054); : check_member(mtk_spm_regs, pcm_event_vector6, 0x0058); : check_member(mtk_spm_regs, pcm_event_vector7, 0x005c); : check_member(mtk_spm_regs, pcm_event_vector8, 0x0060); : check_member(mtk_spm_regs, pcm_event_vector9, 0x0064); : check_member(mtk_spm_regs, pcm_event_vector10, 0x0068); : check_member(mtk_spm_regs, pcm_event_vector11, 0x006c); : check_member(mtk_spm_regs, pcm_event_vector12, 0x0070); : check_member(mtk_spm_regs, pcm_event_vector13, 0x0074); : check_member(mtk_spm_regs, pcm_event_vector14, 0x0078); : check_member(mtk_spm_regs, pcm_event_vector15, 0x007c); : check_member(mtk_spm_regs, spm_swint_clr, 0x0094); : check_member(mtk_spm_regs, spm_cpu_wakeup_event, 0x00b0); : check_member(mtk_spm_regs, spm_irq_mask, 0x00b4); : check_member(mtk_spm_regs, spm_wakeup_event_mask, 0x00c4); : check_member(mtk_spm_regs, ddr_en_dbc_len, 0x00d8);
yes
Done
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/in... File src/soc/mediatek/mt8183/include/soc/spm_init.h:
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/in... PS14, Line 16: #ifndef __SPM_INIT__
Done
Done
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/in... PS14, Line 137: WAKE_SRC_R12_PCM_TIMER = (1U << 0),
ok, I would change it on next version.
Done
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/in... PS14, Line 178: u32 vec0;
ok, I would change it on next version.
Done
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/in... PS14, Line 196: pwr_ctrl
Yes, I would remove these on next version.
Done
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/in... PS14, Line 301: wake_status
Yes, I would remove these on next version.
Done
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... File src/soc/mediatek/mt8183/spm.c:
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 46: if ((pcm_fsm_sta & PCM_FSM_STA_MASK) != PCM_FSM_STA_DEF) : printk(BIOS_ERR, "PCM reset failed\n"); :
ok, I would update it on next version.
Done
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 94: unsigned int
ok, I would update it on next version.
Done
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 102: wait_us
ok, I would update it on next version.
Done
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 105: r15
ok, I would update it on next version.
Done
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 117: unsigned int
ok, I would update it on next version.
Done
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 128: write32
Yes, but I need write project code for high 16 bit to clear such register at the same time. […]
Done
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 138: 0x%x
ok, I would update it on next version.
Done
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 162: unsigned int
ok, I would update it on next version.
Done
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 174: nsigned int
ok, I would update it on next version.
Done
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 184: ptr = (unsigned int)(unsigned long)(pcmdesc->base);
ok, I would update it on next version.
Done
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 185: len = pcmdesc->size - 1;
ok, I would update it on next version.
Done
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 214: write32
ok, I would try to update it on next version.
Done
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 260: 2;
ok, I would update it on next version.
Done
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 323: void
ok, I would update it on next version.
Done
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 346: return
ok, I would update it on next version.
Done
https://review.coreboot.org/c/coreboot/+/34545/25/src/soc/mediatek/mt8183/sp... File src/soc/mediatek/mt8183/spm.c:
https://review.coreboot.org/c/coreboot/+/34545/25/src/soc/mediatek/mt8183/sp... PS25, Line 127: !(read32(&mtk_spm->pcm_reg15_data) == SPM_PCM_REG15_DATA_CHECK)) { Change !(x == y) to (x != y), and remove unnecessary parentheses.
https://review.coreboot.org/c/coreboot/+/34545/25/src/soc/mediatek/mt8183/sp... PS25, Line 226: 16 Consider using ARRAY_SIZE(mtk_spm->pcm_event_vector).