Re: [coreboot] cleanup

[Myles Watson]

> -----Original Message-----
> From: [e-mail] [mailto:[e-mail]]
> On Behalf Of Uwe Hermann
> Sent: Tuesday, October 28, 2008 6:55 PM
> To: ron minnich
> Cc: Coreboot
> Subject: Re: [coreboot] cleanup
>
> On Tue, Oct 28, 2008 at 05:36:07PM -0700, ron minnich wrote:
> > General cleanup and comments for things that should be fixed in future.
> > Most substantive change is getting rid of 'initialized', which was only
> > ever needed in v2 due to an implementation mistake.
>
> Can you elaborate? What was 'initialized' used for specifically and why is
> it not needed anymore?

It's only ever used in device.c and makes sure that devices don't get
initialized twice.  As long as your device list doesn't have duplicates you
should be safe.
> > Signed-off-by: Ronald G. Minnich

Acked-by: Myles Watson
> > Index: southbridge/intel/i82371eb/i82371eb.c
> >
> > --- southbridge/intel/i82371eb/i82371eb.c	(revision 954)
> > +++ southbridge/intel/i82371eb/i82371eb.c	(working copy)
> > @@ -43,6 +43,7 @@
> >  {
> >    unsigned short c;
> > /* These should be controlled in the dts. */
>
> Btw, don't waste too much time on 82371EB in v3, I'll move the v2 version
> (which is a lot more complete) to v3 soonish. That'll work for the
> 440BX (which will be moved to v3 sooner or later too), but also for QEMU
> of course.
> > printk(BIOS_DEBUG, "Enabling IDE channel 1\n");
> >    c = pci_read_config16(dev, 0x40);
> >    c |= 0x8000;
> > @@ -82,6 +83,9 @@
> >    pci_write_config8(dev, 0x80, 1);
> >  }
> >
> > +/*NOTE: We need our own read and set resources for this part! It has
> > BARS that are not in the normal place (such as SMBUS)
> > */
> >
> > /* You can override or extend each operation as needed for the device.
>
> */
> > struct device_operations i82371eb_isa = {
> >    .id = {.type = DEVICE_ID_PCI,
> > Index: include/device/device.h
> > ===================================================================
> > --- include/device/device.h	(revision 954)
> > +++ include/device/device.h	(working copy)
> > @@ -214,7 +214,6 @@
> >    unsigned int	class;		/* 3 bytes: (base,sub,prog-if)
>
> */
> > unsigned int	hdr_type;	/* PCI header type */
> >    unsigned int    enabled : 1;	/* set if we should enable the

device
> */
> > unsigned int    initialized : 1; /* set if we have initialized the
>
> device */
> > unsigned int    have_resources : 1; /* Set if we have read the
>
> devices resources */
> > unsigned int    on_mainboard : 1;
> >    unsigned long   rom_address;
> > Index: mainboard/amd/dbm690t/mainboard.c
> > ===================================================================
> > --- mainboard/amd/dbm690t/mainboard.c	(revision 954)
> > +++ mainboard/amd/dbm690t/mainboard.c	(working copy)
> > @@ -17,6 +17,11 @@
> > Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-
>
> 1301 USA
> > */
> >
> > +/* N.B. This file should be removed in the long term. */
> > +/* the nic code goes to the south support. The UMA code should
> > be moved to the cpu support.
> > */
> >
> > #include <mainboard.h>
> >  #include <config.h>
> >  #include <types.h>
> > Index: mainboard/emulation/qemu-x86/vga.c
> > ===================================================================
> > --- mainboard/emulation/qemu-x86/vga.c	(revision 954)
> > +++ mainboard/emulation/qemu-x86/vga.c	(working copy)
> > @@ -38,6 +38,8 @@
> >    /*
> >     * FIXME: This should be in the Super I/O code some day,
> >     * but since QEMU has no Super I/O...
> > * we need to create superio/emulation/qemu and move the keyboard
> > * bits there.
> > */
> >  init_pc_keyboard(0x60, 0x64, &conf);
> >  /* now run the rom */
> > Index: device/device.c
> >
> > --- device/device.c	(revision 954)
> > +++ device/device.c	(working copy)
> > @@ -805,6 +805,7 @@
> >  #warning do we call phase3_enable here.
> >    	new_max = busdevice->ops->phase3_scan(busdevice, max);
> >    	do_phase3 = 0;
> > /* do we *ever* use this path */
>
> Is this a question or observation?

I get the "dev_phase3_scan: scanning..." messages in my boot logs.

Thanks,
Myles
> > for (link = 0; link < busdevice->links; link++) {
> > 	if (busdevice->link[link].reset_needed) {
> > 		if (reset_bus(&busdevice->link[link])) {
> >
> > @@ -973,7 +974,7 @@
> >
> > printk(BIOS_INFO, "Phase 6: Initializing devices...\n");
> >    for (dev = all_devices; dev; dev = dev->next) {
> > if (dev->enabled && !dev->initialized &&
> > if (dev->enabled &&
> > dev->ops && dev->ops->phase6_init) {
>
> Merge line 2 into line 1 here, the code should now fit into 80
> chars/line.
> > 	if (dev->path.type == DEVICE_PATH_I2C) {
> > 		printk(BIOS_DEBUG, "Phase 6: smbus:

%s[%d]->",
> > @@ -981,7 +982,6 @@
> >    		}
> >    		printk(BIOS_DEBUG, "Phase 6: %s init.\n",
> >    		       dev_path(dev));
> > 	dev->initialized = 1;
> > dev->ops->phase6_init(dev);
> > }
> > }
> >
> > @@ -995,8 +995,8 @@
> >    printk(BIOS_INFO, "Show all devs...\n");
> >    for (dev = all_devices; dev; dev = dev->next) {
> >    	printk(BIOS_SPEW,
> >        "%s(%s): enabled %d have_resources %d initialized
>
> %d\n",
> >        "%s(%s): enabled %d have_resources %d\n",
> >   dev->dtsname, dev_path(dev), dev->enabled,
> >        dev->have_resources, dev->initialized);
> >        dev->have_resources);
> > }
> >
> > }
> > Index: device/device_util.c
> > ===================================================================
> > --- device/device_util.c	(revision 953)
> > +++ device/device_util.c	(working copy)
> > @@ -430,10 +430,7 @@
> >    for (i = 0; i < dev->resources;) {
> >    	resource = &dev->resource[i];
> >    	if (!resource->flags) {
> > 	/* Note: memmove() was used here. But this can never
> > 	 * overlap, right?
> > 	 */
> > 	memcpy(resource, resource + 1, (dev->resources-i)*
>
> sizeof(*resource));
> > 	memmove(resource, resource + 1, (dev->resources-i)*
>
> sizeof(*resource));
>
> Specific bug or is this "just in case"?
>
> I cannot say much about the 'initialized' issue, so I'll leave a
> review/ack to others.
> Uwe.
>
> http://www.hermann-uwe.de  | http://www.holsham-traders.de
> http://www.crazy-hacks.org | http://www.unmaintained-free-software.org