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 9: Code-Review+2
(11 comments)
https://review.coreboot.org/#/c/32069/9/src/northbridge/intel/sandybridge/ch... File src/northbridge/intel/sandybridge/chip.h:
https://review.coreboot.org/#/c/32069/9/src/northbridge/intel/sandybridge/ch... PS9, Line 58: Use 8bit notation where BIT0 is always zero. Or shift by 1 when copying to `pei_data`?
https://review.coreboot.org/#/c/32069/7/src/northbridge/intel/sandybridge/ch... File src/northbridge/intel/sandybridge/chip.h:
PS7:
Haven't tested actual code size. Removed in favour of enums.
Ack
https://review.coreboot.org/#/c/32069/7/src/northbridge/intel/sandybridge/ch... PS7, Line 58: u8 spd_addresses[4];
Same for usb_port_config. Comments will be adjusted once native code makes use of them.
Ack
https://review.coreboot.org/#/c/32069/7/src/northbridge/intel/sandybridge/ch... PS7, Line 68: u16 pcie_init : 2;
Not sure. Removed and drived from PEG device on Ivybridge.
Ack
NB. There are only two boards that set it, and I think the Roda is wrong (might be copy-pasta on my side).
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 […]
Ack
https://review.coreboot.org/#/c/32069/7/src/northbridge/intel/sandybridge/ch... PS7, Line 91: u16 dimm_channel1_disabled : 2;
Yes. Done.
Ack
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));
Done
Ack
https://review.coreboot.org/#/c/32069/7/src/northbridge/intel/sandybridge/ra... PS7, Line 307: = NULL
Done
Ack
https://review.coreboot.org/#/c/32069/7/src/northbridge/intel/sandybridge/ra... PS7, Line 310: if (!dev)
Done
Ack
https://review.coreboot.org/#/c/32069/7/src/northbridge/intel/sandybridge/ra... PS7, Line 329: pei_data->max_ddr3_freq = 0;
Done
Ack
https://review.coreboot.org/#/c/32069/9/src/northbridge/intel/sandybridge/ra... File src/northbridge/intel/sandybridge/raminit_mrc.c:
https://review.coreboot.org/#/c/32069/9/src/northbridge/intel/sandybridge/ra... PS9, Line 385: pei_data.dimm_channel0_disabled =
this might not work on lumpy as the onboard spd config looks fishy. […]
Probably worth to ask around if it uses the second channel.