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 
jsbatch force-pushed the main-pse84_dma_pr branch from a1db355 to 77b6dbd Compare 
jsbatch force-pushed the main-pse84_dma_pr branch from 77b6dbd to 86a7fa3 Compare 
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.
drivers/dma/dma_ifx_cat1_pdl.c Outdated
| 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.
There was a problem hiding this comment.
I've reworked things done within the irq_lock in an effort to minimize the amount of time.
| } | ||
| | ||
| /* 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.
There was a problem hiding this comment.
As above, I've reworked the irq lock to avoid this.
drivers/dma/dma_ifx_cat1_pdl.c Outdated
| * 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
There was a problem hiding this comment.
I agree invalidating all of cache overkill, and we plan on addressing this in the future. I've updated the function calls to use the sys_cache calls instead of calling into SCB directly.
drivers/dma/dma_ifx_cat1_pdl.c Outdated
| * written back to the memory | ||
| */ | ||
| #ifdef CONFIG_CPU_HAS_DCACHE | ||
| SCB_CleanDCache(); |
There was a problem hiding this comment.
Updated to use sys_cache.
| 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. |
jsbatch force-pushed the main-pse84_dma_pr branch 2 times, most recently from a22a9af to 4d796ac Compare
I've reworked this to use a Kconfig flag to differentiate between which devices use the older dma_ifx_cat1.c driver and the new dma_ifx_cat1_pdl.c driver. The older driver uses an older HAL which is Infineon is phasing out. The plan is for all devices to eventually migrate to the new PDL based driver, at which point the Kconfig flag will be removed. |
jsbatch force-pushed the main-pse84_dma_pr branch from 4d796ac to f479f86 Compare Adding binding file for IFX Cat1 DMA PDL based driver implementation. Signed-off-by: John Batch <john.batch@infineon.com>
Updates DMA includes from the modules needed for DMA PDL based driver implementation. Signed-off-by: John Batch <john.batch@infineon.com>
Adds USE_INFINEON_LEGACY_HAL kconfig option to the PSOC6, CYW920829, and xmc7200 series SOCs. These devices have not transitioned to newer HAL based drivers yet. Signed-off-by: John Batch <john.batch@infineon.com>
jsbatch force-pushed the main-pse84_dma_pr branch from f479f86 to b6463de Compare Adds Infineon Cat1 PDL based driver for DMA. Signed-off-by: John Batch <john.batch@infineon.com>
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 <john.batch@infineon.com>
Adds support to the kit_pse84_eval board for DMA. Signed-off-by: John Batch <john.batch@infineon.com>
jsbatch force-pushed the main-pse84_dma_pr branch from b6463de to 2824447 Compare 
|
I've added this issue to capture moving Infineon drivers from HAL to PDL. #98600 |
| @jsbatch @billwatersiii out-of-scope for this PR, but the approach we followed here to introduce a PDL-based driver for the same hw device should be applied to all other infineon drivers, for ex. here zephyr/drivers/serial/Kconfig.ifx_cat1 Line 23 in 0f5e03f |
Yes, I’m already working on updating other drivers to follow the same approach. I wanted to get the approach finalized in this PR before getting too far into other driver changes. |
5 participants
Adding Infineon PDL based driver to support DMA on the PSOC Edge 84 microcontroller family. Adds support for the following tests: