| Message ID | 1517776881-14115-1-git-send-email-rbraun@sceen.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
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.
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.
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.
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)
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(-)