>-----Original Message----- >From: Peter Maydell [mailto:peter.mayd...@linaro.org] >Sent: Monday, March 9, 2020 7:36 PM >To: Chenqun (kuhn) <kuhn.chen...@huawei.com> >Cc: QEMU Developers <qemu-devel@nongnu.org>; QEMU Trivial <qemu- >triv...@nongnu.org>; Zhanghailiang <zhang.zhanghaili...@huawei.com>; >Jason Wang <jasow...@redhat.com>; Peter Chubb ><peter.ch...@nicta.com.au>; qemu-arm <qemu-...@nongnu.org>; Euler >Robot <euler.ro...@huawei.com> >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 <kuhn.chen...@huawei.com> 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 <euler.ro...@huawei.com> >> Signed-off-by: Chen Qun <kuhn.chen...@huawei.com> >> --- >> 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
