Re: [PATCH v3 01/14] dp8393x: Mask EOL bit from descriptor addresses

[Finn Thain <fthain@telegraphics.com.au>]

[-- Attachment #1: Type: text/plain, Size: 4623 bytes --]

On Tue, 28 Jan 2020, Philippe Mathieu-Daudé wrote:

> Hi Finn,
> 
> On 1/19/20 11:59 PM, Finn Thain wrote:
> > The Least Significant bit of a descriptor address register is used as
> > an EOL flag. It has to be masked when the register value is to be used
> > as an actual address 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>
> > Tested-by: Laurent Vivier <laurent@vivier.eu>
> > ---
> > Changed since v1:
> >   - Added macros to name constants as requested by Philippe Mathieu-Daudé.
> > ---
> >   hw/net/dp8393x.c | 19 ++++++++++++-------
> >   1 file changed, 12 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> > index cdc2631c0c..14901c1445 100644
> > --- a/hw/net/dp8393x.c
> > +++ b/hw/net/dp8393x.c
> > @@ -145,6 +145,9 @@ do { printf("sonic ERROR: %s: " fmt, __func__ , ##
> > __VA_ARGS__); } while (0)
> >   #define SONIC_ISR_PINT   0x0800
> >   #define SONIC_ISR_LCD    0x1000
> >   +#define SONIC_DESC_EOL   0x0001
> > +#define SONIC_DESC_ADDR  0xFFFE
> 
> I'd rather not add SONIC_DESC_ADDR and use ~SONIC_DESC_EOL instead.
> 
> Please consider it if you respin the series.
> 

I chose to use 0xFFFE instead of ~SONIC_DESC_EOL because the former 
correctly implies an unsigned short word, while the latter mask may 
suggest some need for sign extension or longer words.

I agree that ~SONIC_DESC_EOL is easily understood as "all the other bits". 
But the bits in SONIC_DESC_EOL will never change, since this value is 
dictated by the hardware.

> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 

Thanks for reviewing this series.

> > +
> >   #define TYPE_DP8393X "dp8393x"
> >   #define DP8393X(obj) OBJECT_CHECK(dp8393xState, (obj), TYPE_DP8393X)
> >   @@ -197,7 +200,8 @@ 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] & SONIC_DESC_ADDR);
> >   }
> >     static uint32_t dp8393x_rbwc(dp8393xState *s)
> > @@ -217,7 +221,8 @@ 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] & SONIC_DESC_ADDR);
> >   }
> >     static uint32_t dp8393x_wt(dp8393xState *s)
> > @@ -506,8 +511,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] & SONIC_DESC_EOL) {
> >                   /* EOL detected */
> >                   break;
> >               }
> > @@ -763,13 +768,13 @@ static ssize_t dp8393x_receive(NetClientState *nc,
> > const uint8_t * buf,
> >       /* XXX: Check byte ordering */
> >         /* Check for EOL */
> > -    if (s->regs[SONIC_LLFA] & 0x1) {
> > +    if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
> >           /* Are we still in resource exhaustion? */
> >           size = sizeof(uint16_t) * 1 * width;
> >           address = dp8393x_crda(s) + sizeof(uint16_t) * 5 * width;
> >           address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> >                            (uint8_t *)s->data, size, 0);
> > -        if (dp8393x_get(s, width, 0) & 0x1) {
> > +        if (dp8393x_get(s, width, 0) & SONIC_DESC_EOL) {
> >               /* Still EOL ; stop reception */
> >               return -1;
> >           } else {
> > @@ -827,7 +832,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
> > uint8_t * buf,
> >       address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 5 *
> > width,
> >           MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> >       s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
> > -    if (s->regs[SONIC_LLFA] & 0x1) {
> > +    if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
> >           /* EOL detected */
> >           s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
> >       } else {
> > 
> 
>