Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34332 )
Change subject: mediatek/mt8183: Support DDR frequency 3600Mbps ......................................................................
Patch Set 12:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/dr... File src/soc/mediatek/mt8183/dramc_pi_basic_api.c:
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/dr... PS12, Line 177: (0x1 << 29) | (0x0 << 4) | (0x1 << 0));
I see the check-patch script only complain line over 96 characters, so to reduce the line count,move […]
I think "line count of existing file" is not important; in fact it's more important for "line count of diff in one patchset for review".
There're some ongoing discussion in columns of line for Coreboot. Some suggested using clangformat, but the docs.coreboot.org is still saying 80, "unless exceeding 80 columns significantly increases readability and does not hide information".
No matter what, we should not put these white-space changes into a change with real functional changes. So please remove/revert them. You can create one dedicated patchset for fixing all whitespace if you really think that's the better format.
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/in... File src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h:
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/in... PS12, Line 59: LP4X_MIDDLE_FREQ
DVFS feature will using 3 freq, that is low,middle,high freq.
Got it. In that case, Can we put them into an array, and let some code (in emi.c?) select the right array? Unless if the high/middle/low will be used in many different places and can't be parsed as arguments.