Re: [RFC PATCH 12/14] can: netlink: add CAN XL support

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 4237 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]