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

On Tue, Jul 28, 2026 at 10:43:57AM -0500, Frank Li wrote:
> On Tue, Jul 28, 2026 at 02:38:16PM +0900, Koichiro Den wrote:
> > 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 <[email protected]>
> > > > ---
> > > > 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?
> 
> Yes, doorbell() should include memory barrier.

Got it, thanks for the confirmation. In that case, and if the two points (a) and
(b) above are otherwise fine, I will probably move more of the software logic
into dw_edma_core_ch_doorbell(). Let me think through the exact implementation
for v4.

Thanks for the review,
Koichiro

> 
> Frank
> 
> >
> > 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
> > > >