Tristan Corrick has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30890 )
Change subject: autoport: Add support for Haswell-LynxPoint platform ......................................................................
Patch Set 1:
(15 comments)
Thanks for doing this!
https://review.coreboot.org/#/c/30890/1/util/autoport/azalia.go File util/autoport/azalia.go:
https://review.coreboot.org/#/c/30890/1/util/autoport/azalia.go@66 PS1, Line 66: RegisterPCI(0x8086, 0x0c0c, azalia{}) Mini-HD audio config is hardcoded in `nb/intel/haswell/minihd.c`. Is there a case where a different config is preferable?
https://review.coreboot.org/#/c/30890/1/util/autoport/haswell.go File util/autoport/haswell.go:
https://review.coreboot.org/#/c/30890/1/util/autoport/haswell.go@92 PS1, Line 92: Non-ULT boards will need to select `TSC_MONOTONIC_TIMER` to boot. They will also need to set `GFX_GMA_CPU_VARIANT` to "Normal" to get libgfxinit working, at least until the dynamic CPU detection patches are merged.
https://review.coreboot.org/#/c/30890/1/util/autoport/lynxpoint.go File util/autoport/lynxpoint.go:
https://review.coreboot.org/#/c/30890/1/util/autoport/lynxpoint.go@2 PS1, Line 2: A lot of this code (especially the GPIO stuff) is the same as bd82x6x. It would be great to see a common implementation shared between the two, instead of duplicated code.
https://review.coreboot.org/#/c/30890/1/util/autoport/lynxpoint.go@161 PS1, Line 161: if dmi.Vendor == "LENOVO" { : KconfigInt["DRAM_RESET_GATE_GPIO"] = 10 : } else { : KconfigInt["DRAM_RESET_GATE_GPIO"] = 60 : } : KconfigComment["DRAM_RESET_GATE_GPIO"] = "FIXME: check this" Lynx Point doesn't currently implement handling of DRAM_RESET_GATE_GPIO.
https://review.coreboot.org/#/c/30890/1/util/autoport/lynxpoint.go@189 PS1, Line 189: : /* SPI init */ : MainboardIncludes = append(MainboardIncludes, "southbridge/intel/lynxpoint/pch.h") This isn't needed, I think.
https://review.coreboot.org/#/c/30890/1/util/autoport/lynxpoint.go@211 PS1, Line 211: "sata_port_map": fmt.Sprintf("0x%x", PCIMap[PCIAddr{Bus: 0, Dev: 0x1f, Func: 2}].ConfigDump[0x92]&0x3f), I think this should go next to the other SATA register.
https://review.coreboot.org/#/c/30890/1/util/autoport/lynxpoint.go@214 PS1, Line 214: USB 3.0 Better to say xHCI controller.
https://review.coreboot.org/#/c/30890/1/util/autoport/lynxpoint.go@231 PS1, Line 231: PCISlot{PCIAddr: PCIAddr{Dev: 0x1e, Func: 0}, writeEmpty: true, additionalComment: "PCI bridge"}, Doesn't exist on Lynx Point.
https://review.coreboot.org/#/c/30890/1/util/autoport/lynxpoint.go@311 PS1, Line 311: FIXME: check this Perhaps mention somewhere that mrc.bin expects the SPD addresses left- shifted by 1.
https://review.coreboot.org/#/c/30890/1/util/autoport/lynxpoint.go@320 PS1, Line 320: pdo1 := PCIMap[PCIAddr{Bus: 0, Dev: 0x1d, Func: 0}].ConfigDump[0x64] : ocmap1 := PCIMap[PCIAddr{Bus: 0, Dev: 0x1d, Func: 0}].ConfigDump[0x74:0x78] : pdo2 := PCIMap[PCIAddr{Bus: 0, Dev: 0x1a, Func: 0}].ConfigDump[0x64] : ocmap2 := PCIMap[PCIAddr{Bus: 0, Dev: 0x1a, Func: 0}].ConfigDump[0x74:0x78] It's possible for the USB 2 ports to be routed to the xHCI controller, and in that case the EHCI controller(s) may be disabled, or the EHCI PDO/OCMAP invalid. I think it would be best to check the register XUSB2PR of the xHCI device to see which ports are routed where, and use that to decide, for each port, which device's OCMAP and PDO to use.
Also, LP only has a single EHCI controller: 0x1d.
https://review.coreboot.org/#/c/30890/1/util/autoport/lynxpoint.go@356 PS1, Line 356: 0x0040 Sometimes the USB port might need a greater value here, if the trace length is long enough. I experienced an error on my X10SLM+-F with 0x40 set, causing FreeBSD to crash while running rsync. It did take multiple terabytes of transfer to manifest. I haven't experienced it again after setting it to 0x110, so I'm reasonably confident that this setting does have an effect.
Documentation about this setting was actually removed a while ago, with commit b1c25e74af0a. Hopefully the description of the setting there is (still) accurate.
https://review.coreboot.org/#/c/30890/1/util/autoport/lynxpoint.go@392 PS1, Line 392: &rcba_config[0] Just `rcba_config` should be fine.
https://review.coreboot.org/#/c/30890/1/util/autoport/lynxpoint.go@461 PS1, Line 461: RegisterPCI(0x8086, 0x1c22, GenericPCI{MissingParent: "smbus"}) : RegisterPCI(0x8086, 0x1e22, GenericPCI{MissingParent: "smbus"}) These are for Cougar/Panther Point, not Lynx Point.
https://review.coreboot.org/#/c/30890/1/util/autoport/lynxpoint.go@488 PS1, Line 488: 0x8c3a, 0x8c3b, Also 0x8c3c, 0x8c3d, 0x9c3a, 0x9c3b, 0x9c3c, 0x9c3d.
https://review.coreboot.org/#/c/30890/1/util/autoport/lynxpoint.go@494 PS1, Line 494: RegisterPCI(0x8086, 0x8c33, GenericPCI{}) Some boards may have the thermal device 1f.6 (0x8c24) enabled.
LP boards can have a few more:
LAN: 0x155a (but it can vary depending on an EEPROM), SDIO: 0x9c35, "Intel(R) Smart Sound Technology": 0x9c36, Serial I/O: 0x9c6{0..6}.