Feature/stm32 usart wide data #54354

JvanDooren · 2023-02-02T10:23:28Z

drivers: serial: stm32: add wide data support

Add wide data support to STM32 into

interrupt driven code
async code
polling code
DTS init code

Adds 9 bit datawidth support to serial driver.
Validated interrupt driven code on an STM32H743.

Signed-off-by: Jeroen van Dooren [email protected]

erwango

Thanks for this change.

To ease the review process, I'd suggest to split changes into multiple elementary commits.
There are too much changes impacting the driver in a single commit to allow a clear assessment of the change and impact.
For instance:

all changes that are not directly linked to WIDE_DATA introduction should be
split from the changes introducing WIDE_DATA
formating changes should be done in a dedicated commit
...

Also, I suggest submitting the non STM32 related changes in a dedicated PR to get a broader audience.

JvanDooren · 2023-02-02T14:55:59Z

Thanks for this change.

To ease the review process, I'd suggest to split changes into multiple elementary commits. There are too much changes impacting the driver in a single commit to allow a clear assessment of the change and impact. For instance:
* all changes that are not directly linked to WIDE_DATA introduction should be
  split from the changes introducing WIDE_DATA

* formating changes should be done in a dedicated commit

* ...
Also, I suggest submitting the non STM32 related changes in a dedicated PR to get a broader audience.

I guess you're addressing the dts binding change? I made that change because I saw we're slowly moving from runtime configuration to devicetree configuration and these were missing. Shall I split that off into a dedicated PR and keep the change here? Because this PR doesn't build without it.

erwango · 2023-02-02T15:04:05Z

I guess you're addressing the dts binding change? I made that change because I saw we're slowly moving from runtime configuration to devicetree configuration and these were missing. Shall I split that off into a dedicated PR and keep the change here?

DTS indeed but also test changes (which are btw welcome so that we are able to verify functional status).

Because this PR doesn't build without it.

Sure, but you can put them in a stand alone PR which commits are also included in current PR.
Same with tests btw.
Once dts and tests PR are accepted current PR could be rebased.

JvanDooren · 2023-02-03T08:56:30Z

I guess you're addressing the dts binding change? I made that change because I saw we're slowly moving from runtime configuration to devicetree configuration and these were missing. Shall I split that off into a dedicated PR and keep the change here?

DTS indeed but also test changes (which are btw welcome so that we are able to verify functional status).

Because this PR doesn't build without it.

Sure, but you can put them in a stand alone PR which commits are also included in current PR. Same with tests btw. Once dts and tests PR are accepted current PR could be rebased.

Added another PR for only the binding change: binding

erwango · 2023-02-03T10:18:54Z

Added another PR for only the binding change: #54400

Thanks.

Note that my comment regarding the driver changes still stands.

JvanDooren · 2023-02-03T14:55:16Z

Added another PR for only the binding change: #54400

Thanks.

Note that my comment regarding the driver changes still stands.

Thanks.
About the formatting changes, all changes I did relate to WIDE support, and I formatted those touched parts with clang (actually the whole file should be formatted once because I had to revert a lot of lines changed).
What is then left to do?

erwango · 2023-02-03T15:07:32Z

Added another PR for only the binding change: #54400

Thanks.
Note that my comment regarding the driver changes still stands.

Thanks. About the formatting changes, all changes I did relate to WIDE support, and I formatted those touched parts with clang (actually the whole file should be formatted once because I had to revert a lot of lines changed). What is then left to do?

