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

[Fabien Chouteau]

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 <address@hidden>
> 
> 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