Re: [PATCH net-next v1 7/7] net: usb: lan78xx: Enable EEE support with phylink integration

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

On Sat, Jan 18, 2025 at 11:01:22AM +0100, Oleksij Rempel wrote:
> On Sat, Jan 18, 2025 at 09:04:07AM +0000, Russell King (Oracle) wrote:
> > On Sat, Jan 18, 2025 at 08:22:15AM +0100, Oleksij Rempel wrote:
> > > On Fri, Jan 17, 2025 at 04:23:52PM +0000, Russell King (Oracle) wrote:
> > > > I'm unsure about many DSA drivers. mt753x:
> > > > 
> > > >         u32 set, mask = LPI_THRESH_MASK | LPI_MODE_EN;
> > > > 
> > > >         if (e->tx_lpi_timer > 0xFFF)
> > > >                 return -EINVAL;
> > > > 
> > > >         set = LPI_THRESH_SET(e->tx_lpi_timer);
> > > >         if (!e->tx_lpi_enabled)
> > > >                 /* Force LPI Mode without a delay */
> > > >                 set |= LPI_MODE_EN;
> > > >         mt7530_rmw(priv, MT753X_PMEEECR_P(port), mask, set);
> > > > 
> > > > Why force LPI *without* a delay if tx_lpi_enabled is false? This
> > > > seems to go against the documented API:
> > > > 
> > > >  * @tx_lpi_enabled: Whether the interface should assert its tx lpi, given
> > > >  *      that eee was negotiated.
> > > 
> > > According to MT7531 manual, I would say, the code is not correct:
> > > https://repo.librerouter.org/misc/lr2/MT7531_switch_Reference_Manual_for_Development_Board.pdf
> > > 
> > > The LPI_MODE_EN_Px bit has following meaning:
> > > 
> > > When there is no packet to be transmitted, and the idle time is greater
> > > than P2_LPI_THRESHOLD, the TXMAC will automatically enter LPI (Low
> > > Power Idle) mode and send EEE LPI frame to the link partner.
> > > 0: LPI mode depends on the P2_LPI_THRESHOLD.
> > > 1: Let the system enter the LPI mode immediately and send EEE LPI frame
> > >    to the link partner.
> > 
> > Okay, so LPI_MODE_EN_Px causes it to disregard the LPI timer, and enter
> > LPI mode immediately. Thus, the code should never set LPI_MODE_EN_Px.
> > 
> > > This chip seems to not have support for tx_lpi_enabled != eee_enabled
> > > configuration.
> > 
> > Sorry, I don't see your reasoning there - and I think your
> > interpretation is different from the documentation (which is
> > the whole point of having a generic implementation in phylib
> > to avoid these kinds of different interpretation.)
> > 
> >  * @eee_enabled: EEE configured mode (enabled/disabled).
> >  * @tx_lpi_enabled: Whether the interface should assert its tx lpi, given
> >  *      that eee was negotiated.
> > 
> >            eee on|off
> >                   Enables/disables the device support of EEE.
> > 
> >            tx-lpi on|off
> >                   Determines whether the device should assert its Tx LPI.
> > 
> > The way phylib interprets eee_enabled is whether EEE is advertised
> > to the remote device or not. If EEE is not advertised, then EEE is
> > not negotiated, and thus EEE will not become active. If EEE is not
> > active, then LPI must not be asserted. tx_lpi_enabled defines whether,
> > given that EEE has been negotiated, whether LPI should be signalled
> > after the LPI timer has expired.
> > 
> > phylib deals with all this logic, and its all encoded into the
> > phydev->enable_tx_lpi flag to give consistency of implementation.
> > 
> > Thus, phydev->enable_tx_lpi is only true when eee_enabled && eee
> > negotiated at the specified speed && tx_lpi_enabled. In that state,
> > LPI is expected to be signalled after the LPI timer has expired.
> 
> I mean, the configuration where EEE can be enabled and in active state,
> but TX LPI is disabled: eee_enabled = true; eee_active = true;
> enable_tx_lpi = false. UAPI allows this configuration and it seems to

enable_tx_lpi is the result of phylib's management, and not a uAPI
thing. I think you mean the uAPI tx_lpi_enabled.

> work for 100Mbit/s. Atheros documentation call it asymmetric EEE
> operation - where each link partner enters LPI mode independently. In
> comparison, the same documentation calls 1000Mbit EEE mode, symmetric
> operation - where both link partner must enter the LPI mode
> simulatneously.

I'm not sure you are entirely correct.

FORCE_MODE_EEE100_P2
FORCE_MODE_EEE1G_P2

These bits seem to control whether the MT753x uses the result of polling
the PHY or the two force bits below to determine whether "EEE ability"
is determined.

FORCE_EEE1G_P2
FORCE_EEE100_P2

These bits determine whether, when their respective FORCE_MODE_EEE*_P2
bit is set, "EEE ability" is set or not.

"EEE ability" in this case would seem to basically be what we call
"EEE active" in kernel speak.

So, an implementation that would support our current uAPI fully would
be:

- Set FORCE_MODE_EEE*_P2 bits (thus making the "EEE ability" be
  under software control rather than the result of the PHY polling
  unit.)
- Set/clear FORCE_EEE*_P2 bits depending on phydev->enable_tx_lpi
- Set the timer according to phydev->eee_cfg.tx_lpi_timer

and that will support the user API in the way that its intended to be.

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