[PATCH net v1 1/2] net: phy: microchip: lan87xx: reinit PHY after cable test

[PATCH net v1 1/2] net: phy: microchip: lan87xx: reinit PHY after cable test

[Oleksij Rempel]

Reinit PHY after cable test, otherwise link can't be established on
tested port. This issue is reproducible on LAN9372 switches with
integrated 100BaseT1 PHYs.

Fixes: 788050256c411 ("net: phy: microchip_t1: add cable test support for lan87xx phy")
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/microchip_t1.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/microchip_t1.c b/drivers/net/phy/microchip_t1.c
index a838b61cd844b..a35528497a576 100644
--- a/drivers/net/phy/microchip_t1.c
+++ b/drivers/net/phy/microchip_t1.c
@@ -748,7 +748,7 @@ static int lan87xx_cable_test_report(struct phy_device *phydev)
 	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_A,
 				lan87xx_cable_test_report_trans(detect));
 
-	return 0;
+	return phy_init_hw(phydev);
 }
 
 static int lan87xx_cable_test_get_status(struct phy_device *phydev,
-- 
2.39.2

[PATCH net v1 2/2] net: phy: microchip: lan87xx: do not report SQI if no link

[Oleksij Rempel]

Do not report SQI if no link is detected. Otherwise ethtool will show
non zero value even if no cable is attached.

Fixes: b649695248b15 ("net: phy: LAN87xx: add ethtool SQI support")
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/microchip_t1.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/phy/microchip_t1.c b/drivers/net/phy/microchip_t1.c
index a35528497a576..22530a5b76365 100644
--- a/drivers/net/phy/microchip_t1.c
+++ b/drivers/net/phy/microchip_t1.c
@@ -840,6 +840,9 @@ static int lan87xx_get_sqi(struct phy_device *phydev)
 	u8 sqi_value = 0;
 	int rc;
 
+	if (!phydev->link)
+		return 0;
+
 	rc = access_ereg(phydev, PHYACC_ATTR_MODE_WRITE,
 			 PHYACC_ATTR_BANK_DSP, T1_COEF_RW_CTL_CFG, 0x0301);
 	if (rc < 0)
-- 
2.39.2

Re: [PATCH net v1 2/2] net: phy: microchip: lan87xx: do not report SQI if no link

[Michal Kubiak]

On Wed, Jul 03, 2024 at 03:28:01PM +0200, Oleksij Rempel wrote:
> Do not report SQI if no link is detected. Otherwise ethtool will show
> non zero value even if no cable is attached.
> 
> Fixes: b649695248b15 ("net: phy: LAN87xx: add ethtool SQI support")
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/phy/microchip_t1.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/phy/microchip_t1.c b/drivers/net/phy/microchip_t1.c
> index a35528497a576..22530a5b76365 100644
> --- a/drivers/net/phy/microchip_t1.c
> +++ b/drivers/net/phy/microchip_t1.c
> @@ -840,6 +840,9 @@ static int lan87xx_get_sqi(struct phy_device *phydev)
>  	u8 sqi_value = 0;
>  	int rc;
>  
> +	if (!phydev->link)
> +		return 0;
> +
>  	rc = access_ereg(phydev, PHYACC_ATTR_MODE_WRITE,
>  			 PHYACC_ATTR_BANK_DSP, T1_COEF_RW_CTL_CFG, 0x0301);
>  	if (rc < 0)
> -- 
> 2.39.2
> 
> 

Thanks,
Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>

Re: [PATCH net v1 2/2] net: phy: microchip: lan87xx: do not report SQI if no link

[Andrew Lunn]

On Wed, Jul 03, 2024 at 03:28:01PM +0200, Oleksij Rempel wrote:
> Do not report SQI if no link is detected. Otherwise ethtool will show
> non zero value even if no cable is attached.
> 
> Fixes: b649695248b15 ("net: phy: LAN87xx: add ethtool SQI support")
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/phy/microchip_t1.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/phy/microchip_t1.c b/drivers/net/phy/microchip_t1.c
> index a35528497a576..22530a5b76365 100644
> --- a/drivers/net/phy/microchip_t1.c
> +++ b/drivers/net/phy/microchip_t1.c
> @@ -840,6 +840,9 @@ static int lan87xx_get_sqi(struct phy_device *phydev)
>  	u8 sqi_value = 0;
>  	int rc;
>  
> +	if (!phydev->link)
> +		return 0;
> +

Is this the correct place to fix this? Can any PHY report an SQI value
if there is no link? Maybe an automotive PHY using T1 and good old
fashioned CSMA/CD could report about background noise? But do they?

Maybe this should be fixed in linkstate_get_sqi()?

Also, maybe it should return -ENETDOWN, not 0. Do we want to say
"worse than class A SQI (unstable link)" when in fact the link is
"class G SQI (very good link)" once it is up?

	Andrew

RE: [PATCH net v1 2/2] net: phy: microchip: lan87xx: do not report SQI if no link

[Woojung.Huh]

Hi Andrew & Oleksij,

> On Wed, Jul 03, 2024 at 03:28:01PM +0200, Oleksij Rempel wrote:
> > Do not report SQI if no link is detected. Otherwise ethtool will show
> > non zero value even if no cable is attached.
> >
> > Fixes: b649695248b15 ("net: phy: LAN87xx: add ethtool SQI support")
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >  drivers/net/phy/microchip_t1.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/net/phy/microchip_t1.c
> b/drivers/net/phy/microchip_t1.c
> > index a35528497a576..22530a5b76365 100644
> > --- a/drivers/net/phy/microchip_t1.c
> > +++ b/drivers/net/phy/microchip_t1.c
> > @@ -840,6 +840,9 @@ static int lan87xx_get_sqi(struct phy_device *phydev)
> >       u8 sqi_value = 0;
> >       int rc;
> >
> > +     if (!phydev->link)
> > +             return 0;
> > +
> 
> Is this the correct place to fix this? Can any PHY report an SQI value
> if there is no link? Maybe an automotive PHY using T1 and good old
> fashioned CSMA/CD could report about background noise? But do they?
> 
> Maybe this should be fixed in linkstate_get_sqi()?
> 
> Also, maybe it should return -ENETDOWN, not 0. Do we want to say
> "worse than class A SQI (unstable link)" when in fact the link is
> "class G SQI (very good link)" once it is up?

I lean to Andew's idea because "SQI values are only valid
if link-up condition is present" per OpenAlliance specification of 
100Base-T1 Interoperability Test suite. [1]

[1] https://opensig.org/automotive-ethernet-specifications/#

Thanks.
Woojung

Re: [PATCH net v1 1/2] net: phy: microchip: lan87xx: reinit PHY after cable test

[Michal Kubiak]

On Wed, Jul 03, 2024 at 03:28:00PM +0200, Oleksij Rempel wrote:
> Reinit PHY after cable test, otherwise link can't be established on
> tested port. This issue is reproducible on LAN9372 switches with
> integrated 100BaseT1 PHYs.
> 
> Fixes: 788050256c411 ("net: phy: microchip_t1: add cable test support for lan87xx phy")
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/phy/microchip_t1.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/microchip_t1.c b/drivers/net/phy/microchip_t1.c
> index a838b61cd844b..a35528497a576 100644
> --- a/drivers/net/phy/microchip_t1.c
> +++ b/drivers/net/phy/microchip_t1.c
> @@ -748,7 +748,7 @@ static int lan87xx_cable_test_report(struct phy_device *phydev)
>  	ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_A,
>  				lan87xx_cable_test_report_trans(detect));
>  
> -	return 0;
> +	return phy_init_hw(phydev);
>  }
>  
>  static int lan87xx_cable_test_get_status(struct phy_device *phydev,
> -- 
> 2.39.2
> 
>

Thanks,
Reviewed-by: Michal Kubiak <michal.kubiak@intel.com>

Re: [PATCH net v1 1/2] net: phy: microchip: lan87xx: reinit PHY after cable test

[Andrew Lunn]

On Wed, Jul 03, 2024 at 03:28:00PM +0200, Oleksij Rempel wrote:
> Reinit PHY after cable test, otherwise link can't be established on
> tested port. This issue is reproducible on LAN9372 switches with
> integrated 100BaseT1 PHYs.

Since this is a patchset, a cover letter would be normal.

> 
> Fixes: 788050256c411 ("net: phy: microchip_t1: add cable test support for lan87xx phy")
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net v1 1/2] net: phy: microchip: lan87xx: reinit PHY after cable test

[Florian Fainelli]

On 7/3/2024 2:28 PM, Oleksij Rempel wrote:
> Reinit PHY after cable test, otherwise link can't be established on
> tested port. This issue is reproducible on LAN9372 switches with
> integrated 100BaseT1 PHYs.
> 
> Fixes: 788050256c411 ("net: phy: microchip_t1: add cable test support for lan87xx phy")
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH net v1 1/2] net: phy: microchip: lan87xx: reinit PHY after cable test

[Florian Fainelli]

On 7/3/2024 2:28 PM, Oleksij Rempel wrote:
> Reinit PHY after cable test, otherwise link can't be established on
> tested port. This issue is reproducible on LAN9372 switches with
> integrated 100BaseT1 PHYs.
> 
> Fixes: 788050256c411 ("net: phy: microchip_t1: add cable test support for lan87xx phy")
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian