[RFC,5/5] can: m_can: Add hrtimer to generate software interrupt

  Add a hrtimer to MCAN struct. Each MCAN will have its own
hrtimer instantiated if there is no hardware interrupt found.

The hrtimer will generate a software interrupt every 1 ms. In
hrtimer callback, we check if there is a transaction pending by
reading a register, then process by calling the isr if there is.

Signed-off-by: Judith Mendez <jm@ti.com>
---
 drivers/net/can/m_can/m_can.c          | 24 ++++++++++++++++++++++--
 drivers/net/can/m_can/m_can.h          |  3 +++
 drivers/net/can/m_can/m_can_platform.c |  9 +++++++--
 3 files changed, 32 insertions(+), 4 deletions(-)
  

On 13.04.2023 17:30:51, Judith Mendez wrote:
> Add a hrtimer to MCAN struct. Each MCAN will have its own
> hrtimer instantiated if there is no hardware interrupt found.
> 
> The hrtimer will generate a software interrupt every 1 ms. In

Are you sure about the 1ms?

> hrtimer callback, we check if there is a transaction pending by
> reading a register, then process by calling the isr if there is.
> 
> Signed-off-by: Judith Mendez <jm@ti.com>
> ---
>  drivers/net/can/m_can/m_can.c          | 24 ++++++++++++++++++++++--
>  drivers/net/can/m_can/m_can.h          |  3 +++
>  drivers/net/can/m_can/m_can_platform.c |  9 +++++++--
>  3 files changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index 8e83d6963d85..bb9d53f4d3cc 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -23,6 +23,7 @@
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/hrtimer.h>
>  
>  #include "m_can.h"
>  
> @@ -1584,6 +1585,11 @@ static int m_can_close(struct net_device *dev)
>  	if (!cdev->is_peripheral)
>  		napi_disable(&cdev->napi);
>  
> +	if (dev->irq < 0) {
> +		dev_info(cdev->dev, "Disabling the hrtimer\n");

Make it a dev_dbg() or remove completely.

> +		hrtimer_cancel(&cdev->hrtimer);
> +	}
> +
>  	m_can_stop(dev);
>  	m_can_clk_stop(cdev);
>  	free_irq(dev->irq, dev);
> @@ -1792,6 +1798,19 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
>  	return NETDEV_TX_OK;
>  }
>  
> +enum hrtimer_restart hrtimer_callback(struct hrtimer *timer)
> +{
> +	irqreturn_t ret;

never read value?

> +	struct m_can_classdev *cdev =
> +		container_of(timer, struct m_can_classdev, hrtimer);
> +
> +	ret = m_can_isr(0, cdev->net);
> +
> +	hrtimer_forward_now(timer, ns_to_ktime(5 * NSEC_PER_MSEC));

There's ms_to_ktime()....and the "5" doesn't match your patch
description.

> +
> +	return HRTIMER_RESTART;
> +}
> +
>  static int m_can_open(struct net_device *dev)
>  {
>  	struct m_can_classdev *cdev = netdev_priv(dev);
> @@ -1836,8 +1855,9 @@ static int m_can_open(struct net_device *dev)
>  	}
>  
>  	if (err < 0) {
> -		netdev_err(dev, "failed to request interrupt\n");
> -		goto exit_irq_fail;
> +		dev_info(cdev->dev, "Enabling the hrtimer\n");
> +		cdev->hrtimer.function = &hrtimer_callback;
> +		hrtimer_start(&cdev->hrtimer, ns_to_ktime(0), HRTIMER_MODE_REL_PINNED);

IMHO it makes no sense to request an IRQ if the device doesn't have one,
and then try to fix up things in the error path. What about this?

--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1831,9 +1831,11 @@ static int m_can_open(struct net_device *dev)
                 err = request_threaded_irq(dev->irq, NULL, m_can_isr,
                                            IRQF_ONESHOT,
                                            dev->name, dev);
-        } else {
+        } else if (dev->irq) {
                 err = request_irq(dev->irq, m_can_isr, IRQF_SHARED, dev->name,
                                   dev);
+        } else {
+                // polling
         }
 
         if (err < 0) {

>  	}
>  
>  	/* start the m_can controller */
> diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
> index a839dc71dc9b..ed046d77fdb9 100644
> --- a/drivers/net/can/m_can/m_can.h
> +++ b/drivers/net/can/m_can/m_can.h
> @@ -28,6 +28,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/slab.h>
>  #include <linux/uaccess.h>
> +#include <linux/hrtimer.h>
>  
>  /* m_can lec values */
>  enum m_can_lec_type {
> @@ -93,6 +94,8 @@ struct m_can_classdev {
>  	int is_peripheral;
>  
>  	struct mram_cfg mcfg[MRAM_CFG_NUM];
> +
> +	struct hrtimer hrtimer;
>  };
>  
>  struct m_can_classdev *m_can_class_allocate_dev(struct device *dev, int sizeof_priv);
> diff --git a/drivers/net/can/m_can/m_can_platform.c b/drivers/net/can/m_can/m_can_platform.c
> index 9c1dcf838006..53e1648e9dab 100644
> --- a/drivers/net/can/m_can/m_can_platform.c
> +++ b/drivers/net/can/m_can/m_can_platform.c
> @@ -7,6 +7,7 @@
>  
>  #include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
> +#include <linux/hrtimer.h>
>  
>  #include "m_can.h"
>  
> @@ -98,8 +99,12 @@ static int m_can_plat_probe(struct platform_device *pdev)
>  	addr = devm_platform_ioremap_resource_byname(pdev, "m_can");
>  	irq = platform_get_irq_byname(pdev, "int0");
>  	if (IS_ERR(addr) || irq < 0) {

What about the IS_ERR(addr) case?

> -		ret = -EINVAL;
> -		goto probe_fail;
> +		if (irq == -EPROBE_DEFER) {
> +			ret = -EPROBE_DEFER;
> +			goto probe_fail;
> +		}
> +		dev_info(mcan_class->dev, "Failed to get irq, initialize hrtimer\n");
> +		hrtimer_init(&mcan_class->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_PINNED);

I don't like it when polling is unconditionally set up in case of an irq
error. I'm not sure if we need an explicit device tree property....

>  	}
>  
>  	/* message ram could be shared */
> -- 
> 2.17.1
> 
>

Marc
  

On 4/14/23 20:20, Marc Kleine-Budde wrote:
> On 13.04.2023 17:30:51, Judith Mendez wrote:
>> Add a hrtimer to MCAN struct. Each MCAN will have its own
>> hrtimer instantiated if there is no hardware interrupt found.
>>
>> The hrtimer will generate a software interrupt every 1 ms. In
> 
> Are you sure about the 1ms?

The "shortest" 11 bit CAN ID CAN frame is a Classical CAN frame with DLC 
= 0 and 1 Mbit/s (arbitration) bitrate. This should be 48 bits @1Mbit => 
~50 usecs

So it should be something about

     50 usecs * (FIFO queue len - 2)

if there is some FIFO involved, right?

Best regards,
Oliver

>> hrtimer callback, we check if there is a transaction pending by
>> reading a register, then process by calling the isr if there is.
>>
>> Signed-off-by: Judith Mendez <jm@ti.com>
>> ---
>>   drivers/net/can/m_can/m_can.c          | 24 ++++++++++++++++++++++--
>>   drivers/net/can/m_can/m_can.h          |  3 +++
>>   drivers/net/can/m_can/m_can_platform.c |  9 +++++++--
>>   3 files changed, 32 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
>> index 8e83d6963d85..bb9d53f4d3cc 100644
>> --- a/drivers/net/can/m_can/m_can.c
>> +++ b/drivers/net/can/m_can/m_can.c
>> @@ -23,6 +23,7 @@
>>   #include <linux/pinctrl/consumer.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/pm_runtime.h>
>> +#include <linux/hrtimer.h>
>>   
>>   #include "m_can.h"
>>   
>> @@ -1584,6 +1585,11 @@ static int m_can_close(struct net_device *dev)
>>   	if (!cdev->is_peripheral)
>>   		napi_disable(&cdev->napi);
>>   
>> +	if (dev->irq < 0) {
>> +		dev_info(cdev->dev, "Disabling the hrtimer\n");
> 
> Make it a dev_dbg() or remove completely.
> 
>> +		hrtimer_cancel(&cdev->hrtimer);
>> +	}
>> +
>>   	m_can_stop(dev);
>>   	m_can_clk_stop(cdev);
>>   	free_irq(dev->irq, dev);
>> @@ -1792,6 +1798,19 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
>>   	return NETDEV_TX_OK;
>>   }
>>   
>> +enum hrtimer_restart hrtimer_callback(struct hrtimer *timer)
>> +{
>> +	irqreturn_t ret;
> 
> never read value?
> 
>> +	struct m_can_classdev *cdev =
>> +		container_of(timer, struct m_can_classdev, hrtimer);
>> +
>> +	ret = m_can_isr(0, cdev->net);
>> +
>> +	hrtimer_forward_now(timer, ns_to_ktime(5 * NSEC_PER_MSEC));
> 
> There's ms_to_ktime()....and the "5" doesn't match your patch
> description.
> 
>> +
>> +	return HRTIMER_RESTART;
>> +}
>> +
>>   static int m_can_open(struct net_device *dev)
>>   {
>>   	struct m_can_classdev *cdev = netdev_priv(dev);
>> @@ -1836,8 +1855,9 @@ static int m_can_open(struct net_device *dev)
>>   	}
>>   
>>   	if (err < 0) {
>> -		netdev_err(dev, "failed to request interrupt\n");
>> -		goto exit_irq_fail;
>> +		dev_info(cdev->dev, "Enabling the hrtimer\n");
>> +		cdev->hrtimer.function = &hrtimer_callback;
>> +		hrtimer_start(&cdev->hrtimer, ns_to_ktime(0), HRTIMER_MODE_REL_PINNED);
> 
> IMHO it makes no sense to request an IRQ if the device doesn't have one,
> and then try to fix up things in the error path. What about this?
> 
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -1831,9 +1831,11 @@ static int m_can_open(struct net_device *dev)
>                   err = request_threaded_irq(dev->irq, NULL, m_can_isr,
>                                              IRQF_ONESHOT,
>                                              dev->name, dev);
> -        } else {
> +        } else if (dev->irq) {
>                   err = request_irq(dev->irq, m_can_isr, IRQF_SHARED, dev->name,
>                                     dev);
> +        } else {
> +                // polling
>           }
>   
>           if (err < 0) {
> 
>>   	}
>>   
>>   	/* start the m_can controller */
>> diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
>> index a839dc71dc9b..ed046d77fdb9 100644
>> --- a/drivers/net/can/m_can/m_can.h
>> +++ b/drivers/net/can/m_can/m_can.h
>> @@ -28,6 +28,7 @@
>>   #include <linux/pm_runtime.h>
>>   #include <linux/slab.h>
>>   #include <linux/uaccess.h>
>> +#include <linux/hrtimer.h>
>>   
>>   /* m_can lec values */
>>   enum m_can_lec_type {
>> @@ -93,6 +94,8 @@ struct m_can_classdev {
>>   	int is_peripheral;
>>   
>>   	struct mram_cfg mcfg[MRAM_CFG_NUM];
>> +
>> +	struct hrtimer hrtimer;
>>   };
>>   
>>   struct m_can_classdev *m_can_class_allocate_dev(struct device *dev, int sizeof_priv);
>> diff --git a/drivers/net/can/m_can/m_can_platform.c b/drivers/net/can/m_can/m_can_platform.c
>> index 9c1dcf838006..53e1648e9dab 100644
>> --- a/drivers/net/can/m_can/m_can_platform.c
>> +++ b/drivers/net/can/m_can/m_can_platform.c
>> @@ -7,6 +7,7 @@
>>   
>>   #include <linux/phy/phy.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/hrtimer.h>
>>   
>>   #include "m_can.h"
>>   
>> @@ -98,8 +99,12 @@ static int m_can_plat_probe(struct platform_device *pdev)
>>   	addr = devm_platform_ioremap_resource_byname(pdev, "m_can");
>>   	irq = platform_get_irq_byname(pdev, "int0");
>>   	if (IS_ERR(addr) || irq < 0) {
> 
> What about the IS_ERR(addr) case?
> 
>> -		ret = -EINVAL;
>> -		goto probe_fail;
>> +		if (irq == -EPROBE_DEFER) {
>> +			ret = -EPROBE_DEFER;
>> +			goto probe_fail;
>> +		}
>> +		dev_info(mcan_class->dev, "Failed to get irq, initialize hrtimer\n");
>> +		hrtimer_init(&mcan_class->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_PINNED);
> 
> I don't like it when polling is unconditionally set up in case of an irq
> error. I'm not sure if we need an explicit device tree property....
> 
>>   	}
>>   
>>   	/* message ram could be shared */
>> -- 
>> 2.17.1
>>
>>
> 
> Marc
>
  

On 16.04.2023 14:33:11, Oliver Hartkopp wrote:
> 
> 
> On 4/14/23 20:20, Marc Kleine-Budde wrote:
> > On 13.04.2023 17:30:51, Judith Mendez wrote:
> > > Add a hrtimer to MCAN struct. Each MCAN will have its own
> > > hrtimer instantiated if there is no hardware interrupt found.
> > > 
> > > The hrtimer will generate a software interrupt every 1 ms. In
> > 
> > Are you sure about the 1ms?

I had the 5ms that are actually used in the code in mind. But this is a
good calculation.

> The "shortest" 11 bit CAN ID CAN frame is a Classical CAN frame with DLC = 0
> and 1 Mbit/s (arbitration) bitrate. This should be 48 bits @1Mbit => ~50
> usecs
> 
> So it should be something about
> 
>     50 usecs * (FIFO queue len - 2)

Where does the "2" come from?

> if there is some FIFO involved, right?

Yes, the mcan core has a FIFO. In the current driver the FIFO
configuration is done via device tree and fixed after that. And I don't
know the size of the available RAM in the mcan IP core on that TI SoC.

Marc
  

On 16.04.23 17:35, Marc Kleine-Budde wrote:
> On 16.04.2023 14:33:11, Oliver Hartkopp wrote:
>>
>>
>> On 4/14/23 20:20, Marc Kleine-Budde wrote:
>>> On 13.04.2023 17:30:51, Judith Mendez wrote:
>>>> Add a hrtimer to MCAN struct. Each MCAN will have its own
>>>> hrtimer instantiated if there is no hardware interrupt found.
>>>>
>>>> The hrtimer will generate a software interrupt every 1 ms. In
>>>
>>> Are you sure about the 1ms?
> 
> I had the 5ms that are actually used in the code in mind. But this is a
> good calculation.

@Judith: Can you acknowledge the value calculation?

>> The "shortest" 11 bit CAN ID CAN frame is a Classical CAN frame with DLC = 0
>> and 1 Mbit/s (arbitration) bitrate. This should be 48 bits @1Mbit => ~50
>> usecs
>>
>> So it should be something about
>>
>>      50 usecs * (FIFO queue len - 2)
> 
> Where does the "2" come from?

I thought about handling the FIFO earlier than it gets completely "full".

The fetching routine would need some time too and the hrtimer could also 
jitter to some extend.

>> if there is some FIFO involved, right?
> 
> Yes, the mcan core has a FIFO. In the current driver the FIFO
> configuration is done via device tree and fixed after that. And I don't
> know the size of the available RAM in the mcan IP core on that TI SoC.
> 
> Marc
> 

Best regards,
Oliver
  

On 16.04.2023 21:46:40, Oliver Hartkopp wrote:
> > I had the 5ms that are actually used in the code in mind. But this is a
> > good calculation.
> 
> @Judith: Can you acknowledge the value calculation?
> 
> > > The "shortest" 11 bit CAN ID CAN frame is a Classical CAN frame with DLC = 0
> > > and 1 Mbit/s (arbitration) bitrate. This should be 48 bits @1Mbit => ~50
> > > usecs
> > > 
> > > So it should be something about
> > > 
> > >      50 usecs * (FIFO queue len - 2)
> > 
> > Where does the "2" come from?
> 
> I thought about handling the FIFO earlier than it gets completely "full".
> 
> The fetching routine would need some time too and the hrtimer could also
> jitter to some extend.

I was assuming something like this.

I would argue that the polling time should be:

    50 µs * FIFO length - IRQ overhead.

The max IRQ overhead depends on your SoC and kernel configuration.

regards,
Marc
  

On 17.04.23 09:26, Marc Kleine-Budde wrote:
> On 16.04.2023 21:46:40, Oliver Hartkopp wrote:
>>> I had the 5ms that are actually used in the code in mind. But this is a
>>> good calculation.
>>
>> @Judith: Can you acknowledge the value calculation?
>>
>>>> The "shortest" 11 bit CAN ID CAN frame is a Classical CAN frame with DLC = 0
>>>> and 1 Mbit/s (arbitration) bitrate. This should be 48 bits @1Mbit => ~50
>>>> usecs
>>>>
>>>> So it should be something about
>>>>
>>>>       50 usecs * (FIFO queue len - 2)
>>>
>>> Where does the "2" come from?
>>
>> I thought about handling the FIFO earlier than it gets completely "full".
>>
>> The fetching routine would need some time too and the hrtimer could also
>> jitter to some extend.
> 
> I was assuming something like this.
> 
> I would argue that the polling time should be:
> 
>      50 µs * FIFO length - IRQ overhead.
> 
> The max IRQ overhead depends on your SoC and kernel configuration.

I just tried an educated guess to prevent the FIFO to be filled up 
completely. How can you estimate the "IRQ overhead"? And how do you 
catch the CAN frames that are received while the IRQ is handled?

Best regards,
Oliver
  

On 17.04.2023 19:34:03, Oliver Hartkopp wrote:
> On 17.04.23 09:26, Marc Kleine-Budde wrote:
> > On 16.04.2023 21:46:40, Oliver Hartkopp wrote:
> > > > I had the 5ms that are actually used in the code in mind. But this is a
> > > > good calculation.
> > > 
> > > @Judith: Can you acknowledge the value calculation?
> > > 
> > > > > The "shortest" 11 bit CAN ID CAN frame is a Classical CAN frame with DLC = 0
> > > > > and 1 Mbit/s (arbitration) bitrate. This should be 48 bits @1Mbit => ~50
> > > > > usecs
> > > > > 
> > > > > So it should be something about
> > > > > 
> > > > >       50 usecs * (FIFO queue len - 2)
> > > > 
> > > > Where does the "2" come from?
> > > 
> > > I thought about handling the FIFO earlier than it gets completely "full".
> > > 
> > > The fetching routine would need some time too and the hrtimer could also
> > > jitter to some extend.
> > 
> > I was assuming something like this.
> > 
> > I would argue that the polling time should be:
> > 
> >      50 µs * FIFO length - IRQ overhead.
> > 
> > The max IRQ overhead depends on your SoC and kernel configuration.
> 
> I just tried an educated guess to prevent the FIFO to be filled up
> completely. How can you estimate the "IRQ overhead"? And how do you catch
> the CAN frames that are received while the IRQ is handled?

We're talking about polling, better call it "overhead" or "latency from
timer expiration until FIFO has at least one frame room". This value
depends on your system.

It depends on many, many factors, SoC, Kernel configuration (preempt RT,
powersaving, frequency scaling, system load. In your example it's 100
µs. I wanted to say there's an overhead (or latency) and we need enough
space in the FIFO, to cover it.

Marc
  

Hello Marc,

On 4/17/2023 2:26 PM, Marc Kleine-Budde wrote:
> On 17.04.2023 19:34:03, Oliver Hartkopp wrote:
>> On 17.04.23 09:26, Marc Kleine-Budde wrote:
>>> On 16.04.2023 21:46:40, Oliver Hartkopp wrote:
>>>>> I had the 5ms that are actually used in the code in mind. But this is a
>>>>> good calculation.
>>>>
>>>> @Judith: Can you acknowledge the value calculation?
>>>>
>>>>>> The "shortest" 11 bit CAN ID CAN frame is a Classical CAN frame with DLC = 0
>>>>>> and 1 Mbit/s (arbitration) bitrate. This should be 48 bits @1Mbit => ~50
>>>>>> usecs
>>>>>>
>>>>>> So it should be something about
>>>>>>
>>>>>>        50 usecs * (FIFO queue len - 2)
>>>>>
>>>>> Where does the "2" come from?
>>>>
>>>> I thought about handling the FIFO earlier than it gets completely "full".
>>>>
>>>> The fetching routine would need some time too and the hrtimer could also
>>>> jitter to some extend.
>>>
>>> I was assuming something like this.
>>>
>>> I would argue that the polling time should be:
>>>
>>>       50 µs * FIFO length - IRQ overhead.
>>>
>>> The max IRQ overhead depends on your SoC and kernel configuration.
>>
>> I just tried an educated guess to prevent the FIFO to be filled up
>> completely. How can you estimate the "IRQ overhead"? And how do you catch
>> the CAN frames that are received while the IRQ is handled?
> 
> We're talking about polling, better call it "overhead" or "latency from
> timer expiration until FIFO has at least one frame room". This value
> depends on your system.
> 
> It depends on many, many factors, SoC, Kernel configuration (preempt RT,
> powersaving, frequency scaling, system load. In your example it's 100
> µs. I wanted to say there's an overhead (or latency) and we need enough
> space in the FIFO, to cover it.
> 

I am not sure how to estimate IRQ overhead, but FIFO length should be 64
elements.

50 us * 62 is about 3.1 ms and we are using 1 ms timer polling interval.

Running a few benchmarks showed that using 0.5 ms timer polling interval 
starts to take a toll on CPU load, that is why I chose 1 ms polling 
interval.

regards,
Judith
  

On 18.04.2023 15:59:57, Mendez, Judith wrote:
> > > > > > > The "shortest" 11 bit CAN ID CAN frame is a Classical CAN frame with DLC = 0
> > > > > > > and 1 Mbit/s (arbitration) bitrate. This should be 48 bits @1Mbit => ~50
> > > > > > > usecs
> > > > > > > 
> > > > > > > So it should be something about
> > > > > > > 
> > > > > > >        50 usecs * (FIFO queue len - 2)
> > > > > > 
> > > > > > Where does the "2" come from?
> > > > > 
> > > > > I thought about handling the FIFO earlier than it gets completely "full".
> > > > > 
> > > > > The fetching routine would need some time too and the hrtimer could also
> > > > > jitter to some extend.
> > > > 
> > > > I was assuming something like this.
> > > > 
> > > > I would argue that the polling time should be:
> > > > 
> > > >       50 µs * FIFO length - IRQ overhead.
> > > > 
> > > > The max IRQ overhead depends on your SoC and kernel configuration.
> > > 
> > > I just tried an educated guess to prevent the FIFO to be filled up
> > > completely. How can you estimate the "IRQ overhead"? And how do you catch
> > > the CAN frames that are received while the IRQ is handled?
> > 
> > We're talking about polling, better call it "overhead" or "latency from
> > timer expiration until FIFO has at least one frame room". This value
> > depends on your system.
> > 
> > It depends on many, many factors, SoC, Kernel configuration (preempt RT,
> > powersaving, frequency scaling, system load. In your example it's 100
> > µs. I wanted to say there's an overhead (or latency) and we need enough
> > space in the FIFO, to cover it.
> > 
> 
> I am not sure how to estimate IRQ overhead, but FIFO length should be 64
> elements.

Ok

> 50 us * 62 is about 3.1 ms and we are using 1 ms timer polling interval.

Sounds good.

> Running a few benchmarks showed that using 0.5 ms timer polling interval
> starts to take a toll on CPU load, that is why I chose 1 ms polling
> interval.

However in the code you use 5 ms.

Marc
  

Hello Marc,

On 4/19/2023 1:13 AM, Marc Kleine-Budde wrote:
> On 18.04.2023 15:59:57, Mendez, Judith wrote:
>>>>>>>> The "shortest" 11 bit CAN ID CAN frame is a Classical CAN frame with DLC = 0
>>>>>>>> and 1 Mbit/s (arbitration) bitrate. This should be 48 bits @1Mbit => ~50
>>>>>>>> usecs
>>>>>>>>
>>>>>>>> So it should be something about
>>>>>>>>
>>>>>>>>         50 usecs * (FIFO queue len - 2)
>>>>>>>
>>>>>>> Where does the "2" come from?
>>>>>>
>>>>>> I thought about handling the FIFO earlier than it gets completely "full".
>>>>>>
>>>>>> The fetching routine would need some time too and the hrtimer could also
>>>>>> jitter to some extend.
>>>>>
>>>>> I was assuming something like this.
>>>>>
>>>>> I would argue that the polling time should be:
>>>>>
>>>>>        50 µs * FIFO length - IRQ overhead.
>>>>>
>>>>> The max IRQ overhead depends on your SoC and kernel configuration.
>>>>
>>>> I just tried an educated guess to prevent the FIFO to be filled up
>>>> completely. How can you estimate the "IRQ overhead"? And how do you catch
>>>> the CAN frames that are received while the IRQ is handled?
>>>
>>> We're talking about polling, better call it "overhead" or "latency from
>>> timer expiration until FIFO has at least one frame room". This value
>>> depends on your system.
>>>
>>> It depends on many, many factors, SoC, Kernel configuration (preempt RT,
>>> powersaving, frequency scaling, system load. In your example it's 100
>>> µs. I wanted to say there's an overhead (or latency) and we need enough
>>> space in the FIFO, to cover it.
>>>
>>
>> I am not sure how to estimate IRQ overhead, but FIFO length should be 64
>> elements.
> 
> Ok
> 
>> 50 us * 62 is about 3.1 ms and we are using 1 ms timer polling interval.
> 
> Sounds good.
> 
>> Running a few benchmarks showed that using 0.5 ms timer polling interval
>> starts to take a toll on CPU load, that is why I chose 1 ms polling
>> interval.
> 
> However in the code you use 5 ms.

Yes, it was a mistake, will send out a respin with the correct value, 
thanks.

regards,
Judith
  

Hello Marc,

On 4/14/2023 1:20 PM, Marc Kleine-Budde wrote:
> On 13.04.2023 17:30:51, Judith Mendez wrote:
>> Add a hrtimer to MCAN struct. Each MCAN will have its own
>> hrtimer instantiated if there is no hardware interrupt found.
>>
>> The hrtimer will generate a software interrupt every 1 ms. In
> 
> Are you sure about the 1ms?
> 
>> hrtimer callback, we check if there is a transaction pending by
>> reading a register, then process by calling the isr if there is.
>>
>> Signed-off-by: Judith Mendez <jm@ti.com>
>> ---
>>   drivers/net/can/m_can/m_can.c          | 24 ++++++++++++++++++++++--
>>   drivers/net/can/m_can/m_can.h          |  3 +++
>>   drivers/net/can/m_can/m_can_platform.c |  9 +++++++--
>>   3 files changed, 32 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
>> index 8e83d6963d85..bb9d53f4d3cc 100644
>> --- a/drivers/net/can/m_can/m_can.c
>> +++ b/drivers/net/can/m_can/m_can.c
>> @@ -23,6 +23,7 @@
>>   #include <linux/pinctrl/consumer.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/pm_runtime.h>
>> +#include <linux/hrtimer.h>
>>   
>>   #include "m_can.h"
>>   
>> @@ -1584,6 +1585,11 @@ static int m_can_close(struct net_device *dev)
>>   	if (!cdev->is_peripheral)
>>   		napi_disable(&cdev->napi);
>>   
>> +	if (dev->irq < 0) {
>> +		dev_info(cdev->dev, "Disabling the hrtimer\n");
> 
> Make it a dev_dbg() or remove completely.
> 

Will do, thanks.

>> +		hrtimer_cancel(&cdev->hrtimer);
>> +	}
>> +
>>   	m_can_stop(dev);
>>   	m_can_clk_stop(cdev);
>>   	free_irq(dev->irq, dev);
>> @@ -1792,6 +1798,19 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
>>   	return NETDEV_TX_OK;
>>   }
>>   
>> +enum hrtimer_restart hrtimer_callback(struct hrtimer *timer)
>> +{
>> +	irqreturn_t ret;
> 
> never read value?
> 

I have removed ret completely now.

>> +	struct m_can_classdev *cdev =
>> +		container_of(timer, struct m_can_classdev, hrtimer);
>> +
>> +	ret = m_can_isr(0, cdev->net);
>> +
>> +	hrtimer_forward_now(timer, ns_to_ktime(5 * NSEC_PER_MSEC));
> 
> There's ms_to_ktime()....and the "5" doesn't match your patch
> description.
> 
>> +
>> +	return HRTIMER_RESTART;
>> +}
>> +
>>   static int m_can_open(struct net_device *dev)
>>   {
>>   	struct m_can_classdev *cdev = netdev_priv(dev);
>> @@ -1836,8 +1855,9 @@ static int m_can_open(struct net_device *dev)
>>   	}
>>   
>>   	if (err < 0) {
>> -		netdev_err(dev, "failed to request interrupt\n");
>> -		goto exit_irq_fail;
>> +		dev_info(cdev->dev, "Enabling the hrtimer\n");
>> +		cdev->hrtimer.function = &hrtimer_callback;
>> +		hrtimer_start(&cdev->hrtimer, ns_to_ktime(0), HRTIMER_MODE_REL_PINNED);
> 
> IMHO it makes no sense to request an IRQ if the device doesn't have one,
> and then try to fix up things in the error path. What about this?
> 
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -1831,9 +1831,11 @@ static int m_can_open(struct net_device *dev)
>                   err = request_threaded_irq(dev->irq, NULL, m_can_isr,
>                                              IRQF_ONESHOT,
>                                              dev->name, dev);
> -        } else {
> +        } else if (dev->irq) {
>                   err = request_irq(dev->irq, m_can_isr, IRQF_SHARED, dev->name,
>                                     dev);
> +        } else {
> +                // polling
>           }
>   
>           if (err < 0) {
> 
>>   	}

Thanks for the recommendation, I will include in v2.

>>   
>>   	/* start the m_can controller */
>> diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
>> index a839dc71dc9b..ed046d77fdb9 100644
>> --- a/drivers/net/can/m_can/m_can.h
>> +++ b/drivers/net/can/m_can/m_can.h
>> @@ -28,6 +28,7 @@
>>   #include <linux/pm_runtime.h>
>>   #include <linux/slab.h>
>>   #include <linux/uaccess.h>
>> +#include <linux/hrtimer.h>
>>   
>>   /* m_can lec values */
>>   enum m_can_lec_type {
>> @@ -93,6 +94,8 @@ struct m_can_classdev {
>>   	int is_peripheral;
>>   
>>   	struct mram_cfg mcfg[MRAM_CFG_NUM];
>> +
>> +	struct hrtimer hrtimer;
>>   };
>>   
>>   struct m_can_classdev *m_can_class_allocate_dev(struct device *dev, int sizeof_priv);
>> diff --git a/drivers/net/can/m_can/m_can_platform.c b/drivers/net/can/m_can/m_can_platform.c
>> index 9c1dcf838006..53e1648e9dab 100644
>> --- a/drivers/net/can/m_can/m_can_platform.c
>> +++ b/drivers/net/can/m_can/m_can_platform.c
>> @@ -7,6 +7,7 @@
>>   
>>   #include <linux/phy/phy.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/hrtimer.h>
>>   
>>   #include "m_can.h"
>>   
>> @@ -98,8 +99,12 @@ static int m_can_plat_probe(struct platform_device *pdev)
>>   	addr = devm_platform_ioremap_resource_byname(pdev, "m_can");
>>   	irq = platform_get_irq_byname(pdev, "int0");
>>   	if (IS_ERR(addr) || irq < 0) {
> 
> What about the IS_ERR(addr) case?
> 

Added, thanks.

>> -		ret = -EINVAL;
>> -		goto probe_fail;
>> +		if (irq == -EPROBE_DEFER) {
>> +			ret = -EPROBE_DEFER;
>> +			goto probe_fail;
>> +		}
>> +		dev_info(mcan_class->dev, "Failed to get irq, initialize hrtimer\n");
>> +		hrtimer_init(&mcan_class->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_PINNED);
> 
> I don't like it when polling is unconditionally set up in case of an irq
> error. I'm not sure if we need an explicit device tree property....

This is an interesting thought, I would definitely like to look more 
into this. Though I am thinking it would be part of future optimization 
patch. Thanks so much for your recommendation.

regards,
Judith