RE: [PATCH RESEND v10 2/2] dmaengine: dw-edma: Add non-LL mode

From: Verma, Devendra

Date: Fri Feb 20 2026 - 06:48:19 EST

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Frank
Please check my response inline

Regards,
Devendra
> -----Original Message-----
> From: Frank Li <Frank.li@xxxxxxx>
> Sent: Thursday, February 19, 2026 10:38 PM
> To: Verma, Devendra <Devendra.Verma@xxxxxxx>
> Cc: bhelgaas@xxxxxxxxxx; mani@xxxxxxxxxx; vkoul@xxxxxxxxxx;
> dmaengine@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Simek, Michal <michal.simek@xxxxxxx>
> Subject: Re: [PATCH RESEND v10 2/2] dmaengine: dw-edma: Add non-LL
> mode

---[ Snipped some text to reduce mail size ]---

> > > > > > > On Mon, Feb 16, 2026 at 04:25:46PM +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
> > > > > > > > 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@xxxxxxx>
> > > > > > > > ---
> > > > > > > > Changes in v10
> > > > > > > > Added the peripheral_config check only for HDMA IP in
> > > > > > > > dw_edma_device_config().
> > > > > > > > Replaced the loop with single entry retrieval for non-LL
> > > > > > > > mode.
> > > > > > > > Addressed review comments and handled the burst allocation
> > > > > > > > by defining 'bursts_max' as per suggestions.
> > > > > > > >
> > > > > > > > Changes in v9
> > > > > > > > Fixed compilation errors related to macro name mismatch.
> > > > > > > >
> > > > > > > > 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 | 35 ++++++++++++++-
> > > > > > > > drivers/dma/dw-edma/dw-edma-core.h | 1 +
> > > > > > > > drivers/dma/dw-edma/dw-edma-pcie.c | 44 ++++++++++++----
> --
> > > > > > > > drivers/dma/dw-edma/dw-hdma-v0-core.c | 65
> > > > > > > > ++++++++++++++++++++++++++- drivers/dma/dw-edma/dw-
> hdma-
> > > v0-
> > > > > > > regs.h | 1 +
> > > > > > > > include/linux/dma/edma.h | 1 +
> > > > > > > > 6 files changed, 132 insertions(+), 15 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/dma/dw-edma/dw-edma-core.c
> > > > > > > > b/drivers/dma/dw-edma/dw-edma-core.c
> > > > > > > > index b43255f914f3..ef3d79a9f88d 100644
> > > > > > > > --- a/drivers/dma/dw-edma/dw-edma-core.c
> > > > > > > > +++ b/drivers/dma/dw-edma/dw-edma-core.c
> > > > > > > > @@ -223,6 +223,31 @@ 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;
> > > > > > > > +
> > > > > > > > + chan->non_ll = false;
> > > > > > > > + if (chan->dw->chip->mf == EDMA_MF_HDMA_NATIVE) {
> > > > > > >
> > > > > > > Need handle EMDA case. if mf is EDMA, need return error when
> > > > > > > config->peripheral_config is not NULL. Or remove above check
> > > > > > > config->to make
> > > > > > > code work for both EDMA or HDMA.
> > > > > > >
> > > > > >
> > > > > > For the case of EDMA, the behavior is unchanged.
> > > > > > Even if the config->peripheral_config param is set then it
> > > > > > would be simply
> > > > > ignored.
> > > > > > This is retention of the previous behavior. This is done
> > > > > > because device_slave_config has other params which affect the
> > > > > > behavior of the DMA transactions, we do not check all those
> > > > > > params and return any error. The error is returned only in the
> > > > > > cases where particular parameter from dma_slave_config is used
> > > > > > and if the parameter is not as expected or in the expected
> > > > > > form. The parameter used from dma_slave_config for the
> > > > > > particular IP type need to be known first then it
> > > > > can be checked for its correctness. This is behavior for the
> > > > > peripheral_config which is used for HDMA and thus error checked.
> > > > >
> > > > > It actaully hidden error. Your patch provide an option, which
> > > > > need't ll memory to do DMA transfer. DMA consumer actaully don't
> > > > > know which backend used, which is private information by DMA
> engine providor.
> > > > >
> > > > > dw-edma-pcie.c is only one of all edma users, which know DMA
> > > > > engine's information because it creates this interface.
> > > > >
> > > > > PCIE-EP framework also create dmaegine, PCIE-EP function driver
> > > > > use DMA standard interface to get dma-chan.
> > > > >
> > > > > if (config->peripheral_config) { /* only your specific dma
> > > > > consumer set it now */
> > > > > /* optional config information */
> > > > > if (chan->dw->chip->mf != EDMA_MF_HDMA_NATIVE) {
> > > > > dev_err("edma have not implmentent no-lll mode\n")
> > > > > return -EINVAL
> > > > > }
> > > > >
> > > > > ...
> > > > > }
> > > > >
> > > > > Add EDMA support no-ll mode is quite easily in future.
> > > > >
> > > >
> > > > This looks reasonable provided that HDMA got the support for this
> param.
> > > > An error check can be added in the next revision.
> > > > The addition may be slightly different as following:
> > > > If (chan->dw->chip->mf == EDMA_MF_HDMA_NATIVE) { ...
> > > > } else if (config->peripheral_config) {
> > > > /* error handling */
> > > > }
> > > >
> > > > Using the above, if support needs to be added to EDMA then a check
> > > > for
> > > correct 'mf'
> > > > in the if() shall be sufficient.
> > > >
> > > > > >
> > > > > > > > + if (config->peripheral_config &&
> > > > > > > > + config->peripheral_size != sizeof(int)) {
> > > > > > > > + dev_err(dchan->device->dev,
> > > > > > > > + "config param peripheral size mismatch\n");
> > > > > > > > + return -EINVAL;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + /*
> > > > > > > > + * 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;
> > > > > > > > +
> > > > > > > > + if (chan->dw->chip->non_ll ||
> > > > > > > > + (!chan->dw->chip->non_ll && non_ll))
> > > > > > >
> > > > > > > if chan->dw->chip->non_ll is true, It should return error if
> > > > > > > you set non_ll false because no LLP base available.
> > > > > > >
> > > > > >
> > > > > > In case the 'chan->dw->chip->non_ll' is true, then the default
> > > > > >mode becomes non-LL for HDMA ONLY. There is no option to the
> > > > > >user to configure the LL mode by giving 'non_ll = false' as
> > > > > >part of the
> > > > > >config- peripheral_config.
> > > > >
> > > > > This is API's callback, you can't assume caller do all correct things.
> > > > >
> > > > > > The configuration of default non-LL mode depends on how the IP
> > > > > > is programmed by the user. The user is aware of the IP configuration.
> > > > >
> > > > > It is not true. DMA consumer don't know such detail information,
> > > > > which only know which dma engineer providor.
> > > > >
> > > >
> > > > For the DMA consumer the only option is LL mode as default mode.
> > > > In order to use the non-LL mode it need to provide the parameter
> > > > in the form
> > > of peripheral_config.
> > > > Given the above statement, the consumer even if gives the 'non_ll
> > > > = false', it is not going to change any behavior.
> > > > Even if the user is not giving the option the assumption is that
> > > > controller is in LL mode, unless the DMA engine provider provides
> > > > the information regarding non-LL mode as default mode to the DMA
> > > > consumer
> > > explicitly.
> > > > In the case where chan->dw->chip->non_ll = true, following case
> > > > may
> > > happen:
> > > > - DMA consumer provides no peripheral_config param or simply
> > > >config- peripheral_config = NULL,
> > > > in this case non_ll = false which is the current flow.
> > > > - DMA consumer provides a valid peripheral_config (!= NULL) param
> > > >but the
> > > value is '0', in this case
> > > > It is explicit but it would have the same effect as above case.
> > > >
> > > > DMA consumer is supposed to provide the only option non_ll as it
> > > > was not available and LL mode is set as default for the DMA operations.
> > > > When 'chan->dw->chip->non_ll = true' then this was added to make
> > > > the chip usable when the LLP base addresses are not configured.
> > > > Without this, user cannot use any of the modes be it LL or non-LL
> > > > if the LLP base
> > > address is not configured.
> > >
> > > little bit confuse, Maybe the same as you. I expected behavor
> > >
> > > config->peripheral_config = NULL choose hardware default one
> > > - LL mode if hardware support
> > > - none-LL mode if not ll list region
> > >
> > > config->peripheral_config != NULL
> > > EDMA: return false
> > > HDMA:
> > > 0 force to none_ll mode. (always success)
> > > 1 force back to ll mode (return false if no ll list region
> in
> > > chip)
> > >
> > > DMA consumer decide if fall back to none_ll to continue.
> > >
> >
> > Thank you for the elaboration!
> > I have few questions, why shall a DMA consumer decide to enable LL
> > mode when the default mode supported is LL mode only?
>
> LL mode only is software driver implement. Hardware support both LL mode
> and no-LL mode. Previous driver implement only support LL mode. You try to
> add non-LL mode. Choose straightforward forward method.
>
> One indicate hardware capacity, one actually used. Like PCI INTX and MSI.
> If support MSI, most case use MSI. But still support switch to use INTX.
>
> My key point avoid hidden beavior. Every branch is clean and straightforward.
>
> >
> > If DMA consumer is trying to enable the LL mode, then one must be
> > knowing the configuration of the controller that controller is working
> > in non-LL mode, as LLP base address is not configured,then why to try and
> enable the LL mode?
>
> The DMA consumer don't know these informaiton.
>
> >
> > The user need to know, at least, one detail from the above two cases.
> >
> > The use for non-LL mode is useful in the following scenario:
> > - When user want to utilize the LL regions also for DMA data transfers.
> > - For single and big chunks non-LL mode is useful in both use-cases when
> non-LL mode is default or
> > user enables it via peripheral_config params.
> > - This addition, inadvertently, makes the DMA controller usable, for AMD
> (Xilinx) only, when the LLP
> > base addresses are not configured; it can be used in non-LL mode.
>
> LL regions may not visiable, User can use non-ll to config LL-region and switch
> back to use LL-region to continue transfer. User may use non-ll as indirectly
> reg access.
>
> > For Synopsys, DMA controller
> > cannot be used in any mode if LLP base address is not configured.
>
> Does spec said it? It doesn't make sense. it should be controlled by LLE of
> DMA_CH_CONTROL1_OFF_RDCH_0.
>
> >
> > Based on the above points, if user is trying to enable LL mode when
> > default mode is LL mode, it looks Intentionally making the choice when user
> is aware of the mode DMA controller operating in.
> > Please let me know if this clarifies the doubt.
>
> No API to get mode, only use set and test to know it.
>
> Actually Needn't consider so complex. like functions API(x)
>
> We just consider input x,
>
> validate x's input ragion,
>
> if x is out of region, just return error.
>

Thanks! Will update in the next version.

> >
> > > >
> > > > > > The check for non-LL option
> > > > > > provided by the user is useful when LL-mode is default. That
> > > > > > is why the value of non_ll == false is not even evaluated.
> > > > > >
> > > > > > > > + chan->non_ll = true;
> > > > > > > > + }
> > > > > > > >
> > > > > ...
> > > > > > > > diff --git a/include/linux/dma/edma.h
> > > > > > > > b/include/linux/dma/edma.h index
> > > > > > > > 3080747689f6..78ce31b049ae
> > > > > > > > 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;
> > > > > > >
> > > > > > > Can you check ll_region directly? it should be equal to
> > > > > > > (ll_region->sz == 0)
> > > > > ?
> > > > >
> > > > > Do you miss this questin?
> > > > >
> > > > > Frank
> > > > >
> > > >
> > > > Yes, looks like I missed this question. Could you explain a little
> > > > bit more? I
> > > am unable to understand the context.
> > >
> > > you set chip->non_ll = non_ll in dw_edma_pcie_probe()
> > >
> > > and only set ll_region->sz = ll_block->sz when !chip->non_ll.
> > >
> > > Thats means ll_region->sz is 0 when chip->non_ll is true.
> > >
> > > So non_ll have not bring new infomation into dw_edma_chip.
> > >
> > > check !ll_region->sz, which should be equal to this non_ll.
> > >
> > > dw_edma_chip is the exchange information between controller and dma
> > > core driver. Needn't cache it here because you already save a copy in dma-
> chan.
> > >
> > > Frank
> >
> > I understand the concern here but it does not look good to piggyback
> > the non_ll related information on the existing variable.
> > The use of bool readily points out the information related to what
> > mode is being intended but using the ll_region->sz is an inference the user
> has to make.
> >
> > Having ll_region->sz == 0 does not really tell it is non_ll mode or
> > not, it can also mean that the size of LL region is zero while in LL mode
> which could be an error.
> > This does not translate to support for non-LL mode. This brings the
> ambiguity.
> > The introduction of the non_ll provides clarity and easy comparison
> > with the similar choice (non_ll) provided by the DMA consumer in the
> dmaengine_slave_config().
> > I request we shall retain the clarity here.
>
> You can use helper(dw_chip_is_support_ll()) macro to check chip's
> capatiblity.
>

I do not understand what you mean with the above statement.
But if it about writing a new function to check the LL mode support
then I think the current variable is good enough which provides good readability
and do not create any ambiguity compared to the ll region size comparison.

> Frank
> >
> > > >
> > > > > > >
> > > > > > > Frank
> > > > > > > > };
> > > > > > > >
> > > > > > > > /* Export to the platform drivers */
> > > > > > > > --
> > > > > > > > 2.43.0
> > > > > > > >