On 9/10/24 02:34, Peter Maydell wrote:
> On Mon, 9 Sept 2024 at 18:40, Philippe Mathieu-Daudé <phi...@linaro.org>
> 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 <satur9n...@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 <satur9n...@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?
>
> I was intending to summarise the effects of making the code
> changes above (both .impl and .valid), without any reference
> to what the real hardware behaviour might or might not be
> (as a starter for figuring out whether the change is reasonable).
>
>> 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?
>
> If we make the .impl and .valid changes, then the result is
> that we permit 16 bit writes to come through to the read
> and write functions. Since we don't make any changes to
> those functions to specially handle size == 2, you get the
> effects of the existing code. If the 16 bit write is aligned
> to a 4-aligned register offset it will match one of the A_*
> cases, and will write 16-bit-value-zero-extended to it.
> If the 16 bit write isn't to a 4-aligned offset it will fall
> into the "default" case and be logged as a GUEST_ERROR.
>
> Did I miss some aspect of what the behaviour change is?
> -- PMM
Ah I see your point now regarding the zero extension writes. Basically you are
saying this patch will have the effect that a 16-bit write to one of the USART
registers such as USART_RTOR via an ARM instruction like STRH would not be
handled correctly because value to be written will be zero extended to 32-bits
before being written.
So for example if USART_RTOR currently has the value 0xAAAABBBB and STRH of
value 0xCCCC is being applied to the lower 16 bits then one would expect the
value to be 0xAAAACCCC in real HW but after my code change in Qemu then
USART_RTOR would end up with 0x0000CCCC because the size parameter of
stm32l4x5_usart_base_write is not being taken into account.
I do believe this is a valid problem. I will test the behavior on my STM32L476
and report back the real HW behavior and I suspect the result would be STRH
write to such registers is allowed and unwritten bits are preserved and not
overwritten with zeros.
I submitted a new patch for just TEACK fix already and will report back on the
real HW test result later and adjust 16-bit read/write code as needed to behave
properly.
I have also submitted a question to my STM contact regarding the reference
manual and assuming the answer is yes it is in error I will add comments
explaining that to my next patch for the 16-bit read/write handling.
-- Jacob
