Attention is currently required from: Angel Pons, Evgeny Zinoviev. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45500 )
Change subject: nb/intel/sandybridge: Improve channel disable logic ......................................................................
Patch Set 5:
(2 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/45500/comment/1e9c3e87_d125e9f4 PS5, Line 9: Do not consider the failed channel's SPDs in emergency mode. Also, force
AIUI, each DIMM would fail to boot without any other DIMMs in the system. […]
It could work by coincidence if one DIMM is broken in a way that it works with the SPD data of the other. It's just a made up example, nothing I would really expect. But the logic of your change dictates that such cases could regress. So I would mention that in the commit message, e.g.
In corner cases with unreliable DIMMs in both channels, this could result in a boot failure, even if the old code succeeded.
File src/northbridge/intel/sandybridge/raminit.c:
https://review.coreboot.org/c/coreboot/+/45500/comment/2dcab14e_249e1fc7 PS5, Line 348: }
How would you handle S3 resume, then?
not (preferably)
The emergency mode seems like a nice feature so you don't end up with a brick. But I don't see a reason to support STR in such a state.
IIRC, the current code allows both without risking a flashing loop. Your change to automatically try the training again seems valid, but you have to choose then: Either drop STR support, or risk a flashing loop.