Message-ID:
 <SA1PR12MB81203BB877B64C4E525EC0269594A@SA1PR12MB8120.namprd12.prod.outlook.com>
Date: Fri, 23 Jan 2026 14:26:15 +0000
From: "Verma, Devendra" <Devendra.Verma@....com>
To: Frank Li <Frank.li@....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>, "Verma, Devendra" <Devendra.Verma@....com>
Subject: RE: [PATCH v8 2/2] dmaengine: dw-edma: Add non-LL mode

[AMD Official Use Only - AMD Internal Distribution Only]

> -----Original Message-----
> From: Frank Li <Frank.li@....com>
> Sent: Wednesday, January 21, 2026 10:49 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.

--[ Snipped some headers to reduce the size of this mail ]--

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

Thank you for your suggestion. The ll_max is max limit on the descriptors that can
be accommodated on the device side DDR. The ll_avail will always be less than ll_max.
The optimization being referred can be tried without even having to declare the ll_avail
cause the number of descriptors given can be controlled by the DMA client based on the length
argument to the dmaengine_prep_* APIs. So, the use of dynamic ll_avail is not necessarily
required. Without increasing the ll_max, ll_avail cannot be increased. In order to increase
ll_max one may need to alter size and recompile this driver.

However, the requirement of ll_avail does not help for the supporting the non-LL mode.
For non-LL mode to use:
1) Either LL mode shall be not available, as it can happen for the Xilinx IP.
2) User provides the preference for non-LL mode.
For both above, the function calls are different which can be differentiated by using
the "non_ll" flag. So, even if I try to accommodate ll_avail, the call for LL or non-LL would be
ambiguous as in case of LL mode also we can have a single descriptor as similar to non-LL mode.
Please check the function dw_hdma_v0_core_start() in this review where the decision is taken
Based on non_ll flag.

> >
> > > >
> > > > > >
> ...
> > > > > > +
> > > > > > + 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.
>

I understand your concern about making the review easier. However, given that
we've been iterating on this patch series since September and are now at v9,
I believe the current approach is justified. The function renames from
pci_bus_address() to dw_edma_get_phys_addr() is directly tied to the
non-LL mode functionality being added - it's needed because the same code
now supports different IPs from different vendors.

Splitting this into a separate preparatory patch at this stage would further
delay the review process. The change is kind of straightforward and the context is clear
within the current patch. I request you to review this patch to avoid additional review cycles.

This also increases the work related to testing and maintaining multiple patches.
I have commitment for delivery of this, and I can see adding one more series definitely
add 3-4 months of review cycle from here. Please excuse me but this code has already
been reviewed extensively by other reviewers and almost by you as well. You can check
the detailed discussion wrt this function at the following link:
https://lore.kernel.org/all/SA1PR12MB8120341DFFD56D90EAD70EDE9514A@SA1PR12MB8120.namprd12.prod.outlook.com/

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

The polling is not required. The single burst will raise the interrupt and from the
interrupt context another chunk will be scheduled. This cycle repeats till all the chunks
with single burst are exhausted.

The following comment made in function dw_edma_device_transfer() in the same patch
makes it amply clear that only a single burst would be handled for the non-LL mode.
+       /*
+        * For non-LL mode, only a single burst can be handled
+        * in a single chunk unlike LL mode where multiple bursts
+        * can be configured in a single chunk.
+        */

Looking at the way bursts are appended to chunks and chunks in dw_edma_device_transfer()
are scheduled for non-LL mode then it is clear what non-LL mode would handle in terms of bursts.
I just kept the format to match it with the LL mode format otherwise there is no
need of this comment and we can follow the syntax for a single entry alone.
Please share your suggestion if these descriptions fail to provide the clear context and intent.

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