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.

Sign up for GitHub

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

Jump to bottom

[RACL] Add RACL support for spi_device #26044

Merged
merged 2 commits into from
Jan 31, 2025
Merged

[RACL] Add RACL support for spi_device #26044

merged 2 commits into from
Jan 31, 2025

Conversation

davidschrammel
Copy link
Contributor

@davidschrammel davidschrammel commented Jan 28, 2025 •
edited
Loading

This PR enables RACL support for spi_device.

The issues raised in #26004 also affect generated files here.
They are fixed in #26056 and I will rebase this PR on top assuming #26056 will be merged to master before this PR.

@davidschrammel davidschrammel requested review from Razer6 and removed request for msfschaffner January 28, 2025 13:53
rswarbrick
rswarbrick reviewed Jan 29, 2025
View reviewed changes
util/raclgen.py Outdated Show resolved Hide resolved
util/raclgen.py Outdated Show resolved Hide resolved
hw/ip/spi_device/rtl/spi_device.sv
parameter spi_device_pkg::sram_type_e SramType = spi_device_pkg::DefaultSramType,
parameter bit EnableRacl = 1'b0,
parameter bit RaclErrorRsp = 1'b1,
parameter int unsigned RaclPolicySelVec[73] = '{73{0}},
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a comment about this PR, but I suddenly noticed that it's a bit awkward because we have to reflect the number of registers in the parameter type here.

I wonder whether there's a way this could be more automatic, or maybe some associative array (assuming those are allowed for parameters) keyed by the register name that selects policy zero if a register isn't in the array.

Clearly for a follow-up, but I'm interested in your opinion here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a good point. I will try to look into this and see what's possible.

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, there is no constant provided by the corresponding reg_pkg. I guess it would be a fair change to let this package render a constant describing the number of registers?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I agree. I've just opened #26074 to track it (but, honestly, I think it should be pretty easy to do!)

@davidschrammel davidschrammel force-pushed the racl-support-spidev branch 2 times, most recently from f2b57cb to 7860312 Compare January 29, 2025 17:59
@davidschrammel
Copy link
Contributor Author

@rswarbrick I have unified the notes across all template copies.

However, before merging I would still like to rebase this PR on top of #26056 so I can include those fixes as well.

@davidschrammel davidschrammel mentioned this pull request Jan 29, 2025
Merged
@davidschrammel
[racl] Fix RACL parameter names and update template note.
dfff886 
Signed-off-by: David Schrammel <davidschrammel@rivosinc.com>
@davidschrammel
[hw,spi_device,rtl,racl] Enable RACL for spi_device
33b43a4 
Signed-off-by: David Schrammel <davidschrammel@rivosinc.com>
@davidschrammel
Copy link
Contributor Author

Rebased to master and included fixes from #26056.

@rswarbrick
Copy link
Contributor

CHANGE AUTHORIZED: hw/ip/spi_device/data/spi_device.hjson
CHANGE AUTHORIZED: hw/ip/spi_device/rtl/spi_device.sv
CHANGE AUTHORIZED: hw/ip/spi_device/rtl/spi_device_reg_top.sv
CHANGE AUTHORIZED: hw/top_earlgrey/rtl/autogen/top_earlgrey.sv

The change to spi_device.hjson just adds the new ports for RACL signals. The change to spi_device.sv looks nontrivial, but that's just because it's wiring together various racl_error signals (which will all be both zero and unused in the earlgrey context. The change to the register top is just wiring in RACL support, which will have no effect if RACL is disabled. Finally, the change to top_earlgrey.sv is just stubbing the newly added ports.

In all, there is no significant risk to earlgrey from this change.

rswarbrick
rswarbrick approved these changes Jan 30, 2025
View reviewed changes
Copy link
Contributor

@rswarbrick rswarbrick 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 the work here. I'm happy that this is ready to come in!

@vogelpi
Copy link
Contributor

vogelpi commented Jan 31, 2025

CHANGE AUTHORIZED: hw/ip/spi_device/data/spi_device.hjson
CHANGE AUTHORIZED: hw/ip/spi_device/rtl/spi_device.sv
CHANGE AUTHORIZED: hw/ip/spi_device/rtl/spi_device_reg_top.sv
CHANGE AUTHORIZED: hw/top_earlgrey/rtl/autogen/top_earlgrey.sv

As per Rupert's assessment above.

@vogelpi vogelpi merged commit 282dd8d into lowRISC:master Jan 31, 2025
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vogelpi vogelpi vogelpi approved these changes

@rswarbrick rswarbrick rswarbrick approved these changes

@Razer6 Razer6 Awaiting requested review from Razer6

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@davidschrammel @rswarbrick @vogelpi @Razer6