All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v5 0/4] iplink_can: cleaning, fixes and adding TDC support.
@ 2021-08-14 10:17 Vincent Mailhol
  2021-08-14 10:17 ` [PATCH v5 1/4] iplink_can: fix configuration ranges in print_usage() and add unit Vincent Mailhol
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Vincent Mailhol @ 2021-08-14 10:17 UTC (permalink / raw)
  To: Marc Kleine-Budde, Stephen Hemminger, linux-can
  Cc: Stefan Mätje, netdev, linux-kernel, 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 and add
  the units to add clarity: some parameters are in milliseconds, some
  in nano seconds, some in time quantum and the newly TDC parameters
  introduced in this series would be in clock period.

  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/20210814091750.73931-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 RFC v4 to RFC v5:
  * Add the unit (bps, tq, ns or ms) in print_usage()
  * Rewrote void can_print_timing_min_max() to better factorize the
    code.
  * Rewrote the commit message of the two last patches (those related
    to TDC) to either add clarification of fix inacurracies.

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() and add unit
  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                  | 460 +++++++++++++++++--------------
 2 files changed, 279 insertions(+), 211 deletions(-)

-- 
2.31.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v5 1/4] iplink_can: fix configuration ranges in print_usage() and add unit
  2021-08-14 10:17 [PATCH v5 0/4] iplink_can: cleaning, fixes and adding TDC support Vincent Mailhol
@ 2021-08-14 10:17 ` Vincent Mailhol
  2021-08-14 10:17 ` [PATCH v5 2/4] iplink_can: use PRINT_ANY to factorize code and fix signedness Vincent Mailhol
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 12+ messages in thread
From: Vincent Mailhol @ 2021-08-14 10:17 UTC (permalink / raw)
  To: Marc Kleine-Budde, Stephen Hemminger, linux-can
  Cc: Stefan Mätje, netdev, linux-kernel, 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 in the standard, all given ranges
{ a..b } are simply replaced with { NUMBER }.

The actual ranges are specific to each device and can 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

Finally, the unit (bps, tq, ns or ms) are given. The rationale to add
the units is that the TDC parameters (that will be introduced in the
upcoming patches) are measured in a different unit than the other
bittiming parameters: clock period (a.k.a. minimum time quantum)
instead of time quantum. Adding the units disambiguates things.

For reference, before the change:
$ ip link set can0 type can help
Usage: ip link set DEVICE type can
	[ bitrate BITRATE [ sample-point SAMPLE-POINT] ] |
	[ tq TQ prop-seg PROP_SEG phase-seg1 PHASE-SEG1
 	  phase-seg2 PHASE-SEG2 [ sjw SJW ] ]

	[ dbitrate BITRATE [ dsample-point SAMPLE-POINT] ] |
	[ dtq TQ dprop-seg PROP_SEG dphase-seg1 PHASE-SEG1
 	  dphase-seg2 PHASE-SEG2 [ dsjw SJW ] ]

	[ loopback { on | off } ]
	[ listen-only { on | off } ]
	[ triple-sampling { on | off } ]
	[ one-shot { on | off } ]
	[ berr-reporting { on | off } ]
	[ fd { on | off } ]
	[ fd-non-iso { on | off } ]
	[ presume-ack { on | off } ]

	[ restart-ms TIME-MS ]
	[ restart ]

	[ termination { 0..65535 } ]

	Where: BITRATE	:= { 1..1000000 }
		  SAMPLE-POINT	:= { 0.000..0.999 }
		  TQ		:= { NUMBER }
		  PROP-SEG	:= { 1..8 }
		  PHASE-SEG1	:= { 1..8 }
		  PHASE-SEG2	:= { 1..8 }
		  SJW		:= { 1..4 }
		  RESTART-MS	:= { 0 | NUMBER }

...and after it:
$ ip link set can0 type can help
Usage: ip link set DEVICE type can
	[ bitrate BITRATE [ sample-point SAMPLE-POINT] ] |
	[ tq TQ prop-seg PROP_SEG phase-seg1 PHASE-SEG1
 	  phase-seg2 PHASE-SEG2 [ sjw SJW ] ]

	[ dbitrate BITRATE [ dsample-point SAMPLE-POINT] ] |
	[ dtq TQ dprop-seg PROP_SEG dphase-seg1 PHASE-SEG1
 	  dphase-seg2 PHASE-SEG2 [ dsjw SJW ] ]

	[ loopback { on | off } ]
	[ listen-only { on | off } ]
	[ triple-sampling { on | off } ]
	[ one-shot { on | off } ]
	[ berr-reporting { on | off } ]
	[ fd { on | off } ]
	[ fd-non-iso { on | off } ]
	[ presume-ack { on | off } ]
	[ cc-len8-dlc { on | off } ]

	[ restart-ms TIME-MS ]
	[ restart ]

	[ termination { 0..65535 } ]

	Where: BITRATE	:= { NUMBER in bps }
		  SAMPLE-POINT	:= { 0.000..0.999 }
		  TQ		:= { NUMBER in ns }
		  PROP-SEG	:= { NUMBER in tq }
		  PHASE-SEG1	:= { NUMBER in tq }
		  PHASE-SEG2	:= { NUMBER in tq }
		  SJW		:= { NUMBER in tq }
		  RESTART-MS	:= { 0 | NUMBER in ms }

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

diff --git a/ip/iplink_can.c b/ip/iplink_can.c
index 6a26f3ff..0b2ff8a3 100644
--- a/ip/iplink_can.c
+++ b/ip/iplink_can.c
@@ -44,14 +44,14 @@ static void print_usage(FILE *f)
 		"\n"
 		"\t[ termination { 0..65535 } ]\n"
 		"\n"
-		"\tWhere: BITRATE	:= { 1..1000000 }\n"
+		"\tWhere: BITRATE	:= { NUMBER in bps }\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	  RESTART-MS	:= { 0 | NUMBER }\n"
+		"\t	  TQ		:= { NUMBER in ns }\n"
+		"\t	  PROP-SEG	:= { NUMBER in tq }\n"
+		"\t	  PHASE-SEG1	:= { NUMBER in tq }\n"
+		"\t	  PHASE-SEG2	:= { NUMBER in tq }\n"
+		"\t	  SJW		:= { NUMBER in tq }\n"
+		"\t	  RESTART-MS	:= { 0 | NUMBER in ms }\n"
 		);
 }
 
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v5 2/4] iplink_can: use PRINT_ANY to factorize code and fix signedness
  2021-08-14 10:17 [PATCH v5 0/4] iplink_can: cleaning, fixes and adding TDC support Vincent Mailhol
  2021-08-14 10:17 ` [PATCH v5 1/4] iplink_can: fix configuration ranges in print_usage() and add unit Vincent Mailhol
@ 2021-08-14 10:17 ` Vincent Mailhol
  2021-08-14 10:17 ` [PATCH v5 3/4] iplink_can: print brp and dbrp bittiming variables Vincent Mailhol
  2021-08-14 10:17 ` [PATCH v5 4/4] iplink_can: add new CAN FD bittiming parameters: Transmitter Delay Compensation (TDC) Vincent Mailhol
  3 siblings, 0 replies; 12+ messages in thread
From: Vincent Mailhol @ 2021-08-14 10:17 UTC (permalink / raw)
  To: Marc Kleine-Budde, Stephen Hemminger, linux-can
  Cc: Stefan Mätje, netdev, linux-kernel, 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 single 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 | 331 +++++++++++++++++++-----------------------------
 1 file changed, 130 insertions(+), 201 deletions(-)

diff --git a/ip/iplink_can.c b/ip/iplink_can.c
index 0b2ff8a3..52ba3d12 100644
--- a/ip/iplink_can.c
+++ b/ip/iplink_can.c
@@ -266,11 +266,19 @@ 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)
 {
-	open_json_object(attr);
-	print_int(PRINT_JSON, "min", NULL, min);
-	print_int(PRINT_JSON, "max", NULL, max);
+	print_nl();
+	print_string(PRINT_FP, NULL, "%s", "\t ");
+}
+
+static void can_print_timing_min_max(const char *json_attr, const char *fp_attr,
+				     int min, int max)
+{
+	print_null(PRINT_FP, NULL, fp_attr, NULL);
+	open_json_object(json_attr);
+	print_uint(PRINT_ANY, "min", " %d", min);
+	print_uint(PRINT_ANY, "max", "..%d", max);
 	close_json_object();
 }
 
@@ -297,56 +305,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 +344,18 @@ 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", " tseg1",
+					 btc->tseg1_min, btc->tseg1_max);
+		can_print_timing_min_max("tseg2", " tseg2",
+					 btc->tseg2_min, btc->tseg2_max);
+		can_print_timing_min_max("sjw", " sjw", 1, btc->sjw_max);
+		can_print_timing_min_max("brp", " 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 +371,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 +420,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_timing_min_max("tseg1", " dtseg1",
+					 dbtc->tseg1_min, dbtc->tseg1_max);
+		can_print_timing_min_max("tseg2", " dtseg2",
+					 dbtc->tseg2_min, dbtc->tseg2_max);
+		can_print_timing_min_max("sjw", " dsjw", 1, dbtc->sjw_max);
+		can_print_timing_min_max("brp", " dbrp",
+					 dbtc->brp_min, dbtc->brp_max);
+		print_uint(PRINT_ANY, "brp_inc", " dbrp_inc %u", dbtc->brp_inc);
+		close_json_object();
 	}
 
 	if (tb[IFLA_CAN_DATA_BITRATE_CONST]) {
@@ -497,30 +449,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 +475,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 +502,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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v5 3/4] iplink_can: print brp and dbrp bittiming variables
  2021-08-14 10:17 [PATCH v5 0/4] iplink_can: cleaning, fixes and adding TDC support Vincent Mailhol
  2021-08-14 10:17 ` [PATCH v5 1/4] iplink_can: fix configuration ranges in print_usage() and add unit Vincent Mailhol
  2021-08-14 10:17 ` [PATCH v5 2/4] iplink_can: use PRINT_ANY to factorize code and fix signedness Vincent Mailhol
@ 2021-08-14 10:17 ` Vincent Mailhol
  2021-08-14 10:17 ` [PATCH v5 4/4] iplink_can: add new CAN FD bittiming parameters: Transmitter Delay Compensation (TDC) Vincent Mailhol
  3 siblings, 0 replies; 12+ messages in thread
From: Vincent Mailhol @ 2021-08-14 10:17 UTC (permalink / raw)
  To: Marc Kleine-Budde, Stephen Hemminger, linux-can
  Cc: Stefan Mätje, netdev, linux-kernel, 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.

Noticeably, brp is not used as an input for bittiming calculation and
although it could be calculated by hand from the other bittiming
parameters with below formula:

        brp = clock * tq / 1000000000

with clock in hertz and tq in nano second (thus the need of a 1
billion factor to convert it back to second).

But because above formula is not so trivial to remember and is
subjected to rounding errors, it makes sense to directly output
{d,}bpr.

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 52ba3d12..e438e416 100644
--- a/ip/iplink_can.c
+++ b/ip/iplink_can.c
@@ -336,6 +336,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();
 	}
 
@@ -411,6 +412,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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v5 4/4] iplink_can: add new CAN FD bittiming parameters: Transmitter Delay Compensation (TDC)
  2021-08-14 10:17 [PATCH v5 0/4] iplink_can: cleaning, fixes and adding TDC support Vincent Mailhol
                   ` (2 preceding siblings ...)
  2021-08-14 10:17 ` [PATCH v5 3/4] iplink_can: print brp and dbrp bittiming variables Vincent Mailhol
@ 2021-08-14 10:17 ` Vincent Mailhol
  2021-08-14 11:14   ` Marc Kleine-Budde
                     ` (2 more replies)
  3 siblings, 3 replies; 12+ messages in thread
