| Message ID | 20241213090526.71516-3-maxime.chevallier@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: stmmac: dwmac-socfpga: Allow using 1000BaseX | expand |
On Fri, Dec 13, 2024 at 10:05:25AM +0100, Maxime Chevallier wrote: > On Socfpga, the dwmac controller uses a variation of the Lynx PCS to get > additional support for SGMII and 1000BaseX. The switch between these > modes may occur at runtime (e.g. when the interface is wired to an SFP > cage). In such case, phylink will validate the newly selected interface > between the MAC and SFP based on the internal "supported_interfaces" > field. > > For now in stmmac, this field is populated based on : > - The interface specified in firmware (DT) > - The interfaces supported by XPCS, when XPCS is in use. > > In our case, the PCS in Lynx and not XPCS. > > This commit makes so that the .pcs_init() implementation of > dwmac-socfpga populates the supported_interface when the Lynx PCS was > successfully initialized. I think it would also be worth adding this to Lynx, so phylink also gets to know (via its validation) which PHY interface modes the PCS can support. However, maybe at this point we need to introduce an interface bitmap into struct phylink_pcs so that these kinds of checks can be done in phylink itself when it has the PCS, and it would also mean that stmmac could do something like: struct phylink_pcs *pcs; if (priv->hw->xpcs) pcs = xpcs_to_phylink_pcs(priv->hw->xpcs); else pcs = priv->hw->phylink_pcs; if (pcs) phy_interface_or(priv->phylink_config.supported_interfaces, priv->phylink_config.supported_interfaces, pcs->supported_interfaces); and not have to worry about this from individual PCS or platform code. 8<=== From: "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> Subject: [PATCH net-next] net: pcs: lynx: implement pcs_validate() Implement .pcs_validate() to restrict the interfaces to those which the Lynx PCS supports. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/pcs/pcs-lynx.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/drivers/net/pcs/pcs-lynx.c b/drivers/net/pcs/pcs-lynx.c index 767a8c0714ac..fd2e06dba92e 100644 --- a/drivers/net/pcs/pcs-lynx.c +++ b/drivers/net/pcs/pcs-lynx.c @@ -326,7 +326,22 @@ static void lynx_pcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode, } } +static int lynx_pcs_validate(struct phylink_pcs *pcs, unsigned long *supported, + const struct phylink_link_state *state) +{ + if (state->interface != PHY_INTERFACE_MODE_SGMII && + state->interface != PHY_INTERFACE_MODE_QSGMII && + state->interface != PHY_INTERFACE_MODE_1000BASEX && + state->interface != PHY_INTERFACE_MODE_2500BASEX && + state->interface != PHY_INTERFACE_MODE_10GBASER && + state->interface != PHY_INTERFACE_MODE_USXGMII) + return -EINVAL; + + return 0; +} + static const struct phylink_pcs_ops lynx_pcs_phylink_ops = { + .pcs_validate = lynx_pcs_validate, .pcs_inband_caps = lynx_pcs_inband_caps, .pcs_get_state = lynx_pcs_get_state, .pcs_config = lynx_pcs_config,
Hi Russell, On Fri, 13 Dec 2024 12:22:45 +0000 "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > On Fri, Dec 13, 2024 at 10:05:25AM +0100, Maxime Chevallier wrote: > > On Socfpga, the dwmac controller uses a variation of the Lynx PCS to get > > additional support for SGMII and 1000BaseX. The switch between these > > modes may occur at runtime (e.g. when the interface is wired to an SFP > > cage). In such case, phylink will validate the newly selected interface > > between the MAC and SFP based on the internal "supported_interfaces" > > field. > > > > For now in stmmac, this field is populated based on : > > - The interface specified in firmware (DT) > > - The interfaces supported by XPCS, when XPCS is in use. > > > > In our case, the PCS in Lynx and not XPCS. > > > > This commit makes so that the .pcs_init() implementation of > > dwmac-socfpga populates the supported_interface when the Lynx PCS was > > successfully initialized. > > I think it would also be worth adding this to Lynx, so phylink also > gets to know (via its validation) which PHY interface modes the PCS > can support. > > However, maybe at this point we need to introduce an interface bitmap > into struct phylink_pcs so that these kinds of checks can be done in > phylink itself when it has the PCS, and it would also mean that stmmac > could do something like: > > struct phylink_pcs *pcs; > > if (priv->hw->xpcs) > pcs = xpcs_to_phylink_pcs(priv->hw->xpcs); > else > pcs = priv->hw->phylink_pcs; > > if (pcs) > phy_interface_or(priv->phylink_config.supported_interfaces, > priv->phylink_config.supported_interfaces, > pcs->supported_interfaces); > > and not have to worry about this from individual PCS or platform code. I like the idea, I will give it a go and send a series for that if that's ok :) Thanks, Maxime
On Fri, Dec 13, 2024 at 06:29:04PM +0100, Maxime Chevallier wrote: > Hi Russell, > > On Fri, 13 Dec 2024 12:22:45 +0000 > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > > > On Fri, Dec 13, 2024 at 10:05:25AM +0100, Maxime Chevallier wrote: > > > On Socfpga, the dwmac controller uses a variation of the Lynx PCS to get > > > additional support for SGMII and 1000BaseX. The switch between these > > > modes may occur at runtime (e.g. when the interface is wired to an SFP > > > cage). In such case, phylink will validate the newly selected interface > > > between the MAC and SFP based on the internal "supported_interfaces" > > > field. > > > > > > For now in stmmac, this field is populated based on : > > > - The interface specified in firmware (DT) > > > - The interfaces supported by XPCS, when XPCS is in use. > > > > > > In our case, the PCS in Lynx and not XPCS. > > > > > > This commit makes so that the .pcs_init() implementation of > > > dwmac-socfpga populates the supported_interface when the Lynx PCS was > > > successfully initialized. > > > > I think it would also be worth adding this to Lynx, so phylink also > > gets to know (via its validation) which PHY interface modes the PCS > > can support. > > > > However, maybe at this point we need to introduce an interface bitmap > > into struct phylink_pcs so that these kinds of checks can be done in > > phylink itself when it has the PCS, and it would also mean that stmmac > > could do something like: > > > > struct phylink_pcs *pcs; > > > > if (priv->hw->xpcs) > > pcs = xpcs_to_phylink_pcs(priv->hw->xpcs); > > else > > pcs = priv->hw->phylink_pcs; > > > > if (pcs) > > phy_interface_or(priv->phylink_config.supported_interfaces, > > priv->phylink_config.supported_interfaces, > > pcs->supported_interfaces); > > > > and not have to worry about this from individual PCS or platform code. > > I like the idea, I will give it a go and send a series for that if > that's ok :) I've actually already created that series!
Hello Russell, On Fri, 13 Dec 2024 19:21:38 +0000 "Russell King (Oracle)" <linux@armlinux.org.uk> wrote: > > > However, maybe at this point we need to introduce an interface bitmap > > > into struct phylink_pcs so that these kinds of checks can be done in > > > phylink itself when it has the PCS, and it would also mean that stmmac > > > could do something like: [...] > > > and not have to worry about this from individual PCS or platform code. > > > > I like the idea, I will give it a go and send a series for that if > > that's ok :) > > I've actually already created that series! Woaw that was fast ! I'll review and give it a test on my setup then. Maybe one thing to clarify with the net maintainers is that this work you've done doesn't replace the series this thread is replying to, which still makes sense (we need the stmmac_priv->phylink_config.supported_interfaces to be correctly populated on socfpga). Thanks a lot for that work Russell, Maxime Thanks a lot, Maxime
On Mon, 16 Dec 2024 09:42:24 +0100 Maxime Chevallier wrote: > > I've actually already created that series! > > Woaw that was fast ! I'll review and give it a test on my setup then. > > Maybe one thing to clarify with the net maintainers is that this work > you've done doesn't replace the series this thread is replying to, > which still makes sense (we need the > stmmac_priv->phylink_config.supported_interfaces to be correctly > populated on socfpga). Ah, sorry. Should have asked. I assumed since Lynx PCS will have the SGMII and 1000BASEX set - Russell's patch 5 will copy them for you to priv->phylink_config.supported_interfaces. Your patch 1 is still needed. Did I get it wrong?
Hi Jakub, On Mon, 16 Dec 2024 17:33:33 -0800 Jakub Kicinski <kuba@kernel.org> wrote: > On Mon, 16 Dec 2024 09:42:24 +0100 Maxime Chevallier wrote: > > > I've actually already created that series! > > > > Woaw that was fast ! I'll review and give it a test on my setup then. > > > > Maybe one thing to clarify with the net maintainers is that this work > > you've done doesn't replace the series this thread is replying to, > > which still makes sense (we need the > > stmmac_priv->phylink_config.supported_interfaces to be correctly > > populated on socfpga). > > Ah, sorry. Should have asked. > > I assumed since Lynx PCS will have the SGMII and 1000BASEX set - > Russell's patch 5 will copy them for you to > priv->phylink_config.supported_interfaces. Your patch 1 is still needed. > Did I get it wrong? Both are needed actually :) There are 2 issues on socfpga : - 1000BaseX needs to be understood by the socfpga wrapper (dwmac-socfpga) so that the internal serdes is enabled in the wrapper, that would be patch 1 - The priv->phylink_config.supported_interfaces is incomplete on dwmac-socfpga. Russell's patch 5 intersects these modes with that the PCS supports : + phy_interface_or(priv->phylink_config.supported_interfaces, + priv->phylink_config.supported_interfaces, + pcs->supported_interfaces); So without patch 2 in the series, we'll be missing PHY_INTERFACE_MODE_1000BASEX in the end result :) Thanks, Maxime
Let me triple check ;) On Tue, 17 Dec 2024 13:59:32 +0100 Maxime Chevallier wrote: > - The priv->phylink_config.supported_interfaces is incomplete on > dwmac-socfpga. Russell's patch 5 intersects these modes with that the ^^^^^^^^^^ > PCS supports : > > + phy_interface_or(priv->phylink_config.supported_interfaces, ^^ > + priv->phylink_config.supported_interfaces, > + pcs->supported_interfaces); > > So without patch 2 in the series, we'll be missing > PHY_INTERFACE_MODE_1000BASEX in the end result :) "Or" is a sum/union, not intersection. You set the bits in priv->phylink_config.supported_interfaces. Russell does: priv->phylink_config.supported_interfaces |= pcs->supported_interfaces; If I'm missing the point please repost once Russell's patches are merged :)
On Tue, 17 Dec 2024 06:49:07 -0800 Jakub Kicinski <kuba@kernel.org> wrote: > Let me triple check ;) > > On Tue, 17 Dec 2024 13:59:32 +0100 Maxime Chevallier wrote: > > - The priv->phylink_config.supported_interfaces is incomplete on > > dwmac-socfpga. Russell's patch 5 intersects these modes with that the > ^^^^^^^^^^ > > PCS supports : > > > > + phy_interface_or(priv->phylink_config.supported_interfaces, > ^^ > > + priv->phylink_config.supported_interfaces, > > + pcs->supported_interfaces); > > > > So without patch 2 in the series, we'll be missing > > PHY_INTERFACE_MODE_1000BASEX in the end result :) > > "Or" is a sum/union, not intersection. > > You set the bits in priv->phylink_config.supported_interfaces. > Russell does: > > priv->phylink_config.supported_interfaces |= > pcs->supported_interfaces; > > If I'm missing the point please repost once Russell's patches > are merged :) Erf no I was missing the point, time to catch-up on some sleep I guess... I read an 'and' and it was firmly stuck in my mind... nevermind then, patch 2 isn't required anymore... Maxime
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c index 8c7b82d29fd1..ff9a30afd7e1 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c @@ -425,6 +425,17 @@ static int socfpga_dwmac_pcs_init(struct stmmac_priv *priv) return PTR_ERR(pcs); priv->hw->phylink_pcs = pcs; + + /* Having a Lynx PCS means we can use SGMII and 1000BaseX. Phylink's + * supported_interface is populated according to what's found in + * devicetree, but as we can dynamically switch between both modes at + * runtime, make sure both modes are marked as supported + */ + __set_bit(PHY_INTERFACE_MODE_1000BASEX, + priv->phylink_config.supported_interfaces); + __set_bit(PHY_INTERFACE_MODE_SGMII, + priv->phylink_config.supported_interfaces); + return 0; }
On Socfpga, the dwmac controller uses a variation of the Lynx PCS to get additional support for SGMII and 1000BaseX. The switch between these modes may occur at runtime (e.g. when the interface is wired to an SFP cage). In such case, phylink will validate the newly selected interface between the MAC and SFP based on the internal "supported_interfaces" field. For now in stmmac, this field is populated based on : - The interface specified in firmware (DT) - The interfaces supported by XPCS, when XPCS is in use. In our case, the PCS in Lynx and not XPCS. This commit makes so that the .pcs_init() implementation of dwmac-socfpga populates the supported_interface when the Lynx PCS was successfully initialized. Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> --- drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c | 11 +++++++++++ 1 file changed, 11 insertions(+)