Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31685
Change subject: device/pci_ops: Reuse romstage PCI config for ramstage ......................................................................
device/pci_ops: Reuse romstage PCI config for ramstage
Changing the signatures we do not need to define PCI config access functions twice.
Change-Id: I9364cb34fe8127972c772516a0a0b1d281c5ed00 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/pci_ops_conf1.c M src/console/die.c M src/device/pci_ops.c M src/device/pci_ops_mmconf.c M src/include/console/console.h M src/include/device/pci.h M src/include/device/pci_ops.h M src/include/device/pci_type.h 8 files changed, 54 insertions(+), 133 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/31685/1
diff --git a/src/arch/x86/pci_ops_conf1.c b/src/arch/x86/pci_ops_conf1.c index 1180a82..03c2b64 100644 --- a/src/arch/x86/pci_ops_conf1.c +++ b/src/arch/x86/pci_ops_conf1.c @@ -14,65 +14,17 @@ #include <arch/io.h> #include <device/pci.h> #include <device/pci_ops.h> +#include <arch/pci_io_cfg.h> + /* * Functions for accessing PCI configuration space with type 1 accesses */
-#if !IS_ENABLED(CONFIG_PCI_IO_CFG_EXT) -#define CONF_CMD(bus, devfn, where) (0x80000000 | (bus << 16) | \ - (devfn << 8) | (where & ~3)) -#else -#define CONF_CMD(bus, devfn, where) (0x80000000 | (bus << 16) | \ - (devfn << 8) | ((where & 0xff) & ~3) |\ - ((where & 0xf00)<<16)) -#endif - -static uint8_t pci_conf1_read_config8(int bus, int devfn, int where) -{ - outl(CONF_CMD(bus, devfn, where), 0xCF8); - return inb(0xCFC + (where & 3)); -} - -static uint16_t pci_conf1_read_config16(int bus, int devfn, int where) -{ - outl(CONF_CMD(bus, devfn, where), 0xCF8); - return inw(0xCFC + (where & 2)); -} - -static uint32_t pci_conf1_read_config32(int bus, int devfn, int where) -{ - outl(CONF_CMD(bus, devfn, where), 0xCF8); - return inl(0xCFC); -} - -static void pci_conf1_write_config8(int bus, int devfn, int where, - uint8_t value) -{ - outl(CONF_CMD(bus, devfn, where), 0xCF8); - outb(value, 0xCFC + (where & 3)); -} - -static void pci_conf1_write_config16(int bus, int devfn, int where, - uint16_t value) -{ - outl(CONF_CMD(bus, devfn, where), 0xCF8); - outw(value, 0xCFC + (where & 2)); -} - -static void pci_conf1_write_config32(int bus, int devfn, int where, - uint32_t value) -{ - outl(CONF_CMD(bus, devfn, where), 0xCF8); - outl(value, 0xCFC); -} - -#undef CONF_CMD - const struct pci_bus_operations pci_cf8_conf1 = { - .read8 = pci_conf1_read_config8, - .read16 = pci_conf1_read_config16, - .read32 = pci_conf1_read_config32, - .write8 = pci_conf1_write_config8, - .write16 = pci_conf1_write_config16, - .write32 = pci_conf1_write_config32, + .read8 = pci_io_read_config8, + .read16 = pci_io_read_config16, + .read32 = pci_io_read_config32, + .write8 = pci_io_write_config8, + .write16 = pci_io_write_config16, + .write32 = pci_io_write_config32, }; diff --git a/src/console/die.c b/src/console/die.c index aedf66e..3fae3bd 100644 --- a/src/console/die.c +++ b/src/console/die.c @@ -37,4 +37,9 @@ die_notify(); halt(); } + +void __noreturn pcidev_die(void) +{ + die("PCI: dev is NULL!\n"); +} #endif diff --git a/src/device/pci_ops.c b/src/device/pci_ops.c index 12c4e26..35f4261 100644 --- a/src/device/pci_ops.c +++ b/src/device/pci_ops.c @@ -16,61 +16,48 @@ */
#include <stdint.h> -#include <console/console.h> #include <device/device.h> #include <device/pci.h> #include <device/pci_ops.h> +#include <device/pci_type.h>
static __always_inline const struct pci_bus_operations *pci_bus_ops(void) { return pci_bus_default_ops(); }
-static void pcidev_assert(const struct device *dev) -{ - if (dev) - return; - die("PCI: dev is NULL!\n"); -} - u8 pci_read_config8(struct device *dev, unsigned int where) { - pcidev_assert(dev); - return pci_bus_ops()->read8(dev->bus->secondary, - dev->path.pci.devfn, where); + pci_devfn_t bdf = PCI_BDF(dev); + return pci_bus_ops()->read8(bdf, where); }
u16 pci_read_config16(struct device *dev, unsigned int where) { - pcidev_assert(dev); - return pci_bus_ops()->read16(dev->bus->secondary, - dev->path.pci.devfn, where); + pci_devfn_t bdf = PCI_BDF(dev); + return pci_bus_ops()->read16(bdf, where); }
u32 pci_read_config32(struct device *dev, unsigned int where) { - pcidev_assert(dev); - return pci_bus_ops()->read32(dev->bus->secondary, - dev->path.pci.devfn, where); + pci_devfn_t bdf = PCI_BDF(dev); + return pci_bus_ops()->read32(bdf, where); }
void pci_write_config8(struct device *dev, unsigned int where, u8 val) { - pcidev_assert(dev); - pci_bus_ops()->write8(dev->bus->secondary, - dev->path.pci.devfn, where, val); + pci_devfn_t bdf = PCI_BDF(dev); + pci_bus_ops()->write8(bdf, where, val); }
void pci_write_config16(struct device *dev, unsigned int where, u16 val) { - pcidev_assert(dev); - pci_bus_ops()->write16(dev->bus->secondary, - dev->path.pci.devfn, where, val); + pci_devfn_t bdf = PCI_BDF(dev); + pci_bus_ops()->write16(bdf, where, val); }
void pci_write_config32(struct device *dev, unsigned int where, u32 val) { - pcidev_assert(dev); - pci_bus_ops()->write32(dev->bus->secondary, - dev->path.pci.devfn, where, val); + pci_devfn_t bdf = PCI_BDF(dev); + pci_bus_ops()->write32(bdf, where, val); } diff --git a/src/device/pci_ops_mmconf.c b/src/device/pci_ops_mmconf.c index 0af8f8e..d6fce1d 100644 --- a/src/device/pci_ops_mmconf.c +++ b/src/device/pci_ops_mmconf.c @@ -14,6 +14,7 @@ #include <arch/io.h> #include <device/pci.h> #include <device/pci_ops.h> +#include <device/pci_mmio_cfg.h>
#if (CONFIG_MMCONF_BASE_ADDRESS == 0) #error "CONFIG_MMCONF_BASE_ADDRESS needs to be non-zero!" @@ -23,52 +24,13 @@ * Functions for accessing PCI configuration space with mmconf accesses */
-#define PCI_MMIO_ADDR(SEGBUS, DEVFN, WHERE, MASK) \ - ((void *)(((uintptr_t)CONFIG_MMCONF_BASE_ADDRESS |\ - (((SEGBUS) & 0xFFF) << 20) |\ - (((DEVFN) & 0xFF) << 12) |\ - ((WHERE) & 0xFFF)) & ~MASK)) - -static uint8_t pci_mmconf_read_config8(int bus, int devfn, int where) -{ - return read8(PCI_MMIO_ADDR(bus, devfn, where, 0)); -} - -static uint16_t pci_mmconf_read_config16(int bus, int devfn, int where) -{ - return read16(PCI_MMIO_ADDR(bus, devfn, where, 1)); -} - -static uint32_t pci_mmconf_read_config32(int bus, int devfn, int where) -{ - return read32(PCI_MMIO_ADDR(bus, devfn, where, 3)); -} - -static void pci_mmconf_write_config8(int bus, int devfn, int where, - uint8_t value) -{ - write8(PCI_MMIO_ADDR(bus, devfn, where, 0), value); -} - -static void pci_mmconf_write_config16(int bus, int devfn, int where, - uint16_t value) -{ - write16(PCI_MMIO_ADDR(bus, devfn, where, 1), value); -} - -static void pci_mmconf_write_config32(int bus, int devfn, int where, - uint32_t value) -{ - write32(PCI_MMIO_ADDR(bus, devfn, where, 3), value); -} - static const struct pci_bus_operations pci_ops_mmconf = { - .read8 = pci_mmconf_read_config8, - .read16 = pci_mmconf_read_config16, - .read32 = pci_mmconf_read_config32, - .write8 = pci_mmconf_write_config8, - .write16 = pci_mmconf_write_config16, - .write32 = pci_mmconf_write_config32, + .read8 = pci_mmio_read_config8, + .read16 = pci_mmio_read_config16, + .read32 = pci_mmio_read_config32, + .write8 = pci_mmio_write_config8, + .write16 = pci_mmio_write_config16, + .write32 = pci_mmio_write_config32, };
const struct pci_bus_operations *pci_bus_default_ops(void) diff --git a/src/include/console/console.h b/src/include/console/console.h index 2b02334..06658f2 100644 --- a/src/include/console/console.h +++ b/src/include/console/console.h @@ -41,6 +41,7 @@ /* this function is weak and can be overridden by a mainboard function. */ void mainboard_post(u8 value); void __noreturn die(const char *msg); +void __noreturn pcidev_die(void);
/* * This function is weak and can be overridden to provide additional diff --git a/src/include/device/pci.h b/src/include/device/pci.h index 0ead578..f254c70 100644 --- a/src/include/device/pci.h +++ b/src/include/device/pci.h @@ -36,12 +36,12 @@
/* Common pci bus operations */ struct pci_bus_operations { - uint8_t (*read8)(int bus, int devfn, int where); - uint16_t (*read16)(int bus, int devfn, int where); - uint32_t (*read32)(int bus, int devfn, int where); - void (*write8)(int bus, int devfn, int where, uint8_t val); - void (*write16)(int bus, int devfn, int where, uint16_t val); - void (*write32)(int bus, int devfn, int where, uint32_t val); + uint8_t (*read8)(pci_devfn_t dev, unsigned int where); + uint16_t (*read16)(pci_devfn_t dev, unsigned int where); + uint32_t (*read32)(pci_devfn_t dev, unsigned int where); + void (*write8)(pci_devfn_t dev, unsigned int where, uint8_t val); + void (*write16)(pci_devfn_t dev, unsigned int where, uint16_t val); + void (*write32)(pci_devfn_t dev, unsigned int where, uint32_t val); };
struct pci_driver { diff --git a/src/include/device/pci_ops.h b/src/include/device/pci_ops.h index 6971ce4..f7ad181 100644 --- a/src/include/device/pci_ops.h +++ b/src/include/device/pci_ops.h @@ -18,6 +18,17 @@ #define pci_write_config16 pci_s_write_config16 #define pci_write_config32 pci_s_write_config32 #else + +/* FIXME: ROMCC chokes on this. */ +#include <console/console.h> + +static __always_inline pci_devfn_t pcidev_assert(const struct device *dev) +{ + if (!dev) + pcidev_die(); + return __PCI_BDF(dev); +} + u8 pci_read_config8(struct device *dev, unsigned int where); u16 pci_read_config16(struct device *dev, unsigned int where); u32 pci_read_config32(struct device *dev, unsigned int where); diff --git a/src/include/device/pci_type.h b/src/include/device/pci_type.h index 0b08e8d..2f03186 100644 --- a/src/include/device/pci_type.h +++ b/src/include/device/pci_type.h @@ -26,4 +26,7 @@
#define PCI_DEV_INVALID (0xffffffffU)
+#define __PCI_BDF(dev) ((dev)->bus->secondary << 20 | (dev)->path.pci.devfn << 12) +#define PCI_BDF(dev) pcidev_assert((dev)) + #endif /* DEVICE_PCI_TYPE_H */
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31685 )
Change subject: device/pci_ops: Reuse romstage PCI config for ramstage ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31685/1/src/include/device/pci_type.h File src/include/device/pci_type.h:
https://review.coreboot.org/#/c/31685/1/src/include/device/pci_type.h@29 PS1, Line 29: #define __PCI_BDF(dev) ((dev)->bus->secondary << 20 | (dev)->path.pci.devfn << 12) line over 80 characters
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31685
to look at the new patch set (#2).
Change subject: device/pci_ops: Reuse romstage PCI config for ramstage ......................................................................
device/pci_ops: Reuse romstage PCI config for ramstage
Changing the signatures we do not need to define PCI config access functions twice.
Change-Id: I9364cb34fe8127972c772516a0a0b1d281c5ed00 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/arch/x86/pci_ops_conf1.c M src/device/pci_device.c M src/device/pci_ops.c M src/device/pci_ops_mmconf.c M src/include/device/pci.h M src/include/device/pci_ops.h M src/include/device/pci_type.h M src/southbridge/amd/rs780/rs780.c M src/southbridge/amd/sr5650/pcie.c 9 files changed, 81 insertions(+), 158 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/31685/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31685 )
Change subject: device/pci_ops: Reuse romstage PCI config for ramstage ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31685/2/src/include/device/pci_type.h File src/include/device/pci_type.h:
https://review.coreboot.org/#/c/31685/2/src/include/device/pci_type.h@29 PS2, Line 29: #define __PCI_BDF(dev) ((dev)->bus->secondary << 20 | (dev)->path.pci.devfn << 12) line over 80 characters
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31685 )
Change subject: device/pci_ops: Reuse romstage PCI config for ramstage ......................................................................
Patch Set 4:
This change is ready for review.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31685 )
Change subject: device/pci_ops: Reuse romstage PCI config for ramstage ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/31685/4/src/include/device/pci_type.h File src/include/device/pci_type.h:
https://review.coreboot.org/#/c/31685/4/src/include/device/pci_type.h@29 PS4, Line 29: #define PCI_BDF(dev) pcidev_assert((dev)) Why the macro? Why not just name the function pcidev_bdf()?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31685 )
Change subject: device/pci_ops: Reuse romstage PCI config for ramstage ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/31685/4/src/include/device/pci_type.h File src/include/device/pci_type.h:
https://review.coreboot.org/#/c/31685/4/src/include/device/pci_type.h@29 PS4, Line 29: #define PCI_BDF(dev) pcidev_assert((dev))
Why the macro? Why not just name the function pcidev_bdf()?
We have those pesky NULL pointers from dev_find_slot() on amdfam.
My plan is to have some debugging with PCI_BDF(dev) expanding to some printk using __func__, __line__ to catch those. Has to be done with preprocessor, right?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31685 )
Change subject: device/pci_ops: Reuse romstage PCI config for ramstage ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/31685/4/src/include/device/pci_type.h File src/include/device/pci_type.h:
https://review.coreboot.org/#/c/31685/4/src/include/device/pci_type.h@29 PS4, Line 29: #define PCI_BDF(dev) pcidev_assert((dev))
We have those pesky NULL pointers from dev_find_slot() on amdfam. […]
Right, needs the preprocessor. But with the PCI_BDF() used in generic code, I wonder if that information is of much use?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31685 )
Change subject: device/pci_ops: Reuse romstage PCI config for ramstage ......................................................................
Patch Set 5:
This change is ready for review.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31685 )
Change subject: device/pci_ops: Reuse romstage PCI config for ramstage ......................................................................
Patch Set 5: Code-Review+1
(1 comment)
I'm not very fond of the FIXME ;) is there a solution in the queue, yet? If not, I fear callers could get used to the fact that the "ops" check for NULL while we should actually encourage them to explicitly check before calling.
https://review.coreboot.org/#/c/31685/5/src/include/device/pci_type.h File src/include/device/pci_type.h:
https://review.coreboot.org/#/c/31685/5/src/include/device/pci_type.h@32 PS5, Line 32: * the check themselves. Maybe the following strategy would work: for pcidev_behind() and alike add a _or_die() version and always call that in case the author doesn't care about error handling. And then assume "ops" aren't called with NULL, ofc.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31685 )
Change subject: device/pci_ops: Reuse romstage PCI config for ramstage ......................................................................
Patch Set 5:
(1 comment)
Patch Set 5: Code-Review+1
(1 comment)
I'm not very fond of the FIXME ;) is there a solution in the queue, yet? If not, I fear callers could get used to the fact that the "ops" check for NULL while we should actually encourage them to explicitly check before calling.
For statically declared "ops" functions the call always takes form dev->ops->func(dev), unless you get innovative you will never pass NULL there.
https://review.coreboot.org/#/c/31685/5/src/include/device/pci_type.h File src/include/device/pci_type.h:
https://review.coreboot.org/#/c/31685/5/src/include/device/pci_type.h@32 PS5, Line 32: * the check themselves.
Maybe the following strategy would work: for pcidev_behind() and alike […]
Yep, I just did not get that done yet. Going thru assert() retains behaviour to parent commit and is not a regression per-se.
Also, pcidev_die() could dump return address from stack, to avoid added weight of __func__ strings.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31685 )
Change subject: device/pci_ops: Reuse romstage PCI config for ramstage ......................................................................
Patch Set 5: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/31685/5/src/include/device/pci_type.h File src/include/device/pci_type.h:
https://review.coreboot.org/#/c/31685/5/src/include/device/pci_type.h@32 PS5, Line 32: * the check themselves.
Going thru assert() retains behaviour to parent commit and is not a regression per-se.
Oh, right, must have mixed changes up.
Kyösti Mälkki has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31685 )
Change subject: device/pci_ops: Reuse romstage PCI config for ramstage ......................................................................
device/pci_ops: Reuse romstage PCI config for ramstage
By changing the signatures we do not need to define PCI config accessors separately for ramstage.
Change-Id: I9364cb34fe8127972c772516a0a0b1d281c5ed00 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/31685 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nico Huber nico.h@gmx.de --- M src/arch/x86/pci_ops_conf1.c M src/device/pci_ops_mmconf.c M src/include/device/pci.h M src/include/device/pci_ops.h M src/include/device/pci_type.h 5 files changed, 54 insertions(+), 120 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved
diff --git a/src/arch/x86/pci_ops_conf1.c b/src/arch/x86/pci_ops_conf1.c index 4e2f24e..03c2b64 100644 --- a/src/arch/x86/pci_ops_conf1.c +++ b/src/arch/x86/pci_ops_conf1.c @@ -14,65 +14,17 @@ #include <arch/io.h> #include <device/pci.h> #include <device/pci_ops.h> +#include <arch/pci_io_cfg.h> + /* * Functions for accessing PCI configuration space with type 1 accesses */
-#if !CONFIG(PCI_IO_CFG_EXT) -#define CONF_CMD(dev, reg) (0x80000000 | ((dev)->bus->secondary << 16) | \ - ((dev)->path.pci.devfn << 8) | (reg & ~3)) -#else -#define CONF_CMD(dev, reg) (0x80000000 | ((dev)->bus->secondary << 16) | \ - ((dev)->path.pci.devfn << 8) | ((reg & 0xff) & ~3) |\ - ((reg & 0xf00)<<16)) -#endif - -static uint8_t pci_conf1_read_config8(const struct device *dev, uint16_t reg) -{ - outl(CONF_CMD(dev, reg), 0xCF8); - return inb(0xCFC + (reg & 3)); -} - -static uint16_t pci_conf1_read_config16(const struct device *dev, uint16_t reg) -{ - outl(CONF_CMD(dev, reg), 0xCF8); - return inw(0xCFC + (reg & 2)); -} - -static uint32_t pci_conf1_read_config32(const struct device *dev, uint16_t reg) -{ - outl(CONF_CMD(dev, reg), 0xCF8); - return inl(0xCFC); -} - -static void pci_conf1_write_config8(const struct device *dev, uint16_t reg, - uint8_t value) -{ - outl(CONF_CMD(dev, reg), 0xCF8); - outb(value, 0xCFC + (reg & 3)); -} - -static void pci_conf1_write_config16(const struct device *dev, uint16_t reg, - uint16_t value) -{ - outl(CONF_CMD(dev, reg), 0xCF8); - outw(value, 0xCFC + (reg & 2)); -} - -static void pci_conf1_write_config32(const struct device *dev, uint16_t reg, - uint32_t value) -{ - outl(CONF_CMD(dev, reg), 0xCF8); - outl(value, 0xCFC); -} - -#undef CONF_CMD - const struct pci_bus_operations pci_cf8_conf1 = { - .read8 = pci_conf1_read_config8, - .read16 = pci_conf1_read_config16, - .read32 = pci_conf1_read_config32, - .write8 = pci_conf1_write_config8, - .write16 = pci_conf1_write_config16, - .write32 = pci_conf1_write_config32, + .read8 = pci_io_read_config8, + .read16 = pci_io_read_config16, + .read32 = pci_io_read_config32, + .write8 = pci_io_write_config8, + .write16 = pci_io_write_config16, + .write32 = pci_io_write_config32, }; diff --git a/src/device/pci_ops_mmconf.c b/src/device/pci_ops_mmconf.c index c7c005c..2fcb961 100644 --- a/src/device/pci_ops_mmconf.c +++ b/src/device/pci_ops_mmconf.c @@ -14,6 +14,7 @@ #include <device/mmio.h> #include <device/pci.h> #include <device/pci_ops.h> +#include <device/pci_mmio_cfg.h>
#if (CONFIG_MMCONF_BASE_ADDRESS == 0) #error "CONFIG_MMCONF_BASE_ADDRESS needs to be non-zero!" @@ -23,52 +24,13 @@ * Functions for accessing PCI configuration space with mmconf accesses */
-#define PCI_MMIO_ADDR(dev, reg, mask) \ - ((void *)(((uintptr_t)CONFIG_MMCONF_BASE_ADDRESS |\ - (((dev)->bus->secondary & 0xFFF) << 20) |\ - (((dev)->path.pci.devfn & 0xFF) << 12) |\ - ((reg) & 0xFFF)) & ~mask)) - -static uint8_t pci_mmconf_read_config8(const struct device *dev, uint16_t reg) -{ - return read8(PCI_MMIO_ADDR(dev, reg, 0)); -} - -static uint16_t pci_mmconf_read_config16(const struct device *dev, uint16_t reg) -{ - return read16(PCI_MMIO_ADDR(dev, reg, 1)); -} - -static uint32_t pci_mmconf_read_config32(const struct device *dev, uint16_t reg) -{ - return read32(PCI_MMIO_ADDR(dev, reg, 3)); -} - -static void pci_mmconf_write_config8(const struct device *dev, uint16_t reg, - uint8_t value) -{ - write8(PCI_MMIO_ADDR(dev, reg, 0), value); -} - -static void pci_mmconf_write_config16(const struct device *dev, uint16_t reg, - uint16_t value) -{ - write16(PCI_MMIO_ADDR(dev, reg, 1), value); -} - -static void pci_mmconf_write_config32(const struct device *dev, uint16_t reg, - uint32_t value) -{ - write32(PCI_MMIO_ADDR(dev, reg, 3), value); -} - static const struct pci_bus_operations pci_ops_mmconf = { - .read8 = pci_mmconf_read_config8, - .read16 = pci_mmconf_read_config16, - .read32 = pci_mmconf_read_config32, - .write8 = pci_mmconf_write_config8, - .write16 = pci_mmconf_write_config16, - .write32 = pci_mmconf_write_config32, + .read8 = pci_mmio_read_config8, + .read16 = pci_mmio_read_config16, + .read32 = pci_mmio_read_config32, + .write8 = pci_mmio_write_config8, + .write16 = pci_mmio_write_config16, + .write32 = pci_mmio_write_config32, };
const struct pci_bus_operations *pci_bus_default_ops(void) diff --git a/src/include/device/pci.h b/src/include/device/pci.h index 2cfcb60..14c4693 100644 --- a/src/include/device/pci.h +++ b/src/include/device/pci.h @@ -35,14 +35,17 @@
/* Common pci bus operations */ struct pci_bus_operations { - uint8_t (*read8)(const struct device *dev, uint16_t reg); - uint16_t (*read16)(const struct device *dev, uint16_t reg); - uint32_t (*read32)(const struct device *dev, uint16_t reg); - void (*write8)(const struct device *dev, uint16_t reg, uint8_t val); - void (*write16)(const struct device *dev, uint16_t reg, uint16_t val); - void (*write32)(const struct device *dev, uint16_t reg, uint32_t val); + uint8_t (*read8)(pci_devfn_t dev, uint16_t reg); + uint16_t (*read16)(pci_devfn_t dev, uint16_t reg); + uint32_t (*read32)(pci_devfn_t dev, uint16_t reg); + void (*write8)(pci_devfn_t dev, uint16_t reg, uint8_t val); + void (*write16)(pci_devfn_t dev, uint16_t reg, uint16_t val); + void (*write32)(pci_devfn_t dev, uint16_t reg, uint32_t val); };
+// FIXME: Needs complete pci_bus_operations +#include <device/pci_ops.h> + struct pci_driver { const struct device_operations *ops; unsigned short vendor; diff --git a/src/include/device/pci_ops.h b/src/include/device/pci_ops.h index 82f9658..bb77754 100644 --- a/src/include/device/pci_ops.h +++ b/src/include/device/pci_ops.h @@ -48,52 +48,58 @@
void __noreturn pcidev_die(void);
-static __always_inline void pcidev_assert(const struct device *dev) +static __always_inline pci_devfn_t pcidev_bdf(const struct device *dev) +{ + return (dev->path.pci.devfn << 12) | (dev->bus->secondary << 20); +} + +static __always_inline pci_devfn_t pcidev_assert(const struct device *dev) { if (!dev) pcidev_die(); + return pcidev_bdf(dev); }
static __always_inline u8 pci_read_config8(const struct device *dev, u16 reg) { - pcidev_assert(dev); - return pci_bus_ops()->read8(dev, reg); + pci_devfn_t bdf = PCI_BDF(dev); + return pci_bus_ops()->read8(bdf, reg); }
static __always_inline u16 pci_read_config16(const struct device *dev, u16 reg) { - pcidev_assert(dev); - return pci_bus_ops()->read16(dev, reg); + pci_devfn_t bdf = PCI_BDF(dev); + return pci_bus_ops()->read16(bdf, reg); }
static __always_inline u32 pci_read_config32(const struct device *dev, u16 reg) { - pcidev_assert(dev); - return pci_bus_ops()->read32(dev, reg); + pci_devfn_t bdf = PCI_BDF(dev); + return pci_bus_ops()->read32(bdf, reg); }
static __always_inline void pci_write_config8(const struct device *dev, u16 reg, u8 val) { - pcidev_assert(dev); - pci_bus_ops()->write8(dev, reg, val); + pci_devfn_t bdf = PCI_BDF(dev); + pci_bus_ops()->write8(bdf, reg, val); }
static __always_inline void pci_write_config16(const struct device *dev, u16 reg, u16 val) { - pcidev_assert(dev); - pci_bus_ops()->write16(dev, reg, val); + pci_devfn_t bdf = PCI_BDF(dev); + pci_bus_ops()->write16(bdf, reg, val); }
static __always_inline void pci_write_config32(const struct device *dev, u16 reg, u32 val) { - pcidev_assert(dev); - pci_bus_ops()->write32(dev, reg, val); + pci_devfn_t bdf = PCI_BDF(dev); + pci_bus_ops()->write32(bdf, reg, val); }
#endif diff --git a/src/include/device/pci_type.h b/src/include/device/pci_type.h index 3f72c5f..27d3558 100644 --- a/src/include/device/pci_type.h +++ b/src/include/device/pci_type.h @@ -25,4 +25,15 @@
#define PCI_DEV_INVALID (0xffffffffU)
+#if 1 +/* FIXME: For most of the time in ramstage, we get valid device pointer + * from calling the driver entry points. The assert should only be used + * with searches like pcidev_behind(), and only if caller does not make + * the check themselves. + */ +#define PCI_BDF(dev) pcidev_assert((dev)) +#else +#define PCI_BDF(dev) pcidev_bdf((dev)) +#endif + #endif /* DEVICE_PCI_TYPE_H */