Re: [coreboot] [PATCH 3/5] Intel 3100, version 3

[Uwe Hermann]

On Thu, Mar 13, 2008 at 05:16:26PM -0700, Ed Swierk wrote:
> +/* This code is based on src/southbridge/intel/esb6300/chip.h */

Drop this, it's just a list of #defines.
> Index: coreboot-v2-3137/src/southbridge/intel/i3100/i3100_early_smbus.c

[...]
> +/* This code is based on src/southbridge/intel/esb6300/esb6300_early_smbus.c */

Drop, this code is trivial.
> +#include "i3100_smbus.h"
>
> +#define SMBUS_IO_BASE 0x0f00
>
> +static void enable_smbus(void)
> +{
> device_t dev;
> dev = pci_locate_device(PCI_ID(0x8086, 0x269b), 0);

Use values from pci_id.h, please.
> if (dev == PCI_DEV_INVALID) {
> die("SMBus controller not found\r\n");
> }
> uint8_t enable;

Move all variables to the top.
> print_spew("SMBus controller enabled\r\n");
> pci_write_config32(dev, 0x20, SMBUS_IO_BASE | 1);
> pci_write_config8(dev, 0x40, 1);
> pci_write_config8(dev, 0x4, 1);
> /* SMBALERT_DIS */
>    outb(4, SMBUS_IO_BASE + SMBSLVCMD);
> /* Disable interrupt generation */
> outb(0, SMBUS_IO_BASE + SMBHSTCTL);
>
> +}
>
> +static int smbus_read_byte(unsigned device, unsigned address)

Use u8, u16 et al.
> +{
> return do_smbus_read_byte(SMBUS_IO_BASE, device, address);
>
> +}
> Index: coreboot-v2-3137/src/southbridge/intel/i3100/i3100_ehci.c
> ===================================================================
> --- /dev/null
> +++ coreboot-v2-3137/src/southbridge/intel/i3100/i3100_ehci.c
> @@ -0,0 +1,71 @@
> +/*
> This file is part of the coreboot project.
> Copyright (C) 2008 Arastra, Inc.
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License version 2 as
> published by the Free Software Foundation.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +/* This code is based on src/southbridge/intel/esb6300/esb6300_ehci.c */

Drop, trivial/canonical code.
> +#include <console/console.h>
> +#include <device/device.h>
> +#include <device/pci.h>
> +#include <device/pci_ids.h>
> +#include <device/pci_ops.h>
> +#include "i3100.h"
>
> +static void ehci_init(struct device *dev)
> +{
> uint32_t cmd;
> /* Not sure if this is needed on i3100: */
> printk_debug("EHCI: Setting up controller.. ");
> cmd = pci_read_config32(dev, PCI_COMMAND);
> pci_write_config32(dev, PCI_COMMAND, cmd | PCI_COMMAND_MASTER);
> printk_debug("done.\n");

Tested? I'd say drop it (i.e. make the function empty) until it's proven
that this is really required. AFAIK it's not (as seen on other boards).
> +}
>
> +static void ehci_set_subsystem(device_t dev, unsigned vendor, unsigned device)
> +{
> uint8_t access_cntl;
> access_cntl = pci_read_config8(dev, 0x80);
> /* Enable writes to protected registers */
> pci_write_config8(dev, 0x80, access_cntl | 1);
> /* Write the subsystem vendor and device id */
> pci_write_config32(dev, PCI_SUBSYSTEM_VENDOR_ID,
> ((device & 0xffff) << 16) | (vendor & 0xffff));
> /* Restore protection */
> pci_write_config8(dev, 0x80, access_cntl);
>
> +}
>
> +static struct pci_operations lops_pci = {
> .set_subsystem = &ehci_set_subsystem,
>
> +};
> +static struct device_operations ehci_ops  = {
> .read_resources   = pci_dev_read_resources,
> .set_resources    = pci_dev_set_resources,
> .enable_resources = pci_dev_enable_resources,
> .init             = ehci_init,
> .scan_bus         = 0,
> .enable           = i3100_enable,
> .ops_pci          = &lops_pci,
>
> +};
>
> +static struct pci_driver ehci_driver __pci_driver = {
> .ops    = &ehci_ops,
> .vendor = PCI_VENDOR_ID_INTEL,
> .device = PCI_DEVICE_ID_INTEL_3100_EHCI,
>
> +};
> Index: coreboot-v2-3137/src/southbridge/intel/i3100/i3100_lpc.c
> ===================================================================
> --- /dev/null
> +++ coreboot-v2-3137/src/southbridge/intel/i3100/i3100_lpc.c
> @@ -0,0 +1,316 @@
> +/*
> This file is part of the coreboot project.
> Copyright (C) 2004 Linux Networx
> Copyright (C) 2008 Arastra, Inc.
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License version 2 as
> published by the Free Software Foundation.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +/* This code is based on src/southbridge/intel/esb6300/esb6300_lpc.c */

