On Thu, Feb 8, 2018 at 6:58 AM, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 4 February 2018 at 20:41, Richard Braun <rbr...@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 <rbr...@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
>
