| Message ID | 20200225025923.19328-1-kuhn.chenqun@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/net/imx_fec: write TGSR and TCSR3 in imx_enet_write() | expand |
On 2020/2/25 上午10:59, Chen Qun 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.robot@huawei.com> > Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com> > --- > I'm not sure if this modification is correct, just from the function > definition, it is correct. > --- > hw/net/imx_fec.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c > index 6a124a154a..92f6215712 100644 > --- a/hw/net/imx_fec.c > +++ b/hw/net/imx_fec.c > @@ -855,13 +855,13 @@ 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] = value & 0x0000000f; > break; > case ENET_TCSR0: > case ENET_TCSR1: > case ENET_TCSR2: > case ENET_TCSR3: > - value = value & 0x000000fd; > + s->regs[index] = value & 0x000000fd; > break; > case ENET_TCCR0: > case ENET_TCCR1: Applied. Thanks
On Tue, 25 Feb 2020 at 05:41, Jason Wang <jasowang@redhat.com> wrote: > > > On 2020/2/25 上午10:59, Chen Qun 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.robot@huawei.com> > > Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com> > > --- > > I'm not sure if this modification is correct, just from the function > > definition, it is correct. > > --- > > hw/net/imx_fec.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c > > index 6a124a154a..92f6215712 100644 > > --- a/hw/net/imx_fec.c > > +++ b/hw/net/imx_fec.c > > @@ -855,13 +855,13 @@ 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] = value & 0x0000000f; > > break; Hi; the datasheet for this SoC says that these bits of the register are write-1-to-clear, so while this is definitely a bug I don't think this is the right fix. > > case ENET_TCSR0: > > case ENET_TCSR1: > > case ENET_TCSR2: > > case ENET_TCSR3: > > - value = value & 0x000000fd; > > + s->regs[index] = value & 0x000000fd; > > break; Here bit 7 is write-1-to-clear, though bits 0 and 2..5 are simple write-the-value. > > case ENET_TCCR0: > > case ENET_TCCR1: > > > Applied. Could you drop this from your queue, please? thanks -- PMM
On 2020/2/25 下午6:18, Peter Maydell wrote: > On Tue, 25 Feb 2020 at 05:41, Jason Wang <jasowang@redhat.com> wrote: >> >> On 2020/2/25 上午10:59, Chen Qun 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.robot@huawei.com> >>> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com> >>> --- >>> I'm not sure if this modification is correct, just from the function >>> definition, it is correct. >>> --- >>> hw/net/imx_fec.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c >>> index 6a124a154a..92f6215712 100644 >>> --- a/hw/net/imx_fec.c >>> +++ b/hw/net/imx_fec.c >>> @@ -855,13 +855,13 @@ 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] = value & 0x0000000f; >>> break; > Hi; the datasheet for this SoC says that these bits > of the register are write-1-to-clear, so while this > is definitely a bug I don't think this is the right fix. > >>> case ENET_TCSR0: >>> case ENET_TCSR1: >>> case ENET_TCSR2: >>> case ENET_TCSR3: >>> - value = value & 0x000000fd; >>> + s->regs[index] = value & 0x000000fd; >>> break; > Here bit 7 is write-1-to-clear, though bits 0 and > 2..5 are simple write-the-value. > >>> case ENET_TCCR0: >>> case ENET_TCCR1: >> >> Applied. > Could you drop this from your queue, please? > > thanks > -- PMM Sure, Chen please send V2 to address Peter's comment. Thanks
>-----Original Message----- >From: Jason Wang [mailto:jasowang@redhat.com] >Sent: Wednesday, February 26, 2020 11:03 AM >To: Peter Maydell <peter.maydell@linaro.org> >Cc: Chenqun (kuhn) <kuhn.chenqun@huawei.com>; QEMU Developers ><qemu-devel@nongnu.org>; QEMU Trivial <qemu-trivial@nongnu.org>; >Zhanghailiang <zhang.zhanghailiang@huawei.com>; Peter Chubb ><peter.chubb@nicta.com.au>; qemu-arm <qemu-arm@nongnu.org> >Subject: Re: [PATCH] hw/net/imx_fec: write TGSR and TCSR3 in >imx_enet_write() > > >On 2020/2/25 下午6:18, Peter Maydell wrote: >> On Tue, 25 Feb 2020 at 05:41, Jason Wang <jasowang@redhat.com> wrote: >>> >>> On 2020/2/25 上午10:59, Chen Qun 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.robot@huawei.com> >>>> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com> >>>> --- >>>> I'm not sure if this modification is correct, just from the function >>>> definition, it is correct. >>>> --- >>>> hw/net/imx_fec.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c index >>>> 6a124a154a..92f6215712 100644 >>>> --- a/hw/net/imx_fec.c >>>> +++ b/hw/net/imx_fec.c >>>> @@ -855,13 +855,13 @@ 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] = value & 0x0000000f; >>>> break; >> Hi; the datasheet for this SoC says that these bits of the register >> are write-1-to-clear, so while this is definitely a bug I don't think >> this is the right fix. >> >>>> case ENET_TCSR0: >>>> case ENET_TCSR1: >>>> case ENET_TCSR2: >>>> case ENET_TCSR3: >>>> - value = value & 0x000000fd; >>>> + s->regs[index] = value & 0x000000fd; >>>> break; >> Here bit 7 is write-1-to-clear, though bits 0 and >> 2..5 are simple write-the-value. >> >>>> case ENET_TCCR0: >>>> case ENET_TCCR1: >>> >>> Applied. >> Could you drop this from your queue, please? >> >> thanks >> -- PMM > > >Sure, Chen please send V2 to address Peter's comment. OK, but I didn't find the datasheet that contains these two registers description. Could someone provide me with a connection for the datasheet ?
diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c index 6a124a154a..92f6215712 100644 --- a/hw/net/imx_fec.c +++ b/hw/net/imx_fec.c @@ -855,13 +855,13 @@ 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] = value & 0x0000000f; break; case ENET_TCSR0: case ENET_TCSR1: case ENET_TCSR2: case ENET_TCSR3: - value = value & 0x000000fd; + s->regs[index] = value & 0x000000fd; break; case ENET_TCCR0: case ENET_TCCR1:
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.robot@huawei.com> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com> --- I'm not sure if this modification is correct, just from the function definition, it is correct. --- hw/net/imx_fec.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)