Message-ID: <aXEKeojreN06HIdF@lizhi-Precision-Tower-5810>
Date: Wed, 21 Jan 2026 12:18:50 -0500
From: Frank Li <Frank.li@....com>
To: "Verma, Devendra" <Devendra.Verma@....com>
Cc: "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
	"mani@...nel.org" <mani@...nel.org>,
	"vkoul@...nel.org" <vkoul@...nel.org>,
	"dmaengine@...r.kernel.org" <dmaengine@...r.kernel.org>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Simek, Michal" <michal.simek@....com>
Subject: Re: [PATCH v8 2/2] dmaengine: dw-edma: Add non-LL mode

On Tue, Jan 20, 2026 at 11:36:10AM +0000, Verma, Devendra wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> > -----Original Message-----
> > From: Frank Li <Frank.li@....com>
> > Sent: Monday, January 19, 2026 9:15 PM
> > To: Verma, Devendra <Devendra.Verma@....com>
> > Cc: bhelgaas@...gle.com; mani@...nel.org; vkoul@...nel.org;
> > dmaengine@...r.kernel.org; linux-pci@...r.kernel.org; linux-
> > kernel@...r.kernel.org; Simek, Michal <michal.simek@....com>
> > Subject: Re: [PATCH v8 2/2] dmaengine: dw-edma: Add non-LL mode
> >
> > Caution: This message originated from an External Source. Use proper caution
> > when opening attachments, clicking links, or responding.
> >
> >
> > On Mon, Jan 19, 2026 at 09:09:17AM +0000, Verma, Devendra wrote:
> > > [AMD Official Use Only - AMD Internal Distribution Only]
> > >
> > > Hi Frank
> > >
> > > Please check my response inline.
> > >
> > > Regards,
> > > Devendra
> > > > -----Original Message-----
> > > > From: Frank Li <Frank.li@....com>
> > > > Sent: Thursday, January 15, 2026 10:07 PM
> > > > To: Verma, Devendra <Devendra.Verma@....com>
> > > > Cc: bhelgaas@...gle.com; mani@...nel.org; vkoul@...nel.org;
> > > > dmaengine@...r.kernel.org; linux-pci@...r.kernel.org; linux-
> > > > kernel@...r.kernel.org; Simek, Michal <michal.simek@....com>
> > > > Subject: Re: [PATCH v8 2/2] dmaengine: dw-edma: Add non-LL mode
> > > >
> > > > Caution: This message originated from an External Source. Use proper
> > > > caution when opening attachments, clicking links, or responding.
> > > >
> > > >
> > > > On Fri, Jan 09, 2026 at 05:33:54PM +0530, Devendra K Verma wrote:
> > > > > AMD MDB IP supports Linked List (LL) mode as well as non-LL mode.
> > > > > The current code does not have the mechanisms to enable the DMA
> > > > > transactions using the non-LL mode. The following two cases are
> > > > > added with this patch:
> > > > > - For the AMD (Xilinx) only, when a valid physical base address of
> > > > >   the device side DDR is not configured, then the IP can still be
> > > > >   used in non-LL mode. For all the channels DMA transactions will
> > > >
> > > > If DDR have not configured, where DATA send to in device side by
> > > > non-LL mode.
> > > >
> > >
> > > The DDR base address in the VSEC capability is used for driving the
> > > DMA transfers when used in the LL mode. The DDR is configured and
> > > present all the time but the DMA PCIe driver uses this DDR base
> > > address (physical address) to configure the LLP address.
> > >
> > > In the scenario, where this DDR base address in VSEC capability is not
> > > configured then the current controller cannot be used as the default
> > > mode supported is LL mode only. In order to make the controller usable
> > > non-LL mode is being added which just needs SAR, DAR, XFERLEN and
> > > control register to initiate the transfer. So, the DDR is always
> > > present, but the DMA PCIe driver need to know the DDR base physical
> > > address to make the transfer. This is useful in scenarios where the memory
> > allocated for LL can be used for DMA transactions as well.
> >
> > Do you means use DMA transfer LL's context?
> >
>
> Yes, the device side memory reserved for the link list to store the descriptors,
> accessed by the host via BAR_2 in this driver code.
>
> > >
> > > > >   be using the non-LL mode only. This, the default non-LL mode,
> > > > >   is not applicable for Synopsys IP with the current code addition.
> > > > >
> > > > > - If the default mode is LL-mode, for both AMD (Xilinx) and Synosys,
> > > > >   and if user wants to use non-LL mode then user can do so via
> > > > >   configuring the peripheral_config param of dma_slave_config.
> > > > >
> > > > > Signed-off-by: Devendra K Verma <devendra.verma@....com>
> > > > > ---
> > > > > Changes in v8
> > > > >   Cosmetic change related to comment and code.
> > > > >
> > > > > Changes in v7
> > > > >   No change
> > > > >
> > > > > Changes in v6
> > > > >   Gave definition to bits used for channel configuration.
> > > > >   Removed the comment related to doorbell.
> > > > >
> > > > > Changes in v5
> > > > >   Variable name 'nollp' changed to 'non_ll'.
> > > > >   In the dw_edma_device_config() WARN_ON replaced with dev_err().
> > > > >   Comments follow the 80-column guideline.
> > > > >
> > > > > Changes in v4
> > > > >   No change
> > > > >
> > > > > Changes in v3
> > > > >   No change
> > > > >
> > > > > Changes in v2
> > > > >   Reverted the function return type to u64 for
> > > > >   dw_edma_get_phys_addr().
> > > > >
> > > > > Changes in v1
> > > > >   Changed the function return type for dw_edma_get_phys_addr().
> > > > >   Corrected the typo raised in review.
> > > > > ---
> > > > >  drivers/dma/dw-edma/dw-edma-core.c    | 42
> > +++++++++++++++++++++---
> > > > >  drivers/dma/dw-edma/dw-edma-core.h    |  1 +
> > > > >  drivers/dma/dw-edma/dw-edma-pcie.c    | 46 ++++++++++++++++++--
> > ------
> > > > >  drivers/dma/dw-edma/dw-hdma-v0-core.c | 61
> > > > > ++++++++++++++++++++++++++++++++++-
> > > > >  drivers/dma/dw-edma/dw-hdma-v0-regs.h |  1 +
> > > >
> > > > edma-v0-core.c have not update, if don't support, at least need
> > > > return failure at dw_edma_device_config() when backend is eDMA.
> > > >
> > > > >  include/linux/dma/edma.h              |  1 +
> > > > >  6 files changed, 132 insertions(+), 20 deletions(-)
> > > > >
> > > > > diff --git a/drivers/dma/dw-edma/dw-edma-core.c
> > > > > b/drivers/dma/dw-edma/dw-edma-core.c
> > > > > index b43255f..d37112b 100644
> > > > > --- a/drivers/dma/dw-edma/dw-edma-core.c
> > > > > +++ b/drivers/dma/dw-edma/dw-edma-core.c
> > > > > @@ -223,8 +223,32 @@ static int dw_edma_device_config(struct
> > > > dma_chan *dchan,
> > > > >                                struct dma_slave_config *config)  {
> > > > >       struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> > > > > +     int non_ll = 0;
> > > > > +
> > > > > +     if (config->peripheral_config &&
> > > > > +         config->peripheral_size != sizeof(int)) {
> > > > > +             dev_err(dchan->device->dev,
> > > > > +                     "config param peripheral size mismatch\n");
> > > > > +             return -EINVAL;
> > > > > +     }
> > > > >
> > > > >       memcpy(&chan->config, config, sizeof(*config));
> > > > > +
> > > > > +     /*
> > > > > +      * When there is no valid LLP base address available then the default
> > > > > +      * DMA ops will use the non-LL mode.
> > > > > +      *
> > > > > +      * Cases where LL mode is enabled and client wants to use the non-LL
> > > > > +      * mode then also client can do so via providing the peripheral_config
> > > > > +      * param.
> > > > > +      */
> > > > > +     if (config->peripheral_config)
> > > > > +             non_ll = *(int *)config->peripheral_config;
> > > > > +
> > > > > +     chan->non_ll = false;
> > > > > +     if (chan->dw->chip->non_ll || (!chan->dw->chip->non_ll && non_ll))
> > > > > +             chan->non_ll = true;
> > > > > +
> > > > >       chan->configured = true;
> > > > >
> > > > >       return 0;
> > > > > @@ -353,7 +377,7 @@ static void
> > > > > dw_edma_device_issue_pending(struct
> > > > dma_chan *dchan)
> > > > >       struct dw_edma_chan *chan = dchan2dw_edma_chan(xfer->dchan);
> > > > >       enum dma_transfer_direction dir = xfer->direction;
> > > > >       struct scatterlist *sg = NULL;
> > > > > -     struct dw_edma_chunk *chunk;
> > > > > +     struct dw_edma_chunk *chunk = NULL;
> > > > >       struct dw_edma_burst *burst;
> > > > >       struct dw_edma_desc *desc;
> > > > >       u64 src_addr, dst_addr;
> > > > > @@ -419,9 +443,11 @@ static void
> > > > > dw_edma_device_issue_pending(struct
> > > > dma_chan *dchan)
> > > > >       if (unlikely(!desc))
> > > > >               goto err_alloc;
> > > > >
> > > > > -     chunk = dw_edma_alloc_chunk(desc);
> > > > > -     if (unlikely(!chunk))
> > > > > -             goto err_alloc;
> > > > > +     if (!chan->non_ll) {
> > > > > +             chunk = dw_edma_alloc_chunk(desc);
> > > > > +             if (unlikely(!chunk))
> > > > > +                     goto err_alloc;
> > > > > +     }
> > > >
> > > > non_ll is the same as ll_max = 1. (or 2, there are link back entry).
> > > >
> > > > If you set ll_max = 1, needn't change this code.
> > > >
> > >
> > > The ll_max is defined for the session till the driver is loaded in the kernel.
> > > This code also enables the non-LL mode dynamically upon input from the
> > > DMA client. In this scenario, touching ll_max would not be a good idea
> > > as the ll_max controls the LL entries for all the DMA channels not
> > > just for a single DMA transaction.
> >
> > You can use new variable, such as ll_avail.
> >
>
> In order to separate out the execution paths a new meaningful variable "non_ll"
> is used. The variable "non_ll" alone is sufficient. Using another variable
> along side "non_ll" for the similar purpose may not have any added advantage.

ll_avail can help debug/fine tune how much impact preformance by adjust
ll length. And it make code logic clean and consistent. also ll_avail can
help test corner case when ll item small. Normall case it is hard to reach
ll_max.

>
> > >
> > > > >
...
> > > > > +
> > > > > + ll_block->bar);
> > > >
> > > > This change need do prepare patch, which only change
> > > > pci_bus_address() to dw_edma_get_phys_addr().
> > > >
> > >
> > > This is not clear.
> >
> > why not. only trivial add helper patch, which help reviewer
> >
>
> I was not clear on the question you asked.
> It does not look justified when a patch is raised alone just to replace this function.
> The function change is required cause the same code *can* support different IPs from
> different vendors. And, with this single change alone in the code the support for another
> IP is added. That's why it is easier to get the reason for the change in
> the function name and syntax.

Add replace pci_bus_address() with dw_edma_get_phys_addr() to make review
easily and get ack for such replacement patches.

two patches
patch1, just replace exist pci_bus_address() with dw_edma_get_phys_addr()
patch2, add new logic in dw_edma_get_phys_addr() to support new vendor.

>
> > >
> > > > >               ll_region->paddr += ll_block->off;
> > > > >               ll_region->sz = ll_block->sz;
> > > > >
...
> > > > >
> > > > > +static void dw_hdma_v0_core_non_ll_start(struct dw_edma_chunk
> > > > *chunk)
> > > > > +{
> > > > > +     struct dw_edma_chan *chan = chunk->chan;
> > > > > +     struct dw_edma *dw = chan->dw;
> > > > > +     struct dw_edma_burst *child;
> > > > > +     u32 val;
> > > > > +
> > > > > +     list_for_each_entry(child, &chunk->burst->list, list) {
> > > >
> > > > why need iterated list, it doesn't support ll. Need wait for irq to start next
> > one.
> > > >
> > > > Frank
> > >
> > > Yes, this is true. The format is kept similar to LL mode.
> >
> > Just fill one. list_for_each_entry() cause confuse.
> >
> > Frank
>
> I see, we can use list_first_entry_or_null() which is dependent on giving
> the type of pointer, compared to this list_for_each_entry() looks neat and
> agnostic to the pointer type being used. Though, it can be explored further.
> Also, when the chunk is allocated, the comment clearly spells out how
> the allocation would be for the non LL mode so it is evident that each
> chunk would have single entry and with that understanding it is clear that
> loop will also be used in a similar manner, to retrieve a single entry. It is a
> similar use case as of "do {}while (0)" albeit needs a context to understand it.

I don't think so. list_for_each_entry() is miss leading to reader think it
is not only to one item in burst list, and use polling method to to finish
many burst transfer.

list_for_each_entry() {
	...
	readl_timeout()
}

Generally, EDMA is very quick, polling is much quicker than irq if data
is small.

Frank

>
> > >
> > > >
> > > > > +             SET_CH_32(dw, chan->dir, chan->id, ch_en,
> > > > > + HDMA_V0_CH_EN);
> > > > > +
> > > > > +             /* Source address */
> > > > > +             SET_CH_32(dw, chan->dir, chan->id, sar.lsb,
> > > > > +                       lower_32_bits(child->sar));
> > > > > +             SET_CH_32(dw, chan->dir, chan->id, sar.msb,
> > > > > +                       upper_32_bits(child->sar));
> > > > > +
> > > > > +             /* Destination address */
> > > > > +             SET_CH_32(dw, chan->dir, chan->id, dar.lsb,
> > > > > +                       lower_32_bits(child->dar));
> > > > > +             SET_CH_32(dw, chan->dir, chan->id, dar.msb,
> > > > > +                       upper_32_bits(child->dar));
> > > > > +
> > > > > +             /* Transfer size */
> > > > > +             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;
> > > > > +             }
> > > > > +
> > > > > +             SET_CH_32(dw, chan->dir, chan->id, int_setup, val);
> > > > > +
> > > > > +             /* Channel control setup */
> > > > > +             val = GET_CH_32(dw, chan->dir, chan->id, control1);
> > > > > +             val &= ~HDMA_V0_LINKLIST_EN;
> > > > > +             SET_CH_32(dw, chan->dir, chan->id, control1, val);
> > > > > +
> > > > > +             SET_CH_32(dw, chan->dir, chan->id, doorbell,
> > > > > +                       HDMA_V0_DOORBELL_START);
> > > > > +     }
> > > > > +}
> > > > > +
> > > > > +static void dw_hdma_v0_core_start(struct dw_edma_chunk *chunk,
> > > > > +bool
> > > > > +first) {
> > > > > +     struct dw_edma_chan *chan = chunk->chan;
> > > > > +
> > > > > +     if (chan->non_ll)
> > > > > +             dw_hdma_v0_core_non_ll_start(chunk);
> > > > > +     else
> > > > > +             dw_hdma_v0_core_ll_start(chunk, first); }
> > > > > +
> > > > >  static void dw_hdma_v0_core_ch_config(struct dw_edma_chan *chan)
> > {
> > > > >       struct dw_edma *dw = chan->dw; diff --git
> > > > > a/drivers/dma/dw-edma/dw-hdma-v0-regs.h
> > > > > b/drivers/dma/dw-edma/dw-hdma-v0-regs.h
> > > > > index eab5fd7..7759ba9 100644
> > > > > --- a/drivers/dma/dw-edma/dw-hdma-v0-regs.h
> > > > > +++ b/drivers/dma/dw-edma/dw-hdma-v0-regs.h
> > > > > @@ -12,6 +12,7 @@
> > > > >  #include <linux/dmaengine.h>
> > > > >
> > > > >  #define HDMA_V0_MAX_NR_CH                    8
> > > > > +#define HDMA_V0_CH_EN                                BIT(0)
> > > > >  #define HDMA_V0_LOCAL_ABORT_INT_EN           BIT(6)
> > > > >  #define HDMA_V0_REMOTE_ABORT_INT_EN          BIT(5)
> > > > >  #define HDMA_V0_LOCAL_STOP_INT_EN            BIT(4)
> > > > > diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h
> > > > > index 3080747..78ce31b 100644
> > > > > --- a/include/linux/dma/edma.h
> > > > > +++ b/include/linux/dma/edma.h
> > > > > @@ -99,6 +99,7 @@ struct dw_edma_chip {
> > > > >       enum dw_edma_map_format mf;
> > > > >
> > > > >       struct dw_edma          *dw;
> > > > > +     bool                    non_ll;
> > > > >  };
> > > > >
> > > > >  /* Export to the platform drivers */
> > > > > --
> > > > > 1.8.3.1
> > > > >

Powered by blists - more mailing lists