Re: [PATCH v4 1/1] can: ucan: add driver for Theobroma Systems UCAN devices

[Martin Elshuber]

Hallo Wolfgang,

thank you for the review, we have the update ready but we will test a
while it before sending V5.

Regarding your question on the firmware behavior, find the answer inline
below.
> Hello Jacob,
>
> just a few minor issues and a question about bus-off handling...
>
> Am 03.04.2018 um 16:35 schrieb Jakob Unterwurzacher:
>> The UCAN driver supports the microcontroller-based USB/CAN
>> adapters from Theobroma Systems. There are two form-factors
>> that run essentially the same firmware:
>>
>> * Seal: standalone USB stick ( https://www.theobroma-systems.com/seal )
>>
>> * Mule: integrated on the PCB of various System-on-Modules from
>>   Theobroma Systems like the A31-µQ7 and the RK3399-Q7
>>   ( https://www.theobroma-systems.com/rk3399-q7 )
>>
[..snip..]
>> +
>> +/* we switched into a worse state */
>> +up->can.state = new_state;
>> +switch (new_state) {
>> +case CAN_STATE_BUS_OFF:
>> +can_stats->bus_off++;
>> +can_bus_off(up->netdev);
> How does the CAN controller firmware handle bus-off? Does it recover
> automatically? "can_bus_off()" will restart the CAN controller after
> "restart-ms" or manually via netlink "restart" command by calling
> "ucan_set_mode(CAN_START_MODE)":
>
>   # ip link set canX type can restart-ms 100
>   # ip link set canX type can restart
>
> See also:
>
>   https://elixir.bootlin.com/linux/latest/source/drivers/net/can/dev.c#L597
1) Once a bus-off condition is detected, the device sends an error
   message do indicate the bus state change (the same happens
   also for bus state changes into ERROR_ACTIVE/WARNING/PASSIVE).
2) The device will not restart the bus itself (to give application
   control how to handle this event).
3) The device initiates a bus of recovery upon receiving the control
   message UCAN_COMMAND_RESTART (see ucan_set_mode())
4) The Linux CAN framework handles sending a CAN_ERR_RESTARTED message
   and calling ucan_set_mode().
5) Once the device completes the recovery, it sends a state change
   message indicating that it is in ERROR_ACTIVE state (see 1).
6) During BUS-OFF no error frames or normal frames are set from the
   device to the host.
7) If the device receives a frame from the host when in bus-off (e.g.:
   just after detecting the condition) the device confirms reception by
   sending an TX_COMPLETE with and error indicator and drops the frame.
   Finally the driver calls can_free_echo_skb() upon reception of the
   TX_COMPLETE message (see ucan_tx_complete_msg()).

The protocol documentation will be updated accordingly

[..snip..]
> Thanks for your contribution!
>
> Wolfgang.
>
Best regards
Martin

-- 
Martin Elshuber
Theobroma Systems Design und Consulting GmbH
Seestadtstraße 27 (Aspern IQ), 1220 Wien, Austria
Phone: +43 1 236 98 93-405, Fax: +43 1 236 98 93-9
http://www.theobroma-systems.com

Re: [PATCH v4 1/1] can: ucan: add driver for Theobroma Systems UCAN devices

[Martin Elshuber]

Hallo Wolfgang,

thank you for the review, we have the update ready but we will test a
while it before sending V5.

Regarding your question on the firmware behavior, find the answer inline
below.
> Hello Jacob,
>
> just a few minor issues and a question about bus-off handling...
>
> Am 03.04.2018 um 16:35 schrieb Jakob Unterwurzacher:
>> The UCAN driver supports the microcontroller-based USB/CAN
>> adapters from Theobroma Systems. There are two form-factors
>> that run essentially the same firmware:
>>
>> * Seal: standalone USB stick ( https://www.theobroma-systems.com/seal )
>>
>> * Mule: integrated on the PCB of various System-on-Modules from
>>   Theobroma Systems like the A31-µQ7 and the RK3399-Q7
>>   ( https://www.theobroma-systems.com/rk3399-q7 )
>>
[..snip..]
>> +
>> +/* we switched into a worse state */
>> +up->can.state = new_state;
>> +switch (new_state) {
>> +case CAN_STATE_BUS_OFF:
>> +can_stats->bus_off++;
>> +can_bus_off(up->netdev);
> How does the CAN controller firmware handle bus-off? Does it recover
> automatically? "can_bus_off()" will restart the CAN controller after
> "restart-ms" or manually via netlink "restart" command by calling
> "ucan_set_mode(CAN_START_MODE)":
>
>   # ip link set canX type can restart-ms 100
>   # ip link set canX type can restart
>
> See also:
>
>   https://elixir.bootlin.com/linux/latest/source/drivers/net/can/dev.c#L597
1) Once a bus-off condition is detected, the device sends an error
   message do indicate the bus state change (the same happens
   also for bus state changes into ERROR_ACTIVE/WARNING/PASSIVE).
2) The device will not restart the bus itself (to give application
   control how to handle this event).
3) The device initiates a bus of recovery upon receiving the control
   message UCAN_COMMAND_RESTART (see ucan_set_mode())
