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. > > * 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 > >
