All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] net: davinci_emac: Fix interrupt pacing disable
@ 2021-11-01  9:21 Maxim Kiselev
  2021-11-01 11:54 ` Grygorii Strashko
  2021-11-01 14:03 ` Maxim Kiselev
  0 siblings, 2 replies; 6+ messages in thread
From: Maxim Kiselev @ 2021-11-01  9:21 UTC (permalink / raw)
  Cc: Maxim Kiselev, Grygorii Strashko, David S. Miller,
	Jakub Kicinski, Andrew Lunn, Yang Yingliang, Colin Ian King,
	Yufeng Mo, Michael Walle, Sriram, linux-omap, netdev,
	linux-kernel

This patch allows to use 0 for `coal->rx_coalesce_usecs` param to
disable rx irq coalescing.

Previously we could enable rx irq coalescing via ethtool
(For ex: `ethtool -C eth0 rx-usecs 2000`) but we couldn't disable
it because this part rejects 0 value:

       if (!coal->rx_coalesce_usecs)
               return -EINVAL;

Fixes: 84da2658a619 ("TI DaVinci EMAC : Implement interrupt pacing
functionality.")

Signed-off-by: Maxim Kiselev <bigunclemax@gmail.com>
---
 drivers/net/ethernet/ti/davinci_emac.c | 77 ++++++++++++++------------
 1 file changed, 41 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
index e8291d8488391..a3a02c4e5eb68 100644
--- a/drivers/net/ethernet/ti/davinci_emac.c
+++ b/drivers/net/ethernet/ti/davinci_emac.c
@@ -417,46 +417,47 @@ static int emac_set_coalesce(struct net_device *ndev,
 			     struct netlink_ext_ack *extack)
 {
 	struct emac_priv *priv = netdev_priv(ndev);
-	u32 int_ctrl, num_interrupts = 0;
+	u32 int_ctrl = 0, num_interrupts = 0;
 	u32 prescale = 0, addnl_dvdr = 1, coal_intvl = 0;
 
-	if (!coal->rx_coalesce_usecs)
-		return -EINVAL;
-
 	coal_intvl = coal->rx_coalesce_usecs;
 
 	switch (priv->version) {
 	case EMAC_VERSION_2:
-		int_ctrl =  emac_ctrl_read(EMAC_DM646X_CMINTCTRL);
-		prescale = priv->bus_freq_mhz * 4;
-
-		if (coal_intvl < EMAC_DM646X_CMINTMIN_INTVL)
-			coal_intvl = EMAC_DM646X_CMINTMIN_INTVL;
-
-		if (coal_intvl > EMAC_DM646X_CMINTMAX_INTVL) {
-			/*
-			 * Interrupt pacer works with 4us Pulse, we can
-			 * throttle further by dilating the 4us pulse.
-			 */
-			addnl_dvdr = EMAC_DM646X_INTPRESCALE_MASK / prescale;
-
-			if (addnl_dvdr > 1) {
-				prescale *= addnl_dvdr;
-				if (coal_intvl > (EMAC_DM646X_CMINTMAX_INTVL
-							* addnl_dvdr))
-					coal_intvl = (EMAC_DM646X_CMINTMAX_INTVL
-							* addnl_dvdr);
-			} else {
-				addnl_dvdr = 1;
-				coal_intvl = EMAC_DM646X_CMINTMAX_INTVL;
+		if (coal->rx_coalesce_usecs) {
+			int_ctrl =  emac_ctrl_read(EMAC_DM646X_CMINTCTRL);
+			prescale = priv->bus_freq_mhz * 4;
+
+			if (coal_intvl < EMAC_DM646X_CMINTMIN_INTVL)
+				coal_intvl = EMAC_DM646X_CMINTMIN_INTVL;
+
+			if (coal_intvl > EMAC_DM646X_CMINTMAX_INTVL) {
+				/*
+				 * Interrupt pacer works with 4us Pulse, we can
+				 * throttle further by dilating the 4us pulse.
+				 */
+				addnl_dvdr =
+					EMAC_DM646X_INTPRESCALE_MASK / prescale;
+
+				if (addnl_dvdr > 1) {
+					prescale *= addnl_dvdr;
+					if (coal_intvl > (EMAC_DM646X_CMINTMAX_INTVL
+								* addnl_dvdr))
+						coal_intvl = (EMAC_DM646X_CMINTMAX_INTVL
+								* addnl_dvdr);
+				} else {
+					addnl_dvdr = 1;
+					coal_intvl = EMAC_DM646X_CMINTMAX_INTVL;
+				}
 			}
-		}
 
-		num_interrupts = (1000 * addnl_dvdr) / coal_intvl;
+			num_interrupts = (1000 * addnl_dvdr) / coal_intvl;
+
+			int_ctrl |= EMAC_DM646X_INTPACEEN;
+			int_ctrl &= (~EMAC_DM646X_INTPRESCALE_MASK);
+			int_ctrl |= (prescale & EMAC_DM646X_INTPRESCALE_MASK);
+		}
 
-		int_ctrl |= EMAC_DM646X_INTPACEEN;
-		int_ctrl &= (~EMAC_DM646X_INTPRESCALE_MASK);
-		int_ctrl |= (prescale & EMAC_DM646X_INTPRESCALE_MASK);
 		emac_ctrl_write(EMAC_DM646X_CMINTCTRL, int_ctrl);
 
 		emac_ctrl_write(EMAC_DM646X_CMRXINTMAX, num_interrupts);
@@ -466,17 +467,21 @@ static int emac_set_coalesce(struct net_device *ndev,
 	default:
 		int_ctrl = emac_ctrl_read(EMAC_CTRL_EWINTTCNT);
 		int_ctrl &= (~EMAC_DM644X_EWINTCNT_MASK);
-		prescale = coal_intvl * priv->bus_freq_mhz;
-		if (prescale > EMAC_DM644X_EWINTCNT_MASK) {
-			prescale = EMAC_DM644X_EWINTCNT_MASK;
-			coal_intvl = prescale / priv->bus_freq_mhz;
+
+		if (coal->rx_coalesce_usecs) {
+			prescale = coal_intvl * priv->bus_freq_mhz;
+			if (prescale > EMAC_DM644X_EWINTCNT_MASK) {
+				prescale = EMAC_DM644X_EWINTCNT_MASK;
+				coal_intvl = prescale / priv->bus_freq_mhz;
+			}
 		}
+
 		emac_ctrl_write(EMAC_CTRL_EWINTTCNT, (int_ctrl | prescale));
 
 		break;
 	}
 
-	printk(KERN_INFO"Set coalesce to %d usecs.\n", coal_intvl);
+	netdev_info(ndev, "Set coalesce to %d usecs.\n", coal_intvl);
 	priv->coal_intvl = coal_intvl;
 
 	return 0;
-- 
2.30.2


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

* Re: [PATCH] net: davinci_emac: Fix interrupt pacing disable
  2021-11-01  9:21 [PATCH] net: davinci_emac: Fix interrupt pacing disable Maxim Kiselev
@ 2021-11-01 11:54 ` Grygorii Strashko
  2021-11-01 12:05   ` Maxim Kiselev
  2021-11-01 14:03 ` Maxim Kiselev
  1 sibling, 1 reply; 6+ messages in thread
From: Grygorii Strashko @ 2021-11-01 11:54 UTC (permalink / raw)
  To: Maxim Kiselev
  Cc: David S. Miller, Jakub Kicinski, Andrew Lunn, Yang Yingliang,
	Colin Ian King, Yufeng Mo, Michael Walle, Sriram, linux-omap,
	netdev, linux-kernel



On 01/11/2021 11:21, Maxim Kiselev wrote:
> This patch allows to use 0 for `coal->rx_coalesce_usecs` param to
> disable rx irq coalescing.
> 
> Previously we could enable rx irq coalescing via ethtool
> (For ex: `ethtool -C eth0 rx-usecs 2000`) but we couldn't disable
> it because this part rejects 0 value:
> 
>         if (!coal->rx_coalesce_usecs)
>                 return -EINVAL;
> 
> Fixes: 84da2658a619 ("TI DaVinci EMAC : Implement interrupt pacing
> functionality.")
> 
> Signed-off-by: Maxim Kiselev <bigunclemax@gmail.com>
> ---
>   drivers/net/ethernet/ti/davinci_emac.c | 77 ++++++++++++++------------
>   1 file changed, 41 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
> index e8291d8488391..a3a02c4e5eb68 100644
> --- a/drivers/net/ethernet/ti/davinci_emac.c
> +++ b/drivers/net/ethernet/ti/davinci_emac.c
> @@ -417,46 +417,47 @@ static int emac_set_coalesce(struct net_device *ndev,
>   			     struct netlink_ext_ack *extack)
>   {
>   	struct emac_priv *priv = netdev_priv(ndev);
> -	u32 int_ctrl, num_interrupts = 0;
> +	u32 int_ctrl = 0, num_interrupts = 0;
>   	u32 prescale = 0, addnl_dvdr = 1, coal_intvl = 0;
>   
> -	if (!coal->rx_coalesce_usecs)
> -		return -EINVAL;
> -
>   	coal_intvl = coal->rx_coalesce_usecs;

Wouldn't be more simple if you just handle !coal->rx_coalesce_usecs here and exit?
it seems you can just write 0 t0 INTCTRL.

>   
>   	switch (priv->version) {
>   	case EMAC_VERSION_2:
> -		int_ctrl =  emac_ctrl_read(EMAC_DM646X_CMINTCTRL);
> -		prescale = priv->bus_freq_mhz * 4;
> -
> -		if (coal_intvl < EMAC_DM646X_CMINTMIN_INTVL)
> -			coal_intvl = EMAC_DM646X_CMINTMIN_INTVL;
> -
> -		if (coal_intvl > EMAC_DM646X_CMINTMAX_INTVL) {
> -			/*
> -			 * Interrupt pacer works with 4us Pulse, we can
> -			 * throttle further by dilating the 4us pulse.
> -			 */
> -			addnl_dvdr = EMAC_DM646X_INTPRESCALE_MASK / prescale;
> -
> -			if (addnl_dvdr > 1) {
> -				prescale *= addnl_dvdr;
> -				if (coal_intvl > (EMAC_DM646X_CMINTMAX_INTVL
> -							* addnl_dvdr))
> -					coal_intvl = (EMAC_DM646X_CMINTMAX_INTVL
> -							* addnl_dvdr);
> -			} else {
> -				addnl_dvdr = 1;
> -				coal_intvl = EMAC_DM646X_CMINTMAX_INTVL;
> +		if (coal->rx_coalesce_usecs) {
> +			int_ctrl =  emac_ctrl_read(EMAC_DM646X_CMINTCTRL);
> +			prescale = priv->bus_freq_mhz * 4;
> +
> +			if (coal_intvl < EMAC_DM646X_CMINTMIN_INTVL)
> +				coal_intvl = EMAC_DM646X_CMINTMIN_INTVL;
> +
> +			if (coal_intvl > EMAC_DM646X_CMINTMAX_INTVL) {
> +				/*
> +				 * Interrupt pacer works with 4us Pulse, we can
> +				 * throttle further by dilating the 4us pulse.
> +				 */
> +				addnl_dvdr =
> +					EMAC_DM646X_INTPRESCALE_MASK / prescale;
> +
> +				if (addnl_dvdr > 1) {
> +					prescale *= addnl_dvdr;
> +					if (coal_intvl > (EMAC_DM646X_CMINTMAX_INTVL
> +								* addnl_dvdr))
> +						coal_intvl = (EMAC_DM646X_CMINTMAX_INTVL
> +								* addnl_dvdr);
> +				} else {
> +					addnl_dvdr = 1;
> +					coal_intvl = EMAC_DM646X_CMINTMAX_INTVL;
> +				}
>   			}
> -		}
>   
> -		num_interrupts = (1000 * addnl_dvdr) / coal_intvl;
> +			num_interrupts = (1000 * addnl_dvdr) / coal_intvl;
> +
> +			int_ctrl |= EMAC_DM646X_INTPACEEN;
> +			int_ctrl &= (~EMAC_DM646X_INTPRESCALE_MASK);
> +			int_ctrl |= (prescale & EMAC_DM646X_INTPRESCALE_MASK);
> +		}
>   
> -		int_ctrl |= EMAC_DM646X_INTPACEEN;
> -		int_ctrl &= (~EMAC_DM646X_INTPRESCALE_MASK);
> -		int_ctrl |= (prescale & EMAC_DM646X_INTPRESCALE_MASK);
>   		emac_ctrl_write(EMAC_DM646X_CMINTCTRL, int_ctrl);
>   
>   		emac_ctrl_write(EMAC_DM646X_CMRXINTMAX, num_interrupts);
> @@ -466,17 +467,21 @@ static int emac_set_coalesce(struct net_device *ndev,
>   	default:
>   		int_ctrl = emac_ctrl_read(EMAC_CTRL_EWINTTCNT);
>   		int_ctrl &= (~EMAC_DM644X_EWINTCNT_MASK);
> -		prescale = coal_intvl * priv->bus_freq_mhz;
> -		if (prescale > EMAC_DM644X_EWINTCNT_MASK) {
> -			prescale = EMAC_DM644X_EWINTCNT_MASK;
> -			coal_intvl = prescale / priv->bus_freq_mhz;
> +
> +		if (coal->rx_coalesce_usecs) {
> +			prescale = coal_intvl * priv->bus_freq_mhz;
> +			if (prescale > EMAC_DM644X_EWINTCNT_MASK) {
> +				prescale = EMAC_DM644X_EWINTCNT_MASK;
> +				coal_intvl = prescale / priv->bus_freq_mhz;
> +			}
>   		}
> +
>   		emac_ctrl_write(EMAC_CTRL_EWINTTCNT, (int_ctrl | prescale));
>   
>   		break;
>   	}
>   
> -	printk(KERN_INFO"Set coalesce to %d usecs.\n", coal_intvl);
> +	netdev_info(ndev, "Set coalesce to %d usecs.\n", coal_intvl);
>   	priv->coal_intvl = coal_intvl;
>   
>   	return 0;
> 

-- 
Best regards,
grygorii

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

* Re: [PATCH] net: davinci_emac: Fix interrupt pacing disable
  2021-11-01 11:54 ` Grygorii Strashko
@ 2021-11-01 12:05   ` Maxim Kiselev
  2021-11-01 12:33     ` Grygorii Strashko
  0 siblings, 1 reply; 6+ messages in thread
From: Maxim Kiselev @ 2021-11-01 12:05 UTC (permalink / raw)
  To: Grygorii Strashko
  Cc: David S. Miller, Jakub Kicinski, Andrew Lunn, Yang Yingliang,
	Colin Ian King, Yufeng Mo, Michael Walle, Sriram, linux-omap,
	netdev, linux-kernel

Yes, I can write 0 to INTCTRL for ` case EMAC_VERSION_2` but we also
need to handle `default case`

пн, 1 нояб. 2021 г. в 14:54, Grygorii Strashko <grygorii.strashko@ti.com>:
>
>
>
> On 01/11/2021 11:21, Maxim Kiselev wrote:
> > This patch allows to use 0 for `coal->rx_coalesce_usecs` param to
> > disable rx irq coalescing.
> >
> > Previously we could enable rx irq coalescing via ethtool
> > (For ex: `ethtool -C eth0 rx-usecs 2000`) but we couldn't disable
> > it because this part rejects 0 value:
> >
> >         if (!coal->rx_coalesce_usecs)
> >                 return -EINVAL;
> >
> > Fixes: 84da2658a619 ("TI DaVinci EMAC : Implement interrupt pacing
> > functionality.")
> >
> > Signed-off-by: Maxim Kiselev <bigunclemax@gmail.com>
> > ---
> >   drivers/net/ethernet/ti/davinci_emac.c | 77 ++++++++++++++------------
> >   1 file changed, 41 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
> > index e8291d8488391..a3a02c4e5eb68 100644
> > --- a/drivers/net/ethernet/ti/davinci_emac.c
> > +++ b/drivers/net/ethernet/ti/davinci_emac.c
> > @@ -417,46 +417,47 @@ static int emac_set_coalesce(struct net_device *ndev,
> >                            struct netlink_ext_ack *extack)
> >   {
> >       struct emac_priv *priv = netdev_priv(ndev);
> > -     u32 int_ctrl, num_interrupts = 0;
> > +     u32 int_ctrl = 0, num_interrupts = 0;
> >       u32 prescale = 0, addnl_dvdr = 1, coal_intvl = 0;
> >
> > -     if (!coal->rx_coalesce_usecs)
> > -             return -EINVAL;
> > -
> >       coal_intvl = coal->rx_coalesce_usecs;
>
> Wouldn't be more simple if you just handle !coal->rx_coalesce_usecs here and exit?
> it seems you can just write 0 t0 INTCTRL.
>
> >
> >       switch (priv->version) {
> >       case EMAC_VERSION_2:
> > -             int_ctrl =  emac_ctrl_read(EMAC_DM646X_CMINTCTRL);
> > -             prescale = priv->bus_freq_mhz * 4;
> > -
> > -             if (coal_intvl < EMAC_DM646X_CMINTMIN_INTVL)
> > -                     coal_intvl = EMAC_DM646X_CMINTMIN_INTVL;
> > -
> > -             if (coal_intvl > EMAC_DM646X_CMINTMAX_INTVL) {
> > -                     /*
> > -                      * Interrupt pacer works with 4us Pulse, we can
> > -                      * throttle further by dilating the 4us pulse.
> > -                      */
> > -                     addnl_dvdr = EMAC_DM646X_INTPRESCALE_MASK / prescale;
> > -
> > -                     if (addnl_dvdr > 1) {
> > -                             prescale *= addnl_dvdr;
> > -                             if (coal_intvl > (EMAC_DM646X_CMINTMAX_INTVL
> > -                                                     * addnl_dvdr))
> > -                                     coal_intvl = (EMAC_DM646X_CMINTMAX_INTVL
> > -                                                     * addnl_dvdr);
> > -                     } else {
> > -                             addnl_dvdr = 1;
> > -                             coal_intvl = EMAC_DM646X_CMINTMAX_INTVL;
> > +             if (coal->rx_coalesce_usecs) {
> > +                     int_ctrl =  emac_ctrl_read(EMAC_DM646X_CMINTCTRL);
> > +                     prescale = priv->bus_freq_mhz * 4;
> > +
> > +                     if (coal_intvl < EMAC_DM646X_CMINTMIN_INTVL)
> > +                             coal_intvl = EMAC_DM646X_CMINTMIN_INTVL;
> > +
> > +                     if (coal_intvl > EMAC_DM646X_CMINTMAX_INTVL) {
> > +                             /*
> > +                              * Interrupt pacer works with 4us Pulse, we can
> > +                              * throttle further by dilating the 4us pulse.
> > +                              */
> > +                             addnl_dvdr =
> > +                                     EMAC_DM646X_INTPRESCALE_MASK / prescale;
> > +
> > +                             if (addnl_dvdr > 1) {
> > +                                     prescale *= addnl_dvdr;
> > +                                     if (coal_intvl > (EMAC_DM646X_CMINTMAX_INTVL
> > +                                                             * addnl_dvdr))
> > +                                             coal_intvl = (EMAC_DM646X_CMINTMAX_INTVL
> > +                                                             * addnl_dvdr);
> > +                             } else {
> > +                                     addnl_dvdr = 1;
> > +                                     coal_intvl = EMAC_DM646X_CMINTMAX_INTVL;
> > +                             }
> >                       }
> > -             }
> >
> > -             num_interrupts = (1000 * addnl_dvdr) / coal_intvl;
> > +                     num_interrupts = (1000 * addnl_dvdr) / coal_intvl;
> > +
> > +                     int_ctrl |= EMAC_DM646X_INTPACEEN;
> > +                     int_ctrl &= (~EMAC_DM646X_INTPRESCALE_MASK);
> > +                     int_ctrl |= (prescale & EMAC_DM646X_INTPRESCALE_MASK);
> > +             }
> >
> > -             int_ctrl |= EMAC_DM646X_INTPACEEN;
> > -             int_ctrl &= (~EMAC_DM646X_INTPRESCALE_MASK);
> > -             int_ctrl |= (prescale & EMAC_DM646X_INTPRESCALE_MASK);
> >               emac_ctrl_write(EMAC_DM646X_CMINTCTRL, int_ctrl);
> >
> >               emac_ctrl_write(EMAC_DM646X_CMRXINTMAX, num_interrupts);
> > @@ -466,17 +467,21 @@ static int emac_set_coalesce(struct net_device *ndev,
> >       default:
> >               int_ctrl = emac_ctrl_read(EMAC_CTRL_EWINTTCNT);
> >               int_ctrl &= (~EMAC_DM644X_EWINTCNT_MASK);
> > -             prescale = coal_intvl * priv->bus_freq_mhz;
> > -             if (prescale > EMAC_DM644X_EWINTCNT_MASK) {
> > -                     prescale = EMAC_DM644X_EWINTCNT_MASK;
> > -                     coal_intvl = prescale / priv->bus_freq_mhz;
> > +
> > +             if (coal->rx_coalesce_usecs) {
> > +                     prescale = coal_intvl * priv->bus_freq_mhz;
> > +                     if (prescale > EMAC_DM644X_EWINTCNT_MASK) {
> > +                             prescale = EMAC_DM644X_EWINTCNT_MASK;
> > +                             coal_intvl = prescale / priv->bus_freq_mhz;
> > +                     }
> >               }
> > +
> >               emac_ctrl_write(EMAC_CTRL_EWINTTCNT, (int_ctrl | prescale));
> >
> >               break;
> >       }
> >
> > -     printk(KERN_INFO"Set coalesce to %d usecs.\n", coal_intvl);
> > +     netdev_info(ndev, "Set coalesce to %d usecs.\n", coal_intvl);
> >       priv->coal_intvl = coal_intvl;
> >
> >       return 0;
> >
>
> --
> Best regards,
> grygorii

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

* Re: [PATCH] net: davinci_emac: Fix interrupt pacing disable
  2021-11-01 12:05   ` Maxim Kiselev
@ 2021-11-01 12:33     ` Grygorii Strashko
  0 siblings, 0 replies; 6+ messages in thread
From: Grygorii Strashko @ 2021-11-01 12:33 UTC (permalink / raw)
  To: Maxim Kiselev
  Cc: David S. Miller, Jakub Kicinski, Andrew Lunn, Yang Yingliang,
	Colin Ian King, Yufeng Mo, Michael Walle, Sriram, linux-omap,
	netdev, linux-kernel



On 01/11/2021 14:05, Maxim Kiselev wrote:
> Yes, I can write 0 to INTCTRL for ` case EMAC_VERSION_2` but we also
> need to handle `default case`
	
pls, do not top post.

> 
> пн, 1 нояб. 2021 г. в 14:54, Grygorii Strashko <grygorii.strashko@ti.com>:
>>
>>
>>
>> On 01/11/2021 11:21, Maxim Kiselev wrote:
>>> This patch allows to use 0 for `coal->rx_coalesce_usecs` param to
>>> disable rx irq coalescing.
>>>
>>> Previously we could enable rx irq coalescing via ethtool
>>> (For ex: `ethtool -C eth0 rx-usecs 2000`) but we couldn't disable
>>> it because this part rejects 0 value:
>>>
>>>          if (!coal->rx_coalesce_usecs)
>>>                  return -EINVAL;
>>>
>>> Fixes: 84da2658a619 ("TI DaVinci EMAC : Implement interrupt pacing
>>> functionality.")
>>>
>>> Signed-off-by: Maxim Kiselev <bigunclemax@gmail.com>
>>> ---
>>>    drivers/net/ethernet/ti/davinci_emac.c | 77 ++++++++++++++------------
>>>    1 file changed, 41 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
>>> index e8291d8488391..a3a02c4e5eb68 100644
>>> --- a/drivers/net/ethernet/ti/davinci_emac.c
>>> +++ b/drivers/net/ethernet/ti/davinci_emac.c
>>> @@ -417,46 +417,47 @@ static int emac_set_coalesce(struct net_device *ndev,
>>>                             struct netlink_ext_ack *extack)
>>>    {
>>>        struct emac_priv *priv = netdev_priv(ndev);
>>> -     u32 int_ctrl, num_interrupts = 0;
>>> +     u32 int_ctrl = 0, num_interrupts = 0;
>>>        u32 prescale = 0, addnl_dvdr = 1, coal_intvl = 0;
>>>
>>> -     if (!coal->rx_coalesce_usecs)
>>> -             return -EINVAL;
>>> -
>>>        coal_intvl = coal->rx_coalesce_usecs;
>>
>> Wouldn't be more simple if you just handle !coal->rx_coalesce_usecs here and exit?
>> it seems you can just write 0 t0 INTCTRL.

nothing prevents you from handling all cases here, like

if (!coal->rx_coalesce_usecs)
	switch (priv->version) {
	case EMAC_VERSION_2:
		emac_ctrl_write(EMAC_DM646X_CMINTCTRL, 0);
		break;
	default:
		emac_ctrl_write(EMAC_CTRL_EWINTTCNT, 0);
		break;
	}

	return 0;
}

No?

>>
>>>
>>>        switch (priv->version) {
>>>        case EMAC_VERSION_2:
>>> -             int_ctrl =  emac_ctrl_read(EMAC_DM646X_CMINTCTRL);
>>> -             prescale = priv->bus_freq_mhz * 4;
>>> -
>>> -             if (coal_intvl < EMAC_DM646X_CMINTMIN_INTVL)
>>> -                     coal_intvl = EMAC_DM646X_CMINTMIN_INTVL;
>>> -
>>> -             if (coal_intvl > EMAC_DM646X_CMINTMAX_INTVL) {
>>> -                     /*
>>> -                      * Interrupt pacer works with 4us Pulse, we can
>>> -                      * throttle further by dilating the 4us pulse.
>>> -                      */
>>> -                     addnl_dvdr = EMAC_DM646X_INTPRESCALE_MASK / prescale;
>>> -
>>> -                     if (addnl_dvdr > 1) {
>>> -                             prescale *= addnl_dvdr;
>>> -                             if (coal_intvl > (EMAC_DM646X_CMINTMAX_INTVL
>>> -                                                     * addnl_dvdr))
>>> -                                     coal_intvl = (EMAC_DM646X_CMINTMAX_INTVL
>>> -                                                     * addnl_dvdr);
>>> -                     } else {
>>> -                             addnl_dvdr = 1;
>>> -                             coal_intvl = EMAC_DM646X_CMINTMAX_INTVL;
>>> +             if (coal->rx_coalesce_usecs) {
>>> +                     int_ctrl =  emac_ctrl_read(EMAC_DM646X_CMINTCTRL);
>>> +                     prescale = priv->bus_freq_mhz * 4;
>>> +
>>> +                     if (coal_intvl < EMAC_DM646X_CMINTMIN_INTVL)
>>> +                             coal_intvl = EMAC_DM646X_CMINTMIN_INTVL;
>>> +
>>> +                     if (coal_intvl > EMAC_DM646X_CMINTMAX_INTVL) {
>>> +                             /*
>>> +                              * Interrupt pacer works with 4us Pulse, we can
>>> +                              * throttle further by dilating the 4us pulse.
>>> +                              */
>>> +                             addnl_dvdr =
>>> +                                     EMAC_DM646X_INTPRESCALE_MASK / prescale;
>>> +
>>> +                             if (addnl_dvdr > 1) {
>>> +                                     prescale *= addnl_dvdr;
>>> +                                     if (coal_intvl > (EMAC_DM646X_CMINTMAX_INTVL
>>> +                                                             * addnl_dvdr))
>>> +                                             coal_intvl = (EMAC_DM646X_CMINTMAX_INTVL
>>> +                                                             * addnl_dvdr);
>>> +                             } else {
>>> +                                     addnl_dvdr = 1;
>>> +                                     coal_intvl = EMAC_DM646X_CMINTMAX_INTVL;
>>> +                             }
>>>                        }
>>> -             }
>>>
>>> -             num_interrupts = (1000 * addnl_dvdr) / coal_intvl;
>>> +                     num_interrupts = (1000 * addnl_dvdr) / coal_intvl;
>>> +
>>> +                     int_ctrl |= EMAC_DM646X_INTPACEEN;
>>> +                     int_ctrl &= (~EMAC_DM646X_INTPRESCALE_MASK);
>>> +                     int_ctrl |= (prescale & EMAC_DM646X_INTPRESCALE_MASK);
>>> +             }
>>>
>>> -             int_ctrl |= EMAC_DM646X_INTPACEEN;
>>> -             int_ctrl &= (~EMAC_DM646X_INTPRESCALE_MASK);
>>> -             int_ctrl |= (prescale & EMAC_DM646X_INTPRESCALE_MASK);
>>>                emac_ctrl_write(EMAC_DM646X_CMINTCTRL, int_ctrl);
>>>
>>>                emac_ctrl_write(EMAC_DM646X_CMRXINTMAX, num_interrupts);
>>> @@ -466,17 +467,21 @@ static int emac_set_coalesce(struct net_device *ndev,
>>>        default:
>>>                int_ctrl = emac_ctrl_read(EMAC_CTRL_EWINTTCNT);
>>>                int_ctrl &= (~EMAC_DM644X_EWINTCNT_MASK);
>>> -             prescale = coal_intvl * priv->bus_freq_mhz;
>>> -             if (prescale > EMAC_DM644X_EWINTCNT_MASK) {
>>> -                     prescale = EMAC_DM644X_EWINTCNT_MASK;
>>> -                     coal_intvl = prescale / priv->bus_freq_mhz;
>>> +
>>> +             if (coal->rx_coalesce_usecs) {
>>> +                     prescale = coal_intvl * priv->bus_freq_mhz;
>>> +                     if (prescale > EMAC_DM644X_EWINTCNT_MASK) {
>>> +                             prescale = EMAC_DM644X_EWINTCNT_MASK;
>>> +                             coal_intvl = prescale / priv->bus_freq_mhz;
>>> +                     }
>>>                }
>>> +
>>>                emac_ctrl_write(EMAC_CTRL_EWINTTCNT, (int_ctrl | prescale));
>>>
>>>                break;
>>>        }
>>>
>>> -     printk(KERN_INFO"Set coalesce to %d usecs.\n", coal_intvl);
>>> +     netdev_info(ndev, "Set coalesce to %d usecs.\n", coal_intvl);
>>>        priv->coal_intvl = coal_intvl;
>>>
>>>        return 0;
>>>
>>
>> --
>> Best regards,
>> grygorii

-- 
Best regards,
grygorii

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

* Re: [PATCH] net: davinci_emac: Fix interrupt pacing disable
  2021-11-01  9:21 [PATCH] net: davinci_emac: Fix interrupt pacing disable Maxim Kiselev
  2021-11-01 11:54 ` Grygorii Strashko
@ 2021-11-01 14:03 ` Maxim Kiselev
  2021-11-01 15:13   ` Grygorii Strashko
  1 sibling, 1 reply; 6+ messages in thread
From: Maxim Kiselev @ 2021-11-01 14:03 UTC (permalink / raw)
  Cc: Grygorii Strashko, David S. Miller, Jakub Kicinski, Andrew Lunn,
	Yang Yingliang, Colin Ian King, Yufeng Mo, Michael Walle, Sriram,
	linux-omap, netdev, linux-kernel

From ca26bf62366f249a2ed360b00c1883652848bfdc Mon Sep 17 00:00:00 2001
From: Maxim Kiselev <bigunclemax@gmail.com>
Date: Mon, 1 Nov 2021 16:37:12 +0300
Subject: [PATCH v2] net: davinci_emac: Fix interrupt pacing disable

This patch allows to use 0 for `coal->rx_coalesce_usecs` param to
disable rx irq coalescing.

Previously we could enable rx irq coalescing via ethtool
(For ex: `ethtool -C eth0 rx-usecs 2000`) but we couldn't disable
it because this part rejects 0 value:

       if (!coal->rx_coalesce_usecs)
               return -EINVAL;

Fixes: 84da2658a619 ("TI DaVinci EMAC : Implement interrupt pacing
functionality.")

Signed-off-by: Maxim Kiselev <bigunclemax@gmail.com>
---
Changes v1 -> v2 (after review of Grygorii Strashko):

 - Simplify !coal->rx_coalesce_usecs handler

---
 drivers/net/ethernet/ti/davinci_emac.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ti/davinci_emac.c
b/drivers/net/ethernet/ti/davinci_emac.c
index e8291d8488391..d243ca5dfde00 100644
--- a/drivers/net/ethernet/ti/davinci_emac.c
+++ b/drivers/net/ethernet/ti/davinci_emac.c
@@ -420,8 +420,20 @@ static int emac_set_coalesce(struct net_device *ndev,
        u32 int_ctrl, num_interrupts = 0;
        u32 prescale = 0, addnl_dvdr = 1, coal_intvl = 0;

-       if (!coal->rx_coalesce_usecs)
-               return -EINVAL;
+       if (!coal->rx_coalesce_usecs) {
+               priv->coal_intvl = 0;
+
+               switch (priv->version) {
+               case EMAC_VERSION_2:
+                       emac_ctrl_write(EMAC_DM646X_CMINTCTRL, 0);
+                       break;
+               default:
+                       emac_ctrl_write(EMAC_CTRL_EWINTTCNT, 0);
+                       break;
+               }
+
+               return 0;
+       }

        coal_intvl = coal->rx_coalesce_usecs;

-- 
2.30.2

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

* Re: [PATCH] net: davinci_emac: Fix interrupt pacing disable
  2021-11-01 14:03 ` Maxim Kiselev
@ 2021-11-01 15:13   ` Grygorii Strashko
  0 siblings, 0 replies; 6+ messages in thread
From: Grygorii Strashko @ 2021-11-01 15:13 UTC (permalink / raw)
  To: Maxim Kiselev
  Cc: David S. Miller, Jakub Kicinski, Andrew Lunn, Yang Yingliang,
	Colin Ian King, Yufeng Mo, Michael Walle, Sriram, linux-omap,
	netdev, linux-kernel



On 01/11/2021 16:03, Maxim Kiselev wrote:
>  From ca26bf62366f249a2ed360b00c1883652848bfdc Mon Sep 17 00:00:00 2001
> From: Maxim Kiselev <bigunclemax@gmail.com>
> Date: Mon, 1 Nov 2021 16:37:12 +0300
> Subject: [PATCH v2] net: davinci_emac: Fix interrupt pacing disable
> 
> This patch allows to use 0 for `coal->rx_coalesce_usecs` param to
> disable rx irq coalescing.
> 
> Previously we could enable rx irq coalescing via ethtool
> (For ex: `ethtool -C eth0 rx-usecs 2000`) but we couldn't disable
> it because this part rejects 0 value:
> 
>         if (!coal->rx_coalesce_usecs)
>                 return -EINVAL;
> 
> Fixes: 84da2658a619 ("TI DaVinci EMAC : Implement interrupt pacing
> functionality.")
> 
> Signed-off-by: Maxim Kiselev <bigunclemax@gmail.com>
> ---
> Changes v1 -> v2 (after review of Grygorii Strashko):
> 
>   - Simplify !coal->rx_coalesce_usecs handler


Do not send v2 as reply to v1 - pls, re-send as it will not hit
https://patchwork.kernel.org/project/netdevbpf/list/ properly

Otherwise:
Reviewed-by: Grygorii Strashko <grygorii.strashko@ti.com>

> 
> ---
>   drivers/net/ethernet/ti/davinci_emac.c | 16 ++++++++++++++--
>   1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/davinci_emac.c
> b/drivers/net/ethernet/ti/davinci_emac.c
> index e8291d8488391..d243ca5dfde00 100644
> --- a/drivers/net/ethernet/ti/davinci_emac.c
> +++ b/drivers/net/ethernet/ti/davinci_emac.c
> @@ -420,8 +420,20 @@ static int emac_set_coalesce(struct net_device *ndev,
>          u32 int_ctrl, num_interrupts = 0;
>          u32 prescale = 0, addnl_dvdr = 1, coal_intvl = 0;
> 
> -       if (!coal->rx_coalesce_usecs)
> -               return -EINVAL;
> +       if (!coal->rx_coalesce_usecs) {
> +               priv->coal_intvl = 0;
> +
> +               switch (priv->version) {
> +               case EMAC_VERSION_2:
> +                       emac_ctrl_write(EMAC_DM646X_CMINTCTRL, 0);
> +                       break;
> +               default:
> +                       emac_ctrl_write(EMAC_CTRL_EWINTTCNT, 0);
> +                       break;
> +               }
> +
> +               return 0;
> +       }
> 
>          coal_intvl = coal->rx_coalesce_usecs;
> 

-- 
Best regards,
grygorii

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

end of thread, other threads:[~2021-11-01 15:14 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-01  9:21 [PATCH] net: davinci_emac: Fix interrupt pacing disable Maxim Kiselev
2021-11-01 11:54 ` Grygorii Strashko
2021-11-01 12:05   ` Maxim Kiselev
2021-11-01 12:33     ` Grygorii Strashko
2021-11-01 14:03 ` Maxim Kiselev
2021-11-01 15:13   ` Grygorii Strashko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.