On 1/15/21 2:01 PM, Oliver Hartkopp wrote: > -DaveM > -JacubK > -netdev > > @Vincent: No need for cross posting and putting the networking > maintainers in CC for these really deep CAN driver specific topics IHMO > > On 15.01.21 08:26, Marc Kleine-Budde wrote: >> On 1/15/21 1:41 AM, Vincent MAILHOL wrote: >>> On Fri. 15 Jan 2021 at 02:23, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote: >>>> >>>> Hi Vincent, >>>> >>>> On 12.01.21 14:05, Vincent Mailhol wrote: >>>>> This driver supports the ES581.4, ES582.1 and ES584.1 interfaces from >>>>> ETAS GmbH (https://www.etas.com/en/products/es58x.php). >>>> >>>> (..) >>>> >>>>> diff --git a/drivers/net/can/usb/etas_es58x/es58x_fd.c b/drivers/net/can/usb/etas_es58x/es58x_fd.c >>>>> new file mode 100644 >>>>> index 000000000000..6b9534f23c96 >>>>> --- /dev/null >>>>> +++ b/drivers/net/can/usb/etas_es58x/es58x_fd.c >>>> >>>> (..) >>>> >>>>> +static void es58x_fd_print_bittiming(struct net_device *netdev, >>>>> + struct es58x_fd_bittiming >>>>> + *es58x_fd_bittiming, char *type) >>>>> +{ >>>>> + netdev_vdbg(netdev, "bitrate %s = %d\n", type, >>>>> + le32_to_cpu(es58x_fd_bittiming->bitrate)); >>>>> + netdev_vdbg(netdev, "tseg1 %s = %d\n", type, >>>>> + le16_to_cpu(es58x_fd_bittiming->tseg1)); >>>>> + netdev_vdbg(netdev, "tseg2 %s = %d\n", type, >>>>> + le16_to_cpu(es58x_fd_bittiming->tseg2)); >>>>> + netdev_vdbg(netdev, "brp %s = %d\n", type, >>>>> + le16_to_cpu(es58x_fd_bittiming->brp)); >>>>> + netdev_vdbg(netdev, "sjw %s = %d\n", type, >>>>> + le16_to_cpu(es58x_fd_bittiming->sjw)); >>>>> +} >>>> >>>> What is the reason for this code? >>>> >>>> These values can be retrieved with the 'ip' tool and are probably >>>> interesting for development - but not in the final code. >>> >>> First thing, I used netdev_vdbg() (verbose debug). That macro >>> will only produce code if VERBOSE_DEBUG is defined. Normal users >>> will not see those. So yes, this is mostly for development. >>> >>> Also, just realised that netdev_vdbg() is barely used >>> anywhere (only three files use it: >>> https://elixir.bootlin.com/linux/v5.11-rc3/C/ident/netdev_vdbg). >>> >>> I guess that I will remove it :) >>> > > Thanks! That will remove some more code in the background too. > >>>>> + >>>>> +static void es58x_fd_print_conf(struct net_device *netdev, >>>>> + struct es58x_fd_tx_conf_msg *tx_conf_msg) >>>>> +{ >>>>> + es58x_fd_print_bittiming(netdev, &tx_conf_msg->nominal_bittiming, >>>>> + "nominal"); >>>>> + netdev_vdbg(netdev, "samples_per_bit = %d\n", >>>>> + tx_conf_msg->samples_per_bit); >>>>> + netdev_vdbg(netdev, "sync_edge = %d\n", >>>>> + tx_conf_msg->sync_edge); >>>>> + netdev_vdbg(netdev, "physical_layer = %d\n", >>>>> + tx_conf_msg->physical_layer); >>>>> + netdev_vdbg(netdev, "self_reception = %d\n", >>>>> + tx_conf_msg->self_reception_mode); >>>>> + netdev_vdbg(netdev, "ctrlmode = %d\n", tx_conf_msg->ctrlmode); >>>>> + netdev_vdbg(netdev, "canfd_enabled = %d\n", >>>>> + tx_conf_msg->canfd_enabled); >>>>> + if (tx_conf_msg->canfd_enabled) { >>>>> + es58x_fd_print_bittiming(netdev, >>>>> + &tx_conf_msg->data_bittiming, "data"); >>>>> + netdev_vdbg(netdev, >>>>> + "Transmitter Delay Compensation = %d\n", >>>>> + tx_conf_msg->tdc); >>>>> + netdev_vdbg(netdev, >>>>> + "Transmitter Delay Compensation Offset = %d\n", >>>>> + le16_to_cpu(tx_conf_msg->tdco)); >>>>> + netdev_vdbg(netdev, >>>>> + "Transmitter Delay Compensation Filter = %d\n", >>>>> + le16_to_cpu(tx_conf_msg->tdcf)); >>>>> + } >>>>> +} >>>> >>>> Same here. >>>> >>>> Either the information can be retrieved with the 'ip' tool OR the are >>>> not necessary as set to some reasonable default anyway >>> >>> Ack, will remove. >>> >>>> OR we should >>>> implement the functionality in the general CAN driver infrastructure. >>> >>> Would make sense to me to add the tdco (Transmitter Delay >>> Compensation Offset). Ref: ISO 11898-1 section >>> 11.3.3 "Transmitter delay compensation" >>> >>> I would just like your opinion on one topic: the tdco is specific >>> to CAN FD. If we add it, we have two choices: >>> 1. put it in struct can_bittiming: that will mean that we will >>> have an unused field for classical CAN (field bittiming of >>> struct can_priv). >>> 2. put it in struct can_priv (but outside of struct >>> can_bittiming): no unused field but less pretty. >> >> 3. Deprecate struct can_bittiming as the user space interface >> and transfer each member individually via netlink. Extend >> the kernel-only can_bittiming by the tdc related >> parameters, and add these to the new netlink interface. > > I also saw the current netlink interface as the problem here. > > But even when 'deprecating' the old interface we still need to provide > the API, right? ACK > Would therefore the new parameters overwrite the content which is > transferred by can_bittiming or how would you like to make sure the > mixed content remains consistent? For my use case the API is extended and should not contain conflicting data. I think it's 3 steps to convert the kernel (where the 2.* and the 3.* have to be done atomically): 1. Introduce a new struct can_bittiming_kernel_const, with tseg1_{min,max} replaced by prop_seg_{min,max} and phase_seg1_{min,max}. 2.1. Convert drivers to use new struct can_bittiming_kernel_const. 2.2. Convert existing CAN-netlink interface to translate between old usersapce struct can_bittiming_const and new struct can_bittiming_kernel_const: new -> old: > old->phase_seg1_{min,max} = new->prop_seg_{min,max} + phase_seg1_{min,max} old -> new: > new->prop_seg_{min,max} = old->tseg1_{min,max} / 2; > new->phase_seg1_{min,max} = old->tseg1_{min,max} - new->prop_seg; 3.1. Add new netlink parsing that works with struct can_bittiming_kernel_const 3.2. Add netlink validity check, so that either old or new API is used in a single netlink call. >> I prefer this, as I want to extend the bittiming_const in this way, too. There >> are CAN controllers, where the bit timing calculation: >> >>> bt->prop_seg = tseg1 / 2; >>> bt->phase_seg1 = tseg1 - bt->prop_seg; >> >> doesn't work anymore, as they have asymmetric prog_seg and phase_seg1, so that >> splitting tseg1 in half doesn't work anymore. > > Interesting. The flexcan driver works around the problem this way: > /* CBT[EPSEG1] is 5 bit long and CBT[EPROPSEG] is 6 bit > * long. The can_calc_bittiming() tries to divide the tseg1 > * equally between phase_seg1 and prop_seg, which may not fit > * in CBT register. Therefore, if phase_seg1 is more than > * possible value, increase prop_seg and decrease phase_seg1. > */ > if (bt->phase_seg1 > 0x20) { > bt->prop_seg += (bt->phase_seg1 - 0x20); > bt->phase_seg1 = 0x20; > } 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 |
Attachment:
signature.asc
Description: OpenPGP digital signature
