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

[Neill Kapron <nkapron@google.com>]

Hi RD,

Thanks for sending v7. I've reviewed the changes and identified a few
functional issues, and a couple minor items as seen below:


On Fri, Sep 18, 2026 at 10:25:14PM +0000, RD Babiera wrote:
> Add USB3 PHY support for the Google Tensor G5 USB PHY driver.
...
> --- a/drivers/phy/phy-google-usb.c
> +++ b/drivers/phy/phy-google-usb.c
> @@ -20,6 +20,7 @@
>  #include <linux/reset.h>
>  #include <linux/usb/typec_mux.h>

The driver is now using readl_poll_timeout() and
pm_runtime_get_if_active(), we should be including linux/iopoll.h and
linux/pm_runtime.h explicitly.

> +#define TCA_CTRLSYNCMODE_CFG1_XA_TIMEOUT_VAL_100MS 0x1e85
> +#define TCA_PSTATE_0_OFFSET 0x50
> +#define TCA_PSTATE_0_UPCS_LANE0_PHYSTATUS BIT(8)
> +
> +#define GPHY_TCA_DELAY_US 10
> +#define GPHY_TCA_TIMEOUT_US 100000

With the addition of TCA_CTRLSYNCMODE_CFG1_XA_TIMEOUT_VAL_100MS, we
should consider bumping GPHY_TCA_TIMEOUT_US to be slightly larger (e.g.
110000us) to ensure the hardware timeout is guaranteed to expire before
the software poll timeout.

> +static const char * const u2phy_clk_names[] = {
> +	"usb2",
> +	"usb2_apb",
> +};
> +static const char * const u3phy_clk_names[] = {
> +	"usb3"
> +};
> +static const char * const u2phy_rst_names[] = {
> +	"usb2",
> +	"usb2_apb",
> +};
> +static const char * const u3phy_rst_names[] = {
> +	"usb3"
> +};

nit: checkpatch.pl --strict flags missing blank lines between these
array declarations (and the inline helper functions + DEFINE__FREE
macros below).

> +
> +static const struct google_usb_phy_config phy_configs[GOOGLE_USB_PHY_NUM] = {
> +	[GOOGLE_USB2_PHY] = {
> +		.clk_names = u2phy_clk_names,
> +		.num_clks = ARRAY_SIZE(u2phy_clk_names),
> +		.rst_names = u2phy_rst_names,
> +		.num_rsts = ARRAY_SIZE(u2phy_rst_names),
> +	},
> +	[GOOGLE_USB3_PHY] = {
> +		.clk_names = u3phy_clk_names,
> +		.num_clks = ARRAY_SIZE(u3phy_clk_names),
> +		.rst_names = u3phy_rst_names,
> +		.num_rsts = ARRAY_SIZE(u3phy_rst_names),
> +	},
> +};
> +
> +static inline void google_usb_phy_clk_disable(struct google_usb_phy_instance *inst)
> +{
> +	clk_bulk_disable_unprepare(inst->num_clks, inst->clks);
> +}
> +DEFINE_FREE(inst_clk_disable, struct google_usb_phy_instance *,
> +	    if (_T) google_usb_phy_clk_disable(_T))
> +
> +static inline void google_usb_phy_rst_disable(struct google_usb_phy_instance *inst)
> +{
> +	reset_control_bulk_assert(inst->num_rsts, inst->rsts);
> +}
> +DEFINE_FREE(inst_rst_disable, struct google_usb_phy_instance *,
> +	    if (_T) google_usb_phy_rst_disable(_T))
> +
...
>   
>  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(gphy->dev);
> +
> +	return ret;
>  }

Previously, sashiko recommended moving to pm_runtime_get_if_active(),
which was done in v6. However I think this may have changed the behavior
of google_usb_set_orientation() and potentially introduced a regression
due to the pre-existing ordering of calls in gooogle_usb_phy_probe(),
causing this function to always take the early 'return 0' path.

In google_usb_phy_probe(), we call devm_phy_create() prior to calling
pm_runtime_enable(dev).

In drivers/phy/phy-core.c, devm_phy_create() calls phy_create(), which
has the following check:

    if (pm_runtime_enabled(dev)) {
        pm_runtime_enable(&phy->dev);
        pm_runtime_no_callbacks(&phy->dev);
    }

