Re: [U-Boot] PCIe bridges on Jetson TK1

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3476 bytes --]

On Mon, Jul 18, 2016 at 08:27:18AM +0200, Thierry Reding wrote:
> On Sun, Jul 17, 2016 at 02:27:25AM +0200, Andreas Färber wrote:
> > Hi,
> > 
> > When I boot my Jetson TK1, by default I get this from lspci:
> > 
> > 00:02.0 PCI bridge: NVIDIA Corporation TegraK1 PCIe x1 Bridge (rev a1)
> > 01:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd.
> > RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller (rev 0c)
> > 
> > If however I plug some mini PCIe card, I get this instead:
> > 
> > 00:01.0 PCI bridge: NVIDIA Corporation TegraK1 PCIe x4 Bridge (rev a1)
> > 00:02.0 PCI bridge: NVIDIA Corporation TegraK1 PCIe x1 Bridge (rev a1)
> > 01:00.0 Network controller: MEDIATEK Corp. Device 7612
> > 02:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd.
> > RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller (rev 0c)
> > 
> > I.e., there is a new Tegra PCIe x4 bridge and the number of the previous
> > x1 bridge changed.
> > 
> > That is ugly because it changes the ID of the on-board PCI NIC from
> > 01:00.0 to 02:00.0, which on openSUSE renames the network interface from
> > enp1s0 to enp2s0, so that my /etc/sysconfig/network/ifcfg-enp1s0 does
> > not take effect and the network interface doesn't come up.
> > 
> > Tested with U-Boot v2016.05 and v2016.07 and kernel 4.6.2 and 4.7-rc6.
> > 
> > Shouldn't U-Boot or the kernel driver always configure the PCIe ports
> > the same way (both bridges available) since the slot is always there on
> > this board?
> 
> I don't think that's going to ensure stable naming of devices. Linux
> uses depth-first sorting when enumerating devices, so if you attach any
> kind of bridge device to the first port, anything downstream of the
> second port still won't get a stable B/D/F.
> 
> That said, I see how what you're proposing could help at least minimize
> the potential for instability in numbering. Could you try to uncomment
> the tegra_pcie_port_free() line in tegra_pcie_enable() of the Tegra PCI
> host controller driver (drivers/pci/host/pci-tegra.c) and see if that
> improves things in your use-case? It's slightly hackish because it does
> allow access to the root port even if it's disabled, so I'm not sure it
> will work (might give you an external abort or something like that) but
> it might be worth a quick try.

The below seems to work fine for me. Can you verify that this works for
you, too?

Thanks,
Thierry

--- >8 ---
diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index c388468c202a..0b8616e25d7b 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -587,17 +587,6 @@ static void tegra_pcie_port_disable(struct tegra_pcie_port *port)
 	afi_writel(port->pcie, value, ctrl);
 }
 
-static void tegra_pcie_port_free(struct tegra_pcie_port *port)
-{
-	struct tegra_pcie *pcie = port->pcie;
-
-	devm_iounmap(pcie->dev, port->base);
-	devm_release_mem_region(pcie->dev, port->regs.start,
-				resource_size(&port->regs));
-	list_del(&port->list);
-	devm_kfree(pcie->dev, port);
-}
-
 /* Tegra PCIE root complex wrongly reports device class */
 static void tegra_pcie_fixup_class(struct pci_dev *dev)
 {
@@ -2053,7 +2042,6 @@ static int tegra_pcie_enable(struct tegra_pcie *pcie)
 		dev_info(pcie->dev, "link %u down, ignoring\n", port->index);
 
 		tegra_pcie_port_disable(port);
-		tegra_pcie_port_free(port);
 	}
 
 	memset(&hw, 0, sizeof(hw));

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]