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

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

On Mon, Sep 21, 2026 at 11:29 AM Neill Kapron [off-list ref] 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.

quoted

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

quoted

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

quoted

 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