[v4,2/4] can: m_can: Add hrtimer to generate software interrupt

  Add an hrtimer to MCAN class device. Each MCAN will have its own
hrtimer instantiated if there is no hardware interrupt found and
poll-interval property is defined in device tree M_CAN node.

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>
---
Changelog:
v1:
 1. Sort list of includes
 2. Create a define for HR_TIMER_POLL_INTERVAL
 3. Fix indentations and style issues/warnings
 4. Change polling variable to type bool
 5. Change platform_get_irq to optional so not to print error msg
 6. Move error check for addr directly after assignment
 7. Print appropriate error msg with dev_err_probe insead of dev_dbg

v2:
 1. Add poll-interval to MCAN class device to check if poll-interval propery is
    present in MCAN node, this enables timer polling method
 2. Add 'polling' flag to MCAN class device to check if a device is using timer
    polling method
 3. Check if both timer polling and hardware interrupt are enabled for a MCAN
    device, default to hardware interrupt mode if both are enabled
 4. Change ms_to_ktime() to ns_to_ktime()
 5. Remove newlines, tabs, and restructure if/else section
 
 drivers/net/can/m_can/m_can.c          | 29 +++++++++++++++++++++--
 drivers/net/can/m_can/m_can.h          |  4 ++++
 drivers/net/can/m_can/m_can_platform.c | 32 +++++++++++++++++++++++---
 3 files changed, 60 insertions(+), 5 deletions(-)
  

On 01.05.2023 17:46:22, Judith Mendez wrote:
> Add an hrtimer to MCAN class device. Each MCAN will have its own
> hrtimer instantiated if there is no hardware interrupt found and
> poll-interval property is defined in device tree M_CAN node.
> 
> 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>

I think this patch is as good as it gets, given the HW and SW
limitations of the coprocessor.

Some minor nitpicks inline. No need to resend from my point of view,
I'll fixup while applying the patch.

Marc

