Patchwork [v2,1/4] Add i.MX FEC Ethernet driver

login
register
mail settings
Submitter Jean-Christophe DUBOIS
Date May 4, 2013, 2:09 p.m.
Message ID <6476d358d7f54aa7db6b7dc435d010bc83b2e806.1367676178.git.jcd@tribudubois.net>
Download mbox | patch
Permalink /patch/241464/
State New
Headers show

Comments

Jean-Christophe DUBOIS - May 4, 2013, 2:09 p.m.
This is based on the mcf_fec.c FEC implementation for ColdFire.

    * a generic phy was added (borrowed from lan9118).
    * The buffer management is also modified as buffers are
      slightly different between coldfire and i.MX.

Signed-off-by: Jean-Christophe DUBOIS <jcd@tribudubois.net>
---
 default-configs/arm-softmmu.mak |   1 +
 hw/net/Makefile.objs            |   1 +
 hw/net/imx_fec.c                | 748 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 750 insertions(+)
 create mode 100644 hw/net/imx_fec.c
Peter Crosthwaite - May 5, 2013, 3:11 a.m.
Hi JC,

On Sun, May 5, 2013 at 12:09 AM, Jean-Christophe DUBOIS
<jcd@tribudubois.net> wrote:
> This is based on the mcf_fec.c FEC implementation for ColdFire.
>
>     * a generic phy was added (borrowed from lan9118).
>     * The buffer management is also modified as buffers are
>       slightly different between coldfire and i.MX.
>
> Signed-off-by: Jean-Christophe DUBOIS <jcd@tribudubois.net>
> ---
>  default-configs/arm-softmmu.mak |   1 +
>  hw/net/Makefile.objs            |   1 +
>  hw/net/imx_fec.c                | 748 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 750 insertions(+)
>  create mode 100644 hw/net/imx_fec.c
>
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index 27cbe3d..b3a0207 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -28,6 +28,7 @@ CONFIG_SSI_SD=y
>  CONFIG_SSI_M25P80=y
>  CONFIG_LAN9118=y
>  CONFIG_SMC91C111=y
> +CONFIG_IMX_FEC=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..5c84727 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_IMX_FEC) += imx_fec.o
>
>  common-obj-$(CONFIG_CADENCE) += cadence_gem.o
>  common-obj-$(CONFIG_STELLARIS_ENET) += stellaris_enet.o
> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
> new file mode 100644
> index 0000000..e894d50
> --- /dev/null
> +++ b/hw/net/imx_fec.c
> @@ -0,0 +1,748 @@
> +/*
> + * i.MX Fast Ethernet Controller emulation.
> + *
> + * Copyright (c) 2013 Jean-Christophe Dubois.
> + *
> + * Based on Coldfire Fast Ethernet Controller emulation.
> + *
> + * Copyright (c) 2007 CodeSourcery.
> + *
> + * This code is licensed under the GPL
> + */
> +#include "hw/sysbus.h"
> +#include "net/net.h"
> +#include "hw/devices.h"
> +

is devices.h needed? Its a collection of helper init fns for misc
devices pre QOM style.

> +/* For crc32 */
> +#include <zlib.h>
> +
> +#include "hw/arm/imx.h"
> +
> +#ifndef IMX_FEC_DEBUG
> +#define IMX_FEC_DEBUG          0
> +#endif
> +
> +#ifndef IMX_PHY_DEBUG
> +#define IMX_PHY_DEBUG          0
> +#endif
> +
> +#if IMX_FEC_DEBUG
> +#define DPRINTF(fmt, ...) \
> +do { printf("imx_fec: " fmt , ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...) do {} while (0)
> +#endif
> +
> +#if IMX_PHY_DEBUG
> +#define DPPRINTF(fmt, ...) \
> +do { printf("imx_fec_phy: " fmt , ## __VA_ARGS__); } while (0)
> +#else
> +#define DPPRINTF(fmt, ...) do {} while (0)
> +#endif
> +
> +#define FEC_MAX_FRAME_SIZE 2032
> +
> +typedef struct {
> +    SysBusDevice busdev;

parent_obj

> +    NICState *nic;
> +    NICConf conf;
> +    qemu_irq irq;
> +    MemoryRegion iomem;
> +
> +    uint32_t irq_state;
> +    uint32_t eir;
> +    uint32_t eimr;
> +    uint32_t rx_enabled;
> +    uint32_t rx_descriptor;
> +    uint32_t tx_descriptor;
> +    uint32_t ecr;
> +    uint32_t mmfr;
> +    uint32_t mscr;
> +    uint32_t mibc;
> +    uint32_t rcr;
> +    uint32_t tcr;
> +    uint32_t tfwr;
> +    uint32_t frsr;
> +    uint32_t erdsr;
> +    uint32_t etdsr;
> +    uint32_t emrbr;
> +    uint32_t miigsk_cfgr;
> +    uint32_t miigsk_enr;
> +
> +    uint32_t phy_status;
> +    uint32_t phy_control;
> +    uint32_t phy_advertise;
> +    uint32_t phy_int;
> +    uint32_t phy_int_mask;
> +} imx_fec_state;

types are CamelCase (IMXFECState)

> +
> +static const VMStateDescription vmstate_imx_fec = {
> +    .name = "fec",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(irq_state, imx_fec_state),
> +        VMSTATE_UINT32(eir, imx_fec_state),
> +        VMSTATE_UINT32(eimr, imx_fec_state),
> +        VMSTATE_UINT32(rx_enabled, imx_fec_state),
> +        VMSTATE_UINT32(rx_descriptor, imx_fec_state),
> +        VMSTATE_UINT32(tx_descriptor, imx_fec_state),
> +        VMSTATE_UINT32(ecr, imx_fec_state),
> +        VMSTATE_UINT32(mmfr, imx_fec_state),
> +        VMSTATE_UINT32(mscr, imx_fec_state),
> +        VMSTATE_UINT32(mibc, imx_fec_state),
> +        VMSTATE_UINT32(rcr, imx_fec_state),
> +        VMSTATE_UINT32(tcr, imx_fec_state),
> +        VMSTATE_UINT32(tfwr, imx_fec_state),
> +        VMSTATE_UINT32(frsr, imx_fec_state),
> +        VMSTATE_UINT32(erdsr, imx_fec_state),
> +        VMSTATE_UINT32(etdsr, imx_fec_state),
> +        VMSTATE_UINT32(emrbr, imx_fec_state),
> +        VMSTATE_UINT32(miigsk_cfgr, imx_fec_state),
> +        VMSTATE_UINT32(miigsk_enr, imx_fec_state),
> +
> +        VMSTATE_UINT32(phy_status, imx_fec_state),
> +        VMSTATE_UINT32(phy_control, imx_fec_state),
> +        VMSTATE_UINT32(phy_advertise, imx_fec_state),
> +        VMSTATE_UINT32(phy_int, imx_fec_state),
> +        VMSTATE_UINT32(phy_int_mask, imx_fec_state),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +#define PHY_INT_ENERGYON            (1 << 7)
> +#define PHY_INT_AUTONEG_COMPLETE    (1 << 6)
> +#define PHY_INT_FAULT               (1 << 5)
> +#define PHY_INT_DOWN                (1 << 4)
> +#define PHY_INT_AUTONEG_LP          (1 << 3)
> +#define PHY_INT_PARFAULT            (1 << 2)
> +#define PHY_INT_AUTONEG_PAGE        (1 << 1)
> +
> +static void imx_fec_update(imx_fec_state *s);
> +
> +/*
> + * The MII phy could raise a GPIO to the processor which in turn
> + * could be handled as an interrpt by the OS.

"interrupt". Also a bit of a nitpick but s/OS/guest. Guest is the
common term for the software running on a QEMU emulation.

> + * For now we don't handle any GPIO/interrupt line, so the OS will
> + * have to poll for the PHY status.
> + */
> +static void phy_update_irq(imx_fec_state *s)
> +{
> +    imx_fec_update(s);
> +}
> +
> +static void phy_update_link(imx_fec_state *s)
> +{
> +    /* Autonegotiation status mirrors link status.  */
> +    if (qemu_get_queue(s->nic)->link_down) {
> +        DPPRINTF("%s: link is down\n", __func__);
> +        s->phy_status &= ~0x0024;
> +        s->phy_int |= PHY_INT_DOWN;
> +    } else {
> +        DPPRINTF("%s: link is up\n", __func__);
> +        s->phy_status |= 0x0024;
> +        s->phy_int |= PHY_INT_ENERGYON;
> +        s->phy_int |= PHY_INT_AUTONEG_COMPLETE;
> +    }
> +    phy_update_irq(s);
> +}
> +
> +static void imx_fec_set_link(NetClientState *nc)
> +{
> +    phy_update_link(qemu_get_nic_opaque(nc));
> +}
> +
> +static void phy_reset(imx_fec_state *s)
> +{
> +    DPPRINTF("%s\n", __func__);
> +
> +    s->phy_status = 0x7809;
> +    s->phy_control = 0x3000;
> +    s->phy_advertise = 0x01e1;
> +    s->phy_int_mask = 0;
> +    s->phy_int = 0;
> +    phy_update_link(s);
> +}
> +
> +static uint32_t do_phy_read(imx_fec_state *s, int reg)
> +{
> +    uint32_t val;
> +
> +    switch (reg) {
> +    case 0:     /* Basic Control */
> +        DPPRINTF("PHY read reg %d = %04x\n", reg, s->phy_control);

Can you factor out these DPRINTFs in some way? I think you had
something going for your I2C with the array of register names and a
%s?

> +        return s->phy_control;
> +    case 1:     /* Basic Status */
> +        DPPRINTF("PHY read reg %d = %04x\n", reg, s->phy_status);
> +        return s->phy_status;
> +    case 2:     /* ID1 */
> +        DPPRINTF("PHY read reg %d = %04x\n", reg, 0x0007);
> +        return 0x0007;
> +    case 3:     /* ID2 */
> +        DPPRINTF("PHY read reg %d = %04x\n", reg, 0xc0d1);
> +        return 0xc0d1;
> +    case 4:     /* Auto-neg advertisement */
> +        DPPRINTF("PHY read reg %d = %04x\n", reg, s->phy_advertise);
> +        return s->phy_advertise;
> +    case 5:     /* Auto-neg Link Partner Ability */
> +        DPPRINTF("PHY read reg %d = %04x\n", reg, 0x0f71);
> +        return 0x0f71;
> +    case 6:     /* Auto-neg Expansion */
> +        DPPRINTF("PHY read reg %d = %04x\n", reg, 1);
> +        return 1;
> +        /* TODO 17, 18, 27, 29, 30, 31 */

This un-implemented functionality? If so, create a case (6 way fall
through), and qemu_log_mask(LOG_UNIMP to allow debugging users to know
that QEMU is ignoring something.

> +    case 29:    /* Interrupt source.  */
> +        val = s->phy_int;
> +        s->phy_int = 0;
> +        phy_update_irq(s);
> +        return val;
> +    case 30:    /* Interrupt mask */
> +        return s->phy_int_mask;
> +    default:
> +        DPPRINTF("PHY read reg %d, ignored, returning 0\n", reg);

Is this un-defined register space? if so, qemu_log_mask(LOG_GUEST_ERROR

> +        return 0;
> +    }
> +}
> +
> +static void do_phy_write(imx_fec_state *s, int reg, uint32_t val)
> +{
> +    switch (reg) {
> +    case 0:     /* Basic Control */
> +        if (val & 0x8000) {
> +            phy_reset(s);
> +        } else {
> +            s->phy_control = val & 0x7980;
> +            /* Complete autonegotiation immediately.  */
> +            if (val & 0x1000) {
> +                s->phy_status |= 0x0020;
> +            }
> +        }
> +        break;
> +    case 4:     /* Auto-neg advertisement */
> +        s->phy_advertise = (val & 0x2d7f) | 0x80;
> +        break;
> +        /* TODO 17, 18, 27, 31 */
> +    case 30:    /* Interrupt mask */
> +        s->phy_int_mask = val & 0xff;
> +        phy_update_irq(s);
> +        break;
> +    default:
> +        DPPRINTF("PHY write reg %d = 0x%04x, ignored\n", reg, val);
> +    }
> +}
> +
> +#define FEC_INT_HB      (1 << 31)
> +#define FEC_INT_BABR    (1 << 30)
> +#define FEC_INT_BABT    (1 << 29)
> +#define FEC_INT_GRA     (1 << 28)
> +#define FEC_INT_TXF     (1 << 27)
> +#define FEC_INT_TXB     (1 << 26)
> +#define FEC_INT_RXF     (1 << 25)
> +#define FEC_INT_RXB     (1 << 24)
> +#define FEC_INT_MII     (1 << 23)
> +#define FEC_INT_EBERR   (1 << 22)
> +#define FEC_INT_LC      (1 << 21)
> +#define FEC_INT_RL      (1 << 20)
> +#define FEC_INT_UN      (1 << 19)
> +
> +#define FEC_EN      2
> +#define FEC_RESET   1
> +
> +/* Buffer Descriptor.  */
> +typedef struct {
> +    uint16_t length;
> +    uint16_t flags;
> +    uint32_t data;
> +} imx_fec_bd;
> +
> +#define FEC_BD_R    (1 << 15)
> +#define FEC_BD_E    (1 << 15)
> +#define FEC_BD_O1   (1 << 14)
> +#define FEC_BD_W    (1 << 13)
> +#define FEC_BD_O2   (1 << 12)
> +#define FEC_BD_L    (1 << 11)
> +#define FEC_BD_TC   (1 << 10)
> +#define FEC_BD_ABC  (1 << 9)
> +#define FEC_BD_M    (1 << 8)
> +#define FEC_BD_BC   (1 << 7)
> +#define FEC_BD_MC   (1 << 6)
> +#define FEC_BD_LG   (1 << 5)
> +#define FEC_BD_NO   (1 << 4)
> +#define FEC_BD_CR   (1 << 2)
> +#define FEC_BD_OV   (1 << 1)
> +#define FEC_BD_TR   (1 << 0)
> +
> +static void imx_fec_read_bd(imx_fec_bd *bd, uint32_t addr)
> +{
> +    cpu_physical_memory_read(addr, (uint8_t *) bd, sizeof(*bd));

cpu_physical_memory_read/write is deprecated for new code (at least
for doing DMA from device land). Use dma_memory_read/write. pl330.c
has some examples and has the DMAContext bolierplate for setting this
up.

> +}
> +
> +static void imx_fec_write_bd(imx_fec_bd *bd, uint32_t addr)
> +{
> +    cpu_physical_memory_write(addr, (uint8_t *) bd, sizeof(*bd));
> +}
> +
> +static void imx_fec_update(imx_fec_state *s)
> +{
> +    uint32_t active;
> +    uint32_t changed;
> +
> +    active = s->eir & s->eimr;
> +    changed = active ^ s->irq_state;
> +    qemu_set_irq(s->irq, changed);

This looks strange (although I haven't read your devices specs so I am
speculating). You set the IRQ pin on an edge of eir & eimr. Should
this be

if (changed) {
    qemu_set_irq(s->irq, active)
}

?

> +    s->irq_state = active;
> +}
> +
> +static void imx_fec_do_tx(imx_fec_state *s)
> +{
> +    uint32_t addr;
> +    imx_fec_bd bd;
> +    int frame_size;
> +    int len;
> +    uint8_t frame[FEC_MAX_FRAME_SIZE];
> +    uint8_t *ptr;
> +
> +    DPRINTF("%s:\n", __func__);
> +    ptr = frame;
> +    frame_size = 0;
> +    addr = s->tx_descriptor;
> +    while (1) {

at least len and bd (maybe other local variables) are local to this
while block so could be defined here. Helps to clarify which variables
have cross iteration life, and which ones do not. (not a blocker just
a suggestion)

> +        imx_fec_read_bd(&bd, addr);
> +        DPRINTF("%s: tx_bd %x flags %04x len %d data %08x\n",
> +                __func__, addr, bd.flags, bd.length, bd.data);
> +        if ((bd.flags & FEC_BD_R) == 0) {
> +            /* Run out of descriptors to transmit.  */
> +            break;
> +        }
> +        len = bd.length;
> +        if (frame_size + len > FEC_MAX_FRAME_SIZE) {
> +            len = FEC_MAX_FRAME_SIZE - frame_size;
> +            s->eir |= FEC_INT_BABT;
> +        }
> +        cpu_physical_memory_read(bd.data, ptr, len);
> +        ptr += len;
> +        frame_size += len;
> +        if (bd.flags & FEC_BD_L) {
> +            /* Last buffer in frame.  */
> +            DPRINTF("Sending packet\n");
> +            qemu_send_packet(qemu_get_queue(s->nic), frame, len);
> +            ptr = frame;
> +            frame_size = 0;
> +            s->eir |= FEC_INT_TXF;
> +        }
> +        s->eir |= FEC_INT_TXB;
> +        bd.flags &= ~FEC_BD_R;
> +        /* Write back the modified descriptor.  */
> +        imx_fec_write_bd(&bd, addr);
> +        /* Advance to the next descriptor.  */
> +        if ((bd.flags & FEC_BD_W) != 0) {
> +            addr = s->etdsr;
> +        } else {
> +            addr += 8;
> +        }
> +    }
> +    s->tx_descriptor = addr;
> +    imx_fec_update(s);
> +}
> +
> +static void imx_fec_enable_rx(imx_fec_state *s)
> +{
> +    imx_fec_bd bd;
> +
> +    imx_fec_read_bd(&bd, s->rx_descriptor);
> +    s->rx_enabled = ((bd.flags & FEC_BD_E) != 0);

s->rx_enabled is used to return false from imx_fec_can_recieve. This
means you need to check for queued packets here using
qemu_flush_queued_packets, as 0->1 change of rx_enabled may require a
flush. If your device has implemented packet dropping logic, then you
could instead just always return true from can_recieve, although my
experience is a fully implemented can_recieve path is much better
(although slightly less faithful to the real hardware).

> +    if (!s->rx_enabled) {
> +        DPRINTF("%s: RX buffer full\n", __func__);
> +    }
> +}
> +
> +static void imx_fec_reset(DeviceState *d)
> +{
> +    imx_fec_state *s = FROM_SYSBUS(imx_fec_state, SYS_BUS_DEVICE(d));
> +

QOM cast macro needed.

> +    /* Reset the FEC */
> +    s->eir = 0;
> +    s->eimr = 0;
> +    s->rx_enabled = 0;
> +    s->ecr = 0;
> +    s->mscr = 0;
> +    s->mibc = 0xc0000000;
> +    s->rcr = 0x05ee0001;
> +    s->tcr = 0;
> +    s->tfwr = 0;
> +    s->frsr = 0x500;
> +    s->miigsk_cfgr = 0;
> +    s->miigsk_enr = 0x6;
> +
> +    /* We also reset the PHY */
> +    phy_reset(s);
> +}
> +
> +static uint64_t imx_fec_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    imx_fec_state *s = (imx_fec_state *) opaque;
> +
> +    DPRINTF("%s: addr = 0x%x\n", __func__, (int)addr);
> +
> +    switch (addr & 0x3ff) {
> +    case 0x004:
> +        return s->eir;
> +    case 0x008:
> +        return s->eimr;
> +    case 0x010:
> +        return s->rx_enabled ? (1 << 24) : 0;   /* RDAR */
> +    case 0x014:
> +        return 0;   /* TDAR */
> +    case 0x024:
> +        return s->ecr;
> +    case 0x040:
> +        return s->mmfr;
> +    case 0x044:
> +        return s->mscr;
> +    case 0x064:
> +        return s->mibc; /* MIBC */
> +    case 0x084:
> +        return s->rcr;
> +    case 0x0c4:
> +        return s->tcr;
> +    case 0x0e4:     /* PALR */
> +        return (s->conf.macaddr.a[0] << 24) | (s->conf.macaddr.
> +                                               a[1] << 16)
> +               | (s->conf.macaddr.a[2] << 8) | s->conf.macaddr.a[3];
> +        break;
> +    case 0x0e8:     /* PAUR */
> +        return (s->conf.macaddr.a[4] << 24) | (s->conf.macaddr.
> +                                               a[5] << 16) | 0x8808;
> +    case 0x0ec:
> +        return 0x10000; /* OPD */
> +    case 0x118:
> +        return 0;
> +    case 0x11c:
> +        return 0;
> +    case 0x120:
> +        return 0;
> +    case 0x124:
> +        return 0;
> +    case 0x144:
> +        return s->tfwr;
> +    case 0x14c:
> +        return 0x600;
> +    case 0x150:
> +        return s->frsr;
> +    case 0x180:
> +        return s->erdsr;
> +    case 0x184:
> +        return s->etdsr;
> +    case 0x188:
> +        return s->emrbr;
> +    case 0x300:
> +        return s->miigsk_cfgr;
> +    case 0x308:
> +        return s->miigsk_enr;
> +    default:
> +        hw_error("imx_fec_read: Bad address 0x%x\n", (int)addr);
> +        return 0;
> +    }
> +}
> +
> +static void imx_fec_write(void *opaque, hwaddr addr,
> +                          uint64_t value, unsigned size)
> +{
> +    imx_fec_state *s = (imx_fec_state *) opaque;
> +
> +    DPRINTF("%s: 0x%x @ addr = 0x%x\n", __func__, (int)value, (int)addr);
> +
> +    switch (addr & 0x3ff) {
> +    case 0x004: /* EIR */
> +        s->eir &= ~value;
> +        break;
> +    case 0x008: /* EIMR */
> +        s->eimr = value;
> +        break;
> +    case 0x010: /* RDAR */
> +        if ((s->ecr & FEC_EN) && !s->rx_enabled) {
> +            DPRINTF("RX enable\n");
> +            imx_fec_enable_rx(s);
> +        }
> +        break;
> +    case 0x014: /* TDAR */
> +        if (s->ecr & FEC_EN) {
> +            imx_fec_do_tx(s);
> +        }
> +        break;
> +    case 0x024: /* ECR */
> +        s->ecr = value;
> +        if (value & FEC_RESET) {
> +            DPRINTF("Reset\n");
> +            imx_fec_reset(&s->busdev.qdev);
> +        }
> +        if ((s->ecr & FEC_EN) == 0) {
> +            s->rx_enabled = 0;
> +        }
> +        break;
> +    case 0x040: /* MMFR */
> +        /* store the value */
> +        s->mmfr = value;
> +        if (value & (1 << 28)) {
> +            DPRINTF("PHY write %d = 0x%04x\n",
> +                    ((int)value >> 18) & 0x1f, (int)value & 0xffff);
> +            do_phy_write(s, (value >> 18) & 0x1f, value & 0xffff);
> +        } else {
> +            s->mmfr = do_phy_read(s, (value >> 18) & 0x1f);
> +            DPRINTF("PHY read %d = 0x%04x\n",
> +                    ((int)value >> 18) & 0x1f, s->mmfr & 0xffff);
> +        }
> +        /* raise the interrupt as the PHY operation is done */
> +        s->eir |= FEC_INT_MII;
> +        break;
> +    case 0x044: /* MSCR */
> +        s->mscr = value & 0xfe;
> +        break;
> +    case 0x064: /* MIBC */
> +        /* TODO: Implement MIB.  */
> +        s->mibc = (value & 0x80000000) ? 0xc0000000 : 0;
> +        break;
> +    case 0x084: /* RCR */
> +        s->rcr = value & 0x07ff003f;
> +        /* TODO: Implement LOOP mode.  */
> +        break;
> +    case 0x0c4: /* TCR */
> +        /* We transmit immediately, so raise GRA immediately.  */
> +        s->tcr = value;
> +        if (value & 1) {
> +            s->eir |= FEC_INT_GRA;
> +        }
> +        break;
> +    case 0x0e4: /* PALR */
> +        s->conf.macaddr.a[0] = value >> 24;
> +        s->conf.macaddr.a[1] = value >> 16;
> +        s->conf.macaddr.a[2] = value >> 8;
> +        s->conf.macaddr.a[3] = value;
> +        break;
> +    case 0x0e8: /* PAUR */
> +        s->conf.macaddr.a[4] = value >> 24;
> +        s->conf.macaddr.a[5] = value >> 16;
> +        break;
> +    case 0x0ec: /* OPDR */
> +        break;
> +    case 0x118: /* IAUR */
> +    case 0x11c: /* IALR */
> +    case 0x120: /* GAUR */
> +    case 0x124: /* GALR */
> +        /* TODO: implement MAC hash filtering.  */
> +        break;
> +    case 0x144: /* TFWR */
> +        s->tfwr = value & 3;
> +        break;
> +    case 0x14c: /* FRBR */
> +        /* FRBR writes ignored.  */
> +        break;
> +    case 0x150: /* FRSR */
> +        s->frsr = (value & 0x3fc) | 0x400;
> +        break;
> +    case 0x180: /* ERDSR */
> +        s->erdsr = value & ~3;
> +        s->rx_descriptor = s->erdsr;
> +        break;
> +    case 0x184: /* ETDSR */
> +        s->etdsr = value & ~3;
> +        s->tx_descriptor = s->etdsr;
> +        break;
> +    case 0x188: /* EMRBR */
> +        s->emrbr = value & 0x7f0;
> +        break;
> +    case 0x300: /* MIIGSK_CFGR */
> +        s->miigsk_cfgr = value & 0x53;
> +        break;
> +    case 0x308: /* MIIGSK_ENR */
> +        s->miigsk_enr = (value & 0x2) ? 0x6 : 0;
> +        break;
> +    default:
> +        hw_error("imx_fec_write Bad address 0x%x\n", (int)addr);
> +    }
> +    imx_fec_update(s);
> +}
> +
> +static int imx_fec_can_receive(NetClientState *nc)
> +{
> +    imx_fec_state *s = qemu_get_nic_opaque(nc);
> +

Andreas, do we care about QOM casts coming from random void* opaques?

> +    return s->rx_enabled;
> +}
> +
> +static ssize_t imx_fec_receive(NetClientState *nc, const uint8_t *buf,
> +                               size_t len)
> +{
> +    imx_fec_state *s = qemu_get_nic_opaque(nc);
> +    imx_fec_bd bd;
> +    uint32_t flags = 0;
> +    uint32_t addr;
> +    uint32_t crc;
> +    uint32_t buf_addr;
> +    uint8_t *crc_ptr;
> +    unsigned int buf_len;
> +    size_t size = len;
> +
> +    DPRINTF("%s: len %d\n", __func__, (int)size);
> +
> +    if (!s->rx_enabled) {
> +        DPRINTF("%s: Unexpected packet\n", __func__);
> +        return 0;
> +    }
> +    /* 4 bytes for the CRC.  */
> +    size += 4;
> +    crc = cpu_to_be32(crc32(~0, buf, size));
> +    crc_ptr = (uint8_t *) &crc;
> +    /* Huge frames are truncted.  */
> +    if (size > FEC_MAX_FRAME_SIZE) {
> +        size = FEC_MAX_FRAME_SIZE;
> +        flags |= FEC_BD_TR | FEC_BD_LG;
> +    }
> +    /* Frames larger than the user limit just set error flags.  */
> +    if (size > (s->rcr >> 16)) {
> +        flags |= FEC_BD_LG;
> +    }
> +    addr = s->rx_descriptor;
> +    while (size > 0) {
> +        imx_fec_read_bd(&bd, addr);
> +        if ((bd.flags & FEC_BD_E) == 0) {
> +            /* No descriptors available.  Bail out.  */
> +            /*
> +             * FIXME: This is wrong. We should probably either
> +             * save the remainder for when more RX buffers are
> +             * available, or flag an error.
> +             */

Wouldn't the real hardware just drops the packet? Does the device have
bits and interrupts for signalling this condition? (most NICs do).

> +            DPRINTF("%s: Lost end of frame\n", __func__);
> +            break;
> +        }
> +        buf_len = (size <= s->emrbr) ? size : s->emrbr;
> +        bd.length = buf_len;
> +        size -= buf_len;
> +        DPRINTF("rx_bd %x length %d\n", addr, bd.length);
> +        /* The last 4 bytes are the CRC.  */
> +        if (size < 4) {
> +            buf_len += size - 4;
> +        }
> +        buf_addr = bd.data;
> +        cpu_physical_memory_write(buf_addr, buf, buf_len);
> +        buf += buf_len;
> +        if (size < 4) {
> +            cpu_physical_memory_write(buf_addr + buf_len, crc_ptr,
> +                                      4 - size);
> +            crc_ptr += 4 - size;
> +        }
> +        bd.flags &= ~FEC_BD_E;
> +        if (size == 0) {
> +            /* Last buffer in frame.  */
> +            bd.flags |= flags | FEC_BD_L;
> +            DPRINTF("rx frame flags %04x\n", bd.flags);
> +            s->eir |= FEC_INT_RXF;
> +        } else {
> +            s->eir |= FEC_INT_RXB;
> +        }
> +        imx_fec_write_bd(&bd, addr);
> +        /* Advance to the next descriptor.  */
> +        if ((bd.flags & FEC_BD_W) != 0) {
> +            addr = s->erdsr;
> +        } else {
> +            addr += 8;
> +        }
> +    }
> +    s->rx_descriptor = addr;
> +    imx_fec_enable_rx(s);
> +    imx_fec_update(s);
> +    return len;
> +}
> +
> +static const MemoryRegionOps imx_fec_ops = {
> +    .read = imx_fec_read,
> +    .write = imx_fec_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static void imx_fec_cleanup(NetClientState *nc)
> +{
> +    imx_fec_state *s = qemu_get_nic_opaque(nc);
> +
> +    s->nic = NULL;
> +}
> +
> +static NetClientInfo net_imx_fec_info = {
> +    .type = NET_CLIENT_OPTIONS_KIND_NIC,
> +    .size = sizeof(NICState),
> +    .can_receive = imx_fec_can_receive,
> +    .receive = imx_fec_receive,
> +    .cleanup = imx_fec_cleanup,
> +    .link_status_changed = imx_fec_set_link,
> +};
> +
> +static int imx_fec_init(SysBusDevice *dev)
> +{
> +    imx_fec_state *s = FROM_SYSBUS(imx_fec_state, dev);
> +
> +    memory_region_init_io(&s->iomem, &imx_fec_ops, s, "fec_mmio", 0x400);
> +    sysbus_init_mmio(dev, &s->iomem);
> +    sysbus_init_irq(dev, &s->irq);
> +    qemu_macaddr_default_if_unset(&s->conf.macaddr);
> +
> +    s->conf.peers.ncs[0] = nd_table[0].netdev;
> +
> +    s->nic = qemu_new_nic(&net_imx_fec_info, &s->conf,
> +                          object_get_typename(OBJECT(dev)), dev->qdev.id,
> +                          s);
> +    qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
> +
> +    return 0;
> +}
> +
> +static Property imx_fec_properties[] = {
> +    DEFINE_NIC_PROPERTIES(imx_fec_state, conf),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void imx_fec_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +
> +    k->init = imx_fec_init;
> +    dc->reset = imx_fec_reset;
> +    dc->vmsd = &vmstate_imx_fec;
> +    dc->props = imx_fec_properties;
> +}
> +
> +static const TypeInfo imx_fec_info = {
> +    .name = "fec",
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(imx_fec_state),
> +    .class_init = imx_fec_class_init,
> +};
> +
> +static void imx_fec_register_types(void)
> +{
> +    type_register_static(&imx_fec_info);
> +}
> +
> +void imx_fec_create(int nic, const hwaddr base, qemu_irq irq)

does this compile with Werror? I thought there was a requirement that
all non-static functions must have a fn prototype defined beforehand
which I cant see in this patch.

Regards,
Peter


> +{
> +    NICInfo *nd;
> +    DeviceState *dev;
> +    SysBusDevice *s;
> +
> +    if (nic >= MAX_NICS) {
> +        hw_error("Cannot assign nic %d: QEMU supports only %d ports\n",
> +                 nic, MAX_NICS);
> +    }
> +
> +    nd = &nd_table[nic];
> +
> +    qemu_check_nic_model(nd, "fec");
> +    dev = qdev_create(NULL, "fec");
> +    qdev_set_nic_properties(dev, nd);
> +    qdev_init_nofail(dev);
> +    s = SYS_BUS_DEVICE(dev);
> +    sysbus_mmio_map(s, 0, base);
> +    sysbus_connect_irq(s, 0, irq);
> +};
> +
> +type_init(imx_fec_register_types)
> --
> 1.8.1.2
>
>
Andreas Färber - May 5, 2013, 11:46 a.m.
Am 05.05.2013 05:11, schrieb Peter Crosthwaite:
> On Sun, May 5, 2013 at 12:09 AM, Jean-Christophe DUBOIS
> <jcd@tribudubois.net> wrote:
>> +void imx_fec_create(int nic, const hwaddr base, qemu_irq irq)
> 
> does this compile with Werror? I thought there was a requirement that
> all non-static functions must have a fn prototype defined beforehand
> which I cant see in this patch.

It went into 2/4 accidentally. :)

Andreas
Peter Crosthwaite - May 5, 2013, 11:59 a.m.
Hi Andreas,

On Sun, May 5, 2013 at 9:46 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 05.05.2013 05:11, schrieb Peter Crosthwaite:
>> On Sun, May 5, 2013 at 12:09 AM, Jean-Christophe DUBOIS
>> <jcd@tribudubois.net> wrote:
>>> +void imx_fec_create(int nic, const hwaddr base, qemu_irq irq)
>>
>> does this compile with Werror? I thought there was a requirement that
>> all non-static functions must have a fn prototype defined beforehand
>> which I cant see in this patch.
>
> It went into 2/4 accidentally. :)
>

Thanks,

While your here I have an on topic question, do we want creation
helpers like this? I notice that we progressively moving towards the
state where machine models talk to QOM directly without wrapper init
fns such as this.. Are functions like this compulsory, optional or
deprecated?

Regards,
Peter

> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>
Peter Maydell - May 5, 2013, 12:41 p.m.
On 5 May 2013 12:59, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> While your here I have an on topic question, do we want creation
> helpers like this? I notice that we progressively moving towards the
> state where machine models talk to QOM directly without wrapper init
> fns such as this.. Are functions like this compulsory, optional or
> deprecated?

I don't think there's a consensus. Personally I think that the
ideal would be that the helper functions are unnecessary (they
really indicate areas where our QOM instantiation syntax is
unbearably clunky and in an ideal world we'd improve it somehow).

My personal suggestion would be:
 * if you have to have them they should be static inline in a
   header somewhere (if you stick them in the .c file then there
   is no compiler guard against the temptation to have them directly
   access bits of the internals of the device)
 * err on the side of not having them

In this case I think I'd go for "not necessary".

[Aside: I really need to make some time to look more closely at NIC
init for embedded boards. I have a feeling that recent-ish changes
to move away from deprecated '-net' syntax for PC don't really
work for the embedded boards. (The idea for the PC is that you
create your NIC with "-device whatever,id=thingy", which of course
doesn't work for onboard NICs which you don't need to and in fact
can't create with a command line option.) I'm not sure how much
sense board-level manipulation of nd_table[] still makes, since
it's totally tied to old style -net options.]

-- PMM
Jean-Christophe DUBOIS - May 5, 2013, 1:14 p.m.
On 05/05/2013 05:11 AM, Peter Crosthwaite wrote:
> Hi JC,
>
> On Sun, May 5, 2013 at 12:09 AM, Jean-Christophe DUBOIS
> <jcd@tribudubois.net>  wrote:
>> This is based on the mcf_fec.c FEC implementation for ColdFire.
>>
>>      * a generic phy was added (borrowed from lan9118).
>>      * The buffer management is also modified as buffers are
>>        slightly different between coldfire and i.MX.
>>
>> Signed-off-by: Jean-Christophe DUBOIS<jcd@tribudubois.net>
>> ---
>>   default-configs/arm-softmmu.mak |   1 +
>>   hw/net/Makefile.objs            |   1 +
>>   hw/net/imx_fec.c                | 748 ++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 750 insertions(+)
>>   create mode 100644 hw/net/imx_fec.c
>>
>> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
>> index 27cbe3d..b3a0207 100644
>> --- a/default-configs/arm-softmmu.mak
>> +++ b/default-configs/arm-softmmu.mak
>> @@ -28,6 +28,7 @@ CONFIG_SSI_SD=y
>>   CONFIG_SSI_M25P80=y
>>   CONFIG_LAN9118=y
>>   CONFIG_SMC91C111=y
>> +CONFIG_IMX_FEC=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..5c84727 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_IMX_FEC) += imx_fec.o
>>
>>   common-obj-$(CONFIG_CADENCE) += cadence_gem.o
>>   common-obj-$(CONFIG_STELLARIS_ENET) += stellaris_enet.o
>> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
>> new file mode 100644
>> index 0000000..e894d50
>> --- /dev/null
>> +++ b/hw/net/imx_fec.c
>> @@ -0,0 +1,748 @@
>> +/*
>> + * i.MX Fast Ethernet Controller emulation.
>> + *
>> + * Copyright (c) 2013 Jean-Christophe Dubois.
>> + *
>> + * Based on Coldfire Fast Ethernet Controller emulation.
>> + *
>> + * Copyright (c) 2007 CodeSourcery.
>> + *
>> + * This code is licensed under the GPL
>> + */
>> +#include "hw/sysbus.h"
>> +#include "net/net.h"
>> +#include "hw/devices.h"
>> +
> is devices.h needed? Its a collection of helper init fns for misc
> devices pre QOM style.

No, I'll remove it. It was part of mcf_fec.c
>> +/* For crc32 */
>> +#include <zlib.h>
>> +
>> +#include "hw/arm/imx.h"
>> +
>> +#ifndef IMX_FEC_DEBUG
>> +#define IMX_FEC_DEBUG          0
>> +#endif
>> +
>> +#ifndef IMX_PHY_DEBUG
>> +#define IMX_PHY_DEBUG          0
>> +#endif
>> +
>> +#if IMX_FEC_DEBUG
>> +#define DPRINTF(fmt, ...) \
>> +do { printf("imx_fec: " fmt , ## __VA_ARGS__); } while (0)
>> +#else
>> +#define DPRINTF(fmt, ...) do {} while (0)
>> +#endif
>> +
>> +#if IMX_PHY_DEBUG
>> +#define DPPRINTF(fmt, ...) \
>> +do { printf("imx_fec_phy: " fmt , ## __VA_ARGS__); } while (0)
>> +#else
>> +#define DPPRINTF(fmt, ...) do {} while (0)
>> +#endif
>> +
>> +#define FEC_MAX_FRAME_SIZE 2032
>> +
>> +typedef struct {
>> +    SysBusDevice busdev;
> parent_obj
OK
>> +    NICState *nic;
>> +    NICConf conf;
>> +    qemu_irq irq;
>> +    MemoryRegion iomem;
>> +
>> +    uint32_t irq_state;
>> +    uint32_t eir;
>> +    uint32_t eimr;
>> +    uint32_t rx_enabled;
>> +    uint32_t rx_descriptor;
>> +    uint32_t tx_descriptor;
>> +    uint32_t ecr;
>> +    uint32_t mmfr;
>> +    uint32_t mscr;
>> +    uint32_t mibc;
>> +    uint32_t rcr;
>> +    uint32_t tcr;
>> +    uint32_t tfwr;
>> +    uint32_t frsr;
>> +    uint32_t erdsr;
>> +    uint32_t etdsr;
>> +    uint32_t emrbr;
>> +    uint32_t miigsk_cfgr;
>> +    uint32_t miigsk_enr;
>> +
>> +    uint32_t phy_status;
>> +    uint32_t phy_control;
>> +    uint32_t phy_advertise;
>> +    uint32_t phy_int;
>> +    uint32_t phy_int_mask;
>> +} imx_fec_state;
> types are CamelCase (IMXFECState)
OK
>> +
>> +static const VMStateDescription vmstate_imx_fec = {
>> +    .name = "fec",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT32(irq_state, imx_fec_state),
>> +        VMSTATE_UINT32(eir, imx_fec_state),
>> +        VMSTATE_UINT32(eimr, imx_fec_state),
>> +        VMSTATE_UINT32(rx_enabled, imx_fec_state),
>> +        VMSTATE_UINT32(rx_descriptor, imx_fec_state),
>> +        VMSTATE_UINT32(tx_descriptor, imx_fec_state),
>> +        VMSTATE_UINT32(ecr, imx_fec_state),
>> +        VMSTATE_UINT32(mmfr, imx_fec_state),
>> +        VMSTATE_UINT32(mscr, imx_fec_state),
>> +        VMSTATE_UINT32(mibc, imx_fec_state),
>> +        VMSTATE_UINT32(rcr, imx_fec_state),
>> +        VMSTATE_UINT32(tcr, imx_fec_state),
>> +        VMSTATE_UINT32(tfwr, imx_fec_state),
>> +        VMSTATE_UINT32(frsr, imx_fec_state),
>> +        VMSTATE_UINT32(erdsr, imx_fec_state),
>> +        VMSTATE_UINT32(etdsr, imx_fec_state),
>> +        VMSTATE_UINT32(emrbr, imx_fec_state),
>> +        VMSTATE_UINT32(miigsk_cfgr, imx_fec_state),
>> +        VMSTATE_UINT32(miigsk_enr, imx_fec_state),
>> +
>> +        VMSTATE_UINT32(phy_status, imx_fec_state),
>> +        VMSTATE_UINT32(phy_control, imx_fec_state),
>> +        VMSTATE_UINT32(phy_advertise, imx_fec_state),
>> +        VMSTATE_UINT32(phy_int, imx_fec_state),
>> +        VMSTATE_UINT32(phy_int_mask, imx_fec_state),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>> +#define PHY_INT_ENERGYON            (1 << 7)
>> +#define PHY_INT_AUTONEG_COMPLETE    (1 << 6)
>> +#define PHY_INT_FAULT               (1 << 5)
>> +#define PHY_INT_DOWN                (1 << 4)
>> +#define PHY_INT_AUTONEG_LP          (1 << 3)
>> +#define PHY_INT_PARFAULT            (1 << 2)
>> +#define PHY_INT_AUTONEG_PAGE        (1 << 1)
>> +
>> +static void imx_fec_update(imx_fec_state *s);
>> +
>> +/*
>> + * The MII phy could raise a GPIO to the processor which in turn
>> + * could be handled as an interrpt by the OS.
> "interrupt". Also a bit of a nitpick but s/OS/guest. Guest is the
> common term for the software running on a QEMU emulation.
OK
>> + * For now we don't handle any GPIO/interrupt line, so the OS will
>> + * have to poll for the PHY status.
>> + */
>> +static void phy_update_irq(imx_fec_state *s)
>> +{
>> +    imx_fec_update(s);
>> +}
>> +
>> +static void phy_update_link(imx_fec_state *s)
>> +{
>> +    /* Autonegotiation status mirrors link status.  */
>> +    if (qemu_get_queue(s->nic)->link_down) {
>> +        DPPRINTF("%s: link is down\n", __func__);
>> +        s->phy_status &= ~0x0024;
>> +        s->phy_int |= PHY_INT_DOWN;
>> +    } else {
>> +        DPPRINTF("%s: link is up\n", __func__);
>> +        s->phy_status |= 0x0024;
>> +        s->phy_int |= PHY_INT_ENERGYON;
>> +        s->phy_int |= PHY_INT_AUTONEG_COMPLETE;
>> +    }
>> +    phy_update_irq(s);
>> +}
>> +
>> +static void imx_fec_set_link(NetClientState *nc)
>> +{
>> +    phy_update_link(qemu_get_nic_opaque(nc));
>> +}
>> +
>> +static void phy_reset(imx_fec_state *s)
>> +{
>> +    DPPRINTF("%s\n", __func__);
>> +
>> +    s->phy_status = 0x7809;
>> +    s->phy_control = 0x3000;
>> +    s->phy_advertise = 0x01e1;
>> +    s->phy_int_mask = 0;
>> +    s->phy_int = 0;
>> +    phy_update_link(s);
>> +}
>> +
>> +static uint32_t do_phy_read(imx_fec_state *s, int reg)
>> +{
>> +    uint32_t val;
>> +
>> +    switch (reg) {
>> +    case 0:     /* Basic Control */
>> +        DPPRINTF("PHY read reg %d = %04x\n", reg, s->phy_control);
> Can you factor out these DPRINTFs in some way? I think you had
> something going for your I2C with the array of register names and a
> %s?
This is a copy/paste of the lan9118 code. But I'll change it.
>> +        return s->phy_control;
>> +    case 1:     /* Basic Status */
>> +        DPPRINTF("PHY read reg %d = %04x\n", reg, s->phy_status);
>> +        return s->phy_status;
>> +    case 2:     /* ID1 */
>> +        DPPRINTF("PHY read reg %d = %04x\n", reg, 0x0007);
>> +        return 0x0007;
>> +    case 3:     /* ID2 */
>> +        DPPRINTF("PHY read reg %d = %04x\n", reg, 0xc0d1);
>> +        return 0xc0d1;
>> +    case 4:     /* Auto-neg advertisement */
>> +        DPPRINTF("PHY read reg %d = %04x\n", reg, s->phy_advertise);
>> +        return s->phy_advertise;
>> +    case 5:     /* Auto-neg Link Partner Ability */
>> +        DPPRINTF("PHY read reg %d = %04x\n", reg, 0x0f71);
>> +        return 0x0f71;
>> +    case 6:     /* Auto-neg Expansion */
>> +        DPPRINTF("PHY read reg %d = %04x\n", reg, 1);
>> +        return 1;
>> +        /* TODO 17, 18, 27, 29, 30, 31 */
> This un-implemented functionality? If so, create a case (6 way fall
> through), and qemu_log_mask(LOG_UNIMP to allow debugging users to know
> that QEMU is ignoring something.

Will do
>> +    case 29:    /* Interrupt source.  */
>> +        val = s->phy_int;
>> +        s->phy_int = 0;
>> +        phy_update_irq(s);
>> +        return val;
>> +    case 30:    /* Interrupt mask */
>> +        return s->phy_int_mask;
>> +    default:
>> +        DPPRINTF("PHY read reg %d, ignored, returning 0\n", reg);
> Is this un-defined register space? if so, qemu_log_mask(LOG_GUEST_ERROR

Will do
>> +        return 0;
>> +    }
>> +}
>> +
>> +static void do_phy_write(imx_fec_state *s, int reg, uint32_t val)
>> +{
>> +    switch (reg) {
>> +    case 0:     /* Basic Control */
>> +        if (val & 0x8000) {
>> +            phy_reset(s);
>> +        } else {
>> +            s->phy_control = val & 0x7980;
>> +            /* Complete autonegotiation immediately.  */
>> +            if (val & 0x1000) {
>> +                s->phy_status |= 0x0020;
>> +            }
>> +        }
>> +        break;
>> +    case 4:     /* Auto-neg advertisement */
>> +        s->phy_advertise = (val & 0x2d7f) | 0x80;
>> +        break;
>> +        /* TODO 17, 18, 27, 31 */
>> +    case 30:    /* Interrupt mask */
>> +        s->phy_int_mask = val & 0xff;
>> +        phy_update_irq(s);
>> +        break;
>> +    default:
>> +        DPPRINTF("PHY write reg %d = 0x%04x, ignored\n", reg, val);
>> +    }
>> +}
>> +
>> +#define FEC_INT_HB      (1 << 31)
>> +#define FEC_INT_BABR    (1 << 30)
>> +#define FEC_INT_BABT    (1 << 29)
>> +#define FEC_INT_GRA     (1 << 28)
>> +#define FEC_INT_TXF     (1 << 27)
>> +#define FEC_INT_TXB     (1 << 26)
>> +#define FEC_INT_RXF     (1 << 25)
>> +#define FEC_INT_RXB     (1 << 24)
>> +#define FEC_INT_MII     (1 << 23)
>> +#define FEC_INT_EBERR   (1 << 22)
>> +#define FEC_INT_LC      (1 << 21)
>> +#define FEC_INT_RL      (1 << 20)
>> +#define FEC_INT_UN      (1 << 19)
>> +
>> +#define FEC_EN      2
>> +#define FEC_RESET   1
>> +
>> +/* Buffer Descriptor.  */
>> +typedef struct {
>> +    uint16_t length;
>> +    uint16_t flags;
>> +    uint32_t data;
>> +} imx_fec_bd;
>> +
>> +#define FEC_BD_R    (1 << 15)
>> +#define FEC_BD_E    (1 << 15)
>> +#define FEC_BD_O1   (1 << 14)
>> +#define FEC_BD_W    (1 << 13)
>> +#define FEC_BD_O2   (1 << 12)
>> +#define FEC_BD_L    (1 << 11)
>> +#define FEC_BD_TC   (1 << 10)
>> +#define FEC_BD_ABC  (1 << 9)
>> +#define FEC_BD_M    (1 << 8)
>> +#define FEC_BD_BC   (1 << 7)
>> +#define FEC_BD_MC   (1 << 6)
>> +#define FEC_BD_LG   (1 << 5)
>> +#define FEC_BD_NO   (1 << 4)
>> +#define FEC_BD_CR   (1 << 2)
>> +#define FEC_BD_OV   (1 << 1)
>> +#define FEC_BD_TR   (1 << 0)
>> +
>> +static void imx_fec_read_bd(imx_fec_bd *bd, uint32_t addr)
>> +{
>> +    cpu_physical_memory_read(addr, (uint8_t *) bd, sizeof(*bd));
> cpu_physical_memory_read/write is deprecated for new code (at least
> for doing DMA from device land). Use dma_memory_read/write. pl330.c
> has some examples and has the DMAContext bolierplate for setting this
> up.

OK
>> +}
>> +
>> +static void imx_fec_write_bd(imx_fec_bd *bd, uint32_t addr)
>> +{
>> +    cpu_physical_memory_write(addr, (uint8_t *) bd, sizeof(*bd));
>> +}
>> +
>> +static void imx_fec_update(imx_fec_state *s)
>> +{
>> +    uint32_t active;
>> +    uint32_t changed;
>> +
>> +    active = s->eir & s->eimr;
>> +    changed = active ^ s->irq_state;
>> +    qemu_set_irq(s->irq, changed);
> This looks strange (although I haven't read your devices specs so I am
> speculating). You set the IRQ pin on an edge of eir & eimr. Should
> this be
>
> if (changed) {
>      qemu_set_irq(s->irq, active)
> }
>
> ?

You are right.
>> +    s->irq_state = active;
>> +}
>> +
>> +static void imx_fec_do_tx(imx_fec_state *s)
>> +{
>> +    uint32_t addr;
>> +    imx_fec_bd bd;
>> +    int frame_size;
>> +    int len;
>> +    uint8_t frame[FEC_MAX_FRAME_SIZE];
>> +    uint8_t *ptr;
>> +
>> +    DPRINTF("%s:\n", __func__);
>> +    ptr = frame;
>> +    frame_size = 0;
>> +    addr = s->tx_descriptor;
>> +    while (1) {
> at least len and bd (maybe other local variables) are local to this
> while block so could be defined here. Helps to clarify which variables
> have cross iteration life, and which ones do not. (not a blocker just
> a suggestion)

OK
>> +        imx_fec_read_bd(&bd, addr);
>> +        DPRINTF("%s: tx_bd %x flags %04x len %d data %08x\n",
>> +                __func__, addr, bd.flags, bd.length, bd.data);
>> +        if ((bd.flags & FEC_BD_R) == 0) {
>> +            /* Run out of descriptors to transmit.  */
>> +            break;
>> +        }
>> +        len = bd.length;
>> +        if (frame_size + len > FEC_MAX_FRAME_SIZE) {
>> +            len = FEC_MAX_FRAME_SIZE - frame_size;
>> +            s->eir |= FEC_INT_BABT;
>> +        }
>> +        cpu_physical_memory_read(bd.data, ptr, len);
>> +        ptr += len;
>> +        frame_size += len;
>> +        if (bd.flags & FEC_BD_L) {
>> +            /* Last buffer in frame.  */
>> +            DPRINTF("Sending packet\n");
>> +            qemu_send_packet(qemu_get_queue(s->nic), frame, len);
>> +            ptr = frame;
>> +            frame_size = 0;
>> +            s->eir |= FEC_INT_TXF;
>> +        }
>> +        s->eir |= FEC_INT_TXB;
>> +        bd.flags &= ~FEC_BD_R;
>> +        /* Write back the modified descriptor.  */
>> +        imx_fec_write_bd(&bd, addr);
>> +        /* Advance to the next descriptor.  */
>> +        if ((bd.flags & FEC_BD_W) != 0) {
>> +            addr = s->etdsr;
>> +        } else {
>> +            addr += 8;
>> +        }
>> +    }
>> +    s->tx_descriptor = addr;
>> +    imx_fec_update(s);
>> +}
>> +
>> +static void imx_fec_enable_rx(imx_fec_state *s)
>> +{
>> +    imx_fec_bd bd;
>> +
>> +    imx_fec_read_bd(&bd, s->rx_descriptor);
>> +    s->rx_enabled = ((bd.flags & FEC_BD_E) != 0);
> s->rx_enabled is used to return false from imx_fec_can_recieve. This
> means you need to check for queued packets here using
> qemu_flush_queued_packets, as 0->1 change of rx_enabled may require a
> flush. If your device has implemented packet dropping logic, then you
> could instead just always return true from can_recieve, although my
> experience is a fully implemented can_recieve path is much better
> (although slightly less faithful to the real hardware).

Will do
>
>> +    if (!s->rx_enabled) {
>> +        DPRINTF("%s: RX buffer full\n", __func__);
>> +    }
>> +}
>> +
>> +static void imx_fec_reset(DeviceState *d)
>> +{
>> +    imx_fec_state *s = FROM_SYSBUS(imx_fec_state, SYS_BUS_DEVICE(d));
>> +
> QOM cast macro needed.
OK
>> +    /* Reset the FEC */
>> +    s->eir = 0;
>> +    s->eimr = 0;
>> +    s->rx_enabled = 0;
>> +    s->ecr = 0;
>> +    s->mscr = 0;
>> +    s->mibc = 0xc0000000;
>> +    s->rcr = 0x05ee0001;
>> +    s->tcr = 0;
>> +    s->tfwr = 0;
>> +    s->frsr = 0x500;
>> +    s->miigsk_cfgr = 0;
>> +    s->miigsk_enr = 0x6;
>> +
>> +    /* We also reset the PHY */
>> +    phy_reset(s);
>> +}
>> +
>> +static uint64_t imx_fec_read(void *opaque, hwaddr addr, unsigned size)
>> +{
>> +    imx_fec_state *s = (imx_fec_state *) opaque;
>> +
>> +    DPRINTF("%s: addr = 0x%x\n", __func__, (int)addr);
>> +
>> +    switch (addr & 0x3ff) {
>> +    case 0x004:
>> +        return s->eir;
>> +    case 0x008:
>> +        return s->eimr;
>> +    case 0x010:
>> +        return s->rx_enabled ? (1 << 24) : 0;   /* RDAR */
>> +    case 0x014:
>> +        return 0;   /* TDAR */
>> +    case 0x024:
>> +        return s->ecr;
>> +    case 0x040:
>> +        return s->mmfr;
>> +    case 0x044:
>> +        return s->mscr;
>> +    case 0x064:
>> +        return s->mibc; /* MIBC */
>> +    case 0x084:
>> +        return s->rcr;
>> +    case 0x0c4:
>> +        return s->tcr;
>> +    case 0x0e4:     /* PALR */
>> +        return (s->conf.macaddr.a[0] << 24) | (s->conf.macaddr.
>> +                                               a[1] << 16)
>> +               | (s->conf.macaddr.a[2] << 8) | s->conf.macaddr.a[3];
>> +        break;
>> +    case 0x0e8:     /* PAUR */
>> +        return (s->conf.macaddr.a[4] << 24) | (s->conf.macaddr.
>> +                                               a[5] << 16) | 0x8808;
>> +    case 0x0ec:
>> +        return 0x10000; /* OPD */
>> +    case 0x118:
>> +        return 0;
>> +    case 0x11c:
>> +        return 0;
>> +    case 0x120:
>> +        return 0;
>> +    case 0x124:
>> +        return 0;
>> +    case 0x144:
>> +        return s->tfwr;
>> +    case 0x14c:
>> +        return 0x600;
>> +    case 0x150:
>> +        return s->frsr;
>> +    case 0x180:
>> +        return s->erdsr;
>> +    case 0x184:
>> +        return s->etdsr;
>> +    case 0x188:
>> +        return s->emrbr;
>> +    case 0x300:
>> +        return s->miigsk_cfgr;
>> +    case 0x308:
>> +        return s->miigsk_enr;
>> +    default:
>> +        hw_error("imx_fec_read: Bad address 0x%x\n", (int)addr);
>> +        return 0;
>> +    }
>> +}
>> +
>> +static void imx_fec_write(void *opaque, hwaddr addr,
>> +                          uint64_t value, unsigned size)
>> +{
>> +    imx_fec_state *s = (imx_fec_state *) opaque;
>> +
>> +    DPRINTF("%s: 0x%x @ addr = 0x%x\n", __func__, (int)value, (int)addr);
>> +
>> +    switch (addr & 0x3ff) {
>> +    case 0x004: /* EIR */
>> +        s->eir &= ~value;
>> +        break;
>> +    case 0x008: /* EIMR */
>> +        s->eimr = value;
>> +        break;
>> +    case 0x010: /* RDAR */
>> +        if ((s->ecr & FEC_EN) && !s->rx_enabled) {
>> +            DPRINTF("RX enable\n");
>> +            imx_fec_enable_rx(s);
>> +        }
>> +        break;
>> +    case 0x014: /* TDAR */
>> +        if (s->ecr & FEC_EN) {
>> +            imx_fec_do_tx(s);
>> +        }
>> +        break;
>> +    case 0x024: /* ECR */
>> +        s->ecr = value;
>> +        if (value & FEC_RESET) {
>> +            DPRINTF("Reset\n");
>> +            imx_fec_reset(&s->busdev.qdev);
>> +        }
>> +        if ((s->ecr & FEC_EN) == 0) {
>> +            s->rx_enabled = 0;
>> +        }
>> +        break;
>> +    case 0x040: /* MMFR */
>> +        /* store the value */
>> +        s->mmfr = value;
>> +        if (value & (1 << 28)) {
>> +            DPRINTF("PHY write %d = 0x%04x\n",
>> +                    ((int)value >> 18) & 0x1f, (int)value & 0xffff);
>> +            do_phy_write(s, (value >> 18) & 0x1f, value & 0xffff);
>> +        } else {
>> +            s->mmfr = do_phy_read(s, (value >> 18) & 0x1f);
>> +            DPRINTF("PHY read %d = 0x%04x\n",
>> +                    ((int)value >> 18) & 0x1f, s->mmfr & 0xffff);
>> +        }
>> +        /* raise the interrupt as the PHY operation is done */
>> +        s->eir |= FEC_INT_MII;
>> +        break;
>> +    case 0x044: /* MSCR */
>> +        s->mscr = value & 0xfe;
>> +        break;
>> +    case 0x064: /* MIBC */
>> +        /* TODO: Implement MIB.  */
>> +        s->mibc = (value & 0x80000000) ? 0xc0000000 : 0;
>> +        break;
>> +    case 0x084: /* RCR */
>> +        s->rcr = value & 0x07ff003f;
>> +        /* TODO: Implement LOOP mode.  */
>> +        break;
>> +    case 0x0c4: /* TCR */
>> +        /* We transmit immediately, so raise GRA immediately.  */
>> +        s->tcr = value;
>> +        if (value & 1) {
>> +            s->eir |= FEC_INT_GRA;
>> +        }
>> +        break;
>> +    case 0x0e4: /* PALR */
>> +        s->conf.macaddr.a[0] = value >> 24;
>> +        s->conf.macaddr.a[1] = value >> 16;
>> +        s->conf.macaddr.a[2] = value >> 8;
>> +        s->conf.macaddr.a[3] = value;
>> +        break;
>> +    case 0x0e8: /* PAUR */
>> +        s->conf.macaddr.a[4] = value >> 24;
>> +        s->conf.macaddr.a[5] = value >> 16;
>> +        break;
>> +    case 0x0ec: /* OPDR */
>> +        break;
>> +    case 0x118: /* IAUR */
>> +    case 0x11c: /* IALR */
>> +    case 0x120: /* GAUR */
>> +    case 0x124: /* GALR */
>> +        /* TODO: implement MAC hash filtering.  */
>> +        break;
>> +    case 0x144: /* TFWR */
>> +        s->tfwr = value & 3;
>> +        break;
>> +    case 0x14c: /* FRBR */
>> +        /* FRBR writes ignored.  */
>> +        break;
>> +    case 0x150: /* FRSR */
>> +        s->frsr = (value & 0x3fc) | 0x400;
>> +        break;
>> +    case 0x180: /* ERDSR */
>> +        s->erdsr = value & ~3;
>> +        s->rx_descriptor = s->erdsr;
>> +        break;
>> +    case 0x184: /* ETDSR */
>> +        s->etdsr = value & ~3;
>> +        s->tx_descriptor = s->etdsr;
>> +        break;
>> +    case 0x188: /* EMRBR */
>> +        s->emrbr = value & 0x7f0;
>> +        break;
>> +    case 0x300: /* MIIGSK_CFGR */
>> +        s->miigsk_cfgr = value & 0x53;
>> +        break;
>> +    case 0x308: /* MIIGSK_ENR */
>> +        s->miigsk_enr = (value & 0x2) ? 0x6 : 0;
>> +        break;
>> +    default:
>> +        hw_error("imx_fec_write Bad address 0x%x\n", (int)addr);
>> +    }
>> +    imx_fec_update(s);
>> +}
>> +
>> +static int imx_fec_can_receive(NetClientState *nc)
>> +{
>> +    imx_fec_state *s = qemu_get_nic_opaque(nc);
>> +
> Andreas, do we care about QOM casts coming from random void* opaques?

OK, I am going to do a QOM cast anyway.
>> +    return s->rx_enabled;
>> +}
>> +
>> +static ssize_t imx_fec_receive(NetClientState *nc, const uint8_t *buf,
>> +                               size_t len)
>> +{
>> +    imx_fec_state *s = qemu_get_nic_opaque(nc);
>> +    imx_fec_bd bd;
>> +    uint32_t flags = 0;
>> +    uint32_t addr;
>> +    uint32_t crc;
>> +    uint32_t buf_addr;
>> +    uint8_t *crc_ptr;
>> +    unsigned int buf_len;
>> +    size_t size = len;
>> +
>> +    DPRINTF("%s: len %d\n", __func__, (int)size);
>> +
>> +    if (!s->rx_enabled) {
>> +        DPRINTF("%s: Unexpected packet\n", __func__);
>> +        return 0;
>> +    }
>> +    /* 4 bytes for the CRC.  */
>> +    size += 4;
>> +    crc = cpu_to_be32(crc32(~0, buf, size));
>> +    crc_ptr = (uint8_t *) &crc;
>> +    /* Huge frames are truncted.  */
>> +    if (size > FEC_MAX_FRAME_SIZE) {
>> +        size = FEC_MAX_FRAME_SIZE;
>> +        flags |= FEC_BD_TR | FEC_BD_LG;
>> +    }
>> +    /* Frames larger than the user limit just set error flags.  */
>> +    if (size > (s->rcr >> 16)) {
>> +        flags |= FEC_BD_LG;
>> +    }
>> +    addr = s->rx_descriptor;
>> +    while (size > 0) {
>> +        imx_fec_read_bd(&bd, addr);
>> +        if ((bd.flags & FEC_BD_E) == 0) {
>> +            /* No descriptors available.  Bail out.  */
>> +            /*
>> +             * FIXME: This is wrong. We should probably either
>> +             * save the remainder for when more RX buffers are
>> +             * available, or flag an error.
>> +             */
> Wouldn't the real hardware just drops the packet? Does the device have
> bits and interrupts for signalling this condition? (most NICs do).

This is the ColdFire FEC code (in mcf_fec.c). I did not try to improve 
it on this side.

>> +            DPRINTF("%s: Lost end of frame\n", __func__);
>> +            break;
>> +        }
>> +        buf_len = (size <= s->emrbr) ? size : s->emrbr;
>> +        bd.length = buf_len;
>> +        size -= buf_len;
>> +        DPRINTF("rx_bd %x length %d\n", addr, bd.length);
>> +        /* The last 4 bytes are the CRC.  */
>> +        if (size < 4) {
>> +            buf_len += size - 4;
>> +        }
>> +        buf_addr = bd.data;
>> +        cpu_physical_memory_write(buf_addr, buf, buf_len);
>> +        buf += buf_len;
>> +        if (size < 4) {
>> +            cpu_physical_memory_write(buf_addr + buf_len, crc_ptr,
>> +                                      4 - size);
>> +            crc_ptr += 4 - size;
>> +        }
>> +        bd.flags &= ~FEC_BD_E;
>> +        if (size == 0) {
>> +            /* Last buffer in frame.  */
>> +            bd.flags |= flags | FEC_BD_L;
>> +            DPRINTF("rx frame flags %04x\n", bd.flags);
>> +            s->eir |= FEC_INT_RXF;
>> +        } else {
>> +            s->eir |= FEC_INT_RXB;
>> +        }
>> +        imx_fec_write_bd(&bd, addr);
>> +        /* Advance to the next descriptor.  */
>> +        if ((bd.flags & FEC_BD_W) != 0) {
>> +            addr = s->erdsr;
>> +        } else {
>> +            addr += 8;
>> +        }
>> +    }
>> +    s->rx_descriptor = addr;
>> +    imx_fec_enable_rx(s);
>> +    imx_fec_update(s);
>> +    return len;
>> +}
>> +
>> +static const MemoryRegionOps imx_fec_ops = {
>> +    .read = imx_fec_read,
>> +    .write = imx_fec_write,
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>> +};
>> +
>> +static void imx_fec_cleanup(NetClientState *nc)
>> +{
>> +    imx_fec_state *s = qemu_get_nic_opaque(nc);
>> +
>> +    s->nic = NULL;
>> +}
>> +
>> +static NetClientInfo net_imx_fec_info = {
>> +    .type = NET_CLIENT_OPTIONS_KIND_NIC,
>> +    .size = sizeof(NICState),
>> +    .can_receive = imx_fec_can_receive,
>> +    .receive = imx_fec_receive,
>> +    .cleanup = imx_fec_cleanup,
>> +    .link_status_changed = imx_fec_set_link,
>> +};
>> +
>> +static int imx_fec_init(SysBusDevice *dev)
>> +{
>> +    imx_fec_state *s = FROM_SYSBUS(imx_fec_state, dev);
>> +
>> +    memory_region_init_io(&s->iomem, &imx_fec_ops, s, "fec_mmio", 0x400);
>> +    sysbus_init_mmio(dev, &s->iomem);
>> +    sysbus_init_irq(dev, &s->irq);
>> +    qemu_macaddr_default_if_unset(&s->conf.macaddr);
>> +
>> +    s->conf.peers.ncs[0] = nd_table[0].netdev;
>> +
>> +    s->nic = qemu_new_nic(&net_imx_fec_info, &s->conf,
>> +                          object_get_typename(OBJECT(dev)), dev->qdev.id,
>> +                          s);
>> +    qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
>> +
>> +    return 0;
>> +}
>> +
>> +static Property imx_fec_properties[] = {
>> +    DEFINE_NIC_PROPERTIES(imx_fec_state, conf),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void imx_fec_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>> +
>> +    k->init = imx_fec_init;
>> +    dc->reset = imx_fec_reset;
>> +    dc->vmsd = &vmstate_imx_fec;
>> +    dc->props = imx_fec_properties;
>> +}
>> +
>> +static const TypeInfo imx_fec_info = {
>> +    .name = "fec",
>> +    .parent = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(imx_fec_state),
>> +    .class_init = imx_fec_class_init,
>> +};
>> +
>> +static void imx_fec_register_types(void)
>> +{
>> +    type_register_static(&imx_fec_info);
>> +}
>> +
>> +void imx_fec_create(int nic, const hwaddr base, qemu_irq irq)
> does this compile with Werror? I thought there was a requirement that
> all non-static functions must have a fn prototype defined beforehand
> which I cant see in this patch.
It is part of imx.h which was (wrongly) added to another patch.
> Regards,
> Peter
>
>
>> +{
>> +    NICInfo *nd;
>> +    DeviceState *dev;
>> +    SysBusDevice *s;
>> +
>> +    if (nic >= MAX_NICS) {
>> +        hw_error("Cannot assign nic %d: QEMU supports only %d ports\n",
>> +                 nic, MAX_NICS);
>> +    }
>> +
>> +    nd = &nd_table[nic];
>> +
>> +    qemu_check_nic_model(nd, "fec");
>> +    dev = qdev_create(NULL, "fec");
>> +    qdev_set_nic_properties(dev, nd);
>> +    qdev_init_nofail(dev);
>> +    s = SYS_BUS_DEVICE(dev);
>> +    sysbus_mmio_map(s, 0, base);
>> +    sysbus_connect_irq(s, 0, irq);
>> +};
>> +
>> +type_init(imx_fec_register_types)
>> --
>> 1.8.1.2
>>
>>
Andreas Färber - May 5, 2013, 1:31 p.m.
Am 05.05.2013 15:14, schrieb Jean-Christophe DUBOIS:
> On 05/05/2013 05:11 AM, Peter Crosthwaite wrote:
>> Hi JC,
>>
>> On Sun, May 5, 2013 at 12:09 AM, Jean-Christophe DUBOIS
>> <jcd@tribudubois.net>  wrote:
>>> This is based on the mcf_fec.c FEC implementation for ColdFire.

Note that ColdFire is one of the least maintained targets in QEMU...

>>> +static int imx_fec_can_receive(NetClientState *nc)
>>> +{
>>> +    imx_fec_state *s = qemu_get_nic_opaque(nc);
>>> +
>> Andreas, do we care about QOM casts coming from random void* opaques?

Generally no. If we ever switch to C++ or some other OO language we'll
have to touch casts anyway.

Peter, please note that I usually don't have time to read through all
patches - noticed this inline question incidentally only.

> OK, I am going to do a QOM cast anyway.

You should check how frequently this function is called - the current
OBJECT_CHECK() implementation does at least one string comparison, so
I'd suggest to avoid it here. That is, if the type passed in as opaque
matches the type assigned here (thinking of interfaces).

>>> +    return s->rx_enabled;
>>> +}
[snip]

Not knowing this piece of hardware, might it make sense to call it
"imx-fec" when there's another FEC for ColdFire? Or are they the same
thing and should be unified?

Regards,
Andreas
Jean-Christophe DUBOIS - May 5, 2013, 2:05 p.m.
On 05/05/2013 03:31 PM, Andreas Färber wrote:
> Am 05.05.2013 15:14, schrieb Jean-Christophe DUBOIS:
>> On 05/05/2013 05:11 AM, Peter Crosthwaite wrote:
>>> Hi JC,
>>>
>>> On Sun, May 5, 2013 at 12:09 AM, Jean-Christophe DUBOIS
>>> <jcd@tribudubois.net>  wrote:
>>>> This is based on the mcf_fec.c FEC implementation for ColdFire.
> Note that ColdFire is one of the least maintained targets in QEMU...

Well, that's too bad.

I actually believe the ColdFire FEC implementation would not work in 
actual QEMU (but I don't plan to build a ColdFire Kernel and RootFS to 
find out).

>
>>>> +static int imx_fec_can_receive(NetClientState *nc)
>>>> +{
>>>> +    imx_fec_state *s = qemu_get_nic_opaque(nc);
>>>> +
>>> Andreas, do we care about QOM casts coming from random void* opaques?
> Generally no. If we ever switch to C++ or some other OO language we'll
> have to touch casts anyway.
>
> Peter, please note that I usually don't have time to read through all
> patches - noticed this inline question incidentally only.
>
>> OK, I am going to do a QOM cast anyway.
> You should check how frequently this function is called - the current
> OBJECT_CHECK() implementation does at least one string comparison, so
> I'd suggest to avoid it here. That is, if the type passed in as opaque
> matches the type assigned here (thinking of interfaces).

Hum, ... you are going to be unhappy with my new patch.

>
>>>> +    return s->rx_enabled;
>>>> +}
> [snip]
>
> Not knowing this piece of hardware, might it make sense to call it
> "imx-fec" when there's another FEC for ColdFire?

I am calling it "imx.fec" in the new version of the file (to be 
equivalent to "imx.i2c" I did for I2C).

>   Or are they the same
> thing and should be unified?
Well, they are certainly very similar but are different on a few points. 
In particular the "buffer descriptors" are reversed between i.MX and 
Coldfire. Linux does a compile time decision about these buffer 
descriptors but I don't know how this would work with Qemu (can you make 
a different compile time decision for 2 different targets?).

We could also implement a more cumbersome run time decision but I wanted 
to keep simple.

And anyway I don't have Coldfire (cross compilers, rootfs, ...) setup 
around to test.

>
> Regards,
> Andreas
>
Michael S. Tsirkin - May 5, 2013, 5:49 p.m.
On Sat, May 04, 2013 at 04:09:11PM +0200, Jean-Christophe DUBOIS wrote:
> This is based on the mcf_fec.c FEC implementation for ColdFire.
> 
>     * a generic phy was added (borrowed from lan9118).
>     * The buffer management is also modified as buffers are
>       slightly different between coldfire and i.MX.
> 
> Signed-off-by: Jean-Christophe DUBOIS <jcd@tribudubois.net>
> ---
>  default-configs/arm-softmmu.mak |   1 +
>  hw/net/Makefile.objs            |   1 +
>  hw/net/imx_fec.c                | 748 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 750 insertions(+)
>  create mode 100644 hw/net/imx_fec.c
> 
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index 27cbe3d..b3a0207 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -28,6 +28,7 @@ CONFIG_SSI_SD=y
>  CONFIG_SSI_M25P80=y
>  CONFIG_LAN9118=y
>  CONFIG_SMC91C111=y
> +CONFIG_IMX_FEC=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..5c84727 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_IMX_FEC) += imx_fec.o
>  
>  common-obj-$(CONFIG_CADENCE) += cadence_gem.o
>  common-obj-$(CONFIG_STELLARIS_ENET) += stellaris_enet.o
> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
> new file mode 100644
> index 0000000..e894d50
> --- /dev/null
> +++ b/hw/net/imx_fec.c
> @@ -0,0 +1,748 @@
> +/*
> + * i.MX Fast Ethernet Controller emulation.
> + *
> + * Copyright (c) 2013 Jean-Christophe Dubois.
> + *
> + * Based on Coldfire Fast Ethernet Controller emulation.
> + *
> + * Copyright (c) 2007 CodeSourcery.
> + *
> + * This code is licensed under the GPL

Let's do as we did in other places like this,
and say 'Contributions after XYZ are licensed under the terms of the
GNU GPL, version 2 or (at your option) any later version.'


> + */
> +#include "hw/sysbus.h"
> +#include "net/net.h"
> +#include "hw/devices.h"
> +
> +/* For crc32 */
> +#include <zlib.h>
> +
> +#include "hw/arm/imx.h"
> +
> +#ifndef IMX_FEC_DEBUG
> +#define IMX_FEC_DEBUG          0
> +#endif
> +
> +#ifndef IMX_PHY_DEBUG
> +#define IMX_PHY_DEBUG          0
> +#endif
> +
> +#if IMX_FEC_DEBUG
> +#define DPRINTF(fmt, ...) \
> +do { printf("imx_fec: " fmt , ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...) do {} while (0)
> +#endif
> +
> +#if IMX_PHY_DEBUG
> +#define DPPRINTF(fmt, ...) \
> +do { printf("imx_fec_phy: " fmt , ## __VA_ARGS__); } while (0)
> +#else
> +#define DPPRINTF(fmt, ...) do {} while (0)
> +#endif
> +
> +#define FEC_MAX_FRAME_SIZE 2032
> +
> +typedef struct {
> +    SysBusDevice busdev;
> +    NICState *nic;
> +    NICConf conf;
> +    qemu_irq irq;
> +    MemoryRegion iomem;
> +
> +    uint32_t irq_state;
> +    uint32_t eir;
> +    uint32_t eimr;
> +    uint32_t rx_enabled;
> +    uint32_t rx_descriptor;
> +    uint32_t tx_descriptor;
> +    uint32_t ecr;
> +    uint32_t mmfr;
> +    uint32_t mscr;
> +    uint32_t mibc;
> +    uint32_t rcr;
> +    uint32_t tcr;
> +    uint32_t tfwr;
> +    uint32_t frsr;
> +    uint32_t erdsr;
> +    uint32_t etdsr;
> +    uint32_t emrbr;
> +    uint32_t miigsk_cfgr;
> +    uint32_t miigsk_enr;
> +
> +    uint32_t phy_status;
> +    uint32_t phy_control;
> +    uint32_t phy_advertise;
> +    uint32_t phy_int;
> +    uint32_t phy_int_mask;
> +} imx_fec_state;
> +
> +static const VMStateDescription vmstate_imx_fec = {
> +    .name = "fec",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(irq_state, imx_fec_state),
> +        VMSTATE_UINT32(eir, imx_fec_state),
> +        VMSTATE_UINT32(eimr, imx_fec_state),
> +        VMSTATE_UINT32(rx_enabled, imx_fec_state),
> +        VMSTATE_UINT32(rx_descriptor, imx_fec_state),
> +        VMSTATE_UINT32(tx_descriptor, imx_fec_state),
> +        VMSTATE_UINT32(ecr, imx_fec_state),
> +        VMSTATE_UINT32(mmfr, imx_fec_state),
> +        VMSTATE_UINT32(mscr, imx_fec_state),
> +        VMSTATE_UINT32(mibc, imx_fec_state),
> +        VMSTATE_UINT32(rcr, imx_fec_state),
> +        VMSTATE_UINT32(tcr, imx_fec_state),
> +        VMSTATE_UINT32(tfwr, imx_fec_state),
> +        VMSTATE_UINT32(frsr, imx_fec_state),
> +        VMSTATE_UINT32(erdsr, imx_fec_state),
> +        VMSTATE_UINT32(etdsr, imx_fec_state),
> +        VMSTATE_UINT32(emrbr, imx_fec_state),
> +        VMSTATE_UINT32(miigsk_cfgr, imx_fec_state),
> +        VMSTATE_UINT32(miigsk_enr, imx_fec_state),
> +
> +        VMSTATE_UINT32(phy_status, imx_fec_state),
> +        VMSTATE_UINT32(phy_control, imx_fec_state),
> +        VMSTATE_UINT32(phy_advertise, imx_fec_state),
> +        VMSTATE_UINT32(phy_int, imx_fec_state),
> +        VMSTATE_UINT32(phy_int_mask, imx_fec_state),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +#define PHY_INT_ENERGYON            (1 << 7)
> +#define PHY_INT_AUTONEG_COMPLETE    (1 << 6)
> +#define PHY_INT_FAULT               (1 << 5)
> +#define PHY_INT_DOWN                (1 << 4)
> +#define PHY_INT_AUTONEG_LP          (1 << 3)
> +#define PHY_INT_PARFAULT            (1 << 2)
> +#define PHY_INT_AUTONEG_PAGE        (1 << 1)
> +
> +static void imx_fec_update(imx_fec_state *s);
> +
> +/*
> + * The MII phy could raise a GPIO to the processor which in turn
> + * could be handled as an interrpt by the OS.
> + * For now we don't handle any GPIO/interrupt line, so the OS will
> + * have to poll for the PHY status.
> + */
> +static void phy_update_irq(imx_fec_state *s)
> +{
> +    imx_fec_update(s);
> +}
> +
> +static void phy_update_link(imx_fec_state *s)
> +{
> +    /* Autonegotiation status mirrors link status.  */
> +    if (qemu_get_queue(s->nic)->link_down) {
> +        DPPRINTF("%s: link is down\n", __func__);
> +        s->phy_status &= ~0x0024;
> +        s->phy_int |= PHY_INT_DOWN;
> +    } else {
> +        DPPRINTF("%s: link is up\n", __func__);
> +        s->phy_status |= 0x0024;
> +        s->phy_int |= PHY_INT_ENERGYON;
> +        s->phy_int |= PHY_INT_AUTONEG_COMPLETE;
> +    }
> +    phy_update_irq(s);
> +}
> +
> +static void imx_fec_set_link(NetClientState *nc)
> +{
> +    phy_update_link(qemu_get_nic_opaque(nc));
> +}
> +
> +static void phy_reset(imx_fec_state *s)
> +{
> +    DPPRINTF("%s\n", __func__);
> +
> +    s->phy_status = 0x7809;
> +    s->phy_control = 0x3000;
> +    s->phy_advertise = 0x01e1;
> +    s->phy_int_mask = 0;
> +    s->phy_int = 0;
> +    phy_update_link(s);
> +}
> +
> +static uint32_t do_phy_read(imx_fec_state *s, int reg)
> +{
> +    uint32_t val;
> +
> +    switch (reg) {
> +    case 0:     /* Basic Control */
> +        DPPRINTF("PHY read reg %d = %04x\n", reg, s->phy_control);
> +        return s->phy_control;
> +    case 1:     /* Basic Status */
> +        DPPRINTF("PHY read reg %d = %04x\n", reg, s->phy_status);
> +        return s->phy_status;
> +    case 2:     /* ID1 */
> +        DPPRINTF("PHY read reg %d = %04x\n", reg, 0x0007);
> +        return 0x0007;
> +    case 3:     /* ID2 */
> +        DPPRINTF("PHY read reg %d = %04x\n", reg, 0xc0d1);
> +        return 0xc0d1;
> +    case 4:     /* Auto-neg advertisement */
> +        DPPRINTF("PHY read reg %d = %04x\n", reg, s->phy_advertise);
> +        return s->phy_advertise;
> +    case 5:     /* Auto-neg Link Partner Ability */
> +        DPPRINTF("PHY read reg %d = %04x\n", reg, 0x0f71);
> +        return 0x0f71;
> +    case 6:     /* Auto-neg Expansion */
> +        DPPRINTF("PHY read reg %d = %04x\n", reg, 1);
> +        return 1;
> +        /* TODO 17, 18, 27, 29, 30, 31 */
> +    case 29:    /* Interrupt source.  */
> +        val = s->phy_int;
> +        s->phy_int = 0;
> +        phy_update_irq(s);
> +        return val;
> +    case 30:    /* Interrupt mask */
> +        return s->phy_int_mask;
> +    default:
> +        DPPRINTF("PHY read reg %d, ignored, returning 0\n", reg);
> +        return 0;
> +    }
> +}
> +
> +static void do_phy_write(imx_fec_state *s, int reg, uint32_t val)
> +{
> +    switch (reg) {
> +    case 0:     /* Basic Control */
> +        if (val & 0x8000) {
> +            phy_reset(s);
> +        } else {
> +            s->phy_control = val & 0x7980;
> +            /* Complete autonegotiation immediately.  */
> +            if (val & 0x1000) {
> +                s->phy_status |= 0x0020;
> +            }
> +        }
> +        break;
> +    case 4:     /* Auto-neg advertisement */
> +        s->phy_advertise = (val & 0x2d7f) | 0x80;
> +        break;
> +        /* TODO 17, 18, 27, 31 */
> +    case 30:    /* Interrupt mask */
> +        s->phy_int_mask = val & 0xff;
> +        phy_update_irq(s);
> +        break;
> +    default:
> +        DPPRINTF("PHY write reg %d = 0x%04x, ignored\n", reg, val);
> +    }
> +}
> +
> +#define FEC_INT_HB      (1 << 31)
> +#define FEC_INT_BABR    (1 << 30)
> +#define FEC_INT_BABT    (1 << 29)
> +#define FEC_INT_GRA     (1 << 28)
> +#define FEC_INT_TXF     (1 << 27)
> +#define FEC_INT_TXB     (1 << 26)
> +#define FEC_INT_RXF     (1 << 25)
> +#define FEC_INT_RXB     (1 << 24)
> +#define FEC_INT_MII     (1 << 23)
> +#define FEC_INT_EBERR   (1 << 22)
> +#define FEC_INT_LC      (1 << 21)
> +#define FEC_INT_RL      (1 << 20)
> +#define FEC_INT_UN      (1 << 19)
> +
> +#define FEC_EN      2
> +#define FEC_RESET   1
> +
> +/* Buffer Descriptor.  */
> +typedef struct {
> +    uint16_t length;
> +    uint16_t flags;
> +    uint32_t data;
> +} imx_fec_bd;
> +
> +#define FEC_BD_R    (1 << 15)
> +#define FEC_BD_E    (1 << 15)
> +#define FEC_BD_O1   (1 << 14)
> +#define FEC_BD_W    (1 << 13)
> +#define FEC_BD_O2   (1 << 12)
> +#define FEC_BD_L    (1 << 11)
> +#define FEC_BD_TC   (1 << 10)
> +#define FEC_BD_ABC  (1 << 9)
> +#define FEC_BD_M    (1 << 8)
> +#define FEC_BD_BC   (1 << 7)
> +#define FEC_BD_MC   (1 << 6)
> +#define FEC_BD_LG   (1 << 5)
> +#define FEC_BD_NO   (1 << 4)
> +#define FEC_BD_CR   (1 << 2)
> +#define FEC_BD_OV   (1 << 1)
> +#define FEC_BD_TR   (1 << 0)
> +
> +static void imx_fec_read_bd(imx_fec_bd *bd, uint32_t addr)
> +{
> +    cpu_physical_memory_read(addr, (uint8_t *) bd, sizeof(*bd));
> +}
> +
> +static void imx_fec_write_bd(imx_fec_bd *bd, uint32_t addr)
> +{
> +    cpu_physical_memory_write(addr, (uint8_t *) bd, sizeof(*bd));
> +}
> +
> +static void imx_fec_update(imx_fec_state *s)
> +{
> +    uint32_t active;
> +    uint32_t changed;
> +
> +    active = s->eir & s->eimr;
> +    changed = active ^ s->irq_state;
> +    qemu_set_irq(s->irq, changed);
> +    s->irq_state = active;
> +}
> +
> +static void imx_fec_do_tx(imx_fec_state *s)
> +{
> +    uint32_t addr;
> +    imx_fec_bd bd;
> +    int frame_size;
> +    int len;
> +    uint8_t frame[FEC_MAX_FRAME_SIZE];
> +    uint8_t *ptr;
> +
> +    DPRINTF("%s:\n", __func__);
> +    ptr = frame;
> +    frame_size = 0;
> +    addr = s->tx_descriptor;
> +    while (1) {
> +        imx_fec_read_bd(&bd, addr);
> +        DPRINTF("%s: tx_bd %x flags %04x len %d data %08x\n",
> +                __func__, addr, bd.flags, bd.length, bd.data);
> +        if ((bd.flags & FEC_BD_R) == 0) {
> +            /* Run out of descriptors to transmit.  */
> +            break;
> +        }
> +        len = bd.length;
> +        if (frame_size + len > FEC_MAX_FRAME_SIZE) {
> +            len = FEC_MAX_FRAME_SIZE - frame_size;
> +            s->eir |= FEC_INT_BABT;
> +        }
> +        cpu_physical_memory_read(bd.data, ptr, len);
> +        ptr += len;
> +        frame_size += len;
> +        if (bd.flags & FEC_BD_L) {
> +            /* Last buffer in frame.  */
> +            DPRINTF("Sending packet\n");
> +            qemu_send_packet(qemu_get_queue(s->nic), frame, len);
> +            ptr = frame;
> +            frame_size = 0;
> +            s->eir |= FEC_INT_TXF;
> +        }
> +        s->eir |= FEC_INT_TXB;
> +        bd.flags &= ~FEC_BD_R;
> +        /* Write back the modified descriptor.  */
> +        imx_fec_write_bd(&bd, addr);
> +        /* Advance to the next descriptor.  */
> +        if ((bd.flags & FEC_BD_W) != 0) {
> +            addr = s->etdsr;
> +        } else {
> +            addr += 8;
> +        }
> +    }
> +    s->tx_descriptor = addr;
> +    imx_fec_update(s);
> +}
> +
> +static void imx_fec_enable_rx(imx_fec_state *s)
> +{
> +    imx_fec_bd bd;
> +
> +    imx_fec_read_bd(&bd, s->rx_descriptor);
> +    s->rx_enabled = ((bd.flags & FEC_BD_E) != 0);
> +    if (!s->rx_enabled) {
> +        DPRINTF("%s: RX buffer full\n", __func__);
> +    }
> +}
> +
> +static void imx_fec_reset(DeviceState *d)
> +{
> +    imx_fec_state *s = FROM_SYSBUS(imx_fec_state, SYS_BUS_DEVICE(d));
> +
> +    /* Reset the FEC */
> +    s->eir = 0;
> +    s->eimr = 0;
> +    s->rx_enabled = 0;
> +    s->ecr = 0;
> +    s->mscr = 0;
> +    s->mibc = 0xc0000000;
> +    s->rcr = 0x05ee0001;
> +    s->tcr = 0;
> +    s->tfwr = 0;
> +    s->frsr = 0x500;
> +    s->miigsk_cfgr = 0;
> +    s->miigsk_enr = 0x6;
> +
> +    /* We also reset the PHY */
> +    phy_reset(s);
> +}
> +
> +static uint64_t imx_fec_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    imx_fec_state *s = (imx_fec_state *) opaque;
> +
> +    DPRINTF("%s: addr = 0x%x\n", __func__, (int)addr);
> +
> +    switch (addr & 0x3ff) {
> +    case 0x004:
> +        return s->eir;
> +    case 0x008:
> +        return s->eimr;
> +    case 0x010:
> +        return s->rx_enabled ? (1 << 24) : 0;   /* RDAR */
> +    case 0x014:
> +        return 0;   /* TDAR */
> +    case 0x024:
> +        return s->ecr;
> +    case 0x040:
> +        return s->mmfr;
> +    case 0x044:
> +        return s->mscr;
> +    case 0x064:
> +        return s->mibc; /* MIBC */
> +    case 0x084:
> +        return s->rcr;
> +    case 0x0c4:
> +        return s->tcr;
> +    case 0x0e4:     /* PALR */
> +        return (s->conf.macaddr.a[0] << 24) | (s->conf.macaddr.
> +                                               a[1] << 16)
> +               | (s->conf.macaddr.a[2] << 8) | s->conf.macaddr.a[3];
> +        break;
> +    case 0x0e8:     /* PAUR */
> +        return (s->conf.macaddr.a[4] << 24) | (s->conf.macaddr.
> +                                               a[5] << 16) | 0x8808;
> +    case 0x0ec:
> +        return 0x10000; /* OPD */
> +    case 0x118:
> +        return 0;
> +    case 0x11c:
> +        return 0;
> +    case 0x120:
> +        return 0;
> +    case 0x124:
> +        return 0;
> +    case 0x144:
> +        return s->tfwr;
> +    case 0x14c:
> +        return 0x600;
> +    case 0x150:
> +        return s->frsr;
> +    case 0x180:
> +        return s->erdsr;
> +    case 0x184:
> +        return s->etdsr;
> +    case 0x188:
> +        return s->emrbr;
> +    case 0x300:
> +        return s->miigsk_cfgr;
> +    case 0x308:
> +        return s->miigsk_enr;
> +    default:
> +        hw_error("imx_fec_read: Bad address 0x%x\n", (int)addr);
> +        return 0;
> +    }
> +}
> +
> +static void imx_fec_write(void *opaque, hwaddr addr,
> +                          uint64_t value, unsigned size)
> +{
> +    imx_fec_state *s = (imx_fec_state *) opaque;
> +
> +    DPRINTF("%s: 0x%x @ addr = 0x%x\n", __func__, (int)value, (int)addr);
> +
> +    switch (addr & 0x3ff) {
> +    case 0x004: /* EIR */
> +        s->eir &= ~value;
> +        break;
> +    case 0x008: /* EIMR */
> +        s->eimr = value;
> +        break;
> +    case 0x010: /* RDAR */
> +        if ((s->ecr & FEC_EN) && !s->rx_enabled) {
> +            DPRINTF("RX enable\n");
> +            imx_fec_enable_rx(s);
> +        }
> +        break;
> +    case 0x014: /* TDAR */
> +        if (s->ecr & FEC_EN) {
> +            imx_fec_do_tx(s);
> +        }
> +        break;
> +    case 0x024: /* ECR */
> +        s->ecr = value;
> +        if (value & FEC_RESET) {
> +            DPRINTF("Reset\n");
> +            imx_fec_reset(&s->busdev.qdev);
> +        }
> +        if ((s->ecr & FEC_EN) == 0) {
> +            s->rx_enabled = 0;
> +        }
> +        break;
> +    case 0x040: /* MMFR */
> +        /* store the value */
> +        s->mmfr = value;
> +        if (value & (1 << 28)) {
> +            DPRINTF("PHY write %d = 0x%04x\n",
> +                    ((int)value >> 18) & 0x1f, (int)value & 0xffff);
> +            do_phy_write(s, (value >> 18) & 0x1f, value & 0xffff);
> +        } else {
> +            s->mmfr = do_phy_read(s, (value >> 18) & 0x1f);
> +            DPRINTF("PHY read %d = 0x%04x\n",
> +                    ((int)value >> 18) & 0x1f, s->mmfr & 0xffff);
> +        }
> +        /* raise the interrupt as the PHY operation is done */
> +        s->eir |= FEC_INT_MII;
> +        break;
> +    case 0x044: /* MSCR */
> +        s->mscr = value & 0xfe;
> +        break;
> +    case 0x064: /* MIBC */
> +        /* TODO: Implement MIB.  */
> +        s->mibc = (value & 0x80000000) ? 0xc0000000 : 0;
> +        break;
> +    case 0x084: /* RCR */
> +        s->rcr = value & 0x07ff003f;
> +        /* TODO: Implement LOOP mode.  */
> +        break;
> +    case 0x0c4: /* TCR */
> +        /* We transmit immediately, so raise GRA immediately.  */
> +        s->tcr = value;
> +        if (value & 1) {
> +            s->eir |= FEC_INT_GRA;
> +        }
> +        break;
> +    case 0x0e4: /* PALR */
> +        s->conf.macaddr.a[0] = value >> 24;
> +        s->conf.macaddr.a[1] = value >> 16;
> +        s->conf.macaddr.a[2] = value >> 8;
> +        s->conf.macaddr.a[3] = value;
> +        break;
> +    case 0x0e8: /* PAUR */
> +        s->conf.macaddr.a[4] = value >> 24;
> +        s->conf.macaddr.a[5] = value >> 16;
> +        break;
> +    case 0x0ec: /* OPDR */
> +        break;
> +    case 0x118: /* IAUR */
> +    case 0x11c: /* IALR */
> +    case 0x120: /* GAUR */
> +    case 0x124: /* GALR */
> +        /* TODO: implement MAC hash filtering.  */
> +        break;
> +    case 0x144: /* TFWR */
> +        s->tfwr = value & 3;
> +        break;
> +    case 0x14c: /* FRBR */
> +        /* FRBR writes ignored.  */
> +        break;
> +    case 0x150: /* FRSR */
> +        s->frsr = (value & 0x3fc) | 0x400;
> +        break;
> +    case 0x180: /* ERDSR */
> +        s->erdsr = value & ~3;
> +        s->rx_descriptor = s->erdsr;
> +        break;
> +    case 0x184: /* ETDSR */
> +        s->etdsr = value & ~3;
> +        s->tx_descriptor = s->etdsr;
> +        break;
> +    case 0x188: /* EMRBR */
> +        s->emrbr = value & 0x7f0;
> +        break;
> +    case 0x300: /* MIIGSK_CFGR */
> +        s->miigsk_cfgr = value & 0x53;
> +        break;
> +    case 0x308: /* MIIGSK_ENR */
> +        s->miigsk_enr = (value & 0x2) ? 0x6 : 0;
> +        break;
> +    default:
> +        hw_error("imx_fec_write Bad address 0x%x\n", (int)addr);
> +    }
> +    imx_fec_update(s);
> +}
> +
> +static int imx_fec_can_receive(NetClientState *nc)
> +{
> +    imx_fec_state *s = qemu_get_nic_opaque(nc);
> +
> +    return s->rx_enabled;
> +}
> +
> +static ssize_t imx_fec_receive(NetClientState *nc, const uint8_t *buf,
> +                               size_t len)
> +{
> +    imx_fec_state *s = qemu_get_nic_opaque(nc);
> +    imx_fec_bd bd;
> +    uint32_t flags = 0;
> +    uint32_t addr;
> +    uint32_t crc;
> +    uint32_t buf_addr;
> +    uint8_t *crc_ptr;
> +    unsigned int buf_len;
> +    size_t size = len;
> +
> +    DPRINTF("%s: len %d\n", __func__, (int)size);
> +
> +    if (!s->rx_enabled) {
> +        DPRINTF("%s: Unexpected packet\n", __func__);
> +        return 0;
> +    }
> +    /* 4 bytes for the CRC.  */
> +    size += 4;
> +    crc = cpu_to_be32(crc32(~0, buf, size));
> +    crc_ptr = (uint8_t *) &crc;
> +    /* Huge frames are truncted.  */
> +    if (size > FEC_MAX_FRAME_SIZE) {
> +        size = FEC_MAX_FRAME_SIZE;
> +        flags |= FEC_BD_TR | FEC_BD_LG;
> +    }
> +    /* Frames larger than the user limit just set error flags.  */
> +    if (size > (s->rcr >> 16)) {
> +        flags |= FEC_BD_LG;
> +    }
> +    addr = s->rx_descriptor;
> +    while (size > 0) {
> +        imx_fec_read_bd(&bd, addr);
> +        if ((bd.flags & FEC_BD_E) == 0) {
> +            /* No descriptors available.  Bail out.  */
> +            /*
> +             * FIXME: This is wrong. We should probably either
> +             * save the remainder for when more RX buffers are
> +             * available, or flag an error.
> +             */
> +            DPRINTF("%s: Lost end of frame\n", __func__);
> +            break;
> +        }
> +        buf_len = (size <= s->emrbr) ? size : s->emrbr;
> +        bd.length = buf_len;
> +        size -= buf_len;
> +        DPRINTF("rx_bd %x length %d\n", addr, bd.length);
> +        /* The last 4 bytes are the CRC.  */
> +        if (size < 4) {
> +            buf_len += size - 4;
> +        }
> +        buf_addr = bd.data;
> +        cpu_physical_memory_write(buf_addr, buf, buf_len);
> +        buf += buf_len;
> +        if (size < 4) {
> +            cpu_physical_memory_write(buf_addr + buf_len, crc_ptr,
> +                                      4 - size);
> +            crc_ptr += 4 - size;
> +        }
> +        bd.flags &= ~FEC_BD_E;
> +        if (size == 0) {
> +            /* Last buffer in frame.  */
> +            bd.flags |= flags | FEC_BD_L;
> +            DPRINTF("rx frame flags %04x\n", bd.flags);
> +            s->eir |= FEC_INT_RXF;
> +        } else {
> +            s->eir |= FEC_INT_RXB;
> +        }
> +        imx_fec_write_bd(&bd, addr);
> +        /* Advance to the next descriptor.  */
> +        if ((bd.flags & FEC_BD_W) != 0) {
> +            addr = s->erdsr;
> +        } else {
> +            addr += 8;
> +        }
> +    }
> +    s->rx_descriptor = addr;
> +    imx_fec_enable_rx(s);
> +    imx_fec_update(s);
> +    return len;
> +}
> +
> +static const MemoryRegionOps imx_fec_ops = {
> +    .read = imx_fec_read,
> +    .write = imx_fec_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static void imx_fec_cleanup(NetClientState *nc)
> +{
> +    imx_fec_state *s = qemu_get_nic_opaque(nc);
> +
> +    s->nic = NULL;
> +}
> +
> +static NetClientInfo net_imx_fec_info = {
> +    .type = NET_CLIENT_OPTIONS_KIND_NIC,
> +    .size = sizeof(NICState),
> +    .can_receive = imx_fec_can_receive,
> +    .receive = imx_fec_receive,
> +    .cleanup = imx_fec_cleanup,
> +    .link_status_changed = imx_fec_set_link,
> +};
> +
> +static int imx_fec_init(SysBusDevice *dev)
> +{
> +    imx_fec_state *s = FROM_SYSBUS(imx_fec_state, dev);
> +
> +    memory_region_init_io(&s->iomem, &imx_fec_ops, s, "fec_mmio", 0x400);
> +    sysbus_init_mmio(dev, &s->iomem);
> +    sysbus_init_irq(dev, &s->irq);
> +    qemu_macaddr_default_if_unset(&s->conf.macaddr);
> +
> +    s->conf.peers.ncs[0] = nd_table[0].netdev;
> +
> +    s->nic = qemu_new_nic(&net_imx_fec_info, &s->conf,
> +                          object_get_typename(OBJECT(dev)), dev->qdev.id,
> +                          s);
> +    qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
> +
> +    return 0;
> +}
> +
> +static Property imx_fec_properties[] = {
> +    DEFINE_NIC_PROPERTIES(imx_fec_state, conf),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void imx_fec_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +
> +    k->init = imx_fec_init;
> +    dc->reset = imx_fec_reset;
> +    dc->vmsd = &vmstate_imx_fec;
> +    dc->props = imx_fec_properties;
> +}
> +
> +static const TypeInfo imx_fec_info = {
> +    .name = "fec",
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(imx_fec_state),
> +    .class_init = imx_fec_class_init,
> +};
> +
> +static void imx_fec_register_types(void)
> +{
> +    type_register_static(&imx_fec_info);
> +}
> +
> +void imx_fec_create(int nic, const hwaddr base, qemu_irq irq)
> +{
> +    NICInfo *nd;
> +    DeviceState *dev;
> +    SysBusDevice *s;
> +
> +    if (nic >= MAX_NICS) {
> +        hw_error("Cannot assign nic %d: QEMU supports only %d ports\n",
> +                 nic, MAX_NICS);
> +    }
> +
> +    nd = &nd_table[nic];
> +
> +    qemu_check_nic_model(nd, "fec");
> +    dev = qdev_create(NULL, "fec");
> +    qdev_set_nic_properties(dev, nd);
> +    qdev_init_nofail(dev);

Let's not bother with legacy -net support
for new devices.

Anyone who wants it can create it with the new style
-device flag.


> +    s = SYS_BUS_DEVICE(dev);
> +    sysbus_mmio_map(s, 0, base);
> +    sysbus_connect_irq(s, 0, irq);
> +};
> +
> +type_init(imx_fec_register_types)
> -- 
> 1.8.1.2
>
Peter Maydell - May 5, 2013, 6:01 p.m.
On 5 May 2013 18:49, Michael S. Tsirkin <mst@redhat.com> wrote:
> Let's not bother with legacy -net support
> for new devices.
>
> Anyone who wants it can create it with the new style
> -device flag.

Sorry, you can't say this until we've sorted out the mess
that is new-style networking options in a machine which
creates embedded network controllers.

-- PMM
Michael S. Tsirkin - May 5, 2013, 9:15 p.m.
On Sun, May 05, 2013 at 07:01:34PM +0100, Peter Maydell wrote:
> On 5 May 2013 18:49, Michael S. Tsirkin <mst@redhat.com> wrote:
> > Let's not bother with legacy -net support
> > for new devices.
> >
> > Anyone who wants it can create it with the new style
> > -device flag.
> 
> Sorry, you can't say this until we've sorted out the mess
> that is new-style networking options in a machine which
> creates embedded network controllers.
> 
> -- PMM

What is missing exactly?
Could you please give some examples of the problems
that -netdev + -device has but -net does not have?
Peter Maydell - May 5, 2013, 10 p.m.
On 5 May 2013 22:15, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sun, May 05, 2013 at 07:01:34PM +0100, Peter Maydell wrote:
>> Sorry, you can't say this until we've sorted out the mess
>> that is new-style networking options in a machine which
>> creates embedded network controllers.

> What is missing exactly?
> Could you please give some examples of the problems
> that -netdev + -device has but -net does not have?

-netdev + -device is fine (unsurprisingly since that's the
PC usecase); -netdev + a device that's preinstantiated by the
board is not so fine. And you can't use -device to instantiate
most embedded network controllers because there's no way to
wire up the IRQs and MMIOs.

There's probably a nasty workaround involving '-global', but:
 * that requires the user to know the device name for the
   onboard NIC for the board, which is a regression from
   the -net situation
 * it's not clear how it works if the board has two NICs
   of the same type
 * if we claim -global is the right approach we need to actually
   document it (and document all the board NIC names, yuck)
 * we need to fix existing boards which do the "don't instantiate
   NIC unless the user said -net nic" trick by looking at
   nd_table[]
 * we need to make the board code pass the right NIC properties
   in both the "legacy -net option" and "new style" cases (at the
   moment, for instance, lan911_init() insists on having a
   NICInfo* passed to it)

-net nic works for these use cases because it will operate on
the NICs created by the machine models, because the machine
models look at the nd_table[] when they create the NICs.

thanks
-- PMM
Michael S. Tsirkin - May 6, 2013, 8:51 a.m.
On Sun, May 05, 2013 at 11:00:24PM +0100, Peter Maydell wrote:
> On 5 May 2013 22:15, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Sun, May 05, 2013 at 07:01:34PM +0100, Peter Maydell wrote:
> >> Sorry, you can't say this until we've sorted out the mess
> >> that is new-style networking options in a machine which
> >> creates embedded network controllers.
> 
> > What is missing exactly?
> > Could you please give some examples of the problems
> > that -netdev + -device has but -net does not have?
> 
> -netdev + -device is fine (unsurprisingly since that's the
> PC usecase); -netdev + a device that's preinstantiated by the
> board is not so fine. And you can't use -device to instantiate
> most embedded network controllers because there's no way to
> wire up the IRQs and MMIOs.

Can't board code look for instanciated controllers
and wire them up?

> 
> There's probably a nasty workaround involving '-global', but:
>  * that requires the user to know the device name for the
>    onboard NIC for the board, which is a regression from
>    the -net situation
>  * it's not clear how it works if the board has two NICs
>    of the same type

How does it work now?
I am guessing each -net nic gets mapped to a random device.
At some level that's worse than documenting about internal names,
we are teaching users to learn order of initialization
by trial and error and then rely on this.

>  * if we claim -global is the right approach we need to actually
>    document it (and document all the board NIC names, yuck)
>  * we need to fix existing boards which do the "don't instantiate
>    NIC unless the user said -net nic" trick by looking at
>    nd_table[]
>  * we need to make the board code pass the right NIC properties
>    in both the "legacy -net option" and "new style" cases (at the
>    moment, for instance, lan911_init() insists on having a
>    NICInfo* passed to it)
> 
> -net nic works for these use cases because it will operate on
> the NICs created by the machine models, because the machine
> models look at the nd_table[] when they create the NICs.
> 
> thanks
> -- PMM
Peter Maydell - May 6, 2013, 9:08 a.m.
[cc'd Anthony since this has drifted into a more general topic]

On 6 May 2013 09:51, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sun, May 05, 2013 at 11:00:24PM +0100, Peter Maydell wrote:
>> On 5 May 2013 22:15, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Sun, May 05, 2013 at 07:01:34PM +0100, Peter Maydell wrote:
>> >> Sorry, you can't say this until we've sorted out the mess
>> >> that is new-style networking options in a machine which
>> >> creates embedded network controllers.
>>
>> > What is missing exactly?
>> > Could you please give some examples of the problems
>> > that -netdev + -device has but -net does not have?
>>
>> -netdev + -device is fine (unsurprisingly since that's the
>> PC usecase); -netdev + a device that's preinstantiated by the
>> board is not so fine. And you can't use -device to instantiate
>> most embedded network controllers because there's no way to
>> wire up the IRQs and MMIOs.
>
> Can't board code look for instanciated controllers
> and wire them up?

I don't think this will work, because -device does both
'instance_init' and 'realize', and some of the things the
board needs to set and wire up must be done before 'realize'.

>> There's probably a nasty workaround involving '-global', but:
>>  * that requires the user to know the device name for the
>>    onboard NIC for the board, which is a regression from
>>    the -net situation
>>  * it's not clear how it works if the board has two NICs
>>    of the same type
>
> How does it work now?
> I am guessing each -net nic gets mapped to a random device.
> At some level that's worse than documenting about internal names,
> we are teaching users to learn order of initialization
> by trial and error and then rely on this.

Well, it gets mapped to a specific device (hopefully we pick
the same order as the kernel so first nic is eth0, second
is eth1, and so on). This isn't a question of initialization
order, because you can happily initialize the NIC corresponding
to nd_table[1] before the one for nd_table[0] if you like.
It's just a matter of picking which bit of hardware we call
the "first" ethernet device, in the same way that we pick
one of two serial ports to call the "first" serial port.

thanks
-- PMM
Michael S. Tsirkin - May 6, 2013, 9:24 a.m.
On Mon, May 06, 2013 at 10:08:42AM +0100, Peter Maydell wrote:
> [cc'd Anthony since this has drifted into a more general topic]
> 
> On 6 May 2013 09:51, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Sun, May 05, 2013 at 11:00:24PM +0100, Peter Maydell wrote:
> >> On 5 May 2013 22:15, Michael S. Tsirkin <mst@redhat.com> wrote:
> >> > On Sun, May 05, 2013 at 07:01:34PM +0100, Peter Maydell wrote:
> >> >> Sorry, you can't say this until we've sorted out the mess
> >> >> that is new-style networking options in a machine which
> >> >> creates embedded network controllers.
> >>
> >> > What is missing exactly?
> >> > Could you please give some examples of the problems
> >> > that -netdev + -device has but -net does not have?
> >>
> >> -netdev + -device is fine (unsurprisingly since that's the
> >> PC usecase); -netdev + a device that's preinstantiated by the
> >> board is not so fine. And you can't use -device to instantiate
> >> most embedded network controllers because there's no way to
> >> wire up the IRQs and MMIOs.
> >
> > Can't board code look for instanciated controllers
> > and wire them up?
> 
> I don't think this will work, because -device does both
> 'instance_init' and 'realize', and some of the things the
> board needs to set and wire up must be done before 'realize'.

Well let's add a flag that tells QM to delay realize then?
It's not "abstract" but maybe "embedded" type?

> >> There's probably a nasty workaround involving '-global', but:
> >>  * that requires the user to know the device name for the
> >>    onboard NIC for the board, which is a regression from
> >>    the -net situation
> >>  * it's not clear how it works if the board has two NICs
> >>    of the same type
> >
> > How does it work now?
> > I am guessing each -net nic gets mapped to a random device.
> > At some level that's worse than documenting about internal names,
> > we are teaching users to learn order of initialization
> > by trial and error and then rely on this.
> 
> Well, it gets mapped to a specific device (hopefully we pick
> the same order as the kernel so first nic is eth0, second
> is eth1, and so on). This isn't a question of initialization
> order, because you can happily initialize the NIC corresponding
> to nd_table[1] before the one for nd_table[0] if you like.
> It's just a matter of picking which bit of hardware we call
> the "first" ethernet device, in the same way that we pick
> one of two serial ports to call the "first" serial port.
> 
> thanks
> -- PMM

In other words, it's an undocumented hack :(
Scary as it sounds, for this case I like documenting
internal names better.
Peter Maydell - May 6, 2013, 12:01 p.m.
On 6 May 2013 10:24, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, May 06, 2013 at 10:08:42AM +0100, Peter Maydell wrote:
>> On 6 May 2013 09:51, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Sun, May 05, 2013 at 11:00:24PM +0100, Peter Maydell wrote:
>> >> On 5 May 2013 22:15, Michael S. Tsirkin <mst@redhat.com> wrote:
>> >> > On Sun, May 05, 2013 at 07:01:34PM +0100, Peter Maydell wrote:

>> > Can't board code look for instanciated controllers
>> > and wire them up?
>>
>> I don't think this will work, because -device does both
>> 'instance_init' and 'realize', and some of the things the
>> board needs to set and wire up must be done before 'realize'.
>
> Well let's add a flag that tells QM to delay realize then?
> It's not "abstract" but maybe "embedded" type?

This still requires users to know what their board's NIC
happens to be, and how do you match up the half-finished
thing created with -device to the device that the board
creates later?

>> >> There's probably a nasty workaround involving '-global', but:
>> >>  * that requires the user to know the device name for the
>> >>    onboard NIC for the board, which is a regression from
>> >>    the -net situation
>> >>  * it's not clear how it works if the board has two NICs
>> >>    of the same type
>> >
>> > How does it work now?
>> > I am guessing each -net nic gets mapped to a random device.
>> > At some level that's worse than documenting about internal names,
>> > we are teaching users to learn order of initialization
>> > by trial and error and then rely on this.
>>
>> Well, it gets mapped to a specific device (hopefully we pick
>> the same order as the kernel so first nic is eth0, second
>> is eth1, and so on). This isn't a question of initialization
>> order, because you can happily initialize the NIC corresponding
>> to nd_table[1] before the one for nd_table[0] if you like.
>> It's just a matter of picking which bit of hardware we call
>> the "first" ethernet device, in the same way that we pick
>> one of two serial ports to call the "first" serial port.

> In other words, it's an undocumented hack :(
> Scary as it sounds, for this case I like documenting
> internal names better.

How does that work when both internal NICs are the same kind
of device?

-- PMM
Peter Crosthwaite - May 7, 2013, 12:39 a.m.
Hi Peter, Michael,

On Mon, May 6, 2013 at 10:01 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 6 May 2013 10:24, Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Mon, May 06, 2013 at 10:08:42AM +0100, Peter Maydell wrote:
>>> On 6 May 2013 09:51, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> > On Sun, May 05, 2013 at 11:00:24PM +0100, Peter Maydell wrote:
>>> >> On 5 May 2013 22:15, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> >> > On Sun, May 05, 2013 at 07:01:34PM +0100, Peter Maydell wrote:
>
>>> > Can't board code look for instanciated controllers
>>> > and wire them up?
>>>
>>> I don't think this will work, because -device does both
>>> 'instance_init' and 'realize', and some of the things the
>>> board needs to set and wire up must be done before 'realize'.
>>
>> Well let's add a flag that tells QM to delay realize then?
>> It's not "abstract" but maybe "embedded" type?
>

This seems fundamentally flawed to me. -device should create a new
device to the users specification, whereas this flow will create a new
device to user specification but then let a machine model modify as it
sees fit.

> This still requires users to know what their board's NIC
> happens to be,

Which is ugly detail the user should not have to care about.

> and how do you match up the half-finished
> thing created with -device to the device that the board
> creates later?
>

There may also be cases where machine model want to create a NIC
regardless of whether its used or not. Relevant for sysbus NICs as we
don't have the luxury of a PCI probe process so a generic guest (e.g.
a kernel and its pre-canned dtb) may assume a NIC exists and crash if
the sysbus device is not there. I'm half tempted to pull out the
nb_nics conditionals on Zynqs NIC creation for this very reason.
Bottom line is we shouldn't have to rely on a -device or -net arg at
all to get a NIC.

>>> >> There's probably a nasty workaround involving '-global', but:
>>> >>  * that requires the user to know the device name for the
>>> >>    onboard NIC for the board, which is a regression from
>>> >>    the -net situation
>>> >>  * it's not clear how it works if the board has two NICs
>>> >>    of the same type
>>> >
>>> > How does it work now?
>>> > I am guessing each -net nic gets mapped to a random device.
>>> > At some level that's worse than documenting about internal names,
>>> > we are teaching users to learn order of initialization
>>> > by trial and error and then rely on this.
>>>
>>> Well, it gets mapped to a specific device (hopefully we pick
>>> the same order as the kernel so first nic is eth0, second
>>> is eth1, and so on). This isn't a question of initialization
>>> order, because you can happily initialize the NIC corresponding
>>> to nd_table[1] before the one for nd_table[0] if you like.
>>> It's just a matter of picking which bit of hardware we call
>>> the "first" ethernet device, in the same way that we pick
>>> one of two serial ports to call the "first" serial port.
>
>> In other words, it's an undocumented hack :(
>> Scary as it sounds, for this case I like documenting
>> internal names better.

+1 and give machine-model created NICs a reasonable naming scheme.
Could we also expose the names to the monitor somehow so they can be
looked up easily?

>
> How does that work when both internal NICs are the same kind
> of device?
>

Sanitize the naming scheme:

candence_gem.0 and cadence_gem.1 or something for Zynqs two NICs.

Regards,
Peter

> -- PMM
>
Peter Maydell - May 7, 2013, 9:03 a.m.
On 7 May 2013 01:39, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> There may also be cases where machine model want to create a NIC
> regardless of whether its used or not. Relevant for sysbus NICs as we
> don't have the luxury of a PCI probe process so a generic guest (e.g.
> a kernel and its pre-canned dtb) may assume a NIC exists and crash if
> the sysbus device is not there. I'm half tempted to pull out the
> nb_nics conditionals on Zynqs NIC creation for this very reason.
> Bottom line is we shouldn't have to rely on a -device or -net arg at
> all to get a NIC.

I agree here -- we should just always create all the
hardware the embedded board has. I think the nb_nics conditional
stuff is legacy (not all board models do it).

>>> In other words, it's an undocumented hack :(
>>> Scary as it sounds, for this case I like documenting
>>> internal names better.
>
> +1 and give machine-model created NICs a reasonable naming scheme.
> Could we also expose the names to the monitor somehow so they can be
> looked up easily?

This is basically asking for -global to work on instance
names rather than class names, I think. Sounds like a
reasonable idea.

-- PMM

Patch

diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index 27cbe3d..b3a0207 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -28,6 +28,7 @@  CONFIG_SSI_SD=y
 CONFIG_SSI_M25P80=y
 CONFIG_LAN9118=y
 CONFIG_SMC91C111=y
+CONFIG_IMX_FEC=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..5c84727 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_IMX_FEC) += imx_fec.o
 
 common-obj-$(CONFIG_CADENCE) += cadence_gem.o
 common-obj-$(CONFIG_STELLARIS_ENET) += stellaris_enet.o
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c
new file mode 100644
index 0000000..e894d50
--- /dev/null
+++ b/hw/net/imx_fec.c
@@ -0,0 +1,748 @@ 
+/*
+ * i.MX Fast Ethernet Controller emulation.
+ *
+ * Copyright (c) 2013 Jean-Christophe Dubois.
+ *
+ * Based on Coldfire Fast Ethernet Controller emulation.
+ *
+ * Copyright (c) 2007 CodeSourcery.
+ *
+ * This code is licensed under the GPL
+ */
+#include "hw/sysbus.h"
+#include "net/net.h"
+#include "hw/devices.h"
+
+/* For crc32 */
+#include <zlib.h>
+
+#include "hw/arm/imx.h"
+
+#ifndef IMX_FEC_DEBUG
+#define IMX_FEC_DEBUG          0
+#endif
+
+#ifndef IMX_PHY_DEBUG
+#define IMX_PHY_DEBUG          0
+#endif
+
+#if IMX_FEC_DEBUG
+#define DPRINTF(fmt, ...) \
+do { printf("imx_fec: " fmt , ## __VA_ARGS__); } while (0)
+#else
+#define DPRINTF(fmt, ...) do {} while (0)
+#endif
+
+#if IMX_PHY_DEBUG
+#define DPPRINTF(fmt, ...) \
+do { printf("imx_fec_phy: " fmt , ## __VA_ARGS__); } while (0)
+#else
+#define DPPRINTF(fmt, ...) do {} while (0)
+#endif
+
+#define FEC_MAX_FRAME_SIZE 2032
+
+typedef struct {
+    SysBusDevice busdev;
+    NICState *nic;
+    NICConf conf;
+    qemu_irq irq;
+    MemoryRegion iomem;
+
+    uint32_t irq_state;
+    uint32_t eir;
+    uint32_t eimr;
+    uint32_t rx_enabled;
+    uint32_t rx_descriptor;
+    uint32_t tx_descriptor;
+    uint32_t ecr;
+    uint32_t mmfr;
+    uint32_t mscr;
+    uint32_t mibc;
+    uint32_t rcr;
+    uint32_t tcr;
+    uint32_t tfwr;
+    uint32_t frsr;
+    uint32_t erdsr;
+    uint32_t etdsr;
+    uint32_t emrbr;
+    uint32_t miigsk_cfgr;
+    uint32_t miigsk_enr;
+
+    uint32_t phy_status;
+    uint32_t phy_control;
+    uint32_t phy_advertise;
+    uint32_t phy_int;
+    uint32_t phy_int_mask;
+} imx_fec_state;
+
+static const VMStateDescription vmstate_imx_fec = {
+    .name = "fec",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(irq_state, imx_fec_state),
+        VMSTATE_UINT32(eir, imx_fec_state),
+        VMSTATE_UINT32(eimr, imx_fec_state),
+        VMSTATE_UINT32(rx_enabled, imx_fec_state),
+        VMSTATE_UINT32(rx_descriptor, imx_fec_state),
+        VMSTATE_UINT32(tx_descriptor, imx_fec_state),
+        VMSTATE_UINT32(ecr, imx_fec_state),
+        VMSTATE_UINT32(mmfr, imx_fec_state),
+        VMSTATE_UINT32(mscr, imx_fec_state),
+        VMSTATE_UINT32(mibc, imx_fec_state),
+        VMSTATE_UINT32(rcr, imx_fec_state),
+        VMSTATE_UINT32(tcr, imx_fec_state),
+        VMSTATE_UINT32(tfwr, imx_fec_state),
+        VMSTATE_UINT32(frsr, imx_fec_state),
+        VMSTATE_UINT32(erdsr, imx_fec_state),
+        VMSTATE_UINT32(etdsr, imx_fec_state),
+        VMSTATE_UINT32(emrbr, imx_fec_state),
+        VMSTATE_UINT32(miigsk_cfgr, imx_fec_state),
+        VMSTATE_UINT32(miigsk_enr, imx_fec_state),
+
+        VMSTATE_UINT32(phy_status, imx_fec_state),
+        VMSTATE_UINT32(phy_control, imx_fec_state),
+        VMSTATE_UINT32(phy_advertise, imx_fec_state),
+        VMSTATE_UINT32(phy_int, imx_fec_state),
+        VMSTATE_UINT32(phy_int_mask, imx_fec_state),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+#define PHY_INT_ENERGYON            (1 << 7)
+#define PHY_INT_AUTONEG_COMPLETE    (1 << 6)
+#define PHY_INT_FAULT               (1 << 5)
+#define PHY_INT_DOWN                (1 << 4)
+#define PHY_INT_AUTONEG_LP          (1 << 3)
+#define PHY_INT_PARFAULT            (1 << 2)
+#define PHY_INT_AUTONEG_PAGE        (1 << 1)
+
+static void imx_fec_update(imx_fec_state *s);
+
+/*
+ * The MII phy could raise a GPIO to the processor which in turn
+ * could be handled as an interrpt by the OS.
+ * For now we don't handle any GPIO/interrupt line, so the OS will
+ * have to poll for the PHY status.
+ */
+static void phy_update_irq(imx_fec_state *s)
+{
+    imx_fec_update(s);
+}
+
+static void phy_update_link(imx_fec_state *s)
+{
+    /* Autonegotiation status mirrors link status.  */
+    if (qemu_get_queue(s->nic)->link_down) {
+        DPPRINTF("%s: link is down\n", __func__);
+        s->phy_status &= ~0x0024;
+        s->phy_int |= PHY_INT_DOWN;
+    } else {
+        DPPRINTF("%s: link is up\n", __func__);
+        s->phy_status |= 0x0024;
+        s->phy_int |= PHY_INT_ENERGYON;
+        s->phy_int |= PHY_INT_AUTONEG_COMPLETE;
+    }
+    phy_update_irq(s);
+}
+
+static void imx_fec_set_link(NetClientState *nc)
+{
+    phy_update_link(qemu_get_nic_opaque(nc));
+}
+
+static void phy_reset(imx_fec_state *s)
+{
+    DPPRINTF("%s\n", __func__);
+
+    s->phy_status = 0x7809;
+    s->phy_control = 0x3000;
+    s->phy_advertise = 0x01e1;
+    s->phy_int_mask = 0;
+    s->phy_int = 0;
+    phy_update_link(s);
+}
+
+static uint32_t do_phy_read(imx_fec_state *s, int reg)
+{
+    uint32_t val;
+
+    switch (reg) {
+    case 0:     /* Basic Control */
+        DPPRINTF("PHY read reg %d = %04x\n", reg, s->phy_control);
+        return s->phy_control;
+    case 1:     /* Basic Status */
+        DPPRINTF("PHY read reg %d = %04x\n", reg, s->phy_status);
+        return s->phy_status;
+    case 2:     /* ID1 */
+        DPPRINTF("PHY read reg %d = %04x\n", reg, 0x0007);
+        return 0x0007;
+    case 3:     /* ID2 */
+        DPPRINTF("PHY read reg %d = %04x\n", reg, 0xc0d1);
+        return 0xc0d1;
+    case 4:     /* Auto-neg advertisement */
+        DPPRINTF("PHY read reg %d = %04x\n", reg, s->phy_advertise);
+        return s->phy_advertise;
+    case 5:     /* Auto-neg Link Partner Ability */
+        DPPRINTF("PHY read reg %d = %04x\n", reg, 0x0f71);
+        return 0x0f71;
+    case 6:     /* Auto-neg Expansion */
+        DPPRINTF("PHY read reg %d = %04x\n", reg, 1);
+        return 1;
+        /* TODO 17, 18, 27, 29, 30, 31 */
+    case 29:    /* Interrupt source.  */
+        val = s->phy_int;
+        s->phy_int = 0;
+        phy_update_irq(s);
+        return val;
+    case 30:    /* Interrupt mask */
+        return s->phy_int_mask;
+    default:
+        DPPRINTF("PHY read reg %d, ignored, returning 0\n", reg);
+        return 0;
+    }
+}
+
+static void do_phy_write(imx_fec_state *s, int reg, uint32_t val)
+{
+    switch (reg) {
+    case 0:     /* Basic Control */
+        if (val & 0x8000) {
+            phy_reset(s);
+        } else {
+            s->phy_control = val & 0x7980;
+            /* Complete autonegotiation immediately.  */
+            if (val & 0x1000) {
+                s->phy_status |= 0x0020;
+            }
+        }
+        break;
+    case 4:     /* Auto-neg advertisement */
+        s->phy_advertise = (val & 0x2d7f) | 0x80;
+        break;
+        /* TODO 17, 18, 27, 31 */
+    case 30:    /* Interrupt mask */
+        s->phy_int_mask = val & 0xff;
+        phy_update_irq(s);
+        break;
+    default:
+        DPPRINTF("PHY write reg %d = 0x%04x, ignored\n", reg, val);
+    }
+}
+
+#define FEC_INT_HB      (1 << 31)
+#define FEC_INT_BABR    (1 << 30)
+#define FEC_INT_BABT    (1 << 29)
+#define FEC_INT_GRA     (1 << 28)
+#define FEC_INT_TXF     (1 << 27)
+#define FEC_INT_TXB     (1 << 26)
+#define FEC_INT_RXF     (1 << 25)
+#define FEC_INT_RXB     (1 << 24)
+#define FEC_INT_MII     (1 << 23)
+#define FEC_INT_EBERR   (1 << 22)
+#define FEC_INT_LC      (1 << 21)
+#define FEC_INT_RL      (1 << 20)
+#define FEC_INT_UN      (1 << 19)
+
+#define FEC_EN      2
+#define FEC_RESET   1
+
+/* Buffer Descriptor.  */
+typedef struct {
+    uint16_t length;
+    uint16_t flags;
+    uint32_t data;
+} imx_fec_bd;
+
+#define FEC_BD_R    (1 << 15)
+#define FEC_BD_E    (1 << 15)
+#define FEC_BD_O1   (1 << 14)
+#define FEC_BD_W    (1 << 13)
+#define FEC_BD_O2   (1 << 12)
+#define FEC_BD_L    (1 << 11)
+#define FEC_BD_TC   (1 << 10)
+#define FEC_BD_ABC  (1 << 9)
+#define FEC_BD_M    (1 << 8)
+#define FEC_BD_BC   (1 << 7)
+#define FEC_BD_MC   (1 << 6)
+#define FEC_BD_LG   (1 << 5)
+#define FEC_BD_NO   (1 << 4)
+#define FEC_BD_CR   (1 << 2)
+#define FEC_BD_OV   (1 << 1)
+#define FEC_BD_TR   (1 << 0)
+
+static void imx_fec_read_bd(imx_fec_bd *bd, uint32_t addr)
+{
+    cpu_physical_memory_read(addr, (uint8_t *) bd, sizeof(*bd));
+}
+
+static void imx_fec_write_bd(imx_fec_bd *bd, uint32_t addr)
+{
+    cpu_physical_memory_write(addr, (uint8_t *) bd, sizeof(*bd));
+}
+
+static void imx_fec_update(imx_fec_state *s)
+{
+    uint32_t active;
+    uint32_t changed;
+
+    active = s->eir & s->eimr;
+    changed = active ^ s->irq_state;
+    qemu_set_irq(s->irq, changed);
+    s->irq_state = active;
+}
+
+static void imx_fec_do_tx(imx_fec_state *s)
+{
+    uint32_t addr;
+    imx_fec_bd bd;
+    int frame_size;
+    int len;
+    uint8_t frame[FEC_MAX_FRAME_SIZE];
+    uint8_t *ptr;
+
+    DPRINTF("%s:\n", __func__);
+    ptr = frame;
+    frame_size = 0;
+    addr = s->tx_descriptor;
+    while (1) {
+        imx_fec_read_bd(&bd, addr);
+        DPRINTF("%s: tx_bd %x flags %04x len %d data %08x\n",
+                __func__, addr, bd.flags, bd.length, bd.data);
+        if ((bd.flags & FEC_BD_R) == 0) {
+            /* Run out of descriptors to transmit.  */
+            break;
+        }
+        len = bd.length;
+        if (frame_size + len > FEC_MAX_FRAME_SIZE) {
+            len = FEC_MAX_FRAME_SIZE - frame_size;
+            s->eir |= FEC_INT_BABT;
+        }
+        cpu_physical_memory_read(bd.data, ptr, len);
+        ptr += len;
+        frame_size += len;
+        if (bd.flags & FEC_BD_L) {
+            /* Last buffer in frame.  */
+            DPRINTF("Sending packet\n");
+            qemu_send_packet(qemu_get_queue(s->nic), frame, len);
+            ptr = frame;
+            frame_size = 0;
+            s->eir |= FEC_INT_TXF;
+        }
+        s->eir |= FEC_INT_TXB;
+        bd.flags &= ~FEC_BD_R;
+        /* Write back the modified descriptor.  */
+        imx_fec_write_bd(&bd, addr);
+        /* Advance to the next descriptor.  */
+        if ((bd.flags & FEC_BD_W) != 0) {
+            addr = s->etdsr;
+        } else {
+            addr += 8;
+        }
+    }
+    s->tx_descriptor = addr;
+    imx_fec_update(s);
+}
+
+static void imx_fec_enable_rx(imx_fec_state *s)
+{
+    imx_fec_bd bd;
+
+    imx_fec_read_bd(&bd, s->rx_descriptor);
+    s->rx_enabled = ((bd.flags & FEC_BD_E) != 0);
+    if (!s->rx_enabled) {
+        DPRINTF("%s: RX buffer full\n", __func__);
+    }
+}
+
+static void imx_fec_reset(DeviceState *d)
+{
+    imx_fec_state *s = FROM_SYSBUS(imx_fec_state, SYS_BUS_DEVICE(d));
+
+    /* Reset the FEC */
+    s->eir = 0;
+    s->eimr = 0;
+    s->rx_enabled = 0;
+    s->ecr = 0;
+    s->mscr = 0;
+    s->mibc = 0xc0000000;
+    s->rcr = 0x05ee0001;
+    s->tcr = 0;
+    s->tfwr = 0;
+    s->frsr = 0x500;
+    s->miigsk_cfgr = 0;
+    s->miigsk_enr = 0x6;
+
+    /* We also reset the PHY */
+    phy_reset(s);
+}
+
+static uint64_t imx_fec_read(void *opaque, hwaddr addr, unsigned size)
+{
+    imx_fec_state *s = (imx_fec_state *) opaque;
+
+    DPRINTF("%s: addr = 0x%x\n", __func__, (int)addr);
+
+    switch (addr & 0x3ff) {
+    case 0x004:
+        return s->eir;
+    case 0x008:
+        return s->eimr;
+    case 0x010:
+        return s->rx_enabled ? (1 << 24) : 0;   /* RDAR */
+    case 0x014:
+        return 0;   /* TDAR */
+    case 0x024:
+        return s->ecr;
+    case 0x040:
+        return s->mmfr;
+    case 0x044:
+        return s->mscr;
+    case 0x064:
+        return s->mibc; /* MIBC */
+    case 0x084:
+        return s->rcr;
+    case 0x0c4:
+        return s->tcr;
+    case 0x0e4:     /* PALR */
+        return (s->conf.macaddr.a[0] << 24) | (s->conf.macaddr.
+                                               a[1] << 16)
+               | (s->conf.macaddr.a[2] << 8) | s->conf.macaddr.a[3];
+        break;
+    case 0x0e8:     /* PAUR */
+        return (s->conf.macaddr.a[4] << 24) | (s->conf.macaddr.
+                                               a[5] << 16) | 0x8808;
+    case 0x0ec:
+        return 0x10000; /* OPD */
+    case 0x118:
+        return 0;
+    case 0x11c:
+        return 0;
+    case 0x120:
+        return 0;
+    case 0x124:
+        return 0;
+    case 0x144:
+        return s->tfwr;
+    case 0x14c:
+        return 0x600;
+    case 0x150:
+        return s->frsr;
+    case 0x180:
+        return s->erdsr;
+    case 0x184:
+        return s->etdsr;
+    case 0x188:
+        return s->emrbr;
+    case 0x300:
+        return s->miigsk_cfgr;
+    case 0x308:
+        return s->miigsk_enr;
+    default:
+        hw_error("imx_fec_read: Bad address 0x%x\n", (int)addr);
+        return 0;
+    }
+}
+
+static void imx_fec_write(void *opaque, hwaddr addr,
+                          uint64_t value, unsigned size)
+{
+    imx_fec_state *s = (imx_fec_state *) opaque;
+
+    DPRINTF("%s: 0x%x @ addr = 0x%x\n", __func__, (int)value, (int)addr);
+
+    switch (addr & 0x3ff) {
+    case 0x004: /* EIR */
+        s->eir &= ~value;
+        break;
+    case 0x008: /* EIMR */
+        s->eimr = value;
+        break;
+    case 0x010: /* RDAR */
+        if ((s->ecr & FEC_EN) && !s->rx_enabled) {
+            DPRINTF("RX enable\n");
+            imx_fec_enable_rx(s);
+        }
+        break;
+    case 0x014: /* TDAR */
+        if (s->ecr & FEC_EN) {
+            imx_fec_do_tx(s);
+        }
+        break;
+    case 0x024: /* ECR */
+        s->ecr = value;
+        if (value & FEC_RESET) {
+            DPRINTF("Reset\n");
+            imx_fec_reset(&s->busdev.qdev);
+        }
+        if ((s->ecr & FEC_EN) == 0) {
+            s->rx_enabled = 0;
+        }
+        break;
+    case 0x040: /* MMFR */
+        /* store the value */
+        s->mmfr = value;
+        if (value & (1 << 28)) {
+            DPRINTF("PHY write %d = 0x%04x\n",
+                    ((int)value >> 18) & 0x1f, (int)value & 0xffff);
+            do_phy_write(s, (value >> 18) & 0x1f, value & 0xffff);
+        } else {
+            s->mmfr = do_phy_read(s, (value >> 18) & 0x1f);
+            DPRINTF("PHY read %d = 0x%04x\n",
+                    ((int)value >> 18) & 0x1f, s->mmfr & 0xffff);
+        }
+        /* raise the interrupt as the PHY operation is done */
+        s->eir |= FEC_INT_MII;
+        break;
+    case 0x044: /* MSCR */
+        s->mscr = value & 0xfe;
+        break;
+    case 0x064: /* MIBC */
+        /* TODO: Implement MIB.  */
+        s->mibc = (value & 0x80000000) ? 0xc0000000 : 0;
+        break;
+    case 0x084: /* RCR */
+        s->rcr = value & 0x07ff003f;
+        /* TODO: Implement LOOP mode.  */
+        break;
+    case 0x0c4: /* TCR */
+        /* We transmit immediately, so raise GRA immediately.  */
+        s->tcr = value;
+        if (value & 1) {
+            s->eir |= FEC_INT_GRA;
+        }
+        break;
+    case 0x0e4: /* PALR */
+        s->conf.macaddr.a[0] = value >> 24;
+        s->conf.macaddr.a[1] = value >> 16;
+        s->conf.macaddr.a[2] = value >> 8;
+        s->conf.macaddr.a[3] = value;
+        break;
+    case 0x0e8: /* PAUR */
+        s->conf.macaddr.a[4] = value >> 24;
+        s->conf.macaddr.a[5] = value >> 16;
+        break;
+    case 0x0ec: /* OPDR */
+        break;
+    case 0x118: /* IAUR */
+    case 0x11c: /* IALR */
+    case 0x120: /* GAUR */
+    case 0x124: /* GALR */
+        /* TODO: implement MAC hash filtering.  */
+        break;
+    case 0x144: /* TFWR */
+        s->tfwr = value & 3;
+        break;
+    case 0x14c: /* FRBR */
+        /* FRBR writes ignored.  */
+        break;
+    case 0x150: /* FRSR */
+        s->frsr = (value & 0x3fc) | 0x400;
+        break;
+    case 0x180: /* ERDSR */
+        s->erdsr = value & ~3;
+        s->rx_descriptor = s->erdsr;
+        break;
+    case 0x184: /* ETDSR */
+        s->etdsr = value & ~3;
+        s->tx_descriptor = s->etdsr;
+        break;
+    case 0x188: /* EMRBR */
+        s->emrbr = value & 0x7f0;
+        break;
+    case 0x300: /* MIIGSK_CFGR */
+        s->miigsk_cfgr = value & 0x53;
+        break;
+    case 0x308: /* MIIGSK_ENR */
+        s->miigsk_enr = (value & 0x2) ? 0x6 : 0;
+        break;
+    default:
+        hw_error("imx_fec_write Bad address 0x%x\n", (int)addr);
+    }
+    imx_fec_update(s);
+}
+
+static int imx_fec_can_receive(NetClientState *nc)
+{
+    imx_fec_state *s = qemu_get_nic_opaque(nc);
+
+    return s->rx_enabled;
+}
+
+static ssize_t imx_fec_receive(NetClientState *nc, const uint8_t *buf,
+                               size_t len)
+{
+    imx_fec_state *s = qemu_get_nic_opaque(nc);
+    imx_fec_bd bd;
+    uint32_t flags = 0;
+    uint32_t addr;
+    uint32_t crc;
+    uint32_t buf_addr;
+    uint8_t *crc_ptr;
+    unsigned int buf_len;
+    size_t size = len;
+
+    DPRINTF("%s: len %d\n", __func__, (int)size);
+
+    if (!s->rx_enabled) {
+        DPRINTF("%s: Unexpected packet\n", __func__);
+        return 0;
+    }
+    /* 4 bytes for the CRC.  */
+    size += 4;
+    crc = cpu_to_be32(crc32(~0, buf, size));
+    crc_ptr = (uint8_t *) &crc;
+    /* Huge frames are truncted.  */
+    if (size > FEC_MAX_FRAME_SIZE) {
+        size = FEC_MAX_FRAME_SIZE;
+        flags |= FEC_BD_TR | FEC_BD_LG;
+    }
+    /* Frames larger than the user limit just set error flags.  */
+    if (size > (s->rcr >> 16)) {
+        flags |= FEC_BD_LG;
+    }
+    addr = s->rx_descriptor;
+    while (size > 0) {
+        imx_fec_read_bd(&bd, addr);
+        if ((bd.flags & FEC_BD_E) == 0) {
+            /* No descriptors available.  Bail out.  */
+            /*
+             * FIXME: This is wrong. We should probably either
+             * save the remainder for when more RX buffers are
+             * available, or flag an error.
+             */
+            DPRINTF("%s: Lost end of frame\n", __func__);
+            break;
+        }
+        buf_len = (size <= s->emrbr) ? size : s->emrbr;
+        bd.length = buf_len;
+        size -= buf_len;
+        DPRINTF("rx_bd %x length %d\n", addr, bd.length);
+        /* The last 4 bytes are the CRC.  */
+        if (size < 4) {
+            buf_len += size - 4;
+        }
+        buf_addr = bd.data;
+        cpu_physical_memory_write(buf_addr, buf, buf_len);
+        buf += buf_len;
+        if (size < 4) {
+            cpu_physical_memory_write(buf_addr + buf_len, crc_ptr,
+                                      4 - size);
+            crc_ptr += 4 - size;
+        }
+        bd.flags &= ~FEC_BD_E;
+        if (size == 0) {
+            /* Last buffer in frame.  */
+            bd.flags |= flags | FEC_BD_L;
+            DPRINTF("rx frame flags %04x\n", bd.flags);
+            s->eir |= FEC_INT_RXF;
+        } else {
+            s->eir |= FEC_INT_RXB;
+        }
+        imx_fec_write_bd(&bd, addr);
+        /* Advance to the next descriptor.  */
+        if ((bd.flags & FEC_BD_W) != 0) {
+            addr = s->erdsr;
+        } else {
+            addr += 8;
+        }
+    }
+    s->rx_descriptor = addr;
+    imx_fec_enable_rx(s);
+    imx_fec_update(s);
+    return len;
+}
+
+static const MemoryRegionOps imx_fec_ops = {
+    .read = imx_fec_read,
+    .write = imx_fec_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static void imx_fec_cleanup(NetClientState *nc)
+{
+    imx_fec_state *s = qemu_get_nic_opaque(nc);
+
+    s->nic = NULL;
+}
+
+static NetClientInfo net_imx_fec_info = {
+    .type = NET_CLIENT_OPTIONS_KIND_NIC,
+    .size = sizeof(NICState),
+    .can_receive = imx_fec_can_receive,
+    .receive = imx_fec_receive,
+    .cleanup = imx_fec_cleanup,
+    .link_status_changed = imx_fec_set_link,
+};
+
+static int imx_fec_init(SysBusDevice *dev)
+{
+    imx_fec_state *s = FROM_SYSBUS(imx_fec_state, dev);
+
+    memory_region_init_io(&s->iomem, &imx_fec_ops, s, "fec_mmio", 0x400);
+    sysbus_init_mmio(dev, &s->iomem);
+    sysbus_init_irq(dev, &s->irq);
+    qemu_macaddr_default_if_unset(&s->conf.macaddr);
+
+    s->conf.peers.ncs[0] = nd_table[0].netdev;
+
+    s->nic = qemu_new_nic(&net_imx_fec_info, &s->conf,
+                          object_get_typename(OBJECT(dev)), dev->qdev.id,
+                          s);
+    qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
+
+    return 0;
+}
+
+static Property imx_fec_properties[] = {
+    DEFINE_NIC_PROPERTIES(imx_fec_state, conf),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void imx_fec_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+
+    k->init = imx_fec_init;
+    dc->reset = imx_fec_reset;
+    dc->vmsd = &vmstate_imx_fec;
+    dc->props = imx_fec_properties;
+}
+
+static const TypeInfo imx_fec_info = {
+    .name = "fec",
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(imx_fec_state),
+    .class_init = imx_fec_class_init,
+};
+
+static void imx_fec_register_types(void)
+{
+    type_register_static(&imx_fec_info);
+}
+
+void imx_fec_create(int nic, const hwaddr base, qemu_irq irq)
+{
+    NICInfo *nd;
+    DeviceState *dev;
+    SysBusDevice *s;
+
+    if (nic >= MAX_NICS) {
+        hw_error("Cannot assign nic %d: QEMU supports only %d ports\n",
+                 nic, MAX_NICS);
+    }
+
+    nd = &nd_table[nic];
+
+    qemu_check_nic_model(nd, "fec");
+    dev = qdev_create(NULL, "fec");
+    qdev_set_nic_properties(dev, nd);
+    qdev_init_nofail(dev);
+    s = SYS_BUS_DEVICE(dev);
+    sysbus_mmio_map(s, 0, base);
+    sysbus_connect_irq(s, 0, irq);
+};
+
+type_init(imx_fec_register_types)