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

[Wolfgang Grandegger <wg@grandegger.com>]

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.