[PATCH 0/2] can: kvaser_usb: CAN clock frequency regression

[PATCH 0/2] can: kvaser_usb: CAN clock frequency regression

[Jimmy Assarsson]

When fixing the CAN clock frequency,
fb12797ab1fe ("can: kvaser_usb: get CAN clock frequency from device"),
I introduced a regression.

For Leaf devices based on M32C, the firmware expects bittiming parameters
calculated for 16MHz clock. Regardless of the actual clock frequency.

This regression affects M32C based Leaf devices with non-16MHz clock.

Also correct the bittiming constants in kvaser_usb_leaf.c, where the limits
are different depending on which firmware/device being used.

Once merged to mainline, I'll backport these fixes for the stable kernels.

Jimmy Assarsson (2):
  can: kvaser_usb: kvaser_usb_leaf: Fix CAN clock frequency regression
  can: kvaser_usb: kvaser_usb_leaf: Fix bittiming limits

 drivers/net/can/usb/kvaser_usb/kvaser_usb.h   | 17 ++++
 .../net/can/usb/kvaser_usb/kvaser_usb_core.c  | 20 +++-
 .../net/can/usb/kvaser_usb/kvaser_usb_hydra.c | 14 +--
 .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c  | 95 +++++++++++--------
 4 files changed, 89 insertions(+), 57 deletions(-)

--
2.36.1

[PATCH 1/2] can: kvaser_usb: kvaser_usb_leaf: Fix CAN clock frequency regression

[Jimmy Assarsson]

The firmware of M32C based Leaf devices expects bittiming parameters
calculated for 16MHz clock.
Since we use the actual clock frequency of the device, the device may end
up with wrong bittiming parameters, depending on user requested parameters.

This regression affects M32C based Leaf devices with non-16MHz clock.

Fixes: fb12797ab1fe ("can: kvaser_usb: get CAN clock frequency from device")
Cc: stable@vger.kernel.org
Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
---
 drivers/net/can/usb/kvaser_usb/kvaser_usb.h   |  4 +++
 .../net/can/usb/kvaser_usb/kvaser_usb_core.c  | 20 +++++++++++----
 .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c  | 25 ++++++++++++-------
 3 files changed, 35 insertions(+), 14 deletions(-)

diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb.h b/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
index 3a49257f9fa6..cb588228d7a1 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
@@ -44,6 +44,9 @@
 #define KVASER_USB_CAP_EXT_CAP			0x02
 #define KVASER_USB_HYDRA_CAP_EXT_CMD		0x04
 
+/* Quriks */
+#define KVASER_USB_QUIRK_IGNORE_CLK_FREQ BIT(0)
+
 struct kvaser_usb_dev_cfg;
 
 enum kvaser_usb_leaf_family {
@@ -65,6 +68,7 @@ struct kvaser_usb_dev_card_data_hydra {
 struct kvaser_usb_dev_card_data {
 	u32 ctrlmode_supported;
 	u32 capabilities;
+	u32 quirks;
 	union {
 		struct {
 			enum kvaser_usb_leaf_family family;
diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
index e67658b53d02..5880e9719c9d 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
@@ -94,10 +94,14 @@
 
 static inline bool kvaser_is_leaf(const struct usb_device_id *id)
 {
-	return (id->idProduct >= USB_LEAF_DEVEL_PRODUCT_ID &&
-		id->idProduct <= USB_CAN_R_PRODUCT_ID) ||
-		(id->idProduct >= USB_LEAF_LITE_V2_PRODUCT_ID &&
-		 id->idProduct <= USB_LEAF_PRODUCT_ID_END);
+	return id->idProduct >= USB_LEAF_DEVEL_PRODUCT_ID &&
+	       id->idProduct <= USB_CAN_R_PRODUCT_ID;
+}
+
+static inline bool kvaser_is_leafimx(const struct usb_device_id *id)
+{
+	return id->idProduct >= USB_LEAF_LITE_V2_PRODUCT_ID &&
+	       id->idProduct <= USB_LEAF_PRODUCT_ID_END;
 }
 
 static inline bool kvaser_is_usbcan(const struct usb_device_id *id)
@@ -113,7 +117,7 @@ static inline bool kvaser_is_hydra(const struct usb_device_id *id)
 }
 
 static const struct usb_device_id kvaser_usb_table[] = {
-	/* Leaf USB product IDs */
+	/* Leaf M32C USB product IDs */
 	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_DEVEL_PRODUCT_ID) },
 	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LITE_PRODUCT_ID) },
 	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_PRO_PRODUCT_ID),
@@ -161,6 +165,8 @@ static const struct usb_device_id kvaser_usb_table[] = {
 		.driver_info = KVASER_USB_HAS_TXRX_ERRORS },
 	{ USB_DEVICE(KVASER_VENDOR_ID, USB_CAN_R_PRODUCT_ID),
 		.driver_info = KVASER_USB_HAS_TXRX_ERRORS },
+
+	/* Leaf i.MX28 USB product IDs */
 	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LITE_V2_PRODUCT_ID) },
 	{ USB_DEVICE(KVASER_VENDOR_ID, USB_MINI_PCIE_HS_PRODUCT_ID) },
 	{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LIGHT_HS_V2_OEM_PRODUCT_ID) },
@@ -737,6 +743,10 @@ static int kvaser_usb_probe(struct usb_interface *intf,
 	if (kvaser_is_leaf(id)) {
 		dev->card_data.leaf.family = KVASER_LEAF;
 		dev->ops = &kvaser_usb_leaf_dev_ops;
+		dev->card_data.quirks = KVASER_USB_QUIRK_IGNORE_CLK_FREQ;
+	} else if (kvaser_is_leafimx(id)) {
+		dev->card_data.leaf.family = KVASER_LEAF;
+		dev->ops = &kvaser_usb_leaf_dev_ops;
 	} else if (kvaser_is_usbcan(id)) {
 		dev->card_data.leaf.family = KVASER_USBCAN;
 		dev->ops = &kvaser_usb_leaf_dev_ops;
diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
index c805b999c543..68c698e3b2b2 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
@@ -524,16 +524,23 @@ static void kvaser_usb_leaf_get_software_info_leaf(struct kvaser_usb *dev,
 	dev->fw_version = le32_to_cpu(softinfo->fw_version);
 	dev->max_tx_urbs = le16_to_cpu(softinfo->max_outstanding_tx);
 
-	switch (sw_options & KVASER_USB_LEAF_SWOPTION_FREQ_MASK) {
-	case KVASER_USB_LEAF_SWOPTION_FREQ_16_MHZ_CLK:
+	if (dev->card_data.quirks & KVASER_USB_QUIRK_IGNORE_CLK_FREQ) {
+		/* Firmware expects bittiming parameters calculated for 16MHz
+		 * clock, regardless of the actual clock
+		 */
 		dev->cfg = &kvaser_usb_leaf_dev_cfg_16mhz;
-		break;
-	case KVASER_USB_LEAF_SWOPTION_FREQ_24_MHZ_CLK:
-		dev->cfg = &kvaser_usb_leaf_dev_cfg_24mhz;
-		break;
-	case KVASER_USB_LEAF_SWOPTION_FREQ_32_MHZ_CLK:
-		dev->cfg = &kvaser_usb_leaf_dev_cfg_32mhz;
-		break;
+	} else {
+		switch (sw_options & KVASER_USB_LEAF_SWOPTION_FREQ_MASK) {
+		case KVASER_USB_LEAF_SWOPTION_FREQ_16_MHZ_CLK:
+			dev->cfg = &kvaser_usb_leaf_dev_cfg_16mhz;
+			break;
+		case KVASER_USB_LEAF_SWOPTION_FREQ_24_MHZ_CLK:
+			dev->cfg = &kvaser_usb_leaf_dev_cfg_24mhz;
+			break;
+		case KVASER_USB_LEAF_SWOPTION_FREQ_32_MHZ_CLK:
+			dev->cfg = &kvaser_usb_leaf_dev_cfg_32mhz;
+			break;
+		}
 	}
 }
 
-- 
2.36.1

[PATCH 2/2] can: kvaser_usb: kvaser_usb_leaf: Fix bittiming limits

[Jimmy Assarsson]

Use correct bittiming limits depending on device.
For devices based on USBcanII, Leaf M32C or Leaf i.MX28.

Fixes: 080f40a6fa28 ("can: kvaser_usb: Add support for Kvaser CAN/USB devices")
Fixes: b4f20130af23 ("can: kvaser_usb: add support for Kvaser Leaf v2 and usb mini PCIe")
Fixes: f5d4abea3ce0 ("can: kvaser_usb: Add support for the USBcan-II family")
Cc: stable@vger.kernel.org
Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
---
 drivers/net/can/usb/kvaser_usb/kvaser_usb.h   | 13 ++++
 .../net/can/usb/kvaser_usb/kvaser_usb_hydra.c | 14 +---
 .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c  | 76 +++++++++++--------
 3 files changed, 57 insertions(+), 46 deletions(-)

diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb.h b/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
index cb588228d7a1..8069a67881bb 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
@@ -28,6 +28,7 @@
 
 #include <linux/can.h>
 #include <linux/can/dev.h>
+#include <linux/can/netlink.h>
 
 #define KVASER_USB_MAX_RX_URBS			4
 #define KVASER_USB_MAX_TX_URBS			128
@@ -188,4 +189,16 @@ int kvaser_usb_send_cmd_async(struct kvaser_usb_net_priv *priv, void *cmd,
 			      int len);
 
 int kvaser_usb_can_rx_over_error(struct net_device *netdev);
+
+static const struct can_bittiming_const kvaser_usb_flexc_bittiming_const = {
+	.name = "kvaser_usb_flex",
+	.tseg1_min = 4,
+	.tseg1_max = 16,
+	.tseg2_min = 2,
+	.tseg2_max = 8,
+	.sjw_max = 4,
+	.brp_min = 1,
+	.brp_max = 256,
+	.brp_inc = 1,
+};
 #endif /* KVASER_USB_H */
diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
index a26823c5b62a..faf901e7e568 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
@@ -375,18 +375,6 @@ static const struct can_bittiming_const kvaser_usb_hydra_kcan_bittiming_c = {
 	.brp_inc = 1,
 };
 
-static const struct can_bittiming_const kvaser_usb_hydra_flexc_bittiming_c = {
-	.name = "kvaser_usb_flex",
-	.tseg1_min = 4,
-	.tseg1_max = 16,
-	.tseg2_min = 2,
-	.tseg2_max = 8,
-	.sjw_max = 4,
-	.brp_min = 1,
-	.brp_max = 256,
-	.brp_inc = 1,
-};
-
 static const struct can_bittiming_const kvaser_usb_hydra_rt_bittiming_c = {
 	.name = "kvaser_usb_rt",
 	.tseg1_min = 2,
@@ -2052,7 +2040,7 @@ static const struct kvaser_usb_dev_cfg kvaser_usb_hydra_dev_cfg_flexc = {
 		.freq = 24 * MEGA /* Hz */,
 	},
 	.timestamp_freq = 1,
-	.bittiming_const = &kvaser_usb_hydra_flexc_bittiming_c,
+	.bittiming_const = &kvaser_usb_flexc_bittiming_const,
 };
 
 static const struct kvaser_usb_dev_cfg kvaser_usb_hydra_dev_cfg_rt = {
diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
index 68c698e3b2b2..c5829a3cfd45 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
@@ -101,16 +101,6 @@
 #define USBCAN_ERROR_STATE_RX_ERROR	BIT(1)
 #define USBCAN_ERROR_STATE_BUSERROR	BIT(2)
 
-/* bittiming parameters */
-#define KVASER_USB_TSEG1_MIN		1
-#define KVASER_USB_TSEG1_MAX		16
-#define KVASER_USB_TSEG2_MIN		1
-#define KVASER_USB_TSEG2_MAX		8
-#define KVASER_USB_SJW_MAX		4
-#define KVASER_USB_BRP_MIN		1
-#define KVASER_USB_BRP_MAX		64
-#define KVASER_USB_BRP_INC		1
-
 /* ctrl modes */
 #define KVASER_CTRL_MODE_NORMAL		1
 #define KVASER_CTRL_MODE_SILENT		2
@@ -343,48 +333,68 @@ struct kvaser_usb_err_summary {
 	};
 };
 
-static const struct can_bittiming_const kvaser_usb_leaf_bittiming_const = {
-	.name = "kvaser_usb",
-	.tseg1_min = KVASER_USB_TSEG1_MIN,
-	.tseg1_max = KVASER_USB_TSEG1_MAX,
-	.tseg2_min = KVASER_USB_TSEG2_MIN,
-	.tseg2_max = KVASER_USB_TSEG2_MAX,
-	.sjw_max = KVASER_USB_SJW_MAX,
-	.brp_min = KVASER_USB_BRP_MIN,
-	.brp_max = KVASER_USB_BRP_MAX,
-	.brp_inc = KVASER_USB_BRP_INC,
+static const struct can_bittiming_const kvaser_usb_leaf_m16c_bittiming_const = {
+	.name = "kvaser_usb_ucii",
+	.tseg1_min = 4,
+	.tseg1_max = 16,
+	.tseg2_min = 2,
+	.tseg2_max = 8,
+	.sjw_max = 4,
+	.brp_min = 1,
+	.brp_max = 16,
+	.brp_inc = 1,
+};
+
+static const struct can_bittiming_const kvaser_usb_leaf_m32c_bittiming_const = {
+	.name = "kvaser_usb_leaf",
+	.tseg1_min = 3,
+	.tseg1_max = 16,
+	.tseg2_min = 2,
+	.tseg2_max = 8,
+	.sjw_max = 4,
+	.brp_min = 2,
+	.brp_max = 128,
+	.brp_inc = 2,
 };
 
-static const struct kvaser_usb_dev_cfg kvaser_usb_leaf_dev_cfg_8mhz = {
+static const struct kvaser_usb_dev_cfg kvaser_usb_leaf_usbcan_dev_cfg = {
 	.clock = {
 		.freq = 8 * MEGA /* Hz */,
 	},
 	.timestamp_freq = 1,
-	.bittiming_const = &kvaser_usb_leaf_bittiming_const,
+	.bittiming_const = &kvaser_usb_leaf_m16c_bittiming_const,
+};
+
+static const struct kvaser_usb_dev_cfg kvaser_usb_leaf_m32c_dev_cfg = {
+	.clock = {
+		.freq = 16 * MEGA /* Hz */,
+	},
+	.timestamp_freq = 1,
+	.bittiming_const = &kvaser_usb_leaf_m32c_bittiming_const,
 };
 
-static const struct kvaser_usb_dev_cfg kvaser_usb_leaf_dev_cfg_16mhz = {
+static const struct kvaser_usb_dev_cfg kvaser_usb_leaf_imx_dev_cfg_16mhz = {
 	.clock = {
 		.freq = 16 * MEGA /* Hz */,
 	},
 	.timestamp_freq = 1,
-	.bittiming_const = &kvaser_usb_leaf_bittiming_const,
+	.bittiming_const = &kvaser_usb_flexc_bittiming_const,
 };
 
-static const struct kvaser_usb_dev_cfg kvaser_usb_leaf_dev_cfg_24mhz = {
+static const struct kvaser_usb_dev_cfg kvaser_usb_leaf_imx_dev_cfg_24mhz = {
 	.clock = {
 		.freq = 24 * MEGA /* Hz */,
 	},
 	.timestamp_freq = 1,
-	.bittiming_const = &kvaser_usb_leaf_bittiming_const,
+	.bittiming_const = &kvaser_usb_flexc_bittiming_const,
 };
 
-static const struct kvaser_usb_dev_cfg kvaser_usb_leaf_dev_cfg_32mhz = {
+static const struct kvaser_usb_dev_cfg kvaser_usb_leaf_imx_dev_cfg_32mhz = {
 	.clock = {
 		.freq = 32 * MEGA /* Hz */,
 	},
 	.timestamp_freq = 1,
-	.bittiming_const = &kvaser_usb_leaf_bittiming_const,
+	.bittiming_const = &kvaser_usb_flexc_bittiming_const,
 };
 
 static void *
@@ -528,17 +538,17 @@ static void kvaser_usb_leaf_get_software_info_leaf(struct kvaser_usb *dev,
 		/* Firmware expects bittiming parameters calculated for 16MHz
 		 * clock, regardless of the actual clock
 		 */
-		dev->cfg = &kvaser_usb_leaf_dev_cfg_16mhz;
+		dev->cfg = &kvaser_usb_leaf_m32c_dev_cfg;
 	} else {
 		switch (sw_options & KVASER_USB_LEAF_SWOPTION_FREQ_MASK) {
 		case KVASER_USB_LEAF_SWOPTION_FREQ_16_MHZ_CLK:
-			dev->cfg = &kvaser_usb_leaf_dev_cfg_16mhz;
+			dev->cfg = &kvaser_usb_leaf_imx_dev_cfg_16mhz;
 			break;
 		case KVASER_USB_LEAF_SWOPTION_FREQ_24_MHZ_CLK:
-			dev->cfg = &kvaser_usb_leaf_dev_cfg_24mhz;
+			dev->cfg = &kvaser_usb_leaf_imx_dev_cfg_24mhz;
 			break;
 		case KVASER_USB_LEAF_SWOPTION_FREQ_32_MHZ_CLK:
-			dev->cfg = &kvaser_usb_leaf_dev_cfg_32mhz;
+			dev->cfg = &kvaser_usb_leaf_imx_dev_cfg_32mhz;
 			break;
 		}
 	}
@@ -565,7 +575,7 @@ static int kvaser_usb_leaf_get_software_info_inner(struct kvaser_usb *dev)
 		dev->fw_version = le32_to_cpu(cmd.u.usbcan.softinfo.fw_version);
 		dev->max_tx_urbs =
 			le16_to_cpu(cmd.u.usbcan.softinfo.max_outstanding_tx);
-		dev->cfg = &kvaser_usb_leaf_dev_cfg_8mhz;
+		dev->cfg = &kvaser_usb_leaf_usbcan_dev_cfg;
 		break;
 	}
 
-- 
2.36.1

Re: [PATCH 1/2] can: kvaser_usb: kvaser_usb_leaf: Fix CAN clock frequency regression

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3473 bytes --]

On 02.06.2022 08:30:30, Jimmy Assarsson wrote:
> The firmware of M32C based Leaf devices expects bittiming parameters
> calculated for 16MHz clock.
> Since we use the actual clock frequency of the device, the device may end
> up with wrong bittiming parameters, depending on user requested parameters.
> 
> This regression affects M32C based Leaf devices with non-16MHz clock.

Oh. Thanks for the patch!

> Fixes: fb12797ab1fe ("can: kvaser_usb: get CAN clock frequency from device")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
> ---
>  drivers/net/can/usb/kvaser_usb/kvaser_usb.h   |  4 +++
>  .../net/can/usb/kvaser_usb/kvaser_usb_core.c  | 20 +++++++++++----
>  .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c  | 25 ++++++++++++-------
>  3 files changed, 35 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb.h b/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
> index 3a49257f9fa6..cb588228d7a1 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
> @@ -44,6 +44,9 @@
>  #define KVASER_USB_CAP_EXT_CAP			0x02
>  #define KVASER_USB_HYDRA_CAP_EXT_CMD		0x04
>  
> +/* Quriks */
      ^^^^^^
Typo

> +#define KVASER_USB_QUIRK_IGNORE_CLK_FREQ BIT(0)
> +
>  struct kvaser_usb_dev_cfg;
>  
>  enum kvaser_usb_leaf_family {
> @@ -65,6 +68,7 @@ struct kvaser_usb_dev_card_data_hydra {
>  struct kvaser_usb_dev_card_data {
>  	u32 ctrlmode_supported;
>  	u32 capabilities;
> +	u32 quirks;
>  	union {
>  		struct {
>  			enum kvaser_usb_leaf_family family;
> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
> index e67658b53d02..5880e9719c9d 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
> @@ -94,10 +94,14 @@
>  
>  static inline bool kvaser_is_leaf(const struct usb_device_id *id)
>  {
> -	return (id->idProduct >= USB_LEAF_DEVEL_PRODUCT_ID &&
> -		id->idProduct <= USB_CAN_R_PRODUCT_ID) ||
> -		(id->idProduct >= USB_LEAF_LITE_V2_PRODUCT_ID &&
> -		 id->idProduct <= USB_LEAF_PRODUCT_ID_END);
> +	return id->idProduct >= USB_LEAF_DEVEL_PRODUCT_ID &&
> +	       id->idProduct <= USB_CAN_R_PRODUCT_ID;
> +}
> +
> +static inline bool kvaser_is_leafimx(const struct usb_device_id *id)
> +{
> +	return id->idProduct >= USB_LEAF_LITE_V2_PRODUCT_ID &&
> +	       id->idProduct <= USB_LEAF_PRODUCT_ID_END;
>  }

Is this getting a bit complicated now?
In this driver we have:

1) struct usb_device_id::driver_info
2) kvaser_is_*()

which is used to set

3) dev->card_data.leaf.family
4) dev->ops

and now you're adding:

5) dev->card_data.quirks

which then affects

6) dev->cfg

The straight forward way would be to define a struct that describes the
a device completely:

struct kvaser_driver_info {
       u32 quirks;        /* KVASER_USB_HAS_ */
       enum kvaser_usb_leaf_family;
       const struct kvaser_usb_dev_*ops;
       const struct kvaser_usb_dev_*cfg;
};

and then assign that to every device listed in the kvaser_usb_table.

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: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] can: kvaser_usb: kvaser_usb_leaf: Fix CAN clock frequency regression

[Jimmy Assarsson]

On 6/2/22 10:02, Marc Kleine-Budde wrote:
> On 02.06.2022 08:30:30, Jimmy Assarsson wrote:
>> The firmware of M32C based Leaf devices expects bittiming parameters
>> calculated for 16MHz clock.
>> Since we use the actual clock frequency of the device, the device may end
>> up with wrong bittiming parameters, depending on user requested parameters.
>>
>> This regression affects M32C based Leaf devices with non-16MHz clock.
> 
> Oh. Thanks for the patch!
> 
>> Fixes: fb12797ab1fe ("can: kvaser_usb: get CAN clock frequency from device")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
>> ---
>>   drivers/net/can/usb/kvaser_usb/kvaser_usb.h   |  4 +++
>>   .../net/can/usb/kvaser_usb/kvaser_usb_core.c  | 20 +++++++++++----
>>   .../net/can/usb/kvaser_usb/kvaser_usb_leaf.c  | 25 ++++++++++++-------
>>   3 files changed, 35 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb.h b/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
>> index 3a49257f9fa6..cb588228d7a1 100644
>> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
>> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb.h
>> @@ -44,6 +44,9 @@
>>   #define KVASER_USB_CAP_EXT_CAP			0x02
>>   #define KVASER_USB_HYDRA_CAP_EXT_CMD		0x04
>>   
>> +/* Quriks */
>        ^^^^^^
> Typo

Oops!

>> +#define KVASER_USB_QUIRK_IGNORE_CLK_FREQ BIT(0)
>> +
>>   struct kvaser_usb_dev_cfg;
>>   
>>   enum kvaser_usb_leaf_family {
>> @@ -65,6 +68,7 @@ struct kvaser_usb_dev_card_data_hydra {
>>   struct kvaser_usb_dev_card_data {
>>   	u32 ctrlmode_supported;
>>   	u32 capabilities;
>> +	u32 quirks;
>>   	union {
>>   		struct {
>>   			enum kvaser_usb_leaf_family family;
>> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
>> index e67658b53d02..5880e9719c9d 100644
>> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
>> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
>> @@ -94,10 +94,14 @@
>>   
>>   static inline bool kvaser_is_leaf(const struct usb_device_id *id)
>>   {
>> -	return (id->idProduct >= USB_LEAF_DEVEL_PRODUCT_ID &&
>> -		id->idProduct <= USB_CAN_R_PRODUCT_ID) ||
>> -		(id->idProduct >= USB_LEAF_LITE_V2_PRODUCT_ID &&
>> -		 id->idProduct <= USB_LEAF_PRODUCT_ID_END);
>> +	return id->idProduct >= USB_LEAF_DEVEL_PRODUCT_ID &&
>> +	       id->idProduct <= USB_CAN_R_PRODUCT_ID;
>> +}
>> +
>> +static inline bool kvaser_is_leafimx(const struct usb_device_id *id)
>> +{
>> +	return id->idProduct >= USB_LEAF_LITE_V2_PRODUCT_ID &&
>> +	       id->idProduct <= USB_LEAF_PRODUCT_ID_END;
>>   }
> 
> Is this getting a bit complicated now?
> In this driver we have:
> 
> 1) struct usb_device_id::driver_info
> 2) kvaser_is_*()
> 
> which is used to set
> 
> 3) dev->card_data.leaf.family
> 4) dev->ops
> 
> and now you're adding:
> 
> 5) dev->card_data.quirks
> 
> which then affects
> 
> 6) dev->cfg
> 
> The straight forward way would be to define a struct that describes the
> a device completely:
> 
> struct kvaser_driver_info {
>         u32 quirks;        /* KVASER_USB_HAS_ */
>         enum kvaser_usb_leaf_family;
>         const struct kvaser_usb_dev_*ops;
>         const struct kvaser_usb_dev_*cfg;
> };
> 
> and then assign that to every device listed in the kvaser_usb_table.

Thanks for the feedback!
I agree, but I prefer if we can keep assigning dev->cfg based on the 
information that we get from the device.
So we get:
struct kvaser_driver_info {
         u32 quirks;        /* KVASER_USB_HAS_ */
         enum kvaser_usb_leaf_family;
         const struct kvaser_usb_dev_*ops;
};
And quirks and family still affect dev->cfg.

Do you think this is OK?

Best regards,
jimmy

> Marc 

Re: [PATCH 1/2] can: kvaser_usb: kvaser_usb_leaf: Fix CAN clock frequency regression

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2659 bytes --]

On 02.06.2022 11:22:31, Jimmy Assarsson wrote:
> > > diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
> > > index e67658b53d02..5880e9719c9d 100644
> > > --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
> > > +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
> > > @@ -94,10 +94,14 @@
> > >   static inline bool kvaser_is_leaf(const struct usb_device_id *id)
> > >   {
> > > -	return (id->idProduct >= USB_LEAF_DEVEL_PRODUCT_ID &&
> > > -		id->idProduct <= USB_CAN_R_PRODUCT_ID) ||
> > > -		(id->idProduct >= USB_LEAF_LITE_V2_PRODUCT_ID &&
> > > -		 id->idProduct <= USB_LEAF_PRODUCT_ID_END);
> > > +	return id->idProduct >= USB_LEAF_DEVEL_PRODUCT_ID &&
> > > +	       id->idProduct <= USB_CAN_R_PRODUCT_ID;
> > > +}
> > > +
> > > +static inline bool kvaser_is_leafimx(const struct usb_device_id *id)
> > > +{
> > > +	return id->idProduct >= USB_LEAF_LITE_V2_PRODUCT_ID &&
> > > +	       id->idProduct <= USB_LEAF_PRODUCT_ID_END;
> > >   }
> > 
> > Is this getting a bit complicated now?
> > In this driver we have:
> > 
> > 1) struct usb_device_id::driver_info
> > 2) kvaser_is_*()
> > 
> > which is used to set
> > 
> > 3) dev->card_data.leaf.family
> > 4) dev->ops
> > 
> > and now you're adding:
> > 
> > 5) dev->card_data.quirks
> > 
> > which then affects
> > 
> > 6) dev->cfg
> > 
> > The straight forward way would be to define a struct that describes the
> > a device completely:
> > 
> > struct kvaser_driver_info {
> >         u32 quirks;        /* KVASER_USB_HAS_ */
> >         enum kvaser_usb_leaf_family;
> >         const struct kvaser_usb_dev_*ops;
> >         const struct kvaser_usb_dev_*cfg;
> > };
> > 
> > and then assign that to every device listed in the kvaser_usb_table.
> 
> Thanks for the feedback!
> I agree, but I prefer if we can keep assigning dev->cfg based on the
> information that we get from the device.

Ok, if you cannot tell from the USB product ID.

> So we get:
> struct kvaser_driver_info {
>         u32 quirks;        /* KVASER_USB_HAS_ */

That holds the existing quirks and the new one.

>         enum kvaser_usb_leaf_family;
>         const struct kvaser_usb_dev_*ops;
> };
> And quirks and family still affect dev->cfg.

...as is depends on sw_options read from the device?

regards,
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] can: kvaser_usb: kvaser_usb_leaf: Fix CAN clock frequency regression

[Jimmy Assarsson]

On 6/2/22 11:54, Marc Kleine-Budde wrote:
> On 02.06.2022 11:22:31, Jimmy Assarsson wrote:
>>>> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
>>>> index e67658b53d02..5880e9719c9d 100644
>>>> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
>>>> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
>>>> @@ -94,10 +94,14 @@
>>>>    static inline bool kvaser_is_leaf(const struct usb_device_id *id)
>>>>    {
>>>> -	return (id->idProduct >= USB_LEAF_DEVEL_PRODUCT_ID &&
>>>> -		id->idProduct <= USB_CAN_R_PRODUCT_ID) ||
>>>> -		(id->idProduct >= USB_LEAF_LITE_V2_PRODUCT_ID &&
>>>> -		 id->idProduct <= USB_LEAF_PRODUCT_ID_END);
>>>> +	return id->idProduct >= USB_LEAF_DEVEL_PRODUCT_ID &&
>>>> +	       id->idProduct <= USB_CAN_R_PRODUCT_ID;
>>>> +}
>>>> +
>>>> +static inline bool kvaser_is_leafimx(const struct usb_device_id *id)
>>>> +{
>>>> +	return id->idProduct >= USB_LEAF_LITE_V2_PRODUCT_ID &&
>>>> +	       id->idProduct <= USB_LEAF_PRODUCT_ID_END;
>>>>    }
>>>
>>> Is this getting a bit complicated now?
>>> In this driver we have:
>>>
>>> 1) struct usb_device_id::driver_info
>>> 2) kvaser_is_*()
>>>
>>> which is used to set
>>>
>>> 3) dev->card_data.leaf.family
>>> 4) dev->ops
>>>
>>> and now you're adding:
>>>
>>> 5) dev->card_data.quirks
>>>
>>> which then affects
>>>
>>> 6) dev->cfg
>>>
>>> The straight forward way would be to define a struct that describes the
>>> a device completely:
>>>
>>> struct kvaser_driver_info {
>>>          u32 quirks;        /* KVASER_USB_HAS_ */
>>>          enum kvaser_usb_leaf_family;
>>>          const struct kvaser_usb_dev_*ops;
>>>          const struct kvaser_usb_dev_*cfg;
>>> };
>>>
>>> and then assign that to every device listed in the kvaser_usb_table.
>>
>> Thanks for the feedback!
>> I agree, but I prefer if we can keep assigning dev->cfg based on the
>> information that we get from the device.
> 
> Ok, if you cannot tell from the USB product ID.

It should be possible, but it will eliminate the risk of me setting
wrong cfg. I don't got access to all the different devices, especially
not the old ones.

>> So we get:
>> struct kvaser_driver_info {
>>          u32 quirks;        /* KVASER_USB_HAS_ */
> 
> That holds the existing quirks and the new one.

Yep!

>>          enum kvaser_usb_leaf_family;
>>          const struct kvaser_usb_dev_*ops;
>> };
>> And quirks and family still affect dev->cfg.
> 
> ...as is depends on sw_options read from the device?

Correct.

Best regards,
jimmy