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

[Jon Hunter]

Hi Vinod,

On 15/05/2025 12:03, Jon Hunter wrote:
> 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@xxxxxxxxxxxxxxx
> > Signed-off-by: Wayne Chang <waynec@xxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxx>
> Tested-by: Jon Hunter <jonathanh@xxxxxxxxxx>
>
> Thanks for fixing this!

Can we get this into -next now?

Thanks!

Jon

--
nvpublic