[RFC] drivers: intc: plic: multi-instance support - approach 2 #62481

ycsin · 2023-09-11T07:08:54Z

This PR add multi-instance support to the intc_plic driver. As all the IRQs are sharing the same ISR table, 2nd levels and onwards IRQs in one instance of a controller can collide with another in the ISR table.

To avoid this, the ISR should have an offset as large as the sum of the number of IRQs in the previous instances in the devicetree, so they dont get installed to the same index.

The RISCV's arch_irq_connect_dynamic then get this offset from the controller to install the ISR accordingly.

This PR aims to get a look and feel of the approach.

~~Why dont we just let interrupt controller to install the ISR?~~ practiced in this PR, for RISCV intc_plic only

ycsin · 2023-09-11T14:37:19Z

cc @carlocaione

soc/riscv/riscv-privileged/common/soc_common_irq.c

include/zephyr/sys/arch_interface.h

cfriedt · 2023-09-15T16:45:53Z

drivers/interrupt_controller/Kconfig

@@ -5,6 +5,15 @@

 menu "Interrupt controller drivers"

+config MULTI_INSTANCE_INTC


This shouldn't require a Kconfig - devicetree to describe hw, kconfig to describe sw

cfriedt · 2023-09-15T16:46:04Z

drivers/interrupt_controller/Kconfig

+config MULTI_INSTANCE_INTC
+	bool
+
+config HAS_MUTLI_INSTANCE_INTC


this shouldn't require a kconfig

drivers/interrupt_controller/intc_plic.c

cfriedt · 2023-09-15T17:04:56Z

include/zephyr/drivers/interrupt_controller/riscv_plic.h

 /**
 * @brief Enable interrupt
 *
 * @param irq interrupt ID
 */
-void riscv_plic_irq_enable(uint32_t irq);
+void riscv_plic_irq_enable(uint32_t irq, const struct device *dev);


The signature for this should probably not be changed.

In this approach, the IRQN doesn't carry information about the plic instance, so the dev is required

cfriedt · 2023-09-15T17:05:17Z

include/zephyr/drivers/interrupt_controller/riscv_plic.h


 /**
 * @brief Disable interrupt
 *
 * @param irq interrupt ID
 */
-void riscv_plic_irq_disable(uint32_t irq);
+void riscv_plic_irq_disable(uint32_t irq, const struct device *dev);


the signature for this should probably not be changed.

cfriedt · 2023-09-15T17:05:34Z

include/zephyr/drivers/interrupt_controller/riscv_plic.h


 /**
 * @brief Check if an interrupt is enabled
 *
 * @param irq interrupt ID
 * @return Returns true if interrupt is enabled, false otherwise
 */
-int riscv_plic_irq_is_enabled(uint32_t irq);
+int riscv_plic_irq_is_enabled(uint32_t irq, const struct device *dev);


the signature for this should probably not be changed.

as the irq doesnt carry the interrupt controller info, an additional dev arg is required in a multi-instance configuration

cfriedt · 2023-09-15T17:06:52Z

include/zephyr/drivers/interrupt_controller/riscv_plic.h

+ *
+ * @return Returns the device struct of the interrupt controller
+ */
+const struct device *riscv_plic_get_dev(void);


There can be multiple active interrupts on separate cores - is that taken into consideration here?

SMP is not considered in this PR

cfriedt · 2023-09-15T17:07:39Z

include/zephyr/irq.h

@@ -48,6 +48,20 @@ extern "C" {
 #define IRQ_CONNECT(irq_p, priority_p, isr_p, isr_param_p, flags_p) \
 	ARCH_IRQ_CONNECT(irq_p, priority_p, isr_p, isr_param_p, flags_p)

+#ifdef CONFIG_MULTI_INSTANCE_INTC


No Kconfig is necessary

drivers/interrupt_controller/intc_plic.c

cfriedt · 2023-09-15T17:34:19Z

drivers/serial/uart_ns16550.c

@@ -1237,7 +1240,8 @@ static const struct uart_driver_api uart_ns16550_driver_api = {
 	static void irq_config_func##n(const struct device *dev)              \
 	{                                                                     \
 		ARG_UNUSED(dev);                                              \
-		IRQ_CONNECT(DT_INST_IRQN(n), DT_INST_IRQ(n, priority),	      \
+		NS16550_IRQ_CONNECT(DT_INST_INTC_PARENT(n), DT_INST_IRQN(n),  \


Shouldn't this just be converted to use IRQ_INTC_CONNECT() directly? It also looks like the first parameter is supposed to be an instance number, but here it's a const struct device *.

In this approach, the IRQN doesn't carry information about the plic instance, we will need to pass in the parent interrupt controller of the uart

cfriedt · 2023-09-15T17:35:40Z

This actually looks somewhat reasonable to me, except for all of the DT changes. We don't need to adjust vendor .dts or .dtsi to fix the driver. I would leave that stuff alone.

cfriedt · 2023-09-15T17:46:55Z

drivers/interrupt_controller/intc_plic.c

+void z_riscv_plic_isr_install(unsigned int irq, unsigned int priority,
+			      void (*routine)(const void *), const void *param)
+{
+	z_riscv_plic_isr_intc_install(NULL, irq, priority, routine, param);


Here, we have the ability to get dev upfront, so might as well do that, and then it doesn't need to be done in z_riscv_plic_isr_intc_install()

LaurentiuM1234 · 2023-09-18T14:56:14Z

arch/riscv/core/irq_manage.c

-	ARG_UNUSED(flags);
-
-	z_isr_install(irq, routine, parameter);
-
 #if defined(CONFIG_RISCV_HAS_PLIC)


Pardon my ignorance but won't this break the shared interrupt support on PLIC-based RISCV SoCs? z_riscv_plic_isr_intc_install() doesn't seem to be aware of the fact that the ISR may be set to z_shared_isr() and will overwrite it. Is this behavior desired?

Yes I'm aware of this issue, the isr installer in the shared interrupt part has to be updated as well for this approach to work.

Pardon my ignorance but won't this break the shared interrupt support on PLIC-based RISCV SoCs? z_riscv_plic_isr_intc_install() doesn't seem to be aware of the fact that the ISR may be set to z_shared_isr() and will overwrite it. Is this behavior desired?

@LaurentiuM1234 - IIRC you recently updated the shared interrupt code. Were multi-level interrupts taken into account in the shared interrupt design?

If so, what would be the best way to support them?

@LaurentiuM1234 - IIRC you recently updated the shared interrupt code. Were multi-level interrupts taken into account in the shared interrupt design?

Yes the shared interrupt code is aware of the multi-level organization of the interrupts via z_get_sw_isr_table_idx() (for dynamic interrupts)

If so, what would be the best way to support them?

Mm, not sure how helpful I can be here since I'm not familiar with RISCV and its quirks. If the multiple PLIC instances are basically aggregators for different interrupt lines I would suggest taking a look at #62776. The idea is that you register a dispatcher for each of these interrupt lines which will, in turn, call of the higher level reigstered ISRs. z_isr_install() is kept unchanged to keep the support for shared interrupts.

Use a config struct to store per-instance device config during init and connect the IRQ based on the devicetree instead of hardcoded value and instance. The `get_plic_dev_from_irq` is still a placeholder for now and always return the first instance. Signed-off-by: Yong Cong Sin <ycsin@meta.com>

Extends the `riscv_plic` public header to provide an ISR installer. The ISR installer implementation in the `intc_plic` will install the ISR to the correct offset using infomation from the devicetree properties. The RISCV's `arch_irq_connect_dynamic` will use this ISR installer when `CONFIG_RISCV_HAS_PLIC` is enabled. Signed-off-by: Yong Cong Sin <ycsin@meta.com>

ycsin requested review from carlocaione, cfriedt and jgl-meta September 11, 2023 07:08

ycsin linked an issue Sep 11, 2023 that may be closed by this pull request

[RFC][RISCV] Support for multiple instances of multi-level interrupt controller #62441

Closed

ycsin force-pushed the pr/plic_multi_instance_0 branch from 598a30c to c9e14e4 Compare September 11, 2023 09:59

ycsin mentioned this pull request Sep 11, 2023

[RFC][RISCV] Support for multiple instances of multi-level interrupt controller #62441

Closed

ycsin force-pushed the pr/plic_multi_instance_0 branch 4 times, most recently from 002a64e to eb87551 Compare September 11, 2023 14:18

ycsin force-pushed the pr/plic_multi_instance_0 branch from eb87551 to 1044109 Compare September 12, 2023 05:30

ycsin changed the title ~~drivers: intc: plic: initial refactor for multi-instance support~~ drivers: intc: plic: multi-instance support Sep 12, 2023

ycsin changed the title ~~drivers: intc: plic: multi-instance support~~ drivers: intc: plic: multi-instance support - approach 2 Sep 12, 2023

ycsin force-pushed the pr/plic_multi_instance_0 branch from 1044109 to a5f21f9 Compare September 12, 2023 13:59

ycsin marked this pull request as ready for review September 12, 2023 13:59

ycsin requested review from dcpleung, nashif, andyross, mateusz-holenko, kgugala, pgielda, andy-liu-telink and ioannisg as code owners September 12, 2023 13:59

zephyrbot added area: Interrupt Controller area: GPIO area: Architectures area: RISCV RISCV Architecture (32-bit & 64-bit) area: Devicetree Binding PR modifies or adds a Device Tree binding labels Sep 12, 2023

zephyrbot requested a review from fkokosinski September 12, 2023 14:00

ycsin force-pushed the pr/plic_multi_instance_0 branch 3 times, most recently from 7b438a1 to bac14c6 Compare September 15, 2023 12:59