This is ok, this file is huge and non-trivial.
> Index: coreboot-v2-3137/src/southbridge/intel/i3100/i3100_pci.c
>
> --- /dev/null
> +++ coreboot-v2-3137/src/southbridge/intel/i3100/i3100_pci.c
> @@ -0,0 +1,58 @@
> +/*
> This file is part of the coreboot project.
> Copyright (C) 2008 Arastra, Inc.
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License version 2 as
> published by the Free Software Foundation.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +/* This code is based on src/southbridge/intel/esb6300/esb6300_pci.c */

Drop, trivial.
> +#include <console/console.h>
> +#include <device/device.h>
> +#include <device/pci.h>
> +#include <device/pci_ids.h>
> +#include <device/pci_ops.h>
> +#include "i3100.h"
>
> +static void pci_init(struct device *dev)
> +{
> uint16_t word;
> /* Clear system errors */
> word = pci_read_config16(dev, 0x06);
> word |= 0xf900; /* Clear possible errors */
> pci_write_config16(dev, 0x06, word);
> word = pci_read_config16(dev, 0x1e);
> word |= 0xf800; /* Clear possible errors */
> pci_write_config16(dev, 0x1e, word);

Needed? Maybe yes, not sure. Have you tested dropping it?
> +}
>
> +static struct device_operations pci_ops  = {
> .read_resources   = pci_bus_read_resources,
> .set_resources    = pci_dev_set_resources,
> .enable_resources = pci_bus_enable_resources,
> .init             = pci_init,
> .scan_bus         = pci_scan_bridge,
> .ops_pci          = 0,
>
> +};
>
> +static struct pci_driver pci_driver __pci_driver = {
> .ops    = &pci_ops,
> .vendor = PCI_VENDOR_ID_INTEL,
> .device = PCI_DEVICE_ID_INTEL_3100_PCI,
>
> +};
> Index: coreboot-v2-3137/src/southbridge/intel/i3100/i3100_sata.c

[...]
> +static void sata_set_subsystem(device_t dev, unsigned vendor, unsigned device)

u16 for vendor/device.
> +{
> pci_write_config32(dev, PCI_SUBSYSTEM_VENDOR_ID,
> ((device & 0xffff) << 16) | (vendor & 0xffff));
>
> +}
>
> +static struct pci_operations lops_pci = {
> .set_subsystem = sata_set_subsystem,
>
> +};
> +static struct device_operations sata_ops  = {
> .read_resources   = pci_dev_read_resources,
> .set_resources    = pci_dev_set_resources,
> .enable_resources = pci_dev_enable_resources,
> .init             = sata_init,
> .scan_bus         = 0,
> .ops_pci          = &lops_pci,
>
> +};
>
> +static struct pci_driver sata_driver __pci_driver = {
>    .ops    = &sata_ops,
>    .vendor = PCI_VENDOR_ID_INTEL,
>    .device = PCI_DEVICE_ID_INTEL_3100_SATA,
>
> +};
>
> +static struct pci_driver sata_driver_nr __pci_driver = {
>    .ops    = &sata_ops,
>    .vendor = PCI_VENDOR_ID_INTEL,
>    .device = PCI_DEVICE_ID_INTEL_3100_SATA_R,
>
> +};
> Index: coreboot-v2-3137/src/southbridge/intel/i3100/i3100_smbus.c
>
> --- /dev/null
> +++ coreboot-v2-3137/src/southbridge/intel/i3100/i3100_smbus.c
> @@ -0,0 +1,69 @@
> +/*
> This file is part of the coreboot project.
> Copyright (C) 2008 Arastra, Inc.
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License version 2 as
> published by the Free Software Foundation.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +/* This code is based on src/southbridge/intel/esb6300/esb6300_smbus.c */

