Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/20979 )
Change subject: nb/intel/x4x/raminit: Move dummy reads after JEDEC init ......................................................................
Patch Set 8: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/20979/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/20979/8//COMMIT_MSG@14 PS8, Line 14: ready.
Why can’t we remove the dummy reads?
<rant> First things first, there's cases in which registers are read right after writing to them, but the read value is never used. This is because the act of reading a value can have intended and desired side-effects. Cursed sand is cursed!
About JEDEC dummy reads: Intel Memory Reference Code (the thing used in vendor firmwares) for Eaglelake (known here as x4x) does this, as well as any other Intel raminit I've looked at. Feel free to check the JEDEC DDR2 and DDR3 specs and ponder whether these are needed or not. And then you'd have to go bug Intel about it so that they validate raminit with the proposed change...
Ehhh... I have better things to do, so I'd stick to what's known to work. </rant>
TL;DR: A rant about hardware being sorely undocumented, the main reason to stick with the dummy reads: these are known to work, so removing them could cause things to not work later. If it's working, don't break it :)
PS: Please do not take this rant as a personal attack. It's late and I needed to rant about something.
https://review.coreboot.org/c/coreboot/+/20979/8/src/northbridge/intel/x4x/r... File src/northbridge/intel/x4x/raminit_ddr23.c:
https://review.coreboot.org/c/coreboot/+/20979/8/src/northbridge/intel/x4x/r... PS8, Line 2163: (ch << 29) | (r * 0x08000000)
reimplementation of test_address(ch, r). […]
I wonder why this was lost when moving the code. Also, printk()'s were also lost
https://review.coreboot.org/c/coreboot/+/20979/8/src/northbridge/intel/x4x/r... PS8, Line 2164: 0x800000
I wonder where this offset is for
me too