2021-11-01 09:38:01

by Maksim Kiselev

Subject: [PATCH] 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 <[email protected]>
---
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

2021-11-01 11:55:06

by Grygorii Strashko

Subject: Re: [PATCH] net: davinci_emac: Fix interrupt pacing disable

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 <[email protected]>
> ---
> 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

2021-11-01 12:06:11

by Maksim Kiselev

Subject: Re: [PATCH] net: davinci_emac: Fix interrupt pacing disable

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 <[email protected]>:
>
>
>
> 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 <[email protected]>
> > ---
> > 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

2021-11-01 12:34:33

by Grygorii Strashko

Subject: Re: [PATCH] net: davinci_emac: Fix interrupt pacing disable

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 <[email protected]>:
>>
>>
>>
>> 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 <[email protected]>
>>> ---
>>> 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

2021-11-01 14:04:39

by Maksim Kiselev

Subject: Re: [PATCH] net: davinci_emac: Fix interrupt pacing disable

From ca26bf62366f249a2ed360b00c1883652848bfdc Mon Sep 17 00:00:00 2001
From: Maxim Kiselev <[email protected]>
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 <[email protected]>
---
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

2021-11-01 15:14:13

by Grygorii Strashko

Subject: Re: [PATCH] net: davinci_emac: Fix interrupt pacing disable

On 01/11/2021 16:03, Maxim Kiselev wrote:
> From ca26bf62366f249a2ed360b00c1883652848bfdc Mon Sep 17 00:00:00 2001
> From: Maxim Kiselev <[email protected]>
> 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 <[email protected]>
> ---
> 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 <[email protected]>

>
> ---
> 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