Re: flexcan missing error state transitions

[Wolfgang Grandegger <wg@grandegger.com>]

Hello ZHU Yi,

Am 31.08.2017 um 10:33 schrieb ZHU Yi (ST-FIR/ENG1-Zhu):
> Hello Wolfgang,
> 
>> From: Wolfgang Grandegger [mailto:wg@grandegger.com]
>> Sent: Wednesday, August 30, 2017 6:59 PM
>> [...]
>>
>>> The error interrupt will always be triggered after the send, because the
>>> auto-retransmission will keep getting Ack error if the cable is not connected,
>>> that's where the interrupt flooding come from (and the flooding won't stop
>>> until the cable is connected again).
>>
>> You mean the error interrupt signalling the ACK error occurs even with
>> FLEXCAN_CTRL_ERR_MSK *not* set in "ctrl"?
>> Or is it set from the beginning?
> Sorry for the incomplete description, I forgot to mention it is assumed
> that the broken error state quirk fix is enabled for the core, hence
> the error interrupt is enabled from the beginning.

OK.

> Although only p1010 enabled the quirk so far, but to our understanding,
> the rest core seems also need the quirk enabled to report correct state
> transitions, e.g., i.MX6.

I will have a i.MX53 and i.MX28 board soon on my table to check error 
passive handling. Would be nice to reduce interrupt flooding on these as 
well.

> However, as neither of us can test the legacy and other cores, so we
> cannot touch them without testing. Perhaps someone who has the access
> to these cores can give a hand to verify that in future.

Other tester are welcome!

>>
>> [...]
>> Here FLEXCAN_CTRL_ERR_MSK (0x4000) is set in "ctrl"!
>>
>> Could you please add to flexcan_irq():
>>
>> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
>> index 13f0f21..0df0c34 100644
>> --- a/drivers/net/can/flexcan.c
>> +++ b/drivers/net/can/flexcan.c
>> @@ -758,6 +758,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
>>          }
>>
>>          reg_esr = flexcan_read(&regs->esr);
>> +       netdev_dbg(dev, "reg_iflag1=%#x reg_esr=%#x\n", reg_iflag1, regs->esr);
>>
>>          /* ACK all bus error and state change IRQ sources */
>>          if (reg_esr & FLEXCAN_ESR_ALL_INT) {
>> 	reg_esr = flexcan_read(&regs->esr);
>>
>> This would make cleaner what is going on.
> Sure. Below test is based on kernel v4.11 with the proposed patches.
> To demonstrate the theory of the legacy cores should also work, the
> "[TR]WRN_INT" are disabled.
> (clear WRN_EN in MCR and [TR]WRN_MSK in CTRL1)

OK, then I understand how the following works:
> Test case:
> 1. Disconnect the cable
> 2. "ip link set xxx type can bitrate 500000" and "ip link set xxx up"
> 3. "cansend xxx 123#ffffffffffffffff"
> 
> # dmesg | grep flexcan
> [    1.561243] flexcan 2090000.flexcan: 2090000.flexcan supply xceiver not found, using dummy regulator
> [    1.570621] flexcan 2090000.flexcan: can_rx_offload_init_queue: skb_queue_len_max=512
> [    1.571207] flexcan 2090000.flexcan: device registered (reg_base=e0a78000, irq=34)
> [    6.227746] flexcan 2090000.flexcan mob0: renamed from can0
> [   34.400669] flexcan 2090000.flexcan mob0: writing ctrl=0x013a2007
> [   34.400698] flexcan 2090000.flexcan mob0: flexcan_set_bittiming: mcr=0x5980000f ctrl=0x013a2007
> [   34.400715] flexcan 2090000.flexcan mob0: flexcan_chip_start: writing mcr=0x5983023f
> [   34.400731] flexcan 2090000.flexcan mob0: flexcan_chip_start: writing ctrl=0x013a2057
> [   34.400829] flexcan 2090000.flexcan mob0: flexcan_chip_start: reading mcr=0x4083023f ctrl=0x013ae057
> [   44.112307] flexcan 2090000.flexcan mob0: reg_iflag1=0x0 reg_esr=0x42042
> [   44.112547] flexcan 2090000.flexcan mob0: reg_iflag1=0x0 reg_esr=0x42042
> [   44.112810] flexcan 2090000.flexcan mob0: reg_iflag1=0x0 reg_esr=0x42042
> [   44.113074] flexcan 2090000.flexcan mob0: reg_iflag1=0x0 reg_esr=0x42042
> [   44.113331] flexcan 2090000.flexcan mob0: reg_iflag1=0x0 reg_esr=0x42042
> [   44.113603] flexcan 2090000.flexcan mob0: reg_iflag1=0x0 reg_esr=0x42042
> [   44.113851] flexcan 2090000.flexcan mob0: reg_iflag1=0x0 reg_esr=0x42042
> [   44.114124] flexcan 2090000.flexcan mob0: reg_iflag1=0x0 reg_esr=0x42042
> [   44.114367] flexcan 2090000.flexcan mob0: reg_iflag1=0x0 reg_esr=0x42042
> [   44.114631] flexcan 2090000.flexcan mob0: reg_iflag1=0x0 reg_esr=0x42042
> [   44.114884] flexcan 2090000.flexcan mob0: reg_iflag1=0x0 reg_esr=0x42042
> [   44.115142] flexcan 2090000.flexcan mob0: reg_iflag1=0x0 reg_esr=0x42242
> [   44.115162] flexcan 2090000.flexcan mob0: New error state: 1
> [   44.115404] flexcan 2090000.flexcan mob0: reg_iflag1=0x0 reg_esr=0x42242
> [   44.115661] flexcan 2090000.flexcan mob0: reg_iflag1=0x0 reg_esr=0x42242
> [   44.115921] flexcan 2090000.flexcan mob0: reg_iflag1=0x0 reg_esr=0x42242
> [   44.116180] flexcan 2090000.flexcan mob0: reg_iflag1=0x0 reg_esr=0x42252
> [   44.116199] flexcan 2090000.flexcan mob0: New error state: 2

We disable the bus errors when error passive is reached. But what 
happens if the error state decreases. It will require successful 
transmissions. When the TX done interrupt comes, changes to error 
warning are realized. Then we re-enable the error interrupt. But below 
error warning, we would like to disable the error interrupt (for the 
mx6q). That might be a problem but could be fixed by special treatment 
(for legacy cores).

Wolfgang.