Duan huayang 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 10:
(11 comments)
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: it maybe : cause the TX data transmission error.
Please add this description to the commit message.
Done
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
Let's use u8 here for consistency.
Ack
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 431: dramc_hw_dqsosc(chn);
Same as above. Type u8 is used for channel throughout soc/mediatek/mt8183.
Ack
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 2139: while (!wait_us(10, read32(&ch[chn].nao.spcmdresp) & (0x1 << 10))) {
Paul, wait_us() is a convenience wrapper around the stopwatch framework (see https://review. […]
Done
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2142: return;
So what would happen if this failed?
the DDR not response to the DRAM controler, the error result undefined!
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2150: u8 mr23 = 0x3F;
Add that as a comment?
I think no need add a comment for this.
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2171: u8
Same as above.
Ack
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2189: tck = 1000000 / clock_rate;
We've seen plenty of situations where the truncation in integral division leads to inaccurate result […]
Ack
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)?
most of this is accomplished by the DRAM control hardware, no need detail description.
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2251: u8 mr23 = 0x3F;
Good idea.
Done
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.
use DIV_ROUND_CLOSEST replace this API.remove this API later.