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

[Marc Kleine-Budde]

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

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

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

Attachment: signature.asc
Description: PGP signature