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

[AngeloGioacchino Del Regno]

On 9/15/26 12:26, sashiko-bot at kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] phy: mediatek: endianness bug when reading split NVMEM cells in mtk_pcie_sphy_get_one_cal_para()
> --
> 
> commit ab276d5dc9414b3b5618ea96c84dcc086bdf9036
> Author: AngeloGioacchino Del Regno <angelogioacchino.delregno at collabora.com>
> 
> phy: mediatek: Add support for PCI-Express Gen4 S-PHY
> 
> This commit introduces the driver for the PCI-Express Gen4 S-PHY found in
> recent MediaTek SoCs such as MT6991 and MT8196. It implements the PHY
> initialization routines and handles reading eFuse calibration data from
> the NVMEM framework.
> 
>> diff --git a/drivers/phy/mediatek/phy-mtk-pcie-sphy.c b/drivers/phy/mediatek/phy-mtk-pcie-sphy.c
>> new file mode 100644
>> index 0000000000000..2bf10346ea97a
>> --- /dev/null
>> +++ b/drivers/phy/mediatek/phy-mtk-pcie-sphy.c
> [ ... ]
>> +static int mtk_pcie_sphy_get_one_cal_para(struct device *dev, const char *name, u8 max_val)
>> +{
>> +	u16 buf;
>> +	u8 tmp;
>> +	int ret;
>> +
>> +	/*
>> +	 * All of the calibrations are always max 8 bits long, but some may
>> +	 * be split between two different 8-bits cells: handle this corner
>> +	 * case by retrying reading as u16.
>> +	 */
>> +	ret = nvmem_cell_read_u8(dev, name, &tmp);
>> +	if (ret == 0)
>> +		buf = tmp;
>> +	else
>> +		ret = nvmem_cell_read_u16(dev, name, &buf);
> 
> [Severity: Medium]
> When an 8-bit (or smaller) NVMEM cell crosses a byte boundary in hardware,
> the NVMEM core evaluates its length as 2 bytes, causing nvmem_cell_read_u8()
> to fail with -EINVAL.
> 
> Does falling back to nvmem_cell_read_u16() cause an endianness issue here?

No, it reads fine.

> 
> Because the NVMEM core populates the buffer with a direct memory copy, the
> shifted valid data is placed in the first byte and zeroes in the second byte.
> On big-endian architectures

But this runs only on LE architectures.

>, interpreting this buffer directly as a native
> u16 variable evaluates to buf[0] << 8, which effectively multiplies the
> calibration value by 256. This could lead to spurious -ERANGE failures or
> incorrect calibration values being written to the PHY.
> 
> Would it be safer to use nvmem_cell_read_variable_le_u32() to handle these
> split cells robustly across different architectures?
> 
>> +
>> +	if (ret == -ENOENT) {
>> +		dev_info(dev, "No calibration for %s. Using defaults\n", name);
>> +		return -ENOENT;
>> +	} else if (ret)
>> +		return dev_err_probe(dev, ret,
>> +				     "Cannot get calibration data for %s\n", name);
>> +
>> +	if (buf > max_val)
>> +		return dev_err_probe(dev, -ERANGE,
>> +				     "Bad value %u retrieved for %s.\n", buf, name);
>> +
>> +	return buf;
>> +}
> [ ... ]
>