[PATCH v2 0/2] add the netlink interface for CAN-FD Transmitter Delay Compensation (TDC)

[PATCH v2 0/2] add the netlink interface for CAN-FD Transmitter Delay Compensation (TDC)

[Vincent Mailhol]

This series adds a netlink interface for the TDC parameters using
netlink nested attributes.

The first patch remove a redundant check. The second patch is the real
thing: the TDC netlink interface.

In March, I introduced the Transmitter Delay Compensation (TDC) to the
kernel though two patches:
  - commit 289ea9e4ae59 ("can: add new CAN FD bittiming parameters:
    Transmitter Delay Compensation (TDC)")
  - commit c25cc7993243 ("can: bittiming: add calculation for CAN FD
    Transmitter Delay Compensation (TDC)")

The netlink interface was missing from this series because the initial
patch needed rework in order to make it more flexible for future
changes.

At that time, Marc suggested to take inspiration from the recently
released ethtool-netlink interface.
Ref: https://lore.kernel.org/linux-can/20210407081557.m3sotnepbgasarri@pengutronix.de/

ethtool uses nested attributes (c.f. NLA_NESTED type in validation
policy). A bit of trivia: the NLA_NESTED type was introduced in
version 2.6.15 of the kernel and thus actually predates Socket CAN.
Ref: commit bfa83a9e03cf ("[NETLINK]: Type-safe netlink messages/attributes interface")

I sent a v1 as an RFC which got zero comments, so I am assuming that
the overall design is OK :)

Now, I feel confident enough to drop the RFC tag. Thanks for your review!

For those who would like to test it, please refer to this iproute2 patch:
https://lore.kernel.org/linux-can/20210507102819.1932386-1-mailhol.vincent@wanadoo.fr/t/#u

** Changelog **
The nested structure (IFLAC_CAN_TDC*) remains unchanged since RFC v1.
The v2 fixes several issue in can_tdc_get_size() and
can_tdc_fill_info(). Namely: can_tdc_get_size() returned an incorrect
size if TDC was not implemented and can_tdc_fill_info() did not
include a fail path with nla_nest_cancel().


Vincent Mailhol (2):
  can: netlink: remove redundant check in can_validate()
  can: netlink: add interface for CAN-FD Transmitter Delay Compensation
    (TDC)

 drivers/net/can/dev/netlink.c    | 140 ++++++++++++++++++++++++++++++-
 include/uapi/linux/can/netlink.h |  26 +++++-
 2 files changed, 160 insertions(+), 6 deletions(-)

-- 
2.31.1

[PATCH v2 1/2] can: netlink: remove redundant check in can_validate()

[Vincent Mailhol]

can_validate() does a first check:
|	if (is_can_fd) {
|		if (!data[IFLA_CAN_BITTIMING] || !data[IFLA_CAN_DATA_BITTIMING])
|			return -EOPNOTSUPP;
|	}

If that first if succeeds, we know that if is_can_fd is true then
data[IFLA_CAN_BITTIMING is set.

However, the next if switch does not leverage on above knowledge and
redoes the check:
| 	if (data[IFLA_CAN_DATA_BITTIMING]) {
|		if (!is_can_fd || !data[IFLA_CAN_BITTIMING])
|		                   ^~~~~~~~~~~~~~~~~~~~~~~~
| 			return -EOPNOTSUPP;
| 	}

This patch removes that redundant check.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/dev/netlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index e38c2566aff4..32c603a09809 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -47,7 +47,7 @@ static int can_validate(struct nlattr *tb[], struct nlattr *data[],
 	}
 
 	if (data[IFLA_CAN_DATA_BITTIMING]) {
-		if (!is_can_fd || !data[IFLA_CAN_BITTIMING])
+		if (!is_can_fd)
 			return -EOPNOTSUPP;
 	}
 
-- 
2.31.1

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

[Vincent Mailhol]

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

Contrary to the can_bittiming(_const) structures for which there is
just a single IFLA_CAN(_DATA)_BITTMING(_CONST) entry per structure,
here, we create a nested entry IFLA_CAN_TDC. Within this nested entry,
additional IFLA_CAN_TDC_TDC* entries are added for each of the TDC
parameters of the newly introduced struct can_tdc_const and struct
can_tdc.

For struct can_tdc_const, these are:
        IFLA_CAN_TDC_TDCV_MAX
        IFLA_CAN_TDC_TDCO_MAX
        IFLA_CAN_TDC_TDCF_MAX

For struct can_tdc, these are:
        IFLA_CAN_TDC_TDCV
        IFLA_CAN_TDC_TDCO
        IFLA_CAN_TDC_TDCF

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

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

This patch is the third and last one to introduce TDC in the
kernel. It completes below series:
  - commit 289ea9e4ae59 ("can: add new CAN FD bittiming parameters:
    Transmitter Delay Compensation (TDC)")
  - commit c25cc7993243 ("can: bittiming: add calculation for CAN FD
    Transmitter Delay Compensation (TDC)")

Reference: https://lore.kernel.org/linux-can/20210224002008.4158-1-mailhol.vincent@wanadoo.fr/T/#t

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/dev/netlink.c    | 138 ++++++++++++++++++++++++++++++-
 include/uapi/linux/can/netlink.h |  26 +++++-
 2 files changed, 159 insertions(+), 5 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 32c603a09809..6134bbf69c10 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -19,6 +19,16 @@ static const struct nla_policy can_policy[IFLA_CAN_MAX + 1] = {
 	[IFLA_CAN_DATA_BITTIMING] = { .len = sizeof(struct can_bittiming) },
 	[IFLA_CAN_DATA_BITTIMING_CONST]	= { .len = sizeof(struct can_bittiming_const) },
 	[IFLA_CAN_TERMINATION] = { .type = NLA_U16 },
+	[IFLA_CAN_TDC] = { .type = NLA_NESTED },
+};
+
+static const struct nla_policy can_tdc_policy[IFLA_CAN_TDC_MAX + 1] = {
+	[IFLA_CAN_TDC_TDCV_MAX] = { .type = NLA_U32 },
+	[IFLA_CAN_TDC_TDCO_MAX] = { .type = NLA_U32 },
+	[IFLA_CAN_TDC_TDCF_MAX] = { .type = NLA_U32 },
+	[IFLA_CAN_TDC_TDCV] = { .type = NLA_U32 },
+	[IFLA_CAN_TDC_TDCO] = { .type = NLA_U32 },
+	[IFLA_CAN_TDC_TDCF] = { .type = NLA_U32 },
 };
 
 static int can_validate(struct nlattr *tb[], struct nlattr *data[],
@@ -46,7 +56,7 @@ static int can_validate(struct nlattr *tb[], struct nlattr *data[],
 			return -EOPNOTSUPP;
 	}
 
-	if (data[IFLA_CAN_DATA_BITTIMING]) {
+	if (data[IFLA_CAN_DATA_BITTIMING] || data[IFLA_CAN_TDC]) {
 		if (!is_can_fd)
 			return -EOPNOTSUPP;
 	}
@@ -54,6 +64,65 @@ static int can_validate(struct nlattr *tb[], struct nlattr *data[],
 	return 0;
 }
 
+static int can_tdc_changelink(struct net_device *dev, const struct nlattr *nla,
+			      struct netlink_ext_ack *extack)
+{
+	struct nlattr *tb[IFLA_CAN_TDC_MAX + 1];
+	struct can_priv *priv = netdev_priv(dev);
+	struct can_tdc *tdc = &priv->tdc;
+	const struct can_tdc_const *tdc_const = priv->tdc_const;
+	int err;
+
+	if (!tdc_const)
+		return -EOPNOTSUPP;
+
+	if (dev->flags & IFF_UP)
+		return -EBUSY;
+
+	err = nla_parse_nested(tb, IFLA_CAN_TDC_MAX, nla,
+			       can_tdc_policy, extack);
+	if (err)
+		return err;
+
+	if (tb[IFLA_CAN_TDC_TDCV]) {
+		u32 tdcv = nla_get_u32(tb[IFLA_CAN_TDC_TDCV]);
+
+		if (tdcv && !tdc_const->tdcv_max)
+			return -EOPNOTSUPP;
+
+		if (tdcv > tdc_const->tdcv_max)
+			return -EINVAL;
+
+		tdc->tdcv = tdcv;
+	}
+
+	if (tb[IFLA_CAN_TDC_TDCO]) {
+		u32 tdco = nla_get_u32(tb[IFLA_CAN_TDC_TDCO]);
+
+		if (tdco && !tdc_const->tdco_max)
+			return -EOPNOTSUPP;
+
+		if (tdco > tdc_const->tdco_max)
+			return -EINVAL;
+
+		tdc->tdco = tdco;
+	}
+
+	if (tb[IFLA_CAN_TDC_TDCF]) {
+		u32 tdcf = nla_get_u32(tb[IFLA_CAN_TDC_TDCF]);
+
+		if (tdcf && !tdc_const->tdcf_max)
+			return -EOPNOTSUPP;
+
+		if (tdcf > tdc_const->tdcf_max)
+			return -EINVAL;
+
+		tdc->tdcf = tdcf;
+	}
+
+	return 0;
+}
+
 static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 			  struct nlattr *data[],
 			  struct netlink_ext_ack *extack)
@@ -220,9 +289,37 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 		priv->termination = termval;
 	}
 
+	if (data[IFLA_CAN_TDC]) {
+		err = can_tdc_changelink(dev, data[IFLA_CAN_TDC], extack);
+		if (err)
+			return err;
+	}
+
 	return 0;
 }
 
+static size_t can_tdc_get_size(const struct net_device *dev)
+{
+	struct can_priv *priv = netdev_priv(dev);
+	size_t size;
+
+	if (!priv->tdc_const)
+		return 0;
+
+	size = nla_total_size(0);			/* nest IFLA_CAN_TDC */
+	size += nla_total_size(sizeof(u32));		/* IFLA_CAN_TDCV_MAX */
+	size += nla_total_size(sizeof(u32));		/* IFLA_CAN_TDCO_MAX */
+	size += nla_total_size(sizeof(u32));		/* IFLA_CAN_TDCF_MAX */
+
+	if (priv->tdc.tdco) {
+		size += nla_total_size(sizeof(u32));	/* IFLA_CAN_TDCV */
+		size += nla_total_size(sizeof(u32));	/* IFLA_CAN_TDCO */
+		size += nla_total_size(sizeof(u32));	/* IFLA_CAN_TDCF */
+	}
+
+	return size;
+}
+
 static size_t can_get_size(const struct net_device *dev)
 {
 	struct can_priv *priv = netdev_priv(dev);
@@ -254,10 +351,43 @@ static size_t can_get_size(const struct net_device *dev)
 		size += nla_total_size(sizeof(*priv->data_bitrate_const) *
 				       priv->data_bitrate_const_cnt);
 	size += sizeof(priv->bitrate_max);			/* IFLA_CAN_BITRATE_MAX */
-
+	size += can_tdc_get_size(dev);				/* IFLA_CAN_TDC */
 	return size;
 }
 
+static int can_tdc_fill_info(struct sk_buff *skb, const struct net_device *dev)
+{
+	struct nlattr *nest;
+	struct can_priv *priv = netdev_priv(dev);
+	struct can_tdc *tdc = &priv->tdc;
+	const struct can_tdc_const *tdc_const = priv->tdc_const;
+
+	if (!tdc_const)
+		return 0;
+
+	nest = nla_nest_start(skb, IFLA_CAN_TDC);
+	if (!nest)
+		return -EMSGSIZE;
+
+	if (nla_put_u32(skb, IFLA_CAN_TDC_TDCV_MAX, tdc_const->tdcv_max) ||
+	    nla_put_u32(skb, IFLA_CAN_TDC_TDCO_MAX, tdc_const->tdco_max) ||
+	    nla_put_u32(skb, IFLA_CAN_TDC_TDCF_MAX, tdc_const->tdcf_max))
+		goto err_cancel;
+
+	if (priv->tdc.tdco)
+		if (nla_put_u32(skb, IFLA_CAN_TDC_TDCV, tdc->tdcv) ||
+		    nla_put_u32(skb, IFLA_CAN_TDC_TDCO, tdc->tdco) ||
+		    nla_put_u32(skb, IFLA_CAN_TDC_TDCF, tdc->tdcf))
+			goto err_cancel;
+
+	nla_nest_end(skb, nest);
+	return 0;
+
+err_cancel:
+	nla_nest_cancel(skb, nest);
+	return -EMSGSIZE;
+}
+
 static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
 {
 	struct can_priv *priv = netdev_priv(dev);
@@ -315,7 +445,9 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
 
 	    (nla_put(skb, IFLA_CAN_BITRATE_MAX,
 		     sizeof(priv->bitrate_max),
-		     &priv->bitrate_max))
+		     &priv->bitrate_max)) ||
+
+	    (can_tdc_fill_info(skb, dev))
 	    )
 
 		return -EMSGSIZE;
diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
index f730d443b918..3e81cad069a1 100644
--- a/include/uapi/linux/can/netlink.h
+++ b/include/uapi/linux/can/netlink.h
@@ -134,10 +134,32 @@ enum {
 	IFLA_CAN_BITRATE_CONST,
 	IFLA_CAN_DATA_BITRATE_CONST,
 	IFLA_CAN_BITRATE_MAX,
-	__IFLA_CAN_MAX
+	IFLA_CAN_TDC,
+
+	/* add new constants above here */
+	__IFLA_CAN_MAX,
+	IFLA_CAN_MAX = __IFLA_CAN_MAX - 1
 };
 
-#define IFLA_CAN_MAX	(__IFLA_CAN_MAX - 1)
+/*
+ * CAN FD Transmitter Delay Compensation (TDC)
+ *
+ * Please refer to struct can_tdc_const and can_tdc in
+ * include/linux/can/bittiming.h for further details.
+ */
+enum {
+	IFLA_CAN_TDC_UNSPEC,
+	IFLA_CAN_TDC_TDCV_MAX,	/* u32 */
+	IFLA_CAN_TDC_TDCO_MAX,	/* 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
+};
 
 /* u16 termination range: 1..65535 Ohms */
 #define CAN_TERMINATION_DISABLED 0
-- 
2.31.1

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

[Marc Kleine-Budde]

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

On 04.06.2021 00:15:50, Vincent Mailhol wrote:
[...]

> +static size_t can_tdc_get_size(const struct net_device *dev)
> +{
> +	struct can_priv *priv = netdev_priv(dev);
> +	size_t size;
> +
> +	if (!priv->tdc_const)
> +		return 0;
> +
> +	size = nla_total_size(0);			/* nest IFLA_CAN_TDC */
> +	size += nla_total_size(sizeof(u32));		/* IFLA_CAN_TDCV_MAX */
> +	size += nla_total_size(sizeof(u32));		/* IFLA_CAN_TDCO_MAX */
> +	size += nla_total_size(sizeof(u32));		/* IFLA_CAN_TDCF_MAX */
> +
> +	if (priv->tdc.tdco) {

Naively I'd say, iff the device has tdc_const give the user space the
tdc parameters, regardless if some value is 0 or not.

What do you think?

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

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

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

[Vincent MAILHOL]

On Wed. 16 Jun 2021 at 18:46, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 04.06.2021 00:15:50, Vincent Mailhol wrote:
> [...]
>
> > +static size_t can_tdc_get_size(const struct net_device *dev)
> > +{
> > +     struct can_priv *priv = netdev_priv(dev);
> > +     size_t size;
> > +
> > +     if (!priv->tdc_const)
> > +             return 0;
> > +
> > +     size = nla_total_size(0);                       /* nest IFLA_CAN_TDC */
> > +     size += nla_total_size(sizeof(u32));            /* IFLA_CAN_TDCV_MAX */
> > +     size += nla_total_size(sizeof(u32));            /* IFLA_CAN_TDCO_MAX */
> > +     size += nla_total_size(sizeof(u32));            /* IFLA_CAN_TDCF_MAX */
> > +
> > +     if (priv->tdc.tdco) {
>
> Naively I'd say, iff the device has tdc_const give the user space the
> tdc parameters, regardless if some value is 0 or not.
>
> What do you think?

I thought about that.
The first important remark is that if tdc.tdco is zero, then TDC
is off (c.f. documentation of struct can_tdc::tdco).

Let me illustrate my vision through examples.


** Case 1: link is not configured at all. **
Here, only the constant values are displayed.

# 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 (berr-counter tx 0 rx 0) 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 brp_inc 1
      tdcv_max 0 tdco_max 127 tdcf_max 127
      clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536
gso_max_segs 65535


** Case 2: only the nominal bitrate is configured. **
The data bittiming variables (including TDC) are not shown.

# 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 (berr-counter tx 0 rx 0) restart-ms 0
      bitrate 500000 sample-point 0.875
      tq 12 prop-seg 69 phase-seg1 70phase-seg2 20  sjw 1
      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 brp_inc 1
      tdcv_max 0 tdco_max 127 tdcf_max 127
      clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536
gso_max_segs 65535


** Case 3: both nominal and data bitrates are configured (but not TDC). **
Only the TDC variables are not shown.

# 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> state STOPPED (berr-counter tx 0 rx 0) restart-ms 0
      bitrate 500000 sample-point 0.875
      tq 12 prop-seg 69 phase-seg1 70phase-seg2 20  sjw 1
      ES582.1/ES584.1: tseg1 2..256 tseg2 2..128 sjw 1..128 brp 1..512 brp_inc 1
      dbitrate 2000000 dsample-point 0.750
      dtq 12 dprop-seg 14 dphase-seg1 15 dphase-seg2 10 dsjw 1
      ES582.1/ES584.1: dtseg1 2..32 dtseg2 1..16 dsjw 1..8 dbrp 1..32 brp_inc 1
      tdcv_max 0 tdco_max 127 tdcf_max 127
      clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536
gso_max_segs 65535


** Case 4: nominal and data bitrates and TDC are configured. **
Everything is shown.

# 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> 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
      ES582.1/ES584.1: tseg1 2..256 tseg2 2..128 sjw 1..128 brp 1..512 brp_inc 1
      dbitrate 5000000 dsample-point 0.750
      dtq 12 dprop-seg 5 dphase-seg1 6 dphase-seg2 4 dsjw 1
      tdcv 0 tdco 12 tdcf 0
      ES582.1/ES584.1: dtseg1 2..32 dtseg2 1..16 dsjw 1..8 dbrp 1..32 brp_inc 1
      tdcv_max 0 tdco_max 127 tdcf_max 127
      clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536
gso_max_segs 65535


I think that we can agree on Cases 1, 2 (it would not make sense
to display TDC without the data bittiming variables) and 4.

The edge case is Case 3. It depends if we consider the TDC as a
separate set or not. It is not silly to display the TDC whenever
fd is on. I prefer to keep it the way I did it. But I would not
object to changing this if you insist. That would mean:
-     if (priv->tdc.tdco) {
+     if (priv->data_bittiming.bitrate) {


Finally, I have one side comment. It seems to me that you did not
understand that the intent of
|     if (priv->tdc.tdco)
was to actually check whether TDC was on or off. In other words, my
code was unclear.

I am now thinking to introduce an helper macro:
static bool can_tdc_is_enabled(const struct can_priv *priv)
|{
|    return !!priv->tdc.tdco;
|}

The code would look more clear like that.
-     if (priv->tdc.tdco) {
+     if (can_tdc_is_enabled(priv) {


Yours sincerely,
Vincent

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

[Marc Kleine-Budde]

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

On 16.06.2021 22:53:02, Vincent MAILHOL wrote:
> On Wed. 16 Jun 2021 at 18:46, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > On 04.06.2021 00:15:50, Vincent Mailhol wrote:
> > [...]
> >
> > > +static size_t can_tdc_get_size(const struct net_device *dev)
> > > +{
> > > +     struct can_priv *priv = netdev_priv(dev);
> > > +     size_t size;
> > > +
> > > +     if (!priv->tdc_const)
> > > +             return 0;
> > > +
> > > +     size = nla_total_size(0);                       /* nest IFLA_CAN_TDC */
> > > +     size += nla_total_size(sizeof(u32));            /* IFLA_CAN_TDCV_MAX */
> > > +     size += nla_total_size(sizeof(u32));            /* IFLA_CAN_TDCO_MAX */
> > > +     size += nla_total_size(sizeof(u32));            /* IFLA_CAN_TDCF_MAX */
> > > +
> > > +     if (priv->tdc.tdco) {
> >
> > Naively I'd say, iff the device has tdc_const give the user space the
> > tdc parameters, regardless if some value is 0 or not.
> >
> > What do you think?
> 
> I thought about that.
> The first important remark is that if tdc.tdco is zero, then TDC
> is off (c.f. documentation of struct can_tdc::tdco).
> 
> Let me illustrate my vision through examples.

[...]

examples makes sense \o/

[...]

> Finally, I have one side comment. It seems to me that you did not
> understand that the intent of
> |     if (priv->tdc.tdco)
> was to actually check whether TDC was on or off. In other words, my
> code was unclear.
> 
> I am now thinking to introduce an helper macro:
> static bool can_tdc_is_enabled(const struct can_priv *priv)
> |{
> |    return !!priv->tdc.tdco;
> |}
> 
> The code would look more clear like that.
> -     if (priv->tdc.tdco) {
> +     if (can_tdc_is_enabled(priv) {

Sounds good, I'm squashing this patch:

| diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
| index 6134bbf69c10..d48be574eae7 100644
| --- a/drivers/net/can/dev/netlink.c
| +++ b/drivers/net/can/dev/netlink.c
| @@ -311,7 +311,7 @@ static size_t can_tdc_get_size(const struct net_device *dev)
|         size += nla_total_size(sizeof(u32));            /* IFLA_CAN_TDCO_MAX */
|         size += nla_total_size(sizeof(u32));            /* IFLA_CAN_TDCF_MAX */
|  
| -       if (priv->tdc.tdco) {
| +       if (can_tdc_is_enabled(priv)) {
|                 size += nla_total_size(sizeof(u32));    /* IFLA_CAN_TDCV */
|                 size += nla_total_size(sizeof(u32));    /* IFLA_CAN_TDCO */
|                 size += nla_total_size(sizeof(u32));    /* IFLA_CAN_TDCF */
| @@ -352,6 +352,7 @@ static size_t can_get_size(const struct net_device *dev)
|                                        priv->data_bitrate_const_cnt);
|         size += sizeof(priv->bitrate_max);                      /* IFLA_CAN_BITRATE_MAX */
|         size += can_tdc_get_size(dev);                          /* IFLA_CAN_TDC */
| +
|         return size;
|  }
|  
| @@ -374,7 +375,7 @@ static int can_tdc_fill_info(struct sk_buff *skb, const struct net_device *dev)
|             nla_put_u32(skb, IFLA_CAN_TDC_TDCF_MAX, tdc_const->tdcf_max))
|                 goto err_cancel;
|  
| -       if (priv->tdc.tdco)
| +       if (can_tdc_is_enabled(priv)) {
|                 if (nla_put_u32(skb, IFLA_CAN_TDC_TDCV, tdc->tdcv) ||
|                     nla_put_u32(skb, IFLA_CAN_TDC_TDCO, tdc->tdco) ||
|                     nla_put_u32(skb, IFLA_CAN_TDC_TDCF, tdc->tdcf))
| diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h
| index 9de6e9053e34..b6d1db1e7258 100644
| --- a/include/linux/can/bittiming.h
| +++ b/include/linux/can/bittiming.h
| @@ -83,6 +83,11 @@ struct can_tdc_const {
|         u32 tdcf_max;
|  };
|  
| +static inline bool can_tdc_is_enabled(const struct can_priv *priv)
| +{
| +       return !!priv->tdc.tdco;
| +}
| +
|  #ifdef CONFIG_CAN_CALC_BITTIMING
|  int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
|                        const struct can_bittiming_const *btc);

regards,
Marc

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

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

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

[Vincent MAILHOL]

On Wed. 16 Jun 2021 à 23:29, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 16.06.2021 22:53:02, Vincent MAILHOL wrote:
> > On Wed. 16 Jun 2021 at 18:46, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > > On 04.06.2021 00:15:50, Vincent Mailhol wrote:
> > > [...]
> > >
> > > > +static size_t can_tdc_get_size(const struct net_device *dev)
> > > > +{
> > > > +     struct can_priv *priv = netdev_priv(dev);
> > > > +     size_t size;
> > > > +
> > > > +     if (!priv->tdc_const)
> > > > +             return 0;
> > > > +
> > > > +     size = nla_total_size(0);                       /* nest IFLA_CAN_TDC */
> > > > +     size += nla_total_size(sizeof(u32));            /* IFLA_CAN_TDCV_MAX */
> > > > +     size += nla_total_size(sizeof(u32));            /* IFLA_CAN_TDCO_MAX */
> > > > +     size += nla_total_size(sizeof(u32));            /* IFLA_CAN_TDCF_MAX */
> > > > +
> > > > +     if (priv->tdc.tdco) {
> > >
> > > Naively I'd say, iff the device has tdc_const give the user space the
> > > tdc parameters, regardless if some value is 0 or not.
> > >
> > > What do you think?
> >
> > I thought about that.
> > The first important remark is that if tdc.tdco is zero, then TDC
> > is off (c.f. documentation of struct can_tdc::tdco).
> >
> > Let me illustrate my vision through examples.
>
> [...]
>
> examples makes sense \o/

Great!

> [...]
>
> > Finally, I have one side comment. It seems to me that you did not
> > understand that the intent of
> > |     if (priv->tdc.tdco)
> > was to actually check whether TDC was on or off. In other words, my
> > code was unclear.
> >
> > I am now thinking to introduce an helper macro:
> > static bool can_tdc_is_enabled(const struct can_priv *priv)
> > |{
> > |    return !!priv->tdc.tdco;
> > |}
> >
> > The code would look more clear like that.
> > -     if (priv->tdc.tdco) {
> > +     if (can_tdc_is_enabled(priv) {
>
> Sounds good, I'm squashing this patch:
>
> | diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
> | index 6134bbf69c10..d48be574eae7 100644
> | --- a/drivers/net/can/dev/netlink.c
> | +++ b/drivers/net/can/dev/netlink.c
> | @@ -311,7 +311,7 @@ static size_t can_tdc_get_size(const struct net_device *dev)
> |         size += nla_total_size(sizeof(u32));            /* IFLA_CAN_TDCO_MAX */
> |         size += nla_total_size(sizeof(u32));            /* IFLA_CAN_TDCF_MAX */
> |
> | -       if (priv->tdc.tdco) {
> | +       if (can_tdc_is_enabled(priv)) {
> |                 size += nla_total_size(sizeof(u32));    /* IFLA_CAN_TDCV */
> |                 size += nla_total_size(sizeof(u32));    /* IFLA_CAN_TDCO */
> |                 size += nla_total_size(sizeof(u32));    /* IFLA_CAN_TDCF */
> | @@ -352,6 +352,7 @@ static size_t can_get_size(const struct net_device *dev)
> |                                        priv->data_bitrate_const_cnt);
> |         size += sizeof(priv->bitrate_max);                      /* IFLA_CAN_BITRATE_MAX */
> |         size += can_tdc_get_size(dev);                          /* IFLA_CAN_TDC */
> | +
> |         return size;
> |  }
> |
> | @@ -374,7 +375,7 @@ static int can_tdc_fill_info(struct sk_buff *skb, const struct net_device *dev)
> |             nla_put_u32(skb, IFLA_CAN_TDC_TDCF_MAX, tdc_const->tdcf_max))
> |                 goto err_cancel;
> |
> | -       if (priv->tdc.tdco)
> | +       if (can_tdc_is_enabled(priv)) {
> |                 if (nla_put_u32(skb, IFLA_CAN_TDC_TDCV, tdc->tdcv) ||
> |                     nla_put_u32(skb, IFLA_CAN_TDC_TDCO, tdc->tdco) ||
> |                     nla_put_u32(skb, IFLA_CAN_TDC_TDCF, tdc->tdcf))
> | diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h
> | index 9de6e9053e34..b6d1db1e7258 100644
> | --- a/include/linux/can/bittiming.h
> | +++ b/include/linux/can/bittiming.h
> | @@ -83,6 +83,11 @@ struct can_tdc_const {
> |         u32 tdcf_max;
> |  };
> |
> | +static inline bool can_tdc_is_enabled(const struct can_priv *priv)

Did you try to compile? I am not sure if bittiming.h is able to
see struct can_priv which is defined in dev.h.


Yours sincerely,
Vincent

> | +{
> | +       return !!priv->tdc.tdco;
> | +}
> | +
> |  #ifdef CONFIG_CAN_CALC_BITTIMING
> |  int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
> |                        const struct can_bittiming_const *btc);

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

[Marc Kleine-Budde]

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

On 16.06.2021 23:43:35, Vincent MAILHOL wrote:
> > Sounds good, I'm squashing this patch:
> >
> > | diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
> > | index 6134bbf69c10..d48be574eae7 100644
> > | --- a/drivers/net/can/dev/netlink.c
> > | +++ b/drivers/net/can/dev/netlink.c
> > | @@ -311,7 +311,7 @@ static size_t can_tdc_get_size(const struct net_device *dev)
> > |         size += nla_total_size(sizeof(u32));            /* IFLA_CAN_TDCO_MAX */
> > |         size += nla_total_size(sizeof(u32));            /* IFLA_CAN_TDCF_MAX */
> > |
> > | -       if (priv->tdc.tdco) {
> > | +       if (can_tdc_is_enabled(priv)) {
> > |                 size += nla_total_size(sizeof(u32));    /* IFLA_CAN_TDCV */
> > |                 size += nla_total_size(sizeof(u32));    /* IFLA_CAN_TDCO */
> > |                 size += nla_total_size(sizeof(u32));    /* IFLA_CAN_TDCF */
> > | @@ -352,6 +352,7 @@ static size_t can_get_size(const struct net_device *dev)
> > |                                        priv->data_bitrate_const_cnt);
> > |         size += sizeof(priv->bitrate_max);                      /* IFLA_CAN_BITRATE_MAX */
> > |         size += can_tdc_get_size(dev);                          /* IFLA_CAN_TDC */
> > | +
> > |         return size;
> > |  }
> > |
> > | @@ -374,7 +375,7 @@ static int can_tdc_fill_info(struct sk_buff *skb, const struct net_device *dev)
> > |             nla_put_u32(skb, IFLA_CAN_TDC_TDCF_MAX, tdc_const->tdcf_max))
> > |                 goto err_cancel;
> > |
> > | -       if (priv->tdc.tdco)
> > | +       if (can_tdc_is_enabled(priv)) {
> > |                 if (nla_put_u32(skb, IFLA_CAN_TDC_TDCV, tdc->tdcv) ||
> > |                     nla_put_u32(skb, IFLA_CAN_TDC_TDCO, tdc->tdco) ||
> > |                     nla_put_u32(skb, IFLA_CAN_TDC_TDCF, tdc->tdcf))
> > | diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h
> > | index 9de6e9053e34..b6d1db1e7258 100644
> > | --- a/include/linux/can/bittiming.h
> > | +++ b/include/linux/can/bittiming.h
> > | @@ -83,6 +83,11 @@ struct can_tdc_const {
> > |         u32 tdcf_max;
> > |  };
> > |
> > | +static inline bool can_tdc_is_enabled(const struct can_priv *priv)
> 
> Did you try to compile?

Not before sending that mail :)

> I am not sure if bittiming.h is able to see struct can_priv which is
> defined in dev.h.

Nope it doesn't, I moved the can_tdc_is_enabled() to
include/linux/can/dev.h

Marc

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

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

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

[Vincent MAILHOL]

On Wed. 16 Jun 2021 at 23:46, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 16.06.2021 23:43:35, Vincent MAILHOL wrote:
> > > Sounds good, I'm squashing this patch:
> > >
> > > | diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
> > > | index 6134bbf69c10..d48be574eae7 100644
> > > | --- a/drivers/net/can/dev/netlink.c
> > > | +++ b/drivers/net/can/dev/netlink.c
> > > | @@ -311,7 +311,7 @@ static size_t can_tdc_get_size(const struct net_device *dev)
> > > |         size += nla_total_size(sizeof(u32));            /* IFLA_CAN_TDCO_MAX */
> > > |         size += nla_total_size(sizeof(u32));            /* IFLA_CAN_TDCF_MAX */
> > > |
> > > | -       if (priv->tdc.tdco) {
> > > | +       if (can_tdc_is_enabled(priv)) {
> > > |                 size += nla_total_size(sizeof(u32));    /* IFLA_CAN_TDCV */
> > > |                 size += nla_total_size(sizeof(u32));    /* IFLA_CAN_TDCO */
> > > |                 size += nla_total_size(sizeof(u32));    /* IFLA_CAN_TDCF */
> > > | @@ -352,6 +352,7 @@ static size_t can_get_size(const struct net_device *dev)
> > > |                                        priv->data_bitrate_const_cnt);
> > > |         size += sizeof(priv->bitrate_max);                      /* IFLA_CAN_BITRATE_MAX */
> > > |         size += can_tdc_get_size(dev);                          /* IFLA_CAN_TDC */
> > > | +
> > > |         return size;
> > > |  }
> > > |
> > > | @@ -374,7 +375,7 @@ static int can_tdc_fill_info(struct sk_buff *skb, const struct net_device *dev)
> > > |             nla_put_u32(skb, IFLA_CAN_TDC_TDCF_MAX, tdc_const->tdcf_max))
> > > |                 goto err_cancel;
> > > |
> > > | -       if (priv->tdc.tdco)
> > > | +       if (can_tdc_is_enabled(priv)) {
> > > |                 if (nla_put_u32(skb, IFLA_CAN_TDC_TDCV, tdc->tdcv) ||
> > > |                     nla_put_u32(skb, IFLA_CAN_TDC_TDCO, tdc->tdco) ||
> > > |                     nla_put_u32(skb, IFLA_CAN_TDC_TDCF, tdc->tdcf))
> > > | diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h
> > > | index 9de6e9053e34..b6d1db1e7258 100644
> > > | --- a/include/linux/can/bittiming.h
> > > | +++ b/include/linux/can/bittiming.h
> > > | @@ -83,6 +83,11 @@ struct can_tdc_const {
> > > |         u32 tdcf_max;
> > > |  };
> > > |
> > > | +static inline bool can_tdc_is_enabled(const struct can_priv *priv)
> >
> > Did you try to compile?
>
> Not before sending that mail :)
>
> > I am not sure if bittiming.h is able to see struct can_priv which is
> > defined in dev.h.
>
> Nope it doesn't, I moved the can_tdc_is_enabled() to
> include/linux/can/dev.h

Ack. It seems to be the only solution…

Moving forward, I will do one more round of tests and send the
patch for iproute2-next (warning, the RFC I sent last month has
some issues, if you wish to test it on your side, please wait).

I will also apply can_tdc_is_enabled() to the etas_es58x driver.

Could you push the recent changes on the testing branch of linux-can-next? It
would be really helpful for me!


Yours sincerely,
Vincent

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

[Marc Kleine-Budde]

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

On 17.06.2021 00:44:12, Vincent MAILHOL wrote:
> > > Did you try to compile?
> >
> > Not before sending that mail :)
> >
> > > I am not sure if bittiming.h is able to see struct can_priv which is
> > > defined in dev.h.
> >
> > Nope it doesn't, I moved the can_tdc_is_enabled() to
> > include/linux/can/dev.h
> 
> Ack. It seems to be the only solution…
> 
> Moving forward, I will do one more round of tests and send the
> patch for iproute2-next (warning, the RFC I sent last month has
> some issues, if you wish to test it on your side, please wait).
> 
> I will also apply can_tdc_is_enabled() to the etas_es58x driver.
> 
> Could you push the recent changes on the testing branch of linux-can-next? It
> would be really helpful for me!

done

Marc.

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

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

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

[Marc Kleine-Budde]

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

On 04.06.2021 00:15:50, Vincent Mailhol wrote:
> Add the netlink interface for TDC parameters of struct can_tdc_const
> and can_tdc.
> 
> Contrary to the can_bittiming(_const) structures for which there is
> just a single IFLA_CAN(_DATA)_BITTMING(_CONST) entry per structure,
> here, we create a nested entry IFLA_CAN_TDC. Within this nested entry,
> additional IFLA_CAN_TDC_TDC* entries are added for each of the TDC
> parameters of the newly introduced struct can_tdc_const and struct
> can_tdc.
> 
> For struct can_tdc_const, these are:
>         IFLA_CAN_TDC_TDCV_MAX
>         IFLA_CAN_TDC_TDCO_MAX
>         IFLA_CAN_TDC_TDCF_MAX
> 
> For struct can_tdc, these are:
>         IFLA_CAN_TDC_TDCV
>         IFLA_CAN_TDC_TDCO
>         IFLA_CAN_TDC_TDCF

I just noticed in the mcp2518fd data sheet:

| bit 14-8 TDCO[6:0]: Transmitter Delay Compensation Offset bits;
| Secondary Sample Point (SSP) Two’s complement; offset can be positive,
| zero, or negative.
| 
| 011 1111 = 63 x TSYSCLK
| ...
| 000 0000 = 0 x TSYSCLK
| ...
| 111 1111 = –64 x TSYSCLK

Have you takes this into account?

regards,
Marc

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

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

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

[Vincent MAILHOL]

On Fri. 18 Jun 2021 at 18:34, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 04.06.2021 00:15:50, Vincent Mailhol wrote:
> > Add the netlink interface for TDC parameters of struct can_tdc_const
> > and can_tdc.
> >
> > Contrary to the can_bittiming(_const) structures for which there is
> > just a single IFLA_CAN(_DATA)_BITTMING(_CONST) entry per structure,
> > here, we create a nested entry IFLA_CAN_TDC. Within this nested entry,
> > additional IFLA_CAN_TDC_TDC* entries are added for each of the TDC
> > parameters of the newly introduced struct can_tdc_const and struct
> > can_tdc.
> >
> > For struct can_tdc_const, these are:
> >         IFLA_CAN_TDC_TDCV_MAX
> >         IFLA_CAN_TDC_TDCO_MAX
> >         IFLA_CAN_TDC_TDCF_MAX
> >
> > For struct can_tdc, these are:
> >         IFLA_CAN_TDC_TDCV
> >         IFLA_CAN_TDC_TDCO
> >         IFLA_CAN_TDC_TDCF
>
> I just noticed in the mcp2518fd data sheet:
>
> | bit 14-8 TDCO[6:0]: Transmitter Delay Compensation Offset bits;
> | Secondary Sample Point (SSP) Two’s complement; offset can be positive,
> | zero, or negative.
> |
> | 011 1111 = 63 x TSYSCLK
> | ...
> | 000 0000 = 0 x TSYSCLK
> | ...
> | 111 1111 = –64 x TSYSCLK
>
> Have you takes this into account?

I have not. And I fail to understand what would be the physical
meaning if TDCO is zero or negative.

TDCV indicates the position of the bit start on the RX pin. If
TDCO is zero, the measurement occurs on the bit start when all
the ringing occurs. That is a really bad choice to do the
measurement.  If it is negative, it means that you are measuring
the previous bit o_O !?

Maybe I am missing something but I just do not get it.

I believe you started to implement the mcp2518fd. Can you force a
zero and a negative value and tell me if the bus is stable?


Yours sincerely,
Vincent Mailhol

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

[Vincent MAILHOL]

On Fri. 18 Jun 2021 at 19:23, Vincent MAILHOL
<mailhol.vincent@wanadoo.fr> wrote:
>
> On Fri. 18 Jun 2021 at 18:34, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > On 04.06.2021 00:15:50, Vincent Mailhol wrote:
> > > Add the netlink interface for TDC parameters of struct can_tdc_const
> > > and can_tdc.
> > >
> > > Contrary to the can_bittiming(_const) structures for which there is
> > > just a single IFLA_CAN(_DATA)_BITTMING(_CONST) entry per structure,
> > > here, we create a nested entry IFLA_CAN_TDC. Within this nested entry,
> > > additional IFLA_CAN_TDC_TDC* entries are added for each of the TDC
> > > parameters of the newly introduced struct can_tdc_const and struct
> > > can_tdc.
> > >
> > > For struct can_tdc_const, these are:
> > >         IFLA_CAN_TDC_TDCV_MAX
> > >         IFLA_CAN_TDC_TDCO_MAX
> > >         IFLA_CAN_TDC_TDCF_MAX
> > >
> > > For struct can_tdc, these are:
> > >         IFLA_CAN_TDC_TDCV
> > >         IFLA_CAN_TDC_TDCO
> > >         IFLA_CAN_TDC_TDCF
> >
> > I just noticed in the mcp2518fd data sheet:
> >
> > | bit 14-8 TDCO[6:0]: Transmitter Delay Compensation Offset bits;
> > | Secondary Sample Point (SSP) Two’s complement; offset can be positive,
> > | zero, or negative.
> > |
> > | 011 1111 = 63 x TSYSCLK
> > | ...
> > | 000 0000 = 0 x TSYSCLK
> > | ...
> > | 111 1111 = –64 x TSYSCLK
> >
> > Have you takes this into account?
>
> I have not. And I fail to understand what would be the physical
> meaning if TDCO is zero or negative.
>
> TDCV indicates the position of the bit start on the RX pin. If
> TDCO is zero, the measurement occurs on the bit start when all
> the ringing occurs. That is a really bad choice to do the
> measurement.  If it is negative, it means that you are measuring
> the previous bit o_O !?
>
> Maybe I am missing something but I just do not get it.
>
> I believe you started to implement the mcp2518fd. Can you force a
> zero and a negative value and tell me if the bus is stable?

Actually, ISO 11898-1 specifies that the "SSP position should be
at least 0 to 63 minimum time quanta". This means that we can
have SSP = TDCV + TDCO = 0. In my implementation, I used 0 as a
reserved value for TDCV and TDCO. To comply with the standard, I
now need to allow both TDCV and TDCO to be zero and add a new
field in struct tdc to manage the automatic/manual options.

That said, these zero values still make no sense to me. Why would
someone do the measurement on the bit edge?

Concerning the negative values, the ISO standard says nothing
about it.  If you are using the automatic measurement, a negative
TDCO is impossible to use. TDCV is measured on every bit. When
the measurement is done, it is too late to subtract from it (or
maybe the mcp2518fd has a time machine built in?). If you are
using the manual mode for TDCV, just choose two positive values
so that TDCV + TDCO = SSF.

CAN-FD Transmitter Delay Compensation (TDC) on mcp2518fd

[Marc Kleine-Budde]

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

On 18.06.2021 20:17:51, Vincent MAILHOL wrote:
> > > I just noticed in the mcp2518fd data sheet:
> > >
> > > | bit 14-8 TDCO[6:0]: Transmitter Delay Compensation Offset bits;
> > > | Secondary Sample Point (SSP) Two’s complement; offset can be positive,
> > > | zero, or negative.
> > > |
> > > | 011 1111 = 63 x TSYSCLK
> > > | ...
> > > | 000 0000 = 0 x TSYSCLK
> > > | ...
> > > | 111 1111 = –64 x TSYSCLK
> > >
> > > Have you takes this into account?
> >
> > I have not. And I fail to understand what would be the physical
> > meaning if TDCO is zero or negative.

The mcp25xxfd family data sheet says:

| SSP = TDCV + TDCO

> > TDCV indicates the position of the bit start on the RX pin.

If I understand correctly in automatic mode TDCV is measured by the CAN
controller and reflects the transceiver delay. I don't know why you want
to subtract a time from that....

The rest of the relevant registers:

| TDCMOD[1:0]: Transmitter Delay Compensation Mode bits; Secondary Sample Point (SSP)
| 10-11 = Auto; measure delay and add TDCO.
| 01 = Manual; Do not measure, use TDCV + TDCO from register
| 00 = TDC Disabled
| 
| TDCO[6:0]: Transmitter Delay Compensation Offset bits; Secondary Sample Point (SSP)
| Two’s complement; offset can be positive, zero, or negative.
| 011 1111 = 63 x TSYSCLK
| ...
| 000 0000 = 0 x TSYSCLK
| ...
| 111 1111 = –64 x TSYSCLK
| 
| TDCV[5:0]: Transmitter Delay Compensation Value bits; Secondary Sample Point (SSP)
| 11 1111 = 63 x TSYSCLK
| ...
| 00 0000 = 0 x TSYSCLK

> > If TDCO is zero, the measurement occurs on the bit start when all
> > the ringing occurs. That is a really bad choice to do the
> > measurement. If it is negative, it means that you are measuring the
> > previous bit o_O !?

I don't know...

> > Maybe I am missing something but I just do not get it.
> >
> > I believe you started to implement the mcp2518fd.

No I've just looked into the register description.

> > Can you force a
> > zero and a negative value and tell me if the bus is stable?
> 
> Actually, ISO 11898-1 specifies that the "SSP position should be
> at least 0 to 63 minimum time quanta". This means that we can
> have SSP = TDCV + TDCO = 0. In my implementation, I used 0 as a
> reserved value for TDCV and TDCO. To comply with the standard, I
> now need to allow both TDCV and TDCO to be zero and add a new
> field in struct tdc to manage the automatic/manual options.
> 
> That said, these zero values still make no sense to me. Why would
> someone do the measurement on the bit edge?
> 
> Concerning the negative values, the ISO standard says nothing
> about it.  If you are using the automatic measurement, a negative
> TDCO is impossible to use. TDCV is measured on every bit. When
> the measurement is done, it is too late to subtract from it (or
> maybe the mcp2518fd has a time machine built in?).

:)

> If you are
> using the manual mode for TDCV, just choose two positive values
> so that TDCV + TDCO = SSF.

Marc

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

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

Re: CAN-FD Transmitter Delay Compensation (TDC) on mcp2518fd

[Vincent MAILHOL]

On Fri. 18 Jun 2021 at 21:44, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 18.06.2021 20:17:51, Vincent MAILHOL wrote:
> > > > I just noticed in the mcp2518fd data sheet:
> > > >
> > > > | bit 14-8 TDCO[6:0]: Transmitter Delay Compensation Offset bits;
> > > > | Secondary Sample Point (SSP) Two’s complement; offset can be positive,
> > > > | zero, or negative.
> > > > |
> > > > | 011 1111 = 63 x TSYSCLK
> > > > | ...
> > > > | 000 0000 = 0 x TSYSCLK
> > > > | ...
> > > > | 111 1111 = –64 x TSYSCLK
> > > >
> > > > Have you takes this into account?
> > >
> > > I have not. And I fail to understand what would be the physical
> > > meaning if TDCO is zero or negative.
>
> The mcp25xxfd family data sheet says:
>
> | SSP = TDCV + TDCO
>
> > > TDCV indicates the position of the bit start on the RX pin.
>
> If I understand correctly in automatic mode TDCV is measured by the CAN
> controller and reflects the transceiver delay.

Yes. I phrased it poorly but this is what I wanted to say. It is
the delay to propagate from the TX pin to the RX pin.

If TDCO = 0 then SSP = TDCV + 0 = TDCV thus the measurement
occurs at the bit start on the RX pin.

> I don't know why you want
> to subtract a time from that....
>
> The rest of the relevant registers:
>
> | TDCMOD[1:0]: Transmitter Delay Compensation Mode bits; Secondary Sample Point (SSP)
> | 10-11 = Auto; measure delay and add TDCO.
> | 01 = Manual; Do not measure, use TDCV + TDCO from register
> | 00 = TDC Disabled
> |
> | TDCO[6:0]: Transmitter Delay Compensation Offset bits; Secondary Sample Point (SSP)
> | Two’s complement; offset can be positive, zero, or negative.
> | 011 1111 = 63 x TSYSCLK
> | ...
> | 000 0000 = 0 x TSYSCLK
> | ...
> | 111 1111 = –64 x TSYSCLK
> |
> | TDCV[5:0]: Transmitter Delay Compensation Value bits; Secondary Sample Point (SSP)
> | 11 1111 = 63 x TSYSCLK
> | ...
> | 00 0000 = 0 x TSYSCLK

Aside from the negative TDCO, the rest is standard stuff. We can
note the absence of the TDCF but that's not a blocker.

> > > If TDCO is zero, the measurement occurs on the bit start when all
> > > the ringing occurs. That is a really bad choice to do the
> > > measurement. If it is negative, it means that you are measuring the
> > > previous bit o_O !?
>
> I don't know...
>
> > > Maybe I am missing something but I just do not get it.
> > >
> > > I believe you started to implement the mcp2518fd.
>
> No I've just looked into the register description.

OK. For your information, the ETAS ES58x FD devices do not allow
the use of manual mode for TDCV. The microcontroller from
Microchip supports it but ETAS firmware only exposes the
automatic TDCV mode. So it can not be used to test what would
occur if SSP = 0.

I will prepare a patch to allow zero value for both TDCV and
TDCO (I am a bit sad because I prefer the current design, but if
ISO allows it, I feel like I have no choice).  However, I refuse
to allow the negative TDCO value unless someone is able to
explain the rationale.


Yours sincerely,
Vincent

Re: CAN-FD Transmitter Delay Compensation (TDC) on mcp2518fd

[Stefan Mätje]

Am Freitag, den 18.06.2021, 23:27 +0900 schrieb Vincent MAILHOL:
> On Fri. 18 Jun 2021 at 21:44, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > On 18.06.2021 20:17:51, Vincent MAILHOL wrote:
> > > > > I just noticed in the mcp2518fd data sheet:
> > > > > 
> > > > > > bit 14-8 TDCO[6:0]: Transmitter Delay Compensation Offset bits;
> > > > > > Secondary Sample Point (SSP) Two’s complement; offset can be
> > > > > > positive,
> > > > > > zero, or negative.
> > > > > > 
> > > > > > 011 1111 = 63 x TSYSCLK
> > > > > > ...
> > > > > > 000 0000 = 0 x TSYSCLK
> > > > > > ...
> > > > > > 111 1111 = –64 x TSYSCLK
> > > > > 
> > > > > Have you takes this into account?
> > > > 
> > > > I have not. And I fail to understand what would be the physical
> > > > meaning if TDCO is zero or negative.
> > 
> > The mcp25xxfd family data sheet says:
> > 
> > > SSP = TDCV + TDCO
> > > > TDCV indicates the position of the bit start on the RX pin.
> > 
> > If I understand correctly in automatic mode TDCV is measured by the CAN
> > controller and reflects the transceiver delay.
> 
> Yes. I phrased it poorly but this is what I wanted to say. It is
> the delay to propagate from the TX pin to the RX pin.
> 
> If TDCO = 0 then SSP = TDCV + 0 = TDCV thus the measurement
> occurs at the bit start on the RX pin.
> 
> > I don't know why you want
> > to subtract a time from that....
> > 
> > The rest of the relevant registers:
> > 
> > > TDCMOD[1:0]: Transmitter Delay Compensation Mode bits; Secondary Sample
> > > Point (SSP)
> > > 10-11 = Auto; measure delay and add TDCO.
> > > 01 = Manual; Do not measure, use TDCV + TDCO from register
> > > 00 = TDC Disabled
> > > 
> > > TDCO[6:0]: Transmitter Delay Compensation Offset bits; Secondary Sample
> > > Point (SSP)
> > > Two’s complement; offset can be positive, zero, or negative.
> > > 011 1111 = 63 x TSYSCLK
> > > ...
> > > 000 0000 = 0 x TSYSCLK
> > > ...
> > > 111 1111 = –64 x TSYSCLK
> > > 
> > > TDCV[5:0]: Transmitter Delay Compensation Value bits; Secondary Sample
> > > Point (SSP)
> > > 11 1111 = 63 x TSYSCLK
> > > ...
> > > 00 0000 = 0 x TSYSCLK
> 
> Aside from the negative TDCO, the rest is standard stuff. We can
> note the absence of the TDCF but that's not a blocker.
> 
> > > > If TDCO is zero, the measurement occurs on the bit start when all
> > > > the ringing occurs. That is a really bad choice to do the
> > > > measurement. If it is negative, it means that you are measuring the
> > > > previous bit o_O !?
> > 
> > I don't know...
> > 
> > > > Maybe I am missing something but I just do not get it.
> > > > 
> > > > I believe you started to implement the mcp2518fd.
> > 
> > No I've just looked into the register description.
> 
> OK. For your information, the ETAS ES58x FD devices do not allow
> the use of manual mode for TDCV. The microcontroller from
> Microchip supports it but ETAS firmware only exposes the
> automatic TDCV mode. So it can not be used to test what would
> occur if SSP = 0.
> 
> I will prepare a patch to allow zero value for both TDCV and
> TDCO (I am a bit sad because I prefer the current design, but if
> ISO allows it, I feel like I have no choice).  However, I refuse
> to allow the negative TDCO value unless someone is able to
> explain the rationale.

Hi,

perhaps I can shed some light on the idea why it is a good idea to allow
negative TDC offset values. Therefore I would describe the TDC register
interface of the ESDACC CAN-FD controller of our company (see 
https://esd.eu/en/products/esdacc).

Register description of TDC-CAN-FD register (reserved bits not shown):

bits [5..0], RO: TDC Measured (TDCmeas)
	Currently measured TDC value, needs baudrate to be set and CAN traffic

bits [21..16], R/W: TDC offset (TDCoffs)
	Depending on the selected mode (see TDC mode)
	- Auto TDC, automatic mode (default)
		signed offset onto measured TDC (TDCeff = TDCmeas + TDCoffs),
		interpreted as 6-bit two's complement value
	- Manual TDC
		absolute unsigned offset (TDCeff = TDCoffs),
		interpreted as 6-bit unsigned value
	- Other modes
		ignored
	In either case TDC offset is a number of CAN clock cycles.

bits [31..30], R/W: TDC mode
	00 = Auto TDC
	01 = Manual TDC
	10 = reserved
	11 = TDC off

So in automatic mode the goal is to be able to move the real sample point
forward and(!) backward from the measured transmitter delay. Therefore the
TDCoffs is interpreted as 6-bit two's complement value to make negative offsets
possible and to decrease the effective (used) TDCeff below the measured value
TDCmeas.

As far as I have understood our FPGA guy the TDCmeas value is the number of
clock cycles counted from the start of transmitting a dominant bit until the
dominant state reaches the RX pin.

During the data phase the sample point is controlled by the tseg values set for
the data phase but is moved additionally by the number of clocks specified by
TDCeff (or SSP in the mcp2518fd case).

Best regards,
    Stefan

PS: I'm interested in this topic because I'm in the process of preparing a
SocketCAN driver for our ESDACC base PCIe/402 CAN interface family.

Re: CAN-FD Transmitter Delay Compensation (TDC) on mcp2518fd

[Vincent MAILHOL]

On Sat. 19 Jun 2021 à 00:55, Stefan Mätje <Stefan.Maetje@esd.eu> wrote:
> Am Freitag, den 18.06.2021, 23:27 +0900 schrieb Vincent MAILHOL:
> > On Fri. 18 Jun 2021 at 21:44, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > > On 18.06.2021 20:17:51, Vincent MAILHOL wrote:
> > > > > > I just noticed in the mcp2518fd data sheet:
> > > > > >
> > > > > > > bit 14-8 TDCO[6:0]: Transmitter Delay Compensation Offset bits;
> > > > > > > Secondary Sample Point (SSP) Two’s complement; offset can be
> > > > > > > positive,
> > > > > > > zero, or negative.
> > > > > > >
> > > > > > > 011 1111 = 63 x TSYSCLK
> > > > > > > ...
> > > > > > > 000 0000 = 0 x TSYSCLK
> > > > > > > ...
> > > > > > > 111 1111 = –64 x TSYSCLK
> > > > > >
> > > > > > Have you takes this into account?
> > > > >
> > > > > I have not. And I fail to understand what would be the physical
> > > > > meaning if TDCO is zero or negative.
> > >
> > > The mcp25xxfd family data sheet says:
> > >
> > > > SSP = TDCV + TDCO
> > > > > TDCV indicates the position of the bit start on the RX pin.
> > >
> > > If I understand correctly in automatic mode TDCV is measured by the CAN
> > > controller and reflects the transceiver delay.
> >
> > Yes. I phrased it poorly but this is what I wanted to say. It is
> > the delay to propagate from the TX pin to the RX pin.
> >
> > If TDCO = 0 then SSP = TDCV + 0 = TDCV thus the measurement
> > occurs at the bit start on the RX pin.
> >
> > > I don't know why you want
> > > to subtract a time from that....
> > >
> > > The rest of the relevant registers:
> > >
> > > > TDCMOD[1:0]: Transmitter Delay Compensation Mode bits; Secondary Sample
> > > > Point (SSP)
> > > > 10-11 = Auto; measure delay and add TDCO.
> > > > 01 = Manual; Do not measure, use TDCV + TDCO from register
> > > > 00 = TDC Disabled
> > > >
> > > > TDCO[6:0]: Transmitter Delay Compensation Offset bits; Secondary Sample
> > > > Point (SSP)
> > > > Two’s complement; offset can be positive, zero, or negative.
> > > > 011 1111 = 63 x TSYSCLK
> > > > ...
> > > > 000 0000 = 0 x TSYSCLK
> > > > ...
> > > > 111 1111 = –64 x TSYSCLK
> > > >
> > > > TDCV[5:0]: Transmitter Delay Compensation Value bits; Secondary Sample
> > > > Point (SSP)
> > > > 11 1111 = 63 x TSYSCLK
> > > > ...
> > > > 00 0000 = 0 x TSYSCLK
> >
> > Aside from the negative TDCO, the rest is standard stuff. We can
> > note the absence of the TDCF but that's not a blocker.
> >
> > > > > If TDCO is zero, the measurement occurs on the bit start when all
> > > > > the ringing occurs. That is a really bad choice to do the
> > > > > measurement. If it is negative, it means that you are measuring the
> > > > > previous bit o_O !?
> > >
> > > I don't know...
> > >
> > > > > Maybe I am missing something but I just do not get it.
> > > > >
> > > > > I believe you started to implement the mcp2518fd.
> > >
> > > No I've just looked into the register description.
> >
> > OK. For your information, the ETAS ES58x FD devices do not allow
> > the use of manual mode for TDCV. The microcontroller from
> > Microchip supports it but ETAS firmware only exposes the
> > automatic TDCV mode. So it can not be used to test what would
> > occur if SSP = 0.
> >
> > I will prepare a patch to allow zero value for both TDCV and
> > TDCO (I am a bit sad because I prefer the current design, but if
> > ISO allows it, I feel like I have no choice).  However, I refuse
> > to allow the negative TDCO value unless someone is able to
> > explain the rationale.
>
> Hi,
>
> perhaps I can shed some light on the idea why it is a good idea to allow
> negative TDC offset values. Therefore I would describe the TDC register
> interface of the ESDACC CAN-FD controller of our company (see
> https://esd.eu/en/products/esdacc).

Thanks for joining the conversation. I am happy to receive help
from more experts!

> Register description of TDC-CAN-FD register (reserved bits not shown):
>
> bits [5..0], RO: TDC Measured (TDCmeas)
>         Currently measured TDC value, needs baudrate to be set and CAN traffic
>
> bits [21..16], R/W: TDC offset (TDCoffs)
>         Depending on the selected mode (see TDC mode)
>         - Auto TDC, automatic mode (default)
>                 signed offset onto measured TDC (TDCeff = TDCmeas + TDCoffs),
>                 interpreted as 6-bit two's complement value
>         - Manual TDC
>                 absolute unsigned offset (TDCeff = TDCoffs),
>                 interpreted as 6-bit unsigned value
>         - Other modes
>                 ignored
>         In either case TDC offset is a number of CAN clock cycles.
>
> bits [31..30], R/W: TDC mode
>         00 = Auto TDC
>         01 = Manual TDC
>         10 = reserved
>         11 = TDC off

First remark is that you use different naming than what I
witnessed so far in other datasheets. Let me try to give the
equivalences between your device and the struct can_tdc which I
proposed in my patches.

The Left members are ESDACC CAN-FD registers, the right members
are variables from Socket CAN.

** Auto TDC **
TDCoffs = struct can_tdc::tdco

** Manual TDC **
TDCoffs = struct can_tdc::tdcv + struct can_tdc::tdco

In both cases, TDCeff corresponds to the SSP position.

> So in automatic mode the goal is to be able to move the real sample point
> forward and(!) backward from the measured transmitter delay. Therefore the
> TDCoffs is interpreted as 6-bit two's complement value to make negative offsets
> possible and to decrease the effective (used) TDCeff below the measured value
> TDCmeas.
>
> As far as I have understood our FPGA guy the TDCmeas value is the number of
> clock cycles counted from the start of transmitting a dominant bit until the
> dominant state reaches the RX pin.

Your definition of TDCmeas is consistent with the definition of
TDCV in socket CAN.

What I miss to understand is what does it mean to subtract from
that TDCmeas/TDCV value. If you subtract from it, it means that
TDCeff/SSP is sampled before the signal reaches the RX
pin. Correct?

> During the data phase the sample point is controlled by the tseg values set for
> the data phase but is moved additionally by the number of clocks specified by
> TDCeff (or SSP in the mcp2518fd case).

Here I do not follow you. The SSP, as specified in ISO 11898-1
is "specified by its distance from the start of the bit
time". Either you do not use TDC and the measurement is done on
the SP according to the tseg values, either you do use TDC and
the measurement is done on the SSP according to the TDC
values. There is no mention of mixing the tseg and tdc values.

P.S.: don't hesitate to invite your FPGA guy to this thread!


Yours sincerely,
Vincent

Re: CAN-FD Transmitter Delay Compensation (TDC) on mcp2518fd

[Stefan Mätje]

Am Samstag, den 19.06.2021, 21:34 +0900 schrieb Vincent MAILHOL:
> On Sat. 19 Jun 2021 à 00:55, Stefan Mätje <Stefan.Maetje@esd.eu> wrote:
> > Am Freitag, den 18.06.2021, 23:27 +0900 schrieb Vincent MAILHOL:
> > > On Fri. 18 Jun 2021 at 21:44, Marc Kleine-Budde <mkl@pengutronix.de>
> > > wrote:
> > > > On 18.06.2021 20:17:51, Vincent MAILHOL wrote:
> > > > > > > I just noticed in the mcp2518fd data sheet:
> > > > > > > 
> > > > > > > > bit 14-8 TDCO[6:0]: Transmitter Delay Compensation Offset bits;
> > > > > > > > Secondary Sample Point (SSP) Two’s complement; offset can be
> > > > > > > > positive,
> > > > > > > > zero, or negative.
> > > > > > > > 
> > > > > > > > 011 1111 = 63 x TSYSCLK
> > > > > > > > ...
> > > > > > > > 000 0000 = 0 x TSYSCLK
> > > > > > > > ...
> > > > > > > > 111 1111 = –64 x TSYSCLK
> > > > > > > 
> > > > > > > Have you takes this into account?
> > > > > > 
> > > > > > I have not. And I fail to understand what would be the physical
> > > > > > meaning if TDCO is zero or negative.
> > > > 
> > > > The mcp25xxfd family data sheet says:
> > > > 
> > > > > SSP = TDCV + TDCO
> > > > > > TDCV indicates the position of the bit start on the RX pin.
> > > > 
> > > > If I understand correctly in automatic mode TDCV is measured by the CAN
> > > > controller and reflects the transceiver delay.
> > > 
> > > Yes. I phrased it poorly but this is what I wanted to say. It is
> > > the delay to propagate from the TX pin to the RX pin.
> > > 
> > > If TDCO = 0 then SSP = TDCV + 0 = TDCV thus the measurement
> > > occurs at the bit start on the RX pin.
> > > 
> > > > I don't know why you want
> > > > to subtract a time from that....
> > > > 
> > > > The rest of the relevant registers:
> > > > 
> > > > > TDCMOD[1:0]: Transmitter Delay Compensation Mode bits; Secondary
> > > > > Sample
> > > > > Point (SSP)
> > > > > 10-11 = Auto; measure delay and add TDCO.
> > > > > 01 = Manual; Do not measure, use TDCV + TDCO from register
> > > > > 00 = TDC Disabled
> > > > > 
> > > > > TDCO[6:0]: Transmitter Delay Compensation Offset bits; Secondary
> > > > > Sample
> > > > > Point (SSP)
> > > > > Two’s complement; offset can be positive, zero, or negative.
> > > > > 011 1111 = 63 x TSYSCLK
> > > > > ...
> > > > > 000 0000 = 0 x TSYSCLK
> > > > > ...
> > > > > 111 1111 = –64 x TSYSCLK
> > > > > 
> > > > > TDCV[5:0]: Transmitter Delay Compensation Value bits; Secondary Sample
> > > > > Point (SSP)
> > > > > 11 1111 = 63 x TSYSCLK
> > > > > ...
> > > > > 00 0000 = 0 x TSYSCLK
> > > 
> > > Aside from the negative TDCO, the rest is standard stuff. We can
> > > note the absence of the TDCF but that's not a blocker.
> > > 
> > > > > > If TDCO is zero, the measurement occurs on the bit start when all
> > > > > > the ringing occurs. That is a really bad choice to do the
> > > > > > measurement. If it is negative, it means that you are measuring the
> > > > > > previous bit o_O !?
> > > > 
> > > > I don't know...
> > > > 
> > > > > > Maybe I am missing something but I just do not get it.
> > > > > > 
> > > > > > I believe you started to implement the mcp2518fd.
> > > > 
> > > > No I've just looked into the register description.
> > > 
> > > OK. For your information, the ETAS ES58x FD devices do not allow
> > > the use of manual mode for TDCV. The microcontroller from
> > > Microchip supports it but ETAS firmware only exposes the
> > > automatic TDCV mode. So it can not be used to test what would
> > > occur if SSP = 0.
> > > 
> > > I will prepare a patch to allow zero value for both TDCV and
> > > TDCO (I am a bit sad because I prefer the current design, but if
> > > ISO allows it, I feel like I have no choice).  However, I refuse
> > > to allow the negative TDCO value unless someone is able to
> > > explain the rationale.
> > 
> > Hi,
> > 
> > perhaps I can shed some light on the idea why it is a good idea to allow
> > negative TDC offset values. Therefore I would describe the TDC register
> > interface of the ESDACC CAN-FD controller of our company (see
> > https://esd.eu/en/products/esdacc).
> 
> Thanks for joining the conversation. I am happy to receive help
> from more experts!
> 
> > Register description of TDC-CAN-FD register (reserved bits not shown):
> > 
> > bits [5..0], RO: TDC Measured (TDCmeas)
> >         Currently measured TDC value, needs baudrate to be set and CAN
> > traffic
> > 
> > bits [21..16], R/W: TDC offset (TDCoffs)
> >         Depending on the selected mode (see TDC mode)
> >         - Auto TDC, automatic mode (default)
> >                 signed offset onto measured TDC (TDCeff = TDCmeas +
> > TDCoffs),
> >                 interpreted as 6-bit two's complement value
> >         - Manual TDC
> >                 absolute unsigned offset (TDCeff = TDCoffs),
> >                 interpreted as 6-bit unsigned value
> >         - Other modes
> >                 ignored
> >         In either case TDC offset is a number of CAN clock cycles.
> > 
> > bits [31..30], R/W: TDC mode
> >         00 = Auto TDC
> >         01 = Manual TDC
> >         10 = reserved
> >         11 = TDC off
> 
> First remark is that you use different naming than what I
> witnessed so far in other datasheets. Let me try to give the
> equivalences between your device and the struct can_tdc which I
> proposed in my patches.
> 
> The Left members are ESDACC CAN-FD registers, the right members
> are variables from Socket CAN.
> 
> ** Auto TDC **
> TDCoffs = struct can_tdc::tdco
> 
> ** Manual TDC **
> TDCoffs = struct can_tdc::tdcv + struct can_tdc::tdco
> 
> In both cases, TDCeff corresponds to the SSP position.

TDCeff is not the SSP position in our implementation. see below.

> 
> > So in automatic mode the goal is to be able to move the real sample point
> > forward and(!) backward from the measured transmitter delay. Therefore the
> > TDCoffs is interpreted as 6-bit two's complement value to make negative
> > offsets
> > possible and to decrease the effective (used) TDCeff below the measured
> > value
> > TDCmeas.
> > 
> > As far as I have understood our FPGA guy the TDCmeas value is the number of
> > clock cycles counted from the start of transmitting a dominant bit until the
> > dominant state reaches the RX pin.
> 
> Your definition of TDCmeas is consistent with the definition of
> TDCV in socket CAN.
> 
> What I miss to understand is what does it mean to subtract from
> that TDCmeas/TDCV value. If you subtract from it, it means that
> TDCeff/SSP is sampled before the signal reaches the RX
> pin. Correct?
> 
> > During the data phase the sample point is controlled by the tseg values set
> > for
> > the data phase but is moved additionally by the number of clocks specified
> > by
> > TDCeff (or SSP in the mcp2518fd case).
> 
> Here I do not follow you. The SSP, as specified in ISO 11898-1
> is "specified by its distance from the start of the bit
> time". Either you do not use TDC and the measurement is done on
> the SP according to the tseg values, either you do use TDC and
> the measurement is done on the SSP according to the TDC
> values. There is no mention of mixing the tseg and tdc values.
> 
> P.S.: don't hesitate to invite your FPGA guy to this thread!

I would like to do but he left off for holidays this weekend. So what I tell
here should be taken with a grain of salt.

I've had a look at the ISO specificaton and the chapter on Transmitter delay
compensation. It was not aware of the exact definition of SSP in the ISO spec.
But I can explain our implementation and the relation to the ISO spec.

In our implementation during transmit our RX-state machine runs skewed later by
TDCeff timequanta than the TX-state machine. This leads to the timing effects
described below.

For this discussion I would define SPO (Sample Point Offset) as sum of time
quanta of Sync Segment, Prop_Segment and Phase1 segment set for the data phase,
i. e. the time quanta till Sample Point in the data phase.


Bittiming for a single bit in the data phase


  |<-------- SPO ---------->|
  |                                   |
--+-----------------------------------+-- TX-Bit
   \                        ^
    \
     \
      \
       \
        \
         \
  |<---->| TDCmeas
          \
           \
            \
         |<->| TDCoffs
             |
  |<-------->| TDCeff
             |<-------- SPO ---------->|
             |                                   |
           --+-----------------------------------+-- RX-Bit
                                       ^
  |<------------ SSP ----------------->|


The sketch should show the timing relations between transmitted and received
bits. You see in our implementation the SSP is calculated as the sum of TDCeff
and SPO where in turn TDCeff is the sum of TDCmeas and TDCoffs:

SSP = TDCeff + SPO	and TDCeff = TDCmeas + TDCoffs ==>

SSP = TDCmeas + TDCoffs + SPO

Where (all in time quanta)
TDCmeas = measured TDC delay like TDCV from Microchip data sheet
TDCoffs = our ESDACC register value for the TDC offset
SPO     = offset to data phase sample point as defined before

In comparision to the ISO spec the SSP offset "SSPO" from figure 24 would then
be for our implementation:

SSPO = TDCoffs + SPO

And from your description your are thinking to implement the SSPO to be struct
can_tdc::tdco. 

If you take "our" formula for SSPO into account you can see that a negative
TDCoffs can be of use because it is always offsetted by SPO. And you're right
that a SSPO less than zero would sample the line before the bit has arrived.

I think the reason for this kind of implementation was that if you enable
automatic mode and set TDCoffs to zero it does basically "the right thing". That
is TDCoffs is independent from the settings done for segments in the data phase
because the resulting sample point offset (SPO) is cared for automatically.


To understand the TDC configuration opportunities of the MPC2518FD more
thoroughly I've looked at its reference manual


https://ww1.microchip.com/downloads/en/DeviceDoc/MCP25XXFD-CAN-FD-Controller-Module-Family-Reference-Manual-DS20005678E.pdf

and also had a look at some example bittiming calculations done with 

https://ww1.microchip.com/downloads/en/DeviceDoc/MCP2517FD%20Bit%20Time%20Calculations%20-%20UG.xlsx

These documents are linked here: 
https://www.microchip.com/wwwproducts/en/MCP2518FD

In the Excel calculation sheet the TDCO is calculated as
TDCO = (DPRSEG + DPH1SEG) * DBRP
which makes it  also dependend from the data phase prescaler. This means that
the recommended initial TDCO (which really seems to be SSPO in their
implementation) depends on (DPRSEG + DPH1SEG) which is basically the SPO as
defined for our implementation.

But this also means for a user that when setting TDCO via struct can_tdc::tdco
the full configuration of the data bitrate must be known. Additionally changing
the data bitrate sample point will make the TDC settings unsuitable for the the
new data bitrate setting.

What this means on how to implement a nice user interface to these parameters I
don't know at the moment.

Best regards,
    Stefan Mätje

Re: CAN-FD Transmitter Delay Compensation (TDC) on mcp2518fd

[Vincent MAILHOL]

Le mar. 22 juin 2021 à 03:42, Stefan Mätje <Stefan.Maetje@esd.eu> a écrit :
>
> Am Samstag, den 19.06.2021, 21:34 +0900 schrieb Vincent MAILHOL:
> > On Sat. 19 Jun 2021 à 00:55, Stefan Mätje <Stefan.Maetje@esd.eu> wrote:
> > > Am Freitag, den 18.06.2021, 23:27 +0900 schrieb Vincent MAILHOL:
> > > > On Fri. 18 Jun 2021 at 21:44, Marc Kleine-Budde <mkl@pengutronix.de>
> > > > wrote:
> > > > > On 18.06.2021 20:17:51, Vincent MAILHOL wrote:
> > > > > > > > I just noticed in the mcp2518fd data sheet:
> > > > > > > >
> > > > > > > > > bit 14-8 TDCO[6:0]: Transmitter Delay Compensation Offset bits;
> > > > > > > > > Secondary Sample Point (SSP) Two’s complement; offset can be
> > > > > > > > > positive,
> > > > > > > > > zero, or negative.
> > > > > > > > >
> > > > > > > > > 011 1111 = 63 x TSYSCLK
> > > > > > > > > ...
> > > > > > > > > 000 0000 = 0 x TSYSCLK
> > > > > > > > > ...
> > > > > > > > > 111 1111 = –64 x TSYSCLK
> > > > > > > >
> > > > > > > > Have you takes this into account?
> > > > > > >
> > > > > > > I have not. And I fail to understand what would be the physical
> > > > > > > meaning if TDCO is zero or negative.
> > > > >
> > > > > The mcp25xxfd family data sheet says:
> > > > >
> > > > > > SSP = TDCV + TDCO
> > > > > > > TDCV indicates the position of the bit start on the RX pin.
> > > > >
> > > > > If I understand correctly in automatic mode TDCV is measured by the CAN
> > > > > controller and reflects the transceiver delay.
> > > >
> > > > Yes. I phrased it poorly but this is what I wanted to say. It is
> > > > the delay to propagate from the TX pin to the RX pin.
> > > >
> > > > If TDCO = 0 then SSP = TDCV + 0 = TDCV thus the measurement
> > > > occurs at the bit start on the RX pin.
> > > >
> > > > > I don't know why you want
> > > > > to subtract a time from that....
> > > > >
> > > > > The rest of the relevant registers:
> > > > >
> > > > > > TDCMOD[1:0]: Transmitter Delay Compensation Mode bits; Secondary
> > > > > > Sample
> > > > > > Point (SSP)
> > > > > > 10-11 = Auto; measure delay and add TDCO.
> > > > > > 01 = Manual; Do not measure, use TDCV + TDCO from register
> > > > > > 00 = TDC Disabled
> > > > > >
> > > > > > TDCO[6:0]: Transmitter Delay Compensation Offset bits; Secondary
> > > > > > Sample
> > > > > > Point (SSP)
> > > > > > Two’s complement; offset can be positive, zero, or negative.
> > > > > > 011 1111 = 63 x TSYSCLK
> > > > > > ...
> > > > > > 000 0000 = 0 x TSYSCLK
> > > > > > ...
> > > > > > 111 1111 = –64 x TSYSCLK
> > > > > >
> > > > > > TDCV[5:0]: Transmitter Delay Compensation Value bits; Secondary Sample
> > > > > > Point (SSP)
> > > > > > 11 1111 = 63 x TSYSCLK
> > > > > > ...
> > > > > > 00 0000 = 0 x TSYSCLK
> > > >
> > > > Aside from the negative TDCO, the rest is standard stuff. We can
> > > > note the absence of the TDCF but that's not a blocker.
> > > >
> > > > > > > If TDCO is zero, the measurement occurs on the bit start when all
> > > > > > > the ringing occurs. That is a really bad choice to do the
> > > > > > > measurement. If it is negative, it means that you are measuring the
> > > > > > > previous bit o_O !?
> > > > >
> > > > > I don't know...
> > > > >
> > > > > > > Maybe I am missing something but I just do not get it.
> > > > > > >
> > > > > > > I believe you started to implement the mcp2518fd.
> > > > >
> > > > > No I've just looked into the register description.
> > > >
> > > > OK. For your information, the ETAS ES58x FD devices do not allow
> > > > the use of manual mode for TDCV. The microcontroller from
> > > > Microchip supports it but ETAS firmware only exposes the
> > > > automatic TDCV mode. So it can not be used to test what would
> > > > occur if SSP = 0.
> > > >
> > > > I will prepare a patch to allow zero value for both TDCV and
> > > > TDCO (I am a bit sad because I prefer the current design, but if
> > > > ISO allows it, I feel like I have no choice).  However, I refuse
> > > > to allow the negative TDCO value unless someone is able to
> > > > explain the rationale.
> > >
> > > Hi,
> > >
> > > perhaps I can shed some light on the idea why it is a good idea to allow
> > > negative TDC offset values. Therefore I would describe the TDC register
> > > interface of the ESDACC CAN-FD controller of our company (see
> > > https://esd.eu/en/products/esdacc).
> >
> > Thanks for joining the conversation. I am happy to receive help
> > from more experts!
> >
> > > Register description of TDC-CAN-FD register (reserved bits not shown):
> > >
> > > bits [5..0], RO: TDC Measured (TDCmeas)
> > >         Currently measured TDC value, needs baudrate to be set and CAN
> > > traffic
> > >
> > > bits [21..16], R/W: TDC offset (TDCoffs)
> > >         Depending on the selected mode (see TDC mode)
> > >         - Auto TDC, automatic mode (default)
> > >                 signed offset onto measured TDC (TDCeff = TDCmeas +
> > > TDCoffs),
> > >                 interpreted as 6-bit two's complement value
> > >         - Manual TDC
> > >                 absolute unsigned offset (TDCeff = TDCoffs),
> > >                 interpreted as 6-bit unsigned value
> > >         - Other modes
> > >                 ignored
> > >         In either case TDC offset is a number of CAN clock cycles.
> > >
> > > bits [31..30], R/W: TDC mode
> > >         00 = Auto TDC
> > >         01 = Manual TDC
> > >         10 = reserved
> > >         11 = TDC off
> >
> > First remark is that you use different naming than what I
> > witnessed so far in other datasheets. Let me try to give the
> > equivalences between your device and the struct can_tdc which I
> > proposed in my patches.
> >
> > The Left members are ESDACC CAN-FD registers, the right members
> > are variables from Socket CAN.
> >
> > ** Auto TDC **
> > TDCoffs = struct can_tdc::tdco
> >
> > ** Manual TDC **
> > TDCoffs = struct can_tdc::tdcv + struct can_tdc::tdco
> >
> > In both cases, TDCeff corresponds to the SSP position.
>
> TDCeff is not the SSP position in our implementation. see below.
>
> >
> > > So in automatic mode the goal is to be able to move the real sample point
> > > forward and(!) backward from the measured transmitter delay. Therefore the
> > > TDCoffs is interpreted as 6-bit two's complement value to make negative
> > > offsets
> > > possible and to decrease the effective (used) TDCeff below the measured
> > > value
> > > TDCmeas.
> > >
> > > As far as I have understood our FPGA guy the TDCmeas value is the number of
> > > clock cycles counted from the start of transmitting a dominant bit until the
> > > dominant state reaches the RX pin.
> >
> > Your definition of TDCmeas is consistent with the definition of
> > TDCV in socket CAN.
> >
> > What I miss to understand is what does it mean to subtract from
> > that TDCmeas/TDCV value. If you subtract from it, it means that
> > TDCeff/SSP is sampled before the signal reaches the RX
> > pin. Correct?
> >
> > > During the data phase the sample point is controlled by the tseg values set
> > > for
> > > the data phase but is moved additionally by the number of clocks specified
> > > by
> > > TDCeff (or SSP in the mcp2518fd case).
> >
> > Here I do not follow you. The SSP, as specified in ISO 11898-1
> > is "specified by its distance from the start of the bit
> > time". Either you do not use TDC and the measurement is done on
> > the SP according to the tseg values, either you do use TDC and
> > the measurement is done on the SSP according to the TDC
> > values. There is no mention of mixing the tseg and tdc values.
> >
> > P.S.: don't hesitate to invite your FPGA guy to this thread!
>
> I would like to do but he left off for holidays this weekend. So what I tell
> here should be taken with a grain of salt.
>
> I've had a look at the ISO specificaton and the chapter on Transmitter delay
> compensation. It was not aware of the exact definition of SSP in the ISO spec.
> But I can explain our implementation and the relation to the ISO spec.
>
> In our implementation during transmit our RX-state machine runs skewed later by
> TDCeff timequanta than the TX-state machine. This leads to the timing effects
> described below.
>
> For this discussion I would define SPO (Sample Point Offset) as sum of time
> quanta of Sync Segment, Prop_Segment and Phase1 segment set for the data phase,
> i. e. the time quanta till Sample Point in the data phase.

FYI, the sample point is already available in Socket CAN but it
is expressed in tenth of percent. You can simply convert it back
to time quanta doing:
|        u32 sample_point_in_tq = can_bit_time(dbt) * dbt->sample_point / 1000;

(which is a formula I already used to calculate TDCO:
https://elixir.bootlin.com/linux/v5.13-rc6/source/drivers/net/can/dev/bittiming.c#L194)

> Bittiming for a single bit in the data phase
>
>
>   |<-------- SPO ---------->|
>   |                                   |
> --+-----------------------------------+-- TX-Bit
>    \                        ^
>     \
>      \
>       \
>        \
>         \
>          \
>   |<---->| TDCmeas
>           \
>            \
>             \
>          |<->| TDCoffs
>              |
>   |<-------->| TDCeff
>              |<-------- SPO ---------->|
>              |                                   |
>            --+-----------------------------------+-- RX-Bit
>                                        ^
>   |<------------ SSP ----------------->|
>
>
> The sketch should show the timing relations between transmitted and received
> bits. You see in our implementation the SSP is calculated as the sum of TDCeff
> and SPO where in turn TDCeff is the sum of TDCmeas and TDCoffs:
>
> SSP = TDCeff + SPO      and TDCeff = TDCmeas + TDCoffs ==>
>
> SSP = TDCmeas + TDCoffs + SPO
>
> Where (all in time quanta)
> TDCmeas = measured TDC delay like TDCV from Microchip data sheet
> TDCoffs = our ESDACC register value for the TDC offset
> SPO     = offset to data phase sample point as defined before
>
> In comparision to the ISO spec the SSP offset "SSPO" from figure 24 would then
> be for our implementation:
>
> SSPO = TDCoffs + SPO
>
> And from your description your are thinking to implement the SSPO to be struct
> can_tdc::tdco.
>
> If you take "our" formula for SSPO into account you can see that a negative
> TDCoffs can be of use because it is always offsetted by SPO. And you're right
> that a SSPO less than zero would sample the line before the bit has arrived.
>
> I think the reason for this kind of implementation was that if you enable
> automatic mode and set TDCoffs to zero it does basically "the right thing". That
> is TDCoffs is independent from the settings done for segments in the data phase
> because the resulting sample point offset (SPO) is cared for automatically.

Thank you. What you are saying makes sense. To me, there is only
one thing that is a bit strange in your sketch: the TDCmeas/TDCV
does not indicate the beginning of the RX-bit.

I tried to modify your sketch with my vision.

  |<-------- SPO ---------->|
  |                                   |
--+-----------------------------------+-- TX-Bit
   \                        ^
    \
     \
      \
       \
        \
         \
          \
           \
            \
|<---------->| TDCmeas (by definition, TDCmeas/TDCV is the measured delay,
             |          i.e. it indicates the beginning of the RX-bit)
             |
             |<-------- SPO ---------->|
             |                         |<->| TDCoffs (might be negative)
             |<--------------------------->| TDCeff = SPO + TDCoffs
             |                                   |
           --+-----------------------------------+-- RX-Bit
                                           ^
|<---------------- SSP ------------------->|

The above is still consistent with your formulas for ESDACC CAN-FD:

SSP = TDCmeas + TDCeff
    = TDCmeas + SPO + TDCoffs

And this is how to use Socket CAN variables to calculates yours:

SPO = can_bit_time(&priv->data_bittiming) *
      priv->data_bittiming.sample_point / 1000;

TDCoffs = priv->tdc.tdco - SPO

So here, Socket CAN's TDCO is indeed an absolute value because it
is an offset on TDCmeas/TDCV whereas in your implementation,
TDCoffs is a relative value because it is an offset on
TDCmeas/TDCV + TDCeff.

> To understand the TDC configuration opportunities of the MPC2518FD more
> thoroughly I've looked at its reference manual
>
>
> https://ww1.microchip.com/downloads/en/DeviceDoc/MCP25XXFD-CAN-FD-Controller-Module-Family-Reference-Manual-DS20005678E.pdf
>
> and also had a look at some example bittiming calculations done with
>
> https://ww1.microchip.com/downloads/en/DeviceDoc/MCP2517FD%20Bit%20Time%20Calculations%20-%20UG.xlsx
>
> These documents are linked here:
> https://www.microchip.com/wwwproducts/en/MCP2518FD
>
> In the Excel calculation sheet the TDCO is calculated as
> TDCO = (DPRSEG + DPH1SEG) * DBRP
> which makes it  also dependend from the data phase prescaler. This means that
> the recommended initial TDCO (which really seems to be SSPO in their
> implementation) depends on (DPRSEG + DPH1SEG) which is basically the SPO as
> defined for our implementation.
>
> But this also means for a user that when setting TDCO via struct can_tdc::tdco
> the full configuration of the data bitrate must be known. Additionally changing
> the data bitrate sample point will make the TDC settings unsuitable for the the
> new data bitrate setting.
>
> What this means on how to implement a nice user interface to these parameters I
> don't know at the moment.

This should not be an issue. In the interface I am writing, I am
forcing the user to provide both the data bitrate and the TDC
settings at the same time.

Long story short, I now understand that negative TDCO thing (and
thanks again for your long write-up). It is just that the
calculation is done differently. I am thinking of continuing to
use an unsigned TDCO in the socket CAN implementation and maybe
provide an helper function: can_tdc_get_relative_tdco() that will
return the signed TDCO needed for the ESDACC and the
MPC2518FD (and probably other controllers as well).


Yours sincerely,
Vincent

Re: CAN-FD Transmitter Delay Compensation (TDC) on mcp2518fd

[Vincent MAILHOL]

On Tue. 22 Jun 2021 at 08:52, Vincent MAILHOL
<mailhol.vincent@wanadoo.fr> wrote:
> Le Tue. 22 Jun 2021 at 03:42, Stefan Mätje <Stefan.Maetje@esd.eu> wrote:
> > Am Samstag, den 19.06.2021, 21:34 +0900 schrieb Vincent MAILHOL:
> > > On Sat. 19 Jun 2021 à 00:55, Stefan Mätje <Stefan.Maetje@esd.eu> wrote:
> > > > Am Freitag, den 18.06.2021, 23:27 +0900 schrieb Vincent MAILHOL:
> > > > > On Fri. 18 Jun 2021 at 21:44, Marc Kleine-Budde <mkl@pengutronix.de>
> > > > > wrote:
> > > > > > On 18.06.2021 20:17:51, Vincent MAILHOL wrote:
> > > > > > > > > I just noticed in the mcp2518fd data sheet:
> > > > > > > > >
> > > > > > > > > > bit 14-8 TDCO[6:0]: Transmitter Delay Compensation Offset bits;
> > > > > > > > > > Secondary Sample Point (SSP) Two’s complement; offset can be
> > > > > > > > > > positive,
> > > > > > > > > > zero, or negative.
> > > > > > > > > >
> > > > > > > > > > 011 1111 = 63 x TSYSCLK
> > > > > > > > > > ...
> > > > > > > > > > 000 0000 = 0 x TSYSCLK
> > > > > > > > > > ...
> > > > > > > > > > 111 1111 = –64 x TSYSCLK
> > > > > > > > >
> > > > > > > > > Have you takes this into account?
> > > > > > > >
> > > > > > > > I have not. And I fail to understand what would be the physical
> > > > > > > > meaning if TDCO is zero or negative.
> > > > > >
> > > > > > The mcp25xxfd family data sheet says:
> > > > > >
> > > > > > > SSP = TDCV + TDCO
> > > > > > > > TDCV indicates the position of the bit start on the RX pin.
> > > > > >
> > > > > > If I understand correctly in automatic mode TDCV is measured by the CAN
> > > > > > controller and reflects the transceiver delay.
> > > > >
> > > > > Yes. I phrased it poorly but this is what I wanted to say. It is
> > > > > the delay to propagate from the TX pin to the RX pin.
> > > > >
> > > > > If TDCO = 0 then SSP = TDCV + 0 = TDCV thus the measurement
> > > > > occurs at the bit start on the RX pin.
> > > > >
> > > > > > I don't know why you want
> > > > > > to subtract a time from that....
> > > > > >
> > > > > > The rest of the relevant registers:
> > > > > >
> > > > > > > TDCMOD[1:0]: Transmitter Delay Compensation Mode bits; Secondary
> > > > > > > Sample
> > > > > > > Point (SSP)
> > > > > > > 10-11 = Auto; measure delay and add TDCO.
> > > > > > > 01 = Manual; Do not measure, use TDCV + TDCO from register
> > > > > > > 00 = TDC Disabled
> > > > > > >
> > > > > > > TDCO[6:0]: Transmitter Delay Compensation Offset bits; Secondary
> > > > > > > Sample
> > > > > > > Point (SSP)
> > > > > > > Two’s complement; offset can be positive, zero, or negative.
> > > > > > > 011 1111 = 63 x TSYSCLK
> > > > > > > ...
> > > > > > > 000 0000 = 0 x TSYSCLK
> > > > > > > ...
> > > > > > > 111 1111 = –64 x TSYSCLK
> > > > > > >
> > > > > > > TDCV[5:0]: Transmitter Delay Compensation Value bits; Secondary Sample
> > > > > > > Point (SSP)
> > > > > > > 11 1111 = 63 x TSYSCLK
> > > > > > > ...
> > > > > > > 00 0000 = 0 x TSYSCLK
> > > > >
> > > > > Aside from the negative TDCO, the rest is standard stuff. We can
> > > > > note the absence of the TDCF but that's not a blocker.
> > > > >
> > > > > > > > If TDCO is zero, the measurement occurs on the bit start when all
> > > > > > > > the ringing occurs. That is a really bad choice to do the
> > > > > > > > measurement. If it is negative, it means that you are measuring the
> > > > > > > > previous bit o_O !?
> > > > > >
> > > > > > I don't know...
> > > > > >
> > > > > > > > Maybe I am missing something but I just do not get it.
> > > > > > > >
> > > > > > > > I believe you started to implement the mcp2518fd.
> > > > > >
> > > > > > No I've just looked into the register description.
> > > > >
> > > > > OK. For your information, the ETAS ES58x FD devices do not allow
> > > > > the use of manual mode for TDCV. The microcontroller from
> > > > > Microchip supports it but ETAS firmware only exposes the
> > > > > automatic TDCV mode. So it can not be used to test what would
> > > > > occur if SSP = 0.
> > > > >
> > > > > I will prepare a patch to allow zero value for both TDCV and
> > > > > TDCO (I am a bit sad because I prefer the current design, but if
> > > > > ISO allows it, I feel like I have no choice).  However, I refuse
> > > > > to allow the negative TDCO value unless someone is able to
> > > > > explain the rationale.
> > > >
> > > > Hi,
> > > >
> > > > perhaps I can shed some light on the idea why it is a good idea to allow
> > > > negative TDC offset values. Therefore I would describe the TDC register
> > > > interface of the ESDACC CAN-FD controller of our company (see
> > > > https://esd.eu/en/products/esdacc).
> > >
> > > Thanks for joining the conversation. I am happy to receive help
> > > from more experts!
> > >
> > > > Register description of TDC-CAN-FD register (reserved bits not shown):
> > > >
> > > > bits [5..0], RO: TDC Measured (TDCmeas)
> > > >         Currently measured TDC value, needs baudrate to be set and CAN
> > > > traffic
> > > >
> > > > bits [21..16], R/W: TDC offset (TDCoffs)
> > > >         Depending on the selected mode (see TDC mode)
> > > >         - Auto TDC, automatic mode (default)
> > > >                 signed offset onto measured TDC (TDCeff = TDCmeas +
> > > > TDCoffs),
> > > >                 interpreted as 6-bit two's complement value
> > > >         - Manual TDC
> > > >                 absolute unsigned offset (TDCeff = TDCoffs),
> > > >                 interpreted as 6-bit unsigned value
> > > >         - Other modes
> > > >                 ignored
> > > >         In either case TDC offset is a number of CAN clock cycles.
> > > >
> > > > bits [31..30], R/W: TDC mode
> > > >         00 = Auto TDC
> > > >         01 = Manual TDC
> > > >         10 = reserved
> > > >         11 = TDC off
> > >
> > > First remark is that you use different naming than what I
> > > witnessed so far in other datasheets. Let me try to give the
> > > equivalences between your device and the struct can_tdc which I
> > > proposed in my patches.
> > >
> > > The Left members are ESDACC CAN-FD registers, the right members
> > > are variables from Socket CAN.
> > >
> > > ** Auto TDC **
> > > TDCoffs = struct can_tdc::tdco
> > >
> > > ** Manual TDC **
> > > TDCoffs = struct can_tdc::tdcv + struct can_tdc::tdco
> > >
> > > In both cases, TDCeff corresponds to the SSP position.
> >
> > TDCeff is not the SSP position in our implementation. see below.
> >
> > >
> > > > So in automatic mode the goal is to be able to move the real sample point
> > > > forward and(!) backward from the measured transmitter delay. Therefore the
> > > > TDCoffs is interpreted as 6-bit two's complement value to make negative
> > > > offsets
> > > > possible and to decrease the effective (used) TDCeff below the measured
> > > > value
> > > > TDCmeas.
> > > >
> > > > As far as I have understood our FPGA guy the TDCmeas value is the number of
> > > > clock cycles counted from the start of transmitting a dominant bit until the
> > > > dominant state reaches the RX pin.
> > >
> > > Your definition of TDCmeas is consistent with the definition of
> > > TDCV in socket CAN.
> > >
> > > What I miss to understand is what does it mean to subtract from
> > > that TDCmeas/TDCV value. If you subtract from it, it means that
> > > TDCeff/SSP is sampled before the signal reaches the RX
> > > pin. Correct?
> > >
> > > > During the data phase the sample point is controlled by the tseg values set
> > > > for
> > > > the data phase but is moved additionally by the number of clocks specified
> > > > by
> > > > TDCeff (or SSP in the mcp2518fd case).
> > >
> > > Here I do not follow you. The SSP, as specified in ISO 11898-1
> > > is "specified by its distance from the start of the bit
> > > time". Either you do not use TDC and the measurement is done on
> > > the SP according to the tseg values, either you do use TDC and
> > > the measurement is done on the SSP according to the TDC
> > > values. There is no mention of mixing the tseg and tdc values.
> > >
> > > P.S.: don't hesitate to invite your FPGA guy to this thread!
> >
> > I would like to do but he left off for holidays this weekend. So what I tell
> > here should be taken with a grain of salt.
> >
> > I've had a look at the ISO specificaton and the chapter on Transmitter delay
> > compensation. It was not aware of the exact definition of SSP in the ISO spec.
> > But I can explain our implementation and the relation to the ISO spec.
> >
> > In our implementation during transmit our RX-state machine runs skewed later by
> > TDCeff timequanta than the TX-state machine. This leads to the timing effects
> > described below.
> >
> > For this discussion I would define SPO (Sample Point Offset) as sum of time
> > quanta of Sync Segment, Prop_Segment and Phase1 segment set for the data phase,
> > i. e. the time quanta till Sample Point in the data phase.
>
> FYI, the sample point is already available in Socket CAN but it
> is expressed in tenth of percent. You can simply convert it back
> to time quanta doing:
> |        u32 sample_point_in_tq = can_bit_time(dbt) * dbt->sample_point / 1000;
>
> (which is a formula I already used to calculate TDCO:
> https://elixir.bootlin.com/linux/v5.13-rc6/source/drivers/net/can/dev/bittiming.c#L194)
>
> > Bittiming for a single bit in the data phase
> >
> >
> >   |<-------- SPO ---------->|
> >   |                                   |
> > --+-----------------------------------+-- TX-Bit
> >    \                        ^
> >     \
> >      \
> >       \
> >        \
> >         \
> >          \
> >   |<---->| TDCmeas
> >           \
> >            \
> >             \
> >          |<->| TDCoffs
> >              |
> >   |<-------->| TDCeff
> >              |<-------- SPO ---------->|
> >              |                                   |
> >            --+-----------------------------------+-- RX-Bit
> >                                        ^
> >   |<------------ SSP ----------------->|
> >
> >
> > The sketch should show the timing relations between transmitted and received
> > bits. You see in our implementation the SSP is calculated as the sum of TDCeff
> > and SPO where in turn TDCeff is the sum of TDCmeas and TDCoffs:
> >
> > SSP = TDCeff + SPO      and TDCeff = TDCmeas + TDCoffs ==>
> >
> > SSP = TDCmeas + TDCoffs + SPO
> >
> > Where (all in time quanta)
> > TDCmeas = measured TDC delay like TDCV from Microchip data sheet
> > TDCoffs = our ESDACC register value for the TDC offset
> > SPO     = offset to data phase sample point as defined before
> >
> > In comparision to the ISO spec the SSP offset "SSPO" from figure 24 would then
> > be for our implementation:
> >
> > SSPO = TDCoffs + SPO
> >
> > And from your description your are thinking to implement the SSPO to be struct
> > can_tdc::tdco.
> >
> > If you take "our" formula for SSPO into account you can see that a negative
> > TDCoffs can be of use because it is always offsetted by SPO. And you're right
> > that a SSPO less than zero would sample the line before the bit has arrived.
> >
> > I think the reason for this kind of implementation was that if you enable
> > automatic mode and set TDCoffs to zero it does basically "the right thing". That
> > is TDCoffs is independent from the settings done for segments in the data phase
> > because the resulting sample point offset (SPO) is cared for automatically.
>
> Thank you. What you are saying makes sense. To me, there is only
> one thing that is a bit strange in your sketch: the TDCmeas/TDCV
> does not indicate the beginning of the RX-bit.
>
> I tried to modify your sketch with my vision.
>
>   |<-------- SPO ---------->|
>   |                                   |
> --+-----------------------------------+-- TX-Bit
>    \                        ^
>     \
>      \
>       \
>        \
>         \
>          \
>           \
>            \
>             \
> |<---------->| TDCmeas (by definition, TDCmeas/TDCV is the measured delay,
>              |          i.e. it indicates the beginning of the RX-bit)
>              |
>              |<-------- SPO ---------->|
>              |                         |<->| TDCoffs (might be negative)
>              |<--------------------------->| TDCeff = SPO + TDCoffs
>              |                                   |
>            --+-----------------------------------+-- RX-Bit
>                                            ^
> |<---------------- SSP ------------------->|

Sorry, the above sketch is incorrect. This is the correct version:

  |<-------- SPO ---------->|
  |                                   |
--+-----------------------------------+-- TX-Bit
   \                        ^
    \
     \
      \
       \
        \
         \
          \
           \
            \
|<---------->| TDCmeas (by definition, TDCmeas/TDCV is the measured delay,
             |          i.e. it indicates the beginning of the RX-bit)
             |
             |<->| TDCoffs (might be negative)
|<-------------->| TDCeff = TDCmeas + TDCoffs
             |   |<-------- SPO ---------->|
             |                                   |
           --+-----------------------------------+-- RX-Bit
                                           ^
|<---------------- SSP ------------------->|

> The above is still consistent with your formulas for ESDACC CAN-FD:
>
> SSP = TDCmeas + TDCeff
>     = TDCmeas + SPO + TDCoffs

I also made a mistake here. What you sent in your previous
message was correct:

SSP = TDCeff + SPO
    = TDCmeas + TDCoffs + SPO

> And this is how to use Socket CAN variables to calculates yours:
>
> SPO = can_bit_time(&priv->data_bittiming) *
>       priv->data_bittiming.sample_point / 1000;
>
> TDCoffs = priv->tdc.tdco - SPO
>
> So here, Socket CAN's TDCO is indeed an absolute value because it
> is an offset on TDCmeas/TDCV whereas in your implementation,
> TDCoffs is a relative value because it is an offset on
> TDCmeas/TDCV + TDCeff.
>
> > To understand the TDC configuration opportunities of the MPC2518FD more
> > thoroughly I've looked at its reference manual
> >
> >
> > https://ww1.microchip.com/downloads/en/DeviceDoc/MCP25XXFD-CAN-FD-Controller-Module-Family-Reference-Manual-DS20005678E.pdf
> >
> > and also had a look at some example bittiming calculations done with
> >
> > https://ww1.microchip.com/downloads/en/DeviceDoc/MCP2517FD%20Bit%20Time%20Calculations%20-%20UG.xlsx
> >
> > These documents are linked here:
> > https://www.microchip.com/wwwproducts/en/MCP2518FD
> >
> > In the Excel calculation sheet the TDCO is calculated as
> > TDCO = (DPRSEG + DPH1SEG) * DBRP
> > which makes it  also dependend from the data phase prescaler. This means that
> > the recommended initial TDCO (which really seems to be SSPO in their
> > implementation) depends on (DPRSEG + DPH1SEG) which is basically the SPO as
> > defined for our implementation.
> >
> > But this also means for a user that when setting TDCO via struct can_tdc::tdco
> > the full configuration of the data bitrate must be known. Additionally changing
> > the data bitrate sample point will make the TDC settings unsuitable for the the
> > new data bitrate setting.
> >
> > What this means on how to implement a nice user interface to these parameters I
> > don't know at the moment.
>
> This should not be an issue. In the interface I am writing, I am
> forcing the user to provide both the data bitrate and the TDC
> settings at the same time.
>
> Long story short, I now understand that negative TDCO thing (and
> thanks again for your long write-up). It is just that the
> calculation is done differently. I am thinking of continuing to
> use an unsigned TDCO in the socket CAN implementation and maybe
> provide an helper function: can_tdc_get_relative_tdco() that will
> return the signed TDCO needed for the ESDACC and the
> MPC2518FD (and probably other controllers as well).
>
>
> Yours sincerely,
> Vincent