Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31046 )
Change subject: mediatek/mt8183: Add RTC support ......................................................................
Patch Set 6:
(5 comments)
https://review.coreboot.org/#/c/31046/6/src/soc/mediatek/mt8183/rtc.c File src/soc/mediatek/mt8183/rtc.c:
https://review.coreboot.org/#/c/31046/6/src/soc/mediatek/mt8183/rtc.c@192 PS6, Line 192: directly sent to SoC */ Looks like you're not actually doing that in the 8183 version, so the comment doesn't belong here?
https://review.coreboot.org/#/c/31046/6/src/soc/mediatek/mt8183/rtc.c@198 PS6, Line 198: void rtc_bbpu_power_down(void) This shuts the system down, right? If so, please call it poweroff() (from <halt.h>), and call halt() at the end to make sure execution cannot continue.
https://review.coreboot.org/#/c/31046/6/src/soc/mediatek/mt8183/rtc.c@237 PS6, Line 237: mdelay(5); 6ms is sort of long... is there any way we could shorten this wait?
https://review.coreboot.org/#/c/31046/6/src/soc/mediatek/mt8183/rtc.c@251 PS6, Line 251: pwrap_write_field(0x790, 0xF, 0xF, 0); Please give all registers used here named constants.
https://review.coreboot.org/#/c/31046/6/src/soc/mediatek/mt8183/rtc.c@254 PS6, Line 254: switch (rtc_check_state()) { Everything below this line is identical to mt8173. Please factor it out (e.g. rtc_boot_common() or something like that).