Message-ID: <5a094e3b95f1219435056d87ca4f643398bcb1d3.camel@pengutronix.de>
Date: Wed, 25 Jun 2025 20:06:32 +0200
From: Lucas Stach <l.stach@...gutronix.de>
To: Oleksij Rempel <o.rempel@...gutronix.de>, 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>
Cc: 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

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.

> 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?

Regards,
Lucas

> +	}
> +
> +	avg_raw_sqi = sum / KSZ9477_SQI_SAMPLE_COUNT;
> +
> +	/* Handle the pre-fail/failed state first. */
> +	if (avg_raw_sqi >= ARRAY_SIZE(ksz_sqi_mapping))
> +		return 0;
> +
> +	/* Use the lookup table for the good signal range. */
> +	return ksz_sqi_mapping[avg_raw_sqi];
> +}
> +
> +static int kszphy_get_sqi_max(struct phy_device *phydev)
> +{
> +	return KSZ9477_SQI_MAX;
> +}
> +
>  static void kszphy_enable_clk(struct phy_device *phydev)
>  {
>  	struct kszphy_priv *priv = phydev->priv;
> @@ -5801,6 +5923,8 @@ static struct phy_driver ksphy_driver[] = {
>  	.update_stats	= kszphy_update_stats,
>  	.cable_test_start	= ksz9x31_cable_test_start,
>  	.cable_test_get_status	= ksz9x31_cable_test_get_status,
> +	.get_sqi	= kszphy_get_sqi,
> +	.get_sqi_max	= kszphy_get_sqi_max,
>  } };
>  
>  module_phy_driver(ksphy_driver);

Powered by blists - more mailing lists