Re: [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, [e-mail] 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.