| Message ID | E1sHhoM-00Fesu-8E@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6c3282a6b296385bee2c383442c39f507b0d51dd |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: stmmac: provide platform select_pcs method | expand |
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 >
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.
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;