Re: [coreboot] [PATCH] Disable integrated Intel 3100 devices properly

[Uwe Hermann]

On Mon, Mar 31, 2008 at 04:38:34PM -0700, Ed Swierk wrote:
> Setting an integrated southbridge device (like SATA or USB2.0) to
> "off" in Config.lb should cause the PCI device not to respond to
> configuration requests. The code that takes care of this for the Intel
> 3100 is in src/southbridge/intel/i3100/i3100.c. Unfortunately I copied
> the esb6300 implementation without noticing that it makes no sense at
> all on the Intel 3100. The attached patch implements this properly for
> the Intel 3100.

You forgot the Signed-off-by.
> /* Temporarily enable the root complex register block at 0xa0000000 */
> lpc_dev = dev_find_slot(0x0, PCI_DEVFN(0x1f, 0x0));
> pci_write_config32(lpc_dev, 0xf0, 0xa0000000 | (1 << 0));
> disable = (volatile u32 *) 0xa0003418;
> func = PCI_FUNC(dev->path.u.pci.devfn);
> switch (PCI_SLOT(dev->path.u.pci.devfn)) {
> case 0x1f:
> *disable |= (1 << (func == 0x0 ? 14 : func));
> break;
> case 0x1d:
> *disable |= (1 << (func + 8));
> break;
> case 0x1c:
> *disable |= (1 << (func + 16));
> break;

This part could use a small comment about what exactly is disabled in
each "case".

Rest of the patch looks good. Is this build-tested and tested on
hardware? If so, I can ack and commit if you resend with a Signed-off-by.

Uwe.