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

[AngeloGioacchino Del Regno]

On 7/20/26 16:13, sashiko-bot at kernel.org wrote:
> 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 at 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?
> 

I forgot to remove those definitions. Will do for a v3 tomorrow.

> [ ... ]
>> +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.
> 

Eh yes, fixed for v3.

> [ ... ]
>> +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:
> 

Fixed for v3.

> [ ... ]
>> +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.
> 

Fixed for v3.