From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46679) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uz9P7-0005fT-PK for qemu-devel@nongnu.org; Tue, 16 Jul 2013 13:51:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Uz9P4-0007Qt-KM for qemu-devel@nongnu.org; Tue, 16 Jul 2013 13:51:13 -0400 Date: Tue, 16 Jul 2013 12:50:13 -0500 From: Scott Wood In-Reply-To: <51E5669C.2080602@adacore.com> (from chouteau@adacore.com on Tue Jul 16 10:28:28 2013) Message-ID: <1373997013.8183.332@snotra> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; delsp=Yes; format=Flowed Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/2] Add Enhanced Three-Speed Ethernet Controller (eTSEC) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fabien Chouteau Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org 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 =20 > Off-Load, QoS. > >> > >> Signed-off-by: Fabien Chouteau > > > > From the code comments I gather this has been tested on VxWorks. =20 > Has it > > been tested on Linux, or anywhere else? > > >=20 > You're right, as I said in the cover letter, this has only been =20 > 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) +=3D mcf_fec.o > >> obj-$(CONFIG_MILKYMIST) +=3D milkymist-minimac2.o > >> obj-$(CONFIG_PSERIES) +=3D spapr_llan.o > >> obj-$(CONFIG_XILINX_ETHLITE) +=3D xilinx_ethlite.o > >> +obj-$(CONFIG_ETSEC) +=3D etsec.o etsec_registers.o etsec_rings.o =20 > etsec_miim.o > > > > Maybe an fsl_etsec directory? > > >=20 > 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 &=3D ~value; > >> +} > > > > What about the error interrupt? You raise it but never lower it =20 > that I > > can see. > > > > TXB/RXB should also be included, and you should only lower the =20 > interrupt > > if neither ?XB nor ?XF are set for a particular direction. > > >=20 > 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?) =20 > to > > call this? > > >=20 > No I don't, not for the moment. I use it in one of our machine (that =20 > 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 =3D=3D etsec->rx_padding) { > >> + /* The remaining bytes are for padding which is not =20 > actually allocated > >> + in the buffer */ > >> + > >> + rem =3D MIN(etsec->regs[MRBLR].value - bd->length, =20 > etsec->rx_padding); > >> + > >> + if (rem > 0) { > >> + memset(padd, 0x0, sizeof(padd)); > >> + etsec->rx_padding -=3D rem; > >> + *size -=3D rem; > >> + bd->length +=3D rem; > >> + cpu_physical_memory_write(bufptr, padd, rem); > >> + } > >> + } > > > > What if *size > 0 && *size < etsec->rx_padding? >=20 > 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 =20 padding a short frame, since MRBLR must be a multiple of 64, but what =20 if it's 4 bytes for CRC? > >> +static void rx_init_frame(eTSEC *etsec, const uint8_t *buf, =20 > size_t size) > >> +{ > >> + uint32_t fcb_size =3D 0; > >> + uint8_t prsdep =3D (etsec->regs[RCTRL].value >> =20 > RCTRL_PRSDEP_OFFSET) > >> + & RCTRL_PRSDEP_MASK; > >> + > >> + if (prsdep !=3D 0) { > >> + /* Prepend FCB */ > >> + fcb_size =3D 8 + 2; /* FCB size + align */ > >> + /* I can't find this 2 bytes alignement in fsl =20 > documentation but VxWorks > >> + expects them */ > > > > Could these 2 bytes be from RCTRL[PAD], which you seem to ignore? >=20 > Did you mean RCTRL[PAL]? It could be that. Yes, sorry. > >> + etsec->rx_padding =3D 4; >=20 > CRC. >=20 > >> + if (size < 60) { > >> + etsec->rx_padding +=3D 60 - size; > >> + } > > > > Could you explain what you're doing with rx_padding? >=20 > Avoiding short frames. I'll add a comments. This is for when RCTRL[RSF] is set, to accept short frames? Is it =20 documented anywhere that these frames are zero-padded to 64 bytes on =20 rx? If not, is this the observed behavior of real hardware? In any case, please add some comments. > >> + /* ring_base =3D (etsec->regs[RBASEH].value & 0xF) << 32; */ > >> + ring_base +=3D etsec->regs[RBASE0 + ring_nbr].value & ~0x7; > >> + start_bd_addr =3D bd_addr =3D etsec->regs[RBPTR0 + =20 > ring_nbr].value & ~0x7; > > > > What about RBDBPH (upper bits of physical address)? Likewise for =20 > TX. > > >=20 > I'm only interested in 32bits address spaces, so RBASEH, TBASEH, =20 > RBDBPH or TBDBPH. When adding code to mainline, it's about more than what you're =20 personally interested in... Could you at least have a way to diagnose when the guest OS tries to =20 use some functionality that you don't support, rather than silently =20 doing the wrong thing? > >> + /* NOTE: non-octet aligned frame is impossible in =20 > qemu */ > > > > Is it possible in eTSEC? > > >=20 > I think eTSEC can handle it and there's a flag in RxBD for that: >=20 > NO | Rx non-octet aligned frame, written by the eTSEC (only valid if =20 > L is set). > A frame that contained a number of bits not divisible by eight was =20 > received. >=20 > But we will never receive such frame from QEMU. OK, it's just an error detection flag, not something that's actually =20 supported. -Scott=