4
0
Fork
You've already forked libopencm3
3

Feature: usart_get_baudrate for STM32 common USART and TM4C UART #5

Merged
ALTracer merged 2 commits from feature/usart_get_baudrate into main 2024年08月20日 22:26:32 +02:00
ALTracer commented 2024年08月17日 11:46:06 +02:00 (Migrated from github.com)
Copy link

Implement library functions to compute effective U(S)ART baudrate as counterparts to usart_set_baudrate().
This code is extracted from https://github.com/blackmagic-debug/blackmagic/pull/1864

TM4C seems to have an instance of ARM PrimeCell UART PL011. I cannot test my implementation as I don't have the hardware.

STM32F4 has USART v1 IP with OVER8 support (pending). The value in these functions is that they should handle OVER8 enabled/disabled differences in calculations. STM32F1 does not support OVER8 and can use this already. I remember testing on either bluepill or blackpill-f411ce, and I can test on these devices.

This is required for Blackmagic Debug Firmware to support reporting precise baud rates used in TraceSWO Async capture.

Implement library functions to compute effective U(S)ART baudrate as counterparts to `usart_set_baudrate()`. This code is extracted from https://github.com/blackmagic-debug/blackmagic/pull/1864 TM4C seems to have an instance of ARM PrimeCell UART PL011. I cannot test my implementation as I don't have the hardware. STM32F4 has USART v1 IP with OVER8 support (pending). The value in these functions is that they should handle OVER8 enabled/disabled differences in calculations. STM32F1 does not support OVER8 and can use this already. I remember testing on either bluepill or blackpill-f411ce, and I can test on these devices. This is required for Blackmagic Debug Firmware to support reporting precise baud rates used in TraceSWO Async capture.
dragonmux left a comment
Copy link

Couple of hopefully small things, but with them addressed we'll be happy to merge this.

Couple of hopefully small things, but with them addressed we'll be happy to merge this.
@ -130,6 +130,25 @@ void uart_set_baudrate(uint32_t uart, uint32_t baud)
UART_FBRD(uart) = div % 64;

The constant for PIOSC mode is missing its U suffix, please include it so the expression is unsigned.

The constant for `PIOSC` mode is missing its `U` suffix, please include it so the expression is unsigned.
@ -94,6 +94,39 @@ void usart_set_baudrate(uint32_t usart, uint32_t baud)
#endif

Believe this should depend on the value of USART_CR1_OVER8 being set too - it's probably worth integrating that into this calculation as above in the usart_set_baudrate() implementation.

Believe this should depend on the value of USART_CR1_OVER8 being set too - it's probably worth integrating that into this calculation as above in the `usart_set_baudrate()` implementation.
ALTracer (Migrated from github.com) reviewed 2024年08月19日 09:02:28 +02:00
@ -130,6 +130,25 @@ void uart_set_baudrate(uint32_t uart, uint32_t baud)
UART_FBRD(uart) = div % 64;
ALTracer (Migrated from github.com) commented 2024年08月19日 09:02:28 +02:00
Copy link

Accepted. I was looking at uart_set_baudrate() implementation up above which sets uint32_t clock to 16000000 signed.

Accepted. I was looking at `uart_set_baudrate()` implementation up above which sets `uint32_t clock` to 16000000 signed.
ALTracer (Migrated from github.com) reviewed 2024年08月19日 09:02:47 +02:00
@ -94,6 +94,39 @@ void usart_set_baudrate(uint32_t usart, uint32_t baud)
#endif
ALTracer (Migrated from github.com) commented 2024年08月19日 09:02:47 +02:00
Copy link

Added untested code. Should I also handle LPUART1 specially?

Added untested code. Should I also handle LPUART1 specially?
@ -94,6 +94,39 @@ void usart_set_baudrate(uint32_t usart, uint32_t baud)
#endif

