[Qemu-devel] eepro100: New patches

[Qemu-devel] eepro100: New patches

[Stefan Weil]

These patches fix two regressions (1, 9) which made eepro100 rather useless,
use a simple method to handle the different device variants (2),
add a new device variant (4) and fix or clean some smaller issues.

[PATCH 1/9] eepro100: Don't allow writing SCBStatus
[PATCH 2/9] eepro100: Simplify status handling
[PATCH 3/9] eepro100: Simplified device instantiation
[PATCH 4/9] eepro100: Add new device variant i82801
[PATCH 5/9] eepro100: Set configuration bit for standard TCB
[PATCH 6/9] eepro100: Support compilation without EEPROM
[PATCH 7/9] eepro100: Set power management capability using pci_reserve_capability
[PATCH 8/9] eepro100: Fix mapping of flash memory
[PATCH 9/9] eepro100: Fix PCI interrupt pin configuration regression

Regards,
Stefan

[Qemu-devel] [PATCH 1/9] eepro100: Don't allow writing SCBStatus

[Stefan Weil]

SCBStatus is readonly, but most drivers which were derived
from the old Linux eepro100.c do a word write to this address
when they want to acknowledge interrupts.

So we have to mask these writes here.

The patch also removes old unused code for status read / write.

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 hw/eepro100.c |   49 +++++--------------------------------------------
 1 files changed, 5 insertions(+), 44 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index 7db6fb5..0415132 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -237,10 +237,6 @@ typedef struct {
     /* Statistical counters. Also used for wake-up packet (i82559). */
     eepro100_stats_t statistics;
 
-#if 0
-    uint16_t status;
-#endif
-
     /* Configuration bytes. */
     uint8_t configuration[22];
 
@@ -693,21 +689,6 @@ static char *regname(uint32_t addr)
 }
 #endif                          /* DEBUG_EEPRO100 */
 
-#if 0
-static uint16_t eepro100_read_status(EEPRO100State * s)
-{
-    uint16_t val = s->status;
-    TRACE(OTHER, logout("val=0x%04x\n", val));
-    return val;
-}
-
-static void eepro100_write_status(EEPRO100State * s, uint16_t val)
-{
-    TRACE(OTHER, logout("val=0x%04x\n", val));
-    s->status = val;
-}
-#endif
-
 /*****************************************************************************
  *
  * Command emulation.
@@ -1364,15 +1345,7 @@ static uint8_t eepro100_read1(EEPRO100State * s, uint32_t addr)
 
     switch (addr) {
     case SCBStatus:
-#if 0
-        val = eepro100_read_status(s);
-#endif
-        TRACE(OTHER, logout("addr=%s val=0x%02x\n", regname(addr), val));
-        break;
     case SCBAck:
-#if 0
-        val = eepro100_read_status(s);
-#endif
         TRACE(OTHER, logout("addr=%s val=0x%02x\n", regname(addr), val));
         break;
     case SCBCmd:
@@ -1415,9 +1388,6 @@ static uint16_t eepro100_read2(EEPRO100State * s, uint32_t addr)
 
     switch (addr) {
     case SCBStatus:
-#if 0
-        val = eepro100_read_status(s);
-#endif
     case SCBCmd:
         TRACE(OTHER, logout("addr=%s val=0x%04x\n", regname(addr), val));
         break;
@@ -1441,9 +1411,6 @@ static uint32_t eepro100_read4(EEPRO100State * s, uint32_t addr)
 
     switch (addr) {
     case SCBStatus:
-#if 0
-        val = eepro100_read_status(s);
-#endif
         TRACE(OTHER, logout("addr=%s val=0x%08x\n", regname(addr), val));
         break;
     case SCBPointer:
@@ -1468,7 +1435,8 @@ static uint32_t eepro100_read4(EEPRO100State * s, uint32_t addr)
 
 static void eepro100_write1(EEPRO100State * s, uint32_t addr, uint8_t val)
 {
-    if (addr <= sizeof(s->mem) - sizeof(val)) {
+    /* SCBStatus is readonly. */
+    if (addr > SCBStatus && addr <= sizeof(s->mem) - sizeof(val)) {
         memcpy(&s->mem[addr], &val, sizeof(val));
     }
 
@@ -1476,9 +1444,6 @@ static void eepro100_write1(EEPRO100State * s, uint32_t addr, uint8_t val)
 
     switch (addr) {
     case SCBStatus:
-#if 0
-        eepro100_write_status(s, val);
-#endif
         break;
     case SCBAck:
         eepro100_acknowledge(s);
@@ -1510,7 +1475,8 @@ static void eepro100_write1(EEPRO100State * s, uint32_t addr, uint8_t val)
 
 static void eepro100_write2(EEPRO100State * s, uint32_t addr, uint16_t val)
 {
-    if (addr <= sizeof(s->mem) - sizeof(val)) {
+    /* SCBStatus is readonly. */
+    if (addr > SCBStatus && addr <= sizeof(s->mem) - sizeof(val)) {
         memcpy(&s->mem[addr], &val, sizeof(val));
     }
 
@@ -1518,9 +1484,7 @@ static void eepro100_write2(EEPRO100State * s, uint32_t addr, uint16_t val)
 
     switch (addr) {
     case SCBStatus:
-#if 0
-        eepro100_write_status(s, val);
-#endif
+        s->mem[SCBAck] = (val >> 8);
         eepro100_acknowledge(s);
         break;
     case SCBCmd:
@@ -1908,9 +1872,6 @@ static const VMStateDescription vmstate_eepro100 = {
         VMSTATE_UINT32(statistics.fc_rcv_unsupported, EEPRO100State),
         VMSTATE_UINT16(statistics.xmt_tco_frames, EEPRO100State),
         VMSTATE_UINT16(statistics.rcv_tco_frames, EEPRO100State),
-#if 0
-        VMSTATE_UINT16(status, EEPRO100State),
-#endif
         /* Configuration bytes. */
         VMSTATE_BUFFER(configuration, EEPRO100State),
         VMSTATE_END_OF_LIST()
-- 
1.7.0

[Qemu-devel] [PATCH 2/9] eepro100: Simplify status handling

[Stefan Weil]

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 hw/eepro100.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index 0415132..741031c 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -175,6 +175,7 @@ typedef enum {
 } scb_command_bit;
 
 typedef enum {
+    STATUS_NOT_OK = 0,
     STATUS_C = BIT(15),
     STATUS_OK = BIT(13),
 } scb_status_bit;
@@ -882,7 +883,7 @@ static void action_command(EEPRO100State *s)
         bool bit_s;
         bool bit_i;
         bool bit_nc;
-        bool success = true;
+        uint16_t ok_status = STATUS_OK;
         s->cb_address = s->cu_base + s->cu_offset;
         read_cb(s);
         bit_el = ((s->tx.command & COMMAND_EL) != 0);
@@ -915,7 +916,7 @@ static void action_command(EEPRO100State *s)
         case CmdTx:
             if (bit_nc) {
                 missing("CmdTx: NC = 0");
-                success = false;
+                ok_status = STATUS_NOT_OK;
                 break;
             }
             tx_command(s);
@@ -932,11 +933,11 @@ static void action_command(EEPRO100State *s)
             break;
         default:
             missing("undefined command");
-            success = false;
+            ok_status = STATUS_NOT_OK;
             break;
         }
         /* Write new status. */
-        stw_phys(s->cb_address, s->tx.status | STATUS_C | (success ? STATUS_OK : 0));
+        stw_phys(s->cb_address, s->tx.status | ok_status | STATUS_C);
         if (bit_i) {
             /* CU completed action. */
             eepro100_cx_interrupt(s);
-- 
1.7.0

[Qemu-devel] [PATCH 3/9] eepro100: Simplified device instantiation

[Stefan Weil]

By using a private device info structure
(as suggested by Gerd Hoffmann), handling of the
different device variants becomes much easier.

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 hw/eepro100.c |  447 +++++++++++++++++++++------------------------------------
 1 files changed, 167 insertions(+), 280 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index 741031c..fde45c9 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -117,6 +117,16 @@
 #define INT_MASK        0x0100
 #define DRVR_INT        0x0200  /* Driver generated interrupt. */
 
+typedef struct {
+    PCIDeviceInfo pci;
+    uint32_t device;
+    uint16_t device_id;
+    uint8_t revision;
+    uint8_t stats_size;
+    bool has_extended_tcb_support;
+    bool power_management;
+} E100PCIDeviceInfo;
+
 /* Offsets to the various registers.
    All accesses need not be longword aligned. */
 enum speedo_offsets {
@@ -444,136 +454,70 @@ static void eepro100_fcp_interrupt(EEPRO100State * s)
 }
 #endif
 
-static void pci_reset(EEPRO100State * s)
+static void e100_pci_reset(EEPRO100State * s, E100PCIDeviceInfo *e100_device)
 {
+    /* TODO: Use pci_add_capability(&s->dev, PCI_CAP_ID_PM, ...) for PM. */
     uint32_t device = s->device;
     uint8_t *pci_conf = s->dev.config;
-    bool power_management = 1;
 
     TRACE(OTHER, logout("%p\n", s));
 
     /* PCI Vendor ID */
     pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_INTEL);
-    /* PCI Device ID depends on device and is set below. */
+    /* PCI Device ID */
+    pci_config_set_device_id(pci_conf, e100_device->device_id);
     /* PCI Status */
-    pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM | PCI_STATUS_FAST_BACK);
+    if (e100_device->power_management) {
+        pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
+                                            PCI_STATUS_FAST_BACK |
+                                            PCI_STATUS_CAP_LIST);
+    } else {
+        pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
+                                            PCI_STATUS_FAST_BACK);
+    }
     /* PCI Revision ID */
-    pci_set_byte(pci_conf + PCI_REVISION_ID, 0x08);
+    pci_config_set_revision(pci_conf, e100_device->revision);
     pci_config_set_class(pci_conf, PCI_CLASS_NETWORK_ETHERNET);
     /* PCI Latency Timer */
     pci_set_byte(pci_conf + PCI_LATENCY_TIMER, 0x20);   /* latency timer = 32 clocks */
     /* Capability Pointer */
-    /* TODO: revisions with power_management 1 use this but
-     * do not set new capability list bit in status register. */
-    pci_set_byte(pci_conf + PCI_CAPABILITY_LIST, 0xdc);
+    if (e100_device->power_management) {
+        pci_set_byte(pci_conf + PCI_CAPABILITY_LIST, 0xdc);
+    } else {
+        pci_set_byte(pci_conf + PCI_CAPABILITY_LIST, 0x00);
+    }
     /* Minimum Grant */
     pci_set_byte(pci_conf + PCI_MIN_GNT, 0x08);
     /* Maximum Latency */
     pci_set_byte(pci_conf + PCI_MAX_LAT, 0x18);
 
+    s->stats_size = e100_device->stats_size;
+    s->has_extended_tcb_support = e100_device->has_extended_tcb_support;
+
     switch (device) {
     case i82550:
-        /* TODO: check device id. */
-        pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82551IT);
-        /* Revision ID: 0x0c, 0x0d, 0x0e. */
-        pci_set_byte(pci_conf + PCI_REVISION_ID, 0x0e);
-        /* TODO: check size of statistical counters. */
-        s->stats_size = 80;
-        /* TODO: check extended tcb support. */
-        s->has_extended_tcb_support = 1;
-        break;
     case i82551:
-        pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82551IT);
-        /* Revision ID: 0x0f, 0x10. */
-        pci_set_byte(pci_conf + PCI_REVISION_ID, 0x0f);
-        /* TODO: check size of statistical counters. */
-        s->stats_size = 80;
-        s->has_extended_tcb_support = 1;
-        break;
     case i82557A:
-        pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82557);
-        pci_set_byte(pci_conf + PCI_REVISION_ID, 0x01);
-        pci_set_byte(pci_conf + PCI_CAPABILITY_LIST, 0x00);
-        power_management = 0;
-        break;
     case i82557B:
-        pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82557);
-        pci_set_byte(pci_conf + PCI_REVISION_ID, 0x02);
-        pci_set_byte(pci_conf + PCI_CAPABILITY_LIST, 0x00);
-        power_management = 0;
-        break;
     case i82557C:
-        pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82557);
-        pci_set_byte(pci_conf + PCI_REVISION_ID, 0x03);
-        pci_set_byte(pci_conf + PCI_CAPABILITY_LIST, 0x00);
-        power_management = 0;
-        break;
     case i82558A:
-        pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82557);
-        pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
-                                  PCI_STATUS_FAST_BACK | PCI_STATUS_CAP_LIST);
-        pci_set_byte(pci_conf + PCI_REVISION_ID, 0x04);
-        s->stats_size = 76;
-        s->has_extended_tcb_support = 1;
-        break;
     case i82558B:
-        pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82557);
-        pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
-                                  PCI_STATUS_FAST_BACK | PCI_STATUS_CAP_LIST);
-        pci_set_byte(pci_conf + PCI_REVISION_ID, 0x05);
-        s->stats_size = 76;
-        s->has_extended_tcb_support = 1;
-        break;
     case i82559A:
-        pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82557);
-        pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
-                                  PCI_STATUS_FAST_BACK | PCI_STATUS_CAP_LIST);
-        pci_set_byte(pci_conf + PCI_REVISION_ID, 0x06);
-        s->stats_size = 80;
-        s->has_extended_tcb_support = 1;
-        break;
     case i82559B:
-        pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82557);
-        pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
-                                  PCI_STATUS_FAST_BACK | PCI_STATUS_CAP_LIST);
-        pci_set_byte(pci_conf + PCI_REVISION_ID, 0x07);
-        s->stats_size = 80;
-        s->has_extended_tcb_support = 1;
+    case i82559ER:
+    case i82562:
         break;
     case i82559C:
-        pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82557);
-        pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
-                                  PCI_STATUS_FAST_BACK | PCI_STATUS_CAP_LIST);
-        pci_set_byte(pci_conf + PCI_REVISION_ID, 0x08);
-        /* TODO: Windows wants revision id 0x0c. */
-        pci_set_byte(pci_conf + PCI_REVISION_ID, 0x0c);
 #if EEPROM_SIZE > 0
-        pci_set_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID, 0x8086);
+        pci_set_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID, PCI_VENDOR_ID_INTEL);
         pci_set_word(pci_conf + PCI_SUBSYSTEM_ID, 0x0040);
 #endif
-        s->stats_size = 80;
-        s->has_extended_tcb_support = 1;
-        break;
-    case i82559ER:
-        pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82551IT);
-        pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
-                                  PCI_STATUS_FAST_BACK | PCI_STATUS_CAP_LIST);
-        pci_set_byte(pci_conf + PCI_REVISION_ID, 0x09);
-        s->stats_size = 80;
-        s->has_extended_tcb_support = 1;
-        break;
-    case i82562:
-        /* TODO: check device id. */
-        pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_INTEL_82551IT);
-        /* TODO: wrong revision id. */
-        pci_set_byte(pci_conf + PCI_REVISION_ID, 0x0e);
-        s->stats_size = 80;
-        s->has_extended_tcb_support = 1;
         break;
     default:
         logout("Device %X is undefined!\n", device);
     }
 
+    /* Standard statistical counters. */
     s->configuration[6] |= BIT(5);
 
     if (s->stats_size == 80) {
@@ -598,9 +542,9 @@ static void pci_reset(EEPRO100State * s)
     }
     assert(s->stats_size > 0 && s->stats_size <= sizeof(s->statistics));
 
-    if (power_management) {
+    if (e100_device->power_management) {
         /* Power Management Capabilities */
-        pci_set_byte(pci_conf + 0xdc, 0x01);
+        pci_set_byte(pci_conf + 0xdc, PCI_CAP_ID_PM);
         /* Next Item Pointer */
         /* Capability ID */
         pci_set_word(pci_conf + 0xde, 0x7e21);
@@ -1905,15 +1849,17 @@ static NetClientInfo net_eepro100_info = {
     .cleanup = nic_cleanup,
 };
 
-static int nic_init(PCIDevice *pci_dev, uint32_t device)
+static int e100_nic_init(PCIDevice *pci_dev)
 {
     EEPRO100State *s = DO_UPCAST(EEPRO100State, dev, pci_dev);
+    E100PCIDeviceInfo *e100_device = DO_UPCAST(E100PCIDeviceInfo, pci.qdev,
+                                               pci_dev->qdev.info);
 
     TRACE(OTHER, logout("\n"));
 
-    s->device = device;
+    s->device = e100_device->device;
 
-    pci_reset(s);
+    e100_pci_reset(s, e100_device);
 
     /* Add 64 * 2 EEPROM. i82557 and i82558 support a 64 word EEPROM,
      * i82559 and later support 64 or 256 word EEPROM. */
@@ -1953,207 +1899,148 @@ static int nic_init(PCIDevice *pci_dev, uint32_t device)
     return 0;
 }
 
-static int pci_i82550_init(PCIDevice *pci_dev)
-{
-    return nic_init(pci_dev, i82550);
-}
-
-static int pci_i82551_init(PCIDevice *pci_dev)
-{
-    return nic_init(pci_dev, i82551);
-}
-
-static int pci_i82557a_init(PCIDevice *pci_dev)
-{
-    return nic_init(pci_dev, i82557A);
-}
-
-static int pci_i82557b_init(PCIDevice *pci_dev)
-{
-    return nic_init(pci_dev, i82557B);
-}
-
-static int pci_i82557c_init(PCIDevice *pci_dev)
-{
-    return nic_init(pci_dev, i82557C);
-}
-
-static int pci_i82558a_init(PCIDevice *pci_dev)
-{
-    return nic_init(pci_dev, i82558A);
-}
-
-static int pci_i82558b_init(PCIDevice *pci_dev)
-{
-    return nic_init(pci_dev, i82558B);
-}
-
-static int pci_i82559a_init(PCIDevice *pci_dev)
-{
-    return nic_init(pci_dev, i82559A);
-}
-
-static int pci_i82559b_init(PCIDevice *pci_dev)
-{
-    return nic_init(pci_dev, i82559B);
-}
-
-static int pci_i82559c_init(PCIDevice *pci_dev)
-{
-    return nic_init(pci_dev, i82559C);
-}
-
-static int pci_i82559er_init(PCIDevice *pci_dev)
-{
-    return nic_init(pci_dev, i82559ER);
-}
-
-static int pci_i82562_init(PCIDevice *pci_dev)
-{
-    return nic_init(pci_dev, i82562);
-}
-
-static PCIDeviceInfo eepro100_info[] = {
+static E100PCIDeviceInfo e100_devices[] = {
     {
-        .qdev.name = "i82550",
-        .qdev.desc = "Intel i82550 Ethernet",
-        .qdev.size = sizeof(EEPRO100State),
-        .init      = pci_i82550_init,
-        .exit      = pci_nic_uninit,
-        .romfile   = "gpxe-eepro100-80861209.rom",
-        .qdev.props = (Property[]) {
-            DEFINE_NIC_PROPERTIES(EEPRO100State, conf),
-            DEFINE_PROP_END_OF_LIST(),
-        },
-    },{
-        .qdev.name = "i82551",
-        .qdev.desc = "Intel i82551 Ethernet",
-        .qdev.size = sizeof(EEPRO100State),
-        .init      = pci_i82551_init,
-        .exit      = pci_nic_uninit,
-        .romfile   = "gpxe-eepro100-80861209.rom",
-        .qdev.props = (Property[]) {
-            DEFINE_NIC_PROPERTIES(EEPRO100State, conf),
-            DEFINE_PROP_END_OF_LIST(),
-        },
+        .pci.qdev.name = "i82550",
+        .pci.qdev.desc = "Intel i82550 Ethernet",
+        .device = i82550,
+        /* TODO: check device id. */
+        .device_id = PCI_DEVICE_ID_INTEL_82551IT,
+        /* Revision ID: 0x0c, 0x0d, 0x0e. */
+        .revision = 0x0e,
+        /* TODO: check size of statistical counters. */
+        .stats_size = 80,
+        /* TODO: check extended tcb support. */
+        .has_extended_tcb_support = true,
+        .power_management = true,
     },{
-        .qdev.name = "i82557a",
-        .qdev.desc = "Intel i82557A Ethernet",
-        .qdev.size = sizeof(EEPRO100State),
-        .init      = pci_i82557a_init,
-        .exit      = pci_nic_uninit,
-        .romfile   = "gpxe-eepro100-80861229.rom",
-        .qdev.props = (Property[]) {
-            DEFINE_NIC_PROPERTIES(EEPRO100State, conf),
-            DEFINE_PROP_END_OF_LIST(),
-        },
+        .pci.qdev.name = "i82551",
+        .pci.qdev.desc = "Intel i82551 Ethernet",
+        .device = i82551,
+        .device_id = PCI_DEVICE_ID_INTEL_82551IT,
+        /* Revision ID: 0x0f, 0x10. */
+        .revision = 0x0f,
+        /* TODO: check size of statistical counters. */
+        .stats_size = 80,
+        .has_extended_tcb_support = true,
+        .power_management = true,
     },{
-        .qdev.name = "i82557b",
-        .qdev.desc = "Intel i82557B Ethernet",
-        .qdev.size = sizeof(EEPRO100State),
-        .init      = pci_i82557b_init,
-        .exit      = pci_nic_uninit,
-        .romfile   = "gpxe-eepro100-80861229.rom",
-        .qdev.props = (Property[]) {
-            DEFINE_NIC_PROPERTIES(EEPRO100State, conf),
-            DEFINE_PROP_END_OF_LIST(),
-        },
+        .pci.qdev.name = "i82557a",
+        .pci.qdev.desc = "Intel i82557A Ethernet",
+        .device = i82557A,
+        .device_id = PCI_DEVICE_ID_INTEL_82557,
+        .revision = 0x01,
+        .power_management = false,
     },{
-        .qdev.name = "i82557c",
-        .qdev.desc = "Intel i82557C Ethernet",
-        .qdev.size = sizeof(EEPRO100State),
-        .init      = pci_i82557c_init,
-        .exit      = pci_nic_uninit,
-        .romfile   = "gpxe-eepro100-80861229.rom",
-        .qdev.props = (Property[]) {
-            DEFINE_NIC_PROPERTIES(EEPRO100State, conf),
-            DEFINE_PROP_END_OF_LIST(),
-        },
+        .pci.qdev.name = "i82557b",
+        .pci.qdev.desc = "Intel i82557B Ethernet",
+        .device = i82557B,
+        .device_id = PCI_DEVICE_ID_INTEL_82557,
+        .revision = 0x02,
+        .power_management = false,
     },{
-        .qdev.name = "i82558a",
-        .qdev.desc = "Intel i82558A Ethernet",
-        .qdev.size = sizeof(EEPRO100State),
-        .init      = pci_i82558a_init,
-        .exit      = pci_nic_uninit,
-        .romfile   = "gpxe-eepro100-80861229.rom",
-        .qdev.props = (Property[]) {
-            DEFINE_NIC_PROPERTIES(EEPRO100State, conf),
-            DEFINE_PROP_END_OF_LIST(),
-        },
+        .pci.qdev.name = "i82557c",
+        .pci.qdev.desc = "Intel i82557C Ethernet",
+        .device = i82557C,
+        .device_id = PCI_DEVICE_ID_INTEL_82557,
+        .revision = 0x03,
+        .power_management = false,
     },{
-        .qdev.name = "i82558b",
-        .qdev.desc = "Intel i82558B Ethernet",
-        .qdev.size = sizeof(EEPRO100State),
-        .init      = pci_i82558b_init,
-        .exit      = pci_nic_uninit,
-        .romfile   = "gpxe-eepro100-80861229.rom",
-        .qdev.props = (Property[]) {
-            DEFINE_NIC_PROPERTIES(EEPRO100State, conf),
-            DEFINE_PROP_END_OF_LIST(),
-        },
+        .pci.qdev.name = "i82558a",
+        .pci.qdev.desc = "Intel i82558A Ethernet",
+        .device = i82558A,
+        .device_id = PCI_DEVICE_ID_INTEL_82557,
+        .revision = 0x04,
+        .stats_size = 76,
+        .has_extended_tcb_support = true,
+        .power_management = true,
     },{
-        .qdev.name = "i82559a",
-        .qdev.desc = "Intel i82559A Ethernet",
-        .qdev.size = sizeof(EEPRO100State),
-        .init      = pci_i82559a_init,
-        .exit      = pci_nic_uninit,
-        .romfile   = "gpxe-eepro100-80861229.rom",
-        .qdev.props = (Property[]) {
-            DEFINE_NIC_PROPERTIES(EEPRO100State, conf),
-            DEFINE_PROP_END_OF_LIST(),
-        },
+        .pci.qdev.name = "i82558b",
+        .pci.qdev.desc = "Intel i82558B Ethernet",
+        .device = i82558B,
+        .device_id = PCI_DEVICE_ID_INTEL_82557,
+        .revision = 0x05,
+        .stats_size = 76,
+        .has_extended_tcb_support = true,
+        .power_management = true,
     },{
-        .qdev.name = "i82559b",
-        .qdev.desc = "Intel i82559B Ethernet",
-        .qdev.size = sizeof(EEPRO100State),
-        .init      = pci_i82559b_init,
-        .exit      = pci_nic_uninit,
-        .romfile   = "gpxe-eepro100-80861229.rom",
-        .qdev.props = (Property[]) {
-            DEFINE_NIC_PROPERTIES(EEPRO100State, conf),
-            DEFINE_PROP_END_OF_LIST(),
-        },
+        .pci.qdev.name = "i82559a",
+        .pci.qdev.desc = "Intel i82559A Ethernet",
+        .device = i82559A,
+        .device_id = PCI_DEVICE_ID_INTEL_82557,
+        .revision = 0x06,
+        .stats_size = 80,
+        .has_extended_tcb_support = true,
+        .power_management = true,
     },{
-        .qdev.name = "i82559c",
-        .qdev.desc = "Intel i82559C Ethernet",
-        .qdev.size = sizeof(EEPRO100State),
-        .init      = pci_i82559c_init,
-        .exit      = pci_nic_uninit,
-        .romfile   = "gpxe-eepro100-80861229.rom",
-        .qdev.props = (Property[]) {
-            DEFINE_NIC_PROPERTIES(EEPRO100State, conf),
-            DEFINE_PROP_END_OF_LIST(),
-        },
+        .pci.qdev.name = "i82559b",
+        .pci.qdev.desc = "Intel i82559B Ethernet",
+        .device = i82559B,
+        .device_id = PCI_DEVICE_ID_INTEL_82557,
+        .revision = 0x07,
+        .stats_size = 80,
+        .has_extended_tcb_support = true,
+        .power_management = true,
     },{
-        .qdev.name = "i82559er",
-        .qdev.desc = "Intel i82559ER Ethernet",
-        .qdev.size = sizeof(EEPRO100State),
-        .init      = pci_i82559er_init,
-        .exit      = pci_nic_uninit,
-        .romfile   = "gpxe-eepro100-80861209.rom",
-        .qdev.props = (Property[]) {
-            DEFINE_NIC_PROPERTIES(EEPRO100State, conf),
-            DEFINE_PROP_END_OF_LIST(),
-        },
+        .pci.qdev.name = "i82559c",
+        .pci.qdev.desc = "Intel i82559C Ethernet",
+        .device = i82559C,
+        .device_id = PCI_DEVICE_ID_INTEL_82557,
+#if 0
+        .revision = 0x08,
+#endif
+        /* TODO: Windows wants revision id 0x0c. */
+        .revision = 0x0c,
+        .stats_size = 80,
+        .has_extended_tcb_support = true,
+        .power_management = true,
     },{
-        .qdev.name = "i82562",
-        .qdev.desc = "Intel i82562 Ethernet",
-        .qdev.size = sizeof(EEPRO100State),
-        .init      = pci_i82562_init,
-        .exit      = pci_nic_uninit,
-        .romfile   = "gpxe-eepro100-80861209.rom",
-        .qdev.props = (Property[]) {
-            DEFINE_NIC_PROPERTIES(EEPRO100State, conf),
-            DEFINE_PROP_END_OF_LIST(),
-        },
+        .pci.qdev.name = "i82559er",
+        .pci.qdev.desc = "Intel i82559ER Ethernet",
+        .device = i82559ER,
+        .device_id = PCI_DEVICE_ID_INTEL_82551IT,
+        .revision = 0x09,
+        .stats_size = 80,
+        .has_extended_tcb_support = true,
+        .power_management = true,
     },{
-        /* end of list */
+        .pci.qdev.name = "i82562",
+        .pci.qdev.desc = "Intel i82562 Ethernet",
+        .device = i82562,
+        /* TODO: check device id. */
+        .device_id = PCI_DEVICE_ID_INTEL_82551IT,
+        /* TODO: wrong revision id. */
+        .revision = 0x0e,
+        .stats_size = 80,
+        .has_extended_tcb_support = true,
+        .power_management = true,
     }
 };
 
+static Property e100_properties[] = {
+    DEFINE_NIC_PROPERTIES(EEPRO100State, conf),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void eepro100_register_devices(void)
 {
-    pci_qdev_register_many(eepro100_info);
+    size_t i;
+    for (i = 0; i < ARRAY_SIZE(e100_devices); i++) {
+        PCIDeviceInfo *pci_dev = &e100_devices[i].pci;
+        switch (e100_devices[i].device_id) {
+            case PCI_DEVICE_ID_INTEL_82551IT:
+                pci_dev->romfile = "gpxe-eepro100-80861209.rom";
+                break;
+            case PCI_DEVICE_ID_INTEL_82557:
+                pci_dev->romfile = "gpxe-eepro100-80861229.rom";
+                break;
+        }
+        pci_dev->init = e100_nic_init;
+        pci_dev->exit = pci_nic_uninit;
+        pci_dev->qdev.props = e100_properties;
+        pci_dev->qdev.size = sizeof(EEPRO100State);
+        pci_qdev_register(pci_dev);
+    }
 }
 
 device_init(eepro100_register_devices)
-- 
1.7.0

[Qemu-devel] [PATCH 4/9] eepro100: Add new device variant i82801

[Stefan Weil]

This ethernet device is used in Toshiba Tecra 8200 notebooks.

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 hw/eepro100.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index fde45c9..52c5888 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -87,6 +87,7 @@
 #define i82559C         0x82559c
 #define i82559ER        0x82559e
 #define i82562          0x82562
+#define i82801          0x82801
 
 /* Use 64 word EEPROM. TODO: could be a runtime option. */
 #define EEPROM_SIZE     64
@@ -506,6 +507,7 @@ static void e100_pci_reset(EEPRO100State * s, E100PCIDeviceInfo *e100_device)
     case i82559B:
     case i82559ER:
     case i82562:
+    case i82801:
         break;
     case i82559C:
 #if EEPROM_SIZE > 0
@@ -2014,6 +2016,16 @@ static E100PCIDeviceInfo e100_devices[] = {
         .stats_size = 80,
         .has_extended_tcb_support = true,
         .power_management = true,
+    },{
+        /* Toshiba Tecra 8200. */
+        .pci.qdev.name = "i82801",
+        .pci.qdev.desc = "Intel i82801 Ethernet",
+        .device = i82801,
+        .device_id = 0x2449,
+        .revision = 0x03,
+        .stats_size = 80,
+        .has_extended_tcb_support = true,
+        .power_management = true,
     }
 };
 
@@ -2034,6 +2046,9 @@ static void eepro100_register_devices(void)
             case PCI_DEVICE_ID_INTEL_82557:
                 pci_dev->romfile = "gpxe-eepro100-80861229.rom";
                 break;
+            case 0x2449:
+                pci_dev->romfile = "gpxe-eepro100-80862449.rom";
+                break;
         }
         pci_dev->init = e100_nic_init;
         pci_dev->exit = pci_nic_uninit;
-- 
1.7.0

[Qemu-devel] [PATCH 5/9] eepro100: Set configuration bit for standard TCB

[Stefan Weil]

For some devices, this bit is always set.
For the others, it is set by default.

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 hw/eepro100.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index 52c5888..cedc427 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -519,6 +519,9 @@ static void e100_pci_reset(EEPRO100State * s, E100PCIDeviceInfo *e100_device)
         logout("Device %X is undefined!\n", device);
     }
 
+    /* Standard TxCB. */
+    s->configuration[6] |= BIT(4);
+
     /* Standard statistical counters. */
     s->configuration[6] |= BIT(5);
 
-- 
1.7.0

[Qemu-devel] [PATCH 6/9] eepro100: Support compilation without EEPROM

[Stefan Weil]

To emulate hardware without an EEPROM,
EEPROM_SIZE may be set to 0.

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 hw/eepro100.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index cedc427..e12ee23 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -1866,9 +1866,11 @@ static int e100_nic_init(PCIDevice *pci_dev)
 
     e100_pci_reset(s, e100_device);
 
+#if EEPROM_SIZE > 0
     /* Add 64 * 2 EEPROM. i82557 and i82558 support a 64 word EEPROM,
      * i82559 and later support 64 or 256 word EEPROM. */
     s->eeprom = eeprom93xx_new(EEPROM_SIZE);
+#endif
 
     /* Handler for memory-mapped I/O */
     s->mmio_index =
-- 
1.7.0

[Qemu-devel] [PATCH 7/9] eepro100: Set power management capability using pci_reserve_capability

[Stefan Weil]

pci_reserve_capability automatically updates PCI status and
PCI capability pointer, so use it.

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 hw/eepro100.c |   39 ++++++++++++++++++---------------------
 1 files changed, 18 insertions(+), 21 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index e12ee23..f0acdbc 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -457,7 +457,6 @@ static void eepro100_fcp_interrupt(EEPRO100State * s)
 
 static void e100_pci_reset(EEPRO100State * s, E100PCIDeviceInfo *e100_device)
 {
-    /* TODO: Use pci_add_capability(&s->dev, PCI_CAP_ID_PM, ...) for PM. */
     uint32_t device = s->device;
     uint8_t *pci_conf = s->dev.config;
 
@@ -468,25 +467,14 @@ static void e100_pci_reset(EEPRO100State * s, E100PCIDeviceInfo *e100_device)
     /* PCI Device ID */
     pci_config_set_device_id(pci_conf, e100_device->device_id);
     /* PCI Status */
-    if (e100_device->power_management) {
-        pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
-                                            PCI_STATUS_FAST_BACK |
-                                            PCI_STATUS_CAP_LIST);
-    } else {
-        pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
-                                            PCI_STATUS_FAST_BACK);
-    }
+    pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
+                                        PCI_STATUS_FAST_BACK);
     /* PCI Revision ID */
     pci_config_set_revision(pci_conf, e100_device->revision);
     pci_config_set_class(pci_conf, PCI_CLASS_NETWORK_ETHERNET);
     /* PCI Latency Timer */
     pci_set_byte(pci_conf + PCI_LATENCY_TIMER, 0x20);   /* latency timer = 32 clocks */
