Re: [RFC PATCH v2 net-next 9/9] net: pcs: xpcs: convert to phylink_pcs_ops

[Vladimir Oltean <olteanv@gmail.com>]

On Wed, Jun 02, 2021 at 02:47:49PM +0100, Russell King (Oracle) wrote:
> On Wed, Jun 02, 2021 at 04:43:21PM +0300, Vladimir Oltean wrote:
> > On Tue, Jun 01, 2021 at 01:10:33PM +0100, Russell King (Oracle) wrote:
> > > On Tue, Jun 01, 2021 at 03:33:25AM +0300, Vladimir Oltean wrote:
> > > >  static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
> > > >  	.validate = stmmac_validate,
> > > > -	.mac_pcs_get_state = stmmac_mac_pcs_get_state,
> > > > -	.mac_config = stmmac_mac_config,
> > > 
> > > mac_config is still a required function.
> > 
> > This is correct, thanks.
> > 
> > VK, would you mind testing again with this extra patch added to the mix?
> > If it works, I will add it to the series in v3, ordered properly.
> > 
> > -----------------------------[ cut here]-----------------------------
> > From a79863027998451c73d5bbfaf1b77cf6097a110c Mon Sep 17 00:00:00 2001
> > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > Date: Wed, 2 Jun 2021 16:35:55 +0300
> > Subject: [PATCH] net: phylink: allow the mac_config method to be missing if
> >  pcs_ops are provided
> > 
> > The pcs_config method from struct phylink_pcs_ops does everything that
> > the mac_config method from struct phylink_mac_ops used to do in the
> > legacy approach of driving a MAC PCS. So allow drivers to not implement
> > the mac_config method if there is nothing to do. Keep the method
> > required for setups that do not provide pcs_ops.
> > 
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > ---
> >  drivers/net/phy/phylink.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > index 96d8e88b4e46..a8842c6ce3a2 100644
> > --- a/drivers/net/phy/phylink.c
> > +++ b/drivers/net/phy/phylink.c
> > @@ -415,6 +415,9 @@ static void phylink_resolve_flow(struct phylink_link_state *state)
> >  static void phylink_mac_config(struct phylink *pl,
> >  			       const struct phylink_link_state *state)
> >  {
> > +	if (!pl->mac_ops->mac_config)
> > +		return;
> > +
> >  	phylink_dbg(pl,
> >  		    "%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
> >  		    __func__, phylink_an_mode_str(pl->cur_link_an_mode),
> > @@ -1192,6 +1195,12 @@ void phylink_start(struct phylink *pl)
> >  
> >  	ASSERT_RTNL();
> >  
> > +	/* The mac_ops::mac_config method may be absent only if the
> > +	 * pcs_ops are present.
> > +	 */
> > +	if (WARN_ON_ONCE(!pl->mac_ops->mac_config && !pl->pcs_ops))
> > +		return;
> > +
> >  	phylink_info(pl, "configuring for %s/%s link mode\n",
> >  		     phylink_an_mode_str(pl->cur_link_an_mode),
> >  		     phy_modes(pl->link_config.interface));
> 
> I would rather we didn't do that - I suspect your case is not the
> common scenario, so please add a dummy function to stmmac instead.

Ok, in that case the only delta to be applied should be:

-----------------------------[ cut here]-----------------------------
From 998569108392befc591f790e46fe5dcd1d0b6278 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Wed, 2 Jun 2021 17:00:12 +0300
Subject: [PATCH] fixup! net: pcs: xpcs: convert to phylink_pcs_ops

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index d5685a74f3b7..704aa91b145a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1000,6 +1000,12 @@ static void stmmac_validate(struct phylink_config *config,
 		xpcs_validate(priv->hw->xpcs, supported, state);
 }
 
+static void stmmac_mac_config(struct phylink_config *config, unsigned int mode,
+			      const struct phylink_link_state *state)
+{
+	/* Nothing to do, xpcs_config() handles everything */
+}
+
 static void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up)
 {
 	struct stmmac_fpe_cfg *fpe_cfg = priv->plat->fpe_cfg;
-----------------------------[ cut here]-----------------------------