Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39599 )
Change subject: nb/intel/sandybridge: Tidy up code and comments ......................................................................
Patch Set 2: Code-Review+1
(7 comments)
since this patch doesn't change a timeless build, i'm ok with the patch cleaning up more than just one thing
https://review.coreboot.org/c/coreboot/+/39599/2/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/39599/2/src/northbridge/intel/sandy... PS2, Line 2280: return; i find the control flow structure in the old version clearer
https://review.coreboot.org/c/coreboot/+/39599/2/src/northbridge/intel/sandy... PS2, Line 2881: see my comment below
https://review.coreboot.org/c/coreboot/+/39599/2/src/northbridge/intel/sandy... PS2, Line 3096: at least in gerrit these additional spaces look odd. same below
https://review.coreboot.org/c/coreboot/+/39599/2/src/northbridge/intel/sandy... PS2, Line 3193: b4_8_12 = (ctrl->pi_coding_threshold < max_pi - min_pi) ? 0x3330 : 0x2220; found the old version to be a bit more obvious. feel free to ignore this comment though
https://review.coreboot.org/c/coreboot/+/39599/2/src/northbridge/intel/sandy... PS2, Line 3309: = i'd leave the = on the line above
https://review.coreboot.org/c/coreboot/+/39599/2/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_ivy.c:
https://review.coreboot.org/c/coreboot/+/39599/2/src/northbridge/intel/sandy... PS2, Line 389: } maybe put the } on the newline above?
https://review.coreboot.org/c/coreboot/+/39599/2/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_sandy.c:
https://review.coreboot.org/c/coreboot/+/39599/2/src/northbridge/intel/sandy... PS2, Line 188: 1400MHz (DDR3 2800) is this true for snb? iirc that's for ivb