Skip to contentSkip to conversation

ARM: imx: Explicitly pass the active level of the PHY reset

PATCH New Fabio Estevam 4 participants 8 messages Jan 3, 2014 – Jan 6, 2014

Subsystems (2)
Device tree bindings Arm i.MX and Layerscape
lore
Share & tools
lore

Fabio Estevam

author reviewer
#0 lore raw

PATCH 1/1 ARM: imx: Explicitly pass the active level of the PHY reset

From: Fabio Estevam <[email protected]>

Since commit 7a399e3a2e (fec: Do not assume that PHY reset is active low), we no longer assume that the PHY reset is active low.

Now we need to retrieve this information from the device tree instead.

Passing '0' as the phy-reset-gpios means 'active high' according to the definition from include/dt-bindings/gpio/gpio.h:

#define GPIO_ACTIVE_HIGH 0
#define GPIO_ACTIVE_LOW 1

, so in order to keep things working as before we need to explicitly pass GPIO_ACTIVE_LOW as the flag.

Signed-off-by Fabio Estevam <[email protected]>

---
Olof,

This is needed for 3.13 as 7a399e3a2e (fec: Do not assume that PHY reset is active low) has already reached mainline.

This patch was generated against linus' tree.

 arch/arm/boot/dts/imx27-phytec-phycore-som.dts | 2 +-
 arch/arm/boot/dts/imx27.dtsi                   | 1 +
 arch/arm/boot/dts/imx28-apf28.dts              | 2 +-
 arch/arm/boot/dts/imx28-apf28dev.dts           | 2 +-
 arch/arm/boot/dts/imx28-cfa10037.dts           | 2 +-
 arch/arm/boot/dts/imx28-cfa10049.dts           | 2 +-
 arch/arm/boot/dts/imx28-cfa10057.dts           | 2 +-
 arch/arm/boot/dts/imx28-cfa10058.dts           | 2 +-
 arch/arm/boot/dts/imx28-evk.dts                | 2 +-
 arch/arm/boot/dts/imx28-m28cu3.dts             | 2 +-
 arch/arm/boot/dts/imx28.dtsi                   | 1 +
 arch/arm/boot/dts/imx51-apf51.dts              | 2 +-
 arch/arm/boot/dts/imx51.dtsi                   | 1 +
 arch/arm/boot/dts/imx53-evk.dts                | 2 +-
 arch/arm/boot/dts/imx53-mba53.dts              | 2 +-
 arch/arm/boot/dts/imx53-qsb.dts                | 2 +-
 arch/arm/boot/dts/imx53-smd.dts                | 2 +-
 arch/arm/boot/dts/imx53.dtsi                   | 1 +
 arch/arm/boot/dts/imx6q-phytec-pfla02.dtsi     | 2 +-
 arch/arm/boot/dts/imx6q-sabrelite.dts          | 2 +-
 arch/arm/boot/dts/imx6qdl-sabresd.dtsi         | 2 +-
 arch/arm/boot/dts/imx6qdl-wandboard.dtsi       | 2 +-
 arch/arm/boot/dts/imx6qdl.dtsi                 | 1 +
 23 files changed, 23 insertions(+), 18 deletions(-)
arch/arm/boot/dts/imx27-phytec-phycore-som.dts +1 -1
index 4ec402c..49b5e67 100644
@@ -134,7 +134,7 @@
 };
 
 &fec {
-	phy-reset-gpios = <&gpio3 30 0>;
+	phy-reset-gpios = <&gpio3 30 GPIO_ACTIVE_LOW>;
 	status = "okay";
 };
arch/arm/boot/dts/imx27.dtsi +1 -0
index 826231e..5708e42 100644
@@ -9,6 +9,7 @@
  * http://www.gnu.org/copyleft/gpl.html
  */
 
