ARM: dts: imx51-babbage: Fix ULPI PHY reset modelling

PATCH New Fabio Estevam 4 participants 8 messages Jan 5, 2015 – Jan 6, 2015

Subsystems (2)

Device tree bindings Arm i.MX and Layerscape

lore

Share & tools

lore

Fabio Estevam

author

2015-01-05 17:19 UTC #0 lore raw

PATCH 1/1 ARM: dts: imx51-babbage: Fix ULPI PHY reset modelling

GPIO2_5 is the reset GPIO for the USB3317 ULPI PHY. Instead of modelling it as a regulator, the correct approach is to use the 'reset_gpios' property of the "usb-nop-xceiv" node.

GPIO1_7 is the reset GPIO for the USB2517 USB hub. As we currently don't have dt bindings to describe a HUB reset, let's keep using the regulator approach.

Rename the regulator to 'reg_hub_reset' to better describe its function and bind it with the USB host1 port instead.

USB host support has been introduced by commit 9bf206a9d13be3 ("ARM: dts:
imx51-babbage: Add USB Host1 support"), which landed in 3.16 and it seems that USB has only been functional due to previous bootloader initialization.

With this patch applied we can get USB host to work without relying on the bootloader.

Cc <[email protected]> # 3.16+

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

---
 arch/arm/boot/dts/imx51-babbage.dts | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

arch/arm/boot/dts/imx51-babbage.dts +5 ~~-17~~

index 56569ce..649befe 100644
@@ -127,24 +127,12 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 
-		reg_usbh1_vbus: regulator at 0 {
-			compatible = "regulator-fixed";
-			pinctrl-names = "default";
-			pinctrl-0 = <&pinctrl_usbh1reg>;
-			reg = <0>;
-			regulator-name = "usbh1_vbus";
-			regulator-min-microvolt = <5000000>;
-			regulator-max-microvolt = <5000000>;
-			gpio = <&gpio2 5 GPIO_ACTIVE_HIGH>;
-			enable-active-high;
-		};
-
-		reg_usbotg_vbus: regulator at 1 {
+		reg_hub_reset: regulator at 0 {
 			compatible = "regulator-fixed";
 			pinctrl-names = "default";
 			pinctrl-0 = <&pinctrl_usbotgreg>;
-			reg = <1>;
-			regulator-name = "usbotg_vbus";
+			reg = <0>;
+			regulator-name = "hub_reset";
 			regulator-min-microvolt = <5000000>;
 			regulator-max-microvolt = <5000000>;
 			gpio = <&gpio1 7 GPIO_ACTIVE_HIGH>;

@@ -176,6 +164,7 @@
 			reg = <0>;
 			clocks = <&clks IMX5_CLK_DUMMY>;
 			clock-names = "main_clk";
+			reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
 		};
 	};
 };

@@ -419,7 +408,7 @@
 &usbh1 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_usbh1>;
-	vbus-supply = <&reg_usbh1_vbus>;
+	vbus-supply = <&reg_hub_reset>;
 	fsl,usbphy = <&usbh1phy>;
 	phy_type = "ulpi";
 	status = "okay";

@@ -429,7 +418,6 @@
 	dr_mode = "otg";
 	disable-over-current;
 	phy_type = "utmi_wide";
-	vbus-supply = <&reg_usbotg_vbus>;
 	status = "okay";
 };

Signature · 1 line

1.9.1

Shawn Guo

Applied

2015-01-06 12:43 UTC #1 lore raw

PATCH 1/1 ARM: dts: imx51-babbage: Fix ULPI PHY reset modelling

Quoted 14 lines · Fabio Estevam

