drivers/can/sja1000: Add SJA1000 CAN driver support

[radek-pesina]

Summary

This driver is based on ESP32 TWAI CAN drivers currently available in Nuttx, and captures the differences currently present across the TWAI drivers for easy future adaption to remaining ESP32 platforms with no loss of support/function. Also provides a generic SJA1000 CAN driver solution that is CPU-architecture independent.

Impact

• Low-level driver re-written to allow usage independent of CPU architecture, and support both SJA1000 and TWAI CAN controllers.
• Platform-specific settings abstracted away to be provided by board layer.
• Support for multiple instances of SJA1000 driver.

Testing

Tested on ESP32C3 (additional PR to follow with integration for ESP32C3).

[ppisa]

In general, I am happy that driver will be moved to the common code area and can be used with all ESP32 chips and even SJA1000 chips from single source which allows to keep it update and reuse profit of each enhancement for all SJA compatible/based controllers integrated int broad range of chips. I expect that you use code with our old SJA1000 FD tolerant design https://gitlab.fel.cvut.cz/canbus/zynq/sja1000-fdtol . I have no time to look at it for years but if you have some enhancement, fix of problems available, I will be happy to integrate it when I have some more time.

I expect that you will use driver with some FPGA based design. In such case I can help you with basic functional SJA1000 emulation in QEMU which allows to develop driver core without need to debug on hardware (bittiming and lot more has to be developed on real HW anyway). Our SJA1000 emulation is included in QEMU for years https://www.qemu.org/docs/master/system/devices/can.html but mapping is only for PCI/PCIe devices. But during our CTU CAN FD driver development for RTEMS, I have found way how to implement QDEV mapping the core into Zynq FPGA address space by QEMU commandline arguments. Some part of the code is still a hack and I need to discuss with people more knowledgeable about QEMU internals how to resolve it better, but it works for us with RTEMS and Linux kernel for testing. The branch and mapping file are there https://github.com/ppisa/qemu/blob/net-can-ctucanfd-platform/hw/net/can/ctucan_mm.c

I expect that adding such mapping to system bus for our SJA1000 emulation on Zynq and other SoC targets would be simple task. I can try that. It could be extended to TWAI emulation on ESP32C3, C6, etc... if somebody from Espressif is interested. I can try to find some CTU student to do that as summer job for some small funding.

We have reached quite huge progress with our RTEMS CTU CAN FD drivers and complete new CAN FD framework and we are ahead of schedule. In some of my past exchange with MoTeC, I have stated that we should have driver ready by May and it can be reused on NuttX. We have some minor things to resolve still, but base code is performing excellent including link priority inversion between priority classes prevention. We will have article at iCC about that https://www.can-cia.org/icc/ . The RTEMS CTU CAN FD driver core is there https://gitlab.fel.cvut.cz/otrees/rtems/rtems-canfd/-/blob/master/lib/candrv/ctucanfd/ctucanfd.c . As result of review process, we will change queues locking to RTEMS core maintainers suggestion etc. But I think that porting to NuttX can start soon when somebody finds time for that. I can get to it myself, but probably not earlier than in summer. @michallenc (actual main developer on RTEMS side) has lot of tasks as well so he probably cannot find time earlier either.

Edit: CTU CAN FD RTEMS driver is permissive in wide range with porting to NuttX in mind. But NuttX (or better Apache Software Foundation) officials newer finally replied to the multilicense SPDX discussion how to make our live to support multiple RTOSes easier.

[ppisa]

As for the TWAI code changes, I ma not sure if to change identifier of the type struct can_bittiming_const to to struct sja1000_can_bittiming_const. My long term goal is to reach agreement that these types will be defined in some header file like include/nuttx/can/can-bittiming.h. It should be usable for all kinds of controllers, on Linux it works for all. For CAN FD there are two set of exactly these parameters provided.

as for canioc_bittiming_s, I would prefer there some option how to provide bitrate and sapmling point percentage in general way from from application to the driver. Computation algorithm should be general. It can be used from drivers or it can be used in userspace and can_bittiming_const wold be read from low level chip driver by some IOCTL.

