[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/1] stm32f2xx_usart: implement TX interrupts
|
From: |
Philippe Mathieu-Daudé |
|
Subject: |
Re: [PATCH 1/1] stm32f2xx_usart: implement TX interrupts |
|
Date: |
Fri, 20 Oct 2023 14:42:32 +0200 |
|
User-agent: |
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.15.1 |
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:
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?
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?
Regards,
Phil.