| Message ID | 1511638333-22951-6-git-send-email-mmaddireddy@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add loadable kernel module and power management support | expand |
[+cc Lorenzo] On Sun, Nov 26, 2017 at 01:02:09AM +0530, Manikanta Maddireddy wrote: > pci_flags variable is used in inline functions in 'pci.h', Tegra PCIe > driver use one of these functions pci_add_flags() and includes 'pci.h'. > Export pci_flags to allow Tegra PCIe host controller driver to be > compiled as loadable kernel module. Here's the usage in tegra_pcie_probe(): pci_add_flags(PCI_REASSIGN_ALL_RSRC | PCI_REASSIGN_ALL_BUS); We've probably had this discussion before, but I don't know why Tegra needs PCI_REASSIGN_ALL_RSRC and PCI_REASSIGN_ALL_BUS. I would prefer to drop this usage of pci_add_flags() if possible. It seems to be just an arm/powerpc thing and I'm not convinced it's really necessary. > Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com> > --- > V2: > * commit message update > > drivers/pci/setup-bus.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c > index b1ad466199ad..3567e1c4e340 100644 > --- a/drivers/pci/setup-bus.c > +++ b/drivers/pci/setup-bus.c > @@ -29,6 +29,7 @@ > #include "pci.h" > > unsigned int pci_flags; > +EXPORT_SYMBOL(pci_flags); > > struct pci_dev_resource { > struct list_head list; > -- > 2.1.4 >
On Wed, Nov 29, 2017 at 11:01:33AM -0600, Bjorn Helgaas wrote: > [+cc Lorenzo] > > On Sun, Nov 26, 2017 at 01:02:09AM +0530, Manikanta Maddireddy wrote: > > pci_flags variable is used in inline functions in 'pci.h', Tegra PCIe > > driver use one of these functions pci_add_flags() and includes 'pci.h'. > > Export pci_flags to allow Tegra PCIe host controller driver to be > > compiled as loadable kernel module. > > Here's the usage in tegra_pcie_probe(): > > pci_add_flags(PCI_REASSIGN_ALL_RSRC | PCI_REASSIGN_ALL_BUS); > > We've probably had this discussion before, but I don't know why Tegra > needs PCI_REASSIGN_ALL_RSRC and PCI_REASSIGN_ALL_BUS. > > I would prefer to drop this usage of pci_add_flags() if possible. It > seems to be just an arm/powerpc thing and I'm not convinced it's > really necessary. It is hard to say if it is really necessary (because it depends on firmware configuration - ie pci_scan_bridge()), that's the problem. I suspect it can trigger regressions if we do not set it (since it affects what pcibios_assign_all_busses() returns on eg arm/arm64). There are two things we can do: 1) Set it unconditionally in arch code (in a hook to be defined) 2) We remove it on a per-host bridge basis and ask for testing I agree this may have trickled from host bridge to host bridge through copy'n'paste and it is not based on any firmware assumtpion but I can't say if it is really needed. Lorenzo
On Thu, Nov 30, 2017 at 10:24:37AM +0000, Lorenzo Pieralisi wrote: > On Wed, Nov 29, 2017 at 11:01:33AM -0600, Bjorn Helgaas wrote: > > [+cc Lorenzo] > > > > On Sun, Nov 26, 2017 at 01:02:09AM +0530, Manikanta Maddireddy wrote: > > > pci_flags variable is used in inline functions in 'pci.h', Tegra PCIe > > > driver use one of these functions pci_add_flags() and includes 'pci.h'. > > > Export pci_flags to allow Tegra PCIe host controller driver to be > > > compiled as loadable kernel module. > > > > Here's the usage in tegra_pcie_probe(): > > > > pci_add_flags(PCI_REASSIGN_ALL_RSRC | PCI_REASSIGN_ALL_BUS); > > > > We've probably had this discussion before, but I don't know why Tegra > > needs PCI_REASSIGN_ALL_RSRC and PCI_REASSIGN_ALL_BUS. > > > > I would prefer to drop this usage of pci_add_flags() if possible. It > > seems to be just an arm/powerpc thing and I'm not convinced it's > > really necessary. > > It is hard to say if it is really necessary (because it depends > on firmware configuration - ie pci_scan_bridge()), that's the > problem. > > I suspect it can trigger regressions if we do not set it (since > it affects what pcibios_assign_all_busses() returns on eg arm/arm64). > > There are two things we can do: > > 1) Set it unconditionally in arch code (in a hook to be defined) > 2) We remove it on a per-host bridge basis and ask for testing > > I agree this may have trickled from host bridge to host bridge through > copy'n'paste and it is not based on any firmware assumtpion but I can't > say if it is really needed. My basic position is that if resources are not assigned correctly, the PCI core should automatically try to assign them, regardless of whether PCI_REASSIGN_ALL_RSRC or PCI_REASSIGN_ALL_BUS is set. If that doesn't work, I think there's something wrongin the PCI core and we should fix that. This might be an opportunity to try removing the use of pci_add_flags() and see what breaks. Bjorn
On 01-Dec-17 12:12 AM, Bjorn Helgaas wrote: > On Thu, Nov 30, 2017 at 10:24:37AM +0000, Lorenzo Pieralisi wrote: >> On Wed, Nov 29, 2017 at 11:01:33AM -0600, Bjorn Helgaas wrote: >>> [+cc Lorenzo] >>> >>> On Sun, Nov 26, 2017 at 01:02:09AM +0530, Manikanta Maddireddy wrote: >>>> pci_flags variable is used in inline functions in 'pci.h', Tegra PCIe >>>> driver use one of these functions pci_add_flags() and includes 'pci.h'. >>>> Export pci_flags to allow Tegra PCIe host controller driver to be >>>> compiled as loadable kernel module. >>> >>> Here's the usage in tegra_pcie_probe(): >>> >>> pci_add_flags(PCI_REASSIGN_ALL_RSRC | PCI_REASSIGN_ALL_BUS); >>> >>> We've probably had this discussion before, but I don't know why Tegra >>> needs PCI_REASSIGN_ALL_RSRC and PCI_REASSIGN_ALL_BUS. >>> >>> I would prefer to drop this usage of pci_add_flags() if possible. It >>> seems to be just an arm/powerpc thing and I'm not convinced it's >>> really necessary. >> >> It is hard to say if it is really necessary (because it depends >> on firmware configuration - ie pci_scan_bridge()), that's the >> problem. >> >> I suspect it can trigger regressions if we do not set it (since >> it affects what pcibios_assign_all_busses() returns on eg arm/arm64). >> >> There are two things we can do: >> >> 1) Set it unconditionally in arch code (in a hook to be defined) >> 2) We remove it on a per-host bridge basis and ask for testing >> >> I agree this may have trickled from host bridge to host bridge through >> copy'n'paste and it is not based on any firmware assumtpion but I can't >> say if it is really needed. > > My basic position is that if resources are not assigned correctly, the > PCI core should automatically try to assign them, regardless of > whether PCI_REASSIGN_ALL_RSRC or PCI_REASSIGN_ALL_BUS is set. If that > doesn't work, I think there's something wrongin the PCI core and we > should fix that. > > This might be an opportunity to try removing the use of > pci_add_flags() and see what breaks. > > Bjorn > As per the Tegra TRM primary, secondary and subordinate default bus numbers are 0 and it is expecting SW to program these numbers. I believe this is the reason for adding PCI_REASSIGN_ALL_RSRC and PCI_REASSIGN_ALL_BUS flags. Looking at the function pci_scan_bridge_extend(), if secondary and subordinate bus numbers are 0 it is assigning bus numbers even if these flags are not set. In the basic testing with one endpoint PCIe link up is working fine without these flags, however I would like test with PCIe switch and multiple endpoints connected to Tegra. Manikanta
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c index b1ad466199ad..3567e1c4e340 100644 --- a/drivers/pci/setup-bus.c +++ b/drivers/pci/setup-bus.c @@ -29,6 +29,7 @@ #include "pci.h" unsigned int pci_flags; +EXPORT_SYMBOL(pci_flags); struct pci_dev_resource { struct list_head list;
pci_flags variable is used in inline functions in 'pci.h', Tegra PCIe driver use one of these functions pci_add_flags() and includes 'pci.h'. Export pci_flags to allow Tegra PCIe host controller driver to be compiled as loadable kernel module. Signed-off-by: Manikanta Maddireddy <mmaddireddy@nvidia.com> --- V2: * commit message update drivers/pci/setup-bus.c | 1 + 1 file changed, 1 insertion(+)