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

[PATCH RFC] 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

Signed-off-by: Tim Harvey <tharvey@gateworks.com>

This is an RFC because I'm assuming the decision to default to Gen1 link only
is going to ruffle some feathers. My understanding is that if you do not
use an external Gen2 compliant clockgen for peripepherals 'and' the IMX6 core
you should not claim Gen2 compliance. This was not obvious on original IMX6
reference designs and I believe the jitter issue was discovered by Freescale
later and future reference designs were modified to state you need an ext
clockgen for Gen2 compliance.

---
 .../devicetree/bindings/pci/fsl,imx6q-pcie.txt     |  3 +++
 drivers/pci/host/pci-imx6.c                        | 22 ++++++++++++++++------
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
index 6fbba53..7dff332 100644
--- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
@@ -12,6 +12,8 @@ Required properties:
 	- "msi": The interrupt that is asserted when an MSI is received
 - clock-names: Must include the following additional entries:
 	- "pcie_phy"
+- fsl,max-link-speed: Specify PCI gen. Defaults to 1, can be 2 if board has
+  an external clock generator fed back to PCIe core.
 
 Example:
 
@@ -37,4 +39,5 @@ Example:
 		                <0 0 0 4 &intc GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&clks 144>, <&clks 206>, <&clks 189>;
 		clock-names = "pcie", "pcie_bus", "pcie_phy";
+		fsl,max-link-speed = <2>;
 	};
diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index 22e8224..16412c4 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -39,6 +39,7 @@ struct imx6_pcie {
 	struct pcie_port	pp;
 	struct regmap		*iomuxc_gpr;
 	void __iomem		*mem_base;
+	int			link_cap;
 };
 
 /* PCIe Root Complex registers (memory-mapped) */
@@ -393,11 +394,15 @@ static int imx6_pcie_establish_link(struct pcie_port *pp)
 	if (ret)
 		return ret;
 
