Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/25664 )
Change subject: nb/intel/sandybridge: support more XMP timings ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/#/c/25664/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/25664/1//COMMIT_MSG@8 PS1, Line 8: How did you test it ? Why introduce the change ? Does it improve performance or stability ?
https://review.coreboot.org/#/c/25664/1/src/northbridge/intel/sandybridge/ra... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/#/c/25664/1/src/northbridge/intel/sandybridge/ra... PS1, Line 2362: cmdrate = MIN(DIV_ROUND_UP(ctrl->tCMD, ctrl->tCK) - 1, 1); As long as you can't prove that it'll work on any board (see comment above): No.
I would accept cmdrate = MAX(cmdrage, DIV_ROUND_UP(ctrl->tCMD, ctrl->tCK));
Also please make sure that DIV_ROUND_UP(ctrl->tCMD, ctrl->tCK) never exceeds 2.
https://review.coreboot.org/#/c/25664/1/src/northbridge/intel/sandybridge/ra... File src/northbridge/intel/sandybridge/raminit_ivy.c:
https://review.coreboot.org/#/c/25664/1/src/northbridge/intel/sandybridge/ra... PS1, Line 485: ctrl->CWL = get_CWL(ctrl->tCK); why not MAX(get_CWL(ctrl->tCK), DIV_ROUND_UP(ctrl->tCWL, ctrl->tCK)) ?