[PATCH] simplify coreboot PCI code

[Stefan Reinauer]

See patch

Simplify coreboot PCI handling

This patch drops the conf1/conf2 autodetection and replaces it by 
(usually northbridge specific) hardcodes.

This patch also adds pci_domain_init() which needs to be called by
mainboard enable_dev() functions in order to be able to use the pci
config space functions. This allows to drop i386 specific code from
generic files again...

There is an even better approach to the PCI config space access in mainboard
specific init files problem, but that should go into another patch:

static void init(struct device *dev)
{
    // Do the stuff here!
}

static void enable_dev(struct device *dev)
{
   // Install an init function for this mainboard device
   dev->ops->init = init;
}

struct chip_operations mainboard_ops = {
  .enable_dev = enable_dev,
};

Signed-off-by: Stefan Reinauer

Index: src/southbridge/amd/rs690/rs690.c
===================================================================
--- src/southbridge/amd/rs690/rs690.c	(revision 4681)
+++ src/southbridge/amd/rs690/rs690.c	(working copy)
@@ -37,34 +37,33 @@
 {
    u32 reg;
    u16 word;
-	/* u8 byte; */
-	struct bus pbus; /* fake bus for dev0 fun1 */
+	device_t nb2_dev = dev_find_slot(0, PCI_DEVFN(0, 1));

reg = pci_read_config32(nb_dev, 0x4c);
    reg |= 1 << 0;
    pci_write_config32(nb_dev, 0x4c, reg);

-	word = pci_cf8_conf1.read16(&pbus, 0, 1, 0xf8);
+	word = pci_read_config16(nb2_dev, 0xf8);
    word &= 0xf00;
-	pci_cf8_conf1.write16(&pbus, 0, 1, 0xf8, word);
+	pci_write_config16(nb2_dev, 0xf8, word);

-	word = pci_cf8_conf1.read16(&pbus, 0, 1, 0xe8);
+	word = pci_read_config16(nb2_dev, 0xe8);
    word &= ~((1 << 12) | (1 << 13) | (1 << 14));
    word |= 1 << 13;
-	pci_cf8_conf1.write16(&pbus, 0, 1, 0xe8, word);
+	pci_write_config16(nb2_dev, 0xe8, word);

-	reg =  pci_cf8_conf1.read32(&pbus, 0, 1, 0x94);
+	reg = pci_read_config32(nb2_dev, 0x94);
    reg &= ~((1 << 16) | (1 << 24) | (1 << 28));
-	pci_cf8_conf1.write32(&pbus, 0, 1, 0x94, reg);
+	pci_write_config32(nb2_dev, 0x94, reg);

-	reg = pci_cf8_conf1.read32(&pbus, 0, 1, 0x8c);
+	reg = pci_read_config32(nb2_dev, 0x8c);
    reg &= ~((1 << 13) | (1 << 14) | (1 << 24) | (1 << 25));
    reg |= 1 << 13;
-	pci_cf8_conf1.write32(&pbus, 0, 1, 0x8c, reg);
+	pci_write_config32(nb2_dev, 0x8c, reg);

-	reg = pci_cf8_conf1.read32(&pbus, 0, 1, 0xcc);
+	reg = pci_read_config32(nb2_dev, 0xcc);
    reg |= 1 << 24;
-	pci_cf8_conf1.write32(&pbus, 0, 1, 0xcc, reg);
+	pci_write_config32(nb2_dev, 0xcc, reg);

reg = nbmc_read_index(nb_dev, 0x7a);
    reg &= ~0x3f;
@@ -72,26 +71,28 @@
    reg &= ~(1 << 6);
    set_htiu_enable_bits(nb_dev, 0x05, 1 << 11, 1 << 11);
    nbmc_write_index(nb_dev, 0x7a, reg);
+
    /* Powering Down efuse and strap block clocks after boot-up. GFX Mode. */
-	reg = pci_cf8_conf1.read32(&pbus, 0, 1, 0xcc);
+	reg = pci_read_config32(nb2_dev, 0xcc);
    reg &= ~(1 << 23);
-	reg |= 1 << 24;
-	pci_cf8_conf1.write32(&pbus, 0, 1, 0xcc, reg);
+	reg |= 1 << 24; // already set?
+	pci_write_config32(nb2_dev, 0xcc, reg);
+
 #if 0
    /* Powerdown reference clock to graphics core PLL in northbridge only mode */
-	reg = pci_cf8_conf1.read32(&pbus, 0, 1, 0x8c);
+	reg = pci_read_config32(nb2_dev, 0x8c);
    reg |= 1 << 21;
-	pci_cf8_conf1.write32(&pbus, 0, 1, 0x8c, reg);
+	pci_write_config32(nb2_dev, 0x8c, reg);

/* Powering Down efuse and strap block clocks after boot-up. NB Only Mode. */
-	reg = pci_cf8_conf1.read32(&pbus, 0, 1, 0xcc);
+	reg = pci_read_config32(nb2_dev, 0xcc);
    reg |= (1 << 23) | (1 << 24);
-	pci_cf8_conf1.write32(&pbus, 0, 1, 0xcc, reg);
+	pci_write_config32(nb2_dev, 0xcc, reg);

/* Powerdown clock to memory controller in northbridge only mode */
-	byte = pci_cf8_conf1.read8(&pbus, 0, 1, 0xe4);
+	byte = pci_read_config8(nb2_dev, 0xe4);
    byte |= 1 << 0;
-	pci_cf8_conf1.write8(&pbus, 0, 1, 0xe4, reg);
+	pci_write_config8(nb2_dev, 0xe4, reg);

/* CLKCFG:0xE8 Bit[17] = 0x1 	 Powerdown clock to IOC GFX block in no external graphics mode */
    /* TODO: */
Index: src/southbridge/amd/rs690/rs690_pcie.c
===================================================================
--- src/southbridge/amd/rs690/rs690_pcie.c	(revision 4681)
+++ src/southbridge/amd/rs690/rs690_pcie.c	(working copy)
@@ -361,7 +361,7 @@
 void pcie_config_misc_clk(device_t nb_dev)
 {
    u32 reg;
-	struct bus pbus; /* fake bus for dev0 fun1 */
+	device_t nb2_dev = dev_find_slot(0, PCI_DEVFN(0, 1));

reg = pci_read_config32(nb_dev, 0x4c);
    reg |= 1 << 0;
@@ -374,9 +374,9 @@
    	set_pcie_enable_bits(nb_dev, 0x11 | PCIE_CORE_INDEX_GFX, (3 << 6) | (~0xf), 3 << 6);

/* LCLK Clock Gating */
-		reg =  pci_cf8_conf1.read32(&pbus, 0, 1, 0x94);
+		reg =  pci_read_config32(nb2_dev, 0x94);
    	reg &= ~(1 << 16);
-		pci_cf8_conf1.write32(&pbus, 0, 1, 0x94, reg);
+		pci_write_config32(nb2_dev, 0x94, reg);
    }

if (AtiPcieCfg.Config & PCIE_GPP_CLK_GATING) {
@@ -386,9 +386,9 @@
    	set_pcie_enable_bits(nb_dev, 0x11 | PCIE_CORE_INDEX_GPPSB, (3 << 6) | (~0xf), 3 << 6);

/* LCLK Clock Gating */
-		reg =  pci_cf8_conf1.read32(&pbus, 0, 1, 0x94);
+		reg =  pci_read_config32(nb2_dev, 0x94);
    	reg &= ~(1 << 24);
-		pci_cf8_conf1.write32(&pbus, 0, 1, 0x94, reg);
+		pci_write_config32(nb2_dev, 0x94, reg);
    }

reg = pci_read_config32(nb_dev, 0x4c);
Index: src/southbridge/intel/i82801gx/i82801gx_pci.c
===================================================================
--- src/southbridge/intel/i82801gx/i82801gx_pci.c	(revision 4681)
+++ src/southbridge/intel/i82801gx/i82801gx_pci.c	(working copy)
@@ -68,7 +68,10 @@
    uint16_t command;

/* Set the subsystem vendor and device id for mainboard devices */
-	ops = ops_pci(dev);
+	if (dev && dev->ops) {
+		ops = dev->ops->ops_pci;
+	}
+
    if (dev->on_mainboard && ops && ops->set_subsystem) {
    	printk_debug("%s subsystem <- %02x/%02x\n",
    		dev_path(dev),
Index: src/devices/root_device.c
===================================================================
--- src/devices/root_device.c	(revision 4681)
+++ src/devices/root_device.c	(working copy)
@@ -174,7 +174,7 @@
  * should be fully usable as is.  However the chip_operations::enable_dev()
  * of a motherboard can override this if you want non-default behavior.
  */
-struct device_operations default_dev_ops_root = {
+const struct device_operations default_dev_ops_root = {
    .read_resources   = root_dev_read_resources,
    .set_resources    = root_dev_set_resources,
    .enable_resources = root_dev_enable_resources,
@@ -183,10 +183,3 @@
    .reset_bus        = root_dev_reset,
 };

-/**
- * @brief The root of device tree.
- *
- * This is the root of the device tree. The device tree is defined in the
- * static.c file and is generated by config tool during compile time.
- */
-extern struct device dev_root;
Index: src/devices/pci_device.c
===================================================================
--- src/devices/pci_device.c	(revision 4681)
+++ src/devices/pci_device.c	(working copy)
@@ -602,11 +602,14 @@

void pci_dev_enable_resources(struct device *dev)
 {
-	const struct pci_operations *ops;
+	const struct pci_operations *ops = NULL;
    u16 command;

/* Set the subsystem vendor and device id for mainboard devices. */
-	ops = ops_pci(dev);
+	if (dev && dev->ops) {
+		ops = dev->ops->ops_pci;
+	}
+
    if (dev->on_mainboard && ops && ops->set_subsystem) {
    	printk_debug("%s subsystem <- %02x/%02x\n",
    		     dev_path(dev),
@@ -1199,6 +1202,31 @@
    return max;
 }

+/**
+ * @brief Initialize PCI domain for early PCI accesses.
+ *
+ * The PCI domain is a sub device of the mainboard device. Some mainboards use
+ * PCI accesses in their enable_dev instead of their .ops->init, and thus the PCI
+ * domain is not yet initialized. Such behavior is strictly speaking wrong,
+ * but also convenient. The behavior can be fixed by running pci_domain_init()
+ * before doing the actual PCI accesses.
+ *
+ * @param mainboard_dev mainboard device
+ */
+
+void pci_domain_init(device_t mainboard_dev)
+{
+	device_t dev = mainboard_dev->link[0].children;
+
+	while (dev && (dev->path.type != DEVICE_PATH_PCI_DOMAIN))
+		dev=dev->sibling;
+
+	if (dev)
+		dev->chip_ops->enable_dev(dev);
+}
+
+
+
 #if CONFIG_PC80_SYSTEM == 1
 /**
  * 
@@ -1224,7 +1252,9 @@
    device_t pdev;
    u8 line;
    u8 irq;
+#ifdef PARANOID_IRQ_ASSIGNMENTS
    u8 readback;
+#endif

/* Each slot may contain up to eight functions */
    for (funct = 0; funct < 8; funct++) {
Index: src/devices/pci_ops.c
===================================================================
--- src/devices/pci_ops.c	(revision 4681)
+++ src/devices/pci_ops.c	(working copy)
@@ -25,19 +25,27 @@
 #include <device/pci_ids.h>
 #include <device/pci_ops.h>

-/* The only consumer of the return value of get_pbus() is ops_pci_bus().
- * ops_pci_bus() can handle being passed NULL and auto-picks working ops.
- */
+static inline const struct pci_bus_operations *ops_pci_bus(struct bus *bus)
+{
+	const struct pci_bus_operations *bops;
+	bops = NULL;
+	if (bus && bus->dev && bus->dev->ops) {
+		bops = bus->dev->ops->ops_pci_bus;
+	}
+	return bops;
+}
+
 static struct bus *get_pbus(device_t dev)
 {
    struct bus *pbus = NULL;

if (!dev)
    	die("get_pbus: dev is NULL!\n");
-	else
-		pbus = dev->bus;

-	while(pbus && pbus->dev && !ops_pci_bus(pbus)) {
+	pbus = dev->bus;
+
+	while(pbus && pbus->dev && pbus->dev->ops && ! pbus->dev->ops->ops_pci_bus) {
+		/* TODO explain how such a loop can happen */
    	if (pbus == pbus->dev->bus) {
    		printk_alert("%s in endless loop looking for a parent "
    			"bus with ops_pci_bus for %s, breaking out.\n",
@@ -46,6 +54,7 @@
    	}
    	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 set up completely. */
    	//printk_emerg("%s: Cannot find pci bus operations.\n", dev_path(dev));
Index: src/include/device/pci_ops.h
===================================================================
--- src/include/device/pci_ops.h	(revision 4681)
+++ src/include/device/pci_ops.h	(working copy)
@@ -21,7 +21,4 @@
 void pci_mmio_write_config32(device_t dev, unsigned where, uint32_t val);
 #endif

-/* This function lives in pci_ops_auto.c */
-const struct pci_bus_operations *pci_remember_direct(void);
-
 #endif /* PCI_OPS_H */
Index: src/include/device/device.h
===================================================================
--- src/include/device/device.h	(revision 4681)
+++ src/include/device/device.h	(working copy)
@@ -90,6 +90,12 @@
    void *chip_info;
 };

+/**
+ * @brief The root of device tree.
+ *
+ * This is the root of the device tree. The device tree is defined in the
+ * static.c file and is generated by config tool during compile time.
+ */
 extern struct device	dev_root;	/* root bus */
 extern struct device	*all_devices;	/* list of all devices */

@@ -138,9 +144,10 @@
 #define DEVICE_IO_ALIGN 16 
 #define DEVICE_MEM_ALIGN 4096

-extern struct device_operations default_dev_ops_root;
+extern const struct device_operations default_dev_ops_root;
 void pci_domain_read_resources(struct device *dev);
 unsigned int pci_domain_scan_bus(struct device *dev, unsigned int max);
+void pci_domain_init(device_t mainboard_dev);
 void root_dev_read_resources(device_t dev);
 void root_dev_set_resources(device_t dev);
 unsigned int scan_static_bus(device_t bus, unsigned int max);
Index: src/include/device/pci.h
===================================================================
--- src/include/device/pci.h	(revision 4681)
+++ src/include/device/pci.h	(working copy)
@@ -80,26 +80,4 @@
 #define PCI_IO_BRIDGE_ALIGN 4096
 #define PCI_MEM_BRIDGE_ALIGN (1024*1024)

-static inline const struct pci_operations *ops_pci(device_t dev)
-{
-	const struct pci_operations *pops;
-	pops = 0;
-	if (dev && dev->ops) {
-		pops = dev->ops->ops_pci;
-	}
-	return pops;
-}
-
-static inline const struct pci_bus_operations *ops_pci_bus(struct bus *bus)
-{
-	const struct pci_bus_operations *bops;
-	bops = 0;
-	if (bus && bus->dev && bus->dev->ops) {
-		bops = bus->dev->ops->ops_pci_bus;
-	}
-	if (!bops)
-		bops = pci_remember_direct();
-	return bops;
-}
-
 #endif /* PCI_H */
Index: src/cpu/amd/sc520/sc520.c
===================================================================
--- src/cpu/amd/sc520/sc520.c	(revision 4681)
+++ src/cpu/amd/sc520/sc520.c	(working copy)
@@ -215,6 +215,7 @@
         .enable_resources = 0, //enable_resources,
         .init             = 0,
         .scan_bus         = pci_domain_scan_bus,
+	.ops_pci_bus	  = &pci_cf8_conf1,
 };

#if 0
@@ -242,7 +243,6 @@
         /* Set the operations if it is a special bus type */
         if (dev->path.type == DEVICE_PATH_PCI_DOMAIN) {
                 dev->ops = &pci_domain_ops;
-		pci_set_method(dev);
         }
 #if 0
    /* This is never hit as none of the sc520 boards have
Index: src/cpu/emulation/qemu-x86/northbridge.c
===================================================================
--- src/cpu/emulation/qemu-x86/northbridge.c	(revision 4681)
+++ src/cpu/emulation/qemu-x86/northbridge.c	(working copy)
@@ -141,6 +141,7 @@
    .enable_resources	= enable_childrens_resources,
    .init			= 0,
    .scan_bus		= pci_domain_scan_bus,
+	.ops_pci_bus		= &pci_cf8_conf1,
 };

static void enable_dev(struct device *dev)
@@ -148,7 +149,6 @@
    /* Set the operations if it is a special bus type */
    if (dev->path.type == DEVICE_PATH_PCI_DOMAIN) {
    	dev->ops = &pci_domain_ops;
-		pci_set_method(dev);
    }
 }

Index: src/mainboard/amd/pistachio/mainboard.c
===================================================================
--- src/mainboard/amd/pistachio/mainboard.c	(revision 4681)
+++ src/mainboard/amd/pistachio/mainboard.c	(working copy)
@@ -263,6 +263,8 @@

printk_info("Mainboard Pistachio Enable. dev=0x%p\n", dev);

+	pci_domain_init(dev);
+
 #if (CONFIG_GFXUMA == 1)
    msr_t msr, msr2;

Index: src/mainboard/amd/dbm690t/mainboard.c
===================================================================
--- src/mainboard/amd/dbm690t/mainboard.c	(revision 4681)
+++ src/mainboard/amd/dbm690t/mainboard.c	(working copy)
@@ -180,7 +180,6 @@
     * pm_iowrite(0x67, byte);
     */
 }
-
 /*************************************************
 * enable the dedicated function in dbm690t board.
 * This function called early than rs690_enable.
@@ -192,6 +191,8 @@

printk_info("Mainboard DBM690T Enable. dev=0x%p\n", dev);

+	pci_domain_init(dev);
+
 #if (CONFIG_GFXUMA == 1)
    msr_t msr, msr2;

Index: src/mainboard/technexion/tim8690/mainboard.c
===================================================================
--- src/mainboard/technexion/tim8690/mainboard.c	(revision 4681)
+++ src/mainboard/technexion/tim8690/mainboard.c	(working copy)
@@ -155,6 +155,8 @@

printk_info("Mainboard tim8690 Enable. dev=0x%p\n", dev);

+	pci_domain_init(dev);
+
 #if (CONFIG_GFXUMA == 1)
    msr_t msr, msr2;

Index: src/northbridge/via/cx700/northbridge.c
===================================================================
--- src/northbridge/via/cx700/northbridge.c	(revision 4681)
+++ src/northbridge/via/cx700/northbridge.c	(working copy)
@@ -137,6 +137,7 @@
    .enable_resources = enable_childrens_resources,
    .init		  = 0,
    .scan_bus	  = pci_domain_scan_bus,
+	.ops_pci_bus	  = &pci_cf8_conf1,
 };

static void cpu_bus_init(device_t dev)
@@ -161,7 +162,6 @@
    /* Our wonderful device model */
    if (dev->path.type == DEVICE_PATH_PCI_DOMAIN) {
    	dev->ops = &pci_domain_ops;
-		pci_set_method(dev);
    } else if (dev->path.type == DEVICE_PATH_APIC_CLUSTER) {
    	dev->ops = &cpu_bus_ops;
    }
Index: src/northbridge/via/cn400/northbridge.c
===================================================================
--- src/northbridge/via/cn400/northbridge.c	(revision 4681)
+++ src/northbridge/via/cn400/northbridge.c	(working copy)
@@ -284,6 +284,7 @@
    .enable_resources = enable_childrens_resources,
    .init             = 0,
    .scan_bus         = cn400_domain_scan_bus,
+	.ops_pci_bus	  = &pci_cf8_conf1,
 };

static void cpu_bus_init(device_t dev)
@@ -310,7 +311,6 @@
    /* Set the operations if it is a special bus type. */
    if (dev->path.type == DEVICE_PATH_PCI_DOMAIN) {
    	dev->ops = &pci_domain_ops;
-		pci_set_method(dev);
    } else if (dev->path.type == DEVICE_PATH_APIC_CLUSTER) {
    	dev->ops = &cpu_bus_ops;
    }
Index: src/northbridge/via/vx800/northbridge.c
===================================================================
--- src/northbridge/via/vx800/northbridge.c	(revision 4681)
+++ src/northbridge/via/vx800/northbridge.c	(working copy)
@@ -185,6 +185,7 @@
    .enable_resources = enable_childrens_resources,
    .init = 0,
    .scan_bus = pci_domain_scan_bus,
+	.ops_pci_bus	  = &pci_cf8_conf1,
 };

static void cpu_bus_init(device_t dev)
@@ -211,7 +212,6 @@
    /* Set the operations if it is a special bus type */
    if (dev->path.type == DEVICE_PATH_PCI_DOMAIN) {
    	dev->ops = &pci_domain_ops;
-		pci_set_method(dev);
    } else if (dev->path.type == DEVICE_PATH_APIC_CLUSTER) {
    	dev->ops = &cpu_bus_ops;
    }
Index: src/northbridge/via/cn700/northbridge.c
===================================================================
--- src/northbridge/via/cn700/northbridge.c	(revision 4681)
+++ src/northbridge/via/cn700/northbridge.c	(working copy)
@@ -208,6 +208,7 @@
    .enable_resources = enable_childrens_resources,
    .init             = 0,
    .scan_bus         = pci_domain_scan_bus,
+	.ops_pci_bus	  = &pci_cf8_conf1,
 };

static void cpu_bus_init(device_t dev)
@@ -234,7 +235,6 @@
    /* Set the operations if it is a special bus type. */
    if (dev->path.type == DEVICE_PATH_PCI_DOMAIN) {
    	dev->ops = &pci_domain_ops;
-		pci_set_method(dev);
    } else if (dev->path.type == DEVICE_PATH_APIC_CLUSTER) {
    	dev->ops = &cpu_bus_ops;
    }
Index: src/northbridge/via/vt8601/northbridge.c
===================================================================
--- src/northbridge/via/vt8601/northbridge.c	(revision 4681)
+++ src/northbridge/via/vt8601/northbridge.c	(working copy)
@@ -149,6 +149,7 @@
         .enable_resources = enable_childrens_resources,
         .init             = 0,
         .scan_bus         = pci_domain_scan_bus,
+	.ops_pci_bus	  = &pci_cf8_conf1,
 };

static void cpu_bus_init(device_t dev)
@@ -173,7 +174,6 @@
         /* Set the operations if it is a special bus type */
         if (dev->path.type == DEVICE_PATH_PCI_DOMAIN) {
                 dev->ops = &pci_domain_ops;
-		pci_set_method(dev);
         }
         else if (dev->path.type == DEVICE_PATH_APIC_CLUSTER) {
                 dev->ops = &cpu_bus_ops;
Index: src/northbridge/via/vt8623/northbridge.c
===================================================================
--- src/northbridge/via/vt8623/northbridge.c	(revision 4681)
+++ src/northbridge/via/vt8623/northbridge.c	(working copy)
@@ -295,6 +295,7 @@
         .enable_resources = enable_childrens_resources,
         .init             = 0,
         .scan_bus         = pci_domain_scan_bus,
+	.ops_pci_bus	  = &pci_cf8_conf1,
 };

static void cpu_bus_init(device_t dev)
@@ -321,7 +322,6 @@
         /* Set the operations if it is a special bus type */
         if (dev->path.type == DEVICE_PATH_PCI_DOMAIN) {
                 dev->ops = &pci_domain_ops;
-		pci_set_method(dev);
         }
         else if (dev->path.type == DEVICE_PATH_APIC_CLUSTER) {
                 dev->ops = &cpu_bus_ops;
Index: src/northbridge/amd/gx1/northbridge.c
===================================================================
--- src/northbridge/amd/gx1/northbridge.c	(revision 4681)
+++ src/northbridge/amd/gx1/northbridge.c	(working copy)
@@ -172,6 +172,7 @@
         .enable_resources = enable_childrens_resources,
         .init             = 0,
         .scan_bus         = pci_domain_scan_bus,
+	.ops_pci_bus	  = &pci_cf8_conf1,
 };

static void cpu_bus_init(device_t dev)
@@ -199,7 +200,6 @@
         if (dev->path.type == DEVICE_PATH_PCI_DOMAIN) {
         	printk_spew("DEVICE_PATH_PCI_DOMAIN\n");
                 dev->ops = &pci_domain_ops;
-		pci_set_method(dev);
         }
         else if (dev->path.type == DEVICE_PATH_APIC_CLUSTER) {
         	printk_spew("DEVICE_PATH_APIC_CLUSTER\n");
Index: src/northbridge/amd/gx2/northbridge.c
===================================================================
--- src/northbridge/amd/gx2/northbridge.c	(revision 4681)
+++ src/northbridge/amd/gx2/northbridge.c	(working copy)
@@ -455,6 +455,7 @@
         .enable_resources = enable_childrens_resources,
         .init             = 0,
         .scan_bus         = pci_domain_scan_bus,
+	.ops_pci_bus	  = &pci_cf8_conf1,
 };

static void cpu_bus_init(device_t dev)
@@ -501,7 +502,6 @@
    	do_vsmbios();
    	graphics_init();
    	dev->ops = &pci_domain_ops;
-		pci_set_method(dev);
    	tomk = ((sizeram() - VIDEO_MB) * 1024) - SMM_SIZE;
 #if CONFIG_HAVE_HIGH_TABLES==1
    	/* Leave some space for ACPI, PIRQ and MP tables */
Index: src/northbridge/amd/lx/northbridge.c
===================================================================
--- src/northbridge/amd/lx/northbridge.c	(revision 4681)
+++ src/northbridge/amd/lx/northbridge.c	(working copy)
@@ -447,7 +447,6 @@
    // print_conf();

graphics_init();
-	pci_set_method(dev);
 }

static struct device_operations pci_domain_ops = {
@@ -456,6 +455,7 @@
    .enable_resources = enable_childrens_resources,
    .scan_bus = pci_domain_scan_bus,
    .enable = pci_domain_enable,
+	.ops_pci_bus	  = &pci_cf8_conf1,
 };

static void cpu_bus_init(device_t dev)
Index: src/northbridge/intel/i855gme/northbridge.c
===================================================================
--- src/northbridge/intel/i855gme/northbridge.c	(revision 4681)
+++ src/northbridge/intel/i855gme/northbridge.c	(working copy)
@@ -144,6 +144,7 @@
         .enable_resources = enable_childrens_resources,
         .init             = 0,
         .scan_bus         = pci_domain_scan_bus,
+	.ops_pci_bus	  = &pci_cf8_conf1,
 };

static void cpu_bus_init(device_t dev)
@@ -170,7 +171,6 @@
         /* Set the operations if it is a special bus type */
         if (dev->path.type == DEVICE_PATH_PCI_DOMAIN) {
                 dev->ops = &pci_domain_ops;
-		pci_set_method(dev);
         }
         else if (dev->path.type == DEVICE_PATH_APIC_CLUSTER) {
                 dev->ops = &cpu_bus_ops;
Index: src/northbridge/intel/i855pm/northbridge.c
===================================================================
--- src/northbridge/intel/i855pm/northbridge.c	(revision 4681)
+++ src/northbridge/intel/i855pm/northbridge.c	(working copy)
@@ -112,6 +112,7 @@
         .enable_resources = enable_childrens_resources,
         .init             = 0,
         .scan_bus         = pci_domain_scan_bus,
+	.ops_pci_bus	  = &pci_cf8_conf1,
 };

static void cpu_bus_init(device_t dev)
@@ -138,7 +139,6 @@
         /* Set the operations if it is a special bus type */
         if (dev->path.type == DEVICE_PATH_PCI_DOMAIN) {
                 dev->ops = &pci_domain_ops;
-		pci_set_method(dev);
         }
         else if (dev->path.type == DEVICE_PATH_APIC_CLUSTER) {
                 dev->ops = &cpu_bus_ops;
Index: src/northbridge/intel/i440bx/northbridge.c
===================================================================
--- src/northbridge/intel/i440bx/northbridge.c	(revision 4681)
+++ src/northbridge/intel/i440bx/northbridge.c	(working copy)
@@ -128,6 +128,7 @@
    .enable_resources	= enable_childrens_resources,
    .init			= 0,
    .scan_bus		= pci_domain_scan_bus,
+	.ops_pci_bus	  = &pci_cf8_conf1,
 };

static void cpu_bus_init(device_t dev)
@@ -152,9 +153,7 @@
    /* Set the operations if it is a special bus type */
    if (dev->path.type == DEVICE_PATH_PCI_DOMAIN) {
    	dev->ops = &pci_domain_ops;
-		pci_set_method(dev);
-	}
-	else if (dev->path.type == DEVICE_PATH_APIC_CLUSTER) {
+	} else if (dev->path.type == DEVICE_PATH_APIC_CLUSTER) {
    	dev->ops = &cpu_bus_ops;
    }
 }
Index: src/northbridge/intel/i82810/northbridge.c
===================================================================
--- src/northbridge/intel/i82810/northbridge.c	(revision 4681)
+++ src/northbridge/intel/i82810/northbridge.c	(working copy)
@@ -187,6 +187,7 @@
    .enable_resources	= enable_childrens_resources,
    .init			= 0,
    .scan_bus		= pci_domain_scan_bus,
+	.ops_pci_bus	  = &pci_cf8_conf1,
 };

static void cpu_bus_init(device_t dev)
@@ -213,7 +214,6 @@
    /* Set the operations if it is a special bus type */
    if (dev->path.type == DEVICE_PATH_PCI_DOMAIN) {
    	dev->ops = &pci_domain_ops;
-		pci_set_method(dev);
    } else if (dev->path.type == DEVICE_PATH_APIC_CLUSTER) {
    	dev->ops = &cpu_bus_ops;
    }
Index: src/northbridge/intel/i82830/northbridge.c
===================================================================
--- src/northbridge/intel/i82830/northbridge.c	(revision 4681)
+++ src/northbridge/intel/i82830/northbridge.c	(working copy)
@@ -145,6 +145,7 @@
    .enable_resources	= enable_childrens_resources,
    .init			= 0,
    .scan_bus		= pci_domain_scan_bus,
+	.ops_pci_bus		= &pci_cf8_conf1,
 };

static void cpu_bus_init(device_t dev)
@@ -171,7 +172,6 @@
    /* Set the operations if it is a special bus type. */
    if (dev->path.type == DEVICE_PATH_PCI_DOMAIN) {
    	dev->ops = &pci_domain_ops;
-		pci_set_method(dev);
    } else if (dev->path.type == DEVICE_PATH_APIC_CLUSTER) {
    	dev->ops = &cpu_bus_ops;
    }
Index: src/arch/i386/include/arch/pci_ops.h
===================================================================
--- src/arch/i386/include/arch/pci_ops.h	(revision 4681)
+++ src/arch/i386/include/arch/pci_ops.h	(working copy)
@@ -8,6 +8,4 @@
 extern const struct pci_bus_operations pci_ops_mmconf;
 #endif

-void pci_set_method(device_t dev);
-
 #endif /* ARCH_I386_PCI_OPS_H */
Index: src/arch/i386/lib/Makefile.inc
===================================================================
--- src/arch/i386/lib/Makefile.inc	(revision 4681)
+++ src/arch/i386/lib/Makefile.inc	(working copy)
@@ -3,7 +3,6 @@
 obj-y += pci_ops_conf1.o
 obj-y += pci_ops_conf2.o
 obj-y += pci_ops_mmconf.o
-obj-y += pci_ops_auto.o
 obj-y += exception.o

initobj-y += printk_init.o
Index: src/arch/i386/lib/Config.lb
===================================================================
--- src/arch/i386/lib/Config.lb	(revision 4681)
+++ src/arch/i386/lib/Config.lb	(working copy)
@@ -8,7 +8,6 @@
 object pci_ops_conf1.c
 object pci_ops_conf2.c
 object pci_ops_mmconf.c
-object pci_ops_auto.c
 object exception.c

initobject printk_init.o
Index: src/arch/i386/lib/pci_ops_auto.c
===================================================================
--- src/arch/i386/lib/pci_ops_auto.c	(revision 4681)
+++ src/arch/i386/lib/pci_ops_auto.c	(working copy)
@@ -1,100 +0,0 @@
-#include <stddef.h>
-#include <console/console.h>
-#include <arch/io.h>
-#include <arch/pciconf.h>
-#include <device/pci.h>
-#include <device/pci_ids.h>
-#include <device/pci_ops.h>
-
-/*
- * Before we decide to use direct hardware access mechanisms, we try to do some
- * trivial checks to ensure it at least _seems_ to be working -- we just test
- * whether bus 00 contains a host bridge (this is similar to checking
- * techniques used in XFree86, but ours should be more reliable since we
- * attempt to make use of direct access hints provided by the PCI BIOS).
- *
- * This should be close to trivial, but it isn't, because there are buggy
- * chipsets (yes, you guessed it, by Intel and Compaq) that have no class ID.
- */
-static int pci_sanity_check(const struct pci_bus_operations *o)
-{
-	uint16_t class, vendor;
-	unsigned bus;
-	int devfn;
-	struct bus pbus; /* Dummy device */
-#define PCI_CLASS_BRIDGE_HOST		0x0600
-#define PCI_CLASS_DISPLAY_VGA		0x0300
-#define PCI_VENDOR_ID_COMPAQ		0x0e11
-#define PCI_VENDOR_ID_INTEL		0x8086
-#define PCI_VENDOR_ID_MOTOROLA		0x1057
-
-	for (bus = 0, devfn = 0; devfn < 0x100; devfn++) {
-		class = o->read16(&pbus, bus, devfn, PCI_CLASS_DEVICE);
-		vendor = o->read16(&pbus, bus, devfn, PCI_VENDOR_ID);
-		if (((class == PCI_CLASS_BRIDGE_HOST) || (class == PCI_CLASS_DISPLAY_VGA)) ||
-			((vendor == PCI_VENDOR_ID_INTEL) || (vendor == PCI_VENDOR_ID_COMPAQ) ||
-				(vendor == PCI_VENDOR_ID_MOTOROLA))) { 
-			return 1;
-		}
-	}
-	printk_err("PCI: Sanity check failed\n");
-	return 0;
-}
-
-struct pci_bus_operations *pci_bus_fallback_ops = NULL;
-
-const struct pci_bus_operations *pci_check_direct(void)
-{
-	unsigned int tmp;
-
-	/*
-	 * Check if configuration type 1 works.
-	 */
-	{
-		outb(0x01, 0xCFB);
-		tmp = inl(0xCF8);
-		outl(0x80000000, 0xCF8);
-		if ((inl(0xCF8) == 0x80000000) && 
-			pci_sanity_check(&pci_cf8_conf1)) 
-		{
-			outl(tmp, 0xCF8);
-			printk_debug("PCI: Using configuration type 1\n");
-			return &pci_cf8_conf1;
-		}
-		outl(tmp, 0xCF8);
-	}
-
-	/*
-	 * Check if configuration type 2 works.
-	 */
-	{
-		outb(0x00, 0xCFB);
-		outb(0x00, 0xCF8);
-		outb(0x00, 0xCFA);
-		if ((inb(0xCF8) == 0x00 && inb(0xCFA) == 0x00) &&
-			pci_sanity_check(&pci_cf8_conf2))
-		{
-			printk_debug("PCI: Using configuration type 2\n");
-			return &pci_cf8_conf2;
-		}
-	}
-
-	die("pci_check_direct failed\n");
-	return NULL;
-}
-
-const struct pci_bus_operations *pci_remember_direct(void)
-{
-	if (!pci_bus_fallback_ops)
-		pci_bus_fallback_ops = pci_check_direct();
-	return pci_bus_fallback_ops;
-}
-
-/** Set the method to be used for PCI, type I or type II
- */
-void pci_set_method(device_t dev)
-{
-	printk_info("Finding PCI configuration type.\n");
-	dev->ops->ops_pci_bus = pci_remember_direct();
-	post_code(0x5f);
-}

[Carl-Daniel Hailfinger]

On 27.09.2009 20:49, Stefan Reinauer wrote:
> Simplify coreboot PCI handling
>
> This patch drops the conf1/conf2 autodetection and replaces it by 
> (usually northbridge specific) hardcodes.
>
> This patch also adds pci_domain_init() which needs to be called by
> mainboard enable_dev() functions in order to be able to use the pci
> config space functions. This allows to drop i386 specific code from
> generic files again...
>
> There is an even better approach to the PCI config space access in mainboard
> specific init files problem, but that should go into another patch:
>
> static void init(struct device *dev)
> {
>    // Do the stuff here!
> }
>
> static void enable_dev(struct device *dev)
> {
>    // Install an init function for this mainboard device
>    dev->ops->init = init;
> }
>
> struct chip_operations mainboard_ops = {
>   .enable_dev = enable_dev,
> };
>
> Signed-off-by: Stefan Reinauer

I really like this patch. There are one or two things I don't understand
yet, but I hope stuff will become clearer tomorrow after testing. Will
ack after testing.

Regards,
Carl-Daniel

[Carl-Daniel Hailfinger]

On 27.09.2009 22:29, Carl-Daniel Hailfinger wrote:
> On 27.09.2009 20:49, Stefan Reinauer wrote:
> > Simplify coreboot PCI handling
> >
> > This patch drops the conf1/conf2 autodetection and replaces it by 
> > (usually northbridge specific) hardcodes.
> >
> > This patch also adds pci_domain_init() which needs to be called by
> > mainboard enable_dev() functions in order to be able to use the pci
> > config space functions. This allows to drop i386 specific code from
> > generic files again...
> >
> > There is an even better approach to the PCI config space access in mainboard
> > specific init files problem, but that should go into another patch:
> >
> > static void init(struct device *dev)
> > {
> >    // Do the stuff here!
> > }
> >
> > static void enable_dev(struct device *dev)
> > {
> >    // Install an init function for this mainboard device
> >    dev->ops->init = init;
> > }
> >
> > struct chip_operations mainboard_ops = {
> >   .enable_dev = enable_dev,
> > };
> >
> > Signed-off-by: Stefan Reinauer
>
> I really like this patch. There are one or two things I don't understand
> yet, but I hope stuff will become clearer tomorrow after testing. Will
> ack after testing.

