Re: [RFC PATCH 00/14] can: netlink: add CAN XL

[Oliver Hartkopp <socketcan@hartkopp.net>]

Hello Vincent!

Very impressive! Thanks for the effort.
Together with the example in the dummyxl driver it should become quite 
easy to integrate the netlink API into my XCANB driver hack for testing.

Picking up the dummyxl driver I wonder if it would make sense to 
mainline this driver probably as can_nltest driver?!?
Of course this driver should be disabled or combined with some kernel 
testing Kconfig stuff. But is it a great testing tool.

 From what I can see with the bitrate and tdc configurations the 
extension for CAN XL is ok.

If you take a look at this manual

https://github.com/linux-can/can-doc/blob/master/x_can/xcan_user_manual_v350.pdf

on page 268/304 you will find the PWM configuration which consists of 
three values with 6 bits each. I assume this to be similar in all CAN XL 
controllers.

The PWM feature switches the physical layer for a CAN XL transceiver in 
the CAN XL data phase. This is a weird feature to do some PWM on the 
controllers' TX data pin to be able to switch the physical layer while 
maintaining the CAN transceiver package with 8 pin layout.

Additionally to this PWM configuration register, the PWM CAN XL 
transceiver switch feature has to be enabled similar to the way we 
enable 'fd on' or 'xl on' today.

You can see this bit called XLTR in the Operating Mode section on page 
269/304 and 270/304 .

E.g. that might be named 'xltrx [on|off]' (default off)

Best regards,
Oliver


On 10.11.24 16:55, Vincent Mailhol wrote:
> Because of popular request [1] and to celebrate the fact that I joined
> the kernel web of trust this week [2], here is an RFC to introduce the
> CAN XL netlink support.
> 
> The logic is simple. The CAN XL basically just reuse the same kind of
> parameters as CAN FD: the databittiming and the TDC. So the series is
> just a bunch of refactor to either:
> 
>    - factorize code.
>    - rename some variable to differentiate between CAN FD and XL.
>    - make some function more generic to accept both CAN FD and XL.
> 
> The feature is working™: there is a dummy driver at the end of the
> series to show the traffic from the userland to a driver. This said, I
> did close to zero testing. Once I had one CAN XL frame reaching the
> driver, I call it a day, and decided to send the work. Regardless, it
> is Sunday night. If I do not send it now, that would be next week-end.
> Probably some mistakes are hidden here and there, but this should be
> enough for an RFC level.
> 
> Also, I am not fully happy that can_dbt_changelink() requires 8
> parameters. I will probably revisit this later on. But for the moment,
> I think this is acceptable for an RFC.
> 
> Overall, I do not want to rush this series. Linus should send the rc7
> anytime soon, and the merge window will probably start in eight days.
> I do not think this series will be finalized by then. I still need to
> give a deeper look at ISO 11898-1:2024 [3] to check that everything is
> good. However, if I receive positive feedback on this RFC, I would
> probably like to have the first patch merged so that I do not have to
> rebase that tree wide patch each time someone makes a change.
> 
> I will send a second RFC series for iplink2 just after this one. Stay
> tuned!
> 
> [1] https://lore.kernel.org/linux-can/CAEQ16vpxthctdrpv0kBKEZJA8VNYffjGGPBGBY93RmKDD49bAQ@mail.gmail.com/
> [2] https://git.kernel.org/pub/scm/docs/kernel/pgpkeys.git/commit/?id=81d335d3586c
> [3] https://www.iso.org/fr/standard/86384.html
> 
> Vincent Mailhol (14):
>    can: dev: add struct data_bittiming_params to group FD parameters
>    can: netlink: replace tabulation by space in assignement
>    can: bittiming: rename CAN_CTRLMODE_TDC_MASK into
>      CAN_CTRLMODE_FD_TDC_MASK
>    can: bittiming: rename can_tdc_is_enabled() into
>      can_fd_tdc_is_enabled()
>    can: netlink: can_changelink(): rename tdc_mask into
>      fd_tdc_flag_provided
>    can: netlink: make can_tdc_changelink() FD agnostic
>    can: netlink: add can_dtb_changelink()
>    can: netlink: add can_validate_tdc()
>    can: netlink: make can_tdc_get_size() FD agnostic
>    can: netlink: make can_tdc_fill_info() FD agnostic
>    can: netlink: document which symbols are FD specific
>    can: netlink: add CAN XL support
>    can: netlink: add userland error messages
>    !!! DO NOT MERGE !!! can: add dummyxl driver
> 
>   drivers/net/can/Kconfig                       |   8 +
>   drivers/net/can/Makefile                      |   1 +
>   drivers/net/can/ctucanfd/ctucanfd_base.c      |   8 +-
>   drivers/net/can/dev/calc_bittiming.c          |   2 +-
>   drivers/net/can/dev/dev.c                     |  12 +-
>   drivers/net/can/dev/netlink.c                 | 422 ++++++++++++------
>   drivers/net/can/dummyxl.c                     | 224 ++++++++++
>   drivers/net/can/flexcan/flexcan-core.c        |   4 +-
>   drivers/net/can/ifi_canfd/ifi_canfd.c         |  10 +-
>   drivers/net/can/kvaser_pciefd.c               |   6 +-
>   drivers/net/can/m_can/m_can.c                 |   8 +-
>   drivers/net/can/peak_canfd/peak_canfd.c       |   6 +-
>   drivers/net/can/rcar/rcar_canfd.c             |   4 +-
>   .../net/can/rockchip/rockchip_canfd-core.c    |   4 +-
>   .../can/rockchip/rockchip_canfd-timestamp.c   |   2 +-
>   .../net/can/spi/mcp251xfd/mcp251xfd-core.c    |   4 +-
>   drivers/net/can/usb/esd_usb.c                 |   6 +-
>   drivers/net/can/usb/etas_es58x/es58x_core.c   |   4 +-
>   drivers/net/can/usb/etas_es58x/es58x_fd.c     |   8 +-
>   drivers/net/can/usb/gs_usb.c                  |   8 +-
>   drivers/net/can/usb/kvaser_usb/kvaser_usb.h   |   2 +-
>   .../net/can/usb/kvaser_usb/kvaser_usb_core.c  |   6 +-
>   drivers/net/can/usb/peak_usb/pcan_usb_core.c  |   6 +-
>   drivers/net/can/xilinx_can.c                  |   6 +-
>   include/linux/can/bittiming.h                 |   4 +-
>   include/linux/can/dev.h                       |  39 +-
>   include/uapi/linux/can/netlink.h              |  21 +-
>   27 files changed, 606 insertions(+), 229 deletions(-)
>   create mode 100644 drivers/net/can/dummyxl.c
>