Re: [PATCH v3 01/13] dmaengine: dw-edma: Add per-channel interrupt routing control

[Koichiro Den]

On Mon, Jun 22, 2026 at 10:34:33AM -0500, Frank Li wrote:
> On Sun, Jun 21, 2026 at 02:00:28AM +0900, Koichiro Den wrote:
> > DesignWare eDMA can signal completion locally through edma_int[] and
> > remotely through IMWr/MSI. When channels are delegated to a remote
> > frontend, the local endpoint side and the remote host side must not both
> > service the same DONE/ABORT status.
> >
> > Add channel interrupt routing state and initialize it from the
> > controller instance configuration. Update the v0 eDMA and HDMA native
> > paths so linked-list interrupt generation, HDMA non-linked-list
> > interrupt enables, and DONE/ABORT masking follow the selected mode. For
> > HDMA native non-linked-list channels, use the dedicated remote
> > stop/abort enables without local stop/abort enables.
> >
> > Keep the existing dw-edma-pcie host-side instances in remote interrupt
> > routing mode so their IMWr/MSI completion model remains unchanged after
> > local routing becomes the zero value.
> >
> > Note that the routing mode describes where a channel should report
> > completion. It does not, by itself, say whether this dw-edma instance
> > owns the interrupt status. A local instance must ignore remote-only
> > channels, and a remote instance must ignore local-only channels, even if
> > such interrupts are unexpectedly delivered. Otherwise the non-owner side
> > could steal the interrupt from the owner by clearing shared DONE/ABORT
> > status.
> >
> > Suggested-by: Frank Li <Frank.Li@xxxxxxx>
> > Signed-off-by: Koichiro Den <den@xxxxxxxxxxxxx>
> > ---
> > Changes in v3:
> >   - Remove DW_EDMA_CH_IRQ_DEFAULT; local routing is the zero value
> >     (Frank).
> >   - Set existing dw-edma-pcie host-side instances to remote interrupt
> >     routing in this patch, preserving the legacy IMWr completion model.
> >   - Remove an unreachable HDMA native check (Sashiko).
> >   - Clarify local/remote instance ownership after Frank's question.
> >   - Mark non-owner IRQ handling guard paths unlikely.
> >   - Add HDMA native interrupt routing while keeping the existing non-LL
> >     int config ABI.
> >   - Keep HDMA native linked-list local interrupt generation enabled for
> >     remote-routed channels while masking the local edma_int[] output.
> >   - Use remote-only stop/abort enables for HDMA native non-LL remote-routed
> >     channels.
> >   - Drop the peripheral_config IRQ-routing ABI; initial routing comes from
> >     chip setup and channel ownership handoff can override it.
> >   - Keep dma_slave_config from resetting channel ownership routing.
> >
> >  drivers/dma/dw-edma/dw-edma-core.c    | 14 +++++++++
> >  drivers/dma/dw-edma/dw-edma-core.h    | 13 +++++++++
> >  drivers/dma/dw-edma/dw-edma-pcie.c    |  1 +
> >  drivers/dma/dw-edma/dw-edma-v0-core.c | 22 ++++++++++----
> >  drivers/dma/dw-edma/dw-hdma-v0-core.c | 41 ++++++++++++++++-----------
> >  include/linux/dma/edma.h              | 30 ++++++++++++++++++++
> >  6 files changed, 99 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> > index 89a4c498a17b..7a24248b84e9 100644
> > --- a/drivers/dma/dw-edma/dw-edma-core.c
> > +++ b/drivers/dma/dw-edma/dw-edma-core.c
> > @@ -219,6 +219,17 @@ static void dw_edma_device_caps(struct dma_chan *dchan,
> >  	}
> >  }
> >
> > +static enum dw_edma_ch_irq_mode dw_edma_get_irq_mode(struct dw_edma_chan *chan)
> > +{
> 
> dw_edma_get_default_irq_mode() ?

Yes, that naming is a better fit.

> 
> > +	struct dw_edma_chip *chip = chan->dw->chip;
> > +
> > +	if (chip->irq_mode == DW_EDMA_CH_IRQ_REMOTE &&
> > +	    !(chip->flags & DW_EDMA_CHIP_LOCAL))
> > +		return DW_EDMA_CH_IRQ_REMOTE;
> > +
> 
> return chip->flags & DW_EDMA_CHIP_LOCAL ? DW_EDMA_CH_IRQ_LOCAL : DW_EDMA_CH_IRQ_LOCAL

Assuming the second value meant DW_EDMA_CH_IRQ_REMOTE, yes.

The default can be derived from DW_EDMA_CHIP_LOCAL only (even if CHIP_PARTIAL is
introduced), so the chip-level irq_mode is unnecessary, as you pointed out
below for another hunk.

I will simplify this.

> 
> > +	return DW_EDMA_CH_IRQ_LOCAL;
> > +}
> > +
> >  static int dw_edma_device_config(struct dma_chan *dchan,
> >  				 struct dma_slave_config *config)
> >  {
> > @@ -853,6 +864,8 @@ static int dw_edma_alloc_chan_resources(struct dma_chan *dchan)
> >  	if (chan->status != EDMA_ST_IDLE)
> >  		return -EBUSY;
> >
> > +	chan->irq_mode = dw_edma_get_irq_mode(chan);
> > +
> >  	return 0;
> >  }
> >
> > @@ -904,6 +917,7 @@ static int dw_edma_channel_setup(struct dw_edma *dw, u32 wr_alloc, u32 rd_alloc)
> >  		chan->configured = false;
> >  		chan->request = EDMA_REQ_NONE;
> >  		chan->status = EDMA_ST_IDLE;
> > +		chan->irq_mode = dw_edma_get_irq_mode(chan);
> 
> if already set in dw_edma_alloc_chan_resources(), needn't set again?

You're right, but I would drop the hunk in dw_edma_alloc_chan_resources().
The initial routing can be set during channel setup.

> 
> >
> >  		if (chan->dir == EDMA_DIR_WRITE)
> >  			chan->ll_max = (chip->ll_region_wr[chan->id].sz / EDMA_LL_SZ);
> > diff --git a/drivers/dma/dw-edma/dw-edma-core.h b/drivers/dma/dw-edma/dw-edma-core.h
> > index 6474cacf7195..42f2f25ef377 100644
> > --- a/drivers/dma/dw-edma/dw-edma-core.h
> > +++ b/drivers/dma/dw-edma/dw-edma-core.h
> > @@ -81,6 +81,8 @@ struct dw_edma_chan {
> >
> >  	struct msi_msg			msi;
> >
> > +	enum dw_edma_ch_irq_mode	irq_mode;
> > +
> >  	enum dw_edma_request		request;
> >  	enum dw_edma_status		status;
> >  	u8				configured;
> > @@ -224,4 +226,15 @@ dw_edma_core_db_offset(struct dw_edma *dw)
> >  	return dw->core->db_offset(dw);
> >  }
> >
> > +static inline bool
> > +dw_edma_core_ch_ignore_irq(struct dw_edma_chan *chan)
> > +{
> > +	struct dw_edma *dw = chan->dw;
> > +
> > +	if (dw->chip->flags & DW_EDMA_CHIP_LOCAL)
> > +		return chan->irq_mode == DW_EDMA_CH_IRQ_REMOTE;
> 
> is it okay to simple return false here?

I do not think so. For a delegated channel, the EP-side dw-edma must not clear
DONE/ABORT status if the local handler is entered for another channel and
happens to observe status on the delegated channel.

For example, when nr_irqs == 1, dw_edma_interrupt_common() handles both write
and read directions from the same IRQ line. A local EP-owned WRITE completion
can enter the handler while an RC-owned READ channel also has DONE/ABORT status
pending. Without the ownership guard, the EP-side handler could clear the
RC-owned READ status.

HDMA native has a similar case within one direction: if one WRITE channel is
owned locally and another WRITE channel is delegated to the RC side, an
interrupt for the local channel can make the handler walk the same direction and
observe the delegated channel status.

So this should not simply return false. The guard is not for the expected
interrupt path; it is there to keep the non-owner side from consuming shared
sompletion status when the handler is entered for some other reason.

If I am missing the concern behind your question here, please let me know.

> 
> > +	else
> > +		return chan->irq_mode == DW_EDMA_CH_IRQ_LOCAL;
> > +}
> > +
> >  #endif /* _DW_EDMA_CORE_H */
> > diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw-edma/dw-edma-pcie.c
> > index 791c46e8ae4c..70ea031147d1 100644
> > --- a/drivers/dma/dw-edma/dw-edma-pcie.c
> > +++ b/drivers/dma/dw-edma/dw-edma-pcie.c
> > @@ -419,6 +419,7 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
> >  	chip->dev = dev;
> >
> >  	chip->mf = vsec_data->mf;
> > +	chip->irq_mode = DW_EDMA_CH_IRQ_REMOTE;
> >  	chip->nr_irqs = nr_irqs;
> >  	chip->ops = &dw_edma_pcie_plat_ops;
> >  	chip->cfg_non_ll = non_ll;
> > diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
> > index cfdd6463252e..1781ba4f022e 100644
> > --- a/drivers/dma/dw-edma/dw-edma-v0-core.c
> > +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c
> > @@ -256,9 +256,11 @@ dw_edma_v0_core_handle_int(struct dw_edma_irq *dw_irq, enum dw_edma_dir dir,
> >  	for_each_set_bit(pos, &val, total) {
> >  		chan = &dw->chan[pos + off];
> >
> > +		if (unlikely(dw_edma_core_ch_ignore_irq(chan)))
> > +			continue;
> > +
> >  		dw_edma_v0_core_clear_done_int(chan);
> >  		done(chan);
> > -
> 
> clean up these unncessary chagnes.

Will fix.

> 
> >  		ret = IRQ_HANDLED;
> >  	}
> >
> > @@ -267,9 +269,11 @@ dw_edma_v0_core_handle_int(struct dw_edma_irq *dw_irq, enum dw_edma_dir dir,
> >  	for_each_set_bit(pos, &val, total) {
> >  		chan = &dw->chan[pos + off];
> >
> > +		if (unlikely(dw_edma_core_ch_ignore_irq(chan)))
> > +			continue;
> > +
> >  		dw_edma_v0_core_clear_abort_int(chan);
> >  		abort(chan);
> > -
> >  		ret = IRQ_HANDLED;
> >  	}
> >
> > @@ -331,7 +335,8 @@ static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk)
> >  		j--;
> >  		if (!j) {
> >  			control |= DW_EDMA_V0_LIE;
> > -			if (!(chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL))
> > +			if (!(chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL) &&
> > +			    chan->irq_mode != DW_EDMA_CH_IRQ_LOCAL)
> 
> you define DW_EMDA_CH_IRQ_REMOTE, sugget don't use reverise logic.
> 
> 	chan->irq_mode == chan->irq_mode

Agreed. I will use that for more simple comparison.

> 
> >  				control |= DW_EDMA_V0_RIE;
> >  		}
> >
> > @@ -408,12 +413,17 @@ static void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
> >  				break;
> >  			}
> >  		}
> > -		/* Interrupt unmask - done, abort */
> > +		/* Interrupt mask/unmask - done, abort */
> >  		raw_spin_lock_irqsave(&dw->lock, flags);
> >
> >  		tmp = GET_RW_32(dw, chan->dir, int_mask);
> > -		tmp &= ~FIELD_PREP(EDMA_V0_DONE_INT_MASK, BIT(chan->id));
> > -		tmp &= ~FIELD_PREP(EDMA_V0_ABORT_INT_MASK, BIT(chan->id));
> > +		if (chan->irq_mode == DW_EDMA_CH_IRQ_REMOTE) {
> 
> I think need also check chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL

If the mask operation is limited to DW_EDMA_CHIP_LOCAL, the RC-side dw-edma-pcie
instance for delegated EP DMA would clear the local interrupt mask when it
programs the channel, as that instance is not marked DW_EDMA_CHIP_LOCAL.
That would re-enable the EP-local edma_int[] path for a channel whose completion
is owned by the RC-side.

Those redundant interrupts would not be fatal because
dw_edma_core_ch_ignore_irq() guards against the local side consuming the status,
but they should still be avoided in the normal path.

I guess your concern is the existing EDDA/MDB/CPM6 path, right? Those devices
historically did not mask the local interrupt when using remote completion.

> 
> > +			tmp |= FIELD_PREP(EDMA_V0_DONE_INT_MASK, BIT(chan->id));
> > +			tmp |= FIELD_PREP(EDMA_V0_ABORT_INT_MASK, BIT(chan->id));
> > +		} else {
> > +			tmp &= ~FIELD_PREP(EDMA_V0_DONE_INT_MASK, BIT(chan->id));
> > +			tmp &= ~FIELD_PREP(EDMA_V0_ABORT_INT_MASK, BIT(chan->id));
> > +		}
> >  		SET_RW_32(dw, chan->dir, int_mask, tmp);
> >  		/* Linked list error */
> >  		tmp = GET_RW_32(dw, chan->dir, linked_list_err_en);
> > diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> > index 2beec876b184..7ba6bdbffc17 100644
> > --- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
> > +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> > @@ -49,6 +49,26 @@ __dw_ch_regs(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch)
> >  		writel(value, &(__dw_ch_regs(dw, EDMA_DIR_READ, ch)->name));	\
> >  	} while (0)
> >
> > +static u32 dw_hdma_v0_core_int_setup(struct dw_edma_chan *chan, u32 val)
> > +{
> > +	val &= ~(HDMA_V0_LOCAL_ABORT_INT_EN | HDMA_V0_REMOTE_ABORT_INT_EN |
> > +		 HDMA_V0_LOCAL_STOP_INT_EN | HDMA_V0_REMOTE_STOP_INT_EN |
> > +		 HDMA_V0_ABORT_INT_MASK | HDMA_V0_STOP_INT_MASK);
> > +
> > +	if (chan->irq_mode == DW_EDMA_CH_IRQ_REMOTE && chan->non_ll)
> > +		return val | HDMA_V0_REMOTE_ABORT_INT_EN |
> > +		       HDMA_V0_REMOTE_STOP_INT_EN;
> > +
> > +	if (chan->irq_mode == DW_EDMA_CH_IRQ_REMOTE)
> > +		return val | HDMA_V0_LOCAL_ABORT_INT_EN |
> > +		       HDMA_V0_REMOTE_ABORT_INT_EN |
> > +		       HDMA_V0_LOCAL_STOP_INT_EN |
> > +		       HDMA_V0_REMOTE_STOP_INT_EN | HDMA_V0_ABORT_INT_MASK |
> > +		       HDMA_V0_STOP_INT_MASK;
> > +
> > +	return val | HDMA_V0_LOCAL_ABORT_INT_EN | HDMA_V0_LOCAL_STOP_INT_EN;
> > +}
> > +
> >  /* HDMA management callbacks */
> >  static void dw_hdma_v0_core_off(struct dw_edma *dw)
> >  {
> > @@ -132,6 +152,8 @@ dw_hdma_v0_core_handle_int(struct dw_edma_irq *dw_irq, enum dw_edma_dir dir,
> >
> >  	for_each_set_bit(pos, &mask, total) {
> >  		chan = &dw->chan[pos + off];
> > +		if (unlikely(dw_edma_core_ch_ignore_irq(chan)))
> > +			continue;
> >
> >  		val = dw_hdma_v0_core_status_int(chan);
> >  		if (FIELD_GET(HDMA_V0_STOP_INT_MASK, val)) {
> > @@ -238,11 +260,7 @@ static void dw_hdma_v0_core_ll_start(struct dw_edma_chunk *chunk, bool first)
> >  		SET_CH_32(dw, chan->dir, chan->id, ch_en, BIT(0));
> >  		/* Interrupt unmask - stop, abort */
> >  		tmp = GET_CH_32(dw, chan->dir, chan->id, int_setup);
> > -		tmp &= ~(HDMA_V0_STOP_INT_MASK | HDMA_V0_ABORT_INT_MASK);
> > -		/* Interrupt enable - stop, abort */
> > -		tmp |= HDMA_V0_LOCAL_STOP_INT_EN | HDMA_V0_LOCAL_ABORT_INT_EN;
> > -		if (!(dw->chip->flags & DW_EDMA_CHIP_LOCAL))
> > -			tmp |= HDMA_V0_REMOTE_STOP_INT_EN | HDMA_V0_REMOTE_ABORT_INT_EN;
> > +		tmp = dw_hdma_v0_core_int_setup(chan, tmp);
> 
> Can you use small patch to create helper dw_hdma_v0_core_int_setup() only

Sure, I will split it out.

> 
> >  		SET_CH_32(dw, chan->dir, chan->id, int_setup, tmp);
> >  		/* Channel control */
> >  		SET_CH_32(dw, chan->dir, chan->id, control1, HDMA_V0_LINKLIST_EN);
> > @@ -293,17 +311,8 @@ static void dw_hdma_v0_core_non_ll_start(struct dw_edma_chunk *chunk)
> >  	SET_CH_32(dw, chan->dir, chan->id, transfer_size, child->sz);
> >
> >  	/* Interrupt setup */
> > -	val = GET_CH_32(dw, chan->dir, chan->id, int_setup) |
> > -			HDMA_V0_STOP_INT_MASK |
> > -			HDMA_V0_ABORT_INT_MASK |
> > -			HDMA_V0_LOCAL_STOP_INT_EN |
> > -			HDMA_V0_LOCAL_ABORT_INT_EN;
> > -
> > -	if (!(dw->chip->flags & DW_EDMA_CHIP_LOCAL)) {
> > -		val |= HDMA_V0_REMOTE_STOP_INT_EN |
> > -		       HDMA_V0_REMOTE_ABORT_INT_EN;
> > -	}
> > -
> > +	val = GET_CH_32(dw, chan->dir, chan->id, int_setup);
> > +	val = dw_hdma_v0_core_int_setup(chan, val);
> >  	SET_CH_32(dw, chan->dir, chan->id, int_setup, val);
> >
> >  	/* Channel control setup */
> > diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h
> > index 1fafd5b0e315..c0906221a7c7 100644
> > --- a/include/linux/dma/edma.h
> > +++ b/include/linux/dma/edma.h
> > @@ -60,6 +60,34 @@ enum dw_edma_chip_flags {
> >  	DW_EDMA_CHIP_LOCAL	= BIT(0),
> >  };
> >
> > +/**
> > + * enum dw_edma_ch_irq_mode - per-channel interrupt routing control
> > + * @DW_EDMA_CH_IRQ_LOCAL:     local interrupt only (edma_int[])
> > + * @DW_EDMA_CH_IRQ_REMOTE:    remote interrupt only (IMWr/MSI), without
> > + *                            delivering local edma_int[].
> > + *
> > + * DesignWare EP eDMA can signal interrupts locally through the edma_int[]
> > + * bus, and remotely using posted memory writes (IMWr) that may be
> > + * interpreted as MSI/MSI-X by the RC.
> > + *
> > + * For the v0 eDMA linked-list programming path, DMA_*_INT_MASK gates the local
> > + * edma_int[] assertion, while there is no dedicated per-channel mask for IMWr
> > + * generation. To request a remote-only interrupt, Synopsys recommends setting
> > + * both LIE and RIE, and masking the local interrupt in DMA_*_INT_MASK. See the
> > + * DesignWare endpoint databook 6.30a, Linked List Mode interrupt handling
> > + * ("Software Programming of an Endpoint's LIE and RIE Bits for Linked List
> > + * Transfers", Attention).
> > + *
> > + * HDMA linked-list watermark interrupts have the same LWIE/RWIE guidance. HDMA
> > + * non-linked-list mode has dedicated local and remote stop/abort interrupt
> > + * enables, and the remote CPU programming examples use remote enables without
> > + * local enables.
> > + */
> > +enum dw_edma_ch_irq_mode {
> > +	DW_EDMA_CH_IRQ_LOCAL	= 0,
> > +	DW_EDMA_CH_IRQ_REMOTE,
> > +};
> > +
> >  /**
> >   * struct dw_edma_chip - representation of DesignWare eDMA controller hardware
> >   * @dev:		 struct device of the eDMA controller
> > @@ -76,6 +104,7 @@ enum dw_edma_chip_flags {
> >   * @db_irq:		 Virtual IRQ dedicated to interrupt emulation
> >   * @db_offset:		 Offset from DMA register base
> >   * @mf:			 DMA register map format
> > + * @irq_mode:		 default per-channel interrupt routing
> >   * @dw:			 struct dw_edma that is filled by dw_edma_probe()
> >   */
> >  struct dw_edma_chip {
> > @@ -101,6 +130,7 @@ struct dw_edma_chip {
> >  	resource_size_t		db_offset;
> >
> >  	enum dw_edma_map_format	mf;
> > +	enum dw_edma_ch_irq_mode	irq_mode;
> 
> This are already have flags dw_edma_chip_flags, not sure why need irq_mode,
> suppose, if set DW_EDMA_CHIP_LOCAL, pre channel should be default as
> DW_EDMA_CH_IRQ_LOCAL

Yes, the chip-level irq_mode is unnecessary.

Thanks a lot for the detailed review,
Koichiro

> 
> Frank
> >
> >  	struct dw_edma		*dw;
> >  	bool			cfg_non_ll;
> > --
> > 2.51.0
> >