|
| From: | Philippe Mathieu-Daudé |
| Subject: | Re: [PATCH 1/1] stm32f2xx_usart: implement TX interrupts |
| Date: | Fri, 20 Oct 2023 17:59:51 +0200 |
| User-agent: | Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.15.1 |
On 20/10/23 16:40, Hans-Erik Floryd wrote:
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?
I had to do it because I found your patch including too many changes at once. Other might find the same (usually we ask for one atomical change per commit: easier to review, and also useful if there is a bug, you can revert the single offending commit). Since I did the split, I can post it and you can re-use the splitted patch series.
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?
I'm not against adding the definitions, but better if they are used :) My review reflex was "oh, definitions are added but not used, is that a bug or a left over?". I'll post your splitted patches shortly. Regard, Phil.
| [Prev in Thread] | Current Thread | [Next in Thread] |