Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34332 )
Change subject: mediatek/mt8183: Support more DRAM frequency bootup ......................................................................
Patch Set 18:
(15 comments)
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/dr... File src/soc/mediatek/mt8183/dramc_init_setting.c:
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/dr... PS12, Line 271: ((freq_group == LP4X_DDR3200) || (freq_group == LP4X_DDR3600))
No need to quote. […]
Done
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/dr... PS12, Line 309: ((freq == LP4X_DDR1600) || (freq == LP4X_DDR2400)) ? FSP_0 : FSP_1;
I'd say readability. […]
Done
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/dr... PS12, Line 651: #define TIME_OUT_CNT 100 //100us
since only one function use it, put this into dramc_zq_calibration as […]
Done
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/dr... PS12, Line 672: do { : resp = (read32(&ch[chn].nao.spcmdresp) >> 4) & 0x1; : u4TimeCnt--; : udelay(1); : } while ((resp == 0) && (u4TimeCnt > 0)); : : if (u4TimeCnt == 0) { : dramc_dbg("ZQCAL Start fail (time out)\n"); : return 1; : }
use wait_us macro?
Done
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/dr... PS12, Line 688: do { : resp = (read32(&ch[chn].nao.spcmdresp) >> 6) & 0x1; : u4TimeCnt--; : udelay(1); : } while ((resp == 0) && (u4TimeCnt > 0)); : : if (u4TimeCnt == 0) { : dramc_dbg("ZQCAL Latch fail (time out)\n"); : return 1; : }
use wait_us macro?
Done
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 248: 0x3fffff << 10);
no need to fix white space here - keep this unchanged (as two lines).
Done
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
Done
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
Done
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.
Done
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] = { […]
Done
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.
Done
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).
Done
https://review.coreboot.org/c/coreboot/+/34332/12/src/soc/mediatek/mt8183/em... PS12, Line 287: freq_group,
exceed col 80
Done
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)
Done
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. […]
Done