Re: can, tcan4x5x: look to merge rpi support into rpi kernel tree

Re: can, tcan4x5x: look to merge rpi support into rpi kernel tree

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3113 bytes --]

I've added the linux-can mailing list on Cc.

On 15.02.2021 15:41:57, Torin Cooper-Bennun wrote:
> On Mon, 15 Feb 2021 at 14:45, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > Sadly, the driver is still not in good shape...I think it will explode
> > as soon as you receive a CAN frame on the rpi, as the frames are passed
> > into the networking stack from the wrong context...
> >
> > Maybe I'll find some time to get receive properly working.
> 
> I'm afraid I've just found that myself -- in fact, I'm having problems
> getting TX to behave as well. I think the chip configuration is
> incorrect (at the very least, the chip is never put into standby mode,
> which the datasheet says is paramount!)

Do you have the wake-gpio in your DT? This one works for me:

|         tcan4x5x: tcan4x5x@1 {
|                 reg = <1>;
|                 compatible = "ti,tcan4x5x";
|                 vsup-supply = <&reg_tcan4x5x_vsup>;
|                 pinctrl-names = "default";
|                 pinctrl-0 = <&pinctrl_tcan4x5x>;
|                 spi-max-frequency = <10000000>;
|                 bosch,mram-cfg = <0x0 0 0 16 0 0 1 1>;
|                 interrupt-parent = <&gpio4>;
|                 interrupts = <31 IRQ_TYPE_LEVEL_LOW>;
|                 clock-names = "cclk";
|                 clocks = <&tcan4x5x_osc>;
|                 device-wake-gpios = <&gpio5 5 GPIO_ACTIVE_HIGH>;
|         };

> The current config procedure
> ends up with bizarre activity on the CAN lines. I also observed
> intermittent refcount warnings during driver use.

You mean something like these...

| [  543.116807] WARNING: CPU: 0 PID: 11 at lib/refcount.c:25 refcount_warn_saturate+0x108/0x174
| [  543.116820] refcount_t: addition on 0; use-after-free.

with can_put_echo_skb() in the call stack?

| [  543.117745] [<bf186edc>] (can_put_echo_skb [can_dev]) from [<bf1d67ec>] (mcp251xfd_start_xmit+0x2b0/0x3bc [mcp251xfd])

We tried to fix the problem, but the patch made the problem more
visible. Working on this.

> I've cherry-picked the relevant changes onto RPi kernel 5.10 :
> https://github.com/tcbennun/linux/commit/c32a0d422b551390f6960243f29e1afacfe30d48
> and I'll be next trying the bleeding-edge driver with 5.11.
> 
> > BTW: what kind of hardware are you using?
> 
> This is a Raspberry Pi 3 Model B v1.2, hosting a TCAN4550 on spi0. The
> external oscillator for the TCAN4550 is 20 MHz.

Is that a custom tcan pi hat, or is it officially sold somewhere?

> Since you've confirmed it needs work, I'll probably be able to put
> some time into it myself.

First thing I'd do is to rewrite the RX function and IRQ handler for the
"peripheral", that's the code path used for the SPI attached m-can
core. TX doesn't look efficient, but it should work at least.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: can, tcan4x5x: look to merge rpi support into rpi kernel tree

[Torin Cooper-Bennun]

On Mon, 15 Feb 2021 at 17:44, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> Do you have the wake-gpio in your DT? This one works for me:

We actually don't break out WAKE on our board, and using this board
I've written a TCAN4550 driver for MCUs and haven't required device
wake-up via WAKE or other means.

My DT:

|            tcan4x5x: tcan4x5x@0 {
|                reg = <0>;
|                compatible = "ti,tcan4x5x";
|                pinctrl-names = "default";
|                pinctrl-0 = <&tcan4x5x_pins>;
|                spi-max-frequency = <4000000>;
|                bosch,mram-cfg = <0x0 0 0 10 0 0 0 10>;
|                interrupt-parent = <&gpio>;
|                interrupts = <25 IRQ_TYPE_LEVEL_LOW>;
|                clock-names = "cclk";
|                clocks = <&clk_tcan4x5x_osc>;
|            };

> You mean something like these...
>
> | [  543.116807] WARNING: CPU: 0 PID: 11 at lib/refcount.c:25 refcount_warn_saturate+0x108/0x174
> | [  543.116820] refcount_t: addition on 0; use-after-free.
>
> with can_put_echo_skb() in the call stack?
>
> | [  543.117745] [<bf186edc>] (can_put_echo_skb [can_dev]) from [<bf1d67ec>] (mcp251xfd_start_xmit+0x2b0/0x3bc [mcp251xfd])

Yes, exactly. I have also seen, when putting the interface down...

[   69.378407] WARNING: CPU: 3 PID: 740 at lib/refcount.c:28
refcount_warn_saturate+0x13c/0x174
[   69.378413] refcount_t: underflow; use-after-free.

...with can_flush_echo_skb() in the stack this time:

[   69.378857] [<7f1de528>] (can_flush_echo_skb [can_dev]) from
[<7f1de5c8>] (close_candev+0x2c/0x30 [can_dev])

> > This is a Raspberry Pi 3 Model B v1.2, hosting a TCAN4550 on spi0. The
> > external oscillator for the TCAN4550 is 20 MHz.
>
> Is that a custom tcan pi hat, or is it officially sold somewhere?

It's a custom board, jerry-rigged to a Pi.

> First thing I'd do is to rewrite the RX function and IRQ handler for the
> "peripheral", that's the code path used for the SPI attached m-can
> core. TX doesn't look efficient, but it should work at least.

Thanks, I'll take a look. I am concerned about this weird behaviour
when trying to TX, though. I'll walk through the chip config and
compare with my known working process.

-- 
Regards,

Torin Cooper-Bennun
www.maxiluxsystems.com | Software Engineer

Re: can, tcan4x5x: look to merge rpi support into rpi kernel tree

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1137 bytes --]

On 2/16/21 10:06 AM, Torin Cooper-Bennun wrote:
> On Mon, 15 Feb 2021 at 17:44, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>> Do you have the wake-gpio in your DT? This one works for me:
> 
> We actually don't break out WAKE on our board, and using this board
> I've written a TCAN4550 driver for MCUs and haven't required device
> wake-up via WAKE or other means.
> 
> My DT:
> 
> |            tcan4x5x: tcan4x5x@0 {
> |                reg = <0>;
> |                compatible = "ti,tcan4x5x";
> |                pinctrl-names = "default";
> |                pinctrl-0 = <&tcan4x5x_pins>;
> |                spi-max-frequency = <4000000>;
> |                bosch,mram-cfg = <0x0 0 0 10 0 0 0 10>;

Your mram-cfg looks broken, You don't have any TX Event FIFO entries. Please use
this one:

bosch,mram-cfg = <0x0 0 0 16 0 0 1 1>;

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: can, tcan4x5x: look to merge rpi support into rpi kernel tree

[Torin Cooper-Bennun]

> Your mram-cfg looks broken, You don't have any TX Event FIFO entries. Please use
> this one:
>
> bosch,mram-cfg = <0x0 0 0 16 0 0 1 1>;

Okay, thanks for the tip, I usually don't make use of the TX event
feature! This hasn't fixed the TX behaviour, though. I've verified
that the frame is loaded into MRAM and requested for TX correctly, but
the transceiver does this...

1. sends start-of-frame, then 5 bits of ID correctly, at the correct bitrate
2. bus then seems to be stuck dominant for 6 bits instead of
transmitting further ID bits
3. bus lines drift back recessive (not a clean edge) over the duration
of 12 bits
4. process repeats

Steps 2-3 constitute an error frame, if my understanding is correct.
Presently the TCAN4550 is not connected to any other CAN nodes; I
would therefore expect the transmit to at least continue to the ACK
field.


--
Regards,

Torin Cooper-Bennun
www.maxiluxsystems.com | Software Engineer

Re: can, tcan4x5x: look to merge rpi support into rpi kernel tree

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1650 bytes --]

On 16.02.2021 09:44:54, Torin Cooper-Bennun wrote:
> > Your mram-cfg looks broken, You don't have any TX Event FIFO entries. Please use
> > this one:
> >
> > bosch,mram-cfg = <0x0 0 0 16 0 0 1 1>;
> 
> Okay, thanks for the tip, I usually don't make use of the TX event
> feature!

You might not, but the driver does :)

> This hasn't fixed the TX behaviour, though. I've verified
> that the frame is loaded into MRAM and requested for TX correctly, but
> the transceiver does this...
> 
> 1. sends start-of-frame, then 5 bits of ID correctly, at the correct bitrate
> 2. bus then seems to be stuck dominant for 6 bits instead of
> transmitting further ID bits
> 3. bus lines drift back recessive (not a clean edge) over the duration
> of 12 bits
> 4. process repeats
> 
> Steps 2-3 constitute an error frame, if my understanding is correct.
> Presently the TCAN4550 is not connected to any other CAN nodes; I
> would therefore expect the transmit to at least continue to the ACK
> field.

