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

[Philippe Mathieu-Daudé]

Hi,

(Cc'ing Arnaud & Inès who are listed as maintainers)

On 6/9/24 18:12, Peter Maydell wrote:
> 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(-)


> >   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.

Peter, are you describing the .valid.min_access_size 4 -> 2 change
or the .impl.min_access_size one?

My understanding of the implementation is a 32-bit one:

  REG32(CR1, 0x00)

  struct Stm32l4x5UsartBaseState {
      ...
      uint32_t cr1;

  static void stm32l4x5_usart_base_write(void *opaque, hwaddr addr,
                                  uint64_t val64, unsigned int size)
  {
      ...
      switch (addr) {
      case A_CR1:
          s->cr1 = value;

Am I missing something?

Now, back to .valid.min_access_size, per the section "40.8 USART
registers" of the reference manual:

  The peripheral registers have to be accessed by words (32 bits).

So I don't get the "USART registers may be accessed via 16-bit
instructions." part of this patch.

Jacob, for clarity, can you split this patch in 3 distinct parts
(TE bit, UE bit, unaligned access) so this discussion doesn't delay
the part which are OK?

Thanks,

Phil.