Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Support 6GB, 8GB DDR bootup ......................................................................
Patch Set 7:
(6 comments)
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... PS4, Line 134: u16
the MR value read by in value = READ32_BITFIELD(&ch[chn].nao. […]
Shouldn't the local variable 'value' be defined to be 'u16' instead of 'u8'? Then we can change the return type of this function to 'u8' and return 'value & 0xff' in this function.
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... PS4, Line 2694: get_dram_infor_after_cal
Done
I agree. Please split this function into two, and call them with
dramc_show("Vendor id is %#x\n", dramc_get_calibrated_vendor_id()); density = dramc_get_calibrated_max_density();
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... PS4, Line 2737: 27
from the LPDDR4 standard SPEC of ‘JESD209-4B.pdf’, the MR8[5:2] is the density size. […]
What's the unit of 'dram_size'? If it's in bytes, this should be 30 instead of 27.
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... File src/soc/mediatek/mt8183/emi.c:
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 389: dramc_dbg
Done
Forgot to change this?
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 390: break
from the LPDDR4 standard SPEC of ‘JESD209-4B. […]
If it's impossible to reach the 'default' case, let's change this to 'return'.
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 393: rf_cab_opt[freq_group][rfcab_grp]
Done
Forgot to change this?