Message-ID: <21cf712a-8418-4351-928a-c5246430cc16@ideasonboard.com>
Date: Wed, 12 Nov 2025 14:37:37 +0200
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: "Gupta, Suraj" <Suraj.Gupta2@....com>, Vinod Koul <vkoul@...nel.org>,
 "Simek, Michal" <michal.simek@....com>
Cc: "dmaengine@...r.kernel.org" <dmaengine@...r.kernel.org>,
 "linux-arm-kernel@...ts.infradead.org"
 <linux-arm-kernel@...ts.infradead.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "Sagar, Vishal" <vishal.sagar@....com>
Subject: Re: [PATCH] dmaengine: xilinx_dma: Enable IRQs for AXIDMA in
 start_transfer

Hi,

On 12/11/2025 14:22, Gupta, Suraj wrote:
> [Public]
> 
>> -----Original Message-----
>> From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
>> Sent: Tuesday, November 11, 2025 8:37 PM
>> To: Vinod Koul <vkoul@...nel.org>; Simek, Michal <michal.simek@....com>
>> Cc: dmaengine@...r.kernel.org; linux-arm-kernel@...ts.infradead.org; linux-
>> kernel@...r.kernel.org; Sagar, Vishal <vishal.sagar@....com>; Tomi
>> Valkeinen <tomi.valkeinen@...asonboard.com>
>> Subject: [PATCH] dmaengine: xilinx_dma: Enable IRQs for AXIDMA in
>> start_transfer
>>
>> Caution: This message originated from an External Source. Use proper
>> caution when opening attachments, clicking links, or responding.
>>
>>
>> A single AXIDMA controller can have one or two channels. When it has two
>> channels, the reset for both are tied together: resetting one channel
>> resets the other as well. This creates a problem where resetting one
>> channel will reset the registers for both channels, including clearing
>> interrupt enable bits for the other channel, which can then lead  to
>> timeouts as the driver is waiting for an interrupt which never comes.
>>
>> The driver currently has a probe-time work around for this: when a
>> channel is created, the driver also resets and enables the
>> interrupts. With two channels the reset for the second channel will
>> clear the interrupt enables for the first one. The work around in the
>> driver is just to manually enable the interrupts again in
>> xilinx_dma_alloc_chan_resources().
>>
>> This workaround only addresses the probe-time issue. When channels are
>> reset at runtime (e.g., in xilinx_dma_terminate_all() or during error
>> recovery), there's no corresponding mechanism to restore the other
>> channel's interrupt enables. This leads to one channel having its
>> interrupts disabled while the driver expects them to work, causing
>> timeouts and DMA failures.
>>
>> A proper fix is a complicated matter, as we should not reset the other
>> channel when it's operating normally. So, perhaps, there should be some
>> kind of synchronization for a common reset, which is not trivial to
>> implement. To add to the complexity, the driver also supports other DMA
>> types, like VDMA, CDMA and MCDMA, which don't have a shared reset.
>>
>> However, when the two-channel AXIDMA is used in the (assumably) normal
>> use case, providing DMA for a single memory-to-memory device, the
>> common
>> reset is a bit smaller issue: when something bad happens on one channel,
>> or when one channel is terminated, the assumption is that we also want
>> to terminate the other channel. And thus resetting both at the same time
>> is "ok".
>>
>> With that line of thinking we can implement a bit better work around
>> than just the current probe time work around: let's enable the
>> AXIDMA interrupts at xilinx_dma_start_transfer() instead.
>> This ensures interrupts are enabled whenever a transfer starts,
>> regardless of any prior resets that may have cleared them.
>>
>> This approach is also more logical: enable interrupts only when needed
>> for a transfer, rather than at resource allocation time, and, I think,
>> all the other DMA types should also use this model, but I'm reluctant to
>> do such changes as I cannot test them.
>>
>> The reset function still enables interrupts even though it's not needed
>> for AXIDMA anymore, but it's common code for all DMA types (VDMA, CDMA,
>> MCDMA), so leave it unchanged to avoid affecting other variants.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
>> ---
> 
> A warning has long been included in the PG stating that multichannel support for AXI DMA will be discontinued. Please refer 2022.1 AXIDMA PG:
>  https://www.amd.com/content/dam/xilinx/support/documents/ip_documentation/axi_dma/v7_1/pg021_axi_dma.pdf : Product Specification/Multichannel DMA support.
> Therefore, it's unnecessary to make changes for multiple channel support.

A warning in a document doesn't help the users who use AXI DMA with
multichannel, and have the kernel driver time-outing.

The multichannel AXI DMA is supported in the latest Vivado release. And
even if it wasn't, in my experience people often use older Vivado
releases. And even if they didn't, the IP has been out there for a long
time, so there are existing FPGA bitstreams with multichannel AXI DMA in
use.

I realized this patch should say it's a fix in the subject line, and
have a Fixes tag, so I'll send a v2.

 Tomi

> 
>>  drivers/dma/xilinx/xilinx_dma.c | 9 +--------
>>  1 file changed, 1 insertion(+), 8 deletions(-)
>>
>> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
>> index a34d8f0ceed8..50dd93ce6741 100644
>> --- a/drivers/dma/xilinx/xilinx_dma.c
>> +++ b/drivers/dma/xilinx/xilinx_dma.c
>> @@ -1216,14 +1216,6 @@ static int xilinx_dma_alloc_chan_resources(struct
>> dma_chan *dchan)
>>
>>         dma_cookie_init(dchan);
>>
>> -       if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
>> -               /* For AXI DMA resetting once channel will reset the
>> -                * other channel as well so enable the interrupts here.
>> -                */
>> -               dma_ctrl_set(chan, XILINX_DMA_REG_DMACR,
>> -                             XILINX_DMA_DMAXR_ALL_IRQ_MASK);
>> -       }
>> -
>>         if ((chan->xdev->dma_config->dmatype == XDMA_TYPE_CDMA) &&
>> chan->has_sg)
>>                 dma_ctrl_set(chan, XILINX_DMA_REG_DMACR,
>>                              XILINX_CDMA_CR_SGMODE);
>> @@ -1572,6 +1564,7 @@ static void xilinx_dma_start_transfer(struct
>> xilinx_dma_chan *chan)
>>                              head_desc->async_tx.phys);
>>         reg  &= ~XILINX_DMA_CR_DELAY_MAX;
>>         reg  |= chan->irq_delay << XILINX_DMA_CR_DELAY_SHIFT;
>> +       reg |= XILINX_DMA_DMAXR_ALL_IRQ_MASK;
>>         dma_ctrl_write(chan, XILINX_DMA_REG_DMACR, reg);
>>
>>         xilinx_dma_start(chan);
>>
>> ---
>> base-commit: e5f0a698b34ed76002dc5cff3804a61c80233a7a
>> change-id: 20251111-xilinx-dma-fix-bc26a6e9be5a
>>
>> Best regards,
>> --
>> Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
>>
> 

Powered by blists - more mailing lists