Drivers: DMA: Infineon PSE84 Cat1 PDL driver #98035
Conversation
zephyrbot
added
area: Tests
jsbatch
force-pushed
the
main-pse84_dma_pr
branch
from
18ff39e to
a1db355
Compare
zephyrbot
requested review from
mcatee-infineon,
nashif,
parthitce,
sreeramIfx,
talih0 and
teburd
Adding binding file for IFX Cat1 DMA PDL based driver implementation. Signed-off-by: John Batch <[email protected]>
Updates DMA includes from the modules needed for DMA PDL based driver implementation. Signed-off-by: John Batch <[email protected]>
jsbatch
force-pushed
the
main-pse84_dma_pr
branch
from
a1db355 to
77b6dbd
Compare
Adds Infineon Cat1 PDL based driver for DMA. Signed-off-by: John Batch <[email protected]>
Adds overlays to enable test runs for DMA on the following tests: -tests/drivers/dma/chan_blen_transfer -tests/drivers/dma/loop_transfer Signed-off-by: John Batch <[email protected]>
Adds support to the kit_pse84_eval board for DMA. Signed-off-by: John Batch <[email protected]>
jsbatch
force-pushed
the
main-pse84_dma_pr
branch
from
77b6dbd to
86a7fa3
Compare
|
There was a problem hiding this comment.
Given that PDL refers to Peripheral Driver Library, a software component, the hw device binding should have nothing to do with it.
Also, there already is a binding for this device, dts/bindings/dma/infineon,cat1-dma.yaml.
There was a problem hiding this comment.
See the discussion in #94264 (comment) for a way to switch to another driver, and deprecate the old.
There was a problem hiding this comment.
This looks like a duplicate of the existing dma_ifx_cat1.c driver to me. With maybe some added features. There shouldn't be two of these unless they are in fact two distinct IP blocks with different register maps and/or HAL interfaces.
The binding has a large number of options that don't appear to be used. These really should be added as they are required. By exposing these properties you are giving the appearance to someone creating a board port that these are configurable when they are not. That'll likely lead to some confusion.
| data->channels[channel].error_callback_dis = config->error_callback_dis; | ||
|
|
||
| /* Lock and page in the channel configuration */ | ||
| uint32_t key = irq_lock(); |
There was a problem hiding this comment.
While this is ok, its not great to block interrupts while translating the descriptors. I get the reasoning here is in part because you have a single pool of descriptors of each dma that is shared among all channels. sys_mem_blocks could be used instead at a small cost to maintain the shared pool per dma instance.
| } | ||
|
|
||
| /* initialize descriptor */ | ||
| dma_status = Cy_DMA_Descriptor_Init(descriptor, &descriptor_config); |
There was a problem hiding this comment.
It's unclear from the HAL usage here, but if registers are setup per channel then no locking at all should be needed. If some register is shared, a spin lock around the shared register updates is all that should be needed.
| * cache and will lead to errors) | ||
| */ | ||
| #ifdef CONFIG_CPU_HAS_DCACHE | ||
| SCB_InvalidateDCache(); |
There was a problem hiding this comment.
sys_cache_data_invd_all() though this is a bit of a shotgun approach
| * written back to the memory | ||
| */ | ||
| #ifdef CONFIG_CPU_HAS_DCACHE | ||
| SCB_CleanDCache(); |
|
I understand now a bit better that this is an alternative driver to the cat1 dma IP built on a newer/redesigned HAL, that's fine. The issues still need to be addressed and the compatible should not change. A Kconfig selection should choose the driver variant and some documentation/comments about all of this should be done here to avoid confusion. I did not know what PDL means at first glance. Please add some comments in the driver file and Kconfig description noting this and add a zephyr issue to deprecate the older driver. |
5 participants
Adding Infineon PDL based driver to support DMA on the PSOC Edge 84 microcontroller family. Adds support for the following tests: