Re: [PATCH V2] PCI: tegra: Use the DMA-API to get the MSI address

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 11/04/2019 10:40, Lorenzo Pieralisi wrote:
[+Robin]

I would like Robin to have a look before merging this patch so
that we agree that's the expected approach.

It's a bit crazy, but I guess it's not really any worse than the existing implementation. As long the comments and commit message clearly document that this is just trickery to reserve a 32-bit DMA address (which AFAICS they more or less do) I don't think I have a significant objection.

One suggestion I would make is using dma_alloc_attrs() with DMA_ATTR_NO_KERNEL_MAPPING, to make it clear that this is being set aside for 'special' device purposes and is not intended to be accessed as a regular buffer (plus I believe this is non-coherent, so that should also skip the small overhead of creating a non-cacheable remap).

Ultimately, it might make sense to have an API in the PCI core which can look at memblock/bridge windows/etc. to find a suitable non-overlapping address for this kind of root-complex-integrated doorbell without having to subvert the page allocator, as it seems to be a fairly common setup in 'embedded' IPs.

Robin.


Lorenzo

On Tue, Mar 19, 2019 at 07:32:01PM +0530, Vidya Sagar wrote:
Since the upstream MSI memory writes are generated by downstream devices,
it is logically correct to have MSI target memory coming from the DMA pool
reserved for PCIe than from the general memory pool reserved for CPU
access. This avoids PCIe DMA addresses coinciding with MSI target address
thereby raising unwanted MSI interrupts. This patch also enforces to limit
the MSI target address to 32-bits to make it work for PCIe endponits that
support only 32-bit MSI target address and those endpoints that support
64-bit MSI target address anyway work with 32-bit MSI target address.

Signed-off-by: Vidya Sagar <vidyas@xxxxxxxxxx>
Reviewed-by: Thierry Reding <treding@xxxxxxxxxx>
Acked-by: Thierry Reding <treding@xxxxxxxxxx>
---
v2:
* changed 'phys' type to 'dma_addr_t' from 'u64'
* added a comment on why DMA mask is set to 32-bit
* replaced 'dma' with 'DMA'

  drivers/pci/controller/pci-tegra.c | 35 ++++++++++++++++++++++++++---------
  1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
index f4f53d092e00..f8173a5e352d 100644
--- a/drivers/pci/controller/pci-tegra.c
+++ b/drivers/pci/controller/pci-tegra.c
@@ -231,9 +231,9 @@ struct tegra_msi {
  	struct msi_controller chip;
  	DECLARE_BITMAP(used, INT_PCI_MSI_NR);
  	struct irq_domain *domain;
-	unsigned long pages;
  	struct mutex lock;
-	u64 phys;
+	void *virt;
+	dma_addr_t phys;
  	int irq;
  };
@@ -1536,7 +1536,7 @@ static int tegra_pcie_msi_setup(struct tegra_pcie *pcie)
  	err = platform_get_irq_byname(pdev, "msi");
  	if (err < 0) {
  		dev_err(dev, "failed to get IRQ: %d\n", err);
-		goto err;
+		goto free_irq_domain;
  	}
msi->irq = err;
@@ -1545,17 +1545,34 @@ static int tegra_pcie_msi_setup(struct tegra_pcie *pcie)
  			  tegra_msi_irq_chip.name, pcie);
  	if (err < 0) {
  		dev_err(dev, "failed to request IRQ: %d\n", err);
-		goto err;
+		goto free_irq_domain;
+	}
+
+	/* Though the PCIe controller can address >32-bit address space, to
+	 * facilitate endpoints that support only 32-bit MSI target address,
+	 * the mask is set to 32-bit to make sure that MSI target address is
+	 * always a 32-bit address
+	 */
+	err = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
+	if (err < 0) {
+		dev_err(dev, "failed to set DMA coherent mask: %d\n", err);
+		goto free_irq;
+	}
+
+	msi->virt = dma_alloc_coherent(dev, PAGE_SIZE, &msi->phys, GFP_KERNEL);
+	if (!msi->virt) {
+		dev_err(dev, "failed to allocate DMA memory for MSI\n");
+		err = -ENOMEM;
+		goto free_irq;
  	}
- /* setup AFI/FPCI range */
-	msi->pages = __get_free_pages(GFP_KERNEL, 0);
-	msi->phys = virt_to_phys((void *)msi->pages);
  	host->msi = &msi->chip;
return 0; -err:
+free_irq:
+	free_irq(msi->irq, pcie);
+free_irq_domain:
  	irq_domain_remove(msi->domain);
  	return err;
  }
@@ -1592,7 +1609,7 @@ static void tegra_pcie_msi_teardown(struct tegra_pcie *pcie)
  	struct tegra_msi *msi = &pcie->msi;
  	unsigned int i, irq;
- free_pages(msi->pages, 0);
+	dma_free_coherent(pcie->dev, PAGE_SIZE, msi->virt, msi->phys);
if (msi->irq > 0)
  		free_irq(msi->irq, pcie);
--
2.7.4




[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux