On 6/26/19 12:11 PM, Laurent Vivier wrote: > Le 26/06/2019 à 10:57, Philippe Mathieu-Daudé a écrit : >> On 6/25/19 7:09 PM, Laurent Vivier wrote: >>> Le 25/06/2019 à 17:57, Philippe Mathieu-Daudé a écrit : >>>> On 6/24/19 10:07 PM, Laurent Vivier wrote: >>>>> Hi, >>>>> >>>>> Jason, Can I have an Acked-by from you (as network devices maintainer)? >>>> >>>> Hmm something seems odd here indeed... >>>> >>>> What a stable model! This file has no logical modification since its >>>> introduction, a65f56eeba "Implement sonic netcard (MIPS Jazz)" >>>> >>>> Here we had: >>>> >>>> static void dp8393x_writeb(void *opaque, hwaddr addr, uint32_t val) >>>> { >>>> uint16_t old_val = dp8393x_readw(opaque, addr & ~0x1); >>>> >>>> switch (addr & 3) { >>>> case 0: >>>> val = val | (old_val & 0xff00); >>>> break; >>>> case 1: >>>> val = (val << 8) | (old_val & 0x00ff); >>>> break; >>>> } >>>> dp8393x_writew(opaque, addr & ~0x1, val); >>>> } >>>> >>>> So we had 16-bit endian shifting there. >>>> >>>> And few lines below: >>>> >>>> /* XXX: Check byte ordering */ >>>> ... >>>> /* Calculate the ethernet checksum */ >>>> #ifdef SONIC_CALCULATE_RXCRC >>>> checksum = cpu_to_le32(crc32(0, buf, rx_len)); >>>> #else >>>> checksum = 0; >>>> #endif >>>> >>>> After various housekeeping, we get: >>>> >>>> 84689cbb97 "net/dp8393x: do not use old_mmio accesses" >>>> >>>> The MIPS Jazz is known to run in both endianess, but I haven't checked >>>> if at that time both were available. >>>> >>>> Have you tried this patch? >>>> >>>> -- >8 -- >>>> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c >>>> index bdb0b3b2c2..646e11206f 100644 >>>> @@ -651,7 +651,7 @@ static const MemoryRegionOps dp8393x_ops = { >>>> .write = dp8393x_write, >>>> .impl.min_access_size = 2, >>>> .impl.max_access_size = 2, >>>> - .endianness = DEVICE_NATIVE_ENDIAN, >>>> + .endianness = DEVICE_LITTLE_ENDIAN, >>>> }; >>>> --- >>>> >>>> (but then mips64-softmmu Jazz would have networking broken). >>>> >>> >>> I doesn't help, the endianness is a MemoryRegion property (see >>> memory_region_wrong_endianness()) so it is used when the CPU writes to >>> the device MMIO, not when the device accesses the other memory. >>> In this case, it reads from system_memory. Perhaps we can create the >>> address_space with a system_memory in big endian mode? >> >> Ah I missed that... >> >> What about not using address_space_rw(data) but directly use >> address_space_lduw_le() and address_space_stw_le() instead? >> > > It's more complicated than that, because access size depends on a > register value: > > 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; > } > > and width is: > > width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1; > > So in the end we always need the big_endian flag to know how to read the > memory. I think it's simpler to read/write the memory (like a real DMA > access), and then to swap data internally.
Fair enough. My R-b tag stands anyway :) > Moreover, the big-endian/little-endian is a real feature of the > controller (see 1.3 DATA WIDTH AND BYTE ORDERING, > http://pccomponents.com/datasheets/NSC83932.PDF ) Can you (or the maintainer taking this series) amend this information to your commit? Thanks for the info provided in this thread, Phil.