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

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

[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@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>
---
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..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;
+
 	ret = imx6_add_pcie_port(pp, pdev);
 	if (ret < 0)
 		return ret;
-- 
1.9.1

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

[Bjorn Helgaas]

On Tue, Apr 05, 2016 at 09:29:14AM -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>

Waiting for an ack from Richard and/or Lucas.

> ---
> 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..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;
> +
>  	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

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

[Lucas Stach]

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@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>

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@pengutronix.de>
> ---
> 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..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;

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

[Tim Harvey]

On Wed, Apr 6, 2016 at 2:41 AM, Lucas Stach <l.stach@pengutronix.de> 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@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>
>
> 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@pengutronix.de>
>> ---
>> 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..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.

Tim

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

[Lucas Stach]

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@pengutronix.de> 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@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>
> >
> > 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@pengutronix.de>
> >> ---
> >> 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..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