Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC)

[Scott Wood <scottwood@freescale.com>]

On 07/10/2013 12:10:02 PM, Fabien Chouteau wrote:
> This implementation doesn't include ring priority, TCP/IP Off-Load, QoS.
> 
> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>

>From the code comments I gather this has been tested on VxWorks.  Has it
been tested on Linux, or anywhere else?

> ---
>  default-configs/ppc-softmmu.mak |    1 +
>  hw/net/Makefile.objs            |    1 +
>  hw/net/etsec.c                  |  472 +++++++++++++++++++++++++++
>  hw/net/etsec.h                  |  169 ++++++++++
>  hw/net/etsec_miim.c             |  146 +++++++++
>  hw/net/etsec_registers.c        |  295 +++++++++++++++++
>  hw/net/etsec_registers.h        |  302 ++++++++++++++++++
>  hw/net/etsec_rings.c            |  673 +++++++++++++++++++++++++++++++++++++++
>  8 files changed, 2059 insertions(+)
>  create mode 100644 hw/net/etsec.c
>  create mode 100644 hw/uuunet/etsec.h
>  create mode 100644 hw/net/etsec_miim.c
>  create mode 100644 hw/net/etsec_registers.c
>  create mode 100644 hw/net/etsec_registers.h
>  create mode 100644 hw/net/etsec_rings.c

This should probably be namespaced as something like fsl_etsec.

> diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
> index 73e4cc5..c7541cf 100644
> --- a/default-configs/ppc-softmmu.mak
> +++ b/default-configs/ppc-softmmu.mak
> @@ -46,3 +46,4 @@ CONFIG_E500=y
>  CONFIG_OPENPIC_KVM=$(and $(CONFIG_E500),$(CONFIG_KVM))
>  # For PReP
>  CONFIG_MC146818RTC=y
> +CONFIG_ETSEC=y
> diff --git a/hw/net/Makefile.objs b/hw/net/Makefile.objs
> index 951cca3..ca03c3f 100644
> --- a/hw/net/Makefile.objs
> +++ b/hw/net/Makefile.objs
> @@ -28,6 +28,7 @@ obj-$(CONFIG_COLDFIRE) += mcf_fec.o
>  obj-$(CONFIG_MILKYMIST) += milkymist-minimac2.o
>  obj-$(CONFIG_PSERIES) += spapr_llan.o
>  obj-$(CONFIG_XILINX_ETHLITE) += xilinx_ethlite.o
> +obj-$(CONFIG_ETSEC) += etsec.o etsec_registers.o etsec_rings.o etsec_miim.o

Maybe an fsl_etsec directory?

> +static uint64_t etsec_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    eTSEC          *etsec     = opaque;
> +    uint32_t        reg_index = addr / 4;
> +    eTSEC_Register *reg       = NULL;
> +    uint32_t        ret       = 0x0;

It's always awkward when QEMU's type naming convention collides with
names that have pre-existing significant capitalization, but I suspect
this ought to be Etsec and EtsecRegister.  Or maybe ETSEC and
ETSECRegister?  Oh well.

> +    assert(reg_index < REG_NUMBER);
> +
> +    reg = &etsec->regs[reg_index];
> +
> +
> +    switch (reg->access) {
> +    case ACC_WO:
> +        ret = 0x00000000;
> +        break;
> +
> +    case ACC_RW:
> +    case ACC_w1c:
> +    case ACC_RO:
> +    default:
> +        ret = reg->value;
> +        break;
> +    }

Why is "w1c" lowercase when the rest are uppercase?  I realize the
hardware docs do that, but in this case I don't think that takes
precedence over consistent coding style for #defines.

> +#ifdef DEBUG_REGISTER
> +    printf("Read  0x%08x @ 0x" TARGET_FMT_plx
> +           "                            : %s (%s)\n",
> +           ret, addr, reg->name, reg->desc);
> +#endif

This is likely to bitrot -- please consider doing something similar to DPRINTF in hw/intc/openpic.c.

> +static void write_ievent(eTSEC          *etsec,
> +                         eTSEC_Register *reg,
> +                         uint32_t        reg_index,
> +                         uint32_t        value)
> +{
> +    if (value & IEVENT_TXF) {
> +        qemu_irq_lower(etsec->tx_irq);
> +    }
> +    if (value & IEVENT_RXF) {
> +        qemu_irq_lower(etsec->rx_irq);
> +    }
> +
> +    /* Write 1 to clear */
> +    reg->value &= ~value;
> +}

What about the error interrupt?  You raise it but never lower it that I
can see.

TXB/RXB should also be included, and you should only lower the interrupt
if neither ?XB nor ?XF are set for a particular direction.

> +#ifdef HEX_DUMP
> +static void hex_dump(FILE *f, const uint8_t *buf, int size)
> +{
> +    int len, i, j, c;
> +
> +    for (i = 0; i < size; i += 16) {
> +        len = size - i;
> +        if (len > 16) {
> +            len = 16;
> +        }
> +        fprintf(f, "%08x ", i);
> +        for (j = 0; j < 16; j++) {
> +            if (j < len) {
> +                fprintf(f, " %02x", buf[i + j]);
> +            } else {
> +                fprintf(f, "   ");
> +            }
> +        }
> +        fprintf(f, " ");
> +        for (j = 0; j < len; j++) {
> +            c = buf[i + j];
> +            if (c < ' ' || c > '~') {
> +                c = '.';
> +            }
> +            fprintf(f, "%c", c);
> +        }
> +        fprintf(f, "\n");
> +    }
> +}
> +#endif

qemu_hexdump()

> +static int etsec_init(SysBusDevice *dev)
> +{
> +    eTSEC *etsec = FROM_SYSBUS(typeof(*etsec), dev);

I was recently yelled at for using FROM_SYSBUS and related
deprecated infrastructure -- see http://wiki.qemu.org/QOMConventions

> +DeviceState *etsec_create(hwaddr         base,
> +                          MemoryRegion * mr,
> +                          NICInfo      * nd,
> +                          qemu_irq       tx_irq,
> +                          qemu_irq       rx_irq,
> +                          qemu_irq       err_irq)
> +{
> +    DeviceState *dev;
> +
> +    dev = qdev_create(NULL, "eTSEC");
> +    qdev_set_nic_properties(dev, nd);
> +
> +    if (qdev_init(dev)) {
> +        return NULL;
> +    }
> +
> +    sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, tx_irq);
> +    sysbus_connect_irq(SYS_BUS_DEVICE(dev), 1, rx_irq);
> +    sysbus_connect_irq(SYS_BUS_DEVICE(dev), 2, err_irq);
> +
> +    memory_region_add_subregion(mr, base,
> +                                SYS_BUS_DEVICE(dev)->mmio[0].memory);
> +
> +    return dev;
> +}

Do you plan to update hw/ppc/e500.c (or maybe some other platform?) to
call this?

If you're centralizing this part of device creation, how about the device
tree bits as well?

> +/* eTSEC */
> +
> +#define REG_NUMBER 1024

This is pretty vague.

> +DeviceState *etsec_create(hwaddr        base,
> +                          MemoryRegion *mr,
> +                          NICInfo      *nd,
> +                          qemu_irq      tx_irq,
> +                          qemu_irq      rx_irq,
> +                          qemu_irq      err_irq);

You've got stuff in this file that isn't properly namespaced for
inclusion by arbitrary QEMU code (especially board code that needs to
include headers for several devices), such as REG_NUMBER, yet you declare
etsec_create here which has to be called from board code.

> +#ifdef ETSEC_RING_DEBUG
> +#define RING_DEBUG(fmt, ...) printf("%s:%s " fmt, __func__ ,\
> +                                    etsec->nic->nc.name, ## __VA_ARGS__)
> +#else
> +#define RING_DEBUG(...)
> +#endif  /* ETSEC_RING_DEBUG */

