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

[Beniamino Galvani <b.galvani@gmail.com>]

On Thu, Jan 23, 2014 at 11:04:32PM +1000, Peter Crosthwaite wrote:
> On Mon, Jan 20, 2014 at 9:25 AM, 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         |  589 +++++++++++++++++++++++++++++++++++++++
> >  include/hw/net/allwinner_emac.h |  222 +++++++++++++++
> >  4 files changed, 813 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..4cad7e0
> > --- /dev/null
> > +++ b/hw/net/allwinner_emac.c
> > @@ -0,0 +1,589 @@
> > +/*
> > + * Emulation of Allwinner EMAC Fast Ethernet controller and
> > + * Realtek RTL8201CP PHY
> > + *
> > + * Copyright (C) 2014 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 uint8_t padding[60];
> > +
> > +static void mii_set_link(RTL8201CPState *mii, bool link_ok)
> > +{
> > +    if (link_ok) {
> > +        mii->bmsr |= MII_BMSR_LINK_ST;
> > +        mii->anlpar |= MII_ANAR_TXFD | MII_ANAR_10FD | MII_ANAR_10 |
> > +                       MII_ANAR_CSMACD;
> > +    } else {
> > +        mii->bmsr &= ~MII_BMSR_LINK_ST;
> > +        mii->anlpar = MII_ANAR_TX;
> > +    }
> > +}
> > +
> > +static void mii_reset(RTL8201CPState *mii, bool link_ok)
> > +{
> > +    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, link_ok);
> > +}
> > +
> > +static uint16_t aw_emac_mdio_read(AwEmacState *s, uint8_t addr, uint8_t reg)
> 
> Drop the AW reference here (replace with "RTL8201").
> 
> > +{
> > +    RTL8201CPState *mii = &s->mii;
> > +    uint16_t ret = 0xffff;
> > +
> > +    if (addr == s->phy_addr) {
> > +        switch (reg) {
> > +        case MII_BMCR:
> > +            return mii->bmcr;
> > +        case MII_BMSR:
> > +            return mii->bmsr;
> > +        case MII_PHYID1:
> > +            return RTL8201CP_PHYID1;
> > +        case MII_PHYID2:
> > +            return RTL8201CP_PHYID2;
> > +        case MII_ANAR:
> > +            return mii->anar;
> > +        case MII_ANLPAR:
> > +            return mii->anlpar;
> > +        case MII_ANER:
> > +        case MII_NSR:
> > +        case MII_LBREMR:
> > +        case MII_REC:
> > +        case MII_SNRDR:
> > +        case MII_TEST:
> > +            qemu_log_mask(LOG_UNIMP,
> > +                          "allwinner_emac: read from unimpl. mii reg 0x%x\n",
> > +                          reg);
> > +            return 0;
> > +        default:
> > +            qemu_log_mask(LOG_GUEST_ERROR,
> > +                          "allwinner_emac: read from invalid mii reg 0x%x\n",
> > +                          reg);
> > +            return 0;
> > +        }
> > +    }
> > +    return ret;
> > +}
> > +
> > +static void aw_emac_mdio_write(AwEmacState *s, uint8_t addr, uint8_t reg,
> > +                               uint16_t value)
> > +{
> > +    RTL8201CPState *mii = &s->mii;
> > +    NetClientState *nc;
> > +
> > +    if (addr == s->phy_addr) {
> > +        switch (reg) {
> > +        case MII_BMCR:
> > +            if (value & MII_BMCR_RESET) {
> > +                nc = qemu_get_queue(s->nic);
> > +                mii_reset(mii, !nc->link_down);
> > +            } else {
> > +                mii->bmcr = value;
> > +            }
> > +            break;
> > +        case MII_ANAR:
> > +            mii->anar = value;
> > +            break;
> > +        case MII_BMSR:
> > +        case MII_PHYID1:
> > +        case MII_PHYID2:
> > +        case MII_ANLPAR:
> > +        case MII_ANER:
> > +            qemu_log_mask(LOG_GUEST_ERROR,
> > +                          "allwinner_emac: write to read-only mii reg 0x%x\n",
> > +                          reg);
> > +            break;
> 
> Theres plenty of precedent for not bothering making the distinction
> between read_only and out of range if you just want to collapse into
> the one GUEST_ERROR message. Either way though.
> 
> > +        case MII_NSR:
> > +        case MII_LBREMR:
> > +        case MII_REC:
> > +        case MII_SNRDR:
> > +        case MII_TEST:
> > +            qemu_log_mask(LOG_UNIMP,
> > +                          "allwinner_emac: write to unimpl. mii reg 0x%x\n",
> > +                          reg);
> > +            break;
> > +        default:
> > +            qemu_log_mask(LOG_GUEST_ERROR,
> > +                          "allwinner_emac: write to invalid 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 void aw_emac_tx_reset(AwEmacTxFifo *fifo)
> > +{
> > +    fifo->length = 0;
> > +    fifo->offset = 0;
> > +}
> > +
> > +static int aw_emac_tx_has_space(AwEmacTxFifo *fifo, int n)
> > +{
> > +    return fifo->offset + n <= TX_FIFO_SIZE;
> > +}
> > +
> > +static void aw_emac_tx_pushw(AwEmacTxFifo *fifo, uint32_t val)
> > +{
> > +    assert(fifo->offset + 4 <= TX_FIFO_SIZE);
> > +    fifo->data[fifo->offset++] = val;
> > +    fifo->data[fifo->offset++] = val >> 8;
> > +    fifo->data[fifo->offset++] = val >> 16;
> > +    fifo->data[fifo->offset++] = val >> 24;
> > +}
> > +
> > +static void aw_emac_rx_reset(AwEmacRxFifo *fifo)
> > +{
> > +    fifo->head = 0;
> > +    fifo->num = 0;
> > +    fifo->num_packets = 0;
> > +    fifo->packet_size = 0;
> > +    fifo->packet_pos = 0;
> > +}
> > +
> > +static int aw_emac_rx_has_space(AwEmacRxFifo *fifo, int n)
> > +{
> > +    return fifo->num + n <= RX_FIFO_SIZE;
> > +}
> > +
> > +static void aw_emac_rx_push(AwEmacRxFifo *fifo, const uint8_t *data, int size)
> 
> This is pretty much exactly the proposed addition to the fifo API to
> solve the no-buffer-write incompletness. I'm happy to ACK a patch to
> that API with this implementation, especially if it means you can
> collapse use for the rx fifo here to save on the ring-buffer
> boilerplate.

Should I base the patch on top of your proposed generalisation to
different element widths or on current master?

> 
> > +{
> > +    int index;
> > +
> > +    assert(fifo->num + size <= RX_FIFO_SIZE);
> > +    index = (fifo->head + fifo->num) % RX_FIFO_SIZE;
> > +
> > +    if (index + size <= RX_FIFO_SIZE) {
> > +        memcpy(&fifo->data[index], data, size);
> > +    } else {
> > +        memcpy(&fifo->data[index], data, RX_FIFO_SIZE - index);
> > +        memcpy(&fifo->data[0], &data[RX_FIFO_SIZE - index],
> > +               size - RX_FIFO_SIZE + index);
> > +    }
> > +
> > +    fifo->num += size;
> > +}
> > +
> > +static void aw_emac_rx_pushb(AwEmacRxFifo *fifo, uint8_t val)
> > +{
> > +    return aw_emac_rx_push(fifo, &val, 1);
> > +}
> > +
> > +static void aw_emac_rx_pushw(AwEmacRxFifo *fifo, uint32_t val)
> > +{
> > +    aw_emac_rx_pushb(fifo, val);
> > +    aw_emac_rx_pushb(fifo, val >> 8);
> > +    aw_emac_rx_pushb(fifo, val >> 16);
> > +    aw_emac_rx_pushb(fifo, val >> 24);
> > +}
> > +
> > +static uint8_t aw_emac_rx_popb(AwEmacRxFifo *fifo)
> > +{
> > +    uint8_t ret;
> > +
> > +    assert(fifo->num > 0);
> > +    ret = fifo->data[fifo->head];
> > +    fifo->head = (fifo->head + 1) % RX_FIFO_SIZE;
> > +    fifo->num--;
> > +
> > +    return ret;
> > +}
> > +
> > +static uint32_t aw_emac_rx_popw(AwEmacRxFifo *fifo)
> > +{
> > +    uint32_t ret;
> > +
> > +    ret = aw_emac_rx_popb(fifo);
> > +    ret |= aw_emac_rx_popb(fifo) << 8;
> > +    ret |= aw_emac_rx_popb(fifo) << 16;
> > +    ret |= aw_emac_rx_popb(fifo) << 24;
> > +
> > +    return ret;
> > +}
> > +
> > +static int aw_emac_can_receive(NetClientState *nc)
> > +{
> > +    AwEmacState *s = qemu_get_nic_opaque(nc);
> > +
> > +    return s->ctl & EMAC_CTL_RX_EN;
> 
> So I'm not a fan of ethernets that just drop all packets on a full
> fifo (they tend to have very poor tftp performance esp. when using
> u-boot as a guest). Since we completely fabricated the size of the rx
> fifo (due to no specs), you could just make a conservative assumption
> that you need a full frames-worth of space spare before you accept a
> packet of any size. There simple is no known or specified behaviour to
> preserve here WRT to fifo occupancy, so having some form of fifo-full
> awareness will at least give you good performance.

Ok, I will add this check in next version.

Beniamino