Ran Bi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31046 )
Change subject: mediatek/mt8183: Add RTC support ......................................................................
Patch Set 7:
(5 comments)
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?
Removed
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. […]
Renamed this to poweroff()
https://review.coreboot.org/#/c/31046/6/src/soc/mediatek/mt8183/rtc.c@237 PS6, Line 237: mdelay(5);
6ms is sort of long... […]
According the spec of DCXO we can only shrink this 6ms to 5.1ms.
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.
Fixed
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. […]
Fixed