Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host

From: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
To: Andrew Murray <andrew.murray-5wv7dgnIgG8@public.gmane.org>
Cc: "linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	Grant Likely
	<grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>,
	"rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org"
	<rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>,
	Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>,
	Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>,
	Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>,
	Jason Gunthorpe
	<jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>,
	Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>,
	Thomas Petazzoni
	<thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>,
	"devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org"
	<devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org>,
	"linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	"linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org"
	<linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>,
	"linux-pci-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<linux-pci-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Subject: Re: [PATCH 10/14] PCI: tegra: Move PCIe driver to drivers/pci/host
Date: Fri, 18 Jan 2013 11:09:50 +0100	[thread overview]
Message-ID: <20130118100950.GA7596@avionic-0098.adnet.avionic-design.de> (raw)
In-Reply-To: <20130118095620.GA7552-5wv7dgnIgG8@public.gmane.org>

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

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>) as well as device tree support.
> > 
> > Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> 
> [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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]