diff mbox series

[net-next,v2,5/6] enetc: Add interrupt coalescing support

Message ID 1595000224-6883-6-git-send-email-claudiu.manoil@nxp.com
State Changes Requested
Delegated to: David Miller
Headers show
Series Add adaptive interrupt coalescing | expand

Commit Message

Claudiu Manoil July 17, 2020, 3:37 p.m. UTC 
Enable programming of the interrupt coalescing registers
and allow manual configuration of the coalescing time
thresholds via ethtool.  Packet thresholds have been fixed
to predetermined values as there's no point in making them
run-time configurable, also anticipating the dynamic interrupt
moderation (DIM) algorithm which uses fixed packet thresholds
as well.  If the interface is up when the operation mode of
traffic interrupt events is changed by the user (i.e. switching
from default per-packet interrupts to coalesced interrupts),
the traffic needs to be paused in the process.
This patch also prepares the ground for introducing DIM on Rx.

Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>
---
v2: removed tx_ictt from the "fast path", as Tx updates
remain static (dropped Tx DIM idea)

 drivers/net/ethernet/freescale/enetc/enetc.c  | 32 ++++++--
 drivers/net/ethernet/freescale/enetc/enetc.h  | 18 +++++
 .../ethernet/freescale/enetc/enetc_ethtool.c  | 75 ++++++++++++++++++-
 .../net/ethernet/freescale/enetc/enetc_hw.h   | 19 ++++-
 4 files changed, 134 insertions(+), 10 deletions(-)

Comments

Jakub Kicinski July 17, 2020, 7:32 p.m. UTC | #1 
On Fri, 17 Jul 2020 18:37:03 +0300 Claudiu Manoil wrote:
> +	if (ic->rx_max_coalesced_frames != ENETC_RXIC_PKTTHR)
> +		netif_warn(priv, hw, ndev, "rx-frames fixed to %d\n",
> +			   ENETC_RXIC_PKTTHR);
> +
> +	if (ic->tx_max_coalesced_frames != ENETC_TXIC_PKTTHR)
> +		netif_warn(priv, hw, ndev, "tx-frames fixed to %d\n",
> +			   ENETC_TXIC_PKTTHR);

On second thought - why not return an error here? Since only one value
is supported seems like the right way to communicate to the users that
they can't change this.

> +	if (netif_running(ndev) && changed) {
> +		/* reconfigure the operation mode of h/w interrupts,
> +		 * traffic needs to be paused in the process
> +		 */
> +		enetc_stop(ndev);
> +		enetc_start(ndev);

Is start going to print an error when it fails? Kinda scary if this
could turn into a silent failure.
Claudiu Manoil July 18, 2020, 5:20 p.m. UTC | #2 
On 17.07.2020 22:32, Jakub Kicinski wrote:
> On Fri, 17 Jul 2020 18:37:03 +0300 Claudiu Manoil wrote:
>> +	if (ic->rx_max_coalesced_frames != ENETC_RXIC_PKTTHR)
>> +		netif_warn(priv, hw, ndev, "rx-frames fixed to %d\n",
>> +			   ENETC_RXIC_PKTTHR);
>> +
>> +	if (ic->tx_max_coalesced_frames != ENETC_TXIC_PKTTHR)
>> +		netif_warn(priv, hw, ndev, "tx-frames fixed to %d\n",
>> +			   ENETC_TXIC_PKTTHR);
> 
> On second thought - why not return an error here? Since only one value
> is supported seems like the right way to communicate to the users that
> they can't change this.
> 

Do you mean to return -EOPNOTSUPP without any error message instead?
If so, I think it's less punishing not to return an error code and 
invalidate the rest of the ethtool -C parameters that might have been
correct if the user forgets that rx/tx-frames cannot be changed.
There's also this flag:
	.supported_coalesce_params = .. |
				     ETHTOOL_COALESCE_MAX_FRAMES |
				     ..,
needed for printing the preconfigured values for the rx/tx packet 
thresholds, and this flag basically says that the 'rx/tx-frames'
parameters are supported (just that they cannot be changed... :) ).
But I don't have a strong bias for this, if you prefer the return
-EOPNOTSUPP option I'll make this change, just let me know if I got
it right.

>> +	if (netif_running(ndev) && changed) {
>> +		/* reconfigure the operation mode of h/w interrupts,
>> +		 * traffic needs to be paused in the process
>> +		 */
>> +		enetc_stop(ndev);
>> +		enetc_start(ndev);
> 
> Is start going to print an error when it fails? Kinda scary if this
> could turn into a silent failure.
> 

enetc_start() returns void, just like enetc_stop().  If you look it up
it only sets some run-time configurable registers and calls some basic
run-time commands that should no fail like napi_enable(), enable_irq(), 
phy_start(), all void returning functions. This function doesn't 
allocate resources or anything of that sort, and should be kept that 
way. And indeed, it should not fail. But regarding error codes there's
nothing I can do for this function, as nothing inside it generates any 
error code.
Jakub Kicinski July 20, 2020, 4:58 p.m. UTC | #3 
On Sat, 18 Jul 2020 20:20:10 +0300 Claudiu Manoil wrote:
> On 17.07.2020 22:32, Jakub Kicinski wrote:
> > On Fri, 17 Jul 2020 18:37:03 +0300 Claudiu Manoil wrote:  
> >> +	if (ic->rx_max_coalesced_frames != ENETC_RXIC_PKTTHR)
> >> +		netif_warn(priv, hw, ndev, "rx-frames fixed to %d\n",
> >> +			   ENETC_RXIC_PKTTHR);
> >> +
> >> +	if (ic->tx_max_coalesced_frames != ENETC_TXIC_PKTTHR)
> >> +		netif_warn(priv, hw, ndev, "tx-frames fixed to %d\n",
> >> +			   ENETC_TXIC_PKTTHR);  
> > 
> > On second thought - why not return an error here? Since only one value
> > is supported seems like the right way to communicate to the users that
> > they can't change this.
> 
> Do you mean to return -EOPNOTSUPP without any error message instead?

Yes.

> If so, I think it's less punishing not to return an error code and 
> invalidate the rest of the ethtool -C parameters that might have been
> correct if the user forgets that rx/tx-frames cannot be changed.

IMHO if configuring manually - user can correct the params on the CLI.
If there's an orchestration system trying to configure those - it's 
better to return an error and alert the administrator than confuse 
the orchestration by allowing a write which is then not reflected 
on read.

> There's also this flag:
> 	.supported_coalesce_params = .. |
> 				     ETHTOOL_COALESCE_MAX_FRAMES |
> 				     ..,
> needed for printing the preconfigured values for the rx/tx packet 
> thresholds, and this flag basically says that the 'rx/tx-frames'
> parameters are supported (just that they cannot be changed... :) ).

Right, unfortunately core can't do the checking here, but I think 
the driver should.

