| Message ID | 20220324014836.19149-26-Sergey.Semin@baikalelectronics.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dmaengine: dw-edma: Add RP/EP local DMA controllers support | expand |
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@baikalelectronics.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
On Tue, Apr 19, 2022 at 11:54:03PM +0300, Serge Semin wrote: > 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@baikalelectronics.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, Right. > 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. > That's fine, thanks. > > > > > 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 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. > > > > > + 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. > > > > > + } 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. I'm not talking about "hypothetically malfunction hardware" but real customized ones like all Qcom platforms supporting PCIe. As I said in my previous comment, the default eDMA offset won't work on Qcom platforms. So if this driver tries to access the registers based on the default offset, it may result in SMMU errors. The edma region needs to be defined in DT for probing edma correctly. But even if we add dt support for edma and probe edma unconditionally, we'd be breaking the dts compatibility with older ones that don't define it. > 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. > Please do so. Thanks, Mani > -Sergey > > > > > Thanks, > > Mani
On Sat, Apr 23, 2022 at 08:10:55PM +0530, Manivannan Sadhasivam wrote: > On Tue, Apr 19, 2022 at 11:54:03PM +0300, Serge Semin wrote: > > 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@baikalelectronics.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.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 [...] > > > 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. > > I'm not talking about "hypothetically malfunction hardware" but real customized > ones like all Qcom platforms supporting PCIe. Correction: It is not "all Qcom platforms" but some like SM8250, SM8450 etc... Thanks, Mani
On Sat, Apr 23, 2022 at 08:10:55PM +0530, Manivannan Sadhasivam wrote: > On Tue, Apr 19, 2022 at 11:54:03PM +0300, Serge Semin wrote: > > 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@baikalelectronics.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, > > Right. > > > 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. > > > > That's fine, thanks. > > > > > > > > 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 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. > > > > > > > > + 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. What I can suggest to modify in my solution is: 1) Make sure that the iATU/eDMA space fits the range [atu_base; atu_base + atu_size]. If it doesn't just return an error. 2) Just realised that DEFAULT_DBI_DMA_OFFSET = DEFAULT_DBI_ATU_OFFSET + PCIE_DMA_UNROLL_BASE, so my conditional statement in the subject can be dropped thus having the eDMA CSRs base address calculated as: pci->edma.reg_base = pci->atu_base + PCIE_DMA_UNROLL_BASE; -Sergey > > > > > > > > + } 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. > > I'm not talking about "hypothetically malfunction hardware" but real customized > ones like all Qcom platforms supporting PCIe. As I said in my previous comment, > the default eDMA offset won't work on Qcom platforms. So if this driver tries > to access the registers based on the default offset, it may result in SMMU > errors. The edma region needs to be defined in DT for probing edma correctly. > > But even if we add dt support for edma and probe edma unconditionally, we'd be > breaking the dts compatibility with older ones that don't define it. > > > 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. > > > > Please do so. > > Thanks, > Mani > > > -Sergey > > > > > > > > Thanks, > > > Mani
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 As you can see, these offsets doesn't really fit in both the cases you shared above. 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
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. > > > 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); } 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
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
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; 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 @@ -11,6 +11,7 @@ #include <linux/align.h> #include <linux/bitops.h> #include <linux/delay.h> +#include <linux/dma/edma.h> #include <linux/of.h> #include <linux/of_platform.h> #include <linux/sizes.h> @@ -680,6 +681,193 @@ void dw_pcie_iatu_detect(struct dw_pcie *pci) pci->region_align / SZ_1K, (pci->region_limit + 1) / SZ_1G); } +static u32 dw_pcie_readl_dma(struct dw_pcie *pci, u32 reg) +{ + u32 val = 0; + int ret; + + if (pci->ops && pci->ops->read_dbi) + return pci->ops->read_dbi(pci, pci->edma.reg_base, reg, 4); + + ret = dw_pcie_read(pci->edma.reg_base + reg, 4, &val); + if (ret) + dev_err(pci->dev, "Read DMA address failed\n"); + + return val; +} + +static bool dw_pcie_edma_unroll_enabled(struct dw_pcie *pci) +{ + u32 val; + + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL); + if (val == 0xffffffff) + return true; + + return false; +} + +static int dw_pcie_edma_irq_vector(struct device *dev, unsigned int nr) +{ + struct platform_device *pdev = to_platform_device(dev); + char name[6]; + int ret; + + if (nr >= EDMA_MAX_WR_CH + EDMA_MAX_RD_CH) + return -EINVAL; + + ret = platform_get_irq_byname_optional(pdev, "dma"); + if (ret > 0) + return ret; + + snprintf(name, sizeof(name), "dma%u", nr); + + return platform_get_irq_byname_optional(pdev, name); +} + +static struct dw_edma_core_ops dw_pcie_edma_ops = { + .irq_vector = dw_pcie_edma_irq_vector, +}; + +static int dw_pcie_edma_detect_channels(struct dw_pcie *pci) +{ + u32 val; + + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL); + if (!val || val == 0xffffffff) + return 0; + + pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val); + pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val); + + if (pci->edma.ll_wr_cnt > EDMA_MAX_WR_CH || + pci->edma.ll_rd_cnt > EDMA_MAX_RD_CH) + return -EINVAL; + + return 0; +} + +static int dw_pcie_edma_irq_verify(struct dw_pcie *pci) +{ + struct platform_device *pdev = to_platform_device(pci->dev); + u16 ch_cnt = pci->edma.ll_wr_cnt + pci->edma.ll_rd_cnt; + char name[6]; + int ret; + + if (pci->edma.nr_irqs == 1) + return 0; + else if (pci->edma.nr_irqs > 1) + return pci->edma.nr_irqs != ch_cnt ? -EINVAL : 0; + + ret = platform_get_irq_byname_optional(pdev, "dma"); + if (ret > 0) { + pci->edma.nr_irqs = 1; + return 0; + } + + for (; pci->edma.nr_irqs < ch_cnt; pci->edma.nr_irqs++) { + snprintf(name, sizeof(name), "dma%d", pci->edma.nr_irqs); + + ret = platform_get_irq_byname_optional(pdev, name); + if (ret <= 0) + return -EINVAL; + } + + return 0; +} + +static int dw_pcie_edma_ll_alloc(struct dw_pcie *pci) +{ + struct dw_edma_region *ll; + dma_addr_t paddr; + int i; + + for (i = 0; i < pci->edma.ll_wr_cnt; i++) { + ll = &pci->edma.ll_region_wr[i]; + ll->sz = DMA_LLP_MEM_SIZE; + ll->vaddr = dmam_alloc_coherent(pci->dev, ll->sz, + &paddr, GFP_KERNEL); + if (!ll->vaddr) + return -ENOMEM; + + ll->paddr = paddr; + } + + for (i = 0; i < pci->edma.ll_rd_cnt; i++) { + ll = &pci->edma.ll_region_rd[i]; + ll->sz = DMA_LLP_MEM_SIZE; + ll->vaddr = dmam_alloc_coherent(pci->dev, ll->sz, + &paddr, GFP_KERNEL); + if (!ll->vaddr) + return -ENOMEM; + + ll->paddr = paddr; + } + + return 0; +} + +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); + 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; + } 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 */ + if (!pci->edma.ll_wr_cnt && !pci->edma.ll_rd_cnt) + return 0; + + /* Don't return failure here for the backward compatibility */ + ret = dw_pcie_edma_irq_verify(pci); + if (ret) { + dev_err(pci->dev, "No valid eDMA IRQs set found\n"); + return 0; + } + + ret = dw_pcie_edma_ll_alloc(pci); + if (ret) { + dev_err(pci->dev, "Couldn't allocate LLP memory\n"); + return ret; + } + + ret = dw_edma_probe(&pci->edma); + if (ret) { + dev_err(pci->dev, "Couldn't register eDMA device\n"); + return ret; + } + + dev_info(pci->dev, "eDMA channels: %hu wr, %hu rd\n", + pci->edma.ll_wr_cnt, pci->edma.ll_rd_cnt); + + return 0; +} + +void dw_pcie_edma_remove(struct dw_pcie *pci) +{ + dw_edma_remove(&pci->edma); +} + void dw_pcie_setup(struct dw_pcie *pci) { u32 val; diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h index 11c52d2eaf79..9b92f79664f2 100644 --- a/drivers/pci/controller/dwc/pcie-designware.h +++ b/drivers/pci/controller/dwc/pcie-designware.h @@ -13,6 +13,7 @@ #include <linux/bitfield.h> #include <linux/dma-mapping.h> +#include <linux/dma/edma.h> #include <linux/irq.h> #include <linux/msi.h> #include <linux/pci.h> @@ -144,6 +145,18 @@ #define PCIE_MSIX_DOORBELL 0x948 #define PCIE_MSIX_DOORBELL_PF_SHIFT 24 +/* + * eDMA CSRs. DW PCIe IP-core v4.70a and older had the eDMA registers accessible + * over the Port Logic registers space. Afterwords the unrolled mapping was + * introduced so eDMA and iATU could be accessed via a dedicated registers + * space. + */ +#define PCIE_DMA_VIEWPORT_BASE 0x970 +#define PCIE_DMA_UNROLL_BASE 0x80000 +#define PCIE_DMA_CTRL 0x008 +#define PCIE_DMA_NUM_WR_CHAN GENMASK(3, 0) +#define PCIE_DMA_NUM_RD_CHAN GENMASK(19, 16) + #define PCIE_PL_CHK_REG_CONTROL_STATUS 0xB20 #define PCIE_PL_CHK_REG_CHK_REG_START BIT(0) #define PCIE_PL_CHK_REG_CHK_REG_CONTINUOUS BIT(1) @@ -160,6 +173,7 @@ * this offset, if atu_base not set. */ #define DEFAULT_DBI_ATU_OFFSET (0x3 << 20) +#define DEFAULT_DBI_DMA_OFFSET (0x7 << 19) #define MAX_MSI_IRQS 256 #define MAX_MSI_IRQS_PER_CTRL 32 @@ -171,6 +185,9 @@ #define MAX_IATU_IN 256 #define MAX_IATU_OUT 256 +/* Default eDMA LLP memory size */ +#define DMA_LLP_MEM_SIZE PAGE_SIZE + struct pcie_port; struct dw_pcie; struct dw_pcie_ep; @@ -295,7 +312,7 @@ struct dw_pcie { struct device *dev; void __iomem *dbi_base; void __iomem *dbi_base2; - /* Used when iatu_unroll_enabled is true */ + /* Used when {iatu,edma}_unroll_enabled is true */ void __iomem *atu_base; size_t atu_size; u32 num_ib_windows; @@ -310,7 +327,9 @@ struct dw_pcie { int num_lanes; int link_gen; u8 n_fts[2]; + struct dw_edma_chip edma; bool iatu_unroll_enabled: 1; + bool edma_unroll_enabled: 1; bool io_cfg_atu_shared: 1; }; @@ -344,6 +363,8 @@ int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index, void dw_pcie_disable_atu(struct dw_pcie *pci, u32 dir, int index); void dw_pcie_setup(struct dw_pcie *pci); void dw_pcie_iatu_detect(struct dw_pcie *pci); +int dw_pcie_edma_detect(struct dw_pcie *pci); +void dw_pcie_edma_remove(struct dw_pcie *pci); static inline void dw_pcie_writel_dbi(struct dw_pcie *pci, u32 reg, u32 val) {
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@baikalelectronics.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(-)