Erin Lo has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31516 )
Change subject: google/kukui: boot up sspm ......................................................................
Patch Set 2:
(14 comments)
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/Makefile.... File src/mainboard/google/kukui/Makefile.inc:
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/Makefile.... PS1, Line 32: =$
insert a space character
Done
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... File src/mainboard/google/kukui/mainboard.c:
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 16: #include <console/console.h> : #include <cbfs.h> : #include <device/device.h> : #include <soc/gpio.h> : #include <soc/mmu_operations.h> : #include <soc/usb.h> : #include <soc/sspm.h>
sort include lines
Done
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 49: CBFS_TYPE_RAW);
code indent should use tabs where possible
Done
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 49: CBFS_TYPE_RAW);
please, no spaces at the start of a line
Done
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 55: buf[0], fw_size - 1, buf[fw_size - 1]);
please, no spaces at the start of a line
Done
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 55: buf[0], fw_size - 1, buf[fw_size - 1]);
code indent should use tabs where possible
Done
https://review.coreboot.org/#/c/31516/1/src/mainboard/google/kukui/mainboard... PS1, Line 65: sspm_boot
configure_sspm, to this is more consistent with other functions in same file.
Done
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/include/soc/... File src/soc/mediatek/mt8183/include/soc/addressmap.h:
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/include/soc/... PS1, Line 37:
tab
Done
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/include/soc/... File src/soc/mediatek/mt8183/include/soc/sspm.h:
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/include/soc/... PS1, Line 26: #define CFG_SSPM_SRAM 0x10400000
I will move it to addresmap. […]
Done
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/include/soc/... PS1, Line 27: s32
Would it be better to return int?
Done
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/sspm.c File src/soc/mediatek/mt8183/sspm.c:
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/sspm.c@22 PS1, Line 22: #define SSPM_UART 1
It is about pin selection depends on platform and just for debug. […]
Done
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/sspm.c@25 PS1, Line 25: memcpy((void*)CFG_SSPM_SRAM, buf, len);
"(foo*)" should be "(foo *)"
Done
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/sspm.c@30 PS1, Line 30: mb();
memory barrier without comment
Done
https://review.coreboot.org/#/c/31516/1/src/soc/mediatek/mt8183/sspm.c@31 PS1, Line 31: write32
resetting SSPM
Done