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

Hi,

On Fri, Aug 26, 2022 at 11:26:12AM +0200, Michael Walle wrote:

[+ 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,

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.

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/ (local)

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?

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.

Wow! I was so possessed with one-pair networks, that forgot to image
that there is 1000Base-T with more then one pairs :D

Yes, your are right. We wont to have readings from all RX channels and
be able to export them to the user space. In fact, if i see it
correctly, the LAN8814_DCQ_CTRL_CHANNEL_MASK value should be synced with
the MDI-X state. Otherwise we will be reading TX channels.

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 |