Message-ID: <CAPgEAj6N9d=s1a-P_P0mBe1aV2tQBQ4m6shvbPcPvX7W1NNzJw@mail.gmail.com>
Date:   Wed, 21 Apr 2021 12:58:23 -0700
From:   Drew Fustini <drew@...gleboard.org>
To:     Marc Kleine-Budde <mkl@...gutronix.de>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, linux-can@...r.kernel.org,
        kernel@...gutronix.de, Manivannan Sadhasivam <mani@...nel.org>,
        Will C <will@...china.cc>,
        Thomas Kopp <thomas.kopp@...rochip.com>
Subject: Re: [net-next 6/6] can: mcp251xfd: mcp251xfd_regmap_crc_read(): work
 around broken CRC on TBC register

On Wed, Apr 7, 2021 at 1:01 AM Marc Kleine-Budde <mkl@...gutronix.de> wrote:
>
> MCP251XFD_REG_TBC is the time base counter register. It increments
> once per SYS clock tick, which is 20 or 40 MHz. Observation shows that
> if the lowest byte (which is transferred first on the SPI bus) of that
> register is 0x00 or 0x80 the calculated CRC doesn't always match the
> transferred one.
>
> To reproduce this problem let the driver read the TBC register in a
> high frequency. This can be done by attaching only the mcp251xfd CAN
> controller to a valid terminated CAN bus and send a single CAN frame.
> As there are no other CAN controller on the bus, the sent CAN frame is
> not ACKed and the mcp251xfd repeats it. If user space enables the bus
> error reporting, each of the NACK errors is reported with a time
> stamp (which is read from the TBC register) to user space.
>
> $ ip link set can0 down
> $ ip link set can0 up type can bitrate 500000 berr-reporting on
> $ cansend can0 4FF#ff.01.00.00.00.00.00.00
>
> This leads to several error messages per second:
>
> | mcp251xfd spi0.0 can0: CRC read error at address 0x0010 (length=4, data=00 3a 86 da, CRC=0x7753) retrying.
> | mcp251xfd spi0.0 can0: CRC read error at address 0x0010 (length=4, data=80 01 b4 da, CRC=0x5830) retrying.
> | mcp251xfd spi0.0 can0: CRC read error at address 0x0010 (length=4, data=00 e9 23 db, CRC=0xa723) retrying.
> | mcp251xfd spi0.0 can0: CRC read error at address 0x0010 (length=4, data=00 8a 30 db, CRC=0x4a9c) retrying.
> | mcp251xfd spi0.0 can0: CRC read error at address 0x0010 (length=4, data=80 f3 43 db, CRC=0x66d2) retrying.
>
> 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.
>
> This patch implements the following workaround:
>
> - If a CRC read error on the TBC register is detected and the lowest
>   byte is 0x00 or 0x80, the highest bit of the lowest byte is flipped
>   and the CRC is calculated again.
> - If the CRC now matches, the _original_ data is passed to the reader.
>   For now we assume transferred data was OK.
>
> Link: https://lore.kernel.org/r/20210406110617.1865592-5-mkl@pengutronix.de
> Cc: Manivannan Sadhasivam <mani@...nel.org>
> Cc: Thomas Kopp <thomas.kopp@...rochip.com>
> Signed-off-by: Marc Kleine-Budde <mkl@...gutronix.de>
> ---
>  .../net/can/spi/mcp251xfd/mcp251xfd-regmap.c  | 34 +++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
> index 35557ac43c03..297491516a26 100644
> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-regmap.c
> @@ -321,6 +321,40 @@ mcp251xfd_regmap_crc_read(void *context,
>                 if (err != -EBADMSG)
>                         return err;
>
> +               /* MCP251XFD_REG_TBC is the time base counter
> +                * register. It increments once per SYS clock tick,
> +                * which is 20 or 40 MHz.
> +                *
> +                * Observation shows that if the lowest byte (which is
> +                * transferred first on the SPI bus) of that register
> +                * is 0x00 or 0x80 the calculated CRC doesn't always
> +                * match the transferred one.
> +                *
> +                * 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.
> +                */
> +               if (reg == MCP251XFD_REG_TBC &&
> +                   (buf_rx->data[0] == 0x0 || buf_rx->data[0] == 0x80)) {
> +                       /* Flip highest bit in lowest byte of le32 */
> +                       buf_rx->data[0] ^= 0x80;
> +
> +                       /* re-check CRC */
> +                       err = mcp251xfd_regmap_crc_read_check_crc(buf_rx,
> +                                                                 buf_tx,
> +                                                                 val_len);
> +                       if (!err) {
> +                               /* If CRC is now correct, assume
> +                                * transferred data was OK, flip bit
> +                                * back to original value.
> +                                */
> +                               buf_rx->data[0] ^= 0x80;
> +                               goto out;
> +                       }
> +               }
> +
>                 /* MCP251XFD_REG_OSC is the first ever reg we read from.
>                  *
>                  * The chip may be in deep sleep and this SPI transfer
> --
> 2.30.2
>
>

Hello Marc,

I am encountering similar error with the 5.10 raspberrypi kernel on
RPi 4 with MCP2518FD:

  mcp251xfd spi0.0 can0: CRC read error at address 0x0010 (length=4,
data=00 ad 58 67, CRC=0xbbfd) retrying

Would it be possible for you to pull these patches into a v5.10 branch
in your linux-rpi repo [1]?

Thanks,
Drew

[1] https://github.com/marckleinebudde/linux-rpi

Powered by blists - more mailing lists