[v2,1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller

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         |  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

On Sat, Jan 11, 2014 at 11:13:32AM +0100, Beniamino Galvani 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         |  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

Acked-by: Stefan Hajnoczi <stefanha@redhat.com>

On Sat, Jan 11, 2014 at 8:13 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         |  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 <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>
> +
> +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.

> +    } 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.

> +}
> +
> +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.

> +{
> +    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.

> +            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?

> +            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.

> +    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] ...

> +}
> +
> +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.

> +            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 <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
> +

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.

> +/* 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.

FYI Im working off this:

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/allwinner/sun4i-emac.h

Regards,
Peter

> +#define RX_HDR_SIZE         8
> +#define CRC_SIZE            4
> +
> +#define PHY_REG_SHIFT       0
> +#define PHY_ADDR_SHIFT      8
> +
> +typedef struct AwEmacFifo {
> +    uint8_t  data[FIFO_SIZE];
> +    int      length;
> +    int      offset;
> +} AwEmacFifo;
> +
> +typedef struct AwEmacMii {
> +    uint16_t bmcr;
> +    uint16_t bmsr;
> +    uint16_t anar;
> +    uint16_t anlpar;
> +    bool     link_ok;
> +    uint8_t  phy_addr;
> +} AwEmacMii;
> +
> +typedef struct AwEmacState {
> +    /*< private >*/
> +    SysBusDevice  parent_obj;
> +    /*< public >*/
> +
> +    MemoryRegion  iomem;
> +    qemu_irq      irq;
> +    NICState      *nic;
> +    NICConf       conf;
> +    AwEmacMii     mii;
> +    uint8_t       phy_addr;
> +
> +    uint32_t      ctl;
> +    uint32_t      tx_mode;
> +    uint32_t      rx_ctl;
> +    uint32_t      int_ctl;
> +    uint32_t      int_sta;
> +    uint32_t      phy_target;
> +
> +    AwEmacFifo    tx_fifos[NUM_TX_FIFOS];
> +    int           tx_channel;             /* Currently selected TX channel */
> +
> +    AwEmacFifo    rx_fifos[NUM_RX_FIFOS];
> +    int           first_rx;               /* First packet in the RX ring */
> +    int           num_rx;                 /* Number of packets in RX ring */
> +} AwEmacState;
> +
> +#endif

cc Stefan

On Mon, Jan 13, 2014 at 11:15 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Sat, Jan 11, 2014 at 8:13 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         |  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 <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>
>> +
>> +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.
>
>> +    } 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.
>
>> +}
>> +
>> +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.
>
>> +{
>> +    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.
>
>> +            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?
>
>> +            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.
>
>> +    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] ...
>
>> +}
>> +
>> +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.
>
>> +            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 <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
>> +
>
> 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.
>
>> +/* 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.
>
> FYI Im working off this:
>
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/allwinner/sun4i-emac.h
>
> Regards,
> Peter
>
>> +#define RX_HDR_SIZE         8
>> +#define CRC_SIZE            4
>> +
>> +#define PHY_REG_SHIFT       0
>> +#define PHY_ADDR_SHIFT      8
>> +
>> +typedef struct AwEmacFifo {
>> +    uint8_t  data[FIFO_SIZE];
>> +    int      length;
>> +    int      offset;
>> +} AwEmacFifo;
>> +
>> +typedef struct AwEmacMii {
>> +    uint16_t bmcr;
>> +    uint16_t bmsr;
>> +    uint16_t anar;
>> +    uint16_t anlpar;
>> +    bool     link_ok;
>> +    uint8_t  phy_addr;
>> +} AwEmacMii;
>> +
>> +typedef struct AwEmacState {
>> +    /*< private >*/
>> +    SysBusDevice  parent_obj;
>> +    /*< public >*/
>> +
>> +    MemoryRegion  iomem;
>> +    qemu_irq      irq;
>> +    NICState      *nic;
>> +    NICConf       conf;
>> +    AwEmacMii     mii;
>> +    uint8_t       phy_addr;
>> +
>> +    uint32_t      ctl;
>> +    uint32_t      tx_mode;
>> +    uint32_t      rx_ctl;
>> +    uint32_t      int_ctl;
>> +    uint32_t      int_sta;
>> +    uint32_t      phy_target;
>> +
>> +    AwEmacFifo    tx_fifos[NUM_TX_FIFOS];
>> +    int           tx_channel;             /* Currently selected TX channel */
>> +
>> +    AwEmacFifo    rx_fifos[NUM_RX_FIFOS];
>> +    int           first_rx;               /* First packet in the RX ring */
>> +    int           num_rx;                 /* Number of packets in RX ring */
>> +} AwEmacState;
>> +
>> +#endif

