[Qemu-devel] [PATCH v2 0/2] seabios: pci: pci initialization clean up

[Qemu-devel] [PATCH v2 0/2] seabios: pci: pci initialization clean up

[Isaku Yamahata]

Changes from v1 -> v2
- renamed files i440fx -> dev->i440fx.
- dropped copyright notice from dev-i440fx.h


This patch series cleans up pci device initialization by introducing
a helper function.
This makes it easier to add more device initialization code cleanly
as preparation for q35 chipset.


Isaku Yamahata (2):
  seabios: pci: introduce helper function to initialize a given device.
  seabios: pciinit: use pci device initializer helper function.

 Makefile         |    2 +-
 src/dev-i440fx.c |   52 ++++++++++++++++++
 src/dev-i440fx.h |    8 +++
 src/pci.c        |   20 +++++++
 src/pci.h        |   34 ++++++++++++
 src/pciinit.c    |  160 +++++++++++++++++++++++++-----------------------------
 6 files changed, 188 insertions(+), 88 deletions(-)
 create mode 100644 src/dev-i440fx.c
 create mode 100644 src/dev-i440fx.h

[Qemu-devel] [PATCH v2 1/2] seabios: pci: introduce helper function to initialize a given device.

[Isaku Yamahata]

introduce helper function to initialize a given device,
This will be used later.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 src/pci.c |   20 ++++++++++++++++++++
 src/pci.h |   34 ++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 0 deletions(-)

diff --git a/src/pci.c b/src/pci.c
index 1ab3c2c..c54b084 100644
--- a/src/pci.c
+++ b/src/pci.c
@@ -183,3 +183,23 @@ pci_find_class(u16 classid)
     }
     return -1;
 }
+
+int pci_init_device(const struct pci_device_id *ids, u16 bdf, void *arg)
+{
+    u16 vendor_id = pci_config_readw(bdf, PCI_VENDOR_ID);
+    u16 device_id = pci_config_readw(bdf, PCI_DEVICE_ID);
+    u16 class = pci_config_readw(bdf, PCI_CLASS_DEVICE);
+
+    while (ids->vendid || ids->class_mask) {
+        if ((ids->vendid == PCI_ANY_ID || ids->vendid == vendor_id) &&
+            (ids->devid == PCI_ANY_ID || ids->devid == device_id) &&
+            !((ids->class ^ class) & ids->class_mask)) {
+            if (ids->func) {
+                ids->func(bdf, arg);
+            }
+            return 0;
+        }
+        ids++;
+    }
+    return -1;
+}
diff --git a/src/pci.h b/src/pci.h
index e40e116..fa6a32d 100644
--- a/src/pci.h
+++ b/src/pci.h
@@ -60,6 +60,40 @@ int pci_next(int bdf, int *pmax);
          ; MAX = pci_bus_devfn_to_bdf(BUS, 0) + 0x0100,                 \
            BDF = pci_next(BDF + 1, &MAX))
 
+#define PCI_ANY_ID      (~0)
+struct pci_device_id {
+    u32 vendid;
+    u32 devid;
+    u32 class;
+    u32 class_mask;
+    void (*func)(u16 bdf, void *arg);
+};
+
+#define PCI_DEVICE(vendor_id, device_id, init_func)     \
+    {                                                   \
+        .vendid = (vendor_id),                          \
+        .devid = (device_id),                           \
+        .class = PCI_ANY_ID,                            \
+        .class_mask = 0,                                \
+        .func = (init_func)                             \
+    }
+
+#define PCI_DEVICE_CLASS(vendor_id, device_id, class_code, init_func)   \
+    {                                                                   \
+        .vendid = (vendor_id),                                          \
+        .devid = (device_id),                                           \
+        .class = (class_code),                                          \
+        .class_mask = ~0,                                               \
+        .func = (init_func)                                             \
+    }
+
+#define PCI_DEVICE_END                          \
+    {                                           \
+        .vendid = 0,                            \
+    }
+
+int pci_init_device(const struct pci_device_id *table, u16 bdf, void *arg);
+
 // pirtable.c
 void create_pirtable(void);
 
-- 
1.7.1.1

[Qemu-devel] [PATCH v2 2/2] seabios: pciinit: use pci device initializer helper function.

[Isaku Yamahata]

This patch makes use of pci device initialization helper function
to convert if/switch clause to table driven.
So this makes it easier to add q35 initialization code.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

---
changes v1 -> v2
- renamed files i440fx -> dev->i440fx.
- dropped copyright notice from dev-i440fx.h
---
 Makefile         |    2 +-
 src/dev-i440fx.c |   52 ++++++++++++++++++
 src/dev-i440fx.h |    8 +++
 src/pciinit.c    |  160 +++++++++++++++++++++++++-----------------------------
 4 files changed, 134 insertions(+), 88 deletions(-)
 create mode 100644 src/dev-i440fx.c
 create mode 100644 src/dev-i440fx.h

diff --git a/Makefile b/Makefile
index 3798ca6..47f5625 100644
--- a/Makefile
+++ b/Makefile
@@ -19,7 +19,7 @@ SRCBOTH=misc.c pmm.c stacks.c output.c util.c block.c floppy.c ata.c mouse.c \
 SRC16=$(SRCBOTH) system.c disk.c apm.c font.c
 SRC32FLAT=$(SRCBOTH) post.c shadow.c memmap.c coreboot.c boot.c \
       acpi.c smm.c mptable.c smbios.c pciinit.c optionroms.c mtrr.c \
-      lzmadecode.c bootsplash.c jpeg.c usb-hub.c paravirt.c
+      lzmadecode.c bootsplash.c jpeg.c usb-hub.c paravirt.c dev-i440fx.c
 SRC32SEG=util.c output.c pci.c pcibios.c apm.c stacks.c
 
 cc-option = $(shell if test -z "`$(1) $(2) -S -o /dev/null -xc \
diff --git a/src/dev-i440fx.c b/src/dev-i440fx.c
new file mode 100644
index 0000000..e0b68f4
--- /dev/null
+++ b/src/dev-i440fx.c
@@ -0,0 +1,52 @@
+// initialization function which are specific to i440fx chipset
+//
+// Copyright (C) 2008  Kevin O'Connor <kevin@koconnor.net>
+// Copyright (C) 2006 Fabrice Bellard
+//
+// Copyright (C) 2010 Isaku Yamahata <yamahata at valinux co jp>
+// Split out from pciinit.c
+//
+// This file may be distributed under the terms of the GNU LGPLv3 license.
+//
+
+#include "dev-i440fx.h"
+
+/* PIIX3/PIIX4 PCI to ISA bridge */
+void piix_isa_bridge_init(u16 bdf, void *arg)
+{
+    int i, irq;
+    u8 elcr[2];
+
+    elcr[0] = 0x00;
+    elcr[1] = 0x00;
+    for (i = 0; i < 4; i++) {
+        irq = pci_irqs[i];
+        /* set to trigger level */
+        elcr[irq >> 3] |= (1 << (irq & 7));
+        /* activate irq remapping in PIIX */
+        pci_config_writeb(bdf, 0x60 + i, irq);
+    }
+    outb(elcr[0], 0x4d0);
+    outb(elcr[1], 0x4d1);
+    dprintf(1, "PIIX3/PIIX4 init: elcr=%02x %02x\n", elcr[0], elcr[1]);
+}
+
+/* PIIX3/PIIX4 IDE */
+void piix_ide_init(u16 bdf, void *arg)
+{
+    pci_config_writew(bdf, 0x40, 0x8000); // enable IDE0
+    pci_config_writew(bdf, 0x42, 0x8000); // enable IDE1
+    pci_bios_allocate_regions(bdf, NULL);
+}
+
+/* PIIX4 Power Management device (for ACPI) */
+void piix4_pm_init(u16 bdf, void *arg)
+{
+    // acpi sci is hardwired to 9
+    pci_config_writeb(bdf, PCI_INTERRUPT_LINE, 9);
+
+    pci_config_writel(bdf, 0x40, PORT_ACPI_PM_BASE | 1);
+    pci_config_writeb(bdf, 0x80, 0x01); /* enable PM io space */
+    pci_config_writel(bdf, 0x90, PORT_SMB_BASE | 1);
+    pci_config_writeb(bdf, 0xd2, 0x09); /* enable SMBus io space */
+}
diff --git a/src/dev-i440fx.h b/src/dev-i440fx.h
new file mode 100644
index 0000000..acf1f50
--- /dev/null
+++ b/src/dev-i440fx.h
@@ -0,0 +1,8 @@
+#ifndef __I440FX_H
+#define __I440FX_H
+
+void piix_isa_bridge_init(u16 bdf, void *arg);
+void piix_ide_init(u16 bdf, void *arg);
+void piix4_pm_init(u16 bdf, void *arg);
+
+#endif // __I440FX_H
diff --git a/src/pciinit.c b/src/pciinit.c
index 3f6d79c..462d473 100644
--- a/src/pciinit.c
+++ b/src/pciinit.c
@@ -10,6 +10,7 @@
 #include "biosvar.h" // GET_EBDA
 #include "pci_ids.h" // PCI_VENDOR_ID_INTEL
 #include "pci_regs.h" // PCI_COMMAND
+#include "dev-i440fx.h"
 
 #define PCI_ROM_SLOT 6
 #define PCI_NUM_REGIONS 7
@@ -125,7 +126,7 @@ static int pci_bios_allocate_region(u16 bdf, int region_num)
     return is_64bit;
 }
 
