Re: [PATCH] hw/char/stm32l4x5_usart.c: Fix ACK and min access size

[Peter Maydell <peter.maydell@linaro.org>]

On Mon, 2 Sept 2024 at 14:38, Jacob Abrams <satur9nine@gmail.com> wrote:
>
> These changes allow the official STM32L4xx HAL UART driver to function
> properly with the b-l475e-iot01a machine.
>
> Modifying USART_CR1 TE bit should alter USART_ISR TEACK bit, and
> likewise for RE and REACK bit.
>
> USART registers may be accessed via 16-bit instructions.
>
> Reseting USART_CR1 UE bit should restore ISR to default value.
>
> Fixes: 87b77e6e01ca ("hw/char/stm32l4x5_usart: Enable serial read and write")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2540
> Signed-off-by: Jacob Abrams <satur9nine@gmail.com>

Thanks for this patch. I have one question below, and one
minor nit.

> ---
>  hw/char/stm32l4x5_usart.c          | 29 +++++++++++++++++++---
>  tests/qtest/stm32l4x5_usart-test.c | 39 +++++++++++++++++++++++++++++-
>  2 files changed, 64 insertions(+), 4 deletions(-)
>
> diff --git a/hw/char/stm32l4x5_usart.c b/hw/char/stm32l4x5_usart.c
> index fc5dcac0c4..859fc6236a 100644
> --- a/hw/char/stm32l4x5_usart.c
> +++ b/hw/char/stm32l4x5_usart.c
> @@ -154,6 +154,28 @@ REG32(RDR, 0x24)
>  REG32(TDR, 0x28)
>      FIELD(TDR, TDR, 0, 9)
>
> +#define ISR_RESET_VALUE (0x020000C0)
> +
> +static void stm32l4x5_update_isr(Stm32l4x5UsartBaseState *s)
> +{
> +    if (!(s->cr1 & R_CR1_UE_MASK)) {
> +        s->isr = ISR_RESET_VALUE;
> +        return;
> +    }
> +
> +    if (s->cr1 & R_CR1_TE_MASK) {
> +        s->isr |= R_ISR_TEACK_MASK;
> +    } else {
> +        s->isr &= ~R_ISR_TEACK_MASK;
> +    }
> +
> +    if (s->cr1 & R_CR1_RE_MASK) {
> +        s->isr |= R_ISR_REACK_MASK;
> +    } else {
> +        s->isr &= ~R_ISR_REACK_MASK;
> +    }
> +}

Should we be doing these things based on the value of
the CR1 bits (as this code does), or instead do them
when the bit changes from 0 to 1 (or 1 to 0)?
The wording in the datasheet seems unclear to me; my
impression is that hardware designers often like to
do these things on transitions rather than level based,
but of course you can design h/w both ways...

I guess it could be tested on real hardware:
 * write CR1.TE = 1
 * wait for ISR.TEACK = 1
 * write ISR.TEACK = 0
 * write CR1.TE = 1 (i.e. write again to the register
   without changing its value)
 * does ISR.TEACK go to 1 again, or does it stay 0?

> +
>  static void stm32l4x5_update_irq(Stm32l4x5UsartBaseState *s)
>  {
>      if (((s->isr & R_ISR_WUF_MASK) && (s->cr3 & R_CR3_WUFIE_MASK))        ||
> @@ -367,7 +389,7 @@ static void stm32l4x5_usart_base_reset_hold(Object *obj, ResetType type)
>      s->brr = 0x00000000;
>      s->gtpr = 0x00000000;
>      s->rtor = 0x00000000;
> -    s->isr = 0x020000C0;
> +    s->isr = ISR_RESET_VALUE;
>      s->rdr = 0x00000000;
>      s->tdr = 0x00000000;
>
> @@ -456,6 +478,7 @@ static void stm32l4x5_usart_base_write(void *opaque, hwaddr addr,
>      case A_CR1:
>          s->cr1 = value;
>          stm32l4x5_update_params(s);
> +        stm32l4x5_update_isr(s);
>          stm32l4x5_update_irq(s);
>          return;
>      case A_CR2:
> @@ -508,12 +531,12 @@ static const MemoryRegionOps stm32l4x5_usart_base_ops = {
>      .endianness = DEVICE_NATIVE_ENDIAN,
>      .valid = {
>          .max_access_size = 4,
> -        .min_access_size = 4,
> +        .min_access_size = 2,
>          .unaligned = false
>      },
>      .impl = {
>          .max_access_size = 4,
> -        .min_access_size = 4,
> +        .min_access_size = 2,
>          .unaligned = false
>      },

The effect of these is that a 16-bit write not aligned
to a (4-aligned) register offset will generate a GUEST_ERROR
logged message, and a 16-bit write aligned to a 4-aligned
register offset will write the value zero-extended to 32 bits.
That seems reasonable to me.

>  };
> diff --git a/tests/qtest/stm32l4x5_usart-test.c b/tests/qtest/stm32l4x5_usart-test.c
> index 8902518233..ef886074c0 100644
> --- a/tests/qtest/stm32l4x5_usart-test.c
> +++ b/tests/qtest/stm32l4x5_usart-test.c
> @@ -36,6 +36,8 @@ REG32(GTPR, 0x10)
>  REG32(RTOR, 0x14)
>  REG32(RQR, 0x18)
>  REG32(ISR, 0x1C)
> +    FIELD(ISR, REACK, 22, 1)
> +    FIELD(ISR, TEACK, 21, 1)
>      FIELD(ISR, TXE, 7, 1)
>      FIELD(ISR, RXNE, 5, 1)
>      FIELD(ISR, ORE, 3, 1)
> @@ -191,7 +193,7 @@ static void init_uart(QTestState *qts)
>
>      /* Enable the transmitter, the receiver and the USART. */
>      qtest_writel(qts, (USART1_BASE_ADDR + A_CR1),
> -        R_CR1_UE_MASK | R_CR1_RE_MASK | R_CR1_TE_MASK);
> +        cr1 | R_CR1_UE_MASK | R_CR1_RE_MASK | R_CR1_TE_MASK);
>  }
>
>  static void test_write_read(void)
> @@ -202,6 +204,11 @@ static void test_write_read(void)
>      qtest_writel(qts, USART1_BASE_ADDR + A_TDR, 0xFFFFFFFF);
>      const uint32_t tdr = qtest_readl(qts, USART1_BASE_ADDR + A_TDR);
>      g_assert_cmpuint(tdr, ==, 0x000001FF);
> +
> +    /* Official STM HAL uses uint16_t for TDR */
> +    qtest_writew(qts, USART1_BASE_ADDR + A_TDR, 0xFFFF);
> +    const uint16_t tdr16 = qtest_readw(qts, USART1_BASE_ADDR + A_TDR);
> +    g_assert_cmpuint(tdr16, ==, 0x000001FF);
>  }
>
>  static void test_receive_char(void)
> @@ -296,6 +303,35 @@ static void test_send_str(void)
>      qtest_quit(qts);
>  }
>
> +static void test_ack(void)
> +{
> +    uint32_t cr1;
> +    uint32_t isr;
> +    QTestState *qts = qtest_init("-M b-l475e-iot01a");
> +
> +    init_uart(qts);
> +
> +    cr1 = qtest_readl(qts, (USART1_BASE_ADDR + A_CR1));
> +
> +    /* Disable the transmitter and receiver. */
> +    qtest_writel(qts, (USART1_BASE_ADDR + A_CR1),
> +        cr1 & ~(R_CR1_RE_MASK | R_CR1_TE_MASK));
> +
> +    /* Test ISR ACK for transmitter and receiver disabled */
> +    isr = qtest_readl(qts, (USART1_BASE_ADDR + A_ISR));
> +    g_assert_false(isr & R_ISR_TEACK_MASK);
> +    g_assert_false(isr & R_ISR_REACK_MASK);
> +
> +    /* Enable the transmitter and receiver. */
> +    qtest_writel(qts, (USART1_BASE_ADDR + A_CR1),
> +        cr1 | (R_CR1_RE_MASK | R_CR1_TE_MASK));
> +
> +    /* Test ISR ACK for transmitter and receiver disabled */
> +    isr = qtest_readl(qts, (USART1_BASE_ADDR + A_ISR));
> +    g_assert_true(isr & R_ISR_TEACK_MASK);
> +    g_assert_true(isr & R_ISR_REACK_MASK);

This is missing a
       qtest_quit(qts);
at the end of the function. Without it, on non-Linux
hosts the QEMU process-under-tests will not be properly
killed. We were also missing one at the end of
test_write_read() in this file, which we just fixed
this week in commit d1e8bea9c9c186.

> +}
> +
>  int main(int argc, char **argv)
>  {
>      int ret;
> @@ -308,6 +344,7 @@ int main(int argc, char **argv)
>      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);
> +    qtest_add_func("stm32l4x5/usart/ack", test_ack);
>      ret = g_test_run();
>
>      return ret;
> --
> 2.43.0

thanks
-- PMM