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 :) > 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. > As a both values rely on the same tq can't we just split the tseg1 into > prop_seg + phase_seg1 values with some common e.g. 70:30 pattern? We currently split 50:50 (hard-coded). > IMO changing the bittiming API has no value for the user just to satisfy the > "2 CAN-FD cores" that came late to the party. > > > > > A struct in netlink means we cannot change it. > > > > > > But are we not already in this situation with CAN FD that we can not change > > > the bittiming (const) in the userspace API? > > > > Yes, we have to support it. But we can add a new nested type that > > serializes the individual members of an improved struct bittiming_const. > > The old user space tools will just keep working, iproute2 can be updated > > to use the new bittiming_const if it's available and fall back to the > > existing one. > > Ok. Nice - but maybe obsolete due to my question above ;-) 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
