Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38474 )
Change subject: soc/mediatek/mt8183: Fix typo error of DRAMC setting ......................................................................
Patch Set 3: Code-Review+1
(4 comments)
https://review.coreboot.org/c/coreboot/+/38474/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38474/3//COMMIT_MSG@7 PS3, Line 7: soc/mediatek/mt8183: Fix typo error of DRAMC setting As the errors are not typos, maybe change the commit summary:
soc/mediatek/mt8183: Fix programming errors of DRAMC settings
https://review.coreboot.org/c/coreboot/+/38474/3//COMMIT_MSG@11 PS3, Line 11:
Why not two commits?
I'm not sure if it's worth the hassle with such tiny changes.
https://review.coreboot.org/c/coreboot/+/38474/3/src/soc/mediatek/mt8183/emi... File src/soc/mediatek/mt8183/emi.c:
https://review.coreboot.org/c/coreboot/+/38474/3/src/soc/mediatek/mt8183/emi... PS3, Line 328: [LP4X_DDR2400] = {.rfc = 72, .rfc_05t = 0, .tx_ref_cnt = 91},
Agree with Paul Menzel. This is clearly not a typo.
I guess the error was a "copy-pasta" (copied and pasted, but did not change the values)
https://review.coreboot.org/c/coreboot/+/38474/3/src/soc/mediatek/mt8183/emi... PS3, Line 461: clrsetbits32(dst_addr, 0x7f << 16, value << 16);
That’s the second item of the commit message.
It is intentional. Notice the code below that does the same operation with other values.