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: Tuesday, November 21, 2017 7:43 AM
> To: Pankaj Bansal <pankaj.bansal@nxp.com>; Wolfgang Grandegger
> <wg@grandegger.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: Pankaj Bansal [mailto:pankaj.bansal@nxp.com]
> >Sent: Monday, November 20, 2017 7:11 PM  [...] Thank you Zhu Yi for
> >this explanation. This helped me in understanding the quirks.
> >Is there any flexcan core which generates passive state interrupt ?
> I'm afraid so far no known flexcan core supports it.
> 
> >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.
> Yes, you're right. We use the error interrupts to report active -> warning ->
> passive, and use the tx/rx interrupts to report passive -> warning -> active.
>

Thanks Zhu Yi. In that case do we even need to put these changes in a quirk?
Shouldn't this code be generic flexcan code, irrespective of the platform?
 
> >
> > [...]
> >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}
> Seems it needs the FLEXCAN_QUIRK_BROKEN_PERR_STATE. After that, above
> steps should be able to report the correct state - error passive, and all the
> scenarios Wolfgang mentioned should also works fine.
> 
> Please be aware that you need to send multiple can frames (manually with
> cansend or automatically with cangen) to get the state back to warning and
> active if removed the error condition on the bus.
> 

I will test this after PERR_STATE quirk. But as I mentioned above, in my opinion
we should remove FLEXCAN_QUIRK_BROKEN_PERR_STATE, and make these changes
common for all platforms.

> Best regards
> Yi