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

[Philippe Mathieu-Daudé]

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 <address@hidden>
> ---
>   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;
>               }