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 <address@hidden> wrote:
> On 4 February 2018 at 20:41, Richard Braun <address@hidden> 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 <address@hidden>
>> ---
>>  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
>