To make it simple:
You impact code which is not linked to WIDE_DATA feature (such as changes to uart_stm32_poll_out), this should be done in a specific commit (with a commit message which explains the reason of the change).
Also, you're introducing poll_out_fn and poll_in_f type_def. Why is that required ? A dedicated commit with a proper justification is needed...
Then there will be a commit with WIDE_DATA specific bits (roughly all the code under #ifdef CONFIG_UART_WIDE_DATA)
Note that this is the minimum split. Depending of the nature of the changes more commits might be required.

JvanDooren · 2023-02-06T08:55:23Z

Added another PR for only the binding change: #54400

Thanks.
Note that my comment regarding the driver changes still stands.

Thanks. About the formatting changes, all changes I did relate to WIDE support, and I formatted those touched parts with clang (actually the whole file should be formatted once because I had to revert a lot of lines changed). What is then left to do?

To make it simple: You impact code which is not linked to WIDE_DATA feature (such as changes to uart_stm32_poll_out), this should be done in a specific commit (with a commit message which explains the reason of the change). Also, you're introducing poll_out_fn and poll_in_f type_def. Why is that required ? A dedicated commit with a proper justification is needed... Then there will be a commit with WIDE_DATA specific bits (roughly all the code under #ifdef CONFIG_UART_WIDE_DATA) Note that this is the minimum split. Depending of the nature of the changes more commits might be required.

Ok, splitted the single commit into 4 separate ones.

erwango · 2023-06-29T21:44:56Z

DNM until author is back (@JvanDooren don't hesitate to remove label when needed)

benner · 2023-07-23T05:59:44Z

Nice initiative @JvanDooren. Can't wait for it to be merged.

heinwessels · 2023-08-16T15:16:36Z

Rebased, and fixed small conflict to test_uart_async.c. I ran the async tests on the nucleo_h743zi and the async tests pass. Will wait for original author to come back from vacation to verify if that's all that's needed, because he mentioned that there's a lot of work to be done to get catch up to main. If this is still the case might drop this PR again. Simply trying to nudge it forward.

scripts/twister -c -T tests/drivers/uart/uart_async_api --platform nucleo_h743zi --device-testing --device-serial /dev/ttyACM0 --fixture gpio_loopback

INFO    - 4/5 nucleo_h743zi             tests/drivers/uart/uart_async_api/drivers.uart.async_api PASSED (device 11.958s)
INFO    - 5/5 nucleo_h743zi             tests/drivers/uart/uart_async_api/drivers.uart.async_api.wide PASSED (device 12.373s)

…ation Preventing code duplication of macros checking for HW support on stop bits and data-bits during runtime configuration. Validated runtime configuration on an STM32H743. Signed-off-by: Jeroen van Dooren <[email protected]>

Restructuring code for poll_in/poll_out/fifo_fill/fifo_read because for wide data support, all code is identical except the calls to LL_USART_{ReceiveData8/TransmitData8}. This allows both implementations, 8 and 9 bit data-width to call a visitor function, passing the either the 8 bit or 9 bit data-width function pointer. Signed-off-by: Jeroen van Dooren <[email protected]>

Add wide data support to STM32. Validated interrupt driven code on an STM32H743. Signed-off-by: Jeroen van Dooren <[email protected]>

Moving struct in preparation of wide test. Signed-off-by: Jeroen van Dooren <[email protected]>

Extending configuration tests with wide data support. Signed-off-by: Jeroen van Dooren <[email protected]>

Resetting uart state before every testcase. Signed-off-by: Jeroen van Dooren <[email protected]>

Extending async tests with wide data support. Signed-off-by: Jeroen van Dooren <[email protected]>

JvanDooren · 2023-08-29T06:03:18Z

Rebased, and fixed small conflict to test_uart_async.c. I ran the async tests on the nucleo_h743zi and the async tests pass. Will wait for original author to come back from vacation to verify if that's all that's needed, because he mentioned that there's a lot of work to be done to get catch up to main. If this is still the case might drop this PR again. Simply trying to nudge it forward.
scripts/twister -c -T tests/drivers/uart/uart_async_api --platform nucleo_h743zi --device-testing --device-serial /dev/ttyACM0 --fixture gpio_loopback
INFO    - 4/5 nucleo_h743zi             tests/drivers/uart/uart_async_api/drivers.uart.async_api PASSED (device 11.958s)
INFO    - 5/5 nucleo_h743zi             tests/drivers/uart/uart_async_api/drivers.uart.async_api.wide PASSED (device 12.373s)

Nope, the tests were basically the main culprit. So should be fine if all tests pass.

JvanDooren · 2023-08-29T06:09:50Z

DNM until author is back (@JvanDooren don't hesitate to remove label when needed)

Doesn't seem I'm authorized to edit labels?

heinwessels · 2023-08-29T09:31:34Z

@erwango As far as I can see all comments are addressed. So now we only have to wait for approvals.

erwango · 2023-08-29T09:45:51Z

@erwango As far as I can see all comments are addressed. So now we only have to wait for approvals.

For the tests part, I'd like @dcpleung to approve

dcpleung

Changes in tests LGTM.

JvanDooren requested review from ABOSTM, FRASTM, erwango, galak and nashif as code owners February 2, 2023 10:23

zephyrbot added area: UART Universal Asynchronous Receiver-Transmitter area: Devicetree Binding PR modifies or adds a Device Tree binding platform: STM32 ST Micro STM32 labels Feb 2, 2023

zephyrbot requested review from GeorgeCGV, dcpleung and gautierg-st February 2, 2023 10:24

zephyrbot assigned dcpleung Feb 2, 2023

erwango added this to the v3.4.0 milestone Feb 2, 2023

erwango requested changes Feb 2, 2023

View reviewed changes

JvanDooren force-pushed the feature/stm32_usart_wide_data branch from 95a6d97 to 957170f Compare February 2, 2023 14:51

JvanDooren force-pushed the feature/stm32_usart_wide_data branch from 957170f to 06b5040 Compare February 2, 2023 14:57

JvanDooren force-pushed the feature/stm32_usart_wide_data branch from 06b5040 to 65d5fcb Compare February 3, 2023 08:46

JvanDooren force-pushed the feature/stm32_usart_wide_data branch from 65d5fcb to e8468ec Compare February 3, 2023 14:51

JvanDooren force-pushed the feature/stm32_usart_wide_data branch from e8468ec to 4c2e703 Compare February 3, 2023 15:50

dcpleung assigned erwango and unassigned dcpleung Feb 3, 2023

JvanDooren force-pushed the feature/stm32_usart_wide_data branch from 4c2e703 to 1545dee Compare February 6, 2023 08:54

JvanDooren removed the request for review from nashif February 6, 2023 08:55

erwango added the DNM This PR should not be merged (Do Not Merge) label Jun 29, 2023

heinwessels dismissed erwango’s stale review via 0cad81f August 16, 2023 15:10

heinwessels force-pushed the feature/stm32_usart_wide_data branch from a46fe73 to 0cad81f Compare August 16, 2023 15:10

heinwessels force-pushed the feature/stm32_usart_wide_data branch from 0cad81f to 3c801b5 Compare August 17, 2023 09:37

JvanDooren added 7 commits August 25, 2023 15:38

drivers: serial: stm32: add wide data support

ae486de

Add wide data support to STM32. Validated interrupt driven code on an STM32H743. Signed-off-by: Jeroen van Dooren <[email protected]>

tests: drivers: uart: moving struct declaration

099331e

Moving struct in preparation of wide test. Signed-off-by: Jeroen van Dooren <[email protected]>

tests: drivers: uart: Adding wide data cfg tests

1187f4e

Extending configuration tests with wide data support. Signed-off-by: Jeroen van Dooren <[email protected]>

tests: drivers: uart: Resetting uart state

cad43aa

Resetting uart state before every testcase. Signed-off-by: Jeroen van Dooren <[email protected]>

tests: drivers: uart: Adding wide data async tests

cd89942

Extending async tests with wide data support. Signed-off-by: Jeroen van Dooren <[email protected]>

heinwessels force-pushed the feature/stm32_usart_wide_data branch from 3c801b5 to cd89942 Compare August 25, 2023 13:52

erwango removed the DNM This PR should not be merged (Do Not Merge) label Aug 29, 2023

gautierg-st approved these changes Aug 29, 2023

View reviewed changes

erwango requested a review from dcpleung August 29, 2023 09:44

erwango self-requested a review August 29, 2023 11:50

dcpleung approved these changes Aug 29, 2023

View reviewed changes

erwango approved these changes Aug 30, 2023

View reviewed changes

erwango requested review from fabiobaltieri and jhedberg August 30, 2023 06:28

carlescufi merged commit 772e6fd into zephyrproject-rtos:main Aug 30, 2023

JvanDooren deleted the feature/stm32_usart_wide_data branch August 30, 2023 09:22

erwango mentioned this pull request Aug 30, 2023

drivers: serial: stm32: Refactor for PM handling #59406

Merged

Feature/stm32 usart wide data #54354

Feature/stm32 usart wide data #54354

Uh oh!

Conversation

JvanDooren commented Feb 2, 2023

Uh oh!

erwango left a comment

Choose a reason for hiding this comment

Uh oh!

JvanDooren commented Feb 2, 2023

Uh oh!

erwango commented Feb 2, 2023

Uh oh!

JvanDooren commented Feb 3, 2023

Uh oh!

erwango commented Feb 3, 2023

Uh oh!

JvanDooren commented Feb 3, 2023

Uh oh!

erwango commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JvanDooren commented Feb 6, 2023

Uh oh!

erwango commented Jun 29, 2023

Uh oh!

benner commented Jul 23, 2023

Uh oh!

heinwessels commented Aug 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JvanDooren commented Aug 29, 2023

Uh oh!

JvanDooren commented Aug 29, 2023

Uh oh!

heinwessels commented Aug 29, 2023

Uh oh!

erwango commented Aug 29, 2023

Uh oh!

dcpleung left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

erwango commented Feb 3, 2023 •

edited

Loading

heinwessels commented Aug 16, 2023 •

edited

Loading