On 07/07/2021 00:51, Finn Thain wrote:
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?
Currently in QEMU the dp8393x device is connected directly to the system bus i.e. in
effect the CPU. The CPU issues either a big-endian or little-endian access, the
device declares what it would like to receive, and the memory API handles all the
intermediate layers. In this case DEVICE_NATIVE_ENDIAN specifies it expects to
receive accesses in the same endian as the machine and so everything "just works".
For this reason MMIO accesses generally shouldn't need any explicit byte swapping: if
you're doing this then it's a good indicator that something is wrong. There are some
special cases around for things like virtio, but we can ignore those for now.
The dp8393x_get() and dp8393x_put() functions are for bus master accesses from the
device to the RAM, and these accesses are controlled by the "big_endian" property and
the DW bit. As you point out the "big_endian" property is effectively the BMODE bit:
in my datasheet I see nothing about this pin changing the signalling, only that it
affects the byte ordering for RAM accesses.
Now it is highly likely that the BMODE bit should match the target endian, in which
case we could eliminate the "big_endian" property as you suggest. However this
conflicts with what you mention above that the SONIC is hard-coded into little-endian
mode, in which case we would still need to keep it.
I've sent a fix for the CAP registers, but other than that I'm happy that the first
patchset works fine here, and the consolidation of most of the endian swaps is a good
tidy-up. If this works for you then I suggest we merge the first patchset including
the CAP fix in time for freeze next week and go from there.
Certainly we can look to improve things in the future, but without anyone having a
working big-endian MIPS image to test against, I don't think it's worth guessing what
changes are required as we can easily double the length of this thread and still have
no idea if any changes we've made are correct.
ATB,
Mark.
