stmhal: Add DMA support for sdcard #1625
Conversation
- added some comments to explain the priority/sub-priority. - adds an entry for SDIO (to be used in a later patch) - increases DMA priority above USB so that DMA can be used for sdcard I/O when using USB Mass Storage.
|
Thanks @dhylands! I think though that calling dma_tick_handler every ms is quite a bit of overhead, don't you think? Some options:
|
|
@chuckbook says in another thread (moved here to keep conversation contained):
Yes, no problem, please make your tests! stmhal's systick is also at the highest prioriyt so it is important to keep that interrupt very lean (I made some suggestions above how to do that). |
Turning on each DMA block increases the current consumption by about 8 mA. This code adds an idle timer for each DMA block and turns off the clocks when no streams are in use for 128 msec. Having a small timeout allows for improved performance when back-to-back transfers are being performed. The 128 msec is basically a guess.
This started out using IgorLektorovEpam work in PR micropython#1389 and reworked it.
|
@dpgeorge I waffled back and forth when I first implemented it. I like the idea of only calling every Nth tick, so I did both that and check if its enabled. I picked once every 16 msec (it's rather arbitrary) and made the timeout be 128 msec. I also check to see if the idle counter is enabled. Since I figured the DMA idle counter enabled would be less often than every 16 msec, I put that test first. |
|
And as for @chuckbook's point, I believe that I took care of any interference by the dma_idle_handler and long DMA's. In the dma_enable_clock routine, I grab a copy of the dma_enable_mask and mark the stream as enabled as an atomic operation. The tick_handler won't turn off the DMA clock (for a given controller) if any stream is marked as enabled. |
I thought about using TIM3, and that's certainly a viable option. I was concerned though that down the road, not all systems would have the need for the TIM3 functionality, 8000 which is why I opted for the SysTick functionality instead. (i.e. if USB & flash storage were both disabled then TIM3 could be freed up). The work actually performed by the dma_idle_handler is quite minimal, which is why I thought using SysTick would be fine. I think that running the USB CDC stuff off SysTick would require lowering the systick priority to be below flash/sdio/dma, or implementing the I/O as async. |
|
I like the SYSTICK approach. I'm only concerned of stuffing too much into it. A simple time slicer in SysTickHandler with maybe 16 weak entries could do the job. Eventually the two DMA engines could be distributed as well. |
|
@chuckbook Just wanted to make that you get the latest version. I did some changes based on @dpgeorge 's feedback, so that it only calls the dma_idle_handler at most once every 16 msec, and only if there are dma idle countdowns enabled. |
|
|
||
| #define IRQ_PRI_OTG_FS 0x6 | ||
| #define IRQ_SUBPRI_OTG_FS 0x0 | ||
| #define IRQ_PRI_SDIO 4 |
There was a problem hiding this comment.
I think we need to add a comment here that SDIO IRQ must be higher priority than the DMA that it uses (it is, but just need a comment).
|
In closing pr#1565 @dpgeorge commented:
I didn't follow up because: 1) I was trying to keep 1565 a no-risk text-substitution-zero-functionality-change PR, and 2) I'm not sure what mechanisms are in place to validate the sub-priority change, even though it should be no-risk. Since @dhylands is digging a little deeper into IRQs with this change, I wonder if it might make sense to set the sub-pri's to zero with this PR, since presumably the same validation effort should cover both. |
|
I finally completed my tests regarding DMA / Systick handling. With @dhylands modifications we can now stream 32 24-bit channels at ~20kHz to SDCARD (fatfs). There is only one modification to @dhylands PR. was added and dma_tick_handler() was split up into two functions handling each of the DMAx subsystems. With 16ms cycle time 14 additional slots would be available for other tasks while keeping default exec times low. |
|
Ok, I'll merge this as-is and we can improve the dispatch as per @chuckbook later. |
|
@dhylands @chuckbook I implemented some optimisations in 522d454, 9936aa3 and 22bd231. |
This also adds code to turn off the DMA blocks when they're idle which saves approx 8mA per DMA block.
This PR superceds PR #1608