[PATCH net-next v2 0/5] net: stmmac: provide platform select_pcs method

[PATCH net-next v2 0/5] net: stmmac: provide platform select_pcs method

[Russell King (Oracle)]

Hi,

This series adds a select_pcs() method to the stmmac platform data to
allow platforms that need to provide their own PCSes to do so, moving
the decision making into platform code.

This avoids questions such as "what should the priority of XPCS vs
some other platform PCS be?" and when we provide a PCS for the
internal PCS, how that interacts with both the XPCS and platform
provided PCS.

Note that if a platform implements the select_pcs() method, then the
return values are:
- a phylink_pcs pointer - the PCS to be used.
- NULL - no phylink_pcs to be used.
Otherwise (if not implemented or returns an error-pointer), then
allow the the stmmac internal PCS to be used if appropriate (once
that patch set is merged.)

Patch 1 introduces the new method.
Patch 2 converts Intel mGBE to use this to provide the XPCS and
 removes the XPCS decision making from core code.
Patch 3 provides an implementation for rzn1 to return its PCS.
Patch 4 does the same for socfpga.
Patch 5 removes the core code returning priv->hw->phylink_pcs.

No functional change is anticipated. Once this has been merged, it
will be expected that platforms should populate all three PCS
methods or none of the PCS methods.

v1->v2:
- Fix build error in patch 2.
- Add attributations from Romain Gantois.

 drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c   | 11 +++++++++++
 drivers/net/ethernet/stmicro/stmmac/dwmac-rzn1.c    |  7 +++++++
 drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c |  7 +++++++
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c   | 10 +++++++---
 include/linux/stmmac.h                              |  4 +++-
 5 files changed, 35 insertions(+), 4 deletions(-)

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

[PATCH net-next v2 1/5] net: stmmac: add select_pcs() platform method

[Russell King (Oracle)]

Allow platform drivers to provide their logic to select an appropriate
PCS.

Tested-by: Romain Gantois <romain.gantois@bootlin.com>
Reviewed-by: Romain Gantois <romain.gantois@bootlin.com>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 7 +++++++
 include/linux/stmmac.h                            | 4 +++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index bbedf2a8c60f..302aa4080de3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -949,6 +949,13 @@ static struct phylink_pcs *stmmac_mac_select_pcs(struct phylink_config *config,
 						 phy_interface_t interface)
 {
 	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
+	struct phylink_pcs *pcs;
+
+	if (priv->plat->select_pcs) {
+		pcs = priv->plat->select_pcs(priv, interface);
+		if (!IS_ERR(pcs))
+			return pcs;
+	}
 
 	if (priv->hw->xpcs)
 		return &priv->hw->xpcs->pcs;
diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index 8f0f156d50d3..9c54f82901a1 100644
--- a/include/linux/stmmac.h
+++ b/include/linux/stmmac.h
@@ -13,7 +13,7 @@
 #define __STMMAC_PLATFORM_DATA
 
 #include <linux/platform_device.h>
-#include <linux/phy.h>
+#include <linux/phylink.h>
 
 #define MTL_MAX_RX_QUEUES	8
 #define MTL_MAX_TX_QUEUES	8
@@ -271,6 +271,8 @@ struct plat_stmmacenet_data {
 	void (*dump_debug_regs)(void *priv);
 	int (*pcs_init)(struct stmmac_priv *priv);
 	void (*pcs_exit)(struct stmmac_priv *priv);
+	struct phylink_pcs *(*select_pcs)(struct stmmac_priv *priv,
+					  phy_interface_t interface);
 	void *bsp_priv;
 	struct clk *stmmac_clk;
 	struct clk *pclk;
-- 
2.30.2

Re: [PATCH net-next v2 1/5] net: stmmac: add select_pcs() platform method

[Serge Semin]

Hi Russell

On Thu, Jun 13, 2024 at 11:36:06AM +0100, Russell King (Oracle) wrote:
> Allow platform drivers to provide their logic to select an appropriate
> PCS.
> 
> Tested-by: Romain Gantois <romain.gantois@bootlin.com>
> Reviewed-by: Romain Gantois <romain.gantois@bootlin.com>
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 7 +++++++
>  include/linux/stmmac.h                            | 4 +++-
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index bbedf2a8c60f..302aa4080de3 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -949,6 +949,13 @@ static struct phylink_pcs *stmmac_mac_select_pcs(struct phylink_config *config,
>  						 phy_interface_t interface)
>  {
>  	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
> +	struct phylink_pcs *pcs;
> +
> +	if (priv->plat->select_pcs) {
> +		pcs = priv->plat->select_pcs(priv, interface);
> +		if (!IS_ERR(pcs))
> +			return pcs;
> +	}
>  
>  	if (priv->hw->xpcs)
>  		return &priv->hw->xpcs->pcs;
> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
> index 8f0f156d50d3..9c54f82901a1 100644
> --- a/include/linux/stmmac.h
> +++ b/include/linux/stmmac.h
> @@ -13,7 +13,7 @@
>  #define __STMMAC_PLATFORM_DATA
>  
>  #include <linux/platform_device.h>
> -#include <linux/phy.h>
> +#include <linux/phylink.h>
>  
>  #define MTL_MAX_RX_QUEUES	8
>  #define MTL_MAX_TX_QUEUES	8
> @@ -271,6 +271,8 @@ struct plat_stmmacenet_data {
>  	void (*dump_debug_regs)(void *priv);

>  	int (*pcs_init)(struct stmmac_priv *priv);
>  	void (*pcs_exit)(struct stmmac_priv *priv);
> +	struct phylink_pcs *(*select_pcs)(struct stmmac_priv *priv,
> +					  phy_interface_t interface);

Just a small note/nitpick. We've got pcs_init() and pcs_exit()
callbacks. Both of them have the pcs_ prefix followed by the action
verb. What about using the same notation for the PCS-select method,
using the plat_stmmacenet_data::pcs_select() callback-name instead?

-Serge(y)

>  	void *bsp_priv;
>  	struct clk *stmmac_clk;
>  	struct clk *pclk;
> -- 
> 2.30.2
> 

Re: [PATCH net-next v2 1/5] net: stmmac: add select_pcs() platform method

[Russell King (Oracle)]

On Fri, Jun 14, 2024 at 07:38:40PM +0300, Serge Semin wrote:
> Hi Russell
> 
> On Thu, Jun 13, 2024 at 11:36:06AM +0100, Russell King (Oracle) wrote:
> > Allow platform drivers to provide their logic to select an appropriate
> > PCS.
> > 
> > Tested-by: Romain Gantois <romain.gantois@bootlin.com>
> > Reviewed-by: Romain Gantois <romain.gantois@bootlin.com>
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 7 +++++++
> >  include/linux/stmmac.h                            | 4 +++-
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index bbedf2a8c60f..302aa4080de3 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -949,6 +949,13 @@ static struct phylink_pcs *stmmac_mac_select_pcs(struct phylink_config *config,
> >  						 phy_interface_t interface)
> >  {
> >  	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
> > +	struct phylink_pcs *pcs;
> > +
> > +	if (priv->plat->select_pcs) {
> > +		pcs = priv->plat->select_pcs(priv, interface);
> > +		if (!IS_ERR(pcs))
> > +			return pcs;
> > +	}
> >  
> >  	if (priv->hw->xpcs)
> >  		return &priv->hw->xpcs->pcs;
> > diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
> > index 8f0f156d50d3..9c54f82901a1 100644
> > --- a/include/linux/stmmac.h
> > +++ b/include/linux/stmmac.h
> > @@ -13,7 +13,7 @@
> >  #define __STMMAC_PLATFORM_DATA
> >  
> >  #include <linux/platform_device.h>
> > -#include <linux/phy.h>
> > +#include <linux/phylink.h>
> >  
> >  #define MTL_MAX_RX_QUEUES	8
> >  #define MTL_MAX_TX_QUEUES	8
> > @@ -271,6 +271,8 @@ struct plat_stmmacenet_data {
> >  	void (*dump_debug_regs)(void *priv);
> 
> >  	int (*pcs_init)(struct stmmac_priv *priv);
> >  	void (*pcs_exit)(struct stmmac_priv *priv);
> > +	struct phylink_pcs *(*select_pcs)(struct stmmac_priv *priv,
> > +					  phy_interface_t interface);
> 
> Just a small note/nitpick. We've got pcs_init() and pcs_exit()
> callbacks. Both of them have the pcs_ prefix followed by the action
> verb. What about using the same notation for the PCS-select method,
> using the plat_stmmacenet_data::pcs_select() callback-name instead?

From phylink's perspective, it's not part of the PCS, it's something
that the MAC does.

The interface passed in to mac_select_pcs() so so the MAC code can
decide which PCS (if it has many to choose from) will be used for
the specified interface mode to either a PHY or other device
connected to the netdev. It isn't a PCS operation, it's an
operation that returns an appropriate PCS.

So, I want to keep "select_pcs" as at least a suffix, because it
is selecting a PCS. Eventually, I would like to see the stmmac
implementations check the "interface" passed to it before deciding
whether to return a PCS or not - thus how it's intended to be
implemented.

"pcs_select" seems to make it sound like it's part of a PCS
implementation, which as I've explained above, it isn't. It also
doesn't convey that it's selecting a PCS based on its arguments.

I'd also like to keep the ability to grep for "select_pcs" to
find implementations and not have complex grep expressions to
find whatever the driver has called it! To that end, I much prefer
that drivers that name sub-implementations the same way that
phylink names them to make grepping easier.

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

[PATCH net-next v2 2/5] net: stmmac: dwmac-intel: provide a select_pcs() implementation

[Russell King (Oracle)]

Move the code returning the XPCS into dwmac-intel, which is the only
user of XPCS. Fill in the select_pcs() implementation only when we are
going to setup the XPCS, thus when it should be present.

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

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
index 56649edb18cd..094d34c4193c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
@@ -443,6 +443,16 @@ static void common_default_data(struct plat_stmmacenet_data *plat)
 	plat->rx_queues_cfg[0].pkt_route = 0x0;
 }
 
+static struct phylink_pcs *intel_mgbe_select_pcs(struct stmmac_priv *priv,
+						 phy_interface_t interface)
+{
+	/* plat->mdio_bus_data->has_xpcs has been set true, so there
+	 * should always be an XPCS. The original code would always
+	 * return this if present.
+	 */
+	return &priv->hw->xpcs->pcs;
+}
+
 static int intel_mgbe_common_data(struct pci_dev *pdev,
 				  struct plat_stmmacenet_data *plat)
 {
@@ -587,6 +597,7 @@ static int intel_mgbe_common_data(struct pci_dev *pdev,
 	    plat->phy_interface == PHY_INTERFACE_MODE_1000BASEX) {
 		plat->mdio_bus_data->has_xpcs = true;
 		plat->mdio_bus_data->default_an_inband = true;
+		plat->select_pcs = intel_mgbe_select_pcs;
 	}
 
 	/* Ensure mdio bus scan skips intel serdes and pcs-xpcs */
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 302aa4080de3..e9e2a95c91a3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -957,9 +957,6 @@ static struct phylink_pcs *stmmac_mac_select_pcs(struct phylink_config *config,
 			return pcs;
 	}
 
-	if (priv->hw->xpcs)
-		return &priv->hw->xpcs->pcs;
-
 	return priv->hw->phylink_pcs;
 }
 
-- 
2.30.2

[PATCH net-next v2 3/5] net: stmmac: dwmac-rzn1: provide select_pcs() implementation

[Russell King (Oracle)]

Provide a .select_pcs() implementation which returns the phylink PCS
that was created in the .pcs_init() method.

Tested-by: Romain Gantois <romain.gantois@bootlin.com>
Reviewed-by: Romain Gantois <romain.gantois@bootlin.com>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-rzn1.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rzn1.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rzn1.c
index 848cf3c01f4a..59a7bd560f96 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rzn1.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rzn1.c
@@ -39,6 +39,12 @@ static void rzn1_dwmac_pcs_exit(struct stmmac_priv *priv)
 		miic_destroy(priv->hw->phylink_pcs);
 }
 
+static struct phylink_pcs *rzn1_dwmac_select_pcs(struct stmmac_priv *priv,
+						 phy_interface_t interface)
+{
+	return priv->hw->phylink_pcs;
+}
+
 static int rzn1_dwmac_probe(struct platform_device *pdev)
 {
 	struct plat_stmmacenet_data *plat_dat;
@@ -57,6 +63,7 @@ static int rzn1_dwmac_probe(struct platform_device *pdev)
 	plat_dat->bsp_priv = plat_dat;
 	plat_dat->pcs_init = rzn1_dwmac_pcs_init;
 	plat_dat->pcs_exit = rzn1_dwmac_pcs_exit;
+	plat_dat->select_pcs = rzn1_dwmac_select_pcs;
 
 	ret = stmmac_dvr_probe(dev, plat_dat, &stmmac_res);
 	if (ret)
-- 
2.30.2

[PATCH net-next v2 4/5] net: stmmac: dwmac-socfpga: provide select_pcs() implementation

[Russell King (Oracle)]

Provide a .select_pcs() implementation which returns the phylink PCS
that was created in the .pcs_init() method.

Reviewed-by: Romain Gantois <romain.gantois@bootlin.com>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
index b3d45f9dfb55..fdb4c773ec98 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-socfpga.c
@@ -429,6 +429,12 @@ static void socfpga_dwmac_pcs_exit(struct stmmac_priv *priv)
 		lynx_pcs_destroy(priv->hw->phylink_pcs);
 }
 
+static struct phylink_pcs *socfpga_dwmac_select_pcs(struct stmmac_priv *priv,
+						    phy_interface_t interface)
+{
+	return priv->hw->phylink_pcs;
+}
+
 static int socfpga_dwmac_probe(struct platform_device *pdev)
 {
 	struct plat_stmmacenet_data *plat_dat;
@@ -478,6 +484,7 @@ static int socfpga_dwmac_probe(struct platform_device *pdev)
 	plat_dat->fix_mac_speed = socfpga_dwmac_fix_mac_speed;
 	plat_dat->pcs_init = socfpga_dwmac_pcs_init;
 	plat_dat->pcs_exit = socfpga_dwmac_pcs_exit;
+	plat_dat->select_pcs = socfpga_dwmac_select_pcs;
 
 	ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
 	if (ret)
-- 
2.30.2

[PATCH net-next v2 5/5] net: stmmac: clean up stmmac_mac_select_pcs()

[Russell King (Oracle)]

Since all platform providers of PCS now populate the select_pcs()
method, there is no need for the common code to look at
priv->hw->phylink_pcs, so remove it.

Reviewed-by: Romain Gantois <romain.gantois@bootlin.com>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 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 e9e2a95c91a3..5ddbb0d44373 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -957,7 +957,7 @@ static struct phylink_pcs *stmmac_mac_select_pcs(struct phylink_config *config,
 			return pcs;
 	}
 
-	return priv->hw->phylink_pcs;
+	return NULL;
 }
 
 static void stmmac_mac_config(struct phylink_config *config, unsigned int mode,
-- 
2.30.2

Re: [PATCH net-next v2 0/5] net: stmmac: provide platform select_pcs method

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 13 Jun 2024 11:35:25 +0100 you wrote:
> Hi,
> 
> This series adds a select_pcs() method to the stmmac platform data to
> allow platforms that need to provide their own PCSes to do so, moving
> the decision making into platform code.
> 
> This avoids questions such as "what should the priority of XPCS vs
> some other platform PCS be?" and when we provide a PCS for the
> internal PCS, how that interacts with both the XPCS and platform
> provided PCS.
> 
> [...]

Here is the summary with links:
  - [net-next,v2,1/5] net: stmmac: add select_pcs() platform method
    https://git.kernel.org/netdev/net-next/c/6c3282a6b296
  - [net-next,v2,2/5] net: stmmac: dwmac-intel: provide a select_pcs() implementation
    https://git.kernel.org/netdev/net-next/c/135553da844c
  - [net-next,v2,3/5] net: stmmac: dwmac-rzn1: provide select_pcs() implementation
    https://git.kernel.org/netdev/net-next/c/804c9866e078
  - [net-next,v2,4/5] net: stmmac: dwmac-socfpga: provide select_pcs() implementation
    https://git.kernel.org/netdev/net-next/c/98a6d9f192d3
  - [net-next,v2,5/5] net: stmmac: clean up stmmac_mac_select_pcs()
    https://git.kernel.org/netdev/net-next/c/93f84152e4ae

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html