[PATCH v2 0/4] stm32f2xx_usart: implement TX interrupts

[PATCH v2 0/4] stm32f2xx_usart: implement TX interrupts

[Philippe Mathieu-Daudé]

Hi Hans-Erik,

Here is the respin of your patch [*] split in 4
parts for easy reviewing.

[*] https://lore.kernel.org/qemu-devel/20231020111428.3260965-1-hans-erik.floryd@rt-labs.com/

Hans-Erik Floryd (4):
  hw/char/stm32f2xx_usart: Extract common IRQ update code to
    update_irq()
  hw/char/stm32f2xx_usart: Update IRQ when SR is read
  hw/char/stm32f2xx_usart: Update IRQ when DR is written
  hw/char/stm32f2xx_usart: Add more definitions for CR1 register

 include/hw/char/stm32f2xx_usart.h | 10 ++++++----
 hw/char/stm32f2xx_usart.c         | 30 ++++++++++++++++++------------
 2 files changed, 24 insertions(+), 16 deletions(-)

-- 
2.41.0

[PATCH v2 1/4] hw/char/stm32f2xx_usart: Extract common IRQ update code to update_irq()

[Philippe Mathieu-Daudé]

From: Hans-Erik Floryd <hans-erik.floryd@rt-labs.com>

Signed-off-by: Hans-Erik Floryd <hans-erik.floryd@rt-labs.com>
[PMD: Split from bigger patch]
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/char/stm32f2xx_usart.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

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.41.0

[PATCH v2 2/4] hw/char/stm32f2xx_usart: Update IRQ when SR is read

[Philippe Mathieu-Daudé]

From: Hans-Erik Floryd <hans-erik.floryd@rt-labs.com>

Signed-off-by: Hans-Erik Floryd <hans-erik.floryd@rt-labs.com>
[PMD: Split from bigger patch]
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
Is this required?
---
 hw/char/stm32f2xx_usart.c | 1 +
 1 file changed, 1 insertion(+)

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);
-- 
2.41.0

[PATCH v2 3/4] hw/char/stm32f2xx_usart: Update IRQ when DR is written

[Philippe Mathieu-Daudé]

From: Hans-Erik Floryd <hans-erik.floryd@rt-labs.com>

Signed-off-by: Hans-Erik Floryd <hans-erik.floryd@rt-labs.com>
[PMD: Split from bigger patch]
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/char/stm32f2xx_usart.c | 1 +
 1 file changed, 1 insertion(+)

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:
-- 
2.41.0

[PATCH v2 4/4] hw/char/stm32f2xx_usart: Add more definitions for CR1 register

[Philippe Mathieu-Daudé]

From: Hans-Erik Floryd <hans-erik.floryd@rt-labs.com>

Signed-off-by: Hans-Erik Floryd <hans-erik.floryd@rt-labs.com>
[PMD: Split from bigger patch]
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
Useful if unused?
---
 include/hw/char/stm32f2xx_usart.h | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

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)
-- 
2.41.0

Re: [PATCH v2 1/4] hw/char/stm32f2xx_usart: Extract common IRQ update code to update_irq()

[Richard Henderson]

On 10/20/23 10:00, Philippe Mathieu-Daudé wrote:
> From: Hans-Erik Floryd <hans-erik.floryd@rt-labs.com>
> 
> Signed-off-by: Hans-Erik Floryd <hans-erik.floryd@rt-labs.com>
> [PMD: Split from bigger patch]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/char/stm32f2xx_usart.c | 28 ++++++++++++++++------------
>   1 file changed, 16 insertions(+), 12 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v2 2/4] hw/char/stm32f2xx_usart: Update IRQ when SR is read

[Richard Henderson]

On 10/20/23 10:00, Philippe Mathieu-Daudé wrote:
> From: Hans-Erik Floryd <hans-erik.floryd@rt-labs.com>
> 
> Signed-off-by: Hans-Erik Floryd <hans-erik.floryd@rt-labs.com>
> [PMD: Split from bigger patch]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> Is this required?
> ---
>   hw/char/stm32f2xx_usart.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> 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);

Is there a status bit change missing?  There doesn't seem to be anything to update here.


r~

Re: [PATCH v2 3/4] hw/char/stm32f2xx_usart: Update IRQ when DR is written

