Re: [PATCH v2 2/2] phy: mediatek: Add support for PCI-Express Gen4 S-PHY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Improper handling of missing or unsupported NVMEM calibration data causes spurious error logs and hard probe failures.
- [Medium] A valid calibration value of 0 is incorrectly treated as missing data, discarding all calibrations.
- [Low] The TX_IMPSEL registers are updated twice with identical values during initialization.
- [Low] Macros for AEQ_EGEQ_RATIO are defined but unused in favor of literal magic numbers.
--

--- Patch [2]: [PATCH v2 2/2] phy: mediatek: Add support for PCI-Express Gen4 S-PHY ---
commit d91c6c0718440c44adc7e6a83f5aeca706a6c3d5
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Subject: phy: mediatek: Add support for PCI-Express Gen4 S-PHY

This patch adds support for the PCI-Express Gen4 S-PHY found in the latest
MediaTek SoCs, including MT8196 and MT8894.

> --- a/drivers/phy/mediatek/phy-mtk-pcie-sphy.c
> +++ b/drivers/phy/mediatek/phy-mtk-pcie-sphy.c
[ ... ]
> +#define PEXTP_DIG_LN_RX2_AEQ_EDGE_0			0x6004
> +#  define RG_XTP_LN_RX_AEQ_EGEQ_RATIO_GEN3		GENMASK(21, 16)
> +#  define RG_XTP_LN_RX_AEQ_EGEQ_RATIO_GEN4		GENMASK(29, 24)
> +#  define AEQ_EGEQ_RATIO_GEN3_TO_22			0x16
> +#  define AEQ_EGEQ_RATIO_GEN4_TO_22			0x16
[ ... ]
> +		mtk_phy_update_field(sif_lane_base + PEXTP_DIG_LN_RX2_AEQ_EDGE_0,
> +				     RG_XTP_LN_RX_AEQ_EGEQ_RATIO_GEN3, 22);
> +
> +		mtk_phy_update_field(sif_lane_base + PEXTP_DIG_LN_RX2_AEQ_EDGE_0,
> +				     RG_XTP_LN_RX_AEQ_EGEQ_RATIO_GEN4, 22);

[Severity: Low]
Is there a reason the literal 22 is used here instead of the
AEQ_EGEQ_RATIO_GEN3_TO_22 and AEQ_EGEQ_RATIO_GEN4_TO_22 macros defined
earlier?

[ ... ]
> +static void mtk_pcie_sphy_apply_calibration(struct mtk_pcie_sphy *pcie_sphy)
> +{
[ ... ]
> +		mtk_phy_update_field(sif_lane_base + PEXTP_ANA_LN_TRX_A0,
> +				     RG_XTP_LN_TX_IMPSEL_PMOS, cal->tx_impsel.pmos);
> +
> +		mtk_phy_update_field(sif_lane_base + PEXTP_ANA_LN_TRX_A0,
> +				     RG_XTP_LN_TX_IMPSEL_NMOS, cal->tx_impsel.nmos);
> +	}
> +}
> +
[ ... ]
> +static int mtk_pcie_sphy_init(struct phy *phy)
> +{
[ ... ]
> +	if (pcie_sphy->calibration.supported) {
> +		mtk_pcie_sphy_apply_calibration(pcie_sphy);
> +
> +		tx_impsel.pmos = pcie_sphy->calibration.tx_impsel.pmos;
> +		tx_impsel.nmos = pcie_sphy->calibration.tx_impsel.nmos;
> +	} else {
> +		/* Set P=10, N=9 to prevent EMI if no calibration present */
> +		tx_impsel.pmos = 10;
> +		tx_impsel.nmos = 9;
> +	}
> +
> +	/* Select TX Impedance on N and P MOSFETs */
> +	for (i = 0; i < pcie_sphy->num_lanes; i++) {
> +		void __iomem *sif_lane_base = pcie_sphy->sif_base + PEXTP_REG_LANE(i);
> +
> +		mtk_phy_update_field(sif_lane_base + PEXTP_ANA_LN_TRX_A0,
> +				     RG_XTP_LN_TX_IMPSEL_PMOS, tx_impsel.pmos);
> +
> +		mtk_phy_update_field(sif_lane_base + PEXTP_ANA_LN_TRX_A0,
> +				     RG_XTP_LN_TX_IMPSEL_NMOS, tx_impsel.nmos);
> +	}

[Severity: Low]
Are these register updates redundant?

When calibration is supported, mtk_pcie_sphy_apply_calibration() already
writes cal->tx_impsel.pmos and cal->tx_impsel.nmos to PEXTP_ANA_LN_TRX_A0.

Then this loop immediately overwrites the same registers with the exact same
values.

[ ... ]
> +static int mtk_pcie_sphy_get_one_cal_para(struct device *dev, const char *name, u8 max_val)
> +{
[ ... ]
> +	ret = nvmem_cell_read_u8(dev, name, &tmp);
> +	if (ret == 0)
> +		buf = tmp;
> +	else
> +		ret = nvmem_cell_read_u16(dev, name, &buf);
> +
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Cannot get calibration data for %s\n", name);

[Severity: High]
Will this cause spurious error logs or fail the probe when calibration data is
missing or unsupported?

If the system does not have CONFIG_NVMEM enabled, nvmem_cell_read_u8() returns
-EOPNOTSUPP, which gets logged as an error here.

This error is then passed back up to mtk_pcie_sphy_get_calibration_data(),
where it is not caught. This will cause the entire driver probe to fail.

Similarly, if the cell is simply missing from the Device Tree, it returns
-ENOENT. This also triggers an error log via dev_err_probe(), even though
missing calibration data is explicitly documented as non-fatal and optional.

> +
> +	if (buf == 0) {
> +		dev_info(dev, "No calibration for %s. Using defaults\n", name);
> +		return -ENOENT;
> +	}

[Severity: Medium]
Is a value of 0 always indicative of missing calibration?

If a valid tuned calibration value is 0, this check will return -ENOENT and
cause mtk_pcie_sphy_get_calibration_data() to discard all calibrations entirely.

This also seems to prevent the quirk for cal-version == 0 from ever being
applied:

[ ... ]
> +static int mtk_pcie_sphy_get_calibration_data(struct mtk_pcie_sphy *pcie_sphy)
> +{
[ ... ]
> +	ret = mtk_pcie_sphy_get_one_cal_para(dev, "cal-version", 15);
> +	if (ret < 0)
> +		goto end;
> +	version = ret;
> +
> +	/* Quirk for eFuse calibration table versions 0 to 4 */
> +	if ((version <= MTK_PCIE_SPHY_CALIBRATION_LAST_QUIRK_VER) &&
> +	    cal->rx_leq_rl_ctle == 10) {

Because mtk_pcie_sphy_get_one_cal_para() returns -ENOENT when it reads a 0,
the version == 0 condition will never be reached.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720135344.84183-1-angelogioacchino.delregno@collabora.com?part=2

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