[7/7] can: m_can: workaround for transmit data less than 4 bytes

Commit Message

Aisheng Dong Nov. 4, 2014, 9:27 a.m. UTC

On Tue, Nov 04, 2014 at 10:22:16AM +0100, Marc Kleine-Budde wrote:
> 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@imx6qdlsolo:~# ip link set can0 up type can bitrate 1000000
> >>> [   66.882520] IPv6: ADDRCONF(NETDEV_CHANGE): can0: link becomes ready
> >>> root@imx6qdlsolo:~# cansend can0 123#112233
> >>> [   66.935640] m_can 20e8000.can can0: Bit Error Uncorrected
> >>>
> >>> Signed-off-by: Dong Aisheng <[e-mail]>
> >>> ---
> >>>  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@imx6sxsabresd:~# ip link set can0 up type can bitrate 1000000
> > [   62.326014] IPv6: ADDRCONF(NETDEV_CHANGE): can0: link becomes ready
> > root@imx6sxsabresd:~# 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:
> 

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

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

Regards
Dong Aisheng

> 
> Marc
> -- 
> Pengutronix e.K.                  | Marc Kleine-Budde           |
> Industrial Linux Solutions        | Phone: [phone]     |
> Vertretung West/Dortmund          | Fax:   [phone]-5555 |
> Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |
>

Comments

Marc Kleine-Budde Nov. 4, 2014, 10:33 a.m. UTC | #1

On 11/04/2014 10:27 AM, Dong Aisheng wrote:
>>>> 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.

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

> +        * 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?

Marc

Oliver Hartkopp Nov. 4, 2014, 1:13 p.m. UTC | #2

On 04.11.2014 11:33, Marc Kleine-Budde wrote:
> On 11/04/2014 10:27 AM, Dong Aisheng wrote:


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

Yes. Also put the current IP revision (3.0.x) into the comment.
Did inform the Bosch guys from this issue - or is it already in some errata sheet?

>
>> +        * 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?

Just a nitpick:

DLC can just be 0 .. 15

and the length (struct canfd_frame.len) can be from 0 .. 64

See:

http://lxr.free-electrons.com/source/include/uapi/linux/can.h#L83

That's the reason for all these helpers

http://lxr.free-electrons.com/source/drivers/net/can/dev.c#L36

that hide the evil "DLC" from userspace now and make 'len' a usable loop 
variable as we were able to use the former dlc for classic CAN :-)

Regards,
Oliver

Marc Kleine-Budde Nov. 4, 2014, 1:28 p.m. UTC | #3

On 11/04/2014 02:13 PM, Oliver Hartkopp wrote:
> 
> 
> On 04.11.2014 11:33, Marc Kleine-Budde wrote:
>> On 11/04/2014 10:27 AM, Dong Aisheng wrote:
> 
> 
>>> +       /* 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.
> 
> Yes. Also put the current IP revision (3.0.x) into the comment.

Good idea - also add the SoC's mask revision.

> Did inform the Bosch guys from this issue - or is it already in some
> errata sheet?
> 
>>
>>> +        * 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?
> 
> Just a nitpick:
> 
> DLC can just be 0 .. 15

Correct, I was talking about the length

> and the length (struct canfd_frame.len) can be from 0 .. 64
> 
> See:
> 
> http://lxr.free-electrons.com/source/include/uapi/linux/can.h#L83
> 
> That's the reason for all these helpers
> 
> http://lxr.free-electrons.com/source/drivers/net/can/dev.c#L36
> 
> that hide the evil "DLC" from userspace now and make 'len' a usable loop
> variable as we were able to use the former dlc for classic CAN :-)

Marc

Aisheng Dong Nov. 5, 2014, 2:03 a.m. UTC | #4

On Tue, Nov 04, 2014 at 11:33:09AM +0100, Marc Kleine-Budde wrote:
> On 11/04/2014 10:27 AM, Dong Aisheng wrote:
> >>>> 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.
> 
> >> /* 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.

> > +        * 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: [phone]     |
> Vertretung West/Dortmund          | Fax:   [phone]-5555 |
> Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |
>

Aisheng Dong Nov. 5, 2014, 2:07 a.m. UTC | #5

On Tue, Nov 04, 2014 at 02:13:30PM +0100, Oliver Hartkopp wrote:
> 
> 
> On 04.11.2014 11:33, Marc Kleine-Budde wrote:
> >On 11/04/2014 10:27 AM, Dong Aisheng wrote:
> 
> 
> >>+       /* 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.
> 
> Yes. Also put the current IP revision (3.0.x) into the comment.
> Did inform the Bosch guys from this issue - or is it already in some errata sheet?
> 

Good idea, will add it.
I will try if we can talk Bosch guys about this issue.

Regards
Dong Aisheng

> >
> >>+        * 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?
> 
> Just a nitpick:
> 
> DLC can just be 0 .. 15
> 
> and the length (struct canfd_frame.len) can be from 0 .. 64
> 
> See:
> 
> http://lxr.free-electrons.com/source/include/uapi/linux/can.h#L83
> 
> That's the reason for all these helpers
> 
> http://lxr.free-electrons.com/source/drivers/net/can/dev.c#L36
> 
> that hide the evil "DLC" from userspace now and make 'len' a usable
> loop variable as we were able to use the former dlc for classic CAN
> :-)
> 
> Regards,
> Oliver
>

Patch

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
+        * 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);
 }