nrfx_usbd: Copy nrfx usbd code to Zephyr #64174
Merged
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
carlescufi
reviewed
Oct 20, 2023
carlescufi
requested changes
Oct 20, 2023
tmon-nordic
force-pushed
the
nrfx-usbd
branch
3 times, most recently
from
2fc19e8 to
deb360a
Compare
carlescufi
previously requested changes
Oct 20, 2023
carlescufi
previously approved these changes
Oct 23, 2023
tmon-nordic
force-pushed
the
nrfx-usbd
branch
3 times, most recently
from
69bf6bf to
900ae55
Compare
tmon-nordic
force-pushed
the
nrfx-usbd
branch
from
900ae55 to
094d74d
Compare
Copy nrfx_usbd code from zephyrproject-rtos/hal_nordic git revision d054a315eb888ba70e09e5f6decd4097b0276d1f. This enables us to refactor the code towards a native driver in a step by step manner where the smallest changes that bring biggest performance improvements are done early on. The code is not a vanilla copy from zephyrproject-rtos/hal_nordic. The code was reformated with clang-format to match project style. Manual modifications were done to change comments formatting, place constant comparisons on the right and add blank lines to pass Zephyr compliance check. Relicense to Apache 2.0. No functional changes. Signed-off-by: Tomasz Moń <[email protected]>
tmon-nordic
force-pushed
the
nrfx-usbd
branch
from
094d74d to
40b7a7a
Compare
carlescufi
previously approved these changes
Oct 30, 2023
anangl
reviewed
Oct 30, 2023
tmon-nordic
force-pushed
the
nrfx-usbd
branch
2 times, most recently
from
a64b1df to
0d6772b
Compare
tmon-nordic
force-pushed
the
nrfx-usbd
branch
from
0d6772b to
8552e4b
Compare
carlescufi
previously approved these changes
Oct 30, 2023
jfischer-no
previously approved these changes
Oct 31, 2023
tmon-nordic
added
DNM
anangl
requested changes
Oct 31, 2023
Rename local usbd copy from nrfx_usbd to nrf_usbd_common and use it in both USB stacks. Renaming header to nrf_usbd_common.h allows breaking changes in exposed interface. Mark all doxygen comments as internal because local usbd copy should not be treated as public interface because we are under refactoring process that aims to arrive at native driver and therefore drop nrf_usbd_common in the future. Use Zephyr constructs directly instead of nrfx glue macros. No functional changes. Signed-off-by: Tomasz Moń <[email protected]>
Rely on semaphore to serialize access to DMA instead of busy looping after triggering DMA. With this change Ozone Code Profile generated with J-Trace Pro on nrf52840dk_nrf52840 board running headphones microphone sample shows following Load changes (trace data was reset once playback and recording started and percentages were taken when memcpy reached 200 000 Run Count): * usbd_dmareq_process() from 17.16% to 2.24% * memcpy() from 9.37% to 8.36% * nrf_usbd_common_irq_handler() from 8.89% to 10.88% Mark nrf_usbd_common_stop() as static because the caller must acquire DMA semaphore before calling this function and the only place where it is used is already acquiring the semaphore. Disable EP0 SETUP interrupt when there is active DMA on EP0 to eliminate the need for aborting DMA on EP0. This code path should not really happen in real life though because hosts must not issue new SETUP before a relatively long timeout (at least 50 ms). Signed-off-by: Tomasz Moń <[email protected]>
Early DMA process handling actually decreases performance in real world scenarios because real world scenarios tend to be CPU bound. Moreover the actual optimization is questionable because DMA semaphore can only be released after the respective endpoint end event is handled. Remove early DMA processing altogether and only check pending DMA at the end of interrupt handler. Signed-off-by: Tomasz Moń <[email protected]>
There is no point in calling an empty function on every single DMA transfer start. While the empty function is just BX LR the fact that is is called on every DMA transfer makes its impact visible. This change improves CDC ACM echo throughput by approximately 2%. Signed-off-by: Tomasz Moń <[email protected]>
Ensure strict order within interrupt processing to eliminate workaround in IN transfer acknowledged handler. EPDATASTATUS and DMA done events are processed in a way that eliminates the possibility for race condions between interrupt handler and host IN tokens. Store last started DMA endpoint and use it in common handler for all DMA finished events. Unify use of ep dma waiting variable because atomics only make sense if all accesses are through atomic functions - here the accesses are guarded with critical section. Do not disable SETUP interrupt when DMA transfers data on endpoint 0 but simply postpone handling if currently active DMA is on endpoint 0. Signed-off-by: Tomasz Moń <[email protected]>
Do not enable nor disable endpoint done interrupts at runtime because it is not necessary. Simply keep all relevant interrupts enabled when USBD is active and disable all interrupts when USBD is disabled. Signed-off-by: Tomasz Moń <[email protected]>
There is no point in setting the feeder or consumer at runtime. The time saved due to not checking conditions on each transaction is smaller than the additional cost of calling function via pointer. Signed-off-by: Tomasz Moń <[email protected]>
tmon-nordic
dismissed stale reviews from jfischer-no and carlescufi
via
b0a1808
tmon-nordic
force-pushed
the
nrfx-usbd
branch
from
8552e4b to
b0a1808
Compare
Use MDK directly instead of via USBD HAL to not mix direct register accesses with USBD HAL. In my opinion this change is not really necessary but reviewers are strict about consistency. Signed-off-by: Tomasz Moń <[email protected]>
tmon-nordic
force-pushed
the
nrfx-usbd
branch
from
b0a1808 to
77267c2
Compare
anangl
approved these changes
Nov 3, 2023
jfischer-no
approved these changes
Nov 7, 2023
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.
Copy nrfx_usbd code from zephyrproject-rtos/hal_nordic git revision d054a315eb888ba70e09e5f6decd4097b0276d1f. This enables us to refactor the code towards a native driver in a step by step manner where the smallest changes that bring biggest performance improvements are done early on.