Please consume the arguments even if debug output is not enabled (so you
don't get unused variable warnings), and ideally do a printf inside an
if-statement (on a constant so it will be optimized away) so you still
get format checking -- again, similar to DPRINTF in hw/intc/openpic.c.

> +#define RING_DEBUG_A(fmt, ...) printf("%s:%s " fmt, __func__ ,\
> +                                      etsec->nic->nc.name, ## __VA_ARGS__)

"A"?

If this means "always", why not define RING_DEBUG in terms of this?

> +#ifdef HEX_DUMP
> +
> +static void hex_dump(FILE *f, const uint8_t *buf, int size)
> +{
> +    int len, i, j, c;
> +
> +    for (i = 0; i < size; i += 16) {
> +        len = size - i;
> +        if (len > 16) {
> +            len = 16;
> +        }
> +        fprintf(f, "%08x ", i);
> +        for (j = 0; j < 16; j++) {
> +            if (j < len) {
> +                fprintf(f, " %02x", buf[i + j]);
> +            } else {
> +                fprintf(f, "   ");
> +            }
> +        }
> +        fprintf(f, " ");
> +        for (j = 0; j < len; j++) {
> +            c = buf[i + j];
> +            if (c < ' ' || c > '~') {
> +                c = '.';
> +            }
> +            fprintf(f, "%c", c);
> +        }
> +        fprintf(f, "\n");
> +    }
> +}
> +
> +#endif

Two instances of this even in the same driver?

> +static void fill_rx_bd(eTSEC          *etsec,
> +                       eTSEC_rxtx_bd  *bd,
> +                       const uint8_t **buf,
> +                       size_t         *size)
> +{
> +    uint16_t to_write = MIN(etsec->rx_fcb_size + *size - etsec->rx_padding,
> +                            etsec->regs[MRBLR].value);
> +    uint32_t bufptr   = bd->bufptr;
> +    uint8_t  padd[etsec->rx_padding];
> +    uint8_t  rem;
> +
> +    RING_DEBUG("eTSEC fill Rx buffer @ 0x%08x"
> +               " size:%u(padding + crc:%u) + fcb:%u\n",
> +               bufptr, *size, etsec->rx_padding, etsec->rx_fcb_size);
> +
> +    bd->length = 0;
> +    if (etsec->rx_fcb_size != 0) {
> +        cpu_physical_memory_write(bufptr, etsec->rx_fcb, etsec->rx_fcb_size);
> +
> +        bufptr             += etsec->rx_fcb_size;
> +        bd->length         += etsec->rx_fcb_size;
> +        to_write           -= etsec->rx_fcb_size;
> +        etsec->rx_fcb_size  = 0;
> +
> +    }
> +
> +    if (to_write > 0) {
> +        cpu_physical_memory_write(bufptr, *buf, to_write);
> +
> +        *buf   += to_write;
> +        bufptr += to_write;
> +        *size  -= to_write;
> +
> +        bd->flags  &= ~BD_RX_EMPTY;
> +        bd->length += to_write;
> +    }
> +
> +    if (*size == etsec->rx_padding) {
> +        /* The remaining bytes are for padding which is not actually allocated
> +           in the buffer */
> +
> +        rem = MIN(etsec->regs[MRBLR].value - bd->length, etsec->rx_padding);
> +
> +        if (rem > 0) {
> +            memset(padd, 0x0, sizeof(padd));
> +            etsec->rx_padding -= rem;
> +            *size             -= rem;
> +            bd->length        += rem;
> +            cpu_physical_memory_write(bufptr, padd, rem);
> +        }
> +    }

What if *size > 0 && *size < etsec->rx_padding?

> +static void rx_init_frame(eTSEC *etsec, const uint8_t *buf, size_t size)
> +{
> +    uint32_t fcb_size = 0;
> +    uint8_t  prsdep   = (etsec->regs[RCTRL].value >> RCTRL_PRSDEP_OFFSET)
> +        & RCTRL_PRSDEP_MASK;
> +
> +    if (prsdep != 0) {
> +        /* Prepend FCB */
> +        fcb_size = 8 + 2;          /* FCB size + align */
> +        /* I can't find this 2 bytes alignement in fsl documentation but VxWorks
> +           expects them */

Could these 2 bytes be from RCTRL[PAD], which you seem to ignore?

> +    etsec->rx_padding    = 4;
> +    if (size < 60) {
> +        etsec->rx_padding += 60 - size;
> +    }

Could you explain what you're doing with rx_padding?

> +    /* ring_base = (etsec->regs[RBASEH].value & 0xF) << 32; */
> +    ring_base     += etsec->regs[RBASE0 + ring_nbr].value & ~0x7;
> +    start_bd_addr  = bd_addr = etsec->regs[RBPTR0 + ring_nbr].value & ~0x7;

What about RBDBPH (upper bits of physical address)?  Likewise for TX.

> +                /* NOTE: non-octet aligned frame is impossible in qemu */

Is it possible in eTSEC?

-Scott