| Message ID | 20220726210217.3368497-9-dario.binacchi@amarulasolutions.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
On 26.07.2022 23:02:16, Dario Binacchi wrote: > It allows to set the bit time register with tunable values. > The setting can only be changed if the interface is down: > > ip link set dev can0 down > ethtool --set-tunable can0 can-btr 0x31c > ip link set dev can0 up 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. Do you have a use case for this? Marc
Hello Marc, On Wed, Jul 27, 2022 at 1:31 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote: > > On 26.07.2022 23:02:16, Dario Binacchi wrote: > > It allows to set the bit time register with tunable values. > > The setting can only be changed if the interface is down: > > > > ip link set dev can0 down > > ethtool --set-tunable can0 can-btr 0x31c > > ip link set dev can0 up > > As far as I understand, setting the btr is an alternative way to set the > bitrate, right? I thought of a non-standard bitrate or, in addition to the bitrate, the possibility of enabling some specific CAN controller options. Maybe Oliver could help us come up with the right answer. This is the the slcan source code: https://github.com/linux-can/can-utils/blob/cad1cecf1ca19277b5f5db39f8ef6f8ae426191d/slcand.c#L331 btr case cames after speed but they don't seem to be considered alternative. > I don't like the idea of poking arbitrary values into a > hardware from user space. However this is already possible through the slcand and slcan_attach applications. Furthermore, the driver implements the LAWICEL ASCII protocol for CAN frame transport over serial lines, and this is one of the supported commands. > > Do you have a use case for this? I use the applications slcand and slcan_attach as a reference, I try to make the driver independent from them for what concerns the CAN setup. And the bit time register setting is the last dependency. Thanks and regards, Dario > > 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 |
On 27.07.2022 17:55:10, Dario Binacchi wrote: > Hello Marc, > > On Wed, Jul 27, 2022 at 1:31 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote: > > > > On 26.07.2022 23:02:16, Dario Binacchi wrote: > > > It allows to set the bit time register with tunable values. > > > The setting can only be changed if the interface is down: > > > > > > ip link set dev can0 down > > > ethtool --set-tunable can0 can-btr 0x31c > > > ip link set dev can0 up > > > > As far as I understand, setting the btr is an alternative way to set the > > bitrate, right? > > I thought of a non-standard bitrate or, in addition to the bitrate, the > possibility of enabling some specific CAN controller options. Maybe Oliver > could help us come up with the right answer. > > This is the the slcan source code: > https://github.com/linux-can/can-utils/blob/cad1cecf1ca19277b5f5db39f8ef6f8ae426191d/slcand.c#L331 > btr case cames after speed but they don't seem to be considered alternative. > > > I don't like the idea of poking arbitrary values into a > > hardware from user space. > > However this is already possible through the slcand and slcan_attach > applications. > Furthermore, the driver implements the LAWICEL ASCII protocol for CAN > frame transport over serial lines, > and this is one of the supported commands. > > > > > Do you have a use case for this? > > I use the applications slcand and slcan_attach as a reference, I try to make the > driver independent from them for what concerns the CAN setup. And the bit time > register setting is the last dependency. Ok - We avoided writing bit timing registers from user space into the hardware for all existing drivers. If there isn't a specific use case, let's skip this patch. If someone comes up with a use case we can think of a proper solution. Marc
On Wed, 27 Jul 2022 13:30:54 +0200 Marc Kleine-Budde <mkl@pengutronix.de> wrote: > 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 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. 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 short, to me, this isn't a deal breaker for your patch series. Max
On Wed, Jul 27, 2022 at 7:21 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote: > > On 27.07.2022 17:55:10, Dario Binacchi wrote: > > Hello Marc, > > > > On Wed, Jul 27, 2022 at 1:31 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote: > > > > > > On 26.07.2022 23:02:16, Dario Binacchi wrote: > > > > It allows to set the bit time register with tunable values. > > > > The setting can only be changed if the interface is down: > > > > > > > > ip link set dev can0 down > > > > ethtool --set-tunable can0 can-btr 0x31c > > > > ip link set dev can0 up > > > > > > As far as I understand, setting the btr is an alternative way to set the > > > bitrate, right? > > > > I thought of a non-standard bitrate or, in addition to the bitrate, the > > possibility of enabling some specific CAN controller options. Maybe Oliver > > could help us come up with the right answer. > > > > This is the the slcan source code: > > https://github.com/linux-can/can-utils/blob/cad1cecf1ca19277b5f5db39f8ef6f8ae426191d/slcand.c#L331 > > btr case cames after speed but they don't seem to be considered alternative. > > > > > I don't like the idea of poking arbitrary values into a > > > hardware from user space. > > > > However this is already possible through the slcand and slcan_attach > > applications. > > Furthermore, the driver implements the LAWICEL ASCII protocol for CAN > > frame transport over serial lines, > > and this is one of the supported commands. > > > > > > > > Do you have a use case for this? > > > > I use the applications slcand and slcan_attach as a reference, I try to make the > > driver independent from them for what concerns the CAN setup. And the bit time > > register setting is the last dependency. > > Ok - We avoided writing bit timing registers from user space into the > hardware for all existing drivers. If there isn't a specific use case, > let's skip this patch. If someone comes up with a use case we can think > of a proper solution. Ok. So do I also remove the 7/9 "ethtool: add support to get/set CAN bit time register" patch ? Thanks and regards, Dario > > 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 |
On Wed, 27 Jul 2022 19:28:45 +0200 Dario Binacchi <dario.binacchi@amarulasolutions.com> wrote: > On Wed, Jul 27, 2022 at 7:21 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote: > > > > Ok - We avoided writing bit timing registers from user space into the > > hardware for all existing drivers. If there isn't a specific use case, > > let's skip this patch. If someone comes up with a use case we can think > > of a proper solution. > > Ok. So do I also remove the 7/9 "ethtool: add support to get/set CAN > bit time register" > patch ? If I may answer as well - IMHO, yes. Unless we know that BTR is something other than just a different way to express the bitrate, I'd skip it, yes. Because bitrate is already handled by other, cross-device mechanisms. Max
Hello Marc and Max, On Wed, Jul 27, 2022 at 7:33 PM Max Staudt <max@enpas.org> wrote: > > On Wed, 27 Jul 2022 19:28:45 +0200 > Dario Binacchi <dario.binacchi@amarulasolutions.com> wrote: > > > On Wed, Jul 27, 2022 at 7:21 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote: > > > > > > Ok - We avoided writing bit timing registers from user space into the > > > hardware for all existing drivers. If there isn't a specific use case, > > > let's skip this patch. If someone comes up with a use case we can think > > > of a proper solution. > > > > Ok. So do I also remove the 7/9 "ethtool: add support to get/set CAN > > bit time register" > > patch ? > > If I may answer as well - IMHO, yes. > > Unless we know that BTR is something other than just a different way to > express the bitrate, I'd skip it, yes. Because bitrate is already > handled by other, cross-device mechanisms. Thanks to both of you for the explanations. Regards, Dario > > > Max
On 27.07.2022 19:28:39, Max Staudt wrote: > On Wed, 27 Jul 2022 13:30:54 +0200 > Marc Kleine-Budde <mkl@pengutronix.de> wrote: > > > 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". [2] https://elixir.bootlin.com/linux/v5.18/source/include/uapi/linux/can/netlink.h#L31 > 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. > 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. > In short, to me, this isn't a deal breaker for your patch series. Marc
On 27.07.22 20:24, Marc Kleine-Budde wrote: > On 27.07.2022 19:28:39, Max Staudt wrote: >> On Wed, 27 Jul 2022 13:30:54 +0200 >> Marc Kleine-Budde <mkl@pengutronix.de> wrote: >> >>> 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". > > [2] https://elixir.bootlin.com/linux/v5.18/source/include/uapi/linux/can/netlink.h#L31 > >> 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. > >> 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. The Microchip CAN BUS Analyzer Tool is another example for fixed bitrates: https://elixir.bootlin.com/linux/v5.18.14/source/drivers/net/can/usb/mcba_usb.c#L156 So this might be the way to go here too ... Best regards, Oliver > >> In short, to me, this isn't a deal breaker for your patch series. > > Marc >
On 27.07.2022 22:12:13, Oliver Hartkopp wrote: > > > On 27.07.22 20:24, Marc Kleine-Budde wrote: > > On 27.07.2022 19:28:39, Max Staudt wrote: > > > On Wed, 27 Jul 2022 13:30:54 +0200 > > > Marc Kleine-Budde <mkl@pengutronix.de> wrote: > > > > > > > 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". > > > > [2] https://elixir.bootlin.com/linux/v5.18/source/include/uapi/linux/can/netlink.h#L31 > > > > > 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. > > > > > 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. > > The Microchip CAN BUS Analyzer Tool is another example for fixed bitrates: > > https://elixir.bootlin.com/linux/v5.18.14/source/drivers/net/can/usb/mcba_usb.c#L156 > > So this might be the way to go here too ... The slcan driver already supports fixed bitrates. This discussion is about setting the BTR registers in one way or another. Marc
Hello Marc, On Wed, Jul 27, 2022 at 8:24 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote: > > On 27.07.2022 19:28:39, Max Staudt wrote: > > On Wed, 27 Jul 2022 13:30:54 +0200 > > Marc Kleine-Budde <mkl@pengutronix.de> wrote: > > > > > 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 > > > 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. > > > 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. > > > 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 |
On 28.07.2022 09:36:21, Dario Binacchi wrote: > > 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 The driver doesn't support DT and DT only works for static serial interfaces. > 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. > 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? > 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? regards, Marc
On Thu, Jul 28, 2022 at 11:02 AM Marc Kleine-Budde <mkl@pengutronix.de> wrote: > > On 28.07.2022 09:36:21, Dario Binacchi wrote: > > > 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 > > The driver doesn't support DT and DT only works for static serial > interfaces. > > > 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. > > > 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. 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). > > > 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. Thanks and regards, Dario > > 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@amarulasolutions.com
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? > > > 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. > > > > > 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 > > > 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
On Thu, 28 Jul 2022 12:50:49 +0200 Marc Kleine-Budde <mkl@pengutronix.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. 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 :) Max
Hello Marc and Max, On Thu, Jul 28, 2022 at 12:57 PM Max Staudt <max@enpas.org> wrote: > > On Thu, 28 Jul 2022 12:50:49 +0200 > Marc Kleine-Budde <mkl@pengutronix.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 :)). I think it is equally clear that it is not worth going down this path. > > 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. 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 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. Thanks and regards, Dario > > Max
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@enpas.org> wrote: > > > > On Thu, 28 Jul 2022 12:50:49 +0200 > > Marc Kleine-Budde <mkl@pengutronix.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! > 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. 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
Hi Marc, On Fri, Jul 29, 2022 at 9:33 AM Marc Kleine-Budde <mkl@pengutronix.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@enpas.org> wrote: > > > > > > On Thu, 28 Jul 2022 12:50:49 +0200 > > > Marc Kleine-Budde <mkl@pengutronix.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 |
On 31.07.2022 17:54:01, Dario Binacchi wrote: > > 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. I hope the effective clock speed is documented somewhere, as you need this information to set the registers. > 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. I expected something like that. regards, Marc
diff --git a/drivers/net/can/slcan/slcan-core.c b/drivers/net/can/slcan/slcan-core.c index 45e521910236..3905f21e7788 100644 --- a/drivers/net/can/slcan/slcan-core.c +++ b/drivers/net/can/slcan/slcan-core.c @@ -99,6 +99,7 @@ struct slcan { #define CF_ERR_RST 0 /* Reset errors on open */ wait_queue_head_t xcmd_wait; /* Wait queue for commands */ /* transmission */ + u32 btr; /* bit timing register */ }; static const u32 slcan_bitrate_const[] = { @@ -128,6 +129,17 @@ int slcan_enable_err_rst_on_open(struct net_device *ndev, bool on) return 0; } +int slcan_set_btr(struct net_device *ndev, u32 btr) +{ + struct slcan *sl = netdev_priv(ndev); + + if (netif_running(ndev)) + return -EBUSY; + + sl->btr = btr; + return 0; +} + /************************************************************************* * SLCAN ENCAPSULATION FORMAT * *************************************************************************/ @@ -699,22 +711,40 @@ static int slcan_netdev_open(struct net_device *dev) return err; } - if (sl->can.bittiming.bitrate != CAN_BITRATE_UNKNOWN) { - for (s = 0; s < ARRAY_SIZE(slcan_bitrate_const); s++) { - if (sl->can.bittiming.bitrate == slcan_bitrate_const[s]) - break; + if (sl->can.bittiming.bitrate != CAN_BITRATE_UNKNOWN || sl->btr) { + if (sl->can.bittiming.bitrate != CAN_BITRATE_UNKNOWN) { + for (s = 0; s < ARRAY_SIZE(slcan_bitrate_const); s++) { + if (sl->can.bittiming.bitrate == + slcan_bitrate_const[s]) + break; + } + + /* The CAN framework has already validate the bitrate + * value, so we can avoid to check if `s' has been + * properly set. + */ + snprintf(cmd, sizeof(cmd), "C\rS%d\r", s); + err = slcan_transmit_cmd(sl, cmd); + if (err) { + netdev_err(dev, + "failed to send bitrate command 'C\\rS%d\\r'\n", + s); + goto cmd_transmit_failed; + } } - /* The CAN framework has already validate the bitrate value, - * so we can avoid to check if `s' has been properly set. - */ - snprintf(cmd, sizeof(cmd), "C\rS%d\r", s); - err = slcan_transmit_cmd(sl, cmd); - if (err) { - netdev_err(dev, - "failed to send bitrate command 'C\\rS%d\\r'\n", - s); - goto cmd_transmit_failed; + if (sl->btr) { + u32 btr = sl->btr & GENMASK(15, 0); + + snprintf(cmd, sizeof(cmd), "C\rs%04x\r", btr); + err = slcan_transmit_cmd(sl, cmd); + if (err) { + netdev_err(dev, + "failed to send bit timing command 'C\\rs%04x\\r'\n", + btr); + goto cmd_transmit_failed; + } + } if (test_bit(CF_ERR_RST, &sl->cmd_flags)) { diff --git a/drivers/net/can/slcan/slcan-ethtool.c b/drivers/net/can/slcan/slcan-ethtool.c index bf0afdc4e49d..8e2e77bbffda 100644 --- a/drivers/net/can/slcan/slcan-ethtool.c +++ b/drivers/net/can/slcan/slcan-ethtool.c @@ -52,11 +52,24 @@ static int slcan_get_sset_count(struct net_device *netdev, int sset) } } +static int slcan_set_tunable(struct net_device *netdev, + const struct ethtool_tunable *tuna, + const void *data) +{ + switch (tuna->id) { + case ETHTOOL_CAN_BTR: + return slcan_set_btr(netdev, *(u32 *)data); + default: + return -EINVAL; + } +} + static const struct ethtool_ops slcan_ethtool_ops = { .get_strings = slcan_get_strings, .get_priv_flags = slcan_get_priv_flags, .set_priv_flags = slcan_set_priv_flags, .get_sset_count = slcan_get_sset_count, + .set_tunable = slcan_set_tunable, }; void slcan_set_ethtool_ops(struct net_device *netdev) diff --git a/drivers/net/can/slcan/slcan.h b/drivers/net/can/slcan/slcan.h index d463c8d99e22..1ac412fe8c95 100644 --- a/drivers/net/can/slcan/slcan.h +++ b/drivers/net/can/slcan/slcan.h @@ -13,6 +13,7 @@ bool slcan_err_rst_on_open(struct net_device *ndev); int slcan_enable_err_rst_on_open(struct net_device *ndev, bool on); +int slcan_set_btr(struct net_device *ndev, u32 btr); void slcan_set_ethtool_ops(struct net_device *ndev); #endif /* _SLCAN_H */
It allows to set the bit time register with tunable values. The setting can only be changed if the interface is down: ip link set dev can0 down ethtool --set-tunable can0 can-btr 0x31c ip link set dev can0 up Suggested-by: Marc Kleine-Budde <mkl@pengutronix.de> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com> --- (no changes since v1) drivers/net/can/slcan/slcan-core.c | 58 ++++++++++++++++++++------- drivers/net/can/slcan/slcan-ethtool.c | 13 ++++++ drivers/net/can/slcan/slcan.h | 1 + 3 files changed, 58 insertions(+), 14 deletions(-)