If the process repeats infinitely then the CAN controller doesn't go
into bus-off, which means the CAN bus is terminated correctly.

With your setup of only one node on the bus and correct termination, I
too think the frame should be send until the ACK field. I suggest to
first create a working CAN bus setup and then add the tcan to it.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: can, tcan4x5x: look to merge rpi support into rpi kernel tree

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2163 bytes --]

Please keep the mailing list on Cc.

On 16.02.2021 11:11:34, Torin Cooper-Bennun wrote:
> On Tue, 16 Feb 2021 at 10:28, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > You might not, but the driver does :)
> 
> Indeed, on closer inspection I can see that!
> 
> > If the process repeats infinitely then the CAN controller doesn't go
> > into bus-off, which means the CAN bus is terminated correctly.
> >
> > With your setup of only one node on the bus and correct termination, I
> > too think the frame should be send until the ACK field. I suggest to
> > first create a working CAN bus setup and then add the tcan to it.
> 
> I have to thank you for mentioning termination, because it turned out
> I had not used correct termination at all. How embarrassing! With that
> fixed, the transceiver is happy and I have working TX and RX.

\o/

> Mostly working RX, anyway. When putting the interface down and then
> back up (but allowing a second node to continue attempting TX in the
> meantime) I see some blank frames...
> 
> can0  001   [8]  00 01 02 03 04 05 06 07
> can0  001   [8]  00 01 02 03 04 05 06 07
> can0  001   [8]  00 01 02 03 04 05 06 07
> can0  000   [0]
> can0  000   [0]
> can0  001   [8]  00 01 02 03 04 05 06 07
> can0  001   [8]  00 01 02 03 04 05 06 07
> etc.
> 
> (where the 8-byte frame is what the bus is actually seeing.)

Where do you see these blank frames? On the sending rpi with candump? An
on the bus (with a second system) you only see the full 8 byte long
frames?

Use "candump any,0~0,#FFFFFFFF -extA" to get RX/TX annotation.
Use "cangen -Ii -L8 -D000102030405060708 -g100 can0" to get increasing
CAN-IDs, so you can figure out if a CAN frame got lost.

Seems I have to add the TX path to the list of broken things...

The mcp251xfd driver can be used as a template for the tcan4x5x driver.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: can, tcan4x5x: look to merge rpi support into rpi kernel tree

[Torin Cooper-Bennun]

On Tue, 16 Feb 2021 at 11:19, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> Please keep the mailing list on Cc.

Agh! My bad.

> Where do you see these blank frames? On the sending rpi with candump? An
> on the bus (with a second system) you only see the full 8 byte long
> frames?

My apologies, I should have clarified. The Pi is receiving all these
frames. The sending node is sending the same frame every time (I'll
have it send increasing IDs from now on).

> Use "candump any,0~0,#FFFFFFFF -extA" to get RX/TX annotation.
> Use "cangen -Ii -L8 -D000102030405060708 -g100 can0" to get increasing
> CAN-IDs, so you can figure out if a CAN frame got lost.

Fab, thank you.

> Seems I have to add the TX path to the list of broken things...
>
> The mcp251xfd driver can be used as a template for the tcan4x5x driver.

I'll go ahead and compare with mcp251xfd and continue testing. I'll be
working towards getting this stable in the coming weeks.


-- 
Regards,

Torin Cooper-Bennun
www.maxiluxsystems.com | Software Engineer

Re: can, tcan4x5x: look to merge rpi support into rpi kernel tree

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2381 bytes --]

On 16.02.2021 11:38:17, Torin Cooper-Bennun wrote:
> > Where do you see these blank frames? On the sending rpi with candump? An
> > on the bus (with a second system) you only see the full 8 byte long
> > frames?
> 
> My apologies, I should have clarified. The Pi is receiving all these
> frames. The sending node is sending the same frame every time (I'll
> have it send increasing IDs from now on).

This means the tcan receives two "ghost" frames here?

| can0  001   [8]  00 01 02 03 04 05 06 07
| can0  001   [8]  00 01 02 03 04 05 06 07
| can0  001   [8]  00 01 02 03 04 05 06 07
| can0  000   [0]
| can0  000   [0]
| can0  001   [8]  00 01 02 03 04 05 06 07
| can0  001   [8]  00 01 02 03 04 05 06 07

If the tcan driver shuts down the tcan chip properly, depending on your
setup, the sending CAN node might be the only one on the bus, leading to
repeat the CAN frame due to no ACK. This easily triggers race conditions
when starting up the tcan driver again.

With proper timestamps and increasing CAN-IDs you can figure out if the
driver clears the bits on the CAN messages (number of blank frames ==
number of missing frames) or if it "inserts" blank frames (== no missing
frames).

If the number of blank frames equals the number of missing frames, then
have a look where the message RAM is initialized:

    https://elixir.bootlin.com/linux/latest/source/drivers/net/can/m_can/tcan4x5x.c#L335

Oh! This doesn't look right :(

I think it's a bad idea to first bring the chip into normal mode and
then initialize the RAM.

> > Use "candump any,0~0,#FFFFFFFF -extA" to get RX/TX annotation.
> > Use "cangen -Ii -L8 -D000102030405060708 -g100 can0" to get increasing
> > CAN-IDs, so you can figure out if a CAN frame got lost.
> 
> Fab, thank you.
> 
> > Seems I have to add the TX path to the list of broken things...
> >
> > The mcp251xfd driver can be used as a template for the tcan4x5x driver.
> 
> I'll go ahead and compare with mcp251xfd and continue testing. I'll be
> working towards getting this stable in the coming weeks.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: can, tcan4x5x: look to merge rpi support into rpi kernel tree

[Torin Cooper-Bennun]

On Tue, Feb 16, 2021 at 01:32:35PM +0100, Marc Kleine-Budde wrote:
> If the number of blank frames equals the number of missing frames, then
> have a look where the message RAM is initialized:

This is the case, but it occurred over a matter of seconds (the frames
were only being sent about twice a second). I've finally got some time
to investigate, so I'll try to find out what's going on.

>     https://elixir.bootlin.com/linux/latest/source/drivers/net/can/m_can/tcan4x5x.c#L335
> 
> Oh! This doesn't look right :(
> 
> I think it's a bad idea to first bring the chip into normal mode and
> then initialize the RAM.

I'm unsure whether this caused it, but needless to say, this is bad in
any case!

--
Regards,

Torin Cooper-Bennun
Software Engineer  | maxiluxsystems.com

Re: can, tcan4x5x: look to merge rpi support into rpi kernel tree

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1107 bytes --]

On 2/26/21 1:27 PM, Torin Cooper-Bennun wrote:
> On Tue, Feb 16, 2021 at 01:32:35PM +0100, Marc Kleine-Budde wrote:
>> If the number of blank frames equals the number of missing frames, then
>> have a look where the message RAM is initialized:
> 
> This is the case, but it occurred over a matter of seconds (the frames
> were only being sent about twice a second). I've finally got some time
> to investigate, so I'll try to find out what's going on.
> 
>>     https://elixir.bootlin.com/linux/latest/source/drivers/net/can/m_can/tcan4x5x.c#L335
>>
>> Oh! This doesn't look right :(
>>
>> I think it's a bad idea to first bring the chip into normal mode and
>> then initialize the RAM.
> 
> I'm unsure whether this caused it, but needless to say, this is bad in
> any case!

Feel free to send patches. :D

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: can, tcan4x5x: look to merge rpi support into rpi kernel tree

[Torin Cooper-Bennun]

Hi again. Going back to the start of our conversation:

On Mon, Feb 15, 2021 at 03:45:09PM +0100, Marc Kleine-Budde wrote:
> Sadly, the driver is still not in good shape...I think it will explode
> as soon as you receive a CAN frame on the rpi, as the frames are passed
> into the networking stack from the wrong context...
> 
> Maybe I'll find some time to get receive properly working.

Could you clarify what your concern is? netif_receive_skb seems to be
used in the same way in m_can.c as in rx-offload.c.

Re: can, tcan4x5x: look to merge rpi support into rpi kernel tree

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 993 bytes --]

On 2/26/21 1:18 PM, Torin Cooper-Bennun wrote:
> Hi again. Going back to the start of our conversation:
> 
> On Mon, Feb 15, 2021 at 03:45:09PM +0100, Marc Kleine-Budde wrote:
>> Sadly, the driver is still not in good shape...I think it will explode
>> as soon as you receive a CAN frame on the rpi, as the frames are passed
>> into the networking stack from the wrong context...
>>
>> Maybe I'll find some time to get receive properly working.
> 
> Could you clarify what your concern is? netif_receive_skb seems to be
> used in the same way in m_can.c as in rx-offload.c.

