device/pci: Enable full 16-bit VGA port i/o decoding

So, the PCI to PCI bridge specification had a pitfall for us:
Originally, when decoding i/o ports for legacy VGA cycles, bridges
should only consider the 10 least significant bits of the port address.
This means all VGA registers were aliased every 1024 ports!
    e.g. 0x3b0 was also decoded as 0x7b0, 0xbb0 etc.

However, it seems, we never reserved the aliased ports, resulting in
silent conflicts we preallocated resources. We neither use much
external VGA nor many i/o ports these days, so nobody noticed.

To avoid this mess, a bridge control bit (VGA16) was introduced in
2003 to enable decoding of 16-bit port addresses. As older systems
seem rather safe and well tested, and newer systems should support
this bit, we'll use it if possible and only warn if not.

With old (AGP era) hardware one will likely encounter a warning like
this:

    found VGA at PCI: 06:00.0
    A bridge on the path doesn't support 16-bit VGA decoding!

This is not generally fatal, but makes unnoticed resource conflicts
more likely.

Change-Id: Id7a07f069dd54331df79f605c6bcda37882a602d
Signed-off-by: Nico Huber <nico.h@gmx.de>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/35516
Reviewed-by: Kyösti Mälkki <kyosti.malkki@gmail.com>
Reviewed-by: Michael Niewöhner
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>

4 files changed, 46 insertions, 1 deletions

diff --git a/src/device/device.c b/src/device/device.c
index 44d1f95f75..333f1f0f1d 100644
--- a/src/device/device.c
+++ b/src/device/device.c
@@ -759,6 +759,10 @@ static void set_vga_bridge_bits(void)
 			continue;
 
 		printk(BIOS_DEBUG, "found VGA at %s\n", dev_path(dev));
+		if (dev->bus->no_vga16) {
+			printk(BIOS_WARNING,
+				"A bridge on the path doesn't support 16-bit VGA decoding!");
+		}
 
 		if (dev->on_mainboard) {
 			vga_onboard = dev;
@@ -797,7 +801,7 @@ static void set_vga_bridge_bits(void)
 	while (bus) {
 		printk(BIOS_DEBUG, "Setting PCI_BRIDGE_CTL_VGA for bridge %s\n",
 		       dev_path(bus->dev));
-		bus->bridge_ctrl |= PCI_BRIDGE_CTL_VGA;
+		bus->bridge_ctrl |= PCI_BRIDGE_CTL_VGA | PCI_BRIDGE_CTL_VGA16;
 		bus = (bus == bus->dev->bus) ? 0 : bus->dev->bus;
 	}
 }
diff --git a/src/device/pci_device.c b/src/device/pci_device.c
index 0a4b69bfb2..191c8460c8 100644
--- a/src/device/pci_device.c
+++ b/src/device/pci_device.c
@@ -793,6 +793,43 @@ struct device_operations default_pci_ops_bus = {
 };
 
 /**
+ * Check for compatibility to route legacy VGA cycles through a bridge.
+ *
+ * Originally, when decoding i/o ports for legacy VGA cycles, bridges
+ * should only consider the 10 least significant bits of the port address.
+ * This means all VGA registers were aliased every 1024 ports!
+ *     e.g. 0x3b0 was also decoded as 0x7b0, 0xbb0 etc.
+ *
+ * To avoid this mess, a bridge control bit (VGA16) was introduced in
+ * 2003 to enable decoding of 16-bit port addresses. As we don't want
+ * to make this any more complex for now, we use this bit if possible
+ * and only warn if it's not supported (in set_vga_bridge_bits()).
+ */
+static void pci_bridge_vga_compat(struct bus *const bus)
+{
+	uint16_t bridge_ctrl;
+
+	bridge_ctrl = pci_read_config16(bus->dev, PCI_BRIDGE_CONTROL);
+
+	/* Ensure VGA decoding is disabled during probing (it should
+	   be by default, but we run blobs nowadays) */
+	bridge_ctrl &= ~PCI_BRIDGE_CTL_VGA;
+	pci_write_config16(bus->dev, PCI_BRIDGE_CONTROL, bridge_ctrl);
+
+	/* If the upstream bridge doesn't support VGA16, we don't have to check */
+	bus->no_vga16 |= bus->dev->bus->no_vga16;
+	if (bus->no_vga16)
+		return;
+
+	/* Test if we can enable 16-bit decoding */
+	bridge_ctrl |= PCI_BRIDGE_CTL_VGA16;
+	pci_write_config16(bus->dev, PCI_BRIDGE_CONTROL, bridge_ctrl);
+	bridge_ctrl = pci_read_config16(bus->dev, PCI_BRIDGE_CONTROL);
+
+	bus->no_vga16 = !(bridge_ctrl & PCI_BRIDGE_CTL_VGA16);
+}
+
+/**
  * Detect the type of downstream bridge.
  *
  * This function is a heuristic to detect which type of bus is downstream
@@ -1293,6 +1330,8 @@ void do_pci_scan_bridge(struct device *dev,
 
 	bus = dev->link_list;
 
+	pci_bridge_vga_compat(bus);
+
 	pci_bridge_route(bus, PCI_ROUTE_SCAN);
 
 	do_scan_bus(bus, 0x00, 0xff);
diff --git a/src/include/device/device.h b/src/include/device/device.h
index cb37c096e4..991bd38f2f 100644
--- a/src/include/device/device.h
+++ b/src/include/device/device.h
@@ -94,6 +94,7 @@ struct bus {
 	unsigned int	reset_needed : 1;
 	unsigned int	disable_relaxed_ordering : 1;
 	unsigned int	ht_link_up : 1;
+	unsigned int	no_vga16 : 1;	/* No support for 16-bit VGA decoding */
 };
 
 /*
diff --git a/src/include/device/pci_def.h b/src/include/device/pci_def.h
index bc5bc79e28..c8b86d5b44 100644
--- a/src/include/device/pci_def.h
+++ b/src/include/device/pci_def.h
@@ -138,6 +138,7 @@
 #define  PCI_BRIDGE_CTL_SERR	0x02	/* The same for SERR forwarding */
 #define  PCI_BRIDGE_CTL_NO_ISA	0x04	/* Disable bridging of ISA ports */
 #define  PCI_BRIDGE_CTL_VGA	0x08	/* Forward VGA addresses */
+#define  PCI_BRIDGE_CTL_VGA16	0x10	/* Enable 16-bit i/o port decoding */
 #define  PCI_BRIDGE_CTL_MASTER_ABORT 0x20  /* Report master aborts */
 #define  PCI_BRIDGE_CTL_BUS_RESET 0x40	/* Secondary bus reset */
 /* Fast Back2Back enabled on secondary interface */