From: Marc Kleine-Budde <mkl@pengutronix.de>
To: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
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 14:25:19 +0200 [thread overview]
Message-ID: <20210816122519.mme272z6tqrkyc6x@pengutronix.de> (raw)
In-Reply-To: <CAMZ6RqK5t62UppiMe9k5jG8EYvnSbFW3doydhCvp72W_X2rXAw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4917 bytes --]
On 16.08.2021 19:24:43, Vincent MAILHOL wrote:
> 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.
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
>
> 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 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
next prev parent reply other threads:[~2021-08-16 12:25 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 [this message]
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
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=20210816122519.mme272z6tqrkyc6x@pengutronix.de \
--to=mkl@pengutronix.de \
--cc=Stefan.Maetje@esd.eu \
--cc=linux-can@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mailhol.vincent@wanadoo.fr \
--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).