[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 1/2] hw/net: add support for Allwinner EMAC F
|
From: |
Peter Crosthwaite |
|
Subject: |
Re: [Qemu-devel] [PATCH v3 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller |
|
Date: |
Sun, 26 Jan 2014 09:58:03 +1000 |
On Sat, Jan 25, 2014 at 11:37 PM, Beniamino Galvani <address@hidden> wrote:
> On Thu, Jan 23, 2014 at 11:04:32PM +1000, Peter Crosthwaite wrote:
>> On Mon, Jan 20, 2014 at 9:25 AM, 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 | 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 <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 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?
>
Current master. You are much closer to a merge here, than I am with that one.
Regards,
Peter
>>
>> > +{
>> > + 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
>