Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38359 )
Change subject: nb/intel/nehalem: Try to clean up code ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38359/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38359/6//COMMIT_MSG@9 PS6, Line 9: raminit.c That file makes gerrit slow down to a halt on my laptop. There was an open comment there:
i dislike the semantics of this macro; imho it would be much clearer to define it as #define FOR_EACH_LANE(lanes) FOR_RANGE_UP(lane, (lanes)) and use FOR_EACH_LANE(LANES) or FOR_EACH_LANE(LANES_ECC) in the code. or have a FOR_EACH_LANE and a separate FOR_EACH_LANE_ECC. At least for me the FOR_EACH_LANE(0) or FOR_EACH_LANE(1) isn't very obvious without looking at the macro definition
An option is to make 0 and 1 carry their meaning: #define EXCEPT_ECC 0 #define INCLUDING_ECC 1
That way, the resulting macros read rather naturally. If that sounds too cursed, the names can be applied to the FOR_EACH_LANE(x) macros instead.
https://review.coreboot.org/c/coreboot/+/38359/6/src/northbridge/intel/nehal... File src/northbridge/intel/nehalem/raminit.c:
https://review.coreboot.org/c/coreboot/+/38359/6/src/northbridge/intel/nehal... PS6, Line 3879: /* Don't enable S4-assertion stretch. Makes trouble on roda/rk9. : reg8 = pci_read_config8(PCI_DEV(0, 0x1f, 0), 0xa4); : pci_write_config8(PCI_DEV(0, 0x1f, 0), 0xa4, reg8 | 0x08); : */
is it intentional that this comment gets dropped?
Yes. roda/rk9 is not a nehalem platform, the comment was copied off some other platform.
https://review.coreboot.org/c/coreboot/+/38359/6/src/northbridge/intel/nehal... PS6, Line 71: #define FOR_EACH_LANE(ecc) FOR_RANGE_UP(lane, LANES + (ecc))
i dislike the semantics of this macro; imho it would be much clearer to define it as […]
Agreed, opened a comment in the commit message instead. It's too laggy in here.