Attention is currently required from: Máté Kukri, Maxim Polyakov, Tristan Corrick, Arthur Heymans, Iru Cai. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30890 )
Change subject: autoport: Add support for Haswell-LynxPoint platform ......................................................................
Patch Set 32:
(6 comments)
File util/autoport/bd82x6x.go:
https://review.coreboot.org/c/coreboot/+/30890/comment/77bad9af_7fced718 PS32, Line 13: func writeGPIOSet(ctx Context, sb *os.File, Should non-bd82x6x stuff be moved into a separate file?
File util/autoport/lynxpoint.go:
https://review.coreboot.org/c/coreboot/+/30890/comment/f7b63d6b_5749cdc7 PS1, Line 272: RestoreDIRRoute(sb, "D31IR", uint16(inteltool.RCBA[0x3140])) : RestoreDIRRoute(sb, "D29IR", uint16(inteltool.RCBA[0x3144])) : RestoreDIRRoute(sb, "D28IR", uint16(inteltool.RCBA[0x3146])) : RestoreDIRRoute(sb, "D27IR", uint16(inteltool.RCBA[0x3148])) : RestoreDIRRoute(sb, "D26IR", uint16(inteltool.RCBA[0x314c])) : RestoreDIRRoute(sb, "D25IR", uint16(inteltool.RCBA[0x3150])) : RestoreDIRRoute(sb, "D22IR", uint16(inteltool.RCBA[0x315c])) : RestoreDIRRoute(sb, "D20IR", uint16(inteltool.RCBA[0x3160]))
DxxIR and DxxIP are only useful to support legacy interrupt routing. I'm not aware of a major, mainstream OS that uses legacy interrupt routing by default. It would be fine to leave these registers at their reset defaults. As far as I know, legacy interrupt routing should still work in that case, but the interrupts might not be balanced very well.
Lynx Point could benefit from making this config PCH-specific instead of board-specific, as was done in commit 33b535f15ded for earlier PCHs.
I totally agree.
File util/autoport/lynxpoint.go:
https://review.coreboot.org/c/coreboot/+/30890/comment/3570a2ad_08eebfde PS29, Line 524: const struct usb2_port_config mainboard_usb2_ports[MAX_USB2_PORTS] = {
Shouldn't we check if the USB2 ports are actually routed to EHCI or xHCI before reading these values […]
I imagine. It's better than nothing, though
File util/autoport/lynxpoint.go:
https://review.coreboot.org/c/coreboot/+/30890/comment/243de1b2_036cad1a PS32, Line 170: uint16(0x100+g*8)]
this is magic )) What does 0x100 mean? What does g mean?
`g` is the GPIO number, and the resulting value is the offset of the `GPgCONFIGA` register for GPIO `g`
https://review.coreboot.org/c/coreboot/+/30890/comment/7c8195c6_1dcac0ee PS32, Line 199: lvl = "LOW"
I don't think we need any more variables […]
+1
https://review.coreboot.org/c/coreboot/+/30890/comment/b7aedfbf_2dd0148e PS32, Line 536: !isULT I'd flip the if/else branches to avoid the negation in the condition