[PATCH 1/2] can: D_CAN: perform a sofware reset on open

[PATCH 1/2] can: D_CAN: perform a sofware reset on open

[Jeroen Hofstee]

When the C_CAN interface is closed it is put in power down mode, but
does not reset the error counters / state. So reset the D_CAN on open,
so the reported state and the actual state match.

According to [1], the C_CAN module doesn't have the software reset.

[1] http://www.bosch-semiconductors.com/media/ip_modules/pdf_2/c_can_fd8/users_manual_c_can_fd8_r210_1.pdf

Signed-off-by: Jeroen Hofstee <jhofstee@victronenergy.com>
---
 drivers/net/can/c_can/c_can.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 606b7d8ffe13..502a181d02e7 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -52,6 +52,7 @@
 #define CONTROL_EX_PDR		BIT(8)
 
 /* control register */
+#define CONTROL_SWR		BIT(15)
 #define CONTROL_TEST		BIT(7)
 #define CONTROL_CCE		BIT(6)
 #define CONTROL_DISABLE_AR	BIT(5)
@@ -569,6 +570,26 @@ static void c_can_configure_msg_objects(struct net_device *dev)
 				   IF_MCONT_RCV_EOB);
 }
 
+static int software_reset(struct net_device *dev)
+{
+	struct c_can_priv *priv = netdev_priv(dev);
+	int retry = 0;
+
+	if (priv->type != BOSCH_D_CAN)
+		return 0;
+
+	priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_SWR | CONTROL_INIT);
+	while (priv->read_reg(priv, C_CAN_CTRL_REG) & CONTROL_SWR) {
+		msleep(20);
+		if (retry++ > 100) {
+			netdev_err(dev, "CCTRL: software reset failed\n");
+			return -EIO;
+		}
+	}
+
+	return 0;
+}
+
 /*
  * Configure C_CAN chip:
  * - enable/disable auto-retransmission
@@ -578,6 +599,11 @@ static void c_can_configure_msg_objects(struct net_device *dev)
 static int c_can_chip_config(struct net_device *dev)
 {
 	struct c_can_priv *priv = netdev_priv(dev);
+	int err;
+
+	err = software_reset(dev);
+	if (err)
+		return err;
 
 	/* enable automatic retransmission */
 	priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_ENABLE_AR);
-- 
2.17.1

[PATCH 2/2] can: C_CAN: add bus recovery events

[Jeroen Hofstee]

While the state is update when the error counters increase and decrease,
there is no event when the bus recovers and the error counters decrease
again. So add that event as well.

Change the state going downward to be ERROR_PASSIVE -> ERROR_WARNING ->
ERROR_ACTIVE instead of directly to ERROR_ACTIVE again.

