On Mon, 17 Aug 2026 at 06:03, Jacob Whitaker Abrams <[email protected]> wrote: > > Official STM32CubeL4 drivers use and require support for 16-bit writes to > UART registers for proper function.
This is a breach of the datasheet (assuming I have the right one: RM0351 "STM32L47xxx, STM32L48xxx, STM32L49xxx and STM32L4Axxx advanced Arm®-based 32-bit MCUs"), which is pretty clear: "The peripheral registers have to be accessed by words (32 bits)". Still, if the official drivers are doing it then presumably the hardware actually does allow smaller accesses. Do the drivers do small accesses to the top parts of registers, or do they always use 4-aligned addresses but just sometimes do smaller width accesses ? If only the latter is needed, we can make the code simpler (reads return the full register value and the QEMU core code chops off the unneeded high parts, writes are "write as if zero-extended"). > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2540 > Signed-off-by: Jacob Whitaker Abrams <[email protected]> > --- > hw/char/stm32l4x5_usart.c | 93 +++++++++++++++++++++++------- > tests/qtest/stm32l4x5_usart-test.c | 50 +++++++++++++++- > 2 files changed, 120 insertions(+), 23 deletions(-) > > diff --git a/hw/char/stm32l4x5_usart.c b/hw/char/stm32l4x5_usart.c > index dd1b099195..88c4a3b5a7 100644 > --- a/hw/char/stm32l4x5_usart.c > +++ b/hw/char/stm32l4x5_usart.c > @@ -154,8 +154,15 @@ 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; > + } This seems to be an unrelated change to adding 16-bit access handling ? > + > if (s->cr1 & R_CR1_TE_MASK) { > s->isr |= R_ISR_TEACK_MASK; > } else { > @@ -404,9 +411,11 @@ static uint64_t stm32l4x5_usart_base_read(void *opaque, > hwaddr addr, > unsigned int size) > { > Stm32l4x5UsartBaseState *s = opaque; > + hwaddr base = addr & ~0x3ULL; > + unsigned int offset = addr & 0x3; > uint64_t retvalue = 0; > > - switch (addr) { > + switch (base) { > case A_CR1: > retvalue = s->cr1; > break; > @@ -451,6 +460,13 @@ static uint64_t stm32l4x5_usart_base_read(void *opaque, > hwaddr addr, > break; > } > > + /* Adjust for partial access */ > + if (size == 1) { > + retvalue = (retvalue >> (offset * 8)) & 0xFF; > + } else if (size == 2) { > + retvalue = (retvalue >> (offset * 8)) & 0xFFFF; > + } You can avoid the if(): retvalue = extract32(retvalue, offset * 8, size * 8); > + > trace_stm32l4x5_usart_read(addr, retvalue); > > return retvalue; > @@ -460,55 +476,88 @@ static void stm32l4x5_usart_base_write(void *opaque, > hwaddr addr, > uint64_t val64, unsigned int size) > { > Stm32l4x5UsartBaseState *s = opaque; > - const uint32_t value = val64; > + hwaddr base = addr & ~0x3ULL; > + unsigned int offset = addr & 0x3; > + uint32_t value = (uint32_t)val64; > + > + /* Build mask for partial access */ > + uint32_t mask; > + if (size == 4) { > + mask = 0xFFFFFFFF; > + } else if (size == 2) { > + mask = 0xFFFF << (offset * 8); > + } else if (size == 1) { > + mask = 0xFF << (offset * 8); > + } else { > + qemu_log_mask(LOG_GUEST_ERROR, "%s: Unsupported access size %u\n", > __func__, size); > + return; > + } > + value = (value << (offset * 8)) & mask; > > trace_stm32l4x5_usart_write(addr, value); > > - switch (addr) { > - case A_CR1: > - s->cr1 = value; > + switch (base) { > + case A_CR1: { > + uint32_t old = s->cr1; > + s->cr1 = (old & ~mask) | value; If you write these as s->cr1 = deposit32(s->cr1, value, offset * 8, size * 8); then you don't need to calculate a mask or shift-and-mask the value by hand. thanks -- PMM
