diff mbox series

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

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

Commit Message

Chenqun (kuhn) March 5, 2020, 10:53 a.m. UTC
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(-)

Comments

Chenqun (kuhn) March 5, 2020, 11:04 a.m. UTC | #1
>-----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
>
no-reply@patchew.org March 5, 2020, 11:50 a.m. UTC | #2
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
no-reply@patchew.org March 5, 2020, 11:55 a.m. UTC | #3
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
Peter Maydell March 9, 2020, 11:35 a.m. UTC | #4
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
Chenqun (kuhn) March 10, 2020, 8:08 a.m. UTC | #5
>-----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
Peter Maydell March 12, 2020, 5 p.m. UTC | #6
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
Chenqun (kuhn) March 13, 2020, 2:08 a.m. UTC | #7
>-----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 mbox series

Patch

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: