Re: [Qemu-devel] [RFC v3 09/10] dp8393x: manage big endian bus

["Hervé Poussineau" <hpoussin@reactos.org>]

Le 08/07/2018 à 23:11, Peter Maydell a écrit :
> On 28 June 2018 at 00:29, Laurent Vivier <laurent@vivier.eu> wrote:
>> This is needed by Quadra 800, this card can run on little-endian
>> or big-endian bus.
>>
>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>> Tested-by: Hervé Poussineau <hpoussin@reactos.org>
>> ---
>>   hw/net/dp8393x.c | 88 ++++++++++++++++++++++++++++++++++++--------------------
>>   1 file changed, 57 insertions(+), 31 deletions(-)
>>
>> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
>> index f2d2ce344c..62adff9ba3 100644
>> --- a/hw/net/dp8393x.c
>> +++ b/hw/net/dp8393x.c
>> @@ -150,6 +150,7 @@ typedef struct dp8393xState {
>>
>>       /* Hardware */
>>       uint8_t it_shift;
>> +    bool big_endian;
>>       qemu_irq irq;
>>   #ifdef DEBUG_SONIC
>>       int irq_level;
>> @@ -220,6 +221,29 @@ static uint32_t dp8393x_wt(dp8393xState *s)
>>       return s->regs[SONIC_WT1] << 16 | s->regs[SONIC_WT0];
>>   }
>>
>> +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;
>> +}
>> +
>> +static void dp8393x_put(dp8393xState *s, int width, uint16_t *base, int offset,
>> +                        uint16_t val)
>> +{
>> +    if (s->big_endian) {
>> +        base[offset * width + width - 1] = cpu_to_be16(val);
>> +    } else {
>> +        base[offset * width] = cpu_to_le16(val);
>> +    }
>> +}
>> +
>>   static void dp8393x_update_irq(dp8393xState *s)
>>   {
>>       int level = (s->regs[SONIC_IMR] & s->regs[SONIC_ISR]) ? 1 : 0;
>> @@ -251,12 +275,12 @@ static void dp8393x_do_load_cam(dp8393xState *s)
>>           /* Fill current entry */
>>           address_space_rw(&s->as, dp8393x_cdp(s),
>>               MEMTXATTRS_UNSPECIFIED, (uint8_t *)data, size, 0);
>> -        s->cam[index][0] = data[1 * width] & 0xff;
>> -        s->cam[index][1] = data[1 * width] >> 8;
>> -        s->cam[index][2] = data[2 * width] & 0xff;
>> -        s->cam[index][3] = data[2 * width] >> 8;
>> -        s->cam[index][4] = data[3 * width] & 0xff;
>> -        s->cam[index][5] = data[3 * width] >> 8;
>> +        s->cam[index][0] = dp8393x_get(s, width, data, 1) & 0xff;
>> +        s->cam[index][1] = dp8393x_get(s, width, data, 1) >> 8;
>> +        s->cam[index][2] = dp8393x_get(s, width, data, 2) & 0xff;
>> +        s->cam[index][3] = dp8393x_get(s, width, data, 2) >> 8;
>> +        s->cam[index][4] = dp8393x_get(s, width, data, 3) & 0xff;
>> +        s->cam[index][5] = dp8393x_get(s, width, data, 3) >> 8;
> 
> The general pattern with the code in this device seems to be
> "load sizeof(uint16_t) * width * N bytes; then look at
> data[i * width], possibly with an endianness flip", where
> width is either 1 or 2 depending on a config register.
> 
> Maybe it would be clearer if the data was loaded by doing a
> loop of N iterations of address_space_ldl_le / address_space_ldw_le /
> address_space_ldl_be / address_space_ldw_be ? Then you're doing
> something more like what the device is doing (ie a series of
> bus accesses of a particular width and endianness), and the
> handling of width and endianness is in the code doing the data
> loads rather than in the code that is manipulating the loaded
> data.
> 
> Incidentally, the only other use of this device is in
> the mips 'jazz' boards. Those seem to be compiled for both
> mips64el and mips64 -- does anybody know if it actually
> works correctly in both those configurations ?

dp8393x in mips64el works well. mips64 is not testable.

Hervé