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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.