Re: [PATCH 1/4] dp8393x: don't force 32-bit register access

[Finn Thain]

On Mon, 5 Jul 2021, Mark Cave-Ayland wrote:

> Commit 3fe9a838ec "dp8393x: Always use 32-bit accesses" set 
> .impl.min_access_size
> and .impl.max_access_size to 4 to try and fix the Linux jazzsonic driver 
> which uses
> 32-bit accesses.
> 
> The problem with forcing the register access to 32-bit in this way is that 
> since the
> dp8393x uses 16-bit registers, a manual endian swap is required for devices 
> on big
> endian machines with 32-bit accesses.
> 
> For both access sizes and machine endians the QEMU memory API can do the 
> right thing
> automatically: all that is needed is to set .impl.min_access_size to 2 to 
> declare that
> the dp8393x implements 16-bit registers.
> 
> Normally .impl.max_access_size should also be set to 2, however that doesn't 
> quite
> work in this case since the register stride is specified using a (dynamic) 
> it_shift
> property which is applied during the MMIO access itself. The effect of this 
> is that
> for a 32-bit access the memory API performs 2 x 16-bit accesses, but the use 
> of
> it_shift within the MMIO access itself causes the register value to be 
> repeated in both
> the top 16-bits and bottom 16-bits. The Linux jazzsonic driver expects the 
> stride to be
> zero-extended up to access size and therefore fails to correctly detect the 
> dp8393x
> device due to the extra data in the top 16-bits.
> 
> The solution here is to remove .impl.max_access_size so that the memory API 
> will
> correctly zero-extend the 16-bit registers to the access size up to and 
> including
> it_shift. Since it_shift is never greater than 2 than this will always do the 
> right
> thing for both 16-bit and 32-bit accesses regardless of the machine endian, 
> allowing
> the manual endian swap code to be removed.
> 

IIUC, this patch replaces an explicit word swap with an implicit byte 
swap. The explicit word swap was conditional on the big_endian flag.

This flag seems to work like the chip's BMODE pin which switches between 
Intel and Motorola bus modes (not just byte ordering but bus signalling in 
general). The BMODE pin or big_endian flag should effect a byte swap not a 
word swap so there must be a bug though it's not clear how that manifests.

Regardless of this patch, the big_endian flag also controls byte swapping 
during DMA by the device. IIUC, the flag is set to indicate that RAM is 
big_endian, so it's not actually a property of the dp8393x but of the 
RAM...

The Magnum hardware can run in big endian or little endian mode. But the 
SONIC chip must remain in little endian mode always because asserting 
BMODE would invoke Motorola signalling and that would contradict 
Philippe's datasheet which says that the SONIC device is attached to an 
"i386 compatible bus".

This seems contrary to mips_jazz_init(), which sets the dp8393x big_endian 
flag whenever TARGET_WORDS_BIGENDIAN is defined, i.e. risc/os guest. 

QEMU's dp8393x device has native endianness, so perhaps a big endian guest 
or a big endian host could trigger the bug that's being addressed in this 
patch.

Anyway, I think that this patch is heading in the right direction but 
can't it go further? Shouldn't the big_endian flag disappear altogether so 
that the memory API can also take care of the byte swapping needed by 
dp8393x_get() and dp8393x_put() for DMA?

> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Fixes: 3fe9a838ec ("dp8393x: Always use 32-bit accesses")
> ---
>  hw/net/dp8393x.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 11810c9b60..44a1955015 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -602,15 +602,14 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr, 
> unsigned int size)
>  
>      trace_dp8393x_read(reg, reg_names[reg], val, size);
>  
> -    return s->big_endian ? val << 16 : val;
> +    return val;
>  }
>  
> -static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
> +static void dp8393x_write(void *opaque, hwaddr addr, uint64_t val,
>                            unsigned int size)
>  {
>      dp8393xState *s = opaque;
>      int reg = addr >> s->it_shift;
> -    uint32_t val = s->big_endian ? data >> 16 : data;
>  
>      trace_dp8393x_write(reg, reg_names[reg], val, size);
>  
> @@ -691,11 +690,16 @@ static void dp8393x_write(void *opaque, hwaddr addr, 
> uint64_t data,
>      }
>  }
>  
> +/*
> + * Since .impl.max_access_size is effectively controlled by the it_shift
> + * property, leave it unspecified for now to allow the memory API to
> + * correctly zero extend the 16-bit register values to the access size up to 
> and
> + * including it_shift.
> + */
>  static const MemoryRegionOps dp8393x_ops = {
>      .read = dp8393x_read,
>      .write = dp8393x_write,
> -    .impl.min_access_size = 4,
> -    .impl.max_access_size = 4,
> +    .impl.min_access_size = 2,
>      .endianness = DEVICE_NATIVE_ENDIAN,
>  };
>  
>