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

Hi,

> -----Original Message-----
> From: Stephen Hemminger <[email protected]>
> Sent: Thursday, August 6, 2026 10:42 PM
> To: Gagandeep Singh <[email protected]>
> Cc: [email protected]; Hemant Agrawal <[email protected]>
> Subject: 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.

Submitted the V2 series with the fixes. Also added a dependent patch in the series.