| Message ID | 20250502092606.2275682-1-waynec@nvidia.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [1/1] phy: tegra: xusb: Fix unbalanced regulator disable in UTMI PHY mode | expand |
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
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@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! Can we get this into -next now? Thanks! Jon
On Fri, 02 May 2025 17:26:06 +0800, 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: > > [...] Applied, thanks! [1/1] phy: tegra: xusb: Fix unbalanced regulator disable in UTMI PHY mode commit: cefc1caee9dd06c69e2d807edc5949b329f52b22 Best regards,
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; }
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(-)