by Manivannan Sadhasivam

[permalink] [raw]

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