[Richard Henderson]

On 10/20/23 10:00, Philippe Mathieu-Daudé wrote:
> From: Hans-Erik Floryd <hans-erik.floryd@rt-labs.com>
> 
> Signed-off-by: Hans-Erik Floryd <hans-erik.floryd@rt-labs.com>
> [PMD: Split from bigger patch]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/char/stm32f2xx_usart.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> 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:

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 4/4] hw/char/stm32f2xx_usart: Add more definitions for CR1 register

[Richard Henderson]

On 10/20/23 10:00, Philippe Mathieu-Daudé wrote:
> From: Hans-Erik Floryd <hans-erik.floryd@rt-labs.com>
> 
> Signed-off-by: Hans-Erik Floryd <hans-erik.floryd@rt-labs.com>
> [PMD: Split from bigger patch]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> Useful if unused?
> ---
>   include/hw/char/stm32f2xx_usart.h | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> 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)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 1/4] hw/char/stm32f2xx_usart: Extract common IRQ update code to update_irq()

[Alistair Francis]

On Sat, Oct 21, 2023 at 4:14 AM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> From: Hans-Erik Floryd <hans-erik.floryd@rt-labs.com>
>
> Signed-off-by: Hans-Erik Floryd <hans-erik.floryd@rt-labs.com>
> [PMD: Split from bigger patch]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

Alistair

> ---
>  hw/char/stm32f2xx_usart.c | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
>
> 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.41.0
>
>

Re: [PATCH v2 2/4] hw/char/stm32f2xx_usart: Update IRQ when SR is read

[Alistair Francis]

On Sat, Oct 21, 2023 at 4:12 AM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> From: Hans-Erik Floryd <hans-erik.floryd@rt-labs.com>
>
> Signed-off-by: Hans-Erik Floryd <hans-erik.floryd@rt-labs.com>
> [PMD: Split from bigger patch]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> Is this required?

I don't think this is required

Alistair

> ---
>  hw/char/stm32f2xx_usart.c | 1 +
>  1 file changed, 1 insertion(+)
>
> 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);
> --
> 2.41.0
>
>

Re: [PATCH v2 3/4] hw/char/stm32f2xx_usart: Update IRQ when DR is written

[Alistair Francis]

On Sat, Oct 21, 2023 at 3:01 AM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> From: Hans-Erik Floryd <hans-erik.floryd@rt-labs.com>
>
> Signed-off-by: Hans-Erik Floryd <hans-erik.floryd@rt-labs.com>
> [PMD: Split from bigger patch]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

Alistair

> ---
>  hw/char/stm32f2xx_usart.c | 1 +
>  1 file changed, 1 insertion(+)
>
> 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:
> --
> 2.41.0
>
>

Re: [PATCH v2 4/4] hw/char/stm32f2xx_usart: Add more definitions for CR1 register

[Alistair Francis]

On Sat, Oct 21, 2023 at 4:20 AM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> From: Hans-Erik Floryd <hans-erik.floryd@rt-labs.com>
>
> Signed-off-by: Hans-Erik Floryd <hans-erik.floryd@rt-labs.com>
> [PMD: Split from bigger patch]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> Useful if unused?

Still useful I think

Acked-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

Re: [PATCH v2 2/4] hw/char/stm32f2xx_usart: Update IRQ when SR is read

[Hans-Erik Floryd]

On Mon, 23 Oct 2023 at 03:37, Alistair Francis <alistair23@gmail.com> wrote:
>
> On Sat, Oct 21, 2023 at 4:12 AM Philippe Mathieu-Daudé
> <philmd@linaro.org> wrote:
> >
> > From: Hans-Erik Floryd <hans-erik.floryd@rt-labs.com>
> >
> > Signed-off-by: Hans-Erik Floryd <hans-erik.floryd@rt-labs.com>
> > [PMD: Split from bigger patch]
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > ---
> > Is this required?
>
> I don't think this is required
>
> Alistair

I agree this should be removed. It was added because the manual says
that some irq:s are cleared by a combination of reading SR and writing
to DR. However, those bits are not implemented yet, and probably the
clearing should be done when writing DR anyway.

Thanks for reviewing,
 Hans-Erik