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

[Scott Wood]

On 07/16/2013 10:28:28 AM, Fabien Chouteau wrote:
> 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.

OK, I didn't see the cover.

> >> 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?

Not strictly necessary, but it would help keep things tidy.

> >> +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...

These are interrupt flags that you do set elsewhere.

> >> +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).

Someone should, so we can test this without your out-of-tree code.

> e500.c would require PCI support I think.

e500.c has PCI support, but how is that relevant to eTSEC?

> >> +    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...

Maybe throw in an assertion, then?

I can see how it might not be possible if rx_padding is being used for  
padding a short frame, since MRBLR must be a multiple of 64, but what  
if it's 4 bytes for CRC?

> >> +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.

Yes, sorry.

> >> +    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.

This is for when RCTRL[RSF] is set, to accept short frames?  Is it  
documented anywhere that these frames are zero-padded to 64 bytes on  
rx?  If not, is this the observed behavior of real hardware?

In any case, please add some 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.

When adding code to mainline, it's about more than what you're  
personally interested in...

Could you at least have a way to diagnose when the guest OS tries to  
use some functionality that you don't support, rather than silently  
doing the wrong thing?

> >> +                /* 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.

OK, it's just an error detection flag, not something that's actually  
supported.

-Scott