Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH net-next 0/2] split phylink PCS operations and add PCS support for dpaa2
@ 2020-03-26 15:14 Russell King - ARM Linux admin
  2020-03-26 15:15 ` [PATCH net-next 1/2] net: phylink: rename 'ops' to 'mac_ops' Russell King
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Russell King - ARM Linux admin @ 2020-03-26 15:14 UTC (permalink / raw)
  To: Andrew Lunn, Florian Fainelli, Heiner Kallweit; +Cc: David S. Miller, netdev

This series splits the phylink_mac_ops structure so that PCS can be
supported separately with their own PCS operations, separating them
from the MAC layer.  This may need adaption later as more users come
along.

 drivers/net/phy/phylink.c | 102 ++++++++++++++++++++++++++++++----------------
 include/linux/phylink.h   |  11 +++++
 2 files changed, 78 insertions(+), 35 deletions(-)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH net-next 1/2] net: phylink: rename 'ops' to 'mac_ops'
  2020-03-26 15:14 [PATCH net-next 0/2] split phylink PCS operations and add PCS support for dpaa2 Russell King - ARM Linux admin
@ 2020-03-26 15:15 ` Russell King
  2020-03-26 15:15 ` [PATCH net-next 2/2] net: phylink: add separate pcs operations structure Russell King
  2020-03-26 15:19 ` [PATCH net-next 0/2] split phylink PCS operations and add PCS support for dpaa2 Russell King - ARM Linux admin
  2 siblings, 0 replies; 7+ messages in thread
From: Russell King @ 2020-03-26 15:15 UTC (permalink / raw)
  To: Andrew Lunn, Florian Fainelli, Heiner Kallweit; +Cc: David S. Miller, netdev

Rename the bland 'ops' member of struct phylink to be a more
descriptive 'mac_ops' - this is necessary as we're about to introduce
another set of operations.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index fed0c5907c6a..a34a3be92dba 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -40,7 +40,7 @@ enum {
 struct phylink {
 	/* private: */
 	struct net_device *netdev;
-	const struct phylink_mac_ops *ops;
+	const struct phylink_mac_ops *mac_ops;
 	struct phylink_config *config;
 	struct device *dev;
 	unsigned int old_link_state:1;
@@ -154,7 +154,7 @@ static const char *phylink_an_mode_str(unsigned int mode)
 static int phylink_validate(struct phylink *pl, unsigned long *supported,
 			    struct phylink_link_state *state)
 {
-	pl->ops->validate(pl->config, supported, state);
+	pl->mac_ops->validate(pl->config, supported, state);
 
 	return phylink_is_empty_linkmode(supported) ? -EINVAL : 0;
 }
@@ -415,7 +415,7 @@ static void phylink_mac_config(struct phylink *pl,
 		    __ETHTOOL_LINK_MODE_MASK_NBITS, state->advertising,
 		    state->pause, state->link, state->an_enabled);
 
-	pl->ops->mac_config(pl->config, pl->cur_link_an_mode, state);
+	pl->mac_ops->mac_config(pl->config, pl->cur_link_an_mode, state);
 }
 
 static void phylink_mac_config_up(struct phylink *pl,
@@ -429,7 +429,7 @@ static void phylink_mac_an_restart(struct phylink *pl)
 {
 	if (pl->link_config.an_enabled &&
 	    phy_interface_mode_is_8023z(pl->link_config.interface))
-		pl->ops->mac_an_restart(pl->config);
+		pl->mac_ops->mac_an_restart(pl->config);
 }
 
 static void phylink_mac_pcs_get_state(struct phylink *pl,
@@ -445,7 +445,7 @@ static void phylink_mac_pcs_get_state(struct phylink *pl,
 	state->an_complete = 0;
 	state->link = 1;
 
-	pl->ops->mac_pcs_get_state(pl->config, state);
+	pl->mac_ops->mac_pcs_get_state(pl->config, state);
 }
 
 /* The fixed state is... fixed except for the link state,
@@ -512,11 +512,11 @@ static void phylink_mac_link_up(struct phylink *pl,
 	struct net_device *ndev = pl->netdev;
 
 	pl->cur_interface = link_state.interface;
-	pl->ops->mac_link_up(pl->config, pl->phydev,
-			     pl->cur_link_an_mode, pl->cur_interface,
-			     link_state.speed, link_state.duplex,
-			     !!(link_state.pause & MLO_PAUSE_TX),
-			     !!(link_state.pause & MLO_PAUSE_RX));
+	pl->mac_ops->mac_link_up(pl->config, pl->phydev,
+				 pl->cur_link_an_mode, pl->cur_interface,
+				 link_state.speed, link_state.duplex,
+				 !!(link_state.pause & MLO_PAUSE_TX),
+				 !!(link_state.pause & MLO_PAUSE_RX));
 
 	if (ndev)
 		netif_carrier_on(ndev);
@@ -534,8 +534,8 @@ static void phylink_mac_link_down(struct phylink *pl)
 
 	if (ndev)
 		netif_carrier_off(ndev);
-	pl->ops->mac_link_down(pl->config, pl->cur_link_an_mode,
-			       pl->cur_interface);
+	pl->mac_ops->mac_link_down(pl->config, pl->cur_link_an_mode,
+				   pl->cur_interface);
 	phylink_info(pl, "Link is Down\n");
 }
 
@@ -666,7 +666,7 @@ static int phylink_register_sfp(struct phylink *pl,
  * @fwnode: a pointer to a &struct fwnode_handle describing the network
  *	interface
  * @iface: the desired link mode defined by &typedef phy_interface_t
- * @ops: a pointer to a &struct phylink_mac_ops for the MAC.
+ * @mac_ops: a pointer to a &struct phylink_mac_ops for the MAC.
  *
  * Create a new phylink instance, and parse the link parameters found in @np.
  * This will parse in-band modes, fixed-link or SFP configuration.
@@ -679,7 +679,7 @@ static int phylink_register_sfp(struct phylink *pl,
 struct phylink *phylink_create(struct phylink_config *config,
 			       struct fwnode_handle *fwnode,
 			       phy_interface_t iface,
-			       const struct phylink_mac_ops *ops)
+			       const struct phylink_mac_ops *mac_ops)
 {
 	struct phylink *pl;
 	int ret;
@@ -712,7 +712,7 @@ struct phylink *phylink_create(struct phylink_config *config,
 	pl->link_config.speed = SPEED_UNKNOWN;
 	pl->link_config.duplex = DUPLEX_UNKNOWN;
 	pl->link_config.an_enabled = true;
-	pl->ops = ops;
+	pl->mac_ops = mac_ops;
 	__set_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state);
 	timer_setup(&pl->link_poll, phylink_fixed_poll, 0);
 
-- 
2.20.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH net-next 2/2] net: phylink: add separate pcs operations structure
  2020-03-26 15:14 [PATCH net-next 0/2] split phylink PCS operations and add PCS support for dpaa2 Russell King - ARM Linux admin
  2020-03-26 15:15 ` [PATCH net-next 1/2] net: phylink: rename 'ops' to 'mac_ops' Russell King
@ 2020-03-26 15:15 ` Russell King
  2020-03-26 21:49   ` Ioana Ciornei
  2020-03-26 15:19 ` [PATCH net-next 0/2] split phylink PCS operations and add PCS support for dpaa2 Russell King - ARM Linux admin
  2 siblings, 1 reply; 7+ messages in thread
From: Russell King @ 2020-03-26 15:15 UTC (permalink / raw)
  To: Andrew Lunn, Florian Fainelli, Heiner Kallweit; +Cc: David S. Miller, netdev

Add a separate set of PCS operations, which MAC drivers can use to
couple phylink with their associated MAC PCS layer.  The PCS
operations include:

- pcs_get_state() - reads the link up/down, resolved speed, duplex
   and pause from the PCS.
- pcs_config() - configures the PCS for the specified mode, PHY
   interface type, and setting the advertisement.
- pcs_an_restart() - restarts 802.3 in-band negotiation with the
   link partner
- pcs_link_up() - informs the PCS that link has come up, and the
   parameters of the link. Link parameters are used to program the
   PCS for fixed speed and non-inband modes.

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

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index a34a3be92dba..abe2cc168f93 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -41,6 +41,7 @@ struct phylink {
 	/* private: */
 	struct net_device *netdev;
 	const struct phylink_mac_ops *mac_ops;
+	const struct phylink_pcs_ops *pcs_ops;
 	struct phylink_config *config;
 	struct device *dev;
 	unsigned int old_link_state:1;
@@ -425,11 +426,31 @@ static void phylink_mac_config_up(struct phylink *pl,
 		phylink_mac_config(pl, state);
 }
 
-static void phylink_mac_an_restart(struct phylink *pl)
+static void phylink_mac_pcs_an_restart(struct phylink *pl)
 {
 	if (pl->link_config.an_enabled &&
-	    phy_interface_mode_is_8023z(pl->link_config.interface))
-		pl->mac_ops->mac_an_restart(pl->config);
+	    phy_interface_mode_is_8023z(pl->link_config.interface)) {
+		if (pl->pcs_ops)
+			pl->pcs_ops->pcs_an_restart(pl->config);
+		else
+			pl->mac_ops->mac_an_restart(pl->config);
+	}
+}
+
+static void phylink_pcs_config(struct phylink *pl, bool force_restart,
+			       const struct phylink_link_state *state)
+{
+	bool restart = force_restart;
+
+	if (pl->pcs_ops && pl->pcs_ops->pcs_config(pl->config,
+						   pl->cur_link_an_mode,
+						   state))
+		restart = true;
+
+	phylink_mac_config(pl, state);
+
+	if (restart)
+		phylink_mac_pcs_an_restart(pl);
 }
 
 static void phylink_mac_pcs_get_state(struct phylink *pl,
@@ -445,7 +466,10 @@ static void phylink_mac_pcs_get_state(struct phylink *pl,
 	state->an_complete = 0;
 	state->link = 1;
 
-	pl->mac_ops->mac_pcs_get_state(pl->config, state);
+	if (pl->pcs_ops)
+		pl->pcs_ops->pcs_get_state(pl->config, state);
+	else
+		pl->mac_ops->mac_pcs_get_state(pl->config, state);
 }
 
 /* The fixed state is... fixed except for the link state,
@@ -463,7 +487,7 @@ static void phylink_get_fixed_state(struct phylink *pl,
 	phylink_resolve_flow(state);
 }
 
-static void phylink_mac_initial_config(struct phylink *pl)
+static void phylink_mac_initial_config(struct phylink *pl, bool force_restart)
 {
 	struct phylink_link_state link_state;
 
@@ -489,7 +513,7 @@ static void phylink_mac_initial_config(struct phylink *pl)
 	link_state.link = false;
 
 	phylink_apply_manual_flow(pl, &link_state);
-	phylink_mac_config(pl, &link_state);
+	phylink_pcs_config(pl, force_restart, &link_state);
 }
 
 static const char *phylink_pause_to_str(int pause)
@@ -506,12 +530,18 @@ static const char *phylink_pause_to_str(int pause)
 	}
 }
 
-static void phylink_mac_link_up(struct phylink *pl,
-				struct phylink_link_state link_state)
+static void phylink_link_up(struct phylink *pl,
+			    struct phylink_link_state link_state)
 {
 	struct net_device *ndev = pl->netdev;
 
 	pl->cur_interface = link_state.interface;
+
+	if (pl->pcs_ops && pl->pcs_ops->pcs_link_up)
+		pl->pcs_ops->pcs_link_up(pl->config, pl->cur_link_an_mode,
+					 pl->cur_interface,
+					 link_state.speed, link_state.duplex);
+
 	pl->mac_ops->mac_link_up(pl->config, pl->phydev,
 				 pl->cur_link_an_mode, pl->cur_interface,
 				 link_state.speed, link_state.duplex,
@@ -528,7 +558,7 @@ static void phylink_mac_link_up(struct phylink *pl,
 		     phylink_pause_to_str(link_state.pause));
 }
 
-static void phylink_mac_link_down(struct phylink *pl)
+static void phylink_link_down(struct phylink *pl)
 {
 	struct net_device *ndev = pl->netdev;
 
@@ -597,9 +627,9 @@ static void phylink_resolve(struct work_struct *w)
 	if (link_changed) {
 		pl->old_link_state = link_state.link;
 		if (!link_state.link)
-			phylink_mac_link_down(pl);
+			phylink_link_down(pl);
 		else
-			phylink_mac_link_up(pl, link_state);
+			phylink_link_up(pl, link_state);
 	}
 	if (!link_state.link && pl->mac_link_dropped) {
 		pl->mac_link_dropped = false;
@@ -746,6 +776,12 @@ struct phylink *phylink_create(struct phylink_config *config,
 }
 EXPORT_SYMBOL_GPL(phylink_create);
 
+void phylink_add_pcs(struct phylink *pl, const struct phylink_pcs_ops *ops)
+{
+	pl->pcs_ops = ops;
+}
+EXPORT_SYMBOL_GPL(phylink_add_pcs);
+
 /**
  * phylink_destroy() - cleanup and destroy the phylink instance
  * @pl: a pointer to a &struct phylink returned from phylink_create()
@@ -1082,14 +1118,12 @@ void phylink_start(struct phylink *pl)
 	/* Apply the link configuration to the MAC when starting. This allows
 	 * a fixed-link to start with the correct parameters, and also
 	 * ensures that we set the appropriate advertisement for Serdes links.
-	 */
-	phylink_mac_initial_config(pl);
-
-	/* Restart autonegotiation if using 802.3z to ensure that the link
+	 *
+	 * Restart autonegotiation if using 802.3z to ensure that the link
 	 * parameters are properly negotiated.  This is necessary for DSA
 	 * switches using 802.3z negotiation to ensure they see our modes.
 	 */
-	phylink_mac_an_restart(pl);
+	phylink_mac_initial_config(pl, true);
 
 	clear_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state);
 	phylink_run_resolve(pl);
@@ -1386,8 +1420,7 @@ int phylink_ethtool_ksettings_set(struct phylink *pl,
 			 * advertisement; the only thing we have is the pause
 			 * modes which can only come from a PHY.
 			 */
-			phylink_mac_config(pl, &pl->link_config);
-			phylink_mac_an_restart(pl);
+			phylink_pcs_config(pl, true, &pl->link_config);
 		}
 		mutex_unlock(&pl->state_mutex);
 	}
@@ -1415,7 +1448,7 @@ int phylink_ethtool_nway_reset(struct phylink *pl)
 
 	if (pl->phydev)
 		ret = phy_restart_aneg(pl->phydev);
-	phylink_mac_an_restart(pl);
+	phylink_mac_pcs_an_restart(pl);
 
 	return ret;
 }
@@ -1494,8 +1527,7 @@ int phylink_ethtool_set_pauseparam(struct phylink *pl,
 				   pause->tx_pause);
 	} else if (!test_bit(PHYLINK_DISABLE_STOPPED,
 			     &pl->phylink_disable_state)) {
-		phylink_mac_config(pl, &pl->link_config);
-		phylink_mac_an_restart(pl);
+		phylink_pcs_config(pl, true, &pl->link_config);
 	}
 	mutex_unlock(&pl->state_mutex);
 
@@ -1901,7 +1933,7 @@ static int phylink_sfp_config(struct phylink *pl, u8 mode,
 
 	if (changed && !test_bit(PHYLINK_DISABLE_STOPPED,
 				 &pl->phylink_disable_state))
-		phylink_mac_initial_config(pl);
+		phylink_mac_initial_config(pl, false);
 
 	return ret;
 }
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 8fa6df3b881b..dc27dd341ebd 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -97,6 +97,16 @@ struct phylink_mac_ops {
 			    bool tx_pause, bool rx_pause);
 };
 
+struct phylink_pcs_ops {
+	void (*pcs_get_state)(struct phylink_config *config,
+			      struct phylink_link_state *state);
+	int (*pcs_config)(struct phylink_config *config, unsigned int mode,
+			  const struct phylink_link_state *state);
+	void (*pcs_an_restart)(struct phylink_config *config);
+	void (*pcs_link_up)(struct phylink_config *config, unsigned int mode,
+			    phy_interface_t interface, int speed, int duplex);
+};
+
 #if 0 /* For kernel-doc purposes only. */
 /**
  * validate - Validate and update the link configuration
@@ -273,6 +283,7 @@ void mac_link_up(struct phylink_config *config, struct phy_device *phy,
 struct phylink *phylink_create(struct phylink_config *, struct fwnode_handle *,
 			       phy_interface_t iface,
 			       const struct phylink_mac_ops *ops);
+void phylink_add_pcs(struct phylink *, const struct phylink_pcs_ops *ops);
 void phylink_destroy(struct phylink *);
 
 int phylink_connect_phy(struct phylink *, struct phy_device *);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next 0/2] split phylink PCS operations and add PCS support for dpaa2
  2020-03-26 15:14 [PATCH net-next 0/2] split phylink PCS operations and add PCS support for dpaa2 Russell King - ARM Linux admin
  2020-03-26 15:15 ` [PATCH net-next 1/2] net: phylink: rename 'ops' to 'mac_ops' Russell King
  2020-03-26 15:15 ` [PATCH net-next 2/2] net: phylink: add separate pcs operations structure Russell King
@ 2020-03-26 15:19 ` Russell King - ARM Linux admin
  2 siblings, 0 replies; 7+ messages in thread
From: Russell King - ARM Linux admin @ 2020-03-26 15:19 UTC (permalink / raw)
  To: Andrew Lunn, Florian Fainelli, Heiner Kallweit; +Cc: David S. Miller, netdev

On Thu, Mar 26, 2020 at 03:14:58PM +0000, Russell King - ARM Linux admin wrote:
> This series splits the phylink_mac_ops structure so that PCS can be
> supported separately with their own PCS operations, separating them
> from the MAC layer.  This may need adaption later as more users come
> along.

I see I've messed up the subject line on this email, does it matter, or
shall I resend again?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH net-next 2/2] net: phylink: add separate pcs operations structure
  2020-03-26 15:15 ` [PATCH net-next 2/2] net: phylink: add separate pcs operations structure Russell King
@ 2020-03-26 21:49   ` Ioana Ciornei
  2020-03-29 11:47     ` Russell King - ARM Linux admin
  0 siblings, 1 reply; 7+ messages in thread
From: Ioana Ciornei @ 2020-03-26 21:49 UTC (permalink / raw)
  To: Russell King, Andrew Lunn, Florian Fainelli, Heiner Kallweit
  Cc: David S. Miller, netdev

> Subject: [PATCH net-next 2/2] net: phylink: add separate pcs operations
> structure
> 
> Add a separate set of PCS operations, which MAC drivers can use to couple
> phylink with their associated MAC PCS layer.  The PCS operations include:
> 
> - pcs_get_state() - reads the link up/down, resolved speed, duplex
>    and pause from the PCS.
> - pcs_config() - configures the PCS for the specified mode, PHY
>    interface type, and setting the advertisement.
> - pcs_an_restart() - restarts 802.3 in-band negotiation with the
>    link partner
> - pcs_link_up() - informs the PCS that link has come up, and the
>    parameters of the link. Link parameters are used to program the
>    PCS for fixed speed and non-inband modes.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---

Are the old mac ops going to be removed at some point (after the drivers have been converted to the PCS operations)?
I am referring to mac_pcs_get_state() and mac_an_restart().

Also, what are the rules for what should and shouldn't be done in the new pcs_config() method?
Maybe a documentation entry for this would help.

>  drivers/net/phy/phylink.c | 76 +++++++++++++++++++++++++++------------
>  include/linux/phylink.h   | 11 ++++++
>  2 files changed, 65 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index
> a34a3be92dba..abe2cc168f93 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -41,6 +41,7 @@ struct phylink {
>  	/* private: */
>  	struct net_device *netdev;
>  	const struct phylink_mac_ops *mac_ops;
> +	const struct phylink_pcs_ops *pcs_ops;
>  	struct phylink_config *config;
>  	struct device *dev;
>  	unsigned int old_link_state:1;
> @@ -425,11 +426,31 @@ static void phylink_mac_config_up(struct phylink *pl,
>  		phylink_mac_config(pl, state);
>  }
> 
> -static void phylink_mac_an_restart(struct phylink *pl)
> +static void phylink_mac_pcs_an_restart(struct phylink *pl)
>  {
>  	if (pl->link_config.an_enabled &&
> -	    phy_interface_mode_is_8023z(pl->link_config.interface))
> -		pl->mac_ops->mac_an_restart(pl->config);
> +	    phy_interface_mode_is_8023z(pl->link_config.interface)) {
> +		if (pl->pcs_ops)
> +			pl->pcs_ops->pcs_an_restart(pl->config);
> +		else
> +			pl->mac_ops->mac_an_restart(pl->config);
> +	}
> +}
> +
> +static void phylink_pcs_config(struct phylink *pl, bool force_restart,
> +			       const struct phylink_link_state *state) {
> +	bool restart = force_restart;
> +
> +	if (pl->pcs_ops && pl->pcs_ops->pcs_config(pl->config,
> +						   pl->cur_link_an_mode,
> +						   state))
> +		restart = true;
> +
> +	phylink_mac_config(pl, state);
> +
> +	if (restart)
> +		phylink_mac_pcs_an_restart(pl);
>  }
> 
>  static void phylink_mac_pcs_get_state(struct phylink *pl, @@ -445,7 +466,10
> @@ static void phylink_mac_pcs_get_state(struct phylink *pl,
>  	state->an_complete = 0;
>  	state->link = 1;
> 
> -	pl->mac_ops->mac_pcs_get_state(pl->config, state);
> +	if (pl->pcs_ops)
> +		pl->pcs_ops->pcs_get_state(pl->config, state);
> +	else
> +		pl->mac_ops->mac_pcs_get_state(pl->config, state);
>  }
> 
>  /* The fixed state is... fixed except for the link state, @@ -463,7 +487,7 @@
> static void phylink_get_fixed_state(struct phylink *pl,
>  	phylink_resolve_flow(state);
>  }
> 
> -static void phylink_mac_initial_config(struct phylink *pl)
> +static void phylink_mac_initial_config(struct phylink *pl, bool
> +force_restart)
>  {
>  	struct phylink_link_state link_state;
> 
> @@ -489,7 +513,7 @@ static void phylink_mac_initial_config(struct phylink *pl)
>  	link_state.link = false;
> 
>  	phylink_apply_manual_flow(pl, &link_state);
> -	phylink_mac_config(pl, &link_state);
> +	phylink_pcs_config(pl, force_restart, &link_state);
>  }
> 
>  static const char *phylink_pause_to_str(int pause) @@ -506,12 +530,18 @@
> static const char *phylink_pause_to_str(int pause)
>  	}
>  }
> 
> -static void phylink_mac_link_up(struct phylink *pl,
> -				struct phylink_link_state link_state)
> +static void phylink_link_up(struct phylink *pl,
> +			    struct phylink_link_state link_state)
>  {
>  	struct net_device *ndev = pl->netdev;
> 
>  	pl->cur_interface = link_state.interface;
> +
> +	if (pl->pcs_ops && pl->pcs_ops->pcs_link_up)
> +		pl->pcs_ops->pcs_link_up(pl->config, pl->cur_link_an_mode,
> +					 pl->cur_interface,
> +					 link_state.speed, link_state.duplex);
> +
>  	pl->mac_ops->mac_link_up(pl->config, pl->phydev,
>  				 pl->cur_link_an_mode, pl->cur_interface,
>  				 link_state.speed, link_state.duplex, @@ -528,7
> +558,7 @@ static void phylink_mac_link_up(struct phylink *pl,
>  		     phylink_pause_to_str(link_state.pause));
>  }
> 
> -static void phylink_mac_link_down(struct phylink *pl)
> +static void phylink_link_down(struct phylink *pl)
>  {
>  	struct net_device *ndev = pl->netdev;
> 
> @@ -597,9 +627,9 @@ static void phylink_resolve(struct work_struct *w)
>  	if (link_changed) {
>  		pl->old_link_state = link_state.link;
>  		if (!link_state.link)
> -			phylink_mac_link_down(pl);
> +			phylink_link_down(pl);
>  		else
> -			phylink_mac_link_up(pl, link_state);
> +			phylink_link_up(pl, link_state);
>  	}
>  	if (!link_state.link && pl->mac_link_dropped) {
>  		pl->mac_link_dropped = false;
> @@ -746,6 +776,12 @@ struct phylink *phylink_create(struct phylink_config
> *config,  }  EXPORT_SYMBOL_GPL(phylink_create);
> 
> +void phylink_add_pcs(struct phylink *pl, const struct phylink_pcs_ops
> +*ops) {
> +	pl->pcs_ops = ops;
> +}
> +EXPORT_SYMBOL_GPL(phylink_add_pcs);
> +
>  /**
>   * phylink_destroy() - cleanup and destroy the phylink instance
>   * @pl: a pointer to a &struct phylink returned from phylink_create() @@ -
> 1082,14 +1118,12 @@ void phylink_start(struct phylink *pl)
>  	/* Apply the link configuration to the MAC when starting. This allows
>  	 * a fixed-link to start with the correct parameters, and also
>  	 * ensures that we set the appropriate advertisement for Serdes links.
> -	 */
> -	phylink_mac_initial_config(pl);
> -
> -	/* Restart autonegotiation if using 802.3z to ensure that the link
> +	 *
> +	 * Restart autonegotiation if using 802.3z to ensure that the link
>  	 * parameters are properly negotiated.  This is necessary for DSA
>  	 * switches using 802.3z negotiation to ensure they see our modes.
>  	 */
> -	phylink_mac_an_restart(pl);
> +	phylink_mac_initial_config(pl, true);
> 
>  	clear_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state);
>  	phylink_run_resolve(pl);
> @@ -1386,8 +1420,7 @@ int phylink_ethtool_ksettings_set(struct phylink *pl,
>  			 * advertisement; the only thing we have is the pause
>  			 * modes which can only come from a PHY.
>  			 */
> -			phylink_mac_config(pl, &pl->link_config);
> -			phylink_mac_an_restart(pl);
> +			phylink_pcs_config(pl, true, &pl->link_config);
>  		}
>  		mutex_unlock(&pl->state_mutex);
>  	}
> @@ -1415,7 +1448,7 @@ int phylink_ethtool_nway_reset(struct phylink *pl)
> 
>  	if (pl->phydev)
>  		ret = phy_restart_aneg(pl->phydev);
> -	phylink_mac_an_restart(pl);
> +	phylink_mac_pcs_an_restart(pl);
> 
>  	return ret;
>  }
> @@ -1494,8 +1527,7 @@ int phylink_ethtool_set_pauseparam(struct phylink
> *pl,
>  				   pause->tx_pause);
>  	} else if (!test_bit(PHYLINK_DISABLE_STOPPED,
>  			     &pl->phylink_disable_state)) {
> -		phylink_mac_config(pl, &pl->link_config);
> -		phylink_mac_an_restart(pl);
> +		phylink_pcs_config(pl, true, &pl->link_config);
>  	}
>  	mutex_unlock(&pl->state_mutex);
> 
> @@ -1901,7 +1933,7 @@ static int phylink_sfp_config(struct phylink *pl, u8
> mode,
> 
>  	if (changed && !test_bit(PHYLINK_DISABLE_STOPPED,
>  				 &pl->phylink_disable_state))
> -		phylink_mac_initial_config(pl);
> +		phylink_mac_initial_config(pl, false);
> 
>  	return ret;
>  }
> diff --git a/include/linux/phylink.h b/include/linux/phylink.h index
> 8fa6df3b881b..dc27dd341ebd 100644
> --- a/include/linux/phylink.h
> +++ b/include/linux/phylink.h
> @@ -97,6 +97,16 @@ struct phylink_mac_ops {
>  			    bool tx_pause, bool rx_pause);
>  };
> 
> +struct phylink_pcs_ops {
> +	void (*pcs_get_state)(struct phylink_config *config,
> +			      struct phylink_link_state *state);
> +	int (*pcs_config)(struct phylink_config *config, unsigned int mode,
> +			  const struct phylink_link_state *state);
> +	void (*pcs_an_restart)(struct phylink_config *config);
> +	void (*pcs_link_up)(struct phylink_config *config, unsigned int mode,
> +			    phy_interface_t interface, int speed, int duplex); };
> +
>  #if 0 /* For kernel-doc purposes only. */
>  /**
>   * validate - Validate and update the link configuration @@ -273,6 +283,7 @@
> void mac_link_up(struct phylink_config *config, struct phy_device *phy,  struct
> phylink *phylink_create(struct phylink_config *, struct fwnode_handle *,
>  			       phy_interface_t iface,
>  			       const struct phylink_mac_ops *ops);
> +void phylink_add_pcs(struct phylink *, const struct phylink_pcs_ops
> +*ops);
>  void phylink_destroy(struct phylink *);
> 
>  int phylink_connect_phy(struct phylink *, struct phy_device *);
> --
> 2.20.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next 2/2] net: phylink: add separate pcs operations structure
  2020-03-26 21:49   ` Ioana Ciornei
@ 2020-03-29 11:47     ` Russell King - ARM Linux admin
  2020-03-29 15:10       ` Andrew Lunn
  0 siblings, 1 reply; 7+ messages in thread
From: Russell King - ARM Linux admin @ 2020-03-29 11:47 UTC (permalink / raw)
  To: Ioana Ciornei
  Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller, netdev

On Thu, Mar 26, 2020 at 09:49:02PM +0000, Ioana Ciornei wrote:
> > Subject: [PATCH net-next 2/2] net: phylink: add separate pcs operations
> > structure
> > 
> > Add a separate set of PCS operations, which MAC drivers can use to couple
> > phylink with their associated MAC PCS layer.  The PCS operations include:
> > 
> > - pcs_get_state() - reads the link up/down, resolved speed, duplex
> >    and pause from the PCS.
> > - pcs_config() - configures the PCS for the specified mode, PHY
> >    interface type, and setting the advertisement.
> > - pcs_an_restart() - restarts 802.3 in-band negotiation with the
> >    link partner
> > - pcs_link_up() - informs the PCS that link has come up, and the
> >    parameters of the link. Link parameters are used to program the
> >    PCS for fixed speed and non-inband modes.
> > 
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> 
> Are the old mac ops going to be removed at some point (after the drivers
> have been converted to the PCS operations)?
> I am referring to mac_pcs_get_state() and mac_an_restart().

If (and only if) it's possible to convert mvneta and mvpp2 sanely.
Splitting the PCS makes total sense, but we have to cope with hardware
where there is no clear demarcation between the PCS and MAC.  DSA also
has issues, because it's written as a layered driver, which does not
lend itself to easy gradual conversion of DSA drivers.

> Also, what are the rules for what should and shouldn't be done in the
> new pcs_config() method?

The pcs_config() method should set the PCS according to the interface
mode (state->interface) and the advertisement (state->advertising).
Nothing else should be used. I suppose I should pass those explicitly
rather than the whole state structure to prevent any mis-use.

> Maybe a documentation entry for this would help.

Yep.

> >  drivers/net/phy/phylink.c | 76 +++++++++++++++++++++++++++------------
> >  include/linux/phylink.h   | 11 ++++++
> >  2 files changed, 65 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index
> > a34a3be92dba..abe2cc168f93 100644
> > --- a/drivers/net/phy/phylink.c
> > +++ b/drivers/net/phy/phylink.c
> > @@ -41,6 +41,7 @@ struct phylink {
> >  	/* private: */
> >  	struct net_device *netdev;
> >  	const struct phylink_mac_ops *mac_ops;
> > +	const struct phylink_pcs_ops *pcs_ops;
> >  	struct phylink_config *config;
> >  	struct device *dev;
> >  	unsigned int old_link_state:1;
> > @@ -425,11 +426,31 @@ static void phylink_mac_config_up(struct phylink *pl,
> >  		phylink_mac_config(pl, state);
> >  }
> > 
> > -static void phylink_mac_an_restart(struct phylink *pl)
> > +static void phylink_mac_pcs_an_restart(struct phylink *pl)
> >  {
> >  	if (pl->link_config.an_enabled &&
> > -	    phy_interface_mode_is_8023z(pl->link_config.interface))
> > -		pl->mac_ops->mac_an_restart(pl->config);
> > +	    phy_interface_mode_is_8023z(pl->link_config.interface)) {
> > +		if (pl->pcs_ops)
> > +			pl->pcs_ops->pcs_an_restart(pl->config);
> > +		else
> > +			pl->mac_ops->mac_an_restart(pl->config);
> > +	}
> > +}
> > +
> > +static void phylink_pcs_config(struct phylink *pl, bool force_restart,
> > +			       const struct phylink_link_state *state) {
> > +	bool restart = force_restart;
> > +
> > +	if (pl->pcs_ops && pl->pcs_ops->pcs_config(pl->config,
> > +						   pl->cur_link_an_mode,
> > +						   state))
> > +		restart = true;
> > +
> > +	phylink_mac_config(pl, state);
> > +
> > +	if (restart)
> > +		phylink_mac_pcs_an_restart(pl);
> >  }
> > 
> >  static void phylink_mac_pcs_get_state(struct phylink *pl, @@ -445,7 +466,10
> > @@ static void phylink_mac_pcs_get_state(struct phylink *pl,
> >  	state->an_complete = 0;
> >  	state->link = 1;
> > 
> > -	pl->mac_ops->mac_pcs_get_state(pl->config, state);
> > +	if (pl->pcs_ops)
> > +		pl->pcs_ops->pcs_get_state(pl->config, state);
> > +	else
> > +		pl->mac_ops->mac_pcs_get_state(pl->config, state);
> >  }
> > 
> >  /* The fixed state is... fixed except for the link state, @@ -463,7 +487,7 @@
> > static void phylink_get_fixed_state(struct phylink *pl,
> >  	phylink_resolve_flow(state);
> >  }
> > 
> > -static void phylink_mac_initial_config(struct phylink *pl)
> > +static void phylink_mac_initial_config(struct phylink *pl, bool
> > +force_restart)
> >  {
> >  	struct phylink_link_state link_state;
> > 
> > @@ -489,7 +513,7 @@ static void phylink_mac_initial_config(struct phylink *pl)
> >  	link_state.link = false;
> > 
> >  	phylink_apply_manual_flow(pl, &link_state);
> > -	phylink_mac_config(pl, &link_state);
> > +	phylink_pcs_config(pl, force_restart, &link_state);
> >  }
> > 
> >  static const char *phylink_pause_to_str(int pause) @@ -506,12 +530,18 @@
> > static const char *phylink_pause_to_str(int pause)
> >  	}
> >  }
> > 
> > -static void phylink_mac_link_up(struct phylink *pl,
> > -				struct phylink_link_state link_state)
> > +static void phylink_link_up(struct phylink *pl,
> > +			    struct phylink_link_state link_state)
> >  {
> >  	struct net_device *ndev = pl->netdev;
> > 
> >  	pl->cur_interface = link_state.interface;
> > +
> > +	if (pl->pcs_ops && pl->pcs_ops->pcs_link_up)
> > +		pl->pcs_ops->pcs_link_up(pl->config, pl->cur_link_an_mode,
> > +					 pl->cur_interface,
> > +					 link_state.speed, link_state.duplex);
> > +
> >  	pl->mac_ops->mac_link_up(pl->config, pl->phydev,
> >  				 pl->cur_link_an_mode, pl->cur_interface,
> >  				 link_state.speed, link_state.duplex, @@ -528,7
> > +558,7 @@ static void phylink_mac_link_up(struct phylink *pl,
> >  		     phylink_pause_to_str(link_state.pause));
> >  }
> > 
> > -static void phylink_mac_link_down(struct phylink *pl)
> > +static void phylink_link_down(struct phylink *pl)
> >  {
> >  	struct net_device *ndev = pl->netdev;
> > 
> > @@ -597,9 +627,9 @@ static void phylink_resolve(struct work_struct *w)
> >  	if (link_changed) {
> >  		pl->old_link_state = link_state.link;
> >  		if (!link_state.link)
> > -			phylink_mac_link_down(pl);
> > +			phylink_link_down(pl);
> >  		else
> > -			phylink_mac_link_up(pl, link_state);
> > +			phylink_link_up(pl, link_state);
> >  	}
> >  	if (!link_state.link && pl->mac_link_dropped) {
> >  		pl->mac_link_dropped = false;
> > @@ -746,6 +776,12 @@ struct phylink *phylink_create(struct phylink_config
> > *config,  }  EXPORT_SYMBOL_GPL(phylink_create);
> > 
> > +void phylink_add_pcs(struct phylink *pl, const struct phylink_pcs_ops
> > +*ops) {
> > +	pl->pcs_ops = ops;
> > +}
> > +EXPORT_SYMBOL_GPL(phylink_add_pcs);
> > +
> >  /**
> >   * phylink_destroy() - cleanup and destroy the phylink instance
> >   * @pl: a pointer to a &struct phylink returned from phylink_create() @@ -
> > 1082,14 +1118,12 @@ void phylink_start(struct phylink *pl)
> >  	/* Apply the link configuration to the MAC when starting. This allows
> >  	 * a fixed-link to start with the correct parameters, and also
> >  	 * ensures that we set the appropriate advertisement for Serdes links.
> > -	 */
> > -	phylink_mac_initial_config(pl);
> > -
> > -	/* Restart autonegotiation if using 802.3z to ensure that the link
> > +	 *
> > +	 * Restart autonegotiation if using 802.3z to ensure that the link
> >  	 * parameters are properly negotiated.  This is necessary for DSA
> >  	 * switches using 802.3z negotiation to ensure they see our modes.
> >  	 */
> > -	phylink_mac_an_restart(pl);
> > +	phylink_mac_initial_config(pl, true);
> > 
> >  	clear_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state);
> >  	phylink_run_resolve(pl);
> > @@ -1386,8 +1420,7 @@ int phylink_ethtool_ksettings_set(struct phylink *pl,
> >  			 * advertisement; the only thing we have is the pause
> >  			 * modes which can only come from a PHY.
> >  			 */
> > -			phylink_mac_config(pl, &pl->link_config);
> > -			phylink_mac_an_restart(pl);
> > +			phylink_pcs_config(pl, true, &pl->link_config);
> >  		}
> >  		mutex_unlock(&pl->state_mutex);
> >  	}
> > @@ -1415,7 +1448,7 @@ int phylink_ethtool_nway_reset(struct phylink *pl)
> > 
> >  	if (pl->phydev)
> >  		ret = phy_restart_aneg(pl->phydev);
> > -	phylink_mac_an_restart(pl);
> > +	phylink_mac_pcs_an_restart(pl);
> > 
> >  	return ret;
> >  }
> > @@ -1494,8 +1527,7 @@ int phylink_ethtool_set_pauseparam(struct phylink
> > *pl,
> >  				   pause->tx_pause);
> >  	} else if (!test_bit(PHYLINK_DISABLE_STOPPED,
> >  			     &pl->phylink_disable_state)) {
> > -		phylink_mac_config(pl, &pl->link_config);
> > -		phylink_mac_an_restart(pl);
> > +		phylink_pcs_config(pl, true, &pl->link_config);
> >  	}
> >  	mutex_unlock(&pl->state_mutex);
> > 
> > @@ -1901,7 +1933,7 @@ static int phylink_sfp_config(struct phylink *pl, u8
> > mode,
> > 
> >  	if (changed && !test_bit(PHYLINK_DISABLE_STOPPED,
> >  				 &pl->phylink_disable_state))
> > -		phylink_mac_initial_config(pl);
> > +		phylink_mac_initial_config(pl, false);
> > 
> >  	return ret;
> >  }
> > diff --git a/include/linux/phylink.h b/include/linux/phylink.h index
> > 8fa6df3b881b..dc27dd341ebd 100644
> > --- a/include/linux/phylink.h
> > +++ b/include/linux/phylink.h
> > @@ -97,6 +97,16 @@ struct phylink_mac_ops {
> >  			    bool tx_pause, bool rx_pause);
> >  };
> > 
> > +struct phylink_pcs_ops {
> > +	void (*pcs_get_state)(struct phylink_config *config,
> > +			      struct phylink_link_state *state);
> > +	int (*pcs_config)(struct phylink_config *config, unsigned int mode,
> > +			  const struct phylink_link_state *state);
> > +	void (*pcs_an_restart)(struct phylink_config *config);
> > +	void (*pcs_link_up)(struct phylink_config *config, unsigned int mode,
> > +			    phy_interface_t interface, int speed, int duplex); };
> > +
> >  #if 0 /* For kernel-doc purposes only. */
> >  /**
> >   * validate - Validate and update the link configuration @@ -273,6 +283,7 @@
> > void mac_link_up(struct phylink_config *config, struct phy_device *phy,  struct
> > phylink *phylink_create(struct phylink_config *, struct fwnode_handle *,
> >  			       phy_interface_t iface,
> >  			       const struct phylink_mac_ops *ops);
> > +void phylink_add_pcs(struct phylink *, const struct phylink_pcs_ops
> > +*ops);
> >  void phylink_destroy(struct phylink *);
> > 
> >  int phylink_connect_phy(struct phylink *, struct phy_device *);
> > --
> > 2.20.1
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next 2/2] net: phylink: add separate pcs operations structure
  2020-03-29 11:47     ` Russell King - ARM Linux admin
@ 2020-03-29 15:10       ` Andrew Lunn
  0 siblings, 0 replies; 7+ messages in thread
From: Andrew Lunn @ 2020-03-29 15:10 UTC (permalink / raw)
  To: Russell King - ARM Linux admin
  Cc: Ioana Ciornei, Florian Fainelli, Heiner Kallweit,
	David S. Miller, netdev

> The pcs_config() method should set the PCS according to the interface
> mode (state->interface) and the advertisement (state->advertising).
> Nothing else should be used. I suppose I should pass those explicitly
> rather than the whole state structure to prevent any mis-use.

Hi Russell

I think that has been a learning from phylink so far. Only pass what
you are supposed to act on, nothing more.

    Andrew

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-26 15:14 [PATCH net-next 0/2] split phylink PCS operations and add PCS support for dpaa2 Russell King - ARM Linux admin
2020-03-26 15:15 ` [PATCH net-next 1/2] net: phylink: rename 'ops' to 'mac_ops' Russell King
2020-03-26 15:15 ` [PATCH net-next 2/2] net: phylink: add separate pcs operations structure Russell King
2020-03-26 21:49   ` Ioana Ciornei
2020-03-29 11:47     ` Russell King - ARM Linux admin
2020-03-29 15:10       ` Andrew Lunn
2020-03-26 15:19 ` [PATCH net-next 0/2] split phylink PCS operations and add PCS support for dpaa2 Russell King - ARM Linux admin

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git