from:"Wolfgang Grandegger"

Re: [PATCH v4 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

2017-03-20 Thread Wolfgang Grandegger


Hello Akshay,

Am 20.03.2017 um 16:14 schrieb Akshay Bhat:

Hi Wolfgang,

On 03/19/2017 12:17 PM, Wolfgang Grandegger wrote:

Hello Akshay,

I still see some improvements...

Am 17.03.2017 um 22:10 schrieb Akshay Bhat:

This patch adds support for the Holt HI-311x CAN controller. The HI311x
CAN controller is capable of transmitting and receiving standard data
frames, extended data frames and remote frames. The HI311x interfaces
with the host over SPI.

Datasheet: www.holtic.com/documents/371-hi-3110_v-rev-jpdf.do

Signed-off-by: Akshay Bhat <nod...@gmail.com>
---

v3 -> v4:
Address comments from Wolfgang Grandegger:
- Add support for CAN warning state reporting
- Add support for reporting tx/rx error counts in bus error messages
- Keep bus error interrupts enabled to detect CAN state changes
- Fix bug in restart code after BUSOFF state
- Clean up error handling code

 drivers/net/can/spi/Kconfig  |6 +
 drivers/net/can/spi/Makefile |1 +
 drivers/net/can/spi/hi311x.c | 1072
++
 3 files changed, 1079 insertions(+)
 create mode 100644 drivers/net/can/spi/hi311x.c

diff --git a/drivers/net/can/spi/Kconfig b/drivers/net/can/spi/Kconfig
index 148cae5..8f2e0dd 100644
--- a/drivers/net/can/spi/Kconfig
+++ b/drivers/net/can/spi/Kconfig
@@ -1,6 +1,12 @@
 menu "CAN SPI interfaces"
 depends on SPI

+config CAN_HI311X
+tristate "Holt HI311x SPI CAN controllers"
+depends on CAN_DEV && SPI && HAS_DMA
+---help---
+  Driver for the Holt HI311x SPI CAN controllers.
+
 config CAN_MCP251X
 tristate "Microchip MCP251x SPI CAN controllers"
 depends on HAS_DMA
diff --git a/drivers/net/can/spi/Makefile b/drivers/net/can/spi/Makefile
index 0e86040..f59fa37 100644
--- a/drivers/net/can/spi/Makefile
+++ b/drivers/net/can/spi/Makefile
@@ -3,4 +3,5 @@
 #


+obj-$(CONFIG_CAN_HI311X)+= hi311x.o
 obj-$(CONFIG_CAN_MCP251X)+= mcp251x.o
diff --git a/drivers/net/can/spi/hi311x.c b/drivers/net/can/spi/hi311x.c
new file mode 100644
index 000..2a3d794
--- /dev/null
+++ b/drivers/net/can/spi/hi311x.c
@@ -0,0 +1,1072 @@
+/* CAN bus driver for Holt HI3110 CAN Controller with SPI Interface
+ *
+ * Copyright(C) Timesys Corporation 2016
+ *
+ * Based on Microchip 251x CAN Controller (mcp251x) Linux kernel driver
+ * Copyright 2009 Christian Pellegrin EVOL S.r.l.
+ * Copyright 2007 Raymarine UK, Ltd. All Rights Reserved.
+ * Copyright 2006 Arcom Control Systems Ltd.
+ *
+ * Based on CAN bus driver for the CCAN controller written by
+ * - Sascha Hauer, Marc Kleine-Budde, Pengutronix
+ * - Simon Kallweit, intefo AG
+ * Copyright 2007
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */


... snip...


+
+static irqreturn_t hi3110_can_ist(int irq, void *dev_id)
+{
+struct hi3110_priv *priv = dev_id;
+struct spi_device *spi = priv->spi;
+struct net_device *net = priv->net;
+
+mutex_lock(>hi3110_lock);
+
+while (!priv->force_quit) {
+enum can_state new_state;
+u8 intf, eflag, statf;
+
+while (!(HI3110_STAT_RXFMTY &
+   (statf = hi3110_read(spi, HI3110_READ_STATF {
+hi3110_hw_rx(spi);
+}
+
+intf = hi3110_read(spi, HI3110_READ_INTF);


I think HI3110_READ_ERR is only valid if HI3110_INT_BUSERR is set!
Therefore:

if ((intf & HI3110_INT_BUSERR) {

It saves reading one SPI register in the message processing path. Please
check if back-to-error-active still comes.



The top 3 bits of HI3110_READ_ERR (BUSOFF, TXERRP, RXERRP) are valid
even if HI3110_INT_BUSERR is not set.


I'm confused! If you disable BUSERR interrupts, you do not get the 
status bits any longer, you said. But the manual says: "Bits 4:0 in the 
ERR register can be read to determine the source of the error.", which 
excludes the above bits... but obviously the controller does it that way.


Sorry for the noise,

Wolfgang.

Re: [PATCH v4 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

2017-03-19 Thread Wolfgang Grandegger


Hello Akshay,

I still see some improvements...

Am 17.03.2017 um 22:10 schrieb Akshay Bhat:

This patch adds support for the Holt HI-311x CAN controller. The HI311x
CAN controller is capable of transmitting and receiving standard data
frames, extended data frames and remote frames. The HI311x interfaces
with the host over SPI.

Datasheet: www.holtic.com/documents/371-hi-3110_v-rev-jpdf.do

Signed-off-by: Akshay Bhat <nod...@gmail.com>
---

v3 -> v4:
Address comments from Wolfgang Grandegger:
- Add support for CAN warning state reporting
- Add support for reporting tx/rx error counts in bus error messages
- Keep bus error interrupts enabled to detect CAN state changes
- Fix bug in restart code after BUSOFF state
- Clean up error handling code

 drivers/net/can/spi/Kconfig  |6 +
 drivers/net/can/spi/Makefile |1 +
 drivers/net/can/spi/hi311x.c | 1072 ++
 3 files changed, 1079 insertions(+)
 create mode 100644 drivers/net/can/spi/hi311x.c

diff --git a/drivers/net/can/spi/Kconfig b/drivers/net/can/spi/Kconfig
index 148cae5..8f2e0dd 100644
--- a/drivers/net/can/spi/Kconfig
+++ b/drivers/net/can/spi/Kconfig
@@ -1,6 +1,12 @@
 menu "CAN SPI interfaces"
depends on SPI

+config CAN_HI311X
+   tristate "Holt HI311x SPI CAN controllers"
+   depends on CAN_DEV && SPI && HAS_DMA
+   ---help---
+ Driver for the Holt HI311x SPI CAN controllers.
+
 config CAN_MCP251X
tristate "Microchip MCP251x SPI CAN controllers"
depends on HAS_DMA
diff --git a/drivers/net/can/spi/Makefile b/drivers/net/can/spi/Makefile
index 0e86040..f59fa37 100644
--- a/drivers/net/can/spi/Makefile
+++ b/drivers/net/can/spi/Makefile
@@ -3,4 +3,5 @@
 #


+obj-$(CONFIG_CAN_HI311X)   += hi311x.o
 obj-$(CONFIG_CAN_MCP251X)  += mcp251x.o
diff --git a/drivers/net/can/spi/hi311x.c b/drivers/net/can/spi/hi311x.c
new file mode 100644
index 000..2a3d794
--- /dev/null
+++ b/drivers/net/can/spi/hi311x.c
@@ -0,0 +1,1072 @@
+/* CAN bus driver for Holt HI3110 CAN Controller with SPI Interface
+ *
+ * Copyright(C) Timesys Corporation 2016
+ *
+ * Based on Microchip 251x CAN Controller (mcp251x) Linux kernel driver
+ * Copyright 2009 Christian Pellegrin EVOL S.r.l.
+ * Copyright 2007 Raymarine UK, Ltd. All Rights Reserved.
+ * Copyright 2006 Arcom Control Systems Ltd.
+ *
+ * Based on CAN bus driver for the CCAN controller written by
+ * - Sascha Hauer, Marc Kleine-Budde, Pengutronix
+ * - Simon Kallweit, intefo AG
+ * Copyright 2007
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */


... snip...


+
+static irqreturn_t hi3110_can_ist(int irq, void *dev_id)
+{
+   struct hi3110_priv *priv = dev_id;
+   struct spi_device *spi = priv->spi;
+   struct net_device *net = priv->net;
+
+   mutex_lock(>hi3110_lock);
+
+   while (!priv->force_quit) {
+   enum can_state new_state;
+   u8 intf, eflag, statf;
+
+   while (!(HI3110_STAT_RXFMTY &
+  (statf = hi3110_read(spi, HI3110_READ_STATF {
+   hi3110_hw_rx(spi);
+   }
+
+   intf = hi3110_read(spi, HI3110_READ_INTF);


I think HI3110_READ_ERR is only valid if HI3110_INT_BUSERR is set! 
Therefore:


if ((intf & HI3110_INT_BUSERR) {

It saves reading one SPI register in the message processing path. Please 
check if back-to-error-active still comes.



+   eflag = hi3110_read(spi, HI3110_READ_ERR);
+   /* Update can state */
+   if (eflag & HI3110_ERR_BUSOFF)
+   new_state = CAN_STATE_BUS_OFF;
+   else if (eflag & HI3110_ERR_PASSIVE_MASK)
+   new_state = CAN_STATE_ERROR_PASSIVE;
+   else if (statf & HI3110_STAT_ERRW)
+   new_state = CAN_STATE_ERROR_WARNING;
+   else
+   new_state = CAN_STATE_ERROR_ACTIVE;
+
+   if (new_state != priv->can.state) {
+   struct can_frame *cf;
+   struct sk_buff *skb;
+   enum can_state rx_state, tx_state;
+   u8 rxerr, txerr;
+
+   skb = alloc_can_err_skb(net, );
+   if (!skb)
+   break;
+
+   txerr = hi3110_read(spi, HI3110_READ_TEC);
+   rxerr = hi3110_read(spi, HI3110_READ_REC);
+   cf->data[6] = txerr;
+   cf->data[7] = rxerr;
+   tx_state = txerr >= rxerr ? new_state : 0;
+   rx_state = txerr <= rxerr ? new_state : 0;
+   can_change_state(net, cf,

Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

2017-03-18 Thread Wolfgang Grandegger


Hello Akshay,

Am 17.03.2017 um 19:28 schrieb Akshay Bhat:

Hi Wolfgang,

On 03/17/2017 01:04 PM, Wolfgang Grandegger wrote:


Hm, that's unusual. Cable disconnected and then send a message:

$ grep /proc/interrupts; sleep 10; /proc/interrupts

should make things clear. But maybe it's a clever chip and it does stop
sending error messages if the error counter does not change any more.
After bus-off, the chip is quiet, of course. Should have a closer look
to the CAN standard.



The interrupt count does not increment after device reaches
tx-error-passive (with cable disconnected).

# while true; do grep -i hi3110 /proc/interrupts; sleep 10; done &
[1] 793
#
111:  0  0  gpio-mxc  12 Edge  hi3110
# candump -t d -e any,0:0,#FFF &
[2] 798
# cansend can0 123#
#
 (000.00)  can0  2004   [8]  00 08 00 00 00 00 60 00   ERRORFRAME
controller-problem{tx-error-warning}
error-counter-tx-rx{{96}{0}}
 (000.002122)  can0  2004   [8]  00 20 00 00 00 00 80 00   ERRORFRAME
controller-problem{tx-error-passive}
error-counter-tx-rx{{128}{0}}
111: 10  0  gpio-mxc  12 Edge  hi3110
111: 10  0  gpio-mxc  12 Edge  hi3110
111: 10  0  gpio-mxc  12 Edge  hi3110


OK, then there is no good reason connecting the STAT interrupt pin.

Wolfgang.

Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

2017-03-17 Thread Wolfgang Grandegger


Hi Akshay,

Am 17.03.2017 um 17:00 schrieb Akshay Bhat:

Hi Wolfgang,

On 03/17/2017 03:39 AM, Wolfgang Grandegger wrote:

Hello Akshay,

Am 16.03.2017 um 23:29 schrieb Akshay Bhat:

Hi Wolfgang,

On 03/16/2017 04:02 PM, Wolfgang Grandegger wrote:


Looks much better now! There are message for state changes to error
warning and passive. Just the following message is not correct:

 (000.200824)  can0  2004   [8]  00 40 00 00 00 00 5F 19
ERRORFRAME
controller-problem{}
error-counter-tx-rx{{95}{25}}

Sorry, forgot to mention... the function can_change_state() [1]
should be used for that purpose, if possible. It fixes the issue
above as well.



The updated driver (the one used to create the above log) is using
can_change_state() function. data[1] 40 corresponds to
CAN_ERR_CRTL_ACTIVE, so looks correct? Could it be that the can-utils I
am using is old and not reporting state change?


Hm, yes. The raw data looks correct. You could download and build a
recent version from "https://github.com/linux-can/can-utils; to check.
It could also be a bug.



Turned out to be a old version of can-utils. Using the above git tree
reports the flag.

 (000.200308)  can0  2004   [8]  00 40 00 00 00 00 5F 00   ERRORFRAME
controller-problem{back-to-error-active}
error-counter-tx-rx{{95}{0}}


Enabling BUSOFF/ERRP/ERRW bits in STATFE did not generate any interrupts
on the INT pin. Should we make it a requirement that both INT and STAT
pins need to be connected in hardware for the driver to do the error
reporting?


As I said, it's the better solution, especially if interrupt flooding
does harm. How does your system behave when bus errors come in due to no
cable connected?



I did not see any issues on the system with the cable disconnected. In
my particular setup with the cable disconnected the system goes to
tx-error-passive and does not get any further interrupts until a state
change occurs.


Hm, that's unusual. Cable disconnected and then send a message:

$ grep /proc/interrupts; sleep 10; /proc/interrupts

should make things clear. But maybe it's a clever chip and it does stop 
sending error messages if the error counter does not change any more. 
After bus-off, the chip is quiet, of course. Should have a closer look 
to the CAN standard.



So far using NAPI was mandatory. There is the problem of out-of-order
message reception if handled in the isr on multi processor systems.
Marc, what is the current policy?



Since this is a SPI based CAN, I am wary for any additional latencies
NAPI might introduce. The RX handling is being done at the very
beginning of the ISR for this reason.

Can we go ahead with the existing implementation and re-visit this at a
later time?


Likely yes, as Marc has already reviewed the driver once.

BTW: what system board/processor are you using?


Thanks again for all your help in reviewing/improving the driver :)


You are welcome!

Wolfgang.

Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

2017-03-17 Thread Wolfgang Grandegger


Hello Akshay,

Am 17.03.2017 um 08:39 schrieb Wolfgang Grandegger:

Hello Akshay,

Am 16.03.2017 um 23:29 schrieb Akshay Bhat:

Hi Wolfgang,

On 03/16/2017 04:02 PM, Wolfgang Grandegger wrote:


Looks much better now! There are message for state changes to error
warning and passive. Just the following message is not correct:

 (000.200824)  can0  2004   [8]  00 40 00 00 00 00 5F 19
ERRORFRAME
controller-problem{}
error-counter-tx-rx{{95}{25}}

Sorry, forgot to mention... the function can_change_state() [1]
should be used for that purpose, if possible. It fixes the issue
above as well.



The updated driver (the one used to create the above log) is using
can_change_state() function. data[1] 40 corresponds to
CAN_ERR_CRTL_ACTIVE, so looks correct? Could it be that the can-utils I
am using is old and not reporting state change?


Hm, yes. The raw data looks correct. You could download and build a
recent version from "https://github.com/linux-can/can-utils; to check.
It could also be a bug.


Support for that flags has been added in December 2014.

Wolfgang-

Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

2017-03-17 Thread Wolfgang Grandegger


Hello Akshay,

Am 16.03.2017 um 23:29 schrieb Akshay Bhat:

Hi Wolfgang,

On 03/16/2017 04:02 PM, Wolfgang Grandegger wrote:


Looks much better now! There are message for state changes to error
warning and passive. Just the following message is not correct:

 (000.200824)  can0  2004   [8]  00 40 00 00 00 00 5F 19   ERRORFRAME
controller-problem{}
error-counter-tx-rx{{95}{25}}

Sorry, forgot to mention... the function can_change_state() [1]
should be used for that purpose, if possible. It fixes the issue
above as well.



The updated driver (the one used to create the above log) is using
can_change_state() function. data[1] 40 corresponds to
CAN_ERR_CRTL_ACTIVE, so looks correct? Could it be that the can-utils I
am using is old and not reporting state change?


Hm, yes. The raw data looks correct. You could download and build a 
recent version from "https://github.com/linux-can/can-utils; to check. 
It could also be a bug.



Tentative v4 driver for reference:
http://pastebin.com/4xFVL1Sj


berr-reporting off case:
http://pastebin.com/fUn3j7qU


Ditto.

I just had another look to the manual and there is this undocumented
STATFE register at offset 0x1E. It's mentioned in some other parts of
the doc as interrupt enable register for STATF events. I would assume
the same bit layout than STATF. If you set bit 2 (BUSOFF), 3 (ERRP)
and 4 (ERRW), you may get interrupts. It's worth a try, I think. If
it works, it's the much better solution.



The HI-311x has a INT pin and a STAT pin. The hardware I have has only
the INT pin connected to the processor. If the STAT pin was also
connected, then like you mentioned it could be a much better solution to
use STAT for state changes.


OK, I understand.


Enabling BUSOFF/ERRP/ERRW bits in STATFE did not generate any interrupts
on the INT pin. Should we make it a requirement that both INT and STAT
pins need to be connected in hardware for the driver to do the error
reporting?


As I said, it's the better solution, especially if interrupt flooding 
does harm. How does your system behave when bus errors come in due to no 
cable connected?


So far using NAPI was mandatory. There is the problem of out-of-order 
message reception if handled in the isr on multi processor systems. 
Marc, what is the current policy?


Wolfgang.

Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

2017-03-16 Thread Wolfgang Grandegger

Hello Akshay,

Am 16.03.2017 um 18:06 schrieb Akshay Bhat:
> Hi Wolfgang,
> 
> On 03/15/2017 05:42 AM, Wolfgang Grandegger wrote:
>> Hello Akshay,
>>
> ..snip..
>>>
>>> So here is my plan:
>>> - Have the bus error interrupt always enabled
>>> - If berr-reporting off, then have the isr checks/reports state changes
>>
>> Error state change messages should always be there. These are the
>> important one.
>>
>>> - if berr-reporting on, then have the isr checks/reports bus errors
>>> and state changes (Does it make sense packing the error message, if
>>> the ISR finds both bus and state changes?)
>>
>> If berr-reporting is off, simply do not create an error message for bus
>> errors, and only if the state changed. If it's "on" create an additional
>> bus error message.
>>
>> http://lxr.free-electrons.com/source/drivers/net/can/flexcan.c#L334
>>
>>
> 
> I have fixed the driver to handle the error reporting. Also thanks for
> your tip for generating bus-off by setting the host device at a
> different CAN bit rate! Below are logs with the updated driver. Let me
> know if you have any concerns, if not I will submit the v4 patch.
> 
> berr-reporting on case:
> http://pastebin.com/qDRLERmW

Looks much better now! There are message for state changes to error
warning and passive. Just the following message is not correct:

 (000.200824)  can0  2004   [8]  00 40 00 00 00 00 5F 19   ERRORFRAME
controller-problem{}
error-counter-tx-rx{{95}{25}}

Sorry, forgot to mention... the function can_change_state() [1]
should be used for that purpose, if possible. It fixes the issue
above as well.

> berr-reporting off case:
> http://pastebin.com/fUn3j7qU

Ditto.

I just had another look to the manual and there is this undocumented
STATFE register at offset 0x1E. It's mentioned in some other parts of
the doc as interrupt enable register for STATF events. I would assume
the same bit layout than STATF. If you set bit 2 (BUSOFF), 3 (ERRP)
and 4 (ERRW), you may get interrupts. It's worth a try, I think. If
it works, it's the much better solution.

Wolfgang.

[1] http://lxr.free-electrons.com/ident?i=can_change_state

Wolfgang.

Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

2017-03-15 Thread Wolfgang Grandegger


Hello Akshay,

Am 15.03.2017 um 05:44 schrieb Akshay Bhat:

Hi Wolfgang,

On Tue, Mar 14, 2017 at 2:08 PM, Wolfgang Grandegger <w...@grandegger.com> 
wrote:
...snip

/disconnect cable
  can0  2088   [8]  00 00 00 19 00 00 28 00   ERRORFRAME
protocol-violation{{}{acknowledge-slot}}
bus-error
error-counter-tx-rx{{40}{0}}
  can0  2088   [8]  00 00 00 19 00 00 58 00   ERRORFRAME
protocol-violation{{}{acknowledge-slot}}
bus-error
error-counter-tx-rx{{88}{0}}
  can0  2088   [8]  00 00 00 19 00 00 80 00   ERRORFRAME
protocol-violation{{}{acknowledge-slot}}
bus-error
error-counter-tx-rx{{128}{0}}



TX error warning is missing.



This support was missing in the driver, added in V4 patch.


  can0  208C   [8]  00 20 00 19 00 00 80 00   ERRORFRAME
controller-problem{tx-error-passive}
protocol-violation{{}{acknowledge-slot}}
bus-error
error-counter-tx-rx{{128}{0}}



Here "tx-error-passiv" is packed with a bus error. What I'm looking for are
state change messages similar to:

   can0  2204  [8] 00 08 00 00 00 00 60 00   ERRORFRAME
controller-problem{tx-error-warning}
state-change{tx-error-warning}
error-counter-tx-rx{{96}{0}}
   can0  2204  [8] 00 30 00 00 00 00 80 00   ERRORFRAME
controller-problem{tx-error-passive}
state-change{tx-error-passive}
error-counter-tx-rx{{128}{0}

They should always come, even with "berr-reporting off".



HI-3110 has only 1 bus error interrupt. There is no dedicated state
change interrupts like other controllers.


I have little hope! Some revision of the flexcan controller have the 
same problem




So here is my plan:
- Have the bus error interrupt always enabled
- If berr-reporting off, then have the isr checks/reports state changes


Error state change messages should always be there. These are the 
important one.



- if berr-reporting on, then have the isr checks/reports bus errors
and state changes (Does it make sense packing the error message, if
the ISR finds both bus and state changes?)


If berr-reporting is off, simply do not create an error message for bus 
errors, and only if the state changed. If it's "on" create an additional 
bus error message.


http://lxr.free-electrons.com/source/drivers/net/can/flexcan.c#L334



write: No buffer space available
root@imx6qrom5420b1:~# ip -s -d link show can0
4: can0: <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 100 sample-point 0.750
  tq 62 prop-seg 5 phase-seg1 6 phase-seg2 4 sjw 1
  hi3110: tseg1 2..16 tseg2 2..8 sjw 1..4 brp 1..64 brp-inc 1
  clock 1600
  re-started bus-errors arbit-lost error-warn error-pass bus-off
  0  6  0  1  1  0



The error warning and passive counter increased , though. Also the bus error
should come in at a rather hight rate. Looking to the code, maybe
you need to test STATF to check for state changes (and not ERR).



Apologize, just realized In the above case some error packets were
lost, because I forgot to set the CPU frequency to max. Will resend
the log.

..snip...


After some more messages there should be also:

can0  2200  [8] 00 40 00 00 00 00 5F 00   ERRORFRAME
state-change{back-to-error-active}
error-counter-tx-rx{{95}{0}}

For each message sent, the error counter decreases by 8.



The HI-3110 controller decrements the error counter by 1 for every message sent.
The error count increments by 8 when there is an error.


Seems OK according to:

http://electronics.stackexchange.com/questions/220596/can-error-counters-behaviour





root@imx6qrom5420b1:~# ip -s -d link show can0
4: can0: <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 117 rx 0) restart-ms 0
  bitrate 100 sample-point 0.750
  tq 62 prop-seg 5 phase-seg1 6 phase-seg2 4 sjw 1
  hi3110: tseg1 2..16 tseg2 2..8 sjw 1..4 brp 1..64 brp-inc 1
  clock 1600
  re-started bus-errors arbit-lost error-warn error-pass bus-off
  0  1  0  0  0  0



Strange, some counters got lost.



This was a bug introduced when adding berr-reporting, have fixed in v4 patch.



I have not been able to check the bus-off condition by (short-circuiting
CAN low and high). The tec error count remains at 128 when I short the
CAN low and high pins and the status never goes BUSOFF.



You also need to send a message and the short-circuit should be at the
connector of the sending host. What tranceiver is used? Do you know?



ADM3054 transceiv

Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

2017-03-15 Thread Wolfgang Grandegger


Hello Akshay,

Am 15.03.2017 um 05:44 schrieb Akshay Bhat:

Hi Wolfgang,

On Tue, Mar 14, 2017 at 2:08 PM, Wolfgang Grandegger <w...@grandegger.com> 
wrote:
...snip

/disconnect cable
  can0  2088   [8]  00 00 00 19 00 00 28 00   ERRORFRAME
protocol-violation{{}{acknowledge-slot}}
bus-error
error-counter-tx-rx{{40}{0}}
  can0  2088   [8]  00 00 00 19 00 00 58 00   ERRORFRAME
protocol-violation{{}{acknowledge-slot}}
bus-error
error-counter-tx-rx{{88}{0}}
  can0  2088   [8]  00 00 00 19 00 00 80 00   ERRORFRAME
protocol-violation{{}{acknowledge-slot}}
bus-error
error-counter-tx-rx{{128}{0}}



TX error warning is missing.



This support was missing in the driver, added in V4 patch.


  can0  208C   [8]  00 20 00 19 00 00 80 00   ERRORFRAME
controller-problem{tx-error-passive}
protocol-violation{{}{acknowledge-slot}}
bus-error
error-counter-tx-rx{{128}{0}}



Here "tx-error-passiv" is packed with a bus error. What I'm looking for are
state change messages similar to:

   can0  2204  [8] 00 08 00 00 00 00 60 00   ERRORFRAME
controller-problem{tx-error-warning}
state-change{tx-error-warning}
error-counter-tx-rx{{96}{0}}
   can0  2204  [8] 00 30 00 00 00 00 80 00   ERRORFRAME
controller-problem{tx-error-passive}
state-change{tx-error-passive}
error-counter-tx-rx{{128}{0}

They should always come, even with "berr-reporting off".



HI-3110 has only 1 bus error interrupt. There is no dedicated state
change interrupts like other controllers.


To double check: Could you please read INTF, ERR and STATF at the 
beginning of the ISR and print it out (using dev_dbg and fiends). Then 
run a test with no cable connected and bus error reporting off.


Wolfgang.

Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

2017-03-14 Thread Wolfgang Grandegger


Am 14.03.2017 um 19:08 schrieb Wolfgang Grandegger:

Hello Akshay,

Am 14.03.2017 um 17:20 schrieb Akshay Bhat:


Hi Wolfgang,

On 03/14/2017 08:11 AM, Wolfgang Grandegger wrote:

... snip ...

A few other things to check:

Run "cangen" and monitor the message with "candump -e
any,0:0,#FFF".
Then 1) disconnect the cable or 2) short-circuit CAN low and high
at the
connector. You should see error messages. After reconnection or
removing
the short-circuit (and bus-off recovery) the state should go back to
"active".



With the above sequence, candump reports "ERRORFRAME" with
protocol-violation{{}{acknowledge-slot}}, bus-error. On re-connecting
the cable the can state goes back to ACTIVE and I see the messages that
were in the queue being sent.


Do you get the ACK error also with berr-reporting off? Would be nice if
you could show a candump log here.



Below is a log for disconnecting and re-connecting CAN cable scenario:
(Note this is on a 4.1.18 kernel with RT patch)

root@imx6qrom5420b1:~# ip link set can0 up type can bitrate 100
berr-reporting on
root@imx6qrom5420b1:~# candump -e any,0:0,#FFF &


Please add "-td" ...


[1] 768
root@imx6qrom5420b1:~# cangen can0


and "-i" here.


  can0  21C   [8]  35 98 C0 7A 95 03 E6 2A
  can0  6E6   [1]  F2
  can0  5C7   [2]  42 50
  can0  57C   [8]  83 7A E4 0C 03 8B 90 45
  can0  55C   [8]  B9 74 87 52 D8 F4 64 04
  can0  014   [8]  28 CB 96 57 3B 80 67 4F
  can0  6AF   [1]  35
  can0  51E   [8]  B6 C8 6C 1D 3A 87 ED 2E
  can0  527   [8]  D0 8A D3 59 0E 34 40 78
  can0  30C   [2]  6A 12
  can0  145   [8]  CB 6E FF 55 C1 BE C3 22
  can0  5A5   [8]  C4 49 54 68 02 63 F9 35
  can0  0BA   [8]  DA 57 5E 3A CE 88 20 1C
  can0  516   [2]  09 09
  can0  743   [8]  7C 4D 25 47 61 4C 56 3D
  can0  31D   [2]  9C D3
  can0  71E   [8]  53 7C 97 2A 2A F2 9F 56
  can0  52E   [8]  FE DA 2D 51 73 96 DF 79
/disconnect cable
  can0  2088   [8]  00 00 00 19 00 00 28 00   ERRORFRAME
protocol-violation{{}{acknowledge-slot}}
bus-error
error-counter-tx-rx{{40}{0}}
  can0  2088   [8]  00 00 00 19 00 00 58 00   ERRORFRAME
protocol-violation{{}{acknowledge-slot}}
bus-error
error-counter-tx-rx{{88}{0}}
  can0  2088   [8]  00 00 00 19 00 00 80 00   ERRORFRAME
protocol-violation{{}{acknowledge-slot}}
bus-error
error-counter-tx-rx{{128}{0}}


TX error warning is missing.


  can0  208C   [8]  00 20 00 19 00 00 80 00   ERRORFRAME
controller-problem{tx-error-passive}
protocol-violation{{}{acknowledge-slot}}
bus-error
error-counter-tx-rx{{128}{0}}


Here "tx-error-passiv" is packed with a bus error. What I'm looking for
are state change messages similar to:

   can0  2204  [8] 00 08 00 00 00 00 60 00   ERRORFRAME
controller-problem{tx-error-warning}
state-change{tx-error-warning}
error-counter-tx-rx{{96}{0}}
   can0  2204  [8] 00 30 00 00 00 00 80 00   ERRORFRAME
controller-problem{tx-error-passive}
state-change{tx-error-passive}
error-counter-tx-rx{{128}{0}

They should always come, even with "berr-reporting off".


write: No buffer space available
root@imx6qrom5420b1:~# ip -s -d link show can0
4: can0: <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 100 sample-point 0.750
  tq 62 prop-seg 5 phase-seg1 6 phase-seg2 4 sjw 1
  hi3110: tseg1 2..16 tseg2 2..8 sjw 1..4 brp 1..64 brp-inc 1
  clock 1600
  re-started bus-errors arbit-lost error-warn error-pass bus-off
  0  6  0  1  1  0


The error warning and passive counter increased , though. Also the bus
error should come in at a rather hight rate. Looking to the code, maybe
you need to test STATF to check for state changes (and not ERR).


Likely the ERR bits are only valid if the BUSERR bit in INTF is set.


RX: bytes  packets  errors  dropped overrun mcast
0  06   0   0   0
TX: bytes  packets  errors  dropped carrier collsns
10618   0   0   0   0
root@imx6qrom5420b1:~#
/re-connect cable
  can0  169   [8]  35 55 A3 1C 0F 47 2E 5B
  can0  318   [8]  11 AA 27 11 D2 1B CE 34
  can0  577   [8]  A0 A4 EE 50 8D A2 E1 3E
  can0  4ED   [8]  52 96 17 7E 31 FC 7D 7C
  can0  2E7   [8]  92 48 D4 39 05 1E 9F 50
  can0  200   [8]  4A 66 F6 02 1E 71 8E 26
  can0  29A   [8]  49 63 2E 7D C9 77 85 7A
  can0  15A   [7]  3C 0E 65 74 C3 62 80
  can0  011   [1]  D2
  can0  26B   [3]  FC D6 68
  can0  5CE   [8]  6F 02 B5 14 BC 7A D7 02

root@imx6qrom5420b1:~# ip -s -d link show can0
4: can0: <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-ACT

Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

2017-03-14 Thread Wolfgang Grandegger


Hello Akshay,

Am 14.03.2017 um 17:20 schrieb Akshay Bhat:


Hi Wolfgang,

On 03/14/2017 08:11 AM, Wolfgang Grandegger wrote:

... snip ...

A few other things to check:

Run "cangen" and monitor the message with "candump -e any,0:0,#FFF".
Then 1) disconnect the cable or 2) short-circuit CAN low and high at the
connector. You should see error messages. After reconnection or removing
the short-circuit (and bus-off recovery) the state should go back to
"active".



With the above sequence, candump reports "ERRORFRAME" with
protocol-violation{{}{acknowledge-slot}}, bus-error. On re-connecting
the cable the can state goes back to ACTIVE and I see the messages that
were in the queue being sent.


Do you get the ACK error also with berr-reporting off? Would be nice if
you could show a candump log here.



Below is a log for disconnecting and re-connecting CAN cable scenario:
(Note this is on a 4.1.18 kernel with RT patch)

root@imx6qrom5420b1:~# ip link set can0 up type can bitrate 100
berr-reporting on
root@imx6qrom5420b1:~# candump -e any,0:0,#FFF &


Please add "-td" ...


[1] 768
root@imx6qrom5420b1:~# cangen can0


and "-i" here.


  can0  21C   [8]  35 98 C0 7A 95 03 E6 2A
  can0  6E6   [1]  F2
  can0  5C7   [2]  42 50
  can0  57C   [8]  83 7A E4 0C 03 8B 90 45
  can0  55C   [8]  B9 74 87 52 D8 F4 64 04
  can0  014   [8]  28 CB 96 57 3B 80 67 4F
  can0  6AF   [1]  35
  can0  51E   [8]  B6 C8 6C 1D 3A 87 ED 2E
  can0  527   [8]  D0 8A D3 59 0E 34 40 78
  can0  30C   [2]  6A 12
  can0  145   [8]  CB 6E FF 55 C1 BE C3 22
  can0  5A5   [8]  C4 49 54 68 02 63 F9 35
  can0  0BA   [8]  DA 57 5E 3A CE 88 20 1C
  can0  516   [2]  09 09
  can0  743   [8]  7C 4D 25 47 61 4C 56 3D
  can0  31D   [2]  9C D3
  can0  71E   [8]  53 7C 97 2A 2A F2 9F 56
  can0  52E   [8]  FE DA 2D 51 73 96 DF 79
/disconnect cable
  can0  2088   [8]  00 00 00 19 00 00 28 00   ERRORFRAME
protocol-violation{{}{acknowledge-slot}}
bus-error
error-counter-tx-rx{{40}{0}}
  can0  2088   [8]  00 00 00 19 00 00 58 00   ERRORFRAME
protocol-violation{{}{acknowledge-slot}}
bus-error
error-counter-tx-rx{{88}{0}}
  can0  2088   [8]  00 00 00 19 00 00 80 00   ERRORFRAME
protocol-violation{{}{acknowledge-slot}}
bus-error
error-counter-tx-rx{{128}{0}}


TX error warning is missing.


  can0  208C   [8]  00 20 00 19 00 00 80 00   ERRORFRAME
controller-problem{tx-error-passive}
protocol-violation{{}{acknowledge-slot}}
bus-error
error-counter-tx-rx{{128}{0}}


Here "tx-error-passiv" is packed with a bus error. What I'm looking for 
are state change messages similar to:


   can0  2204  [8] 00 08 00 00 00 00 60 00   ERRORFRAME
controller-problem{tx-error-warning}
state-change{tx-error-warning}
error-counter-tx-rx{{96}{0}}
   can0  2204  [8] 00 30 00 00 00 00 80 00   ERRORFRAME
controller-problem{tx-error-passive}
state-change{tx-error-passive}
error-counter-tx-rx{{128}{0}

They should always come, even with "berr-reporting off".


write: No buffer space available
root@imx6qrom5420b1:~# ip -s -d link show can0
4: can0: <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 100 sample-point 0.750
  tq 62 prop-seg 5 phase-seg1 6 phase-seg2 4 sjw 1
  hi3110: tseg1 2..16 tseg2 2..8 sjw 1..4 brp 1..64 brp-inc 1
  clock 1600
  re-started bus-errors arbit-lost error-warn error-pass bus-off
  0  6  0  1  1  0


The error warning and passive counter increased , though. Also the bus 
error should come in at a rather hight rate. Looking to the code, maybe

you need to test STATF to check for state changes (and not ERR).


RX: bytes  packets  errors  dropped overrun mcast
0  06   0   0   0
TX: bytes  packets  errors  dropped carrier collsns
10618   0   0   0   0
root@imx6qrom5420b1:~#
/re-connect cable
  can0  169   [8]  35 55 A3 1C 0F 47 2E 5B
  can0  318   [8]  11 AA 27 11 D2 1B CE 34
  can0  577   [8]  A0 A4 EE 50 8D A2 E1 3E
  can0  4ED   [8]  52 96 17 7E 31 FC 7D 7C
  can0  2E7   [8]  92 48 D4 39 05 1E 9F 50
  can0  200   [8]  4A 66 F6 02 1E 71 8E 26
  can0  29A   [8]  49 63 2E 7D C9 77 85 7A
  can0  15A   [7]  3C 0E 65 74 C3 62 80
  can0  011   [1]  D2
  can0  26B   [3]  FC D6 68
  can0  5CE   [8]  6F 02 B5 14 BC 7A D7 02

root@imx6qrom5420b1:~# ip -s -d link show can0
4: can0: <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 117 rx 0)
restart-ms 0

Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

2017-03-14 Thread Wolfgang Grandegger


Hallo Akshay,

Am 13.03.2017 um 16:38 schrieb Akshay Bhat:

Hi Wolfgang,

On 03/09/2017 12:36 PM, Wolfgang Grandegger wrote:

Hello,

doing a quick review... I realized a few issues...

Am 17.01.2017 um 20:22 schrieb Akshay Bhat:

... snip ...

A few other things to check:

Run "cangen" and monitor the message with "candump -e any,0:0,#FFF".
Then 1) disconnect the cable or 2) short-circuit CAN low and high at the
connector. You should see error messages. After reconnection or removing
the short-circuit (and bus-off recovery) the state should go back to
"active".



With the above sequence, candump reports "ERRORFRAME" with
protocol-violation{{}{acknowledge-slot}}, bus-error. On re-connecting
the cable the can state goes back to ACTIVE and I see the messages that
were in the queue being sent.


Do you get the ACK error also with berr-reporting off? Would be nice if 
you could show a candump log here.


Also, any error message should show the bus error counts in data[7,8]:

http://lxr.free-electrons.com/source/drivers/net/can/sja1000/sja1000.c#L408

And please check bus-off as well (short-circuiting CAN low and high).

Wolfgang.

Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

2017-03-09 Thread Wolfgang Grandegger


Hello,

doing a quick review... I realized a few issues...

Am 17.01.2017 um 20:22 schrieb Akshay Bhat:

This patch adds support for the Holt HI-311x CAN controller. The HI311x
CAN controller is capable of transmitting and receiving standard data
frames, extended data frames and remote frames. The HI311x interfaces
with the host over SPI.

Datasheet: www.holtic.com/documents/371-hi-3110_v-rev-jpdf.do

Signed-off-by: Akshay Bhat 
---

v1 -> v2:
Address comments from Marc Kleine-Budde:
- use u8 instead of uint8_t
- alphabetically sort Makefile and Kconfig
- copy over copyright information from mcp251x
- use single space after each marco
- add missing HI3110_ namespace in defines
- remove magic number for IDE & SRR bits
- simplify logic to populate extended CAN ID
- remove unused parameters in hi3110_setup function
- remove redundant frame->can_dlc length check
- simplify error handling in hi3110_open function
Address comments from Julia Lawall:
- remove unnecessary semicolon after while loop in hi3110_can_ist

 drivers/net/can/spi/Kconfig  |6 +
 drivers/net/can/spi/Makefile |1 +
 drivers/net/can/spi/hi311x.c | 1069 ++
 3 files changed, 1076 insertions(+)
 create mode 100644 drivers/net/can/spi/hi311x.c

diff --git a/drivers/net/can/spi/Kconfig b/drivers/net/can/spi/Kconfig
index 148cae5..8f2e0dd 100644
--- a/drivers/net/can/spi/Kconfig
+++ b/drivers/net/can/spi/Kconfig
@@ -1,6 +1,12 @@
 menu "CAN SPI interfaces"
depends on SPI

+config CAN_HI311X
+   tristate "Holt HI311x SPI CAN controllers"
+   depends on CAN_DEV && SPI && HAS_DMA
+   ---help---
+ Driver for the Holt HI311x SPI CAN controllers.
+
 config CAN_MCP251X
tristate "Microchip MCP251x SPI CAN controllers"
depends on HAS_DMA
diff --git a/drivers/net/can/spi/Makefile b/drivers/net/can/spi/Makefile
index 0e86040..f59fa37 100644
--- a/drivers/net/can/spi/Makefile
+++ b/drivers/net/can/spi/Makefile
@@ -3,4 +3,5 @@
 #


+obj-$(CONFIG_CAN_HI311X)   += hi311x.o
 obj-$(CONFIG_CAN_MCP251X)  += mcp251x.o
diff --git a/drivers/net/can/spi/hi311x.c b/drivers/net/can/spi/hi311x.c
new file mode 100644
index 000..cccfe2d
--- /dev/null
+++ b/drivers/net/can/spi/hi311x.c
@@ -0,0 +1,1069 @@
+/* CAN bus driver for Holt HI3110 CAN Controller with SPI Interface
+ *
+ * Copyright(C) Timesys Corporation 2016
+ *
+ * Based on Microchip 251x CAN Controller (mcp251x) Linux kernel driver
+ * Copyright 2009 Christian Pellegrin EVOL S.r.l.
+ * Copyright 2007 Raymarine UK, Ltd. All Rights Reserved.
+ * Copyright 2006 Arcom Control Systems Ltd.
+ *
+ * Based on CAN bus driver for the CCAN controller written by
+ * - Sascha Hauer, Marc Kleine-Budde, Pengutronix
+ * - Simon Kallweit, intefo AG
+ * Copyright 2007
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define HI3110_MASTER_RESET 0x56
+#define HI3110_READ_CTRL0 0xD2
+#define HI3110_READ_CTRL1 0xD4
+#define HI3110_READ_STATF 0xE2
+#define HI3110_WRITE_CTRL0 0x14
+#define HI3110_WRITE_CTRL1 0x16
+#define HI3110_WRITE_INTE 0x1C
+#define HI3110_WRITE_BTR0 0x18
+#define HI3110_WRITE_BTR1 0x1A
+#define HI3110_READ_BTR0 0xD6
+#define HI3110_READ_BTR1 0xD8
+#define HI3110_READ_INTF 0xDE
+#define HI3110_READ_ERR 0xDC
+#define HI3110_READ_FIFO_WOTIME 0x48
+#define HI3110_WRITE_FIFO 0x12
+#define HI3110_READ_MESSTAT 0xDA
+#define HI3110_READ_TEC 0xEC
+
+#define HI3110_CTRL0_MODE_MASK (7 << 5)
+#define HI3110_CTRL0_NORMAL_MODE (0 << 5)
+#define HI3110_CTRL0_LOOPBACK_MODE (1 << 5)
+#define HI3110_CTRL0_MONITOR_MODE (2 << 5)
+#define HI3110_CTRL0_SLEEP_MODE (3 << 5)
+#define HI3110_CTRL0_INIT_MODE (4 << 5)
+
+#define HI3110_CTRL1_TXEN BIT(7)
+
+#define HI3110_INT_RXTMP BIT(7)
+#define HI3110_INT_RXFIFO BIT(6)
+#define HI3110_INT_TXCPLT BIT(5)
+#define HI3110_INT_BUSERR BIT(4)
+#define HI3110_INT_MCHG BIT(3)
+#define HI3110_INT_WAKEUP BIT(2)
+#define HI3110_INT_F1MESS BIT(1)
+#define HI3110_INT_F0MESS BIT(0)
+
+#define HI3110_ERR_BUSOFF BIT(7)
+#define HI3110_ERR_TXERRP BIT(6)
+#define HI3110_ERR_RXERRP BIT(5)
+#define HI3110_ERR_BITERR BIT(4)
+#define HI3110_ERR_FRMERR BIT(3)
+#define HI3110_ERR_CRCERR BIT(2)
+#define HI3110_ERR_ACKERR BIT(1)
+#define HI3110_ERR_STUFERR BIT(0)
+#define HI3110_ERR_PROTOCOL_MASK (0x1F)
+
+#define HI3110_STAT_RXFMTY BIT(1)
+
+#define HI3110_BTR0_SJW_SHIFT 6
+#define HI3110_BTR0_BRP_SHIFT 0
+
+#define HI3110_BTR1_SAMP_3PERBIT (1 << 7)
+#define HI3110_BTR1_SAMP_1PERBIT (0 << 7)
+#define HI3110_BTR1_TSEG2_SHIFT 4
+#define HI3110_BTR1_TSEG1_SHIFT 0
+
+#define HI3110_FIFO_WOTIME_TAG_OFF 0
+#define

Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

2017-03-09 Thread Wolfgang Grandegger


Hello Akshay,

Am 09.03.2017 um 13:34 schrieb Akshay Bhat:



On 03/09/2017 04:59 AM, Wolfgang Grandegger wrote:

Hello Akshay,

unfortunately there are not many CAN controllers for the SPI bus. I just
know the MPC251x, which behaves badly (message losses) under Linux,
especially at hight bit-rates due to insufficient RX buffering. What is
your experience with that driver for the HI-311x?



Hi Wolfgang,

Good question. I have not worked with MPC251x but the HI-311x performs
much better because HI-3110 features:
8 message FIFO (as opposed to 2 buffers on MPC2510)
20 MHz SPI interface (as opposed to 2.5 MHz on MPC2510)

As for the real world test results:

With RT patch applied to the kernel running on a i.MX6 Dual processor
(worst case interrupt latency of 50us as reported by cyclictest), there
are ZERO packet drops.
Tested with Kvaser Leaf sending 100 burst messages (back to back) every
40ms at a 1M CAN bit rate. 10 million messages were sent by the Kvaser
leaf and received successfully by the HI-311x driver.


This corresponds to a bus load of approx. 50%, I think?


Even without the RT patch, I was able to get the packet drop to zero but
this was by moving the CAN/SPI IRQ threads to CPU1 instead of CPU0.


Vanilla Linux is more critical here due to higher latencies. With 2500 
Messages per sec the RX FIFO (8 Messages) fills up within 3.2 ms... and 
in a burst even quicker. That's already heavy load.


Wolfgang.

Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

2017-03-09 Thread Wolfgang Grandegger


Hello Akshay,

unfortunately there are not many CAN controllers for the SPI bus. I just 
know the MPC251x, which behaves badly (message losses) under Linux, 
especially at hight bit-rates due to insufficient RX buffering. What is 
your experience with that driver for the HI-311x?


Thanks,

Wolfgang.

Am 07.03.2017 um 16:31 schrieb Akshay Bhat:



On 01/17/2017 02:22 PM, Akshay Bhat wrote:

This patch adds support for the Holt HI-311x CAN controller. The HI311x
CAN controller is capable of transmitting and receiving standard data
frames, extended data frames and remote frames. The HI311x interfaces
with the host over SPI.

Datasheet: www.holtic.com/documents/371-hi-3110_v-rev-jpdf.do

Signed-off-by: Akshay Bhat 
---




Hi Marc,

Wanted to check if this patch can be included in the next kernel release
(4.12).

Thanks,
Akshay
--
To unsubscribe from this list: send the line "unsubscribe linux-can" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller driver

2016-08-09 Thread Wolfgang Grandegger


Am 09.08.2016 um 08:10 schrieb Andreas Werner:

On Mon, Aug 08, 2016 at 04:35:34PM +0200, Wolfgang Grandegger wrote:

Am 08.08.2016 um 16:05 schrieb Andreas Werner:

On Mon, Aug 08, 2016 at 02:28:39PM +0200, Wolfgang Grandegger wrote:


---snip---


+
+   ndev = alloc_candev(sizeof(struct men_z192), 1);


You specify here one echo_skb but it's not used anywhere. Local loopback
seems not to be implemented.



Agree with you, will set it to "0".


No, the local loopback is mandetory!



Hm ok, but if i check alloc_candev() in drivers/net/can/dev.c
it is not mandatory. In the Documentation/networking/can.txt
there is also a "should" and a fallback mechnism if the driver
does not support the local loopback.


Well, s/driver/hardware/ ! Local loopback is the preferred mechanism.



Sure...


I'm currently ok with this fallback mechanism.

Anyway I am not sure that the driver can handle the echo skb correctly.
If i understand it correctly, the can_get_echo_skb() is normally called
on a "TX done IRQ" to get the skb and loop it back.
I do not have such a "TX done IRQ" and have not implemented implemented
and added the local looback.


What does "MEN_Z192_TFLG_TXIF" signal?



It is not a "TX Done" IRQ, it is the tx buffer level IRQ.
The IRQ is triggered when the number of available tx buffer entries is as
configured with txlvl. (after the buffer was full)

Example:
txlvl = 0
tx buffer has 255 entries.

-> The IRQ is triggered as soon as 1 frame got transmitted (254 entries).

---

txlvl = 254
tx buffer has 255 entries.

-> The IRQ is triggered as soon as the buffer has one entry and it got 
transmitted



May be I can put and get the echo skb within the xmit function?
Does this make sense?


It only makes sense if the driver knows when one or more transfers are done.



Then i do not think that I can use the txlvl IRQ in this case and need to use
the fallback mechanism.



You could store "MEN_Z192_TX_BUF_CNT(readl(>rx_tx_sts))" in the 
start_xmit function and check again in the isr function to find out how 
much transfers have been transmitted in the meantime. Does it make sense?


Wolfgang.

Re: [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller driver

2016-08-08 Thread Wolfgang Grandegger


Am 08.08.2016 um 16:05 schrieb Andreas Werner:

On Mon, Aug 08, 2016 at 02:28:39PM +0200, Wolfgang Grandegger wrote:

Hello,

Am 08.08.2016 um 13:39 schrieb Andreas Werner:

On Mon, Aug 08, 2016 at 11:27:25AM +0200, Wolfgang Grandegger wrote:

Hello Andreas,

a first quick review

Am 26.07.2016 um 11:16 schrieb Andreas Werner:

This CAN Controller is found on MEN Chameleon FPGAs.

The driver/device supports the CAN2.0 specification.
There are 255 RX and 255 Tx buffer within the IP. The
pointer for the buffer are handled by HW to make the
access from within the driver as simple as possible.

The driver also supports parameters to configure the
buffer level interrupt for RX/TX as well as a RX timeout
interrupt.

With this configuration options, the driver/device
provides flexibility for different types of usecases.

Signed-off-by: Andreas Werner <andreas.wer...@men.de>
---
drivers/net/can/Kconfig|  10 +
drivers/net/can/Makefile   |   1 +
drivers/net/can/men_z192_can.c | 989 +
3 files changed, 1000 insertions(+)
create mode 100644 drivers/net/can/men_z192_can.c


---snip---


+/* Buffer level control values */
+#define MEN_Z192_MIN_BUF_LVL   0
+#define MEN_Z192_MAX_BUF_LVL   254
+#define MEN_Z192_RX_BUF_LVL_DEF5
+#define MEN_Z192_TX_BUF_LVL_DEF5
+#define MEN_Z192_RX_TOUT_MIN   0
+#define MEN_Z192_RX_TOUT_MAX   65535
+#define MEN_Z192_RX_TOUT_DEF   1000
+
+static int txlvl = MEN_Z192_TX_BUF_LVL_DEF;
+module_param(txlvl, int, S_IRUGO);
+MODULE_PARM_DESC(txlvl, "TX IRQ trigger level (in frames) 0-254, default="
+__MODULE_STRING(MEN_Z192_TX_BUF_LVL_DEF) ")");
+
+static int rxlvl = MEN_Z192_RX_BUF_LVL_DEF;
+module_param(rxlvl, int, S_IRUGO);
+MODULE_PARM_DESC(rxlvl, "RX IRQ trigger level (in frames) 0-254, default="
+__MODULE_STRING(MEN_Z192_RX_BUF_LVL_DEF) ")");
+


What impact does the level have on the latency? Could you please add some
comments.


It has a impact on the latency.
rxlvl = 0 -> if one frame got received, a IRQ will be generated
rxlvl = 254 -> if 255 frames got received, a IRQ will be generated


Well, what's your usecase for rxlvl > 0? For me it's not obvious what it can
be good for. The application usually wants the message as soon as possible.
Anyway, the default should be *0*. For RX and TX.



The HW provides such feature and the driver should be able to control it.
It was developed to control the IRQ load (like NAPI) by defining a level of the 
buffer
when the IRQ got asserted.

I aggree with you to set the default to "0" which is the main usecase.


+static int rx_timeout = MEN_Z192_RX_TOUT_DEF;
+module_param(rx_timeout, int, S_IRUGO);
+MODULE_PARM_DESC(rx_timeout, "RX IRQ timeout (in 100usec steps), default="
+__MODULE_STRING(MEN_Z192_RX_TOUT_DEF) ")");


Ditto. What is "rx_timeout" good for.



The rx timeout is used im combination with the rxlvl to assert the
if the buffer level is not reached within this timeout.


What event will the application receive in case of a timeout.



Its just to control the time when the RX IRQ will be asserted if the buffer
level is not reached.
Imagine if the rx_timeout is not existing and the rxlvl is set to 50 and
only 30 packets are received. The RX IRQ will be never asserted.

By defining the rx_timeout, we can control the time when the RX IRQ is asserted
if the buffer level is not reached.

The application does not receive any special signal, its just the RX IRQ.


Now I got it. After timeout an interrupt will be trigger regardless of 
the thresholds. The default settings should result in minimum latencies.



Both, the timeout and the level are used to give the user as much
control over the latency and the IRQ handling as possible.
With this two options, the driver can be configured for different
use cases.

I will add this as the comment to make it more clear.


Even a bit more would be appreciated.



Sure...



---snip---


+static int men_z192_read_frame(struct net_device *ndev, unsigned int frame_nr)
+{
+   struct net_device_stats *stats = >stats;
+   struct men_z192 *priv = netdev_priv(ndev);
+   struct men_z192_cf_buf __iomem *cf_buf;
+   struct can_frame *cf;
+   struct sk_buff *skb;
+   u32 cf_offset;
+   u32 length;
+   u32 data;
+   u32 id;
+
+   skb = alloc_can_skb(ndev, );
+   if (unlikely(!skb)) {
+   stats->rx_dropped++;
+   return 0;
+   }
+
+   cf_offset = sizeof(struct men_z192_cf_buf) * frame_nr;
+
+   cf_buf = priv->dev_base + MEN_Z192_RX_BUF_START + cf_offset;
+   length = readl(_buf->length) & MEN_Z192_CFBUF_LEN;
+   id = readl(_buf->can_id);
+
+   if (id & MEN_Z192_CFBUF_IDE) {
+   /* Extended frame */
+   cf->can_id = (id & MEN_Z192_CFBUF_ID1) >> 3;
+

Re: [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller driver

2016-08-08 Thread Wolfgang Grandegger


Hello,

Am 08.08.2016 um 13:39 schrieb Andreas Werner:

On Mon, Aug 08, 2016 at 11:27:25AM +0200, Wolfgang Grandegger wrote:

Hello Andreas,

a first quick review

Am 26.07.2016 um 11:16 schrieb Andreas Werner:

This CAN Controller is found on MEN Chameleon FPGAs.

The driver/device supports the CAN2.0 specification.
There are 255 RX and 255 Tx buffer within the IP. The
pointer for the buffer are handled by HW to make the
access from within the driver as simple as possible.

The driver also supports parameters to configure the
buffer level interrupt for RX/TX as well as a RX timeout
interrupt.

With this configuration options, the driver/device
provides flexibility for different types of usecases.

Signed-off-by: Andreas Werner <andreas.wer...@men.de>
---
drivers/net/can/Kconfig|  10 +
drivers/net/can/Makefile   |   1 +
drivers/net/can/men_z192_can.c | 989 +
3 files changed, 1000 insertions(+)
create mode 100644 drivers/net/can/men_z192_can.c


---snip---


+/* Buffer level control values */
+#define MEN_Z192_MIN_BUF_LVL   0
+#define MEN_Z192_MAX_BUF_LVL   254
+#define MEN_Z192_RX_BUF_LVL_DEF5
+#define MEN_Z192_TX_BUF_LVL_DEF5
+#define MEN_Z192_RX_TOUT_MIN   0
+#define MEN_Z192_RX_TOUT_MAX   65535
+#define MEN_Z192_RX_TOUT_DEF   1000
+
+static int txlvl = MEN_Z192_TX_BUF_LVL_DEF;
+module_param(txlvl, int, S_IRUGO);
+MODULE_PARM_DESC(txlvl, "TX IRQ trigger level (in frames) 0-254, default="
+__MODULE_STRING(MEN_Z192_TX_BUF_LVL_DEF) ")");
+
+static int rxlvl = MEN_Z192_RX_BUF_LVL_DEF;
+module_param(rxlvl, int, S_IRUGO);
+MODULE_PARM_DESC(rxlvl, "RX IRQ trigger level (in frames) 0-254, default="
+__MODULE_STRING(MEN_Z192_RX_BUF_LVL_DEF) ")");
+


What impact does the level have on the latency? Could you please add some
comments.


It has a impact on the latency.
rxlvl = 0 -> if one frame got received, a IRQ will be generated
rxlvl = 254 -> if 255 frames got received, a IRQ will be generated


Well, what's your usecase for rxlvl > 0? For me it's not obvious what it 
can be good for. The application usually wants the message as soon as 
possible. Anyway, the default should be *0*. For RX and TX.



+static int rx_timeout = MEN_Z192_RX_TOUT_DEF;
+module_param(rx_timeout, int, S_IRUGO);
+MODULE_PARM_DESC(rx_timeout, "RX IRQ timeout (in 100usec steps), default="
+__MODULE_STRING(MEN_Z192_RX_TOUT_DEF) ")");


Ditto. What is "rx_timeout" good for.



The rx timeout is used im combination with the rxlvl to assert the
if the buffer level is not reached within this timeout.


What event will the application receive in case of a timeout.


Both, the timeout and the level are used to give the user as much
control over the latency and the IRQ handling as possible.
With this two options, the driver can be configured for different
use cases.

>

I will add this as the comment to make it more clear.


Even a bit more would be appreciated.


---snip---


+static int men_z192_read_frame(struct net_device *ndev, unsigned int frame_nr)
+{
+   struct net_device_stats *stats = >stats;
+   struct men_z192 *priv = netdev_priv(ndev);
+   struct men_z192_cf_buf __iomem *cf_buf;
+   struct can_frame *cf;
+   struct sk_buff *skb;
+   u32 cf_offset;
+   u32 length;
+   u32 data;
+   u32 id;
+
+   skb = alloc_can_skb(ndev, );
+   if (unlikely(!skb)) {
+   stats->rx_dropped++;
+   return 0;
+   }
+
+   cf_offset = sizeof(struct men_z192_cf_buf) * frame_nr;
+
+   cf_buf = priv->dev_base + MEN_Z192_RX_BUF_START + cf_offset;
+   length = readl(_buf->length) & MEN_Z192_CFBUF_LEN;
+   id = readl(_buf->can_id);
+
+   if (id & MEN_Z192_CFBUF_IDE) {
+   /* Extended frame */
+   cf->can_id = (id & MEN_Z192_CFBUF_ID1) >> 3;
+   cf->can_id |= (id & MEN_Z192_CFBUF_ID2) >>
+   MEN_Z192_CFBUF_ID2_SHIFT;
+
+   cf->can_id |= CAN_EFF_FLAG;
+
+   if (id & MEN_Z192_CFBUF_E_RTR)
+   cf->can_id |= CAN_RTR_FLAG;
+   } else {
+   /* Standard frame */
+   cf->can_id = (id & MEN_Z192_CFBUF_ID1) >>
+   MEN_Z192_CFBUF_ID1_SHIFT;
+
+   if (id & MEN_Z192_CFBUF_S_RTR)
+   cf->can_id |= CAN_RTR_FLAG;
+   }
+
+   cf->can_dlc = get_can_dlc(length);
+
+   /* remote transmission request frame
+* contains no data field even if the
+* data length is set to a value > 0
+*/
+   if (!(cf->can_id & CAN_RTR_FLAG)) {
+   if (cf->can_dlc > 0) {
+   data = readl(_buf->data[0]);
+

Re: [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller driver

2016-08-08 Thread Wolfgang Grandegger


Hello Andreas,

a first quick review

Am 26.07.2016 um 11:16 schrieb Andreas Werner:

This CAN Controller is found on MEN Chameleon FPGAs.

The driver/device supports the CAN2.0 specification.
There are 255 RX and 255 Tx buffer within the IP. The
pointer for the buffer are handled by HW to make the
access from within the driver as simple as possible.

The driver also supports parameters to configure the
buffer level interrupt for RX/TX as well as a RX timeout
interrupt.

With this configuration options, the driver/device
provides flexibility for different types of usecases.

Signed-off-by: Andreas Werner 
---
 drivers/net/can/Kconfig|  10 +
 drivers/net/can/Makefile   |   1 +
 drivers/net/can/men_z192_can.c | 989 +
 3 files changed, 1000 insertions(+)
 create mode 100644 drivers/net/can/men_z192_can.c

diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
index 0d40aef..0fa0387 100644
--- a/drivers/net/can/Kconfig
+++ b/drivers/net/can/Kconfig
@@ -104,6 +104,16 @@ config CAN_JANZ_ICAN3
  This driver can also be built as a module. If so, the module will be
  called janz-ican3.ko.

+config CAN_MEN_Z192
+   tristate "MEN 16Z192-00 CAN Controller"
+   depends on MCB
+   ---help---
+ Driver for MEN 16Z192-00 CAN Controller IP-Core, which
+ is connected to the MEN Chameleon Bus.
+
+ This driver can also be built as a module. If so, the module will be
+ called men_z192_can.ko.
+
 config CAN_RCAR
tristate "Renesas R-Car CAN controller"
depends on ARCH_RENESAS || ARM
diff --git a/drivers/net/can/Makefile b/drivers/net/can/Makefile
index e3db0c8..eb206b3 100644
--- a/drivers/net/can/Makefile
+++ b/drivers/net/can/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_CAN_FLEXCAN) += flexcan.o
 obj-$(CONFIG_CAN_GRCAN)+= grcan.o
 obj-$(CONFIG_CAN_IFI_CANFD)+= ifi_canfd/
 obj-$(CONFIG_CAN_JANZ_ICAN3)   += janz-ican3.o
+obj-$(CONFIG_CAN_MEN_Z192) += men_z192_can.o
 obj-$(CONFIG_CAN_MSCAN)+= mscan/
 obj-$(CONFIG_CAN_M_CAN)+= m_can/
 obj-$(CONFIG_CAN_RCAR) += rcar_can.o
diff --git a/drivers/net/can/men_z192_can.c b/drivers/net/can/men_z192_can.c
new file mode 100644
index 000..b39ffee
--- /dev/null
+++ b/drivers/net/can/men_z192_can.c
@@ -0,0 +1,989 @@
+/*
+ * MEN 16Z192 CAN Controller driver
+ *
+ * Copyright (C) 2016 MEN Mikroelektronik GmbH (www.men.de)
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; version 2 of the License.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define DRV_NAME   "z192_can"
+
+#define MEN_Z192_NAPI_WEIGHT   64
+#define MEN_Z192_MODE_TOUT_US  40
+
+/* CTL/BTR Register Bits */
+#define MEN_Z192_CTL0_INITRQ   BIT(0)
+#define MEN_Z192_CTL0_SLPRQBIT(1)
+#define MEN_Z192_CTL1_INITAK   BIT(8)
+#define MEN_Z192_CTL1_SLPAKBIT(9)
+#define MEN_Z192_CTL1_LISTEN   BIT(12)
+#define MEN_Z192_CTL1_LOOPBBIT(13)
+#define MEN_Z192_CTL1_CANE BIT(15)
+#define MEN_Z192_BTR0_BRP(x)   (((x) & 0x3f) << 16)
+#define MEN_Z192_BTR0_SJW(x)   (((x) & 0x03) << 22)
+#define MEN_Z192_BTR1_TSEG1(x) (((x) & 0x0f) << 24)
+#define MEN_Z192_BTR1_TSEG2(x) (((x) & 0x07) << 28)
+#define MEN_Z192_BTR1_SAMP BIT(31)
+
+/* IER Interrupt Enable Register bits */
+#define MEN_Z192_RXIE  BIT(0)
+#define MEN_Z192_OVRIE BIT(1)
+#define MEN_Z192_CSCIE BIT(6)
+#define MEN_Z192_TOUTE BIT(7)
+#define MEN_Z192_TXIE  BIT(16)
+#define MEN_Z192_ERRIE BIT(17)
+
+#define MEN_Z192_IRQ_ALL   \
+   (MEN_Z192_RXIE | MEN_Z192_OVRIE |   \
+MEN_Z192_CSCIE | MEN_Z192_TOUTE |  \
+MEN_Z192_TXIE)
+
+#define MEN_Z192_IRQ_NAPI  (MEN_Z192_RXIE | MEN_Z192_TOUTE)
+
+/* RX_TX_STAT RX/TX Status status register bits */
+#define MEN_Z192_RX_BUF_CNT(x) ((x) & 0xff)
+#define MEN_Z192_TX_BUF_CNT(x) (((x) & 0xff00) >> 8)
+#defineMEN_Z192_RFLG_RXIF  BIT(16)
+#defineMEN_Z192_RFLG_OVRF  BIT(17)
+#defineMEN_Z192_RFLG_TSTATEGENMASK(19, 18)
+#defineMEN_Z192_RFLG_RSTATEGENMASK(21, 20)
+#defineMEN_Z192_RFLG_CSCIF BIT(22)
+#defineMEN_Z192_RFLG_TOUTF BIT(23)
+#define MEN_Z192_TFLG_TXIF BIT(24)
+
+#define MEN_Z192_GET_TSTATE(x) (((x) & MEN_Z192_RFLG_TSTATE) >> 18)
+#define MEN_Z192_GET_RSTATE(x) (((x) & MEN_Z192_RFLG_RSTATE) >> 20)
+
+#define MEN_Z192_IRQ_FLAGS_ALL \
+   (MEN_Z192_RFLG_RXIF | MEN_Z192_RFLG_OVRF |  \
+MEN_Z192_RFLG_TSTATE | MEN_Z192_RFLG_RSTATE |  \
+MEN_Z192_RFLG_CSCIF | MEN_Z192_RFLG_TOUTF |\
+MEN_Z192_TFLG_TXIF)
+
+/*

Re: [PATCH v4 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

Re: [PATCH v4 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

Re: [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller driver

Re: [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller driver

Re: [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller driver

Re: [PATCH RESEND] net: can: Introduce MEN 16Z192-00 CAN controller driver

19 matches

Site Navigation

Mail list logo

Footer information