The problem is that netif_receive_skb(skb) is called from a threaded interrupt
handler, but you should call it from NAPI.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: can, tcan4x5x: look to merge rpi support into rpi kernel tree

[Torin Cooper-Bennun]

On Fri, Feb 26, 2021 at 01:22:59PM +0100, Marc Kleine-Budde wrote:
> The problem is that netif_receive_skb(skb) is called from a threaded interrupt
> handler, but you should call it from NAPI.

Ah, I was sure it was in fact called from NAPI, but that's only if
is_peripheral is false. It should be as simple as getting rid of this
switch, then, no?

https://elixir.bootlin.com/linux/latest/source/drivers/net/can/m_can/m_can.c#L966

--
Regards,

Torin Cooper-Bennun
Software Engineer  | maxiluxsystems.com

Re: can, tcan4x5x: look to merge rpi support into rpi kernel tree

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 877 bytes --]

On 2/26/21 1:31 PM, Torin Cooper-Bennun wrote:
> On Fri, Feb 26, 2021 at 01:22:59PM +0100, Marc Kleine-Budde wrote:
>> The problem is that netif_receive_skb(skb) is called from a threaded interrupt
>> handler, but you should call it from NAPI.
> 
> Ah, I was sure it was in fact called from NAPI, but that's only if
> is_peripheral is false. It should be as simple as getting rid of this
> switch, then, no?
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/net/can/m_can/m_can.c#L966

I think you cannot do (synchronous) SPI from NAPI (which is SoftIRQ context).

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: can, tcan4x5x: look to merge rpi support into rpi kernel tree

[Torin Cooper-Bennun]

On Fri, Feb 26, 2021 at 01:40:35PM +0100, Marc Kleine-Budde wrote:
> I think you cannot do (synchronous) SPI from NAPI (which is SoftIRQ context).

I see. I now understand why you recommend using mcp251xfd as a template
for m_can.

Am I correct in saying that rx-offload requires RX timestamps to be
available? They can be enabled in M_CAN, but the v3.3 manual states that
the internal timestamp generation (apparently only option for tcan4x5x)
cannot be used for CAN-FD.

--
Regards,

Torin Cooper-Bennun
Software Engineer | maxiluxsystems.com

Re: can, tcan4x5x: look to merge rpi support into rpi kernel tree

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1017 bytes --]

On 26.02.2021 13:26:20, Torin Cooper-Bennun wrote:
> On Fri, Feb 26, 2021 at 01:40:35PM +0100, Marc Kleine-Budde wrote:
> > I think you cannot do (synchronous) SPI from NAPI (which is SoftIRQ context).
> 
> I see. I now understand why you recommend using mcp251xfd as a template
> for m_can.
> 
> Am I correct in saying that rx-offload requires RX timestamps to be
> available?

Proper timestamps for RX and TX would be best, but you can use
can_rx_offload_queue_tail() if you don't have any timestamps.

> They can be enabled in M_CAN, but the v3.3 manual states that
> the internal timestamp generation (apparently only option for tcan4x5x)
> cannot be used for CAN-FD.

Can you give me a reference to this?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: can, tcan4x5x: look to merge rpi support into rpi kernel tree

[Torin Cooper-Bennun]

On Fri, Feb 26, 2021 at 02:39:36PM +0100, Marc Kleine-Budde wrote:
> > Am I correct in saying that rx-offload requires RX timestamps to be
> > available?
> 
> Proper timestamps for RX and TX would be best, but you can use
> can_rx_offload_queue_tail() if you don't have any timestamps.
> 
> > They can be enabled in M_CAN, but the v3.3 manual states that
> > the internal timestamp generation (apparently only option for tcan4x5x)
> > cannot be used for CAN-FD.
> 
> Can you give me a reference to this?

Here's the manual for 3.3 (annoyingly, I can't find corresponding
documents for earlier M_CAN versions, and TCAN4550 uses 3.2...)

http://www.bosch-semiconductors.com/media/ip_modules/pdf_2/m_can/mcan_users_manual_v330.pdf

See Section 2.3.9, "Timestamp Counter Configuration (TSCC)". Quote:

    Note: With CAN FD an external counter is required for timestamp
    generation (TSS = “10”)

--
Regards,

Torin Cooper-Bennun
Software Engineer | maxiluxsystems.com

Re: can, tcan4x5x: look to merge rpi support into rpi kernel tree

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 928 bytes --]

On 26.02.2021 13:45:22, Torin Cooper-Bennun wrote:
> > Can you give me a reference to this?
> 
> Here's the manual for 3.3 (annoyingly, I can't find corresponding
> documents for earlier M_CAN versions, and TCAN4550 uses 3.2...)

https://github.com/hartkopp/M_CAN-User-Manual-History

> http://www.bosch-semiconductors.com/media/ip_modules/pdf_2/m_can/mcan_users_manual_v330.pdf
> 
> See Section 2.3.9, "Timestamp Counter Configuration (TSCC)". Quote:
> 
>     Note: With CAN FD an external counter is required for timestamp
>     generation (TSS = “10”)

I suggest to check on the tcan hardware, what's really in the register.

Marc 

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: can, tcan4x5x: look to merge rpi support into rpi kernel tree

[Torin Cooper-Bennun]

On Fri, Feb 26, 2021 at 03:00:54PM +0100, Marc Kleine-Budde wrote:
> https://github.com/hartkopp/M_CAN-User-Manual-History

Great, thanks. I can see this also sits in the linux-can repo which is
handy.

> >     Note: With CAN FD an external counter is required for timestamp
> >     generation (TSS = “10”)
> 
> I suggest to check on the tcan hardware, what's really in the register.

I've just verified that internal timestamping works on the TCAN4550 (in
both RX FIFO and TX event FIFO), with FD and BRS both enabled. The
16-bit timestamps are generated specifically from the nomimal bit times,
and you can prescale up to 16x. \o/

Is timestamp-wrapping a concern for rx-offloading?

--
Regards,

Torin Cooper-Bennun
Software Engineer | maxiluxsystems.com

Re: can, tcan4x5x: look to merge rpi support into rpi kernel tree

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1769 bytes --]

On 26.02.2021 15:26:33, Torin Cooper-Bennun wrote:
> On Fri, Feb 26, 2021 at 03:00:54PM +0100, Marc Kleine-Budde wrote:
> > https://github.com/hartkopp/M_CAN-User-Manual-History
> 
> Great, thanks. I can see this also sits in the linux-can repo which is
> handy.
> 
> > >     Note: With CAN FD an external counter is required for timestamp
> > >     generation (TSS = “10”)
> > 
> > I suggest to check on the tcan hardware, what's really in the register.
> 
> I've just verified that internal timestamping works on the TCAN4550 (in
> both RX FIFO and TX event FIFO), with FD and BRS both enabled. The
> 16-bit timestamps are generated specifically from the nomimal bit times,
> and you can prescale up to 16x. \o/

NICE! \o/

> Is timestamp-wrapping a concern for rx-offloading?

No, as long as you provide a proper u32 timestamp, which means wrap
around at 0xffffffff to 0x0. As the tcan4x5x has only a 16 bit
timestamp, shift it to full 32 bit, like this:

https://elixir.bootlin.com/linux/v5.11/source/drivers/net/can/flexcan.c#L805

On the mcp with true 32 bit timestamp there's a wraparound every 107
seconds at 40 MHz external clock (0xffffffff / 40000000).

The tcan has the TSS.TCP as a prescaler and increments in bit time,
which is 5MHz max. This results in a wrap around every 13ms (0xffff /
5000000) with a prescaler of 1. With a prescaler of 16, you can increase
this to 209ms (0xffff / 5000000 * 16), which gives an accuracy of 2
bytes.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: can, tcan4x5x: look to merge rpi support into rpi kernel tree

[Torin Cooper-Bennun]

On Fri, Feb 26, 2021 at 05:01:35PM +0100, Marc Kleine-Budde wrote:
> > Is timestamp-wrapping a concern for rx-offloading?
> 
> No, as long as you provide a proper u32 timestamp, which means wrap
> around at 0xffffffff to 0x0. As the tcan4x5x has only a 16 bit
> timestamp, shift it to full 32 bit, like this:
> 
> https://elixir.bootlin.com/linux/v5.11/source/drivers/net/can/flexcan.c#L805
> 
> On the mcp with true 32 bit timestamp there's a wraparound every 107
> seconds at 40 MHz external clock (0xffffffff / 40000000).
> 
> The tcan has the TSS.TCP as a prescaler and increments in bit time,
> which is 5MHz max. This results in a wrap around every 13ms (0xffff /
> 5000000) with a prescaler of 1. With a prescaler of 16, you can increase
> this to 209ms (0xffff / 5000000 * 16), which gives an accuracy of 2
> bytes.

Nice, thank you, that all sounds good. I'll get to work. :)

--
Regards,

Torin Cooper-Bennun
Software Engineer | maxiluxsystems.com