[U-Boot] [PATCH 15/23] pci: tegra: Add Tegra PCIe driver

[Thierry Reding <thierry.reding@gmail.com>]

On Wed, Aug 20, 2014 at 01:04:22PM -0600, Stephen Warren wrote:
> On 08/18/2014 01:16 AM, Thierry Reding wrote:
> >From: Thierry Reding <treding@nvidia.com>
> >
> >Add support for the PCIe controller found on some generations of Tegra.
> >Tegra20 has 2 root ports with a total of 4 lanes, Tegra30 has 3 root
> >ports with a total of 6 lanes and Tegra124 has 2 root ports with a total
> >of 5 lanes.
> 
> >diff --git a/drivers/pci/pci_tegra.c b/drivers/pci/pci_tegra.c
> 
> >+ * Copyright (c) 2010, CompuLab, Ltd.
> >+ * Author: Mike Rapoport <mike@compulab.co.il>
> >+ *
> >+ * Based on NVIDIA PCIe driver
> >+ * Copyright (c) 2008-2009, NVIDIA Corporation.
> 
> Presumably also at least (c) 2013-2014 NVIDIA too?

Yes.

> Do you have Mike's S-o-b for the initial work?

Yes, I can take it from the kernel's git log.

> >+static int tegra_pcie_read_conf(struct pci_controller *hose, pci_dev_t bdf,
> >+				int where, u32 *value)
> >+{
> >+	struct tegra_pcie *pcie = to_tegra_pcie(hose);
> >+	unsigned long address;
> >+	int err;
> >+
> >+	err = tegra_pcie_conf_address(pcie, bdf, where, &address);
> >+	if (err < 0) {
> >+		*value = 0xffffffff;
> >+		return 1;
> >+	}
> 
> In the kernel, don't we have to play games to dynamically map/unmap memory
> regions and/or map pages in a non-linear fashion since the config space is
> so large, and bus/device ID -> address mapping is odd? I don't see that
> here.

We do that in the kernel to avoid having to I/O remap a full 256 MiB of
memory even if only a single PCI bus is needed. Since memory is mapped
1:1 in U-Boot and we access physical memory "directly", we don't have to
worry about being wasteful. The tegra_pcie_conf_offset() function
computes the proper offset:

	static unsigned long tegra_pcie_conf_offset(pci_dev_t bdf, int where)
	{
		return ((where & 0xf00) << 16) | (PCI_BUS(bdf) << 16) |
		       (PCI_DEV(bdf) << 11) | (PCI_FUNC(bdf) << 8) |
		       (where & 0xfc);
	}

> >+static bool tegra_pcie_port_check_link(struct tegra_pcie_port *port)
> >+{
> >+	unsigned int retries = 3;
> >+	unsigned long value;
> >+
> >+	value = rp_readl(port, RP_PRIV_MISC);
> >+	value &= ~RP_PRIV_MISC_PRSNT_MAP_EP_ABSNT;
> >+	value |= RP_PRIV_MISC_PRSNT_MAP_EP_PRSNT;
> >+	rp_writel(port, value, RP_PRIV_MISC);
> >+
> >+	do {
> >+		unsigned int timeout = 200;
> >+
> >+		do {
> >+			value = rp_readl(port, RP_VEND_XP);
> >+			if (value & RP_VEND_XP_DL_UP)
> >+				break;
> 
> On my board, this almost never succeeds for Jetson TK1's built-in NIC,
> although it works fine for the mini-PCIe slot. On the other two boards I
> borrowed, this succeeds almost always. I'd prefer if we could get to the
> bottom of this before we actually apply this, although the bug is proving
> quite perplexing. The mainline kernel PCIe patches have the same issue.

That's indeed odd. It's possible that the powergate fix that I've
applied (to wait for the partition to become powered) may improve
things. I'll be pushing out my latest work shortly (I don't think
I'll manage to send out the whole series for review, though), so
in case you get the chance you may want to test it.

> >+static int process_nodes(const void *fdt, int nodes[], unsigned int count)
> 
> >+#ifdef CONFIG_PCI_SCAN_SHOW
> >+		printf("PCI: Enumerating devices...\n");
> >+		printf("---------------------------------------\n");
> >+		printf("  Device        ID          Description\n");
> >+		printf("  ------        --          -----------\n");
> >+#endif
> >+
> >+		pcie->hose.last_busno = pci_hose_scan(&pcie->hose);
> 
> Shouldn't that ifdef'd block be part of pci_hose_scan()?

Every driver does it this way, but I agree that it should presumably go
in the core somewhere. I'll see if I can find a good place to do it.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140822/c0069960/attachment.pgp>