Re: [PATCH 05/11] PCI: epf-{mhi/test}: Move DMA initialization to EPC init callback

From: Niklas Cassel <cassel@kernel.org>
To: Manivannan Sadhasivam <mani@kernel.org>
Cc: "Vinod Koul" <vkoul@kernel.org>,
	"Manivannan Sadhasivam" <manivannan.sadhasivam@linaro.org>,
	"Lorenzo Pieralisi" <lpieralisi@kernel.org>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Rob Herring" <robh@kernel.org>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Kishon Vijay Abraham I" <kishon@kernel.org>,
	"Thierry Reding" <thierry.reding@gmail.com>,
	"Jonathan Hunter" <jonathanh@nvidia.com>,
	"Jingoo Han" <jingoohan1@gmail.com>,
	"Gustavo Pimentel" <gustavo.pimentel@synopsys.com>,
	linux-pci@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-kernel@vger.kernel.org, mhi@lists.linux.dev,
	linux-tegra@vger.kernel.org,
	"Damien Le Moal" <dlemoal@kernel.org>,
	"Frank Li" <Frank.Li@nxp.com>
Subject: Re: [PATCH 05/11] PCI: epf-{mhi/test}: Move DMA initialization to EPC init callback
Date: Mon, 22 Apr 2024 11:30:09 +0200	[thread overview]
Message-ID: <ZiYuIaX7ZV0exKMt@ryzen> (raw)
In-Reply-To: <20240422075521.GB9775@thinkpad>

On Mon, Apr 22, 2024 at 01:25:21PM +0530, Manivannan Sadhasivam wrote:
> > 
> > What I would like is more consistency between the EPF drivers.
> > 
> > I guess an if-statement that skips the pci_epc_map_addr() in pci-epf-test
> > if using eDMA would make pci-epf-mhi and pci-epf-test most consistent.
> > 
> 
> Agree.

> > 1) Do we want to rely on the fact that hopefully none of the iATUs in the DWC
> > controller has configured a mapping that might mess things up for us?
> > I don't see why the PCI/DMA address of the remote buffer, supplied to
> > pci-epf-test via test_reg BAR, might not fall within the physical iATU window
> > on the local EP system. (As long as the PCI EPF driver has mapped any address
> > using pci_epc_map_addr().)
> > 
> > This is a big argument that EPF drivers running on a DWC-based EPC should
> > definitely NOT call pci_epc_map_addr() needlessly when using eDMA, as it
> > can be catastrophic. (pci-epf-test needs to be patched.)
> > 
> 
> Right. There is no need to do iATU translation for DMA. I avoid that in MHI
> driver.

There is no need for pci_epc_map_addr() when using DMA_SLAVE *for DWC-based
controllers*.

Are we certain that this will not break pci-epf-test for non DWC-based
controllers?

> > 2) Can we really assume that both pci-epf-test and pci-epf-mhi does not need
> > to call pci_epc_map_addr() when using a DMA_SLAVE DMA controller?
> > This seems to be designed only with DWC in mind. Other PCIe endpoint
> > controllers might require this.
> > (Yes, for DWC-based controllers, this definitely should be skipped, but EPF
> > drivers are supposed to be independent from a specific EPC.)
> > 
> 
> For TEST yes, but for MHI, no. In MHI, I kind of mix both iATU and DMA to ripe
> most of the performance (small vs big transactions). But for the TEST driver, it
> is fair to not call pci_epc_map_addr() when DMA_SLAVE is supported.

I agree that we should definitely skip pci_epc_map_addr() in pci-epf-test when
using DMA_SLAVE on DWC-based controllers, but I don't feel comfortable in
submitting a patch that does this unconditionally for pci-epf-test.c,
as I don't know how the DMA hardware in:
drivers/pci/controller/cadence/pcie-cadence-ep.c
drivers/pci/controller/pcie-rcar-ep.c
drivers/pci/controller/pcie-rockchip-ep.c

works, and I do not want to regress them.

I did suggest that DWC-based drivers could set a DMA_SLAVE_SKIP_MEM_MAP flag
or similar when registering the eDMA, which pci-epf-test then could check,
but I got no response if anoyone else thought that this was a good idea.

Kind regards,
Niklas