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

[Peter Maydell <peter.maydell@linaro.org>]

On 28 June 2018 at 00:29, Laurent Vivier <laurent@vivier.eu> wrote:
> This is needed by Quadra 800, this card can run on little-endian
> or big-endian bus.
>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> Tested-by: Hervé Poussineau <hpoussin@reactos.org>
> ---
>  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 ?

thanks
-- PMM