Re: [PATCH net-next-2.6 v5 1/1] can: c_can: Added support for Bosch C_CAN controller

[Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>]

Hi Bhupesh,

On 02/01/2011 05:29 AM, Bhupesh SHARMA wrote:
> Hello Wolfgang,
> 
>> ...
>>>>> +		/* handle error on the bus */
>>>>> +		lec_type = c_can_has_and_handle_berr(priv);
>>>>> +		if (lec_type && (error_type != C_CAN_NO_ERROR))
>>>>> +			work_done += c_can_err(dev, error_type, lec_type);
>>>>
>>>> State changes are only reported if berr_reporting is enabled and a
>> bus
>>>> error occured. This needs to be fixed.
>>>
>>> As I mentioned earlier in a response to a review comment, the Bus
>> Error
>>> reporting for C_CAN seems different from sja1000 and flexcan
>> approaches.
>>> Do you think it will be useful to drop CAN_CTRLMODE_BERR_REPORTING
>> from
>>> priv->can.ctrlmode_supported as done by *pch* driver? Or do you have
>>> a better idea..
>>
>> You bus error reporting is OK. The problem is that it does not only
>> affect bus errors but also state changes. State change messages should
>> alway be send independent of priv->can.ctrlmode. It's just a matter of
>> moving code to the right location. E.g. the code snippet above inside
>> c_can_err() before you check for bus errors.
>>
>>>> Feel free to send the output of "candump any,0:0,#FFFFFFFF" when
>>>> sending
>>>> messages without cable connected and with a bus error provocuted.
>>>
>>> OK. I will try to cross-compile candump for my arm-v7 architecture
>>> and will send the output.
>>
> 
> I did some changes to the code to ensure that the state change and lec
> handling are handled separately and properly.

Great.

> Please find the candump any,0:0,#FFFFFFFF output below:
> 
> 1. With No-Cable connected, I keep getting:
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME
>   can0  20000004  [8] 00 28 00 00 00 00 00 00   ERRORFRAME

The SJA1000 reports:

---- Error-active -> error-warning -> error-passive
---- cable disconnected
-bash-3.2# ./candump -t d any,0:0,#FFFFFFFF

 (0.000000)  can0  20000004  [8] 00 08 00 00 00 00 60 00   ERROR-WARNING
 (0.002588)  can0  20000004  [8] 00 20 00 00 00 00 80 00   ERROR-PASSIVE

I wonder why you get data[1]=28 immediately. Also it's nice to have the
txerr and rxerr in data[6..7].

> 2. With Tx and Rx shorted to simulate bus-error, I get:
>   can0  20000044  [8] 00 20 00 00 00 00 00 00   ERRORFRAME

Also here, you should see:

---- Error-active -> error-warning -> error-passive -> bus-off

-bash-3.2# ./candump -t d any,0:0,#FFFFFFFF
 (0.000000)  can0  20000004  [8] 00 08 00 00 00 00 88 00   ERROR-WARNING
 (0.001362)  can0  20000004  [8] 00 20 00 00 00 00 88 00   ERROR-PASSIVE
 (0.001560)  can0  20000044  [8] 00 00 00 00 00 00 7F 00   BUS-OFF

> In case 2, when I enable debug messages I get the correct state change sequence:
> entered error warning state
> entered error passive state
> entered bus off state

As the log confirms.

> Does this result seem fine to you?

Not yet. Also the output with "berr-reporting on" should be checked. And
the output when you recover from bus-off manually (using "ip link set
type can restart")

Thanks,

Wolfgang.