DAWEI CHIEN 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];
it is ok to me
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. […]
Do you mean that we don't need to check every register what we use, just check the few critical register which is enough, especially the last one for close number, right?
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. […]
Actually, it's up to you and reviewer's comment, I could move to spm.h. In my opinion, since these are for SPM initial setting, so why don't add a new spm_init. How 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 () […]
ok, I would change it on next version.
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/in... PS14, Line 178: u32 vec0;
u32 vector[16];
ok, I would change it on next version.
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/in... PS14, Line 196: pwr_ctrl
this seems not used anywhere. […]
Yes, I would remove these on next version.
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/in... PS14, Line 301: wake_status
this seems not used anywhere. […]
Yes, I would remove these on next version.
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?
ok, I would update it on next version.
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 94: unsigned int
u32
ok, I would update it on next version.
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?
ok, I would update it on next version.
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
ok, I would update it on next version.
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.
ok, I would update it on next version.
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 128: write32
sounds like you want clrsetbits32
Yes, but I need write project code for high 16 bit to clear such register at the same time. pcm_con1 = pcm con1 | (0xb16 << 16) & ~PCM_TIMER_EN_LSB How do you suggest
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 138: 0x%x
%p
ok, I would update it on next version.
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 162: unsigned int
u32
ok, I would update it on next version.
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 174: nsigned int
u32
ok, I would update it on next version.
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? […]
ok, I would update it on next version.
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.
ok, I would update it on next version.
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
ok, I would try to update it on next version.
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 260: 2;
sizeof(firmware_size)
ok, I would update it on next version.
https://review.coreboot.org/c/coreboot/+/34545/14/src/soc/mediatek/mt8183/sp... PS14, Line 323: void
int
ok, I would update it on next version.
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. […]
ok, I would update it on next version.
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
ok, I would update it on next version.