-static void pci_bios_allocate_regions(u16 bdf)
+static void pci_bios_allocate_regions(u16 bdf, void *arg)
 {
     int i;
     for (i = 0; i < PCI_NUM_REGIONS; i++) {
@@ -145,34 +146,15 @@ static int pci_slot_get_pirq(u16 bdf, int irq_num)
     return (irq_num + slot_addend) & 3;
 }
 
-static void pci_bios_init_bridges(u16 bdf)
-{
-    u16 vendor_id = pci_config_readw(bdf, PCI_VENDOR_ID);
-    u16 device_id = pci_config_readw(bdf, PCI_DEVICE_ID);
-
-    if (vendor_id == PCI_VENDOR_ID_INTEL
-        && (device_id == PCI_DEVICE_ID_INTEL_82371SB_0
-            || device_id == PCI_DEVICE_ID_INTEL_82371AB_0)) {
-        int i, irq;
-        u8 elcr[2];
-
-        /* PIIX3/PIIX4 PCI to ISA bridge */
-
-        elcr[0] = 0x00;
-        elcr[1] = 0x00;
-        for (i = 0; i < 4; i++) {
-            irq = pci_irqs[i];
-            /* set to trigger level */
-            elcr[irq >> 3] |= (1 << (irq & 7));
-            /* activate irq remapping in PIIX */
-            pci_config_writeb(bdf, 0x60 + i, irq);
-        }
-        outb(elcr[0], 0x4d0);
-        outb(elcr[1], 0x4d1);
-        dprintf(1, "PIIX3/PIIX4 init: elcr=%02x %02x\n",
-                elcr[0], elcr[1]);
-    }
-}
+static const struct pci_device_id pci_isa_bridge_tbl[] = {
+    /* PIIX3/PIIX4 PCI to ISA bridge */
+    PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371SB_0,
+               piix_isa_bridge_init),
+    PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371AB_0,
+               piix_isa_bridge_init),
+
+    PCI_DEVICE_END
+};
 
 #define PCI_IO_ALIGN            4096
 #define PCI_IO_SHIFT            8
@@ -181,7 +163,7 @@ static void pci_bios_init_bridges(u16 bdf)
 #define PCI_PREF_MEMORY_ALIGN   (1UL << 20)
 #define PCI_PREF_MEMORY_SHIFT   16
 
-static void pci_bios_init_device_bridge(u16 bdf)
+static void pci_bios_init_device_bridge(u16 bdf, void *arg)
 {
     pci_bios_allocate_region(bdf, 0);
     pci_bios_allocate_region(bdf, 1);
@@ -263,58 +245,73 @@ static void pci_bios_init_device_bridge(u16 bdf)
     pci_config_maskw(bdf, PCI_BRIDGE_CONTROL, 0, PCI_BRIDGE_CTL_SERR);
 }
 
