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

[Serge Semin <fancer.lancer@gmail.com>]

On Fri, Mar 18, 2022 at 11:36:05PM +0530, Manivannan Sadhasivam wrote:
> On Mon, Mar 14, 2022 at 11:33:40AM +0300, Serge Semin wrote:
> > 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.
> > 
> 

> Any update on your patches?

No worries. The patches are ready. But since Frank was on vacation I
decided to rebase all of my work on top of his series. I'll finish it
up shortly and send out my patchset till Monday for review. Then Frank
will be able to pick up two patches from there so to close up his
patchset (I'll give a note which one of them is of Frank' interes).
My series will be able to be merged in after Frank's series is reviewed
and accepted.

> 
> Btw, my colleage worked on merging the two dma devices used by the eDMA core
> for read & write channels into one. Initially I thought that was not needed as
> he did that for devicetree integration, but looking deeply I think that patch is
> necessary irrespective of DT.
> 
> One standout problem is, we can't register debugfs directory under "dmaengine"
> properly because, both read and write dma devices share the same parent
> chip->dev.

Right, my series fixes that and some other problems too. So please be
patient for a few more days.

-Sergey

> 
> Thanks,
> Mani
> 
> > -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
> > > > > > >