[LEDE-DEV,v2] imx6: fix pcie scanning on boot

By default, when the imx6 PCIe RC boots up, the subordinate is set
equally to the secondary bus (1), and does not alter afterwards.

This means that theoretically, the highest bus reachable downstream is
bus 1.

Before upstream commit a20c7f36bd3d ("PCI: Do not allocate more buses
than available in parent"), the driver ignored the subord value and just
allowed up to 0xff on each device downstream.

This caused a lot of errors to be printed, as this is not logical
according to spec. (but it worked ..)

After this commit, the driver stopped scanning deeper when the last
allocated busnr equals the subordinate of it's master, causing devices
to be undiscovered (especially behind bridges), uncovering the impact of
this bug.

Before:

00:00.0 PCI bridge: Synopsys, Inc. Device abcd (rev 01) (prog-if 00 ...
	Bus: primary=00, secondary=01, subordinate=01, sec-latency=0

00:00.0 0604: 16c3:abcd (rev 01)
01:00.0 0604: 10b5:8604 (rev ba)
... stops after bus 1 ...

After:

00:00.0 PCI bridge: Synopsys, Inc. Device abcd (rev 01) (prog-if 00
...
	Bus: primary=00, secondary=01, subordinate=ff, sec-latency=0

00:00.0 0604: 16c3:abcd (rev 01)
01:00.0 0604: 10b5:8604 (rev ba)
02:01.0 0604: 10b5:8604 (rev ba)
02:04.0 0604: 10b5:8604 (rev ba)
02:05.0 0604: 10b5:8604 (rev ba)
03:00.0 0280: 168c:0033 (rev 01)
05:00.0 0280: 168c:0033 (rev 01)

This upstream commit was introduced in kernel 4.9.71

Signed-off-by: Koen Vandeputte <koen.vandeputte@ncentric.com>
---

V2:

Proper fix for the real rootcause
kernel 4.4 is not affected


 .../patches-4.9/230-fix-pcie-enumeration.patch     | 73 ++++++++++++++++++++++
 1 file changed, 73 insertions(+)
 create mode 100644 target/linux/imx6/patches-4.9/230-fix-pcie-enumeration.patch

On Thu, Jan 4, 2018 at 8:23 AM, Koen Vandeputte
<koen.vandeputte@ncentric.com> wrote:
> By default, when the imx6 PCIe RC boots up, the subordinate is set
> equally to the secondary bus (1), and does not alter afterwards.
>
> This means that theoretically, the highest bus reachable downstream is
> bus 1.
>
> Before upstream commit a20c7f36bd3d ("PCI: Do not allocate more buses
> than available in parent"), the driver ignored the subord value and just
> allowed up to 0xff on each device downstream.
>
> This caused a lot of errors to be printed, as this is not logical
> according to spec. (but it worked ..)
>
> After this commit, the driver stopped scanning deeper when the last
> allocated busnr equals the subordinate of it's master, causing devices
> to be undiscovered (especially behind bridges), uncovering the impact of
> this bug.
>
> Before:
>
> 00:00.0 PCI bridge: Synopsys, Inc. Device abcd (rev 01) (prog-if 00 ...
>         Bus: primary=00, secondary=01, subordinate=01, sec-latency=0
>
> 00:00.0 0604: 16c3:abcd (rev 01)
> 01:00.0 0604: 10b5:8604 (rev ba)
> ... stops after bus 1 ...
>
> After:
>
> 00:00.0 PCI bridge: Synopsys, Inc. Device abcd (rev 01) (prog-if 00
> ...
>         Bus: primary=00, secondary=01, subordinate=ff, sec-latency=0
>
> 00:00.0 0604: 16c3:abcd (rev 01)
> 01:00.0 0604: 10b5:8604 (rev ba)
> 02:01.0 0604: 10b5:8604 (rev ba)
> 02:04.0 0604: 10b5:8604 (rev ba)
> 02:05.0 0604: 10b5:8604 (rev ba)
> 03:00.0 0280: 168c:0033 (rev 01)
> 05:00.0 0280: 168c:0033 (rev 01)
>
> This upstream commit was introduced in kernel 4.9.71
>
> Signed-off-by: Koen Vandeputte <koen.vandeputte@ncentric.com>
> ---
>
> V2:
>
> Proper fix for the real rootcause
> kernel 4.4 is not affected
>
>
>  .../patches-4.9/230-fix-pcie-enumeration.patch     | 73 ++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
>  create mode 100644 target/linux/imx6/patches-4.9/230-fix-pcie-enumeration.patch
>
> diff --git a/target/linux/imx6/patches-4.9/230-fix-pcie-enumeration.patch b/target/linux/imx6/patches-4.9/230-fix-pcie-enumeration.patch
> new file mode 100644
> index 000000000000..18f22347172b
> --- /dev/null
> +++ b/target/linux/imx6/patches-4.9/230-fix-pcie-enumeration.patch
> @@ -0,0 +1,73 @@
> +By default, when the imx6 PCIe RC boots up, the subordinate is set
> +equally to the secondary bus (1), and does not alter afterwards.
> +
> +This means that theoretically, the highest bus reachable downstream is
> +bus 1.
> +
> +Before commit a20c7f36bd3d ("PCI: Do not allocate more buses than
> +available in parent"), the driver ignored the subord value and just
> +allowed up to 0xff on each device downstream.
> +
> +This caused a lot of errors to be printed, as this is not logical
> +according to spec. (but it worked ..)
> +
> +After this commit, the driver stopped scanning deeper when the last
> +allocated busnr equals the subordinate of it's master, causing devices
> +to be undiscovered (especially behind bridges), uncovering the impact of
> +this bug.
> +
> +Before:
> +
> +00:00.0 PCI bridge: Synopsys, Inc. Device abcd (rev 01) (prog-if 00 ...
> +       Bus: primary=00, secondary=01, subordinate=01, sec-latency=0
> +
> +00:00.0 0604: 16c3:abcd (rev 01)
> +01:00.0 0604: 10b5:8604 (rev ba)
> +... stops after bus 1 ...
> +
> +After:
> +
> +00:00.0 PCI bridge: Synopsys, Inc. Device abcd (rev 01) (prog-if 00
> +...
> +       Bus: primary=00, secondary=01, subordinate=ff, sec-latency=0
> +
> +00:00.0 0604: 16c3:abcd (rev 01)
> +01:00.0 0604: 10b5:8604 (rev ba)
> +02:01.0 0604: 10b5:8604 (rev ba)
> +02:04.0 0604: 10b5:8604 (rev ba)
> +02:05.0 0604: 10b5:8604 (rev ba)
> +03:00.0 0280: 168c:0033 (rev 01)
> +05:00.0 0280: 168c:0033 (rev 01)
> +
> +Signed-off-by: Koen Vandeputte <koen.vandeputte@ncentric.com>
> +
> +--- a/drivers/pci/host/pci-imx6.c
> ++++ b/drivers/pci/host/pci-imx6.c
> +@@ -64,6 +64,9 @@ struct imx6_pcie {
> +
> + #define PCIE_RC_LCSR                          0x80
> +
> ++#define PCIE_RC_BNR                           0x18
> ++#define PCIE_RC_BNR_MAX_SUBORDINATE           (0xff << 16)
> ++
> + /* PCIe Port Logic registers (memory-mapped) */
> + #define PL_OFFSET 0x700
> + #define PCIE_PL_PFLR (PL_OFFSET + 0x08)
> +@@ -488,6 +491,17 @@ static int imx6_pcie_establish_link(stru
> +       int ret;
> +
> +       /*
> ++       * By default, the subordinate is set equally to the secondary
> ++       * bus (0x01) when the RC boots.
> ++       * This means that theoretically, only bus 1 is reachable from the RC.
> ++       * Force the PCIe RC subordinate to 0xff, otherwise no downstream
> ++       * devices will be detected behind bus 1.
> ++      */
> ++      tmp = dw_pcie_readl_rc(pp, PCIE_RC_BNR);
> ++      tmp |= PCIE_RC_BNR_MAX_SUBORDINATE;
> ++      dw_pcie_writel_rc(pp, PCIE_RC_BNR, tmp);
> ++
> ++      /*
> +        * Force Gen1 operation when starting the link.  In case the link is
> +        * started in Gen2 mode, there is a possibility the devices on the
> +        * bus will not be detected at all.  This happens with PCIe switches.
> --
> 2.7.4
>

Hi Koen,

Thanks for this info!

Have you asked the linux-pci group about this? I'm curious what the
patch to 4.9.71 was trying to do and if this is the proper place to
fix this. I assume if we set that value in the bootloader it would get
reset when the kernel does a PCI reset.

It seems like every couple of months there is some upstream breakage
dealing with the i.MX6 and a PCIe switch :(

Regards,

Tim

3rd time's the charm :)  .. without html ..

>
>>     >
>>
>>     Hi Koen,
>>
>>     Thanks for this info!
>>
>>     Have you asked the linux-pci group about this? I'm curious what the
>>     patch to 4.9.71 was trying to do and if this is the proper place to
>>     fix this. I assume if we set that value in the bootloader it
>>     would get
>>     reset when the kernel does a PCI reset.
>>
>>     It seems like every couple of months there is some upstream breakage
>>     dealing with the i.MX6 and a PCIe switch :(
>>
>>     Regards,
>>
>>     Tim
>>
>
> Hi Tim,
>
> After this quick fix, Ive investigated this deeper.
>
> It seems not only imx6 was affected but all platforms using dwc for pcie.
> meanwhile, a definitive fix was submitted upstream towards the pci guys:
> https://patchwork.kernel.org/patch/10163363/
>
>
> The patch causing this actually makes sense:
> It enforces setting all port params to be set correctly according to 
> pcie spec.
>
> Separate backports are needed for 4.9 and older LTS kernels as the dwc 
> init code got moved.
> In 4.9 the backport will actually fix enumeration behind bridges.
> in 4.4 and older, no functional behaviour changes, but the nasty 
> warnings and errors are gone during boot.
>
> it is correct that setting this in bootloader doesnt affect the kernel 
> which resets the bus and reinits it completely.
> Just to ensure both are correct, Ive also submitted a fix to uboot 
> (imx6 only)
> http://git.denx.de/?p=u-boot.git;a=commit;h=f57263ee9bb8b5d9f39b48d09d21114c9dbb6a02
>
>
> Kind regards,
>
> Koen