RE: [PATCH v2 2/2] can: flexcan: adding platform specific details for LS1021A

[Pankaj Bansal <pankaj.bansal@nxp.com>]

> -----Original Message-----
> From: ZHU Yi (ST-FIR/ENG1-Zhu) [mailto:Yi.Zhu5@cn.bosch.com]
> Sent: Thursday, November 16, 2017 12:53 PM
> To: Wolfgang Grandegger <wg@grandegger.com>; Pankaj Bansal
> <pankaj.bansal@nxp.com>; Marc Kleine-Budde <mkl@pengutronix.de>;
> linux-can@vger.kernel.org
> Cc: Varun Sethi <V.Sethi@nxp.com>; Poonam Aggrwal
> <poonam.aggrwal@nxp.com>; Bhupesh Sharma
> <bhupesh.sharma@freescale.com>
> Subject: RE: [PATCH v2 2/2] can: flexcan: adding platform specific details for
> LS1021A
> 
> Hello Pankaj,
> 
> >From: Wolfgang Grandegger [mailto:wg@grandegger.com]
> >Sent: Thursday, November 16, 2017 3:06 PM
> >
> >Am 16.11.2017 um 06:34 schrieb Pankaj Bansal:
> >> [...]
> >>> Please add your IP core to the "FLEXCAN hardware feature flags"
> >>> table [1] in the driver. Also please test the transition interrupts
> >>> for changes in the CAN error state, see the commits of ZHU Yi
> >>> (ST-FIR/ENG1-Zhu) <Yi.Zhu5@cn.bosch.com> on the driver. [2]
> >>>
> >>
> >> I did not understand this quirk (FLEXCAN_QUIRK_BROKEN_PERR_STATE).
> >> What does this quirk do ? how do I check if my IP supports it or not ?
> >> How do I test this ?
> >>
> >> Can you please help me to understand ?
> >
> >It's about reporting CAN error state changes "error-active" -> "warning"
> >-> "error-passive" -> "bus-off" and back. The quirk might be required
> >-> to
> >trigger the state-change to "error-passive".
> 
> The FLEXCAN_QUIRK_BROKEN_PERR_STATE in conjunction with the
> FLEXCAN_QUIRK_BROKEN_WERR_STATE were added for:
> 1. solve error state transition problems found in some core 2. throttle error
> interrupt flooding when applicable due to some core cannot generate state
> interrupt for error warning (that's the WERR_STATE stands for) and/or error
> passive (that's the PERR_STATE stands for), thus the user space cannot
> receive correct state transitions via listening to the SocketCAN socket.
> 
> The workaround overcomes the interrupt support shortage by:
> 1. derive correct state from error counters upon any interrupt.
> 2. disable/enable error interrupt to minimize the potential
>    performance impact caused by error interrupt flooding.
> 
> These two quirks need to set according to hardware features, e.g., the
> i.MX28 and i.MX6 set the PERR_STATE quirk because they have the
> [TR]WRN_INT connected but no interrupt for error passive. If they don't have
> the [TR]WRN_INT too, then both quirks are required.
> (PS: As we understand so far, there is no flexcan core lack of error warning
> interrupt but have error passive interrupt, so the WERR_STATE should never
> been set alone).
> 

Thank you Zhu Yi for this explanation. This helped me in understanding the quirks.
Is there any flexcan core which generates passive state interrupt ?
The reason I ask this is because I see in flexcan doc :

The module has many interrupt sources: interrupts due to message buffers and interrupts
due to the ORed interrupts from MBs, Bus Off, Error, Tx Warning, and Rx Warning.

Also in flexcan.c :
#define FLEXCAN_ESR_ALL_INT \
	(FLEXCAN_ESR_TWRN_INT | FLEXCAN_ESR_RWRN_INT | \
	 FLEXCAN_ESR_BOFF_INT | FLEXCAN_ESR_ERR_INT)

I don't see (in document or in code), if passive state interrupts are even generated ?
I see that we can detect passive state using ESR[FLTCONF]  = 01.
But I think we need to rely on ERROR interrupts to do that (even if the BERR_REPORTING is off).

Please correct me if I am wrong in my understanding.

> >
> >You can test such state changes by doing:
> >
> >1. Send messages without CAN cable connected
> >
> >2. Short-circuit the CAN low and high signals
> >
> >Then connect the cable and remove the short-circuit and send messages
> >using "cansend" or "cangen". Please also use "ip set link can0 ...
> >restart-ms 100" to recover from bus-off automatically.
> >
> >While testing watch the error messages with the command below and
> >report the results to the list.
> >
> ># candump -td -e any,0:0,#FFFFFFFF
> 
> The fast way to check whether your IP needs the quirk or not is look into the
> reference and find whether it supports the aforementioned interrupts, if
> anyone is missing, then the quirk is needed. Please consult suggestion from
> Wolfgang about how to test.
> 

Thank you Wolfgang. With the steps you mentioned and some debug logs I see that LS1021A
supports [TR]WRN_INT.

root@TinyDistro:~# ip link set can0 up type can bitrate 125000 restart-ms 100 berr-reporting off
[  596.117066] IPv6: ADDRCONF(NETDEV_CHANGE): can0: link becomes ready
root@TinyDistro:~# ip link set can1 up type can bitrate 125000 restart-ms 100 berr-reporting off
[  600.724547] IPv6: ADDRCONF(NETDEV_CHANGE): can1: link becomes ready
root@TinyDistro:~# candump -td -e any,0:0,#FFFFFFFF &
[1] 632
root@TinyDistro:~# cansend can0 5A1#123412341234
root@TinyDistro:~# [  647.522941] flexcan_irq 804 reg_esr=0x00062242, reg_ecr=0x00000060, reg_ctrl=0x4a31ac55
 (000.000000)  can0  20000004   [8]  00 08 00 00 00 00 00 00   ERRORFRAME
        controller-problem{tx-error-warning}

> Best regards
> Yi