Message-ID: <aXe5ts7E6lUF7YRq@lizhi-Precision-Tower-5810>
Date: Mon, 26 Jan 2026 14:00:06 -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 Fri, Jan 23, 2026 at 02:26:15PM +0000, Verma, Devendra wrote:
> [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.

I optimzied it to allow dynatmic appended dma descriptors.

https://lore.kernel.org/dmaengine/20260109-edma_dymatic-v1-0-9a98c9c98536@nxp.com/T/#t

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

We can treat ll_avail == 1 as no_ll mode because needn't set extra LL in
memory at all.

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

Thank you for your persevere.

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

But still not got reviewed by tags. The recently,  Manivannan Sadhasivam
, Niklas Cassel and me active worked on this driver. You'd better to get
their feedback. Bjorn as pci maintainer to provide generally feedback.


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

Avoid use list_for_each_entry() here to prevent miss-leading.

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