Message-ID: <aFzWiZ9ohbE_Unuz@pengutronix.de>
Date: Thu, 26 Jun 2025 07:11:37 +0200
From: Oleksij Rempel <o.rempel@...gutronix.de>
To: Lucas Stach <l.stach@...gutronix.de>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	kernel@...gutronix.de, Russell King <linux@...linux.org.uk>
Subject: Re: [PATCH net-next v2 1/1] phy: micrel: add Signal Quality
 Indicator (SQI) support for KSZ9477 switch PHYs

On Wed, Jun 25, 2025 at 08:06:32PM +0200, Lucas Stach wrote:
> Hi Oleksij,
> 
> Am Mittwoch, dem 25.06.2025 um 14:41 +0200 schrieb Oleksij Rempel:
> > 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.
> 
> I wonder if it wouldn't be more useful to report the worst SQI from all
> the channels instead.

It was my first idea too, just to report the worst SQI from all
channels. But this makes it impossible to report SQI for each pair
later. If we ever want to support SQI per pair, the current code would
suddenly start to show only SQI for pair A, not the worst one, so the
SQI interface would change meaning without warning.

There is another problem if we want to extend the SQI UAPI for per-pair
support: with 100Mbit/s links, we can't know which pair is used. The PHY
reports SQI only for the RX pair, which can change depending on MDI-X
resolution, and with auto MDI-X mode, this PHY doesn't tell us which
pair it is.

That means, at this point, we have hardware which in some modes can't
provide pair-related information. So, it is better to keep the already
existing UAPI explicitly per link instead of per pair. This matches the
current hardware limits and avoids confusion for users and developers.
If we want per-pair SQI in the future, the API must handle these cases
clearly.

> > 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.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@...gutronix.de>
> > ---
> > changes v2:
> > - Reword commit message
> > - Fix SQI value inversion
> > - Implement an empirically-derived, non-linear mapping to the standard
> >   0-7 SQI scale
> > ---
> >  drivers/net/phy/micrel.c | 124 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 124 insertions(+)
> > 
> > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> > index d0429dc8f561..6422d9a7c09f 100644
> > --- a/drivers/net/phy/micrel.c
> > +++ b/drivers/net/phy/micrel.c
> > @@ -2173,6 +2173,128 @@ static void kszphy_get_phy_stats(struct phy_device *phydev,
> >  	stats->rx_errors = priv->phy_stats.rx_err_pkt_cnt;
> >  }
> >  
> > +/* Base register for Signal Quality Indicator (SQI) - Channel A
> > + *
> > + * MMD Address: MDIO_MMD_PMAPMD (0x01)
> > + * Register:    0xAC (Channel A)
> > + * Each channel (pair) has its own register:
> > + *   Channel A: 0xAC
> > + *   Channel B: 0xAD
> > + *   Channel C: 0xAE
> > + *   Channel D: 0xAF
> > + */
> > +#define KSZ9477_MMD_SIGNAL_QUALITY_CHAN_A	0xAC
> > +
> > +/* SQI field mask for bits [14:8]
> > + *
> > + * SQI indicates relative quality of the signal.
> > + * A lower value indicates better signal quality.
> > + */
> > +#define KSZ9477_MMD_SQI_MASK			GENMASK(14, 8)
> > +
> > +#define KSZ9477_SQI_MAX				7
> > +
> > +/* Delay between consecutive SQI register reads in microseconds.
> > + *
> > + * Reference: KSZ9477S Datasheet DS00002392C, Section 4.1.11 (page 26)
> > + * The register is updated every 2 µs. Use 3 µs to avoid redundant reads.
> > + */
> > +#define KSZ9477_MMD_SQI_READ_DELAY_US		3
> > +
> > +/* Number of SQI samples to average for a stable result.
> > + *
> > + * Reference: KSZ9477S Datasheet DS00002392C, Section 4.1.11 (page 26)
> > + * For noisy environments, a minimum of 30–50 readings is recommended.
> > + */
> > +#define KSZ9477_SQI_SAMPLE_COUNT		40
> > +
> > +/* The hardware SQI register provides a raw value from 0-127, where a lower
> > + * value indicates better signal quality. However, empirical testing has
> > + * shown that only the 0-7 range is relevant for a functional link. A raw
> > + * value of 8 or higher was measured directly before link drop. This aligns
> > + * with the OPEN Alliance recommendation that SQI=0 should represent the
> > + * pre-failure state.
> > + *
> > + * This table provides a non-linear mapping from the useful raw hardware
> > + * values (0-7) to the standard 0-7 SQI scale, where higher is better.
> > + */
> > +static const u8 ksz_sqi_mapping[] = {
> > +	7, /* raw 0 -> SQI 7 */
> > +	7, /* raw 1 -> SQI 7 */
> > +	6, /* raw 2 -> SQI 6 */
> > +	5, /* raw 3 -> SQI 5 */
> > +	4, /* raw 4 -> SQI 4 */
> > +	3, /* raw 5 -> SQI 3 */
> > +	2, /* raw 6 -> SQI 2 */
> > +	1, /* raw 7 -> SQI 1 */
> > +};
> > +
> > +/**
> > + * 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;
> > +	else
> > +		return -EOPNOTSUPP;
> > +
> > +	/*
> > +	 * Sample and accumulate SQI readings for each pair (currently only one).
> > +	 *
> > +	 * Reference: KSZ9477S Datasheet DS00002392C, Section 4.1.11 (page 26)
> > +	 * - The SQI register is updated every 2 µs.
> > +	 * - Values may fluctuate significantly, even in low-noise environments.
> > +	 * - For reliable estimation, average a minimum of 30–50 samples
> > +	 *   (recommended for noisy environments)
> > +	 * - In noisy environments, individual readings are highly unreliable.
> > +	 *
> > +	 * We use 40 samples per pair with a delay of 3 µs between each
> > +	 * read to ensure new values are captured (2 µs update interval).
> > +	 */
> > +	for (i = 0; i < KSZ9477_SQI_SAMPLE_COUNT; i++) {
> > +		val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD,
> > +				   KSZ9477_MMD_SIGNAL_QUALITY_CHAN_A);
> > +		if (val < 0)
> > +			return val;
> > +
> > +		raw_sqi = FIELD_GET(KSZ9477_MMD_SQI_MASK, val);
> > +		sum += raw_sqi;
> > +
> > +		udelay(KSZ9477_MMD_SQI_READ_DELAY_US);
> 
> This ends up spending a sizable amount of time just spinning the CPU to
> collect the samples for the averaging. Given that only very low values
> seem to indicate a working link, I wonder how significant the
> fluctuations in reported link quality are in reality. Is it really
> worth spending 120us of CPU time to average those values?
> 
> Maybe a running average updated with a new sample each time this
> function is called would be sufficient?

Hm. Good point. I'l try it. We already have proper interface for this
case :)

Best Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Powered by blists - more mailing lists