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(+)

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;
 }
 
-- 
2.47.1

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,
-- 
2.30.2

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

Add support for the PCS to specify which interfaces it supports, which
can be used by MAC drivers to build the main supported_interfaces
bitmap. Phylink also validates that the PCS returned by the MAC driver
supports the interface that the MAC was asked for.

An empty supported_interfaces bitmap from the PCS indicates that it
does not provide this information, and we handle that appropriately.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 11 +++++++++++
 include/linux/phylink.h   |  3 +++
 2 files changed, 14 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 95fbc363f9a6..3e9960f54550 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -691,6 +691,17 @@ static int phylink_validate_mac_and_pcs(struct phylink *pl,
 			return -EINVAL;
 		}
 
+		/* Ensure that this PCS supports the interface which the MAC
+		 * returned it for. It is an error for the MAC to return a PCS
+		 * that does not support the interface mode.
+		 */
+		if (!phy_interface_empty(pcs->supported_interfaces) &&
+		    !test_bit(state->interface, pcs->supported_interfaces)) {
+			phylink_err(pl, "MAC returned PCS which does not support %s\n",
+				    phy_modes(state->interface));
+			return -EINVAL;
+		}
+
 		/* Validate the link parameters with the PCS */
 		if (pcs->ops->pcs_validate) {
 			ret = pcs->ops->pcs_validate(pcs, supported, state);
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 5462cc6a37dc..4b7a20620b49 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -393,6 +393,8 @@ struct phylink_pcs_ops;
 
 /**
  * struct phylink_pcs - PHYLINK PCS instance
+ * @supported_interfaces: describing which PHY_INTERFACE_MODE_xxx
+ *                        are supported by this PCS.
  * @ops: a pointer to the &struct phylink_pcs_ops structure
  * @phylink: pointer to &struct phylink_config
  * @neg_mode: provide PCS neg mode via "mode" argument
@@ -409,6 +411,7 @@ struct phylink_pcs_ops;
  * the PCS driver.
  */
 struct phylink_pcs {
+	DECLARE_PHY_INTERFACE_MASK(supported_interfaces);
 	const struct phylink_pcs_ops *ops;
 	struct phylink *phylink;
 	bool neg_mode;
-- 
2.30.2

Hi Russell,

On Fri, 13 Dec 2024 19:34:51 +0000
"Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:

> Add support for the PCS to specify which interfaces it supports, which
> can be used by MAC drivers to build the main supported_interfaces
> bitmap. Phylink also validates that the PCS returned by the MAC driver
> supports the interface that the MAC was asked for.
> 
> An empty supported_interfaces bitmap from the PCS indicates that it
> does not provide this information, and we handle that appropriately.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Thanks,

Maxime

Fill in the new PCS supported_interfaces member with the interfaces
that XPCS supports.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/pcs/pcs-xpcs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index f70ca39f0905..cf41dc5e74e8 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -1446,6 +1446,8 @@ static struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev)
 	if (ret)
 		goto out_clear_clks;
 
+	xpcs_get_interfaces(xpcs, xpcs->pcs.supported_interfaces);
+
 	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID)
 		xpcs->pcs.poll = false;
 	else
-- 
2.30.2

Hi Russell,

On Fri, 13 Dec 2024 19:34:56 +0000
"Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:

> Fill in the new PCS supported_interfaces member with the interfaces
> that XPCS supports.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Thanks,

Maxime

Fill in the new PCS supported_interfaces member with the interfaces
that the Mediatek LynxI supports.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/pcs/pcs-mtk-lynxi.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/pcs/pcs-mtk-lynxi.c b/drivers/net/pcs/pcs-mtk-lynxi.c
index 7de804535229..1377fb78eaa1 100644
--- a/drivers/net/pcs/pcs-mtk-lynxi.c
+++ b/drivers/net/pcs/pcs-mtk-lynxi.c
@@ -306,6 +306,11 @@ struct phylink_pcs *mtk_pcs_lynxi_create(struct device *dev,
 	mpcs->pcs.poll = true;
 	mpcs->interface = PHY_INTERFACE_MODE_NA;
 
+	__set_bit(PHY_INTERFACE_MODE_SGMII, mpcs->pcs.supported_interfaces);
+	__set_bit(PHY_INTERFACE_MODE_QSGMII, mpcs->pcs.supported_interfaces);
+	__set_bit(PHY_INTERFACE_MODE_1000BASEX, mpcs->pcs.supported_interfaces);
+	__set_bit(PHY_INTERFACE_MODE_2500BASEX, mpcs->pcs.supported_interfaces);
+
 	return &mpcs->pcs;
 }
 EXPORT_SYMBOL(mtk_pcs_lynxi_create);
-- 
2.30.2

Hi Russell,

On Fri, 13 Dec 2024 19:35:01 +0000
"Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:

> Fill in the new PCS supported_interfaces member with the interfaces
> that the Mediatek LynxI supports.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/pcs/pcs-mtk-lynxi.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/pcs/pcs-mtk-lynxi.c b/drivers/net/pcs/pcs-mtk-lynxi.c
> index 7de804535229..1377fb78eaa1 100644
> --- a/drivers/net/pcs/pcs-mtk-lynxi.c
> +++ b/drivers/net/pcs/pcs-mtk-lynxi.c
> @@ -306,6 +306,11 @@ struct phylink_pcs *mtk_pcs_lynxi_create(struct device *dev,
>  	mpcs->pcs.poll = true;
>  	mpcs->interface = PHY_INTERFACE_MODE_NA;
>  
> +	__set_bit(PHY_INTERFACE_MODE_SGMII, mpcs->pcs.supported_interfaces);
> +	__set_bit(PHY_INTERFACE_MODE_QSGMII, mpcs->pcs.supported_interfaces);

I'm sorry if I missed something, but I don't find where the QSGMII
support comes from based on the current codebase :/

I didn't spot that in the inband_caps commit, sorry :(

> +	__set_bit(PHY_INTERFACE_MODE_1000BASEX, mpcs->pcs.supported_interfaces);
> +	__set_bit(PHY_INTERFACE_MODE_2500BASEX, mpcs->pcs.supported_interfaces);

Thanks,

Maxime

On Tue, Dec 17, 2024 at 02:15:47PM +0100, Maxime Chevallier wrote:
> Hi Russell,
> 
> On Fri, 13 Dec 2024 19:35:01 +0000
> "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:
> 
> > Fill in the new PCS supported_interfaces member with the interfaces
> > that the Mediatek LynxI supports.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/net/pcs/pcs-mtk-lynxi.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/net/pcs/pcs-mtk-lynxi.c b/drivers/net/pcs/pcs-mtk-lynxi.c
> > index 7de804535229..1377fb78eaa1 100644
> > --- a/drivers/net/pcs/pcs-mtk-lynxi.c
> > +++ b/drivers/net/pcs/pcs-mtk-lynxi.c
> > @@ -306,6 +306,11 @@ struct phylink_pcs *mtk_pcs_lynxi_create(struct device *dev,
> >  	mpcs->pcs.poll = true;
> >  	mpcs->interface = PHY_INTERFACE_MODE_NA;
> >  
> > +	__set_bit(PHY_INTERFACE_MODE_SGMII, mpcs->pcs.supported_interfaces);
> > +	__set_bit(PHY_INTERFACE_MODE_QSGMII, mpcs->pcs.supported_interfaces);
> 
> I'm sorry if I missed something, but I don't find where the QSGMII
> support comes from based on the current codebase :/
> 
> I didn't spot that in the inband_caps commit, sorry :(
> 
> > +	__set_bit(PHY_INTERFACE_MODE_1000BASEX, mpcs->pcs.supported_interfaces);
> > +	__set_bit(PHY_INTERFACE_MODE_2500BASEX, mpcs->pcs.supported_interfaces);

This list comes from the behaviour of the PCS as it stood before any of
these changes - the PCS code itself never validates the interface it's
passed, except for the call to
phylink_mii_c22_pcs_encode_advertisement() and checking that the
return value is non-negative. This is the only place that the
interfaces will be restricted - and they will be restricted to the
four interfaces I've listed above.

I don't have information on the hardware; so I can only go by the
behaviour of the existing code when making changes - and I take the
approach when adding new stuff of trying to avoid changing the code
behaviour, even if the existing code is doing something wrong.

I think, therefore, that a patch to remove stuff that isn't actually
supported should come after these patches, because that changes the
driver behaviour - otherwise the reason why QSGMII isn't included in
the patch would have needed to be described in each commit adding
extra code dealing with the interface mode.

It would've been nice had the driver implemented .pcs_validate() from
the start, which would've made it obvious which interface modes were
supported!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Fill in the new PCS supported_interfaces member with the interfaces
that Lynx supports.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/pcs/pcs-lynx.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/net/pcs/pcs-lynx.c b/drivers/net/pcs/pcs-lynx.c
index 767a8c0714ac..6457190ec6e7 100644
--- a/drivers/net/pcs/pcs-lynx.c
+++ b/drivers/net/pcs/pcs-lynx.c
@@ -334,9 +334,19 @@ static const struct phylink_pcs_ops lynx_pcs_phylink_ops = {
 	.pcs_link_up = lynx_pcs_link_up,
 };
 
+static const phy_interface_t lynx_interfaces[] = {
+	PHY_INTERFACE_MODE_SGMII,
+	PHY_INTERFACE_MODE_QSGMII,
+	PHY_INTERFACE_MODE_1000BASEX,
+	PHY_INTERFACE_MODE_2500BASEX,
+	PHY_INTERFACE_MODE_10GBASER,
+	PHY_INTERFACE_MODE_USXGMII,
+};
+
 static struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio)
 {
 	struct lynx_pcs *lynx;
+	int i;
 
 	lynx = kzalloc(sizeof(*lynx), GFP_KERNEL);
 	if (!lynx)
@@ -348,6 +358,9 @@ static struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio)
 	lynx->pcs.neg_mode = true;
 	lynx->pcs.poll = true;
 
+	for (i = 0; i < ARRAY_SIZE(lynx_interfaces); i++)
+		__set_bit(lynx_interfaces[i], lynx->pcs.supported_interfaces);
+
 	return lynx_to_phylink_pcs(lynx);
 }
 
-- 
2.30.2

On Fri, 13 Dec 2024 19:35:07 +0000
"Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:

> Fill in the new PCS supported_interfaces member with the interfaces
> that Lynx supports.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Thanks,

Maxime

Use the PCS' supported_interfaces member to build the MAC level
supported_interfaces bitmap.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index d45fd7a3acd5..0e45c4a48bb5 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1206,6 +1206,7 @@ static int stmmac_phy_setup(struct stmmac_priv *priv)
 	struct stmmac_mdio_bus_data *mdio_bus_data;
 	int mode = priv->plat->phy_interface;
 	struct fwnode_handle *fwnode;
+	struct phylink_pcs *pcs;
 	struct phylink *phylink;
 
 	priv->phylink_config.dev = &priv->dev->dev;
@@ -1227,8 +1228,14 @@ static int stmmac_phy_setup(struct stmmac_priv *priv)
 
 	/* If we have an xpcs, it defines which PHY interfaces are supported. */
 	if (priv->hw->xpcs)
-		xpcs_get_interfaces(priv->hw->xpcs,
-				    priv->phylink_config.supported_interfaces);
+		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);
 
 	fwnode = priv->plat->port_node;
 	if (!fwnode)
-- 
2.30.2

Hi Russell,

On Fri, 13 Dec 2024 19:35:12 +0000
"Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:

> Use the PCS' supported_interfaces member to build the MAC level
> supported_interfaces bitmap.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index d45fd7a3acd5..0e45c4a48bb5 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1206,6 +1206,7 @@ static int stmmac_phy_setup(struct stmmac_priv *priv)
>  	struct stmmac_mdio_bus_data *mdio_bus_data;
>  	int mode = priv->plat->phy_interface;
>  	struct fwnode_handle *fwnode;
> +	struct phylink_pcs *pcs;
>  	struct phylink *phylink;
>  
>  	priv->phylink_config.dev = &priv->dev->dev;
> @@ -1227,8 +1228,14 @@ static int stmmac_phy_setup(struct stmmac_priv *priv)
>  
>  	/* If we have an xpcs, it defines which PHY interfaces are supported. */
>  	if (priv->hw->xpcs)
> -		xpcs_get_interfaces(priv->hw->xpcs,
> -				    priv->phylink_config.supported_interfaces);
> +		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);
>  
>  	fwnode = priv->plat->port_node;
>  	if (!fwnode)

I think that we could even make xpcs_get_interfaces a static
non-exported function now :) But this can be done in a subsequent patch.

Thanks for that work !

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

On Tue, Dec 17, 2024 at 02:19:12PM +0100, Maxime Chevallier wrote:
> Hi Russell,
> 
> On Fri, 13 Dec 2024 19:35:12 +0000
> "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:
> 
> > Use the PCS' supported_interfaces member to build the MAC level
> > supported_interfaces bitmap.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index d45fd7a3acd5..0e45c4a48bb5 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -1206,6 +1206,7 @@ static int stmmac_phy_setup(struct stmmac_priv *priv)
> >  	struct stmmac_mdio_bus_data *mdio_bus_data;
> >  	int mode = priv->plat->phy_interface;
> >  	struct fwnode_handle *fwnode;
> > +	struct phylink_pcs *pcs;
> >  	struct phylink *phylink;
> >  
> >  	priv->phylink_config.dev = &priv->dev->dev;
> > @@ -1227,8 +1228,14 @@ static int stmmac_phy_setup(struct stmmac_priv *priv)
> >  
> >  	/* If we have an xpcs, it defines which PHY interfaces are supported. */
> >  	if (priv->hw->xpcs)
> > -		xpcs_get_interfaces(priv->hw->xpcs,
> > -				    priv->phylink_config.supported_interfaces);
> > +		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);
> >  
> >  	fwnode = priv->plat->port_node;
> >  	if (!fwnode)
> 
> I think that we could even make xpcs_get_interfaces a static
> non-exported function now :) But this can be done in a subsequent patch.

Yes, that can definitely be done. I already have a patch for this.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!