Re: [PATCH 7/7] tests/qtest: Add tests for the STM32L4x5 USART

[Peter Maydell]

On Sun, 17 Mar 2024 at 10:42, Arnaud Minier
<arnaud.minier@telecom-paris.fr> wrote:
>
> Test:
> - read/write from/to the usart registers
> - send/receive a character/string over the serial port
>
> The test to detect overrun is implemented but disabled
> because overruns are currently impossible due to how we signal
> in the USART when we are ready to receive a new character.
>
> Signed-off-by: Arnaud Minier <arnaud.minier@telecom-paris.fr>
> Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
> ---



> --- /dev/null
> +++ b/tests/qtest/stm32l4x5_usart-test.c
> @@ -0,0 +1,399 @@
> +/*
> + * QTest testcase for STML4X5_USART
> + *
> + * Copyright (c) 2023 Arnaud Minier <arnaud.minier@telecom-paris.fr>
> + * Copyright (c) 2023 Inès Varhol <ines.varhol@telecom-paris.fr>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqtest-single.h"
> +#include "hw/misc/stm32l4x5_rcc_internals.h"
> +#include "hw/registerfields.h"
> +
> +/*
> + * All page references in the following test
> + * refer to the ST RM0351 Reference Manuel.

"Manual". Also, what page references? I couldn't see any
from a quick scroll through.


> + */
> +
> +#define RCC_BASE_ADDR 0x40021000
> +/* Use USART 1 ADDR, assume the others work the same */
> +#define USART1_BASE_ADDR 0x40013800
> +
> +/* See stm32l4x5_usart for definitions */
> +REG32(CR1, 0x00)
> +    FIELD(CR1, M1, 28, 1)
> +    FIELD(CR1, OVER8, 15, 1)
> +    FIELD(CR1, M0, 12, 1)
> +    FIELD(CR1, PCE, 10, 1)
> +    FIELD(CR1, TXEIE, 7, 1)
> +    FIELD(CR1, RXNEIE, 5, 1)
> +    FIELD(CR1, TE, 3, 1)
> +    FIELD(CR1, RE, 2, 1)
> +    FIELD(CR1, UE, 0, 1)
> +REG32(CR2, 0x04)
> +REG32(CR3, 0x08)
> +    FIELD(CR3, OVRDIS, 12, 1)
> +REG32(BRR, 0x0C)
> +REG32(GTPR, 0x10)
> +REG32(RTOR, 0x14)
> +REG32(RQR, 0x18)
> +REG32(ISR, 0x1C)
> +    FIELD(ISR, TXE, 7, 1)
> +    FIELD(ISR, RXNE, 5, 1)
> +    FIELD(ISR, ORE, 3, 1)
> +REG32(ICR, 0x20)
> +REG32(RDR, 0x24)
> +REG32(TDR, 0x28)
> +
> +#define NVIC_ISPR1 0XE000E204
> +#define NVIC_ICPR1 0xE000E284
> +#define USART1_IRQ 37
> +
> +static bool check_nvic_pending(QTestState *qts, unsigned int n)
> +{
> +    /* No USART interrupts are less than 32 */
> +    if (n < 32) {
> +        return false;
> +    }

I think this would be better as an assert() -- it would be
a bug in the test case if it called this with a bad interrupt number.

> +    n -= 32;
> +    return qtest_readl(qts, NVIC_ISPR1) & (1 << n);
> +}
> +
> +static bool clear_nvic_pending(QTestState *qts, unsigned int n)
> +{
> +    /* No USART interrupts are less than 32 */
> +    if (n < 32) {
> +        return false;
> +    }

Similarly here.

> +    n -= 32;
> +    qtest_writel(qts, NVIC_ICPR1, (1 << n));
> +    return true;
> +}
> +
> +static void usart_writel(unsigned int offset, uint32_t value)
> +{
> +    writel(USART1_BASE_ADDR + offset, value);
> +}
> +
> +static uint32_t usart_readl(unsigned int offset)
> +{
> +    return readl(USART1_BASE_ADDR + offset);
> +}
> +
> +static bool usart_wait_for_flag(QTestState *qts, uint32_t event_addr, 
> uint32_t flag)
> +{
> +    /* Wait at most 5 seconds */
> +    for (int i = 0; i < 5000; i++) {
> +        if ((qtest_readl(qts, event_addr) & flag)) {
> +            return true;
> +        }
> +        g_usleep(1000);
> +    }

qtest tests should never need to sleep(). If you need to advance
the guest clock, use clock_step(). If this is because we've sent some
data to the UART over the socket and need to wait for it to appear
in the QEMU process, that might be trickier, but I would see if
advancing the guest clock works reliably for that.

In general having tests which encode "wait for some wallclock
time" is flaky, because while it might be plenty of time on your
fast development machine, it can cause intermittent failures due
to timeouts if the test is on some heavily-loaded slow CI runner.

I note that the microbit version of this "wait for the UART"
has a timeout of 10 minutes.

> +
> +    return false;
> +}



> +int main(int argc, char **argv)
> +{
> +    int ret;
> +
> +    g_test_init(&argc, &argv, NULL);
> +    g_test_set_nonfatal_assertions();
> +
> +    qtest_add_func("stm32l4x5/usart/write_read", test_write_read);
> +    qtest_add_func("stm32l4x5/usart/receive_char", test_receive_char);
> +    qtest_add_func("stm32l4x5/usart/send_char", test_send_char);
> +    qtest_add_func("stm32l4x5/usart/receive_str", test_receive_str);
> +    qtest_add_func("stm32l4x5/usart/send_str", test_send_str);
> +    /* Disabled tests */
> +    if (false) {
> +        qtest_add_func("stm32l4x5/usart/overrun", test_overrun);
> +    }

If the test doesn't work because QEMU's implementation never
generates overruns, I would just not put it in the file, rather
than having dead code we never run.

> +    qtest_start("-machine b-l475e-iot01a");
> +    ret = g_test_run();
> +    qtest_end();
> +
> +    return ret;
> +}

thanks
-- PMM