Re: [Qemu-devel] [PATCH v8 03/10] dp8393x: manage big endian bus

[Philippe Mathieu-Daudé]

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.