Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31684
Change subject: device/pci_ops: Simplify logic for PCI bus ops ......................................................................
device/pci_ops: Simplify logic for PCI bus ops
Nobody ever sets ops_pci_bus. This implies pci_bus_ops() always returns pci_bus_default_ops() and get_pbus returns NULL.
Change-Id: Ia30d579e1efe6542dc58714f2e7077507847c0de Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/device/pci_ops.c M src/include/device/device.h 2 files changed, 20 insertions(+), 55 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/31684/1
diff --git a/src/device/pci_ops.c b/src/device/pci_ops.c index b7c8942..12c4e26 100644 --- a/src/device/pci_ops.c +++ b/src/device/pci_ops.c @@ -15,96 +15,62 @@ * GNU General Public License for more details. */
+#include <stdint.h> #include <console/console.h> +#include <device/device.h> #include <device/pci.h> #include <device/pci_ops.h>
-static const struct pci_bus_operations *pci_bus_ops(struct bus *bus, struct device *dev) +static __always_inline const struct pci_bus_operations *pci_bus_ops(void) { - const struct pci_bus_operations *bops; - bops = NULL; - if (bus && bus->dev && bus->dev->ops && bus->dev->ops->ops_pci_bus) { - bops = bus->dev->ops->ops_pci_bus(dev); - } - if (!bops) - bops = pci_bus_default_ops(); - return bops; + return pci_bus_default_ops(); }
-/* - * The only consumer of the return value of get_pbus() is pci_bus_ops(). - * pci_bus_ops() can handle being passed NULL and auto-picks working ops. - */ -static struct bus *get_pbus(struct device *dev) +static void pcidev_assert(const struct device *dev) { - struct bus *pbus = NULL; - - if (!dev) - die("get_pbus: dev is NULL!\n"); - else - pbus = dev->bus; - - while (pbus && pbus->dev && !pci_bus_ops(pbus, dev)) { - if (pbus == pbus->dev->bus) { - printk(BIOS_ALERT, "%s in endless loop looking for a " - "parent bus with pci_bus_ops for %s, breaking " - "out.\n", __func__, dev_path(dev)); - break; - } - pbus = pbus->dev->bus; - } - - if (!pbus || !pbus->dev || !pbus->dev->ops - || !pbus->dev->ops->ops_pci_bus) { - /* This can happen before the device tree is fully set up. */ - - // printk(BIOS_EMERG, "%s: Cannot find PCI bus operations.\n", - // dev_path(dev)); - - pbus = NULL; - } - - return pbus; + if (dev) + return; + die("PCI: dev is NULL!\n"); }
u8 pci_read_config8(struct device *dev, unsigned int where) { - struct bus *pbus = get_pbus(dev); - return pci_bus_ops(pbus, dev)->read8(dev->bus->secondary, + pcidev_assert(dev); + return pci_bus_ops()->read8(dev->bus->secondary, dev->path.pci.devfn, where); }
u16 pci_read_config16(struct device *dev, unsigned int where) { - struct bus *pbus = get_pbus(dev); - return pci_bus_ops(pbus, dev)->read16(dev->bus->secondary, + pcidev_assert(dev); + return pci_bus_ops()->read16(dev->bus->secondary, dev->path.pci.devfn, where); }
u32 pci_read_config32(struct device *dev, unsigned int where) { - struct bus *pbus = get_pbus(dev); - return pci_bus_ops(pbus, dev)->read32(dev->bus->secondary, + pcidev_assert(dev); + return pci_bus_ops()->read32(dev->bus->secondary, dev->path.pci.devfn, where); }
void pci_write_config8(struct device *dev, unsigned int where, u8 val) { - struct bus *pbus = get_pbus(dev); - pci_bus_ops(pbus, dev)->write8(dev->bus->secondary, + pcidev_assert(dev); + pci_bus_ops()->write8(dev->bus->secondary, dev->path.pci.devfn, where, val); }
void pci_write_config16(struct device *dev, unsigned int where, u16 val) { - struct bus *pbus = get_pbus(dev); - pci_bus_ops(pbus, dev)->write16(dev->bus->secondary, + pcidev_assert(dev); + pci_bus_ops()->write16(dev->bus->secondary, dev->path.pci.devfn, where, val); }
void pci_write_config32(struct device *dev, unsigned int where, u32 val) { - struct bus *pbus = get_pbus(dev); - pci_bus_ops(pbus, dev)->write32(dev->bus->secondary, + pcidev_assert(dev); + pci_bus_ops()->write32(dev->bus->secondary, dev->path.pci.devfn, where, val); } diff --git a/src/include/device/device.h b/src/include/device/device.h index c794322..2161c32 100644 --- a/src/include/device/device.h +++ b/src/include/device/device.h @@ -69,7 +69,6 @@ const struct i2c_bus_operations *ops_i2c_bus; const struct spi_bus_operations *ops_spi_bus; const struct smbus_bus_operations *ops_smbus_bus; - const struct pci_bus_operations * (*ops_pci_bus)(struct device *dev); const struct pnp_mode_ops *ops_pnp_mode; };
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31684 )
Change subject: device/pci_ops: Simplify logic for PCI bus ops ......................................................................
Patch Set 2:
This change is ready for review.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31684 )
Change subject: device/pci_ops: Simplify logic for PCI bus ops ......................................................................
Patch Set 2: Code-Review+2
Did no one try to implement mmconf access on an old amd device where the northbridge pci functions couldn't be accessed through mmconf in the southbridge? I would have expected this function to be utilized for such a scenario. But if no one used it then we can reduce the current complexity.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31684 )
Change subject: device/pci_ops: Simplify logic for PCI bus ops ......................................................................
Patch Set 2: Code-Review+2
Nico Huber has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31684 )
Change subject: device/pci_ops: Simplify logic for PCI bus ops ......................................................................
device/pci_ops: Simplify logic for PCI bus ops
Nobody ever sets ops_pci_bus. This implies pci_bus_ops() always returns pci_bus_default_ops() and get_pbus returns NULL.
Change-Id: Ia30d579e1efe6542dc58714f2e7077507847c0de Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/31684 Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Nico Huber nico.h@gmx.de Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/device/pci_ops.c M src/include/device/device.h 2 files changed, 20 insertions(+), 55 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Aaron Durbin: Looks good to me, approved
diff --git a/src/device/pci_ops.c b/src/device/pci_ops.c index b7c8942..12c4e26 100644 --- a/src/device/pci_ops.c +++ b/src/device/pci_ops.c @@ -15,96 +15,62 @@ * GNU General Public License for more details. */
+#include <stdint.h> #include <console/console.h> +#include <device/device.h> #include <device/pci.h> #include <device/pci_ops.h>
-static const struct pci_bus_operations *pci_bus_ops(struct bus *bus, struct device *dev) +static __always_inline const struct pci_bus_operations *pci_bus_ops(void) { - const struct pci_bus_operations *bops; - bops = NULL; - if (bus && bus->dev && bus->dev->ops && bus->dev->ops->ops_pci_bus) { - bops = bus->dev->ops->ops_pci_bus(dev); - } - if (!bops) - bops = pci_bus_default_ops(); - return bops; + return pci_bus_default_ops(); }
-/* - * The only consumer of the return value of get_pbus() is pci_bus_ops(). - * pci_bus_ops() can handle being passed NULL and auto-picks working ops. - */ -static struct bus *get_pbus(struct device *dev) +static void pcidev_assert(const struct device *dev) { - struct bus *pbus = NULL; - - if (!dev) - die("get_pbus: dev is NULL!\n"); - else - pbus = dev->bus; - - while (pbus && pbus->dev && !pci_bus_ops(pbus, dev)) { - if (pbus == pbus->dev->bus) { - printk(BIOS_ALERT, "%s in endless loop looking for a " - "parent bus with pci_bus_ops for %s, breaking " - "out.\n", __func__, dev_path(dev)); - break; - } - pbus = pbus->dev->bus; - } - - if (!pbus || !pbus->dev || !pbus->dev->ops - || !pbus->dev->ops->ops_pci_bus) { - /* This can happen before the device tree is fully set up. */ - - // printk(BIOS_EMERG, "%s: Cannot find PCI bus operations.\n", - // dev_path(dev)); - - pbus = NULL; - } - - return pbus; + if (dev) + return; + die("PCI: dev is NULL!\n"); }
u8 pci_read_config8(struct device *dev, unsigned int where) { - struct bus *pbus = get_pbus(dev); - return pci_bus_ops(pbus, dev)->read8(dev->bus->secondary, + pcidev_assert(dev); + return pci_bus_ops()->read8(dev->bus->secondary, dev->path.pci.devfn, where); }
u16 pci_read_config16(struct device *dev, unsigned int where) { - struct bus *pbus = get_pbus(dev); - return pci_bus_ops(pbus, dev)->read16(dev->bus->secondary, + pcidev_assert(dev); + return pci_bus_ops()->read16(dev->bus->secondary, dev->path.pci.devfn, where); }
u32 pci_read_config32(struct device *dev, unsigned int where) { - struct bus *pbus = get_pbus(dev); - return pci_bus_ops(pbus, dev)->read32(dev->bus->secondary, + pcidev_assert(dev); + return pci_bus_ops()->read32(dev->bus->secondary, dev->path.pci.devfn, where); }
void pci_write_config8(struct device *dev, unsigned int where, u8 val) { - struct bus *pbus = get_pbus(dev); - pci_bus_ops(pbus, dev)->write8(dev->bus->secondary, + pcidev_assert(dev); + pci_bus_ops()->write8(dev->bus->secondary, dev->path.pci.devfn, where, val); }
void pci_write_config16(struct device *dev, unsigned int where, u16 val) { - struct bus *pbus = get_pbus(dev); - pci_bus_ops(pbus, dev)->write16(dev->bus->secondary, + pcidev_assert(dev); + pci_bus_ops()->write16(dev->bus->secondary, dev->path.pci.devfn, where, val); }
void pci_write_config32(struct device *dev, unsigned int where, u32 val) { - struct bus *pbus = get_pbus(dev); - pci_bus_ops(pbus, dev)->write32(dev->bus->secondary, + pcidev_assert(dev); + pci_bus_ops()->write32(dev->bus->secondary, dev->path.pci.devfn, where, val); } diff --git a/src/include/device/device.h b/src/include/device/device.h index 3b38322..4c67064 100644 --- a/src/include/device/device.h +++ b/src/include/device/device.h @@ -68,7 +68,6 @@ const struct i2c_bus_operations *ops_i2c_bus; const struct spi_bus_operations *ops_spi_bus; const struct smbus_bus_operations *ops_smbus_bus; - const struct pci_bus_operations * (*ops_pci_bus)(struct device *dev); const struct pnp_mode_ops *ops_pnp_mode; };
-
Aaron Durbin (Code Review) -
Kyösti Mälkki (Code Review)
-
Nico Huber (Code Review)