> ---
> Changelog:
> v1:
>  1. Sort list of includes
>  2. Create a define for HR_TIMER_POLL_INTERVAL
>  3. Fix indentations and style issues/warnings
>  4. Change polling variable to type bool
>  5. Change platform_get_irq to optional so not to print error msg
>  6. Move error check for addr directly after assignment
>  7. Print appropriate error msg with dev_err_probe insead of dev_dbg
> 
> v2:
>  1. Add poll-interval to MCAN class device to check if poll-interval propery is
>     present in MCAN node, this enables timer polling method
>  2. Add 'polling' flag to MCAN class device to check if a device is using timer
>     polling method
>  3. Check if both timer polling and hardware interrupt are enabled for a MCAN
>     device, default to hardware interrupt mode if both are enabled
>  4. Change ms_to_ktime() to ns_to_ktime()
>  5. Remove newlines, tabs, and restructure if/else section
>  
>  drivers/net/can/m_can/m_can.c          | 29 +++++++++++++++++++++--
>  drivers/net/can/m_can/m_can.h          |  4 ++++
>  drivers/net/can/m_can/m_can_platform.c | 32 +++++++++++++++++++++++---
>  3 files changed, 60 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index a5003435802b..e1ac0c1d85a3 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -11,6 +11,7 @@
>  #include <linux/bitfield.h>
>  #include <linux/can/dev.h>
>  #include <linux/ethtool.h>
> +#include <linux/hrtimer.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/iopoll.h>
> @@ -308,6 +309,9 @@ enum m_can_reg {
>  #define TX_EVENT_MM_MASK	GENMASK(31, 24)
>  #define TX_EVENT_TXTS_MASK	GENMASK(15, 0)
>  
> +/* Hrtimer polling interval */
> +#define HRTIMER_POLL_INTERVAL		1
> +
>  /* The ID and DLC registers are adjacent in M_CAN FIFO memory,
>   * and we can save a (potentially slow) bus round trip by combining
>   * reads and writes to them.
> @@ -1587,6 +1591,11 @@ static int m_can_close(struct net_device *dev)
>  	if (!cdev->is_peripheral)
>  		napi_disable(&cdev->napi);
>  
> +	if (cdev->polling) {
> +		dev_dbg(cdev->dev, "Disabling the hrtimer\n");
> +		hrtimer_cancel(&cdev->hrtimer);
> +	}
> +
>  	m_can_stop(dev);
>  	m_can_clk_stop(cdev);
>  	free_irq(dev->irq, dev);
> @@ -1793,6 +1802,18 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
>  	return NETDEV_TX_OK;
>  }
>  
> +static enum hrtimer_restart hrtimer_callback(struct hrtimer *timer)
> +{
> +	struct m_can_classdev *cdev = container_of(timer, struct
> +						   m_can_classdev, hrtimer);
> +
> +	m_can_isr(0, cdev->net);
> +
> +	hrtimer_forward_now(timer, ms_to_ktime(HRTIMER_POLL_INTERVAL));
> +
> +	return HRTIMER_RESTART;
> +}
> +
>  static int m_can_open(struct net_device *dev)
>  {
>  	struct m_can_classdev *cdev = netdev_priv(dev);
> @@ -1827,13 +1848,17 @@ static int m_can_open(struct net_device *dev)
>  		}
>  
>  		INIT_WORK(&cdev->tx_work, m_can_tx_work_queue);
> -

unrelated change

>  		err = request_threaded_irq(dev->irq, NULL, m_can_isr,
>  					   IRQF_ONESHOT,
>  					   dev->name, dev);
> -	} else {
> +	} else if (!cdev->polling) {
>  		err = request_irq(dev->irq, m_can_isr, IRQF_SHARED, dev->name,
>  				  dev);
> +	} else {
> +		dev_dbg(cdev->dev, "Start hrtimer\n");
> +		cdev->hrtimer.function = &hrtimer_callback;
> +		hrtimer_start(&cdev->hrtimer, ms_to_ktime(HRTIMER_POLL_INTERVAL),
> +			      HRTIMER_MODE_REL_PINNED);
>  	}
>  
>  	if (err < 0) {
> diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
> index a839dc71dc9b..e9db5cce4e68 100644
> --- a/drivers/net/can/m_can/m_can.h
> +++ b/drivers/net/can/m_can/m_can.h
> @@ -15,6 +15,7 @@
>  #include <linux/device.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/freezer.h>
> +#include <linux/hrtimer.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/iopoll.h>
> @@ -93,6 +94,9 @@ struct m_can_classdev {
>  	int is_peripheral;
>  
>  	struct mram_cfg mcfg[MRAM_CFG_NUM];
> +
> +	struct hrtimer hrtimer;
> +	bool polling;
>  };
>  
>  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..0fcb436298f8 100644
> --- a/drivers/net/can/m_can/m_can_platform.c
> +++ b/drivers/net/can/m_can/m_can_platform.c
> @@ -5,6 +5,7 @@
>  //
>  // Copyright (C) 2018-19 Texas Instruments Incorporated - http://www.ti.com/
>  
> +#include <linux/hrtimer.h>
>  #include <linux/phy/phy.h>
>  #include <linux/platform_device.h>
>  
> @@ -96,12 +97,37 @@ static int m_can_plat_probe(struct platform_device *pdev)
>  		goto probe_fail;
>  
>  	addr = devm_platform_ioremap_resource_byname(pdev, "m_can");
> -	irq = platform_get_irq_byname(pdev, "int0");
> -	if (IS_ERR(addr) || irq < 0) {
> -		ret = -EINVAL;
> +	if (IS_ERR(addr)) {
> +		ret = PTR_ERR(addr);
>  		goto probe_fail;
>  	}
>  
> +	irq = platform_get_irq_byname_optional(pdev, "int0");
> +	if (irq == -EPROBE_DEFER) {
> +		ret = -EPROBE_DEFER;
> +		goto probe_fail;
> +	}
> +
> +	if (device_property_present(mcan_class->dev, "poll-interval"))
> +		mcan_class->polling = 1;

true

> +
> +	if (!mcan_class->polling && irq < 0) {
> +		ret = -ENXIO;
> +		dev_err_probe(mcan_class->dev, ret, "IRQ int0 not found and polling not activated\n");
> +		goto probe_fail;
> +	}
> +
> +	if (mcan_class->polling) {
> +		if (irq > 0) {
> +			mcan_class->polling = 0;

false

> +			dev_dbg(mcan_class->dev, "Polling enabled and hardware IRQ found, use hardware IRQ\n");

"...using hardware IRQ"

Use dev_info(), as there is something not 100% correct with the DT.

> +		} else {
> +			dev_dbg(mcan_class->dev, "Polling enabled, initialize hrtimer");
> +			hrtimer_init(&mcan_class->hrtimer, CLOCK_MONOTONIC,
> +				     HRTIMER_MODE_REL_PINNED);
> +		}
> +	}
> +
>  	/* message ram could be shared */
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "message_ram");
>  	if (!res) {
> -- 
> 2.17.1
> 
>
  

Hello Marc

On 5/2/23 01:37, Marc Kleine-Budde wrote:
> On 01.05.2023 17:46:22, Judith Mendez wrote:
>> Add an hrtimer to MCAN class device. Each MCAN will have its own
>> hrtimer instantiated if there is no hardware interrupt found and
>> poll-interval property is defined in device tree M_CAN node.
>>
>> 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>
> 
> I think this patch is as good as it gets, given the HW and SW
> limitations of the coprocessor.
> 
> Some minor nitpicks inline. No need to resend from my point of view,
> I'll fixup while applying the patch.

Thanks Marc, really appreciate your feedback and attention.

Same to everyone who helped make these patches better. (:

regards,
Judith
  

Hello Marc,

On 5/2/23 01:37, Marc Kleine-Budde wrote:
> On 01.05.2023 17:46:22, Judith Mendez wrote:
>> Add an hrtimer to MCAN class device. Each MCAN will have its own
>> hrtimer instantiated if there is no hardware interrupt found and
>> poll-interval property is defined in device tree M_CAN node.
>>
>> 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>
> 
> I think this patch is as good as it gets, given the HW and SW
> limitations of the coprocessor.
> 
> Some minor nitpicks inline. No need to resend from my point of view,
> I'll fixup while applying the patch.
> 
> Marc
> 
>> ---
>> Changelog:
>> v1:
>>  1. Sort list of includes
>>  2. Create a define for HR_TIMER_POLL_INTERVAL
>>  3. Fix indentations and style issues/warnings
>>  4. Change polling variable to type bool
>>  5. Change platform_get_irq to optional so not to print error msg
>>  6. Move error check for addr directly after assignment
>>  7. Print appropriate error msg with dev_err_probe insead of dev_dbg
>>
>> v2:
>>  1. Add poll-interval to MCAN class device to check if poll-interval propery is
>>     present in MCAN node, this enables timer polling method
>>  2. Add 'polling' flag to MCAN class device to check if a device is using timer
>>     polling method
>>  3. Check if both timer polling and hardware interrupt are enabled for a MCAN
>>     device, default to hardware interrupt mode if both are enabled
>>  4. Change ms_to_ktime() to ns_to_ktime()
>>  5. Remove newlines, tabs, and restructure if/else section
>>  
>>  drivers/net/can/m_can/m_can.c          | 29 +++++++++++++++++++++--
>>  drivers/net/can/m_can/m_can.h          |  4 ++++
>>  drivers/net/can/m_can/m_can_platform.c | 32 +++++++++++++++++++++++---
>>  3 files changed, 60 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
>> index a5003435802b..e1ac0c1d85a3 100644
>> --- a/drivers/net/can/m_can/m_can.c
>> +++ b/drivers/net/can/m_can/m_can.c
>> @@ -11,6 +11,7 @@
>>  #include <linux/bitfield.h>
>>  #include <linux/can/dev.h>
>>  #include <linux/ethtool.h>
>> +#include <linux/hrtimer.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/io.h>
>>  #include <linux/iopoll.h>
>> @@ -308,6 +309,9 @@ enum m_can_reg {
>>  #define TX_EVENT_MM_MASK	GENMASK(31, 24)
>>  #define TX_EVENT_TXTS_MASK	GENMASK(15, 0)
>>  
>> +/* Hrtimer polling interval */
>> +#define HRTIMER_POLL_INTERVAL		1
>> +
>>  /* The ID and DLC registers are adjacent in M_CAN FIFO memory,
>>   * and we can save a (potentially slow) bus round trip by combining
>>   * reads and writes to them.
>> @@ -1587,6 +1591,11 @@ static int m_can_close(struct net_device *dev)
>>  	if (!cdev->is_peripheral)
>>  		napi_disable(&cdev->napi);
>>  
>> +	if (cdev->polling) {
>> +		dev_dbg(cdev->dev, "Disabling the hrtimer\n");
>> +		hrtimer_cancel(&cdev->hrtimer);
>> +	}
>> +
>>  	m_can_stop(dev);
>>  	m_can_clk_stop(cdev);
>>  	free_irq(dev->irq, dev);
>> @@ -1793,6 +1802,18 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
>>  	return NETDEV_TX_OK;
>>  }
>>  
>> +static enum hrtimer_restart hrtimer_callback(struct hrtimer *timer)
>> +{
>> +	struct m_can_classdev *cdev = container_of(timer, struct
>> +						   m_can_classdev, hrtimer);
>> +
>> +	m_can_isr(0, cdev->net);
>> +
>> +	hrtimer_forward_now(timer, ms_to_ktime(HRTIMER_POLL_INTERVAL));
>> +
>> +	return HRTIMER_RESTART;
>> +}
>> +
>>  static int m_can_open(struct net_device *dev)
>>  {
>>  	struct m_can_classdev *cdev = netdev_priv(dev);
>> @@ -1827,13 +1848,17 @@ static int m_can_open(struct net_device *dev)
>>  		}
>>  
>>  		INIT_WORK(&cdev->tx_work, m_can_tx_work_queue);
>> -
> 
> unrelated change
> 
>>  		err = request_threaded_irq(dev->irq, NULL, m_can_isr,
>>  					   IRQF_ONESHOT,
>>  					   dev->name, dev);
>> -	} else {
>> +	} else if (!cdev->polling) {
>>  		err = request_irq(dev->irq, m_can_isr, IRQF_SHARED, dev->name,
>>  				  dev);
>> +	} else {
>> +		dev_dbg(cdev->dev, "Start hrtimer\n");
>> +		cdev->hrtimer.function = &hrtimer_callback;
>> +		hrtimer_start(&cdev->hrtimer, ms_to_ktime(HRTIMER_POLL_INTERVAL),
>> +			      HRTIMER_MODE_REL_PINNED);
>>  	}
>>  
>>  	if (err < 0) {
>> diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
>> index a839dc71dc9b..e9db5cce4e68 100644
>> --- a/drivers/net/can/m_can/m_can.h
>> +++ b/drivers/net/can/m_can/m_can.h
>> @@ -15,6 +15,7 @@
>>  #include <linux/device.h>
>>  #include <linux/dma-mapping.h>
>>  #include <linux/freezer.h>
>> +#include <linux/hrtimer.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/io.h>
>>  #include <linux/iopoll.h>
>> @@ -93,6 +94,9 @@ struct m_can_classdev {
>>  	int is_peripheral;
>>  
>>  	struct mram_cfg mcfg[MRAM_CFG_NUM];
>> +
>> +	struct hrtimer hrtimer;
>> +	bool polling;
>>  };
>>  
>>  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..0fcb436298f8 100644
>> --- a/drivers/net/can/m_can/m_can_platform.c
>> +++ b/drivers/net/can/m_can/m_can_platform.c
>> @@ -5,6 +5,7 @@
>>  //
>>  // Copyright (C) 2018-19 Texas Instruments Incorporated - http://www.ti.com/
>>  
>> +#include <linux/hrtimer.h>
>>  #include <linux/phy/phy.h>
>>  #include <linux/platform_device.h>
>>  
>> @@ -96,12 +97,37 @@ static int m_can_plat_probe(struct platform_device *pdev)
>>  		goto probe_fail;
>>  
>>  	addr = devm_platform_ioremap_resource_byname(pdev, "m_can");
>> -	irq = platform_get_irq_byname(pdev, "int0");
>> -	if (IS_ERR(addr) || irq < 0) {
>> -		ret = -EINVAL;
>> +	if (IS_ERR(addr)) {
>> +		ret = PTR_ERR(addr);
>>  		goto probe_fail;
>>  	}
>>  
>> +	irq = platform_get_irq_byname_optional(pdev, "int0");
>> +	if (irq == -EPROBE_DEFER) {
>> +		ret = -EPROBE_DEFER;
>> +		goto probe_fail;
>> +	}
>> +
>> +	if (device_property_present(mcan_class->dev, "poll-interval"))
>> +		mcan_class->polling = 1;
> 
> true
> 
>> +
>> +	if (!mcan_class->polling && irq < 0) {
>> +		ret = -ENXIO;
>> +		dev_err_probe(mcan_class->dev, ret, "IRQ int0 not found and polling not activated\n");
>> +		goto probe_fail;
>> +	}
>> +
>> +	if (mcan_class->polling) {
>> +		if (irq > 0) {
>> +			mcan_class->polling = 0;
> 
> false
> 
>> +			dev_dbg(mcan_class->dev, "Polling enabled and hardware IRQ found, use hardware IRQ\n");
> 
> "...using hardware IRQ"
> 
> Use dev_info(), as there is something not 100% correct with the DT.

Is it dev_info or dev_dbg? I used to have dev_info since it was nice to see when polling was
enabled. Also, I had seen this print and the next as informative prints, hence the dev_info().
However, I was told in this review process to change to dev_dbg. Which is correct?

>> +		} else {
>> +			dev_dbg(mcan_class->dev, "Polling enabled, initialize hrtimer");
>> +			hrtimer_init(&mcan_class->hrtimer, CLOCK_MONOTONIC,
>> +				     HRTIMER_MODE_REL_PINNED);
>> +		}
>> +	}
>> +
>>  	/* message ram could be shared */
>>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "message_ram");
>>  	if (!res) {
>> -- 
>> 2.17.1
>>
>>
> 

regards,
Judith
  

On 09.05.2023 17:18:06, Judith Mendez wrote:
[...]
> >> +	if (!mcan_class->polling && irq < 0) {
> >> +		ret = -ENXIO;
> >> +		dev_err_probe(mcan_class->dev, ret, "IRQ int0 not found and polling not activated\n");
> >> +		goto probe_fail;
> >> +	}
> >> +
> >> +	if (mcan_class->polling) {
> >> +		if (irq > 0) {
> >> +			mcan_class->polling = 0;
> > 
> > false
> > 
> >> +			dev_dbg(mcan_class->dev, "Polling enabled and hardware IRQ found, use hardware IRQ\n");
> > 
> > "...using hardware IRQ"
> > 
> > Use dev_info(), as there is something not 100% correct with the DT.
> 
> Is it dev_info or dev_dbg?

dev_info() - But without an explicit "poll-interval' in the DT, this
code path doesn't exist anymore.

> I used to have dev_info since it was nice to see when polling was
> enabled.

Re-read your code, this is not about enabling polling. This message
handles the case where an IRQ was given _and_ "poll-interval" was
specified. So there is something not 100% correct with the DT (IRQ _and_
polling), but this is obsolete now.

> Also, I had seen this print and the next as informative prints, hence the dev_info().

We don't print messages when IRQs are enabled, so enabling polling
should be a dev_dbg(), too.

> However, I was told in this review process to change to dev_dbg. Which is correct?

Driver works correct -> dev_dbg()
Something is strange -> dev_info()

Hope that helps,
Marc
  

Hello Marc,

On 5/10/23 02:21, Marc Kleine-Budde wrote:
> On 09.05.2023 17:18:06, Judith Mendez wrote:
> [...]
>>>> +	if (!mcan_class->polling && irq < 0) {
>>>> +		ret = -ENXIO;
>>>> +		dev_err_probe(mcan_class->dev, ret, "IRQ int0 not found and polling not activated\n");
>>>> +		goto probe_fail;
>>>> +	}
>>>> +
>>>> +	if (mcan_class->polling) {
>>>> +		if (irq > 0) {
>>>> +			mcan_class->polling = 0;
>>>
>>> false
>>>
>>>> +			dev_dbg(mcan_class->dev, "Polling enabled and hardware IRQ found, use hardware IRQ\n");
>>>
>>> "...using hardware IRQ"
>>>
>>> Use dev_info(), as there is something not 100% correct with the DT.
>>
>> Is it dev_info or dev_dbg?
> 
> dev_info() - But without an explicit "poll-interval' in the DT, this
> code path doesn't exist anymore.
> 
>> I used to have dev_info since it was nice to see when polling was
>> enabled.
> 
> Re-read your code, this is not about enabling polling. This message
> handles the case where an IRQ was given _and_ "poll-interval" was
> specified. So there is something not 100% correct with the DT (IRQ _and_
> polling), but this is obsolete now.

Sorry, I meant it was nice to see when polling was enabled or not. In

this case, if polling was enabled but hardware IRQ exists, it is good

information to see this print. But I understand now how this is a case

where 'something is strange'.

>> Also, I had seen this print and the next as informative prints, hence the dev_info().
> 
> We don't print messages when IRQs are enabled, so enabling polling
> should be a dev_dbg(), too.
> 
>> However, I was told in this review process to change to dev_dbg. Which is correct?
> 
> Driver works correct -> dev_dbg()
> Something is strange -> dev_info()

Got it, this is very helpful. Thanks.



regards,

Judith