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

["Jernej Škrabec" <jernej.skrabec@gmail.com>]

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;
>  
>