+static void storage_ide_init(u16 bdf, void *arg)
+{
+    /* IDE: we map it as in ISA mode */
+    pci_set_io_region_addr(bdf, 0, PORT_ATA1_CMD_BASE);
+    pci_set_io_region_addr(bdf, 1, PORT_ATA1_CTRL_BASE);
+    pci_set_io_region_addr(bdf, 2, PORT_ATA2_CMD_BASE);
+    pci_set_io_region_addr(bdf, 3, PORT_ATA2_CTRL_BASE);
+}
+
+static void pic_ibm_init(u16 bdf, void *arg)
+{
+    /* PIC, IBM, MPIC & MPIC2 */
+    pci_set_io_region_addr(bdf, 0, 0x80800000 + 0x00040000);
+}
+
+static void apple_macio_init(u16 bdf, void *arg)
+{
+    /* macio bridge */
+    pci_set_io_region_addr(bdf, 0, 0x80800000);
+}
+
+static const struct pci_device_id pci_class_tbl[] = {
+    /* STORAGE IDE */
+    PCI_DEVICE_CLASS(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371SB_1,
+                     PCI_CLASS_STORAGE_IDE, piix_ide_init),
+    PCI_DEVICE_CLASS(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371AB,
+                     PCI_CLASS_STORAGE_IDE, piix_ide_init),
+    PCI_DEVICE_CLASS(PCI_ANY_ID, PCI_ANY_ID, PCI_CLASS_STORAGE_IDE,
+                     storage_ide_init),
+
+    /* PIC, IBM, MIPC & MPIC2 */
+    PCI_DEVICE_CLASS(PCI_VENDOR_ID_IBM, 0x0046, PCI_CLASS_SYSTEM_PIC,
+                     pic_ibm_init),
+    PCI_DEVICE_CLASS(PCI_VENDOR_ID_IBM, 0xFFFF, PCI_CLASS_SYSTEM_PIC,
+                     pic_ibm_init),
+
+    /* 0xff00 */
+    PCI_DEVICE_CLASS(PCI_VENDOR_ID_APPLE, 0x0017, 0xff00, apple_macio_init),
+    PCI_DEVICE_CLASS(PCI_VENDOR_ID_APPLE, 0x0022, 0xff00, apple_macio_init),
+
+    /* PCI bridge */
+    PCI_DEVICE_CLASS(PCI_ANY_ID, PCI_ANY_ID, PCI_CLASS_BRIDGE_PCI,
+                     pci_bios_init_device_bridge),
+
+    /* default */
+    PCI_DEVICE(PCI_ANY_ID, PCI_ANY_ID, pci_bios_allocate_regions),
+
+    PCI_DEVICE_END,
+};
+
+static const struct pci_device_id pci_device_tbl[] = {
+    /* PIIX4 Power Management device (for ACPI) */
+    PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371AB_3,
+               piix4_pm_init),
+
+    PCI_DEVICE_END,
+};
+
 static void pci_bios_init_device(u16 bdf)
 {
-    int class;
     int pin, pic_irq, vendor_id, device_id;
 
-    class = pci_config_readw(bdf, PCI_CLASS_DEVICE);
     vendor_id = pci_config_readw(bdf, PCI_VENDOR_ID);
     device_id = pci_config_readw(bdf, PCI_DEVICE_ID);
     dprintf(1, "PCI: bus=%d devfn=0x%02x: vendor_id=0x%04x device_id=0x%04x\n"
             , pci_bdf_to_bus(bdf), pci_bdf_to_devfn(bdf), vendor_id, device_id);
-    switch (class) {
-    case PCI_CLASS_STORAGE_IDE:
-        if (vendor_id == PCI_VENDOR_ID_INTEL
-            && (device_id == PCI_DEVICE_ID_INTEL_82371SB_1
-                || device_id == PCI_DEVICE_ID_INTEL_82371AB)) {
-            /* PIIX3/PIIX4 IDE */
-            pci_config_writew(bdf, 0x40, 0x8000); // enable IDE0
-            pci_config_writew(bdf, 0x42, 0x8000); // enable IDE1
-            pci_bios_allocate_regions(bdf);
-        } else {
-            /* IDE: we map it as in ISA mode */
-            pci_set_io_region_addr(bdf, 0, PORT_ATA1_CMD_BASE);
-            pci_set_io_region_addr(bdf, 1, PORT_ATA1_CTRL_BASE);
-            pci_set_io_region_addr(bdf, 2, PORT_ATA2_CMD_BASE);
-            pci_set_io_region_addr(bdf, 3, PORT_ATA2_CTRL_BASE);
-        }
-        break;
-    case PCI_CLASS_SYSTEM_PIC:
-        /* PIC */
-        if (vendor_id == PCI_VENDOR_ID_IBM) {
-            /* IBM */
-            if (device_id == 0x0046 || device_id == 0xFFFF) {
-                /* MPIC & MPIC2 */
-                pci_set_io_region_addr(bdf, 0, 0x80800000 + 0x00040000);
-            }
-        }
-        break;
-    case 0xff00:
-        if (vendor_id == PCI_VENDOR_ID_APPLE &&
-            (device_id == 0x0017 || device_id == 0x0022)) {
-            /* macio bridge */
-            pci_set_io_region_addr(bdf, 0, 0x80800000);
-        }
-        break;
-    case PCI_CLASS_BRIDGE_PCI:
-        pci_bios_init_device_bridge(bdf);
-        break;
-    default:
-        /* default memory mappings */
-        pci_bios_allocate_regions(bdf);
-        break;
-    }
+    pci_init_device(pci_class_tbl, bdf, NULL);
 
     /* enable memory mappings */
     pci_config_maskw(bdf, PCI_COMMAND, 0, PCI_COMMAND_IO | PCI_COMMAND_MEMORY);
@@ -327,18 +324,7 @@ static void pci_bios_init_device(u16 bdf)
         pci_config_writeb(bdf, PCI_INTERRUPT_LINE, pic_irq);
     }
 
