Am Mittwoch, den 06.04.2016, 07:00 -0700 schrieb Tim Harvey: > On Wed, Apr 6, 2016 at 2:41 AM, Lucas Stach <l.stach@xxxxxxxxxxxxxx> wrote: > > Am Dienstag, den 05.04.2016, 09:29 -0700 schrieb Tim Harvey: > >> Freescale has stated [1] that the LVDS clock source of the IMX6 does not pass > >> the PCI Gen2 clock jitter test, therefore unless an external Gen2 compliant > >> external clock source is present and supplied back to the IMX6 PCIe core > >> via LVDS CLK1/CLK2 you can not claim Gen2 compliance. > >> > >> Add a dt property to specify gen1 vs gen2 and check this before allowing > >> a Gen2 link. > >> > >> We default to Gen1 if the property is not present because at this time there > >> are no IMX6 boards in mainline that 'input' a clock on LVDS CLK1/CLK2. > >> > >> In order to be Gen2 compliant on IMX6 you need to: > >> - have a Gen2 compliant external clock generator and route that clock back > >> to either LVDS CLK1 or LVDS CLK2 as an input. > >> (see IMX6SX-SabreSD reference design) > >> - specify this clock in the pcie node in the dt > >> (ie IMX6QDL_CLK_LVDS1_IN or IMX6QDL_CLK_LVDS2_IN instead of > >> IMX6QDL_CLK_LVDS1_GATE which configures it as a CLK output) > >> > >> [1] https://community.freescale.com/message/453209 > >> > >> Cc: Lucas Stach <l.stach@xxxxxxxxxxxxxx> > >> Cc: Bjorn Helgaas <helgaas@xxxxxxxxxx> > >> Cc: Fabio Estevam <fabio.estevam@xxxxxxxxxxxxx> > >> Cc: Zhu Richard <Richard.Zhu@xxxxxxxxxxxxx> > >> Cc: Akshay Bhat <akshay.bhat@xxxxxxxxxxx> > >> Cc: Rob Herring <robh+dt@xxxxxxxxxx> > >> Cc: Shawn Guo <shawnguo@xxxxxxxxxx> > >> Signed-off-by: Tim Harvey <tharvey@xxxxxxxxxxxxx> > > > > One nit below. Maybe Bjorn can fix this up while applying, otherwise if > > you need to resend you may carry the blow tag. > > > > Reviewed-by: Lucas Stach <l.stach@xxxxxxxxxxxxxx> > >> --- > >> v4: > >> - rebase against linux-pci/master > >> - add fsl vendor prefix to dt prop > >> > >> v3: > >> - added note in dt bindings doc that we limit to gen1 unless this is specified > >> as gen2 capable > >> - move property to imx6 pcie phy instead of designware core > >> - don't use &ret as temp storage as of_property_read_u32() doesn't change the > >> outval if property isn't found > >> v2: > >> - moved dt property to designware core > >> > >> Signed-off-by: Tim Harvey <tharvey@xxxxxxxxxxxxx> > >> --- > >> .../devicetree/bindings/pci/fsl,imx6q-pcie.txt | 4 ++++ > >> drivers/pci/host/pci-imx6.c | 24 +++++++++++++++------- > >> 2 files changed, 21 insertions(+), 7 deletions(-) > >> > >> diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > >> index 3be80c6..b92fafb 100644 > >> --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > >> +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > >> @@ -19,6 +19,10 @@ Optional properties: > >> - fsl,tx-deemph-gen2-6db: Gen2 (6db) De-emphasis value. Default: 20 > >> - fsl,tx-swing-full: Gen2 TX SWING FULL value. Default: 127 > >> - fsl,tx-swing-low: TX launch amplitude swing_low value. Default: 127 > >> +- 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. > >> > >> Example: > >> > >> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c > >> index eb5a275..9656ac7 100644 > >> --- a/drivers/pci/host/pci-imx6.c > >> +++ b/drivers/pci/host/pci-imx6.c > >> @@ -44,6 +44,7 @@ struct imx6_pcie { > >> u32 tx_deemph_gen2_6db; > >> u32 tx_swing_full; > >> u32 tx_swing_low; > >> + int link_gen; > >> }; > >> > >> /* PCIe Root Complex registers (memory-mapped) */ > >> @@ -417,11 +418,15 @@ static int imx6_pcie_establish_link(struct pcie_port *pp) > >> goto err_reset_phy; > >> } > >> > >> - /* Allow Gen2 mode after the link is up. */ > >> - tmp = readl(pp->dbi_base + PCIE_RC_LCR); > >> - tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK; > >> - tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN2; > >> - writel(tmp, pp->dbi_base + PCIE_RC_LCR); > >> + if (imx6_pcie->link_gen == 2) { > >> + /* Allow Gen2 mode after the link is up. */ > >> + tmp = readl(pp->dbi_base + PCIE_RC_LCR); > >> + tmp &= ~PCIE_RC_LCR_MAX_LINK_SPEEDS_MASK; > >> + tmp |= PCIE_RC_LCR_MAX_LINK_SPEEDS_GEN2; > >> + writel(tmp, pp->dbi_base + PCIE_RC_LCR); > >> + } else { > >> + dev_info(pp->dev, "Link: Gen2 disabled\n"); > >> + } > >> > >> /* > >> * Start Directed Speed Change so the best possible speed both link > >> @@ -445,8 +450,7 @@ static int imx6_pcie_establish_link(struct pcie_port *pp) > >> } > >> > >> tmp = readl(pp->dbi_base + PCIE_RC_LCSR); > >> - dev_dbg(pp->dev, "Link up, Gen=%i\n", (tmp >> 16) & 0xf); > >> - > >> + dev_info(pp->dev, "Link up, Gen%i\n", (tmp >> 16) & 0xf); > >> return 0; > >> > >> err_reset_phy: > >> @@ -598,6 +602,12 @@ static int __init imx6_pcie_probe(struct platform_device *pdev) > >> &imx6_pcie->tx_swing_low)) > >> imx6_pcie->tx_swing_low = 127; > >> > >> + /* Limit link speed */ > >> + ret = of_property_read_u32(pp->dev->of_node, "fsl,max-link-speed", > >> + &imx6_pcie->link_gen); > >> + if (ret) > >> + imx6_pcie->link_gen = -1; > > > > The binding specifies that if the property is absent, it will default to > > Gen1, so I would like to see this reflected here by setting the link_gen > > to 1, not -1. > > > >> + > >> ret = imx6_add_pcie_port(pp, pdev); > >> if (ret < 0) > >> return ret; > > > > > > Lucas, > > I can resend if really necessary but link_gen = -1 specifies to the > code that there was no default. When link_gen is used during link > 'that' is where it defaults to gen1 if not specifically gen 2. > I've noticed this and the code as is is correct. This is why it already has my Rb. ;) I still prefer to have it set to the binding specified default at the DT parsing site. It's cleaner this way IMO. Regards, Lucas -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