-    /* Capability Pointer */
-    if (e100_device->power_management) {
-        pci_set_byte(pci_conf + PCI_CAPABILITY_LIST, 0xdc);
-    } else {
-        pci_set_byte(pci_conf + PCI_CAPABILITY_LIST, 0x00);
-    }
+    /* Capability Pointer is set by PCI framework. */
     /* Minimum Grant */
     pci_set_byte(pci_conf + PCI_MIN_GNT, 0x08);
     /* Maximum Latency */
@@ -549,12 +537,21 @@ static void e100_pci_reset(EEPRO100State * s, E100PCIDeviceInfo *e100_device)
 
     if (e100_device->power_management) {
         /* Power Management Capabilities */
-        pci_set_byte(pci_conf + 0xdc, PCI_CAP_ID_PM);
-        /* Next Item Pointer */
-        /* Capability ID */
-        pci_set_word(pci_conf + 0xde, 0x7e21);
-        /* TODO: Power Management Control / Status. */
-        /* TODO: Ethernet Power Consumption Registers (i82559 and later). */
+        int cfg_offset;
+        pci_reserve_capability(&s->dev, PCI_CONFIG_HEADER_SIZE,
+                               0xdc - PCI_CONFIG_HEADER_SIZE);
+        cfg_offset = pci_add_capability(&s->dev, PCI_CAP_ID_PM, PCI_PM_SIZEOF);
+        assert(cfg_offset == 0xdc);
+        if (cfg_offset > 0) {
+            /* Power Management Capabilities */
+            pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
+#if 0 /* TODO: replace dummy code for power management emulation. */
+            /* TODO: Power Management Control / Status. */
+            pci_set_word(pci_conf + cfg_offset + PCI_PM_CTRL, 0x0000);
+            /* TODO: Ethernet Power Consumption Registers (i82559 and later). */
+            pci_set_byte(pci_conf + cfg_offset + PCI_PM_PPB_EXTENSIONS, 0x0000);
+#endif
+        }
     }
 
 #if EEPROM_SIZE > 0
-- 
1.7.0

[Qemu-devel] [PATCH 8/9] eepro100: Fix mapping of flash memory

[Stefan Weil]

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 hw/eepro100.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index f0acdbc..2401888 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -1622,8 +1622,9 @@ static void pci_mmio_map(PCIDevice * pci_dev, int region_num,
           "size=0x%08"FMT_PCIBUS", type=%d\n",
           region_num, addr, size, type));
 
-    if (region_num == 0) {
-        /* Map control / status registers. */
+    assert(region_num == 0 || region_num == 2);
+    if (region_num == 0 || region_num == 2) {
+        /* Map control / status registers and flash. */
         cpu_register_physical_memory(addr, size, s->mmio_index);
         s->region[region_num] = addr;
     }
-- 
1.7.0

[Qemu-devel] [PATCH 9/9] eepro100: Fix PCI interrupt pin configuration regression

[Stefan Weil]

Commit 15e89f5916c9e82347cbd1fd416db3e348bab426
removed this setting, but it is still needed.

Without this patch, e100 device drivers using
interrupts don't work with qemu.

See other nic emulations which also set the
PCI interrupt pin.

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 hw/eepro100.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/hw/eepro100.c b/hw/eepro100.c
index 2401888..bc7e3f1 100644
--- a/hw/eepro100.c
+++ b/hw/eepro100.c
@@ -475,6 +475,10 @@ static void e100_pci_reset(EEPRO100State * s, E100PCIDeviceInfo *e100_device)
     /* PCI Latency Timer */
     pci_set_byte(pci_conf + PCI_LATENCY_TIMER, 0x20);   /* latency timer = 32 clocks */
     /* Capability Pointer is set by PCI framework. */
+    /* Interrupt Line */
+    /* Interrupt Pin */
+    /* TODO: RST# value should be 0. */
+    pci_set_byte(pci_conf + PCI_INTERRUPT_PIN, 1);      /* interrupt pin 0 */
     /* Minimum Grant */
     pci_set_byte(pci_conf + PCI_MIN_GNT, 0x08);
     /* Maximum Latency */
-- 
1.7.0

[Qemu-devel] Re: [PATCH 9/9] eepro100: Fix PCI interrupt pin configuration regression

[Michael S. Tsirkin]

On Tue, Apr 06, 2010 at 01:44:09PM +0200, Stefan Weil wrote:
> Commit 15e89f5916c9e82347cbd1fd416db3e348bab426
> removed this setting, but it is still needed.
> 
> Without this patch, e100 device drivers using
> interrupts don't work with qemu.
> 
> See other nic emulations which also set the
> PCI interrupt pin.
> 
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>

I actually misread the spec. We don't need the TODO
either. I'll drop it.

> ---
>  hw/eepro100.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/eepro100.c b/hw/eepro100.c
> index 2401888..bc7e3f1 100644
> --- a/hw/eepro100.c
> +++ b/hw/eepro100.c
> @@ -475,6 +475,10 @@ static void e100_pci_reset(EEPRO100State * s, E100PCIDeviceInfo *e100_device)
>      /* PCI Latency Timer */
>      pci_set_byte(pci_conf + PCI_LATENCY_TIMER, 0x20);   /* latency timer = 32 clocks */
>      /* Capability Pointer is set by PCI framework. */
> +    /* Interrupt Line */
> +    /* Interrupt Pin */
> +    /* TODO: RST# value should be 0. */
> +    pci_set_byte(pci_conf + PCI_INTERRUPT_PIN, 1);      /* interrupt pin 0 */
>      /* Minimum Grant */
>      pci_set_byte(pci_conf + PCI_MIN_GNT, 0x08);
>      /* Maximum Latency */
> -- 
> 1.7.0

[Qemu-devel] Re: [PATCH 8/9] eepro100: Fix mapping of flash memory

[Michael S. Tsirkin]

On Tue, Apr 06, 2010 at 01:44:08PM +0200, Stefan Weil wrote:
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> ---
>  hw/eepro100.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/eepro100.c b/hw/eepro100.c
> index f0acdbc..2401888 100644
> --- a/hw/eepro100.c
> +++ b/hw/eepro100.c
> @@ -1622,8 +1622,9 @@ static void pci_mmio_map(PCIDevice * pci_dev, int region_num,
>            "size=0x%08"FMT_PCIBUS", type=%d\n",
>            region_num, addr, size, type));
>  
> -    if (region_num == 0) {
> -        /* Map control / status registers. */
> +    assert(region_num == 0 || region_num == 2);
> +    if (region_num == 0 || region_num == 2) {

Looks a bit strange ...  Why do we need the if here?

> +        /* Map control / status registers and flash. */
>          cpu_register_physical_memory(addr, size, s->mmio_index);
>          s->region[region_num] = addr;
>      }
> -- 
> 1.7.0

[Qemu-devel] Re: [PATCH 7/9] eepro100: Set power management capability using pci_reserve_capability

[Michael S. Tsirkin]

On Tue, Apr 06, 2010 at 01:44:07PM +0200, Stefan Weil wrote:
> pci_reserve_capability automatically updates PCI status and
> PCI capability pointer, so use it.
> 
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> ---
>  hw/eepro100.c |   39 ++++++++++++++++++---------------------
>  1 files changed, 18 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/eepro100.c b/hw/eepro100.c
> index e12ee23..f0acdbc 100644
> --- a/hw/eepro100.c
> +++ b/hw/eepro100.c
> @@ -457,7 +457,6 @@ static void eepro100_fcp_interrupt(EEPRO100State * s)
>  
>  static void e100_pci_reset(EEPRO100State * s, E100PCIDeviceInfo *e100_device)
>  {
> -    /* TODO: Use pci_add_capability(&s->dev, PCI_CAP_ID_PM, ...) for PM. */
>      uint32_t device = s->device;
>      uint8_t *pci_conf = s->dev.config;
>  
> @@ -468,25 +467,14 @@ static void e100_pci_reset(EEPRO100State * s, E100PCIDeviceInfo *e100_device)
>      /* PCI Device ID */
>      pci_config_set_device_id(pci_conf, e100_device->device_id);
>      /* PCI Status */
> -    if (e100_device->power_management) {
> -        pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
> -                                            PCI_STATUS_FAST_BACK |
> -                                            PCI_STATUS_CAP_LIST);
> -    } else {
> -        pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
> -                                            PCI_STATUS_FAST_BACK);
> -    }
> +    pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM |
> +                                        PCI_STATUS_FAST_BACK);
>      /* PCI Revision ID */
>      pci_config_set_revision(pci_conf, e100_device->revision);
>      pci_config_set_class(pci_conf, PCI_CLASS_NETWORK_ETHERNET);
>      /* PCI Latency Timer */
>      pci_set_byte(pci_conf + PCI_LATENCY_TIMER, 0x20);   /* latency timer = 32 clocks */
> -    /* Capability Pointer */
> -    if (e100_device->power_management) {
> -        pci_set_byte(pci_conf + PCI_CAPABILITY_LIST, 0xdc);
> -    } else {
> -        pci_set_byte(pci_conf + PCI_CAPABILITY_LIST, 0x00);
> -    }
> +    /* Capability Pointer is set by PCI framework. */
>      /* Minimum Grant */
>      pci_set_byte(pci_conf + PCI_MIN_GNT, 0x08);
>      /* Maximum Latency */
> @@ -549,12 +537,21 @@ static void e100_pci_reset(EEPRO100State * s, E100PCIDeviceInfo *e100_device)
>  
>      if (e100_device->power_management) {
>          /* Power Management Capabilities */
> -        pci_set_byte(pci_conf + 0xdc, PCI_CAP_ID_PM);
> -        /* Next Item Pointer */
> -        /* Capability ID */
> -        pci_set_word(pci_conf + 0xde, 0x7e21);
> -        /* TODO: Power Management Control / Status. */
> -        /* TODO: Ethernet Power Consumption Registers (i82559 and later). */
> +        int cfg_offset;
> +        pci_reserve_capability(&s->dev, PCI_CONFIG_HEADER_SIZE,
> +                               0xdc - PCI_CONFIG_HEADER_SIZE);
> +        cfg_offset = pci_add_capability(&s->dev, PCI_CAP_ID_PM, PCI_PM_SIZEOF);

So this works. Long term, I think I should just extend pci_add_capability
with an offset parameter.

> +        assert(cfg_offset == 0xdc);
> +        if (cfg_offset > 0) {
> +            /* Power Management Capabilities */
> +            pci_set_word(pci_conf + cfg_offset + PCI_PM_PMC, 0x7e21);
> +#if 0 /* TODO: replace dummy code for power management emulation. */
> +            /* TODO: Power Management Control / Status. */
> +            pci_set_word(pci_conf + cfg_offset + PCI_PM_CTRL, 0x0000);
> +            /* TODO: Ethernet Power Consumption Registers (i82559 and later). */
> +            pci_set_byte(pci_conf + cfg_offset + PCI_PM_PPB_EXTENSIONS, 0x0000);
> +#endif
> +        }
>      }
>  
>  #if EEPROM_SIZE > 0
> -- 
> 1.7.0

