Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34332 )
Change subject: mediatek/mt8183: Support more DRAM frequency bootup ......................................................................
Patch Set 39:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34332/35/src/soc/mediatek/mt8183/dr... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/34332/35/src/soc/mediatek/mt8183/dr... PS35, Line 108: ((cke_on ? 1 : 0) << 6) | ((cke_off ? 1 : 0) << 7));
@hungte […]
I'm ok either way.
For C++ bool is properly defined to be true=1, false=0. For C99 the stdbool has same definition. For other, that is undefined.
Coreboot is not really using stdbool, but there was plan to migrate so it's fine.
https://review.coreboot.org/c/coreboot/+/34332/35/src/soc/mediatek/mt8183/dr... PS35, Line 1254: else : *begin = -64;
I don't see how this could possibly happen because the above conditions seem to include all the case […]
Ack
https://review.coreboot.org/c/coreboot/+/34332/35/src/soc/mediatek/mt8183/dr... PS35, Line 1545: clock_rate = 800;
@hungte […]
I think we should really die on things we can't be sure if that will work or not.
https://review.coreboot.org/c/coreboot/+/34332/38/src/soc/mediatek/mt8183/dr... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/34332/38/src/soc/mediatek/mt8183/dr... PS38, Line 1258: else : *begin = -64; this implies not a well-supported case that maybe we should probably die or also warn something.