| Message ID | 7d220205700c43b15d6ae6cefd6520a97c763709.1576286757.git.fthain@telegraphics.com.au |
|---|---|
| State | New |
| Headers | show |
| Series | Fixes for DP8393X SONIC device emulation | expand |
Hi Finn, On 12/14/19 2:25 AM, Finn Thain wrote: > The LSB of descriptor address registers is used as an EOL flag. > It has to be masked when those registers are to be used as actual > addresses for copying memory around. But when the registers are > to be updated the EOL bit should not be masked. > > Signed-off-by: Finn Thain <fthain@telegraphics.com.au> > --- > hw/net/dp8393x.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c > index 3d991af163..164311c055 100644 > --- a/hw/net/dp8393x.c > +++ b/hw/net/dp8393x.c > @@ -197,7 +197,7 @@ static uint32_t dp8393x_crba(dp8393xState *s) > > static uint32_t dp8393x_crda(dp8393xState *s) > { > - return (s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA]; > + return (s->regs[SONIC_URDA] << 16) | (s->regs[SONIC_CRDA] & 0xfffe); > } > > static uint32_t dp8393x_rbwc(dp8393xState *s) > @@ -217,7 +217,7 @@ static uint32_t dp8393x_tsa(dp8393xState *s) > > static uint32_t dp8393x_ttda(dp8393xState *s) > { > - return (s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA]; > + return (s->regs[SONIC_UTDA] << 16) | (s->regs[SONIC_TTDA] & 0xfffe); > } > > static uint32_t dp8393x_wt(dp8393xState *s) > @@ -506,8 +506,8 @@ static void dp8393x_do_transmit_packets(dp8393xState *s) > sizeof(uint16_t) * > (4 + 3 * s->regs[SONIC_TFC]) * width, > MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0); > - s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0) & ~0x1; > - if (dp8393x_get(s, width, 0) & 0x1) { > + s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0); > + if (s->regs[SONIC_CTDA] & 0x1) { Can you add a definition for the EOL bit and use it, instead of these magic 0x1/0xfffe values? That way the meaning will be obvious for future reviewers. Thanks, Phil. > /* EOL detected */ > break; > } >
On Sat, 14 Dec 2019, Philippe Mathieu-Daud? wrote: > Hi Finn, > > On 12/14/19 2:25 AM, Finn Thain wrote: > > The LSB of descriptor address registers is used as an EOL flag. > > It has to be masked when those registers are to be used as actual > > addresses for copying memory around. But when the registers are > > to be updated the EOL bit should not be masked. > > > > Signed-off-by: Finn Thain <fthain@telegraphics.com.au> > > --- > > hw/net/dp8393x.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c > > index 3d991af163..164311c055 100644 > > --- a/hw/net/dp8393x.c > > +++ b/hw/net/dp8393x.c > > @@ -197,7 +197,7 @@ static uint32_t dp8393x_crba(dp8393xState *s) > > static uint32_t dp8393x_crda(dp8393xState *s) > > { > > - return (s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA]; > > + return (s->regs[SONIC_URDA] << 16) | (s->regs[SONIC_CRDA] & 0xfffe); > > } > > static uint32_t dp8393x_rbwc(dp8393xState *s) > > @@ -217,7 +217,7 @@ static uint32_t dp8393x_tsa(dp8393xState *s) > > static uint32_t dp8393x_ttda(dp8393xState *s) > > { > > - return (s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA]; > > + return (s->regs[SONIC_UTDA] << 16) | (s->regs[SONIC_TTDA] & 0xfffe); > > } > > static uint32_t dp8393x_wt(dp8393xState *s) > > @@ -506,8 +506,8 @@ static void dp8393x_do_transmit_packets(dp8393xState *s) > > sizeof(uint16_t) * > > (4 + 3 * s->regs[SONIC_TFC]) * width, > > MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0); > > - s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0) & ~0x1; > > - if (dp8393x_get(s, width, 0) & 0x1) { > > + s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0); > > + if (s->regs[SONIC_CTDA] & 0x1) { > > Can you add a definition for the EOL bit and use it, instead of these magic > 0x1/0xfffe values? That way the meaning will be obvious for future reviewers. > Sure. I'll prepare v2. Thanks for your review.
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c index 3d991af163..164311c055 100644 --- a/hw/net/dp8393x.c +++ b/hw/net/dp8393x.c @@ -197,7 +197,7 @@ static uint32_t dp8393x_crba(dp8393xState *s) static uint32_t dp8393x_crda(dp8393xState *s) { - return (s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA]; + return (s->regs[SONIC_URDA] << 16) | (s->regs[SONIC_CRDA] & 0xfffe); } static uint32_t dp8393x_rbwc(dp8393xState *s) @@ -217,7 +217,7 @@ static uint32_t dp8393x_tsa(dp8393xState *s) static uint32_t dp8393x_ttda(dp8393xState *s) { - return (s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA]; + return (s->regs[SONIC_UTDA] << 16) | (s->regs[SONIC_TTDA] & 0xfffe); } static uint32_t dp8393x_wt(dp8393xState *s) @@ -506,8 +506,8 @@ static void dp8393x_do_transmit_packets(dp8393xState *s) sizeof(uint16_t) * (4 + 3 * s->regs[SONIC_TFC]) * width, MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0); - s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0) & ~0x1; - if (dp8393x_get(s, width, 0) & 0x1) { + s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0); + if (s->regs[SONIC_CTDA] & 0x1) { /* EOL detected */ break; }
The LSB of descriptor address registers is used as an EOL flag. It has to be masked when those registers are to be used as actual addresses for copying memory around. But when the registers are to be updated the EOL bit should not be masked. Signed-off-by: Finn Thain <fthain@telegraphics.com.au> --- hw/net/dp8393x.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)