diff mbox

[V2,5/9] PCI: Export pci_flags

Message ID 1511638333-22951-6-git-send-email-mmaddireddy@nvidia.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Manikanta Maddireddy Nov. 25, 2017, 7:32 p.m. UTC
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(+)

Comments

Bjorn Helgaas Nov. 29, 2017, 5:01 p.m. UTC | #1
[+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
>
Lorenzo Pieralisi Nov. 30, 2017, 10:24 a.m. UTC | #2
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
Bjorn Helgaas Nov. 30, 2017, 6:42 p.m. UTC | #3
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
Manikanta Maddireddy Nov. 30, 2017, 7:38 p.m. UTC | #4
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 mbox

Patch

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;