Skip to contentSkip to conversation

phy: don't let controllers change vbus reg

RFC PATCH In review Philippe Simons 2 participants 8 messages Jan 18, 2025 – Feb 21, 2025

Subsystems (3)
Device tree bindings Arm/Allwinner sunXi SoC support Generic PHY framework
lore
Share & tools
lore

Philippe Simons

author
#0 lore raw

RFC PATCH 1/3 phy: don't let controllers change vbus reg

Allwinners SoCs share phy0 between the MUSB controller and HCI controller. If we let these controllers independently power on the vbus on that phy, peripheral mode is dangerous because HCI never power down the phy, resulting in 5v being applied against the host 5v.

Override power_on/off for phy0 in that case, and let regulator be enabled/disabled based on id_det only.

Signed-off-by Philippe Simons <[email protected]>
---
 drivers/phy/allwinner/phy-sun4i-usb.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)
drivers/phy/allwinner/phy-sun4i-usb.c +26 -0
index b0f19e950..24fbabe7a 100644
@@ -465,6 +465,10 @@ static int sun4i_usb_phy_power_on(struct phy *_phy)
 	struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
 	int ret;
 
+	/* phy0 power is controlled by sun4i_usb_phy0_reroute and id_det state */
+	if (phy->index == 0 && data->cfg->phy0_dual_route)
+		return 0;
+
 	if (!phy->vbus || phy->regulator_on)
 		return 0;
 

@@ -493,6 +497,10 @@ static int sun4i_usb_phy_power_off(struct phy *_phy)
 	struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
 	struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
 
+	/* phy0 power is controlled by sun4i_usb_phy0_reroute and id_det state */
+	if (phy->index == 0 && data->cfg->phy0_dual_route)
+		return 0;
+
 	if (!phy->vbus || !phy->regulator_on)
 		return 0;
 

@@ -573,9 +581,21 @@ static void sun4i_usb_phy0_reroute(struct sun4i_usb_phy_data *data, int id_det)
 	if (id_det == 0) {
 		/* Host mode. Route phy0 to EHCI/OHCI */
 		regval &= ~OTGCTL_ROUTE_MUSB;
+
+		/* Enable VBUS reg */
+		if (phy->vbus && !phy->regulator_on) {
+			regulator_enable(phy->vbus);
+			phy->regulator_on = true;
+		}
 	} else {
 		/* Peripheral mode. Route phy0 to MUSB */
 		regval |= OTGCTL_ROUTE_MUSB;
+		
+		/* Disable VBUS reg */
+		if (phy->vbus && phy->regulator_on) {
+			regulator_disable(phy->vbus);
+			phy->regulator_on = false;
+		}
 	}
 	writel(regval, data->base + REG_PHY_OTGCTL);
 }

@@ -603,6 +623,12 @@ static void sun4i_usb_phy0_id_vbus_det_scan(struct work_struct *work)
 		return;
 	}
 
+	/* Disable internal VBUS reg if there is an external VBUS */
+	if (vbus_det != 0 && phy->vbus && phy->regulator_on) {
+		regulator_disable(phy->vbus);
+		phy->regulator_on = false;
+	}
+
 	force_session_end = data->force_session_end;
 	data->force_session_end = false;
Signature · 1 line
2.47.1
author
#1 lore raw

RFC PATCH 2/3 phy: override dr_mode for phy0

PHY0 mode is fully determined by the selected mux route. Ignore changes requests from controllers.

Signed-off-by Philippe Simons <[email protected]>
---
 drivers/phy/allwinner/phy-sun4i-usb.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
drivers/phy/allwinner/phy-sun4i-usb.c +12 -0
index 24fbabe7a..f163b355e 100644
@@ -544,6 +544,18 @@ static int sun4i_usb_phy_set_mode(struct phy *_phy,
 		return -EINVAL;
 	}
 
