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

[PATCH v2 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. It is not supported for 10Mbps speed

Signed-off-by: Divya Koppera <Divya.Koppera@microchip.com>
---
v1 -> v2
- Given SQI support for all pairs of wires in 1000/100 base-T phy's
  uAPI may run through all instances in future. At present returning
  only first instance as uAPI supports for only 1 pair.
- SQI is not supported for 10Mbps speed, handled accordingly.
---
 drivers/net/phy/micrel.c | 44 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 7b8c5c8d013e..37845efe2cb6 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;
@@ -2933,6 +2940,41 @@ static int lan8814_probe(struct phy_device *phydev)
 	return 0;
 }
 
+static int lan8814_get_sqi(struct phy_device *phydev)
+{
+	int ret, val, pair;
+	int sqi_val[4];
+
+	if (phydev->speed == SPEED_10)
+		return -EOPNOTSUPP;
+
+	for (pair = 0; pair < 4; pair++) {
+		val = lanphy_read_page_reg(phydev, 1, LAN8814_DCQ_CTRL);
+		if (val < 0)
+			return val;
+
+		val &= ~LAN8814_DCQ_CTRL_CHANNEL_MASK;
+		val |= pair;
+		val |= LAN8814_DCQ_CTRL_READ_CAPTURE_;
+		ret = lanphy_write_page_reg(phydev, 1, LAN8814_DCQ_CTRL, val);
+		if (ret < 0)
+			return ret;
+
+		ret = lanphy_read_page_reg(phydev, 1, LAN8814_DCQ_SQI);
+		if (ret < 0)
+			return ret;
+
+		sqi_val[pair] = FIELD_GET(LAN8814_DCQ_SQI_VAL_MASK, ret);
+	}
+
+	return *sqi_val;
+}
+
+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,
@@ -3123,6 +3165,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 v2 net-next] net: phy: micrel: Adding SQI support for lan8814 phy

[Andrew Lunn]

On Mon, Sep 05, 2022 at 03:47:30PM +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. It is not supported for 10Mbps speed
> 
> Signed-off-by: Divya Koppera <Divya.Koppera@microchip.com>
> ---
> v1 -> v2
> - Given SQI support for all pairs of wires in 1000/100 base-T phy's
>   uAPI may run through all instances in future. At present returning
>   only first instance as uAPI supports for only 1 pair.
> - SQI is not supported for 10Mbps speed, handled accordingly.

I would prefer you solve the problem of returning all pairs.

I'm not sure how useful the current implementation is, especially at
100Mbps, where pair 0 could actually be the transmit pair. Does it
give a sensible value in that case?

