[coreboot] r3157 - in trunk/coreboot-v2/src/southbridge/intel: . i3100

[Uwe Hermann]

A few more comments on things I noticed:

On Mon, Mar 17, 2008 at 12:34:17AM +0100, svn at coreboot.org wrote:
> +static void i3100_enable_superio(void)
> +{
> +	device_t dev;
> +	dev = pci_locate_device(PCI_ID(PCI_VENDOR_ID_INTEL,
> +				       PCI_DEVICE_ID_INTEL_3100_LPC), 0);
> +	if (dev == PCI_DEV_INVALID) {
> +		die("LPC bridge not found\r\n");
> +	}

Do we actually need this kind of check? If the LPC device is not there
or is the wrong one, that's clearly a programming bug, no?
This function will only be called from i3100 boards, so the LPC device
is surely available.

Similar example: when you write code for an Intel board and include an AMD
CPU init file or something -- clearly a bug, no need for bloating the
code to check for that kind of programmer bug. Opinions?

I'd say drop this check (here and in many other files/chipsets).

Or is there some other valid reason why this has to be checked at 
run-time in the code?


> +
> +	/* Enable decoding of I/O locations for SuperIO devices */
> +	pci_write_config16(dev, 0x82, 0x340f);
> +}
> 
> Added: trunk/coreboot-v2/src/southbridge/intel/i3100/i3100_early_smbus.c
> ===================================================================
[...]
> +#include "i3100_smbus.h"
> +
> +#define SMBUS_IO_BASE 0x0f00
> +
> +static void enable_smbus(void)
> +{
> +	device_t dev;
> +	dev = pci_locate_device(PCI_ID(PCI_VENDOR_ID_INTEL,
> +				       PCI_DEVICE_ID_INTEL_3100_SMB), 0);
> +	if (dev == PCI_DEV_INVALID) {
> +		die("SMBus controller not found\r\n");
> +	}

Ditto, see above.


> +#include <console/console.h>
> +#include <device/device.h>
> +#include <device/pci.h>

> +#include <device/pci_ids.h>

Currently needed, but I think we should also have pci.h include this
one, as we do in v3 -- pretty much every file needs pci_ids.h.


> +#include <device/pci_ops.h>

Not needed, already included by pci.h.


> Added: trunk/coreboot-v2/src/southbridge/intel/i3100/i3100_reset.c
> ===================================================================
> --- trunk/coreboot-v2/src/southbridge/intel/i3100/i3100_reset.c	                        (rev 0)
> +++ trunk/coreboot-v2/src/southbridge/intel/i3100/i3100_reset.c	2008-03-16 23:34:10 UTC (rev 3157)
> @@ -0,0 +1,26 @@
[...]
> +#include <arch/io.h>
> +
> +void hard_reset(void)
> +{
> +	outb(0x06, 0xcf9);
> +}

Is this tested? When and where is it used/invoked? coreboot-internally
as part of bringup or when you say 'reboot' in Linux?

Does it have to be in an extra file? That's a bit of an overkill for one
line of code. Can we merge it into some other i3100 file?


Uwe.
-- 
http://www.hermann-uwe.de  | http://www.holsham-traders.de
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org