Re: [PATCH 1/1] phy: tegra: xusb: Fix unbalanced regulator disable in UTMI PHY mode

[Jon Hunter <jonathanh@nvidia.com>]

On 02/05/2025 10:26, Wayne Chang wrote:
> When transitioning from USB_ROLE_DEVICE to USB_ROLE_NONE, the code
> assumed that the regulator should be disabled. However, if the regulator
> is marked as always-on, regulator_is_enabled() continues to return true,
> leading to an incorrect attempt to disable a regulator which is not
> enabled.
> 
> This can result in warnings such as:
> 
> [  250.155624] WARNING: CPU: 1 PID: 7326 at drivers/regulator/core.c:3004
> _regulator_disable+0xe4/0x1a0
> [  250.155652] unbalanced disables for VIN_SYS_5V0
> 
> To fix this, we move the regulator control logic into
> tegra186_xusb_padctl_id_override() function since it's directly related
> to the ID override state. The regulator is now only disabled when the role
> transitions from USB_ROLE_HOST to USB_ROLE_NONE, by checking the VBUS_ID
> register. This ensures that regulator enable/disable operations are
> properly balanced and only occur when actually transitioning to/from host
> mode.
> 
> Fixes: 49d46e3c7e59 ("phy: tegra: xusb: Add set_mode support for UTMI phy on Tegra186")
> Cc: stable@vger.kernel.org
> Signed-off-by: Wayne Chang <waynec@nvidia.com>
> ---
>   drivers/phy/tegra/xusb-tegra186.c | 59 +++++++++++++++++++------------
>   1 file changed, 37 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/phy/tegra/xusb-tegra186.c b/drivers/phy/tegra/xusb-tegra186.c
> index fae6242aa730..1b35d50821f7 100644
> --- a/drivers/phy/tegra/xusb-tegra186.c
> +++ b/drivers/phy/tegra/xusb-tegra186.c
> @@ -774,13 +774,15 @@ static int tegra186_xusb_padctl_vbus_override(struct tegra_xusb_padctl *padctl,
>   }
>   
>   static int tegra186_xusb_padctl_id_override(struct tegra_xusb_padctl *padctl,
> -					    bool status)
> +					    struct tegra_xusb_usb2_port *port, bool status)
>   {
> -	u32 value;
> +	u32 value, id_override;
> +	int err = 0;
>   
>   	dev_dbg(padctl->dev, "%s id override\n", status ? "set" : "clear");
>   
>   	value = padctl_readl(padctl, USB2_VBUS_ID);
> +	id_override = value & ID_OVERRIDE(~0);
>   
>   	if (status) {
>   		if (value & VBUS_OVERRIDE) {
> @@ -791,15 +793,35 @@ static int tegra186_xusb_padctl_id_override(struct tegra_xusb_padctl *padctl,
>   			value = padctl_readl(padctl, USB2_VBUS_ID);
>   		}
>   
> -		value &= ~ID_OVERRIDE(~0);
> -		value |= ID_OVERRIDE_GROUNDED;
> +		if (id_override != ID_OVERRIDE_GROUNDED) {
> +			value &= ~ID_OVERRIDE(~0);
> +			value |= ID_OVERRIDE_GROUNDED;
> +			padctl_writel(padctl, value, USB2_VBUS_ID);
> +
> +			err = regulator_enable(port->supply);
> +			if (err) {
> +				dev_err(padctl->dev, "Failed to enable regulator: %d\n", err);
> +				return err;
> +			}
> +		}
>   	} else {
> -		value &= ~ID_OVERRIDE(~0);
> -		value |= ID_OVERRIDE_FLOATING;
> +		if (id_override == ID_OVERRIDE_GROUNDED) {
> +			/*
> +			 * The regulator is disabled only when the role transitions
> +			 * from USB_ROLE_HOST to USB_ROLE_NONE.
> +			 */
> +			err = regulator_disable(port->supply);
> +			if (err) {
> +				dev_err(padctl->dev, "Failed to disable regulator: %d\n", err);
> +				return err;
> +			}
> +
> +			value &= ~ID_OVERRIDE(~0);
> +			value |= ID_OVERRIDE_FLOATING;
> +			padctl_writel(padctl, value, USB2_VBUS_ID);
> +		}
>   	}
>   
> -	padctl_writel(padctl, value, USB2_VBUS_ID);
> -
>   	return 0;
>   }
>   
> @@ -818,27 +840,20 @@ static int tegra186_utmi_phy_set_mode(struct phy *phy, enum phy_mode mode,
>   
>   	if (mode == PHY_MODE_USB_OTG) {
>   		if (submode == USB_ROLE_HOST) {
> -			tegra186_xusb_padctl_id_override(padctl, true);
> -
> -			err = regulator_enable(port->supply);
> +			err = tegra186_xusb_padctl_id_override(padctl, port, true);
> +			if (err)
> +				goto out;
>   		} else if (submode == USB_ROLE_DEVICE) {
>   			tegra186_xusb_padctl_vbus_override(padctl, true);
>   		} else if (submode == USB_ROLE_NONE) {
> -			/*
> -			 * When port is peripheral only or role transitions to
> -			 * USB_ROLE_NONE from USB_ROLE_DEVICE, regulator is not
> -			 * enabled.
> -			 */
> -			if (regulator_is_enabled(port->supply))
> -				regulator_disable(port->supply);
> -
> -			tegra186_xusb_padctl_id_override(padctl, false);
> +			err = tegra186_xusb_padctl_id_override(padctl, port, false);
> +			if (err)
> +				goto out;
>   			tegra186_xusb_padctl_vbus_override(padctl, false);
>   		}
>   	}
> -
> +out:
>   	mutex_unlock(&padctl->lock);
> -
>   	return err;
>   }
>   


Reviewed-by: Jon Hunter <jonathanh@nvidia.com>
Tested-by: Jon Hunter <jonathanh@nvidia.com>

Thanks for fixing this!

Jon

-- 
nvpublic