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 rminnich@gmail.com
Acked-by: Peter Stuge peter@stuge.se
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;