+#include <dt-bindings/gpio/gpio.h>
 #include "skeleton.dtsi"
 
 / {
arch/arm/boot/dts/imx28-apf28.dts +1 -1
index 7198fe3..070e59c 100644
@@ -78,7 +78,7 @@
 			phy-mode = "rmii";
 			pinctrl-names = "default";
 			pinctrl-0 = <&mac0_pins_a>;
-			phy-reset-gpios = <&gpio4 13 0>;
+			phy-reset-gpios = <&gpio4 13 GPIO_ACTIVE_LOW>;
 			status = "okay";
 		};
 	};
arch/arm/boot/dts/imx28-apf28dev.dts +1 -1
index e2efd8d..f47a30a 100644
@@ -143,7 +143,7 @@
 			phy-mode = "rmii";
 			pinctrl-names = "default";
 			pinctrl-0 = <&mac1_pins_a>;
-			phy-reset-gpios = <&gpio0 23 0>;
+			phy-reset-gpios = <&gpio0 23 GPIO_ACTIVE_LOW>;
 			status = "okay";
 		};
 	};
arch/arm/boot/dts/imx28-cfa10037.dts +1 -1
index f93e9a7..b2bbd21 100644
@@ -64,7 +64,7 @@
 			pinctrl-names = "default";
 			pinctrl-0 = <&mac0_pins_a
 				&mac0_pins_cfa10037>;
-			phy-reset-gpios = <&gpio2 21 0>;
+			phy-reset-gpios = <&gpio2 21 GPIO_ACTIVE_LOW>;
 			phy-reset-duration = <100>;
 			status = "okay";
 		};
arch/arm/boot/dts/imx28-cfa10049.dts +1 -1
index 7087b4b..f57b245 100644
@@ -300,7 +300,7 @@
 			pinctrl-names = "default";
 			pinctrl-0 = <&mac0_pins_a
 				&mac0_pins_cfa10049>;
-			phy-reset-gpios = <&gpio2 21 0>;
+			phy-reset-gpios = <&gpio2 21 GPIO_ACTIVE_LOW>;
 			phy-reset-duration = <100>;
 			status = "okay";
 		};
arch/arm/boot/dts/imx28-cfa10057.dts +1 -1
index 3c13128..fb6c6e7 100644
@@ -159,7 +159,7 @@
 			phy-mode = "rmii";
 			pinctrl-names = "default";
 			pinctrl-0 = <&mac0_pins_a>;
-			phy-reset-gpios = <&gpio2 21 0>;
+			phy-reset-gpios = <&gpio2 21 GPIO_ACTIVE_LOW>;
 			phy-reset-duration = <100>;
 			status = "okay";
 		};
arch/arm/boot/dts/imx28-cfa10058.dts +1 -1
index 2469d34..ba0a625 100644
@@ -126,7 +126,7 @@
 			phy-mode = "rmii";
 			pinctrl-names = "default";
 			pinctrl-0 = <&mac0_pins_a>;
-			phy-reset-gpios = <&gpio2 21 0>;
+			phy-reset-gpios = <&gpio2 21 GPIO_ACTIVE_LOW>;
 			phy-reset-duration = <100>;
 			status = "okay";
 		};
arch/arm/boot/dts/imx28-evk.dts +1 -1
index 4267c2b..fef39a4 100644
@@ -263,7 +263,7 @@
 			pinctrl-names = "default";
 			pinctrl-0 = <&mac0_pins_a>;
 			phy-supply = <&reg_fec_3v3>;
-			phy-reset-gpios = <&gpio4 13 0>;
+			phy-reset-gpios = <&gpio4 13 GPIO_ACTIVE_LOW>;
 			phy-reset-duration = <100>;
 			status = "okay";
 		};
arch/arm/boot/dts/imx28-m28cu3.dts +1 -1
index d3958da..82cb4f8 100644
@@ -189,7 +189,7 @@
 			phy-mode = "rmii";
 			pinctrl-names = "default";
 			pinctrl-0 = <&mac0_pins_a>;
-			phy-reset-gpios = <&gpio4 13 0>;
+			phy-reset-gpios = <&gpio4 13 GPIO_ACTIVE_LOW>;
 			phy-reset-duration = <100>;
 			status = "okay";
 		};
