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

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

Hi Jacob,

On 10/9/24 06:34, Jacob Abrams wrote:
> On 9/9/24 10:40, Philippe Mathieu-Daudé wrote:
>> 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.
> 
> Hmm it does appear to be a documentation error in RM0351 section 40.8.
> 
> I have an STM32L476 board (chip is nearly identical to the STM32L475 minus the display module) which is functioning with the USART driver code provided by STM in their examples that uses 16-bit access for several of the USART registers, see:
> 
> https://github.com/STMicroelectronics/cmsis_device_l4/blob/a2530753e86dd326a75467d28feb92e2ba7d0df2/Include/stm32l475xx.h#L950

OK, we need to mention that in a comment around, something like:

   /*
    * While the reference manual states "the peripheral registers
    * have to be accessed by words (32 bits)", code -- like the
    * STM32CubeL4 CMSIS Device MCU Component -- ran on real
    * hardware shows some registers can be accessed by 16 bits.
    */
   .valid.min_access_size = 2,

> I have a contact at STM through my work that I can mention this to just to double check.

Thanks, that is great.

> Ultimately my goal is to get an STM32CubeL4 example such as UART_Printf running as-is in Qemu and it was only while attempting to run such a project in Qemu that I ran into this problem, see:
> 
> https://github.com/STMicroelectronics/STM32CubeL4/tree/master/Projects/STM32L476G-EVAL/Examples/UART/UART_Printf

It would be nice to add a test using this file in our functional
test suite, see for example tests/functional/test_avr_mega2560.py.

> 
> This project works on my HW dev board just fine.
> 
> Of course if you still desire I don't mind splitting this change into 3 parts.

Well, you clearly list 3 steps in the commit description.
Besides, the discussions whether .imp.min_access_size = 2
is valid still stands.

Regards,

Phil.