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 16:
(17 comments)
https://review.coreboot.org/c/coreboot/+/34545/1/src/soc/mediatek/mt8183/spm... File src/soc/mediatek/mt8183/spm.c:
https://review.coreboot.org/c/coreboot/+/34545/1/src/soc/mediatek/mt8183/spm... PS1, Line 65: 0x154
I would put a define for meaningful name on next version. […]
Ack
https://review.coreboot.org/c/coreboot/+/34545/1/src/soc/mediatek/mt8183/spm... PS1, Line 68: 0x3f
I would put a define for meaningful name on next version. […]
Ack
https://review.coreboot.org/c/coreboot/+/34545/1/src/soc/mediatek/mt8183/spm... PS1, Line 69: 0x8
I would put a define for meaningful name on next version. […]
Ack
https://review.coreboot.org/c/coreboot/+/34545/1/src/soc/mediatek/mt8183/spm... PS1, Line 77: 5000
retry for time, and next version that I would add udelay(1) on while loop […]
Ack
https://review.coreboot.org/c/coreboot/+/34545/1/src/soc/mediatek/mt8183/spm... PS1, Line 77: unsigned int con1, retry = 0, timeout = 5000;
You might want to add the unit to the variable name: timeout_us. […]
Ack
https://review.coreboot.org/c/coreboot/+/34545/1/src/soc/mediatek/mt8183/spm... PS1, Line 123: (PCM_WDT_WAKE_MODE_LSB)
ok, would fix on next version.
Ack
https://review.coreboot.org/c/coreboot/+/34545/1/src/soc/mediatek/mt8183/spm... PS1, Line 162: (unsigned int)(unsigned long)
This is for ptr get the dram address of pcmdesc->base […]
Ack
https://review.coreboot.org/c/coreboot/+/34545/1/src/soc/mediatek/mt8183/spm... PS1, Line 168: printk(BIOS_INFO, "spm_kick_im_to_fetch: ptr = 0x%x\n", ptr);
Prefer using '"%s... […]
Ack
https://review.coreboot.org/c/coreboot/+/34545/1/src/soc/mediatek/mt8183/spm... PS1, Line 169: printk(BIOS_INFO, "spm_kick_im_to_fetch: len = %d\n", len);
Prefer using '"%s... […]
Ack
https://review.coreboot.org/c/coreboot/+/34545/1/src/soc/mediatek/mt8183/spm... PS1, Line 208: static const char *dyna_load_pcm_path[] = {
static const char * array should probably be static const char * const
Ack
https://review.coreboot.org/c/coreboot/+/34545/1/src/soc/mediatek/mt8183/spm... PS1, Line 253: 128 - 1
ok, I would update it on next version for name and don't need to -1.
Ack
https://review.coreboot.org/c/coreboot/+/34545/1/src/soc/mediatek/mt8183/spm... PS1, Line 255:
ok, I would update it on next version.
Ack
https://review.coreboot.org/c/coreboot/+/34545/1/src/soc/mediatek/mt8183/spm... PS1, Line 276: printk(BIOS_INFO, "SPM spm_kick_pcm_to_run\n");
Prefer using '"%s... […]
Ack
https://review.coreboot.org/c/coreboot/+/34545/1/src/soc/mediatek/mt8183/spm... PS1, Line 288: printk(BIOS_INFO, "SPM spm_kick_pcm_to_run done\n");
Prefer using '"%s... […]
Ack
https://review.coreboot.org/c/coreboot/+/34545/1/src/soc/mediatek/mt8183/spm... PS1, Line 291: void
In my opinion, it maybe keep inside SOC since SPM HW is inside SOC, and we can't mount it on board. […]
That is the DRAM config we'll load from board folder.
https://review.coreboot.org/c/coreboot/+/34545/1/src/soc/mediatek/mt8183/spm... PS1, Line 297: DRAM_TYPE_EMCP
Hi Hung-Te, […]
Ack
https://review.coreboot.org/c/coreboot/+/34545/1/src/soc/mediatek/mt8183/spm... PS1, Line 320: printk(BIOS_INFO, "SPM spm_init done\n");
Prefer using '"%s... […]
Ack