arch/arm/boot/dts/imx28.dtsi +1 -0
index cda19c8..48529cb 100644
@@ -9,6 +9,7 @@
  * http://www.gnu.org/copyleft/gpl.html
  */
 
+#include <dt-bindings/gpio/gpio.h>
 #include "skeleton.dtsi"
 #include "imx28-pinfunc.h"
arch/arm/boot/dts/imx51-apf51.dts +1 -1
index b360699..3eae080 100644
@@ -36,7 +36,7 @@
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_fec_2>;
 	phy-mode = "mii";
-	phy-reset-gpios = <&gpio3 0 0>;
+	phy-reset-gpios = <&gpio3 0 GPIO_ACTIVE_LOW>;
 	phy-reset-duration = <1>;
 	status = "okay";
 };
arch/arm/boot/dts/imx51.dtsi +1 -0
index 4bcdd3a..f1ab0e2 100644
@@ -10,6 +10,7 @@
  * http://www.gnu.org/copyleft/gpl.html
  */
 
+#include <dt-bindings/gpio/gpio.h>
 #include "skeleton.dtsi"
 #include "imx51-pinfunc.h"
arch/arm/boot/dts/imx53-evk.dts +1 -1
index 801fda7..5ee3d7d 100644
@@ -121,6 +121,6 @@
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_fec_1>;
 	phy-mode = "rmii";
-	phy-reset-gpios = <&gpio7 6 0>;
+	phy-reset-gpios = <&gpio7 6 GPIO_ACTIVE_LOW>;
 	status = "okay";
 };
arch/arm/boot/dts/imx53-mba53.dts +1 -1
index a630902..d57e43e 100644
@@ -175,7 +175,7 @@
 };
 
 &fec {
-	phy-reset-gpios = <&gpio7 6 0>;
+	phy-reset-gpios = <&gpio7 6 GPIO_ACTIVE_LOW>;
 	status = "okay";
 };
arch/arm/boot/dts/imx53-qsb.dts +1 -1
index 91a5935..3e655e9 100644
@@ -303,7 +303,7 @@
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_fec_1>;
 	phy-mode = "rmii";
-	phy-reset-gpios = <&gpio7 6 0>;
+	phy-reset-gpios = <&gpio7 6 GPIO_ACTIVE_LOW>;
 	status = "okay";
 };
arch/arm/boot/dts/imx53-smd.dts +1 -1
index a9b6e10..91dc916 100644
@@ -177,6 +177,6 @@
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_fec_1>;
 	phy-mode = "rmii";
-	phy-reset-gpios = <&gpio7 6 0>;
+	phy-reset-gpios = <&gpio7 6 GPIO_ACTIVE_LOW>;
 	status = "okay";
 };
arch/arm/boot/dts/imx53.dtsi +1 -0
index 4307e80..abf1101 100644
@@ -10,6 +10,7 @@
  * http://www.gnu.org/copyleft/gpl.html
  */
 
+#include <dt-bindings/gpio/gpio.h>
 #include "skeleton.dtsi"
 #include "imx53-pinfunc.h"
arch/arm/boot/dts/imx6q-phytec-pfla02.dtsi +1 -1
index 1a3b50d..3e28d13 100644
@@ -152,7 +152,7 @@
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_enet_3>;
 	phy-mode = "rgmii";
-	phy-reset-gpios = <&gpio3 23 0>;
+	phy-reset-gpios = <&gpio3 23 GPIO_ACTIVE_LOW>;
 	status = "disabled";
 };
arch/arm/boot/dts/imx6q-sabrelite.dts +1 -1
index f004913..e44a69b 100644
@@ -89,7 +89,7 @@
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_enet_1>;
 	phy-mode = "rgmii";
-	phy-reset-gpios = <&gpio3 23 0>;
+	phy-reset-gpios = <&gpio3 23 GPIO_ACTIVE_LOW>;
 	status = "okay";
 };
