[PATCH v2 0/5] Introducing new CAN FD bittiming parameters: Transmission Delay Compensation (TDC)

[PATCH v2 0/5] Introducing new CAN FD bittiming parameters: Transmission Delay Compensation (TDC)

[Vincent Mailhol]

At high bit rates, the propagation delay from the TX pin to the RX pin
of the transceiver causes measurement errors and needs to be
corrected.

To solve this issue, ISO 11898-1 introduces in section 11.3.3
"Transmitter delay compensation" (here after TDC) a SSP (Secondary
Sample Point) equal to the distance, in time quanta, from the start of
the bit time on the TX pin to the actual measurement on the RX pin.

This patch series implements the TDC parameters which allow the
controller to calculate that SSP.

The first patch introduces the new structures, the second one saves
eight bytes on struct can_priv, the third fixes some existing
checkpatch warnings in preparation on the next one, the fourth one
implements the netlink interface and, finally, the fifth one adds the
calculation.

This is a follow-up on the RFC is sent already more than one month
ago.

Ref: https://lore.kernel.org/linux-can/CAMZ6RqLtg1ynVeePLLriUw0+KLbTpPJHapTEanv1_EZYJSrK=g@mail.gmail.com/T/#u

You might also be interested in below patch which implement those
changes in iproute command line:
https://lore.kernel.org/linux-can/20210223181714.219655-1-mailhol.vincent@wanadoo.fr/T/#t
Because this v2 introduced no changes on the netlink interface, the
iproute patch will *not* be resend.

Changes from v1:
  - Changed the commit message of the first patch of the serie to
    clarify the condition of when TDC is active.
  - Changed the alignment style from tabulations to single space in
    drivers/net/can/dev/netlink.c
  - Remove duplicated [IFLA_CAN_TERMINATION] entry in
    drivers/net/can/dev/netlink.c

Changes from the RFC:
  - The RFC was a single e-mail with all comments in bulk, made this a
    proper patch series.
  - Added the netlink interface.
  - Rename the function can_set_tdc to can_calc_tdco (the formula is
    the same, everything around it was reworked).
  - Other small miscellaneous changes.

TODO: the documentation will be updated after the netlink interface
gets approved.


Yours sincerely,
Vincent

Vincent Mailhol (5):
  can: add new CAN FD bittiming parameters: Transmitter Delay
    Compensation (TDC)
  can: dev: reorder struct can_priv members for better packing
  can: netlink: move '=' operators back to previous line (checkpatch
    fix)
  can: add netlink interface for CAN-FD Transmitter Delay Compensation
    (TDC)
  can: bittiming: add calculation for CAN FD Transmitter Delay
    Compensation (TDC)

 drivers/net/can/dev/bittiming.c  |  23 +++++++
 drivers/net/can/dev/netlink.c    | 100 ++++++++++++++++++++++++++-----
 include/linux/can/bittiming.h    |  71 ++++++++++++++++++++++
 include/linux/can/dev.h          |  14 +++--
 include/uapi/linux/can/netlink.h |   6 ++
 5 files changed, 194 insertions(+), 20 deletions(-)


base-commit: 1b34628ac60360ef7455c00f01dc62f82ed08d8c
-- 
2.26.2

[PATCH v2 1/5] can: add new CAN FD bittiming parameters: Transmitter Delay Compensation (TDC)

[Vincent Mailhol]

At high bit rates, the propagation delay from the TX pin to the RX pin
of the transceiver causes measurement errors: the sample point on the
RX pin might occur on the previous bit.

This issue is addressed in ISO 11898-1 section 11.3.3 "Transmitter
delay compensation" (TDC).

This patch adds two new structures: can_tdc and can_tdc_const in order
to implement this TDC.

The structures are then added to can_priv.

A controller supports TDC if an only if can_priv::tdc_const is not
NULL.

TDC is active if and only if:
  - fd flag is on
  - can_priv::tdc.tdco is not zero.
It is the driver responsibility to check those two conditions are met.

No new controller modes are introduced (i.e. no CAN_CTRL_MODE_TDC) in
order not to be redundant with above logic.

The names of the parameters are chosen to match existing CAN
controllers specification. References:
  - Bosch C_CAN FD8:
https://www.bosch-semiconductors.com/media/ip_modules/pdf_2/c_can_fd8/users_manual_c_can_fd8_r210_1.pdf
  - Microchip CAN FD Controller Module:
http://ww1.microchip.com/downloads/en/DeviceDoc/MCP251XXFD-CAN-FD-Controller-Module-Family-Reference-Manual-20005678B.pdf
  - SAM E701/S70/V70/V71 Family:
https://www.mouser.com/datasheet/2/268/60001527A-1284321.pdf

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 include/linux/can/bittiming.h | 65 +++++++++++++++++++++++++++++++++++
 include/linux/can/dev.h       |  3 ++
 2 files changed, 68 insertions(+)

diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h
index 707575c668f4..91bf9f8926a7 100644
--- a/include/linux/can/bittiming.h
+++ b/include/linux/can/bittiming.h
@@ -1,5 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
 /* Copyright (c) 2020 Pengutronix, Marc Kleine-Budde <kernel@pengutronix.de>
+ * Copyright (c) 2021 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
  */
 
 #ifndef _CAN_BITTIMING_H
@@ -10,6 +11,70 @@
 
 #define CAN_SYNC_SEG 1
 
+/*
+ * struct can_tdc - CAN FD Transmission Delay Compensation parameters
+ *
+ * At high bit rates, the propagation delay from the TX pin to the RX
+ * pin of the transceiver causes measurement errors: the sample point
+ * on the RX pin might occur on the previous bit.
+ *
+ * To solve this issue, ISO 11898-1 introduces in section 11.3.3
+ * "Transmitter delay compensation" a SSP (Secondary Sample Point)
+ * equal to the distance, in time quanta, from the start of the bit
+ * time on the TX pin to the actual measurement on the RX pin.
+ *
+ * This structure contains the parameters to calculate that SSP.
+ *
+ * @tdcv: Transmitter Delay Compensation Value. Distance, in time
+ *	quanta, from when the bit is sent on the TX pin to when it is
+ *	received on the RX pin of the transmitter. Possible options:
+ *
+ *	  O: automatic mode. The controller dynamically measure @tdcv
+ *	  for each transmited CAN FD frame.
+ *
+ *	  Other values: manual mode. Use the fixed provided value.
+ *
+ * @tdco: Transmitter Delay Compensation Offset. Offset value, in time
+ *	quanta, defining the distance between the start of the bit
+ *	reception on the RX pin of the transceiver and the SSP
+ *	position such as SSP = @tdcv + @tdco.
+ *
+ *	If @tdco is zero, then TDC is disabled and both @tdcv and
+ *	@tdcf should be ignored.
+ *
+ * @tdcf: Transmitter Delay Compensation Filter window. Defines the
+ *	minimum value for the SSP position in time quanta. If SSP is
+ *	less than @tdcf, then no delay compensations occur and the
+ *	normal sampling point is used instead. The feature is enabled
+ *	if and only if @tdcv is set to zero (automatic mode) and @tdcf
+ *	is configured to a value greater than @tdco.
+ */
+struct can_tdc {
+	u32 tdcv;
+	u32 tdco;
+	u32 tdcf;
+};
+
+/*
+ * struct can_tdc_const - CAN hardware-dependent constant for
+ *	Transmission Delay Compensation
+ *
+ * @tdcv_max: Transmitter Delay Compensation Value maximum value.
+ *	Should be set to zero if the controller does not support
+ *	manual mode for tdcv.
+ * @tdco_max: Transmitter Delay Compensation Offset maximum value.
+ *	Should not be zero. If the controller does not support TDC,
+ *	then the pointer to this structure should be NULL.
+ * @tdcf_max: Transmitter Delay Compensation Filter window maximum
+ *	value. Should be set to zero if the controller does not
+ *	support this feature.
+ */
+struct can_tdc_const {
+	u32 tdcv_max;
+	u32 tdco_max;
+	u32 tdcf_max;
+};
+
 #ifdef CONFIG_CAN_CALC_BITTIMING
 int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
 		       const struct can_bittiming_const *btc);
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index ac4d83a1ab81..4795da0eb949 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -42,6 +42,9 @@ struct can_priv {
 	struct can_bittiming bittiming, data_bittiming;
 	const struct can_bittiming_const *bittiming_const,
 		*data_bittiming_const;
+	struct can_tdc tdc;
+	const struct can_tdc_const *tdc_const;
+
 	const u16 *termination_const;
 	unsigned int termination_const_cnt;
 	u16 termination;
-- 
2.26.2

[PATCH v2 2/5] can: dev: reorder struct can_priv members for better packing

[Vincent Mailhol]

Save eight bytes of holes on x86-64 architectures by reordering struct
can_priv members.

Before:

$ pahole -C can_priv drivers/net/can/dev/dev.o
struct can_priv {
	struct net_device *        dev;                  /*     0     8 */
	struct can_device_stats    can_stats;            /*     8    24 */
	struct can_bittiming       bittiming;            /*    32    32 */
	/* --- cacheline 1 boundary (64 bytes) --- */
	struct can_bittiming       data_bittiming;       /*    64    32 */
	const struct can_bittiming_const  * bittiming_const; /*    96     8 */
	const struct can_bittiming_const  * data_bittiming_const; /*   104     8 */
	struct can_tdc             tdc;                  /*   112    12 */

	/* XXX 4 bytes hole, try to pack */

	/* --- cacheline 2 boundary (128 bytes) --- */
	const struct can_tdc_const  * tdc_const;         /*   128     8 */
	const u16  *               termination_const;    /*   136     8 */
	unsigned int               termination_const_cnt; /*   144     4 */
	u16                        termination;          /*   148     2 */

	/* XXX 2 bytes hole, try to pack */

	const u32  *               bitrate_const;        /*   152     8 */
	unsigned int               bitrate_const_cnt;    /*   160     4 */

	/* XXX 4 bytes hole, try to pack */

	const u32  *               data_bitrate_const;   /*   168     8 */
	unsigned int               data_bitrate_const_cnt; /*   176     4 */
	u32                        bitrate_max;          /*   180     4 */
	struct can_clock           clock;                /*   184     4 */
	enum can_state             state;                /*   188     4 */
	/* --- cacheline 3 boundary (192 bytes) --- */
	u32                        ctrlmode;             /*   192     4 */
	u32                        ctrlmode_supported;   /*   196     4 */
	u32                        ctrlmode_static;      /*   200     4 */
	int                        restart_ms;           /*   204     4 */
	struct delayed_work        restart_work;         /*   208   168 */

	/* XXX last struct has 4 bytes of padding */

	/* --- cacheline 5 boundary (320 bytes) was 56 bytes ago --- */
	int                        (*do_set_bittiming)(struct net_device *); /*   376     8 */
	/* --- cacheline 6 boundary (384 bytes) --- */
	int                        (*do_set_data_bittiming)(struct net_device *); /*   384     8 */
	int                        (*do_set_mode)(struct net_device *, enum can_mode); /*   392     8 */
	int                        (*do_set_termination)(struct net_device *, u16); /*   400     8 */
	int                        (*do_get_state)(const struct net_device  *, enum can_state *); /*   408     8 */
	int                        (*do_get_berr_counter)(const struct net_device  *, struct can_berr_counter *); /*   416     8 */
	unsigned int               echo_skb_max;         /*   424     4 */

	/* XXX 4 bytes hole, try to pack */

	struct sk_buff * *         echo_skb;             /*   432     8 */

	/* size: 440, cachelines: 7, members: 31 */
	/* sum members: 426, holes: 4, sum holes: 14 */
	/* paddings: 1, sum paddings: 4 */
	/* last cacheline: 56 bytes */
};

After:

$ pahole -C can_priv drivers/net/can/dev/dev.o
struct can_priv {
	struct net_device *        dev;                  /*     0     8 */
	struct can_device_stats    can_stats;            /*     8    24 */
	const struct can_bittiming_const  * bittiming_const; /*    32     8 */
	const struct can_bittiming_const  * data_bittiming_const; /*    40     8 */
	struct can_bittiming       bittiming;            /*    48    32 */
	/* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */
	struct can_bittiming       data_bittiming;       /*    80    32 */
	const struct can_tdc_const  * tdc_const;         /*   112     8 */
	struct can_tdc             tdc;                  /*   120    12 */
	/* --- cacheline 2 boundary (128 bytes) was 4 bytes ago --- */
	unsigned int               bitrate_const_cnt;    /*   132     4 */
	const u32  *               bitrate_const;        /*   136     8 */
	const u32  *               data_bitrate_const;   /*   144     8 */
	unsigned int               data_bitrate_const_cnt; /*   152     4 */
	u32                        bitrate_max;          /*   156     4 */
	struct can_clock           clock;                /*   160     4 */
	unsigned int               termination_const_cnt; /*   164     4 */
	const u16  *               termination_const;    /*   168     8 */
	u16                        termination;          /*   176     2 */

	/* XXX 2 bytes hole, try to pack */

	enum can_state             state;                /*   180     4 */
	u32                        ctrlmode;             /*   184     4 */
	u32                        ctrlmode_supported;   /*   188     4 */
	/* --- cacheline 3 boundary (192 bytes) --- */
	u32                        ctrlmode_static;      /*   192     4 */
	int                        restart_ms;           /*   196     4 */
	struct delayed_work        restart_work;         /*   200   168 */

	/* XXX last struct has 4 bytes of padding */

	/* --- cacheline 5 boundary (320 bytes) was 48 bytes ago --- */
	int                        (*do_set_bittiming)(struct net_device *); /*   368     8 */
	int                        (*do_set_data_bittiming)(struct net_device *); /*   376     8 */
	/* --- cacheline 6 boundary (384 bytes) --- */
	int                        (*do_set_mode)(struct net_device *, enum can_mode); /*   384     8 */
	int                        (*do_set_termination)(struct net_device *, u16); /*   392     8 */
	int                        (*do_get_state)(const struct net_device  *, enum can_state *); /*   400     8 */
	int                        (*do_get_berr_counter)(const struct net_device  *, struct can_berr_counter *); /*   408     8 */
	unsigned int               echo_skb_max;         /*   416     4 */

	/* XXX 4 bytes hole, try to pack */

	struct sk_buff * *         echo_skb;             /*   424     8 */

	/* size: 432, cachelines: 7, members: 31 */
	/* sum members: 426, holes: 2, sum holes: 6 */
	/* paddings: 1, sum paddings: 4 */
	/* last cacheline: 48 bytes */
};

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 include/linux/can/dev.h | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 4795da0eb949..27b275e463da 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -39,22 +39,23 @@ struct can_priv {
 	struct net_device *dev;
 	struct can_device_stats can_stats;
 
-	struct can_bittiming bittiming, data_bittiming;
 	const struct can_bittiming_const *bittiming_const,
 		*data_bittiming_const;
-	struct can_tdc tdc;
+	struct can_bittiming bittiming, data_bittiming;
 	const struct can_tdc_const *tdc_const;
+	struct can_tdc tdc;
 
-	const u16 *termination_const;
-	unsigned int termination_const_cnt;
-	u16 termination;
-	const u32 *bitrate_const;
 	unsigned int bitrate_const_cnt;
+	const u32 *bitrate_const;
 	const u32 *data_bitrate_const;
 	unsigned int data_bitrate_const_cnt;
 	u32 bitrate_max;
 	struct can_clock clock;
 
+	unsigned int termination_const_cnt;
+	const u16 *termination_const;
+	u16 termination;
+
 	enum can_state state;
 
 	/* CAN controller features - see include/uapi/linux/can/netlink.h */
-- 
2.26.2

[PATCH v2 3/5] can: netlink: move '=' operators back to previous line (checkpatch fix)

[Vincent Mailhol]

Fix the warning triggered by having an '=' at the beginning of the
line by moving it back to the previous line. Also replace all
indentations with a single space so that future entries can be more
easily added.

Extract of ./scripts/checkpatch.pl -f drivers/net/can/dev/netlink.c:

CHECK: Assignment operator '=' should be on the previous line
+       [IFLA_CAN_BITTIMING_CONST]
+                               = { .len = sizeof(struct can_bittiming_const) },

CHECK: Assignment operator '=' should be on the previous line
+       [IFLA_CAN_DATA_BITTIMING]
+                               = { .len = sizeof(struct can_bittiming) },

CHECK: Assignment operator '=' should be on the previous line
+       [IFLA_CAN_DATA_BITTIMING_CONST]
+                               = { .len = sizeof(struct can_bittiming_const) },

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/dev/netlink.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 867f6be31230..c19eef775ec8 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -8,20 +8,17 @@
 #include <net/rtnetlink.h>
 
 static const struct nla_policy can_policy[IFLA_CAN_MAX + 1] = {
-	[IFLA_CAN_STATE]	= { .type = NLA_U32 },
-	[IFLA_CAN_CTRLMODE]	= { .len = sizeof(struct can_ctrlmode) },
-	[IFLA_CAN_RESTART_MS]	= { .type = NLA_U32 },
-	[IFLA_CAN_RESTART]	= { .type = NLA_U32 },
-	[IFLA_CAN_BITTIMING]	= { .len = sizeof(struct can_bittiming) },
-	[IFLA_CAN_BITTIMING_CONST]
-				= { .len = sizeof(struct can_bittiming_const) },
-	[IFLA_CAN_CLOCK]	= { .len = sizeof(struct can_clock) },
-	[IFLA_CAN_BERR_COUNTER]	= { .len = sizeof(struct can_berr_counter) },
-	[IFLA_CAN_DATA_BITTIMING]
-				= { .len = sizeof(struct can_bittiming) },
-	[IFLA_CAN_DATA_BITTIMING_CONST]
-				= { .len = sizeof(struct can_bittiming_const) },
-	[IFLA_CAN_TERMINATION]	= { .type = NLA_U16 },
+	[IFLA_CAN_STATE] = { .type = NLA_U32 },
+	[IFLA_CAN_CTRLMODE] = { .len = sizeof(struct can_ctrlmode) },
+	[IFLA_CAN_RESTART_MS] = { .type = NLA_U32 },
+	[IFLA_CAN_RESTART] = { .type = NLA_U32 },
+	[IFLA_CAN_BITTIMING] = { .len = sizeof(struct can_bittiming) },
+	[IFLA_CAN_BITTIMING_CONST] = { .len = sizeof(struct can_bittiming_const) },
+	[IFLA_CAN_CLOCK] = { .len = sizeof(struct can_clock) },
+	[IFLA_CAN_BERR_COUNTER] = { .len = sizeof(struct can_berr_counter) },
+	[IFLA_CAN_DATA_BITTIMING] = { .len = sizeof(struct can_bittiming) },
+	[IFLA_CAN_DATA_BITTIMING_CONST]	= { .len = sizeof(struct can_bittiming_const) },
+	[IFLA_CAN_TERMINATION] = { .type = NLA_U16 },
 };
 
 static int can_validate(struct nlattr *tb[], struct nlattr *data[],
-- 
2.26.2

[PATCH v2 4/5] can: add netlink interface for CAN-FD Transmitter Delay Compensation (TDC)

[Vincent Mailhol]

Add the netlink interface for TDC parameters of struct can_tdc and
can_tdc_const.

Contrary to the can_bittiming(_const) structures for which there is
just a single IFLA_CAN(_DATA)_BITTMING(_CONST) entry per structure,
here, an IFLA_CAN_TDC* entry is added for each of the TDC parameters
of the newly introduced struct can_tdc and struct can_tdc_const.

For struct can_tdc, these are:
	IFLA_CAN_TDCV
	IFLA_CAN_TDCO
	IFLA_CAN_TDCF

For struct can_tdc_const, these are:
	IFLA_CAN_TDCV_MAX_CONST
	IFLA_CAN_TDCO_MAX_CONST
	IFLA_CAN_TDCF_MAX_CONST

This is done so that changes can be applied in the future to the
structures without breaking the netlink interface.

All the new parameters are defined as u32. This arbitrary choice is
done to mimic the other bittiming values with are also all of type
u32. An u16 would have been sufficient to hold the TDC values.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/dev/netlink.c    | 74 +++++++++++++++++++++++++++++++-
 include/uapi/linux/can/netlink.h |  6 +++
 2 files changed, 79 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index c19eef775ec8..c3f75c09d6c8 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -19,6 +19,12 @@ static const struct nla_policy can_policy[IFLA_CAN_MAX + 1] = {
 	[IFLA_CAN_DATA_BITTIMING] = { .len = sizeof(struct can_bittiming) },
 	[IFLA_CAN_DATA_BITTIMING_CONST]	= { .len = sizeof(struct can_bittiming_const) },
 	[IFLA_CAN_TERMINATION] = { .type = NLA_U16 },
+	[IFLA_CAN_TDCV] = { .type = NLA_U32 },
+	[IFLA_CAN_TDCV_MAX_CONST] = { .type = NLA_U32 },
+	[IFLA_CAN_TDCO] = { .type = NLA_U32 },
+	[IFLA_CAN_TDCO_MAX_CONST] = { .type = NLA_U32 },
+	[IFLA_CAN_TDCF] = { .type = NLA_U32 },
+	[IFLA_CAN_TDCF_MAX_CONST] = { .type = NLA_U32 },
 };
 
 static int can_validate(struct nlattr *tb[], struct nlattr *data[],
@@ -218,6 +224,51 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 		priv->termination = termval;
 	}
 
+	if (data[IFLA_CAN_TDCV]) {
+		u32 tdcv = nla_get_u32(data[IFLA_CAN_TDCV]);
+
+		if (!priv->tdc_const || !priv->tdc_const->tdcv_max)
+			return -EOPNOTSUPP;
+
+		if (tdcv > priv->tdc_const->tdcv_max)
+			return -EINVAL;
+
+		if (dev->flags & IFF_UP)
+			return -EBUSY;
+
+		priv->tdc.tdcv = tdcv;
+	}
+
+	if (data[IFLA_CAN_TDCO]) {
+		u32 tdco = nla_get_u32(data[IFLA_CAN_TDCO]);
+
+		if (!priv->tdc_const || !priv->tdc_const->tdco_max)
+			return -EOPNOTSUPP;
+
+		if (tdco > priv->tdc_const->tdco_max)
+			return -EINVAL;
+
+		if (dev->flags & IFF_UP)
+			return -EBUSY;
+
+		priv->tdc.tdco = tdco;
+	}
+
+	if (data[IFLA_CAN_TDCF]) {
+		u32 tdcf = nla_get_u32(data[IFLA_CAN_TDCF]);
+
+		if (!priv->tdc_const || !priv->tdc_const->tdcf_max)
+			return -EOPNOTSUPP;
+
+		if (tdcf > priv->tdc_const->tdcf_max)
+			return -EINVAL;
+
+		if (dev->flags & IFF_UP)
+			return -EBUSY;
+
+		priv->tdc.tdcf = tdcf;
+	}
+
 	return 0;
 }
 
@@ -252,6 +303,16 @@ static size_t can_get_size(const struct net_device *dev)
 		size += nla_total_size(sizeof(*priv->data_bitrate_const) *
 				       priv->data_bitrate_const_cnt);
 	size += sizeof(priv->bitrate_max);			/* IFLA_CAN_BITRATE_MAX */
+	if (priv->tdc.tdco) {
+		size += nla_total_size(sizeof(u32));	/* IFLA_CAN_TDCV */
+		size += nla_total_size(sizeof(u32));	/* IFLA_CAN_TDCO */
+		size += nla_total_size(sizeof(u32));	/* IFLA_CAN_TDCF */
+	}
+	if (priv->tdc_const) {
+		size += nla_total_size(sizeof(u32));	/* IFLA_CAN_TDCV_MAX_CONST */
+		size += nla_total_size(sizeof(u32));	/* IFLA_CAN_TDCO_MAX_CONST */
+		size += nla_total_size(sizeof(u32));	/* IFLA_CAN_TDCF_MAX_CONST */
+	}
 
 	return size;
 }
@@ -313,7 +374,18 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
 
 	    (nla_put(skb, IFLA_CAN_BITRATE_MAX,
 		     sizeof(priv->bitrate_max),
-		     &priv->bitrate_max))
+		     &priv->bitrate_max)) ||
+
+	    (priv->tdc_const &&
+	     (nla_put_u32(skb, IFLA_CAN_TDCV, priv->tdc.tdcv) ||
+	      nla_put_u32(skb, IFLA_CAN_TDCO, priv->tdc.tdco) ||
+	      nla_put_u32(skb, IFLA_CAN_TDCF, priv->tdc.tdcf) ||
+	      nla_put_u32(skb, IFLA_CAN_TDCV_MAX_CONST,
+			  priv->tdc_const->tdcv_max) ||
+	      nla_put_u32(skb, IFLA_CAN_TDCO_MAX_CONST,
+			  priv->tdc_const->tdco_max) ||
+	      nla_put_u32(skb, IFLA_CAN_TDCF_MAX_CONST,
+			  priv->tdc_const->tdcf_max)))
 	    )
 
 		return -EMSGSIZE;
diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
index f730d443b918..e69c4b330ae6 100644
--- a/include/uapi/linux/can/netlink.h
+++ b/include/uapi/linux/can/netlink.h
@@ -134,6 +134,12 @@ enum {
 	IFLA_CAN_BITRATE_CONST,
 	IFLA_CAN_DATA_BITRATE_CONST,
 	IFLA_CAN_BITRATE_MAX,
+	IFLA_CAN_TDCV,
+	IFLA_CAN_TDCO,
+	IFLA_CAN_TDCF,
+	IFLA_CAN_TDCV_MAX_CONST,
+	IFLA_CAN_TDCO_MAX_CONST,
+	IFLA_CAN_TDCF_MAX_CONST,
 	__IFLA_CAN_MAX
 };
 
-- 
2.26.2

[PATCH v2 5/5] can: bittiming: add calculation for CAN FD Transmitter Delay Compensation (TDC)

[Vincent Mailhol]

The logic for the tdco calculation is to just reuse the normal sample
point: tdco = sp. Because the sample point is expressed in tenth of
percent and the tdco is expressed in time quanta, a conversion is
needed.

At the end,
     ssp = tdcv + tdco
         = tdcv + sp.

Another popular method is to set tdco to the middle of the bit:
     tdc->tdco = can_bit_time(dbt) / 2
During benchmark tests, we could not find a clear advantages for one
of the two methods.

The tdco calculation is triggered each time the data_bittiming is
changed so that users relying on automated calculation can use the
netlink interface the exact same way without need of new parameters.
For example, a command such as:
	ip link set canX type can bitrate 500000 dbitrate 4000000 fd on
would trigger the calculation.

The user using CONFIG_CAN_CALC_BITTIMING who does not want automated
calculation needs to manually set tdco to zero.
For example with:
	ip link set canX type can tdco 0 bitrate 500000 dbitrate 4000000 fd on
(if the tdco parameter is provided in a previous command, it will be
overwritten).

If tdcv is set to zero (default), it is automatically calculated by
the transiver for each frame. As such, there is no code in the kernel
to calculate it.

tdcf has no automated calculation functions because we could not
figure out a formula for this parameter.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/dev/bittiming.c | 23 +++++++++++++++++++++++
 drivers/net/can/dev/netlink.c   |  1 +
 include/linux/can/bittiming.h   |  6 ++++++
 3 files changed, 30 insertions(+)

diff --git a/drivers/net/can/dev/bittiming.c b/drivers/net/can/dev/bittiming.c
index f7fe226bb395..5d6c2f217210 100644
--- a/drivers/net/can/dev/bittiming.c
+++ b/drivers/net/can/dev/bittiming.c
@@ -174,6 +174,29 @@ int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
 
 	return 0;
 }
+
+void can_calc_tdco(struct net_device *dev)
+{
+	struct can_priv *priv = netdev_priv(dev);
+	const struct can_bittiming *dbt = &priv->data_bittiming;
+	struct can_tdc *tdc = &priv->tdc;
+	const struct can_tdc_const *tdc_const = priv->tdc_const;
+
+	if (!tdc_const)
+		return;
+
+	/* As specified in ISO 11898-1 section 11.3.3 "Transmitter
+	 * delay compensation" (TDC) is only applicable if data BRP is
+	 * one or two.
+	 */
+	if (dbt->brp == 1 || dbt->brp == 2) {
+		/* Reuse "normal" sample point and convert it to time quanta */
+		tdc->tdco = min(can_bit_time(dbt) * dbt->sample_point / 1000,
+				tdc_const->tdco_max);
+	} else {
+		tdc->tdco = 0;
+	}
+}
 #endif /* CONFIG_CAN_CALC_BITTIMING */
 
 /* Checks the validity of the specified bit-timing parameters prop_seg,
diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index c3f75c09d6c8..a3b71465849c 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -183,6 +183,7 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 					priv->data_bitrate_const_cnt);
 		if (err)
 			return err;
+		can_calc_tdco(dev);
 
 		if (priv->bitrate_max && dbt.bitrate > priv->bitrate_max) {
 			netdev_err(dev, "canfd data bitrate surpasses transceiver capabilities of %d bps\n",
diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h
index 91bf9f8926a7..8c042a025a7f 100644
--- a/include/linux/can/bittiming.h
+++ b/include/linux/can/bittiming.h
@@ -78,6 +78,8 @@ struct can_tdc_const {
 #ifdef CONFIG_CAN_CALC_BITTIMING
 int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
 		       const struct can_bittiming_const *btc);
+
+void can_calc_tdco(struct net_device *dev);
 #else /* !CONFIG_CAN_CALC_BITTIMING */
 static inline int
 can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
@@ -86,6 +88,10 @@ can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
 	netdev_err(dev, "bit-timing calculation not available\n");
 	return -EINVAL;
 }
+
+static inline void can_calc_tdco(struct net_device *dev)
+{
+}
 #endif /* CONFIG_CAN_CALC_BITTIMING */
 
 int can_get_bittiming(struct net_device *dev, struct can_bittiming *bt,
-- 
2.26.2

Re: [PATCH v2 1/5] can: add new CAN FD bittiming parameters: Transmitter Delay Compensation (TDC)

[Marc Kleine-Budde]

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

On 24.02.2021 09:20:04, Vincent Mailhol wrote:
> --- a/include/linux/can/bittiming.h
> +++ b/include/linux/can/bittiming.h
> @@ -1,5 +1,6 @@
>  /* SPDX-License-Identifier: GPL-2.0-only */
>  /* Copyright (c) 2020 Pengutronix, Marc Kleine-Budde <kernel@pengutronix.de>
> + * Copyright (c) 2021 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>   */
>  
>  #ifndef _CAN_BITTIMING_H
> @@ -10,6 +11,70 @@
>  
>  #define CAN_SYNC_SEG 1
>  
> +/*
> + * struct can_tdc - CAN FD Transmission Delay Compensation parameters
> + *
> + * At high bit rates, the propagation delay from the TX pin to the RX
> + * pin of the transceiver causes measurement errors: the sample point
> + * on the RX pin might occur on the previous bit.
> + *
> + * To solve this issue, ISO 11898-1 introduces in section 11.3.3
> + * "Transmitter delay compensation" a SSP (Secondary Sample Point)
> + * equal to the distance, in time quanta, from the start of the bit
> + * time on the TX pin to the actual measurement on the RX pin.
> + *
> + * This structure contains the parameters to calculate that SSP.
> + *
> + * @tdcv: Transmitter Delay Compensation Value. Distance, in time
> + *	quanta, from when the bit is sent on the TX pin to when it is
> + *	received on the RX pin of the transmitter. Possible options:
> + *
> + *	  O: automatic mode. The controller dynamically measure @tdcv
> + *	  for each transmited CAN FD frame.
                         ^^^
transmitted

Fixed while applying to linux-can-next/testing. As net-next ist still
closed, there is still some time to the next pull request and I'll
squash patches if needed.

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 v2 1/5] can: add new CAN FD bittiming parameters: Transmitter Delay Compensation (TDC)

[Jimmy Assarsson]

On 2021-02-24 08:31, Marc Kleine-Budde wrote:
> On 24.02.2021 09:20:04, Vincent Mailhol wrote:
>> --- a/include/linux/can/bittiming.h
>> +++ b/include/linux/can/bittiming.h
>> @@ -1,5 +1,6 @@
>>   /* SPDX-License-Identifier: GPL-2.0-only */
>>   /* Copyright (c) 2020 Pengutronix, Marc Kleine-Budde <kernel@pengutronix.de>
>> + * Copyright (c) 2021 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>>    */
>>   
>>   #ifndef _CAN_BITTIMING_H
>> @@ -10,6 +11,70 @@
>>   
>>   #define CAN_SYNC_SEG 1
>>   
>> +/*
>> + * struct can_tdc - CAN FD Transmission Delay Compensation parameters
>> + *
>> + * At high bit rates, the propagation delay from the TX pin to the RX
>> + * pin of the transceiver causes measurement errors: the sample point
>> + * on the RX pin might occur on the previous bit.
>> + *
>> + * To solve this issue, ISO 11898-1 introduces in section 11.3.3
>> + * "Transmitter delay compensation" a SSP (Secondary Sample Point)
>> + * equal to the distance, in time quanta, from the start of the bit
>> + * time on the TX pin to the actual measurement on the RX pin.
>> + *
>> + * This structure contains the parameters to calculate that SSP.
>> + *
>> + * @tdcv: Transmitter Delay Compensation Value. Distance, in time
>> + *	quanta, from when the bit is sent on the TX pin to when it is
>> + *	received on the RX pin of the transmitter. Possible options:
>> + *
>> + *	  O: automatic mode. The controller dynamically measure @tdcv
>> + *	  for each transmited CAN FD frame.
>                           ^^^
> transmitted
> 
> Fixed while applying to linux-can-next/testing. As net-next ist still
> closed, there is still some time to the next pull request and I'll
> squash patches if needed.

Hi Marc,

Can't find this in linux-can-next/testing
https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/log/?h=testing

Regards,
jimmy

Re: [PATCH v2 1/5] can: add new CAN FD bittiming parameters: Transmitter Delay Compensation (TDC)

[Marc Kleine-Budde]

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

On 09.03.2021 09:30:19, Jimmy Assarsson wrote:
> Can't find this in linux-can-next/testing
> https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/log/?h=testing

Try again.

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 v2 1/5] can: add new CAN FD bittiming parameters: Transmitter Delay Compensation (TDC)

[Jimmy Assarsson]

On 2021-03-09 09:34, Marc Kleine-Budde wrote:
> On 09.03.2021 09:30:19, Jimmy Assarsson wrote:
>> Can't find this in linux-can-next/testing
>> https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/log/?h=testing
> 
> Try again.
> 
> Marc

Thanks!

Re: [PATCH v2 4/5] can: add netlink interface for CAN-FD Transmitter Delay Compensation (TDC)

[Vincent MAILHOL]

On Wed. 24 Feb 2021 at 09:20, Vincent Mailhol
<mailhol.vincent@wanadoo.fr> wrote:
>
> diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
> index c19eef775ec8..c3f75c09d6c8 100644
> --- a/drivers/net/can/dev/netlink.c
> +++ b/drivers/net/can/dev/netlink.c
> @@ -19,6 +19,12 @@ static const struct nla_policy can_policy[IFLA_CAN_MAX + 1] = {
>         [IFLA_CAN_DATA_BITTIMING] = { .len = sizeof(struct can_bittiming) },
>         [IFLA_CAN_DATA_BITTIMING_CONST] = { .len = sizeof(struct can_bittiming_const) },
>         [IFLA_CAN_TERMINATION] = { .type = NLA_U16 },
> +       [IFLA_CAN_TDCV] = { .type = NLA_U32 },
> +       [IFLA_CAN_TDCV_MAX_CONST] = { .type = NLA_U32 },
> +       [IFLA_CAN_TDCO] = { .type = NLA_U32 },
> +       [IFLA_CAN_TDCO_MAX_CONST] = { .type = NLA_U32 },
> +       [IFLA_CAN_TDCF] = { .type = NLA_U32 },
> +       [IFLA_CAN_TDCF_MAX_CONST] = { .type = NLA_U32 },
>  };

Looking back at my patch, I just realized that the values are not
ordered in a consistent way. Here, I alternate between the TDCx
and the TDCx_CONST...

> (...)

> diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
> index f730d443b918..e69c4b330ae6 100644
> --- a/include/uapi/linux/can/netlink.h
> +++ b/include/uapi/linux/can/netlink.h
> @@ -134,6 +134,12 @@ enum {
>         IFLA_CAN_BITRATE_CONST,
>         IFLA_CAN_DATA_BITRATE_CONST,
>         IFLA_CAN_BITRATE_MAX,
> +       IFLA_CAN_TDCV,
> +       IFLA_CAN_TDCO,
> +       IFLA_CAN_TDCF,
> +       IFLA_CAN_TDCV_MAX_CONST,
> +       IFLA_CAN_TDCO_MAX_CONST,
> +       IFLA_CAN_TDCF_MAX_CONST,
>         __IFLA_CAN_MAX
>  };

... and there, all the TDCx and the TDCx_CONST are grouped together.

Marc, because the patches are already in the
linux-can-next/testing, how should I proceed to fix this? Should
I resend the full patch series with the changes or can I just
prepare one patch and ask you to squash it?


Yours sincerely,
Vincent

Re: [PATCH v2 4/5] can: add netlink interface for CAN-FD Transmitter Delay Compensation (TDC)

[Marc Kleine-Budde]

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

On 09.03.2021 22:16:45, Vincent MAILHOL wrote:
> On Wed. 24 Feb 2021 at 09:20, Vincent Mailhol
> <mailhol.vincent@wanadoo.fr> wrote:
> >
> > diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
> > index c19eef775ec8..c3f75c09d6c8 100644
> > --- a/drivers/net/can/dev/netlink.c
> > +++ b/drivers/net/can/dev/netlink.c
> > @@ -19,6 +19,12 @@ static const struct nla_policy can_policy[IFLA_CAN_MAX + 1] = {
> >         [IFLA_CAN_DATA_BITTIMING] = { .len = sizeof(struct can_bittiming) },
> >         [IFLA_CAN_DATA_BITTIMING_CONST] = { .len = sizeof(struct can_bittiming_const) },
> >         [IFLA_CAN_TERMINATION] = { .type = NLA_U16 },
> > +       [IFLA_CAN_TDCV] = { .type = NLA_U32 },
> > +       [IFLA_CAN_TDCV_MAX_CONST] = { .type = NLA_U32 },
> > +       [IFLA_CAN_TDCO] = { .type = NLA_U32 },
> > +       [IFLA_CAN_TDCO_MAX_CONST] = { .type = NLA_U32 },
> > +       [IFLA_CAN_TDCF] = { .type = NLA_U32 },
> > +       [IFLA_CAN_TDCF_MAX_CONST] = { .type = NLA_U32 },
> >  };
> 
> Looking back at my patch, I just realized that the values are not
> ordered in a consistent way. Here, I alternate between the TDCx
> and the TDCx_CONST...
> 
> > (...)
> 
> > diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
> > index f730d443b918..e69c4b330ae6 100644
> > --- a/include/uapi/linux/can/netlink.h
> > +++ b/include/uapi/linux/can/netlink.h
> > @@ -134,6 +134,12 @@ enum {
> >         IFLA_CAN_BITRATE_CONST,
> >         IFLA_CAN_DATA_BITRATE_CONST,
> >         IFLA_CAN_BITRATE_MAX,
> > +       IFLA_CAN_TDCV,
> > +       IFLA_CAN_TDCO,
> > +       IFLA_CAN_TDCF,
> > +       IFLA_CAN_TDCV_MAX_CONST,
> > +       IFLA_CAN_TDCO_MAX_CONST,
> > +       IFLA_CAN_TDCF_MAX_CONST,
> >         __IFLA_CAN_MAX
> >  };
> 
> ... and there, all the TDCx and the TDCx_CONST are grouped together.
> 
> Marc, because the patches are already in the
> linux-can-next/testing, how should I proceed to fix this? Should
> I resend the full patch series with the changes or can I just
> prepare one patch and ask you to squash it?

Please send an incremental patch and I'll squash it.

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 v2 4/5] can: add netlink interface for CAN-FD Transmitter Delay Compensation (TDC)

[Marc Kleine-Budde]

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

On 24.02.2021 09:20:07, Vincent Mailhol wrote:
> Add the netlink interface for TDC parameters of struct can_tdc and
> can_tdc_const.
> 
> Contrary to the can_bittiming(_const) structures for which there is
> just a single IFLA_CAN(_DATA)_BITTMING(_CONST) entry per structure,
> here, an IFLA_CAN_TDC* entry is added for each of the TDC parameters
> of the newly introduced struct can_tdc and struct can_tdc_const.
> 
> For struct can_tdc, these are:
> 	IFLA_CAN_TDCV
> 	IFLA_CAN_TDCO
> 	IFLA_CAN_TDCF
> 
> For struct can_tdc_const, these are:
> 	IFLA_CAN_TDCV_MAX_CONST
> 	IFLA_CAN_TDCO_MAX_CONST
> 	IFLA_CAN_TDCF_MAX_CONST
> 
> This is done so that changes can be applied in the future to the
> structures without breaking the netlink interface.
> 
> All the new parameters are defined as u32. This arbitrary choice is
> done to mimic the other bittiming values with are also all of type
> u32. An u16 would have been sufficient to hold the TDC values.

I just had a look at the ethtool-netlink interface:

| Documentation/networking/ethtool-netlink.rst

this is much better designed than the CAN netlink interface. It was done
by the pros and much later than CAN. :D So I'd like to have a similar
structure for new CAN netlink stuff.

So I think I'll remove this patch for now from can-next-testing. The
kernel internal interface to tdc is still OK, we can leave it as is and
change it if needed. But netlink is user space and I'd like to have it
properly designed.

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 v2 4/5] can: add netlink interface for CAN-FD Transmitter Delay Compensation (TDC)

[Vincent MAILHOL]

On Tue. 16 Mar 2021 at 00:59, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 24.02.2021 09:20:07, Vincent Mailhol wrote:
> > Add the netlink interface for TDC parameters of struct can_tdc and
> > can_tdc_const.
> >
> > Contrary to the can_bittiming(_const) structures for which there is
> > just a single IFLA_CAN(_DATA)_BITTMING(_CONST) entry per structure,
> > here, an IFLA_CAN_TDC* entry is added for each of the TDC parameters
> > of the newly introduced struct can_tdc and struct can_tdc_const.
> >
> > For struct can_tdc, these are:
> >       IFLA_CAN_TDCV
> >       IFLA_CAN_TDCO
> >       IFLA_CAN_TDCF
> >
> > For struct can_tdc_const, these are:
> >       IFLA_CAN_TDCV_MAX_CONST
> >       IFLA_CAN_TDCO_MAX_CONST
> >       IFLA_CAN_TDCF_MAX_CONST
> >
> > This is done so that changes can be applied in the future to the
> > structures without breaking the netlink interface.
> >
> > All the new parameters are defined as u32. This arbitrary choice is
> > done to mimic the other bittiming values with are also all of type
> > u32. An u16 would have been sufficient to hold the TDC values.
>
> I just had a look at the ethtool-netlink interface:
>
> | Documentation/networking/ethtool-netlink.rst
>
> this is much better designed than the CAN netlink interface. It was done
> by the pros and much later than CAN. :D So I'd like to have a similar
> structure for new CAN netlink stuff.
>
> So I think I'll remove this patch for now from can-next-testing. The
> kernel internal interface to tdc is still OK, we can leave it as is and
> change it if needed. But netlink is user space and I'd like to have it
> properly designed.

Understood. However, I will need more time to read and understand
the ethtool-netlink interface. The new patch will come later, I
do not know when.


Yours sincerely,
Vincent

Re: [PATCH v2 4/5] can: add netlink interface for CAN-FD Transmitter Delay Compensation (TDC)

[Marc Kleine-Budde]

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

On 17.03.2021 00:16:01, Vincent MAILHOL wrote:
> > I just had a look at the ethtool-netlink interface:
> >
> > | Documentation/networking/ethtool-netlink.rst
> >
> > this is much better designed than the CAN netlink interface. It was done
> > by the pros and much later than CAN. :D So I'd like to have a similar
> > structure for new CAN netlink stuff.
> >
> > So I think I'll remove this patch for now from can-next-testing. The
> > kernel internal interface to tdc is still OK, we can leave it as is and
> > change it if needed. But netlink is user space and I'd like to have it
> > properly designed.
> 
> Understood. However, I will need more time to read and understand
> the ethtool-netlink interface. The new patch will come later, I
> do not know when.

No Problem

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 v2 4/5] can: add netlink interface for CAN-FD Transmitter Delay Compensation (TDC)

[Vincent MAILHOL]

Hi Marc,

On Wed. 17 Mar 2021 at 00:29, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 17.03.2021 00:16:01, Vincent MAILHOL wrote:
> > > I just had a look at the ethtool-netlink interface:
> > >
> > > | Documentation/networking/ethtool-netlink.rst
> > >
> > > this is much better designed than the CAN netlink interface. It was done
> > > by the pros and much later than CAN. :D So I'd like to have a similar
> > > structure for new CAN netlink stuff.
> > >
> > > So I think I'll remove this patch for now from can-next-testing. The
> > > kernel internal interface to tdc is still OK, we can leave it as is and
> > > change it if needed. But netlink is user space and I'd like to have it
> > > properly designed.
> >
> > Understood. However, I will need more time to read and understand
> > the ethtool-netlink interface. The new patch will come later, I
> > do not know when.
>
> No Problem

I started to look at Ethtool netlink, but as far as my understanding
goes, this seems purely restricted to the ethtool application (i.e.
not to iproute2). I double checked the latest versions of iproute2
but there isn’t a single #include <linux/ethtool_netlink.h> nor
anything else related to that new API.

Please let me know if I missed the point.


Yours sincerely,
Vincent

Re: [PATCH v2 4/5] can: add netlink interface for CAN-FD Transmitter Delay Compensation (TDC)

[Marc Kleine-Budde]

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

On 05.04.2021 11:29:31, Vincent MAILHOL wrote:
> Hi Marc,
> 
> On Wed. 17 Mar 2021 at 00:29, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > On 17.03.2021 00:16:01, Vincent MAILHOL wrote:
> > > > I just had a look at the ethtool-netlink interface:
> > > >
> > > > | Documentation/networking/ethtool-netlink.rst
> > > >
> > > > this is much better designed than the CAN netlink interface. It was done
> > > > by the pros and much later than CAN. :D So I'd like to have a similar
> > > > structure for new CAN netlink stuff.
> > > >
> > > > So I think I'll remove this patch for now from can-next-testing. The
> > > > kernel internal interface to tdc is still OK, we can leave it as is and
> > > > change it if needed. But netlink is user space and I'd like to have it
> > > > properly designed.
> > >
> > > Understood. However, I will need more time to read and understand
> > > the ethtool-netlink interface. The new patch will come later, I
> > > do not know when.
> >
> > No Problem
> 
> I started to look at Ethtool netlink, but as far as my understanding
> goes, this seems purely restricted to the ethtool application (i.e.
> not to iproute2). I double checked the latest versions of iproute2
> but there isn’t a single #include <linux/ethtool_netlink.h> nor
> anything else related to that new API.
> 
> Please let me know if I missed the point.

For example have a look at the RINGS_GET of ethtool-netlink.rst:

| RINGS_GET
| =========
| 
| Gets ring sizes like ``ETHTOOL_GRINGPARAM`` ioctl request.
| 
| Request contents:
| 
|   ====================================  ======  ==========================
|   ``ETHTOOL_A_RINGS_HEADER``            nested  request header
|   ====================================  ======  ==========================
| 
| Kernel response contents:
| 
|   ====================================  ======  ==========================
|   ``ETHTOOL_A_RINGS_HEADER``            nested  reply header
|   ``ETHTOOL_A_RINGS_RX_MAX``            u32     max size of RX ring
|   ``ETHTOOL_A_RINGS_RX_MINI_MAX``       u32     max size of RX mini ring
|   ``ETHTOOL_A_RINGS_RX_JUMBO_MAX``      u32     max size of RX jumbo ring
|   ``ETHTOOL_A_RINGS_TX_MAX``            u32     max size of TX ring
|   ``ETHTOOL_A_RINGS_RX``                u32     size of RX ring
|   ``ETHTOOL_A_RINGS_RX_MINI``           u32     size of RX mini ring
|   ``ETHTOOL_A_RINGS_RX_JUMBO``          u32     size of RX jumbo ring
|   ``ETHTOOL_A_RINGS_TX``                u32     size of TX ring
|   ====================================  ======  ==========================

The response consists of a header and several nested values. This looks
to me to be the netlink way to serialize a C-struct. If I understand
your code correctly it was just the values without the nested header.

| RINGS_SET
| =========
| 
| Sets ring sizes like ``ETHTOOL_SRINGPARAM`` ioctl request.
| 
| Request contents:
| 
|   ====================================  ======  ==========================
|   ``ETHTOOL_A_RINGS_HEADER``            nested  reply header
|   ``ETHTOOL_A_RINGS_RX``                u32     size of RX ring
|   ``ETHTOOL_A_RINGS_RX_MINI``           u32     size of RX mini ring
|   ``ETHTOOL_A_RINGS_RX_JUMBO``          u32     size of RX jumbo ring
|   ``ETHTOOL_A_RINGS_TX``                u32     size of TX ring
|   ====================================  ======  ==========================
| 
| Kernel checks that requested ring sizes do not exceed limits reported by
| driver. Driver may impose additional constraints and may not suspport all
| attributes.

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 v2 1/5] can: add new CAN FD bittiming parameters: Transmitter Delay Compensation (TDC)

[Marc Kleine-Budde]

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

On 24.02.2021 09:20:04, Vincent Mailhol wrote:
> At high bit rates, the propagation delay from the TX pin to the RX pin
> of the transceiver causes measurement errors: the sample point on the
> RX pin might occur on the previous bit.
> 
> This issue is addressed in ISO 11898-1 section 11.3.3 "Transmitter
> delay compensation" (TDC).
> 
> This patch adds two new structures: can_tdc and can_tdc_const in order
> to implement this TDC.
> 
> The structures are then added to can_priv.
> 
> A controller supports TDC if an only if can_priv::tdc_const is not
> NULL.
> 
> TDC is active if and only if:
>   - fd flag is on
>   - can_priv::tdc.tdco is not zero.
> It is the driver responsibility to check those two conditions are met.
> 
> No new controller modes are introduced (i.e. no CAN_CTRL_MODE_TDC) in
> order not to be redundant with above logic.
> 
> The names of the parameters are chosen to match existing CAN
> controllers specification. References:
>   - Bosch C_CAN FD8:
> https://www.bosch-semiconductors.com/media/ip_modules/pdf_2/c_can_fd8/users_manual_c_can_fd8_r210_1.pdf
>   - Microchip CAN FD Controller Module:
> http://ww1.microchip.com/downloads/en/DeviceDoc/MCP251XXFD-CAN-FD-Controller-Module-Family-Reference-Manual-20005678B.pdf
>   - SAM E701/S70/V70/V71 Family:
> https://www.mouser.com/datasheet/2/268/60001527A-1284321.pdf
> 
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
>  include/linux/can/bittiming.h | 65 +++++++++++++++++++++++++++++++++++
>  include/linux/can/dev.h       |  3 ++
>  2 files changed, 68 insertions(+)
> 
> diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h
> index 707575c668f4..91bf9f8926a7 100644
> --- a/include/linux/can/bittiming.h
> +++ b/include/linux/can/bittiming.h
> @@ -1,5 +1,6 @@
>  /* SPDX-License-Identifier: GPL-2.0-only */
>  /* Copyright (c) 2020 Pengutronix, Marc Kleine-Budde <kernel@pengutronix.de>
> + * Copyright (c) 2021 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>   */
>  
>  #ifndef _CAN_BITTIMING_H
> @@ -10,6 +11,70 @@
>  
>  #define CAN_SYNC_SEG 1
>  
> +/*
> + * struct can_tdc - CAN FD Transmission Delay Compensation parameters
> + *
> + * At high bit rates, the propagation delay from the TX pin to the RX
> + * pin of the transceiver causes measurement errors: the sample point
> + * on the RX pin might occur on the previous bit.
> + *
> + * To solve this issue, ISO 11898-1 introduces in section 11.3.3
> + * "Transmitter delay compensation" a SSP (Secondary Sample Point)
> + * equal to the distance, in time quanta, from the start of the bit
> + * time on the TX pin to the actual measurement on the RX pin.
> + *
> + * This structure contains the parameters to calculate that SSP.
> + *
> + * @tdcv: Transmitter Delay Compensation Value. Distance, in time
> + *	quanta, from when the bit is sent on the TX pin to when it is
> + *	received on the RX pin of the transmitter. Possible options:
> + *
> + *	  O: automatic mode. The controller dynamically measure @tdcv
          ^

I think this is supposed to be a 0, not a O?
I'll send a patch

> + *	  for each transmited CAN FD frame.

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 v2 1/5] can: add new CAN FD bittiming parameters: Transmitter Delay Compensation (TDC)

[Vincent MAILHOL]

On Wed. 16 Jun 2021 at 18:54, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 24.02.2021 09:20:04, Vincent Mailhol wrote:
> > At high bit rates, the propagation delay from the TX pin to the RX pin
> > of the transceiver causes measurement errors: the sample point on the
> > RX pin might occur on the previous bit.
> >
> > This issue is addressed in ISO 11898-1 section 11.3.3 "Transmitter
> > delay compensation" (TDC).
> >
> > This patch adds two new structures: can_tdc and can_tdc_const in order
> > to implement this TDC.
> >
> > The structures are then added to can_priv.
> >
> > A controller supports TDC if an only if can_priv::tdc_const is not
> > NULL.
> >
> > TDC is active if and only if:
> >   - fd flag is on
> >   - can_priv::tdc.tdco is not zero.
> > It is the driver responsibility to check those two conditions are met.
> >
> > No new controller modes are introduced (i.e. no CAN_CTRL_MODE_TDC) in
> > order not to be redundant with above logic.
> >
> > The names of the parameters are chosen to match existing CAN
> > controllers specification. References:
> >   - Bosch C_CAN FD8:
> > https://www.bosch-semiconductors.com/media/ip_modules/pdf_2/c_can_fd8/users_manual_c_can_fd8_r210_1.pdf
> >   - Microchip CAN FD Controller Module:
> > http://ww1.microchip.com/downloads/en/DeviceDoc/MCP251XXFD-CAN-FD-Controller-Module-Family-Reference-Manual-20005678B.pdf
> >   - SAM E701/S70/V70/V71 Family:
> > https://www.mouser.com/datasheet/2/268/60001527A-1284321.pdf
> >
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > ---
> >  include/linux/can/bittiming.h | 65 +++++++++++++++++++++++++++++++++++
> >  include/linux/can/dev.h       |  3 ++
> >  2 files changed, 68 insertions(+)
> >
> > diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h
> > index 707575c668f4..91bf9f8926a7 100644
> > --- a/include/linux/can/bittiming.h
> > +++ b/include/linux/can/bittiming.h
> > @@ -1,5 +1,6 @@
> >  /* SPDX-License-Identifier: GPL-2.0-only */
> >  /* Copyright (c) 2020 Pengutronix, Marc Kleine-Budde <kernel@pengutronix.de>
> > + * Copyright (c) 2021 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> >   */
> >
> >  #ifndef _CAN_BITTIMING_H
> > @@ -10,6 +11,70 @@
> >
> >  #define CAN_SYNC_SEG 1
> >
> > +/*
> > + * struct can_tdc - CAN FD Transmission Delay Compensation parameters
> > + *
> > + * At high bit rates, the propagation delay from the TX pin to the RX
> > + * pin of the transceiver causes measurement errors: the sample point
> > + * on the RX pin might occur on the previous bit.
> > + *
> > + * To solve this issue, ISO 11898-1 introduces in section 11.3.3
> > + * "Transmitter delay compensation" a SSP (Secondary Sample Point)
> > + * equal to the distance, in time quanta, from the start of the bit
> > + * time on the TX pin to the actual measurement on the RX pin.
> > + *
> > + * This structure contains the parameters to calculate that SSP.
> > + *
> > + * @tdcv: Transmitter Delay Compensation Value. Distance, in time
> > + *   quanta, from when the bit is sent on the TX pin to when it is
> > + *   received on the RX pin of the transmitter. Possible options:
> > + *
> > + *     O: automatic mode. The controller dynamically measure @tdcv
>           ^
>
> I think this is supposed to be a 0, not a O?
> I'll send a patch

Absolutely. I am not sure how I did such a mistake. I was
probably not fully awake as there are two other grammar issues
there. I sent another patch to fix those.

Yours sincerely,
Vincent