diff for duplicates of <1923573.HjpiSnITgW@en-pc05> diff --git a/a/1.txt b/N1/1.txt index ca231b7..10bcb69 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,31 +1,29 @@ On Tuesday, June 13, 2017 8:58:47 AM CEST Bjorn Helgaas wrote: -> On Tue, Jun 13, 2017 at 05:43:14AM +0000, Sch=F6fegger Stefan 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 complia= -nce +> > > > Link speed must not be limited to gen1 during link test for compliance > > > > tests -> > > >=20 -> > > > Signed-off-by: Stefan Schoefegger <stefan.schoefegger@ginzinger.com= -> +> > > > +> > > > Signed-off-by: Stefan Schoefegger <stefan.schoefegger@ginzinger.com> > > > > --- -> > > >=20 +> > > > > > > > Changes since v1: > > > > - pci-imx6.c moved to dwc directory -> > > > =20 +> > > > > > > > drivers/pci/dwc/Kconfig | 10 ++++++++++ > > > > drivers/pci/dwc/pci-imx6.c | 21 ++++++++++++--------- > > > > 2 files changed, 22 insertions(+), 9 deletions(-) -> > > >=20 +> > > > > > > > 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 -> > > >=20 +> > > > > > > > select PCIEPORTBUS > > > > select PCIE_DW_HOST -> > > >=20 +> > > > > > > > +config PCI_IMX6_COMPLIANCE_TEST > > > > + bool "Enable pcie compliance tests on imx6" > > > > + depends on PCI_IMX6 @@ -35,106 +33,93 @@ nce > > > > + The link speed wouldn't be limited to gen1 when enabled. > > > > + Enable only during compliance tests, otherwise > > > > + link detection will fail on some peripherals. -> > >=20 +> > > > > > 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? -> >=20 +> > > > 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 +> > 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 i= -s +> > 100MHz (1ms) clk pulse on the receiver. Without this patch link speed is > > forced to gen1 afterwards. ->=20 +> > 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.=20 -(Signal level, signal timing, jitter and rise/fall times on tx lines). Spec= -ial=20 -hardware is needed to do this tests (compliance load board, oszilloscope wi= -th=20 -about 10GHz bandwith). The pcie phy switches in a complinace test state whe= -re=20 -the phy outputs a special test pattern (without having a real link to a=20 -device). The bitrate and de-emphasis must be switched. The driver (without= -=20 -this patches) does not allow to switch to gen2 because it falls back to gen= -1.=20 +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=20 -pattern. gen1 / gen2 switch is done through signals generated on the=20 +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). ->=20 +> > "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. ->=20 +> > 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=20 +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. ->=20 +> > 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. ->=20 +> > > 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). ->=20 +> > 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. ->=20 +> > 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 fo= -r=20 -doing compliance tests on signal level. But I think we don't need to discus= -s=20 +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. ->=20 +> > > > > config PCIE_SPEAR13XX -> > > > =20 +> > > > > > > > bool "STMicroelectronics SPEAr PCIe controller" > > > > depends on PCI -> > > >=20 -> > > > diff --git a/drivers/pci/dwc/pci-imx6.c b/drivers/pci/dwc/pci-imx6.= -c +> > > > +> > > > 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)> -> > > >=20 +> > > > > > > > u32 tmp; > > > > int ret; -> > > >=20 +> > > > > > > > - /* -> > > > - * Force Gen1 operation when starting the link. In case the link= - is -> > > > - * started in Gen2 mode, there is a possibility the devices on th= -e +> > > > - * 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 =3D dw_pcie_readl_dbi(pci, PCIE_RC_LCR); -> > > > - tmp &=3D ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK; -> > > > - tmp |=3D PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1; +> > > > - 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)) { > > > > + /* @@ -143,11 +128,11 @@ e > > > > + * devices on the bus will not be detected at all. This > > > > + * happens with PCIe switches. > > > > + */ -> > > > + tmp =3D dw_pcie_readl_dbi(pci, PCIE_RC_LCR); -> > > > + tmp &=3D ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK; -> > > > + tmp |=3D PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1; +> > > > + 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); > > > > + } -> > > >=20 +> > > > > > > > /* Start LTSSM. */ -> > > > if (imx6_pcie->variant =3D=3D IMX7D) +> > > > if (imx6_pcie->variant == IMX7D) diff --git a/a/content_digest b/N1/content_digest index 8e1ac6d..314ebc1 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -1,49 +1,38 @@ "ref\01472121518-9340-1-git-send-email-stefan.schoefegger@ginzinger.com\0" "ref\02668097.SlerkO2uVy@en-pc05\0" "ref\020170613135847.GA7128@bhelgaas-glaptop.roam.corp.google.com\0" - "From\0Sch\303\266fegger Stefan <Stefan.Schoefegger@ginzinger.com>\0" - "Subject\0Re: [PATCH v2 1/1] PCI: imx6: Add pcie compliance test option\0" + "From\0Stefan.Schoefegger@ginzinger.com (Sch\303\266fegger Stefan)\0" + "Subject\0[PATCH v2 1/1] PCI: imx6: Add pcie compliance test option\0" "Date\0Wed, 14 Jun 2017 05:52:58 +0000\0" - "To\0Bjorn Helgaas <helgaas@kernel.org>\0" - "Cc\0linux-pci@vger.kernel.org <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>\0" + "To\0linux-arm-kernel@lists.infradead.org\0" "\00:1\0" "b\0" "On Tuesday, June 13, 2017 8:58:47 AM CEST Bjorn Helgaas wrote:\n" - "> On Tue, Jun 13, 2017 at 05:43:14AM +0000, Sch=F6fegger Stefan wrote:\n" + "> On Tue, Jun 13, 2017 at 05:43:14AM +0000, Sch?fegger Stefan wrote:\n" "> > On Monday, June 12, 2017 6:49:24 PM CEST Bjorn Helgaas wrote:\n" "> > > On Wed, Jun 07, 2017 at 01:36:11PM +0200, Stefan Schoefegger wrote:\n" - "> > > > Link speed must not be limited to gen1 during link test for complia=\n" - "nce\n" + "> > > > Link speed must not be limited to gen1 during link test for compliance\n" "> > > > tests\n" - "> > > >=20\n" - "> > > > Signed-off-by: Stefan Schoefegger <stefan.schoefegger@ginzinger.com=\n" - ">\n" + "> > > > \n" + "> > > > Signed-off-by: Stefan Schoefegger <stefan.schoefegger@ginzinger.com>\n" "> > > > ---\n" - "> > > >=20\n" + "> > > > \n" "> > > > Changes since v1:\n" "> > > > - pci-imx6.c moved to dwc directory\n" - "> > > > =20\n" + "> > > > \n" "> > > > drivers/pci/dwc/Kconfig | 10 ++++++++++\n" "> > > > drivers/pci/dwc/pci-imx6.c | 21 ++++++++++++---------\n" "> > > > 2 files changed, 22 insertions(+), 9 deletions(-)\n" - "> > > >=20\n" + "> > > > \n" "> > > > diff --git a/drivers/pci/dwc/Kconfig b/drivers/pci/dwc/Kconfig\n" "> > > > index b7e15526d676..b6e9ced5a45d 100644\n" "> > > > --- a/drivers/pci/dwc/Kconfig\n" "> > > > +++ b/drivers/pci/dwc/Kconfig\n" "> > > > @@ -77,6 +77,16 @@ config PCI_IMX6\n" - "> > > >=20\n" + "> > > > \n" "> > > > \tselect PCIEPORTBUS\n" "> > > > \tselect PCIE_DW_HOST\n" - "> > > >=20\n" + "> > > > \n" "> > > > +config PCI_IMX6_COMPLIANCE_TEST\n" "> > > > +\tbool \"Enable pcie compliance tests on imx6\"\n" "> > > > +\tdepends on PCI_IMX6\n" @@ -53,106 +42,93 @@ "> > > > +\t The link speed wouldn't be limited to gen1 when enabled.\n" "> > > > +\t Enable only during compliance tests, otherwise\n" "> > > > +\t link detection will fail on some peripherals.\n" - "> > >=20\n" + "> > > \n" "> > > I'm puzzled about why we would want to merge this patch. It looks\n" "> > > like we're trying to game the system to make the device pass\n" "> > > compliance testing when it isn't really compliant. Is this config\n" "> > > option useful to users, or is it only useful during internal\n" "> > > development of iMX SoCs?\n" - "> >=20\n" + "> > \n" "> > It's not for passing compliance tests, it is necessary to do the\n" "> > compliance\n" - "> > tests. Without this patch only gen 1 speed is possible to test. Also i.=\n" - "mx6\n" + "> > tests. Without this patch only gen 1 speed is possible to test. Also i.mx6\n" "> > is not fully gen2 compliant (withour external clk) we should have the\n" "> > option to do gen2 tests. Switching from gen1 to gen2 is done with a\n" - "> > 100MHz (1ms) clk pulse on the receiver. Without this patch link speed i=\n" - "s\n" + "> > 100MHz (1ms) clk pulse on the receiver. Without this patch link speed is\n" "> > forced to gen1 afterwards.\n" - ">=20\n" + "> \n" "> I don't understand the purpose of this yet, so maybe all we need is a\n" "> better description.\n" "\n" - "This patch enabled compliance tests for pcie gen2 published by pci-sig.=20\n" - "(Signal level, signal timing, jitter and rise/fall times on tx lines). Spec=\n" - "ial=20\n" - "hardware is needed to do this tests (compliance load board, oszilloscope wi=\n" - "th=20\n" - "about 10GHz bandwith). The pcie phy switches in a complinace test state whe=\n" - "re=20\n" - "the phy outputs a special test pattern (without having a real link to a=20\n" - "device). The bitrate and de-emphasis must be switched. The driver (without=\n" - "=20\n" - "this patches) does not allow to switch to gen2 because it falls back to gen=\n" - "1.=20\n" + "This patch enabled compliance tests for pcie gen2 published by pci-sig. \n" + "(Signal level, signal timing, jitter and rise/fall times on tx lines). Special \n" + "hardware is needed to do this tests (compliance load board, oszilloscope with \n" + "about 10GHz bandwith). The pcie phy switches in a complinace test state where \n" + "the phy outputs a special test pattern (without having a real link to a \n" + "device). The bitrate and de-emphasis must be switched. The driver (without \n" + "this patches) does not allow to switch to gen2 because it falls back to gen1. \n" "It is impossible to generate the gen2 test pattern.\n" - "The patch now removes the forced gen1 start to allow generating gen2 test=20\n" - "pattern. gen1 / gen2 switch is done through signals generated on the=20\n" + "The patch now removes the forced gen1 start to allow generating gen2 test \n" + "pattern. gen1 / gen2 switch is done through signals generated on the \n" "compliance load board (which triggers switching in the phy).\n" - ">=20\n" + "> \n" "> \"It's not for passing compliance testing, it is necessary to do the\n" "> compliance tests\" doesn't make sense to me -- it seems\n" "> self-contradictory.\n" - ">=20\n" + "> \n" "> The Kconfig text says \"enable only for testing because it makes link\n" "> detection fail.\" To me that means this option is not useful for\n" "> users. We need some justification for why it should be in the\n" "> mainline kernel, where users and distros may enable it.\n" "\n" - "It use useless for users and distros. Only board designer want this option.=\n" - " If=20\n" + "It use useless for users and distros. Only board designer want this option. If \n" "this is not a reason for applying it's ok for me.\n" - ">=20\n" + "> \n" "> If you can only support gen2 in certain board configurations, maybe\n" "> you should add a config option that can always be enabled for those\n" "> boards.\n" - ">=20\n" + "> \n" "> > Yes it is only useful for board setup, it is comparable to\n" "> > CONFIG_USB_EHSET_TEST_FIXTURE for usb (ok, this is more general and not\n" "> > host specific).\n" - ">=20\n" + "> \n" "> USB_EHSET_TEST_FIXTURE (added by 1353aa53851e (\"usb: misc: EHSET Test\n" "> Fixture device driver for host compliance\")) looks like just another\n" "> driver in the sense that it's always safe to enable it and it doesn't\n" "> hurt anything if you enable it without having the hardware.\n" - ">=20\n" + "> \n" "> This patch doesn't seem comparable to USB_EHSET_TEST_FIXTURE because\n" "> this new option apparently breaks link detection in some cases.\n" "\n" - "This depends on the point of view (Black box vs. white box) :-) Both are fo=\n" - "r=20\n" - "doing compliance tests on signal level. But I think we don't need to discus=\n" - "s=20\n" + "This depends on the point of view (Black box vs. white box) :-) Both are for \n" + "doing compliance tests on signal level. But I think we don't need to discuss \n" "this further.\n" - ">=20\n" + "> \n" "> > > > config PCIE_SPEAR13XX\n" - "> > > > =20\n" + "> > > > \n" "> > > > \tbool \"STMicroelectronics SPEAr PCIe controller\"\n" "> > > > \tdepends on PCI\n" - "> > > >=20\n" - "> > > > diff --git a/drivers/pci/dwc/pci-imx6.c b/drivers/pci/dwc/pci-imx6.=\n" - "c\n" + "> > > > \n" + "> > > > diff --git a/drivers/pci/dwc/pci-imx6.c b/drivers/pci/dwc/pci-imx6.c\n" "> > > > index 19a289b8cc94..b0fbe52e25b0 100644\n" "> > > > --- a/drivers/pci/dwc/pci-imx6.c\n" "> > > > +++ b/drivers/pci/dwc/pci-imx6.c\n" "> > > > @@ -533,15 +533,18 @@ static int imx6_pcie_establish_link(struct\n" "> > > > imx6_pcie\n" "> > > > *imx6_pcie)>\n" - "> > > >=20\n" + "> > > > \n" "> > > > \tu32 tmp;\n" "> > > > \tint ret;\n" - "> > > >=20\n" + "> > > > \n" "> > > > -\t/*\n" - "> > > > -\t * Force Gen1 operation when starting the link. In case the link=\n" - " is\n" - "> > > > -\t * started in Gen2 mode, there is a possibility the devices on th=\n" - "e\n" + "> > > > -\t * Force Gen1 operation when starting the link. In case the link is\n" + "> > > > -\t * started in Gen2 mode, there is a possibility the devices on the\n" "> > > > -\t * bus will not be detected at all. This happens with PCIe\n" "> > > > switches.\n" "> > > > -\t */\n" - "> > > > -\ttmp =3D dw_pcie_readl_dbi(pci, PCIE_RC_LCR);\n" - "> > > > -\ttmp &=3D ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK;\n" - "> > > > -\ttmp |=3D PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1;\n" + "> > > > -\ttmp = dw_pcie_readl_dbi(pci, PCIE_RC_LCR);\n" + "> > > > -\ttmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK;\n" + "> > > > -\ttmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1;\n" "> > > > -\tdw_pcie_writel_dbi(pci, PCIE_RC_LCR, tmp);\n" "> > > > +\tif (!IS_ENABLED(CONFIG_PCI_IMX6_COMPLIANCE_TEST)) {\n" "> > > > +\t\t/*\n" @@ -161,13 +137,13 @@ "> > > > +\t\t * devices on the bus will not be detected at all. This\n" "> > > > +\t\t * happens with PCIe switches.\n" "> > > > +\t\t */\n" - "> > > > +\t\ttmp =3D dw_pcie_readl_dbi(pci, PCIE_RC_LCR);\n" - "> > > > +\t\ttmp &=3D ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK;\n" - "> > > > +\t\ttmp |=3D PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1;\n" + "> > > > +\t\ttmp = dw_pcie_readl_dbi(pci, PCIE_RC_LCR);\n" + "> > > > +\t\ttmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK;\n" + "> > > > +\t\ttmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1;\n" "> > > > +\t\tdw_pcie_writel_dbi(pci, PCIE_RC_LCR, tmp);\n" "> > > > +\t}\n" - "> > > >=20\n" + "> > > > \n" "> > > > \t/* Start LTSSM. */\n" - "> > > > \tif (imx6_pcie->variant =3D=3D IMX7D)" + "> > > > \tif (imx6_pcie->variant == IMX7D)" -40f7621b5bb4caa387bd132dc84dd3cf2a2221e7d4bc8a0cbe3d804b5de47d3a +05dfb2f9d64b808a0c439c422913295d2702a2d50bfa11b99278ba6db4fe51f7
diff --git a/a/1.txt b/N2/1.txt index ca231b7..777117e 100644 --- a/a/1.txt +++ b/N2/1.txt @@ -1,31 +1,29 @@ On Tuesday, June 13, 2017 8:58:47 AM CEST Bjorn Helgaas wrote: -> On Tue, Jun 13, 2017 at 05:43:14AM +0000, Sch=F6fegger Stefan 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 complia= -nce +> > > > Link speed must not be limited to gen1 during link test for compliance > > > > tests -> > > >=20 -> > > > Signed-off-by: Stefan Schoefegger <stefan.schoefegger@ginzinger.com= -> +> > > > +> > > > Signed-off-by: Stefan Schoefegger <stefan.schoefegger@ginzinger.com> > > > > --- -> > > >=20 +> > > > > > > > Changes since v1: > > > > - pci-imx6.c moved to dwc directory -> > > > =20 +> > > > > > > > drivers/pci/dwc/Kconfig | 10 ++++++++++ > > > > drivers/pci/dwc/pci-imx6.c | 21 ++++++++++++--------- > > > > 2 files changed, 22 insertions(+), 9 deletions(-) -> > > >=20 +> > > > > > > > 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 -> > > >=20 +> > > > > > > > select PCIEPORTBUS > > > > select PCIE_DW_HOST -> > > >=20 +> > > > > > > > +config PCI_IMX6_COMPLIANCE_TEST > > > > + bool "Enable pcie compliance tests on imx6" > > > > + depends on PCI_IMX6 @@ -35,106 +33,93 @@ nce > > > > + The link speed wouldn't be limited to gen1 when enabled. > > > > + Enable only during compliance tests, otherwise > > > > + link detection will fail on some peripherals. -> > >=20 +> > > > > > 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? -> >=20 +> > > > 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 +> > 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 i= -s +> > 100MHz (1ms) clk pulse on the receiver. Without this patch link speed is > > forced to gen1 afterwards. ->=20 +> > 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.=20 -(Signal level, signal timing, jitter and rise/fall times on tx lines). Spec= -ial=20 -hardware is needed to do this tests (compliance load board, oszilloscope wi= -th=20 -about 10GHz bandwith). The pcie phy switches in a complinace test state whe= -re=20 -the phy outputs a special test pattern (without having a real link to a=20 -device). The bitrate and de-emphasis must be switched. The driver (without= -=20 -this patches) does not allow to switch to gen2 because it falls back to gen= -1.=20 +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=20 -pattern. gen1 / gen2 switch is done through signals generated on the=20 +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). ->=20 +> > "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. ->=20 +> > 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=20 +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. ->=20 +> > 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. ->=20 +> > > 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). ->=20 +> > 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. ->=20 +> > 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 fo= -r=20 -doing compliance tests on signal level. But I think we don't need to discus= -s=20 +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. ->=20 +> > > > > config PCIE_SPEAR13XX -> > > > =20 +> > > > > > > > bool "STMicroelectronics SPEAr PCIe controller" > > > > depends on PCI -> > > >=20 -> > > > diff --git a/drivers/pci/dwc/pci-imx6.c b/drivers/pci/dwc/pci-imx6.= -c +> > > > +> > > > 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)> -> > > >=20 +> > > > > > > > u32 tmp; > > > > int ret; -> > > >=20 +> > > > > > > > - /* -> > > > - * Force Gen1 operation when starting the link. In case the link= - is -> > > > - * started in Gen2 mode, there is a possibility the devices on th= -e +> > > > - * 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 =3D dw_pcie_readl_dbi(pci, PCIE_RC_LCR); -> > > > - tmp &=3D ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK; -> > > > - tmp |=3D PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1; +> > > > - 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)) { > > > > + /* @@ -143,11 +128,11 @@ e > > > > + * devices on the bus will not be detected at all. This > > > > + * happens with PCIe switches. > > > > + */ -> > > > + tmp =3D dw_pcie_readl_dbi(pci, PCIE_RC_LCR); -> > > > + tmp &=3D ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK; -> > > > + tmp |=3D PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1; +> > > > + 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); > > > > + } -> > > >=20 +> > > > > > > > /* Start LTSSM. */ -> > > > if (imx6_pcie->variant =3D=3D IMX7D) +> > > > if (imx6_pcie->variant == IMX7D) diff --git a/a/content_digest b/N2/content_digest index 8e1ac6d..954e961 100644 --- a/a/content_digest +++ b/N2/content_digest @@ -17,33 +17,31 @@ "\00:1\0" "b\0" "On Tuesday, June 13, 2017 8:58:47 AM CEST Bjorn Helgaas wrote:\n" - "> On Tue, Jun 13, 2017 at 05:43:14AM +0000, Sch=F6fegger Stefan wrote:\n" + "> On Tue, Jun 13, 2017 at 05:43:14AM +0000, Sch\303\266fegger Stefan wrote:\n" "> > On Monday, June 12, 2017 6:49:24 PM CEST Bjorn Helgaas wrote:\n" "> > > On Wed, Jun 07, 2017 at 01:36:11PM +0200, Stefan Schoefegger wrote:\n" - "> > > > Link speed must not be limited to gen1 during link test for complia=\n" - "nce\n" + "> > > > Link speed must not be limited to gen1 during link test for compliance\n" "> > > > tests\n" - "> > > >=20\n" - "> > > > Signed-off-by: Stefan Schoefegger <stefan.schoefegger@ginzinger.com=\n" - ">\n" + "> > > > \n" + "> > > > Signed-off-by: Stefan Schoefegger <stefan.schoefegger@ginzinger.com>\n" "> > > > ---\n" - "> > > >=20\n" + "> > > > \n" "> > > > Changes since v1:\n" "> > > > - pci-imx6.c moved to dwc directory\n" - "> > > > =20\n" + "> > > > \n" "> > > > drivers/pci/dwc/Kconfig | 10 ++++++++++\n" "> > > > drivers/pci/dwc/pci-imx6.c | 21 ++++++++++++---------\n" "> > > > 2 files changed, 22 insertions(+), 9 deletions(-)\n" - "> > > >=20\n" + "> > > > \n" "> > > > diff --git a/drivers/pci/dwc/Kconfig b/drivers/pci/dwc/Kconfig\n" "> > > > index b7e15526d676..b6e9ced5a45d 100644\n" "> > > > --- a/drivers/pci/dwc/Kconfig\n" "> > > > +++ b/drivers/pci/dwc/Kconfig\n" "> > > > @@ -77,6 +77,16 @@ config PCI_IMX6\n" - "> > > >=20\n" + "> > > > \n" "> > > > \tselect PCIEPORTBUS\n" "> > > > \tselect PCIE_DW_HOST\n" - "> > > >=20\n" + "> > > > \n" "> > > > +config PCI_IMX6_COMPLIANCE_TEST\n" "> > > > +\tbool \"Enable pcie compliance tests on imx6\"\n" "> > > > +\tdepends on PCI_IMX6\n" @@ -53,106 +51,93 @@ "> > > > +\t The link speed wouldn't be limited to gen1 when enabled.\n" "> > > > +\t Enable only during compliance tests, otherwise\n" "> > > > +\t link detection will fail on some peripherals.\n" - "> > >=20\n" + "> > > \n" "> > > I'm puzzled about why we would want to merge this patch. It looks\n" "> > > like we're trying to game the system to make the device pass\n" "> > > compliance testing when it isn't really compliant. Is this config\n" "> > > option useful to users, or is it only useful during internal\n" "> > > development of iMX SoCs?\n" - "> >=20\n" + "> > \n" "> > It's not for passing compliance tests, it is necessary to do the\n" "> > compliance\n" - "> > tests. Without this patch only gen 1 speed is possible to test. Also i.=\n" - "mx6\n" + "> > tests. Without this patch only gen 1 speed is possible to test. Also i.mx6\n" "> > is not fully gen2 compliant (withour external clk) we should have the\n" "> > option to do gen2 tests. Switching from gen1 to gen2 is done with a\n" - "> > 100MHz (1ms) clk pulse on the receiver. Without this patch link speed i=\n" - "s\n" + "> > 100MHz (1ms) clk pulse on the receiver. Without this patch link speed is\n" "> > forced to gen1 afterwards.\n" - ">=20\n" + "> \n" "> I don't understand the purpose of this yet, so maybe all we need is a\n" "> better description.\n" "\n" - "This patch enabled compliance tests for pcie gen2 published by pci-sig.=20\n" - "(Signal level, signal timing, jitter and rise/fall times on tx lines). Spec=\n" - "ial=20\n" - "hardware is needed to do this tests (compliance load board, oszilloscope wi=\n" - "th=20\n" - "about 10GHz bandwith). The pcie phy switches in a complinace test state whe=\n" - "re=20\n" - "the phy outputs a special test pattern (without having a real link to a=20\n" - "device). The bitrate and de-emphasis must be switched. The driver (without=\n" - "=20\n" - "this patches) does not allow to switch to gen2 because it falls back to gen=\n" - "1.=20\n" + "This patch enabled compliance tests for pcie gen2 published by pci-sig. \n" + "(Signal level, signal timing, jitter and rise/fall times on tx lines). Special \n" + "hardware is needed to do this tests (compliance load board, oszilloscope with \n" + "about 10GHz bandwith). The pcie phy switches in a complinace test state where \n" + "the phy outputs a special test pattern (without having a real link to a \n" + "device). The bitrate and de-emphasis must be switched. The driver (without \n" + "this patches) does not allow to switch to gen2 because it falls back to gen1. \n" "It is impossible to generate the gen2 test pattern.\n" - "The patch now removes the forced gen1 start to allow generating gen2 test=20\n" - "pattern. gen1 / gen2 switch is done through signals generated on the=20\n" + "The patch now removes the forced gen1 start to allow generating gen2 test \n" + "pattern. gen1 / gen2 switch is done through signals generated on the \n" "compliance load board (which triggers switching in the phy).\n" - ">=20\n" + "> \n" "> \"It's not for passing compliance testing, it is necessary to do the\n" "> compliance tests\" doesn't make sense to me -- it seems\n" "> self-contradictory.\n" - ">=20\n" + "> \n" "> The Kconfig text says \"enable only for testing because it makes link\n" "> detection fail.\" To me that means this option is not useful for\n" "> users. We need some justification for why it should be in the\n" "> mainline kernel, where users and distros may enable it.\n" "\n" - "It use useless for users and distros. Only board designer want this option.=\n" - " If=20\n" + "It use useless for users and distros. Only board designer want this option. If \n" "this is not a reason for applying it's ok for me.\n" - ">=20\n" + "> \n" "> If you can only support gen2 in certain board configurations, maybe\n" "> you should add a config option that can always be enabled for those\n" "> boards.\n" - ">=20\n" + "> \n" "> > Yes it is only useful for board setup, it is comparable to\n" "> > CONFIG_USB_EHSET_TEST_FIXTURE for usb (ok, this is more general and not\n" "> > host specific).\n" - ">=20\n" + "> \n" "> USB_EHSET_TEST_FIXTURE (added by 1353aa53851e (\"usb: misc: EHSET Test\n" "> Fixture device driver for host compliance\")) looks like just another\n" "> driver in the sense that it's always safe to enable it and it doesn't\n" "> hurt anything if you enable it without having the hardware.\n" - ">=20\n" + "> \n" "> This patch doesn't seem comparable to USB_EHSET_TEST_FIXTURE because\n" "> this new option apparently breaks link detection in some cases.\n" "\n" - "This depends on the point of view (Black box vs. white box) :-) Both are fo=\n" - "r=20\n" - "doing compliance tests on signal level. But I think we don't need to discus=\n" - "s=20\n" + "This depends on the point of view (Black box vs. white box) :-) Both are for \n" + "doing compliance tests on signal level. But I think we don't need to discuss \n" "this further.\n" - ">=20\n" + "> \n" "> > > > config PCIE_SPEAR13XX\n" - "> > > > =20\n" + "> > > > \n" "> > > > \tbool \"STMicroelectronics SPEAr PCIe controller\"\n" "> > > > \tdepends on PCI\n" - "> > > >=20\n" - "> > > > diff --git a/drivers/pci/dwc/pci-imx6.c b/drivers/pci/dwc/pci-imx6.=\n" - "c\n" + "> > > > \n" + "> > > > diff --git a/drivers/pci/dwc/pci-imx6.c b/drivers/pci/dwc/pci-imx6.c\n" "> > > > index 19a289b8cc94..b0fbe52e25b0 100644\n" "> > > > --- a/drivers/pci/dwc/pci-imx6.c\n" "> > > > +++ b/drivers/pci/dwc/pci-imx6.c\n" "> > > > @@ -533,15 +533,18 @@ static int imx6_pcie_establish_link(struct\n" "> > > > imx6_pcie\n" "> > > > *imx6_pcie)>\n" - "> > > >=20\n" + "> > > > \n" "> > > > \tu32 tmp;\n" "> > > > \tint ret;\n" - "> > > >=20\n" + "> > > > \n" "> > > > -\t/*\n" - "> > > > -\t * Force Gen1 operation when starting the link. In case the link=\n" - " is\n" - "> > > > -\t * started in Gen2 mode, there is a possibility the devices on th=\n" - "e\n" + "> > > > -\t * Force Gen1 operation when starting the link. In case the link is\n" + "> > > > -\t * started in Gen2 mode, there is a possibility the devices on the\n" "> > > > -\t * bus will not be detected at all. This happens with PCIe\n" "> > > > switches.\n" "> > > > -\t */\n" - "> > > > -\ttmp =3D dw_pcie_readl_dbi(pci, PCIE_RC_LCR);\n" - "> > > > -\ttmp &=3D ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK;\n" - "> > > > -\ttmp |=3D PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1;\n" + "> > > > -\ttmp = dw_pcie_readl_dbi(pci, PCIE_RC_LCR);\n" + "> > > > -\ttmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK;\n" + "> > > > -\ttmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1;\n" "> > > > -\tdw_pcie_writel_dbi(pci, PCIE_RC_LCR, tmp);\n" "> > > > +\tif (!IS_ENABLED(CONFIG_PCI_IMX6_COMPLIANCE_TEST)) {\n" "> > > > +\t\t/*\n" @@ -161,13 +146,13 @@ "> > > > +\t\t * devices on the bus will not be detected at all. This\n" "> > > > +\t\t * happens with PCIe switches.\n" "> > > > +\t\t */\n" - "> > > > +\t\ttmp =3D dw_pcie_readl_dbi(pci, PCIE_RC_LCR);\n" - "> > > > +\t\ttmp &=3D ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK;\n" - "> > > > +\t\ttmp |=3D PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1;\n" + "> > > > +\t\ttmp = dw_pcie_readl_dbi(pci, PCIE_RC_LCR);\n" + "> > > > +\t\ttmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK;\n" + "> > > > +\t\ttmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN1;\n" "> > > > +\t\tdw_pcie_writel_dbi(pci, PCIE_RC_LCR, tmp);\n" "> > > > +\t}\n" - "> > > >=20\n" + "> > > > \n" "> > > > \t/* Start LTSSM. */\n" - "> > > > \tif (imx6_pcie->variant =3D=3D IMX7D)" + "> > > > \tif (imx6_pcie->variant == IMX7D)" -40f7621b5bb4caa387bd132dc84dd3cf2a2221e7d4bc8a0cbe3d804b5de47d3a +a6d33f728a9386f4743fe371a18ca9ad8d669056bd7a116b6105576d4043fa80
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.