[PATCH 00/17] Add support for CPC-USB/FD CAN FD interface

[PATCH 00/17] Add support for CPC-USB/FD CAN FD interface

[Gerhard Uttenthaler]

These patches extend the ems_usb.c driver to support both devices, the
classic CAN CPC-USB/ARM7 and the CAN FD CPC-USB/FD. Also support for
the listen only mode for CPC-USB/ARM7 was implemented. Most issues given
by checkpatch.pl were resolved.

Gerhard Uttenthaler (17):
  can: ems_usb: Fixed warnings given by checkpatch.pl and fixed some
    outdated comments
  can: ems_usb: Added defines and product id needed for CPC-USB/FD
  can: ems_usb: Added CAN FD message representation
  can: ems_usb: Added struct used for CAN FD initialization
  can: ems_usb: Replace constant define RX_BUFFER_SIZE by variable
    bulk_rd_buf_size, because this will have different values for
    CPC-USB/ARM7 and CPC-USB/FD. For the same reason added a function
    pointer ems_usb_write_mode. In device probe function added a switch
    statement to select between CPC-USB/ARM7 and CPC-USB/FD and
    rearranged initialization sequence accordingly.
  can: ems_usb: Added listen only mode for CPC-USB/ARM7 and moved
    evaluation of can.ctrlmode from set_bittiming routine to
    ems_usb_write_mode_arm7 routine. Wrapped a long line.
  can: ems_usb: Added CPC_ClearCmdQueue routine to get access to the
    interface even it is flooded with messages which cannot successfully
    be sent. Set timestamp to 0 in ems_usb_control_cmd.
  can: ems_usb: Modified ems_usb_read_bulk_callback to be able to handle
    also CPC-USB/FD
  can: ems_usb: For CPC-USB/FD added clock definitions, bittiming
    constants, set_bittiming functions, bittiming init function and add
    all that to probe function
  can: ems_usb: Added receive routine for CAN FD messages and its call
    in ems_usb_read_bulk_callback
  can: ems_usb: Added ems_usb_write_mode_fd and its call in device probe
    routine. Fixed indentation.
  can: ems_usb: In ems_usb_start_xmit send only bytes with valid content
    to interface and not the complete buffer. Set first four bytes of
    buffer to 0. Wrapped long lines.
  can: ems_usb: Rearrange code in ems_usb_start_xmit to check with
    can_is_canfd_skb for CAN FD frames.
  can: ems_usb: Added code to handle CAN FD frames in ems_usb_start_xmit
  can: ems_usb: In CAN error handling routine checking which CAN
    controller type is issuing the error
  can: ems_usb: Added error handling part for CPC-USB/FD. Get some
    structures packed
  can: ems_usb: Made another struct packed. Enable CPC-USB/FD by adding
    it to the drivers device table

 drivers/net/can/usb/ems_usb.c | 860 +++++++++++++++++++++++++++-------
 1 file changed, 681 insertions(+), 179 deletions(-)

-- 
2.26.2

-- 
EMS Dr. Thomas Wuensche e.K.
Sonnenhang 3
85304 Ilmmuenster
HR Ingolstadt, HRA 170106

Phone: +49-8441-490260
Fax  : +49-8441-81860
http://www.ems-wuensche.com

[PATCH 01/17] can: ems_usb: Fixed warnings given by checkpatch.pl and fixed some outdated comments

[Gerhard Uttenthaler]

Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
---
 drivers/net/can/usb/ems_usb.c | 74 ++++++++++++++---------------------
 1 file changed, 30 insertions(+), 44 deletions(-)

diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
index 4f52810bebf8..c6ea82b01418 100644
--- a/drivers/net/can/usb/ems_usb.c
+++ b/drivers/net/can/usb/ems_usb.c
@@ -1,8 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
-/*
- * CAN driver for EMS Dr. Thomas Wuensche CPC-USB/ARM7
+/* CAN driver for EMS Dr. Thomas Wuensche CPC-USB/ARM7
  *
- * Copyright (C) 2004-2009 EMS Dr. Thomas Wuensche
+ * Copyright (C) 2004-2020 EMS Dr. Thomas Wuensche
  */
 #include <linux/signal.h>
 #include <linux/slab.h>
@@ -15,6 +14,7 @@
 #include <linux/can/error.h>
 
 MODULE_AUTHOR("Sebastian Haas <haas@ems-wuensche.com>");
+MODULE_AUTHOR("Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>");
 MODULE_DESCRIPTION("CAN driver for EMS Dr. Thomas Wuensche CAN/USB interfaces");
 MODULE_LICENSE("GPL v2");
 
@@ -55,7 +55,7 @@ MODULE_LICENSE("GPL v2");
 #define CPC_CMD_TYPE_CLEAR_MSG_QUEUE 8  /* clear CPC_MSG queue */
 #define CPC_CMD_TYPE_CLEAR_CMD_QUEUE 28 /* clear CPC_CMD queue */
 
-#define CPC_CC_TYPE_SJA1000 2 /* Philips basic CAN controller */
+#define CPC_CC_TYPE_SJA1000 2 /* NXP CAN controller */
 
 #define CPC_CAN_ECODE_ERRFRAME 0x01 /* Ecode type */
 
@@ -64,8 +64,7 @@ MODULE_LICENSE("GPL v2");
 #define CPC_OVR_EVENT_CANSTATE  0x02
 #define CPC_OVR_EVENT_BUSERROR  0x04
 
-/*
- * If the CAN controller lost a message we indicate it with the highest bit
+/* If the CAN controller lost a message we indicate it with the highest bit
  * set in the count field.
  */
 #define CPC_OVR_HW 0x80
@@ -98,8 +97,7 @@ MODULE_LICENSE("GPL v2");
 
 #define SJA1000_DEFAULT_OUTPUT_CONTROL 0xDA
 
-/*
- * The device actually uses a 16MHz clock to generate the CAN clock
+/* CPC-USB/ARM7 actually uses a 16MHz clock to generate the CAN clock
  * but it expects SJA1000 bit settings based on 8MHz (is internally
  * converted).
  */
@@ -108,8 +106,7 @@ MODULE_LICENSE("GPL v2");
 #define CPC_TX_QUEUE_TRIGGER_LOW	25
 #define CPC_TX_QUEUE_TRIGGER_HIGH	35
 
-/*
- * CAN-Message representation in a CPC_MSG. Message object type is
+/* CAN-Message representation in a CPC_MSG. Message object type is
  * CPC_MSG_TYPE_CAN_FRAME or CPC_MSG_TYPE_RTR_FRAME or
  * CPC_MSG_TYPE_EXT_CAN_FRAME or CPC_MSG_TYPE_EXT_RTR_FRAME.
  */
@@ -139,7 +136,6 @@ struct cpc_sja1000_params {
 struct cpc_can_params {
 	u8 cc_type;
 
-	/* Will support M16C CAN controller in the future */
 	union {
 		struct cpc_sja1000_params sja1000;
 	} cc_params;
@@ -177,8 +173,7 @@ struct cpc_can_error {
 	} cc;
 };
 
-/*
- * Structure containing RX/TX error counter. This structure is used to request
+/* Structure containing RX/TX error counter. This structure is used to request
  * the values of the CAN controllers TX and RX error counter.
  */
 struct cpc_can_err_counter {
@@ -206,8 +201,7 @@ struct __packed ems_cpc_msg {
 	} msg;
 };
 
-/*
- * Table of devices that work with this driver
+/* Table of devices that work with this driver
  * NOTE: This driver supports only CPC-USB/ARM7 (LPC2119) yet.
  */
 static struct usb_device_id ems_usb_table[] = {
@@ -302,7 +296,7 @@ static void ems_usb_rx_can_msg(struct ems_usb *dev, struct ems_cpc_msg *msg)
 	struct net_device_stats *stats = &dev->netdev->stats;
 
 	skb = alloc_can_skb(dev->netdev, &cf);
-	if (skb == NULL)
+	if (!skb)
 		return;
 
 	cf->can_id = le32_to_cpu(msg->msg.can_msg.id);
@@ -332,7 +326,7 @@ static void ems_usb_rx_err(struct ems_usb *dev, struct ems_cpc_msg *msg)
 	struct net_device_stats *stats = &dev->netdev->stats;
 
 	skb = alloc_can_err_skb(dev->netdev, &cf);
-	if (skb == NULL)
+	if (!skb)
 		return;
 
 	if (msg->type == CPC_MSG_TYPE_CAN_STATE) {
@@ -400,8 +394,7 @@ static void ems_usb_rx_err(struct ems_usb *dev, struct ems_cpc_msg *msg)
 	netif_rx(skb);
 }
 
-/*
- * callback for bulk IN urb
+/* Callback for bulk IN urb
  */
 static void ems_usb_read_bulk_callback(struct urb *urb)
 {
@@ -486,8 +479,7 @@ static void ems_usb_read_bulk_callback(struct urb *urb)
 			   "failed resubmitting read bulk urb: %d\n", retval);
 }
 
-/*
- * callback for bulk IN urb
+/* Callback for bulk IN urb
  */
 static void ems_usb_write_bulk_callback(struct urb *urb)
 {
@@ -495,15 +487,19 @@ static void ems_usb_write_bulk_callback(struct urb *urb)
 	struct ems_usb *dev;
 	struct net_device *netdev;
 
-	BUG_ON(!context);
-
-	dev = context->dev;
-	netdev = dev->netdev;
-
 	/* free up our allocated buffer */
 	usb_free_coherent(urb->dev, urb->transfer_buffer_length,
 			  urb->transfer_buffer, urb->transfer_dma);
 
+	if (!context) {
+		// Should definitely not happen
+		printk(KERN_ERR "%s with no context\n", __func__);
+		return;
+	}
+
+	dev = context->dev;
+	netdev = dev->netdev;
+
 	atomic_dec(&dev->active_tx_urbs);
 
 	if (!netif_device_present(netdev))
@@ -522,11 +518,9 @@ static void ems_usb_write_bulk_callback(struct urb *urb)
 
 	/* Release context */
 	context->echo_index = MAX_TX_URBS;
-
 }
 
-/*
- * Send the given CPC command synchronously
+/* Send the given CPC command synchronously
  */
 static int ems_usb_command_msg(struct ems_usb *dev, struct ems_cpc_msg *msg)
 {
@@ -545,8 +539,7 @@ static int ems_usb_command_msg(struct ems_usb *dev, struct ems_cpc_msg *msg)
 			    &actual_length, 1000);
 }
 
-/*
- * Change CAN controllers' mode register
+/* Change CAN controllers' mode register
  */
 static int ems_usb_write_mode(struct ems_usb *dev, u8 mode)
 {
@@ -555,8 +548,7 @@ static int ems_usb_write_mode(struct ems_usb *dev, u8 mode)
 	return ems_usb_command_msg(dev, &dev->active_params);
 }
 
-/*
- * Send a CPC_Control command to change behaviour when interface receives a CAN
+/* Send a CPC_Control command to change behaviour when interface receives a CAN
  * message, bus error or CAN state changed notifications.
  */
 static int ems_usb_control_cmd(struct ems_usb *dev, u8 val)
@@ -573,8 +565,7 @@ static int ems_usb_control_cmd(struct ems_usb *dev, u8 val)
 	return ems_usb_command_msg(dev, &cmd);
 }
 
-/*
- * Start interface
+/* Start interface
  */
 static int ems_usb_start(struct ems_usb *dev)
 {
@@ -718,7 +709,6 @@ static int ems_usb_open(struct net_device *netdev)
 		return err;
 	}
 
-
 	netif_start_queue(netdev);
 
 	return 0;
@@ -779,8 +769,7 @@ static netdev_tx_t ems_usb_start_xmit(struct sk_buff *skb, struct net_device *ne
 		}
 	}
 
-	/*
-	 * May never happen! When this happens we'd more URBs in flight as
+	/* May never happen! When this happens we'd more URBs in flight as
 	 * allowed (MAX_TX_URBS).
 	 */
 	if (!context) {
@@ -832,8 +821,7 @@ static netdev_tx_t ems_usb_start_xmit(struct sk_buff *skb, struct net_device *ne
 		}
 	}
 
-	/*
-	 * Release our reference to this URB, the USB core will eventually free
+	/* Release our reference to this URB, the USB core will eventually free
 	 * it entirely.
 	 */
 	usb_free_urb(urb);
@@ -954,8 +942,7 @@ static void init_params_sja1000(struct ems_cpc_msg *msg)
 	sja1000->mode = SJA1000_MOD_RM;
 }
 
-/*
- * probe function for new CPC-USB devices
+/* probe function for new CPC-USB devices
  */
 static int ems_usb_probe(struct usb_interface *intf,
 			 const struct usb_device_id *id)
@@ -1042,8 +1029,7 @@ static int ems_usb_probe(struct usb_interface *intf,
 	return err;
 }
 
-/*
- * called by the usb core when the device is removed from the system
+/* Called by the usb core when the device is removed from the system
  */
 static void ems_usb_disconnect(struct usb_interface *intf)
 {
-- 
2.26.2

-- 
EMS Dr. Thomas Wuensche e.K.
Sonnenhang 3
85304 Ilmmuenster
HR Ingolstadt, HRA 170106

Phone: +49-8441-490260
Fax  : +49-8441-81860
http://www.ems-wuensche.com

Re: [PATCH 01/17] can: ems_usb: Fixed warnings given by checkpatch.pl and fixed some outdated comments

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 439 bytes --]

On 11/6/20 6:01 PM, Gerhard Uttenthaler wrote:

please provide a patch description.

> Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 02/17] can: ems_usb: Added defines and product id needed for CPC-USB/FD

[Gerhard Uttenthaler]

Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
---
 drivers/net/can/usb/ems_usb.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
index c6ea82b01418..50736e031eb2 100644
--- a/drivers/net/can/usb/ems_usb.c
+++ b/drivers/net/can/usb/ems_usb.c
@@ -40,6 +40,7 @@ MODULE_LICENSE("GPL v2");
 #define CPC_MSG_TYPE_OVERRUN         21 /* overrun events */
 #define CPC_MSG_TYPE_CAN_FRAME_ERROR 23 /* detected bus errors */
 #define CPC_MSG_TYPE_ERR_COUNTER     25 /* RX/TX error counter */
+#define CPC_MSG_TYPE_CANFD_FRAME     26 /* CAN FD frame */
 
 /* Messages from the PC to the CPC interface  */
 #define CPC_CMD_TYPE_CAN_FRAME     1   /* CAN data frame */
@@ -49,6 +50,7 @@ MODULE_LICENSE("GPL v2");
 #define CPC_CMD_TYPE_CAN_STATE     14  /* CAN state message */
 #define CPC_CMD_TYPE_EXT_CAN_FRAME 15  /* Extended CAN data frame */
 #define CPC_CMD_TYPE_EXT_RTR_FRAME 16  /* Extended CAN remote frame */
+#define CPC_CMD_TYPE_CANFD_FRAME   26  /* CAN FD frame */
 #define CPC_CMD_TYPE_CAN_EXIT      200 /* exit the CAN */
 
 #define CPC_CMD_TYPE_INQ_ERR_COUNTER 25 /* request the CAN error counters */
@@ -56,6 +58,7 @@ MODULE_LICENSE("GPL v2");
 #define CPC_CMD_TYPE_CLEAR_CMD_QUEUE 28 /* clear CPC_CMD queue */
 
 #define CPC_CC_TYPE_SJA1000 2 /* NXP CAN controller */
+#define CPC_CC_TYPE_GENERIC 6 /* GENERIC CAN controller */
 
 #define CPC_CAN_ECODE_ERRFRAME 0x01 /* Ecode type */
 
@@ -77,6 +80,7 @@ MODULE_LICENSE("GPL v2");
 #define USB_CPCUSB_VENDOR_ID 0x12D6
 
 #define USB_CPCUSB_ARM7_PRODUCT_ID 0x0444
+#define USB_CPCUSB_FD_PRODUCT_ID   0x0544
 
 /* Mode register NXP LPC2119/SJA1000 CAN Controller */
 #define SJA1000_MOD_NORMAL 0x00
-- 
2.26.2

-- 
EMS Dr. Thomas Wuensche e.K.
Sonnenhang 3
85304 Ilmmuenster
HR Ingolstadt, HRA 170106

Phone: +49-8441-490260
Fax  : +49-8441-81860
http://www.ems-wuensche.com

[PATCH 03/17] can: ems_usb: Added CAN FD message representation

[Gerhard Uttenthaler]

Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
---
 drivers/net/can/usb/ems_usb.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
index 50736e031eb2..fa96217c7d72 100644
--- a/drivers/net/can/usb/ems_usb.c
+++ b/drivers/net/can/usb/ems_usb.c
@@ -114,12 +114,33 @@ MODULE_LICENSE("GPL v2");
  * CPC_MSG_TYPE_CAN_FRAME or CPC_MSG_TYPE_RTR_FRAME or
  * CPC_MSG_TYPE_EXT_CAN_FRAME or CPC_MSG_TYPE_EXT_RTR_FRAME.
  */
