Re: [PATCH v4 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 3 potential issue(s) to consider:
- [Low] Typo in macro name: LPFS instead of LFPS.
- [Low] RX registers mistakenly prefixed with _TX_.
- [Low] Missing _N4 prefix in macro definition.
--

--- Patch [4]: [PATCH v4 4/5] phy: qualcomm: qmp-combo: Add USB4/TBT3 configuration data for Hamoa ---
commit afe3f2e27c72fec44781edf41959e4fc3ad27e3e
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 dfdeec3ee4f5f..c150a4ef03a78 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c

[ ... ]

> @@ -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_RX_SIGDET_CNTRL, 0x04),

[ ... ]

> +	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]
This isn't a bug, but should these UCDR (Universal Clock and Data Recovery)
registers be prefixed with _RX_ instead of _TX_? They are functionally
receiver registers falling within the RX offset block, and are being
initialized here inside x1e80100_usb4_rx_tbl.

[ ... ]

> 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_USB3_RCVR_DTCT_DLY_U3_L		0x40
>  #define QPHY_V6_PCS_USB3_RCVR_DTCT_DLY_U3_H		0x44
>  
> +#define QPHY_V6_PCS_USB4_POWER_STATE_CONFIG3		0x98

[ ... ]

> +#define QPHY_V6_PCS_USB4_G3_FOM_EQ_CONFIG5		0x218
> +#define QPHY_V6_PCS_USB4_LPFS_TX_ECSTART		0x220

[Severity: Low]
This isn't a bug, but is there a typo in this macro name? It uses LPFS
instead of LFPS (Low Frequency Periodic Signaling).

[ ... ]

> 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_TRAN_DRVR_EMP_EN		0xac
>  #define QSERDES_V6_N4_TX_TX_BAND			0xd8
>  #define QSERDES_V6_N4_TX_INTERFACE_SELECT		0xe4

[ ... ]

> +#define QSERDES_V6_N4_TX_RX_MARG_COARSE_THRESH3_RATE210	0xf8
> +#define QSERDES_V6_N4_TX_RX_MARG_COARSE_THRESH3_RATE3	0xfc
>  #define QSERDES_V6_N4_TX_VMODE_CTRL1			0xb0
> +#define QSERDES_V6_TX_LDO_TIMER_CTRL			0x120

[Severity: Low]
This isn't a bug, but is this macro missing the _N4 infix? All other
SoC-specific macros added in this header use the _N4 format (e.g.,
QSERDES_V6_N4_TX_LANE_MODE_1).

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