Re: [PATCH 2/2] phy: qcom-qmp-pcie: Add support for ipq5210 PCIe phys

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Redundant array duplication and misleading naming for PHY init tables
--

commit 1a4fb65a84494415ea2c151ef7e1a6c4328d3d7f
Author: Varadarajan Narayanan <varadarajan.narayanan@oss.qualcomm.com>

phy: qcom-qmp-pcie: Add support for ipq5210 PCIe phys

This commit adds support for the PCIe PHYs found on the Qualcomm
ipq5210 platform.

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> @@ -514,6 +514,105 @@ static const struct qmp_phy_init_tbl ipq8074_pcie_gen3_pcs_misc_tbl[] = {
[ ... ]
> +static const struct qmp_phy_init_tbl ipq5210_gen3x1_pcie_ep_tx_tbl[] = {
> +	QMP_PHY_INIT_CFG(QSERDES_V4_TX_RES_CODE_LANE_OFFSET_TX, 0x02),
> +	QMP_PHY_INIT_CFG(QSERDES_V4_TX_LANE_MODE_1, 0x06),
> +	QMP_PHY_INIT_CFG(QSERDES_V4_TX_RCV_DETECT_LVL_2, 0x12),
> +};

Does this new table duplicate the existing ipq6018_pcie_tx_tbl? Could we
reuse the existing table instead of creating a redundant copy?

[ ... ]
> +static const struct qmp_phy_init_tbl ipq5210_gen3x1_pcie_ep_pcs_tbl[] = {
> +	QMP_PHY_INIT_CFG(QPHY_V4_PCS_FLL_CNTRL1, 0x01),
> +	QMP_PHY_INIT_CFG(QPHY_V4_PCS_REFGEN_REQ_CONFIG1, 0x0d),
> +	QMP_PHY_INIT_CFG(QPHY_V4_PCS_G12S1_TXDEEMPH_M3P5DB, 0x10),
> +	QMP_PHY_INIT_CFG(QPHY_V4_PCS_RX_SIGDET_LVL, 0xaa),
> +	QMP_PHY_INIT_CFG(QPHY_V4_PCS_P2U3_WAKEUP_DLY_TIME_AUXCLK_L, 0x01),
> +	QMP_PHY_INIT_CFG(QPHY_V4_PCS_RX_DCC_CAL_CONFIG, 0x01),
> +	QMP_PHY_INIT_CFG(QPHY_V4_PCS_EQ_CONFIG5, 0x01),
> +};

Is it possible this is an exact duplicate of ipq6018_pcie_pcs_tbl?

[ ... ]
> @@ -654,6 +753,13 @@ static const struct qmp_phy_init_tbl ipq9574_gen3x2_pcie_serdes_tbl[] = {
[ ... ]
> +static const struct qmp_phy_init_tbl ipq9574_pcie_tx_tbl[] = {
> +	QMP_PHY_INIT_CFG(QSERDES_V4_TX_RES_CODE_LANE_OFFSET_TX, 0x02),
> +	QMP_PHY_INIT_CFG(QSERDES_V4_TX_RCV_DETECT_LVL_2, 0x12),
> +	QMP_PHY_INIT_CFG(QSERDES_V4_TX_HIGHZ_DRVR_EN, 0x10),
> +	QMP_PHY_INIT_CFG(QSERDES_V4_TX_LANE_MODE_1, 0x06),
> +};

Could this table be entirely replaced by ipq8074_pcie_gen3_tx_tbl?

Also, does the name ipq9574_pcie_tx_tbl accurately reflect its usage? It
seems to be used exclusively by the newly added ipq5210_gen3x1_pciephy_cfg,
while the actual IPQ9574 configurations retain their use of the ipq8074 table.

[ ... ]
> @@ -3613,6 +3719,49 @@ static const struct qmp_phy_cfg ipq6018_pciephy_cfg = {
[ ... ]
> +static const struct qmp_phy_cfg ipq5210_gen3x1_pciephy_cfg = {
> +	.lanes			= 1,
> +
> +	.offsets		= &qmp_pcie_offsets_v4x1,
> +
> +	.tbls =  {
> +		.serdes		= ipq9574_gen3x1_pcie_serdes_tbl,
> +		.serdes_num	= ARRAY_SIZE(ipq9574_gen3x1_pcie_serdes_tbl),
> +		.tx		= ipq9574_pcie_tx_tbl,
> +		.tx_num		= ARRAY_SIZE(ipq9574_pcie_tx_tbl),

If ipq9574_pcie_tx_tbl is identical to ipq8074_pcie_gen3_tx_tbl, should
this reference the existing ipq8074_pcie_gen3_tx_tbl instead?

> +		.rx		= ipq9574_pcie_rx_tbl,
> +		.rx_num		= ARRAY_SIZE(ipq9574_pcie_rx_tbl),
[ ... ]
> +	.tbls_ep = &(const struct qmp_phy_cfg_tbls) {
> +		.serdes		= ipq5210_gen3x1_pcie_ep_serdes_tbl,
> +		.serdes_num	= ARRAY_SIZE(ipq5210_gen3x1_pcie_ep_serdes_tbl),
> +		.tx		= ipq5210_gen3x1_pcie_ep_tx_tbl,
> +		.tx_num		= ARRAY_SIZE(ipq5210_gen3x1_pcie_ep_tx_tbl),
> +		.rx		= ipq5210_gen3x1_pcie_ep_rx_tbl,
> +		.rx_num		= ARRAY_SIZE(ipq5210_gen3x1_pcie_ep_rx_tbl),
> +		.pcs		= ipq5210_gen3x1_pcie_ep_pcs_tbl,
> +		.pcs_num	= ARRAY_SIZE(ipq5210_gen3x1_pcie_ep_pcs_tbl),

Similarly, could these point to ipq6018_pcie_tx_tbl and
ipq6018_pcie_pcs_tbl respectively?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514-pci-phy-v1-0-482429192746@oss.qualcomm.com?part=2