[RFC] Fix D_CAN corrupted bytes by using 32 bit register r/w

[RFC] Fix D_CAN corrupted bytes by using 32 bit register r/w

[tthayer at opensource.altera.com]

From: Thor Thayer <tthayer@opensource.altera.com>

Corrupted bytes in CAN transmission on the Altera CycloneV seem
to be an issue because the D_CAN registers are 32 bits [1].
Changing to a 32 bit write fixes the problem and this patch
includes one method of fixing the problem by selecting 32 bit
writes for D_CAN or 16 bit writes for C_CAN.

Another option would be to remove the D_CAN if test and always use
the priv->read_reg32() and priv->write_reg32() for both C_CAN and
D_CAN. The C_CAN read_reg32() function performs two 16 bit writes.
The code is cleaner without the D_CAN if branch but it adds additional
overhead for C_CAN (2nd 16 bit write may not be needed in many cases).

This patch isolates the changes to D_CAN and has been tesed on
on the Altera CycloneV devkit using a flood write test.

[1] http://comments.gmane.org/gmane.linux.can/9402

Thor Thayer (1):
  can: c_can: Update D_CAN TX and RX functions to 32 bit.

 drivers/net/can/c_can/c_can.c |   38 +++++++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 7 deletions(-)

-- 
1.7.9.5

[RFC] can: c_can: Update D_CAN TX and RX functions to 32 bit.

[tthayer at opensource.altera.com]

From: Thor Thayer <tthayer@opensource.altera.com>

When testing CAN write floods on Altera's CycloneV, the
first 2 bytes are sometimes 0x00, 0x00 or corrupted
instead of the values sent. Also observed bytes 4 & 5
were corrupted in some cases.

The D_CAN Data registers are 32 bits and changing from
16 bit writes to 32 bit writes fixes the problem.

Testing performed on Altera CycloneV (D_CAN).
Requesting tests on other C_CAN & D_CAN platforms.

Reported-by: Richard Andrysek <richard.andrysek@gomtec.de>
Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
---
 drivers/net/can/c_can/c_can.c |   38 +++++++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 7 deletions(-)

diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index f91b094..e3dccd3 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -332,9 +332,23 @@ static void c_can_setup_tx_object(struct net_device *dev, int iface,
 
 	priv->write_reg(priv, C_CAN_IFACE(MSGCTRL_REG, iface), ctrl);
 
-	for (i = 0; i < frame->can_dlc; i += 2) {
-		priv->write_reg(priv, C_CAN_IFACE(DATA1_REG, iface) + i / 2,
-				frame->data[i] | (frame->data[i + 1] << 8));
+	if (priv->type == BOSCH_D_CAN) {
+		u32 data = 0, dreg = C_CAN_IFACE(DATA1_REG, iface);
+
+		for (i = 0; i < frame->can_dlc; i += 4, dreg += 2) {
+			data = (u32)frame->data[i];
+			data |= (u32)frame->data[i + 1] << 8;
+			data |= (u32)frame->data[i + 2] << 16;
+			data |= (u32)frame->data[i + 3] << 24;
+			priv->write_reg32(priv, dreg, data);
+		}
+	} else {
+		for (i = 0; i < frame->can_dlc; i += 2) {
+			priv->write_reg(priv,
+					C_CAN_IFACE(DATA1_REG, iface) + i / 2,
+					frame->data[i] |
+					(frame->data[i + 1] << 8));
+		}
 	}
 }
 
@@ -402,10 +416,20 @@ static int c_can_read_msg_object(struct net_device *dev, int iface, u32 ctrl)
 	} else {
 		int i, dreg = C_CAN_IFACE(DATA1_REG, iface);
 
-		for (i = 0; i < frame->can_dlc; i += 2, dreg ++) {
-			data = priv->read_reg(priv, dreg);
-			frame->data[i] = data;
-			frame->data[i + 1] = data >> 8;
+		if (priv->type == BOSCH_D_CAN) {
+			for (i = 0; i < frame->can_dlc; i += 4, dreg += 2) {
+				data = priv->read_reg32(priv, dreg);
+				frame->data[i] = data;
+				frame->data[i + 1] = data >> 8;
+				frame->data[i + 2] = data >> 16;
+				frame->data[i + 3] = data >> 24;
+			}
+		} else {
+			for (i = 0; i < frame->can_dlc; i += 2, dreg++) {
+				data = priv->read_reg(priv, dreg);
+				frame->data[i] = data;
+				frame->data[i + 1] = data >> 8;
+			}
 		}
 	}
 
-- 
1.7.9.5

[RFC] can: c_can: Update D_CAN TX and RX functions to 32 bit.

[Marc Kleine-Budde]

On 06/16/2016 06:10 PM, tthayer at opensource.altera.com wrote:
> From: Thor Thayer <tthayer@opensource.altera.com>
> 
> When testing CAN write floods on Altera's CycloneV, the
> first 2 bytes are sometimes 0x00, 0x00 or corrupted
> instead of the values sent. Also observed bytes 4 & 5
> were corrupted in some cases.
> 
> The D_CAN Data registers are 32 bits and changing from
> 16 bit writes to 32 bit writes fixes the problem.
> 
> Testing performed on Altera CycloneV (D_CAN).
> Requesting tests on other C_CAN & D_CAN platforms.
> 
> Reported-by: Richard Andrysek <richard.andrysek@gomtec.de>
> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>

Applied to can with stable on Cc.

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   |

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 455 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160617/fcc0cd34/attachment.sig>

AW: [RFC] can: c_can: Update D_CAN TX and RX functions to 32 bit.

[Richard Andrysek]

Sorry for a delay. I've run my test with 16 messages per channel (can0, can1 concurrently) with a cycle time 2.8ms, 1Mbps. And it works now properly.

Thank you all participated on this issue

Richard

-----Urspr?ngliche Nachricht-----
Von: Marc Kleine-Budde [mailto:mkl at pengutronix.de] 
Gesendet: Freitag, 17. Juni 2016 11:04
An: tthayer at opensource.altera.com; socketcan at hartkopp.net; wg at grandegger.com; Richard Andrysek <richard.andrysek@gomtec.de>
Cc: anilkumar at ti.com; linux-can at vger.kernel.org; linux-arm-kernel at lists.infradead.org; tthayer.linux at gmail.com
Betreff: Re: [RFC] can: c_can: Update D_CAN TX and RX functions to 32 bit.

On 06/16/2016 06:10 PM, tthayer at opensource.altera.com wrote:
> From: Thor Thayer <tthayer@opensource.altera.com>
> 
> When testing CAN write floods on Altera's CycloneV, the first 2 bytes 
> are sometimes 0x00, 0x00 or corrupted instead of the values sent. Also 
> observed bytes 4 & 5 were corrupted in some cases.
> 
> The D_CAN Data registers are 32 bits and changing from
> 16 bit writes to 32 bit writes fixes the problem.
> 
> Testing performed on Altera CycloneV (D_CAN).
> Requesting tests on other C_CAN & D_CAN platforms.
> 
> Reported-by: Richard Andrysek <richard.andrysek@gomtec.de>
> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>

Applied to can with stable on Cc.

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   |

[RFC] Fix D_CAN corrupted bytes by using 32 bit register r/w

[Marc Kleine-Budde]

On 06/16/2016 06:10 PM, tthayer at opensource.altera.com wrote:
> From: Thor Thayer <tthayer@opensource.altera.com>
> 
> Corrupted bytes in CAN transmission on the Altera CycloneV seem
> to be an issue because the D_CAN registers are 32 bits [1].
> Changing to a 32 bit write fixes the problem and this patch
> includes one method of fixing the problem by selecting 32 bit
> writes for D_CAN or 16 bit writes for C_CAN.
> 
> Another option would be to remove the D_CAN if test and always use
> the priv->read_reg32() and priv->write_reg32() for both C_CAN and
> D_CAN. The C_CAN read_reg32() function performs two 16 bit writes.
> The code is cleaner without the D_CAN if branch but it adds additional
> overhead for C_CAN (2nd 16 bit write may not be needed in many cases).

Better save the overhead, as an additional register access may seems to
be quite expensive on some platforms.

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   |

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 455 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160617/550a4c14/attachment.sig>