Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32069 )
Change subject: nb/intel/sandybridge: Migrate MRC settings to devicetree ......................................................................
Patch Set 7:
(10 comments)
https://review.coreboot.org/#/c/32069/7/src/northbridge/intel/sandybridge/ch... File src/northbridge/intel/sandybridge/chip.h:
PS7: When I see bit fields that are not part of an ABI or hardware interface, I always wonder if it doesn't increase code size more than what is saved by packing them. What do we gain here?
https://review.coreboot.org/#/c/32069/7/src/northbridge/intel/sandybridge/ch... PS7, Line 58: u8 spd_addresses[4]; These are not MRC specific, we could use the values for all DIMM configs.
https://review.coreboot.org/#/c/32069/7/src/northbridge/intel/sandybridge/ch... PS7, Line 68: u16 pcie_init : 2; Why 2 bits?
Can we derive it from PEG device on/off?
https://review.coreboot.org/#/c/32069/7/src/northbridge/intel/sandybridge/ch... PS7, Line 69: /* N mode functionality. Leave this setting at 0. Doesn't sound like we'd ever need it in a devicetree.
https://review.coreboot.org/#/c/32069/7/src/northbridge/intel/sandybridge/ch... PS7, Line 72: * 2 2N I prefer enums over such comments. Makes the devicetrees more readable without copying the comments all over. Doesn't even need a separate declaration:
enum { NMODE_AUTO, NMODE_1N, NMODE_2N, } nmode;
https://review.coreboot.org/#/c/32069/7/src/northbridge/intel/sandybridge/ch... PS7, Line 91: u16 dimm_channel1_disabled : 2; Can we derive this from `spd_addresses`?
https://review.coreboot.org/#/c/32069/7/src/northbridge/intel/sandybridge/ra... File src/northbridge/intel/sandybridge/raminit_mrc.c:
https://review.coreboot.org/#/c/32069/7/src/northbridge/intel/sandybridge/ra... PS7, Line 294: const struct device *dev = pcidev_path_on_root(PCI_DEVFN(0, 0x19)); that would be `pcidev_on_root(0, 0x19)`
https://review.coreboot.org/#/c/32069/7/src/northbridge/intel/sandybridge/ra... PS7, Line 307: = NULL initialization is overwritten
https://review.coreboot.org/#/c/32069/7/src/northbridge/intel/sandybridge/ra... PS7, Line 310: if (!dev) check `!dev->chip_info` too?
https://review.coreboot.org/#/c/32069/7/src/northbridge/intel/sandybridge/ra... PS7, Line 329: pei_data->max_ddr3_freq = 0; Fall back to 800, with a warning? 0 doesn't seem valid.