Message-ID: <20130118095620.GA7552@arm.com>
Date:	Fri, 18 Jan 2013 09:56:20 +0000
From:	Andrew Murray <andrew.murray@....com>
To:	Thierry Reding <thierry.reding@...onic-design.de>
Cc:	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
	Grant Likely <grant.likely@...retlab.ca>,
	"rob.herring@...xeda.com" <rob.herring@...xeda.com>,
	Russell King <linux@....linux.org.uk>,
	Stephen Warren <swarren@...dotorg.org>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Jason Gunthorpe <jgunthorpe@...idianresearch.com>,
	Arnd Bergmann <arnd@...db.de>,
	Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
	"devicetree-discuss@...ts.ozlabs.org" 
	<devicetree-discuss@...ts.ozlabs.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>
Subject: Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host

On Wed, Jan 09, 2013 at 08:43:10PM +0000, Thierry Reding wrote:
> Move the PCIe driver from arch/arm/mach-tegra into the drivers/pci/host
> directory. The motivation is to collect various host controller drivers
> in the same location in order to facilitate refactoring.
> 
> The Tegra PCIe driver has been largely rewritten, both in order to turn
> it into a proper platform driver and to add MSI (based on code by
> Krishna Kishore <kthota@...dia.com>) as well as device tree support.
> 
> Signed-off-by: Thierry Reding <thierry.reding@...onic-design.de>

[snip]

> +static int tegra_pcie_enable(struct tegra_pcie *pcie)
> +{
> +       struct hw_pci hw;
> +
> +       memset(&hw, 0, sizeof(hw));
> +
> +       hw.nr_controllers = 1;
> +       hw.private_data = (void **)&pcie;
> +       hw.setup = tegra_pcie_setup;
> +       hw.scan = tegra_pcie_scan_bus;
> +       hw.map_irq = tegra_pcie_map_irq;
> +
> +       pci_common_init(&hw);
> +
> +       return 0;
> +}

[snip]

> +static int tegra_pcie_probe(struct platform_device *pdev)
> +{
> +       struct device_node *port;
> +       struct tegra_pcie *pcie;
> +       int err;
> +
> +       pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
> +       if (!pcie)
> +               return -ENOMEM;
> +
> +       INIT_LIST_HEAD(&pcie->ports);
> +       pcie->dev = &pdev->dev;
> +
> +       err = tegra_pcie_parse_dt(pcie);
> +       if (err < 0)
> +               return err;
> +
> +       pcibios_min_mem = 0;
> +
> +       err = tegra_pcie_get_resources(pcie);
> +       if (err < 0) {
> +               dev_err(&pdev->dev, "failed to request resources: %d\n", err);
> +               return err;
> +       }
> +
> +       err = tegra_pcie_enable_controller(pcie);
> +       if (err)
> +               goto put_resources;
> +
> +       /* probe root ports */
> +       for_each_child_of_node(pdev->dev.of_node, port) {
> +               if (!of_device_is_available(port))
> +                       continue;
> +
> +               err = tegra_pcie_add_port(pcie, port);
> +               if (err < 0) {
> +                       dev_err(&pdev->dev, "failed to add port %s: %d\n",
> +                               port->name, err);
> +               }
> +       }
> +
> +       /* setup the AFI address translations */
> +       tegra_pcie_setup_translations(pcie);
> +
> +       if (IS_ENABLED(CONFIG_PCI_MSI)) {
> +               err = tegra_pcie_enable_msi(pcie);
> +               if (err < 0) {
> +                       dev_err(&pdev->dev,
> +                               "failed to enable MSI support: %d\n",
> +                               err);
> +                       goto put_resources;
> +               }
> +       }
> +
> +       err = tegra_pcie_enable(pcie);
> +       if (err < 0) {
> +               dev_err(&pdev->dev, "failed to enable PCIe ports: %d\n", err);
> +               goto disable_msi;
> +       }
> +
> +       platform_set_drvdata(pdev, pcie);
> +       return 0;
> +
> +disable_msi:
> +       if (IS_ENABLED(CONFIG_PCI_MSI))
> +               tegra_pcie_disable_msi(pcie);
> +put_resources:
> +       tegra_pcie_put_resources(pcie);
> +       return err;
> +}
> +

[snip]

> +
> +static const struct of_device_id tegra_pcie_of_match[] = {
> +       { .compatible = "nvidia,tegra20-pcie", },
> +       { },
> +};
> +
> +static struct platform_driver tegra_pcie_driver = {
> +       .driver = {
> +               .name = "tegra-pcie",
> +               .owner = THIS_MODULE,
> +               .of_match_table = tegra_pcie_of_match,
> +       },
> +       .probe = tegra_pcie_probe,
> +       .remove = tegra_pcie_remove,
> +};
> +module_platform_driver(tegra_pcie_driver);

If you have multiple 'nvidia,tegra20-pcie's in your DT then you will end up
with multiple calls to tegra_pcie_probe/tegra_pcie_enable/pci_common_init.

However pci_common_init/pcibios_init_hw assumes it will only ever be called
once, and will thus result in trying to create multiple busses with the same
bus number. (The first root bus it creates is always zero provided you haven't
implemented hw->scan).

I have a patch for this if you want to fold it into your series? (I see you've
made changes to bios32 for per-controller data).

Andrew Murray

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists