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

Marc Kleine-Budde posted 7 patches 1 month, 3 weeks ago
There is a newer version of this series
[PATCH 4/7] can: m_can: m_can_chip_config(): bring up interface in correct state
Posted by Marc Kleine-Budde 1 month, 3 weeks ago
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
Re: [PATCH 4/7] can: m_can: m_can_chip_config(): bring up interface in correct state
Posted by Markus Schneider-Pargmann 1 month, 2 weeks ago
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
Re: [PATCH 4/7] can: m_can: m_can_chip_config(): bring up interface in correct state
Posted by Marc Kleine-Budde 3 weeks, 4 days ago
On 20.08.2025 11:00:51, Markus Schneider-Pargmann wrote:
> > 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?

Sorry, I don't exactly get what you mean here.

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

see next patch :)

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

It's m_can_state_get_by_psr() now.

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