| Message ID | 20171214135620.13564-1-thierry.reding@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Thursday 14 December 2017 07:26 PM, Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > The PCI host bridge found on Tegra SoCs doesn't require the MSI target > address to be backed by physical system memory. Writes are intercepted > within the controller and never make it to the memory pointed to. > > Since no actual system memory is required, remove the allocation of a > single page and hardcode the MSI target address with a special address > on a per-SoC basis. Ideally this would be an address to an MMIO memory > region (such as where the controller's register are located). However, > those addresses don't work reliably across all Tegra generations. The > only set of addresses that work consistently are those that point to > external memory. > > This is not ideal, since those addresses could technically be used for > DMA and hence be confusing. However, the first page of external memory > is unlikely to be used and special enough to avoid confusion. > > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > Changes in v2: > - use physical base address of system memory as MSI target address > > Vidya, Manikanta, > > I've tested this on all of Tegra20, Tegra30, Tegra124, Tegra210 and > Tegra186, so I'm confident that this is finally a good set of addresses. > However, it'd be good to get confirmation from you since we had > conflicting test results the last time around. > > Thanks, > Thierry > > drivers/pci/host/pci-tegra.c | 41 +++++++++++++++++++++++++++++++++++------ > 1 file changed, 35 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c > index ee193767f77b..f2214ca1ad17 100644 > --- a/drivers/pci/host/pci-tegra.c > +++ b/drivers/pci/host/pci-tegra.c > @@ -236,7 +236,6 @@ 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; > int irq; > @@ -246,6 +245,7 @@ struct tegra_msi { > struct tegra_pcie_soc { > unsigned int num_ports; > unsigned int msi_base_shift; > + u64 msi_target; > u32 pads_pll_ctl; > u32 tx_ref_sel; > u32 pads_refclk_cfg0; > @@ -1576,9 +1576,35 @@ static int tegra_pcie_enable_msi(struct tegra_pcie *pcie) > goto err; > } > > - /* setup AFI/FPCI range */ > - msi->pages = __get_free_pages(GFP_KERNEL, 0); > - msi->phys = virt_to_phys((void *)msi->pages); > + /* > + * The PCI host bridge on Tegra contains some logic that intercepts > + * MSI writes, which means that the MSI target address doesn't have > + * to point to actual physical memory. Rather than allocating one 4 > + * KiB page of system memory that's never used, we can simply pick > + * an arbitrary address within a reserved area in the FPCI address > + * map. > + * > + * Note that MSI writes are never committed to memory, so it doesn't > + * really matter which address we pick. However, since the address > + * may show up at some point and potentially confuse users, the best > + * solution would be to pick an address that is obviously not within > + * system memory. The FPCI address map contains a range that cannot > + * be accessed by any Tegra CPU, but unfortunately that excludes the > + * usage for 32-bit MSI capable devices. > + * > + * A good choice that supports both 32-bit and 64-bit MSI would be to > + * pick a memory address within the PCI MMIO region that is currently > + * not used for configuration space, downstream I/O, prefetchable or > + * non prefetchable memory. However, it seems like not all chips will > + * be able to intercept MSI writes to those addresses. > + * > + * The only reliable addresses that seem to work point to external > + * memory. Pick the physical base address of system memory as the MSI > + * target address. This is slightly less than ideal because it could > + * technically, though unlikely, be used for DMA. However, since the > + * MSI writes will be intercepted, this should be of little concern. > + */ > + msi->phys = soc->msi_target; > > afi_writel(pcie, msi->phys >> soc->msi_base_shift, AFI_MSI_FPCI_BAR_ST); > afi_writel(pcie, msi->phys, AFI_MSI_AXI_BAR_ST); > @@ -1630,8 +1656,6 @@ static int tegra_pcie_disable_msi(struct tegra_pcie *pcie) > afi_writel(pcie, 0, AFI_MSI_EN_VEC6); > afi_writel(pcie, 0, AFI_MSI_EN_VEC7); > > - free_pages(msi->pages, 0); > - > if (msi->irq > 0) > free_irq(msi->irq, pcie); > > @@ -2140,6 +2164,7 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie) > static const struct tegra_pcie_soc tegra20_pcie = { > .num_ports = 2, > .msi_base_shift = 0, > + .msi_target = 0x00000000, > .pads_pll_ctl = PADS_PLL_CTL_TEGRA20, > .tx_ref_sel = PADS_PLL_CTL_TXCLKREF_DIV10, > .pads_refclk_cfg0 = 0xfa5cfa5c, > @@ -2155,6 +2180,7 @@ static const struct tegra_pcie_soc tegra20_pcie = { > static const struct tegra_pcie_soc tegra30_pcie = { > .num_ports = 3, > .msi_base_shift = 8, > + .msi_target = 0x80000000, > .pads_pll_ctl = PADS_PLL_CTL_TEGRA30, > .tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN, > .pads_refclk_cfg0 = 0xfa5cfa5c, > @@ -2171,6 +2197,7 @@ static const struct tegra_pcie_soc tegra30_pcie = { > static const struct tegra_pcie_soc tegra124_pcie = { > .num_ports = 2, > .msi_base_shift = 8, > + .msi_target = 0x80000000, > .pads_pll_ctl = PADS_PLL_CTL_TEGRA30, > .tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN, > .pads_refclk_cfg0 = 0x44ac44ac, > @@ -2186,6 +2213,7 @@ static const struct tegra_pcie_soc tegra124_pcie = { > static const struct tegra_pcie_soc tegra210_pcie = { > .num_ports = 2, > .msi_base_shift = 8, > + .msi_target = 0x80000000, > .pads_pll_ctl = PADS_PLL_CTL_TEGRA30, > .tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN, > .pads_refclk_cfg0 = 0x90b890b8, > @@ -2201,6 +2229,7 @@ static const struct tegra_pcie_soc tegra210_pcie = { > static const struct tegra_pcie_soc tegra186_pcie = { > .num_ports = 3, > .msi_base_shift = 8, > + .msi_target = 0x80000000, When SMMU is enabled for PCIe, I think the IOVA region used is 0x8000_0000 ~ 0xFFF0_0000 Wouldn't that be an issue then? > .pads_pll_ctl = PADS_PLL_CTL_TEGRA30, > .tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN, > .pads_refclk_cfg0 = 0x80b880b8,
diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c index ee193767f77b..f2214ca1ad17 100644 --- a/drivers/pci/host/pci-tegra.c +++ b/drivers/pci/host/pci-tegra.c @@ -236,7 +236,6 @@ 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; int irq; @@ -246,6 +245,7 @@ struct tegra_msi { struct tegra_pcie_soc { unsigned int num_ports; unsigned int msi_base_shift; + u64 msi_target; u32 pads_pll_ctl; u32 tx_ref_sel; u32 pads_refclk_cfg0; @@ -1576,9 +1576,35 @@ static int tegra_pcie_enable_msi(struct tegra_pcie *pcie) goto err; } - /* setup AFI/FPCI range */ - msi->pages = __get_free_pages(GFP_KERNEL, 0); - msi->phys = virt_to_phys((void *)msi->pages); + /* + * The PCI host bridge on Tegra contains some logic that intercepts + * MSI writes, which means that the MSI target address doesn't have + * to point to actual physical memory. Rather than allocating one 4 + * KiB page of system memory that's never used, we can simply pick + * an arbitrary address within a reserved area in the FPCI address + * map. + * + * Note that MSI writes are never committed to memory, so it doesn't + * really matter which address we pick. However, since the address + * may show up at some point and potentially confuse users, the best + * solution would be to pick an address that is obviously not within + * system memory. The FPCI address map contains a range that cannot + * be accessed by any Tegra CPU, but unfortunately that excludes the + * usage for 32-bit MSI capable devices. + * + * A good choice that supports both 32-bit and 64-bit MSI would be to + * pick a memory address within the PCI MMIO region that is currently + * not used for configuration space, downstream I/O, prefetchable or + * non prefetchable memory. However, it seems like not all chips will + * be able to intercept MSI writes to those addresses. + * + * The only reliable addresses that seem to work point to external + * memory. Pick the physical base address of system memory as the MSI + * target address. This is slightly less than ideal because it could + * technically, though unlikely, be used for DMA. However, since the + * MSI writes will be intercepted, this should be of little concern. + */ + msi->phys = soc->msi_target; afi_writel(pcie, msi->phys >> soc->msi_base_shift, AFI_MSI_FPCI_BAR_ST); afi_writel(pcie, msi->phys, AFI_MSI_AXI_BAR_ST); @@ -1630,8 +1656,6 @@ static int tegra_pcie_disable_msi(struct tegra_pcie *pcie) afi_writel(pcie, 0, AFI_MSI_EN_VEC6); afi_writel(pcie, 0, AFI_MSI_EN_VEC7); - free_pages(msi->pages, 0); - if (msi->irq > 0) free_irq(msi->irq, pcie); @@ -2140,6 +2164,7 @@ static void tegra_pcie_enable_ports(struct tegra_pcie *pcie) static const struct tegra_pcie_soc tegra20_pcie = { .num_ports = 2, .msi_base_shift = 0, + .msi_target = 0x00000000, .pads_pll_ctl = PADS_PLL_CTL_TEGRA20, .tx_ref_sel = PADS_PLL_CTL_TXCLKREF_DIV10, .pads_refclk_cfg0 = 0xfa5cfa5c, @@ -2155,6 +2180,7 @@ static const struct tegra_pcie_soc tegra20_pcie = { static const struct tegra_pcie_soc tegra30_pcie = { .num_ports = 3, .msi_base_shift = 8, + .msi_target = 0x80000000, .pads_pll_ctl = PADS_PLL_CTL_TEGRA30, .tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN, .pads_refclk_cfg0 = 0xfa5cfa5c, @@ -2171,6 +2197,7 @@ static const struct tegra_pcie_soc tegra30_pcie = { static const struct tegra_pcie_soc tegra124_pcie = { .num_ports = 2, .msi_base_shift = 8, + .msi_target = 0x80000000, .pads_pll_ctl = PADS_PLL_CTL_TEGRA30, .tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN, .pads_refclk_cfg0 = 0x44ac44ac, @@ -2186,6 +2213,7 @@ static const struct tegra_pcie_soc tegra124_pcie = { static const struct tegra_pcie_soc tegra210_pcie = { .num_ports = 2, .msi_base_shift = 8, + .msi_target = 0x80000000, .pads_pll_ctl = PADS_PLL_CTL_TEGRA30, .tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN, .pads_refclk_cfg0 = 0x90b890b8, @@ -2201,6 +2229,7 @@ static const struct tegra_pcie_soc tegra210_pcie = { static const struct tegra_pcie_soc tegra186_pcie = { .num_ports = 3, .msi_base_shift = 8, + .msi_target = 0x80000000, .pads_pll_ctl = PADS_PLL_CTL_TEGRA30, .tx_ref_sel = PADS_PLL_CTL_TXCLKREF_BUF_EN, .pads_refclk_cfg0 = 0x80b880b8,