Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32070 )
Change subject: [WIP]mb/*: Migrate sandybridge MRC settings to devicetree.cb ......................................................................
Patch Set 11:
(5 comments)
I like the idea. However, I would consider unifying both the native and mrc bootpaths' settings. Ideally, I would want that any sandy/ivy board could use either bootpath without requiring extra code.
https://review.coreboot.org/#/c/32070/11/src/mainboard/google/butterfly/devi... File src/mainboard/google/butterfly/devicetree.cb:
https://review.coreboot.org/#/c/32070/11/src/mainboard/google/butterfly/devi... PS11, Line 23: register "spd_addresses" = "{ 0xA0, 0x00,0xA4,0x00 }" Could this be reused with the native raminit?
https://review.coreboot.org/#/c/32070/11/src/mainboard/google/butterfly/devi... PS11, Line 27: Do we need this many tabs?
https://review.coreboot.org/#/c/32070/11/src/mainboard/google/butterfly/devi... PS11, Line 29: register "usb_port_config" = "{ : /* enabled usb oc pin length */ : { 1, 0, 0x0040 }, /* P0: Right USB 3.0 #1 (no OC) */ : { 1, 0, 0x0040 }, /* P1: Right USB 3.0 #2 (no OC) */ : { 1, 0, 0x0040 }, /* P2: Camera (no OC) */ : { 0, 0, 0x0000 }, /* P3: Empty */ : { 0, 0, 0x0000 }, /* P4: Empty */ : { 0, 0, 0x0000 }, /* P5: Empty */ : { 0, 0, 0x0000 }, /* P6: Empty */ : { 0, 0, 0x0000 }, /* P7: Empty */ : { 0, 4, 0x0000 }, /* P8: Empty */ : { 1, 4, 0x0080 }, /* P9: Left USB 1 (no OC) */ : { 1, 4, 0x0040 }, /* P10: Mini PCIe - WLAN / BT (no OC) */ : { 0, 4, 0x0000 }, /* P11: Empty */ : { 0, 4, 0x0000 }, /* P12: Empty */ : { 0, 4, 0x0000 }, /* P13: Empty */ : }" Couldn't this be generated from the native USB port settings? Two thirds of these values are exactly the same.
https://review.coreboot.org/#/c/32070/11/src/mainboard/intel/dcp847ske/usb.h File src/mainboard/intel/dcp847ske/usb.h:
PS11: This file is an attempt to unify USB configs. While it looks like a hack to me and should be replaced, it is interesting because it defines how to unify the USB settings for both the native and mrc bootpaths
https://review.coreboot.org/#/c/32070/11/src/mainboard/lenovo/x220/devicetre... File src/mainboard/lenovo/x220/devicetree.cb:
https://review.coreboot.org/#/c/32070/11/src/mainboard/lenovo/x220/devicetre... PS11, Line 27: register "usb_port_config" = "{ I'd say some USB port configs are missing here. Plus, this file is shared between the x220 and x1 variants.