Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37996 )
Change subject: soc/mediatek/mt8183: do TX tracking for DRAM DVFS feature ......................................................................
Patch Set 9:
(26 comments)
https://review.coreboot.org/c/coreboot/+/37996/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37996/5//COMMIT_MSG@10 PS5, Line 10:
Ack
Done
https://review.coreboot.org/c/coreboot/+/37996/9//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37996/9//COMMIT_MSG@9 PS9, Line 9: maybe may be
https://review.coreboot.org/c/coreboot/+/37996/9//COMMIT_MSG@9 PS9, Line 9: do does
https://review.coreboot.org/c/coreboot/+/37996/9//COMMIT_MSG@9 PS9, Line 9: it maybe : cause the TX data transmission error.
It may cause a TX data transmission error.
Is that visible in the logs? Does the boot hang then?
https://review.coreboot.org/c/coreboot/+/37996/9//COMMIT_MSG@11 PS9, Line 11: Need use standard dqsosc do TX window tracking for DDR DVFS feature. Do you mean?
So, use standard dqsosc TX window tracking for DDR DVFS feature.
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_basic_api.c:
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 430: u8 unsigned int
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 431: dramc_hw_dqsosc(chn); Please change the function signature to `unsigned int`.
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 114: /* Wait MRW command fired */ Maybe?
Wait until MRW command fired
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 1635: {0} Please add spaces around the 0.
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 1683: dramc_dbg("u1DelayCellOfst[%d]=%d cells (%d PI)\n", Should be a separate commit next time.
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2139: while (!wait_us(10, read32(&ch[chn].nao.spcmdresp) & (0x1 << 10))) { Can you use coreboot’s stopwatch framework?
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2139: 0x1 << 10 Define a macro for that bit?
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2141: dramc_err("start DQSOSC fail (time out)\n"); Maybe:
Start of DQSOSC timed out after ten iterations
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2141: time out The noun is spelled *timeout* or *time-out*.
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2148: u16 *osc_thrd_inc, u16 *osc_thrd_dec) This should fit on one line with 96 text width?
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2150: u8 mr23 = 0x3F; Add a comment, why you initialize this to 0x3f?
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2171: u8 unsigned int?
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2183: 0xFF coreboot uses lowercase hex numbers.
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2185: 0x%X %#X adds the 0x itself to my knowledge.
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2214: void dramc_hw_dqsosc(u8 chn) Add a function description (with a reference to the datasheet section)?
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2243: u8 unsigned int
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2248: u16 *osc_thrd_inc, u16 *osc_thrd_dec) Should fit on one line.
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2251: u8 mr23 = 0x3F; Why this value?
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2291: u8 unsigned int
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2316: return ((n < 0) ^ (d < 0)) ? ((n - d/2)/d) : ((n + d/2)/d); Please add spaces around the operators.
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/emi... File src/soc/mediatek/mt8183/emi.c:
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/emi... PS9, Line 87: freq_tbl = freq_shuffle; freq_tbl = CONFIG_MT8183_DRAM_EMCP ? freq_shuffle_emcp : freq_shuffle