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