Date:   Tue, 19 Apr 2022 23:54:03 +0300
From:   Serge Semin <fancer.lancer@...il.com>
To:     Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
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 Mon, Mar 28, 2022 at 07:45:21PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Mar 24, 2022 at 04:48:36AM +0300, Serge Semin wrote:
> > Since the DW eDMA driver now supports eDMA controllers embedded into the
> > locally accessible DW PCIe Root Ports and End-points, we can use the
> > updated interface to register DW eDMA as DMA engine device if it's
> > available. In order to successfully do that the DW PCIe core driver need
> > to perform some preparations first. First of all it needs to find out the
> > eDMA controller CSRs base address, whether they are accessible over the
> > Port Logic or iATU unrolled space. Afterwards it can try to auto-detect
> > the eDMA controller availability and number of it's read/write channels.
> > If none was found the procedure will just silently halt with no error
> > returned. Secondly the platform is supposed to provide either combined or
> > per-channel IRQ signals. If no valid IRQs set is found the procedure will
> > also halt with no error returned so to be backward compatible with
> > platforms where DW PCIe controllers have eDMA embedded but lack of the
> > IRQs defined for them. Finally before actually probing the eDMA device we
> > need to allocate LLP items buffers. After that the DW eDMA can be
> > registered. If registration is successful the info-message regarding the
> > number of detected Read/Write eDMA channels will be printed to the system
> > log in the same way as it's done for iATU settings.
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@...kalelectronics.ru>
> > ---
> >  .../pci/controller/dwc/pcie-designware-ep.c   |   4 +
> >  .../pci/controller/dwc/pcie-designware-host.c |  13 +-
> >  drivers/pci/controller/dwc/pcie-designware.c  | 188 ++++++++++++++++++
> >  drivers/pci/controller/dwc/pcie-designware.h  |  23 ++-
> >  4 files changed, 225 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index 23401f17e8f0..b2840d1a5b9a 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -712,6 +712,10 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> >  
> >  	dw_pcie_iatu_detect(pci);
> >  
> > +	ret = dw_pcie_edma_detect(pci);
> > +	if (ret)
> > +		return ret;
> > +
> >  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
> >  	if (!res)
> >  		return -EINVAL;
> 

> eDMA needs to be removed on error path

So does the EPC memory... The dw_pcie_ep_init() and the code using it
are broken in the cleanup part. Neither the platform drivers nor the
method itself de-allocate the epc memory on any further error. See,
the dw_pcie_ep_exit() function isn't called by any glue driver, while
some of them do have the device remove method implemented. I
am not going to fix the platform drivers (though the devm_add_action()
method could be used for that in a least painful fix) due to lacking
of an EP device to test it out. But since you are asking to revert
the eDMA initialization in case of the EP init failure I'll add the
cleanup-on-error path to the dw_pcie_ep_init() method. But it will be
done in v2 of the "PCI: dwc: Various fixes and cleanups" series.

> 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index 715a13b90e43..048b452ee4f3 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -405,14 +405,18 @@ int dw_pcie_host_init(struct pcie_port *pp)
> >  
> >  	dw_pcie_iatu_detect(pci);
> >  
> > -	ret = dw_pcie_setup_rc(pp);
> > +	ret = dw_pcie_edma_detect(pci);
> >  	if (ret)
> >  		goto err_free_msi;
> >  
> > +	ret = dw_pcie_setup_rc(pp);
> > +	if (ret)
> > +		goto err_edma_remove;
> > +
> >  	if (!dw_pcie_link_up(pci) && pci->ops && pci->ops->start_link) {
> >  		ret = pci->ops->start_link(pci);
> >  		if (ret)
> > -			goto err_free_msi;
> > +			goto err_edma_remove;
> >  	}
> >  
> >  	/* Ignore errors, the link may come up later */
> > @@ -430,6 +434,9 @@ int dw_pcie_host_init(struct pcie_port *pp)
> >  	if (pci->ops && pci->ops->stop_link)
> >  		pci->ops->stop_link(pci);
> >  
> > +err_edma_remove:
> > +	dw_pcie_edma_remove(pci);
> > +
> >  err_free_msi:
> >  	if (pp->has_msi_ctrl)
> >  		dw_pcie_free_msi(pp);
> > @@ -452,6 +459,8 @@ void dw_pcie_host_deinit(struct pcie_port *pp)
> >  	if (pci->ops && pci->ops->stop_link)
> >  		pci->ops->stop_link(pci);
> >  
> > +	dw_pcie_edma_remove(pci);
> > +
> >  	if (pp->has_msi_ctrl)
> >  		dw_pcie_free_msi(pp);
> >  
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index 4a95a7b112e9..dbe39a7ecb71 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> 
> [...]
> 
> > +int dw_pcie_edma_detect(struct dw_pcie *pci)
> > +{
> > +	int ret;
> > +
> > +	pci->edma.dev = pci->dev;
> > +	if (!pci->edma.ops)
> > +		pci->edma.ops = &dw_pcie_edma_ops;
> > +	pci->edma.flags |= DW_EDMA_CHIP_LOCAL;
> > +

> > +	pci->edma_unroll_enabled = dw_pcie_edma_unroll_enabled(pci);
> 
> Is is possible to continue the unroll path for eDMA if iATU unroll is enabled?

Don't get it. Could you elaborate your question in more details?
Are you talking about using the same flag for both eDMA and iATU
unrolled space? If so then most likely yes. But in that case the
iatu_unroll_enabled flag name and semantics need to be changed.

> 
> > +	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.

> 
> > +	} else {
> > +		pci->edma.mf = EDMA_MF_EDMA_LEGACY;
> > +		pci->edma.reg_base = pci->dbi_base + PCIE_DMA_VIEWPORT_BASE;
> > +	}
> > +
> > +	ret = dw_pcie_edma_detect_channels(pci);
> > +	if (ret) {
> > +		dev_err(pci->dev, "Unexpected NoF eDMA channels found\n");
> > +		return ret;
> > +	}
> > +
> > +	/* Skip any further initialization if no eDMA found */
> 

> Should we introduce a new Kconfig option for enabling eDMA? My concern here is,
> if eDMA is really needed for an usecase and if the platform support is broken
> somehow (DT issues?), then we'll just simply go ahead without probe failure and
> it may break somewhere else.
> 
> And we are returning errors if something wrong happens during eDMA probe. This
> might annoy the existing users who don't care about eDMA but turning those
> errors to debug will affect the real users of eDMA.
> 
> For these reasons, I think it'd be better to probe eDMA only if the Kconfig
> option is enabled (which would be disabled by default). And properly return the
> failure.

I don't see a need in introducing of a new parametrization. Neither
there is a point in dropping the eDMA support on all the platforms for
the sake of some hypothetically malfunction hardware. Regarding the
config, the DW eDMA driver already has one. It's CONFIG_DW_EDMA which
can be used for what you say. Though I need to fix this patch a bit so
the -ENODEV errno returned from the dw_edma_probe() method would be
ignored in the dw_pcie_edma_detect() procedure to support the case of
the disabled DW eDMA driver.

-Sergey

> 
> Thanks,
> Mani

Powered by blists - more mailing lists