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

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

[Philippe Simons]

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 <simons.philippe@gmail.com>
---
 drivers/phy/allwinner/phy-sun4i-usb.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c
index b0f19e950..24fbabe7a 100644
--- a/drivers/phy/allwinner/phy-sun4i-usb.c
+++ b/drivers/phy/allwinner/phy-sun4i-usb.c
@@ -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;
 
-- 
2.47.1


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Philippe Simons]

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

Signed-off-by: Philippe Simons <simons.philippe@gmail.com>
---
 drivers/phy/allwinner/phy-sun4i-usb.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c
index 24fbabe7a..f163b355e 100644
--- a/drivers/phy/allwinner/phy-sun4i-usb.c
+++ b/drivers/phy/allwinner/phy-sun4i-usb.c
@@ -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;
-- 
2.47.1


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Jernej Škrabec]

Dne sobota, 18. januar 2025 ob 11:22:05 Srednjeevropski standardni čas je Philippe Simons napisal(a):
> PHY0 mode is fully determined by the selected mux route.
> Ignore changes requests from controllers.
> 
> Signed-off-by: Philippe Simons <simons.philippe@gmail.com>

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

> ---
>  drivers/phy/allwinner/phy-sun4i-usb.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c
> index 24fbabe7a..f163b355e 100644
> --- a/drivers/phy/allwinner/phy-sun4i-usb.c
> +++ b/drivers/phy/allwinner/phy-sun4i-usb.c
> @@ -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;
> 





-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Jernej Škrabec]

Dne sobota, 18. januar 2025 ob 11:22:04 Srednjeevropski standardni čas je Philippe Simons napisal(a):
> 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 <simons.philippe@gmail.com>

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

> ---
>  drivers/phy/allwinner/phy-sun4i-usb.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c
> index b0f19e950..24fbabe7a 100644
> --- a/drivers/phy/allwinner/phy-sun4i-usb.c
> +++ b/drivers/phy/allwinner/phy-sun4i-usb.c
> @@ -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) {

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

> +			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) {

Ideally, this should never happen.

Best regards,
Jernej

> +		regulator_disable(phy->vbus);
> +		phy->regulator_on = false;
> +	}
> +
>  	force_session_end = data->force_session_end;
>  	data->force_session_end = false;
>  
> 





-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy