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

[SkyLake Huang (黃啟澤)]

On Tue, 2024-05-21 at 19:20 +0200, Andrew Lunn wrote:
> > > > +/* 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.
> 
Then I'll add comments and remove "returning error if picntrl switching
fails" like this:
/* Switch pinctrl after setting polarity to avoid bogus blinking */
pinctrl = devm_pinctrl_get_select(&phydev->mdio.dev, "i2p5gbe-led");
if (IS_ERR(pinctrl)) {
	dev_err(&phydev->mdio.dev, "Fail to set LED pins!\n");
}

Actually current drivers/net/phy/mediatek-ge-soc.c has the same
mechanism, which utilizing gbe-led pinmux group in
mt7988_phy_fix_leds_polarities():

/* Only now setup pinctrl to avoid bogus blinking */
pinctrl = devm_pinctrl_get_select(&phydev->mdio.dev, "gbe-led");
if (IS_ERR(pinctrl))
	dev_err(&phydev->mdio.bus->dev, "Failed to setup PHY LED
pinctrl\n");

Actually those pinmux groups exist in drivers/pinctrl/mediatek/pinctrl-
mt7988 of current openWRT tree:

https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/mediatek/files-6.1/drivers/pinctrl/mediatek/pinctrl-mt7988.c;h=9f9291124522c916c4e92f1598e0be44c3badad5;hb=f16dc4b42fb265affb2298e815a7ce0a13d60da6

I believe this will be upstreamed later.

> > > 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.
> 
Thanks for your clear explanation.

> 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
> 
According to this, I think maybe we may need to patch
drivers/net/ethernet/mediatek/mtk_eth_soc.c later. I'll fix
mt798x_2p5ge_phy_config_aneg() like this first:

if (phydev->autoneg == AUTONEG_DISABLE)
	return -EOPNOTSUPP;