Message-ID: <20130118100950.GA7596@avionic-0098.adnet.avionic-design.de>
Date:	Fri, 18 Jan 2013 11:09:50 +0100
From:	Thierry Reding <thierry.reding@...onic-design.de>
To:	Andrew Murray <andrew.murray@....com>
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 Fri, Jan 18, 2013 at 09:56:20AM +0000, Andrew Murray wrote:
> 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).

Right, I hadn't noticed. There's currently no hardware that actually has
two PCIe host bridges but I wanted to keep the driver properly prepared
in case this ever happened.

Actually I've reimplemented hw->scan, but it still forwards the bus
number setup by pcibios_init_hw() (sys->busnr) to pci_create_root_bus()
so it will still break. I wonder, though, if a better approach would be
to take this number from the bus-range property in DT instead.

> 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).

I would certainly like to take a look at it.

Thierry

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists