[coreboot] cleanup
Uwe Hermann
uwe at hermann-uwe.de
Wed Oct 29 01:55:08 CET 2008
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
More information about the coreboot
mailing list