Re: [PATCH net-next v13 12/15] net: stmmac: Fixed failure to set network speed to 1000.

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

On Tue, Jul 02, 2024 at 01:31:44PM +0300, Serge Semin wrote:
> On Tue, Jun 04, 2024 at 11:29:43AM +0000, si.yanteng@linux.dev wrote:
> > 2024年5月30日 15:22, "Russell King (Oracle)" <linux@armlinux.org.uk> 写到:
> > 
> > Hi, Russell, Serge,
> > 
> > > I would like this patch to be held off until more thought can be put
> > > 
> > > into how to handle this without having a hack in the driver (stmmac
> > > 
> > > has too many hacks and we're going to have to start saying no to
> > > 
> > > these.)
> > Yeah, you have a point there, but I would also like to hear Serge's opinion.
> 
> I would be really glad not to have so many hacks in the STMMAC driver.
> It would have been awesome if we are able to find a solution without
> introducing one more quirk in the common driver code.
> 
> I started digging into this sometime ago, but failed to come up with
> any decent hypothesis about the problem nature. One of the glimpse
> idea was that the loongson_gnet_fix_speed() method code might be somehow
> connected with the problem. But I didn't have much time to go further
> than that.
> 
> Anyway I guess we'll need to have at least two more patchset
> re-spins to settle all the found issues. Until then we can keep
> discussing the problem Yanteng experience on his device. Russell, do
> you have any suggestion of what info Yanteng should provide to better
> understand the problem and get closer to it' cause?

First question: is auto-negotiation required by 802.3 for 1000base-T?
By "required" I mean "required to be used" not "required to be
implemented". This is an important distinction, and 802.3 tends to be
a bit wooly about the exact meaning. However, I think on balance the
conclusion is that AN is mandatory _to be used_ for 1000base-T links.

Annex 40C's state diagrams seems to imply that mr_autoneg_enable
(BMCR AN ENABLE) doesn't affect whether or not the AN state machines
work for 1000base-T, and some PHY datasheets (e.g. Marvell Alaska)
state that disabling mr_autoneg_enable leaves AN enabled but forced
to 1G full duplex.

So, I'm thinking is that the ethtool interface should reject any
request where we have a PHY supporting *only* base-T for gigabit
speeds, where the user is requesting !AN && SPEED_1000 on the basis
that AN is part of the link setup of 1000base-T links.

Maybe this should be a property of the struct phy_device so we can
transition phylib and phylink to return an appropriate error to
userspace?

Alternatively, maybe just implement the Marvell Alaska solution
to this problem (if the user attempts to disable AN on a PHY
supporting only base-T at gigabit speeds, then we silently force
AN with SPEED_1000 and DUPLEX_FULL.

Andrew - seems to be an IEEE 802.3 requirement that's been missed
in phylib, any thoughts?

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