[Qemu-devel] [PATCH] hw/char/stm32f2xx_usart: improve TXE/TC bit handling

[Qemu-devel] [PATCH] hw/char/stm32f2xx_usart: improve TXE/TC bit handling

[Richard Braun]

Consider that data is always immediately sent. As a result, keep
the SR_TXE and SR_TC bits always set. In addition, fix the reset value
of the USART status register.

Signed-off-by: Richard Braun <rbraun@sceen.net>
---
 hw/char/stm32f2xx_usart.c         | 4 ----
 include/hw/char/stm32f2xx_usart.h | 7 ++++++-
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/char/stm32f2xx_usart.c b/hw/char/stm32f2xx_usart.c
index 07b462d4b6..a914f98a2a 100644
--- a/hw/char/stm32f2xx_usart.c
+++ b/hw/char/stm32f2xx_usart.c
@@ -96,12 +96,10 @@ static uint64_t stm32f2xx_usart_read(void *opaque, hwaddr addr,
     switch (addr) {
     case USART_SR:
         retvalue = s->usart_sr;
-        s->usart_sr &= ~USART_SR_TC;
         qemu_chr_fe_accept_input(&s->chr);
         return retvalue;
     case USART_DR:
         DB_PRINT("Value: 0x%" PRIx32 ", %c\n", s->usart_dr, (char) s->usart_dr);
-        s->usart_sr |= USART_SR_TXE;
         s->usart_sr &= ~USART_SR_RXNE;
         qemu_chr_fe_accept_input(&s->chr);
         qemu_set_irq(s->irq, 0);
@@ -151,8 +149,6 @@ static void stm32f2xx_usart_write(void *opaque, hwaddr addr,
             /* XXX this blocks entire thread. Rewrite to use
              * qemu_chr_fe_write and background I/O callbacks */
             qemu_chr_fe_write_all(&s->chr, &ch, 1);
-            s->usart_sr |= USART_SR_TC;
-            s->usart_sr &= ~USART_SR_TXE;
         }
         return;
     case USART_BRR:
diff --git a/include/hw/char/stm32f2xx_usart.h b/include/hw/char/stm32f2xx_usart.h
index 9d03a7527c..bbba3965a1 100644
--- a/include/hw/char/stm32f2xx_usart.h
+++ b/include/hw/char/stm32f2xx_usart.h
@@ -37,7 +37,12 @@
 #define USART_CR3  0x14
 #define USART_GTPR 0x18
 
-#define USART_SR_RESET 0x00C00000
+/*
+ * XXX The reset value mentioned in 24.6.1 Status register seems bogus.
+ * Looking at Table 98 USART register map and reset values, it seems it
+ * should be 0xc0, and that's how real hardware behaves.
+ */
+#define USART_SR_RESET (USART_SR_TXE | USART_SR_TC)
 
 #define USART_SR_TXE  (1 << 7)
 #define USART_SR_TC   (1 << 6)
-- 
2.11.0

Re: [Qemu-devel] [PATCH] hw/char/stm32f2xx_usart: improve TXE/TC bit handling

[Peter Maydell]

On 4 February 2018 at 20:41, Richard Braun <rbraun@sceen.net> wrote:
> Consider that data is always immediately sent. As a result, keep
> the SR_TXE and SR_TC bits always set. In addition, fix the reset value
> of the USART status register.

Do you know what the data sheet means when it says that TC
can be cleared by "a read from the USART_SR register followed
by a write to the USART_DR register" ?

If we supported interrupts properly (which we don't seem to)
I suspect we'd need something more than "TXE and TC are always set",
or the guest would probably never clear the TXE and TC interrupts.

cc'ing Alistair, who wrote the stm32 USART code.

> Signed-off-by: Richard Braun <rbraun@sceen.net>
> ---
>  hw/char/stm32f2xx_usart.c         | 4 ----
>  include/hw/char/stm32f2xx_usart.h | 7 ++++++-
>  2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/hw/char/stm32f2xx_usart.c b/hw/char/stm32f2xx_usart.c
> index 07b462d4b6..a914f98a2a 100644
> --- a/hw/char/stm32f2xx_usart.c
> +++ b/hw/char/stm32f2xx_usart.c
> @@ -96,12 +96,10 @@ static uint64_t stm32f2xx_usart_read(void *opaque, hwaddr addr,
>      switch (addr) {
>      case USART_SR:
>          retvalue = s->usart_sr;
> -        s->usart_sr &= ~USART_SR_TC;
>          qemu_chr_fe_accept_input(&s->chr);
>          return retvalue;
>      case USART_DR:
>          DB_PRINT("Value: 0x%" PRIx32 ", %c\n", s->usart_dr, (char) s->usart_dr);
> -        s->usart_sr |= USART_SR_TXE;
>          s->usart_sr &= ~USART_SR_RXNE;
>          qemu_chr_fe_accept_input(&s->chr);
>          qemu_set_irq(s->irq, 0);
> @@ -151,8 +149,6 @@ static void stm32f2xx_usart_write(void *opaque, hwaddr addr,
>              /* XXX this blocks entire thread. Rewrite to use
>               * qemu_chr_fe_write and background I/O callbacks */
>              qemu_chr_fe_write_all(&s->chr, &ch, 1);
> -            s->usart_sr |= USART_SR_TC;
> -            s->usart_sr &= ~USART_SR_TXE;

The guest can clear the TC and TXE bits by writing to the USART_SR
directly, so this code should set both of them, I think ?

>          }
>          return;
>      case USART_BRR:
> diff --git a/include/hw/char/stm32f2xx_usart.h b/include/hw/char/stm32f2xx_usart.h
> index 9d03a7527c..bbba3965a1 100644
> --- a/include/hw/char/stm32f2xx_usart.h
> +++ b/include/hw/char/stm32f2xx_usart.h
> @@ -37,7 +37,12 @@
>  #define USART_CR3  0x14
>  #define USART_GTPR 0x18
>
> -#define USART_SR_RESET 0x00C00000
> +/*
> + * XXX The reset value mentioned in 24.6.1 Status register seems bogus.
> + * Looking at Table 98 USART register map and reset values, it seems it
> + * should be 0xc0, and that's how real hardware behaves.
> + */
> +#define USART_SR_RESET (USART_SR_TXE | USART_SR_TC)
>
>  #define USART_SR_TXE  (1 << 7)
>  #define USART_SR_TC   (1 << 6)

Yep, I agree that the previous reset value was wrong.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] hw/char/stm32f2xx_usart: improve TXE/TC bit handling

[Alistair Francis]

On Thu, Feb 8, 2018 at 6:58 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 4 February 2018 at 20:41, Richard Braun <rbraun@sceen.net> wrote:
>> Consider that data is always immediately sent. As a result, keep
>> the SR_TXE and SR_TC bits always set. In addition, fix the reset value
>> of the USART status register.
>
> Do you know what the data sheet means when it says that TC
> can be cleared by "a read from the USART_SR register followed
> by a write to the USART_DR register" ?

I'm not sure if they have to be consecutive or as long as they
eventually happen it's fine.

I no longer have HW to test on so it's hard to say.

>
> If we supported interrupts properly (which we don't seem to)
> I suspect we'd need something more than "TXE and TC are always set",
> or the guest would probably never clear the TXE and TC interrupts.
>
> cc'ing Alistair, who wrote the stm32 USART code.
>
>> Signed-off-by: Richard Braun <rbraun@sceen.net>
>> ---
>>  hw/char/stm32f2xx_usart.c         | 4 ----
>>  include/hw/char/stm32f2xx_usart.h | 7 ++++++-
>>  2 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/char/stm32f2xx_usart.c b/hw/char/stm32f2xx_usart.c
>> index 07b462d4b6..a914f98a2a 100644
>> --- a/hw/char/stm32f2xx_usart.c
>> +++ b/hw/char/stm32f2xx_usart.c
>> @@ -96,12 +96,10 @@ static uint64_t stm32f2xx_usart_read(void *opaque, hwaddr addr,
>>      switch (addr) {
>>      case USART_SR:
>>          retvalue = s->usart_sr;
>> -        s->usart_sr &= ~USART_SR_TC;

This does seem to be the wrong behavior. It should be cleared after
writing to the USART_DR register (and after this read).

>>          qemu_chr_fe_accept_input(&s->chr);
>>          return retvalue;
>>      case USART_DR:
>>          DB_PRINT("Value: 0x%" PRIx32 ", %c\n", s->usart_dr, (char) s->usart_dr);
>> -        s->usart_sr |= USART_SR_TXE;

Doesn't software expect this to be set? Maybe this was just a nasty
workaround to ensure it was set at some point.

>>          s->usart_sr &= ~USART_SR_RXNE;
>>          qemu_chr_fe_accept_input(&s->chr);
>>          qemu_set_irq(s->irq, 0);
>> @@ -151,8 +149,6 @@ static void stm32f2xx_usart_write(void *opaque, hwaddr addr,
>>              /* XXX this blocks entire thread. Rewrite to use
>>               * qemu_chr_fe_write and background I/O callbacks */
>>              qemu_chr_fe_write_all(&s->chr, &ch, 1);
>> -            s->usart_sr |= USART_SR_TC;
>> -            s->usart_sr &= ~USART_SR_TXE;
>
> The guest can clear the TC and TXE bits by writing to the USART_SR
> directly, so this code should set both of them, I think ?

Shouldn't this clear both?

TXE: "It is cleared by a write to the USART_DR register."
TC: "a read from the USART_SR register followed by a write to the
USART_DR register"

I guess the TC should only clear after a read though.

>
>>          }
>>          return;
>>      case USART_BRR:
>> diff --git a/include/hw/char/stm32f2xx_usart.h b/include/hw/char/stm32f2xx_usart.h
>> index 9d03a7527c..bbba3965a1 100644
>> --- a/include/hw/char/stm32f2xx_usart.h
>> +++ b/include/hw/char/stm32f2xx_usart.h
>> @@ -37,7 +37,12 @@
>>  #define USART_CR3  0x14
>>  #define USART_GTPR 0x18
>>
>> -#define USART_SR_RESET 0x00C00000
>> +/*
>> + * XXX The reset value mentioned in 24.6.1 Status register seems bogus.
>> + * Looking at Table 98 USART register map and reset values, it seems it
>> + * should be 0xc0, and that's how real hardware behaves.
>> + */
>> +#define USART_SR_RESET (USART_SR_TXE | USART_SR_TC)
>>
>>  #define USART_SR_TXE  (1 << 7)
>>  #define USART_SR_TC   (1 << 6)
>
> Yep, I agree that the previous reset value was wrong.

Ah, that is confusing that it's different in different places. The new
one looks fine though.

Alistair

>
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH] hw/char/stm32f2xx_usart: improve TXE/TC bit handling

[Richard Braun]

On Thu, Feb 08, 2018 at 02:58:29PM +0000, Peter Maydell wrote:
> On 4 February 2018 at 20:41, Richard Braun <rbraun@sceen.net> wrote:
> > Consider that data is always immediately sent. As a result, keep
> > the SR_TXE and SR_TC bits always set. In addition, fix the reset value
> > of the USART status register.
> 
> Do you know what the data sheet means when it says that TC
> can be cleared by "a read from the USART_SR register followed
> by a write to the USART_DR register" ?

It's meant for software either polling the TC bit or waiting for a
transmission complete interrupt. Once the bit is seen, the next
transmission automatically clears it.

> If we supported interrupts properly (which we don't seem to)
> I suspect we'd need something more than "TXE and TC are always set",
> or the guest would probably never clear the TXE and TC interrupts.

That's the idea of the patch. Since everything is currently synchronous,
there is no need for these transient states to even exist.

> The guest can clear the TC and TXE bits by writing to the USART_SR
> directly, so this code should set both of them, I think ?

Right, nice catch.

-- 
Richard Braun

Re: [Qemu-devel] [PATCH] hw/char/stm32f2xx_usart: improve TXE/TC bit handling

[Richard Braun]

On Fri, Feb 09, 2018 at 10:23:13AM +0100, Richard Braun wrote:
> On Thu, Feb 08, 2018 at 02:58:29PM +0000, Peter Maydell wrote:
> > The guest can clear the TC and TXE bits by writing to the USART_SR
> > directly, so this code should set both of them, I think ?
> 
> Right, nice catch.

Although, after reading the manual again, TXE is actually read-only and
only set by hardware.

-- 
Richard Braun

Re: [Qemu-devel] [PATCH] hw/char/stm32f2xx_usart: improve TXE/TC bit handling

[Richard Braun]

On Thu, Feb 08, 2018 at 03:33:32PM -0800, Alistair Francis wrote:
> >> @@ -96,12 +96,10 @@ static uint64_t stm32f2xx_usart_read(void *opaque, hwaddr addr,
> >>      switch (addr) {
> >>      case USART_SR:
> >>          retvalue = s->usart_sr;
> >> -        s->usart_sr &= ~USART_SR_TC;
> 
> This does seem to be the wrong behavior. It should be cleared after
> writing to the USART_DR register (and after this read).

Writing to DR being synchronous, the transmission is complete immediately
after doing it, making that bit set again.

> >>      case USART_DR:
> >>          DB_PRINT("Value: 0x%" PRIx32 ", %c\n", s->usart_dr, (char) s->usart_dr);
> >> -        s->usart_sr |= USART_SR_TXE;
> 
> Doesn't software expect this to be set? Maybe this was just a nasty
> workaround to ensure it was set at some point.

This bit should always be set in the current implementation. It may be
cleared and set the day I/O threads are used, since then, the transient
state will be visible to software.

> >> @@ -151,8 +149,6 @@ static void stm32f2xx_usart_write(void *opaque, hwaddr addr,
> >>              /* XXX this blocks entire thread. Rewrite to use
> >>               * qemu_chr_fe_write and background I/O callbacks */
> >>              qemu_chr_fe_write_all(&s->chr, &ch, 1);
> >> -            s->usart_sr |= USART_SR_TC;
> >> -            s->usart_sr &= ~USART_SR_TXE;
> >
> > The guest can clear the TC and TXE bits by writing to the USART_SR
> > directly, so this code should set both of them, I think ?
> 
> Shouldn't this clear both?
> 
> TXE: "It is cleared by a write to the USART_DR register."
> TC: "a read from the USART_SR register followed by a write to the
> USART_DR register"
> 
> I guess the TC should only clear after a read though.

We're not aiming at a full emulation of the device here, we only want to
emulate visible states. As mentioned above, software should always see
the TXE bit set, since writes are synchronous. TC should be set though,
because it may be cleared by an explicit write to SR setting it to 0.

I'll post a new version of the patch soon.

-- 
Richard Braun

[Qemu-devel] [PATCH v2] hw/char/stm32f2xx_usart: fix TXE/TC bit handling

[Richard Braun]

I/O currently being synchronous, there is no reason to ever clear the
SR_TXE bit. However the SR_TC bit may be cleared by software writing
to the SR register, so set it on each write.

In addition, fix the reset value of the USART status register.

Signed-off-by: Richard Braun <rbraun@sceen.net>
---
 hw/char/stm32f2xx_usart.c         | 12 ++++++++----
 include/hw/char/stm32f2xx_usart.h |  7 ++++++-
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/hw/char/stm32f2xx_usart.c b/hw/char/stm32f2xx_usart.c
index 07b462d4b6..032b5fda13 100644
--- a/hw/char/stm32f2xx_usart.c
+++ b/hw/char/stm32f2xx_usart.c
@@ -96,12 +96,10 @@ static uint64_t stm32f2xx_usart_read(void *opaque, hwaddr addr,
     switch (addr) {
     case USART_SR:
         retvalue = s->usart_sr;
-        s->usart_sr &= ~USART_SR_TC;
         qemu_chr_fe_accept_input(&s->chr);
         return retvalue;
     case USART_DR:
         DB_PRINT("Value: 0x%" PRIx32 ", %c\n", s->usart_dr, (char) s->usart_dr);
-        s->usart_sr |= USART_SR_TXE;
         s->usart_sr &= ~USART_SR_RXNE;
         qemu_chr_fe_accept_input(&s->chr);
         qemu_set_irq(s->irq, 0);
@@ -137,7 +135,9 @@ static void stm32f2xx_usart_write(void *opaque, hwaddr addr,
     switch (addr) {
     case USART_SR:
         if (value <= 0x3FF) {
-            s->usart_sr = value;
+            /* I/O being synchronous, TXE is always set. In addition, it may
+               only be set by hardware, so keep it set here. */
+            s->usart_sr = value | USART_SR_TXE;
         } else {
             s->usart_sr &= value;
         }
@@ -151,8 +151,12 @@ static void stm32f2xx_usart_write(void *opaque, hwaddr addr,
             /* XXX this blocks entire thread. Rewrite to use
              * qemu_chr_fe_write and background I/O callbacks */
             qemu_chr_fe_write_all(&s->chr, &ch, 1);
+            /* XXX I/O are currently synchronous, making it impossible for
+               software to observe transient states where TXE or TC aren't
+               set. Unlike TXE however, which is read-only, software may
+               clear TC by writing 0 to the SR register, so set it again
+               on each write. */
             s->usart_sr |= USART_SR_TC;
-            s->usart_sr &= ~USART_SR_TXE;
         }
         return;
     case USART_BRR:
diff --git a/include/hw/char/stm32f2xx_usart.h b/include/hw/char/stm32f2xx_usart.h
index 9d03a7527c..7ea7448813 100644
--- a/include/hw/char/stm32f2xx_usart.h
+++ b/include/hw/char/stm32f2xx_usart.h
@@ -37,7 +37,12 @@
 #define USART_CR3  0x14
 #define USART_GTPR 0x18
 
-#define USART_SR_RESET 0x00C00000
+/*
+ * XXX The reset value mentioned in "24.6.1 Status register" seems bogus.
+ * Looking at "Table 98 USART register map and reset values", it seems it
+ * should be 0xc0, and that's how real hardware behaves.
+ */
+#define USART_SR_RESET (USART_SR_TXE | USART_SR_TC)
 
 #define USART_SR_TXE  (1 << 7)
 #define USART_SR_TC   (1 << 6)
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2] hw/char/stm32f2xx_usart: fix TXE/TC bit handling

[Alistair Francis]

On Tue, Feb 13, 2018 at 12:54 PM, Richard Braun <rbraun@sceen.net> wrote:
> I/O currently being synchronous, there is no reason to ever clear the
> SR_TXE bit. However the SR_TC bit may be cleared by software writing
> to the SR register, so set it on each write.
>
> In addition, fix the reset value of the USART status register.
>
> Signed-off-by: Richard Braun <rbraun@sceen.net>

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Alistair

> ---
>  hw/char/stm32f2xx_usart.c         | 12 ++++++++----
>  include/hw/char/stm32f2xx_usart.h |  7 ++++++-
>  2 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/hw/char/stm32f2xx_usart.c b/hw/char/stm32f2xx_usart.c
> index 07b462d4b6..032b5fda13 100644
> --- a/hw/char/stm32f2xx_usart.c
> +++ b/hw/char/stm32f2xx_usart.c
> @@ -96,12 +96,10 @@ static uint64_t stm32f2xx_usart_read(void *opaque, hwaddr addr,
>      switch (addr) {
>      case USART_SR:
>          retvalue = s->usart_sr;
> -        s->usart_sr &= ~USART_SR_TC;
>          qemu_chr_fe_accept_input(&s->chr);
>          return retvalue;
>      case USART_DR:
>          DB_PRINT("Value: 0x%" PRIx32 ", %c\n", s->usart_dr, (char) s->usart_dr);
> -        s->usart_sr |= USART_SR_TXE;
>          s->usart_sr &= ~USART_SR_RXNE;
>          qemu_chr_fe_accept_input(&s->chr);
>          qemu_set_irq(s->irq, 0);
> @@ -137,7 +135,9 @@ static void stm32f2xx_usart_write(void *opaque, hwaddr addr,
>      switch (addr) {
>      case USART_SR:
>          if (value <= 0x3FF) {
> -            s->usart_sr = value;
> +            /* I/O being synchronous, TXE is always set. In addition, it may
> +               only be set by hardware, so keep it set here. */
> +            s->usart_sr = value | USART_SR_TXE;
>          } else {
>              s->usart_sr &= value;
>          }
> @@ -151,8 +151,12 @@ static void stm32f2xx_usart_write(void *opaque, hwaddr addr,
>              /* XXX this blocks entire thread. Rewrite to use
>               * qemu_chr_fe_write and background I/O callbacks */
>              qemu_chr_fe_write_all(&s->chr, &ch, 1);
> +            /* XXX I/O are currently synchronous, making it impossible for
> +               software to observe transient states where TXE or TC aren't
> +               set. Unlike TXE however, which is read-only, software may
> +               clear TC by writing 0 to the SR register, so set it again
> +               on each write. */
>              s->usart_sr |= USART_SR_TC;
> -            s->usart_sr &= ~USART_SR_TXE;
>          }
>          return;
>      case USART_BRR:
> diff --git a/include/hw/char/stm32f2xx_usart.h b/include/hw/char/stm32f2xx_usart.h
> index 9d03a7527c..7ea7448813 100644
> --- a/include/hw/char/stm32f2xx_usart.h
> +++ b/include/hw/char/stm32f2xx_usart.h
> @@ -37,7 +37,12 @@
>  #define USART_CR3  0x14
>  #define USART_GTPR 0x18
>
> -#define USART_SR_RESET 0x00C00000
> +/*
> + * XXX The reset value mentioned in "24.6.1 Status register" seems bogus.
> + * Looking at "Table 98 USART register map and reset values", it seems it
> + * should be 0xc0, and that's how real hardware behaves.
> + */
> +#define USART_SR_RESET (USART_SR_TXE | USART_SR_TC)
>
>  #define USART_SR_TXE  (1 << 7)
>  #define USART_SR_TC   (1 << 6)
> --
> 2.11.0
>
>

Re: [Qemu-devel] [PATCH v2] hw/char/stm32f2xx_usart: fix TXE/TC bit handling

[Peter Maydell]

On 15 February 2018 at 22:27, Alistair Francis <alistair23@gmail.com> wrote:
> On Tue, Feb 13, 2018 at 12:54 PM, Richard Braun <rbraun@sceen.net> wrote:
>> I/O currently being synchronous, there is no reason to ever clear the
>> SR_TXE bit. However the SR_TC bit may be cleared by software writing
>> to the SR register, so set it on each write.
>>
>> In addition, fix the reset value of the USART status register.
>>
>> Signed-off-by: Richard Braun <rbraun@sceen.net>
>
> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>



Applied to target-arm.next, thanks.

-- PMM