Re: [Qemu-block] [Qemu-devel] [RFC v3 09/10] dp8393x: manage big endian bus

[Hervé Poussineau]

Le 08/07/2018 à 23:11, Peter Maydell a écrit :
> On 28 June 2018 at 00:29, Laurent Vivier <address@hidden> wrote:
> > This is needed by Quadra 800, this card can run on little-endian
> > or big-endian bus.
> >
> > Signed-off-by: Laurent Vivier <address@hidden>
> > Tested-by: Hervé Poussineau <address@hidden>
> > ---
> >   hw/net/dp8393x.c | 88 ++++++++++++++++++++++++++++++++++++--------------------
> >   1 file changed, 57 insertions(+), 31 deletions(-)
> >
> > diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> > index f2d2ce344c..62adff9ba3 100644
> > --- a/hw/net/dp8393x.c
> > +++ b/hw/net/dp8393x.c
> > @@ -150,6 +150,7 @@ typedef struct dp8393xState {
> >
> >       /* Hardware */
> >       uint8_t it_shift;
> > +    bool big_endian;
> >       qemu_irq irq;
> >   #ifdef DEBUG_SONIC
> >       int irq_level;
> > @@ -220,6 +221,29 @@ static uint32_t dp8393x_wt(dp8393xState *s)
> >       return s->regs[SONIC_WT1] << 16 | s->regs[SONIC_WT0];
> >   }
> >
> > +static uint16_t dp8393x_get(dp8393xState *s, int width, uint16_t *base,
> > +                            int offset)
> > +{
> > +    uint16_t val;
> > +
> > +    if (s->big_endian) {
> > +        val = be16_to_cpu(base[offset * width + width - 1]);
> > +    } else {
> > +        val = le16_to_cpu(base[offset * width]);
> > +    }
> > +    return val;
> > +}
> > +
> > +static void dp8393x_put(dp8393xState *s, int width, uint16_t *base, int offset,
> > +                        uint16_t val)
> > +{
> > +    if (s->big_endian) {
> > +        base[offset * width + width - 1] = cpu_to_be16(val);
> > +    } else {
> > +        base[offset * width] = cpu_to_le16(val);
> > +    }
> > +}
> > +
> >   static void dp8393x_update_irq(dp8393xState *s)
> >   {
> >       int level = (s->regs[SONIC_IMR] & s->regs[SONIC_ISR]) ? 1 : 0;
> > @@ -251,12 +275,12 @@ static void dp8393x_do_load_cam(dp8393xState *s)
> >           /* Fill current entry */
> >           address_space_rw(&s->as, dp8393x_cdp(s),
> >               MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size, 0);
> > -        s->cam[index][0] = data[1 * width] & 0xff;
> > -        s->cam[index][1] = data[1 * width] >> 8;
> > -        s->cam[index][2] = data[2 * width] & 0xff;
> > -        s->cam[index][3] = data[2 * width] >> 8;
> > -        s->cam[index][4] = data[3 * width] & 0xff;
> > -        s->cam[index][5] = data[3 * width] >> 8;
> > +        s->cam[index][0] = dp8393x_get(s, width, data, 1) & 0xff;
> > +        s->cam[index][1] = dp8393x_get(s, width, data, 1) >> 8;
> > +        s->cam[index][2] = dp8393x_get(s, width, data, 2) & 0xff;
> > +        s->cam[index][3] = dp8393x_get(s, width, data, 2) >> 8;
> > +        s->cam[index][4] = dp8393x_get(s, width, data, 3) & 0xff;
> > +        s->cam[index][5] = dp8393x_get(s, width, data, 3) >> 8;
>
> The general pattern with the code in this device seems to be
> "load sizeof(uint16_t) * width * N bytes; then look at
> data[i * width], possibly with an endianness flip", where
> width is either 1 or 2 depending on a config register.
>
> Maybe it would be clearer if the data was loaded by doing a
> loop of N iterations of address_space_ldl_le / address_space_ldw_le /
> address_space_ldl_be / address_space_ldw_be ? Then you're doing
> something more like what the device is doing (ie a series of
> bus accesses of a particular width and endianness), and the
> handling of width and endianness is in the code doing the data
> loads rather than in the code that is manipulating the loaded
> data.
>
> Incidentally, the only other use of this device is in
> the mips 'jazz' boards. Those seem to be compiled for both
> mips64el and mips64 -- does anybody know if it actually
> works correctly in both those configurations ?

dp8393x in mips64el works well. mips64 is not testable.

Hervé