Re: [PATCH v2 0/5] dma/imx_edma5: introduce NXP i.MX95 eDMA5 driver

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri,  7 Aug 2026 12:18:48 +0530
Gagandeep Singh <g.singh@nxp.com> wrote:

> V2-changes:
>  - Added dependent patch: bus/platform: match device by devicetree compatible string
>  - Fixed multiple AI reported issues includes:
>    - removed the alias from the driver, no need of it
>    - removed imx_edma5_write32 from data-path, reported as coherency
>      concern.
>    - fix submit for previously enqueued jobs.
>    - Added dsb in data path
>    - Added a commnet for rte_mem_iova2virt() NULL behaviour.
>    - Fixed: TCD NBYTES bits 31:30 are SMLOE/DMLOE — full 32-bit length corrupts them
>    - Data path is fully synchronous: Acknowledged but intentionally deferred —
>      the synchronous design is a deliberate simplification for the initial upstream submission.
>      Noted in docs.
>    - In-memory TCD64 pool is more complex than needed:
>      Noted/deferred — the pool structure was retained for forward compatibility
>      with hardware SG chaining.
>    - Extended `imx_edma5_dump()` to read and print `CH_CSR` and `CH_ES` registers
>      for every configured vchan.
>    - Double and stary lines removed.
> 
> V1:
> This patch series adds a new dmadev Poll-Mode Driver (PMD) for the NXP
> i.MX95 eDMA5 (Enhanced DMA Type 5) controller.
> 
> Key features supported by this driver:
>   - Memory-to-memory copy (RTE_DMA_OP_TYPE_MEMCPY)
>   - Scatter-gather memory copy (RTE_DMA_OP_TYPE_SG)
>   - 64-bit TCD (Transfer Control Descriptor) format
>   - Non-coherent DMA with explicit cache clean/invalidate
>   - Per-channel statistics and register dump for debug
> 
> Patch breakdown:
>   [1/4] Skeleton: bus probe/remove, dmadev registration, MAINTAINERS,
>         doc index, and release notes for 26.11.
>   [2/4] Device configuration: vchan setup, TCD ring allocation,
>         start/stop, and capability reporting.
>   [3/4] Data path: enqueue (copy and sg), doorbell, completion poll.
>   [4/4] Statistics and dump: per-channel counters and register dump.
> 
> Tested on NXP i.MX95 EVK with vfio-platform.
> 
> Gagandeep Singh (5):
>   bus/platform: match device by devicetree compatible string
>   dma/imx_edma5: introduce eDMA5 dmadev skeleton
>   dma/imx_edma5: add device configuration
>   dma/imx_edma5: add data path
>   dma/imx_edma5: add statistics and dump
> 
>  MAINTAINERS                                |    5 +
>  doc/guides/dmadevs/imx_edma5.rst           |   61 +
>  doc/guides/dmadevs/index.rst               |    1 +
>  doc/guides/rel_notes/release_26_11.rst     |    6 +
>  drivers/bus/platform/bus_platform_driver.h |    3 +-
>  drivers/bus/platform/platform.c            |   67 +-
>  drivers/dma/imx_edma5/imx_edma5_dmadev.c   | 1162 ++++++++++++++++++++
>  drivers/dma/imx_edma5/imx_edma5_dmadev.h   |  220 ++++
>  drivers/dma/imx_edma5/imx_edma5_hw.h       |  157 +++
>  drivers/dma/imx_edma5/imx_edma5_logs.h     |   16 +
>  drivers/dma/imx_edma5/meson.build          |   10 +
>  drivers/dma/meson.build                    |    1 +
>  12 files changed, 1707 insertions(+), 2 deletions(-)
>  create mode 100644 doc/guides/dmadevs/imx_edma5.rst
>  create mode 100644 drivers/dma/imx_edma5/imx_edma5_dmadev.c
>  create mode 100644 drivers/dma/imx_edma5/imx_edma5_dmadev.h
>  create mode 100644 drivers/dma/imx_edma5/imx_edma5_hw.h
>  create mode 100644 drivers/dma/imx_edma5/imx_edma5_logs.h
>  create mode 100644 drivers/dma/imx_edma5/meson.build
> 

More AI feedback on V2

Review of [PATCH v2 0/5] dma/imx_edma5: introduce NXP i.MX95 eDMA5 driver

Verification: each of the five commits builds independently with
-Dwerror=true on x86_64 and aarch64 cross (gcc), full build including
documentation is clean, check-git-log reports 5/5 valid. Confirmed by
disassembly that the new DSB sequences are emitted on aarch64.

