Re: [PATCH v3 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 5 potential issue(s) to consider:
- [High] Missing clk_disable_unprepare() for p2rr2p_pipe_clk on error paths in qmp_combo_usb4_init().
- [High] Double initialization of the PHY in qmp_combo_usb4_init() when switching from USB3 mode.
- [Medium] Silent replacement of USB3 configuration values.
- [Medium] Unchecked usage of `pcs_aon` during USB4 initialization.
- [Low] UCDR registers are incorrectly prefixed with TX instead of RX.
--

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

Add 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

[ ... ]

> +static const struct qmp_phy_init_tbl x1e80100_usb3dp_serdes_tbl[] = {
>  	QMP_PHY_INIT_CFG(QSERDES_V6_COM_SSC_EN_CENTER, 0x01),
>  	QMP_PHY_INIT_CFG(QSERDES_V6_COM_SSC_PER1, 0x62),

[Severity: Medium]
Does this introduce a silent functional change to the USB3 SerDes
initialization sequence?

The patch renames the original x1e80100_usb43dp_serdes_tbl to
x1e80100_usb4_serdes_tbl for USB4, and introduces this new table for USB3
operations.

However, this new table contains distinctly different tuning parameters
(e.g., QSERDES_V6_COM_SSC_STEP_SIZE1_MODE0 changes from 0xd9 to 0xc2, and
LOCK_CMP1_MODE0 changes from 0x1a to 0x08).

Since this change is not documented in the commit message, is there a risk of
regression for USB3 operations?

[ ... ]

> +static const struct qmp_phy_init_tbl x1e80100_usb4_rx_tbl[] = {
> +	QMP_PHY_INIT_CFG(QSERDES_V6_N4_RX_SIGDET_CNTRL, 0x04),
> +	QMP_PHY_INIT_CFG(QSERDES_V6_N4_RX_SIGDET_DEGLITCH_CNTRL, 0x0e),

[ ... ]

> +	QMP_PHY_INIT_CFG(QSERDES_V6_N4_TX_UCDR_FASTLOCK_COUNTER_LOW_RATE2, 0x30),

[Severity: Low]
This isn't a bug, but should these UCDR macros use an RX prefix instead of TX?

UCDR (Universal Clock and Data Recovery) is strictly an RX feature, and these
offsets belong to the RX block. They are populated correctly in this RX table,
but the misleading _TX_ prefix could pose a maintainability risk.

[ ... ]

> @@ -2689,11 +2864,11 @@ static const struct qmp_phy_cfg sc8280xp_usb43dpphy_cfg = {
>  	.regs			= qmp_v5_5nm_usb3phy_regs_layout,
>  };
>  
> -static const struct qmp_phy_cfg x1e80100_usb3dpphy_cfg = {
> -	.offsets		= &qmp_combo_offsets_v5,
> +static const struct qmp_phy_cfg x1e80100_usb43dpphy_cfg = {
> +	.offsets		= &qmp_combo_offsets_v6_n4,

[Severity: Medium]
Does supporting USB4 introduce a latent bug in qmp_combo_com_init()?

In qmp_combo_com_init(), if qmp->qmpphy_mode == QMPPHY_MODE_USB4, the driver
directly executes:

qmp_combo_com_init() {
    ...
    if (qmp->qmpphy_mode == QMPPHY_MODE_USB4)
        qphy_setbits(pcs_aon, cfg->regs[QPHY_PCS_USB4_CLAMP_ENABLE], CLAMP_EN);
    ...
}

Unlike other instances where pcs_aon is strictly checked (if (pcs_aon)) before
dereferencing, this assumes pcs_aon is populated. While this works on
x1e80100 because its layout defines .usb3_pcs_aon (in qmp_combo_offsets_v6_n4),
any future configuration supporting USB4 but omitting usb3_pcs_aon (e.g.,
sc8280xp which uses qmp_combo_offsets_v5) would trigger a NULL pointer
dereference.

[Severity: High]
Does enabling USB4 expose resource leaks in qmp_combo_usb4_init()?

If a failure occurs during initialization, qmp_combo_usb4_init() immediately
returns the error code:

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

However, it fails to call clk_disable_unprepare(qmp->p2rr2p_pipe_clk), which
was successfully enabled earlier in the function. Does this result in a
resource leak on the error paths for qmp_combo_com_init(),
qmp_combo_reconfigure_phy(), and qmp_combo_usb_power_on()?

[Severity: High]
Can switching from USB3 mode cause a double initialization of the PHY?

When qmp_combo_usb4_init() executes and USB3 is already active
(qmp->usb_init_count > 0), it calls:

qmp_combo_usb4_init() {
    ...
    ret = qmp_combo_reconfigure_phy(qmp, QMPPHY_MODE_USB4);
    if (ret)
        return ret;

    ret = qmp_combo_usb_power_on(phy);
    ...
}

Inside qmp_combo_reconfigure_phy(), if qmp->usb_init_count is non-zero, it
explicitly calls qmp_combo_usb_power_on(qmp->usb_phy).

After returning successfully, qmp_combo_usb4_init() proceeds to call
qmp_combo_usb_power_on(phy) a second time. Could writing the entire
configuration sequence (tuning parameters, clearing resets, setting
SERDES_START | PCS_START) to an actively running PHY cause hardware lockups
or polling timeouts?

[ ... ]

> 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_TX_UCDR_FASTLOCK_COUNTER_LOW_RATE1	0x224

[Severity: Low]
This isn't a bug, but as mentioned above, these UCDR macros are prefixed with
TX instead of RX despite UCDR being an RX feature.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730-topic-usb4phy-v3-0-ccf0f7ea98b5@oss.qualcomm.com?part=4

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