Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35516 )
Change subject: device/pci: Ensure full 16-bit VGA port i/o decoding ......................................................................
Patch Set 1:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35516/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35516/1//COMMIT_MSG@17 PS1, Line 17: these days, so nobody noticed. Looks like this bit is only defined when upstream is PCIe and downstream is PCI/PCI-X/AGP(?), given that the definition of CTL_VGA and CTL_VGA_16 is found only in PCIe-to-PCI(-X) bridge specs, paragraph 5.1.2.13.
VGA IO decodes are not documented for PCI Express base specification rev 2.1 and 3.0 section 7.5.3.6. I believe that one would cover PCIe slots?
https://review.coreboot.org/c/coreboot/+/35516/1/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/35516/1/src/device/pci_device.c@807 PS1, Line 807: bridge_ctrl = pci_read_config16(bus->dev, PCI_BRIDGE_CONTROL); It's a bit ugly to do this with cardbus configuration header, which should use PCI_CB_BRIDGE_CONTROL instead. Since cardbus is PCI, I believe CTL_VGA and CTL_VGA16 are defined so maybe this is fine.
https://review.coreboot.org/c/coreboot/+/35516/1/src/device/pci_device.c@814 PS1, Line 814: /* If the upstream bridge doesn't support VGA, we don't have to check */ See comments on commit message. I am not sure if CTL_VGA and CTL_VGA16 bits are no longer defined for PCIe root ports and bridges.
https://review.coreboot.org/c/coreboot/+/35516/1/src/device/pci_device.c@822 PS1, Line 822: bridge_ctrl = pci_read_config16(bus->dev, PCI_BRIDGE_CONTROL); Write with CTL_VGA=0,CTL_VGA16=1 is strange/unexpected. Some hardware could respond with CTL_VGA16=0 here, even if CTL_VGA=1,CTL_VGA16 would be supported.
https://review.coreboot.org/c/coreboot/+/35516/1/src/include/device/pci_def.... File src/include/device/pci_def.h:
https://review.coreboot.org/c/coreboot/+/35516/1/src/include/device/pci_def.... PS1, Line 174: #define PCI_CB_BRIDGE_CTL_VGA 0x08 Not sure what 0x10 is with cardbus. You can see 0x80 below does not match with PCI_BRIDGE_CONTROL register definitions.