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

[RFC] risc-v: drivers: intc_plic: enabled interrupts to be handled by all cores #65922

Conversation

con-pax
Copy link
Contributor

@con-pax con-pax commented Nov 29, 2023

This patch set is to fix an issue #65489.
The problem to fix is that the first context of the PLIC was hardcoded into the driver. The side effect of that was to disable other cores in a multi-core complex from handling interrupts.

The solution presented here started off life as a cut and past from what is done in Linux, however A slightly more elegant solution raised its head during the process: basically, We know how many CPU's will be associated with a Zephyr application prior to compile time. Also, if its an SMP set up we have a mapping of the hartid to the cpu right from the start.

We can leverage this list of CPU's to enable only the contexts of the harts that are required to run in Zephyr, allowing Zephyr to also play nicely in an AMP setup.

fkokosinski
fkokosinski previously approved these changes Nov 30, 2023
Copy link
Collaborator

@ycsin ycsin left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this PR!

drivers/interrupt_controller/intc_plic.c Outdated Show resolved Hide resolved
drivers/interrupt_controller/intc_plic.c Outdated Show resolved Hide resolved
drivers/interrupt_controller/intc_plic.c Outdated Show resolved Hide resolved
drivers/interrupt_controller/intc_plic.c Outdated Show resolved Hide resolved
drivers/interrupt_controller/intc_plic.c Outdated Show resolved Hide resolved
Comment on lines +199 to +216
uint32_t en_value;
uint32_t key;
Copy link
Collaborator

Choose a reason for hiding this comment

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

these 2 line changes are probably not necessary

drivers/interrupt_controller/intc_plic.c Outdated Show resolved Hide resolved
drivers/interrupt_controller/intc_plic.c Outdated Show resolved Hide resolved
@cfriedt
Copy link
Member

cfriedt commented Dec 1, 2023

I think it would be important for @luchnikov to review as well.

@con-pax con-pax force-pushed the mpfs-icicle_plic_multi-context_support branch 3 times, most recently from 2ee7256 to 58cd3d2 Compare December 7, 2023 12:50
Copy link
Collaborator

@ycsin ycsin left a comment

Choose a reason for hiding this comment

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

LGTM in general, just some nits

drivers/interrupt_controller/intc_plic.c Outdated Show resolved Hide resolved
@con-pax con-pax force-pushed the mpfs-icicle_plic_multi-context_support branch from 58cd3d2 to 1abdf05 Compare December 8, 2023 09:33
cfriedt
cfriedt previously approved these changes Dec 8, 2023
ycsin
ycsin previously approved these changes Dec 8, 2023
fkokosinski
fkokosinski previously approved these changes Dec 8, 2023
@cfriedt
Copy link
Member

cfriedt commented Dec 8, 2023

Ugh... @con-pax - may need to rebase and resolve conflicts.

@con-pax
Copy link
Contributor Author

con-pax commented Dec 8, 2023

Ugh... @con-pax - may need to rebase and resolve conflicts.

@cfriedt hehe, I was pipped to the post! That's no problem, I can rebase now and push again. Thanks a million!

@con-pax con-pax dismissed stale reviews from fkokosinski, ycsin, and cfriedt via 644efbc December 8, 2023 13:05
@con-pax con-pax force-pushed the mpfs-icicle_plic_multi-context_support branch from 1abdf05 to 644efbc Compare December 8, 2023 13:05
@con-pax
Copy link
Contributor Author

con-pax commented Dec 8, 2023

@cfriedt rebased and pushed.
@ycsin @fkokosinski could I trouble you guys and ask for your re-approval?
Thanks a million for all the help on this!

fkokosinski
fkokosinski previously approved these changes Dec 8, 2023
@cfriedt
Copy link
Member

cfriedt commented Dec 8, 2023

@ycsin - can you do a bisect on the test that's failing? I would guess it's probably from one of the ones that were just merged ahead of this.

@con-pax
Copy link
Contributor Author

con-pax commented Dec 8, 2023

@ycsin - can you do a bisect on the test that's failing? I would guess it's probably from one of the ones that were just merged ahead of this.

@cfriedt Hey Chris, apologies, this was my bad: I see something that in my haste, my rebase wasn't successful: I will rebase again from the point of previous approvals

@con-pax
Copy link
Contributor Author

con-pax commented Dec 8, 2023

Hey @cfriedt, @ycsin
All CI builds are passing again after subsequent rebase.
Apologies for the total spam.
Cheers!

The plic uses contexts to seperate irq enables, threshold priority and
claim complete registers from each core for a given platform. As well as
this, each privilege level has its own context.
for multi-core platform's, we need to be able to enable/ disable a
global interrupt for all the cores that are associated with Zephyr.

To do this, we need to make some assumptions:
1. The privilege contexts are contiguous
2. M mode context is first, followed by S mode.

We know how many cpus are used in an application and each cpu's hartid,
thanks to some very handy inline functions. So we iterate through each
cpu and use the hartid of a cpu in the calculation of the context.

While we are at it, In an effort to make the driver more readable,
allign with the macro naming convention outlined in Linux's PLIC driver

Signed-off-by: Conor Paxton <conor.paxton@microchip.com>
The plic has a very simple mechanism to claim an interrupt as well as to
complete and clear it. The same register is read from/ written to to
achieve this.

Get the ID of the HART that serviced the interrupt and write to the
claim complete register in the correct context
Signed-off-by: Conor Paxton <conor.paxton@microchip.com>
@con-pax
Copy link
Contributor Author

con-pax commented Dec 12, 2023

Hey @ycsin @fkokosinski
full apologies for the spam here: could I ask you guys for re-approval again (hopefully for the last time) so we can get this merged?
Thanks again for all the help on this, really appreciated.

@carlescufi carlescufi merged commit 7e192e9 into zephyrproject-rtos:main Dec 12, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Interrupt Controller area: RISCV RISCV Architecture (32-bit & 64-bit)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants