From: "Philippe Mathieu-Daudé" <philmd@linaro.org>
To: Hans-Erik Floryd <hans-erik.floryd@rt-labs.com>
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 17:59:51 +0200 [thread overview]
Message-ID: <5563056c-0144-ee4b-bd99-e51f31369feb@linaro.org> (raw)
In-Reply-To: <CADS1RdQLmbdobcAsO+DM79N2TgaErCwrctATXNf6a7VYT6hBqA@mail.gmail.com>
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 parent reply other threads:[~2023-10-20 16:00 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
2023-10-20 15:59 ` Philippe Mathieu-Daudé [this message]
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=5563056c-0144-ee4b-bd99-e51f31369feb@linaro.org \
--to=philmd@linaro.org \
--cc=alistair@alistair23.me \
--cc=hans-erik.floryd@rt-labs.com \
--cc=marcandre.lureau@redhat.com \
--cc=pbonzini@redhat.com \
--cc=peter.maydell@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).