On Sun. 17 Jan 2021 at 05:58, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote: > On 16.01.21 12:33, Vincent Mailhol wrote: > > Hello Oliver, > > > > On Sat. 16 Jan 2021 à 19:41, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote: > >> > >> Hello Vincent, > >> > >> I like this separate struct too. > >> > >> On 16.01.21 08:02, Vincent Mailhol wrote: > >>> This RFC is a follow-up on the discussion started in: > >>> https://lore.kernel.org/linux-can/20210112130538.14912-1-mailhol.vincent@xxxxxxxxxx/T/#t > >>> > >>> * Scope of this RFC * > >>> > >>> I want to discuss 1/ which TDC values we should provide to the > >>> user and 2/ how to calculate those. In this RFC, I will not > >>> directly discuss how to actually modify the netlink ABI. > >>> > >>> > >>> * The code * > >>> > >>> /** > >>> * struct can_tdc - CAN FD Transmission Delay Compensation parameters > >>> * > >>> * At high bit rates, the propagation delay from the TX pin to the RX > >>> * pin of the transceiver causes measurement errors and needs to be > >>> * corrected. > >>> * > >>> * To solve this issue, ISO 11898-1 introduces in section 11.3.3 > >>> * "Transmitter delay compensation" a SSP (Secondary Sample Point) > >>> * equal to the distance, in time quanta, from the start of the bit > >>> * time to the measurement on the RX pin. > >>> * > >>> * This structure contains the parameters to calculate that SSP. > >>> * > >>> * @tdcv: Transmitter Delay Compensation Value. Distance, in time > >> > >> would name it @tdc without 'v' as you already have it in your struct below. > > > > Both the MCP251XXFD-CAN-FD and the Microchip CAN FD Controller > > Module use this naming for their register. > > > > Throughout the documents, I also encountered "Transmitter Delay > > Value" or just "Transmitter Delay" to designate that value (but > > not as a register name). I do not recall seeing "Transmitter > > Delay Compensation" to designate the value: this nomination is > > rather used for the full concept (i.e. the name of the structure). > > > > I myself am not a huge fan of the tdcv but I still prefer to use > > it to follow the trend. If you insist to shorten it, I would > > prefer to use tdv for "Transmitter Delay Value". > > > > The tdc in my struct below was a typo. I meant to also name it tdcv. > > > >>> * quanta, from when the bit is sent on the TX pin to when it is > >>> * received on the RX pin of the transmitter. > >>> * > >>> * 0: Automatic mode (default). Use the value dynamically > >>> * measured by the controller. > >>> * > >>> * Other values: manual mode. Use the fixed provided value. > >>> * > >>> * N.B. when using the automatic mode, the dynamically measured > >>> * value might not be visible to the kernel. > >>> * > >>> * @tdco: Transmitter Delay Compensation Offset. Offset value, in time > >>> * quanta, defining the delay between the start of the bit > >>> * reception on the CANRX pin of the transceiver and the SSP such > >>> * as SSP = @tdcv + @tdco. > >> > >> SSP = @tdc + @tdco. > >> > >>> * > >>> * @tdcf: Transmitter Delay Compensation Filter window. Defines the > >> > >> @tdcw: Transmitter Delay Compensation Window. > >> > >> The value describes the window (of a filter). > > > > Same here, the microcontrollers which use this feature all tend to > > name their register tdcf. I prefer not to deviate from the existing. > > Hm, yes. I've checked your referenced CAN controller specs - and the > M_CAN spec does it similarly. > > Therefore we should go with your suggestion. > > But we should always check for the best naming and description in our > APIs. The CAN controller guys do not always find good names for their > features. It was a hard discussion to rename EDL (extended data length) > to FDF in CAN FD ;-) I didn't know that. So at the end, you could influence the naming in the ISO standard. Nice! I think that it also depends on the timing. When you had your EDL vs. FDF discussion, the ink on the Bosch specification was probably still fresh and the ISO standard was not yet finalised. Now, we are too late to the party to do any changes on the naming. If we could go back in time, I would have liked to rename "Transmitter Delay" into "Propagation Delay": https://en.wikipedia.org/wiki/Propagation_delay At least, I rewrote the description of each field. Hope it is easy enough to understand for a newcomer. > > > >>> * minimum value for the SSP position, in time quanta. If SSP is > >>> * less than @tdcf, then no compensation delay occurs and the > >>> * normal sampling point is used instead. The feature is enabled > >>> * if and only if @tdcf is configured to a value greater than @tdco. > >>> */ > >>> struct can_tdc { > >>> u16 tdc; > > > > As written above, this is a typo. I meant: > > u16 tdcv; > > > >>> u16 tdco; > >>> u16 tdcf; > >>> }; > >> > >> > >> Currently all these kind of values are __u32: > >> > >> https://elixir.bootlin.com/linux/v5.11-rc3/source/include/uapi/linux/can/netlink.h#L31 > >> > >> Shouldn't we stick on this pattern here? > >> > >> struct can_tdc { > >> __u32 tdc; > > > > __u32 tdcv; > > > >> __u32 tdco; > >> __u32 tdcw; > >> }; > > > > Based on current value: yes. I used u16 as an anticipation of the > > modification that will be done on the netlink interface. Once we > > have a dedicated kernel only bittiming structure for the kernel, > > we will have more freedom and could save a few bytes. > > > > Nonetheless, this is a discussion we can have when actually doing > > the netlink modification. For now, I am fine to use __u32. > > > >>> /** > >>> * struct can_tdc_const - CAN hardware-dependent bit-timing constant > >>> * for TDC > >>> * > >>> * @tdcv_max: Transmitter Delay Compensation Value maximum value, > >>> * should be set to zero if the controller does not support > >>> * manual mode for tdcv. > >>> * @tdco_max: Transmitter Delay Compensation Offset maximum value. > >>> * Should be set to zero if the controller does not support TDC. > >>> * @tdcf_max: Transmitter Delay Compensation Filter window maximum > >>> * value. Should be set to zero if the controller does not > >>> * support this feature. > >>> */ > >>> struct can_tdc_const { > >>> u16 tdcv_max; > >>> u16 tdco_max; > >>> u16 tdcf_max; > >>> }; > >> > >> Same question here with naming and __u32 data structure. > > > > Ack, OK for __32 for the moment. > > > >> Regards, > >> Oliver > >> > >>> > >>> /* Do the bittiming calculation of the tdc parameters */ > >>> static void can_set_tdc(const struct can_bittiming *dbt, struct can_tdc *tdc, > >>> const struct can_tdc_const *tdc_const)> > { > >>> /* As specified in ISO 11898-1 section 11.3.3 "Transmitter > >>> * delay compensation" (TDC) is only applicable if data BRP is > >>> * one or two. > >>> */ > >>> if ((dbt->brp == 1) || (dbt->brp == 2)) { > >>> memset(tdc, 0, sizeof(*tdc)); > >>> return; > >>> } > >>> > >>> tdc->tdcv = 0; > >>> /* Convert the sample point from tenth of a percent to time quanta */ > >>> tdc->tdco = min(can_bit_time(dbt) * dbt->sample_point / 100, > >>> tdc_const->tdco_max); > >>> tdc->tdcf = 0; > >>> } > >>> > >>> > >>> * Explanation on the code * > >>> > >>> The only way to calculate tdcv is through measurement which is > >>> done by the controller. This parameter is only here to receive a > >>> fixed value that would be given by the user through the netlink > >>> interface. > >>> > >>> The calculation of tdco is of my conception. I could not find any > >>> reference formula. My logic is to just reuse the normal sample > >>> point so that at the end ssp = tdcv + sp. Another method might > >>> set ssp to the middle of the bit: > >>> tdc->tdco = can_bit_time(dbt) / 2 > >>> > >>> My current proposal is not to use tdcf by default (leave it to > >>> zero) if doing bittiming calculation. Meaning that this value > >>> will only be used if configured by the user through the netlink > >>> interface. The reason for this choice are: > >>> * the lack of test environment: on my hardware (the ETAS > >>> ES582.1), everything works up fine to 8 Mbps just by using > >>> the tdco. > >>> * I could not find a good reference to calculate that value. > >>> Any ideas of how to calculate tdcf is welcome! > >>> > >>> > >>> * References * > >>> > >>> This is a collection of specifications and references which I > >>> used while writing this RFC. I believe that all TDC use cases are > >>> covered in this RFC. > >>> > >>> - Bosch C_CAN FD8: > >>> https://www.bosch-semiconductors.com/media/ip_modules/pdf_2/c_can_fd8/users_manual_c_can_fd8_r210_1.pdf > >>> > >>> - Microchip CAN FD Controller Module: > >>> http://ww1.microchip.com/downloads/en/DeviceDoc/MCP251XXFD-CAN-FD-Controller-Module-Family-Reference-Manual-20005678B.pdf > >>> > >>> - SAM E701/S70/V70/V71 Family: > >>> https://www.mouser.com/datasheet/2/268/60001527A-1284321.pdf > >>> > >>> - ISO 11898-1 > >>> > >>> > >>> Thank you for your comments, > >>> > >>> > >>> Yours sincerely, > >>> Vincent > >>>
