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

Add riscv section for RISC-V targets #265

Merged
merged 5 commits into from
Jul 21, 2024

Conversation

romancardenas
Copy link
Contributor

@romancardenas romancardenas commented Jun 29, 2024

This is a proposal to allow svd2pac to generate the corresponding code for riscv targets using the new riscv-pac and riscv-peripheral crates. The main features that we need are:

  1. Identifying which peripherals can use a stardard implementation available at riscv-peripheral (currently, CLINT or PLIC, in the near future, CLIC).
  2. Identifying which interrupt sources can cause iterrupts at core to implement the riscv_pac::CoreInterruptNumber trait. Interrupt sources defined in the peripherals sections should be considered only external, and the riscv_pac::ExternalInterruptNumber should be used for those.
  3. Identifying the available priority levels to implement the riscv_pac::PriorityNumber trait.
  4. Identifying the HART IDs of the microcontroller to implement the riscv_pac::HartIdNumber trait.

To do so, I think the easiest approach is adding a new section in the SVD files that contains all the relevant information. This PR uses the following extension:

<device>
  <riscv>
    <coreInterrupts> <!-- to enumerate sources that cause interrupt AT CORE (riscv_pac::CoreInterruptNumber) -->
      <interrupt>
        <name>MachineSoft</name>
        <description>Machine Software Interrupt</description>
        <value>3</value>
      </interrupt>
      <interrupt>
        <name>MachineTimer</name>
        <description>Machine Timer Interrupt</description>
        <value>7</value>
      </interrupt>
      <interrupt>
        <name>MachineExternal</name>
        <description>Machine External Interrupt</description>
        <value>11</value>
      </interrupt>
    </coreInterrupts>
    <priorities> <!-- to enumerate available priority levels (riscv_pac::PriorityNumber) -->
      <priority>
        <name>P0</name>
        <description>Priority level 0</description>
        <value>0</value>
      </priority >
      < priority >
        <name>P1</name>
        <description>Priority level 1</description>
        <value>1</value>
      </priority >
    </priorities>
    <harts> <!-- to enumerate HART IDs (riscv_pac::HartId) -->
      <hart>
        <name>H0</name>
        <description>HART 0</description>
        <value>0</value>
      </hart >
    </harts >
  </riscv>
  ...
</device>

Related issues and PRs:

Let me know what you think.

@romancardenas romancardenas requested a review from a team as a code owner June 29, 2024 19:21
@Emilgardis
Copy link
Member

I feel like this (or similar) should make it into the spec

svd-encoder/Cargo.toml Outdated Show resolved Hide resolved
@burrbull
Copy link
Member

burrbull commented Jul 1, 2024

I feel like this (or similar) should make it into the spec

I think so too. I'm not sure who controls specification. Possibly those guys: https://github.com/Open-CMSIS-Pack/devtools/
SVD was designed for ARM so I'm not sure how it should looks like.

We could move this under some feature. And mark it as "unstable" or something like this.

@romancardenas
Copy link
Contributor Author

I already have a proof of concept working for the E310x microcontroller. You can check the outcome here.

While implementing this solution, I found a few things that could be changed:

  1. Instead of defining which peripheral is the PLIC, CLINT, etc. in the new riscv section, we can add a new optional field to peripheral, riscvPeripheral. In this way, each special peripheral would announce its special condition in their definition.
  2. Instead of defining core interrupts in the new riscv section, we can add a new optional field to interrupt, riscvCoreInterrupt. This field would default to false, meaning they are external interrupts.

This alternative would look like this:

<peripherals>
  <peripheral>
    <name>PLIC</name> 
    <riscvPeripheral>PLIC</riscvPeripheral>  <!-- Alternative to having it in the riscv section -->
    <baseAddress>0x0C000000</baseAddress>
    <groupName>PLIC</groupName>
    <description>Platform Level Interrupt Control</description>
    <interrupt>
      <name>MachineExternal</name>
      <value>11</value>
      <riscvCoreInterrupt>true</riscvCoreInterrupt> <!-- Alternative to coreInterrupts section -->
    </interrupt>
    <registers>
      <!-- Omitted -->
    </registers>
 </peripheral>
   <!-- Omitted -->
</peripherals>

However, we still need the riscv section to define the priority levels and HART IDs. Furthermore, the new approach "pollutes" standardized fields such as peripherals and interrupts, which may not be desirable.

Feedback is more than welcome. I'm new in this SVD world and probably there are easier ways to do what the RISC-V ecosystem needs.

@romancardenas
Copy link
Contributor Author

We could move this under some feature. And mark it as "unstable" or something like this.

I tried to isolate all new stuff under a non-standard tag riscv so we could manipulate it as we want. I think this special treatment is more a Rust ecosystem issue than a RISC-V ecosystem issue, as we have different traits that must be implemented to access essential parts of the riscv and riscv-rt crates.

I would suggest defining a reasonable riscv section for our purposes and leaving it under an unstable feature gate. After some time working with it, we could try to elevate it to the SVD specification (or at least let them know that we need something extra to make our ecosystem work).

@Emilgardis
Copy link
Member

Alright, I'm ok with experimenting under a feature gate 👍🏼

@romancardenas
Copy link
Contributor Author

I simplified the section. For now, I will just use a naming convention as done for Cortex-M core peripherals, so there is no need for a clic, clint, or plic section.

Also, all the riscv-related code is now under the unstable-riscv feature gate.

@burrbull burrbull added this pull request to the merge queue Jul 21, 2024
Merged via the queue into rust-embedded:master with commit f4376aa Jul 21, 2024
7 checks passed
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.

3 participants