Re: [coreboot] r983 - in coreboot-v3: device include/device

[Carl-Daniel Hailfinger]

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, [e-mail] 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 
> Acked-by: Ronald G. Minnich
> 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