[Qemu-devel] Re: [PATCH 6/9] eepro100: Support compilation without EEPROM

[Michael S. Tsirkin]

On Tue, Apr 06, 2010 at 01:44:06PM +0200, Stefan Weil wrote:
> To emulate hardware without an EEPROM,
> EEPROM_SIZE may be set to 0.
> 
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> ---
>  hw/eepro100.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/eepro100.c b/hw/eepro100.c
> index cedc427..e12ee23 100644
> --- a/hw/eepro100.c
> +++ b/hw/eepro100.c
> @@ -1866,9 +1866,11 @@ static int e100_nic_init(PCIDevice *pci_dev)
>  
>      e100_pci_reset(s, e100_device);
>  
> +#if EEPROM_SIZE > 0
>      /* Add 64 * 2 EEPROM. i82557 and i82558 support a 64 word EEPROM,
>       * i82559 and later support 64 or 256 word EEPROM. */
>      s->eeprom = eeprom93xx_new(EEPROM_SIZE);
> +#endif

I expect long-term EEPROM_SIZE will stop being a compile-time
constant then?

>  
>      /* Handler for memory-mapped I/O */
>      s->mmio_index =
> -- 
> 1.7.0

[Qemu-devel] Re: [PATCH 2/9] eepro100: Simplify status handling

[Michael S. Tsirkin]

On Tue, Apr 06, 2010 at 01:44:02PM +0200, Stefan Weil wrote:
> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> ---
>  hw/eepro100.c |    9 +++++----
>  1 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/eepro100.c b/hw/eepro100.c
> index 0415132..741031c 100644
> --- a/hw/eepro100.c
> +++ b/hw/eepro100.c
> @@ -175,6 +175,7 @@ typedef enum {
>  } scb_command_bit;
>  
>  typedef enum {
> +    STATUS_NOT_OK = 0,
>      STATUS_C = BIT(15),
>      STATUS_OK = BIT(13),
>  } scb_status_bit;

0 is not a bit, I would just use 0 as a constant below.

> @@ -882,7 +883,7 @@ static void action_command(EEPRO100State *s)
>          bool bit_s;
>          bool bit_i;
>          bool bit_nc;
> -        bool success = true;
> +        uint16_t ok_status = STATUS_OK;
>          s->cb_address = s->cu_base + s->cu_offset;
>          read_cb(s);
>          bit_el = ((s->tx.command & COMMAND_EL) != 0);
> @@ -915,7 +916,7 @@ static void action_command(EEPRO100State *s)
>          case CmdTx:
>              if (bit_nc) {
>                  missing("CmdTx: NC = 0");
> -                success = false;
> +                ok_status = STATUS_NOT_OK;
>                  break;
>              }
>              tx_command(s);
> @@ -932,11 +933,11 @@ static void action_command(EEPRO100State *s)
>              break;
>          default:
>              missing("undefined command");
> -            success = false;
> +            ok_status = STATUS_NOT_OK;
>              break;
>          }
>          /* Write new status. */
> -        stw_phys(s->cb_address, s->tx.status | STATUS_C | (success ? STATUS_OK : 0));
> +        stw_phys(s->cb_address, s->tx.status | ok_status | STATUS_C);
>          if (bit_i) {
>              /* CU completed action. */
>              eepro100_cx_interrupt(s);
> -- 
> 1.7.0

[Qemu-devel] Re: eepro100: New patches

[Michael S. Tsirkin]

On Tue, Apr 06, 2010 at 01:44:00PM +0200, Stefan Weil wrote:
> These patches fix two regressions (1, 9) which made eepro100 rather useless,
> use a simple method to handle the different device variants (2),
> add a new device variant (4) and fix or clean some smaller issues.
> 
> [PATCH 1/9] eepro100: Don't allow writing SCBStatus
> [PATCH 2/9] eepro100: Simplify status handling
> [PATCH 3/9] eepro100: Simplified device instantiation
> [PATCH 4/9] eepro100: Add new device variant i82801
> [PATCH 5/9] eepro100: Set configuration bit for standard TCB
> [PATCH 6/9] eepro100: Support compilation without EEPROM
> [PATCH 7/9] eepro100: Set power management capability using pci_reserve_capability
> [PATCH 8/9] eepro100: Fix mapping of flash memory
> [PATCH 9/9] eepro100: Fix PCI interrupt pin configuration regression
> 
> Regards,
> Stefan

I've applied these on my tree with some minor tweaks.
Could you please let me know whether the result looks
sane to you?

Thanks!

-- 
MST

[Qemu-devel] Re: [PATCH 8/9] eepro100: Fix mapping of flash memory

[Stefan Weil]

Michael S. Tsirkin schrieb:
> On Tue, Apr 06, 2010 at 01:44:08PM +0200, Stefan Weil wrote:
>   
>> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
>> ---
>>  hw/eepro100.c |    5 +++--
>>  1 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/eepro100.c b/hw/eepro100.c
>> index f0acdbc..2401888 100644
>> --- a/hw/eepro100.c
>> +++ b/hw/eepro100.c
>> @@ -1622,8 +1622,9 @@ static void pci_mmio_map(PCIDevice * pci_dev, int region_num,
>>            "size=0x%08"FMT_PCIBUS", type=%d\n",
>>            region_num, addr, size, type));
>>  
>> -    if (region_num == 0) {
>> -        /* Map control / status registers. */
>> +    assert(region_num == 0 || region_num == 2);
>> +    if (region_num == 0 || region_num == 2) {
>>     
>
> Looks a bit strange ...  Why do we need the if here?
>   

It is not needed if everything works as it should.

For compilations without NDEBUG, assert will catch
a wrong region_num anyway.

If code is compiled with NDEBUG, the assert does
nothing, so the if is an additional guard.

>   
>> +        /* Map control / status registers and flash. */
>>          cpu_register_physical_memory(addr, size, s->mmio_index);
>>          s->region[region_num] = addr;
>>      }
>> -- 
>> 1.7.0
>>     

[Qemu-devel] Re: [PATCH 6/9] eepro100: Support compilation without EEPROM

[Stefan Weil]

Michael S. Tsirkin schrieb:
> On Tue, Apr 06, 2010 at 01:44:06PM +0200, Stefan Weil wrote:
>   
>> To emulate hardware without an EEPROM,
>> EEPROM_SIZE may be set to 0.
>>
>> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
>> ---
>>  hw/eepro100.c |    2 ++
>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/eepro100.c b/hw/eepro100.c
>> index cedc427..e12ee23 100644
>> --- a/hw/eepro100.c
>> +++ b/hw/eepro100.c
>> @@ -1866,9 +1866,11 @@ static int e100_nic_init(PCIDevice *pci_dev)
>>  
>>      e100_pci_reset(s, e100_device);
>>  
>> +#if EEPROM_SIZE > 0
>>      /* Add 64 * 2 EEPROM. i82557 and i82558 support a 64 word EEPROM,
>>       * i82559 and later support 64 or 256 word EEPROM. */
>>      s->eeprom = eeprom93xx_new(EEPROM_SIZE);
>> +#endif
>>     
>
> I expect long-term EEPROM_SIZE will stop being a compile-time
> constant then?
>
>   
EEPROM_SIZE might be a qdev parameter, so a new eeprom_size
would become part of the device status.

Up to now, there was no need for it.

>>  
>>      /* Handler for memory-mapped I/O */
>>      s->mmio_index =
>> -- 
>> 1.7.0
>>     

[Qemu-devel] Re: [PATCH 2/9] eepro100: Simplify status handling

[Stefan Weil]

Michael S. Tsirkin schrieb:
> On Tue, Apr 06, 2010 at 01:44:02PM +0200, Stefan Weil wrote:
>   
>> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
>> ---
>>  hw/eepro100.c |    9 +++++----
>>  1 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/eepro100.c b/hw/eepro100.c
>> index 0415132..741031c 100644
>> --- a/hw/eepro100.c
>> +++ b/hw/eepro100.c
>> @@ -175,6 +175,7 @@ typedef enum {
>>  } scb_command_bit;
>>  
>>  typedef enum {
>> +    STATUS_NOT_OK = 0,
>>      STATUS_C = BIT(15),
>>      STATUS_OK = BIT(13),
>>  } scb_status_bit;
>>     
>
> 0 is not a bit, I would just use 0 as a constant below.
>   

In a philosophical way, not a bit is some kind of a bit.

I think ok_status = STATUS_NOT_OK is clearer than
ok_status = 0.

>   
>> @@ -882,7 +883,7 @@ static void action_command(EEPRO100State *s)
>>          bool bit_s;
>>          bool bit_i;
>>          bool bit_nc;
>> -        bool success = true;
>> +        uint16_t ok_status = STATUS_OK;
>>          s->cb_address = s->cu_base + s->cu_offset;
>>          read_cb(s);
>>          bit_el = ((s->tx.command & COMMAND_EL) != 0);
>> @@ -915,7 +916,7 @@ static void action_command(EEPRO100State *s)
>>          case CmdTx:
>>              if (bit_nc) {
>>                  missing("CmdTx: NC = 0");
>> -                success = false;
>> +                ok_status = STATUS_NOT_OK;
>>                  break;
>>              }
>>              tx_command(s);
>> @@ -932,11 +933,11 @@ static void action_command(EEPRO100State *s)
>>              break;
>>          default:
>>              missing("undefined command");
>> -            success = false;
>> +            ok_status = STATUS_NOT_OK;
>>              break;
>>          }
>>          /* Write new status. */
>> -        stw_phys(s->cb_address, s->tx.status | STATUS_C | (success ? STATUS_OK : 0));
>> +        stw_phys(s->cb_address, s->tx.status | ok_status | STATUS_C);
>>          if (bit_i) {
>>              /* CU completed action. */
>>              eepro100_cx_interrupt(s);
>> -- 
>> 1.7.0
>>     

[Qemu-devel] Re: [PATCH 8/9] eepro100: Fix mapping of flash memory

[Michael S. Tsirkin]

On Tue, Apr 06, 2010 at 04:23:41PM +0200, Stefan Weil wrote:
> Michael S. Tsirkin schrieb:
> > On Tue, Apr 06, 2010 at 01:44:08PM +0200, Stefan Weil wrote:
> >   
> >> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> >> ---
> >>  hw/eepro100.c |    5 +++--
> >>  1 files changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/eepro100.c b/hw/eepro100.c
> >> index f0acdbc..2401888 100644
> >> --- a/hw/eepro100.c
> >> +++ b/hw/eepro100.c
> >> @@ -1622,8 +1622,9 @@ static void pci_mmio_map(PCIDevice * pci_dev, int region_num,
> >>            "size=0x%08"FMT_PCIBUS", type=%d\n",
> >>            region_num, addr, size, type));
> >>  
> >> -    if (region_num == 0) {
> >> -        /* Map control / status registers. */
> >> +    assert(region_num == 0 || region_num == 2);
> >> +    if (region_num == 0 || region_num == 2) {
> >>     
> >
> > Looks a bit strange ...  Why do we need the if here?
> >   
> 
> It is not needed if everything works as it should.
> 
> For compilations without NDEBUG, assert will catch
> a wrong region_num anyway.
> 
> If code is compiled with NDEBUG, the assert does
> nothing, so the if is an additional guard.

We don't need the guard though: the only way to get
non 0 and non 2 region is because of a bug in code.
So the check is just a debugging aid.

> >   
> >> +        /* Map control / status registers and flash. */
> >>          cpu_register_physical_memory(addr, size, s->mmio_index);
> >>          s->region[region_num] = addr;
> >>      }
> >> -- 
> >> 1.7.0
> >>     

[Qemu-devel] Re: [PATCH 2/9] eepro100: Simplify status handling

[Michael S. Tsirkin]

On Tue, Apr 06, 2010 at 04:29:45PM +0200, Stefan Weil wrote:
> Michael S. Tsirkin schrieb:
> > On Tue, Apr 06, 2010 at 01:44:02PM +0200, Stefan Weil wrote:
> >   
> >> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> >> ---
> >>  hw/eepro100.c |    9 +++++----
> >>  1 files changed, 5 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/hw/eepro100.c b/hw/eepro100.c
> >> index 0415132..741031c 100644
> >> --- a/hw/eepro100.c
> >> +++ b/hw/eepro100.c
> >> @@ -175,6 +175,7 @@ typedef enum {
> >>  } scb_command_bit;
> >>  
> >>  typedef enum {
> >> +    STATUS_NOT_OK = 0,
> >>      STATUS_C = BIT(15),
> >>      STATUS_OK = BIT(13),
> >>  } scb_status_bit;
> >>     
> >
> > 0 is not a bit, I would just use 0 as a constant below.
> >   
> 
> In a philosophical way, not a bit is some kind of a bit.
> 
> I think ok_status = STATUS_NOT_OK is clearer than
> ok_status = 0.

The reason it's not clear is because STATUS_OK | STATUS_NOT_OK
is not a valid combination. IOW, each enum should be:
- a set of bits. 0 is not a bit, you write 0 to say "no bits".
  you can | the values.
- a set of values. you can not | values together.

So either we have ok_status = 0 -> no bits set, and then
ok_status | STATUS_C, or

      STATUS_NOT_OK = BIT(15)
      STATUS_OK = BIT(13) | BIT(15),

and then just use ok_status.

> >   
> >> @@ -882,7 +883,7 @@ static void action_command(EEPRO100State *s)
> >>          bool bit_s;
> >>          bool bit_i;
> >>          bool bit_nc;
> >> -        bool success = true;
> >> +        uint16_t ok_status = STATUS_OK;
> >>          s->cb_address = s->cu_base + s->cu_offset;
> >>          read_cb(s);
> >>          bit_el = ((s->tx.command & COMMAND_EL) != 0);
> >> @@ -915,7 +916,7 @@ static void action_command(EEPRO100State *s)
> >>          case CmdTx:
> >>              if (bit_nc) {
> >>                  missing("CmdTx: NC = 0");
> >> -                success = false;
> >> +                ok_status = STATUS_NOT_OK;
> >>                  break;
> >>              }
> >>              tx_command(s);
> >> @@ -932,11 +933,11 @@ static void action_command(EEPRO100State *s)
> >>              break;
> >>          default:
> >>              missing("undefined command");
> >> -            success = false;
> >> +            ok_status = STATUS_NOT_OK;
> >>              break;
> >>          }
> >>          /* Write new status. */
> >> -        stw_phys(s->cb_address, s->tx.status | STATUS_C | (success ? STATUS_OK : 0));
> >> +        stw_phys(s->cb_address, s->tx.status | ok_status | STATUS_C);
> >>          if (bit_i) {
> >>              /* CU completed action. */
> >>              eepro100_cx_interrupt(s);
> >> -- 
> >> 1.7.0
> >>     

Re: [Qemu-devel] [PATCH 6/9] eepro100: Support compilation without EEPROM

[Richard Henderson]

On 04/06/2010 04:44 AM, Stefan Weil wrote:
> +#if EEPROM_SIZE > 0
>      /* Add 64 * 2 EEPROM. i82557 and i82558 support a 64 word EEPROM,
>       * i82559 and later support 64 or 256 word EEPROM. */
>      s->eeprom = eeprom93xx_new(EEPROM_SIZE);
> +#endif

If EEPROM_SIZE is known to be defined, even if zero, it is
better to write this as a C if, not a preprocessor ifdef.
Let the compiler eliminate the dead code for you.


r~

Re: [Qemu-devel] [PATCH 6/9] eepro100: Support compilation without EEPROM

[Stefan Weil]

Richard Henderson schrieb:
> On 04/06/2010 04:44 AM, Stefan Weil wrote:
>   
>> +#if EEPROM_SIZE > 0
>>      /* Add 64 * 2 EEPROM. i82557 and i82558 support a 64 word EEPROM,
>>       * i82559 and later support 64 or 256 word EEPROM. */
>>      s->eeprom = eeprom93xx_new(EEPROM_SIZE);
>> +#endif
>>     
>
> If EEPROM_SIZE is known to be defined, even if zero, it is
> better to write this as a C if, not a preprocessor ifdef.
> Let the compiler eliminate the dead code for you.
>
>
> r~
>   

Why do you think that a C if is better than a CPP if
in this case?

Some compilers give warnings for code which will
never be reached. Try gcc -Wunreachable-code.
It's a really useful option which sometimes even
detects programming errors, so maybe it would
be good for qemu, too.

Stefan

[Qemu-devel] Re: eepro100: New patches

[Stefan Weil]

Michael S. Tsirkin schrieb:
> On Tue, Apr 06, 2010 at 01:44:00PM +0200, Stefan Weil wrote:
>   
>> These patches fix two regressions (1, 9) which made eepro100 rather useless,
>> use a simple method to handle the different device variants (2),
>> add a new device variant (4) and fix or clean some smaller issues.
>>
>> [PATCH 1/9] eepro100: Don't allow writing SCBStatus
>> [PATCH 2/9] eepro100: Simplify status handling
>> [PATCH 3/9] eepro100: Simplified device instantiation
>> [PATCH 4/9] eepro100: Add new device variant i82801
>> [PATCH 5/9] eepro100: Set configuration bit for standard TCB
>> [PATCH 6/9] eepro100: Support compilation without EEPROM
>> [PATCH 7/9] eepro100: Set power management capability using pci_reserve_capability
>> [PATCH 8/9] eepro100: Fix mapping of flash memory
>> [PATCH 9/9] eepro100: Fix PCI interrupt pin configuration regression
>>
>> Regards,
>> Stefan
>>     
>
> I've applied these on my tree with some minor tweaks.
> Could you please let me know whether the result looks
> sane to you?
>
> Thanks!
>   

I noticed minor tweaks in patches 2 and 8, both ok.

Regards
Stefan

Re: [Qemu-devel] [PATCH 6/9] eepro100: Support compilation without EEPROM

[Richard Henderson]

On 04/06/2010 09:01 AM, Stefan Weil wrote:
> Richard Henderson schrieb:
>> On 04/06/2010 04:44 AM, Stefan Weil wrote:
>>   
>>> +#if EEPROM_SIZE > 0
>>>      /* Add 64 * 2 EEPROM. i82557 and i82558 support a 64 word EEPROM,
>>>       * i82559 and later support 64 or 256 word EEPROM. */
>>>      s->eeprom = eeprom93xx_new(EEPROM_SIZE);
>>> +#endif
>>>     
...
> Why do you think that a C if is better than a CPP if
> in this case?
> 
> Some compilers give warnings for code which will
> never be reached. Try gcc -Wunreachable-code.
> It's a really useful option which sometimes even
> detects programming errors, so maybe it would
> be good for qemu, too.

Having the compiler detect syntax and type mismatch errors in all
code paths is generally far more valuable than warnings for unreachable
code.  These tend to creep in very easily with ifdef'ed code.

This has follow-on effects as well.  Suppose this instance above is
the only place that eeprom93xx_new is called.  With an ifdef here at
the use site, the compiler will complain that eeprom93xx_new is unused,
leading you to introduce more ifdefs, which means that even more code
is unchecked.

If you use a C if, then the compiler will see that eeprom93xx_new
is used under other circumstances, not complain, and eliminate
it as unused -- after having verified that it is both syntactically
and semantically correct.

Preprocessor spaghetti code is extremely hard to read.  I know that
QEMU is chock full of it, but let's try to reduce that rather than
introduce more.


r~

Re: [Qemu-devel] [PATCH 6/9] eepro100: Support compilation without EEPROM

[Paul Brook]

> To emulate hardware without an EEPROM,
> EEPROM_SIZE may be set to 0.

If might, but it isn't.

This patch introduces a condition that will never be false. Please don't do 
that.  I consider code that is never used to be actively harmful. Any feature 
that requires the user hack the source may as well not exist.  The only 
possible exception is debug output intended solely for qemu developers.

If there's something worth noting for future reference then add a proper 
comment, if necessary marked as TODO/FIXME.

Paul

Re: [Qemu-devel] [PATCH 6/9] eepro100: Support compilation without EEPROM

[Stefan Weil]

Paul Brook schrieb:
>> To emulate hardware without an EEPROM,
>> EEPROM_SIZE may be set to 0.
>
> If might, but it isn't.
>
> This patch introduces a condition that will never be false. Please
> don't do
> that. I consider code that is never used to be actively harmful. Any
> feature
> that requires the user hack the source may as well not exist. The only
> possible exception is debug output intended solely for qemu developers.
>
> If there's something worth noting for future reference then add a proper
> comment, if necessary marked as TODO/FIXME.
>
> Paul

In this case, it is code which is normally always used,
so maybe it is a little less harmful :-)

Anyway - Richard already gave a good feedback on the same topic.

His feedback convinced me that adding an eeprom size or model
property as a device option would be the better way to
support both developer needs (I want to make tests with
no eeprom) and user needs (normally, an eeprom is
available). The preprocessor #if would be replaced by
a normal C if.

I'll do this in a future patch.

Michael, I suggest either omitting this patch or adding a
TODO comment to the preprocessor #if like this:

#if EEPROM_SIZE > 0 /* TODO: add a new EEPROM property and use it here */

Regards,
Stefan

Re: [Qemu-devel] [PATCH 6/9] eepro100: Support compilation without EEPROM

[Michael S. Tsirkin]

On Wed, Apr 07, 2010 at 09:02:06AM +0200, Stefan Weil wrote:
> Paul Brook schrieb:
> >> To emulate hardware without an EEPROM,
> >> EEPROM_SIZE may be set to 0.
> >
> > If might, but it isn't.
> >
> > This patch introduces a condition that will never be false. Please
> > don't do
> > that. I consider code that is never used to be actively harmful. Any
> > feature
> > that requires the user hack the source may as well not exist. The only
> > possible exception is debug output intended solely for qemu developers.
> >
> > If there's something worth noting for future reference then add a proper
> > comment, if necessary marked as TODO/FIXME.
> >
> > Paul
> 
> In this case, it is code which is normally always used,
> so maybe it is a little less harmful :-)
> 
> Anyway - Richard already gave a good feedback on the same topic.
> 
> His feedback convinced me that adding an eeprom size or model
> property as a device option would be the better way to
> support both developer needs (I want to make tests with
> no eeprom) and user needs (normally, an eeprom is
> available). The preprocessor #if would be replaced by
> a normal C if.
> 
> I'll do this in a future patch.
> 
> Michael, I suggest either omitting this patch or adding a
> TODO comment to the preprocessor #if like this:
> 
> #if EEPROM_SIZE > 0 /* TODO: add a new EEPROM property and use it here */
> 
> Regards,
> Stefan

I'll omit the patch. Thanks!

[Qemu-devel] Re: eepro100: New patches

[Michael S. Tsirkin]

On Tue, Apr 06, 2010 at 06:09:56PM +0200, Stefan Weil wrote:
> Michael S. Tsirkin schrieb:
> > On Tue, Apr 06, 2010 at 01:44:00PM +0200, Stefan Weil wrote:
> >   
> >> These patches fix two regressions (1, 9) which made eepro100 rather useless,
> >> use a simple method to handle the different device variants (2),
> >> add a new device variant (4) and fix or clean some smaller issues.
> >>
> >> [PATCH 1/9] eepro100: Don't allow writing SCBStatus
> >> [PATCH 2/9] eepro100: Simplify status handling
> >> [PATCH 3/9] eepro100: Simplified device instantiation
> >> [PATCH 4/9] eepro100: Add new device variant i82801
> >> [PATCH 5/9] eepro100: Set configuration bit for standard TCB
> >> [PATCH 6/9] eepro100: Support compilation without EEPROM
> >> [PATCH 7/9] eepro100: Set power management capability using pci_reserve_capability
> >> [PATCH 8/9] eepro100: Fix mapping of flash memory
> >> [PATCH 9/9] eepro100: Fix PCI interrupt pin configuration regression
> >>
> >> Regards,
> >> Stefan
> >>     
> >
> > I've applied these on my tree with some minor tweaks.
> > Could you please let me know whether the result looks
> > sane to you?
> >
> > Thanks!
> >   
> 
> I noticed minor tweaks in patches 2 and 8, both ok.
> 
> Regards
> Stefan


OK, I added an API to add a capability at a known offset,
eepro100 code becomes simpler if we use it.
Can you review and ack please?

-- 
MST