-    if (vendor_id == PCI_VENDOR_ID_INTEL
-        && device_id == PCI_DEVICE_ID_INTEL_82371AB_3) {
-        /* PIIX4 Power Management device (for ACPI) */
-
-        // acpi sci is hardwired to 9
-        pci_config_writeb(bdf, PCI_INTERRUPT_LINE, 9);
-
-        pci_config_writel(bdf, 0x40, PORT_ACPI_PM_BASE | 1);
-        pci_config_writeb(bdf, 0x80, 0x01); /* enable PM io space */
-        pci_config_writel(bdf, 0x90, PORT_SMB_BASE | 1);
-        pci_config_writeb(bdf, 0xd2, 0x09); /* enable SMBus io space */
-    }
+    pci_init_device(pci_device_tbl, bdf, NULL);
 }
 
 static void pci_bios_init_device_in_bus(int bus)
@@ -434,7 +420,7 @@ pci_setup(void)
 
     int bdf, max;
     foreachpci(bdf, max) {
-        pci_bios_init_bridges(bdf);
+        pci_init_device(pci_isa_bridge_tbl, bdf, NULL);
     }
     pci_bios_init_device_in_bus(0 /* host bus */);
 }
-- 
1.7.1.1

[Qemu-devel] Re: [SeaBIOS] [PATCH v2 2/2] seabios: pciinit: use pci device initializer helper function.

[Kevin O'Connor]

On Wed, Jul 07, 2010 at 12:14:02PM +0900, Isaku Yamahata wrote:
> This patch makes use of pci device initialization helper function
> to convert if/switch clause to table driven.
> So this makes it easier to add q35 initialization code.
> 
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>

I committed the first patch.  I committed the second patch after
fixing up the compile on it.

You're likely seeing " Working around non-functional -combine" printed
during the build.  When in this mode the build textually includes all
the .c files in order to still take advantage of the -fwhole-program
optimization.  Unfortunately in this mode, improper includes can slip
through.

You might want to try grabbing a newer version of gcc.  I believe both
the latest gcc v4.4 and gcc v4.5 work correctly.

-Kevin

[Qemu-devel] Re: [SeaBIOS] [PATCH v2 2/2] seabios: pciinit: use pci device initializer helper function.

[Isaku Yamahata]

On Sat, Jul 10, 2010 at 01:14:06PM -0400, Kevin O'Connor wrote:
> On Wed, Jul 07, 2010 at 12:14:02PM +0900, Isaku Yamahata wrote:
> > This patch makes use of pci device initialization helper function
> > to convert if/switch clause to table driven.
> > So this makes it easier to add q35 initialization code.
> > 
> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> 
> I committed the first patch.  I committed the second patch after
> fixing up the compile on it.

Thank you for fixing the patch.


> You're likely seeing " Working around non-functional -combine" printed
> during the build.  When in this mode the build textually includes all
> the .c files in order to still take advantage of the -fwhole-program
> optimization.  Unfortunately in this mode, improper includes can slip
> through.
> 
> You might want to try grabbing a newer version of gcc.  I believe both
> the latest gcc v4.4 and gcc v4.5 work correctly.

Yes, the message was printed out.
I've upgraded gcc from v4.3.4 to v4.4.4 and confirmed the error.
Future patches will be tested with gcc v4.4.
-- 
yamahata