Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34545 )
Change subject: mediatek/mt8183: Init SPM driver ......................................................................
Patch Set 14:
(23 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; should we change these to pcm_event_vector[16];
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); I don't think these are needed. Usually we just need to check few critical (or those very close to reserved) members, especially the last. Adding lots of regs here do not help.
(same for lines below)
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__ I think we can move whole spm_init.h into spm.h. What do you think?
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/in... PS14, Line 137: WAKE_SRC_R12_PCM_TIMER = (1U << 0), for enum you don't need ()
just
WAKE_SRC_R12_PCM_TIMER = 1U << 0,
(same for lines below)
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/in... PS14, Line 178: u32 vec0; u32 vector[16];
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/in... PS14, Line 196: pwr_ctrl this seems not used anywhere. do we need it?
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/in... PS14, Line 301: wake_status this seems not used anywhere. do we need it?
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"); : should we abort and return?
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 94: unsigned int u32
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 102: wait_us should we abort and return on timeout?
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 105: r15 Should we say something like timeout? this message seems obfuscated
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 117: unsigned int since we'll use these for write/read32 they should be u32.
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 128: write32 sounds like you want clrsetbits32
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 138: 0x%x %p
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 162: unsigned int u32
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 174: nsigned int u32
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 184: ptr = (unsigned int)(unsigned long)(pcmdesc->base); so you only want to print base?
Just do
ptr=%p
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 185: len = pcmdesc->size - 1; just put that in printk.
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 214: write32 change the regs to array and do a for-loop
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 260: 2; sizeof(firmware_size)
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 323: void int
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 331: if CONFIG(MT8183_DRAM_EMCP) : index = DYNA_LOAD_PCM_SUSPEND_LP4_3733; : #else : index = DYNA_LOAD_PCM_SUSPEND_LP4_3200; : #endif Use real code.
if (CONFIG(MT8183_DRAM_EMCP)) index = DYNA_LOAD_PCM...; else index = ....
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 346: return return 1 or return -1 to indicate failure