Attention is currently required from: Felix Singer, Nico Huber, Arthur Heymans, Patrick Rudolph. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49287 )
Change subject: nb/intel/sandybridge: Fix programming command timings ......................................................................
Patch Set 1: Code-Review-2
(3 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/49287/comment/27ecc548_56dff78f PS1, Line 11:
That's basically an empty commit message. If you want to avoid documenting […]
Yeah, I wrote it while trying not to fall asleep. Now that I know what needs to be fixed and how, I'd rather re-do the whole patch train.
File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/49287/comment/bd86596f_4fa83593 PS1, Line 943: FOR_ALL_POPULATED_RANKS {
Please consider making these macros readable before any further reviews.
I don't like them myself. I'm just dragging them around because I don't know how to make them better other than outright removing them, and I don't want to deal with a change that always needs to be rebased manually.
https://review.coreboot.org/c/coreboot/+/49287/comment/799a8e0a_95c486c4 PS1, Line 939: /* : * Compute command phase shift as the most negative CCC setting : * across all ranks. Use zero if none of the values is negative. : */ : FOR_ALL_POPULATED_RANKS { : cmd_delay = MAX(cmd_delay, -ctrl->timings[channel][slotrank].pi_coding); : } :
This part looks like a mere refactoring. […]
The `if (cmd_delay == 0) {` check is nonsense, and clock needs to be offset by `ctrl->pi_code_offset`. I agree there's some refactoring that could have been split out, though.
-
Angel Pons (Code Review)