Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44730 )
Change subject: soc/mediatek/mt8192: Switch to highest DDR frequency to reduce bootup time ......................................................................
Patch Set 40:
(4 comments)
https://review.coreboot.org/c/coreboot/+/44730/40//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44730/40//COMMIT_MSG@8 PS40, Line 8: Please elaborate, what the highest frequency is, and give concrete boottime numbers without and with the change.
https://review.coreboot.org/c/coreboot/+/44730/40/src/soc/mediatek/mt8192/dr... File src/soc/mediatek/mt8192/dramc_dvfs.c:
https://review.coreboot.org/c/coreboot/+/44730/40/src/soc/mediatek/mt8192/dr... PS40, Line 34: perfctl0_bak = (read32(&ch[0].ao.perfctl0) >> 10) & 0x3; Is this in the right if branch?
https://review.coreboot.org/c/coreboot/+/44730/40/src/soc/mediatek/mt8192/dr... PS40, Line 33: if (wa_enable) { : perfctl0_bak = (read32(&ch[0].ao.perfctl0) >> 10) & 0x3; : SET32_BITFIELDS(&ch[0].ao.perfctl0, : PERFCTL0_RWAGEEN, 0, : PERFCTL0_EMILLATEN, 0); : } else { : SET32_BITFIELDS(&ch[0].ao.perfctl0, : PERFCTL0_RWAGEEN, perfctl0_bak & 0x1, : PERFCTL0_EMILLATEN, (perfctl0_bak >> 1) & 0x1); : } I’d use the ternary operator:
wa_enable ? 0 : PERFCTL0_RWAGEEN, perfctl0_bak & 0x1
https://review.coreboot.org/c/coreboot/+/44730/40/src/soc/mediatek/mt8192/dr... PS40, Line 49: u8 ack_state = 0, complete = 1; Why not use a boolean?