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(-)
>
> 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?
> +
> 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.
> };
> 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.
> +}
> +
> 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
