Michael Niewöhner has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35514 )
Change subject: device/pci: fix legacy VGA encoding ......................................................................
device/pci: fix legacy VGA encoding
Enable decoding of 16-bit legacy VGA addresses to prevent resource conflicts with other devices (like SMBus and SATA on X11SS* boards).
Tested on X11SSM-F.
Change-Id: I29e37ac69d68ef8e0f37ef02660a53101f254992 Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/device/cardbus_device.c M src/device/device.c M src/include/device/pci_def.h 3 files changed, 18 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/35514/1
diff --git a/src/device/cardbus_device.c b/src/device/cardbus_device.c index a440746..4cd9f85 100644 --- a/src/device/cardbus_device.c +++ b/src/device/cardbus_device.c @@ -156,6 +156,7 @@ PCI_BRIDGE_CTL_SERR | PCI_BRIDGE_CTL_NO_ISA | PCI_BRIDGE_CTL_VGA | + PCI_BRIDGE_CTL_VGA16 | PCI_BRIDGE_CTL_MASTER_ABORT | PCI_BRIDGE_CTL_BUS_RESET)); /* Error check */ diff --git a/src/device/device.c b/src/device/device.c index 44d1f95..d0806ea 100644 --- a/src/device/device.c +++ b/src/device/device.c @@ -19,6 +19,7 @@ #include <device/device.h> #include <device/pci_def.h> #include <device/pci_ids.h> +#include <device/pci_ops.h> #include <stdlib.h> #include <string.h> #include <smp/spinlock.h> @@ -748,6 +749,7 @@ /* FIXME: Handle the VGA palette snooping. */ struct device *dev, *vga, *vga_onboard; struct bus *bus; + u16 ctrl;
bus = 0; vga = 0; @@ -793,11 +795,21 @@ bus = vga->bus; }
- /* Now walk up the bridges setting the VGA enable. */ + /* Now walk up the bridges setting the VGA enable if supported. */ 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; + /* try to set VGA16; it will return 1 if it is supported */ + ctrl = pci_read_config16(bus->dev, PCI_CB_BRIDGE_CONTROL); + if (!(ctrl & PCI_BRIDGE_CTL_VGA16)) { // no test needed if already set + pci_or_config16(bus->dev, PCI_CB_BRIDGE_CONTROL, PCI_BRIDGE_CTL_VGA16); + ctrl = pci_read_config16(bus->dev, PCI_CB_BRIDGE_CONTROL); + /* restore */ + pci_write_config16(bus->dev, PCI_CB_BRIDGE_CONTROL, ctrl & ~PCI_BRIDGE_CTL_VGA16); + } + if (ctrl & PCI_BRIDGE_CTL_VGA16) { + printk(BIOS_DEBUG, "Setting PCI_BRIDGE_CTL_VGA(16) for bridge %s\n", dev_path(bus->dev)); + bus->bridge_ctrl |= (PCI_BRIDGE_CTL_VGA | PCI_BRIDGE_CTL_VGA16); + } + bus = (bus == bus->dev->bus) ? 0 : bus->dev->bus; } } diff --git a/src/include/device/pci_def.h b/src/include/device/pci_def.h index bc5bc79..660edcb 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 0x16 /* Enable VGA16 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 */
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35514 )
Change subject: device/pci: fix legacy VGA encoding ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35514/1/src/device/device.c File src/device/device.c:
https://review.coreboot.org/c/coreboot/+/35514/1/src/device/device.c@806 PS1, Line 806: pci_write_config16(bus->dev, PCI_CB_BRIDGE_CONTROL, ctrl & ~PCI_BRIDGE_CTL_VGA16); line over 96 characters
https://review.coreboot.org/c/coreboot/+/35514/1/src/device/device.c@809 PS1, Line 809: printk(BIOS_DEBUG, "Setting PCI_BRIDGE_CTL_VGA(16) for bridge %s\n", dev_path(bus->dev)); line over 96 characters
Hello Patrick Rudolph, Angel Pons, Christian Walter, Philipp Deppenwiese, Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35514
to look at the new patch set (#2).
Change subject: device/pci: fix legacy VGA encoding ......................................................................
device/pci: fix legacy VGA encoding
Enable decoding of 16-bit legacy VGA addresses to prevent resource conflicts with other devices (like SMBus and SATA on X11SS* boards).
Tested on X11SSM-F.
Change-Id: I29e37ac69d68ef8e0f37ef02660a53101f254992 Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/device/cardbus_device.c M src/device/device.c M src/include/device/pci_def.h 3 files changed, 20 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/35514/2
Hello Patrick Rudolph, Angel Pons, Christian Walter, Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35514
to look at the new patch set (#3).
Change subject: device/pci: fix legacy VGA encoding ......................................................................
device/pci: fix legacy VGA encoding
Enable decoding of 16-bit legacy VGA addresses to prevent resource conflicts with other devices (like SMBus and SATA on X11SS* boards).
Tested on X11SSM-F.
Change-Id: I29e37ac69d68ef8e0f37ef02660a53101f254992 Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/device/cardbus_device.c M src/device/device.c M src/include/device/pci_def.h 3 files changed, 21 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/35514/3
Hello Patrick Rudolph, Angel Pons, Christian Walter, Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35514
to look at the new patch set (#4).
Change subject: device/pci: fix legacy VGA encoding ......................................................................
device/pci: fix legacy VGA encoding
Enable decoding of 16-bit legacy VGA addresses to prevent resource conflicts with other devices (like SMBus and SATA on X11SS* boards).
Tested on X11SSM-F.
Change-Id: I29e37ac69d68ef8e0f37ef02660a53101f254992 Signed-off-by: Michael Niewöhner foss@mniewoehner.de --- M src/device/cardbus_device.c M src/device/device.c M src/include/device/pci_def.h 3 files changed, 20 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/14/35514/4
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35514 )
Change subject: device/pci: fix legacy VGA encoding ......................................................................
Patch Set 4:
(12 comments)
Some comments went into patchset #2, most in #4.
https://review.coreboot.org/c/coreboot/+/35514/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35514/2//COMMIT_MSG@10 PS2, Line 10: conflicts with other devices (like SMBus and SATA on X11SS* boards). Please elaborate, how/why this solves any resource conflicts. Like, did you see errors in coreboot or kernel logs?
https://review.coreboot.org/c/coreboot/+/35514/4/src/device/cardbus_device.c File src/device/cardbus_device.c:
https://review.coreboot.org/c/coreboot/+/35514/4/src/device/cardbus_device.c... PS4, Line 161: PCI_BRIDGE_CTL_BUS_RESET)); This is CARDBUS context, these should be PCI_CB_.
I did not check specs why there is BRIDGE_CTL_NO_ISA and CB_BRIDGE_CTL_ISA, is the logic really inverted??
https://review.coreboot.org/c/coreboot/+/35514/4/src/device/cardbus_device.c... PS4, Line 163: ctrl |= (PCI_CB_BRIDGE_CTL_PARITY + PCI_CB_BRIDGE_CTL_SERR); I'll prepare commit to replace '+' with '|' here, we have more cases like this.
https://review.coreboot.org/c/coreboot/+/35514/4/src/device/cardbus_device.c... PS4, Line 165: pci_write_config16(dev, PCI_BRIDGE_CONTROL, ctrl); I'll prepare commit to replace this with PCI_CB_, like read_config16() above... (sigh).
https://review.coreboot.org/c/coreboot/+/35514/2/src/device/device.c File src/device/device.c:
https://review.coreboot.org/c/coreboot/+/35514/2/src/device/device.c@744 PS2, Line 744: * FIXME: Modify set_vga_bridge() so it is less PCI-centric! Apparently someone wishes less, not more, PCI register accesses here.
https://review.coreboot.org/c/coreboot/+/35514/2/src/device/device.c@798 PS2, Line 798: /* Now walk up the bridges setting the VGA enable if supported. */ If there is any bridge without CTL_VGA16 support, we probably should not set it at all since transaction will not reach the target.
https://review.coreboot.org/c/coreboot/+/35514/2/src/device/device.c@801 PS2, Line 801: ctrl = pci_read_config16(bus->dev, PCI_CB_BRIDGE_CONTROL); PCI_CB_BRIDGE_CONTROL is only valid for CARDBUS bridges and should not be used here, PCI_BRIDGE_CONTROL would be more correct. Seems to be same 0x3e register though.
https://review.coreboot.org/c/coreboot/+/35514/2/src/device/device.c@812 PS2, Line 812: bus->bridge_ctrl |= (PCI_BRIDGE_CTL_VGA | PCI_BRIDGE_CTL_VGA16); So CTL_VGA is no longer unconditionally set? Is that intentional?
https://review.coreboot.org/c/coreboot/+/35514/4/src/include/device/pci_def.... File src/include/device/pci_def.h:
https://review.coreboot.org/c/coreboot/+/35514/4/src/include/device/pci_def.... PS4, Line 141: #define PCI_BRIDGE_CTL_VGA16 0x16 /* Enable VGA16 decoding */ Huh.. you probably want 0x10 here???
https://review.coreboot.org/c/coreboot/+/35514/4/src/include/device/pci_def.... PS4, Line 170: /* Similar to standard bridge control register */ Similar, but not same? This implies the need to evaluate header type before accessing register 0x3e.
https://review.coreboot.org/c/coreboot/+/35514/4/src/include/device/pci_def.... PS4, Line 173: #define PCI_CB_BRIDGE_CTL_ISA 0x04 CTL_ISA here vs. CTL_NO_ISA above. Check the specs what is correct.
https://review.coreboot.org/c/coreboot/+/35514/4/src/include/device/pci_def.... PS4, Line 174: #define PCI_CB_BRIDGE_CTL_VGA 0x08 CTL_VGA16 should be defined here as well (after checking the specs it is implemented).
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35514 )
Change subject: device/pci: fix legacy VGA encoding ......................................................................
Patch Set 4:
Hi Kyösti,
sorry, I originally proposed to handle it in set_vga_bridge_bit(). Didn't like it either, when I saw it, hence CB:35516. I'll try to find CardBus specs to confirm if they added the bit there, too.
Michael Niewöhner has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/35514 )
Change subject: device/pci: fix legacy VGA encoding ......................................................................
Abandoned
abandoned in favour of CB:35516
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35514 )
Change subject: device/pci: fix legacy VGA encoding ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35514/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35514/2//COMMIT_MSG@10 PS2, Line 10: conflicts with other devices (like SMBus and SATA on X11SS* boards).
Please elaborate, how/why this solves any resource conflicts. […]
yes, smbus broken, AHCI registers overriden. discussion is going on for about a week on irc