On 28.07.2022 12:23:04, Dario Binacchi wrote:

quoted

quoted

Does it make sense to use the device tree

The driver doesn't support DT and DT only works for static serial
interfaces.

Have you seen my remarks about Device Tree?

quoted

quoted

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?

The device tree usually says it's a CAN controller compatible to X and
the following clock(s) are connected. The driver for CAN controller X
knows the bit timing const. Some USB CAN drivers query the bit timing
const from the USB device.

quoted

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.

The driver knows how to map the "struct can_bittiming" to the BTR
register values of the hardware.

What does the serial protocol say to the BTR values? Are these standard
SJA1000 layout with 8 MHz CAN clock or are those adapter specific?

I think they are adapter specific.

The BTR values depend on the used CAN controller, the clock rate, and on
the firmware, if it supports BTR at all.

This is  what the can232_ver3_Manual.pdf reports:

sxxyy[CR]         Setup with BTR0/BTR1 CAN bit-rates where xx and yy is a hex
                         value. This command is only active if the CAN
channel is closed.

xx     BTR0 value in hex
yy     BTR1 value in hex

Example:            s031C[CR]
                           Setup CAN with BTR0=0x03 & BTR1=0x1C
                           which equals to 125Kbit.

But I think the example is misleading because IMHO it depends on the
adapter's controller (0x31C -> 125Kbit).

I think the BTR in can232_ver3_Manual.pdf is compatible to a sja1000
controller with 8 MHz ref clock. See:

| Bit timing parameters for sja1000 with 8.000000 MHz ref clock using algo 'v4.8'
|  nominal                                  real  Bitrt    nom   real  SampP
|  Bitrate TQ[ns] PrS PhS1 PhS2 SJW BRP  Bitrate  Error  SampP  SampP  Error   BTR0 BTR1
|  1000000    125   2    3    2   1   1  1000000   0.0%  75.0%  75.0%   0.0%   0x00 0x14
|   800000    125   3    4    2   1   1   800000   0.0%  80.0%  80.0%   0.0%   0x00 0x16
|   500000    125   6    7    2   1   1   500000   0.0%  87.5%  87.5%   0.0%   0x00 0x1c
|   250000    250   6    7    2   1   2   250000   0.0%  87.5%  87.5%   0.0%   0x01 0x1c
|   125000    500   6    7    2   1   4   125000   0.0%  87.5%  87.5%   0.0%   0x03 0x1c        <---
|   100000    625   6    7    2   1   5   100000   0.0%  87.5%  87.5%   0.0%   0x04 0x1c
|    50000   1250   6    7    2   1  10    50000   0.0%  87.5%  87.5%   0.0%   0x09 0x1c
|    20000   3125   6    7    2   1  25    20000   0.0%  87.5%  87.5%   0.0%   0x18 0x1c
|    10000   6250   6    7    2   1  50    10000   0.0%  87.5%  87.5%   0.0%   0x31 0x1c

quoted

quoted

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

The serial protocol defines fixed bit rates, no need to describe them in
the DT:

|           0            10 Kbit/s
|           1            20 Kbit/s
|           2            50 Kbit/s
|           3           100 Kbit/s
|           4           125 Kbit/s
|           5           250 Kbit/s
|           6           500 Kbit/s
|           7           800 Kbit/s
|           8          1000 Kbit/s

Are there more bit rates?

No, the manual can232_ver3_Manual.pdf does not contain any others.

What about defining a device tree node for the slcan (foo adapter):

slcan {
    compatible = "can,slcan";
                                     /* bit rate btr0btr1 */
    additional-bitrates = < 33333  0x0123
                                        80000  0x4567
                                        83333  0x89ab
                                      150000 0xcd10
                                      175000 0x2345
                                      200000 0x6789>
};

So that the can_priv::bitrate_cons array (dynamically created) will
contain the bitrates
           10000,
           20000,
           50000,
         100000,
         125000,
         250000,
         500000,
         800000,
        1000000 /* end of standards bitrates,  use S command */
           33333,  /* use s command, btr 0x0123 */
           80000,  /* use s command, btr 0x4567 */
           83333,  /* use s command, btr 0x89ab */
         150000,  /* use s command, btr 0xcd10 */
         175000, /* use s command, btr 0x2345 */
         200000  /* use s command, btr 0x6789 */
};

So if a standard bitrate is requested, the S command is used,
otherwise the s command with the associated btr.

That would be an option. For proper DT support, the driver needs to be
extended to support serdev. But DT only supports "static" devices.

But if you can calculate BTR values you know the bit timing constants
(and frequency) and then it's better to have the bit timing in the
driver so that arbitrary bit rates can be calculated by the kernel.

regards,
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 |