| Message ID | 1496835374-13053-1-git-send-email-stefan.schoefegger@ginzinger.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Wed, Jun 07, 2017 at 01:36:11PM +0200, Stefan Schoefegger wrote: > Link speed must not be limited to gen1 during link test for compliance > tests > > Signed-off-by: Stefan Schoefegger <stefan.schoefegger@ginzinger.com> > --- > > Changes since v1: > - pci-imx6.c moved to dwc directory > > drivers/pci/dwc/Kconfig | 10 ++++++++++ > drivers/pci/dwc/pci-imx6.c | 21 ++++++++++++--------- > 2 files changed, 22 insertions(+), 9 deletions(-) > > diff --git a/drivers/pci/dwc/Kconfig b/drivers/pci/dwc/Kconfig > index b7e15526d676..b6e9ced5a45d 100644 > --- a/drivers/pci/dwc/Kconfig > +++ b/drivers/pci/dwc/Kconfig > @@ -77,6 +77,16 @@ config PCI_IMX6 > select PCIEPORTBUS > select PCIE_DW_HOST > > +config PCI_IMX6_COMPLIANCE_TEST > + bool "Enable pcie compliance tests on imx6" > + depends on PCI_IMX6 > + default n > + help > + Enables support for pcie compliance test on FSL iMX SoCs. > + The link speed wouldn't be limited to gen1 when enabled. > + Enable only during compliance tests, otherwise > + link detection will fail on some peripherals. I'm puzzled about why we would want to merge this patch. It looks like we're trying to game the system to make the device pass compliance testing when it isn't really compliant. Is this config option useful to users, or is it only useful during internal development of iMX SoCs? > config PCIE_SPEAR13XX > bool "STMicroelectronics SPEAr PCIe controller" > depends on PCI > diff --git a/drivers/pci/dwc/pci-imx6.c b/drivers/pci/dwc/pci-imx6.c > index 19a289b8cc94..b0fbe52e25b0 100644 > --- a/drivers/pci/dwc/pci-imx6.c > +++ b/drivers/pci/dwc/pci-imx6.c > @@ -533,15 +533,18 @@ static int imx6_pcie_establish_link(struct imx6_pcie *imx6_pcie) > u32 tmp; > int ret; > > - /* > - * 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. > - */ > - tmp = dw_pcie_readl_dbi(pci, PCIE_RC_LCR); > - tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK; > - tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1; > - dw_pcie_writel_dbi(pci, PCIE_RC_LCR, tmp); > + if (!IS_ENABLED(CONFIG_PCI_IMX6_COMPLIANCE_TEST)) { > + /* > + * 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. > + */ > + tmp = dw_pcie_readl_dbi(pci, PCIE_RC_LCR); > + tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK; > + tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1; > + dw_pcie_writel_dbi(pci, PCIE_RC_LCR, tmp); > + } > > /* Start LTSSM. */ > if (imx6_pcie->variant == IMX7D) > -- > 2.11.0 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> -----Original Message----- > From: Bjorn Helgaas [mailto:helgaas@kernel.org] > Sent: Tuesday, June 13, 2017 7:49 AM > To: Stefan Schoefegger <stefan.schoefegger@ginzinger.com> > Cc: linux-pci@vger.kernel.org; Richard Zhu <hongxing.zhu@nxp.com>; Arnd > Bergmann <arnd@arndb.de>; open list <linux-kernel@vger.kernel.org>; > Kishon Vijay Abraham I <kishon@ti.com>; Jingoo Han > <jingoohan1@gmail.com>; Bjorn Helgaas <bhelgaas@google.com>; > moderated list:PCI DRIVER FOR IMX6 <linux-arm-kernel@lists.infradead.org>; > Lucas Stach <l.stach@pengutronix.de> > Subject: Re: [PATCH v2 1/1] PCI: imx6: Add pcie compliance test option > > On Wed, Jun 07, 2017 at 01:36:11PM +0200, Stefan Schoefegger wrote: > > Link speed must not be limited to gen1 during link test for compliance > > tests > > > > Signed-off-by: Stefan Schoefegger <stefan.schoefegger@ginzinger.com> > > --- > > > > Changes since v1: > > - pci-imx6.c moved to dwc directory > > > > drivers/pci/dwc/Kconfig | 10 ++++++++++ > > drivers/pci/dwc/pci-imx6.c | 21 ++++++++++++--------- > > 2 files changed, 22 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/pci/dwc/Kconfig b/drivers/pci/dwc/Kconfig index > > b7e15526d676..b6e9ced5a45d 100644 > > --- a/drivers/pci/dwc/Kconfig > > +++ b/drivers/pci/dwc/Kconfig > > @@ -77,6 +77,16 @@ config PCI_IMX6 > > select PCIEPORTBUS > > select PCIE_DW_HOST > > > > +config PCI_IMX6_COMPLIANCE_TEST > > + bool "Enable pcie compliance tests on imx6" > > + depends on PCI_IMX6 > > + default n > > + help > > + Enables support for pcie compliance test on FSL iMX SoCs. > > + The link speed wouldn't be limited to gen1 when enabled. > > + Enable only during compliance tests, otherwise > > + link detection will fail on some peripherals. > > I'm puzzled about why we would want to merge this patch. It looks like > we're trying to game the system to make the device pass compliance testing > when it isn't really compliant. Is this config option useful to users, or is it only > useful during internal development of iMX SoCs? > [Zhu hongxing] Agree with Bjorn. These modifications shouldn't be merged. They are used for the boards used to pass the PCIe RC certification, when one Standalone external OSC is used as PCIe reference clock source in the boards design. > > config PCIE_SPEAR13XX > > bool "STMicroelectronics SPEAr PCIe controller" > > depends on PCI > > diff --git a/drivers/pci/dwc/pci-imx6.c b/drivers/pci/dwc/pci-imx6.c > > index 19a289b8cc94..b0fbe52e25b0 100644 > > --- a/drivers/pci/dwc/pci-imx6.c > > +++ b/drivers/pci/dwc/pci-imx6.c > > @@ -533,15 +533,18 @@ static int imx6_pcie_establish_link(struct > imx6_pcie *imx6_pcie) > > u32 tmp; > > int ret; > > > > - /* > > - * 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. > > - */ > > - tmp = dw_pcie_readl_dbi(pci, PCIE_RC_LCR); > > - tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK; > > - tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1; > > - dw_pcie_writel_dbi(pci, PCIE_RC_LCR, tmp); > > + if (!IS_ENABLED(CONFIG_PCI_IMX6_COMPLIANCE_TEST)) { > > + /* > > + * 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. > > + */ > > + tmp = dw_pcie_readl_dbi(pci, PCIE_RC_LCR); > > + tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK; > > + tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1; > > + dw_pcie_writel_dbi(pci, PCIE_RC_LCR, tmp); > > + } > > > > /* Start LTSSM. */ > > if (imx6_pcie->variant == IMX7D) > > -- > > 2.11.0 > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Monday, June 12, 2017 6:49:24 PM CEST Bjorn Helgaas wrote: > On Wed, Jun 07, 2017 at 01:36:11PM +0200, Stefan Schoefegger wrote: > > Link speed must not be limited to gen1 during link test for compliance > > tests > > > > Signed-off-by: Stefan Schoefegger <stefan.schoefegger@ginzinger.com> > > --- > > > > Changes since v1: > > - pci-imx6.c moved to dwc directory > > > > drivers/pci/dwc/Kconfig | 10 ++++++++++ > > drivers/pci/dwc/pci-imx6.c | 21 ++++++++++++--------- > > 2 files changed, 22 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/pci/dwc/Kconfig b/drivers/pci/dwc/Kconfig > > index b7e15526d676..b6e9ced5a45d 100644 > > --- a/drivers/pci/dwc/Kconfig > > +++ b/drivers/pci/dwc/Kconfig > > @@ -77,6 +77,16 @@ config PCI_IMX6 > > > > select PCIEPORTBUS > > select PCIE_DW_HOST > > > > +config PCI_IMX6_COMPLIANCE_TEST > > + bool "Enable pcie compliance tests on imx6" > > + depends on PCI_IMX6 > > + default n > > + help > > + Enables support for pcie compliance test on FSL iMX SoCs. > > + The link speed wouldn't be limited to gen1 when enabled. > > + Enable only during compliance tests, otherwise > > + link detection will fail on some peripherals. > > I'm puzzled about why we would want to merge this patch. It looks > like we're trying to game the system to make the device pass > compliance testing when it isn't really compliant. Is this config > option useful to users, or is it only useful during internal > development of iMX SoCs? It's not for passing compliance tests, it is necessary to do the compliance tests. Without this patch only gen 1 speed is possible to test. Also i.mx6 is not fully gen2 compliant (withour external clk) we should have the option to do gen2 tests. Switching from gen1 to gen2 is done with a 100MHz (1ms) clk pulse on the receiver. Without this patch link speed is forced to gen1 afterwards. Yes it is only useful for board setup, it is comparable to CONFIG_USB_EHSET_TEST_FIXTURE for usb (ok, this is more general and not host specific). > > > config PCIE_SPEAR13XX > > > > bool "STMicroelectronics SPEAr PCIe controller" > > depends on PCI > > > > diff --git a/drivers/pci/dwc/pci-imx6.c b/drivers/pci/dwc/pci-imx6.c > > index 19a289b8cc94..b0fbe52e25b0 100644 > > --- a/drivers/pci/dwc/pci-imx6.c > > +++ b/drivers/pci/dwc/pci-imx6.c > > @@ -533,15 +533,18 @@ static int imx6_pcie_establish_link(struct imx6_pcie > > *imx6_pcie)> > > u32 tmp; > > int ret; > > > > - /* > > - * 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. > > - */ > > - tmp = dw_pcie_readl_dbi(pci, PCIE_RC_LCR); > > - tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK; > > - tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1; > > - dw_pcie_writel_dbi(pci, PCIE_RC_LCR, tmp); > > + if (!IS_ENABLED(CONFIG_PCI_IMX6_COMPLIANCE_TEST)) { > > + /* > > + * 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. > > + */ > > + tmp = dw_pcie_readl_dbi(pci, PCIE_RC_LCR); > > + tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK; > > + tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1; > > + dw_pcie_writel_dbi(pci, PCIE_RC_LCR, tmp); > > + } > > > > /* Start LTSSM. */ > > if (imx6_pcie->variant == IMX7D)
On Tuesday, June 13, 2017 2:00:16 AM CEST Richard Zhu wrote: > > -----Original Message----- > > From: Bjorn Helgaas [mailto:helgaas@kernel.org] > > Sent: Tuesday, June 13, 2017 7:49 AM > > To: Stefan Schoefegger <stefan.schoefegger@ginzinger.com> > > Cc: linux-pci@vger.kernel.org; Richard Zhu <hongxing.zhu@nxp.com>; Arnd > > Bergmann <arnd@arndb.de>; open list <linux-kernel@vger.kernel.org>; > > Kishon Vijay Abraham I <kishon@ti.com>; Jingoo Han > > <jingoohan1@gmail.com>; Bjorn Helgaas <bhelgaas@google.com>; > > moderated list:PCI DRIVER FOR IMX6 <linux-arm-kernel@lists.infradead.org>; > > Lucas Stach <l.stach@pengutronix.de> > > Subject: Re: [PATCH v2 1/1] PCI: imx6: Add pcie compliance test option > > > > On Wed, Jun 07, 2017 at 01:36:11PM +0200, Stefan Schoefegger wrote: > > > Link speed must not be limited to gen1 during link test for compliance > > > tests > > > > > > Signed-off-by: Stefan Schoefegger <stefan.schoefegger@ginzinger.com> > > > --- > > > > > > Changes since v1: > > > - pci-imx6.c moved to dwc directory > > > > > > drivers/pci/dwc/Kconfig | 10 ++++++++++ > > > drivers/pci/dwc/pci-imx6.c | 21 ++++++++++++--------- > > > 2 files changed, 22 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/pci/dwc/Kconfig b/drivers/pci/dwc/Kconfig index > > > b7e15526d676..b6e9ced5a45d 100644 > > > --- a/drivers/pci/dwc/Kconfig > > > +++ b/drivers/pci/dwc/Kconfig > > > @@ -77,6 +77,16 @@ config PCI_IMX6 > > > > > > select PCIEPORTBUS > > > select PCIE_DW_HOST > > > > > > +config PCI_IMX6_COMPLIANCE_TEST > > > + bool "Enable pcie compliance tests on imx6" > > > + depends on PCI_IMX6 > > > + default n > > > + help > > > + Enables support for pcie compliance test on FSL iMX SoCs. > > > + The link speed wouldn't be limited to gen1 when enabled. > > > + Enable only during compliance tests, otherwise > > > + link detection will fail on some peripherals. > > > > I'm puzzled about why we would want to merge this patch. It looks like > > we're trying to game the system to make the device pass compliance testing > > when it isn't really compliant. Is this config option useful to users, or > > is it only useful during internal development of iMX SoCs? > > [Zhu hongxing] Agree with Bjorn. These modifications shouldn't be merged. > They are used for the boards used to pass the PCIe RC certification, when > one Standalone external OSC is used as PCIe reference clock source in the > boards design. Yes, passing gen2 compliance test is only possible with external clk. But this is a errata of imx6 pci phy of PCIe clk jitter. Why should we not be able to do gen2 tests? I think it is useful to do other gen2 tests (signal integrity) during board design. > > > config PCIE_SPEAR13XX > > > > > > bool "STMicroelectronics SPEAr PCIe controller" > > > depends on PCI > > > > > > diff --git a/drivers/pci/dwc/pci-imx6.c b/drivers/pci/dwc/pci-imx6.c > > > index 19a289b8cc94..b0fbe52e25b0 100644 > > > --- a/drivers/pci/dwc/pci-imx6.c > > > +++ b/drivers/pci/dwc/pci-imx6.c > > > @@ -533,15 +533,18 @@ static int imx6_pcie_establish_link(struct > > > > imx6_pcie *imx6_pcie) > > > > > u32 tmp; > > > int ret; > > > > > > - /* > > > - * 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. > > > - */ > > > - tmp = dw_pcie_readl_dbi(pci, PCIE_RC_LCR); > > > - tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK; > > > - tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1; > > > - dw_pcie_writel_dbi(pci, PCIE_RC_LCR, tmp); > > > + if (!IS_ENABLED(CONFIG_PCI_IMX6_COMPLIANCE_TEST)) { > > > + /* > > > + * 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. > > > + */ > > > + tmp = dw_pcie_readl_dbi(pci, PCIE_RC_LCR); > > > + tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK; > > > + tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1; > > > + dw_pcie_writel_dbi(pci, PCIE_RC_LCR, tmp); > > > + } > > > > > > /* Start LTSSM. */ > > > if (imx6_pcie->variant == IMX7D) > > > > > > -- > > > 2.11.0 > > > > > > > > > _______________________________________________ > > > linux-arm-kernel mailing list > > > linux-arm-kernel@lists.infradead.org > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Jun 13, 2017 at 05:43:14AM +0000, Schöfegger Stefan wrote: > On Monday, June 12, 2017 6:49:24 PM CEST Bjorn Helgaas wrote: > > On Wed, Jun 07, 2017 at 01:36:11PM +0200, Stefan Schoefegger wrote: > > > Link speed must not be limited to gen1 during link test for compliance > > > tests > > > > > > Signed-off-by: Stefan Schoefegger <stefan.schoefegger@ginzinger.com> > > > --- > > > > > > Changes since v1: > > > - pci-imx6.c moved to dwc directory > > > > > > drivers/pci/dwc/Kconfig | 10 ++++++++++ > > > drivers/pci/dwc/pci-imx6.c | 21 ++++++++++++--------- > > > 2 files changed, 22 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/pci/dwc/Kconfig b/drivers/pci/dwc/Kconfig > > > index b7e15526d676..b6e9ced5a45d 100644 > > > --- a/drivers/pci/dwc/Kconfig > > > +++ b/drivers/pci/dwc/Kconfig > > > @@ -77,6 +77,16 @@ config PCI_IMX6 > > > > > > select PCIEPORTBUS > > > select PCIE_DW_HOST > > > > > > +config PCI_IMX6_COMPLIANCE_TEST > > > + bool "Enable pcie compliance tests on imx6" > > > + depends on PCI_IMX6 > > > + default n > > > + help > > > + Enables support for pcie compliance test on FSL iMX SoCs. > > > + The link speed wouldn't be limited to gen1 when enabled. > > > + Enable only during compliance tests, otherwise > > > + link detection will fail on some peripherals. > > > > I'm puzzled about why we would want to merge this patch. It looks > > like we're trying to game the system to make the device pass > > compliance testing when it isn't really compliant. Is this config > > option useful to users, or is it only useful during internal > > development of iMX SoCs? > > It's not for passing compliance tests, it is necessary to do the compliance > tests. Without this patch only gen 1 speed is possible to test. Also i.mx6 is > not fully gen2 compliant (withour external clk) we should have the option to > do gen2 tests. Switching from gen1 to gen2 is done with a 100MHz (1ms) clk > pulse on the receiver. Without this patch link speed is forced to gen1 > afterwards. I don't understand the purpose of this yet, so maybe all we need is a better description. "It's not for passing compliance testing, it is necessary to do the compliance tests" doesn't make sense to me -- it seems self-contradictory. The Kconfig text says "enable only for testing because it makes link detection fail." To me that means this option is not useful for users. We need some justification for why it should be in the mainline kernel, where users and distros may enable it. If you can only support gen2 in certain board configurations, maybe you should add a config option that can always be enabled for those boards. > Yes it is only useful for board setup, it is comparable to > CONFIG_USB_EHSET_TEST_FIXTURE for usb (ok, this is more general and not host > specific). USB_EHSET_TEST_FIXTURE (added by 1353aa53851e ("usb: misc: EHSET Test Fixture device driver for host compliance")) looks like just another driver in the sense that it's always safe to enable it and it doesn't hurt anything if you enable it without having the hardware. This patch doesn't seem comparable to USB_EHSET_TEST_FIXTURE because this new option apparently breaks link detection in some cases. > > > config PCIE_SPEAR13XX > > > > > > bool "STMicroelectronics SPEAr PCIe controller" > > > depends on PCI > > > > > > diff --git a/drivers/pci/dwc/pci-imx6.c b/drivers/pci/dwc/pci-imx6.c > > > index 19a289b8cc94..b0fbe52e25b0 100644 > > > --- a/drivers/pci/dwc/pci-imx6.c > > > +++ b/drivers/pci/dwc/pci-imx6.c > > > @@ -533,15 +533,18 @@ static int imx6_pcie_establish_link(struct imx6_pcie > > > *imx6_pcie)> > > > u32 tmp; > > > int ret; > > > > > > - /* > > > - * 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. > > > - */ > > > - tmp = dw_pcie_readl_dbi(pci, PCIE_RC_LCR); > > > - tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK; > > > - tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1; > > > - dw_pcie_writel_dbi(pci, PCIE_RC_LCR, tmp); > > > + if (!IS_ENABLED(CONFIG_PCI_IMX6_COMPLIANCE_TEST)) { > > > + /* > > > + * 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. > > > + */ > > > + tmp = dw_pcie_readl_dbi(pci, PCIE_RC_LCR); > > > + tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK; > > > + tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1; > > > + dw_pcie_writel_dbi(pci, PCIE_RC_LCR, tmp); > > > + } > > > > > > /* Start LTSSM. */ > > > if (imx6_pcie->variant == IMX7D) >
On Tuesday, June 13, 2017 8:58:47 AM CEST Bjorn Helgaas wrote: > On Tue, Jun 13, 2017 at 05:43:14AM +0000, Schöfegger Stefan wrote: > > On Monday, June 12, 2017 6:49:24 PM CEST Bjorn Helgaas wrote: > > > On Wed, Jun 07, 2017 at 01:36:11PM +0200, Stefan Schoefegger wrote: > > > > Link speed must not be limited to gen1 during link test for compliance > > > > tests > > > > > > > > Signed-off-by: Stefan Schoefegger <stefan.schoefegger@ginzinger.com> > > > > --- > > > > > > > > Changes since v1: > > > > - pci-imx6.c moved to dwc directory > > > > > > > > drivers/pci/dwc/Kconfig | 10 ++++++++++ > > > > drivers/pci/dwc/pci-imx6.c | 21 ++++++++++++--------- > > > > 2 files changed, 22 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/drivers/pci/dwc/Kconfig b/drivers/pci/dwc/Kconfig > > > > index b7e15526d676..b6e9ced5a45d 100644 > > > > --- a/drivers/pci/dwc/Kconfig > > > > +++ b/drivers/pci/dwc/Kconfig > > > > @@ -77,6 +77,16 @@ config PCI_IMX6 > > > > > > > > select PCIEPORTBUS > > > > select PCIE_DW_HOST > > > > > > > > +config PCI_IMX6_COMPLIANCE_TEST > > > > + bool "Enable pcie compliance tests on imx6" > > > > + depends on PCI_IMX6 > > > > + default n > > > > + help > > > > + Enables support for pcie compliance test on FSL iMX SoCs. > > > > + The link speed wouldn't be limited to gen1 when enabled. > > > > + Enable only during compliance tests, otherwise > > > > + link detection will fail on some peripherals. > > > > > > I'm puzzled about why we would want to merge this patch. It looks > > > like we're trying to game the system to make the device pass > > > compliance testing when it isn't really compliant. Is this config > > > option useful to users, or is it only useful during internal > > > development of iMX SoCs? > > > > It's not for passing compliance tests, it is necessary to do the > > compliance > > tests. Without this patch only gen 1 speed is possible to test. Also i.mx6 > > is not fully gen2 compliant (withour external clk) we should have the > > option to do gen2 tests. Switching from gen1 to gen2 is done with a > > 100MHz (1ms) clk pulse on the receiver. Without this patch link speed is > > forced to gen1 afterwards. > > I don't understand the purpose of this yet, so maybe all we need is a > better description. This patch enabled compliance tests for pcie gen2 published by pci-sig. (Signal level, signal timing, jitter and rise/fall times on tx lines). Special hardware is needed to do this tests (compliance load board, oszilloscope with about 10GHz bandwith). The pcie phy switches in a complinace test state where the phy outputs a special test pattern (without having a real link to a device). The bitrate and de-emphasis must be switched. The driver (without this patches) does not allow to switch to gen2 because it falls back to gen1. It is impossible to generate the gen2 test pattern. The patch now removes the forced gen1 start to allow generating gen2 test pattern. gen1 / gen2 switch is done through signals generated on the compliance load board (which triggers switching in the phy). > > "It's not for passing compliance testing, it is necessary to do the > compliance tests" doesn't make sense to me -- it seems > self-contradictory. > > The Kconfig text says "enable only for testing because it makes link > detection fail." To me that means this option is not useful for > users. We need some justification for why it should be in the > mainline kernel, where users and distros may enable it. It use useless for users and distros. Only board designer want this option. If this is not a reason for applying it's ok for me. > > If you can only support gen2 in certain board configurations, maybe > you should add a config option that can always be enabled for those > boards. > > > Yes it is only useful for board setup, it is comparable to > > CONFIG_USB_EHSET_TEST_FIXTURE for usb (ok, this is more general and not > > host specific). > > USB_EHSET_TEST_FIXTURE (added by 1353aa53851e ("usb: misc: EHSET Test > Fixture device driver for host compliance")) looks like just another > driver in the sense that it's always safe to enable it and it doesn't > hurt anything if you enable it without having the hardware. > > This patch doesn't seem comparable to USB_EHSET_TEST_FIXTURE because > this new option apparently breaks link detection in some cases. This depends on the point of view (Black box vs. white box) :-) Both are for doing compliance tests on signal level. But I think we don't need to discuss this further. > > > > > config PCIE_SPEAR13XX > > > > > > > > bool "STMicroelectronics SPEAr PCIe controller" > > > > depends on PCI > > > > > > > > diff --git a/drivers/pci/dwc/pci-imx6.c b/drivers/pci/dwc/pci-imx6.c > > > > index 19a289b8cc94..b0fbe52e25b0 100644 > > > > --- a/drivers/pci/dwc/pci-imx6.c > > > > +++ b/drivers/pci/dwc/pci-imx6.c > > > > @@ -533,15 +533,18 @@ static int imx6_pcie_establish_link(struct > > > > imx6_pcie > > > > *imx6_pcie)> > > > > > > > > u32 tmp; > > > > int ret; > > > > > > > > - /* > > > > - * 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. > > > > - */ > > > > - tmp = dw_pcie_readl_dbi(pci, PCIE_RC_LCR); > > > > - tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK; > > > > - tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1; > > > > - dw_pcie_writel_dbi(pci, PCIE_RC_LCR, tmp); > > > > + if (!IS_ENABLED(CONFIG_PCI_IMX6_COMPLIANCE_TEST)) { > > > > + /* > > > > + * 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. > > > > + */ > > > > + tmp = dw_pcie_readl_dbi(pci, PCIE_RC_LCR); > > > > + tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK; > > > > + tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1; > > > > + dw_pcie_writel_dbi(pci, PCIE_RC_LCR, tmp); > > > > + } > > > > > > > > /* Start LTSSM. */ > > > > if (imx6_pcie->variant == IMX7D)
On Wed, Jun 14, 2017 at 2:52 AM, Schöfegger Stefan <Stefan.Schoefegger@ginzinger.com> wrote: > device). The bitrate and de-emphasis must be switched. The driver (without > this patches) does not allow to switch to gen2 because it falls back to gen1. > It is impossible to generate the gen2 test pattern. If you pass 'fsl,max-link-speed = <2>;' in your device tree, then it will allow gen2. From Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt: "- fsl,max-link-speed: Specify PCI gen for link capability. Must be '2' for gen2, otherwise will default to gen1. Note that the IMX6 LVDS clock outputs do not meet gen2 jitter requirements and thus for gen2 capability a gen2 compliant clock generator should be used and configured." Wouldn't this solve the problem?
On Wednesday, June 14, 2017 4:11:29 PM CEST Fabio Estevam wrote: > On Wed, Jun 14, 2017 at 2:52 AM, Schöfegger Stefan > > <Stefan.Schoefegger@ginzinger.com> wrote: > > device). The bitrate and de-emphasis must be switched. The driver (without > > this patches) does not allow to switch to gen2 because it falls back to > > gen1. It is impossible to generate the gen2 test pattern. > > If you pass 'fsl,max-link-speed = <2>;' in your device tree, then it > will allow gen2. > > From Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt: > > "- fsl,max-link-speed: Specify PCI gen for link capability. Must be '2' for > gen2, otherwise will default to gen1. Note that the IMX6 LVDS clock > outputs do not meet gen2 jitter requirements and thus for gen2 capability a > gen2 compliant clock generator should be used and configured." > > Wouldn't this solve the problem? No, it only sets the maximum allowed link speed but it is forced to gen1 before. During compliance test there is no real link established and there is no link speed negotiation, the phy is in a special compliance test state. Stefan
diff --git a/drivers/pci/dwc/Kconfig b/drivers/pci/dwc/Kconfig index b7e15526d676..b6e9ced5a45d 100644 --- a/drivers/pci/dwc/Kconfig +++ b/drivers/pci/dwc/Kconfig @@ -77,6 +77,16 @@ config PCI_IMX6 select PCIEPORTBUS select PCIE_DW_HOST +config PCI_IMX6_COMPLIANCE_TEST + bool "Enable pcie compliance tests on imx6" + depends on PCI_IMX6 + default n + help + Enables support for pcie compliance test on FSL iMX SoCs. + The link speed wouldn't be limited to gen1 when enabled. + Enable only during compliance tests, otherwise + link detection will fail on some peripherals. + config PCIE_SPEAR13XX bool "STMicroelectronics SPEAr PCIe controller" depends on PCI diff --git a/drivers/pci/dwc/pci-imx6.c b/drivers/pci/dwc/pci-imx6.c index 19a289b8cc94..b0fbe52e25b0 100644 --- a/drivers/pci/dwc/pci-imx6.c +++ b/drivers/pci/dwc/pci-imx6.c @@ -533,15 +533,18 @@ static int imx6_pcie_establish_link(struct imx6_pcie *imx6_pcie) u32 tmp; int ret; - /* - * 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. - */ - tmp = dw_pcie_readl_dbi(pci, PCIE_RC_LCR); - tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK; - tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1; - dw_pcie_writel_dbi(pci, PCIE_RC_LCR, tmp); + if (!IS_ENABLED(CONFIG_PCI_IMX6_COMPLIANCE_TEST)) { + /* + * 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. + */ + tmp = dw_pcie_readl_dbi(pci, PCIE_RC_LCR); + tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK; + tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1; + dw_pcie_writel_dbi(pci, PCIE_RC_LCR, tmp); + } /* Start LTSSM. */ if (imx6_pcie->variant == IMX7D)
Link speed must not be limited to gen1 during link test for compliance tests Signed-off-by: Stefan Schoefegger <stefan.schoefegger@ginzinger.com> --- Changes since v1: - pci-imx6.c moved to dwc directory drivers/pci/dwc/Kconfig | 10 ++++++++++ drivers/pci/dwc/pci-imx6.c | 21 ++++++++++++--------- 2 files changed, 22 insertions(+), 9 deletions(-)