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 11:
(12 comments)
https://review.coreboot.org/c/coreboot/+/30890/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/30890/1//COMMIT_MSG@12 PS1, Line 12: Support Lynx Point LP (GPIO registers differ from non-LP)
I have some C code here: https://github.com/Th3Fanbus/gpio-scripts/blob/master/hswgpio. […]
Ack
https://review.coreboot.org/c/coreboot/+/30890/10/util/autoport/haswell.go File util/autoport/haswell.go:
https://review.coreboot.org/c/coreboot/+/30890/10/util/autoport/haswell.go@3... PS10, Line 35: "gfx.did": "{ 0x80000100, 0x80000240, 0x80000410, 0x80000410, 0x00000005 }",
if you hardcode this, at least make it consistent with the gfx. […]
Done
https://review.coreboot.org/c/coreboot/+/30890/11/util/autoport/haswell.go File util/autoport/haswell.go:
https://review.coreboot.org/c/coreboot/+/30890/11/util/autoport/haswell.go@1... PS11, Line 109: File: "drivers/intel/gma/acpi/default_brightness_levels.asl", FIXME: This is not necessary if the board doesn't have backlight
https://review.coreboot.org/c/coreboot/+/30890/1/util/autoport/lynxpoint.go File util/autoport/lynxpoint.go:
https://review.coreboot.org/c/coreboot/+/30890/1/util/autoport/lynxpoint.go@... PS1, Line 228: PCISlot{PCIAddr: PCIAddr{Dev: 0x1c, Func: 6}, writeEmpty: true, additionalComment: "PCIe Port #7"}, : PCISlot{PCIAddr: PCIAddr{Dev: 0x1c, Func: 7}, writeEmpty: true, additionalComment: "PCIe Port #8"},
These only exist on some variants. See int max_root_ports(void) in sb/intel/lynxpoint/pcie. […]
We're not handling this for sandybridge
https://review.coreboot.org/c/coreboot/+/30890/1/util/autoport/lynxpoint.go@... PS1, Line 356: 0x0040
I saw in mrc.bin that the position and length are computed to another value, then written to IOBP. […]
These values depend on the trace lengths for USB and are documented somewhere, I think
https://review.coreboot.org/c/coreboot/+/30890/11/util/autoport/lynxpoint.go File util/autoport/lynxpoint.go:
https://review.coreboot.org/c/coreboot/+/30890/11/util/autoport/lynxpoint.go... PS11, Line 188: DSDTDefine{ : Key: "BRIGHTNESS_UP", : Value: "\_SB.PCI0.GFX0.INCB", : }, : DSDTDefine{ : Key: "BRIGHTNESS_DOWN", : Value: "\_SB.PCI0.GFX0.DECB", : }, : DSDTDefine{ : Key: "ACPI_VIDEO_DEVICE", : Value: "\_SB.PCI0.GFX0", : }) Those should be removed as they are unused in most cases
https://review.coreboot.org/c/coreboot/+/30890/11/util/autoport/lynxpoint.go... PS11, Line 240: SATA Controller 1 SATA Controller (AHCI)
https://review.coreboot.org/c/coreboot/+/30890/11/util/autoport/lynxpoint.go... PS11, Line 242: SATA Controller 2 SATA Controller (Legacy)
https://review.coreboot.org/c/coreboot/+/30890/11/util/autoport/lynxpoint.go... PS11, Line 250: PCI-LPC bridge Drop this part, it will result in "LPC bridge PCI-LPC bridge"
https://review.coreboot.org/c/coreboot/+/30890/11/util/autoport/lynxpoint.go... PS11, Line 309: .wdbbar = 0x4000000, : .wdbsize = 0x1000, These should not be needed at all
https://review.coreboot.org/c/coreboot/+/30890/11/util/autoport/lynxpoint.go... PS11, Line 426: : /* Disable USB ports in S3 by default */ : gnvs->s3u0 = 0; : gnvs->s3u1 = 0; : : /* Disable USB ports in S5 by default */ : gnvs->s5u0 = 0; : gnvs->s5u1 = 0; Drop these
https://review.coreboot.org/c/coreboot/+/30890/11/util/autoport/lynxpoint.go... PS11, Line 435: // the lid is open by default. /* The lid is open by default. FIXME: Not on desktops? */