From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-oo2-f40.google.com (mail-oo2-f40.google.com [74.125.231.168]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4153B4E0B98 for ; Mon, 21 Sep 2026 18:29:24 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=74.125.231.168 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1790015366; cv=none; b=jViYfbC6J60I7a5YzWwSMbb01RMUvsKLsJpxNeiULyI1xysQoyqWKnOovSpxqp6e1rx5n4oty2RzXffZMXYccABv7YNxCgnlMSb4DO5BI7MjTe2Y9AX/dbm7cmHHwf1k1ay0H9E2CYMRa4/2PKgo2QcnrbM2vxSUoClKKLPKbGA= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1790015366; c=relaxed/simple; bh=vHLcczVG0u5/7NzvBvHE9sbFAGBbTxZPZu97ylHOl4I=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Qp4pViJVfUMp0NgNQa4hqTmKFbtaGH23Cb8QJcWou3F5lUYmuZ3HeRNdGLRhr18tkpvjipmCuKVrQ6ZxvfHLlHYL7WjteYq+JSzMz9HqXv41ii0TccFoDx0hv3QlIHci0/pdeB2tP+2KzajHZc7ruNFtmIyY2LgPuFqtJXtZeJw= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=LIMynF+S; arc=none smtp.client-ip=74.125.231.168 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="LIMynF+S" Received: by mail-oo2-f40.google.com with SMTP id 006d021491bc7-6c24d19ebcfso1626981eaf.3 for ; Mon, 21 Sep 2026 11:29:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20251104; t=1790015363; x=1790620163; darn=vger.kernel.org; h=in-reply-to:content-disposition:content-type:mime-version :references:message-id:subject:cc:to:from:date:from:to:cc:subject :date:message-id:reply-to:content-type; bh=zBEpExP60BKwR+80aEYT0motmzonf8fi0uI5o05psGA=; b=LIMynF+SHwEz1x90yjJCnq1/rRfdWpdI8Wd2IU5EuIALeA25em/NvjgdXV4t6f2c57 rkdEX423g7arh4CWBEJIZTi9nXJBktWJgM4zT2DR65qoGyevDnxq4ivkOIvIADQw7blN Lvqva3zUaWJJXy6miGC1dFlJfMyaKDZl3wxAVQih1aqHFi24s8xX5ekBwRxMPW9OvKkw aF1YmYBSmJ+9w9Pht/pkQNLWsIp4iW2TghP4cO4ZrJAXss5xAAzmSgrwi0+c9CJ1Lidq 70PeVjg4uAxqN5Fr+uFtvqW/UDMC4wYTi9u8JThYjDuq0frL/Lg+6UZn6sJD4LT1xRci 2ooQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20260707; t=1790015363; x=1790620163; h=in-reply-to:content-disposition:content-type:mime-version :references:message-id:subject:cc:to:from:date:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to :content-type; bh=zBEpExP60BKwR+80aEYT0motmzonf8fi0uI5o05psGA=; b=xPeN0BpKQFEXD4qRhzbbNoRivyenyrO1KdfuZrijyETEqlG1URJgkavKRjVIayEMzP KPPvHxaUE4pmrYeJKKGo1YKoYm0fXy70G8oDU3nXoHrkO6EHciJHCq0AIRnQhOLD0j7R m2IgtWq6hV9fegjLefzNCjv1z0W57DwVVVNOdYVn6Y6hFDkudlfob738Kqo+mWbo9z3K JcubxnpUROpiHPntB/x+rfKJqphsMsHj30A8UOYXJJ3zZoTncd/d5xbIaj1Nq4x1inPw pz5Q64KJjYJIXnuhC6r5nnPT1pDB0i0VibDKYQ+ShSsjK4+tmxQhSTBiSnNEHGgz+Kqy hJjQ== X-Forwarded-Encrypted: i=1; AKwUvByi5rQC2U1D0aOeHBNyPEI7lNaGwFGbjFewPEpXDnwm4cZ4QsP3qbNfoK4Mgj9iHLhPEjqALO3fM8Nyqmc=@vger.kernel.org X-Gm-Message-State: AFuF++nRVsJarCpsV0bwRzdfBigcMfZSiun+q9eSxKVYVtV9IOJV4D+5 +c1vqJphSIEWtu06KkNAKJilB//njE3anMznTGzxsECXWi2UyqLXWZUyAdf9o/qicREcVqXO8jQ mxrlpwz+F X-Gm-Gg: AYBFou10GiIy7/RmeyM8ZmtzTKY2pO8Veb67CxaBA81vTW7fBGlPV7jIRRslSz+O/mf HeJB8y6FXL5Pfx05Yny9vgj8DUrnjc0hcEQqrxTJ25ljDz+f/wRZf9Zmd6dndzFDhDYckZpos7y CAMK9H9t6yTJ+Mk4ZMqY7cLa5wDv6PtrybrkAn2LOEWzO7N+OtGp0OXjcTYcBkhQLvOLR4xQQFn JbWVKGo+u2zg7zREKnZxUynkFGuhUTXrgY5y0SeuNCGOlELrZhObfkfkCWOfzW70lIlitgzPcpX Oh1nPQj2DAbT57iBG5j/81MQbWmXxM1gnBiXeTPNWGHQ7huejLNlvkrBe1ipByta7vQeDJVWEfX oOmZgz66jkGZt+ag5YnrDZWR23JvaQm9XQLqYfDGuST4nl1VKSiB6UoypeVpPYXqqCWgF91N1OY oeA8SV7OGGt0UmHtfCcz4AkjJside+BtDzfgPKMUqIovOAIycck08U6Jth2TIX61sugLMmhVb2z JglmfoPK3UdeOalXLDKTPi4USwqJ2w0jhZu X-Received: by 2002:a05:6820:607:b0:6c2:c9:2c76 with SMTP id 006d021491bc7-6ca9a85289amr10091458eaf.16.1790015362272; Mon, 21 Sep 2026 11:29:22 -0700 (PDT) Received: from google.com (222.97.173.34.bc.googleusercontent.com. [34.173.97.222]) by smtp.gmail.com with ESMTPSA id 006d021491bc7-6d181ae5a14sm26230eaf.3.2026.09.21.11.29.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 21 Sep 2026 11:29:21 -0700 (PDT) Date: Mon, 21 Sep 2026 18:29:19 +0000 From: Neill Kapron To: RD Babiera 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 v7] phy: Add USB3 PHY support to Google Tensor SoC USB PHY driver Message-ID: References: <20260918222513.2633456-2-rdbabiera@google.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260918222513.2633456-2-rdbabiera@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 > #include 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