> But I don't have a strong bias for this, if you prefer the return
> -EOPNOTSUPP option I'll make this change, just let me know if I got
> it right.
> 
> >> +	if (netif_running(ndev) && changed) {
> >> +		/* reconfigure the operation mode of h/w interrupts,
> >> +		 * traffic needs to be paused in the process
> >> +		 */
> >> +		enetc_stop(ndev);
> >> +		enetc_start(ndev);  
> > 
> > Is start going to print an error when it fails? Kinda scary if this
> > could turn into a silent failure.
> 
> enetc_start() returns void, just like enetc_stop().  If you look it up
> it only sets some run-time configurable registers and calls some basic
> run-time commands that should no fail like napi_enable(), enable_irq(), 
> phy_start(), all void returning functions. This function doesn't 
> allocate resources or anything of that sort, and should be kept that 
> way. And indeed, it should not fail. But regarding error codes there's
> nothing I can do for this function, as nothing inside it generates any 
> error code.

Got it!
Claudiu Manoil July 21, 2020, 8 a.m. UTC | #4 
>-----Original Message-----
>From: Jakub Kicinski <kuba@kernel.org>
>Sent: Monday, July 20, 2020 7:59 PM
[...]
>Subject: Re: [PATCH net-next v2 5/6] enetc: Add interrupt coalescing support
>
>On Sat, 18 Jul 2020 20:20:10 +0300 Claudiu Manoil wrote:
>> On 17.07.2020 22:32, Jakub Kicinski wrote:
>> > On Fri, 17 Jul 2020 18:37:03 +0300 Claudiu Manoil wrote:
>> >> +	if (ic->rx_max_coalesced_frames != ENETC_RXIC_PKTTHR)
>> >> +		netif_warn(priv, hw, ndev, "rx-frames fixed to %d\n",
>> >> +			   ENETC_RXIC_PKTTHR);
>> >> +
>> >> +	if (ic->tx_max_coalesced_frames != ENETC_TXIC_PKTTHR)
>> >> +		netif_warn(priv, hw, ndev, "tx-frames fixed to %d\n",
>> >> +			   ENETC_TXIC_PKTTHR);
>> >
>> > On second thought - why not return an error here? Since only one value
>> > is supported seems like the right way to communicate to the users that
>> > they can't change this.
>>
>> Do you mean to return -EOPNOTSUPP without any error message instead?
>
>Yes.
>
>> If so, I think it's less punishing not to return an error code and
>> invalidate the rest of the ethtool -C parameters that might have been
>> correct if the user forgets that rx/tx-frames cannot be changed.
>
>IMHO if configuring manually - user can correct the params on the CLI.
>If there's an orchestration system trying to configure those - it's
>better to return an error and alert the administrator than confuse
>the orchestration by allowing a write which is then not reflected
>on read.
>

Good point, ok.  Updated accordingly in v3.
Thanks.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index be594c7af538..f4593c044043 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -265,6 +265,7 @@  static irqreturn_t enetc_msix(int irq, void *data)
 
 	/* disable interrupts */
 	enetc_wr_reg(v->rbier, 0);
+	enetc_wr_reg(v->ricr1, v->rx_ictt);
 
 	for_each_set_bit(i, &v->tx_rings_map, ENETC_MAX_NUM_TXQS)
 		enetc_wr_reg(v->tbier_base + ENETC_BDR_OFF(i), 0);
@@ -1268,6 +1269,7 @@  static int enetc_setup_irqs(struct enetc_ndev_priv *priv)
 
 		v->tbier_base = hw->reg + ENETC_BDR(TX, 0, ENETC_TBIER);
 		v->rbier = hw->reg + ENETC_BDR(RX, i, ENETC_RBIER);
+		v->ricr1 = hw->reg + ENETC_BDR(RX, i, ENETC_RBICR1);
 
 		enetc_wr(hw, ENETC_SIMSIRRV(i), entry);
 
@@ -1309,17 +1311,35 @@  static void enetc_free_irqs(struct enetc_ndev_priv *priv)
 
 static void enetc_setup_interrupts(struct enetc_ndev_priv *priv)
 {
+	struct enetc_hw *hw = &priv->si->hw;
+	u32 icpt, ictt;
 	int i;
 
 	/* enable Tx & Rx event indication */
+	if (priv->ic_mode & ENETC_IC_RX_MANUAL) {
+		icpt = ENETC_RBICR0_SET_ICPT(ENETC_RXIC_PKTTHR);
+		/* init to non-0 minimum, will be adjusted later */
+		ictt = 0x1;
+	} else {
+		icpt = 0x1; /* enable Rx ints by setting pkt thr to 1 */
+		ictt = 0;
+	}
+
 	for (i = 0; i < priv->num_rx_rings; i++) {
-		enetc_rxbdr_wr(&priv->si->hw, i,
-			       ENETC_RBIER, ENETC_RBIER_RXTIE);
+		enetc_rxbdr_wr(hw, i, ENETC_RBICR1, ictt);
+		enetc_rxbdr_wr(hw, i, ENETC_RBICR0, ENETC_RBICR0_ICEN | icpt);
+		enetc_rxbdr_wr(hw, i, ENETC_RBIER, ENETC_RBIER_RXTIE);
 	}
 
+	if (priv->ic_mode & ENETC_IC_TX_MANUAL)
+		icpt = ENETC_TBICR0_SET_ICPT(ENETC_TXIC_PKTTHR);
+	else
+		icpt = 0x1; /* enable Tx ints by setting pkt thr to 1 */
+
 	for (i = 0; i < priv->num_tx_rings; i++) {
-		enetc_txbdr_wr(&priv->si->hw, i,
-			       ENETC_TBIER, ENETC_TBIER_TXTIE);
+		enetc_txbdr_wr(hw, i, ENETC_TBICR1, priv->tx_ictt);
+		enetc_txbdr_wr(hw, i, ENETC_TBICR0, ENETC_TBICR0_ICEN | icpt);
+		enetc_txbdr_wr(hw, i, ENETC_TBIER, ENETC_TBIER_TXTIE);
 	}
 }
 
@@ -1370,7 +1390,7 @@  static int enetc_phy_connect(struct net_device *ndev)
 	return 0;
 }
 
-static void enetc_start(struct net_device *ndev)
+void enetc_start(struct net_device *ndev)
 {
 	struct enetc_ndev_priv *priv = netdev_priv(ndev);
 	int i;
@@ -1440,7 +1460,7 @@  int enetc_open(struct net_device *ndev)
 	return err;
 }
 
-static void enetc_stop(struct net_device *ndev)
+void enetc_stop(struct net_device *ndev)
 {
 	struct enetc_ndev_priv *priv = netdev_priv(ndev);
 	int i;
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h
index 81e9072e10d4..4e3af7f07892 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc.h
@@ -190,8 +190,10 @@  static inline bool enetc_si_is_pf(struct enetc_si *si)
 struct enetc_int_vector {
 	void __iomem *rbier;
 	void __iomem *tbier_base;
+	void __iomem *ricr1;
 	unsigned long tx_rings_map;
 	int count_tx_rings;
+	u32 rx_ictt;
 	struct napi_struct napi;
 	char name[ENETC_INT_NAME_MAX];
 
@@ -221,6 +223,18 @@  enum enetc_active_offloads {
 	ENETC_F_QCI		= BIT(3),
 };
 
+/* interrupt coalescing modes */
+enum enetc_ic_mode {
+	/* one interrupt per frame */
+	ENETC_IC_NONE = 0,
+	/* activated when int coalescing time is set to a non-0 value */
+	ENETC_IC_RX_MANUAL = BIT(0),
+	ENETC_IC_TX_MANUAL = BIT(1),
+};
+
+#define ENETC_RXIC_PKTTHR	min_t(u32, 256, ENETC_RX_RING_DEFAULT_SIZE / 2)
+#define ENETC_TXIC_PKTTHR	min_t(u32, 128, ENETC_TX_RING_DEFAULT_SIZE / 2)
+
 struct enetc_ndev_priv {
 	struct net_device *ndev;
 	struct device *dev; /* dma-mapping device */
@@ -245,6 +259,8 @@  struct enetc_ndev_priv {
 
 	struct device_node *phy_node;
 	phy_interface_t if_mode;
+	int ic_mode;
+	u32 tx_ictt;
 };
 
 /* Messaging */
@@ -274,6 +290,8 @@  void enetc_free_si_resources(struct enetc_ndev_priv *priv);
 
 int enetc_open(struct net_device *ndev);
 int enetc_close(struct net_device *ndev);
+void enetc_start(struct net_device *ndev);
+void enetc_stop(struct net_device *ndev);
 netdev_tx_t enetc_xmit(struct sk_buff *skb, struct net_device *ndev);
 struct net_device_stats *enetc_get_stats(struct net_device *ndev);
 int enetc_set_features(struct net_device *ndev,
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
index 8aeaa3de0012..3bf4fc0bc64a 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_ethtool.c
@@ -14,12 +14,14 @@  static const u32 enetc_si_regs[] = {
 
 static const u32 enetc_txbdr_regs[] = {
 	ENETC_TBMR, ENETC_TBSR, ENETC_TBBAR0, ENETC_TBBAR1,
-	ENETC_TBPIR, ENETC_TBCIR, ENETC_TBLENR, ENETC_TBIER
+	ENETC_TBPIR, ENETC_TBCIR, ENETC_TBLENR, ENETC_TBIER, ENETC_TBICR0,
+	ENETC_TBICR1
 };
 
 static const u32 enetc_rxbdr_regs[] = {
 	ENETC_RBMR, ENETC_RBSR, ENETC_RBBSR, ENETC_RBCIR, ENETC_RBBAR0,
-	ENETC_RBBAR1, ENETC_RBPIR, ENETC_RBLENR, ENETC_RBICR0, ENETC_RBIER
+	ENETC_RBBAR1, ENETC_RBPIR, ENETC_RBLENR, ENETC_RBIER, ENETC_RBICR0,
+	ENETC_RBICR1
 };
 
 static const u32 enetc_port_regs[] = {
@@ -561,6 +563,67 @@  static void enetc_get_ringparam(struct net_device *ndev,
 	}
 }
 
+static int enetc_get_coalesce(struct net_device *ndev,
+			      struct ethtool_coalesce *ic)
+{
+	struct enetc_ndev_priv *priv = netdev_priv(ndev);
+	struct enetc_int_vector *v = priv->int_vector[0];
+
+	ic->tx_coalesce_usecs = enetc_cycles_to_usecs(priv->tx_ictt);
+	ic->rx_coalesce_usecs = enetc_cycles_to_usecs(v->rx_ictt);
+
+	ic->tx_max_coalesced_frames = ENETC_TXIC_PKTTHR;
+	ic->rx_max_coalesced_frames = ENETC_RXIC_PKTTHR;
+
+	return 0;
+}
+
+static int enetc_set_coalesce(struct net_device *ndev,
+			      struct ethtool_coalesce *ic)
+{
+	struct enetc_ndev_priv *priv = netdev_priv(ndev);
+	u32 rx_ictt, tx_ictt;
+	int i, ic_mode;
+	bool changed;
+
+	tx_ictt = enetc_usecs_to_cycles(ic->tx_coalesce_usecs);
+	rx_ictt = enetc_usecs_to_cycles(ic->rx_coalesce_usecs);
+
+	if (ic->rx_max_coalesced_frames != ENETC_RXIC_PKTTHR)
+		netif_warn(priv, hw, ndev, "rx-frames fixed to %d\n",
+			   ENETC_RXIC_PKTTHR);
+
+	if (ic->tx_max_coalesced_frames != ENETC_TXIC_PKTTHR)
+		netif_warn(priv, hw, ndev, "tx-frames fixed to %d\n",
+			   ENETC_TXIC_PKTTHR);
+
+	ic_mode = ENETC_IC_NONE;
+	ic_mode |= tx_ictt ? ENETC_IC_TX_MANUAL : 0;
+	ic_mode |= rx_ictt ? ENETC_IC_RX_MANUAL : 0;
+
+	/* commit the settings */
+	changed = (ic_mode != priv->ic_mode);
+
+	priv->ic_mode = ic_mode;
+	priv->tx_ictt = tx_ictt;
+
+	for (i = 0; i < priv->bdr_int_num; i++) {
+		struct enetc_int_vector *v = priv->int_vector[i];
+
+		v->rx_ictt = rx_ictt;
+	}
+
+	if (netif_running(ndev) && changed) {
+		/* reconfigure the operation mode of h/w interrupts,
+		 * traffic needs to be paused in the process
+		 */
+		enetc_stop(ndev);
+		enetc_start(ndev);
+	}
+
+	return 0;
+}
+
 static int enetc_get_ts_info(struct net_device *ndev,
 			     struct ethtool_ts_info *info)
 {
@@ -617,6 +680,8 @@  static int enetc_set_wol(struct net_device *dev,
 }
 
 static const struct ethtool_ops enetc_pf_ethtool_ops = {
+	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
+				     ETHTOOL_COALESCE_MAX_FRAMES,
 	.get_regs_len = enetc_get_reglen,
 	.get_regs = enetc_get_regs,
 	.get_sset_count = enetc_get_sset_count,
@@ -629,6 +694,8 @@  static const struct ethtool_ops enetc_pf_ethtool_ops = {
 	.get_rxfh = enetc_get_rxfh,
 	.set_rxfh = enetc_set_rxfh,
 	.get_ringparam = enetc_get_ringparam,
+	.get_coalesce = enetc_get_coalesce,
+	.set_coalesce = enetc_set_coalesce,
 	.get_link_ksettings = phy_ethtool_get_link_ksettings,
 	.set_link_ksettings = phy_ethtool_set_link_ksettings,
 	.get_link = ethtool_op_get_link,
@@ -638,6 +705,8 @@  static const struct ethtool_ops enetc_pf_ethtool_ops = {
 };
 
 static const struct ethtool_ops enetc_vf_ethtool_ops = {
+	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
+				     ETHTOOL_COALESCE_MAX_FRAMES,
 	.get_regs_len = enetc_get_reglen,
 	.get_regs = enetc_get_regs,
 	.get_sset_count = enetc_get_sset_count,
@@ -649,6 +718,8 @@  static const struct ethtool_ops enetc_vf_ethtool_ops = {
 	.get_rxfh = enetc_get_rxfh,
 	.set_rxfh = enetc_set_rxfh,
 	.get_ringparam = enetc_get_ringparam,
+	.get_coalesce = enetc_get_coalesce,
+	.set_coalesce = enetc_set_coalesce,
 	.get_link = ethtool_op_get_link,
 	.get_ts_info = enetc_get_ts_info,
 };
diff --git a/drivers/net/ethernet/freescale/enetc/enetc_hw.h b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
index 05bb4c525897..95f3c4d8f602 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_hw.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc_hw.h
@@ -122,7 +122,10 @@  enum enetc_bdr_type {TX, RX};
 #define ENETC_RBIER_RXTIE	BIT(0)
 #define ENETC_RBIDR	0xa4
 #define ENETC_RBICR0	0xa8
-#define ENETC_RBICR0_ICEN	BIT(31)
+#define ENETC_RBICR0_ICEN		BIT(31)
+#define ENETC_RBICR0_ICPT_MASK		0x1ff
+#define ENETC_RBICR0_SET_ICPT(n)	((n) & ENETC_RBICR0_ICPT_MASK)
+#define ENETC_RBICR1	0xac
 
 /* TX BDR reg offsets */
 #define ENETC_TBMR	0
@@ -142,7 +145,10 @@  enum enetc_bdr_type {TX, RX};
 #define ENETC_TBIER_TXTIE	BIT(0)
 #define ENETC_TBIDR	0xa4
 #define ENETC_TBICR0	0xa8
-#define ENETC_TBICR0_ICEN	BIT(31)
+#define ENETC_TBICR0_ICEN		BIT(31)
+#define ENETC_TBICR0_ICPT_MASK		0xf
+#define ENETC_TBICR0_SET_ICPT(n) ((ilog2(n) + 1) & ENETC_TBICR0_ICPT_MASK)
+#define ENETC_TBICR1	0xac
 
 #define ENETC_RTBLENR_LEN(n)	((n) & ~0x7)
 
@@ -784,6 +790,15 @@  struct enetc_cbd {
 };
 
 #define ENETC_CLK  400000000ULL
+static inline u32 enetc_cycles_to_usecs(u32 cycles)
+{
+	return (u32)div_u64(cycles * 1000000ULL, ENETC_CLK);
+}
+
+static inline u32 enetc_usecs_to_cycles(u32 usecs)
+{
+	return (u32)div_u64(usecs * ENETC_CLK, 1000000ULL);
+}
 
 /* port time gating control register */
 #define ENETC_QBV_PTGCR_OFFSET		0x11a00