Re: [PATCH net-next v2 1/1] phy: micrel: add Signal Quality Indicator (SQI) support for KSZ9477 switch PHYs

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi Oleksij,

On Wed, 25 Jun 2025 14:41:26 +0200
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> Add support for the Signal Quality Index (SQI) feature on KSZ9477 family
> switches. This feature provides a relative measure of receive signal
> quality.
> 
> The KSZ9477 PHY provides four separate SQI values for a 1000BASE-T link,
> one for each differential pair (Channel A-D). Since the current get_sqi
> UAPI only supports returning a single value per port, this
> implementation reads the SQI from Channel A as a representative metric.
> This can be extended to provide per-channel readings once the UAPI is
> enhanced for multi-channel support.
> 
> The hardware provides a raw 7-bit SQI value (0-127), where lower is
> better. This raw value is converted to the standard 0-7 scale to provide
> a usable, interoperable metric for userspace tools, abstracting away
> hardware-specifics. The mapping to the standard 0-7 SQI scale was
> determined empirically by injecting a 30MHz sine wave into the receive
> pair with a signal generator. It was observed that the link becomes
> unstable and drops when the raw SQI value reaches 8. This
> implementation is based on these test results.

[...]

> +/**
> + * kszphy_get_sqi - Read, average, and map Signal Quality Index (SQI)
> + * @phydev: the PHY device
> + *
> + * This function reads and processes the raw Signal Quality Index from the
> + * PHY. Based on empirical testing, a raw value of 8 or higher indicates a
> + * pre-failure state and is mapped to SQI 0. Raw values from 0-7 are
> + * mapped to the standard 0-7 SQI scale via a lookup table.
> + *
> + * Return: SQI value (0–7), or a negative errno on failure.
> + */
> +static int kszphy_get_sqi(struct phy_device *phydev)
> +{
> +	int sum = 0;
> +	int i, val, raw_sqi, avg_raw_sqi;
> +	u8 channels;
> +
> +	/* Determine applicable channels based on link speed */
> +	if (phydev->speed == SPEED_1000)
> +		/* TODO: current SQI API only supports 1 channel. */
> +		channels = 1;
> +	else if (phydev->speed == SPEED_100)
> +		channels = 1;

I understand the placeholder logic waiting for some improved uAPI, but
this triggers an unused variable warning :( I think the commit log and
the comment below are enough to explain that this can be improved later
on.
	
> +	else
> +		return -EOPNOTSUPP;
> +
> +	/*
> +	 * Sample and accumulate SQI readings for each pair (currently only one).

Maxime