Re: [PATCH v6] phy: Add USB3 PHY support to Google Tensor SoC USB PHY driver

From: Neill Kapron <nkapron@google.com>
To: RD Babiera <rdbabiera@google.com>
Cc: vkoul@kernel.org, peter.griffin@linaro.org,
	andre.draszik@linaro.org, tudor.ambarus@linaro.org,
	p.zabel@pengutronix.de, neil.armstrong@linaro.org,
	badhri@google.com, linux-arm-kernel@lists.infradead.org,
	linux-samsung-soc@vger.kernel.org, linux-phy@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6] phy: Add USB3 PHY support to Google Tensor SoC USB PHY driver
Date: Tue, 25 Aug 2026 18:24:25 +0000	[thread overview]
Message-ID: <ao3d2TlIRqxbz4WP@google.com> (raw)
In-Reply-To: <20260710172946.15633-2-rdbabiera@google.com>

Hi RD, some minor comments below.

On Fri, Jul 10, 2026 at 05:29:47PM +0000, RD Babiera wrote:
> +#define GPHY_TCA_DELAY_US 10
> +#define GPHY_TCA_TIMEOUT_US 2500000

Do we really need a 2.5s timeout?

...
> +
> +static int wait_tca_xa_ack(struct google_usb_phy *gphy)
> +{
> +	int ret;
> +	u32 reg;
> +
> +	ret = readl_poll_timeout(gphy->usb3_tca_base + TCA_INTR_STS_OFFSET,
> +				 reg, !!(reg & TCA_INTR_STS_XA_ACT_EVT),
> +				 GPHY_TCA_DELAY_US, GPHY_TCA_TIMEOUT_US);
> +	if (ret)
> +		dev_err(gphy->dev, "tca xa_ack timeout, ret=%d", ret);

The dev_* calls throughout this patch should have trailing newline
characters.

> +
> +	return ret;
> +}

...
>  static int google_usb_set_orientation(struct typec_switch_dev *sw,
>  				      enum typec_orientation orientation)
>  {
>  	struct google_usb_phy *gphy = typec_switch_get_drvdata(sw);
> +	int ret = 0;
>  
>  	dev_dbg(gphy->dev, "set orientation %d\n", orientation);
>  
> -	gphy->orientation = orientation;
> +	guard(mutex)(&gphy->phy_mutex);
>  
> -	if (pm_runtime_suspended(gphy->dev))
> -		return 0;
> +	gphy->orientation = orientation;
>  
> -	guard(mutex)(&gphy->phy_mutex);
> +	if (IS_ENABLED(CONFIG_PM)) {
> +		if (pm_runtime_get_if_active(gphy->dev) <= 0)
> +			return 0;
> +	}
>  
>  	set_vbus_valid(gphy);
>  
> -	return 0;
> +	if (gphy->phy_state == COMBO_PHY_TCA_READY && orientation != TYPEC_ORIENTATION_NONE)
> +		ret = program_tca_locked(gphy);
> +
> +	pm_runtime_put_autosuspend(gphy->dev);

Here you are using the _autosuspend variant, but autosuspend is never
configured in probe(). Should this be changed to the plain
pm_runtime_put(), or actually enable autosuspend in the probe function?

> +
> +	return ret;
>  }

...
> +
> +static int google_usb3_phy_power_on(struct phy *_phy)
> +{
> +	struct google_usb_phy_instance *inst = phy_get_drvdata(_phy);
> +	struct google_usb_phy *gphy = inst->parent;
> +	int ret;
> +
> +	dev_dbg(gphy->dev, "power on usb3 phy\n");
> +
> +	guard(mutex)(&gphy->phy_mutex);
> +
> +	if (gphy->phy_state != COMBO_PHY_TCA_READY) {

I think prior to this we should add a check if phy_state is
COMBO_PHY_IDLE, return an error and change this check to phy_state ==
COMBO_PHY_INIT_DONE to make sure we are explicit in our state handling
to prevent issues with any potential refactoring.

> +		/* Wait for PoR -> NC transitions*/
> +		ret = wait_tca_xa_ack(gphy);
> +		if (ret) {
> +			dev_err(gphy->dev, "PoR->NC transition timeout");
> +			return ret;
> +		}
> +		gphy->phy_state = COMBO_PHY_TCA_READY;
> +	}
> +
> +	if (gphy->orientation != TYPEC_ORIENTATION_NONE)
> +		return program_tca_locked(gphy);
> +
> +	return 0;
> +}
...

Thanks,
Neill 