Skip to content

Conversation

@knthm
Copy link
Contributor

@knthm knthm commented Jun 19, 2023

Summary

This PR refactors the initialization/configuration logic of STM32 UART driver. It is intended to complement PR #59200 by making reinitialization of the peripheral clocks and registers more straightforward, for when the SoC loses their context and contents in certain low-power modes.

In refactoring I've also stumbled on a few bugs that I've fixed.

  • Added static uart_config struct to STM32_UART_INIT macro
  • Added uart_config struct pointer as uart_stm32_config member
  • (Re)moved now superfluous baud_rate, parity, and hw_flow_control struct members
  • Moved overlapping baudrate/parity/data-/stop-bits/flow control configuration to common function, with persistence in static instance uart_config struct
  • Removed incorrect runtime parity/data bits configuration, and replaced with thoroughly tested compile-time BUILD_ASSERT statements
    • Original runtime logic in init forced a very small subset of UART configurations, and straight-up ignored many valid DT property combinations (e.g 7E1)
    • This now also allows invalid configurations to get flagged compile-time
    • The previous logic partially conflicted with the logic in uart_stm32_configure, especially that relating to parity/data-bits combinations, where either enforced opposite configurations
  • The #if defined(LL_USART_STOPBITS_1_5) && HAS_LPUART_1 conditional broke "half-bit" parity application configurations of USARTs, this is now fixed

Change Description

Add static uart_config struct to init macro with corresponding pointer as uart_stm32_config struct member. This struct will store boot and runtime UART configuration for reinitialization upon exiting low-power modes, where register contents might be lost.

Remove hw_flow_control, parity, and baud_rate from uart_stm32_config and uart_stm32_data structs, respectively, as the need for these is now obviated by the presence of the uart_config struct, which contains equivalent members.

Add default baudrate, parity, stop, and data bits constants, used to initialize the uart_config struct, in case the corresponding UART DT properties are not set.

Move overlapping UART parameter configuration to new uart_stm32_parameters_set function. This function is called by uart_stm32_configure upon application/subsystem configuration, and uart_stm32_registers_configure upon (re-)initialization of driver instances.

Fix flawed stop bit/LPUART conditionals, which didn't allow for 0.5/1.5 stop bit USART configurations via application uart_configure syscalls.

Replace incorrect and limited runtime parity/data bits conditional configuration with build-time logic using BUILD_ASSERT. This allows for more flexible UART configurations, while preventing invalid DTS configurations at build-time.

Issues Closed by this PR

Related Issues or Pull Requests

@knthm
Copy link
Contributor Author

knthm commented Jun 19, 2023

  • Split into individual commits to assist review

Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok generally.
The only point is the use of new struct in the device const config struct while in theory runtime updatable parameters should go in data struct.

@knthm knthm force-pushed the stm32-uart-refactor branch 2 times, most recently from a7f533b to b27e59c Compare June 23, 2023 02:53
@knthm
Copy link
Contributor Author

knthm commented Jun 23, 2023

  • Add separate commit for include directives move
  • Add separate commit for moving reset configuration to config struct
  • Move uart_config pointer to uart_stm32_data struct
  • Fix indentation and remove inline keyword from function called more than once
  • Simplify DTS stop bit BUILD_ASSERT check

@knthm
Copy link
Contributor Author

knthm commented Aug 7, 2023

  • Rebase on main and resolve conflicts

@erwango erwango self-requested a review August 21, 2023 14:51
erwango
erwango previously approved these changes Aug 23, 2023
Copy link
Contributor

@gautierg-st gautierg-st left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments about the datawidth check, otherwise looks good

gautierg-st
gautierg-st previously approved these changes Aug 29, 2023
@erwango
Copy link
Member

erwango commented Aug 30, 2023

#54354 has been merged. Please rebase

knthm added 6 commits August 30, 2023 17:31
Move select include directives from source to header file, as these pull
data types required by the struct definitions contained within it.

This promotes a more modular file structure.

Signed-off-by: Kenneth J. Miller <[email protected]>
Move reset configuration from uart_stm32_data to
const uart_stm32_config struct, as this is set once at boot and isn't
modified during runtime.

Signed-off-by: Kenneth J. Miller <[email protected]>
Add static uart_config struct to init macro with corresponding pointer
as uart_stm32_config struct member. This struct will store boot and
runtime UART configuration for reinitialization upon exiting low-power
modes, where register contents might be lost.

Remove hw_flow_control, parity, and baud_rate from uart_stm32_config and
uart_stm32_data structs, respectively, as the need for these is now
obviated by the presence of the uart_config struct, which contains
equivalent members.

Add default baudrate, parity, stop, and data bits constants, used to
initialize the uart_config struct, in case the corresponding UART DT
properties are not set.

Signed-off-by: Kenneth J. Miller <[email protected]>
Move clock enable and register configuration to their own functions in
preparation for later uart_stm32_reinit function that will use these in
addition to uart_stm32_init.

Signed-off-by: Kenneth J. Miller <[email protected]>
Move overlapping UART parameter configuration to new
uart_stm32_parameters_set function. This function is called by
uart_stm32_configure upon application/subsystem configuration, and
uart_stm32_registers_configure upon (re-)initialization of driver
instances.

Signed-off-by: Kenneth J. Miller <[email protected]>
Replace incorrect and limited runtime parity/data bits conditional
configuration with build-time logic using BUILD_ASSERT. This allows for
more flexible UART configurations, while preventing invalid DTS
configurations at build-time.

Signed-off-by: Kenneth J. Miller <[email protected]>
@knthm
Copy link
Contributor Author

knthm commented Aug 31, 2023

  • Rebase on main and resolve conflicts

@knthm knthm assigned gautierg-st and unassigned FRASTM Aug 31, 2023
@knthm knthm requested a review from gautierg-st August 31, 2023 15:34
@carlescufi carlescufi merged commit 044de03 into zephyrproject-rtos:main Sep 1, 2023
@knthm knthm deleted the stm32-uart-refactor branch September 2, 2023 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: UART Universal Asynchronous Receiver-Transmitter platform: STM32 ST Micro STM32

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants