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

[Chenqun (kuhn)]

>-----Original Message-----
>From: Peter Maydell [mailto:address@hidden]
>Sent: Monday, March 9, 2020 7:36 PM
>To: Chenqun (kuhn) <address@hidden>
>Cc: QEMU Developers <address@hidden>; QEMU Trivial <qemu-
>address@hidden>; Zhanghailiang <address@hidden>;
>Jason Wang <address@hidden>; Peter Chubb
><address@hidden>; qemu-arm <address@hidden>; Euler
>Robot <address@hidden>
>Subject: Re: [PATCH v2] hw/net/imx_fec: write TGSR and TCSR3 in
>imx_enet_write()
>
>On Thu, 5 Mar 2020 at 10:53, Chen Qun <address@hidden> wrote:
>>
>> The current code causes clang static code analyzer generate warning:
>> hw/net/imx_fec.c:858:9: warning: Value stored to 'value' is never read
>>         value = value & 0x0000000f;
>>         ^       ~~~~~~~~~~~~~~~~~~
>> hw/net/imx_fec.c:864:9: warning: Value stored to 'value' is never read
>>         value = value & 0x000000fd;
>>         ^       ~~~~~~~~~~~~~~~~~~
>>
>> According to the definition of the function, the two “value”
>> assignments  should be written to registers.
>>
>> Reported-by: Euler Robot <address@hidden>
>> Signed-off-by: Chen Qun <address@hidden>
>> ---
>> v1->v2:
>>   The register 'ENET_TGSR' write-1-to-clear timer flag.
>>   The register 'ENET_TCSRn' 7bit(TF) write-1-to-clear timer flag.
>> ---
>>  hw/net/imx_fec.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> 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 ?

We see that other registers have explicitly cleared the reserved bit,  which 
also meets the I.MX datasheet requirements.

s->regs[index] &= ~(value & 0xf) & 0xf;    /* 0-3 bits W1C, 4-31 reserved  bits 
write zero */

>If some but not all bits are W1C, as for TCSR*:
>
Yes,  this patch is  just only W1C for 7bit, not all bits for TCSRn.
But do we need clear the reserved bit (31 - 8 bits) explicitly ?

>   s->regs[index] &= ~(value & 0x80); /* W1C bits */

s->regs[index] &= ~(value & 0x80)   & 0xff ; /* 7 bits  W1C,  8-31 reserved  
bits write zero */

>   s->regs[index] &= ~0x7d; /* writable fields */
>   s->regs[index] |= (value & 0x7d); 
>
>thanks
>-- PMM