Re: [PATCH v2 4/5] phy: qualcomm: qmp-combo: Add USB4/TBT3 configuration data for Hamoa

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The driver reads the wrong register to check the `DP_TUNNELING_CLOCK_GEN_EN` flag and then unconditionally clobbers the correct register with `writel`.
- [Medium] Missing `clk_disable_unprepare` for `p2rr2p_pipe_clk` on error paths in `qmp_combo_usb4_init`.
- [Medium] Double execution of `qmp_combo_usb_power_on` in `qmp_combo_usb4_init` if the USB3 PHY is already active.
- [Low] Several UCDR (Universal Clock and Data Recovery) register offsets are misleadingly defined with a `_TX_` prefix instead of `_RX_`.
--

--- Patch [4]: [PATCH v2 4/5] phy: qualcomm: qmp-combo: Add USB4/TBT3 configuration data for Hamoa ---
commit cbebd51448ab869d435da5d2defb78f367d02956
Author: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Subject: phy: qualcomm: qmp-combo: Add USB4/TBT3 configuration data for Hamoa

This commit adds the offsets and configuration tables to support USB4 and
Thunderbolt 3 operation on the USB4-capable PHYs found on Hamoa chips.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index b80eac9d71401..419668e014906 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
[ ... ]
> @@ -1873,6 +1952,68 @@ static const struct qmp_phy_init_tbl x1e80100_usb43dp_tx_tbl[] = {
>  	QMP_PHY_INIT_CFG(QSERDES_V6_N4_TX_RES_CODE_LANE_OFFSET_RX, 0x0a),
>  };
>  
> +static const struct qmp_phy_init_tbl x1e80100_usb4_rx_tbl[] = {
[ ... ]
> +	QMP_PHY_INIT_CFG(QSERDES_V6_N4_RX_MODE_RATE3_B6, 0xe3),
> +	QMP_PHY_INIT_CFG(QSERDES_V6_N4_TX_UCDR_FASTLOCK_COUNTER_LOW_RATE2, 0x30),

[Severity: Low]
Is this macro name accurate? It uses a "_TX_" prefix but is placed inside an
"rx_tbl".

Since UCDR is fundamentally a receiver-side block and its offsets logically
fall within the contiguous RX register block, using the "_TX_" prefix
creates a misleading hardware representation for future maintenance.

> +	QMP_PHY_INIT_CFG(QSERDES_V6_N4_TX_UCDR_FASTLOCK_COUNTER_HIGH_RATE2, 0x14),
> +	QMP_PHY_INIT_CFG(QSERDES_V6_N4_TX_UCDR_FASTLOCK_COUNTER_LOW_RATE3, 0xff),
> +	QMP_PHY_INIT_CFG(QSERDES_V6_N4_TX_UCDR_FASTLOCK_COUNTER_HIGH_RATE3, 0x3f),
[ ... ]
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-qserdes-txrx-v6_n4.h b/drivers/phy/qualcomm/phy-qcom-qmp-qserdes-txrx-v6_n4.h
> index d37cc0d4fd365..e72ae2cb460ac 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-qserdes-txrx-v6_n4.h
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-qserdes-txrx-v6_n4.h
[ ... ]
> @@ -23,27 +23,63 @@
[ ... ]
> +#define QSERDES_V6_N4_RX_UCDR_SO_SATURATION		0x214
> +#define QSERDES_V6_N4_TX_UCDR_FASTLOCK_COUNTER_LOW_RATE1	0x224
> +#define QSERDES_V6_N4_TX_UCDR_FASTLOCK_COUNTER_HIGH_RATE1	0x228

[Severity: Low]
Are these registers correctly named? As noted above, they have a "_TX_"
prefix but are logically UCDR receiver-side block offsets.

> +#define QSERDES_V6_N4_TX_UCDR_FASTLOCK_COUNTER_LOW_RATE2	0x22c
> +#define QSERDES_V6_N4_TX_UCDR_FASTLOCK_COUNTER_HIGH_RATE2	0x230
[ ... ]

With the introduction of USB4 support, does the initialization code in
qmp_combo_com_init() corrupt the DP_TUNNELING_CLOCK_GEN_EN state?

drivers/phy/qualcomm/phy-qcom-qmp-combo.c:qmp_combo_com_init() {
    ...
	if (qmp->qmpphy_mode == QMPPHY_MODE_USB4) {
		pcs = qmp->usb4_pcs;
        ...
		/* Do not disturb the DP PLL in case there's an active DP tunnel */
		if (readl(com + QPHY_V3_DP_COM_RESET_OVRD_CTRL) & DP_TUNNELING_CLOCK_GEN_EN)
			dp_reset_val &= ~SW_DPPHY_RESET_MUX;
	}
    ...
	case QMPPHY_MODE_USB4:
		writel(USB4_MODE, com + QPHY_V3_DP_COM_PHY_MODE_CTRL);
    ...
}

[Severity: High]
The code checks the tunneling state by reading QPHY_V3_DP_COM_RESET_OVRD_CTRL
instead of the correct PHY_MODE_CTRL register.

It then unconditionally overwrites QPHY_V3_DP_COM_PHY_MODE_CTRL completely
via writel(), which clears the DP_TUNNELING_CLOCK_GEN_EN flag if it was
previously active.

Can this break active DP tunnels by inappropriately resetting the DP PLL?

Are we missing a clk_disable_unprepare() on the error paths in
qmp_combo_usb4_init()?

drivers/phy/qualcomm/phy-qcom-qmp-combo.c:qmp_combo_usb4_init() {
    ...
	ret = clk_prepare_enable(qmp->p2rr2p_pipe_clk);
	if (ret) {
		dev_err(qmp->dev, "p2rr2p_pipe enable failed: %d\n", ret);
		return ret;
	}

	ret = qmp_combo_com_init(qmp, false);
	if (ret)
		return ret;
    ...
}

[Severity: Medium]
If qmp_combo_com_init(), qmp_combo_reconfigure_phy(), or
qmp_combo_usb_power_on() fail during USB4 PHY initialization, the function
returns without disabling the p2rr2p_pipe_clk prepared earlier, leaking
the clock reference.

Does qmp_combo_usb4_init() cause a double execution of
qmp_combo_usb_power_on()?

drivers/phy/qualcomm/phy-qcom-qmp-combo.c:qmp_combo_reconfigure_phy() {
    ...
	if ((qmp->usb_init_count || qmp->usb4phy_init_count) &&
	    new_mode != QMPPHY_MODE_DP_ONLY)
		qmp_combo_usb_power_on(qmp->usb_phy);
    ...
}

drivers/phy/qualcomm/phy-qcom-qmp-combo.c:qmp_combo_usb4_init() {
    ...
	ret = qmp_combo_reconfigure_phy(qmp, QMPPHY_MODE_USB4);
	if (ret)
		return ret;

	ret = qmp_combo_usb_power_on(phy);
    ...
}

[Severity: Medium]
When initializing USB4 mode and the USB3 PHY is already active
(usb_init_count > 0), qmp_combo_reconfigure_phy() automatically invokes
qmp_combo_usb_power_on().

However, qmp_combo_usb4_init() unconditionally calls it a second time
immediately after. This might redundantly rewrite all configuration
registers and needlessly re-toggle SW_RESET and START_CTRL for the PHY.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728-topic-usb4phy-v2-0-5d9dd5149ec7@oss.qualcomm.com?part=4

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