[PATCH 0/7] can: m_can: fix pm_runtime and CAN state handling

[PATCH 0/7] can: m_can: fix pm_runtime and CAN state handling

[Marc Kleine-Budde]

The first patch fixes a pm_runtime imbalance in the m_can_platform
driver.

The rest of this series fixes the CAN state handling in the m_can
driver:
- add the missing state transition from from Error Warning back to
  Error Active (Patches 2+3)
- address the fact that in some SoCs (observed on the STM32MP15) the
  M_CAN IP core keeps the CAN state and CAN error counters over an
  internal reset cycle. Set the correct CAN state during ifup and
  system resume (Patches 4+5)
- add support for optional shared external reset, to properly reset
  the IP core (Patches 6+7)

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
Marc Kleine-Budde (7):
      can: m_can: m_can_plat_remove(): add missing pm_runtime_disable()
      can: m_can: m_can_rx_handler(): only handle active interrupts
      can: m_can: m_can_handle_state_errors(): fix CAN state transition to Error Active
      can: m_can: m_can_chip_config(): bring up interface in correct state
      can: m_can: fix CAN state in system PM
      can: m_can: m_can_get_berr_counter(): don't wake up controller if interface is down
      can: m_can: add optional support for reset

 drivers/net/can/m_can/m_can.c          | 93 ++++++++++++++++++++++++----------
 drivers/net/can/m_can/m_can.h          |  1 +
 drivers/net/can/m_can/m_can_platform.c |  2 +-
 3 files changed, 68 insertions(+), 28 deletions(-)
---
base-commit: 89886abd073489e26614e4d80fb8eb70d3938a0b
change-id: 20250811-m_can-fix-state-handling-0ba4bda6cb8e

Best regards,
--  
Marc Kleine-Budde <mkl@pengutronix.de>

[PATCH 1/7] can: m_can: m_can_plat_remove(): add missing pm_runtime_disable()

[Marc Kleine-Budde]

Commit 227619c3ff7c ("can: m_can: move runtime PM enable/disable to
m_can_platform") moved the PM runtime enable from the m_can core
driver into the m_can_platform.

That patch forgot to move the pm_runtime_disable() to
m_can_plat_remove(), so that unloading the m_can_platform driver
causes an "Unbalanced pm_runtime_enable!" error message.

Add the missing pm_runtime_disable() to m_can_plat_remove() to fix the
problem.

Cc: Patrik Flykt <patrik.flykt@linux.intel.com>
Fixes: 227619c3ff7c ("can: m_can: move runtime PM enable/disable to m_can_platform")
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/m_can/m_can_platform.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/m_can/m_can_platform.c b/drivers/net/can/m_can/m_can_platform.c
index b832566efda0..057eaa7b8b4b 100644
--- a/drivers/net/can/m_can/m_can_platform.c
+++ b/drivers/net/can/m_can/m_can_platform.c
@@ -180,7 +180,7 @@ static void m_can_plat_remove(struct platform_device *pdev)
 	struct m_can_classdev *mcan_class = &priv->cdev;
 
 	m_can_class_unregister(mcan_class);
-
+	pm_runtime_disable(mcan_class->dev);
 	m_can_class_free_dev(mcan_class->net);
 }
 

-- 
2.50.1

[PATCH 2/7] can: m_can: m_can_rx_handler(): only handle active interrupts

[Marc Kleine-Budde]

Among other things, the M_CAN IP core has an Interrupt Register (IR)
and an Interrupt Enable (IE) register. An interrupt is triggered if at
least 1 bit is set in the bitwise and of IR and IE.

Depending on the configuration not all interrupts are enabled in the
IE register. However the m_can_rx_handler() IRQ handler looks at all
interrupts not just the enabled ones. This may lead to handling of not
activated interrupts.

Fix the problem and mask the irqstatus (IR register) with the
active_interrupts (cache value of IE register).

Fixes: e0d1f4816f2a ("can: m_can: add Bosch M_CAN controller support")
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/m_can/m_can.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index fe74dbd2c966..a51dc0bb8124 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1006,6 +1006,7 @@ static int m_can_rx_handler(struct net_device *dev, int quota, u32 irqstatus)
 	int rx_work_or_err;
 	int work_done = 0;
 
+	irqstatus &= cdev->active_interrupts;
 	if (!irqstatus)
 		goto end;
 

-- 
2.50.1

[PATCH 3/7] can: m_can: m_can_handle_state_errors(): fix CAN state transition to Error Active

[Marc Kleine-Budde]

The CAN Error State is determined by the receive and transmit error
counters. The CAN error counters decrease when reception/transmission
is successful, so that a status transition back to the Error Active
status is possible. This transition is not handled by
m_can_handle_state_errors().

Add the missing detection of the Error Active state to
m_can_handle_state_errors() and extend the handling of this state in
m_can_handle_state_change().

