Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stm32g0: add fdcan2 in dts file #67905

Merged

Conversation

adri1mart1
Copy link

@adri1mart1 adri1mart1 commented Jan 22, 2024

Hello,

Following my recent opened discussion, I have tried to deal with the dual can controller of the stm32g0.

Right now, it is not supported, however, it seems very small changes are needed to make it work.

STM32G0 shares the same interrupt line for both its CAN controllers and about a year ago, the shared interrupt line had been added (see shared interrupt PR)

Another interesting information is about the driver itself, the can_stm32_fdcan.c implements a macro to generate fdcan instances based on all enabled fdcan controller nodes in dts that are with a status okay (see macro).

So on my side, i've just added the fdcan2 for the stm32g0 in the stm32g0b1.dtsi and also on the nucleo_g0b1re board with appropriate pinout.

To test my setup, I have used the nucleo_g0b1re board with 2 external transceivers on a breadboard to set up a communication between fdcan1 and fdcan2. I used a PEAK CAN probe to check frames are well physically transferred from one to another.
I have tested only fdcan1, then only fdcan2, then both, it seems to work great !

Right now, there is no sample program from zephyr ready to go with this configuration so I had to update the counter example for my needs and I'm definitely not sure it is absolutely needed.

Important note on this modification. In the nucleo_g0b1re dts file, I have added the fdcan2 node with the correct pinout. However, fdcan2 is disabled by default whereas fdcan1 is enabled.

If you enable fdcan1 and fdcan2, you will get this error:

[151/155] Generating isr_tables.c
FAILED: zephyr/isr_tables.c .../build/zephyr/isr_tables.c 
[...]
gen_isr_tables.py: error: multiple registrations at table_index 22 for irq 22 (0x16)
Existing handler 0x8009c8b, new handler 0x8009c8b
Has IRQ_CONNECT or IRQ_DIRECT_CONNECT accidentally been invoked on the same irq multiple times?

You must add the CONFIG_SHARED_INTERRUPT=y flag to make it work. This could be confusing for anyone who is not familiar with the fact that stm32g0 fdcan controller is concerned about shared interrupt.

I am not sure if the zephyr community wants to live with it, improve the comment above when the error is raised, add a specific paragraph in the nucleo_g0b1re doc page. Any feedback will be very appreciated.

Regards,
Adrien M.

Copy link

Hello @adri1mart1, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

Copy link
Member

@henrikbrixandersen henrikbrixandersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erwan Do you know if all compatible = "st,stm32-fdcan" instances across the STM32 use shared interrupts?

If so, we need to automatically enable shared IRQs on the driver Kconfig level. If not, we need to enable it on SoC Kconfig level. Regardless of which it is, we'll likely need a new Kconfig function for checking the number of status=okay nodes with a given compatible.

@erwango
Copy link
Member

erwango commented Jan 22, 2024

@henrikbrixandersen From what I can see, only stm32g0b1 and stm32g0c1 provide FDCAN, but both would require shared interrupts.

@henrikbrixandersen
Copy link
Member

@henrikbrixandersen From what I can see, only stm32g0b1 and stm32g0c1 provide FDCAN, but both would require shared interrupts.

Thanks. Based on that, I'd like to see something like the following added to this PR:

diff --git a/soc/arm/st_stm32/stm32g0/Kconfig.defconfig.stm32g0b1xx b/soc/arm/st_stm32/stm32g0/Kconfig.defconfig.stm32g0b1xx
index 86c47b158e..05d2116225 100644
--- a/soc/arm/st_stm32/stm32g0/Kconfig.defconfig.stm32g0b1xx
+++ b/soc/arm/st_stm32/stm32g0/Kconfig.defconfig.stm32g0b1xx
@@ -11,4 +11,7 @@ config SOC
 config NUM_IRQS
        default 31
 
+config SHARED_INTERRUPTS
+       default y if $(dt_nodelabel_enabled,fdcan1) && $(dt_nodelabel_enabled,fdcan2)
+
 endif # SOC_STM32G0B1XX

@henrikbrixandersen
Copy link
Member

Okay, sounds good. Please split this into two commits, though. One for the SoC+DTS, one for the board.

@adri1mart1
Copy link
Author

Done on my side

Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update nucleo_g0b1re.yaml file to mention it supports can.
Aim is that it could be automatically compiled/tested in CI.
Otherwise LGTM, thanks for this contribution

erwango
erwango previously approved these changes Jan 25, 2024
Adrien MARTIN added 2 commits January 25, 2024 12:16
The STM32G0 soc has 2 CAN controllers. The 2nd on was not working
with zephyr yet as both controllers shares the same IRQ. Recently, the
shared irq system was integrated on now, both can controllers can work
on this chip. Shared interrupts must be enabled only if both can
controllers are enabled.

Signed-off-by: Adrien MARTIN <adrienmar@kickmaker.net>
The nucleo_g0b1re board is based on stm32g0 that supports 2 can
controllers. This commit adds support for the second can controller.
Add also can label in yml board file.

Signed-off-by: Adrien MARTIN <adrienmar@kickmaker.net>
Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved as I don't want to be picky, but still advise use of depends on FOO when only one symbol is impacted (while if FOO should be reserved when more than one are impacted by the condition).

@fabiobaltieri fabiobaltieri merged commit 2d8f4d1 into zephyrproject-rtos:main Jan 25, 2024
20 checks passed
Copy link

Hi @adri1mart1!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants