Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34988 )
Change subject: mediatek/mt8183: Implement the dramc init setting ......................................................................
Patch Set 20:
(6 comments)
https://review.coreboot.org/c/coreboot/+/34988/17/src/soc/mediatek/mt8183/dr... File src/soc/mediatek/mt8183/dramc_init_setting.c:
https://review.coreboot.org/c/coreboot/+/34988/17/src/soc/mediatek/mt8183/dr... PS17, Line 688: u8 MR01Value[FSP_MAX] = {0x26, 0x56}; : u8 MR13Value = (1 << 4) | (1 << 3);
According to @huayang's reply in patchset 2, dramc_sw_impedance_save_reg() will be called many times […]
Yes I think we should put everything together.
https://review.coreboot.org/c/coreboot/+/34988/20/src/soc/mediatek/mt8183/dr... File src/soc/mediatek/mt8183/dramc_init_setting.c:
https://review.coreboot.org/c/coreboot/+/34988/20/src/soc/mediatek/mt8183/dr... PS20, Line 121: 0x0 no need to put 0 - it's automatically initialized to 0 in this case.
https://review.coreboot.org/c/coreboot/+/34988/20/src/soc/mediatek/mt8183/dr... PS20, Line 122: 0x no need to add 0
https://review.coreboot.org/c/coreboot/+/34988/20/src/soc/mediatek/mt8183/dr... PS20, Line 123: 0x0 no need to add 0
https://review.coreboot.org/c/coreboot/+/34988/17/src/soc/mediatek/mt8183/dr... File src/soc/mediatek/mt8183/dramc_pi_basic_api.c:
https://review.coreboot.org/c/coreboot/+/34988/17/src/soc/mediatek/mt8183/dr... PS17, Line 23: impedance can we also pack the impedance to the shared variable?
in fact, I think maybe it's not too hard to simply make this a local var and pass to only needed functions.
https://review.coreboot.org/c/coreboot/+/34988/20/src/soc/mediatek/mt8183/em... File src/soc/mediatek/mt8183/emi.c:
https://review.coreboot.org/c/coreboot/+/34988/20/src/soc/mediatek/mt8183/em... PS20, Line 22: #define LP4X_HIGH_FREQ LP4X_DDR3200 since we'll make this dynamic in future, I see no reason to keep it here as a separate define.
Can we just put LP4X_DDR3200 in the init_dram call?
(Or, make it passed from mainboard.c)