driver: can_stm32fd: STM32U5 series support

warasilapm commented 2 years ago

Is this request related to a missing driver support for a particular hardware platform, SoC or board? Please describe.

In most ways the current "st,stm32-fdcan" driver is ready to be used with the STM32U5 series of processors from ST, but there are a couple of small hiccups in its usage that I've had to address while setting it up on our board to test with. At the moment, I'm still trying to work out what appears to be an IRQ related issue.

The STM32U5 is an excellent IoT MCU and I fully expect it to see quite a bit of use in the future.

zephyr/dts/arm/st/u5/stm32u5.dtsi does not contain a node for the FDCAN1 peripheral. This is a standard feature across the entire line and so should be in the base .dtsi. I know that @henrikbrixandersen is making some updates to m-can (https://github.com/zephyrproject-rtos/zephyr/pull/45014), but the below does not reflect those. (I've been on de5510668a7f7d4258cea663655dcf949591da9f locally.) This is based off of the STM32G4 device tree.

soc {
can {
    compatible = "bosch,m-can-base";
    #address-cells = <1>;
    #size-cells = <1>;
    std-filter-elements = <28>;
    ext-filter-elements = <8>;
    rx-fifo0-elements = <3>;
    rx-fifo1-elements = <3>;
    rx-buffer-elements = <0>;
    tx-buffer-elements = <3>;

    can1: can@4000a400 {
        compatible = "st,stm32-fdcan";
        reg = <0x4000a400 0x400>, <0x4000ac00 0x350>;
        reg-names = "m_can", "message_ram";
        interrupts = <47 0>, <48 0>;
        interrupt-names = "LINE_0", "LINE_1";
        status = "disabled";
        label = "CAN_1";
        sjw = <1>;
        sample-point = <875>;
        sjw-data = <1>;
        sample-point-data = <875>;
    };
};
};

For one reason or another, ST has decided to change the available clock sources for the FDCAN1 peripheral on the STM32U5 series as compared to their previous devices. I implemented a quick fix shown below based on the how the ADC driver currently handles SOC specific behavior in zephyr/drivers/can/can_stm32fd.c. Considering https://github.com/zephyrproject-rtos/zephyr/pull/44988 and https://github.com/zephyrproject-rtos/zephyr/pull/42097 it seems like there should be a better and more forward thinking way of accomplishing this.
```
// in can_stm32fd_clock_enable()
#ifdef CONFIG_SOC_SERIES_STM32U5X
LL_RCC_PLL1_EnableDomain_48M();
LL_RCC_SetFDCANClockSource(LL_RCC_FDCAN_CLKSOURCE_PLL1);
LL_APB1_GRP2_EnableClock(LL_APB1_GRP2_PERIPH_FDCAN1); // requires #include <stm32_ll_bus.h>
#else
LL_RCC_SetFDCANClockSource(LL_RCC_FDCAN_CLKSOURCE_PCLK1);
__HAL_RCC_FDCAN_CLK_ENABLE();
#endif
```
Additionally, this driver currently uses the __HAL_RCC_FDCAN_CLK_ENABLE() macro where only the LL drivers are included. While this seems to compile fine on G474 (the only other FDCAN board I have access to), my application/board combination for the STM32U5 does not expose this HAL macro to zephyr/drivers/can/can_stm32fd.c. It also seems odd right next to an LL only function call. This does require an inclusion of stm32_ll_bus.h, but I don't believe that's an issue.

Describe why you are asking for this support? I'm relatively new to using Zephyr. Between the changes to how the STM32 clocks are being implemented on H7 and U5 as well as all the work that @henrikbrixandersen is doing I'm unsure of what the correct path is for adding this support.

I am more than happy to help contribute to adding this (and other) support for the STM32U5 to Zephyr. I am simply not yet equipped to do it without potentially making a mess.

Additional context This is where I've gotten to using the blocking call to the driver. The driver hangs waiting on data->tx_fin_sem[put_idx].

Changing this to using the non-blocking call which provides a callback simply exhausts the tx mailboxes and then hangs somewhere internally. I'm not sure where precisely.

In both cases the messages are successfully sent on the bus. I believe this is an IRQ issue, but I'm not sure what/where is configured incorrectly. I just wanted to share that my changes shared above are not complete/drop-in.

erwango commented 2 years ago

Thanks for sharing this.

1- Looking to the device tree snippet submitted. It looks like the interrupt number provided are not correct. Please have a try with:

        can1: can@4000a400 {
            compatible = "st,stm32-fdcan";
            reg = <0x4000a400 0x400>, <0x4000ac00 0x350>;
            reg-names = "m_can", "message_ram";
-           interrupts = <47 0>, <48 0>;
+           interrupts = <39 0>, <40 0>;

2- Indeed, there are changes ahead regarding clock selection, but until this is merged, just follow the current scheme.

3- Good observation, I hope we should be able to get rid of this once #42097 and follow ups are merged.

warasilapm commented 2 years ago

Thanks for sharing this.

1- Looking to the device tree snippet submitted. It looks like the interrupt number provided are not correct. Please have a try with:
      can1: can@4000a400 {
          compatible = "st,stm32-fdcan";
          reg = <0x4000a400 0x400>, <0x4000ac00 0x350>;
          reg-names = "m_can", "message_ram";
-         interrupts = <47 0>, <48 0>;
+         interrupts = <39 0>, <40 0>;
2- Indeed, there are changes ahead regarding clock selection, but until this is merged, just follow the current scheme.

3- Good observation, I hope we should be able to get rid of this once #42097 and follow ups are merged.

Thanks for pointing out the interrupt line numbering. I had misunderstood which column in the data sheet mapped to the DTS numbering. My test code is working alright now. I'll run through the CAN driver example and make a PR with this support.

zephyrproject-rtos / zephyr

driver: can_stm32fd: STM32U5 series support #45123