[PATCH 0/2] Workaround for IMX7d PCI-e PLL lock failure

[PATCH 0/2] Workaround for IMX7d PCI-e PLL lock failure

[Trent Piepho]

This is the workaround for the IMX7d Erratum e10728, failure of
initialize PCIe PLL VCO oscillation resulting in PLL lock failure and
failure of the PCI-e link to come up.

The registers used in the workaround are based on the latest patch in
the NXP kernel, but many things around that have been changed.

This uses a new node of type fsl,imx-pcie-phy to get the PHY's
registers.  The node is found via a phandle added to the PCI-e
controller's node, rather than the incorrect way done in the NXP kernel.

There is no error if the phandle is not preset (since it's needed except
for the imx7d workaround and no existing dtses have it), but if preset
it is an error if something relating to it does not work.

** Should the node be fsl,imx7d-pcie-phy?  snps,dw-pcie-phy?  

There is little to no documenation from NXP and Synopsis about this, so I'm
unsure of the PHY's lineage.

The imx6 PCI-e driver does not use the generic phy layer to interact
with the PHY.  It appears PHY related hardware, like clocks, regulators,
and resets, are part of the fsl,imx6q-pcie node.  But again, the
topology of this hardware is not documented very well.

Another approach would be to add the PHY registers as another bank in
the PCI-e node.  This would match how the PHY reset, clock, etc.  are
done.  However, the PHY is attached to a different AXI master than the
PCI-e controller, so the register range really does not belong there.

Trent Piepho (2):
  ARM: dts: imx7d: Add node for PCIe PHY
  PCI: imx: Add workaround for e10728, IMX7d PCIe PLL failure

 .../devicetree/bindings/pci/fsl,imx6q-pcie.txt     | 11 ++++
 arch/arm/boot/dts/imx7d.dtsi                       |  9 ++++
 drivers/pci/dwc/pci-imx6.c                         | 59 ++++++++++++++++++++++
 3 files changed, 79 insertions(+)

Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Richard Zhu <hongxing.zhu@nxp.com>
Cc: Lucas Stach <l.stach@pengutronix.de>

-- 
2.14.4

[PATCH 1/2] ARM: dts: imx7d: Add node for PCIe PHY

[Trent Piepho]

There isn't yet any code in the kernel that uses this device's register,
but there will be some for a PCIe PLL erratum wortkaround.

This adds the PHY as a new node.  The PCI-e controller node gains a
phandle property that points to it.  There is no driver for the PHY at
this point and all the existing code that relates to the PHY is part of
the PCI-e controller driver (and does not need register access, yet).

Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Richard Zhu <hongxing.zhu@nxp.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Signed-off-by: Trent Piepho <tpiepho@impinj.com>
---
 Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt | 11 +++++++++++
 arch/arm/boot/dts/imx7d.dtsi                             |  9 +++++++++
 2 files changed, 20 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
index cb33421184a0..c7aeda6878ff 100644
--- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
@@ -50,6 +50,7 @@ Additional required properties for imx7d-pcie:
 - reset-names: Must contain the following entires:
 	       - "pciephy"
 	       - "apps"
+- fsl,pcie-phy: A phandle to an fsl,imx-pcie-phy node.
 
 Example:
 
@@ -76,3 +77,13 @@ Example:
 		clocks = <&clks 144>, <&clks 206>, <&clks 189>;
 		clock-names = "pcie", "pcie_bus", "pcie_phy";
 	};
+
+* Freescale i.MX7d PCIe PHY
+
+This is the PHY associated with the IMX7d PCIe controller.  It's used by the
+PCI-e controller via the fsl,pcie-phy phandle.
+
+Required properties:
+- compatible:
+	- "fsl,imx-pcie-phy"
+- reg: base address and length of the PCIe PHY controller
diff --git a/arch/arm/boot/dts/imx7d.dtsi b/arch/arm/boot/dts/imx7d.dtsi
index 200714e3feea..31f5c8576251 100644
--- a/arch/arm/boot/dts/imx7d.dtsi
+++ b/arch/arm/boot/dts/imx7d.dtsi
@@ -94,6 +94,14 @@
 	};
 };
 
+&aips2 {
+	pcie_phy: pcie-phy at 306d0000 {
+		  compatible = "fsl,imx-pcie-phy";
+		  reg = <0x306d0000 0x10000>;
+		  status = "disabled";
+	};
+};
+
 &aips3 {
 	usbotg2: usb at 30b20000 {
 		compatible = "fsl,imx7d-usb", "fsl,imx27-usb";
@@ -167,6 +175,7 @@
 			 <&src IMX7_RESET_PCIE_CTRL_APPS_EN>;
 		reset-names = "pciephy", "apps";
 		status = "disabled";
+		fsl,pcie-phy = <&pcie_phy>;
 	};
 };
 
-- 
2.14.4

[PATCH 1/2] ARM: dts: imx7d: Add node for PCIe PHY

[Shawn Guo]

On Wed, Jul 18, 2018 at 12:44:23PM -0700, Trent Piepho wrote:
> There isn't yet any code in the kernel that uses this device's register,
> but there will be some for a PCIe PLL erratum wortkaround.
> 
> This adds the PHY as a new node.  The PCI-e controller node gains a
> phandle property that points to it.  There is no driver for the PHY at
> this point and all the existing code that relates to the PHY is part of
> the PCI-e controller driver (and does not need register access, yet).
> 
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Signed-off-by: Trent Piepho <tpiepho@impinj.com>
> ---
>  Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt | 11 +++++++++++
>  arch/arm/boot/dts/imx7d.dtsi                             |  9 +++++++++
>  2 files changed, 20 insertions(+)

Please have separate patches for bindings and DTS.

Shawn

> 
> diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> index cb33421184a0..c7aeda6878ff 100644
> --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> @@ -50,6 +50,7 @@ Additional required properties for imx7d-pcie:
>  - reset-names: Must contain the following entires:
>  	       - "pciephy"
>  	       - "apps"
> +- fsl,pcie-phy: A phandle to an fsl,imx-pcie-phy node.
>  
>  Example:
>  
> @@ -76,3 +77,13 @@ Example:
>  		clocks = <&clks 144>, <&clks 206>, <&clks 189>;
>  		clock-names = "pcie", "pcie_bus", "pcie_phy";
>  	};
> +
> +* Freescale i.MX7d PCIe PHY
> +
> +This is the PHY associated with the IMX7d PCIe controller.  It's used by the
> +PCI-e controller via the fsl,pcie-phy phandle.
> +
> +Required properties:
> +- compatible:
> +	- "fsl,imx-pcie-phy"
> +- reg: base address and length of the PCIe PHY controller
> diff --git a/arch/arm/boot/dts/imx7d.dtsi b/arch/arm/boot/dts/imx7d.dtsi
> index 200714e3feea..31f5c8576251 100644
> --- a/arch/arm/boot/dts/imx7d.dtsi
> +++ b/arch/arm/boot/dts/imx7d.dtsi
> @@ -94,6 +94,14 @@
>  	};
>  };
>  
> +&aips2 {
> +	pcie_phy: pcie-phy at 306d0000 {
> +		  compatible = "fsl,imx-pcie-phy";
> +		  reg = <0x306d0000 0x10000>;
> +		  status = "disabled";
> +	};
> +};
> +
>  &aips3 {
>  	usbotg2: usb at 30b20000 {
>  		compatible = "fsl,imx7d-usb", "fsl,imx27-usb";
> @@ -167,6 +175,7 @@
>  			 <&src IMX7_RESET_PCIE_CTRL_APPS_EN>;
>  		reset-names = "pciephy", "apps";
>  		status = "disabled";
> +		fsl,pcie-phy = <&pcie_phy>;
>  	};
>  };
>  
> -- 
> 2.14.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/2] ARM: dts: imx7d: Add node for PCIe PHY

[Lucas Stach]

Am Mittwoch, den 18.07.2018, 12:44 -0700 schrieb Trent Piepho:
> There isn't yet any code in the kernel that uses this device's register,
> but there will be some for a PCIe PLL erratum wortkaround.
> 
> This adds the PHY as a new node.??The PCI-e controller node gains a
> phandle property that points to it.??There is no driver for the PHY at
> this point and all the existing code that relates to the PHY is part of
> the PCI-e controller driver (and does not need register access, yet).
> 
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Cc: Sascha Hauer <kernel@pengutronix.de>
> > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > Cc: Richard Zhu <hongxing.zhu@nxp.com>
> > Cc: Lucas Stach <l.stach@pengutronix.de>
> > Signed-off-by: Trent Piepho <tpiepho@impinj.com>
> ---
> ?Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt | 11 +++++++++++
> ?arch/arm/boot/dts/imx7d.dtsi?????????????????????????????|??9 +++++++++
> ?2 files changed, 20 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> index cb33421184a0..c7aeda6878ff 100644
> --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> @@ -50,6 +50,7 @@ Additional required properties for imx7d-pcie:
> ?- reset-names: Must contain the following entires:
> > ?	???????- "pciephy"
> > ?	???????- "apps"
> +- fsl,pcie-phy: A phandle to an fsl,imx-pcie-phy node.
> ?
> ?Example:
> ?
> @@ -76,3 +77,13 @@ Example:
> > ?		clocks = <&clks 144>, <&clks 206>, <&clks 189>;
> > ?		clock-names = "pcie", "pcie_bus", "pcie_phy";
> > ?	};
> +
> +* Freescale i.MX7d PCIe PHY
> +
> +This is the PHY associated with the IMX7d PCIe controller.??It's used by the
> +PCI-e controller via the fsl,pcie-phy phandle.
> +
> +Required properties:
> +- compatible:
> +	- "fsl,imx-pcie-phy"

This is too generic. Please change it to "fsl,imx7-pcie-phy".

> +- reg: base address and length of the PCIe PHY controller
> diff --git a/arch/arm/boot/dts/imx7d.dtsi b/arch/arm/boot/dts/imx7d.dtsi
> index 200714e3feea..31f5c8576251 100644
> --- a/arch/arm/boot/dts/imx7d.dtsi
> +++ b/arch/arm/boot/dts/imx7d.dtsi
> @@ -94,6 +94,14 @@
> > ?	};
> ?};
> ?
> +&aips2 {
> > > +	pcie_phy: pcie-phy at 306d0000 {
> > +		??compatible = "fsl,imx-pcie-phy";
> > +		??reg = <0x306d0000 0x10000>;
> > +		??status = "disabled";
> > +	};
> +};
> +
> ?&aips3 {
> > > ?	usbotg2: usb at 30b20000 {
> > ?		compatible = "fsl,imx7d-usb", "fsl,imx27-usb";
> @@ -167,6 +175,7 @@
> > ?			?<&src IMX7_RESET_PCIE_CTRL_APPS_EN>;
> > ?		reset-names = "pciephy", "apps";
> > ?		status = "disabled";
> > +		fsl,pcie-phy = <&pcie_phy>;
> > ?	};
> ?};
> ?

[PATCH 1/2] ARM: dts: imx7d: Add node for PCIe PHY

[Trent Piepho]

On Wed, 2018-08-01 at 12:39 +0200, Lucas Stach wrote:
> 
> > +This is the PHY associated with the IMX7d PCIe controller.  It's
> > used by the
> > +PCI-e controller via the fsl,pcie-phy phandle.
> > +
> > +Required properties:
> > +- compatible:
> > +	- "fsl,imx-pcie-phy"
> 
> This is too generic. Please change it to "fsl,imx7-pcie-phy".

Can anyone from NXP tell us if an external PCIe PHY block is present in
other IMX designs?  I suspect that this is generic, but no design other
than imx7d has had any reason to use this PHY register space yet.

If this is specific to this SoC, then maybe it shoudl be fsl,imx7d-
pcie-phy, since the iMX7s has no pcie.

[PATCH 1/2] ARM: dts: imx7d: Add node for PCIe PHY

[Richard Zhu]