Signed-off-by: Jeroen Hofstee <jhofstee@victronenergy.com>
---
 drivers/net/can/c_can/c_can.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 502a181d02e7..5cfaab18e10b 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -912,6 +912,9 @@ static int c_can_handle_state_change(struct net_device *dev,
 	struct can_berr_counter bec;
 
 	switch (error_type) {
+	case C_CAN_NO_ERROR:
+		priv->can.state = CAN_STATE_ERROR_ACTIVE;
+		break;
 	case C_CAN_ERROR_WARNING:
 		/* error warning state */
 		priv->can.can_stats.error_warning++;
@@ -942,6 +945,13 @@ static int c_can_handle_state_change(struct net_device *dev,
 				ERR_CNT_RP_SHIFT;
 
 	switch (error_type) {
+	case C_CAN_NO_ERROR:
+		/* error warning state */
+		cf->can_id |= CAN_ERR_CRTL;
+		cf->data[1] = CAN_ERR_CRTL_ACTIVE;
+		cf->data[6] = bec.txerr;
+		cf->data[7] = bec.rxerr;
+		break;
 	case C_CAN_ERROR_WARNING:
 		/* error warning state */
 		cf->can_id |= CAN_ERR_CRTL;
@@ -1080,11 +1090,17 @@ static int c_can_poll(struct napi_struct *napi, int quota)
 	/* handle bus recovery events */
 	if ((!(curr & STATUS_BOFF)) && (last & STATUS_BOFF)) {
 		netdev_dbg(dev, "left bus off state\n");
-		priv->can.state = CAN_STATE_ERROR_ACTIVE;
+		work_done += c_can_handle_state_change(dev, C_CAN_ERROR_PASSIVE);
 	}
+
 	if ((!(curr & STATUS_EPASS)) && (last & STATUS_EPASS)) {
 		netdev_dbg(dev, "left error passive state\n");
-		priv->can.state = CAN_STATE_ERROR_ACTIVE;
+		work_done += c_can_handle_state_change(dev, C_CAN_ERROR_WARNING);
+	}
+
+	if ((!(curr & STATUS_EWARN)) && (last & STATUS_EWARN)) {
+		netdev_dbg(dev, "left error warning state\n");
+		work_done += c_can_handle_state_change(dev, C_CAN_NO_ERROR);
 	}
 
 	/* handle lec errors on the bus */
-- 
2.17.1

Re: [PATCH 2/2] can: C_CAN: add bus recovery events

[Kurt Van Dijck]

On do, 26 sep 2019 08:50:51 +0000, Jeroen Hofstee wrote:
> While the state is update when the error counters increase and decrease,
> there is no event when the bus recovers and the error counters decrease
> again. So add that event as well.
> 
> Change the state going downward to be ERROR_PASSIVE -> ERROR_WARNING ->
> ERROR_ACTIVE instead of directly to ERROR_ACTIVE again.

This looks like a proper thing to do
Acked-by: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>

Re: [PATCH 2/2] can: C_CAN: add bus recovery events

[Kurt Van Dijck]

On di, 01 okt 2019 09:40:57 +0200, Kurt Van Dijck wrote:
> On do, 26 sep 2019 08:50:51 +0000, Jeroen Hofstee wrote:
> > While the state is update when the error counters increase and decrease,
> > there is no event when the bus recovers and the error counters decrease
> > again. So add that event as well.
> > 
> > Change the state going downward to be ERROR_PASSIVE -> ERROR_WARNING ->
> > ERROR_ACTIVE instead of directly to ERROR_ACTIVE again.
> 
> This looks like a proper thing to do
> Acked-by: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>

Thanks for this, now I see when the bus is normal again.

Tested-by: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>

Re: [PATCH 1/2] can: D_CAN: perform a sofware reset on open

[Kurt Van Dijck]

On do, 26 sep 2019 08:50:44 +0000, Jeroen Hofstee wrote:
> When the C_CAN interface is closed it is put in power down mode, but
> does not reset the error counters / state. So reset the D_CAN on open,
> so the reported state and the actual state match.
> 
> According to [1], the C_CAN module doesn't have the software reset.
> 
> [1] http://www.bosch-semiconductors.com/media/ip_modules/pdf_2/c_can_fd8/users_manual_c_can_fd8_r210_1.pdf
> 
> Signed-off-by: Jeroen Hofstee <jhofstee@victronenergy.com>

I observed no problems after applying the patch.

Tested-by: Kurt Van Dijck <dev.kurt@vandijck-laurijssen.be>

Re: [PATCH 1/2] can: D_CAN: perform a sofware reset on open

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 2484 bytes --]

On 9/26/19 10:50 AM, Jeroen Hofstee wrote:
> When the C_CAN interface is closed it is put in power down mode, but
> does not reset the error counters / state. So reset the D_CAN on open,
> so the reported state and the actual state match.
> 
> According to [1], the C_CAN module doesn't have the software reset.
> 
> [1] http://www.bosch-semiconductors.com/media/ip_modules/pdf_2/c_can_fd8/users_manual_c_can_fd8_r210_1.pdf
> 
> Signed-off-by: Jeroen Hofstee <jhofstee@victronenergy.com>
> ---
>  drivers/net/can/c_can/c_can.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 606b7d8ffe13..502a181d02e7 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -52,6 +52,7 @@
>  #define CONTROL_EX_PDR		BIT(8)
>  
>  /* control register */
> +#define CONTROL_SWR		BIT(15)
>  #define CONTROL_TEST		BIT(7)
>  #define CONTROL_CCE		BIT(6)
>  #define CONTROL_DISABLE_AR	BIT(5)
> @@ -569,6 +570,26 @@ static void c_can_configure_msg_objects(struct net_device *dev)
>  				   IF_MCONT_RCV_EOB);
>  }
>  
> +static int software_reset(struct net_device *dev)

Please add the common prefix "c_can_" to the function

> +{
> +	struct c_can_priv *priv = netdev_priv(dev);
> +	int retry = 0;
> +
> +	if (priv->type != BOSCH_D_CAN)
> +		return 0;
> +
> +	priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_SWR | CONTROL_INIT);
> +	while (priv->read_reg(priv, C_CAN_CTRL_REG) & CONTROL_SWR) {
> +		msleep(20);
> +		if (retry++ > 100) {
> +			netdev_err(dev, "CCTRL: software reset failed\n");
> +			return -EIO;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * Configure C_CAN chip:
>   * - enable/disable auto-retransmission
> @@ -578,6 +599,11 @@ static void c_can_configure_msg_objects(struct net_device *dev)
>  static int c_can_chip_config(struct net_device *dev)
>  {
>  	struct c_can_priv *priv = netdev_priv(dev);
> +	int err;
> +
> +	err = software_reset(dev);
> +	if (err)
> +		return err;
>  
>  	/* enable automatic retransmission */
>  	priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_ENABLE_AR);
> 

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] can: D_CAN: perform a sofware reset on open

[Jeroen Hofstee]

Hello Marc,

On 10/1/19 4:32 PM, Marc Kleine-Budde wrote:
> On 9/26/19 10:50 AM, Jeroen Hofstee wrote:
>> When the C_CAN interface is closed it is put in power down mode, but
>> does not reset the error counters / state. So reset the D_CAN on open,
>> so the reported state and the actual state match.
>>
>> According to [1], the C_CAN module doesn't have the software reset.
>>
>> [1] http://www.bosch-semiconductors.com/media/ip_modules/pdf_2/c_can_fd8/users_manual_c_can_fd8_r210_1.pdf
>>
>> Signed-off-by: Jeroen Hofstee <jhofstee@victronenergy.com>
>> ---
>>   drivers/net/can/c_can/c_can.c | 26 ++++++++++++++++++++++++++
>>   1 file changed, 26 insertions(+)
>>
>> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
>> index 606b7d8ffe13..502a181d02e7 100644
>> --- a/drivers/net/can/c_can/c_can.c
>> +++ b/drivers/net/can/c_can/c_can.c
>> @@ -52,6 +52,7 @@
>>   #define CONTROL_EX_PDR		BIT(8)
>>   
>>   /* control register */
>> +#define CONTROL_SWR		BIT(15)
>>   #define CONTROL_TEST		BIT(7)
>>   #define CONTROL_CCE		BIT(6)
>>   #define CONTROL_DISABLE_AR	BIT(5)
>> @@ -569,6 +570,26 @@ static void c_can_configure_msg_objects(struct net_device *dev)
>>   				   IF_MCONT_RCV_EOB);
>>   }
>>   
>> +static int software_reset(struct net_device *dev)
> Please add the common prefix "c_can_" to the function

Fine with me, I did sent a v2.

Regards,
Jeroen