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

[Choong Yong Liang]

On 14/11/2024 6:16 pm, Russell King (Oracle) wrote:
> 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.

Thank you for all the suggestions, the provided prototype, and the tested 
results.

I will study the suggestions in depth, test the provided prototype, and 
provide more feedback.