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.

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

[Ed Swierk]

On Mon, Mar 17, 2008 at 6:45 AM, Uwe Hermann  wrote:
> 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).

Agreed. I think it is reasonable for code in
src/southbridge/intel/i3100 to assume it is actually being run on an
Intel 3100 southbridge.
> > 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?

It is invoked by coreboot itself (in hardwaremain(), I believe) but
only after a reboot, not at initial powerup. Without it, coreboot
hangs during the root PCI bus scan.
> 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?

It would be nice if it could go into i3100.c, but I'm not sure I
understand how drivers get linked in.

--Ed