r556 - in coreboot-v3: device include/device

Author: rminnich
Date: 2008-01-17 17:32:12 +0100 (Thu, 17 Jan 2008)
New Revision: 556

Modified:
   coreboot-v3/device/device.c
   coreboot-v3/include/device/device.h
Log:
This change is for tidying up some unfinished business in the device code. 
I just uncovered this problem while trying to get the lx going.

The symptom was that the northbridge ops were never getting run, 
in particular the phase 2 ops for the geodelx were not running. The 
reason was that the ops struct member for the device was not set.

How is the ops struct member set? 
Currently, the ops vector for a static device (i.e. a device created from the 
dts) has to be set by hand, as in mainboard/emulation/qemu-x86/dts:
    domain0 {
    	/config/("northbridge/intel/i440bxemulation");
    	ops = "i440bxemulation_pcidomainops";

This requirement is ridiculous (it's my fault). If we know the part, 
and have the dts, we should not have to explicitly name the ops. In fact the
constructors array, defined at the end of the various device files, makes 
searching for an ops struct for a dynamic device automatic. We should 
support this automatic behavior for static devices too.

Given the function find_constructor
in device/device.c, why don't we just use that? The problem is that we did 
not set up the device struct to include a device id, just a device path, and
find_constructor requires a device_id -- which makes sense, I hope, 
as the path is its pci path (e.g. 0:1.0) and the constructors are defined by the 
device id (i.e. it is the same constructor for a given part, no matter how many
of the part we have).

So, as a start to fixing this limitation (this is going to take several patches), 
I've done the following:
1. add a struct device_id to the device struct. 
2. extended the dev_init code in device/device.c -- this is the first function
    called from lib/stage2.c -- to find a constructor for the dev->id and, if
    found, set dev->ops to it.

Result: for static devices with the id set, the ops pointer will be set 
automatically. Coreboot builds fine with this change.

The next change will be to add dtc commands to set ids. 
Currently, we have commands like pcipath, pcidomain, etc.; 
the new commands will look like pciid, domainid, etc. Once we have these
commands, we will have made it possible to set ops automatically. We 
can just set the ids in the device dts file, and users will never have to 
see any of this complication.

The final change will be a bit more complicated. Right now, if you look in, 
e.g., northbridge/amd/geodelx/dts, you'll see that we have one dts, but 
the northbridge plays three roles. We can't easily contain those three 
roles in one dts (I am open to suggestions showing I am wrong). 
I am going to propose that we have more than one dts file
in a directory, so instead of 
northbridge/amd/geodelx/dts 
we would have
northbridge/amd/geodelx/dtsdomain
northbridge/amd/geodelx/dtsapic
northbridge/amd/geodelx/dtspci
so that we could set the variables for each of these individual components.

There is no need to split geodelx.c into three .c files, however.

Finally, I will be removing the archaic vendor and device unsigned's from
the device struct in future, but as I say, I am trying to stage these changes
to keep them understandable.

Signed-off-by: Ronald G. Minnich

Acked-by: Peter Stuge

Modified: coreboot-v3/device/device.c
===================================================================
--- coreboot-v3/device/device.c	2008-01-17 05:37:41 UTC (rev 555)
+++ coreboot-v3/device/device.c	2008-01-17 16:32:12 UTC (rev 556)
@@ -88,21 +88,6 @@
 }

/**
- * Initialization tasks for the device tree code. 
- * 
- * At present, merely sets up last_dev_p, which used to be done by
- * Fucking Magic (FM) in the config tool.
- */
-void dev_init(void)
-{
-	struct device *dev;
-
-	for (dev = all_devices; dev; dev = dev->next) {
-		last_dev_p = &dev->next;
-	}
-}
-
-/**
  * The default constructor, which simply sets the ops pointer.
  * 
  * Initialize device->ops of a newly allocated device structure.
@@ -149,6 +134,30 @@
 }

/**
+ * Initialization tasks for the device tree code. 
+ * 
+ * Sets up last_dev_p, which used to be done by
+ * Fucking Magic (FM) in the config tool. Also, for each of the 
+ * devices, tries to find the constructor, and from there, the ops, 
+ * for the device. 
+ */
+void dev_init(void)
+{
+	struct device *dev;
+	struct constructor *c;
+
+	for (dev = all_devices; dev; dev = dev->next) {
+		c = find_constructor(&dev->id);
+		/* note the difference from the constructor function below. 
+		 * we are not allocating the device here, just setting the ops. 
+		 */
+		if (c)
+			dev->ops = c->ops;
+		last_dev_p = &dev->next;
+	}
+}
+
+/**
  * Given a path, find a constructor, and run it.
  * 
  * Given a path, call find_constructor to find the constructor for it.
@@ -725,6 +734,9 @@
    printk(BIOS_DEBUG, "Phase 2: Early setup...\n");
    for (dev = all_devices; dev; dev = dev->next) {
    	printk(BIOS_SPEW, "%s: dev %s: ", __FUNCTION__, dev->dtsname);
+		printk(BIOS_SPEW, "%s: ops %p ops->phase2_setup_scan_bus %p\n",
+			__FUNCTION__, dev->ops, 
+			dev->ops? dev->ops->phase2_setup_scan_bus : "None");
    	if (dev->ops && dev->ops->phase2_setup_scan_bus) {
    		printk(BIOS_SPEW,
    		       "Calling phase2 phase2_setup_scan_bus...");

Modified: coreboot-v3/include/device/device.h
===================================================================
--- coreboot-v3/include/device/device.h	2008-01-17 05:37:41 UTC (rev 555)
+++ coreboot-v3/include/device/device.h	2008-01-17 16:32:12 UTC (rev 556)
@@ -185,9 +185,11 @@
    struct device *	next;		/* chain of all devices */

struct device_path path;
+	struct device_id id;
    char 		dtsname[MAX_DTSNAME_SIZE];	/* the name from the dts */
-	unsigned 	vendor;
-	unsigned 	device;
+	/* XXX remove this soon */
+	unsigned 	device, vendor;
+	/* XXX */
    u16 status;
    u8 revision;
    u8 cache_line;