[PATCH] net: fec: allow disable coalescing

[PATCH] net: fec: allow disable coalescing

[Jonas Rebmann]

In the current implementation, IP coalescing is always enabled and
cannot be disabled.

As setting maximum frames to 0 or 1, or setting delay to zero implies
immediate delivery of single packets/IRQs, disable coalescing in
hardware in these cases.

Signed-off-by: Jonas Rebmann <jre@pengutronix.de>
---
 drivers/net/ethernet/freescale/fec_main.c | 34 ++++++++++++++-----------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 63dac42720453a8b8a847bdd1eec76ac072030bf..07676f35664736a89d53c1cb4a436218c28524f7 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -3121,27 +3121,23 @@ static int fec_enet_us_to_itr_clock(struct net_device *ndev, int us)
 static void fec_enet_itr_coal_set(struct net_device *ndev)
 {
 	struct fec_enet_private *fep = netdev_priv(ndev);
-	int rx_itr, tx_itr;
+	int rx_itr = 0, tx_itr = 0;
 
-	/* Must be greater than zero to avoid unpredictable behavior */
-	if (!fep->rx_time_itr || !fep->rx_pkts_itr ||
-	    !fep->tx_time_itr || !fep->tx_pkts_itr)
-		return;
-
-	/* Select enet system clock as Interrupt Coalescing
-	 * timer Clock Source
-	 */
-	rx_itr = FEC_ITR_CLK_SEL;
-	tx_itr = FEC_ITR_CLK_SEL;
-
-	/* set ICFT and ICTT */
-	rx_itr |= FEC_ITR_ICFT(fep->rx_pkts_itr);
-	rx_itr |= FEC_ITR_ICTT(fec_enet_us_to_itr_clock(ndev, fep->rx_time_itr));
-	tx_itr |= FEC_ITR_ICFT(fep->tx_pkts_itr);
-	tx_itr |= FEC_ITR_ICTT(fec_enet_us_to_itr_clock(ndev, fep->tx_time_itr));
+	if (fep->rx_time_itr > 0 && fep->rx_pkts_itr > 1) {
+		/* Select enet system clock as Interrupt Coalescing timer Clock Source */
+		rx_itr = FEC_ITR_CLK_SEL;
+		rx_itr |= FEC_ITR_EN;
+		rx_itr |= FEC_ITR_ICFT(fep->rx_pkts_itr);
+		rx_itr |= FEC_ITR_ICTT(fec_enet_us_to_itr_clock(ndev, fep->rx_time_itr));
+	}
 
-	rx_itr |= FEC_ITR_EN;
-	tx_itr |= FEC_ITR_EN;
+	if (fep->tx_time_itr > 0 && fep->tx_pkts_itr > 1) {
+		/* Select enet system clock as Interrupt Coalescing timer Clock Source */
+		tx_itr = FEC_ITR_CLK_SEL;
+		tx_itr |= FEC_ITR_EN;
+		tx_itr |= FEC_ITR_ICFT(fep->tx_pkts_itr);
+		tx_itr |= FEC_ITR_ICTT(fec_enet_us_to_itr_clock(ndev, fep->tx_time_itr));
+	}
 
 	writel(tx_itr, fep->hwp + FEC_TXIC0);
 	writel(rx_itr, fep->hwp + FEC_RXIC0);

---
base-commit: 8dacfd92dbefee829ca555a860e86108fdd1d55b
change-id: 20250625-fec_deactivate_coalescing-c6f8d14a1d66

Best regards,
-- 
Jonas Rebmann <jre@pengutronix.de>

RE: [PATCH] net: fec: allow disable coalescing

[Wei Fang]

>  static void fec_enet_itr_coal_set(struct net_device *ndev)
>  {
>         struct fec_enet_private *fep = netdev_priv(ndev);
> -       int rx_itr, tx_itr;
> +       int rx_itr = 0, tx_itr = 0;

Since you modified this line, it would be a good idea to change
the type to u32.

> 
> -       /* Must be greater than zero to avoid unpredictable behavior */
> -       if (!fep->rx_time_itr || !fep->rx_pkts_itr ||
> -           !fep->tx_time_itr || !fep->tx_pkts_itr)
> -               return;
> -
> -       /* Select enet system clock as Interrupt Coalescing
> -        * timer Clock Source
> -        */
> -       rx_itr = FEC_ITR_CLK_SEL;
> -       tx_itr = FEC_ITR_CLK_SEL;
> -
> -       /* set ICFT and ICTT */
> -       rx_itr |= FEC_ITR_ICFT(fep->rx_pkts_itr);
> -       rx_itr |= FEC_ITR_ICTT(fec_enet_us_to_itr_clock(ndev,
> fep->rx_time_itr));
> -       tx_itr |= FEC_ITR_ICFT(fep->tx_pkts_itr);
> -       tx_itr |= FEC_ITR_ICTT(fec_enet_us_to_itr_clock(ndev,
> fep->tx_time_itr));
> +       if (fep->rx_time_itr > 0 && fep->rx_pkts_itr > 1) {
> +               /* Select enet system clock as Interrupt Coalescing timer
> Clock Source */
> +               rx_itr = FEC_ITR_CLK_SEL;
> +               rx_itr |= FEC_ITR_EN;

nitpicking: the above two lines can be simplified to one line as below,
but the comment needs to be changed appropriately.

	rx_itr = FEC_ITR_CLK_SEL | FEC_ITR_EN;

> +               rx_itr |= FEC_ITR_ICFT(fep->rx_pkts_itr);
> +               rx_itr |= FEC_ITR_ICTT(fec_enet_us_to_itr_clock(ndev,
> fep->rx_time_itr));
> +       }
> 
> -       rx_itr |= FEC_ITR_EN;
> -       tx_itr |= FEC_ITR_EN;
> +       if (fep->tx_time_itr > 0 && fep->tx_pkts_itr > 1) {
> +               /* Select enet system clock as Interrupt Coalescing timer
> Clock Source */
> +               tx_itr = FEC_ITR_CLK_SEL;
> +               tx_itr |= FEC_ITR_EN;

Same as above

Re: [PATCH] net: fec: allow disable coalescing

[Andrew Lunn]

On Thu, Jun 26, 2025 at 02:36:37AM +0000, Wei Fang wrote:
> >  static void fec_enet_itr_coal_set(struct net_device *ndev)
> >  {
> >         struct fec_enet_private *fep = netdev_priv(ndev);
> > -       int rx_itr, tx_itr;
> > +       int rx_itr = 0, tx_itr = 0;
> 
> Since you modified this line, it would be a good idea to change
> the type to u32.
> 
> > 
> > -       /* Must be greater than zero to avoid unpredictable behavior */
> > -       if (!fep->rx_time_itr || !fep->rx_pkts_itr ||
> > -           !fep->tx_time_itr || !fep->tx_pkts_itr)
> > -               return;

Hi Wei

When i see a comment like this being removed, i wounder if there is
any danger of side effects? Do you know what is being done here is
actually safe, for all the different versions of the FEC which support
coalescence?

Thanks
	Andrew

Re: [PATCH] net: fec: allow disable coalescing

[Jonas Rebmann]

Hi Andrew,

On 2025-06-26 10:12, Andrew Lunn wrote:
> On Thu, Jun 26, 2025 at 02:36:37AM +0000, Wei Fang wrote:
>>>
>>> -       /* Must be greater than zero to avoid unpredictable behavior */
>>> -       if (!fep->rx_time_itr || !fep->rx_pkts_itr ||
>>> -           !fep->tx_time_itr || !fep->tx_pkts_itr)
>>> -               return;
> 
> Hi Wei
> 
> When i see a comment like this being removed, i wounder if there is
> any danger of side effects? Do you know what is being done here is
> actually safe, for all the different versions of the FEC which support
> coalescence?

For reference, this comment is taken in plain from section 11.6.4.1.16.3
in the i.MX 8M Plus Applications Processor Reference Manual (and is the
same for the 6UL).

I was also worried about this so I made sure that in any case where
either of those is zero, the coalescing enable bit (FEC_ITR_EN) is
explicitly disabled.

fec_enet_itr_coal_set is only ever called if FEC_QUIRK_HAS_COALESCE is
set and for those models, we expect disabling coalescing via FEC_ITR_EN
-- and consequently also setting the parameters to zero -- to be
unproblematic. This is also the reset default.

Regards,
Jonas

-- 
Pengutronix e.K.                           | Jonas Rebmann               |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-9    |

RE: [PATCH] net: fec: allow disable coalescing

[Wei Fang]

> On 2025-06-26 10:12, Andrew Lunn wrote:
> > On Thu, Jun 26, 2025 at 02:36:37AM +0000, Wei Fang wrote:
> >>>
> >>> -       /* Must be greater than zero to avoid unpredictable behavior */
> >>> -       if (!fep->rx_time_itr || !fep->rx_pkts_itr ||
> >>> -           !fep->tx_time_itr || !fep->tx_pkts_itr)
> >>> -               return;
> >
> > Hi Wei
> >
> > When i see a comment like this being removed, i wounder if there is
> > any danger of side effects? Do you know what is being done here is
> > actually safe, for all the different versions of the FEC which support
> > coalescence?
> 
> For reference, this comment is taken in plain from section 11.6.4.1.16.3
> in the i.MX 8M Plus Applications Processor Reference Manual (and is the
> same for the 6UL).
> 
> I was also worried about this so I made sure that in any case where
> either of those is zero, the coalescing enable bit (FEC_ITR_EN) is
> explicitly disabled.
> 
> fec_enet_itr_coal_set is only ever called if FEC_QUIRK_HAS_COALESCE is
> set and for those models, we expect disabling coalescing via FEC_ITR_EN
> -- and consequently also setting the parameters to zero -- to be
> unproblematic. This is also the reset default.
> 

Yes, ICFT and ICTT cannot be set to zero when FEC_ITR_EN is set. But it is
safe to 0 when FEC_ITR_EN is not set.

Re: [PATCH] net: fec: allow disable coalescing

[Andrew Lunn]

On Thu, Jun 26, 2025 at 10:33:51AM +0200, Jonas Rebmann wrote:
> Hi Andrew,
> 
> On 2025-06-26 10:12, Andrew Lunn wrote:
> > On Thu, Jun 26, 2025 at 02:36:37AM +0000, Wei Fang wrote:
> > > > 
> > > > -       /* Must be greater than zero to avoid unpredictable behavior */
> > > > -       if (!fep->rx_time_itr || !fep->rx_pkts_itr ||
> > > > -           !fep->tx_time_itr || !fep->tx_pkts_itr)
> > > > -               return;
> > 
> > Hi Wei
> > 
> > When i see a comment like this being removed, i wounder if there is
> > any danger of side effects? Do you know what is being done here is
> > actually safe, for all the different versions of the FEC which support
> > coalescence?
> 
> For reference, this comment is taken in plain from section 11.6.4.1.16.3
> in the i.MX 8M Plus Applications Processor Reference Manual (and is the
> same for the 6UL).
> 
> I was also worried about this so I made sure that in any case where
> either of those is zero, the coalescing enable bit (FEC_ITR_EN) is
> explicitly disabled.
> 
> fec_enet_itr_coal_set is only ever called if FEC_QUIRK_HAS_COALESCE is
> set and for those models, we expect disabling coalescing via FEC_ITR_EN
> -- and consequently also setting the parameters to zero -- to be
> unproblematic. This is also the reset default.

Thanks for the additional details. You could mention this in the
commit message. The commit message allows you to answer reviewers
questions before they ask them...

	Andrew