| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aYIn4PQioFJD7qzK@lizhi-Precision-Tower-5810>
Date: Tue, 3 Feb 2026 11:52:48 -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, Feb 03, 2026 at 12:03:54PM +0000, Verma, Devendra wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> Hi Frank
>
> Any update on this review?
>
> Regards,
> Devendra
>
> > -----Original Message-----
> > From: Verma, Devendra <Devendra.Verma@....com>
> > Sent: Thursday, January 29, 2026 5:26 PM
> > To: Frank Li <Frank.li@....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>; 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 28, 2026 8:38 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
> >
> > --[ Snipped some headers to reduce the size of this mail ]--
> >
> > > > > > > > > > > > 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@....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.
> > > > >
> > > >
> > > > I analyzed the use of ll_avail but I think the use of this variable
> > > > does not fit at this location in the code for the following reasons:
> > > > 1. The use of a new variable breaks the continuity for non-LL mode.
> > > > The
> > > variable
> > > > name non_ll is being used for driving the non-LL mode not only
> > > > in this file
> > > but also
> > > > in the files relevant to HDMA. This flag gives the clear
> > > > indication of LL vs
> > > non-LL mode.
> > > > In the function dw_hdma_v0_core_start(), non_ll decided the mode
> > > > to
> > > choose.
> > > > 2. The use of "ll_avail" is ambiguous for both the modes. First, it
> > > > is
> > > associated with LL mode only.
> > > > It will be used for optimizing / testing the Controller
> > > > performance based
> > > on the
> > > > number of descriptors available on the Device DDR side which is
> > > > for LL
> > > mode. So when
> > > > it is being used for LL mode then it is obvious that it excludes
> > > > any use for
> > > non-LL mode.
> > > > In the API dw_edma_device_transfer(), the ll_avail will be used
> > > > for
> > > creating the bursts.
> > > > If ll_avail = 1 in the above API, then it is ambiguous whether
> > > > it is creating
> > > the burst for
> > > > LL or non-LL mode. In the above API, the non_ll is sufficient
> > > > to have the
> > > LL mode
> > > > and non-LL burst allocation related piece of code.
> > >
> > > If really like non-ll, you'd better set ll_avail = 1 in prepare code.
> > > Normal case ll_avail should be ll_max. It will reduce if-else branch
> > > in prep dma burst code.
> > >
> >
> > I think we are not on the same page, and it is creating confusion.
> > If non_ll flag is being used to differentiate between the modes, then in this
> > scenario the use of ll_avail does not fit any requirement related to
> > differentiation of different modes. In the last response, I pointed out that
> > ll_avail, if used, creates ambiguity rather than bringing clarity for both LL &
> > non-LL mode. If non_ll flag is used and initialized properly then this is
> > sufficient to drive the execution for non-LL mode.
> >
> > In the function doing the preparation, there also no if-else clause is
> > introduced rather the same "if" condition is extended to support the non-LL
> > mode.
> >
> > Could you elaborate what is the approach using ll_avail if I need to maintain
> > the continuity of the non-LL context and use non-LL mode without any
> > ambiguity anywhere, instead of using non_ll flag?
> > If possible, please give a code snippet. Depending upon the usability and
> > issue it fixes, I will check its feasibility.
> >
> > > + /*
> > > + * 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.
> > > + */
> > >
> > > It is actually wrong, current software should handle that. If there
> > > are multiple bursts, only HEAD of bursts trigger DMA, in irq handle,
> > > it will auto move to next burst. (only irq latency is bigger compared
> > > to LL, software's resule is the same).
> > >
> > > The code logic is totally equal ll_max = 1, except write differece registers.
> > >
> >
> > Changing the ll_max dynamically for a single request is not feasible. As
> > pointed out earlier it may require the logic to retain the initially configured
> > value, during the probe, and then use the retained value depending on the
> > use-case.
As my previous suggest, add ll_avail, config can set it in [1..ll_max].
then replace ll_max with ll_avail.
> > Could you elaborate the statement,
> > " The code logic is totally equal ll_max = 1, except write differece registers." ?
My means don't touch actual logic in dw_edma_device_transfer() except
change ll_max to ll_avail or other variable, with value 1.
Even though you really want to use non_ll, you can use below code
dw_edma_device_transfer()
{
size_t avail = no_ll ? 1 : ll_max;
...
if (chunk->bursts_alloc == avail)
...
}
> >
> > The irq_handler() for success case calls dw_edma_start_transfer() which
> > schedules the chunks not bursts.
Current code, it is that. I forget I just change it.
> The bursts associated with that chunk will
> > get written to controller DDR area / scheduled (for non-LL) in the
> > dw_hdma_v0_core_start(), for HDMA.
Original code have unnecessary complex about chunks and burst, which actually
add overhead.
See my patch to reduce 2 useless malloc()
https://lore.kernel.org/dmaengine/20251212-edma_ll-v1-0-fc863d9f5ca3@nxp.com/
> > With this flow, for the non-LL mode each chunk needs to contain a single
> > burst as controller can handle only one burst at a time in non-LL mode.
Non-LL case, you just fill one. The only difference is fill to DDR or
registers.
Frank
> >
> >
> > > And anthor important is that,
> > >
> > > in dw_edma_device_config() should return if backend is not HDMA.
> > >
> >
> > Thanks, this is a valid concern, will address in the upcoming version.
> >
> > > Frank
> > >
> > > >
> > > > I think ll_avail, if used for trying out to optimize / debug the
> > > > settings related to number of descriptors for LL mode then it should
> > > > be part of the separate discussion / update not related to non-LL.
> > > >
> > > > > >
> > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > ...
> > > > > > > > > > > > +
> > > > > > > > > > > > + 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.
> > > > >
> > > >
> > > > Thank you for your support.
> > > >
> > > > > > 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@
> > > > > SA1
> > > > > > PR12MB8120.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.
> > > > >
> > > >
> > > > Hi Manivannan Sadhasivam, Vinod Koul and Bjorn Helgaas Could you
> > > > please provide your feedback on the patch?
> > > > You have reviewed these patches extensively on the previous versions
> > > > of
> > > the same series.
> > > >
> > > > Regards,
> > > > Devendra
> > > >
> > > > >
> > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > > 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
> > > > >
> > > >
> > > > Sure, thanks, I will push this change in next version.
> > > >
> > > > > >
> > > > > > > >
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > + 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