Re: [PATCH net-next] net: phy: microchip_t1: Adds support for LAN887x phy

[Andrew Lunn <andrew@lunn.ch>]

> > On Thu, Aug 08, 2024 at 08:29:16PM +0530, Divya Koppera wrote:
> > > +static int lan887x_config_init(struct phy_device *phydev) {
> > > +     /* Disable pause frames */
> > > +     linkmode_clear_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev-
> > >supported);
> > > +     /* Disable asym pause */
> > > +     linkmode_clear_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> > > +phydev->supported);
> > 
> > Why is this here? Pause frames are just like normal ethernet frames, they only
> > have meaning to the MAC, not to the PHY.
> > 
> > In any case, by the time the config_init() method has been called, the higher
> > levels have already looked at phydev->supported and made decisions on
> > what's there.
> > 
> 
> We tried to disable this in get_features.
> These are set again in phy_probe API.
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/phy/phy_device.c#n3544
> 
> We will re-look into these settings while submitting auto-negotiation patch in future series.

Let me see if i understand this correctly. You don't have autoneg at
the moment. Hence you cannot negotiate pause. PHYLIB is setting pause
is supported by default. Ethtool then probably suggests pause is
supported, if the MAC you are using is not masking it out.

Since pause frames are just regular frames, the PHY should just be
passing them through. So you should be able to forced pause, rather
than autoneg pause:

ethtool --pause eth42 autoneg off] rx on tx on

assuming the MAC supports pause.

Does this still work if you clear the PUASE bits from supported as you
are doing? Ideally we want to offer force paused configuration if the
MAC supports it.

> > > +static int lan887x_config_aneg(struct phy_device *phydev) {
> > > +     int ret;
> > > +
> > > +     /* First patch only supports 100Mbps and 1000Mbps force-mode.
> > > +      * T1 Auto-Negotiation (Clause 98 of IEEE 802.3) will be added later.
> > > +      */
> > > +     if (phydev->autoneg != AUTONEG_DISABLE) {
> > > +             /* PHY state is inconsistent due to ANEG Enable set
> > > +              * so we need to assign ANEG Disable for consistent behavior
> > > +              */
> > > +             phydev->autoneg = AUTONEG_DISABLE;
> > 
> > If you clear phydev->supported's autoneg bit, then phylib ought to enforce
> > this for you. Please check this rather than adding code to drivers.
> 
> Phylib is checking if advertisement is empty or not, but the feature is not verified against supported parameter.
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/phy/phy.c#n1092
> 
> But in the following statement phylib is updating advertising parameter.
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/drivers/net/phy/phy.c#n1113
> 
> This is making the feature enabled in driver, the right thing is to fix the library.
> We will fix the phylib in next series.

I'm not too surprised you are hitting such issues. Not actually
supporting autoneg is pretty uncommon, and is not well tested. Thanks
for offering to fix this up.

    Andrew