linux-can.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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: Wed, 18 Aug 2021 18:22:33 +0900	[thread overview]
Message-ID: <CAMZ6RqKsjPF2gBbzsKatFG7S4qcOahSX9vSU=dj_e9R-Kqq0CA@mail.gmail.com> (raw)
In-Reply-To: <20210817200123.4wcdwsdfsdjr3ovk@pengutronix.de>

On Wed. 18 Aug 2021 at 05:01, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> On 17.08.2021 00:49:35, Vincent MAILHOL wrote:
> > > We have 4 operations:
> > > - tdc-mode off                  switch off tdc altogether
> > > - tdc-mode manual tdco X tdcv Y configure X and Y for tdco and tdcv
> > > - tdc-mode auto tdco X          configure X tdco and
> > >                                 controller measures tdcv automatically
> > > - /* nothing */                 configure default value for tdco
> > >                                 controller measures tdcv automatically
> >
> > The "nothing" does one more thing: it decides whether TDC should
> > be activated or not.
> >
> > > The /* nothing */ operation is what the old "ip" tool does, so we're
> > > backwards compatible here (using the old "ip" tool on an updated
> > > kernel/driver).
> >
> > That's true but this isn't the real intent. By doing this design,
> > I wanted the user to be able to transparently use TDC while
> > continuing to use the exact same ip commands she or he is used
> > to using.
>
> Backwards compatibility using an old ip tool on a new kernel/driver must
> work.

I am not trying to argue against backward compatibility :)
My comment was just to point out that I had other intents as well.

> In case of the mcp251xfd the tdc mode must be activated and tdcv
> set to the automatic calculated value and tdco automatically measured.

Sorry but I am not sure if I will follow you. Here, do you mean
that "nothing" should do the "fully automated" calculation?

In your previous message, you said:

> Does it make sense to let "mode auto" without a tdco value switch the
> controller into full automatic mode and /* nothing */ not tough the tdc
> config at all?

So, you would like this behavior:

| "nothing" -> TDC is off (not touch the tdc config at all)
| mode auto, no tdco provided -> kernel decides between TDC_AUTO and TDC off.
| mode auto, tdco provided -> TDC_AUTO
| mode manual, tdcv and tdco provided -> TDC_MANUAL
| mode off is not needed anymore (redundant with "nothing")
(TDCF left out of the picture intentionally)

Correct?

If you do so, I see three issues:

1/ Some of the drivers already implement TDC. Those will
automatically do a calculation as long as FD is on. If "nothing"
now brings TDC off, some users will find themselves with some
error on the bus after the iproute2 update if they continue using
the same command.

2/ Users will need to read and understand how to use the TDC
parameters of iproute2. And by experience, too many people just
don't read the doc. If I can make the interface transparent and
do the correct thing by default ("nothing"), I prefer to do so.

3/ Final one is more of a nitpick. The mode auto might result in
TDC being off. If we have a TDC_AUTO flag, I would expect the
auto mode to always set that flag (unless error occurs). I see
this to be slightly counter intuitive (I recognize that my
solution also has some aspects which are not intuitive, I just
want to point here that none are perfect).


To be honest, I really preferred the v1 of this series where
there were no tdc-mode {auto,manual,off} and where the "off"
behavior was controlled by setting TDCO to zero. However, as we
realized, zero is a valid value and thus, I had to add all this
complexity just to allow that damn zero value.


Yours sincerely,
Vincent

  reply	other threads:[~2021-08-18  9:22 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-15  3:32 [PATCH v5 0/7] add the netlink interface for CAN-FD Transmitter Delay Compensation (TDC) Vincent Mailhol
2021-08-15  3:32 ` [PATCH v5 1/7] can: netlink: allow user to turn off unsupported features Vincent Mailhol
2021-08-19  7:45   ` Marc Kleine-Budde
2021-08-19  9:24     ` Vincent MAILHOL
2021-08-19 10:07       ` Marc Kleine-Budde
2021-08-15  3:32 ` [PATCH v5 2/7] can: bittiming: allow TDC{V,O} to be zero and add can_tdc_const::tdc{v,o,f}_min Vincent Mailhol
2021-08-16  8:42   ` Marc Kleine-Budde
2021-08-16 10:24     ` Vincent MAILHOL
2021-08-16 12:25       ` Marc Kleine-Budde
2021-08-16 12:33         ` Marc Kleine-Budde
2021-08-16 13:43           ` Marc Kleine-Budde
2021-08-16 15:49             ` Vincent MAILHOL
2021-08-16 16:04               ` Vincent MAILHOL
2021-08-17  1:12                 ` Vincent MAILHOL
2021-08-17 20:01               ` Marc Kleine-Budde
2021-08-18  9:22                 ` Vincent MAILHOL [this message]
2021-08-18 12:29                   ` Marc Kleine-Budde
2021-08-18 14:17                     ` Vincent MAILHOL
2021-08-20  6:12                       ` Vincent MAILHOL
2021-08-16 14:10           ` Vincent MAILHOL
2021-08-16 14:30             ` Marc Kleine-Budde
2021-08-17  9:43               ` Marc Kleine-Budde
2021-08-16 13:49         ` Stefan Mätje
2021-08-16 15:56           ` Vincent MAILHOL
2021-08-15  3:32 ` [PATCH v5 3/7] can: bittiming: change unit of TDC parameters to clock periods Vincent Mailhol
2021-08-15  3:32 ` [PATCH v5 4/7] can: dev: add can_tdc_get_relative_tdco() helper function Vincent Mailhol
2021-08-16 11:27   ` Marc Kleine-Budde
2021-08-15  3:32 ` [PATCH v5 5/7] can: netlink: add interface for CAN-FD Transmitter Delay Compensation (TDC) Vincent Mailhol
2021-08-17 19:55   ` Marc Kleine-Budde
2021-08-18  8:08     ` Vincent MAILHOL
2021-08-18  8:19       ` Marc Kleine-Budde
2021-08-18  8:37         ` Vincent MAILHOL
2021-08-18  8:43           ` Marc Kleine-Budde
2021-08-15  3:32 ` [PATCH v5 6/7] can: netlink: add can_priv::do_get_auto_tdcv() to retrieve tdcv from device Vincent Mailhol
2021-08-15  3:32 ` [PATCH v5 7/7] can: etas_es58x: clean-up documentation of struct es58x_fd_tx_conf_msg Vincent Mailhol
2021-08-19 10:08   ` Marc Kleine-Budde

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAMZ6RqKsjPF2gBbzsKatFG7S4qcOahSX9vSU=dj_e9R-Kqq0CA@mail.gmail.com' \
    --to=mailhol.vincent@wanadoo.fr \
    --cc=Stefan.Maetje@esd.eu \
    --cc=linux-can@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mkl@pengutronix.de \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).