Re: [PATCH net-next v1 7/7] net: fec: add support for PHYs with SmartEEE support

[Larysa Zaremba]

On Tue, Feb 14, 2023 at 02:26:36PM +0100, Andrew Lunn wrote:
> On Tue, Feb 14, 2023 at 10:03:14AM +0100, Oleksij Rempel wrote:
> > Ethernet controller in i.MX6*/i.MX7* series do not provide EEE support.
> > But this chips are used sometimes in combinations with SmartEEE capable
> > PHYs.
> > So, instead of aborting get/set_eee access on MACs without EEE support,
> > ask PHY if it is able to do the EEE job by using SmartEEE.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
> > ---
> >  drivers/net/ethernet/freescale/fec_main.c | 22 ++++++++++++++++++----
> >  1 file changed, 18 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> > index c73e25f8995e..00f3703db69d 100644
> > --- a/drivers/net/ethernet/freescale/fec_main.c
> > +++ b/drivers/net/ethernet/freescale/fec_main.c
> > @@ -3102,8 +3102,15 @@ fec_enet_get_eee(struct net_device *ndev, struct ethtool_eee *edata)
> >  	struct fec_enet_private *fep = netdev_priv(ndev);
> >  	struct ethtool_eee *p = &fep->eee;
> >  
> > -	if (!(fep->quirks & FEC_QUIRK_HAS_EEE))
> > -		return -EOPNOTSUPP;
> > +	if (!(fep->quirks & FEC_QUIRK_HAS_EEE)) {
> > +		if (!netif_running(ndev))
> > +			return -ENETDOWN;
> > +
> > +		if (!phy_has_smarteee(ndev->phydev))
> > +			return -EOPNOTSUPP;
> > +
> > +		return phy_ethtool_get_eee(ndev->phydev, edata);
> > +	}
> 
> I can see two different ways we do this. As you have here, we modify
> every MAC driver which is paired to a SmartEEE PHY and have it call
> into phylib. Or we modify the ethtool core, if it gets -EOPNOTSUPP,
> and there is an ndev->phydev call directly into phylib. That should
> make all boards with SmartEEE supported. We do this for a few calls,
> TS Info, and SFP module info.
> 
> Either way, i don't think we need phy_has_smarteee() exposed outside
> of phylib. SmartEEE is supposed to be transparent to the MAC, so it
> should not need to care. Same as WOL, the MAC does not care if the PHY
> supports WoL, it should just call the APIs to get and set WoL and
> assume they do the right thing.
> 
> What is also unclear to me is how we negotiate between EEE and
> SmartEEE. I assume if the MAC is EEE capable, we prefer that over
> SmartEEE. But i don't think i've seen anything in these patches which
> addresses this. Maybe we want phy_init_eee() to disable SmartEEE?
> 
> 	  Andrew

I agree with the attitude that we shouldn't expect legacy MAC driver to handle 
fancy features of PHY, so modifying core code instead of driver code would be a 
much better solution.

Also, I too would like to see the case when both MAC EEE and SmartEEE are 
enabled, to be prevented or acknowledged, if it's actually useful for something.

Other than that, patchset look fine to me, so in v2, you can put my Reviewed-By 
(Larysa Zaremba <larysa.zaremba@xxxxxxxxx>) under any of the previous patches 
which stay unchanged (except the 4th one, you have to correct 'specifica'
in the title).