Re: [PATCH 3/4] phy: apple: atc: Add initial M3 series support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] phy: apple: atc: missing power off on RCAL1 timeout in
  atcphy_configure()
- [Low] phy: apple: atc: missing @hw kernel-doc entry in struct
  apple_atcphy
--

commit 70e818649e5b1dc004594c72a6012be4bd8cf369
Author: Sasha Finkelstein <k@chaosmail.tech>

phy: apple: atc: Add initial M3 series support

This commit adds initial USB and Thunderbolt PHY support for the Apple
M3 series (t8122).

> diff --git a/drivers/phy/apple/atc.c b/drivers/phy/apple/atc.c
> index 45f1280a6faa3..256a084996469 100644
> --- a/drivers/phy/apple/atc.c
> +++ b/drivers/phy/apple/atc.c

[ ... ]

> @@ -599,9 +647,10 @@ struct apple_atcphy {
>  		struct apple_tunable *lane_usb4[2];
>  	} tunables;
>  
> +	const struct atcphy_hw *hw;
>  	enum atcphy_mode mode;
> -	bool swap_lanes;
>  	int dp_link_rate;
> +	bool swap_lanes;
>  	bool pipehandler_up;
>  
>  	struct {

[Severity: Low]
struct apple_atcphy has a kernel-doc comment block, so adding hw without
a matching @hw entry triggers a W=1 kernel-doc warning (and later commit
"phy: apple: atc: Add T8122 DisplayPort support" also leaves @hw
undocumented).

[ ... ]

> +static int atcphy_configure(struct apple_atcphy *atcphy, enum atcphy_mode mode)
> +{
> +	int ret = 0;
> +	u32 reg;
> +
> +	lockdep_assert_held(&atcphy->lock);
> +
> +	if (mode == APPLE_ATCPHY_MODE_OFF) {
> +		ret = atcphy_power_off(atcphy);
> +		atcphy->mode = mode;
> +		return ret;
> +	}
> +
> +	ret = atcphy_power_on(atcphy);
> +	if (ret)
> +		return ret;
> +
> +	atcphy_apply_tunables(atcphy, mode);
> +	atcphy->hw->configure_post_tunable(atcphy, mode);
>  
>  	/* Setup AUX channel if DP altmode is requested */
>  	if (atcphy_modes[mode].enable_dp_aux)
>  		atcphy_enable_dp_aux(atcphy);
>  
>  	/* Enable clocks and configure lanes */
> -	core_set32(atcphy, CIO3PLL_CLK_CTRL, CIO3PLL_CLK_PCLK_EN);
> -	core_set32(atcphy, CIO3PLL_CLK_CTRL, CIO3PLL_CLK_REFCLK_EN);
> +	if (atcphy->hw->gen == ATCPHY_GENERATION_T8103) {
> +		core_set32(atcphy, CIO3PLL_CLK_CTRL, CIO3PLL_CLK_PCLK_EN);
> +		core_set32(atcphy, CIO3PLL_CLK_CTRL, CIO3PLL_CLK_REFCLK_EN);
> +	}
>  	atcphy_configure_lanes(atcphy, mode);
>  
>  	/* Take the USB3 PHY out of reset */
>  	core_set32(atcphy, ATCPHY_POWER_CTRL, ATCPHY_POWER_PHY_RESET_N);
> +	/*
> +	 * NOTE: Not what macos does, it takes the phy out of reset right before writing to
> +	 * ACIOPHY_CFG0_COMMON_SMALL does lane configuration with phy out of reset, and the poll is
> +	 * right after AUS_COMMON_SHIM_BLK_BIAS_REG write. Does not seem to matter, but if something
> +	 * does not work, consider re-ordering things to macos order.
> +	 */
> +	if (atcphy->hw->gen == ATCPHY_GENERATION_T8122 &&
> +	    atcphy_modes[mode].pipehandler_state == ATCPHY_PIPEHANDLER_STATE_USB3) {
> +		ret = readl_poll_timeout(atcphy->regs.core + AUS_COMMON_DIG_RCAL1, reg,
> +					 (reg & AUS_COMMON_DIG_RCAL1_ALL_CODES_DONE), 10, 100000);
> +		if (ret) {
> +			dev_err(atcphy->dev, "Failed to wait for RCAL1 done\n");
> +			return ret;
> +		}
> +	}

[Severity: Medium]
Should this error path call atcphy_power_off(atcphy) before returning?

When readl_poll_timeout() fails here, the PHY has already been powered
on and taken out of reset (ATCPHY_POWER_PHY_RESET_N), while atcphy->mode
remains APPLE_ATCPHY_MODE_OFF.  Because teardown in
atcphy_usb3_power_off() and atcphy_mux_set() (for TYPEC_STATE_SAFE) only
powers down the PHY when atcphy->mode != APPLE_ATCPHY_MODE_OFF, the PHY
and DP AUX stay powered on after disconnect and ATCPHY_POWER_PHY_RESET_N
remains deasserted when tunables are applied on the next connection.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108-t8122-phy-v1-0-c2783b2a53a8@chaosmail.tech?part=3

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