[RFC PATCH v4 0/4] iplink_can: cleaning, fixes and adding TDC support.

[RFC PATCH v4 0/4] iplink_can: cleaning, fixes and adding TDC support.

[Vincent Mailhol]

The main purpose is to add commandline support for Transmitter Delay
Compensation (TDC) in iproute. Other issues found during the
development of this feature also get addressed.

This patch series contains four patches which respectively:
  1. Correct the bittiming ranges in the print_usage function.
  2. factorize the many print_*(PRINT_JSON, ...) and fprintf
  occurrences in a single print_*(PRINT_ANY, ...) call and fix the
  signedness while doing that.
  3. report the value of the bitrate prescalers (brp and dbrp).
  4. adds command line support for the TDC in iproute and goes together
  with below series in the kernel:
  https://lore.kernel.org/linux-can/20210628150607.1128574-1-mailhol.vincent@wanadoo.fr/T/#t

I am sending this series as RFC because the related patch series on
the kernel side have yet to be approved. Aside of that, I consider
this series to be ready. If the can:netlink patch series get accepted,
I will resend this one as is (just remove the RFC tag).

** Changelog **

From v3 to RFC v4:
  * Reflect the changes made on the kernel side.

From RFC v2 to v3:
  * Dropped the RFC tag. Now that the kernel patch reach the testing
    branch, I am finaly ready.
  * Regression fix: configuring a link with only nominal bittiming
    returned -EOPNOTSUPP
  * Added two more patches to the series:
      - iplink_can: fix configuration ranges in print_usage()
      - iplink_can: print brp and dbrp bittiming variables
  * Other small fixes on formatting.

From RFC v1 to RFC v2:
  * Add an additional patch to the series to fix the issues reported
    by Stephen Hemminger
    Ref: https://lore.kernel.org/linux-can/20210506112007.1666738-1-mailhol.vincent@wanadoo.fr/T/#t

Vincent Mailhol (4):
  iplink_can: fix configuration ranges in print_usage()
  iplink_can: use PRINT_ANY to factorize code and fix signedness
  iplink_can: print brp and dbrp bittiming variables
  iplink_can: add new CAN FD bittiming parameters: Transmitter Delay
    Compensation (TDC)

 include/uapi/linux/can/netlink.h |  30 +-
 ip/iplink_can.c                  | 464 +++++++++++++++++--------------
 2 files changed, 286 insertions(+), 208 deletions(-)

-- 
2.31.1

[RFC PATCH v4 1/4] iplink_can: fix configuration ranges in print_usage()

[Vincent Mailhol]

The configuration ranges in print_usage() are taken from "Table 8 -
Time segments' minimum configuration ranges" in section 11.3.1.2
"Configuration of the bit time parameters" of ISO 11898-1.

The standard clearly specifies that "implementations may allow time
segments that exceed the minimum required configuration ranges
specified in Table 8".

Because no maximum ranges are given, all given ranges { a..b } are
simply replaced with { NUMBER }.

The actual ranges are specific to earch device and should be confirmed
doing:

$ ip --details link show can0
1: can0: <NOARP,ECHO> mtu 16 qdisc noop state DOWN mode DEFAULT group default qlen 10
    link/can  promiscuity 0 minmtu 0 maxmtu 0
    can state STOPPED restart-ms 0
	  ES582.1/ES584.1: tseg1 2..256 tseg2 2..128 sjw 1..128 brp 1..512 brp-inc 1
	  ES582.1/ES584.1: dtseg1 2..32 dtseg2 1..16 dsjw 1..8 dbrp 1..32 dbrp-inc 1
	  clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 ip/iplink_can.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/ip/iplink_can.c b/ip/iplink_can.c
index 6a26f3ff..2736f3ab 100644
--- a/ip/iplink_can.c
+++ b/ip/iplink_can.c
@@ -44,13 +44,13 @@ static void print_usage(FILE *f)
 		"\n"
 		"\t[ termination { 0..65535 } ]\n"
 		"\n"
-		"\tWhere: BITRATE	:= { 1..1000000 }\n"
+		"\tWhere: BITRATE	:= { NUMBER }\n"
 		"\t	  SAMPLE-POINT	:= { 0.000..0.999 }\n"
 		"\t	  TQ		:= { NUMBER }\n"
-		"\t	  PROP-SEG	:= { 1..8 }\n"
-		"\t	  PHASE-SEG1	:= { 1..8 }\n"
-		"\t	  PHASE-SEG2	:= { 1..8 }\n"
-		"\t	  SJW		:= { 1..4 }\n"
+		"\t	  PROP-SEG	:= { NUMBER }\n"
+		"\t	  PHASE-SEG1	:= { NUMBER }\n"
+		"\t	  PHASE-SEG2	:= { NUMBER }\n"
+		"\t	  SJW		:= { NUMBER }\n"
 		"\t	  RESTART-MS	:= { 0 | NUMBER }\n"
 		);
 }
-- 
2.31.1

[RFC PATCH v4 2/4] iplink_can: use PRINT_ANY to factorize code and fix signedness

[Vincent Mailhol]

Current implementation heavily relies on some "if (is_json_context())"
switches to decide the context and then does some print_*(PRINT_JSON,
...) when in json context and some fprintf(...) else.

Furthermore, current implementation uses either print_int() or the
conversion specifier %d to print unsigned integers.

This patch factorizes each pairs of print_*(PRINT_JSON, ...) and
fprintf into a signle print_*(PRINT_ANY, ...) call. While doing this
replacement, it uses proper unsigned function print_uint() as well as
the conversion specifier %u when the parameter is an unsigned integer.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 ip/iplink_can.c | 339 ++++++++++++++++++++----------------------------
 1 file changed, 139 insertions(+), 200 deletions(-)

diff --git a/ip/iplink_can.c b/ip/iplink_can.c
index 2736f3ab..9dbe6afd 100644
--- a/ip/iplink_can.c
+++ b/ip/iplink_can.c
@@ -266,14 +266,33 @@ static const char *can_state_names[CAN_STATE_MAX] = {
 	[CAN_STATE_SLEEPING] = "SLEEPING"
 };
 
-static void can_print_json_timing_min_max(const char *attr, int min, int max)
+static void can_print_nl_indent(void)
+{
+	print_nl();
+	print_string(PRINT_FP, NULL, "%s", "\t ");
+}
+
+static void __can_print_timing_min_max(const char *attr, int min, int max)
 {
 	open_json_object(attr);
-	print_int(PRINT_JSON, "min", NULL, min);
-	print_int(PRINT_JSON, "max", NULL, max);
+	print_uint(PRINT_ANY, "min", " %d", min);
+	print_uint(PRINT_ANY, "max", "..%d", max);
 	close_json_object();
 }
 
+static void can_print_timing_min_max(const char *attr, int min, int max)
+{
+	print_string(PRINT_FP, NULL, " %s", attr);
+	__can_print_timing_min_max(attr, min, max);
+}
+
+/* Print data bittiming: use a "d" prefix when printing to the file pointer */
+static void can_print_data_timing_min_max(const char *attr, int min, int max)
+{
+	print_string(PRINT_FP, NULL, " d%s", attr);
+	__can_print_timing_min_max(attr, min, max);
+}
+
 static void can_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 {
 	if (!tb)
@@ -297,56 +316,38 @@ static void can_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 		struct can_berr_counter *bc =
 			RTA_DATA(tb[IFLA_CAN_BERR_COUNTER]);
 
-		if (is_json_context()) {
-			open_json_object("berr_counter");
-			print_int(PRINT_JSON, "tx", NULL, bc->txerr);
-			print_int(PRINT_JSON, "rx", NULL, bc->rxerr);
-			close_json_object();
-		} else {
-			fprintf(f, "(berr-counter tx %d rx %d) ",
-				bc->txerr, bc->rxerr);
-		}
+		open_json_object("berr_counter");
+		print_uint(PRINT_ANY, "tx", "(berr-counter tx %u", bc->txerr);
+		print_uint(PRINT_ANY, "rx", " rx %u) ", bc->rxerr);
+		close_json_object();
 	}
 
 	if (tb[IFLA_CAN_RESTART_MS]) {
 		__u32 *restart_ms = RTA_DATA(tb[IFLA_CAN_RESTART_MS]);
 
-		print_int(PRINT_ANY,
-			  "restart_ms",
-			  "restart-ms %d ",
-			  *restart_ms);
+		print_uint(PRINT_ANY, "restart_ms", "restart-ms %u ",
+			   *restart_ms);
 	}
 
 	/* bittiming is irrelevant if fixed bitrate is defined */
 	if (tb[IFLA_CAN_BITTIMING] && !tb[IFLA_CAN_BITRATE_CONST]) {
 		struct can_bittiming *bt = RTA_DATA(tb[IFLA_CAN_BITTIMING]);
-
-		if (is_json_context()) {
-			json_writer_t *jw;
-
-			open_json_object("bittiming");
-			print_int(PRINT_ANY, "bitrate", NULL, bt->bitrate);
-			jw = get_json_writer();
-			jsonw_name(jw, "sample_point");
-			jsonw_printf(jw, "%.3f",
-				     (float) bt->sample_point / 1000);
-			print_int(PRINT_ANY, "tq", NULL, bt->tq);
-			print_int(PRINT_ANY, "prop_seg", NULL, bt->prop_seg);
-			print_int(PRINT_ANY, "phase_seg1",
-				  NULL, bt->phase_seg1);
-			print_int(PRINT_ANY, "phase_seg2",
-				  NULL, bt->phase_seg2);
-			print_int(PRINT_ANY, "sjw", NULL, bt->sjw);
-			close_json_object();
-		} else {
-			fprintf(f, "\n	  bitrate %d sample-point %.3f ",
-				bt->bitrate, (float) bt->sample_point / 1000.);
-			fprintf(f,
-				"\n	  tq %d prop-seg %d phase-seg1 %d phase-seg2 %d sjw %d",
-				bt->tq, bt->prop_seg,
-				bt->phase_seg1, bt->phase_seg2,
-				bt->sjw);
-		}
+		char sp[6];
+
+		open_json_object("bittiming");
+		can_print_nl_indent();
+		print_uint(PRINT_ANY, "bitrate", " bitrate %u", bt->bitrate);
+		snprintf(sp, sizeof(sp), "%.3f", bt->sample_point / 1000.);
+		print_string(PRINT_ANY, "sample_point", " sample-point %s", sp);
+		can_print_nl_indent();
+		print_uint(PRINT_ANY, "tq", " tq %u", bt->tq);
+		print_uint(PRINT_ANY, "prop_seg", " prop-seg %u", bt->prop_seg);
+		print_uint(PRINT_ANY, "phase_seg1", " phase-seg1 %u",
+			   bt->phase_seg1);
+		print_uint(PRINT_ANY, "phase_seg2", "phase-seg2 %u ",
+			   bt->phase_seg2);
+		print_uint(PRINT_ANY, "sjw", " sjw %u", bt->sjw);
+		close_json_object();
 	}
 
 	/* bittiming const is irrelevant if fixed bitrate is defined */
@@ -354,28 +355,17 @@ static void can_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 		struct can_bittiming_const *btc =
 			RTA_DATA(tb[IFLA_CAN_BITTIMING_CONST]);
 
-		if (is_json_context()) {
-			open_json_object("bittiming_const");
-			print_string(PRINT_JSON, "name", NULL, btc->name);
-			can_print_json_timing_min_max("tseg1",
-						      btc->tseg1_min,
-						      btc->tseg1_max);
-			can_print_json_timing_min_max("tseg2",
-						      btc->tseg2_min,
-						      btc->tseg2_max);
-			can_print_json_timing_min_max("sjw", 1, btc->sjw_max);
-			can_print_json_timing_min_max("brp",
-						      btc->brp_min,
-						      btc->brp_max);
-			print_int(PRINT_JSON, "brp_inc", NULL, btc->brp_inc);
-			close_json_object();
-		} else {
-			fprintf(f, "\n	  %s: tseg1 %d..%d tseg2 %d..%d "
-				"sjw 1..%d brp %d..%d brp-inc %d",
-				btc->name, btc->tseg1_min, btc->tseg1_max,
-				btc->tseg2_min, btc->tseg2_max, btc->sjw_max,
-				btc->brp_min, btc->brp_max, btc->brp_inc);
-		}
+		open_json_object("bittiming_const");
+		can_print_nl_indent();
+		print_string(PRINT_ANY, "name", " %s:", btc->name);
+		can_print_timing_min_max("tseg1",
+					 btc->tseg1_min, btc->tseg1_max);
+		can_print_timing_min_max("tseg2",
+					 btc->tseg2_min, btc->tseg2_max);
+		can_print_timing_min_max("sjw", 1, btc->sjw_max);
+		can_print_timing_min_max("brp", btc->brp_min, btc->brp_max);
+		print_uint(PRINT_ANY, "brp_inc", " brp_inc %u", btc->brp_inc);
+		close_json_object();
 	}
 
 	if (tb[IFLA_CAN_BITRATE_CONST]) {
@@ -391,64 +381,47 @@ static void can_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 			bitrate = bt->bitrate;
 		}
 