From: Vincent Mailhol @ 2021-08-14 10:17 UTC (permalink / raw)
  To: Marc Kleine-Budde, Stephen Hemminger, linux-can
  Cc: Stefan Mätje, netdev, linux-kernel, 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

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

A tdc-mode parameter allow to specify how to operate. 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 TDCV and TDCO and may also
    specify TDCF if supported.

  * off: TDC is explicitly disabled.

  * tdc-mode parameter omitted (default mode): the kernel decides
    whether TDC should be enabled or not and if so, it calculates the
    TDC values. TDC parameters are an expert option and the average
    user is not expected to provide those, thus the presence of this
    "default mode".

TDCV is always reported in manual mode. In auto mode, TDCV is reported
only if the value is available. Especially, the TDCV might not be
available if the controller has no feature to report it or if the
value in not yet available (i.e. no data sent yet and measurement did
not occur).

TDCF is reported only if tdcf_max is not zero (i.e. if supported by the controller).

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 30 phase-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 dbrp_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 e438e416..e698655d 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 in tq }\n"
 		"\t	  PHASE-SEG2	:= { NUMBER in tq }\n"
 		"\t	  SJW		:= { NUMBER in tq }\n"
+		"\t	  TDCV		:= { NUMBER in tc}\n"
+		"\t	  TDCO		:= { NUMBER in tc}\n"
+		"\t	  TDCF		:= { NUMBER in tc}\n"
 		"\t	  RESTART-MS	:= { 0 | NUMBER in ms }\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;
 }
 
@@ -282,6 +331,62 @@ static void can_print_timing_min_max(const char *json_attr, const char *fp_attr,
 	close_json_object();
 }
 
+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", *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", *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", *tdcf_min, *tdcf_max);
+	}
+	close_json_object();
+}
+
 static void can_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 {
 	if (!tb)
@@ -413,6 +518,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();
 	}
 
@@ -433,6 +542,10 @@ static void can_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 		can_print_timing_min_max("brp", " dbrp",
 					 dbtc->brp_min, dbtc->brp_max);
 		print_uint(PRINT_ANY, "brp_inc", " dbrp_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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v5 4/4] iplink_can: add new CAN FD bittiming parameters: Transmitter Delay Compensation (TDC)
  2021-08-14 10:17 ` [PATCH v5 4/4] iplink_can: add new CAN FD bittiming parameters: Transmitter Delay Compensation (TDC) Vincent Mailhol
@ 2021-08-14 11:14   ` Marc Kleine-Budde
  2021-08-16 11:02     ` Vincent MAILHOL
  2021-08-16  8:12   ` Marc Kleine-Budde
  2021-08-16 13:51   ` Marc Kleine-Budde
  2 siblings, 1 reply; 12+ messages in thread
From: Marc Kleine-Budde @ 2021-08-14 11:14 UTC (permalink / raw)
  To: Vincent Mailhol
  Cc: Stephen Hemminger, linux-can, Stefan Mätje, netdev, linux-kernel

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

On 14.08.2021 19:17:28, Vincent Mailhol wrote:
>  include/uapi/linux/can/netlink.h |  30 +++++++-

IIRC, changes of the uapi headers will be pull in regularly from the
mainline kernel.

Marc

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

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v5 4/4] iplink_can: add new CAN FD bittiming parameters: Transmitter Delay Compensation (TDC)
  2021-08-14 10:17 ` [PATCH v5 4/4] iplink_can: add new CAN FD bittiming parameters: Transmitter Delay Compensation (TDC) Vincent Mailhol
  2021-08-14 11:14   ` Marc Kleine-Budde
@ 2021-08-16  8:12   ` Marc Kleine-Budde
  2021-08-16 10:40     ` Vincent MAILHOL
  2021-08-16 13:51   ` Marc Kleine-Budde
  2 siblings, 1 reply; 12+ messages in thread
From: Marc Kleine-Budde @ 2021-08-16  8:12 UTC (permalink / raw)
  To: Vincent Mailhol
  Cc: Stephen Hemminger, linux-can, Stefan Mätje, netdev, linux-kernel

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

On 14.08.2021 19:17:28, Vincent Mailhol wrote:
> At high bit rates, the propagation delay from the TX pin to the RX pin
> of the transceiver causes measurement errors: the sample point on the
> RX pin might occur on the previous bit.
> 
> This issue is addressed in ISO 11898-1 section 11.3.3 "Transmitter
> delay compensation" (TDC).
> 
> This patch 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
> 
> All those new parameters are nested together into the attribute
> IFLA_CAN_TDC.
> 
> A tdc-mode parameter allow to specify how to operate. 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
                           ^^^^^                      ^^^
                           singular                   plural
>     mode, the user must specify both TDCV and TDCO and may also
>     specify TDCF if supported.
> 

Marc

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

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v5 4/4] iplink_can: add new CAN FD bittiming parameters: Transmitter Delay Compensation (TDC)
  2021-08-16  8:12   ` Marc Kleine-Budde
@ 2021-08-16 10:40     ` Vincent MAILHOL
  0 siblings, 0 replies; 12+ messages in thread
From: Vincent MAILHOL @ 2021-08-16 10:40 UTC (permalink / raw)
  To: Marc Kleine-Budde
  Cc: Stephen Hemminger, linux-can, Stefan Mätje, netdev, open list

On Mon. 16 août 2021 at 17:12, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 14.08.2021 19:17:28, Vincent Mailhol wrote:
> > At high bit rates, the propagation delay from the TX pin to the RX pin
> > of the transceiver causes measurement errors: the sample point on the
> > RX pin might occur on the previous bit.
> >
> > This issue is addressed in ISO 11898-1 section 11.3.3 "Transmitter
> > delay compensation" (TDC).
> >
> > This patch 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
> >
> > All those new parameters are nested together into the attribute
> > IFLA_CAN_TDC.
> >
> > A tdc-mode parameter allow to specify how to operate. 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
>                            ^^^^^                      ^^^
>                            singular                   plural

ACK. I fixed that broken grammar in my local branch. As commented
before, I will send the next version of the iproute series after
we agree on the kernel part (unless someone finds a major issue).

FYI, this is the fixed sentence:
  * manual: use a static TDCV provided by the user. In this mode, the
    user must specify both TDCV and TDCO and may also specify TDCF if
    supported.


Yours sincerely,
Vincent

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v5 4/4] iplink_can: add new CAN FD bittiming parameters: Transmitter Delay Compensation (TDC)
  2021-08-14 11:14   ` Marc Kleine-Budde
@ 2021-08-16 11:02     ` Vincent MAILHOL
  0 siblings, 0 replies; 12+ messages in thread
From: Vincent MAILHOL @ 2021-08-16 11:02 UTC (permalink / raw)
  To: Marc Kleine-Budde
  Cc: Stephen Hemminger, linux-can, Stefan Mätje, netdev, open list

I originally sent the below answer on Saturday but omitted to CC
the mailing list so only Marc received it.
Resending it for the record.

On Sat. 14 Aug 2021 at 20:14, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 14.08.2021 19:17:28, Vincent Mailhol wrote:
> >  include/uapi/linux/can/netlink.h |  30 +++++++-
>
> IIRC, changes of the uapi headers will be pull in regularly from the
> mainline kernel.

I see... This kind of makes sense. However, that would be less
easier to test as people will have to update the uapi headers by
hand.