Tested, ends up in a reboot loop. Sorry. Will post the logs (old vs.
new) in an hour.

There were some irritating ACPI IRQ routing changes between r4614 and
r4681 (IRQ lines with IRQ 0 were suddenly enabled), but they had no
influence on booting.

Regards,
Carl-Daniel

[Stefan Reinauer]

Carl-Daniel Hailfinger wrote:
> On 27.09.2009 22:29, Carl-Daniel Hailfinger wrote:
> > On 27.09.2009 20:49, Stefan Reinauer wrote:
> > > Simplify coreboot PCI handling
> > >
> > > This patch drops the conf1/conf2 autodetection and replaces it by 
> > > (usually northbridge specific) hardcodes.
> > >
> > > This patch also adds pci_domain_init() which needs to be called by
> > > mainboard enable_dev() functions in order to be able to use the pci
> > > config space functions. This allows to drop i386 specific code from
> > > generic files again...
> > >
> > > There is an even better approach to the PCI config space access in mainboard
> > > specific init files problem, but that should go into another patch:
> > >
> > > static void init(struct device *dev)
> > > {
> > >    // Do the stuff here!
> > > }
> > >
> > > static void enable_dev(struct device *dev)
> > > {
> > >    // Install an init function for this mainboard device
> > >    dev->ops->init = init;
> > > }
> > >
> > > struct chip_operations mainboard_ops = {
> > >   .enable_dev = enable_dev,
> > > };
> > >
> > > Signed-off-by: Stefan Reinauer
> >
> > I really like this patch. There are one or two things I don't understand
> > yet, but I hope stuff will become clearer tomorrow after testing. Will
> > ack after testing.
>
> Tested, ends up in a reboot loop. Sorry. Will post the logs (old vs.
> new) in an hour.

Oh, sorry to hear that. I have to admit, I sent that patch untested, as
I wanted to get it out asap.
> There were some irritating ACPI IRQ routing changes between r4614 and
> r4681 (IRQ lines with IRQ 0 were suddenly enabled), but they had no
> influence on booting.

Odd.. Can you do some bisecting?

Stefan

[Carl-Daniel Hailfinger]

On 28.09.2009 22:39, Stefan Reinauer wrote:
> Carl-Daniel Hailfinger wrote:
> > On 27.09.2009 22:29, Carl-Daniel Hailfinger wrote:
> > > On 27.09.2009 20:49, Stefan Reinauer wrote:
> > > > Simplify coreboot PCI handling
> > > >
> > > > This patch drops the conf1/conf2 autodetection and replaces it by 
> > > > (usually northbridge specific) hardcodes.
> > > >
> > > > This patch also adds pci_domain_init() which needs to be called by
> > > > mainboard enable_dev() functions in order to be able to use the pci
> > > > config space functions. This allows to drop i386 specific code from
> > > > generic files again...
> > > >
> > > > There is an even better approach to the PCI config space access in mainboard
> > > > specific init files problem, but that should go into another patch:
> > > >
> > > > static void init(struct device *dev)
> > > > {
> > > >    // Do the stuff here!
> > > > }
> > > >
> > > > static void enable_dev(struct device *dev)
> > > > {
> > > >    // Install an init function for this mainboard device
> > > >    dev->ops->init = init;
> > > > }
> > > >
> > > > struct chip_operations mainboard_ops = {
> > > >   .enable_dev = enable_dev,
> > > > };
> > > >
> > > > Signed-off-by: Stefan Reinauer
> > >
> > > I really like this patch. There are one or two things I don't understand
> > > yet, but I hope stuff will become clearer tomorrow after testing. Will
> > > ack after testing.
> >
> > Tested, ends up in a reboot loop. Sorry. Will post the logs (old vs.
> > new) in an hour.
>
> Oh, sorry to hear that. I have to admit, I sent that patch untested, as
> I wanted to get it out asap.

Yes, and I appreciate that. Logs attached. I have cropped the reboot
loop after the second iteration because the log was an exact repetition.

Working log: minicom_2009-09-28T212527.cap
Failing log: minicom_2009-09-28T212703.cap
> > There were some irritating ACPI IRQ routing changes between r4614 and
> > r4681 (IRQ lines with IRQ 0 were suddenly enabled), but they had no
> > influence on booting.
>
> Odd.. Can you do some bisecting?

Sure, will do tonight or early tomorrow.

Regards,
Carl-Daniel

[Carl-Daniel Hailfinger]

On 29.09.2009 12:19, Carl-Daniel Hailfinger wrote:
> On 28.09.2009 22:39, Stefan Reinauer wrote:
> > Carl-Daniel Hailfinger wrote:
> > > On 27.09.2009 22:29, Carl-Daniel Hailfinger wrote:
> > > > On 27.09.2009 20:49, Stefan Reinauer wrote:
> > > > > Simplify coreboot PCI handling
> > > > >
> > > > > This patch drops the conf1/conf2 autodetection and replaces it by 
> > > > > (usually northbridge specific) hardcodes.
> > > > >
> > > > > This patch also adds pci_domain_init() which needs to be called by
> > > > > mainboard enable_dev() functions in order to be able to use the pci
> > > > > config space functions. This allows to drop i386 specific code from
> > > > > generic files again...
> > > > >
> > > > > There is an even better approach to the PCI config space access in mainboard
> > > > > specific init files problem, but that should go into another patch:
> > > > >
> > > > > static void init(struct device *dev)
> > > > > {
> > > > >    // Do the stuff here!
> > > > > }
> > > > >
> > > > > static void enable_dev(struct device *dev)
> > > > > {
> > > > >    // Install an init function for this mainboard device
> > > > >    dev->ops->init = init;
> > > > > }
> > > > >
> > > > > struct chip_operations mainboard_ops = {
> > > > >   .enable_dev = enable_dev,
> > > > > };
> > > > >
> > > > > Signed-off-by: Stefan Reinauer
> > > >
> > > > I really like this patch. There are one or two things I don't understand
> > > > yet, but I hope stuff will become clearer tomorrow after testing. Will
> > > > ack after testing.
> > >
> > > Tested, ends up in a reboot loop. Sorry. Will post the logs (old vs.
> > > new) in an hour.
> >
> > Oh, sorry to hear that. I have to admit, I sent that patch untested, as
> > I wanted to get it out asap.
>
> Yes, and I appreciate that. Logs attached. I have cropped the reboot
> loop after the second iteration because the log was an exact repetition.
>
> Working log: minicom_2009-09-28T212527.cap
> Failing log: minicom_2009-09-28T212703.cap

I kept only the RS690 changes and reverted the rest. Result attached.

Will debug further.

Regards,
Carl-Daniel