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.
Patch set 2:-Code-Review
18 comments:
Patch Set #2, Line 9: core boot
coreboot
ok, would update on next version.
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,...)
Patch Set #2, Line 11: Change-Id: I3393a772f025b0912a5a25a63a87512454fbc86e
Please add your Signed-off-by line below.
ok, I would update on next version.
File src/soc/mediatek/mt8183/spm.c:
Patch Set #1, 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
what is 0x3f?
I would put a define for meaningful name on next version.
=> 0x3f = IFR_SRAMROM_ROM_PDN
what is 0x8?
I would put a define for meaningful name on next version.
=> 0x8 = SPM_PC_TRACE_OFFSET
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)
Patch Set #1, Line 123: (PCM_WDT_WAKE_MODE_LSB)
no need to quote.
ok, would fix on next version.
Patch Set #1, 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'.
Patch Set #1, 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.
no space between type casting and variable,
ok, I would update it on next version.
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?
Patch Set #1, 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.
File src/soc/mediatek/mt8183/spm.c:
Patch Set #2, Line 225: CBFS_TYPE_RAW);
Please use the full text width.
ok, I would update on next version.
Patch Set #2, Line 243: BIOS_INFO
Probably more debug level?
ok, I would update on next version.
Patch Set #2, 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.
Patch Set #2, 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.
Patch Set #2, 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.
To view, visit change 34545. To unsubscribe, or for help writing mail filters, visit settings.