Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43775 )
Change subject: src: Never set ISA Enable on PCI bridges ......................................................................
src: Never set ISA Enable on PCI bridges
Looks like no one really knows what this bit would be useful for, nor when it would need to be set. Especially if coreboot is setting it even on PCI *Express* bridges. Digging through git history, nearly all instances of setting it on PCIe bridges comes from i82801gx, for which no reason was given as to why this would be needed. The other instances in Intel code seem to have been, unsurprisingly, copy-pasted.
Drop all uses of this definition and rename it to avoid confusion. The negation in the name could trick people into setting this bit again.
Tested on Asrock B85M Pro4, no visible difference.
Change-Id: Ifaff29561769c111fb7897e95dbea842faec5df4 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/device/cardbus_device.c M src/include/device/pci_def.h M src/soc/intel/broadwell/pcie.c M src/soc/intel/common/block/pcie/pcie.c M src/southbridge/intel/bd82x6x/pcie.c M src/southbridge/intel/i82801gx/pcie.c M src/southbridge/intel/i82801ix/pcie.c M src/southbridge/intel/i82801jx/pcie.c M src/southbridge/intel/lynxpoint/pcie.c 9 files changed, 6 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/43775/1
diff --git a/src/device/cardbus_device.c b/src/device/cardbus_device.c index 266e194..b014946 100644 --- a/src/device/cardbus_device.c +++ b/src/device/cardbus_device.c @@ -119,7 +119,6 @@
ctrl = pci_read_config16(dev, PCI_CB_BRIDGE_CONTROL); ctrl |= (dev->link_list->bridge_ctrl & ( - PCI_BRIDGE_CTL_NO_ISA | PCI_BRIDGE_CTL_VGA | PCI_BRIDGE_CTL_MASTER_ABORT | PCI_BRIDGE_CTL_BUS_RESET)); diff --git a/src/include/device/pci_def.h b/src/include/device/pci_def.h index 07ba4a2..25372bf 100644 --- a/src/include/device/pci_def.h +++ b/src/include/device/pci_def.h @@ -136,7 +136,7 @@ /* Enable parity detection on secondary interface */ #define PCI_BRIDGE_CTL_PARITY 0x01 #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_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 */ diff --git a/src/soc/intel/broadwell/pcie.c b/src/soc/intel/broadwell/pcie.c index 14dcd3f..00a8595 100644 --- a/src/soc/intel/broadwell/pcie.c +++ b/src/soc/intel/broadwell/pcie.c @@ -587,7 +587,6 @@
reg16 = pci_read_config16(dev, PCI_BRIDGE_CONTROL); reg16 &= ~PCI_BRIDGE_CTL_PARITY; - reg16 |= PCI_BRIDGE_CTL_NO_ISA; pci_write_config16(dev, PCI_BRIDGE_CONTROL, reg16);
/* Clear errors in status registers */ diff --git a/src/soc/intel/common/block/pcie/pcie.c b/src/soc/intel/common/block/pcie/pcie.c index f36366a..ada380e 100644 --- a/src/soc/intel/common/block/pcie/pcie.c +++ b/src/soc/intel/common/block/pcie/pcie.c @@ -24,9 +24,8 @@ /* Set Cache Line Size to 0x10 */ pci_write_config8(dev, PCI_CACHE_LINE_SIZE, CACHE_LINE_SIZE);
- /* disable parity error response, enable ISA */ - pci_update_config16(dev, PCI_BRIDGE_CONTROL, - ~PCI_BRIDGE_CTL_PARITY, PCI_BRIDGE_CTL_NO_ISA); + /* disable parity error response */ + pci_and_config16(dev, PCI_BRIDGE_CONTROL, ~PCI_BRIDGE_CTL_PARITY);
if (CONFIG(PCIE_DEBUG_INFO)) { printk(BIOS_SPEW, " MBL = 0x%08x\n", diff --git a/src/southbridge/intel/bd82x6x/pcie.c b/src/southbridge/intel/bd82x6x/pcie.c index 2eff162..f6bffbb 100644 --- a/src/southbridge/intel/bd82x6x/pcie.c +++ b/src/southbridge/intel/bd82x6x/pcie.c @@ -216,8 +216,7 @@ // This has no effect but the OS might expect it pci_write_config8(dev, 0x0c, 0x10);
- pci_update_config16(dev, PCI_BRIDGE_CONTROL, - ~PCI_BRIDGE_CTL_PARITY, PCI_BRIDGE_CTL_NO_ISA); + pci_and_config16(dev, PCI_BRIDGE_CONTROL, ~PCI_BRIDGE_CTL_PARITY);
/* Clear errors in status registers. FIXME: Do something? */ reg16 = pci_read_config16(dev, 0x06); diff --git a/src/southbridge/intel/i82801gx/pcie.c b/src/southbridge/intel/i82801gx/pcie.c index 6c0ca5d..ca0ae2e 100644 --- a/src/southbridge/intel/i82801gx/pcie.c +++ b/src/southbridge/intel/i82801gx/pcie.c @@ -54,7 +54,6 @@
reg16 = pci_read_config16(dev, PCI_BRIDGE_CONTROL); reg16 &= ~PCI_BRIDGE_CTL_PARITY; - reg16 |= PCI_BRIDGE_CTL_NO_ISA; pci_write_config16(dev, PCI_BRIDGE_CONTROL, reg16);
/* Enable IO xAPIC on this PCIe port */ diff --git a/src/southbridge/intel/i82801ix/pcie.c b/src/southbridge/intel/i82801ix/pcie.c index a8e7b11..3900e92 100644 --- a/src/southbridge/intel/i82801ix/pcie.c +++ b/src/southbridge/intel/i82801ix/pcie.c @@ -23,8 +23,7 @@ // This has no effect but the OS might expect it pci_write_config8(dev, 0x0c, 0x10);
- pci_update_config16(dev, PCI_BRIDGE_CONTROL, ~PCI_BRIDGE_CTL_PARITY, - PCI_BRIDGE_CTL_NO_ISA); + pci_and_config16(dev, PCI_BRIDGE_CONTROL, ~PCI_BRIDGE_CTL_PARITY);
/* Enable IO xAPIC on this PCIe port */ pci_or_config32(dev, 0xd8, 1 << 7); diff --git a/src/southbridge/intel/i82801jx/pcie.c b/src/southbridge/intel/i82801jx/pcie.c index 5195522..18d2c72 100644 --- a/src/southbridge/intel/i82801jx/pcie.c +++ b/src/southbridge/intel/i82801jx/pcie.c @@ -23,8 +23,7 @@ // This has no effect but the OS might expect it pci_write_config8(dev, 0x0c, 0x10);
- pci_update_config16(dev, PCI_BRIDGE_CONTROL, ~PCI_BRIDGE_CTL_PARITY, - PCI_BRIDGE_CTL_NO_ISA); + pci_and_config16(dev, PCI_BRIDGE_CONTROL, ~PCI_BRIDGE_CTL_PARITY);
/* Enable IO xAPIC on this PCIe port */ pci_or_config32(dev, 0xd8, 1 << 7); diff --git a/src/southbridge/intel/lynxpoint/pcie.c b/src/southbridge/intel/lynxpoint/pcie.c index 883dfc7..d2950e7 100644 --- a/src/southbridge/intel/lynxpoint/pcie.c +++ b/src/southbridge/intel/lynxpoint/pcie.c @@ -667,7 +667,6 @@
reg16 = pci_read_config16(dev, PCI_BRIDGE_CONTROL); reg16 &= ~PCI_BRIDGE_CTL_PARITY; - reg16 |= PCI_BRIDGE_CTL_NO_ISA; pci_write_config16(dev, PCI_BRIDGE_CONTROL, reg16);
/* Clear errors in status registers */
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43775 )
Change subject: src: Never set ISA Enable on PCI bridges ......................................................................
Patch Set 1:
There is some more: AIUI, this bit relates to the avoidance of 0x100..0x3ff ranges in the v3 allocator. Which was dropped in v4. I assume if there were >256 ports resources below a bridge, the v4 allocator would allocate in a range that is later broken by setting this bit.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43775 )
Change subject: src: Never set ISA Enable on PCI bridges ......................................................................
Patch Set 1:
Patch Set 1:
There is some more: AIUI, this bit relates to the avoidance of 0x100..0x3ff ranges in the v3 allocator. Which was dropped in v4. I assume if there were >256 ports resources below a bridge, the v4 allocator would allocate in a range that is later broken by setting this bit.
Should we see if it breaks and react if it does? Basically, let this go and see what happens.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43775 )
Change subject: src: Never set ISA Enable on PCI bridges ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
There is some more: AIUI, this bit relates to the avoidance of 0x100..0x3ff ranges in the v3 allocator. Which was dropped in v4. I assume if there were >256 ports resources below a bridge, the v4 allocator would allocate in a range that is later broken by setting this bit.
Should we see if it breaks and react if it does? Basically, let this go and see what happens.
I can try plugging in a few cards into the Asrock B85M Pro4 and see if it survives.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43775 )
Change subject: src: Never set ISA Enable on PCI bridges ......................................................................
Patch Set 1: Code-Review+2
This might even conflict with bridge I/O resources assigned by the PCI allocator.
The best source for ISA I could find is the book "EISA System Architecture" by Tom Shanley. See chapter "ISA I/O Address Space Problem". Setting the bit makes sure that the address pins A[9:8] on the ISA bus can never be > 0. That adds support for ISA devices only decoding 8bit addresses. As we cannot detect such ISA devices/slots that should be a devicetree option or part of the mainboard code. I doubt it's useful on recent PCIe based platforms.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43775 )
Change subject: src: Never set ISA Enable on PCI bridges ......................................................................
Patch Set 1: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43775 )
Change subject: src: Never set ISA Enable on PCI bridges ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
There is some more: AIUI, this bit relates to the avoidance of 0x100..0x3ff ranges in the v3 allocator. Which was dropped in v4. I assume if there were >256 ports resources below a bridge, the v4 allocator would allocate in a range that is later broken by setting this bit.
Should we see if it breaks and react if it does? Basically, let this go and see what happens.
I can try plugging in a few cards into the Asrock B85M Pro4 and see if it survives.
Log: https://dpaste.com/47X6KH8SL.txt
With the following local tree commits: * cd9edb3c37 (HEAD -> master) nb/intel/haswell: Configure VCs on Egress Port * 506e15a7e1 [NOTFORMERGE] mb/asrock/b85m_pro4/devicetree.cb: Make Windows 10 boot * 322bf1785b nb/intel/haswell: Add RMRR for USB devices * fa405dc028 haswell: Report only one HPET timer device * ec05106662 sb/intel/lynxpoint: Add debug print * bae9c0974f src: Never set ISA Enable on PCI bridges * eed8ae9c4e device/pci_device.c: Do not complain about disabled devices * 83d3ca5248 haswell: Set up Root Complex topology * 78b6bfdaec nb/intel/haswell: Enable DMI ASPM * d54c9b0fef (origin/master, origin/HEAD) mb/google/dedede/var/waddledoo: Configure stop delay for SiS TS
Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43775 )
Change subject: src: Never set ISA Enable on PCI bridges ......................................................................
src: Never set ISA Enable on PCI bridges
Looks like no one really knows what this bit would be useful for, nor when it would need to be set. Especially if coreboot is setting it even on PCI *Express* bridges. Digging through git history, nearly all instances of setting it on PCIe bridges comes from i82801gx, for which no reason was given as to why this would be needed. The other instances in Intel code seem to have been, unsurprisingly, copy-pasted.
Drop all uses of this definition and rename it to avoid confusion. The negation in the name could trick people into setting this bit again.
Tested on Asrock B85M Pro4, no visible difference.
Change-Id: Ifaff29561769c111fb7897e95dbea842faec5df4 Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/43775 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Rudolph siro@das-labor.org Reviewed-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/device/cardbus_device.c M src/include/device/pci_def.h M src/soc/intel/broadwell/pcie.c M src/soc/intel/common/block/pcie/pcie.c M src/southbridge/intel/bd82x6x/pcie.c M src/southbridge/intel/i82801gx/pcie.c M src/southbridge/intel/i82801ix/pcie.c M src/southbridge/intel/i82801jx/pcie.c M src/southbridge/intel/lynxpoint/pcie.c 9 files changed, 6 insertions(+), 14 deletions(-)
Approvals: build bot (Jenkins): Verified Kyösti Mälkki: Looks good to me, but someone else must approve Patrick Rudolph: Looks good to me, approved
diff --git a/src/device/cardbus_device.c b/src/device/cardbus_device.c index 266e194..b014946 100644 --- a/src/device/cardbus_device.c +++ b/src/device/cardbus_device.c @@ -119,7 +119,6 @@
ctrl = pci_read_config16(dev, PCI_CB_BRIDGE_CONTROL); ctrl |= (dev->link_list->bridge_ctrl & ( - PCI_BRIDGE_CTL_NO_ISA | PCI_BRIDGE_CTL_VGA | PCI_BRIDGE_CTL_MASTER_ABORT | PCI_BRIDGE_CTL_BUS_RESET)); diff --git a/src/include/device/pci_def.h b/src/include/device/pci_def.h index 07ba4a2..25372bf 100644 --- a/src/include/device/pci_def.h +++ b/src/include/device/pci_def.h @@ -136,7 +136,7 @@ /* Enable parity detection on secondary interface */ #define PCI_BRIDGE_CTL_PARITY 0x01 #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_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 */ diff --git a/src/soc/intel/broadwell/pcie.c b/src/soc/intel/broadwell/pcie.c index 14dcd3f..00a8595 100644 --- a/src/soc/intel/broadwell/pcie.c +++ b/src/soc/intel/broadwell/pcie.c @@ -587,7 +587,6 @@
reg16 = pci_read_config16(dev, PCI_BRIDGE_CONTROL); reg16 &= ~PCI_BRIDGE_CTL_PARITY; - reg16 |= PCI_BRIDGE_CTL_NO_ISA; pci_write_config16(dev, PCI_BRIDGE_CONTROL, reg16);
/* Clear errors in status registers */ diff --git a/src/soc/intel/common/block/pcie/pcie.c b/src/soc/intel/common/block/pcie/pcie.c index f36366a..ada380e 100644 --- a/src/soc/intel/common/block/pcie/pcie.c +++ b/src/soc/intel/common/block/pcie/pcie.c @@ -24,9 +24,8 @@ /* Set Cache Line Size to 0x10 */ pci_write_config8(dev, PCI_CACHE_LINE_SIZE, CACHE_LINE_SIZE);
- /* disable parity error response, enable ISA */ - pci_update_config16(dev, PCI_BRIDGE_CONTROL, - ~PCI_BRIDGE_CTL_PARITY, PCI_BRIDGE_CTL_NO_ISA); + /* disable parity error response */ + pci_and_config16(dev, PCI_BRIDGE_CONTROL, ~PCI_BRIDGE_CTL_PARITY);
if (CONFIG(PCIE_DEBUG_INFO)) { printk(BIOS_SPEW, " MBL = 0x%08x\n", diff --git a/src/southbridge/intel/bd82x6x/pcie.c b/src/southbridge/intel/bd82x6x/pcie.c index 2eff162..f6bffbb 100644 --- a/src/southbridge/intel/bd82x6x/pcie.c +++ b/src/southbridge/intel/bd82x6x/pcie.c @@ -216,8 +216,7 @@ // This has no effect but the OS might expect it pci_write_config8(dev, 0x0c, 0x10);
- pci_update_config16(dev, PCI_BRIDGE_CONTROL, - ~PCI_BRIDGE_CTL_PARITY, PCI_BRIDGE_CTL_NO_ISA); + pci_and_config16(dev, PCI_BRIDGE_CONTROL, ~PCI_BRIDGE_CTL_PARITY);
/* Clear errors in status registers. FIXME: Do something? */ reg16 = pci_read_config16(dev, 0x06); diff --git a/src/southbridge/intel/i82801gx/pcie.c b/src/southbridge/intel/i82801gx/pcie.c index 6c0ca5d..ca0ae2e 100644 --- a/src/southbridge/intel/i82801gx/pcie.c +++ b/src/southbridge/intel/i82801gx/pcie.c @@ -54,7 +54,6 @@
reg16 = pci_read_config16(dev, PCI_BRIDGE_CONTROL); reg16 &= ~PCI_BRIDGE_CTL_PARITY; - reg16 |= PCI_BRIDGE_CTL_NO_ISA; pci_write_config16(dev, PCI_BRIDGE_CONTROL, reg16);
/* Enable IO xAPIC on this PCIe port */ diff --git a/src/southbridge/intel/i82801ix/pcie.c b/src/southbridge/intel/i82801ix/pcie.c index a8e7b11..3900e92 100644 --- a/src/southbridge/intel/i82801ix/pcie.c +++ b/src/southbridge/intel/i82801ix/pcie.c @@ -23,8 +23,7 @@ // This has no effect but the OS might expect it pci_write_config8(dev, 0x0c, 0x10);
- pci_update_config16(dev, PCI_BRIDGE_CONTROL, ~PCI_BRIDGE_CTL_PARITY, - PCI_BRIDGE_CTL_NO_ISA); + pci_and_config16(dev, PCI_BRIDGE_CONTROL, ~PCI_BRIDGE_CTL_PARITY);
/* Enable IO xAPIC on this PCIe port */ pci_or_config32(dev, 0xd8, 1 << 7); diff --git a/src/southbridge/intel/i82801jx/pcie.c b/src/southbridge/intel/i82801jx/pcie.c index 5195522..18d2c72 100644 --- a/src/southbridge/intel/i82801jx/pcie.c +++ b/src/southbridge/intel/i82801jx/pcie.c @@ -23,8 +23,7 @@ // This has no effect but the OS might expect it pci_write_config8(dev, 0x0c, 0x10);
- pci_update_config16(dev, PCI_BRIDGE_CONTROL, ~PCI_BRIDGE_CTL_PARITY, - PCI_BRIDGE_CTL_NO_ISA); + pci_and_config16(dev, PCI_BRIDGE_CONTROL, ~PCI_BRIDGE_CTL_PARITY);
/* Enable IO xAPIC on this PCIe port */ pci_or_config32(dev, 0xd8, 1 << 7); diff --git a/src/southbridge/intel/lynxpoint/pcie.c b/src/southbridge/intel/lynxpoint/pcie.c index 883dfc7..d2950e7 100644 --- a/src/southbridge/intel/lynxpoint/pcie.c +++ b/src/southbridge/intel/lynxpoint/pcie.c @@ -667,7 +667,6 @@
reg16 = pci_read_config16(dev, PCI_BRIDGE_CONTROL); reg16 &= ~PCI_BRIDGE_CTL_PARITY; - reg16 |= PCI_BRIDGE_CTL_NO_ISA; pci_write_config16(dev, PCI_BRIDGE_CONTROL, reg16);
/* Clear errors in status registers */
-
Aaron Durbin (Code Review) -
Angel Pons (Code Review)
-
Kyösti Mälkki (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)