On Tue, Nov 04, 2014 at 11:33:09AM +0100, Marc Kleine-Budde wrote:

On 11/04/2014 10:27 AM, Dong Aisheng wrote:

quoted

quoted

quoted

quoted

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:

Not the same thing.
The later one will cover payload up to 64 bytes.

Doh! I'm not used to CAN-FD, yet :) However, I'll apply this fix before
adding the CAN-FD support.

quoted

quoted

/* 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.

I tried put them into open() function and the quick test showed it worked.

Do you think it's ok to put things into open() function for this issue
as follows?

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 065e4f1..ca55988 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c

@@ -901,6 +901,15 @@ static void m_can_chip_config(struct net_device *dev)
        /* set bittiming params */
        m_can_set_bittiming(dev);

+       /* We meet an IC issue that we have to write the full 8

At least on the *insert SoC name here*, an issue with the Message RAM
was discovered. Sending CAN frames with dlc less than 4 bytes will lead
to bit errors, when the first 8 bytes of the Message RAM have not been
initialized (i.e. written to). To work around this issue, the first 8
bytes are initialized here.

Looks good.
Will do like that.

quoted

+        * bytes (whatever value for the second word) in Message RAM to
+        * avoid bit error for transmit data less than 4 bytes at the first
+        * time. By initializing the first 8 bytes of tx buffer before using
+        * it can avoid such issue.
+        */
+       m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0), 0x0);
+       m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1), 0x0);
+
        m_can_config_endisable(priv, false);
 }

Can you trigger the issue when sending CAN-FD frames with dlc > 8 && dlc
< 64?

No, i did not see the issue with dlc > 8 && dlc < 64.

Regards
Dong Aisheng

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   |