c_can driver sometimes sends first two bytes filled with zeros

c_can driver sometimes sends first two bytes filled with zeros

[Richard Andrysek]

We can reproduce an issue with the canutils. We send messages in the loop with non-zero bytes and from time to time we get first two bytes of the message with zero values. The test script looks so:

#!/bin/sh

echo "Press [CTRL+C] to stop.."
while true
do
               cansend can1 --loop=15 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
done

With CAN analyzer we see normally the right message, but in cycles ~1min we see first two bytes are zero.

If we add some delays between messages, like this:

do
               cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
               usleep 5
	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
               usleep 5
	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
               usleep 5
	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
               usleep 5
	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
               usleep 5
	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
               usleep 5
	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
               usleep 5
	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
               usleep 5
	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
               usleep 5
	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
               usleep 5
	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
               usleep 5

done

It works fine.

We use Altera Cyclone V, where the  c_can driver is used. It runs with Linux kernel 3.16, but I've checked 4.5 version of a driver and it is a same one.

Have somebody idea how to find a reason for that? 

Richard

Re: c_can driver sometimes sends first two bytes filled with zeros

[Thor Thayer]

Hi Richard,

On 05/12/2016 04:23 AM, Richard Andrysek wrote:
> We can reproduce an issue with the canutils. We send messages in the loop with non-zero bytes and from time to time we get first two bytes of the message with zero values. The test script looks so:
>
> #!/bin/sh
>
> echo "Press [CTRL+C] to stop.."
> while true
> do
>                 cansend can1 --loop=15 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
> done
>
> With CAN analyzer we see normally the right message, but in cycles ~1min we see first two bytes are zero.
>
> If we add some delays between messages, like this:
>
> do
>                 cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
>
> done
>
> It works fine.
>
> We use Altera Cyclone V, where the  c_can driver is used. It runs with Linux kernel 3.16, but I've checked 4.5 version of a driver and it is a same one.
>
> Have somebody idea how to find a reason for that?
>
> Richard
>
How old are your canutils? I had a similar issue on an older version of 
cansend (4.0.6 from the Pengutronix site - with a copyright date of 2009).

There is an old thread (http://comments.gmane.org/gmane.linux.can/2339) 
that suggests adding a delay in the cansend utility to work around a 
poll/select bug.

I added a delay for 4.0.6 but you may want to grab the latest from 
github (https://github.com/linux-can/can-utils).

> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

AW: c_can driver sometimes sends first two bytes filled with zeros

[Richard Andrysek]

Hi Thor,

I've made two use cases with can0:

a)	RAW socket can with 200us steps
b)	8 CAN messages with 150us step in a broadcast mode

Both works perfect, only my PCAN USB has from time to time an overrun. I think I need another logger:)

But if I take case b) and reduce steps to 128us I get two frames with wrong two first bytes per 3min.
Similar behavior in case a).

Questions:
   Q1) What I still don't understand, if there is a failure I do not get any error value. Why that?
   Q2) Is there somewhere missing some kind of locking mechanism?

-----Ursprüngliche Nachricht-----
Von: Thor Thayer [mailto:tthayer@opensource.altera.com] 
Gesendet: Montag, 16. Mai 2016 20:15
An: Richard Andrysek <richard.andrysek@gomtec.de>; linux-can@vger.kernel.org
Betreff: Re: c_can driver sometimes sends first two bytes filled with zeros

Hi Richard,

On 05/12/2016 04:23 AM, Richard Andrysek wrote:
> We can reproduce an issue with the canutils. We send messages in the loop with non-zero bytes and from time to time we get first two bytes of the message with zero values. The test script looks so:
>
> #!/bin/sh
>
> echo "Press [CTRL+C] to stop.."
> while true
> do
>                 cansend can1 --loop=15 -i 933 0xde 0xde 0xde 0xde 0xde 
> 0xde done
>
> With CAN analyzer we see normally the right message, but in cycles ~1min we see first two bytes are zero.
>
> If we add some delays between messages, like this:
>
> do
>                 cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
>
> done
>
> It works fine.
>
> We use Altera Cyclone V, where the  c_can driver is used. It runs with Linux kernel 3.16, but I've checked 4.5 version of a driver and it is a same one.
>
> Have somebody idea how to find a reason for that?
>
> Richard
>
How old are your canutils? I had a similar issue on an older version of cansend (4.0.6 from the Pengutronix site - with a copyright date of 2009).

There is an old thread (http://comments.gmane.org/gmane.linux.can/2339)
that suggests adding a delay in the cansend utility to work around a poll/select bug.

I added a delay for 4.0.6 but you may want to grab the latest from github (https://github.com/linux-can/can-utils).

> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" 
> in the body of a message to majordomo@vger.kernel.org More majordomo 
> info at  http://vger.kernel.org/majordomo-info.html
>

Re: AW: c_can driver sometimes sends first two bytes filled with zeros

[Thor Thayer]

Hi Richard,

On 05/17/2016 12:18 PM, Richard Andrysek wrote:
> Hi Thor,
>
> I've made two use cases with can0:
>
> a)	RAW socket can with 200us steps
> b)	8 CAN messages with 150us step in a broadcast mode
>
> Both works perfect, only my PCAN USB has from time to time an overrun. I think I need another logger:)
>
> But if I take case b) and reduce steps to 128us I get two frames with wrong two first bytes per 3min.
> Similar behavior in case a).
>
I'd like to clarify. When you refer to steps, are you talking about usec 
delays between messages using the usleep function as described in your 
initial post?

