Re: [PATCH 1/1] phy: tegra: xusb: Use a bitmask for UTMI pad power state tracking

[Jon Hunter <jonathanh@nvidia.com>]

On 14/03/2025 07:33, 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@vger.kernel.org
> Fixes: a30951d31b25 ("xhci: tegra: USB2 pad power controls")
> Signed-off-by: Wayne Chang <waynec@nvidia.com>
> ---
>   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;
> +
>   	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);
>   }
>   
>   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;
> +
>   	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);
> +
>   	tegra186_utmi_bias_pad_power_off(padctl);
>   }
>   


Looks good to me!

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

Thanks for fixing!

Jon

-- 
nvpublic