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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Unconditional acquisition of USB3 resources breaks backwards compatibility with existing device trees.
- [High] Missing `.power_off` callback prevents PHY state reset, breaking Type-C routing on resume.
- [High] Premature software state update breaks hardware programming retries on failure.
- [High] Missing `PMGT_REF_CLK_REQ_N` deassertion on error paths leaks the reference clock request.
--

commit c027571e30437423b7f6c6808b9e70f2bc1b650a
Author: RD Babiera <rdbabiera@google.com>

phy: Add USB3 PHY support to Google Tensor SoC USB PHY driver

Add USB3 PHY support for the Google Tensor G5 USB PHY driver.
This patch adds functionality for the usb3_tca register, usb3 clock,
and usb3 reset as defined in google,lga-usb-phy.yaml. Kconfig now lists
USB SuperSpeed support.

> diff --git a/drivers/phy/phy-google-usb.c b/drivers/phy/phy-google-usb.c
> index ab20bc20f19ea..c6f4d8283e7c2 100644
> --- a/drivers/phy/phy-google-usb.c
> +++ b/drivers/phy/phy-google-usb.c
[ ... ]
> @@ -161,6 +313,118 @@ static const struct phy_ops google_usb2_phy_ops = {
> +static int google_usb3_phy_init(struct phy *_phy)
> +{
[ ... ]
> +	set_pmgt_ref_clk_req_n(gphy, true);
> +
> +	ret = clk_bulk_prepare_enable(inst->num_clks, inst->clks);
> +	if (ret)
> +		return ret;

[Severity: High]
Will this error path leak the reference clock request?

Since set_pmgt_ref_clk_req_n(gphy, true) was called earlier, if
clk_bulk_prepare_enable() fails we return without deasserting the clock
request.

The driver core doesn't invoke phy_exit() if phy_init() fails, so the
request will remain permanently asserted.

> +
> +	ret = reset_control_bulk_deassert(inst->num_rsts, inst->rsts);
> +	if (ret) {
> +		clk_bulk_disable_unprepare(inst->num_clks, inst->clks);
> +		return ret;
> +	}
> +
> +	ret = readl_poll_timeout(gphy->usb3_tca_base + TCA_PSTATE_0_OFFSET,
> +				 reg, !(reg & TCA_PSTATE_0_UPCS_LANE0_PHYSTATUS),
> +				 GPHY_TCA_DELAY_US, GPHY_TCA_TIMEOUT_US);
> +	if (ret) {
> +		dev_err(gphy->dev, "wait for lane0 phystatus timed out");
> +		reset_control_bulk_assert(inst->num_rsts, inst->rsts);
> +		clk_bulk_disable_unprepare(inst->num_clks, inst->clks);
> +		return ret;
> +	}

[Severity: High]
Does this error path also leak the reference clock request? 

It properly unwinds the clocks and resets, but it misses calling
set_pmgt_ref_clk_req_n(gphy, false) before returning.

[ ... ]
> +static int google_usb3_phy_power_on(struct phy *_phy)
> +{
[ ... ]
> +	if (gphy->phy_state == COMBO_PHY_TCA_READY) {
> +		dev_warn(gphy->dev, "usb3 phy already powered on");
> +		return 0;
> +	}
[ ... ]
> +	gphy->phy_state = COMBO_PHY_TCA_READY;
> +
> +	ret = program_tca_locked(gphy);
> +
> +	return ret;
> +}

[Severity: High]
If program_tca_locked() fails, will hardware programming retries fail
because the software state was prematurely updated?

If the USB subsystem retries powering on the PHY after an error, the early
success check at the top of google_usb3_phy_power_on() will hit and return 0
without actually re-attempting the hardware configuration.

> +
> +static const struct phy_ops google_usb3_phy_ops = {
> +	.init		= google_usb3_phy_init,
> +	.exit		= google_usb3_phy_exit,
> +	.power_on	= google_usb3_phy_power_on,
> +};

[Severity: High]
Is the missing .power_off callback going to cause the state machine to
desync on suspend and resume? 

During suspend, without a power off hook, gphy->phy_state remains
COMBO_PHY_TCA_READY. On resume, google_usb3_phy_power_on() checks this
state and returns 0 without reprogramming the TCA hardware, which could
permanently break the link.

[ ... ]
> @@ -173,14 +437,61 @@ static struct phy *google_usb_phy_xlate(struct device *dev,
> +static int google_usb_phy_parse_clocks(struct google_usb_phy *gphy)
> +{
[ ... ]
> +		ret = devm_clk_bulk_get(dev, inst->num_clks, inst->clks);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "failed to get phy%d clks\n", id);
> +	}
> +
> +	return 0;
> +}

[Severity: Critical]
Will unconditionally requesting the usb3 clocks break backwards compatibility
on systems using older device trees?

If an older device tree only provides USB2 resources, devm_clk_bulk_get()
will fail here and abort the entire probe sequence.

[ ... ]
> @@ -212,39 +523,39 @@ static int google_usb_phy_probe(struct platform_device *pdev)
[ ... ]
> +	gphy->usb3_tca_base = devm_platform_ioremap_resource_byname(pdev,
> +								    "usb3_tca");
> +	if (IS_ERR(gphy->usb3_tca_base))
> +		return dev_err_probe(dev, PTR_ERR(gphy->usb3_tca_base),
> +				    "invalid usb3 tca\n");
> +

[Severity: Critical]
Similarly, will unconditionally requiring the usb3_tca resource break probe
for older device trees that only define USB2 resources?

If the memory region is missing from the DT, this will return an error and
prevent the USB2 PHY from working on those devices.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260622175240.2469937-2-rdbabiera@google.com?part=1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy