riscv: privileged: custom Interrupt Controller API #63214

amr-sc · 2023-09-28T10:19:15Z

It looks like the existing path providing IRQ handling functionality for PLIC within the RISC-V Privileged IRQ infrastructure basically fits to any Interrupt Controller which handles 2nd level IRQs, including the custom ones.

This change introduces the extension for RISC-V privileged IRQ infrastructure API to simplify addition of the custom Interrupt Controllers.

The specific Inerrupt Controller implementation is getting connected to the IRQ infrastructure through this API. The related functions are to be implemented by any custom Interrupt Controllers for 2nd level IRQ handling. At the same time no changes for the infrastructure itself is necessary when a new Interrupt Controller is added.

amr-sc · 2023-09-28T10:21:05Z

This change paves ground for #62606, ping @carlocaione

amr-sc · 2023-10-02T07:35:23Z

Hi @carlocaione , @fkokosinski , guys could you please take a look?
Thanks!

soc/riscv/riscv-privileged/Kconfig

soc/riscv/riscv-privileged/common/soc_common.h

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

amr-sc · 2023-10-05T10:33:23Z

Hi @carlocaione ,

Could you please clarify the questions above?
Thanks!

amr-sc · 2023-10-10T07:10:09Z

Hi @carlocaione , could you please take a look? I've addressed your comments.
Thanks!

soc/riscv/riscv-privileged/Kconfig

soc/riscv/riscv-privileged/common/soc_common.h

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

soc/riscv/riscv-privileged/common/soc_common.h

soc/riscv/riscv-privileged/Kconfig

amr-sc · 2023-10-11T11:59:32Z