Fixes: e0d1f4816f2a ("can: m_can: add Bosch M_CAN controller support")
Fixes: cd0d83eab2e0 ("can: m_can: m_can_handle_state_change(): fix state change")
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/m_can/m_can.c | 48 ++++++++++++++++++++++++++-----------------
 1 file changed, 29 insertions(+), 19 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index a51dc0bb8124..b485d2f3d971 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -812,6 +812,10 @@ static int m_can_handle_state_change(struct net_device *dev,
 	u32 timestamp = 0;
 
 	switch (new_state) {
+	case CAN_STATE_ERROR_ACTIVE:
+		/* error active state */
+		cdev->can.state = CAN_STATE_ERROR_ACTIVE;
+		break;
 	case CAN_STATE_ERROR_WARNING:
 		/* error warning state */
 		cdev->can.can_stats.error_warning++;
@@ -841,6 +845,13 @@ static int m_can_handle_state_change(struct net_device *dev,
 	__m_can_get_berr_counter(dev, &bec);
 
 	switch (new_state) {
+	case CAN_STATE_ERROR_ACTIVE:
+		/* error active state */
+		cf->can_id |= CAN_ERR_CRTL | CAN_ERR_CNT;
+		cf->data[1] = CAN_ERR_CRTL_ACTIVE;
+		cf->data[6] = bec.txerr;
+		cf->data[7] = bec.rxerr;
+		break;
 	case CAN_STATE_ERROR_WARNING:
 		/* error warning state */
 		cf->can_id |= CAN_ERR_CRTL | CAN_ERR_CNT;
@@ -877,30 +888,29 @@ static int m_can_handle_state_change(struct net_device *dev,
 	return 1;
 }
 
+static enum can_state
+m_can_can_state_get_by_psr(const u32 psr)
+{
+	if (psr & PSR_BO)
+		return CAN_STATE_BUS_OFF;
+	if (psr & PSR_EP)
+		return CAN_STATE_ERROR_PASSIVE;
+	if (psr & PSR_EW)
+		return CAN_STATE_ERROR_WARNING;
+
+	return CAN_STATE_ERROR_ACTIVE;
+}
+
 static int m_can_handle_state_errors(struct net_device *dev, u32 psr)
 {
 	struct m_can_classdev *cdev = netdev_priv(dev);
-	int work_done = 0;
+	enum can_state new_state;
 
-	if (psr & PSR_EW && cdev->can.state != CAN_STATE_ERROR_WARNING) {
-		netdev_dbg(dev, "entered error warning state\n");
-		work_done += m_can_handle_state_change(dev,
-						       CAN_STATE_ERROR_WARNING);
-	}
+	new_state = m_can_can_state_get_by_psr(psr);
+	if (new_state == cdev->can.state)
+		return 0;
 
-	if (psr & PSR_EP && cdev->can.state != CAN_STATE_ERROR_PASSIVE) {
-		netdev_dbg(dev, "entered error passive state\n");
-		work_done += m_can_handle_state_change(dev,
-						       CAN_STATE_ERROR_PASSIVE);
-	}
-
-	if (psr & PSR_BO && cdev->can.state != CAN_STATE_BUS_OFF) {
-		netdev_dbg(dev, "entered error bus off state\n");
-		work_done += m_can_handle_state_change(dev,
-						       CAN_STATE_BUS_OFF);
-	}
-
-	return work_done;
+	return m_can_handle_state_change(dev, new_state);
 }
 
 static void m_can_handle_other_err(struct net_device *dev, u32 irqstatus)

-- 
2.50.1

[PATCH 4/7] can: m_can: m_can_chip_config(): bring up interface in correct state

[Marc Kleine-Budde]

In some SoCs (observed on the STM32MP15) the M_CAN IP core keeps the
CAN state and CAN error counters over an internal reset cycle. An
external reset is not always possible, due to the shared reset with
the other CAN core. This caused the core not always be in Error Active
state when bringing up the controller.

Instead of always setting the CAN state to Error Active in
m_can_chip_config(), fix this by reading and decoding the Protocol
Status Regitser (PSR) and set the CAN state accordingly.

Fixes: e0d1f4816f2a ("can: m_can: add Bosch M_CAN controller support")
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/m_can/m_can.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index b485d2f3d971..310a907cbb7e 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1607,6 +1607,7 @@ static int m_can_chip_config(struct net_device *dev)
 static int m_can_start(struct net_device *dev)
 {
 	struct m_can_classdev *cdev = netdev_priv(dev);
+	u32 reg_psr;
 	int ret;
 
 	/* basic m_can configuration */
@@ -1617,7 +1618,8 @@ static int m_can_start(struct net_device *dev)
 	netdev_queue_set_dql_min_limit(netdev_get_tx_queue(cdev->net, 0),
 				       cdev->tx_max_coalesced_frames);
 
-	cdev->can.state = CAN_STATE_ERROR_ACTIVE;
+	reg_psr = m_can_read(cdev, M_CAN_PSR);
+	cdev->can.state = m_can_can_state_get_by_psr(reg_psr);
 
 	m_can_enable_all_interrupts(cdev);
 

-- 
2.50.1

[PATCH 5/7] can: m_can: fix CAN state in system PM

[Marc Kleine-Budde]

A suspend/resume cycle on a down interface results in the interface
coming up in Error Active state. A suspend/resume cycle on an Up
interface will always result in Error Active state, regardless of the
actual CAN state.

During suspend, only set running interfaces to CAN_STATE_SLEEPING.
During resume only touch the CAN state of running interfaces. For
wakeup sources, set the CAN state depending on the Protocol Status
Regitser (PSR), for non wakeup source interfaces m_can_start() will do
the same.

Fixes: e0d1f4816f2a ("can: m_can: add Bosch M_CAN controller support")
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/m_can/m_can.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 310a907cbb7e..149f3a8b5f7e 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -2507,12 +2507,11 @@ int m_can_class_suspend(struct device *dev)
 		}
 
 		m_can_clk_stop(cdev);
+		cdev->can.state = CAN_STATE_SLEEPING;
 	}
 
 	pinctrl_pm_select_sleep_state(dev);
 
-	cdev->can.state = CAN_STATE_SLEEPING;
-
 	return ret;
 }
 EXPORT_SYMBOL_GPL(m_can_class_suspend);
@@ -2525,14 +2524,14 @@ int m_can_class_resume(struct device *dev)
 
 	pinctrl_pm_select_default_state(dev);
 
-	cdev->can.state = CAN_STATE_ERROR_ACTIVE;
-
 	if (netif_running(ndev)) {
 		ret = m_can_clk_start(cdev);
 		if (ret)
 			return ret;
 
 		if (cdev->pm_wake_source) {
+			u32 reg_psr;
+
 			/* Restore active interrupts but disable coalescing as
 			 * we may have missed important waterlevel interrupts
 			 * between suspend and resume. Timers are already
@@ -2544,6 +2543,9 @@ int m_can_class_resume(struct device *dev)
 			if (cdev->ops->init)
 				ret = cdev->ops->init(cdev);
 
+			reg_psr = m_can_read(cdev, M_CAN_PSR);
+			cdev->can.state = m_can_can_state_get_by_psr(reg_psr);
+
 			m_can_write(cdev, M_CAN_IE, cdev->active_interrupts);
 		} else {
 			ret  = m_can_start(ndev);

-- 
2.50.1

[PATCH 6/7] can: m_can: m_can_get_berr_counter(): don't wake up controller if interface is down

[Marc Kleine-Budde]

If the interface is down, the CAN controller might be powered down,
the clock disabled, and/or it's external reset asserted.

Don't wake up the controller to read the CAN bus error counters, if
the interface is down.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/m_can/m_can.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 149f3a8b5f7e..c24ea0e5599f 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -790,6 +790,10 @@ static int m_can_get_berr_counter(const struct net_device *dev,
 	struct m_can_classdev *cdev = netdev_priv(dev);
 	int err;
 
+	/* Avoid waking up the controller if the interface is down */
+	if (!(dev->flags & IFF_UP))
+		return 0;
+
 	err = m_can_clk_start(cdev);
 	if (err)
 		return err;

-- 
2.50.1

[PATCH 7/7] can: m_can: add optional support for reset

[Marc Kleine-Budde]

In some SoCs (observed on the STM32MP15) the M_CAN IP core keeps the
CAN state and CAN error counters over an internal reset cycle. The
STM32MP15 SoC provides an external reset, which is shared between both
M_CAN cores.

Add support for an optional external reset. Take care of shared
resets, de-assert reset during the probe phase in
m_can_class_register() and while the interface is up, assert the reset
otherwise.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/m_can/m_can.c | 26 +++++++++++++++++++++++---
 drivers/net/can/m_can/m_can.h |  1 +
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index c24ea0e5599f..0a6d4b523c33 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/reset.h>
 
 #include "m_can.h"
 
@@ -1833,6 +1834,7 @@ static int m_can_close(struct net_device *dev)
 
 	close_candev(dev);
 
+	reset_control_assert(cdev->rsts);
 	m_can_clk_stop(cdev);
 	phy_power_off(cdev->transceiver);
 
@@ -2075,11 +2077,15 @@ static int m_can_open(struct net_device *dev)
 	if (err)
 		goto out_phy_power_off;
 
+	err = reset_control_deassert(cdev->rsts);
+	if (err)
+		goto exit_disable_clks;
+
 	/* open the can device */
 	err = open_candev(dev);
 	if (err) {
 		netdev_err(dev, "failed to open can device\n");
-		goto exit_disable_clks;
+		goto out_reset_control_assert;
 	}
 
 	if (cdev->is_peripheral)
@@ -2135,6 +2141,8 @@ static int m_can_open(struct net_device *dev)
 	else
 		napi_disable(&cdev->napi);
 	close_candev(dev);
+out_reset_control_assert:
+	reset_control_assert(cdev->rsts);
 exit_disable_clks:
 	m_can_clk_stop(cdev);
 out_phy_power_off:
@@ -2425,15 +2433,23 @@ int m_can_class_register(struct m_can_classdev *cdev)
 		}
 	}
 
+	cdev->rsts = devm_reset_control_get_optional_shared(cdev->dev, NULL);
+	if (IS_ERR(cdev->rsts))
+		return PTR_ERR(cdev->rsts);
+
 	ret = m_can_clk_start(cdev);
 	if (ret)
 		return ret;
 
+	ret = reset_control_deassert(cdev->rsts);
+	if (ret)
+		goto clk_disable;
+
 	if (cdev->is_peripheral) {
 		ret = can_rx_offload_add_manual(cdev->net, &cdev->offload,
 						NAPI_POLL_WEIGHT);
 		if (ret)
-			goto clk_disable;
+			goto out_reset_control_assert;
 	}
 
 	if (!cdev->net->irq) {
@@ -2462,8 +2478,10 @@ int m_can_class_register(struct m_can_classdev *cdev)
 		 KBUILD_MODNAME, cdev->net->irq, cdev->version);
 
 	/* Probe finished
-	 * Stop clocks. They will be reactivated once the M_CAN device is opened
+	 * Assert rest and stop clocks.
+	 * They will be reactivated once the M_CAN device is opened
 	 */
+	reset_control_assert(cdev->rsts);
 	m_can_clk_stop(cdev);
 
 	return 0;
@@ -2471,6 +2489,8 @@ int m_can_class_register(struct m_can_classdev *cdev)
 rx_offload_del:
 	if (cdev->is_peripheral)
 		can_rx_offload_del(&cdev->offload);
+out_reset_control_assert:
+	reset_control_assert(cdev->rsts);
 clk_disable:
 	m_can_clk_stop(cdev);
 
diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
index bd4746c63af3..5e901d5bf6ff 100644
--- a/drivers/net/can/m_can/m_can.h
+++ b/drivers/net/can/m_can/m_can.h
@@ -86,6 +86,7 @@ struct m_can_classdev {
 	struct device *dev;
 	struct clk *hclk;
 	struct clk *cclk;
+	struct reset_control *rsts;
 
 	struct workqueue_struct *tx_wq;
 	struct phy *transceiver;

-- 
2.50.1

Re: [PATCH 0/7] can: m_can: fix pm_runtime and CAN state handling

[Philipp Zabel]

On Di, 2025-08-12 at 19:36 +0200, Marc Kleine-Budde wrote:
> The first patch fixes a pm_runtime imbalance in the m_can_platform
> driver.
> 
> The rest of this series fixes the CAN state handling in the m_can
> driver:
> - add the missing state transition from from Error Warning back to
>   Error Active (Patches 2+3)
> - address the fact that in some SoCs (observed on the STM32MP15) the
>   M_CAN IP core keeps the CAN state and CAN error counters over an
>   internal reset cycle. Set the correct CAN state during ifup and
>   system resume (Patches 4+5)
> - add support for optional shared external reset, to properly reset
>   the IP core (Patches 6+7)

Should this declare a dependency on
https://lore.kernel.org/all/20250807-stm32mp15-m_can-add-reset-v2-1-f69ebbfced1f@pengutronix.de/
or is that already merged?

regards
Philipp

Re: [PATCH 7/7] can: m_can: add optional support for reset

[Philipp Zabel]

On Di, 2025-08-12 at 19:36 +0200, Marc Kleine-Budde wrote:
> In some SoCs (observed on the STM32MP15) the M_CAN IP core keeps the
> CAN state and CAN error counters over an internal reset cycle. The
> STM32MP15 SoC provides an external reset, which is shared between both
> M_CAN cores.
> 
> Add support for an optional external reset. Take care of shared
> resets, de-assert reset during the probe phase in
> m_can_class_register() and while the interface is up, assert the reset
> otherwise.
>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/can/m_can/m_can.c | 26 +++++++++++++++++++++++---
>  drivers/net/can/m_can/m_can.h |  1 +
>  2 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index c24ea0e5599f..0a6d4b523c33 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/reset.h>
>  
>  #include "m_can.h"
>  
> @@ -1833,6 +1834,7 @@ static int m_can_close(struct net_device *dev)
>  
>  	close_candev(dev);
>  
> +	reset_control_assert(cdev->rsts);

Nitpick, "rsts" as in plural? 

[...]
> @@ -2462,8 +2478,10 @@ int m_can_class_register(struct m_can_classdev *cdev)
>  		 KBUILD_MODNAME, cdev->net->irq, cdev->version);
>  
>  	/* Probe finished
> -	 * Stop clocks. They will be reactivated once the M_CAN device is opened
> +	 * Assert rest and stop clocks.

Typo, s/rest/reset/.

Otherwise,


Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

Re: [PATCH 7/7] can: m_can: add optional support for reset

[Marc Kleine-Budde]

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

On 13.08.2025 10:17:52, Philipp Zabel wrote:
> On Di, 2025-08-12 at 19:36 +0200, Marc Kleine-Budde wrote:
> > In some SoCs (observed on the STM32MP15) the M_CAN IP core keeps the
> > CAN state and CAN error counters over an internal reset cycle. The
> > STM32MP15 SoC provides an external reset, which is shared between both
> > M_CAN cores.
> > 
> > Add support for an optional external reset. Take care of shared
> > resets, de-assert reset during the probe phase in
> > m_can_class_register() and while the interface is up, assert the reset
> > otherwise.
> >
> > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > ---
> >  drivers/net/can/m_can/m_can.c | 26 +++++++++++++++++++++++---
> >  drivers/net/can/m_can/m_can.h |  1 +
> >  2 files changed, 24 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> > index c24ea0e5599f..0a6d4b523c33 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/reset.h>
> >  
> >  #include "m_can.h"
> >  
> > @@ -1833,6 +1834,7 @@ static int m_can_close(struct net_device *dev)
> >  
> >  	close_candev(dev);
> >  
> > +	reset_control_assert(cdev->rsts);
> 
> Nitpick, "rsts" as in plural?

fixed.

> 
> [...]
> > @@ -2462,8 +2478,10 @@ int m_can_class_register(struct m_can_classdev *cdev)
> >  		 KBUILD_MODNAME, cdev->net->irq, cdev->version);
> >  
> >  	/* Probe finished
> > -	 * Stop clocks. They will be reactivated once the M_CAN device is opened
> > +	 * Assert rest and stop clocks.
> 
> Typo, s/rest/reset/.

fixed.

> 
> Otherwise,
> 
> 
> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

Thanks, hmmm, why doesn't b4 pick that up?

Marc

-- 
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 7/7] can: m_can: add optional support for reset

[Marc Kleine-Budde]

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

On 13.08.2025 10:34:12, Marc Kleine-Budde wrote:
> > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
> 
> Thanks, hmmm, why doesn't b4 pick that up?

Ahh, that's intentional. b4 will only apply the trailer if the patch is
unchanged to the posted one.

Marc

-- 
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/7] can: m_can: fix pm_runtime and CAN state handling

[Marc Kleine-Budde]

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

On 13.08.2025 10:17:49, Philipp Zabel wrote:
> On Di, 2025-08-12 at 19:36 +0200, Marc Kleine-Budde wrote:
> > The first patch fixes a pm_runtime imbalance in the m_can_platform
> > driver.
> > 
> > The rest of this series fixes the CAN state handling in the m_can
> > driver:
> > - add the missing state transition from from Error Warning back to
> >   Error Active (Patches 2+3)
> > - address the fact that in some SoCs (observed on the STM32MP15) the
> >   M_CAN IP core keeps the CAN state and CAN error counters over an
> >   internal reset cycle. Set the correct CAN state during ifup and
> >   system resume (Patches 4+5)
> > - add support for optional shared external reset, to properly reset
> >   the IP core (Patches 6+7)
> 
> Should this declare a dependency on
> https://lore.kernel.org/all/20250807-stm32mp15-m_can-add-reset-v2-1-f69ebbfced1f@pengutronix.de/
> or is that already merged?

Still open, reference added to the cover letter.

Marc

-- 
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/7] can: m_can: m_can_plat_remove(): add missing pm_runtime_disable()

[Markus Schneider-Pargmann]

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

On Tue Aug 12, 2025 at 7:36 PM CEST, Marc Kleine-Budde wrote:
> Commit 227619c3ff7c ("can: m_can: move runtime PM enable/disable to
> m_can_platform") moved the PM runtime enable from the m_can core
> driver into the m_can_platform.
>
> That patch forgot to move the pm_runtime_disable() to
> m_can_plat_remove(), so that unloading the m_can_platform driver
> causes an "Unbalanced pm_runtime_enable!" error message.
>
> Add the missing pm_runtime_disable() to m_can_plat_remove() to fix the
> problem.
>
> Cc: Patrik Flykt <patrik.flykt@linux.intel.com>
> Fixes: 227619c3ff7c ("can: m_can: move runtime PM enable/disable to m_can_platform")
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

Reviewed-by: Markus Schneider-Pargmann <msp@baylibre.com>

> ---
>  drivers/net/can/m_can/m_can_platform.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/can/m_can/m_can_platform.c b/drivers/net/can/m_can/m_can_platform.c
> index b832566efda0..057eaa7b8b4b 100644
> --- a/drivers/net/can/m_can/m_can_platform.c
> +++ b/drivers/net/can/m_can/m_can_platform.c
> @@ -180,7 +180,7 @@ static void m_can_plat_remove(struct platform_device *pdev)
>  	struct m_can_classdev *mcan_class = &priv->cdev;
>  
>  	m_can_class_unregister(mcan_class);
> -
> +	pm_runtime_disable(mcan_class->dev);
>  	m_can_class_free_dev(mcan_class->net);
>  }
>  


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

Re: [PATCH 2/7] can: m_can: m_can_rx_handler(): only handle active interrupts

[Markus Schneider-Pargmann]

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

On Tue Aug 12, 2025 at 7:36 PM CEST, Marc Kleine-Budde wrote:
> Among other things, the M_CAN IP core has an Interrupt Register (IR)
> and an Interrupt Enable (IE) register. An interrupt is triggered if at
> least 1 bit is set in the bitwise and of IR and IE.
>
> Depending on the configuration not all interrupts are enabled in the
> IE register. However the m_can_rx_handler() IRQ handler looks at all
> interrupts not just the enabled ones. This may lead to handling of not
> activated interrupts.

But isn't that happening for m_can_interrupt_handler() in general then?

Best
Markus

>
> Fix the problem and mask the irqstatus (IR register) with the
> active_interrupts (cache value of IE register).
>
> Fixes: e0d1f4816f2a ("can: m_can: add Bosch M_CAN controller support")
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/can/m_can/m_can.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index fe74dbd2c966..a51dc0bb8124 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -1006,6 +1006,7 @@ static int m_can_rx_handler(struct net_device *dev, int quota, u32 irqstatus)
>  	int rx_work_or_err;
>  	int work_done = 0;
>  
> +	irqstatus &= cdev->active_interrupts;
>  	if (!irqstatus)
>  		goto end;
>  


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

Re: [PATCH 3/7] can: m_can: m_can_handle_state_errors(): fix CAN state transition to Error Active

[Markus Schneider-Pargmann]

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

Hi,

On Tue Aug 12, 2025 at 7:36 PM CEST, Marc Kleine-Budde wrote:
> The CAN Error State is determined by the receive and transmit error
> counters. The CAN error counters decrease when reception/transmission
> is successful, so that a status transition back to the Error Active
> status is possible. This transition is not handled by
> m_can_handle_state_errors().
>
> Add the missing detection of the Error Active state to
> m_can_handle_state_errors() and extend the handling of this state in
> m_can_handle_state_change().
>
> Fixes: e0d1f4816f2a ("can: m_can: add Bosch M_CAN controller support")
> Fixes: cd0d83eab2e0 ("can: m_can: m_can_handle_state_change(): fix state change")
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/can/m_can/m_can.c | 48 ++++++++++++++++++++++++++-----------------
>  1 file changed, 29 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index a51dc0bb8124..b485d2f3d971 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -812,6 +812,10 @@ static int m_can_handle_state_change(struct net_device *dev,
>  	u32 timestamp = 0;
>  
>  	switch (new_state) {
> +	case CAN_STATE_ERROR_ACTIVE:
> +		/* error active state */

This comment and the one below don't really help IMHO.

> +		cdev->can.state = CAN_STATE_ERROR_ACTIVE;
> +		break;
>  	case CAN_STATE_ERROR_WARNING:
>  		/* error warning state */
>  		cdev->can.can_stats.error_warning++;
> @@ -841,6 +845,13 @@ static int m_can_handle_state_change(struct net_device *dev,
>  	__m_can_get_berr_counter(dev, &bec);
>  
>  	switch (new_state) {
> +	case CAN_STATE_ERROR_ACTIVE:
> +		/* error active state */
> +		cf->can_id |= CAN_ERR_CRTL | CAN_ERR_CNT;
> +		cf->data[1] = CAN_ERR_CRTL_ACTIVE;
> +		cf->data[6] = bec.txerr;
> +		cf->data[7] = bec.rxerr;
> +		break;
>  	case CAN_STATE_ERROR_WARNING:
>  		/* error warning state */
>  		cf->can_id |= CAN_ERR_CRTL | CAN_ERR_CNT;
> @@ -877,30 +888,29 @@ static int m_can_handle_state_change(struct net_device *dev,
>  	return 1;
>  }
>  
> +static enum can_state
> +m_can_can_state_get_by_psr(const u32 psr)
> +{
> +	if (psr & PSR_BO)
> +		return CAN_STATE_BUS_OFF;
> +	if (psr & PSR_EP)
> +		return CAN_STATE_ERROR_PASSIVE;
> +	if (psr & PSR_EW)
> +		return CAN_STATE_ERROR_WARNING;

Why should m_can_handle_state_errors() should be called if none of these
flags are set?

m_can_handle_state_errors() seems to only be called if IR_ERR_STATE
which is defined as:
  #define IR_ERR_STATE	(IR_BO | IR_EW | IR_EP)

This is the for the interrupt register but will the PSR register bits be
set without the interrupt register being set?

Best
Markus

> +
> +	return CAN_STATE_ERROR_ACTIVE;
> +}
> +
>  static int m_can_handle_state_errors(struct net_device *dev, u32 psr)
>  {
>  	struct m_can_classdev *cdev = netdev_priv(dev);
> -	int work_done = 0;
> +	enum can_state new_state;
>  
> -	if (psr & PSR_EW && cdev->can.state != CAN_STATE_ERROR_WARNING) {
> -		netdev_dbg(dev, "entered error warning state\n");
> -		work_done += m_can_handle_state_change(dev,
> -						       CAN_STATE_ERROR_WARNING);
> -	}
> +	new_state = m_can_can_state_get_by_psr(psr);
> +	if (new_state == cdev->can.state)
> +		return 0;
>  
> -	if (psr & PSR_EP && cdev->can.state != CAN_STATE_ERROR_PASSIVE) {
> -		netdev_dbg(dev, "entered error passive state\n");
> -		work_done += m_can_handle_state_change(dev,
> -						       CAN_STATE_ERROR_PASSIVE);
> -	}
> -
> -	if (psr & PSR_BO && cdev->can.state != CAN_STATE_BUS_OFF) {
> -		netdev_dbg(dev, "entered error bus off state\n");
> -		work_done += m_can_handle_state_change(dev,
> -						       CAN_STATE_BUS_OFF);
> -	}
> -
> -	return work_done;
> +	return m_can_handle_state_change(dev, new_state);
>  }
>  
>  static void m_can_handle_other_err(struct net_device *dev, u32 irqstatus)


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

Re: [PATCH 4/7] can: m_can: m_can_chip_config(): bring up interface in correct state

[Markus Schneider-Pargmann]

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

Hi,

On Tue Aug 12, 2025 at 7:36 PM CEST, Marc Kleine-Budde wrote:
> In some SoCs (observed on the STM32MP15) the M_CAN IP core keeps the
> CAN state and CAN error counters over an internal reset cycle. An
> external reset is not always possible, due to the shared reset with
> the other CAN core. This caused the core not always be in Error Active
> state when bringing up the controller.
>
> Instead of always setting the CAN state to Error Active in
> m_can_chip_config(), fix this by reading and decoding the Protocol
> Status Regitser (PSR) and set the CAN state accordingly.
>
> Fixes: e0d1f4816f2a ("can: m_can: add Bosch M_CAN controller support")
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/can/m_can/m_can.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index b485d2f3d971..310a907cbb7e 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -1607,6 +1607,7 @@ static int m_can_chip_config(struct net_device *dev)
>  static int m_can_start(struct net_device *dev)
>  {
>  	struct m_can_classdev *cdev = netdev_priv(dev);
> +	u32 reg_psr;
>  	int ret;
>  
>  	/* basic m_can configuration */
> @@ -1617,7 +1618,8 @@ static int m_can_start(struct net_device *dev)
>  	netdev_queue_set_dql_min_limit(netdev_get_tx_queue(cdev->net, 0),
>  				       cdev->tx_max_coalesced_frames);
>  
> -	cdev->can.state = CAN_STATE_ERROR_ACTIVE;
> +	reg_psr = m_can_read(cdev, M_CAN_PSR);
> +	cdev->can.state = m_can_can_state_get_by_psr(reg_psr);

Previous patch makes sense for use here. But how is the state set back
in operation after mcan was in an error state? Maybe I missed the path
back to CAN_STATE_ERROR_ACTIVE somewhere?

Also CAN_STATE_ERROR_ACTIVE is set in resume() as well, should that also
read the PSR instead?

Ans lastly I don't like the function name, because of the repeated can,
maybe something like m_can_error_state_by_psr()?

Best
Markus

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

Re: [PATCH 5/7] can: m_can: fix CAN state in system PM

[Markus Schneider-Pargmann]

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

On Tue Aug 12, 2025 at 7:36 PM CEST, Marc Kleine-Budde wrote:
> A suspend/resume cycle on a down interface results in the interface
> coming up in Error Active state. A suspend/resume cycle on an Up
> interface will always result in Error Active state, regardless of the
> actual CAN state.
>
> During suspend, only set running interfaces to CAN_STATE_SLEEPING.
> During resume only touch the CAN state of running interfaces. For
> wakeup sources, set the CAN state depending on the Protocol Status
> Regitser (PSR), for non wakeup source interfaces m_can_start() will do
> the same.
>
> Fixes: e0d1f4816f2a ("can: m_can: add Bosch M_CAN controller support")
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>

Reviewed-by: Markus Schneider-Pargmann <msp@baylibre.com>

> ---
>  drivers/net/can/m_can/m_can.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index 310a907cbb7e..149f3a8b5f7e 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -2507,12 +2507,11 @@ int m_can_class_suspend(struct device *dev)
>  		}
>  
>  		m_can_clk_stop(cdev);
> +		cdev->can.state = CAN_STATE_SLEEPING;
>  	}
>  
>  	pinctrl_pm_select_sleep_state(dev);
>  
> -	cdev->can.state = CAN_STATE_SLEEPING;
> -
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(m_can_class_suspend);
> @@ -2525,14 +2524,14 @@ int m_can_class_resume(struct device *dev)
>  
>  	pinctrl_pm_select_default_state(dev);
>  
> -	cdev->can.state = CAN_STATE_ERROR_ACTIVE;
> -
>  	if (netif_running(ndev)) {
>  		ret = m_can_clk_start(cdev);
>  		if (ret)
>  			return ret;
>  
>  		if (cdev->pm_wake_source) {
> +			u32 reg_psr;
> +
>  			/* Restore active interrupts but disable coalescing as
>  			 * we may have missed important waterlevel interrupts
>  			 * between suspend and resume. Timers are already
> @@ -2544,6 +2543,9 @@ int m_can_class_resume(struct device *dev)
>  			if (cdev->ops->init)
>  				ret = cdev->ops->init(cdev);
>  
> +			reg_psr = m_can_read(cdev, M_CAN_PSR);
> +			cdev->can.state = m_can_can_state_get_by_psr(reg_psr);
> +
>  			m_can_write(cdev, M_CAN_IE, cdev->active_interrupts);
>  		} else {
>  			ret  = m_can_start(ndev);


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

Re: [PATCH 3/7] can: m_can: m_can_handle_state_errors(): fix CAN state transition to Error Active

[Markus Schneider-Pargmann]

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

On Wed Aug 20, 2025 at 10:49 AM CEST, Markus Schneider-Pargmann wrote:
> Hi,
>
> On Tue Aug 12, 2025 at 7:36 PM CEST, Marc Kleine-Budde wrote:
>> The CAN Error State is determined by the receive and transmit error
>> counters. The CAN error counters decrease when reception/transmission
>> is successful, so that a status transition back to the Error Active
>> status is possible. This transition is not handled by
>> m_can_handle_state_errors().
>>
>> Add the missing detection of the Error Active state to
>> m_can_handle_state_errors() and extend the handling of this state in
>> m_can_handle_state_change().
>>
>> Fixes: e0d1f4816f2a ("can: m_can: add Bosch M_CAN controller support")
>> Fixes: cd0d83eab2e0 ("can: m_can: m_can_handle_state_change(): fix state change")
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>> ---
>>  drivers/net/can/m_can/m_can.c | 48 ++++++++++++++++++++++++++-----------------
>>  1 file changed, 29 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
>> index a51dc0bb8124..b485d2f3d971 100644
>> --- a/drivers/net/can/m_can/m_can.c
>> +++ b/drivers/net/can/m_can/m_can.c
>> @@ -812,6 +812,10 @@ static int m_can_handle_state_change(struct net_device *dev,
>>  	u32 timestamp = 0;
>>  
>>  	switch (new_state) {
>> +	case CAN_STATE_ERROR_ACTIVE:
>> +		/* error active state */
>
> This comment and the one below don't really help IMHO.
>
>> +		cdev->can.state = CAN_STATE_ERROR_ACTIVE;
>> +		break;
>>  	case CAN_STATE_ERROR_WARNING:
>>  		/* error warning state */
>>  		cdev->can.can_stats.error_warning++;
>> @@ -841,6 +845,13 @@ static int m_can_handle_state_change(struct net_device *dev,
>>  	__m_can_get_berr_counter(dev, &bec);
>>  
>>  	switch (new_state) {
>> +	case CAN_STATE_ERROR_ACTIVE:
>> +		/* error active state */
>> +		cf->can_id |= CAN_ERR_CRTL | CAN_ERR_CNT;
>> +		cf->data[1] = CAN_ERR_CRTL_ACTIVE;
>> +		cf->data[6] = bec.txerr;
>> +		cf->data[7] = bec.rxerr;
>> +		break;
>>  	case CAN_STATE_ERROR_WARNING:
>>  		/* error warning state */
>>  		cf->can_id |= CAN_ERR_CRTL | CAN_ERR_CNT;
>> @@ -877,30 +888,29 @@ static int m_can_handle_state_change(struct net_device *dev,
>>  	return 1;
>>  }
>>  
>> +static enum can_state
>> +m_can_can_state_get_by_psr(const u32 psr)
>> +{
>> +	if (psr & PSR_BO)
>> +		return CAN_STATE_BUS_OFF;
>> +	if (psr & PSR_EP)
>> +		return CAN_STATE_ERROR_PASSIVE;
>> +	if (psr & PSR_EW)
>> +		return CAN_STATE_ERROR_WARNING;
>
> Why should m_can_handle_state_errors() should be called if none of these
> flags are set?
>
> m_can_handle_state_errors() seems to only be called if IR_ERR_STATE
> which is defined as:
>   #define IR_ERR_STATE	(IR_BO | IR_EW | IR_EP)
>
> This is the for the interrupt register but will the PSR register bits be
> set without the interrupt register being set?

After reading the other users of the above function, I do see why this
was added. I am still wondering if there is a way to return to
ERROR_ACTIVE once the errors are cleared from the error register.

Also looking at all the users added for the function above, could you
read the register inside the function? Currently you are adding a
reg variable and a read call for each call to this function.
m_can_handle_state_errors() also doesn't need the psr value with your
refactoring.

Best
Markus

>
> Best
> Markus
>
>> +
>> +	return CAN_STATE_ERROR_ACTIVE;
>> +}
>> +
>>  static int m_can_handle_state_errors(struct net_device *dev, u32 psr)
>>  {
>>  	struct m_can_classdev *cdev = netdev_priv(dev);
>> -	int work_done = 0;
>> +	enum can_state new_state;
>>  
>> -	if (psr & PSR_EW && cdev->can.state != CAN_STATE_ERROR_WARNING) {
>> -		netdev_dbg(dev, "entered error warning state\n");
>> -		work_done += m_can_handle_state_change(dev,
>> -						       CAN_STATE_ERROR_WARNING);
>> -	}
>> +	new_state = m_can_can_state_get_by_psr(psr);
>> +	if (new_state == cdev->can.state)
>> +		return 0;
>>  
>> -	if (psr & PSR_EP && cdev->can.state != CAN_STATE_ERROR_PASSIVE) {
>> -		netdev_dbg(dev, "entered error passive state\n");
>> -		work_done += m_can_handle_state_change(dev,
>> -						       CAN_STATE_ERROR_PASSIVE);
>> -	}
>> -
>> -	if (psr & PSR_BO && cdev->can.state != CAN_STATE_BUS_OFF) {
>> -		netdev_dbg(dev, "entered error bus off state\n");
>> -		work_done += m_can_handle_state_change(dev,
>> -						       CAN_STATE_BUS_OFF);
>> -	}
>> -
>> -	return work_done;
>> +	return m_can_handle_state_change(dev, new_state);
>>  }
>>  
>>  static void m_can_handle_other_err(struct net_device *dev, u32 irqstatus)


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