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 6:
(13 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.
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.
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... PS4, Line 2730: dram_size = 0; break
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... PS4, Line 2737: 27 Why is this 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 326: [LP4X_DDR1600] Could we preserve this?
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 326: 101 This value won't be used anywhere.
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 329: struct const struct
Or even make this static to simplify the initialization.
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 331: { Could we write
[tRFCAB_130] = {...}
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 389: dramc_dbg dramc_err
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?
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?
const struct optimize_ac_time *opt; // Or whatever name you prefer opt = &rf_cab_opt[freq_group][rfcab_grp]; trfc = opt->trfc; ...
Maybe we don't even need the "trfc"... local variables.
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
1. Do not modify this line, or 2. Align all comments in struct dramc_param_header
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.googlesource.com/c/chromeos/vendor/mtk-dramk/...). Would you consider renaming this to DQS_NUMBER_LP4, or doing the opposite?