On Mon, Jan 13, 2014 at 11:16:37PM +1000, Peter Crosthwaite wrote:
> On Mon, Jan 13, 2014 at 11:15 PM, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
> > On Sat, Jan 11, 2014 at 8:13 PM, Beniamino Galvani <b.galvani@gmail.com> wrote:
> >> +    } 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.

The net layer doesn't have a reset point for the link status.  The net
client must explicitly transition states itself.

> >> +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?

It is safer to perform full checks in .receive() since
qemu_net_queue_flush() calls .receive() repeatedly without
.can_receive().  This means especially if the state can change between
calls, you need to do a full check.

> >> +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.

The net layer is not part of live migration.  It's up to the emulated
device to migrate link state and restore it after migration.  In other
words, link state should be migrated as part of PHY vmstate.

On Tue, Jan 14, 2014 at 3:19 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Mon, Jan 13, 2014 at 11:16:37PM +1000, Peter Crosthwaite wrote:
>> On Mon, Jan 13, 2014 at 11:15 PM, Peter Crosthwaite
>> <peter.crosthwaite@xilinx.com> wrote:
>> > On Sat, Jan 11, 2014 at 8:13 PM, Beniamino Galvani <b.galvani@gmail.com> wrote:
>> >> +    } 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.
>
> The net layer doesn't have a reset point for the link status.  The net
> client must explicitly transition states itself.
>
>> >> +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?
>
> It is safer to perform full checks in .receive() since
> qemu_net_queue_flush() calls .receive() repeatedly without
> .can_receive().  This means especially if the state can change between
> calls, you need to do a full check.
>
>> >> +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.
>
> The net layer is not part of live migration.  It's up to the emulated
> device to migrate link state and restore it after migration.  In other
> words, link state should be migrated as part of PHY vmstate.
>

But is the saved link state meaningful? The fact that net is not
migrating means that this link_ok migrated variable is potentially
incoherent with the net layer. E.G. What happens in the following
case:

Run QEMU
Link is Good
Save

Run Again
Link is bad
Load

The loaded emulation will have link_ok due to the vmsd load despite
the link being down in the new net layer environment. It will not
correct until net-layer activity causes a ->link_state_changed
callback.

Can this be just solved cleanly by calling the mii_set_link fn (added
in this patch) always on post load and removing this link_ok state?
This will mean that guest will see a link state transition immediately
on load if the link state changes from the saved-machine state to the
loaded-machine state.

Regards,
Peter

Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:

> But is the saved link state meaningful? The fact that net is not
> migrating means that this link_ok migrated variable is potentially
> incoherent with the net layer. E.G. What happens in the following
> case:
>
> Run QEMU
> Link is Good
> Save
>
> Run Again
> Link is bad
> Load
>
> The loaded emulation will have link_ok due to the vmsd load despite
> the link being down in the new net layer environment. It will not
> correct until net-layer activity causes a ->link_state_changed
> callback.
>
> Can this be just solved cleanly by calling the mii_set_link fn (added
> in this patch) always on post load and removing this link_ok state?
> This will mean that guest will see a link state transition immediately
> on load if the link state changes from the saved-machine state to the
> loaded-machine state.

You can do that on post-load, but the problem is that when you run
post-load, the guest is not running yet on destination, so you need to
be careful about what functions do you use.

Later, Juan.

