Re: [PATCH v5 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:

New issues:
- [Low] Incorrect prefix for RX UCDR FASTLOCK_COUNTER registers.
- [Low] Typo in USB4 LFPS register name (written as LPFS).
- [Low] Register sort order violation in header file.

Pre-existing issues:
- [Critical] Unclocked and unpowered register access in qmp_combo_com_init when in USB4 mode.
--

commit dbef849ad62b34a45390169ec84b4136bf981184
Author: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>

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 a4f130fc33e39..284efc3f23d42 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> @@ -240,6 +240,7 @@ static const unsigned int qmp_v6_n4_usb3phy_regs_layout[QPHY_LAYOUT_SIZE] = {
>  	[QPHY_PCS_LFPS_RXTERM_IRQ_CLEAR] = QPHY_V6_PCS_USB3_LFPS_RXTERM_IRQ_CLEAR,
>  
>  	[QPHY_PCS_CLAMP_ENABLE]		= QPHY_V6_PCS_AON_CLAMP_ENABLE,
> +	[QPHY_PCS_USB4_CLAMP_ENABLE]	= QPHY_V6_PCS_AON_USB4_CLAMP_ENABLE,

[Severity: Critical]
This is a pre-existing issue, but providing this layout entry exposes
qmp_combo_com_init() to an unclocked and unpowered register access in
USB4 mode.

When qmp->qmpphy_mode == QMPPHY_MODE_USB4, qmp_combo_com_init() immediately
executes qphy_setbits() using QPHY_PCS_USB4_CLAMP_ENABLE. This occurs before
regulator_bulk_enable() and clk_bulk_prepare_enable() are called later in
the function.

Accessing unclocked and unpowered APB/AHB mapped registers can cause a
synchronous external abort (bus hang). Could this access be deferred until
after the clocks and regulators are enabled?

[ ... ]
> @@ -2028,6 +2107,68 @@ static const struct qmp_phy_init_tbl x1e80100_usb43dp_tx_tbl[] = {
[ ... ]
> +static const struct qmp_phy_init_tbl x1e80100_usb4_rx_tbl[] = {
[ ... ]
> +	QMP_PHY_INIT_CFG(QSERDES_V6_N4_TX_UCDR_FASTLOCK_COUNTER_LOW_RATE2, 0x30),
> +	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),

[Severity: Low]
Are these TX macros intentionally placed in the rx_tbl, or do they refer to
RX UCDR FASTLOCK_COUNTER registers?

If they are UCDR registers mapping into the RX hardware block, should they
have the QSERDES_V6_N4_RX_ prefix instead to prevent confusion?

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcs-usb-v6.h b/drivers/phy/qualcomm/phy-qcom-qmp-pcs-usb-v6.h
> index df670143feb1e..30fdb437146c4 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcs-usb-v6.h
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcs-usb-v6.h
> @@ -14,4 +14,19 @@
[ ... ]
> +#define QPHY_V6_PCS_USB4_G3_FOM_EQ_CONFIG3		0x210
> +#define QPHY_V6_PCS_USB4_G3_FOM_EQ_CONFIG5		0x218
> +#define QPHY_V6_PCS_USB4_LPFS_TX_ECSTART		0x220

[Severity: Low]
Is this intended to be LFPS (Low Frequency Periodic Signaling) instead of
LPFS? It might be clearer to name it QPHY_V6_PCS_USB4_LFPS_TX_ECSTART to
match standard USB protocol terminology.

> 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_FO_GAIN_RATE2		0x8
> +#define QSERDES_V6_N4_RX_UCDR_FO_GAIN_RATE3		0xc
> +#define QSERDES_V6_N4_RX_UCDR_SO_GAIN_RATE3		0x1c
>  #define QSERDES_V6_N4_RX_UCDR_SO_GAIN_RATE2		0x18

[Severity: Low]
Does this break the numerical sort order of the register offsets? Offset
0x1c is defined here before 0x18.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-topic-usb4phy-v5-0-73aac69578ef@oss.qualcomm.com?part=4