Re: [PATCH v5 2/7] can: bittiming: allow TDC{V,O} to be zero and add can_tdc_const::tdc{v,o,f}_min

From: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
To: Marc Kleine-Budde <mkl@pengutronix.de>
Cc: linux-can <linux-can@vger.kernel.org>,
	"Stefan Mätje" <Stefan.Maetje@esd.eu>,
	netdev <netdev@vger.kernel.org>,
	"open list" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v5 2/7] can: bittiming: allow TDC{V,O} to be zero and add can_tdc_const::tdc{v,o,f}_min
Date: Mon, 16 Aug 2021 19:24:43 +0900	[thread overview]
Message-ID: <CAMZ6RqK5t62UppiMe9k5jG8EYvnSbFW3doydhCvp72W_X2rXAw@mail.gmail.com> (raw)
In-Reply-To: <20210816084235.fr7fzau2ce7zl4d4@pengutronix.de>

On Mon. 16 Aug 2021 at 17:42, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 15.08.2021 12:32:43, Vincent Mailhol wrote:
> > 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?

> > 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.
| SSP = TDCV + absolute TDCO
|     = TDCV + SP + relative TDCO

Consequently:
| relative TDCO = absolute TDCO - SP

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

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.

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

Yours sincerely,
Vincent