Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35672 )
Change subject: device/pci_early: Drop some __SIMPLE_DEVICE__ use ......................................................................
device/pci_early: Drop some __SIMPLE_DEVICE__ use
The simple PCI config accessors are always available under names pci_s_[read|write]_configX.
We have some use for PCI bridge configurations and resets in romstages, so expose them.
Change-Id: Ia97a4e1f1b4c80b3dae800d80615bdc118414ed3 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/device/pci_early.c M src/include/device/pci.h 2 files changed, 40 insertions(+), 39 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/35672/1
diff --git a/src/device/pci_early.c b/src/device/pci_early.c index 880480d..b15f4a3 100644 --- a/src/device/pci_early.c +++ b/src/device/pci_early.c @@ -11,64 +11,61 @@ * GNU General Public License for more details. */
-#define __SIMPLE_DEVICE__ - #include <device/pci.h> #include <device/pci_def.h> #include <device/pci_ops.h> #include <device/pci_type.h> #include <delay.h>
-static void pci_bridge_reset_secondary(pci_devfn_t p2p_bridge) +void pci_s_assert_secondary_reset(pci_devfn_t p2p_bridge) { u16 reg16; - - /* First we reset the secondary bus. */ - reg16 = pci_read_config16(p2p_bridge, PCI_BRIDGE_CONTROL); - reg16 |= (1 << 6); /* SRESET */ - pci_write_config16(p2p_bridge, PCI_BRIDGE_CONTROL, reg16); - - /* Assume we don't have to wait here forever */ - - /* Read back and clear reset bit. */ - reg16 = pci_read_config16(p2p_bridge, PCI_BRIDGE_CONTROL); - reg16 &= ~(1 << 6); /* SRESET */ - pci_write_config16(p2p_bridge, PCI_BRIDGE_CONTROL, reg16); + reg16 = pci_s_read_config16(p2p_bridge, PCI_BRIDGE_CONTROL); + reg16 |= PCI_BRIDGE_CTL_BUS_RESET; + pci_s_write_config16(p2p_bridge, PCI_BRIDGE_CONTROL, reg16); }
-static void pci_bridge_set_secondary(pci_devfn_t p2p_bridge, u8 secondary) +void pci_s_deassert_secondary_reset(pci_devfn_t p2p_bridge) +{ + u16 reg16; + reg16 = pci_s_read_config16(p2p_bridge, PCI_BRIDGE_CONTROL); + reg16 &= ~PCI_BRIDGE_CTL_BUS_RESET; + pci_s_write_config16(p2p_bridge, PCI_BRIDGE_CONTROL, reg16); +} + +void pci_s_bridge_set_secondary(pci_devfn_t p2p_bridge, u8 secondary) { /* Disable config transaction forwarding. */ - pci_write_config8(p2p_bridge, PCI_SECONDARY_BUS, 0x00); - pci_write_config8(p2p_bridge, PCI_SUBORDINATE_BUS, 0x00); + pci_s_write_config8(p2p_bridge, PCI_SECONDARY_BUS, 0x00); + pci_s_write_config8(p2p_bridge, PCI_SUBORDINATE_BUS, 0x00); /* Enable config transaction forwarding. */ - pci_write_config8(p2p_bridge, PCI_SECONDARY_BUS, secondary); - pci_write_config8(p2p_bridge, PCI_SUBORDINATE_BUS, secondary); + pci_s_write_config8(p2p_bridge, PCI_SECONDARY_BUS, secondary); + pci_s_write_config8(p2p_bridge, PCI_SUBORDINATE_BUS, secondary); }
-static void pci_bridge_set_mmio(pci_devfn_t p2p_bridge, u32 base, u32 size) +static void pci_s_bridge_set_mmio(pci_devfn_t p2p_bridge, u32 base, u32 size) { u16 reg16;
/* Disable MMIO window behind the bridge. */ - reg16 = pci_read_config16(p2p_bridge, PCI_COMMAND); + reg16 = pci_s_read_config16(p2p_bridge, PCI_COMMAND); reg16 &= ~PCI_COMMAND_MEMORY; - pci_write_config16(p2p_bridge, PCI_COMMAND, reg16); - pci_write_config32(p2p_bridge, PCI_MEMORY_BASE, 0x10); + pci_s_write_config16(p2p_bridge, PCI_COMMAND, reg16); + pci_s_write_config32(p2p_bridge, PCI_MEMORY_BASE, 0x10);
if (!size) return;
/* Enable MMIO window behind the bridge. */ - pci_write_config32(p2p_bridge, PCI_MEMORY_BASE, + pci_s_write_config32(p2p_bridge, PCI_MEMORY_BASE, ((base + size - 1) & 0xfff00000) | ((base >> 16) & 0xfff0));
- reg16 = pci_read_config16(p2p_bridge, PCI_COMMAND); + reg16 = pci_s_read_config16(p2p_bridge, PCI_COMMAND); reg16 |= PCI_COMMAND_MEMORY; - pci_write_config16(p2p_bridge, PCI_COMMAND, reg16); + pci_s_write_config16(p2p_bridge, PCI_COMMAND, reg16); }
-void pci_early_mmio_window(pci_devfn_t p2p_bridge, u32 mmio_base, u32 mmio_size) +static void pci_s_early_mmio_window(pci_devfn_t p2p_bridge, u32 mmio_base, u32 mmio_size) { int timeout, ret = -1;
@@ -79,12 +76,14 @@ u8 dev = 0;
/* Enable configuration and MMIO over bridge. */ - pci_bridge_reset_secondary(p2p_bridge); - pci_bridge_set_secondary(p2p_bridge, secondary); - pci_bridge_set_mmio(p2p_bridge, mmio_base, mmio_size); + pci_s_assert_secondary_reset(p2p_bridge); + pci_s_deassert_secondary_reset(p2p_bridge); + pci_s_bridge_set_secondary(p2p_bridge, secondary); + pci_s_bridge_set_mmio(p2p_bridge, mmio_base, mmio_size);
for (timeout = 20000; timeout; timeout--) { - u32 id = pci_read_config32(PCI_DEV(secondary, dev, 0), PCI_VENDOR_ID); + pci_devfn_t dbg_dev = PCI_DEV(secondary, dev, 0); + u32 id = pci_s_read_config32(dbg_dev, PCI_VENDOR_ID); if (id != 0 && id != 0xffffffff && id != 0xffff0001) break; udelay(10); @@ -95,13 +94,13 @@
/* Disable MMIO window if we found no suitable device. */ if (ret) - pci_bridge_set_mmio(p2p_bridge, 0, 0); + pci_s_bridge_set_mmio(p2p_bridge, 0, 0);
/* Resource allocator will reconfigure bridges and secondary bus * number may change. Thus early device cannot reliably use config * transactions from here on, so we may as well disable them. */ - pci_bridge_set_secondary(p2p_bridge, 0); + pci_s_bridge_set_secondary(p2p_bridge, 0); }
void pci_early_bridge_init(void) @@ -112,7 +111,7 @@ pci_devfn_t p2p_bridge = PCI_DEV(0, CONFIG_EARLY_PCI_BRIDGE_DEVICE, CONFIG_EARLY_PCI_BRIDGE_FUNCTION);
- pci_early_mmio_window(p2p_bridge, CONFIG_EARLY_PCI_MMIO_BASE, 0x4000); + pci_s_early_mmio_window(p2p_bridge, CONFIG_EARLY_PCI_MMIO_BASE, 0x4000); }
/* FIXME: A lot of issues using the following, please avoid. @@ -123,7 +122,7 @@ { for (; dev <= PCI_DEV(255, 31, 7); dev += PCI_DEV(0, 0, 1)) { unsigned int id; - id = pci_read_config32(dev, 0); + id = pci_s_read_config32(dev, 0); if (id == pci_id) return dev; } @@ -139,7 +138,7 @@
for (; dev <= last; dev += PCI_DEV(0, 0, 1)) { unsigned int id; - id = pci_read_config32(dev, 0); + id = pci_s_read_config32(dev, 0); if (id == pci_id) return dev; } diff --git a/src/include/device/pci.h b/src/include/device/pci.h index 8d6a9ae..f091105 100644 --- a/src/include/device/pci.h +++ b/src/include/device/pci.h @@ -116,8 +116,10 @@ pci_devfn_t pci_locate_device(unsigned int pci_id, pci_devfn_t dev); pci_devfn_t pci_locate_device_on_bus(unsigned int pci_id, unsigned int bus);
-void pci_early_mmio_window(pci_devfn_t p2p_bridge, u32 mmio_base, - u32 mmio_size); +void pci_s_assert_secondary_reset(pci_devfn_t p2p_bridge); +void pci_s_deassert_secondary_reset(pci_devfn_t p2p_bridge); +void pci_s_bridge_set_secondary(pci_devfn_t p2p_bridge, u8 secondary); + int pci_early_device_probe(u8 bus, u8 dev, u32 mmio_base);
static inline int pci_base_address_is_memory_space(unsigned int attr)
Petr Cvek has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35672 )
Change subject: device/pci_early: Drop some __SIMPLE_DEVICE__ use ......................................................................
Patch Set 1:
(2 comments)
I may have had a problem with PCIe x16 detection on kontron 986lcd-m if there was no delay in i945_setup_pci_express_x16(). There is a waiting in pci_bus_reset(). The use of the same wait values (10ms between, 1s after) fixed the problem.
https://review.coreboot.org/c/coreboot/+/35672/1/src/device/pci_early.c File src/device/pci_early.c:
https://review.coreboot.org/c/coreboot/+/35672/1/src/device/pci_early.c@a31 PS1, Line 31: / pci_bus_reset() has 10ms here
https://review.coreboot.org/c/coreboot/+/35672/1/src/device/pci_early.c@a36 PS1, Line 36: pci_write_config16(p2p_bridge, PCI_BRIDGE_CONTROL, reg16); pci_bus_reset() has 1 second delay here
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35672 )
Change subject: device/pci_early: Drop some __SIMPLE_DEVICE__ use ......................................................................
Patch Set 1:
(2 comments)
Patch Set 1:
(2 comments)
I may have had a problem with PCIe x16 detection on kontron 986lcd-m if there was no delay in i945_setup_pci_express_x16(). There is a waiting in pci_bus_reset(). The use of the same wait values (10ms between, 1s after) fixed the problem.
See CB:35678 such mdelay() can be followup commit.
https://review.coreboot.org/c/coreboot/+/35672/1/src/device/pci_early.c File src/device/pci_early.c:
https://review.coreboot.org/c/coreboot/+/35672/1/src/device/pci_early.c@a31 PS1, Line 31: /
pci_bus_reset() has 10ms here
I agree there should be some minimum time secondary bus is held down, need to check specs what the value is, and fix the couple call sites that may violate it. Followup work, unless you notice I have removed such delay here somewhere.
https://review.coreboot.org/c/coreboot/+/35672/1/src/device/pci_early.c@a36 PS1, Line 36: pci_write_config16(p2p_bridge, PCI_BRIDGE_CONTROL, reg16);
pci_bus_reset() has 1 second delay here
Right.. (long) arbitrary delay in the wrong place. After deassert of secondary reset, there should be a retry loop on accessing secondary side PCI configuration space VENDOR_ID register.
AFAICS, pci_bus_reset() is never called, unless you have HyperTransport hardware that sets reset_needed flag.
Petr Cvek has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35672 )
Change subject: device/pci_early: Drop some __SIMPLE_DEVICE__ use ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35672/1/src/device/pci_early.c File src/device/pci_early.c:
https://review.coreboot.org/c/coreboot/+/35672/1/src/device/pci_early.c@a36 PS1, Line 36: pci_write_config16(p2p_bridge, PCI_BRIDGE_CONTROL, reg16);
Right.. (long) arbitrary delay in the wrong place. […]
ad retry loop: I'm not sure if device 0 is always required to be on secondary side I think I've seen configurations with only device 1 or 5 on the whole bus.
ad hypertransport: yeah that seems to be true, but still, the pci_bus_reset() is used in pcie, pciX and cardbus device operation structure.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35672 )
Change subject: device/pci_early: Drop some __SIMPLE_DEVICE__ use ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35672/1/src/device/pci_early.c File src/device/pci_early.c:
https://review.coreboot.org/c/coreboot/+/35672/1/src/device/pci_early.c@a36 PS1, Line 36: pci_write_config16(p2p_bridge, PCI_BRIDGE_CONTROL, reg16);
ad retry loop: I'm not sure if device 0 is always required to be on secondary side I think I've seen […]
Conventional PCI might not have device 0 on secondary. I think PCIe bridges have device 0 compulsory, and only PCIe switches may have devices other than 0 on secondary.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35672 )
Change subject: device/pci_early: Drop some __SIMPLE_DEVICE__ use ......................................................................
Patch Set 1: Code-Review+2
Kyösti Mälkki has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35672 )
Change subject: device/pci_early: Drop some __SIMPLE_DEVICE__ use ......................................................................
device/pci_early: Drop some __SIMPLE_DEVICE__ use
The simple PCI config accessors are always available under names pci_s_[read|write]_configX.
We have some use for PCI bridge configurations and resets in romstages, so expose them.
Change-Id: Ia97a4e1f1b4c80b3dae800d80615bdc118414ed3 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35672 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org --- M src/device/pci_early.c M src/include/device/pci.h 2 files changed, 40 insertions(+), 39 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
diff --git a/src/device/pci_early.c b/src/device/pci_early.c index 880480d..b15f4a3 100644 --- a/src/device/pci_early.c +++ b/src/device/pci_early.c @@ -11,64 +11,61 @@ * GNU General Public License for more details. */
-#define __SIMPLE_DEVICE__ - #include <device/pci.h> #include <device/pci_def.h> #include <device/pci_ops.h> #include <device/pci_type.h> #include <delay.h>
-static void pci_bridge_reset_secondary(pci_devfn_t p2p_bridge) +void pci_s_assert_secondary_reset(pci_devfn_t p2p_bridge) { u16 reg16; - - /* First we reset the secondary bus. */ - reg16 = pci_read_config16(p2p_bridge, PCI_BRIDGE_CONTROL); - reg16 |= (1 << 6); /* SRESET */ - pci_write_config16(p2p_bridge, PCI_BRIDGE_CONTROL, reg16); - - /* Assume we don't have to wait here forever */ - - /* Read back and clear reset bit. */ - reg16 = pci_read_config16(p2p_bridge, PCI_BRIDGE_CONTROL); - reg16 &= ~(1 << 6); /* SRESET */ - pci_write_config16(p2p_bridge, PCI_BRIDGE_CONTROL, reg16); + reg16 = pci_s_read_config16(p2p_bridge, PCI_BRIDGE_CONTROL); + reg16 |= PCI_BRIDGE_CTL_BUS_RESET; + pci_s_write_config16(p2p_bridge, PCI_BRIDGE_CONTROL, reg16); }
-static void pci_bridge_set_secondary(pci_devfn_t p2p_bridge, u8 secondary) +void pci_s_deassert_secondary_reset(pci_devfn_t p2p_bridge) +{ + u16 reg16; + reg16 = pci_s_read_config16(p2p_bridge, PCI_BRIDGE_CONTROL); + reg16 &= ~PCI_BRIDGE_CTL_BUS_RESET; + pci_s_write_config16(p2p_bridge, PCI_BRIDGE_CONTROL, reg16); +} + +void pci_s_bridge_set_secondary(pci_devfn_t p2p_bridge, u8 secondary) { /* Disable config transaction forwarding. */ - pci_write_config8(p2p_bridge, PCI_SECONDARY_BUS, 0x00); - pci_write_config8(p2p_bridge, PCI_SUBORDINATE_BUS, 0x00); + pci_s_write_config8(p2p_bridge, PCI_SECONDARY_BUS, 0x00); + pci_s_write_config8(p2p_bridge, PCI_SUBORDINATE_BUS, 0x00); /* Enable config transaction forwarding. */ - pci_write_config8(p2p_bridge, PCI_SECONDARY_BUS, secondary); - pci_write_config8(p2p_bridge, PCI_SUBORDINATE_BUS, secondary); + pci_s_write_config8(p2p_bridge, PCI_SECONDARY_BUS, secondary); + pci_s_write_config8(p2p_bridge, PCI_SUBORDINATE_BUS, secondary); }
-static void pci_bridge_set_mmio(pci_devfn_t p2p_bridge, u32 base, u32 size) +static void pci_s_bridge_set_mmio(pci_devfn_t p2p_bridge, u32 base, u32 size) { u16 reg16;
/* Disable MMIO window behind the bridge. */ - reg16 = pci_read_config16(p2p_bridge, PCI_COMMAND); + reg16 = pci_s_read_config16(p2p_bridge, PCI_COMMAND); reg16 &= ~PCI_COMMAND_MEMORY; - pci_write_config16(p2p_bridge, PCI_COMMAND, reg16); - pci_write_config32(p2p_bridge, PCI_MEMORY_BASE, 0x10); + pci_s_write_config16(p2p_bridge, PCI_COMMAND, reg16); + pci_s_write_config32(p2p_bridge, PCI_MEMORY_BASE, 0x10);
if (!size) return;
/* Enable MMIO window behind the bridge. */ - pci_write_config32(p2p_bridge, PCI_MEMORY_BASE, + pci_s_write_config32(p2p_bridge, PCI_MEMORY_BASE, ((base + size - 1) & 0xfff00000) | ((base >> 16) & 0xfff0));
- reg16 = pci_read_config16(p2p_bridge, PCI_COMMAND); + reg16 = pci_s_read_config16(p2p_bridge, PCI_COMMAND); reg16 |= PCI_COMMAND_MEMORY; - pci_write_config16(p2p_bridge, PCI_COMMAND, reg16); + pci_s_write_config16(p2p_bridge, PCI_COMMAND, reg16); }
-void pci_early_mmio_window(pci_devfn_t p2p_bridge, u32 mmio_base, u32 mmio_size) +static void pci_s_early_mmio_window(pci_devfn_t p2p_bridge, u32 mmio_base, u32 mmio_size) { int timeout, ret = -1;
@@ -79,12 +76,14 @@ u8 dev = 0;
/* Enable configuration and MMIO over bridge. */ - pci_bridge_reset_secondary(p2p_bridge); - pci_bridge_set_secondary(p2p_bridge, secondary); - pci_bridge_set_mmio(p2p_bridge, mmio_base, mmio_size); + pci_s_assert_secondary_reset(p2p_bridge); + pci_s_deassert_secondary_reset(p2p_bridge); + pci_s_bridge_set_secondary(p2p_bridge, secondary); + pci_s_bridge_set_mmio(p2p_bridge, mmio_base, mmio_size);
for (timeout = 20000; timeout; timeout--) { - u32 id = pci_read_config32(PCI_DEV(secondary, dev, 0), PCI_VENDOR_ID); + pci_devfn_t dbg_dev = PCI_DEV(secondary, dev, 0); + u32 id = pci_s_read_config32(dbg_dev, PCI_VENDOR_ID); if (id != 0 && id != 0xffffffff && id != 0xffff0001) break; udelay(10); @@ -95,13 +94,13 @@
/* Disable MMIO window if we found no suitable device. */ if (ret) - pci_bridge_set_mmio(p2p_bridge, 0, 0); + pci_s_bridge_set_mmio(p2p_bridge, 0, 0);
/* Resource allocator will reconfigure bridges and secondary bus * number may change. Thus early device cannot reliably use config * transactions from here on, so we may as well disable them. */ - pci_bridge_set_secondary(p2p_bridge, 0); + pci_s_bridge_set_secondary(p2p_bridge, 0); }
void pci_early_bridge_init(void) @@ -112,7 +111,7 @@ pci_devfn_t p2p_bridge = PCI_DEV(0, CONFIG_EARLY_PCI_BRIDGE_DEVICE, CONFIG_EARLY_PCI_BRIDGE_FUNCTION);
- pci_early_mmio_window(p2p_bridge, CONFIG_EARLY_PCI_MMIO_BASE, 0x4000); + pci_s_early_mmio_window(p2p_bridge, CONFIG_EARLY_PCI_MMIO_BASE, 0x4000); }
/* FIXME: A lot of issues using the following, please avoid. @@ -123,7 +122,7 @@ { for (; dev <= PCI_DEV(255, 31, 7); dev += PCI_DEV(0, 0, 1)) { unsigned int id; - id = pci_read_config32(dev, 0); + id = pci_s_read_config32(dev, 0); if (id == pci_id) return dev; } @@ -139,7 +138,7 @@
for (; dev <= last; dev += PCI_DEV(0, 0, 1)) { unsigned int id; - id = pci_read_config32(dev, 0); + id = pci_s_read_config32(dev, 0); if (id == pci_id) return dev; } diff --git a/src/include/device/pci.h b/src/include/device/pci.h index 8d6a9ae..f091105 100644 --- a/src/include/device/pci.h +++ b/src/include/device/pci.h @@ -116,8 +116,10 @@ pci_devfn_t pci_locate_device(unsigned int pci_id, pci_devfn_t dev); pci_devfn_t pci_locate_device_on_bus(unsigned int pci_id, unsigned int bus);
-void pci_early_mmio_window(pci_devfn_t p2p_bridge, u32 mmio_base, - u32 mmio_size); +void pci_s_assert_secondary_reset(pci_devfn_t p2p_bridge); +void pci_s_deassert_secondary_reset(pci_devfn_t p2p_bridge); +void pci_s_bridge_set_secondary(pci_devfn_t p2p_bridge, u8 secondary); + int pci_early_device_probe(u8 bus, u8 dev, u32 mmio_base);
static inline int pci_base_address_is_memory_space(unsigned int attr)
-
Aaron Durbin (Code Review) -
Kyösti Mälkki (Code Review)
-
Petr Cvek (Code Review)