[coreboot] r983 - in coreboot-v3: device include/device
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Thu Nov 6 11:36:08 CET 2008
Hi Myles,
I'm sorry I didn't have time to review your patch a bit sooner.
AFAICS it introduced a few bugs regarding conditional compilation.
Review follows.
On 05.11.2008 23:27, svn at coreboot.org wrote:
> Author: myles
> Date: 2008-11-05 23:27:36 +0100 (Wed, 05 Nov 2008)
> New Revision: 983
>
> Modified:
> coreboot-v3/device/Makefile
> coreboot-v3/device/cardbus_device.c
> coreboot-v3/device/device.c
> coreboot-v3/device/pci_device.c
> coreboot-v3/device/pcie_device.c
> coreboot-v3/include/device/cardbus.h
> coreboot-v3/include/device/device.h
> coreboot-v3/include/device/pci.h
> coreboot-v3/include/device/pcie.h
> Log:
> This patch continues the device code cleanup.
>
> The largest changes are to get_pci_bridge_ops, and related changes to make it
> compile and use correct declarations.
>
> While I was doing that I moved the checks for CONFIG_<BUS>_PLUGIN_SUPPORT to
> the Makefile.
>
> The only functional difference is a possible NULL dereference in a debug
> statement.
>
> I also added a few more consts, now that my other patch is in.
>
> Signed-off-by: Myles Watson <mylesgw at gmail.com>
> Acked-by: Ronald G. Minnich <rminnich at gmail.com>
>
>
> Modified: coreboot-v3/device/Makefile
> ===================================================================
> --- coreboot-v3/device/Makefile 2008-11-05 22:18:53 UTC (rev 982)
> +++ coreboot-v3/device/Makefile 2008-11-05 22:27:36 UTC (rev 983)
> @@ -29,13 +29,27 @@
> smbus_ops.c
>
> # this is only needed on the K8
> +# This could also check for CONFIG_HYPERTRANSPORT_PLUGIN_SUPPORT
> ifeq ($(CONFIG_NORTHBRIDGE_AMD_K8),y)
> STAGE2_DEVICE_SRC += hypertransport.c
> endif
>
> # this is only needed for pcix devices
> +# This should also check for CONFIG_PCIX_PLUGIN_SUPPORT
> ifeq ($(CONFIG_SOUTHBRIDGE_AMD_AMD8132),y)
> STAGE2_DEVICE_SRC += pcix_device.c
> endif
>
> +ifeq ($(CONFIG_PCIE_PLUGIN_SUPPORT),y)
> +STAGE2_DEVICE_SRC += pcie_device.c
> +endif
> +
> +ifeq ($(CONFIG_CARDBUS_PLUGIN_SUPPORT),y)
> +STAGE2_DEVICE_SRC += cardbus_device.c
> +endif
> +
> +ifeq ($(CONFIG_AGP_PLUGIN_SUPPORT),y)
> +STAGE2_DEVICE_SRC += agp_device.c
> +endif
> +
> $(obj)/device/pci_device.o: $(src)/device/pci_device.c $(obj)/statictree.h
> Modified: coreboot-v3/device/pci_device.c
> ===================================================================
> --- coreboot-v3/device/pci_device.c 2008-11-05 22:18:53 UTC (rev 982)
> +++ coreboot-v3/device/pci_device.c 2008-11-05 22:27:36 UTC (rev 983)
> @@ -30,28 +30,7 @@
> #include <device/device.h>
> #include <device/pci.h>
> #include <device/pci_ids.h>
> -/* We should move these so they're really config options */
> -#define CONFIG_HYPERTRANSPORT_PLUGIN_SUPPORT 0
> -#define CONFIG_PCIX_PLUGIN_SUPPORT 0
> -#define CONFIG_PCIE_PLUGIN_SUPPORT 0
> -#define CONFIG_CARDBUS_PLUGIN_SUPPORT 0
> -#define CONFIG_AGP_PLUGIN_SUPPORT 0
>
> -#if CONFIG_HYPERTRANSPORT_PLUGIN_SUPPORT == 1
> -#include <device/hypertransport.h>
> -#endif
> -#if CONFIG_PCIX_PLUGIN_SUPPORT == 1
> -#include <device/pcix.h>
> -#endif
> -#if CONFIG_PCIE_PLUGIN_SUPPORT == 1
> -#include <device/pcie.h>
> -#endif
> -#if CONFIG_AGP_PLUGIN_SUPPORT == 1
> -#include <device/agp.h>
> -#endif
> -#if CONFIG_CARDBUS_PLUGIN_SUPPORT == 1
> -#include <device/cardbus.h>
> -#endif
> #include <statictree.h>
>
> u8 pci_moving_config8(struct device *dev, unsigned int reg)
> @@ -801,26 +780,28 @@
> * @param dev Pointer to the device structure of the bridge.
> * @return Appropriate bridge operations.
> */
> -static struct device_operations *get_pci_bridge_ops(struct device *dev)
> +static const struct device_operations *get_pci_bridge_ops(struct device *dev)
> {
> - // unsigned int pos;
> -
> -#if CONFIG_PCIX_PLUGIN_SUPPORT == 1
> - pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
> - if (pos) {
> +#ifdef DEVICE_PCIX_H
>
That won't work the way you want it to work. The ifdef above will never
be true.
> + unsigned int pcix_pos;
> + pcix_pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
> + if (pcix_pos) {
> printk(BIOS_DEBUG, "%s subordinate bus PCI-X\n",
> dev_path(dev));
> return &default_pcix_ops_bus;
> }
> #endif
> -#if CONFIG_AGP_PLUGIN_SUPPORT == 1
> +#ifdef DEVICE_AGP_H
>
Same here. The ifdef will never be true.
> /* How do I detect an PCI to AGP bridge? */
> +#warning AGP detection not implemented, so AGP bridge plugin not supported.
> +
> #endif
> -#if CONFIG_HYPERTRANSPORT_PLUGIN_SUPPORT == 1
> - pos = 0;
> - while ((pos = pci_find_next_capability(dev, PCI_CAP_ID_HT, pos))) {
> +#ifdef DEVICE_HYPERTRANSPORT_H
>
And here.
> + unsigned int ht_pos;
> + ht_pos = 0;
> + while ((ht_pos = pci_find_next_capability(dev, PCI_CAP_ID_HT, ht_pos))) {
> unsigned int flags;
> - flags = pci_read_config16(dev, pos + PCI_CAP_FLAGS);
> + flags = pci_read_config16(dev, ht_pos + PCI_CAP_FLAGS);
> if ((flags >> 13) == 1) {
> /* Host or Secondary Interface. */
> printk(BIOS_DEBUG,
> @@ -830,11 +811,12 @@
> }
> }
> #endif
> -#if CONFIG_PCIE_PLUGIN_SUPPORT == 1
> - pos = pci_find_capability(dev, PCI_CAP_ID_PCIE);
> - if (pos) {
> +#ifdef DEVICE_PCIE_H
>
And here.
> + unsigned int pcie_pos;
> + pcie_pos = pci_find_capability(dev, PCI_CAP_ID_PCIE);
> + if (pcie_pos) {
> unsigned int flags;
> - flags = pci_read_config16(dev, pos + PCI_EXP_FLAGS);
> + flags = pci_read_config16(dev, pcie_pos + PCI_EXP_FLAGS);
> switch ((flags & PCI_EXP_FLAGS_TYPE) >> 4) {
> case PCI_EXP_TYPE_ROOT_PORT:
> case PCI_EXP_TYPE_UPSTREAM:
> @@ -864,7 +846,6 @@
> static void set_pci_ops(struct device *dev)
> {
> struct device_operations *c;
> - struct device_id id;
>
> if (dev->ops) {
> printk(BIOS_SPEW, "%s: dev %s already has ops of type %x\n",
> @@ -890,21 +869,30 @@
> switch (dev->hdr_type & 0x7f) { /* Header type. */
> case PCI_HEADER_TYPE_NORMAL: /* Standard header. */
> if ((dev->class >> 8) == PCI_CLASS_BRIDGE_PCI)
> - goto bad;
> - dev->ops = &default_pci_ops_dev;
> + printk(BIOS_ERR,
> + "%s [%s] hdr_type %02x doesn't match"
> + "class %06x, ignoring.\n", dev_path(dev),
> + dev_id_string(&dev->id), dev->class >> 8,
> + dev->hdr_type);
> + else
> + dev->ops = &default_pci_ops_dev;
> break;
> case PCI_HEADER_TYPE_BRIDGE:
> if ((dev->class >> 8) != PCI_CLASS_BRIDGE_PCI)
> - goto bad;
> - dev->ops = get_pci_bridge_ops(dev);
> + printk(BIOS_ERR,
> + "%s [%s] hdr_type %02x doesn't match"
> + "class %06x, ignoring.\n", dev_path(dev),
> + dev_id_string(&dev->id), dev->class >> 8,
> + dev->hdr_type);
> + else
> + dev->ops = get_pci_bridge_ops(dev);
> break;
> -#if CONFIG_CARDBUS_PLUGIN_SUPPORT == 1
> +#ifdef DEVICE_CARDBUS_H
>
And here.
> case PCI_HEADER_TYPE_CARDBUS:
> dev->ops = &default_cardbus_ops_bus;
> break;
> #endif
> default:
> - bad:
> if (dev->enabled) {
> printk(BIOS_ERR,
> "%s [%s/%06x] has unknown header "
>
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
More information about the coreboot
mailing list