[acassis]

As for the TWAI code changes, I ma not sure if to change identifier of the type struct can_bittiming_const to to struct sja1000_can_bittiming_const. My long term goal is to reach agreement that these types will be defined in some header file like include/nuttx/can/can-bittiming.h. It should be usable for all kinds of controllers, on Linux it works for all. For CAN FD there are two set of exactly these parameters provided.

as for canioc_bittiming_s, I would prefer there some option how to provide bitrate and sapmling point percentage in general way from from application to the driver. Computation algorithm should be general. It can be used from drivers or it can be used in userspace and can_bittiming_const wold be read from low level chip driver by some IOCTL.

[acassis]

Sorry, pressed the incorrect button, I was willing to cancel my comment. These github buttons are prone to lead us to make mistakes.

[AndrewD]

In general, I am happy that driver will be moved to the common code area and can be used with all ESP32 chips and even SJA1000 chips from single source which allows to keep it update and reuse profit of each enhancement for all SJA compatible/based controllers integrated int broad range of chips. I expect that you use code with our old SJA1000 FD tolerant design https://gitlab.fel.cvut.cz/canbus/zynq/sja1000-fdtol . I have no time to look at it for years but if you have some enhancement, fix of problems available, I will be happy to integrate it when I have some more time.

Correct - we decided to experiment with sja1000-fdtol in the short term as there was an existing nuttx driver. This started while we were waiting for the licencing discussions to resolve.

I expect that you will use driver with some FPGA based design. In such case I can help you with basic functional SJA1000 emulation in QEMU which allows to develop driver core without need to debug on hardware (bittiming and lot more has to be developed on real HW anyway). Our SJA1000 emulation is included in QEMU for years https://www.qemu.org/docs/master/system/devices/can.html but mapping is only for PCI/PCIe devices. But during our CTU CAN FD driver development for RTEMS, I have found way how to implement QDEV mapping the core into Zynq FPGA address space by QEMU commandline arguments. Some part of the code is still a hack and I need to discuss with people more knowledgeable about QEMU internals how to resolve it better, but it works for us with RTEMS and Linux kernel for testing. The branch and mapping file are there https://github.com/ppisa/qemu/blob/net-can-ctucanfd-platform/hw/net/can/ctucan_mm.c

This is great news - I looked at the qemu support for sja1000 and ctucanfd but after determining it was PCI-only and seemed to required qemu changes to map onto a standard bus we decided to just focus on hardware based testing.

I expect that adding such mapping to system bus for our SJA1000 emulation on Zynq and other SoC targets would be simple task. I can try that. It could be extended to TWAI emulation on ESP32C3, C6, etc... if somebody from Espressif is interested. I can try to find some CTU student to do that as summer job for some small funding.

We have reached quite huge progress with our RTEMS CTU CAN FD drivers and complete new CAN FD framework and we are ahead of schedule. In some of my past exchange with MoTeC, I have stated that we should have driver ready by May and it can be reused on NuttX. We have some minor things to resolve still, but base code is performing excellent including link priority inversion between priority classes prevention. We will have article at iCC about that https://www.can-cia.org/icc/ . The RTEMS CTU CAN FD driver core is there https://gitlab.fel.cvut.cz/otrees/rtems/rtems-canfd/-/blob/master/lib/candrv/ctucanfd/ctucanfd.c . As result of review process, we will change queues locking to RTEMS core maintainers suggestion etc. But I think that porting to NuttX can start soon when somebody finds time for that. I can get to it myself, but probably not earlier than in summer. @michallenc (actual main developer on RTEMS side) has lot of tasks as well so he probably cannot find time earlier either.

This is good news - I had been wondering about the progress on this. One minor detail I noticed: ctucanfd_hw.h is still licenced: /* SPDX-License-Identifier: GPL-2.0-or-later */

We can investigate porting this to Nuttx fairly soon after we complete some other Nuttx tasks we intend to contribute.

Edit: CTU CAN FD RTEMS driver is permissive in wide range with porting to NuttX in mind. But NuttX (or better Apache Software Foundation) officials newer finally replied to the multilicense SPDX discussion how to make our live to support multiple RTOSes easier.

This was finally resolved by a reply from apache legal - the licence you proposed was acceptable.

[AndrewD]

As for the TWAI code changes, I ma not sure if to change identifier of the type struct can_bittiming_const to to struct sja1000_can_bittiming_const.

This prefix was added after an internal review - only because our understanding is it was the required nuttx coding style.

My long term goal is to reach agreement that these types will be defined in some header file like include/nuttx/can/can-bittiming.h. It should be usable for all kinds of controllers, on Linux it works for all. For CAN FD there are two set of exactly these parameters provided.

We agree that there is a lot of duplication in Nuttx that could (should?) be refactored. For example there are many copies of the sja1000 (aka TWAI) driver with minimal differences - we decided to start by unifying / generalising a single driver we wanted to use, to determine if this approach was acceptable to the Nuttx community, before going further.

as for canioc_bittiming_s, I would prefer there some option how to provide bitrate and sapmling point percentage in general way from from application to the driver. Computation algorithm should be general. It can be used from drivers or it can be used in userspace and can_bittiming_const wold be read from low level chip driver by some IOCTL.

We have some further work that implements IOCTL for features like this, we just wanted to get the basic driver cleanup reviewed first, with minimal changes from the existing driver in ESP32C3 to make review easier. A subsequent PR will demonstrate the integration with ESP32C3 then we will publish the IOCTL additions.

[ppisa]

I expect that you use code with our old SJA1000 FD tolerant design https://gitlab.fel.cvut.cz/canbus/zynq/sja1000-fdtol . I have no time to look at it for years but if you have some enhancement, fix of problems available, I will be happy to integrate it when I have some more time.

Correct - we decided to experiment with sja1000-fdtol in the short term as there was an existing nuttx driver. This started while we were waiting for the licencing discussions to resolve.

OK, it will be good to know how it performs. The all CAN patents (as I know) have expired so this solution should be without any licensing fees. But it is limited to CAN and have problem that it could not detect bit-flip in the FDF bit, because its CAN FD frame indicating value switches controller into mode where it ignores all data until interframe iddle...

I expect that you will use driver with some FPGA based design. In such case I can help you with basic functional SJA1000 emulation in QEMU which allows to develop driver core without need to debug on hardware (bittiming and lot more has to be developed on real HW anyway). Our SJA1000 emulation is included in QEMU for years https://www.qemu.org/docs/master/system/devices/can.html but mapping is only for PCI/PCIe devices. But during our CTU CAN FD driver development for RTEMS, I have found way how to implement QDEV mapping the core into Zynq FPGA address space by QEMU commandline arguments. Some part of the code is still a hack and I need to discuss with people more knowledgeable about QEMU internals how to resolve it better, but it works for us with RTEMS and Linux kernel for testing. The branch and mapping file are there https://github.com/ppisa/qemu/blob/net-can-ctucanfd-platform/hw/net/can/ctucan_mm.c

This is great news - I looked at the qemu support for sja1000 and ctucanfd but after determining it was PCI-only and seemed to required qemu changes to map onto a standard bus we decided to just focus on hardware based testing.

I plan discuss with QEMU people how is it intended in the core and we can get it into mainline, but I probably do not get to more than dirty quick hacks till summer, I am overloaded by preparation and teaching of computer architectures (with RISC-V) and prepare remotely course at Sweden LTU as well..

The RTEMS CTU CAN FD driver core is there https://gitlab.fel.cvut.cz/otrees/rtems/rtems-canfd/-/blob/master/lib/candrv/ctucanfd/ctucanfd.c . As result of review process, we will change queues locking to RTEMS core maintainers suggestion etc. But I think that porting to NuttX can start soon when somebody finds time for that. I can get to it myself, but probably not earlier than in summer. @michallenc (actual main developer on RTEMS side) has lot of tasks as well so he probably cannot find time earlier either.