+	if (data->cfg->phy0_dual_route) {
+		/*
+		 For SoCs with dual route the PHY mode is fully determined by 
+		 the selected mux route (i.e. USB controller to use).
+		 As both host (EHCI/OHCI) and peripheral (MUSB) controllers uses
+		 the same PHY, both drivers can try to set PHY mode.
+		 We need to ignore this requests, but not report error in case
+		 of valid mode values.
+		*/
+		return 0;
+	}
+
 	if (new_mode != data->dr_mode) {
 		dev_info(&_phy->dev, "Changing dr_mode to %d\n", new_mode);
 		data->dr_mode = new_mode;
Signature · 1 line
2.47.1
maintainer
#2 lore raw

Re: [RFC PATCH 2/3] phy: override dr_mode for phy0

Dne sobota, 18. januar 2025 ob 11:22:05 Srednjeevropski standardni čas je Philippe Simons napisal(a):

Quoted 4 lines

PHY0 mode is fully determined by the selected mux route. Ignore changes requests from controllers.

Signed-off-by Philippe Simons <[email protected]>

Some context is missing. Why is this patch needed? Which scenario causes issues without this patch?

Please also provide cover letter to the series, with "big picture" explanation.

Best regards,
Jernej

Quoted 28 lines
---
 drivers/phy/allwinner/phy-sun4i-usb.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
drivers/phy/allwinner/phy-sun4i-usb.c +12 -0
index 24fbabe7a..f163b355e 100644
@@ -544,6 +544,18 @@ static int sun4i_usb_phy_set_mode(struct phy *_phy,
 		return -EINVAL;
 	}
 
+	if (data->cfg->phy0_dual_route) {
+		/*
+		 For SoCs with dual route the PHY mode is fully determined by 
+		 the selected mux route (i.e. USB controller to use).
+		 As both host (EHCI/OHCI) and peripheral (MUSB) controllers uses
+		 the same PHY, both drivers can try to set PHY mode.
+		 We need to ignore this requests, but not report error in case
+		 of valid mode values.
+		*/
+		return 0;
+	}
+
 	if (new_mode != data->dr_mode) {
 		dev_info(&_phy->dev, "Changing dr_mode to %d\n", new_mode);
 		data->dr_mode = new_mode;
author
#3 lore raw

RFC PATCH 3/3 arm64: dts: allwinner: h700: Enable USB OTG

In reply to Philippe Simons

RG35XX have a GPIO controlled regulator for phy0 vbus, add it. Enable HCI0s controllers and otg for dr_mode.
Add phy0 properties to descrive id_det, external vbus, and internal vbus

Signed-off-by Philippe Simons <[email protected]>
---
 .../sun50i-h700-anbernic-rg35xx-2024.dts      | 25 +++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)
arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts +23 -2
index 80ccab7b5..5a6ae42de 100644
@@ -175,6 +175,16 @@ reg_vcc5v: regulator-vcc5v { /* USB-C power input */
 		regulator-min-microvolt = <5000000>;
 		regulator-max-microvolt = <5000000>;
 	};
+
+	reg_usb0_vbus: regulator-usb0-vbus {
+		compatible = "regulator-fixed";
+		enable-active-high;
+		gpio = <&pio 8 16 GPIO_ACTIVE_HIGH>; /* PI16 */
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		regulator-name = "usb0-vbus";
+		vin-supply = <&reg_boost>;
+	};
 };
 
 &cpu0 {

@@ -337,12 +347,23 @@ &uart0 {
 	status = "okay";
 };
 
-/* the AXP717 has USB type-C role switch functionality, not yet described by the binding */
+/* the AXP717 has USB type-C role switch functionality */
 &usbotg {
-	dr_mode = "peripheral";   /* USB type-C receptable */
+	dr_mode = "otg";   /* USB type-C receptable */
+	status = "okay";
+};
+
+&ehci0 {
+	status = "okay";
+};
+
+&ohci0 {
 	status = "okay";
 };
 
 &usbphy {
+	usb0_id_det-gpios = <&pio 8 4 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>; /* PI4 */
+	usb0_vbus_power-supply = <&usb_power>;
+	usb0_vbus-supply = <&reg_usb0_vbus>;
 	status = "okay";
 };
Signature · 1 line
2.47.1
maintainer Reviewed-by on v1 3/3
#4 lore raw

Re: [RFC PATCH 3/3] arm64: dts: allwinner: h700: Enable USB OTG

Dne sobota, 18. januar 2025 ob 11:22:06 Srednjeevropski standardni čas je Philippe Simons napisal(a):

Quoted 5 lines

RG35XX have a GPIO controlled regulator for phy0 vbus, add it. Enable HCI0s controllers and otg for dr_mode.
Add phy0 properties to descrive id_det, external vbus, and internal vbus

Signed-off-by Philippe Simons <[email protected]>

This could work without previous two patches, right?

Reviewed-by Jernej Skrabec <[email protected]>

Best regards,
Jernej

Quoted 52 lines
---
 .../sun50i-h700-anbernic-rg35xx-2024.dts      | 25 +++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)
arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts +23 -2
index 80ccab7b5..5a6ae42de 100644
@@ -175,6 +175,16 @@ reg_vcc5v: regulator-vcc5v { /* USB-C power input */
 		regulator-min-microvolt = <5000000>;
 		regulator-max-microvolt = <5000000>;
 	};
+
+	reg_usb0_vbus: regulator-usb0-vbus {
+		compatible = "regulator-fixed";
+		enable-active-high;
+		gpio = <&pio 8 16 GPIO_ACTIVE_HIGH>; /* PI16 */
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		regulator-name = "usb0-vbus";
+		vin-supply = <&reg_boost>;
+	};
 };
 
 &cpu0 {

@@ -337,12 +347,23 @@ &uart0 {
 	status = "okay";
 };
 
-/* the AXP717 has USB type-C role switch functionality, not yet described by the binding */
+/* the AXP717 has USB type-C role switch functionality */
 &usbotg {
-	dr_mode = "peripheral";   /* USB type-C receptable */
+	dr_mode = "otg";   /* USB type-C receptable */
+	status = "okay";
+};
+
+&ehci0 {
+	status = "okay";
+};
+
+&ohci0 {
 	status = "okay";
 };
 
 &usbphy {
+	usb0_id_det-gpios = <&pio 8 4 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>; /* PI4 */
+	usb0_vbus_power-supply = <&usb_power>;
+	usb0_vbus-supply = <&reg_usb0_vbus>;
 	status = "okay";
 };
author
#5 lore raw

Re: [RFC PATCH 3/3] arm64: dts: allwinner: h700: Enable USB OTG

Quoted 9 lines · Jernej Škrabec

Dne sobota, 18. januar 2025 ob 11:22:06 Srednjeevropski standardni čas je Philippe Simons napisal(a):

Quoted 5 lines

RG35XX have a GPIO controlled regulator for phy0 vbus, add it. Enable HCI0s controllers and otg for dr_mode.
Add phy0 properties to descrive id_det, external vbus, and internal vbus

Signed-off-by Philippe Simons <[email protected]>

This could work without previous two patches, right?

While this correctly describes the board, it currently doesn't works as expected.
HCIs will enable the 5v on PHY and will never disable it. Resulting in a blown regulator on the board if plugged with another host. I managed to get this working by removing the PHY ref from the HCI, but this is wrong.

Quoted 62 lines
Reviewed-by Jernej Skrabec <[email protected]>

Best regards,
Jernej

Quoted 52 lines
---
 .../sun50i-h700-anbernic-rg35xx-2024.dts      | 25 +++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)
arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts +23 -2
index 80ccab7b5..5a6ae42de 100644
@@ -175,6 +175,16 @@ reg_vcc5v: regulator-vcc5v { /* USB-C power input */
              regulator-min-microvolt = <5000000>;
              regulator-max-microvolt = <5000000>;
      };
