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

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

[Martin Hundebøll]

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

-- 
2.42.0

[PATCH v3 1/3] can: m_can: allow keeping the transceiver running in suspend

[Martin Hundebøll]

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);
+			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;
 	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

[PATCH v3 2/3] can: tcan4x5x: support resuming from rx interrupt signal

[Martin Hundebøll]

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);
+
 	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

[PATCH v3 3/3] dt-bindings: can: tcan4x5x: Document the wakeup-source flag

[Martin Hundebøll]

Let it be known that the tcan4x5x device can now be configured to wake
the host from suspend when a can frame is received.

Signed-off-by: Martin Hundebøll <martin@geanix.com>
---

Change in v3:
 * New patch

 Documentation/devicetree/bindings/net/can/tcan4x5x.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/can/tcan4x5x.txt b/Documentation/devicetree/bindings/net/can/tcan4x5x.txt
index 170e23f0610d..20c0572c9853 100644
--- a/Documentation/devicetree/bindings/net/can/tcan4x5x.txt
+++ b/Documentation/devicetree/bindings/net/can/tcan4x5x.txt
@@ -28,6 +28,8 @@ Optional properties:
 			      available with tcan4552/4553.
 	- device-wake-gpios: Wake up GPIO to wake up the TCAN device. Not
 			     available with tcan4552/4553.
+	- wakeup-source: Leave the chip running when suspended, and configure
+			 the RX interrupt to wake up the device.
 
 Example:
 tcan4x5x: tcan4x5x@0 {
@@ -42,4 +44,5 @@ tcan4x5x: tcan4x5x@0 {
 		device-state-gpios = <&gpio3 21 GPIO_ACTIVE_HIGH>;
 		device-wake-gpios = <&gpio1 15 GPIO_ACTIVE_HIGH>;
 		reset-gpios = <&gpio1 27 GPIO_ACTIVE_HIGH>;
+		wakeup-source;
 };
-- 
2.42.0

Re: [PATCH v3 3/3] dt-bindings: can: tcan4x5x: Document the wakeup-source flag

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1407 bytes --]

On Mon, Nov 13, 2023 at 02:14:52PM +0100, Martin Hundebøll wrote:
> Let it be known that the tcan4x5x device can now be configured to wake
> the host from suspend when a can frame is received.
> 
> Signed-off-by: Martin Hundebøll <martin@geanix.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

> ---
> 
> Change in v3:
>  * New patch
> 
>  Documentation/devicetree/bindings/net/can/tcan4x5x.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/can/tcan4x5x.txt b/Documentation/devicetree/bindings/net/can/tcan4x5x.txt
> index 170e23f0610d..20c0572c9853 100644
> --- a/Documentation/devicetree/bindings/net/can/tcan4x5x.txt
> +++ b/Documentation/devicetree/bindings/net/can/tcan4x5x.txt
> @@ -28,6 +28,8 @@ Optional properties:
>  			      available with tcan4552/4553.
>  	- device-wake-gpios: Wake up GPIO to wake up the TCAN device. Not
>  			     available with tcan4552/4553.
> +	- wakeup-source: Leave the chip running when suspended, and configure
> +			 the RX interrupt to wake up the device.
>  
>  Example:
>  tcan4x5x: tcan4x5x@0 {
> @@ -42,4 +44,5 @@ tcan4x5x: tcan4x5x@0 {
>  		device-state-gpios = <&gpio3 21 GPIO_ACTIVE_HIGH>;
>  		device-wake-gpios = <&gpio1 15 GPIO_ACTIVE_HIGH>;
>  		reset-gpios = <&gpio1 27 GPIO_ACTIVE_HIGH>;
> +		wakeup-source;
>  };
> -- 
> 2.42.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 1/3] can: m_can: allow keeping the transceiver running in suspend

[Markus Schneider-Pargmann]

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
> 

Re: [PATCH v3 2/3] can: tcan4x5x: support resuming from rx interrupt signal

[Markus Schneider-Pargmann]

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
>