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

Plic inc support #133

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Plic inc support #133

wants to merge 3 commits into from

Conversation

disdi
Copy link

@disdi disdi commented Jan 11, 2025

Add support for PLIC interrupt controller as per RISC-V PLIC specs.

Copy link
Contributor

@marnovandermaas marnovandermaas left a comment

Choose a reason for hiding this comment

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

Thank you so much for your contribution. Just a few comments based on a quick review. How have you tested this? What are the area costs of adding the PLIC to the system?

@@ -71,6 +71,10 @@ module ibex_demo_system #(
parameter logic [31:0] SIM_CTRL_START = 32'h20000;
parameter logic [31:0] SIM_CTRL_MASK = ~(SIM_CTRL_SIZE-1);

localparam logic [31:0] PLIC_SIZE = 4 * 1024; // 4 KiB
localparam logic [31:0] PLIC_START = 32'h80005000;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably best to keep the 8000X000 addresses for those that are only 1 KiB in size. Maybe create it at 0x8010000?

Copy link
Author

Choose a reason for hiding this comment

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

I corrected this in latest commit.

.irq_external_i(1'b0),
.irq_fast_i ({14'b0, uart_irq}),
.irq_external_i(irq_external),
.irq_fast_i (1'b0),
Copy link
Contributor

Choose a reason for hiding this comment

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

This should still be 15 bits right?

Copy link
Author

Choose a reason for hiding this comment

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

Corrected in the latest commit.

@@ -494,6 +502,39 @@ module ibex_demo_system #(
assign ndmreset_req = 1'b0;
end

assign irq_sources = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is irq_sources defined somewhere? If so what is its width?

Copy link
Author

Choose a reason for hiding this comment

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

I declared irq_sources now.


assign rdata_o = reg_rdata;

endmodule
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: fix end line

Copy link
Author

Choose a reason for hiding this comment

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

Corrected.

@@ -71,6 +71,10 @@ module ibex_demo_system #(
parameter logic [31:0] SIM_CTRL_START = 32'h20000;
parameter logic [31:0] SIM_CTRL_MASK = ~(SIM_CTRL_SIZE-1);

localparam logic [31:0] PLIC_SIZE = 4 * 1024; // 4 KiB
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this enough? I thought this needed to be 64 MiB?

Copy link
Author

Choose a reason for hiding this comment

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

Corrected.

@disdi
Copy link
Author

disdi commented Jan 18, 2025

How have you tested this?

I am still not able to get UART interrupt working with PLIC. I think my sw driver needs some change.

What are the area costs of adding the PLIC to the system?

LUTs increased from 6686 to 6790, FFs increased from 6279 to 6411.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants