[coreboot] cleanup

[Uwe Hermann]

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?


> Signed-off-by: Ronald G. Minnich <rminnich at gmail.com>
> 
> 
> 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?


>  		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