DAWEI CHIEN has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34545 )
Change subject: SPM: mt8183: support spm suspend in coreboot ......................................................................
Patch Set 2: -Code-Review
(18 comments)
Patch Set 2:
(5 comments)
Please add a more elaborate commit message for such a huge diff stat. What is SPM and why is it needed? Why does it have to be set up by the firmware, and it’s not enough that the OS sets it up? From what datasheet did you take the macro definitions?
Also, please add a follow-up commit with additional time-stamps (or stopwatch messages) so the time of this can be measured.
What is the boot time penalty of this commit?
==> Yes, I would add more information in comment message, and try to explain it more clear.
What is SPM and why is it needed? Why does it have to be set up by the firmware? ==> For SOC power saving There is a HW module inside SOC, we call this HW module SPM. After linux PM suspend, SPM would turn off last CPU power and do more powersave for SOC (such like DRAM self-refresh mode, reduce SOC voltage level, turn off 26M crystal,...)
SPM need firmware to turn off SoC power or do power-save in right time and right condition. Otherwise, SPM can't do any thing after linux PM suspend.
From what datasheet did you take the macro definitions? ==> MT8183 SPM datasheet
Also, please add a follow-up commit with additional time-stamps (or stopwatch messages) so the time of this can be measured ==> I would have a try to add time-stamps on next version.
https://review.coreboot.org/c/coreboot/+/34545/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34545/2//COMMIT_MSG@9 PS2, Line 9: core boot
coreboot
ok, would update on next version.
https://review.coreboot.org/c/coreboot/+/34545/2//COMMIT_MSG@10 PS2, Line 10:
Tested how? Why is the SPM firmware needed for suspend?
Test by powerd_dbus_suspend (it would call linux suspend to ram)
There is a HW module inside SOC, we call this HW module SPM. After linux PM suspend, SPM would turn off last CPU power and do more powersave for SOC (such like DRAM self-refresh mode, reduce SOC voltage level, turn off 26M crystal,...)
https://review.coreboot.org/c/coreboot/+/34545/2//COMMIT_MSG@11 PS2, Line 11: Change-Id: I3393a772f025b0912a5a25a63a87512454fbc86e
Please add your Signed-off-by line below.
ok, I would update on next version.
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
what's magic 0x154? maybe put a #define or enum with meaningful name.
I would put a define for meaningful name on next version. => 0x154 = MD_DDR_EN_0_DBC_LEN => (0x154 << 10) = MD_DDR_EN_1_DBC_LEN => (0x154 << 20) = CONN_DDR_EN_DBC_LEN
https://review.coreboot.org/c/coreboot/+/34545/1/src/soc/mediatek/mt8183/spm... PS1, Line 68: 0x3f
what is 0x3f?
I would put a define for meaningful name on next version. => 0x3f = IFR_SRAMROM_ROM_PDN
https://review.coreboot.org/c/coreboot/+/34545/1/src/soc/mediatek/mt8183/spm... PS1, Line 69: 0x8
what is 0x8?
I would put a define for meaningful name on next version. => 0x8 = SPM_PC_TRACE_OFFSET
https://review.coreboot.org/c/coreboot/+/34545/1/src/soc/mediatek/mt8183/spm... PS1, Line 77: 5000
do you want to retry by "number" or "time (i.e. […]
retry for time, and next version that I would add udelay(1) on while loop (max timeout = 5000 * 1us)
https://review.coreboot.org/c/coreboot/+/34545/1/src/soc/mediatek/mt8183/spm... PS1, Line 123: (PCM_WDT_WAKE_MODE_LSB)
no need to quote.
ok, would fix on next version.
https://review.coreboot.org/c/coreboot/+/34545/1/src/soc/mediatek/mt8183/spm... PS1, Line 162: (unsigned int)(unsigned long)
why doing twice
This is for ptr get the dram address of pcmdesc->base (ptr is for unsigned int and the value is from dram address of pcmdesc->base) Actually, it is for debug purpose only, maybe I need remove 'ptr'.
https://review.coreboot.org/c/coreboot/+/34545/1/src/soc/mediatek/mt8183/spm... PS1, Line 253: 128 - 1
use sizeof or some named constant. […]
ok, I would update it on next version for name and don't need to -1.
https://review.coreboot.org/c/coreboot/+/34545/1/src/soc/mediatek/mt8183/spm... PS1, Line 255:
no space between type casting and variable,
ok, I would update it on next version.
https://review.coreboot.org/c/coreboot/+/34545/1/src/soc/mediatek/mt8183/spm... PS1, Line 291: void
Does SPM need to be always initialized inside SOC implementation? […]
In my opinion, it maybe keep inside SOC since SPM HW is inside SOC, and we can't mount it on board. and would you please explain what SDRAM config is.
Hi SJ, Would you have any comment?
https://review.coreboot.org/c/coreboot/+/34545/1/src/soc/mediatek/mt8183/spm... PS1, Line 297: DRAM_TYPE_EMCP
if in the end we hve to choose either 3200 or 3733, why not just put that in DRAM configuration as ' […]
Hi Hung-Te, I loop DRAM ownen for reviewer, and let him answer this question.
Hi Huayang, How about this suggest, would you please have a comment, thank you.
https://review.coreboot.org/c/coreboot/+/34545/2/src/soc/mediatek/mt8183/spm... File src/soc/mediatek/mt8183/spm.c:
https://review.coreboot.org/c/coreboot/+/34545/2/src/soc/mediatek/mt8183/spm... PS2, Line 225: CBFS_TYPE_RAW);
Please use the full text width.
ok, I would update on next version.
https://review.coreboot.org/c/coreboot/+/34545/2/src/soc/mediatek/mt8183/spm... PS2, Line 243: BIOS_INFO
Probably more debug level?
ok, I would update on next version.
https://review.coreboot.org/c/coreboot/+/34545/2/src/soc/mediatek/mt8183/spm... PS2, Line 244: (unsigned int)(unsigned long)
Why the double cast?
This is for print dram address of pcmdesc->base (dram address is for unsigned int and the value is from dram address of dyna_load_pcm[index].buf) Actually, it is for debug purpose only, maybe I need remove it, or use %p to print it.
https://review.coreboot.org/c/coreboot/+/34545/2/src/soc/mediatek/mt8183/spm... PS2, Line 305: spm_load_firmware(index);
Did you add time-stamps for this? I thinks these thinks should be measured for how long the take.
I would try to add on next version.
https://review.coreboot.org/c/coreboot/+/34545/2/src/soc/mediatek/mt8183/spm... PS2, Line 310: printk(BIOS_ERR, "SPM: firmware is not ready!!!\n");
As this is a user visible message, please add more information, what the user should do.
User need check dram type and if this driver could get right firmware. I would try to add more information on next version.