Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/22215 )
Change subject: nb/intel/sandybridge/raminit: Add ECC support ......................................................................
Patch Set 18:
(9 comments)
https://review.coreboot.org/c/coreboot/+/22215/16/src/northbridge/intel/sand... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/22215/16/src/northbridge/intel/sand... PS16, Line 21: /*
This was moved to mchbar_regs. […]
Done
https://review.coreboot.org/c/coreboot/+/22215/16/src/northbridge/intel/sand... PS16, Line 3174:
empty line
Done
https://review.coreboot.org/c/coreboot/+/22215/16/src/northbridge/intel/sand... PS16, Line 3175: int channel, slotrank, row; : int rowsize;
declare all in one line?
Done
https://review.coreboot.org/c/coreboot/+/22215/16/src/northbridge/intel/sand... PS16, Line 3179: >>
spaces
Done
https://review.coreboot.org/c/coreboot/+/22215/16/src/northbridge/intel/sand... PS16, Line 3185: 0x0001f006
please replace with JEDEC macros
Done
https://review.coreboot.org/c/coreboot/+/22215/16/src/northbridge/intel/sand... PS16, Line 3187: (MAX((ctrl->tFAW >> 2) + 1, ctrl->tRRD) << 10) : | 1 | (ctrl->tRCD << 16)
maybe precalculate this?
Ack
https://review.coreboot.org/c/coreboot/+/22215/16/src/northbridge/intel/sand... PS16, Line 3190: (slotrank << 24)
This could also be precalculated (to make things fit in one line)
Ack
https://review.coreboot.org/c/coreboot/+/22215/16/src/northbridge/intel/sand... PS16, Line 3206: e
capital E
Ack
https://review.coreboot.org/c/coreboot/+/22215/16/src/northbridge/intel/sand... PS16, Line 3212:
empty line
Done