Therefore, the phy device never has pm_runtime_enabled, causing this
call to pm_runtime_get_if_active() to always return 0, and the function
exits prior to calling `set_vbus_valid()`.

I think moving the pm_runtime_enable(dev) call prior to
devm_phy_create() will resolve the issue, but we should audit power
managment in this driver to verify.

> 
> +static int google_usb3_phy_init(struct phy *_phy)
> +{
> +	struct google_usb_phy_instance *inst = phy_get_drvdata(_phy);
> +	struct google_usb_phy *gphy = inst->parent;
> +	int ret = 0;
> +	u32 reg;
> +
> +	dev_dbg(gphy->dev, "initializing usb3 phy\n");
> +
> +	guard(mutex)(&gphy->phy_mutex);
> +
> +	if (gphy->phy_state != COMBO_PHY_IDLE) {
> +		dev_warn(gphy->dev, "usb3 phy init called when combo phy state is not idle\n");
> +		return 0;
> +	}
> +
> +	reg = readl(gphy->usb3_tca_base + TCA_CTRLSYNCMODE_CFG1_OFFSET);
> +	reg &= ~TCA_CTRLSYNCMODE_CFG1_XA_TIMEOUT_VAL;
> +	reg |= FIELD_PREP(TCA_CTRLSYNCMODE_CFG1_XA_TIMEOUT_VAL,
> +			  TCA_CTRLSYNCMODE_CFG1_XA_TIMEOUT_VAL_100MS);
> +	writel(reg, gphy->usb3_tca_base + TCA_CTRLSYNCMODE_CFG1_OFFSET);

I think this introduces a regression between v6 and v7, as usb3_tca_base
may be accessed prior to the 'usb3' clock being enabled, and
furthermore, the call to reset_control_bulk_deassert() will clear this
value.

Therefore, I think we need to this after the call to
reset_control_bulk_deassert().

> +
> +	reg = readl(gphy->usbdp_top_base + PHY_POWER_CONFIG_REG1_OFFSET);
> +	reg |= PHY_POWER_CONFIG_REG1_PG_MODE_EN;
> +	reg &= ~PHY_POWER_CONFIG_REG1_UPCS_PIPE_CONFIG;
> +	reg |= FIELD_PREP(PHY_POWER_CONFIG_REG1_UPCS_PIPE_CONFIG,
> +			  (UPCS_PIPE_CONFIG_ISO_CPM |
> +			   UPCS_PIPE_CONFIG_PG_MODE_STATIC |
> +			   UPCS_PIPE_CONFIG_LANE_RESET_NO_PG_EXIT));
> +	writel(reg, gphy->usbdp_top_base + PHY_POWER_CONFIG_REG1_OFFSET);
> +
> +	set_vbus_valid(gphy);
> +
> +	reg = readl(gphy->usbdp_top_base + USBCS_PHY_CFG1_OFFSET);
> +	reg |= USBCS_PHY_CFG1_PHY0_MPLLA_SSC_EN;
> +	writel(reg, gphy->usbdp_top_base + USBCS_PHY_CFG1_OFFSET);
> +
> +	set_sram_bypass(gphy, SRAM_BYPASS_MODE_BYPASS_FIRMWARE |
> +			SRAM_BYPASS_MODE_BYPASS_CONTEXT);
> +	set_pmgt_ref_clk_req_n(gphy, true);
> +	struct google_usb_phy *pmgt_ref_clk_req_dev __free(pmgt_ref_clk_req_n) = gphy;
> +
> +	ret = clk_bulk_prepare_enable(inst->num_clks, inst->clks);
> +	if (ret)
> +		return ret;
> +	struct google_usb_phy_instance *clk_dev __free(inst_clk_disable) = inst;
> +
> +	ret = reset_control_bulk_deassert(inst->num_rsts, inst->rsts);
> +	if (ret)
> +		return ret;
> +	struct google_usb_phy_instance *rst_dev __free(inst_rst_disable) = inst;
> +
> +	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\n");
> +		return ret;
> +	}
> +
> +	gphy->phy_state = COMBO_PHY_INIT_DONE;
> +
> +	retain_and_null_ptr(rst_dev);
> +	retain_and_null_ptr(clk_dev);
> +	retain_and_null_ptr(pmgt_ref_clk_req_dev);
> +
> +	return 0;
> +}
> +
> 
>

Thanks,
Neill