For now, I will just wait and I will send a new version without
the uapi header once we agree on the kernel part.


Yours sincerely,
Vincent

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v5 4/4] iplink_can: add new CAN FD bittiming parameters: Transmitter Delay Compensation (TDC)
  2021-08-14 10:17 ` [PATCH v5 4/4] iplink_can: add new CAN FD bittiming parameters: Transmitter Delay Compensation (TDC) Vincent Mailhol
  2021-08-14 11:14   ` Marc Kleine-Budde
  2021-08-16  8:12   ` Marc Kleine-Budde
@ 2021-08-16 13:51   ` Marc Kleine-Budde
  2021-08-16 16:24     ` Vincent MAILHOL
  2 siblings, 1 reply; 12+ messages in thread
From: Marc Kleine-Budde @ 2021-08-16 13:51 UTC (permalink / raw)
  To: Vincent Mailhol
  Cc: Stephen Hemminger, linux-can, Stefan Mätje, netdev, linux-kernel

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

On 14.08.2021 19:17:28, Vincent Mailhol wrote:
> At high bit rates, the propagation delay from the TX pin to the RX pin
> of the transceiver causes measurement errors: the sample point on the
> RX pin might occur on the previous bit.
> 
> This issue is addressed in ISO 11898-1 section 11.3.3 "Transmitter
> delay compensation" (TDC).
> 
> This patch 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
> 
> All those new parameters are nested together into the attribute
> IFLA_CAN_TDC.
> 
> A tdc-mode parameter allow to specify how to operate. 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 TDCV and TDCO and may also
>     specify TDCF if supported.
> 
>   * off: TDC is explicitly disabled.
> 
>   * tdc-mode parameter omitted (default mode): the kernel decides
>     whether TDC should be enabled or not and if so, it calculates the
>     TDC values. TDC parameters are an expert option and the average
>     user is not expected to provide those, thus the presence of this
>     "default mode".
> 
> TDCV is always reported in manual mode. In auto mode, TDCV is reported
> only if the value is available. Especially, the TDCV might not be
> available if the controller has no feature to report it or if the
> value in not yet available (i.e. no data sent yet and measurement did
> not occur).
> 
> TDCF is reported only if tdcf_max is not zero (i.e. if supported by the controller).
> 
> 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
              ^^^^^^^^
This is just the supported mode(s), right?

> 	  bitrate 1000000 sample-point 0.750
> 	  tq 12 prop-seg 29 phase-seg1 30 phase-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 dbrp_inc 1
> 	  tdco 0..127 tdcf 0..127
> 	  clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535

Is there a way to figure out, which tdc mode is currently active?

AFAICS just implicitly:
- tdco + tdcv   -> manual
- tdco          -> automatic
- neither       -> off

correct?

regards,
Marc

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

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v5 4/4] iplink_can: add new CAN FD bittiming parameters: Transmitter Delay Compensation (TDC)
  2021-08-16 13:51   ` Marc Kleine-Budde
@ 2021-08-16 16:24     ` Vincent MAILHOL
  2021-08-16 23:59       ` Vincent MAILHOL
  0 siblings, 1 reply; 12+ messages in thread
From: Vincent MAILHOL @ 2021-08-16 16:24 UTC (permalink / raw)
  To: Marc Kleine-Budde
  Cc: Stephen Hemminger, linux-can, Stefan Mätje, netdev, open list

On Mon. 16 Aug. 2021 at 22:51, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 14.08.2021 19:17:28, Vincent Mailhol wrote:
> > At high bit rates, the propagation delay from the TX pin to the RX pin
> > of the transceiver causes measurement errors: the sample point on the
> > RX pin might occur on the previous bit.
> >
> > This issue is addressed in ISO 11898-1 section 11.3.3 "Transmitter
> > delay compensation" (TDC).
> >
> > This patch 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
> >
> > All those new parameters are nested together into the attribute
> > IFLA_CAN_TDC.
> >
> > A tdc-mode parameter allow to specify how to operate. 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 TDCV and TDCO and may also
> >     specify TDCF if supported.
> >
> >   * off: TDC is explicitly disabled.
> >
> >   * tdc-mode parameter omitted (default mode): the kernel decides
> >     whether TDC should be enabled or not and if so, it calculates the
> >     TDC values. TDC parameters are an expert option and the average
> >     user is not expected to provide those, thus the presence of this
> >     "default mode".
> >
> > TDCV is always reported in manual mode. In auto mode, TDCV is reported
> > only if the value is available. Especially, the TDCV might not be
> > available if the controller has no feature to report it or if the
> > value in not yet available (i.e. no data sent yet and measurement did
> > not occur).
> >
> > TDCF is reported only if tdcf_max is not zero (i.e. if supported by the controller).
> >
> > 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
>               ^^^^^^^^
> This is just the supported mode(s), right?

No, this is the active mode. It should display either TDC_AUTO or
TDC_MANUAL. If both are displayed as you previously experienced,
it is a bug (I will fix).

> >         bitrate 1000000 sample-point 0.750
> >         tq 12 prop-seg 29 phase-seg1 30 phase-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 dbrp_inc 1
> >         tdco 0..127 tdcf 0..127
> >         clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535
>
> Is there a way to figure out, which tdc mode is currently active?
>
> AFAICS just implicitly:
> - tdco + tdcv   -> manual
> - tdco          -> automatic
> - neither       -> off
>
> correct?

If the TDC const values are reported (at least tdco) the controller
supports TDC.

The flags listed between brackets <FD, TDC_AUTO, CC-LEN8-DLC, ...>
are the active flags (this is not only true for TDC but also for
all other ctrlmodes).

There is no way to know which of the modes are supported. The
reason is that netlink only reports
can_priv->ctrlmode (c.f. IFLA_CAN_CTRLMODE), not
can_priv->ctrlmode_supported. We would need to add a
IFLA_CAN_CTRLMODE_SUPPORTED to the netlink interface in order to
confirm the supported mode.

Currently, the only ways are to either look at the kernel source
code or to test the command and see whether it is supported or
not.


Yours sincerely,
Vincent

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v5 4/4] iplink_can: add new CAN FD bittiming parameters: Transmitter Delay Compensation (TDC)
  2021-08-16 16:24     ` Vincent MAILHOL
@ 2021-08-16 23:59       ` Vincent MAILHOL
  0 siblings, 0 replies; 12+ messages in thread
From: Vincent MAILHOL @ 2021-08-16 23:59 UTC (permalink / raw)
  To: Marc Kleine-Budde
  Cc: Stephen Hemminger, linux-can, Stefan Mätje, netdev, open list

