On 07/16/2013 04:06 AM, Scott Wood wrote: > 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? >
You're right, as I said in the cover letter, this has only been tested on vxWorks. >> 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. > Fixed. >> 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? > Is it really necessary? >> +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. 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. Fixed. >> +#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. Fixed. > >> +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. > I don't claim to support all interrupt flags but I will fix this... >> +#ifdef HEX_DUMP >> +static void hex_dump(FILE *f, const uint8_t *buf, int size) >> +{ >> ... >> +} >> +#endif > > qemu_hexdump() > Fixed. >> +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 Me too ;) Fixed. > >> +DeviceState *etsec_create(hwaddr base, >> + MemoryRegion * mr, >> + NICInfo * nd, >> + qemu_irq tx_irq, >> + qemu_irq rx_irq, >> + qemu_irq err_irq) >> > Do you plan to update hw/ppc/e500.c (or maybe some other platform?) to > call this? > No I don't, not for the moment. I use it in one of our machine (that is not in mainstream). e500.c would require PCI support I think. > If you're centralizing this part of device creation, how about the device > tree bits as well? > I don't know about device tree... >> +/* eTSEC */ >> + >> +#define REG_NUMBER 1024 > > This is pretty vague. > Fixed. >> +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. Fixed. > >> +#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. Fixed. > >> +#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? > This was just a handy trick, I will remove it. > > Two instances of this even in the same driver? > Fixed. >> +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? I don't think it's possible... > >> +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? Did you mean RCTRL[PAL]? It could be that. > >> + etsec->rx_padding = 4; CRC. >> + if (size < 60) { >> + etsec->rx_padding += 60 - size; >> + } > > Could you explain what you're doing with rx_padding? Avoiding short frames. I'll add a comments. > >> + /* 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. > I'm only interested in 32bits address spaces, so RBASEH, TBASEH, RBDBPH or TBDBPH. >> + /* NOTE: non-octet aligned frame is impossible in qemu */ > > Is it possible in eTSEC? > I think eTSEC can handle it and there's a flag in RxBD for that: NO | Rx non-octet aligned frame, written by the eTSEC (only valid if L is set). A frame that contained a number of bits not divisible by eight was received. But we will never receive such frame from QEMU. Thanks for your review. -- Fabien Chouteau