On Sat. 16 Jan 2021 at 20:33, Vincent Mailhol <vincent.mailhol@xxxxxxxxx> 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. > > > > * 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)) { I inverted the logic here. The correct test is: if ((dbt->brp != 1) && (dbt->brp != 2)) { because here, we are checking if tdc is *not* applicable. > > > 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 > > >
