Re: [PATCH net-next v3 5/5] net: phy: add driver for built-in 2.5G ethernet PHY on MT7988

[Andrew Lunn <andrew@lunn.ch>]

>     That is to say, for safety, we need to load firmware again right
> atfer booting into Linux kernel. Actually, this takes just about 11ms.

Since this is only 11ms, its not a big deal. If this was going over
MDIO it would be much slower and then it does become significant.

> > > +/* Setup LED */
> > > +phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, MTK_PHY_LED0_ON_CTRL,
> > > + MTK_PHY_LED_ON_POLARITY | MTK_PHY_LED_ON_LINK10 |
> > > + MTK_PHY_LED_ON_LINK100 | MTK_PHY_LED_ON_LINK1000 |
> > > + MTK_PHY_LED_ON_LINK2500);
> > > +phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, MTK_PHY_LED1_ON_CTRL,
> > > + MTK_PHY_LED_ON_FDX | MTK_PHY_LED_ON_HDX);
> > > +
> > > +pinctrl = devm_pinctrl_get_select(&phydev->mdio.dev, "i2p5gbe-
> > led");
> > 
> > Calls to devm_pinctrl_get_select() is pretty unusual in drivers:
> > 
> > 
> https://elixir.bootlin.com/linux/latest/C/ident/devm_pinctrl_get_select
> > 
> > Why is this needed? Generally, the DT file should describe the needed
> > pinmux setting, without needed anything additionally.
> > 
> This is needed because we need to switch to i2p5gbe-led pinmux group
> after we set correct polarity. Or LED may blink unexpectedly.

Since this is unusual, you should add a comment. Also, does the device
tree binding explain this? I expect most DT authors are used to
listing all the needed pins in the default pinmux node, and so will do
that, unless there is a comment in the binding advising against it.

> > It is a bit late doing this now. The user requested this a long time
> > ago, and it will be hard to understand why it now returns EOPNOTSUPP.
> > You should check for AUTONEG_DISABLE in config_aneg() and return the
> > error there.
> > 
> >       Andrew
> Maybe I shouldn't return EOPNOTSUPP in config_aneg directly?
> In this way, _phy_state_machine will be broken if I trigger "$ ethtool
> -s ethtool eth1 autoneg off"
> 
> [  520.781368] ------------[ cut here ]------------
> root@OpenWrt:/# [  520.785978] _phy_start_aneg+0x0/0xa4: returned: -95
> [  520.792263] WARNING: CPU: 3 PID: 423 at drivers/net/phy/phy.c:1254 _phy_state_machine+0x78/0x258
> [  520.801039] Modules linked in:
> [  520.804087] CPU: 3 PID: 423 Comm: kworker/u16:4 Tainted: G        W          6.8.0-rc7-next-20240306-bpi-r3+ #102
> [  520.814335] Hardware name: MediaTek MT7988A Reference Board (DT)
> [  520.820330] Workqueue: events_power_efficient phy_state_machine
> [  520.826240] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [  520.833190] pc : _phy_state_machine+0x78/0x258
> [  520.837623] lr : _phy_state_machine+0x78/0x258
> [  520.842056] sp : ffff800084b7bd30
> [  520.845360] x29: ffff800084b7bd30 x28: 0000000000000000 x27: 0000000000000000
> [  520.852487] x26: ffff000000c56900 x25: ffff000000c56980 x24: ffff000000012ac0
> [  520.859613] x23: ffff00000001d005 x22: ffff000000fdf000 x21: 0000000000000001
> [  520.866738] x20: 0000000000000004 x19: ffff000003a90000 x18: ffffffffffffffff
> [  520.873864] x17: 0000000000000000 x16: 0000000000000000 x15: ffff800104b7b977
> [  520.880988] x14: 0000000000000000 x13: 0000000000000183 x12: 00000000ffffffea
> [  520.888114] x11: 0000000000000001 x10: 0000000000000001 x9 : ffff8000837222f0
> [  520.895239] x8 : 0000000000017fe8 x7 : c0000000ffffefff x6 : 0000000000000001
> [  520.902365] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000000
> [  520.909490] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff000004120000
> [  520.916615] Call trace:
> [  520.919052]  _phy_state_machine+0x78/0x258
> [  520.923139]  phy_state_machine+0x2c/0x80
> [  520.927051]  process_one_work+0x178/0x31c
> [  520.931054]  worker_thread+0x280/0x494
> [  520.934795]  kthread+0xe4/0xe8
> [  520.937841]  ret_from_fork+0x10/0x20
> [  520.941408] ---[ end trace 0000000000000000 ]---
> 
> Now I prefer to give a warning like this, in
> mt798x_2p5ge_phy_config_aneg():
> if (phydev->autoneg == AUTONEG_DISABLE) {
> 	dev_warn(&phydev->mdio.dev, "Once AN off is set, this phy can't
> link.\n");
> 	return genphy_c45_pma_setup_forced(phydev);
> }

That is ugly.

Ideally we should fix phylib to support a PHY which cannot do fixed
link. I suggest you first look at phy_ethtool_ksettings_set() and see
what it does if passed cmd->base.autoneg == True, but
ETHTOOL_LINK_MODE_Autoneg_BIT is not set in supported, because the PHY
does not support autoneg. Is that handled? Does it return EOPNOTSUPP?
Understanding this might help you implement the opposite.

The opposite is however not easy. There is no linkmode bit indicating
a PHY can do forced settings. The BMSR has a bit indicating the PHY is
capable of auto-neg, which is used to set
ETHTOOL_LINK_MODE_Autoneg_BIT. However there is no bit to indicate the
PHY supports forced configuration. The standard just assumes all PHYs
which are standard conforming can do forced settings. And i think this
is the first PHY we have come across which is broken like this.

So maybe we cannot fix this in phylib. In that case, the MAC drivers
ksetting_set() should check if the user is attempting to disable
autoneg, and return -EOPNOTSUPP. And i would keep the stack trace
above, which will help developers understand they need the MAC to help
out work around the broken PHY. You can probably also simplify the PHY
driver, take out any code which tries to handle forced settings.

	Andrew