mcp251x.c : patch to avoid repeated frame bug

mcp251x.c : patch to avoid repeated frame bug

[Benoit]

Hello,

The MCP2515 has a silicon bug causing repeated frame transmission, see 
section 5 of MCP2515 Rev. B Silicon Errata Revision G (March 2007).

Basically, setting TXBnCTRL.TXREQ in either SPI mode (00 or 11) will 
eventually cause the bug. The workaround proposed by Microchip is to use 
mode 00 and send an RTS command on the SPI bus to initiate the 
transmission. Accordingly, I have modified the transmission routine as 
follows in file mcp251x.c:


--- drivers/net/can/mcp251x.c-original	2012-08-26 15:00:28.000000000 +0200
+++ drivers/net/can/mcp251x.c	2012-08-26 20:42:58.000000000 +0200
@@ -83,6 +83,10 @@
  #define INSTRUCTION_LOAD_TXB(n)	(0x40 + 2 * (n))
  #define INSTRUCTION_READ_RXB(n)	(((n) == 0) ? 0x90 : 0x94)
  #define INSTRUCTION_RESET	0xC0
+#define	RTS_TXB0		0x01
+#define	RTS_TXB1		0x02
+#define	RTS_TXB2		0x04
+#define INSTRUCTION_RTS(txb)	(0x80 | ((txb) & 0x07))
  
  /* MPC251x registers */
  #define CANSTAT	      0x0e
@@ -394,9 +398,20 @@
  	}
  }
  
+
+/*
+ * CAN frame is now sent using instruction RTS over SPI to work around
+ * repeated frame problem as per MCP2515 Rev B. Silicon Errata revision G
+ * (March 2007) about repeated frames (see section 5 of document).
+ * Note that repeated frames can still occur if using SPI mode 11. It seems
+ * however that spi mode is forcibly set to 0, so it looks safe.
+ *
+ */
  static void mcp251x_hw_tx(struct spi_device *spi, struct can_frame *frame,
  			  int tx_buf_idx)
  {
+	struct mcp251x_priv *priv = dev_get_drvdata(&spi->dev);
+	int ret;
  	u32 sid, eid, exide, rtr;
  	u8 buf[SPI_TRANSFER_BUF_LEN];
  
@@ -418,7 +433,12 @@
  	buf[TXBDLC_OFF] = (rtr << DLC_RTR_SHIFT) | frame->can_dlc;
  	memcpy(buf + TXBDAT_OFF, frame->data, frame->can_dlc);
  	mcp251x_hw_tx_frame(spi, buf, frame->can_dlc, tx_buf_idx);
-	mcp251x_write_reg(spi, TXBCTRL(tx_buf_idx), TXBCTRL_TXREQ);
+
+	priv->spi_tx_buf[0] = INSTRUCTION_RTS(1 << tx_buf_idx);
+	ret = spi_write(spi, priv->spi_tx_buf, 1);
+	if (ret) {
+		dev_err(&spi->dev, "RTS failed: ret = %d\n", ret);
+	}
  }
  
  static void mcp251x_hw_rx_frame(struct spi_device *spi, u8 *buf,



Would it be possible to integrate this patch into mainstream mcp251x.c?

Kind Regards,

Benoît.

Re: mcp251x.c : patch to avoid repeated frame bug

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3172 bytes --]

On 08/27/2012 03:02 PM, Benoit wrote:
> Hello,
> 
> The MCP2515 has a silicon bug causing repeated frame transmission, see
> section 5 of MCP2515 Rev. B Silicon Errata Revision G (March 2007).
> 
> Basically, setting TXBnCTRL.TXREQ in either SPI mode (00 or 11) will
> eventually cause the bug. The workaround proposed by Microchip is to use
> mode 00 and send an RTS command on the SPI bus to initiate the
> transmission. Accordingly, I have modified the transmission routine as
> follows in file mcp251x.c:

I've some questions about the code:

> --- drivers/net/can/mcp251x.c-original    2012-08-26 15:00:28.000000000
> +0200
> +++ drivers/net/can/mcp251x.c    2012-08-26 20:42:58.000000000 +0200
> @@ -83,6 +83,10 @@
>  #define INSTRUCTION_LOAD_TXB(n)    (0x40 + 2 * (n))
>  #define INSTRUCTION_READ_RXB(n)    (((n) == 0) ? 0x90 : 0x94)
>  #define INSTRUCTION_RESET    0xC0
> +#define    RTS_TXB0        0x01
> +#define    RTS_TXB1        0x02
> +#define    RTS_TXB2        0x04
          ^^^^
Please use a space between define and RTS_

> +#define INSTRUCTION_RTS(txb)    (0x80 | ((txb) & 0x07))
>  
>  /* MPC251x registers */
>  #define CANSTAT          0x0e
> @@ -394,9 +398,20 @@
>      }
>  }
>  
> +
> +/*
> + * CAN frame is now sent using instruction RTS over SPI to work around
> + * repeated frame problem as per MCP2515 Rev B. Silicon Errata revision G
> + * (March 2007) about repeated frames (see section 5 of document).
> + * Note that repeated frames can still occur if using SPI mode 11. It
> seems
> + * however that spi mode is forcibly set to 0, so it looks safe.
> + *
> + */

I think we don't need this in the code it will go into the commit
message. A short note before the priv->spi_tx_buf[0] should be enough.

>  static void mcp251x_hw_tx(struct spi_device *spi, struct can_frame *frame,
>                int tx_buf_idx)
>  {
> +    struct mcp251x_priv *priv = dev_get_drvdata(&spi->dev);
> +    int ret;
>      u32 sid, eid, exide, rtr;
>      u8 buf[SPI_TRANSFER_BUF_LEN];
>  
> @@ -418,7 +433,12 @@
>      buf[TXBDLC_OFF] = (rtr << DLC_RTR_SHIFT) | frame->can_dlc;
>      memcpy(buf + TXBDAT_OFF, frame->data, frame->can_dlc);
>      mcp251x_hw_tx_frame(spi, buf, frame->can_dlc, tx_buf_idx);
> -    mcp251x_write_reg(spi, TXBCTRL(tx_buf_idx), TXBCTRL_TXREQ);
> +
> +    priv->spi_tx_buf[0] = INSTRUCTION_RTS(1 << tx_buf_idx);
> +    ret = spi_write(spi, priv->spi_tx_buf, 1);

Why don't you use mcp251x_spi_trans() here?

> +    if (ret) {
> +        dev_err(&spi->dev, "RTS failed: ret = %d\n", ret);
> +    }
>  }
>  
>  static void mcp251x_hw_rx_frame(struct spi_device *spi, u8 *buf,
> 
> 
> 
> Would it be possible to integrate this patch into mainstream mcp251x.c?
> 
> Kind Regards,
> 
> Benoît

I'll send an updated version of your proposed patch.

regards, Marc
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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]