[1/4] can: m_can: Add hrtimer to generate software interrupt
Commit Message
Add an 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 | 30 ++++++++++++++++++++++++--
drivers/net/can/m_can/m_can.h | 3 +++
drivers/net/can/m_can/m_can_platform.c | 13 +++++++++--
3 files changed, 42 insertions(+), 4 deletions(-)
Comments
On 19.04.2023 17:33:20, Judith Mendez wrote:
> Add an 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 | 30 ++++++++++++++++++++++++--
> drivers/net/can/m_can/m_can.h | 3 +++
> drivers/net/can/m_can/m_can_platform.c | 13 +++++++++--
> 3 files changed, 42 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index a5003435802b..8784bdea300a 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 (dev->irq < 0) {
> + 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,21 @@ 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);
> +
nitpick:
Please remove these 2 newline changes.
> } else {
> - err = request_irq(dev->irq, m_can_isr, IRQF_SHARED, dev->name,
> + if (dev->irq > 0) {
Please follow the kernel coding style and use a space not a tab after
the closing ")" of the "if".
> + err = request_irq(dev->irq, m_can_isr, IRQF_SHARED, dev->name,
> dev);
> + }
> +
> + else {
Please use kernel coding style: "} else {"
> + dev_dbg(cdev->dev, "Enabling the hrtimer\n");
> + cdev->hrtimer.function = &hrtimer_callback;
> + hrtimer_start(&cdev->hrtimer, ns_to_ktime(0), HRTIMER_MODE_REL_PINNED);
> + }
I think there's no need to have nested else branches, what about this
approach?
--- 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) {
> }
>
> 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..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..7540db74b7d0 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,16 @@ 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;
> - goto probe_fail;
> + if (irq == -EPROBE_DEFER) {
> + ret = -EPROBE_DEFER;
> + goto probe_fail;
> + }
> + if (IS_ERR(addr)) {
> + ret = PTR_ERR(addr);
> + goto probe_fail;
> + }
> + dev_dbg(mcan_class->dev, "Failed to get irq, initialize hrtimer\n");
> + hrtimer_init(&mcan_class->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_PINNED);
Looks better. Please remove the outer "if (IS_ERR(addr) || irq < 0)" and
move the error checking directly after "addr = devm_platform_ioremap_resource_byname()".
What do you think about introducing the "poll-interval" property and
only enable polling if it is set?
> }
>
> /* message ram could be shared */
> --
> 2.17.1
>
>
regards,
Marc
@@ -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 (dev->irq < 0) {
+ 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,21 @@ 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,
+ if (dev->irq > 0) {
+ err = request_irq(dev->irq, m_can_isr, IRQF_SHARED, dev->name,
dev);
+ }
+
+ else {
+ dev_dbg(cdev->dev, "Enabling the hrtimer\n");
+ cdev->hrtimer.function = &hrtimer_callback;
+ hrtimer_start(&cdev->hrtimer, ns_to_ktime(0), 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,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);
@@ -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,16 @@ 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;
- goto probe_fail;
+ if (irq == -EPROBE_DEFER) {
+ ret = -EPROBE_DEFER;
+ goto probe_fail;
+ }
+ if (IS_ERR(addr)) {
+ ret = PTR_ERR(addr);
+ goto probe_fail;
+ }
+ dev_dbg(mcan_class->dev, "Failed to get irq, initialize hrtimer\n");
+ hrtimer_init(&mcan_class->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_PINNED);
}
/* message ram could be shared */