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

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....

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

Consequently:
| 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:

| 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:

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

Here the output with the patched ip tool:

| 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

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....

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

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 |