-struct cpc_can_msg {
+struct __packed cpc_can_msg {
 	__le32 id;
 	u8 length;
 	u8 msg[8];
 };
 
+/* CAN FD message representation in a CPC_MSG.
+ * Message object type is CPC_MSG_T_CANFD.
+ */
+struct __packed cpc_canfd_msg {
+	__le32 id;
+	u8  length;
+	u8  flags;
+	u8  msg[64];
+};
+
+/* This defines are used with the flags variable
+ * within the struct cpc_canfd_msg. A cpc_canfd_msg
+ * can also be used to send a classic CAN frame including
+ * RTR frames when CPC_FDFLAG_NONCANFD_MSG is set.
+ */
+#define CPC_FDFLAG_ESI          0x08
+#define CPC_FDFLAG_RTR          0x10
+#define CPC_FDFLAG_NONCANFD_MSG 0x20
+#define CPC_FDFLAG_BRS          0x40
+#define CPC_FDFLAG_XTD          0x80
+
 /* Representation of the CAN parameters for the SJA1000 controller */
 struct cpc_sja1000_params {
 	u8 mode;
@@ -194,8 +215,9 @@ struct __packed ems_cpc_msg {
 	__le32 ts_nsec;	/* timestamp in nano seconds */
 
 	union {
-		u8 generic[64];
+		u8 generic[70];
 		struct cpc_can_msg can_msg;
+		struct cpc_canfd_msg canfd_msg;
 		struct cpc_can_params can_params;
 		struct cpc_confirm confirmation;
 		struct cpc_overrun overrun;
-- 
2.26.2

-- 
EMS Dr. Thomas Wuensche e.K.
Sonnenhang 3
85304 Ilmmuenster
HR Ingolstadt, HRA 170106

Phone: +49-8441-490260
Fax  : +49-8441-81860
http://www.ems-wuensche.com

Re: [PATCH 03/17] can: ems_usb: Added CAN FD message representation

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 2233 bytes --]

On 11/6/20 6:01 PM, Gerhard Uttenthaler wrote:
> Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
> ---
>  drivers/net/can/usb/ems_usb.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
> index 50736e031eb2..fa96217c7d72 100644
> --- a/drivers/net/can/usb/ems_usb.c
> +++ b/drivers/net/can/usb/ems_usb.c
> @@ -114,12 +114,33 @@ MODULE_LICENSE("GPL v2");
>   * CPC_MSG_TYPE_CAN_FRAME or CPC_MSG_TYPE_RTR_FRAME or
>   * CPC_MSG_TYPE_EXT_CAN_FRAME or CPC_MSG_TYPE_EXT_RTR_FRAME.
>   */
> -struct cpc_can_msg {
> +struct __packed cpc_can_msg {

unrelated, please make that a separate patch.

>  	__le32 id;
>  	u8 length;
>  	u8 msg[8];
>  };
>  
> +/* CAN FD message representation in a CPC_MSG.
> + * Message object type is CPC_MSG_T_CANFD.
> + */
> +struct __packed cpc_canfd_msg {
> +	__le32 id;
> +	u8  length;
> +	u8  flags;
> +	u8  msg[64];
> +};
> +
> +/* This defines are used with the flags variable
> + * within the struct cpc_canfd_msg. A cpc_canfd_msg
> + * can also be used to send a classic CAN frame including
> + * RTR frames when CPC_FDFLAG_NONCANFD_MSG is set.
> + */
> +#define CPC_FDFLAG_ESI          0x08
> +#define CPC_FDFLAG_RTR          0x10
> +#define CPC_FDFLAG_NONCANFD_MSG 0x20
> +#define CPC_FDFLAG_BRS          0x40
> +#define CPC_FDFLAG_XTD          0x80

BIT()

> +
>  /* Representation of the CAN parameters for the SJA1000 controller */
>  struct cpc_sja1000_params {
>  	u8 mode;
> @@ -194,8 +215,9 @@ struct __packed ems_cpc_msg {
>  	__le32 ts_nsec;	/* timestamp in nano seconds */
>  
>  	union {
> -		u8 generic[64];
> +		u8 generic[70];
>  		struct cpc_can_msg can_msg;
> +		struct cpc_canfd_msg canfd_msg;
>  		struct cpc_can_params can_params;
>  		struct cpc_confirm confirmation;
>  		struct cpc_overrun overrun;
> 

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 04/17] can: ems_usb: Added struct used for CAN FD initialization

[Gerhard Uttenthaler]

Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
---
 drivers/net/can/usb/ems_usb.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
index fa96217c7d72..4ed0d681a68c 100644
--- a/drivers/net/can/usb/ems_usb.c
+++ b/drivers/net/can/usb/ems_usb.c
@@ -142,7 +142,7 @@ struct __packed cpc_canfd_msg {
 #define CPC_FDFLAG_XTD          0x80
 
 /* Representation of the CAN parameters for the SJA1000 controller */
-struct cpc_sja1000_params {
+struct __packed cpc_sja1000_params {
 	u8 mode;
 	u8 acc_code0;
 	u8 acc_code1;
@@ -157,12 +157,41 @@ struct cpc_sja1000_params {
 	u8 outp_contr;
 };
 
+#define CPC_GENERICCONF_FD          0x00000001
+#define CPC_GENERICCONF_FD_BOSCH    0x00000002
+#define CPC_GENERICCONF_LISTEN_ONLY 0x00000004
+#define CPC_GENERICCONF_SINGLE_SHOT 0x00000008
+#define CPC_GENERICCONF_RESET_MODE  0x00000010
+
+#define CPC_USB_RESET_MODE 0x00
+#define CPC_USB_RUN_MODE   0x01
+
+struct __packed cpc_generic_can_params {
+	/* config sets CAN initialization parameters like LOM */
+	__le32 config;
+	__le32 can_clk;
+	struct {
+		__le16 tseg1;  // Time segment 1 (before sample point)
+		__le16 tseg2;  // Time segment 2 (after sample point)
+		__le16 brp;    // Baud rate rate prescaler
+		__le16 sjw;    // (Re)synchronization jump width
+	} n;  // nominal baud rate
+	struct {
+		__le16 tseg1;  // Time segment 1 (before sample point)
+		__le16 tseg2;  // Time segment 2 (after sample point)
+		__le16 brp;    // Baud rate prescaler
+		__le16 sjw;    // (Re)synchronization jump width
+	} d;  // data baud rate
+	__le32 reserved[5];    // Set to 0. Reserved for future use
+};
+
 /* CAN params message representation */
-struct cpc_can_params {
+struct __packed cpc_can_params {
 	u8 cc_type;
 
 	union {
 		struct cpc_sja1000_params sja1000;
+		struct cpc_generic_can_params generic;
 	} cc_params;
 };
 
-- 
2.26.2

-- 
EMS Dr. Thomas Wuensche e.K.
Sonnenhang 3
85304 Ilmmuenster
HR Ingolstadt, HRA 170106

Phone: +49-8441-490260
Fax  : +49-8441-81860
http://www.ems-wuensche.com

Re: [PATCH 04/17] can: ems_usb: Added struct used for CAN FD initialization

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 2610 bytes --]

On 11/6/20 6:01 PM, Gerhard Uttenthaler wrote:
> Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
> ---
>  drivers/net/can/usb/ems_usb.c | 33 +++++++++++++++++++++++++++++++--
>  1 file changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
> index fa96217c7d72..4ed0d681a68c 100644
> --- a/drivers/net/can/usb/ems_usb.c
> +++ b/drivers/net/can/usb/ems_usb.c
> @@ -142,7 +142,7 @@ struct __packed cpc_canfd_msg {
>  #define CPC_FDFLAG_XTD          0x80
>  
>  /* Representation of the CAN parameters for the SJA1000 controller */
> -struct cpc_sja1000_params {
> +struct __packed cpc_sja1000_params {

please make that a separate patch

>  	u8 mode;
>  	u8 acc_code0;
>  	u8 acc_code1;
> @@ -157,12 +157,41 @@ struct cpc_sja1000_params {
>  	u8 outp_contr;
>  };
>  
> +#define CPC_GENERICCONF_FD          0x00000001
> +#define CPC_GENERICCONF_FD_BOSCH    0x00000002
> +#define CPC_GENERICCONF_LISTEN_ONLY 0x00000004
> +#define CPC_GENERICCONF_SINGLE_SHOT 0x00000008
> +#define CPC_GENERICCONF_RESET_MODE  0x00000010

use BIT()

> +
> +#define CPC_USB_RESET_MODE 0x00
> +#define CPC_USB_RUN_MODE   0x01
> +
> +struct __packed cpc_generic_can_params {
> +	/* config sets CAN initialization parameters like LOM */
> +	__le32 config;
> +	__le32 can_clk;
> +	struct {
> +		__le16 tseg1;  // Time segment 1 (before sample point)
> +		__le16 tseg2;  // Time segment 2 (after sample point)
> +		__le16 brp;    // Baud rate rate prescaler
> +		__le16 sjw;    // (Re)synchronization jump width
> +	} n;  // nominal baud rate
no C++ comments

why not call the variable nominal_bitrate?

> +	struct {
> +		__le16 tseg1;  // Time segment 1 (before sample point)
> +		__le16 tseg2;  // Time segment 2 (after sample point)
> +		__le16 brp;    // Baud rate prescaler
> +		__le16 sjw;    // (Re)synchronization jump width
> +	} d;  // data baud rate

data_bitrate

> +	__le32 reserved[5];    // Set to 0. Reserved for future use
> +};
> +
>  /* CAN params message representation */
> -struct cpc_can_params {
> +struct __packed cpc_can_params {
>  	u8 cc_type;
>  
>  	union {
>  		struct cpc_sja1000_params sja1000;
> +		struct cpc_generic_can_params generic;
>  	} cc_params;
>  };
>  
> 


-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 05/17] can: ems_usb: Replace constant define RX_BUFFER_SIZE by variable bulk_rd_buf_size, because this will have different values for CPC-USB/ARM7 and CPC-USB/FD. For the same reason added a function pointer ems_usb_write_mode. In device probe function added a switch statement to select between CPC-USB/ARM7 and CPC-USB/FD and rearranged initialization sequence accordingly.

[Gerhard Uttenthaler]

Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
---
 drivers/net/can/usb/ems_usb.c | 66 +++++++++++++++++++++++++----------
 1 file changed, 47 insertions(+), 19 deletions(-)

diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
index 4ed0d681a68c..a3943042b8c8 100644
--- a/drivers/net/can/usb/ems_usb.c
+++ b/drivers/net/can/usb/ems_usb.c
@@ -266,7 +266,6 @@ static struct usb_device_id ems_usb_table[] = {
 
 MODULE_DEVICE_TABLE(usb, ems_usb_table);
 
-#define RX_BUFFER_SIZE      64
 #define CPC_HEADER_SIZE     4
 #define INTR_IN_BUFFER_SIZE 4
 
@@ -290,6 +289,8 @@ struct ems_usb {
 	struct usb_device *udev;
 	struct net_device *netdev;
 
+	u32 bulk_rd_buf_size;
+
 	atomic_t active_tx_urbs;
 	struct usb_anchor tx_submitted;
 	struct ems_tx_urb_context tx_contexts[MAX_TX_URBS];
@@ -301,7 +302,9 @@ struct ems_usb {
 	u8 *tx_msg_buffer;
 
 	u8 *intr_in_buffer;
-	unsigned int free_slots; /* remember number of available slots */
+	u32 free_slots; /* remember number of available slots */
+
+	int (*ems_usb_write_mode)(struct ems_usb *dev, u32 mode);
 
 	struct ems_cpc_msg active_params; /* active controller parameters */
 };
@@ -522,7 +525,7 @@ static void ems_usb_read_bulk_callback(struct urb *urb)
 
 resubmit_urb:
 	usb_fill_bulk_urb(urb, dev->udev, usb_rcvbulkpipe(dev->udev, 2),
-			  urb->transfer_buffer, RX_BUFFER_SIZE,
+			  urb->transfer_buffer, dev->bulk_rd_buf_size,
 			  ems_usb_read_bulk_callback, dev);
 
 	retval = usb_submit_urb(urb, GFP_ATOMIC);
@@ -596,9 +599,18 @@ static int ems_usb_command_msg(struct ems_usb *dev, struct ems_cpc_msg *msg)
 
 /* Change CAN controllers' mode register
  */
-static int ems_usb_write_mode(struct ems_usb *dev, u8 mode)
+static int ems_usb_write_mode_arm7(struct ems_usb *dev, u32 mode)
 {
-	dev->active_params.msg.can_params.cc_params.sja1000.mode = mode;
+	struct cpc_sja1000_params *sja1000 =
+		&dev->active_params.msg.can_params.cc_params.sja1000;
+
+	if (mode == CPC_USB_RESET_MODE)
+		sja1000->mode |= SJA1000_MOD_RM;
+	else if (mode == CPC_USB_RUN_MODE)
+		sja1000->mode &= ~SJA1000_MOD_RM;
+
+	else
+		return -EINVAL;
 
 	return ems_usb_command_msg(dev, &dev->active_params);
 }
@@ -641,7 +653,7 @@ static int ems_usb_start(struct ems_usb *dev)
 			break;
 		}
 
-		buf = usb_alloc_coherent(dev->udev, RX_BUFFER_SIZE, GFP_KERNEL,
+		buf = usb_alloc_coherent(dev->udev, dev->bulk_rd_buf_size, GFP_KERNEL,
 					 &urb->transfer_dma);
 		if (!buf) {
 			netdev_err(netdev, "No memory left for USB buffer\n");
@@ -651,7 +663,7 @@ static int ems_usb_start(struct ems_usb *dev)
 		}
 
 		usb_fill_bulk_urb(urb, dev->udev, usb_rcvbulkpipe(dev->udev, 2),
-				  buf, RX_BUFFER_SIZE,
+				  buf, dev->bulk_rd_buf_size,
 				  ems_usb_read_bulk_callback, dev);
 		urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
 		usb_anchor_urb(urb, &dev->rx_submitted);
@@ -659,7 +671,7 @@ static int ems_usb_start(struct ems_usb *dev)
 		err = usb_submit_urb(urb, GFP_KERNEL);
 		if (err) {
 			usb_unanchor_urb(urb);
-			usb_free_coherent(dev->udev, RX_BUFFER_SIZE, buf,
+			usb_free_coherent(dev->udev, dev->bulk_rd_buf_size, buf,
 					  urb->transfer_dma);
 			usb_free_urb(urb);
 			break;
@@ -708,7 +720,7 @@ static int ems_usb_start(struct ems_usb *dev)
 	if (err)
 		goto failed;
 
-	err = ems_usb_write_mode(dev, SJA1000_MOD_NORMAL);
+	err = dev->can.do_set_mode(netdev, CAN_MODE_START);
 	if (err)
 		goto failed;
 
@@ -742,7 +754,7 @@ static int ems_usb_open(struct net_device *netdev)
 	struct ems_usb *dev = netdev_priv(netdev);
 	int err;
 
-	err = ems_usb_write_mode(dev, SJA1000_MOD_RM);
+	err = dev->ems_usb_write_mode(dev, CPC_USB_RESET_MODE);
 	if (err)
 		return err;
 
@@ -900,7 +912,7 @@ static int ems_usb_close(struct net_device *netdev)
 	netif_stop_queue(netdev);
 
 	/* Set CAN controller to reset mode */
-	if (ems_usb_write_mode(dev, SJA1000_MOD_RM))
+	if (dev->ems_usb_write_mode(dev, CPC_USB_RESET_MODE))
 		netdev_warn(netdev, "couldn't stop device");
 
 	close_candev(netdev);
@@ -915,8 +927,8 @@ static const struct net_device_ops ems_usb_netdev_ops = {
 	.ndo_change_mtu = can_change_mtu,
 };
 
-static const struct can_bittiming_const ems_usb_bittiming_const = {
-	.name = "ems_usb",
+static const struct can_bittiming_const ems_usb_bittiming_const_arm7 = {
+	.name = "ems_usb_arm7",
 	.tseg1_min = 1,
 	.tseg1_max = 16,
 	.tseg2_min = 1,
@@ -933,7 +945,7 @@ static int ems_usb_set_mode(struct net_device *netdev, enum can_mode mode)
 
 	switch (mode) {
 	case CAN_MODE_START:
-		if (ems_usb_write_mode(dev, SJA1000_MOD_NORMAL))
+		if (dev->ems_usb_write_mode(dev, CPC_USB_RUN_MODE))
 			netdev_warn(netdev, "couldn't start device");
 
 		if (netif_queue_stopped(netdev))
@@ -947,7 +959,7 @@ static int ems_usb_set_mode(struct net_device *netdev, enum can_mode mode)
 	return 0;
 }
 
-static int ems_usb_set_bittiming(struct net_device *netdev)
+static int ems_usb_set_bittiming_arm7(struct net_device *netdev)
 {
 	struct ems_usb *dev = netdev_priv(netdev);
 	struct can_bittiming *bt = &dev->can.bittiming;
@@ -1018,11 +1030,29 @@ static int ems_usb_probe(struct usb_interface *intf,
 	dev->netdev = netdev;
 
 	dev->can.state = CAN_STATE_STOPPED;
+
+	/* Select CPC-USB/ARM7 or CPC-USB/FD */
+	switch (dev->udev->descriptor.idProduct) {
+	case USB_CPCUSB_ARM7_PRODUCT_ID:
+
 	dev->can.clock.freq = EMS_USB_ARM7_CLOCK;
-	dev->can.bittiming_const = &ems_usb_bittiming_const;
-	dev->can.do_set_bittiming = ems_usb_set_bittiming;
+	dev->can.bittiming_const = &ems_usb_bittiming_const_arm7;
+	dev->can.do_set_bittiming = ems_usb_set_bittiming_arm7;
 	dev->can.do_set_mode = ems_usb_set_mode;
 	dev->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES;
+	init_params_sja1000(&dev->active_params);
+	dev->ems_usb_write_mode = ems_usb_write_mode_arm7;
+	dev->bulk_rd_buf_size = 64;
+	break;
+
+	case USB_CPCUSB_FD_PRODUCT_ID:
+	// Placeholder for next patchess
+	break;
+
+	default:
+		err = -ENODEV;
+		goto cleanup_candev;
+	}
 
 	netdev->netdev_ops = &ems_usb_netdev_ops;
 
@@ -1053,8 +1083,6 @@ static int ems_usb_probe(struct usb_interface *intf,
 
 	SET_NETDEV_DEV(netdev, &intf->dev);
 
-	init_params_sja1000(&dev->active_params);
-
 	err = ems_usb_command_msg(dev, &dev->active_params);
 	if (err) {
 		netdev_err(netdev, "couldn't initialize controller: %d\n", err);
-- 
2.26.2

-- 
EMS Dr. Thomas Wuensche e.K.
Sonnenhang 3
85304 Ilmmuenster
HR Ingolstadt, HRA 170106

Phone: +49-8441-490260
Fax  : +49-8441-81860
http://www.ems-wuensche.com

Re: [PATCH 05/17] can: ems_usb: Replace constant define RX_BUFFER_SIZE by variable bulk_rd_buf_size, because this will have different values for CPC-USB/ARM7 and CPC-USB/FD. For the same reason added a function pointer ems_usb_write_mode. In device probe function added a switch statement to select between CPC-USB/ARM7 and CPC-USB/FD and rearranged initialization sequence accordingly.

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 7672 bytes --]

Please keep the patch subject within reasonable length.
It sould describe what you have done. The patch description should describe the why.

On 11/6/20 6:01 PM, Gerhard Uttenthaler wrote:
> Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
> ---
>  drivers/net/can/usb/ems_usb.c | 66 +++++++++++++++++++++++++----------
>  1 file changed, 47 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
> index 4ed0d681a68c..a3943042b8c8 100644
> --- a/drivers/net/can/usb/ems_usb.c
> +++ b/drivers/net/can/usb/ems_usb.c
> @@ -266,7 +266,6 @@ static struct usb_device_id ems_usb_table[] = {
>  
>  MODULE_DEVICE_TABLE(usb, ems_usb_table);
>  
> -#define RX_BUFFER_SIZE      64

Can you keep this define instead of using a "64" below? Give it a proper
prefix/postfix if needed.

>  #define CPC_HEADER_SIZE     4
>  #define INTR_IN_BUFFER_SIZE 4
>  
> @@ -290,6 +289,8 @@ struct ems_usb {
>  	struct usb_device *udev;
>  	struct net_device *netdev;
>  
> +	u32 bulk_rd_buf_size;
> +
>  	atomic_t active_tx_urbs;
>  	struct usb_anchor tx_submitted;
>  	struct ems_tx_urb_context tx_contexts[MAX_TX_URBS];
> @@ -301,7 +302,9 @@ struct ems_usb {
>  	u8 *tx_msg_buffer;
>  
>  	u8 *intr_in_buffer;
> -	unsigned int free_slots; /* remember number of available slots */
> +	u32 free_slots; /* remember number of available slots */

nitpick
Why this change?

> +
> +	int (*ems_usb_write_mode)(struct ems_usb *dev, u32 mode);
>  
>  	struct ems_cpc_msg active_params; /* active controller parameters */
>  };
> @@ -522,7 +525,7 @@ static void ems_usb_read_bulk_callback(struct urb *urb)
>  
>  resubmit_urb:
>  	usb_fill_bulk_urb(urb, dev->udev, usb_rcvbulkpipe(dev->udev, 2),
> -			  urb->transfer_buffer, RX_BUFFER_SIZE,
> +			  urb->transfer_buffer, dev->bulk_rd_buf_size,
>  			  ems_usb_read_bulk_callback, dev);
>  
>  	retval = usb_submit_urb(urb, GFP_ATOMIC);
> @@ -596,9 +599,18 @@ static int ems_usb_command_msg(struct ems_usb *dev, struct ems_cpc_msg *msg)
>  
>  /* Change CAN controllers' mode register
>   */
> -static int ems_usb_write_mode(struct ems_usb *dev, u8 mode)
> +static int ems_usb_write_mode_arm7(struct ems_usb *dev, u32 mode)
>  {
> -	dev->active_params.msg.can_params.cc_params.sja1000.mode = mode;
> +	struct cpc_sja1000_params *sja1000 =
> +		&dev->active_params.msg.can_params.cc_params.sja1000;
> +
> +	if (mode == CPC_USB_RESET_MODE)
> +		sja1000->mode |= SJA1000_MOD_RM;
> +	else if (mode == CPC_USB_RUN_MODE)
> +		sja1000->mode &= ~SJA1000_MOD_RM;
> +
> +	else
> +		return -EINVAL;
>  
>  	return ems_usb_command_msg(dev, &dev->active_params);
>  }
> @@ -641,7 +653,7 @@ static int ems_usb_start(struct ems_usb *dev)
>  			break;
>  		}
>  
> -		buf = usb_alloc_coherent(dev->udev, RX_BUFFER_SIZE, GFP_KERNEL,
> +		buf = usb_alloc_coherent(dev->udev, dev->bulk_rd_buf_size, GFP_KERNEL,
>  					 &urb->transfer_dma);
>  		if (!buf) {
>  			netdev_err(netdev, "No memory left for USB buffer\n");
> @@ -651,7 +663,7 @@ static int ems_usb_start(struct ems_usb *dev)
>  		}
>  
>  		usb_fill_bulk_urb(urb, dev->udev, usb_rcvbulkpipe(dev->udev, 2),
> -				  buf, RX_BUFFER_SIZE,
> +				  buf, dev->bulk_rd_buf_size,
>  				  ems_usb_read_bulk_callback, dev);
>  		urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
>  		usb_anchor_urb(urb, &dev->rx_submitted);
> @@ -659,7 +671,7 @@ static int ems_usb_start(struct ems_usb *dev)
>  		err = usb_submit_urb(urb, GFP_KERNEL);
>  		if (err) {
>  			usb_unanchor_urb(urb);
> -			usb_free_coherent(dev->udev, RX_BUFFER_SIZE, buf,
> +			usb_free_coherent(dev->udev, dev->bulk_rd_buf_size, buf,
>  					  urb->transfer_dma);
>  			usb_free_urb(urb);
>  			break;
> @@ -708,7 +720,7 @@ static int ems_usb_start(struct ems_usb *dev)
>  	if (err)
>  		goto failed;
>  
> -	err = ems_usb_write_mode(dev, SJA1000_MOD_NORMAL);
> +	err = dev->can.do_set_mode(netdev, CAN_MODE_START);
>  	if (err)
>  		goto failed;
>  
> @@ -742,7 +754,7 @@ static int ems_usb_open(struct net_device *netdev)
>  	struct ems_usb *dev = netdev_priv(netdev);
>  	int err;
>  
> -	err = ems_usb_write_mode(dev, SJA1000_MOD_RM);
> +	err = dev->ems_usb_write_mode(dev, CPC_USB_RESET_MODE);
>  	if (err)
>  		return err;
>  
> @@ -900,7 +912,7 @@ static int ems_usb_close(struct net_device *netdev)
>  	netif_stop_queue(netdev);
>  
>  	/* Set CAN controller to reset mode */
> -	if (ems_usb_write_mode(dev, SJA1000_MOD_RM))
> +	if (dev->ems_usb_write_mode(dev, CPC_USB_RESET_MODE))
>  		netdev_warn(netdev, "couldn't stop device");
>  
>  	close_candev(netdev);
> @@ -915,8 +927,8 @@ static const struct net_device_ops ems_usb_netdev_ops = {
>  	.ndo_change_mtu = can_change_mtu,
>  };
>  
> -static const struct can_bittiming_const ems_usb_bittiming_const = {
> -	.name = "ems_usb",
> +static const struct can_bittiming_const ems_usb_bittiming_const_arm7 = {
> +	.name = "ems_usb_arm7",

You are changing the user space visible name of the CAN device. Is this needed?

>  	.tseg1_min = 1,
>  	.tseg1_max = 16,
>  	.tseg2_min = 1,
> @@ -933,7 +945,7 @@ static int ems_usb_set_mode(struct net_device *netdev, enum can_mode mode)
>  
>  	switch (mode) {
>  	case CAN_MODE_START:
> -		if (ems_usb_write_mode(dev, SJA1000_MOD_NORMAL))
> +		if (dev->ems_usb_write_mode(dev, CPC_USB_RUN_MODE))
>  			netdev_warn(netdev, "couldn't start device");
>  
>  		if (netif_queue_stopped(netdev))
> @@ -947,7 +959,7 @@ static int ems_usb_set_mode(struct net_device *netdev, enum can_mode mode)
>  	return 0;
>  }
>  
> -static int ems_usb_set_bittiming(struct net_device *netdev)
> +static int ems_usb_set_bittiming_arm7(struct net_device *netdev)
>  {
>  	struct ems_usb *dev = netdev_priv(netdev);
>  	struct can_bittiming *bt = &dev->can.bittiming;
> @@ -1018,11 +1030,29 @@ static int ems_usb_probe(struct usb_interface *intf,
>  	dev->netdev = netdev;
>  
>  	dev->can.state = CAN_STATE_STOPPED;
> +
> +	/* Select CPC-USB/ARM7 or CPC-USB/FD */
> +	switch (dev->udev->descriptor.idProduct) {
> +	case USB_CPCUSB_ARM7_PRODUCT_ID:

please indent one level after the case

> +
>  	dev->can.clock.freq = EMS_USB_ARM7_CLOCK;
> -	dev->can.bittiming_const = &ems_usb_bittiming_const;
> -	dev->can.do_set_bittiming = ems_usb_set_bittiming;
> +	dev->can.bittiming_const = &ems_usb_bittiming_const_arm7;
> +	dev->can.do_set_bittiming = ems_usb_set_bittiming_arm7;
>  	dev->can.do_set_mode = ems_usb_set_mode;
>  	dev->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES;
> +	init_params_sja1000(&dev->active_params);
> +	dev->ems_usb_write_mode = ems_usb_write_mode_arm7;
> +	dev->bulk_rd_buf_size = 64;
> +	break;
> +
> +	case USB_CPCUSB_FD_PRODUCT_ID:
> +	// Placeholder for next patchess

Add this case in the patch where you fill the placeholder.

> +	break;
> +
> +	default:
> +		err = -ENODEV;
> +		goto cleanup_candev;
> +	}
>  
>  	netdev->netdev_ops = &ems_usb_netdev_ops;
>  
> @@ -1053,8 +1083,6 @@ static int ems_usb_probe(struct usb_interface *intf,
>  
>  	SET_NETDEV_DEV(netdev, &intf->dev);
>  
> -	init_params_sja1000(&dev->active_params);
> -
>  	err = ems_usb_command_msg(dev, &dev->active_params);
>  	if (err) {
>  		netdev_err(netdev, "couldn't initialize controller: %d\n", err);
> 

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 05/17] can: ems_usb: Replace constant define RX_BUFFER_SIZE by variable bulk_rd_buf_size, because this will have different values for CPC-USB/ARM7 and CPC-USB/FD. For the same reason added a function pointer ems_usb_write_mode. In device probe function added a switch statement to select between CPC-USB/ARM7 and CPC-USB/FD and rearranged initialization sequence accordingly.

[Gerhard Uttenthaler]

Am 06.11.20 um 18:15 schrieb Marc Kleine-Budde:
> Please keep the patch subject within reasonable length.
> It sould describe what you have done. The patch description should describe the why.
OK

> 
> On 11/6/20 6:01 PM, Gerhard Uttenthaler wrote:
>> Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
>> ---
>>  drivers/net/can/usb/ems_usb.c | 66 +++++++++++++++++++++++++----------
>>  1 file changed, 47 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
>> index 4ed0d681a68c..a3943042b8c8 100644
>> --- a/drivers/net/can/usb/ems_usb.c
>> +++ b/drivers/net/can/usb/ems_usb.c
>> @@ -266,7 +266,6 @@ static struct usb_device_id ems_usb_table[] = {
>>  
>>  MODULE_DEVICE_TABLE(usb, ems_usb_table);
>>  
>> -#define RX_BUFFER_SIZE      64
> 
> Can you keep this define instead of using a "64" below? Give it a proper
> prefix/postfix if needed.
OK

> 
>>  #define CPC_HEADER_SIZE     4
>>  #define INTR_IN_BUFFER_SIZE 4
>>  
>> @@ -290,6 +289,8 @@ struct ems_usb {
>>  	struct usb_device *udev;
>>  	struct net_device *netdev;
>>  
>> +	u32 bulk_rd_buf_size;
>> +
>>  	atomic_t active_tx_urbs;
>>  	struct usb_anchor tx_submitted;
>>  	struct ems_tx_urb_context tx_contexts[MAX_TX_URBS];
>> @@ -301,7 +302,9 @@ struct ems_usb {
>>  	u8 *tx_msg_buffer;
>>  
>>  	u8 *intr_in_buffer;
>> -	unsigned int free_slots; /* remember number of available slots */
>> +	u32 free_slots; /* remember number of available slots */
> 
> nitpick
> Why this change?
It was the last "unsigned int". They all are u32 now.

> 
>> +
>> +	int (*ems_usb_write_mode)(struct ems_usb *dev, u32 mode);
>>  
>>  	struct ems_cpc_msg active_params; /* active controller parameters */
>>  };
>> @@ -522,7 +525,7 @@ static void ems_usb_read_bulk_callback(struct urb *urb)
>>  
>>  resubmit_urb:
>>  	usb_fill_bulk_urb(urb, dev->udev, usb_rcvbulkpipe(dev->udev, 2),
>> -			  urb->transfer_buffer, RX_BUFFER_SIZE,
>> +			  urb->transfer_buffer, dev->bulk_rd_buf_size,
>>  			  ems_usb_read_bulk_callback, dev);
>>  
>>  	retval = usb_submit_urb(urb, GFP_ATOMIC);
>> @@ -596,9 +599,18 @@ static int ems_usb_command_msg(struct ems_usb *dev, struct ems_cpc_msg *msg)
>>  
>>  /* Change CAN controllers' mode register
>>   */
>> -static int ems_usb_write_mode(struct ems_usb *dev, u8 mode)
>> +static int ems_usb_write_mode_arm7(struct ems_usb *dev, u32 mode)
>>  {
>> -	dev->active_params.msg.can_params.cc_params.sja1000.mode = mode;
>> +	struct cpc_sja1000_params *sja1000 =
>> +		&dev->active_params.msg.can_params.cc_params.sja1000;
>> +
>> +	if (mode == CPC_USB_RESET_MODE)
>> +		sja1000->mode |= SJA1000_MOD_RM;
>> +	else if (mode == CPC_USB_RUN_MODE)
>> +		sja1000->mode &= ~SJA1000_MOD_RM;
>> +
>> +	else
>> +		return -EINVAL;
>>  
>>  	return ems_usb_command_msg(dev, &dev->active_params);
>>  }
>> @@ -641,7 +653,7 @@ static int ems_usb_start(struct ems_usb *dev)
>>  			break;
>>  		}
>>  
>> -		buf = usb_alloc_coherent(dev->udev, RX_BUFFER_SIZE, GFP_KERNEL,
>> +		buf = usb_alloc_coherent(dev->udev, dev->bulk_rd_buf_size, GFP_KERNEL,
>>  					 &urb->transfer_dma);
>>  		if (!buf) {
>>  			netdev_err(netdev, "No memory left for USB buffer\n");
>> @@ -651,7 +663,7 @@ static int ems_usb_start(struct ems_usb *dev)
>>  		}
>>  
>>  		usb_fill_bulk_urb(urb, dev->udev, usb_rcvbulkpipe(dev->udev, 2),
>> -				  buf, RX_BUFFER_SIZE,
>> +				  buf, dev->bulk_rd_buf_size,
>>  				  ems_usb_read_bulk_callback, dev);
>>  		urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
>>  		usb_anchor_urb(urb, &dev->rx_submitted);
>> @@ -659,7 +671,7 @@ static int ems_usb_start(struct ems_usb *dev)
>>  		err = usb_submit_urb(urb, GFP_KERNEL);
>>  		if (err) {
>>  			usb_unanchor_urb(urb);
>> -			usb_free_coherent(dev->udev, RX_BUFFER_SIZE, buf,
>> +			usb_free_coherent(dev->udev, dev->bulk_rd_buf_size, buf,
>>  					  urb->transfer_dma);
>>  			usb_free_urb(urb);
>>  			break;
>> @@ -708,7 +720,7 @@ static int ems_usb_start(struct ems_usb *dev)
>>  	if (err)
>>  		goto failed;
>>  
>> -	err = ems_usb_write_mode(dev, SJA1000_MOD_NORMAL);
>> +	err = dev->can.do_set_mode(netdev, CAN_MODE_START);
>>  	if (err)
>>  		goto failed;
>>  
>> @@ -742,7 +754,7 @@ static int ems_usb_open(struct net_device *netdev)
>>  	struct ems_usb *dev = netdev_priv(netdev);
>>  	int err;
>>  
>> -	err = ems_usb_write_mode(dev, SJA1000_MOD_RM);
>> +	err = dev->ems_usb_write_mode(dev, CPC_USB_RESET_MODE);
>>  	if (err)
>>  		return err;
>>  
>> @@ -900,7 +912,7 @@ static int ems_usb_close(struct net_device *netdev)
>>  	netif_stop_queue(netdev);
>>  
>>  	/* Set CAN controller to reset mode */
>> -	if (ems_usb_write_mode(dev, SJA1000_MOD_RM))
>> +	if (dev->ems_usb_write_mode(dev, CPC_USB_RESET_MODE))
>>  		netdev_warn(netdev, "couldn't stop device");
>>  
>>  	close_candev(netdev);
>> @@ -915,8 +927,8 @@ static const struct net_device_ops ems_usb_netdev_ops = {
>>  	.ndo_change_mtu = can_change_mtu,
>>  };
>>  
>> -static const struct can_bittiming_const ems_usb_bittiming_const = {
>> -	.name = "ems_usb",
>> +static const struct can_bittiming_const ems_usb_bittiming_const_arm7 = {
>> +	.name = "ems_usb_arm7",
> 
> You are changing the user space visible name of the CAN device. Is this needed?
The driver will be able to handle both devices CPC-USB/ARM7 and
CPC-USB/FD. If a user calls:

ip -details -statistics link show can0

then I noticed that "ems_usb" is inside the output. This seemed
ambiguous for me as it could be a CPC-USB/ARM7 or a CPC-USB/FD. Sure
this will need all patches applied to become visible.

-- 
Gerhard

-- 
EMS Dr. Thomas Wuensche e.K.
Sonnenhang 3
85304 Ilmmuenster
HR Ingolstadt, HRA 170106

Phone: +49-8441-490260
Fax  : +49-8441-81860
http://www.ems-wuensche.com

Re: [PATCH 05/17] can: ems_usb: Replace constant define RX_BUFFER_SIZE by variable bulk_rd_buf_size, because this will have different values for CPC-USB/ARM7 and CPC-USB/FD. For the same reason added a function pointer ems_usb_write_mode. In device probe function added a switch statement to select between CPC-USB/ARM7 and CPC-USB/FD and rearranged initialization sequence accordingly.

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 2901 bytes --]

On 11/10/20 11:31 AM, Gerhard Uttenthaler wrote:
> Am 06.11.20 um 18:15 schrieb Marc Kleine-Budde:
>> Please keep the patch subject within reasonable length.
>> It sould describe what you have done. The patch description should describe the why.
> OK
> 
>>
>> On 11/6/20 6:01 PM, Gerhard Uttenthaler wrote:
>>> Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
>>> ---
>>>  drivers/net/can/usb/ems_usb.c | 66 +++++++++++++++++++++++++----------
>>>  1 file changed, 47 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
>>> index 4ed0d681a68c..a3943042b8c8 100644
>>> --- a/drivers/net/can/usb/ems_usb.c
>>> +++ b/drivers/net/can/usb/ems_usb.c
>>> @@ -266,7 +266,6 @@ static struct usb_device_id ems_usb_table[] = {
>>>  
>>>  MODULE_DEVICE_TABLE(usb, ems_usb_table);
>>>  
>>> -#define RX_BUFFER_SIZE      64
>>
>> Can you keep this define instead of using a "64" below? Give it a proper
>> prefix/postfix if needed.
> OK
> 
>>
>>>  #define CPC_HEADER_SIZE     4
>>>  #define INTR_IN_BUFFER_SIZE 4
>>>  
>>> @@ -290,6 +289,8 @@ struct ems_usb {
>>>  	struct usb_device *udev;
>>>  	struct net_device *netdev;
>>>  
>>> +	u32 bulk_rd_buf_size;
>>> +
>>>  	atomic_t active_tx_urbs;
>>>  	struct usb_anchor tx_submitted;
>>>  	struct ems_tx_urb_context tx_contexts[MAX_TX_URBS];
>>> @@ -301,7 +302,9 @@ struct ems_usb {
>>>  	u8 *tx_msg_buffer;
>>>  
>>>  	u8 *intr_in_buffer;
>>> -	unsigned int free_slots; /* remember number of available slots */
>>> +	u32 free_slots; /* remember number of available slots */
>>
>> nitpick
>> Why this change?
> It was the last "unsigned int". They all are u32 now.

seems unrelated to the rest of the changes?

[...]

>>> -static const struct can_bittiming_const ems_usb_bittiming_const = {
>>> -	.name = "ems_usb",
>>> +static const struct can_bittiming_const ems_usb_bittiming_const_arm7 = {
>>> +	.name = "ems_usb_arm7",
>>
>> You are changing the user space visible name of the CAN device. Is this needed?
> The driver will be able to handle both devices CPC-USB/ARM7 and
> CPC-USB/FD. If a user calls:
> 
> ip -details -statistics link show can0
> 
> then I noticed that "ems_usb" is inside the output. This seemed
> ambiguous for me as it could be a CPC-USB/ARM7 or a CPC-USB/FD. Sure
> this will need all patches applied to become visible.

Consider there is a software that relies on the "ems_usb" string. A kernel
update should not change this if not needed. The new "FD" adapter can and should
have a different name there.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 06/17] can: ems_usb: Added listen only mode for CPC-USB/ARM7 and moved evaluation of can.ctrlmode from set_bittiming routine to ems_usb_write_mode_arm7 routine. Wrapped a long line.

[Gerhard Uttenthaler]

Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
---
 drivers/net/can/usb/ems_usb.c | 39 ++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
index a3943042b8c8..66418e5af87d 100644
--- a/drivers/net/can/usb/ems_usb.c
+++ b/drivers/net/can/usb/ems_usb.c
@@ -85,6 +85,7 @@ MODULE_LICENSE("GPL v2");
 /* Mode register NXP LPC2119/SJA1000 CAN Controller */
 #define SJA1000_MOD_NORMAL 0x00
 #define SJA1000_MOD_RM     0x01
+#define SJA1000_MOD_LOM    0x02
 
 /* ECC register NXP LPC2119/SJA1000 CAN Controller */
 #define SJA1000_ECC_SEG   0x1F
@@ -604,13 +605,23 @@ static int ems_usb_write_mode_arm7(struct ems_usb *dev, u32 mode)
 	struct cpc_sja1000_params *sja1000 =
 		&dev->active_params.msg.can_params.cc_params.sja1000;
 
-	if (mode == CPC_USB_RESET_MODE)
+	if (mode == CPC_USB_RESET_MODE) {
 		sja1000->mode |= SJA1000_MOD_RM;
-	else if (mode == CPC_USB_RUN_MODE)
+	} else if (mode == CPC_USB_RUN_MODE) {
 		sja1000->mode &= ~SJA1000_MOD_RM;
 
-	else
+		if (dev->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
+			sja1000->mode |= SJA1000_MOD_LOM;
+		else
+			sja1000->mode &= ~SJA1000_MOD_LOM;
+
+		if (dev->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
+			sja1000->btr1 |= 0x80;
+		else
+			sja1000->btr1 &= ~0x80;
+	} else {
 		return -EINVAL;
+	}
 
 	return ems_usb_command_msg(dev, &dev->active_params);
 }
@@ -653,7 +664,9 @@ static int ems_usb_start(struct ems_usb *dev)
 			break;
 		}
 
-		buf = usb_alloc_coherent(dev->udev, dev->bulk_rd_buf_size, GFP_KERNEL,
+		buf = usb_alloc_coherent(dev->udev,
+					 dev->bulk_rd_buf_size,
+					 GFP_KERNEL,
 					 &urb->transfer_dma);
 		if (!buf) {
 			netdev_err(netdev, "No memory left for USB buffer\n");
@@ -963,18 +976,16 @@ static int ems_usb_set_bittiming_arm7(struct net_device *netdev)
 {
 	struct ems_usb *dev = netdev_priv(netdev);
 	struct can_bittiming *bt = &dev->can.bittiming;
-	u8 btr0, btr1;
+	struct cpc_sja1000_params *sja1000 =
+		&dev->active_params.msg.can_params.cc_params.sja1000;
 
-	btr0 = ((bt->brp - 1) & 0x3f) | (((bt->sjw - 1) & 0x3) << 6);
-	btr1 = ((bt->prop_seg + bt->phase_seg1 - 1) & 0xf) |
+	sja1000->btr0 = ((bt->brp - 1) & 0x3f) | (((bt->sjw - 1) & 0x3) << 6);
+	sja1000->btr1 = ((bt->prop_seg + bt->phase_seg1 - 1) & 0xf) |
 		(((bt->phase_seg2 - 1) & 0x7) << 4);
-	if (dev->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
-		btr1 |= 0x80;
-
-	netdev_info(netdev, "setting BTR0=0x%02x BTR1=0x%02x\n", btr0, btr1);
 
-	dev->active_params.msg.can_params.cc_params.sja1000.btr0 = btr0;
-	dev->active_params.msg.can_params.cc_params.sja1000.btr1 = btr1;
+	netdev_info(netdev, "Set bit timing for CPC-USB/ARM7\n");
+	netdev_info(netdev, "BTR0=0x%02x, BTR1=0x%02x\n",
+		    sja1000->btr0, sja1000->btr1);
 
 	return ems_usb_command_msg(dev, &dev->active_params);
 }
@@ -1039,7 +1050,7 @@ static int ems_usb_probe(struct usb_interface *intf,
 	dev->can.bittiming_const = &ems_usb_bittiming_const_arm7;
 	dev->can.do_set_bittiming = ems_usb_set_bittiming_arm7;
 	dev->can.do_set_mode = ems_usb_set_mode;
-	dev->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES;
+	dev->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY;
 	init_params_sja1000(&dev->active_params);
 	dev->ems_usb_write_mode = ems_usb_write_mode_arm7;
 	dev->bulk_rd_buf_size = 64;
-- 
2.26.2

-- 
EMS Dr. Thomas Wuensche e.K.
Sonnenhang 3
85304 Ilmmuenster
HR Ingolstadt, HRA 170106

Phone: +49-8441-490260
Fax  : +49-8441-81860
http://www.ems-wuensche.com

Re: [PATCH 06/17] can: ems_usb: Added listen only mode for CPC-USB/ARM7 and moved evaluation of can.ctrlmode from set_bittiming routine to ems_usb_write_mode_arm7 routine. Wrapped a long line.

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 4069 bytes --]

On 11/6/20 6:01 PM, Gerhard Uttenthaler wrote:
> Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
> ---
>  drivers/net/can/usb/ems_usb.c | 39 ++++++++++++++++++++++-------------
>  1 file changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
> index a3943042b8c8..66418e5af87d 100644
> --- a/drivers/net/can/usb/ems_usb.c
> +++ b/drivers/net/can/usb/ems_usb.c
> @@ -85,6 +85,7 @@ MODULE_LICENSE("GPL v2");
>  /* Mode register NXP LPC2119/SJA1000 CAN Controller */
>  #define SJA1000_MOD_NORMAL 0x00
>  #define SJA1000_MOD_RM     0x01
> +#define SJA1000_MOD_LOM    0x02
>  
>  /* ECC register NXP LPC2119/SJA1000 CAN Controller */
>  #define SJA1000_ECC_SEG   0x1F
> @@ -604,13 +605,23 @@ static int ems_usb_write_mode_arm7(struct ems_usb *dev, u32 mode)
>  	struct cpc_sja1000_params *sja1000 =
>  		&dev->active_params.msg.can_params.cc_params.sja1000;
>  
> -	if (mode == CPC_USB_RESET_MODE)
> +	if (mode == CPC_USB_RESET_MODE) {
>  		sja1000->mode |= SJA1000_MOD_RM;
> -	else if (mode == CPC_USB_RUN_MODE)
> +	} else if (mode == CPC_USB_RUN_MODE) {
>  		sja1000->mode &= ~SJA1000_MOD_RM;
>  
> -	else
> +		if (dev->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
> +			sja1000->mode |= SJA1000_MOD_LOM;
> +		else
> +			sja1000->mode &= ~SJA1000_MOD_LOM;
> +
> +		if (dev->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
> +			sja1000->btr1 |= 0x80;
> +		else
> +			sja1000->btr1 &= ~0x80;
> +	} else {
>  		return -EINVAL;
> +	}
>  
>  	return ems_usb_command_msg(dev, &dev->active_params);
>  }
> @@ -653,7 +664,9 @@ static int ems_usb_start(struct ems_usb *dev)
>  			break;
>  		}
>  
> -		buf = usb_alloc_coherent(dev->udev, dev->bulk_rd_buf_size, GFP_KERNEL,
> +		buf = usb_alloc_coherent(dev->udev,
> +					 dev->bulk_rd_buf_size,
> +					 GFP_KERNEL,
>  					 &urb->transfer_dma);

unrelated, please move the the previous patch

>  		if (!buf) {
>  			netdev_err(netdev, "No memory left for USB buffer\n");
> @@ -963,18 +976,16 @@ static int ems_usb_set_bittiming_arm7(struct net_device *netdev)
>  {
>  	struct ems_usb *dev = netdev_priv(netdev);
>  	struct can_bittiming *bt = &dev->can.bittiming;
> -	u8 btr0, btr1;
> +	struct cpc_sja1000_params *sja1000 =
> +		&dev->active_params.msg.can_params.cc_params.sja1000;
>  
> -	btr0 = ((bt->brp - 1) & 0x3f) | (((bt->sjw - 1) & 0x3) << 6);
> -	btr1 = ((bt->prop_seg + bt->phase_seg1 - 1) & 0xf) |
> +	sja1000->btr0 = ((bt->brp - 1) & 0x3f) | (((bt->sjw - 1) & 0x3) << 6);
> +	sja1000->btr1 = ((bt->prop_seg + bt->phase_seg1 - 1) & 0xf) |
>  		(((bt->phase_seg2 - 1) & 0x7) << 4);
> -	if (dev->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
> -		btr1 |= 0x80;
> -
> -	netdev_info(netdev, "setting BTR0=0x%02x BTR1=0x%02x\n", btr0, btr1);
>  
> -	dev->active_params.msg.can_params.cc_params.sja1000.btr0 = btr0;
> -	dev->active_params.msg.can_params.cc_params.sja1000.btr1 = btr1;
> +	netdev_info(netdev, "Set bit timing for CPC-USB/ARM7\n");
> +	netdev_info(netdev, "BTR0=0x%02x, BTR1=0x%02x\n",
> +		    sja1000->btr0, sja1000->btr1);

One line of netdev_info should be enough...

>  
>  	return ems_usb_command_msg(dev, &dev->active_params);
>  }
> @@ -1039,7 +1050,7 @@ static int ems_usb_probe(struct usb_interface *intf,
>  	dev->can.bittiming_const = &ems_usb_bittiming_const_arm7;
>  	dev->can.do_set_bittiming = ems_usb_set_bittiming_arm7;
>  	dev->can.do_set_mode = ems_usb_set_mode;
> -	dev->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES;
> +	dev->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY;
>  	init_params_sja1000(&dev->active_params);
>  	dev->ems_usb_write_mode = ems_usb_write_mode_arm7;
>  	dev->bulk_rd_buf_size = 64;
> 

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 07/17] can: ems_usb: Added CPC_ClearCmdQueue routine to get access to the interface even it is flooded with messages which cannot successfully be sent. Set timestamp to 0 in ems_usb_control_cmd.

[Gerhard Uttenthaler]

Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
---
 drivers/net/can/usb/ems_usb.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
index 66418e5af87d..c664af4499a1 100644
--- a/drivers/net/can/usb/ems_usb.c
+++ b/drivers/net/can/usb/ems_usb.c
@@ -637,12 +637,29 @@ static int ems_usb_control_cmd(struct ems_usb *dev, u8 val)
 	cmd.length = CPC_MSG_HEADER_LEN + 1;
 
 	cmd.msgid = 0;
+	cmd.ts_sec = 0;
+	cmd.ts_nsec = 0;
 
 	cmd.msg.generic[0] = val;
 
 	return ems_usb_command_msg(dev, &cmd);
 }
 
+/* Send a CPC_ClearCmdQueue command
+ */
+static int ems_usb_clear_cmd_queue(struct ems_usb *dev)
+{
+	struct ems_cpc_msg cmd;
+
+	cmd.type = CPC_CMD_TYPE_CLEAR_CMD_QUEUE;
+	cmd.length = CPC_MSG_HEADER_LEN;
+	cmd.msgid = 0;
+	cmd.ts_sec = 0;
+	cmd.ts_nsec = 0;
+
+	return ems_usb_command_msg(dev, &cmd);
+}
+
 /* Start interface
  */
 static int ems_usb_start(struct ems_usb *dev)
@@ -978,11 +995,19 @@ static int ems_usb_set_bittiming_arm7(struct net_device *netdev)
 	struct can_bittiming *bt = &dev->can.bittiming;
 	struct cpc_sja1000_params *sja1000 =
 		&dev->active_params.msg.can_params.cc_params.sja1000;
+	int err;
 
 	sja1000->btr0 = ((bt->brp - 1) & 0x3f) | (((bt->sjw - 1) & 0x3) << 6);
 	sja1000->btr1 = ((bt->prop_seg + bt->phase_seg1 - 1) & 0xf) |
 		(((bt->phase_seg2 - 1) & 0x7) << 4);
 
+	/* If the command queue in the device is full of sending requests
+	 * a reinitialization would not be possible before the queue is cleared.
+	 */
+	err = ems_usb_clear_cmd_queue(dev);
+	if (err)
+		return err;
+
 	netdev_info(netdev, "Set bit timing for CPC-USB/ARM7\n");
 	netdev_info(netdev, "BTR0=0x%02x, BTR1=0x%02x\n",
 		    sja1000->btr0, sja1000->btr1);
-- 
2.26.2

-- 
EMS Dr. Thomas Wuensche e.K.
Sonnenhang 3
85304 Ilmmuenster
HR Ingolstadt, HRA 170106

Phone: +49-8441-490260
Fax  : +49-8441-81860
http://www.ems-wuensche.com

Re: [PATCH 07/17] can: ems_usb: Added CPC_ClearCmdQueue routine to get access to the interface even it is flooded with messages which cannot successfully be sent. Set timestamp to 0 in ems_usb_control_cmd.

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 2497 bytes --]

Is this a fix that is interesting for the stable tree?

On 11/6/20 6:01 PM, Gerhard Uttenthaler wrote:
> Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
> ---
>  drivers/net/can/usb/ems_usb.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
> index 66418e5af87d..c664af4499a1 100644
> --- a/drivers/net/can/usb/ems_usb.c
> +++ b/drivers/net/can/usb/ems_usb.c
> @@ -637,12 +637,29 @@ static int ems_usb_control_cmd(struct ems_usb *dev, u8 val)
>  	cmd.length = CPC_MSG_HEADER_LEN + 1;
>  
>  	cmd.msgid = 0;
> +	cmd.ts_sec = 0;
> +	cmd.ts_nsec = 0;
>  
>  	cmd.msg.generic[0] = val;
>  
>  	return ems_usb_command_msg(dev, &cmd);
>  }
>  
> +/* Send a CPC_ClearCmdQueue command
> + */
> +static int ems_usb_clear_cmd_queue(struct ems_usb *dev)
> +{
> +	struct ems_cpc_msg cmd;

please use C99 style initialization:

struct ems_cpc_msg cmd = {
	.type = CPC_CMD_TYPE_CLEAR_CMD_QUEUE,
	.length = CPC_MSG_HEADER_LEN,
};

> +
> +	cmd.type = CPC_CMD_TYPE_CLEAR_CMD_QUEUE;
> +	cmd.length = CPC_MSG_HEADER_LEN;
> +	cmd.msgid = 0;
> +	cmd.ts_sec = 0;
> +	cmd.ts_nsec = 0;
> +
> +	return ems_usb_command_msg(dev, &cmd);
> +}
> +
>  /* Start interface
>   */
>  static int ems_usb_start(struct ems_usb *dev)
> @@ -978,11 +995,19 @@ static int ems_usb_set_bittiming_arm7(struct net_device *netdev)
>  	struct can_bittiming *bt = &dev->can.bittiming;
>  	struct cpc_sja1000_params *sja1000 =
>  		&dev->active_params.msg.can_params.cc_params.sja1000;
> +	int err;
>  
>  	sja1000->btr0 = ((bt->brp - 1) & 0x3f) | (((bt->sjw - 1) & 0x3) << 6);
>  	sja1000->btr1 = ((bt->prop_seg + bt->phase_seg1 - 1) & 0xf) |
>  		(((bt->phase_seg2 - 1) & 0x7) << 4);
>  
> +	/* If the command queue in the device is full of sending requests
> +	 * a reinitialization would not be possible before the queue is cleared.
> +	 */
> +	err = ems_usb_clear_cmd_queue(dev);
> +	if (err)
> +		return err;
> +
>  	netdev_info(netdev, "Set bit timing for CPC-USB/ARM7\n");
>  	netdev_info(netdev, "BTR0=0x%02x, BTR1=0x%02x\n",
>  		    sja1000->btr0, sja1000->btr1);
> 

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 08/17] can: ems_usb: Modified ems_usb_read_bulk_callback to be able to handle also CPC-USB/FD

[Gerhard Uttenthaler]

Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
---
 drivers/net/can/usb/ems_usb.c | 83 ++++++++++++++++++-----------------
 1 file changed, 42 insertions(+), 41 deletions(-)

diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
index c664af4499a1..6a9ea6a4e687 100644
--- a/drivers/net/can/usb/ems_usb.c
+++ b/drivers/net/can/usb/ems_usb.c
@@ -460,6 +460,7 @@ static void ems_usb_read_bulk_callback(struct urb *urb)
 	struct ems_usb *dev = urb->context;
 	struct net_device *netdev;
 	int retval;
+	u32 length, start;
 
 	netdev = dev->netdev;
 
@@ -478,50 +479,50 @@ static void ems_usb_read_bulk_callback(struct urb *urb)
 		goto resubmit_urb;
 	}
 
-	if (urb->actual_length > CPC_HEADER_SIZE) {
+	length = urb->actual_length;
+	start = CPC_HEADER_SIZE;
+
+	while (length >= CPC_MSG_HEADER_LEN) {
 		struct ems_cpc_msg *msg;
 		u8 *ibuf = urb->transfer_buffer;
-		u8 msg_count, start;
-
-		msg_count = ibuf[0] & ~0x80;
-
-		start = CPC_HEADER_SIZE;
-
-		while (msg_count) {
-			msg = (struct ems_cpc_msg *)&ibuf[start];
-
-			switch (msg->type) {
-			case CPC_MSG_TYPE_CAN_STATE:
-				/* Process CAN state changes */
-				ems_usb_rx_err(dev, msg);
-				break;
-
-			case CPC_MSG_TYPE_CAN_FRAME:
-			case CPC_MSG_TYPE_EXT_CAN_FRAME:
-			case CPC_MSG_TYPE_RTR_FRAME:
-			case CPC_MSG_TYPE_EXT_RTR_FRAME:
-				ems_usb_rx_can_msg(dev, msg);
-				break;
-
-			case CPC_MSG_TYPE_CAN_FRAME_ERROR:
-				/* Process errorframe */
-				ems_usb_rx_err(dev, msg);
-				break;
-
-			case CPC_MSG_TYPE_OVERRUN:
-				/* Message lost while receiving */
-				ems_usb_rx_err(dev, msg);
-				break;
-			}
-
-			start += CPC_MSG_HEADER_LEN + msg->length;
-			msg_count--;
-
-			if (start > urb->transfer_buffer_length) {
-				netdev_err(netdev, "format error\n");
-				break;
-			}
+		u32 read_count;
+
+		msg = (struct ems_cpc_msg *)&ibuf[start];
+
+		switch (msg->type) {
+		case CPC_MSG_TYPE_CAN_STATE:
+			/* Process CAN state changes */
+			ems_usb_rx_err(dev, msg);
+			break;
+
+		case CPC_MSG_TYPE_CAN_FRAME:
+		case CPC_MSG_TYPE_EXT_CAN_FRAME:
+		case CPC_MSG_TYPE_RTR_FRAME:
+		case CPC_MSG_TYPE_EXT_RTR_FRAME:
+			ems_usb_rx_can_msg(dev, msg);
+			break;
+
+		case CPC_MSG_TYPE_CAN_FRAME_ERROR:
+			/* Process errorframe */
+			ems_usb_rx_err(dev, msg);
+			break;
+
+		case CPC_MSG_TYPE_OVERRUN:
+			/* Message lost while receiving */
+			ems_usb_rx_err(dev, msg);
+			break;
 		}
+
+		read_count = CPC_MSG_HEADER_LEN + msg->length;
+		start += read_count;
+
+		if (start > urb->transfer_buffer_length) {
+			netdev_err(netdev, "format error\n");
+			break;
+		}
+
+		if (read_count <= length)
+			length -= read_count;
 	}
 
 resubmit_urb:
-- 
2.26.2

-- 
EMS Dr. Thomas Wuensche e.K.
Sonnenhang 3
85304 Ilmmuenster
HR Ingolstadt, HRA 170106

Phone: +49-8441-490260
Fax  : +49-8441-81860
http://www.ems-wuensche.com

Re: [PATCH 08/17] can: ems_usb: Modified ems_usb_read_bulk_callback to be able to handle also CPC-USB/FD

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 3421 bytes --]

On 11/6/20 6:01 PM, Gerhard Uttenthaler wrote:
> Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
> ---
>  drivers/net/can/usb/ems_usb.c | 83 ++++++++++++++++++-----------------
>  1 file changed, 42 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
> index c664af4499a1..6a9ea6a4e687 100644
> --- a/drivers/net/can/usb/ems_usb.c
> +++ b/drivers/net/can/usb/ems_usb.c
> @@ -460,6 +460,7 @@ static void ems_usb_read_bulk_callback(struct urb *urb)
>  	struct ems_usb *dev = urb->context;
>  	struct net_device *netdev;
>  	int retval;
> +	u32 length, start;
>  
>  	netdev = dev->netdev;
>  
> @@ -478,50 +479,50 @@ static void ems_usb_read_bulk_callback(struct urb *urb)
>  		goto resubmit_urb;
>  	}
>  
> -	if (urb->actual_length > CPC_HEADER_SIZE) {
> +	length = urb->actual_length;
> +	start = CPC_HEADER_SIZE;
> +
> +	while (length >= CPC_MSG_HEADER_LEN) {
>  		struct ems_cpc_msg *msg;
>  		u8 *ibuf = urb->transfer_buffer;
> -		u8 msg_count, start;
> -
> -		msg_count = ibuf[0] & ~0x80;
> -
> -		start = CPC_HEADER_SIZE;
> -
> -		while (msg_count) {
> -			msg = (struct ems_cpc_msg *)&ibuf[start];
> -
> -			switch (msg->type) {
> -			case CPC_MSG_TYPE_CAN_STATE:
> -				/* Process CAN state changes */
> -				ems_usb_rx_err(dev, msg);
> -				break;
> -
> -			case CPC_MSG_TYPE_CAN_FRAME:
> -			case CPC_MSG_TYPE_EXT_CAN_FRAME:
> -			case CPC_MSG_TYPE_RTR_FRAME:
> -			case CPC_MSG_TYPE_EXT_RTR_FRAME:
> -				ems_usb_rx_can_msg(dev, msg);
> -				break;
> -
> -			case CPC_MSG_TYPE_CAN_FRAME_ERROR:
> -				/* Process errorframe */
> -				ems_usb_rx_err(dev, msg);
> -				break;
> -
> -			case CPC_MSG_TYPE_OVERRUN:
> -				/* Message lost while receiving */
> -				ems_usb_rx_err(dev, msg);
> -				break;
> -			}
> -
> -			start += CPC_MSG_HEADER_LEN + msg->length;
> -			msg_count--;
> -
> -			if (start > urb->transfer_buffer_length) {
> -				netdev_err(netdev, "format error\n");
> -				break;
> -			}
> +		u32 read_count;
> +
> +		msg = (struct ems_cpc_msg *)&ibuf[start];
> +
> +		switch (msg->type) {

Is there a check, that that you don't access the buffer after its actual_length?

> +		case CPC_MSG_TYPE_CAN_STATE:
> +			/* Process CAN state changes */
> +			ems_usb_rx_err(dev, msg);
> +			break;
> +
> +		case CPC_MSG_TYPE_CAN_FRAME:
> +		case CPC_MSG_TYPE_EXT_CAN_FRAME:
> +		case CPC_MSG_TYPE_RTR_FRAME:
> +		case CPC_MSG_TYPE_EXT_RTR_FRAME:
> +			ems_usb_rx_can_msg(dev, msg);
> +			break;
> +
> +		case CPC_MSG_TYPE_CAN_FRAME_ERROR:
> +			/* Process errorframe */
> +			ems_usb_rx_err(dev, msg);
> +			break;
> +
> +		case CPC_MSG_TYPE_OVERRUN:
> +			/* Message lost while receiving */
> +			ems_usb_rx_err(dev, msg);
> +			break;
>  		}
> +
> +		read_count = CPC_MSG_HEADER_LEN + msg->length;
> +		start += read_count;
> +
> +		if (start > urb->transfer_buffer_length) {
> +			netdev_err(netdev, "format error\n");
> +			break;
> +		}
> +
> +		if (read_count <= length)
> +			length -= read_count;
>  	}
>  
>  resubmit_urb:
> 

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 09/17] can: ems_usb: For CPC-USB/FD added clock definitions, bittiming constants, set_bittiming functions, bittiming init function and add all that to probe function

[Gerhard Uttenthaler]

Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
---
 drivers/net/can/usb/ems_usb.c | 141 +++++++++++++++++++++++++++++++++-
 1 file changed, 139 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
index 6a9ea6a4e687..d6b52b265536 100644
--- a/drivers/net/can/usb/ems_usb.c
+++ b/drivers/net/can/usb/ems_usb.c
@@ -108,6 +108,17 @@ MODULE_LICENSE("GPL v2");
  */
 #define EMS_USB_ARM7_CLOCK 8000000
 
+/* CPC-USB/FD supports the following CAN clocks
+ */
+#define EMS_USB_FD_8MHZ   8000000
+#define EMS_USB_FD_16MHZ 16000000
+#define EMS_USB_FD_20MHZ 20000000
+#define EMS_USB_FD_24MHZ 24000000
+#define EMS_USB_FD_32MHZ 32000000
+#define EMS_USB_FD_40MHZ 40000000
+#define EMS_USB_FD_80MHZ 80000000
+#define EMS_USB_FD_CLOCK EMS_USB_FD_40MHZ
+
 #define CPC_TX_QUEUE_TRIGGER_LOW	25
 #define CPC_TX_QUEUE_TRIGGER_HIGH	35
 
@@ -970,6 +981,30 @@ static const struct can_bittiming_const ems_usb_bittiming_const_arm7 = {
 	.brp_inc = 1,
 };
 
+static const struct can_bittiming_const ems_usb_bittiming_const_generic = {
+	.name = "ems_usb_fd",
+	.tseg1_min = 1,
+	.tseg1_max = 256,
+	.tseg2_min = 1,
+	.tseg2_max = 128,
+	.sjw_max = 128,
+	.brp_min = 1,
+	.brp_max = 512,
+	.brp_inc = 1,
+};
+
+static const struct can_bittiming_const ems_usb_bittiming_const_generic_data = {
+	.name = "ems_usb_fd",
+	.tseg1_min = 1,
+	.tseg1_max = 16,
+	.tseg2_min = 1,
+	.tseg2_max = 16,
+	.sjw_max = 16,
+	.brp_min = 1,
+	.brp_max = 32,
+	.brp_inc = 1,
+};
+
 static int ems_usb_set_mode(struct net_device *netdev, enum can_mode mode)
 {
 	struct ems_usb *dev = netdev_priv(netdev);
@@ -1016,6 +1051,76 @@ static int ems_usb_set_bittiming_arm7(struct net_device *netdev)
 	return ems_usb_command_msg(dev, &dev->active_params);
 }
 
+static int ems_usb_set_bittiming_generic(struct net_device *netdev)
+{
+	struct ems_usb *dev = netdev_priv(netdev);
+	struct can_bittiming *bt = &dev->can.bittiming;
+	struct cpc_generic_can_params *gcp =
+		&dev->active_params.msg.can_params.cc_params.generic;
+	int err;
+
+	gcp->config = 0;
+	gcp->can_clk = dev->can.clock.freq;
+
+	gcp->n.tseg1 = bt->prop_seg + bt->phase_seg1;
+	gcp->n.tseg2 = bt->phase_seg2;
+	gcp->n.brp = bt->brp;
+	gcp->n.sjw = bt->sjw;
+
+	err = ems_usb_clear_cmd_queue(dev);
+	if (err)
+		return err;
+
+	netdev_info(netdev, "Set nominal bit timing for CPC-USB/FD with config %X\n",
+		    gcp->config);
+	netdev_info(netdev, "CAN Clock: %uMHz, Tseg1: %u, Tseg2: %u, BRP: %u, SJW: %u\n",
+		    gcp->can_clk / 1000000,
+		    gcp->n.tseg1,
+		    gcp->n.tseg2,
+		    gcp->n.brp,
+		    gcp->n.sjw);
+
+	return ems_usb_command_msg(dev, &dev->active_params);
+}
+
+static int ems_usb_set_bittiming_generic_data(struct net_device *netdev)
+{
+	struct ems_usb *dev = netdev_priv(netdev);
+	struct can_bittiming *bt = &dev->can.data_bittiming;
+	struct cpc_generic_can_params *gcp =
+		&dev->active_params.msg.can_params.cc_params.generic;
+	int err;
+
+	if (dev->can.ctrlmode & CAN_CTRLMODE_FD) {
+		gcp->config |= CPC_GENERICCONF_FD;
+		if (dev->can.ctrlmode & CAN_CTRLMODE_FD_NON_ISO)
+			gcp->config |= CPC_GENERICCONF_FD_BOSCH;
+	} else {
+		// If CAN FD is not requested we can return here
+		return 0;
+	}
+
+	gcp->d.tseg1 = bt->prop_seg + bt->phase_seg1;
+	gcp->d.tseg2 = bt->phase_seg2;
+	gcp->d.brp = bt->brp;
+	gcp->d.sjw = bt->sjw;
+
+	err = ems_usb_clear_cmd_queue(dev);
+	if (err)
+		return err;
+
+	netdev_info(netdev, "Set data bit timing for CPC-USB/FD with config %X\n",
+		    gcp->config);
+	netdev_info(netdev, "CAN Clock: %uMHz, Tseg1: %u, Tseg2: %u, BRP: %u, SJW: %u\n",
+		    gcp->can_clk / 1000000,
+		    gcp->d.tseg1,
+		    gcp->d.tseg2,
+		    gcp->d.brp,
+		    gcp->d.sjw);
+
+	return ems_usb_command_msg(dev, &dev->active_params);
+}
+
 static void init_params_sja1000(struct ems_cpc_msg *msg)
 {
 	struct cpc_sja1000_params *sja1000 =
@@ -1024,6 +1129,8 @@ static void init_params_sja1000(struct ems_cpc_msg *msg)
 	msg->type = CPC_CMD_TYPE_CAN_PARAMS;
 	msg->length = sizeof(struct cpc_can_params);
 	msg->msgid = 0;
+	msg->ts_sec = 0;
+	msg->ts_nsec = 0;
 
 	msg->msg.can_params.cc_type = CPC_CC_TYPE_SJA1000;
 
@@ -1046,6 +1153,24 @@ static void init_params_sja1000(struct ems_cpc_msg *msg)
 	sja1000->mode = SJA1000_MOD_RM;
 }
 
+static void init_params_generic(struct ems_cpc_msg *msg)
+{
+	struct cpc_generic_can_params *gcp =
+		&msg->msg.can_params.cc_params.generic;
+
+	msg->type = CPC_CMD_TYPE_CAN_PARAMS;
+	msg->length = sizeof(struct cpc_can_params);
+	msg->msgid = 0;
+	msg->ts_sec = 0;
+	msg->ts_nsec = 0;
+
+	memset((u8 *)gcp, 0, sizeof(struct cpc_generic_can_params));
+	msg->msg.can_params.cc_type = CPC_CC_TYPE_GENERIC;
+
+	gcp->config = CPC_GENERICCONF_RESET_MODE;
+	gcp->can_clk = EMS_USB_FD_CLOCK;
+}
+
 /* probe function for new CPC-USB devices
  */
 static int ems_usb_probe(struct usb_interface *intf,
@@ -1076,14 +1201,26 @@ static int ems_usb_probe(struct usb_interface *intf,
 	dev->can.bittiming_const = &ems_usb_bittiming_const_arm7;
 	dev->can.do_set_bittiming = ems_usb_set_bittiming_arm7;
 	dev->can.do_set_mode = ems_usb_set_mode;
-	dev->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY;
+	dev->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES |
+				      CAN_CTRLMODE_LISTENONLY;
 	init_params_sja1000(&dev->active_params);
 	dev->ems_usb_write_mode = ems_usb_write_mode_arm7;
 	dev->bulk_rd_buf_size = 64;
 	break;
 
 	case USB_CPCUSB_FD_PRODUCT_ID:
-	// Placeholder for next patchess
+		dev->can.clock.freq = EMS_USB_FD_CLOCK;
+		dev->can.bittiming_const = &ems_usb_bittiming_const_generic;
+		dev->can.data_bittiming_const = &ems_usb_bittiming_const_generic_data;
+		dev->can.do_set_bittiming = ems_usb_set_bittiming_generic;
+		dev->can.do_set_data_bittiming = ems_usb_set_bittiming_generic_data;
+		dev->can.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
+				CAN_CTRLMODE_ONE_SHOT |
+				CAN_CTRLMODE_BERR_REPORTING |
+				CAN_CTRLMODE_FD |
+				CAN_CTRLMODE_FD_NON_ISO;
+		init_params_generic(&dev->active_params);
+		dev->bulk_rd_buf_size = 512;
 	break;
 
 	default:
-- 
2.26.2

-- 
EMS Dr. Thomas Wuensche e.K.
Sonnenhang 3
85304 Ilmmuenster
HR Ingolstadt, HRA 170106

Phone: +49-8441-490260
Fax  : +49-8441-81860
http://www.ems-wuensche.com

Re: [PATCH 09/17] can: ems_usb: For CPC-USB/FD added clock definitions, bittiming constants, set_bittiming functions, bittiming init function and add all that to probe function

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 7325 bytes --]

On 11/6/20 6:01 PM, Gerhard Uttenthaler wrote:
> Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
> ---
>  drivers/net/can/usb/ems_usb.c | 141 +++++++++++++++++++++++++++++++++-
>  1 file changed, 139 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
> index 6a9ea6a4e687..d6b52b265536 100644
> --- a/drivers/net/can/usb/ems_usb.c
> +++ b/drivers/net/can/usb/ems_usb.c
> @@ -108,6 +108,17 @@ MODULE_LICENSE("GPL v2");
>   */
>  #define EMS_USB_ARM7_CLOCK 8000000
>  
> +/* CPC-USB/FD supports the following CAN clocks
> + */
> +#define EMS_USB_FD_8MHZ   8000000
                          ^^^ one space only
> +#define EMS_USB_FD_16MHZ 16000000
> +#define EMS_USB_FD_20MHZ 20000000
> +#define EMS_USB_FD_24MHZ 24000000
> +#define EMS_USB_FD_32MHZ 32000000
> +#define EMS_USB_FD_40MHZ 40000000
> +#define EMS_USB_FD_80MHZ 80000000

are these used?

> +#define EMS_USB_FD_CLOCK EMS_USB_FD_40MHZ
> +
>  #define CPC_TX_QUEUE_TRIGGER_LOW	25
>  #define CPC_TX_QUEUE_TRIGGER_HIGH	35
>  
> @@ -970,6 +981,30 @@ static const struct can_bittiming_const ems_usb_bittiming_const_arm7 = {
>  	.brp_inc = 1,
>  };
>  
> +static const struct can_bittiming_const ems_usb_bittiming_const_generic = {
> +	.name = "ems_usb_fd",
> +	.tseg1_min = 1,
> +	.tseg1_max = 256,
> +	.tseg2_min = 1,
> +	.tseg2_max = 128,
> +	.sjw_max = 128,
> +	.brp_min = 1,
> +	.brp_max = 512,
> +	.brp_inc = 1,
> +};
> +
> +static const struct can_bittiming_const ems_usb_bittiming_const_generic_data = {
> +	.name = "ems_usb_fd",
> +	.tseg1_min = 1,
> +	.tseg1_max = 16,
> +	.tseg2_min = 1,
> +	.tseg2_max = 16,
> +	.sjw_max = 16,
> +	.brp_min = 1,
> +	.brp_max = 32,
> +	.brp_inc = 1,
> +};
> +
>  static int ems_usb_set_mode(struct net_device *netdev, enum can_mode mode)
>  {
>  	struct ems_usb *dev = netdev_priv(netdev);
> @@ -1016,6 +1051,76 @@ static int ems_usb_set_bittiming_arm7(struct net_device *netdev)
>  	return ems_usb_command_msg(dev, &dev->active_params);
>  }
>  
> +static int ems_usb_set_bittiming_generic(struct net_device *netdev)
> +{
> +	struct ems_usb *dev = netdev_priv(netdev);
> +	struct can_bittiming *bt = &dev->can.bittiming;
> +	struct cpc_generic_can_params *gcp =
> +		&dev->active_params.msg.can_params.cc_params.generic;
> +	int err;
> +
> +	gcp->config = 0;
> +	gcp->can_clk = dev->can.clock.freq;
> +
> +	gcp->n.tseg1 = bt->prop_seg + bt->phase_seg1;
> +	gcp->n.tseg2 = bt->phase_seg2;
> +	gcp->n.brp = bt->brp;
> +	gcp->n.sjw = bt->sjw;
> +
> +	err = ems_usb_clear_cmd_queue(dev);
> +	if (err)
> +		return err;
> +
> +	netdev_info(netdev, "Set nominal bit timing for CPC-USB/FD with config %X\n",
> +		    gcp->config);
> +	netdev_info(netdev, "CAN Clock: %uMHz, Tseg1: %u, Tseg2: %u, BRP: %u, SJW: %u\n",
> +		    gcp->can_clk / 1000000,
> +		    gcp->n.tseg1,
> +		    gcp->n.tseg2,
> +		    gcp->n.brp,
> +		    gcp->n.sjw);

I suggest to have a more quit driver, make this _dbg() instead.

> +
> +	return ems_usb_command_msg(dev, &dev->active_params);
> +}
> +
> +static int ems_usb_set_bittiming_generic_data(struct net_device *netdev)
> +{
> +	struct ems_usb *dev = netdev_priv(netdev);
> +	struct can_bittiming *bt = &dev->can.data_bittiming;
> +	struct cpc_generic_can_params *gcp =
> +		&dev->active_params.msg.can_params.cc_params.generic;
> +	int err;
> +
> +	if (dev->can.ctrlmode & CAN_CTRLMODE_FD) {
> +		gcp->config |= CPC_GENERICCONF_FD;
> +		if (dev->can.ctrlmode & CAN_CTRLMODE_FD_NON_ISO)
> +			gcp->config |= CPC_GENERICCONF_FD_BOSCH;
> +	} else {
> +		// If CAN FD is not requested we can return here

no C++ comments

Better make it:

if (!(dev->can.ctrlmode & CAN_CTRLMODE_FD))
	return 0;

> +		return 0;
> +	}
> +
> +	gcp->d.tseg1 = bt->prop_seg + bt->phase_seg1;
> +	gcp->d.tseg2 = bt->phase_seg2;
> +	gcp->d.brp = bt->brp;
> +	gcp->d.sjw = bt->sjw;
> +
> +	err = ems_usb_clear_cmd_queue(dev);
> +	if (err)
> +		return err;
> +
> +	netdev_info(netdev, "Set data bit timing for CPC-USB/FD with config %X\n",
> +		    gcp->config);
> +	netdev_info(netdev, "CAN Clock: %uMHz, Tseg1: %u, Tseg2: %u, BRP: %u, SJW: %u\n",
> +		    gcp->can_clk / 1000000,
> +		    gcp->d.tseg1,
> +		    gcp->d.tseg2,
> +		    gcp->d.brp,
> +		    gcp->d.sjw);

I suggest to have a more quit driver, make this _dbg() instead.

> +
> +	return ems_usb_command_msg(dev, &dev->active_params);
> +}
> +
>  static void init_params_sja1000(struct ems_cpc_msg *msg)
>  {
>  	struct cpc_sja1000_params *sja1000 =
> @@ -1024,6 +1129,8 @@ static void init_params_sja1000(struct ems_cpc_msg *msg)
>  	msg->type = CPC_CMD_TYPE_CAN_PARAMS;
>  	msg->length = sizeof(struct cpc_can_params);
>  	msg->msgid = 0;
> +	msg->ts_sec = 0;
> +	msg->ts_nsec = 0;
>  
>  	msg->msg.can_params.cc_type = CPC_CC_TYPE_SJA1000;
>  
> @@ -1046,6 +1153,24 @@ static void init_params_sja1000(struct ems_cpc_msg *msg)
>  	sja1000->mode = SJA1000_MOD_RM;
>  }
>  
> +static void init_params_generic(struct ems_cpc_msg *msg)
> +{
> +	struct cpc_generic_can_params *gcp =
> +		&msg->msg.can_params.cc_params.generic;
> +
> +	msg->type = CPC_CMD_TYPE_CAN_PARAMS;
> +	msg->length = sizeof(struct cpc_can_params);
> +	msg->msgid = 0;
> +	msg->ts_sec = 0;
> +	msg->ts_nsec = 0;
> +
> +	memset((u8 *)gcp, 0, sizeof(struct cpc_generic_can_params));
> +	msg->msg.can_params.cc_type = CPC_CC_TYPE_GENERIC;
> +
> +	gcp->config = CPC_GENERICCONF_RESET_MODE;
> +	gcp->can_clk = EMS_USB_FD_CLOCK;
> +}
> +
>  /* probe function for new CPC-USB devices
>   */
>  static int ems_usb_probe(struct usb_interface *intf,
> @@ -1076,14 +1201,26 @@ static int ems_usb_probe(struct usb_interface *intf,
>  	dev->can.bittiming_const = &ems_usb_bittiming_const_arm7;
>  	dev->can.do_set_bittiming = ems_usb_set_bittiming_arm7;
>  	dev->can.do_set_mode = ems_usb_set_mode;
> -	dev->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_LISTENONLY;
> +	dev->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES |
> +				      CAN_CTRLMODE_LISTENONLY;

unrelated

>  	init_params_sja1000(&dev->active_params);
>  	dev->ems_usb_write_mode = ems_usb_write_mode_arm7;
>  	dev->bulk_rd_buf_size = 64;
>  	break;
>  
>  	case USB_CPCUSB_FD_PRODUCT_ID:
> -	// Placeholder for next patchess
> +		dev->can.clock.freq = EMS_USB_FD_CLOCK;
> +		dev->can.bittiming_const = &ems_usb_bittiming_const_generic;
> +		dev->can.data_bittiming_const = &ems_usb_bittiming_const_generic_data;
> +		dev->can.do_set_bittiming = ems_usb_set_bittiming_generic;
> +		dev->can.do_set_data_bittiming = ems_usb_set_bittiming_generic_data;
> +		dev->can.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
> +				CAN_CTRLMODE_ONE_SHOT |
> +				CAN_CTRLMODE_BERR_REPORTING |
> +				CAN_CTRLMODE_FD |
> +				CAN_CTRLMODE_FD_NON_ISO;
> +		init_params_generic(&dev->active_params);
> +		dev->bulk_rd_buf_size = 512;
>  	break;
>  
>  	default:
> 

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 09/17] can: ems_usb: For CPC-USB/FD added clock definitions, bittiming constants, set_bittiming functions, bittiming init function and add all that to probe function

[Gerhard Uttenthaler]

Am 06.11.20 um 18:51 schrieb Marc Kleine-Budde:
> On 11/6/20 6:01 PM, Gerhard Uttenthaler wrote:
>> Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
>> ---
>>  drivers/net/can/usb/ems_usb.c | 141 +++++++++++++++++++++++++++++++++-
>>  1 file changed, 139 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
>> index 6a9ea6a4e687..d6b52b265536 100644
>> --- a/drivers/net/can/usb/ems_usb.c
>> +++ b/drivers/net/can/usb/ems_usb.c
>> @@ -108,6 +108,17 @@ MODULE_LICENSE("GPL v2");
>>   */
>>  #define EMS_USB_ARM7_CLOCK 8000000
>>  
>> +/* CPC-USB/FD supports the following CAN clocks
>> + */
>> +#define EMS_USB_FD_8MHZ   8000000
>                           ^^^ one space only
I can do that, no problem, but is it really better readable?

>> +#define EMS_USB_FD_16MHZ 16000000
>> +#define EMS_USB_FD_20MHZ 20000000
>> +#define EMS_USB_FD_24MHZ 24000000
>> +#define EMS_USB_FD_32MHZ 32000000
>> +#define EMS_USB_FD_40MHZ 40000000
>> +#define EMS_USB_FD_80MHZ 80000000
> 
> are these used?
These frequencies are supported by the interface. I gave them as a
reference only, if someone wants to compile it for a different clock.
Used is:
#define EMS_USB_FD_CLOCK EMS_USB_FD_40MHZ

-- 
Gerhard

-- 
EMS Dr. Thomas Wuensche e.K.
Sonnenhang 3
85304 Ilmmuenster
HR Ingolstadt, HRA 170106

Phone: +49-8441-490260
Fax  : +49-8441-81860
http://www.ems-wuensche.com

Re: [PATCH 09/17] can: ems_usb: For CPC-USB/FD added clock definitions, bittiming constants, set_bittiming functions, bittiming init function and add all that to probe function

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1753 bytes --]

On 11/11/20 11:22 AM, Gerhard Uttenthaler wrote:
> Am 06.11.20 um 18:51 schrieb Marc Kleine-Budde:
>> On 11/6/20 6:01 PM, Gerhard Uttenthaler wrote:
>>> Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
>>> ---
>>>  drivers/net/can/usb/ems_usb.c | 141 +++++++++++++++++++++++++++++++++-
>>>  1 file changed, 139 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
>>> index 6a9ea6a4e687..d6b52b265536 100644
>>> --- a/drivers/net/can/usb/ems_usb.c
>>> +++ b/drivers/net/can/usb/ems_usb.c
>>> @@ -108,6 +108,17 @@ MODULE_LICENSE("GPL v2");
>>>   */
>>>  #define EMS_USB_ARM7_CLOCK 8000000
>>>  
>>> +/* CPC-USB/FD supports the following CAN clocks
>>> + */
>>> +#define EMS_USB_FD_8MHZ   8000000
>>                           ^^^ one space only
> I can do that, no problem, but is it really better readable?

The rest of the driver seems to use one space, at least what I see from the
contect here.

>>> +#define EMS_USB_FD_16MHZ 16000000
>>> +#define EMS_USB_FD_20MHZ 20000000
>>> +#define EMS_USB_FD_24MHZ 24000000
>>> +#define EMS_USB_FD_32MHZ 32000000
>>> +#define EMS_USB_FD_40MHZ 40000000
>>> +#define EMS_USB_FD_80MHZ 80000000
>>
>> are these used?

> These frequencies are supported by the interface. I gave them as a
> reference only, if someone wants to compile it for a different clock.
> Used is:
> #define EMS_USB_FD_CLOCK EMS_USB_FD_40MHZ

Ok

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 10/17] can: ems_usb: Added receive routine for CAN FD messages and its call in ems_usb_read_bulk_callback

[Gerhard Uttenthaler]

Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
---
 drivers/net/can/usb/ems_usb.c | 45 +++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
index d6b52b265536..a4d9a1b2d2f0 100644
--- a/drivers/net/can/usb/ems_usb.c
+++ b/drivers/net/can/usb/ems_usb.c
@@ -389,6 +389,47 @@ static void ems_usb_rx_can_msg(struct ems_usb *dev, struct ems_cpc_msg *msg)
 	netif_rx(skb);
 }
 
+static void ems_usb_rx_canfd_msg(struct ems_usb *dev, struct ems_cpc_msg *msg)
+{
+	struct cpc_canfd_msg *fd_msg = &msg->msg.canfd_msg;
+
+	/* Although the CPC_FDFLAG_NONCANFD_MSG flag
+	 * should not be set with a received message,
+	 * it seems better to have checked it anyway.
+	 */
+	if (!(fd_msg->flags & CPC_FDFLAG_NONCANFD_MSG)) {
+		/* CAN FD frame */
+		struct canfd_frame *cfd;
+		struct sk_buff *skb;
+		int i;
+		struct net_device_stats *stats = &dev->netdev->stats;
+
+		skb = alloc_canfd_skb(dev->netdev, &cfd);
+		if (!skb)
+			return;
+
+		cfd->can_id = le32_to_cpu(fd_msg->id);
+		cfd->len = fd_msg->length;
+
+		for (i = 0; i < cfd->len; i++)
+			cfd->data[i] = fd_msg->msg[i];
+
+		cfd->flags = 0;
+		if (fd_msg->flags & CPC_FDFLAG_BRS)
+			cfd->flags |= CANFD_BRS;
+
+		if (fd_msg->flags & CPC_FDFLAG_ESI)
+			cfd->flags |= CANFD_ESI;
+
+		if (fd_msg->flags & CPC_FDFLAG_XTD)
+			cfd->can_id |= CAN_EFF_FLAG;
+
+		stats->rx_packets++;
+		stats->rx_bytes += cfd->len;
+		netif_rx(skb);
+	}
+}
+
 static void ems_usb_rx_err(struct ems_usb *dev, struct ems_cpc_msg *msg)
 {
 	struct can_frame *cf;
@@ -513,6 +554,10 @@ static void ems_usb_read_bulk_callback(struct urb *urb)
 			ems_usb_rx_can_msg(dev, msg);
 			break;
 
+		case CPC_MSG_TYPE_CANFD_FRAME:
+			ems_usb_rx_canfd_msg(dev, msg);
+			break;
+
 		case CPC_MSG_TYPE_CAN_FRAME_ERROR:
 			/* Process errorframe */
 			ems_usb_rx_err(dev, msg);
-- 
2.26.2

-- 
EMS Dr. Thomas Wuensche e.K.
Sonnenhang 3
85304 Ilmmuenster
HR Ingolstadt, HRA 170106

Phone: +49-8441-490260
Fax  : +49-8441-81860
http://www.ems-wuensche.com

Re: [PATCH 10/17] can: ems_usb: Added receive routine for CAN FD messages and its call in ems_usb_read_bulk_callback

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 2494 bytes --]

On 11/6/20 6:01 PM, Gerhard Uttenthaler wrote:
> Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
> ---
>  drivers/net/can/usb/ems_usb.c | 45 +++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
> index d6b52b265536..a4d9a1b2d2f0 100644
> --- a/drivers/net/can/usb/ems_usb.c
> +++ b/drivers/net/can/usb/ems_usb.c
> @@ -389,6 +389,47 @@ static void ems_usb_rx_can_msg(struct ems_usb *dev, struct ems_cpc_msg *msg)
>  	netif_rx(skb);
>  }
>  
> +static void ems_usb_rx_canfd_msg(struct ems_usb *dev, struct ems_cpc_msg *msg)
> +{
> +	struct cpc_canfd_msg *fd_msg = &msg->msg.canfd_msg;
> +
> +	/* Although the CPC_FDFLAG_NONCANFD_MSG flag
> +	 * should not be set with a received message,
> +	 * it seems better to have checked it anyway.
> +	 */
> +	if (!(fd_msg->flags & CPC_FDFLAG_NONCANFD_MSG)) {

please exit early, to safe on indention level.

> +		/* CAN FD frame */
> +		struct canfd_frame *cfd;
> +		struct sk_buff *skb;
> +		int i;
> +		struct net_device_stats *stats = &dev->netdev->stats;
> +
> +		skb = alloc_canfd_skb(dev->netdev, &cfd);
> +		if (!skb)
> +			return;
> +
> +		cfd->can_id = le32_to_cpu(fd_msg->id);
> +		cfd->len = fd_msg->length;
> +
> +		for (i = 0; i < cfd->len; i++)
> +			cfd->data[i] = fd_msg->msg[i];
> +
> +		cfd->flags = 0;
> +		if (fd_msg->flags & CPC_FDFLAG_BRS)
> +			cfd->flags |= CANFD_BRS;
> +
> +		if (fd_msg->flags & CPC_FDFLAG_ESI)
> +			cfd->flags |= CANFD_ESI;
> +
> +		if (fd_msg->flags & CPC_FDFLAG_XTD)
> +			cfd->can_id |= CAN_EFF_FLAG;
> +
> +		stats->rx_packets++;
> +		stats->rx_bytes += cfd->len;
> +		netif_rx(skb);
> +	}
> +}
> +
>  static void ems_usb_rx_err(struct ems_usb *dev, struct ems_cpc_msg *msg)
>  {
>  	struct can_frame *cf;
> @@ -513,6 +554,10 @@ static void ems_usb_read_bulk_callback(struct urb *urb)
>  			ems_usb_rx_can_msg(dev, msg);
>  			break;
>  
> +		case CPC_MSG_TYPE_CANFD_FRAME:
> +			ems_usb_rx_canfd_msg(dev, msg);
> +			break;
> +
>  		case CPC_MSG_TYPE_CAN_FRAME_ERROR:
>  			/* Process errorframe */
>  			ems_usb_rx_err(dev, msg);
> 

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 10/17] can: ems_usb: Added receive routine for CAN FD messages and its call in ems_usb_read_bulk_callback

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 2450 bytes --]

On 11/6/20 6:01 PM, Gerhard Uttenthaler wrote:
> Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
> ---
>  drivers/net/can/usb/ems_usb.c | 45 +++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
> index d6b52b265536..a4d9a1b2d2f0 100644
> --- a/drivers/net/can/usb/ems_usb.c
> +++ b/drivers/net/can/usb/ems_usb.c
> @@ -389,6 +389,47 @@ static void ems_usb_rx_can_msg(struct ems_usb *dev, struct ems_cpc_msg *msg)
>  	netif_rx(skb);
>  }
>  
> +static void ems_usb_rx_canfd_msg(struct ems_usb *dev, struct ems_cpc_msg *msg)
> +{
> +	struct cpc_canfd_msg *fd_msg = &msg->msg.canfd_msg;
> +
> +	/* Although the CPC_FDFLAG_NONCANFD_MSG flag
> +	 * should not be set with a received message,
> +	 * it seems better to have checked it anyway.
> +	 */
> +	if (!(fd_msg->flags & CPC_FDFLAG_NONCANFD_MSG)) {
> +		/* CAN FD frame */
> +		struct canfd_frame *cfd;
> +		struct sk_buff *skb;
> +		int i;
> +		struct net_device_stats *stats = &dev->netdev->stats;
> +
> +		skb = alloc_canfd_skb(dev->netdev, &cfd);
> +		if (!skb)
> +			return;
> +
> +		cfd->can_id = le32_to_cpu(fd_msg->id);
> +		cfd->len = fd_msg->length;
> +
> +		for (i = 0; i < cfd->len; i++)
> +			cfd->data[i] = fd_msg->msg[i];

memcpy()

> +
> +		cfd->flags = 0;
> +		if (fd_msg->flags & CPC_FDFLAG_BRS)
> +			cfd->flags |= CANFD_BRS;
> +
> +		if (fd_msg->flags & CPC_FDFLAG_ESI)
> +			cfd->flags |= CANFD_ESI;
> +
> +		if (fd_msg->flags & CPC_FDFLAG_XTD)
> +			cfd->can_id |= CAN_EFF_FLAG;
> +
> +		stats->rx_packets++;
> +		stats->rx_bytes += cfd->len;
> +		netif_rx(skb);
> +	}
> +}
> +
>  static void ems_usb_rx_err(struct ems_usb *dev, struct ems_cpc_msg *msg)
>  {
>  	struct can_frame *cf;
> @@ -513,6 +554,10 @@ static void ems_usb_read_bulk_callback(struct urb *urb)
>  			ems_usb_rx_can_msg(dev, msg);
>  			break;
>  
> +		case CPC_MSG_TYPE_CANFD_FRAME:
> +			ems_usb_rx_canfd_msg(dev, msg);
> +			break;
> +
>  		case CPC_MSG_TYPE_CAN_FRAME_ERROR:
>  			/* Process errorframe */
>  			ems_usb_rx_err(dev, msg);
> 


-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 11/17] can: ems_usb: Added ems_usb_write_mode_fd and its call in device probe routine. Fixed indentation.

[Gerhard Uttenthaler]

Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
---
 drivers/net/can/usb/ems_usb.c | 49 +++++++++++++++++++++++++++--------
 1 file changed, 38 insertions(+), 11 deletions(-)

diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
index a4d9a1b2d2f0..b51a5eb65946 100644
--- a/drivers/net/can/usb/ems_usb.c
+++ b/drivers/net/can/usb/ems_usb.c
@@ -683,6 +683,32 @@ static int ems_usb_write_mode_arm7(struct ems_usb *dev, u32 mode)
 	return ems_usb_command_msg(dev, &dev->active_params);
 }
 
+static int ems_usb_write_mode_fd(struct ems_usb *dev, u32 mode)
+{
+	struct cpc_generic_can_params *gcp =
+		&dev->active_params.msg.can_params.cc_params.generic;
+
+	if (mode == CPC_USB_RESET_MODE) {
+		gcp->config |= CPC_GENERICCONF_RESET_MODE;
+	} else if (mode == CPC_USB_RUN_MODE) {
+		gcp->config &= ~CPC_GENERICCONF_RESET_MODE;
+
+		if (dev->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
+			gcp->config |= CPC_GENERICCONF_LISTEN_ONLY;
+		else
+			gcp->config &= ~CPC_GENERICCONF_LISTEN_ONLY;
+
+		if (dev->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
+			gcp->config |= CPC_GENERICCONF_SINGLE_SHOT;
+		else
+			gcp->config &= ~CPC_GENERICCONF_SINGLE_SHOT;
+	} else {
+		return -EINVAL;
+	}
+
+	return ems_usb_command_msg(dev, &dev->active_params);
+}
+
 /* Send a CPC_Control command to change behaviour when interface receives a CAN
  * message, bus error or CAN state changed notifications.
  */
@@ -1241,17 +1267,16 @@ static int ems_usb_probe(struct usb_interface *intf,
 	/* Select CPC-USB/ARM7 or CPC-USB/FD */
 	switch (dev->udev->descriptor.idProduct) {
 	case USB_CPCUSB_ARM7_PRODUCT_ID:
-
-	dev->can.clock.freq = EMS_USB_ARM7_CLOCK;
-	dev->can.bittiming_const = &ems_usb_bittiming_const_arm7;
-	dev->can.do_set_bittiming = ems_usb_set_bittiming_arm7;
-	dev->can.do_set_mode = ems_usb_set_mode;
-	dev->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES |
+		dev->can.clock.freq = EMS_USB_ARM7_CLOCK;
+		dev->can.bittiming_const = &ems_usb_bittiming_const_arm7;
+		dev->can.do_set_bittiming = ems_usb_set_bittiming_arm7;
+		dev->can.do_set_mode = ems_usb_set_mode;
+		dev->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES |
 				      CAN_CTRLMODE_LISTENONLY;
-	init_params_sja1000(&dev->active_params);
-	dev->ems_usb_write_mode = ems_usb_write_mode_arm7;
-	dev->bulk_rd_buf_size = 64;
-	break;
+		init_params_sja1000(&dev->active_params);
+		dev->ems_usb_write_mode = ems_usb_write_mode_arm7;
+		dev->bulk_rd_buf_size = 64;
+		break;
 
 	case USB_CPCUSB_FD_PRODUCT_ID:
 		dev->can.clock.freq = EMS_USB_FD_CLOCK;
@@ -1259,14 +1284,16 @@ static int ems_usb_probe(struct usb_interface *intf,
 		dev->can.data_bittiming_const = &ems_usb_bittiming_const_generic_data;
 		dev->can.do_set_bittiming = ems_usb_set_bittiming_generic;
 		dev->can.do_set_data_bittiming = ems_usb_set_bittiming_generic_data;
+		dev->can.do_set_mode = ems_usb_set_mode;
 		dev->can.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
 				CAN_CTRLMODE_ONE_SHOT |
 				CAN_CTRLMODE_BERR_REPORTING |
 				CAN_CTRLMODE_FD |
 				CAN_CTRLMODE_FD_NON_ISO;
 		init_params_generic(&dev->active_params);
+		dev->ems_usb_write_mode = ems_usb_write_mode_fd;
 		dev->bulk_rd_buf_size = 512;
-	break;
+		break;
 
 	default:
 		err = -ENODEV;
-- 
2.26.2

-- 
EMS Dr. Thomas Wuensche e.K.
Sonnenhang 3
85304 Ilmmuenster
HR Ingolstadt, HRA 170106

Phone: +49-8441-490260
Fax  : +49-8441-81860
http://www.ems-wuensche.com

Re: [PATCH 11/17] can: ems_usb: Added ems_usb_write_mode_fd and its call in device probe routine. Fixed indentation.

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 3919 bytes --]

As mentioned in the other patch, please fix the indention where you introduce
the switch-case

On 11/6/20 6:02 PM, Gerhard Uttenthaler wrote:
> Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
> ---
>  drivers/net/can/usb/ems_usb.c | 49 +++++++++++++++++++++++++++--------
>  1 file changed, 38 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
> index a4d9a1b2d2f0..b51a5eb65946 100644
> --- a/drivers/net/can/usb/ems_usb.c
> +++ b/drivers/net/can/usb/ems_usb.c
> @@ -683,6 +683,32 @@ static int ems_usb_write_mode_arm7(struct ems_usb *dev, u32 mode)
>  	return ems_usb_command_msg(dev, &dev->active_params);
>  }
>  
> +static int ems_usb_write_mode_fd(struct ems_usb *dev, u32 mode)
> +{
> +	struct cpc_generic_can_params *gcp =
> +		&dev->active_params.msg.can_params.cc_params.generic;
> +
> +	if (mode == CPC_USB_RESET_MODE) {
> +		gcp->config |= CPC_GENERICCONF_RESET_MODE;
> +	} else if (mode == CPC_USB_RUN_MODE) {
> +		gcp->config &= ~CPC_GENERICCONF_RESET_MODE;
> +
> +		if (dev->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
> +			gcp->config |= CPC_GENERICCONF_LISTEN_ONLY;
> +		else
> +			gcp->config &= ~CPC_GENERICCONF_LISTEN_ONLY;
> +
> +		if (dev->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
> +			gcp->config |= CPC_GENERICCONF_SINGLE_SHOT;
> +		else
> +			gcp->config &= ~CPC_GENERICCONF_SINGLE_SHOT;
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	return ems_usb_command_msg(dev, &dev->active_params);
> +}
> +
>  /* Send a CPC_Control command to change behaviour when interface receives a CAN
>   * message, bus error or CAN state changed notifications.
>   */
> @@ -1241,17 +1267,16 @@ static int ems_usb_probe(struct usb_interface *intf,
>  	/* Select CPC-USB/ARM7 or CPC-USB/FD */
>  	switch (dev->udev->descriptor.idProduct) {
>  	case USB_CPCUSB_ARM7_PRODUCT_ID:
> -
> -	dev->can.clock.freq = EMS_USB_ARM7_CLOCK;
> -	dev->can.bittiming_const = &ems_usb_bittiming_const_arm7;
> -	dev->can.do_set_bittiming = ems_usb_set_bittiming_arm7;
> -	dev->can.do_set_mode = ems_usb_set_mode;
> -	dev->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES |
> +		dev->can.clock.freq = EMS_USB_ARM7_CLOCK;
> +		dev->can.bittiming_const = &ems_usb_bittiming_const_arm7;
> +		dev->can.do_set_bittiming = ems_usb_set_bittiming_arm7;
> +		dev->can.do_set_mode = ems_usb_set_mode;
> +		dev->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES |
>  				      CAN_CTRLMODE_LISTENONLY;
> -	init_params_sja1000(&dev->active_params);
> -	dev->ems_usb_write_mode = ems_usb_write_mode_arm7;
> -	dev->bulk_rd_buf_size = 64;
> -	break;
> +		init_params_sja1000(&dev->active_params);
> +		dev->ems_usb_write_mode = ems_usb_write_mode_arm7;
> +		dev->bulk_rd_buf_size = 64;
> +		break;
>  
>  	case USB_CPCUSB_FD_PRODUCT_ID:
>  		dev->can.clock.freq = EMS_USB_FD_CLOCK;
> @@ -1259,14 +1284,16 @@ static int ems_usb_probe(struct usb_interface *intf,
>  		dev->can.data_bittiming_const = &ems_usb_bittiming_const_generic_data;
>  		dev->can.do_set_bittiming = ems_usb_set_bittiming_generic;
>  		dev->can.do_set_data_bittiming = ems_usb_set_bittiming_generic_data;
> +		dev->can.do_set_mode = ems_usb_set_mode;
>  		dev->can.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
>  				CAN_CTRLMODE_ONE_SHOT |
>  				CAN_CTRLMODE_BERR_REPORTING |
>  				CAN_CTRLMODE_FD |
>  				CAN_CTRLMODE_FD_NON_ISO;
>  		init_params_generic(&dev->active_params);
> +		dev->ems_usb_write_mode = ems_usb_write_mode_fd;
>  		dev->bulk_rd_buf_size = 512;
> -	break;
> +		break;
>  
>  	default:
>  		err = -ENODEV;
> 

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 12/17] can: ems_usb: In ems_usb_start_xmit send only bytes with valid content to interface and not the complete buffer. Set first four bytes of buffer to 0. Wrapped long lines.

[Gerhard Uttenthaler]

Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
---
 drivers/net/can/usb/ems_usb.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
index b51a5eb65946..c464d644c833 100644
--- a/drivers/net/can/usb/ems_usb.c
+++ b/drivers/net/can/usb/ems_usb.c
@@ -902,25 +902,37 @@ static netdev_tx_t ems_usb_start_xmit(struct sk_buff *skb, struct net_device *ne
 	struct can_frame *cf = (struct can_frame *)skb->data;
 	struct ems_cpc_msg *msg;
 	struct urb *urb;
-	u8 *buf;
 	int i, err;
-	size_t size = CPC_HEADER_SIZE + CPC_MSG_HEADER_LEN
-			+ sizeof(struct cpc_can_msg);
+
+	u8 *buf;
+	size_t buf_size;
+	size_t buf_len = CPC_HEADER_SIZE + CPC_MSG_HEADER_LEN;
 
 	if (can_dropped_invalid_skb(netdev, skb))
 		return NETDEV_TX_OK;
 
-	/* create a URB, and a buffer for it, and copy the data to the URB */
+	buf_size = CPC_HEADER_SIZE +
+		   CPC_MSG_HEADER_LEN +
+		   sizeof(struct cpc_can_msg);
+
+	/* Create an URB, and a buffer for it
+	 * and copy the data to the URB
+	 */
 	urb = usb_alloc_urb(0, GFP_ATOMIC);
 	if (!urb)
 		goto nomem;
 
-	buf = usb_alloc_coherent(dev->udev, size, GFP_ATOMIC, &urb->transfer_dma);
+	buf = usb_alloc_coherent(dev->udev,
+				 buf_size,
+				 GFP_ATOMIC,
+				 &urb->transfer_dma);
 	if (!buf) {
 		netdev_err(netdev, "No memory left for USB buffer\n");
 		usb_free_urb(urb);
 		goto nomem;
 	}
+	// Clear first 4 bytes
+	*(u32 *)buf = 0;
 
 	msg = (struct ems_cpc_msg *)&buf[CPC_HEADER_SIZE];
 
@@ -942,6 +954,9 @@ static netdev_tx_t ems_usb_start_xmit(struct sk_buff *skb, struct net_device *ne
 		msg->length = CPC_CAN_MSG_MIN_SIZE + cf->can_dlc;
 	}
 
+	// Send only significant bytes of buffer
+	buf_len += msg->length;
+
 	for (i = 0; i < MAX_TX_URBS; i++) {
 		if (dev->tx_contexts[i].echo_index == MAX_TX_URBS) {
 			context = &dev->tx_contexts[i];
@@ -953,7 +968,7 @@ static netdev_tx_t ems_usb_start_xmit(struct sk_buff *skb, struct net_device *ne
 	 * allowed (MAX_TX_URBS).
 	 */
 	if (!context) {
-		usb_free_coherent(dev->udev, size, buf, urb->transfer_dma);
+		usb_free_coherent(dev->udev, buf_size, buf, urb->transfer_dma);
 		usb_free_urb(urb);
 
 		netdev_warn(netdev, "couldn't find free context\n");
@@ -966,7 +981,7 @@ static netdev_tx_t ems_usb_start_xmit(struct sk_buff *skb, struct net_device *ne
 	context->dlc = cf->can_dlc;
 
 	usb_fill_bulk_urb(urb, dev->udev, usb_sndbulkpipe(dev->udev, 2), buf,
-			  size, ems_usb_write_bulk_callback, context);
+			  buf_len, ems_usb_write_bulk_callback, context);
 	urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
 	usb_anchor_urb(urb, &dev->tx_submitted);
 
@@ -979,7 +994,7 @@ static netdev_tx_t ems_usb_start_xmit(struct sk_buff *skb, struct net_device *ne
 		can_free_echo_skb(netdev, context->echo_index);
 
 		usb_unanchor_urb(urb);
-		usb_free_coherent(dev->udev, size, buf, urb->transfer_dma);
+		usb_free_coherent(dev->udev, buf_size, buf, urb->transfer_dma);
 		dev_kfree_skb(skb);
 
 		atomic_dec(&dev->active_tx_urbs);
-- 
2.26.2

-- 
EMS Dr. Thomas Wuensche e.K.
Sonnenhang 3
85304 Ilmmuenster
HR Ingolstadt, HRA 170106

Phone: +49-8441-490260
Fax  : +49-8441-81860
http://www.ems-wuensche.com

Re: [PATCH 12/17] can: ems_usb: In ems_usb_start_xmit send only bytes with valid content to interface and not the complete buffer. Set first four bytes of buffer to 0. Wrapped long lines.

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 3771 bytes --]

On 11/6/20 6:02 PM, Gerhard Uttenthaler wrote:
> Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
> ---
>  drivers/net/can/usb/ems_usb.c | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
> index b51a5eb65946..c464d644c833 100644
> --- a/drivers/net/can/usb/ems_usb.c
> +++ b/drivers/net/can/usb/ems_usb.c
> @@ -902,25 +902,37 @@ static netdev_tx_t ems_usb_start_xmit(struct sk_buff *skb, struct net_device *ne
>  	struct can_frame *cf = (struct can_frame *)skb->data;
>  	struct ems_cpc_msg *msg;
>  	struct urb *urb;
> -	u8 *buf;
>  	int i, err;
> -	size_t size = CPC_HEADER_SIZE + CPC_MSG_HEADER_LEN
> -			+ sizeof(struct cpc_can_msg);
> +
> +	u8 *buf;
> +	size_t buf_size;
> +	size_t buf_len = CPC_HEADER_SIZE + CPC_MSG_HEADER_LEN;
>  
>  	if (can_dropped_invalid_skb(netdev, skb))
>  		return NETDEV_TX_OK;
>  
> -	/* create a URB, and a buffer for it, and copy the data to the URB */
> +	buf_size = CPC_HEADER_SIZE +
> +		   CPC_MSG_HEADER_LEN +
> +		   sizeof(struct cpc_can_msg);

does it make sense to only allocate the length of the buffer needed to hold the
data?

> +
> +	/* Create an URB, and a buffer for it
> +	 * and copy the data to the URB
> +	 */
>  	urb = usb_alloc_urb(0, GFP_ATOMIC);
>  	if (!urb)
>  		goto nomem;
>  
> -	buf = usb_alloc_coherent(dev->udev, size, GFP_ATOMIC, &urb->transfer_dma);
> +	buf = usb_alloc_coherent(dev->udev,
> +				 buf_size,
> +				 GFP_ATOMIC,
> +				 &urb->transfer_dma);
>  	if (!buf) {
>  		netdev_err(netdev, "No memory left for USB buffer\n");
>  		usb_free_urb(urb);
>  		goto nomem;
>  	}
> +	// Clear first 4 bytes

no C++ comments

> +	*(u32 *)buf = 0;
>  
>  	msg = (struct ems_cpc_msg *)&buf[CPC_HEADER_SIZE];
>  
> @@ -942,6 +954,9 @@ static netdev_tx_t ems_usb_start_xmit(struct sk_buff *skb, struct net_device *ne
>  		msg->length = CPC_CAN_MSG_MIN_SIZE + cf->can_dlc;
>  	}
>  
> +	// Send only significant bytes of buffer
> +	buf_len += msg->length;
> +
>  	for (i = 0; i < MAX_TX_URBS; i++) {
>  		if (dev->tx_contexts[i].echo_index == MAX_TX_URBS) {
>  			context = &dev->tx_contexts[i];
> @@ -953,7 +968,7 @@ static netdev_tx_t ems_usb_start_xmit(struct sk_buff *skb, struct net_device *ne
>  	 * allowed (MAX_TX_URBS).
>  	 */
>  	if (!context) {
> -		usb_free_coherent(dev->udev, size, buf, urb->transfer_dma);
> +		usb_free_coherent(dev->udev, buf_size, buf, urb->transfer_dma);
>  		usb_free_urb(urb);
>  
>  		netdev_warn(netdev, "couldn't find free context\n");
> @@ -966,7 +981,7 @@ static netdev_tx_t ems_usb_start_xmit(struct sk_buff *skb, struct net_device *ne
>  	context->dlc = cf->can_dlc;
>  
>  	usb_fill_bulk_urb(urb, dev->udev, usb_sndbulkpipe(dev->udev, 2), buf,
> -			  size, ems_usb_write_bulk_callback, context);
> +			  buf_len, ems_usb_write_bulk_callback, context);
>  	urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
>  	usb_anchor_urb(urb, &dev->tx_submitted);
>  
> @@ -979,7 +994,7 @@ static netdev_tx_t ems_usb_start_xmit(struct sk_buff *skb, struct net_device *ne
>  		can_free_echo_skb(netdev, context->echo_index);
>  
>  		usb_unanchor_urb(urb);
> -		usb_free_coherent(dev->udev, size, buf, urb->transfer_dma);
> +		usb_free_coherent(dev->udev, buf_size, buf, urb->transfer_dma);
>  		dev_kfree_skb(skb);
>  
>  		atomic_dec(&dev->active_tx_urbs);
> 

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 12/17] can: ems_usb: In ems_usb_start_xmit send only bytes with valid content to interface and not the complete buffer. Set first four bytes of buffer to 0. Wrapped long lines.

[Gerhard Uttenthaler]

Am 06.11.20 um 18:58 schrieb Marc Kleine-Budde:
> On 11/6/20 6:02 PM, Gerhard Uttenthaler wrote:
>> Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
>> ---
>>  drivers/net/can/usb/ems_usb.c | 31 +++++++++++++++++++++++--------
>>  1 file changed, 23 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
>> index b51a5eb65946..c464d644c833 100644
>> --- a/drivers/net/can/usb/ems_usb.c
>> +++ b/drivers/net/can/usb/ems_usb.c
>> @@ -902,25 +902,37 @@ static netdev_tx_t ems_usb_start_xmit(struct sk_buff *skb, struct net_device *ne
>>  	struct can_frame *cf = (struct can_frame *)skb->data;
>>  	struct ems_cpc_msg *msg;
>>  	struct urb *urb;
>> -	u8 *buf;
>>  	int i, err;
>> -	size_t size = CPC_HEADER_SIZE + CPC_MSG_HEADER_LEN
>> -			+ sizeof(struct cpc_can_msg);
>> +
>> +	u8 *buf;
>> +	size_t buf_size;
>> +	size_t buf_len = CPC_HEADER_SIZE + CPC_MSG_HEADER_LEN;
>>  
>>  	if (can_dropped_invalid_skb(netdev, skb))
>>  		return NETDEV_TX_OK;
>>  
>> -	/* create a URB, and a buffer for it, and copy the data to the URB */
>> +	buf_size = CPC_HEADER_SIZE +
>> +		   CPC_MSG_HEADER_LEN +
>> +		   sizeof(struct cpc_can_msg);
> 
> does it make sense to only allocate the length of the buffer needed to hold the
> data?I have considered that, but actual length depends on DLC, however not
when the frame is an RTR and this is different for Classic and FD. I
think code will get more complicated with the benefit only to save a few
bytes.

-- 
Gerhard

-- 
EMS Dr. Thomas Wuensche e.K.
Sonnenhang 3
85304 Ilmmuenster
HR Ingolstadt, HRA 170106

Phone: +49-8441-490260
Fax  : +49-8441-81860
http://www.ems-wuensche.com

Re: [PATCH 12/17] can: ems_usb: In ems_usb_start_xmit send only bytes with valid content to interface and not the complete buffer. Set first four bytes of buffer to 0. Wrapped long lines.

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1932 bytes --]

On 11/12/20 9:31 AM, Gerhard Uttenthaler wrote:
> Am 06.11.20 um 18:58 schrieb Marc Kleine-Budde:
>> On 11/6/20 6:02 PM, Gerhard Uttenthaler wrote:
>>> Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
>>> ---
>>>  drivers/net/can/usb/ems_usb.c | 31 +++++++++++++++++++++++--------
>>>  1 file changed, 23 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
>>> index b51a5eb65946..c464d644c833 100644
>>> --- a/drivers/net/can/usb/ems_usb.c
>>> +++ b/drivers/net/can/usb/ems_usb.c
>>> @@ -902,25 +902,37 @@ static netdev_tx_t ems_usb_start_xmit(struct sk_buff *skb, struct net_device *ne
>>>  	struct can_frame *cf = (struct can_frame *)skb->data;
>>>  	struct ems_cpc_msg *msg;
>>>  	struct urb *urb;
>>> -	u8 *buf;
>>>  	int i, err;
>>> -	size_t size = CPC_HEADER_SIZE + CPC_MSG_HEADER_LEN
>>> -			+ sizeof(struct cpc_can_msg);
>>> +
>>> +	u8 *buf;
>>> +	size_t buf_size;
>>> +	size_t buf_len = CPC_HEADER_SIZE + CPC_MSG_HEADER_LEN;
>>>  
>>>  	if (can_dropped_invalid_skb(netdev, skb))
>>>  		return NETDEV_TX_OK;
>>>  
>>> -	/* create a URB, and a buffer for it, and copy the data to the URB */
>>> +	buf_size = CPC_HEADER_SIZE +
>>> +		   CPC_MSG_HEADER_LEN +
>>> +		   sizeof(struct cpc_can_msg);
>>
>> does it make sense to only allocate the length of the buffer needed to hold the
>> data?
>
> I have considered that, but actual length depends on DLC, however not
> when the frame is an RTR and this is different for Classic and FD. I
> think code will get more complicated with the benefit only to save a few
> bytes.

Ok

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 13/17] can: ems_usb: Rearrange code in ems_usb_start_xmit to check with can_is_canfd_skb for CAN FD frames.

[Gerhard Uttenthaler]

Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
---
 drivers/net/can/usb/ems_usb.c | 87 +++++++++++++++++++----------------
 1 file changed, 47 insertions(+), 40 deletions(-)

diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
index c464d644c833..c3159ffaa4fa 100644
--- a/drivers/net/can/usb/ems_usb.c
+++ b/drivers/net/can/usb/ems_usb.c
@@ -899,64 +899,71 @@ static netdev_tx_t ems_usb_start_xmit(struct sk_buff *skb, struct net_device *ne
 	struct ems_usb *dev = netdev_priv(netdev);
 	struct ems_tx_urb_context *context = NULL;
 	struct net_device_stats *stats = &netdev->stats;
-	struct can_frame *cf = (struct can_frame *)skb->data;
 	struct ems_cpc_msg *msg;
 	struct urb *urb;
 	int i, err;
+	u8 dlc;
 
 	u8 *buf;
 	size_t buf_size;
 	size_t buf_len = CPC_HEADER_SIZE + CPC_MSG_HEADER_LEN;
 
-	if (can_dropped_invalid_skb(netdev, skb))
+	if (can_is_canfd_skb(skb)) {
+		// Placeholder for next patch
 		return NETDEV_TX_OK;
+	} else {
+		struct can_frame *cf = (struct can_frame *)skb->data;
 
-	buf_size = CPC_HEADER_SIZE +
-		   CPC_MSG_HEADER_LEN +
-		   sizeof(struct cpc_can_msg);
+		if (can_dropped_invalid_skb(netdev, skb))
+			return NETDEV_TX_OK;
 
-	/* Create an URB, and a buffer for it
-	 * and copy the data to the URB
-	 */
-	urb = usb_alloc_urb(0, GFP_ATOMIC);
-	if (!urb)
-		goto nomem;
-
-	buf = usb_alloc_coherent(dev->udev,
-				 buf_size,
-				 GFP_ATOMIC,
-				 &urb->transfer_dma);
-	if (!buf) {
-		netdev_err(netdev, "No memory left for USB buffer\n");
-		usb_free_urb(urb);
-		goto nomem;
-	}
-	// Clear first 4 bytes
-	*(u32 *)buf = 0;
+		buf_size = CPC_HEADER_SIZE +
+			   CPC_MSG_HEADER_LEN +
+			   sizeof(struct cpc_can_msg);
 
-	msg = (struct ems_cpc_msg *)&buf[CPC_HEADER_SIZE];
+		/* Create an URB, and a buffer for it
+		 * and copy the data to the URB
+		 */
+		urb = usb_alloc_urb(0, GFP_ATOMIC);
+		if (!urb)
+			goto nomem;
+
+		buf = usb_alloc_coherent(dev->udev,
+					 buf_size,
+					 GFP_ATOMIC,
+					 &urb->transfer_dma);
+		if (!buf) {
+			netdev_err(netdev, "No memory left for USB buffer\n");
+			usb_free_urb(urb);
+			goto nomem;
+		}
+		// Clear first 4 bytes
+		*(u32 *)buf = 0;
 
-	msg->msg.can_msg.id = cpu_to_le32(cf->can_id & CAN_ERR_MASK);
-	msg->msg.can_msg.length = cf->can_dlc;
+		msg = (struct ems_cpc_msg *)&buf[CPC_HEADER_SIZE];
 
-	if (cf->can_id & CAN_RTR_FLAG) {
-		msg->type = cf->can_id & CAN_EFF_FLAG ?
-			CPC_CMD_TYPE_EXT_RTR_FRAME : CPC_CMD_TYPE_RTR_FRAME;
+		msg->msg.can_msg.id = cpu_to_le32(cf->can_id & CAN_ERR_MASK);
+		dlc = cf->can_dlc;
+		msg->msg.can_msg.length = dlc;
 
-		msg->length = CPC_CAN_MSG_MIN_SIZE;
-	} else {
-		msg->type = cf->can_id & CAN_EFF_FLAG ?
-			CPC_CMD_TYPE_EXT_CAN_FRAME : CPC_CMD_TYPE_CAN_FRAME;
+		if (cf->can_id & CAN_RTR_FLAG) {
+			msg->type = cf->can_id & CAN_EFF_FLAG ?
+				CPC_CMD_TYPE_EXT_RTR_FRAME : CPC_CMD_TYPE_RTR_FRAME;
 
-		for (i = 0; i < cf->can_dlc; i++)
-			msg->msg.can_msg.msg[i] = cf->data[i];
+			msg->length = CPC_CAN_MSG_MIN_SIZE;
+		} else {
+			msg->type = cf->can_id & CAN_EFF_FLAG ?
+				CPC_CMD_TYPE_EXT_CAN_FRAME : CPC_CMD_TYPE_CAN_FRAME;
 
-		msg->length = CPC_CAN_MSG_MIN_SIZE + cf->can_dlc;
-	}
+			for (i = 0; i < cf->can_dlc; i++)
+				msg->msg.can_msg.msg[i] = cf->data[i];
 
-	// Send only significant bytes of buffer
-	buf_len += msg->length;
+			msg->length = CPC_CAN_MSG_MIN_SIZE + cf->can_dlc;
+		}
 
+		// Send only significant bytes of buffer
+		buf_len += msg->length;
+	}
 	for (i = 0; i < MAX_TX_URBS; i++) {
 		if (dev->tx_contexts[i].echo_index == MAX_TX_URBS) {
 			context = &dev->tx_contexts[i];
@@ -978,7 +985,7 @@ static netdev_tx_t ems_usb_start_xmit(struct sk_buff *skb, struct net_device *ne
 
 	context->dev = dev;
 	context->echo_index = i;
-	context->dlc = cf->can_dlc;
+	context->dlc = dlc;
 
 	usb_fill_bulk_urb(urb, dev->udev, usb_sndbulkpipe(dev->udev, 2), buf,
 			  buf_len, ems_usb_write_bulk_callback, context);
-- 
2.26.2

-- 
EMS Dr. Thomas Wuensche e.K.
Sonnenhang 3
85304 Ilmmuenster
HR Ingolstadt, HRA 170106

Phone: +49-8441-490260
Fax  : +49-8441-81860
http://www.ems-wuensche.com

Re: [PATCH 13/17] can: ems_usb: Rearrange code in ems_usb_start_xmit to check with can_is_canfd_skb for CAN FD frames.

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 4752 bytes --]

On 11/6/20 6:02 PM, Gerhard Uttenthaler wrote:
> Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
> ---
>  drivers/net/can/usb/ems_usb.c | 87 +++++++++++++++++++----------------
>  1 file changed, 47 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
> index c464d644c833..c3159ffaa4fa 100644
> --- a/drivers/net/can/usb/ems_usb.c
> +++ b/drivers/net/can/usb/ems_usb.c
> @@ -899,64 +899,71 @@ static netdev_tx_t ems_usb_start_xmit(struct sk_buff *skb, struct net_device *ne
>  	struct ems_usb *dev = netdev_priv(netdev);
>  	struct ems_tx_urb_context *context = NULL;
>  	struct net_device_stats *stats = &netdev->stats;
> -	struct can_frame *cf = (struct can_frame *)skb->data;
>  	struct ems_cpc_msg *msg;
>  	struct urb *urb;
>  	int i, err;
> +	u8 dlc;
>  
>  	u8 *buf;
>  	size_t buf_size;
>  	size_t buf_len = CPC_HEADER_SIZE + CPC_MSG_HEADER_LEN;
>  
> -	if (can_dropped_invalid_skb(netdev, skb))

why move this into the if? it's common code with fd and non fd

> +	if (can_is_canfd_skb(skb)) {
> +		// Placeholder for next patch

no placeholders please

>  		return NETDEV_TX_OK;
> +	} else {
> +		struct can_frame *cf = (struct can_frame *)skb->data;
>  
> -	buf_size = CPC_HEADER_SIZE +
> -		   CPC_MSG_HEADER_LEN +
> -		   sizeof(struct cpc_can_msg);
> +		if (can_dropped_invalid_skb(netdev, skb))
> +			return NETDEV_TX_OK;
>  
> -	/* Create an URB, and a buffer for it
> -	 * and copy the data to the URB
> -	 */
> -	urb = usb_alloc_urb(0, GFP_ATOMIC);
> -	if (!urb)
> -		goto nomem;
> -
> -	buf = usb_alloc_coherent(dev->udev,
> -				 buf_size,
> -				 GFP_ATOMIC,
> -				 &urb->transfer_dma);
> -	if (!buf) {
> -		netdev_err(netdev, "No memory left for USB buffer\n");
> -		usb_free_urb(urb);
> -		goto nomem;
> -	}
> -	// Clear first 4 bytes
> -	*(u32 *)buf = 0;
> +		buf_size = CPC_HEADER_SIZE +
> +			   CPC_MSG_HEADER_LEN +
> +			   sizeof(struct cpc_can_msg);
>  
> -	msg = (struct ems_cpc_msg *)&buf[CPC_HEADER_SIZE];
> +		/* Create an URB, and a buffer for it
> +		 * and copy the data to the URB
> +		 */
> +		urb = usb_alloc_urb(0, GFP_ATOMIC);
> +		if (!urb)
> +			goto nomem;
> +
> +		buf = usb_alloc_coherent(dev->udev,
> +					 buf_size,
> +					 GFP_ATOMIC,
> +					 &urb->transfer_dma);
> +		if (!buf) {
> +			netdev_err(netdev, "No memory left for USB buffer\n");
> +			usb_free_urb(urb);
> +			goto nomem;
> +		}
> +		// Clear first 4 bytes
> +		*(u32 *)buf = 0;
>  
> -	msg->msg.can_msg.id = cpu_to_le32(cf->can_id & CAN_ERR_MASK);
> -	msg->msg.can_msg.length = cf->can_dlc;
> +		msg = (struct ems_cpc_msg *)&buf[CPC_HEADER_SIZE];
>  
> -	if (cf->can_id & CAN_RTR_FLAG) {
> -		msg->type = cf->can_id & CAN_EFF_FLAG ?
> -			CPC_CMD_TYPE_EXT_RTR_FRAME : CPC_CMD_TYPE_RTR_FRAME;
> +		msg->msg.can_msg.id = cpu_to_le32(cf->can_id & CAN_ERR_MASK);
> +		dlc = cf->can_dlc;
> +		msg->msg.can_msg.length = dlc;
>  
> -		msg->length = CPC_CAN_MSG_MIN_SIZE;
> -	} else {
> -		msg->type = cf->can_id & CAN_EFF_FLAG ?
> -			CPC_CMD_TYPE_EXT_CAN_FRAME : CPC_CMD_TYPE_CAN_FRAME;
> +		if (cf->can_id & CAN_RTR_FLAG) {
> +			msg->type = cf->can_id & CAN_EFF_FLAG ?
> +				CPC_CMD_TYPE_EXT_RTR_FRAME : CPC_CMD_TYPE_RTR_FRAME;
>  
> -		for (i = 0; i < cf->can_dlc; i++)
> -			msg->msg.can_msg.msg[i] = cf->data[i];
> +			msg->length = CPC_CAN_MSG_MIN_SIZE;
> +		} else {
> +			msg->type = cf->can_id & CAN_EFF_FLAG ?
> +				CPC_CMD_TYPE_EXT_CAN_FRAME : CPC_CMD_TYPE_CAN_FRAME;
>  
> -		msg->length = CPC_CAN_MSG_MIN_SIZE + cf->can_dlc;
> -	}
> +			for (i = 0; i < cf->can_dlc; i++)
> +				msg->msg.can_msg.msg[i] = cf->data[i];
>  
> -	// Send only significant bytes of buffer
> -	buf_len += msg->length;
> +			msg->length = CPC_CAN_MSG_MIN_SIZE + cf->can_dlc;
> +		}
>  
> +		// Send only significant bytes of buffer
> +		buf_len += msg->length;
> +	}
>  	for (i = 0; i < MAX_TX_URBS; i++) {
>  		if (dev->tx_contexts[i].echo_index == MAX_TX_URBS) {
>  			context = &dev->tx_contexts[i];
> @@ -978,7 +985,7 @@ static netdev_tx_t ems_usb_start_xmit(struct sk_buff *skb, struct net_device *ne
>  
>  	context->dev = dev;
>  	context->echo_index = i;
> -	context->dlc = cf->can_dlc;
> +	context->dlc = dlc;
>  
>  	usb_fill_bulk_urb(urb, dev->udev, usb_sndbulkpipe(dev->udev, 2), buf,
>  			  buf_len, ems_usb_write_bulk_callback, context);
> 

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 14/17] can: ems_usb: Added code to handle CAN FD frames in ems_usb_start_xmit

[Gerhard Uttenthaler]

Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
---
 drivers/net/can/usb/ems_usb.c | 61 ++++++++++++++++++++++++++++++++---
 1 file changed, 57 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
index c3159ffaa4fa..c36f02eeec85 100644
--- a/drivers/net/can/usb/ems_usb.c
+++ b/drivers/net/can/usb/ems_usb.c
@@ -73,8 +73,9 @@ MODULE_LICENSE("GPL v2");
 #define CPC_OVR_HW 0x80
 
 /* Size of the "struct ems_cpc_msg" without the union */
-#define CPC_MSG_HEADER_LEN   11
-#define CPC_CAN_MSG_MIN_SIZE 5
+#define CPC_MSG_HEADER_LEN     11
+#define CPC_CAN_MSG_MIN_SIZE    5
+#define CPC_CANFD_MSG_MIN_SIZE  6
 
 /* Define these values to match your devices */
 #define USB_CPCUSB_VENDOR_ID 0x12D6
@@ -909,8 +910,60 @@ static netdev_tx_t ems_usb_start_xmit(struct sk_buff *skb, struct net_device *ne
 	size_t buf_len = CPC_HEADER_SIZE + CPC_MSG_HEADER_LEN;
 
 	if (can_is_canfd_skb(skb)) {
-		// Placeholder for next patch
-		return NETDEV_TX_OK;
+		struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
+		struct cpc_canfd_msg *fd_msg;
+
+		if (can_dropped_invalid_skb(netdev, skb))
+			return NETDEV_TX_OK;
+
+		buf_size = CPC_HEADER_SIZE +
+			   CPC_MSG_HEADER_LEN +
+			   sizeof(struct cpc_canfd_msg);
+
+		/* Create an URB and a buffer big enough for
+		 * all message lengths, copy the data to the URB
+		 */
+		urb = usb_alloc_urb(0, GFP_ATOMIC);
+		if (!urb)
+			goto nomem;
+
+		buf = usb_alloc_coherent(dev->udev,
+					 buf_size,
+					 GFP_ATOMIC,
+					 &urb->transfer_dma);
+		if (!buf) {
+			netdev_err(netdev, "No memory left for USB buffer\n");
+			usb_free_urb(urb);
+			goto nomem;
+		}
+		// Clear first 4 bytes
+		*(u32 *)buf = 0;
+
+		msg = (struct ems_cpc_msg *)&buf[CPC_HEADER_SIZE];
+		fd_msg = &msg->msg.canfd_msg;
+
+		msg->type = CPC_CMD_TYPE_CANFD_FRAME;
+
+		fd_msg->id = cpu_to_le32(cfd->can_id & CAN_ERR_MASK);
+		dlc = cfd->len;
+		fd_msg->length = dlc;
+		fd_msg->flags = 0;
+
+		if (cfd->can_id & CAN_EFF_FLAG)
+			fd_msg->flags |= CPC_FDFLAG_XTD;
+
+		if (cfd->flags & CANFD_BRS)
+			fd_msg->flags |= CPC_FDFLAG_BRS;
+
+		if (cfd->flags & CANFD_ESI)
+			fd_msg->flags |= CPC_FDFLAG_ESI;
+
+		for (i = 0; i < cfd->len; i++)
+			fd_msg->msg[i] = cfd->data[i];
+
+		msg->length = CPC_CANFD_MSG_MIN_SIZE + cfd->len;
+		// Send only significant bytes of buffer
+		buf_len += msg->length;
 	} else {
 		struct can_frame *cf = (struct can_frame *)skb->data;
 
-- 
2.26.2

-- 
EMS Dr. Thomas Wuensche e.K.
Sonnenhang 3
85304 Ilmmuenster
HR Ingolstadt, HRA 170106

Phone: +49-8441-490260
Fax  : +49-8441-81860
http://www.ems-wuensche.com

Re: [PATCH 14/17] can: ems_usb: Added code to handle CAN FD frames in ems_usb_start_xmit

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 3259 bytes --]

On 11/6/20 6:02 PM, Gerhard Uttenthaler wrote:
> Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
> ---
>  drivers/net/can/usb/ems_usb.c | 61 ++++++++++++++++++++++++++++++++---
>  1 file changed, 57 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
> index c3159ffaa4fa..c36f02eeec85 100644
> --- a/drivers/net/can/usb/ems_usb.c
> +++ b/drivers/net/can/usb/ems_usb.c
> @@ -73,8 +73,9 @@ MODULE_LICENSE("GPL v2");
>  #define CPC_OVR_HW 0x80
>  
>  /* Size of the "struct ems_cpc_msg" without the union */
> -#define CPC_MSG_HEADER_LEN   11
> -#define CPC_CAN_MSG_MIN_SIZE 5
> +#define CPC_MSG_HEADER_LEN     11
> +#define CPC_CAN_MSG_MIN_SIZE    5
> +#define CPC_CANFD_MSG_MIN_SIZE  6

Don't do this, just use one space.

>  
>  /* Define these values to match your devices */
>  #define USB_CPCUSB_VENDOR_ID 0x12D6
> @@ -909,8 +910,60 @@ static netdev_tx_t ems_usb_start_xmit(struct sk_buff *skb, struct net_device *ne
>  	size_t buf_len = CPC_HEADER_SIZE + CPC_MSG_HEADER_LEN;
>  
>  	if (can_is_canfd_skb(skb)) {
> -		// Placeholder for next patch
> -		return NETDEV_TX_OK;
> +		struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> +		struct cpc_canfd_msg *fd_msg;
> +
> +		if (can_dropped_invalid_skb(netdev, skb))
> +			return NETDEV_TX_OK;
> +
> +		buf_size = CPC_HEADER_SIZE +
> +			   CPC_MSG_HEADER_LEN +
> +			   sizeof(struct cpc_canfd_msg);
> +
> +		/* Create an URB and a buffer big enough for
> +		 * all message lengths, copy the data to the URB
> +		 */
> +		urb = usb_alloc_urb(0, GFP_ATOMIC);
> +		if (!urb)
> +			goto nomem;
> +
> +		buf = usb_alloc_coherent(dev->udev,
> +					 buf_size,
> +					 GFP_ATOMIC,
> +					 &urb->transfer_dma);
> +		if (!buf) {
> +			netdev_err(netdev, "No memory left for USB buffer\n");
> +			usb_free_urb(urb);
> +			goto nomem;
> +		}
> +		// Clear first 4 bytes

not C++ momments

> +		*(u32 *)buf = 0;
> +
> +		msg = (struct ems_cpc_msg *)&buf[CPC_HEADER_SIZE];

What about creating/using a proper struct that holds the header and the cps_msg?
Then you don't need this pointer arithmetics.

> +		fd_msg = &msg->msg.canfd_msg;
> +
> +		msg->type = CPC_CMD_TYPE_CANFD_FRAME;
> +
> +		fd_msg->id = cpu_to_le32(cfd->can_id & CAN_ERR_MASK);
> +		dlc = cfd->len;
> +		fd_msg->length = dlc;
> +		fd_msg->flags = 0;
> +
> +		if (cfd->can_id & CAN_EFF_FLAG)
> +			fd_msg->flags |= CPC_FDFLAG_XTD;
> +
> +		if (cfd->flags & CANFD_BRS)
> +			fd_msg->flags |= CPC_FDFLAG_BRS;
> +
> +		if (cfd->flags & CANFD_ESI)
> +			fd_msg->flags |= CPC_FDFLAG_ESI;
> +
> +		for (i = 0; i < cfd->len; i++)
> +			fd_msg->msg[i] = cfd->data[i];

memcpy()

> +
> +		msg->length = CPC_CANFD_MSG_MIN_SIZE + cfd->len;
> +		// Send only significant bytes of buffer
> +		buf_len += msg->length;
>  	} else {
>  		struct can_frame *cf = (struct can_frame *)skb->data;
>  
> 

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 15/17] can: ems_usb: In CAN error handling routine checking which CAN controller type is issuing the error

[Gerhard Uttenthaler]

Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
---
 drivers/net/can/usb/ems_usb.c | 77 +++++++++++++++++++----------------
 1 file changed, 43 insertions(+), 34 deletions(-)

diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
index c36f02eeec85..4a67c57c4760 100644
--- a/drivers/net/can/usb/ems_usb.c
+++ b/drivers/net/can/usb/ems_usb.c
@@ -103,6 +103,9 @@ MODULE_LICENSE("GPL v2");
 
 #define SJA1000_DEFAULT_OUTPUT_CONTROL 0xDA
 
+#define SJA1000   2 // NXP basic CAN controller
+#define LPC546XX  5 // NXP CAN FD controller
+
 /* CPC-USB/ARM7 actually uses a 16MHz clock to generate the CAN clock
  * but it expects SJA1000 bit settings based on 8MHz (is internally
  * converted).
@@ -441,6 +444,9 @@ static void ems_usb_rx_err(struct ems_usb *dev, struct ems_cpc_msg *msg)
 	if (!skb)
 		return;
 
+	/* The CPC_MSG_TYPE_CAN_STATE works for both
+	 * CPC-USB/ARM7 and CPC-USB/FD
+	 */
 	if (msg->type == CPC_MSG_TYPE_CAN_STATE) {
 		u8 state = msg->msg.can_state;
 
@@ -458,40 +464,43 @@ static void ems_usb_rx_err(struct ems_usb *dev, struct ems_cpc_msg *msg)
 			dev->can.can_stats.error_passive++;
 		}
 	} else if (msg->type == CPC_MSG_TYPE_CAN_FRAME_ERROR) {
-		u8 ecc = msg->msg.error.cc.regs.sja1000.ecc;
-		u8 txerr = msg->msg.error.cc.regs.sja1000.txerr;
-		u8 rxerr = msg->msg.error.cc.regs.sja1000.rxerr;
-
-		/* bus error interrupt */
-		dev->can.can_stats.bus_error++;
-		stats->rx_errors++;
-
-		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
-
-		switch (ecc & SJA1000_ECC_MASK) {
-		case SJA1000_ECC_BIT:
-			cf->data[2] |= CAN_ERR_PROT_BIT;
-			break;
-		case SJA1000_ECC_FORM:
-			cf->data[2] |= CAN_ERR_PROT_FORM;
-			break;
-		case SJA1000_ECC_STUFF:
-			cf->data[2] |= CAN_ERR_PROT_STUFF;
-			break;
-		default:
-			cf->data[3] = ecc & SJA1000_ECC_SEG;
-			break;
-		}
-
-		/* Error occurred during transmission? */
-		if ((ecc & SJA1000_ECC_DIR) == 0)
-			cf->data[2] |= CAN_ERR_PROT_TX;
-
-		if (dev->can.state == CAN_STATE_ERROR_WARNING ||
-		    dev->can.state == CAN_STATE_ERROR_PASSIVE) {
-			cf->can_id |= CAN_ERR_CRTL;
-			cf->data[1] = (txerr > rxerr) ?
-			    CAN_ERR_CRTL_TX_PASSIVE : CAN_ERR_CRTL_RX_PASSIVE;
+		// CPC-USB/ARM7
+		if (msg->msg.error.cc.cc_type == SJA1000) {
+			u8 ecc = msg->msg.error.cc.regs.sja1000.ecc;
+			u8 txerr = msg->msg.error.cc.regs.sja1000.txerr;
+			u8 rxerr = msg->msg.error.cc.regs.sja1000.rxerr;
+
+			/* bus error interrupt */
+			dev->can.can_stats.bus_error++;
+			stats->rx_errors++;
+
+			cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
+
+			switch (ecc & SJA1000_ECC_MASK) {
+			case SJA1000_ECC_BIT:
+				cf->data[2] |= CAN_ERR_PROT_BIT;
+				break;
+			case SJA1000_ECC_FORM:
+				cf->data[2] |= CAN_ERR_PROT_FORM;
+				break;
+			case SJA1000_ECC_STUFF:
+				cf->data[2] |= CAN_ERR_PROT_STUFF;
+				break;
+			default:
+				cf->data[3] = ecc & SJA1000_ECC_SEG;
+				break;
+			}
+
+			/* Error occurred during transmission? */
+			if ((ecc & SJA1000_ECC_DIR) == 0)
+				cf->data[2] |= CAN_ERR_PROT_TX;
+
+			if (dev->can.state == CAN_STATE_ERROR_WARNING ||
+			    dev->can.state == CAN_STATE_ERROR_PASSIVE) {
+				cf->can_id |= CAN_ERR_CRTL;
+				cf->data[1] = (txerr > rxerr) ?
+				    CAN_ERR_CRTL_TX_PASSIVE : CAN_ERR_CRTL_RX_PASSIVE;
+			}
 		}
 	} else if (msg->type == CPC_MSG_TYPE_OVERRUN) {
 		cf->can_id |= CAN_ERR_CRTL;
-- 
2.26.2

-- 
EMS Dr. Thomas Wuensche e.K.
Sonnenhang 3
85304 Ilmmuenster
HR Ingolstadt, HRA 170106

Phone: +49-8441-490260
Fax  : +49-8441-81860
http://www.ems-wuensche.com

Re: [PATCH 15/17] can: ems_usb: In CAN error handling routine checking which CAN controller type is issuing the error

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 4082 bytes --]

On 11/6/20 6:02 PM, Gerhard Uttenthaler wrote:
> Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
> ---
>  drivers/net/can/usb/ems_usb.c | 77 +++++++++++++++++++----------------
>  1 file changed, 43 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
> index c36f02eeec85..4a67c57c4760 100644
> --- a/drivers/net/can/usb/ems_usb.c
> +++ b/drivers/net/can/usb/ems_usb.c
> @@ -103,6 +103,9 @@ MODULE_LICENSE("GPL v2");
>  
>  #define SJA1000_DEFAULT_OUTPUT_CONTROL 0xDA
>  
> +#define SJA1000   2 // NXP basic CAN controller
> +#define LPC546XX  5 // NXP CAN FD controller

please add a proper prefix to these definees

> +
>  /* CPC-USB/ARM7 actually uses a 16MHz clock to generate the CAN clock
>   * but it expects SJA1000 bit settings based on 8MHz (is internally
>   * converted).
> @@ -441,6 +444,9 @@ static void ems_usb_rx_err(struct ems_usb *dev, struct ems_cpc_msg *msg)
>  	if (!skb)
>  		return;
>  
> +	/* The CPC_MSG_TYPE_CAN_STATE works for both
> +	 * CPC-USB/ARM7 and CPC-USB/FD
> +	 */
>  	if (msg->type == CPC_MSG_TYPE_CAN_STATE) {
>  		u8 state = msg->msg.can_state;
>  
> @@ -458,40 +464,43 @@ static void ems_usb_rx_err(struct ems_usb *dev, struct ems_cpc_msg *msg)
>  			dev->can.can_stats.error_passive++;
>  		}
>  	} else if (msg->type == CPC_MSG_TYPE_CAN_FRAME_ERROR) {
> -		u8 ecc = msg->msg.error.cc.regs.sja1000.ecc;
> -		u8 txerr = msg->msg.error.cc.regs.sja1000.txerr;
> -		u8 rxerr = msg->msg.error.cc.regs.sja1000.rxerr;
> -
> -		/* bus error interrupt */
> -		dev->can.can_stats.bus_error++;
> -		stats->rx_errors++;
> -
> -		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> -
> -		switch (ecc & SJA1000_ECC_MASK) {
> -		case SJA1000_ECC_BIT:
> -			cf->data[2] |= CAN_ERR_PROT_BIT;
> -			break;
> -		case SJA1000_ECC_FORM:
> -			cf->data[2] |= CAN_ERR_PROT_FORM;
> -			break;
> -		case SJA1000_ECC_STUFF:
> -			cf->data[2] |= CAN_ERR_PROT_STUFF;
> -			break;
> -		default:
> -			cf->data[3] = ecc & SJA1000_ECC_SEG;
> -			break;
> -		}
> -
> -		/* Error occurred during transmission? */
> -		if ((ecc & SJA1000_ECC_DIR) == 0)
> -			cf->data[2] |= CAN_ERR_PROT_TX;
> -
> -		if (dev->can.state == CAN_STATE_ERROR_WARNING ||
> -		    dev->can.state == CAN_STATE_ERROR_PASSIVE) {
> -			cf->can_id |= CAN_ERR_CRTL;
> -			cf->data[1] = (txerr > rxerr) ?
> -			    CAN_ERR_CRTL_TX_PASSIVE : CAN_ERR_CRTL_RX_PASSIVE;
> +		// CPC-USB/ARM7

no c++ comments

> +		if (msg->msg.error.cc.cc_type == SJA1000) {
> +			u8 ecc = msg->msg.error.cc.regs.sja1000.ecc;
> +			u8 txerr = msg->msg.error.cc.regs.sja1000.txerr;
> +			u8 rxerr = msg->msg.error.cc.regs.sja1000.rxerr;
> +
> +			/* bus error interrupt */
> +			dev->can.can_stats.bus_error++;
> +			stats->rx_errors++;
> +
> +			cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> +
> +			switch (ecc & SJA1000_ECC_MASK) {
> +			case SJA1000_ECC_BIT:
> +				cf->data[2] |= CAN_ERR_PROT_BIT;
> +				break;
> +			case SJA1000_ECC_FORM:
> +				cf->data[2] |= CAN_ERR_PROT_FORM;
> +				break;
> +			case SJA1000_ECC_STUFF:
> +				cf->data[2] |= CAN_ERR_PROT_STUFF;
> +				break;
> +			default:
> +				cf->data[3] = ecc & SJA1000_ECC_SEG;
> +				break;
> +			}
> +
> +			/* Error occurred during transmission? */
> +			if ((ecc & SJA1000_ECC_DIR) == 0)
> +				cf->data[2] |= CAN_ERR_PROT_TX;
> +
> +			if (dev->can.state == CAN_STATE_ERROR_WARNING ||
> +			    dev->can.state == CAN_STATE_ERROR_PASSIVE) {
> +				cf->can_id |= CAN_ERR_CRTL;
> +				cf->data[1] = (txerr > rxerr) ?
> +				    CAN_ERR_CRTL_TX_PASSIVE : CAN_ERR_CRTL_RX_PASSIVE;
> +			}
>  		}
>  	} else if (msg->type == CPC_MSG_TYPE_OVERRUN) {
>  		cf->can_id |= CAN_ERR_CRTL;
> 

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 16/17] can: ems_usb: Added error handling part for CPC-USB/FD. Get some structures packed

[Gerhard Uttenthaler]

Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
---
 drivers/net/can/usb/ems_usb.c | 108 +++++++++++++++++++++++++++++++++-
 1 file changed, 105 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
index 4a67c57c4760..7ede3ac08ed5 100644
--- a/drivers/net/can/usb/ems_usb.c
+++ b/drivers/net/can/usb/ems_usb.c
@@ -103,6 +103,23 @@ MODULE_LICENSE("GPL v2");
 
 #define SJA1000_DEFAULT_OUTPUT_CONTROL 0xDA
 
+#define LPC546XX_ERROR_MASK  0x07
+#define LPC546XX_ERROR_STUFF 0x01
+#define LPC546XX_ERROR_FORM  0x02
+#define LPC546XX_ERROR_ACK   0x03
+#define LPC546XX_ERROR_BIT1  0x04
+#define LPC546XX_ERROR_BIT0  0x05
+#define LPC546XX_ERROR_CRC   0x06
+#define LPC546XX_ERROR_EP    0x20
+#define LPC546XX_ERROR_EW    0x40
+#define LPC546XX_ERROR_BO    0x80
+
+#define LPC546XX_ACT_MASK 0x18
+#define LPC546XX_ACT_SYNC 0x00
+#define LPC546XX_ACT_IDLE 0x08
+#define LPC546XX_ACT_RX   0x10
+#define LPC546XX_ACT_TX   0x18
+
 #define SJA1000   2 // NXP basic CAN controller
 #define LPC546XX  5 // NXP CAN FD controller
 
@@ -217,20 +234,25 @@ struct cpc_confirm {
 };
 
 /* Structure for overrun conditions */
-struct cpc_overrun {
+struct __packed cpc_overrun {
 	u8 event;
 	u8 count;
 };
 
 /* SJA1000 CAN errors (compatible to NXP LPC2119) */
-struct cpc_sja1000_can_error {
+struct __packed cpc_sja1000_can_error {
 	u8 ecc;
 	u8 rxerr;
 	u8 txerr;
 };
 
+struct __packed cpc_lpc546xx_can_error {
+	__le32 psr; /* protocol status register */
+	__le32 ecr; /* error counter register */
+};
+
 /* structure for CAN error conditions */
-struct cpc_can_error {
+struct __packed cpc_can_error {
 	u8 ecode;
 
 	struct {
@@ -239,6 +261,7 @@ struct cpc_can_error {
 		/* Other controllers may also provide error code capture regs */
 		union {
 			struct cpc_sja1000_can_error sja1000;
+			struct cpc_lpc546xx_can_error lpc546xx;
 		} regs;
 	} cc;
 };
@@ -502,6 +525,85 @@ static void ems_usb_rx_err(struct ems_usb *dev, struct ems_cpc_msg *msg)
 				    CAN_ERR_CRTL_TX_PASSIVE : CAN_ERR_CRTL_RX_PASSIVE;
 			}
 		}
+
+		/* CPC-USB/FD */
+		else if (msg->msg.error.cc.cc_type == LPC546XX) {
+			struct net_device *netdev = dev->netdev;
+			u32 psr = msg->msg.error.cc.regs.lpc546xx.psr;
+			u8 txerr = (u8)msg->msg.error.cc.regs.lpc546xx.ecr;
+			u8 rxerr = (u8)(msg->msg.error.cc.regs.lpc546xx.ecr >> 8);
+
+			/* bus error interrupt */
+			dev->can.can_stats.bus_error++;
+
+			cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
+
+			switch (psr & LPC546XX_ERROR_MASK) {
+			case LPC546XX_ERROR_STUFF:
+				netdev_dbg(netdev, "stuff error\n");
+				cf->data[2] |= CAN_ERR_PROT_STUFF;
+				break;
+			case LPC546XX_ERROR_FORM:
+				netdev_dbg(netdev, "form error\n");
+				cf->data[2] |= CAN_ERR_PROT_FORM;
+				break;
+			case LPC546XX_ERROR_ACK:
+				netdev_dbg(netdev, "ack error\n");
+				cf->data[3] = CAN_ERR_PROT_LOC_ACK;
+				break;
+			case LPC546XX_ERROR_BIT1:
+				netdev_dbg(netdev, "bit1 error\n");
+				cf->data[2] |= CAN_ERR_PROT_BIT1;
+				break;
+			case LPC546XX_ERROR_BIT0:
+				netdev_dbg(netdev, "bit0 error\n");
+				cf->data[2] |= CAN_ERR_PROT_BIT0;
+				break;
+			case LPC546XX_ERROR_CRC:
+				netdev_dbg(netdev, "CRC error\n");
+				cf->data[3] = CAN_ERR_PROT_LOC_CRC_SEQ;
+				break;
+			default:
+				break;
+			}
+
+			/* Let activity flags decide direction */
+			switch (psr & LPC546XX_ACT_MASK) {
+			case LPC546XX_ACT_SYNC:
+				__attribute__((fallthrough));
+			case LPC546XX_ACT_IDLE:
+				__attribute__((fallthrough));
+			case LPC546XX_ACT_RX:
+				stats->rx_errors++;
+				break;
+			case LPC546XX_ACT_TX:
+				stats->tx_errors++;
+				break;
+			}
+
+			/* Error warning status */
+			if (psr & LPC546XX_ERROR_EW) {
+				cf->data[1] = (txerr > rxerr) ?
+					CAN_ERR_CRTL_TX_WARNING :
+					CAN_ERR_CRTL_RX_WARNING;
+				cf->can_id |= CAN_ERR_CRTL;
+			}
+
+			/* Error passive status */
+			if (psr & LPC546XX_ERROR_EP) {
+				cf->data[1] |= (txerr > rxerr) ?
+					CAN_ERR_CRTL_TX_PASSIVE :
+					CAN_ERR_CRTL_RX_PASSIVE;
+				cf->can_id |= CAN_ERR_CRTL;
+			}
+
+			/* Error bus off status */
+			if (psr & LPC546XX_ERROR_BO)
+				cf->can_id |= CAN_ERR_BUSOFF;
+
+			cf->data[6] = txerr;
+			cf->data[7] = rxerr;
+		}
 	} else if (msg->type == CPC_MSG_TYPE_OVERRUN) {
 		cf->can_id |= CAN_ERR_CRTL;
 		cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
-- 
2.26.2

-- 
EMS Dr. Thomas Wuensche e.K.
Sonnenhang 3
85304 Ilmmuenster
HR Ingolstadt, HRA 170106

Phone: +49-8441-490260
Fax  : +49-8441-81860
http://www.ems-wuensche.com

Re: [PATCH 16/17] can: ems_usb: Added error handling part for CPC-USB/FD. Get some structures packed

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 5353 bytes --]

please make stuct packing a separate patch

On 11/6/20 6:02 PM, Gerhard Uttenthaler wrote:
> Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
> ---
>  drivers/net/can/usb/ems_usb.c | 108 +++++++++++++++++++++++++++++++++-
>  1 file changed, 105 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
> index 4a67c57c4760..7ede3ac08ed5 100644
> --- a/drivers/net/can/usb/ems_usb.c
> +++ b/drivers/net/can/usb/ems_usb.c
> @@ -103,6 +103,23 @@ MODULE_LICENSE("GPL v2");
>  
>  #define SJA1000_DEFAULT_OUTPUT_CONTROL 0xDA
>  
> +#define LPC546XX_ERROR_MASK  0x07
> +#define LPC546XX_ERROR_STUFF 0x01
> +#define LPC546XX_ERROR_FORM  0x02
> +#define LPC546XX_ERROR_ACK   0x03
> +#define LPC546XX_ERROR_BIT1  0x04
> +#define LPC546XX_ERROR_BIT0  0x05
> +#define LPC546XX_ERROR_CRC   0x06
> +#define LPC546XX_ERROR_EP    0x20
> +#define LPC546XX_ERROR_EW    0x40
> +#define LPC546XX_ERROR_BO    0x80

BIT()
only one space
and add a proper prefix

> +
> +#define LPC546XX_ACT_MASK 0x18
> +#define LPC546XX_ACT_SYNC 0x00
> +#define LPC546XX_ACT_IDLE 0x08
> +#define LPC546XX_ACT_RX   0x10
> +#define LPC546XX_ACT_TX   0x18

only one space
and add a proper prefix

> +
>  #define SJA1000   2 // NXP basic CAN controller
>  #define LPC546XX  5 // NXP CAN FD controller
>  
> @@ -217,20 +234,25 @@ struct cpc_confirm {
>  };
>  
>  /* Structure for overrun conditions */
> -struct cpc_overrun {
> +struct __packed cpc_overrun {
>  	u8 event;
>  	u8 count;
>  };
>  
>  /* SJA1000 CAN errors (compatible to NXP LPC2119) */
> -struct cpc_sja1000_can_error {
> +struct __packed cpc_sja1000_can_error {
>  	u8 ecc;
>  	u8 rxerr;
>  	u8 txerr;
>  };
>  
> +struct __packed cpc_lpc546xx_can_error {
> +	__le32 psr; /* protocol status register */
> +	__le32 ecr; /* error counter register */
> +};
> +
>  /* structure for CAN error conditions */
> -struct cpc_can_error {
> +struct __packed cpc_can_error {
>  	u8 ecode;
>  
>  	struct {
> @@ -239,6 +261,7 @@ struct cpc_can_error {
>  		/* Other controllers may also provide error code capture regs */
>  		union {
>  			struct cpc_sja1000_can_error sja1000;
> +			struct cpc_lpc546xx_can_error lpc546xx;
>  		} regs;
>  	} cc;
>  };
> @@ -502,6 +525,85 @@ static void ems_usb_rx_err(struct ems_usb *dev, struct ems_cpc_msg *msg)
>  				    CAN_ERR_CRTL_TX_PASSIVE : CAN_ERR_CRTL_RX_PASSIVE;
>  			}
>  		}
> +
> +		/* CPC-USB/FD */
> +		else if (msg->msg.error.cc.cc_type == LPC546XX) {
> +			struct net_device *netdev = dev->netdev;
> +			u32 psr = msg->msg.error.cc.regs.lpc546xx.psr;
> +			u8 txerr = (u8)msg->msg.error.cc.regs.lpc546xx.ecr;
> +			u8 rxerr = (u8)(msg->msg.error.cc.regs.lpc546xx.ecr >> 8);

the case ist not needed

> +
> +			/* bus error interrupt */
> +			dev->can.can_stats.bus_error++;
> +
> +			cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> +
> +			switch (psr & LPC546XX_ERROR_MASK) {
> +			case LPC546XX_ERROR_STUFF:
> +				netdev_dbg(netdev, "stuff error\n");
> +				cf->data[2] |= CAN_ERR_PROT_STUFF;
> +				break;
> +			case LPC546XX_ERROR_FORM:
> +				netdev_dbg(netdev, "form error\n");
> +				cf->data[2] |= CAN_ERR_PROT_FORM;
> +				break;
> +			case LPC546XX_ERROR_ACK:
> +				netdev_dbg(netdev, "ack error\n");
> +				cf->data[3] = CAN_ERR_PROT_LOC_ACK;
> +				break;
> +			case LPC546XX_ERROR_BIT1:
> +				netdev_dbg(netdev, "bit1 error\n");
> +				cf->data[2] |= CAN_ERR_PROT_BIT1;
> +				break;
> +			case LPC546XX_ERROR_BIT0:
> +				netdev_dbg(netdev, "bit0 error\n");
> +				cf->data[2] |= CAN_ERR_PROT_BIT0;
> +				break;
> +			case LPC546XX_ERROR_CRC:
> +				netdev_dbg(netdev, "CRC error\n");
> +				cf->data[3] = CAN_ERR_PROT_LOC_CRC_SEQ;
> +				break;
> +			default:
> +				break;
> +			}
> +
> +			/* Let activity flags decide direction */
> +			switch (psr & LPC546XX_ACT_MASK) {
> +			case LPC546XX_ACT_SYNC:
> +				__attribute__((fallthrough));

please use "fallthrough;"

> +			case LPC546XX_ACT_IDLE:
> +				__attribute__((fallthrough));
> +			case LPC546XX_ACT_RX:
> +				stats->rx_errors++;
> +				break;
> +			case LPC546XX_ACT_TX:
> +				stats->tx_errors++;
> +				break;
> +			}
> +
> +			/* Error warning status */
> +			if (psr & LPC546XX_ERROR_EW) {
> +				cf->data[1] = (txerr > rxerr) ?
> +					CAN_ERR_CRTL_TX_WARNING :
> +					CAN_ERR_CRTL_RX_WARNING;
> +				cf->can_id |= CAN_ERR_CRTL;
> +			}
> +
> +			/* Error passive status */
> +			if (psr & LPC546XX_ERROR_EP) {
> +				cf->data[1] |= (txerr > rxerr) ?
> +					CAN_ERR_CRTL_TX_PASSIVE :
> +					CAN_ERR_CRTL_RX_PASSIVE;
> +				cf->can_id |= CAN_ERR_CRTL;
> +			}
> +
> +			/* Error bus off status */
> +			if (psr & LPC546XX_ERROR_BO)
> +				cf->can_id |= CAN_ERR_BUSOFF;
> +
> +			cf->data[6] = txerr;
> +			cf->data[7] = rxerr;
> +		}
>  	} else if (msg->type == CPC_MSG_TYPE_OVERRUN) {
>  		cf->can_id |= CAN_ERR_CRTL;
>  		cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
> 

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 17/17] can: ems_usb: Made another struct packed. Enable CPC-USB/FD by adding it to the drivers device table

[Gerhard Uttenthaler]

Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
---
 drivers/net/can/usb/ems_usb.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
index 7ede3ac08ed5..087069f999e5 100644
--- a/drivers/net/can/usb/ems_usb.c
+++ b/drivers/net/can/usb/ems_usb.c
@@ -269,7 +269,7 @@ struct __packed cpc_can_error {
 /* Structure containing RX/TX error counter. This structure is used to request
  * the values of the CAN controllers TX and RX error counter.
  */
-struct cpc_can_err_counter {
+struct __packed cpc_can_err_counter {
 	u8 rx;
 	u8 tx;
 };
@@ -296,10 +296,11 @@ struct __packed ems_cpc_msg {
 };
 
 /* Table of devices that work with this driver
- * NOTE: This driver supports only CPC-USB/ARM7 (LPC2119) yet.
+ * This driver supports CPC-USB/ARM7 and CPC-USB/FD.
  */
 static struct usb_device_id ems_usb_table[] = {
 	{USB_DEVICE(USB_CPCUSB_VENDOR_ID, USB_CPCUSB_ARM7_PRODUCT_ID)},
+	{USB_DEVICE(USB_CPCUSB_VENDOR_ID, USB_CPCUSB_FD_PRODUCT_ID)},
 	{} /* Terminating entry */
 };
 
-- 
2.26.2

-- 
EMS Dr. Thomas Wuensche e.K.
Sonnenhang 3
85304 Ilmmuenster
HR Ingolstadt, HRA 170106

Phone: +49-8441-490260
Fax  : +49-8441-81860
http://www.ems-wuensche.com

Re: [PATCH 17/17] can: ems_usb: Made another struct packed. Enable CPC-USB/FD by adding it to the drivers device table

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1548 bytes --]

On 11/6/20 6:02 PM, Gerhard Uttenthaler wrote:
> Signed-off-by: Gerhard Uttenthaler <uttenthaler@ems-wuensche.com>
> ---
>  drivers/net/can/usb/ems_usb.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
> index 7ede3ac08ed5..087069f999e5 100644
> --- a/drivers/net/can/usb/ems_usb.c
> +++ b/drivers/net/can/usb/ems_usb.c
> @@ -269,7 +269,7 @@ struct __packed cpc_can_error {
>  /* Structure containing RX/TX error counter. This structure is used to request
>   * the values of the CAN controllers TX and RX error counter.
>   */
> -struct cpc_can_err_counter {
> +struct __packed cpc_can_err_counter {

please make this a separate patch.

>  	u8 rx;
>  	u8 tx;
>  };
> @@ -296,10 +296,11 @@ struct __packed ems_cpc_msg {
>  };
>  
>  /* Table of devices that work with this driver
> - * NOTE: This driver supports only CPC-USB/ARM7 (LPC2119) yet.
> + * This driver supports CPC-USB/ARM7 and CPC-USB/FD.
>   */
>  static struct usb_device_id ems_usb_table[] = {
>  	{USB_DEVICE(USB_CPCUSB_VENDOR_ID, USB_CPCUSB_ARM7_PRODUCT_ID)},
> +	{USB_DEVICE(USB_CPCUSB_VENDOR_ID, USB_CPCUSB_FD_PRODUCT_ID)},
>  	{} /* Terminating entry */
>  };
>  
> 

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 00/17] Add support for CPC-USB/FD CAN FD interface

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 3167 bytes --]

On 11/6/20 6:01 PM, Gerhard Uttenthaler wrote:
> These patches extend the ems_usb.c driver to support both devices, the
> classic CAN CPC-USB/ARM7 and the CAN FD CPC-USB/FD. Also support for
> the listen only mode for CPC-USB/ARM7 was implemented. Most issues given
> by checkpatch.pl were resolved.
> 
> Gerhard Uttenthaler (17):
>   can: ems_usb: Fixed warnings given by checkpatch.pl and fixed some
>     outdated comments
>   can: ems_usb: Added defines and product id needed for CPC-USB/FD
>   can: ems_usb: Added CAN FD message representation
>   can: ems_usb: Added struct used for CAN FD initialization
>   can: ems_usb: Replace constant define RX_BUFFER_SIZE by variable
>     bulk_rd_buf_size, because this will have different values for
>     CPC-USB/ARM7 and CPC-USB/FD. For the same reason added a function
>     pointer ems_usb_write_mode. In device probe function added a switch
>     statement to select between CPC-USB/ARM7 and CPC-USB/FD and
>     rearranged initialization sequence accordingly.
>   can: ems_usb: Added listen only mode for CPC-USB/ARM7 and moved
>     evaluation of can.ctrlmode from set_bittiming routine to
>     ems_usb_write_mode_arm7 routine. Wrapped a long line.
>   can: ems_usb: Added CPC_ClearCmdQueue routine to get access to the
>     interface even it is flooded with messages which cannot successfully
>     be sent. Set timestamp to 0 in ems_usb_control_cmd.
>   can: ems_usb: Modified ems_usb_read_bulk_callback to be able to handle
>     also CPC-USB/FD
>   can: ems_usb: For CPC-USB/FD added clock definitions, bittiming
>     constants, set_bittiming functions, bittiming init function and add
>     all that to probe function
>   can: ems_usb: Added receive routine for CAN FD messages and its call
>     in ems_usb_read_bulk_callback
>   can: ems_usb: Added ems_usb_write_mode_fd and its call in device probe
>     routine. Fixed indentation.
>   can: ems_usb: In ems_usb_start_xmit send only bytes with valid content
>     to interface and not the complete buffer. Set first four bytes of
>     buffer to 0. Wrapped long lines.
>   can: ems_usb: Rearrange code in ems_usb_start_xmit to check with
>     can_is_canfd_skb for CAN FD frames.
>   can: ems_usb: Added code to handle CAN FD frames in ems_usb_start_xmit
>   can: ems_usb: In CAN error handling routine checking which CAN
>     controller type is issuing the error
>   can: ems_usb: Added error handling part for CPC-USB/FD. Get some
>     structures packed
>   can: ems_usb: Made another struct packed. Enable CPC-USB/FD by adding
>     it to the drivers device table
> 
>  drivers/net/can/usb/ems_usb.c | 860 +++++++++++++++++++++++++++-------
>  1 file changed, 681 insertions(+), 179 deletions(-)
> 

Looks good, some nitpicks inline.

After the next iteration, we can squash all the FD related patches into one.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 00/17] Add support for CPC-USB/FD CAN FD interface

[Gerhard Uttenthaler]

Hi Marc,

Am 06.11.20 um 19:07 schrieb Marc Kleine-Budde:
> On 11/6/20 6:01 PM, Gerhard Uttenthaler wrote:
>> These patches extend the ems_usb.c driver to support both devices, the
>> classic CAN CPC-USB/ARM7 and the CAN FD CPC-USB/FD. Also support for
>> the listen only mode for CPC-USB/ARM7 was implemented. Most issues given
>> by checkpatch.pl were resolved.
>>
>> Gerhard Uttenthaler (17):
>>   can: ems_usb: Fixed warnings given by checkpatch.pl and fixed some
>>     outdated comments
>>   can: ems_usb: Added defines and product id needed for CPC-USB/FD
>>   can: ems_usb: Added CAN FD message representation
>>   can: ems_usb: Added struct used for CAN FD initialization
>>   can: ems_usb: Replace constant define RX_BUFFER_SIZE by variable
>>     bulk_rd_buf_size, because this will have different values for
>>     CPC-USB/ARM7 and CPC-USB/FD. For the same reason added a function
>>     pointer ems_usb_write_mode. In device probe function added a switch
>>     statement to select between CPC-USB/ARM7 and CPC-USB/FD and
>>     rearranged initialization sequence accordingly.
>>   can: ems_usb: Added listen only mode for CPC-USB/ARM7 and moved
>>     evaluation of can.ctrlmode from set_bittiming routine to
>>     ems_usb_write_mode_arm7 routine. Wrapped a long line.
>>   can: ems_usb: Added CPC_ClearCmdQueue routine to get access to the
>>     interface even it is flooded with messages which cannot successfully
>>     be sent. Set timestamp to 0 in ems_usb_control_cmd.
>>   can: ems_usb: Modified ems_usb_read_bulk_callback to be able to handle
>>     also CPC-USB/FD
>>   can: ems_usb: For CPC-USB/FD added clock definitions, bittiming
>>     constants, set_bittiming functions, bittiming init function and add
>>     all that to probe function
>>   can: ems_usb: Added receive routine for CAN FD messages and its call
>>     in ems_usb_read_bulk_callback
>>   can: ems_usb: Added ems_usb_write_mode_fd and its call in device probe
>>     routine. Fixed indentation.
>>   can: ems_usb: In ems_usb_start_xmit send only bytes with valid content
>>     to interface and not the complete buffer. Set first four bytes of
>>     buffer to 0. Wrapped long lines.
>>   can: ems_usb: Rearrange code in ems_usb_start_xmit to check with
>>     can_is_canfd_skb for CAN FD frames.
>>   can: ems_usb: Added code to handle CAN FD frames in ems_usb_start_xmit
>>   can: ems_usb: In CAN error handling routine checking which CAN
>>     controller type is issuing the error
>>   can: ems_usb: Added error handling part for CPC-USB/FD. Get some
>>     structures packed
>>   can: ems_usb: Made another struct packed. Enable CPC-USB/FD by adding
>>     it to the drivers device table
>>
>>  drivers/net/can/usb/ems_usb.c | 860 +++++++++++++++++++++++++++-------
>>  1 file changed, 681 insertions(+), 179 deletions(-)
>>
> 
> Looks good, some nitpicks inline.
> 
> After the next iteration, we can squash all the FD related patches into one.
> 
> Marc
> 

Thank you for your instant answers! Would it be OK to resend after
rework all the patches again as [PATCH V2 xx/yy]?

Regards
Gerhard

-- 
EMS Dr. Thomas Wuensche e.K.
Sonnenhang 3
85304 Ilmmuenster
HR Ingolstadt, HRA 170106

Phone: +49-8441-490260
Fax  : +49-8441-81860
http://www.ems-wuensche.com

Re: [PATCH 00/17] Add support for CPC-USB/FD CAN FD interface

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 460 bytes --]

On 11/6/20 8:04 PM, Gerhard Uttenthaler wrote:
> Thank you for your instant answers! Would it be OK to resend after
> rework all the patches again as [PATCH V2 xx/yy]?

sure

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 00/17] Add support for CPC-USB/FD CAN FD interface

[Gerhard Uttenthaler]

Hi Marc,

I resent the patch series a while ago. Did I miss something or did I do
something wrong? You immediately commented on the first series, but now
there is silence.

Regards
Gerhard

Am 06.11.20 um 20:59 schrieb Marc Kleine-Budde:
> On 11/6/20 8:04 PM, Gerhard Uttenthaler wrote:
>> Thank you for your instant answers! Would it be OK to resend after
>> rework all the patches again as [PATCH V2 xx/yy]?
> 
> sure
> 
> Marc
> 


-- 
EMS Dr. Thomas Wuensche e.K.
Sonnenhang 3
85304 Ilmmuenster
HR Ingolstadt, HRA 170106

Phone: +49-8441-490260
Fax  : +49-8441-81860
http://www.ems-wuensche.com

Re: [PATCH 00/17] Add support for CPC-USB/FD CAN FD interface

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 7350 bytes --]

On 11/6/20 6:01 PM, Gerhard Uttenthaler wrote:
FTBFS:

> ems_usb.c: In function ‘ems_usb_rx_err’:
> ems_usb.c:574:20: error: expected ‘)’ before ‘__attribute__’
>   574 |     __attribute__((fallthrough));
>       |                    ^
>       |                    )
> ems_usb.c:574:32: error: expected statement before ‘)’ token
>   574 |     __attribute__((fallthrough));
>       |                                ^
> ems_usb.c:576:20: error: expected ‘)’ before ‘__attribute__’
>   576 |     __attribute__((fallthrough));
>       |                    ^
>       |                    )
> ems_usb.c:576:32: error: expected statement before ‘)’ token
>   576 |     __attribute__((fallthrough));
>       |                                ^

use "fallthrough;" instead.

please fix the following sparse warnings:

> ems_usb.c:533:66: warning: incorrect type in initializer (different base types)                                                                                                                  [14/7920]
> ems_usb.c:533:66:    expected unsigned int [usertype] psr                                                                                                                                                 
> ems_usb.c:533:66:    got restricted __le32 [usertype] psr                                                                                                                                                 
> ems_usb.c:534:37: warning: cast from restricted __le32                                                                                                                                                    
> ems_usb.c:535:72: warning: restricted __le32 degrades to integer                                                                                                                                          
> ems_usb.c:805:29: warning: invalid assignment: |=                                                                                                                                                         
> ems_usb.c:805:29:    left side has type restricted __le32                                                                                                                                                 
> ems_usb.c:805:29:    right side has type int                                                                                                                                                              
> ems_usb.c:807:29: warning: invalid assignment: &=                                                                                                                                                         
> ems_usb.c:807:29:    left side has type restricted __le32                                                                                                                                                 
> ems_usb.c:807:29:    right side has type int                                                                                                                                                              
> ems_usb.c:810:37: warning: invalid assignment: |=                                                                                                                                                         
> ems_usb.c:810:37:    left side has type restricted __le32                                                                                                                                                 
> ems_usb.c:810:37:    right side has type int                                                                                                                                                              
> ems_usb.c:812:37: warning: invalid assignment: &=
> ems_usb.c:812:37:    left side has type restricted __le32
> ems_usb.c:812:37:    right side has type int
> ems_usb.c:815:37: warning: invalid assignment: |=
> ems_usb.c:815:37:    left side has type restricted __le32
> ems_usb.c:815:37:    right side has type int
> ems_usb.c:817:37: warning: invalid assignment: &=
> ems_usb.c:817:37:    left side has type restricted __le32
> ems_usb.c:817:37:    right side has type int
> ems_usb.c:1321:22: warning: incorrect type in assignment (different base types)
> ems_usb.c:1321:22:    expected restricted __le32 [usertype] can_clk
> ems_usb.c:1321:22:    got unsigned int [usertype] freq
> ems_usb.c:1323:22: warning: incorrect type in assignment (different base types)
> ems_usb.c:1323:22:    expected restricted __le16 [usertype] tseg1
> ems_usb.c:1323:22:    got unsigned int
> ems_usb.c:1324:22: warning: incorrect type in assignment (different base types)
> ems_usb.c:1324:22:    expected restricted __le16 [usertype] tseg2
> ems_usb.c:1324:22:    got unsigned int [usertype] phase_seg2
> ems_usb.c:1325:20: warning: incorrect type in assignment (different base types)
> ems_usb.c:1325:20:    expected restricted __le16 [usertype] brp
> ems_usb.c:1325:20:    got unsigned int [usertype] brp
> ems_usb.c:1326:20: warning: incorrect type in assignment (different base types)
> ems_usb.c:1326:20:    expected restricted __le16 [usertype] sjw
> ems_usb.c:1326:20:    got unsigned int [usertype] sjw
> ems_usb.c:1335:24: warning: restricted __le32 degrades to integer
> ems_usb.c:1353:29: warning: invalid assignment: |=
> ems_usb.c:1353:29:    left side has type restricted __le32
> ems_usb.c:1353:29:    right side has type int
> ems_usb.c:1355:37: warning: invalid assignment: |=
> ems_usb.c:1355:37:    left side has type restricted __le32
> ems_usb.c:1355:37:    right side has type int
> ems_usb.c:1361:22: warning: incorrect type in assignment (different base types)
> ems_usb.c:1361:22:    expected restricted __le16 [usertype] tseg1
> ems_usb.c:1361:22:    got unsigned int
> ems_usb.c:1362:22: warning: incorrect type in assignment (different base types)
> ems_usb.c:1362:22:    expected restricted __le16 [usertype] tseg2
> ems_usb.c:1362:22:    got unsigned int [usertype] phase_seg2
> ems_usb.c:1363:20: warning: incorrect type in assignment (different base types)
> ems_usb.c:1363:20:    expected restricted __le16 [usertype] brp
> ems_usb.c:1363:20:    got unsigned int [usertype] brp
> ems_usb.c:1364:20: warning: incorrect type in assignment (different base types)
> ems_usb.c:1364:20:    expected restricted __le16 [usertype] sjw
> ems_usb.c:1364:20:    got unsigned int [usertype] sjw
> ems_usb.c:1373:24: warning: restricted __le32 degrades to integer
> ems_usb.c:1428:21: warning: incorrect type in assignment (different base types)
> ems_usb.c:1428:21:    expected restricted __le32 [usertype] config
> ems_usb.c:1428:21:    got int
> ems_usb.c:1429:22: warning: incorrect type in assignment (different base types)
> ems_usb.c:1429:22:    expected restricted __le32 [usertype] can_clk
> ems_usb.c:1429:22:    got int
> ems_usb.c:1455:38: warning: restricted __le16 degrades to integer
> ems_usb.c:1455:38: warning: restricted __le16 degrades to integer

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]