Re: [PATCH v2] hw/net/imx_fec: write TGSR and TCSR3 in imx_enet_write()

[Peter Maydell]

On Tue, 10 Mar 2020 at 08:08, Chenqun (kuhn) <address@hidden> wrote:
>
> >-----Original Message-----
> >From: Peter Maydell [mailto:address@hidden]
> >>
> >> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c index
> >> 6a124a154a..322cbdcc17 100644
> >> --- a/hw/net/imx_fec.c
> >> +++ b/hw/net/imx_fec.c
> >> @@ -855,13 +855,15 @@ static void imx_enet_write(IMXFECState *s,
> >uint32_t index, uint32_t value)
> >>          break;
> >>      case ENET_TGSR:
> >>          /* implement clear timer flag */
> >> -        value = value & 0x0000000f;
> >> +        s->regs[index] ^= s->regs[index] & value;
> >> +        s->regs[index] &= 0x0000000f;
> >>          break;
> >>      case ENET_TCSR0:
> >>      case ENET_TCSR1:
> >>      case ENET_TCSR2:
> >>      case ENET_TCSR3:
> >> -        value = value & 0x000000fd;
> >> +        s->regs[index] = (value & 0x00000080) ? (0x0000007d & value) :
> >> +                         (value & 0x000000fd);
> >>          break;
> >>      case ENET_TCCR0:
> >>      case ENET_TCCR1:
> >
> >This isn't the usual way to write W1C behaviour.
> >If all the relevant bits are W1C, as for TGSR:
> >
> >   s->regs[index] &= ~(value & 0xf); /* all bits W1C */
> >
> Yes, it looks better.
> But do we need clear the reserved bit (31 - 4 bits) explicitly ?

Not necessarily, but it seems to be how the other registers
in the device have generally been coded, and it's clearly
what the intent was here given that the original (buggy)
code was masking out reserved bits. So I think it makes
sense to continue in that style.

thanks
-- PMM