...
>> + bus = pci_create_root_bus(pcie->dev, 0, &iproc_pcie_ops,
>> + &pcie->sysdata, pcie->resources);
>> + if (!bus) {
>> + dev_err(pcie->dev, "unable to create PCI root bus\n");
>> + ret = -ENOMEM;
>> + goto err_power_off_phy;
>> + }
>> + pcie->root_bus = bus;
>> +
>> + ret = iproc_pcie_check_link(pcie, bus);
>> + if (ret) {
>> + dev_err(pcie->dev, "no PCIe EP device detected\n");
>> + goto err_rm_root_bus;
>> + }
>> +
>> + iproc_pcie_enable(pcie);
>> +
>> + pci_scan_child_bus(bus);
>> + pci_assign_unassigned_bus_resources(bus);
>> + pci_bus_add_devices(bus);
>> +
>> + pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
>
> I think the IRQ fixup should be before pci_bus_add_devices(). CC'd Yijing,
> who's been fixing similar issues.
>
> pci_bus_add_devices() is the point where drivers can claim these devices,
> and we don't want to change things after a driver claims the device.
Yes, we should call pci_bus_add_devices() at last, after all resources(mem/io/irq) are
configured properly. Otherwise, this code could not be run normally in non system boot up path.
Thanks!
Yijing.
>
>> +
>> + return 0;
>> +
>> +err_rm_root_bus:
>> + pci_stop_root_bus(bus);
>> + pci_remove_root_bus(bus);
>> +
>> +err_power_off_phy:
>> + if (pcie->phy)
>> + phy_power_off(pcie->phy);
>> +err_exit_phy:
>> + if (pcie->phy)
>> + phy_exit(pcie->phy);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL(iproc_pcie_setup);
>> +
>> +int iproc_pcie_remove(struct iproc_pcie *pcie)
>> +{
>> + pci_stop_root_bus(pcie->root_bus);
>> + pci_remove_root_bus(pcie->root_bus);
>> +
>> + if (pcie->phy) {
>> + phy_power_off(pcie->phy);
>> + phy_exit(pcie->phy);
>> + }
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(iproc_pcie_remove);
>
> These exports wouldn't be needed if this were all squashed into one file.
>
>> +
>> +MODULE_AUTHOR("Ray Jui <rjui@xxxxxxxxxxxx>");
>> +MODULE_DESCRIPTION("Broadcom iPROC PCIe common driver");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
>> new file mode 100644
>> index 0000000..569bc04
>> --- /dev/null
>> +++ b/drivers/pci/host/pcie-iproc.h
>> @@ -0,0 +1,42 @@
>> +/*
>> + * Copyright (C) 2014-2015 Broadcom Corporation
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation version 2.
>> + *
>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
>> + * kind, whether express or implied; without even the implied warranty
>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#ifndef _PCIE_IPROC_H
>> +#define _PCIE_IPROC_H
>> +
>> +#define IPROC_PCIE_MAX_NUM_IRQS 6
>> +
>> +/**
>> + * iProc PCIe device
>> + * @dev: pointer to device data structure
>> + * @base: PCIe host controller I/O register base
>> + * @resources: linked list of all PCI resources
>> + * @sysdata: Per PCI controller data
>> + * @root_bus: pointer to root bus
>> + * @phy: optional PHY device that controls the Serdes
>> + * @irqs: interrupt IDs
>> + */
>> +struct iproc_pcie {
>> + struct device *dev;
>> + void __iomem *base;
>> + struct list_head *resources;
>> + struct pci_sys_data sysdata;
>> + struct pci_bus *root_bus;
>> + struct phy *phy;
>> + int irqs[IPROC_PCIE_MAX_NUM_IRQS];
>> +};
>> +
>> +extern int iproc_pcie_setup(struct iproc_pcie *pcie);
>> +extern int iproc_pcie_remove(struct iproc_pcie *pcie);
>
> Hopefully you can squash this all into one file, but if you can't, my
> preference is to omit "extern" on function declarations.
>
>> +
>> +#endif /* _PCIE_IPROC_H */
>> --
>> 1.7.9.5
>>
>
> .
>
--
Thanks!
Yijing
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
