Re: [Qemu-devel] [PATCH 3/5] hw/net/stellaris_enet: Rewrite tx fifo handling code

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

(resend reply - the mail gru got some of the 1st one)

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On 1 April 2014 18:26, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> > * Peter Maydell (peter.maydell@linaro.org) wrote:
> >> The datasheet is clear that the frame length written to the DATA
> >> register is actually stored in the TX FIFO; this means we don't
> >> need to keep both tx_frame_len and tx_fifo_len state separately.
> >>
> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> >> ---
> >>  hw/net/stellaris_enet.c | 119 +++++++++++++++++++++++++++++++-----------------
> >>  1 file changed, 77 insertions(+), 42 deletions(-)
> >>
> >> diff --git a/hw/net/stellaris_enet.c b/hw/net/stellaris_enet.c
> >> index d0da819..b99f93e 100644
> >> --- a/hw/net/stellaris_enet.c
> >> +++ b/hw/net/stellaris_enet.c
> >> @@ -59,7 +59,6 @@ typedef struct {
> >>      uint32_t mtxd;
> >>      uint32_t mrxd;
> >>      uint32_t np;
> >> -    int tx_frame_len;
> >>      int tx_fifo_len;
> >>      uint8_t tx_fifo[2048];
> >>      /* Real hardware has a 2k fifo, which works out to be at most 31 packets.
> >> @@ -82,6 +81,62 @@ static void stellaris_enet_update(stellaris_enet_state *s)
> >>      qemu_set_irq(s->irq, (s->ris & s->im) != 0);
> >>  }
> >>
> >> +/* Return the data length of the packet currently being assembled
> >> + * in the TX fifo.
> >> + */
> >> +static inline int stellaris_txpacket_datalen(stellaris_enet_state *s)
> >> +{
> >> +    return s->tx_fifo[0] | (s->tx_fifo[1] << 8);
> >> +}
> >> +
> >> +/* Return true if the packet currently in the TX FIFO is complete,
> >> +* ie the FIFO holds enough bytes for the data length, ethernet header,
> >> +* payload and optionally CRC.
> >> +*/
> >> +static inline bool stellaris_txpacket_complete(stellaris_enet_state *s)
> >> +{
> >> +    int framelen = stellaris_txpacket_datalen(s);
> >> +    framelen += 16;
> >
> > What's the magical 16? (It doesn't jump out from the data sheet).
> 
> See table 15-3: 2 bytes of data length, 6 bytes dest addr,
> 6 bytes source addr, 2 bytes len/type.

Ah yes or the text in 15.3.1.2 para 4 'referes to the Etehernet frame data
payload, as shown in the 5th to nth FIFO positions' (1,2,3,4 each 4 bytes).

> > You should probably increment the migration state version number to 2.
>
> Oops, yes.

Fix if you need to reroll, but not too important since i doubt people are
migrating it.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK