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 9:
(30 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: do
does
Done
https://review.coreboot.org/c/coreboot/+/37996/9//COMMIT_MSG@9 PS9, Line 9: maybe
may be
Done
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. […]
system will do DVFS switch background after DVFS module init success at kernel, if the DRAM have data error, kernel will report random crash errors.
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? […]
yes
https://review.coreboot.org/c/coreboot/+/37996/5/src/soc/mediatek/mt8183/Kco... File src/soc/mediatek/mt8183/Kconfig:
https://review.coreboot.org/c/coreboot/+/37996/5/src/soc/mediatek/mt8183/Kco... PS5, Line 22: default y
Done
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
unsigned int
why use 'unsigned int' instead of u8 here?
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? […]
Done
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 1635: {0}
Please add spaces around the 0.
Done
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.
Ack
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2139: 0x1 << 10
Define a macro for that bit?
Done
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?
what is the 'coreboot’s stopwatch framework', please give a example.
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*.
Ack
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: […]
Done
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2142: return;
Should we return -1 on error?
no need return error value to caller.
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?
Done
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2150:
Remove extra space
Done
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?
is a default value. From LP4 JEDEC-209 standard SPEC, the MR23 is the 'DQS interval timer run time setting'
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2171: u8
unsigned int?
why unsigned int?
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2183: 0xFF
coreboot uses lowercase hex numbers.
Done
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.
Done
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2189: tck = 1000000 / clock_rate;
Are you sure we want to round down here, rather than round up or round to closest?
can you give more detai?
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2194: (mr23 * tck * tck)
Remove unnecessary parentheses. Same below.
Done
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2216: 0
Please use DRAM_DFS_SHUFFLE_1.
Done
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2221: *
Add spaces around binary operators.
Done
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.
Done
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2251: u8 mr23 = 0x3F;
Why this value?
is a default value. From LP4 JEDEC-209 standard SPEC, the MR23 is the 'DQS interval timer run time setting
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2314: static int div_round_close(const int n, const int d)
There's already DIV_ROUND_CLOSEST in commonlib/include/commonlib/helpers. […]
replace with common API DIV_ROUND_CLOSEST later
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2330: shu_dst >= DRAM_DFS_SHUFFLE_MAX) {
Please align with previous line.
Done
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2340: +
Add spaces around binary operators.
Done
https://review.coreboot.org/c/coreboot/+/37996/9/src/soc/mediatek/mt8183/dra... PS9, Line 2386: SHURK0_SELPH_DQSG0_TX_DLY_DQS0_GATED);
Need one more tab. Same below.
Done