| Message ID | 20200305105325.31264-1-kuhn.chenqun@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] hw/net/imx_fec: write TGSR and TCSR3 in imx_enet_write() | expand |
>-----Original Message----- >From: Chenqun (kuhn) >Sent: Thursday, March 5, 2020 6:53 PM >To: qemu-devel@nongnu.org; qemu-trivial@nongnu.org >Cc: peter.maydell@linaro.org; Zhanghailiang ><zhang.zhanghailiang@huawei.com>; jasowang@redhat.com; >peter.chubb@nicta.com.au; qemu-arm@nongnu.org; Chenqun (kuhn) ><kuhn.chenqun@huawei.com>; Euler Robot <euler.robot@huawei.com> >Subject: [PATCH v2] hw/net/imx_fec: write TGSR and TCSR3 in >imx_enet_write() > >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> >--- >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; > In "i.MX 6Dual/6Quad Applications Processor Reference Manual" documentation, the register 'ENET_TGSR' all fields TFn is write-1-to-clear. It is described in detail as follows： ------------------------------------------------------- Field Description ------------------------------------------------------- 31–4 This field is reserved. ------------------------------------------------------- 3 Copy Of Timer Flag For Channel 3 TF3 0 Timer Flag for Channel 3 is clear 1 Timer Flag for Channel 3 is set ------------------------------------------------------- 2 Copy Of Timer Flag For Channel 2 TF2 0 Timer Flag for Channel 2 is clear 1 Timer Flag for Channel 2 is set ------------------------------------------------------- 1 Copy Of Timer Flag For Channel 1 TF1 0 Timer Flag for Channel 1 is clear 1 Timer Flag for Channel 1 is set ------------------------------------------------------- 0 Copy Of Timer Flag For Channel 0 TF0 0 Timer Flag for Channel 0 is clear 1 Timer Flag for Channel 0 is set ------------------------------------------------------ > 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); > The ENET_TCSRn field descriptions: ------------------------------------------------------- Field Description ------------------------------------------------------- 31–8 This field is reserved. ------------------------------------------------------ 7 Timer Flag TF Sets when input capture or output compare occurs. This flag is double buffered between the module clock and 1588 clock domains. When this field is 1, it can be cleared to 0 by writing 1to it. 0 Input Capture or Output Compare has not occurred 1 Input Capture or Output Compare has occurred -------------------------------------------------------- 6 Timer Interrupt Enable TIE 0 Interrupt is disabled 1 Interrupt is enabled -------------------------------------------------------- 2-5 Timer Mode .................. -------------------------------------------------------- 1 This field is reserved. -------------------------------------------------------- 0 Timer DMA Request Enable TDRE 0 DMA request is disabled 1 DMA request is enabled -------------------------------------------------------- > break; > case ENET_TCCR0: > case ENET_TCCR1: >-- >2.23.0 >
Patchew URL: https://patchew.org/QEMU/20200305105325.31264-1-kuhn.chenqun@huawei.com/ Hi, This series failed the docker-quick@centos7 build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash make docker-image-centos7 V=1 NETWORK=1 time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1 === TEST SCRIPT END === Submodule 'slirp' (https://git.qemu.org/git/libslirp.git) registered for path 'slirp' Cloning into 'slirp'... remote: Counting objects: 3095, done. error: RPC failed; result=18, HTTP code = 200 fatal: The remote end hung up unexpectedly fatal: protocol error: bad pack header Clone of 'https://git.qemu.org/git/libslirp.git' into submodule path 'slirp' failed failed to update submodule slirp Cleared directory 'dtc' Submodule 'dtc' (https://git.qemu.org/git/dtc.git) unregistered for path 'dtc' Submodule 'slirp' (https://git.qemu.org/git/libslirp.git) unregistered for path 'slirp' make[1]: *** [/var/tmp/patchew-tester-tmp-vmdpiqef/src/docker-src.2020-03-05-06.41.02.14569] Error 1 make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-vmdpiqef/src' make: *** [docker-run-test-quick@centos7] Error 2 real 9m36.630s user 0m4.117s The full log is available at http://patchew.org/logs/20200305105325.31264-1-kuhn.chenqun@huawei.com/testing.docker-quick@centos7/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
Patchew URL: https://patchew.org/QEMU/20200305105325.31264-1-kuhn.chenqun@huawei.com/ Hi, This series failed the docker-mingw@fedora build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #! /bin/bash export ARCH=x86_64 make docker-image-fedora V=1 NETWORK=1 time make docker-test-mingw@fedora J=14 NETWORK=1 === TEST SCRIPT END === Submodule 'dtc' (https://git.qemu.org/git/dtc.git) registered for path 'dtc' Cloning into 'dtc'... remote: Counting objects: 5394, done. error: RPC failed; result=18, HTTP code = 200 fatal: The remote end hung up unexpectedly fatal: protocol error: bad pack header Clone of 'https://git.qemu.org/git/dtc.git' into submodule path 'dtc' failed failed to update submodule dtc Submodule 'dtc' (https://git.qemu.org/git/dtc.git) unregistered for path 'dtc' make[1]: *** [/var/tmp/patchew-tester-tmp-u0wjl6xm/src/docker-src.2020-03-05-06.51.13.15558] Error 1 make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-u0wjl6xm/src' make: *** [docker-run-test-mingw@fedora] Error 2 real 4m2.749s user 0m2.356s The full log is available at http://patchew.org/logs/20200305105325.31264-1-kuhn.chenqun@huawei.com/testing.docker-mingw@fedora/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com
On Thu, 5 Mar 2020 at 10:53, Chen Qun <kuhn.chenqun@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.robot@huawei.com> > Signed-off-by: Chen Qun <kuhn.chenqun@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 */ If some but not all bits are W1C, as for TCSR*: s->regs[index] &= ~(value & 0x80); /* W1C bits */ s->regs[index] &= ~0x7d; /* writable fields */ s->regs[index] |= (value & 0x7d); thanks -- PMM
>-----Original Message----- >From: Peter Maydell [mailto:peter.maydell@linaro.org] >Sent: Monday, March 9, 2020 7:36 PM >To: Chenqun (kuhn) <kuhn.chenqun@huawei.com> >Cc: QEMU Developers <qemu-devel@nongnu.org>; QEMU Trivial <qemu- >trivial@nongnu.org>; Zhanghailiang <zhang.zhanghailiang@huawei.com>; >Jason Wang <jasowang@redhat.com>; Peter Chubb ><peter.chubb@nicta.com.au>; qemu-arm <qemu-arm@nongnu.org>; Euler >Robot <euler.robot@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.chenqun@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.robot@huawei.com> >> Signed-off-by: Chen Qun <kuhn.chenqun@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
On Tue, 10 Mar 2020 at 08:08, Chenqun (kuhn) <kuhn.chenqun@huawei.com> wrote: > > >-----Original Message----- > >From: Peter Maydell [mailto:peter.maydell@linaro.org] > >> > >> 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
>-----Original Message----- >From: Peter Maydell [mailto:peter.maydell@linaro.org] >Sent: Friday, March 13, 2020 1:01 AM >To: Chenqun (kuhn) <kuhn.chenqun@huawei.com> >Cc: QEMU Developers <qemu-devel@nongnu.org>; QEMU Trivial <qemu- >trivial@nongnu.org>; Zhanghailiang <zhang.zhanghailiang@huawei.com>; >Jason Wang <jasowang@redhat.com>; Peter Chubb ><peter.chubb@nicta.com.au>; qemu-arm <qemu-arm@nongnu.org>; Euler >Robot <euler.robot@huawei.com> >Subject: Re: [PATCH v2] hw/net/imx_fec: write TGSR and TCSR3 in >imx_enet_write() > >On Tue, 10 Mar 2020 at 08:08, Chenqun (kuhn) <kuhn.chenqun@huawei.com> >wrote: >> >> >-----Original Message----- >> >From: Peter Maydell [mailto:peter.maydell@linaro.org] >> >> >> >> 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. > OK, let's keep original code style, and clear reserved bit. I will provide v3 version for it. Thanks.
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:
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> --- 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(-)