[PATCH v2 0/4] ARM: vf610: add FlexCAN support

[Stefan Agner]

This series adds support for Vybrid (aka vf610) to the FlexCAN
controller and extends the device tree accordingly.

Changes in v2:
- Split out patch (seperate dt, clocks and driver changes)
- Use spaces for hardware feature flags table
- Remove drive-by change of esdhc register length
- Added related fix which enables clock in berr_counter

Stefan Agner (4):
ARM: dts: vf610: add FlexCAN node
ARM: imx: clk-vf610: fix FlexCAN clock gating
can: flexcan: switch on clocks before accessing ecr register
can: flexcan: add vf610 support for FlexCAN

arch/arm/boot/dts/vf610.dtsi | 23 +++++++++++
arch/arm/mach-imx/clk-vf610.c | 6 ++-
drivers/net/can/flexcan.c | 89 ++++++++++++++++++++++++++++++++++++++-----
3 files changed, 106 insertions(+), 12 deletions(-)

--
2.0.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Stefan Agner]

Reported-by: Ashutosh Singh <ashule...@gmail.com>
Suggested-by: Marc Kleine-Budde <m...@pengutronix.de>
[ste...@agner.ch: added return check for clk_enable_prepare]

Signed-off-by: Stefan Agner <ste...@agner.ch>
---
drivers/net/can/flexcan.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index f425ec2..4c598c9 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -383,11 +383,25 @@ static int flexcan_get_berr_counter(const struct net_device *dev,
{
const struct flexcan_priv *priv = netdev_priv(dev);
struct flexcan_regs __iomem *regs = priv->base;
- u32 reg = flexcan_read(&regs->ecr);
+ u32 reg, err;
+
+ err = clk_prepare_enable(priv->clk_ipg);
+ if (err)
+ return err;
+
+ err = clk_prepare_enable(priv->clk_per);
+ if (err)
+ goto out_disable_ipg;
+
+ reg = flexcan_read(&regs->ecr);

bec->txerr = (reg >> 0) & 0xff;
bec->rxerr = (reg >> 8) & 0xff;

+ clk_disable_unprepare(priv->clk_per);
+ out_disable_ipg:
+ clk_disable_unprepare(priv->clk_ipg);
+
return 0;

[Stefan Agner]

Extend the clock control for FlexCAN with the second gate which
enable the clocks in the Clock Divider (CCM_CSCDR2) register too.

Signed-off-by: Stefan Agner <ste...@agner.ch>
---

arch/arm/mach-imx/clk-vf610.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-imx/clk-vf610.c b/arch/arm/mach-imx/clk-vf610.c
index 22dc3ee..b12b888 100644
--- a/arch/arm/mach-imx/clk-vf610.c
+++ b/arch/arm/mach-imx/clk-vf610.c
@@ -295,8 +295,10 @@ static void __init vf610_clocks_init(struct device_node *ccm_node)

clk[VF610_CLK_ASRC] = imx_clk_gate2("asrc", "ipg_bus", CCM_CCGR4, CCM_CCGRx_CGn(1));

- clk[VF610_CLK_FLEXCAN0] = imx_clk_gate2("flexcan0", "ipg_bus", CCM_CCGR0, CCM_CCGRx_CGn(0));
- clk[VF610_CLK_FLEXCAN1] = imx_clk_gate2("flexcan1", "ipg_bus", CCM_CCGR9, CCM_CCGRx_CGn(4));
+ clk[VF610_CLK_FLEXCAN0] = imx_clk_gate("flexcan0_en", "ipg_bus", CCM_CSCDR2, 11);
+ clk[VF610_CLK_FLEXCAN0] = imx_clk_gate2("flexcan0", "flexcan0_en", CCM_CCGR0, CCM_CCGRx_CGn(0));
+ clk[VF610_CLK_FLEXCAN1] = imx_clk_gate("flexcan1_en", "ipg_bus", CCM_CSCDR2, 12);
+ clk[VF610_CLK_FLEXCAN1] = imx_clk_gate2("flexcan1", "flexcan1_en", CCM_CCGR9, CCM_CCGRx_CGn(4));

clk[VF610_CLK_DMAMUX0] = imx_clk_gate2("dmamux0", "platform_bus", CCM_CCGR0, CCM_CCGRx_CGn(4));
clk[VF610_CLK_DMAMUX1] = imx_clk_gate2("dmamux1", "platform_bus", CCM_CCGR0, CCM_CCGRx_CGn(5));

[Stefan Agner]

Extend FlexCAN driver to support Vybrid. Vybrids variant of the IP
has ECC support which is controlled through the memory error
control register (MECR). There is also an errata which leads to
false positive error detections (ID e5295). This patch disables
the memory error detection completely.

Signed-off-by: Stefan Agner <ste...@agner.ch>
---

drivers/net/can/flexcan.c | 73 +++++++++++++++++++++++++++++++++++++++++------
1 file changed, 64 insertions(+), 9 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 4c598c9..a0fc53e 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -92,6 +92,27 @@
#define FLEXCAN_CTRL_ERR_ALL \
(FLEXCAN_CTRL_ERR_BUS | FLEXCAN_CTRL_ERR_STATE)

+/* FLEXCAN control register 2 (CTRL2) bits */
+#define FLEXCAN_CRL2_ECRWRE BIT(29)
+#define FLEXCAN_CRL2_WRMFRZ BIT(28)
+#define FLEXCAN_CRL2_RFFN(x) (((x) & 0x0f) << 24)
+#define FLEXCAN_CRL2_TASD(x) (((x) & 0x1f) << 19)
+#define FLEXCAN_CRL2_MRP BIT(18)
+#define FLEXCAN_CRL2_RRS BIT(17)
+#define FLEXCAN_CRL2_EACEN BIT(16)
+
+/* FLEXCAN memory error control register (MECR) bits */
+#define FLEXCAN_MECR_ECRWRDIS BIT(31)
+#define FLEXCAN_MECR_HANCEI_MSK BIT(19)
+#define FLEXCAN_MECR_FANCEI_MSK BIT(18)
+#define FLEXCAN_MECR_CEI_MSK BIT(16)
+#define FLEXCAN_MECR_HAERRIE BIT(15)
+#define FLEXCAN_MECR_FAERRIE BIT(14)
+#define FLEXCAN_MECR_EXTERRIE BIT(13)
+#define FLEXCAN_MECR_RERRDIS BIT(9)
+#define FLEXCAN_MECR_ECCDIS BIT(8)
+#define FLEXCAN_MECR_NCEFAFRZ BIT(7)
+
/* FLEXCAN error and status register (ESR) bits */
#define FLEXCAN_ESR_TWRN_INT BIT(17)
#define FLEXCAN_ESR_RWRN_INT BIT(16)
@@ -150,18 +171,20 @@
* FLEXCAN hardware feature flags
*
* Below is some version info we got:
- * SOC Version IP-Version Glitch- [TR]WRN_INT
- * Filter? connected?
- * MX25 FlexCAN2 03.00.00.00 no no
- * MX28 FlexCAN2 03.00.04.00 yes yes
- * MX35 FlexCAN2 03.00.00.00 no no
- * MX53 FlexCAN2 03.00.00.00 yes no
- * MX6s FlexCAN3 10.00.12.00 yes yes
+ * SOC Version IP-Version Glitch- [TR]WRN_INT Memory err
+ * Filter? connected? detection
+ * MX25 FlexCAN2 03.00.00.00 no no no
+ * MX28 FlexCAN2 03.00.04.00 yes yes no
+ * MX35 FlexCAN2 03.00.00.00 no no no
+ * MX53 FlexCAN2 03.00.00.00 yes no no
+ * MX6s FlexCAN3 10.00.12.00 yes yes no
+ * VF610 FlexCAN3 ? no no yes
*
* Some SOCs do not have the RX_WARN & TX_WARN interrupt line connected.
*/
#define FLEXCAN_HAS_V10_FEATURES BIT(1) /* For core version >= 10 */
#define FLEXCAN_HAS_BROKEN_ERR_STATE BIT(2) /* [TR]WRN_INT not connected */
+#define FLEXCAN_HAS_MECR_FEATURES BIT(3) /* Memory error detection */

/* Structure of the message buffer */
struct flexcan_mb {
@@ -192,8 +215,11 @@ struct flexcan_regs {
u32 crcr; /* 0x44 */
u32 rxfgmask; /* 0x48 */
u32 rxfir; /* 0x4c */
- u32 _reserved3[12];
+ u32 _reserved3[12]; /* 0x50 */
struct flexcan_mb cantxfg[64];
+ u32 _reserved4[408];
+ u32 mecr; /* 0xae0 */
+ u32 erriar; /* 0xae4 */
};

struct flexcan_devtype_data {
@@ -223,6 +249,9 @@ static struct flexcan_devtype_data fsl_imx28_devtype_data;
static struct flexcan_devtype_data fsl_imx6q_devtype_data = {
.features = FLEXCAN_HAS_V10_FEATURES,
};
+static struct flexcan_devtype_data fsl_vf610_devtype_data = {
+ .features = FLEXCAN_HAS_V10_FEATURES | FLEXCAN_HAS_MECR_FEATURES,
+};

static const struct can_bittiming_const flexcan_bittiming_const = {
.name = DRV_NAME,
@@ -807,7 +836,7 @@ static int flexcan_chip_start(struct net_device *dev)

struct flexcan_priv *priv = netdev_priv(dev);
struct flexcan_regs __iomem *regs = priv->base;

int err;
- u32 reg_mcr, reg_ctrl;
+ u32 reg_mcr, reg_ctrl, reg_crl2, reg_mecr;

/* enable module */
err = flexcan_chip_enable(priv);
@@ -884,6 +913,31 @@ static int flexcan_chip_start(struct net_device *dev)
if (priv->devtype_data->features & FLEXCAN_HAS_V10_FEATURES)
flexcan_write(0x0, &regs->rxfgmask);

+ /*
+ * On Vybrid, disable memory error detection interrupts
+ * and freeze mode.
+ * This also works around errata e5295 which generates
+ * false positive memory errors and put the device in
+ * freeze mode.
+ */
+ if (priv->devtype_data->features & FLEXCAN_HAS_MECR_FEATURES) {
+ /*
+ * Follow the protocol as described in "Detection
+ * and Correction of Memory Errors" to write to
+ * MECR register
+ */
+ reg_crl2 = flexcan_read(&regs->crl2);
+ reg_crl2 |= FLEXCAN_CRL2_ECRWRE;
+ flexcan_write(reg_crl2, &regs->crl2);
+
+ reg_mecr = flexcan_read(&regs->mecr);
+ reg_mecr &= ~FLEXCAN_MECR_ECRWRDIS;
+ flexcan_write(reg_mecr, &regs->mecr);
+ reg_mecr &= ~(FLEXCAN_MECR_NCEFAFRZ | FLEXCAN_MECR_HANCEI_MSK |
+ FLEXCAN_MECR_FANCEI_MSK);
+ flexcan_write(reg_mecr, &regs->mecr);
+ }
+
err = flexcan_transceiver_enable(priv);
if (err)
goto out_chip_disable;
@@ -1094,6 +1148,7 @@ static const struct of_device_id flexcan_of_match[] = {
{ .compatible = "fsl,imx6q-flexcan", .data = &fsl_imx6q_devtype_data, },
{ .compatible = "fsl,imx28-flexcan", .data = &fsl_imx28_devtype_data, },
{ .compatible = "fsl,p1010-flexcan", .data = &fsl_p1010_devtype_data, },
+ { .compatible = "fsl,vf610-flexcan", .data = &fsl_vf610_devtype_data, },
{ /* sentinel */ },
};
MODULE_DEVICE_TABLE(of, flexcan_of_match);

[Stefan Agner]

Add FlexCAN node for the two FlexCAN IP instances in Vybrid.

Signed-off-by: Stefan Agner <ste...@agner.ch>
---

arch/arm/boot/dts/vf610.dtsi | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)

diff --git a/arch/arm/boot/dts/vf610.dtsi b/arch/arm/boot/dts/vf610.dtsi
index 3fb127a..9404853 100644
--- a/arch/arm/boot/dts/vf610.dtsi
+++ b/arch/arm/boot/dts/vf610.dtsi
@@ -14,6 +14,8 @@

/ {
aliases {
+ can0 = &can0;
+ can1 = &can1;
serial0 = &uart0;
serial1 = &uart1;
serial2 = &uart2;
@@ -103,6 +105,16 @@
<&clks VF610_CLK_DMAMUX1>;
};

+ can0: flexcan@40020000 {
+ compatible = "fsl,vf610-flexcan";
+ reg = <0x40020000 0x4000>;
+ interrupts = <0 58 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clks VF610_CLK_FLEXCAN0>,
+ <&clks VF610_CLK_FLEXCAN0>;
+ clock-names = "ipg", "per";
+ status = "disabled";
+ };
+
uart0: serial@40027000 {
compatible = "fsl,vf610-lpuart";
reg = <0x40027000 0x1000>;
@@ -414,6 +426,17 @@
clock-names = "nfc";
status = "disabled";
};
+
+ can1: flexcan@400d4000 {
+ compatible = "fsl,vf610-flexcan";
+ reg = <0x400d4000 0x4000>;
+ interrupts = <0 59 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clks VF610_CLK_FLEXCAN1>,
+ <&clks VF610_CLK_FLEXCAN1>;
+ clock-names = "ipg", "per";
+ status = "disabled";
+ };
+
};
};
};

[Marc Kleine-Budde]

On 07/14/2014 09:48 AM, Stefan Agner wrote:
> Extend FlexCAN driver to support Vybrid. Vybrids variant of the IP
> has ECC support which is controlled through the memory error
> control register (MECR). There is also an errata which leads to
> false positive error detections (ID e5295). This patch disables
> the memory error detection completely.
>
> Signed-off-by: Stefan Agner <ste...@agner.ch>

Looks good, small nitpick inline.

One nitpick regarding the comment. In driver/net, the preferred
multi-line comment style is

/* comment starts here
* comment continues

*/

> + reg_crl2 = flexcan_read(&regs->crl2);
> + reg_crl2 |= FLEXCAN_CRL2_ECRWRE;
> + flexcan_write(reg_crl2, &regs->crl2);
> +
> + reg_mecr = flexcan_read(&regs->mecr);
> + reg_mecr &= ~FLEXCAN_MECR_ECRWRDIS;
> + flexcan_write(reg_mecr, &regs->mecr);
> + reg_mecr &= ~(FLEXCAN_MECR_NCEFAFRZ | FLEXCAN_MECR_HANCEI_MSK |
> + FLEXCAN_MECR_FANCEI_MSK);
> + flexcan_write(reg_mecr, &regs->mecr);
> + }
> +
> err = flexcan_transceiver_enable(priv);
> if (err)
> goto out_chip_disable;
> @@ -1094,6 +1148,7 @@ static const struct of_device_id flexcan_of_match[] = {
> { .compatible = "fsl,imx6q-flexcan", .data = &fsl_imx6q_devtype_data, },
> { .compatible = "fsl,imx28-flexcan", .data = &fsl_imx28_devtype_data, },
> { .compatible = "fsl,p1010-flexcan", .data = &fsl_p1010_devtype_data, },
> + { .compatible = "fsl,vf610-flexcan", .data = &fsl_vf610_devtype_data, },
> { /* sentinel */ },
> };
> MODULE_DEVICE_TABLE(of, flexcan_of_match);
>

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 |

[Marc Kleine-Budde]

On 07/14/2014 09:48 AM, Stefan Agner wrote:

You should return an error in case of an error.

[Marc Kleine-Budde]

On 07/14/2014 09:48 AM, Stefan Agner wrote:

> This series adds support for Vybrid (aka vf610) to the FlexCAN
> controller and extends the device tree accordingly.
>
> Changes in v2:
> - Split out patch (seperate dt, clocks and driver changes)
> - Use spaces for hardware feature flags table
> - Remove drive-by change of esdhc register length
> - Added related fix which enables clock in berr_counter
>
> Stefan Agner (4):
> ARM: dts: vf610: add FlexCAN node
> ARM: imx: clk-vf610: fix FlexCAN clock gating
> can: flexcan: switch on clocks before accessing ecr register
> can: flexcan: add vf610 support for FlexCAN
>
> arch/arm/boot/dts/vf610.dtsi | 23 +++++++++++
> arch/arm/mach-imx/clk-vf610.c | 6 ++-
> drivers/net/can/flexcan.c | 89 ++++++++++++++++++++++++++++++++++++++-----
> 3 files changed, 106 insertions(+), 12 deletions(-)

Shawn, what's the best way to upstream this? With your Ack, I can take
everything via linux-can-next.

[Stefan Agner]

Am 2014-07-14 10:07, schrieb Marc Kleine-Budde:
<snip>

>> + /*
>> + * On Vybrid, disable memory error detection interrupts
>> + * and freeze mode.
>> + * This also works around errata e5295 which generates
>> + * false positive memory errors and put the device in
>> + * freeze mode.
>> + */
>> + if (priv->devtype_data->features & FLEXCAN_HAS_MECR_FEATURES) {
>> + /*
>> + * Follow the protocol as described in "Detection
>> + * and Correction of Memory Errors" to write to
>> + * MECR register
>> + */
>
>
> One nitpick regarding the comment. In driver/net, the preferred
> multi-line comment style is
>

Just a few lines above that one, another multi-line comment is written
with the empty line at start... With this changed to the net comment
style, we would end up in a mixed style... What do you think?

--
Stefan

[Shawn Guo]

Copy Jingchang ...

On Mon, Jul 14, 2014 at 09:48:29AM +0200, Stefan Agner wrote:
> Extend the clock control for FlexCAN with the second gate which
> enable the clocks in the Clock Divider (CCM_CSCDR2) register too.
>
> Signed-off-by: Stefan Agner <ste...@agner.ch>
> ---
> arch/arm/mach-imx/clk-vf610.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-imx/clk-vf610.c b/arch/arm/mach-imx/clk-vf610.c
> index 22dc3ee..b12b888 100644
> --- a/arch/arm/mach-imx/clk-vf610.c
> +++ b/arch/arm/mach-imx/clk-vf610.c
> @@ -295,8 +295,10 @@ static void __init vf610_clocks_init(struct device_node *ccm_node)
>
> clk[VF610_CLK_ASRC] = imx_clk_gate2("asrc", "ipg_bus", CCM_CCGR4, CCM_CCGRx_CGn(1));
>
> - clk[VF610_CLK_FLEXCAN0] = imx_clk_gate2("flexcan0", "ipg_bus", CCM_CCGR0, CCM_CCGRx_CGn(0));
> - clk[VF610_CLK_FLEXCAN1] = imx_clk_gate2("flexcan1", "ipg_bus", CCM_CCGR9, CCM_CCGRx_CGn(4));
> + clk[VF610_CLK_FLEXCAN0] = imx_clk_gate("flexcan0_en", "ipg_bus", CCM_CSCDR2, 11);
> + clk[VF610_CLK_FLEXCAN0] = imx_clk_gate2("flexcan0", "flexcan0_en", CCM_CCGR0, CCM_CCGRx_CGn(0));

I do not quite understand what "flexcan0_en" clock is and the
relationship between it and clock "flexcan0". I do not think it's a
parent-child clock relationship. Jingchang, do you have more info on
this?

Also when you add a new clock, you should have a new clock ID, something
like VF610_CLK_FLEXCAN0_EN.

Shawn

[Stefan Agner]

There are two enable (gates) bits to enable the FlexCAN clocks: the
first is in the divider register, the second in the clock gate register.
For most clocks there is a divider in between, then it looks like this:

clk[VF610_CLK_ESDHC0_SEL] = imx_clk_mux("esdhc0_sel", CCM_CSCMR1, 16, 2,
esdhc_sels, 4);
clk[VF610_CLK_ESDHC0_EN] = imx_clk_gate("esdhc0_en", "esdhc0_sel",
CCM_CSCDR2, 28);
clk[VF610_CLK_ESDHC0_DIV] = imx_clk_divider("esdhc0_div", "esdhc0_en",
CCM_CSCDR2, 16, 4);
clk[VF610_CLK_ESDHC0] = imx_clk_gate2("eshc0", "esdhc0_div", CCM_CCGR7,
CCM_CCGRx_CGn(1));

However, for FlexCAN no clock selection and no divider is available,
hence its just a chain of an enable and gate register...

But yes, there should be two clock entries for this (I just asking
myself how this code actually could work, afaik the boot loader don't
touches these clocks).

--
Stefan

[Shawn Guo]

On Mon, Jul 14, 2014 at 03:55:29PM +0200, Stefan Agner wrote:
> There are two enable (gates) bits to enable the FlexCAN clocks: the
> first is in the divider register, the second in the clock gate register.
> For most clocks there is a divider in between, then it looks like this:
>
> clk[VF610_CLK_ESDHC0_SEL] = imx_clk_mux("esdhc0_sel", CCM_CSCMR1, 16, 2,
> esdhc_sels, 4);
> clk[VF610_CLK_ESDHC0_EN] = imx_clk_gate("esdhc0_en", "esdhc0_sel",
> CCM_CSCDR2, 28);
> clk[VF610_CLK_ESDHC0_DIV] = imx_clk_divider("esdhc0_div", "esdhc0_en",
> CCM_CSCDR2, 16, 4);
> clk[VF610_CLK_ESDHC0] = imx_clk_gate2("eshc0", "esdhc0_div", CCM_CCGR7,
> CCM_CCGRx_CGn(1));
>
> However, for FlexCAN no clock selection and no divider is available,
> hence its just a chain of an enable and gate register...

Ah, okay. Thanks for the explanation.

Shawn

[Shawn Guo]

On Mon, Jul 14, 2014 at 10:09:54AM +0200, Marc Kleine-Budde wrote:
> On 07/14/2014 09:48 AM, Stefan Agner wrote:
> > This series adds support for Vybrid (aka vf610) to the FlexCAN
> > controller and extends the device tree accordingly.
> >
> > Changes in v2:
> > - Split out patch (seperate dt, clocks and driver changes)
> > - Use spaces for hardware feature flags table
> > - Remove drive-by change of esdhc register length
> > - Added related fix which enables clock in berr_counter
> >
> > Stefan Agner (4):
> > ARM: dts: vf610: add FlexCAN node
> > ARM: imx: clk-vf610: fix FlexCAN clock gating
> > can: flexcan: switch on clocks before accessing ecr register
> > can: flexcan: add vf610 support for FlexCAN
> >
> > arch/arm/boot/dts/vf610.dtsi | 23 +++++++++++
> > arch/arm/mach-imx/clk-vf610.c | 6 ++-
> > drivers/net/can/flexcan.c | 89 ++++++++++++++++++++++++++++++++++++++-----
> > 3 files changed, 106 insertions(+), 12 deletions(-)
>
> Shawn, what's the best way to upstream this? With your Ack, I can take
> everything via linux-can-next.

Since this is a new support and there is nothing to maintain git bisect,
I think the best way is to have the first two patches to go via my tree.

Shawn

[Marc Kleine-Budde]

On 07/14/2014 04:04 PM, Shawn Guo wrote:
> On Mon, Jul 14, 2014 at 10:09:54AM +0200, Marc Kleine-Budde wrote:
>> On 07/14/2014 09:48 AM, Stefan Agner wrote:
>>> This series adds support for Vybrid (aka vf610) to the FlexCAN
>>> controller and extends the device tree accordingly.
>>>
>>> Changes in v2:
>>> - Split out patch (seperate dt, clocks and driver changes)
>>> - Use spaces for hardware feature flags table
>>> - Remove drive-by change of esdhc register length
>>> - Added related fix which enables clock in berr_counter
>>>
>>> Stefan Agner (4):
>>> ARM: dts: vf610: add FlexCAN node
>>> ARM: imx: clk-vf610: fix FlexCAN clock gating
>>> can: flexcan: switch on clocks before accessing ecr register
>>> can: flexcan: add vf610 support for FlexCAN
>>>
>>> arch/arm/boot/dts/vf610.dtsi | 23 +++++++++++
>>> arch/arm/mach-imx/clk-vf610.c | 6 ++-
>>> drivers/net/can/flexcan.c | 89 ++++++++++++++++++++++++++++++++++++++-----
>>> 3 files changed, 106 insertions(+), 12 deletions(-)
>>
>> Shawn, what's the best way to upstream this? With your Ack, I can take
>> everything via linux-can-next.
>
> Since this is a new support and there is nothing to maintain git bisect,
> I think the best way is to have the first two patches to go via my tree.

Fine, with me.

[Marc Kleine-Budde]

On 07/14/2014 03:37 PM, Stefan Agner wrote:
> Am 2014-07-14 10:07, schrieb Marc Kleine-Budde:
> <snip>
>>> + /*
>>> + * On Vybrid, disable memory error detection interrupts
>>> + * and freeze mode.
>>> + * This also works around errata e5295 which generates
>>> + * false positive memory errors and put the device in
>>> + * freeze mode.
>>> + */
>>> + if (priv->devtype_data->features & FLEXCAN_HAS_MECR_FEATURES) {
>>> + /*
>>> + * Follow the protocol as described in "Detection
>>> + * and Correction of Memory Errors" to write to
>>> + * MECR register
>>> + */
>>
>>
>> One nitpick regarding the comment. In driver/net, the preferred
>> multi-line comment style is
>>
>
> Just a few lines above that one, another multi-line comment is written
> with the empty line at start... With this changed to the net comment
> style, we would end up in a mixed style... What do you think?

Okay, keep the commenting style of the driver.

[Stefan Agner]

Reported-by: Ashutosh Singh <ashule...@gmail.com>
Suggested-by: Marc Kleine-Budde <m...@pengutronix.de>
[ste...@agner.ch: added return check for clk_enable_prepare]

Signed-off-by: Stefan Agner <ste...@agner.ch>
---

drivers/net/can/flexcan.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index f425ec2..89745aa 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -383,12 +383,26 @@ static int flexcan_get_berr_counter(const struct net_device *dev,

{
const struct flexcan_priv *priv = netdev_priv(dev);
struct flexcan_regs __iomem *regs = priv->base;
- u32 reg = flexcan_read(&regs->ecr);
+ u32 reg, err;
+
+ err = clk_prepare_enable(priv->clk_ipg);
+ if (err)
+ return err;
+
+ err = clk_prepare_enable(priv->clk_per);
+ if (err)
+ goto out_disable_ipg;
+
+ reg = flexcan_read(&regs->ecr);

bec->txerr = (reg >> 0) & 0xff;
bec->rxerr = (reg >> 8) & 0xff;

- return 0;

+ clk_disable_unprepare(priv->clk_per);
+ out_disable_ipg:
+ clk_disable_unprepare(priv->clk_ipg);
+

+ return err;
}

static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev)
--
2.0.1

[Stefan Agner]

Extend FlexCAN driver to support Vybrid. Vybrids variant of the IP
has ECC support which is controlled through the memory error
control register (MECR). There is also an errata which leads to
false positive error detections (ID e5295). This patch disables
the memory error detection completely.

Signed-off-by: Stefan Agner <ste...@agner.ch>
---

drivers/net/can/flexcan.c | 73 +++++++++++++++++++++++++++++++++++++++++------
1 file changed, 64 insertions(+), 9 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 89745aa..1c31a5d 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c

struct flexcan_priv *priv = netdev_priv(dev);
struct flexcan_regs __iomem *regs = priv->base;

int err;
- u32 reg_mcr, reg_ctrl;
+ u32 reg_mcr, reg_ctrl, reg_crl2, reg_mecr;

/* enable module */
err = flexcan_chip_enable(priv);
@@ -884,6 +913,31 @@ static int flexcan_chip_start(struct net_device *dev)

if (priv->devtype_data->features & FLEXCAN_HAS_V10_FEATURES)
flexcan_write(0x0, &regs->rxfgmask);

+ /*
+ * On Vybrid, disable memory error detection interrupts
+ * and freeze mode.
+ * This also works around errata e5295 which generates
+ * false positive memory errors and put the device in
+ * freeze mode.
+ */
+ if (priv->devtype_data->features & FLEXCAN_HAS_MECR_FEATURES) {
+ /*
+ * Follow the protocol as described in "Detection
+ * and Correction of Memory Errors" to write to
+ * MECR register
+ */

+ reg_crl2 = flexcan_read(&regs->crl2);
+ reg_crl2 |= FLEXCAN_CRL2_ECRWRE;
+ flexcan_write(reg_crl2, &regs->crl2);
+
+ reg_mecr = flexcan_read(&regs->mecr);
+ reg_mecr &= ~FLEXCAN_MECR_ECRWRDIS;
+ flexcan_write(reg_mecr, &regs->mecr);
+ reg_mecr &= ~(FLEXCAN_MECR_NCEFAFRZ | FLEXCAN_MECR_HANCEI_MSK |
+ FLEXCAN_MECR_FANCEI_MSK);
+ flexcan_write(reg_mecr, &regs->mecr);
+ }
+
err = flexcan_transceiver_enable(priv);
if (err)
goto out_chip_disable;
@@ -1094,6 +1148,7 @@ static const struct of_device_id flexcan_of_match[] = {
{ .compatible = "fsl,imx6q-flexcan", .data = &fsl_imx6q_devtype_data, },
{ .compatible = "fsl,imx28-flexcan", .data = &fsl_imx28_devtype_data, },
{ .compatible = "fsl,p1010-flexcan", .data = &fsl_p1010_devtype_data, },
+ { .compatible = "fsl,vf610-flexcan", .data = &fsl_vf610_devtype_data, },
{ /* sentinel */ },
};
MODULE_DEVICE_TABLE(of, flexcan_of_match);

[Stefan Agner]

Extend the clock control for FlexCAN with the second gate which
enable the clocks in the Clock Divider (CCM_CSCDR2) register too.

Signed-off-by: Stefan Agner <ste...@agner.ch>
---

arch/arm/mach-imx/clk-vf610.c | 6 ++++--
include/dt-bindings/clock/vf610-clock.h | 4 +++-
2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-imx/clk-vf610.c b/arch/arm/mach-imx/clk-vf610.c
index 22dc3ee..f964079 100644

--- a/arch/arm/mach-imx/clk-vf610.c
+++ b/arch/arm/mach-imx/clk-vf610.c
@@ -295,8 +295,10 @@ static void __init vf610_clocks_init(struct device_node *ccm_node)

clk[VF610_CLK_ASRC] = imx_clk_gate2("asrc", "ipg_bus", CCM_CCGR4, CCM_CCGRx_CGn(1));

- clk[VF610_CLK_FLEXCAN0] = imx_clk_gate2("flexcan0", "ipg_bus", CCM_CCGR0, CCM_CCGRx_CGn(0));
- clk[VF610_CLK_FLEXCAN1] = imx_clk_gate2("flexcan1", "ipg_bus", CCM_CCGR9, CCM_CCGRx_CGn(4));

+ clk[VF610_CLK_FLEXCAN0_EN] = imx_clk_gate("flexcan0_en", "ipg_bus", CCM_CSCDR2, 11);

+ clk[VF610_CLK_FLEXCAN0] = imx_clk_gate2("flexcan0", "flexcan0_en", CCM_CCGR0, CCM_CCGRx_CGn(0));

+ clk[VF610_CLK_FLEXCAN1_EN] = imx_clk_gate("flexcan1_en", "ipg_bus", CCM_CSCDR2, 12);

+ clk[VF610_CLK_FLEXCAN1] = imx_clk_gate2("flexcan1", "flexcan1_en", CCM_CCGR9, CCM_CCGRx_CGn(4));

clk[VF610_CLK_DMAMUX0] = imx_clk_gate2("dmamux0", "platform_bus", CCM_CCGR0, CCM_CCGRx_CGn(4));
clk[VF610_CLK_DMAMUX1] = imx_clk_gate2("dmamux1", "platform_bus", CCM_CCGR0, CCM_CCGRx_CGn(5));

diff --git a/include/dt-bindings/clock/vf610-clock.h b/include/dt-bindings/clock/vf610-clock.h
index a916029..00953d9 100644
--- a/include/dt-bindings/clock/vf610-clock.h
+++ b/include/dt-bindings/clock/vf610-clock.h
@@ -164,6 +164,8 @@
#define VF610_CLK_DMAMUX1 151
#define VF610_CLK_DMAMUX2 152
#define VF610_CLK_DMAMUX3 153
-#define VF610_CLK_END 154
+#define VF610_CLK_FLEXCAN0_EN 154
+#define VF610_CLK_FLEXCAN1_EN 155
+#define VF610_CLK_END 156

#endif /* __DT_BINDINGS_CLOCK_VF610_H */

[Stefan Agner]

This series adds support for Vybrid (aka vf610) to the FlexCAN
controller and extends the device tree accordingly.

Changes in v3:
- Return error number in flexcan_get_berr_counter if clock enabling fails
- Add the FLEXCANX_EN clocks as new clocks

Changes in v2:
- Split out patch (seperate dt, clocks and driver changes)
- Use spaces for hardware feature flags table
- Remove drive-by change of esdhc register length
- Added related fix which enables clock in berr_counter

Stefan Agner (4):
ARM: dts: vf610: add FlexCAN node
ARM: imx: clk-vf610: fix FlexCAN clock gating
can: flexcan: switch on clocks before accessing ecr register
can: flexcan: add vf610 support for FlexCAN

arch/arm/boot/dts/vf610.dtsi | 23 +++++++++

arch/arm/mach-imx/clk-vf610.c | 6 ++-
drivers/net/can/flexcan.c | 91 +++++++++++++++++++++++++++++----
include/dt-bindings/clock/vf610-clock.h | 4 +-
4 files changed, 110 insertions(+), 14 deletions(-)

[Stefan Agner]

Add FlexCAN node for the two FlexCAN IP instances in Vybrid.

Signed-off-by: Stefan Agner <ste...@agner.ch>
---

[Marc Kleine-Budde]

On 07/15/2014 02:56 PM, Stefan Agner wrote:
> This series adds support for Vybrid (aka vf610) to the FlexCAN
> controller and extends the device tree accordingly.
>
> Changes in v3:
> - Return error number in flexcan_get_berr_counter if clock enabling fails
> - Add the FLEXCANX_EN clocks as new clocks
>
> Changes in v2:
> - Split out patch (seperate dt, clocks and driver changes)
> - Use spaces for hardware feature flags table
> - Remove drive-by change of esdhc register length
> - Added related fix which enables clock in berr_counter
>
> Stefan Agner (4):
> ARM: dts: vf610: add FlexCAN node
> ARM: imx: clk-vf610: fix FlexCAN clock gating
> can: flexcan: switch on clocks before accessing ecr register
> can: flexcan: add vf610 support for FlexCAN
>
> arch/arm/boot/dts/vf610.dtsi | 23 +++++++++
> arch/arm/mach-imx/clk-vf610.c | 6 ++-
> drivers/net/can/flexcan.c | 91 +++++++++++++++++++++++++++++----
> include/dt-bindings/clock/vf610-clock.h | 4 +-
> 4 files changed, 110 insertions(+), 14 deletions(-)

I'm taking patch 3+4 via the linux-can-next tree.

Thanks,

[Lothar Waßmann]

Hi,

flexcan_get_berr_counter() may be called from interrupt context and
thus must not call any functions that can sleep.
Compiling the driver with CONFIG_DEBUG_ATOMIC_SLEEP would catch this!


Lothar Waßmann
--
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | in...@karo-electronics.de
___________________________________________________________

[Marc Kleine-Budde]

It's called from the NAPI softirq. I'll prepare a patch.

[Marc Kleine-Budde]

On 07/15/2014 02:56 PM, Stefan Agner wrote:

^^ ^^

Can you check the datasheet if the flexcan core has a "Glitch Filter
Width Register (FLEXCANx_GFWR)"

Can you check if the core generates a warning interrupt with the current
setup, if you don't switch on bus error reporting? This means internally
the [TR]WRN_INT is connected and works as specified.

[Shawn Guo]

On Tue, Jul 15, 2014 at 02:56:17PM +0200, Stefan Agner wrote:
> Stefan Agner (4):
> ARM: dts: vf610: add FlexCAN node
> ARM: imx: clk-vf610: fix FlexCAN clock gating

Applied these two, thanks.

> can: flexcan: switch on clocks before accessing ecr register
> can: flexcan: add vf610 support for FlexCAN

[Stefan Agner]

Am 2014-07-15 16:24, schrieb Marc Kleine-Budde:
<snip>

>> @@ -150,18 +171,20 @@
>> * FLEXCAN hardware feature flags
>> *
>> * Below is some version info we got:
>> - * SOC Version IP-Version Glitch- [TR]WRN_INT
>> - * Filter? connected?
>> - * MX25 FlexCAN2 03.00.00.00 no no
>> - * MX28 FlexCAN2 03.00.04.00 yes yes
>> - * MX35 FlexCAN2 03.00.00.00 no no
>> - * MX53 FlexCAN2 03.00.00.00 yes no
>> - * MX6s FlexCAN3 10.00.12.00 yes yes
>> + * SOC Version IP-Version Glitch- [TR]WRN_INT Memory err
>> + * Filter? connected? detection
>> + * MX25 FlexCAN2 03.00.00.00 no no no
>> + * MX28 FlexCAN2 03.00.04.00 yes yes no
>> + * MX35 FlexCAN2 03.00.00.00 no no no
>> + * MX53 FlexCAN2 03.00.00.00 yes no no
>> + * MX6s FlexCAN3 10.00.12.00 yes yes no
>> + * VF610 FlexCAN3 ? no no yes
> ^^ ^^
>
> Can you check the datasheet if the flexcan core has a "Glitch Filter
> Width Register (FLEXCANx_GFWR)"

There is no such register called GFWR/Glitch Filter or similar.

> Can you check if the core generates a warning interrupt with the current
> setup, if you don't switch on bus error reporting? This means internally
> the [TR]WRN_INT is connected and works as specified.

Ok, so I disabled TWRNMSK (Bit 11) in the control register and printed
out the error and status register (ESR1), this is what I get:
[ 191.285295] flexcan_irq, esr=00040080
[ 191.288996] flexcan_irq, ctrl=17092051

Bit 17 (TWRNINT) is not set while TWRNMSK is disabled. Hence [TR]WRN_INT
is not connected?


--
Stefan

[Stefan Agner]

Ping. Anything open/to do from my side?

[Marc Kleine-Budde]

Please keep the printing of esr and ctrl in the interrupt handler, add a
#define DEBUG in the driver, but do not change anything else. Then:

start the driver as usual:
- configure bitrate
- ifconfig can0 up
- _do_not_ enable bit error reporint
- start candump from gitorious with:
candump -e any,0:0,#FFFFFFFF
- short circuit CAN-High and CAN-Low
- send a CAN frame

Another test is to setup a proper CAN bus, but configure the a second
CAN node with a different bitrate. Start the can0 on vf610 as usual,
then candump -e any,0:0,#FFFFFFFF, but do not send any CAN frames on the
vf610. Then on the other system keep sending CAN frames, you might have
to restart the CAN on the second system....

Please send the outputs of candump and demsg to the list. It should
generate a warning, error passive and finally a busoff message.

[Stefan Agner]

Ok, my changes look like this:

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 1c31a5d..fe8b81c 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -19,6 +19,7 @@
*
*/

+#define DEBUG
#include <linux/netdevice.h>
#include <linux/can.h>
#include <linux/can/dev.h>
@@ -743,6 +744,9 @@ static irqreturn_t flexcan_irq(int irq, void
*dev_id)

reg_iflag1 = flexcan_read(&regs->iflag1);
reg_esr = flexcan_read(&regs->esr);
+
+ printk("flexcan_irq, esr=%08x\n", reg_esr);
+ printk("flexcan_irq, ctrl=%08x\n", flexcan_read(&regs->ctrl));
/* ACK all bus error and state change IRQ sources */
if (reg_esr & FLEXCAN_ESR_ALL_INT)
flexcan_write(reg_esr & FLEXCAN_ESR_ALL_INT,
&regs->esr);
@@ -885,8 +889,8 @@ static int flexcan_chip_start(struct net_device
*dev)
*/
reg_ctrl = flexcan_read(&regs->ctrl);
reg_ctrl &= ~FLEXCAN_CTRL_TSYN;
- reg_ctrl |= FLEXCAN_CTRL_BOFF_REC | FLEXCAN_CTRL_LBUF |
- FLEXCAN_CTRL_ERR_STATE;
+ reg_ctrl |= FLEXCAN_CTRL_BOFF_REC | FLEXCAN_CTRL_LBUF;// |
+ //FLEXCAN_CTRL_ERR_STATE;
/*
* enable the "error interrupt" (FLEXCAN_CTRL_ERR_MSK),
* on most Flexcan cores, too. Otherwise we don't get

I'm not sure whether you really want to keep the FLEXCAN_CTRL_ERR_STATE
commented out...

> start the driver as usual:
> - configure bitrate
> - ifconfig can0 up
> - _do_not_ enable bit error reporint
> - start candump from gitorious with:
> candump -e any,0:0,#FFFFFFFF
> - short circuit CAN-High and CAN-Low
> - send a CAN frame

root@colibri-vf:~# ip link set can0 type can bitrate 500000
[ 146.140862] flexcan 40020000.flexcan can0: bitrate error 0.7%
root@colibri-vf:~# ip link set can0 up
[ 146.661430] flexcan 40020000.flexcan can0: writing ctrl=0x17092001
[ 146.667902] flexcan 40020000.flexcan can0: flexcan_set_bittiming:
mcr=0x5980000f ctrl=0x17092001
[ 146.676790] flexcan 40020000.flexcan can0: flexcan_chip_start:
writing mcr=0x79a20208
[ 146.684709] flexcan 40020000.flexcan can0: flexcan_chip_start:
writing ctrl=0x17092051
[ 146.698228] flexcan 40020000.flexcan can0: flexcan_chip_start:
reading mcr=0x60a20208 ctrl=0x1709205

# cansend can0 1F334455#1122334455667788
interface = can0, family = 29, type = 3, proto =

Nothing happens on candump.

> Another test is to setup a proper CAN bus, but configure the a second
> CAN node with a different bitrate. Start the can0 on vf610 as usual,
> then candump -e any,0:0,#FFFFFFFF, but do not send any CAN frames on the
> vf610. Then on the other system keep sending CAN frames, you might have
> to restart the CAN on the second system....

I'm using a PCAN-USB from Peak System for this test. When running with
the same bitrates on both sides (500000, things work smoothly as
expected:

# ip link set can0 type can bitrate 500000
# ip link set can0 up
# cansend can0 1F334455#1122334455667788

root@colibri-vf:~# ./candump -e any,0:0,#FFFFFFFF
can0 1F334455 [8] 11 22 33 44 55 66 77 88

dmesg:
[ 541.772502] flexcan_irq, esr=00040180
[ 541.776207] flexcan_irq, ctrl=17092051

Then I reconfigure the system on my host:
# ip link set can0 down
# ip link set can0 type can bitrate 1000000
# ip link set can0 up
# cansend can0 1F334455#1122334455667788

Nothing happens on Vybrid side.


However, I got once this Kernel panic after I reconfigured to normal
mode. But I could not reproduce this.

[ 461.954394] flexcan_irq, esr=00059d82
[ 461.958093] flexcan_irq, ctrl=17092051
[ 461.961873] ------------[ cut here ]------------
[ 461.966536] WARNING: CPU: 0 PID: 0 at kernel/locking/mutex.c:826
mutex_trylock+0x1b0/0x208()
[ 461.974982] DEBUG_LOCKS_WARN_ON(in_interrupt())
[ 461.979347] Modules linked in:
[ 461.982621] CPU: 0 PID: 0 Comm: swapper/0 Not tainted
3.16.0-rc4-00017-ge9a974f-dirty #215
[ 461.990896] Backtrace:
[ 461.993412] [<80011e1c>] (dump_backtrace) from [<80011fb8>]
(show_stack+0x18/0x1c)
[ 462.000991] r6:806718ec r5:00000000 r4:00000000 r3:00000000
[ 462.006836] [<80011fa0>] (show_stack) from [<8066e160>]
(dump_stack+0x88/0xa4)
[ 462.014143] [<8066e0d8>] (dump_stack) from [<80028f48>]
(warn_slowpath_common+0x70/0x94)
[ 462.022280] r5:00000009 r4:808f9d50
[ 462.025920] [<80028ed8>] (warn_slowpath_common) from [<80029010>]
(warn_slowpath_fmt+0x38/0x40)
[ 462.034666] r8:808f9e14 r7:8110cf7c r6:804c2fa0 r5:8e9ee000
r4:8094cdcc
[ 462.041478] [<80028fdc>] (warn_slowpath_fmt) from [<806718ec>]
(mutex_trylock+0x1b0/0x208)
[ 462.049792] r3:807e3f6c r2:807e1b8c
[ 462.053466] [<8067173c>] (mutex_trylock) from [<804c2fa0>]
(clk_prepare_lock+0x14/0xec)
[ 462.061479] r7:90880000 r6:8e81f800 r5:8e9ee000 r4:8e81f800
[ 462.067280] [<804c2f8c>] (clk_prepare_lock) from [<804c50d0>]
(clk_prepare+0x14/0x2c)
[ 462.075158] r6:8e81f800 r5:8e9ee000 r4:8e81f800 r3:00000000
[ 462.080918] [<804c50bc>] (clk_prepare) from [<803e7a50>]
(flexcan_get_berr_counter+0x24/0xd0)
[ 462.089487] r4:00000001 r3:00000000
[ 462.093156] [<803e7a2c>] (flexcan_get_berr_counter) from [<803e895c>]
(flexcan_poll+0x40c/0x58c)
[ 462.101992] r8:0000000a r7:0000000a r6:8e372388 r5:8e172a80
r4:00000001 r3:00000000
[ 462.109843] [<803e8550>] (flexcan_poll) from [<80511f90>]
(net_rx_action+0xcc/0x1b4)
[ 462.117630] r10:8e9ee6c0 r9:00000003 r8:0000000a r7:8fdde188
r6:808fa0c0 r5:8fdde180
[ 462.125587] r4:0000012c
[ 462.128164] [<80511ec4>] (net_rx_action) from [<8002d290>]
(__do_softirq+0x120/0x26c)
[ 462.136038] r10:808fa080 r9:00000003 r8:00000100 r7:00000003
r6:808f8000 r5:808fa08c
[ 462.143994] r4:00000000
[ 462.146566] [<8002d170>] (__do_softirq) from [<8002d6d4>]
(irq_exit+0xb0/0x104)
[ 462.153923] r10:806788ac r9:808f8000 r8:00000000 r7:0000005a
r6:808f8000 r5:808f5e2c
[ 462.161877] r4:808f8000
[ 462.164459] [<8002d624>] (irq_exit) from [<8000f4bc>]
(handle_IRQ+0x58/0xb8)
[ 462.171520] r4:80900d2c r3:000000a0
[ 462.175200] [<8000f464>] (handle_IRQ) from [<800086c0>]
(gic_handle_irq+0x30/0x68)
[ 462.182818] r8:8095c587 r7:90802100 r6:808f9f28 r5:80900ea0
r4:9080210c r3:000000a0
[ 462.190671] [<80008690>] (gic_handle_irq) from [<80012ae4>]
(__irq_svc+0x44/0x5c)
[ 462.198203] Exception stack(0x808f9f28 to 0x808f9f70)
[ 462.203315] 9f20: 00000001 00000001 00000000
80904070 8090098c 80900938
[ 462.211517] 9f40: 8095c587 00000000 8095c587 808f8000 806788ac
808f9f7c 808f9f40 808f9f70
[ 462.219746] 9f60: 80066a98 8000f834 20000013 ffffffff
[ 462.224852] r7:808f9f5c r6:ffffffff r5:20000013 r4:8000f834
[ 462.230621] [<8000f80c>] (arch_cpu_idle) from [<8005fba4>]
(cpu_startup_entry+0x104/0x16c)
[ 462.238964] [<8005faa0>] (cpu_startup_entry) from [<80668cf8>]
(rest_init+0xb0/0xd8)
[ 462.246757] r7:8ffffcc0 r3:00000000
[ 462.250419] [<80668c48>] (rest_init) from [<808a5bf8>]
(start_kernel+0x340/0x3ac)
[ 462.257979] r5:8095c7c0 r4:80900a38
[ 462.261620] [<808a58b8>] (start_kernel) from [<80008074>]
(0x80008074)
[ 462.268204] ---[ end trace c9c9dee0ce2272a7 ]---
[ 462.272899] flexcan 40020000.flexcan can0: Error Warning IRQ

candump showed this:
can0 20000004 [8] 00 04 00 00 00 00 00 00 ERRORFRAME
controller-problem{rx-error-warning}

>
> Please send the outputs of candump and demsg to the list. It should
> generate a warning, error passive and finally a busoff message.
>

[Marc Kleine-Budde]

On 07/28/2014 06:20 PM, Stefan Agner wrote:
>>> Ping. Anything open/to do from my side?
>>
>> Please keep the printing of esr and ctrl in the interrupt handler, add a
>> #define DEBUG in the driver, but do not change anything else. Then:

^^^^^^^^^^^^^^^^^^^^^^^^^^^

>
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index 1c31a5d..fe8b81c 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -19,6 +19,7 @@
> *
> */
>
> +#define DEBUG

keep

> #include <linux/netdevice.h>
> #include <linux/can.h>
> #include <linux/can/dev.h>
> @@ -743,6 +744,9 @@ static irqreturn_t flexcan_irq(int irq, void
> *dev_id)
>
> reg_iflag1 = flexcan_read(&regs->iflag1);
> reg_esr = flexcan_read(&regs->esr);
> +
> + printk("flexcan_irq, esr=%08x\n", reg_esr);
> + printk("flexcan_irq, ctrl=%08x\n", flexcan_read(&regs->ctrl));

keep

> /* ACK all bus error and state change IRQ sources */
> if (reg_esr & FLEXCAN_ESR_ALL_INT)
> flexcan_write(reg_esr & FLEXCAN_ESR_ALL_INT,
> &regs->esr);
> @@ -885,8 +889,8 @@ static int flexcan_chip_start(struct net_device
> *dev)
> */
> reg_ctrl = flexcan_read(&regs->ctrl);
> reg_ctrl &= ~FLEXCAN_CTRL_TSYN;
> - reg_ctrl |= FLEXCAN_CTRL_BOFF_REC | FLEXCAN_CTRL_LBUF |
> - FLEXCAN_CTRL_ERR_STATE;
> + reg_ctrl |= FLEXCAN_CTRL_BOFF_REC | FLEXCAN_CTRL_LBUF;// |
> + //FLEXCAN_CTRL_ERR_STATE;
> /*
> * enable the "error interrupt" (FLEXCAN_CTRL_ERR_MSK),
> * on most Flexcan cores, too. Otherwise we don't get
>
> I'm not sure whether you really want to keep the FLEXCAN_CTRL_ERR_STATE
> commented out...

No, please remove this change and redo the test.

[Marc Kleine-Budde]

On 07/28/2014 06:20 PM, Stefan Agner wrote:

[...]

> However, I got once this Kernel panic after I reconfigured to normal
> mode. But I could not reproduce this.
>
> [ 461.954394] flexcan_irq, esr=00059d82
> [ 461.958093] flexcan_irq, ctrl=17092051
> [ 461.961873] ------------[ cut here ]------------
> [ 461.966536] WARNING: CPU: 0 PID: 0 at kernel/locking/mutex.c:826
> mutex_trylock+0x1b0/0x208()
> [ 461.974982] DEBUG_LOCKS_WARN_ON(in_interrupt())

Please use this updated version of your patch:

Marc

---

From b6b8ae7cdb04f1a3b9d3a6dd83f8146bfaf48553 Mon Sep 17 00:00:00 2001
From: Stefan Agner <ste...@agner.ch>
Date: Tue, 15 Jul 2014 14:56:20 +0200
Subject: [PATCH] can: flexcan: flexcan_get_berr_counter(): switch on clocks
before accessing ecr register

The funcion flexcan_get_berr_counter() may be called from userspace even if the
interface is down, this the clocks are disabled. This patch switches on the
clocks before accessing the ecr register.

Reported-by: Ashutosh Singh <ashule...@gmail.com>
Signed-off-by: Stefan Agner <ste...@agner.ch>
Signed-off-by: Marc Kleine-Budde <m...@pengutronix.de>
---
drivers/net/can/flexcan.c | 30 +++++++++++++++++++++++++++---
1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index f425ec2..6bfe24a 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -378,8 +378,9 @@ static int flexcan_chip_softreset(struct flexcan_priv *priv)
return 0;
}

-static int flexcan_get_berr_counter(const struct net_device *dev,
- struct can_berr_counter *bec)
+
+static int __flexcan_get_berr_counter(const struct net_device *dev,
+ struct can_berr_counter *bec)

{
const struct flexcan_priv *priv = netdev_priv(dev);
struct flexcan_regs __iomem *regs = priv->base;

@@ -391,6 +392,29 @@ static int flexcan_get_berr_counter(const struct net_device *dev,
return 0;
}

+static int flexcan_get_berr_counter(const struct net_device *dev,
+ struct can_berr_counter *bec)
+{
+ const struct flexcan_priv *priv = netdev_priv(dev);
+ int err;

+
+ err = clk_prepare_enable(priv->clk_ipg);
+ if (err)
+ return err;
+
+ err = clk_prepare_enable(priv->clk_per);
+ if (err)
+ goto out_disable_ipg;
+

+ err = __flexcan_get_berr_counter(dev, bec);
+

+ clk_disable_unprepare(priv->clk_per);
+ out_disable_ipg:
+ clk_disable_unprepare(priv->clk_ipg);
+

+ return err;
+}
+

static int flexcan_start_xmit(struct sk_buff *skb, struct net_device *dev)

{
const struct flexcan_priv *priv = netdev_priv(dev);

@@ -503,7 +527,7 @@ static void do_state(struct net_device *dev,

struct flexcan_priv *priv = netdev_priv(dev);

struct can_berr_counter bec;

- flexcan_get_berr_counter(dev, &bec);
+ __flexcan_get_berr_counter(dev, &bec);

switch (priv->can.state) {
case CAN_STATE_ERROR_ACTIVE:
--
2.0.1

[Stefan Agner]

Am 2014-07-28 18:28, schrieb Marc Kleine-Budde:
> On 07/28/2014 06:20 PM, Stefan Agner wrote:
>> I'm not sure whether you really want to keep the FLEXCAN_CTRL_ERR_STATE
>> commented out...
>
> No, please remove this change and redo the test.
>

Ok, removed that change and did the tests again:

== Wrong Bitrate test

[ 146.485022] flexcan 40020000.flexcan can0: bitrate error 0.7%
[ 148.401793] flexcan 40020000.flexcan can0: writing ctrl=0x17092001
[ 148.408263] flexcan 40020000.flexcan can0: flexcan_set_bittiming:
mcr=0x5980000f ctrl=0x17092001
[ 148.408298] flexcan 40020000.flexcan can0: flexcan_chip_start:
writing mcr=0x79a20208
[ 148.408328] flexcan 40020000.flexcan can0: flexcan_chip_start:
writing ctrl=0x1709ac51
[ 148.414373] flexcan 40020000.flexcan can0: flexcan_chip_start:
reading mcr=0x60a20208 ctrl=0x1709ac51
^---- Initialization
[ 152.968685] flexcan_irq, esr=00040080
[ 152.972386] flexcan_irq, ctrl=1709ac51
[ 155.488623] flexcan_irq, esr=00040080
[ 155.492326] flexcan_irq, ctrl=1709ac51
^---- Two messages with right bitrate
[ 171.014124] flexcan_irq, esr=00058d0a
[ 171.017823] flexcan_irq, ctrl=1709ac51
^---- One message with wrong bitrate
[ 171.021631] flexcan 40020000.flexcan can0: Error Warning IRQ
[ 171.021660] flexcan 40020000.flexcan can0: Error Passive IRQ

# ./candump -e any,0:0,#FFFFFFFF
can0 1F334455 [8] 11 22 33 44 55 66 77 88
can0 1F334455 [8] 11 22 33 44 55 66 77 88

can0 20000004 [8] 00 10 00 00 00 00 00 00 ERRORFRAME
controller-problem{rx-error-passive}

== Loopback test

[ 464.003776] flexcan 40020000.flexcan can0: writing ctrl=0x17092051
[ 464.010005] flexcan 40020000.flexcan can0: flexcan_set_bittiming:
mcr=0x5980000f ctrl=0x17092051
[ 464.010035] flexcan 40020000.flexcan can0: flexcan_chip_start:
writing mcr=0x79a20208
[ 464.010064] flexcan 40020000.flexcan can0: flexcan_chip_start:
writing ctrl=0x1709ac51
[ 464.010199] flexcan 40020000.flexcan can0: flexcan_chip_start:
reading mcr=0x60a20208 ctrl=0x1709ac51
[ 464.011682] IPv6: ADDRCONF(NETDEV_CHANGE): can0: link becomes ready
[ 470.970133] flexcan_irq, esr=00064252
[ 470.974833] flexcan_irq, ctrl=1709ac51
[ 470.980171] flexcan 40020000.flexcan can0: Error Warning IRQ
[ 470.980203] flexcan 40020000.flexcan can0: Error Passive IRQ
[ 470.980328] flexcan_irq, esr=00000034
[ 470.984998] flexcan_irq, ctrl=1709ac51

# ./candump -e any,0:0,#FFFFFFFF

can0 20000044 [8] 00 10 00 00 00 00 00 00 ERRORFRAME
controller-problem{rx-error-passive}
bus-off

[Marc Kleine-Budde]

Thanks for the test, so far looks promising :) With this setup the other
CAN node repeats the CAN frame until it's ACKed. Because there is no
node with a compatible bitrate, there is no ACking CAN node.

Can you add a third CAN node to the network. The second and third node
should use the same bitrate, while your vf610 uses a different one. With
the new setup it should take more than one frame until the vf610 goes
into error warning and even more frames to error passive. This way we
can see it the error warning interrupt is connected or not. The error
counters should increase with each "wrong" bitrate frame it sees, you
can check with:

ip -details link show can0

The output looks like this:

> 4: can0: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN mode DEFAULT qlen 10
> link/can
> can state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 0
^^^^^^^^^^^^^^^^^^^^^^
> bitrate 1000000 sample-point 0.750
> tq 125 prop-seg 2 phase-seg1 3 phase-seg2 2 sjw 1
> sja1000: tseg1 1..16 tseg2 1..8 sjw 1..4 brp 1..64 brp-inc 1
> clock 8000000

When one of the berr-counter crosses 96 (and stays below 128) a warning
interrupt should be generated.

[Stefan Agner]

Ok, created this setup, could successfully communicate with all three
nodes. I then set the Vybrid to half of the bitrate. When I send a frame
from Vybrid, the berr-counter tx immediately ends up at 128 and the
device is in ERROR-PASSIVE:

root@colibri-vf:~# ip -details link show can1
3: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN
mode DEFAULT group default qlen 10
link/can promiscuity 0

can state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 0

bitrate 124990 sample-point 0.739
tq 347 prop-seg 8 phase-seg1 8 phase-seg2 6 sjw 1
flexcan: tseg1 4..16 tseg2 2..8 sjw 1..4 brp 1..256 brp-inc 1
clock 83368421
root@colibri-vf:~# cansend can1 1F334455#1122334455667788
interface = can1, family = 29, type = 3, proto = 1
root@colibri-vf:~# [ 818.886664] flexcan_irq, esr=00062242
[ 818.890365] flexcan_irq, ctrl=1c3dac57

root@colibri-vf:~# ip -details link show can1
3: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN
mode DEFAULT group default qlen 10
link/can promiscuity 0
can state ERROR-PASSIVE (berr-counter tx 128 rx 0) restart-ms 0
bitrate 124990 sample-point 0.739
tq 347 prop-seg 8 phase-seg1 8 phase-seg2 6 sjw 1
flexcan: tseg1 4..16 tseg2 2..8 sjw 1..4 brp 1..256 brp-inc 1
clock 83368421


When I send the frames from another device on the bus, I can see the rx
count incrementing by one on each frame I send. As you expected, the
device changes to ERROR-WARNING when crossing the 96 frame boundary:

root@colibri-vf:~# ip -details link show can1
3: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN
mode DEFAULT group default qlen 10
link/can promiscuity 0
can state ERROR-ACTIVE (berr-counter tx 0 rx 92) restart-ms 0
bitrate 124990 sample-point 0.739
tq 347 prop-seg 8 phase-seg1 8 phase-seg2 6 sjw 1
flexcan: tseg1 4..16 tseg2 2..8 sjw 1..4 brp 1..256 brp-inc 1
clock 83368421
root@colibri-vf:~# [ 448.331150] flexcan_irq, esr=0005050a
[ 448.334851] flexcan_irq, ctrl=1c3dac57
ip -details link show can1
3: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN
mode DEFAULT group default qlen 10
link/can promiscuity 0
can state ERROR-WARNING (berr-counter tx 0 rx 102) restart-ms 0
bitrate 124990 sample-point 0.739
tq 347 prop-seg 8 phase-seg1 8 phase-seg2 6 sjw 1
flexcan: tseg1 4..16 tseg2 2..8 sjw 1..4 brp 1..256 brp-inc 1
clock 83368421

However, once reaching 128, I don't get another interrupt and the device
stays in ERROR-WARNING:

3: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN
mode DEFAULT group default qlen 10
link/can promiscuity 0
can state ERROR-WARNING (berr-counter tx 0 rx 128) restart-ms 0
bitrate 124990 sample-point 0.739
tq 347 prop-seg 8 phase-seg1 8 phase-seg2 6 sjw 1
flexcan: tseg1 4..16 tseg2 2..8 sjw 1..4 brp 1..256 brp-inc 1
clock 83368421

Is this the expected behavior on receive?

[Marc Kleine-Budde]

On 08/04/2014 03:43 PM, Stefan Agner wrote:
[...]

This is expected.

> root@colibri-vf:~# ip -details link show can1
> 3: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN
> mode DEFAULT group default qlen 10
> link/can promiscuity 0
> can state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 0
> bitrate 124990 sample-point 0.739
> tq 347 prop-seg 8 phase-seg1 8 phase-seg2 6 sjw 1
> flexcan: tseg1 4..16 tseg2 2..8 sjw 1..4 brp 1..256 brp-inc 1
> clock 83368421

^^^^^^^^

BTW: the can core has a really weird clock rate, have a look at the
datasheet if you manage to route a 24 MHz clock (or another multiple of
8MHz) to the flexcan core. I had a quick glance at the datasheet, if I
understand it correctly the Fast OSC clock runs with 24 MHz.

> root@colibri-vf:~# cansend can1 1F334455#1122334455667788
> interface = can1, family = 29, type = 3, proto = 1
> root@colibri-vf:~# [ 818.886664] flexcan_irq, esr=00062242
> [ 818.890365] flexcan_irq, ctrl=1c3dac57
>
> root@colibri-vf:~# ip -details link show can1
> 3: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN
> mode DEFAULT group default qlen 10
> link/can promiscuity 0
> can state ERROR-PASSIVE (berr-counter tx 128 rx 0) restart-ms 0
> bitrate 124990 sample-point 0.739
> tq 347 prop-seg 8 phase-seg1 8 phase-seg2 6 sjw 1
> flexcan: tseg1 4..16 tseg2 2..8 sjw 1..4 brp 1..256 brp-inc 1
> clock 83368421
>
>
> When I send the frames from another device on the bus, I can see the rx
> count incrementing by one on each frame I send. As you expected, the
> device changes to ERROR-WARNING when crossing the 96 frame boundary:

This is correct

> root@colibri-vf:~# ip -details link show can1
> 3: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN
> mode DEFAULT group default qlen 10
> link/can promiscuity 0
> can state ERROR-ACTIVE (berr-counter tx 0 rx 92) restart-ms 0
> bitrate 124990 sample-point 0.739
> tq 347 prop-seg 8 phase-seg1 8 phase-seg2 6 sjw 1
> flexcan: tseg1 4..16 tseg2 2..8 sjw 1..4 brp 1..256 brp-inc 1
> clock 83368421
> root@colibri-vf:~# [ 448.331150] flexcan_irq, esr=0005050a
> [ 448.334851] flexcan_irq, ctrl=1c3dac57
> ip -details link show can1
> 3: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN
> mode DEFAULT group default qlen 10
> link/can promiscuity 0
> can state ERROR-WARNING (berr-counter tx 0 rx 102) restart-ms 0
> bitrate 124990 sample-point 0.739
> tq 347 prop-seg 8 phase-seg1 8 phase-seg2 6 sjw 1
> flexcan: tseg1 4..16 tseg2 2..8 sjw 1..4 brp 1..256 brp-inc 1
> clock 83368421

> However, once reaching 128, I don't get another interrupt and the device
> stays in ERROR-WARNING:

The contents of the esr reg would be interesting, especially the
FLT_CONF part.

> 3: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN
> mode DEFAULT group default qlen 10
> link/can promiscuity 0
> can state ERROR-WARNING (berr-counter tx 0 rx 128) restart-ms 0
> bitrate 124990 sample-point 0.739
> tq 347 prop-seg 8 phase-seg1 8 phase-seg2 6 sjw 1
> flexcan: tseg1 4..16 tseg2 2..8 sjw 1..4 brp 1..256 brp-inc 1
> clock 83368421
>
> Is this the expected behavior on receive?

No, it should go into error passive if one of the error counters have >
127. Maybe this is an error onthe vf610, maybe it's a general flexcan
problem.

[Stefan Agner]

This pinmux is actually part of the IPs CTRL register (see CLKSRC).
Currently this is set unconditionally to peripheral clock, which is on
my device 83.3 MHz (500MHz/6). This would be a bit bigger change. Just
thinking how to implement this. Maybe we have to reference a third clock
"osc" and a device tree property fsl,can-clock-osc which one can choose
between "osc" and "per" what do you think?

>> root@colibri-vf:~# cansend can1 1F334455#1122334455667788
>> interface = can1, family = 29, type = 3, proto = 1
>> root@colibri-vf:~# [ 818.886664] flexcan_irq, esr=00062242
>> [ 818.890365] flexcan_irq, ctrl=1c3dac57
>>
>> root@colibri-vf:~# ip -details link show can1
>> 3: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN
>> mode DEFAULT group default qlen 10
>> link/can promiscuity 0
>> can state ERROR-PASSIVE (berr-counter tx 128 rx 0) restart-ms 0
>> bitrate 124990 sample-point 0.739
>> tq 347 prop-seg 8 phase-seg1 8 phase-seg2 6 sjw 1
>> flexcan: tseg1 4..16 tseg2 2..8 sjw 1..4 brp 1..256 brp-inc 1
>> clock 83368421
>>
>>
>> When I send the frames from another device on the bus, I can see the rx
>> count incrementing by one on each frame I send. As you expected, the
>> device changes to ERROR-WARNING when crossing the 96 frame boundary:
>
> This is correct
>

Just found out when using too high bitrate (500000 vs 125000), the error
counter immediately goes up to 128 (or even beyond) and ends up in
ERROR-PASSIVE:

# ip -details link show can1

[ 292.164820] flexcan_get_berr_counter, esr=00000000
[ 292.169715] flexcan_get_berr_counter, esr=00040190

3: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN
mode DEFAULT group default qlen 10
link/can promiscuity 0

can state ERROR-PASSIVE (berr-counter tx 0 rx 135) restart-ms 0
bitrate 298811 sample-point 0.777
tq 371 prop-seg 3 phase-seg1 3 phase-seg2 2 sjw 1

root@vybridvf61-v11:~# [ 222.221651] flexcan_irq, esr=0005050a
[ 222.225349] flexcan_irq, ctrl=1c3dac57

ip -details link show can1

[ 223.791082] flexcan_get_berr_counter, esr=00000000
[ 223.796246] flexcan_get_berr_counter, esr=00040180

3: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN
mode DEFAULT group default qlen
10
link/can promiscuity 0

can state ERROR-WARNING (berr-counter tx 0 rx 96) restart-ms 0

bitrate 124990 sample-point 0.739
tq 347 prop-seg 8 phase-seg1 8 phase-seg2 6 sjw 1
flexcan: tseg1 4..16 tseg2 2..8 sjw 1..4 brp 1..256 brp-inc 1
clock 83368421

root@vybridvf61-v11:~# ip -details link show can1
[ 243.571175] flexcan_get_berr_counter, esr=00000000
[ 243.576343] flexcan_get_berr_counter, esr=00040582

3: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN
mode DEFAULT group default qlen
10
link/can promiscuity 0

can state ERROR-WARNING (berr-counter tx 0 rx 104) restart-ms 0

bitrate 124990 sample-point 0.739
tq 347 prop-seg 8 phase-seg1 8 phase-seg2 6 sjw 1
flexcan: tseg1 4..16 tseg2 2..8 sjw 1..4 brp 1..256 brp-inc 1
clock 83368421

..

root@vybridvf61-v11:~# ip -details link show can1
[ 299.831192] flexcan_get_berr_counter, esr=00000000
[ 299.836358] flexcan_get_berr_counter, esr=00040582

3: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN
mode DEFAULT group default qlen 10
link/can promiscuity 0

can state ERROR-WARNING (berr-counter tx 0 rx 126) restart-ms 0

bitrate 124990 sample-point 0.739
tq 347 prop-seg 8 phase-seg1 8 phase-seg2 6 sjw 1
flexcan: tseg1 4..16 tseg2 2..8 sjw 1..4 brp 1..256 brp-inc 1
clock 83368421

root@vybridvf61-v11:~# ip -details link show can1
[ 302.411025] flexcan_get_berr_counter, esr=00000000
[ 302.416195] flexcan_get_berr_counter, esr=00040592

3: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN
mode DEFAULT group default qlen 10
link/can promiscuity 0
can state ERROR-WARNING (berr-counter tx 0 rx 128) restart-ms 0
bitrate 124990 sample-point 0.739
tq 347 prop-seg 8 phase-seg1 8 phase-seg2 6 sjw 1
flexcan: tseg1 4..16 tseg2 2..8 sjw 1..4 brp 1..256 brp-inc 1
clock 83368421

The FLTCONF is in Error Passive, however the stack did not received that
information. I still have the printk in the interrupt, however I did not
get an interrupt here (while I get one when it switched to
ERROR-WARNING... But it looks like the ERRINT is still pending...

>> 3: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN
>> mode DEFAULT group default qlen 10
>> link/can promiscuity 0
>> can state ERROR-WARNING (berr-counter tx 0 rx 128) restart-ms 0
>> bitrate 124990 sample-point 0.739
>> tq 347 prop-seg 8 phase-seg1 8 phase-seg2 6 sjw 1
>> flexcan: tseg1 4..16 tseg2 2..8 sjw 1..4 brp 1..256 brp-inc 1
>> clock 83368421
>>
>> Is this the expected behavior on receive?
>
> No, it should go into error passive if one of the error counters have >
> 127. Maybe this is an error onthe vf610, maybe it's a general flexcan
> problem.
>
> Marc

--

[Marc Kleine-Budde]

Sigh! I've missed the fact, that this bit is _inside_ the CAN core
(again). On the mx53 and imx6 this bit was obsolete and the clock was
selected outside of the CAN core.

> Maybe we have to reference a third clock
> "osc" and a device tree property fsl,can-clock-osc which one can choose
> between "osc" and "per" what do you think?

Yes, but that's a different patch. I thought it would be as easy as on
the mx53, where you just had to reconfigure the clock tree.

>>> root@colibri-vf:~# cansend can1 1F334455#1122334455667788
>>> interface = can1, family = 29, type = 3, proto = 1
>>> root@colibri-vf:~# [ 818.886664] flexcan_irq, esr=00062242
>>> [ 818.890365] flexcan_irq, ctrl=1c3dac57
>>>
>>> root@colibri-vf:~# ip -details link show can1
>>> 3: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN
>>> mode DEFAULT group default qlen 10
>>> link/can promiscuity 0
>>> can state ERROR-PASSIVE (berr-counter tx 128 rx 0) restart-ms 0
>>> bitrate 124990 sample-point 0.739
>>> tq 347 prop-seg 8 phase-seg1 8 phase-seg2 6 sjw 1
>>> flexcan: tseg1 4..16 tseg2 2..8 sjw 1..4 brp 1..256 brp-inc 1
>>> clock 83368421
>>>
>>>
>>> When I send the frames from another device on the bus, I can see the rx
>>> count incrementing by one on each frame I send. As you expected, the
>>> device changes to ERROR-WARNING when crossing the 96 frame boundary:
>>
>> This is correct
>>
>
> Just found out when using too high bitrate (500000 vs 125000), the error
> counter immediately goes up to 128 (or even beyond) and ends up in
> ERROR-PASSIVE:

Maybe there is a off-by-one error in the flexcan core, so that a rx
error of 127 doesn't trigger an error passive interrupt.

> # ip -details link show can1
> [ 292.164820] flexcan_get_berr_counter, esr=00000000
> [ 292.169715] flexcan_get_berr_counter, esr=00040190
> 3: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN
> mode DEFAULT group default qlen 10
> link/can promiscuity 0
> can state ERROR-PASSIVE (berr-counter tx 0 rx 135) restart-ms 0
> bitrate 298811 sample-point 0.777
> tq 371 prop-seg 3 phase-seg1 3 phase-seg2 2 sjw 1
> flexcan: tseg1 4..16 tseg2 2..8 sjw 1..4 brp 1..256 brp-inc 1
> clock 83368421

[...]

> ...

According to the datasheet the ERRINT indicates a bit error (bit 10...15
is set), not the error passive state.

[Stefan Agner]

Agreed.

True, but fact is, I don't get a interrupt when the system switches from
ERROR-WARNING to ERROR-PASSIVE state. However, when I set berr-reporting
on, I get an interrupt (probably because of a change of bit 10...15),
but the state change is recognized:

ip -details link show can1

[ 2303.571656] flexcan_get_berr_counter, esr=00000000
[ 2303.576832] flexcan_get_berr_counter, esr=00040180

3: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN
mode DEFAULT group default qlen 10
link/can promiscuity 0

can <BERR-REPORTING> state ERROR-WARNING (berr-counter tx 0 rx 127)

restart-ms 0
bitrate 124990 sample-point 0.739
tq 347 prop-seg 8 phase-seg1 8 phase-seg2 6 sjw 1
flexcan: tseg1 4..16 tseg2 2..8 sjw 1..4 brp 1..256 brp-inc 1
clock 83368421

root@vybridvf61-v11:~# [ 2307.023789] flexcan_irq, esr=0004051a
[ 2307.027490] flexcan_irq, ctrl=1c3dec57

ip -details link show can1

[ 2309.081840] flexcan_get_berr_counter, esr=00000000
[ 2309.087016] flexcan_get_berr_counter, esr=00040190

3: can1: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN
mode DEFAULT group default qlen 10
link/can promiscuity 0

can <BERR-REPORTING> state ERROR-PASSIVE (berr-counter tx 0 rx 128)

restart-ms 0
bitrate 124990 sample-point 0.739
tq 347 prop-seg 8 phase-seg1 8 phase-seg2 6 sjw 1
flexcan: tseg1 4..16 tseg2 2..8 sjw 1..4 brp 1..256 brp-inc 1
clock 83368421

It looks like, the FLTCONF state change itself doesn't lead to a
interrupt when the state changes to ERROR-PASSIVE. IMHO, this is also
not explicitly stated in the Vybrid RM (46.3.8).

What is the solution here? Force enabling ERR interrupt? The
FLEXCAN_HAS_BROKEN_ERR_STATE flag would be appropriate, however, this
was meant to work around the missing WRN_INT, which seems not to be the
case here...

[Marc Kleine-Budde]

On 07/15/2014 03:54 PM, Marc Kleine-Budde wrote:
> On 07/15/2014 02:56 PM, Stefan Agner wrote:
>> This series adds support for Vybrid (aka vf610) to the FlexCAN
>> controller and extends the device tree accordingly.
>>
>> Changes in v3:
>> - Return error number in flexcan_get_berr_counter if clock enabling fails
>> - Add the FLEXCANX_EN clocks as new clocks
>>
>> Changes in v2:
>> - Split out patch (seperate dt, clocks and driver changes)
>> - Use spaces for hardware feature flags table
>> - Remove drive-by change of esdhc register length
>> - Added related fix which enables clock in berr_counter
>>
>> Stefan Agner (4):
>> ARM: dts: vf610: add FlexCAN node
>> ARM: imx: clk-vf610: fix FlexCAN clock gating
>> can: flexcan: switch on clocks before accessing ecr register
>> can: flexcan: add vf610 support for FlexCAN
>>
>> arch/arm/boot/dts/vf610.dtsi | 23 +++++++++
>> arch/arm/mach-imx/clk-vf610.c | 6 ++-
>> drivers/net/can/flexcan.c | 91 +++++++++++++++++++++++++++++----
>> include/dt-bindings/clock/vf610-clock.h | 4 +-
>> 4 files changed, 110 insertions(+), 14 deletions(-)
>
> I'm taking patch 3+4 via the linux-can-next tree.

Applied to linux-can-next, with the modified version of the 3/4.

[Marc Kleine-Budde]

On 08/04/2014 06:01 PM, Stefan Agner wrote:

> ...

Can you apply Alexander Stein's "[PATCH 2/4] can: flexcan: Detect error
passive state change" and recheck?