Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35514 )
Change subject: device/pci: fix legacy VGA encoding ......................................................................
Patch Set 4:
(12 comments)
Some comments went into patchset #2, most in #4.
https://review.coreboot.org/c/coreboot/+/35514/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35514/2//COMMIT_MSG@10 PS2, Line 10: conflicts with other devices (like SMBus and SATA on X11SS* boards). Please elaborate, how/why this solves any resource conflicts. Like, did you see errors in coreboot or kernel logs?
https://review.coreboot.org/c/coreboot/+/35514/4/src/device/cardbus_device.c File src/device/cardbus_device.c:
https://review.coreboot.org/c/coreboot/+/35514/4/src/device/cardbus_device.c... PS4, Line 161: PCI_BRIDGE_CTL_BUS_RESET)); This is CARDBUS context, these should be PCI_CB_.
I did not check specs why there is BRIDGE_CTL_NO_ISA and CB_BRIDGE_CTL_ISA, is the logic really inverted??
https://review.coreboot.org/c/coreboot/+/35514/4/src/device/cardbus_device.c... PS4, Line 163: ctrl |= (PCI_CB_BRIDGE_CTL_PARITY + PCI_CB_BRIDGE_CTL_SERR); I'll prepare commit to replace '+' with '|' here, we have more cases like this.
https://review.coreboot.org/c/coreboot/+/35514/4/src/device/cardbus_device.c... PS4, Line 165: pci_write_config16(dev, PCI_BRIDGE_CONTROL, ctrl); I'll prepare commit to replace this with PCI_CB_, like read_config16() above... (sigh).
https://review.coreboot.org/c/coreboot/+/35514/2/src/device/device.c File src/device/device.c:
https://review.coreboot.org/c/coreboot/+/35514/2/src/device/device.c@744 PS2, Line 744: * FIXME: Modify set_vga_bridge() so it is less PCI-centric! Apparently someone wishes less, not more, PCI register accesses here.
https://review.coreboot.org/c/coreboot/+/35514/2/src/device/device.c@798 PS2, Line 798: /* Now walk up the bridges setting the VGA enable if supported. */ If there is any bridge without CTL_VGA16 support, we probably should not set it at all since transaction will not reach the target.
https://review.coreboot.org/c/coreboot/+/35514/2/src/device/device.c@801 PS2, Line 801: ctrl = pci_read_config16(bus->dev, PCI_CB_BRIDGE_CONTROL); PCI_CB_BRIDGE_CONTROL is only valid for CARDBUS bridges and should not be used here, PCI_BRIDGE_CONTROL would be more correct. Seems to be same 0x3e register though.
https://review.coreboot.org/c/coreboot/+/35514/2/src/device/device.c@812 PS2, Line 812: bus->bridge_ctrl |= (PCI_BRIDGE_CTL_VGA | PCI_BRIDGE_CTL_VGA16); So CTL_VGA is no longer unconditionally set? Is that intentional?
https://review.coreboot.org/c/coreboot/+/35514/4/src/include/device/pci_def.... File src/include/device/pci_def.h:
https://review.coreboot.org/c/coreboot/+/35514/4/src/include/device/pci_def.... PS4, Line 141: #define PCI_BRIDGE_CTL_VGA16 0x16 /* Enable VGA16 decoding */ Huh.. you probably want 0x10 here???
https://review.coreboot.org/c/coreboot/+/35514/4/src/include/device/pci_def.... PS4, Line 170: /* Similar to standard bridge control register */ Similar, but not same? This implies the need to evaluate header type before accessing register 0x3e.
https://review.coreboot.org/c/coreboot/+/35514/4/src/include/device/pci_def.... PS4, Line 173: #define PCI_CB_BRIDGE_CTL_ISA 0x04 CTL_ISA here vs. CTL_NO_ISA above. Check the specs what is correct.
https://review.coreboot.org/c/coreboot/+/35514/4/src/include/device/pci_def.... PS4, Line 174: #define PCI_CB_BRIDGE_CTL_VGA 0x08 CTL_VGA16 should be defined here as well (after checking the specs it is implemented).