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

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

[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.
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@pengutronix.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);
+	}
+
+	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);
-- 
2.39.5

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

[Maxime Chevallier]

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

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

[Oleksij Rempel]

On Wed, Jun 25, 2025 at 05:33:23PM +0200, Maxime Chevallier wrote:
> 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.

Grr.. sorry.. 

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

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

[Lucas Stach]

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@pengutronix.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);

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

[Oleksij Rempel]

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@pengutronix.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 |

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

[Oleksij Rempel]

On Thu, Jun 26, 2025 at 07:11:38AM +0200, Oleksij Rempel wrote:
> 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 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 :)

After some more testing with a signal generator, I started to doubt the
usability of our SQI hardware implementation in this case.

The problem is: the signal issue can only be detected if data transfer is
ongoing - more specifically, if data is being received (for example, when
running iperf). The SQI register on this hardware is updated every 3 µs. This
means if any data is received within this window, we can detect noise on the
wire. But if there is no transfer, the SQI register always shows perfect link
quality.

On the other hand, as long as noise or a sine wave is injected into the twisted
pair, it is possible to see bandwidth drops in iperf, but no other error
counters indicate problems. Even the RxErr counter on the PHY side stays silent
(it seems to detect only other kinds of errors). If SQI is actively polled
during this time, it will show a worse value - so in general, SQI seems to be
usable.

At an early stage of the SQI implementation, I didn’t have a strong opinion
about the need to differentiate these interfaces. But now, based on practical
experience, I see that the difference is very important.

It looks like we have two types of SQI hardware implementations:

- Hardware which provides the worst value since last read

- Hardware which automatically updates the value every N microseconds

- Hardware which provides both values

Both types are recommended by the Open Alliance as:

- "worst case SQI value since last read"

- "current SQI value"

My question is: do we really need both interfaces? The "current SQI value"
seems impractical if it only reflects quality during active data transfer.

What do you think?

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 |

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

[kernel test robot]

Hi Oleksij,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Oleksij-Rempel/phy-micrel-add-Signal-Quality-Indicator-SQI-support-for-KSZ9477-switch-PHYs/20250625-204330
base:   net-next/main
patch link:    https://lore.kernel.org/r/20250625124127.4176960-1-o.rempel%40pengutronix.de
patch subject: [PATCH net-next v2 1/1] phy: micrel: add Signal Quality Indicator (SQI) support for KSZ9477 switch PHYs
config: i386-buildonly-randconfig-004-20250626 (https://download.01.org/0day-ci/archive/20250626/202506260756.KhOdmLCy-lkp@intel.com/config)
compiler: clang version 20.1.7 (https://github.com/llvm/llvm-project 6146a88f60492b520a36f8f8f3231e15f3cc6082)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250626/202506260756.KhOdmLCy-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506260756.KhOdmLCy-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/net/phy/micrel.c:2247:5: warning: variable 'channels' set but not used [-Wunused-but-set-variable]
    2247 |         u8 channels;
         |            ^
   1 warning generated.


vim +/channels +2247 drivers/net/phy/micrel.c

  2231	
  2232	/**
  2233	 * kszphy_get_sqi - Read, average, and map Signal Quality Index (SQI)
  2234	 * @phydev: the PHY device
  2235	 *
  2236	 * This function reads and processes the raw Signal Quality Index from the
  2237	 * PHY. Based on empirical testing, a raw value of 8 or higher indicates a
  2238	 * pre-failure state and is mapped to SQI 0. Raw values from 0-7 are
  2239	 * mapped to the standard 0-7 SQI scale via a lookup table.
  2240	 *
  2241	 * Return: SQI value (0–7), or a negative errno on failure.
  2242	 */
  2243	static int kszphy_get_sqi(struct phy_device *phydev)
  2244	{
  2245		int sum = 0;
  2246		int i, val, raw_sqi, avg_raw_sqi;
> 2247		u8 channels;
  2248	
  2249		/* Determine applicable channels based on link speed */
  2250		if (phydev->speed == SPEED_1000)
  2251			/* TODO: current SQI API only supports 1 channel. */
  2252			channels = 1;
  2253		else if (phydev->speed == SPEED_100)
  2254			channels = 1;
  2255		else
  2256			return -EOPNOTSUPP;
  2257	
  2258		/*
  2259		 * Sample and accumulate SQI readings for each pair (currently only one).
  2260		 *
  2261		 * Reference: KSZ9477S Datasheet DS00002392C, Section 4.1.11 (page 26)
  2262		 * - The SQI register is updated every 2 µs.
  2263		 * - Values may fluctuate significantly, even in low-noise environments.
  2264		 * - For reliable estimation, average a minimum of 30–50 samples
  2265		 *   (recommended for noisy environments)
  2266		 * - In noisy environments, individual readings are highly unreliable.
  2267		 *
  2268		 * We use 40 samples per pair with a delay of 3 µs between each
  2269		 * read to ensure new values are captured (2 µs update interval).
  2270		 */
  2271		for (i = 0; i < KSZ9477_SQI_SAMPLE_COUNT; i++) {
  2272			val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD,
  2273					   KSZ9477_MMD_SIGNAL_QUALITY_CHAN_A);
  2274			if (val < 0)
  2275				return val;
  2276	
  2277			raw_sqi = FIELD_GET(KSZ9477_MMD_SQI_MASK, val);
  2278			sum += raw_sqi;
  2279	
  2280			udelay(KSZ9477_MMD_SQI_READ_DELAY_US);
  2281		}
  2282	
  2283		avg_raw_sqi = sum / KSZ9477_SQI_SAMPLE_COUNT;
  2284	
  2285		/* Handle the pre-fail/failed state first. */
  2286		if (avg_raw_sqi >= ARRAY_SIZE(ksz_sqi_mapping))
  2287			return 0;
  2288	
  2289		/* Use the lookup table for the good signal range. */
  2290		return ksz_sqi_mapping[avg_raw_sqi];
  2291	}
  2292	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki