[PATCH net-next v2 1/3] dpaa2-eth: Remove support for changing link settings

[PATCH net-next v2 1/3] dpaa2-eth: Remove support for changing link settings

[Ioana Radulescu]

We only support fixed-link for now, so there is no point in
offering users the option to change link settings via ethtool.

Functionally there is no change, since firmware prevents us from
changing link parameters anyway.

Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com>
---
v2: new patch

 .../net/ethernet/freescale/dpaa2/dpaa2-ethtool.c   | 51 +---------------------
 1 file changed, 1 insertion(+), 50 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c
index 7b182f4..5c9816b 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c
@@ -88,13 +88,7 @@ dpaa2_eth_get_link_ksettings(struct net_device *net_dev,
 		goto out;
 	}
 
-	/* At the moment, we have no way of interrogating the DPMAC
-	 * from the DPNI side - and for that matter there may exist
-	 * no DPMAC at all. So for now we just don't report anything
-	 * beyond the DPNI attributes.
-	 */
-	if (state.options & DPNI_LINK_OPT_AUTONEG)
-		link_settings->base.autoneg = AUTONEG_ENABLE;
+	link_settings->base.autoneg = AUTONEG_DISABLE;
 	if (!(state.options & DPNI_LINK_OPT_HALF_DUPLEX))
 		link_settings->base.duplex = DUPLEX_FULL;
 	link_settings->base.speed = state.rate;
@@ -103,48 +97,6 @@ dpaa2_eth_get_link_ksettings(struct net_device *net_dev,
 	return err;
 }
 
-#define DPNI_DYNAMIC_LINK_SET_VER_MAJOR		7
-#define DPNI_DYNAMIC_LINK_SET_VER_MINOR		1
-static int
-dpaa2_eth_set_link_ksettings(struct net_device *net_dev,
-			     const struct ethtool_link_ksettings *link_settings)
-{
-	struct dpni_link_cfg cfg = {0};
-	struct dpaa2_eth_priv *priv = netdev_priv(net_dev);
-	int err = 0;
-
-	/* If using an older MC version, the DPNI must be down
-	 * in order to be able to change link settings. Taking steps to let
-	 * the user know that.
-	 */
-	if (dpaa2_eth_cmp_dpni_ver(priv, DPNI_DYNAMIC_LINK_SET_VER_MAJOR,
-				   DPNI_DYNAMIC_LINK_SET_VER_MINOR) < 0) {
-		if (netif_running(net_dev)) {
-			netdev_info(net_dev, "Interface must be brought down first.\n");
-			return -EACCES;
-		}
-	}
-
-	cfg.rate = link_settings->base.speed;
-	if (link_settings->base.autoneg == AUTONEG_ENABLE)
-		cfg.options |= DPNI_LINK_OPT_AUTONEG;
-	else
-		cfg.options &= ~DPNI_LINK_OPT_AUTONEG;
-	if (link_settings->base.duplex  == DUPLEX_HALF)
-		cfg.options |= DPNI_LINK_OPT_HALF_DUPLEX;
-	else
-		cfg.options &= ~DPNI_LINK_OPT_HALF_DUPLEX;
-
-	err = dpni_set_link_cfg(priv->mc_io, 0, priv->mc_token, &cfg);
-	if (err)
-		/* ethtool will be loud enough if we return an error; no point
-		 * in putting our own error message on the console by default
-		 */
-		netdev_dbg(net_dev, "ERROR %d setting link cfg\n", err);
-
-	return err;
-}
-
 static void dpaa2_eth_get_strings(struct net_device *netdev, u32 stringset,
 				  u8 *data)
 {
@@ -721,7 +673,6 @@ const struct ethtool_ops dpaa2_ethtool_ops = {
 	.get_drvinfo = dpaa2_eth_get_drvinfo,
 	.get_link = ethtool_op_get_link,
 	.get_link_ksettings = dpaa2_eth_get_link_ksettings,
-	.set_link_ksettings = dpaa2_eth_set_link_ksettings,
 	.get_sset_count = dpaa2_eth_get_sset_count,
 	.get_ethtool_stats = dpaa2_eth_get_ethtool_stats,
 	.get_strings = dpaa2_eth_get_strings,
-- 
2.7.4

[PATCH net-next v2 2/3] dpaa2-eth: Use stored link settings

[Ioana Radulescu]

Whenever a link state change occurs, we get notified and save
the new link settings in the device's private data. In ethtool
get_link_ksettings, use the stored state instead of interrogating
the firmware each time.

Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com>
---
v2: split from main pause frames patch

 drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c     |  6 ++++--
 drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c | 15 +++------------
 2 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
index 0acb115..2c6f9b1 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
@@ -1222,9 +1222,8 @@ static int link_state_update(struct dpaa2_eth_priv *priv)
 
 	/* Chech link state; speed / duplex changes are not treated yet */
 	if (priv->link_state.up == state.up)
-		return 0;
+		goto out;
 
-	priv->link_state = state;
 	if (state.up) {
 		netif_carrier_on(priv->net_dev);
 		netif_tx_start_all_queues(priv->net_dev);
@@ -1236,6 +1235,9 @@ static int link_state_update(struct dpaa2_eth_priv *priv)
 	netdev_info(priv->net_dev, "Link Event: state %s\n",
 		    state.up ? "up" : "down");
 
+out:
+	priv->link_state = state;
+
 	return 0;
 }
 
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c
index 5c9816b..fb17042 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c
@@ -78,23 +78,14 @@ static int
 dpaa2_eth_get_link_ksettings(struct net_device *net_dev,
 			     struct ethtool_link_ksettings *link_settings)
 {
-	struct dpni_link_state state = {0};
-	int err = 0;
 	struct dpaa2_eth_priv *priv = netdev_priv(net_dev);
 
-	err = dpni_get_link_state(priv->mc_io, 0, priv->mc_token, &state);
-	if (err) {
-		netdev_err(net_dev, "ERROR %d getting link state\n", err);
-		goto out;
-	}
-
 	link_settings->base.autoneg = AUTONEG_DISABLE;
-	if (!(state.options & DPNI_LINK_OPT_HALF_DUPLEX))
+	if (!(priv->link_state.options & DPNI_LINK_OPT_HALF_DUPLEX))
 		link_settings->base.duplex = DUPLEX_FULL;
-	link_settings->base.speed = state.rate;
+	link_settings->base.speed = priv->link_state.rate;
 
-out:
-	return err;
+	return 0;
 }
 
 static void dpaa2_eth_get_strings(struct net_device *netdev, u32 stringset,
-- 
2.7.4

[PATCH net-next v2 3/3] dpaa2-eth: Add pause frame support

[Ioana Radulescu]

Starting with firmware version MC10.18.0, we have support for
L2 flow control. Asymmetrical configuration (Rx or Tx only) is
supported, but not pause frame autonegotioation.

Pause frame configuration is done via ethtool. By default, we start
with flow control enabled on both Rx and Tx. Changes are propagated
to hardware through firmware commands.

The hardware can automatically send pause frames when the number
of buffers in the pool goes below a predefined threshold. Due to
this, flow control is incompatible with Rx frame queue taildrop
(both mechanisms target the case when processing of ingress
frames can't keep up with the Rx rate; for large frames, the number
of buffers in the pool may never get low enough to trigger pause
frames as long as taildrop is enabled). So we set pause frame
generation and Rx FQ taildrop as mutually exclusive.

Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com>
---
v2: split priv->link_state changes in a separate patch
    always set pause->autoneg to false
    return -EOPNOTSUPP if user tries to set pause->autoneg

 drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c   | 79 +++++++++++++++++++---
 drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h   |  7 ++
 .../net/ethernet/freescale/dpaa2/dpaa2-ethtool.c   | 55 +++++++++++++++
 drivers/net/ethernet/freescale/dpaa2/dpni-cmd.h    |  3 +-
 drivers/net/ethernet/freescale/dpaa2/dpni.c        | 40 ++++++++++-
 drivers/net/ethernet/freescale/dpaa2/dpni.h        |  5 ++
 6 files changed, 176 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
index 2c6f9b1..e0816d6 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
@@ -1208,9 +1208,37 @@ static void disable_ch_napi(struct dpaa2_eth_priv *priv)
 	}
 }
 
+static void dpaa2_eth_set_rx_taildrop(struct dpaa2_eth_priv *priv, bool enable)
+{
+	struct dpni_taildrop td = {0};
+	int i, err;
+
+	if (priv->rx_td_enabled == enable)
+		return;
+
+	td.enable = enable;
+	td.threshold = DPAA2_ETH_TAILDROP_THRESH;
+
+	for (i = 0; i < priv->num_fqs; i++) {
+		if (priv->fq[i].type != DPAA2_RX_FQ)
+			continue;
+		err = dpni_set_taildrop(priv->mc_io, 0, priv->mc_token,
+					DPNI_CP_QUEUE, DPNI_QUEUE_RX, 0,
+					priv->fq[i].flowid, &td);
+		if (err) {
+			netdev_err(priv->net_dev,
+				   "dpni_set_taildrop() failed\n");
+			break;
+		}
+	}
+
+	priv->rx_td_enabled = enable;
+}
+
 static int link_state_update(struct dpaa2_eth_priv *priv)
 {
 	struct dpni_link_state state = {0};
+	bool tx_pause;
 	int err;
 
 	err = dpni_get_link_state(priv->mc_io, 0, priv->mc_token, &state);
@@ -1220,6 +1248,14 @@ static int link_state_update(struct dpaa2_eth_priv *priv)
 		return err;
 	}
 
+	/* If Tx pause frame settings have changed, we need to update
+	 * Rx FQ taildrop configuration as well. We configure taildrop
+	 * only when pause frame generation is disabled.
+	 */
+	tx_pause = !!(state.options & DPNI_LINK_OPT_PAUSE) ^
+		   !!(state.options & DPNI_LINK_OPT_ASYM_PAUSE);
+	dpaa2_eth_set_rx_taildrop(priv, !tx_pause);
+
 	/* Chech link state; speed / duplex changes are not treated yet */
 	if (priv->link_state.up == state.up)
 		goto out;
@@ -2445,6 +2481,32 @@ static void set_enqueue_mode(struct dpaa2_eth_priv *priv)
 		priv->enqueue = dpaa2_eth_enqueue_fq;
 }
 
+static int set_pause(struct dpaa2_eth_priv *priv)
+{
+	struct device *dev = priv->net_dev->dev.parent;
+	struct dpni_link_cfg link_cfg = {0};
+	int err;
+
+	/* Get the default link options so we don't override other flags */
+	err = dpni_get_link_cfg(priv->mc_io, 0, priv->mc_token, &link_cfg);
+	if (err) {
+		dev_err(dev, "dpni_get_link_cfg() failed\n");
+		return err;
+	}
+
+	link_cfg.options |= DPNI_LINK_OPT_PAUSE;
+	link_cfg.options &= ~DPNI_LINK_OPT_ASYM_PAUSE;
+	err = dpni_set_link_cfg(priv->mc_io, 0, priv->mc_token, &link_cfg);
+	if (err) {
+		dev_err(dev, "dpni_set_link_cfg() failed\n");
+		return err;
+	}
+
+	priv->link_state.options = link_cfg.options;
+
+	return 0;
+}
+
 /* Configure the DPNI object this interface is associated with */
 static int setup_dpni(struct fsl_mc_device *ls_dev)
 {
@@ -2500,6 +2562,13 @@ static int setup_dpni(struct fsl_mc_device *ls_dev)
 
 	set_enqueue_mode(priv);
 
+	/* Enable pause frame support */
+	if (dpaa2_eth_has_pause_support(priv)) {
+		err = set_pause(priv);
+		if (err)
+			goto close;
+	}
+
 	priv->cls_rules = devm_kzalloc(dev, sizeof(struct dpaa2_eth_cls_rule) *
 				       dpaa2_eth_fs_count(priv), GFP_KERNEL);
 	if (!priv->cls_rules)
@@ -2531,7 +2600,6 @@ static int setup_rx_flow(struct dpaa2_eth_priv *priv,
 	struct device *dev = priv->net_dev->dev.parent;
 	struct dpni_queue queue;
 	struct dpni_queue_id qid;
-	struct dpni_taildrop td;
 	int err;
 
 	err = dpni_get_queue(priv->mc_io, 0, priv->mc_token,
@@ -2556,15 +2624,6 @@ static int setup_rx_flow(struct dpaa2_eth_priv *priv,
 		return err;
 	}
 
-	td.enable = 1;
-	td.threshold = DPAA2_ETH_TAILDROP_THRESH;
-	err = dpni_set_taildrop(priv->mc_io, 0, priv->mc_token, DPNI_CP_QUEUE,
-				DPNI_QUEUE_RX, 0, fq->flowid, &td);
-	if (err) {
-		dev_err(dev, "dpni_set_threshold() failed\n");
-		return err;
-	}
-
 	/* xdp_rxq setup */
 	err = xdp_rxq_info_reg(&fq->channel->xdp_rxq, priv->net_dev,
 			       fq->flowid);
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h
index 9af18c2..8a0e65b 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h
@@ -392,6 +392,7 @@ struct dpaa2_eth_priv {
 	struct dpaa2_eth_drv_stats __percpu *percpu_extras;
 
 	u16 mc_token;
+	u8 rx_td_enabled;
 
 	struct dpni_link_state link_state;
 	bool do_link_poll;
@@ -476,6 +477,12 @@ enum dpaa2_eth_rx_dist {
 #define DPAA2_ETH_DIST_L4DST		BIT(8)
 #define DPAA2_ETH_DIST_ALL		(~0ULL)
 
+#define DPNI_PAUSE_VER_MAJOR		7
+#define DPNI_PAUSE_VER_MINOR		13
+#define dpaa2_eth_has_pause_support(priv)			\
+	(dpaa2_eth_cmp_dpni_ver((priv), DPNI_PAUSE_VER_MAJOR,	\
+				DPNI_PAUSE_VER_MINOR) >= 0)
+
 static inline
 unsigned int dpaa2_eth_needed_headroom(struct dpaa2_eth_priv *priv,
 				       struct sk_buff *skb)
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c
index fb17042..93076fe 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c
@@ -88,6 +88,59 @@ dpaa2_eth_get_link_ksettings(struct net_device *net_dev,
 	return 0;
 }
 
+static void dpaa2_eth_get_pauseparam(struct net_device *net_dev,
+				     struct ethtool_pauseparam *pause)
+{
+	struct dpaa2_eth_priv *priv = netdev_priv(net_dev);
+	u64 link_options = priv->link_state.options;
+
+	pause->rx_pause = !!(link_options & DPNI_LINK_OPT_PAUSE);
+	pause->tx_pause = pause->rx_pause ^
+			  !!(link_options & DPNI_LINK_OPT_ASYM_PAUSE);
+	pause->autoneg = AUTONEG_DISABLE;
+}
+
+static int dpaa2_eth_set_pauseparam(struct net_device *net_dev,
+				    struct ethtool_pauseparam *pause)
+{
+	struct dpaa2_eth_priv *priv = netdev_priv(net_dev);
+	struct dpni_link_cfg cfg = {0};
+	int err;
+
+	if (!dpaa2_eth_has_pause_support(priv)) {
+		netdev_info(net_dev, "No pause frame support for DPNI version < %d.%d\n",
+			    DPNI_PAUSE_VER_MAJOR, DPNI_PAUSE_VER_MINOR);
+		return -EOPNOTSUPP;
+	}
+
+	if (pause->autoneg)
+		return -EOPNOTSUPP;
+
+	cfg.rate = priv->link_state.rate;
+	cfg.options = priv->link_state.options;
+	if (pause->rx_pause)
+		cfg.options |= DPNI_LINK_OPT_PAUSE;
+	else
+		cfg.options &= ~DPNI_LINK_OPT_PAUSE;
+	if (!!pause->rx_pause ^ !!pause->tx_pause)
+		cfg.options |= DPNI_LINK_OPT_ASYM_PAUSE;
+	else
+		cfg.options &= ~DPNI_LINK_OPT_ASYM_PAUSE;
+
+	if (cfg.options == priv->link_state.options)
+		return 0;
+
+	err = dpni_set_link_cfg(priv->mc_io, 0, priv->mc_token, &cfg);
+	if (err) {
+		netdev_err(net_dev, "dpni_set_link_state failed\n");
+		return err;
+	}
+
+	priv->link_state.options = cfg.options;
+
+	return 0;
+}
+
 static void dpaa2_eth_get_strings(struct net_device *netdev, u32 stringset,
 				  u8 *data)
 {
@@ -664,6 +717,8 @@ const struct ethtool_ops dpaa2_ethtool_ops = {
 	.get_drvinfo = dpaa2_eth_get_drvinfo,
 	.get_link = ethtool_op_get_link,
 	.get_link_ksettings = dpaa2_eth_get_link_ksettings,
+	.get_pauseparam = dpaa2_eth_get_pauseparam,
+	.set_pauseparam = dpaa2_eth_set_pauseparam,
 	.get_sset_count = dpaa2_eth_get_sset_count,
 	.get_ethtool_stats = dpaa2_eth_get_ethtool_stats,
 	.get_strings = dpaa2_eth_get_strings,
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpni-cmd.h b/drivers/net/ethernet/freescale/dpaa2/dpni-cmd.h
index 7b44d7d..d9b6918 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpni-cmd.h
+++ b/drivers/net/ethernet/freescale/dpaa2/dpni-cmd.h
@@ -84,6 +84,7 @@
 
 #define DPNI_CMDID_SET_RX_FS_DIST			DPNI_CMD(0x273)
 #define DPNI_CMDID_SET_RX_HASH_DIST			DPNI_CMD(0x274)
+#define DPNI_CMDID_GET_LINK_CFG				DPNI_CMD(0x278)
 
 /* Macros for accessing command fields smaller than 1byte */
 #define DPNI_MASK(field)	\
@@ -284,7 +285,7 @@ struct dpni_rsp_get_statistics {
 	__le64 counter[DPNI_STATISTICS_CNT];
 };
 
-struct dpni_cmd_set_link_cfg {
+struct dpni_cmd_link_cfg {
 	/* cmd word 0 */
 	__le64 pad0;
 	/* cmd word 1 */
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpni.c b/drivers/net/ethernet/freescale/dpaa2/dpni.c
index 220dfc8..05e3089 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpni.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpni.c
@@ -838,13 +838,13 @@ int dpni_set_link_cfg(struct fsl_mc_io *mc_io,
 		      const struct dpni_link_cfg *cfg)
 {
 	struct fsl_mc_command cmd = { 0 };
-	struct dpni_cmd_set_link_cfg *cmd_params;
+	struct dpni_cmd_link_cfg *cmd_params;
 
 	/* prepare command */
 	cmd.header = mc_encode_cmd_header(DPNI_CMDID_SET_LINK_CFG,
 					  cmd_flags,
 					  token);
-	cmd_params = (struct dpni_cmd_set_link_cfg *)cmd.params;
+	cmd_params = (struct dpni_cmd_link_cfg *)cmd.params;
 	cmd_params->rate = cpu_to_le32(cfg->rate);
 	cmd_params->options = cpu_to_le64(cfg->options);
 
@@ -853,6 +853,42 @@ int dpni_set_link_cfg(struct fsl_mc_io *mc_io,
 }
 
 /**
+ * dpni_get_link_cfg() - return the link configuration
+ * @mc_io:	Pointer to MC portal's I/O object
+ * @cmd_flags:	Command flags; one or more of 'MC_CMD_FLAG_'
+ * @token:	Token of DPNI object
+ * @cfg:	Link configuration from dpni object
+ *
+ * Return:	'0' on Success; Error code otherwise.
+ */
+int dpni_get_link_cfg(struct fsl_mc_io *mc_io,
+		      u32 cmd_flags,
+		      u16 token,
+		      struct dpni_link_cfg *cfg)
+{
+	struct fsl_mc_command cmd = { 0 };
+	struct dpni_cmd_link_cfg *rsp_params;
+	int err;
+
+	/* prepare command */
+	cmd.header = mc_encode_cmd_header(DPNI_CMDID_GET_LINK_CFG,
+					  cmd_flags,
+					  token);
+
+	/* send command to mc*/
+	err = mc_send_command(mc_io, &cmd);
+	if (err)
+		return err;
+
+	/* retrieve response parameters */
+	rsp_params = (struct dpni_cmd_link_cfg *)cmd.params;
+	cfg->rate = le32_to_cpu(rsp_params->rate);
+	cfg->options = le64_to_cpu(rsp_params->options);
+
+	return err;
+}
+
+/**
  * dpni_get_link_state() - Return the link state (either up or down)
  * @mc_io:	Pointer to MC portal's I/O object
  * @cmd_flags:	Command flags; one or more of 'MC_CMD_FLAG_'
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpni.h b/drivers/net/ethernet/freescale/dpaa2/dpni.h
index a521242..3e8fc6c 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpni.h
+++ b/drivers/net/ethernet/freescale/dpaa2/dpni.h
@@ -485,6 +485,11 @@ int dpni_set_link_cfg(struct fsl_mc_io			*mc_io,
 		      u16				token,
 		      const struct dpni_link_cfg	*cfg);
 
+int dpni_get_link_cfg(struct fsl_mc_io			*mc_io,
+		      u32				cmd_flags,
+		      u16				token,
+		      struct dpni_link_cfg		*cfg);
+
 /**
  * struct dpni_link_state - Structure representing DPNI link state
  * @rate: Rate
-- 
2.7.4

Re: [PATCH net-next v2 1/3] dpaa2-eth: Remove support for changing link settings

[Andrew Lunn]

On Tue, Aug 27, 2019 at 05:15:49PM +0300, Ioana Radulescu wrote:
> We only support fixed-link for now, so there is no point in
> offering users the option to change link settings via ethtool.
> 
> Functionally there is no change, since firmware prevents us from
> changing link parameters anyway.
> 
> Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v2 2/3] dpaa2-eth: Use stored link settings

[Andrew Lunn]

On Tue, Aug 27, 2019 at 05:15:50PM +0300, Ioana Radulescu wrote:
> Whenever a link state change occurs, we get notified and save
> the new link settings in the device's private data. In ethtool
> get_link_ksettings, use the stored state instead of interrogating
> the firmware each time.
> 
> Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v2 3/3] dpaa2-eth: Add pause frame support

[Andrew Lunn]

On Tue, Aug 27, 2019 at 05:15:51PM +0300, Ioana Radulescu wrote:
> Starting with firmware version MC10.18.0, we have support for
> L2 flow control. Asymmetrical configuration (Rx or Tx only) is
> supported, but not pause frame autonegotioation.

> +static int set_pause(struct dpaa2_eth_priv *priv)
> +{
> +	struct device *dev = priv->net_dev->dev.parent;
> +	struct dpni_link_cfg link_cfg = {0};
> +	int err;
> +
> +	/* Get the default link options so we don't override other flags */
> +	err = dpni_get_link_cfg(priv->mc_io, 0, priv->mc_token, &link_cfg);
> +	if (err) {
> +		dev_err(dev, "dpni_get_link_cfg() failed\n");
> +		return err;
> +	}
> +
> +	link_cfg.options |= DPNI_LINK_OPT_PAUSE;
> +	link_cfg.options &= ~DPNI_LINK_OPT_ASYM_PAUSE;
> +	err = dpni_set_link_cfg(priv->mc_io, 0, priv->mc_token, &link_cfg);
> +	if (err) {
> +		dev_err(dev, "dpni_set_link_cfg() failed\n");
> +		return err;
> +	}
> +
> +	priv->link_state.options = link_cfg.options;
> +
> +	return 0;
> +}
> +
>  /* Configure the DPNI object this interface is associated with */
>  static int setup_dpni(struct fsl_mc_device *ls_dev)
>  {
> @@ -2500,6 +2562,13 @@ static int setup_dpni(struct fsl_mc_device *ls_dev)
>  
>  	set_enqueue_mode(priv);
>  
> +	/* Enable pause frame support */
> +	if (dpaa2_eth_has_pause_support(priv)) {
> +		err = set_pause(priv);
> +		if (err)
> +			goto close;

Hi Ioana

So by default you have the MAC do pause, not asym pause?  Generally,
any MAC that can do asym pause does asym pause.

But if this is what you want, it is not wrong.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

RE: [PATCH net-next v2 3/3] dpaa2-eth: Add pause frame support

[Ioana Ciocoi Radulescu]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Wednesday, August 28, 2019 2:22 AM
> To: Ioana Ciocoi Radulescu <ruxandra.radulescu@nxp.com>
> Cc: netdev@vger.kernel.org; davem@davemloft.net; Ioana Ciornei
> <ioana.ciornei@nxp.com>
> Subject: Re: [PATCH net-next v2 3/3] dpaa2-eth: Add pause frame support
> 
> On Tue, Aug 27, 2019 at 05:15:51PM +0300, Ioana Radulescu wrote:
> > Starting with firmware version MC10.18.0, we have support for
> > L2 flow control. Asymmetrical configuration (Rx or Tx only) is
> > supported, but not pause frame autonegotioation.
> 
> > +static int set_pause(struct dpaa2_eth_priv *priv)
> > +{
> > +	struct device *dev = priv->net_dev->dev.parent;
> > +	struct dpni_link_cfg link_cfg = {0};
> > +	int err;
> > +
> > +	/* Get the default link options so we don't override other flags */
> > +	err = dpni_get_link_cfg(priv->mc_io, 0, priv->mc_token, &link_cfg);
> > +	if (err) {
> > +		dev_err(dev, "dpni_get_link_cfg() failed\n");
> > +		return err;
> > +	}
> > +
> > +	link_cfg.options |= DPNI_LINK_OPT_PAUSE;
> > +	link_cfg.options &= ~DPNI_LINK_OPT_ASYM_PAUSE;
> > +	err = dpni_set_link_cfg(priv->mc_io, 0, priv->mc_token, &link_cfg);
> > +	if (err) {
> > +		dev_err(dev, "dpni_set_link_cfg() failed\n");
> > +		return err;
> > +	}
> > +
> > +	priv->link_state.options = link_cfg.options;
> > +
> > +	return 0;
> > +}
> > +
> >  /* Configure the DPNI object this interface is associated with */
> >  static int setup_dpni(struct fsl_mc_device *ls_dev)
> >  {
> > @@ -2500,6 +2562,13 @@ static int setup_dpni(struct fsl_mc_device
> *ls_dev)
> >
> >  	set_enqueue_mode(priv);
> >
> > +	/* Enable pause frame support */
> > +	if (dpaa2_eth_has_pause_support(priv)) {
> > +		err = set_pause(priv);
> > +		if (err)
> > +			goto close;
> 
> Hi Ioana
> 
> So by default you have the MAC do pause, not asym pause?  Generally,
> any MAC that can do asym pause does asym pause.

Clearing the ASYM_PAUSE flag only means we tell the firmware we want
both Rx and Tx pause to be enabled in the beginning. User can still set
an asymmetric config (i.e. only Rx pause or only Tx pause to be enabled)
if needed.

The truth table is like this:

PAUSE | ASYM_PAUSE | Rx pause | Tx pause
----------------------------------------
  0   |     0      | disabled | disabled
  0   |     1      | disabled | enabled
  1   |     0      | enabled  | enabled
  1   |     1      | enabled  | disabled

Thanks,
Ioana

Re: [PATCH net-next v2 3/3] dpaa2-eth: Add pause frame support

[Andrew Lunn]

> Clearing the ASYM_PAUSE flag only means we tell the firmware we want
> both Rx and Tx pause to be enabled in the beginning. User can still set
> an asymmetric config (i.e. only Rx pause or only Tx pause to be enabled)
> if needed.
> 
> The truth table is like this:
> 
> PAUSE | ASYM_PAUSE | Rx pause | Tx pause
> ----------------------------------------
>   0   |     0      | disabled | disabled
>   0   |     1      | disabled | enabled
>   1   |     0      | enabled  | enabled
>   1   |     1      | enabled  | disabled

Hi Ioana

Ah, that is not intuitive. Please add a comment, and maybe this table
to the commit message.

Thanks
	Andrew

RE: [PATCH net-next v2 3/3] dpaa2-eth: Add pause frame support

[Ioana Ciocoi Radulescu]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Wednesday, August 28, 2019 2:53 PM
> To: Ioana Ciocoi Radulescu <ruxandra.radulescu@nxp.com>
> Cc: netdev@vger.kernel.org; davem@davemloft.net; Ioana Ciornei
> <ioana.ciornei@nxp.com>
> Subject: Re: [PATCH net-next v2 3/3] dpaa2-eth: Add pause frame support
> 
> > Clearing the ASYM_PAUSE flag only means we tell the firmware we want
> > both Rx and Tx pause to be enabled in the beginning. User can still set
> > an asymmetric config (i.e. only Rx pause or only Tx pause to be enabled)
> > if needed.
> >
> > The truth table is like this:
> >
> > PAUSE | ASYM_PAUSE | Rx pause | Tx pause
> > ----------------------------------------
> >   0   |     0      | disabled | disabled
> >   0   |     1      | disabled | enabled
> >   1   |     0      | enabled  | enabled
> >   1   |     1      | enabled  | disabled
> 
> Hi Ioana
> 
> Ah, that is not intuitive. Please add a comment, and maybe this table
> to the commit message.

I think firmware tried to mirror the ASM_DIR bit (see
http://www.ieee802.org/3/z/public/presentations/nov1996/asym.pdf),
but I agree it's not really user friendly. Will add comment in v3.

Thanks,
Ioana

Re: [PATCH net-next v2 3/3] dpaa2-eth: Add pause frame support

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Wed, 28 Aug 2019 13:52:50 +0200

>> Clearing the ASYM_PAUSE flag only means we tell the firmware we want
>> both Rx and Tx pause to be enabled in the beginning. User can still set
>> an asymmetric config (i.e. only Rx pause or only Tx pause to be enabled)
>> if needed.
>> 
>> The truth table is like this:
>> 
>> PAUSE | ASYM_PAUSE | Rx pause | Tx pause
>> ----------------------------------------
>>   0   |     0      | disabled | disabled
>>   0   |     1      | disabled | enabled
>>   1   |     0      | enabled  | enabled
>>   1   |     1      | enabled  | disabled
> 
> Hi Ioana
> 
> Ah, that is not intuitive. Please add a comment, and maybe this table
> to the commit message.

Isn't this the same truth table as for the pause bits in the usual MII
registers?