Re: [PATCH net v1 1/2] net: phy: set eee_cfg based on PHY configuration

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Thu, Nov 14, 2024 at 10:05:52AM +0000, Russell King (Oracle) wrote:
> On Thu, Nov 14, 2024 at 09:23:48AM +0000, Russell King (Oracle) wrote:
> > On Thu, Nov 14, 2024 at 04:16:52PM +0800, Choong Yong Liang wrote:
> > > Not all PHYs have EEE enabled by default. For example, Marvell PHYs are
> > > designed to have EEE hardware disabled during the initial state, and it
> > > needs to be configured to turn it on again.
> > > 
> > > This patch reads the PHY configuration and sets it as the initial value for
> > > eee_cfg.tx_lpi_enabled and eee_cfg.eee_enabled instead of having them set to
> > > true by default.
> > 
> > eee_cfg.tx_lpi_enabled is something phylib tracks, and it merely means
> > that LPI needs to be enabled at the MAC if EEE was negotiated:
> > 
> >  * @tx_lpi_enabled: Whether the interface should assert its tx lpi, given
> >  *      that eee was negotiated.
> > 
> > eee_cfg.eee_enabled means that EEE mode was enabled - which is user
> > configuration:
> > 
> >  * @eee_enabled: EEE configured mode (enabled/disabled).
> > 
> > phy_probe() reads the initial PHY state and sets things up
> > appropriately.
> > 
> > However, there is a point where the EEE configuration (advertisement,
> > and therefore eee_enabled state) is written to the PHY, and that should
> > be config_aneg(). Looking at the Marvell driver, it's calling
> > genphy_config_aneg() which eventually calls
> > genphy_c45_an_config_eee_aneg() which does this (via
> > __genphy_config_aneg()).
> > 
> > Please investigate why the hardware state is going out of sync with the
> > software state.
> 
> I think I've found the issue.
> 
> We have phydev->eee_enabled and phydev->eee_cfg.eee_enabled, which looks
> like a bug to me. We write to phydev->eee_cfg.eee_enabled in
> phy_support_eee(), leaving phydev->eee_enabled untouched.
> 
> However, most other places are using phydev->eee_enabled.
> 
> This is (a) confusing and (b) wrong, and having the two members leads
> to this confusion, and makes the code more difficult to follow (unless
> one has already clocked that there are these two different things both
> called eee_enabled).
> 
> This is my untested prototype patch to fix this - it may cause breakage
> elsewhere:

As mentioned in the other thread:

Without a call to phy_support_eee():

EEE settings for eth2:
        EEE status: disabled
        Tx LPI: disabled
        Supported EEE link modes:  100baseT/Full
                                   1000baseT/Full
        Advertised EEE link modes:  Not reported
        Link partner advertised EEE link modes:  100baseT/Full
                                                 1000baseT/Full

With a call to phy_support_eee():

EEE settings for eth2:
        EEE status: enabled - active
        Tx LPI: 0 (us)
        Supported EEE link modes:  100baseT/Full
                                   1000baseT/Full
        Advertised EEE link modes:  100baseT/Full
                                    1000baseT/Full
        Link partner advertised EEE link modes:  100baseT/Full
                                                 1000baseT/Full

So the EEE status is now behaving correctly, and the Marvell PHY is
being programmed with the advertisement correctly.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!