+
+     reg_usb0_vbus: regulator-usb0-vbus {
+             compatible = "regulator-fixed";
+             enable-active-high;
+             gpio = <&pio 8 16 GPIO_ACTIVE_HIGH>; /* PI16 */
+             regulator-min-microvolt = <5000000>;
+             regulator-max-microvolt = <5000000>;
+             regulator-name = "usb0-vbus";
+             vin-supply = <&reg_boost>;
+     };
 };
 
 &cpu0 {

@@ -337,12 +347,23 @@ &uart0 {
      status = "okay";
 };
 
-/* the AXP717 has USB type-C role switch functionality, not yet described by the binding */
+/* the AXP717 has USB type-C role switch functionality */
 &usbotg {
-     dr_mode = "peripheral";   /* USB type-C receptable */
+     dr_mode = "otg";   /* USB type-C receptable */
+     status = "okay";
+};
+
+&ehci0 {
+     status = "okay";
+};
+
+&ohci0 {
      status = "okay";
 };
 
 &usbphy {
+     usb0_id_det-gpios = <&pio 8 4 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>; /* PI4 */
+     usb0_vbus_power-supply = <&usb_power>;
+     usb0_vbus-supply = <&reg_usb0_vbus>;
      status = "okay";
 };
maintainer
#6 lore raw

Re: [RFC PATCH 3/3] arm64: dts: allwinner: h700: Enable USB OTG

Dne petek, 21. februar 2025 ob 10:54:27 Srednjeevropski standardni čas je Philippe Simons napisal(a):

Quoted 9 lines · Jernej Škrabec
Quoted 8 lines

Dne sobota, 18. januar 2025 ob 11:22:06 Srednjeevropski standardni čas je Philippe Simons napisal(a):

Quoted 5 lines

RG35XX have a GPIO controlled regulator for phy0 vbus, add it. Enable HCI0s controllers and otg for dr_mode.
Add phy0 properties to descrive id_det, external vbus, and internal vbus

Signed-off-by Philippe Simons <[email protected]>
Quoted 8 lines
Quoted 1 line

This could work without previous two patches, right?

While this correctly describes the board, it currently doesn't works as expected.
HCIs will enable the 5v on PHY and will never disable it. Resulting in a blown regulator on the board if plugged with another host. I managed to get this working by removing the PHY ref from the HCI, but this is wrong.

Ok, so this patch shouldn't be merged until USB patches are in.

Best regards,
Jernej

Quoted 64 lines
Quoted 63 lines
Reviewed-by Jernej Skrabec <[email protected]>

Best regards,
Jernej

Quoted 52 lines
---
 .../sun50i-h700-anbernic-rg35xx-2024.dts      | 25 +++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)
arch/arm64/boot/dts/allwinner/sun50i-h700-anbernic-rg35xx-2024.dts +23 -2
index 80ccab7b5..5a6ae42de 100644
@@ -175,6 +175,16 @@ reg_vcc5v: regulator-vcc5v { /* USB-C power input */
              regulator-min-microvolt = <5000000>;
              regulator-max-microvolt = <5000000>;
      };
+
+     reg_usb0_vbus: regulator-usb0-vbus {
+             compatible = "regulator-fixed";
+             enable-active-high;
+             gpio = <&pio 8 16 GPIO_ACTIVE_HIGH>; /* PI16 */
+             regulator-min-microvolt = <5000000>;
+             regulator-max-microvolt = <5000000>;
+             regulator-name = "usb0-vbus";
+             vin-supply = <&reg_boost>;
+     };
 };
 
 &cpu0 {

@@ -337,12 +347,23 @@ &uart0 {
      status = "okay";
 };
 
-/* the AXP717 has USB type-C role switch functionality, not yet described by the binding */
+/* the AXP717 has USB type-C role switch functionality */
 &usbotg {
-     dr_mode = "peripheral";   /* USB type-C receptable */
+     dr_mode = "otg";   /* USB type-C receptable */
+     status = "okay";
+};
+
+&ehci0 {
+     status = "okay";
+};
+
+&ohci0 {
      status = "okay";
 };
 
 &usbphy {
+     usb0_id_det-gpios = <&pio 8 4 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>; /* PI4 */
+     usb0_vbus_power-supply = <&usb_power>;
+     usb0_vbus-supply = <&reg_usb0_vbus>;
      status = "okay";
 };
maintainer
#7 lore raw
In reply to Philippe Simons

Dne sobota, 18. januar 2025 ob 11:22:04 Srednjeevropski standardni čas je Philippe Simons napisal(a):

Quoted 9 lines

Allwinners SoCs share phy0 between the MUSB controller and HCI controller. If we let these controllers independently power on the vbus on that phy, peripheral mode is dangerous because HCI never power down the phy, resulting in 5v being applied against the host 5v.

Override power_on/off for phy0 in that case, and let regulator be enabled/disabled based on id_det only.

Signed-off-by Philippe Simons <[email protected]>

Is this fix for some observed issue? If so, Fixes tag would be appropriate.

Quoted 31 lines
---
 drivers/phy/allwinner/phy-sun4i-usb.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)
