* [Qemu-devel] [PATCH v2 0/2] hw/arm: add ethernet support to Allwinner A10
@ 2014-01-11 10:13 Beniamino Galvani
2014-01-11 10:13 ` [Qemu-devel] [PATCH v2 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller Beniamino Galvani
2014-01-11 10:13 ` [Qemu-devel] [PATCH v2 2/2] hw/arm/allwinner-a10: initialize EMAC Beniamino Galvani
0 siblings, 2 replies; 16+ messages in thread
From: Beniamino Galvani @ 2014-01-11 10:13 UTC (permalink / raw)
To: qemu-devel; +Cc: Beniamino Galvani, Peter Maydell, Peter Crosthwaite, Li Guang
This patch series adds support for the EMAC Fast Ethernet controller
found on Allwinner SoCs to the Allwinner A10.
v2: Address Peter Crosthwaite's comments:
* Make phy address customizable through a property
* Call qemu_flush_queued_packets() when rx becomes possible
* Always create EMAC instance in SoC
* Use uint8 arrays for fifos
* Minor cleanups
Beniamino Galvani (2):
hw/net: add support for Allwinner EMAC Fast Ethernet controller
hw/arm/allwinner-a10: initialize EMAC
default-configs/arm-softmmu.mak | 1 +
hw/arm/allwinner-a10.c | 16 ++
hw/arm/cubieboard.c | 7 +
hw/net/Makefile.objs | 1 +
hw/net/allwinner_emac.c | 472 +++++++++++++++++++++++++++++++++++++++
include/hw/arm/allwinner-a10.h | 3 +
include/hw/net/allwinner_emac.h | 204 +++++++++++++++++
7 files changed, 704 insertions(+)
create mode 100644 hw/net/allwinner_emac.c
create mode 100644 include/hw/net/allwinner_emac.h
--
1.7.10.4
^ permalink raw reply [flat|nested] 16+ messages in thread* [Qemu-devel] [PATCH v2 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller 2014-01-11 10:13 [Qemu-devel] [PATCH v2 0/2] hw/arm: add ethernet support to Allwinner A10 Beniamino Galvani @ 2014-01-11 10:13 ` Beniamino Galvani 2014-01-13 5:20 ` Stefan Hajnoczi 2014-01-13 13:15 ` Peter Crosthwaite 2014-01-11 10:13 ` [Qemu-devel] [PATCH v2 2/2] hw/arm/allwinner-a10: initialize EMAC Beniamino Galvani 1 sibling, 2 replies; 16+ messages in thread From: Beniamino Galvani @ 2014-01-11 10:13 UTC (permalink / raw) To: qemu-devel; +Cc: Beniamino Galvani, Peter Maydell, Peter Crosthwaite, Li Guang 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; + } 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); +} + +static uint16_t mii_read(AwEmacMii *mii, uint8_t addr, uint8_t reg) +{ + 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; + 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); + 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) { + return -1; + } + + if (RX_HDR_SIZE + size + CRC_SIZE > FIFO_SIZE) { + return -1; + } + + 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; +} + +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; + 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--; + 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); +} + +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), + 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 + +/* 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 +#define FIFO_SIZE 2048 +#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 -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller 2014-01-11 10:13 ` [Qemu-devel] [PATCH v2 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller Beniamino Galvani @ 2014-01-13 5:20 ` Stefan Hajnoczi 2014-01-13 13:15 ` Peter Crosthwaite 1 sibling, 0 replies; 16+ messages in thread From: Stefan Hajnoczi @ 2014-01-13 5:20 UTC (permalink / raw) To: Beniamino Galvani; +Cc: Peter Maydell, Peter Crosthwaite, qemu-devel, Li Guang 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> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller 2014-01-11 10:13 ` [Qemu-devel] [PATCH v2 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller Beniamino Galvani 2014-01-13 5:20 ` Stefan Hajnoczi @ 2014-01-13 13:15 ` Peter Crosthwaite 2014-01-13 13:16 ` Peter Crosthwaite 2014-01-15 21:42 ` Beniamino Galvani 1 sibling, 2 replies; 16+ messages in thread From: Peter Crosthwaite @ 2014-01-13 13:15 UTC (permalink / raw) To: Beniamino Galvani Cc: Peter Maydell, qemu-devel@nongnu.org Developers, Li Guang 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller 2014-01-13 13:15 ` Peter Crosthwaite @ 2014-01-13 13:16 ` Peter Crosthwaite 2014-01-14 5:19 ` Stefan Hajnoczi 2014-01-15 21:42 ` Beniamino Galvani 1 sibling, 1 reply; 16+ messages in thread From: Peter Crosthwaite @ 2014-01-13 13:16 UTC (permalink / raw) To: Beniamino Galvani, Stefan Hajnoczi Cc: Peter Maydell, qemu-devel@nongnu.org Developers, Li Guang 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller 2014-01-13 13:16 ` Peter Crosthwaite @ 2014-01-14 5:19 ` Stefan Hajnoczi 2014-01-15 8:24 ` Peter Crosthwaite 2014-01-15 12:40 ` Peter Crosthwaite 0 siblings, 2 replies; 16+ messages in thread From: Stefan Hajnoczi @ 2014-01-14 5:19 UTC (permalink / raw) To: Peter Crosthwaite Cc: Beniamino Galvani, Peter Maydell, qemu-devel@nongnu.org Developers, Li Guang 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller 2014-01-14 5:19 ` Stefan Hajnoczi @ 2014-01-15 8:24 ` Peter Crosthwaite 2014-01-15 10:25 ` Juan Quintela 2014-01-16 2:45 ` Stefan Hajnoczi 2014-01-15 12:40 ` Peter Crosthwaite 1 sibling, 2 replies; 16+ messages in thread From: Peter Crosthwaite @ 2014-01-15 8:24 UTC (permalink / raw) To: Stefan Hajnoczi, Edgar Iglesias, Juan Quintela Cc: Beniamino Galvani, Peter Maydell, qemu-devel@nongnu.org Developers, Li Guang 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller 2014-01-15 8:24 ` Peter Crosthwaite @ 2014-01-15 10:25 ` Juan Quintela 2014-01-15 12:15 ` Peter Crosthwaite 2014-01-16 2:45 ` Stefan Hajnoczi 1 sibling, 1 reply; 16+ messages in thread From: Juan Quintela @ 2014-01-15 10:25 UTC (permalink / raw) To: Peter Crosthwaite Cc: Edgar Iglesias, Peter Maydell, qemu-devel@nongnu.org Developers, Beniamino Galvani, Stefan Hajnoczi, Li Guang 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller 2014-01-15 10:25 ` Juan Quintela @ 2014-01-15 12:15 ` Peter Crosthwaite 0 siblings, 0 replies; 16+ messages in thread From: Peter Crosthwaite @ 2014-01-15 12:15 UTC (permalink / raw) To: Juan Quintela Cc: Edgar Iglesias, Peter Maydell, qemu-devel@nongnu.org Developers, Beniamino Galvani, Stefan Hajnoczi, Li Guang 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. > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller 2014-01-15 8:24 ` Peter Crosthwaite 2014-01-15 10:25 ` Juan Quintela @ 2014-01-16 2:45 ` Stefan Hajnoczi 2014-01-16 12:52 ` Peter Crosthwaite 1 sibling, 1 reply; 16+ messages in thread From: Stefan Hajnoczi @ 2014-01-16 2:45 UTC (permalink / raw) To: Peter Crosthwaite Cc: Edgar Iglesias, Peter Maydell, Juan Quintela, qemu-devel@nongnu.org Developers, Beniamino Galvani, Li Guang 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller 2014-01-16 2:45 ` Stefan Hajnoczi @ 2014-01-16 12:52 ` Peter Crosthwaite 2014-01-17 3:25 ` Stefan Hajnoczi 0 siblings, 1 reply; 16+ messages in thread From: Peter Crosthwaite @ 2014-01-16 12:52 UTC (permalink / raw) To: Stefan Hajnoczi Cc: Edgar Iglesias, Peter Maydell, Juan Quintela, qemu-devel@nongnu.org Developers, Beniamino Galvani, Li Guang 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller 2014-01-16 12:52 ` Peter Crosthwaite @ 2014-01-17 3:25 ` Stefan Hajnoczi 0 siblings, 0 replies; 16+ messages in thread From: Stefan Hajnoczi @ 2014-01-17 3:25 UTC (permalink / raw) To: Peter Crosthwaite Cc: Edgar Iglesias, Peter Maydell, Juan Quintela, qemu-devel@nongnu.org Developers, Beniamino Galvani, Li Guang 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller 2014-01-14 5:19 ` Stefan Hajnoczi 2014-01-15 8:24 ` Peter Crosthwaite @ 2014-01-15 12:40 ` Peter Crosthwaite 1 sibling, 0 replies; 16+ messages in thread From: Peter Crosthwaite @ 2014-01-15 12:40 UTC (permalink / raw) To: Stefan Hajnoczi Cc: Beniamino Galvani, Peter Maydell, qemu-devel@nongnu.org Developers, Li Guang 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller 2014-01-13 13:15 ` Peter Crosthwaite 2014-01-13 13:16 ` Peter Crosthwaite @ 2014-01-15 21:42 ` Beniamino Galvani 2014-01-15 23:29 ` Peter Crosthwaite 1 sibling, 1 reply; 16+ messages in thread From: Beniamino Galvani @ 2014-01-15 21:42 UTC (permalink / raw) To: Peter Crosthwaite Cc: Peter Maydell, qemu-devel@nongnu.org Developers, Li Guang 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller 2014-01-15 21:42 ` Beniamino Galvani @ 2014-01-15 23:29 ` Peter Crosthwaite 0 siblings, 0 replies; 16+ messages in thread From: Peter Crosthwaite @ 2014-01-15 23:29 UTC (permalink / raw) To: Beniamino Galvani, Stefan Hajnoczi, Juan Quintela Cc: Peter Maydell, qemu-devel@nongnu.org Developers, Li Guang 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 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH v2 2/2] hw/arm/allwinner-a10: initialize EMAC 2014-01-11 10:13 [Qemu-devel] [PATCH v2 0/2] hw/arm: add ethernet support to Allwinner A10 Beniamino Galvani 2014-01-11 10:13 ` [Qemu-devel] [PATCH v2 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller Beniamino Galvani @ 2014-01-11 10:13 ` Beniamino Galvani 1 sibling, 0 replies; 16+ messages in thread From: Beniamino Galvani @ 2014-01-11 10:13 UTC (permalink / raw) To: qemu-devel; +Cc: Beniamino Galvani, Peter Maydell, Peter Crosthwaite, Li Guang Signed-off-by: Beniamino Galvani <b.galvani@gmail.com> --- hw/arm/allwinner-a10.c | 16 ++++++++++++++++ hw/arm/cubieboard.c | 7 +++++++ include/hw/arm/allwinner-a10.h | 3 +++ 3 files changed, 26 insertions(+) diff --git a/hw/arm/allwinner-a10.c b/hw/arm/allwinner-a10.c index 4658e19..416cd49 100644 --- a/hw/arm/allwinner-a10.c +++ b/hw/arm/allwinner-a10.c @@ -31,6 +31,13 @@ static void aw_a10_init(Object *obj) object_initialize(&s->timer, sizeof(s->timer), TYPE_AW_A10_PIT); qdev_set_parent_bus(DEVICE(&s->timer), sysbus_get_default()); + + object_initialize(&s->emac, sizeof(s->emac), TYPE_AW_EMAC); + qdev_set_parent_bus(DEVICE(&s->emac), sysbus_get_default()); + if (nd_table[0].used) { + qemu_check_nic_model(&nd_table[0], "allwinner_emac"); + qdev_set_nic_properties(DEVICE(&s->emac), &nd_table[0]); + } } static void aw_a10_realize(DeviceState *dev, Error **errp) @@ -76,6 +83,15 @@ static void aw_a10_realize(DeviceState *dev, Error **errp) sysbus_connect_irq(sysbusdev, 4, s->irq[67]); sysbus_connect_irq(sysbusdev, 5, s->irq[68]); + object_property_set_bool(OBJECT(&s->emac), true, "realized", &err); + if (err != NULL) { + error_propagate(errp, err); + return; + } + sysbusdev = SYS_BUS_DEVICE(&s->emac); + sysbus_mmio_map(sysbusdev, 0, AW_A10_EMAC_BASE); + sysbus_connect_irq(sysbusdev, 0, s->irq[55]); + serial_mm_init(get_system_memory(), AW_A10_UART0_REG_BASE, 2, s->irq[1], 115200, serial_hds[0], DEVICE_NATIVE_ENDIAN); } diff --git a/hw/arm/cubieboard.c b/hw/arm/cubieboard.c index 3fcb6d2..b3f8f51 100644 --- a/hw/arm/cubieboard.c +++ b/hw/arm/cubieboard.c @@ -36,6 +36,13 @@ static void cubieboard_init(QEMUMachineInitArgs *args) Error *err = NULL; s->a10 = AW_A10(object_new(TYPE_AW_A10)); + + object_property_set_int(OBJECT(&s->a10->emac), 1, "phyaddr", &err); + if (err != NULL) { + error_report("Couldn't set phy address: %s\n", error_get_pretty(err)); + exit(1); + } + object_property_set_bool(OBJECT(s->a10), true, "realized", &err); if (err != NULL) { error_report("Couldn't realize Allwinner A10: %s\n", diff --git a/include/hw/arm/allwinner-a10.h b/include/hw/arm/allwinner-a10.h index da36647..01a189b 100644 --- a/include/hw/arm/allwinner-a10.h +++ b/include/hw/arm/allwinner-a10.h @@ -6,6 +6,7 @@ #include "hw/arm/arm.h" #include "hw/timer/allwinner-a10-pit.h" #include "hw/intc/allwinner-a10-pic.h" +#include "hw/net/allwinner_emac.h" #include "sysemu/sysemu.h" #include "exec/address-spaces.h" @@ -14,6 +15,7 @@ #define AW_A10_PIC_REG_BASE 0x01c20400 #define AW_A10_PIT_REG_BASE 0x01c20c00 #define AW_A10_UART0_REG_BASE 0x01c28000 +#define AW_A10_EMAC_BASE 0x01c0b000 #define AW_A10_SDRAM_BASE 0x40000000 @@ -29,6 +31,7 @@ typedef struct AwA10State { qemu_irq irq[AW_A10_PIC_INT_NR]; AwA10PITState timer; AwA10PICState intc; + AwEmacState emac; } AwA10State; #define ALLWINNER_H_ -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 16+ messages in thread
end of thread, other threads:[~2014-01-17 3:25 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-01-11 10:13 [Qemu-devel] [PATCH v2 0/2] hw/arm: add ethernet support to Allwinner A10 Beniamino Galvani 2014-01-11 10:13 ` [Qemu-devel] [PATCH v2 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller Beniamino Galvani 2014-01-13 5:20 ` Stefan Hajnoczi 2014-01-13 13:15 ` Peter Crosthwaite 2014-01-13 13:16 ` Peter Crosthwaite 2014-01-14 5:19 ` Stefan Hajnoczi 2014-01-15 8:24 ` Peter Crosthwaite 2014-01-15 10:25 ` Juan Quintela 2014-01-15 12:15 ` Peter Crosthwaite 2014-01-16 2:45 ` Stefan Hajnoczi 2014-01-16 12:52 ` Peter Crosthwaite 2014-01-17 3:25 ` Stefan Hajnoczi 2014-01-15 12:40 ` Peter Crosthwaite 2014-01-15 21:42 ` Beniamino Galvani 2014-01-15 23:29 ` Peter Crosthwaite 2014-01-11 10:13 ` [Qemu-devel] [PATCH v2 2/2] hw/arm/allwinner-a10: initialize EMAC Beniamino Galvani
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).