[v3,0/3] can: tcan4x5x: support resume upon rx can frame

This is the third iteration of the previous submitted patches [0] and
[1].

This revision replaces the "wake_source" function parameters to a flag
in the class device structure, and adds a patch to document the
"wakeup-source" device tree property.

Also, the previous revisions forgot to mention that the patches are
based on Markus' coalescing patches[2]. Those implements caching of the
enabled interrupts, which is handy when restoring the set of interrupts
in the resume path.

[0] https://lore.kernel.org/linux-can/20230912093807.1383720-1-martin@geanix.com/
[1] https://lore.kernel.org/linux-can/20230919122841.3803289-1-martin@geanix.com/
[2] https://lore.kernel.org/linux-can/20230929141304.3934380-1-msp@baylibre.com/

Martin Hundebøll (3):
  can: m_can: allow keeping the transceiver running in suspend
  can: tcan4x5x: support resuming from rx interrupt signal
  dt-bindings: can: tcan4x5x: Document the wakeup-source flag

 .../devicetree/bindings/net/can/tcan4x5x.txt  |  3 ++
 drivers/net/can/m_can/m_can.c                 | 22 ++++++++++---
 drivers/net/can/m_can/m_can.h                 |  1 +
 drivers/net/can/m_can/m_can_pci.c             |  1 +
 drivers/net/can/m_can/m_can_platform.c        |  1 +
 drivers/net/can/m_can/tcan4x5x-core.c         | 33 ++++++++++++++++++-
 6 files changed, 55 insertions(+), 6 deletions(-)

Hi Martin,

On Mon, Nov 13, 2023 at 02:14:50PM +0100, Martin Hundebøll wrote:
> Add a flag to the device class structure that leaves the chip in a
> running state with rx interrupt enabled, so that an m_can device driver
> can configure and use the interrupt as a wakeup source.
> 
> Signed-off-by: Martin Hundebøll <martin@geanix.com>
> ---
> 
> Change in v3:
>  * Replaced the added function parameter with a property in
>    struct m_can_classdev.
> 
> Changes in v2:
>  * Fixed comment formatting
>  * Updated m_can_class_{suspend,resume} calls in m_can_pci.c too
>  * Skipped calling m_can_start() when resuming a wake-source device
> 
>  drivers/net/can/m_can/m_can.c          | 22 +++++++++++++++++-----
>  drivers/net/can/m_can/m_can.h          |  1 +
>  drivers/net/can/m_can/m_can_pci.c      |  1 +
>  drivers/net/can/m_can/m_can_platform.c |  1 +
>  drivers/net/can/m_can/tcan4x5x-core.c  |  1 +
>  5 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index b351597f594b..55df50580480 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -2392,7 +2392,15 @@ int m_can_class_suspend(struct device *dev)
>  	if (netif_running(ndev)) {
>  		netif_stop_queue(ndev);
>  		netif_device_detach(ndev);
> -		m_can_stop(ndev);
> +
> +		/* leave the chip running with rx interrupt enabled if it is
> +		 * used as a wake-up source.
> +		 */
> +		if (cdev->pm_wake_source)
> +			m_can_write(cdev, M_CAN_IE, IR_RF0N);
> +		else
> +			m_can_stop(ndev);
> +
>  		m_can_clk_stop(cdev);
>  	}
>  
> @@ -2419,11 +2427,15 @@ int m_can_class_resume(struct device *dev)
>  		ret = m_can_clk_start(cdev);
>  		if (ret)
>  			return ret;
> -		ret  = m_can_start(ndev);
> -		if (ret) {
> -			m_can_clk_stop(cdev);
>  
> -			return ret;
> +		if (cdev->pm_wake_source) {
> +			m_can_write(cdev, M_CAN_IE, cdev->active_interrupts);
> +		} else {
> +			ret  = m_can_start(ndev);

There is one space too much here.

> +			if (ret) {
> +				m_can_clk_stop(cdev);
> +				return ret;
> +			}
>  		}
>  
>  		netif_device_attach(ndev);
> diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
> index 2986c4ce0b2f..3a9edc292593 100644
> --- a/drivers/net/can/m_can/m_can.h
> +++ b/drivers/net/can/m_can/m_can.h
> @@ -97,6 +97,7 @@ struct m_can_classdev {
>  	u32 irqstatus;
>  
>  	int pm_clock_support;
> +	int pm_wake_source;

Can you avoid this new variable by using device_can_wakeup() and/or
device_may_wakeup() to check if the device is capable to wake up and if
wakeup is actually enabled?

Best,
Markus

>  	int is_peripheral;
>  
>  	// Cached M_CAN_IE register content
> diff --git a/drivers/net/can/m_can/m_can_pci.c b/drivers/net/can/m_can/m_can_pci.c
> index f2219aa2824b..45400de4163d 100644
> --- a/drivers/net/can/m_can/m_can_pci.c
> +++ b/drivers/net/can/m_can/m_can_pci.c
> @@ -125,6 +125,7 @@ static int m_can_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
>  	mcan_class->dev = &pci->dev;
>  	mcan_class->net->irq = pci_irq_vector(pci, 0);
>  	mcan_class->pm_clock_support = 1;
> +	mcan_class->pm_wake_source = 0;
>  	mcan_class->can.clock.freq = id->driver_data;
>  	mcan_class->ops = &m_can_pci_ops;
>  
> diff --git a/drivers/net/can/m_can/m_can_platform.c b/drivers/net/can/m_can/m_can_platform.c
> index ab1b8211a61c..df0367124b4c 100644
> --- a/drivers/net/can/m_can/m_can_platform.c
> +++ b/drivers/net/can/m_can/m_can_platform.c
> @@ -139,6 +139,7 @@ static int m_can_plat_probe(struct platform_device *pdev)
>  
>  	mcan_class->net->irq = irq;
>  	mcan_class->pm_clock_support = 1;
> +	mcan_class->pm_wake_source = 0;
>  	mcan_class->can.clock.freq = clk_get_rate(mcan_class->cclk);
>  	mcan_class->dev = &pdev->dev;
>  	mcan_class->transceiver = transceiver;
> diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
> index 8a4143809d33..870ab4aef610 100644
> --- a/drivers/net/can/m_can/tcan4x5x-core.c
> +++ b/drivers/net/can/m_can/tcan4x5x-core.c
> @@ -411,6 +411,7 @@ static int tcan4x5x_can_probe(struct spi_device *spi)
>  	priv->spi = spi;
>  
>  	mcan_class->pm_clock_support = 0;
> +	mcan_class->pm_wake_source = 0;
>  	mcan_class->can.clock.freq = freq;
>  	mcan_class->dev = &spi->dev;
>  	mcan_class->ops = &tcan4x5x_ops;
> -- 
> 2.42.0
>

Hi Martin,

On Mon, Nov 13, 2023 at 02:14:51PM +0100, Martin Hundebøll wrote:
> Implement the "wakeup-source" device tree property, so the chip is left
> running when suspending, and its rx interrupt is used as a wakeup source
> to resume operation.
> 
> Signed-off-by: Martin Hundebøll <martin@geanix.com>
> ---
> 
> Change in v3:
>  * Updated to use the property in struct m_can_classdev instead of
>    passing parameters to suspend/resume functions.
> 
> Change in v2:
>  * Added `static` keyword to dev_pm_ops sturcture
> 
>  drivers/net/can/m_can/tcan4x5x-core.c | 34 +++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
> index 870ab4aef610..0f4c2ac7e4f6 100644
> --- a/drivers/net/can/m_can/tcan4x5x-core.c
> +++ b/drivers/net/can/m_can/tcan4x5x-core.c
> @@ -411,7 +411,7 @@ static int tcan4x5x_can_probe(struct spi_device *spi)
>  	priv->spi = spi;
>  
>  	mcan_class->pm_clock_support = 0;
> -	mcan_class->pm_wake_source = 0;
> +	mcan_class->pm_wake_source = device_property_read_bool(&spi->dev, "wakeup-source");
>  	mcan_class->can.clock.freq = freq;
>  	mcan_class->dev = &spi->dev;
>  	mcan_class->ops = &tcan4x5x_ops;
> @@ -460,6 +460,9 @@ static int tcan4x5x_can_probe(struct spi_device *spi)
>  		goto out_power;
>  	}
>  
> +	if (mcan_class->pm_wake_source)
> +		device_init_wakeup(&spi->dev, true);
> +

You are automatically enabling the device for wakeup here.

What do you think about using ethtool's wake-on-lan settings to actually
enable tcan as a wakeup source? So the devicetree would describe if the
hardware is capable of wakeups and the software (ethtool) can be used by
the user to decide if CAN wakeups should be enabled.

I am currently working on something similar for m_can, where m_can can
be the wakeup source from very deep sleep states. However I can't keep
the wakeup source always enabled. So this is a kind of a conflict for me
in this patch. Also I would need to use the wakeup-source flag in m_can
device nodes as well.

I can share my work later as well, so we can find a good solution that
works in both cases.

Best,
Markus

>  	ret = m_can_class_register(mcan_class);
>  	if (ret) {
>  		dev_err(&spi->dev, "Failed registering m_can device %pe\n",
> @@ -488,6 +491,29 @@ static void tcan4x5x_can_remove(struct spi_device *spi)
>  	m_can_class_free_dev(priv->cdev.net);
>  }
>  
> +static int __maybe_unused tcan4x5x_suspend(struct device *dev)
> +{
> +	struct m_can_classdev *cdev = dev_get_drvdata(dev);
> +	struct spi_device *spi = to_spi_device(dev);
> +
> +	if (cdev->pm_wake_source)
> +		enable_irq_wake(spi->irq);
> +
> +	return m_can_class_suspend(dev);
> +}
> +
> +static int __maybe_unused tcan4x5x_resume(struct device *dev)
> +{
> +	struct m_can_classdev *cdev = dev_get_drvdata(dev);
> +	struct spi_device *spi = to_spi_device(dev);
> +	int ret = m_can_class_resume(dev);
> +
> +	if (cdev->pm_wake_source)
> +		disable_irq_wake(spi->irq);
> +
> +	return ret;
> +}
> +
>  static const struct of_device_id tcan4x5x_of_match[] = {
>  	{
>  		.compatible = "ti,tcan4x5x",
> @@ -506,11 +532,15 @@ static const struct spi_device_id tcan4x5x_id_table[] = {
>  };
>  MODULE_DEVICE_TABLE(spi, tcan4x5x_id_table);
>  
> +static const struct dev_pm_ops tcan4x5x_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(tcan4x5x_suspend, tcan4x5x_resume)
> +};
> +
>  static struct spi_driver tcan4x5x_can_driver = {
>  	.driver = {
>  		.name = KBUILD_MODNAME,
>  		.of_match_table = tcan4x5x_of_match,
> -		.pm = NULL,
> +		.pm = &tcan4x5x_pm_ops,
>  	},
>  	.id_table = tcan4x5x_id_table,
>  	.probe = tcan4x5x_can_probe,
> -- 
> 2.42.0
>