-	/* 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_cap == 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
@@ -421,7 +426,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;
 }
 
@@ -591,6 +596,11 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
 		}
 	}
 
+	/* default link capability to gen1 */
+	imx6_pcie->link_cap = 1;
+	if (of_property_read_u32(np, "fsl,max-link-speed", &ret) == 0)
+		imx6_pcie->link_cap = ret;
+
 	/* Fetch clocks */
 	imx6_pcie->pcie_phy = devm_clk_get(&pdev->dev, "pcie_phy");
 	if (IS_ERR(imx6_pcie->pcie_phy)) {
-- 
1.9.1

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

[Fabio Estevam]

[Adding Richard Zhu]

On Thu, Nov 5, 2015 at 12:58 PM, Tim Harvey <tharvey@gateworks.com> 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
>
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>
> This is an RFC because I'm assuming the decision to default to Gen1 link only
> is going to ruffle some feathers. My understanding is that if you do not
> use an external Gen2 compliant clockgen for peripepherals 'and' the IMX6 core
> you should not claim Gen2 compliance. This was not obvious on original IMX6
> reference designs and I believe the jitter issue was discovered by Freescale
> later and future reference designs were modified to state you need an ext
> clockgen for Gen2 compliance.

Looks good to me:

Reviewed-by: Fabio Estevam <fabio.estevam@freescale.com>

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

[Zhu Richard]

SXQncyBva2F5IGZvciBtZSB0b28uDQpBY2tlZC1ieTogUmljaGFyZCBaaHUgPFJpY2hhcmQuWmh1
QGZyZWVzY2FsZS5jb20+DQpUaGFua3MgVGltLg0KDQpGcmVlc2NhbGUgTGludXggQlNQIHRlYW0N
CkJlc3QgUmVnYXJkcw0KUmljaGFyZCBaaHUNCg0KLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0N
CkZyb206IEZhYmlvIEVzdGV2YW0gW21haWx0bzpmZXN0ZXZhbUBnbWFpbC5jb21dIA0KU2VudDog
VGh1cnNkYXksIE5vdmVtYmVyIDA1LCAyMDE1IDExOjQxIFBNDQpUbzogVGltIEhhcnZleQ0KQ2M6
IEx1Y2FzIFN0YWNoOyBCam9ybiBIZWxnYWFzOyBsaW51eC1wY2lAdmdlci5rZXJuZWwub3JnOyBF
c3RldmFtIEZhYmlvLVI0OTQ5NjsgWmh1IFJpY2hhcmQtUjY1MDM3DQpTdWJqZWN0OiBSZTogW1BB
VENIIFJGQ10gUENJOiBpbXg2OiBhZGQgZHQgcHJvcCBmb3IgbGluayBnZW4sIGRlZmF1bHQgdG8g
Z2VuMQ0KDQpbQWRkaW5nIFJpY2hhcmQgWmh1XQ0KDQpPbiBUaHUsIE5vdiA1LCAyMDE1IGF0IDEy
OjU4IFBNLCBUaW0gSGFydmV5IDx0aGFydmV5QGdhdGV3b3Jrcy5jb20+IHdyb3RlOg0KPiBGcmVl
c2NhbGUgaGFzIHN0YXRlZCBbMV0gdGhhdCB0aGUgTFZEUyBjbG9jayBzb3VyY2Ugb2YgdGhlIElN
WDYgZG9lcyANCj4gbm90IHBhc3MgdGhlIFBDSSBHZW4yIGNsb2NrIGppdHRlciB0ZXN0LCB0aGVy
ZWZvcmUgdW5sZXNzIGFuIGV4dGVybmFsIA0KPiBHZW4yIGNvbXBsaWFudCBleHRlcm5hbCBjbG9j
ayBzb3VyY2UgaXMgcHJlc2VudCBhbmQgc3VwcGxpZWQgYmFjayB0byANCj4gdGhlIElNWDYgUENJ
ZSBjb3JlIHZpYSBMVkRTIENMSzEvQ0xLMiB5b3UgY2FuIG5vdCBjbGFpbSBHZW4yIGNvbXBsaWFu
Y2UuDQo+DQo+IEFkZCBhIGR0IHByb3BlcnR5IHRvIHNwZWNpZnkgZ2VuMSB2cyBnZW4yIGFuZCBj
aGVjayB0aGlzIGJlZm9yZSANCj4gYWxsb3dpbmcgYSBHZW4yIGxpbmsuDQo+DQo+IFdlIGRlZmF1
bHQgdG8gR2VuMSBpZiB0aGUgcHJvcGVydHkgaXMgbm90IHByZXNlbnQgYmVjYXVzZSBhdCB0aGlz
IHRpbWUgDQo+IHRoZXJlIGFyZSBubyBJTVg2IGJvYXJkcyBpbiBtYWlubGluZSB0aGF0ICdpbnB1
dCcgYSBjbG9jayBvbiBMVkRTIENMSzEvQ0xLMi4NCj4NCj4gSW4gb3JkZXIgdG8gYmUgR2VuMiBj
b21wbGlhbnQgb24gSU1YNiB5b3UgbmVlZCB0bzoNCj4gIC0gaGF2ZSBhIEdlbjIgY29tcGxpYW50
IGV4dGVybmFsIGNsb2NrIGdlbmVyYXRvciBhbmQgcm91dGUgdGhhdCBjbG9jayBiYWNrDQo+ICAg
IHRvIGVpdGhlciBMVkRTIENMSzEgb3IgTFZEUyBDTEsyIGFzIGFuIGlucHV0Lg0KPiAgICAoc2Vl
IElNWDZTWC1TYWJyZVNEIHJlZmVyZW5jZSBkZXNpZ24pDQo+ICAtIHNwZWNpZnkgdGhpcyBjbG9j
ayBpbiB0aGUgcGNpZSBub2RlIGluIHRoZSBkdA0KPiAgICAoaWUgSU1YNlFETF9DTEtfTFZEUzFf
SU4gb3IgSU1YNlFETF9DTEtfTFZEUzJfSU4gaW5zdGVhZCBvZg0KPiAgICAgSU1YNlFETF9DTEtf
TFZEUzFfR0FURSB3aGljaCBjb25maWd1cmVzIGl0IGFzIGEgQ0xLIG91dHB1dCkNCj4NCj4gWzFd
IGh0dHBzOi8vY29tbXVuaXR5LmZyZWVzY2FsZS5jb20vbWVzc2FnZS80NTMyMDkNCj4NCj4gU2ln
bmVkLW9mZi1ieTogVGltIEhhcnZleSA8dGhhcnZleUBnYXRld29ya3MuY29tPg0KPg0KPiBUaGlz
IGlzIGFuIFJGQyBiZWNhdXNlIEknbSBhc3N1bWluZyB0aGUgZGVjaXNpb24gdG8gZGVmYXVsdCB0
byBHZW4xIA0KPiBsaW5rIG9ubHkgaXMgZ29pbmcgdG8gcnVmZmxlIHNvbWUgZmVhdGhlcnMuIE15
IHVuZGVyc3RhbmRpbmcgaXMgdGhhdCANCj4gaWYgeW91IGRvIG5vdCB1c2UgYW4gZXh0ZXJuYWwg
R2VuMiBjb21wbGlhbnQgY2xvY2tnZW4gZm9yIA0KPiBwZXJpcGVwaGVyYWxzICdhbmQnIHRoZSBJ
TVg2IGNvcmUgeW91IHNob3VsZCBub3QgY2xhaW0gR2VuMiANCj4gY29tcGxpYW5jZS4gVGhpcyB3
YXMgbm90IG9idmlvdXMgb24gb3JpZ2luYWwgSU1YNiByZWZlcmVuY2UgZGVzaWducyANCj4gYW5k
IEkgYmVsaWV2ZSB0aGUgaml0dGVyIGlzc3VlIHdhcyBkaXNjb3ZlcmVkIGJ5IEZyZWVzY2FsZSBs
YXRlciBhbmQgDQo+IGZ1dHVyZSByZWZlcmVuY2UgZGVzaWducyB3ZXJlIG1vZGlmaWVkIHRvIHN0
YXRlIHlvdSBuZWVkIGFuIGV4dCBjbG9ja2dlbiBmb3IgR2VuMiBjb21wbGlhbmNlLg0KDQpMb29r
cyBnb29kIHRvIG1lOg0KDQpSZXZpZXdlZC1ieTogRmFiaW8gRXN0ZXZhbSA8ZmFiaW8uZXN0ZXZh
bUBmcmVlc2NhbGUuY29tPg0K

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

[Lucas Stach]

Am Donnerstag, den 05.11.2015, 06:58 -0800 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.
> 
I think I already said this in the last round: there is nothing vendor
specific in a max-link-speed property. I would really like to have this
as a common DW PCIe property right from the beginning, so that we don't
burden us with keeping backward compatibility with vendor specific
properties when this moves to common code eventually.

The only difference between imx6 and all other DW cores is that the
absence of the property should mean unconstrained normally and
constrained to gen1 for imx.

> 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
> 
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> 
> This is an RFC because I'm assuming the decision to default to Gen1 link only
> is going to ruffle some feathers.

I think everyone is okay with that part.

> My understanding is that if you do not
> use an external Gen2 compliant clockgen for peripepherals 'and' the IMX6 core
> you should not claim Gen2 compliance. This was not obvious on original IMX6
> reference designs and I believe the jitter issue was discovered by Freescale
> later and future reference designs were modified to state you need an ext
> clockgen for Gen2 compliance.
> 
> ---
>  .../devicetree/bindings/pci/fsl,imx6q-pcie.txt     |  3 +++
>  drivers/pci/host/pci-imx6.c                        | 22 ++++++++++++++++------
>  2 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> index 6fbba53..7dff332 100644
> --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> @@ -12,6 +12,8 @@ Required properties:
>  	- "msi": The interrupt that is asserted when an MSI is received
>  - clock-names: Must include the following additional entries:
>  	- "pcie_phy"
> +- fsl,max-link-speed: Specify PCI gen. Defaults to 1, can be 2 if board has
> +  an external clock generator fed back to PCIe core.
>  
>  Example:
>  
> @@ -37,4 +39,5 @@ Example:
>  		                <0 0 0 4 &intc GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>;
>  		clocks = <&clks 144>, <&clks 206>, <&clks 189>;
>  		clock-names = "pcie", "pcie_bus", "pcie_phy";
> +		fsl,max-link-speed = <2>;
>  	};
> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index 22e8224..16412c4 100644
> --- a/drivers/pci/host/pci-imx6.c
> +++ b/drivers/pci/host/pci-imx6.c
> @@ -39,6 +39,7 @@ struct imx6_pcie {
>  	struct pcie_port	pp;
>  	struct regmap		*iomuxc_gpr;
>  	void __iomem		*mem_base;
> +	int			link_cap;
>  };
>  
>  /* PCIe Root Complex registers (memory-mapped) */
> @@ -393,11 +394,15 @@ static int imx6_pcie_establish_link(struct pcie_port *pp)
>  	if (ret)
>  		return ret;
>  
> -	/* 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_cap == 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
> @@ -421,7 +426,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;
>  }
>  
> @@ -591,6 +596,11 @@ static int __init imx6_pcie_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	/* default link capability to gen1 */
> +	imx6_pcie->link_cap = 1;
> +	if (of_property_read_u32(np, "fsl,max-link-speed", &ret) == 0)
> +		imx6_pcie->link_cap = ret;
> +
>  	/* Fetch clocks */
>  	imx6_pcie->pcie_phy = devm_clk_get(&pdev->dev, "pcie_phy");
>  	if (IS_ERR(imx6_pcie->pcie_phy)) {

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

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

[Tim Harvey]

On Fri, Nov 6, 2015 at 1:36 AM, Lucas Stach <l.stach@pengutronix.de> wrote:
> Am Donnerstag, den 05.11.2015, 06:58 -0800 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.
>>
> I think I already said this in the last round: there is nothing vendor
> specific in a max-link-speed property. I would really like to have this
> as a common DW PCIe property right from the beginning, so that we don't
> burden us with keeping backward compatibility with vendor specific
> properties when this moves to common code eventually.

Hi Lucas,

I did discuss this patch with you off-list but I think I misunderstood
your meaning then.

So you are asking me to simply rename the properly
'fsl,max-link-speed' to 'max-link-speed' and the rest of the patch is
good with you?

Regards,

Tim

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

[Lucas Stach]

Am Freitag, den 06.11.2015, 11:59 -0800 schrieb Tim Harvey:
> On Fri, Nov 6, 2015 at 1:36 AM, Lucas Stach <l.stach@pengutronix.de> wrote:
> > Am Donnerstag, den 05.11.2015, 06:58 -0800 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.
> >>
> > I think I already said this in the last round: there is nothing vendor
> > specific in a max-link-speed property. I would really like to have this
> > as a common DW PCIe property right from the beginning, so that we don't
> > burden us with keeping backward compatibility with vendor specific
> > properties when this moves to common code eventually.
> 
> Hi Lucas,
> 
> I did discuss this patch with you off-list but I think I misunderstood
> your meaning then.
> 
> So you are asking me to simply rename the properly
> 'fsl,max-link-speed' to 'max-link-speed' and the rest of the patch is
> good with you?
> 
Yes, rename to max-link-speed. Move parsing of the property to
dw_pcie_host_init() (it is a common property, so must be in common code)
before the call to ops->host_init and store value into the pcie_port
structure with -1 meaning property in DT is missing.

imx6_pcie_establish_link can the treat values of -1 and 1 the same, only
allowing Gen2 if the value is 2.

Regards,
Lucas
-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

[PATCH v2] 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

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
v2:
 - moved dt property to designware core

 .../devicetree/bindings/pci/designware-pcie.txt          |  1 +
 drivers/pci/host/pci-imx6.c                              | 16 ++++++++++------
 drivers/pci/host/pcie-designware.c                       |  4 ++++
 drivers/pci/host/pcie-designware.h                       |  1 +
 4 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
index 9f4faa8..a9a94b9 100644
--- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
@@ -26,3 +26,4 @@ Optional properties:
 - bus-range: PCI bus numbers covered (it is recommended for new devicetrees to
   specify this property, to keep backwards compatibility a range of 0x00-0xff
   is assumed if not present)
+- max-link-speed: Specify PCI gen for link capability (ie 2 for gen2)
diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index 8f3a981..6a9f310 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -392,11 +392,15 @@ static int imx6_pcie_establish_link(struct pcie_port *pp)
 	if (ret)
 		return ret;
 
-	/* 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 (pp->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
@@ -420,7 +424,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;
 }
 
diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index 52aa6e3..7aa81ff 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -487,6 +487,10 @@ int dw_pcie_host_init(struct pcie_port *pp)
 		return -EINVAL;
 	}
 
+	pp->link_gen = -1;
+	if (of_property_read_u32(np, "max-link-speed", &ret) == 0)
+		pp->link_gen = ret;
+
 	if (IS_ENABLED(CONFIG_PCI_MSI)) {
 		if (!pp->ops->msi_host_init) {
 			pp->irq_domain = irq_domain_add_linear(pp->dev->of_node,
diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
index d0bbd27..5ce821d 100644
--- a/drivers/pci/host/pcie-designware.h
+++ b/drivers/pci/host/pcie-designware.h
@@ -48,6 +48,7 @@ struct pcie_port {
 	struct resource		busn;
 	int			irq;
 	u32			lanes;
+	int			link_gen;
 	struct pcie_host_ops	*ops;
 	int			msi_irq;
 	struct irq_domain	*irq_domain;
-- 
1.9.1

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

[Lucas Stach]

Am Donnerstag, den 19.11.2015, 06:12 -0800 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
> 
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>

One nit below, but looks good to me:

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
> v2:
>  - moved dt property to designware core
> 
>  .../devicetree/bindings/pci/designware-pcie.txt          |  1 +
>  drivers/pci/host/pci-imx6.c                              | 16 ++++++++++------
>  drivers/pci/host/pcie-designware.c                       |  4 ++++
>  drivers/pci/host/pcie-designware.h                       |  1 +
>  4 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> index 9f4faa8..a9a94b9 100644
> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> @@ -26,3 +26,4 @@ Optional properties:
>  - bus-range: PCI bus numbers covered (it is recommended for new devicetrees to
>    specify this property, to keep backwards compatibility a range of 0x00-0xff
>    is assumed if not present)
> +- max-link-speed: Specify PCI gen for link capability (ie 2 for gen2)
> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index 8f3a981..6a9f310 100644
> --- a/drivers/pci/host/pci-imx6.c
> +++ b/drivers/pci/host/pci-imx6.c
> @@ -392,11 +392,15 @@ static int imx6_pcie_establish_link(struct pcie_port *pp)
>  	if (ret)
>  		return ret;
>  
> -	/* 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 (pp->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
> @@ -420,7 +424,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;
>  }
>  
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index 52aa6e3..7aa81ff 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -487,6 +487,10 @@ int dw_pcie_host_init(struct pcie_port *pp)
>  		return -EINVAL;
>  	}
>  
> +	pp->link_gen = -1;
> +	if (of_property_read_u32(np, "max-link-speed", &ret) == 0)
> +		pp->link_gen = ret;

of_property_read_u32() doesn't change the return parameter if the
property isn't found. So using &ret as temporary storage isn't strictly
needed.

> +
>  	if (IS_ENABLED(CONFIG_PCI_MSI)) {
>  		if (!pp->ops->msi_host_init) {
>  			pp->irq_domain = irq_domain_add_linear(pp->dev->of_node,
> diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
> index d0bbd27..5ce821d 100644
> --- a/drivers/pci/host/pcie-designware.h
> +++ b/drivers/pci/host/pcie-designware.h
> @@ -48,6 +48,7 @@ struct pcie_port {
>  	struct resource		busn;
>  	int			irq;
>  	u32			lanes;
> +	int			link_gen;
>  	struct pcie_host_ops	*ops;
>  	int			msi_irq;
>  	struct irq_domain	*irq_domain;

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

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

[Bjorn Helgaas]

Hi Tim,

On Thu, Nov 19, 2015 at 06:12:45AM -0800, 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
> 
> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> ---
> v2:
>  - moved dt property to designware core
> 
>  .../devicetree/bindings/pci/designware-pcie.txt          |  1 +
>  drivers/pci/host/pci-imx6.c                              | 16 ++++++++++------
>  drivers/pci/host/pcie-designware.c                       |  4 ++++
>  drivers/pci/host/pcie-designware.h                       |  1 +
>  4 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> index 9f4faa8..a9a94b9 100644
> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> @@ -26,3 +26,4 @@ Optional properties:
>  - bus-range: PCI bus numbers covered (it is recommended for new devicetrees to
>    specify this property, to keep backwards compatibility a range of 0x00-0xff
>    is assumed if not present)
> +- max-link-speed: Specify PCI gen for link capability (ie 2 for gen2)

Is there some sort of DT or OF spec that lists "max-link-speed" as a
generic property?  I see Lucas' desire to have this be common across
DesignWare PCIe cores.  Should it be moved up a level even from there,
i.e., to bindings/pci/pci.txt?

It might be worth mentioning in pci/fsl,imx6q-pcie.txt that we limit
the link to gen1 unless max-link-speed is present and has the value
"2".

> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index 8f3a981..6a9f310 100644
> --- a/drivers/pci/host/pci-imx6.c
> +++ b/drivers/pci/host/pci-imx6.c
> @@ -392,11 +392,15 @@ static int imx6_pcie_establish_link(struct pcie_port *pp)
>  	if (ret)
>  		return ret;
>  
> -	/* 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 (pp->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
> @@ -420,7 +424,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;
>  }
>  
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index 52aa6e3..7aa81ff 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -487,6 +487,10 @@ int dw_pcie_host_init(struct pcie_port *pp)
>  		return -EINVAL;
>  	}
>  
> +	pp->link_gen = -1;
> +	if (of_property_read_u32(np, "max-link-speed", &ret) == 0)
> +		pp->link_gen = ret;
> +
>  	if (IS_ENABLED(CONFIG_PCI_MSI)) {
>  		if (!pp->ops->msi_host_init) {
>  			pp->irq_domain = irq_domain_add_linear(pp->dev->of_node,
> diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h
> index d0bbd27..5ce821d 100644
> --- a/drivers/pci/host/pcie-designware.h
> +++ b/drivers/pci/host/pcie-designware.h
> @@ -48,6 +48,7 @@ struct pcie_port {
>  	struct resource		busn;
>  	int			irq;
>  	u32			lanes;
> +	int			link_gen;
>  	struct pcie_host_ops	*ops;
>  	int			msi_irq;
>  	struct irq_domain	*irq_domain;
> -- 
> 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 v2] PCI: imx6: add dt prop for link gen, default to gen1

[Tim Harvey]

On Wed, Nov 25, 2015 at 3:14 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> Hi Tim,
>
> On Thu, Nov 19, 2015 at 06:12:45AM -0800, 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
>>
>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>> ---
>> v2:
>>  - moved dt property to designware core
>>
>>  .../devicetree/bindings/pci/designware-pcie.txt          |  1 +
>>  drivers/pci/host/pci-imx6.c                              | 16 ++++++++++------
>>  drivers/pci/host/pcie-designware.c                       |  4 ++++
>>  drivers/pci/host/pcie-designware.h                       |  1 +
>>  4 files changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>> index 9f4faa8..a9a94b9 100644
>> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
>> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>> @@ -26,3 +26,4 @@ Optional properties:
>>  - bus-range: PCI bus numbers covered (it is recommended for new devicetrees to
>>    specify this property, to keep backwards compatibility a range of 0x00-0xff
>>    is assumed if not present)
>> +- max-link-speed: Specify PCI gen for link capability (ie 2 for gen2)
>
> Is there some sort of DT or OF spec that lists "max-link-speed" as a
> generic property?  I see Lucas' desire to have this be common across
> DesignWare PCIe cores.  Should it be moved up a level even from there,
> i.e., to bindings/pci/pci.txt?

Bjorn,

I don't know what the general consensus is here. As your the PCI
maintainer I would leave that up to you. Are there other platforms
that need to link at a lesser capability than the host controller is
capable of? I am only aware of the IMX6 and SPEAr13XX [1]

I can make that change and re-submit if you feel that is necessary.

>
> It might be worth mentioning in pci/fsl,imx6q-pcie.txt that we limit
> the link to gen1 unless max-link-speed is present and has the value
> "2".

I can add that in a v3 patch.

Tim

[1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/spear13xx-pcie.txt#n14

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

[Tim Harvey]

On Wed, Dec 2, 2015 at 8:35 AM, Tim Harvey <tharvey@gateworks.com> wrote:
> On Wed, Nov 25, 2015 at 3:14 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> Hi Tim,
>>
>> On Thu, Nov 19, 2015 at 06:12:45AM -0800, 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
>>>
>>> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>>> ---
>>> v2:
>>>  - moved dt property to designware core
>>>
>>>  .../devicetree/bindings/pci/designware-pcie.txt          |  1 +
>>>  drivers/pci/host/pci-imx6.c                              | 16 ++++++++++------
>>>  drivers/pci/host/pcie-designware.c                       |  4 ++++
>>>  drivers/pci/host/pcie-designware.h                       |  1 +
>>>  4 files changed, 16 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>> index 9f4faa8..a9a94b9 100644
>>> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>> @@ -26,3 +26,4 @@ Optional properties:
>>>  - bus-range: PCI bus numbers covered (it is recommended for new devicetrees to
>>>    specify this property, to keep backwards compatibility a range of 0x00-0xff
>>>    is assumed if not present)
>>> +- max-link-speed: Specify PCI gen for link capability (ie 2 for gen2)
>>
>> Is there some sort of DT or OF spec that lists "max-link-speed" as a
>> generic property?  I see Lucas' desire to have this be common across
>> DesignWare PCIe cores.  Should it be moved up a level even from there,
>> i.e., to bindings/pci/pci.txt?
>
> Bjorn,
>
> I don't know what the general consensus is here. As your the PCI
> maintainer I would leave that up to you. Are there other platforms
> that need to link at a lesser capability than the host controller is
> capable of? I am only aware of the IMX6 and SPEAr13XX [1]
>
> I can make that change and re-submit if you feel that is necessary.
>

Bjorn,

Any thoughts here? I would really like to see this get picked up
sooner rather than later.

Regards,

Tim

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

[Akshay Bhat]

Tim Harvey <tharvey <at> gateworks.com> writes:

> >>
> >> Is there some sort of DT or OF spec that lists "max-link-speed" as a
> >> generic property?  I see Lucas' desire to have this be common across
> >> DesignWare PCIe cores.  Should it be moved up a level even from there,
> >> i.e., to bindings/pci/pci.txt?
> >
> > Bjorn,
> >
> > I don't know what the general consensus is here. As your the PCI
> > maintainer I would leave that up to you. Are there other platforms
> > that need to link at a lesser capability than the host controller is
> > capable of? I am only aware of the IMX6 and SPEAr13XX [1]
> >
> > I can make that change and re-submit if you feel that is necessary.
> >
> 
> Bjorn,
> 
> Any thoughts here? I would really like to see this get picked up
> sooner rather than later.
> 
> Regards,
> 
> Tim
> 

Bjorn,

It looks like there was no further follow-up on this patch. Could you 
kindly provide feedback to Tim's question? We have multiple i.MX boards 
that could use this patch to resolve PCIe issues.

Thanks,
Akshay

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

[Bjorn Helgaas]

[+cc Tim]

On Wed, Feb 24, 2016 at 07:35:55PM +0000, Akshay Bhat wrote:
> Tim Harvey <tharvey <at> gateworks.com> writes:
> 
> > >>
> > >> Is there some sort of DT or OF spec that lists "max-link-speed" as a
> > >> generic property?  I see Lucas' desire to have this be common across
> > >> DesignWare PCIe cores.  Should it be moved up a level even from there,
> > >> i.e., to bindings/pci/pci.txt?
> > >
> > > Bjorn,
> > >
> > > I don't know what the general consensus is here. As your the PCI
> > > maintainer I would leave that up to you. Are there other platforms
> > > that need to link at a lesser capability than the host controller is
> > > capable of? I am only aware of the IMX6 and SPEAr13XX [1]
> > >
> > > I can make that change and re-submit if you feel that is necessary.
> > >
> > 
> > Bjorn,
> > 
> > Any thoughts here? I would really like to see this get picked up
> > sooner rather than later.
> > 
> > Regards,
> > 
> > Tim
> > 
> 
> Bjorn,
> 
> It looks like there was no further follow-up on this patch. Could you 
> kindly provide feedback to Tim's question? We have multiple i.MX boards 
> that could use this patch to resolve PCIe issues.

Tim had mentioned doing a v3 with a documentation update, so I
dropped the v2 patch in anticipation of v3, which never appeared.

Looking at it again, I have another question, so I'll respond to that
thread again.

Bjorn

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

[Bjorn Helgaas]

[+cc Rob, devicetree list]

On Wed, Dec 02, 2015 at 08:35:06AM -0800, Tim Harvey wrote:
> On Wed, Nov 25, 2015 at 3:14 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Thu, Nov 19, 2015 at 06:12:45AM -0800, 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
> >>
> >> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> >> ---
> >> v2:
> >>  - moved dt property to designware core
> >>
> >>  .../devicetree/bindings/pci/designware-pcie.txt          |  1 +
> >>  drivers/pci/host/pci-imx6.c                              | 16 ++++++++++------
> >>  drivers/pci/host/pcie-designware.c                       |  4 ++++
> >>  drivers/pci/host/pcie-designware.h                       |  1 +
> >>  4 files changed, 16 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> >> index 9f4faa8..a9a94b9 100644
> >> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
> >> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> >> @@ -26,3 +26,4 @@ Optional properties:
> >>  - bus-range: PCI bus numbers covered (it is recommended for new devicetrees to
> >>    specify this property, to keep backwards compatibility a range of 0x00-0xff
> >>    is assumed if not present)
> >> +- max-link-speed: Specify PCI gen for link capability (ie 2 for gen2)
> >
> > Is there some sort of DT or OF spec that lists "max-link-speed" as a
> > generic property?  I see Lucas' desire to have this be common across
> > DesignWare PCIe cores.  Should it be moved up a level even from there,
> > i.e., to bindings/pci/pci.txt?
> 
> I don't know what the general consensus is here. As your the PCI
> maintainer I would leave that up to you. Are there other platforms
> that need to link at a lesser capability than the host controller is
> capable of? I am only aware of the IMX6 and SPEAr13XX [1]

This is really a devicetree question, not a PCI one, so I added Rob
and the devicetree list in case they have any comments on this.

> > It might be worth mentioning in pci/fsl,imx6q-pcie.txt that we limit
> > the link to gen1 unless max-link-speed is present and has the value
> > "2".

This default seems backwards.  It seems like we'd want to configure
the link to go as fast as possible unless we have a quirk, e.g.,
"max-link-speed", that imposes a device-specific link.  In other
words, why don't we penalize the broken board instead of penalizing
all the working ones?

> [1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/spear13xx-pcie.txt#n14

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

[Rob Herring]

On Wed, Feb 24, 2016 at 2:03 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> [+cc Rob, devicetree list]
>
> On Wed, Dec 02, 2015 at 08:35:06AM -0800, Tim Harvey wrote:
>> On Wed, Nov 25, 2015 at 3:14 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> > On Thu, Nov 19, 2015 at 06:12:45AM -0800, 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
>> >>
>> >> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>> >> ---
>> >> v2:
>> >>  - moved dt property to designware core
>> >>
>> >>  .../devicetree/bindings/pci/designware-pcie.txt          |  1 +
>> >>  drivers/pci/host/pci-imx6.c                              | 16 ++++++++++------
>> >>  drivers/pci/host/pcie-designware.c                       |  4 ++++
>> >>  drivers/pci/host/pcie-designware.h                       |  1 +
>> >>  4 files changed, 16 insertions(+), 6 deletions(-)
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>> >> index 9f4faa8..a9a94b9 100644
>> >> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
>> >> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>> >> @@ -26,3 +26,4 @@ Optional properties:
>> >>  - bus-range: PCI bus numbers covered (it is recommended for new devicetrees to
>> >>    specify this property, to keep backwards compatibility a range of 0x00-0xff
>> >>    is assumed if not present)
>> >> +- max-link-speed: Specify PCI gen for link capability (ie 2 for gen2)
>> >
>> > Is there some sort of DT or OF spec that lists "max-link-speed" as a
>> > generic property?  I see Lucas' desire to have this be common across
>> > DesignWare PCIe cores.  Should it be moved up a level even from there,
>> > i.e., to bindings/pci/pci.txt?
>>
>> I don't know what the general consensus is here. As your the PCI
>> maintainer I would leave that up to you. Are there other platforms
>> that need to link at a lesser capability than the host controller is
>> capable of? I am only aware of the IMX6 and SPEAr13XX [1]
>
> This is really a devicetree question, not a PCI one, so I added Rob
> and the devicetree list in case they have any comments on this.

Seems generally useful to me. You could want to limit the speed for a
variety of reasons. There's no standard property that I'm aware of.

Shouldn't this be a property of the phy though?

>> > It might be worth mentioning in pci/fsl,imx6q-pcie.txt that we limit
>> > the link to gen1 unless max-link-speed is present and has the value
>> > "2".
>
> This default seems backwards.  It seems like we'd want to configure
> the link to go as fast as possible unless we have a quirk, e.g.,
> "max-link-speed", that imposes a device-specific link.  In other
> words, why don't we penalize the broken board instead of penalizing
> all the working ones?

I agree. It could be argued that this way doesn't require a DT update
to fix broken boards. However, this problem should really be found
before production and DT updates are normal for enabling new features
(such as compliant PCIe).

Rob

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

[Tim Harvey]

On Wed, Feb 24, 2016 at 12:35 PM, Rob Herring <robh+dt@kernel.org> wrote:
> On Wed, Feb 24, 2016 at 2:03 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> [+cc Rob, devicetree list]
>>
>> On Wed, Dec 02, 2015 at 08:35:06AM -0800, Tim Harvey wrote:
>>> On Wed, Nov 25, 2015 at 3:14 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>>> > On Thu, Nov 19, 2015 at 06:12:45AM -0800, 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
>>> >>
>>> >> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
>>> >> ---
>>> >> v2:
>>> >>  - moved dt property to designware core
>>> >>
>>> >>  .../devicetree/bindings/pci/designware-pcie.txt          |  1 +
>>> >>  drivers/pci/host/pci-imx6.c                              | 16 ++++++++++------
>>> >>  drivers/pci/host/pcie-designware.c                       |  4 ++++
>>> >>  drivers/pci/host/pcie-designware.h                       |  1 +
>>> >>  4 files changed, 16 insertions(+), 6 deletions(-)
>>> >>
>>> >> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>> >> index 9f4faa8..a9a94b9 100644
>>> >> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>> >> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
>>> >> @@ -26,3 +26,4 @@ Optional properties:
>>> >>  - bus-range: PCI bus numbers covered (it is recommended for new devicetrees to
>>> >>    specify this property, to keep backwards compatibility a range of 0x00-0xff
>>> >>    is assumed if not present)
>>> >> +- max-link-speed: Specify PCI gen for link capability (ie 2 for gen2)
>>> >
>>> > Is there some sort of DT or OF spec that lists "max-link-speed" as a
>>> > generic property?  I see Lucas' desire to have this be common across
>>> > DesignWare PCIe cores.  Should it be moved up a level even from there,
>>> > i.e., to bindings/pci/pci.txt?
>>>
>>> I don't know what the general consensus is here. As your the PCI
>>> maintainer I would leave that up to you. Are there other platforms
>>> that need to link at a lesser capability than the host controller is
>>> capable of? I am only aware of the IMX6 and SPEAr13XX [1]
>>
>> This is really a devicetree question, not a PCI one, so I added Rob
>> and the devicetree list in case they have any comments on this.
>
> Seems generally useful to me. You could want to limit the speed for a
> variety of reasons. There's no standard property that I'm aware of.
>
> Shouldn't this be a property of the phy though?
>
>>> > It might be worth mentioning in pci/fsl,imx6q-pcie.txt that we limit
>>> > the link to gen1 unless max-link-speed is present and has the value
>>> > "2".
>>
>> This default seems backwards.  It seems like we'd want to configure
>> the link to go as fast as possible unless we have a quirk, e.g.,
>> "max-link-speed", that imposes a device-specific link.  In other
>> words, why don't we penalize the broken board instead of penalizing
>> all the working ones?
>
> I agree. It could be argued that this way doesn't require a DT update
> to fix broken boards. However, this problem should really be found
> before production and DT updates are normal for enabling new features
> (such as compliant PCIe).
>
> Rob

Rob,

In this case I feel that every IMX6 based board with PCIe likely has
this issue because the original reference schematics from Freescale
did not mention that the LVDS clock source from the IMX6 did not meet
PCIe gen2 requirements so everyone blindly followed the reference
design. Only later did they seem to come out with this information and
if you did use an external clock you would have to setup the IMX6 pcie
clocks differently in the device-tree (configuring the IMX6 to use a
clock input instead of output), which I see no current boards doing.

There are several SoC's that use the designware core that likely are
not in this same boat, so it would be unfair to penalize them by
defaulting a gen1 speed tied to the mac. So yes, perhaps a property of
the imx6 pcie phy and defaulting it to gen1 because no current
device-tree's set the phy's clock as an external input makes the most
sense?

Note that I never have seen any failure when running an IMX6 at Gen2
while using the LVDS clock generated from the IMX6 for PCIe, I'm just
taking Freescale's word for it that it does not meet jitter
requirements (they never provided details on the lack of compliance
failure mode). I assume most board manufacturers have not been able to
run compliance tests on their own designs derived from the reference
design.

Regards,

Tim

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

[Bjorn Helgaas]

On Mon, Feb 29, 2016 at 06:41:29AM -0800, Tim Harvey wrote:
> On Wed, Feb 24, 2016 at 12:35 PM, Rob Herring <robh+dt@kernel.org> wrote:
> > On Wed, Feb 24, 2016 at 2:03 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >> [+cc Rob, devicetree list]
> >>
> >> On Wed, Dec 02, 2015 at 08:35:06AM -0800, Tim Harvey wrote:
> >>> On Wed, Nov 25, 2015 at 3:14 PM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >>> > On Thu, Nov 19, 2015 at 06:12:45AM -0800, 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
> >>> >>
> >>> >> Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> >>> >> ---
> >>> >> v2:
> >>> >>  - moved dt property to designware core
> >>> >>
> >>> >>  .../devicetree/bindings/pci/designware-pcie.txt          |  1 +
> >>> >>  drivers/pci/host/pci-imx6.c                              | 16 ++++++++++------
> >>> >>  drivers/pci/host/pcie-designware.c                       |  4 ++++
> >>> >>  drivers/pci/host/pcie-designware.h                       |  1 +
> >>> >>  4 files changed, 16 insertions(+), 6 deletions(-)
> >>> >>
> >>> >> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> >>> >> index 9f4faa8..a9a94b9 100644
> >>> >> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt
> >>> >> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt
> >>> >> @@ -26,3 +26,4 @@ Optional properties:
> >>> >>  - bus-range: PCI bus numbers covered (it is recommended for new devicetrees to
> >>> >>    specify this property, to keep backwards compatibility a range of 0x00-0xff
> >>> >>    is assumed if not present)
> >>> >> +- max-link-speed: Specify PCI gen for link capability (ie 2 for gen2)
> >>> >
> >>> > Is there some sort of DT or OF spec that lists "max-link-speed" as a
> >>> > generic property?  I see Lucas' desire to have this be common across
> >>> > DesignWare PCIe cores.  Should it be moved up a level even from there,
> >>> > i.e., to bindings/pci/pci.txt?
> >>>
> >>> I don't know what the general consensus is here. As your the PCI
> >>> maintainer I would leave that up to you. Are there other platforms
> >>> that need to link at a lesser capability than the host controller is
> >>> capable of? I am only aware of the IMX6 and SPEAr13XX [1]
> >>
> >> This is really a devicetree question, not a PCI one, so I added Rob
> >> and the devicetree list in case they have any comments on this.
> >
> > Seems generally useful to me. You could want to limit the speed for a
> > variety of reasons. There's no standard property that I'm aware of.
> >
> > Shouldn't this be a property of the phy though?
> >
> >>> > It might be worth mentioning in pci/fsl,imx6q-pcie.txt that we limit
> >>> > the link to gen1 unless max-link-speed is present and has the value
> >>> > "2".
> >>
> >> This default seems backwards.  It seems like we'd want to configure
> >> the link to go as fast as possible unless we have a quirk, e.g.,
> >> "max-link-speed", that imposes a device-specific link.  In other
> >> words, why don't we penalize the broken board instead of penalizing
> >> all the working ones?
> >
> > I agree. It could be argued that this way doesn't require a DT update
> > to fix broken boards. However, this problem should really be found
> > before production and DT updates are normal for enabling new features
> > (such as compliant PCIe).
> >
> > Rob
> 
> Rob,
> 
> In this case I feel that every IMX6 based board with PCIe likely has
> this issue because the original reference schematics from Freescale
> did not mention that the LVDS clock source from the IMX6 did not meet
> PCIe gen2 requirements so everyone blindly followed the reference
> design. Only later did they seem to come out with this information and
> if you did use an external clock you would have to setup the IMX6 pcie
> clocks differently in the device-tree (configuring the IMX6 to use a
> clock input instead of output), which I see no current boards doing.
> 
> There are several SoC's that use the designware core that likely are
> not in this same boat, so it would be unfair to penalize them by
> defaulting a gen1 speed tied to the mac. So yes, perhaps a property of
> the imx6 pcie phy and defaulting it to gen1 because no current
> device-tree's set the phy's clock as an external input makes the most
> sense?

I'm not an IMX6 expert, but this sounds good to me, so I'll look for a
new rev that does this.

Bjorn