Hi Vincent,

I would like to add some comments below:

Am Montag, den 16.08.2021, 14:25 +0200 schrieb Marc Kleine-Budde:

On 16.08.2021 19:24:43, Vincent MAILHOL wrote:

quoted

On Mon. 16 Aug 2021 at 17:42, Marc Kleine-Budde [off-list ref] wrote:

quoted

On 15.08.2021 12:32:43, Vincent Mailhol wrote:

quoted

ISO 11898-1 specifies in section 11.3.3 "Transmitter delay
compensation" that "the configuration range for [the] SSP position
shall be at least 0 to 63 minimum time quanta."

Because SSP = TDCV + TDCO, it means that we should allow both TDCV and
TDCO to hold zero value in order to honor SSP's minimum possible
value.

However, current implementation assigned special meaning to TDCV and
TDCO's zero values:
  * TDCV = 0 -> TDCV is automatically measured by the transceiver.
  * TDCO = 0 -> TDC is off.

In order to allow for those values to really be zero and to maintain
current features, we introduce two new flags:
  * CAN_CTRLMODE_TDC_AUTO indicates that the controller support
    automatic measurement of TDCV.
  * CAN_CTRLMODE_TDC_MANUAL indicates that the controller support
    manual configuration of TDCV. N.B.: current implementation failed
    to provide an option for the driver to indicate that only manual
    mode was supported.

TDC is disabled if both CAN_CTRLMODE_TDC_AUTO and
CAN_CTRLMODE_TDC_MANUAL flags are off, c.f. the helper function
can_tdc_is_enabled() which is also introduced in this patch.

Nitpick: We can only say that TDC is disabled, if the driver supports
the TDC interface at all, which is the case if tdc_const is set.

I would argue that saying that a device does not support TDC is
equivalent to saying that TDC is always disabled for that device.
Especially, the function can_tdc_is_enabled() can be used even if
the device does not support TDC (even if there is no benefit
doing so).

Do you still want me to rephrase this part?

quoted

quoted

Also, this patch adds three fields: tdcv_min, tdco_min and tdcf_min to
struct can_tdc_const. While we are not convinced that those three
fields could be anything else than zero, we can imagine that some
controllers might specify a lower bound on these. Thus, those minimums
are really added "just in case".

I'm not sure, if we talked about the mcp251xfd's tcdo, valid values are
-64...63.

Yes! Stefan shed some light on this. The mcp251xfd uses a tdco
value which is relative to the sample point.

I don't read the documentation this way....

@Vincent: I have to agree with Marc here. Perhaps my email
https://lore.kernel.org/linux-can/094d8a2eab2177e5a5143f96cf745b26897e1793.camel@esd.eu/ (local)
was also misleading. I also referred there to a MicroChip Excel sheet
(https://ww1.microchip.com/downloads/en/DeviceDoc/MCP2517FD%20Bit%20Time%20Calculations%20-%20UG.xlsx)
that describes the calculation of the bit timing and the TDCO. The values calculated 
there correspond to the SPO from the above email. Microchip calculates the TDCO as
TDCO = (DPRSEG + DPH1SEG) * DBRP.
Thus, as already discussed, negative values are not purposeful.

Sorry, that that email was misleading. So far I've seen now only the ESDACC 
controller has a "relative" TDCO register value where a negative value may
be sensible.

quoted

quoted

SSP = TDCV + absolute TDCO
    = TDCV + SP + relative TDCO

Consequently:

quoted

relative TDCO = absolute TDCO - SP

In the mcp15xxfd family manual
(http://ww1.microchip.com/downloads/en/DeviceDoc/MCP251XXFD-CAN-FD-Controller-Module-Family-Reference-Manual-20005678B.pdf)
in the 2mbit/s data bit rate example in table 3-5 (page 21) it says:

quoted

DTSEG1  15 DTQ
DTSEG2   4 DTQ
TDCO    15 DTQ

The mcp251xfd driver uses 15, the framework calculates 16 (== Sync Seg+
tseg1, which is correct), and relative tdco would be 0:

quoted

mcp251xfd_set_bittiming: tdco=15, priv->tdc.tdc=16, relative_tdco=0

Here the output with the patched ip tool:

quoted

4: mcp251xfd0: <NOARP,UP,LOWER_UP,ECHO> mtu 72 qdisc pfifo_fast state UP mode DEFAULT group default qlen 10
    link/can  promiscuity 0 minmtu 0 maxmtu 0 
    can <FD,TDC_AUTO> state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 100 
          bitrate 500000 sample-point 0.875
          tq 25 prop-seg 34 phase-seg1 35 phase-seg2 10 sjw 1 brp 1
          mcp251xfd: tseg1 2..256 tseg2 1..128 sjw 1..128 brp 1..256 brp_inc 1
          dbitrate 2000000 dsample-point 0.750
          dtq 25 dprop-seg 7 dphase-seg1 7 dphase-seg2 5 dsjw 1 dbrp 1
          tdco 15
          mcp251xfd: dtseg1 1..32 dtseg2 1..16 dsjw 1..16 dbrp 1..256 dbrp_inc 1
          tdco 0..127
          clock 40000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 parentbus spi parentdev spi0.0

quoted

Which is also why TDCO can be negative.

I added an helper function can_tdc_get_relative_tdco() in the
fourth path of this series:
https://lore.kernel.org/linux-can/20210814091750.73931-5-mailhol.vincent@wanadoo.fr/T/#u (local)

Devices which use the absolute TDCO can directly use
can_priv->tdc.tdco. Devices which use the relative TDCO such as
the mcp251xfd should use this helper function instead.

Don't think so....

@Vincent: Perhaps you should not implement this helper function as it is only needed
for the ESDACC so far.

quoted

However, you will still need to convert the TDCO valid range from
relative values to absolute ones. In your case 0..127.

Marc

Stefan