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 :)
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.
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.
Best regards,
Oliver
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