On Tue.17 Aug 2021 à 01:24, Vincent MAILHOL <mailhol.vincent@wanadoo.fr> wrote:
> On Mon. 16 Aug. 2021 at 22:51, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > On 14.08.2021 19:17:28, Vincent Mailhol wrote:
> > > At high bit rates, the propagation delay from the TX pin to the RX pin
> > > of the transceiver causes measurement errors: the sample point on the
> > > RX pin might occur on the previous bit.
> > >
> > > This issue is addressed in ISO 11898-1 section 11.3.3 "Transmitter
> > > delay compensation" (TDC).
> > >
> > > This patch 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
> > >
> > > All those new parameters are nested together into the attribute
> > > IFLA_CAN_TDC.
> > >
> > > A tdc-mode parameter allow to specify how to operate. 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 TDCV and TDCO and may also
> > >     specify TDCF if supported.
> > >
> > >   * off: TDC is explicitly disabled.
> > >
> > >   * tdc-mode parameter omitted (default mode): the kernel decides
> > >     whether TDC should be enabled or not and if so, it calculates the
> > >     TDC values. TDC parameters are an expert option and the average
> > >     user is not expected to provide those, thus the presence of this
> > >     "default mode".
> > >
> > > TDCV is always reported in manual mode. In auto mode, TDCV is reported
> > > only if the value is available. Especially, the TDCV might not be
> > > available if the controller has no feature to report it or if the
> > > value in not yet available (i.e. no data sent yet and measurement did
> > > not occur).
> > >
> > > TDCF is reported only if tdcf_max is not zero (i.e. if supported by the controller).
> > >
> > > 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
> >               ^^^^^^^^
> > This is just the supported mode(s), right?
>
> No, this is the active mode. It should display either TDC_AUTO or
> TDC_MANUAL. If both are displayed as you previously experienced,
> it is a bug (I will fix).
>
> > >         bitrate 1000000 sample-point 0.750
> > >         tq 12 prop-seg 29 phase-seg1 30 phase-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 dbrp_inc 1
> > >         tdco 0..127 tdcf 0..127
> > >         clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535
> >
> > Is there a way to figure out, which tdc mode is currently active?
> >
> > AFAICS just implicitly:
> > - tdco + tdcv   -> manual
> > - tdco          -> automatic
> > - neither       -> off
> >
> > correct?
>
> If the TDC const values are reported (at least tdco) the controller
> supports TDC.
>
> The flags listed between brackets <FD, TDC_AUTO, CC-LEN8-DLC, ...>
> are the active flags (this is not only true for TDC but also for
> all other ctrlmodes).
>
> There is no way to know which of the modes are supported. The
> reason is that netlink only reports
> can_priv->ctrlmode (c.f. IFLA_CAN_CTRLMODE), not
> can_priv->ctrlmode_supported. We would need to add a
> IFLA_CAN_CTRLMODE_SUPPORTED to the netlink interface in order to
> confirm the supported mode.

On a second thought, it is actually possible to deduce some of
the supported modes (not all) through the can_tdc_const values
because tdcv_{min,max} are only reported if
CAN_CTRLMODE_TDC_MANUAL is supported.
So:

  - both tdcv_{min,max} and tdco_{min,max} reported ->
    CAN_CTRLMODE_TDC_MANUAL is supported for
    sure. CAN_CTRLMODE_TDC_AUTO might or might not be supported.

  - only tdco_{min,max} reported -> only CAN_CTRLMODE_TDC_AUTO is
    supported (that's the case for the es58x device).

  - none reported -> device is not TDC capable.

  - tdcf_{min,max} reported -> device supports TDCF and the
    reverse is also true.

  - other combinations are incorrect and should not be reported.

> Currently, the only ways are to either look at the kernel source
> code or to test the command and see whether it is supported or
> not.
>
>
> Yours sincerely,
> Vincent

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-08-17  0:00 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-14 10:17 [PATCH v5 0/4] iplink_can: cleaning, fixes and adding TDC support Vincent Mailhol
2021-08-14 10:17 ` [PATCH v5 1/4] iplink_can: fix configuration ranges in print_usage() and add unit Vincent Mailhol
2021-08-14 10:17 ` [PATCH v5 2/4] iplink_can: use PRINT_ANY to factorize code and fix signedness Vincent Mailhol
2021-08-14 10:17 ` [PATCH v5 3/4] iplink_can: print brp and dbrp bittiming variables Vincent Mailhol
2021-08-14 10:17 ` [PATCH v5 4/4] iplink_can: add new CAN FD bittiming parameters: Transmitter Delay Compensation (TDC) Vincent Mailhol
2021-08-14 11:14   ` Marc Kleine-Budde
2021-08-16 11:02     ` Vincent MAILHOL
2021-08-16  8:12   ` Marc Kleine-Budde
2021-08-16 10:40     ` Vincent MAILHOL
2021-08-16 13:51   ` Marc Kleine-Budde
2021-08-16 16:24     ` Vincent MAILHOL
2021-08-16 23:59       ` Vincent MAILHOL

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.