Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34988 )
Change subject: mediatek/mt8183: Implement the dramc init setting ......................................................................
Patch Set 13:
(6 comments)
https://review.coreboot.org/c/coreboot/+/34988/2/src/soc/mediatek/mt8183/dra... File src/soc/mediatek/mt8183/dramc_pi_basic_api.c:
https://review.coreboot.org/c/coreboot/+/34988/2/src/soc/mediatek/mt8183/dra... PS2, Line 23: impedance
dramc_sw_impedance_save_reg() will be called many times at different work flow, […]
Ack
https://review.coreboot.org/c/coreboot/+/34988/2/src/soc/mediatek/mt8183/inc... File src/soc/mediatek/mt8183/include/soc/dramc_pi_api.h:
https://review.coreboot.org/c/coreboot/+/34988/2/src/soc/mediatek/mt8183/inc... PS2, Line 119: 0x2
I think it's usually easier to read if there are named values. […]
By MTK's spec, '0x2' means "Unit: 1x clock", so actually I don't think it's a magic number.
https://review.coreboot.org/c/coreboot/+/34988/2/src/soc/mediatek/mt8183/inc... PS2, Line 121: 0x1
DQS_OEN_DELAY_0P5T ?
This constant is removed in the next CL (https://review.coreboot.org/c/coreboot/+/34332/29).
https://review.coreboot.org/c/coreboot/+/34988/2/src/soc/mediatek/mt8183/inc... PS2, Line 129: 0x3
DQS_DELAY_2T ?
Same as above.
https://review.coreboot.org/c/coreboot/+/34988/2/src/soc/mediatek/mt8183/inc... PS2, Line 139: 0x4
Shouldn't this be 'DQS_DELAY_0P5T' ?
Same as above.
https://review.coreboot.org/c/coreboot/+/34988/2/src/soc/mediatek/mt8183/inc... PS2, Line 141: 0x3
And this, 'DQS_OEN_DELAY_2T' ?
Same as above.