Re: [PATCH net 2/6] can: mcp251xfd: fix TDC setting for low data bit rates

[Jacob Keller <jacob.e.keller@intel.com>]

On 5/6/2025 6:56 AM, Marc Kleine-Budde wrote:
> From: Kelsey Maes <kelsey@vpprocess.com>
> 
> The TDC is currently hardcoded enabled. This means that even for lower
> CAN-FD data bitrates (with a DBRP (data bitrate prescaler) > 2) a TDC
> is configured. This leads to a bus-off condition.
> 
> ISO 11898-1 section 11.3.3 says "Transmitter delay compensation" (TDC)
> is only applicable if DBRP is 1 or 2.
> 
> To fix the problem, switch the driver to use the TDC calculation
> provided by the CAN driver framework (which respects ISO 11898-1
> section 11.3.3). This has the positive side effect that userspace can
> control TDC as needed.
> 
> Demonstration of the feature in action:
> | $ ip link set can0 up type can bitrate 125000 dbitrate 500000 fd on
> | $ ip -details link show can0
> | 3: can0: <NOARP,UP,LOWER_UP,ECHO> mtu 72 qdisc pfifo_fast state UP mode DEFAULT group default qlen 10
> |     link/can  promiscuity 0  allmulti 0 minmtu 0 maxmtu 0
> |     can <FD> state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 0
> | 	  bitrate 125000 sample-point 0.875
> | 	  tq 50 prop-seg 69 phase-seg1 70 phase-seg2 20 sjw 10 brp 2
> | 	  mcp251xfd: tseg1 2..256 tseg2 1..128 sjw 1..128 brp 1..256 brp_inc 1
> | 	  dbitrate 500000 dsample-point 0.875
> | 	  dtq 125 dprop-seg 6 dphase-seg1 7 dphase-seg2 2 dsjw 1 dbrp 5
> | 	  mcp251xfd: dtseg1 1..32 dtseg2 1..16 dsjw 1..16 dbrp 1..256 dbrp_inc 1
> | 	  tdcv 0..63 tdco 0..63
> | 	  clock 40000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 tso_max_size 65536 tso_max_segs 65535 gro_max_size 65536 parentbus spi parentdev spi0.0
> | $ ip link set can0 up type can bitrate 1000000 dbitrate 4000000 fd on
> | $ ip -details link show can0
> | 3: can0: <NOARP,UP,LOWER_UP,ECHO> mtu 72 qdisc pfifo_fast state UP mode DEFAULT group default qlen 10
> |     link/can  promiscuity 0  allmulti 0 minmtu 0 maxmtu 0
> |     can <FD,TDC-AUTO> state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 0
> | 	  bitrate 1000000 sample-point 0.750
> | 	  tq 25 prop-seg 14 phase-seg1 15 phase-seg2 10 sjw 5 brp 1
> | 	  mcp251xfd: tseg1 2..256 tseg2 1..128 sjw 1..128 brp 1..256 brp_inc 1
> | 	  dbitrate 4000000 dsample-point 0.700
> | 	  dtq 25 dprop-seg 3 dphase-seg1 3 dphase-seg2 3 dsjw 1 dbrp 1
> | 	  tdco 7
> | 	  mcp251xfd: dtseg1 1..32 dtseg2 1..16 dsjw 1..16 dbrp 1..256 dbrp_inc 1
> | 	  tdcv 0..63 tdco 0..63
> | 	  clock 40000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 tso_max_size 65536 tso_max_segs 65535 gro_max_size 65536 parentbus spi parentdev spi0.0
> 
> There has been some confusion about the MCP2518FD using a relative or
> absolute TDCO due to the datasheet specifying a range of [-64,63]. I
> have a custom board with a 40 MHz clock and an estimated loop delay of
> 100 to 216 ns. During testing at a data bit rate of 4 Mbit/s I found
> that using can_get_relative_tdco() resulted in bus-off errors. The
> final TDCO value was 1 which corresponds to a 10% SSP in an absolute
> configuration. This behavior is expected if the TDCO value is really
> absolute and not relative. Using priv->can.tdc.tdco instead results in
> a final TDCO of 8, setting the SSP at exactly 80%. This configuration
> works.
> 
> The automatic, manual, and off TDC modes were tested at speeds up to,
> and including, 8 Mbit/s on real hardware and behave as expected.
> 
> Fixes: 55e5b97f003e ("can: mcp25xxfd: add driver for Microchip MCP25xxFD SPI CAN")
> Reported-by: Kelsey Maes <kelsey@vpprocess.com>
> Closes: https://lore.kernel.org/all/C2121586-C87F-4B23-A933-845362C29CA1@vpprocess.com
> Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> Signed-off-by: Kelsey Maes <kelsey@vpprocess.com>
> Link: https://patch.msgid.link/20250430161501.79370-1-kelsey@vpprocess.com
> [mkl: add comment]
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>