Hi @carlocaione , just a general question (please correct me if I'm wrong): after adding the z_soc_ prefix to the API to be implemented by Custom IC we assume that the IC driver should be placed in soc/ directory and not in drivers/ directory, right?

I was expecting that we would use drivers/ directory for the IC driver. So maybe we should have z_ but drop soc part from that API? What do you think?

UPD: in case we decide to place the custom IC driver to drivers/ directory, then probably we shouldn't declare custom IC API under soc/ and move it to arch/riscv/irq.h instead? It does look like a generic RISC-V functionality which is not bound to particular SoC, but to be implemented by particular RISC-V IC driver

UPD2: @carlocaione , one step further in the direction of mentioned above: if we implement custom IC API within the IC driver code in drivers/interrupt_controller then why do we need to redefine the arch_irq_* functions at all? Why not to implement arch_irq_enable/disable/is_enabled/ z_riscv_irq_priority_set directly in the driver?

carlocaione · 2023-10-12T08:01:11Z

Hi @carlocaione , just a general question (please correct me if I'm wrong): after adding the z_soc_ prefix to the API to be implemented by Custom IC we assume that the IC driver should be placed in soc/ directory and not in drivers/ directory, right?

Correct.

I was expecting that we would use drivers/ directory for the IC driver. So maybe we should have z_ but drop soc part from that API? What do you think?

But this IC is specific to your SoC (and in this case it can be very well in the soc/ directory, because it is only used by your platform) or it can be used by other SoCs as well (and in that case it should be indeed in the general drivers/ directory)?

UPD: in case we decide to place the custom IC driver to drivers/ directory, then probably we shouldn't declare custom IC API under soc/ and move it to arch/riscv/irq.h instead? It does look like a generic RISC-V functionality which is not bound to particular SoC, but to be implemented by particular RISC-V IC driver

Yes, if the driver can be used by different SoCs / platforms.

UPD2: @carlocaione , one step further in the direction of mentioned above: if we implement custom IC API within the IC driver code in drivers/interrupt_controller then why do we need to redefine the arch_irq_* functions at all? Why not to implement arch_irq_enable/disable/is_enabled/ z_riscv_irq_priority_set directly in the driver?

I don't think there is a strict rule here but if possible I'd like to keep separate the arch layer from the drivers layer, so avoiding declaring any arch_* function in a driver.

amr-sc · 2023-10-12T09:09:45Z

@carlocaione , please find my answers below:

But this IC is specific to your SoC (and in this case it can be very well in the soc/ directory, because it is only used by your platform) or it can be used by other SoCs as well (and in that case it should be indeed in the general drivers/ directory)?

Our custom IC (IPIC) is used with multiple Syntacore SoCs. Also in common case it seems that the same custom IC could be used with different RISC-V SoCs. Ok, so I'll remove the custom IC API declaration from soc/ to arch/ then.
Meanwhile, if some platform will have a unique SoC-specific IC it would still be able to allocate the driver within the soc/

UPD2: @carlocaione , one step further in the direction of mentioned above: if we implement custom IC API within the IC driver code in drivers/interrupt_controller then why do we need to redefine the arch_irq_* functions at all? Why not to implement arch_irq_enable/disable/is_enabled/ z_riscv_irq_priority_set directly in the driver?

I don't think there is a strict rule here but if possible I'd like to keep separate the arch layer from the drivers layer, so avoiding declaring any arch_* function in a driver.

I've just checked and it seems that actually a lot of interrupt controller drivers implement arch_irq_enable/disable/is_enabled functions. I'm also thinking this way: with redefinition of arch_irq_* functions, we show what? That for custom ICs we are skipping some architecture-wide layer, right? Such common layer is possible for standardized RISC-V controllers, like PLIC, but not the custom ones, since there is no common rule for them. Probably the redefinition is indeed better.
What do you say?

carlocaione · 2023-10-12T09:18:18Z

Our custom IC (IPIC) is used with multiple Syntacore SoCs. Also in common case it seems that the same custom IC could be used with different RISC-V SoCs. Ok, so I'll remove the custom IC API declaration from soc/ to arch/ then. Meanwhile, if some platform will have a unique SoC-specific IC it would still be able to allocate the driver within the soc/

Yup, fine to me.

I've just checked and it seems that actually a lot of interrupt controller drivers implement arch_irq_enable/disable/is_enabled functions. I'm also thinking this way: with redefinition of arch_irq_* functions, we show what? That for custom ICs we are skipping some architecture-wide layer, right? Such common layer is possible for standardized RISC-V controllers, like PLIC, but not the custom ones, since there is no common rule for them. Probably the redefinition is indeed better. What do you say?

We are trying to clean-up tree-wide the usage of prefix and interfaces, see #61588 so I would still suggest to avoid using the arch_* prefix in the drivers to avoid getting caught in the cleaning process.

amr-sc · 2023-10-13T06:38:14Z

Hi @carlocaione , I've addressed your previous comments, please take a look.
Thanks!

amr-sc · 2023-10-16T08:50:16Z

Hi @carlocaione , just a kind reminder for a review

amr-sc · 2023-10-17T15:29:33Z

@carlocaione , @fkokosinski , guys there is no response from you unfortunately

carlocaione · 2023-10-17T19:33:45Z

@carlocaione , @fkokosinski , guys there is no response from you unfortunately

please, stop pinging me every single day.

Read https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#pr-review-escalation if you have not done it yet.

arch/riscv/Kconfig

amr-sc · 2023-10-18T07:31:46Z

@carlocaione , @fkokosinski , guys there is no response from you unfortunately

please, stop pinging me every single day.

Read https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#pr-review-escalation if you have not done it yet.

Hi @carlocaione ,
You're right, I didn't know about 1-week rule. Thanks for sharing this info!
I'm sorry for disturbing you

This change introduces the extension for RISC-V IRQ infrastructure API to simplify addition of the custom Interrupt Controllers. The specific RISC-V Inerrupt Controller implementation is getting connected to the Zephyr IRQ infrastructure through this API. The related functions are to be implemented by any custom Interrupt Controller driver. Signed-off-by: Alexander Razinkov <alexander.razinkov@syntacore.com>

amr-sc · 2023-10-27T07:22:26Z

Hi @carlocaione , just a friendly reminder for a review

carlocaione · 2023-11-03T11:06:14Z

include/zephyr/arch/riscv/irq.h

+ *
+ * @param irq IRQ number to enable
+ */
+void z_irq_enable(unsigned int irq);


@nashif I'm out of touch with this topic. What's the latest on the z_* prefix?

Hi @nashif ,

Is it fine if we use z_* prefix for RISC-V wide API for custom interrupt controllers?
Such API is to be instantiated by the particular RISC-V custom interrupt controller driver in /drivers/interrupt_controller.
Additionally we consider the special case when the interrupt_controller is soc-specific and the driver is placed in /soc/riscv

What do you think?

Hi @nashif ,

There was no response from you unfortunately

Hi @carlocaione , @nashif , @fkokosinski , I came to the Dev Meeting yesterday, but unfortunately you were not participating, so we weren't able to discuss the API. How would we proceed? Would you come to the next Dev Meeting, or maybe let's discuss it on other meeting, like Architecture WG? Or maybe you could suggest your proposal for this API here? This goes terribly slow

sorry for missing all of this dicussion earlier. If you need someone in the devreview meeting, you need to notify them out of github somehow, it is easy to miss notifications. Anyways, the direction we are taking can be seen here: #61547.

Hi @nashif ,

Thanks for your reply! I've checked your proposal and I think I've got your idea.
Please correct me if I'm wrong regarding the next steps here:

Instead of z_irq_* functions introduced below we should bring the new prefix, platfrom_*, and change the API like this:

void platform_irq_init(void); void platform_irq_enable(unsigned int irq); void platform_irq_disable(unsigned int irq); int platform_irq_is_enabled(unsigned int irq);

We still want to define this API on include/zephyr/arch/riscv/ level and not e.g. include/zephyr/arch/ level.

We should change RISCV_CUSTOM_INTERRUPT_CONTROLLER to more generic PLATFORM_HAS_CUSTOM_INTERRUPT_CONTROLLER and move this config from arch/riscv/Kconfig to arch/Kconfig

Will push an update version of #61547 but while working on it, I realized that this issue and can be dealt with by providing an interrupt controller driver and should be a much cleaner solution than doing this using overrides and custom hooks, this is what drivers (and especially intc driver are supposed to do). So why can't this be solved using a driver? all of the above calls are basically what intc drivers are supposed to provide anyways. I will push my changes regardless, but we might to rethink the interrupt related part in it.

Hi @nashif ,

all of the above calls are basically what intc drivers are supposed to provide anyways

Absolutely! Please take a look at how all those arch_irq_xxx() functions are implemented for RISC-V, it is just one or some other form of redirection to particular unique intc_ function.
And those functions are indeed should be implemented by any IC, no matter standard or custom one.
So introduction of the clean API mandatory for all the IC implementation may remove this level of indirection entirely.

We still have ARCH-specifics, but it does look, like this specifics could be kept on arch/ level instead of sipping to soc/ level (like we have with RISC-V) just for the sake of calling the unique intc_ function within a particular driver

#66505 could become a generic solution

include/zephyr/arch/riscv/irq.h

amr-sc · 2023-11-23T13:23:45Z

Hi @carlocaione , @nashif , @fkokosinski ,

How about to discuss it on Architecture WG meeting? In case you like the idea, could you please add a AWG label?

Thanks in advance!

carlocaione · 2023-12-04T08:23:46Z

@carlescufi can you add this to the next arch meeting?

nashif · 2023-12-04T19:52:51Z

@carlocaione

@carlescufi can you add this to the next arch meeting?

this week it is @henrikbrixandersen

henrikbrixandersen · 2023-12-05T09:08:22Z

@carlocaione

@carlescufi can you add this to the next arch meeting?

this week it is @henrikbrixandersen

@carlocaione I've added it to today's Arch WG meeting agenda.

cfriedt · 2023-12-05T16:09:58Z

Kind of an important to note: it isn't always the case that there is a single interrupt controller to service all interrupts in a system.

Whatever API evolves from here must take that into account.

cfriedt · 2023-12-05T16:10:21Z

cc @ycsin @luchnikov

cfriedt · 2023-12-05T16:46:35Z

Just a note, part of the issue with the RISC-V PLIC is that the spec has some ambiguity.

riscv/riscv-plic-spec#52

henrikbrixandersen · 2023-12-06T10:02:44Z

Architecture WG meeting 2023-12-05:

General consensus that the solution described in riscv: privileged: custom Interrupt Controller API #63214 (comment) is the right approach
This approach is described in general in Define a platform hook interface and cleanup misuse of private calls #61547
@amr-sc to rework this PR with inspiration from Define a platform hook interface and cleanup misuse of private calls #61547

github-actions · 2024-02-13T00:29:50Z

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.

amr-sc requested review from kgugala and pgielda as code owners September 28, 2023 10:19

zephyrbot added the area: RISCV RISCV Architecture (32-bit & 64-bit) label Sep 28, 2023

zephyrbot requested review from carlocaione, edersondisouza, fkokosinski, katsuster, mgielda and tgorochowik September 28, 2023 10:19

zephyrbot assigned fkokosinski Sep 28, 2023

amr-sc force-pushed the riscv-custom-ic branch from 43f2e9d to ddf13c4 Compare September 28, 2023 10:26

carlocaione requested changes Oct 2, 2023

View reviewed changes

amr-sc force-pushed the riscv-custom-ic branch 3 times, most recently from 1b07867 to f4cc7f8 Compare October 9, 2023 07:43

carlocaione requested changes Oct 10, 2023

View reviewed changes

amr-sc force-pushed the riscv-custom-ic branch from f4cc7f8 to 4b2a270 Compare October 10, 2023 09:14

carlocaione requested changes Oct 11, 2023

View reviewed changes

soc/riscv/riscv-privileged/common/soc_common.h Outdated Show resolved Hide resolved

soc/riscv/riscv-privileged/common/soc_common.h Outdated Show resolved Hide resolved

soc/riscv/riscv-privileged/Kconfig Outdated Show resolved Hide resolved

amr-sc force-pushed the riscv-custom-ic branch from 4b2a270 to 04351ce Compare October 11, 2023 11:21

amr-sc force-pushed the riscv-custom-ic branch 2 times, most recently from 737f7ba to ac8094c Compare October 12, 2023 10:25

carlocaione requested changes Oct 17, 2023

View reviewed changes

arch/riscv/Kconfig Outdated Show resolved Hide resolved

amr-sc force-pushed the riscv-custom-ic branch from ac8094c to b1bf19f Compare October 18, 2023 07:39

carlocaione reviewed Nov 3, 2023

View reviewed changes

xingrz reviewed Nov 7, 2023

View reviewed changes

include/zephyr/arch/riscv/irq.h Show resolved Hide resolved

xingrz mentioned this pull request Nov 7, 2023

Add support for WCH CH56x series SoC #64472

Draft

3 tasks

carlocaione added the dev-review To be discussed in dev-review meeting label Nov 24, 2023

carlocaione added Architecture Review Discussion in the Architecture WG required and removed dev-review To be discussed in dev-review meeting labels Dec 4, 2023

cfriedt requested a review from ycsin December 5, 2023 16:07

cfriedt requested a review from luchnikov December 5, 2023 16:10

github-actions bot added the Stale label Feb 13, 2024

github-actions bot closed this Feb 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

riscv: privileged: custom Interrupt Controller API #63214

riscv: privileged: custom Interrupt Controller API #63214

amr-sc commented Sep 28, 2023

amr-sc commented Sep 28, 2023

amr-sc commented Oct 2, 2023

amr-sc commented Oct 5, 2023

amr-sc commented Oct 10, 2023

amr-sc commented Oct 11, 2023 •

edited

Loading

carlocaione commented Oct 12, 2023

amr-sc commented Oct 12, 2023

carlocaione commented Oct 12, 2023

amr-sc commented Oct 13, 2023

amr-sc commented Oct 16, 2023

amr-sc commented Oct 17, 2023

carlocaione commented Oct 17, 2023

amr-sc commented Oct 18, 2023

amr-sc commented Oct 27, 2023

carlocaione Nov 3, 2023

amr-sc Nov 10, 2023

amr-sc Nov 17, 2023

amr-sc Dec 1, 2023

nashif Dec 4, 2023

amr-sc Dec 5, 2023

nashif Dec 7, 2023

amr-sc Dec 7, 2023

amr-sc Dec 14, 2023

amr-sc commented Nov 23, 2023

carlocaione commented Dec 4, 2023

nashif commented Dec 4, 2023

henrikbrixandersen commented Dec 5, 2023

cfriedt commented Dec 5, 2023

cfriedt commented Dec 5, 2023

cfriedt commented Dec 5, 2023

henrikbrixandersen commented Dec 6, 2023

github-actions bot commented Feb 13, 2024

riscv: privileged: custom Interrupt Controller API #63214

riscv: privileged: custom Interrupt Controller API #63214

Conversation

amr-sc commented Sep 28, 2023

amr-sc commented Sep 28, 2023

amr-sc commented Oct 2, 2023

amr-sc commented Oct 5, 2023

amr-sc commented Oct 10, 2023

amr-sc commented Oct 11, 2023 • edited Loading

carlocaione commented Oct 12, 2023

amr-sc commented Oct 12, 2023

carlocaione commented Oct 12, 2023

amr-sc commented Oct 13, 2023

amr-sc commented Oct 16, 2023

amr-sc commented Oct 17, 2023

carlocaione commented Oct 17, 2023

amr-sc commented Oct 18, 2023

amr-sc commented Oct 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amr-sc commented Nov 23, 2023

carlocaione commented Dec 4, 2023

nashif commented Dec 4, 2023

henrikbrixandersen commented Dec 5, 2023

cfriedt commented Dec 5, 2023

cfriedt commented Dec 5, 2023

cfriedt commented Dec 5, 2023

henrikbrixandersen commented Dec 6, 2023

github-actions bot commented Feb 13, 2024

amr-sc commented Oct 11, 2023 •

edited

Loading