drivers: serial: nrfx_uarte: Rework driver to support new features #78887
+489
−288
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zephyrbot
added
area: UART
zephyrbot
requested review from
dcpleung,
jaz1-nordic,
kl-cruz,
magp-nordic and
nika-nordic
nordic-krch
force-pushed
the
nrfx_uarte_rework
branch
2 times, most recently
from
c140ae3
to
2bc614a
Compare
bjarki-andreasen
previously approved these changes
Sep 25, 2024
nordic-krch
force-pushed
the
nrfx_uarte_rework
branch
2 times, most recently
from
6279c82
to
863a6e2
Compare
nordic-krch
added a commit
to nordic-krch/fw-nrfconnect-zephyr
that referenced
this pull request
Oct 7, 2024
…struct pointer Refactor RX asynchronous API function to use a pointer to the RX async data structure instead of top level data structure pointer. It improves readability with more concise code. Upstream PR: zephyrproject-rtos/zephyr#78887 Signed-off-by: Krzysztof Chruściński <[email protected]>
nordic-krch
added a commit
to nordic-krch/fw-nrfconnect-zephyr
that referenced
this pull request
Oct 7, 2024
…new features Rework driver to support new way of asynchronous RX handling. Previously RX was handled in two modes: using RXDRDY interrupt for byte counting or TIMER + PPI. Both modes had flaws. RXDRDY interrupt mode could miscalculated amount of received bytes when interrupt was not handled on time. Data was not lost but was not reported on time that could lead to issues. PPI+TIMER mode requires additional resources thus it was not the default mode. Often user was not aware of that option and was expiriencing driver RX faults. New RX mode is switching buffers when there is new data (RXDRDY event not set for given amount of time). It does not require additional resources to get precise byte counting. Additionally, this is in line with new UARTE feature (RX frame timeout) which is present in nRF54X devices. The behavior of the driver is the same for legacy devices and new one. For legacy devices k_timer periodic interrupts are used to check if there are any new bytes and it is not needed when RX frame timeout is present. Improved RX mode is enabled by default (CONFIG_UART_NRFX_UARTE_ENHANCED_RX=y) but legacy modes are still available though not recommended to be used. Note that new RX mode behaves a bit different because timeout always triggers switch of buffers which means that there will be no UART_RX_RDY events with non-zero offset. It also means that every UART_RX_RDY will be followed by UART_RX_BUF_RELEASED. After rework, driver is recommended to be used for all platforms as it performs much better and takes much less code than the second UART shim available for Nordic devices. Upstream PR: zephyrproject-rtos/zephyr#78887 Signed-off-by: Krzysztof Chruściński <[email protected]>
nordic-krch
added a commit
to nordic-krch/fw-nrfconnect-zephyr
that referenced
this pull request
Oct 7, 2024
…eout Frame timeout is a hardware feature present in newer versions of UARTE (e.g. in NRF54X platforms) for detecting idle state on RX line and ending RX after configurable timeout. Upstream PR: zephyrproject-rtos/zephyr#78887 Signed-off-by: Krzysztof Chruściński <[email protected]>
nordic-krch
added a commit
to nordic-krch/zephyr
that referenced
this pull request
Oct 8, 2024
…struct pointer Refactor RX asynchronous API function to use a pointer to the RX async data structure instead of top level data structure pointer. It improves readability with more concise code. Upstream PR: zephyrproject-rtos#78887 Signed-off-by: Krzysztof Chruściński <[email protected]>
nordic-krch
added a commit
to nordic-krch/zephyr
that referenced
this pull request
Oct 8, 2024
…new features Rework driver to support new way of asynchronous RX handling. Previously RX was handled in two modes: using RXDRDY interrupt for byte counting or TIMER + PPI. Both modes had flaws. RXDRDY interrupt mode could miscalculated amount of received bytes when interrupt was not handled on time. Data was not lost but was not reported on time that could lead to issues. PPI+TIMER mode requires additional resources thus it was not the default mode. Often user was not aware of that option and was expiriencing driver RX faults. New RX mode is switching buffers when there is new data (RXDRDY event not set for given amount of time). It does not require additional resources to get precise byte counting. Additionally, this is in line with new UARTE feature (RX frame timeout) which is present in nRF54X devices. The behavior of the driver is the same for legacy devices and new one. For legacy devices k_timer periodic interrupts are used to check if there are any new bytes and it is not needed when RX frame timeout is present. Improved RX mode is enabled by default (CONFIG_UART_NRFX_UARTE_ENHANCED_RX=y) but legacy modes are still available though not recommended to be used. Note that new RX mode behaves a bit different because timeout always triggers switch of buffers which means that there will be no UART_RX_RDY events with non-zero offset. It also means that every UART_RX_RDY will be followed by UART_RX_BUF_RELEASED. After rework, driver is recommended to be used for all platforms as it performs much better and takes much less code than the second UART shim available for Nordic devices. Upstream PR: zephyrproject-rtos#78887 Signed-off-by: Krzysztof Chruściński <[email protected]>
nordic-krch
added a commit
to nordic-krch/zephyr
that referenced
this pull request
Oct 8, 2024
…eout Frame timeout is a hardware feature present in newer versions of UARTE (e.g. in NRF54X platforms) for detecting idle state on RX line and ending RX after configurable timeout. Upstream PR: zephyrproject-rtos#78887 Signed-off-by: Krzysztof Chruściński <[email protected]>
nordic-krch
added a commit
to nordic-krch/fw-nrfconnect-zephyr
that referenced
this pull request
Oct 9, 2024
…struct pointer Refactor RX asynchronous API function to use a pointer to the RX async data structure instead of top level data structure pointer. It improves readability with more concise code. Upstream PR: zephyrproject-rtos/zephyr#78887 Signed-off-by: Krzysztof Chruściński <[email protected]>
nordic-krch
added a commit
to nordic-krch/fw-nrfconnect-zephyr
that referenced
this pull request
Oct 9, 2024
…new features Rework driver to support new way of asynchronous RX handling. Previously RX was handled in two modes: using RXDRDY interrupt for byte counting or TIMER + PPI. Both modes had flaws. RXDRDY interrupt mode could miscalculated amount of received bytes when interrupt was not handled on time. Data was not lost but was not reported on time that could lead to issues. PPI+TIMER mode requires additional resources thus it was not the default mode. Often user was not aware of that option and was expiriencing driver RX faults. New RX mode is switching buffers when there is new data (RXDRDY event not set for given amount of time). It does not require additional resources to get precise byte counting. Additionally, this is in line with new UARTE feature (RX frame timeout) which is present in nRF54X devices. The behavior of the driver is the same for legacy devices and new one. For legacy devices k_timer periodic interrupts are used to check if there are any new bytes and it is not needed when RX frame timeout is present. Improved RX mode is enabled by default (CONFIG_UART_NRFX_UARTE_ENHANCED_RX=y) but legacy modes are still available though not recommended to be used. Note that new RX mode behaves a bit different because timeout always triggers switch of buffers which means that there will be no UART_RX_RDY events with non-zero offset. It also means that every UART_RX_RDY will be followed by UART_RX_BUF_RELEASED. After rework, driver is recommended to be used for all platforms as it performs much better and takes much less code than the second UART shim available for Nordic devices. Upstream PR: zephyrproject-rtos/zephyr#78887 Signed-off-by: Krzysztof Chruściński <[email protected]>
nordic-krch
added a commit
to nordic-krch/fw-nrfconnect-zephyr
that referenced
this pull request
Oct 9, 2024
…eout Frame timeout is a hardware feature present in newer versions of UARTE (e.g. in NRF54X platforms) for detecting idle state on RX line and ending RX after configurable timeout. Upstream PR: zephyrproject-rtos/zephyr#78887 Signed-off-by: Krzysztof Chruściński <[email protected]>
bjarki-andreasen
previously approved these changes
Oct 10, 2024
kl-cruz
previously approved these changes
Oct 10, 2024
nordic-krch
added a commit
to nordic-krch/fw-nrfconnect-zephyr
that referenced
this pull request
Oct 11, 2024
…struct pointer Refactor RX asynchronous API function to use a pointer to the RX async data structure instead of top level data structure pointer. It improves readability with more concise code. Upstream PR: zephyrproject-rtos/zephyr#78887 Signed-off-by: Krzysztof Chruściński <[email protected]>
nordic-krch
added a commit
to nordic-krch/fw-nrfconnect-zephyr
that referenced
this pull request
Oct 11, 2024
…new features Rework driver to support new way of asynchronous RX handling. Previously RX was handled in two modes: using RXDRDY interrupt for byte counting or TIMER + PPI. Both modes had flaws. RXDRDY interrupt mode could miscalculated amount of received bytes when interrupt was not handled on time. Data was not lost but was not reported on time that could lead to issues. PPI+TIMER mode requires additional resources thus it was not the default mode. Often user was not aware of that option and was expiriencing driver RX faults. New RX mode is switching buffers when there is new data (RXDRDY event not set for given amount of time). It does not require additional resources to get precise byte counting. Additionally, this is in line with new UARTE feature (RX frame timeout) which is present in nRF54X devices. The behavior of the driver is the same for legacy devices and new one. For legacy devices k_timer periodic interrupts are used to check if there are any new bytes and it is not needed when RX frame timeout is present. Improved RX mode is enabled by default (CONFIG_UART_NRFX_UARTE_ENHANCED_RX=y) but legacy modes are still available though not recommended to be used. Note that new RX mode behaves a bit different because timeout always triggers switch of buffers which means that there will be no UART_RX_RDY events with non-zero offset. It also means that every UART_RX_RDY will be followed by UART_RX_BUF_RELEASED. After rework, driver is recommended to be used for all platforms as it performs much better and takes much less code than the second UART shim available for Nordic devices. Upstream PR: zephyrproject-rtos/zephyr#78887 Signed-off-by: Krzysztof Chruściński <[email protected]>
nordic-krch
added a commit
to nordic-krch/fw-nrfconnect-zephyr
that referenced
this pull request
Oct 11, 2024
…eout Frame timeout is a hardware feature present in newer versions of UARTE (e.g. in NRF54X platforms) for detecting idle state on RX line and ending RX after configurable timeout. Upstream PR: zephyrproject-rtos/zephyr#78887 Signed-off-by: Krzysztof Chruściński <[email protected]>
|
@masz-nordic can you take a look? |
masz-nordic
previously approved these changes
Oct 14, 2024
drivers/serial/uart_nrfx_uarte.c
Outdated
| uint32_t divisor = 1000000 / timeout; | ||
| uint32_t bauds = baudrate / divisor; | ||
|
|
||
| return MIN(bauds, UARTE_FRAMETIMEOUT_COUNTERTOP_Msk); |
There was a problem hiding this comment.
Should be replaced with HAL define in next nrfx release.
|
@dcpleung can you take a look? |
anangl
requested changes
Oct 15, 2024
drivers/serial/uart_nrfx_uarte.c
Outdated
Comment on lines
855
to
856
| uint32_t divisor = 1000000 / timeout; | ||
| uint32_t bauds = baudrate / divisor; |
There was a problem hiding this comment.
For timeout >= 1 second, you'll get division by 0 here.
I think it will be safer to involve 64 bits here: bauds = (uin64_t)baudrate * timeout / 1000000;
drivers/serial/uart_nrfx_uarte.c
Outdated
| @@ -628,36 +648,43 @@ static int uarte_nrfx_rx_counting_init(const struct device *dev) | |||
|
|
|||
| nrfx_gppi_channel_endpoints_setup(data->async->rx.cnt.ppi, evt_addr, tsk_addr); | |||
| nrfx_gppi_channels_enable(BIT(data->async->rx.cnt.ppi)); | |||
| if (ret != NRFX_SUCCESS) { | |||
There was a problem hiding this comment.
It does not seem possible for ret to not be NRFX_SUCCESS at this point:
zephyr/drivers/serial/uart_nrfx_uarte.c
Lines 642 to 651 in 609b088
drivers/serial/uart_nrfx_uarte.c
Outdated
| @@ -834,8 +857,14 @@ static int uarte_nrfx_rx_enable(const struct device *dev, uint8_t *buf, | |||
| return -EBUSY; | |||
| } | |||
|
|
|||
| #ifdef CONFIG_UART_NRFX_UARTE_ENHANCED_RX | |||
| rdata->timeout = (timeout && timeout == SYS_FOREVER_US) ? | |||
There was a problem hiding this comment.
This timeout && part is unneeded. I'm not sure what was the intention here (timeout == 0 || ?).
There was a problem hiding this comment.
Removed that. I've initially assumed that timeout==0 is the same as SYS_FOREVER_US but it is not stated in API so I removed it.
| @@ -856,10 +886,18 @@ static int uarte_nrfx_rx_enable(const struct device *dev, uint8_t *buf, | |||
| */ | |||
| if (!len) { | |||
| rdata->flush_cnt -= cpy_len; | |||
| notify_uart_rx_rdy(dev, cpy_len); | |||
| rx_buf_release(dev, &rdata->buf); | |||
| notify_rx_disable(dev); | |||
There was a problem hiding this comment.
As RX no longer gets disabled here, the comment right above this block should be updated.
There was a problem hiding this comment.
Comment updated.
drivers/serial/uart_nrfx_uarte.c
Outdated
Comment on lines
2115
to
2116
| uarte_disable_locked(dev, UARTE_FLAG_LOW_POWER_TX, UARTE_FLAG_LOW_POWER_RX); | ||
| uarte_disable_locked(dev, UARTE_FLAG_LOW_POWER_RX, UARTE_FLAG_LOW_POWER_TX); |
There was a problem hiding this comment.
Why not just:
Suggested change
| uarte_disable_locked(dev, UARTE_FLAG_LOW_POWER_TX, UARTE_FLAG_LOW_POWER_RX); | |
| uarte_disable_locked(dev, UARTE_FLAG_LOW_POWER_RX, UARTE_FLAG_LOW_POWER_TX); | |
| uarte_disable_locked(dev, UARTE_FLAG_LOW_POWER_TX | UARTE_FLAG_LOW_POWER_RX, 0); |
drivers/serial/uart_nrfx_uarte.c
Outdated
| @@ -123,7 +137,7 @@ struct uarte_async_rx { | |||
| } cnt; | |||
| /* Flag to ensure that RX timeout won't be executed during ENDRX ISR */ | |||
| volatile bool is_in_irq; | |||
| uint8_t flush_buffer[UARTE_HW_RX_FIFO_SIZE]; | |||
| #endif | |||
There was a problem hiding this comment.
Suggested change
| #endif | |
| #endif /* CONFIG_UART_NRFX_UARTE_ENHANCED_RX */ |
| #if !defined(UARTE_HAS_FRAME_TIMEOUT) | ||
| uint32_t idle_cnt; | ||
| #endif | ||
| k_timeout_t timeout; |
There was a problem hiding this comment.
For !defined(CONFIG_UART_NRFX_UARTE_ENHANCED_RX), a member with the same name but a different type is used. This is quite confusing. It would be good to rename one of them (it seems easier for the new one, to k_timeout maybe).
There was a problem hiding this comment.
changed to timeout_us for !defined(CONFIG_UART_NRFX_UARTE_ENHANCED_RX) case.
drivers/serial/uart_nrfx_uarte.c
Outdated
| uint8_t ppi_ch_endtx; | ||
| #endif | ||
| atomic_t flags; |
There was a problem hiding this comment.
nit: place it before ppi_ch_endtx to avoid padding.
drivers/serial/uart_nrfx_uarte.c
Outdated
|
|
||
| /* If enabled then ENDTX is PPI'ed to TXSTOP */ | ||
| #define UARTE_CFG_FLAG_PPI_ENDTX BIT(0) | ||
|
|
||
| /* If enabled then TIMER and PPI is used for byte counting. */ | ||
| #define UARTE_CFG_FLAG_HW_BYTE_COUNTING BIT(1) | ||
|
|
||
| /* If enabled then cache management is required (except for dmm buffers). */ | ||
| #define UARTE_CFG_FLAG_CACHEABLE BIT(3) |
There was a problem hiding this comment.
This flag doesn't seem to be used.
drivers/serial/uart_nrfx_uarte.c
Outdated
| #else | ||
| rdata->timeout = (timeout && timeout == SYS_FOREVER_US) ? | ||
| K_NO_WAIT : K_USEC(timeout / RX_TIMEOUT_DIV); | ||
| rdata->idle_cnt = RX_TIMEOUT_DIV - 1; |
There was a problem hiding this comment.
- Why
- 1? Timeout is split intoRX_TIMEOUT_DIVparts and theSTOPRXtask is triggered when this counter is decreased to 0, so one such part earlier. - Wouldn't it be more convenient to initialize this counter to 0 (initialization is done in several places) and check if it reaches certain value before triggering the
STOPRXtask (this happens in one place only). - I got a bit confused. The commit message states that there will be no more
UART_RX_RDYevents with non-zero offsets. But because of this timeout split, it seems to me that they still can occur. Only when the frame timeout feature is used, they cannot, because the timeout is handled differently then. What am I missing?
There was a problem hiding this comment.
Changed to start with 0.
Refactor RX asynchronous API function to use a pointer to the RX async data structure instead of top level data structure pointer. It improves readability with more concise code. Signed-off-by: Krzysztof Chruściński <[email protected]>
nordic-krch
dismissed stale reviews from masz-nordic, kl-cruz, and bjarki-andreasen
via
64e292f
nordic-krch
force-pushed
the
nrfx_uarte_rework
branch
3 times, most recently
from
29d2ee9
to
6288a6a
Compare
Rework driver to support new way of asynchronous RX handling. Previously RX was handled in two modes: using RXDRDY interrupt for byte counting or TIMER + PPI. Both modes had flaws. RXDRDY interrupt mode could miscalculated amount of received bytes when interrupt was not handled on time. Data was not lost but was not reported on time that could lead to issues. PPI+TIMER mode requires additional resources thus it was not the default mode. Often user was not aware of that option and was expiriencing driver RX faults. New RX mode is switching buffers when there is new data (RXDRDY event not set for given amount of time). It does not require additional resources to get precise byte counting. Additionally, this is in line with new UARTE feature (RX frame timeout) which is present in nRF54X devices. The behavior of the driver is the same for legacy devices and new one. For legacy devices k_timer periodic interrupts are used to check if there are any new bytes and it is not needed when RX frame timeout is present. Improved RX mode is enabled by default (CONFIG_UART_NRFX_UARTE_ENHANCED_RX=y) but legacy modes are still available though not recommended to be used. Note that new RX mode behaves a bit different because timeout always triggers switch of buffers which means that there will be no UART_RX_RDY events with non-zero offset. It also means that every UART_RX_RDY will be followed by UART_RX_BUF_RELEASED. After rework, driver is recommended to be used for all platforms as it performs much better and takes much less code than the second UART shim available for Nordic devices. Signed-off-by: Krzysztof Chruściński <[email protected]>
Frame timeout is a hardware feature present in newer versions of UARTE (e.g. in NRF54X platforms) for detecting idle state on RX line and ending RX after configurable timeout. Signed-off-by: Krzysztof Chruściński <[email protected]>
nordic-krch
force-pushed
the
nrfx_uarte_rework
branch
from
6288a6a
to
812e4d8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rework driver to support new way of asynchronous RX handling. Previously RX was handled in two modes: using RXDRDY interrupt for byte counting or TIMER + PPI. Both modes had flaws. RXDRDY interrupt mode could miscalculated amount of received bytes when interrupt was not handled on time. Data was not lost but was not reported on time that could lead to issues. PPI+TIMER mode requires additional resources thus it was not the default mode. Often user was not aware of that option and was expiriencing driver RX faults.
New RX mode is switching buffers when there is new data (RXDRDY event not set for given amount of time). It does not require additional resources to get precise byte counting. Additionally, this is in line with new UARTE feature (RX frame timeout) which is present in nRF54X devices. The behavior of the driver is the same for legacy devices and new one. For legacy devices k_timer periodic interrupts are used to check if there are any new bytes and it is not needed when RX frame
timeout is present.
Improved RX mode is enabled by default (CONFIG_UART_NRFX_UARTE_ENHANCED_RX=y) but legacy modes are still
available though not recommended to be used.
Note that new RX mode behaves a bit different because timeout always triggers switch of buffers which means that there will be no UART_RX_RDY events with non-zero offset. It also means that every UART_RX_RDY will be followed by UART_RX_BUF_RELEASED.
After rework, driver is recommended to be used for all platforms as it performs much better and takes much less code than the second UART shim available for Nordic devices.
This PR is main part of #75462 which contains additional changes and significant changes in tests. It is created for ease of review as #75462 is overwhelming.
It also contains #78886.