Ran Bi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31968 )
Change subject: mediatek/mt8183: Fix RTC initialization flow ......................................................................
Patch Set 3:
(10 comments)
https://review.coreboot.org/#/c/31968/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/31968/1//COMMIT_MSG@9 PS1, Line 9: 1. fix RTC initialization flow : 2. fix RTC lpd settings
If there's a specific issue describing details (the 80501386 below is for tracking all firmware chan […]
Done
https://review.coreboot.org/#/c/31968/1//COMMIT_MSG@12 PS1, Line 12: 80501386
127405695?
Done
https://review.coreboot.org/#/c/31968/1/src/soc/mediatek/common/rtc.c File src/soc/mediatek/common/rtc.c:
https://review.coreboot.org/#/c/31968/1/src/soc/mediatek/common/rtc.c@150 PS1, Line 150: printk(BIOS_INFO, "[RTC] con = %x, pwrkey1 = %x, pwrkey2 = %x\n", : con, pwrky1, pwrky2);
Is this for your local debugging, or it is really helpful to always print this?
This log is helpful to get re-init reason. So I would prefer to leave.
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?
Done
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. […]
Done
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?
Add log if pwrap_read or pwrap_write fail. Because pwrap API is RTC basic API. RTC cannot handle this error if pwrap API fail. The only way is print log or assert.
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.
Done
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. […]
Done
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. […]
Yes, the reason of the bug is not write powerkeys again after lpd init.
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?
Removed