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

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

[Divya Koppera]

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 <Divya.Koppera@microchip.com>
---
 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

[Andrew Lunn]

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

[Divya.Koppera]

Hi Andrew,

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Thursday, August 25, 2022 6:50 PM
> To: Divya Koppera - I30481 <Divya.Koppera@microchip.com>
> 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
> <UNGLinuxDriver@microchip.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
> 
> 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?

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

[Andrew Lunn]

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

[Divya.Koppera]

Hi,

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Friday, August 26, 2022 3:24 AM
> To: Divya Koppera - I30481 <Divya.Koppera@microchip.com>
> 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
> <UNGLinuxDriver@microchip.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
> 
> > +#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:
> 

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

[Andrew Lunn]

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

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

[Divya.Koppera]

Hi Andrew,

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Saturday, August 27, 2022 1:13 AM
> To: Divya Koppera - I30481 <Divya.Koppera@microchip.com>
> 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
> <UNGLinuxDriver@microchip.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
> 
> > > 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.
> 
> 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

[Michael Walle]

Hi,

> 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 <Divya.Koppera@microchip.com>
> ---
>  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?

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

[Divya.Koppera]

Hi Michael,

> -----Original Message-----
> From: Michael Walle <michael@walle.cc>
> Sent: Friday, August 26, 2022 2:13 PM
> To: Divya Koppera - I30481 <Divya.Koppera@microchip.com>
> 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
> <michael@walle.cc>
> 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,
> 
> > 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 <Divya.Koppera@microchip.com>
> > ---
> >  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?
> 

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

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

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

[Michael Walle]

[+ Oleksij Rempel]

Hi,

Am 2022-08-26 11:11, schrieb Divya.Koppera@microchip.com:
>> > 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 <Divya.Koppera@microchip.com>

..

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

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

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

[Oleksij Rempel]

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:
> > > > 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 <Divya.Koppera@microchip.com>
> 
> ..
> 
> > > > +#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.
> > 
> > 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.
> 
> > 
> > > 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/
> 
> 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.

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

[Oleksij Rempel]

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:
> > [+ Oleksij Rempel]
> > 
> > Hi,
> > 
> > Am 2022-08-26 11:11, schrieb Divya.Koppera@microchip.com:
> > > > > 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 <Divya.Koppera@microchip.com>
> > 
> > ..
> > 
> > > > > +#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.
> > > 
> > > 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.
> > 
> > > 
> > > > 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/
> > 
> > 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.
> 
> 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

[Andrew Lunn]

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

[Divya.Koppera]

Hi Michael,

> -----Original Message-----
> From: Michael Walle <michael@walle.cc>
> Sent: Friday, August 26, 2022 2:56 PM
> To: Divya Koppera - I30481 <Divya.Koppera@microchip.com>; Oleksij Rempel
> <o.rempel@pengutronix.de>
> 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:
> >> > 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 <Divya.Koppera@microchip.com>
> 
> ..
> 
> >> > +#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.
> >
> > 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.

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

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

[Andrew Lunn]

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

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