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

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:

quoted

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) <redacted>
---
 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);

quoted

 	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!