-		if (is_json_context()) {
-			print_uint(PRINT_JSON,
-				   "bittiming_bitrate",
-				   NULL, bitrate);
-			open_json_array(PRINT_JSON, "bitrate_const");
-			for (i = 0; i < bitrate_cnt; ++i)
-				print_uint(PRINT_JSON, NULL, NULL,
-					   bitrate_const[i]);
-			close_json_array(PRINT_JSON, NULL);
-		} else {
-			fprintf(f, "\n	  bitrate %u", bitrate);
-			fprintf(f, "\n	     [");
-
-			for (i = 0; i < bitrate_cnt - 1; ++i) {
-				/* This will keep lines below 80 signs */
-				if (!(i % 6) && i)
-					fprintf(f, "\n	      ");
-
-				fprintf(f, "%8u, ", bitrate_const[i]);
+		can_print_nl_indent();
+		print_uint(PRINT_ANY, "bittiming_bitrate", " bitrate %u",
+			   bitrate);
+		can_print_nl_indent();
+		open_json_array(PRINT_ANY, is_json_context() ?
+				"bitrate_const" : "    [");
+		for (i = 0; i < bitrate_cnt; ++i) {
+			/* This will keep lines below 80 signs */
+			if (!(i % 6) && i) {
+				can_print_nl_indent();
+				print_string(PRINT_FP, NULL, "%s", "     ");
 			}
-
-			if (!(i % 6) && i)
-				fprintf(f, "\n	      ");
-			fprintf(f, "%8u ]", bitrate_const[i]);
+			print_uint(PRINT_ANY, NULL,
+				   i < bitrate_cnt - 1 ? "%8u, " : "%8u",
+				   bitrate_const[i]);
 		}
+		close_json_array(PRINT_JSON, " ]");
 	}
 
 	/* data bittiming is irrelevant if fixed bitrate is defined */
 	if (tb[IFLA_CAN_DATA_BITTIMING] && !tb[IFLA_CAN_DATA_BITRATE_CONST]) {
 		struct can_bittiming *dbt =
 			RTA_DATA(tb[IFLA_CAN_DATA_BITTIMING]);
-
-		if (is_json_context()) {
-			json_writer_t *jw;
-
-			open_json_object("data_bittiming");
-			print_int(PRINT_JSON, "bitrate", NULL, dbt->bitrate);
-			jw = get_json_writer();
-			jsonw_name(jw, "sample_point");
-			jsonw_printf(jw, "%.3f",
-				     (float) dbt->sample_point / 1000.);
-			print_int(PRINT_JSON, "tq", NULL, dbt->tq);
-			print_int(PRINT_JSON, "prop_seg", NULL, dbt->prop_seg);
-			print_int(PRINT_JSON, "phase_seg1",
-				  NULL, dbt->phase_seg1);
-			print_int(PRINT_JSON, "phase_seg2",
-				  NULL, dbt->phase_seg2);
-			print_int(PRINT_JSON, "sjw", NULL, dbt->sjw);
-			close_json_object();
-		} else {
-			fprintf(f, "\n	  dbitrate %d dsample-point %.3f ",
-				dbt->bitrate,
-				(float) dbt->sample_point / 1000.);
-			fprintf(f, "\n	  dtq %d dprop-seg %d dphase-seg1 %d "
-				"dphase-seg2 %d dsjw %d",
-				dbt->tq, dbt->prop_seg, dbt->phase_seg1,
-				dbt->phase_seg2, dbt->sjw);
-		}
+		char dsp[6];
+
+		open_json_object("data_bittiming");
+		can_print_nl_indent();
+		print_uint(PRINT_ANY, "bitrate", " dbitrate %u", dbt->bitrate);
+		snprintf(dsp, sizeof(dsp), "%.3f", dbt->sample_point / 1000.);
+		print_string(PRINT_ANY, "sample_point", " dsample-point %s",
+			     dsp);
+		can_print_nl_indent();
+		print_uint(PRINT_ANY, "tq", " dtq %u", dbt->tq);
+		print_uint(PRINT_ANY, "prop_seg", " dprop-seg %u",
+			   dbt->prop_seg);
+		print_uint(PRINT_ANY, "phase_seg1", " dphase-seg1 %u",
+			   dbt->phase_seg1);
+		print_uint(PRINT_ANY, "phase_seg2", " dphase-seg2 %u",
+			   dbt->phase_seg2);
+		print_uint(PRINT_ANY, "sjw", " dsjw %u", dbt->sjw);
+		close_json_object();
 	}
 
 	/* data bittiming const is irrelevant if fixed bitrate is defined */
@@ -457,29 +430,18 @@ static void can_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 		struct can_bittiming_const *dbtc =
 			RTA_DATA(tb[IFLA_CAN_DATA_BITTIMING_CONST]);
 
-		if (is_json_context()) {
-			open_json_object("data_bittiming_const");
-			print_string(PRINT_JSON, "name", NULL, dbtc->name);
-			can_print_json_timing_min_max("tseg1",
-						      dbtc->tseg1_min,
-						      dbtc->tseg1_max);
-			can_print_json_timing_min_max("tseg2",
-						      dbtc->tseg2_min,
-						      dbtc->tseg2_max);
-			can_print_json_timing_min_max("sjw", 1, dbtc->sjw_max);
-			can_print_json_timing_min_max("brp",
-						      dbtc->brp_min,
-						      dbtc->brp_max);
-
-			print_int(PRINT_JSON, "brp_inc", NULL, dbtc->brp_inc);
-			close_json_object();
-		} else {
-			fprintf(f, "\n	  %s: dtseg1 %d..%d dtseg2 %d..%d "
-				"dsjw 1..%d dbrp %d..%d dbrp-inc %d",
-				dbtc->name, dbtc->tseg1_min, dbtc->tseg1_max,
-				dbtc->tseg2_min, dbtc->tseg2_max, dbtc->sjw_max,
-				dbtc->brp_min, dbtc->brp_max, dbtc->brp_inc);
-		}
+		open_json_object("data_bittiming_const");
+		can_print_nl_indent();
+		print_string(PRINT_ANY, "name", " %s:", dbtc->name);
+		can_print_data_timing_min_max("tseg1",
+					      dbtc->tseg1_min, dbtc->tseg1_max);
+		can_print_data_timing_min_max("tseg2",
+					      dbtc->tseg2_min, dbtc->tseg2_max);
+		can_print_data_timing_min_max("sjw", 1, dbtc->sjw_max);
+		can_print_data_timing_min_max("brp",
+					      dbtc->brp_min, dbtc->brp_max);
+		print_uint(PRINT_ANY, "brp_inc", " brp_inc %u", dbtc->brp_inc);
+		close_json_object();
 	}
 
 	if (tb[IFLA_CAN_DATA_BITRATE_CONST]) {
@@ -497,30 +459,23 @@ static void can_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 			dbitrate = dbt->bitrate;
 		}
 
-		if (is_json_context()) {
-			print_uint(PRINT_JSON, "data_bittiming_bitrate",
-				   NULL, dbitrate);
-			open_json_array(PRINT_JSON, "data_bitrate_const");
-			for (i = 0; i < dbitrate_cnt; ++i)
-				print_uint(PRINT_JSON, NULL, NULL,
-					   dbitrate_const[i]);
-			close_json_array(PRINT_JSON, NULL);
-		} else {
-			fprintf(f, "\n	  dbitrate %u", dbitrate);
-			fprintf(f, "\n	     [");
-
-			for (i = 0; i < dbitrate_cnt - 1; ++i) {
-				/* This will keep lines below 80 signs */
-				if (!(i % 6) && i)
-					fprintf(f, "\n	      ");
-
-				fprintf(f, "%8u, ", dbitrate_const[i]);
+		can_print_nl_indent();
+		print_uint(PRINT_ANY, "data_bittiming_bitrate", " dbitrate %u",
+			   dbitrate);
+		can_print_nl_indent();
+		open_json_array(PRINT_ANY, is_json_context() ?
+				"data_bitrate_const" : "    [");
+		for (i = 0; i < dbitrate_cnt; ++i) {
+			/* This will keep lines below 80 signs */
+			if (!(i % 6) && i) {
+				can_print_nl_indent();
+				print_string(PRINT_FP, NULL, "%s", "     ");
 			}
-
-			if (!(i % 6) && i)
-				fprintf(f, "\n	      ");
-			fprintf(f, "%8u ]", dbitrate_const[i]);
+			print_uint(PRINT_ANY, NULL,
+				   i < dbitrate_cnt - 1 ? "%8u, " : "%8u",
+				   dbitrate_const[i]);
 		}
+		close_json_array(PRINT_JSON, " ]");
 	}
 
 	if (tb[IFLA_CAN_TERMINATION_CONST] && tb[IFLA_CAN_TERMINATION]) {
@@ -530,29 +485,21 @@ static void can_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 			sizeof(*trm_const);
 		int i;
 
-		if (is_json_context()) {
-			print_hu(PRINT_JSON, "termination", NULL, *trm);
-			open_json_array(PRINT_JSON, "termination_const");
-			for (i = 0; i < trm_cnt; ++i)
-				print_hu(PRINT_JSON, NULL, NULL, trm_const[i]);
-			close_json_array(PRINT_JSON, NULL);
-		} else {
-			fprintf(f, "\n	  termination %hu [ ", *trm);
-
-			for (i = 0; i < trm_cnt - 1; ++i)
-				fprintf(f, "%hu, ", trm_const[i]);
-
-			fprintf(f, "%hu ]", trm_const[i]);
-		}
+		can_print_nl_indent();
+		print_hu(PRINT_ANY, "termination", " termination %hu [ ", *trm);
+		open_json_array(PRINT_JSON, "termination_const");
+		for (i = 0; i < trm_cnt; ++i)
+			print_hu(PRINT_ANY, NULL,
+				 i < trm_cnt - 1 ? "%hu, " : "%hu",
+				 trm_const[i]);
+		close_json_array(PRINT_JSON, " ]");
 	}
 
 	if (tb[IFLA_CAN_CLOCK]) {
 		struct can_clock *clock = RTA_DATA(tb[IFLA_CAN_CLOCK]);
 
-		print_int(PRINT_ANY,
-			  "clock",
-			  "\n	  clock %d ",
-			  clock->freq);
+		can_print_nl_indent();
+		print_uint(PRINT_ANY, "clock", " clock %u ", clock->freq);
 	}
 
 }
@@ -565,31 +512,23 @@ static void can_print_xstats(struct link_util *lu,
 	if (xstats && RTA_PAYLOAD(xstats) == sizeof(*stats)) {
 		stats = RTA_DATA(xstats);
 
-		if (is_json_context()) {
-			print_int(PRINT_JSON, "restarts",
-				  NULL, stats->restarts);
-			print_int(PRINT_JSON, "bus_error",
-				  NULL, stats->bus_error);
-			print_int(PRINT_JSON, "arbitration_lost",
-				  NULL, stats->arbitration_lost);
-			print_int(PRINT_JSON, "error_warning",
-				  NULL, stats->error_warning);
-			print_int(PRINT_JSON, "error_passive",
-				  NULL, stats->error_passive);
-			print_int(PRINT_JSON, "bus_off", NULL, stats->bus_off);
-		} else {
-			fprintf(f, "\n	  re-started bus-errors arbit-lost "
-				"error-warn error-pass bus-off");
-			fprintf(f, "\n	  %-10d %-10d %-10d %-10d %-10d %-10d",
-				stats->restarts, stats->bus_error,
-				stats->arbitration_lost, stats->error_warning,
-				stats->error_passive, stats->bus_off);
-		}
+		can_print_nl_indent();
+		print_string(PRINT_FP, NULL, "%s",
+			     " re-started bus-errors arbit-lost error-warn error-pass bus-off");
+		can_print_nl_indent();
+		print_uint(PRINT_ANY, "restarts", " %-10u", stats->restarts);
+		print_uint(PRINT_ANY, "bus_error", " %-10u", stats->bus_error);
+		print_uint(PRINT_ANY, "arbitration_lost", " %-10u",
+			   stats->arbitration_lost);
+		print_uint(PRINT_ANY, "error_warning", " %-10u",
+			   stats->error_warning);
+		print_uint(PRINT_ANY, "error_passive", " %-10u",
+			   stats->error_passive);
+		print_uint(PRINT_ANY, "bus_off", " %-10u", stats->bus_off);
 	}
 }
 
-static void can_print_help(struct link_util *lu, int argc, char **argv,
-			   FILE *f)
+static void can_print_help(struct link_util *lu, int argc, char **argv, FILE *f)
 {
 	print_usage(f);
 }
-- 
2.31.1

[RFC PATCH v4 3/4] iplink_can: print brp and dbrp bittiming variables

[Vincent Mailhol]

Report the value of the bit-rate prescaler (brp) for both the nominal
and the data bittiming.

Currently, only the constant brp values (brp_{min,max,inc}) are being
reported. Also, brp is the only member of struct can_bittiming not
being reported.

Although brp is not used as an input for bittiming calculation, it
makes sense to output it.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 ip/iplink_can.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ip/iplink_can.c b/ip/iplink_can.c
index 9dbe6afd..00f585c6 100644
--- a/ip/iplink_can.c
+++ b/ip/iplink_can.c
@@ -347,6 +347,7 @@ static void can_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 		print_uint(PRINT_ANY, "phase_seg2", "phase-seg2 %u ",
 			   bt->phase_seg2);
 		print_uint(PRINT_ANY, "sjw", " sjw %u", bt->sjw);
+		print_uint(PRINT_ANY, "brp", " brp %u", bt->brp);
 		close_json_object();
 	}
 
@@ -421,6 +422,7 @@ static void can_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 		print_uint(PRINT_ANY, "phase_seg2", " dphase-seg2 %u",
 			   dbt->phase_seg2);
 		print_uint(PRINT_ANY, "sjw", " dsjw %u", dbt->sjw);
+		print_uint(PRINT_ANY, "brp", " dbrp %u", dbt->brp);
 		close_json_object();
 	}
 
-- 
2.31.1

Re: [RFC PATCH v4 3/4] iplink_can: print brp and dbrp bittiming variables

[Stefan Mätje]

Am Dienstag, den 29.06.2021, 00:44 +0900 schrieb Vincent Mailhol:
> Report the value of the bit-rate prescaler (brp) for both the nominal
> and the data bittiming.
> 
> Currently, only the constant brp values (brp_{min,max,inc}) are being
> reported. Also, brp is the only member of struct can_bittiming not
> being reported.
> 
> Although brp is not used as an input for bittiming calculation, it
> makes sense to output it.
> 
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

I think it is a good idea to display both brp and dbrp values because it makes
the displayed bitrate settings complete. Even if it could be calculated from the
displayed clock and tq values.

