Rpesina/esp32c3 twai

[radek-pesina]

Summary

Replace ESP32C3 TWAI driver with common SJA1000 CAN driver (in a separate pull request). This pull request depends on the SJA1000 PR.

Impact

Commonises the ESP32C3 TWAI driver implementation with the platform-independent SJA1000 CAN driver.

Testing

Tested on ESP32C3 with Examples CAN app. successfully sent/received messages.

[tmedicci]

Please check #11868 (comment)

[ppisa]

Please consider change/simplification in ISR support.

[ppisa]

Ack

[ppisa]

Ack

[ppisa]

Ack

[ppisa]

I am not sure about layering there. I would suggest to leave chop specific

      .putreg = twai_putreg,
      .modifyreg32 = twai_modifyreg32,
      .config = &g_esp32c3_twai_config[0].config,
      .base = DR_REG_TWAI_BASE,

in the struct sja1000_config_s and copied these for speedup into private struct sja1000_dev_s during chip initialization. This way the BSP level does not need to know any details about actual driver private struct sja1000_dev_s. The arguments against this option would be if even priv structure can be kept const (in Flash), but I expect that it should be usable for the device state storage. And yes, there is some duplication and space waste due to copied base and pointers to reg functions in my prefered solution, but it provides speedup and HW access functions simpler.

[ppisa]

I am not sure why there is required so much indirection. I would expect that we can directly register common sja1000_interrupt there. It should receive struct sja1000_dev_s as argument and this function should be registered directly as ISR If you need some specific/extended config for some board then it should be defined to contain common one and can be extended in board specific handler wrapper by container_of.

On the other hand I agree with attach and detach and ability to setup there some integration specific sja1000_interrupt wrapper. But I would use same arguments for common SJA1000 interrupt ISR as for the wrappers.

[tmedicci]

Hi!

This implementation has some problems regarding board-level vs arch-level. Just to summarize these problems, please note that arch/risc-v/src/esp32c3-legacy/esp32c3_twai.c was removed and most of its implementations (like interrupt handling) were moved to boards/risc-v/esp32c3-legacy/common/src/esp32c3_board_twai.c: this means that the TWAI peripheral, that is part of the ESP32-C3 arch, is now part of the ESP32-C3-based boards. This is not the correct approach.

Even considering that this implementation is part of the SJA1000 generic driver, we should keep the TWAI driver (arch/risc-v/src/esp32c3-legacy/esp32c3_twai.c) in its location and move things like interrupt handling, general config and other configs to an init function (like esp32c3_twaiinitialize, for instance) from the board-level to arch driver.

esp32c3_board_twai.c should contain only the initialization and bindings to SJA1000, like:

twai = esp32c3_twaiinitialize(devno, tx_pin, rx_pin);

dev = sja1000_instantiate(twai->priv);

ret = can_register(devpath, dev);

All the logic to support the interrupt handling, the settings for the base register and the functions that will be registered on SJA1000 are handled in the arch driver (arch/risc-v/src/esp32c3-legacy/esp32c3_twai.c), not in the board-level.

That is, the lower-level is the TWAI driver that supports a generic interface (SJA1000) that, finally, is registered as a upper-half driver (can). Take, as an example, the audio interface in nuttx/boards/xtensa/esp32/common/src/esp32_cs4344.c:

  i2s = esp32_i2sbus_initialize(port);

  cs4344 = cs4344_initialize(i2s);

  pcm = pcm_decode_initialize(cs4344);
   
  ret = audio_register(devname, pcm);

The I2S peripheral is used to drive the cs4344 (which is a "dumb" device that received I2S packets only). This CS4344 is registered in the PCM decode (which send PCM-encoded data through I2S peripheral, which is the lower-level) and, finally, the PCM device is registered as an audio interface. All these interfaces are drivers! Some of them are generic (pcm, cs4344) and other arch-specific (the I2S), but are drivers. Device-specific drivers are implemented in the arch folder.

[radek-pesina]

Hi All,

I've provided this as an example driver usage of the commonised SJA1000. Unfortunately our project schedule's tight so I won't get a chance to further this driver as it's not our target platform. If someone finds use in this PR please feel free to take it over, else if not I am happy for it to be closed off.

[ppisa]

I understand and I think that ESP32 would take some time to resolve legacy and others. But as I have suggested, I would be happy with some option to map it into PolarFire FPGA area with configurable base as an example for others how to integrate it. I would suggest to simplify the common interrupt service routine such way that it directly accepts struct can_dev_s *dev as a main argument and can be registered directly into NuttX as the the ISR handler. Possible indirection by specifying handler in struct sja1000_config_s *config can have use for some special IRQ acknowledgement on some targets etc.

As for ESP32, I have actually received two EP32C6 kits from Brno Espressif office, we want to use them with NuttX for education. I have found that most drivers have moved to arch/risc-v/src/common/espressif but TWAI support is lost for non legacy version... So we can try to do something sane if the level of indirection through esp-hal-3rdparty allows it.

We have another NuttX update for https://github.com/ICE-V-Wireless/ICE-V-Wireless . The main community project has stuck, as I know, on the old IDF and is becoming rotten because community have problems to move forward. We have working solution based on prepared generic iCE40 NuttX FPGA SPI driver and then board mapping for legacy ESP32C3 NuttX support. I hope that @tmedicci and other Espressif people would be supportive and help us to use it even with non legacy ESP32C3 version. I have significant fear, that it would get messy and would require to tinker with esp-hal-3rdparty and would tend to break etc... But we will see and you can demonstrate that solution is flexible and works for real, knowledgeable NuttX contributors well.