Message-ID: <54589AC8.4010106@pengutronix.de>
Date:	Tue, 04 Nov 2014 10:22:16 +0100
From:	Marc Kleine-Budde <mkl@...gutronix.de>
To:	Dong Aisheng <b29396@...escale.com>
CC:	linux-can@...r.kernel.org, wg@...ndegger.com,
	varkabhadram@...il.com, netdev@...r.kernel.org,
	socketcan@...tkopp.net, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 7/7] can: m_can: workaround for transmit data less than
 4 bytes

On 11/04/2014 09:25 AM, Dong Aisheng wrote:
>>> We meet an IC issue that we have to write the full 8 bytes (whatever
>>> value for the second word) in Message RAM to avoid bit error for transmit
>>> data less than 4 bytes.
>>
>> Is this a SoC or a m_can problem? Are all versions of the SoC/m_can
>> affected? Is there a m_can version register somewhere?

> I'm still not sure it's SoC or m_can problem.
> Our IC guys ran the simulation code and found this issue.
> But due to some reasons, it may be very slow for they to investigate
> and get the conclusion.

Let's hope they will find the root cause of this problem.

>>> Without the workaround, we can easily see the following errors:
>>> root@...6qdlsolo:~# ip link set can0 up type can bitrate 1000000
>>> [   66.882520] IPv6: ADDRCONF(NETDEV_CHANGE): can0: link becomes ready
>>> root@...6qdlsolo:~# cansend can0 123#112233
>>> [   66.935640] m_can 20e8000.can can0: Bit Error Uncorrected
>>>
>>> Signed-off-by: Dong Aisheng <b29396@...escale.com>
>>> ---
>>>  drivers/net/can/m_can/m_can.c | 11 ++++++++++-
>>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
>>> index 219e0e3..f2d9ebe 100644
>>> --- a/drivers/net/can/m_can/m_can.c
>>> +++ b/drivers/net/can/m_can/m_can.c
>>> @@ -1058,10 +1058,19 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb,
>>>  	m_can_fifo_write(priv, 0, M_CAN_FIFO_ID, id);
>>>  	m_can_fifo_write(priv, 0, M_CAN_FIFO_DLC, can_len2dlc(cf->len) << 16);
>>>  
>>> -	for (i = 0; i < cf->len; i += 4)
>>> +	for (i = 0; i < cf->len; i += 4) {
>>>  		m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(i / 4),
>>>  				 *(u32 *)(cf->data + i));
>>>  
>>> +		/* FIXME: we meet an IC issue that we have to write the full 8
>>
>> FIXME usually indicates that the driver needs some work here. Just
>> describe your hardware bug, you might add a reference to an errata if
>> available, though.
>
> We don't have an errata for it now.
> Because i'm not sure this is the final workaround and also not sure if other
> SoC vendors having the same issue, so i used FIXME here firstly.
> Since the code is harmless, so i wish we could put it here first
> until we find evidence no need for other SoC or only belong to specific
> IP version.

It's better to write this in the comment than a FIXME, which is much
harder to interpret....

>>> +		 * bytes (whatever value for the second word) in Message RAM to
>>> +		 * avoid bit error for transmit data less than 4 bytes
>>> +		 */
>>> +		if (cf->len <= 4)
>>> +			m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(i / 4 + 1),
>>> +					 0x0);
>>
>> This workaround doesn't handle the dlc == 0 case, your error description
>> isn't completely if this is a problem, too.

> You're right.
> I just checked the dlc == 0 case also had such issue and it also needs
> the extra 8 bytes write to avoid such issue.
> 
> BTW the issue only happened on the first time when you send a frame with no
> data(dlc == 0) at the first time.
> e.g.
> root@...6sxsabresd:~# ip link set can0 up type can bitrate 1000000
> [   62.326014] IPv6: ADDRCONF(NETDEV_CHANGE): can0: link becomes ready
> root@...6sxsabresd:~# cansend can0 123#R
> [   69.233645] m_can 20e8000.can can0: Bit Error Uncorrected
> [   69.239167] m_can 20e8000.can can0: Bit Error Corrected
> 
> If we send a frame success first (e.g. 5 bytes data), it will not fail
> again even you send no data frame (dlc == 0) later.
> 
> The former failure of sending data less than 4 bytes is similar.
> 
> Looks like the first 8 bytes of message ram has to be initialised
> for the first using.

What about putting

/* errata description goes here */
m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0), 0x0);
m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1), 0x0);

into the open() function? Can you ask the hardware colleges if this is a
functional workaround.

>> It should be possible to change the for loop to go always to 8, or
>> simply unroll the loop:
>>
>> /* errata description goes here */
>> m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0), *(u32 *)(cf->data + 0));
>> m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1), *(u32 *)(cf->data + 4));
>>
> 
> Yes, i tried to fix it as follows.
> 
> /* FIXME: we meet an IC issue that we have to write the full 8
>  * bytes (whatever value for the second word) in Message RAM to
>  * avoid bit error for transmit data less than 4 bytes
>  */
> if (cf->len <= 4) {
>         m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0),
>                          *(u32 *)(cf->data + 0));
>         m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1),
>                          *(u32 *)(cf->data + 4));
> } else {
>         for (i = 0; i < cf->len; i += 4)
>                 m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(i / 4),
>                                  *(u32 *)(cf->data + i));
> 
> Will update the patch.

Both branches of the above if are doing the same thing, I think you can
replace the while if ... else ... for with this:

/* errata description goes here */
m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0), *(u32 *)(cf->data + 0));
m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1), *(u32 *)(cf->data + 4));

However if writing to DATA(0) and DATA(1) once in the open() function is
enough this code should stay as it is.

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


Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists