[v2,1/4] can: m_can: Add hrtimer to generate software interrupt
Commit Message
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:
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. Changed ms_to_ktime() to ns_to_ktime()
5. Removed newlines, tabs, and restructure if/else section.
drivers/net/can/m_can/m_can.c | 30 ++++++++++++++++++++-----
drivers/net/can/m_can/m_can.h | 5 +++++
drivers/net/can/m_can/m_can_platform.c | 31 ++++++++++++++++++++++++--
3 files changed, 59 insertions(+), 7 deletions(-)
Comments
On 24.04.2023 14:53:59, 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>
> ---
> Changelog:
> 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. Changed ms_to_ktime() to ns_to_ktime()
> 5. Removed newlines, tabs, and restructure if/else section.
>
> drivers/net/can/m_can/m_can.c | 30 ++++++++++++++++++++-----
> drivers/net/can/m_can/m_can.h | 5 +++++
> drivers/net/can/m_can/m_can_platform.c | 31 ++++++++++++++++++++++++--
> 3 files changed, 59 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index a5003435802b..33e094f88da1 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>
keep the list of includes sorted
>
> #include "m_can.h"
>
> @@ -1587,6 +1588,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 +1799,18 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
> return NETDEV_TX_OK;
> }
>
> +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(1));
Please create a define for this
> +
> + return HRTIMER_RESTART;
> +}
> +
> static int m_can_open(struct net_device *dev)
> {
> struct m_can_classdev *cdev = netdev_priv(dev);
> @@ -1827,13 +1845,15 @@ static int m_can_open(struct net_device *dev)
> }
>
> INIT_WORK(&cdev->tx_work, m_can_tx_work_queue);
> -
> err = request_threaded_irq(dev->irq, NULL, m_can_isr,
> - IRQF_ONESHOT,
> - dev->name, dev);
> - } else {
> - err = request_irq(dev->irq, m_can_isr, IRQF_SHARED, dev->name,
> + IRQF_ONESHOT, dev->name, dev);
> + } else if (!cdev->polling) {
> + err = request_irq(dev->irq, m_can_isr, IRQF_SHARED, dev->name,
> dev);
No need to change the indention
> + } else {
> + dev_dbg(cdev->dev, "Start hrtimer\n");
> + cdev->hrtimer.function = &hrtimer_callback;
> + hrtimer_start(&cdev->hrtimer, ms_to_ktime(cdev->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..1ba87eb23f8e 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>
keep the list of includes sorted
>
> /* m_can lec values */
> enum m_can_lec_type {
> @@ -93,6 +94,10 @@ struct m_can_classdev {
> int is_peripheral;
>
> struct mram_cfg mcfg[MRAM_CFG_NUM];
> +
> + struct hrtimer hrtimer;
> + u32 poll_interval;
> + u8 polling;
bool
> };
>
> 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..e899c04edc01 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"
>
> @@ -97,11 +98,37 @@ 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");
use platform_get_irq_byname_optional(), it doesn't print an error
message.
> - if (IS_ERR(addr) || irq < 0) {
> - ret = -EINVAL;
> + if (irq == -EPROBE_DEFER) {
> + ret = -EPROBE_DEFER;
> goto probe_fail;
> }
>
> + if (IS_ERR(addr)) {
> + ret = PTR_ERR(addr);
> + goto probe_fail;
> + }
please move the error check for "addr" directly after the "addr = "
assignment.
> +
> + mcan_class->polling = 0;
No need to init as "0"
> + if (device_property_present(mcan_class->dev, "poll-interval")) {
> + mcan_class->polling = 1;
> + }
No need for the { } here.
> +
> + if (!mcan_class->polling && irq < 0) {
> + ret = -ENODATA;
-ENXIO
> + dev_dbg(mcan_class->dev, "Polling not enabled\n");
print a proper error message using dev_err_probe("IRQ %s not found and
polling not activated\n")
> + goto probe_fail;
> + }
> +
> + if (mcan_class->polling && irq > 0) {
> + mcan_class->polling = 0;
> + dev_dbg(mcan_class->dev, "Polling not enabled, hardware interrupt exists\n");
> + }
> +
> + if (mcan_class->polling && irq < 0) {
> + dev_dbg(mcan_class->dev, "Polling enabled, initialize hrtimer");
> + hrtimer_init(&mcan_class->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_PINNED);
> + }
combine both if (mcan_class->polling) into one.
> +
> /* message ram could be shared */
> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "message_ram");
> if (!res) {
> --
> 2.17.1
>
>
Marc
Hello Marc,
On 4/24/2023 3:14 PM, Marc Kleine-Budde wrote:
> On 24.04.2023 14:53:59, 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>
>> ---
>> Changelog:
>> 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. Changed ms_to_ktime() to ns_to_ktime()
>> 5. Removed newlines, tabs, and restructure if/else section.
>>
>> drivers/net/can/m_can/m_can.c | 30 ++++++++++++++++++++-----
>> drivers/net/can/m_can/m_can.h | 5 +++++
>> drivers/net/can/m_can/m_can_platform.c | 31 ++++++++++++++++++++++++--
>> 3 files changed, 59 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
>> index a5003435802b..33e094f88da1 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>
>
> keep the list of includes sorted
>
Will do.
>>
>> #include "m_can.h"
>>
>> @@ -1587,6 +1588,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 +1799,18 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
>> return NETDEV_TX_OK;
>> }
>>
>> +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(1));
>
> Please create a define for this
>
Thanks, will create a define for the 1 ms polling interval.
>> +
>> + return HRTIMER_RESTART;
>> +}
>> +
>> static int m_can_open(struct net_device *dev)
>> {
>> struct m_can_classdev *cdev = netdev_priv(dev);
>> @@ -1827,13 +1845,15 @@ static int m_can_open(struct net_device *dev)
>> }
>>
>> INIT_WORK(&cdev->tx_work, m_can_tx_work_queue);
>> -
>> err = request_threaded_irq(dev->irq, NULL, m_can_isr,
>> - IRQF_ONESHOT,
>> - dev->name, dev);
>> - } else {
>> - err = request_irq(dev->irq, m_can_isr, IRQF_SHARED, dev->name,
>> + IRQF_ONESHOT, dev->name, dev);
>> + } else if (!cdev->polling) {
>> + err = request_irq(dev->irq, m_can_isr, IRQF_SHARED, dev->name,
>> dev);
>
> No need to change the indention
>
Will fix.
>> + } else {
>> + dev_dbg(cdev->dev, "Start hrtimer\n");
>> + cdev->hrtimer.function = &hrtimer_callback;
>> + hrtimer_start(&cdev->hrtimer, ms_to_ktime(cdev->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..1ba87eb23f8e 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>
>
> keep the list of includes sorted
>
>>
>> /* m_can lec values */
>> enum m_can_lec_type {
>> @@ -93,6 +94,10 @@ struct m_can_classdev {
>> int is_peripheral;
>>
>> struct mram_cfg mcfg[MRAM_CFG_NUM];
>> +
>> + struct hrtimer hrtimer;
>> + u32 poll_interval;
>> + u8 polling;
>
> bool
>
Will use bool instead.
>> };
>>
>> 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..e899c04edc01 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"
>>
>> @@ -97,11 +98,37 @@ 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");
>
> use platform_get_irq_byname_optional(), it doesn't print an error
> message.
>
Thanks.
>> - if (IS_ERR(addr) || irq < 0) {
>> - ret = -EINVAL;
>> + if (irq == -EPROBE_DEFER) {
>> + ret = -EPROBE_DEFER;
>> goto probe_fail;
>> }
>>
>> + if (IS_ERR(addr)) {
>> + ret = PTR_ERR(addr);
>> + goto probe_fail;
>> + }
>
> please move the error check for "addr" directly after the "addr = "
> assignment.
>
Will do.
>> +
>> + mcan_class->polling = 0;
>
> No need to init as "0"
>
Awsome thanks.
>> + if (device_property_present(mcan_class->dev, "poll-interval")) {
>> + mcan_class->polling = 1;
>> + }
>
> No need for the { } here.
>
>> +
>> + if (!mcan_class->polling && irq < 0) {
>> + ret = -ENODATA;
> -ENXIO
>> + dev_dbg(mcan_class->dev, "Polling not enabled\n");
>
> print a proper error message using dev_err_probe("IRQ %s not found and
> polling not activated\n")
>
Why %s when MCAN requests 1 IRQ which is "int0"? If we want to print
"int0", should it be hardcoded into the print error message?
>> + goto probe_fail;
>> + }
>> +
>> + if (mcan_class->polling && irq > 0) {
>> + mcan_class->polling = 0;
>> + dev_dbg(mcan_class->dev, "Polling not enabled, hardware interrupt exists\n");
>> + }
>> +
>> + if (mcan_class->polling && irq < 0) {
>> + dev_dbg(mcan_class->dev, "Polling enabled, initialize hrtimer");
>> + hrtimer_init(&mcan_class->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_PINNED);
>> + }
>
> combine both if (mcan_class->polling) into one.
>
Will do.
>> +
>> /* message ram could be shared */
>> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "message_ram");
>> if (!res) {
>> --
>> 2.17.1
>>
>>
regards,
Judith
On 26.04.2023 11:11:12, Mendez, Judith wrote:
[...]
> > print a proper error message using dev_err_probe("IRQ %s not found and
> > polling not activated\n")
> >
> Why %s when MCAN requests 1 IRQ which is "int0"? If we want to print "int0",
> should it be hardcoded into the print error message?
I think I copied the error message from platform_get_irq_byname() and
extended it. Of course it makes sense to hardcode the IRQ name.
Marc
On Mon, Apr 24, 2023 at 02:53:59PM -0500, 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>
> ---
> Changelog:
> 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. Changed ms_to_ktime() to ns_to_ktime()
> 5. Removed newlines, tabs, and restructure if/else section.
>
> drivers/net/can/m_can/m_can.c | 30 ++++++++++++++++++++-----
> drivers/net/can/m_can/m_can.h | 5 +++++
> drivers/net/can/m_can/m_can_platform.c | 31 ++++++++++++++++++++++++--
> 3 files changed, 59 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index a5003435802b..33e094f88da1 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"
>
> @@ -1587,6 +1588,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 +1799,18 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
> return NETDEV_TX_OK;
> }
>
> +enum hrtimer_restart hrtimer_callback(struct hrtimer *timer)
Hi Judith,
This function seems to only be used in this file,
so it should be static.
> +{
> + 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(1));
> +
> + return HRTIMER_RESTART;
> +}
> +
...
@@ -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"
@@ -1587,6 +1588,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 +1799,18 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
return NETDEV_TX_OK;
}
+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(1));
+
+ return HRTIMER_RESTART;
+}
+
static int m_can_open(struct net_device *dev)
{
struct m_can_classdev *cdev = netdev_priv(dev);
@@ -1827,13 +1845,15 @@ static int m_can_open(struct net_device *dev)
}
INIT_WORK(&cdev->tx_work, m_can_tx_work_queue);
-
err = request_threaded_irq(dev->irq, NULL, m_can_isr,
- IRQF_ONESHOT,
- dev->name, dev);
- } else {
- err = request_irq(dev->irq, m_can_isr, IRQF_SHARED, dev->name,
+ IRQF_ONESHOT, dev->name, dev);
+ } 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(cdev->poll_interval), HRTIMER_MODE_REL_PINNED);
}
if (err < 0) {
@@ -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,10 @@ struct m_can_classdev {
int is_peripheral;
struct mram_cfg mcfg[MRAM_CFG_NUM];
+
+ struct hrtimer hrtimer;
+ u32 poll_interval;
+ u8 polling;
};
struct m_can_classdev *m_can_class_allocate_dev(struct device *dev, int sizeof_priv);
@@ -7,6 +7,7 @@
#include <linux/phy/phy.h>
#include <linux/platform_device.h>
+#include <linux/hrtimer.h>
#include "m_can.h"
@@ -97,11 +98,37 @@ 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) {
- ret = -EINVAL;
+ if (irq == -EPROBE_DEFER) {
+ ret = -EPROBE_DEFER;
goto probe_fail;
}
+ if (IS_ERR(addr)) {
+ ret = PTR_ERR(addr);
+ goto probe_fail;
+ }
+
+ mcan_class->polling = 0;
+ if (device_property_present(mcan_class->dev, "poll-interval")) {
+ mcan_class->polling = 1;
+ }
+
+ if (!mcan_class->polling && irq < 0) {
+ ret = -ENODATA;
+ dev_dbg(mcan_class->dev, "Polling not enabled\n");
+ goto probe_fail;
+ }
+
+ if (mcan_class->polling && irq > 0) {
+ mcan_class->polling = 0;
+ dev_dbg(mcan_class->dev, "Polling not enabled, hardware interrupt exists\n");
+ }
+
+ if (mcan_class->polling && irq < 0) {
+ 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) {