Re: [PATCH v4 5/8] dmaengine: dw-edma: Fix programming the source & dest addresses for ep

From: Serge Semin <fancer.lancer@gmail.com>
To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: Serge Semin <Sergey.Semin@baikalelectronics.ru>,
	Frank Li <Frank.Li@nxp.com>,
	gustavo.pimentel@synopsys.com, hongxing.zhu@nxp.com,
	l.stach@pengutronix.de, linux-imx@nxp.com,
	linux-pci@vger.kernel.org, dmaengine@vger.kernel.org,
	lznuaa@gmail.com, vkoul@kernel.org, lorenzo.pieralisi@arm.com,
	robh@kernel.org, kw@linux.com, bhelgaas@google.com,
	shawnguo@kernel.org
Subject: Re: [PATCH v4 5/8] dmaengine: dw-edma: Fix programming the source & dest addresses for ep
Date: Mon, 14 Mar 2022 11:33:40 +0300	[thread overview]
Message-ID: <20220314083340.244dfwo4v3uuhkkm@mobilestation> (raw)
In-Reply-To: <20220312053720.GA4356@thinkpad>

On Sat, Mar 12, 2022 at 11:07:20AM +0530, Manivannan Sadhasivam wrote:
> On Fri, Mar 11, 2022 at 10:01:47PM +0300, Serge Semin wrote:
> > On Fri, Mar 11, 2022 at 11:11:34PM +0530, Manivannan Sadhasivam wrote:

[nip]

> > 
> > > As per my understanding, the eDMA is solely used in the PCIe endpoint. And the
> > > access to it happens over PCIe bus or by the local CPU.
> > 
> > Not fully correct. Root Ports can also have eDMA embedded. In that
> > case the eDMA can be only accessible from the local CPU. At the same
> > time the DW PCIe End-point case is the IP-core synthesize parameters
> > specific. It's always possible to access the eDMA CSRs from local
> > CPU, but a particular End-point BAR can be pre-synthesize to map
> > either Port Logic, or eDMA or iATU CSRs. Thus a PCIe root port can
> > perform a full End-point configuration. Anyway the case if the eDMA
> > functionality being accessible over the PCIe wire doesn't really make
> > much sense with no info regarding the application logic hidden behind
> > the PCIe End-point interface since SAR/DAR LLP is supposed to be
> > initialized with an address from the local (application) memory space.
> > 
> 
> Thanks for the explanation, it clarifies my doubt. I got misleaded by the
> earlier commits...
> 
> > So AFAICS the main usecase of the controller is 1) - when eDMA is a
> > part of the Root Port/End-point and only local CPU is supposed to have
> > it accessed and configured.
> > 
> > I can resend this patch with my fix to the problem. What do you think?
> > 
> 

> Yes, please do.

Ok. I'll be AFK today, but will send my patches tomorrow.  @Frank,
Could you please hold on with respinning the series for a few days?
I'll send out some of my patches then with a note which one of them
could be picked up by you and merged into this series.

-Sergey

> 
> Thanks,
> Mani
> 
> > -Sergey
> > 
> > > 
> > > The commit from Alan Mikhak is what I took as a reference since the patch was
> > > already merged:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/dma/dw-edma?id=bd96f1b2f43a39310cc576bb4faf2ea24317a4c9
> > > 
> > > Thanks,
> > > Mani
> > > 
> > > > -Sergey
> > > > 
> > > > > +		 *
> > > > > +		 ****************************************************************/
> > > > > +
> > > > 
> > > > > +		if ((dir == DMA_DEV_TO_MEM && chan->dir == EDMA_DIR_READ) ||
> > > > > +		    (dir == DMA_DEV_TO_MEM && chan->dir == EDMA_DIR_WRITE))
> > > > > +			read = true;
> > > > 
> > > > Seeing the driver support only two directions DMA_DEV_TO_MEM/DMA_DEV_TO_MEM
> > > > and EDMA_DIR_READ/EDMA_DIR_WRITE, this conditional statement seems
> > > > redundant.
> > > > 
> > > > > +
> > > > > +		/* Program the source and destination addresses for DMA read/write */
> > > > > +		if (read) {
> > > > >  			burst->sar = src_addr;
> > > > >  			if (xfer->type == EDMA_XFER_CYCLIC) {
> > > > >  				burst->dar = xfer->xfer.cyclic.paddr;
> > > > > -- 
> > > > > 2.24.0.rc1
> > > > > 