| 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:
<SA1PR12MB81204D2B45E1001C00195C169598A@SA1PR12MB8120.namprd12.prod.outlook.com>
Date: Wed, 4 Feb 2026 10:50:09 +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]
Hi Frank
Thanks for your suggestions, please check my response inline.
Regards,
Devendra
> -----Original Message-----
> From: Frank Li <Frank.li@....com>
> Sent: Tuesday, February 3, 2026 10:23 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)
> ...
> }
>
Thank you for the code illustration, this is clear to me.
I suggest we shall use the bursts_max variable to compare with
chunk->bursts_alloc instead of ll_avail, that way it is easy to convey
the meaning. I tried using ll_avail but it did not suite well with the intent.
The use of ll_avail puts the upper limit on the allocation of bursts which
bursts_max shows well in comparison to ll_avail. Please review the next
update with these suggestions included.
> > >
> > > 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@....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
>
Thanks for the link, it is good improvement.
> > >
> > >
> > > > 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