> ---
>  ip/iplink_can.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/ip/iplink_can.c b/ip/iplink_can.c
> index 9dbe6afd..00f585c6 100644
> --- a/ip/iplink_can.c
> +++ b/ip/iplink_can.c
> @@ -347,6 +347,7 @@ static void can_print_opt(struct link_util *lu, FILE *f,
> struct rtattr *tb[])
>  		print_uint(PRINT_ANY, "phase_seg2", "phase-seg2 %u ",
>  			   bt->phase_seg2);
>  		print_uint(PRINT_ANY, "sjw", " sjw %u", bt->sjw);
> +		print_uint(PRINT_ANY, "brp", " brp %u", bt->brp);
>  		close_json_object();
>  	}
>  
> @@ -421,6 +422,7 @@ static void can_print_opt(struct link_util *lu, FILE *f,
> struct rtattr *tb[])
>  		print_uint(PRINT_ANY, "phase_seg2", " dphase-seg2 %u",
>  			   dbt->phase_seg2);
>  		print_uint(PRINT_ANY, "sjw", " dsjw %u", dbt->sjw);
> +		print_uint(PRINT_ANY, "brp", " dbrp %u", dbt->brp);
>  		close_json_object();
>  	}
>  

Re: [RFC PATCH v4 3/4] iplink_can: print brp and dbrp bittiming variables

[Vincent MAILHOL]

On Wed. 7 Jul 2021 at 18:33, Stefan Mätje <Stefan.Maetje@esd.eu> wrote:
> Am Dienstag, den 29.06.2021, 00:44 +0900 schrieb Vincent Mailhol:
> > Report the value of the bit-rate prescaler (brp) for both the nominal
> > and the data bittiming.
> >
> > Currently, only the constant brp values (brp_{min,max,inc}) are being
> > reported. Also, brp is the only member of struct can_bittiming not
> > being reported.
> >
> > Although brp is not used as an input for bittiming calculation, it
> > makes sense to output it.
> >
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>
> I think it is a good idea to display both brp and dbrp values because it makes
> the displayed bitrate settings complete. Even if it could be calculated from the
> displayed clock and tq values.

Your remark is true. I also realized that BRP can be calculated
from the other parameters but because I am lazy, I like to have
it reported so I wrote this patch. I will add a note in the patch
comments to reflect that this value could be calculated by hand.


Yours sincerely,
Vincent

Re: [RFC PATCH v4 3/4] iplink_can: print brp and dbrp bittiming variables

[Stefan Mätje]

Am Freitag, den 09.07.2021, 14:17 +0200 schrieb Vincent MAILHOL:
> On Wed. 7 Jul 2021 at 18:33, Stefan Mätje <Stefan.Maetje@esd.eu> wrote:
> > Am Dienstag, den 29.06.2021, 00:44 +0900 schrieb Vincent Mailhol:
> > > Report the value of the bit-rate prescaler (brp) for both the nominal
> > > and the data bittiming.
> > > 
> > > Currently, only the constant brp values (brp_{min,max,inc}) are being
> > > reported. Also, brp is the only member of struct can_bittiming not
> > > being reported.
> > > 
> > > Although brp is not used as an input for bittiming calculation, it
> > > makes sense to output it.
> > > 
> > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > 
> > I think it is a good idea to display both brp and dbrp values because it makes
> > the displayed bitrate settings complete. Even if it could be calculated from the
> > displayed clock and tq values.
> 
> Your remark is true. I also realized that BRP can be calculated
> from the other parameters but because I am lazy, I like to have
> it reported so I wrote this patch. I will add a note in the patch
> comments to reflect that this value could be calculated by hand.

A late comment on this ...

I didn't mean in any way that the BRP value should not be shown because it 
could be calculated from the other displayed values.

But I'm happy with this change because the CAN clock, BRP, prop seg and 
tsegX values are the input parameters for the CAN bitrate configuration.
These are the "real" parameters.

The tq length and bitrate are only "derived" or calculated values. These
values underlie rounding and truncation effects and are therefore less
"worth" for the description of the bitrate configuration.

Therefore its my opinion its better to have the "real" input values be
shown from where all other values can be derived.

Best regards,
    Stefan

[RFC PATCH v4 4/4] iplink_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 brings command line support to nine TDC parameters which
were recently added to the kernel's CAN netlink interface in order to
implement TDC:
  - IFLA_CAN_TDC_TDCV_MIN: Transmitter Delay Compensation Value
    minimum value
  - IFLA_CAN_TDC_TDCV_MAX: Transmitter Delay Compensation Value
    maximum value
  - IFLA_CAN_TDC_TDCO_MIN: Transmitter Delay Compensation Offset
    minimum value
  - IFLA_CAN_TDC_TDCO_MAX: Transmitter Delay Compensation Offset
    maximum value
  - IFLA_CAN_TDC_TDCF_MIN: Transmitter Delay Compensation Filter
    window minimum value
  - IFLA_CAN_TDC_TDCF_MAX: Transmitter Delay Compensation Filter
    window maximum value
  - IFLA_CAN_TDC_TDCV: Transmitter Delay Compensation Value
  - IFLA_CAN_TDC_TDCO: Transmitter Delay Compensation Offset
  - IFLA_CAN_TDC_TDCF: Transmitter Delay Compensation Filter window

TDCV is reported only if the device supports
CAN_CTRLMODE_TDC_MANUAL. TDCF is reported only if tdcf_max is not
zero.

All those new parameters are nested together into the attribute
IFLA_CAN_TDC.

A tdc-mode parameter allow to specify how to opperate. Valid options
are:

  * auto: the transmitter automatically measures TDCV. As such, TDCV
    values can not be manually provided. In this mode, the user must
    specify TDCO and may also specify TDCF if supported.

  * manual: Use the TDCV value provided by the user are used. In this
    mode, the user must specify both TDCF and TDCO and may also
    specify TDCF if supported.

  * off: TDC is explicitely disabled.

  * tdc-mode parameter omitted: the kernel decides whether TDC should
    be enabled or not and if so, it calculates the TDC values.

For reference, here are a few samples of how the output looks like:

$ ip link set can0 type can bitrate 1000000 dbitrate 8000000 fd on tdco 7 tdcf 8 tdc-mode auto

$ ip --details link show can0
1:  can0: <NOARP,ECHO> mtu 72 qdisc noop state DOWN mode DEFAULT group default qlen 10
    link/can  promiscuity 0 minmtu 0 maxmtu 0
    can <FD,TDC_AUTO> state STOPPED (berr-counter tx 0 rx 0) restart-ms 0
	  bitrate 1000000 sample-point 0.750
	  tq 12 prop-seg 29 phase-seg1 30phase-seg2 20  sjw 1 brp 1
	  ES582.1/ES584.1: tseg1 2..256 tseg2 2..128 sjw 1..128 brp 1..512 brp_inc 1
	  dbitrate 8000000 dsample-point 0.700
	  dtq 12 dprop-seg 3 dphase-seg1 3 dphase-seg2 3 dsjw 1 dbrp 1
	  tdco 7 tdcf 8
	  ES582.1/ES584.1: dtseg1 2..32 dtseg2 1..16 dsjw 1..8 dbrp 1..32 brp_inc 1
	  tdco 0..127 tdcf 0..127
	  clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535

$ ip --details --json --pretty link show can0
[ {
        "ifindex": 1,
        "ifname": "can0",
        "flags": [ "NOARP","ECHO" ],
        "mtu": 72,
        "qdisc": "noop",
        "operstate": "DOWN",
        "linkmode": "DEFAULT",
        "group": "default",
        "txqlen": 10,
        "link_type": "can",
        "promiscuity": 0,
        "min_mtu": 0,
        "max_mtu": 0,
        "linkinfo": {
            "info_kind": "can",
            "info_data": {
                "ctrlmode": [ "FD","TDC_AUTO" ],
                "state": "STOPPED",
                "berr_counter": {
                    "tx": 0,
                    "rx": 0
                },
                "restart_ms": 0,
                "bittiming": {
                    "bitrate": 1000000,
                    "sample_point": "0.750",
                    "tq": 12,
                    "prop_seg": 29,
                    "phase_seg1": 30,
                    "phase_seg2": 20,
                    "sjw": 1,
                    "brp": 1
                },
                "bittiming_const": {
                    "name": "ES582.1/ES584.1",
                    "tseg1": {
                        "min": 2,
                        "max": 256
                    },
                    "tseg2": {
                        "min": 2,
                        "max": 128
                    },
                    "sjw": {
                        "min": 1,
                        "max": 128
                    },
                    "brp": {
                        "min": 1,
                        "max": 512
                    },
                    "brp_inc": 1
                },
                "data_bittiming": {
                    "bitrate": 8000000,
                    "sample_point": "0.700",
                    "tq": 12,
                    "prop_seg": 3,
                    "phase_seg1": 3,
                    "phase_seg2": 3,
                    "sjw": 1,
                    "brp": 1,
                    "tdc": {
                        "tdco": 7,
                        "tdcf": 8
                    }
                },
                "data_bittiming_const": {
                    "name": "ES582.1/ES584.1",
                    "tseg1": {
                        "min": 2,
                        "max": 32
                    },
                    "tseg2": {
                        "min": 1,
                        "max": 16
                    },
                    "sjw": {
                        "min": 1,
                        "max": 8
                    },
                    "brp": {
                        "min": 1,
                        "max": 32
                    },
                    "brp_inc": 1,
                    "tdc": {
                        "tdco": {
                            "min": 0,
                            "max": 127
                        },
                        "tdcf": {
                            "min": 0,
                            "max": 127
                        }
                    }
                },
                "clock": 80000000
            }
        },
        "num_tx_queues": 1,
        "num_rx_queues": 1,
        "gso_max_size": 65536,
        "gso_max_segs": 65535
    } ]

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 include/uapi/linux/can/netlink.h |  30 +++++++-
 ip/iplink_can.c                  | 113 +++++++++++++++++++++++++++++++
 2 files changed, 140 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
index 00c763df..258b2d2b 100644
--- a/include/uapi/linux/can/netlink.h
+++ b/include/uapi/linux/can/netlink.h
@@ -101,6 +101,8 @@ struct can_ctrlmode {
 #define CAN_CTRLMODE_PRESUME_ACK	0x40	/* Ignore missing CAN ACKs */
 #define CAN_CTRLMODE_FD_NON_ISO		0x80	/* CAN FD in non-ISO mode */
 #define CAN_CTRLMODE_CC_LEN8_DLC	0x100	/* Classic CAN DLC option */
+#define CAN_CTRLMODE_TDC_AUTO		0x200	/* CAN transiver automatically calculates TDCV */
+#define CAN_CTRLMODE_TDC_MANUAL		0x400	/* TDCV is manually set up by user */
 
 /*
  * CAN device statistics
@@ -134,12 +136,34 @@ enum {
 	IFLA_CAN_BITRATE_CONST,
 	IFLA_CAN_DATA_BITRATE_CONST,
 	IFLA_CAN_BITRATE_MAX,
-	__IFLA_CAN_MAX
-};
+	IFLA_CAN_TDC,
 
-#define IFLA_CAN_MAX	(__IFLA_CAN_MAX - 1)
+	/* add new constants above here */
+	__IFLA_CAN_MAX,
+	IFLA_CAN_MAX = __IFLA_CAN_MAX - 1
+};
 
 /* u16 termination range: 1..65535 Ohms */
 #define CAN_TERMINATION_DISABLED 0
 
+/*
+ * CAN FD Transmitter Delay Compensation (TDC)
+ */
+enum {
+	IFLA_CAN_TDC_UNSPEC,
+	IFLA_CAN_TDC_TDCV_MIN,	/* u32 */
+	IFLA_CAN_TDC_TDCV_MAX,	/* u32 */
+	IFLA_CAN_TDC_TDCO_MIN,	/* u32 */
+	IFLA_CAN_TDC_TDCO_MAX,	/* u32 */
+	IFLA_CAN_TDC_TDCF_MIN,	/* u32 */
+	IFLA_CAN_TDC_TDCF_MAX,	/* u32 */
+	IFLA_CAN_TDC_TDCV,	/* u32 */
+	IFLA_CAN_TDC_TDCO,	/* u32 */
+	IFLA_CAN_TDC_TDCF,	/* u32 */
+
+	/* add new constants above here */
+	__IFLA_CAN_TDC,
+	IFLA_CAN_TDC_MAX = __IFLA_CAN_TDC - 1
+};
+
 #endif /* !_UAPI_CAN_NETLINK_H */
diff --git a/ip/iplink_can.c b/ip/iplink_can.c
index 00f585c6..8ea0a526 100644
--- a/ip/iplink_can.c
+++ b/ip/iplink_can.c
@@ -28,6 +28,7 @@ static void print_usage(FILE *f)
 		"\n"
 		"\t[ dbitrate BITRATE [ dsample-point SAMPLE-POINT] ] |\n"
 		"\t[ dtq TQ dprop-seg PROP_SEG dphase-seg1 PHASE-SEG1\n \t  dphase-seg2 PHASE-SEG2 [ dsjw SJW ] ]\n"
+		"\t[ tdcv TDCV tdco TDCO tdcf TDCF ]\n"
 		"\n"
 		"\t[ loopback { on | off } ]\n"
 		"\t[ listen-only { on | off } ]\n"
@@ -38,6 +39,7 @@ static void print_usage(FILE *f)
 		"\t[ fd-non-iso { on | off } ]\n"
 		"\t[ presume-ack { on | off } ]\n"
 		"\t[ cc-len8-dlc { on | off } ]\n"
+		"\t[ tdc-mode { auto | manual | off } ]\n"
 		"\n"
 		"\t[ restart-ms TIME-MS ]\n"
 		"\t[ restart ]\n"
@@ -51,6 +53,9 @@ static void print_usage(FILE *f)
 		"\t	  PHASE-SEG1	:= { NUMBER }\n"
 		"\t	  PHASE-SEG2	:= { NUMBER }\n"
 		"\t	  SJW		:= { NUMBER }\n"
+		"\t	  TDCV		:= { NUMBER }\n"
+		"\t	  TDCO		:= { NUMBER }\n"
+		"\t	  TDCF		:= { NUMBER }\n"
 		"\t	  RESTART-MS	:= { 0 | NUMBER }\n"
 		);
 }
@@ -105,6 +110,8 @@ static void print_ctrlmode(FILE *f, __u32 cm)
 	_PF(CAN_CTRLMODE_FD_NON_ISO, "FD-NON-ISO");
 	_PF(CAN_CTRLMODE_PRESUME_ACK, "PRESUME-ACK");
 	_PF(CAN_CTRLMODE_CC_LEN8_DLC, "CC-LEN8-DLC");
+	_PF(CAN_CTRLMODE_TDC_AUTO, "TDC_AUTO");
+	_PF(CAN_CTRLMODE_TDC_MANUAL, "TDC_MANUAL");
 #undef _PF
 	if (cm)
 		print_hex(PRINT_ANY, NULL, "%x", cm);
@@ -116,6 +123,8 @@ static int can_parse_opt(struct link_util *lu, int argc, char **argv,
 {
 	struct can_bittiming bt = {}, dbt = {};
 	struct can_ctrlmode cm = {0, 0};
+	struct rtattr *tdc;
+	__u32 tdcv = -1, tdco = -1, tdcf = -1;
 
 	while (argc > 0) {
 		if (matches(*argv, "bitrate") == 0) {
@@ -181,6 +190,18 @@ static int can_parse_opt(struct link_util *lu, int argc, char **argv,
 			NEXT_ARG();
 			if (get_u32(&dbt.sjw, *argv, 0))
 				invarg("invalid \"dsjw\" value\n", *argv);
+		} else if (matches(*argv, "tdcv") == 0) {
+			NEXT_ARG();
+			if (get_u32(&tdcv, *argv, 0))
+				invarg("invalid \"tdcv\" value\n", *argv);
+		} else if (matches(*argv, "tdco") == 0) {
+			NEXT_ARG();
+			if (get_u32(&tdco, *argv, 0))
+				invarg("invalid \"tdco\" value\n", *argv);
+		} else if (matches(*argv, "tdcf") == 0) {
+			NEXT_ARG();
+			if (get_u32(&tdcf, *argv, 0))
+				invarg("invalid \"tdcf\" value\n", *argv);
 		} else if (matches(*argv, "loopback") == 0) {
 			NEXT_ARG();
 			set_ctrlmode("loopback", *argv, &cm,
@@ -217,6 +238,23 @@ static int can_parse_opt(struct link_util *lu, int argc, char **argv,
 			NEXT_ARG();
 			set_ctrlmode("cc-len8-dlc", *argv, &cm,
 				     CAN_CTRLMODE_CC_LEN8_DLC);
+		} else if (matches(*argv, "tdc-mode") == 0) {
+			NEXT_ARG();
+			if (strcmp(*argv, "auto") == 0) {
+				cm.flags |= CAN_CTRLMODE_TDC_AUTO;
+				cm.mask |= CAN_CTRLMODE_TDC_AUTO;
+			} else if (strcmp(*argv, "manual") == 0) {
+				cm.flags |= CAN_CTRLMODE_TDC_MANUAL;
+				cm.mask |= CAN_CTRLMODE_TDC_MANUAL;
+			} else if (strcmp(*argv, "off") == 0) {
+				cm.mask |= CAN_CTRLMODE_TDC_AUTO |
+					   CAN_CTRLMODE_TDC_MANUAL;
+			} else {
+				fprintf(stderr,
+					"Error: argument of \"tdc-mode\" must be \"auto\", \"manual\" or \"off\", not \"%s\"\n",
+					*argv);
+				exit (-1);
+			}
 		} else if (matches(*argv, "restart") == 0) {
 			__u32 val = 1;
 
@@ -254,6 +292,17 @@ static int can_parse_opt(struct link_util *lu, int argc, char **argv,
 	if (cm.mask)
 		addattr_l(n, 1024, IFLA_CAN_CTRLMODE, &cm, sizeof(cm));
 
+	if (tdcv != -1 || tdco != -1 || tdcf != -1) {
+		tdc = addattr_nest(n, 1024, IFLA_CAN_TDC | NLA_F_NESTED);
+		if (tdcv != -1)
+			addattr32(n, 1024, IFLA_CAN_TDC_TDCV, tdcv);
+		if (tdco != -1)
+			addattr32(n, 1024, IFLA_CAN_TDC_TDCO, tdco);
+		if (tdcf != -1)
+			addattr32(n, 1024, IFLA_CAN_TDC_TDCF, tdcf);
+		addattr_nest_end(n, tdc);
+	}
+
 	return 0;
 }
 
@@ -293,6 +342,62 @@ static void can_print_data_timing_min_max(const char *attr, int min, int max)
 	__can_print_timing_min_max(attr, min, max);
 }
 
+static void can_print_tdc_opt(FILE *f, struct rtattr *tdc_attr)
+{
+	struct rtattr *tb[IFLA_CAN_TDC_MAX + 1];
+
+	parse_rtattr_nested(tb, IFLA_CAN_TDC_MAX, tdc_attr);
+	if (tb[IFLA_CAN_TDC_TDCV] || tb[IFLA_CAN_TDC_TDCO] ||
+	    tb[IFLA_CAN_TDC_TDCF]) {
+		open_json_object("tdc");
+		can_print_nl_indent();
+		if (tb[IFLA_CAN_TDC_TDCV]) {
+			__u32 *tdcv = RTA_DATA(tb[IFLA_CAN_TDC_TDCV]);
+
+			print_uint(PRINT_ANY, "tdcv", " tdcv %u", *tdcv);
+		}
+		if (tb[IFLA_CAN_TDC_TDCO]) {
+			__u32 *tdco = RTA_DATA(tb[IFLA_CAN_TDC_TDCO]);
+
+			print_uint(PRINT_ANY, "tdco", " tdco %u", *tdco);
+		}
+		if (tb[IFLA_CAN_TDC_TDCF]) {
+			__u32 *tdcf = RTA_DATA(tb[IFLA_CAN_TDC_TDCF]);
+
+			print_uint(PRINT_ANY, "tdcf", " tdcf %u", *tdcf);
+		}
+		close_json_object();
+	}
+}
+
+static void can_print_tdc_const_opt(FILE *f, struct rtattr *tdc_attr)
+{
+	struct rtattr *tb[IFLA_CAN_TDC_MAX + 1];
+
+	parse_rtattr_nested(tb, IFLA_CAN_TDC_MAX, tdc_attr);
+	open_json_object("tdc");
+	can_print_nl_indent();
+	if (tb[IFLA_CAN_TDC_TDCV_MIN] && tb[IFLA_CAN_TDC_TDCV_MAX]) {
+		__u32 *tdcv_min = RTA_DATA(tb[IFLA_CAN_TDC_TDCV_MIN]);
+		__u32 *tdcv_max = RTA_DATA(tb[IFLA_CAN_TDC_TDCV_MAX]);
+
+		can_print_timing_min_max("tdcv", *tdcv_min, *tdcv_max);
+	}
+	if (tb[IFLA_CAN_TDC_TDCO_MIN] && tb[IFLA_CAN_TDC_TDCO_MAX]) {
+		__u32 *tdco_min = RTA_DATA(tb[IFLA_CAN_TDC_TDCO_MIN]);
+		__u32 *tdco_max = RTA_DATA(tb[IFLA_CAN_TDC_TDCO_MAX]);
+
+		can_print_timing_min_max("tdco", *tdco_min, *tdco_max);
+	}
+	if (tb[IFLA_CAN_TDC_TDCF_MIN] && tb[IFLA_CAN_TDC_TDCF_MAX]) {
+		__u32 *tdcf_min = RTA_DATA(tb[IFLA_CAN_TDC_TDCF_MIN]);
+		__u32 *tdcf_max = RTA_DATA(tb[IFLA_CAN_TDC_TDCF_MAX]);
+
+		can_print_timing_min_max("tdcf", *tdcf_min, *tdcf_max);
+	}
+	close_json_object();
+}
+
 static void can_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 {
 	if (!tb)
@@ -423,6 +528,10 @@ static void can_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 			   dbt->phase_seg2);
 		print_uint(PRINT_ANY, "sjw", " dsjw %u", dbt->sjw);
 		print_uint(PRINT_ANY, "brp", " dbrp %u", dbt->brp);
+
+		if (tb[IFLA_CAN_TDC])
+			can_print_tdc_opt(f, tb[IFLA_CAN_TDC]);
+
 		close_json_object();
 	}
 
@@ -443,6 +552,10 @@ static void can_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 		can_print_data_timing_min_max("brp",
 					      dbtc->brp_min, dbtc->brp_max);
 		print_uint(PRINT_ANY, "brp_inc", " brp_inc %u", dbtc->brp_inc);
+
+		if (tb[IFLA_CAN_TDC])
+			can_print_tdc_const_opt(f, tb[IFLA_CAN_TDC]);
+
 		close_json_object();
 	}
 
-- 
2.31.1

Re: [RFC PATCH v4 4/4] iplink_can: add new CAN FD bittiming parameters: Transmitter Delay Compensation (TDC)

[Stefan Mätje]

Hi Vincent,

it took some time to get an opinion on the proposed implementation of the user
interface to control TDC.

At first I've inspected the driver implementations for the following CAN cores
in the Linux tree (FlexCAN, ifi_canfd, MCAN, mcp251xfd) on how the TDC is
implemented currently. All the drivers for these controllers by default use the
automatic measurement of TDCV and set up the TDCO value in a way that the SSP
occurs in the delayed received bit at the same position as the sample point is
set for the remote receivers (provided the same bitrate setting is used).

This is the default mode for TDC, i. e. the TDC is always enabled in FD mode
(sometimes only for higher data bitrates) and the TDCO is derived from the data
bitrate settings. This works and the measurement of TDCV compensates for changes
in delay over time and temperature. This is also the recommended setting of the
SSP.

So the opportunity to change the TDC control values is only needed in a test
enviroment or when trying to tune the behavior to evaluate corner cases.

Need to mention that the TDCV and TDCO values (if implemented) for all mentioned
controllers are in CAN clock period (Tc) counts and NOT in time quanta (Tq)
counts! The CAN clock period (Tc) also defines the "minimum time quantum" which
is referenced in the ISO 11898 description of the TDC operation.

See below for further comments ...

Best regards,
    Stefan


Am Dienstag, den 29.06.2021, 00:44 +0900 schrieb 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 brings command line support to nine TDC parameters which
> were recently added to the kernel's CAN netlink interface in order to
> implement TDC:
>   - IFLA_CAN_TDC_TDCV_MIN: Transmitter Delay Compensation Value
>     minimum value
>   - IFLA_CAN_TDC_TDCV_MAX: Transmitter Delay Compensation Value
>     maximum value
>   - IFLA_CAN_TDC_TDCO_MIN: Transmitter Delay Compensation Offset
>     minimum value
>   - IFLA_CAN_TDC_TDCO_MAX: Transmitter Delay Compensation Offset
>     maximum value
>   - IFLA_CAN_TDC_TDCF_MIN: Transmitter Delay Compensation Filter
>     window minimum value
>   - IFLA_CAN_TDC_TDCF_MAX: Transmitter Delay Compensation Filter
>     window maximum value
>   - IFLA_CAN_TDC_TDCV: Transmitter Delay Compensation Value
>   - IFLA_CAN_TDC_TDCO: Transmitter Delay Compensation Offset
>   - IFLA_CAN_TDC_TDCF: Transmitter Delay Compensation Filter window
> 
> TDCV is reported only if the device supports
> CAN_CTRLMODE_TDC_MANUAL. 

I think the TDCV value should be reported in any case. Assume the interface is
in default mode (as described before and selected by omitting the tdc-mode
parameter) or in your auto mode then the TDCV value reported should be the
measured value of the TDCV if available. This could then serve as a useful
starting point for manual configuration experiments. If the measured TDCV is not
available from the controller registers it should return zero.

A similar idea holds true for the TDCO value. When the driver calulates a
suitable value for the TDCO (in default mode) the "ip -details link show" should
report the TDCO value set by the driver.

> TDCF is reported only if tdcf_max is not
> zero.

Note: I think TDCF is only supported by MCAN so far.

> All those new parameters are nested together into the attribute
> IFLA_CAN_TDC.
> 
> A tdc-mode parameter allow to specify how to opperate. Valid options
> are:
> 
>   * auto: the transmitter automatically measures TDCV. As such, TDCV
>     values can not be manually provided. In this mode, the user must
>     specify TDCO and may also specify TDCF if supported.
> 
>   * manual: Use the TDCV value provided by the user are used. In this
>     mode, the user must specify both TDCF and TDCO and may also
>     specify TDCF if supported.
> 
>   * off: TDC is explicitely disabled.
> 
>   * tdc-mode parameter omitted: the kernel decides whether TDC should
>     be enabled or not and if so, it calculates the TDC values.

This mode with tdc-mode parameter omitted I have called "default mode" in my
introduction.

For reference, here are a few samples of how the output looks like:
> 
> $ ip link set can0 type can bitrate 1000000 dbitrate 8000000 fd on tdco 7 tdcf
> 8 tdc-mode auto

So switching from a setting with "tdc-mode auto" (like before) back to the
default mode should then be done by

$ ip link set can0 type can bitrate 1000000 dbitrate 8000000 fd on

But as far as I understand without any TDC parameters the ip tool does not send
the IFLA_CAN_TDC structure to the kernel. So on the kernel level this is the
signal to switch back to "default mode" for TDC?

> 
> $ ip --details link show can0
> 1:  can0: <NOARP,ECHO> mtu 72 qdisc noop state DOWN mode DEFAULT group default
> qlen 10
>     link/can  promiscuity 0 minmtu 0 maxmtu 0
>     can <FD,TDC_AUTO> state STOPPED (berr-counter tx 0 rx 0) restart-ms 0
> 	  bitrate 1000000 sample-point 0.750
> 	  tq 12 prop-seg 29 phase-seg1 30phase-seg2 20  sjw 1 brp 1
> 	  ES582.1/ES584.1: tseg1 2..256 tseg2 2..128 sjw 1..128 brp 1..512
> brp_inc 1
> 	  dbitrate 8000000 dsample-point 0.700
> 	  dtq 12 dprop-seg 3 dphase-seg1 3 dphase-seg2 3 dsjw 1 dbrp 1
> 	  tdco 7 tdcf 8
> 	  ES582.1/ES584.1: dtseg1 2..32 dtseg2 1..16 dsjw 1..8 dbrp 1..32
> brp_inc 1
> 	  tdco 0..127 tdcf 0..127
> 	  clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536
> gso_max_segs 65535

After looking at this I have a concern on the design decision to make the
transferred TDCV/TDCO/TDCF values defined as multiple of time quanta (Tq).

As I mentioned before the all controllers use register values based on multiples
of CAN clock periods (Tc). And in the IFLA_CAN_TDC_TDCx_{MIN|MAX} or "struct
can_tdc_const" tables shown as

>	  tdco 0..127 tdcf 0..127

you use the maximum allowed values for the controller registers which are also
the Tc based values.

But in include/linux/can/bittiming.h the TDCx values are described as being
multiples of time quanta (Tq).

But this creates a problem with data bitrates where dbrp is 2. If this is the
case at the end the driver needs to convert the TDCx values from Tq based
numbers to Tc based numbers. Then you may have the case where "tdco 64" is set
on the "ip" command line. The can_tdc_changelink() function happily compares the
given "tdco" with the limits and lets this value pass. But some code needs to
multiply with dbrp=2 which would then overflow the register bits.

On the other hand even if can_tdc_changelink() does the conversion and the check
already it would signal -EINVAL for a value (64) that is apparently announced as
legal judging from the output of "ip -d link show ...".

To eliminate this discrepancy "ip -d link show ..." would need to display only
"tdco 0..63" as allowed values if dbrp==2. So the kernel possibly needs to
convert the allowed TDCx min / max constants from Tc based values to Tq based
values. I think this would be a nightmare when the allowed limits change based
on the bitrate setting.

So I would propose to change the documentation in include/linux/can/bittiming.h
to say that the TDCx values are based on the CAN clock period (Tc). Then the
code for can_tdc_changelink() checks for the correct limits. And also the help
output of "ip link help can" should explicitely state that the TDCx values are
NOT based on time quanta.

I think the confusion of the user is less with that approach because it is not
so far away from the documented register interface of the CAN controllers. Even
if it would be the only point in the CAN area where settings are not based on
time quanta (Tq) but on CAN clock periods (Tc).

And as I mentioned above the "normal" user should never need to set TDC values
and always use the "default" mode. The expert user should perhaps be able to
understand the different time bases for TDCx values and other timing parameters.

And in a testing and evaluation environment the higher resolution of the Tc
based TDCx values may also be a benefit. I. e. when reading the measured TDCV
value to monitor the change of the delay over time and temperature.



> $ ip --details --json --pretty link show can0
> [ {
>         "ifindex": 1,
>         "ifname": "can0",
>         "flags": [ "NOARP","ECHO" ],
>         "mtu": 72,
>         "qdisc": "noop",
>         "operstate": "DOWN",
>         "linkmode": "DEFAULT",
>         "group": "default",
>         "txqlen": 10,
>         "link_type": "can",
>         "promiscuity": 0,
>         "min_mtu": 0,
>         "max_mtu": 0,
>         "linkinfo": {
>             "info_kind": "can",
>             "info_data": {
>                 "ctrlmode": [ "FD","TDC_AUTO" ],
>                 "state": "STOPPED",
>                 "berr_counter": {
>                     "tx": 0,
>                     "rx": 0
>                 },
>                 "restart_ms": 0,
>                 "bittiming": {
>                     "bitrate": 1000000,
>                     "sample_point": "0.750",
>                     "tq": 12,
>                     "prop_seg": 29,
>                     "phase_seg1": 30,
>                     "phase_seg2": 20,
>                     "sjw": 1,
>                     "brp": 1
>                 },
>                 "bittiming_const": {
>                     "name": "ES582.1/ES584.1",
>                     "tseg1": {
>                         "min": 2,
>                         "max": 256
>                     },
>                     "tseg2": {
>                         "min": 2,
>                         "max": 128
>                     },
>                     "sjw": {
>                         "min": 1,
>                         "max": 128
>                     },
>                     "brp": {
>                         "min": 1,
>                         "max": 512
>                     },
>                     "brp_inc": 1
>                 },
>                 "data_bittiming": {
>                     "bitrate": 8000000,
>                     "sample_point": "0.700",
>                     "tq": 12,
>                     "prop_seg": 3,
>                     "phase_seg1": 3,
>                     "phase_seg2": 3,
>                     "sjw": 1,
>                     "brp": 1,
>                     "tdc": {
>                         "tdco": 7,
>                         "tdcf": 8
>                     }
>                 },
>                 "data_bittiming_const": {
>                     "name": "ES582.1/ES584.1",
>                     "tseg1": {
>                         "min": 2,
>                         "max": 32
>                     },
>                     "tseg2": {
>                         "min": 1,
>                         "max": 16
>                     },
>                     "sjw": {
>                         "min": 1,
>                         "max": 8
>                     },
>                     "brp": {
>                         "min": 1,
>                         "max": 32
>                     },
>                     "brp_inc": 1,
>                     "tdc": {
>                         "tdco": {
>                             "min": 0,
>                             "max": 127
>                         },
>                         "tdcf": {
>                             "min": 0,
>                             "max": 127
>                         }
>                     }
>                 },
>                 "clock": 80000000
>             }
>         },
>         "num_tx_queues": 1,
>         "num_rx_queues": 1,
>         "gso_max_size": 65536,
>         "gso_max_segs": 65535
>     } ]
> 
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
>  include/uapi/linux/can/netlink.h |  30 +++++++-
>  ip/iplink_can.c                  | 113 +++++++++++++++++++++++++++++++
>  2 files changed, 140 insertions(+), 3 deletions(-)
> 
> diff --git a/include/uapi/linux/can/netlink.h
> b/include/uapi/linux/can/netlink.h
> index 00c763df..258b2d2b 100644
> --- a/include/uapi/linux/can/netlink.h
> +++ b/include/uapi/linux/can/netlink.h
> @@ -101,6 +101,8 @@ struct can_ctrlmode {
>  #define CAN_CTRLMODE_PRESUME_ACK	0x40	/* Ignore missing CAN ACKs */
>  #define CAN_CTRLMODE_FD_NON_ISO		0x80	/* CAN FD in non-ISO
> mode */
>  #define CAN_CTRLMODE_CC_LEN8_DLC	0x100	/* Classic CAN DLC option */
> +#define CAN_CTRLMODE_TDC_AUTO		0x200	/* CAN transiver
> automatically calculates TDCV */
> +#define CAN_CTRLMODE_TDC_MANUAL		0x400	/* TDCV is manually
> set up by user */
>  
>  /*
>   * CAN device statistics
> @@ -134,12 +136,34 @@ enum {
>  	IFLA_CAN_BITRATE_CONST,
>  	IFLA_CAN_DATA_BITRATE_CONST,
>  	IFLA_CAN_BITRATE_MAX,
> -	__IFLA_CAN_MAX
> -};
> +	IFLA_CAN_TDC,
>  
> -#define IFLA_CAN_MAX	(__IFLA_CAN_MAX - 1)
> +	/* add new constants above here */
> +	__IFLA_CAN_MAX,
> +	IFLA_CAN_MAX = __IFLA_CAN_MAX - 1
> +};
>  
>  /* u16 termination range: 1..65535 Ohms */
>  #define CAN_TERMINATION_DISABLED 0
>  
> +/*
> + * CAN FD Transmitter Delay Compensation (TDC)
> + */
> +enum {
> +	IFLA_CAN_TDC_UNSPEC,
> +	IFLA_CAN_TDC_TDCV_MIN,	/* u32 */
> +	IFLA_CAN_TDC_TDCV_MAX,	/* u32 */
> +	IFLA_CAN_TDC_TDCO_MIN,	/* u32 */
> +	IFLA_CAN_TDC_TDCO_MAX,	/* u32 */
> +	IFLA_CAN_TDC_TDCF_MIN,	/* u32 */
> +	IFLA_CAN_TDC_TDCF_MAX,	/* u32 */
> +	IFLA_CAN_TDC_TDCV,	/* u32 */
> +	IFLA_CAN_TDC_TDCO,	/* u32 */
> +	IFLA_CAN_TDC_TDCF,	/* u32 */
> +
> +	/* add new constants above here */
> +	__IFLA_CAN_TDC,
> +	IFLA_CAN_TDC_MAX = __IFLA_CAN_TDC - 1
> +};
> +
>  #endif /* !_UAPI_CAN_NETLINK_H */
> diff --git a/ip/iplink_can.c b/ip/iplink_can.c
> index 00f585c6..8ea0a526 100644
> --- a/ip/iplink_can.c
> +++ b/ip/iplink_can.c
> @@ -28,6 +28,7 @@ static void print_usage(FILE *f)
>  		"\n"
>  		"\t[ dbitrate BITRATE [ dsample-point SAMPLE-POINT] ] |\n"
>  		"\t[ dtq TQ dprop-seg PROP_SEG dphase-seg1 PHASE-SEG1\n
> \t  dphase-seg2 PHASE-SEG2 [ dsjw SJW ] ]\n"
> +		"\t[ tdcv TDCV tdco TDCO tdcf TDCF ]\n"
>  		"\n"
>  		"\t[ loopback { on | off } ]\n"
>  		"\t[ listen-only { on | off } ]\n"
> @@ -38,6 +39,7 @@ static void print_usage(FILE *f)
>  		"\t[ fd-non-iso { on | off } ]\n"
>  		"\t[ presume-ack { on | off } ]\n"
>  		"\t[ cc-len8-dlc { on | off } ]\n"
> +		"\t[ tdc-mode { auto | manual | off } ]\n"
>  		"\n"
>  		"\t[ restart-ms TIME-MS ]\n"
>  		"\t[ restart ]\n"
> @@ -51,6 +53,9 @@ static void print_usage(FILE *f)
>  		"\t	  PHASE-SEG1	:= { NUMBER }\n"
>  		"\t	  PHASE-SEG2	:= { NUMBER }\n"
>  		"\t	  SJW		:= { NUMBER }\n"
> +		"\t	  TDCV		:= { NUMBER }\n"
> +		"\t	  TDCO		:= { NUMBER }\n"
> +		"\t	  TDCF		:= { NUMBER }\n"
>  		"\t	  RESTART-MS	:= { 0 | NUMBER }\n"
>  		);
>  }
> @@ -105,6 +110,8 @@ static void print_ctrlmode(FILE *f, __u32 cm)
>  	_PF(CAN_CTRLMODE_FD_NON_ISO, "FD-NON-ISO");
>  	_PF(CAN_CTRLMODE_PRESUME_ACK, "PRESUME-ACK");
>  	_PF(CAN_CTRLMODE_CC_LEN8_DLC, "CC-LEN8-DLC");
> +	_PF(CAN_CTRLMODE_TDC_AUTO, "TDC_AUTO");
> +	_PF(CAN_CTRLMODE_TDC_MANUAL, "TDC_MANUAL");
>  #undef _PF
>  	if (cm)
>  		print_hex(PRINT_ANY, NULL, "%x", cm);
> @@ -116,6 +123,8 @@ static int can_parse_opt(struct link_util *lu, int argc,
> char **argv,
>  {
>  	struct can_bittiming bt = {}, dbt = {};
>  	struct can_ctrlmode cm = {0, 0};
> +	struct rtattr *tdc;
> +	__u32 tdcv = -1, tdco = -1, tdcf = -1;
>  
>  	while (argc > 0) {
>  		if (matches(*argv, "bitrate") == 0) {
> @@ -181,6 +190,18 @@ static int can_parse_opt(struct link_util *lu, int argc,
> char **argv,
>  			NEXT_ARG();
>  			if (get_u32(&dbt.sjw, *argv, 0))
>  				invarg("invalid \"dsjw\" value\n", *argv);
> +		} else if (matches(*argv, "tdcv") == 0) {
> +			NEXT_ARG();
> +			if (get_u32(&tdcv, *argv, 0))
> +				invarg("invalid \"tdcv\" value\n", *argv);
> +		} else if (matches(*argv, "tdco") == 0) {
> +			NEXT_ARG();
> +			if (get_u32(&tdco, *argv, 0))
> +				invarg("invalid \"tdco\" value\n", *argv);
> +		} else if (matches(*argv, "tdcf") == 0) {
> +			NEXT_ARG();
> +			if (get_u32(&tdcf, *argv, 0))
> +				invarg("invalid \"tdcf\" value\n", *argv);
>  		} else if (matches(*argv, "loopback") == 0) {
>  			NEXT_ARG();
>  			set_ctrlmode("loopback", *argv, &cm,
> @@ -217,6 +238,23 @@ static int can_parse_opt(struct link_util *lu, int argc,
> char **argv,
>  			NEXT_ARG();
>  			set_ctrlmode("cc-len8-dlc", *argv, &cm,
>  				     CAN_CTRLMODE_CC_LEN8_DLC);
> +		} else if (matches(*argv, "tdc-mode") == 0) {
> +			NEXT_ARG();
> +			if (strcmp(*argv, "auto") == 0) {
> +				cm.flags |= CAN_CTRLMODE_TDC_AUTO;
> +				cm.mask |= CAN_CTRLMODE_TDC_AUTO;
> +			} else if (strcmp(*argv, "manual") == 0) {
> +				cm.flags |= CAN_CTRLMODE_TDC_MANUAL;
> +				cm.mask |= CAN_CTRLMODE_TDC_MANUAL;
> +			} else if (strcmp(*argv, "off") == 0) {
> +				cm.mask |= CAN_CTRLMODE_TDC_AUTO |
> +					   CAN_CTRLMODE_TDC_MANUAL;
> +			} else {
> +				fprintf(stderr,
> +					"Error: argument of \"tdc-mode\" must be
> \"auto\", \"manual\" or \"off\", not \"%s\"\n",
> +					*argv);
> +				exit (-1);
> +			}
>  		} else if (matches(*argv, "restart") == 0) {
>  			__u32 val = 1;
>  
> @@ -254,6 +292,17 @@ static int can_parse_opt(struct link_util *lu, int argc,
> char **argv,
>  	if (cm.mask)
>  		addattr_l(n, 1024, IFLA_CAN_CTRLMODE, &cm, sizeof(cm));
>  
> +	if (tdcv != -1 || tdco != -1 || tdcf != -1) {
> +		tdc = addattr_nest(n, 1024, IFLA_CAN_TDC | NLA_F_NESTED);
> +		if (tdcv != -1)
> +			addattr32(n, 1024, IFLA_CAN_TDC_TDCV, tdcv);
> +		if (tdco != -1)
> +			addattr32(n, 1024, IFLA_CAN_TDC_TDCO, tdco);
> +		if (tdcf != -1)
> +			addattr32(n, 1024, IFLA_CAN_TDC_TDCF, tdcf);
> +		addattr_nest_end(n, tdc);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -293,6 +342,62 @@ static void can_print_data_timing_min_max(const char
> *attr, int min, int max)
>  	__can_print_timing_min_max(attr, min, max);
>  }
>  
> +static void can_print_tdc_opt(FILE *f, struct rtattr *tdc_attr)
> +{
> +	struct rtattr *tb[IFLA_CAN_TDC_MAX + 1];
> +
> +	parse_rtattr_nested(tb, IFLA_CAN_TDC_MAX, tdc_attr);
> +	if (tb[IFLA_CAN_TDC_TDCV] || tb[IFLA_CAN_TDC_TDCO] ||
> +	    tb[IFLA_CAN_TDC_TDCF]) {
> +		open_json_object("tdc");
> +		can_print_nl_indent();
> +		if (tb[IFLA_CAN_TDC_TDCV]) {
> +			__u32 *tdcv = RTA_DATA(tb[IFLA_CAN_TDC_TDCV]);
> +
> +			print_uint(PRINT_ANY, "tdcv", " tdcv %u", *tdcv);
> +		}
> +		if (tb[IFLA_CAN_TDC_TDCO]) {
> +			__u32 *tdco = RTA_DATA(tb[IFLA_CAN_TDC_TDCO]);
> +
> +			print_uint(PRINT_ANY, "tdco", " tdco %u", *tdco);
> +		}
> +		if (tb[IFLA_CAN_TDC_TDCF]) {
> +			__u32 *tdcf = RTA_DATA(tb[IFLA_CAN_TDC_TDCF]);
> +
> +			print_uint(PRINT_ANY, "tdcf", " tdcf %u", *tdcf);
> +		}
> +		close_json_object();
> +	}
> +}
> +
> +static void can_print_tdc_const_opt(FILE *f, struct rtattr *tdc_attr)
> +{
> +	struct rtattr *tb[IFLA_CAN_TDC_MAX + 1];
> +
> +	parse_rtattr_nested(tb, IFLA_CAN_TDC_MAX, tdc_attr);
> +	open_json_object("tdc");
> +	can_print_nl_indent();
> +	if (tb[IFLA_CAN_TDC_TDCV_MIN] && tb[IFLA_CAN_TDC_TDCV_MAX]) {
> +		__u32 *tdcv_min = RTA_DATA(tb[IFLA_CAN_TDC_TDCV_MIN]);
> +		__u32 *tdcv_max = RTA_DATA(tb[IFLA_CAN_TDC_TDCV_MAX]);
> +
> +		can_print_timing_min_max("tdcv", *tdcv_min, *tdcv_max);
> +	}
> +	if (tb[IFLA_CAN_TDC_TDCO_MIN] && tb[IFLA_CAN_TDC_TDCO_MAX]) {
> +		__u32 *tdco_min = RTA_DATA(tb[IFLA_CAN_TDC_TDCO_MIN]);
> +		__u32 *tdco_max = RTA_DATA(tb[IFLA_CAN_TDC_TDCO_MAX]);
> +
> +		can_print_timing_min_max("tdco", *tdco_min, *tdco_max);
> +	}
> +	if (tb[IFLA_CAN_TDC_TDCF_MIN] && tb[IFLA_CAN_TDC_TDCF_MAX]) {
> +		__u32 *tdcf_min = RTA_DATA(tb[IFLA_CAN_TDC_TDCF_MIN]);
> +		__u32 *tdcf_max = RTA_DATA(tb[IFLA_CAN_TDC_TDCF_MAX]);
> +
> +		can_print_timing_min_max("tdcf", *tdcf_min, *tdcf_max);
> +	}
> +	close_json_object();
> +}
> +
>  static void can_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
>  {
>  	if (!tb)
> @@ -423,6 +528,10 @@ static void can_print_opt(struct link_util *lu, FILE *f,
> struct rtattr *tb[])
>  			   dbt->phase_seg2);
>  		print_uint(PRINT_ANY, "sjw", " dsjw %u", dbt->sjw);
>  		print_uint(PRINT_ANY, "brp", " dbrp %u", dbt->brp);
> +
> +		if (tb[IFLA_CAN_TDC])
> +			can_print_tdc_opt(f, tb[IFLA_CAN_TDC]);
> +
>  		close_json_object();
>  	}
>  
> @@ -443,6 +552,10 @@ static void can_print_opt(struct link_util *lu, FILE *f,
> struct rtattr *tb[])
>  		can_print_data_timing_min_max("brp",
>  					      dbtc->brp_min, dbtc->brp_max);
>  		print_uint(PRINT_ANY, "brp_inc", " brp_inc %u", dbtc->brp_inc);
> +
> +		if (tb[IFLA_CAN_TDC])
> +			can_print_tdc_const_opt(f, tb[IFLA_CAN_TDC]);
> +
>  		close_json_object();
>  	}
>  

Re: [RFC PATCH v4 4/4] iplink_can: add new CAN FD bittiming parameters: Transmitter Delay Compensation (TDC)

[Vincent MAILHOL]

Hi Stefan,

On Wed. 7 Jul 2021 at 18:21, Stefan Mätje <Stefan.Maetje@esd.eu> wrote:
> Hi Vincent,
>
> it took some time to get an opinion on the proposed implementation of the user
> interface to control TDC.

Thanks for your involvement, I am always happy to get constructive feedback!

> At first I've inspected the driver implementations for the following CAN cores
> in the Linux tree (FlexCAN, ifi_canfd, MCAN, mcp251xfd) on how the TDC is
> implemented currently. All the drivers for these controllers by default use the
> automatic measurement of TDCV and set up the TDCO value in a way that the SSP
> occurs in the delayed received bit at the same position as the sample point is
> set for the remote receivers (provided the same bitrate setting is used).

ACK. This means that TDCO = SP and that:
  SSP = TDCV + TDCO
      = TDCV + SP

And that’s also the formula I used in can_calc_tdco():
https://elixir.bootlin.com/linux/latest/source/drivers/net/can/dev/bittiming.c#L178

> This is the default mode for TDC, i. e. the TDC is always enabled in FD mode
> (sometimes only for higher data bitrates) and the TDCO is derived from the data
> bitrate settings. This works and the measurement of TDCV compensates for changes
> in delay over time and temperature. This is also the recommended setting of the
> SSP.

Out of curiosity, when you say that this is the "recommended
setting", do you have any references? I agree that this is the
most natural way to calculate TDC but I never found good
references.

> So the opportunity to change the TDC control values is only needed in a test
> enviroment or when trying to tune the behavior to evaluate corner cases.
>
> Need to mention that the TDCV and TDCO values (if implemented) for all mentioned
> controllers are in CAN clock period (Tc) counts and NOT in time quanta (Tq)
> counts!
> The CAN clock period (Tc) also defines the "minimum time quantum" which
> is referenced in the ISO 11898 description of the TDC operation.

Thanks for the clarification. I completely missed the nuances
between the "time quantum" and the "minimum time quantum" and
wrongly assumed that the TDC parameters were also expressed in
time quanta similar to the other bittiming parameters.

Using the time quantum instead of the clock period is my mistake.

> See below for further comments ...
>
> Best regards,
>     Stefan
>
>
> Am Dienstag, den 29.06.2021, 00:44 +0900 schrieb 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 brings command line support to nine TDC parameters which
> > were recently added to the kernel's CAN netlink interface in order to
> > implement TDC:
> >   - IFLA_CAN_TDC_TDCV_MIN: Transmitter Delay Compensation Value
> >     minimum value
> >   - IFLA_CAN_TDC_TDCV_MAX: Transmitter Delay Compensation Value
> >     maximum value
> >   - IFLA_CAN_TDC_TDCO_MIN: Transmitter Delay Compensation Offset
> >     minimum value
> >   - IFLA_CAN_TDC_TDCO_MAX: Transmitter Delay Compensation Offset
> >     maximum value
> >   - IFLA_CAN_TDC_TDCF_MIN: Transmitter Delay Compensation Filter
> >     window minimum value
> >   - IFLA_CAN_TDC_TDCF_MAX: Transmitter Delay Compensation Filter
> >     window maximum value
> >   - IFLA_CAN_TDC_TDCV: Transmitter Delay Compensation Value
> >   - IFLA_CAN_TDC_TDCO: Transmitter Delay Compensation Offset
> >   - IFLA_CAN_TDC_TDCF: Transmitter Delay Compensation Filter window
> >
> > TDCV is reported only if the device supports
> > CAN_CTRLMODE_TDC_MANUAL.
>
> I think the TDCV value should be reported in any case. Assume the interface is
> in default mode (as described before and selected by omitting the tdc-mode
> parameter) or in your auto mode then the TDCV value reported should be the
> measured value of the TDCV if available. This could then serve as a useful
> starting point for manual configuration experiments. If the measured TDCV is not
> available from the controller registers it should return zero.

Your comment is valid. The reason I did not implement it is
because I do not own a device capable of reporting the value.

I can propose to add a virtual function in struct can_priv:
+

 int (*do_get_auto_tdcv)(struct net_device *dev);

If the function is implemented, use it to report TDCV when in auto mode.

I disagree on one point: if the value is not available, I think
it is better not report TDCV rather than reporting zero. As I
commented in my other patches, zero is a valid value for TDCV, we
can not use it to mean "not available" or "not set".

> A similar idea holds true for the TDCO value. When the driver calulates a
> suitable value for the TDCO (in default mode) the "ip -details link show" should
> report the TDCO value set by the driver.

That should be the current behavior. In either auto or manual
modes, the TDCO value inputted by the user should be returned. In
default (i.e. no parameters calculated), the TDCO value is
reported only if TDC is on (c.f. can_calculate_tdco() function).
Did you witnessed TDCO not being reported after TDC being set?

> > TDCF is reported only if tdcf_max is not
> > zero.
>
> Note: I think TDCF is only supported by MCAN so far.

FYI, the SAM E70/S70/V70/V71 Family from Microchip also support TDCF:
https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/etas_es58x/es58x_fd.c#L511

> > All those new parameters are nested together into the attribute
> > IFLA_CAN_TDC.
> >
> > A tdc-mode parameter allow to specify how to opperate. Valid options
> > are:
> >
> >   * auto: the transmitter automatically measures TDCV. As such, TDCV
> >     values can not be manually provided. In this mode, the user must
> >     specify TDCO and may also specify TDCF if supported.
> >
> >   * manual: Use the TDCV value provided by the user are used. In this
> >     mode, the user must specify both TDCF and TDCO and may also
> >     specify TDCF if supported.
> >
> >   * off: TDC is explicitely disabled.
> >
> >   * tdc-mode parameter omitted: the kernel decides whether TDC should
> >     be enabled or not and if so, it calculates the TDC values.
>
> This mode with tdc-mode parameter omitted I have called "default mode" in my
> introduction.

ACK. This is also my intent to use it as default mode.
I will add a comment to the patch description to reflect this.

> For reference, here are a few samples of how the output looks like:
> >
> > $ ip link set can0 type can bitrate 1000000 dbitrate 8000000 fd on tdco 7 tdcf
> > 8 tdc-mode auto
>
> So switching from a setting with "tdc-mode auto" (like before) back to the
> default mode should then be done by
>
> $ ip link set can0 type can bitrate 1000000 dbitrate 8000000 fd on

Yes

> But as far as I understand without any TDC parameters the ip tool does not send
> the IFLA_CAN_TDC structure to the kernel. So on the kernel level this is the
> signal to switch back to "default mode" for TDC?

The signal is on the CAN_CTRLMODE_TDC_{AUTO,MANUAL} flags:
  * If the flags are omitted (not set): default mode (parameter omitted)
  * If both are set to false: TDC off
  * If CAN_CTRLMODE_TDC_AUTO is set to true: TDC auto mode
  * If CAN_CTRLMODE_TDC_MANUAL is set to true: TDC manual mode

N.B.: The kernel forbids both CAN_CTRLMODE_TDC_{AUTO,MANUAL} to be
both set to true at the same time.

> >
> > $ ip --details link show can0
> > 1:  can0: <NOARP,ECHO> mtu 72 qdisc noop state DOWN mode DEFAULT group default
> > qlen 10
> >     link/can  promiscuity 0 minmtu 0 maxmtu 0
> >     can <FD,TDC_AUTO> state STOPPED (berr-counter tx 0 rx 0) restart-ms 0
> >         bitrate 1000000 sample-point 0.750
> >         tq 12 prop-seg 29 phase-seg1 30phase-seg2 20  sjw 1 brp 1
> >         ES582.1/ES584.1: tseg1 2..256 tseg2 2..128 sjw 1..128 brp 1..512
> > brp_inc 1
> >         dbitrate 8000000 dsample-point 0.700
> >         dtq 12 dprop-seg 3 dphase-seg1 3 dphase-seg2 3 dsjw 1 dbrp 1
> >         tdco 7 tdcf 8
> >         ES582.1/ES584.1: dtseg1 2..32 dtseg2 1..16 dsjw 1..8 dbrp 1..32
> > brp_inc 1
> >         tdco 0..127 tdcf 0..127
> >         clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536
> > gso_max_segs 65535
>
> After looking at this I have a concern on the design decision to make the
> transferred TDCV/TDCO/TDCF values defined as multiple of time quanta (Tq).
>
> As I mentioned before the all controllers use register values based on multiples
> of CAN clock periods (Tc). And in the IFLA_CAN_TDC_TDCx_{MIN|MAX} or "struct
> can_tdc_const" tables shown as

That’s not a design decision but a mistake. I will correct and resend
a new patch.

> >         tdco 0..127 tdcf 0..127
>
> you use the maximum allowed values for the controller registers which are also
> the Tc based values.
>
> But in include/linux/can/bittiming.h the TDCx values are described as being
> multiples of time quanta (Tq).
>
> But this creates a problem with data bitrates where dbrp is 2. If this is the
> case at the end the driver needs to convert the TDCx values from Tq based
> numbers to Tc based numbers. Then you may have the case where "tdco 64" is set
> on the "ip" command line. The can_tdc_changelink() function happily compares the
> given "tdco" with the limits and lets this value pass. But some code needs to
> multiply with dbrp=2 which would then overflow the register bits.
>
> On the other hand even if can_tdc_changelink() does the conversion and the check
> already it would signal -EINVAL for a value (64) that is apparently announced as
> legal judging from the output of "ip -d link show ...".
>
> To eliminate this discrepancy "ip -d link show ..." would need to display only
> "tdco 0..63" as allowed values if dbrp==2. So the kernel possibly needs to
> convert the allowed TDCx min / max constants from Tc based values to Tq based
> values. I think this would be a nightmare when the allowed limits change based
> on the bitrate setting.
>
> So I would propose to change the documentation in include/linux/can/bittiming.h
> to say that the TDCx values are based on the CAN clock period (Tc). Then the
> code for can_tdc_changelink() checks for the correct limits. And also the help
> output of "ip link help can" should explicitely state that the TDCx values are
> NOT based on time quanta.
>
> I think the confusion of the user is less with that approach because it is not
> so far away from the documented register interface of the CAN controllers. Even
> if it would be the only point in the CAN area where settings are not based on
> time quanta (Tq) but on CAN clock periods (Tc).

ACK.

> And as I mentioned above the "normal" user should never need to set TDC values
> and always use the "default" mode. The expert user should perhaps be able to
> understand the different time bases for TDCx values and other timing parameters.

ACK. This is also my intent.

> And in a testing and evaluation environment the higher resolution of the Tc
> based TDCx values may also be a benefit. I. e. when reading the measured TDCV
> value to monitor the change of the delay over time and temperature.

From your comments, I see two immediate actions:
  * Change the unit from time quanta to clock period (Tc)
  * Add a method to retrieve TDCV value when in auto mode

Right now, I am in holiday, with no access to my usual testing
gears.  I will prepare a version of the patches but you might
have to wait until August.


Yours sincerely,
Vincent

Re: [RFC PATCH v4 4/4] iplink_can: add new CAN FD bittiming parameters: Transmitter Delay Compensation (TDC)

[Stefan Mätje]

Some late comments on this ...

Am Freitag, den 09.07.2021, 15:32 +0200 schrieb Vincent MAILHOL:
> Hi Stefan,
> 
> On Wed. 7 Jul 2021 at 18:21, Stefan Mätje <Stefan.Maetje@esd.eu> wrote:
> > Hi Vincent,
> > 
> > it took some time to get an opinion on the proposed implementation of the user
> > interface to control TDC.
> 
> Thanks for your involvement, I am always happy to get constructive feedback!
> 
> > At first I've inspected the driver implementations for the following CAN cores
> > in the Linux tree (FlexCAN, ifi_canfd, MCAN, mcp251xfd) on how the TDC is
> > implemented currently. All the drivers for these controllers by default use the
> > automatic measurement of TDCV and set up the TDCO value in a way that the SSP
> > occurs in the delayed received bit at the same position as the sample point is
> > set for the remote receivers (provided the same bitrate setting is used).
> 
> ACK. This means that TDCO = SP and that:
>   SSP = TDCV + TDCO
>       = TDCV + SP
> 
> And that’s also the formula I used in can_calc_tdco():
> https://elixir.bootlin.com/linux/latest/source/drivers/net/can/dev/bittiming.c#L178

A nitpick on the can_calc_tdco() function. There you calculate the sample_point_in_tq
like this:

	u32 sample_point_in_tq = can_bit_time(dbt) * dbt->sample_point / 1000;

You use there dbt->sample_point which is a calculated value and the whole formula may
undergo rounding and truncation (even already the input value dbt->sample_point has).
A formula where you even don't need to think about this issue would be:

	u32 sample_point_in_tq = can_bit_time(dbt) - dbt->phase_seg2;

> 
> > This is the default mode for TDC, i. e. the TDC is always enabled in FD mode
> > (sometimes only for higher data bitrates) and the TDCO is derived from the data
> > bitrate settings. This works and the measurement of TDCV compensates for changes
> > in delay over time and temperature. This is also the recommended setting of the
> > SSP.
> 
> Out of curiosity, when you say that this is the "recommended
> setting", do you have any references? I agree that this is the
> most natural way to calculate TDC but I never found good
> references.

The basic idea behind the setting of the TDCO is that all nodes on the bus (the
transmitter and the receiving nodes) should sample their received bits at the same
offset during the bit-time. Because the TDCV value compensates the tranmitter round
trip delay (TX -> RX pin) the TDCO value should be the number of CAN clock cycles
to reach the Sample Point Offset (SP) in the bit time which is 

TDCO = (1 SYNC_Tq + PROP_Segment_Tqs + TSEG1_Tqs) * DBRP

I found a recommendation for this in the FlexCAN controller's description in the 
reference manual for a NXP embedded controller family:

S32K1xx Series Reference Manual, Document Number: S32K1XXRM, Rev. 13, 04/2020

On page 1921, section "55.5.9.3 Transceiver delay compensation" you can find this 
recommendated formula:

	Offset = (FPSEG1 + FPROPSEG + 2) x (FPRESDIV + 1)

which boils down to the formula for TDCO shown before if you replace the register
contents of the formula for "Offset" by the "real" values shown in the formula
for TDCO.

Written differently with "real" values in {} (compare the cited reference manual
on page 1874 about the register contents):

	Offset = ({FPSEG1 +1} + {FPROPSEG} + {SYNC_Tq}) * ({FPRESDIV + 1})
	Offset = (TSEG1_Tqs + Prop_Segment_Tqs + SYNC_Tq) * (DBRP)


> > So the opportunity to change the TDC control values is only needed in a test
> > enviroment or when trying to tune the behavior to evaluate corner cases.
> > 
> > Need to mention that the TDCV and TDCO values (if implemented) for all mentioned
> > controllers are in CAN clock period (Tc) counts and NOT in time quanta (Tq)
> > counts!
> > The CAN clock period (Tc) also defines the "minimum time quantum" which
> > is referenced in the ISO 11898 description of the TDC operation.
> 
> Thanks for the clarification. I completely missed the nuances
> between the "time quantum" and the "minimum time quantum" and
> wrongly assumed that the TDC parameters were also expressed in
> time quanta similar to the other bittiming parameters.
> 
> Using the time quantum instead of the clock period is my mistake.
> 
> > See below for further comments ...
> > 
> > Best regards,
> >     Stefan
> > 
> > 
> > Am Dienstag, den 29.06.2021, 00:44 +0900 schrieb 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 brings command line support to nine TDC parameters which
> > > were recently added to the kernel's CAN netlink interface in order to
> > > implement TDC:
> > >   - IFLA_CAN_TDC_TDCV_MIN: Transmitter Delay Compensation Value
> > >     minimum value
> > >   - IFLA_CAN_TDC_TDCV_MAX: Transmitter Delay Compensation Value
> > >     maximum value
> > >   - IFLA_CAN_TDC_TDCO_MIN: Transmitter Delay Compensation Offset
> > >     minimum value
> > >   - IFLA_CAN_TDC_TDCO_MAX: Transmitter Delay Compensation Offset
> > >     maximum value
> > >   - IFLA_CAN_TDC_TDCF_MIN: Transmitter Delay Compensation Filter
> > >     window minimum value
> > >   - IFLA_CAN_TDC_TDCF_MAX: Transmitter Delay Compensation Filter
> > >     window maximum value
> > >   - IFLA_CAN_TDC_TDCV: Transmitter Delay Compensation Value
> > >   - IFLA_CAN_TDC_TDCO: Transmitter Delay Compensation Offset
> > >   - IFLA_CAN_TDC_TDCF: Transmitter Delay Compensation Filter window
> > > 
> > > TDCV is reported only if the device supports
> > > CAN_CTRLMODE_TDC_MANUAL.
> > 
> > I think the TDCV value should be reported in any case. Assume the interface is
> > in default mode (as described before and selected by omitting the tdc-mode
> > parameter) or in your auto mode then the TDCV value reported should be the
> > measured value of the TDCV if available. This could then serve as a useful
> > starting point for manual configuration experiments. If the measured TDCV is not
> > available from the controller registers it should return zero.
> 
> Your comment is valid. The reason I did not implement it is
> because I do not own a device capable of reporting the value.
> 
> I can propose to add a virtual function in struct can_priv:
> +
> 
>  int (*do_get_auto_tdcv)(struct net_device *dev);
> 
> If the function is implemented, use it to report TDCV when in auto mode.

I also think that this function is needed because when the netlink interface
call runs through the kernel there must be driver / controller specific code
to extract the current TDCV value from the CAN controller.


> I disagree on one point: if the value is not available, I think
> it is better not report TDCV rather than reporting zero. As I
> commented in my other patches, zero is a valid value for TDCV, we
> can not use it to mean "not available" or "not set".

This is fine with me. But aren't the TDCx values put in a single structure
(NFLA_NESTED...?). And how can the user level detect that the TDCV value
is provided or not?

Ok, on a second look it seems that there is the choice to add the IFLA_CAN_TDC_TDCV
"structure" in can_fill_info() or not ...


> > A similar idea holds true for the TDCO value. When the driver calulates a
> > suitable value for the TDCO (in default mode) the "ip -details link show" should
> > report the TDCO value set by the driver.
> 
> That should be the current behavior. In either auto or manual
> modes, the TDCO value inputted by the user should be returned. In
> default (i.e. no parameters calculated), the TDCO value is
> reported only if TDC is on (c.f. can_calculate_tdco() function).
> Did you witnessed TDCO not being reported after TDC being set?
> 
> > > TDCF is reported only if tdcf_max is not
> > > zero.
> > 
> > Note: I think TDCF is only supported by MCAN so far.
> 
> FYI, the SAM E70/S70/V70/V71 Family from Microchip also support TDCF:
> https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/etas_es58x/es58x_fd.c#L511

I would like to mention that the controller implemented in the SAM E70* controller
family is a MCAN implementation / IP core. So I think we're both right ;-)


> > > All those new parameters are nested together into the attribute
> > > IFLA_CAN_TDC.
> > > 
> > > A tdc-mode parameter allow to specify how to opperate. Valid options
> > > are:
> > > 
> > >   * auto: the transmitter automatically measures TDCV. As such, TDCV
> > >     values can not be manually provided. In this mode, the user must
> > >     specify TDCO and may also specify TDCF if supported.
> > > 
> > >   * manual: Use the TDCV value provided by the user are used. In this
> > >     mode, the user must specify both TDCF and TDCO and may also
> > >     specify TDCF if supported.
> > > 
> > >   * off: TDC is explicitely disabled.
> > > 
> > >   * tdc-mode parameter omitted: the kernel decides whether TDC should
> > >     be enabled or not and if so, it calculates the TDC values.
> > 
> > This mode with tdc-mode parameter omitted I have called "default mode" in my
> > introduction.
> 
> ACK. This is also my intent to use it as default mode.
> I will add a comment to the patch description to reflect this.
> 
> > For reference, here are a few samples of how the output looks like:
> > > 
> > > $ ip link set can0 type can bitrate 1000000 dbitrate 8000000 fd on tdco 7 tdcf
> > > 8 tdc-mode auto
> > 
> > So switching from a setting with "tdc-mode auto" (like before) back to the
> > default mode should then be done by
> > 
> > $ ip link set can0 type can bitrate 1000000 dbitrate 8000000 fd on
> 
> Yes
> 
> > But as far as I understand without any TDC parameters the ip tool does not send
> > the IFLA_CAN_TDC structure to the kernel. So on the kernel level this is the
> > signal to switch back to "default mode" for TDC?
> 
> The signal is on the CAN_CTRLMODE_TDC_{AUTO,MANUAL} flags:
>   * If the flags are omitted (not set): default mode (parameter omitted)
>   * If both are set to false: TDC off
>   * If CAN_CTRLMODE_TDC_AUTO is set to true: TDC auto mode
>   * If CAN_CTRLMODE_TDC_MANUAL is set to true: TDC manual mode
> 
> N.B.: The kernel forbids both CAN_CTRLMODE_TDC_{AUTO,MANUAL} to be
> both set to true at the same time.
:
Lots of stuff cut away
:
> > So I would propose to change the documentation in include/linux/can/bittiming.h
> > to say that the TDCx values are based on the CAN clock period (Tc). Then the
> > code for can_tdc_changelink() checks for the correct limits. And also the help
> > output of "ip link help can" should explicitely state that the TDCx values are
> > NOT based on time quanta.
> > 
> > I think the confusion of the user is less with that approach because it is not
> > so far away from the documented register interface of the CAN controllers. Even
> > if it would be the only point in the CAN area where settings are not based on
> > time quanta (Tq) but on CAN clock periods (Tc).
> 
> ACK.
> 
> > And as I mentioned above the "normal" user should never need to set TDC values
> > and always use the "default" mode. The expert user should perhaps be able to
> > understand the different time bases for TDCx values and other timing parameters.
> 
> ACK. This is also my intent.
> 
> > And in a testing and evaluation environment the higher resolution of the Tc
> > based TDCx values may also be a benefit. I. e. when reading the measured TDCV
> > value to monitor the change of the delay over time and temperature.
> 
> From your comments, I see two immediate actions:
>   * Change the unit from time quanta to clock period (Tc)
>   * Add a method to retrieve TDCV value when in auto mode

That would be cool. Thanks.

> Right now, I am in holiday, with no access to my usual testing
> gears.  I will prepare a version of the patches but you might
> have to wait until August.
> 
> 
> Yours sincerely,
> Vincent

Hope you had nice holidays ...

Best regards,
    Stefan

-- 
Stefan Mätje
System Design

Phone: +49-511-37298-146
E-Mail: stefan.maetje@esd.eu
_______________________________________
esd electronics gmbh
Vahrenwalder Str. 207
30165 Hannover
www.esd.eu

Quality Products – Made in Germany
_______________________________________

Register Hannover HRB 51373 - VAT-ID DE 115672832
General Manager: Klaus Detering

Re: [RFC PATCH v4 4/4] iplink_can: add new CAN FD bittiming parameters: Transmitter Delay Compensation (TDC)

[Vincent MAILHOL]

Hi Stefan,

On Mon. 9 Aug. 2021 at 23:44, Stefan Mätje <Stefan.Maetje@esd.eu> wrote:
> Some late comments on this ...

Your comments arrived just at the good timing, when I was
starting to work on the new version of the patch series.

> Am Freitag, den 09.07.2021, 15:32 +0200 schrieb Vincent MAILHOL:
> > Hi Stefan,
> >
> > On Wed. 7 Jul 2021 at 18:21, Stefan Mätje <Stefan.Maetje@esd.eu> wrote:
> > > Hi Vincent,
> > >
> > > it took some time to get an opinion on the proposed implementation of the user
> > > interface to control TDC.
> >
> > Thanks for your involvement, I am always happy to get constructive feedback!
> >
> > > At first I've inspected the driver implementations for the following CAN cores
> > > in the Linux tree (FlexCAN, ifi_canfd, MCAN, mcp251xfd) on how the TDC is
> > > implemented currently. All the drivers for these controllers by default use the
> > > automatic measurement of TDCV and set up the TDCO value in a way that the SSP
> > > occurs in the delayed received bit at the same position as the sample point is
> > > set for the remote receivers (provided the same bitrate setting is used).
> >
> > ACK. This means that TDCO = SP and that:
> >   SSP = TDCV + TDCO
> >       = TDCV + SP
> >
> > And that’s also the formula I used in can_calc_tdco():
> > https://elixir.bootlin.com/linux/latest/source/drivers/net/can/dev/bittiming.c#L178
>
> A nitpick on the can_calc_tdco() function. There you calculate the sample_point_in_tq
> like this:
>
>         u32 sample_point_in_tq = can_bit_time(dbt) * dbt->sample_point / 1000;
>
> You use there dbt->sample_point which is a calculated value and the whole formula may
> undergo rounding and truncation (even already the input value dbt->sample_point has).
> A formula where you even don't need to think about this issue would be:
>
>         u32 sample_point_in_tq = can_bit_time(dbt) - dbt->phase_seg2;

Thanks for this remark. Regardless, I had to rewrite the formula
to change from time quantum to clock period. At the end, I opted
for below formula:

u32 sample_point_in_tc = (CAN_SYNC_SEG + dbt->prop_seg +
                          dbt->phase_seg1) * dbt->brp;

> >
> > > This is the default mode for TDC, i. e. the TDC is always enabled in FD mode
> > > (sometimes only for higher data bitrates) and the TDCO is derived from the data
> > > bitrate settings. This works and the measurement of TDCV compensates for changes
> > > in delay over time and temperature. This is also the recommended setting of the
> > > SSP.
> >
> > Out of curiosity, when you say that this is the "recommended
> > setting", do you have any references? I agree that this is the
> > most natural way to calculate TDC but I never found good
> > references.
>
> The basic idea behind the setting of the TDCO is that all nodes on the bus (the
> transmitter and the receiving nodes) should sample their received bits at the same
> offset during the bit-time. Because the TDCV value compensates the tranmitter round
> trip delay (TX -> RX pin) the TDCO value should be the number of CAN clock cycles
> to reach the Sample Point Offset (SP) in the bit time which is
>
> TDCO = (1 SYNC_Tq + PROP_Segment_Tqs + TSEG1_Tqs) * DBRP
>
> I found a recommendation for this in the FlexCAN controller's description in the
> reference manual for a NXP embedded controller family:
>
> S32K1xx Series Reference Manual, Document Number: S32K1XXRM, Rev. 13, 04/2020
>
> On page 1921, section "55.5.9.3 Transceiver delay compensation" you can find this
> recommendated formula:
>
>         Offset = (FPSEG1 + FPROPSEG + 2) x (FPRESDIV + 1)
>
> which boils down to the formula for TDCO shown before if you replace the register
> contents of the formula for "Offset" by the "real" values shown in the formula
> for TDCO.
>
> Written differently with "real" values in {} (compare the cited reference manual
> on page 1874 about the register contents):
>
>         Offset = ({FPSEG1 +1} + {FPROPSEG} + {SYNC_Tq}) * ({FPRESDIV + 1})
>         Offset = (TSEG1_Tqs + Prop_Segment_Tqs + SYNC_Tq) * (DBRP)

Thanks for the reference.

> > > So the opportunity to change the TDC control values is only needed in a test
> > > enviroment or when trying to tune the behavior to evaluate corner cases.
> > >
> > > Need to mention that the TDCV and TDCO values (if implemented) for all mentioned
> > > controllers are in CAN clock period (Tc) counts and NOT in time quanta (Tq)
> > > counts!
> > > The CAN clock period (Tc) also defines the "minimum time quantum" which
> > > is referenced in the ISO 11898 description of the TDC operation.
> >
> > Thanks for the clarification. I completely missed the nuances
> > between the "time quantum" and the "minimum time quantum" and
> > wrongly assumed that the TDC parameters were also expressed in
> > time quanta similar to the other bittiming parameters.
> >
> > Using the time quantum instead of the clock period is my mistake.
> >
> > > See below for further comments ...
> > >
> > > Best regards,
> > >     Stefan
> > >
> > >
> > > Am Dienstag, den 29.06.2021, 00:44 +0900 schrieb 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 brings command line support to nine TDC parameters which
> > > > were recently added to the kernel's CAN netlink interface in order to
> > > > implement TDC:
> > > >   - IFLA_CAN_TDC_TDCV_MIN: Transmitter Delay Compensation Value
> > > >     minimum value
> > > >   - IFLA_CAN_TDC_TDCV_MAX: Transmitter Delay Compensation Value
> > > >     maximum value
> > > >   - IFLA_CAN_TDC_TDCO_MIN: Transmitter Delay Compensation Offset
> > > >     minimum value
> > > >   - IFLA_CAN_TDC_TDCO_MAX: Transmitter Delay Compensation Offset
> > > >     maximum value
> > > >   - IFLA_CAN_TDC_TDCF_MIN: Transmitter Delay Compensation Filter
> > > >     window minimum value
> > > >   - IFLA_CAN_TDC_TDCF_MAX: Transmitter Delay Compensation Filter
> > > >     window maximum value
> > > >   - IFLA_CAN_TDC_TDCV: Transmitter Delay Compensation Value
> > > >   - IFLA_CAN_TDC_TDCO: Transmitter Delay Compensation Offset
> > > >   - IFLA_CAN_TDC_TDCF: Transmitter Delay Compensation Filter window
> > > >
> > > > TDCV is reported only if the device supports
> > > > CAN_CTRLMODE_TDC_MANUAL.
> > >
> > > I think the TDCV value should be reported in any case. Assume the interface is
> > > in default mode (as described before and selected by omitting the tdc-mode
> > > parameter) or in your auto mode then the TDCV value reported should be the
> > > measured value of the TDCV if available. This could then serve as a useful
> > > starting point for manual configuration experiments. If the measured TDCV is not
> > > available from the controller registers it should return zero.
> >
> > Your comment is valid. The reason I did not implement it is
> > because I do not own a device capable of reporting the value.
> >
> > I can propose to add a virtual function in struct can_priv:
> > +
> >
> >  int (*do_get_auto_tdcv)(struct net_device *dev);
> >
> > If the function is implemented, use it to report TDCV when in auto mode.
>
> I also think that this function is needed because when the netlink interface
> call runs through the kernel there must be driver / controller specific code
> to extract the current TDCV value from the CAN controller.
>
>
> > I disagree on one point: if the value is not available, I think
> > it is better not report TDCV rather than reporting zero. As I
> > commented in my other patches, zero is a valid value for TDCV, we
> > can not use it to mean "not available" or "not set".
>
> This is fine with me. But aren't the TDCx values put in a single structure
> (NFLA_NESTED...?). And how can the user level detect that the TDCV value
> is provided or not?
>
> Ok, on a second look it seems that there is the choice to add the IFLA_CAN_TDC_TDCV
> "structure" in can_fill_info() or not ...

Yes. And for more details, you can have a look at
can_print_tdc_opt() function in my patch. This behavior is
already implemented:

|        if (tb[IFLA_CAN_TDC_TDCV]) {
|            __u32 *tdcv = RTA_DATA(tb[IFLA_CAN_TDC_TDCV]);
|
|            print_uint(PRINT_ANY, "tdcv", " tdcv %u", *tdcv);
|        }

> > > A similar idea holds true for the TDCO value. When the driver calulates a
> > > suitable value for the TDCO (in default mode) the "ip -details link show" should
> > > report the TDCO value set by the driver.
> >
> > That should be the current behavior. In either auto or manual
> > modes, the TDCO value inputted by the user should be returned. In
> > default (i.e. no parameters calculated), the TDCO value is
> > reported only if TDC is on (c.f. can_calculate_tdco() function).
> > Did you witnessed TDCO not being reported after TDC being set?
> >
> > > > TDCF is reported only if tdcf_max is not
> > > > zero.
> > >
> > > Note: I think TDCF is only supported by MCAN so far.
> >
> > FYI, the SAM E70/S70/V70/V71 Family from Microchip also support TDCF:
> > https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/etas_es58x/es58x_fd.c#L511
>
> I would like to mention that the controller implemented in the SAM E70* controller
> family is a MCAN implementation / IP core. So I think we're both right ;-)

I never realized that. Thanks for pointing this out.

> > > > All those new parameters are nested together into the attribute
> > > > IFLA_CAN_TDC.
> > > >
> > > > A tdc-mode parameter allow to specify how to opperate. Valid options
> > > > are:
> > > >
> > > >   * auto: the transmitter automatically measures TDCV. As such, TDCV
> > > >     values can not be manually provided. In this mode, the user must
> > > >     specify TDCO and may also specify TDCF if supported.
> > > >
> > > >   * manual: Use the TDCV value provided by the user are used. In this
> > > >     mode, the user must specify both TDCF and TDCO and may also
> > > >     specify TDCF if supported.
> > > >
> > > >   * off: TDC is explicitely disabled.
> > > >
> > > >   * tdc-mode parameter omitted: the kernel decides whether TDC should
> > > >     be enabled or not and if so, it calculates the TDC values.
> > >
> > > This mode with tdc-mode parameter omitted I have called "default mode" in my
> > > introduction.
> >
> > ACK. This is also my intent to use it as default mode.
> > I will add a comment to the patch description to reflect this.
> >
> > > For reference, here are a few samples of how the output looks like:
> > > >
> > > > $ ip link set can0 type can bitrate 1000000 dbitrate 8000000 fd on tdco 7 tdcf
> > > > 8 tdc-mode auto
> > >
> > > So switching from a setting with "tdc-mode auto" (like before) back to the
> > > default mode should then be done by
> > >
> > > $ ip link set can0 type can bitrate 1000000 dbitrate 8000000 fd on
> >
> > Yes
> >
> > > But as far as I understand without any TDC parameters the ip tool does not send
> > > the IFLA_CAN_TDC structure to the kernel. So on the kernel level this is the
> > > signal to switch back to "default mode" for TDC?
> >
> > The signal is on the CAN_CTRLMODE_TDC_{AUTO,MANUAL} flags:
> >   * If the flags are omitted (not set): default mode (parameter omitted)
> >   * If both are set to false: TDC off
> >   * If CAN_CTRLMODE_TDC_AUTO is set to true: TDC auto mode
> >   * If CAN_CTRLMODE_TDC_MANUAL is set to true: TDC manual mode
> >
> > N.B.: The kernel forbids both CAN_CTRLMODE_TDC_{AUTO,MANUAL} to be
> > both set to true at the same time.
> :
> Lots of stuff cut away
> :
> > > So I would propose to change the documentation in include/linux/can/bittiming.h
> > > to say that the TDCx values are based on the CAN clock period (Tc). Then the
> > > code for can_tdc_changelink() checks for the correct limits. And also the help
> > > output of "ip link help can" should explicitely state that the TDCx values are
> > > NOT based on time quanta.
> > >
> > > I think the confusion of the user is less with that approach because it is not
> > > so far away from the documented register interface of the CAN controllers. Even
> > > if it would be the only point in the CAN area where settings are not based on
> > > time quanta (Tq) but on CAN clock periods (Tc).
> >
> > ACK.
> >
> > > And as I mentioned above the "normal" user should never need to set TDC values
> > > and always use the "default" mode. The expert user should perhaps be able to
> > > understand the different time bases for TDCx values and other timing parameters.
> >
> > ACK. This is also my intent.
> >
> > > And in a testing and evaluation environment the higher resolution of the Tc
> > > based TDCx values may also be a benefit. I. e. when reading the measured TDCV
> > > value to monitor the change of the delay over time and temperature.
> >
> > From your comments, I see two immediate actions:
> >   * Change the unit from time quanta to clock period (Tc)
> >   * Add a method to retrieve TDCV value when in auto mode
>
> That would be cool. Thanks.

That done in v4 of the series:
https://lore.kernel.org/linux-can/20210814091750.73931-1-mailhol.vincent@wanadoo.fr/T/#t

N.B. the changes are on the kernel size. I still need to prepare
a new series for iproute but that will be mostly some change in
the patch comments.

> > Right now, I am in holiday, with no access to my usual testing
> > gears.  I will prepare a version of the patches but you might
> > have to wait until August.
> >
> >
> > Yours sincerely,
> > Vincent
>
> Hope you had nice holidays ...

Yes. After more than two years, I could finally travel back to my
home country!

Yours sincerely,
Vincent