Re: [PATCH v2 08/10] dp8393x: don't force 32-bit register access

[Philippe Mathieu-Daudé]

On 7/3/21 4:16 PM, Mark Cave-Ayland wrote:
> On 03/07/2021 14:10, Philippe Mathieu-Daudé wrote:
>> On 7/3/21 2:04 PM, Mark Cave-Ayland wrote:

>>> I've tested this under Linux/m68k, NetBSD/arc and MacOS and networking
>>> seems fine after a quick test in each OS. The slight curiosity is that
>>> the 4 byte accesses used by MacOS are split into 2 x 2 byte accesses,
>>> but since MacOS only uses the bottom 16-bit of the result and ignores
>>> the top 16-bits then despite there being 2 accesses, everything still
>>> works as expected (see below).
>>>
>>>
>>> READ:
>>>
>>> dp8393x_read reg=0x28 [SR] val=0x0004 size=2
>>> memory_region_ops_read cpu 0 mr 0x55f210f44830 addr 0x5000a0a0 value 0x4
>>> size 2
>>> dp8393x_read reg=0x28 [SR] val=0x0004 size=2
>>> memory_region_ops_read cpu 0 mr 0x55f210f44830 addr 0x5000a0a2 value 0x4
>>> size 2
>>> memory_region_ops_read cpu 0 mr 0x55f210c9c330 addr 0x50f0a0a0 value
>>> 0x40004 size 4
>>>
>>> WRITE:
>>>
>>> memory_region_ops_write cpu 0 mr 0x55f210c9c330 addr 0x50f0a01c value
>>> 0x248fe8 size 4
>>> memory_region_ops_write cpu 0 mr 0x55f210f44830 addr 0x5000a01c value
>>> 0x24 size 2
>>> dp8393x_write reg=0x7 [CTDA] val=0x0024 size=2
>>> memory_region_ops_write cpu 0 mr 0x55f210f44830 addr 0x5000a01e value
>>> 0x8fe8 size 2
>>> dp8393x_write reg=0x7 [CTDA] val=0x8fe8 size=2
>>>
>>>
>>> If you're happy with this, I'll resubmit a revised version of the patch
>>> but with an updated commit message: the Fixes: tag is still the same,
>>> but the updated fix is to ensure that .impl.min_access_size and
>>> .impl.max_access_size match the real-life 16-bit register size.
>>
>> Hold on a bit more, I'm still reviewing the datasheet ;)
>>
>> My code note so far (untested) are:
>>
>> -- >8 --
>> @@ -219,34 +225,48 @@ 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, int offset)
>> +static uint16_t dp8393x_get(dp8393xState *s, hwaddr addr, unsigned
>> ofs16)
>>   {
>> +    const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
>>       uint16_t val;
>>
>> -    if (s->big_endian) {
>> -        val = be16_to_cpu(s->data[offset * width + width - 1]);
>> +    if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
>> +        addr += 2 * ofs16;
>> +        if (s->big_endian) {
>> +            val = address_space_ldl_be(&s->as, addr, attrs, NULL);
>> +        } else {
>> +            val = address_space_ldl_le(&s->as, addr, attrs, NULL);
>> +        }
>>       } else {
>> -        val = le16_to_cpu(s->data[offset * width]);
>> +        addr += 1 * ofs16;
>> +        if (s->big_endian) {
>> +            val = address_space_lduw_be(&s->as, addr, attrs, NULL);
>> +        } else {
>> +            val = address_space_lduw_le(&s->as, addr, attrs, NULL);
>> +        }
>>       }
>> +
>>       return val;
>>   }

>>       /* Check link field */
>> -    size = sizeof(uint16_t) * width;
>> -    address_space_read(&s->as,
>> -                       dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
>> -                       MEMTXATTRS_UNSPECIFIED, s->data, size);
>> -    s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
>> +    s->regs[SONIC_LLFA] = dp8393x_get(s, dp8393x_crda(s), 5);
>>       if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
>>           /* EOL detected */
>>           s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
>>       } else {
>>           /* Clear in_use */
>> -        size = sizeof(uint16_t) * width;
>> -        address = dp8393x_crda(s) + sizeof(uint16_t) * 6 * width;
>> -        dp8393x_put(s, width, 0, 0);
>> -        address_space_write(&s->as, address, MEMTXATTRS_UNSPECIFIED,
>> -                            s->data, size);
>> +        dp8393x_put(s, dp8393x_crda(s), 6, 0x0000);
>>
>>           /* Move to next descriptor */
>>           s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
>> ---
>>
>> Now I'll look at the CAM then the cksum.
> 
> I see, so you're also looking to consolidate all the address space
> accesses via the dp8393x_get() and dp8393x_put() functions instead of
> using s->data as an intermediatery - that makes sense to me. Once you
> have a patch that works for MIPS let me know and I can give a test on
> Linux/m68k and MacOS :)

Done and sent. Thanks for the testing :)