Date:   Fri, 29 Apr 2022 22:50:04 +0530
From:   Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To:     Serge Semin <fancer.lancer@...il.com>
Cc:     Serge Semin <Sergey.Semin@...kalelectronics.ru>,
        Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
        Vinod Koul <vkoul@...nel.org>,
        Jingoo Han <jingoohan1@...il.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Frank Li <Frank.Li@....com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        Rob Herring <robh@...nel.org>,
        Krzysztof Wilczyński <kw@...ux.com>,
        Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>,
        Pavel Parkhomenko <Pavel.Parkhomenko@...kalelectronics.ru>,
        linux-pci@...r.kernel.org, dmaengine@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 25/25] PCI: dwc: Add DW eDMA engine support

On Fri, Apr 29, 2022 at 07:13:48PM +0300, Serge Semin wrote:
> On Thu, Apr 28, 2022 at 10:39:29PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Apr 28, 2022 at 05:05:23PM +0300, Serge Semin wrote:
> > 
> > [...]
> > 
> > > > If iATU has unroll enabled then I think we can assume that edma will also be
> > > > the same. So I was wondering if we could just depend on iatu_unroll_enabled
> > > > here.
> > > 
> > > I thought about that, but then I decided it was easier to just define
> > > a new flag. Anyway according to the hw manuals indeed the unroll
> > > mapping is enabled either for both iATU and eDMA modules or for none
> > > of them just because they are mapped over a single space. It's
> > > determined by the internal VHL parameter CC_UNROLL_ENABLE.
> > > On the second thought I agree with you then. I'll convert the
> > > iatu_unroll_enabled flag into a more generic 'reg_unroll' and make
> > > sure it's used for both modules.
> > > 
> > 
> > Sounds good!
> > 
> > > > 
> > > > > > 
> > > > > > > +	if (pci->edma_unroll_enabled && pci->iatu_unroll_enabled) {
> > > > > > > +		pci->edma.mf = EDMA_MF_EDMA_UNROLL;
> > > > > > > +		if (pci->atu_base != pci->dbi_base + DEFAULT_DBI_ATU_OFFSET)
> > > > > > > +			pci->edma.reg_base = pci->atu_base + PCIE_DMA_UNROLL_BASE;
> > > > > > > +		else
> > > > > > > +			pci->edma.reg_base = pci->dbi_base + DEFAULT_DBI_DMA_OFFSET;
> > > > > > 
> > > > > 
> > > > > > This assumption won't work on all platforms. Atleast on our platform, the
> > > > > > offsets vary. So I'd suggest to try getting the reg_base from DT first and use
> > > > > > these offsets as a fallback as we do for iATU.
> > > > > 
> > > > > I don't know how the eDMA offset can vary at least concerning the
> > > > > normal DW PCIe setup. In any case the DW eDMA controller CSRs are
> > > > > mapped in the same way as the iATU space: CS2=1 CDM=1. They are either
> > > > > created as an unrolled region mapped into the particular MMIO space
> > > > > (as a separate MMIO space or as a part of the DBI space), or
> > > > > accessible over the PL viewports (as a part of the Port Logic CSRs).
> > > > > Nothing else is described in the hardware manuals. Based on that I
> > > > > don't see a reason to add one more reg space binding.
> > > > > 
> > > > 
> > > 
> > > > This is not true. Vendors can customize the iATU location inside DBI region
> > > > for unroll too. That's one of the reason why dw_pcie_iatu_detect() works on
> > > > qcom platforms as it tries to get iatu address from DT first and then falls
> > > > back to the default offset if not found.
> > > > 
> > > > So please define an additional DT region for edma.
> > > 
> > > It's obvious that iATU location can vary. I never said it didn't. We
> > > are talking about eDMA here. In accordance with what the DW PCIe hw
> > > manuals say eDMA always resides the same space as the iATU. The space
> > > is enabled by setting the CS2=1 and CDM=1 wires in case of the Native
> > > Controller DBI access. In this case eDMA is defined with the 0x80000
> > > offset over the iATU base address while the iATU base can be placed at
> > > whatever region platform engineer needs.
> > > 
> > > Alternatively the AXI Bridge-based DBI access can be enabled thus
> > > having the DBI+iATU+eDMA mapped over the same MMIO space with
> > > respective offsets 0x0;0x300000;0x380000. This case is handled in the
> > > branch of the conditional statement above if it's found that iATU base
> > > is having the default offset with respect to the DBI base address
> > > (pci->atu_base == pci->dbi_base + DEFAULT_DBI_ATU_OFFSET).
> > > 
> > > To sum up seeing I couldn't find the eDMA region defined in the qcom
> > > bindings and judging by what you say doesn't really contradict to what
> > > is done in my code, I guess there must be some misunderstanding either in
> > > what you see in the code above or what I understand from what you say.
> > > So please be more specific what offsets and whether they are really
> > > different from what I use in the code above.
> > > 
> > 
> > You won't see any edma register offset because no one bothered to define it
> > since it was not used until now. But the memory region should've been
> > documented...
> > 
> 
> > Anyway, here is the offset for the Qcom SoC I'm currently working on:
> > 
> > DBI  - 0x0
> > iATU - 0x1000
> > eDMA - 0x2000
> 
> Finally we've got to something. Earlier you said:
> 
> > > > This is not true. Vendors can customize the iATU location inside DBI region
> > > > for unroll too.
> 
> Actually it is if we are talking about the standard Syopsys DW PCIe
> IP-CoreConsultant methods, which don't imply any eDMA base address
> customization parameter. Thus there must be some address translation
> performed at some layer before the address reaches the DW PCIe DBI
> interface. So it's platform-specific. That happens in your case too.
> 

Seems like it.

> > > > That's one of the reason why dw_pcie_iatu_detect() works on
> > > > qcom platforms as it tries to get iatu address from DT first and then falls
> > > > back to the default offset if not found.
> > 
> > As you can see, these offsets doesn't really fit in both the cases you shared
> > above.
> 
> Seeing the iATU address bits layout can be changed the next
> reg-space calculation code shall work for all the discussed cases:
> 
> if (!unroll) {
> 	pci->edma.reg_base = pci->dbi_base + PCIE_DMA_VIEWPORT_BASE;
> } else if (!pci->edma.reg_base) {
> 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dma");
> 	if (res) {
> 		pci->edma.reg_base = devm_ioremap_resource(dev, res);
> 		if (IS_ERR(pci->edma.reg_base))
> 			return PTR_ERR(pci->edma.reg_base);

This should work for me. Thanks for the work!

Regards,
Mani

> 	} else (pci->atu_size >= 2 * 0x80000) {
> 		pci->edma.reg_base = pci->atu_base + 0x80000;
> 	} else {
> 		/* No standard eDMA CSRs mapping found. Just skip */
> 		return 0;
> 	}
> } else {
> 	/* pci->edma.reg_base can be specified by the platform code. This shall
> 	 * be useful for the tegra194 or intel gw SoCs. The former
> 	 * platform has the "atu_dma" resource declared which implies
> 	 * having the joint iATU+eDMA CSR space, while the later has
> 	 * specific iATU offset with respect to the DBI base address
> 	 * (address is two bits shorter).
> 	 */
> }
> 
> -Sergey
> 
> > 
> > I don't have the knowledge about the internal representation of the IP or what
> > customization Qcom did apart from some high level information.
> > 
> > Hope this clarifies!
> > 
> > Thanks,
> > Mani

Powered by blists - more mailing lists