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

[Jacob Abrams]

On 9/6/24 09: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(-)
>>
>> 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?
> 

Per RM0351 all bits in the ISR register are read-only by software, so we cannot 
test writing ISR.TEACK on a physical chip, the write would be ignored. Precise 
description in RM0351 is "This bit is set/reset by hardware, when the Transmit 
Enable value is taken into account by the USART." Based on this I think it is 
safe to assume that ISR.TEACK should always reflect the value of CR1.TE, though 
in real hardware it would take some microseconds for TEACK to be updated after 
TE is changed.

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

OK sounds good

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

Thanks I will submit a v2 patch with this corrected.

>> +}
>> +
>>  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

regards
-- Jacob Abrams