RFC: Introduce a STM32 SYSCFG interrupt controller driver

[eraquin-kalray]

This PR is a follow up of RFC: Introduce a STM32 SYSCFG interrupt controller driver.

The advantage of this solution over current shared interrupts implementation provided by #61422 is that only relevant ISR is called.
Whereas the current solution is calling each and every ISR sharing the IT, regardless of whether there is a pending IT for the device or not. This can results in error if ISR of device driver relying on this solution isn't safe to be called without any pending IT. For instance drivers/pwm/pwm_stm32.c::pwm_stm32_isr() has some NULL pointer that aren't checked in this case.

Commits details :

• First 3 commits are a split of the first one of the original RFC from @aurel32, as requested during review.
• Purpose of commits 4 and 5 is to fix the issue highlithed by @aurel32 in this comment.
  Based on existing implementation for other socs, like the RV32M1, relying on SOC specific IT management (ARM_CUSTOM_INTERRUPT_CONTROLLER), which is selected when enabling interrupt controller driver on STM32
• Commits 6 and 7 are there to fix incorrect handling of multi level interrupts number when getting from name.
  Identified when validating the implementation using CAN counter sample code. DT_INST_IRQ_BY_NAME wasn't returning the expected multi level encoded interrupt number. Proposal is to add devicetree helpers to get (multi level) interrupt number from name, like existing ones from index.
• Commit 8 is adding a sample code to show how to setup interrupts to use SYSCFG IT lines.
  As discussed during review, IT line muxes nodes aren't enabled in .dtsi. Thus interrupts settings aren't updated to use SYSCFG IT lines, and no configuration is performed in this way. This sample code shows what has to be done on board .dts and project configuration sides.

Implementation tested by building and checking generated isr_tables.c::_sw_isr_table[], with and without SYSCFG and IT line muxes. Tested at runtime as well on NUCLEO-G0B1RE using CAN counter sample code.

Any suggestion welcomed. Please tell me if something should be added, removed, implemented another way !

[github-actions]

Hello @eraquin-kalray, 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. 😊

[eraquin-kalray]

As I feel like failing test isn't related to my changes, marking as ready for review

[erwango]

Initial question on my side :

What is the advantage of this new solution above #61422 which is currently in use to handle IRQ sharing on STM32G0 for instance ? And why we would want to support both ?

PR description should detail this.

[eraquin-kalray]

@erwango

Initial question on my side [...]

Forgot to highlight it, my bad

PR description should detail this

PR description updated accordingly

[eraquin-kalray]

Sorry for repeated force pushes, struggling a little bit with compliance checks
Now passing locally, will re-request review once online check reporting OK

[eraquin-kalray]

@JarmouniA Did you get a chance to have another look after my update ?

[JarmouniA]

@JarmouniA Did you get a chance to have another look after my update ?

I haven't done a detailed review of the driver since I am not super familiar with Zephyr's multi-level interrupts handling, but the general idea and the implementation looks fine to me.
Curious what @erwango thinks of this.

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

[KozhinovAlexander]

@eraquin-kalray Could you please say if it could make problems with my PR #85508 ? On the first look I do not see any collisions.

[eraquin-kalray]

@KozhinovAlexander I had a quick look at your PR, I don't see any collision either
As you didn't update the way IRQ_CONNECT is performed, having SYSCFG itline as interrupt parent of the EXTI should work as-is, if needed

[mathieuchopstm]

Documentation says:
This option indicates that the ARM CPU is connected to a custom (i.e non-GIC or NVIC) interrupt controller.

Since STM32 MCUs (especially the STM32G0) are fitted with an NVIC, I don't see the rationale for enabling this.

MULTI_LEVEL_INTERRUPTS already enables SoC-specific hooks:

zephyr/include/zephyr/arch/arm/irq.h

Lines 50 to 66 in 2338f8e

	#if defined(CONFIG_ARM_CUSTOM_INTERRUPT_CONTROLLER) || defined(CONFIG_MULTI_LEVEL_INTERRUPTS)
	/*
	* When a custom interrupt controller or multi-level interrupts is specified,
	* map the architecture interrupt control functions to the SoC layer interrupt
	* control functions.
	*/
	void z_soc_irq_init(void);
	void z_soc_irq_enable(unsigned int irq);
	void z_soc_irq_disable(unsigned int irq);
	int z_soc_irq_is_enabled(unsigned int irq);
	void z_soc_irq_priority_set(
	unsigned int irq, unsigned int prio, unsigned int flags);
	unsigned int z_soc_irq_get_active(void);
	void z_soc_irq_eoi(unsigned int irq);

Adding ARM_CUSTOM_INTERRUPT_CONTROLLER disables the ARM layer... which you then copy-paste in the ST-specific driver - so why enable ARM_CUSTOM_INTERRUPT_CONTROLLER in the first place?

[eraquin-kalray]

MULTI_LEVEL_INTERRUPTS already enables SoC-specific hooks

Right. Missed it as it isn't the case at revision we are using on our fork
SoC-specific IRQ management source file should be way lighter this way, updating

[mathieuchopstm]

Name should indicate this is a driver that manages only the interrupt controller aspect of SYSCFG.

[eraquin-kalray]

Agreed. Got same remark on our side
Will update both config name and description

[mathieuchopstm]

This seems very convoluted compared to intc_cavs.c / intc_dw.c (the former is multi-instanceable so it should be a good enough reference).

I think this is mostly due to the st_stm32_syscfg device - which is not involved in the interrupt tree - somehow becoming the manager of the individual ITMUX devices...? Is there a reason for this architecture, besides intc_rv32m1_intmux.c being like this?

[eraquin-kalray]

I think this is mostly due to the st_stm32_syscfg device [...] becoming the manager of the individual ITMUX devices

Agreed. Which requires itline_devs and syscfg_register_itline() to work

Is there a reason for this architecture [...] ?

Main reason is rework suggested/requested in this comment

I do agree that intc_cavs.c and intc_dw and look simpler
I also prefer "core" interrupts being enabled/disabled in the SoC-specific interrupt manager instead of the driver

Before rework, implementation was closer to #57310 one
Should we go back to it ? Having driver instances at SYSCFG itline level
I feel like decision has to be made depending on what the DTSI should look like

[mathieuchopstm]

I agree that a single binding with child-properties would be cleaner.

I don't see anything preventing instantiation of only the children (i.e., keep DT_INST_FOREACH_CHILD_STATUS_OKAY(0, ...) and drop DEVICE_DT_INST_DEFINE(0, ...)) - couldn't we go in this direction?

syscfg {  /* this node is not instantiated */
  compatible = "st,g0-syscfg";

  /* these nodes are instantiated by SYSCFG driver as 2nd-level IRQ controllers */
  itlinemux0 { };
  itlinemux1 { };
  /* ... */
};

I'm not familiar yet with how the whole multi-level IRQs work in Zephyr so maybe I'm missing something that makes this not possible... However, intc_cavs.c is used in almost the same way as what I'm proposing - the only difference is that it uses compatible-based instancing, rather than child-based - so I'm hoping it should be fine?

Wasn't aware of #57310 - will take a look.

[mathieuchopstm]

Interrupt line 0 is not shared so a mux is not needed.

Same remark for lines:

• 2 - TAMP is managed by RTC AFAIR
• 3 - both interrupts come from FLASH
• 4 - CRS is managed by RCC
• Possibly 5~7 as EXTI driver should handle shared IRQs by itself
• 9 - not shared
  • Possibly 10 and 11 too? (I have a vague memory DMA driver handles shared IRQs by itself)
• 14 - not shared
• 15 - not shared
• 19 - not shared
• 20 - not shared
• 23 - not shared
• 25 - not shared
• 27 - not shared
• 30 - not shared

[eraquin-kalray]

Agreed for IT lines that aren't shared
I do agree for EXTI as well, from what I got when having a look at #85508
Need to have a closer look for DMA ones

Regarding lines 2 and 4, didn't get your point
What do you mean by "is managed by" ?

[mathieuchopstm]

What I meant by "is managed by" is that (as far as I remember) the RTC driver is responsible for handling the TAMP interrupt, and the RCC driver is responsible for handling the CRS interrupt, so they wouldn't need someone else to do the muxing on their behalf.

[mathieuchopstm]

(General remark, not PR-specific) I'm quite puzzled by what these Kconfig options accomplish. They seem to be completely unused, except in tests/kernel/interrupt/src/sw_isr_table.c - a test that seems to be unaware of 3-level interrupts (i.e., probably very old). Maybe these are historical artifacts we can ignore?

[mathieuchopstm]

I stand corrected: gen_isr_tables.py uses them here:

zephyr/scripts/build/gen_isr_tables.py

Lines 112 to 128 in 459a078

	if self.check_sym("CONFIG_2ND_LEVEL_INTERRUPTS"):
	num_aggregators = self.get_sym("CONFIG_NUM_2ND_LEVEL_AGGREGATORS")
	self.__irq2_baseoffset = self.get_sym("CONFIG_2ND_LVL_ISR_TBL_OFFSET")
	self.__irq2_offsets = [self.get_sym('CONFIG_2ND_LVL_INTR_{}_OFFSET'.
	format(str(i).zfill(2))) for i in
	range(num_aggregators)]
	self.__log.debug('2nd level offsets: {}'.format(self.__irq2_offsets))
	if self.check_sym("CONFIG_3RD_LEVEL_INTERRUPTS"):
	num_aggregators = self.get_sym("CONFIG_NUM_3RD_LEVEL_AGGREGATORS")
	self.__irq3_baseoffset = self.get_sym("CONFIG_3RD_LVL_ISR_TBL_OFFSET")
	self.__irq3_offsets = [self.get_sym('CONFIG_3RD_LVL_INTR_{}_OFFSET'.
	format(str(i).zfill(2))) for i in
	range(num_aggregators)]
	self.__log.debug('3rd level offsets: {}'.format(self.__irq3_offsets))

This also means we can't have more than 8 muxed devices at a time, as far as I understand this... maybe gen_isr_tables could be improved to automagically figure this out (or just not need it), but that's outside this PR's scope.

[henrikbrixandersen]

Please avoid commits combining white-space and functional changes like this. It's very difficult to read.

[henrikbrixandersen]

Please rebase on main, resolve any merge conflicts, and address the review comments.

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.