[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [PATCH v2] hw/net/imx_fec: write TGSR and TCSR3 in imx_enet_write()
From: |
Chenqun (kuhn) |
Subject: |
RE: [PATCH v2] hw/net/imx_fec: write TGSR and TCSR3 in imx_enet_write() |
Date: |
Tue, 10 Mar 2020 08:08:17 +0000 |
>-----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