Yu-Ping Wu 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.
system will do DVFS switch background after DVFS module init success at kernel, if the DRAM have dat […]
Please add this description to the commit message.
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.
yes
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
why use 'unsigned int' instead of u8 here?
Let's use u8 here for consistency.
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`.
Same as above. Type u8 is used for channel throughout soc/mediatek/mt8183.
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))) {
what is the 'coreboot’s stopwatch framework', please give a example.
I think Paul means the wait_us() macro. There's already a while loop inside it, could we change this to
if (!wait_us(10 * 10, ...)) dramc_err(...);
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2142: return;
no need return error value to caller.
So what would happen if this failed?
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2171: u8
why unsigned int?
Same as above.
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2189: tck = 1000000 / clock_rate;
can you give more detai?
We've seen plenty of situations where the truncation in integral division leads to inaccurate result. I'm just saying that we should be careful when doing this.
If you have tested this function with 4 different clock rates, then I think we're good here.
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2243: u8
unsigned int
Same.
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2251: u8 mr23 = 0x3F;
is a default value. […]
Since this value appears more than once, we should define a constant for it. Something like:
#define MR23_DEFAULT_VALUE 0x3f
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2291: u8
unsigned int
Same.