-
Notifications
You must be signed in to change notification settings - Fork 8k
soc: nordic: Add driver for Nordic MVDMA peripheral #95912
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused by this PR to be honest, it is adding the driver to the DMA drivers, but it is not in fact a DMA driver from Zephyr's perspective. I'm not sure exactly where this should go, but I do not believe it should go into drivers/dma.
To be clear, I'm not at all opposed to a vendor creating their own APIs and drivers around DMA, as noted already in the documentation the DMA API is a bit of a union of DMA peripheral capabilities. If DMA-like actions are wanted in a cross platform way we already provide this in peripheral APIs in part. But to put this driver in drivers/dma should imply it implements the dma.h interface which it does not.
@teburd I was also not sure if that is a good location. It can as well be placed in |
The following west manifest projects have changed revision in this Pull Request:
✅ All manifest checks OK Note: This message is automatically posted and updated by the Manifest GitHub Action. |
9815803
to
8a8510d
Compare
I think this is worth discussing in the Architecture WG meeting to ensure that @nordic-krch has a chance to explain why the common DMA API is not suitable in this case and an internal variant is required. |
I guess the only question I have is why not have this live in hal_nrf or whatever, I guess because you’d still need some DT/Kconfig integration which would be missing? |
Add MVDMA binding and nodes to nrf54h20. Signed-off-by: Krzysztof Chruściński <[email protected]>
44c439f
to
998c510
Compare
Add function for getting and putting a lock for all power states. It is much faster version of requesting 0 us latency with actual intention to disable all power states. Signed-off-by: Krzysztof Chruściński <[email protected]>
yes, we keep Zephyr agnostic stuff in hal_nordic. It's Kconfig/DTS but also driver needs to use Zephyr API inside (block PM states when MVDMA is operating). |
MVDMA driver does not implement Zephyr DMA API. It implements its own due to performance reasons and it is intended to be used mainly in the Nordic specific code. Signed-off-by: Krzysztof Chruściński <[email protected]>
MVDMA is present in the application domain so it's configuration need to be restored when waking up from suspend to RAM. Signed-off-by: Krzysztof Chruściński <[email protected]>
Add test suite for MVDMA driver. Signed-off-by: Krzysztof Chruściński <[email protected]>
|
@teburd and also because this driver uses Zephyr primitives, and drivers in HALs should be OS-neutral, otherwise we have a dependency in both directions. |
*/ | ||
void pm_policy_state_lock_put(enum pm_state state, uint8_t substate_id); | ||
|
||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please split this into own PR including PM changes only, making this part of a driver PR does not give it the visibility and reviews it deserves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is there: #96909
Is this driver in any way accessible to an application or usable by an application? If yes then I'd say I still have a concern here that you are going to introduce a nordic specific API for what could be a broadly useful function of asynchronous memcpy/zeroing, where "how its done" could be soc specific and still avoid the cost of dealing with the DMA APIs and its descriptors. If no, consider doing this instead. I think it'd be a great feature, and could internally use mvdma or other soc specifics (e.g. a dedicated dma channel in other scenarios) without needing to fully expose the DMA which often has varying behavior across vendors. |
Currently we don't see a use case. Application operates on "fast" RAM. For memory-to-memory operations time needed to setup a MVDMA will be longer than memory operation itself. nRF54H20 SoC has "slow" RAM that is used by peripherals and main use case for MVDMA is to copy data between "fast/user" memory and "slow" which happens in drivers. |
Architecture WG meeting:
|
Driver does not implement Zephyr DMA API. The reason is that it will mainly be used in the vendor specific code and translation to the Zephyr API significantly decreases the performance to a point where using DMA is no longer beneficial.
Driver offers free running transfer setup and a transfer with a callback. If free running mode is used (without a callback) then
nrf_mvdma_xfer_check
can be used to poll for transfer completion. It is required to poll until function returns success (transfer has finished). That is because MVDMA blocks entering S2RAM PM mode and unblocking is done only when transfer completes.Test contains timings measurements that can be used to decide whether use of MVDMA is beneficial.
DNM as it requires #96909.