4) The Linux CAN framework handles sending a CAN_ERR_RESTARTED message
   and calling ucan_set_mode().
5) Once the device completes the recovery, it sends a state change
   message indicating that it is in ERROR_ACTIVE state (see 1).
6) During BUS-OFF no error frames or normal frames are set from the
   device to the host.
7) If the device receives a frame from the host when in bus-off (e.g.:
   just after detecting the condition) the device confirms reception by
   sending an TX_COMPLETE with and error indicator and drops the frame.
   Finally the driver calls can_free_echo_skb() upon reception of the
   TX_COMPLETE message (see ucan_tx_complete_msg()).

The protocol documentation will be updated accordingly

[..snip..]
> Thanks for your contribution!
>
> Wolfgang.
>
Best regards
Martin

-- 
Martin Elshuber
Theobroma Systems Design und Consulting GmbH
Seestadtstraße 27 (Aspern IQ), 1220 Wien, Austria
Phone: +43 1 236 98 93-405, Fax: +43 1 236 98 93-9
http://www.theobroma-systems.com

Re: [PATCH v4 1/1] can: ucan: add driver for Theobroma Systems UCAN devices

[Wolfgang Grandegger]

Hello Jacob,

just a few minor issues and a question about bus-off handling...

Am 03.04.2018 um 16:35 schrieb Jakob Unterwurzacher:
> The UCAN driver supports the microcontroller-based USB/CAN
> adapters from Theobroma Systems. There are two form-factors
> that run essentially the same firmware:
> 
> * Seal: standalone USB stick ( https://www.theobroma-systems.com/seal )
> 
> * Mule: integrated on the PCB of various System-on-Modules from
>   Theobroma Systems like the A31-µQ7 and the RK3399-Q7
>   ( https://www.theobroma-systems.com/rk3399-q7 )
> 
> The USB wire protocol has been designed to be as generic and
> hardware-indendent as possible in the hope of being useful for
> implementation on other microcontrollers.
> 
> Signed-off-by: Martin Elshuber 
> Signed-off-by: Jakob Unterwurzacher 
> 
> Signed-off-by: Philipp Tomsich 
> ---
>  Documentation/networking/can_ucan_protocol.rst |  315 +
>  Documentation/networking/index.rst |1 +
>  drivers/net/can/usb/Kconfig|   10 +
>  drivers/net/can/usb/Makefile   |1 +
>  drivers/net/can/usb/ucan.c | 1596 
> 
>  5 files changed, 1923 insertions(+)
>  create mode 100644 Documentation/networking/can_ucan_protocol.rst
>  create mode 100644 drivers/net/can/usb/ucan.c
> 
[..snip..]

> diff --git a/drivers/net/can/usb/Kconfig b/drivers/net/can/usb/Kconfig
> index c36f4bdcbf4f..490cdce1f1da 100644
> --- a/drivers/net/can/usb/Kconfig
> +++ b/drivers/net/can/usb/Kconfig
> @@ -89,4 +89,14 @@ config CAN_MCBA_USB
> This driver supports the CAN BUS Analyzer interface
> from Microchip (http://www.microchip.com/development-tools/).
>  
> +config CAN_UCAN
> + tristate "Theobroma Systems UCAN interface"
> + ---help---
> +   This driver supports the Theobroma Systems
> +   UCAN USB-CAN interface.
> +
> +   UCAN is an microcontroller-based USB-CAN interface that
> +   is integrated on System-on-Modules made by Theobroma Systems
> +   (https://www.theobroma-systems.com/som-products).
> +
>  endmenu
> diff --git a/drivers/net/can/usb/Makefile b/drivers/net/can/usb/Makefile
> index 49ac7b99ba32..4176e8358232 100644
> --- a/drivers/net/can/usb/Makefile
> +++ b/drivers/net/can/usb/Makefile
> @@ -10,3 +10,4 @@ obj-$(CONFIG_CAN_KVASER_USB) += kvaser_usb.o
>  obj-$(CONFIG_CAN_PEAK_USB) += peak_usb/
>  obj-$(CONFIG_CAN_8DEV_USB) += usb_8dev.o
>  obj-$(CONFIG_CAN_MCBA_USB) += mcba_usb.o
> +obj-$(CONFIG_CAN_UCAN) += ucan.o
> diff --git a/drivers/net/can/usb/ucan.c b/drivers/net/can/usb/ucan.c
> new file mode 100644
> index ..bb2d62dcbd7b
> --- /dev/null
> +++ b/drivers/net/can/usb/ucan.c
> @@ -0,0 +1,1596 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/* Driver for Theobroma Systems UCAN devices, Protocol Version 3
> + *
> + * Copyright (C) 2018 Theobroma Systems Design und Consulting GmbH
> + *
> + *
> + * General Description:
> + *
> + * The USB Device uses three Endpoints:
> + *
> + *   CONTROL Endpoint: Is used the setup the device (start, stop,
> + *   info, configure).
> + *
> + *   IN Endpoint: The device sends CAN Frame Messages and Device
> + *   Information using the IN endpoint.
> + *
> + *   OUT Endpoint: The driver sends configuration requests, and CAN
> + *   Frames on the out endpoint.
> + *
> + * Error Handling:
> + *
> + *   If error reporting is turned on the device encodes error into CAN
> + *   error frames (see uapi/linux/can/error.h) and sends it using the
> + *   IN Endpoint. The driver updates statistics and forward it.
> + */
> +
> +#include 

[...snip...]

> +static struct ucan_urb_context *ucan_alloc_context(struct ucan_priv *up)
> +{
> + int i;
> + unsigned long flags;
> + struct ucan_urb_context *ret = NULL;
> +
> + if (WARN_ON_ONCE(!up->context_array))
> + return NULL;
> +
> + /* execute context operation atomically */
> + spin_lock_irqsave(>context_lock, flags);
> +
> + for (i = 0; i < up->device_info.tx_fifo; i++) {
> + if (!up->context_array[i].allocated) {
> + /* update context */
> + ret = >context_array[i];
> + up->context_array[i].allocated = true;
> +
> + /* stop queue if necessary */
> + up->available_tx_urbs--;
> + if (!up->available_tx_urbs)
> + netif_stop_queue(up->netdev);
> +
> + goto done_restore;

break instead of goto!?

> + }
> + }
> +
> +done_restore:
> + spin_unlock_irqrestore(>context_lock, flags);
> + return ret;
> +}
> +
> +static bool ucan_release_context(struct ucan_priv *up,
> +  struct ucan_urb_context *ctx)
> +{
> + unsigned long flags;
> + bool ret = false;
> +
> + if 

Re: [PATCH v4 1/1] can: ucan: add driver for Theobroma Systems UCAN devices

[Wolfgang Grandegger]

Hello Jacob,

just a few minor issues and a question about bus-off handling...

Am 03.04.2018 um 16:35 schrieb Jakob Unterwurzacher:
> The UCAN driver supports the microcontroller-based USB/CAN
> adapters from Theobroma Systems. There are two form-factors
> that run essentially the same firmware:
> 
> * Seal: standalone USB stick ( https://www.theobroma-systems.com/seal )
> 
> * Mule: integrated on the PCB of various System-on-Modules from
>   Theobroma Systems like the A31-µQ7 and the RK3399-Q7
>   ( https://www.theobroma-systems.com/rk3399-q7 )
> 
> The USB wire protocol has been designed to be as generic and
> hardware-indendent as possible in the hope of being useful for
> implementation on other microcontrollers.
> 
> Signed-off-by: Martin Elshuber 
> Signed-off-by: Jakob Unterwurzacher 
> 
> Signed-off-by: Philipp Tomsich 
> ---
>  Documentation/networking/can_ucan_protocol.rst |  315 +
>  Documentation/networking/index.rst |1 +
>  drivers/net/can/usb/Kconfig|   10 +
>  drivers/net/can/usb/Makefile   |1 +
>  drivers/net/can/usb/ucan.c | 1596 
> 
>  5 files changed, 1923 insertions(+)
>  create mode 100644 Documentation/networking/can_ucan_protocol.rst
>  create mode 100644 drivers/net/can/usb/ucan.c
> 
[..snip..]

> diff --git a/drivers/net/can/usb/Kconfig b/drivers/net/can/usb/Kconfig
> index c36f4bdcbf4f..490cdce1f1da 100644
> --- a/drivers/net/can/usb/Kconfig
> +++ b/drivers/net/can/usb/Kconfig
> @@ -89,4 +89,14 @@ config CAN_MCBA_USB
> This driver supports the CAN BUS Analyzer interface
> from Microchip (http://www.microchip.com/development-tools/).
>  
> +config CAN_UCAN
> + tristate "Theobroma Systems UCAN interface"
> + ---help---
> +   This driver supports the Theobroma Systems
> +   UCAN USB-CAN interface.
> +
> +   UCAN is an microcontroller-based USB-CAN interface that
> +   is integrated on System-on-Modules made by Theobroma Systems
> +   (https://www.theobroma-systems.com/som-products).
> +
>  endmenu
> diff --git a/drivers/net/can/usb/Makefile b/drivers/net/can/usb/Makefile
> index 49ac7b99ba32..4176e8358232 100644
> --- a/drivers/net/can/usb/Makefile
> +++ b/drivers/net/can/usb/Makefile
> @@ -10,3 +10,4 @@ obj-$(CONFIG_CAN_KVASER_USB) += kvaser_usb.o
>  obj-$(CONFIG_CAN_PEAK_USB) += peak_usb/
>  obj-$(CONFIG_CAN_8DEV_USB) += usb_8dev.o
>  obj-$(CONFIG_CAN_MCBA_USB) += mcba_usb.o
> +obj-$(CONFIG_CAN_UCAN) += ucan.o
> diff --git a/drivers/net/can/usb/ucan.c b/drivers/net/can/usb/ucan.c
> new file mode 100644
> index ..bb2d62dcbd7b
> --- /dev/null
> +++ b/drivers/net/can/usb/ucan.c
> @@ -0,0 +1,1596 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/* Driver for Theobroma Systems UCAN devices, Protocol Version 3
> + *
> + * Copyright (C) 2018 Theobroma Systems Design und Consulting GmbH
> + *
> + *
> + * General Description:
> + *
> + * The USB Device uses three Endpoints:
> + *
> + *   CONTROL Endpoint: Is used the setup the device (start, stop,
> + *   info, configure).
> + *
> + *   IN Endpoint: The device sends CAN Frame Messages and Device
> + *   Information using the IN endpoint.
> + *
> + *   OUT Endpoint: The driver sends configuration requests, and CAN
> + *   Frames on the out endpoint.
> + *
> + * Error Handling:
> + *
> + *   If error reporting is turned on the device encodes error into CAN
> + *   error frames (see uapi/linux/can/error.h) and sends it using the
> + *   IN Endpoint. The driver updates statistics and forward it.
> + */
> +
> +#include 

[...snip...]

> +static struct ucan_urb_context *ucan_alloc_context(struct ucan_priv *up)
> +{
> + int i;
> + unsigned long flags;
> + struct ucan_urb_context *ret = NULL;
> +
> + if (WARN_ON_ONCE(!up->context_array))
> + return NULL;
> +
> + /* execute context operation atomically */
> + spin_lock_irqsave(>context_lock, flags);
> +
> + for (i = 0; i < up->device_info.tx_fifo; i++) {
> + if (!up->context_array[i].allocated) {
> + /* update context */
> + ret = >context_array[i];
> + up->context_array[i].allocated = true;
> +
> + /* stop queue if necessary */
> + up->available_tx_urbs--;
> + if (!up->available_tx_urbs)
> + netif_stop_queue(up->netdev);
> +
> + goto done_restore;

break instead of goto!?

> + }
> + }
> +
> +done_restore:
> + spin_unlock_irqrestore(>context_lock, flags);
> + return ret;
> +}
> +
> +static bool ucan_release_context(struct ucan_priv *up,
> +  struct ucan_urb_context *ctx)
> +{
> + unsigned long flags;
> + bool ret = false;
> +
> + if (WARN_ON_ONCE(!up->context_array))
> + return false;
> +
> + /* execute context operation atomically */
> +