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. 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 ) Thanks, Laurent