[PATCH] net: phy: use generic clause 45 autonegotiation done

[PATCH] net: phy: use generic clause 45 autonegotiation done

[Camelia Groza]

Only Clause 22 PHYs can use genphy_aneg_done(). Use
genphy_c45_aneg_done() for PHYs that implement Clause 45 without
the Clause 22 register set.

This change follows the model of phy_restart_aneg() which
differentiates between the two implementations in a similar way.

Fixes: 41408ad519f7 ("net: phy: avoid genphy_aneg_done() for PHYs without clause 22 support")
Signed-off-by: Camelia Groza <camelia.groza@nxp.com>
---
 drivers/net/phy/phy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 537297d..4fcc703 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -151,7 +151,7 @@ int phy_aneg_done(struct phy_device *phydev)
 	 * implement Clause 22 registers
 	 */
 	if (phydev->is_c45 && !(phydev->c45_ids.devices_in_package & BIT(0)))
-		return -EINVAL;
+		return genphy_c45_aneg_done(phydev);
 
 	return genphy_aneg_done(phydev);
 }
-- 
1.9.1

Re: [PATCH] net: phy: use generic clause 45 autonegotiation done

[Andrew Lunn]

On Wed, Jul 18, 2018 at 04:12:15PM +0300, Camelia Groza wrote:
> Only Clause 22 PHYs can use genphy_aneg_done(). Use
> genphy_c45_aneg_done() for PHYs that implement Clause 45 without
> the Clause 22 register set.
> 
> This change follows the model of phy_restart_aneg() which
> differentiates between the two implementations in a similar way.

Hi Camelia

What about phy_config_aneg()? I would assume any sort of auto-neg
action needs to check for c45 without c22, before calling a genphy_
function. Do you think it is possible to write a
genphy_c45_config_aneg()? If not, we might want to return -EOPNOTSUPP.

	  Andrew

RE: [PATCH] net: phy: use generic clause 45 autonegotiation done

[Camelia Alexandra Groza]

> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: Wednesday, July 18, 2018 17:39
> To: Camelia Alexandra Groza <camelia.groza@nxp.com>
> Cc: f.fainelli@gmail.com; davem@davemloft.net; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] net: phy: use generic clause 45 autonegotiation done
> 
> On Wed, Jul 18, 2018 at 04:12:15PM +0300, Camelia Groza wrote:
> > Only Clause 22 PHYs can use genphy_aneg_done(). Use
> > genphy_c45_aneg_done() for PHYs that implement Clause 45 without the
> > Clause 22 register set.
> >
> > This change follows the model of phy_restart_aneg() which
> > differentiates between the two implementations in a similar way.
> 
> Hi Camelia
> 
> What about phy_config_aneg()? I would assume any sort of auto-neg action
> needs to check for c45 without c22, before calling a genphy_ function. Do you
> think it is possible to write a genphy_c45_config_aneg()? If not, we might
> want to return -EOPNOTSUPP.

Hi Andrew,

Adding Russell to the thread as well, since he wrote the c45 helpers.

Sure, I'll send a v2 with an additional generic phy_config_aneg(). I'll stick to returning -EOPNOTSUPP for c22-less PHYs for now.

Thanks,
Camelia

Re: [PATCH] net: phy: use generic clause 45 autonegotiation done

[Russell King - ARM Linux]

On Thu, Jul 19, 2018 at 12:47:18PM +0000, Camelia Alexandra Groza wrote:
> > -----Original Message-----
> > From: Andrew Lunn [mailto:andrew@lunn.ch]
> > Sent: Wednesday, July 18, 2018 17:39
> > To: Camelia Alexandra Groza <camelia.groza@nxp.com>
> > Cc: f.fainelli@gmail.com; davem@davemloft.net; netdev@vger.kernel.org;
> > linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH] net: phy: use generic clause 45 autonegotiation done
> > 
> > On Wed, Jul 18, 2018 at 04:12:15PM +0300, Camelia Groza wrote:
> > > Only Clause 22 PHYs can use genphy_aneg_done(). Use
> > > genphy_c45_aneg_done() for PHYs that implement Clause 45 without the
> > > Clause 22 register set.
> > >
> > > This change follows the model of phy_restart_aneg() which
> > > differentiates between the two implementations in a similar way.
> > 
> > Hi Camelia
> > 
> > What about phy_config_aneg()? I would assume any sort of auto-neg action
> > needs to check for c45 without c22, before calling a genphy_ function. Do you
> > think it is possible to write a genphy_c45_config_aneg()? If not, we might
> > want to return -EOPNOTSUPP.
> 
> Hi Andrew,
> 
> Adding Russell to the thread as well, since he wrote the c45 helpers.
> 
> Sure, I'll send a v2 with an additional generic phy_config_aneg(). I'll
> stick to returning -EOPNOTSUPP for c22-less PHYs for now.

Be aware that there are no generic registers for configuring (eg) 1G
speeds in C45 phys - that is vendor specific.

It may be that the expectation in the 802.3 specs is that such PHYs
implement the C22 register set in devad 0, but I've no visibility of
that (and the 10G PHYs that phylib does have, apart from marvell10g,
are particularly poor in the features they implement.)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up

Re: [PATCH] net: phy: use generic clause 45 autonegotiation done

[Andrew Lunn]

On Thu, Jul 19, 2018 at 01:56:35PM +0100, Russell King - ARM Linux wrote:
> On Thu, Jul 19, 2018 at 12:47:18PM +0000, Camelia Alexandra Groza wrote:
> > > -----Original Message-----
> > > From: Andrew Lunn [mailto:andrew@lunn.ch]
> > > Sent: Wednesday, July 18, 2018 17:39
> > > To: Camelia Alexandra Groza <camelia.groza@nxp.com>
> > > Cc: f.fainelli@gmail.com; davem@davemloft.net; netdev@vger.kernel.org;
> > > linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH] net: phy: use generic clause 45 autonegotiation done
> > > 
> > > On Wed, Jul 18, 2018 at 04:12:15PM +0300, Camelia Groza wrote:
> > > > Only Clause 22 PHYs can use genphy_aneg_done(). Use
> > > > genphy_c45_aneg_done() for PHYs that implement Clause 45 without the
> > > > Clause 22 register set.
> > > >
> > > > This change follows the model of phy_restart_aneg() which
> > > > differentiates between the two implementations in a similar way.
> > > 
> > > Hi Camelia
> > > 
> > > What about phy_config_aneg()? I would assume any sort of auto-neg action
> > > needs to check for c45 without c22, before calling a genphy_ function. Do you
> > > think it is possible to write a genphy_c45_config_aneg()? If not, we might
> > > want to return -EOPNOTSUPP.
> > 
> > Hi Andrew,
> > 
> > Adding Russell to the thread as well, since he wrote the c45 helpers.
> > 
> > Sure, I'll send a v2 with an additional generic phy_config_aneg(). I'll
> > stick to returning -EOPNOTSUPP for c22-less PHYs for now.
> 
> Be aware that there are no generic registers for configuring (eg) 1G
> speeds in C45 phys - that is vendor specific.

So returning -EOPNOTSUPP sounds like the right thing to do, making it
clear the PHY driver needs to implement it.

Thanks
      Andrew

RE: [PATCH] net: phy: use generic clause 45 autonegotiation done

[Camelia Alexandra Groza]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of Camelia Alexandra Groza
> Sent: Thursday, July 19, 2018 15:47
> To: Andrew Lunn <andrew@lunn.ch>
> Cc: rmk+kernel@armlinux.org.uk; f.fainelli@gmail.com;
> davem@davemloft.net; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: RE: [PATCH] net: phy: use generic clause 45 autonegotiation done
> 
> > -----Original Message-----
> > From: Andrew Lunn [mailto:andrew@lunn.ch]
> > Sent: Wednesday, July 18, 2018 17:39
> > To: Camelia Alexandra Groza <camelia.groza@nxp.com>
> > Cc: f.fainelli@gmail.com; davem@davemloft.net; netdev@vger.kernel.org;
> > linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH] net: phy: use generic clause 45 autonegotiation
> > done
> >
> > On Wed, Jul 18, 2018 at 04:12:15PM +0300, Camelia Groza wrote:
> > > Only Clause 22 PHYs can use genphy_aneg_done(). Use
> > > genphy_c45_aneg_done() for PHYs that implement Clause 45 without
> the
> > > Clause 22 register set.
> > >
> > > This change follows the model of phy_restart_aneg() which
> > > differentiates between the two implementations in a similar way.
> >
> > Hi Camelia
> >
> > What about phy_config_aneg()? I would assume any sort of auto-neg
> > action needs to check for c45 without c22, before calling a genphy_
> > function. Do you think it is possible to write a
> > genphy_c45_config_aneg()? If not, we might want to return -
> EOPNOTSUPP.
> 
> Hi Andrew,
> 
> Adding Russell to the thread as well, since he wrote the c45 helpers.
> 
> Sure, I'll send a v2 with an additional generic phy_config_aneg(). I'll stick to
> returning -EOPNOTSUPP for c22-less PHYs for now.

Since the phy_config_aneg() call isn't synced on the net tree yet, I sent the second patch independently on net-next [1]. Please review this patch separately if it's ok.

[1] https://patchwork.ozlabs.org/patch/947831/

Thank you,
Camelia

RE: [PATCH] net: phy: use generic clause 45 autonegotiation done

[Camelia Alexandra Groza]

> -----Original Message-----
> From: Camelia Groza [mailto:camelia.groza@nxp.com]
> Sent: Wednesday, July 18, 2018 16:12
> To: andrew@lunn.ch; f.fainelli@gmail.com; davem@davemloft.net
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Camelia
> Alexandra Groza <camelia.groza@nxp.com>
> Subject: [PATCH] net: phy: use generic clause 45 autonegotiation done
> 
> Only Clause 22 PHYs can use genphy_aneg_done(). Use
> genphy_c45_aneg_done() for PHYs that implement Clause 45 without the
> Clause 22 register set.
> 
> This change follows the model of phy_restart_aneg() which differentiates
> between the two implementations in a similar way.
> 
> Fixes: 41408ad519f7 ("net: phy: avoid genphy_aneg_done() for PHYs without
> clause 22 support")
> Signed-off-by: Camelia Groza <camelia.groza@nxp.com>
> ---
>  drivers/net/phy/phy.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index
> 537297d..4fcc703 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -151,7 +151,7 @@ int phy_aneg_done(struct phy_device *phydev)
>  	 * implement Clause 22 registers
>  	 */
>  	if (phydev->is_c45 && !(phydev->c45_ids.devices_in_package &
> BIT(0)))
> -		return -EINVAL;
> +		return genphy_c45_aneg_done(phydev);
> 
>  	return genphy_aneg_done(phydev);
>  }
> --
> 1.9.1

Hi

A reminder for the original patch above. Since I sent the second patch for phy_config_aneg() against a different tree [1], I didn't see the need to resubmit this patch. If you need me to resubmit it, please let me know.

[1] https://patchwork.ozlabs.org/patch/947831/

Thank you,
Camelia