Re: [PATCH v3 10/24] dmaengine: dw-edma: Prepare LL kicks for event serialization

[Koichiro Den]

On Mon, Jul 27, 2026 at 04:18:01PM -0400, Frank Li wrote:
> On Tue, Jul 28, 2026 at 02:03:09AM +0900, Koichiro Den wrote:
> > Both eDMA and HDMA perform the same remote LL read-back immediately
> > before writing the doorbell register.
> >
> > A later patch serializes each LL doorbell write against IRQ event
> > capture with a raw spinlock. Keeping the read-back in the provider
> > callback would hold that lock across a remote read. Move the common
> > read-back into a core kick helper, where it can run before the
> > serialized section, and leave dw_edma_core_ch_maybe_doorbell() to decide
> > whether a kick is needed.
> >
> > No functional change.
> >
> > Signed-off-by: Koichiro Den <den@xxxxxxxxxxxxx>
> > ---
> > Changes in v3:
> >   - New prep patch split from "Centralize LL doorbell decisions" to keep
> >     that policy change focused and the remote LL read-back outside the
> >     event lock added later. (Sashiko)
> >
> >  drivers/dma/dw-edma/dw-edma-core.c    | 20 +++++++++++++++++++-
> >  drivers/dma/dw-edma/dw-edma-v0-core.c | 16 ----------------
> >  drivers/dma/dw-edma/dw-hdma-v0-core.c | 16 ----------------
> >  3 files changed, 19 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> > index bb57f2d39d1c..e3d0559206d8 100644
> > --- a/drivers/dma/dw-edma/dw-edma-core.c
> > +++ b/drivers/dma/dw-edma/dw-edma-core.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/dmaengine.h>
> >  #include <linux/err.h>
> >  #include <linux/interrupt.h>
> > +#include <linux/io.h>
> >  #include <linux/irq.h>
> >  #include <linux/dma/edma.h>
> >  #include <linux/dma-mapping.h>
> > @@ -254,6 +255,23 @@ static void dw_edma_finish_termination(struct dw_edma_chan *chan)
> >  	chan->status = EDMA_ST_IDLE;
> >  }
> >
> > +static void dw_edma_core_ll_sync(struct dw_edma_chan *chan)
> > +{
> > +	/*
> > +	 * Remote controller registers and LL memory may be reached through
> > +	 * different paths. Complete posted LL writes before the doorbell.
> > +	 */
> > +	if (!(chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL))
> > +		readl(chan->ll_region.vaddr.io);
> > +}
> > +
> > +/* Must be called with vc.lock held for an LL channel. */
> > +static void dw_edma_core_ch_kick(struct dw_edma_chan *chan)
> > +{
> > +	dw_edma_core_ll_sync(chan);
> > +	dw_edma_core_ch_doorbell(chan);
> > +}
> > +
> 
> Can you move
> 
> 	if (!(chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL))
> 		readl(chan->ll_region.vaddr.io);
> 
> into funciton dw_edma_core_ch_doorbell().

Could you elaborate a bit more on the concern?

I'm guessing that one or both of the following changes look unnecessary to you:

  (a). add the common dw_edma_core_ll_sync() helper
  (b). move it outside of dw_edma_core_ch_doorbell()

Regarding (a), the helper is also used in patch 22, where the LL writes must be
completed before re-enabling the direction, without issuing a doorbell at that
point. So I think (a) is useful on its own.

Regarding (b), this patch 11 puts the raw doorbell write under event_lock.
Simply keeping the sync in dw_edma_core_ch_doorbell would therefor also put the
remote non-posted MRd under that lock. I tried to avoid it for two reasons:

  1. It would otherwise increase event_lock contention, especially for eDMA,
     where the lock granularity is a direction rather than a channel. The IRQ
     path also runs dw_edma_record_irq() while holding the same lock.

  2. If the non-posted MRd completion is delayed, event_lock would remain held
     for the full delay, which can easily be avoided by this patch.

Or is your concern more of the API design, that every doorbell call via
dw_edma_core_ch_doorbell() should include the LL publication barrier?

Thanks for reviewing,
Koichiro

> 
> Frank
> >  /* Must be called with vc.lock held. */
> >  static void dw_edma_core_ch_maybe_doorbell(struct dw_edma_chan *chan)
> >  {
> > @@ -261,7 +279,7 @@ static void dw_edma_core_ch_maybe_doorbell(struct dw_edma_chan *chan)
> >  	    chan->status != EDMA_ST_BUSY || !dw_edma_ll_pending(chan))
> >  		return;
> >
> > -	dw_edma_core_ch_doorbell(chan);
> > +	dw_edma_core_ch_kick(chan);
> >  }
> >
> >  static void dw_edma_device_caps(struct dma_chan *dchan,
> > diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
> > index d883ca446f5a..b3098eb83442 100644
> > --- a/drivers/dma/dw-edma/dw-edma-v0-core.c
> > +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c
> > @@ -480,20 +480,6 @@ static void dw_edma_v0_core_ch_enable(struct dw_edma_chan *chan)
> >  		  upper_32_bits(chan->ll_region.paddr));
> >  }
> >
> > -static void dw_edma_v0_sync_ll_data(struct dw_edma_chan *chan)
> > -{
> > -	/*
> > -	 * In case of remote eDMA engine setup, the DW PCIe RP/EP internal
> > -	 * configuration registers and application memory are normally accessed
> > -	 * over different buses. Ensure LL-data reaches the memory before the
> > -	 * doorbell register is toggled by issuing the dummy-read from the remote
> > -	 * LL memory in a hope that the MRd TLP will return only after the
> > -	 * last MWr TLP is completed
> > -	 */
> > -	if (!(chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL))
> > -		readl(chan->ll_region.vaddr.io);
> > -}
> > -
> >  static void dw_edma_v0_core_ch_config(struct dw_edma_chan *chan)
> >  {
> >  	struct dw_edma *dw = chan->dw;
> > @@ -624,8 +610,6 @@ static void dw_edma_v0_core_ch_doorbell(struct dw_edma_chan *chan)
> >  {
> >  	struct dw_edma *dw = chan->dw;
> >
> > -	dw_edma_v0_sync_ll_data(chan);
> > -
> >  	/* Doorbell */
> >  	SET_RW_32(dw, chan->dir, doorbell,
> >  		  FIELD_PREP(EDMA_V0_DOORBELL_CH_MASK, chan->id));
> > diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> > index afc2f24fecd3..64a07fbc5be3 100644
> > --- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
> > +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> > @@ -282,20 +282,6 @@ static void dw_hdma_v0_core_ch_enable(struct dw_edma_chan *chan)
> >  		  HDMA_V0_CONSUMER_CYCLE_STAT | HDMA_V0_CONSUMER_CYCLE_BIT);
> >  }
> >
> > -static void dw_hdma_v0_sync_ll_data(struct dw_edma_chan *chan)
> > -{
> > -	/*
> > -	 * In case of remote HDMA engine setup, the DW PCIe RP/EP internal
> > -	 * configuration registers and application memory are normally accessed
> > -	 * over different buses. Ensure LL-data reaches the memory before the
> > -	 * doorbell register is toggled by issuing the dummy-read from the remote
> > -	 * LL memory in a hope that the MRd TLP will return only after the
> > -	 * last MWr TLP is completed
> > -	 */
> > -	if (!(chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL))
> > -		readl(chan->ll_region.vaddr.io);
> > -}
> > -
> >  static void dw_hdma_v0_core_non_ll_start(struct dw_edma_chan *chan,
> >  					 struct dw_edma_burst *child)
> >  {
> > @@ -393,8 +379,6 @@ static void dw_hdma_v0_core_ch_doorbell(struct dw_edma_chan *chan)
> >  {
> >  	struct dw_edma *dw = chan->dw;
> >
> > -	dw_hdma_v0_sync_ll_data(chan);
> > -
> >  	/* Doorbell */
> >  	SET_CH_32(dw, chan->dir, chan->id, doorbell, HDMA_V0_DOORBELL_START);
> >  }
> > --
> > 2.51.0
> >