> -----Original Message-----
> From: Trent Piepho [mailto:tpiepho at impinj.com]
> Sent: Thursday, August 2, 2018 1:34 AM
> To: l.stach at pengutronix.de; linux-arm-kernel at lists.infradead.org;
> linux-pci at vger.kernel.org
> Cc: Richard Zhu <hongxing.zhu@nxp.com>; shawnguo at kernel.org;
> kernel at pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>
> Subject: Re: [PATCH 1/2] ARM: dts: imx7d: Add node for PCIe PHY
> 
> On Wed, 2018-08-01 at 12:39 +0200, Lucas Stach wrote:
> >
> > > +This is the PHY associated with the IMX7d PCIe controller.  It's
> > > used by the
> > > +PCI-e controller via the fsl,pcie-phy phandle.
> > > +
> > > +Required properties:
> > > +- compatible:
> > > +	- "fsl,imx-pcie-phy"
> >
> > This is too generic. Please change it to "fsl,imx7-pcie-phy".
> 
> Can anyone from NXP tell us if an external PCIe PHY block is present in other
> IMX designs?  I suspect that this is generic, but no design other than imx7d
> has had any reason to use this PHY register space yet.
>
Hi Trent:
There is a different PCIe PHY block in imx7d PCIe refer to the imx6.
So, the standalone memory region is used to map the imx7d PCIe PHY registers.
Instead of the standalone PHY node, how about to include the PHY registers memory region into PCIe node?
For example:
               pcie: pcie at 0x33800000 {
                        compatible = "fsl,imx7d-pcie", "snps,dw-pcie";
                        reg = <0x33800000 0x4000>, <0x306d0000 0x10000>, <0x4ff00000 0x80000>;
                        reg-names = "dbi", "phy", "config";

Richard Zhu
Best Regards!

 
> If this is specific to this SoC, then maybe it shoudl be fsl,imx7d- pcie-phy, since
> the iMX7s has no pcie.

[PATCH 1/2] ARM: dts: imx7d: Add node for PCIe PHY

[Lucas Stach]

Am Donnerstag, den 02.08.2018, 00:43 +0000 schrieb Richard Zhu:
> > -----Original Message-----
> > > > From: Trent Piepho [mailto:tpiepho at impinj.com]
> > Sent: Thursday, August 2, 2018 1:34 AM
> > > > > > To: l.stach at pengutronix.de; linux-arm-kernel at lists.infradead.org;
> > linux-pci at vger.kernel.org
> > > > > > Cc: Richard Zhu <hongxing.zhu@nxp.com>; shawnguo at kernel.org;
> > > > kernel at pengutronix.de; Fabio Estevam <fabio.estevam@nxp.com>
> > Subject: Re: [PATCH 1/2] ARM: dts: imx7d: Add node for PCIe PHY
> > 
> > On Wed, 2018-08-01 at 12:39 +0200, Lucas Stach wrote:
> > > 
> > > > +This is the PHY associated with the IMX7d PCIe controller.??It's
> > > > used by the
> > > > +PCI-e controller via the fsl,pcie-phy phandle.
> > > > +
> > > > +Required properties:
> > > > +- compatible:
> > > > +	- "fsl,imx-pcie-phy"
> > > 
> > > This is too generic. Please change it to "fsl,imx7-pcie-phy".
> > 
> > Can anyone from NXP tell us if an external PCIe PHY block is present in other
> > IMX designs???I suspect that this is generic, but no design other than imx7d
> > has had any reason to use this PHY register space yet.
> > 
> 
> Hi Trent:
> There is a different PCIe PHY block in imx7d PCIe refer to the imx6.
> So, the standalone memory region is used to map the imx7d PCIe PHY registers.
> Instead of the standalone PHY node, how about to include the PHY registers memory region into PCIe node?
> For example:
> > ???????????????pcie: pcie at 0x33800000 {
> ????????????????????????compatible = "fsl,imx7d-pcie", "snps,dw-pcie";
> ????????????????????????reg = <0x33800000 0x4000>, <0x306d0000 0x10000>, <0x4ff00000 0x80000>;
> ????????????????????????reg-names = "dbi", "phy", "config";

I dislike this solution, as it isn't a correct description of the HW.
Having it as a separate node with it's own compatible, as done in this
patchset, allows us to switch over to a real PHY driver should we ever
have a need for it, all without breaking the DT abstraction again.

Regards,
Lucas 

[PATCH 2/2] PCI: imx: Add workaround for e10728, IMX7d PCIe PLL failure

[Trent Piepho]

This implements the workound described in the NXP IMX7d erratum e10728.

Initial VCO oscillation may fail under corner conditions such as cold
temperature.  It causes PCIe PLL to fail to lock in the initialization
phase, which results in the PCIe link failing to come up.

The workaround is to disable Duty-cycle Corrector (DCC) calibration
after G_RST.

To do this it is necessary to gain access to the undocumented and
currently unused PCIe PHY register bank.  A new device tree node of type
"fsl,imx-pcie-phy" is created for the PHY block and the existing PCIe
device uses a phandle named "fsl,pcie-phy" to point to it.

Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Fabio Estevam <fabio.estevam@nxp.com>
Cc: Richard Zhu <hongxing.zhu@nxp.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Signed-off-by: Trent Piepho <tpiepho@impinj.com>
---
 drivers/pci/dwc/pci-imx6.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/drivers/pci/dwc/pci-imx6.c b/drivers/pci/dwc/pci-imx6.c
index dcf389b62944..32429afc9380 100644
--- a/drivers/pci/dwc/pci-imx6.c
+++ b/drivers/pci/dwc/pci-imx6.c
@@ -18,6 +18,7 @@
 #include <linux/module.h>
 #include <linux/of_gpio.h>
 #include <linux/of_device.h>
+#include <linux/of_address.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
@@ -58,6 +59,7 @@ struct imx6_pcie {
 	u32			tx_swing_low;
 	int			link_gen;
 	struct regulator	*vpcie;
+	void __iomem		*phy_base;
 };
 
 /* Parameters for the waiting for PCIe PHY PLL to lock on i.MX7 */
@@ -98,6 +100,23 @@ struct imx6_pcie {
 #define PCIE_PHY_RX_ASIC_OUT 0x100D
 #define PCIE_PHY_RX_ASIC_OUT_VALID	(1 << 0)
 
+/* iMX7 PCIe PHY registers */
+#define PCIE_PHY_CMN_REG4		0x14
+/* These are probably the bits that *aren't* DCC_FB_EN */
+#define PCIE_PHY_CMN_REG4_DCC_FB_EN	0x29
+
+#define PCIE_PHY_CMN_REG15	        0x54
+#define PCIE_PHY_CMN_REG15_DLY_4	BIT(2)
+#define PCIE_PHY_CMN_REG15_PLL_PD	BIT(5)
+#define PCIE_PHY_CMN_REG15_OVRD_PLL_PD	BIT(7)
+
+#define PCIE_PHY_CMN_REG24		0x90
+#define PCIE_PHY_CMN_REG24_RX_EQ	BIT(6)
+#define PCIE_PHY_CMN_REG24_RX_EQ_SEL	BIT(3)
+
+#define PCIE_PHY_CMN_REG26		0x98
+#define PCIE_PHY_CMN_REG26_ATT_MODE	0xBC
+
 #define PHY_RX_OVRD_IN_LO 0x1005
 #define PHY_RX_OVRD_IN_LO_RX_DATA_EN (1 << 5)
 #define PHY_RX_OVRD_IN_LO_RX_PLL_EN (1 << 3)
@@ -431,6 +450,26 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
 	switch (imx6_pcie->variant) {
 	case IMX7D:
 		reset_control_deassert(imx6_pcie->pciephy_reset);
+
+		/* Workaround for ERR010728, failure of PCI-e PLL VCO to
+		 * oscillate, especially when cold.  This turns off "Duty-cycle
+		 * Corrector" and other mysterious undocumented things.
+		 */
+		if (likely(imx6_pcie->phy_base)) {
+			/* De-assert DCC_FB_EN */
+			writel(PCIE_PHY_CMN_REG4_DCC_FB_EN,
+			       imx6_pcie->phy_base + PCIE_PHY_CMN_REG4);
+			/* Assert RX_EQS and RX_EQS_SEL */
+			writel(PCIE_PHY_CMN_REG24_RX_EQ_SEL
+				| PCIE_PHY_CMN_REG24_RX_EQ,
+			       imx6_pcie->phy_base + PCIE_PHY_CMN_REG24);
+			/* Assert ATT_MODE */
+			writel(PCIE_PHY_CMN_REG26_ATT_MODE,
+			       imx6_pcie->phy_base + PCIE_PHY_CMN_REG26);
+		} else {
+			dev_err(dev, "PHY base address not set\n");
+		}
+
 		imx7d_pcie_wait_for_phy_pll_lock(imx6_pcie);
 		break;
 	case IMX6SX:
@@ -698,6 +737,7 @@ static int imx6_pcie_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct dw_pcie *pci;
 	struct imx6_pcie *imx6_pcie;
+	struct device_node *np;
 	struct resource *dbi_base;
 	struct device_node *node = dev->of_node;
 	int ret;
@@ -717,6 +757,25 @@ static int imx6_pcie_probe(struct platform_device *pdev)
 	imx6_pcie->variant =
 		(enum imx6_pcie_variants)of_device_get_match_data(dev);
 
+	/* Find the PHY if one is defined, only imx7d uses it */
+	np = of_parse_phandle(node, "fsl,pcie-phy", 0);
+	if (np) {
+		struct resource res;
+
+		ret = of_address_to_resource(np, 0, &res);
+		if (ret) {
+			dev_err(dev, "Unable to map PCIe PHY\n");
+			return ret;
+		}
+		imx6_pcie->phy_base = devm_ioremap_resource(dev, &res);
+		if (IS_ERR(imx6_pcie->phy_base)) {
+			dev_err(dev, "Unable to map PCIe PHY\n");
+			return PTR_ERR(imx6_pcie->phy_base);
+		}
+	} else {
+		imx6_pcie->phy_base = NULL;
+	}
+
 	dbi_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	pci->dbi_base = devm_ioremap_resource(dev, dbi_base);
 	if (IS_ERR(pci->dbi_base))
-- 
2.14.4

[PATCH 2/2] PCI: imx: Add workaround for e10728, IMX7d PCIe PLL failure

[Lucas Stach]

Am Mittwoch, den 18.07.2018, 12:44 -0700 schrieb Trent Piepho:
> This implements the workound described in the NXP IMX7d erratum e10728.
> 
> Initial VCO oscillation may fail under corner conditions such as cold
> temperature.??It causes PCIe PLL to fail to lock in the initialization
> phase, which results in the PCIe link failing to come up.
> 
> The workaround is to disable Duty-cycle Corrector (DCC) calibration
> after G_RST.
> 
> To do this it is necessary to gain access to the undocumented and
> currently unused PCIe PHY register bank.??A new device tree node of type
> "fsl,imx-pcie-phy" is created for the PHY block and the existing PCIe
> device uses a phandle named "fsl,pcie-phy" to point to it.
> 
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Cc: Sascha Hauer <kernel@pengutronix.de>
> > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > Cc: Richard Zhu <hongxing.zhu@nxp.com>
> > Cc: Lucas Stach <l.stach@pengutronix.de>
> > Signed-off-by: Trent Piepho <tpiepho@impinj.com>
> ---
> ?drivers/pci/dwc/pci-imx6.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++
> ?1 file changed, 59 insertions(+)
> 
> diff --git a/drivers/pci/dwc/pci-imx6.c b/drivers/pci/dwc/pci-imx6.c
> index dcf389b62944..32429afc9380 100644
> --- a/drivers/pci/dwc/pci-imx6.c
> +++ b/drivers/pci/dwc/pci-imx6.c
> @@ -18,6 +18,7 @@
> ?#include <linux/module.h>
> ?#include <linux/of_gpio.h>
> ?#include <linux/of_device.h>
> +#include <linux/of_address.h>
> ?#include <linux/pci.h>
> ?#include <linux/platform_device.h>
> ?#include <linux/regmap.h>
> @@ -58,6 +59,7 @@ struct imx6_pcie {
> > > ?	u32			tx_swing_low;
> > > ?	int			link_gen;
> > > ?	struct regulator	*vpcie;
> > > +	void __iomem		*phy_base;
> ?};
> ?
> ?/* Parameters for the waiting for PCIe PHY PLL to lock on i.MX7 */
> @@ -98,6 +100,23 @@ struct imx6_pcie {
> ?#define PCIE_PHY_RX_ASIC_OUT 0x100D
> > ?#define PCIE_PHY_RX_ASIC_OUT_VALID	(1 << 0)
> ?
> +/* iMX7 PCIe PHY registers */
> > +#define PCIE_PHY_CMN_REG4		0x14
> +/* These are probably the bits that *aren't* DCC_FB_EN */
> > +#define PCIE_PHY_CMN_REG4_DCC_FB_EN	0x29
> +
> > +#define PCIE_PHY_CMN_REG15	????????0x54
> > +#define PCIE_PHY_CMN_REG15_DLY_4	BIT(2)
> > +#define PCIE_PHY_CMN_REG15_PLL_PD	BIT(5)
> > +#define PCIE_PHY_CMN_REG15_OVRD_PLL_PD	BIT(7)
> +
> > +#define PCIE_PHY_CMN_REG24		0x90
> > +#define PCIE_PHY_CMN_REG24_RX_EQ	BIT(6)
> > +#define PCIE_PHY_CMN_REG24_RX_EQ_SEL	BIT(3)
> +
> > +#define PCIE_PHY_CMN_REG26		0x98
> > +#define PCIE_PHY_CMN_REG26_ATT_MODE	0xBC
> +
> ?#define PHY_RX_OVRD_IN_LO 0x1005
> ?#define PHY_RX_OVRD_IN_LO_RX_DATA_EN (1 << 5)
> ?#define PHY_RX_OVRD_IN_LO_RX_PLL_EN (1 << 3)
> @@ -431,6 +450,26 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
> > ?	switch (imx6_pcie->variant) {
> > ?	case IMX7D:
> > ?		reset_control_deassert(imx6_pcie->pciephy_reset);
> +
> > +		/* Workaround for ERR010728, failure of PCI-e PLL VCO to
> > +		?* oscillate, especially when cold.??This turns off "Duty-cycle
> > +		?* Corrector" and other mysterious undocumented things.
> > +		?*/
> > +		if (likely(imx6_pcie->phy_base)) {
> > +			/* De-assert DCC_FB_EN */
> > +			writel(PCIE_PHY_CMN_REG4_DCC_FB_EN,
> > +			???????imx6_pcie->phy_base + PCIE_PHY_CMN_REG4);
> > +			/* Assert RX_EQS and RX_EQS_SEL */
> > +			writel(PCIE_PHY_CMN_REG24_RX_EQ_SEL
> > +				| PCIE_PHY_CMN_REG24_RX_EQ,
> > +			???????imx6_pcie->phy_base + PCIE_PHY_CMN_REG24);
> > +			/* Assert ATT_MODE */
> > +			writel(PCIE_PHY_CMN_REG26_ATT_MODE,
> > +			???????imx6_pcie->phy_base + PCIE_PHY_CMN_REG26);
> > +		} else {
> +			dev_err(dev, "PHY base address not set\n");

Please tune this down to the dev_warn level, in case we are running a
new kernel on a older DT. Also the message isn't really helpful to the
user. It would be better to have something like "No access to PHY
registers, can't apply workaround for ERR010728." Maybe even with a
hint to update the DT.

> +		}
> +
> > ?		imx7d_pcie_wait_for_phy_pll_lock(imx6_pcie);
> > ?		break;
> > ?	case IMX6SX:
> @@ -698,6 +737,7 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> > ?	struct device *dev = &pdev->dev;
> > ?	struct dw_pcie *pci;
> > ?	struct imx6_pcie *imx6_pcie;
> > +	struct device_node *np;
> > ?	struct resource *dbi_base;
> > ?	struct device_node *node = dev->of_node;
> > ?	int ret;
> @@ -717,6 +757,25 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> > ?	imx6_pcie->variant =
> > ?		(enum imx6_pcie_variants)of_device_get_match_data(dev);
> ?
> > +	/* Find the PHY if one is defined, only imx7d uses it */
> > +	np = of_parse_phandle(node, "fsl,pcie-phy", 0);
> > +	if (np) {
> > +		struct resource res;
> +
> > +		ret = of_address_to_resource(np, 0, &res);
> > +		if (ret) {
> > +			dev_err(dev, "Unable to map PCIe PHY\n");
> > +			return ret;
> > +		}
> > +		imx6_pcie->phy_base = devm_ioremap_resource(dev, &res);
> > +		if (IS_ERR(imx6_pcie->phy_base)) {
> > +			dev_err(dev, "Unable to map PCIe PHY\n");
> > +			return PTR_ERR(imx6_pcie->phy_base);
> > +		}
> > +	} else {
> > +		imx6_pcie->phy_base = NULL;
> +	}

imx6_pcie is zero initialized on allocation, so no need for the else
path setting this to NULL.

Regards,
Lucas

> +
> > ?	dbi_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > ?	pci->dbi_base = devm_ioremap_resource(dev, dbi_base);
> > ?	if (IS_ERR(pci->dbi_base))

[PATCH 0/2] Workaround for IMX7d PCI-e PLL lock failure

[Lucas Stach]

Hi Trent,

Am Mittwoch, den 18.07.2018, 12:44 -0700 schrieb Trent Piepho:
> This is the workaround for the IMX7d Erratum e10728, failure of
> initialize PCIe PLL VCO oscillation resulting in PLL lock failure and
> failure of the PCI-e link to come up.
> 
> The registers used in the workaround are based on the latest patch in
> the NXP kernel, but many things around that have been changed.
> 
> This uses a new node of type fsl,imx-pcie-phy to get the PHY's
> registers.??The node is found via a phandle added to the PCI-e
> controller's node, rather than the incorrect way done in the NXP kernel.
> 
> There is no error if the phandle is not preset (since it's needed except
> for the imx7d workaround and no existing dtses have it), but if preset
> it is an error if something relating to it does not work.
> 
> ** Should the node be fsl,imx7d-pcie-phy???snps,dw-pcie-phy???
> 
> There is little to no documenation from NXP and Synopsis about this, so I'm
> unsure of the PHY's lineage.
> 
> The imx6 PCI-e driver does not use the generic phy layer to interact
> with the PHY.??It appears PHY related hardware, like clocks, regulators,
> and resets, are part of the fsl,imx6q-pcie node.??But again, the
> topology of this hardware is not documented very well.
> 
> Another approach would be to add the PHY registers as another bank in
> the PCI-e node.??This would match how the PHY reset, clock, etc.??are
> done.??However, the PHY is attached to a different AXI master than the
> PCI-e controller, so the register range really does not belong there.

I took some time pondering about the way this is done. There are some
small issues in the implementation (see my replies to the individual
patches), but the overall approach taken looks fine to me.

Regards,
Lucas

[PATCH 0/2] Workaround for IMX7d PCI-e PLL lock failure

[Lorenzo Pieralisi]

On Wed, Jul 18, 2018 at 12:44:22PM -0700, Trent Piepho wrote:
> This is the workaround for the IMX7d Erratum e10728, failure of
> initialize PCIe PLL VCO oscillation resulting in PLL lock failure and
> failure of the PCI-e link to come up.
> 
> The registers used in the workaround are based on the latest patch in
> the NXP kernel, but many things around that have been changed.
> 
> This uses a new node of type fsl,imx-pcie-phy to get the PHY's
> registers.  The node is found via a phandle added to the PCI-e
> controller's node, rather than the incorrect way done in the NXP kernel.
> 
> There is no error if the phandle is not preset (since it's needed except
> for the imx7d workaround and no existing dtses have it), but if preset
> it is an error if something relating to it does not work.
> 
> ** Should the node be fsl,imx7d-pcie-phy?  snps,dw-pcie-phy?  
> 
> There is little to no documenation from NXP and Synopsis about this, so I'm
> unsure of the PHY's lineage.
> 
> The imx6 PCI-e driver does not use the generic phy layer to interact
> with the PHY.  It appears PHY related hardware, like clocks, regulators,
> and resets, are part of the fsl,imx6q-pcie node.  But again, the
> topology of this hardware is not documented very well.
> 
> Another approach would be to add the PHY registers as another bank in
> the PCI-e node.  This would match how the PHY reset, clock, etc.  are
> done.  However, the PHY is attached to a different AXI master than the
> PCI-e controller, so the register range really does not belong there.
> 
> Trent Piepho (2):
>   ARM: dts: imx7d: Add node for PCIe PHY
>   PCI: imx: Add workaround for e10728, IMX7d PCIe PLL failure

Marked as "changes requested", following Lucas' review, please
respin as appropriate.

Lorenzo

>  .../devicetree/bindings/pci/fsl,imx6q-pcie.txt     | 11 ++++
>  arch/arm/boot/dts/imx7d.dtsi                       |  9 ++++
>  drivers/pci/dwc/pci-imx6.c                         | 59 ++++++++++++++++++++++
>  3 files changed, 79 insertions(+)
> 
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: Richard Zhu <hongxing.zhu@nxp.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> 
> -- 
> 2.14.4
> 
>