Duan huayang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38490 )
Change subject: soc/mediatek/mt8183: improve the DRAMC runtime config flow ......................................................................
Patch Set 1:
(12 comments)
https://review.coreboot.org/c/coreboot/+/38490/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38490/1//COMMIT_MSG@9 PS1, Line 9: to reduce the redundancy logic
No, that would change the meaning. […]
Ack
https://review.coreboot.org/c/coreboot/+/38490/1//COMMIT_MSG@9 PS1, Line 9: move
Move
Done
https://review.coreboot.org/c/coreboot/+/38490/1//COMMIT_MSG@10 PS1, Line 10:
BRANCH=kukui […]
Done
https://review.coreboot.org/c/coreboot/+/38490/1/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_basic_api.c:
https://review.coreboot.org/c/coreboot/+/38490/1/src/soc/mediatek/mt8183/dra... PS1, Line 290: (0x7 << 28) | (0x7 << 24), (0x1 << 28) | (0x0 << 24));
Align with previous line.
Done
https://review.coreboot.org/c/coreboot/+/38490/1/src/soc/mediatek/mt8183/dra... PS1, Line 323: size_t
It should be, the existing code uses u8 in most cases.
Done
https://review.coreboot.org/c/coreboot/+/38490/1/src/soc/mediatek/mt8183/dra... PS1, Line 326: (0x7 << 22) | (0x3 << 14) | (0x1 << 19) | (0x1 << 21));
Align with previous line.
what's the align result? like this? clrbits32(&ch[chn].ao.stbcal, (0x7 << 22) | (0x3 << 14) | (0x1 << 19) | (0x1 << 21));
https://review.coreboot.org/c/coreboot/+/38490/1/src/soc/mediatek/mt8183/dra... PS1, Line 352: 0x3fffff << 10);
Fits on previous line
Done
https://review.coreboot.org/c/coreboot/+/38490/1/src/soc/mediatek/mt8183/dra... PS1, Line 361: write32(&ch[chn].phy.ca_dll_fine_tune[3], 0x3a000);
write32(&ch[chn].phy. […]
Done
https://review.coreboot.org/c/coreboot/+/38490/1/src/soc/mediatek/mt8183/dra... PS1, Line 381: setbits32(&ch[chn].ao.dummy_rd, 0x1 << 25 | 0x1 << 20);
Is this bug fix or improvement?
is a bug fix
https://review.coreboot.org/c/coreboot/+/38490/1/src/soc/mediatek/mt8183/dra... PS1, Line 428: size_t
Why size_t here?
it should u8, update it later
https://review.coreboot.org/c/coreboot/+/38490/1/src/soc/mediatek/mt8183/dra... PS1, Line 431: /* RX_TRACKING: ON */
Also applies to all other comments
Done
https://review.coreboot.org/c/coreboot/+/38490/1/src/soc/mediatek/mt8183/dra... PS1, Line 493: (0x1 << 6) | (0x1 << 4)| (0x1 << 2),
Please correct this […]
Done