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?