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 <chout...@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
