Angel Pons 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:
(8 comments)
Oops, looks like I derped out on a few things. Will fix.
https://review.coreboot.org/c/coreboot/+/39599/2/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/mchbar_regs.h:
https://review.coreboot.org/c/coreboot/+/39599/2/src/northbridge/intel/sandy... PS2, Line 16:
So you came up with everything by yourself?
Uh, that wasn't intended to be personal, but it may have sounded a bit like it was. Sorry about that.
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
This change was to reduce the indentation of the write32 statement so that it fits on a single line. I'll try dropping in a variable (that should get optimized away into the same binary).
https://review.coreboot.org/c/coreboot/+/39599/2/src/northbridge/intel/sandy... PS2, Line 2881:
see my comment below
same
https://review.coreboot.org/c/coreboot/+/39599/2/src/northbridge/intel/sandy... PS2, Line 3096:
at least in gerrit these additional spaces look odd. […]
Well, these were before I had the idea of renaming the macros so that they are the same length. And well, I missed these. Oopsie.
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. […]
I'll find something to write about in a comment
https://review.coreboot.org/c/coreboot/+/39599/2/src/northbridge/intel/sandy... PS2, Line 3309: =
i'd leave the = on the line above
Ack. This was to have the statements naturally aligned. I'll use use a space.
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?
I think it will make Jenkins complain, though.
This actually looks more like a switch-case, so it might be worth switching to that 😄
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
Erm, I copypasta'd the comment from guess where. I'll fix that.