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

Message ID 20180718194424.8844-2-tpiepho@impinj.com
State New
Delegated to: Lorenzo Pieralisi
Headers show
Series
  • Workaround for IMX7d PCI-e PLL lock failure
Related show

Commit Message

Trent Piepho July 18, 2018, 7:44 p.m.
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(+)

Comments

Shawn Guo July 19, 2018, 3:24 a.m. | #1
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@306d0000 {
> +		  compatible = "fsl,imx-pcie-phy";
> +		  reg = <0x306d0000 0x10000>;
> +		  status = "disabled";
> +	};
> +};
> +
>  &aips3 {
>  	usbotg2: usb@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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Lucas Stach Aug. 1, 2018, 10:39 a.m. | #2
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@306d0000 {
> > +		  compatible = "fsl,imx-pcie-phy";
> > +		  reg = <0x306d0000 0x10000>;
> > +		  status = "disabled";
> > +	};
> +};
> +
>  &aips3 {
> > >  	usbotg2: usb@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>;
> >  	};
>  };
>
Trent Piepho Aug. 1, 2018, 5:33 p.m. | #3
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.
Richard Zhu Aug. 2, 2018, 12:43 a.m. | #4
> -----Original Message-----
> From: Trent Piepho [mailto:tpiepho@impinj.com]
> Sent: Thursday, August 2, 2018 1:34 AM
> To: l.stach@pengutronix.de; linux-arm-kernel@lists.infradead.org;
> linux-pci@vger.kernel.org
> Cc: Richard Zhu <hongxing.zhu@nxp.com>; shawnguo@kernel.org;
> kernel@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@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.
Lucas Stach Aug. 2, 2018, 6:55 a.m. | #5
Am Donnerstag, den 02.08.2018, 00:43 +0000 schrieb Richard Zhu:
> > -----Original Message-----
> > > > From: Trent Piepho [mailto:tpiepho@impinj.com]
> > Sent: Thursday, August 2, 2018 1:34 AM
> > > > > > To: l.stach@pengutronix.de; linux-arm-kernel@lists.infradead.org;
> > linux-pci@vger.kernel.org
> > > > > > Cc: Richard Zhu <hongxing.zhu@nxp.com>; shawnguo@kernel.org;
> > > > kernel@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@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

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@306d0000 {
+		  compatible = "fsl,imx-pcie-phy";
+		  reg = <0x306d0000 0x10000>;
+		  status = "disabled";
+	};
+};
+
 &aips3 {
 	usbotg2: usb@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>;
 	};
 };