Message-ID: <CABGWkvpgDZohEwPJu0hgm+OqfKbH=PgpPHjMMp=t3PxpPfVhVQ@mail.gmail.com>
Date:   Sun, 31 Jul 2022 17:54:01 +0200
From:   Dario Binacchi <dario.binacchi@...rulasolutions.com>
To:     Marc Kleine-Budde <mkl@...gutronix.de>
Cc:     Max Staudt <max@...as.org>, linux-kernel@...r.kernel.org,
        linux-can@...r.kernel.org,
        Oliver Hartkopp <socketcan@...tkopp.net>,
        michael@...rulasolutions.com,
        Amarula patchwork <linux-amarula@...rulasolutions.com>,
        Jeroen Hofstee <jhofstee@...tronenergy.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Wolfgang Grandegger <wg@...ndegger.com>, netdev@...r.kernel.org
Subject: Re: [RFC PATCH v3 8/9] can: slcan: add support to set bit time
 register (btr)

Hi Marc,

On Fri, Jul 29, 2022 at 9:33 AM Marc Kleine-Budde <mkl@...gutronix.de> wrote:
>
> On 29.07.2022 08:52:07, Dario Binacchi wrote:
> > Hello Marc and Max,
> >
> > On Thu, Jul 28, 2022 at 12:57 PM Max Staudt <max@...as.org> wrote:
> > >
> > > On Thu, 28 Jul 2022 12:50:49 +0200
> > > Marc Kleine-Budde <mkl@...gutronix.de> wrote:
> > >
> > > > On 28.07.2022 12:23:04, Dario Binacchi wrote:
> > > > > > > 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?
> > >
> > > Dario, there seems to be a misunderstanding about the Device Tree.
> > >
> > > It is used *only* for hardware that is permanently attached, present at
> > > boot, and forever after. Not for dyamically added stuff, and definitely
> > > not for ldiscs that have to be attached manually by the user.
> > >
> > >
> > > The only exception to this is if you have an embedded device with an
> > > slcan adapter permanently attached to one of its UARTs. Then you can
> > > use the serdev ldisc adapter to attach the ldisc automatically at boot.
> >
> > It is evident that I am lacking some skills (I will try to fix it :)).
>
> We're all here to learn something!
>
> > I think it is equally clear that it is not worth going down this path.
>
> If you have a static attached serial devices serdev is the way to go.
> But slcan has so many drawbacks compared to "real" CAN adapters that I
> hope the no one uses them in such a scenario.
>
> > > If you are actively developing for such a use case, please let us know,
> > > so we know what you're after and can help you better :)
> >
> > I don't have a use case, other than to try, if possible, to make the driver
> > autonomous from slcand / slcan_attach for the CAN bus setup.
>
> From my point of view your job is done!

Ok.

>
> > Returning to Marc's previous analysis:
> > "... Some USB CAN drivers query the bit timing const from the USB device."
> >
> > Can we think of taking the gs_usb driver as inspiration for getting/setting the
> > bit timings?
> >
> > https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/gs_usb.c#L951
> > https://elixir.bootlin.com/linux/latest/source/drivers/net/can/usb/gs_usb.c#L510
> >
> > and, as done with patches:
> >
> > can: slcan: extend the protocol with CAN state info
> > can: slcan: extend the protocol with error info
>
> You can define a way to query bit timing constants and CAN clock rate,
> but you have to get this into the "official" firmware. You have to roll
> out a firmware update to all devices. What about non official firmware?
>
> > further extend the protocol to get/set the bit timing from / to the adapter ?
> > In the case of non-standard bit rates, the driver would try, depending on the
> > firmware of the adapter, to calculate and set the bit timings autonomously.
>
> If an adapter follows 100% the official firmware doc the BTR registers
> are interpreted as SJA1000 with 8 MHz CAN clock.

I checked the sources and documentation of the usb adapter I used (i.
e. Https://www.fischl.de/usbtin/):
...
sxxyyzz[CR]                 Set can bitrate registers of MCP2515. You
can set non-standard baudrates which are not supported by the "Sx"
command.
                                     xx: CNF1 as hexadecimal value (00-FF)
                                     yy: CNF2 as hexadecimal value (00-FF)
                                     zz: CNF3 as hexadecimal value
...

Different from what is reported by can232_ver3_Manual.pdf :

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

And here is the type of control carried out by the slcan_attach for
the btr parameter:
https://github.com/linux-can/can-utils/blob/master/slcan_attach.c#L144
When I would have expected a different check (i. e. if (strlen(btr) > 4).
Therefore it is possible that each adapter uses these bytes in its own
way as well as
in the content and also in the number of bytes.

Thanks and regards,
Dario

>
> See
>
> | https://lore.kernel.org/all/20220728105049.43gbjuctezxzmm4j@pengutronix.de
>
> where I compare the 125 Kbit/s BTR config of the documentation with the
> bit timing calculated by the kernel algorithm.
>
> 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 |



-- 

Dario Binacchi

Embedded Linux Developer

dario.binacchi@...rulasolutions.com

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

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

www.amarulasolutions.com

Powered by blists - more mailing lists