Duan huayang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41949 )
Change subject: soc/mediatek/mt8183: Add ddr geometry to support 6GB, 8GB DDR bootup ......................................................................
Patch Set 9:
(19 comments)
https://review.coreboot.org/c/coreboot/+/41949/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41949/8//COMMIT_MSG@10 PS8, Line 10: we
Nit: Fits on line above.
Done
https://review.coreboot.org/c/coreboot/+/41949/8//COMMIT_MSG@10 PS8, Line 10: in future by adding geometry information
… in future, so add geometry information …
Done
https://review.coreboot.org/c/coreboot/+/41949/8//COMMIT_MSG@12 PS8, Line 12:
Mention that the header version is increased to 3, and that stuff from `emi.h` is moved.
Done
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 2694: get_dram_infor_after_cal
I agree. Please split this function into two, and call them with […]
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/dra... PS4, Line 2737: 27
What's the unit of 'dram_size'? If it's in bytes, this should be 30 instead of 27.
Done
https://review.coreboot.org/c/coreboot/+/41949/6/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/41949/6/src/soc/mediatek/mt8183/dra... PS6, Line 2694: get_dram_infor_after_cal
ok... you changed that to a pointer. not the type I preferred but fine if you still want to do that. […]
Done
https://review.coreboot.org/c/coreboot/+/41949/8/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_calibration_api.c:
https://review.coreboot.org/c/coreboot/+/41949/8/src/soc/mediatek/mt8183/dra... PS8, Line 2694: infor
Just *info*?
Done
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?
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 326: [LP4X_DDR1600]
Could we preserve this?
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/emi... PS4, Line 390: break
If it's impossible to reach the 'default' case, let's change this to 'return'.
Done
https://review.coreboot.org/c/coreboot/+/41949/6/src/soc/mediatek/mt8183/emi... File src/soc/mediatek/mt8183/emi.c:
https://review.coreboot.org/c/coreboot/+/41949/6/src/soc/mediatek/mt8183/emi... PS6, Line 591: K finished (current clock: %u
K finished (current clock: %u, density: %u)\n"
Done
https://review.coreboot.org/c/coreboot/+/41949/8/src/soc/mediatek/mt8183/emi... File src/soc/mediatek/mt8183/emi.c:
https://review.coreboot.org/c/coreboot/+/41949/8/src/soc/mediatek/mt8183/emi... PS8, Line 56: u8 trfrc_pb_05t;
Renaming the variables, makes this hard to review in Gerrit.
Done
https://review.coreboot.org/c/coreboot/+/41949/8/src/soc/mediatek/mt8183/emi... PS8, Line 327: enum tRFCAB {tRFCAB_130 = 0, tRFCAB_180, tRFCAB_280, tRFCAB_380, tRFCAB_NUM};
Please put each assignment on a separate line.
Done
https://review.coreboot.org/c/coreboot/+/41949/8/src/soc/mediatek/mt8183/emi... PS8, Line 393: dramc_err("density err!\n");
Please make the message more elaborate. Maybe also output the unsupported value.
Done
https://review.coreboot.org/c/coreboot/+/41949/8/src/soc/mediatek/mt8183/emi... PS8, Line 568: u8 density = 0;
The initialization is not needed?
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 67: ddr_geometry
oops, I just realized we're duplicating ddr geometry here. […]
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/inc... PS4, Line 67: ddr_geometry
oops, I just realized we're duplicating ddr geometry here. […]
Done
https://review.coreboot.org/c/coreboot/+/41949/4/src/soc/mediatek/mt8183/inc... PS4, Line 68: DQS_NUMBER
If we follow Hung-Te's suggestion above and keep dramc_param. […]
Done
https://review.coreboot.org/c/coreboot/+/41949/6/src/soc/mediatek/mt8183/inc... File src/soc/mediatek/mt8183/include/soc/emi.h:
https://review.coreboot.org/c/coreboot/+/41949/6/src/soc/mediatek/mt8183/inc... PS6, Line 11: struct dramc_param; : struct dramc_param_ops;
No longer needed given you've included dramc_param. […]
Done