> +static int lan8814_get_sqi(struct phy_device *phydev)
> +{
> +	int ret, val, pair;
> +	int sqi_val[4];
> +
> +	if (phydev->speed == SPEED_10)
> +		return -EOPNOTSUPP;
> +
> +	for (pair = 0; pair < 4; pair++) {
> +		val = lanphy_read_page_reg(phydev, 1, LAN8814_DCQ_CTRL);
> +		if (val < 0)
> +			return val;
> +
> +		val &= ~LAN8814_DCQ_CTRL_CHANNEL_MASK;
> +		val |= pair;
> +		val |= LAN8814_DCQ_CTRL_READ_CAPTURE_;
> +		ret = lanphy_write_page_reg(phydev, 1, LAN8814_DCQ_CTRL, val);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = lanphy_read_page_reg(phydev, 1, LAN8814_DCQ_SQI);
> +		if (ret < 0)
> +			return ret;
> +
> +		sqi_val[pair] = FIELD_GET(LAN8814_DCQ_SQI_VAL_MASK, ret);
> +	}
> +
> +	return *sqi_val;

How is this going to work in the future? sqi_val is on the stack. You
cannot return a pointer to it. So this function is going to need
modifications.

If you really want to prepare for a future implementation which could
return all four, i would probably make this a helper which takes a
pair number. And then have a function call it once for pair 0.

     Andrew

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

[Divya.Koppera]

Hi Andrew,

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Monday, September 5, 2022 6:40 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 v2 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 Mon, Sep 05, 2022 at 03:47:30PM +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. It is not supported
> > for 10Mbps speed
> >
> > Signed-off-by: Divya Koppera <Divya.Koppera@microchip.com>
> > ---
> > v1 -> v2
> > - Given SQI support for all pairs of wires in 1000/100 base-T phy's
> >   uAPI may run through all instances in future. At present returning
> >   only first instance as uAPI supports for only 1 pair.
> > - SQI is not supported for 10Mbps speed, handled accordingly.
> 
> I would prefer you solve the problem of returning all pairs.
> 

I will try to improve uAPI. 

The other way I can try is the point you mentioned below to write helper with pair number and having function cal with pair 0.


> I'm not sure how useful the current implementation is, especially at
> 100Mbps, where pair 0 could actually be the transmit pair. Does it give a
> sensible value in that case?
> 

We do have SQI support for 100Mbps to pair 0 only. For other pairs SQI values are invalid values.

> > +static int lan8814_get_sqi(struct phy_device *phydev) {
> > +     int ret, val, pair;
> > +     int sqi_val[4];
> > +
> > +     if (phydev->speed == SPEED_10)
> > +             return -EOPNOTSUPP;
> > +
> > +     for (pair = 0; pair < 4; pair++) {
> > +             val = lanphy_read_page_reg(phydev, 1, LAN8814_DCQ_CTRL);
> > +             if (val < 0)
> > +                     return val;
> > +
> > +             val &= ~LAN8814_DCQ_CTRL_CHANNEL_MASK;
> > +             val |= pair;
> > +             val |= LAN8814_DCQ_CTRL_READ_CAPTURE_;
> > +             ret = lanphy_write_page_reg(phydev, 1, LAN8814_DCQ_CTRL, val);
> > +             if (ret < 0)
> > +                     return ret;
> > +
> > +             ret = lanphy_read_page_reg(phydev, 1, LAN8814_DCQ_SQI);
> > +             if (ret < 0)
> > +                     return ret;
> > +
> > +             sqi_val[pair] = FIELD_GET(LAN8814_DCQ_SQI_VAL_MASK, ret);
> > +     }
> > +
> > +     return *sqi_val;
> 
> How is this going to work in the future? sqi_val is on the stack. You cannot
> return a pointer to it. So this function is going to need modifications.
> 
> If you really want to prepare for a future implementation which could return
> all four, i would probably make this a helper which takes a pair number. And
> then have a function call it once for pair 0.
> 

I Will go for resolving this if I couldn't resolve that 4 pair issue in uAPI.

>      Andrew

Thanks,
Divya

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

[Andrew Lunn]

> We do have SQI support for 100Mbps to pair 0 only. For other pairs
> SQI values are invalid values.

And you have tested this with auto-cross over, so that the pairs get
swapped?

	Andrew

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

[Michael Walle]

> On Mon, Sep 05, 2022 at 03:47:30PM +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. It is not supported for 10Mbps speed
>> 
>> Signed-off-by: Divya Koppera <Divya.Koppera@microchip.com>
>> ---
>> v1 -> v2
>> - Given SQI support for all pairs of wires in 1000/100 base-T phy's
>>   uAPI may run through all instances in future. At present returning
>>   only first instance as uAPI supports for only 1 pair.
>> - SQI is not supported for 10Mbps speed, handled accordingly.
>
> I would prefer you solve the problem of returning all pairs.
> 
> I'm not sure how useful the current implementation is, especially at
> 100Mbps, where pair 0 could actually be the transmit pair. Does it
> give a sensible value in that case?

It is good practice to CC the patches to the ones who gave feedback
on the previous versions. Not everyone is subscribed to all the
high traffic mailinglist.

Thanks,
-michael

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

[Divya.Koppera]

Hi Michael,

> -----Original Message-----
> From: Michael Walle <michael@walle.cc>
> Sent: Wednesday, September 7, 2022 2:38 PM
> To: Divya Koppera - I30481 <Divya.Koppera@microchip.com>
> Cc: andrew@lunn.ch; UNGLinuxDriver <UNGLinuxDriver@microchip.com>;
> 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; Oleksij Rempel
> <o.rempel@pengutronix.de>; Michael Walle <michael@walle.cc>
> Subject: Re: [PATCH v2 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 Mon, Sep 05, 2022 at 03:47:30PM +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. It is not
> >> supported for 10Mbps speed
> >>
> >> Signed-off-by: Divya Koppera <Divya.Koppera@microchip.com>
> >> ---
> >> v1 -> v2
> >> - Given SQI support for all pairs of wires in 1000/100 base-T phy's
> >>   uAPI may run through all instances in future. At present returning
> >>   only first instance as uAPI supports for only 1 pair.
> >> - SQI is not supported for 10Mbps speed, handled accordingly.
> >
> > I would prefer you solve the problem of returning all pairs.
> >
> > I'm not sure how useful the current implementation is, especially at
> > 100Mbps, where pair 0 could actually be the transmit pair. Does it
> > give a sensible value in that case?
> 
> It is good practice to CC the patches to the ones who gave feedback on the
> previous versions. Not everyone is subscribed to all the high traffic
> mailinglist.
> 

Sorry I missed adding you. 

> Thanks,
> -michael

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

[Oleksij Rempel]

On Tue, Sep 06, 2022 at 04:02:19PM +0200, Andrew Lunn wrote:
> > We do have SQI support for 100Mbps to pair 0 only. For other pairs
> > SQI values are invalid values.
> 
> And you have tested this with auto-cross over, so that the pairs get
> swapped?

auto-cross is probably the default option. You'll need to force MDI or
MDI-X mode.

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

[Divya.Koppera]

Hi,

> -----Original Message-----
> From: Oleksij Rempel <o.rempel@pengutronix.de>
> Sent: Wednesday, September 7, 2022 3:00 PM
> To: Andrew Lunn <andrew@lunn.ch>
> Cc: Divya Koppera - I30481 <Divya.Koppera@microchip.com>;
> 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 v2 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 Tue, Sep 06, 2022 at 04:02:19PM +0200, Andrew Lunn wrote:
> > > We do have SQI support for 100Mbps to pair 0 only. For other pairs
> > > SQI values are invalid values.
> >
> > And you have tested this with auto-cross over, so that the pairs get
> > swapped?
> 
> auto-cross is probably the default option. You'll need to force MDI or MDI-X
> mode.
> 

Yes, its default option. Sure will test in this way. Also I'll discuss with internal team regarding 1 pair access for 100Base-tx. May be its wrong and it needs to be 2 pairs.

Thanks
Divya

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