[PATCH 0/3] can: mcp251xfd: Transceiver suspend, power management and wake-on-can

[PATCH 0/3] can: mcp251xfd: Transceiver suspend, power management and wake-on-can

[Phil Greenland]

Hi Marc,

Here's my first go at a patch series adding transceiver control, power management and wake-on-can  support to the mcp251xfd driver.

Few rough edges to sort out, testing and confidence explained with each patch.

First go at using git sendmail too, so hope you get all of these.

Regards,

Phil

Phil Greenland (3):
  can: mcp251xfd: automatic transceiver standby control
  can: mcp251xfd: suspend and resume handlers
  can: mcp251xfd: wake-on-can

 .../net/can/spi/mcp251xfd/mcp251xfd-core.c    | 198 +++++++++++++++---
 .../can/spi/mcp251xfd/mcp251xfd-timestamp.c   |   6 +-
 drivers/net/can/spi/mcp251xfd/mcp251xfd.h     |   3 +
 3 files changed, 172 insertions(+), 35 deletions(-)

-- 
2.25.1

[PATCH 1/3] can: mcp251xfd: automatic transceiver standby control

[Phil Greenland]

Added a device tree option xceiver_standby, which if present will cause GPIO to be placed in GPIO mode, with automatic transceiver control enabled.

Added a flag to the summary message printed following successful initialisation, to help debug it being enabled.

Have checked with a logic analyser that GPIO0 transitions from high to low as expected when the interface is brought up, and low to high when taken down.

I've renamed mcp251xfd_chip_rx_int_disable to mcp251xfd_chip_configure_gpio trying to reflect its new role, considering that it doesn't just manage the rx interrupt anymore. Perhaps separate functions updating the bits independently might be better?

Signed-off-by: Phil Greenland <phil@quantulum.co.uk>
---
 .../net/can/spi/mcp251xfd/mcp251xfd-core.c    | 56 ++++++++++---------
 drivers/net/can/spi/mcp251xfd/mcp251xfd.h     |  2 +
 2 files changed, 31 insertions(+), 27 deletions(-)

diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
index e16dc482f..9ee2c69c5 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
@@ -801,40 +801,37 @@ static int mcp251xfd_set_bittiming(const struct mcp251xfd_priv *priv)
 	return regmap_write(priv->map_reg, MCP251XFD_REG_TDC, val);
 }
 
-static int mcp251xfd_chip_rx_int_enable(const struct mcp251xfd_priv *priv)
+static int mcp251xfd_chip_configure_gpio(const struct mcp251xfd_priv *priv,
+					 bool rx_int_enable)
 {
-	u32 val;
-
-	if (!priv->rx_int)
-		return 0;
+	u32 val = 0;
 
 	/* Configure GPIOs:
-	 * - PIN0: GPIO Input
-	 * - PIN1: GPIO Input/RX Interrupt
+	 * - PIN0: GPIO Input/Transceiver standby (if enabled)
+	 * - PIN1: GPIO Input/RX Interrupt (if enabled)
 	 *
 	 * PIN1 must be Input, otherwise there is a glitch on the
 	 * rx-INT line. It happens between setting the PIN as output
 	 * (in the first byte of the SPI transfer) and configuring the
 	 * PIN as interrupt (in the last byte of the SPI transfer).
 	 */
-	val = MCP251XFD_REG_IOCON_PM0 | MCP251XFD_REG_IOCON_TRIS1 |
-		MCP251XFD_REG_IOCON_TRIS0;
-	return regmap_write(priv->map_reg, MCP251XFD_REG_IOCON, val);
-}
+	if (priv->xceiver_standby) {
+		/* Transceiver control enabled, place in GPIO mode with auto transceiver standby */
+		val |= MCP251XFD_REG_IOCON_PM0 | MCP251XFD_REG_IOCON_XSTBYEN;
+	} else {
+		/* Transceiver control disabled, place in GPIO mode and tristate */
+		val |= MCP251XFD_REG_IOCON_PM0 | MCP251XFD_REG_IOCON_TRIS0;
+	}
 