drivers/phy/allwinner/phy-sun4i-usb.c +8 -0
index b0f19e950..24fbabe7a 100644
@@ -465,6 +465,10 @@ static int sun4i_usb_phy_power_on(struct phy *_phy)
 	struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
 	int ret;
 
+	/* phy0 power is controlled by sun4i_usb_phy0_reroute and id_det state */
+	if (phy->index == 0 && data->cfg->phy0_dual_route)
+		return 0;
+
 	if (!phy->vbus || phy->regulator_on)
 		return 0;
 

@@ -493,6 +497,10 @@ static int sun4i_usb_phy_power_off(struct phy *_phy)
 	struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
 	struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
 
+	/* phy0 power is controlled by sun4i_usb_phy0_reroute and id_det state */
+	if (phy->index == 0 && data->cfg->phy0_dual_route)
+		return 0;
+
 	if (!phy->vbus || !phy->regulator_on)
 		return 0;

drivers/phy/allwinner/phy-sun4i-usb.c static void sun4i_usb_phy0_reroute(struct sun4i_usb_phy_data *data, int id_det) · line 586

Quoted 6 lines
drivers/phy/allwinner/phy-sun4i-usb.c +3 -0
@@ -573,9 +581,21 @@ static void sun4i_usb_phy0_reroute(struct sun4i_usb_phy_data *data, int id_det)
 	if (id_det == 0) {
 		/* Host mode. Route phy0 to EHCI/OHCI */
 		regval &= ~OTGCTL_ROUTE_MUSB;
+
+		/* Enable VBUS reg */
+		if (phy->vbus && !phy->regulator_on) {

I don't see any "phy" variable in this function. Also, you should add same protection as it is implemented in sun4i_usb_phy_power_on(). Skip enabling regulator if:

sun4i_usb_phy0_have_vbus_det(data) && data->vbus_det

Quoted 16 lines
drivers/phy/allwinner/phy-sun4i-usb.c +9 -0
+			regulator_enable(phy->vbus);
+			phy->regulator_on = true;
+		}
 	} else {
 		/* Peripheral mode. Route phy0 to MUSB */
 		regval |= OTGCTL_ROUTE_MUSB;
+		
+		/* Disable VBUS reg */
+		if (phy->vbus && phy->regulator_on) {
+			regulator_disable(phy->vbus);
+			phy->regulator_on = false;
+		}
 	}
 	writel(regval, data->base + REG_PHY_OTGCTL);
 }

drivers/phy/allwinner/phy-sun4i-usb.c static void sun4i_usb_phy0_id_vbus_det_scan(struct work_struct *work) · line 627

Quoted 5 lines
drivers/phy/allwinner/phy-sun4i-usb.c +2 -0
@@ -603,6 +623,12 @@ static void sun4i_usb_phy0_id_vbus_det_scan(struct work_struct *work)
 		return;
 	}
 
+	/* Disable internal VBUS reg if there is an external VBUS */
+	if (vbus_det != 0 && phy->vbus && phy->regulator_on) {

Ideally, this should never happen.

Best regards,
Jernej

Quoted 8 lines
drivers/phy/allwinner/phy-sun4i-usb.c +4 -0
+		regulator_disable(phy->vbus);
+		phy->regulator_on = false;
+	}
+
 	force_session_end = data->force_session_end;
 	data->force_session_end = false;