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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] phy: qualcomm: incorrectly named TX macro for RX UCDR FASTLOCK registers
- [Low] phy: qualcomm: typo LPFS instead of LFPS in PCS USB4 macro
--

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

phy: qualcomm: qmp-combo: Add USB4/TBT3 configuration data for Hamoa

This commit adds offsets and configuration tables to support USB4 and
Thunderbolt 3 operation on USB4-capable PHYs for Hamoa chips. It exposes a
third PHY structure for the USB4 sub-PHY to be toggled by the USB4 router
driver.

> 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
[ ... ]
> @@ -2028,6 +2107,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),
> +	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]
Do these macros have an incorrect _TX_ prefix instead of _RX_? They are
being used in the RX initialization table x1e80100_usb4_rx_tbl and evaluate to
an offset within the RX block address space.

> +	QMP_PHY_INIT_CFG(QSERDES_V6_N4_RX_UCDR_PI_CTRL1, 0xd0),
[ ... ]
> @@ -2066,6 +2207,23 @@ static const struct qmp_phy_init_tbl x1e80100_usb43dp_rx_tbl[] = {
>  	QMP_PHY_INIT_CFG(QSERDES_V6_N4_RX_RX_BKUP_CTRL1, 0x14),
>  };
>  
> +static const struct qmp_phy_init_tbl x1e80100_usb4_pcs_tbl[] = {
> +	QMP_PHY_INIT_CFG(QPHY_V6_PCS_USB4_POWER_STATE_CONFIG3, 0x4f),
[ ... ]
> +	QMP_PHY_INIT_CFG(QPHY_V6_PCS_USB4_G3_FOM_EQ_CONFIG3, 0x25),
> +	QMP_PHY_INIT_CFG(QPHY_V6_PCS_USB4_G3_FOM_EQ_CONFIG5, 0x22),
> +	QMP_PHY_INIT_CFG(QPHY_V6_PCS_USB4_LPFS_TX_ECSTART, 0x0f),

[Severity: Low]
This is a minor typo, but should LPFS be LFPS (Low Frequency Periodic
Signaling) in this macro name?

> +};
[ ... ]
> 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_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 another instance where LPFS should be LFPS?

> +
>  #endif
> 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_RX_UCDR_FASTLOCK_FO_GAIN_RATE3	0x200
> +#define QSERDES_V6_N4_RX_UCDR_FASTLOCK_SO_GAIN_RATE3	0x210
> +#define QSERDES_V6_N4_RX_UCDR_SO_SATURATION		0x214
> +#define QSERDES_V6_N4_TX_UCDR_FASTLOCK_COUNTER_LOW_RATE1	0x224

[Severity: Low]
Does this define incorrectly use the _TX_ prefix instead of _RX_? This
block of macros appears to fall within the RX address space based on the
surrounding offsets.

> +#define QSERDES_V6_N4_TX_UCDR_FASTLOCK_COUNTER_HIGH_RATE1	0x228
> +#define QSERDES_V6_N4_TX_UCDR_FASTLOCK_COUNTER_LOW_RATE2	0x22c
> +#define QSERDES_V6_N4_TX_UCDR_FASTLOCK_COUNTER_HIGH_RATE2	0x230
> +#define QSERDES_V6_N4_TX_UCDR_FASTLOCK_COUNTER_LOW_RATE3	0x234
> +#define QSERDES_V6_N4_TX_UCDR_FASTLOCK_COUNTER_HIGH_RATE3	0x238
>  #define QSERDES_V6_N4_RX_UCDR_PI_CTRL1			0x23c
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928-topic-usb4phy-v6-1-815a73b063ef@oss.qualcomm.com?part=1

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