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:
(3 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.
It's often reserved, read zero (including PC Card 8.0). It's in PCI-to-PCI […]
Oh.. the documentation state is just perfect :/
It could be that even when implemented for PCIe root ports, CTL_VGA bits for those bridges have no effect when IGD is enabled. We should also make sure only one PCIe root port will have CTL_VGA set.
src/soc/intel/common/block/pcie/pcie.c: pci_update_config16(dev, PCI_BRIDGE_CONTROL, ~1, 1<<2);
Unconditionally setting 'ISA enable' bit is also.. interesting.
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@814 PS1, Line 814: /* If the upstream bridge doesn't support VGA, we don't have to check */
See comments on commit message. […]
Ack
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);
Ooff, I assumed the other case, that hardware might mind if we change VGA16 while VGA is off...
You assumed changing VGA16 while VGA is *on* could be bad? I think my approach would have been to try set them both at same time, and unconditionally reset them both before exit. Let's just keep your approach and revisit if it does not work for someone.