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

[RD Babiera]

Hi Neill, thanks for the thorough review. Will send v8 out shortly.

On Mon, Sep 21, 2026 at 11:29 AM Neill Kapron <nkapron@xxxxxxxxxx> wrote:
> 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.

Acknowledged.

> > +#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.

Will change here.

> > +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).

Will check.

> >  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.

The check here is called on gphy->dev as opposed to phy->dev. fw_devlink uses
FW_DEVLINK_FLAGS_RPM by default, so the dwc3 consumer controller holds
DL_FLAG_PM_RUNTIME on the phy platform device. Every resume call on
the dwc3 controller results in the supplier resuming as well under this
model, so the runtime_get() call here passes.

> 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().

usb3_tca_base is powered by a different clock as opposed to the 'usb3'
one, so the register access will be safe as long as the platform driver's
power domain is on. Register reads in either order both result in the
same behavior during testing, but I would argue that writing before
the call to reset_control_bulk_deassert() ensures that the parameter
is applied on PoR.

Best,
RD