arch: arc: add support for multilevel interrupts

[afongTT]

Update the arch_irq_enable/disable functions to support multilevel interrupts on the ARC architecture.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[ycsin]

The use of multilevel irq APIs LGTM, but I'm not familiar with ARC nor irq_nextlevel

[ruuddw]

Looks ok to me as well, but is there any SoC/board or example also using this (i.e. some testing has been done)?

[tagunil]

Do you have a reason to left unchanged the ARConnect-aware variant above? I believe it should work with multi-level interrupts just as well.

[ruuddw]

Few more remarks/questions, I'm not very familiar with multi-level interrupts in Zephyr:
When enabling a level n interrupt, shouldn't all intermediate levels (1..n-1) be enabled as well?
Should arch_irq_is_enabled() be updated as well to return not only level 1 but also all intermediate level (all levels need to be enabled for an interrupt to really be enabled?) or just the lowest level?

[cfriedt]

Few more remarks/questions, I'm not very familiar with multi-level interrupts in Zephyr: When enabling a level n interrupt, shouldn't all intermediate levels (1..n-1) be enabled as well? Should arch_irq_is_enabled() be updated as well to return not only level 1 but also all intermediate level (all levels need to be enabled for an interrupt to really be enabled?) or just the lowest level?

Yes, that's how multi-level interrupts work in Zephyr.

[alexapostolu]

Few more remarks/questions, I'm not very familiar with multi-level interrupts in Zephyr:
When enabling a level n interrupt, shouldn't all intermediate levels (1..n-1) be enabled as well?
Should arch_irq_is_enabled() be updated as well to return not only level 1 but also all intermediate level (all levels need to be enabled for an interrupt to really be enabled?) or just the lowest level?

I think irq_enable_next_level() already does that

[ycsin]

When enabling a level n interrupt, shouldn't all intermediate levels (1..n-1) be enabled as well?

That dependency is enforced in the Kconfig

zephyr/drivers/interrupt_controller/Kconfig.multilevel

Lines 91 to 96 in b3ccee4

	config 3RD_LEVEL_INTERRUPTS
	bool "Third-level interrupt support"
	depends on 2ND_LEVEL_INTERRUPTS
	help
	Third level interrupts are used to increase the number of
	addressable interrupts in a system.

[afongTT]

Looks ok to me as well, but is there any SoC/board or example also using this (i.e. some testing has been done)?

We're using this in production in Tenstorrent Blackhole chip:

• Level 1 interrupt controller: snps,arcv2-intc
• Level 2 interrupt controller: snps,designware-intc

Few more remarks/questions, I'm not very familiar with multi-level interrupts in Zephyr: When enabling a level n interrupt, shouldn't all intermediate levels (1..n-1) be enabled as well? Should arch_irq_is_enabled() be updated as well to return not only level 1 but also all intermediate level (all levels need to be enabled for an interrupt to really be enabled?) or just the lowest level?

From what I've seen, the initialization function of the interrupt aggregator device is responsible for enabling its parent interrupt line. So irq_enable() should only enable/disable the interrupt at the highest level.

e.g. intc_plic (interrupt aggregator) enabling its parent interrupt during device init:
plic_init()
plic_irq_config...()

[ruuddw]

From the answers, and having a quick look at other multi-level sources in the tree, I'm still not sure yet. Should the sequence be that irq_enable() enables its own interrupt (on its own level), and then call irq_enable_next_level() to ensure the full interrupt level chain is enabled?
The PR only calls irq_enable_next_level() and then returns so z_arc_v2_irq_unit_int_enable(irq) may never be called? Or, will irq_enable() be recursively called again by irq_enable_next_level() with a lower level so ultimately z_arc_v2_irq_unit_int_enable(irq) is executed?

With current logic, things probably work as the 'root' and intermediate interrupts are and remain always enabled. Only 'leaf' interrupts are enabled/disabled. But the "next_level" implies other intended logic to me.

[cfriedt]

@ruuddw - can you make suggestions for code changes? @evgeniy-paltsev - can you please review?

[cfriedt]

@ruuddw @evgeniy-paltsev - just pinging again to see if you are able to revisit

[cfriedt]

@ycsin - since you wrote much of the multi-level interrupt handling code, can you address @ruuddw's comment?

#92269 (comment)

I don't believe there are changes required in this PR, and it is working in our firmware as-is, but maybe we've missed something. If so, please suggest changes. @ruuddw - if you are able to suggest changes in the code itself, that might be helpful.

[ycsin]

@ycsin - since you wrote much of the multi-level interrupt handling code, can you address @ruuddw's comment?

I guess @ruuddw's main concern is that if something used irq_enable() on a third level IRQ, the firmware will only try to enable the third level IRQ and return, leaving second and first level IRQs potentially disabled.

If the interrupt controllers are implemented as a driver (i.e. using one of DEVICE_DT_DEFINE()), the dependency is taken care of by the build system - Zephyr will initialize (and enable the interrupt) level by level during init, so when the app enables something in the third level, the second and first level would have been enabled by Zephyr.

Conversely, if everything is done in the app (i.e. not defined in devicetree, not initialized by Zephyr), then it is the dev's responsibility to call irq_enable() multiple times to ensure that all neccessary levels are enabled.

I reckon we don't do runtime check for these to keep things simple (and probably encourage devs to implement hardware with the device APIs? I don't know)

So @ruuddw's concern is valid, but generally not an issue, that's how the RISCV works as well. Can't really comment on the 'nextlevel' APIs usage as I'm not familiar with it, arc maintainers can probably take a look.

EDIT: had a brief look at the IRQ 'nextlevel' APIs while I'm typing this comment and it seems like the nextlevel API is kinda similar conceptually to the multilevel IRQ but might do things differently

For example, in soc/intel/intel_adsp/cavs/irq.c the irq_enable() implementation would enable the base IRQ then its next level IRQ, the IRQ is multi-level (the implementation will shifts things around), but doesn't seem to be configurable like the multi-level IRQ used by RISCV

This PR's implementation is closer to soc/openisa/rv32m1/soc.c, where the nextlevel API is used as a mean to abstract away the underlying interrupt controller

[ruuddw]

Thanks @ycsin for the explanations. I came to similar conclusions. I see two different approaches in other soc's.
See z_soc_irq_enable() in soc/intel/intel_adsp/cavs\irq.c : it enables both own and next level. It also handles in the z_soc_irq_disable() conditional disabling in the parent when all connected interrupts are disabled.
Then see arch_irq_enable() in soc/openisa/rv32m1/soc.c : it enables either own if level is 1, or passes the request on to next level.

Both may work, explicitly manage all intermediate controllers when enabling/disabling a leave interrupt, or by default enable all intermediate levels to pass on any interrupt and only enable/disable the leave controllers. But the "next_level" name and call made me think the explicit handling of the full chain was intended, instead of always enable all intermediate controllers.
The approaches cannot be mixed, so at least it should be documented which approach is used/assumed for the ARC integrated interrupt controller (and probably mention somewhere the same for the second level controller that the platform uses).

[ruuddw]

Hi @afongTT, can you please confirm that from the two possible strategies for managing the multilevel interrupts, you chose the one best fitting your use-case (enable/disabling the final-level controller interrupt only, and have all others always enabled)? And maybe add a note/documentation that this approach is assumed?
Then we can merge this request and (finally) close it.