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 :)