arch/arm/boot/dts/imx6qdl-sabresd.dtsi +1 -1
index e75e11b..b4a4de5 100644
@@ -116,7 +116,7 @@
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_enet_1>;
 	phy-mode = "rgmii";
-	phy-reset-gpios = <&gpio1 25 0>;
+	phy-reset-gpios = <&gpio1 25 GPIO_ACTIVE_LOW>;
 	status = "okay";
 };
arch/arm/boot/dts/imx6qdl-wandboard.dtsi +1 -1
index 35f5479..773504c 100644
@@ -98,7 +98,7 @@
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_enet_1>;
 	phy-mode = "rgmii";
-	phy-reset-gpios = <&gpio3 29 0>;
+	phy-reset-gpios = <&gpio3 29 GPIO_ACTIVE_LOW>;
 	status = "okay";
 };
arch/arm/boot/dts/imx6qdl.dtsi +1 -0
index fb28b2e..681b07e 100644
@@ -10,6 +10,7 @@
  * http://www.gnu.org/copyleft/gpl.html
  */
 
+#include <dt-bindings/gpio/gpio.h>
 #include "skeleton.dtsi"
 
 / {
Signature · 1 line
1.8.1.2
#1 lore raw

PATCH 1/1 ARM: imx: Explicitly pass the active level of the PHY reset

Quoted 10 lines · Fabio Estevam

From: Fabio Estevam <[email protected]>

Since commit 7a399e3a2e (fec: Do not assume that PHY reset is active low), we no longer assume that the PHY reset is active low.

Now we need to retrieve this information from the device tree instead.

Passing '0' as the phy-reset-gpios means 'active high' according to the definition from include/dt-bindings/gpio/gpio.h:

Quoted 7 lines

#define GPIO_ACTIVE_HIGH 0
#define GPIO_ACTIVE_LOW 1

, so in order to keep things working as before we need to explicitly pass GPIO_ACTIVE_LOW as the flag.

Signed-off-by Fabio Estevam <[email protected]>
Acked-by Shawn Guo <[email protected]>
Quoted 7 lines

---
Olof,

This is needed for 3.13 as 7a399e3a2e (fec: Do not assume that PHY reset is active low) has already reached mainline.

This patch was generated against linus' tree.

Applied
#2 lore raw

PATCH 1/1 ARM: imx: Explicitly pass the active level of the PHY reset

Quoted 14 lines · Shawn Guo
Quoted 13 lines · Fabio Estevam

From: Fabio Estevam <[email protected]>

Since commit 7a399e3a2e (fec: Do not assume that PHY reset is active low), we no longer assume that the PHY reset is active low.

Now we need to retrieve this information from the device tree instead.

Passing '0' as the phy-reset-gpios means 'active high' according to the definition from include/dt-bindings/gpio/gpio.h:

#define GPIO_ACTIVE_HIGH 0
#define GPIO_ACTIVE_LOW 1

Quoted 7 lines
Quoted 4 lines

, so in order to keep things working as before we need to explicitly pass GPIO_ACTIVE_LOW as the flag.

Signed-off-by Fabio Estevam <[email protected]>
Acked-by Shawn Guo <[email protected]>

Applied to fixes. Thanks.

-Olof

#3 lore raw

PATCH 1/1 ARM: imx: Explicitly pass the active level of the PHY reset

Quoted 18 lines · Olof Johansson
Quoted 17 lines · Shawn Guo
Quoted 16 lines · Fabio Estevam

From: Fabio Estevam <[email protected]>

Since commit 7a399e3a2e (fec: Do not assume that PHY reset is active low), we no longer assume that the PHY reset is active low.

Now we need to retrieve this information from the device tree instead.

Passing '0' as the phy-reset-gpios means 'active high' according to the definition from include/dt-bindings/gpio/gpio.h:

#define GPIO_ACTIVE_HIGH 0
#define GPIO_ACTIVE_LOW 1

, so in order to keep things working as before we need to explicitly pass GPIO_ACTIVE_LOW as the flag.

Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Fabio Estevam <[email protected]>
Acked-by Shawn Guo <[email protected]>

Applied to fixes. Thanks.

Actually, Russell brought this to my attention (I should have looked closer earlier). You guys have made an incompatible binding change here. In -rc6? That's not cool, we're not doing those any more.

What should happen is that the driver change should be reverted instead. You can't rely on every device tree out there changing to accommodate these kind of things any more.

I'll drop the fix from the DT side.

-Olof

#4 lore raw

PATCH 1/1 ARM: imx: Explicitly pass the active level of the PHY reset

Quoted 3 lines · Olof Johansson

Actually, Russell brought this to my attention (I should have looked closer earlier). You guys have made an incompatible binding change here. In -rc6? That's not cool, we're not doing those any more.

Yes, the driver change hit mainline too quickly.

Quoted 4 lines

What should happen is that the driver change should be reverted instead. You can't rely on every device tree out there changing to accommodate these kind of things any more.

Fabio just sent a revert patch for it, and David has queued it.

This is a typical example that we did not implement device tree bindings perfectly right in the first place, and we have to live with it forever. If someday there is board design using an active high PHY reset, we basically have to introduce another property for it. But it does no harm to correct the level type of the existing property in device tree sources anyway. So I will still queue Fabio's dts patch [1] for 3.14 - a different one from what you're dropping here. The patch [1] is against my for-next branch while the dropped one is against mainline.

Shawn

[1] http://www.spinics.net/lists/arm-kernel/msg297829.html

#5 lore raw

PATCH 1/1 ARM: imx: Explicitly pass the active level of the PHY reset

Quoted 12 lines · Shawn Guo
Quoted 3 lines · Olof Johansson

Actually, Russell brought this to my attention (I should have looked closer earlier). You guys have made an incompatible binding change here. In -rc6? That's not cool, we're not doing those any more.

Yes, the driver change hit mainline too quickly.

Quoted 4 lines

What should happen is that the driver change should be reverted instead. You can't rely on every device tree out there changing to accommodate these kind of things any more.

Quoted 6 lines

Fabio just sent a revert patch for it, and David has queued it.

This is a typical example that we did not implement device tree bindings perfectly right in the first place, and we have to live with it forever. If someday there is board design using an active high PHY reset, we basically have to introduce another property for it.

Yes, or another, more precise, binding name.

Quoted 6 lines

But
it does no harm to correct the level type of the existing property in device tree sources anyway. So I will still queue Fabio's dts patch [1] for 3.14 - a different one from what you're dropping here. The patch [1] is against my for-next branch while the dropped one is against mainline.

Sounds good. Thanks!

-Olof

#6 lore raw

PATCH 1/1 ARM: imx: Explicitly pass the active level of the PHY reset

In reply to Shawn Guo
Quoted 4 lines · Shawn Guo

This is a typical example that we did not implement device tree bindings perfectly right in the first place, and we have to live with it forever. If someday there is board design using an active high PHY reset, we basically have to introduce another property for it.

Yes and no. That third number in the property is always zero at present, right? If so, just define zero to mean "the default level of reset" which would be the -rc6 behaviour.

Then you can define non-zero values to mean "explicitly state the level".

Signature · 3 lines
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".
#7 lore raw

PATCH 1/1 ARM: imx: Explicitly pass the active level of the PHY reset

Quoted 11 lines · Russell King - ARM Linux
Quoted 4 lines · Shawn Guo

This is a typical example that we did not implement device tree bindings perfectly right in the first place, and we have to live with it forever. If someday there is board design using an active high PHY reset, we basically have to introduce another property for it.

Yes and no. That third number in the property is always zero at present, right? If so, just define zero to mean "the default level of reset" which would be the -rc6 behaviour.

Then you can define non-zero values to mean "explicitly state the level".

This is not nice either, because the third number in *-gpios property is already defined by gpio bindings:

Documentation/devicetree/bindings/gpio/gpio.txt Documentation/devicetree/bindings/gpio/fsl-imx-gpio.txt

Shawn