On 09.12.2024 14:13:29, Oliver Hartkopp wrote: > > > On 05.12.24 10:15, Marc Kleine-Budde wrote: > > On 05.12.2024 09:16:44, Oliver Hartkopp wrote: > > > On 04.12.24 12:44, Marc Kleine-Budde wrote: > > > > On 04.12.2024 12:35:43, Oliver Hartkopp wrote: > > > > > > > > Also, the main reason for not creating the nest was that I thought > > > > > > > > that the current bittiming API was stable. I was not aware of the > > > > > > > > current flaw on how to divide tseg1_min. Maybe we should first discuss > > > > > > > > how to solve this issue for CAN FD? > > > > > > > > > > > > > > I like the current way how you added the CAN XL support. > > > > > > > It maintains the known usage pattern - and the way how CAN XL bit timings > > > > > > > are defined is identical to CAN FD (including TDC). > > > > > > > > > > > > > > Is the separation of propseg and tseg1 that relevant? > > > > > > > Does it really need to be exposed to the user? > > > > > > > > > > > > There are IIRC at least 2 CAN-FD cores where the prop segment and phase > > > > > > segment 1 for the data bit timing have not the same width. This means we > > > > > > have to change the bittiming_const in the kernel. > > > > > > Sure? > > > > I'm sure the registers don't have the same width. And I'm sure about my > > conclusion, but that's up for discussion :) > > > > https://elixir.bootlin.com/linux/v6.12.1/source/drivers/net/can/ctucanfd/ctucanfd_base.c#L197 > > https://elixir.bootlin.com/linux/v6.12.1/source/drivers/net/can/flexcan/flexcan-core.c#L1210 > > > > > In the end (almost) every CAN controller has the tseg1 register which > > > contains prop_seg + phase_seg1 as a sum of these. > > > > Some do (just a short grep): bxcan, esdacc, rcar_can, softing, hi311x, > > ti_hecc. More controllers haven evenly divided prop_seg + phase_seg1. > > > > > The relevant point is behind prop_seg + phase_seg1 and I'm pretty sure these > > > "2 CAN-FD cores" will add the values internally too. > > > > As the ctucanfd is open you can have a look :) As far as I understand, it internally adds sync + prop + phase1: https://gitlab.fel.cvut.cz/canbus/ctucanfd_ip_core/-/blob/master/src/prescaler/bit_time_cfg_capture.vhd?ref_type=heads#L242 > > > I'm a bit concerned that after 40 years someone shows up with the idea to > > > spend two registers for the tseg1 value instead of one. > > > > It doesn't matter if prop_seg and phase_seg1 are in the same register or > > not, what matters is: > > a) 1. does the IP core want separate prop_seg and phase_seg1 values > > - or - > > 2. does the IP core want a single "prop_seg + phase_seg1", a.k.a. > > tseg1 value? > > b) 1. what's the width of the prop_seg and phase_seg1? > > 2. what's the width of tseg1? > > > > Currently the CAN infrastructure allows the driver to specify tseg1 only > > and assumes the width of prop_seg and phase_seg1 to be the same, as it > > distributes tseg1 evenly between prop_seg and phase_seg1: > > > > https://elixir.bootlin.com/linux/v6.12.1/source/drivers/net/can/dev/calc_bittiming.c#L155 > > > > This leads to the workarounds in the CAN drivers, see above for links. > > Yes. But why don't we just let this as-is then? > > Even if prop_seg phase_seg1 registers have a different size, this split up > can be done easily without changing the current bittiming API. > > Maybe a common helper function to split up the values based on given > register sizes could simplify the handling for those CAN drivers. Good idea! What about adding the information about prop_seg and phase_seg1 to bittiming_const and let the can_calc_bittiming() calculate it? > I'm still not convinced that it brings some benefits for the user to extend > the bittiming API. IMHO it just complicates the bitrate settings. The benefit is, that the user knows about the limitation of prop_seg and phase_seg1. regards, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung Nürnberg | Phone: +49-5121-206917-129 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
Attachment:
signature.asc
Description: PGP signature
