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:
(13 comments)
also, I think there are lots of white-space-only changes, to combine those function calls into a larger line > 80 cols.
I think we don't want to mix styling, especially white-space only changes, with CLs that really changing code logic. Please revert those, or have them done in a separate CL (although I don't think those should be merged since they're breaking the col 80 role).
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)); exceed col 80
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/dr... PS12, Line 248: 0x3fffff << 10); no need to fix white space here - keep this unchanged (as two lines).
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/dr... PS12, Line 251: 0x3fffff << 10, 0x2 << 10); should be no white space change here
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/dr... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/dr... PS12, Line 314: , 0x8060033e | (0x40 << (en ? 0x1 : 0))); : write32(&ch[chn].phy.misc_cg_ctrl2, 0x8060033f | (0x40 << (en ? 0x1 : 0))); : write32(&ch[chn].phy.misc_cg_ctrl2, 0x8060033e | (0x40 << (en ? 0x1 : 0))); : : clrsetbits_le32(&ch[chn].phy.misc_ctrl3, 0x3 << 26, (en ? 0 : 0x3) << 26); exceed col 80
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/dr... PS12, Line 349: < 14, (on ? 0x3 : 0) << 14); no white space only changes.
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/em... File src/soc/mediatek/mt8183/emi.c:
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/em... PS12, Line 21: : u32 freqTbl[LP4X_DDRFREQ_MAX] = {DDR_FREQ_1600, DDR_FREQ_3200, DDR_FREQ_3600, DDR_FREQ_2400}; u32 frequency_table[LP4X_DDRFREQ_MAX] = { [LP4X_DDRFREQ_1600] = DDR_FREQ_1600, [LP4X_DDRFREQ_3200] = DDR_FREQ_3200, [LP4X_DDRFREQ_3600] = DDR_FREQ_3600, [LP4X_DDRFREQ_2400] = DDR_FREQ_2400, };
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/em... PS12, Line 37: typedef we don't do typedef according to kernel coding style. just use struct.
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/em... PS12, Line 282: //DDR1600 use the [LP4X_DDRFREQ_1600]= {.rfc...} style so there's no need to comment (and match ordering).
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/em... PS12, Line 287: freq_group, exceed col 80
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 57: (CONFIG(MT8183_DRAM_EMCP)) if CONFIG(MT8183_DRAM_EMCP)
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/in... PS12, Line 58: (LP4X_DDR3600) no need to add () if this is single value.
Meanwhile, from your current implementation there's only emi.c needs to decide this value, so I'd more prefer to not do the header definition; instead you should put the logic in .c file, i.e.,
u32 current_freq;
if (CONFIG(MT8183_DRAM_EMCP)) current_freq = LP4X_DDR3600; else current_freq = LP4X_DDR3200;
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/in... PS12, Line 59: LP4X_MIDDLE_FREQ not used anywhere?
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/in... PS12, Line 60: LP4X_LOW_FREQ not used anywhere?