Skip to content
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

feat(d1): allocate DMA channels from the DMAC #280

Merged
merged 5 commits into from
Sep 16, 2023
Merged

Conversation

hawkw
Copy link
Contributor

@hawkw hawkw commented Sep 16, 2023

Currently, the assignment of D1 DMAC channels to drivers is somewhat ad-hoc. We hard-code channel 0 as belonging to the UART, and channel 1 as belonging to the SPI_DBI driver, with the interrupt handler for DMAC IRQs specifically dispatching those channel's IRQs to those drivers manually.

There are some issues with this. The exclusive use of DMA channels is not enforced by the current API, because you can (unsafely) summon_channel whenever you want and use a DMA channel that someone else is already using. Furthermore, adding new drivers that also use DMA channels requires hard-coding those additional channel IRQs in the interrupt handler, which must then be kept in sync with the channels actually used by the driver. Also, when a driver wants to wait asynchronously on a DMA transfer, it has to ensure that the DMAC ISR will wake its hard-coded WaitCell, and the driver code is responsible for manually pre-registering its waker with that WaitCell before actually starting the DMA request. This is unfortunate, and it would be nicer if there was an async fn for running DMA transfers that the driver could call without having to repeat all of that code.

This branch improves the way we use the D1's DMAC. In particular, rather than hard-coding specific driver-provided WaitCells to correspond to specific DMA channels, we now have the Dmac struct track which channels are in use, and allow drivers to allocate any channel from the DMAC. It will provide them with the next available channel if any are not already claimed. The DMAC now owns a WaitCell for each DMA channel, which is used to provide an async Channel::run_descriptor method. This method handles pre-registering the waker, starting the DMA request, and ensuring the task is woken, and stops the DMA transfer when it's complete.

The allocation of channels by the DMAC means that adding new drivers that also use DMA is easier: we don't need to track who owns which channel. If we don't run a specific driver based on config, any DMA channels it would use are now available to other drivers, rather than being hardcoded. This way, we can have more than 16 driver implementations that use DMA, as long as we don't enable them all at once.

In a follow-up, I'm also going to add dynamic allocation of DMA channels. Currently, DMA channels are still allocated permanently, and drivers allocate them as soon as they're created and hold onto the channel forever. However, we can modify the Channel type to also indicate to the DMAC that it's free when the Channel is dropped, allowing it to be reallocated (potentially by a different driver). If all 16 DMA channels are in use at the same time, a driver could then wait until one becomes available. If drivers then make more short-lived claims on DMA channels, rather than holding one channel for their entire lifetime, this would thenallow us to have more than 16 driver implementations which use DMA, or allow driver implementations to freely use multiple DMA channels at the same time, without having to worry about running out of channels.

@hawkw hawkw marked this pull request as ready for review September 16, 2023 20:55
Copy link
Contributor

@jamesmunns jamesmunns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One big-picture note and a nit or two, but overall looks much better!

}

static DMAC_STATE: DmacState = {
// This `const` is used as a static initializer, so clippy is wrong here...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have a few of these, if we want a blanket allow at the top of the project, that's cool with me. It might be across multiple crates sadly tho.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think there might be a clippy config file or something that we could use to disable the lint globally, without having to stick an allow attribute in every crate? it's definitely Not My Favorite Clippy Lint...

platforms/allwinner-d1/src/dmac/mod.rs Outdated Show resolved Hide resolved
platforms/allwinner-d1/src/dmac/mod.rs Show resolved Hide resolved
platforms/allwinner-d1/src/dmac/mod.rs Show resolved Hide resolved
hawkw and others added 2 commits September 16, 2023 15:52
Co-authored-by: James Munns <james@onevariable.com>
@hawkw hawkw enabled auto-merge (squash) September 16, 2023 22:58
@hawkw hawkw merged commit 5d0dcc8 into main Sep 16, 2023
10 checks passed
@hawkw hawkw deleted the eliza/dmac-allocator branch September 16, 2023 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants