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

[Beniamino Galvani]

On Mon, Jan 13, 2014 at 11:15:17PM +1000, Peter Crosthwaite wrote:
> On Sat, Jan 11, 2014 at 8:13 PM, Beniamino Galvani <address@hidden> 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 <address@hidden>
> > ---
> >  default-configs/arm-softmmu.mak |    1 +
> >  hw/net/Makefile.objs            |    1 +
> >  hw/net/allwinner_emac.c         |  472 
> > +++++++++++++++++++++++++++++++++++++++
> >  include/hw/net/allwinner_emac.h |  204 +++++++++++++++++
> >  4 files changed, 678 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..0c37df2
> > --- /dev/null
> > +++ b/hw/net/allwinner_emac.c
> > @@ -0,0 +1,472 @@
> > +/*
> > + * Emulation of Allwinner EMAC Fast Ethernet controller and
> > + * Realtek RTL8201CP PHY
> > + *
> > + * Copyright (C) 2013 Beniamino Galvani <address@hidden>
> > + *
> > + * 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>
> > +
> > +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;
> 
> mii->anlpar.MII_ANAR_TX is always set so you can drop its transient
> setting entirely. Here and below.

I will remove it in the code above. In the code below, though, the
assignment is used to clear all other bits.

> 
> > +    } 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);
> 
> If this is actually needed, it smells like a bug in the net layer. Is
> the net layer not doing it's resets properly? It's quite odd that you
> are preserving old state across a reset.

This is needed to align the local link status to the net layer after a
reset.
 
> > +}
> > +
> > +static uint16_t mii_read(AwEmacMii *mii, uint8_t addr, uint8_t reg)
> 
> From the architectural discussion RE MII/MDIO busification we
> concluded to limit to MDIO only. So I think this should be
> *_mdio_read. A device specific prefix would be good as well.

Ok, will change to aw_emac_mdio_read.

> 
> > +{
> > +    uint16_t ret = 0xffff;
> > +
> > +    if (addr == mii->phy_addr) {
> > +        switch (reg) {
> > +        case MII_BMCR:
> > +            ret = mii->bmcr;
> > +            break;
> > +        case MII_BMSR:
> > +            ret = mii->bmsr;
> > +            break;
> > +        case MII_PHYID1:
> > +            ret = RTL8201CP_PHYID1;
> 
> And this PHY is really the RTL8201CP not an "allwinner phy". Despite
> being married to the allwinner EMAC for the moment (pending the PHY
> refactorings) you should badge the PHY with its proper name something
> like s/AwEmacMii/RTL8201CPState.

This seems reasonable.

> 
> > +            break;
> > +        case MII_PHYID2:
> > +            ret = RTL8201CP_PHYID2;
> > +            break;
> > +        case MII_ANAR:
> > +            ret = mii->anar;
> > +            break;
> > +        case MII_ANLPAR:
> > +            ret = mii->anlpar;
> > +            break;
> > +        default:
> > +            qemu_log_mask(LOG_UNIMP,
> > +                          "allwinner_emac: read from unknown mii reg 
> > 0x%x\n",
> > +                          reg);
> 
> Is this really a mix of LOG_GUEST_ERROR and LOG_UNIMP. What regs are
> missing, inhibiting the coversion of this to LOG_GUEST_ERROR?

According to the RTL8201CP datasheet, the missing registers are:

- 16 NWay Setup Register (NSR)
- 17 Loopback, Bypass, Receiver Error Mask Register (LBREMR)
- 18 RX_ER Counter (REC)
- 19 SNR Display Register
- 25 Test Register

Do you think it's better to add them and switch to LOG_UNIMP for those register?

> 
> > +            ret = 0;
> > +        }
> > +    }
> > +    return ret;
> > +}
> > +
> > +static void mii_write(AwEmacMii *mii, uint8_t addr, uint8_t reg,
> > +                      uint16_t value)
> > +{
> > +    if (addr == mii->phy_addr) {
> > +        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,
> > +                          "allwinner_emac: invalid write to mii reg 
> > 0x%x\n",
> > +                          reg);
> > +            break;
> > +        case MII_ANAR:
> > +            mii->anar = value;
> > +            break;
> > +        default:
> > +            qemu_log_mask(LOG_UNIMP,
> > +                          "allwinner_emac: write to unknown mii reg 
> > 0x%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 < NUM_RX_FIFOS);
> > +}
> > +
> > +static ssize_t aw_emac_receive(NetClientState *nc, const uint8_t *buf,
> > +                               size_t size)
> > +{
> > +    AwEmacState *s = qemu_get_nic_opaque(nc);
> > +    AwEmacFifo *fifo;
> > +    uint32_t crc, *word;
> > +    uint8_t *dest;
> > +
> > +    if (s->num_rx >= NUM_RX_FIFOS) {
> 
> Seems inconsistent that you check for fifo vacancy but you dont check
> for s->ctl & EMAC_CTL_RX_EN (as above in can_recieve). Then again,
> both conditions should be guarded by can_recieve, so I wonder whether
> you can just drop this.
> 
> Stefan, if you are reading, can the recieve function rely on
> can_recieve success and drop such checks?
> 
> > +        return -1;
> > +    }
> > +
> > +    if (RX_HDR_SIZE + size + CRC_SIZE > FIFO_SIZE) {
> > +        return -1;
> > +    }
> > +
> 
> This needs a comment. If a packet exceeds a certain size you just drop
> it? This is worthy of a LOG_UNIMP - as I am guessing that the real
> hardware will have large packet support, just we cant figure out how
> it works without specs.

This code probably needs to be changed if we implement the
modifications to the rx fifo described below. The check will be done
using the free size of the single big fifo and if a packet doesn't fit
it will be dropped.

In that case, I think that the log message is not necessary because
the drop only occurs in some situations (when the fifo is full) and
not for every packet above a predefined length.

> 
> > +    fifo = &s->rx_fifos[(s->first_rx + s->num_rx) % NUM_RX_FIFOS];
> > +    dest = fifo->data + RX_HDR_SIZE;
> > +    memcpy(dest, buf, size);
> > +
> > +    /* Fill to minimum frame length */
> > +    if (size < 60) {
> > +        memset(dest + size, 0, 60 - size);
> > +        size = 60;
> > +    }
> > +
> > +    /* Append FCS */
> > +    crc = cpu_to_le32(crc32(~0, buf, size));
> > +    memcpy(dest + size, &crc, CRC_SIZE);
> > +
> > +    /* Insert frame headers */
> > +    word = (uint32_t *)fifo->data;
> > +    word[0] = cpu_to_le32(EMAC_UNDOCUMENTED_MAGIC);
> > +    word[1] = cpu_to_le32(EMAC_RX_HEADER(size + CRC_SIZE,
> > +                                         EMAC_RX_IO_DATA_STATUS_OK));
> > +
> > +    fifo->length = QEMU_ALIGN_UP(RX_HDR_SIZE + size + CRC_SIZE, 4);
> > +    fifo->offset = 0;
> > +    s->num_rx++;
> > +
> > +    /* 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->phy_target = 0;
> 
> I think you should reset the phy here [1] ...

Ok.

> 
> > +}
> > +
> > +static uint64_t aw_emac_read(void *opaque, hwaddr offset, unsigned size)
> > +{
> > +    AwEmacState *s = opaque;
> > +    uint64_t ret;
> > +    AwEmacFifo *fifo;
> > +
> > +    switch (offset) {
> > +    case EMAC_CTL_REG:
> > +        ret = s->ctl;
> > +        break;
> 
> You can save on a fair few LOC by just returning. It's quite
> commonplace in these read switches.
> 
> return s->ctl etc.
> 
> > +    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;
> > +        }
> > +        fifo = &s->rx_fifos[s->first_rx];
> > +
> > +        assert(fifo->offset + 4 <= FIFO_SIZE);
> > +        assert(fifo->offset + 4 <= fifo->length);
> > +
> > +        ret = fifo->data[fifo->offset++];
> > +        ret |= fifo->data[fifo->offset++] << 8;
> > +        ret |= fifo->data[fifo->offset++] << 16;
> > +        ret |= fifo->data[fifo->offset++] << 24;
> > +
> > +        if (fifo->offset >= fifo->length) {
> > +            s->first_rx = (s->first_rx + 1) % NUM_RX_FIFOS;
> > +            s->num_rx--;
> 
> So I found this whole multiple RX buffers stuff strange until I got
> here. The only reason you seem to need the individual-packet fifos is
> for maintainence of this guest visible packet counter. Seems like a
> very expensive (in silicon) way to implement simple rx packet
> counting. It leaves me with a couple of alternate theories as to whats
> going on:
> 
> 1: On examination of the linux driver, I see that 'rxcount' is only
> ever used as a boolean. The linux author drivers also mention that
> they are working without specs. So perhaps its much simpler than
> everyone thinks, rxcount is just a boolean to say whether or not there
> is FIFO data available. This obviously makes life much simpler here,
> as you can just do all with one giant FIFO.
> 
> 2: EMAC_RX_FBC_REG auto-decrements on read. There is an exact 1:1
> correlation in the linux driver between non-zero-reads to this
> register and packets recieved allowing this possibility. This would
> greatly simplify your QEMU implementation as software implicitly tells
> the hardware how many packets its rxed rather than keep track with
> complex state logic. Again, the one giant FIFO option opens up.
> 
> 3: There is a small state machine parsing the popped packet data.
> Instead of muxing a discrete number of fixed-size fifos, a little
> state machine counts packet length (by parsing the headers in the
> stream) and decrements num_rx accordingly. This allows you to do RX
> cleanly with one giant fifo. You need to add the little state machine
> here, but I think its better than maintaining these N way which impose
> an with artificially low individual packet limit size.
> 
> Any of these alternates allows use of the full fifo length for a
> single packet. but more importantly, with any of them there is no
> limit on the number of packets the fifo can buffer at any one time. I
> cant find evidence of such a limit in the linux driver, leading my to
> believe you have added a maximum-number-rx-packet limitation just in
> your fifo limitation [2] ...
> 
> If in doubt, option 3 is the most defensive.

My implementation was inspired by other qemu NICs where a fixed size
array of fifo is used, for example stellaris_enet and smc91c111.

But I have just noticed that stellaris datasheet specifies a maximum
number of packets that can be stored in the RX buffer (31), so in that
case limiting the packet number makes sense.

Here I agree that using with a single fifo is better, so I will try to
go on with the third option. Do you have any suggestions for deciding
its size?

> 
> > +            qemu_flush_queued_packets(qemu_get_queue(s->nic));
> > +        }
> > +        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, extract32(s->phy_target, PHY_ADDR_SHIFT, 
> > 8),
> > +                       extract32(s->phy_target, PHY_REG_SHIFT, 8));
> > +        break;
> > +    default:
> > +        qemu_log_mask(LOG_UNIMP,
> > +                      "allwinner_emac: read access to unknown register 0x"
> > +                      TARGET_FMT_plx "\n", offset);
> > +        ret = 0;
> > +    }
> > +
> > +    return ret;
> > +}
> > +
> > +static void aw_emac_write(void *opaque, hwaddr offset, uint64_t value,
> > +                          unsigned size)
> > +{
> > +    AwEmacState *s = opaque;
> > +    AwEmacFifo *fifo;
> > +    NetClientState *nc = qemu_get_queue(s->nic);
> > +    int chan;
> > +
> > +    switch (offset) {
> > +    case EMAC_CTL_REG:
> > +        if (value & EMAC_CTL_RESET) {
> > +            aw_emac_reset(s);
> > +            value &= ~EMAC_CTL_RESET;
> > +        }
> > +        s->ctl = value;
> > +        if (aw_emac_can_receive(nc)) {
> > +            qemu_flush_queued_packets(nc);
> > +        }
> > +        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(nc, 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_TX_FIFOS ? 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) {
> > +            qemu_log_mask(LOG_GUEST_ERROR,
> > +                          "allwinner_emac: 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 + 4 > FIFO_SIZE) {
> > +            qemu_log_mask(LOG_GUEST_ERROR,
> > +                          "allwinner_emac: TX data overruns fifo (%d)\n",
> > +                          fifo->offset);
> > +            break;
> > +        }
> > +        fifo->data[fifo->offset++] = value;
> > +        fifo->data[fifo->offset++] = value >> 8;
> > +        fifo->data[fifo->offset++] = value >> 16;
> > +        fifo->data[fifo->offset++] = value >> 24;
> > +        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, extract32(s->phy_target, PHY_ADDR_SHIFT, 8),
> > +                  extract32(s->phy_target, PHY_REG_SHIFT, 8), value);
> > +        break;
> > +    default:
> > +        qemu_log_mask(LOG_UNIMP,
> > +                      "allwinner_emac: write access to unknown register 0x"
> > +                      TARGET_FMT_plx "\n", 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,
> > +    .valid = {
> > +        .min_access_size = 4,
> > +        .max_access_size = 4,
> > +    },
> > +};
> > +
> > +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);
> > +
> > +    s->mii.phy_addr = s->phy_addr;
> > +    aw_emac_reset(s);
> > +    aw_emac_set_link(qemu_get_queue(s->nic));
> > +    mii_reset(&s->mii);
> 
> ... [1]. You are doing a reset as part of a realize. why shouldnt this
> happen as a reset?
> 
> > +}
> > +
> > +static Property aw_emac_properties[] = {
> > +    DEFINE_NIC_PROPERTIES(AwEmacState, conf),
> > +    DEFINE_PROP_UINT8("phyaddr", AwEmacState, phy_addr, 0),
> > +    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),
> 
> The net layer should probably properly set link_ok on realize, so I
> doubt it's migratable state.
> 
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> > +static const VMStateDescription vmstate_fifo = {
> > +    .name = "allwinner_emac_fifo",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_UINT8_ARRAY(data, AwEmacFifo, FIFO_SIZE),
> > +        VMSTATE_INT32(length, AwEmacFifo),
> > +        VMSTATE_INT32(offset, AwEmacFifo),
> > +        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_STRUCT_ARRAY(tx_fifos, AwEmacState,
> > +                             NUM_TX_FIFOS, 1, vmstate_fifo, AwEmacFifo),
> > +        VMSTATE_INT32(tx_channel, AwEmacState),
> > +        VMSTATE_STRUCT_ARRAY(rx_fifos, AwEmacState,
> > +                             NUM_RX_FIFOS, 1, vmstate_fifo, AwEmacFifo),
> > +        VMSTATE_INT32(first_rx, AwEmacState),
> > +        VMSTATE_INT32(num_rx, 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..f92b9d4
> > --- /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 <address@hidden>
> > + *
> > + * Allwinner EMAC register definitions from Linux kernel are:
> > + *   Copyright 2012 Stefan Roese <address@hidden>
> > + *   Copyright 2013 Maxime Ripard <address@hidden>
> > + *   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
> > +
> 
> With such a long list of defines, a few line breaks between logical
> groupings would be easier on the eyes. I've spaced it out just based
> on naming to illustrate.

Ok.

> 
> > +/* 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_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 NUM_TX_FIFOS        2
> > +#define NUM_RX_FIFOS        12
> 
> ... [2] can you tell me where you got this number from?
> 
> > +#define FIFO_SIZE           2048
> 
> And this one.

Not having a document that specifies the properties of the device,
they are just a guess.

Beniamino