imx6ull-sdma: make BD_DONE handing optional#660
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the i.MX6ULL SDMA driver by introducing an 'auto BD done' configuration option, refactoring the interrupt handler for better readability, and improving the main loop's locking mechanism. It also updates the data write API for const-correctness and adds a FAQ to the documentation. Key review feedback identified a bug where the new option constant was defined as zero, making it impossible to enable. Other suggestions include adding alignment checks for memory transfers and addressing the safety of const-casting in the library implementation.
| dev_ctl.mem.len = size; | ||
|
|
||
| return sdma_dev_ctl(s, &dev_ctl, data, size); | ||
| return sdma_dev_ctl(s, &dev_ctl, (void *)data, size); |
There was a problem hiding this comment.
Casting away const from the data pointer when calling sdma_dev_ctl is potentially unsafe. Although sdma_data_mem_write is a write operation (where the buffer is only read), the sdma_dev_ctl function takes a non-const void * and could potentially modify the buffer if the implementation changes or if the function is misused in other contexts. Ideally, the underlying sdma_dev_ctl API should be updated to be const-correct.
There was a problem hiding this comment.
This is required as msg_t uses non const pointer.
There was a problem hiding this comment.
But this data is o->data (pointer to output data). This is not how it supposed to work!
There was a problem hiding this comment.
Added comment for now. Doing this properly requires more changes (ie splitting devctl type) and needs to be tested.
There was a problem hiding this comment.
And now this comment is actually pushed to this issue
Unit Test Results10 860 tests +275 10 190 ✅ +275 53m 7s ⏱️ +4s Results for commit 8f35865. ± Comparison against base commit a9f6ccd. This pull request removes 3 and adds 278 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
If I would nitpicking you greatly I would say that
imx6ull-sdma: remove noop write to reserved register
is indeed probably noop but register is described in docs and 1 value on each bits is quite-well defined.
It would be great if you could provide some context – why you introduce this feature, how it's supposed to use.
I think you should check your changes if not introduce any issues in DTR project.
| dev_ctl.mem.len = size; | ||
|
|
||
| return sdma_dev_ctl(s, &dev_ctl, data, size); | ||
| return sdma_dev_ctl(s, &dev_ctl, (void *)data, size); |
There was a problem hiding this comment.
But this data is o->data (pointer to output data). This is not how it supposed to work!
xvuko
force-pushed
the
xvuko/msh-36-sdma
branch
2 times, most recently
from
fd06027 to
63d2ca3
Compare
Added more context to commit message: |
Added more context to commit message: |
TASK: MSH-36
TASK: MSH-36
SDMA commands GET_DM and SET_DM take size in 32 bit words. TASK: MSH-36
Lock is always released inside condWait TASK: MSH-36
TASK: MSH-36
There is no need to `DSPOVR`o register. IMX6 does not have DSP core. It is set to 0xffffffff after reset and there is no need for any explicit writes from driver. Line `DSPOVR |= 1 << channel_id` looks like it might do something important. It does not. It makes `sdma_init_core` harder to understand without any benefit. TASK: MSH-36
BD_DONE flags indicate that SDMA "owns" buffer described in BD and can write into it. When SDMA returns ownership to processor/driver buffer content should be read *before* setting BD_DONE flag. This ensures data integrity (no read / write collisions). This description assumes RX transfer, but same applies to TX with read / writes swapped. Automatic BD_DONE set can still be useful in some cases like sending fixed buffer repeatedaly (like read requests from ADC). Backwards incompatible. Use `channelCfg.options = sdma_chOption__auto_bd_done` to keep old behaviour. TASK: MSH-36
2 participants
Description
Motivation and Context
Types of changes
How Has This Been Tested?
Checklist:
Special treatment