diff mbox

imx6: fix pcie enumeration

Message ID 1684b8c6-1006-948b-f4f9-c9aaf9cf26a8@ncentric.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Koen Vandeputte Jan. 8, 2018, 11:13 a.m. UTC 
On 2018-01-08 12:00, Lorenzo Pieralisi wrote:
> [+cc Joao, Jingoo]
>
> On Mon, Jan 08, 2018 at 09:51:37AM +0100, Koen Vandeputte wrote:
>
> [...]
>
>> [ Node 4 | node-4 ] lspci -v
>> 00:00.0 PCI bridge: Synopsys, Inc. Device abcd (rev 01) (prog-if 00
>> [Normal decode])
>>      Flags: bus master, fast devsel, latency 0, IRQ 298
>>      Memory at 01000000 (32-bit, non-prefetchable) [size=1M]
>>      Bus: primary=00, secondary=01, subordinate=01, sec-latency=0
>                                       ^^^^^^^^^^^^^^
>
> So basically, the subordinate number in the root port does not
> affect config space forwarding from what I see and it has always
> been like that for dwc.
>
> You are forced to update it to 0xff because otherwise the kernel
> stops enumerating bus numbers > 1
Indeed, which affects all devices using Designware PCIe init + a PCIe 
bridge downstream
> but that's a software issue
> not HW - the subordinate bus number does not seem to affect anything
> here.

> Sigh.
>
> Another option would consist in forcing the kernel to reassign
> all bus numbers by setting the PCI_REASSIGN_ALL_BUS flag but
> that's not a good idea given how inconsistent that flag usage is.
>
> I think that updating the subordinate bus numbers in the DWC
> config register is the correct solution to make sure the kernel
> won't get confused anymore by what seems to be a fake root port,
> I need input from DWC maintainers to confirm my understanding.
>
> Thanks,
> Lorenzo
>

The patch I'm currently using internally:




Above version logically fixes it for all dwc devices using a bridge 
after the RC, not only imx6.
If this is fine, I would submit the patch above and drop the current one.

Backporting this to stable kernels (4.9 .. 4.4 .. etc) will fix all 
nasty warnings on these setups during boot without any change in 
functionality.
These kernels will require a separate patch as this source file got 
moved & renamed.


Thanks for your time and analysis so far,

Koen

Comments

Lorenzo Pieralisi Jan. 8, 2018, 3:46 p.m. UTC | #1 
On Mon, Jan 08, 2018 at 12:13:34PM +0100, Koen Vandeputte wrote:
> 
> 
> On 2018-01-08 12:00, Lorenzo Pieralisi wrote:
> >[+cc Joao, Jingoo]
> >
> >On Mon, Jan 08, 2018 at 09:51:37AM +0100, Koen Vandeputte wrote:
> >
> >[...]
> >
> >>[ Node 4 | node-4 ] lspci -v
> >>00:00.0 PCI bridge: Synopsys, Inc. Device abcd (rev 01) (prog-if 00
> >>[Normal decode])
> >>     Flags: bus master, fast devsel, latency 0, IRQ 298
> >>     Memory at 01000000 (32-bit, non-prefetchable) [size=1M]
> >>     Bus: primary=00, secondary=01, subordinate=01, sec-latency=0
> >                                      ^^^^^^^^^^^^^^
> >
> >So basically, the subordinate number in the root port does not
> >affect config space forwarding from what I see and it has always
> >been like that for dwc.
> >
> >You are forced to update it to 0xff because otherwise the kernel
> >stops enumerating bus numbers > 1
> Indeed, which affects all devices using Designware PCIe init + a
> PCIe bridge downstream
> >but that's a software issue
> >not HW - the subordinate bus number does not seem to affect anything
> >here.
> 
> >Sigh.
> >
> >Another option would consist in forcing the kernel to reassign
> >all bus numbers by setting the PCI_REASSIGN_ALL_BUS flag but
> >that's not a good idea given how inconsistent that flag usage is.
> >
> >I think that updating the subordinate bus numbers in the DWC
> >config register is the correct solution to make sure the kernel
> >won't get confused anymore by what seems to be a fake root port,
> >I need input from DWC maintainers to confirm my understanding.
> >
> >Thanks,
> >Lorenzo
> >
> 
> The patch I'm currently using internally:
> 
> 
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -861,7 +861,7 @@ void dw_pcie_setup_rc(struct pcie_port *
>      /* setup bus numbers */
>      val = dw_pcie_readl_rc(pp, PCI_PRIMARY_BUS);
>      val &= 0xff000000;
> -    val |= 0x00010100;
> +    val |= 0x00ff0100;
>      dw_pcie_writel_rc(pp, PCI_PRIMARY_BUS, val);
> 
>      /* setup command register */
> 
> 
> Above version logically fixes it for all dwc devices using a bridge
> after the RC, not only imx6.
> If this is fine, I would submit the patch above and drop the current one.
It is fine by me but I won't merge it till I get ACKs and tested-by
from the respective maintainers - it can have potential widespread
impact.

> Backporting this to stable kernels (4.9 .. 4.4 .. etc) will fix all
> nasty warnings on these setups during boot without any change in
> functionality.
> These kernels will require a separate patch as this source file got
> moved & renamed.
> Thanks for your time and analysis so far,

Thank you for reporting it and fixing it.

Lorenzo
Niklas Cassel Jan. 9, 2018, 1:48 p.m. UTC | #2 
On 08/01/18 12:13, Koen Vandeputte wrote:
> 
> 
> On 2018-01-08 12:00, Lorenzo Pieralisi wrote:
>> [+cc Joao, Jingoo]
>>
>> On Mon, Jan 08, 2018 at 09:51:37AM +0100, Koen Vandeputte wrote:
>>
>> [...]
>>
>>> [ Node 4 | node-4 ] lspci -v
>>> 00:00.0 PCI bridge: Synopsys, Inc. Device abcd (rev 01) (prog-if 00
>>> [Normal decode])
>>>      Flags: bus master, fast devsel, latency 0, IRQ 298
>>>      Memory at 01000000 (32-bit, non-prefetchable) [size=1M]
>>>      Bus: primary=00, secondary=01, subordinate=01, sec-latency=0
>>                                       ^^^^^^^^^^^^^^
>>
>> So basically, the subordinate number in the root port does not
>> affect config space forwarding from what I see and it has always
>> been like that for dwc.
>>
>> You are forced to update it to 0xff because otherwise the kernel
>> stops enumerating bus numbers > 1
> Indeed, which affects all devices using Designware PCIe init + a PCIe bridge downstream
>> but that's a software issue
>> not HW - the subordinate bus number does not seem to affect anything
>> here.
> 
>> Sigh.
>>
>> Another option would consist in forcing the kernel to reassign
>> all bus numbers by setting the PCI_REASSIGN_ALL_BUS flag but
>> that's not a good idea given how inconsistent that flag usage is.
>>
>> I think that updating the subordinate bus numbers in the DWC
>> config register is the correct solution to make sure the kernel
>> won't get confused anymore by what seems to be a fake root port,
>> I need input from DWC maintainers to confirm my understanding.
>>
>> Thanks,
>> Lorenzo
>>
> 
> The patch I'm currently using internally:
> 
> 
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -861,7 +861,7 @@ void dw_pcie_setup_rc(struct pcie_port *
>      /* setup bus numbers */
>      val = dw_pcie_readl_rc(pp, PCI_PRIMARY_BUS);
>      val &= 0xff000000;
> -    val |= 0x00010100;
> +    val |= 0x00ff0100;
>      dw_pcie_writel_rc(pp, PCI_PRIMARY_BUS, val);
> 
>      /* setup command register */
> 
> 
> Above version logically fixes it for all dwc devices using a bridge after the RC, not only imx6.
> If this is fine, I would submit the patch above and drop the current one.

I can confirm that commit a20c7f36bd3d ("PCI: Do not allocate more buses than
available in parent") broke enumerating PCIe devices behind a PCIe switch
on ARTPEC-6 (which uses the DWC IP).
(FTR, arch/arm/boot/dts/artpec6.dtsi specifies bus-range = <0x00 0xff>).

This patch resolves the problem.
(I verified on linux-next: next-20180109).

However, note that I had to patch the file
drivers/pci/dwc/pcie-designware-host.c
rather than
drivers/pci/host/pcie-designware.c,
which this patch suggests.

Please feel free to submit a new patch with:

Tested-by: Niklas Cassel <niklas.cassel@axis.com>

> 
> Backporting this to stable kernels (4.9 .. 4.4 .. etc) will fix all nasty warnings on these setups during boot without any change in functionality.
> These kernels will require a separate patch as this source file got moved & renamed.
> 
> 
> Thanks for your time and analysis so far,
> 
> Koen
Koen Vandeputte Jan. 9, 2018, 1:58 p.m. UTC | #3 
On 2018-01-09 14:48, Niklas Cassel wrote:
> <snip>

>>       /* setup bus numbers */
>>       val = dw_pcie_readl_rc(pp, PCI_PRIMARY_BUS);
>>       val &= 0xff000000;
>> -    val |= 0x00010100;
>> +    val |= 0x00ff0100;
>>       dw_pcie_writel_rc(pp, PCI_PRIMARY_BUS, val);
>>
>>       /* setup command register */
>>
>>
>> Above version logically fixes it for all dwc devices using a bridge after the RC, not only imx6.
>> If this is fine, I would submit the patch above and drop the current one.
> I can confirm that commit a20c7f36bd3d ("PCI: Do not allocate more buses than
> available in parent") broke enumerating PCIe devices behind a PCIe switch
> on ARTPEC-6 (which uses the DWC IP).
> (FTR, arch/arm/boot/dts/artpec6.dtsi specifies bus-range = <0x00 0xff>).
>
> This patch resolves the problem.
> (I verified on linux-next: next-20180109).
>
> However, note that I had to patch the file
> drivers/pci/dwc/pcie-designware-host.c
> rather than
> drivers/pci/host/pcie-designware.c,
> which this patch suggests.
Above internally used example patch is based on kernel 4.9.74, where 
this file hasn't moved/renamed yet to dwc subfolder
I'll post a proper one shortly for the upstream tree.

Thanks for testing,

Koen
> Please feel free to submit a new patch with:
>
> Tested-by: Niklas Cassel <niklas.cassel@axis.com>
>
diff mbox

Patch

--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -861,7 +861,7 @@  void dw_pcie_setup_rc(struct pcie_port *
      /* setup bus numbers */
      val = dw_pcie_readl_rc(pp, PCI_PRIMARY_BUS);
      val &= 0xff000000;
-    val |= 0x00010100;
+    val |= 0x00ff0100;
      dw_pcie_writel_rc(pp, PCI_PRIMARY_BUS, val);

      /* setup command register */