Message-ID: <4e9d0d223af785f060103ad67c14272a@agner.ch>
Date:	Tue, 05 Aug 2014 14:38:20 +0200
From:	Stefan Agner <stefan@...er.ch>
To:	Marc Kleine-Budde <mkl@...gutronix.de>
Cc:	shawn.guo@...escale.com, kernel@...gutronix.de,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linux-can@...r.kernel.org
Subject: Re: [PATCH v3 4/4] can: flexcan: add vf610 support for FlexCAN

Am 2014-08-05 11:52, schrieb Marc Kleine-Budde:
> On 08/04/2014 06:01 PM, Stefan Agner wrote:
>> Am 2014-08-04 16:27, schrieb Marc Kleine-Budde:
>>> On 08/04/2014 03:43 PM, Stefan Agner wrote:
>>> [...]
>>>
>>>>> Thanks for the test, so far looks promising :) With this setup the other
>>>>> CAN node repeats the CAN frame until it's ACKed. Because there is no
>>>>> node with a compatible bitrate, there is no ACking CAN node.
>>>>>
>>>>> Can you add a third CAN node to the network. The second and third node
>>>>> should use the same bitrate, while your vf610 uses a different one. With
>>>>> the new setup it should take more than one frame until the vf610 goes
>>>>> into error warning and even more frames to error passive. This way we
>>>>> can see it the error warning interrupt is connected or not. The error
>>>>> counters should increase with each "wrong" bitrate frame it sees, you
>>>>> can check with:
>>>>>
>>>>>     ip -details link show can0
>>>>>
>>>>> The output looks like this:
>>>>>
>>>>>> 4: can0: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN mode DEFAULT qlen 10
>>>>>>     link/can
>>>>>>     can state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 0
>>>>>                               ^^^^^^^^^^^^^^^^^^^^^^
>>>>>>     bitrate 1000000 sample-point 0.750
>>>>>>     tq 125 prop-seg 2 phase-seg1 3 phase-seg2 2 sjw 1
>>>>>>     sja1000: tseg1 1..16 tseg2 1..8 sjw 1..4 brp 1..64 brp-inc 1
>>>>>>     clock 8000000
>>>>>
>>>>> When one of the berr-counter crosses 96 (and stays below 128) a warning
>>>>> interrupt should be generated.
>>>>
>>>> Ok, created this setup, could successfully communicate with all three
>>>> nodes. I then set the Vybrid to half of the bitrate. When I send a frame
>>>> from Vybrid, the berr-counter tx immediately ends up at 128 and the
>>>> device is in ERROR-PASSIVE:
>>>
>>> This is expected.
>>>
>>>> root@...ibri-vf:~# ip -details link show can1
>>>> 3: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN
>>>> mode DEFAULT group default qlen 10
>>>>     link/can  promiscuity 0
>>>>     can state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 0
>>>>     bitrate 124990 sample-point 0.739
>>>>     tq 347 prop-seg 8 phase-seg1 8 phase-seg2 6 sjw 1
>>>>     flexcan: tseg1 4..16 tseg2 2..8 sjw 1..4 brp 1..256 brp-inc 1
>>>>     clock 83368421
>>>             ^^^^^^^^
>>>
>>> BTW: the can core has a really weird clock rate, have a look at the
>>> datasheet if you manage to route a 24 MHz clock (or another multiple of
>>> 8MHz) to the flexcan core. I had a quick glance at the datasheet, if I
>>> understand it correctly the Fast OSC clock runs with 24 MHz.
>>>
>>
>> This pinmux is actually part of the IPs CTRL register (see CLKSRC).
>> Currently this is set unconditionally to peripheral clock, which is on
>> my device 83.3 MHz (500MHz/6). This would be a bit bigger change. Just
>> thinking how to implement this.
> 
> Sigh! I've missed the fact, that this bit is _inside_ the CAN core
> (again). On the mx53 and imx6 this bit was obsolete and the clock was
> selected outside of the CAN core.
> 
>> Maybe we have to reference a third clock
>> "osc" and a device tree property fsl,can-clock-osc which one can choose
>> between "osc" and "per" what do you think?
> 
> Yes, but that's a different patch. I thought it would be as easy as on
> the mx53, where you just had to reconfigure the clock tree.
> 

Agreed.

>>>> root@...ibri-vf:~# cansend can1 1F334455#1122334455667788
>>>> interface = can1, family = 29, type = 3, proto = 1
>>>> root@...ibri-vf:~# [  818.886664] flexcan_irq, esr=00062242
>>>> [  818.890365] flexcan_irq, ctrl=1c3dac57
>>>>
>>>> root@...ibri-vf:~# ip -details link show can1
>>>> 3: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN
>>>> mode DEFAULT group default qlen 10
>>>>     link/can  promiscuity 0
>>>>     can state ERROR-PASSIVE (berr-counter tx 128 rx 0) restart-ms 0
>>>>     bitrate 124990 sample-point 0.739
>>>>     tq 347 prop-seg 8 phase-seg1 8 phase-seg2 6 sjw 1
>>>>     flexcan: tseg1 4..16 tseg2 2..8 sjw 1..4 brp 1..256 brp-inc 1
>>>>     clock 83368421
>>>>
>>>>
>>>> When I send the frames from another device on the bus, I can see the rx
>>>> count incrementing by one on each frame I send. As you expected, the
>>>> device changes to ERROR-WARNING when crossing the 96 frame boundary:
>>>
>>> This is correct
>>>
>>
>> Just found out when using too high bitrate (500000 vs 125000), the error
>> counter immediately goes up to 128 (or even beyond) and ends up in
>> ERROR-PASSIVE:
> 
> Maybe there is a off-by-one error in the flexcan core, so that a rx
> error of 127 doesn't trigger an error passive interrupt.
> 
>> # ip -details link show can1
>> [  292.164820] flexcan_get_berr_counter, esr=00000000
>> [  292.169715] flexcan_get_berr_counter, esr=00040190
>> 3: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN
>> mode DEFAULT group default qlen 10
>>     link/can  promiscuity 0
>>     can state ERROR-PASSIVE (berr-counter tx 0 rx 135) restart-ms 0
>>     bitrate 298811 sample-point 0.777
>>     tq 371 prop-seg 3 phase-seg1 3 phase-seg2 2 sjw 1
>>     flexcan: tseg1 4..16 tseg2 2..8 sjw 1..4 brp 1..256 brp-inc 1
>>     clock 83368421
> 
> [...]
> 
>>>> However, once reaching 128, I don't get another interrupt and the device
>>>> stays in ERROR-WARNING:
>>>
>>> The contents of the esr reg would be interesting, especially the
>>> FLT_CONF part.
> 
>> root@...ridvf61-v11:~# [  222.221651] flexcan_irq, esr=0005050a
>> [  222.225349] flexcan_irq, ctrl=1c3dac57
> 
>> ip -details link show can1
>> [  223.791082] flexcan_get_berr_counter, esr=00000000
>> [  223.796246] flexcan_get_berr_counter, esr=00040180
>> 3: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN
>> mode DEFAULT group default qlen
>> 10
>>     link/can  promiscuity 0
>>     can state ERROR-WARNING (berr-counter tx 0 rx 96) restart-ms 0
>>     bitrate 124990 sample-point 0.739
>>     tq 347 prop-seg 8 phase-seg1 8 phase-seg2 6 sjw 1
>>     flexcan: tseg1 4..16 tseg2 2..8 sjw 1..4 brp 1..256 brp-inc 1
>>     clock 83368421
> 
>> root@...ridvf61-v11:~# ip -details link show can1
>> [  243.571175] flexcan_get_berr_counter, esr=00000000
>> [  243.576343] flexcan_get_berr_counter, esr=00040582
>> 3: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN
>> mode DEFAULT group default qlen
>> 10
>>     link/can  promiscuity 0
>>     can state ERROR-WARNING (berr-counter tx 0 rx 104) restart-ms 0
>>     bitrate 124990 sample-point 0.739
>>     tq 347 prop-seg 8 phase-seg1 8 phase-seg2 6 sjw 1
>>     flexcan: tseg1 4..16 tseg2 2..8 sjw 1..4 brp 1..256 brp-inc 1
>>     clock 83368421
>>
>> ...
>>
>> root@...ridvf61-v11:~# ip -details link show can1
>> [  299.831192] flexcan_get_berr_counter, esr=00000000
>> [  299.836358] flexcan_get_berr_counter, esr=00040582
>> 3: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN
>> mode DEFAULT group default qlen 10
>>     link/can  promiscuity 0
>>     can state ERROR-WARNING (berr-counter tx 0 rx 126) restart-ms 0
>>     bitrate 124990 sample-point 0.739
>>     tq 347 prop-seg 8 phase-seg1 8 phase-seg2 6 sjw 1
>>     flexcan: tseg1 4..16 tseg2 2..8 sjw 1..4 brp 1..256 brp-inc 1
>>     clock 83368421
>> root@...ridvf61-v11:~# ip -details link show can1
>> [  302.411025] flexcan_get_berr_counter, esr=00000000
>> [  302.416195] flexcan_get_berr_counter, esr=00040592
>> 3: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN
>> mode DEFAULT group default qlen 10
>>     link/can  promiscuity 0
>>     can state ERROR-WARNING (berr-counter tx 0 rx 128) restart-ms 0
>>     bitrate 124990 sample-point 0.739
>>     tq 347 prop-seg 8 phase-seg1 8 phase-seg2 6 sjw 1
>>     flexcan: tseg1 4..16 tseg2 2..8 sjw 1..4 brp 1..256 brp-inc 1
>>     clock 83368421
>>
>> The FLTCONF is in Error Passive, however the stack did not received that
>> information. I still have the printk in the interrupt, however I did not
>> get an interrupt here (while I get one when it switched to
>> ERROR-WARNING... But it looks like the ERRINT is still pending...
> 
> According to the datasheet the ERRINT indicates a bit error (bit 10...15
> is set), not the error passive state.
> 
> Marc

True, but fact is, I don't get a interrupt when the system switches from
ERROR-WARNING to ERROR-PASSIVE state. However, when I set berr-reporting
on, I get an interrupt (probably because of a change of bit 10...15),
but the state change is recognized:

ip -details link show can1
[ 2303.571656] flexcan_get_berr_counter, esr=00000000
[ 2303.576832] flexcan_get_berr_counter, esr=00040180
3: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN
mode DEFAULT group default qlen 10
    link/can  promiscuity 0 
    can <BERR-REPORTING> state ERROR-WARNING (berr-counter tx 0 rx 127)
restart-ms 0 
    bitrate 124990 sample-point 0.739 
    tq 347 prop-seg 8 phase-seg1 8 phase-seg2 6 sjw 1
    flexcan: tseg1 4..16 tseg2 2..8 sjw 1..4 brp 1..256 brp-inc 1
    clock 83368421
root@...ridvf61-v11:~# [ 2307.023789] flexcan_irq, esr=0004051a
[ 2307.027490] flexcan_irq, ctrl=1c3dec57
ip -details link show can1
[ 2309.081840] flexcan_get_berr_counter, esr=00000000
[ 2309.087016] flexcan_get_berr_counter, esr=00040190
3: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN
mode DEFAULT group default qlen 10
    link/can  promiscuity 0 
    can <BERR-REPORTING> state ERROR-PASSIVE (berr-counter tx 0 rx 128)
restart-ms 0 
    bitrate 124990 sample-point 0.739 
    tq 347 prop-seg 8 phase-seg1 8 phase-seg2 6 sjw 1
    flexcan: tseg1 4..16 tseg2 2..8 sjw 1..4 brp 1..256 brp-inc 1
    clock 83368421

It looks like, the FLTCONF state change itself doesn't lead to a
interrupt when the state changes to ERROR-PASSIVE. IMHO, this is also
not explicitly stated in the Vybrid RM (46.3.8).

What is the solution here? Force enabling ERR interrupt? The
FLEXCAN_HAS_BROKEN_ERR_STATE flag would be appropriate, however, this
was meant to work around the missing WRN_INT, which seems not to be the
case here...

--
Stefan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists