Re: [net-next 6/6] can: mcp251xfd: mcp251xfd_regmap_crc_read(): work around broken CRC on TBC register

Hello Marc,

We observe the very similar issue with MCP2517FD.

The CRC errors the patch works around are CRC errors introduced by a
chip erratum, not by electromagnetic interference. In my observation

Are you referring this errata doc
https://datasheet.octopart.com/MCP2517FD-H-JHA-Microchip-datasheet-136609045.pdf ?

We have the similar CRC read errors but
the lowest byte is not 0x00 and 0x80, it's actually 0x0x or 0x8x, e.g.

mcp251xfd spi0.0 can0: CRC read error at address 0x0010 (length=4, data=82 d1 fa 6c, CRC=0xd9c2) retrying.

0xb0 0x10 0x04 0x82 0xd1 0xfa 0x6c => 0x59FD (not matching)

but if I flip the first received bit  (highest bit in the lowest byte):
0xb0 0x10 0x04 0x02 0xd1 0xfa 0x6c => 0xD9C2 (matching!)

So, your fix covers only the case of 0x00 and 0x80, 
do you think that the workaround should be extended so check
     (buf_rx->data[0] == 0x0 || buf_rx->data[0] == 0x80)) {
turns into 
     ((buf_rx->data[0] & 0xf0) == 0x0 || (buf_rx->data[0] & 0xf0) == 0x80)) {

Errata, actually says
"Only bits 7/15/23/31 of the following registers can be affected:"

So, we could basically, in simplest case flip bit 31 and re-check CRC without any check of 
rx->data[0]....

Regards,
Pavel

On 07.12.2021 16:53:12, Modilaynen, Pavel wrote:

Hello Marc,

We observe the very similar issue with MCP2517FD.

quoted

The CRC errors the patch works around are CRC errors introduced by a
chip erratum, not by electromagnetic interference. In my observation

Are you referring this errata doc
https://datasheet.octopart.com/MCP2517FD-H-JHA-Microchip-datasheet-136609045.pdf ?

ACK - although there are newer versions available. The newest erratum
for the mcp2517fd is the "C" issue:

| https://ww1.microchip.com/downloads/en/DeviceDoc/MCP2517FD-Silicon-Errata-and-Data-Sheet-Clarification-DS80000792C.pdf

The mpc2518fd can be downloaded here:

| https://ww1.microchip.com/downloads/en/DeviceDoc/MCP2518FD-Silicon-Errata-and-Data-Sheet-Clarification-DS80000789C.pdf

We have the similar CRC read errors but
the lowest byte is not 0x00 and 0x80, it's actually 0x0x or 0x8x, e.g.

mcp251xfd spi0.0 can0: CRC read error at address 0x0010 (length=4, data=82 d1 fa 6c, CRC=0xd9c2) retrying.

0xb0 0x10 0x04 0x82 0xd1 0xfa 0x6c => 0x59FD (not matching)

but if I flip the first received bit  (highest bit in the lowest byte):
0xb0 0x10 0x04 0x02 0xd1 0xfa 0x6c => 0xD9C2 (matching!)

So, your fix covers only the case of 0x00 and 0x80, 
do you think that the workaround should be extended so check
     (buf_rx->data[0] == 0x0 || buf_rx->data[0] == 0x80)) {
turns into 
     ((buf_rx->data[0] & 0xf0) == 0x0 || (buf_rx->data[0] & 0xf0) == 0x80)) {

Errata, actually says
"Only bits 7/15/23/31 of the following registers can be affected:"

So, we could basically, in simplest case flip bit 31 and re-check CRC
without any check of rx->data[0]....

Can you send a patch that updates the workaround and description.

regards,
Marc

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

Hi Pavel,
 

We have the similar CRC read errors but
the lowest byte is not 0x00 and 0x80, it's actually 0x0x or 0x8x, e.g.

mcp251xfd spi0.0 can0: CRC read error at address 0x0010 (length=4,
data=82 d1 fa 6c, CRC=0xd9c2) retrying.

0xb0 0x10 0x04 0x82 0xd1 0xfa 0x6c => 0x59FD (not matching)

but if I flip the first received bit  (highest bit in the lowest byte):
0xb0 0x10 0x04 0x02 0xd1 0xfa 0x6c => 0xD9C2 (matching!)

What settings do you have on your setup? Can you please print the dmesg output from the init? I'm especially interested in Sysclk and SPI speed.

Thanks,
Thomas

Hi All,

we are also seeing the CRC Errors in our setup (rpi4, Kernel 5.10.x)
from time to time. I just wanted to post here what I am seeing, maybe it helps...

[    6.761711] spi_master spi1: will run message pump with realtime priority
[    6.778063] mcp251xfd spi1.0 can1: MCP2518FD rev0.0 (-RX_INT -MAB_NO_WARN +CRC_REG +CRC_RX +CRC_TX +ECC -HD c:40.00MHz m:20.00MHz r:17.00MHz e:16.66MHz) successfully initialized.

[ 4327.107856] mcp251xfd spi1.0 canfd1: CRC read error at address 0x0010 (length=4, data=00 cc 62 c4, CRC=0xa3a0) retrying.
[ 7770.163335] mcp251xfd spi1.0 canfd1: CRC read error at address 0x0010 (length=4, data=00 bf 16 d5, CRC=0x9d3c) retrying.
[ 8000.565955] mcp251xfd spi1.0 canfd1: CRC read error at address 0x0010 (length=4, data=00 40 66 fa, CRC=0x31d7) retrying.
[ 9753.658173] mcp251xfd spi1.0 canfd1: CRC read error at address 0x0010 (length=4, data=80 e9 01 4e, CRC=0xe862) retrying.


Sven

-----Ursprüngliche Nachricht-----
Von: Thomas.Kopp@microchip.com [off-list ref]
Gesendet: Donnerstag, 9. Dezember 2021 11:22
An: pavel.modilaynen@volvocars.com; mkl@pengutronix.de
Cc: drew@beagleboard.org; linux-can@vger.kernel.org; menschel.p@posteo.de;
netdev@vger.kernel.org; will@macchina.cc
Betreff: RE: [net-next 6/6] can: mcp251xfd: mcp251xfd_regmap_crc_read(): work around
broken CRC on TBC register

Hi Pavel,

quoted

We have the similar CRC read errors but
the lowest byte is not 0x00 and 0x80, it's actually 0x0x or 0x8x, e.g.

mcp251xfd spi0.0 can0: CRC read error at address 0x0010 (length=4,
data=82 d1 fa 6c, CRC=0xd9c2) retrying.

0xb0 0x10 0x04 0x82 0xd1 0xfa 0x6c => 0x59FD (not matching)

but if I flip the first received bit  (highest bit in the lowest byte):
0xb0 0x10 0x04 0x02 0xd1 0xfa 0x6c => 0xD9C2 (matching!)

What settings do you have on your setup? Can you please print the dmesg output from the
init? I'm especially interested in Sysclk and SPI speed.

Thanks,
Thomas

On 09.12.2021 11:17:09, Sven Schuchmann wrote:

we are also seeing the CRC Errors in our setup (rpi4, Kernel 5.10.x)
from time to time. I just wanted to post here what I am seeing, maybe
it helps...

[    6.761711] spi_master spi1: will run message pump with realtime priority
[    6.778063] mcp251xfd spi1.0 can1: MCP2518FD rev0.0 (-RX_INT -MAB_NO_WARN +CRC_REG +CRC_RX +CRC_TX +ECC -HD c:40.00MHz m:20.00MHz r:17.00MHz e:16.66MHz) successfully initialized.

[ 4327.107856] mcp251xfd spi1.0 canfd1: CRC read error at address 0x0010 (length=4, data=00 cc 62 c4, CRC=0xa3a0) retrying.
[ 7770.163335] mcp251xfd spi1.0 canfd1: CRC read error at address 0x0010 (length=4, data=00 bf 16 d5, CRC=0x9d3c) retrying.
[ 8000.565955] mcp251xfd spi1.0 canfd1: CRC read error at address 0x0010 (length=4, data=00 40 66 fa, CRC=0x31d7) retrying.
[ 9753.658173] mcp251xfd spi1.0 canfd1: CRC read error at address 0x0010 (length=4, data=80 e9 01 4e, CRC=0xe862) retrying.

You are using the a back port of my HW timestamp in your v5.10 branch.
So every 45 seconds the TBC register (address 0x0010) is read,
additionally for every CAN error frame.

In the mean time, I've implemented a workaround for the CRC read errors:

| c7eb923c3caf can: mcp251xfd: mcp251xfd_regmap_crc_read(): work around broken CRC on TBC register
| ef7a8c3e7599 can: mcp251xfd: mcp251xfd_regmap_crc_read_one(): Factor out crc check into separate function

It fixes the CRC read error, if the first data byte is 0x00 or 0x80.

These messages should disappear, if you cherry-pick the above patches.

regards,
Marc

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

Hello Marc,

Von: Marc Kleine-Budde [off-list ref]
Gesendet: Donnerstag, 9. Dezember 2021 12:28
An: Sven Schuchmann [off-list ref]

On 09.12.2021 11:17:09, Sven Schuchmann wrote:

quoted

we are also seeing the CRC Errors in our setup (rpi4, Kernel 5.10.x)
from time to time. I just wanted to post here what I am seeing, maybe
it helps...

[    6.761711] spi_master spi1: will run message pump with realtime priority
[    6.778063] mcp251xfd spi1.0 can1: MCP2518FD rev0.0 (-RX_INT -MAB_NO_WARN +CRC_REG

+CRC_RX +CRC_TX +ECC -HD c:40.00MHz m:20.00MHz r:17.00MHz e:16.66MHz) successfully
initialized.

quoted

[ 4327.107856] mcp251xfd spi1.0 canfd1: CRC read error at address 0x0010 (length=4,

data=00 cc 62 c4, CRC=0xa3a0) retrying.

quoted

[ 7770.163335] mcp251xfd spi1.0 canfd1: CRC read error at address 0x0010 (length=4,

data=00 bf 16 d5, CRC=0x9d3c) retrying.

quoted

[ 8000.565955] mcp251xfd spi1.0 canfd1: CRC read error at address 0x0010 (length=4,

data=00 40 66 fa, CRC=0x31d7) retrying.

quoted

[ 9753.658173] mcp251xfd spi1.0 canfd1: CRC read error at address 0x0010 (length=4,

data=80 e9 01 4e, CRC=0xe862) retrying.

You are using the a back port of my HW timestamp in your v5.10 branch.
So every 45 seconds the TBC register (address 0x0010) is read,
additionally for every CAN error frame.

In the mean time, I've implemented a workaround for the CRC read errors:

| c7eb923c3caf can: mcp251xfd: mcp251xfd_regmap_crc_read(): work around broken CRC on TBC
register
| ef7a8c3e7599 can: mcp251xfd: mcp251xfd_regmap_crc_read_one(): Factor out crc check into
separate function

It fixes the CRC read error, if the first data byte is 0x00 or 0x80.

These messages should disappear, if you cherry-pick the above patches.

Sorry for the confusion, you are right.
I picked the two patches and so far no more CRC read errors.
Thanks a lot.

Sven

Hi Thomas,

quoted

We have the similar CRC read errors but
the lowest byte is not 0x00 and 0x80, it's actually 0x0x or 0x8x, e.g.

mcp251xfd spi0.0 can0: CRC read error at address 0x0010 (length=4,
data=82 d1 fa 6c, CRC=0xd9c2) retrying.

0xb0 0x10 0x04 0x82 0xd1 0xfa 0x6c => 0x59FD (not matching)

but if I flip the first received bit  (highest bit in the lowest byte):
0xb0 0x10 0x04 0x02 0xd1 0xfa 0x6c => 0xD9C2 (matching!)

What settings do you have on your setup? Can you please print the dmesg output from the init? I'm especially interested in Sysclk and SPI speed.

mcp251xfd spi0.0 can0: MCP2517FD rev0.0 (-RX_INT +MAB_NO_WARN +CRC_REG +CRC_RX +CRC_TX +ECC -HD c:40.00MHz m:10.00MHz r:10.00MHz e:10.00MHz) successfully initialized.
...

Regards,
Pavel

Hi Pavel,

quoted

quoted

We have the similar CRC read errors but
the lowest byte is not 0x00 and 0x80, it's actually 0x0x or 0x8x, e.g.

mcp251xfd spi0.0 can0: CRC read error at address 0x0010 (length=4,
data=82 d1 fa 6c, CRC=0xd9c2) retrying.

0xb0 0x10 0x04 0x82 0xd1 0xfa 0x6c => 0x59FD (not matching)

but if I flip the first received bit  (highest bit in the lowest byte):
0xb0 0x10 0x04 0x02 0xd1 0xfa 0x6c => 0xD9C2 (matching!)

quoted

What settings do you have on your setup? Can you please print the

dmesg output from the init? I'm especially interested in Sysclk and SPI
speed.

mcp251xfd spi0.0 can0: MCP2517FD rev0.0 (-RX_INT +MAB_NO_WARN
+CRC_REG +CRC_RX +CRC_TX +ECC -HD c:40.00MHz m:10.00MHz
r:10.00MHz e:10.00MHz) successfully initialized.

Thanks for the data. I've looked into this and it seems that the second bit being set in your case does not depend on the SPI-Rate (or the quirks for that matter) but it seems to be hardware setup related. 
I'm fine with changing the driver so that it ignores set LSBs but would limit it to 2 or 3 bits:
(buf_rx->data[0] == 0x0 || buf_rx->data[0] == 0x80))
becomes
((buf_rx->data[0] & 0xf8) == 0x0 || (buf_rx->data[0] & 0xf8) == 0x80)) {

The action also needs to be changed and the flip back of the bit needs to be removed. In this case the flipped databit that produces a matching CRC is actually  correct (i.e. consistent with the 7 LSBs in that byte.)
A patch could look like this (I'm currently not close to a setup where I can compile/test this.)

diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
index 297491516a26..e5bc897f37e8 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c

@@ -332,12 +332,10 @@ mcp251xfd_regmap_crc_read(void *context,
                 *
                 * If the highest bit in the lowest byte is flipped
                 * the transferred CRC matches the calculated one. We
-                * assume for now the CRC calculation in the chip
-                * works on wrong data and the transferred data is
-                * correct.
+                * assume for now the CRC operates on the correct data.
                 */
                if (reg == MCP251XFD_REG_TBC &&
-                   (buf_rx->data[0] == 0x0 || buf_rx->data[0] == 0x80)) {
+                   ((buf_rx->data[0] & 0xF8) == 0x0 || (buf_rx->data[0] & 0xF8) == 0x80)) {
                        /* Flip highest bit in lowest byte of le32 */
                        buf_rx->data[0] ^= 0x80;

@@ -347,10 +345,8 @@ mcp251xfd_regmap_crc_read(void *context,
                                                                  val_len);
                        if (!err) {
                                /* If CRC is now correct, assume
-                                * transferred data was OK, flip bit
-                                * back to original value.
+                                * flipped data was OK.
                                 */
-                               buf_rx->data[0] ^= 0x80;
                                goto out;
                        }
                }

Thanks,
Thomas

Picking up this old thread....

On 21.12.2021 22:24:52, Thomas.Kopp@microchip.com wrote:

Thanks for the data. I've looked into this and it seems that the
second bit being set in your case does not depend on the SPI-Rate (or
the quirks for that matter) but it seems to be hardware setup related.

I'm fine with changing the driver so that it ignores set LSBs but
would limit it to 2 or 3 bits:

(buf_rx->data[0] == 0x0 || buf_rx->data[0] == 0x80))
becomes
((buf_rx->data[0] & 0xf8) == 0x0 || (buf_rx->data[0] & 0xf8) == 0x80)) {

The action also needs to be changed and the flip back of the bit needs
to be removed. In this case the flipped databit that produces a
matching CRC is actually  correct (i.e. consistent with the 7 LSBs in
that byte.)

A patch could look like this (I'm currently not close to a setup where
I can compile/test this.)

Thomas, can I have your Signed-off-by for this patch?

Marc

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

On 21.06.2022 16:25:15, Marc Kleine-Budde wrote:

quoted

Thanks for the data. I've looked into this and it seems that the
second bit being set in your case does not depend on the SPI-Rate (or
the quirks for that matter) but it seems to be hardware setup related.

I'm fine with changing the driver so that it ignores set LSBs but
would limit it to 2 or 3 bits:

(buf_rx->data[0] == 0x0 || buf_rx->data[0] == 0x80))
becomes
((buf_rx->data[0] & 0xf8) == 0x0 || (buf_rx->data[0] & 0xf8) == 0x80)) {

The action also needs to be changed and the flip back of the bit
needs to be removed. In this case the flipped databit that produces
a matching CRC is actually correct (i.e. consistent with the 7 LSBs
in that byte.)

The mcp2517fd errata says the transmitted data is okay, but the CRC is
calculated on wrong data:

| It is possible that there is a mismatch between the transmitted CRC
| and the actual CRC for the transmitted data when data are updated at a
| specific time during the SPI READ_CRC command. In these cases, the
| transmitted CRC is wrong. The data transmitted are correct.

regards,
Marc

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

Hi Marc,

Picking up this old thread....

Thomas, can I have your Signed-off-by for this patch?

Marc

quoted

diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c

b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c

quoted

index 297491516a26..e5bc897f37e8 100644

--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c

@@ -332,12 +332,10 @@ mcp251xfd_regmap_crc_read(void *context,
                 *
                 * If the highest bit in the lowest byte is flipped
                 * the transferred CRC matches the calculated one. We
-                * assume for now the CRC calculation in the chip
-                * works on wrong data and the transferred data is
-                * correct.
+                * assume for now the CRC operates on the correct data.
                 */
                if (reg == MCP251XFD_REG_TBC &&
-                   (buf_rx->data[0] == 0x0 || buf_rx->data[0] == 0x80)) {
+                   ((buf_rx->data[0] & 0xF8) == 0x0 || (buf_rx->data[0] & 0xF8) ==

0x80)) {

quoted

                        /* Flip highest bit in lowest byte of le32 */
                        buf_rx->data[0] ^= 0x80;

@@ -347,10 +345,8 @@ mcp251xfd_regmap_crc_read(void *context,
                                                                  val_len);
                        if (!err) {
                                /* If CRC is now correct, assume
-                                * transferred data was OK, flip bit
-                                * back to original value.
+                                * flipped data was OK.
                                 */
-                               buf_rx->data[0] ^= 0x80;
                                goto out;
                        }
                }

Signed-off-by: Thomas Kopp <thomas.kopp@microchip.com>

The mcp2517fd errata says the transmitted data is okay, but the CRC is
calculated on wrong data:

Yes, will be updated.

Best Regards,
Thomas

On 21.12.2021 22:24:52, Thomas.Kopp@microchip.com wrote:

With this change the read of the TBC on the mcp2517fd becomes much more
stable. No more single bit flips in the 1st data byte that can be fixed
with xor 0x80.

regards,
Marc

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