[Qemu-devel] [PATCH] Add support for r6040 NIC

[Qemu-devel] [PATCH] Add support for r6040 NIC

[bifferos]

Signed-off-by: Mark Kelly <mark@bifferos.com>
diff --git a/Makefile.objs b/Makefile.objs
index 6991a9f..7d87503 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -240,6 +240,7 @@ hw-obj-$(CONFIG_PCNET_PCI) += pcnet-pci.o
 hw-obj-$(CONFIG_PCNET_COMMON) += pcnet.o
 hw-obj-$(CONFIG_E1000_PCI) += e1000.o
 hw-obj-$(CONFIG_RTL8139_PCI) += rtl8139.o
+hw-obj-$(CONFIG_R6040_PCI) += r6040.o
 
 hw-obj-$(CONFIG_SMC91C111) += smc91c111.o
 hw-obj-$(CONFIG_LAN9118) += lan9118.o
diff --git a/default-configs/pci.mak b/default-configs/pci.mak
index 22bd350..d2ea7a2 100644
--- a/default-configs/pci.mak
+++ b/default-configs/pci.mak
@@ -10,6 +10,7 @@ CONFIG_PCNET_PCI=y
 CONFIG_PCNET_COMMON=y
 CONFIG_LSI_SCSI_PCI=y
 CONFIG_RTL8139_PCI=y
+CONFIG_R6040_PCI=y
 CONFIG_E1000_PCI=y
 CONFIG_IDE_CORE=y
 CONFIG_IDE_QDEV=y
diff --git a/hw/pci.c b/hw/pci.c
index b904a4e..7e12935 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1527,6 +1527,7 @@ static const char * const pci_nic_models[] = {
     "rtl8139",
     "e1000",
     "pcnet",
+    "r6040",
     "virtio",
     NULL
 };
@@ -1539,6 +1540,7 @@ static const char * const pci_nic_names[] = {
     "rtl8139",
     "e1000",
     "pcnet",
+    "r6040",
     "virtio-net-pci",
     NULL
 };
diff --git a/hw/r6040.c b/hw/r6040.c
new file mode 100644
index 0000000..83587e7
--- /dev/null
+++ b/hw/r6040.c
@@ -0,0 +1,627 @@
+/*
+   Emulation of r6040 ethernet controller found in a number of SoCs.
+   Copyright (c) 2011 Mark Kelly, mark@bifferos.com
+
+   This has been written using the R8610[1] and ip101a[2] datasheets.
+
+   ICs with the embedded controller include R8610, R3210, AMRISC20000
+   and Vortex86SX
+
+   The emulation seems good enough to fool Linux 2.6.37.6.  It is
+   not perfect, but has proven useful.
+
+   [1] http://www.sima.com.tw/download/R8610_D06_20051003.pdf
+   [2] http://www.icplus.com.tw/pp-IP101A.html
+ */
+
+#include "hw.h"
+#include "pci.h"
+#include "net.h"
+#include "loader.h"
+#include "sysemu.h"
+#include "qemu-timer.h"
+
+/* #define DEBUG_R6040 1 */
+
+
+#if defined DEBUG_R6040
+#define DPRINTF(fmt, ...) \
+    do { fprintf(stderr, "R6040: " fmt, ## __VA_ARGS__); } while (0)
+#else
+static inline GCC_FMT_ATTR(1, 2) int DPRINTF(const char *fmt, ...)
+{
+    return 0;
+}
+#endif
+
+
+/* Cast in order of appearance.  _W prevfix means it's used to index the
+   register word array (RegsW)
+ */
+
+#define MPSCCR_W         (0x88 / 2)
+
+#define MAC0_W           (0x68 / 2)
+#define MAC1_W           (0x6a / 2)
+#define MAC2_W           (0x6c / 2)
+
+
+#define RX_START_LOW_W   (0x34 / 2)
+#define RX_START_HIGH_W  (0x38 / 2)
+#define TX_PKT_COUNT_W   (0x5a / 2)
+#define RX_PKT_COUNT_W   (0x50 / 2)
+
+
+#define MCR0_W           (0x00 / 2)
+#define MCR1_W           (0x04 / 2)
+#define BIT_MRST         (1 << 0)
+
+#define MTPR_W           (0x14 / 2)
+#define MRBSR_W          (0x18 / 2)
+#define MISR_W           (0x3c / 2)
+#define MIER_W           (0x40 / 2)
+
+#define MMDIO_W          (0x20 / 2)
+#define MDIO_READ_W      (0x24 / 2)
+#define MDIO_WRITE_W     (0x28 / 2)
+
+#define MRCNT_W          (0x50 / 2)
+#define MTCNT_W          (0x5c / 2)
+
+
+#define MDIO_WRITE      0x4000
+#define MDIO_READ       0x2000
+
+
+typedef struct R6040State {
+    PCIDevice dev;
+    NICState *nic;
+    NICConf conf;
+
+    /* PHY related register sets */
+    uint16_t MID0[3];
+    uint16_t phy_regs[32];
+    uint32_t phy_op_in_progress;
+
+    /* Primary IO address space */
+    union {
+        uint8_t RegsB[0x100];   /* Byte access */
+        uint16_t RegsW[0x100/2];  /* word access */
+        uint32_t RegsL[0x100/4];  /* DWORD access */
+    };
+
+} R6040State;
+
+
+/* some inlines to help access above structure */
+static inline uint32_t TX_START(R6040State *s)
+{
+    uint32_t tmp = s->RegsW[0x2c/2];
+    return tmp | (s->RegsW[0x30/2] << 16);
+}
+
+static inline void TX_START_SET(R6040State *s, uint32_t start)
+{
+    s->RegsW[0x2c/2] = start & 0xffff;
+    s->RegsW[0x30/2] = (start >> 16) & 0xffff;
+}
+
+static inline uint32_t RX_START(R6040State *s)
+{
+    uint32_t tmp = s->RegsW[0x34/2];
+    return tmp | (s->RegsW[0x38/2] << 16);
+}
+
+static inline void RX_START_SET(R6040State *s, uint32_t start)
+{
+    s->RegsW[0x34/2] = start & 0xffff;
+    s->RegsW[0x38/2] = (start >> 16) & 0xffff;
+}
+
+
+static void r6040_update_irq(R6040State *s)
+{
+    uint16_t isr = s->RegsW[MISR_W] & s->RegsW[MIER_W];
+
+    qemu_set_irq(s->dev.irq[0], isr ? 1 : 0);
+}
+
+
+/* Mark auto-neg complete, NIC up.  */
+static void PhysicalLinkUp(void *opaque)
+{
+    R6040State *s = opaque;
+    s->phy_regs[1] |= (1 << 2);
+}
+
+
+/* Transmit and receive descriptors are doubled up
+   One is a subset of the other anyhow
+ */
+typedef struct descriptor {
+    uint16_t DST;
+    uint16_t DLEN;
+    uint32_t DBP;
+    uint32_t DNX;
+    uint16_t HIDX;
+    uint16_t Reserved1;
+    uint16_t Reserved2;
+} descriptor_t;
+
+
+/* Some debugging functions */
+
+#ifdef DEBUG_R6040
+static void addr_dump16(const char *name, uint16_t val)
+{
+    DPRINTF("%s: 0x%04x  ", name, val);
+}
+
+static void addr_dump32(const char *name, uint32_t val)
+{
+    DPRINTF("%s: 0x%x  ", name, val);
+}
+
+static void hex_dump(const uint8_t *data, uint32_t len)
+{
+    uint8_t i;
+    DPRINTF("hex: ");
+    for (i = 0; i < len; i++) {
+        fprintf(stderr, "%02x ", *data);
+        if (i && !(i % 0x20)) {
+            fprintf(stderr, "\n");
+        }
+        data++;
+    }
+    fprintf(stderr, "\n");
+}
+
+static void desc_dump(descriptor_t *d, uint32_t addr)
+{
+    DPRINTF("\nDumping: 0x%x\n", addr);
+    addr_dump16("DST", d->DST);
+    addr_dump16("DLEN", d->DLEN);
+    addr_dump32("DBP", (unsigned long)d->DBP);
+    addr_dump32("DNX", (unsigned long)d->DNX);
+    addr_dump16("HIDX", d->HIDX);
+    printf("\n");
+}
+
+static void dump_phys_mem(uint32_t addr, int len)
+{
+    uint8_t buffer[1024];
+    cpu_physical_memory_read(addr, buffer, len);
+    hex_dump(buffer, len);
+}
+
+static void dump_pci(uint8_t *pci_conf)
+{
+    uint32_t *p = (uint32_t *)pci_conf;
+    int i = 0;
+    for (i = 0; i < 0x40; i += 4) {
+        DPRINTF("Addr: 0x%08x,  Data: 0x%08x\n", i, *p);
+        p++;
+    }
+}
+#endif
+
+
+static const VMStateDescription vmstate_r6040 = {
+    .name = "r6040",
+    .version_id = 3,
+    .minimum_version_id = 2,
+    .minimum_version_id_old = 2,
+    .fields = (VMStateField[]) {
+        VMSTATE_PCI_DEVICE(dev, R6040State),
+        VMSTATE_BUFFER(RegsB, R6040State),
+        VMSTATE_UINT16_ARRAY(MID0, R6040State, 3),
+        VMSTATE_UINT16_ARRAY(phy_regs, R6040State, 32),
+        VMSTATE_UINT32(phy_op_in_progress, R6040State),
+        VMSTATE_MACADDR(conf.macaddr, R6040State),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+
+static int TryToSendOnePacket(void *opaque)
+{
+    R6040State *s = opaque;
+    descriptor_t d;
+    uint8_t pkt_buffer[2000];
+    uint32_t tocopy;
+
+    cpu_physical_memory_read(TX_START(s), (uint8_t *)&d, sizeof(d));
+
+    if (d.DST & 0x8000) {    /* MAC owns it?  */
+        tocopy = d.DLEN;
+        if (tocopy > sizeof(pkt_buffer)) {
+            tocopy = sizeof(pkt_buffer);
+        }
+        /* copy the packet to send it */
+        cpu_physical_memory_read(d.DBP, pkt_buffer, tocopy);
+
+        qemu_send_packet(&s->nic->nc, pkt_buffer, tocopy);
+        s->RegsW[TX_PKT_COUNT_W]++;
+
+        /* relinquish ownership, we're done with it */
+        d.DST &= ~0x8000;
+
+        /* Copy the new version of the descriptor back */
+        cpu_physical_memory_write(TX_START(s), (uint8_t *)&d, sizeof(d));
+
+        /* Advance to the next buffer if packet processed */
+        TX_START_SET(s, d.DNX);
+
+        return 1;
+    }
+
+    return 0;
+}
+
+
+static void r6040_transmit(void *opaque)
+{
+    R6040State *s = opaque;
+    int count = 0;
+
+    while (TryToSendOnePacket(s)) {
+        ++count;
+    }
+
+    if (count) {
+        s->RegsW[MISR_W] |= 0x10;
+        r6040_update_irq(s);
+    }
+}
+
+
+/* Whether to allow callback returning 1 for yes, can receive */
+static int r6040_can_receive(VLANClientState *nc)
+{
+    R6040State *s = DO_UPCAST(NICState, nc, nc)->opaque;
+    int tmp = (s->RegsW[0] & (1 << 1)) ? 1 : 0;
+    return tmp;
+}
+
+
+static int ReceiveOnePacket(void *opaque, const uint8_t *buf, size_t len)
+{
+    R6040State *s = opaque;
+    uint32_t tocopy = len+4;  /* include checksum */
+    descriptor_t d;
+
+    cpu_physical_memory_read(RX_START(s), (uint8_t *)&d, sizeof(descriptor_t));
+    /*desc_dump(&d, 0);*/
+
+    if (d.DST & 0x8000) {    /* MAC owned? */
+
+        uint16_t max_buffer = s->RegsW[MRBSR_W] & 0x07fc;
+        if (tocopy > max_buffer) {
+            tocopy = max_buffer;
+        }
+
+        cpu_physical_memory_write(d.DBP, buf, tocopy-4);
+
+        /* indicate received OK */
+        d.DST |= (1 << 14);
+        d.DLEN = tocopy;
+        /* relinquish ownership */
+        d.DST &= ~0x8000;
+
+        /* Copy the descriptor back */
+        cpu_physical_memory_write(RX_START(s), (uint8_t *)&d,
+                                   sizeof(descriptor_t));
+
+        s->RegsW[RX_PKT_COUNT_W]++;
+
+        s->RegsW[MISR_W] |= 1;  /* received pkt interrupt */
+
+        r6040_update_irq(s);
+
+        RX_START_SET(s, d.DNX);  /* advance */
+
+        return 0;
+    }
+    return -1;
+}
+
+
+/* called on incoming packets */
+static ssize_t r6040_receive(VLANClientState *nc, const uint8_t *buf,
+                                        size_t len)
+{
+    R6040State *s = DO_UPCAST(NICState, nc, nc)->opaque;
+    DPRINTF("Received incoming packet of len %ld\n", len);
+
+    if (0 == ReceiveOnePacket(s, buf, len)) {
+        return len;  /* copied OK */
+    }
+
+    return 0;
+}
+
+
+static void r6040_cleanup(VLANClientState *vc)
+{
+    DPRINTF("r6040_cleanup\n");
+}
+
+
+static inline int BIT_SET(uint16_t old, uint16_t new, uint16_t bit)
+{
+    uint16_t before = (old & (1 << bit));
+    uint16_t after = (new & (1 << bit));
+    if (!before && after) {
+        return 1;
+    }
+    return 0;
+}
+
+
+static void r6040_ioport_writew(void *opaque, uint32_t addr, uint32_t val)
+{
+    R6040State *s = opaque;
+    uint16_t old;
+    addr &= 0xff;   /* get relative to base address */
+    addr /= 2;    /* Get the offset into the word-array */
+    old = s->RegsW[addr];   /* store the old value for future use */
+
+    switch (addr) {
+    case MCR0_W:   /* 0x00 */
+        if (BIT_SET(old, val, 12)) {
+            r6040_transmit(opaque);
+        }
+        break;
+    case MCR1_W:  /* 0x04 */
+        if (val & BIT_MRST) {   /* reset request incoming */
+            /* reset requested, complete it immediately, set this value to
+               default */
+            val = 0x0010;
+        }
+        break;
+    case MTPR_W:  /* TX command reg, 0x14 */
+        if (val & 1) {
+            r6040_transmit(opaque);
+            val &= ~1;
+        }
+        break;
+    case MMDIO_W:  /* MDIO control, 0x20 */
+        {
+            int phy_exists = ((val & 0x1f00) == 0x100) ? 1 : 0;
+            uint16_t *phy = s->phy_regs;
+            phy += (val & 0x1f);
+
+            if  (val & (1 << 13)) {   /* read data */
+                if (phy_exists) {
+                    s->RegsW[MDIO_READ_W] = *phy;
+                } else {
+                    s->RegsW[MDIO_READ_W] = 0xffff;
+                }
+            } else if (val & (1 << 14)) {  /* write data */
+                if (phy_exists) {
+                    *phy = s->RegsW[MDIO_WRITE_W];
+                }
+            }
+
+            /* Whether you request to read or write, both bits go high while
+               the operation is in progress, e.g. tell it to read, and the
+               write-in-progress flag also goes high */
+            val |= 0x6000;  /* signal operation has started */
+            s->phy_op_in_progress = 1;
+
+            break;
+        }
+    case MISR_W:  /* interrupt status reg (read to clear), 0x3c */
+        return;
+
+    case MIER_W:  /* interrupt enable register, 0x40 */
+        s->RegsW[MIER_W] = val;
+        r6040_update_irq(s);
+        return;
+
+    case MRCNT_W:   /* 0x50 */
+    case MTCNT_W:   /* 0x5c */
+        return;  /* Can't write to pkt count registers, skip */
+
+    }
+    s->RegsW[addr] = val & 0xFFFF;
+}
+
+
+
+static uint32_t r6040_ioport_readw(void *opaque, uint32_t addr)
+{
+    R6040State *s = opaque;
+    addr &= 0xff;   /* get relative to base address */
+    addr /= 2;    /* Get the offset into the word-array */
+    uint32_t tmp = s->RegsW[addr];  /* get the value */
+
+    switch (addr) {
+
+    case MMDIO_W:  /* MDIO control, 0x20 */
+        {
+            /* Clear any in-progress MDIO activity for the next read
+               This simulates the polling of the MDIO operation status,
+               so the driver code always has to read the register twice
+               before it thinks the operation is complete. */
+            if (s->phy_op_in_progress) {
+                s->RegsW[addr] &= ~0x6000;
+                s->phy_op_in_progress = 0;
+            }
+            break;
+        }
+    case MISR_W:  /* interrupt status reg (read to clear)  0x3c */
+        s->RegsW[addr] = 0;
+        break;
+    case MIER_W:  /* interrupt enable reg 0x40 */
+        break;
+    case MRCNT_W:  /* 0x50 */
+    case MTCNT_W:  /* 0x5c */
+        s->RegsW[addr] = 0;   /* read to clear */
+        break;
+    default:
+        break;
+    }
+    return tmp;
+}
+
+
+/* byte and long access are routed via the word operation handlers */
+static void r6040_ioport_writeb(void *opaque, uint32_t addr, uint32_t val)
+{
+    R6040State *s = opaque;
+    addr &= 0xFF;
+    val &= 0xFF;
+    uint16_t old = s->RegsW[addr/2];  /* get the current value */
+    if (addr & 1) {
+        old &= 0xff;
+        old |= (val << 8);
+    } else {
+        old &= 0xff00;
+        old |= val;
+    }
+
+    r6040_ioport_writew(opaque, addr, old);  /* call the word-based version */
+}
+
+static void r6040_ioport_writel(void *opaque, uint32_t addr, uint32_t val)
+{
+    /* Set the low value */
+    r6040_ioport_writew(opaque, addr, val & 0xffff);
+    /* Set the high value */
+    r6040_ioport_writew(opaque, addr+2, (val >> 16) & 0xffff);
+}
+
+static uint32_t r6040_ioport_readb(void *opaque, uint32_t addr)
+{
+    uint32_t tmp = r6040_ioport_readw(opaque, addr & ~1);
+    if (addr & 1) {
+        return (tmp & 0xff00) >> 8;
+    }
+    return tmp & 0xff;
+}
+
+static uint32_t r6040_ioport_readl(void *opaque, uint32_t addr)
+{
+    uint32_t tmp = r6040_ioport_readw(opaque, addr);
+    return tmp | (r6040_ioport_readw(opaque, addr+2) << 16);
+}
+
+
+static void r6040_register_ioports(R6040State *s, pcibus_t addr)
+{
+    register_ioport_write(addr, 0x100, 1, r6040_ioport_writeb, s);
+    register_ioport_read(addr, 0x100, 1, r6040_ioport_readb,  s);
+
+    register_ioport_write(addr, 0x100, 2, r6040_ioport_writew, s);
+    register_ioport_read(addr, 0x100, 2, r6040_ioport_readw,  s);
+
+    register_ioport_write(addr, 0x100, 4, r6040_ioport_writel, s);
+    register_ioport_read(addr, 0x100, 4, r6040_ioport_readl,  s);
+}
+
+
+static void r6040_map(PCIDevice *pci_dev, int region_num,
+                       pcibus_t addr, pcibus_t size, int type)
+{
+    R6040State *s = DO_UPCAST(R6040State, dev, pci_dev);;
+
+    DPRINTF("## Mapping to address %lx\n", addr);
+    r6040_register_ioports(s, addr);
+}
+
+
+static NetClientInfo net_r6040_info = {
+    .type = NET_CLIENT_TYPE_NIC,
+    .size = sizeof(NICState),
+    .can_receive = r6040_can_receive,
+    .receive = r6040_receive,
+    .cleanup = r6040_cleanup,
+};
+
+
+static int r6040_init_pci(PCIDevice *dev)
+{
+    QEMUTimer *timer;
+
+    R6040State *s = DO_UPCAST(R6040State, dev, dev);
+    uint8_t *pci_conf;
+
+    /* MAC PHYS status change register.  Linux driver expects something
+       sensible as default and if not will try to set it */
+    s->RegsW[MPSCCR_W] = 0x9f07;
+
+    /* Default value for maximum packet size */
+    s->RegsW[MRBSR_W] = 0x600;
+
+    /* set the MAC, linux driver reads this when it loads, it is
+       normally set by the BIOS, but obviously Qemu BIOS isn't going
+       to do that */
+    s->RegsW[MAC0_W] = 0x5452;
+    s->RegsW[MAC1_W] = 0x1200;
+    s->RegsW[MAC2_W] = 0x5734;
+
+    /* Tell Qemu the same thing */
+    s->conf.macaddr.a[0] = s->RegsW[MAC0_W] & 0xff;
+    s->conf.macaddr.a[1] = (s->RegsW[MAC0_W] >> 8) & 0xff;
+    s->conf.macaddr.a[2] = s->RegsW[MAC1_W] & 0xff;
+    s->conf.macaddr.a[3] = (s->RegsW[MAC1_W] >> 8) & 0xff;
+    s->conf.macaddr.a[4] = s->RegsW[MAC2_W] & 0xff;
+    s->conf.macaddr.a[5] = (s->RegsW[MAC2_W] >> 8) & 0xff;
+
+    /* no commands running */
+    s->phy_op_in_progress = 0;
+
+    /* PHY auto-neg in progress */
+    s->phy_regs[1] = 0x786d & ~(1 << 2);
+    s->phy_regs[2] = 0x0243;
+    s->phy_regs[3] = 0x0c54;
+
+    pci_conf = (uint8_t *)s->dev.config;
+
+    pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; /* header_type */
+    pci_conf[PCI_INTERRUPT_LINE] = 0xa;     /* interrupt line  */
+    pci_conf[PCI_INTERRUPT_PIN] = 1;      /* interrupt pin 0 */
+
+    pci_register_bar(&s->dev, 0, 0x100, PCI_BASE_ADDRESS_SPACE_IO, r6040_map);
+
+    s->nic = qemu_new_nic(&net_r6040_info, &s->conf,
+                            dev->qdev.info->name, dev->qdev.id, s);
+
+    qemu_format_nic_info_str(&s->nic->nc, s->conf.macaddr.a);
+
+    /* Simulate a delay of a couple of seconds for the link to come up.
+       Not required for Linux but very handy for developing BIOS code.
+     */
+    timer = qemu_new_timer_ns(vm_clock, PhysicalLinkUp, s);
+    qemu_mod_timer(timer, qemu_get_clock_ms(vm_clock) + 1500000000);
+
+    /* Register IO port access as well */
+    r6040_register_ioports(s, 0xe800);
+
+    return 0;
+}
+
+
+static PCIDeviceInfo r6040_info = {
+    .qdev.name = "r6040",
+    .qdev.size = sizeof(R6040State),
+    .qdev.vmsd  = &vmstate_r6040,
+    .init      = r6040_init_pci,
+    .vendor_id = 0x17f3,  /* RDC */
+    .device_id = 0x6040,   /* r6040 nic */
+    .class_id   = PCI_CLASS_NETWORK_ETHERNET,
+    .qdev.props = (Property[]) {
+        DEFINE_NIC_PROPERTIES(R6040State, conf),
+        DEFINE_PROP_END_OF_LIST(),
+    }
+};
+
+
+static void r6040_register(void)
+{
+    pci_qdev_register(&r6040_info);
+}
+
+
+device_init(r6040_register)

Re: [Qemu-devel] [PATCH] Add support for r6040 NIC

[Anthony Liguori]

This won't even come close to passing checkpatch.pl

Regards,

Anthony Liguori

On 08/30/2011 08:05 PM, bifferos wrote:
>
> Signed-off-by: Mark Kelly<mark@bifferos.com>
> diff --git a/Makefile.objs b/Makefile.objs
> index 6991a9f..7d87503 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -240,6 +240,7 @@ hw-obj-$(CONFIG_PCNET_PCI) += pcnet-pci.o
>   hw-obj-$(CONFIG_PCNET_COMMON) += pcnet.o
>   hw-obj-$(CONFIG_E1000_PCI) += e1000.o
>   hw-obj-$(CONFIG_RTL8139_PCI) += rtl8139.o
> +hw-obj-$(CONFIG_R6040_PCI) += r6040.o
>
>   hw-obj-$(CONFIG_SMC91C111) += smc91c111.o
>   hw-obj-$(CONFIG_LAN9118) += lan9118.o
> diff --git a/default-configs/pci.mak b/default-configs/pci.mak
> index 22bd350..d2ea7a2 100644
> --- a/default-configs/pci.mak
> +++ b/default-configs/pci.mak
> @@ -10,6 +10,7 @@ CONFIG_PCNET_PCI=y
>   CONFIG_PCNET_COMMON=y
>   CONFIG_LSI_SCSI_PCI=y
>   CONFIG_RTL8139_PCI=y
> +CONFIG_R6040_PCI=y
>   CONFIG_E1000_PCI=y
>   CONFIG_IDE_CORE=y
>   CONFIG_IDE_QDEV=y
> diff --git a/hw/pci.c b/hw/pci.c
> index b904a4e..7e12935 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1527,6 +1527,7 @@ static const char * const pci_nic_models[] = {
>       "rtl8139",
>       "e1000",
>       "pcnet",
> +    "r6040",
>       "virtio",
>       NULL
>   };
> @@ -1539,6 +1540,7 @@ static const char * const pci_nic_names[] = {
>       "rtl8139",
>       "e1000",
>       "pcnet",
> +    "r6040",
>       "virtio-net-pci",
>       NULL
>   };
> diff --git a/hw/r6040.c b/hw/r6040.c
> new file mode 100644
> index 0000000..83587e7
> --- /dev/null
> +++ b/hw/r6040.c
> @@ -0,0 +1,627 @@
> +/*
> +   Emulation of r6040 ethernet controller found in a number of SoCs.
> +   Copyright (c) 2011 Mark Kelly, mark@bifferos.com
> +
> +   This has been written using the R8610[1] and ip101a[2] datasheets.
> +
> +   ICs with the embedded controller include R8610, R3210, AMRISC20000
> +   and Vortex86SX
> +
> +   The emulation seems good enough to fool Linux 2.6.37.6.  It is
> +   not perfect, but has proven useful.
> +
> +   [1] http://www.sima.com.tw/download/R8610_D06_20051003.pdf
> +   [2] http://www.icplus.com.tw/pp-IP101A.html
> + */
> +
> +#include "hw.h"
> +#include "pci.h"
> +#include "net.h"
> +#include "loader.h"
> +#include "sysemu.h"
> +#include "qemu-timer.h"
> +
> +/* #define DEBUG_R6040 1 */
> +
> +
> +#if defined DEBUG_R6040
> +#define DPRINTF(fmt, ...) \
> +    do { fprintf(stderr, "R6040: " fmt, ## __VA_ARGS__); } while (0)
> +#else
> +static inline GCC_FMT_ATTR(1, 2) int DPRINTF(const char *fmt, ...)
> +{
> +    return 0;
> +}
> +#endif
> +
> +
> +/* Cast in order of appearance.  _W prevfix means it's used to index the
> +   register word array (RegsW)
> + */
> +
> +#define MPSCCR_W         (0x88 / 2)
> +
> +#define MAC0_W           (0x68 / 2)
> +#define MAC1_W           (0x6a / 2)
> +#define MAC2_W           (0x6c / 2)
> +
> +
> +#define RX_START_LOW_W   (0x34 / 2)
> +#define RX_START_HIGH_W  (0x38 / 2)
> +#define TX_PKT_COUNT_W   (0x5a / 2)
> +#define RX_PKT_COUNT_W   (0x50 / 2)
> +
> +
> +#define MCR0_W           (0x00 / 2)
> +#define MCR1_W           (0x04 / 2)
> +#define BIT_MRST         (1<<  0)
> +
> +#define MTPR_W           (0x14 / 2)
> +#define MRBSR_W          (0x18 / 2)
> +#define MISR_W           (0x3c / 2)
> +#define MIER_W           (0x40 / 2)
> +
> +#define MMDIO_W          (0x20 / 2)
> +#define MDIO_READ_W      (0x24 / 2)
> +#define MDIO_WRITE_W     (0x28 / 2)
> +
> +#define MRCNT_W          (0x50 / 2)
> +#define MTCNT_W          (0x5c / 2)
> +
> +
> +#define MDIO_WRITE      0x4000
> +#define MDIO_READ       0x2000
> +
> +
> +typedef struct R6040State {
> +    PCIDevice dev;
> +    NICState *nic;
> +    NICConf conf;
> +
> +    /* PHY related register sets */
> +    uint16_t MID0[3];
> +    uint16_t phy_regs[32];
> +    uint32_t phy_op_in_progress;
> +
> +    /* Primary IO address space */
> +    union {
> +        uint8_t RegsB[0x100];   /* Byte access */
> +        uint16_t RegsW[0x100/2];  /* word access */
> +        uint32_t RegsL[0x100/4];  /* DWORD access */
> +    };
> +
> +} R6040State;
> +
> +
> +/* some inlines to help access above structure */
> +static inline uint32_t TX_START(R6040State *s)
> +{
> +    uint32_t tmp = s->RegsW[0x2c/2];
> +    return tmp | (s->RegsW[0x30/2]<<  16);
> +}
> +
> +static inline void TX_START_SET(R6040State *s, uint32_t start)
> +{
> +    s->RegsW[0x2c/2] = start&  0xffff;
> +    s->RegsW[0x30/2] = (start>>  16)&  0xffff;
> +}
> +
> +static inline uint32_t RX_START(R6040State *s)
> +{
> +    uint32_t tmp = s->RegsW[0x34/2];
> +    return tmp | (s->RegsW[0x38/2]<<  16);
> +}
> +
> +static inline void RX_START_SET(R6040State *s, uint32_t start)
> +{
> +    s->RegsW[0x34/2] = start&  0xffff;
> +    s->RegsW[0x38/2] = (start>>  16)&  0xffff;
> +}
> +
> +
> +static void r6040_update_irq(R6040State *s)
> +{
> +    uint16_t isr = s->RegsW[MISR_W]&  s->RegsW[MIER_W];
> +
> +    qemu_set_irq(s->dev.irq[0], isr ? 1 : 0);
> +}
> +
> +
> +/* Mark auto-neg complete, NIC up.  */
> +static void PhysicalLinkUp(void *opaque)
> +{
> +    R6040State *s = opaque;
> +    s->phy_regs[1] |= (1<<  2);
> +}
> +
> +
> +/* Transmit and receive descriptors are doubled up
> +   One is a subset of the other anyhow
> + */
> +typedef struct descriptor {
> +    uint16_t DST;
> +    uint16_t DLEN;
> +    uint32_t DBP;
> +    uint32_t DNX;
> +    uint16_t HIDX;
> +    uint16_t Reserved1;
> +    uint16_t Reserved2;
> +} descriptor_t;
> +
> +
> +/* Some debugging functions */
> +
> +#ifdef DEBUG_R6040
> +static void addr_dump16(const char *name, uint16_t val)
> +{
> +    DPRINTF("%s: 0x%04x  ", name, val);
> +}
> +
> +static void addr_dump32(const char *name, uint32_t val)
> +{
> +    DPRINTF("%s: 0x%x  ", name, val);
> +}
> +
> +static void hex_dump(const uint8_t *data, uint32_t len)
> +{
> +    uint8_t i;
> +    DPRINTF("hex: ");
> +    for (i = 0; i<  len; i++) {
> +        fprintf(stderr, "%02x ", *data);
> +        if (i&&  !(i % 0x20)) {
> +            fprintf(stderr, "\n");
> +        }
> +        data++;
> +    }
> +    fprintf(stderr, "\n");
> +}
> +
> +static void desc_dump(descriptor_t *d, uint32_t addr)
> +{
> +    DPRINTF("\nDumping: 0x%x\n", addr);
> +    addr_dump16("DST", d->DST);
> +    addr_dump16("DLEN", d->DLEN);
> +    addr_dump32("DBP", (unsigned long)d->DBP);
> +    addr_dump32("DNX", (unsigned long)d->DNX);
> +    addr_dump16("HIDX", d->HIDX);
> +    printf("\n");
> +}
> +
> +static void dump_phys_mem(uint32_t addr, int len)
> +{
> +    uint8_t buffer[1024];
> +    cpu_physical_memory_read(addr, buffer, len);
> +    hex_dump(buffer, len);
> +}
> +
> +static void dump_pci(uint8_t *pci_conf)
> +{
> +    uint32_t *p = (uint32_t *)pci_conf;
> +    int i = 0;
> +    for (i = 0; i<  0x40; i += 4) {
> +        DPRINTF("Addr: 0x%08x,  Data: 0x%08x\n", i, *p);
> +        p++;
> +    }
> +}
> +#endif
> +
> +
> +static const VMStateDescription vmstate_r6040 = {
> +    .name = "r6040",
> +    .version_id = 3,
> +    .minimum_version_id = 2,
> +    .minimum_version_id_old = 2,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_PCI_DEVICE(dev, R6040State),
> +        VMSTATE_BUFFER(RegsB, R6040State),
> +        VMSTATE_UINT16_ARRAY(MID0, R6040State, 3),
> +        VMSTATE_UINT16_ARRAY(phy_regs, R6040State, 32),
> +        VMSTATE_UINT32(phy_op_in_progress, R6040State),
> +        VMSTATE_MACADDR(conf.macaddr, R6040State),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +
> +static int TryToSendOnePacket(void *opaque)
> +{
> +    R6040State *s = opaque;
> +    descriptor_t d;
> +    uint8_t pkt_buffer[2000];
> +    uint32_t tocopy;
> +
> +    cpu_physical_memory_read(TX_START(s), (uint8_t *)&d, sizeof(d));
> +
> +    if (d.DST&  0x8000) {    /* MAC owns it?  */
> +        tocopy = d.DLEN;
> +        if (tocopy>  sizeof(pkt_buffer)) {
> +            tocopy = sizeof(pkt_buffer);
> +        }
> +        /* copy the packet to send it */
> +        cpu_physical_memory_read(d.DBP, pkt_buffer, tocopy);
> +
> +        qemu_send_packet(&s->nic->nc, pkt_buffer, tocopy);
> +        s->RegsW[TX_PKT_COUNT_W]++;
> +
> +        /* relinquish ownership, we're done with it */
> +        d.DST&= ~0x8000;
> +
> +        /* Copy the new version of the descriptor back */
> +        cpu_physical_memory_write(TX_START(s), (uint8_t *)&d, sizeof(d));
> +
> +        /* Advance to the next buffer if packet processed */
> +        TX_START_SET(s, d.DNX);
> +
> +        return 1;
> +    }
> +
> +    return 0;
> +}
> +
> +
> +static void r6040_transmit(void *opaque)
> +{
> +    R6040State *s = opaque;
> +    int count = 0;
> +
> +    while (TryToSendOnePacket(s)) {
> +        ++count;
> +    }
> +
> +    if (count) {
> +        s->RegsW[MISR_W] |= 0x10;
> +        r6040_update_irq(s);
> +    }
> +}
> +
> +
> +/* Whether to allow callback returning 1 for yes, can receive */
> +static int r6040_can_receive(VLANClientState *nc)
> +{
> +    R6040State *s = DO_UPCAST(NICState, nc, nc)->opaque;
> +    int tmp = (s->RegsW[0]&  (1<<  1)) ? 1 : 0;
> +    return tmp;
> +}
> +
> +
> +static int ReceiveOnePacket(void *opaque, const uint8_t *buf, size_t len)
> +{
> +    R6040State *s = opaque;
> +    uint32_t tocopy = len+4;  /* include checksum */
> +    descriptor_t d;
> +
> +    cpu_physical_memory_read(RX_START(s), (uint8_t *)&d, sizeof(descriptor_t));
> +    /*desc_dump(&d, 0);*/
> +
> +    if (d.DST&  0x8000) {    /* MAC owned? */
> +
> +        uint16_t max_buffer = s->RegsW[MRBSR_W]&  0x07fc;
> +        if (tocopy>  max_buffer) {
> +            tocopy = max_buffer;
> +        }
> +
> +        cpu_physical_memory_write(d.DBP, buf, tocopy-4);
> +
> +        /* indicate received OK */
> +        d.DST |= (1<<  14);
> +        d.DLEN = tocopy;
> +        /* relinquish ownership */
> +        d.DST&= ~0x8000;
> +
> +        /* Copy the descriptor back */
> +        cpu_physical_memory_write(RX_START(s), (uint8_t *)&d,
> +                                   sizeof(descriptor_t));
> +
> +        s->RegsW[RX_PKT_COUNT_W]++;
> +
> +        s->RegsW[MISR_W] |= 1;  /* received pkt interrupt */
> +
> +        r6040_update_irq(s);
> +
> +        RX_START_SET(s, d.DNX);  /* advance */
> +
> +        return 0;
> +    }
> +    return -1;
> +}
> +
> +
> +/* called on incoming packets */
> +static ssize_t r6040_receive(VLANClientState *nc, const uint8_t *buf,
> +                                        size_t len)
> +{
> +    R6040State *s = DO_UPCAST(NICState, nc, nc)->opaque;
> +    DPRINTF("Received incoming packet of len %ld\n", len);
> +
> +    if (0 == ReceiveOnePacket(s, buf, len)) {
> +        return len;  /* copied OK */
> +    }
> +
> +    return 0;
> +}
> +
> +
> +static void r6040_cleanup(VLANClientState *vc)
> +{
> +    DPRINTF("r6040_cleanup\n");
> +}
> +
> +
> +static inline int BIT_SET(uint16_t old, uint16_t new, uint16_t bit)
> +{
> +    uint16_t before = (old&  (1<<  bit));
> +    uint16_t after = (new&  (1<<  bit));
> +    if (!before&&  after) {
> +        return 1;
> +    }
> +    return 0;
> +}
> +
> +
> +static void r6040_ioport_writew(void *opaque, uint32_t addr, uint32_t val)
> +{
> +    R6040State *s = opaque;
> +    uint16_t old;
> +    addr&= 0xff;   /* get relative to base address */
> +    addr /= 2;    /* Get the offset into the word-array */
> +    old = s->RegsW[addr];   /* store the old value for future use */
> +
> +    switch (addr) {
> +    case MCR0_W:   /* 0x00 */
> +        if (BIT_SET(old, val, 12)) {
> +            r6040_transmit(opaque);
> +        }
> +        break;
> +    case MCR1_W:  /* 0x04 */
> +        if (val&  BIT_MRST) {   /* reset request incoming */
> +            /* reset requested, complete it immediately, set this value to
> +               default */
> +            val = 0x0010;
> +        }
> +        break;
> +    case MTPR_W:  /* TX command reg, 0x14 */
> +        if (val&  1) {
> +            r6040_transmit(opaque);
> +            val&= ~1;
> +        }
> +        break;
> +    case MMDIO_W:  /* MDIO control, 0x20 */
> +        {
> +            int phy_exists = ((val&  0x1f00) == 0x100) ? 1 : 0;
> +            uint16_t *phy = s->phy_regs;
> +            phy += (val&  0x1f);
> +
> +            if  (val&  (1<<  13)) {   /* read data */
> +                if (phy_exists) {
> +                    s->RegsW[MDIO_READ_W] = *phy;
> +                } else {
> +                    s->RegsW[MDIO_READ_W] = 0xffff;
> +                }
> +            } else if (val&  (1<<  14)) {  /* write data */
> +                if (phy_exists) {
> +                    *phy = s->RegsW[MDIO_WRITE_W];
> +                }
> +            }
> +
> +            /* Whether you request to read or write, both bits go high while
> +               the operation is in progress, e.g. tell it to read, and the
> +               write-in-progress flag also goes high */
> +            val |= 0x6000;  /* signal operation has started */
> +            s->phy_op_in_progress = 1;
> +
> +            break;
> +        }
> +    case MISR_W:  /* interrupt status reg (read to clear), 0x3c */
> +        return;
> +
> +    case MIER_W:  /* interrupt enable register, 0x40 */
> +        s->RegsW[MIER_W] = val;
> +        r6040_update_irq(s);
> +        return;
> +
> +    case MRCNT_W:   /* 0x50 */
> +    case MTCNT_W:   /* 0x5c */
> +        return;  /* Can't write to pkt count registers, skip */
> +
> +    }
> +    s->RegsW[addr] = val&  0xFFFF;
> +}
> +
> +
> +
> +static uint32_t r6040_ioport_readw(void *opaque, uint32_t addr)
> +{
> +    R6040State *s = opaque;
> +    addr&= 0xff;   /* get relative to base address */
> +    addr /= 2;    /* Get the offset into the word-array */
> +    uint32_t tmp = s->RegsW[addr];  /* get the value */
> +
> +    switch (addr) {
> +
> +    case MMDIO_W:  /* MDIO control, 0x20 */
> +        {
> +            /* Clear any in-progress MDIO activity for the next read
> +               This simulates the polling of the MDIO operation status,
> +               so the driver code always has to read the register twice
> +               before it thinks the operation is complete. */
> +            if (s->phy_op_in_progress) {
> +                s->RegsW[addr]&= ~0x6000;
> +                s->phy_op_in_progress = 0;
> +            }
> +            break;
> +        }
> +    case MISR_W:  /* interrupt status reg (read to clear)  0x3c */
> +        s->RegsW[addr] = 0;
> +        break;
> +    case MIER_W:  /* interrupt enable reg 0x40 */
> +        break;
> +    case MRCNT_W:  /* 0x50 */
> +    case MTCNT_W:  /* 0x5c */
> +        s->RegsW[addr] = 0;   /* read to clear */
> +        break;
> +    default:
> +        break;
> +    }
> +    return tmp;
> +}
> +
> +
> +/* byte and long access are routed via the word operation handlers */
> +static void r6040_ioport_writeb(void *opaque, uint32_t addr, uint32_t val)
> +{
> +    R6040State *s = opaque;
> +    addr&= 0xFF;
> +    val&= 0xFF;
> +    uint16_t old = s->RegsW[addr/2];  /* get the current value */
> +    if (addr&  1) {
> +        old&= 0xff;
> +        old |= (val<<  8);
> +    } else {
> +        old&= 0xff00;
> +        old |= val;
> +    }
> +
> +    r6040_ioport_writew(opaque, addr, old);  /* call the word-based version */
> +}
> +
> +static void r6040_ioport_writel(void *opaque, uint32_t addr, uint32_t val)
> +{
> +    /* Set the low value */
> +    r6040_ioport_writew(opaque, addr, val&  0xffff);
> +    /* Set the high value */
> +    r6040_ioport_writew(opaque, addr+2, (val>>  16)&  0xffff);
> +}
> +
> +static uint32_t r6040_ioport_readb(void *opaque, uint32_t addr)
> +{
> +    uint32_t tmp = r6040_ioport_readw(opaque, addr&  ~1);
> +    if (addr&  1) {
> +        return (tmp&  0xff00)>>  8;
> +    }
> +    return tmp&  0xff;
> +}
> +
> +static uint32_t r6040_ioport_readl(void *opaque, uint32_t addr)
> +{
> +    uint32_t tmp = r6040_ioport_readw(opaque, addr);
> +    return tmp | (r6040_ioport_readw(opaque, addr+2)<<  16);
> +}
> +
> +
> +static void r6040_register_ioports(R6040State *s, pcibus_t addr)
> +{
> +    register_ioport_write(addr, 0x100, 1, r6040_ioport_writeb, s);
> +    register_ioport_read(addr, 0x100, 1, r6040_ioport_readb,  s);
> +
> +    register_ioport_write(addr, 0x100, 2, r6040_ioport_writew, s);
> +    register_ioport_read(addr, 0x100, 2, r6040_ioport_readw,  s);
> +
> +    register_ioport_write(addr, 0x100, 4, r6040_ioport_writel, s);
> +    register_ioport_read(addr, 0x100, 4, r6040_ioport_readl,  s);
> +}
> +
> +
> +static void r6040_map(PCIDevice *pci_dev, int region_num,
> +                       pcibus_t addr, pcibus_t size, int type)
> +{
> +    R6040State *s = DO_UPCAST(R6040State, dev, pci_dev);;
> +
> +    DPRINTF("## Mapping to address %lx\n", addr);
> +    r6040_register_ioports(s, addr);
> +}
> +
> +
> +static NetClientInfo net_r6040_info = {
> +    .type = NET_CLIENT_TYPE_NIC,
> +    .size = sizeof(NICState),
> +    .can_receive = r6040_can_receive,
> +    .receive = r6040_receive,
> +    .cleanup = r6040_cleanup,
> +};
> +
> +
> +static int r6040_init_pci(PCIDevice *dev)
> +{
> +    QEMUTimer *timer;
> +
> +    R6040State *s = DO_UPCAST(R6040State, dev, dev);
> +    uint8_t *pci_conf;
> +
> +    /* MAC PHYS status change register.  Linux driver expects something
> +       sensible as default and if not will try to set it */
> +    s->RegsW[MPSCCR_W] = 0x9f07;
> +
> +    /* Default value for maximum packet size */
> +    s->RegsW[MRBSR_W] = 0x600;
> +
> +    /* set the MAC, linux driver reads this when it loads, it is
> +       normally set by the BIOS, but obviously Qemu BIOS isn't going
> +       to do that */
> +    s->RegsW[MAC0_W] = 0x5452;
> +    s->RegsW[MAC1_W] = 0x1200;
> +    s->RegsW[MAC2_W] = 0x5734;
> +
> +    /* Tell Qemu the same thing */
> +    s->conf.macaddr.a[0] = s->RegsW[MAC0_W]&  0xff;
> +    s->conf.macaddr.a[1] = (s->RegsW[MAC0_W]>>  8)&  0xff;
> +    s->conf.macaddr.a[2] = s->RegsW[MAC1_W]&  0xff;
> +    s->conf.macaddr.a[3] = (s->RegsW[MAC1_W]>>  8)&  0xff;
> +    s->conf.macaddr.a[4] = s->RegsW[MAC2_W]&  0xff;
> +    s->conf.macaddr.a[5] = (s->RegsW[MAC2_W]>>  8)&  0xff;
> +
> +    /* no commands running */
> +    s->phy_op_in_progress = 0;
> +
> +    /* PHY auto-neg in progress */
> +    s->phy_regs[1] = 0x786d&  ~(1<<  2);
> +    s->phy_regs[2] = 0x0243;
> +    s->phy_regs[3] = 0x0c54;
> +
> +    pci_conf = (uint8_t *)s->dev.config;
> +
> +    pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; /* header_type */
> +    pci_conf[PCI_INTERRUPT_LINE] = 0xa;     /* interrupt line  */
> +    pci_conf[PCI_INTERRUPT_PIN] = 1;      /* interrupt pin 0 */
> +
> +    pci_register_bar(&s->dev, 0, 0x100, PCI_BASE_ADDRESS_SPACE_IO, r6040_map);
> +
> +    s->nic = qemu_new_nic(&net_r6040_info,&s->conf,
> +                            dev->qdev.info->name, dev->qdev.id, s);
> +
> +    qemu_format_nic_info_str(&s->nic->nc, s->conf.macaddr.a);
> +
> +    /* Simulate a delay of a couple of seconds for the link to come up.
> +       Not required for Linux but very handy for developing BIOS code.
> +     */
> +    timer = qemu_new_timer_ns(vm_clock, PhysicalLinkUp, s);
> +    qemu_mod_timer(timer, qemu_get_clock_ms(vm_clock) + 1500000000);
> +
> +    /* Register IO port access as well */
> +    r6040_register_ioports(s, 0xe800);
> +
> +    return 0;
> +}
> +
> +
> +static PCIDeviceInfo r6040_info = {
> +    .qdev.name = "r6040",
> +    .qdev.size = sizeof(R6040State),
> +    .qdev.vmsd  =&vmstate_r6040,
> +    .init      = r6040_init_pci,
> +    .vendor_id = 0x17f3,  /* RDC */
> +    .device_id = 0x6040,   /* r6040 nic */
> +    .class_id   = PCI_CLASS_NETWORK_ETHERNET,
> +    .qdev.props = (Property[]) {
> +        DEFINE_NIC_PROPERTIES(R6040State, conf),
> +        DEFINE_PROP_END_OF_LIST(),
> +    }
> +};
> +
> +
> +static void r6040_register(void)
> +{
> +    pci_qdev_register(&r6040_info);
> +}
> +
> +
> +device_init(r6040_register)
>
>

Re: [Qemu-devel] [PATCH] Add support for r6040 NIC

[malc]

On Tue, 30 Aug 2011, Anthony Liguori wrote:

> This won't even come close to passing checkpatch.pl

Have you actually tried?

> 
> Regards,
> 
> Anthony Liguori
> 
> On 08/30/2011 08:05 PM, bifferos wrote:
> > 
> > Signed-off-by: Mark Kelly<mark@bifferos.com>
> > diff --git a/Makefile.objs b/Makefile.objs
> > index 6991a9f..7d87503 100644
> > --- a/Makefile.objs
> > +++ b/Makefile.objs
> > @@ -240,6 +240,7 @@ hw-obj-$(CONFIG_PCNET_PCI) += pcnet-pci.o
> >   hw-obj-$(CONFIG_PCNET_COMMON) += pcnet.o
> >   hw-obj-$(CONFIG_E1000_PCI) += e1000.o
> >   hw-obj-$(CONFIG_RTL8139_PCI) += rtl8139.o
> > +hw-obj-$(CONFIG_R6040_PCI) += r6040.o
> > 
> >   hw-obj-$(CONFIG_SMC91C111) += smc91c111.o
> >   hw-obj-$(CONFIG_LAN9118) += lan9118.o
> > diff --git a/default-configs/pci.mak b/default-configs/pci.mak
> > index 22bd350..d2ea7a2 100644
> > --- a/default-configs/pci.mak
> > +++ b/default-configs/pci.mak
> > @@ -10,6 +10,7 @@ CONFIG_PCNET_PCI=y
> >   CONFIG_PCNET_COMMON=y
> >   CONFIG_LSI_SCSI_PCI=y
> >   CONFIG_RTL8139_PCI=y
> > +CONFIG_R6040_PCI=y
> >   CONFIG_E1000_PCI=y
> >   CONFIG_IDE_CORE=y
> >   CONFIG_IDE_QDEV=y
> > diff --git a/hw/pci.c b/hw/pci.c
> > index b904a4e..7e12935 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -1527,6 +1527,7 @@ static const char * const pci_nic_models[] = {
> >       "rtl8139",
> >       "e1000",
> >       "pcnet",
> > +    "r6040",
> >       "virtio",
> >       NULL
> >   };
> > @@ -1539,6 +1540,7 @@ static const char * const pci_nic_names[] = {
> >       "rtl8139",
> >       "e1000",
> >       "pcnet",
> > +    "r6040",
> >       "virtio-net-pci",
> >       NULL
> >   };
> > diff --git a/hw/r6040.c b/hw/r6040.c
> > new file mode 100644
> > index 0000000..83587e7
> > --- /dev/null
> > +++ b/hw/r6040.c
> > @@ -0,0 +1,627 @@
> > +/*
> > +   Emulation of r6040 ethernet controller found in a number of SoCs.
> > +   Copyright (c) 2011 Mark Kelly, mark@bifferos.com

This doesn't spell out under which conditions this can be used by
people other than Mark Kelly.

> > +
> > +   This has been written using the R8610[1] and ip101a[2] datasheets.
> > +
> > +   ICs with the embedded controller include R8610, R3210, AMRISC20000
> > +   and Vortex86SX
> > +
> > +   The emulation seems good enough to fool Linux 2.6.37.6.  It is
> > +   not perfect, but has proven useful.
> > +
> > +   [1] http://www.sima.com.tw/download/R8610_D06_20051003.pdf
> > +   [2] http://www.icplus.com.tw/pp-IP101A.html
> > + */
> > +
> > +#include "hw.h"
> > +#include "pci.h"
> > +#include "net.h"
> > +#include "loader.h"
> > +#include "sysemu.h"
> > +#include "qemu-timer.h"
> > +
> > +/* #define DEBUG_R6040 1 */
> > +
> > +
> > +#if defined DEBUG_R6040
> > +#define DPRINTF(fmt, ...) \
> > +    do { fprintf(stderr, "R6040: " fmt, ## __VA_ARGS__); } while (0)
> > +#else
> > +static inline GCC_FMT_ATTR(1, 2) int DPRINTF(const char *fmt, ...)
> > +{
> > +    return 0;
> > +}
> > +#endif
> > +
> > +
> > +/* Cast in order of appearance.  _W prevfix means it's used to index the

                                         prefix (though suffix is even better)

[..snip..]

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] Add support for r6040 NIC

[Anthony Liguori]

On 08/30/2011 08:30 PM, malc wrote:
> On Tue, 30 Aug 2011, Anthony Liguori wrote:
>
>> This won't even come close to passing checkpatch.pl
>
> Have you actually tried?

Sigh.  I was hoping checkpatch.pl was more useful than it appears to be.

At any rate, the patch doesn't follow CODING_STYLE.

Regards,

Anthony Liguori

>
>>
>> Regards,
>>
>> Anthony Liguori
>>
>> On 08/30/2011 08:05 PM, bifferos wrote:
>>>
>>> Signed-off-by: Mark Kelly<mark@bifferos.com>
>>> diff --git a/Makefile.objs b/Makefile.objs
>>> index 6991a9f..7d87503 100644
>>> --- a/Makefile.objs
>>> +++ b/Makefile.objs
>>> @@ -240,6 +240,7 @@ hw-obj-$(CONFIG_PCNET_PCI) += pcnet-pci.o
>>>    hw-obj-$(CONFIG_PCNET_COMMON) += pcnet.o
>>>    hw-obj-$(CONFIG_E1000_PCI) += e1000.o
>>>    hw-obj-$(CONFIG_RTL8139_PCI) += rtl8139.o
>>> +hw-obj-$(CONFIG_R6040_PCI) += r6040.o
>>>
>>>    hw-obj-$(CONFIG_SMC91C111) += smc91c111.o
>>>    hw-obj-$(CONFIG_LAN9118) += lan9118.o
>>> diff --git a/default-configs/pci.mak b/default-configs/pci.mak
>>> index 22bd350..d2ea7a2 100644
>>> --- a/default-configs/pci.mak
>>> +++ b/default-configs/pci.mak
>>> @@ -10,6 +10,7 @@ CONFIG_PCNET_PCI=y
>>>    CONFIG_PCNET_COMMON=y
>>>    CONFIG_LSI_SCSI_PCI=y
>>>    CONFIG_RTL8139_PCI=y
>>> +CONFIG_R6040_PCI=y
>>>    CONFIG_E1000_PCI=y
>>>    CONFIG_IDE_CORE=y
>>>    CONFIG_IDE_QDEV=y
>>> diff --git a/hw/pci.c b/hw/pci.c
>>> index b904a4e..7e12935 100644
>>> --- a/hw/pci.c
>>> +++ b/hw/pci.c
>>> @@ -1527,6 +1527,7 @@ static const char * const pci_nic_models[] = {
>>>        "rtl8139",
>>>        "e1000",
>>>        "pcnet",
>>> +    "r6040",
>>>        "virtio",
>>>        NULL
>>>    };
>>> @@ -1539,6 +1540,7 @@ static const char * const pci_nic_names[] = {
>>>        "rtl8139",
>>>        "e1000",
>>>        "pcnet",
>>> +    "r6040",
>>>        "virtio-net-pci",
>>>        NULL
>>>    };
>>> diff --git a/hw/r6040.c b/hw/r6040.c
>>> new file mode 100644
>>> index 0000000..83587e7
>>> --- /dev/null
>>> +++ b/hw/r6040.c
>>> @@ -0,0 +1,627 @@
>>> +/*
>>> +   Emulation of r6040 ethernet controller found in a number of SoCs.
>>> +   Copyright (c) 2011 Mark Kelly, mark@bifferos.com
>
> This doesn't spell out under which conditions this can be used by
> people other than Mark Kelly.
>
>>> +
>>> +   This has been written using the R8610[1] and ip101a[2] datasheets.
>>> +
>>> +   ICs with the embedded controller include R8610, R3210, AMRISC20000
>>> +   and Vortex86SX
>>> +
>>> +   The emulation seems good enough to fool Linux 2.6.37.6.  It is
>>> +   not perfect, but has proven useful.
>>> +
>>> +   [1] http://www.sima.com.tw/download/R8610_D06_20051003.pdf
>>> +   [2] http://www.icplus.com.tw/pp-IP101A.html
>>> + */
>>> +
>>> +#include "hw.h"
>>> +#include "pci.h"
>>> +#include "net.h"
>>> +#include "loader.h"
>>> +#include "sysemu.h"
>>> +#include "qemu-timer.h"
>>> +
>>> +/* #define DEBUG_R6040 1 */
>>> +
>>> +
>>> +#if defined DEBUG_R6040
>>> +#define DPRINTF(fmt, ...) \
>>> +    do { fprintf(stderr, "R6040: " fmt, ## __VA_ARGS__); } while (0)
>>> +#else
>>> +static inline GCC_FMT_ATTR(1, 2) int DPRINTF(const char *fmt, ...)
>>> +{
>>> +    return 0;
>>> +}
>>> +#endif
>>> +
>>> +
>>> +/* Cast in order of appearance.  _W prevfix means it's used to index the
>
>                                           prefix (though suffix is even better)
>
> [..snip..]
>

Re: [Qemu-devel] [PATCH] Add support for r6040 NIC

[malc]

On Tue, 30 Aug 2011, Anthony Liguori wrote:

> On 08/30/2011 08:30 PM, malc wrote:
> > On Tue, 30 Aug 2011, Anthony Liguori wrote:
> > 
> > > This won't even come close to passing checkpatch.pl
> > 
> > Have you actually tried?
> 
> Sigh.  I was hoping checkpatch.pl was more useful than it appears to be.
> 
> At any rate, the patch doesn't follow CODING_STYLE.
> 

Where?

[..snip..]

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] Add support for r6040 NIC

[Anthony Liguori]

On 08/31/2011 08:17 AM, malc wrote:
> On Tue, 30 Aug 2011, Anthony Liguori wrote:
>
>> On 08/30/2011 08:30 PM, malc wrote:
>>> On Tue, 30 Aug 2011, Anthony Liguori wrote:
>>>
>>>> This won't even come close to passing checkpatch.pl
>>>
>>> Have you actually tried?
>>
>> Sigh.  I was hoping checkpatch.pl was more useful than it appears to be.
>>
>> At any rate, the patch doesn't follow CODING_STYLE.
>>
>
> Where?

3. Naming

Variables are lower_case_with_underscores; easy to type and read. 
Structured
type names are in CamelCase; harder to type but standing out.  Scalar type
names are lower_case_with_underscores_ending_with_a_t, like the POSIX
uint64_t and family.  Note that this last convention contradicts POSIX
and is therefore likely to be changed.

When wrapping standard library functions, use the prefix qemu_ to alert
readers that they are seeing a wrapped version; otherwise avoid this prefix.

Regards,

Anthony Liguori


> [..snip..]
>

Re: [Qemu-devel] [PATCH] Add support for r6040 NIC

[bifferos]

--- On Wed, 31/8/11, malc <av1474@comtv.ru> wrote:

> From: malc <av1474@comtv.ru>
> Subject: Re: [Qemu-devel] [PATCH] Add support for r6040 NIC
> To: "Anthony Liguori" <anthony@codemonkey.ws>
> Cc: qemu-devel@nongnu.org, "bifferos" <bifferos@yahoo.co.uk>
> Date: Wednesday, 31 August, 2011, 14:17
> On Tue, 30 Aug 2011, Anthony Liguori
> wrote:
> 
> > On 08/30/2011 08:30 PM, malc wrote:
> > > On Tue, 30 Aug 2011, Anthony Liguori wrote:
> > > 
> > > > This won't even come close to passing
> checkpatch.pl
> > > 
> > > Have you actually tried?
> > 
> > Sigh.  I was hoping checkpatch.pl was more useful
> than it appears to be.
> > 
> > At any rate, the patch doesn't follow CODING_STYLE.
> > 
> 
> Where?

My apologies, actually I had a half-hearted look for the coding style, came to this link:

http://git.qemu.org/qemu.git/plain/CODING_STYLE

Which was dead, and then fell back on the checkpatch.pl script, thinking nobody cared so much about coding styles.  I should have looked a bit more carefully.  Since then I found this:

http://git.savannah.gnu.org/cgit/qemu.git/tree/CODING_STYLE

AFAICS the problem is with the naming (part 3), which I can correct tonight (along with the other issues raised)

Re: [Qemu-devel] [PATCH] Add support for r6040 NIC

[malc]

On Wed, 31 Aug 2011, Anthony Liguori wrote:

> On 08/31/2011 08:17 AM, malc wrote:
> > On Tue, 30 Aug 2011, Anthony Liguori wrote:
> > 
> > > On 08/30/2011 08:30 PM, malc wrote:
> > > > On Tue, 30 Aug 2011, Anthony Liguori wrote:
> > > > 
> > > > > This won't even come close to passing checkpatch.pl
> > > > 
> > > > Have you actually tried?
> > > 
> > > Sigh.  I was hoping checkpatch.pl was more useful than it appears to be.
> > > 
> > > At any rate, the patch doesn't follow CODING_STYLE.
> > > 
> > 
> > Where?
> 
> 3. Naming
> 
> Variables are lower_case_with_underscores; easy to type and read. Structured
> type names are in CamelCase; harder to type but standing out.  Scalar type
> names are lower_case_with_underscores_ending_with_a_t, like the POSIX
> uint64_t and family.  Note that this last convention contradicts POSIX
> and is therefore likely to be changed.

Where in the patch this was violated, and do note that fields are not
variables.

> 
> When wrapping standard library functions, use the prefix qemu_ to alert
> readers that they are seeing a wrapped version; otherwise avoid this prefix.

And what ^^^ has to do with anything?

> 
> Regards,
> 
> Anthony Liguori
> 
> 
> > [..snip..]
> > 
> 

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] Add support for r6040 NIC

[Anthony Liguori]

On 08/31/2011 08:39 AM, malc wrote:
> On Wed, 31 Aug 2011, Anthony Liguori wrote:
>
>> On 08/31/2011 08:17 AM, malc wrote:
>>> On Tue, 30 Aug 2011, Anthony Liguori wrote:
>>>
>>>> On 08/30/2011 08:30 PM, malc wrote:
>>>>> On Tue, 30 Aug 2011, Anthony Liguori wrote:
>>>>>
>>>>>> This won't even come close to passing checkpatch.pl
>>>>>
>>>>> Have you actually tried?
>>>>
>>>> Sigh.  I was hoping checkpatch.pl was more useful than it appears to be.
>>>>
>>>> At any rate, the patch doesn't follow CODING_STYLE.
>>>>
>>>
>>> Where?
>>
>> 3. Naming
>>
>> Variables are lower_case_with_underscores; easy to type and read. Structured
>> type names are in CamelCase; harder to type but standing out.  Scalar type
>> names are lower_case_with_underscores_ending_with_a_t, like the POSIX
>> uint64_t and family.  Note that this last convention contradicts POSIX
>> and is therefore likely to be changed.
>
> Where in the patch this was violated, and do note that fields are not
> variables.

fields are variables.  And the struct names weren't all CamelCase.

Regards,

Anthony Liguori

>
>>
>> When wrapping standard library functions, use the prefix qemu_ to alert
>> readers that they are seeing a wrapped version; otherwise avoid this prefix.
>
> And what ^^^ has to do with anything?
>
>>
>> Regards,
>>
>> Anthony Liguori
>>
>>
>>> [..snip..]
>>>
>>
>

Re: [Qemu-devel] [PATCH] Add support for r6040 NIC

[malc]

On Wed, 31 Aug 2011, Anthony Liguori wrote:

> On 08/31/2011 08:39 AM, malc wrote:
> > On Wed, 31 Aug 2011, Anthony Liguori wrote:
> > 
> > > On 08/31/2011 08:17 AM, malc wrote:
> > > > On Tue, 30 Aug 2011, Anthony Liguori wrote:
> > > > 
> > > > > On 08/30/2011 08:30 PM, malc wrote:
> > > > > > On Tue, 30 Aug 2011, Anthony Liguori wrote:
> > > > > > 
> > > > > > > This won't even come close to passing checkpatch.pl
> > > > > > 
> > > > > > Have you actually tried?
> > > > > 
> > > > > Sigh.  I was hoping checkpatch.pl was more useful than it appears to
> > > > > be.
> > > > > 
> > > > > At any rate, the patch doesn't follow CODING_STYLE.
> > > > > 
> > > > 
> > > > Where?
> > > 
> > > 3. Naming
> > > 
> > > Variables are lower_case_with_underscores; easy to type and read.
> > > Structured
> > > type names are in CamelCase; harder to type but standing out.  Scalar type
> > > names are lower_case_with_underscores_ending_with_a_t, like the POSIX
> > > uint64_t and family.  Note that this last convention contradicts POSIX
> > > and is therefore likely to be changed.
> > 
> > Where in the patch this was violated, and do note that fields are not
> > variables.
> 
> fields are variables.  And the struct names weren't all CamelCase.

c&v please. The only thing i can agree with is descriptor_t other than
that patch is just fine.

> 
> Regards,
> 
> Anthony Liguori
> 
> > 
> > > 
> > > When wrapping standard library functions, use the prefix qemu_ to alert
> > > readers that they are seeing a wrapped version; otherwise avoid this
> > > prefix.
> > 
> > And what ^^^ has to do with anything?
> > 
> > > 
> > > Regards,
> > > 
> > > Anthony Liguori
> > > 
> > > 
> > > > [..snip..]
> > > > 
> > > 
> > 
> 

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] Add support for r6040 NIC

[Anthony Liguori]

On 08/31/2011 08:51 AM, malc wrote:
> On Wed, 31 Aug 2011, Anthony Liguori wrote:
>
>> On 08/31/2011 08:39 AM, malc wrote:
>>> On Wed, 31 Aug 2011, Anthony Liguori wrote:
>>>
>>>> On 08/31/2011 08:17 AM, malc wrote:
>>>>> On Tue, 30 Aug 2011, Anthony Liguori wrote:
>>>>>
>>>>>> On 08/30/2011 08:30 PM, malc wrote:
>>>>>>> On Tue, 30 Aug 2011, Anthony Liguori wrote:
>>>>>>>
>>>>>>>> This won't even come close to passing checkpatch.pl
>>>>>>>
>>>>>>> Have you actually tried?
>>>>>>
>>>>>> Sigh.  I was hoping checkpatch.pl was more useful than it appears to
>>>>>> be.
>>>>>>
>>>>>> At any rate, the patch doesn't follow CODING_STYLE.
>>>>>>
>>>>>
>>>>> Where?
>>>>
>>>> 3. Naming
>>>>
>>>> Variables are lower_case_with_underscores; easy to type and read.
>>>> Structured
>>>> type names are in CamelCase; harder to type but standing out.  Scalar type
>>>> names are lower_case_with_underscores_ending_with_a_t, like the POSIX
>>>> uint64_t and family.  Note that this last convention contradicts POSIX
>>>> and is therefore likely to be changed.
>>>
>>> Where in the patch this was violated, and do note that fields are not
>>> variables.
>>
>> fields are variables.  And the struct names weren't all CamelCase.
>
> c&v please. The only thing i can agree with is descriptor_t other than
> that patch is just fine.

Upper case field names are not okay.  If you think coding style isn't 
clear, that's a bug in coding style.

Just look at the vast majority of code in the tree.

Regards,

Anthony Liguori

>
>>
>> Regards,
>>
>> Anthony Liguori
>>
>>>
>>>>
>>>> When wrapping standard library functions, use the prefix qemu_ to alert
>>>> readers that they are seeing a wrapped version; otherwise avoid this
>>>> prefix.
>>>
>>> And what ^^^ has to do with anything?
>>>
>>>>
>>>> Regards,
>>>>
>>>> Anthony Liguori
>>>>
>>>>
>>>>> [..snip..]
>>>>>
>>>>
>>>
>>
>

Re: [Qemu-devel] [PATCH] Add support for r6040 NIC

[malc]

On Wed, 31 Aug 2011, Anthony Liguori wrote:

> On 08/31/2011 08:51 AM, malc wrote:
> > On Wed, 31 Aug 2011, Anthony Liguori wrote:
> > 
> > > On 08/31/2011 08:39 AM, malc wrote:
> > > > On Wed, 31 Aug 2011, Anthony Liguori wrote:
> > > > 
> > > > > On 08/31/2011 08:17 AM, malc wrote:
> > > > > > On Tue, 30 Aug 2011, Anthony Liguori wrote:
> > > > > > 
> > > > > > > On 08/30/2011 08:30 PM, malc wrote:
> > > > > > > > On Tue, 30 Aug 2011, Anthony Liguori wrote:
> > > > > > > > 
> > > > > > > > > This won't even come close to passing checkpatch.pl
> > > > > > > > 
> > > > > > > > Have you actually tried?
> > > > > > > 
> > > > > > > Sigh.  I was hoping checkpatch.pl was more useful than it appears
> > > > > > > to
> > > > > > > be.
> > > > > > > 
> > > > > > > At any rate, the patch doesn't follow CODING_STYLE.
> > > > > > > 
> > > > > > 
> > > > > > Where?
> > > > > 
> > > > > 3. Naming
> > > > > 
> > > > > Variables are lower_case_with_underscores; easy to type and read.
> > > > > Structured
> > > > > type names are in CamelCase; harder to type but standing out.  Scalar
> > > > > type
> > > > > names are lower_case_with_underscores_ending_with_a_t, like the POSIX
> > > > > uint64_t and family.  Note that this last convention contradicts POSIX
> > > > > and is therefore likely to be changed.
> > > > 
> > > > Where in the patch this was violated, and do note that fields are not
> > > > variables.
> > > 
> > > fields are variables.  And the struct names weren't all CamelCase.
> > 
> > c&v please. The only thing i can agree with is descriptor_t other than
> > that patch is just fine.
> 
> Upper case field names are not okay.  If you think coding style isn't clear,
> that's a bug in coding style.

Sez hu? Coding style is garbage that should be thrown out of the window.
As for looking, yeah, i'm looking at usb with it's lovely hungarian
fields, should we stampede to "fix" it?

If the one who's going to maintain the code is fine with whatever naming
is used so be it.

[..snip..]

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] Add support for r6040 NIC

[Anthony Liguori]

On 08/31/2011 09:35 AM, malc wrote:
> On Wed, 31 Aug 2011, Anthony Liguori wrote:
>
>> Upper case field names are not okay.  If you think coding style isn't clear,
>> that's a bug in coding style.
>
> Sez hu? Coding style is garbage that should be thrown out of the window.
> As for looking, yeah, i'm looking at usb with it's lovely hungarian
> fields, should we stampede to "fix" it?
>
> If the one who's going to maintain the code is fine with whatever naming
> is used so be it.

No.  That's how we got into the coding style mess we're in in the first 
place.

There's no benefit to going through and changing existing code but new 
code needs to be consistent with the vast majority of code in the rest 
of the tree.  It's about overall code base consistency and maintainability.

Regards,

Anthony Liguori

>
> [..snip..]
>

Re: [Qemu-devel] [PATCH] Add support for r6040 NIC

[malc]

On Wed, 31 Aug 2011, Anthony Liguori wrote:

> On 08/31/2011 09:35 AM, malc wrote:
> > On Wed, 31 Aug 2011, Anthony Liguori wrote:
> > 
> > > Upper case field names are not okay.  If you think coding style isn't
> > > clear,
> > > that's a bug in coding style.
> > 
> > Sez hu? Coding style is garbage that should be thrown out of the window.
> > As for looking, yeah, i'm looking at usb with it's lovely hungarian
> > fields, should we stampede to "fix" it?
> > 
> > If the one who's going to maintain the code is fine with whatever naming
> > is used so be it.
> 
> No.  That's how we got into the coding style mess we're in in the first 
> place.

boblycat.org/~malc/right.ogg

> 
> There's no benefit to going through and changing existing code but new code
> needs to be consistent with the vast majority of code in the rest of the tree.
> It's about overall code base consistency and maintainability.
> 

Hand waving, for instance vast majority of the code never used the
mandatory braces, the choice was arbitrary.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] Add support for r6040 NIC

[Edgar E. Iglesias]

On Wed, Aug 31, 2011 at 11:06:11AM -0500, Anthony Liguori wrote:
> On 08/31/2011 09:35 AM, malc wrote:
> >On Wed, 31 Aug 2011, Anthony Liguori wrote:
> >
> >>Upper case field names are not okay.  If you think coding style isn't clear,
> >>that's a bug in coding style.
> >
> >Sez hu? Coding style is garbage that should be thrown out of the window.
> >As for looking, yeah, i'm looking at usb with it's lovely hungarian
> >fields, should we stampede to "fix" it?
> >
> >If the one who's going to maintain the code is fine with whatever naming
> >is used so be it.
> 
> No.  That's how we got into the coding style mess we're in in the
> first place.

TBH, the codingstyle in QEMU is the least of "problems" we are facing.
We've got lack of documentation, lack of tests, lack of contributors,
etc, etc. IMO, those bring codingstyle issues into the pretty much
neglectable space.

I think we should throw out everything from CS beyond the details of
spaces and braces. Maybe keep the 80 char limit.

We should ofcourse refer to the C and other specs regarding correctness,
like the _t thing, but those are not really stylistic issues. Those are
bugs.

my 5 cents

Cheers

Re: [Qemu-devel] [PATCH] Add support for r6040 NIC

[Blue Swirl]

On Wed, Aug 31, 2011 at 4:06 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 08/31/2011 09:35 AM, malc wrote:
>>
>> On Wed, 31 Aug 2011, Anthony Liguori wrote:
>>
>>> Upper case field names are not okay.  If you think coding style isn't
>>> clear,
>>> that's a bug in coding style.
>>
>> Sez hu? Coding style is garbage that should be thrown out of the window.
>> As for looking, yeah, i'm looking at usb with it's lovely hungarian
>> fields, should we stampede to "fix" it?
>>
>> If the one who's going to maintain the code is fine with whatever naming
>> is used so be it.
>
> No.  That's how we got into the coding style mess we're in in the first
> place.
>
> There's no benefit to going through and changing existing code but new code
> needs to be consistent with the vast majority of code in the rest of the
> tree.  It's about overall code base consistency and maintainability.

I agree about importance of consistency, though I'd even go further
and reformat globally. New code gets introduced based on copying old
code so the pain goes on.

Re: [Qemu-devel] [PATCH] Add support for r6040 NIC

[Blue Swirl]

On Wed, Aug 31, 2011 at 4:24 PM, malc <av1474@comtv.ru> wrote:
> On Wed, 31 Aug 2011, Anthony Liguori wrote:
>
>> On 08/31/2011 09:35 AM, malc wrote:
>> > On Wed, 31 Aug 2011, Anthony Liguori wrote:
>> >
>> > > Upper case field names are not okay.  If you think coding style isn't
>> > > clear,
>> > > that's a bug in coding style.
>> >
>> > Sez hu? Coding style is garbage that should be thrown out of the window.
>> > As for looking, yeah, i'm looking at usb with it's lovely hungarian
>> > fields, should we stampede to "fix" it?
>> >
>> > If the one who's going to maintain the code is fine with whatever naming
>> > is used so be it.
>>
>> No.  That's how we got into the coding style mess we're in in the first
>> place.
>
> boblycat.org/~malc/right.ogg
>
>>
>> There's no benefit to going through and changing existing code but new code
>> needs to be consistent with the vast majority of code in the rest of the tree.
>> It's about overall code base consistency and maintainability.
>>
>
> Hand waving, for instance vast majority of the code never used the
> mandatory braces, the choice was arbitrary.

No, mandatory braces are better than the alternative. The choice has
been made and it has been mostly upheld.

Re: [Qemu-devel] [PATCH] Add support for r6040 NIC

[malc]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1573 bytes --]

On Wed, 31 Aug 2011, Blue Swirl wrote:

> On Wed, Aug 31, 2011 at 4:24 PM, malc <av1474@comtv.ru> wrote:
> > On Wed, 31 Aug 2011, Anthony Liguori wrote:
> >
> >> On 08/31/2011 09:35 AM, malc wrote:
> >> > On Wed, 31 Aug 2011, Anthony Liguori wrote:
> >> >
> >> > > Upper case field names are not okay.  If you think coding style isn't
> >> > > clear,
> >> > > that's a bug in coding style.
> >> >
> >> > Sez hu? Coding style is garbage that should be thrown out of the window.
> >> > As for looking, yeah, i'm looking at usb with it's lovely hungarian
> >> > fields, should we stampede to "fix" it?
> >> >
> >> > If the one who's going to maintain the code is fine with whatever naming
> >> > is used so be it.
> >>
> >> No.  That's how we got into the coding style mess we're in in the first
> >> place.
> >
> > boblycat.org/~malc/right.ogg
> >
> >>
> >> There's no benefit to going through and changing existing code but new code
> >> needs to be consistent with the vast majority of code in the rest of the tree.
> >> It's about overall code base consistency and maintainability.
> >>
> >
> > Hand waving, for instance vast majority of the code never used the
> > mandatory braces, the choice was arbitrary.
> 
> No, mandatory braces are better than the alternative. The choice has
> been made and it has been mostly upheld.
> 

I'm not arguing the merit of braces (though i doubt they has any), i'm 
saying that it was added without looking at the existing code, and fwiw
the only piece of code which consistently used braces was audio/* .

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH] Add support for r6040 NIC

[Blue Swirl]

On Wed, Aug 31, 2011 at 5:59 PM, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> On Wed, Aug 31, 2011 at 11:06:11AM -0500, Anthony Liguori wrote:
>> On 08/31/2011 09:35 AM, malc wrote:
>> >On Wed, 31 Aug 2011, Anthony Liguori wrote:
>> >
>> >>Upper case field names are not okay.  If you think coding style isn't clear,
>> >>that's a bug in coding style.
>> >
>> >Sez hu? Coding style is garbage that should be thrown out of the window.
>> >As for looking, yeah, i'm looking at usb with it's lovely hungarian
>> >fields, should we stampede to "fix" it?
>> >
>> >If the one who's going to maintain the code is fine with whatever naming
>> >is used so be it.
>>
>> No.  That's how we got into the coding style mess we're in in the
>> first place.
>
> TBH, the codingstyle in QEMU is the least of "problems" we are facing.
> We've got lack of documentation, lack of tests, lack of contributors,
> etc, etc. IMO, those bring codingstyle issues into the pretty much
> neglectable space.

The issues you mention are real, but problems in those areas do not
make the case for consistency invalid. We could also make rules for
documentation. For example, must use doxygen decorations for APIs,
must add a file under docs/apis/ when adding an API, must convert old
APIs when adding new ones etc.

Another way would be to downscale the project to match the resources,
we should throw away code which is not maintained and then focus on
the good parts.

> I think we should throw out everything from CS beyond the details of
> spaces and braces. Maybe keep the 80 char limit.

I disagree, those rules try to make the code consistent. I don't care
much what is the standard (could be Linux kernel style for example),
but maintaining the style improves readability and maintainability of
the code.

> We should ofcourse refer to the C and other specs regarding correctness,
> like the _t thing, but those are not really stylistic issues. Those are
> bugs.
>
> my 5 cents
>
> Cheers
>
>

Re: [Qemu-devel] [PATCH] Add support for r6040 NIC

[Anthony Liguori]

On 08/31/2011 12:59 PM, Edgar E. Iglesias wrote:
> On Wed, Aug 31, 2011 at 11:06:11AM -0500, Anthony Liguori wrote:
>> On 08/31/2011 09:35 AM, malc wrote:
>>> On Wed, 31 Aug 2011, Anthony Liguori wrote:
>>>
>>>> Upper case field names are not okay.  If you think coding style isn't clear,
>>>> that's a bug in coding style.
>>>
>>> Sez hu? Coding style is garbage that should be thrown out of the window.
>>> As for looking, yeah, i'm looking at usb with it's lovely hungarian
>>> fields, should we stampede to "fix" it?
>>>
>>> If the one who's going to maintain the code is fine with whatever naming
>>> is used so be it.
>>
>> No.  That's how we got into the coding style mess we're in in the
>> first place.
>
> TBH, the codingstyle in QEMU is the least of "problems" we are facing.
> We've got lack of documentation, lack of tests, lack of contributors,
> etc, etc. IMO, those bring codingstyle issues into the pretty much
> neglectable space.

I don't think we lack contributors.  Documentation and tests are really 
about discipline.  If we can't even be bothered to maintain consistency 
in variable naming, do you really expected that we can be disciplined in 
writing documentation and tests?

Is the next argument going to be that every subsystem should be able to 
have its documentation in it's preferred natural language such that the 
documentation for the block layer is in Esperanto?

Consistent coding style makes the tree a single code base, instead of a 
bunch of independent islands.  This encourages sharing code and ideas 
across subsystems.  Too often, we reproduce the same thing and over 
again in different subsystems (and even different machine architectures).

Regards,

Anthony Liguori

>
> I think we should throw out everything from CS beyond the details of
> spaces and braces. Maybe keep the 80 char limit.
>
> We should ofcourse refer to the C and other specs regarding correctness,
> like the _t thing, but those are not really stylistic issues. Those are
> bugs.
>
> my 5 cents
>
> Cheers
>

Re: [Qemu-devel] [PATCH] Add support for r6040 NIC

[Edgar E. Iglesias]

On Wed, Aug 31, 2011 at 06:46:37PM +0000, Blue Swirl wrote:
> On Wed, Aug 31, 2011 at 5:59 PM, Edgar E. Iglesias
> <edgar.iglesias@gmail.com> wrote:
> > On Wed, Aug 31, 2011 at 11:06:11AM -0500, Anthony Liguori wrote:
> >> On 08/31/2011 09:35 AM, malc wrote:
> >> >On Wed, 31 Aug 2011, Anthony Liguori wrote:
> >> >
> >> >>Upper case field names are not okay.  If you think coding style isn't clear,
> >> >>that's a bug in coding style.
> >> >
> >> >Sez hu? Coding style is garbage that should be thrown out of the window.
> >> >As for looking, yeah, i'm looking at usb with it's lovely hungarian
> >> >fields, should we stampede to "fix" it?
> >> >
> >> >If the one who's going to maintain the code is fine with whatever naming
> >> >is used so be it.
> >>
> >> No.  That's how we got into the coding style mess we're in in the
> >> first place.
> >
> > TBH, the codingstyle in QEMU is the least of "problems" we are facing.
> > We've got lack of documentation, lack of tests, lack of contributors,
> > etc, etc. IMO, those bring codingstyle issues into the pretty much
> > neglectable space.
> 
> The issues you mention are real, but problems in those areas do not
> make the case for consistency invalid. We could also make rules for
> documentation. For example, must use doxygen decorations for APIs,
> must add a file under docs/apis/ when adding an API, must convert old
> APIs when adding new ones etc.
> 
> Another way would be to downscale the project to match the resources,
> we should throw away code which is not maintained and then focus on
> the good parts.
>
> > I think we should throw out everything from CS beyond the details of
> > spaces and braces. Maybe keep the 80 char limit.
> 
> I disagree, those rules try to make the code consistent. I don't care
> much what is the standard (could be Linux kernel style for example),
> but maintaining the style improves readability and maintainability of
> the code.

I agree with you to some extent, but consistent code is relative. I dont
like seeing patches rejected based on details that IMO don't matter.
Of course, what doesn't matter to me, may matter to others. So I accept it
and go on... :)

I guess that my point is that the improvments in readabilty and
maintainnability we get for example on dictating camel case here and there
but not on other places is so minimal that the focus we loose and time
we spend on looking for those, makes it not worth it.

At the moment I'm happy with just seeing that CODING_STYLE doesn't grow
though.

Cheers

Re: [Qemu-devel] [PATCH] Add support for r6040 NIC

[Edgar E. Iglesias]

On Wed, Aug 31, 2011 at 01:48:34PM -0500, Anthony Liguori wrote:
> On 08/31/2011 12:59 PM, Edgar E. Iglesias wrote:
> >On Wed, Aug 31, 2011 at 11:06:11AM -0500, Anthony Liguori wrote:
> >>On 08/31/2011 09:35 AM, malc wrote:
> >>>On Wed, 31 Aug 2011, Anthony Liguori wrote:
> >>>
> >>>>Upper case field names are not okay.  If you think coding style isn't clear,
> >>>>that's a bug in coding style.
> >>>
> >>>Sez hu? Coding style is garbage that should be thrown out of the window.
> >>>As for looking, yeah, i'm looking at usb with it's lovely hungarian
> >>>fields, should we stampede to "fix" it?
> >>>
> >>>If the one who's going to maintain the code is fine with whatever naming
> >>>is used so be it.
> >>
> >>No.  That's how we got into the coding style mess we're in in the
> >>first place.
> >
> >TBH, the codingstyle in QEMU is the least of "problems" we are facing.
> >We've got lack of documentation, lack of tests, lack of contributors,
> >etc, etc. IMO, those bring codingstyle issues into the pretty much
> >neglectable space.
> 
> I don't think we lack contributors.  Documentation and tests are
> really about discipline.  If we can't even be bothered to maintain
> consistency in variable naming, do you really expected that we can
> be disciplined in writing documentation and tests?

Yes I do. It's not white and black, it's not about making the code
completely inconsistent or 100 consistent. It's about find a level
of consistency that is acceptable and doesn't cost too much to
maintain.


> Is the next argument going to be that every subsystem should be able
> to have its documentation in it's preferred natural language such
> that the documentation for the block layer is in Esperanto?

Heh, I think you missunderstood me. I'm talking about details that
dont matter. Like, would you like to enforce American english or UK
english? More that kind of question.

> Consistent coding style makes the tree a single code base, instead
> of a bunch of independent islands.  This encourages sharing code and
> ideas across subsystems.  Too often, we reproduce the same thing and
> over again in different subsystems (and even different machine
> architectures).

I dont think the latter has to do with codingstyle consistensy. But
I agree with the former.

Cheers

Re: [Qemu-devel] [PATCH] Add support for r6040 NIC

[Edgar E. Iglesias]

On Wed, Aug 31, 2011 at 09:12:06PM +0200, Edgar E. Iglesias wrote:
> On Wed, Aug 31, 2011 at 01:48:34PM -0500, Anthony Liguori wrote:
> > On 08/31/2011 12:59 PM, Edgar E. Iglesias wrote:
> > >On Wed, Aug 31, 2011 at 11:06:11AM -0500, Anthony Liguori wrote:
> > >>On 08/31/2011 09:35 AM, malc wrote:
> > >>>On Wed, 31 Aug 2011, Anthony Liguori wrote:
> > >>>
> > >>>>Upper case field names are not okay.  If you think coding style isn't clear,
> > >>>>that's a bug in coding style.
> > >>>
> > >>>Sez hu? Coding style is garbage that should be thrown out of the window.
> > >>>As for looking, yeah, i'm looking at usb with it's lovely hungarian
> > >>>fields, should we stampede to "fix" it?
> > >>>
> > >>>If the one who's going to maintain the code is fine with whatever naming
> > >>>is used so be it.
> > >>
> > >>No.  That's how we got into the coding style mess we're in in the
> > >>first place.
> > >
> > >TBH, the codingstyle in QEMU is the least of "problems" we are facing.
> > >We've got lack of documentation, lack of tests, lack of contributors,
> > >etc, etc. IMO, those bring codingstyle issues into the pretty much
> > >neglectable space.
> > 
> > I don't think we lack contributors.  Documentation and tests are
> > really about discipline.  If we can't even be bothered to maintain
> > consistency in variable naming, do you really expected that we can
> > be disciplined in writing documentation and tests?
> 
> Yes I do. It's not white and black, it's not about making the code
> completely inconsistent or 100 consistent. It's about find a level
> of consistency that is acceptable and doesn't cost too much to
> maintain.

Now if that's my opinion, then the 99999 dollar question is:
why am I wasting so much time on discusing it?

And btw Bifferos, don't worry. Not all patches you contribute will cause
this much controversy :)

Cheers

Re: [Qemu-devel] [PATCH] Add support for r6040 NIC

[Anthony Liguori]

On 08/31/2011 02:12 PM, Edgar E. Iglesias wrote:
> On Wed, Aug 31, 2011 at 01:48:34PM -0500, Anthony Liguori wrote:
>>> etc, etc. IMO, those bring codingstyle issues into the pretty much
>>> neglectable space.
>>
>> I don't think we lack contributors.  Documentation and tests are
>> really about discipline.  If we can't even be bothered to maintain
>> consistency in variable naming, do you really expected that we can
>> be disciplined in writing documentation and tests?
>
> Yes I do. It's not white and black, it's not about making the code
> completely inconsistent or 100 consistent. It's about find a level
> of consistency that is acceptable and doesn't cost too much to
> maintain.

I actually agree.  I don't like the idea of absolutely enforcing a 
coding style that demands no white space at the end of a line (if you 
can't see it, why in the world would you care?).

But coding style deviations that make the code look foreign, like using 
CamelCase for field names, seems important to me.

And I respect that other things seem important to other people (even 
invisible things like trailing white space).  So even though I wouldn't 
want to reject a patch because of coding style in some cases, I think 
it's important that we do our best to enforce it.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] Add support for r6040 NIC

[Blue Swirl]

On Wed, Aug 31, 2011 at 7:23 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 08/31/2011 02:12 PM, Edgar E. Iglesias wrote:
>>
>> On Wed, Aug 31, 2011 at 01:48:34PM -0500, Anthony Liguori wrote:
>>>>
>>>> etc, etc. IMO, those bring codingstyle issues into the pretty much
>>>> neglectable space.
>>>
>>> I don't think we lack contributors.  Documentation and tests are
>>> really about discipline.  If we can't even be bothered to maintain
>>> consistency in variable naming, do you really expected that we can
>>> be disciplined in writing documentation and tests?
>>
>> Yes I do. It's not white and black, it's not about making the code
>> completely inconsistent or 100 consistent. It's about find a level
>> of consistency that is acceptable and doesn't cost too much to
>> maintain.
>
> I actually agree.  I don't like the idea of absolutely enforcing a coding
> style that demands no white space at the end of a line (if you can't see it,
> why in the world would you care?).

Because then the patches for the line could be mangled by e-mail
transport. Actually git apply.whitespace=fix handles this nicely.

> But coding style deviations that make the code look foreign, like using
> CamelCase for field names, seems important to me.
>
> And I respect that other things seem important to other people (even
> invisible things like trailing white space).  So even though I wouldn't want
> to reject a patch because of coding style in some cases, I think it's
> important that we do our best to enforce it.

If the rules are enforced in some case but not in others, this is not
fair on submitters.

[Qemu-devel] [PATCH v2] Add support for r6040 NIC

[bifferos]

Here it is again with copyright, typo fix and hopefully now complying with coding standards.


Signed-off-by: Mark Kelly <mark@bifferos.com>
diff --git a/Makefile.objs b/Makefile.objs
index 6991a9f..7d87503 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -240,6 +240,7 @@ hw-obj-$(CONFIG_PCNET_PCI) += pcnet-pci.o
 hw-obj-$(CONFIG_PCNET_COMMON) += pcnet.o
 hw-obj-$(CONFIG_E1000_PCI) += e1000.o
 hw-obj-$(CONFIG_RTL8139_PCI) += rtl8139.o
+hw-obj-$(CONFIG_R6040_PCI) += r6040.o
 
 hw-obj-$(CONFIG_SMC91C111) += smc91c111.o
 hw-obj-$(CONFIG_LAN9118) += lan9118.o
diff --git a/default-configs/pci.mak b/default-configs/pci.mak
index 22bd350..d2ea7a2 100644
--- a/default-configs/pci.mak
+++ b/default-configs/pci.mak
@@ -10,6 +10,7 @@ CONFIG_PCNET_PCI=y
 CONFIG_PCNET_COMMON=y
 CONFIG_LSI_SCSI_PCI=y
 CONFIG_RTL8139_PCI=y
+CONFIG_R6040_PCI=y
 CONFIG_E1000_PCI=y
 CONFIG_IDE_CORE=y
 CONFIG_IDE_QDEV=y
diff --git a/hw/pci.c b/hw/pci.c
index b904a4e..7e12935 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1527,6 +1527,7 @@ static const char * const pci_nic_models[] = {
     "rtl8139",
     "e1000",
     "pcnet",
+    "r6040",
     "virtio",
     NULL
 };
@@ -1539,6 +1540,7 @@ static const char * const pci_nic_names[] = {
     "rtl8139",
     "e1000",
     "pcnet",
+    "r6040",
     "virtio-net-pci",
     NULL
 };
diff --git a/hw/r6040.c b/hw/r6040.c
new file mode 100644
index 0000000..2908459
--- /dev/null
+++ b/hw/r6040.c
@@ -0,0 +1,644 @@
+/*
+ * Emulation of r6040 ethernet controller found in a number of SoCs.
+ * Copyright (c) 2011 Mark Kelly, mark@bifferos.com
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included
+ * in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+ * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+ * NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+ * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+ * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR
+ * THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * This has been written using the R8610[1] and ip101a[2] datasheets.
+ *
+ * ICs with the embedded controller include R8610, R3210, AMRISC20000
+ * and Vortex86SX
+ *
+ * The emulation seems good enough to fool Linux 2.6.37.6.  It is
+ * not perfect, but has proven useful.
+ *
+ * [1] http://www.sima.com.tw/download/R8610_D06_20051003.pdf
+ * [2] http://www.icplus.com.tw/pp-IP101A.html
+ */
+
+#include "hw.h"
+#include "pci.h"
+#include "net.h"
+#include "loader.h"
+#include "sysemu.h"
+#include "qemu-timer.h"
+
+/* #define DEBUG_R6040 1 */
+
+
+#if defined DEBUG_R6040
+#define DPRINTF(fmt, ...) \
+    do { fprintf(stderr, "R6040: " fmt, ## __VA_ARGS__); } while (0)
+#else
+static inline GCC_FMT_ATTR(1, 2) int DPRINTF(const char *fmt, ...)
+{
+    return 0;
+}
+#endif
+
+
+/* Cast in order of appearance.  _W suffix means it's used to index the
+   register word array (regs_w)
+ */
+
+#define MPSCCR_W         (0x88 / 2)
+
+#define MAC0_W           (0x68 / 2)
+#define MAC1_W           (0x6a / 2)
+#define MAC2_W           (0x6c / 2)
+
+
+#define RX_START_LOW_W   (0x34 / 2)
+#define RX_START_HIGH_W  (0x38 / 2)
+#define TX_PKT_COUNT_W   (0x5a / 2)
+#define RX_PKT_COUNT_W   (0x50 / 2)
+
+
+#define MCR0_W           (0x00 / 2)
+#define MCR1_W           (0x04 / 2)
+#define BIT_MRST         (1 << 0)
+
+#define MTPR_W           (0x14 / 2)
+#define MRBSR_W          (0x18 / 2)
+#define MISR_W           (0x3c / 2)
+#define MIER_W           (0x40 / 2)
+
+#define MMDIO_W          (0x20 / 2)
+#define MDIO_READ_W      (0x24 / 2)
+#define MDIO_WRITE_W     (0x28 / 2)
+
+#define MRCNT_W          (0x50 / 2)
+#define MTCNT_W          (0x5c / 2)
+
+
+#define MDIO_WRITE      0x4000
+#define MDIO_READ       0x2000
+
+
+typedef struct R6040State {
+    PCIDevice dev;
+    NICState *nic;
+    NICConf conf;
+
+    /* PHY related register sets */
+    uint16_t mid0[3];
+    uint16_t phy_regs[32];
+    uint32_t phy_op_in_progress;
+
+    /* Primary IO address space */
+    union {
+        uint8_t regs_b[0x100];   /* Byte access */
+        uint16_t regs_w[0x100/2];  /* word access */
+        uint32_t regs_l[0x100/4];  /* DWORD access */
+    };
+
+} R6040State;
+
+
+/* some inlines to help access above structure */
+static inline uint32_t TX_START(R6040State *s)
+{
+    uint32_t tmp = s->regs_w[0x2c/2];
+    return tmp | (s->regs_w[0x30/2] << 16);
+}
+
+static inline void TX_START_SET(R6040State *s, uint32_t start)
+{
+    s->regs_w[0x2c/2] = start & 0xffff;
+    s->regs_w[0x30/2] = (start >> 16) & 0xffff;
+}
+
+static inline uint32_t RX_START(R6040State *s)
+{
+    uint32_t tmp = s->regs_w[0x34/2];
+    return tmp | (s->regs_w[0x38/2] << 16);
+}
+
+static inline void RX_START_SET(R6040State *s, uint32_t start)
+{
+    s->regs_w[0x34/2] = start & 0xffff;
+    s->regs_w[0x38/2] = (start >> 16) & 0xffff;
+}
+
+
+static void r6040_update_irq(R6040State *s)
+{
+    uint16_t isr = s->regs_w[MISR_W] & s->regs_w[MIER_W];
+
+    qemu_set_irq(s->dev.irq[0], isr ? 1 : 0);
+}
+
+
+/* Mark auto-neg complete, NIC up.  */
+static void PhysicalLinkUp(void *opaque)
+{
+    R6040State *s = opaque;
+    s->phy_regs[1] |= (1 << 2);
+}
+
+
+/* Transmit and receive descriptors are doubled up
+   One is a subset of the other anyhow
+ */
+typedef struct Descriptor {
+    uint16_t dst;
+    uint16_t dlen;
+    uint32_t dbp;
+    uint32_t dnx;
+    uint16_t hidx;
+    uint16_t reserved_1;
+    uint16_t reserved_2;
+} Descriptor;
+
+
+/* Some debugging functions */
+
+#ifdef DEBUG_R6040
+static void addr_dump16(const char *name, uint16_t val)
+{
+    DPRINTF("%s: 0x%04x  ", name, val);
+}
+
+static void addr_dump32(const char *name, uint32_t val)
+{
+    DPRINTF("%s: 0x%x  ", name, val);
+}
+
+static void hex_dump(const uint8_t *data, uint32_t len)
+{
+    uint8_t i;
+    DPRINTF("hex: ");
+    for (i = 0; i < len; i++) {
+        fprintf(stderr, "%02x ", *data);
+        if (i && !(i % 0x20)) {
+            fprintf(stderr, "\n");
+        }
+        data++;
+    }
+    fprintf(stderr, "\n");
+}
+
+static void desc_dump(Descriptor *d, uint32_t addr)
+{
+    DPRINTF("\nDumping: 0x%x\n", addr);
+    addr_dump16("DST", d->dst);
+    addr_dump16("DLEN", d->dlen);
+    addr_dump32("DBP", (unsigned long)d->dbp);
+    addr_dump32("DNX", (unsigned long)d->dnx);
+    addr_dump16("HIDX", d->hidx);
+    printf("\n");
+}
+
+static void dump_phys_mem(uint32_t addr, int len)
+{
+    uint8_t buffer[1024];
+    cpu_physical_memory_read(addr, buffer, len);
+    hex_dump(buffer, len);
+}
+
+static void dump_pci(uint8_t *pci_conf)
+{
+    uint32_t *p = (uint32_t *)pci_conf;
+    int i = 0;
+    for (i = 0; i < 0x40; i += 4) {
+        DPRINTF("Addr: 0x%08x,  Data: 0x%08x\n", i, *p);
+        p++;
+    }
+}
+#endif
+
+
+static const VMStateDescription vmstate_r6040 = {
+    .name = "r6040",
+    .version_id = 3,
+    .minimum_version_id = 2,
+    .minimum_version_id_old = 2,
+    .fields = (VMStateField[]) {
+        VMSTATE_PCI_DEVICE(dev, R6040State),
+        VMSTATE_BUFFER(regs_b, R6040State),
+        VMSTATE_UINT16_ARRAY(mid0, R6040State, 3),
+        VMSTATE_UINT16_ARRAY(phy_regs, R6040State, 32),
+        VMSTATE_UINT32(phy_op_in_progress, R6040State),
+        VMSTATE_MACADDR(conf.macaddr, R6040State),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+
+static int TryToSendOnePacket(void *opaque)
+{
+    R6040State *s = opaque;
+    Descriptor d;
+    uint8_t pkt_buffer[2000];
+    uint32_t tocopy;
+
+    cpu_physical_memory_read(TX_START(s), (uint8_t *)&d, sizeof(d));
+
+    if (d.dst & 0x8000) {    /* MAC owns it? */
+        tocopy = d.dlen;
+        if (tocopy > sizeof(pkt_buffer)) {
+            tocopy = sizeof(pkt_buffer);
+        }
+        /* copy the packet to send it */
+        cpu_physical_memory_read(d.dbp, pkt_buffer, tocopy);
+
+        qemu_send_packet(&s->nic->nc, pkt_buffer, tocopy);
+        s->regs_w[TX_PKT_COUNT_W]++;
+
+        /* relinquish ownership, we're done with it */
+        d.dst &= ~0x8000;
+
+        /* Copy the new version of the descriptor back */
+        cpu_physical_memory_write(TX_START(s), (uint8_t *)&d, sizeof(d));
+
+        /* Advance to the next buffer if packet processed */
+        TX_START_SET(s, d.dnx);
+
+        return 1;
+    }
+
+    return 0;
+}
+
+
+static void r6040_transmit(void *opaque)
+{
+    R6040State *s = opaque;
+    int count = 0;
+
+    while (TryToSendOnePacket(s)) {
+        ++count;
+    }
+
+    if (count) {
+        s->regs_w[MISR_W] |= 0x10;
+        r6040_update_irq(s);
+    }
+}
+
+
+/* Whether to allow callback returning 1 for yes, can receive */
+static int r6040_can_receive(VLANClientState *nc)
+{
+    R6040State *s = DO_UPCAST(NICState, nc, nc)->opaque;
+    int tmp = (s->regs_w[0] & (1 << 1)) ? 1 : 0;
+    return tmp;
+}
+
+
+static int ReceiveOnePacket(void *opaque, const uint8_t *buf, size_t len)
+{
+    R6040State *s = opaque;
+    uint32_t tocopy = len+4;  /* include checksum */
+    Descriptor d;
+
+    cpu_physical_memory_read(RX_START(s), (uint8_t *)&d, sizeof(Descriptor));
+    /*desc_dump(&d, 0);*/
+
+    if (d.dst & 0x8000) {    /* MAC owned? */
+
+        uint16_t max_buffer = s->regs_w[MRBSR_W] & 0x07fc;
+        if (tocopy > max_buffer) {
+            tocopy = max_buffer;
+        }
+
+        cpu_physical_memory_write(d.dbp, buf, tocopy-4);
+
+        /* indicate received OK */
+        d.dst |= (1 << 14);
+        d.dlen = tocopy;
+        /* relinquish ownership */
+        d.dst &= ~0x8000;
+
+        /* Copy the descriptor back */
+        cpu_physical_memory_write(RX_START(s), (uint8_t *)&d,
+                                   sizeof(Descriptor));
+
+        s->regs_w[RX_PKT_COUNT_W]++;
+
+        s->regs_w[MISR_W] |= 1;  /* received pkt interrupt */
+
+        r6040_update_irq(s);
+
+        RX_START_SET(s, d.dnx);  /* advance */
+
+        return 0;
+    }
+    return -1;
+}
+
+
+/* called on incoming packets */
+static ssize_t r6040_receive(VLANClientState *nc, const uint8_t *buf,
+                                        size_t len)
+{
+    R6040State *s = DO_UPCAST(NICState, nc, nc)->opaque;
+    DPRINTF("Received incoming packet of len %ld\n", len);
+
+    if (0 == ReceiveOnePacket(s, buf, len)) {
+        return len;  /* copied OK */
+    }
+
+    return 0;
+}
+
+
+static void r6040_cleanup(VLANClientState *vc)
+{
+    DPRINTF("r6040_cleanup\n");
+}
+
+
+static inline int BIT_SET(uint16_t old, uint16_t new, uint16_t bit)
+{
+    uint16_t before = (old & (1 << bit));
+    uint16_t after = (new & (1 << bit));
+    if (!before && after) {
+        return 1;
+    }
+    return 0;
+}
+
+
+static void r6040_ioport_writew(void *opaque, uint32_t addr, uint32_t val)
+{
+    R6040State *s = opaque;
+    uint16_t old;
+    addr &= 0xff;   /* get relative to base address */
+    addr /= 2;    /* Get the offset into the word-array */
+    old = s->regs_w[addr];   /* store the old value for future use */
+
+    switch (addr) {
+    case MCR0_W:   /* 0x00 */
+        if (BIT_SET(old, val, 12)) {
+            r6040_transmit(opaque);
+        }
+        break;
+    case MCR1_W:  /* 0x04 */
+        if (val & BIT_MRST) {   /* reset request incoming */
+            /* reset requested, complete it immediately, set this value to
+               default */
+            val = 0x0010;
+        }
+        break;
+    case MTPR_W:  /* TX command reg, 0x14 */
+        if (val & 1) {
+            r6040_transmit(opaque);
+            val &= ~1;
+        }
+        break;
+    case MMDIO_W:  /* MDIO control, 0x20 */
+        {
+            int phy_exists = ((val & 0x1f00) == 0x100) ? 1 : 0;
+            uint16_t *phy = s->phy_regs;
+            phy += (val & 0x1f);
+
+            if  (val & (1 << 13)) {   /* read data */
+                if (phy_exists) {
+                    s->regs_w[MDIO_READ_W] = *phy;
+                } else {
+                    s->regs_w[MDIO_READ_W] = 0xffff;
+                }
+            } else if (val & (1 << 14)) {  /* write data */
+                if (phy_exists) {
+                    *phy = s->regs_w[MDIO_WRITE_W];
+                }
+            }
+
+            /* Whether you request to read or write, both bits go high while
+               the operation is in progress, e.g. tell it to read, and the
+               write-in-progress flag also goes high */
+            val |= 0x6000;  /* signal operation has started */
+            s->phy_op_in_progress = 1;
+
+            break;
+        }
+    case MISR_W:  /* interrupt status reg (read to clear), 0x3c */
+        return;
+
+    case MIER_W:  /* interrupt enable register, 0x40 */
+        s->regs_w[MIER_W] = val;
+        r6040_update_irq(s);
+        return;
+
+    case MRCNT_W:   /* 0x50 */
+    case MTCNT_W:   /* 0x5c */
+        return;  /* Can't write to pkt count registers, skip */
+
+    }
+    s->regs_w[addr] = val & 0xFFFF;
+}
+
+
+static uint32_t r6040_ioport_readw(void *opaque, uint32_t addr)
+{
+    R6040State *s = opaque;
+    addr &= 0xff;   /* get relative to base address */
+    addr /= 2;    /* Get the offset into the word-array */
+    uint32_t tmp = s->regs_w[addr];  /* get the value */
+
+    switch (addr) {
+
+    case MMDIO_W:  /* MDIO control, 0x20 */
+        {
+            /* Clear any in-progress MDIO activity for the next read
+               This simulates the polling of the MDIO operation status,
+               so the driver code always has to read the register twice
+               before it thinks the operation is complete. */
+            if (s->phy_op_in_progress) {
+                s->regs_w[addr] &= ~0x6000;
+                s->phy_op_in_progress = 0;
+            }
+            break;
+        }
+    case MISR_W:  /* interrupt status reg (read to clear)  0x3c */
+        s->regs_w[addr] = 0;
+        break;
+    case MIER_W:  /* interrupt enable reg 0x40 */
+        break;
+    case MRCNT_W:  /* 0x50 */
+    case MTCNT_W:  /* 0x5c */
+        s->regs_w[addr] = 0;   /* read to clear */
+        break;
+    default:
+        break;
+    }
+    return tmp;
+}
+
+
+/* byte and long access are routed via the word operation handlers */
+static void r6040_ioport_writeb(void *opaque, uint32_t addr, uint32_t val)
+{
+    R6040State *s = opaque;
+    addr &= 0xFF;
+    val &= 0xFF;
+    uint16_t old = s->regs_w[addr/2];  /* get the current value */
+    if (addr & 1) {
+        old &= 0xff;
+        old |= (val << 8);
+    } else {
+        old &= 0xff00;
+        old |= val;
+    }
+
+    r6040_ioport_writew(opaque, addr, old);  /* call the word-based version */
+}
+
+static void r6040_ioport_writel(void *opaque, uint32_t addr, uint32_t val)
+{
+    /* Set the low value */
+    r6040_ioport_writew(opaque, addr, val & 0xffff);
+    /* Set the high value */
+    r6040_ioport_writew(opaque, addr+2, (val >> 16) & 0xffff);
+}
+
+static uint32_t r6040_ioport_readb(void *opaque, uint32_t addr)
+{
+    uint32_t tmp = r6040_ioport_readw(opaque, addr & ~1);
+    if (addr & 1) {
+        return (tmp & 0xff00) >> 8;
+    }
+    return tmp & 0xff;
+}
+
+static uint32_t r6040_ioport_readl(void *opaque, uint32_t addr)
+{
+    uint32_t tmp = r6040_ioport_readw(opaque, addr);
+    return tmp | (r6040_ioport_readw(opaque, addr+2) << 16);
+}
+
+
+static void r6040_register_ioports(R6040State *s, pcibus_t addr)
+{
+    register_ioport_write(addr, 0x100, 1, r6040_ioport_writeb, s);
+    register_ioport_read(addr, 0x100, 1, r6040_ioport_readb,  s);
+
+    register_ioport_write(addr, 0x100, 2, r6040_ioport_writew, s);
+    register_ioport_read(addr, 0x100, 2, r6040_ioport_readw,  s);
+
+    register_ioport_write(addr, 0x100, 4, r6040_ioport_writel, s);
+    register_ioport_read(addr, 0x100, 4, r6040_ioport_readl,  s);
+}
+
+
+static void r6040_map(PCIDevice *pci_dev, int region_num,
+                       pcibus_t addr, pcibus_t size, int type)
+{
+    R6040State *s = DO_UPCAST(R6040State, dev, pci_dev);;
+
+    DPRINTF("## Mapping to address %lx\n", addr);
+    r6040_register_ioports(s, addr);
+}
+
+
+static NetClientInfo net_r6040_info = {
+    .type = NET_CLIENT_TYPE_NIC,
+    .size = sizeof(NICState),
+    .can_receive = r6040_can_receive,
+    .receive = r6040_receive,
+    .cleanup = r6040_cleanup,
+};
+
+
+static int r6040_init_pci(PCIDevice *dev)
+{
+    QEMUTimer *timer;
+
+    R6040State *s = DO_UPCAST(R6040State, dev, dev);
+    uint8_t *pci_conf;
+
+    /* MAC PHYS status change register.  Linux driver expects something
+       sensible as default and if not will try to set it */
+    s->regs_w[MPSCCR_W] = 0x9f07;
+
+    /* Default value for maximum packet size */
+    s->regs_w[MRBSR_W] = 0x600;
+
+    /* set the MAC, linux driver reads this when it loads, it is
+       normally set by the BIOS, but obviously Qemu BIOS isn't going
+       to do that */
+    s->regs_w[MAC0_W] = 0x5452;
+    s->regs_w[MAC1_W] = 0x1200;
+    s->regs_w[MAC2_W] = 0x5734;
+
+    /* Tell Qemu the same thing */
+    s->conf.macaddr.a[0] = s->regs_w[MAC0_W] & 0xff;
+    s->conf.macaddr.a[1] = (s->regs_w[MAC0_W] >> 8) & 0xff;
+    s->conf.macaddr.a[2] = s->regs_w[MAC1_W] & 0xff;
+    s->conf.macaddr.a[3] = (s->regs_w[MAC1_W] >> 8) & 0xff;
+    s->conf.macaddr.a[4] = s->regs_w[MAC2_W] & 0xff;
+    s->conf.macaddr.a[5] = (s->regs_w[MAC2_W] >> 8) & 0xff;
+
+    /* no commands running */
+    s->phy_op_in_progress = 0;
+
+    /* PHY auto-neg in progress */
+    s->phy_regs[1] = 0x786d & ~(1 << 2);
+    s->phy_regs[2] = 0x0243;
+    s->phy_regs[3] = 0x0c54;
+
+    pci_conf = (uint8_t *)s->dev.config;
+
+    pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; /* header_type */
+    pci_conf[PCI_INTERRUPT_LINE] = 0xa;     /* interrupt line  */
+    pci_conf[PCI_INTERRUPT_PIN] = 1;      /* interrupt pin 0 */
+
+    pci_register_bar(&s->dev, 0, 0x100, PCI_BASE_ADDRESS_SPACE_IO, r6040_map);
+
+    s->nic = qemu_new_nic(&net_r6040_info, &s->conf,
+                            dev->qdev.info->name, dev->qdev.id, s);
+
+    qemu_format_nic_info_str(&s->nic->nc, s->conf.macaddr.a);
+
+    /* Simulate a delay of a couple of seconds for the link to come up.
+       Not required for Linux but very handy for developing BIOS code.
+     */
+    timer = qemu_new_timer_ns(vm_clock, PhysicalLinkUp, s);
+    qemu_mod_timer(timer, qemu_get_clock_ms(vm_clock) + 1500000000);
+
+    /* Register IO port access as well */
+    r6040_register_ioports(s, 0xe800);
+
+    return 0;
+}
+
+
+static PCIDeviceInfo r6040_info = {
+    .qdev.name = "r6040",
+    .qdev.size = sizeof(R6040State),
+    .qdev.vmsd  = &vmstate_r6040,
+    .init      = r6040_init_pci,
+    .vendor_id = 0x17f3,  /* RDC */
+    .device_id = 0x6040,   /* r6040 nic */
+    .class_id   = PCI_CLASS_NETWORK_ETHERNET,
+    .qdev.props = (Property[]) {
+        DEFINE_NIC_PROPERTIES(R6040State, conf),
+        DEFINE_PROP_END_OF_LIST(),
+    }
+};
+
+
+static void r6040_register(void)
+{
+    pci_qdev_register(&r6040_info);
+}
+
+
+device_init(r6040_register)

Re: [Qemu-devel] [PATCH] Add support for r6040 NIC

[bifferos]

--- On Wed, 31/8/11, Blue Swirl <blauwirbel@gmail.com> wrote:

> I agree about importance of consistency, though I'd even go
> further and reformat globally. New code gets introduced 
> based on copying old code so the pain goes on.

I think this is the most important point.

I've worked on many projects where coding standards were applied, generally the order is:

1) Someone comes up with some new standards
2) They want to dictate the standards but are not prepared to invest any
   effort in making the existing code base conform to those standards
3) They decide it's too high risk to modify existing code
4) In conclusion they apply the coding standards to new code only
5) Some... time... passes
6) Evangelist(s) responsible for coding standards leave the team
   *or*
   Are convinced by other developers that coding standards suck
7) Repeat from 1), or drop coding standards

I've seen this pattern pretty much without fail, over the last 20 years of 
commercial software development.  You never reach the standards utopia 
unless you retrospectively change all existing code.  The only way you
ever want to do that is if you have either

a) Extensive unit tests
or 
b) Balls

Preferably both!

So what generally happens in experienced teams, is that you either 
re-work (only) the modules you touch, or you identify the coding 
standard of the module you're working on or the ones nearby and follow them.

Alternatively some developers write coding standards in a rather vague way
such that there are no hard and fast rules.  They categorise types of
problem and their severity rather than using all rules as rejection
criteria.

Couple of examples come to mind:

When developing either driver or device emulation software it's nice
to use the same identifiers as the data sheet.  By forcing everything to lower case it makes it harder to cross-reference things, and God forbid if the data sheet actually used case to distinguish registers you'd then have to invent identifiers to deal with it.

I looked around to see which license I could use, cribbed one from another Qemu file, and then promptly had to reformat it because it broke the line length rule, so I've reformated what was previously a standard license and crafted my own for no other reason that conforming with checkpatch.pl.

cheers,
Biff.

Re: [Qemu-devel] [PATCH] Add support for r6040 NIC

[Markus Armbruster]

Blue Swirl <blauwirbel@gmail.com> writes:

> On Wed, Aug 31, 2011 at 4:06 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
>> On 08/31/2011 09:35 AM, malc wrote:
>>>
>>> On Wed, 31 Aug 2011, Anthony Liguori wrote:
>>>
>>>> Upper case field names are not okay.  If you think coding style isn't
>>>> clear,
>>>> that's a bug in coding style.
>>>
>>> Sez hu? Coding style is garbage that should be thrown out of the window.
>>> As for looking, yeah, i'm looking at usb with it's lovely hungarian
>>> fields, should we stampede to "fix" it?
>>>
>>> If the one who's going to maintain the code is fine with whatever naming
>>> is used so be it.
>>
>> No.  That's how we got into the coding style mess we're in in the first
>> place.
>>
>> There's no benefit to going through and changing existing code but new code
>> needs to be consistent with the vast majority of code in the rest of the
>> tree.  It's about overall code base consistency and maintainability.
>
> I agree about importance of consistency, though I'd even go further
> and reformat globally. New code gets introduced based on copying old
> code so the pain goes on.

If we reformat globally (big if), then we better reformat to a
well-established style (such as linux kernel), not to this idiosyncratic
QEMU-only style.  Because the arguments about sharing and readability
apply across projects, too.

Re: [Qemu-devel] [PATCH] Add support for r6040 NIC

[Avi Kivity]

On 08/31/2011 04:05 AM, bifferos wrote:
> +    pci_register_bar(&s->dev, 0, 0x100, PCI_BASE_ADDRESS_SPACE_IO, r6040_map);
> +
>

pci_register_bar() has been converted to the memory API.  Please rebase 
to the latest qemu.git master and read docs/memory.txt and memory.h.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] Add support for r6040 NIC

[Gerd Hoffmann]

On 08/31/11 18:06, Anthony Liguori wrote:
> On 08/31/2011 09:35 AM, malc wrote:
>> On Wed, 31 Aug 2011, Anthony Liguori wrote:
>>
>>> Upper case field names are not okay. If you think coding style isn't
>>> clear,
>>> that's a bug in coding style.

> There's no benefit to going through and changing existing code but new
> code needs to be consistent with the vast majority of code in the rest
> of the tree. It's about overall code base consistency and maintainability.

I don't see the point in being too picky about this.  Especially 
consistency is a tricky thing.

As USB has been raised in this thread already:  The struct field names 
in the USBDesc* structs are actually consistent.  They are consistent 
with the naming convention in the USB world, i.e. you'll find those 
funky names like "wMaxPacketSize" in the USB specs, in the "lsusb -v" 
output and probably other places too.  Which makes alot of sense IMHO 
and is quite convenient when hacking the USB code.

The USB naming convention and the qemu naming convention don't match, so 
it is impossible to please everybody here ...

cheers,
   Gerd

Re: [Qemu-devel] [PATCH] Add support for r6040 NIC

[bifferos]

--- On Thu, 1/9/11, Avi Kivity <avi@redhat.com> wrote:
> On 08/31/2011 04:05 AM, bifferos
> wrote:
> > +    pci_register_bar(&s->dev, 0,
> 0x100, PCI_BASE_ADDRESS_SPACE_IO, r6040_map);

> pci_register_bar() has been converted to the memory
> API.  Please rebase to the latest qemu.git master and
> read docs/memory.txt and memory.h.

Here you go.


Signed-off-by: Mark Kelly <mark@bifferos.com>
diff --git a/Makefile.objs b/Makefile.objs
index d1f3e5d..345a362 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -253,6 +253,7 @@ hw-obj-$(CONFIG_PCNET_PCI) += pcnet-pci.o
 hw-obj-$(CONFIG_PCNET_COMMON) += pcnet.o
 hw-obj-$(CONFIG_E1000_PCI) += e1000.o
 hw-obj-$(CONFIG_RTL8139_PCI) += rtl8139.o
+hw-obj-$(CONFIG_R6040_PCI) += r6040.o
 
 hw-obj-$(CONFIG_SMC91C111) += smc91c111.o
 hw-obj-$(CONFIG_LAN9118) += lan9118.o
diff --git a/default-configs/pci.mak b/default-configs/pci.mak
index 22bd350..d2ea7a2 100644
--- a/default-configs/pci.mak
+++ b/default-configs/pci.mak
@@ -10,6 +10,7 @@ CONFIG_PCNET_PCI=y
 CONFIG_PCNET_COMMON=y
 CONFIG_LSI_SCSI_PCI=y
 CONFIG_RTL8139_PCI=y
+CONFIG_R6040_PCI=y
 CONFIG_E1000_PCI=y
 CONFIG_IDE_CORE=y
 CONFIG_IDE_QDEV=y
diff --git a/hw/pci.c b/hw/pci.c
index 57ff7b1..827ce8e 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1515,6 +1515,7 @@ static const char * const pci_nic_models[] = {
     "rtl8139",
     "e1000",
     "pcnet",
+    "r6040",
     "virtio",
     NULL
 };
@@ -1527,6 +1528,7 @@ static const char * const pci_nic_names[] = {
     "rtl8139",
     "e1000",
     "pcnet",
+    "r6040",
     "virtio-net-pci",
     NULL
 };
diff --git a/hw/pci_ids.h b/hw/pci_ids.h
index 83f3893..fd35e58 100644
--- a/hw/pci_ids.h
+++ b/hw/pci_ids.h
@@ -118,5 +118,8 @@
 #define PCI_DEVICE_ID_INTEL_82801I_EHCI1 0x293a
 #define PCI_DEVICE_ID_INTEL_82801I_EHCI2 0x293c
 
+#define PCI_VENDOR_ID_RDC                0x17f3
+#define PCI_DEVICE_ID_RDC_R6040          0x6040
+
 #define PCI_VENDOR_ID_XEN               0x5853
 #define PCI_DEVICE_ID_XEN_PLATFORM      0x0001
diff --git a/hw/r6040.c b/hw/r6040.c
new file mode 100644
index 0000000..258c8e6
--- /dev/null
+++ b/hw/r6040.c
@@ -0,0 +1,661 @@
+/*
+ * Emulation of r6040 ethernet controller found in a number of SoCs.
+ * Copyright (c) 2011 Mark Kelly, mark@bifferos.com
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included
+ * in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+ * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+ * NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+ * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+ * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR
+ * THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * This has been written using the R8610[1] and ip101a[2] datasheets.
+ *
+ * ICs with the embedded controller include R8610, R3210, AMRISC20000
+ * and Vortex86SX
+ *
+ * The emulation seems good enough to fool Linux 2.6.37.6.  It is
+ * not perfect, but has proven useful.
+ *
+ * [1] http://www.sima.com.tw/download/R8610_D06_20051003.pdf
+ * [2] http://www.icplus.com.tw/pp-IP101A.html
+ */
+
+#include "hw.h"
+#include "pci.h"
+#include "net.h"
+#include "loader.h"
+#include "sysemu.h"
+#include "qemu-timer.h"
+
+/* #define DEBUG_R6040 1 */
+
+
+#if defined DEBUG_R6040
+#define DPRINTF(fmt, ...) \
+    do { fprintf(stderr, "R6040: " fmt, ## __VA_ARGS__); } while (0)
+#else
+static inline GCC_FMT_ATTR(1, 2) int DPRINTF(const char *fmt, ...)
+{
+    return 0;
+}
+#endif
+
+
+/* Cast in order of appearance.  _W suffix means it's used to index the
+   register word array (regs_w)
+ */
+
+#define MPSCCR_W         (0x88 / 2)
+
+#define MAC0_W           (0x68 / 2)
+#define MAC1_W           (0x6a / 2)
+#define MAC2_W           (0x6c / 2)
+
+
+#define RX_START_LOW_W   (0x34 / 2)
+#define RX_START_HIGH_W  (0x38 / 2)
+#define TX_PKT_COUNT_W   (0x5a / 2)
+#define RX_PKT_COUNT_W   (0x50 / 2)
+
+
+#define MCR0_W           (0x00 / 2)
+#define MCR1_W           (0x04 / 2)
+#define BIT_MRST         (1 << 0)
+
+#define MTPR_W           (0x14 / 2)
+#define MRBSR_W          (0x18 / 2)
+#define MISR_W           (0x3c / 2)
+#define MIER_W           (0x40 / 2)
+
+#define MMDIO_W          (0x20 / 2)
+#define MDIO_READ_W      (0x24 / 2)
+#define MDIO_WRITE_W     (0x28 / 2)
+
+#define MRCNT_W          (0x50 / 2)
+#define MTCNT_W          (0x5c / 2)
+
+
+#define MDIO_WRITE      0x4000
+#define MDIO_READ       0x2000
+
+
+typedef struct R6040State {
+    PCIDevice dev;
+    NICState *nic;
+    NICConf conf;
+
+    /* PHY related register sets */
+    uint16_t mid0[3];
+    uint16_t phy_regs[32];
+    uint32_t phy_op_in_progress;
+
+    /* Primary IO address space */
+    union {
+        uint8_t regs_b[0x100];   /* Byte access */
+        uint16_t regs_w[0x100/2];  /* word access */
+        uint32_t regs_l[0x100/4];  /* DWORD access */
+    };
+
+    MemoryRegion bar_io;
+} R6040State;
+
+
+/* some inlines to help access above structure */
+static inline uint32_t TX_START(R6040State *s)
+{
+    uint32_t tmp = s->regs_w[0x2c/2];
+    return tmp | (s->regs_w[0x30/2] << 16);
+}
+
+static inline void TX_START_SET(R6040State *s, uint32_t start)
+{
+    s->regs_w[0x2c/2] = start & 0xffff;
+    s->regs_w[0x30/2] = (start >> 16) & 0xffff;
+}
+
+static inline uint32_t RX_START(R6040State *s)
+{
+    uint32_t tmp = s->regs_w[0x34/2];
+    return tmp | (s->regs_w[0x38/2] << 16);
+}
+
+static inline void RX_START_SET(R6040State *s, uint32_t start)
+{
+    s->regs_w[0x34/2] = start & 0xffff;
+    s->regs_w[0x38/2] = (start >> 16) & 0xffff;
+}
+
+
+static void r6040_update_irq(R6040State *s)
+{
+    uint16_t isr = s->regs_w[MISR_W] & s->regs_w[MIER_W];
+
+    qemu_set_irq(s->dev.irq[0], isr ? 1 : 0);
+}
+
+
+/* Mark auto-neg complete, NIC up.  */
+static void PhysicalLinkUp(void *opaque)
+{
+    R6040State *s = opaque;
+    s->phy_regs[1] |= (1 << 2);
+}
+
+
+/* Transmit and receive descriptors are doubled up
+   One is a subset of the other anyhow
+ */
+typedef struct Descriptor {
+    uint16_t dst;
+    uint16_t dlen;
+    uint32_t dbp;
+    uint32_t dnx;
+    uint16_t hidx;
+    uint16_t reserved_1;
+    uint16_t reserved_2;
+} Descriptor;
+
+
+/* Some debugging functions */
+
+#ifdef DEBUG_R6040
+static void addr_dump16(const char *name, uint16_t val)
+{
+    DPRINTF("%s: 0x%04x  ", name, val);
+}
+
+static void addr_dump32(const char *name, uint32_t val)
+{
+    DPRINTF("%s: 0x%x  ", name, val);
+}
+
+static void hex_dump(const uint8_t *data, uint32_t len)
+{
+    uint8_t i;
+    DPRINTF("hex: ");
+    for (i = 0; i < len; i++) {
+        fprintf(stderr, "%02x ", *data);
+        if (i && !(i % 0x20)) {
+            fprintf(stderr, "\n");
+        }
+        data++;
+    }
+    fprintf(stderr, "\n");
+}
+
+static void desc_dump(Descriptor *d, uint32_t addr)
+{
+    DPRINTF("\nDumping: 0x%x\n", addr);
+    addr_dump16("DST", d->dst);
+    addr_dump16("DLEN", d->dlen);
+    addr_dump32("DBP", (unsigned long)d->dbp);
+    addr_dump32("DNX", (unsigned long)d->dnx);
+    addr_dump16("HIDX", d->hidx);
+    printf("\n");
+}
+
+static void dump_phys_mem(uint32_t addr, int len)
+{
+    uint8_t buffer[1024];
+    cpu_physical_memory_read(addr, buffer, len);
+    hex_dump(buffer, len);
+}
+
+static void dump_pci(uint8_t *pci_conf)
+{
+    uint32_t *p = (uint32_t *)pci_conf;
+    int i = 0;
+    for (i = 0; i < 0x40; i += 4) {
+        DPRINTF("Addr: 0x%08x,  Data: 0x%08x\n", i, *p);
+        p++;
+    }
+}
+#endif
+
+
+static const VMStateDescription vmstate_r6040 = {
+    .name = "r6040",
+    .version_id = 3,
+    .minimum_version_id = 2,
+    .minimum_version_id_old = 2,
+    .fields = (VMStateField[]) {
+        VMSTATE_PCI_DEVICE(dev, R6040State),
+        VMSTATE_BUFFER(regs_b, R6040State),
+        VMSTATE_UINT16_ARRAY(mid0, R6040State, 3),
+        VMSTATE_UINT16_ARRAY(phy_regs, R6040State, 32),
+        VMSTATE_UINT32(phy_op_in_progress, R6040State),
+        VMSTATE_MACADDR(conf.macaddr, R6040State),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+
+static int TryToSendOnePacket(void *opaque)
+{
+    R6040State *s = opaque;
+    Descriptor d;
+    uint8_t pkt_buffer[2000];
+    uint32_t tocopy;
+
+    cpu_physical_memory_read(TX_START(s), (uint8_t *)&d, sizeof(d));
+
+    if (d.dst & 0x8000) {    /* MAC owns it? */
+        tocopy = d.dlen;
+        if (tocopy > sizeof(pkt_buffer)) {
+            tocopy = sizeof(pkt_buffer);
+        }
+        /* copy the packet to send it */
+        cpu_physical_memory_read(d.dbp, pkt_buffer, tocopy);
+
+        qemu_send_packet(&s->nic->nc, pkt_buffer, tocopy);
+        s->regs_w[TX_PKT_COUNT_W]++;
+
+        /* relinquish ownership, we're done with it */
+        d.dst &= ~0x8000;
+
+        /* Copy the new version of the descriptor back */
+        cpu_physical_memory_write(TX_START(s), (uint8_t *)&d, sizeof(d));
+
+        /* Advance to the next buffer if packet processed */
+        TX_START_SET(s, d.dnx);
+
+        return 1;
+    }
+
+    return 0;
+}
+
+
+static void r6040_transmit(void *opaque)
+{
+    R6040State *s = opaque;
+    int count = 0;
+
+    while (TryToSendOnePacket(s)) {
+        ++count;
+    }
+
+    if (count) {
+        s->regs_w[MISR_W] |= 0x10;
+        r6040_update_irq(s);
+    }
+}
+
+
+/* Whether to allow callback returning 1 for yes, can receive */
+static int r6040_can_receive(VLANClientState *nc)
+{
+    R6040State *s = DO_UPCAST(NICState, nc, nc)->opaque;
+    int tmp = (s->regs_w[0] & (1 << 1)) ? 1 : 0;
+    return tmp;
+}
+
+
+static int ReceiveOnePacket(void *opaque, const uint8_t *buf, size_t len)
+{
+    R6040State *s = opaque;
+    uint32_t tocopy = len+4;  /* include checksum */
+    Descriptor d;
+
+    cpu_physical_memory_read(RX_START(s), (uint8_t *)&d, sizeof(Descriptor));
+    /*desc_dump(&d, 0);*/
+
+    if (d.dst & 0x8000) {    /* MAC owned? */
+
+        uint16_t max_buffer = s->regs_w[MRBSR_W] & 0x07fc;
+        if (tocopy > max_buffer) {
+            tocopy = max_buffer;
+        }
+
+        cpu_physical_memory_write(d.dbp, buf, tocopy-4);
+
+        /* indicate received OK */
+        d.dst |= (1 << 14);
+        d.dlen = tocopy;
+        /* relinquish ownership */
+        d.dst &= ~0x8000;
+
+        /* Copy the descriptor back */
+        cpu_physical_memory_write(RX_START(s), (uint8_t *)&d,
+                                   sizeof(Descriptor));
+
+        s->regs_w[RX_PKT_COUNT_W]++;
+
+        s->regs_w[MISR_W] |= 1;  /* received pkt interrupt */
+
+        r6040_update_irq(s);
+
+        RX_START_SET(s, d.dnx);  /* advance */
+
+        return 0;
+    }
+    return -1;
+}
+
+
+/* called on incoming packets */
+static ssize_t r6040_receive(VLANClientState *nc, const uint8_t *buf,
+                                        size_t len)
+{
+    R6040State *s = DO_UPCAST(NICState, nc, nc)->opaque;
+    DPRINTF("Received incoming packet of len %ld\n", len);
+
+    if (0 == ReceiveOnePacket(s, buf, len)) {
+        return len;  /* copied OK */
+    }
+
+    return 0;
+}
+
+
+static void r6040_cleanup(VLANClientState *vc)
+{
+    DPRINTF("r6040_cleanup\n");
+}
+
+
+static inline int BIT_SET(uint16_t old, uint16_t new, uint16_t bit)
+{
+    uint16_t before = (old & (1 << bit));
+    uint16_t after = (new & (1 << bit));
+    if (!before && after) {
+        return 1;
+    }
+    return 0;
+}
+
+
+static void r6040_ioport_writew(void *opaque, uint32_t addr, uint32_t val)
+{
+    R6040State *s = opaque;
+    uint16_t old;
+    addr &= 0xff;   /* get relative to base address */
+    addr /= 2;    /* Get the offset into the word-array */
+    old = s->regs_w[addr];   /* store the old value for future use */
+
+    switch (addr) {
+    case MCR0_W:   /* 0x00 */
+        if (BIT_SET(old, val, 12)) {
+            r6040_transmit(opaque);
+        }
+        break;
+    case MCR1_W:  /* 0x04 */
+        if (val & BIT_MRST) {   /* reset request incoming */
+            /* reset requested, complete it immediately, set this value to
+               default */
+            val = 0x0010;
+        }
+        break;
+    case MTPR_W:  /* TX command reg, 0x14 */
+        if (val & 1) {
+            r6040_transmit(opaque);
+            val &= ~1;
+        }
+        break;
+    case MMDIO_W:  /* MDIO control, 0x20 */
+        {
+            int phy_exists = ((val & 0x1f00) == 0x100) ? 1 : 0;
+            uint16_t *phy = s->phy_regs;
+            phy += (val & 0x1f);
+
+            if  (val & (1 << 13)) {   /* read data */
+                if (phy_exists) {
+                    s->regs_w[MDIO_READ_W] = *phy;
+                } else {
+                    s->regs_w[MDIO_READ_W] = 0xffff;
+                }
+            } else if (val & (1 << 14)) {  /* write data */
+                if (phy_exists) {
+                    *phy = s->regs_w[MDIO_WRITE_W];
+                }
+            }
+
+            /* Whether you request to read or write, both bits go high while
+               the operation is in progress, e.g. tell it to read, and the
+               write-in-progress flag also goes high */
+            val |= 0x6000;  /* signal operation has started */
+            s->phy_op_in_progress = 1;
+
+            break;
+        }
+    case MISR_W:  /* interrupt status reg (read to clear), 0x3c */
+        return;
+
+    case MIER_W:  /* interrupt enable register, 0x40 */
+        s->regs_w[MIER_W] = val;
+        r6040_update_irq(s);
+        return;
+
+    case MRCNT_W:   /* 0x50 */
+    case MTCNT_W:   /* 0x5c */
+        return;  /* Can't write to pkt count registers, skip */
+
+    }
+    s->regs_w[addr] = val & 0xFFFF;
+}
+
+
+static uint32_t r6040_ioport_readw(void *opaque, uint32_t addr)
+{
+    R6040State *s = opaque;
+    addr &= 0xff;   /* get relative to base address */
+    addr /= 2;    /* Get the offset into the word-array */
+    uint32_t tmp = s->regs_w[addr];  /* get the value */
+
+    switch (addr) {
+
+    case MMDIO_W:  /* MDIO control, 0x20 */
+        {
+            /* Clear any in-progress MDIO activity for the next read
+               This simulates the polling of the MDIO operation status,
+               so the driver code always has to read the register twice
+               before it thinks the operation is complete. */
+            if (s->phy_op_in_progress) {
+                s->regs_w[addr] &= ~0x6000;
+                s->phy_op_in_progress = 0;
+            }
+            break;
+        }
+    case MISR_W:  /* interrupt status reg (read to clear)  0x3c */
+        s->regs_w[addr] = 0;
+        break;
+    case MIER_W:  /* interrupt enable reg 0x40 */
+        break;
+    case MRCNT_W:  /* 0x50 */
+    case MTCNT_W:  /* 0x5c */
+        s->regs_w[addr] = 0;   /* read to clear */
+        break;
+    default:
+        break;
+    }
+    return tmp;
+}
+
+
+/* byte and long access are routed via the word operation handlers */
+static void r6040_ioport_writeb(void *opaque, uint32_t addr, uint32_t val)
+{
+    R6040State *s = opaque;
+    addr &= 0xFF;
+    val &= 0xFF;
+    uint16_t old = s->regs_w[addr/2];  /* get the current value */
+    if (addr & 1) {
+        old &= 0xff;
+        old |= (val << 8);
+    } else {
+        old &= 0xff00;
+        old |= val;
+    }
+
+    r6040_ioport_writew(opaque, addr, old);  /* call the word-based version */
+}
+
+static void r6040_ioport_writel(void *opaque, uint32_t addr, uint32_t val)
+{
+    /* Set the low value */
+    r6040_ioport_writew(opaque, addr, val & 0xffff);
+    /* Set the high value */
+    r6040_ioport_writew(opaque, addr+2, (val >> 16) & 0xffff);
+}
+
+static uint32_t r6040_ioport_readb(void *opaque, uint32_t addr)
+{
+    uint32_t tmp = r6040_ioport_readw(opaque, addr & ~1);
+    if (addr & 1) {
+        return (tmp & 0xff00) >> 8;
+    }
+    return tmp & 0xff;
+}
+
+static uint32_t r6040_ioport_readl(void *opaque, uint32_t addr)
+{
+    uint32_t tmp = r6040_ioport_readw(opaque, addr);
+    return tmp | (r6040_ioport_readw(opaque, addr+2) << 16);
+}
+
+
+static const MemoryRegionPortio r6040_portio[] = {
+    { 0, 0x100, 1, .read = r6040_ioport_readb, },
+    { 0, 0x100, 1, .write = r6040_ioport_writeb, },
+    { 0, 0x100, 2, .read = r6040_ioport_readw, },
+    { 0, 0x100, 2, .write = r6040_ioport_writew, },
+    { 0, 0x100, 4, .read = r6040_ioport_readl, },
+    { 0, 0x100, 4, .write = r6040_ioport_writel, },
+    PORTIO_END_OF_LIST()
+};
+
+static const MemoryRegionOps r6040_io_ops = {
+    .old_portio = r6040_portio,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+
+static void r6040_register_ioports(R6040State *s, pcibus_t addr)
+{
+    register_ioport_write(addr, 0x100, 1, r6040_ioport_writeb, s);
+    register_ioport_read(addr, 0x100, 1, r6040_ioport_readb,  s);
+
+    register_ioport_write(addr, 0x100, 2, r6040_ioport_writew, s);
+    register_ioport_read(addr, 0x100, 2, r6040_ioport_readw,  s);
+
+    register_ioport_write(addr, 0x100, 4, r6040_ioport_writel, s);
+    register_ioport_read(addr, 0x100, 4, r6040_ioport_readl,  s);
+}
+
+
+static NetClientInfo net_r6040_info = {
+    .type = NET_CLIENT_TYPE_NIC,
+    .size = sizeof(NICState),
+    .can_receive = r6040_can_receive,
+    .receive = r6040_receive,
+    .cleanup = r6040_cleanup,
+};
+
+
+static int r6040_init(PCIDevice *dev)
+{
+    QEMUTimer *timer;
+
+    R6040State *s = DO_UPCAST(R6040State, dev, dev);
+    uint8_t *pci_conf;
+
+    /* MAC PHYS status change register.  Linux driver expects something
+       sensible as default and if not will try to set it */
+    s->regs_w[MPSCCR_W] = 0x9f07;
+
+    /* Default value for maximum packet size */
+    s->regs_w[MRBSR_W] = 0x600;
+
+    /* set the MAC, linux driver reads this when it loads, it is
+       normally set by the BIOS, but obviously Qemu BIOS isn't going
+       to do that */
+    s->regs_w[MAC0_W] = 0x5452;
+    s->regs_w[MAC1_W] = 0x1200;
+    s->regs_w[MAC2_W] = 0x5734;
+
+    /* Tell Qemu the same thing */
+    s->conf.macaddr.a[0] = s->regs_w[MAC0_W] & 0xff;
+    s->conf.macaddr.a[1] = (s->regs_w[MAC0_W] >> 8) & 0xff;
+    s->conf.macaddr.a[2] = s->regs_w[MAC1_W] & 0xff;
+    s->conf.macaddr.a[3] = (s->regs_w[MAC1_W] >> 8) & 0xff;
+    s->conf.macaddr.a[4] = s->regs_w[MAC2_W] & 0xff;
+    s->conf.macaddr.a[5] = (s->regs_w[MAC2_W] >> 8) & 0xff;
+
+    /* no commands running */
+    s->phy_op_in_progress = 0;
+
+    /* PHY auto-neg in progress */
+    s->phy_regs[1] = 0x786d & ~(1 << 2);
+    s->phy_regs[2] = 0x0243;
+    s->phy_regs[3] = 0x0c54;
+
+    pci_conf = (uint8_t *)s->dev.config;
+
+    pci_conf[PCI_HEADER_TYPE] = PCI_HEADER_TYPE_NORMAL; /* header_type */
+    pci_conf[PCI_INTERRUPT_LINE] = 0xa;     /* interrupt line  */
+    pci_conf[PCI_INTERRUPT_PIN] = 1;      /* interrupt pin 0 */
+
+    memory_region_init_io(&s->bar_io, &r6040_io_ops, s, "r6040", 0x100);
+    pci_register_bar(&s->dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->bar_io);
+
+    s->nic = qemu_new_nic(&net_r6040_info, &s->conf,
+                            dev->qdev.info->name, dev->qdev.id, s);
+
+    qemu_format_nic_info_str(&s->nic->nc, s->conf.macaddr.a);
+
+    /* Simulate a delay of a couple of seconds for the link to come up.
+       Not required for Linux but very handy for developing BIOS code.
+     */
+    timer = qemu_new_timer_ns(vm_clock, PhysicalLinkUp, s);
+    qemu_mod_timer(timer, qemu_get_clock_ms(vm_clock) + 1500000000);
+
+    /* Register IO port access as well */
+    r6040_register_ioports(s, 0xe800);
+
+    return 0;
+}
+
+
+static int r6040_exit(PCIDevice *dev)
+{
+    R6040State *s = DO_UPCAST(R6040State, dev, dev);
+    memory_region_destroy(&s->bar_io);
+    return 0;
+}
+
+
+static PCIDeviceInfo r6040_info = {
+    .qdev.name = "r6040",
+    .qdev.size = sizeof(R6040State),
+    .qdev.vmsd  = &vmstate_r6040,
+    .init      = r6040_init,
+    .exit      = r6040_exit,
+    .vendor_id = PCI_VENDOR_ID_RDC,
+    .device_id = PCI_DEVICE_ID_RDC_R6040,
+    .class_id   = PCI_CLASS_NETWORK_ETHERNET,
+    .qdev.props = (Property[]) {
+        DEFINE_NIC_PROPERTIES(R6040State, conf),
+        DEFINE_PROP_END_OF_LIST(),
+    }
+};
+
+
+static void r6040_register(void)
+{
+    pci_qdev_register(&r6040_info);
+}
+
+
+device_init(r6040_register)

Re: [Qemu-devel] [PATCH] Add support for r6040 NIC

[Anthony Liguori]

On 09/01/2011 02:39 AM, Markus Armbruster wrote:
> Blue Swirl<blauwirbel@gmail.com>  writes:
>
>> On Wed, Aug 31, 2011 at 4:06 PM, Anthony Liguori<anthony@codemonkey.ws>  wrote:
>>> On 08/31/2011 09:35 AM, malc wrote:
>>>>
>>>> On Wed, 31 Aug 2011, Anthony Liguori wrote:
>>>>
>>>>> Upper case field names are not okay.  If you think coding style isn't
>>>>> clear,
>>>>> that's a bug in coding style.
>>>>
>>>> Sez hu? Coding style is garbage that should be thrown out of the window.
>>>> As for looking, yeah, i'm looking at usb with it's lovely hungarian
>>>> fields, should we stampede to "fix" it?
>>>>
>>>> If the one who's going to maintain the code is fine with whatever naming
>>>> is used so be it.
>>>
>>> No.  That's how we got into the coding style mess we're in in the first
>>> place.
>>>
>>> There's no benefit to going through and changing existing code but new code
>>> needs to be consistent with the vast majority of code in the rest of the
>>> tree.  It's about overall code base consistency and maintainability.
>>
>> I agree about importance of consistency, though I'd even go further
>> and reformat globally. New code gets introduced based on copying old
>> code so the pain goes on.
>
> If we reformat globally (big if),

I'm very strongly opposed to doing a global reformat.  It makes it 
harder to use things like git blame which makes reviewing code difficult.

Following a reasonable policy of using a consistent coding style and 
only fixing style issues when you touch code for other reasons is well 
established (this is the kernel policy) and over time will result in a 
reasonably consistent code base.

Regards,

Anthony Liguori

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] Add support for r6040 NIC

[Blue Swirl]

On Thu, Sep 1, 2011 at 9:49 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 09/01/2011 02:39 AM, Markus Armbruster wrote:
>>
>> Blue Swirl<blauwirbel@gmail.com>  writes:
>>
>>> On Wed, Aug 31, 2011 at 4:06 PM, Anthony Liguori<anthony@codemonkey.ws>
>>>  wrote:
>>>>
>>>> On 08/31/2011 09:35 AM, malc wrote:
>>>>>
>>>>> On Wed, 31 Aug 2011, Anthony Liguori wrote:
>>>>>
>>>>>> Upper case field names are not okay.  If you think coding style isn't
>>>>>> clear,
>>>>>> that's a bug in coding style.
>>>>>
>>>>> Sez hu? Coding style is garbage that should be thrown out of the
>>>>> window.
>>>>> As for looking, yeah, i'm looking at usb with it's lovely hungarian
>>>>> fields, should we stampede to "fix" it?
>>>>>
>>>>> If the one who's going to maintain the code is fine with whatever
>>>>> naming
>>>>> is used so be it.
>>>>
>>>> No.  That's how we got into the coding style mess we're in in the first
>>>> place.
>>>>
>>>> There's no benefit to going through and changing existing code but new
>>>> code
>>>> needs to be consistent with the vast majority of code in the rest of the
>>>> tree.  It's about overall code base consistency and maintainability.
>>>
>>> I agree about importance of consistency, though I'd even go further
>>> and reformat globally. New code gets introduced based on copying old
>>> code so the pain goes on.
>>
>> If we reformat globally (big if),
>
> I'm very strongly opposed to doing a global reformat.  It makes it harder to
> use things like git blame which makes reviewing code difficult.

Only for one commit, the commits before and after would still be
accurate. There is a tradeoff between benefit of consistent, better
quality code and usefulness of git blame. I'd value consistency
higher. Inaccuracy is also relative, the reformatting commit is still
a commit.

> Following a reasonable policy of using a consistent coding style and only
> fixing style issues when you touch code for other reasons is well
> established (this is the kernel policy) and over time will result in a
> reasonably consistent code base.

This assumes that all code gets touched every now and then. But in
reality some things may never need any changes and then they remain
inconsistent. Then bad practices spread from this unchanged base if
it's large enough, just like now.

By the way, if we assume the opposite (all code is under change), then
some time after global reformat git blame would also be accurate since
the reformatting commit would be overwritten by the newer changes.