Re: [PATCH v5] PCI: imx6: add dt prop for link gen, default to gen1

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Apr 06, 2016 at 04:14:54PM -0700, Tim Harvey wrote:
> 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@pengutronix.de>
> Cc: Bjorn Helgaas <helgaas@kernel.org>
> Cc: Fabio Estevam <fabio.estevam@freescale.com>
> Cc: Zhu Richard <Richard.Zhu@freescale.com>
> Cc: Akshay Bhat <akshay.bhat@timesys.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>

Applied to pci/host-imx6 for v4.7 with Lucas' Reviewed-by, thanks!

> ---
> v5:
>  - clearly default link gen to 1 when dt prop is read to avoid confusion
> 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@gateworks.com>
> ---
>  .../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..60a14b8 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;
> +
>  	ret = imx6_add_pcie_port(pp, pdev);
>  	if (ret < 0)
>  		return ret;
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html