From: Hans-Erik Floryd <hans-erik.floryd@rt-labs.com>
To: "Philippe Mathieu-Daudé" <philmd@linaro.org>
Cc: "Alistair Francis" <alistair@alistair23.me>,
"Peter Maydell" <peter.maydell@linaro.org>,
"Marc-André Lureau" <marcandre.lureau@redhat.com>,
"Paolo Bonzini" <pbonzini@redhat.com>,
"open list:STM32F205" <qemu-arm@nongnu.org>,
"open list:All patches CC here" <qemu-devel@nongnu.org>
Subject: Re: [PATCH 1/1] stm32f2xx_usart: implement TX interrupts
Date: Fri, 20 Oct 2023 16:40:14 +0200 [thread overview]
Message-ID: <CADS1RdQLmbdobcAsO+DM79N2TgaErCwrctATXNf6a7VYT6hBqA@mail.gmail.com> (raw)
In-Reply-To: <74d96828-dae9-64fe-5947-ba83ce54206d@linaro.org>
Hi Phil,
On Fri, 20 Oct 2023 at 14:42, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi Hans-Erik,
>
> On 20/10/23 13:14, Hans-Erik Floryd wrote:
> > Generate interrupt if either of the TXE, TC or RXNE bits are active
> > and the corresponding interrupt enable bit is set.
> >
> > Signed-off-by: Hans-Erik Floryd <hans-erik.floryd@rt-labs.com>
> > ---
> > hw/char/stm32f2xx_usart.c | 29 +++++++++++++++++------------
> > include/hw/char/stm32f2xx_usart.h | 10 ++++++----
> > 2 files changed, 23 insertions(+), 16 deletions(-)
> >
> > diff --git a/hw/char/stm32f2xx_usart.c b/hw/char/stm32f2xx_usart.c
> > index fde67f4f03..2947c3a260 100644
> > --- a/hw/char/stm32f2xx_usart.c
> > +++ b/hw/char/stm32f2xx_usart.c
> > @@ -53,6 +53,16 @@ static int stm32f2xx_usart_can_receive(void *opaque)
> > return 0;
> > }
> >
> > +static void stm32f2xx_update(STM32F2XXUsartState *s)
> > +{
> > + uint32_t mask = s->usart_sr & s->usart_cr1;
> > + if (mask & (USART_SR_TXE | USART_SR_TC | USART_SR_RXNE)) {
> > + qemu_set_irq(s->irq, 1);
> > + } else {
> > + qemu_set_irq(s->irq, 0);
> > + }
> > +}
> > +
> > static void stm32f2xx_usart_receive(void *opaque, const uint8_t *buf, int size)
> > {
> > STM32F2XXUsartState *s = opaque;
> > @@ -66,9 +76,7 @@ static void stm32f2xx_usart_receive(void *opaque, const uint8_t *buf, int size)
> > s->usart_dr = *buf;
> > s->usart_sr |= USART_SR_RXNE;
> >
> > - if (s->usart_cr1 & USART_CR1_RXNEIE) {
> > - qemu_set_irq(s->irq, 1);
> > - }
> > + stm32f2xx_update(s);
> >
> > DB_PRINT("Receiving: %c\n", s->usart_dr);
> > }
> > @@ -85,7 +93,7 @@ static void stm32f2xx_usart_reset(DeviceState *dev)
> > s->usart_cr3 = 0x00000000;
> > s->usart_gtpr = 0x00000000;
> >
> > - qemu_set_irq(s->irq, 0);
> > + stm32f2xx_update(s);
> > }
> >
> > static uint64_t stm32f2xx_usart_read(void *opaque, hwaddr addr,
> > @@ -100,13 +108,14 @@ static uint64_t stm32f2xx_usart_read(void *opaque, hwaddr addr,
> > case USART_SR:
> > retvalue = s->usart_sr;
> > qemu_chr_fe_accept_input(&s->chr);
> > + stm32f2xx_update(s);
> > return retvalue;
> > case USART_DR:
> > DB_PRINT("Value: 0x%" PRIx32 ", %c\n", s->usart_dr, (char) s->usart_dr);
> > retvalue = s->usart_dr & 0x3FF;
> > s->usart_sr &= ~USART_SR_RXNE;
> > qemu_chr_fe_accept_input(&s->chr);
> > - qemu_set_irq(s->irq, 0);
> > + stm32f2xx_update(s);
> > return retvalue;
> > case USART_BRR:
> > return s->usart_brr;
> > @@ -145,9 +154,7 @@ static void stm32f2xx_usart_write(void *opaque, hwaddr addr,
> > } else {
> > s->usart_sr &= value;
> > }
> > - if (!(s->usart_sr & USART_SR_RXNE)) {
> > - qemu_set_irq(s->irq, 0);
> > - }
> > + stm32f2xx_update(s);
> > return;
> > case USART_DR:
> > if (value < 0xF000) {
> > @@ -161,6 +168,7 @@ static void stm32f2xx_usart_write(void *opaque, hwaddr addr,
> > clear TC by writing 0 to the SR register, so set it again
> > on each write. */
> > s->usart_sr |= USART_SR_TC;
> > + stm32f2xx_update(s);
> > }
> > return;
> > case USART_BRR:
> > @@ -168,10 +176,7 @@ static void stm32f2xx_usart_write(void *opaque, hwaddr addr,
> > return;
> > case USART_CR1:
> > s->usart_cr1 = value;
> > - if (s->usart_cr1 & USART_CR1_RXNEIE &&
> > - s->usart_sr & USART_SR_RXNE) {
> > - qemu_set_irq(s->irq, 1);
> > - }
> > + stm32f2xx_update(s);
> > return;
> > case USART_CR2:
> > s->usart_cr2 = value;
> > diff --git a/include/hw/char/stm32f2xx_usart.h b/include/hw/char/stm32f2xx_usart.h
> > index 65bcc85470..fdfa7424a7 100644
> > --- a/include/hw/char/stm32f2xx_usart.h
> > +++ b/include/hw/char/stm32f2xx_usart.h
> > @@ -48,10 +48,12 @@
> > #define USART_SR_TC (1 << 6)
> > #define USART_SR_RXNE (1 << 5)
> >
> > -#define USART_CR1_UE (1 << 13)
> > -#define USART_CR1_RXNEIE (1 << 5)
> > -#define USART_CR1_TE (1 << 3)
> > -#define USART_CR1_RE (1 << 2)
> > +#define USART_CR1_UE (1 << 13)
> > +#define USART_CR1_TXEIE (1 << 7)
> > +#define USART_CR1_TCEIE (1 << 6)
> > +#define USART_CR1_RXNEIE (1 << 5)
> > +#define USART_CR1_TE (1 << 3)
> > +#define USART_CR1_RE (1 << 2)
> >
> > #define TYPE_STM32F2XX_USART "stm32f2xx-usart"
> > OBJECT_DECLARE_SIMPLE_TYPE(STM32F2XXUsartState, STM32F2XX_USART)
>
> To keep your changes trivial to review, I split your patch in 4:
Ok - do you also want me to split the patch in the next version?
>
> 1/ Extract stm32f2xx_update_irq()
>
> -- >8 --
> diff --git a/hw/char/stm32f2xx_usart.c b/hw/char/stm32f2xx_usart.c
> index fde67f4f03..519d3461a3 100644
> --- a/hw/char/stm32f2xx_usart.c
> +++ b/hw/char/stm32f2xx_usart.c
> @@ -53,6 +53,17 @@ static int stm32f2xx_usart_can_receive(void *opaque)
> return 0;
> }
>
> +static void stm32f2xx_update_irq(STM32F2XXUsartState *s)
> +{
> + uint32_t mask = s->usart_sr & s->usart_cr1;
> +
> + if (mask & (USART_SR_TXE | USART_SR_TC | USART_SR_RXNE)) {
> + qemu_set_irq(s->irq, 1);
> + } else {
> + qemu_set_irq(s->irq, 0);
> + }
> +}
> +
> static void stm32f2xx_usart_receive(void *opaque, const uint8_t *buf,
> int size)
> {
> STM32F2XXUsartState *s = opaque;
> @@ -66,9 +77,7 @@ static void stm32f2xx_usart_receive(void *opaque,
> const uint8_t *buf, int size)
> s->usart_dr = *buf;
> s->usart_sr |= USART_SR_RXNE;
>
> - if (s->usart_cr1 & USART_CR1_RXNEIE) {
> - qemu_set_irq(s->irq, 1);
> - }
> + stm32f2xx_update_irq(s);
>
> DB_PRINT("Receiving: %c\n", s->usart_dr);
> }
> @@ -85,7 +94,7 @@ static void stm32f2xx_usart_reset(DeviceState *dev)
> s->usart_cr3 = 0x00000000;
> s->usart_gtpr = 0x00000000;
>
> - qemu_set_irq(s->irq, 0);
> + stm32f2xx_update_irq(s);
> }
>
> static uint64_t stm32f2xx_usart_read(void *opaque, hwaddr addr,
> @@ -106,7 +115,7 @@ static uint64_t stm32f2xx_usart_read(void *opaque,
> hwaddr addr,
> retvalue = s->usart_dr & 0x3FF;
> s->usart_sr &= ~USART_SR_RXNE;
> qemu_chr_fe_accept_input(&s->chr);
> - qemu_set_irq(s->irq, 0);
> + stm32f2xx_update_irq(s);
> return retvalue;
> case USART_BRR:
> return s->usart_brr;
> @@ -145,9 +154,7 @@ static void stm32f2xx_usart_write(void *opaque,
> hwaddr addr,
> } else {
> s->usart_sr &= value;
> }
> - if (!(s->usart_sr & USART_SR_RXNE)) {
> - qemu_set_irq(s->irq, 0);
> - }
> + stm32f2xx_update_irq(s);
> return;
> case USART_DR:
> if (value < 0xF000) {
> @@ -168,10 +175,7 @@ static void stm32f2xx_usart_write(void *opaque,
> hwaddr addr,
> return;
> case USART_CR1:
> s->usart_cr1 = value;
> - if (s->usart_cr1 & USART_CR1_RXNEIE &&
> - s->usart_sr & USART_SR_RXNE) {
> - qemu_set_irq(s->irq, 1);
> - }
> + stm32f2xx_update_irq(s);
> return;
> case USART_CR2:
> s->usart_cr2 = value;
> ---
>
> 2/ Update IRQ when SR is read
>
> -- >8 --
> diff --git a/hw/char/stm32f2xx_usart.c b/hw/char/stm32f2xx_usart.c
> index 519d3461a3..46e29089bc 100644
> --- a/hw/char/stm32f2xx_usart.c
> +++ b/hw/char/stm32f2xx_usart.c
> @@ -109,6 +109,7 @@ static uint64_t stm32f2xx_usart_read(void *opaque,
> hwaddr addr,
> case USART_SR:
> retvalue = s->usart_sr;
> qemu_chr_fe_accept_input(&s->chr);
> + stm32f2xx_update_irq(s);
> return retvalue;
> case USART_DR:
> DB_PRINT("Value: 0x%" PRIx32 ", %c\n", s->usart_dr, (char)
> s->usart_dr);
> ---
>
> Why is this required?
It's not required yet although it may be if support for more bits are
added (IDLE for instance). Although rereading the manual I'm not sure
that's how it would be implemented so I'll remove it.
>
> 3/ Update IRQ when DR is written
>
> -- >8 --
> diff --git a/hw/char/stm32f2xx_usart.c b/hw/char/stm32f2xx_usart.c
> index 46e29089bc..74f007591a 100644
> --- a/hw/char/stm32f2xx_usart.c
> +++ b/hw/char/stm32f2xx_usart.c
> @@ -169,6 +169,7 @@ static void stm32f2xx_usart_write(void *opaque,
> hwaddr addr,
> clear TC by writing 0 to the SR register, so set it again
> on each write. */
> s->usart_sr |= USART_SR_TC;
> + stm32f2xx_update_irq(s);
> }
> return;
> case USART_BRR:
> ---
>
> This change makes sense
>
> 4/ Add more CR1 bit definitions
>
> -- >8 --
> diff --git a/include/hw/char/stm32f2xx_usart.h
> b/include/hw/char/stm32f2xx_usart.h
> index 65bcc85470..fdfa7424a7 100644
> --- a/include/hw/char/stm32f2xx_usart.h
> +++ b/include/hw/char/stm32f2xx_usart.h
> @@ -49,6 +49,8 @@
> #define USART_SR_RXNE (1 << 5)
>
> #define USART_CR1_UE (1 << 13)
> +#define USART_CR1_TXEIE (1 << 7)
> +#define USART_CR1_TCEIE (1 << 6)
> #define USART_CR1_RXNEIE (1 << 5)
> #define USART_CR1_TE (1 << 3)
> #define USART_CR1_RE (1 << 2)
> ---
>
> These are not used, why add them?
For completeness since RXNEIE was already defined. Also for
documentation, to show that SR_TX/CR_TXEIE etc share bit numbers,
which the implementation relies on.
I can remove it though. Should I also remove RXNEIE which is no longer needed?
>
> Regards,
>
> Phil.
>
>
Thanks for reviewing,
Hans-Erik
next prev parent reply other threads:[~2023-10-20 14:41 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-20 11:14 [PATCH 1/1] stm32f2xx_usart: implement TX interrupts Hans-Erik Floryd
2023-10-20 12:42 ` Philippe Mathieu-Daudé
2023-10-20 14:40 ` Hans-Erik Floryd [this message]
2023-10-20 15:59 ` Philippe Mathieu-Daudé
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CADS1RdQLmbdobcAsO+DM79N2TgaErCwrctATXNf6a7VYT6hBqA@mail.gmail.com \
--to=hans-erik.floryd@rt-labs.com \
--cc=alistair@alistair23.me \
--cc=marcandre.lureau@redhat.com \
--cc=pbonzini@redhat.com \
--cc=peter.maydell@linaro.org \
--cc=philmd@linaro.org \
--cc=qemu-arm@nongnu.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).