Missing U suffix; please includes spaces around the operator and if you feel the need to show the multiplication must happen first, parens.

Missing `U` suffix; please includes spaces around the operator and if you feel the need to show the multiplication must happen first, parens.
@ -94,6 +94,39 @@ void usart_set_baudrate(uint32_t usart, uint32_t baud)
#endif

Yes, otherwise these are latent bugs due to the mismatched implementations. We would suggest handling LPUART1 is probably a smart/necessary idea. We'll test the OVER8 code on a STM32H7 with known PLL setup and dividers so we know what we should expect out the calculation.

Yes, otherwise these are latent bugs due to the mismatched implementations. We would suggest handling LPUART1 is probably a smart/necessary idea. We'll test the OVER8 code on a STM32H7 with known PLL setup and dividers so we know what we should expect out the calculation.
ALTracer (Migrated from github.com) reviewed 2024年08月19日 17:47:25 +02:00
@ -94,6 +94,39 @@ void usart_set_baudrate(uint32_t usart, uint32_t baud)
#endif
ALTracer (Migrated from github.com) commented 2024年08月19日 17:47:25 +02:00
Copy link

Added LPUART1 block as-if simple 256x clock.
The more I look at it, the more I think it should go into usart_common_f124.c (F1 no OVER8, F2, F4, L1) or even usart_common_v2.c (F0, F3, F7, G0, G4, H7, L0, L4). Were there any specific reasons why you duplicated your usart_set_oversampling() to both sources? Should I? (I originally aimed at F4 only, but the logic for others could be shared, like F723 in stlinkv3)

Added LPUART1 block as-if simple 256x clock. The more I look at it, the more I think it should go into usart_common_f124.c (F1 no OVER8, F2, F4, L1) or even usart_common_v2.c (F0, F3, F7, G0, G4, H7, L0, L4). Were there any specific reasons why you duplicated your `usart_set_oversampling()` to both sources? Should I? (I originally aimed at F4 only, but the logic for others could be shared, like F723 in `stlinkv3`)
ALTracer (Migrated from github.com) reviewed 2024年08月19日 17:50:40 +02:00
@ -94,6 +94,39 @@ void usart_set_baudrate(uint32_t usart, uint32_t baud)
#endif
ALTracer (Migrated from github.com) commented 2024年08月19日 17:50:40 +02:00
Copy link

Added. As long as USART kernel clock is below 2147 MHz, this multiplication should not overflow uint32_t (contrary to LPUART), and APB clocks are usually around 48-168 MHz (caveat dynamic kernel clocksource switching)

Added. As long as USART kernel clock is below 2147 MHz, this multiplication should not overflow `uint32_t` (contrary to LPUART), and APB clocks are usually around 48-168 MHz (caveat dynamic kernel clocksource switching)
dragonmux left a comment
Copy link

This all LGTM now. Merging - please update blackmagic-debug/blackmagic#1864 accordingly as you'll need to update the submodule to point to the new commits in main here to make use of them and not duplicate logic into BMD. Thank you for the contribution!

This all LGTM now. Merging - please update blackmagic-debug/blackmagic#1864 accordingly as you'll need to update the submodule to point to the new commits in `main` here to make use of them and not duplicate logic into BMD. Thank you for the contribution!
@ -94,6 +94,39 @@ void usart_set_baudrate(uint32_t usart, uint32_t baud)
#endif

We did it that way to match the organisation of other similar functions in the code, and so we'd have access to that LIBOPENCM3_USART_COMMON_F24_H macro to guard it's definition on F1 parts.

We did it that way to match the organisation of other similar functions in the code, and so we'd have access to that LIBOPENCM3_USART_COMMON_F24_H macro to guard it's definition on F1 parts.
Sign in to join this conversation.
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
blackmagic-debug/libopencm3!5
Reference in a new issue
blackmagic-debug/libopencm3
No description provided.
Delete branch "feature/usart_get_baudrate"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?