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

Subsystems: ethernet phy library, networking drivers, the rest

STALE1448d

15 messages, 4 authors, 2022-08-29 · open the first message on its own page

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

From: Divya Koppera <hidden>
Date: 2022-08-25 08:06:24

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>
---
 drivers/net/phy/micrel.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index e78d0bf69bc3..3775da7afc64 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c

@@ -1975,6 +1975,13 @@ static int ksz886x_cable_test_get_status(struct phy_device *phydev,
 #define LAN8814_CLOCK_MANAGEMENT			0xd
 #define LAN8814_LINK_QUALITY				0x8e
 
+#define LAN8814_DCQ_CTRL				0xe6
+#define LAN8814_DCQ_CTRL_READ_CAPTURE_			BIT(15)
+#define LAN8814_DCQ_CTRL_CHANNEL_MASK			GENMASK(1, 0)
+#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)
 	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;
+	val |= LAN8814_DCQ_CTRL_READ_CAPTURE_;
+	rc = lanphy_write_page_reg(phydev, 1, LAN8814_DCQ_CTRL, val);
+	if (rc < 0)
+		return rc;
+
+	rc = lanphy_read_page_reg(phydev, 1, LAN8814_DCQ_SQI);
+	if (rc < 0)
+		return rc;
+
+	return FIELD_GET(LAN8814_DCQ_SQI_VAL_MASK, rc);
+}
+
+static int lan8814_get_sqi_max(struct phy_device *phydev)
+{
+	return LAN8814_DCQ_SQI_MAX;
+}
+
 static struct phy_driver ksphy_driver[] = {
 {
 	.phy_id		= PHY_ID_KS8737,

@@ -3117,6 +3150,8 @@ static struct phy_driver ksphy_driver[] = {
 	.resume		= kszphy_resume,
 	.config_intr	= lan8814_config_intr,
 	.handle_interrupt = lan8814_handle_interrupt,
+	.get_sqi	= lan8814_get_sqi,
+	.get_sqi_max	= lan8814_get_sqi_max,
 }, {
 	.phy_id		= PHY_ID_LAN8804,
 	.phy_id_mask	= MICREL_PHY_ID_MASK,

-- 
2.17.1

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

From: Andrew Lunn <andrew@lunn.ch>
Date: 2022-08-25 13:20:07

On Thu, Aug 25, 2022 at 01:35:49PM +0530, Divya Koppera wrote:

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.

This driver supports 16 PHY devices. You only add this to one. Are you
sure it does not work with others?

     Andrew

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

From: Andrew Lunn <andrew@lunn.ch>
Date: 2022-08-25 21:54:00

quoted hunk

+#define LAN8814_DCQ_CTRL				0xe6
+#define LAN8814_DCQ_CTRL_READ_CAPTURE_			BIT(15)
+#define LAN8814_DCQ_CTRL_CHANNEL_MASK			GENMASK(1, 0)
+#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)
 	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;

I just took a quick look at the datasheet. It says:

All registers references in this section are in MMD Device Address 1

So you should be using phy_read_mmd(phydev, MDIO_MMD_PMAPMD, xxx) to
read/write these registers. The datasheet i have however is missing
the register map, so i've no idea if it is still 0xe6.

    Andrew

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

From: <hidden>
Date: 2022-08-26 03:47:10

Hi Andrew,

-----Original Message-----
From: Andrew Lunn <andrew@lunn.ch>
Sent: Thursday, August 25, 2022 6:50 PM
To: Divya Koppera - I30481 <redacted>
Cc: hkallweit1@gmail.com; linux@armlinux.org.uk; davem@davemloft.net;
edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
netdev@vger.kernel.org; linux-kernel@vger.kernel.org; UNGLinuxDriver
[off-list ref]
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

On Thu, Aug 25, 2022 at 01:35:49PM +0530, Divya Koppera wrote:

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.

This driver supports 16 PHY devices. You only add this to one. Are you sure it
does not work with others?

I don't have other hardware to test or check. I have only lan8814, where I have tested.
Also the register access or register address may not be same for other phy's. The one who has ownership
May provide support for other phy's.

     Andrew

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

From: <hidden>
Date: 2022-08-26 03:50:56

Hi,

-----Original Message-----
From: Andrew Lunn <andrew@lunn.ch>
Sent: Friday, August 26, 2022 3:24 AM
To: Divya Koppera - I30481 <redacted>
Cc: hkallweit1@gmail.com; linux@armlinux.org.uk; davem@davemloft.net;
edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
netdev@vger.kernel.org; linux-kernel@vger.kernel.org; UNGLinuxDriver
[off-list ref]
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

quoted

+#define LAN8814_DCQ_CTRL                             0xe6
+#define LAN8814_DCQ_CTRL_READ_CAPTURE_                       BIT(15)
+#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;

I just took a quick look at the datasheet. It says:

I'm not sure the datasheet you looked into is the right one. Could you please crosscheck if its lan8814 or lan8841.
Lan8814 is quad port phy where register access are of extended page. Lan8841 is 1 port phy where register access are mmd access.

All registers references in this section are in MMD Device Address 1

So you should be using phy_read_mmd(phydev, MDIO_MMD_PMAPMD,
xxx) to read/write these registers. The datasheet i have however is missing
the register map, so i've no idea if it is still 0xe6.

I hope above explanation gives answer.

    Andrew

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

From: Michael Walle <hidden>
Date: 2022-08-26 08:43:33

Hi,

quoted hunk

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>
---
 drivers/net/phy/micrel.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index e78d0bf69bc3..3775da7afc64 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c

@@ -1975,6 +1975,13 @@ static int ksz886x_cable_test_get_status(struct phy_device *phydev,
 #define LAN8814_CLOCK_MANAGEMENT			0xd
 #define LAN8814_LINK_QUALITY				0x8e
 
+#define LAN8814_DCQ_CTRL				0xe6
+#define LAN8814_DCQ_CTRL_READ_CAPTURE_			BIT(15)

Why does it end with an underscore?

quoted hunk

+#define LAN8814_DCQ_CTRL_CHANNEL_MASK			GENMASK(1, 0)
+#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)
 	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.
So I can only guess that this "channel mask" is for the 4 rx/tx pairs
on GbE? And you only seem to evaluate one of them. Is that the correct
thing to do here?

-michael

+	val |= LAN8814_DCQ_CTRL_READ_CAPTURE_;
+	rc = lanphy_write_page_reg(phydev, 1, LAN8814_DCQ_CTRL, val);
+	if (rc < 0)
+		return rc;
+
+	rc = lanphy_read_page_reg(phydev, 1, LAN8814_DCQ_SQI);
+	if (rc < 0)
+		return rc;
+
+	return FIELD_GET(LAN8814_DCQ_SQI_VAL_MASK, rc);
+}

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

From: <hidden>
Date: 2022-08-26 09:11:52

Hi Michael,

-----Original Message-----
From: Michael Walle <redacted>
Sent: Friday, August 26, 2022 2:13 PM
To: Divya Koppera - I30481 <redacted>
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; Michael Walle
[off-list ref]
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

Hi,

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>
---
 drivers/net/phy/micrel.c | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index

e78d0bf69bc3..3775da7afc64 100644

--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c

@@ -1975,6 +1975,13 @@ static int ksz886x_cable_test_get_status(struct

phy_device *phydev,

quoted

 #define LAN8814_CLOCK_MANAGEMENT                     0xd
 #define LAN8814_LINK_QUALITY                         0x8e

+#define LAN8814_DCQ_CTRL                             0xe6
+#define LAN8814_DCQ_CTRL_READ_CAPTURE_                       BIT(15)

Why does it end with an underscore?

All LAN8814 Macros that carries bit numbers are ending with _ in this driver. So following same.

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.

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.

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/

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

-michael

quoted

+     val |= LAN8814_DCQ_CTRL_READ_CAPTURE_;
+     rc = lanphy_write_page_reg(phydev, 1, LAN8814_DCQ_CTRL, val);
+     if (rc < 0)
+             return rc;
+
+     rc = lanphy_read_page_reg(phydev, 1, LAN8814_DCQ_SQI);
+     if (rc < 0)
+             return rc;
+
+     return FIELD_GET(LAN8814_DCQ_SQI_VAL_MASK, rc); }

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

From: Michael Walle <hidden>
Date: 2022-08-26 09:27:13

[+ Oleksij Rempel]

Hi,

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

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

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

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.

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.

-michael

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

From: Oleksij Rempel <o.rempel@pengutronix.de>
Date: 2022-08-26 09:54:53

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

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

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

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?

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 |

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

From: Oleksij Rempel <o.rempel@pengutronix.de>
Date: 2022-08-26 10:43:56

On Fri, Aug 26, 2022 at 11:54:29AM +0200, Oleksij Rempel wrote:

Hi,

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

quoted

[+ Oleksij Rempel]

Hi,

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

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

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

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?

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.

Just an idea not really related to this patch. It will be coll to be
able to generate diagnostic graphs like this:

Interface | pairs | data      | power | SQI | cable | selft
                  | direction |       |     | test  | test
eth0                                                  ok
            0     | RX        | NC    | 7/7 | ok    |
	    1     | TX        | NC    | NA  | ok    |
	    2     | NC        | + 10W | NA  | ok    |
	    3     | NC        | -     | NA  | ok    |

-- 
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] net: phy: micrel: Adding SQI support for lan8814 phy

From: Andrew Lunn <andrew@lunn.ch>
Date: 2022-08-26 19:42:55

quoted

I just took a quick look at the datasheet. It says:

I'm not sure the datasheet you looked into is the right one. Could you please crosscheck if its lan8814 or lan8841.
Lan8814 is quad port phy where register access are of extended page. Lan8841 is 1 port phy where register access are mmd access.

quoted

All registers references in this section are in MMD Device Address 1

So you should be using phy_read_mmd(phydev, MDIO_MMD_PMAPMD,
xxx) to read/write these registers. The datasheet i have however is missing
the register map, so i've no idea if it is still 0xe6.

https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/DS-LAN8814-00003592C.pdf

5.13.4 OPEN ALLIANCE TC1/TC12 DCQ SIGNAL QUALITY INDEX

Note: All registers references in this section are in MMD Device Address 1.

This section defines the implementation of section 6.1.2 of the TC1
and TC12 specifications. This mode builds upon the OPEN Alliance
TC1/TC12 DCQ Mean Square Error method by mapping the MSE value onto a
simple quality index. This mode is enabled by setting the sqi_enable
bit, in the DCQ Configuration register.

The MSE value is compared to the thresholds set in the DCQ SQI Table
Registers to provide an SQI value between 0 (worst value) and 7 (best
value) as follows:

In order to capture the SQI value, the DCQ Read Capture bit in the DCQ
Configuration register needs to be written as a high with the desired
cable pair specified in the DCQ Channel Number field of the same
register. The DCQ Read Capture bit will immediately self-clear and the
result will be available in the DCQ SQI register. In addition to the
current SQI, the worst case (lowest) SQI since the last read is
available in the SQI Worst Case field. The correlation between the
SQI values stored in the DCQ SQI register and an according Signal to
Noise Ratio (SNR) based on Additive White Gaussian (AWG) noise
(bandwidth of 80 MHz @ 100 Mbps / 550 MHz @ 1000 Mbps) is shown in
Table 5-5. The bit error rates to be expected in the case of white
noise as interference signal is shown in the table as well for
information purposes.

I had a quick look at OPEN ALLIANCE specification. It seems to specify
how each of these registers should look. It just failed to specify
where in the address map they are. So if you look at drivers
implementing SQI, you see most poke around in MDIO_MMD_VEND1. I
wounder if we can actually share the implementation between drivers,
those that follow the standard, with some paramatirisation where the
registers are.

Andrew

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

From: Andrew Lunn <andrew@lunn.ch>
Date: 2022-08-26 19:51:47

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.

I don't know if i should trust the datasheet i found, but it suggests
the register in MMD device 1 space has a field to indicate which cable
pair should be measured. So for 1000Base-T all pairs are both Rx and
Tx, so i would expect 4 values are returned. That then might need an
uAPI extension, if you were focused on T1 :-)

    Andrew

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

From: <hidden>
Date: 2022-08-29 05:23:23

Hi Andrew,

-----Original Message-----
From: Andrew Lunn <andrew@lunn.ch>
Sent: Saturday, August 27, 2022 1:13 AM
To: Divya Koppera - I30481 <redacted>
Cc: hkallweit1@gmail.com; linux@armlinux.org.uk; davem@davemloft.net;
edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
netdev@vger.kernel.org; linux-kernel@vger.kernel.org; UNGLinuxDriver
[off-list ref]
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

quoted

I just took a quick look at the datasheet. It says:

I'm not sure the datasheet you looked into is the right one. Could you

please crosscheck if its lan8814 or lan8841.

quoted

Lan8814 is quad port phy where register access are of extended page.

Lan8841 is 1 port phy where register access are mmd access.

quoted

All registers references in this section are in MMD Device Address 1

So you should be using phy_read_mmd(phydev,

MDIO_MMD_PMAPMD,

quoted

xxx) to read/write these registers. The datasheet i have however is
missing the register map, so i've no idea if it is still 0xe6.

https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/
ProductDocuments/DataSheets/DS-LAN8814-00003592C.pdf

5.13.4 OPEN ALLIANCE TC1/TC12 DCQ SIGNAL QUALITY INDEX

Note: All registers references in this section are in MMD Device Address 1.

Ah.. I too looked into it. Its a mistake. I suggested for correction of document internally. Also requested team to add 
SQI register set in register document that is published.

This section defines the implementation of section 6.1.2 of the TC1 and TC12
specifications. This mode builds upon the OPEN Alliance
TC1/TC12 DCQ Mean Square Error method by mapping the MSE value onto a
simple quality index. This mode is enabled by setting the sqi_enable bit, in
the DCQ Configuration register.

The MSE value is compared to the thresholds set in the DCQ SQI Table
Registers to provide an SQI value between 0 (worst value) and 7 (best
value) as follows:

In order to capture the SQI value, the DCQ Read Capture bit in the DCQ
Configuration register needs to be written as a high with the desired cable
pair specified in the DCQ Channel Number field of the same register. The
DCQ Read Capture bit will immediately self-clear and the result will be
available in the DCQ SQI register.  In addition to the current SQI, the worst
case (lowest) SQI since the last read is available in the SQI Worst Case field.
The correlation between the SQI values stored in the DCQ SQI register and an
according Signal to Noise Ratio (SNR) based on Additive White Gaussian
(AWG) noise (bandwidth of 80 MHz @ 100 Mbps / 550 MHz @ 1000 Mbps) is
shown in Table 5-5. The bit error rates to be expected in the case of white
noise as interference signal is shown in the table as well for information
purposes.

I had a quick look at OPEN ALLIANCE specification. It seems to specify how
each of these registers should look. It just failed to specify where in the
address map they are. So if you look at drivers implementing SQI, you see
most poke around in MDIO_MMD_VEND1.  I wounder if we can actually
share the implementation between drivers, those that follow the standard,
with some paramatirisation where the registers are.

I don't think it will work, each phy may have different register set and may or may not supports SQI.
Also it contains drivers of too old and that may not support SQI.

Apart from this lan8814 is quad port phy and register set is completely different from other drivers.

          Andrew

Thanks,
Divya

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

From: <hidden>
Date: 2022-08-29 05:50:57

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

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

+#define LAN8814_DCQ_CTRL_CHANNEL_MASK

GENMASK(1,

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

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

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

From: Andrew Lunn <andrew@lunn.ch>
Date: 2022-08-29 12:46:22

Yes Channel 0 is correct.

quoted

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.

I think the current uAPI actually allows it, sort of. You can have
multiple instances of a netlink property in a netlink message.  So
simply add 2 or 4 ETHTOOL_A_LINKSTATE_SQI properties. The existing
user space tools will likely just print the first value it
finds. Newer versions can walk the messages and print them all.

The alternative is to add a new nest, like i did for cable test
results:

 +-+-------------------------------------------+--------+---------------------+
 | | ``ETHTOOL_A_CABLE_NEST_RESULT``           | nested | cable test result   |
 +-+-+-----------------------------------------+--------+---------------------+
 | | | ``ETHTOOL_A_CABLE_RESULTS_PAIR``        | u8     | pair number         |
 +-+-+-----------------------------------------+--------+---------------------+
 | | | ``ETHTOOL_A_CABLE_RESULTS_CODE``        | u8     | result code         |
 +-+-+-----------------------------------------+--------+---------------------+

You can then explicitly indicate which cable pair the SQI value
corresponds to. In order to keep backwards compatibility, you would
still need to provide ETHTOOL_A_LINKSTATE_SQI, and then additionally
have these nests.

     Andrew

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help