On Fri, Mar 14, 2025 at 03:33:48PM +0800, Wayne Chang wrote:
> The current implementation uses bias_pad_enable as a reference count to
> manage the shared bias pad for all UTMI PHYs. However, during system
> suspension with connected USB devices, multiple power-down requests for
> the UTMI pad result in a mismatch in the reference count, which in turn
> produces warnings such as:
>
> [ 237.762967] WARNING: CPU: 10 PID: 1618 at tegra186_utmi_pad_power_down+0x160/0x170
> [ 237.763103] Call trace:
> [ 237.763104] tegra186_utmi_pad_power_down+0x160/0x170
> [ 237.763107] tegra186_utmi_phy_power_off+0x10/0x30
> [ 237.763110] phy_power_off+0x48/0x100
> [ 237.763113] tegra_xusb_enter_elpg+0x204/0x500
> [ 237.763119] tegra_xusb_suspend+0x48/0x140
> [ 237.763122] platform_pm_suspend+0x2c/0xb0
> [ 237.763125] dpm_run_callback.isra.0+0x20/0xa0
> [ 237.763127] __device_suspend+0x118/0x330
> [ 237.763129] dpm_suspend+0x10c/0x1f0
> [ 237.763130] dpm_suspend_start+0x88/0xb0
> [ 237.763132] suspend_devices_and_enter+0x120/0x500
> [ 237.763135] pm_suspend+0x1ec/0x270
>
> The root cause was traced back to the dynamic power-down changes
> introduced in commit a30951d31b25 ("xhci: tegra: USB2 pad power controls"),
> where the UTMI pad was being powered down without verifying its current
> state. This unbalanced behavior led to discrepancies in the reference
> count.
>
> To rectify this issue, this patch replaces the single reference counter
> with a bitmask, renamed to utmi_pad_enabled. Each bit in the mask
> corresponds to one of the four USB2 PHYs, allowing us to track each pad's
> enablement status individually.
>
> With this change:
> - The bias pad is powered on only when the mask is clear.
> - Each UTMI pad is powered on or down based on its corresponding bit
> in the mask, preventing redundant operations.
> - The overall power state of the shared bias pad is maintained
> correctly during suspend/resume cycles.
>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: a30951d31b25 ("xhci: tegra: USB2 pad power controls")
> Signed-off-by: Wayne Chang <waynec@xxxxxxxxxx>
> ---
> drivers/phy/tegra/xusb-tegra186.c | 25 +++++++++++++++++--------
> 1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/phy/tegra/xusb-tegra186.c b/drivers/phy/tegra/xusb-tegra186.c
> index fae6242aa730..77bb27a34738 100644
> --- a/drivers/phy/tegra/xusb-tegra186.c
> +++ b/drivers/phy/tegra/xusb-tegra186.c
> @@ -237,6 +237,8 @@
> #define DATA0_VAL_PD BIT(1)
> #define USE_XUSB_AO BIT(4)
>
> +#define TEGRA_UTMI_PAD_MAX 4
> +
> #define TEGRA186_LANE(_name, _offset, _shift, _mask, _type) \
> { \
> .name = _name, \
> @@ -269,7 +271,7 @@ struct tegra186_xusb_padctl {
>
> /* UTMI bias and tracking */
> struct clk *usb2_trk_clk;
> - unsigned int bias_pad_enable;
> + DECLARE_BITMAP(utmi_pad_enabled, TEGRA_UTMI_PAD_MAX);
>
> /* padctl context */
> struct tegra186_xusb_padctl_context context;
> @@ -605,7 +607,7 @@ static void tegra186_utmi_bias_pad_power_on(struct tegra_xusb_padctl *padctl)
>
> mutex_lock(&padctl->lock);
>
> - if (priv->bias_pad_enable++ > 0) {
> + if (!bitmap_empty(priv->utmi_pad_enabled, TEGRA_UTMI_PAD_MAX)) {
> mutex_unlock(&padctl->lock);
> return;
> }
> @@ -669,12 +671,7 @@ static void tegra186_utmi_bias_pad_power_off(struct tegra_xusb_padctl *padctl)
>
> mutex_lock(&padctl->lock);
>
> - if (WARN_ON(priv->bias_pad_enable == 0)) {
> - mutex_unlock(&padctl->lock);
> - return;
> - }
> -
> - if (--priv->bias_pad_enable > 0) {
> + if (!bitmap_empty(priv->utmi_pad_enabled, TEGRA_UTMI_PAD_MAX)) {
> mutex_unlock(&padctl->lock);
> return;
> }
> @@ -697,6 +694,7 @@ static void tegra186_utmi_pad_power_on(struct phy *phy)
> {
> struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
> struct tegra_xusb_padctl *padctl = lane->pad->padctl;
> + struct tegra186_xusb_padctl *priv = to_tegra186_xusb_padctl(padctl);
> struct tegra_xusb_usb2_port *port;
> struct device *dev = padctl->dev;
> unsigned int index = lane->index;
> @@ -705,6 +703,9 @@ static void tegra186_utmi_pad_power_on(struct phy *phy)
> if (!phy)
> return;
>
> + if (test_bit(index, priv->utmi_pad_enabled))
> + return;
Don't we need to take the padctl->lock mutex before this...
> +
> port = tegra_xusb_find_usb2_port(padctl, index);
> if (!port) {
> dev_err(dev, "no port found for USB2 lane %u\n", index);
> @@ -724,18 +725,24 @@ static void tegra186_utmi_pad_power_on(struct phy *phy)
> value = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL1(index));
> value &= ~USB2_OTG_PD_DR;
> padctl_writel(padctl, value, XUSB_PADCTL_USB2_OTG_PADX_CTL1(index));
> +
> + set_bit(index, priv->utmi_pad_enabled);
... and release it here? Otherwise we might end up testing, setting and/
or clearing from two pads concurrently and loose consistency.
> }
>
> static void tegra186_utmi_pad_power_down(struct phy *phy)
> {
> struct tegra_xusb_lane *lane = phy_get_drvdata(phy);
> struct tegra_xusb_padctl *padctl = lane->pad->padctl;
> + struct tegra186_xusb_padctl *priv = to_tegra186_xusb_padctl(padctl);
> unsigned int index = lane->index;
> u32 value;
>
> if (!phy)
> return;
>
> + if (!test_bit(index, priv->utmi_pad_enabled))
> + return;
> +
Same here...
> dev_dbg(padctl->dev, "power down UTMI pad %u\n", index);
>
> value = padctl_readl(padctl, XUSB_PADCTL_USB2_OTG_PADX_CTL0(index));
> @@ -748,6 +755,8 @@ static void tegra186_utmi_pad_power_down(struct phy *phy)
>
> udelay(2);
>
> + clear_bit(index, priv->utmi_pad_enabled);
and here.
Thierry
Attachment:
signature.asc
Description: PGP signature
