-
Notifications
You must be signed in to change notification settings - Fork 151
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
Use riscv
section for RISC-V targets
#856
Conversation
This PR shows how an updated PAC would look like over e310x chips |
Added the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good start! should probably be marked as a draft, I see a bunch of todos
Yes! I'm currently working on a bunch of changes to use standard peripherals when possible, so it will take a few more iterations until it is ready to merge |
As shown in this PR, now it is possible to use riscv_peripheral :: clint_codegen ! (base 0x2000000 , mtimecmps [mtimecmp0 = (crate :: interrupt :: Hart :: H0 , "[0](crate::interrupt::Hart::H0)")] , msips [msip0 = (crate :: interrupt :: Hart :: H0 , "[0](crate::interrupt::Hart::H0)")] ,);
riscv_peripheral :: plic_codegen ! (base 0xC000000 , ctxs [ctx0 = (crate :: interrupt :: Hart :: H0 , "[0](crate::interrupt::Hart::H0)")] ,); To do list
Regarding the second point: what do you think about providing the CLINT clock frequency via a configuration parameter? |
I started with the configuration parameters... and decided to explore using configuration parameters only, leaving the SVD file untouched. For the E310x chip, the configuration file would look like this. |
Makes sense. But why a TOML? YAML should looks much cleaner for such things. Also what should we do with svd-rs changes and released crates? |
I converted the TOML file to YAML and the following command fails:
From here, it looks like
If we prefer this isolated configuration file approach (which I do), then all the stuff under the |
I think this should not be in |
I addressed your suggestions. I'm not sure if my independent YAML file implementation is the best approach, let me know what you think. You can see how I use it for E310x chips here |
|
@romancardenas still a draft? |
Not really, I don't expect to add any changes. I'm waiting to release the next version of the |
src/config.rs
Outdated
pub core_interrupts: Option<Vec<RiscvEnumItem>>, | ||
pub exceptions: Option<Vec<RiscvEnumItem>>, | ||
pub priorities: Option<Vec<RiscvEnumItem>>, | ||
pub harts: Option<Vec<RiscvEnumItem>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is None
different from Some(Vec::empty)
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, if you define core_interrupts
in your YAML file, svd2rust
will generate a custom CoreInterrupt
struct for your target, regardless of whether it is empty or not. Thus, an empty vector would probably be meaningless. I can change this behavior following any of your design decisions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to say that you can leave just Vec<T>
and check vec.is_empty()
instead of option.is_some
.
Possibly you should also mark a field with serde(default)
. Not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will take a look to your suggestion!
What about this? I'd want to see something like this: /// Common svd2rust options
struct Config {
...
/// Path to YAML file with chip related settings
pub settings: Option<PathBuf>,
}
#[non_exhaustive]
struct Settings {
pub riscv_config: Option<RiscvConfig>,
// other settings (for example #576)
} |
Oh, I see! I added a |
@burrbull Let me know if the new version looks as you expected |
src/generate/device.rs
Outdated
@@ -30,6 +30,14 @@ pub fn render(d: &Device, config: &Config, device_x: &mut String) -> Result<Toke | |||
} | |||
}; | |||
|
|||
let _settings = match config.settings.as_ref() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it underscored?
Cargo.toml
Outdated
@@ -35,13 +35,14 @@ default = ["bin", "json", "yaml"] | |||
bin = ["dep:clap", "dep:env_logger", "serde", "dep:irx-config"] | |||
json = ["dep:serde_json"] | |||
yaml = ["dep:serde_yaml"] | |||
unstable-riscv = ["irx-config/yaml"] | |||
unstable-riscv = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it unstable yet?
Squash commits, please. |
Done |
We use setting.yaml to support riscv 0.12.0 and riscv-rt 0.13.0 This feature also allows RISC-V targets to use standard peripheral implementations with the riscv-peripheral crate.
Please, revert commits from |
This PR uses my proposed
riscv
SVD section to generate RISC-V PACs using the new set of traits inriscv-pac
.In this PR, the
mod interrupt
in RISC-V targets is handled differently, and includes:ExternalInterrupt
s (very similar to currentInterrupt
s)CoreInterrupt
s (this is new, previously it just assumed the peripheral follows the RISC-V standard)Priority
levels (necessary for interfacing standard peripherals with trait bounds)HartId
s (necessary for interfacing standard peripherals with trait bounds).Also, the PAC does not directly generate all the handlers array and so on. Instead, it relies on macros exposed in
riscv-pac
.I still need some time to pollish everything, but feedback is more than welcome :)
Solves #786