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

qemu-devel
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/2] hw/net: add support for Allwinner EMAC F

From:	Peter Crosthwaite
Subject:	Re: [Qemu-devel] [PATCH v2 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller
Date:	Thu, 16 Jan 2014 09:29:30 +1000
On Thu, Jan 16, 2014 at 7:42 AM, Beniamino Galvani <address@hidden> 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 <address@hidden> wrote:
>> > This patch adds support for the Fast Ethernet MAC found on Allwinner
>> > SoCs, together with a basic emulation of Realtek RTL8201CP PHY.
>> >
>> > Since there is no public documentation of the Allwinner controller, the
>> > implementation is based on Linux kernel driver.
>> >
>> > Signed-off-by: Beniamino Galvani <address@hidden>
>> > ---
>> >  default-configs/arm-softmmu.mak |    1 +
>> >  hw/net/Makefile.objs            |    1 +
>> >  hw/net/allwinner_emac.c         |  472 
>> > +++++++++++++++++++++++++++++++++++++++
>> >  include/hw/net/allwinner_emac.h |  204 +++++++++++++++++
>> >  4 files changed, 678 insertions(+)
>> >  create mode 100644 hw/net/allwinner_emac.c
>> >  create mode 100644 include/hw/net/allwinner_emac.h
>> >
>> > diff --git a/default-configs/arm-softmmu.mak 
>> > b/default-configs/arm-softmmu.mak
>> > index ce1d620..f3513fa 100644
>> > --- a/default-configs/arm-softmmu.mak
>> > +++ b/default-configs/arm-softmmu.mak
>> > @@ -27,6 +27,7 @@ CONFIG_SSI_SD=y
>> >  CONFIG_SSI_M25P80=y
>> >  CONFIG_LAN9118=y
>> >  CONFIG_SMC91C111=y
>> > +CONFIG_ALLWINNER_EMAC=y
>> >  CONFIG_DS1338=y
>> >  CONFIG_PFLASH_CFI01=y
>> >  CONFIG_PFLASH_CFI02=y
>> > diff --git a/hw/net/Makefile.objs b/hw/net/Makefile.objs
>> > index 951cca3..75e80c2 100644
>> > --- a/hw/net/Makefile.objs
>> > +++ b/hw/net/Makefile.objs
>> > @@ -18,6 +18,7 @@ common-obj-$(CONFIG_OPENCORES_ETH) += opencores_eth.o
>> >  common-obj-$(CONFIG_XGMAC) += xgmac.o
>> >  common-obj-$(CONFIG_MIPSNET) += mipsnet.o
>> >  common-obj-$(CONFIG_XILINX_AXI) += xilinx_axienet.o
>> > +common-obj-$(CONFIG_ALLWINNER_EMAC) += allwinner_emac.o
>> >
>> >  common-obj-$(CONFIG_CADENCE) += cadence_gem.o
>> >  common-obj-$(CONFIG_STELLARIS_ENET) += stellaris_enet.o
>> > diff --git a/hw/net/allwinner_emac.c b/hw/net/allwinner_emac.c
>> > new file mode 100644
>> > index 0000000..0c37df2
>> > --- /dev/null
>> > +++ b/hw/net/allwinner_emac.c
>> > @@ -0,0 +1,472 @@
>> > +/*
>> > + * Emulation of Allwinner EMAC Fast Ethernet controller and
>> > + * Realtek RTL8201CP PHY
>> > + *
>> > + * Copyright (C) 2013 Beniamino Galvani <address@hidden>
>> > + *
>> > + * This program is free software; you can redistribute it and/or modify
>> > + * it under the terms of the GNU General Public License as published by
>> > + * the Free Software Foundation; either version 2 of the License, or
>> > + * (at your option) any later version.
>> > + *
>> > + * This program is distributed in the hope that it will be useful,
>> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> > + * GNU General Public License for more details.
>> > + *
>> > + */
>> > +#include "hw/sysbus.h"
>> > +#include "net/net.h"
>> > +#include "hw/net/allwinner_emac.h"
>> > +#include <zlib.h>
>> > +
>> > +static void mii_set_link(AwEmacMii *mii, bool link_ok)
>> > +{
>> > +    if (link_ok) {
>> > +        mii->bmsr |= MII_BMSR_LINK_ST;
>> > +        mii->anlpar |= MII_ANAR_TXFD | MII_ANAR_TX | MII_ANAR_10FD |
>> > +                       MII_ANAR_10 | MII_ANAR_CSMACD;
>>
>> mii->anlpar.MII_ANAR_TX is always set so you can drop its transient
>> setting entirely. Here and below.
>
> I will remove it in the code above. In the code below, though, the
> assignment is used to clear all other bits.
>
>>
>> > +    } else {
>> > +        mii->bmsr &= ~MII_BMSR_LINK_ST;
>> > +        mii->anlpar = MII_ANAR_TX;
>> > +    }
>> > +    mii->link_ok = link_ok;
>> > +}
>> > +
>> > +static void mii_reset(AwEmacMii *mii)
>> > +{
>> > +    mii->bmcr = MII_BMCR_FD | MII_BMCR_AUTOEN | MII_BMCR_SPEED;
>> > +    mii->bmsr = MII_BMSR_100TX_FD | MII_BMSR_100TX_HD | MII_BMSR_10T_FD |
>> > +                MII_BMSR_10T_HD | MII_BMSR_MFPS | MII_BMSR_AUTONEG;
>> > +    mii->anar = MII_ANAR_TXFD | MII_ANAR_TX | MII_ANAR_10FD | MII_ANAR_10 
>> > |
>> > +                MII_ANAR_CSMACD;
>> > +    mii->anlpar = MII_ANAR_TX;
>> > +    mii_set_link(mii, mii->link_ok);
>>
>> If this is actually needed, it smells like a bug in the net layer. Is
>> the net layer not doing it's resets properly? It's quite odd that you
>> are preserving old state across a reset.
>
> This is needed to align the local link status to the net layer after a
> reset.
>

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 <address@hidden>
>> > + *
>> > + * Allwinner EMAC register definitions from Linux kernel are:
>> > + *   Copyright 2012 Stefan Roese <address@hidden>
>> > + *   Copyright 2013 Maxime Ripard <address@hidden>
>> > + *   Copyright 1997 Sten Wang
>> > + *
>> > + * This program is free software; you can redistribute it and/or modify
>> > + * it under the terms of the GNU General Public License as published by
>> > + * the Free Software Foundation; either version 2 of the License, or
>> > + * (at your option) any later version.
>> > + *
>> > + * This program is distributed in the hope that it will be useful,
>> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> > + * GNU General Public License for more details.
>> > + *
>> > + */
>> > +#ifndef AW_EMAC_H
>> > +#define AW_EMAC_H
>> > +
>> > +#include "net/net.h"
>> > +
>> > +#define TYPE_AW_EMAC "allwinner_emac"
>> > +#define AW_EMAC(obj) OBJECT_CHECK(AwEmacState, (obj), TYPE_AW_EMAC)
>> > +
>> > +/*
>> > + * Allwinner EMAC register list
>> > + */
>> > +#define EMAC_CTL_REG            0x00
>>
>> > +#define EMAC_TX_MODE_REG        0x04
>> > +#define EMAC_TX_FLOW_REG        0x08
>> > +#define EMAC_TX_CTL0_REG        0x0C
>> > +#define EMAC_TX_CTL1_REG        0x10
>> > +#define EMAC_TX_INS_REG         0x14
>> > +#define EMAC_TX_PL0_REG         0x18
>> > +#define EMAC_TX_PL1_REG         0x1C
>> > +#define EMAC_TX_STA_REG         0x20
>> > +#define EMAC_TX_IO_DATA_REG     0x24
>> > +#define EMAC_TX_IO_DATA1_REG    0x28
>> > +#define EMAC_TX_TSVL0_REG       0x2C
>> > +#define EMAC_TX_TSVH0_REG       0x30
>> > +#define EMAC_TX_TSVL1_REG       0x34
>> > +#define EMAC_TX_TSVH1_REG       0x38
>>
>> > +#define EMAC_RX_CTL_REG         0x3C
>> > +#define EMAC_RX_HASH0_REG       0x40
>> > +#define EMAC_RX_HASH1_REG       0x44
>> > +#define EMAC_RX_STA_REG         0x48
>> > +#define EMAC_RX_IO_DATA_REG     0x4C
>> > +#define EMAC_RX_FBC_REG         0x50
>>
>> > +#define EMAC_INT_CTL_REG        0x54
>> > +#define EMAC_INT_STA_REG        0x58
>>
>> > +#define EMAC_MAC_CTL0_REG       0x5C
>> > +#define EMAC_MAC_CTL1_REG       0x60
>> > +#define EMAC_MAC_IPGT_REG       0x64
>> > +#define EMAC_MAC_IPGR_REG       0x68
>> > +#define EMAC_MAC_CLRT_REG       0x6C
>> > +#define EMAC_MAC_MAXF_REG       0x70
>> > +#define EMAC_MAC_SUPP_REG       0x74
>> > +#define EMAC_MAC_TEST_REG       0x78
>> > +#define EMAC_MAC_MCFG_REG       0x7C
>> > +#define EMAC_MAC_MCMD_REG       0x80
>> > +#define EMAC_MAC_MADR_REG       0x84
>> > +#define EMAC_MAC_MWTD_REG       0x88
>> > +#define EMAC_MAC_MRDD_REG       0x8C
>> > +#define EMAC_MAC_MIND_REG       0x90
>> > +#define EMAC_MAC_SSRR_REG       0x94
>> > +#define EMAC_MAC_A0_REG         0x98
>> > +#define EMAC_MAC_A1_REG         0x9C
>> > +#define EMAC_MAC_A2_REG         0xA0
>>
>> > +#define EMAC_SAFX_L_REG0        0xA4
>> > +#define EMAC_SAFX_H_REG0        0xA8
>> > +#define EMAC_SAFX_L_REG1        0xAC
>> > +#define EMAC_SAFX_H_REG1        0xB0
>> > +#define EMAC_SAFX_L_REG2        0xB4
>> > +#define EMAC_SAFX_H_REG2        0xB8
>> > +#define EMAC_SAFX_L_REG3        0xBC
>> > +#define EMAC_SAFX_H_REG3        0xC0
>> > +
>>
>> With such a long list of defines, a few line breaks between logical
>> groupings would be easier on the eyes. I've spaced it out just based
>> on naming to illustrate.
>
> Ok.
>
>>
>> > +/* CTL register fields */
>> > +#define EMAC_CTL_RESET                  (1 << 0)
>> > +#define EMAC_CTL_TX_EN                  (1 << 1)
>> > +#define EMAC_CTL_RX_EN                  (1 << 2)
>> > +
>> > +/* TX MODE register fields */
>> > +#define EMAC_TX_MODE_ABORTED_FRAME_EN   (1 << 0)
>> > +#define EMAC_TX_MODE_DMA_EN             (1 << 1)
>> > +
>> > +/* RX CTL register fields */
>> > +#define EMAC_RX_CTL_AUTO_DRQ_EN         (1 << 1)
>> > +#define EMAC_RX_CTL_DMA_EN              (1 << 2)
>> > +#define EMAC_RX_CTL_PASS_ALL_EN         (1 << 4)
>> > +#define EMAC_RX_CTL_PASS_CTL_EN         (1 << 5)
>> > +#define EMAC_RX_CTL_PASS_CRC_ERR_EN     (1 << 6)
>> > +#define EMAC_RX_CTL_PASS_LEN_ERR_EN     (1 << 7)
>> > +#define EMAC_RX_CTL_PASS_LEN_OOR_EN     (1 << 8)
>> > +#define EMAC_RX_CTL_ACCEPT_UNICAST_EN   (1 << 16)
>> > +#define EMAC_RX_CTL_DA_FILTER_EN        (1 << 17)
>> > +#define EMAC_RX_CTL_ACCEPT_MULTICAST_EN (1 << 20)
>> > +#define EMAC_RX_CTL_HASH_FILTER_EN      (1 << 21)
>> > +#define EMAC_RX_CTL_ACCEPT_BROADCAST_EN (1 << 22)
>> > +#define EMAC_RX_CTL_SA_FILTER_EN        (1 << 24)
>> > +#define EMAC_RX_CTL_SA_FILTER_INVERT_EN (1 << 25)
>> > +
>> > +/* RX IO DATA register fields */
>> > +#define EMAC_RX_IO_DATA_LEN(x)          (x & 0xffff)
>> > +#define EMAC_RX_HEADER(len, status)     (((len) & 0xffff) | ((status) << 
>> > 16))
>> > +#define EMAC_RX_IO_DATA_STATUS_CRC_ERR  (1 << 4)
>> > +#define EMAC_RX_IO_DATA_STATUS_LEN_ERR  (3 << 5)
>> > +#define EMAC_RX_IO_DATA_STATUS_OK       (1 << 7)
>> > +#define EMAC_UNDOCUMENTED_MAGIC         0x0143414d  /* header for RX 
>> > frames */
>> > +
>> > +/* PHY registers */
>> > +#define MII_BMCR            0
>> > +#define MII_BMSR            1
>> > +#define MII_PHYID1          2
>> > +#define MII_PHYID2          3
>> > +#define MII_ANAR            4
>> > +#define MII_ANLPAR          5
>> > +
>> > +/* PHY registers fields */
>> > +#define MII_BMCR_RESET      (1 << 15)
>> > +#define MII_BMCR_LOOPBACK   (1 << 14)
>> > +#define MII_BMCR_SPEED      (1 << 13)
>> > +#define MII_BMCR_AUTOEN     (1 << 12)
>> > +#define MII_BMCR_FD         (1 << 8)
>> > +
>> > +#define MII_BMSR_100TX_FD   (1 << 14)
>> > +#define MII_BMSR_100TX_HD   (1 << 13)
>> > +#define MII_BMSR_10T_FD     (1 << 12)
>> > +#define MII_BMSR_10T_HD     (1 << 11)
>> > +#define MII_BMSR_MFPS       (1 << 6)
>> > +#define MII_BMSR_AUTONEG    (1 << 3)
>> > +#define MII_BMSR_LINK_ST    (1 << 2)
>> > +
>> > +#define MII_ANAR_TXFD       (1 << 8)
>> > +#define MII_ANAR_TX         (1 << 7)
>> > +#define MII_ANAR_10FD       (1 << 6)
>> > +#define MII_ANAR_10         (1 << 5)
>> > +#define MII_ANAR_CSMACD     (1 << 0)
>> > +
>> > +#define RTL8201CP_PHYID1    0x0000
>> > +#define RTL8201CP_PHYID2    0x8201
>> > +
>> > +/* INT CTL and INT STA registers fields */
>> > +#define EMAC_INT_TX_CHAN(x) (1 << (x))
>> > +#define EMAC_INT_RX         (1 << 8)
>> > +
>> > +#define NUM_TX_FIFOS        2
>> > +#define NUM_RX_FIFOS        12
>>
>> ... [2] can you tell me where you got this number from?
>>
>> > +#define FIFO_SIZE           2048
>>
>> And this one.
>
> Not having a document that specifies the properties of the device,
> they are just a guess.
>
> Beniamino
>
[Prev in Thread]
Current Thread
[Next in Thread]
Re: [Qemu-devel] [PATCH v2 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller, (continued)
- [Qemu-devel] [PATCH v2 2/2] hw/arm/allwinner-a10: initialize EMAC, Beniamino Galvani, 2014/01/11
Prev by Date: Re: [Qemu-devel] Secondary VGA passthrough not working, hitting linux kernel's pci_fixup_video
Next by Date: [Qemu-devel] [Bug 1269628] [NEW] Feature Request: Please add TCG OPAL 2 emulation support to the virtio disk emulation
Previous by thread: Re: [Qemu-devel] [PATCH v2 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller
Next by thread: [Qemu-devel] [PATCH v2 2/2] hw/arm/allwinner-a10: initialize EMAC
Index(es):
- Date
- Thread