And to clarify the setup: a) is a RAW socket (is this a custom user 
space program?) sending 1 message every 200us and b) uses the older 
4.0.6 cansend sending 1 message with 150us between each message repeated 
8 times? Or is b) sending 8 messages back-to-back (no usleep() in 
between messages) with 150us between the 8 messages?

And the problem is that when you reduce the delay between messages to 
128us, you start losing the first 2 bytes in 2 frames if you record for 
3 minutes.

> Questions:
>     Q1) What I still don't understand, if there is a failure I do not get any error value. Why that?
>     Q2) Is there somewhere missing some kind of locking mechanism?
>

> -----Ursprüngliche Nachricht-----
> Von: Thor Thayer [mailto:tthayer@opensource.altera.com]
> Gesendet: Montag, 16. Mai 2016 20:15
> An: Richard Andrysek <richard.andrysek@gomtec.de>; linux-can@vger.kernel.org
> Betreff: Re: c_can driver sometimes sends first two bytes filled with zeros
>
> Hi Richard,
>
> On 05/12/2016 04:23 AM, Richard Andrysek wrote:
>> We can reproduce an issue with the canutils. We send messages in the loop with non-zero bytes and from time to time we get first two bytes of the message with zero values. The test script looks so:
>>
>> #!/bin/sh
>>
>> echo "Press [CTRL+C] to stop.."
>> while true
>> do
>>                  cansend can1 --loop=15 -i 933 0xde 0xde 0xde 0xde 0xde
>> 0xde done
>>
>> With CAN analyzer we see normally the right message, but in cycles ~1min we see first two bytes are zero.
>>
>> If we add some delays between messages, like this:
>>
>> do
>>                  cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>>                  usleep 5
>> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>>                  usleep 5
>> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>>                  usleep 5
>> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>>                  usleep 5
>> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>>                  usleep 5
>> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>>                  usleep 5
>> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>>                  usleep 5
>> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>>                  usleep 5
>> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>>                  usleep 5
>> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>>                  usleep 5
>> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>>                  usleep 5
>>
>> done
>>
>> It works fine.
>>
>> We use Altera Cyclone V, where the  c_can driver is used. It runs with Linux kernel 3.16, but I've checked 4.5 version of a driver and it is a same one.
>>
>> Have somebody idea how to find a reason for that?
>>
>> Richard
>>
> How old are your canutils? I had a similar issue on an older version of cansend (4.0.6 from the Pengutronix site - with a copyright date of 2009).
>
> There is an old thread (http://comments.gmane.org/gmane.linux.can/2339)
> that suggests adding a delay in the cansend utility to work around a poll/select bug.
>
> I added a delay for 4.0.6 but you may want to grab the latest from github (https://github.com/linux-can/can-utils).
>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-can"
>> in the body of a message to majordomo@vger.kernel.org More majordomo
>> info at  http://vger.kernel.org/majordomo-info.html
>>

Re: AW: AW: c_can driver sometimes sends first two bytes filled with zeros

[Thor Thayer]

Hi Richard,

On 05/19/2016 09:07 AM, Richard Andrysek wrote:
> Hi Thor,
>
> I am sending my test program and a makefile. The application accept up to 2 arguments:
> ## send 8 messages in 2ms cycle
> # test  8 2000
>
> ## default 16 messages in 2.6ms (the zip file data)
> # test
>
> In the zip file you can see my logging files (channel 1 and channel 2), on the channel 2 you can find failures
> in the "Failure_dump.txt".
>
Yes, I can see the zero bytes in your data.

Am I correct that both can0 and can1 are receiving the same data? If I'm 
understanding, can0 seems to be fine but can1 periodically shows 
corruption with the first 2 bytes set to 0?

Since the attached program uses a write, I'm confused about who is 
sending the data? Is that the PCAN USB you referred to in an earlier email?

Also, where is the 2.6ms that you refer to and is in the code? From the 
data, it seems like the spacing between frames is ~.120ms and ~1msec 
between 16 frame bursts.

> Initialization of can channels is done from the shell:
>
> # ip link set can0 up type can bitrate 1000000
> # ip link set can1 up type can bitrate 1000000
>
> The cpu load is max 5%.
>
> Question1:
> I've checked a driver it looks good. Except one line in the function :
>
> static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
> 				    struct net_device *dev)
> {
> 	struct can_frame *frame = (struct can_frame *)skb->data;
> 	struct c_can_priv *priv = netdev_priv(dev);
> 	u32 idx, obj;
>
> 	if (can_dropped_invalid_skb(dev, skb))
> 		return NETDEV_TX_OK;
> 	/*
> 	 * This is not a FIFO. C/D_CAN sends out the buffers
> 	 * prioritized. The lowest buffer number wins.
> 	 */
> 	idx = fls(atomic_read(&priv->tx_active));  /*!!!!! Why so !!!! */
> 	...
> 	...
> 	/* Update the active bits */
> 	atomic_add((1 << idx), &priv->tx_active);
>
> Are these atomic operations correct? atomic_add I understand, but shall not be atomic_read used like this:
>
>               Atomic block start
>               idx = fls(read(&priv->tx->active))
>               remove_idx_from(&priv->tx->active, idx)
>               Atomic block stop	
>
I'm not sure about this. I'm including the maintainers in the CC.

> Question2: What can make corrupted CAN messages on the channel 2?
>
Hmm. I've only worked with the first CAN so I can't speak for the 2nd 
can device.

> Ch.
>
> Richard
>
>

AW: AW: AW: c_can driver sometimes sends first two bytes filled with zeros

[Richard Andrysek]

Thanks for informing maintainers. If needed we can Skype or phone or ... 

Gateway devices can0 and can1 send CAN messages. I have new logger "PCAN-USB-Pro"(I've tested also with a CANCaseXL) with two channels which only receives messages. One logger's channel is connected with can0 and one with can1. On one channel I do not see any failure (healthy channel), on the second one from time to time (bad channel).

The physical transmission of one CAN message on the bus takes 108+bit stuffing time us = ~118us. As you can see in the c-code, in the while-loop, first are written 16 messages ("burst") into each peripherals (can0,can1) and after that it waits 2600us for a next cycle. The peripheral itself has message buffers (small extra RAM), in the Linux driver are programmed 16 buffers for TX. So I can write 16 messages into buffers and then give enough time to be physically transmitted on the bus - 2600us (variable "delayTime_us".   

Real one cycle takes 2822 because:  "time of write calls" + "delayTime_us" => it seems that "time of 32 write calls" is roughly ~222us.

One additional test: if I physically disconnect a "healthy channel", which means physically it does not send any message, the "bad channel" still have a problem.

Q: What I see in the code that there is writing into peripheral in 16bit steps, could it be related with that?
	File: "c_can.c"
	Function : c_can_setup_tx_object
	...	
	for (i = 0; i < frame->can_dlc; i += 2) {
		priv->write_reg(priv, C_CAN_IFACE(DATA1_REG, iface) + i / 2,
				frame->data[i] | (frame->data[i + 1] << 8));
	} 
	...
   Q2: Is it safe to use u8 like this: "(frame->data[i + 1] << 8)"?
 
Ch.

Richard

-----Ursprüngliche Nachricht-----
Von: Thor Thayer [mailto:tthayer@opensource.altera.com] 
Gesendet: Freitag, 20. Mai 2016 01:01
An: Richard Andrysek <richard.andrysek@gomtec.de>; linux-can@vger.kernel.org
Cc: wg@grandegger.com; mkl@pengutronix.de
Betreff: Re: AW: AW: c_can driver sometimes sends first two bytes filled with zeros

Hi Richard,

On 05/19/2016 09:07 AM, Richard Andrysek wrote:
> Hi Thor,
>
> I am sending my test program and a makefile. The application accept up to 2 arguments:
> ## send 8 messages in 2ms cycle
> # test  8 2000
>
> ## default 16 messages in 2.6ms (the zip file data) # test
>
> In the zip file you can see my logging files (channel 1 and channel 
> 2), on the channel 2 you can find failures in the "Failure_dump.txt".
>
Yes, I can see the zero bytes in your data.

Am I correct that both can0 and can1 are receiving the same data? If I'm understanding, can0 seems to be fine but can1 periodically shows corruption with the first 2 bytes set to 0?

Since the attached program uses a write, I'm confused about who is sending the data? Is that the PCAN USB you referred to in an earlier email?

Also, where is the 2.6ms that you refer to and is in the code? From the data, it seems like the spacing between frames is ~.120ms and ~1msec between 16 frame bursts.

> Initialization of can channels is done from the shell:
>
> # ip link set can0 up type can bitrate 1000000 # ip link set can1 up 
> type can bitrate 1000000
>
> The cpu load is max 5%.
>
> Question1:
> I've checked a driver it looks good. Except one line in the function :
>
> static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
> 				    struct net_device *dev)
> {
> 	struct can_frame *frame = (struct can_frame *)skb->data;
> 	struct c_can_priv *priv = netdev_priv(dev);
> 	u32 idx, obj;
>
> 	if (can_dropped_invalid_skb(dev, skb))
> 		return NETDEV_TX_OK;
> 	/*
> 	 * This is not a FIFO. C/D_CAN sends out the buffers
> 	 * prioritized. The lowest buffer number wins.
> 	 */
> 	idx = fls(atomic_read(&priv->tx_active));  /*!!!!! Why so !!!! */
> 	...
> 	...
> 	/* Update the active bits */
> 	atomic_add((1 << idx), &priv->tx_active);
>
> Are these atomic operations correct? atomic_add I understand, but shall not be atomic_read used like this:
>
>               Atomic block start
>               idx = fls(read(&priv->tx->active))
>               remove_idx_from(&priv->tx->active, idx)
>               Atomic block stop	
>
I'm not sure about this. I'm including the maintainers in the CC.

> Question2: What can make corrupted CAN messages on the channel 2?
>
Hmm. I've only worked with the first CAN so I can't speak for the 2nd can device.

> Ch.
>
> Richard
>
>

Re: AW: AW: AW: c_can driver sometimes sends first two bytes filled with zeros

[Thor Thayer]

Thanks for the explanation of your setup.

On 05/20/2016 07:01 AM, Richard Andrysek wrote:
> Thanks for informing maintainers. If needed we can Skype or phone or ...
>
> Gateway devices can0 and can1 send CAN messages. I have new logger "PCAN-USB-Pro"(I've tested also with a CANCaseXL) with two channels which only receives messages. One logger's channel is connected with can0 and one with can1. On one channel I do not see any failure (healthy channel), on the second one from time to time (bad channel).
>
> The physical transmission of one CAN message on the bus takes 108+bit stuffing time us = ~118us. As you can see in the c-code, in the while-loop, first are written 16 messages ("burst") into each peripherals (can0,can1) and after that it waits 2600us for a next cycle. The peripheral itself has message buffers (small extra RAM), in the Linux driver are programmed 16 buffers for TX. So I can write 16 messages into buffers and then give enough time to be physically transmitted on the bus - 2600us (variable "delayTime_us".
>
> Real one cycle takes 2822 because:  "time of write calls" + "delayTime_us" => it seems that "time of 32 write calls" is roughly ~222us.
>
> One additional test: if I physically disconnect a "healthy channel", which means physically it does not send any message, the "bad channel" still have a problem.
>
That is interesting.

> Q: What I see in the code that there is writing into peripheral in 16bit steps, could it be related with that?
> 	File: "c_can.c"
> 	Function : c_can_setup_tx_object
> 	...	
> 	for (i = 0; i < frame->can_dlc; i += 2) {
> 		priv->write_reg(priv, C_CAN_IFACE(DATA1_REG, iface) + i / 2,
> 				frame->data[i] | (frame->data[i + 1] << 8));
> 	}
> 	...
>     Q2: Is it safe to use u8 like this: "(frame->data[i + 1] << 8)"?
>
Good point. According to the C spec, if an int can represent all values 
of the original type, the value is converted to an int.

> Ch.
>
> Richard
>
> -----Ursprüngliche Nachricht-----
> Von: Thor Thayer [mailto:tthayer@opensource.altera.com]
> Gesendet: Freitag, 20. Mai 2016 01:01
> An: Richard Andrysek <richard.andrysek@gomtec.de>; linux-can@vger.kernel.org
> Cc: wg@grandegger.com; mkl@pengutronix.de
> Betreff: Re: AW: AW: c_can driver sometimes sends first two bytes filled with zeros
>
> Hi Richard,
>
> On 05/19/2016 09:07 AM, Richard Andrysek wrote:
>> Hi Thor,
>>
>> I am sending my test program and a makefile. The application accept up to 2 arguments:
>> ## send 8 messages in 2ms cycle
>> # test  8 2000
>>
>> ## default 16 messages in 2.6ms (the zip file data) # test
>>
>> In the zip file you can see my logging files (channel 1 and channel
>> 2), on the channel 2 you can find failures in the "Failure_dump.txt".
>>
> Yes, I can see the zero bytes in your data.
>
> Am I correct that both can0 and can1 are receiving the same data? If I'm understanding, can0 seems to be fine but can1 periodically shows corruption with the first 2 bytes set to 0?
>
> Since the attached program uses a write, I'm confused about who is sending the data? Is that the PCAN USB you referred to in an earlier email?
>
> Also, where is the 2.6ms that you refer to and is in the code? From the data, it seems like the spacing between frames is ~.120ms and ~1msec between 16 frame bursts.
>
>> Initialization of can channels is done from the shell:
>>
>> # ip link set can0 up type can bitrate 1000000 # ip link set can1 up
>> type can bitrate 1000000
>>
>> The cpu load is max 5%.
>>
>> Question1:
>> I've checked a driver it looks good. Except one line in the function :
>>
>> static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
>> 				    struct net_device *dev)
>> {
>> 	struct can_frame *frame = (struct can_frame *)skb->data;
>> 	struct c_can_priv *priv = netdev_priv(dev);
>> 	u32 idx, obj;
>>
>> 	if (can_dropped_invalid_skb(dev, skb))
>> 		return NETDEV_TX_OK;
>> 	/*
>> 	 * This is not a FIFO. C/D_CAN sends out the buffers
>> 	 * prioritized. The lowest buffer number wins.
>> 	 */
>> 	idx = fls(atomic_read(&priv->tx_active));  /*!!!!! Why so !!!! */
>> 	...
>> 	...
>> 	/* Update the active bits */
>> 	atomic_add((1 << idx), &priv->tx_active);
>>
>> Are these atomic operations correct? atomic_add I understand, but shall not be atomic_read used like this:
>>
>>                Atomic block start
>>                idx = fls(read(&priv->tx->active))
>>                remove_idx_from(&priv->tx->active, idx)
>>                Atomic block stop	
>>
> I'm not sure about this. I'm including the maintainers in the CC.
>
>> Question2: What can make corrupted CAN messages on the channel 2?
>>
> Hmm. I've only worked with the first CAN so I can't speak for the 2nd can device.
>
>> Ch.
>>
>> Richard
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: c_can driver sometimes sends first two bytes filled with zeros

[Wolfgang Grandegger]

Hello,

Am 12.05.2016 um 11:23 schrieb Richard Andrysek:
> We can reproduce an issue with the canutils. We send messages in the loop with non-zero bytes and from time to time we get first two bytes of the message with zero values. The test script looks so:
>
> #!/bin/sh
>
> echo "Press [CTRL+C] to stop.."
> while true
> do
>                 cansend can1 --loop=15 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
> done
>
> With CAN analyzer we see normally the right message, but in cycles ~1min we see first two bytes are zero.
>
> If we add some delays between messages, like this:
>
> do
>                 cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
>
> done
>
> It works fine.
>
> We use Altera Cyclone V, where the  c_can driver is used. It runs with Linux kernel 3.16, but I've checked 4.5 version of a driver and it is a same one.
>
> Have somebody idea how to find a reason for that?

The official "cansend" does not support "--loop". The official canutils
have cangen and canfdtest for more thorough testing. Anyway, does
"ip -s -d link show" report any errors? And could you run
"candump any,0:0,#FFFFFFFF" while sending. Does it also list the 
messages with the wrong data?

Wolfgang.

AW: c_can driver sometimes sends first two bytes filled with zeros

[Richard Andrysek]

Hi Wolfgang,

Sorry for one week break. Good idea with dumps, see answers below.

>> The official "cansend" does not support "--loop".
I've made the extra test program, please see my email from " 20 May 14:01 2016	Richard Andrysek".
With this test I've made following steps.

>>ip  -s -d link show
After 1 hour I've called this command
# ip -s -d link show
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN mode DEFAULT group default
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00 promiscuity 0
    RX: bytes  packets  errors  dropped overrun mcast
    0          0        0       0       0       0
    TX: bytes  packets  errors  dropped carrier collsns
    0          0        0       0       0       0
2: can0: <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 1000000 sample-point 0.750
          tq 50 prop-seg 7 phase-seg1 7 phase-seg2 5 sjw 1
          c_can: tseg1 2..16 tseg2 1..8 sjw 1..4 brp 1..1024 brp-inc 1
          clock 100000000
          re-started bus-errors arbit-lost error-warn error-pass bus-off
          0          0          0          0          0          0
    RX: bytes  packets  errors  dropped overrun mcast
    0          0        0       0       0       0
    TX: bytes  packets  errors  dropped carrier collsns
    85337216   10667152 0       0       0       0
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 1000000 sample-point 0.750
          tq 50 prop-seg 7 phase-seg1 7 phase-seg2 5 sjw 1
          c_can: tseg1 2..16 tseg2 1..8 sjw 1..4 brp 1..1024 brp-inc 1
          clock 100000000
          re-started bus-errors arbit-lost error-warn error-pass bus-off
          0          0          0          0          0          0
    RX: bytes  packets  errors  dropped overrun mcast
    0          0        0       0       0       0
    TX: bytes  packets  errors  dropped carrier collsns
    85337216   10667152 0       0       0       0

>> run "candump any,0:0,#FFFFFFFF"
I can run following commands, I let to run for 5min, 10 min, and more:
  # candump can0 | grep -v "01 02 03 04"
  interface = can0, family = 29, type = 3, proto = 1
   Or
 # candump can1 | grep -v "01 02 03 04"
 interface = can1, family = 29, type = 3, proto = 1 

Nothing except a first line, which is OK.

Summary:
    If I understand it correctly socketcan is OK. On the bus I have logging without any error, so from Altera to PEAK it is also OK. It is somewhere between "socketcan layer" and peripheral out buffer. What can I do now? E.g. shall I try to make the  atomic call over " priv->write_reg(...)", if yes how?

CH.

Richard
    

-----Ursprüngliche Nachricht-----
Von: Wolfgang Grandegger [mailto:wg@grandegger.com] 
Gesendet: Montag, 23. Mai 2016 20:20
An: Richard Andrysek <richard.andrysek@gomtec.de>; linux-can@vger.kernel.org
Betreff: Re: c_can driver sometimes sends first two bytes filled with zeros

Hello,

Am 12.05.2016 um 11:23 schrieb Richard Andrysek:
> We can reproduce an issue with the canutils. We send messages in the loop with non-zero bytes and from time to time we get first two bytes of the message with zero values. The test script looks so:
>
> #!/bin/sh
>
> echo "Press [CTRL+C] to stop.."
> while true
> do
>                 cansend can1 --loop=15 -i 933 0xde 0xde 0xde 0xde 0xde 
> 0xde done
>
> With CAN analyzer we see normally the right message, but in cycles ~1min we see first two bytes are zero.
>
> If we add some delays between messages, like this:
>
> do
>                 cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>                 usleep 5
>
> done
>
> It works fine.
>
> We use Altera Cyclone V, where the  c_can driver is used. It runs with Linux kernel 3.16, but I've checked 4.5 version of a driver and it is a same one.
>
> Have somebody idea how to find a reason for that?

The official "cansend" does not support "--loop". The official canutils have cangen and canfdtest for more thorough testing. Anyway, does "ip -s -d link show" report any errors? And could you run "candump any,0:0,#FFFFFFFF" while sending. Does it also list the messages with the wrong data?

Wolfgang.

Re: c_can driver sometimes sends first two bytes filled with zeros

[Wolfgang Grandegger]

Hello Richard,

Am 01.06.2016 um 11:40 schrieb Richard Andrysek:
> Hi Wolfgang,
>
> Sorry for one week break. Good idea with dumps, see answers below.
>
>>> The official "cansend" does not support "--loop".
> I've made the extra test program, please see my email from " 20 May 14:01 2016	Richard Andrysek".
> With this test I've made following steps.
>
>>> ip  -s -d link show
> After 1 hour I've called this command
> # ip -s -d link show
> 1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN mode DEFAULT group default
>      link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00 promiscuity 0
>      RX: bytes  packets  errors  dropped overrun mcast
>      0          0        0       0       0       0
>      TX: bytes  packets  errors  dropped carrier collsns
>      0          0        0       0       0       0
> 2: can0: <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 1000000 sample-point 0.750

>            tq 50 prop-seg 7 phase-seg1 7 phase-seg2 5 sjw 1
>            c_can: tseg1 2..16 tseg2 1..8 sjw 1..4 brp 1..1024 brp-inc 1
>            clock 100000000
>            re-started bus-errors arbit-lost error-warn error-pass bus-off
>            0          0          0          0          0          0
>      RX: bytes  packets  errors  dropped overrun mcast
>      0          0        0       0       0       0
>      TX: bytes  packets  errors  dropped carrier collsns
>      85337216   10667152 0       0       0       0
> 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 1000000 sample-point 0.750
>            tq 50 prop-seg 7 phase-seg1 7 phase-seg2 5 sjw 1
>            c_can: tseg1 2..16 tseg2 1..8 sjw 1..4 brp 1..1024 brp-inc 1
>            clock 100000000
>            re-started bus-errors arbit-lost error-warn error-pass bus-off
>            0          0          0          0          0          0
>      RX: bytes  packets  errors  dropped overrun mcast
>      0          0        0       0       0       0
>      TX: bytes  packets  errors  dropped carrier collsns
>      85337216   10667152 0       0       0       0

OK, no errors reported.

>>> run "candump any,0:0,#FFFFFFFF"
> I can run following commands, I let to run for 5min, 10 min, and more:
>    # candump can0 | grep -v "01 02 03 04"
>    interface = can0, family = 29, type = 3, proto = 1
>     Or
>   # candump can1 | grep -v "01 02 03 04"
>   interface = can1, family = 29, type = 3, proto = 1
>
> Nothing except a first line, which is OK.

Ok, anyway, please use the official CAN utilities [1] sooner than later.


> Summary:
>      If I understand it correctly socketcan is OK. On the bus I have logging without any error, so from Altera to PEAK it is also OK. It is somewhere between "socketcan layer" and peripheral out buffer. What can I do now? E.g. shall I try to make the  atomic call over " priv->write_reg(...)", if yes how?

Making "priv->write_regs() atomic is something I would try as well. My 
suspicion is that something goes wrong writing(/reading) the controller 
registers.

[1] https://github.com/linux-can/can-utils

Wolfgang.

> CH.
>
> Richard
>
>
> -----Ursprüngliche Nachricht-----
> Von: Wolfgang Grandegger [mailto:wg@grandegger.com]
> Gesendet: Montag, 23. Mai 2016 20:20
> An: Richard Andrysek <richard.andrysek@gomtec.de>; linux-can@vger.kernel.org
> Betreff: Re: c_can driver sometimes sends first two bytes filled with zeros
>
> Hello,
>
> Am 12.05.2016 um 11:23 schrieb Richard Andrysek:
>> We can reproduce an issue with the canutils. We send messages in the loop with non-zero bytes and from time to time we get first two bytes of the message with zero values. The test script looks so:
>>
>> #!/bin/sh
>>
>> echo "Press [CTRL+C] to stop.."
>> while true
>> do
>>                  cansend can1 --loop=15 -i 933 0xde 0xde 0xde 0xde 0xde
>> 0xde done
>>
>> With CAN analyzer we see normally the right message, but in cycles ~1min we see first two bytes are zero.
>>
>> If we add some delays between messages, like this:
>>
>> do
>>                  cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>>                  usleep 5
>> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>>                  usleep 5
>> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>>                  usleep 5
>> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>>                  usleep 5
>> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>>                  usleep 5
>> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>>                  usleep 5
>> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>>                  usleep 5
>> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>>                  usleep 5
>> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>>                  usleep 5
>> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>>                  usleep 5
>> 	cansend can1 --loop=1 -i 933 0xde 0xde 0xde 0xde 0xde 0xde
>>                  usleep 5
>>
>> done
>>
>> It works fine.
>>
>> We use Altera Cyclone V, where the  c_can driver is used. It runs with Linux kernel 3.16, but I've checked 4.5 version of a driver and it is a same one.
>>
>> Have somebody idea how to find a reason for that?
>
> The official "cansend" does not support "--loop". The official canutils have cangen and canfdtest for more thorough testing. Anyway, does "ip -s -d link show" report any errors? And could you run "candump any,0:0,#FFFFFFFF" while sending. Does it also list the messages with the wrong data?
>
> Wolfgang.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

Re: c_can driver sometimes sends first two bytes filled with zeros

[Andy Haydon]

Hi Wolfgang,

Wolfgang Grandegger <wg <at> grandegger.com> writes:

> 
> Making "priv->write_regs() atomic is something I would try as well. My 
> suspicion is that something goes wrong writing(/reading) the controller 
> registers.

We have also been looking at a similar issue over the last few days. In our 
case, bytes 0 and 1 and bytes 4 and 5 of an 8 byte message can be corrupted 
randomly - not necessarily zeroed. Bytes 2 and 3 and bytes 6 and 7 are never
corrupted. The Tx task is running as a high priority real time thread in 
PREEMPT_RT.

I've found that so far the only thing that fixes this issue is to perform 
the writes to the data registers as 32 bit operations something like this 
(for 3.18.20):

--- a/drivers/net/can/c_can/c_can.c	2015-08-07 20:08:04.000000000 +0100
+++ b/drivers/net/can/c_can/c_can.c	2016-06-09 14:46:51.497895357 +0100
@@ -331,9 +332,19 @@
 
 	priv->write_reg(priv, C_CAN_IFACE(MSGCTRL_REG, iface), ctrl);
 
-	for (i = 0; i < frame->can_dlc; i += 2) {
-		priv->write_reg(priv, C_CAN_IFACE(DATA1_REG, iface) + i / 2,
-				frame->data[i] | (frame->data[i + 1] << 8));
+// this doesn't work for Cyclone V
+//	for (i = 0; i < frame->can_dlc; i += 2) {
+//		priv->write_reg(priv, C_CAN_IFACE(DATA1_REG, iface) + i / 2,
+//				frame->data[i] | (frame->data[i + 1] << 8));
+
+// replaced with this as a workaround
+	u32 data = 0;
+	for (i = 0; i < frame->can_dlc; i += 4) {
+		data = (u32)frame->data[i];
+		data |= (u32)frame->data[i + 1] << 8;
+		data |= (u32)frame->data[i + 2] << 16;
+		data |= (u32)frame->data[i + 3] << 24;
+		priv->write_reg32(priv, C_CAN_IFACE(DATA1_REG, iface) + i / 
2, data);
 	}
 }
 
At the moment I can't explain this behaviour, or if this is really the only 
way to fix the issue for the Cyclone V. 
The tests that I've done so far seem to indicate that the data is correct 
in the driver right up to the point at which it is actually written to the 
CAN peripheral registers, so I think that your suspicion is correct.

Regards,
Andy

Re: c_can driver sometimes sends first two bytes filled with zeros

[Wolfgang Grandegger]

Hello Andy,

Am 10.06.2016 um 12:49 schrieb Andy Haydon:
> Hi Wolfgang,
>
> Wolfgang Grandegger <wg <at> grandegger.com> writes:
>
>>
>> Making "priv->write_regs() atomic is something I would try as well. My
>> suspicion is that something goes wrong writing(/reading) the controller
>> registers.
>
> We have also been looking at a similar issue over the last few days. In our
> case, bytes 0 and 1 and bytes 4 and 5 of an 8 byte message can be corrupted
> randomly - not necessarily zeroed. Bytes 2 and 3 and bytes 6 and 7 are never
> corrupted. The Tx task is running as a high priority real time thread in
> PREEMPT_RT.
>
> I've found that so far the only thing that fixes this issue is to perform
> the writes to the data registers as 32 bit operations something like this
> (for 3.18.20):
>
> --- a/drivers/net/can/c_can/c_can.c	2015-08-07 20:08:04.000000000 +0100
> +++ b/drivers/net/can/c_can/c_can.c	2016-06-09 14:46:51.497895357 +0100
> @@ -331,9 +332,19 @@
>
>   	priv->write_reg(priv, C_CAN_IFACE(MSGCTRL_REG, iface), ctrl);
>
> -	for (i = 0; i < frame->can_dlc; i += 2) {
> -		priv->write_reg(priv, C_CAN_IFACE(DATA1_REG, iface) + i / 2,
> -				frame->data[i] | (frame->data[i + 1] << 8));
> +// this doesn't work for Cyclone V
> +//	for (i = 0; i < frame->can_dlc; i += 2) {
> +//		priv->write_reg(priv, C_CAN_IFACE(DATA1_REG, iface) + i / 2,
> +//				frame->data[i] | (frame->data[i + 1] << 8));
> +
> +// replaced with this as a workaround
> +	u32 data = 0;
> +	for (i = 0; i < frame->can_dlc; i += 4) {
> +		data = (u32)frame->data[i];
> +		data |= (u32)frame->data[i + 1] << 8;
> +		data |= (u32)frame->data[i + 2] << 16;
> +		data |= (u32)frame->data[i + 3] << 24;
> +		priv->write_reg32(priv, C_CAN_IFACE(DATA1_REG, iface) + i /
> 2, data);
>   	}
>   }

Interesting observation! Richard (on CC now), does the patch also fix 
your similar issues?

> At the moment I can't explain this behaviour, or if this is really the only
> way to fix the issue for the Cyclone V.

Maybe the hardware does not like the 16-bit accesses. BTW: How is the 
device configured? C_CAN with 32-bit accesses or D_CAN? Anyway, real 
32-bit accesses would be faster anyway.

> The tests that I've done so far seem to indicate that the data is correct
> in the driver right up to the point at which it is actually written to the
> CAN peripheral registers, so I think that your suspicion is correct.

Maybe some hardware guys from Altera could help.

Wolfgang.

Re: c_can driver sometimes sends first two bytes filled with zeros

[Andy Haydon]

Hi Wolfgang,

Wolfgang Grandegger <wg <at> grandegger.com> writes:

> 
> Hello Andy,
> 
> Am 10.06.2016 um 12:49 schrieb Andy Haydon:
> > Hi Wolfgang,
> >
> > Wolfgang Grandegger <wg <at> grandegger.com> writes:
> >
> >>
> >> Making "priv->write_regs() atomic is something I would try as well. My
> >> suspicion is that something goes wrong writing(/reading) the controller
> >> registers.
> >
> > We have also been looking at a similar issue over the last few days. In 
our
> > case, bytes 0 and 1 and bytes 4 and 5 of an 8 byte message can be 
corrupted
> > randomly - not necessarily zeroed. Bytes 2 and 3 and bytes 6 and 7 are 
never
> > corrupted. The Tx task is running as a high priority real time thread in
> > PREEMPT_RT.
> >
> > I've found that so far the only thing that fixes this issue is to 
perform
> > the writes to the data registers as 32 bit operations something like 
this
> > (for 3.18.20):
> >
> > --- a/drivers/net/can/c_can/c_can.c	2015-08-07 20:08:04.000000000 +0100
> > +++ b/drivers/net/can/c_can/c_can.c	2016-06-09 14:46:51.497895357 +0100
> >  <at>  <at>  -331,9 +332,19  <at>  <at> 
> >
> >   	priv->write_reg(priv, C_CAN_IFACE(MSGCTRL_REG, iface), ctrl);
> >
> > -	for (i = 0; i < frame->can_dlc; i += 2) {
> > -		priv->write_reg(priv, C_CAN_IFACE(DATA1_REG, iface) + i / 2,
> > -				frame->data[i] | (frame->data[i + 1] << 8));
> > +// this doesn't work for Cyclone V
> > +//	for (i = 0; i < frame->can_dlc; i += 2) {
> > +//		priv->write_reg(priv, C_CAN_IFACE(DATA1_REG, iface) + i / 2,
> > +//				frame->data[i] | (frame->data[i + 1] << 8));
> > +
> > +// replaced with this as a workaround
> > +	u32 data = 0;
> > +	for (i = 0; i < frame->can_dlc; i += 4) {
> > +		data = (u32)frame->data[i];
> > +		data |= (u32)frame->data[i + 1] << 8;
> > +		data |= (u32)frame->data[i + 2] << 16;
> > +		data |= (u32)frame->data[i + 3] << 24;
> > +		priv->write_reg32(priv, C_CAN_IFACE(DATA1_REG, iface) + i /
> > 2, data);
> >   	}
> >   }
> 
> Interesting observation! Richard (on CC now), does the patch also fix 
> your similar issues?
> 
> > At the moment I can't explain this behaviour, or if this is really the 
only
> > way to fix the issue for the Cyclone V.
> 
> Maybe the hardware does not like the 16-bit accesses. BTW: How is the 
> device configured? C_CAN with 32-bit accesses or D_CAN? Anyway, real 
> 32-bit accesses would be faster anyway.

The peripheral is D_CAN with 32-bit registers. 
There are many other 16-bit accesses in the driver, but it is only the data 
registers that seem not to like them. And the read from the data registers 
works fine as 16-bit accesses.
I agree that 32-bit accesses would be faster in this case. If we were to 
make a kernel patch, would the correct way to do it be to add a 32-bit 
access property to the d_can device tree binding?

> 
> > The tests that I've done so far seem to indicate that the data is 
correct
> > in the driver right up to the point at which it is actually written to 
the
> > CAN peripheral registers, so I think that your suspicion is correct.
> 
> Maybe some hardware guys from Altera could help.

I've created a service request with Altera. Hopefully I can eventually get 
this investigated by their hardware guys.

Regards,
Andy

Re: c_can driver sometimes sends first two bytes filled with zeros

[Wolfgang Grandegger]

Am 10.06.2016 um 15:12 schrieb Andy Haydon:
> Hi Wolfgang,
>
> Wolfgang Grandegger <wg <at> grandegger.com> writes:
>
>>
>> Hello Andy,
>>
>> Am 10.06.2016 um 12:49 schrieb Andy Haydon:
>>> Hi Wolfgang,
>>>
>>> Wolfgang Grandegger <wg <at> grandegger.com> writes:
>>>
>>>>
>>>> Making "priv->write_regs() atomic is something I would try as well. My
>>>> suspicion is that something goes wrong writing(/reading) the controller
>>>> registers.
>>>
>>> We have also been looking at a similar issue over the last few days. In
> our
>>> case, bytes 0 and 1 and bytes 4 and 5 of an 8 byte message can be
> corrupted
>>> randomly - not necessarily zeroed. Bytes 2 and 3 and bytes 6 and 7 are
> never
>>> corrupted. The Tx task is running as a high priority real time thread in
>>> PREEMPT_RT.
>>>
>>> I've found that so far the only thing that fixes this issue is to
> perform
>>> the writes to the data registers as 32 bit operations something like
> this
>>> (for 3.18.20):
>>>
>>> --- a/drivers/net/can/c_can/c_can.c	2015-08-07 20:08:04.000000000 +0100
>>> +++ b/drivers/net/can/c_can/c_can.c	2016-06-09 14:46:51.497895357 +0100
>>>   <at>  <at>  -331,9 +332,19  <at>  <at>
>>>
>>>    	priv->write_reg(priv, C_CAN_IFACE(MSGCTRL_REG, iface), ctrl);
>>>
>>> -	for (i = 0; i < frame->can_dlc; i += 2) {
>>> -		priv->write_reg(priv, C_CAN_IFACE(DATA1_REG, iface) + i / 2,
>>> -				frame->data[i] | (frame->data[i + 1] << 8));
>>> +// this doesn't work for Cyclone V
>>> +//	for (i = 0; i < frame->can_dlc; i += 2) {
>>> +//		priv->write_reg(priv, C_CAN_IFACE(DATA1_REG, iface) + i / 2,
>>> +//				frame->data[i] | (frame->data[i + 1] << 8));
>>> +
>>> +// replaced with this as a workaround
>>> +	u32 data = 0;
>>> +	for (i = 0; i < frame->can_dlc; i += 4) {
>>> +		data = (u32)frame->data[i];
>>> +		data |= (u32)frame->data[i + 1] << 8;
>>> +		data |= (u32)frame->data[i + 2] << 16;
>>> +		data |= (u32)frame->data[i + 3] << 24;
>>> +		priv->write_reg32(priv, C_CAN_IFACE(DATA1_REG, iface) + i /
>>> 2, data);
>>>    	}
>>>    }
>>
>> Interesting observation! Richard (on CC now), does the patch also fix
>> your similar issues?
>>
>>> At the moment I can't explain this behaviour, or if this is really the
> only
>>> way to fix the issue for the Cyclone V.
>>
>> Maybe the hardware does not like the 16-bit accesses. BTW: How is the
>> device configured? C_CAN with 32-bit accesses or D_CAN? Anyway, real
>> 32-bit accesses would be faster anyway.
>
> The peripheral is D_CAN with 32-bit registers.
> There are many other 16-bit accesses in the driver, but it is only the data
> registers that seem not to like them. And the read from the data registers
> works fine as 16-bit accesses.
> I agree that 32-bit accesses would be faster in this case. If we were to
> make a kernel patch, would the correct way to do it be to add a 32-bit
> access property to the d_can device tree binding?

My understanding is that D_CAN can perform 32bit accesses while the 
C_CAN always does 16bit accesses. Therefore I would use 
[read|write]_reg32 "if (priv->type != BOSCH_D_CAN)" using code similar to:

   http://lxr.free-electrons.com/source/drivers/net/can/flexcan.c#L492

Wolfgang.