[Qemu-devel] [PATCH 0/2] hw/arm: add ethernet support to Allwinner A10

[Qemu-devel] [PATCH 0/2] hw/arm: add ethernet support to Allwinner A10

[Beniamino Galvani]

This patch series adds support for the EMAC Fast Ethernet controller
found on Allwinner SoCs to the Allwinner A10.

Beniamino Galvani (2):
  hw/net: add support for Allwinner EMAC Fast Ethernet controller
  hw/arm/allwinner-a10: initialize EMAC

 default-configs/arm-softmmu.mak |    1 +
 hw/arm/allwinner-a10.c          |   20 ++
 hw/net/Makefile.objs            |    1 +
 hw/net/allwinner_emac.c         |  466 +++++++++++++++++++++++++++++++++++++++
 include/hw/arm/allwinner-a10.h  |    4 +
 include/hw/net/allwinner_emac.h |  204 +++++++++++++++++
 6 files changed, 696 insertions(+)
 create mode 100644 hw/net/allwinner_emac.c
 create mode 100644 include/hw/net/allwinner_emac.h

-- 
1.7.10.4

[Qemu-devel] [PATCH 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller

[Beniamino Galvani]

This patch adds support for the Fast Ethernet MAC found on Allwinner
SoCs, together with a basic emulation of Realtek RTL8201CP PHY.

Since there is no public documentation of the Allwinner controller, the
implementation is based on Linux kernel driver.

Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
---
 default-configs/arm-softmmu.mak |    1 +
 hw/net/Makefile.objs            |    1 +
 hw/net/allwinner_emac.c         |  466 +++++++++++++++++++++++++++++++++++++++
 include/hw/net/allwinner_emac.h |  204 +++++++++++++++++
 4 files changed, 672 insertions(+)
 create mode 100644 hw/net/allwinner_emac.c
 create mode 100644 include/hw/net/allwinner_emac.h

diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index ce1d620..f3513fa 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -27,6 +27,7 @@ CONFIG_SSI_SD=y
 CONFIG_SSI_M25P80=y
 CONFIG_LAN9118=y
 CONFIG_SMC91C111=y
+CONFIG_ALLWINNER_EMAC=y
 CONFIG_DS1338=y
 CONFIG_PFLASH_CFI01=y
 CONFIG_PFLASH_CFI02=y
diff --git a/hw/net/Makefile.objs b/hw/net/Makefile.objs
index 951cca3..75e80c2 100644
--- a/hw/net/Makefile.objs
+++ b/hw/net/Makefile.objs
@@ -18,6 +18,7 @@ common-obj-$(CONFIG_OPENCORES_ETH) += opencores_eth.o
 common-obj-$(CONFIG_XGMAC) += xgmac.o
 common-obj-$(CONFIG_MIPSNET) += mipsnet.o
 common-obj-$(CONFIG_XILINX_AXI) += xilinx_axienet.o
+common-obj-$(CONFIG_ALLWINNER_EMAC) += allwinner_emac.o
 
 common-obj-$(CONFIG_CADENCE) += cadence_gem.o
 common-obj-$(CONFIG_STELLARIS_ENET) += stellaris_enet.o
diff --git a/hw/net/allwinner_emac.c b/hw/net/allwinner_emac.c
new file mode 100644
index 0000000..9791be4
--- /dev/null
+++ b/hw/net/allwinner_emac.c
@@ -0,0 +1,466 @@
+/*
+ * Emulation of Allwinner EMAC Fast Ethernet controller and
+ * Realtek RTL8201CP PHY
+ *
+ * Copyright (C) 2013 Beniamino Galvani <b.galvani@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+#include "hw/sysbus.h"
+#include "net/net.h"
+#include "hw/net/allwinner_emac.h"
+#include <zlib.h>
+
+#undef AW_EMAC_DEBUG
+
+#ifdef AW_EMAC_DEBUG
+#define debug(...)                                              \
+    do {                                                        \
+        fprintf(stderr,  "allwinner_emac : %s: ", __func__);    \
+        fprintf(stderr, ## __VA_ARGS__);                        \
+    } while (0)
+#else
+#define debug(...) do {} while (0)
+#endif
+
+static void mii_set_link(AwEmacMii *mii, bool link_ok)
+{
+    if (link_ok) {
+        mii->bmsr |= MII_BMSR_LINK_ST;
+        mii->anlpar |= MII_ANAR_TXFD | MII_ANAR_TX | MII_ANAR_10FD |
+                       MII_ANAR_10 | MII_ANAR_CSMACD;
+    } else {
+        mii->bmsr &= ~MII_BMSR_LINK_ST;
+        mii->anlpar = MII_ANAR_TX;
+    }
+    mii->link_ok = link_ok;
+}
+
+static void mii_reset(AwEmacMii *mii)
+{
+    mii->bmcr = MII_BMCR_FD | MII_BMCR_AUTOEN | MII_BMCR_SPEED;
+    mii->bmsr = MII_BMSR_100TX_FD | MII_BMSR_100TX_HD | MII_BMSR_10T_FD |
+                MII_BMSR_10T_HD | MII_BMSR_MFPS | MII_BMSR_AUTONEG;
+    mii->anar = MII_ANAR_TXFD | MII_ANAR_TX | MII_ANAR_10FD | MII_ANAR_10 |
+                MII_ANAR_CSMACD;
+    mii->anlpar = MII_ANAR_TX;
+    mii_set_link(mii, mii->link_ok);
+}
+
+static uint16_t mii_read(AwEmacMii *mii, uint8_t phy_addr, uint8_t reg)
+{
+    uint16_t ret = 0xffff;
+
+    if (phy_addr == BOARD_PHY_ADDRESS) {
+        switch (reg) {
+        case MII_BMCR:
+            ret = mii->bmcr;
+            break;
+        case MII_BMSR:
+            ret = mii->bmsr;
+            break;
+        case MII_PHYID1:
+            ret = RTL8201CP_PHYID1;
+            break;
+        case MII_PHYID2:
+            ret = RTL8201CP_PHYID2;
+            break;
+        case MII_ANAR:
+            ret = mii->anar;
+            break;
+        case MII_ANLPAR:
+            ret = mii->anlpar;
+            break;
+        default:
+            debug("unknown mii register %x\n", reg);
+            ret = 0;
+        }
+    }
+    return ret;
+}
+
+static void mii_write(AwEmacMii *mii, uint8_t phy_addr, uint8_t reg,
+                      uint16_t value)
+{
+    if (phy_addr == BOARD_PHY_ADDRESS) {
+        switch (reg) {
+        case MII_BMCR:
+            if (value & MII_BMCR_RESET) {
+                mii_reset(mii);
+            } else {
+                mii->bmcr = value;
+            }
+            break;
+        case MII_BMSR:
+        case MII_PHYID1:
+        case MII_PHYID2:
+        case MII_ANLPAR:
+            qemu_log_mask(LOG_GUEST_ERROR, "%s: write to mii register %x\n",
+                          __func__, reg);
+            break;
+        case MII_ANAR:
+            mii->anar = value;
+            break;
+        default:
+            debug("unknown mii register %x\n", reg);
+        }
+    }
+}
+
+static void aw_emac_update_irq(AwEmacState *s)
+{
+    qemu_set_irq(s->irq, (s->int_sta & s->int_ctl) != 0);
+}
+
+static int aw_emac_can_receive(NetClientState *nc)
+{
+    AwEmacState *s = qemu_get_nic_opaque(nc);
+
+    return (s->ctl & EMAC_CTL_RX_EN) && (s->num_rx < MAX_RX);
+}
+
+static ssize_t aw_emac_receive(NetClientState *nc, const uint8_t *buf,
+                               size_t size)
+{
+    AwEmacState *s = qemu_get_nic_opaque(nc);
+    uint32_t *fifo;
+    uint32_t crc;
+    char *dest;
+
+    if (s->num_rx >= MAX_RX) {
+        debug("rx queue full - packed dropped\n");
+        return -1;
+    }
+
+    if (size + RX_HDR_SIZE > FIFO_SIZE) {
+        debug("packet too big\n");
+        return -1;
+    }
+
+    fifo = s->rx_fifos[(s->first_rx + s->num_rx) % MAX_RX];
+    dest = (char *)&fifo[2];
+    s->num_rx++;
+
+    memcpy(dest, buf, size);
+
+    /* Fill to minimum frame length */
+    if (size < 60) {
+        memset(dest + size, 0, 60 - size);
+        size = 60;
+    }
+
+    /* Append FCS */
+    crc = crc32(~0, buf, size);
+    memcpy(dest + size, &crc, 4);
+
+    fifo[0] = EMAC_UNDOCUMENTED_MAGIC;
+    fifo[1] = EMAC_RX_HEADER(size + 4, EMAC_RX_IO_DATA_STATUS_OK);
+
+    /* Set rx interrupt flag */
+    s->int_sta |= EMAC_INT_RX;
+    aw_emac_update_irq(s);
+
+    return size;
+}
+
+static void aw_emac_cleanup(NetClientState *nc)
+{
+    AwEmacState *s = qemu_get_nic_opaque(nc);
+
+    s->nic = NULL;
+}
+
+static void aw_emac_reset(AwEmacState *s)
+{
+    s->ctl = 0;
+    s->tx_mode = 0;
+    s->int_ctl = 0;
+    s->int_sta = 0;
+    s->tx_channel = 0;
+    s->first_rx = 0;
+    s->num_rx = 0;
+    s->rx_offset = 0;
+    s->phy_target = 0;
+}
+
+static uint64_t aw_emac_read(void *opaque, hwaddr offset, unsigned size)
+{
+    AwEmacState *s = opaque;
+    uint64_t ret;
+    uint32_t *rx_fifo;
+
+    switch (offset) {
+    case EMAC_CTL_REG:
+        ret = s->ctl;
+        break;
+    case EMAC_TX_MODE_REG:
+        ret = s->tx_mode;
+        break;
+    case EMAC_TX_INS_REG:
+        ret = s->tx_channel;
+        break;
+    case EMAC_RX_CTL_REG:
+        ret = s->rx_ctl;
+        break;
+    case EMAC_RX_IO_DATA_REG:
+        if (!s->num_rx) {
+            ret = 0;
+            break;
+        }
+        rx_fifo = s->rx_fifos[s->first_rx];
+
+        if (s->rx_offset >= FIFO_SIZE ||
+            s->rx_offset >= EMAC_RX_IO_DATA_LEN(rx_fifo[1]) + RX_HDR_SIZE) {
+            /* This should never happen */
+            debug("RX fifo overrun\n");
+            s->first_rx = (s->first_rx + 1) % MAX_RX;
+            s->num_rx--;
+            s->rx_offset = 0;
+            ret = 0;
+            break;
+        }
+
+        ret = rx_fifo[s->rx_offset >> 2];
+        s->rx_offset += 4;
+
+        if (s->rx_offset >= EMAC_RX_IO_DATA_LEN(rx_fifo[1]) + RX_HDR_SIZE) {
+            s->first_rx = (s->first_rx + 1) % MAX_RX;
+            s->num_rx--;
+            s->rx_offset = 0;
+        }
+        break;
+    case EMAC_RX_FBC_REG:
+        ret = s->num_rx;
+        break;
+    case EMAC_INT_CTL_REG:
+        ret = s->int_ctl;
+        break;
+    case EMAC_INT_STA_REG:
+        ret = s->int_sta;
+        break;
+    case EMAC_MAC_MRDD_REG:
+        ret = mii_read(&s->mii, PHY_TARGET_ADDR(s->phy_target),
+                       PHY_TARGET_REG(s->phy_target));
+        break;
+    default:
+        debug("unknown offset %08x\n", (unsigned int)offset);
+        ret = 0;
+    }
+
+    return ret;
+}
+
+static void aw_emac_write(void *opaque, hwaddr offset, uint64_t value,
+                          unsigned size)
+{
+    AwEmacState *s = opaque;
+    AwEmacTxFifo *fifo;
+    int chan;
+
+    switch (offset) {
+    case EMAC_CTL_REG:
+        if (value & EMAC_CTL_RESET) {
+            debug("reset\n");
+            aw_emac_reset(s);
+            value &= ~EMAC_CTL_RESET;
+        }
+        s->ctl = value;
+        break;
+    case EMAC_TX_MODE_REG:
+        s->tx_mode = value;
+        break;
+    case EMAC_TX_CTL0_REG:
+    case EMAC_TX_CTL1_REG:
+        chan = (offset == EMAC_TX_CTL0_REG ? 0 : 1);
+        if ((value & 1) && (s->ctl & EMAC_CTL_TX_EN)) {
+            qemu_send_packet(qemu_get_queue(s->nic),
+                             (uint8_t *)s->tx_fifos[chan].data,
+                             s->tx_fifos[chan].length);
+
+            /* Raise TX interrupt */
+            s->int_sta |= EMAC_INT_TX_CHAN(chan);
+            s->tx_fifos[chan].offset = 0;
+            aw_emac_update_irq(s);
+        }
+        break;
+    case EMAC_TX_INS_REG:
+        s->tx_channel = value < NUM_CHAN ? value : 0;
+        break;
+    case EMAC_TX_PL0_REG:
+    case EMAC_TX_PL1_REG:
+        chan = (offset == EMAC_TX_PL0_REG ? 0 : 1);
+        if (value > FIFO_SIZE) {
+            debug("invalid TX frame length %d\n", (int)value);
+            value = FIFO_SIZE;
+        }
+        s->tx_fifos[chan].length = value;
+        break;
+    case EMAC_TX_IO_DATA_REG:
+        fifo = &s->tx_fifos[s->tx_channel];
+        if (fifo->offset >= FIFO_SIZE) {
+            debug("TX frame data overruns fifo (%d)\n", fifo->offset);
+            break;
+        }
+        fifo->data[fifo->offset >> 2] = value;
+        fifo->offset += 4;
+        break;
+    case EMAC_RX_CTL_REG:
+        s->rx_ctl = value;
+        break;
+    case EMAC_RX_FBC_REG:
+        if (value == 0) {
+            s->num_rx = 0;
+        }
+        break;
+    case EMAC_INT_CTL_REG:
+        s->int_ctl = value;
+        break;
+    case EMAC_INT_STA_REG:
+        s->int_sta &= ~value;
+        break;
+    case EMAC_MAC_MADR_REG:
+        s->phy_target = value;
+        break;
+    case EMAC_MAC_MWTD_REG:
+        mii_write(&s->mii, PHY_TARGET_ADDR(s->phy_target),
+                  PHY_TARGET_REG(s->phy_target), value);
+        break;
+    default:
+        debug("unknown offset %08x\n", (unsigned int)offset);
+    }
+}
+
+static void aw_emac_set_link(NetClientState *nc)
+{
+    AwEmacState *s = qemu_get_nic_opaque(nc);
+
+    mii_set_link(&s->mii, !nc->link_down);
+}
+
+static const MemoryRegionOps aw_emac_mem_ops = {
+    .read = aw_emac_read,
+    .write = aw_emac_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static NetClientInfo net_aw_emac_info = {
+    .type = NET_CLIENT_OPTIONS_KIND_NIC,
+    .size = sizeof(NICState),
+    .can_receive = aw_emac_can_receive,
+    .receive = aw_emac_receive,
+    .cleanup = aw_emac_cleanup,
+    .link_status_changed = aw_emac_set_link,
+};
+
+static void aw_emac_init(Object *obj)
+{
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+    AwEmacState *s = AW_EMAC(obj);
+
+    memory_region_init_io(&s->iomem, OBJECT(s), &aw_emac_mem_ops, s,
+                          "aw_emac", 0x1000);
+    sysbus_init_mmio(sbd, &s->iomem);
+    sysbus_init_irq(sbd, &s->irq);
+}
+
+static void aw_emac_realize(DeviceState *dev, Error **errp)
+{
+    AwEmacState *s = AW_EMAC(dev);
+
+    qemu_macaddr_default_if_unset(&s->conf.macaddr);
+
+    s->nic = qemu_new_nic(&net_aw_emac_info, &s->conf,
+                          object_get_typename(OBJECT(dev)), dev->id, s);
+    qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
+
+    aw_emac_reset(s);
+    aw_emac_set_link(qemu_get_queue(s->nic));
+    mii_reset(&s->mii);
+}
+
+static Property aw_emac_properties[] = {
+    DEFINE_NIC_PROPERTIES(AwEmacState, conf),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static const VMStateDescription vmstate_mii = {
+    .name = "allwinner_emac_mii",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT16(bmcr, AwEmacMii),
+        VMSTATE_UINT16(bmsr, AwEmacMii),
+        VMSTATE_UINT16(anar, AwEmacMii),
+        VMSTATE_UINT16(anlpar, AwEmacMii),
+        VMSTATE_BOOL(link_ok, AwEmacMii),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static const VMStateDescription vmstate_tx_fifo = {
+    .name = "allwinner_emac_tx_fifo",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_ARRAY(data, AwEmacTxFifo, FIFO_SIZE >> 2),
+        VMSTATE_INT32(length, AwEmacTxFifo),
+        VMSTATE_INT32(offset, AwEmacTxFifo),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static const VMStateDescription vmstate_aw_emac = {
+    .name = "allwinner_emac",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_STRUCT(mii, AwEmacState, 1, vmstate_mii, AwEmacMii),
+        VMSTATE_UINT32(ctl, AwEmacState),
+        VMSTATE_UINT32(tx_mode, AwEmacState),
+        VMSTATE_UINT32(rx_ctl, AwEmacState),
+        VMSTATE_UINT32(int_ctl, AwEmacState),
+        VMSTATE_UINT32(int_sta, AwEmacState),
+        VMSTATE_UINT32(phy_target, AwEmacState),
+        VMSTATE_INT32(tx_channel, AwEmacState),
+        VMSTATE_STRUCT_ARRAY(tx_fifos, AwEmacState,
+                             NUM_CHAN, 1, vmstate_tx_fifo, AwEmacTxFifo),
+        VMSTATE_BUFFER_UNSAFE(tx_fifos, AwEmacState, 1, MAX_RX * FIFO_SIZE),
+        VMSTATE_INT32(first_rx, AwEmacState),
+        VMSTATE_INT32(num_rx, AwEmacState),
+        VMSTATE_INT32(rx_offset, AwEmacState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void aw_emac_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->realize = aw_emac_realize;
+    dc->props = aw_emac_properties;
+    dc->vmsd = &vmstate_aw_emac;
+}
+
+static const TypeInfo aw_emac_info = {
+    .name           = TYPE_AW_EMAC,
+    .parent         = TYPE_SYS_BUS_DEVICE,
+    .instance_size  = sizeof(AwEmacState),
+    .instance_init   = aw_emac_init,
+    .class_init     = aw_emac_class_init,
+};
+
+static void aw_emac_register_types(void)
+{
+    type_register_static(&aw_emac_info);
+}
+
+type_init(aw_emac_register_types)
diff --git a/include/hw/net/allwinner_emac.h b/include/hw/net/allwinner_emac.h
new file mode 100644
index 0000000..f9f9682
--- /dev/null
+++ b/include/hw/net/allwinner_emac.h
@@ -0,0 +1,204 @@
+/*
+ * Emulation of Allwinner EMAC Fast Ethernet controller and
+ * Realtek RTL8201CP PHY
+ *
+ * Copyright (C) 2013 Beniamino Galvani <b.galvani@gmail.com>
+ *
+ * Allwinner EMAC register definitions from Linux kernel are:
+ *   Copyright 2012 Stefan Roese <sr@denx.de>
+ *   Copyright 2013 Maxime Ripard <maxime.ripard@free-electrons.com>
+ *   Copyright 1997 Sten Wang
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+#ifndef AW_EMAC_H
+#define AW_EMAC_H
+
+#include "net/net.h"
+
+#define TYPE_AW_EMAC "allwinner_emac"
+#define AW_EMAC(obj) OBJECT_CHECK(AwEmacState, (obj), TYPE_AW_EMAC)
+
+/*
+ * Allwinner EMAC register list
+ */
+#define EMAC_CTL_REG            0x00
+#define EMAC_TX_MODE_REG        0x04
+#define EMAC_TX_FLOW_REG        0x08
+#define EMAC_TX_CTL0_REG        0x0C
+#define EMAC_TX_CTL1_REG        0x10
+#define EMAC_TX_INS_REG         0x14
+#define EMAC_TX_PL0_REG         0x18
+#define EMAC_TX_PL1_REG         0x1C
+#define EMAC_TX_STA_REG         0x20
+#define EMAC_TX_IO_DATA_REG     0x24
+#define EMAC_TX_IO_DATA1_REG    0x28
+#define EMAC_TX_TSVL0_REG       0x2C
+#define EMAC_TX_TSVH0_REG       0x30
+#define EMAC_TX_TSVL1_REG       0x34
+#define EMAC_TX_TSVH1_REG       0x38
+#define EMAC_RX_CTL_REG         0x3C
+#define EMAC_RX_HASH0_REG       0x40
+#define EMAC_RX_HASH1_REG       0x44
+#define EMAC_RX_STA_REG         0x48
+#define EMAC_RX_IO_DATA_REG     0x4C
+#define EMAC_RX_FBC_REG         0x50
+#define EMAC_INT_CTL_REG        0x54
+#define EMAC_INT_STA_REG        0x58
+#define EMAC_MAC_CTL0_REG       0x5C
+#define EMAC_MAC_CTL1_REG       0x60
+#define EMAC_MAC_IPGT_REG       0x64
+#define EMAC_MAC_IPGR_REG       0x68
+#define EMAC_MAC_CLRT_REG       0x6C
+#define EMAC_MAC_MAXF_REG       0x70
+#define EMAC_MAC_SUPP_REG       0x74
+#define EMAC_MAC_TEST_REG       0x78
+#define EMAC_MAC_MCFG_REG       0x7C
+#define EMAC_MAC_MCMD_REG       0x80
+#define EMAC_MAC_MADR_REG       0x84
+#define EMAC_MAC_MWTD_REG       0x88
+#define EMAC_MAC_MRDD_REG       0x8C
+#define EMAC_MAC_MIND_REG       0x90
+#define EMAC_MAC_SSRR_REG       0x94
+#define EMAC_MAC_A0_REG         0x98
+#define EMAC_MAC_A1_REG         0x9C
+#define EMAC_MAC_A2_REG         0xA0
+#define EMAC_SAFX_L_REG0        0xA4
+#define EMAC_SAFX_H_REG0        0xA8
+#define EMAC_SAFX_L_REG1        0xAC
+#define EMAC_SAFX_H_REG1        0xB0
+#define EMAC_SAFX_L_REG2        0xB4
+#define EMAC_SAFX_H_REG2        0xB8
+#define EMAC_SAFX_L_REG3        0xBC
+#define EMAC_SAFX_H_REG3        0xC0
+
+/* CTL register fields */
+#define EMAC_CTL_RESET                  (1 << 0)
+#define EMAC_CTL_TX_EN                  (1 << 1)
+#define EMAC_CTL_RX_EN                  (1 << 2)
+
+/* TX MODE register fields */
+#define EMAC_TX_MODE_ABORTED_FRAME_EN   (1 << 0)
+#define EMAC_TX_MODE_DMA_EN             (1 << 1)
+
+/* RX CTL register fields */
+#define EMAC_RX_CTL_AUTO_DRQ_EN         (1 << 1)
+#define EMAC_RX_CTL_DMA_EN              (1 << 2)
+#define EMAC_RX_CTL_PASS_ALL_EN         (1 << 4)
+#define EMAC_RX_CTL_PASS_CTL_EN         (1 << 5)
+#define EMAC_RX_CTL_PASS_CRC_ERR_EN     (1 << 6)
+#define EMAC_RX_CTL_PASS_LEN_ERR_EN     (1 << 7)
+#define EMAC_RX_CTL_PASS_LEN_OOR_EN     (1 << 8)
+#define EMAC_RX_CTL_ACCEPT_UNICAST_EN   (1 << 16)
+#define EMAC_RX_CTL_DA_FILTER_EN        (1 << 17)
+#define EMAC_RX_CTL_ACCEPT_MULTICAST_EN (1 << 20)
+#define EMAC_RX_CTL_HASH_FILTER_EN      (1 << 21)
+#define EMAC_RX_CTL_ACCEPT_BROADCAST_EN (1 << 22)
+#define EMAC_RX_CTL_SA_FILTER_EN        (1 << 24)
+#define EMAC_RX_CTL_SA_FILTER_INVERT_EN (1 << 25)
+
+/* RX IO DATA register fields */
+#define EMAC_RX_IO_DATA_LEN(x)          (x & 0xffff)
+#define EMAC_RX_IO_DATA_STATUS(x)       ((x >> 16) & 0xffff)
+#define EMAC_RX_HEADER(len, status)     (((len) & 0xffff) | ((status) << 16))
+#define EMAC_RX_IO_DATA_STATUS_CRC_ERR  (1 << 4)
+#define EMAC_RX_IO_DATA_STATUS_LEN_ERR  (3 << 5)
+#define EMAC_RX_IO_DATA_STATUS_OK       (1 << 7)
+#define EMAC_UNDOCUMENTED_MAGIC         0x0143414d  /* header for RX frames */
+
+/* PHY registers */
+#define MII_BMCR            0
+#define MII_BMSR            1
+#define MII_PHYID1          2
+#define MII_PHYID2          3
+#define MII_ANAR            4
+#define MII_ANLPAR          5
+
+/* PHY registers fields */
+#define MII_BMCR_RESET      (1 << 15)
+#define MII_BMCR_LOOPBACK   (1 << 14)
+#define MII_BMCR_SPEED      (1 << 13)
+#define MII_BMCR_AUTOEN     (1 << 12)
+#define MII_BMCR_FD         (1 << 8)
+
+#define MII_BMSR_100TX_FD   (1 << 14)
+#define MII_BMSR_100TX_HD   (1 << 13)
+#define MII_BMSR_10T_FD     (1 << 12)
+#define MII_BMSR_10T_HD     (1 << 11)
+#define MII_BMSR_MFPS       (1 << 6)
+#define MII_BMSR_AUTONEG    (1 << 3)
+#define MII_BMSR_LINK_ST    (1 << 2)
+
+#define MII_ANAR_TXFD       (1 << 8)
+#define MII_ANAR_TX         (1 << 7)
+#define MII_ANAR_10FD       (1 << 6)
+#define MII_ANAR_10         (1 << 5)
+#define MII_ANAR_CSMACD     (1 << 0)
+
+#define RTL8201CP_PHYID1    0x0000
+#define RTL8201CP_PHYID2    0x8201
+
+/* INT CTL and INT STA registers fields */
+#define EMAC_INT_TX_CHAN(x) (1 << (x))
+#define EMAC_INT_RX         (1 << 8)
+
+#define BOARD_PHY_ADDRESS   1
+
+#define NUM_CHAN            2
+#define FIFO_SIZE           2048
+#define MAX_RX              12
+#define RX_HDR_SIZE         8
+
+#define PHY_TARGET_ADDR(x)  (((x) >> 8) & 0xff)
+#define PHY_TARGET_REG(x)   ((x) & 0xff)
+
+typedef struct AwEmacTxFifo {
+    uint32_t data[FIFO_SIZE >> 2];
+    int      length;
+    int      offset;
+} AwEmacTxFifo;
+
+typedef struct AwEmacMii {
+    uint16_t bmcr;
+    uint16_t bmsr;
+    uint16_t anar;
+    uint16_t anlpar;
+    bool     link_ok;
+} AwEmacMii;
+
+typedef struct AwEmacState {
+    /*< private >*/
+    SysBusDevice  parent_obj;
+    /*< public >*/
+
+    MemoryRegion  iomem;
+    qemu_irq      irq;
+    NICState      *nic;
+    NICConf       conf;
+    AwEmacMii     mii;
+
+    uint32_t      ctl;
+    uint32_t      tx_mode;
+    uint32_t      rx_ctl;
+    uint32_t      int_ctl;
+    uint32_t      int_sta;
+    uint32_t      phy_target;
+
+    int           tx_channel;             /* Currently selected TX channel */
+    AwEmacTxFifo  tx_fifos[NUM_CHAN];
+    uint32_t      rx_fifos[MAX_RX][FIFO_SIZE >> 2];
+    int           first_rx;               /* First packet in the RX ring */
+    int           num_rx;                 /* Number of packets in RX ring */
+    int           rx_offset;              /* Offset of next read */
+} AwEmacState;
+
+#endif
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller

[Peter Crosthwaite]

Hi Beniamino,

On Thu, Jan 2, 2014 at 7:18 PM, Beniamino Galvani <b.galvani@gmail.com> wrote:
> This patch adds support for the Fast Ethernet MAC found on Allwinner
> SoCs, together with a basic emulation of Realtek RTL8201CP PHY.
>

More a comment for net in general, but I think sooner or later we need
to move towards a split between phy and mac on the device level.
continuing the phy-within-mac philosophy is going to make the
socification efforts awkward. Are MII and friends a busses (as in
TYPE_BUS) in their own right, and connection of mac and phy has to
happen on the board level?

> Since there is no public documentation of the Allwinner controller, the
> implementation is based on Linux kernel driver.
>
> Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
> ---
>  default-configs/arm-softmmu.mak |    1 +
>  hw/net/Makefile.objs            |    1 +
>  hw/net/allwinner_emac.c         |  466 +++++++++++++++++++++++++++++++++++++++
>  include/hw/net/allwinner_emac.h |  204 +++++++++++++++++
>  4 files changed, 672 insertions(+)
>  create mode 100644 hw/net/allwinner_emac.c
>  create mode 100644 include/hw/net/allwinner_emac.h
>
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index ce1d620..f3513fa 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -27,6 +27,7 @@ CONFIG_SSI_SD=y
>  CONFIG_SSI_M25P80=y
>  CONFIG_LAN9118=y
>  CONFIG_SMC91C111=y
> +CONFIG_ALLWINNER_EMAC=y
>  CONFIG_DS1338=y
>  CONFIG_PFLASH_CFI01=y
>  CONFIG_PFLASH_CFI02=y
> diff --git a/hw/net/Makefile.objs b/hw/net/Makefile.objs
> index 951cca3..75e80c2 100644
> --- a/hw/net/Makefile.objs
> +++ b/hw/net/Makefile.objs
> @@ -18,6 +18,7 @@ common-obj-$(CONFIG_OPENCORES_ETH) += opencores_eth.o
>  common-obj-$(CONFIG_XGMAC) += xgmac.o
>  common-obj-$(CONFIG_MIPSNET) += mipsnet.o
>  common-obj-$(CONFIG_XILINX_AXI) += xilinx_axienet.o
> +common-obj-$(CONFIG_ALLWINNER_EMAC) += allwinner_emac.o
>
>  common-obj-$(CONFIG_CADENCE) += cadence_gem.o
>  common-obj-$(CONFIG_STELLARIS_ENET) += stellaris_enet.o
> diff --git a/hw/net/allwinner_emac.c b/hw/net/allwinner_emac.c
> new file mode 100644
> index 0000000..9791be4
> --- /dev/null
> +++ b/hw/net/allwinner_emac.c
> @@ -0,0 +1,466 @@
> +/*
> + * Emulation of Allwinner EMAC Fast Ethernet controller and
> + * Realtek RTL8201CP PHY
> + *
> + * Copyright (C) 2013 Beniamino Galvani <b.galvani@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +#include "hw/sysbus.h"
> +#include "net/net.h"
> +#include "hw/net/allwinner_emac.h"
> +#include <zlib.h>
> +
> +#undef AW_EMAC_DEBUG
> +
> +#ifdef AW_EMAC_DEBUG
> +#define debug(...)                                              \
> +    do {                                                        \
> +        fprintf(stderr,  "allwinner_emac : %s: ", __func__);    \
> +        fprintf(stderr, ## __VA_ARGS__);                        \
> +    } while (0)
> +#else
> +#define debug(...) do {} while (0)
> +#endif

For debug macros, its better to use a regular if (DEBUG_SYMBOL) so the
body of the macro always gets compile tested. We have had incidences
where major change patterns break stripped debug instrumentation
because the code is compiled out.

> +
> +static void mii_set_link(AwEmacMii *mii, bool link_ok)
> +{
> +    if (link_ok) {
> +        mii->bmsr |= MII_BMSR_LINK_ST;
> +        mii->anlpar |= MII_ANAR_TXFD | MII_ANAR_TX | MII_ANAR_10FD |
> +                       MII_ANAR_10 | MII_ANAR_CSMACD;
> +    } else {
> +        mii->bmsr &= ~MII_BMSR_LINK_ST;
> +        mii->anlpar = MII_ANAR_TX;
> +    }
> +    mii->link_ok = link_ok;
> +}
> +
> +static void mii_reset(AwEmacMii *mii)
> +{
> +    mii->bmcr = MII_BMCR_FD | MII_BMCR_AUTOEN | MII_BMCR_SPEED;
> +    mii->bmsr = MII_BMSR_100TX_FD | MII_BMSR_100TX_HD | MII_BMSR_10T_FD |
> +                MII_BMSR_10T_HD | MII_BMSR_MFPS | MII_BMSR_AUTONEG;
> +    mii->anar = MII_ANAR_TXFD | MII_ANAR_TX | MII_ANAR_10FD | MII_ANAR_10 |
> +                MII_ANAR_CSMACD;
> +    mii->anlpar = MII_ANAR_TX;
> +    mii_set_link(mii, mii->link_ok);
> +}
> +
> +static uint16_t mii_read(AwEmacMii *mii, uint8_t phy_addr, uint8_t reg)
> +{
> +    uint16_t ret = 0xffff;
> +
> +    if (phy_addr == BOARD_PHY_ADDRESS) {
> +        switch (reg) {
> +        case MII_BMCR:
> +            ret = mii->bmcr;
> +            break;
> +        case MII_BMSR:
> +            ret = mii->bmsr;
> +            break;
> +        case MII_PHYID1:
> +            ret = RTL8201CP_PHYID1;
> +            break;
> +        case MII_PHYID2:
> +            ret = RTL8201CP_PHYID2;
> +            break;
> +        case MII_ANAR:
> +            ret = mii->anar;
> +            break;
> +        case MII_ANLPAR:
> +            ret = mii->anlpar;
> +            break;
> +        default:
> +            debug("unknown mii register %x\n", reg);
> +            ret = 0;
> +        }
> +    }
> +    return ret;
> +}
> +
> +static void mii_write(AwEmacMii *mii, uint8_t phy_addr, uint8_t reg,
> +                      uint16_t value)
> +{
> +    if (phy_addr == BOARD_PHY_ADDRESS) {
> +        switch (reg) {
> +        case MII_BMCR:
> +            if (value & MII_BMCR_RESET) {
> +                mii_reset(mii);
> +            } else {
> +                mii->bmcr = value;
> +            }
> +            break;
> +        case MII_BMSR:
> +        case MII_PHYID1:
> +        case MII_PHYID2:
> +        case MII_ANLPAR:
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: write to mii register %x\n",
> +                          __func__, reg);
> +            break;
> +        case MII_ANAR:
> +            mii->anar = value;
> +            break;
> +        default:
> +            debug("unknown mii register %x\n", reg);
> +        }
> +    }
> +}
> +
> +static void aw_emac_update_irq(AwEmacState *s)
> +{
> +    qemu_set_irq(s->irq, (s->int_sta & s->int_ctl) != 0);
> +}
> +
> +static int aw_emac_can_receive(NetClientState *nc)
> +{
> +    AwEmacState *s = qemu_get_nic_opaque(nc);
> +
> +    return (s->ctl & EMAC_CTL_RX_EN) && (s->num_rx < MAX_RX);

If you return false from a can_recieve(), you need to explictly flush
queued packets (qemu_flush_queued_packets()) when the blocking
condition is lifted, heres a good example a bugfix patch addressing
this issue for another mac:

http://lists.gnu.org/archive/html/qemu-devel/2013-11/msg02255.html

> +}
> +
> +static ssize_t aw_emac_receive(NetClientState *nc, const uint8_t *buf,
> +                               size_t size)
> +{
> +    AwEmacState *s = qemu_get_nic_opaque(nc);
> +    uint32_t *fifo;
> +    uint32_t crc;
> +    char *dest;
> +
> +    if (s->num_rx >= MAX_RX) {
> +        debug("rx queue full - packed dropped\n");
> +        return -1;
> +    }
> +
> +    if (size + RX_HDR_SIZE > FIFO_SIZE) {
> +        debug("packet too big\n");
> +        return -1;
> +    }
> +
> +    fifo = s->rx_fifos[(s->first_rx + s->num_rx) % MAX_RX];
> +    dest = (char *)&fifo[2];
> +    s->num_rx++;
> +
> +    memcpy(dest, buf, size);
> +
> +    /* Fill to minimum frame length */
> +    if (size < 60) {
> +        memset(dest + size, 0, 60 - size);
> +        size = 60;
> +    }
> +
> +    /* Append FCS */
> +    crc = crc32(~0, buf, size);
> +    memcpy(dest + size, &crc, 4);
> +
> +    fifo[0] = EMAC_UNDOCUMENTED_MAGIC;
> +    fifo[1] = EMAC_RX_HEADER(size + 4, EMAC_RX_IO_DATA_STATUS_OK);
> +
> +    /* Set rx interrupt flag */
> +    s->int_sta |= EMAC_INT_RX;
> +    aw_emac_update_irq(s);
> +
> +    return size;
> +}
> +
> +static void aw_emac_cleanup(NetClientState *nc)
> +{
> +    AwEmacState *s = qemu_get_nic_opaque(nc);
> +
> +    s->nic = NULL;
> +}
> +
> +static void aw_emac_reset(AwEmacState *s)
> +{
> +    s->ctl = 0;
> +    s->tx_mode = 0;
> +    s->int_ctl = 0;
> +    s->int_sta = 0;
> +    s->tx_channel = 0;
> +    s->first_rx = 0;
> +    s->num_rx = 0;
> +    s->rx_offset = 0;
> +    s->phy_target = 0;
> +}
> +
> +static uint64_t aw_emac_read(void *opaque, hwaddr offset, unsigned size)
> +{
> +    AwEmacState *s = opaque;
> +    uint64_t ret;
> +    uint32_t *rx_fifo;
> +
> +    switch (offset) {
> +    case EMAC_CTL_REG:
> +        ret = s->ctl;
> +        break;
> +    case EMAC_TX_MODE_REG:
> +        ret = s->tx_mode;
> +        break;
> +    case EMAC_TX_INS_REG:
> +        ret = s->tx_channel;
> +        break;
> +    case EMAC_RX_CTL_REG:
> +        ret = s->rx_ctl;
> +        break;
> +    case EMAC_RX_IO_DATA_REG:
> +        if (!s->num_rx) {
> +            ret = 0;
> +            break;
> +        }
> +        rx_fifo = s->rx_fifos[s->first_rx];
> +
> +        if (s->rx_offset >= FIFO_SIZE ||
> +            s->rx_offset >= EMAC_RX_IO_DATA_LEN(rx_fifo[1]) + RX_HDR_SIZE) {
> +            /* This should never happen */

Why? If its impossible via your implementation logic then you should
assert. Its a misbehaving guest then you should
qemu_log_mask(LOG_GUEST_ERROR

> +            debug("RX fifo overrun\n");
> +            s->first_rx = (s->first_rx + 1) % MAX_RX;
> +            s->num_rx--;
> +            s->rx_offset = 0;
> +            ret = 0;
> +            break;
> +        }
> +
> +        ret = rx_fifo[s->rx_offset >> 2];
> +        s->rx_offset += 4;
> +
> +        if (s->rx_offset >= EMAC_RX_IO_DATA_LEN(rx_fifo[1]) + RX_HDR_SIZE) {
> +            s->first_rx = (s->first_rx + 1) % MAX_RX;
> +            s->num_rx--;
> +            s->rx_offset = 0;
> +        }
> +        break;
> +    case EMAC_RX_FBC_REG:
> +        ret = s->num_rx;
> +        break;
> +    case EMAC_INT_CTL_REG:
> +        ret = s->int_ctl;
> +        break;
> +    case EMAC_INT_STA_REG:
> +        ret = s->int_sta;
> +        break;
> +    case EMAC_MAC_MRDD_REG:
> +        ret = mii_read(&s->mii, PHY_TARGET_ADDR(s->phy_target),
> +                       PHY_TARGET_REG(s->phy_target));
> +        break;
> +    default:
> +        debug("unknown offset %08x\n", (unsigned int)offset);

qemu_log_mask(LOG_UNIMP

I'm thinking you should UNIMP rather than the usual GUEST_ERROR as you
have no specs to work on (same problem as the recent Digic series).

> +        ret = 0;
> +    }
> +
> +    return ret;
> +}
> +
> +static void aw_emac_write(void *opaque, hwaddr offset, uint64_t value,
> +                          unsigned size)
> +{
> +    AwEmacState *s = opaque;
> +    AwEmacTxFifo *fifo;
> +    int chan;
> +
> +    switch (offset) {
> +    case EMAC_CTL_REG:
> +        if (value & EMAC_CTL_RESET) {
> +            debug("reset\n");
> +            aw_emac_reset(s);
> +            value &= ~EMAC_CTL_RESET;
> +        }
> +        s->ctl = value;

This is one example of a place where you may need to flush queued packets.

> +        break;
> +    case EMAC_TX_MODE_REG:
> +        s->tx_mode = value;
> +        break;
> +    case EMAC_TX_CTL0_REG:
> +    case EMAC_TX_CTL1_REG:
> +        chan = (offset == EMAC_TX_CTL0_REG ? 0 : 1);
> +        if ((value & 1) && (s->ctl & EMAC_CTL_TX_EN)) {
> +            qemu_send_packet(qemu_get_queue(s->nic),
> +                             (uint8_t *)s->tx_fifos[chan].data,
> +                             s->tx_fifos[chan].length);
> +
> +            /* Raise TX interrupt */
> +            s->int_sta |= EMAC_INT_TX_CHAN(chan);
> +            s->tx_fifos[chan].offset = 0;
> +            aw_emac_update_irq(s);
> +        }
> +        break;
> +    case EMAC_TX_INS_REG:
> +        s->tx_channel = value < NUM_CHAN ? value : 0;
> +        break;
> +    case EMAC_TX_PL0_REG:
> +    case EMAC_TX_PL1_REG:
> +        chan = (offset == EMAC_TX_PL0_REG ? 0 : 1);
> +        if (value > FIFO_SIZE) {
> +            debug("invalid TX frame length %d\n", (int)value);

assert or GUEST_ERROR - any debug errory type messages should be one
or the other depending on root cause. If caused by bad sw GUEST_ERROR.
If a bug in this device model code, assert(false).

> +            value = FIFO_SIZE;
> +        }
> +        s->tx_fifos[chan].length = value;
> +        break;
> +    case EMAC_TX_IO_DATA_REG:
> +        fifo = &s->tx_fifos[s->tx_channel];
> +        if (fifo->offset >= FIFO_SIZE) {
> +            debug("TX frame data overruns fifo (%d)\n", fifo->offset);
> +            break;
> +        }
> +        fifo->data[fifo->offset >> 2] = value;
> +        fifo->offset += 4;
> +        break;
> +    case EMAC_RX_CTL_REG:
> +        s->rx_ctl = value;
> +        break;
> +    case EMAC_RX_FBC_REG:
> +        if (value == 0) {
> +            s->num_rx = 0;
> +        }
> +        break;
> +    case EMAC_INT_CTL_REG:
> +        s->int_ctl = value;
> +        break;
> +    case EMAC_INT_STA_REG:
> +        s->int_sta &= ~value;
> +        break;
> +    case EMAC_MAC_MADR_REG:
> +        s->phy_target = value;
> +        break;
> +    case EMAC_MAC_MWTD_REG:
> +        mii_write(&s->mii, PHY_TARGET_ADDR(s->phy_target),
> +                  PHY_TARGET_REG(s->phy_target), value);
> +        break;
> +    default:
> +        debug("unknown offset %08x\n", (unsigned int)offset);

LOG_UNIMP

> +    }
> +}
> +
> +static void aw_emac_set_link(NetClientState *nc)
> +{
> +    AwEmacState *s = qemu_get_nic_opaque(nc);
> +
> +    mii_set_link(&s->mii, !nc->link_down);
> +}
> +
> +static const MemoryRegionOps aw_emac_mem_ops = {
> +    .read = aw_emac_read,
> +    .write = aw_emac_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,

Does your linux driver ever do sub-word accesses? If not you could set
the appropriate restrictions to access size/alignment here for
defensiveness.

> +};
> +
> +static NetClientInfo net_aw_emac_info = {
> +    .type = NET_CLIENT_OPTIONS_KIND_NIC,
> +    .size = sizeof(NICState),
> +    .can_receive = aw_emac_can_receive,
> +    .receive = aw_emac_receive,
> +    .cleanup = aw_emac_cleanup,
> +    .link_status_changed = aw_emac_set_link,
> +};
> +
> +static void aw_emac_init(Object *obj)
> +{
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +    AwEmacState *s = AW_EMAC(obj);
> +
> +    memory_region_init_io(&s->iomem, OBJECT(s), &aw_emac_mem_ops, s,
> +                          "aw_emac", 0x1000);
> +    sysbus_init_mmio(sbd, &s->iomem);
> +    sysbus_init_irq(sbd, &s->irq);
> +}
> +
> +static void aw_emac_realize(DeviceState *dev, Error **errp)
> +{
> +    AwEmacState *s = AW_EMAC(dev);
> +
> +    qemu_macaddr_default_if_unset(&s->conf.macaddr);
> +
> +    s->nic = qemu_new_nic(&net_aw_emac_info, &s->conf,
> +                          object_get_typename(OBJECT(dev)), dev->id, s);
> +    qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
> +
> +    aw_emac_reset(s);
> +    aw_emac_set_link(qemu_get_queue(s->nic));
> +    mii_reset(&s->mii);
> +}
> +
> +static Property aw_emac_properties[] = {
> +    DEFINE_NIC_PROPERTIES(AwEmacState, conf),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static const VMStateDescription vmstate_mii = {
> +    .name = "allwinner_emac_mii",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT16(bmcr, AwEmacMii),
> +        VMSTATE_UINT16(bmsr, AwEmacMii),
> +        VMSTATE_UINT16(anar, AwEmacMii),
> +        VMSTATE_UINT16(anlpar, AwEmacMii),
> +        VMSTATE_BOOL(link_ok, AwEmacMii),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static const VMStateDescription vmstate_tx_fifo = {
> +    .name = "allwinner_emac_tx_fifo",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32_ARRAY(data, AwEmacTxFifo, FIFO_SIZE >> 2),
> +        VMSTATE_INT32(length, AwEmacTxFifo),
> +        VMSTATE_INT32(offset, AwEmacTxFifo),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};

This might warrant a dup of fifo8 as fifo32 if you want to clean this
up. I think I have a patch handy that might help, will send tmrw.
Check util/fifo8.c for fifo8 example.

> +
> +static const VMStateDescription vmstate_aw_emac = {
> +    .name = "allwinner_emac",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_STRUCT(mii, AwEmacState, 1, vmstate_mii, AwEmacMii),
> +        VMSTATE_UINT32(ctl, AwEmacState),
> +        VMSTATE_UINT32(tx_mode, AwEmacState),
> +        VMSTATE_UINT32(rx_ctl, AwEmacState),
> +        VMSTATE_UINT32(int_ctl, AwEmacState),
> +        VMSTATE_UINT32(int_sta, AwEmacState),
> +        VMSTATE_UINT32(phy_target, AwEmacState),
> +        VMSTATE_INT32(tx_channel, AwEmacState),
> +        VMSTATE_STRUCT_ARRAY(tx_fifos, AwEmacState,
> +                             NUM_CHAN, 1, vmstate_tx_fifo, AwEmacTxFifo),
> +        VMSTATE_BUFFER_UNSAFE(tx_fifos, AwEmacState, 1, MAX_RX * FIFO_SIZE),
> +        VMSTATE_INT32(first_rx, AwEmacState),
> +        VMSTATE_INT32(num_rx, AwEmacState),
> +        VMSTATE_INT32(rx_offset, AwEmacState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void aw_emac_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = aw_emac_realize;
> +    dc->props = aw_emac_properties;
> +    dc->vmsd = &vmstate_aw_emac;
> +}
> +
> +static const TypeInfo aw_emac_info = {
> +    .name           = TYPE_AW_EMAC,
> +    .parent         = TYPE_SYS_BUS_DEVICE,
> +    .instance_size  = sizeof(AwEmacState),
> +    .instance_init   = aw_emac_init,
> +    .class_init     = aw_emac_class_init,
> +};
> +
> +static void aw_emac_register_types(void)
> +{
> +    type_register_static(&aw_emac_info);
> +}
> +
> +type_init(aw_emac_register_types)
> diff --git a/include/hw/net/allwinner_emac.h b/include/hw/net/allwinner_emac.h
> new file mode 100644
> index 0000000..f9f9682
> --- /dev/null
> +++ b/include/hw/net/allwinner_emac.h
> @@ -0,0 +1,204 @@
> +/*
> + * Emulation of Allwinner EMAC Fast Ethernet controller and
> + * Realtek RTL8201CP PHY
> + *
> + * Copyright (C) 2013 Beniamino Galvani <b.galvani@gmail.com>
> + *
> + * Allwinner EMAC register definitions from Linux kernel are:
> + *   Copyright 2012 Stefan Roese <sr@denx.de>
> + *   Copyright 2013 Maxime Ripard <maxime.ripard@free-electrons.com>
> + *   Copyright 1997 Sten Wang
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +#ifndef AW_EMAC_H
> +#define AW_EMAC_H
> +
> +#include "net/net.h"
> +
> +#define TYPE_AW_EMAC "allwinner_emac"
> +#define AW_EMAC(obj) OBJECT_CHECK(AwEmacState, (obj), TYPE_AW_EMAC)
> +
> +/*
> + * Allwinner EMAC register list
> + */
> +#define EMAC_CTL_REG            0x00
> +#define EMAC_TX_MODE_REG        0x04
> +#define EMAC_TX_FLOW_REG        0x08
> +#define EMAC_TX_CTL0_REG        0x0C
> +#define EMAC_TX_CTL1_REG        0x10
> +#define EMAC_TX_INS_REG         0x14
> +#define EMAC_TX_PL0_REG         0x18
> +#define EMAC_TX_PL1_REG         0x1C
> +#define EMAC_TX_STA_REG         0x20
> +#define EMAC_TX_IO_DATA_REG     0x24
> +#define EMAC_TX_IO_DATA1_REG    0x28
> +#define EMAC_TX_TSVL0_REG       0x2C
> +#define EMAC_TX_TSVH0_REG       0x30
> +#define EMAC_TX_TSVL1_REG       0x34
> +#define EMAC_TX_TSVH1_REG       0x38
> +#define EMAC_RX_CTL_REG         0x3C
> +#define EMAC_RX_HASH0_REG       0x40
> +#define EMAC_RX_HASH1_REG       0x44
> +#define EMAC_RX_STA_REG         0x48
> +#define EMAC_RX_IO_DATA_REG     0x4C
> +#define EMAC_RX_FBC_REG         0x50
> +#define EMAC_INT_CTL_REG        0x54
> +#define EMAC_INT_STA_REG        0x58
> +#define EMAC_MAC_CTL0_REG       0x5C
> +#define EMAC_MAC_CTL1_REG       0x60
> +#define EMAC_MAC_IPGT_REG       0x64
> +#define EMAC_MAC_IPGR_REG       0x68
> +#define EMAC_MAC_CLRT_REG       0x6C
> +#define EMAC_MAC_MAXF_REG       0x70
> +#define EMAC_MAC_SUPP_REG       0x74
> +#define EMAC_MAC_TEST_REG       0x78
> +#define EMAC_MAC_MCFG_REG       0x7C
> +#define EMAC_MAC_MCMD_REG       0x80
> +#define EMAC_MAC_MADR_REG       0x84
> +#define EMAC_MAC_MWTD_REG       0x88
> +#define EMAC_MAC_MRDD_REG       0x8C
> +#define EMAC_MAC_MIND_REG       0x90
> +#define EMAC_MAC_SSRR_REG       0x94
> +#define EMAC_MAC_A0_REG         0x98
> +#define EMAC_MAC_A1_REG         0x9C
> +#define EMAC_MAC_A2_REG         0xA0
> +#define EMAC_SAFX_L_REG0        0xA4
> +#define EMAC_SAFX_H_REG0        0xA8
> +#define EMAC_SAFX_L_REG1        0xAC
> +#define EMAC_SAFX_H_REG1        0xB0
> +#define EMAC_SAFX_L_REG2        0xB4
> +#define EMAC_SAFX_H_REG2        0xB8
> +#define EMAC_SAFX_L_REG3        0xBC
> +#define EMAC_SAFX_H_REG3        0xC0
> +
> +/* CTL register fields */
> +#define EMAC_CTL_RESET                  (1 << 0)
> +#define EMAC_CTL_TX_EN                  (1 << 1)
> +#define EMAC_CTL_RX_EN                  (1 << 2)
> +
> +/* TX MODE register fields */
> +#define EMAC_TX_MODE_ABORTED_FRAME_EN   (1 << 0)
> +#define EMAC_TX_MODE_DMA_EN             (1 << 1)
> +
> +/* RX CTL register fields */
> +#define EMAC_RX_CTL_AUTO_DRQ_EN         (1 << 1)
> +#define EMAC_RX_CTL_DMA_EN              (1 << 2)
> +#define EMAC_RX_CTL_PASS_ALL_EN         (1 << 4)
> +#define EMAC_RX_CTL_PASS_CTL_EN         (1 << 5)
> +#define EMAC_RX_CTL_PASS_CRC_ERR_EN     (1 << 6)
> +#define EMAC_RX_CTL_PASS_LEN_ERR_EN     (1 << 7)
> +#define EMAC_RX_CTL_PASS_LEN_OOR_EN     (1 << 8)
> +#define EMAC_RX_CTL_ACCEPT_UNICAST_EN   (1 << 16)
> +#define EMAC_RX_CTL_DA_FILTER_EN        (1 << 17)
> +#define EMAC_RX_CTL_ACCEPT_MULTICAST_EN (1 << 20)
> +#define EMAC_RX_CTL_HASH_FILTER_EN      (1 << 21)
> +#define EMAC_RX_CTL_ACCEPT_BROADCAST_EN (1 << 22)
> +#define EMAC_RX_CTL_SA_FILTER_EN        (1 << 24)
> +#define EMAC_RX_CTL_SA_FILTER_INVERT_EN (1 << 25)
> +
> +/* RX IO DATA register fields */
> +#define EMAC_RX_IO_DATA_LEN(x)          (x & 0xffff)
> +#define EMAC_RX_IO_DATA_STATUS(x)       ((x >> 16) & 0xffff)

use extract32 rather than >> & logic. Although I find the macrofied
extractors a bit wierd. Usually only a shift and width are macroified
and the extraction process is done inline.

> +#define EMAC_RX_HEADER(len, status)     (((len) & 0xffff) | ((status) << 16))
> +#define EMAC_RX_IO_DATA_STATUS_CRC_ERR  (1 << 4)
> +#define EMAC_RX_IO_DATA_STATUS_LEN_ERR  (3 << 5)
> +#define EMAC_RX_IO_DATA_STATUS_OK       (1 << 7)
> +#define EMAC_UNDOCUMENTED_MAGIC         0x0143414d  /* header for RX frames */
> +
> +/* PHY registers */
> +#define MII_BMCR            0
> +#define MII_BMSR            1
> +#define MII_PHYID1          2
> +#define MII_PHYID2          3
> +#define MII_ANAR            4
> +#define MII_ANLPAR          5
> +
> +/* PHY registers fields */
> +#define MII_BMCR_RESET      (1 << 15)
> +#define MII_BMCR_LOOPBACK   (1 << 14)
> +#define MII_BMCR_SPEED      (1 << 13)
> +#define MII_BMCR_AUTOEN     (1 << 12)
> +#define MII_BMCR_FD         (1 << 8)
> +
> +#define MII_BMSR_100TX_FD   (1 << 14)
> +#define MII_BMSR_100TX_HD   (1 << 13)
> +#define MII_BMSR_10T_FD     (1 << 12)
> +#define MII_BMSR_10T_HD     (1 << 11)
> +#define MII_BMSR_MFPS       (1 << 6)
> +#define MII_BMSR_AUTONEG    (1 << 3)
> +#define MII_BMSR_LINK_ST    (1 << 2)
> +
> +#define MII_ANAR_TXFD       (1 << 8)
> +#define MII_ANAR_TX         (1 << 7)
> +#define MII_ANAR_10FD       (1 << 6)
> +#define MII_ANAR_10         (1 << 5)
> +#define MII_ANAR_CSMACD     (1 << 0)
> +
> +#define RTL8201CP_PHYID1    0x0000
> +#define RTL8201CP_PHYID2    0x8201
> +
> +/* INT CTL and INT STA registers fields */
> +#define EMAC_INT_TX_CHAN(x) (1 << (x))
> +#define EMAC_INT_RX         (1 << 8)
> +
> +#define BOARD_PHY_ADDRESS   1

This board level configurable?

> +
> +#define NUM_CHAN            2
> +#define FIFO_SIZE           2048
> +#define MAX_RX              12
> +#define RX_HDR_SIZE         8
> +
> +#define PHY_TARGET_ADDR(x)  (((x) >> 8) & 0xff)

extract32 (or 16?).

Regards,
Peter

> +#define PHY_TARGET_REG(x)   ((x) & 0xff)
> +
> +typedef struct AwEmacTxFifo {
> +    uint32_t data[FIFO_SIZE >> 2];
> +    int      length;
> +    int      offset;
> +} AwEmacTxFifo;
> +
> +typedef struct AwEmacMii {
> +    uint16_t bmcr;
> +    uint16_t bmsr;
> +    uint16_t anar;
> +    uint16_t anlpar;
> +    bool     link_ok;
> +} AwEmacMii;
> +
> +typedef struct AwEmacState {
> +    /*< private >*/
> +    SysBusDevice  parent_obj;
> +    /*< public >*/
> +
> +    MemoryRegion  iomem;
> +    qemu_irq      irq;
> +    NICState      *nic;
> +    NICConf       conf;
> +    AwEmacMii     mii;
> +
> +    uint32_t      ctl;
> +    uint32_t      tx_mode;
> +    uint32_t      rx_ctl;
> +    uint32_t      int_ctl;
> +    uint32_t      int_sta;
> +    uint32_t      phy_target;
> +
> +    int           tx_channel;             /* Currently selected TX channel */
> +    AwEmacTxFifo  tx_fifos[NUM_CHAN];
> +    uint32_t      rx_fifos[MAX_RX][FIFO_SIZE >> 2];
> +    int           first_rx;               /* First packet in the RX ring */
> +    int           num_rx;                 /* Number of packets in RX ring */
> +    int           rx_offset;              /* Offset of next read */
> +} AwEmacState;
> +
> +#endif
> --
> 1.7.10.4
>
>

Re: [Qemu-devel] [PATCH 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller

[Beniamino Galvani]

On Thu, Jan 02, 2014 at 08:25:10PM +1000, Peter Crosthwaite wrote:
> > +#undef AW_EMAC_DEBUG
> > +
> > +#ifdef AW_EMAC_DEBUG
> > +#define debug(...)                                              \
> > +    do {                                                        \
> > +        fprintf(stderr,  "allwinner_emac : %s: ", __func__);    \
> > +        fprintf(stderr, ## __VA_ARGS__);                        \
> > +    } while (0)
> > +#else
> > +#define debug(...) do {} while (0)
> > +#endif
> 
> For debug macros, its better to use a regular if (DEBUG_SYMBOL) so the
> body of the macro always gets compile tested. We have had incidences
> where major change patterns break stripped debug instrumentation
> because the code is compiled out.

Ok.

> 
> > +
> > +static void mii_set_link(AwEmacMii *mii, bool link_ok)
> > +{
> > +    if (link_ok) {
> > +        mii->bmsr |= MII_BMSR_LINK_ST;
> > +        mii->anlpar |= MII_ANAR_TXFD | MII_ANAR_TX | MII_ANAR_10FD |
> > +                       MII_ANAR_10 | MII_ANAR_CSMACD;
> > +    } else {
> > +        mii->bmsr &= ~MII_BMSR_LINK_ST;
> > +        mii->anlpar = MII_ANAR_TX;
> > +    }
> > +    mii->link_ok = link_ok;
> > +}
> > +
> > +static void mii_reset(AwEmacMii *mii)
> > +{
> > +    mii->bmcr = MII_BMCR_FD | MII_BMCR_AUTOEN | MII_BMCR_SPEED;
> > +    mii->bmsr = MII_BMSR_100TX_FD | MII_BMSR_100TX_HD | MII_BMSR_10T_FD |
> > +                MII_BMSR_10T_HD | MII_BMSR_MFPS | MII_BMSR_AUTONEG;
> > +    mii->anar = MII_ANAR_TXFD | MII_ANAR_TX | MII_ANAR_10FD | MII_ANAR_10 |
> > +                MII_ANAR_CSMACD;
> > +    mii->anlpar = MII_ANAR_TX;
> > +    mii_set_link(mii, mii->link_ok);
> > +}
> > +
> > +static uint16_t mii_read(AwEmacMii *mii, uint8_t phy_addr, uint8_t reg)
> > +{
> > +    uint16_t ret = 0xffff;
> > +
> > +    if (phy_addr == BOARD_PHY_ADDRESS) {
> > +        switch (reg) {
> > +        case MII_BMCR:
> > +            ret = mii->bmcr;
> > +            break;
> > +        case MII_BMSR:
> > +            ret = mii->bmsr;
> > +            break;
> > +        case MII_PHYID1:
> > +            ret = RTL8201CP_PHYID1;
> > +            break;
> > +        case MII_PHYID2:
> > +            ret = RTL8201CP_PHYID2;
> > +            break;
> > +        case MII_ANAR:
> > +            ret = mii->anar;
> > +            break;
> > +        case MII_ANLPAR:
> > +            ret = mii->anlpar;
> > +            break;
> > +        default:
> > +            debug("unknown mii register %x\n", reg);
> > +            ret = 0;
> > +        }
> > +    }
> > +    return ret;
> > +}
> > +
> > +static void mii_write(AwEmacMii *mii, uint8_t phy_addr, uint8_t reg,
> > +                      uint16_t value)
> > +{
> > +    if (phy_addr == BOARD_PHY_ADDRESS) {
> > +        switch (reg) {
> > +        case MII_BMCR:
> > +            if (value & MII_BMCR_RESET) {
> > +                mii_reset(mii);
> > +            } else {
> > +                mii->bmcr = value;
> > +            }
> > +            break;
> > +        case MII_BMSR:
> > +        case MII_PHYID1:
> > +        case MII_PHYID2:
> > +        case MII_ANLPAR:
> > +            qemu_log_mask(LOG_GUEST_ERROR, "%s: write to mii register %x\n",
> > +                          __func__, reg);
> > +            break;
> > +        case MII_ANAR:
> > +            mii->anar = value;
> > +            break;
> > +        default:
> > +            debug("unknown mii register %x\n", reg);
> > +        }
> > +    }
> > +}
> > +
> > +static void aw_emac_update_irq(AwEmacState *s)
> > +{
> > +    qemu_set_irq(s->irq, (s->int_sta & s->int_ctl) != 0);
> > +}
> > +
> > +static int aw_emac_can_receive(NetClientState *nc)
> > +{
> > +    AwEmacState *s = qemu_get_nic_opaque(nc);
> > +
> > +    return (s->ctl & EMAC_CTL_RX_EN) && (s->num_rx < MAX_RX);
> 
> If you return false from a can_recieve(), you need to explictly flush
> queued packets (qemu_flush_queued_packets()) when the blocking
> condition is lifted, heres a good example a bugfix patch addressing
> this issue for another mac:
> 
> http://lists.gnu.org/archive/html/qemu-devel/2013-11/msg02255.html

Ok.

> > +}
> > +
> > +static ssize_t aw_emac_receive(NetClientState *nc, const uint8_t *buf,
> > +                               size_t size)
> > +{
> > +    AwEmacState *s = qemu_get_nic_opaque(nc);
> > +    uint32_t *fifo;
> > +    uint32_t crc;
> > +    char *dest;
> > +
> > +    if (s->num_rx >= MAX_RX) {
> > +        debug("rx queue full - packed dropped\n");
> > +        return -1;
> > +    }
> > +
> > +    if (size + RX_HDR_SIZE > FIFO_SIZE) {
> > +        debug("packet too big\n");
> > +        return -1;
> > +    }
> > +
> > +    fifo = s->rx_fifos[(s->first_rx + s->num_rx) % MAX_RX];
> > +    dest = (char *)&fifo[2];
> > +    s->num_rx++;
> > +
> > +    memcpy(dest, buf, size);
> > +
> > +    /* Fill to minimum frame length */
> > +    if (size < 60) {
> > +        memset(dest + size, 0, 60 - size);
> > +        size = 60;
> > +    }
> > +
> > +    /* Append FCS */
> > +    crc = crc32(~0, buf, size);
> > +    memcpy(dest + size, &crc, 4);
> > +
> > +    fifo[0] = EMAC_UNDOCUMENTED_MAGIC;
> > +    fifo[1] = EMAC_RX_HEADER(size + 4, EMAC_RX_IO_DATA_STATUS_OK);
> > +
> > +    /* Set rx interrupt flag */
> > +    s->int_sta |= EMAC_INT_RX;
> > +    aw_emac_update_irq(s);
> > +
> > +    return size;
> > +}
> > +
> > +static void aw_emac_cleanup(NetClientState *nc)
> > +{
> > +    AwEmacState *s = qemu_get_nic_opaque(nc);
> > +
> > +    s->nic = NULL;
> > +}
> > +
> > +static void aw_emac_reset(AwEmacState *s)
> > +{
> > +    s->ctl = 0;
> > +    s->tx_mode = 0;
> > +    s->int_ctl = 0;
> > +    s->int_sta = 0;
> > +    s->tx_channel = 0;
> > +    s->first_rx = 0;
> > +    s->num_rx = 0;
> > +    s->rx_offset = 0;
> > +    s->phy_target = 0;
> > +}
> > +
> > +static uint64_t aw_emac_read(void *opaque, hwaddr offset, unsigned size)
> > +{
> > +    AwEmacState *s = opaque;
> > +    uint64_t ret;
> > +    uint32_t *rx_fifo;
> > +
> > +    switch (offset) {
> > +    case EMAC_CTL_REG:
> > +        ret = s->ctl;
> > +        break;
> > +    case EMAC_TX_MODE_REG:
> > +        ret = s->tx_mode;
> > +        break;
> > +    case EMAC_TX_INS_REG:
> > +        ret = s->tx_channel;
> > +        break;
> > +    case EMAC_RX_CTL_REG:
> > +        ret = s->rx_ctl;
> > +        break;
> > +    case EMAC_RX_IO_DATA_REG:
> > +        if (!s->num_rx) {
> > +            ret = 0;
> > +            break;
> > +        }
> > +        rx_fifo = s->rx_fifos[s->first_rx];
> > +
> > +        if (s->rx_offset >= FIFO_SIZE ||
> > +            s->rx_offset >= EMAC_RX_IO_DATA_LEN(rx_fifo[1]) + RX_HDR_SIZE) {
> > +            /* This should never happen */
> 
> Why? If its impossible via your implementation logic then you should
> assert. Its a misbehaving guest then you should
> qemu_log_mask(LOG_GUEST_ERROR

In this case it should be impossible due to the implementation of the
model, so I will add an assertion.

> > +            debug("RX fifo overrun\n");
> > +            s->first_rx = (s->first_rx + 1) % MAX_RX;
> > +            s->num_rx--;
> > +            s->rx_offset = 0;
> > +            ret = 0;
> > +            break;
> > +        }
> > +
> > +        ret = rx_fifo[s->rx_offset >> 2];
> > +        s->rx_offset += 4;
> > +
> > +        if (s->rx_offset >= EMAC_RX_IO_DATA_LEN(rx_fifo[1]) + RX_HDR_SIZE) {
> > +            s->first_rx = (s->first_rx + 1) % MAX_RX;
> > +            s->num_rx--;
> > +            s->rx_offset = 0;
> > +        }
> > +        break;
> > +    case EMAC_RX_FBC_REG:
> > +        ret = s->num_rx;
> > +        break;
> > +    case EMAC_INT_CTL_REG:
> > +        ret = s->int_ctl;
> > +        break;
> > +    case EMAC_INT_STA_REG:
> > +        ret = s->int_sta;
> > +        break;
> > +    case EMAC_MAC_MRDD_REG:
> > +        ret = mii_read(&s->mii, PHY_TARGET_ADDR(s->phy_target),
> > +                       PHY_TARGET_REG(s->phy_target));
> > +        break;
> > +    default:
> > +        debug("unknown offset %08x\n", (unsigned int)offset);
> 
> qemu_log_mask(LOG_UNIMP
> 
> I'm thinking you should UNIMP rather than the usual GUEST_ERROR as you
> have no specs to work on (same problem as the recent Digic series).

I agree.

> 
> > +        ret = 0;
> > +    }
> > +
> > +    return ret;
> > +}
> > +
> > +static void aw_emac_write(void *opaque, hwaddr offset, uint64_t value,
> > +                          unsigned size)
> > +{
> > +    AwEmacState *s = opaque;
> > +    AwEmacTxFifo *fifo;
> > +    int chan;
> > +
> > +    switch (offset) {
> > +    case EMAC_CTL_REG:
> > +        if (value & EMAC_CTL_RESET) {
> > +            debug("reset\n");
> > +            aw_emac_reset(s);
> > +            value &= ~EMAC_CTL_RESET;
> > +        }
> > +        s->ctl = value;
> 
> This is one example of a place where you may need to flush queued packets.

Ok.

> > +        break;
> > +    case EMAC_TX_MODE_REG:
> > +        s->tx_mode = value;
> > +        break;
> > +    case EMAC_TX_CTL0_REG:
> > +    case EMAC_TX_CTL1_REG:
> > +        chan = (offset == EMAC_TX_CTL0_REG ? 0 : 1);
> > +        if ((value & 1) && (s->ctl & EMAC_CTL_TX_EN)) {
> > +            qemu_send_packet(qemu_get_queue(s->nic),
> > +                             (uint8_t *)s->tx_fifos[chan].data,
> > +                             s->tx_fifos[chan].length);
> > +
> > +            /* Raise TX interrupt */
> > +            s->int_sta |= EMAC_INT_TX_CHAN(chan);
> > +            s->tx_fifos[chan].offset = 0;
> > +            aw_emac_update_irq(s);
> > +        }
> > +        break;
> > +    case EMAC_TX_INS_REG:
> > +        s->tx_channel = value < NUM_CHAN ? value : 0;
> > +        break;
> > +    case EMAC_TX_PL0_REG:
> > +    case EMAC_TX_PL1_REG:
> > +        chan = (offset == EMAC_TX_PL0_REG ? 0 : 1);
> > +        if (value > FIFO_SIZE) {
> > +            debug("invalid TX frame length %d\n", (int)value);
> 
> assert or GUEST_ERROR - any debug errory type messages should be one
> or the other depending on root cause. If caused by bad sw GUEST_ERROR.
> If a bug in this device model code, assert(false).

Ok, in this case it's a guest error.

> 
> > +            value = FIFO_SIZE;
> > +        }
> > +        s->tx_fifos[chan].length = value;
> > +        break;
> > +    case EMAC_TX_IO_DATA_REG:
> > +        fifo = &s->tx_fifos[s->tx_channel];
> > +        if (fifo->offset >= FIFO_SIZE) {
> > +            debug("TX frame data overruns fifo (%d)\n", fifo->offset);
> > +            break;
> > +        }
> > +        fifo->data[fifo->offset >> 2] = value;
> > +        fifo->offset += 4;
> > +        break;
> > +    case EMAC_RX_CTL_REG:
> > +        s->rx_ctl = value;
> > +        break;
> > +    case EMAC_RX_FBC_REG:
> > +        if (value == 0) {
> > +            s->num_rx = 0;
> > +        }
> > +        break;
> > +    case EMAC_INT_CTL_REG:
> > +        s->int_ctl = value;
> > +        break;
> > +    case EMAC_INT_STA_REG:
> > +        s->int_sta &= ~value;
> > +        break;
> > +    case EMAC_MAC_MADR_REG:
> > +        s->phy_target = value;
> > +        break;
> > +    case EMAC_MAC_MWTD_REG:
> > +        mii_write(&s->mii, PHY_TARGET_ADDR(s->phy_target),
> > +                  PHY_TARGET_REG(s->phy_target), value);
> > +        break;
> > +    default:
> > +        debug("unknown offset %08x\n", (unsigned int)offset);
> 
> LOG_UNIMP
> 
> > +    }
> > +}
> > +
> > +static void aw_emac_set_link(NetClientState *nc)
> > +{
> > +    AwEmacState *s = qemu_get_nic_opaque(nc);
> > +
> > +    mii_set_link(&s->mii, !nc->link_down);
> > +}
> > +
> > +static const MemoryRegionOps aw_emac_mem_ops = {
> > +    .read = aw_emac_read,
> > +    .write = aw_emac_write,
> > +    .endianness = DEVICE_NATIVE_ENDIAN,
> 
> Does your linux driver ever do sub-word accesses? If not you could set
> the appropriate restrictions to access size/alignment here for
> defensiveness.

No, all accesses have word size and are aligned; I will add a
restriction on the size.

> 
> > +};
> > +
> > +static NetClientInfo net_aw_emac_info = {
> > +    .type = NET_CLIENT_OPTIONS_KIND_NIC,
> > +    .size = sizeof(NICState),
> > +    .can_receive = aw_emac_can_receive,
> > +    .receive = aw_emac_receive,
> > +    .cleanup = aw_emac_cleanup,
> > +    .link_status_changed = aw_emac_set_link,
> > +};
> > +
> > +static void aw_emac_init(Object *obj)
> > +{
> > +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> > +    AwEmacState *s = AW_EMAC(obj);
> > +
> > +    memory_region_init_io(&s->iomem, OBJECT(s), &aw_emac_mem_ops, s,
> > +                          "aw_emac", 0x1000);
> > +    sysbus_init_mmio(sbd, &s->iomem);
> > +    sysbus_init_irq(sbd, &s->irq);
> > +}
> > +
> > +static void aw_emac_realize(DeviceState *dev, Error **errp)
> > +{
> > +    AwEmacState *s = AW_EMAC(dev);
> > +
> > +    qemu_macaddr_default_if_unset(&s->conf.macaddr);
> > +
> > +    s->nic = qemu_new_nic(&net_aw_emac_info, &s->conf,
> > +                          object_get_typename(OBJECT(dev)), dev->id, s);
> > +    qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
> > +
> > +    aw_emac_reset(s);
> > +    aw_emac_set_link(qemu_get_queue(s->nic));
> > +    mii_reset(&s->mii);
> > +}
> > +
> > +static Property aw_emac_properties[] = {
> > +    DEFINE_NIC_PROPERTIES(AwEmacState, conf),
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> > +static const VMStateDescription vmstate_mii = {
> > +    .name = "allwinner_emac_mii",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_UINT16(bmcr, AwEmacMii),
> > +        VMSTATE_UINT16(bmsr, AwEmacMii),
> > +        VMSTATE_UINT16(anar, AwEmacMii),
> > +        VMSTATE_UINT16(anlpar, AwEmacMii),
> > +        VMSTATE_BOOL(link_ok, AwEmacMii),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> > +static const VMStateDescription vmstate_tx_fifo = {
> > +    .name = "allwinner_emac_tx_fifo",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_UINT32_ARRAY(data, AwEmacTxFifo, FIFO_SIZE >> 2),
> > +        VMSTATE_INT32(length, AwEmacTxFifo),
> > +        VMSTATE_INT32(offset, AwEmacTxFifo),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> 
> This might warrant a dup of fifo8 as fifo32 if you want to clean this
> up. I think I have a patch handy that might help, will send tmrw.
> Check util/fifo8.c for fifo8 example.

Yes, probably AwEmacTxFifo can be replaced with Fifo32.
 
> > +
> > +static const VMStateDescription vmstate_aw_emac = {
> > +    .name = "allwinner_emac",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_STRUCT(mii, AwEmacState, 1, vmstate_mii, AwEmacMii),
> > +        VMSTATE_UINT32(ctl, AwEmacState),
> > +        VMSTATE_UINT32(tx_mode, AwEmacState),
> > +        VMSTATE_UINT32(rx_ctl, AwEmacState),
> > +        VMSTATE_UINT32(int_ctl, AwEmacState),
> > +        VMSTATE_UINT32(int_sta, AwEmacState),
> > +        VMSTATE_UINT32(phy_target, AwEmacState),
> > +        VMSTATE_INT32(tx_channel, AwEmacState),
> > +        VMSTATE_STRUCT_ARRAY(tx_fifos, AwEmacState,
> > +                             NUM_CHAN, 1, vmstate_tx_fifo, AwEmacTxFifo),
> > +        VMSTATE_BUFFER_UNSAFE(tx_fifos, AwEmacState, 1, MAX_RX * FIFO_SIZE),
> > +        VMSTATE_INT32(first_rx, AwEmacState),
> > +        VMSTATE_INT32(num_rx, AwEmacState),
> > +        VMSTATE_INT32(rx_offset, AwEmacState),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> > +static void aw_emac_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +
> > +    dc->realize = aw_emac_realize;
> > +    dc->props = aw_emac_properties;
> > +    dc->vmsd = &vmstate_aw_emac;
> > +}
> > +
> > +static const TypeInfo aw_emac_info = {
> > +    .name           = TYPE_AW_EMAC,
> > +    .parent         = TYPE_SYS_BUS_DEVICE,
> > +    .instance_size  = sizeof(AwEmacState),
> > +    .instance_init   = aw_emac_init,
> > +    .class_init     = aw_emac_class_init,
> > +};
> > +
> > +static void aw_emac_register_types(void)
> > +{
> > +    type_register_static(&aw_emac_info);
> > +}
> > +
> > +type_init(aw_emac_register_types)
> > diff --git a/include/hw/net/allwinner_emac.h b/include/hw/net/allwinner_emac.h
> > new file mode 100644
> > index 0000000..f9f9682
> > --- /dev/null
> > +++ b/include/hw/net/allwinner_emac.h
> > @@ -0,0 +1,204 @@
> > +/*
> > + * Emulation of Allwinner EMAC Fast Ethernet controller and
> > + * Realtek RTL8201CP PHY
> > + *
> > + * Copyright (C) 2013 Beniamino Galvani <b.galvani@gmail.com>
> > + *
> > + * Allwinner EMAC register definitions from Linux kernel are:
> > + *   Copyright 2012 Stefan Roese <sr@denx.de>
> > + *   Copyright 2013 Maxime Ripard <maxime.ripard@free-electrons.com>
> > + *   Copyright 1997 Sten Wang
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + */
> > +#ifndef AW_EMAC_H
> > +#define AW_EMAC_H
> > +
> > +#include "net/net.h"
> > +
> > +#define TYPE_AW_EMAC "allwinner_emac"
> > +#define AW_EMAC(obj) OBJECT_CHECK(AwEmacState, (obj), TYPE_AW_EMAC)
> > +
> > +/*
> > + * Allwinner EMAC register list
> > + */
> > +#define EMAC_CTL_REG            0x00
> > +#define EMAC_TX_MODE_REG        0x04
> > +#define EMAC_TX_FLOW_REG        0x08
> > +#define EMAC_TX_CTL0_REG        0x0C
> > +#define EMAC_TX_CTL1_REG        0x10
> > +#define EMAC_TX_INS_REG         0x14
> > +#define EMAC_TX_PL0_REG         0x18
> > +#define EMAC_TX_PL1_REG         0x1C
> > +#define EMAC_TX_STA_REG         0x20
> > +#define EMAC_TX_IO_DATA_REG     0x24
> > +#define EMAC_TX_IO_DATA1_REG    0x28
> > +#define EMAC_TX_TSVL0_REG       0x2C
> > +#define EMAC_TX_TSVH0_REG       0x30
> > +#define EMAC_TX_TSVL1_REG       0x34
> > +#define EMAC_TX_TSVH1_REG       0x38
> > +#define EMAC_RX_CTL_REG         0x3C
> > +#define EMAC_RX_HASH0_REG       0x40
> > +#define EMAC_RX_HASH1_REG       0x44
> > +#define EMAC_RX_STA_REG         0x48
> > +#define EMAC_RX_IO_DATA_REG     0x4C
> > +#define EMAC_RX_FBC_REG         0x50
> > +#define EMAC_INT_CTL_REG        0x54
> > +#define EMAC_INT_STA_REG        0x58
> > +#define EMAC_MAC_CTL0_REG       0x5C
> > +#define EMAC_MAC_CTL1_REG       0x60
> > +#define EMAC_MAC_IPGT_REG       0x64
> > +#define EMAC_MAC_IPGR_REG       0x68
> > +#define EMAC_MAC_CLRT_REG       0x6C
> > +#define EMAC_MAC_MAXF_REG       0x70
> > +#define EMAC_MAC_SUPP_REG       0x74
> > +#define EMAC_MAC_TEST_REG       0x78
> > +#define EMAC_MAC_MCFG_REG       0x7C
> > +#define EMAC_MAC_MCMD_REG       0x80
> > +#define EMAC_MAC_MADR_REG       0x84
> > +#define EMAC_MAC_MWTD_REG       0x88
> > +#define EMAC_MAC_MRDD_REG       0x8C
> > +#define EMAC_MAC_MIND_REG       0x90
> > +#define EMAC_MAC_SSRR_REG       0x94
> > +#define EMAC_MAC_A0_REG         0x98
> > +#define EMAC_MAC_A1_REG         0x9C
> > +#define EMAC_MAC_A2_REG         0xA0
> > +#define EMAC_SAFX_L_REG0        0xA4
> > +#define EMAC_SAFX_H_REG0        0xA8
> > +#define EMAC_SAFX_L_REG1        0xAC
> > +#define EMAC_SAFX_H_REG1        0xB0
> > +#define EMAC_SAFX_L_REG2        0xB4
> > +#define EMAC_SAFX_H_REG2        0xB8
> > +#define EMAC_SAFX_L_REG3        0xBC
> > +#define EMAC_SAFX_H_REG3        0xC0
> > +
> > +/* CTL register fields */
> > +#define EMAC_CTL_RESET                  (1 << 0)
> > +#define EMAC_CTL_TX_EN                  (1 << 1)
> > +#define EMAC_CTL_RX_EN                  (1 << 2)
> > +
> > +/* TX MODE register fields */
> > +#define EMAC_TX_MODE_ABORTED_FRAME_EN   (1 << 0)
> > +#define EMAC_TX_MODE_DMA_EN             (1 << 1)
> > +
> > +/* RX CTL register fields */
> > +#define EMAC_RX_CTL_AUTO_DRQ_EN         (1 << 1)
> > +#define EMAC_RX_CTL_DMA_EN              (1 << 2)
> > +#define EMAC_RX_CTL_PASS_ALL_EN         (1 << 4)
> > +#define EMAC_RX_CTL_PASS_CTL_EN         (1 << 5)
> > +#define EMAC_RX_CTL_PASS_CRC_ERR_EN     (1 << 6)
> > +#define EMAC_RX_CTL_PASS_LEN_ERR_EN     (1 << 7)
> > +#define EMAC_RX_CTL_PASS_LEN_OOR_EN     (1 << 8)
> > +#define EMAC_RX_CTL_ACCEPT_UNICAST_EN   (1 << 16)
> > +#define EMAC_RX_CTL_DA_FILTER_EN        (1 << 17)
> > +#define EMAC_RX_CTL_ACCEPT_MULTICAST_EN (1 << 20)
> > +#define EMAC_RX_CTL_HASH_FILTER_EN      (1 << 21)
> > +#define EMAC_RX_CTL_ACCEPT_BROADCAST_EN (1 << 22)
> > +#define EMAC_RX_CTL_SA_FILTER_EN        (1 << 24)
> > +#define EMAC_RX_CTL_SA_FILTER_INVERT_EN (1 << 25)
> > +
> > +/* RX IO DATA register fields */
> > +#define EMAC_RX_IO_DATA_LEN(x)          (x & 0xffff)
> > +#define EMAC_RX_IO_DATA_STATUS(x)       ((x >> 16) & 0xffff)
> 
> use extract32 rather than >> & logic. Although I find the macrofied
> extractors a bit wierd. Usually only a shift and width are macroified
> and the extraction process is done inline.

Ok.

> 
> > +#define EMAC_RX_HEADER(len, status)     (((len) & 0xffff) | ((status) << 16))
> > +#define EMAC_RX_IO_DATA_STATUS_CRC_ERR  (1 << 4)
> > +#define EMAC_RX_IO_DATA_STATUS_LEN_ERR  (3 << 5)
> > +#define EMAC_RX_IO_DATA_STATUS_OK       (1 << 7)
> > +#define EMAC_UNDOCUMENTED_MAGIC         0x0143414d  /* header for RX frames */
> > +
> > +/* PHY registers */
> > +#define MII_BMCR            0
> > +#define MII_BMSR            1
> > +#define MII_PHYID1          2
> > +#define MII_PHYID2          3
> > +#define MII_ANAR            4
> > +#define MII_ANLPAR          5
> > +
> > +/* PHY registers fields */
> > +#define MII_BMCR_RESET      (1 << 15)
> > +#define MII_BMCR_LOOPBACK   (1 << 14)
> > +#define MII_BMCR_SPEED      (1 << 13)
> > +#define MII_BMCR_AUTOEN     (1 << 12)
> > +#define MII_BMCR_FD         (1 << 8)
> > +
> > +#define MII_BMSR_100TX_FD   (1 << 14)
> > +#define MII_BMSR_100TX_HD   (1 << 13)
> > +#define MII_BMSR_10T_FD     (1 << 12)
> > +#define MII_BMSR_10T_HD     (1 << 11)
> > +#define MII_BMSR_MFPS       (1 << 6)
> > +#define MII_BMSR_AUTONEG    (1 << 3)
> > +#define MII_BMSR_LINK_ST    (1 << 2)
> > +
> > +#define MII_ANAR_TXFD       (1 << 8)
> > +#define MII_ANAR_TX         (1 << 7)
> > +#define MII_ANAR_10FD       (1 << 6)
> > +#define MII_ANAR_10         (1 << 5)
> > +#define MII_ANAR_CSMACD     (1 << 0)
> > +
> > +#define RTL8201CP_PHYID1    0x0000
> > +#define RTL8201CP_PHYID2    0x8201
> > +
> > +/* INT CTL and INT STA registers fields */
> > +#define EMAC_INT_TX_CHAN(x) (1 << (x))
> > +#define EMAC_INT_RX         (1 << 8)
> > +
> > +#define BOARD_PHY_ADDRESS   1
> 
> This board level configurable?

This is the value hardwired on the cubieboard, the only board that
uses the Allwinner A10 at the moment in qemu.
Should I add a property to the EMAC for changing the phy address?

> 
> > +
> > +#define NUM_CHAN            2
> > +#define FIFO_SIZE           2048
> > +#define MAX_RX              12
> > +#define RX_HDR_SIZE         8
> > +
> > +#define PHY_TARGET_ADDR(x)  (((x) >> 8) & 0xff)
> 
> extract32 (or 16?).

Ok, thanks for the review.

Regards,
Beniamino

Re: [Qemu-devel] [PATCH 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller

[Peter Crosthwaite]

On Fri, Jan 3, 2014 at 12:58 AM, Beniamino Galvani <b.galvani@gmail.com> wrote:
> On Thu, Jan 02, 2014 at 08:25:10PM +1000, Peter Crosthwaite wrote:
>> > +#undef AW_EMAC_DEBUG
>> > +
>> > +#ifdef AW_EMAC_DEBUG
>> > +#define debug(...)                                              \
>> > +    do {                                                        \
>> > +        fprintf(stderr,  "allwinner_emac : %s: ", __func__);    \
>> > +        fprintf(stderr, ## __VA_ARGS__);                        \
>> > +    } while (0)
>> > +#else
>> > +#define debug(...) do {} while (0)
>> > +#endif
>>
>> For debug macros, its better to use a regular if (DEBUG_SYMBOL) so the
>> body of the macro always gets compile tested. We have had incidences
>> where major change patterns break stripped debug instrumentation
>> because the code is compiled out.
>
> Ok.
>
>>
>> > +
>> > +static void mii_set_link(AwEmacMii *mii, bool link_ok)
>> > +{
>> > +    if (link_ok) {
>> > +        mii->bmsr |= MII_BMSR_LINK_ST;
>> > +        mii->anlpar |= MII_ANAR_TXFD | MII_ANAR_TX | MII_ANAR_10FD |
>> > +                       MII_ANAR_10 | MII_ANAR_CSMACD;
>> > +    } else {
>> > +        mii->bmsr &= ~MII_BMSR_LINK_ST;
>> > +        mii->anlpar = MII_ANAR_TX;
>> > +    }
>> > +    mii->link_ok = link_ok;
>> > +}
>> > +
>> > +static void mii_reset(AwEmacMii *mii)
>> > +{
>> > +    mii->bmcr = MII_BMCR_FD | MII_BMCR_AUTOEN | MII_BMCR_SPEED;
>> > +    mii->bmsr = MII_BMSR_100TX_FD | MII_BMSR_100TX_HD | MII_BMSR_10T_FD |
>> > +                MII_BMSR_10T_HD | MII_BMSR_MFPS | MII_BMSR_AUTONEG;
>> > +    mii->anar = MII_ANAR_TXFD | MII_ANAR_TX | MII_ANAR_10FD | MII_ANAR_10 |
>> > +                MII_ANAR_CSMACD;
>> > +    mii->anlpar = MII_ANAR_TX;
>> > +    mii_set_link(mii, mii->link_ok);
>> > +}
>> > +
>> > +static uint16_t mii_read(AwEmacMii *mii, uint8_t phy_addr, uint8_t reg)
>> > +{
>> > +    uint16_t ret = 0xffff;
>> > +
>> > +    if (phy_addr == BOARD_PHY_ADDRESS) {
>> > +        switch (reg) {
>> > +        case MII_BMCR:
>> > +            ret = mii->bmcr;
>> > +            break;
>> > +        case MII_BMSR:
>> > +            ret = mii->bmsr;
>> > +            break;
>> > +        case MII_PHYID1:
>> > +            ret = RTL8201CP_PHYID1;
>> > +            break;
>> > +        case MII_PHYID2:
>> > +            ret = RTL8201CP_PHYID2;
>> > +            break;
>> > +        case MII_ANAR:
>> > +            ret = mii->anar;
>> > +            break;
>> > +        case MII_ANLPAR:
>> > +            ret = mii->anlpar;
>> > +            break;
>> > +        default:
>> > +            debug("unknown mii register %x\n", reg);
>> > +            ret = 0;
>> > +        }
>> > +    }
>> > +    return ret;
>> > +}
>> > +
>> > +static void mii_write(AwEmacMii *mii, uint8_t phy_addr, uint8_t reg,
>> > +                      uint16_t value)
>> > +{
>> > +    if (phy_addr == BOARD_PHY_ADDRESS) {
>> > +        switch (reg) {
>> > +        case MII_BMCR:
>> > +            if (value & MII_BMCR_RESET) {
>> > +                mii_reset(mii);
>> > +            } else {
>> > +                mii->bmcr = value;
>> > +            }
>> > +            break;
>> > +        case MII_BMSR:
>> > +        case MII_PHYID1:
>> > +        case MII_PHYID2:
>> > +        case MII_ANLPAR:
>> > +            qemu_log_mask(LOG_GUEST_ERROR, "%s: write to mii register %x\n",
>> > +                          __func__, reg);
>> > +            break;
>> > +        case MII_ANAR:
>> > +            mii->anar = value;
>> > +            break;
>> > +        default:
>> > +            debug("unknown mii register %x\n", reg);
>> > +        }
>> > +    }
>> > +}
>> > +
>> > +static void aw_emac_update_irq(AwEmacState *s)
>> > +{
>> > +    qemu_set_irq(s->irq, (s->int_sta & s->int_ctl) != 0);
>> > +}
>> > +
>> > +static int aw_emac_can_receive(NetClientState *nc)
>> > +{
>> > +    AwEmacState *s = qemu_get_nic_opaque(nc);
>> > +
>> > +    return (s->ctl & EMAC_CTL_RX_EN) && (s->num_rx < MAX_RX);
>>
>> If you return false from a can_recieve(), you need to explictly flush
>> queued packets (qemu_flush_queued_packets()) when the blocking
>> condition is lifted, heres a good example a bugfix patch addressing
>> this issue for another mac:
>>
>> http://lists.gnu.org/archive/html/qemu-devel/2013-11/msg02255.html
>
> Ok.
>
>> > +}
>> > +
>> > +static ssize_t aw_emac_receive(NetClientState *nc, const uint8_t *buf,
>> > +                               size_t size)
>> > +{
>> > +    AwEmacState *s = qemu_get_nic_opaque(nc);
>> > +    uint32_t *fifo;
>> > +    uint32_t crc;
>> > +    char *dest;
>> > +
>> > +    if (s->num_rx >= MAX_RX) {
>> > +        debug("rx queue full - packed dropped\n");
>> > +        return -1;
>> > +    }
>> > +
>> > +    if (size + RX_HDR_SIZE > FIFO_SIZE) {
>> > +        debug("packet too big\n");
>> > +        return -1;
>> > +    }
>> > +
>> > +    fifo = s->rx_fifos[(s->first_rx + s->num_rx) % MAX_RX];
>> > +    dest = (char *)&fifo[2];
>> > +    s->num_rx++;
>> > +
>> > +    memcpy(dest, buf, size);
>> > +
>> > +    /* Fill to minimum frame length */
>> > +    if (size < 60) {
>> > +        memset(dest + size, 0, 60 - size);
>> > +        size = 60;
>> > +    }
>> > +
>> > +    /* Append FCS */
>> > +    crc = crc32(~0, buf, size);
>> > +    memcpy(dest + size, &crc, 4);
>> > +
>> > +    fifo[0] = EMAC_UNDOCUMENTED_MAGIC;
>> > +    fifo[1] = EMAC_RX_HEADER(size + 4, EMAC_RX_IO_DATA_STATUS_OK);
>> > +
>> > +    /* Set rx interrupt flag */
>> > +    s->int_sta |= EMAC_INT_RX;
>> > +    aw_emac_update_irq(s);
>> > +
>> > +    return size;
>> > +}
>> > +
>> > +static void aw_emac_cleanup(NetClientState *nc)
>> > +{
>> > +    AwEmacState *s = qemu_get_nic_opaque(nc);
>> > +
>> > +    s->nic = NULL;
>> > +}
>> > +
>> > +static void aw_emac_reset(AwEmacState *s)
>> > +{
>> > +    s->ctl = 0;
>> > +    s->tx_mode = 0;
>> > +    s->int_ctl = 0;
>> > +    s->int_sta = 0;
>> > +    s->tx_channel = 0;
>> > +    s->first_rx = 0;
>> > +    s->num_rx = 0;
>> > +    s->rx_offset = 0;
>> > +    s->phy_target = 0;
>> > +}
>> > +
>> > +static uint64_t aw_emac_read(void *opaque, hwaddr offset, unsigned size)
>> > +{
>> > +    AwEmacState *s = opaque;
>> > +    uint64_t ret;
>> > +    uint32_t *rx_fifo;
>> > +
>> > +    switch (offset) {
>> > +    case EMAC_CTL_REG:
>> > +        ret = s->ctl;
>> > +        break;
>> > +    case EMAC_TX_MODE_REG:
>> > +        ret = s->tx_mode;
>> > +        break;
>> > +    case EMAC_TX_INS_REG:
>> > +        ret = s->tx_channel;
>> > +        break;
>> > +    case EMAC_RX_CTL_REG:
>> > +        ret = s->rx_ctl;
>> > +        break;
>> > +    case EMAC_RX_IO_DATA_REG:
>> > +        if (!s->num_rx) {
>> > +            ret = 0;
>> > +            break;
>> > +        }
>> > +        rx_fifo = s->rx_fifos[s->first_rx];
>> > +
>> > +        if (s->rx_offset >= FIFO_SIZE ||
>> > +            s->rx_offset >= EMAC_RX_IO_DATA_LEN(rx_fifo[1]) + RX_HDR_SIZE) {
>> > +            /* This should never happen */
>>
>> Why? If its impossible via your implementation logic then you should
>> assert. Its a misbehaving guest then you should
>> qemu_log_mask(LOG_GUEST_ERROR
>
> In this case it should be impossible due to the implementation of the
> model, so I will add an assertion.
>
>> > +            debug("RX fifo overrun\n");
>> > +            s->first_rx = (s->first_rx + 1) % MAX_RX;
>> > +            s->num_rx--;
>> > +            s->rx_offset = 0;
>> > +            ret = 0;
>> > +            break;
>> > +        }
>> > +
>> > +        ret = rx_fifo[s->rx_offset >> 2];
>> > +        s->rx_offset += 4;
>> > +
>> > +        if (s->rx_offset >= EMAC_RX_IO_DATA_LEN(rx_fifo[1]) + RX_HDR_SIZE) {
>> > +            s->first_rx = (s->first_rx + 1) % MAX_RX;
>> > +            s->num_rx--;
>> > +            s->rx_offset = 0;
>> > +        }
>> > +        break;
>> > +    case EMAC_RX_FBC_REG:
>> > +        ret = s->num_rx;
>> > +        break;
>> > +    case EMAC_INT_CTL_REG:
>> > +        ret = s->int_ctl;
>> > +        break;
>> > +    case EMAC_INT_STA_REG:
>> > +        ret = s->int_sta;
>> > +        break;
>> > +    case EMAC_MAC_MRDD_REG:
>> > +        ret = mii_read(&s->mii, PHY_TARGET_ADDR(s->phy_target),
>> > +                       PHY_TARGET_REG(s->phy_target));
>> > +        break;
>> > +    default:
>> > +        debug("unknown offset %08x\n", (unsigned int)offset);
>>
>> qemu_log_mask(LOG_UNIMP
>>
>> I'm thinking you should UNIMP rather than the usual GUEST_ERROR as you
>> have no specs to work on (same problem as the recent Digic series).
>
> I agree.
>
>>
>> > +        ret = 0;
>> > +    }
>> > +
>> > +    return ret;
>> > +}
>> > +
>> > +static void aw_emac_write(void *opaque, hwaddr offset, uint64_t value,
>> > +                          unsigned size)
>> > +{
>> > +    AwEmacState *s = opaque;
>> > +    AwEmacTxFifo *fifo;
>> > +    int chan;
>> > +
>> > +    switch (offset) {
>> > +    case EMAC_CTL_REG:
>> > +        if (value & EMAC_CTL_RESET) {
>> > +            debug("reset\n");
>> > +            aw_emac_reset(s);
>> > +            value &= ~EMAC_CTL_RESET;
>> > +        }
>> > +        s->ctl = value;
>>
>> This is one example of a place where you may need to flush queued packets.
>
> Ok.
>
>> > +        break;
>> > +    case EMAC_TX_MODE_REG:
>> > +        s->tx_mode = value;
>> > +        break;
>> > +    case EMAC_TX_CTL0_REG:
>> > +    case EMAC_TX_CTL1_REG:
>> > +        chan = (offset == EMAC_TX_CTL0_REG ? 0 : 1);
>> > +        if ((value & 1) && (s->ctl & EMAC_CTL_TX_EN)) {
>> > +            qemu_send_packet(qemu_get_queue(s->nic),
>> > +                             (uint8_t *)s->tx_fifos[chan].data,
>> > +                             s->tx_fifos[chan].length);
>> > +
>> > +            /* Raise TX interrupt */
>> > +            s->int_sta |= EMAC_INT_TX_CHAN(chan);
>> > +            s->tx_fifos[chan].offset = 0;
>> > +            aw_emac_update_irq(s);
>> > +        }
>> > +        break;
>> > +    case EMAC_TX_INS_REG:
>> > +        s->tx_channel = value < NUM_CHAN ? value : 0;
>> > +        break;
>> > +    case EMAC_TX_PL0_REG:
>> > +    case EMAC_TX_PL1_REG:
>> > +        chan = (offset == EMAC_TX_PL0_REG ? 0 : 1);
>> > +        if (value > FIFO_SIZE) {
>> > +            debug("invalid TX frame length %d\n", (int)value);
>>
>> assert or GUEST_ERROR - any debug errory type messages should be one
>> or the other depending on root cause. If caused by bad sw GUEST_ERROR.
>> If a bug in this device model code, assert(false).
>
> Ok, in this case it's a guest error.
>
>>
>> > +            value = FIFO_SIZE;
>> > +        }
>> > +        s->tx_fifos[chan].length = value;
>> > +        break;
>> > +    case EMAC_TX_IO_DATA_REG:
>> > +        fifo = &s->tx_fifos[s->tx_channel];
>> > +        if (fifo->offset >= FIFO_SIZE) {
>> > +            debug("TX frame data overruns fifo (%d)\n", fifo->offset);
>> > +            break;
>> > +        }
>> > +        fifo->data[fifo->offset >> 2] = value;
>> > +        fifo->offset += 4;
>> > +        break;
>> > +    case EMAC_RX_CTL_REG:
>> > +        s->rx_ctl = value;
>> > +        break;
>> > +    case EMAC_RX_FBC_REG:
>> > +        if (value == 0) {
>> > +            s->num_rx = 0;
>> > +        }
>> > +        break;
>> > +    case EMAC_INT_CTL_REG:
>> > +        s->int_ctl = value;
>> > +        break;
>> > +    case EMAC_INT_STA_REG:
>> > +        s->int_sta &= ~value;
>> > +        break;
>> > +    case EMAC_MAC_MADR_REG:
>> > +        s->phy_target = value;
>> > +        break;
>> > +    case EMAC_MAC_MWTD_REG:
>> > +        mii_write(&s->mii, PHY_TARGET_ADDR(s->phy_target),
>> > +                  PHY_TARGET_REG(s->phy_target), value);
>> > +        break;
>> > +    default:
>> > +        debug("unknown offset %08x\n", (unsigned int)offset);
>>
>> LOG_UNIMP
>>
>> > +    }
>> > +}
>> > +
>> > +static void aw_emac_set_link(NetClientState *nc)
>> > +{
>> > +    AwEmacState *s = qemu_get_nic_opaque(nc);
>> > +
>> > +    mii_set_link(&s->mii, !nc->link_down);
>> > +}
>> > +
>> > +static const MemoryRegionOps aw_emac_mem_ops = {
>> > +    .read = aw_emac_read,
>> > +    .write = aw_emac_write,
>> > +    .endianness = DEVICE_NATIVE_ENDIAN,
>>
>> Does your linux driver ever do sub-word accesses? If not you could set
>> the appropriate restrictions to access size/alignment here for
>> defensiveness.
>
> No, all accesses have word size and are aligned; I will add a
> restriction on the size.
>
>>
>> > +};
>> > +
>> > +static NetClientInfo net_aw_emac_info = {
>> > +    .type = NET_CLIENT_OPTIONS_KIND_NIC,
>> > +    .size = sizeof(NICState),
>> > +    .can_receive = aw_emac_can_receive,
>> > +    .receive = aw_emac_receive,
>> > +    .cleanup = aw_emac_cleanup,
>> > +    .link_status_changed = aw_emac_set_link,
>> > +};
>> > +
>> > +static void aw_emac_init(Object *obj)
>> > +{
>> > +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>> > +    AwEmacState *s = AW_EMAC(obj);
>> > +
>> > +    memory_region_init_io(&s->iomem, OBJECT(s), &aw_emac_mem_ops, s,
>> > +                          "aw_emac", 0x1000);
>> > +    sysbus_init_mmio(sbd, &s->iomem);
>> > +    sysbus_init_irq(sbd, &s->irq);
>> > +}
>> > +
>> > +static void aw_emac_realize(DeviceState *dev, Error **errp)
>> > +{
>> > +    AwEmacState *s = AW_EMAC(dev);
>> > +
>> > +    qemu_macaddr_default_if_unset(&s->conf.macaddr);
>> > +
>> > +    s->nic = qemu_new_nic(&net_aw_emac_info, &s->conf,
>> > +                          object_get_typename(OBJECT(dev)), dev->id, s);
>> > +    qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
>> > +
>> > +    aw_emac_reset(s);
>> > +    aw_emac_set_link(qemu_get_queue(s->nic));
>> > +    mii_reset(&s->mii);
>> > +}
>> > +
>> > +static Property aw_emac_properties[] = {
>> > +    DEFINE_NIC_PROPERTIES(AwEmacState, conf),
>> > +    DEFINE_PROP_END_OF_LIST(),
>> > +};
>> > +
>> > +static const VMStateDescription vmstate_mii = {
>> > +    .name = "allwinner_emac_mii",
>> > +    .version_id = 1,
>> > +    .minimum_version_id = 1,
>> > +    .fields = (VMStateField[]) {
>> > +        VMSTATE_UINT16(bmcr, AwEmacMii),
>> > +        VMSTATE_UINT16(bmsr, AwEmacMii),
>> > +        VMSTATE_UINT16(anar, AwEmacMii),
>> > +        VMSTATE_UINT16(anlpar, AwEmacMii),
>> > +        VMSTATE_BOOL(link_ok, AwEmacMii),
>> > +        VMSTATE_END_OF_LIST()
>> > +    }
>> > +};
>> > +
>> > +static const VMStateDescription vmstate_tx_fifo = {
>> > +    .name = "allwinner_emac_tx_fifo",
>> > +    .version_id = 1,
>> > +    .minimum_version_id = 1,
>> > +    .fields = (VMStateField[]) {
>> > +        VMSTATE_UINT32_ARRAY(data, AwEmacTxFifo, FIFO_SIZE >> 2),
>> > +        VMSTATE_INT32(length, AwEmacTxFifo),
>> > +        VMSTATE_INT32(offset, AwEmacTxFifo),
>> > +        VMSTATE_END_OF_LIST()
>> > +    }
>> > +};
>>
>> This might warrant a dup of fifo8 as fifo32 if you want to clean this
>> up. I think I have a patch handy that might help, will send tmrw.
>> Check util/fifo8.c for fifo8 example.
>
> Yes, probably AwEmacTxFifo can be replaced with Fifo32.
>
>> > +
>> > +static const VMStateDescription vmstate_aw_emac = {
>> > +    .name = "allwinner_emac",
>> > +    .version_id = 1,
>> > +    .minimum_version_id = 1,
>> > +    .fields = (VMStateField[]) {
>> > +        VMSTATE_STRUCT(mii, AwEmacState, 1, vmstate_mii, AwEmacMii),
>> > +        VMSTATE_UINT32(ctl, AwEmacState),
>> > +        VMSTATE_UINT32(tx_mode, AwEmacState),
>> > +        VMSTATE_UINT32(rx_ctl, AwEmacState),
>> > +        VMSTATE_UINT32(int_ctl, AwEmacState),
>> > +        VMSTATE_UINT32(int_sta, AwEmacState),
>> > +        VMSTATE_UINT32(phy_target, AwEmacState),
>> > +        VMSTATE_INT32(tx_channel, AwEmacState),
>> > +        VMSTATE_STRUCT_ARRAY(tx_fifos, AwEmacState,
>> > +                             NUM_CHAN, 1, vmstate_tx_fifo, AwEmacTxFifo),
>> > +        VMSTATE_BUFFER_UNSAFE(tx_fifos, AwEmacState, 1, MAX_RX * FIFO_SIZE),
>> > +        VMSTATE_INT32(first_rx, AwEmacState),
>> > +        VMSTATE_INT32(num_rx, AwEmacState),
>> > +        VMSTATE_INT32(rx_offset, AwEmacState),
>> > +        VMSTATE_END_OF_LIST()
>> > +    }
>> > +};
>> > +
>> > +static void aw_emac_class_init(ObjectClass *klass, void *data)
>> > +{
>> > +    DeviceClass *dc = DEVICE_CLASS(klass);
>> > +
>> > +    dc->realize = aw_emac_realize;
>> > +    dc->props = aw_emac_properties;
>> > +    dc->vmsd = &vmstate_aw_emac;
>> > +}
>> > +
>> > +static const TypeInfo aw_emac_info = {
>> > +    .name           = TYPE_AW_EMAC,
>> > +    .parent         = TYPE_SYS_BUS_DEVICE,
>> > +    .instance_size  = sizeof(AwEmacState),
>> > +    .instance_init   = aw_emac_init,
>> > +    .class_init     = aw_emac_class_init,
>> > +};
>> > +
>> > +static void aw_emac_register_types(void)
>> > +{
>> > +    type_register_static(&aw_emac_info);
>> > +}
>> > +
>> > +type_init(aw_emac_register_types)
>> > diff --git a/include/hw/net/allwinner_emac.h b/include/hw/net/allwinner_emac.h
>> > new file mode 100644
>> > index 0000000..f9f9682
>> > --- /dev/null
>> > +++ b/include/hw/net/allwinner_emac.h
>> > @@ -0,0 +1,204 @@
>> > +/*
>> > + * Emulation of Allwinner EMAC Fast Ethernet controller and
>> > + * Realtek RTL8201CP PHY
>> > + *
>> > + * Copyright (C) 2013 Beniamino Galvani <b.galvani@gmail.com>
>> > + *
>> > + * Allwinner EMAC register definitions from Linux kernel are:
>> > + *   Copyright 2012 Stefan Roese <sr@denx.de>
>> > + *   Copyright 2013 Maxime Ripard <maxime.ripard@free-electrons.com>
>> > + *   Copyright 1997 Sten Wang
>> > + *
>> > + * This program is free software; you can redistribute it and/or modify
>> > + * it under the terms of the GNU General Public License as published by
>> > + * the Free Software Foundation; either version 2 of the License, or
>> > + * (at your option) any later version.
>> > + *
>> > + * This program is distributed in the hope that it will be useful,
>> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> > + * GNU General Public License for more details.
>> > + *
>> > + */
>> > +#ifndef AW_EMAC_H
>> > +#define AW_EMAC_H
>> > +
>> > +#include "net/net.h"
>> > +
>> > +#define TYPE_AW_EMAC "allwinner_emac"
>> > +#define AW_EMAC(obj) OBJECT_CHECK(AwEmacState, (obj), TYPE_AW_EMAC)
>> > +
>> > +/*
>> > + * Allwinner EMAC register list
>> > + */
>> > +#define EMAC_CTL_REG            0x00
>> > +#define EMAC_TX_MODE_REG        0x04
>> > +#define EMAC_TX_FLOW_REG        0x08
>> > +#define EMAC_TX_CTL0_REG        0x0C
>> > +#define EMAC_TX_CTL1_REG        0x10
>> > +#define EMAC_TX_INS_REG         0x14
>> > +#define EMAC_TX_PL0_REG         0x18
>> > +#define EMAC_TX_PL1_REG         0x1C
>> > +#define EMAC_TX_STA_REG         0x20
>> > +#define EMAC_TX_IO_DATA_REG     0x24
>> > +#define EMAC_TX_IO_DATA1_REG    0x28
>> > +#define EMAC_TX_TSVL0_REG       0x2C
>> > +#define EMAC_TX_TSVH0_REG       0x30
>> > +#define EMAC_TX_TSVL1_REG       0x34
>> > +#define EMAC_TX_TSVH1_REG       0x38
>> > +#define EMAC_RX_CTL_REG         0x3C
>> > +#define EMAC_RX_HASH0_REG       0x40
>> > +#define EMAC_RX_HASH1_REG       0x44
>> > +#define EMAC_RX_STA_REG         0x48
>> > +#define EMAC_RX_IO_DATA_REG     0x4C
>> > +#define EMAC_RX_FBC_REG         0x50
>> > +#define EMAC_INT_CTL_REG        0x54
>> > +#define EMAC_INT_STA_REG        0x58
>> > +#define EMAC_MAC_CTL0_REG       0x5C
>> > +#define EMAC_MAC_CTL1_REG       0x60
>> > +#define EMAC_MAC_IPGT_REG       0x64
>> > +#define EMAC_MAC_IPGR_REG       0x68
>> > +#define EMAC_MAC_CLRT_REG       0x6C
>> > +#define EMAC_MAC_MAXF_REG       0x70
>> > +#define EMAC_MAC_SUPP_REG       0x74
>> > +#define EMAC_MAC_TEST_REG       0x78
>> > +#define EMAC_MAC_MCFG_REG       0x7C
>> > +#define EMAC_MAC_MCMD_REG       0x80
>> > +#define EMAC_MAC_MADR_REG       0x84
>> > +#define EMAC_MAC_MWTD_REG       0x88
>> > +#define EMAC_MAC_MRDD_REG       0x8C
>> > +#define EMAC_MAC_MIND_REG       0x90
>> > +#define EMAC_MAC_SSRR_REG       0x94
>> > +#define EMAC_MAC_A0_REG         0x98
>> > +#define EMAC_MAC_A1_REG         0x9C
>> > +#define EMAC_MAC_A2_REG         0xA0
>> > +#define EMAC_SAFX_L_REG0        0xA4
>> > +#define EMAC_SAFX_H_REG0        0xA8
>> > +#define EMAC_SAFX_L_REG1        0xAC
>> > +#define EMAC_SAFX_H_REG1        0xB0
>> > +#define EMAC_SAFX_L_REG2        0xB4
>> > +#define EMAC_SAFX_H_REG2        0xB8
>> > +#define EMAC_SAFX_L_REG3        0xBC
>> > +#define EMAC_SAFX_H_REG3        0xC0
>> > +
>> > +/* CTL register fields */
>> > +#define EMAC_CTL_RESET                  (1 << 0)
>> > +#define EMAC_CTL_TX_EN                  (1 << 1)
>> > +#define EMAC_CTL_RX_EN                  (1 << 2)
>> > +
>> > +/* TX MODE register fields */
>> > +#define EMAC_TX_MODE_ABORTED_FRAME_EN   (1 << 0)
>> > +#define EMAC_TX_MODE_DMA_EN             (1 << 1)
>> > +
>> > +/* RX CTL register fields */
>> > +#define EMAC_RX_CTL_AUTO_DRQ_EN         (1 << 1)
>> > +#define EMAC_RX_CTL_DMA_EN              (1 << 2)
>> > +#define EMAC_RX_CTL_PASS_ALL_EN         (1 << 4)
>> > +#define EMAC_RX_CTL_PASS_CTL_EN         (1 << 5)
>> > +#define EMAC_RX_CTL_PASS_CRC_ERR_EN     (1 << 6)
>> > +#define EMAC_RX_CTL_PASS_LEN_ERR_EN     (1 << 7)
>> > +#define EMAC_RX_CTL_PASS_LEN_OOR_EN     (1 << 8)
>> > +#define EMAC_RX_CTL_ACCEPT_UNICAST_EN   (1 << 16)
>> > +#define EMAC_RX_CTL_DA_FILTER_EN        (1 << 17)
>> > +#define EMAC_RX_CTL_ACCEPT_MULTICAST_EN (1 << 20)
>> > +#define EMAC_RX_CTL_HASH_FILTER_EN      (1 << 21)
>> > +#define EMAC_RX_CTL_ACCEPT_BROADCAST_EN (1 << 22)
>> > +#define EMAC_RX_CTL_SA_FILTER_EN        (1 << 24)
>> > +#define EMAC_RX_CTL_SA_FILTER_INVERT_EN (1 << 25)
>> > +
>> > +/* RX IO DATA register fields */
>> > +#define EMAC_RX_IO_DATA_LEN(x)          (x & 0xffff)
>> > +#define EMAC_RX_IO_DATA_STATUS(x)       ((x >> 16) & 0xffff)
>>
>> use extract32 rather than >> & logic. Although I find the macrofied
>> extractors a bit wierd. Usually only a shift and width are macroified
>> and the extraction process is done inline.
>
> Ok.
>
>>
>> > +#define EMAC_RX_HEADER(len, status)     (((len) & 0xffff) | ((status) << 16))
>> > +#define EMAC_RX_IO_DATA_STATUS_CRC_ERR  (1 << 4)
>> > +#define EMAC_RX_IO_DATA_STATUS_LEN_ERR  (3 << 5)
>> > +#define EMAC_RX_IO_DATA_STATUS_OK       (1 << 7)
>> > +#define EMAC_UNDOCUMENTED_MAGIC         0x0143414d  /* header for RX frames */
>> > +
>> > +/* PHY registers */
>> > +#define MII_BMCR            0
>> > +#define MII_BMSR            1
>> > +#define MII_PHYID1          2
>> > +#define MII_PHYID2          3
>> > +#define MII_ANAR            4
>> > +#define MII_ANLPAR          5
>> > +
>> > +/* PHY registers fields */
>> > +#define MII_BMCR_RESET      (1 << 15)
>> > +#define MII_BMCR_LOOPBACK   (1 << 14)
>> > +#define MII_BMCR_SPEED      (1 << 13)
>> > +#define MII_BMCR_AUTOEN     (1 << 12)
>> > +#define MII_BMCR_FD         (1 << 8)
>> > +
>> > +#define MII_BMSR_100TX_FD   (1 << 14)
>> > +#define MII_BMSR_100TX_HD   (1 << 13)
>> > +#define MII_BMSR_10T_FD     (1 << 12)
>> > +#define MII_BMSR_10T_HD     (1 << 11)
>> > +#define MII_BMSR_MFPS       (1 << 6)
>> > +#define MII_BMSR_AUTONEG    (1 << 3)
>> > +#define MII_BMSR_LINK_ST    (1 << 2)
>> > +
>> > +#define MII_ANAR_TXFD       (1 << 8)
>> > +#define MII_ANAR_TX         (1 << 7)
>> > +#define MII_ANAR_10FD       (1 << 6)
>> > +#define MII_ANAR_10         (1 << 5)
>> > +#define MII_ANAR_CSMACD     (1 << 0)
>> > +
>> > +#define RTL8201CP_PHYID1    0x0000
>> > +#define RTL8201CP_PHYID2    0x8201
>> > +
>> > +/* INT CTL and INT STA registers fields */
>> > +#define EMAC_INT_TX_CHAN(x) (1 << (x))
>> > +#define EMAC_INT_RX         (1 << 8)
>> > +
>> > +#define BOARD_PHY_ADDRESS   1
>>
>> This board level configurable?
>
> This is the value hardwired on the cubieboard, the only board that
> uses the Allwinner A10 at the moment in qemu.

Yeh but this code should make efforts to be generic to all allwinner SoC.

> Should I add a property to the EMAC for changing the phy address?
>

Well the PHY address is property of the PHY if anything not the MAC.
But your proposal is at least more flexible than harcoding to a
specific board. Until we have a plan RE proper seperation of PHYs and
MACs your proposal is as good as it gets.

Regards,
Peter

>>
>> > +
>> > +#define NUM_CHAN            2
>> > +#define FIFO_SIZE           2048
>> > +#define MAX_RX              12
>> > +#define RX_HDR_SIZE         8
>> > +
>> > +#define PHY_TARGET_ADDR(x)  (((x) >> 8) & 0xff)
>>
>> extract32 (or 16?).
>
> Ok, thanks for the review.
>
> Regards,
> Beniamino
>

Re: [Qemu-devel] [PATCH 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller

[Beniamino Galvani]

On Fri, Jan 03, 2014 at 11:26:01AM +1000, Peter Crosthwaite wrote:
> >> > +static const VMStateDescription vmstate_tx_fifo = {
> >> > +    .name = "allwinner_emac_tx_fifo",
> >> > +    .version_id = 1,
> >> > +    .minimum_version_id = 1,
> >> > +    .fields = (VMStateField[]) {
> >> > +        VMSTATE_UINT32_ARRAY(data, AwEmacTxFifo, FIFO_SIZE >> 2),
> >> > +        VMSTATE_INT32(length, AwEmacTxFifo),
> >> > +        VMSTATE_INT32(offset, AwEmacTxFifo),
> >> > +        VMSTATE_END_OF_LIST()
> >> > +    }
> >> > +};
> >>
> >> This might warrant a dup of fifo8 as fifo32 if you want to clean this
> >> up. I think I have a patch handy that might help, will send tmrw.
> >> Check util/fifo8.c for fifo8 example.
> >
> > Yes, probably AwEmacTxFifo can be replaced with Fifo32.
> >

I need to obtain a pointer to the fifo backing buffer after the frame
has been copied completely in order to pass it to qemu_send_packet().
The generic fifo implementation doesn't allow that (unless I access
directly the structure member, but I suppose that the intent is to
treat the fifo object as opaque).

Maybe a function fifo_get_data() could be added to the generic
implementation? Otherwise I can go on using my custom implementation.

Beniamino

Re: [Qemu-devel] [PATCH 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller

[Stefan Hajnoczi]

On Thu, Jan 02, 2014 at 08:25:10PM +1000, Peter Crosthwaite wrote:
> Hi Beniamino,
> 
> On Thu, Jan 2, 2014 at 7:18 PM, Beniamino Galvani <b.galvani@gmail.com> wrote:
> > This patch adds support for the Fast Ethernet MAC found on Allwinner
> > SoCs, together with a basic emulation of Realtek RTL8201CP PHY.
> >
> 
> More a comment for net in general, but I think sooner or later we need
> to move towards a split between phy and mac on the device level.
> continuing the phy-within-mac philosophy is going to make the
> socification efforts awkward. Are MII and friends a busses (as in
> TYPE_BUS) in their own right, and connection of mac and phy has to
> happen on the board level?

I see PHY and MAC split as advantageous because it allows code reuse and
better testing.  The main thing I'd like to see is PHY device tests
using tests/libqtest.h.

If someone wants to implement it, great.  It would make it easier to add
more NIC models in the future.

Regarding SOCification and busses, I'm not sure.  Is it okay to just say
a NIC has-a PHY (i.e. composition)?

Stefan

Re: [Qemu-devel] [PATCH 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller

[Peter Crosthwaite]

On Mon, Jan 6, 2014 at 1:27 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Thu, Jan 02, 2014 at 08:25:10PM +1000, Peter Crosthwaite wrote:
>> Hi Beniamino,
>>
>> On Thu, Jan 2, 2014 at 7:18 PM, Beniamino Galvani <b.galvani@gmail.com> wrote:
>> > This patch adds support for the Fast Ethernet MAC found on Allwinner
>> > SoCs, together with a basic emulation of Realtek RTL8201CP PHY.
>> >
>>
>> More a comment for net in general, but I think sooner or later we need
>> to move towards a split between phy and mac on the device level.
>> continuing the phy-within-mac philosophy is going to make the
>> socification efforts awkward. Are MII and friends a busses (as in
>> TYPE_BUS) in their own right, and connection of mac and phy has to
>> happen on the board level?
>
> I see PHY and MAC split as advantageous because it allows code reuse and
> better testing.  The main thing I'd like to see is PHY device tests
> using tests/libqtest.h.
>
> If someone wants to implement it, great.  It would make it easier to add
> more NIC models in the future.
>
> Regarding SOCification and busses, I'm not sure.  Is it okay to just say
> a NIC has-a PHY (i.e. composition)?
>

Generally speaking, in the (ARM) SoCification the MAC is part of the
SoC which in the latest styling guidelines is a composite device. This
composite is supposed to reflect the self contained SoC product which
the PHY is usually not a part of. So we have two opposing compositions
here:

NIC = MAC + PHY
SOC = CPUs + MAC + ...

MAC can't be in both. So for SoCs the NIC concept needs to abandoned.
After all the expansion of NIC as "Network Interface Card" is a little
bit PCish. Your average SoC networking solution has no such "card".
Just an on chip MAC (same pacakge/die as CPU etc) connecting to a PHY
via PCB traces.

So I think long term, MII has to be a TYPE_BUS that is visible on the
top level SoC device. Self contained NICs (as we know them today) are
then also implementable as container devices (of MAC and PHY) that use
this bus internally (in much the same way the SoC boards would attach
external PHY to SoC).

Regards,
Peter

> Stefan
>

Re: [Qemu-devel] [PATCH 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller

[Stefan Hajnoczi]

On Mon, Jan 06, 2014 at 01:46:54PM +1000, Peter Crosthwaite wrote:
> On Mon, Jan 6, 2014 at 1:27 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > On Thu, Jan 02, 2014 at 08:25:10PM +1000, Peter Crosthwaite wrote:
> >> Hi Beniamino,
> >>
> >> On Thu, Jan 2, 2014 at 7:18 PM, Beniamino Galvani <b.galvani@gmail.com> wrote:
> >> > This patch adds support for the Fast Ethernet MAC found on Allwinner
> >> > SoCs, together with a basic emulation of Realtek RTL8201CP PHY.
> >> >
> >>
> >> More a comment for net in general, but I think sooner or later we need
> >> to move towards a split between phy and mac on the device level.
> >> continuing the phy-within-mac philosophy is going to make the
> >> socification efforts awkward. Are MII and friends a busses (as in
> >> TYPE_BUS) in their own right, and connection of mac and phy has to
> >> happen on the board level?
> >
> > I see PHY and MAC split as advantageous because it allows code reuse and
> > better testing.  The main thing I'd like to see is PHY device tests
> > using tests/libqtest.h.
> >
> > If someone wants to implement it, great.  It would make it easier to add
> > more NIC models in the future.
> >
> > Regarding SOCification and busses, I'm not sure.  Is it okay to just say
> > a NIC has-a PHY (i.e. composition)?
> >
> 
> Generally speaking, in the (ARM) SoCification the MAC is part of the
> SoC which in the latest styling guidelines is a composite device. This
> composite is supposed to reflect the self contained SoC product which
> the PHY is usually not a part of. So we have two opposing compositions
> here:
> 
> NIC = MAC + PHY
> SOC = CPUs + MAC + ...
> 
> MAC can't be in both. So for SoCs the NIC concept needs to abandoned.
> After all the expansion of NIC as "Network Interface Card" is a little
> bit PCish. Your average SoC networking solution has no such "card".
> Just an on chip MAC (same pacakge/die as CPU etc) connecting to a PHY
> via PCB traces.
> 
> So I think long term, MII has to be a TYPE_BUS that is visible on the
> top level SoC device. Self contained NICs (as we know them today) are
> then also implementable as container devices (of MAC and PHY) that use
> this bus internally (in much the same way the SoC boards would attach
> external PHY to SoC).

Okay, that makes sense.  Given the amount of emulated hardware in QEMU
today, I think it would be okay to simply add new MAC/PHYs while still
supporting the NICs of old.  If someone is enthusiastic about
refactoring and testing existing NICs then great.  But I think it's more
pragmatic to simply start working with a split MAC/PHY where that is
beneficial.

Stefan

Re: [Qemu-devel] [PATCH 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller

[Beniamino Galvani]

On Mon, Jan 06, 2014 at 02:12:27PM +0800, Stefan Hajnoczi wrote:
> > >> More a comment for net in general, but I think sooner or later we need
> > >> to move towards a split between phy and mac on the device level.
> > >> continuing the phy-within-mac philosophy is going to make the
> > >> socification efforts awkward. Are MII and friends a busses (as in
> > >> TYPE_BUS) in their own right, and connection of mac and phy has to
> > >> happen on the board level?
> > >
> > > I see PHY and MAC split as advantageous because it allows code reuse and
> > > better testing.  The main thing I'd like to see is PHY device tests
> > > using tests/libqtest.h.
> > >
> > > If someone wants to implement it, great.  It would make it easier to add
> > > more NIC models in the future.
> > >
> > > Regarding SOCification and busses, I'm not sure.  Is it okay to just say
> > > a NIC has-a PHY (i.e. composition)?
> > >
> > 
> > Generally speaking, in the (ARM) SoCification the MAC is part of the
> > SoC which in the latest styling guidelines is a composite device. This
> > composite is supposed to reflect the self contained SoC product which
> > the PHY is usually not a part of. So we have two opposing compositions
> > here:
> > 
> > NIC = MAC + PHY
> > SOC = CPUs + MAC + ...
> > 
> > MAC can't be in both. So for SoCs the NIC concept needs to abandoned.
> > After all the expansion of NIC as "Network Interface Card" is a little
> > bit PCish. Your average SoC networking solution has no such "card".
> > Just an on chip MAC (same pacakge/die as CPU etc) connecting to a PHY
> > via PCB traces.
> > 
> > So I think long term, MII has to be a TYPE_BUS that is visible on the
> > top level SoC device. Self contained NICs (as we know them today) are
> > then also implementable as container devices (of MAC and PHY) that use
> > this bus internally (in much the same way the SoC boards would attach
> > external PHY to SoC).
> 
> Okay, that makes sense.  Given the amount of emulated hardware in QEMU
> today, I think it would be okay to simply add new MAC/PHYs while still
> supporting the NICs of old.  If someone is enthusiastic about
> refactoring and testing existing NICs then great.  But I think it's more
> pragmatic to simply start working with a split MAC/PHY where that is
> beneficial.

Regarding the patch, can I resubmit it with MAC and PHY modeled as a
single device? Or it's better to start thinking on how to implement
proper MAC/PHY split?

Beniamino

Re: [Qemu-devel] [PATCH 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller

[Peter Crosthwaite]

On Sat, Jan 11, 2014 at 7:48 AM, Beniamino Galvani <b.galvani@gmail.com> wrote:
> On Mon, Jan 06, 2014 at 02:12:27PM +0800, Stefan Hajnoczi wrote:
>> > >> More a comment for net in general, but I think sooner or later we need
>> > >> to move towards a split between phy and mac on the device level.
>> > >> continuing the phy-within-mac philosophy is going to make the
>> > >> socification efforts awkward. Are MII and friends a busses (as in
>> > >> TYPE_BUS) in their own right, and connection of mac and phy has to
>> > >> happen on the board level?
>> > >
>> > > I see PHY and MAC split as advantageous because it allows code reuse and
>> > > better testing.  The main thing I'd like to see is PHY device tests
>> > > using tests/libqtest.h.
>> > >
>> > > If someone wants to implement it, great.  It would make it easier to add
>> > > more NIC models in the future.
>> > >
>> > > Regarding SOCification and busses, I'm not sure.  Is it okay to just say
>> > > a NIC has-a PHY (i.e. composition)?
>> > >
>> >
>> > Generally speaking, in the (ARM) SoCification the MAC is part of the
>> > SoC which in the latest styling guidelines is a composite device. This
>> > composite is supposed to reflect the self contained SoC product which
>> > the PHY is usually not a part of. So we have two opposing compositions
>> > here:
>> >
>> > NIC = MAC + PHY
>> > SOC = CPUs + MAC + ...
>> >
>> > MAC can't be in both. So for SoCs the NIC concept needs to abandoned.
>> > After all the expansion of NIC as "Network Interface Card" is a little
>> > bit PCish. Your average SoC networking solution has no such "card".
>> > Just an on chip MAC (same pacakge/die as CPU etc) connecting to a PHY
>> > via PCB traces.
>> >
>> > So I think long term, MII has to be a TYPE_BUS that is visible on the
>> > top level SoC device. Self contained NICs (as we know them today) are
>> > then also implementable as container devices (of MAC and PHY) that use
>> > this bus internally (in much the same way the SoC boards would attach
>> > external PHY to SoC).
>>
>> Okay, that makes sense.  Given the amount of emulated hardware in QEMU
>> today, I think it would be okay to simply add new MAC/PHYs while still
>> supporting the NICs of old.  If someone is enthusiastic about
>> refactoring and testing existing NICs then great.  But I think it's more
>> pragmatic to simply start working with a split MAC/PHY where that is
>> beneficial.
>
> Regarding the patch, can I resubmit it with MAC and PHY modeled as a
> single device? Or it's better to start thinking on how to implement
> proper MAC/PHY split?
>

Resubmit as a single. Don't wait on the proposed fifo cleanups either.
I'm not going to block.

Regards,
Peter

> Beniamino
>

Re: [Qemu-devel] [PATCH 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller

[Peter Crosthwaite]

On Mon, Jan 6, 2014 at 4:12 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Mon, Jan 06, 2014 at 01:46:54PM +1000, Peter Crosthwaite wrote:
>> On Mon, Jan 6, 2014 at 1:27 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>> > On Thu, Jan 02, 2014 at 08:25:10PM +1000, Peter Crosthwaite wrote:
>> >> Hi Beniamino,
>> >>
>> >> On Thu, Jan 2, 2014 at 7:18 PM, Beniamino Galvani <b.galvani@gmail.com> wrote:
>> >> > This patch adds support for the Fast Ethernet MAC found on Allwinner
>> >> > SoCs, together with a basic emulation of Realtek RTL8201CP PHY.
>> >> >
>> >>
>> >> More a comment for net in general, but I think sooner or later we need
>> >> to move towards a split between phy and mac on the device level.
>> >> continuing the phy-within-mac philosophy is going to make the
>> >> socification efforts awkward. Are MII and friends a busses (as in
>> >> TYPE_BUS) in their own right, and connection of mac and phy has to
>> >> happen on the board level?
>> >
>> > I see PHY and MAC split as advantageous because it allows code reuse and
>> > better testing.  The main thing I'd like to see is PHY device tests
>> > using tests/libqtest.h.
>> >
>> > If someone wants to implement it, great.  It would make it easier to add
>> > more NIC models in the future.
>> >
>> > Regarding SOCification and busses, I'm not sure.  Is it okay to just say
>> > a NIC has-a PHY (i.e. composition)?
>> >
>>
>> Generally speaking, in the (ARM) SoCification the MAC is part of the
>> SoC which in the latest styling guidelines is a composite device. This
>> composite is supposed to reflect the self contained SoC product which
>> the PHY is usually not a part of. So we have two opposing compositions
>> here:
>>
>> NIC = MAC + PHY
>> SOC = CPUs + MAC + ...
>>
>> MAC can't be in both. So for SoCs the NIC concept needs to abandoned.
>> After all the expansion of NIC as "Network Interface Card" is a little
>> bit PCish. Your average SoC networking solution has no such "card".
>> Just an on chip MAC (same pacakge/die as CPU etc) connecting to a PHY
>> via PCB traces.
>>
>> So I think long term, MII has to be a TYPE_BUS that is visible on the
>> top level SoC device. Self contained NICs (as we know them today) are
>> then also implementable as container devices (of MAC and PHY) that use
>> this bus internally (in much the same way the SoC boards would attach
>> external PHY to SoC).
>
> Okay, that makes sense.  Given the amount of emulated hardware in QEMU
> today, I think it would be okay to simply add new MAC/PHYs while still
> supporting the NICs of old.  If someone is enthusiastic about
> refactoring and testing existing NICs then great.  But I think it's more
> pragmatic to simply start working with a split MAC/PHY where that is
> beneficial.
>

Alright,

So lets make some plans. There is devil in the detail here. There was
a previous attempt to do something similar by Grant early last year so
cc as FYI.

So the main question is whether or not this new interface is just for
MDIO or is the full MII interface (both MDIO and packet data).

My inclination is the latter, we want a new proper QOM bus that is
both. What this would mean, is that these MAC-only devices wont be net
devices at all. the -net args are instead applied to the PHY. This
makes the most sense to me as its the phy that actually has copper
connection to the external network, not MAC.

MAC <---- TYPE_MII_BUS ----> PHY <-----net layer ------> external
network: "-net foo,bar,baz"

Another approach is to make both net devices in their own right. Phy
has two net-layer-managed attachments, one for external network, and
one point-to-point for the MII connecting to MAC. The MDIO bus is then
a side channel which may or may not be QOMified (depending on effort
levels). So you can still connect a standalone MAC to an external
network, assuming the guest can handle no PHY (may in reality have
limited use).

MAC <---- net layer --------> PHY <-----net layer ------> external network
    <---- TYPE_MDIO_BUS ---->

OR:

MAC <---- net layer --------> external network


The third approach (which is closest to current implementation) is to
only have the phy do MDIO and still connect the MAC straight to an
external network:

MAC <---- net layer --------> external network
     \
      <-- TYPE_MDIO_BUS ----> PHY

I dont like this though, as its a little mismatched to real hw.
Although it may be a good stepping stone to approaches 1 or 2.

RFC

Regards,
Peter

> Stefan
>

Re: [Qemu-devel] [PATCH 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller

[Edgar E. Iglesias]

On Sat, Jan 11, 2014 at 08:48:12AM +1000, Peter Crosthwaite wrote:
> On Mon, Jan 6, 2014 at 4:12 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > On Mon, Jan 06, 2014 at 01:46:54PM +1000, Peter Crosthwaite wrote:
> >> On Mon, Jan 6, 2014 at 1:27 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >> > On Thu, Jan 02, 2014 at 08:25:10PM +1000, Peter Crosthwaite wrote:
> >> >> Hi Beniamino,
> >> >>
> >> >> On Thu, Jan 2, 2014 at 7:18 PM, Beniamino Galvani <b.galvani@gmail.com> wrote:
> >> >> > This patch adds support for the Fast Ethernet MAC found on Allwinner
> >> >> > SoCs, together with a basic emulation of Realtek RTL8201CP PHY.
> >> >> >
> >> >>
> >> >> More a comment for net in general, but I think sooner or later we need
> >> >> to move towards a split between phy and mac on the device level.
> >> >> continuing the phy-within-mac philosophy is going to make the
> >> >> socification efforts awkward. Are MII and friends a busses (as in
> >> >> TYPE_BUS) in their own right, and connection of mac and phy has to
> >> >> happen on the board level?
> >> >
> >> > I see PHY and MAC split as advantageous because it allows code reuse and
> >> > better testing.  The main thing I'd like to see is PHY device tests
> >> > using tests/libqtest.h.
> >> >
> >> > If someone wants to implement it, great.  It would make it easier to add
> >> > more NIC models in the future.
> >> >
> >> > Regarding SOCification and busses, I'm not sure.  Is it okay to just say
> >> > a NIC has-a PHY (i.e. composition)?
> >> >
> >>
> >> Generally speaking, in the (ARM) SoCification the MAC is part of the
> >> SoC which in the latest styling guidelines is a composite device. This
> >> composite is supposed to reflect the self contained SoC product which
> >> the PHY is usually not a part of. So we have two opposing compositions
> >> here:
> >>
> >> NIC = MAC + PHY
> >> SOC = CPUs + MAC + ...
> >>
> >> MAC can't be in both. So for SoCs the NIC concept needs to abandoned.
> >> After all the expansion of NIC as "Network Interface Card" is a little
> >> bit PCish. Your average SoC networking solution has no such "card".
> >> Just an on chip MAC (same pacakge/die as CPU etc) connecting to a PHY
> >> via PCB traces.
> >>
> >> So I think long term, MII has to be a TYPE_BUS that is visible on the
> >> top level SoC device. Self contained NICs (as we know them today) are
> >> then also implementable as container devices (of MAC and PHY) that use
> >> this bus internally (in much the same way the SoC boards would attach
> >> external PHY to SoC).
> >
> > Okay, that makes sense.  Given the amount of emulated hardware in QEMU
> > today, I think it would be okay to simply add new MAC/PHYs while still
> > supporting the NICs of old.  If someone is enthusiastic about
> > refactoring and testing existing NICs then great.  But I think it's more
> > pragmatic to simply start working with a split MAC/PHY where that is
> > beneficial.
> >
> 
> Alright,
> 
> So lets make some plans. There is devil in the detail here. There was
> a previous attempt to do something similar by Grant early last year so
> cc as FYI.
> 
> So the main question is whether or not this new interface is just for
> MDIO or is the full MII interface (both MDIO and packet data).
> 
> My inclination is the latter, we want a new proper QOM bus that is
> both. What this would mean, is that these MAC-only devices wont be net
> devices at all. the -net args are instead applied to the PHY. This
> makes the most sense to me as its the phy that actually has copper
> connection to the external network, not MAC.
> 
> MAC <---- TYPE_MII_BUS ----> PHY <-----net layer ------> external
> network: "-net foo,bar,baz"

I don't really agree with this. You can do ethernet without a PHY,
it is in fact fairly common in the embedded switch space. You can also
have a single MDIO iface that is completely separate from any MAC take
care of many PHYs.

IMO, the MDIO bus should be separate from the data path.


> Another approach is to make both net devices in their own right. Phy
> has two net-layer-managed attachments, one for external network, and
> one point-to-point for the MII connecting to MAC. The MDIO bus is then
> a side channel which may or may not be QOMified (depending on effort
> levels). So you can still connect a standalone MAC to an external
> network, assuming the guest can handle no PHY (may in reality have
> limited use).
> 
> MAC <---- net layer --------> PHY <-----net layer ------> external network
>     <---- TYPE_MDIO_BUS ---->
> 
> OR:
> 
> MAC <---- net layer --------> external network
> 
> 
> The third approach (which is closest to current implementation) is to
> only have the phy do MDIO and still connect the MAC straight to an
> external network:
> 
> MAC <---- net layer --------> external network
>      \
>       <-- TYPE_MDIO_BUS ----> PHY
> 
> I dont like this though, as its a little mismatched to real hw.
> Although it may be a good stepping stone to approaches 1 or 2.

I'd go for this third one and possibly do something about the
data path later if needed. Or possibly nr 2, not sure if I understood
that one correctly though.

Cheers,
Edgar

Re: [Qemu-devel] [PATCH 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller

[Peter Crosthwaite]

On Sun, Jan 12, 2014 at 11:59 PM, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> On Sat, Jan 11, 2014 at 08:48:12AM +1000, Peter Crosthwaite wrote:
>> On Mon, Jan 6, 2014 at 4:12 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>> > On Mon, Jan 06, 2014 at 01:46:54PM +1000, Peter Crosthwaite wrote:
>> >> On Mon, Jan 6, 2014 at 1:27 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>> >> > On Thu, Jan 02, 2014 at 08:25:10PM +1000, Peter Crosthwaite wrote:
>> >> >> Hi Beniamino,
>> >> >>
>> >> >> On Thu, Jan 2, 2014 at 7:18 PM, Beniamino Galvani <b.galvani@gmail.com> wrote:
>> >> >> > This patch adds support for the Fast Ethernet MAC found on Allwinner
>> >> >> > SoCs, together with a basic emulation of Realtek RTL8201CP PHY.
>> >> >> >
>> >> >>
>> >> >> More a comment for net in general, but I think sooner or later we need
>> >> >> to move towards a split between phy and mac on the device level.
>> >> >> continuing the phy-within-mac philosophy is going to make the
>> >> >> socification efforts awkward. Are MII and friends a busses (as in
>> >> >> TYPE_BUS) in their own right, and connection of mac and phy has to
>> >> >> happen on the board level?
>> >> >
>> >> > I see PHY and MAC split as advantageous because it allows code reuse and
>> >> > better testing.  The main thing I'd like to see is PHY device tests
>> >> > using tests/libqtest.h.
>> >> >
>> >> > If someone wants to implement it, great.  It would make it easier to add
>> >> > more NIC models in the future.
>> >> >
>> >> > Regarding SOCification and busses, I'm not sure.  Is it okay to just say
>> >> > a NIC has-a PHY (i.e. composition)?
>> >> >
>> >>
>> >> Generally speaking, in the (ARM) SoCification the MAC is part of the
>> >> SoC which in the latest styling guidelines is a composite device. This
>> >> composite is supposed to reflect the self contained SoC product which
>> >> the PHY is usually not a part of. So we have two opposing compositions
>> >> here:
>> >>
>> >> NIC = MAC + PHY
>> >> SOC = CPUs + MAC + ...
>> >>
>> >> MAC can't be in both. So for SoCs the NIC concept needs to abandoned.
>> >> After all the expansion of NIC as "Network Interface Card" is a little
>> >> bit PCish. Your average SoC networking solution has no such "card".
>> >> Just an on chip MAC (same pacakge/die as CPU etc) connecting to a PHY
>> >> via PCB traces.
>> >>
>> >> So I think long term, MII has to be a TYPE_BUS that is visible on the
>> >> top level SoC device. Self contained NICs (as we know them today) are
>> >> then also implementable as container devices (of MAC and PHY) that use
>> >> this bus internally (in much the same way the SoC boards would attach
>> >> external PHY to SoC).
>> >
>> > Okay, that makes sense.  Given the amount of emulated hardware in QEMU
>> > today, I think it would be okay to simply add new MAC/PHYs while still
>> > supporting the NICs of old.  If someone is enthusiastic about
>> > refactoring and testing existing NICs then great.  But I think it's more
>> > pragmatic to simply start working with a split MAC/PHY where that is
>> > beneficial.
>> >
>>
>> Alright,
>>
>> So lets make some plans. There is devil in the detail here. There was
>> a previous attempt to do something similar by Grant early last year so
>> cc as FYI.
>>
>> So the main question is whether or not this new interface is just for
>> MDIO or is the full MII interface (both MDIO and packet data).
>>
>> My inclination is the latter, we want a new proper QOM bus that is
>> both. What this would mean, is that these MAC-only devices wont be net
>> devices at all. the -net args are instead applied to the PHY. This
>> makes the most sense to me as its the phy that actually has copper
>> connection to the external network, not MAC.
>>
>> MAC <---- TYPE_MII_BUS ----> PHY <-----net layer ------> external
>> network: "-net foo,bar,baz"
>
> I don't really agree with this. You can do ethernet without a PHY,
> it is in fact fairly common in the embedded switch space. You can also
> have a single MDIO iface that is completely separate from any MAC take
> care of many PHYs.
>
> IMO, the MDIO bus should be separate from the data path.
>
>
>> Another approach is to make both net devices in their own right. Phy
>> has two net-layer-managed attachments, one for external network, and
>> one point-to-point for the MII connecting to MAC. The MDIO bus is then
>> a side channel which may or may not be QOMified (depending on effort
>> levels). So you can still connect a standalone MAC to an external
>> network, assuming the guest can handle no PHY (may in reality have
>> limited use).
>>
>> MAC <---- net layer --------> PHY <-----net layer ------> external network
>>     <---- TYPE_MDIO_BUS ---->
>>
>> OR:
>>
>> MAC <---- net layer --------> external network
>>
>>
>> The third approach (which is closest to current implementation) is to
>> only have the phy do MDIO and still connect the MAC straight to an
>> external network:
>>
>> MAC <---- net layer --------> external network
>>      \
>>       <-- TYPE_MDIO_BUS ----> PHY
>>
>> I dont like this though, as its a little mismatched to real hw.
>> Although it may be a good stepping stone to approaches 1 or 2.
>
> I'd go for this third one and possibly do something about the
> data path later if needed.

Yeh, so I think that really translates to do option 3 and go to option
2 later if needed. So option 3 looks to be the winning plan for the
first series.

> Or possibly nr 2, not sure if I understood
> that one correctly though.
>

Option 2 is is the hardest but does solve your problem of either MAC
or PHY connecting to a network and with a more accurate data flow
model. I think you want to do 3 first to get there. So we can plan 3
with consideration for 2.

Regards,
Peter

> Cheers,
> Edgar
>

Re: [Qemu-devel] [PATCH 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller

[Stefan Hajnoczi]

On Mon, Jan 13, 2014 at 08:00:37AM +1000, Peter Crosthwaite wrote:
> On Sun, Jan 12, 2014 at 11:59 PM, Edgar E. Iglesias
> <edgar.iglesias@gmail.com> wrote:
> > On Sat, Jan 11, 2014 at 08:48:12AM +1000, Peter Crosthwaite wrote:
> >> On Mon, Jan 6, 2014 at 4:12 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >> > On Mon, Jan 06, 2014 at 01:46:54PM +1000, Peter Crosthwaite wrote:
> >> >> On Mon, Jan 6, 2014 at 1:27 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >> >> > On Thu, Jan 02, 2014 at 08:25:10PM +1000, Peter Crosthwaite wrote:
> >> >> >> Hi Beniamino,
> >> >> >>
> >> >> >> On Thu, Jan 2, 2014 at 7:18 PM, Beniamino Galvani <b.galvani@gmail.com> wrote:
> >> >> >> > This patch adds support for the Fast Ethernet MAC found on Allwinner
> >> >> >> > SoCs, together with a basic emulation of Realtek RTL8201CP PHY.
> >> >> >> >
> >> >> >>
> >> >> >> More a comment for net in general, but I think sooner or later we need
> >> >> >> to move towards a split between phy and mac on the device level.
> >> >> >> continuing the phy-within-mac philosophy is going to make the
> >> >> >> socification efforts awkward. Are MII and friends a busses (as in
> >> >> >> TYPE_BUS) in their own right, and connection of mac and phy has to
> >> >> >> happen on the board level?
> >> >> >
> >> >> > I see PHY and MAC split as advantageous because it allows code reuse and
> >> >> > better testing.  The main thing I'd like to see is PHY device tests
> >> >> > using tests/libqtest.h.
> >> >> >
> >> >> > If someone wants to implement it, great.  It would make it easier to add
> >> >> > more NIC models in the future.
> >> >> >
> >> >> > Regarding SOCification and busses, I'm not sure.  Is it okay to just say
> >> >> > a NIC has-a PHY (i.e. composition)?
> >> >> >
> >> >>
> >> >> Generally speaking, in the (ARM) SoCification the MAC is part of the
> >> >> SoC which in the latest styling guidelines is a composite device. This
> >> >> composite is supposed to reflect the self contained SoC product which
> >> >> the PHY is usually not a part of. So we have two opposing compositions
> >> >> here:
> >> >>
> >> >> NIC = MAC + PHY
> >> >> SOC = CPUs + MAC + ...
> >> >>
> >> >> MAC can't be in both. So for SoCs the NIC concept needs to abandoned.
> >> >> After all the expansion of NIC as "Network Interface Card" is a little
> >> >> bit PCish. Your average SoC networking solution has no such "card".
> >> >> Just an on chip MAC (same pacakge/die as CPU etc) connecting to a PHY
> >> >> via PCB traces.
> >> >>
> >> >> So I think long term, MII has to be a TYPE_BUS that is visible on the
> >> >> top level SoC device. Self contained NICs (as we know them today) are
> >> >> then also implementable as container devices (of MAC and PHY) that use
> >> >> this bus internally (in much the same way the SoC boards would attach
> >> >> external PHY to SoC).
> >> >
> >> > Okay, that makes sense.  Given the amount of emulated hardware in QEMU
> >> > today, I think it would be okay to simply add new MAC/PHYs while still
> >> > supporting the NICs of old.  If someone is enthusiastic about
> >> > refactoring and testing existing NICs then great.  But I think it's more
> >> > pragmatic to simply start working with a split MAC/PHY where that is
> >> > beneficial.
> >> >
> >>
> >> Alright,
> >>
> >> So lets make some plans. There is devil in the detail here. There was
> >> a previous attempt to do something similar by Grant early last year so
> >> cc as FYI.
> >>
> >> So the main question is whether or not this new interface is just for
> >> MDIO or is the full MII interface (both MDIO and packet data).
> >>
> >> My inclination is the latter, we want a new proper QOM bus that is
> >> both. What this would mean, is that these MAC-only devices wont be net
> >> devices at all. the -net args are instead applied to the PHY. This
> >> makes the most sense to me as its the phy that actually has copper
> >> connection to the external network, not MAC.
> >>
> >> MAC <---- TYPE_MII_BUS ----> PHY <-----net layer ------> external
> >> network: "-net foo,bar,baz"
> >
> > I don't really agree with this. You can do ethernet without a PHY,
> > it is in fact fairly common in the embedded switch space. You can also
> > have a single MDIO iface that is completely separate from any MAC take
> > care of many PHYs.
> >
> > IMO, the MDIO bus should be separate from the data path.
> >
> >
> >> Another approach is to make both net devices in their own right. Phy
> >> has two net-layer-managed attachments, one for external network, and
> >> one point-to-point for the MII connecting to MAC. The MDIO bus is then
> >> a side channel which may or may not be QOMified (depending on effort
> >> levels). So you can still connect a standalone MAC to an external
> >> network, assuming the guest can handle no PHY (may in reality have
> >> limited use).
> >>
> >> MAC <---- net layer --------> PHY <-----net layer ------> external network
> >>     <---- TYPE_MDIO_BUS ---->
> >>
> >> OR:
> >>
> >> MAC <---- net layer --------> external network
> >>
> >>
> >> The third approach (which is closest to current implementation) is to
> >> only have the phy do MDIO and still connect the MAC straight to an
> >> external network:
> >>
> >> MAC <---- net layer --------> external network
> >>      \
> >>       <-- TYPE_MDIO_BUS ----> PHY
> >>
> >> I dont like this though, as its a little mismatched to real hw.
> >> Although it may be a good stepping stone to approaches 1 or 2.
> >
> > I'd go for this third one and possibly do something about the
> > data path later if needed.
> 
> Yeh, so I think that really translates to do option 3 and go to option
> 2 later if needed. So option 3 looks to be the winning plan for the
> first series.

I agree, option 3 is good.  We're trying to achieve a few things:

1. Model hardware visible to the guest
2. Allow flexibility to model various architectures (PCI NIC, MAC part
   of SoC, etc.)
3. Code reuse for MDIO PHY code

IMO we don't have to model things that are not visible to the guest.
Guests don't really care about the MII but they do care about MDIO for
link status, etc.

Stefan

Re: [Qemu-devel] [PATCH 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller

[Peter Crosthwaite]

On Thu, Jan 2, 2014 at 7:18 PM, Beniamino Galvani <b.galvani@gmail.com> wrote:
> This patch adds support for the Fast Ethernet MAC found on Allwinner
> SoCs, together with a basic emulation of Realtek RTL8201CP PHY.
>
> Since there is no public documentation of the Allwinner controller, the
> implementation is based on Linux kernel driver.
>
> Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
> ---
>  default-configs/arm-softmmu.mak |    1 +
>  hw/net/Makefile.objs            |    1 +
>  hw/net/allwinner_emac.c         |  466 +++++++++++++++++++++++++++++++++++++++
>  include/hw/net/allwinner_emac.h |  204 +++++++++++++++++
>  4 files changed, 672 insertions(+)
>  create mode 100644 hw/net/allwinner_emac.c
>  create mode 100644 include/hw/net/allwinner_emac.h
>
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index ce1d620..f3513fa 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -27,6 +27,7 @@ CONFIG_SSI_SD=y
>  CONFIG_SSI_M25P80=y
>  CONFIG_LAN9118=y
>  CONFIG_SMC91C111=y
> +CONFIG_ALLWINNER_EMAC=y
>  CONFIG_DS1338=y
>  CONFIG_PFLASH_CFI01=y
>  CONFIG_PFLASH_CFI02=y
> diff --git a/hw/net/Makefile.objs b/hw/net/Makefile.objs
> index 951cca3..75e80c2 100644
> --- a/hw/net/Makefile.objs
> +++ b/hw/net/Makefile.objs
> @@ -18,6 +18,7 @@ common-obj-$(CONFIG_OPENCORES_ETH) += opencores_eth.o
>  common-obj-$(CONFIG_XGMAC) += xgmac.o
>  common-obj-$(CONFIG_MIPSNET) += mipsnet.o
>  common-obj-$(CONFIG_XILINX_AXI) += xilinx_axienet.o
> +common-obj-$(CONFIG_ALLWINNER_EMAC) += allwinner_emac.o
>
>  common-obj-$(CONFIG_CADENCE) += cadence_gem.o
>  common-obj-$(CONFIG_STELLARIS_ENET) += stellaris_enet.o
> diff --git a/hw/net/allwinner_emac.c b/hw/net/allwinner_emac.c
> new file mode 100644
> index 0000000..9791be4
> --- /dev/null
> +++ b/hw/net/allwinner_emac.c
> @@ -0,0 +1,466 @@
> +/*
> + * Emulation of Allwinner EMAC Fast Ethernet controller and
> + * Realtek RTL8201CP PHY
> + *
> + * Copyright (C) 2013 Beniamino Galvani <b.galvani@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +#include "hw/sysbus.h"
> +#include "net/net.h"
> +#include "hw/net/allwinner_emac.h"
> +#include <zlib.h>
> +
> +#undef AW_EMAC_DEBUG
> +
> +#ifdef AW_EMAC_DEBUG
> +#define debug(...)                                              \
> +    do {                                                        \
> +        fprintf(stderr,  "allwinner_emac : %s: ", __func__);    \
> +        fprintf(stderr, ## __VA_ARGS__);                        \
> +    } while (0)
> +#else
> +#define debug(...) do {} while (0)
> +#endif
> +
> +static void mii_set_link(AwEmacMii *mii, bool link_ok)
> +{
> +    if (link_ok) {
> +        mii->bmsr |= MII_BMSR_LINK_ST;
> +        mii->anlpar |= MII_ANAR_TXFD | MII_ANAR_TX | MII_ANAR_10FD |
> +                       MII_ANAR_10 | MII_ANAR_CSMACD;
> +    } else {
> +        mii->bmsr &= ~MII_BMSR_LINK_ST;
> +        mii->anlpar = MII_ANAR_TX;
> +    }
> +    mii->link_ok = link_ok;
> +}
> +
> +static void mii_reset(AwEmacMii *mii)
> +{
> +    mii->bmcr = MII_BMCR_FD | MII_BMCR_AUTOEN | MII_BMCR_SPEED;
> +    mii->bmsr = MII_BMSR_100TX_FD | MII_BMSR_100TX_HD | MII_BMSR_10T_FD |
> +                MII_BMSR_10T_HD | MII_BMSR_MFPS | MII_BMSR_AUTONEG;
> +    mii->anar = MII_ANAR_TXFD | MII_ANAR_TX | MII_ANAR_10FD | MII_ANAR_10 |
> +                MII_ANAR_CSMACD;
> +    mii->anlpar = MII_ANAR_TX;
> +    mii_set_link(mii, mii->link_ok);
> +}
> +
> +static uint16_t mii_read(AwEmacMii *mii, uint8_t phy_addr, uint8_t reg)
> +{
> +    uint16_t ret = 0xffff;
> +
> +    if (phy_addr == BOARD_PHY_ADDRESS) {
> +        switch (reg) {
> +        case MII_BMCR:
> +            ret = mii->bmcr;
> +            break;
> +        case MII_BMSR:
> +            ret = mii->bmsr;
> +            break;
> +        case MII_PHYID1:
> +            ret = RTL8201CP_PHYID1;
> +            break;
> +        case MII_PHYID2:
> +            ret = RTL8201CP_PHYID2;
> +            break;
> +        case MII_ANAR:
> +            ret = mii->anar;
> +            break;
> +        case MII_ANLPAR:
> +            ret = mii->anlpar;
> +            break;
> +        default:
> +            debug("unknown mii register %x\n", reg);
> +            ret = 0;
> +        }
> +    }
> +    return ret;
> +}
> +
> +static void mii_write(AwEmacMii *mii, uint8_t phy_addr, uint8_t reg,
> +                      uint16_t value)
> +{
> +    if (phy_addr == BOARD_PHY_ADDRESS) {
> +        switch (reg) {
> +        case MII_BMCR:
> +            if (value & MII_BMCR_RESET) {
> +                mii_reset(mii);
> +            } else {
> +                mii->bmcr = value;
> +            }
> +            break;
> +        case MII_BMSR:
> +        case MII_PHYID1:
> +        case MII_PHYID2:
> +        case MII_ANLPAR:
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: write to mii register %x\n",
> +                          __func__, reg);
> +            break;
> +        case MII_ANAR:
> +            mii->anar = value;
> +            break;
> +        default:
> +            debug("unknown mii register %x\n", reg);
> +        }
> +    }
> +}
> +
> +static void aw_emac_update_irq(AwEmacState *s)
> +{
> +    qemu_set_irq(s->irq, (s->int_sta & s->int_ctl) != 0);
> +}
> +
> +static int aw_emac_can_receive(NetClientState *nc)
> +{
> +    AwEmacState *s = qemu_get_nic_opaque(nc);
> +
> +    return (s->ctl & EMAC_CTL_RX_EN) && (s->num_rx < MAX_RX);
> +}
> +
> +static ssize_t aw_emac_receive(NetClientState *nc, const uint8_t *buf,
> +                               size_t size)
> +{
> +    AwEmacState *s = qemu_get_nic_opaque(nc);
> +    uint32_t *fifo;
> +    uint32_t crc;
> +    char *dest;
> +
> +    if (s->num_rx >= MAX_RX) {
> +        debug("rx queue full - packed dropped\n");
> +        return -1;
> +    }
> +
> +    if (size + RX_HDR_SIZE > FIFO_SIZE) {
> +        debug("packet too big\n");
> +        return -1;
> +    }
> +
> +    fifo = s->rx_fifos[(s->first_rx + s->num_rx) % MAX_RX];
> +    dest = (char *)&fifo[2];
> +    s->num_rx++;
> +
> +    memcpy(dest, buf, size);
> +
> +    /* Fill to minimum frame length */
> +    if (size < 60) {
> +        memset(dest + size, 0, 60 - size);
> +        size = 60;
> +    }
> +
> +    /* Append FCS */
> +    crc = crc32(~0, buf, size);
> +    memcpy(dest + size, &crc, 4);
> +
> +    fifo[0] = EMAC_UNDOCUMENTED_MAGIC;
> +    fifo[1] = EMAC_RX_HEADER(size + 4, EMAC_RX_IO_DATA_STATUS_OK);
> +
> +    /* Set rx interrupt flag */
> +    s->int_sta |= EMAC_INT_RX;
> +    aw_emac_update_irq(s);
> +
> +    return size;
> +}
> +
> +static void aw_emac_cleanup(NetClientState *nc)
> +{
> +    AwEmacState *s = qemu_get_nic_opaque(nc);
> +
> +    s->nic = NULL;
> +}
> +
> +static void aw_emac_reset(AwEmacState *s)
> +{
> +    s->ctl = 0;
> +    s->tx_mode = 0;
> +    s->int_ctl = 0;
> +    s->int_sta = 0;
> +    s->tx_channel = 0;
> +    s->first_rx = 0;
> +    s->num_rx = 0;
> +    s->rx_offset = 0;
> +    s->phy_target = 0;
> +}
> +
> +static uint64_t aw_emac_read(void *opaque, hwaddr offset, unsigned size)
> +{
> +    AwEmacState *s = opaque;
> +    uint64_t ret;
> +    uint32_t *rx_fifo;
> +
> +    switch (offset) {
> +    case EMAC_CTL_REG:
> +        ret = s->ctl;
> +        break;
> +    case EMAC_TX_MODE_REG:
> +        ret = s->tx_mode;
> +        break;
> +    case EMAC_TX_INS_REG:
> +        ret = s->tx_channel;
> +        break;
> +    case EMAC_RX_CTL_REG:
> +        ret = s->rx_ctl;
> +        break;
> +    case EMAC_RX_IO_DATA_REG:
> +        if (!s->num_rx) {
> +            ret = 0;
> +            break;
> +        }
> +        rx_fifo = s->rx_fifos[s->first_rx];
> +
> +        if (s->rx_offset >= FIFO_SIZE ||
> +            s->rx_offset >= EMAC_RX_IO_DATA_LEN(rx_fifo[1]) + RX_HDR_SIZE) {
> +            /* This should never happen */
> +            debug("RX fifo overrun\n");
> +            s->first_rx = (s->first_rx + 1) % MAX_RX;
> +            s->num_rx--;
> +            s->rx_offset = 0;
> +            ret = 0;
> +            break;
> +        }
> +
> +        ret = rx_fifo[s->rx_offset >> 2];
> +        s->rx_offset += 4;
> +
> +        if (s->rx_offset >= EMAC_RX_IO_DATA_LEN(rx_fifo[1]) + RX_HDR_SIZE) {
> +            s->first_rx = (s->first_rx + 1) % MAX_RX;
> +            s->num_rx--;
> +            s->rx_offset = 0;
> +        }
> +        break;
> +    case EMAC_RX_FBC_REG:
> +        ret = s->num_rx;
> +        break;
> +    case EMAC_INT_CTL_REG:
> +        ret = s->int_ctl;
> +        break;
> +    case EMAC_INT_STA_REG:
> +        ret = s->int_sta;
> +        break;
> +    case EMAC_MAC_MRDD_REG:
> +        ret = mii_read(&s->mii, PHY_TARGET_ADDR(s->phy_target),
> +                       PHY_TARGET_REG(s->phy_target));
> +        break;
> +    default:
> +        debug("unknown offset %08x\n", (unsigned int)offset);
> +        ret = 0;
> +    }
> +
> +    return ret;
> +}
> +
> +static void aw_emac_write(void *opaque, hwaddr offset, uint64_t value,
> +                          unsigned size)
> +{
> +    AwEmacState *s = opaque;
> +    AwEmacTxFifo *fifo;
> +    int chan;
> +
> +    switch (offset) {
> +    case EMAC_CTL_REG:
> +        if (value & EMAC_CTL_RESET) {
> +            debug("reset\n");
> +            aw_emac_reset(s);
> +            value &= ~EMAC_CTL_RESET;
> +        }
> +        s->ctl = value;
> +        break;
> +    case EMAC_TX_MODE_REG:
> +        s->tx_mode = value;
> +        break;
> +    case EMAC_TX_CTL0_REG:
> +    case EMAC_TX_CTL1_REG:
> +        chan = (offset == EMAC_TX_CTL0_REG ? 0 : 1);
> +        if ((value & 1) && (s->ctl & EMAC_CTL_TX_EN)) {
> +            qemu_send_packet(qemu_get_queue(s->nic),
> +                             (uint8_t *)s->tx_fifos[chan].data,

So this cast here is perhaps suspicious. Your write handler just loads
uint32_t values from the bus access into s->tx_fifos[chan] without any
endianness checking. This means that s->tx_fifos[chan].data is a
collection of uint32_t's of host endianness.

Ultimately, the net layer wants a sequence of bytes (uint8_t *) while
the bus access logic is thinking in (uint32_t *). I would suggest
going lowest common denominitor with uint8_t * as your FIFO data type.
Implement your endianess check in the bus access.

> +                             s->tx_fifos[chan].length);
> +
> +            /* Raise TX interrupt */
> +            s->int_sta |= EMAC_INT_TX_CHAN(chan);
> +            s->tx_fifos[chan].offset = 0;
> +            aw_emac_update_irq(s);
> +        }
> +        break;
> +    case EMAC_TX_INS_REG:
> +        s->tx_channel = value < NUM_CHAN ? value : 0;
> +        break;
> +    case EMAC_TX_PL0_REG:
> +    case EMAC_TX_PL1_REG:
> +        chan = (offset == EMAC_TX_PL0_REG ? 0 : 1);
> +        if (value > FIFO_SIZE) {
> +            debug("invalid TX frame length %d\n", (int)value);
> +            value = FIFO_SIZE;
> +        }
> +        s->tx_fifos[chan].length = value;
> +        break;
> +    case EMAC_TX_IO_DATA_REG:
> +        fifo = &s->tx_fifos[s->tx_channel];
> +        if (fifo->offset >= FIFO_SIZE) {
> +            debug("TX frame data overruns fifo (%d)\n", fifo->offset);
> +            break;
> +        }
> +        fifo->data[fifo->offset >> 2] = value;

This is where the endianess issue occurs. Also, if you convert to
uint8_t * you can ditch on the shift. Theres probably some neat way to
do this using cpu_to_le32 (or_be, depending on what the device is) to
avoid a 4 iteration byte loop.

> +        fifo->offset += 4;
> +        break;
> +    case EMAC_RX_CTL_REG:
> +        s->rx_ctl = value;
> +        break;
> +    case EMAC_RX_FBC_REG:
> +        if (value == 0) {
> +            s->num_rx = 0;
> +        }
> +        break;
> +    case EMAC_INT_CTL_REG:
> +        s->int_ctl = value;
> +        break;
> +    case EMAC_INT_STA_REG:
> +        s->int_sta &= ~value;
> +        break;
> +    case EMAC_MAC_MADR_REG:
> +        s->phy_target = value;
> +        break;
> +    case EMAC_MAC_MWTD_REG:
> +        mii_write(&s->mii, PHY_TARGET_ADDR(s->phy_target),
> +                  PHY_TARGET_REG(s->phy_target), value);
> +        break;
> +    default:
> +        debug("unknown offset %08x\n", (unsigned int)offset);
> +    }
> +}
> +
> +static void aw_emac_set_link(NetClientState *nc)
> +{
> +    AwEmacState *s = qemu_get_nic_opaque(nc);
> +
> +    mii_set_link(&s->mii, !nc->link_down);
> +}
> +
> +static const MemoryRegionOps aw_emac_mem_ops = {
> +    .read = aw_emac_read,
> +    .write = aw_emac_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static NetClientInfo net_aw_emac_info = {
> +    .type = NET_CLIENT_OPTIONS_KIND_NIC,
> +    .size = sizeof(NICState),
> +    .can_receive = aw_emac_can_receive,
> +    .receive = aw_emac_receive,
> +    .cleanup = aw_emac_cleanup,
> +    .link_status_changed = aw_emac_set_link,
> +};
> +
> +static void aw_emac_init(Object *obj)
> +{
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +    AwEmacState *s = AW_EMAC(obj);
> +
> +    memory_region_init_io(&s->iomem, OBJECT(s), &aw_emac_mem_ops, s,
> +                          "aw_emac", 0x1000);
> +    sysbus_init_mmio(sbd, &s->iomem);
> +    sysbus_init_irq(sbd, &s->irq);
> +}
> +
> +static void aw_emac_realize(DeviceState *dev, Error **errp)
> +{
> +    AwEmacState *s = AW_EMAC(dev);
> +
> +    qemu_macaddr_default_if_unset(&s->conf.macaddr);
> +
> +    s->nic = qemu_new_nic(&net_aw_emac_info, &s->conf,
> +                          object_get_typename(OBJECT(dev)), dev->id, s);
> +    qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
> +
> +    aw_emac_reset(s);
> +    aw_emac_set_link(qemu_get_queue(s->nic));
> +    mii_reset(&s->mii);
> +}
> +
> +static Property aw_emac_properties[] = {
> +    DEFINE_NIC_PROPERTIES(AwEmacState, conf),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static const VMStateDescription vmstate_mii = {
> +    .name = "allwinner_emac_mii",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT16(bmcr, AwEmacMii),
> +        VMSTATE_UINT16(bmsr, AwEmacMii),
> +        VMSTATE_UINT16(anar, AwEmacMii),
> +        VMSTATE_UINT16(anlpar, AwEmacMii),
> +        VMSTATE_BOOL(link_ok, AwEmacMii),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static const VMStateDescription vmstate_tx_fifo = {
> +    .name = "allwinner_emac_tx_fifo",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32_ARRAY(data, AwEmacTxFifo, FIFO_SIZE >> 2),
> +        VMSTATE_INT32(length, AwEmacTxFifo),
> +        VMSTATE_INT32(offset, AwEmacTxFifo),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static const VMStateDescription vmstate_aw_emac = {
> +    .name = "allwinner_emac",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_STRUCT(mii, AwEmacState, 1, vmstate_mii, AwEmacMii),
> +        VMSTATE_UINT32(ctl, AwEmacState),
> +        VMSTATE_UINT32(tx_mode, AwEmacState),
> +        VMSTATE_UINT32(rx_ctl, AwEmacState),
> +        VMSTATE_UINT32(int_ctl, AwEmacState),
> +        VMSTATE_UINT32(int_sta, AwEmacState),
> +        VMSTATE_UINT32(phy_target, AwEmacState),
> +        VMSTATE_INT32(tx_channel, AwEmacState),
> +        VMSTATE_STRUCT_ARRAY(tx_fifos, AwEmacState,
> +                             NUM_CHAN, 1, vmstate_tx_fifo, AwEmacTxFifo),
> +        VMSTATE_BUFFER_UNSAFE(tx_fifos, AwEmacState, 1, MAX_RX * FIFO_SIZE),
> +        VMSTATE_INT32(first_rx, AwEmacState),
> +        VMSTATE_INT32(num_rx, AwEmacState),
> +        VMSTATE_INT32(rx_offset, AwEmacState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void aw_emac_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = aw_emac_realize;
> +    dc->props = aw_emac_properties;
> +    dc->vmsd = &vmstate_aw_emac;
> +}
> +
> +static const TypeInfo aw_emac_info = {
> +    .name           = TYPE_AW_EMAC,
> +    .parent         = TYPE_SYS_BUS_DEVICE,
> +    .instance_size  = sizeof(AwEmacState),
> +    .instance_init   = aw_emac_init,
> +    .class_init     = aw_emac_class_init,
> +};
> +
> +static void aw_emac_register_types(void)
> +{
> +    type_register_static(&aw_emac_info);
> +}
> +
> +type_init(aw_emac_register_types)
> diff --git a/include/hw/net/allwinner_emac.h b/include/hw/net/allwinner_emac.h
> new file mode 100644
> index 0000000..f9f9682
> --- /dev/null
> +++ b/include/hw/net/allwinner_emac.h
> @@ -0,0 +1,204 @@
> +/*
> + * Emulation of Allwinner EMAC Fast Ethernet controller and
> + * Realtek RTL8201CP PHY
> + *
> + * Copyright (C) 2013 Beniamino Galvani <b.galvani@gmail.com>
> + *
> + * Allwinner EMAC register definitions from Linux kernel are:
> + *   Copyright 2012 Stefan Roese <sr@denx.de>
> + *   Copyright 2013 Maxime Ripard <maxime.ripard@free-electrons.com>
> + *   Copyright 1997 Sten Wang
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +#ifndef AW_EMAC_H
> +#define AW_EMAC_H
> +
> +#include "net/net.h"
> +
> +#define TYPE_AW_EMAC "allwinner_emac"
> +#define AW_EMAC(obj) OBJECT_CHECK(AwEmacState, (obj), TYPE_AW_EMAC)
> +
> +/*
> + * Allwinner EMAC register list
> + */
> +#define EMAC_CTL_REG            0x00
> +#define EMAC_TX_MODE_REG        0x04
> +#define EMAC_TX_FLOW_REG        0x08
> +#define EMAC_TX_CTL0_REG        0x0C
> +#define EMAC_TX_CTL1_REG        0x10
> +#define EMAC_TX_INS_REG         0x14
> +#define EMAC_TX_PL0_REG         0x18
> +#define EMAC_TX_PL1_REG         0x1C
> +#define EMAC_TX_STA_REG         0x20
> +#define EMAC_TX_IO_DATA_REG     0x24
> +#define EMAC_TX_IO_DATA1_REG    0x28
> +#define EMAC_TX_TSVL0_REG       0x2C
> +#define EMAC_TX_TSVH0_REG       0x30
> +#define EMAC_TX_TSVL1_REG       0x34
> +#define EMAC_TX_TSVH1_REG       0x38
> +#define EMAC_RX_CTL_REG         0x3C
> +#define EMAC_RX_HASH0_REG       0x40
> +#define EMAC_RX_HASH1_REG       0x44
> +#define EMAC_RX_STA_REG         0x48
> +#define EMAC_RX_IO_DATA_REG     0x4C
> +#define EMAC_RX_FBC_REG         0x50
> +#define EMAC_INT_CTL_REG        0x54
> +#define EMAC_INT_STA_REG        0x58
> +#define EMAC_MAC_CTL0_REG       0x5C
> +#define EMAC_MAC_CTL1_REG       0x60
> +#define EMAC_MAC_IPGT_REG       0x64
> +#define EMAC_MAC_IPGR_REG       0x68
> +#define EMAC_MAC_CLRT_REG       0x6C
> +#define EMAC_MAC_MAXF_REG       0x70
> +#define EMAC_MAC_SUPP_REG       0x74
> +#define EMAC_MAC_TEST_REG       0x78
> +#define EMAC_MAC_MCFG_REG       0x7C
> +#define EMAC_MAC_MCMD_REG       0x80
> +#define EMAC_MAC_MADR_REG       0x84
> +#define EMAC_MAC_MWTD_REG       0x88
> +#define EMAC_MAC_MRDD_REG       0x8C
> +#define EMAC_MAC_MIND_REG       0x90
> +#define EMAC_MAC_SSRR_REG       0x94
> +#define EMAC_MAC_A0_REG         0x98
> +#define EMAC_MAC_A1_REG         0x9C
> +#define EMAC_MAC_A2_REG         0xA0
> +#define EMAC_SAFX_L_REG0        0xA4
> +#define EMAC_SAFX_H_REG0        0xA8
> +#define EMAC_SAFX_L_REG1        0xAC
> +#define EMAC_SAFX_H_REG1        0xB0
> +#define EMAC_SAFX_L_REG2        0xB4
> +#define EMAC_SAFX_H_REG2        0xB8
> +#define EMAC_SAFX_L_REG3        0xBC
> +#define EMAC_SAFX_H_REG3        0xC0
> +
> +/* CTL register fields */
> +#define EMAC_CTL_RESET                  (1 << 0)
> +#define EMAC_CTL_TX_EN                  (1 << 1)
> +#define EMAC_CTL_RX_EN                  (1 << 2)
> +
> +/* TX MODE register fields */
> +#define EMAC_TX_MODE_ABORTED_FRAME_EN   (1 << 0)
> +#define EMAC_TX_MODE_DMA_EN             (1 << 1)
> +
> +/* RX CTL register fields */
> +#define EMAC_RX_CTL_AUTO_DRQ_EN         (1 << 1)
> +#define EMAC_RX_CTL_DMA_EN              (1 << 2)
> +#define EMAC_RX_CTL_PASS_ALL_EN         (1 << 4)
> +#define EMAC_RX_CTL_PASS_CTL_EN         (1 << 5)
> +#define EMAC_RX_CTL_PASS_CRC_ERR_EN     (1 << 6)
> +#define EMAC_RX_CTL_PASS_LEN_ERR_EN     (1 << 7)
> +#define EMAC_RX_CTL_PASS_LEN_OOR_EN     (1 << 8)
> +#define EMAC_RX_CTL_ACCEPT_UNICAST_EN   (1 << 16)
> +#define EMAC_RX_CTL_DA_FILTER_EN        (1 << 17)
> +#define EMAC_RX_CTL_ACCEPT_MULTICAST_EN (1 << 20)
> +#define EMAC_RX_CTL_HASH_FILTER_EN      (1 << 21)
> +#define EMAC_RX_CTL_ACCEPT_BROADCAST_EN (1 << 22)
> +#define EMAC_RX_CTL_SA_FILTER_EN        (1 << 24)
> +#define EMAC_RX_CTL_SA_FILTER_INVERT_EN (1 << 25)
> +
> +/* RX IO DATA register fields */
> +#define EMAC_RX_IO_DATA_LEN(x)          (x & 0xffff)
> +#define EMAC_RX_IO_DATA_STATUS(x)       ((x >> 16) & 0xffff)
> +#define EMAC_RX_HEADER(len, status)     (((len) & 0xffff) | ((status) << 16))
> +#define EMAC_RX_IO_DATA_STATUS_CRC_ERR  (1 << 4)
> +#define EMAC_RX_IO_DATA_STATUS_LEN_ERR  (3 << 5)
> +#define EMAC_RX_IO_DATA_STATUS_OK       (1 << 7)
> +#define EMAC_UNDOCUMENTED_MAGIC         0x0143414d  /* header for RX frames */
> +
> +/* PHY registers */
> +#define MII_BMCR            0
> +#define MII_BMSR            1
> +#define MII_PHYID1          2
> +#define MII_PHYID2          3
> +#define MII_ANAR            4
> +#define MII_ANLPAR          5
> +
> +/* PHY registers fields */
> +#define MII_BMCR_RESET      (1 << 15)
> +#define MII_BMCR_LOOPBACK   (1 << 14)
> +#define MII_BMCR_SPEED      (1 << 13)
> +#define MII_BMCR_AUTOEN     (1 << 12)
> +#define MII_BMCR_FD         (1 << 8)
> +
> +#define MII_BMSR_100TX_FD   (1 << 14)
> +#define MII_BMSR_100TX_HD   (1 << 13)
> +#define MII_BMSR_10T_FD     (1 << 12)
> +#define MII_BMSR_10T_HD     (1 << 11)
> +#define MII_BMSR_MFPS       (1 << 6)
> +#define MII_BMSR_AUTONEG    (1 << 3)
> +#define MII_BMSR_LINK_ST    (1 << 2)
> +
> +#define MII_ANAR_TXFD       (1 << 8)
> +#define MII_ANAR_TX         (1 << 7)
> +#define MII_ANAR_10FD       (1 << 6)
> +#define MII_ANAR_10         (1 << 5)
> +#define MII_ANAR_CSMACD     (1 << 0)
> +
> +#define RTL8201CP_PHYID1    0x0000
> +#define RTL8201CP_PHYID2    0x8201
> +
> +/* INT CTL and INT STA registers fields */
> +#define EMAC_INT_TX_CHAN(x) (1 << (x))
> +#define EMAC_INT_RX         (1 << 8)
> +
> +#define BOARD_PHY_ADDRESS   1
> +
> +#define NUM_CHAN            2
> +#define FIFO_SIZE           2048
> +#define MAX_RX              12
> +#define RX_HDR_SIZE         8
> +
> +#define PHY_TARGET_ADDR(x)  (((x) >> 8) & 0xff)
> +#define PHY_TARGET_REG(x)   ((x) & 0xff)
> +
> +typedef struct AwEmacTxFifo {
> +    uint32_t data[FIFO_SIZE >> 2];

uint8_t data[FIFO_SIZE];


> +    int      length;
> +    int      offset;
> +} AwEmacTxFifo;
> +
> +typedef struct AwEmacMii {
> +    uint16_t bmcr;
> +    uint16_t bmsr;
> +    uint16_t anar;
> +    uint16_t anlpar;
> +    bool     link_ok;
> +} AwEmacMii;
> +
> +typedef struct AwEmacState {
> +    /*< private >*/
> +    SysBusDevice  parent_obj;
> +    /*< public >*/
> +
> +    MemoryRegion  iomem;
> +    qemu_irq      irq;
> +    NICState      *nic;
> +    NICConf       conf;
> +    AwEmacMii     mii;
> +
> +    uint32_t      ctl;
> +    uint32_t      tx_mode;
> +    uint32_t      rx_ctl;
> +    uint32_t      int_ctl;
> +    uint32_t      int_sta;
> +    uint32_t      phy_target;
> +
> +    int           tx_channel;             /* Currently selected TX channel */
> +    AwEmacTxFifo  tx_fifos[NUM_CHAN];
> +    uint32_t      rx_fifos[MAX_RX][FIFO_SIZE >> 2];
> +    int           first_rx;               /* First packet in the RX ring */
> +    int           num_rx;                 /* Number of packets in RX ring */
> +    int           rx_offset;              /* Offset of next read */
> +} AwEmacState;
> +
> +#endif
> --
> 1.7.10.4
>

I'll look into your direct buffer access problem hopefully soon, and
see what can be done to make Fifo8 DMA (for want of a better term)
friendly. I'm thinking two functions:

/** pop a number of elements from the FIFO up to a maximum of max. The
buffer containing the popped data is returned. This buffer points
directly into the Fifo backing store and data is invalidated once any
of the fifo_* APIs are called on the FIFO. May short return, the
number of valid bytes returned in populated in *num. Will always
return at least 1 byte. Behavior is undefined if max > the number of
bytes in the Fifo or max == 0.
*/

const uint8_t *fifo_pop_buf(Fifo *fifo, uint32_t max, uint32_t *num);

/** Pop num bytes from the Fifo into a newly created buffer. Behavior
is undefined if num > the number of bytes in the Fifo. The caller is
responsible for freeing the returned buffer via g_free. */

uint8_t *fifo_pop_all(Fifo *fifo, uint32_t num);

fifo_push is much simpler. Just as simple as:

void fifo_push_all(Fifo *fifo, uint32_t num)

Would this handle your cases without need for roll-your-own Fifo's?

Regards,
Peter

>

Re: [Qemu-devel] [PATCH 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller

[Beniamino Galvani]

On Sat, Jan 04, 2014 at 10:56:13AM +1000, Peter Crosthwaite wrote:
> On Thu, Jan 2, 2014 at 7:18 PM, Beniamino Galvani <b.galvani@gmail.com> wrote:
> > This patch adds support for the Fast Ethernet MAC found on Allwinner
> > SoCs, together with a basic emulation of Realtek RTL8201CP PHY.
> >
> > Since there is no public documentation of the Allwinner controller, the
> > implementation is based on Linux kernel driver.
> >
> > Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
> > ---
> >  default-configs/arm-softmmu.mak |    1 +
> >  hw/net/Makefile.objs            |    1 +
> >  hw/net/allwinner_emac.c         |  466 +++++++++++++++++++++++++++++++++++++++
> >  include/hw/net/allwinner_emac.h |  204 +++++++++++++++++
> >  4 files changed, 672 insertions(+)
> >  create mode 100644 hw/net/allwinner_emac.c
> >  create mode 100644 include/hw/net/allwinner_emac.h
> >
> > diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> > index ce1d620..f3513fa 100644
> > --- a/default-configs/arm-softmmu.mak
> > +++ b/default-configs/arm-softmmu.mak
> > @@ -27,6 +27,7 @@ CONFIG_SSI_SD=y
> >  CONFIG_SSI_M25P80=y
> >  CONFIG_LAN9118=y
> >  CONFIG_SMC91C111=y
> > +CONFIG_ALLWINNER_EMAC=y
> >  CONFIG_DS1338=y
> >  CONFIG_PFLASH_CFI01=y
> >  CONFIG_PFLASH_CFI02=y
> > diff --git a/hw/net/Makefile.objs b/hw/net/Makefile.objs
> > index 951cca3..75e80c2 100644
> > --- a/hw/net/Makefile.objs
> > +++ b/hw/net/Makefile.objs
> > @@ -18,6 +18,7 @@ common-obj-$(CONFIG_OPENCORES_ETH) += opencores_eth.o
> >  common-obj-$(CONFIG_XGMAC) += xgmac.o
> >  common-obj-$(CONFIG_MIPSNET) += mipsnet.o
> >  common-obj-$(CONFIG_XILINX_AXI) += xilinx_axienet.o
> > +common-obj-$(CONFIG_ALLWINNER_EMAC) += allwinner_emac.o
> >
> >  common-obj-$(CONFIG_CADENCE) += cadence_gem.o
> >  common-obj-$(CONFIG_STELLARIS_ENET) += stellaris_enet.o
> > diff --git a/hw/net/allwinner_emac.c b/hw/net/allwinner_emac.c
> > new file mode 100644
> > index 0000000..9791be4
> > --- /dev/null
> > +++ b/hw/net/allwinner_emac.c
> > @@ -0,0 +1,466 @@
> > +/*
> > + * Emulation of Allwinner EMAC Fast Ethernet controller and
> > + * Realtek RTL8201CP PHY
> > + *
> > + * Copyright (C) 2013 Beniamino Galvani <b.galvani@gmail.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + */
> > +#include "hw/sysbus.h"
> > +#include "net/net.h"
> > +#include "hw/net/allwinner_emac.h"
> > +#include <zlib.h>
> > +
> > +#undef AW_EMAC_DEBUG
> > +
> > +#ifdef AW_EMAC_DEBUG
> > +#define debug(...)                                              \
> > +    do {                                                        \
> > +        fprintf(stderr,  "allwinner_emac : %s: ", __func__);    \
> > +        fprintf(stderr, ## __VA_ARGS__);                        \
> > +    } while (0)
> > +#else
> > +#define debug(...) do {} while (0)
> > +#endif
> > +
> > +static void mii_set_link(AwEmacMii *mii, bool link_ok)
> > +{
> > +    if (link_ok) {
> > +        mii->bmsr |= MII_BMSR_LINK_ST;
> > +        mii->anlpar |= MII_ANAR_TXFD | MII_ANAR_TX | MII_ANAR_10FD |
> > +                       MII_ANAR_10 | MII_ANAR_CSMACD;
> > +    } else {
> > +        mii->bmsr &= ~MII_BMSR_LINK_ST;
> > +        mii->anlpar = MII_ANAR_TX;
> > +    }
> > +    mii->link_ok = link_ok;
> > +}
> > +
> > +static void mii_reset(AwEmacMii *mii)
> > +{
> > +    mii->bmcr = MII_BMCR_FD | MII_BMCR_AUTOEN | MII_BMCR_SPEED;
> > +    mii->bmsr = MII_BMSR_100TX_FD | MII_BMSR_100TX_HD | MII_BMSR_10T_FD |
> > +                MII_BMSR_10T_HD | MII_BMSR_MFPS | MII_BMSR_AUTONEG;
> > +    mii->anar = MII_ANAR_TXFD | MII_ANAR_TX | MII_ANAR_10FD | MII_ANAR_10 |
> > +                MII_ANAR_CSMACD;
> > +    mii->anlpar = MII_ANAR_TX;
> > +    mii_set_link(mii, mii->link_ok);
> > +}
> > +
> > +static uint16_t mii_read(AwEmacMii *mii, uint8_t phy_addr, uint8_t reg)
> > +{
> > +    uint16_t ret = 0xffff;
> > +
> > +    if (phy_addr == BOARD_PHY_ADDRESS) {
> > +        switch (reg) {
> > +        case MII_BMCR:
> > +            ret = mii->bmcr;
> > +            break;
> > +        case MII_BMSR:
> > +            ret = mii->bmsr;
> > +            break;
> > +        case MII_PHYID1:
> > +            ret = RTL8201CP_PHYID1;
> > +            break;
> > +        case MII_PHYID2:
> > +            ret = RTL8201CP_PHYID2;
> > +            break;
> > +        case MII_ANAR:
> > +            ret = mii->anar;
> > +            break;
> > +        case MII_ANLPAR:
> > +            ret = mii->anlpar;
> > +            break;
> > +        default:
> > +            debug("unknown mii register %x\n", reg);
> > +            ret = 0;
> > +        }
> > +    }
> > +    return ret;
> > +}
> > +
> > +static void mii_write(AwEmacMii *mii, uint8_t phy_addr, uint8_t reg,
> > +                      uint16_t value)
> > +{
> > +    if (phy_addr == BOARD_PHY_ADDRESS) {
> > +        switch (reg) {
> > +        case MII_BMCR:
> > +            if (value & MII_BMCR_RESET) {
> > +                mii_reset(mii);
> > +            } else {
> > +                mii->bmcr = value;
> > +            }
> > +            break;
> > +        case MII_BMSR:
> > +        case MII_PHYID1:
> > +        case MII_PHYID2:
> > +        case MII_ANLPAR:
> > +            qemu_log_mask(LOG_GUEST_ERROR, "%s: write to mii register %x\n",
> > +                          __func__, reg);
> > +            break;
> > +        case MII_ANAR:
> > +            mii->anar = value;
> > +            break;
> > +        default:
> > +            debug("unknown mii register %x\n", reg);
> > +        }
> > +    }
> > +}
> > +
> > +static void aw_emac_update_irq(AwEmacState *s)
> > +{
> > +    qemu_set_irq(s->irq, (s->int_sta & s->int_ctl) != 0);
> > +}
> > +
> > +static int aw_emac_can_receive(NetClientState *nc)
> > +{
> > +    AwEmacState *s = qemu_get_nic_opaque(nc);
> > +
> > +    return (s->ctl & EMAC_CTL_RX_EN) && (s->num_rx < MAX_RX);
> > +}
> > +
> > +static ssize_t aw_emac_receive(NetClientState *nc, const uint8_t *buf,
> > +                               size_t size)
> > +{
> > +    AwEmacState *s = qemu_get_nic_opaque(nc);
> > +    uint32_t *fifo;
> > +    uint32_t crc;
> > +    char *dest;
> > +
> > +    if (s->num_rx >= MAX_RX) {
> > +        debug("rx queue full - packed dropped\n");
> > +        return -1;
> > +    }
> > +
> > +    if (size + RX_HDR_SIZE > FIFO_SIZE) {
> > +        debug("packet too big\n");
> > +        return -1;
> > +    }
> > +
> > +    fifo = s->rx_fifos[(s->first_rx + s->num_rx) % MAX_RX];
> > +    dest = (char *)&fifo[2];
> > +    s->num_rx++;
> > +
> > +    memcpy(dest, buf, size);
> > +
> > +    /* Fill to minimum frame length */
> > +    if (size < 60) {
> > +        memset(dest + size, 0, 60 - size);
> > +        size = 60;
> > +    }
> > +
> > +    /* Append FCS */
> > +    crc = crc32(~0, buf, size);
> > +    memcpy(dest + size, &crc, 4);
> > +
> > +    fifo[0] = EMAC_UNDOCUMENTED_MAGIC;
> > +    fifo[1] = EMAC_RX_HEADER(size + 4, EMAC_RX_IO_DATA_STATUS_OK);
> > +
> > +    /* Set rx interrupt flag */
> > +    s->int_sta |= EMAC_INT_RX;
> > +    aw_emac_update_irq(s);
> > +
> > +    return size;
> > +}
> > +
> > +static void aw_emac_cleanup(NetClientState *nc)
> > +{
> > +    AwEmacState *s = qemu_get_nic_opaque(nc);
> > +
> > +    s->nic = NULL;
> > +}
> > +
> > +static void aw_emac_reset(AwEmacState *s)
> > +{
> > +    s->ctl = 0;
> > +    s->tx_mode = 0;
> > +    s->int_ctl = 0;
> > +    s->int_sta = 0;
> > +    s->tx_channel = 0;
> > +    s->first_rx = 0;
> > +    s->num_rx = 0;
> > +    s->rx_offset = 0;
> > +    s->phy_target = 0;
> > +}
> > +
> > +static uint64_t aw_emac_read(void *opaque, hwaddr offset, unsigned size)
> > +{
> > +    AwEmacState *s = opaque;
> > +    uint64_t ret;
> > +    uint32_t *rx_fifo;
> > +
> > +    switch (offset) {
> > +    case EMAC_CTL_REG:
> > +        ret = s->ctl;
> > +        break;
> > +    case EMAC_TX_MODE_REG:
> > +        ret = s->tx_mode;
> > +        break;
> > +    case EMAC_TX_INS_REG:
> > +        ret = s->tx_channel;
> > +        break;
> > +    case EMAC_RX_CTL_REG:
> > +        ret = s->rx_ctl;
> > +        break;
> > +    case EMAC_RX_IO_DATA_REG:
> > +        if (!s->num_rx) {
> > +            ret = 0;
> > +            break;
> > +        }
> > +        rx_fifo = s->rx_fifos[s->first_rx];
> > +
> > +        if (s->rx_offset >= FIFO_SIZE ||
> > +            s->rx_offset >= EMAC_RX_IO_DATA_LEN(rx_fifo[1]) + RX_HDR_SIZE) {
> > +            /* This should never happen */
> > +            debug("RX fifo overrun\n");
> > +            s->first_rx = (s->first_rx + 1) % MAX_RX;
> > +            s->num_rx--;
> > +            s->rx_offset = 0;
> > +            ret = 0;
> > +            break;
> > +        }
> > +
> > +        ret = rx_fifo[s->rx_offset >> 2];
> > +        s->rx_offset += 4;
> > +
> > +        if (s->rx_offset >= EMAC_RX_IO_DATA_LEN(rx_fifo[1]) + RX_HDR_SIZE) {
> > +            s->first_rx = (s->first_rx + 1) % MAX_RX;
> > +            s->num_rx--;
> > +            s->rx_offset = 0;
> > +        }
> > +        break;
> > +    case EMAC_RX_FBC_REG:
> > +        ret = s->num_rx;
> > +        break;
> > +    case EMAC_INT_CTL_REG:
> > +        ret = s->int_ctl;
> > +        break;
> > +    case EMAC_INT_STA_REG:
> > +        ret = s->int_sta;
> > +        break;
> > +    case EMAC_MAC_MRDD_REG:
> > +        ret = mii_read(&s->mii, PHY_TARGET_ADDR(s->phy_target),
> > +                       PHY_TARGET_REG(s->phy_target));
> > +        break;
> > +    default:
> > +        debug("unknown offset %08x\n", (unsigned int)offset);
> > +        ret = 0;
> > +    }
> > +
> > +    return ret;
> > +}
> > +
> > +static void aw_emac_write(void *opaque, hwaddr offset, uint64_t value,
> > +                          unsigned size)
> > +{
> > +    AwEmacState *s = opaque;
> > +    AwEmacTxFifo *fifo;
> > +    int chan;
> > +
> > +    switch (offset) {
> > +    case EMAC_CTL_REG:
> > +        if (value & EMAC_CTL_RESET) {
> > +            debug("reset\n");
> > +            aw_emac_reset(s);
> > +            value &= ~EMAC_CTL_RESET;
> > +        }
> > +        s->ctl = value;
> > +        break;
> > +    case EMAC_TX_MODE_REG:
> > +        s->tx_mode = value;
> > +        break;
> > +    case EMAC_TX_CTL0_REG:
> > +    case EMAC_TX_CTL1_REG:
> > +        chan = (offset == EMAC_TX_CTL0_REG ? 0 : 1);
> > +        if ((value & 1) && (s->ctl & EMAC_CTL_TX_EN)) {
> > +            qemu_send_packet(qemu_get_queue(s->nic),
> > +                             (uint8_t *)s->tx_fifos[chan].data,
> 
> So this cast here is perhaps suspicious. Your write handler just loads
> uint32_t values from the bus access into s->tx_fifos[chan] without any
> endianness checking. This means that s->tx_fifos[chan].data is a
> collection of uint32_t's of host endianness.
> 
> Ultimately, the net layer wants a sequence of bytes (uint8_t *) while
> the bus access logic is thinking in (uint32_t *). I would suggest
> going lowest common denominitor with uint8_t * as your FIFO data type.
> Implement your endianess check in the bus access.

The same problem is present also in the RX code; I will change both to
use a uint8_t array with endianness conversion in read/write handlers.

> > +                             s->tx_fifos[chan].length);
> > +
> > +            /* Raise TX interrupt */
> > +            s->int_sta |= EMAC_INT_TX_CHAN(chan);
> > +            s->tx_fifos[chan].offset = 0;
> > +            aw_emac_update_irq(s);
> > +        }
> > +        break;
> > +    case EMAC_TX_INS_REG:
> > +        s->tx_channel = value < NUM_CHAN ? value : 0;
> > +        break;
> > +    case EMAC_TX_PL0_REG:
> > +    case EMAC_TX_PL1_REG:
> > +        chan = (offset == EMAC_TX_PL0_REG ? 0 : 1);
> > +        if (value > FIFO_SIZE) {
> > +            debug("invalid TX frame length %d\n", (int)value);
> > +            value = FIFO_SIZE;
> > +        }
> > +        s->tx_fifos[chan].length = value;
> > +        break;
> > +    case EMAC_TX_IO_DATA_REG:
> > +        fifo = &s->tx_fifos[s->tx_channel];
> > +        if (fifo->offset >= FIFO_SIZE) {
> > +            debug("TX frame data overruns fifo (%d)\n", fifo->offset);
> > +            break;
> > +        }
> > +        fifo->data[fifo->offset >> 2] = value;
> 
> This is where the endianess issue occurs. Also, if you convert to
> uint8_t * you can ditch on the shift. Theres probably some neat way to
> do this using cpu_to_le32 (or_be, depending on what the device is) to
> avoid a 4 iteration byte loop.

Ok.

>
> > +        fifo->offset += 4;
> > +        break;
> > +    case EMAC_RX_CTL_REG:
> > +        s->rx_ctl = value;
> > +        break;
> > +    case EMAC_RX_FBC_REG:
> > +        if (value == 0) {
> > +            s->num_rx = 0;
> > +        }
> > +        break;
> > +    case EMAC_INT_CTL_REG:
> > +        s->int_ctl = value;
> > +        break;
> > +    case EMAC_INT_STA_REG:
> > +        s->int_sta &= ~value;
> > +        break;
> > +    case EMAC_MAC_MADR_REG:
> > +        s->phy_target = value;
> > +        break;
> > +    case EMAC_MAC_MWTD_REG:
> > +        mii_write(&s->mii, PHY_TARGET_ADDR(s->phy_target),
> > +                  PHY_TARGET_REG(s->phy_target), value);
> > +        break;
> > +    default:
> > +        debug("unknown offset %08x\n", (unsigned int)offset);
> > +    }
> > +}
> > +
> > +static void aw_emac_set_link(NetClientState *nc)
> > +{
> > +    AwEmacState *s = qemu_get_nic_opaque(nc);
> > +
> > +    mii_set_link(&s->mii, !nc->link_down);
> > +}
> > +
> > +static const MemoryRegionOps aw_emac_mem_ops = {
> > +    .read = aw_emac_read,
> > +    .write = aw_emac_write,
> > +    .endianness = DEVICE_NATIVE_ENDIAN,
> > +};
> > +
> > +static NetClientInfo net_aw_emac_info = {
> > +    .type = NET_CLIENT_OPTIONS_KIND_NIC,
> > +    .size = sizeof(NICState),
> > +    .can_receive = aw_emac_can_receive,
> > +    .receive = aw_emac_receive,
> > +    .cleanup = aw_emac_cleanup,
> > +    .link_status_changed = aw_emac_set_link,
> > +};
> > +
> > +static void aw_emac_init(Object *obj)
> > +{
> > +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> > +    AwEmacState *s = AW_EMAC(obj);
> > +
> > +    memory_region_init_io(&s->iomem, OBJECT(s), &aw_emac_mem_ops, s,
> > +                          "aw_emac", 0x1000);
> > +    sysbus_init_mmio(sbd, &s->iomem);
> > +    sysbus_init_irq(sbd, &s->irq);
> > +}
> > +
> > +static void aw_emac_realize(DeviceState *dev, Error **errp)
> > +{
> > +    AwEmacState *s = AW_EMAC(dev);
> > +
> > +    qemu_macaddr_default_if_unset(&s->conf.macaddr);
> > +
> > +    s->nic = qemu_new_nic(&net_aw_emac_info, &s->conf,
> > +                          object_get_typename(OBJECT(dev)), dev->id, s);
> > +    qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
> > +
> > +    aw_emac_reset(s);
> > +    aw_emac_set_link(qemu_get_queue(s->nic));
> > +    mii_reset(&s->mii);
> > +}
> > +
> > +static Property aw_emac_properties[] = {
> > +    DEFINE_NIC_PROPERTIES(AwEmacState, conf),
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> > +static const VMStateDescription vmstate_mii = {
> > +    .name = "allwinner_emac_mii",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_UINT16(bmcr, AwEmacMii),
> > +        VMSTATE_UINT16(bmsr, AwEmacMii),
> > +        VMSTATE_UINT16(anar, AwEmacMii),
> > +        VMSTATE_UINT16(anlpar, AwEmacMii),
> > +        VMSTATE_BOOL(link_ok, AwEmacMii),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> > +static const VMStateDescription vmstate_tx_fifo = {
> > +    .name = "allwinner_emac_tx_fifo",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_UINT32_ARRAY(data, AwEmacTxFifo, FIFO_SIZE >> 2),
> > +        VMSTATE_INT32(length, AwEmacTxFifo),
> > +        VMSTATE_INT32(offset, AwEmacTxFifo),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> > +static const VMStateDescription vmstate_aw_emac = {
> > +    .name = "allwinner_emac",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_STRUCT(mii, AwEmacState, 1, vmstate_mii, AwEmacMii),
> > +        VMSTATE_UINT32(ctl, AwEmacState),
> > +        VMSTATE_UINT32(tx_mode, AwEmacState),
> > +        VMSTATE_UINT32(rx_ctl, AwEmacState),
> > +        VMSTATE_UINT32(int_ctl, AwEmacState),
> > +        VMSTATE_UINT32(int_sta, AwEmacState),
> > +        VMSTATE_UINT32(phy_target, AwEmacState),
> > +        VMSTATE_INT32(tx_channel, AwEmacState),
> > +        VMSTATE_STRUCT_ARRAY(tx_fifos, AwEmacState,
> > +                             NUM_CHAN, 1, vmstate_tx_fifo, AwEmacTxFifo),
> > +        VMSTATE_BUFFER_UNSAFE(tx_fifos, AwEmacState, 1, MAX_RX * FIFO_SIZE),
> > +        VMSTATE_INT32(first_rx, AwEmacState),
> > +        VMSTATE_INT32(num_rx, AwEmacState),
> > +        VMSTATE_INT32(rx_offset, AwEmacState),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> > +static void aw_emac_class_init(ObjectClass *klass, void *data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +
> > +    dc->realize = aw_emac_realize;
> > +    dc->props = aw_emac_properties;
> > +    dc->vmsd = &vmstate_aw_emac;
> > +}
> > +
> > +static const TypeInfo aw_emac_info = {
> > +    .name           = TYPE_AW_EMAC,
> > +    .parent         = TYPE_SYS_BUS_DEVICE,
> > +    .instance_size  = sizeof(AwEmacState),
> > +    .instance_init   = aw_emac_init,
> > +    .class_init     = aw_emac_class_init,
> > +};
> > +
> > +static void aw_emac_register_types(void)
> > +{
> > +    type_register_static(&aw_emac_info);
> > +}
> > +
> > +type_init(aw_emac_register_types)
> > diff --git a/include/hw/net/allwinner_emac.h b/include/hw/net/allwinner_emac.h
> > new file mode 100644
> > index 0000000..f9f9682
> > --- /dev/null
> > +++ b/include/hw/net/allwinner_emac.h
> > @@ -0,0 +1,204 @@
> > +/*
> > + * Emulation of Allwinner EMAC Fast Ethernet controller and
> > + * Realtek RTL8201CP PHY
> > + *
> > + * Copyright (C) 2013 Beniamino Galvani <b.galvani@gmail.com>
> > + *
> > + * Allwinner EMAC register definitions from Linux kernel are:
> > + *   Copyright 2012 Stefan Roese <sr@denx.de>
> > + *   Copyright 2013 Maxime Ripard <maxime.ripard@free-electrons.com>
> > + *   Copyright 1997 Sten Wang
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + */
> > +#ifndef AW_EMAC_H
> > +#define AW_EMAC_H
> > +
> > +#include "net/net.h"
> > +
> > +#define TYPE_AW_EMAC "allwinner_emac"
> > +#define AW_EMAC(obj) OBJECT_CHECK(AwEmacState, (obj), TYPE_AW_EMAC)
> > +
> > +/*
> > + * Allwinner EMAC register list
> > + */
> > +#define EMAC_CTL_REG            0x00
> > +#define EMAC_TX_MODE_REG        0x04
> > +#define EMAC_TX_FLOW_REG        0x08
> > +#define EMAC_TX_CTL0_REG        0x0C
> > +#define EMAC_TX_CTL1_REG        0x10
> > +#define EMAC_TX_INS_REG         0x14
> > +#define EMAC_TX_PL0_REG         0x18
> > +#define EMAC_TX_PL1_REG         0x1C
> > +#define EMAC_TX_STA_REG         0x20
> > +#define EMAC_TX_IO_DATA_REG     0x24
> > +#define EMAC_TX_IO_DATA1_REG    0x28
> > +#define EMAC_TX_TSVL0_REG       0x2C
> > +#define EMAC_TX_TSVH0_REG       0x30
> > +#define EMAC_TX_TSVL1_REG       0x34
> > +#define EMAC_TX_TSVH1_REG       0x38
> > +#define EMAC_RX_CTL_REG         0x3C
> > +#define EMAC_RX_HASH0_REG       0x40
> > +#define EMAC_RX_HASH1_REG       0x44
> > +#define EMAC_RX_STA_REG         0x48
> > +#define EMAC_RX_IO_DATA_REG     0x4C
> > +#define EMAC_RX_FBC_REG         0x50
> > +#define EMAC_INT_CTL_REG        0x54
> > +#define EMAC_INT_STA_REG        0x58
> > +#define EMAC_MAC_CTL0_REG       0x5C
> > +#define EMAC_MAC_CTL1_REG       0x60
> > +#define EMAC_MAC_IPGT_REG       0x64
> > +#define EMAC_MAC_IPGR_REG       0x68
> > +#define EMAC_MAC_CLRT_REG       0x6C
> > +#define EMAC_MAC_MAXF_REG       0x70
> > +#define EMAC_MAC_SUPP_REG       0x74
> > +#define EMAC_MAC_TEST_REG       0x78
> > +#define EMAC_MAC_MCFG_REG       0x7C
> > +#define EMAC_MAC_MCMD_REG       0x80
> > +#define EMAC_MAC_MADR_REG       0x84
> > +#define EMAC_MAC_MWTD_REG       0x88
> > +#define EMAC_MAC_MRDD_REG       0x8C
> > +#define EMAC_MAC_MIND_REG       0x90
> > +#define EMAC_MAC_SSRR_REG       0x94
> > +#define EMAC_MAC_A0_REG         0x98
> > +#define EMAC_MAC_A1_REG         0x9C
> > +#define EMAC_MAC_A2_REG         0xA0
> > +#define EMAC_SAFX_L_REG0        0xA4
> > +#define EMAC_SAFX_H_REG0        0xA8
> > +#define EMAC_SAFX_L_REG1        0xAC
> > +#define EMAC_SAFX_H_REG1        0xB0
> > +#define EMAC_SAFX_L_REG2        0xB4
> > +#define EMAC_SAFX_H_REG2        0xB8
> > +#define EMAC_SAFX_L_REG3        0xBC
> > +#define EMAC_SAFX_H_REG3        0xC0
> > +
> > +/* CTL register fields */
> > +#define EMAC_CTL_RESET                  (1 << 0)
> > +#define EMAC_CTL_TX_EN                  (1 << 1)
> > +#define EMAC_CTL_RX_EN                  (1 << 2)
> > +
> > +/* TX MODE register fields */
> > +#define EMAC_TX_MODE_ABORTED_FRAME_EN   (1 << 0)
> > +#define EMAC_TX_MODE_DMA_EN             (1 << 1)
> > +
> > +/* RX CTL register fields */
> > +#define EMAC_RX_CTL_AUTO_DRQ_EN         (1 << 1)
> > +#define EMAC_RX_CTL_DMA_EN              (1 << 2)
> > +#define EMAC_RX_CTL_PASS_ALL_EN         (1 << 4)
> > +#define EMAC_RX_CTL_PASS_CTL_EN         (1 << 5)
> > +#define EMAC_RX_CTL_PASS_CRC_ERR_EN     (1 << 6)
> > +#define EMAC_RX_CTL_PASS_LEN_ERR_EN     (1 << 7)
> > +#define EMAC_RX_CTL_PASS_LEN_OOR_EN     (1 << 8)
> > +#define EMAC_RX_CTL_ACCEPT_UNICAST_EN   (1 << 16)
> > +#define EMAC_RX_CTL_DA_FILTER_EN        (1 << 17)
> > +#define EMAC_RX_CTL_ACCEPT_MULTICAST_EN (1 << 20)
> > +#define EMAC_RX_CTL_HASH_FILTER_EN      (1 << 21)
> > +#define EMAC_RX_CTL_ACCEPT_BROADCAST_EN (1 << 22)
> > +#define EMAC_RX_CTL_SA_FILTER_EN        (1 << 24)
> > +#define EMAC_RX_CTL_SA_FILTER_INVERT_EN (1 << 25)
> > +
> > +/* RX IO DATA register fields */
> > +#define EMAC_RX_IO_DATA_LEN(x)          (x & 0xffff)
> > +#define EMAC_RX_IO_DATA_STATUS(x)       ((x >> 16) & 0xffff)
> > +#define EMAC_RX_HEADER(len, status)     (((len) & 0xffff) | ((status) << 16))
> > +#define EMAC_RX_IO_DATA_STATUS_CRC_ERR  (1 << 4)
> > +#define EMAC_RX_IO_DATA_STATUS_LEN_ERR  (3 << 5)
> > +#define EMAC_RX_IO_DATA_STATUS_OK       (1 << 7)
> > +#define EMAC_UNDOCUMENTED_MAGIC         0x0143414d  /* header for RX frames */
> > +
> > +/* PHY registers */
> > +#define MII_BMCR            0
> > +#define MII_BMSR            1
> > +#define MII_PHYID1          2
> > +#define MII_PHYID2          3
> > +#define MII_ANAR            4
> > +#define MII_ANLPAR          5
> > +
> > +/* PHY registers fields */
> > +#define MII_BMCR_RESET      (1 << 15)
> > +#define MII_BMCR_LOOPBACK   (1 << 14)
> > +#define MII_BMCR_SPEED      (1 << 13)
> > +#define MII_BMCR_AUTOEN     (1 << 12)
> > +#define MII_BMCR_FD         (1 << 8)
> > +
> > +#define MII_BMSR_100TX_FD   (1 << 14)
> > +#define MII_BMSR_100TX_HD   (1 << 13)
> > +#define MII_BMSR_10T_FD     (1 << 12)
> > +#define MII_BMSR_10T_HD     (1 << 11)
> > +#define MII_BMSR_MFPS       (1 << 6)
> > +#define MII_BMSR_AUTONEG    (1 << 3)
> > +#define MII_BMSR_LINK_ST    (1 << 2)
> > +
> > +#define MII_ANAR_TXFD       (1 << 8)
> > +#define MII_ANAR_TX         (1 << 7)
> > +#define MII_ANAR_10FD       (1 << 6)
> > +#define MII_ANAR_10         (1 << 5)
> > +#define MII_ANAR_CSMACD     (1 << 0)
> > +
> > +#define RTL8201CP_PHYID1    0x0000
> > +#define RTL8201CP_PHYID2    0x8201
> > +
> > +/* INT CTL and INT STA registers fields */
> > +#define EMAC_INT_TX_CHAN(x) (1 << (x))
> > +#define EMAC_INT_RX         (1 << 8)
> > +
> > +#define BOARD_PHY_ADDRESS   1
> > +
> > +#define NUM_CHAN            2
> > +#define FIFO_SIZE           2048
> > +#define MAX_RX              12
> > +#define RX_HDR_SIZE         8
> > +
> > +#define PHY_TARGET_ADDR(x)  (((x) >> 8) & 0xff)
> > +#define PHY_TARGET_REG(x)   ((x) & 0xff)
> > +
> > +typedef struct AwEmacTxFifo {
> > +    uint32_t data[FIFO_SIZE >> 2];
> 
> uint8_t data[FIFO_SIZE];
> 
> 
> > +    int      length;
> > +    int      offset;
> > +} AwEmacTxFifo;
> > +
> > +typedef struct AwEmacMii {
> > +    uint16_t bmcr;
> > +    uint16_t bmsr;
> > +    uint16_t anar;
> > +    uint16_t anlpar;
> > +    bool     link_ok;
> > +} AwEmacMii;
> > +
> > +typedef struct AwEmacState {
> > +    /*< private >*/
> > +    SysBusDevice  parent_obj;
> > +    /*< public >*/
> > +
> > +    MemoryRegion  iomem;
> > +    qemu_irq      irq;
> > +    NICState      *nic;
> > +    NICConf       conf;
> > +    AwEmacMii     mii;
> > +
> > +    uint32_t      ctl;
> > +    uint32_t      tx_mode;
> > +    uint32_t      rx_ctl;
> > +    uint32_t      int_ctl;
> > +    uint32_t      int_sta;
> > +    uint32_t      phy_target;
> > +
> > +    int           tx_channel;             /* Currently selected TX channel */
> > +    AwEmacTxFifo  tx_fifos[NUM_CHAN];
> > +    uint32_t      rx_fifos[MAX_RX][FIFO_SIZE >> 2];
> > +    int           first_rx;               /* First packet in the RX ring */
> > +    int           num_rx;                 /* Number of packets in RX ring */
> > +    int           rx_offset;              /* Offset of next read */
> > +} AwEmacState;
> > +
> > +#endif
> > --
> > 1.7.10.4
> >
> 
> I'll look into your direct buffer access problem hopefully soon, and
> see what can be done to make Fifo8 DMA (for want of a better term)
> friendly. I'm thinking two functions:
> 
> /** pop a number of elements from the FIFO up to a maximum of max. The
> buffer containing the popped data is returned. This buffer points
> directly into the Fifo backing store and data is invalidated once any
> of the fifo_* APIs are called on the FIFO. May short return, the
> number of valid bytes returned in populated in *num. Will always
> return at least 1 byte. Behavior is undefined if max > the number of
> bytes in the Fifo or max == 0.
> */
> 
> const uint8_t *fifo_pop_buf(Fifo *fifo, uint32_t max, uint32_t *num);
> 
> /** Pop num bytes from the Fifo into a newly created buffer. Behavior
> is undefined if num > the number of bytes in the Fifo. The caller is
> responsible for freeing the returned buffer via g_free. */
> 
> uint8_t *fifo_pop_all(Fifo *fifo, uint32_t num);
> 
> fifo_push is much simpler. Just as simple as:
> 
> void fifo_push_all(Fifo *fifo, uint32_t num)
> 
> Would this handle your cases without need for roll-your-own Fifo's?

Yes, the functions cover those cases.

Thanks,
Beniamino

[Qemu-devel] [PATCH 2/2] hw/arm/allwinner-a10: initialize EMAC

[Beniamino Galvani]

Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
---
 hw/arm/allwinner-a10.c         |   20 ++++++++++++++++++++
 include/hw/arm/allwinner-a10.h |    4 ++++
 2 files changed, 24 insertions(+)

diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
index 4658e19..155e026 100644
--- a/hw/arm/allwinner-a10.c
+++ b/hw/arm/allwinner-a10.c
@@ -22,6 +22,7 @@
 static void aw_a10_init(Object *obj)
 {
     AwA10State *s = AW_A10(obj);
+    DeviceState *dev;
 
     object_initialize(&s->cpu, sizeof(s->cpu), "cortex-a8-" TYPE_ARM_CPU);
     object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL);
@@ -31,6 +32,14 @@ static void aw_a10_init(Object *obj)
 
     object_initialize(&s->timer, sizeof(s->timer), TYPE_AW_A10_PIT);
     qdev_set_parent_bus(DEVICE(&s->timer), sysbus_get_default());
+
+    if (nd_table[0].used) {
+        qemu_check_nic_model(&nd_table[0], "allwinner_emac");
+        object_initialize(&s->emac, sizeof(s->emac), TYPE_AW_EMAC);
+        dev = DEVICE(&s->emac);
+        qdev_set_nic_properties(dev, &nd_table[0]);
+        qdev_set_parent_bus(dev, sysbus_get_default());
+    }
 }
 
 static void aw_a10_realize(DeviceState *dev, Error **errp)
@@ -76,6 +85,17 @@ static void aw_a10_realize(DeviceState *dev, Error **errp)
     sysbus_connect_irq(sysbusdev, 4, s->irq[67]);
     sysbus_connect_irq(sysbusdev, 5, s->irq[68]);
 
+    if (nd_table[0].used) {
+        object_property_set_bool(OBJECT(&s->emac), true, "realized", &err);
+        if (err != NULL) {
+            error_propagate(errp, err);
+            return;
+        }
+        sysbusdev = SYS_BUS_DEVICE(&s->emac);
+        sysbus_mmio_map(sysbusdev, 0, AW_A10_EMAC_BASE);
+        sysbus_connect_irq(sysbusdev, 0, s->irq[55]);
+    }
+
     serial_mm_init(get_system_memory(), AW_A10_UART0_REG_BASE, 2, s->irq[1],
                    115200, serial_hds[0], DEVICE_NATIVE_ENDIAN);
 }
diff --git a/include/hw/arm/allwinner-a10.h b/include/hw/arm/allwinner-a10.h
index da36647..6ea5988 100644
--- a/include/hw/arm/allwinner-a10.h
+++ b/include/hw/arm/allwinner-a10.h
@@ -6,6 +6,7 @@
 #include "hw/arm/arm.h"
 #include "hw/timer/allwinner-a10-pit.h"
 #include "hw/intc/allwinner-a10-pic.h"
+#include "hw/net/allwinner_emac.h"
 
 #include "sysemu/sysemu.h"
 #include "exec/address-spaces.h"
@@ -14,9 +15,11 @@
 #define AW_A10_PIC_REG_BASE     0x01c20400
 #define AW_A10_PIT_REG_BASE     0x01c20c00
 #define AW_A10_UART0_REG_BASE   0x01c28000
+#define AW_A10_EMAC_BASE        0x01c0b000
 
 #define AW_A10_SDRAM_BASE       0x40000000
 
+
 #define TYPE_AW_A10 "allwinner-a10"
 #define AW_A10(obj) OBJECT_CHECK(AwA10State, (obj), TYPE_AW_A10)
 
@@ -29,6 +32,7 @@ typedef struct AwA10State {
     qemu_irq irq[AW_A10_PIC_INT_NR];
     AwA10PITState timer;
     AwA10PICState intc;
+    AwEmacState emac;
 } AwA10State;
 
 #define ALLWINNER_H_
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 2/2] hw/arm/allwinner-a10: initialize EMAC

[Peter Crosthwaite]

On Thu, Jan 2, 2014 at 7:18 PM, Beniamino Galvani <b.galvani@gmail.com> wrote:
> Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
> ---
>  hw/arm/allwinner-a10.c         |   20 ++++++++++++++++++++
>  include/hw/arm/allwinner-a10.h |    4 ++++
>  2 files changed, 24 insertions(+)
>
> diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
> index 4658e19..155e026 100644
> --- a/hw/arm/allwinner-a10.c
> +++ b/hw/arm/allwinner-a10.c
> @@ -22,6 +22,7 @@
>  static void aw_a10_init(Object *obj)
>  {
>      AwA10State *s = AW_A10(obj);
> +    DeviceState *dev;

For consistency with surrounding code, you could just cast to DEVICE
include and drop the new local var.

>
>      object_initialize(&s->cpu, sizeof(s->cpu), "cortex-a8-" TYPE_ARM_CPU);
>      object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL);
> @@ -31,6 +32,14 @@ static void aw_a10_init(Object *obj)
>
>      object_initialize(&s->timer, sizeof(s->timer), TYPE_AW_A10_PIT);
>      qdev_set_parent_bus(DEVICE(&s->timer), sysbus_get_default());
> +
> +    if (nd_table[0].used) {

So with SoC MACs, they are "always there". I think this conditional
should be removed, and the MAC is always instantiated. ...

> +        qemu_check_nic_model(&nd_table[0], "allwinner_emac");
> +        object_initialize(&s->emac, sizeof(s->emac), TYPE_AW_EMAC);
> +        dev = DEVICE(&s->emac);
> +        qdev_set_nic_properties(dev, &nd_table[0]);

... and then only this is conditional on nd_table.used. The mac model
itself then of course needs to be hardened against a null netargs.

> +        qdev_set_parent_bus(dev, sysbus_get_default());
> +    }
>  }
>
>  static void aw_a10_realize(DeviceState *dev, Error **errp)
> @@ -76,6 +85,17 @@ static void aw_a10_realize(DeviceState *dev, Error **errp)
>      sysbus_connect_irq(sysbusdev, 4, s->irq[67]);
>      sysbus_connect_irq(sysbusdev, 5, s->irq[68]);
>
> +    if (nd_table[0].used) {
> +        object_property_set_bool(OBJECT(&s->emac), true, "realized", &err);
> +        if (err != NULL) {
> +            error_propagate(errp, err);
> +            return;
> +        }
> +        sysbusdev = SYS_BUS_DEVICE(&s->emac);
> +        sysbus_mmio_map(sysbusdev, 0, AW_A10_EMAC_BASE);
> +        sysbus_connect_irq(sysbusdev, 0, s->irq[55]);
> +    }
> +
>      serial_mm_init(get_system_memory(), AW_A10_UART0_REG_BASE, 2, s->irq[1],
>                     115200, serial_hds[0], DEVICE_NATIVE_ENDIAN);
>  }
> diff --git a/include/hw/arm/allwinner-a10.h b/include/hw/arm/allwinner-a10.h
> index da36647..6ea5988 100644
> --- a/include/hw/arm/allwinner-a10.h
> +++ b/include/hw/arm/allwinner-a10.h
> @@ -6,6 +6,7 @@
>  #include "hw/arm/arm.h"
>  #include "hw/timer/allwinner-a10-pit.h"
>  #include "hw/intc/allwinner-a10-pic.h"
> +#include "hw/net/allwinner_emac.h"
>
>  #include "sysemu/sysemu.h"
>  #include "exec/address-spaces.h"
> @@ -14,9 +15,11 @@
>  #define AW_A10_PIC_REG_BASE     0x01c20400
>  #define AW_A10_PIT_REG_BASE     0x01c20c00
>  #define AW_A10_UART0_REG_BASE   0x01c28000
> +#define AW_A10_EMAC_BASE        0x01c0b000
>
>  #define AW_A10_SDRAM_BASE       0x40000000
>
> +

This whitespace change intended?

Regards,
Peter

>  #define TYPE_AW_A10 "allwinner-a10"
>  #define AW_A10(obj) OBJECT_CHECK(AwA10State, (obj), TYPE_AW_A10)
>
> @@ -29,6 +32,7 @@ typedef struct AwA10State {
>      qemu_irq irq[AW_A10_PIC_INT_NR];
>      AwA10PITState timer;
>      AwA10PICState intc;
> +    AwEmacState emac;
>  } AwA10State;
>
>  #define ALLWINNER_H_
> --
> 1.7.10.4
>
>

Re: [Qemu-devel] [PATCH 2/2] hw/arm/allwinner-a10: initialize EMAC

[Peter Crosthwaite]

On Thu, Jan 2, 2014 at 8:20 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Thu, Jan 2, 2014 at 7:18 PM, Beniamino Galvani <b.galvani@gmail.com> wrote:
>> Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
>> ---
>>  hw/arm/allwinner-a10.c         |   20 ++++++++++++++++++++
>>  include/hw/arm/allwinner-a10.h |    4 ++++
>>  2 files changed, 24 insertions(+)
>>
>> diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
>> index 4658e19..155e026 100644
>> --- a/hw/arm/allwinner-a10.c
>> +++ b/hw/arm/allwinner-a10.c
>> @@ -22,6 +22,7 @@
>>  static void aw_a10_init(Object *obj)
>>  {
>>      AwA10State *s = AW_A10(obj);
>> +    DeviceState *dev;
>
> For consistency with surrounding code, you could just cast to DEVICE
> include and drop the new local var.
>

inline* not include

Sorry,
Peter

>>
>>      object_initialize(&s->cpu, sizeof(s->cpu), "cortex-a8-" TYPE_ARM_CPU);
>>      object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL);
>> @@ -31,6 +32,14 @@ static void aw_a10_init(Object *obj)
>>
>>      object_initialize(&s->timer, sizeof(s->timer), TYPE_AW_A10_PIT);
>>      qdev_set_parent_bus(DEVICE(&s->timer), sysbus_get_default());
>> +
>> +    if (nd_table[0].used) {
>
> So with SoC MACs, they are "always there". I think this conditional
> should be removed, and the MAC is always instantiated. ...
>
>> +        qemu_check_nic_model(&nd_table[0], "allwinner_emac");
>> +        object_initialize(&s->emac, sizeof(s->emac), TYPE_AW_EMAC);
>> +        dev = DEVICE(&s->emac);
>> +        qdev_set_nic_properties(dev, &nd_table[0]);
>
> ... and then only this is conditional on nd_table.used. The mac model
> itself then of course needs to be hardened against a null netargs.
>
>> +        qdev_set_parent_bus(dev, sysbus_get_default());
>> +    }
>>  }
>>
>>  static void aw_a10_realize(DeviceState *dev, Error **errp)
>> @@ -76,6 +85,17 @@ static void aw_a10_realize(DeviceState *dev, Error **errp)
>>      sysbus_connect_irq(sysbusdev, 4, s->irq[67]);
>>      sysbus_connect_irq(sysbusdev, 5, s->irq[68]);
>>
>> +    if (nd_table[0].used) {
>> +        object_property_set_bool(OBJECT(&s->emac), true, "realized", &err);
>> +        if (err != NULL) {
>> +            error_propagate(errp, err);
>> +            return;
>> +        }
>> +        sysbusdev = SYS_BUS_DEVICE(&s->emac);
>> +        sysbus_mmio_map(sysbusdev, 0, AW_A10_EMAC_BASE);
>> +        sysbus_connect_irq(sysbusdev, 0, s->irq[55]);
>> +    }
>> +
>>      serial_mm_init(get_system_memory(), AW_A10_UART0_REG_BASE, 2, s->irq[1],
>>                     115200, serial_hds[0], DEVICE_NATIVE_ENDIAN);
>>  }
>> diff --git a/include/hw/arm/allwinner-a10.h b/include/hw/arm/allwinner-a10.h
>> index da36647..6ea5988 100644
>> --- a/include/hw/arm/allwinner-a10.h
>> +++ b/include/hw/arm/allwinner-a10.h
>> @@ -6,6 +6,7 @@
>>  #include "hw/arm/arm.h"
>>  #include "hw/timer/allwinner-a10-pit.h"
>>  #include "hw/intc/allwinner-a10-pic.h"
>> +#include "hw/net/allwinner_emac.h"
>>
>>  #include "sysemu/sysemu.h"
>>  #include "exec/address-spaces.h"
>> @@ -14,9 +15,11 @@
>>  #define AW_A10_PIC_REG_BASE     0x01c20400
>>  #define AW_A10_PIT_REG_BASE     0x01c20c00
>>  #define AW_A10_UART0_REG_BASE   0x01c28000
>> +#define AW_A10_EMAC_BASE        0x01c0b000
>>
>>  #define AW_A10_SDRAM_BASE       0x40000000
>>
>> +
>
> This whitespace change intended?
>
> Regards,
> Peter
>
>>  #define TYPE_AW_A10 "allwinner-a10"
>>  #define AW_A10(obj) OBJECT_CHECK(AwA10State, (obj), TYPE_AW_A10)
>>
>> @@ -29,6 +32,7 @@ typedef struct AwA10State {
>>      qemu_irq irq[AW_A10_PIC_INT_NR];
>>      AwA10PITState timer;
>>      AwA10PICState intc;
>> +    AwEmacState emac;
>>  } AwA10State;
>>
>>  #define ALLWINNER_H_
>> --
>> 1.7.10.4
>>
>>

Re: [Qemu-devel] [PATCH 2/2] hw/arm/allwinner-a10: initialize EMAC

[Beniamino Galvani]

On Thu, Jan 02, 2014 at 08:20:12PM +1000, Peter Crosthwaite wrote:
> On Thu, Jan 2, 2014 at 7:18 PM, Beniamino Galvani <b.galvani@gmail.com> wrote:
> > Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
> > ---
> >  hw/arm/allwinner-a10.c         |   20 ++++++++++++++++++++
> >  include/hw/arm/allwinner-a10.h |    4 ++++
> >  2 files changed, 24 insertions(+)
> >
> > diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
> > index 4658e19..155e026 100644
> > --- a/hw/arm/allwinner-a10.c
> > +++ b/hw/arm/allwinner-a10.c
> > @@ -22,6 +22,7 @@
> >  static void aw_a10_init(Object *obj)
> >  {
> >      AwA10State *s = AW_A10(obj);
> > +    DeviceState *dev;
> 
> For consistency with surrounding code, you could just cast to DEVICE
> include and drop the new local var.
> 

Ok.

> >
> >      object_initialize(&s->cpu, sizeof(s->cpu), "cortex-a8-" TYPE_ARM_CPU);
> >      object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL);
> > @@ -31,6 +32,14 @@ static void aw_a10_init(Object *obj)
> >
> >      object_initialize(&s->timer, sizeof(s->timer), TYPE_AW_A10_PIT);
> >      qdev_set_parent_bus(DEVICE(&s->timer), sysbus_get_default());
> > +
> > +    if (nd_table[0].used) {
> 
> So with SoC MACs, they are "always there". I think this conditional
> should be removed, and the MAC is always instantiated. ...
> 
> > +        qemu_check_nic_model(&nd_table[0], "allwinner_emac");
> > +        object_initialize(&s->emac, sizeof(s->emac), TYPE_AW_EMAC);
> > +        dev = DEVICE(&s->emac);
> > +        qdev_set_nic_properties(dev, &nd_table[0]);
> 
> ... and then only this is conditional on nd_table.used. The mac model
> itself then of course needs to be hardened against a null netargs.
> 
> > +        qdev_set_parent_bus(dev, sysbus_get_default());
> > +    }
> >  }
> >

So, if I understand correctly, the part under the condition should be:

             qdev_set_parent_bus(dev, sysbus_get_default());

right?

> >  static void aw_a10_realize(DeviceState *dev, Error **errp)
> > @@ -76,6 +85,17 @@ static void aw_a10_realize(DeviceState *dev, Error **errp)
> >      sysbus_connect_irq(sysbusdev, 4, s->irq[67]);
> >      sysbus_connect_irq(sysbusdev, 5, s->irq[68]);
> >
> > +    if (nd_table[0].used) {
> > +        object_property_set_bool(OBJECT(&s->emac), true, "realized", &err);
> > +        if (err != NULL) {
> > +            error_propagate(errp, err);
> > +            return;
> > +        }
> > +        sysbusdev = SYS_BUS_DEVICE(&s->emac);
> > +        sysbus_mmio_map(sysbusdev, 0, AW_A10_EMAC_BASE);
> > +        sysbus_connect_irq(sysbusdev, 0, s->irq[55]);
> > +    }
> > +
> >      serial_mm_init(get_system_memory(), AW_A10_UART0_REG_BASE, 2, s->irq[1],
> >                     115200, serial_hds[0], DEVICE_NATIVE_ENDIAN);
> >  }
> > diff --git a/include/hw/arm/allwinner-a10.h b/include/hw/arm/allwinner-a10.h
> > index da36647..6ea5988 100644
> > --- a/include/hw/arm/allwinner-a10.h
> > +++ b/include/hw/arm/allwinner-a10.h
> > @@ -6,6 +6,7 @@
> >  #include "hw/arm/arm.h"
> >  #include "hw/timer/allwinner-a10-pit.h"
> >  #include "hw/intc/allwinner-a10-pic.h"
> > +#include "hw/net/allwinner_emac.h"
> >
> >  #include "sysemu/sysemu.h"
> >  #include "exec/address-spaces.h"
> > @@ -14,9 +15,11 @@
> >  #define AW_A10_PIC_REG_BASE     0x01c20400
> >  #define AW_A10_PIT_REG_BASE     0x01c20c00
> >  #define AW_A10_UART0_REG_BASE   0x01c28000
> > +#define AW_A10_EMAC_BASE        0x01c0b000
> >
> >  #define AW_A10_SDRAM_BASE       0x40000000
> >
> > +
> 
> This whitespace change intended?

No, a leftover.

Thanks,
Beniamino

Re: [Qemu-devel] [PATCH 2/2] hw/arm/allwinner-a10: initialize EMAC

[Peter Crosthwaite]

On Fri, Jan 3, 2014 at 3:19 AM, Beniamino Galvani <b.galvani@gmail.com> wrote:
> On Thu, Jan 02, 2014 at 08:20:12PM +1000, Peter Crosthwaite wrote:
>> On Thu, Jan 2, 2014 at 7:18 PM, Beniamino Galvani <b.galvani@gmail.com> wrote:
>> > Signed-off-by: Beniamino Galvani <b.galvani@gmail.com>
>> > ---
>> >  hw/arm/allwinner-a10.c         |   20 ++++++++++++++++++++
>> >  include/hw/arm/allwinner-a10.h |    4 ++++
>> >  2 files changed, 24 insertions(+)
>> >
>> > diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
>> > index 4658e19..155e026 100644
>> > --- a/hw/arm/allwinner-a10.c
>> > +++ b/hw/arm/allwinner-a10.c
>> > @@ -22,6 +22,7 @@
>> >  static void aw_a10_init(Object *obj)
>> >  {
>> >      AwA10State *s = AW_A10(obj);
>> > +    DeviceState *dev;
>>
>> For consistency with surrounding code, you could just cast to DEVICE
>> include and drop the new local var.
>>
>
> Ok.
>
>> >
>> >      object_initialize(&s->cpu, sizeof(s->cpu), "cortex-a8-" TYPE_ARM_CPU);
>> >      object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL);
>> > @@ -31,6 +32,14 @@ static void aw_a10_init(Object *obj)
>> >
>> >      object_initialize(&s->timer, sizeof(s->timer), TYPE_AW_A10_PIT);
>> >      qdev_set_parent_bus(DEVICE(&s->timer), sysbus_get_default());
>> > +
>> > +    if (nd_table[0].used) {
>>
>> So with SoC MACs, they are "always there". I think this conditional
>> should be removed, and the MAC is always instantiated. ...
>>
>> > +        qemu_check_nic_model(&nd_table[0], "allwinner_emac");
>> > +        object_initialize(&s->emac, sizeof(s->emac), TYPE_AW_EMAC);
>> > +        dev = DEVICE(&s->emac);
>> > +        qdev_set_nic_properties(dev, &nd_table[0]);
>>
>> ... and then only this is conditional on nd_table.used. The mac model
>> itself then of course needs to be hardened against a null netargs.
>>
>> > +        qdev_set_parent_bus(dev, sysbus_get_default());
>> > +    }
>> >  }
>> >
>
> So, if I understand correctly, the part under the condition should be:
>
>              qdev_set_parent_bus(dev, sysbus_get_default());
>

No, I meant the line above, qdev_set_nic_properties. This corresponds
to attaching the always-present NIC to an emulated network.

Regards,
Peter

> right?
>
>> >  static void aw_a10_realize(DeviceState *dev, Error **errp)
>> > @@ -76,6 +85,17 @@ static void aw_a10_realize(DeviceState *dev, Error **errp)
>> >      sysbus_connect_irq(sysbusdev, 4, s->irq[67]);
>> >      sysbus_connect_irq(sysbusdev, 5, s->irq[68]);
>> >
>> > +    if (nd_table[0].used) {
>> > +        object_property_set_bool(OBJECT(&s->emac), true, "realized", &err);
>> > +        if (err != NULL) {
>> > +            error_propagate(errp, err);
>> > +            return;
>> > +        }
>> > +        sysbusdev = SYS_BUS_DEVICE(&s->emac);
>> > +        sysbus_mmio_map(sysbusdev, 0, AW_A10_EMAC_BASE);
>> > +        sysbus_connect_irq(sysbusdev, 0, s->irq[55]);
>> > +    }
>> > +
>> >      serial_mm_init(get_system_memory(), AW_A10_UART0_REG_BASE, 2, s->irq[1],
>> >                     115200, serial_hds[0], DEVICE_NATIVE_ENDIAN);
>> >  }
>> > diff --git a/include/hw/arm/allwinner-a10.h b/include/hw/arm/allwinner-a10.h
>> > index da36647..6ea5988 100644
>> > --- a/include/hw/arm/allwinner-a10.h
>> > +++ b/include/hw/arm/allwinner-a10.h
>> > @@ -6,6 +6,7 @@
>> >  #include "hw/arm/arm.h"
>> >  #include "hw/timer/allwinner-a10-pit.h"
>> >  #include "hw/intc/allwinner-a10-pic.h"
>> > +#include "hw/net/allwinner_emac.h"
>> >
>> >  #include "sysemu/sysemu.h"
>> >  #include "exec/address-spaces.h"
>> > @@ -14,9 +15,11 @@
>> >  #define AW_A10_PIC_REG_BASE     0x01c20400
>> >  #define AW_A10_PIT_REG_BASE     0x01c20c00
>> >  #define AW_A10_UART0_REG_BASE   0x01c28000
>> > +#define AW_A10_EMAC_BASE        0x01c0b000
>> >
>> >  #define AW_A10_SDRAM_BASE       0x40000000
>> >
>> > +
>>
>> This whitespace change intended?
>
> No, a leftover.
>
> Thanks,
> Beniamino
>

Re: [Qemu-devel] [PATCH 2/2] hw/arm/allwinner-a10: initialize EMAC

[Li Guang]

Hi,
please use prefix AwA10 for names instead of Aw,
also PATCH 1/2.
Thanks for your effort on this!

Beniamino Galvani wrote:
> Signed-off-by: Beniamino Galvani<b.galvani@gmail.com>
> ---
>   hw/arm/allwinner-a10.c         |   20 ++++++++++++++++++++
>   include/hw/arm/allwinner-a10.h |    4 ++++
>   2 files changed, 24 insertions(+)
>
> diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c
> index 4658e19..155e026 100644
> --- a/hw/arm/allwinner-a10.c
> +++ b/hw/arm/allwinner-a10.c
> @@ -22,6 +22,7 @@
>   static void aw_a10_init(Object *obj)
>   {
>       AwA10State *s = AW_A10(obj);
> +    DeviceState *dev;
>
>       object_initialize(&s->cpu, sizeof(s->cpu), "cortex-a8-" TYPE_ARM_CPU);
>       object_property_add_child(obj, "cpu", OBJECT(&s->cpu), NULL);
> @@ -31,6 +32,14 @@ static void aw_a10_init(Object *obj)
>
>       object_initialize(&s->timer, sizeof(s->timer), TYPE_AW_A10_PIT);
>       qdev_set_parent_bus(DEVICE(&s->timer), sysbus_get_default());
> +
> +    if (nd_table[0].used) {
> +        qemu_check_nic_model(&nd_table[0], "allwinner_emac");
> +        object_initialize(&s->emac, sizeof(s->emac), TYPE_AW_EMAC);
> +        dev = DEVICE(&s->emac);
> +        qdev_set_nic_properties(dev,&nd_table[0]);
> +        qdev_set_parent_bus(dev, sysbus_get_default());
> +    }
>   }
>
>   static void aw_a10_realize(DeviceState *dev, Error **errp)
> @@ -76,6 +85,17 @@ static void aw_a10_realize(DeviceState *dev, Error **errp)
>       sysbus_connect_irq(sysbusdev, 4, s->irq[67]);
>       sysbus_connect_irq(sysbusdev, 5, s->irq[68]);
>
> +    if (nd_table[0].used) {
> +        object_property_set_bool(OBJECT(&s->emac), true, "realized",&err);
> +        if (err != NULL) {
> +            error_propagate(errp, err);
> +            return;
> +        }
> +        sysbusdev = SYS_BUS_DEVICE(&s->emac);
> +        sysbus_mmio_map(sysbusdev, 0, AW_A10_EMAC_BASE);
> +        sysbus_connect_irq(sysbusdev, 0, s->irq[55]);
> +    }
> +
>       serial_mm_init(get_system_memory(), AW_A10_UART0_REG_BASE, 2, s->irq[1],
>                      115200, serial_hds[0], DEVICE_NATIVE_ENDIAN);
>   }
> diff --git a/include/hw/arm/allwinner-a10.h b/include/hw/arm/allwinner-a10.h
> index da36647..6ea5988 100644
> --- a/include/hw/arm/allwinner-a10.h
> +++ b/include/hw/arm/allwinner-a10.h
> @@ -6,6 +6,7 @@
>   #include "hw/arm/arm.h"
>   #include "hw/timer/allwinner-a10-pit.h"
>   #include "hw/intc/allwinner-a10-pic.h"
> +#include "hw/net/allwinner_emac.h"
>
>   #include "sysemu/sysemu.h"
>   #include "exec/address-spaces.h"
> @@ -14,9 +15,11 @@
>   #define AW_A10_PIC_REG_BASE     0x01c20400
>   #define AW_A10_PIT_REG_BASE     0x01c20c00
>   #define AW_A10_UART0_REG_BASE   0x01c28000
> +#define AW_A10_EMAC_BASE        0x01c0b000
>
>   #define AW_A10_SDRAM_BASE       0x40000000
>
> +
>   #define TYPE_AW_A10 "allwinner-a10"
>   #define AW_A10(obj) OBJECT_CHECK(AwA10State, (obj), TYPE_AW_A10)
>
> @@ -29,6 +32,7 @@ typedef struct AwA10State {
>       qemu_irq irq[AW_A10_PIC_INT_NR];
>       AwA10PITState timer;
>       AwA10PICState intc;
> +    AwEmacState emac;
>   } AwA10State;
>
>   #define ALLWINNER_H_
>    

Re: [Qemu-devel] [PATCH 2/2] hw/arm/allwinner-a10: initialize EMAC

[Beniamino Galvani]

On Mon, Jan 06, 2014 at 08:49:18AM +0800, Li Guang wrote:
> Hi,
> please use prefix AwA10 for names instead of Aw,
> also PATCH 1/2.

Hi,

I agree with you that there is an inconsistency in the naming of EMAC
and other A10 devices (timer, interrupt controller).

But the EMAC core is used not only on the A10; since it can be found
on other SoC of the Allwinner family, shouldn't the name be generic so
that it can be reused more easily in the future by other SoC
implementations?

Regards,
Beniamino

Re: [Qemu-devel] [PATCH 2/2] hw/arm/allwinner-a10: initialize EMAC

[Li Guang]

Beniamino Galvani wrote:
> On Mon, Jan 06, 2014 at 08:49:18AM +0800, Li Guang wrote:
>    
>> Hi,
>> please use prefix AwA10 for names instead of Aw,
>> also PATCH 1/2.
>>      
> Hi,
>
> I agree with you that there is an inconsistency in the naming of EMAC
> and other A10 devices (timer, interrupt controller).
>
> But the EMAC core is used not only on the A10; since it can be found
> on other SoC of the Allwinner family, shouldn't the name be generic so
> that it can be reused more easily in the future by other SoC
> implementations?
>
>    

logic is:
we emulated devices in A10, then when emulate other chips
with same devices can freely use them.

Thanks!

Re: [Qemu-devel] [PATCH 2/2] hw/arm/allwinner-a10: initialize EMAC

[Peter Crosthwaite]

[-- Attachment #1: Type: text/plain, Size: 946 bytes --]

On Wed, Jan 8, 2014 at 5:27 PM, Li Guang <lig.fnst@cn.fujitsu.com> wrote:
> Beniamino Galvani wrote:
>>
>> On Mon, Jan 06, 2014 at 08:49:18AM +0800, Li Guang wrote:
>>
>>>
>>> Hi,
>>> please use prefix AwA10 for names instead of Aw,
>>> also PATCH 1/2.
>>>
>>
>> Hi,
>>
>> I agree with you that there is an inconsistency in the naming of EMAC
>> and other A10 devices (timer, interrupt controller).
>>
>> But the EMAC core is used not only on the A10; since it can be found
>> on other SoC of the Allwinner family, shouldn't the name be generic so
>> that it can be reused more easily in the future by other SoC
>> implementations?
>>
>>
>
>
> logic is:
> we emulated devices in A10, then when emulate other chips
> with same devices can freely use them.
>

That shouldn't dictate the naming scheme of the sharable IP however. In
this case, the name should reflect what it can be used for, not what it is
used for.

Regards,
Peter

> Thanks!
>
>

[-- Attachment #2: Type: text/html, Size: 1562 bytes --]