Thanks for the v2. The bus matching, the SUBMIT-flag semantics, the missing
DSB, the coherency contradiction and the style nits are all resolved. The
NBYTES cap is applied to copy() but not to copy_sg(), and a few smaller
items remain.

Patch 1/5 (bus/platform: match by devicetree compatible):

Warning: The quote-stripping in of_device_is_compatible() is unnecessary
and should be dropped. Now that imx_edma5 sets .driver.alias directly in
the driver struct, no in-tree caller passes a quoted string, so the branch
is dead code. It exists to paper over RTE_PMD_REGISTER_ALIAS, which
stringifies its argument via RTE_STR and therefore cannot express a
compatible string containing a comma. Silently accepting a mangled alias
in the bus hides that defect from the next driver that hits it. Either
leave the macro alone and drop the quote handling, or fix the macro (e.g.
a variant that takes the string unmodified) and drop it as well.

Info: The last match block ends with "goto out;" immediately above the
"out:" label; the goto is redundant.

Info: The commit changes matching behaviour for every platform-bus driver
and has no release notes entry. There is currently one in-tree consumer,
so this is a judgement call, but a line under New Features would be
reasonable.

Patch 2/5 (skeleton):

Warning: The driver documentation does not match the code. Both the
Supported Features and Limitations sections describe scatter-gather as
requiring equal-length or equal-sized source and destination segment
pairs, but imx_edma5_copy_sg() walks the two lists as independent cursors
and correctly handles unequal segmentation, requiring only equal totals.
The doc understates the driver. Please update both sections.

Info: IMX_EDMA5_CH_MATTR_COHERENT (and the RCACHE/WCACHE/RDOMAINS/WDOMAINS
macros it is built from) is now unused after the CH_MATTR write was
dropped in patch 3/5. Remove the definitions or note why they are kept.

Patch 4/5 (data path):

Error: The IMX_EDMA5_MAX_NBYTES cap is enforced in imx_edma5_copy() but
not in imx_edma5_copy_sg(). Sub-transfer lengths there come from
RTE_MIN(s_rem, d_rem) over rte_dma_sge.length, which is uint32_t, so a
single segment larger than 1 GiB - 1 reaches imx_edma5_program_copy() and
writes a count with bit 30 or 31 set into TCD_NBYTES - exactly the
SMLOE/DMLOE corruption the copy() check was added to prevent. Validate
each segment length (or each computed sub-transfer length) against
IMX_EDMA5_MAX_NBYTES and return -EINVAL.

Warning: An enqueue call can now block for a very long time. Each
sub-transfer waits up to IMX_EDMA5_WAIT_TIMEOUT_MS (1000 ms), and
imx_edma5_run_job() serialises SG sub-transfers, so a job with the maximum
32 sub-transfers can spin for up to 32 seconds inside rte_dma_copy_sg()
with RTE_DMA_OP_FLAG_SUBMIT, or inside rte_dma_submit(). Consider a
per-job deadline rather than a per-sub-transfer one.

Warning: imx_edma5_wait_done() still cannot stop a transfer it gave up on.
On timeout it calls imx_edma5_reset_hw_chan(), which writes CH_CSR.DONE,
CH_ES and the TCD control fields but does not cancel an in-flight transfer
(there is no MP_CSR.CX use in the driver), so a channel that is genuinely
stuck rather than merely slow will still be reprogrammed while ACTIVE and
the abandoned transfer keeps writing to the old destination. The length
cap makes this much harder to hit, so this is no longer an error, but a
cancel on the timeout path would close it properly.

Warning: The data path remains fully synchronous - enqueue or submit
programs the TCD, starts the channel and busy-waits for DONE - so the CPU
spins for the duration of every copy and the offload gains nothing over
memcpy. The hardware can run detached: program and START at submit time,
poll CH_CSR.DONE in completed()/completed_status(), and serialise only
when a second job needs the single register TCD. If this is deliberate for
the first submission, please state it in the Limitations section of the
driver doc rather than leaving it implicit.

Warning: When rte_mem_iova2virt() returns NULL, cache maintenance is
skipped and, on this non-coherent master, the transfer silently returns
wrong data. The v2 comment documents this, but a code comment is not
reachable by the application author. Either reject such addresses with
-EINVAL or state the restriction in the driver documentation.

Warning: rte_mem_iova2virt() is called per operation (source and
destination, and per segment in copy_sg) in the fast path; it walks the
memseg lists. In IOVA=VA mode the lookup is unnecessary and the value is
the address itself.