Daisuke Nojiri has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31968 )
Change subject: mediatek/mt8183: Fix RTC initialization flow ......................................................................
Patch Set 1: Code-Review-1
(7 comments)
https://review.coreboot.org/#/c/31968/1/src/soc/mediatek/mt8183/rtc.c File src/soc/mediatek/mt8183/rtc.c:
https://review.coreboot.org/#/c/31968/1/src/soc/mediatek/mt8183/rtc.c@25 PS1, Line 25: void Why are we not handling any errors?
https://review.coreboot.org/#/c/31968/1/src/soc/mediatek/mt8183/rtc.c@94 PS1, Line 94: pwrap_read(RTC_CON, &con); : con |= RTC_CON_XOSC32_LPEN; : con &= ~RTC_CON_LPRST; : pwrap_write(RTC_CON, con); : if (!rtc_write_trigger()) : return 0; : : con |= RTC_CON_LPRST; : pwrap_write(RTC_CON, con); : if (!rtc_write_trigger()) : return 0; : : con &= ~RTC_CON_LPRST; : pwrap_write(RTC_CON, con); : if (!rtc_write_trigger()) : return 0; : : pwrap_read(RTC_CON, &con); : con |= RTC_CON_EOSC32_LPEN; : con &= ~RTC_CON_LPRST; : pwrap_write(RTC_CON, con); : if (!rtc_write_trigger()) : return 0; : : con |= RTC_CON_LPRST; : pwrap_write(RTC_CON, con); : if (!rtc_write_trigger()) : return 0; : : con &= ~RTC_CON_LPRST; : pwrap_write(RTC_CON, con); : if (!rtc_write_trigger()) : return 0; Can you do this in a sub-routine to avoid repetition? The only difference is XOSC32 and EOSC32. Also, what does this RTC_CON_LPRST toggle do? More comments are appreciated.
https://review.coreboot.org/#/c/31968/1/src/soc/mediatek/mt8183/rtc.c@128 PS1, Line 128: pwrap_read Why do we assume this is always successful?
https://review.coreboot.org/#/c/31968/1/src/soc/mediatek/mt8183/rtc.c@136 PS1, Line 136: pwrap_write This one too and all the calls in the entire file.
https://review.coreboot.org/#/c/31968/1/src/soc/mediatek/mt8183/rtc.c@167 PS1, Line 167: bool I suggest we return int here as an error code and print it in mediatek/common/rtc.c or add printk for each 'return false' below. That way, at least we can tell how rtc_init failed (or succeeded). Currently, all the failures are seen as 'recovery' and the result of the recovery run is invisible.
https://review.coreboot.org/#/c/31968/1/src/soc/mediatek/mt8183/rtc.c@197 PS1, Line 197: write powerkeys again to enable lpd How does writing powerkeys enable LPD? More comments are appreciated. Was it the source of the bug that rtc_lpd_init wasn't called before powerkeys?
https://review.coreboot.org/#/c/31968/1/src/soc/mediatek/mt8183/rtc.c@203 PS1, Line 203: rtc_writeif_unlock Why do you have to unlock again?