Felix Held 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:
(2 comments)
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?
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 #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