Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34545 )
Change subject: SPM: mt8183: support spm suspend in coreboot ......................................................................
Patch Set 1:
(10 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 what's magic 0x154? maybe put a #define or enum with meaningful name.
https://review.coreboot.org/c/coreboot/+/34545/1/src/soc/mediatek/mt8183/spm... PS1, Line 68: 0x3f what is 0x3f?
https://review.coreboot.org/c/coreboot/+/34545/1/src/soc/mediatek/mt8183/spm... PS1, Line 69: 0x8 what is 0x8?
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., 50us or 50us?)"?
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.
https://review.coreboot.org/c/coreboot/+/34545/1/src/soc/mediatek/mt8183/spm... PS1, Line 162: (unsigned int)(unsigned long) why doing twice
https://review.coreboot.org/c/coreboot/+/34545/1/src/soc/mediatek/mt8183/spm... PS1, Line 253: 128 - 1 use sizeof or some named constant.
also, snprintf will add NUL so you 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,
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?
I feel it makes more sense if we do this in board level, that in ROM stage or somewhere after RAM is loaded, we load SDRAM config, and pass the type/speed into spm_init.
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 'int maxspeed'?
Then we can even decide the path by sprintf("..%d", speed), no need to do t he conversion so many times.