GPIO2_5 is the reset GPIO for the USB3317 ULPI PHY. Instead of modelling it as a regulator, the correct approach is to use the 'reset_gpios' property of the "usb-nop-xceiv" node.
GPIO1_7 is the reset GPIO for the USB2517 USB hub. As we currently don't have dt bindings to describe a HUB reset, let's keep using the regulator approach.
Rename the regulator to 'reg_hub_reset' to better describe its function and bind it with the USB host1 port instead.
USB host support has been introduced by commit 9bf206a9d13be3 ("ARM: dts:
imx51-babbage: Add USB Host1 support"), which landed in 3.16 and it seems that USB has only been functional due to previous bootloader initialization.

Quoted 5 lines

With this patch applied we can get USB host to work without relying on the bootloader.
Cc <[email protected]> # 3.16+
Signed-off-by Fabio Estevam <[email protected]>

Applied, thanks.

Lucas Stach

2015-01-06 13:36 UTC #2 lore raw

PATCH 1/1 ARM: dts: imx51-babbage: Fix ULPI PHY reset modelling

Am Dienstag, den 06.01.2015, 20:43 +0800 schrieb Shawn Guo:

Quoted 15 lines · Fabio Estevam

Quoted 14 lines
GPIO2_5 is the reset GPIO for the USB3317 ULPI PHY. Instead of modelling it as a regulator, the correct approach is to use the 'reset_gpios' property of the "usb-nop-xceiv" node.
GPIO1_7 is the reset GPIO for the USB2517 USB hub. As we currently don't have dt bindings to describe a HUB reset, let's keep using the regulator approach.
Rename the regulator to 'reg_hub_reset' to better describe its function and bind it with the USB host1 port instead.
USB host support has been introduced by commit 9bf206a9d13be3 ("ARM: dts:
imx51-babbage: Add USB Host1 support"), which landed in 3.16 and it seems that USB has only been functional due to previous bootloader initialization.

Quoted 8 lines

Quoted 5 lines
With this patch applied we can get USB host to work without relying on the bootloader.
Cc <[email protected]> # 3.16+
Signed-off-by Fabio Estevam <[email protected]>
Applied, thanks.

For the record:
I'm not happy with this patch. It still abuses the regulator API for something completely unrelated and this may well break again in the future. But given that it fixes an actual regression and you already sent a pull including this I'm okay to keep it.

We still need a proper fix to handle ULPI resets to get rid of this hackery.

Regards,
Lucas

Signature · 2 lines

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

Shawn Guo

2015-01-06 13:41 UTC #3 lore raw

PATCH 1/1 ARM: dts: imx51-babbage: Fix ULPI PHY reset modelling

Quoted 24 lines · Lucas Stach

Am Dienstag, den 06.01.2015, 20:43 +0800 schrieb Shawn Guo:
Quoted 15 lines · Fabio Estevam
Quoted 14 lines
GPIO2_5 is the reset GPIO for the USB3317 ULPI PHY. Instead of modelling it as a regulator, the correct approach is to use the 'reset_gpios' property of the "usb-nop-xceiv" node.
GPIO1_7 is the reset GPIO for the USB2517 USB hub. As we currently don't have dt bindings to describe a HUB reset, let's keep using the regulator approach.
Rename the regulator to 'reg_hub_reset' to better describe its function and bind it with the USB host1 port instead.
USB host support has been introduced by commit 9bf206a9d13be3 ("ARM: dts:
imx51-babbage: Add USB Host1 support"), which landed in 3.16 and it seems that USB has only been functional due to previous bootloader initialization.
Quoted 8 lines
Quoted 5 lines
With this patch applied we can get USB host to work without relying on the bootloader.
Cc <[email protected]> # 3.16+
Signed-off-by Fabio Estevam <[email protected]>
Applied, thanks.

Quoted 8 lines

For the record:
I'm not happy with this patch. It still abuses the regulator API for something completely unrelated and this may well break again in the future. But given that it fixes an actual regression and you already sent a pull including this I'm okay to keep it.
We still need a proper fix to handle ULPI resets to get rid of this hackery.

Yeah, at least this fix patch removes one user of the regulator hackery.

Shawn

Fabio Estevam

reviewer

2015-01-06 13:47 UTC #4 lore raw

PATCH 1/1 ARM: dts: imx51-babbage: Fix ULPI PHY reset modelling

In reply to Lucas Stach

Quoted 8 lines · Lucas Stach

For the record:
I'm not happy with this patch. It still abuses the regulator API for something completely unrelated and this may well break again in the future. But given that it fixes an actual regression and you already sent a pull including this I'm okay to keep it.
We still need a proper fix to handle ULPI resets to get rid of this hackery.

Actually the ULPI PHY reset is properly handled now. What is missing is the USB Hub reset to be properly handled.

According to the USB maintainer, Alan Stern, a proper solution has not come up yet:
http://www.spinics.net/lists/linux-usb/msg97483.html

When a proper USB hub reset solution shows up, we can certainly use it, but for now let's have USB host functional by using the regulator API hack.

Lucas Stach

2015-01-06 13:54 UTC #5 lore raw

PATCH 1/1 ARM: dts: imx51-babbage: Fix ULPI PHY reset modelling

Am Dienstag, den 06.01.2015, 11:47 -0200 schrieb Fabio Estevam:

Quoted 8 lines · Lucas Stach

Quoted 7 lines
For the record:
I'm not happy with this patch. It still abuses the regulator API for something completely unrelated and this may well break again in the future. But given that it fixes an actual regression and you already sent a pull including this I'm okay to keep it.

Quoted 6 lines

Quoted 2 lines
We still need a proper fix to handle ULPI resets to get rid of this hackery.
Actually the ULPI PHY reset is properly handled now. What is missing is the USB Hub reset to be properly handled.

No it is not. A regulator isn't the proper abstraction for a reset at all. Even if you (ab)use the regulator drivers ability to toggle a reset GPIO.
A ULPI PHY reset needs to be modeled as a GPIO reset attached to USB controller node. There is no other way to describe the hardware accurately.

Regards,
Lucas

Signature · 2 lines

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

Fabio Estevam

reviewer

2015-01-06 13:59 UTC #6 lore raw

PATCH 1/1 ARM: dts: imx51-babbage: Fix ULPI PHY reset modelling

Quoted 9 lines · Lucas Stach

Quoted 3 lines
Actually the ULPI PHY reset is properly handled now. What is missing is the USB Hub reset to be properly handled.
No it is not. A regulator isn't the proper abstraction for a reset at all. Even if you (ab)use the regulator drivers ability to toggle a reset GPIO.
A ULPI PHY reset needs to be modeled as a GPIO reset attached to USB controller node. There is no other way to describe the hardware accurately.

The ULPI PHY is modelled as a GPIO reset via reset-gpios property. This is documented at Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt:

- reset-gpios: Should specify the GPIO for reset.

This is the property we use right now for resetting the ULPI PHY.

Lucas Stach

2015-01-06 14:03 UTC #7 lore raw

PATCH 1/1 ARM: dts: imx51-babbage: Fix ULPI PHY reset modelling

Am Dienstag, den 06.01.2015, 11:59 -0200 schrieb Fabio Estevam:

Quoted 12 lines · Lucas Stach

Quoted 10 lines
Quoted 4 lines
Actually the ULPI PHY reset is properly handled now. What is missing is the USB Hub reset to be properly handled.
No it is not. A regulator isn't the proper abstraction for a reset at all. Even if you (ab)use the regulator drivers ability to toggle a reset GPIO.
A ULPI PHY reset needs to be modeled as a GPIO reset attached to USB controller node. There is no other way to describe the hardware accurately.

Quoted 7 lines

The ULPI PHY is modelled as a GPIO reset via reset-gpios property. This is documented at Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt:
- reset-gpios: Should specify the GPIO for reset.
This is the property we use right now for resetting the ULPI PHY.

Right. I misread that part of the patch. Sorry about that.

Regards,
Lucas

Signature · 2 lines

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