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?

View Change

3 comments:

• File src/device/device.c:

  • Patch Set #1, 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

  • Patch Set #1, 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

  • Patch Set #1, 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

To view, visit change 35516. To unsubscribe, or for help writing mail filters, visit settings.