Nico Huber 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)
The code in device.c seems to attempt to avoid VGA IO and "legacy" PCI IO windows when allocating (dynamic) resources. I am not convinced that code always works, and those windows would appear to be unnecessary for platforms where we can have programming of CTL_VGA=1, CTL_VGA16=1 and CTL_(NO)_ISA=0 for PCI_BRIDGE_CONTROL.
Sounds plausible, more plausible than the comments in the existing code :)
I think we could first throw errors if assigned resources overlap some of those aliased (and enabled) IO address ranges. Should be easy to (manually) adjust the conflicting static resources when they appear.
How would you implement that? It would be nice to just add the resources when deciding to set CTL_VGA, but I assume then we'd have to exclude them from being included in the bridge's regular i/o / memory base regions?
https://review.coreboot.org/c/coreboot/+/35516/1/src/device/device.c File src/device/device.c:
https://review.coreboot.org/c/coreboot/+/35516/1/src/device/device.c@403 PS1, Line 403: The legacy PCI decodes : * only 10 bits, uses 0x100 - 0x3ff. This comment suggests that there is more lurking than the ISA thing. I could only find specs back to PCI 2.3 which already says 32 bits must be decoded. But maybe an earlier spec really allowed to decode only 10 bits? I mean regular PCI devices, not bridges.
OTOH, taking the comment literally would imply that we couldn't assign any resources to these... waaaaaaaaaa
https://review.coreboot.org/c/coreboot/+/35516/1/src/device/device.c@417 PS1, Line 417: }
So.. we are shifting the start of resource upwards here.. […]
The `else if` branch is dead. For `(base >= 0x3b0) && (base <= 0x3df)`, `(base & 0x300) != 0`!
also, TIL so much... PCI says i/o BARs have to be <= 256 ports
https://review.coreboot.org/c/coreboot/+/35516/1/src/device/device.c@794 PS1, Line 794: /* VGA is first add-on card or the only onboard VGA. */
I thought it would be the last add-on dev_find_class() call returns? Just assuming here it would be […]
The code here is full of weird assumptions, the comment only adds to it...
* .on_mainboard means integrated? wrong if a discrete VGA chip is mentioned in the dt. * .on_mainboard appears only once? ... * integrated VGA is already disabled if not mentioned in the dt (.on_mainboard == 0)? implementation defined