| Message ID | 20230404091442.3540092-1-michael.wei.hong.sit@intel.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC,net,1/1] net: stmmac: skip PHY scanning when PHY already attached in DT mode | expand |
Hi, On 04.04.2023 11:14, Michael Sit Wei Hong wrote: > If PHY is successfully attached during phylink_fwnode_phy_connect() > in DT mode. MAC should not need to scan for PHY again. > > Adding a logic to check if ovr_an_inband is set before scanning for > a PHY, since phylink_fwnode_phy_connect() returns 0 when > > phy_fwnode = fwnode_get_phy_node(fwnode); > if (IS_ERR(phy_fwnode)) { > if (pl->cfg_link_an_mode == MLO_AN_PHY) > return -ENODEV; > return 0; > } > > Fixes: fe2cfbc96803 ("net: stmmac: check if MAC needs to attach to a PHY") > Signed-off-by: Michael Sit Wei Hong <michael.wei.hong.sit@intel.com> This fixes broken ethernet observed recently on various Amlogic Meson based boards (like Khadas VIM3 or Odroid-C4). Thanks! Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index d41a5f92aee7..4b8d3d975678 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -1149,7 +1149,7 @@ static int stmmac_init_phy(struct net_device *dev) > /* Some DT bindings do not set-up the PHY handle. Let's try to > * manually parse it > */ > - if (!fwnode || phy_needed || ret) { > + if (!fwnode || (phy_needed && priv->phylink_config.ovr_an_inband) || ret) { > int addr = priv->plat->phy_addr; > struct phy_device *phydev; > Best regards
On Tue, Apr 04, 2023 at 05:14:42PM +0800, Michael Sit Wei Hong wrote: > If PHY is successfully attached during phylink_fwnode_phy_connect() > in DT mode. MAC should not need to scan for PHY again. > > Adding a logic to check if ovr_an_inband is set before scanning for > a PHY, since phylink_fwnode_phy_connect() returns 0 when > > phy_fwnode = fwnode_get_phy_node(fwnode); > if (IS_ERR(phy_fwnode)) { > if (pl->cfg_link_an_mode == MLO_AN_PHY) > return -ENODEV; > return 0; > } > > Fixes: fe2cfbc96803 ("net: stmmac: check if MAC needs to attach to a PHY") > Signed-off-by: Michael Sit Wei Hong <michael.wei.hong.sit@intel.com> > --- > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index d41a5f92aee7..4b8d3d975678 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -1149,7 +1149,7 @@ static int stmmac_init_phy(struct net_device *dev) > /* Some DT bindings do not set-up the PHY handle. Let's try to > * manually parse it > */ > - if (!fwnode || phy_needed || ret) { > + if (!fwnode || (phy_needed && priv->phylink_config.ovr_an_inband) || ret) { > int addr = priv->plat->phy_addr; > struct phy_device *phydev; > Sorry, but this just doesn't look right to me. And Gnrrrrr, I wish I'd spotted this stupidity during the review of phylink_expects_phy(). phy_needed will be true if phylink thinks there should be a PHY on the link, that being: MLO_AN_PHY mode MLO_AN_INBAND mode and non-802.3z interface mode If !phy_needed, then the code should not be attempting to attach a PHY, but calling phylink_fwnode_phy_connect() is fine as it will just return zero. If phy_needed is true, then phylink_fwnode_phy_connect() will check to see whether a PHY is in the fwnode. If we fail to find a PHY, then if we're in MLO_AN_PHY mode, that's an error, and we return -ENODEV. If there is no PHY device associated with the handle, we also return -ENODEV. If phy_needed is true, and phylink_fwnode_phy_connect() doesn't find a PHY in the fwnode, and we're in MLO_AN_INBAND mode (e.g. for SGMII) then we'll return zero, because we can cope without a PHY in this instance - it's a success. If we do find a PHY, then we will make use of it, and also return zero. The problem is this hacky code wants to know the difference between those two situations, but phylink doesn't allow you to, and I don't think now that phylink_expects_phy() solves that problem. I think you're better off doing this: struct fwnode_handle *phy_fwnode; if (!phylink_expects_phy(priv->phylink)) return 0; fwnode = of_fwnode_handle(priv->plat->phylink_node); if (!fwnode) fwnode = dev_fwnode(priv->device); if (fwnode) phy_fwnode = fwnode_get_phy_node(fwnode); else phy_fwnode = NULL; if (!phy_fwnode) { ... do non-DT PHY stuff ... ret = phylink_connect_phy(priv->phylink, phydev); } else { fwnode_handle_put(phy_fwnode); ret = phylink_fwnode_phy_connect(priv->phylink, fwnode, 0); } ... ethtool wol stuff ... Doesn't that more closely reflect what you actually want this code to be doing, rather than messing about trying to guess it from phylink's return code etc?
On Tue, Apr 4, 2023 at 11:15 AM Michael Sit Wei Hong <michael.wei.hong.sit@intel.com> wrote: > > If PHY is successfully attached during phylink_fwnode_phy_connect() > in DT mode. MAC should not need to scan for PHY again. > > Adding a logic to check if ovr_an_inband is set before scanning for > a PHY, since phylink_fwnode_phy_connect() returns 0 when > > phy_fwnode = fwnode_get_phy_node(fwnode); > if (IS_ERR(phy_fwnode)) { > if (pl->cfg_link_an_mode == MLO_AN_PHY) > return -ENODEV; > return 0; > } > > Fixes: fe2cfbc96803 ("net: stmmac: check if MAC needs to attach to a PHY") > Signed-off-by: Michael Sit Wei Hong <michael.wei.hong.sit@intel.com> Thank you for working on this! Your patch fixes the issue I reported, Ethernet is back on my X96 Air so this patch is: Tested-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> My understanding is that Russell King is asking for a second iteration of this patch with his feedback incorporated. I'm happy to test this second version as well if you keep me Cc'ed. For this second version you can also (unconditionally) add: Reported-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> Best regards, Martin
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index d41a5f92aee7..4b8d3d975678 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -1149,7 +1149,7 @@ static int stmmac_init_phy(struct net_device *dev) /* Some DT bindings do not set-up the PHY handle. Let's try to * manually parse it */ - if (!fwnode || phy_needed || ret) { + if (!fwnode || (phy_needed && priv->phylink_config.ovr_an_inband) || ret) { int addr = priv->plat->phy_addr; struct phy_device *phydev;
If PHY is successfully attached during phylink_fwnode_phy_connect() in DT mode. MAC should not need to scan for PHY again. Adding a logic to check if ovr_an_inband is set before scanning for a PHY, since phylink_fwnode_phy_connect() returns 0 when phy_fwnode = fwnode_get_phy_node(fwnode); if (IS_ERR(phy_fwnode)) { if (pl->cfg_link_an_mode == MLO_AN_PHY) return -ENODEV; return 0; } Fixes: fe2cfbc96803 ("net: stmmac: check if MAC needs to attach to a PHY") Signed-off-by: Michael Sit Wei Hong <michael.wei.hong.sit@intel.com> --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)