RFC: [PATCH v2 1/2] can: flexcan: Correctly initialize mailboxes

[Marc Kleine-Budde]

From: David Jander <***@protonic.nl>

Apparently mailboxes may contain random data at startup, causing some of
them being prepared for message reception. This causes overruns being
missed or even confusing the IRQ check for trasmitted messages, increasing
the transmit counter instead of the error counter.

Signed-off-by: David Jander <***@protonic.nl>
[mkl: adjust starting value of loop]
Signed-off-by: Marc Kleine-Budde <***@pengutronix.de>
---
Changes since v1:
- don't remove existing initialization of FLEXCAN_TX_BUF_ID
- start loop at FLEXCAN_TX_BUF_ID + 1

Marc

drivers/net/can/flexcan.c | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 630c7bf..6ec49bd 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -801,6 +801,7 @@ static int flexcan_chip_start(struct net_device *dev)
struct flexcan_regs __iomem *regs = priv->base;
int err;
u32 reg_mcr, reg_ctrl;
+ int i;

/* enable module */
err = flexcan_chip_enable(priv);
@@ -870,6 +871,11 @@ static int flexcan_chip_start(struct net_device *dev)
/* Abort any pending TX, mark Mailbox as INACTIVE */
flexcan_write(FLEXCAN_MB_CNT_CODE(0x4),
&regs->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
+ /* Clear and invalidate all mailboxes first */
+ for (i = FLEXCAN_TX_BUF_ID + 1; i < ARRAY_SIZE(regs->cantxfg; i++) {
+ flexcan_write(FLEXCAN_MB_CNT_CODE(0),
+ &regs->cantxfg[i].can_ctrl);
+ }

/* acceptance mask/acceptance code (accept everything) */
flexcan_write(0x0, &regs->rxgmask);

[Marc Kleine-Budde]

This patch increases the mask in the FLEXCAN_MCR_MAXMB() to 7 bits as in the
newer flexcan cores the MAXMB field is 7 bits wide.

Signed-off-by: Marc Kleine-Budde <***@pengutronix.de>
---

Changes since v1:
- new

drivers/net/can/flexcan.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 6ec49bd..44ba836 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -62,7 +62,7 @@
#define FLEXCAN_MCR_BCC BIT(16)
#define FLEXCAN_MCR_LPRIO_EN BIT(13)
#define FLEXCAN_MCR_AEN BIT(12)
-#define FLEXCAN_MCR_MAXMB(x) ((x) & 0x1f)
+#define FLEXCAN_MCR_MAXMB(x) ((x) & 0x7f)
#define FLEXCAN_MCR_IDAM_A (0 << 8)
#define FLEXCAN_MCR_IDAM_B (1 << 8)
#define FLEXCAN_MCR_IDAM_C (2 << 8)

[David Jander]

On Tue, 2 Sep 2014 17:03:35 +0200

Post by Marc Kleine-Budde
Apparently mailboxes may contain random data at startup, causing some of
them being prepared for message reception. This causes overruns being
missed or even confusing the IRQ check for trasmitted messages, increasing
the transmit counter instead of the error counter.
[mkl: adjust starting value of loop]
---
- don't remove existing initialization of FLEXCAN_TX_BUF_ID
- start loop at FLEXCAN_TX_BUF_ID + 1
Marc
drivers/net/can/flexcan.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 630c7bf..6ec49bd 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -801,6 +801,7 @@ static int flexcan_chip_start(struct net_device *dev)
struct flexcan_regs __iomem *regs = priv->base;
int err;
u32 reg_mcr, reg_ctrl;
+ int i;
/* enable module */
err = flexcan_chip_enable(priv);
@@ -870,6 +871,11 @@ static int flexcan_chip_start(struct net_device *dev)
/* Abort any pending TX, mark Mailbox as INACTIVE */
flexcan_write(FLEXCAN_MB_CNT_CODE(0x4),
&regs->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
+ /* Clear and invalidate all mailboxes first */
+ for (i = FLEXCAN_TX_BUF_ID + 1; i < ARRAY_SIZE(regs->cantxfg; i++) {
+ flexcan_write(FLEXCAN_MB_CNT_CODE(0),
+ &regs->cantxfg[i].can_ctrl);
+ }
/* acceptance mask/acceptance code (accept everything) */
flexcan_write(0x0, &regs->rxgmask);

This assumes that FLEXCAN_TX_BUF_ID is the ID of the first available MB. Other
than that, it should work I believe.
OTOH, since this loop is more like a memset(...0...), wouldn't it be nice to
just clear all MB's CODE registers and then initialize TX_BUF (and others)
afterwards?

Best regards,

[Marc Kleine-Budde]

Post by David Jander
On Tue, 2 Sep 2014 17:03:35 +0200

Post by Marc Kleine-Budde
Apparently mailboxes may contain random data at startup, causing some of
them being prepared for message reception. This causes overruns being
missed or even confusing the IRQ check for trasmitted messages, increasing
the transmit counter instead of the error counter.
[mkl: adjust starting value of loop]
---
- don't remove existing initialization of FLEXCAN_TX_BUF_ID
- start loop at FLEXCAN_TX_BUF_ID + 1
Marc
drivers/net/can/flexcan.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 630c7bf..6ec49bd 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -801,6 +801,7 @@ static int flexcan_chip_start(struct net_device *dev)
struct flexcan_regs __iomem *regs = priv->base;
int err;
u32 reg_mcr, reg_ctrl;
+ int i;
/* enable module */
err = flexcan_chip_enable(priv);
@@ -870,6 +871,11 @@ static int flexcan_chip_start(struct net_device *dev)
/* Abort any pending TX, mark Mailbox as INACTIVE */
flexcan_write(FLEXCAN_MB_CNT_CODE(0x4),
&regs->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
+ /* Clear and invalidate all mailboxes first */
+ for (i = FLEXCAN_TX_BUF_ID + 1; i < ARRAY_SIZE(regs->cantxfg; i++) {
+ flexcan_write(FLEXCAN_MB_CNT_CODE(0),
+ &regs->cantxfg[i].can_ctrl);
+ }
/* acceptance mask/acceptance code (accept everything) */
flexcan_write(0x0, &regs->rxgmask);

This assumes that FLEXCAN_TX_BUF_ID is the ID of the first available MB. Other
than that, it should work I believe.
OTOH, since this loop is more like a memset(...0...), wouldn't it be nice to
just clear all MB's CODE registers and then initialize TX_BUF (and others)
afterwards?

See patch v3, I've changed the order, first do the loop, then initialize
FLEXCAN_TX_BUF_ID as 0x4. I've pushed this series to the testing branch
of the linux-can repo on gitorious. Make your imx6 errata fix patch
based on this branch.

You probably have to change FLEXCAN_TX_BUF_ID and/or introduce a new
define for the invalid mailbox. When your patch is ready we can look at
the patches and see if it makes sense to shuffle some changes around.

Marc

[David Jander]

On Tue, 2 Sep 2014 17:03:35 +0200

Post by Marc Kleine-Budde
Apparently mailboxes may contain random data at startup, causing some of
them being prepared for message reception. This causes overruns being
missed or even confusing the IRQ check for trasmitted messages, increasing
the transmit counter instead of the error counter.
[mkl: adjust starting value of loop]
---
- don't remove existing initialization of FLEXCAN_TX_BUF_ID
- start loop at FLEXCAN_TX_BUF_ID + 1
Marc
drivers/net/can/flexcan.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 630c7bf..6ec49bd 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -801,6 +801,7 @@ static int flexcan_chip_start(struct net_device *dev)
struct flexcan_regs __iomem *regs = priv->base;
int err;
u32 reg_mcr, reg_ctrl;
+ int i;
/* enable module */
err = flexcan_chip_enable(priv);
@@ -870,6 +871,11 @@ static int flexcan_chip_start(struct net_device *dev)
/* Abort any pending TX, mark Mailbox as INACTIVE */
flexcan_write(FLEXCAN_MB_CNT_CODE(0x4),
&regs->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
+ /* Clear and invalidate all mailboxes first */
+ for (i = FLEXCAN_TX_BUF_ID + 1; i < ARRAY_SIZE(regs->cantxfg; i++) {

Oops! You are missing a closing parenthesis in this line.
I will assume you amend this commit and base mine on top of that...

Post by Marc Kleine-Budde
+ flexcan_write(FLEXCAN_MB_CNT_CODE(0),
+ &regs->cantxfg[i].can_ctrl);
+ }
/* acceptance mask/acceptance code (accept everything) */
flexcan_write(0x0, &regs->rxgmask);

Best regards,

[Marc Kleine-Budde]

Post by David Jander
On Tue, 2 Sep 2014 17:03:35 +0200

Post by Marc Kleine-Budde
Apparently mailboxes may contain random data at startup, causing some of
them being prepared for message reception. This causes overruns being
missed or even confusing the IRQ check for trasmitted messages, increasing
the transmit counter instead of the error counter.
[mkl: adjust starting value of loop]
---
- don't remove existing initialization of FLEXCAN_TX_BUF_ID
- start loop at FLEXCAN_TX_BUF_ID + 1
Marc
drivers/net/can/flexcan.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 630c7bf..6ec49bd 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -801,6 +801,7 @@ static int flexcan_chip_start(struct net_device *dev)
struct flexcan_regs __iomem *regs = priv->base;
int err;
u32 reg_mcr, reg_ctrl;
+ int i;
/* enable module */
err = flexcan_chip_enable(priv);
@@ -870,6 +871,11 @@ static int flexcan_chip_start(struct net_device *dev)
/* Abort any pending TX, mark Mailbox as INACTIVE */
flexcan_write(FLEXCAN_MB_CNT_CODE(0x4),
&regs->cantxfg[FLEXCAN_TX_BUF_ID].can_ctrl);
+ /* Clear and invalidate all mailboxes first */
+ for (i = FLEXCAN_TX_BUF_ID + 1; i < ARRAY_SIZE(regs->cantxfg; i++) {

Oops! You are missing a closing parenthesis in this line.
I will assume you amend this commit and base mine on top of that...

Doh! I introduced this problem, when converting the 64 to
ARRAY_SIZE(...without the closing ).

Fixed.

Marc