This is good news - I had been wondering about the progress on this. One minor detail I noticed: ctucanfd_hw.h is still licenced: /* SPDX-License-Identifier: GPL-2.0-or-later */

This is generated file the mostly and there has been agreement with Mr. Ille and Mr. Jerabek on switch to permissive license. In the fact, it is in strong advantage for Mr. Ille because he has decided to be only one to have right to license HDL part and re-licensing to permissive license my years of work on driver and corresponding architectural design and correction of whole IP core design, I lose control about his future development. But I do not want to block progress so I lose profit and my other plans.

We can investigate porting this to Nuttx fairly soon after we complete some other Nuttx tasks we intend to contribute.

Edit: CTU CAN FD RTEMS driver is permissive in wide range with porting to NuttX in mind. But NuttX (or better Apache Software Foundation) officials newer finally replied to the multilicense SPDX discussion how to make our live to support multiple RTOSes easier.

This was finally resolved by a reply from apache legal - the licence you proposed was acceptable.

But there should be provided and archived public reply in the NuttX devel list to clarify and archive the answer that the arrangement is according to the rules. Or signed paper should be scanned and PDF sent if the lawyers do not know how to respond and coordinate cooperation in open source project through mailing list.

[ppisa]

As for the TWAI code changes, I ma not sure if to change identifier of the type struct can_bittiming_const to to struct sja1000_can_bittiming_const.

This prefix was added after an internal review - only because our understanding is it was the required nuttx coding style.

Yes, agreement from NuttX community for pushing the structures as generic for whole CAN subsystem is necessary. You can leave prefix there. But add TODO comment that it should became or be aligned with generic solution.

My long term goal is to reach agreement that these types will be defined in some header file like include/nuttx/can/can-bittiming.h. It should be usable for all kinds of controllers, on Linux it works for all. For CAN FD there are two set of exactly these parameters provided.

We agree that there is a lot of duplication in Nuttx that could (should?) be refactored. For example there are many copies of the sja1000 (aka TWAI) driver with minimal differences - we decided to start by unifying / generalising a single driver we wanted to use, to determine if this approach was acceptable to the Nuttx community, before going further.

as for canioc_bittiming_s, I would prefer there some option how to provide bitrate and sapmling point percentage in general way from from application to the driver. Computation algorithm should be general. It can be used from drivers or it can be used in userspace and can_bittiming_const wold be read from low level chip driver by some IOCTL.

We have some further work that implements IOCTL for features like this, we just wanted to get the basic driver cleanup reviewed first, with minimal changes from the existing driver in ESP32C3 to make review easier. A subsequent PR will demonstrate the integration with ESP32C3 then we will publish the IOCTL additions.

Great, you can look at SoketCAN, LinCAN and other implementations to find some reasonable set of IOCTLs and again they should be discussed and became generic for NuttX CAN. You can look at our proposal of CAN FD subsystem for RTEMS, but we have not solved there bittiming IOCTLs yet There are ones to maintain mechanism to setup and manage priority classes/queues, wait for all data sent and then to provide equivalent of poll/select which POSIX version is limited to sockets only on RTEMS

https://gitlab.fel.cvut.cz/otrees/rtems/rtems-canfd/-/blob/master/lib/candrv/dev/can/can.h?ref_type=heads#L62

[tmedicci]

Hi!

I checked this code and, as far as I was able to figure out, it uses a generic interface (TWAI peripheral for ESP32-C3, for instance) to interact with the SJA1000. Is it correct? (I'm analyzing it along with #11913 ) and I'll concentrate the comments here.

I see potential problems: usually, when we have some device (SJA1000) that is connected to a peripheral, we have two drivers: a low-level driver of the peripheral which is attached to the device driver. This is not done at the board-level (as in #11913 ), but in the arch itself. The board-level is responsible for initializing both the peripheral, getting an instance of it and initializing the device. Also, I saw that SJA1000 is selected by default for ESP32-C3. This should be selectable (a user's choice). See: the board itself does not contain a SJA1000, but it does contain a TWAI peripheral.

As an inspiration, you can check RMT peripheral and WS2812 (RMT-based) implementation in boards/risc-v/esp32c3/common/src/esp_board_rmt.c:

  rmt = esp_rmt_tx_init(ch, pin);
  if (rmt == NULL)
    {
      rmterr("ERROR: esp_rmt_tx_init failed\n");
      return -ENODEV;
    }

  ret = rmtchar_register(rmt);
  if (ret < 0)
    {
      rmterr("ERROR: rmtchar_register failed: %d\n", ret);
      return ret;
    }

#ifdef CONFIG_WS2812_NON_SPI_DRIVER
  led = esp_ws2812_setup("/dev/leds0", rmt, CONFIG_WS2812_LED_COUNT, false);
  if (led == NULL)
    {
      rmterr("ERROR: esp_ws2812_setup failed\n");
      return -ENODEV;
    }
#endif

It:

1. Initializes a RMT TX instance;
2. Registers the character device (this is optional. This interface interacts directly with the RMT peripheral);
3. Initialize an esp_ws2812 instance using the RMT instance as low-level implementation;

Note that esp_ws2812_setup itself is a wrapper is defined in nuttx/arch/risc-v/src/common/espressif/esp_ws2812.c and calls the generic WS2818 driver (ws2812_register).

[tmedicci]

Please check #11868 (comment)

[radek-pesina]

Hi!

I checked this code and, as far as I was able to figure out, it uses a generic interface (TWAI peripheral for ESP32-C3, for instance) to interact with the SJA1000. Is it correct? (I'm analyzing it along with #11913 ) and I'll concentrate the comments here.

I see potential problems: usually, when we have some device (SJA1000) that is connected to a peripheral, we have two drivers: a low-level driver of the peripheral which is attached to the device driver. This is not done at the board-level (as in #11913 ), but in the arch itself. The board-level is responsible for initializing both the peripheral, getting an instance of it and initializing the device. Also, I saw that SJA1000 is selected by default for ESP32-C3. This should be selectable (a user's choice). See: the board itself does not contain a SJA1000, but it does contain a TWAI peripheral.

As an inspiration, you can check RMT peripheral and WS2812 (RMT-based) implementation in boards/risc-v/esp32c3/common/src/esp_board_rmt.c:

  rmt = esp_rmt_tx_init(ch, pin);
  if (rmt == NULL)
    {
      rmterr("ERROR: esp_rmt_tx_init failed\n");
      return -ENODEV;
    }

  ret = rmtchar_register(rmt);
  if (ret < 0)
    {
      rmterr("ERROR: rmtchar_register failed: %d\n", ret);
      return ret;
    }

#ifdef CONFIG_WS2812_NON_SPI_DRIVER
  led = esp_ws2812_setup("/dev/leds0", rmt, CONFIG_WS2812_LED_COUNT, false);
  if (led == NULL)
    {
      rmterr("ERROR: esp_ws2812_setup failed\n");
      return -ENODEV;
    }
#endif

It:

1. Initializes a RMT TX instance;
2. Registers the character device (this is optional. This interface interacts directly with the RMT peripheral);
3. Initialize an esp_ws2812 instance using the RMT instance as low-level implementation;

Note that esp_ws2812_setup itself is a wrapper is defined in nuttx/arch/risc-v/src/common/espressif/esp_ws2812.c and calls the generic WS2818 driver (ws2812_register).

Hi,

The TWAI is a ESP32 implementation of the SJA1000 controller - I think they renamed it to TWAI for legal purposes when implementing it within the ESP32? Anyway it's a fully-compatible SJA1000 controller, just with a different name.

[tmedicci]

Hi,

The TWAI is a ESP32 implementation of the SJA1000 controller - I think they renamed it to TWAI for legal purposes when implementing it within the ESP32? Anyway it's a fully-compatible SJA1000 controller, just with a different name.

Oh, I see. Thank you for making it clear. In this case, I think we can have the same approach: a generic driver (the SJA1000) which expects a "lower-half" reference of the ESP32's TWAI peripheral (in the arch-level). Please let me know if you have figured this out or if you need more information on how to do that...

[radek-pesina]

Accidentally closed the pull request yesterday (wrong button). Re-opened.

[radek-pesina]

Hi,
The TWAI is a ESP32 implementation of the SJA1000 controller - I think they renamed it to TWAI for legal purposes when implementing it within the ESP32? Anyway it's a fully-compatible SJA1000 controller, just with a different name.

Oh, I see. Thank you for making it clear. In this case, I think we can have the same approach: a generic driver (the SJA1000) which expects a "lower-half" reference of the ESP32's TWAI peripheral (in the arch-level). Please let me know if you have figured this out or if you need more information on how to do that...

Hi.

Hi, apologies, I'm not sure I fully follow - the SJA1000 is the lower-half driver, implementing the SJA1000 (or in this instance you can say TWAI) peripheral. The board level esp32c3_board_twai.c is initialising the driver and registering it with the upper-half driver, and providing the necessary board-specific implementations (such as GPIO setup, irq handling etc). I'm having difficulty picturing what changes are being suggested so if you can describe it further then that'd be appreciated. Is the thought to implement the board-specific changes as a primary lower-half driver which calls the SJA1000 lower-half driver? I haven't observed this pattern (two layers of lower-half drivers) in Nuttx so if you can provide further insights that'd be appreciated.

[ppisa]

@radek-pesina, @tmedicci and others, it would be great if the core part of the driver can sit in common location. In the fact, we have interest in ESP32C6 now and there is that change in ESP32C3 to legacy so driver in the common place makes lot of sense. But there is that move to esp-hal-3rdparty and I am not sure how well that all will play together.

I would prefer to have drivers as part of mainline because if somebody does changes then he/she can do them in global way. When we have tried to switch from esp32c3-legacy to non-legacy (or in the fact keep esp32c3 from older project) we have been out of all peripherals. We are trying to move forward ICE-V (ESP32C3+iCE-40) when original one has stuck on ancient ESP IDF version and it seems that we will be force to switch to some hybrid again. So in the fact I would like to discuss future of ESP32 support. Is the code in esp-hal-3rdparty repository compatible with Appache license and can be common Apache licensed merged self-contained NuttX repository be create and released? We planned to use ESP32C6 as the platform for education in prestigious study program on the partner university but I am getting to fear that not only WiFi but more ESP32 support is no go anymore when strict open educational environment is targeted.

[ppisa]

@tmedicci: In the fact as I am trying to find from which location our TWAI work should be used with non-legacy setup, I cannot find it. It seems to me that our whole work is lost.... It seems to be in nuttx/arch/xtensa/src/esp32s3, nuttx/arch/xtensa/src/esp32s2, nuttx/arch/xtensa/src/esp32 and nuttx/arch/risc-v/src/esp32c3-legacy. Maintaining for copies in sync would be nightmare and it seems to be unusable for current ESP32C3 and ESP32C6 work anyway. And I expect that code in esp-hal-3rdparty does not pass through NuttX review process so it is again bad from long term perspective.

[ppisa]

Hi, apologies, I'm not sure I fully follow - the SJA1000 is the lower-half driver, implementing the SJA1000 (or in this instance you can say TWAI) peripheral. The board level esp32c3_board_twai.c is initialising the driver and registering it with the upper-half driver, and providing the necessary board-specific implementations (such as GPIO setup, irq handling etc). I'm having difficulty picturing what changes are being suggested so if you can describe it further then that'd be appreciated. Is the thought to implement the board-specific changes as a primary lower-half driver which calls the SJA1000 lower-half driver? I haven't observed this pattern (two layers of lower-half drivers) in Nuttx so if you can provide further insights that'd be appreciated.

I left aside specific Espressif use of our original code for their ESP32C3 now, I see it problematic when I see how external code is mixed with mainline NuttX and I would like to have more insight into it, because we thought that we could use ESP32Cx for our projects and I am getting unrest now.

I am replying from the long term CAN drivers, emulations maintainer/developer. I value your attempt to move code to common place because it fits well to the same location as nuttx/drivers/can/mcp2515.c. It is great that you have tested it with OpenCores SJA1000, because it works well with GNU/Linux historic original SJA1000 drivers and other systems, we have QEMU support for it which is used by Zephyr people, so it is THE reference. In the fact, I remember times when SJA1000 has been a chip and we have connected it to PC ISA bus or local even emulated buses on ARM and original LinCAN driver has been usable for all these targets.

So I support your work to introduce clean common SJA1000 IP/chip model support.

Kudos for the switching and rewritting code to plain offset usable even for PC ISA implementation

#define SJA1000_MODE_REG         (0x00)
#define SJA1000_CMD_REG          (0x01)

same for

  CODE uint32_t (*getreg)(struct sja1000_dev_s *sja_priv, uint32_t addr);
  CODE void (*putreg)(struct sja1000_dev_s *sja_priv, uint32_t addr,
                      uint32_t value);
  CODE void (*modifyreg32)(struct sja1000_dev_s *sja_priv, uint32_t addr,
                           uint32_t clearbits, uint32_t setbits);
};

You have moved the base address to the main structure representing the device struct sja1000_dev_s which makes implementation of registers access functions clean

static inline void sja1000_modifyreg32(struct sja1000_dev_s *sja_priv,
    uint32_t addr, uint32_t clearbits, uint32_t setbits)
{
  sja_priv->modifyreg32(sja_priv, addr, clearbits, setbits);
}

and allows to implement chip specific registers access functions simpler accessing only base form priv, resolving span 8, 16, 32 bits etc... I have only suggestion to not use addr name the function prototype now. It should be reg or offs or something which describes that it is not the full address.

I like non sja1000 specific struct can_bittiming_const_s and would be happy if it can be move out of SJA1000 header file when/if there is agreement on common bitrate calculations later. But may be that driver should define struct sja1000_can_bittiming_const_s now and then switch to generic one later. But name is not clashing with any other driver or even generic header now, so my weak vote is to keep, introduce generic name through this driver with example how it is used.

I would like to see the followup patch how you intend to integrate this generic driver to some BSP, for example ESP32C3 or even your design to see what is intention for attach, detach and how you intend to copy modifyreg32 from struct sja1000_config_s to struct sja1000_dev_s. I know that we have some part of similar code over-engineered in LinCAN and at the end it was too much PC ISA specific and mapping to PCI/PCIe has been hacky.

If we find some HW which is in broader use and can be used as reference in NuttX and you provide even patch to test with modified ESP32 support then I am for inclusion into mainline as generic SJA1000 driver (without ESP specific followup patch for now) and resolving use with ESP and removing all these copies later. It would be great if we find some target which allows to build CI with NuttX and QEMU branch with some experimental changes for SJA1000 mapping. I hope we would be able to get such mapping into QEMU mainline one day. From my actual perspective, it would be easiest to provide this for Zynq, but it does not have NuttX support as I know. Polarfire (MPFS) is interesting target even for me and if NutttX can be run on its QEMU emulation then it could be ideal target and I try to help as time allows (which is not much till summer).

[radek-pesina]

Hi @ppisa,

I've updated the use of the word "addr" as per your suggestion to "reg".

Regarding a demonstration of this driver's use, please refer to the pull request #11913 for an implementation on the ESP32C3.

[ppisa]

Hi @ppisa,

I've updated the use of the word "addr" as per your suggestion to "reg".

Regarding a demonstration of this driver's use, please refer to the pull request #11913 for an implementation on the ESP32C3.

Thanks for update.

As for the

static const struct can_bittiming_const_s esp32c3_twai_bittiming_const =

Are the TWAI specific const parameters required? They should be same for all give IP core/chip integrations, but may be that there are some changes in registers. I have not checked. If not, I would provide and use global sja1000_bittiming_const there.

I have provided some review to ESP32 integration where I suggest to simplify even generic interrupt registration and allow direct use, registration when argument changed to sja1000_interrupt(FAR struct sja1000_dev_s *priv, ...

As for mainline inclusion, it seems that ESP32C3 can be more problematic now and it would be for legacy only which future is probably doubtful, even that I consider integration of drivers and targets support directly into NuttX as much more long term maintainable then actual switching to the mix of repositories.

But some clean and simple integration into NuttX mainline is necessary as reference if the common part should be reusable. If at least one tested and clean integration of common code to the BSP is not included then support will become rotten in a time.

Your main target is use in FPGA with OpenCores SJA1000 or our SJA1000 FD TOL. NuttX supports only PolarFire as usable target for this now. So I suggest to add integration for the driver to PolarFire BSP and an optional device with option to setup base and irq to adapt mapping to the actual integration/bitsream. I will cooperate in longer time perspective there on ability to model such integration by QEMU mainline. It would be great if somebody can provide knowledge/help/test with building NuttX for QEMU PoalarFire SoC emulation. We can get n some time to reasonable setup for continuous integration testing for such target which can be reproduces with everybody. Expected experimental ETA for testing with QEMU end of the summer. QEMU mainline ETA end of this year or next year.

I hope that with your effort ETA for mainline NuttX would be much earlier.

[tmedicci]

Well, there are a couple of things to be explained here:

This PR itself implements a generic driver. Generic drivers should provide interfaces to be run using different interfaces (at least from different manufacturers). I see that you considered ESP32-C3 (legacy) as the starting point and, although there are some changes to be made as I just commented in #11913 (comment), I'm not against using this implementation for it, but it's worth to mention that esp32c3-legacy is planned to be discontinued in favor of the new HAL-based approach. We are no longer adding features to ESP32-C3 legacy. Unless a very nasty bug comes up, we are not touching it. It also means that any support for this peripheral will always find its way back to you. TWAI peripheral is already implemented (considering register-level access) for xtensa-based devices (ESP32, ESP32-S2 and ESP32-S3), so you can find it easier to use this generic SJA1000 with those devices (considering the same considerations of #11913 (comment)).

Something to mention about esp-hal-3rdparty. "HAL" code is tested for different systems and applications and is continuously tested with hardware in the loop. As the name suggests, it provides low-level functions to abstract register-level access between different SoCs: it's an effort of Espressif to make code cleaner and improve support for Espressif devices in NuttX, avoiding errors regarding registers (if you check, you'll end up finding same registers and memory address being read/written). The code is open and Apache-2.0-licensed (same as NuttX): please take a look at https://github.com/espressif/esp-hal-3rdparty. Drivers are always implemented on NuttX. That being said, if the objective of implementing the generic SJA1000 driver on NuttX is to ease porting between different chips, that won't be true for the ESP32-C3, ESP32-C6 and ESP32-H2 (and other RISC-V chips): TWAI driver will be common to all these devices as HAL enable us to deal with minor low-level differences between devices. Also, there is no guarantee that the TWAI peripheral will be compliant for other Espressif chips.

The xtensa-based devices and their already-existing drivers will be kept as-is if 1) they are implemented for all xtensa-based devices (the case of TWAI) and 2) no bug is found. Otherwise, the driver will be switched to use the HAL-based approach (no direct access to the registers, although anyone can check and debug it because the code is open on esp-hal-3rdparty) to support a common driver.

Finally, we highly encourage efforts for supporting common drivers for RISC-V-based Espressif SoCs using HAL. Please take this into consideration if you need a TWAI driver that is easily maintained across different SoCs.

[acassis]

Nice work @radek-pesina !!!