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

On Thu,  6 Aug 2026 14:12:41 +0530
Gagandeep Singh <[email protected]> wrote:

> 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 (4):
>   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/dma/imx_edma5/imx_edma5_dmadev.c | 1096 ++++++++++++++++++++++
>  drivers/dma/imx_edma5/imx_edma5_dmadev.h |  203 ++++
>  drivers/dma/imx_edma5/imx_edma5_hw.h     |  158 ++++
>  drivers/dma/imx_edma5/imx_edma5_logs.h   |   16 +
>  drivers/dma/imx_edma5/meson.build        |   10 +
>  drivers/dma/meson.build                  |    1 +
>  10 files changed, 1557 insertions(+)
>  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
> 

Always good to see more hardware support.

Detailed AI review showed lots of problems:

Patch 1/4 (skeleton):

Error: The driver can never match a device through the upstream platform
bus. platform_bus_match() compares (1) the kernel driver name against the
DPDK driver name, (2) the kernel driver name against the alias, and (3) the
sysfs device name against the DPDK driver name. Devices are only scanned if
bound to vfio-platform, so the kernel driver name is always
"vfio-platform", and a DT device name like "42000000.dma-controller" never
equals "dma_imx_edma5". No match path can succeed, so probe is unreachable.
The cover letter says this was tested on i.MX95 EVK - presumably against a
modified bus. The series needs an accompanying bus/platform change (e.g.
match on of_node/compatible) or an explanation of the intended binding.

Error: RTE_PMD_REGISTER_ALIAS(dma_imx_edma5, IMX_EDMA5_COMPAT) produces a
broken alias. RTE_PMD_REGISTER_ALIAS stringifies its argument via RTE_STR,
and IMX_EDMA5_COMPAT is already a string literal, so the alias becomes
"\"fsl,imx95-edma5\"" with embedded quote characters (verified with cpp).
It can never strcmp-equal anything. The macro takes an unquoted token, and
"fsl,imx95-edma5" cannot be expressed as one (comma splits macro args), so
the compatible string cannot be used as an alias at all - which feeds back
into the matching problem above.

Warning: probe() does not verify the device's compatible string. If the bus
matching is fixed, any allowlisted vfio-platform device would be claimed by
this driver. Read of_node/compatible and check for "fsl,imx95-edma5"
before touching registers.

Patch 2/4 (device configuration):

Warning: Internal contradiction on coherency. imx_edma5_reset_hw_chan()
programs CH_MATTR to IMX_EDMA5_CH_MATTR_COHERENT with a comment saying the
eDMA "snoops the CPU caches, matching the Linux fsl-edma driver on a
dma-coherent controller", while the cover letter, patch 3 commit message,
and the data path all say the master is non-coherent and do full manual
clean/invalidate. Both cannot be the intended design: if the interconnect
honors those attributes, the per-op cache maintenance and iova2virt lookups
are pure overhead; if it does not, this comment is wrong. Pick one and make
the comments match the code.

Patch 3/4 (data path):

Error: RTE_DMA_OP_FLAG_SUBMIT does not submit previously enqueued jobs.
The API defines the flag as issuing the doorbell "after enqueued jobs",
i.e. equivalent to rte_dma_submit(). imx_edma5_copy()/copy_sg() with the
flag run only the current job. Jobs enqueued earlier without the flag are
skipped, so they execute out of order relative to the flagged job, and if
the application relies on the flag alone they never execute:
imx_edma5_completed() stops at the unsubmitted job at tail and the
application waits forever. In the flag branch, run all pending jobs from
tail through this one (same walk as imx_edma5_submit()).

Error: The arm64 cache maintenance helpers lack a DSB. imx_edma5_cache_clean()
issues DC CVAC per line but never executes DSB; completion of cache
maintenance to the PoC is only guaranteed after a DSB, and the DMB inside
rte_write32() does not provide that. The device can be started before the
cleans reach memory and read stale source data; likewise the CPU can read
the destination before the CIVAC loop in imx_edma5_cache_inval() has
completed. Add asm volatile("dsb sy" ::: "memory") at the end of both
helpers (compare Linux arch_sync_dma_for_device()).

Error: 1 ms completion timeout with unbounded transfer length can corrupt
memory. The API length is uint32_t and the driver imposes no maximum, but
the timeout comment assumes "the largest single-block copy ... completes in
well under a millisecond". A large copy exceeds 1 ms, imx_edma5_wait_done()
times out, and imx_edma5_reset_hw_chan() does not actually cancel an active
transfer (it only writes CH_CSR.DONE; there is no MP_CSR.CX cancel in this
driver), so the next job reprograms the TCD while the channel is still
ACTIVE and the aborted transfer keeps writing to the old destination.
Enforce a maximum length in copy()/copy_sg() and scale the timeout, or
implement cancel via MP_CSR.CX on timeout.

Error: TCD NBYTES is written with the full 32-bit length, but on
eDMA3/4/5 the NBYTES register carries SMLOE/DMLOE in bits 31:30 when
minor-loop offsets are supported, leaving a 30-bit count. A length with
bit 30 or 31 set silently enables minor-loop offset mode and truncates the
count. (Confidence moderate - please confirm against the i.MX95 RM.) A
length cap per the previous item resolves this too.

Warning: The data path is 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 only serialize when a
second job needs the single register TCD. At minimum the limitation
deserves a line in the driver doc.

Warning: rte_mem_iova2virt() is called per operation (src and dst) in the
hot path; it walks the memseg lists and is expensive. In IOVA=VA mode the
lookup is unnecessary. Also, when it returns NULL (e.g. external memory),
cache maintenance is silently skipped, which corrupts data on the
non-coherent path - reject such addresses or document the restriction.

Info: The in-memory TCD64 pool is only used as a parameter store: fields
are converted to LE at enqueue and read back with le_to_cpu in
imx_edma5_run_job()/job_invalidate_dst(); no descriptor is ever fetched by
hardware, and the "must be cleaned from the CPU cache" comment in
imx_edma5_hw.h is not (and need not be) honored. A plain array of
src/dst/len triples would be simpler, or use E_SG hardware chaining.

Patch 4/4 (statistics and dump):

Info: The cover letter advertises "register dump for debug", but dev_dump
prints software state only (the commit message is accurate). Either dump
CH_CSR/CH_ES/TCD registers or fix the cover letter wording.

Series:

Info: A few double blank lines (imx_edma5_dmadev.h:46, 69, 132;
imx_edma5_hw.h:157) and a stray blank line before the closing brace of
struct imx_edma5_dev; checkpatch will flag these.