Duan huayang 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:
(11 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
u8? The type of the argument 'value' of dramc_mode_reg_write_by_rank() is u8.
the MR value read by in value = READ32_BITFIELD(&ch[chn].nao.mrr_status, MRR_STATUS_MRR_REG); in dramc_mode_reg_read(), the read length is 16, so return value is 16 bits too. but the driver use less 8bit only.
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... PS4, Line 2699: & 0xff
If we return u8 from dramc_mode_reg_write_by_rank(), then we don't have to "& 0xff" here.
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... PS4, Line 2730: dram_size = 0;
break
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... PS4, Line 2737: 27
Why is this 27?
from the LPDDR4 standard SPEC of ‘JESD209-4B.pdf’, the MR8[5:2] is the density size. 0000B: 4Gb dual channel die 0001B: 6Gb dual channel die 0010B: 8Gb dual channel die 0011B: 12Gb dual channel die 0100B: 16Gb dual channel die 0101B: 24Gb dual channel die 0110B: 32Gb dual channel die
this is just print for human readable size
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 329: struct
const struct […]
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 331: {
Could we write […]
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 389: dramc_dbg
dramc_err
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 390: break
Do we want to return here, or assume rfcab_grp = 0 for the remaining of this function?
from the LPDDR4 standard SPEC of ‘JESD209-4B.pdf’, the MR8[5:2] is the density size, it only return 0-6 0000B: 4Gb dual channel die 0001B: 6Gb dual channel die 0010B: 8Gb dual channel die 0011B: 12Gb dual channel die 0100B: 16Gb dual channel die 0101B: 24Gb dual channel die 0110B: 32Gb dual channel die
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 393: rf_cab_opt[freq_group][rfcab_grp]
Could we declare a pointer for this? […]
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/inc... File src/soc/mediatek/mt8183/include/soc/dramc_param.h:
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/inc... PS4, Line 57: u32 size; /* size of whole dramc_param */
Please either […]
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/inc... PS4, Line 68: DQS_NUMBER
This file is starting to diverge from the one in vendor/mtk-dramk (https://chrome-internal-review. […]
the MT8183 SOC also support LPDDR3 DDR, DQS_NUMBER of LP3 is 4 and DQS_NUMBER of the LP4X is 2. but chromepad only have LP4x DDR, so need rename all QS_NUMBER to DQS_NUMBER_LP4 in coreboot?