RE: [PATCH net-next] net: phy: micrel: Adding SQI support for lan8814 phy

Hi Michael,

-----Original Message-----
From: Michael Walle <redacted>
Sent: Friday, August 26, 2022 2:56 PM
To: Divya Koppera - I30481 <redacted>; Oleksij Rempel
[off-list ref]
Cc: UNGLinuxDriver <UNGLinuxDriver@microchip.com>; andrew@lunn.ch;
davem@davemloft.net; edumazet@google.com; hkallweit1@gmail.com;
kuba@kernel.org; linux-kernel@vger.kernel.org; linux@armlinux.org.uk;
netdev@vger.kernel.org; pabeni@redhat.com
Subject: Re: [PATCH net-next] net: phy: micrel: Adding SQI support for
lan8814 phy

EXTERNAL EMAIL: Do not click links or open attachments unless you know the
content is safe

[+ Oleksij Rempel]

Hi,

Am 2022-08-26 11:11, schrieb Divya.Koppera@microchip.com:

quoted

quoted

quoted

Supports SQI(Signal Quality Index) for lan8814 phy, where it has
SQI index of 0-7 values and this indicator can be used for cable
integrity diagnostic and investigating other noise sources.

Signed-off-by: Divya Koppera <redacted>

..

quoted

quoted

quoted

+#define LAN8814_DCQ_CTRL_CHANNEL_MASK

GENMASK(1,

quoted

quoted

0)

quoted

+#define LAN8814_DCQ_SQI                                      0xe4
+#define LAN8814_DCQ_SQI_MAX                          7
+#define LAN8814_DCQ_SQI_VAL_MASK                     GENMASK(3, 1)
+
 static int lanphy_read_page_reg(struct phy_device *phydev, int
page,
u32 addr)  {
      int data;

@@ -2927,6 +2934,32 @@ static int lan8814_probe(struct phy_device

*phydev)

quoted

      return 0;
 }

+static int lan8814_get_sqi(struct phy_device *phydev) {
+     int rc, val;
+
+     val = lanphy_read_page_reg(phydev, 1, LAN8814_DCQ_CTRL);
+     if (val < 0)
+             return val;
+
+     val &= ~LAN8814_DCQ_CTRL_CHANNEL_MASK;

I do have a datasheet for this PHY, but it doesn't mention 0xe6 on
EP1.

This register values are present in GPHY hard macro as below

4.2.225       DCQ Control Register
Index (In Decimal):   EP 1.230        Size:   16 bits

Can you give me the name of the datasheet which you are following, so
that I'll check and let you know the reason.

I have the AN4286/DS00004286A ("LAN8804/LAN8814 GPHY Register
Definitions"). Maybe there is a newer version of it.

I just looked into it, it doesn't have SQI registers. I requested internal team to add SQI register set in published document.

quoted

quoted

So I can only guess that this "channel mask" is for the 4 rx/tx pairs
on GbE?

Yes channel mask is for wire pair.

quoted

And you only seem to evaluate one of them. Is that the correct thing
to do here?

I found in below link is that, get_SQI returns sqi value for 100
base-t1 phy's
https://lore.kernel.org/netdev/20200519075200.24631-2-

o.rempel@pengutronix.de/T/

That one is for the 100base-t1 which has only one pair.

quoted

In lan8814 phy only channel 0 is used for 100base-tx. So returning SQI
value for channel 0.

What if the other pairs are bad? Maybe Oleksij has an opinion here.

Also 100baseTX (and 10baseT) has two pairs, one for transmitting and one
for receiving. I guess you meassure the SQI on the receiving side. So is
channel 0 correct here?

Yes Channel 0 is correct.

Again this is the first time I hear about SQI but it puzzles me that
it only evaluate one pair in this case. So as a user who reads this
SQI might be misleaded.

Yeah, It needs uAPI extension.

-michael