Hello Marc,

On Wed, Jul 27, 2022 at 8:24 PM Marc Kleine-Budde [off-list ref] wrote:

On 27.07.2022 19:28:39, Max Staudt wrote:

quoted

On Wed, 27 Jul 2022 13:30:54 +0200
Marc Kleine-Budde [off-list ref] wrote:

quoted

As far as I understand, setting the btr is an alternative way to set the
bitrate, right? I don't like the idea of poking arbitrary values into a
hardware from user space.

I agree with Marc here.

This is a modification across the whole stack, specific to a single
device, when there are ways around.

If I understand correctly, the CAN232 "S" command sets one of the fixed
bitrates, whereas "s" sets the two BTR registers. Now the question is,
what do BTR0/BTR1 mean, and what are they? If they are merely a divider
in a CAN controller's master clock, like in ELM327, then you could

  a) Calculate the BTR values from the bitrate userspace requests, or

Most of the other CAN drivers write the BTR values into the register of
the hardware. How are these BTR values transported into the driver?

There are 2 ways:

1) - user space configures a bitrate
   - the kernel calculates with the "struct can_bittiming_const" [1] given
     by driver and the CAN clock rate the low level timing parameters.

     [1] https://elixir.bootlin.com/linux/v5.18/source/include/uapi/linux/can/netlink.h#L47

2) - user space configures low level bit timing parameter
     (Sample point in one-tenth of a percent, Time quanta (TQ) in
      nanoseconds, Propagation segment in TQs, Phase buffer segment 1 in
      TQs, Phase buffer segment 2 in TQs, Synchronisation jump width in
      TQs)
    - the kernel calculates the Bit-rate prescaler from the given TQ and
      CAN clock rate

Both ways result in a fully calculated "struct can_bittiming" [2]. The
driver translates this into the hardware specific BTR values and writes
the into the registers.

If you know the CAN clock and the bit timing const parameters of the
slcan's BTR register you can make use of the automatic BTR calculation,
too. Maybe the framework needs some tweaking if the driver supports both
fixed CAN bit rate _and_ "struct can_bittiming_const".

Does it make sense to use the device tree to provide the driver with those
parameters required for the automatic calculation of the BTR (clock rate,
struct can_bittiming_const, ...) that depend on the connected controller? In
this way the solution should be generic and therefore scalable. I think we
should also add some properties to map the calculated BTR value on the
physical register of the controller.

Or, use the device tree to extend the bittates supported by the controller
to the fixed ones (struct can_priv::bitrate_const)?

Thanks and regards,
Dario

[2] https://elixir.bootlin.com/linux/v5.18/source/include/uapi/linux/can/netlink.h#L31

quoted

  b) pre-calculate a huge table of possible bitrates and present them
     all to userspace. Sounds horrible, but that's what I did in can327,
     haha. Maybe I should have reigned them in a little, to the most
     useful values.

If your adapter only supports fixed values, then that's the only way to
go.

quoted

  c) just limit the bitrates to whatever seems most useful (like the
     "S" command's table), and let users complain if they really need
     something else. In the meantime, they are still free to slcand or
     minicom to their heart's content before attaching slcan, thanks to
     your backwards compatibility efforts.

In the early stages of the non-mainline CAN framework we had tables for
BTR values for some fixed bit rates, but that turned out to be not
scaleable.

quoted

In short, to me, this isn't a deal breaker for your patch series.

Marc

--
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

-- 

Dario Binacchi

Embedded Linux Developer

dario.binacchi@amarulasolutions.com

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

T. +39 042 243 5310
info@amarulasolutions.com

www.amarulasolutions.com