| Message ID | 20230314070208.3703963-2-michael.wei.hong.sit@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix PHY handle no longer parsing | expand |
On Tue, 14 Mar 2023 15:02:07 +0800 Michael Sit Wei Hong wrote: > + fixed_node = fwnode_get_named_child_node(fwnode, "fixed-link"); > + fwnode_handle_put(fixed_node); fwnode_property_present() ?
> -----Original Message----- > From: Jakub Kicinski <kuba@kernel.org> > Sent: Friday, March 17, 2023 8:00 AM > To: Sit, Michael Wei Hong <michael.wei.hong.sit@intel.com> > Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre > Torgue <alexandre.torgue@foss.st.com>; Jose Abreu > <joabreu@synopsys.com>; David S . Miller > <davem@davemloft.net>; Eric Dumazet > <edumazet@google.com>; Paolo Abeni <pabeni@redhat.com>; > Maxime Coquelin <mcoquelin.stm32@gmail.com>; Ong, Boon > Leong <boon.leong.ong@intel.com>; netdev@vger.kernel.org; > linux-stm32@st-md-mailman.stormreply.com; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Looi, > Hong Aun <hong.aun.looi@intel.com>; Voon, Weifeng > <weifeng.voon@intel.com>; Lai, Peter Jun Ann > <peter.jun.ann.lai@intel.com> > Subject: Re: [PATCH net v2 1/2] net: stmmac: fix PHY handle > parsing > > On Tue, 14 Mar 2023 15:02:07 +0800 Michael Sit Wei Hong wrote: > > + fixed_node = > fwnode_get_named_child_node(fwnode, "fixed-link"); > > + fwnode_handle_put(fixed_node); > > fwnode_property_present() ? Good suggestion, will modify and submit in next revision.
On Tue, Mar 14, 2023 at 03:02:07PM +0800, Michael Sit Wei Hong wrote: > phylink_fwnode_phy_connect returns 0 when set to MLO_AN_INBAND. > This causes the PHY handle parsing to skip and the PHY will not be attached > to the MAC. Please could you expand the commit message because i'm having trouble following this. phylink_fwnode_phy_connect() says: /* Fixed links and 802.3z are handled without needing a PHY */ if (pl->cfg_link_an_mode == MLO_AN_FIXED || (pl->cfg_link_an_mode == MLO_AN_INBAND && phy_interface_mode_is_8023z(pl->link_interface))) return 0; So your first statement is not true. It should be MLO_AN_INBAND and phy_interface_mode_is_8023z. > Add additional check for PHY handle parsing when set to MLO_AN_INBAND. Looking at the patch, there is no reference to MLO_AN_INBAND, or managed = "in-band-status"; Andrew
On Fri, Mar 17, 2023 at 08:56:19PM +0100, Andrew Lunn wrote: > On Tue, Mar 14, 2023 at 03:02:07PM +0800, Michael Sit Wei Hong wrote: > > phylink_fwnode_phy_connect returns 0 when set to MLO_AN_INBAND. > > This causes the PHY handle parsing to skip and the PHY will not be attached > > to the MAC. > > Please could you expand the commit message because i'm having trouble > following this. > > phylink_fwnode_phy_connect() says: > > /* Fixed links and 802.3z are handled without needing a PHY */ > if (pl->cfg_link_an_mode == MLO_AN_FIXED || > (pl->cfg_link_an_mode == MLO_AN_INBAND && > phy_interface_mode_is_8023z(pl->link_interface))) > return 0; > > So your first statement is not true. It should be MLO_AN_INBAND > and phy_interface_mode_is_8023z. > > > Add additional check for PHY handle parsing when set to MLO_AN_INBAND. > > Looking at the patch, there is no reference to MLO_AN_INBAND, or > managed = "in-band-status"; That's the pesky "xpcs_an_inband" which ends up as phylink's "ovr_an_inband"... I'm sure these are random renames of stuff to make sure that people struggle to follow the code.
> -----Original Message----- > From: Russell King <linux@armlinux.org.uk> > Sent: Saturday, March 18, 2023 4:58 AM > To: Andrew Lunn <andrew@lunn.ch> > Cc: Sit, Michael Wei Hong <michael.wei.hong.sit@intel.com>; > Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre > Torgue <alexandre.torgue@foss.st.com>; Jose Abreu > <joabreu@synopsys.com>; David S . Miller > <davem@davemloft.net>; Eric Dumazet > <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; > Paolo Abeni <pabeni@redhat.com>; Maxime Coquelin > <mcoquelin.stm32@gmail.com>; Ong, Boon Leong > <boon.leong.ong@intel.com>; netdev@vger.kernel.org; linux- > stm32@st-md-mailman.stormreply.com; linux-arm- > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Looi, > Hong Aun <hong.aun.looi@intel.com>; Voon, Weifeng > <weifeng.voon@intel.com>; Lai, Peter Jun Ann > <peter.jun.ann.lai@intel.com> > Subject: Re: [PATCH net v2 1/2] net: stmmac: fix PHY handle > parsing > > On Fri, Mar 17, 2023 at 08:56:19PM +0100, Andrew Lunn wrote: > > On Tue, Mar 14, 2023 at 03:02:07PM +0800, Michael Sit Wei > Hong wrote: > > > phylink_fwnode_phy_connect returns 0 when set to > MLO_AN_INBAND. > > > This causes the PHY handle parsing to skip and the PHY will not > be > > > attached to the MAC. > > > > Please could you expand the commit message because i'm > having trouble > > following this. > > > > phylink_fwnode_phy_connect() says: > > > > /* Fixed links and 802.3z are handled without needing a > PHY */ > > if (pl->cfg_link_an_mode == MLO_AN_FIXED || > > (pl->cfg_link_an_mode == MLO_AN_INBAND && > > phy_interface_mode_is_8023z(pl->link_interface))) > > return 0; > > > > So your first statement is not true. It should be > MLO_AN_INBAND and > > phy_interface_mode_is_8023z. > > > > > Add additional check for PHY handle parsing when set to > MLO_AN_INBAND. > > > > Looking at the patch, there is no reference to > MLO_AN_INBAND, or > > managed = "in-band-status"; > > That's the pesky "xpcs_an_inband" which ends up as phylink's > "ovr_an_inband"... I'm sure these are random renames of stuff > to make sure that people struggle to follow the code. > It is as mentioned above, the "xpcs_an_inband" will end up as "ovr_an_inband" which will then set pl->cfg_link_an_mode = MLO_AN_INBAND in the phylink_parse_mode() in phylink.c The phylink_fwnode_phy_connect() checks if both MLO_AN_INBAND && phy_interface_mode_is_8023z() true before returning 0. But in our case, we only have MLO_AN_INBAND is true, which then goes to the next part of the code. 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; } Where here the IS_ERR(phy_fwnode) returns true, then it Checks for MLO_AN_PHY, which in our case is not, so it returns a 0. When returned 0, our driver will then skip the manual phy parsing due to if (!fwnode || ret) > -- > RMK's Patch system: > https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at > last!
On Tue, Mar 21, 2023 at 08:34:49AM +0000, Sit, Michael Wei Hong wrote: > > > > -----Original Message----- > > From: Russell King <linux@armlinux.org.uk> > > Sent: Saturday, March 18, 2023 4:58 AM > > To: Andrew Lunn <andrew@lunn.ch> > > Cc: Sit, Michael Wei Hong <michael.wei.hong.sit@intel.com>; > > Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre > > Torgue <alexandre.torgue@foss.st.com>; Jose Abreu > > <joabreu@synopsys.com>; David S . Miller > > <davem@davemloft.net>; Eric Dumazet > > <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; > > Paolo Abeni <pabeni@redhat.com>; Maxime Coquelin > > <mcoquelin.stm32@gmail.com>; Ong, Boon Leong > > <boon.leong.ong@intel.com>; netdev@vger.kernel.org; linux- > > stm32@st-md-mailman.stormreply.com; linux-arm- > > kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Looi, > > Hong Aun <hong.aun.looi@intel.com>; Voon, Weifeng > > <weifeng.voon@intel.com>; Lai, Peter Jun Ann > > <peter.jun.ann.lai@intel.com> > > Subject: Re: [PATCH net v2 1/2] net: stmmac: fix PHY handle > > parsing > > > > On Fri, Mar 17, 2023 at 08:56:19PM +0100, Andrew Lunn wrote: > > > On Tue, Mar 14, 2023 at 03:02:07PM +0800, Michael Sit Wei > > Hong wrote: > > > > phylink_fwnode_phy_connect returns 0 when set to > > MLO_AN_INBAND. > > > > This causes the PHY handle parsing to skip and the PHY will not > > be > > > > attached to the MAC. > > > > > > Please could you expand the commit message because i'm > > having trouble > > > following this. > > > > > > phylink_fwnode_phy_connect() says: > > > > > > /* Fixed links and 802.3z are handled without needing a > > PHY */ > > > if (pl->cfg_link_an_mode == MLO_AN_FIXED || > > > (pl->cfg_link_an_mode == MLO_AN_INBAND && > > > phy_interface_mode_is_8023z(pl->link_interface))) > > > return 0; > > > > > > So your first statement is not true. It should be > > MLO_AN_INBAND and > > > phy_interface_mode_is_8023z. > > > > > > > Add additional check for PHY handle parsing when set to > > MLO_AN_INBAND. > > > > > > Looking at the patch, there is no reference to > > MLO_AN_INBAND, or > > > managed = "in-band-status"; > > > > That's the pesky "xpcs_an_inband" which ends up as phylink's > > "ovr_an_inband"... I'm sure these are random renames of stuff > > to make sure that people struggle to follow the code. > > > It is as mentioned above, the "xpcs_an_inband" will end up as > "ovr_an_inband" which will then > set pl->cfg_link_an_mode = MLO_AN_INBAND in the > phylink_parse_mode() in phylink.c Let me make my comment more clear, because I don't think you understood it correctly. Please rename "xpcs_an_inband" to "ovr_an_inband" or "phylink_ovr_an_inband" so it's obvious what it is and where it ends up. Thanks.
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 8f543c3ab5c5..398adcd68ee8 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -1134,6 +1134,7 @@ static void stmmac_check_pcs_mode(struct stmmac_priv *priv) static int stmmac_init_phy(struct net_device *dev) { struct stmmac_priv *priv = netdev_priv(dev); + struct fwnode_handle *fixed_node; struct fwnode_handle *fwnode; int ret; @@ -1141,13 +1142,16 @@ static int stmmac_init_phy(struct net_device *dev) if (!fwnode) fwnode = dev_fwnode(priv->device); - if (fwnode) + if (fwnode) { + fixed_node = fwnode_get_named_child_node(fwnode, "fixed-link"); + fwnode_handle_put(fixed_node); ret = phylink_fwnode_phy_connect(priv->phylink, fwnode, 0); + } /* Some DT bindings do not set-up the PHY handle. Let's try to * manually parse it */ - if (!fwnode || ret) { + if (!fwnode || ret || !fixed_node) { int addr = priv->plat->phy_addr; struct phy_device *phydev;
phylink_fwnode_phy_connect returns 0 when set to MLO_AN_INBAND. This causes the PHY handle parsing to skip and the PHY will not be attached to the MAC. Add additional check for PHY handle parsing when set to MLO_AN_INBAND. Fixes: ab21cf920928 ("net: stmmac: make mdio register skips PHY scanning for fixed-link") Signed-off-by: Michael Sit Wei Hong <michael.wei.hong.sit@intel.com> Signed-off-by: Lai Peter Jun Ann <peter.jun.ann.lai@intel.com> --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)