-static int mcp251xfd_chip_rx_int_disable(const struct mcp251xfd_priv *priv)
-{
-	u32 val;
+	if (priv->rx_int && rx_int_enable) {
+		/* INT1 enabled, tristate GPIO1 (overridden by INT enable) */
+		val |= MCP251XFD_REG_IOCON_TRIS1;
 
-	if (!priv->rx_int)
-		return 0;
+	} else {
+		/* INT1 disabled, place in GPIO mode and tristate */
+		val |= MCP251XFD_REG_IOCON_PM1 | MCP251XFD_REG_IOCON_TRIS1;
+	}
 
-	/* Configure GPIOs:
-	 * - PIN0: GPIO Input
-	 * - PIN1: GPIO Input
-	 */
-	val = MCP251XFD_REG_IOCON_PM1 | MCP251XFD_REG_IOCON_PM0 |
-		MCP251XFD_REG_IOCON_TRIS1 | MCP251XFD_REG_IOCON_TRIS0;
 	return regmap_write(priv->map_reg, MCP251XFD_REG_IOCON, val);
 }
 
@@ -1070,7 +1067,7 @@ static int mcp251xfd_chip_stop(struct mcp251xfd_priv *priv,
 	priv->can.state = state;
 
 	mcp251xfd_chip_interrupts_disable(priv);
-	mcp251xfd_chip_rx_int_disable(priv);
+	mcp251xfd_chip_configure_gpio(priv, false);
 	return mcp251xfd_chip_set_mode(priv, MCP251XFD_REG_CON_MODE_SLEEP);
 }
 
@@ -1090,7 +1087,8 @@ static int mcp251xfd_chip_start(struct mcp251xfd_priv *priv)
 	if (err)
 		goto out_chip_stop;
 
-	err = mcp251xfd_chip_rx_int_enable(priv);
+	/* enable rx interrupt */
+	err = mcp251xfd_chip_configure_gpio(priv, true);
 	if (err)
 		goto out_chip_stop;
 
@@ -2645,7 +2643,8 @@ static int mcp251xfd_register_check_rx_int(struct mcp251xfd_priv *priv)
 	if (!priv->rx_int)
 		return 0;
 
-	err = mcp251xfd_chip_rx_int_enable(priv);
+	/* enable rx interrupt */
+	err = mcp251xfd_chip_configure_gpio(priv, true);
 	if (err)
 		return err;
 
@@ -2654,7 +2653,8 @@ static int mcp251xfd_register_check_rx_int(struct mcp251xfd_priv *priv)
 	 */
 	rx_pending = gpiod_get_value_cansleep(priv->rx_int);
 
-	err = mcp251xfd_chip_rx_int_disable(priv);
+	/* disable rx interrupt */
+	err = mcp251xfd_chip_configure_gpio(priv, false);
 	if (err)
 		return err;
 
@@ -2724,11 +2724,12 @@ mcp251xfd_register_done(const struct mcp251xfd_priv *priv)
 		return err;
 
 	netdev_info(priv->ndev,
-		    "%s rev%lu.%lu (%cRX_INT %cMAB_NO_WARN %cCRC_REG %cCRC_RX %cCRC_TX %cECC %cHD c:%u.%02uMHz m:%u.%02uMHz r:%u.%02uMHz e:%u.%02uMHz) successfully initialized.\n",
+		    "%s rev%lu.%lu (%cRX_INT %cXCVR_STBY %cMAB_NO_WARN %cCRC_REG %cCRC_RX %cCRC_TX %cECC %cHD c:%u.%02uMHz m:%u.%02uMHz r:%u.%02uMHz e:%u.%02uMHz) successfully initialized.\n",
 		    mcp251xfd_get_model_str(priv),
 		    FIELD_GET(MCP251XFD_REG_DEVID_ID_MASK, dev_id),
 		    FIELD_GET(MCP251XFD_REG_DEVID_REV_MASK, dev_id),
 		    priv->rx_int ? '+' : '-',
+			priv->xceiver_standby ? '+' : '-',
 		    MCP251XFD_QUIRK_ACTIVE(MAB_NO_WARN),
 		    MCP251XFD_QUIRK_ACTIVE(CRC_REG),
 		    MCP251XFD_QUIRK_ACTIVE(CRC_RX),
@@ -2948,6 +2949,7 @@ static int mcp251xfd_probe(struct spi_device *spi)
 	priv->clk = clk;
 	priv->reg_vdd = reg_vdd;
 	priv->reg_xceiver = reg_xceiver;
+	priv->xceiver_standby = device_property_read_bool(&spi->dev, "xceiver_standby");
 
 	match = device_get_match_data(&spi->dev);
 	if (match)
diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd.h b/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
index 0f322daba..ad658faa2 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
@@ -612,6 +612,8 @@ struct mcp251xfd_priv {
 
 	struct mcp251xfd_devtype_data devtype_data;
 	struct can_berr_counter bec;
+
+	bool xceiver_standby;
 };
 
 #define MCP251XFD_IS(_model) \
-- 
2.25.1

[PATCH 2/3] can: mcp251xfd: suspend and resume handlers

[Phil Greenland]

Added suspend and resume handlers, placing the device into sleep on entry to suspend and waking it on resume.

Have tested both suspend to idle (freeze) and suspend to ram (mem).

Freeze was tested on an Allwinner A64 (pine 64 dev board I had lying around which didn't support suspend to ram), running 5.15.13.

Both freeze and mem were tested on a Coretex A7 (Quectel QuecOpen AG5xx), running my horrible mashup of 4.9.217 with a lot of backporting.

Each device has entered and exited suspend, triggered by another device, serial console in my case multiple times without issue (~50 times at least). With candump and cangen running, each time reception and transmission have resumed as I would have expected.

Logic analyser confirmed interrupt main / rx interrupt lines remain high despite traffic on the bus, transceiver enable line changes from low to high when entering sleep and reverse when resuming.

Signed-off-by: Phil Greenland <phil@quantulum.co.uk>
---
 .../net/can/spi/mcp251xfd/mcp251xfd-core.c    | 110 ++++++++++++++++--
 .../can/spi/mcp251xfd/mcp251xfd-timestamp.c   |   6 +-
 drivers/net/can/spi/mcp251xfd/mcp251xfd.h     |   1 +
 3 files changed, 109 insertions(+), 8 deletions(-)

diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
index 9ee2c69c5..98eb597d8 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
@@ -425,6 +425,30 @@ static void mcp251xfd_ring_init(struct mcp251xfd_priv *priv)
 	}
 }
 
+static void mcp251xfd_ring_reset(struct mcp251xfd_priv *priv)
+{
+	struct mcp251xfd_tef_ring *tef_ring;
+	struct mcp251xfd_tx_ring *tx_ring;
+	struct mcp251xfd_rx_ring *rx_ring = NULL;
+	int i;
+
+	/* TEF */
+	tef_ring = priv->tef;
+	tef_ring->head = 0;
+	tef_ring->tail = 0;
+
+	/* TX */
+	tx_ring = priv->tx;
+	tx_ring->head = 0;
+	tx_ring->tail = 0;
+
+	/* RX */
+	mcp251xfd_for_each_rx_ring(priv, rx_ring, i) {
+		rx_ring->head = 0;
+		rx_ring->tail = 0;
+	}
+}
+
 static void mcp251xfd_ring_free(struct mcp251xfd_priv *priv)
 {
 	int i;
@@ -703,13 +727,8 @@ static int mcp251xfd_chip_clock_init(const struct mcp251xfd_priv *priv)
 	u32 osc;
 	int err;
 
-	/* Activate Low Power Mode on Oscillator Disable. This only
-	 * works on the MCP2518FD. The MCP2517FD will go into normal
-	 * Sleep Mode instead.
-	 */
-	osc = MCP251XFD_REG_OSC_LPMEN |
-		FIELD_PREP(MCP251XFD_REG_OSC_CLKODIV_MASK,
-			   MCP251XFD_REG_OSC_CLKODIV_10);
+	/* Divide clock by 10 before presenting on clk output */
+	osc = FIELD_PREP(MCP251XFD_REG_OSC_CLKODIV_MASK, MCP251XFD_REG_OSC_CLKODIV_10);
 	err = regmap_write(priv->map_reg, MCP251XFD_REG_OSC, osc);
 	if (err)
 		return err;
@@ -723,6 +742,16 @@ static int mcp251xfd_chip_clock_init(const struct mcp251xfd_priv *priv)
 			    MCP251XFD_REG_TSCON_TBCEN);
 }
 
+static int mcp251xfd_sleep_in_low_power_mode(const struct mcp251xfd_priv *priv)
+{
+	/* Activate Low Power Mode for sleep. This only
+	 * works on the MCP2518FD. The MCP2517FD will go into normal
+	 * Sleep Mode instead.
+	 */
+	return regmap_update_bits(priv->map_reg, MCP251XFD_REG_OSC,
+				  MCP251XFD_REG_OSC_LPMEN, MCP251XFD_REG_OSC_LPMEN);
+}
+
 static int mcp251xfd_set_bittiming(const struct mcp251xfd_priv *priv)
 {
 	const struct can_bittiming *bt = &priv->can.bittiming;
@@ -1068,6 +1097,7 @@ static int mcp251xfd_chip_stop(struct mcp251xfd_priv *priv,
 
 	mcp251xfd_chip_interrupts_disable(priv);
 	mcp251xfd_chip_configure_gpio(priv, false);
+	mcp251xfd_sleep_in_low_power_mode(priv);
 	return mcp251xfd_chip_set_mode(priv, MCP251XFD_REG_CON_MODE_SLEEP);
 }
 
@@ -2525,6 +2555,7 @@ static int mcp251xfd_open(struct net_device *ndev)
 		goto out_transceiver_disable;
 
 	mcp251xfd_timestamp_init(priv);
+	mcp251xfd_timestamp_start(priv);
 	can_rx_offload_enable(&priv->offload);
 
 	err = request_threaded_irq(spi->irq, NULL, mcp251xfd_irq,
@@ -3030,6 +3061,70 @@ static int mcp251xfd_remove(struct spi_device *spi)
 	return 0;
 }
 
+static int __maybe_unused mcp251xfd_suspend(struct device *device)
+{
+	struct spi_device *spi = to_spi_device(device);
+	struct mcp251xfd_priv *priv = spi_get_drvdata(spi);
+	int err;
+
+	if (!netif_running(priv->ndev))
+		return 0;
+
+	disable_irq(priv->ndev->irq);
+
+	mcp251xfd_timestamp_stop(priv);
+
+	err = mcp251xfd_chip_interrupts_disable(priv);
+	if (err)
+		return err;
+
+	err = mcp251xfd_chip_set_mode(priv, MCP251XFD_REG_CON_MODE_SLEEP);
+	if (err)
+		return err;
+
+	netif_stop_queue(priv->ndev);
+	netif_device_detach(priv->ndev);
+
+	return 0;
+}
+
+static int __maybe_unused mcp251xfd_resume(struct device *device)
+{
+	struct spi_device *spi = to_spi_device(device);
+	struct mcp251xfd_priv *priv = spi_get_drvdata(spi);
+	int err;
+
+	if (!netif_running(priv->ndev))
+		return 0;
+
+	netif_device_attach(priv->ndev);
+	netif_start_queue(priv->ndev);
+
+	/* restart oscillator (disabled automatically when entering sleep) */
+	err = mcp251xfd_chip_clock_enable(priv);
+	if (err)
+		return err;
+
+	/* reset tx and rx fifos (controller resets them all when entering configuration
+	 * mode as it does when exiting sleep)
+	 */
+	mcp251xfd_ring_reset(priv);
+
+	err = mcp251xfd_chip_set_normal_mode(priv);
+	if (err)
+		return err;
+
+	err = mcp251xfd_chip_interrupts_enable(priv);
+	if (err)
+		return err;
+
+	mcp251xfd_timestamp_start(priv);
+
+	enable_irq(priv->ndev->irq);
+
+	return 0;
+}
+
 static int __maybe_unused mcp251xfd_runtime_suspend(struct device *device)
 {
 	const struct mcp251xfd_priv *priv = dev_get_drvdata(device);
@@ -3045,6 +3140,7 @@ static int __maybe_unused mcp251xfd_runtime_resume(struct device *device)
 }
 
 static const struct dev_pm_ops mcp251xfd_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(mcp251xfd_suspend, mcp251xfd_resume)
 	SET_RUNTIME_PM_OPS(mcp251xfd_runtime_suspend,
 			   mcp251xfd_runtime_resume, NULL)
 };
diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-timestamp.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-timestamp.c
index 712e09186..d889c19b0 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-timestamp.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-timestamp.c
@@ -61,8 +61,12 @@ void mcp251xfd_timestamp_init(struct mcp251xfd_priv *priv)
 	timecounter_init(&priv->tc, &priv->cc, ktime_get_real_ns());
 
 	INIT_DELAYED_WORK(&priv->timestamp, mcp251xfd_timestamp_work);
+}
+
+void mcp251xfd_timestamp_start(struct mcp251xfd_priv *priv)
+{
 	schedule_delayed_work(&priv->timestamp,
-			      MCP251XFD_TIMESTAMP_WORK_DELAY_SEC * HZ);
+						  MCP251XFD_TIMESTAMP_WORK_DELAY_SEC * HZ);
 }
 
 void mcp251xfd_timestamp_stop(struct mcp251xfd_priv *priv)
diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd.h b/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
index ad658faa2..111f88dc2 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
@@ -858,6 +858,7 @@ u16 mcp251xfd_crc16_compute(const void *data, size_t data_size);
 void mcp251xfd_skb_set_timestamp(const struct mcp251xfd_priv *priv,
 				 struct sk_buff *skb, u32 timestamp);
 void mcp251xfd_timestamp_init(struct mcp251xfd_priv *priv);
+void mcp251xfd_timestamp_start(struct mcp251xfd_priv *priv);
 void mcp251xfd_timestamp_stop(struct mcp251xfd_priv *priv);
 
 #if IS_ENABLED(CONFIG_DEV_COREDUMP)
-- 
2.25.1

[PATCH 3/3] can: mcp251xfd: wake-on-can

[Phil Greenland]

Made driver wakeup capable, building on suspend and resume handlers. When suspending if wakeup is enabled for the device, its interrupt is flagged for wakeup and the wake interrupt is enabled before the controller is placed into sleep mode.

This appears to work well if the system is allowed to fully enter sleep before a wakeup message arrives. Performed many suspend / resume cycles via can message. Initially very encouraging.

However if the system is suspended while there's traffic on the bus. Tested with 200 msgs/s from another machine. A race condition is exposed. If enabled for wakeup I would expect the controller to either cause the system to abort the suspend process, or suspend and immediately wake up. What happens instead is the system enters suspend and remains there until woken by another source.

Having had a poke around I suspect this is related to placing the controller in sleep mode in the suspend, rather than suspend_noirq stage handler.
When a wake interrupt is registered via enable_irq_wake, after all devices have had suspend called. The wake interrupts are "armed".
Once armed the noirq handlers are called, where the docs hinted that the device should be configured to wake.
It seems that as part of handing an interrupt, irq_may_run is called, which in turn calls irq_pm_check_wakeup, which if the irq is wake enabled will wake the system.
I'd guess that the controller enters suspend and immediately wakes, generating the WAKE interrupt. As interrupts are disabled at the start of suspend (to prevent the handler running after the SPI controller has been suspended) the interrupt doesn't schedule the system to wake as the interrupt isn't "armed" yet. But is somehow marked as pending and isn't re-evaluated after the system finishes suspending.
Unfortunately I don't know all that much about the inner workings of interrupt handling within the kernel.

Let me know what you think / if you're able to give me any pointers of how to solve this one?

Signed-off-by: Phil Greenland <phil@quantulum.co.uk>
---
 .../net/can/spi/mcp251xfd/mcp251xfd-core.c    | 38 +++++++++++++++++--
 1 file changed, 35 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
index 98eb597d8..bbe7bed9e 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
@@ -1072,6 +1072,12 @@ static int mcp251xfd_chip_interrupts_enable(const struct mcp251xfd_priv *priv)
 	return regmap_write(priv->map_reg, MCP251XFD_REG_INT, val);
 }
 
+static int mcp251xfd_chip_interrupts_enable_suspend(const struct mcp251xfd_priv *priv)
+{
+	/* Enable only WAKE interrupt */
+	return regmap_write(priv->map_reg, MCP251XFD_REG_INT, MCP251XFD_REG_INT_WAKIE);
+}
+
 static int mcp251xfd_chip_interrupts_disable(const struct mcp251xfd_priv *priv)
 {
 	int err;
@@ -2814,6 +2820,11 @@ static int mcp251xfd_register(struct mcp251xfd_priv *priv)
 	if (err)
 		goto out_chip_set_mode_sleep;
 
+	device_set_wakeup_capable(&ndev->dev, true);
+
+	if (device_property_read_bool(&ndev->dev, "wakeup-source"))
+		device_set_wakeup_enable(&ndev->dev, true);
+
 	err = mcp251xfd_register_done(priv);
 	if (err)
 		goto out_unregister_candev;
@@ -2851,6 +2862,8 @@ static inline void mcp251xfd_unregister(struct mcp251xfd_priv *priv)
 
 	pm_runtime_get_sync(ndev->dev.parent);
 	pm_runtime_put_noidle(ndev->dev.parent);
+	device_set_wakeup_enable(&ndev->dev, false);
+	device_set_wakeup_capable(&ndev->dev, false);
 	mcp251xfd_clks_and_vdd_disable(priv);
 	pm_runtime_disable(ndev->dev.parent);
 }
@@ -3074,9 +3087,22 @@ static int __maybe_unused mcp251xfd_suspend(struct device *device)
 
 	mcp251xfd_timestamp_stop(priv);
 
-	err = mcp251xfd_chip_interrupts_disable(priv);
-	if (err)
-		return err;
+	if (device_may_wakeup(&priv->ndev->dev)) {
+		/* Device may wake system from sleep, enable wake on irq */
+		err = enable_irq_wake(priv->ndev->irq);
+		if (err)
+			return err;
+
+		/* Enable (only) wake interrupt */
+		err = mcp251xfd_chip_interrupts_enable_suspend(priv);
+		if (err)
+			return err;
+	} else {
+		/* Device not waking system from sleep, disable all interrupts */
+		err = mcp251xfd_chip_interrupts_disable(priv);
+		if (err)
+			return err;
+	}
 
 	err = mcp251xfd_chip_set_mode(priv, MCP251XFD_REG_CON_MODE_SLEEP);
 	if (err)
@@ -3100,6 +3126,12 @@ static int __maybe_unused mcp251xfd_resume(struct device *device)
 	netif_device_attach(priv->ndev);
 	netif_start_queue(priv->ndev);
 
+	if (device_may_wakeup(&priv->ndev->dev)) {
+		err = disable_irq_wake(priv->ndev->irq);
+		if (err)
+			return err;
+	}
+
 	/* restart oscillator (disabled automatically when entering sleep) */
 	err = mcp251xfd_chip_clock_enable(priv);
 	if (err)
-- 
2.25.1