On Wed, Jan 15, 2014 at 8:25 PM, Juan Quintela <quintela@redhat.com> wrote:
> Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>
>> But is the saved link state meaningful? The fact that net is not
>> migrating means that this link_ok migrated variable is potentially
>> incoherent with the net layer. E.G. What happens in the following
>> case:
>>
>> Run QEMU
>> Link is Good
>> Save
>>
>> Run Again
>> Link is bad
>> Load
>>
>> The loaded emulation will have link_ok due to the vmsd load despite
>> the link being down in the new net layer environment. It will not
>> correct until net-layer activity causes a ->link_state_changed
>> callback.
>>
>> Can this be just solved cleanly by calling the mii_set_link fn (added
>> in this patch) always on post load and removing this link_ok state?
>> This will mean that guest will see a link state transition immediately
>> on load if the link state changes from the saved-machine state to the
>> loaded-machine state.
>
> You can do that on post-load, but the problem is that when you run
> post-load, the guest is not running yet on destination, so you need to
> be careful about what functions do you use.
>

Ok,

But in this case the guest visible side effect is just register state
change. There is no transitional event to worry about. I guess in the
case where a link state transition can cause an interrupt it could get
complicated?

Regards,
Peter

> Later, Juan.
>

On Tue, Jan 14, 2014 at 3:19 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Mon, Jan 13, 2014 at 11:16:37PM +1000, Peter Crosthwaite wrote:
>> On Mon, Jan 13, 2014 at 11:15 PM, Peter Crosthwaite
>> <peter.crosthwaite@xilinx.com> wrote:
>> > On Sat, Jan 11, 2014 at 8:13 PM, Beniamino Galvani <b.galvani@gmail.com> wrote:
>> >> +    } else {
...
>> >> +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?
>
> It is safer to perform full checks in .receive() since
> qemu_net_queue_flush() calls .receive() repeatedly without
> .can_receive().  This means especially if the state can change between
> calls, you need to do a full check.
>

Ok, then original implementation is all good. No change required for
this issue. Sry for the noise.

Regards,
Peter

On Mon, Jan 13, 2014 at 11:15:17PM +1000, Peter Crosthwaite wrote:
> On Sat, Jan 11, 2014 at 8:13 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         |  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 <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>
> > +
> > +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 <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
> > +
> 
> 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

On Thu, Jan 16, 2014 at 7:42 AM, Beniamino Galvani <b.galvani@gmail.com> wrote:
> On Mon, Jan 13, 2014 at 11:15:17PM +1000, Peter Crosthwaite wrote:
>> On Sat, Jan 11, 2014 at 8:13 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         |  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 <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>
>> > +
>> > +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.
>

Yes I agree following Stefans comments. Although this could change
depending on how that VMSD tangential discussion concludes.

>> > +}
>> > +
>> > +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?
>

Yes. I think you just did the hard work (the research). Now its just a case of:

case MDIO_NSR:
case MDIO_LBREMR:
...
case MDIO_TEST:
   qemu_log_mask(LOG_UNIMP,
   break;
default:
   qemu_log_mask(LOG_GUEST_ERROR,
}

>>
>> > +            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.

Ok sounds good.

> Do you have any suggestions for deciding
> its size?
>

Be generous. currently you have 12*2K = 24K. Round up the 32K and I
thinks that's as good as any without any specs. Add a comment to the
def explaining that it's arbitrary.

Regards,
Peter

>>
>> > +            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 <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
>> > +
>>
>> 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
>

On Wed, Jan 15, 2014 at 06:24:24PM +1000, Peter Crosthwaite wrote:
> On Tue, Jan 14, 2014 at 3:19 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > On Mon, Jan 13, 2014 at 11:16:37PM +1000, Peter Crosthwaite wrote:
> >> On Mon, Jan 13, 2014 at 11:15 PM, Peter Crosthwaite
> >> <peter.crosthwaite@xilinx.com> wrote:
> >> > On Sat, Jan 11, 2014 at 8:13 PM, Beniamino Galvani <b.galvani@gmail.com> wrote:
> >> >> +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.
> >
> > The net layer is not part of live migration.  It's up to the emulated
> > device to migrate link state and restore it after migration.  In other
> > words, link state should be migrated as part of PHY vmstate.
> >
> 
> But is the saved link state meaningful? The fact that net is not
> migrating means that this link_ok migrated variable is potentially
> incoherent with the net layer. E.G. What happens in the following
> case:
> 
> Run QEMU
> Link is Good
> Save
> 
> Run Again
> Link is bad
> Load
> 
> The loaded emulation will have link_ok due to the vmsd load despite
> the link being down in the new net layer environment. It will not
> correct until net-layer activity causes a ->link_state_changed
> callback.

I would agrue that this example is not valid.  NetClient->link_down is
influenced by two things:
1. User actions that disable the link (e.g. setting link status)
2. Backends may take the link down, e.g. if the net/socket.c connection
   breaks.

In either case, the guest should migrate with its original link status.
Then, when it resumes on the destination host the link state may change
or an interrupt can be raised.

> Can this be just solved cleanly by calling the mii_set_link fn (added
> in this patch) always on post load and removing this link_ok state?
> This will mean that guest will see a link state transition immediately
> on load if the link state changes from the saved-machine state to the
> loaded-machine state.

Is the link_ok field even necessary?  Perhaps we should just rely on
NetClient->link_ok.

On Thu, Jan 16, 2014 at 12:45 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Wed, Jan 15, 2014 at 06:24:24PM +1000, Peter Crosthwaite wrote:
>> On Tue, Jan 14, 2014 at 3:19 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>> > On Mon, Jan 13, 2014 at 11:16:37PM +1000, Peter Crosthwaite wrote:
>> >> On Mon, Jan 13, 2014 at 11:15 PM, Peter Crosthwaite
>> >> <peter.crosthwaite@xilinx.com> wrote:
>> >> > On Sat, Jan 11, 2014 at 8:13 PM, Beniamino Galvani <b.galvani@gmail.com> wrote:
>> >> >> +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.
>> >
>> > The net layer is not part of live migration.  It's up to the emulated
>> > device to migrate link state and restore it after migration.  In other
>> > words, link state should be migrated as part of PHY vmstate.
>> >
>>
>> But is the saved link state meaningful? The fact that net is not
>> migrating means that this link_ok migrated variable is potentially
>> incoherent with the net layer. E.G. What happens in the following
>> case:
>>
>> Run QEMU
>> Link is Good
>> Save
>>
>> Run Again
>> Link is bad
>> Load
>>
>> The loaded emulation will have link_ok due to the vmsd load despite
>> the link being down in the new net layer environment. It will not
>> correct until net-layer activity causes a ->link_state_changed
>> callback.
>
> I would agrue that this example is not valid.  NetClient->link_down is
> influenced by two things:
> 1. User actions that disable the link (e.g. setting link status)
> 2. Backends may take the link down, e.g. if the net/socket.c connection
>    breaks.
>
> In either case, the guest should migrate with its original link status.
> Then, when it resumes on the destination host the link state may change
> or an interrupt can be raised.
>

Will you reliably get your link_state_changed callback post_load though?

>> Can this be just solved cleanly by calling the mii_set_link fn (added
>> in this patch) always on post load and removing this link_ok state?
>> This will mean that guest will see a link state transition immediately
>> on load if the link state changes from the saved-machine state to the
>> loaded-machine state.
>
> Is the link_ok field even necessary?

No. Thats my theory underpinning my proposed change anyway.

> Perhaps we should just rely on
> NetClient->link_ok.
>

I think we are in agreement then? There is no need for phy.link_ok as
same information is captured in NetClient. This means there is no
link_ok field to migrate. Just migrate the phy registers themselves.

For completeness you just need to poll NetClient state post_load which
will nicely handle my degenerate case of link activity between save
and load (same handler as link_state_changed). In the more usual case
of the link state not changing between save and load, this function
call will just be a nop, leading to no change of state on the freshly
migrated phy registers.

Regards
Peter

On Thu, Jan 16, 2014 at 10:52:19PM +1000, Peter Crosthwaite wrote:
> > Perhaps we should just rely on
> > NetClient->link_ok.
> >
> 
> I think we are in agreement then? There is no need for phy.link_ok as
> same information is captured in NetClient. This means there is no
> link_ok field to migrate. Just migrate the phy registers themselves.
> 
> For completeness you just need to poll NetClient state post_load which
> will nicely handle my degenerate case of link activity between save
> and load (same handler as link_state_changed). In the more usual case
> of the link state not changing between save and load, this function
> call will just be a nop, leading to no change of state on the freshly
> migrated phy registers.

Sounds good.

Stefan