Drop, trivial.
> Index: coreboot-v2-3137/src/southbridge/intel/i3100/i3100_uhci.c
>
> --- /dev/null
> +++ coreboot-v2-3137/src/southbridge/intel/i3100/i3100_uhci.c
> @@ -0,0 +1,71 @@
> +/*
> This file is part of the coreboot project.
> Copyright (C) 2008 Arastra, Inc.
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License version 2 as
> published by the Free Software Foundation.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +/* This code is based on src/southbridge/intel/esb6300/esb6300_uhci.c */

Drop, trivial file.
> +#include <console/console.h>
> +#include <device/device.h>
> +#include <device/pci.h>
> +#include <device/pci_ids.h>
> +#include <device/pci_ops.h>
> +#include "i3100.h"
>
> +static void uhci_init(struct device *dev)
> +{
> uint32_t cmd;
> /* Not sure if this is needed on i3100: */
> printk_debug("UHCI: Setting up controller.. ");
> cmd = pci_read_config32(dev, PCI_COMMAND);
> pci_write_config32(dev, PCI_COMMAND, cmd | PCI_COMMAND_MASTER);
> printk_debug("done.\n");

Likely also not needed, please test.
> +}
>
> +static void uhci_set_subsystem(device_t dev, unsigned vendor, unsigned device)
> +{
> pci_write_config32(dev, PCI_SUBSYSTEM_VENDOR_ID,
> ((device & 0xffff) << 16) | (vendor & 0xffff));
>
> +}

Hm, looks redundant, should be in some global file later maybe. Won't
hold up the commit, though.
> +static struct pci_operations lops_pci = {
> .set_subsystem = &uhci_set_subsystem,
>
> +};
>
> +static struct device_operations uhci_ops  = {
> .read_resources   = pci_dev_read_resources,
> .set_resources    = pci_dev_set_resources,
> .enable_resources = pci_dev_enable_resources,
> .init             = uhci_init,
> .scan_bus         = 0,
> .enable           = i3100_enable,
> .ops_pci          = &lops_pci,
>
> +};
>
> +static struct pci_driver uhci_driver __pci_driver = {
> .ops    = &uhci_ops,
> .vendor = PCI_VENDOR_ID_INTEL,
> .device = PCI_DEVICE_ID_INTEL_3100_USB,
>
> +};
>
> +static struct pci_driver usb2_driver __pci_driver = {
> .ops    = &uhci_ops,
> .vendor = PCI_VENDOR_ID_INTEL,
> .device = PCI_DEVICE_ID_INTEL_3100_USB2,
>
> +};
> Index: coreboot-v2-3137/src/southbridge/intel/i3100/i3100_early_lpc.c
> ===================================================================
> --- /dev/null
> +++ coreboot-v2-3137/src/southbridge/intel/i3100/i3100_early_lpc.c
> @@ -0,0 +1,32 @@
> +/*
> This file is part of the coreboot project.
> Copyright (C) 2008 Arastra, Inc.
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License version 2 as
> published by the Free Software Foundation.
> This program is distributed in the hope that it will be useful,
> but WITHOUT ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> GNU General Public License for more details.
> You should have received a copy of the GNU General Public License
> along with this program; if not, write to the Free Software
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> */
>
> +static void i3100_enable_superio(void)
> +{
> device_t dev;
> dev = pci_locate_device(PCI_ID(0x8086, 0x2670), 0);

Use pci_ids.h.
> if (dev == PCI_DEV_INVALID) {
> outb(0xd0, 0x80);

Why 0xd0?
> die("LPC bridge not found\r\n");
> }
> /* Enable decoding of I/O locations for SuperIO devices */
> pci_write_config16(dev, 0x82, 0x340f);
>
> +}

Uwe.