rust-embedded / riscv

Low level access to RISC-V processors
822 stars 160 forks source link

Generic PLIC peripheral #125

Closed romancardenas closed 1 year ago

romancardenas commented 1 year ago

I developed a generic driver for interfacing PLIC peripherals, as mentioned in #124.

References

For developing this peripheral, I got inspired by the following resources:

Particularities

For atomic operations, I use the volatile-register crate (as done in cortex-m).

Unlike the NVIC for Cortex-M microcontrollers, the standard leaves the base address as "implementation-specific". Thus, my implementation uses a const generic to define the base address of the PLIC. In PACs, developers can do something like this:

pub type PLIC = riscv::peripheral::PLIC<BASE_ADDRESS>;

I decided to use associated functions whenever possible, so we don't need to steal/pass the control of the PLIC. However, those functions that are not atomic are defined as methods.

Finally, I followed the safety guidelines of the NVIC implementation, so those associated functions that modify priorities/thresholds are marked as unsafe.

Let me know what you think. If you prefer any other style, I'll happily modify my implementation.

dkhayes117 commented 1 year ago

Thank you for the PR. I'll look it over as soon as I can.

dkhayes117 commented 1 year ago

Not all riscv targets will have a PLIC, should this be feature gated?

dkhayes117 commented 1 year ago

Although, having to supply a PLIC base address would kind of act like a feature gate 🤔

romancardenas commented 1 year ago

I'm fine with both. I plan to develop other peripherals (e.g., CLIC and (A)CLINT) in the same fashion. From what I read, they all have an implementation-specific base address. Using feature gates to only import the peripherals implemented on each PAC sounds reasonable.

romancardenas commented 1 year ago

Maybe we need to reconsider the concept of a context for the PLIC peripheral. These are my thoughts:

So I was thinking that the Context trait may be a bit annoying to maintain. Maybe it would be more useful to have one PLIC per context, and the context number could be a const generic too. Something like PLIC<BASE, CTX>. From what I saw, most boards only implement one context. Thus, their crate would have:

pub type PLIC = riscv::peripheral::PLIC<BASE_ADDRESS, 0>;

If the microcontroller has two contexts, then it would look like this:

pub type PLIC0 = riscv::peripheral::PLIC<BASE_ADDRESS, 0>;
pub type PLIC1 = riscv::peripheral::PLIC<BASE_ADDRESS, 1>;

And each HART can own a context. What do you think?

romancardenas commented 1 year ago

@dkhayes117 I developed an alternative implementation of the PLIC that stores the context as a const generic. I personally like this approach more, because otherwise, I think that dealing with the context number would be non-ergonomic in practice.

I also added a feature gate for optionally adding the peripheral. Let me know what you think!

romancardenas commented 1 year ago

I added additional methods and generics to the current traits. The reason for that is that I am currently working on a port of RTIC for the E310X microcontrollers. As part of this work, I identified nice-to-have features that will ease the integration of RTIC to any platform with PLIC.

romancardenas commented 1 year ago

@dkhayes117 take a look at my latest commit. Basically, I added utility functions to access the MIE and MIP CSRs. I also take advantage of the try_from methods of the traits to avoid returning raw numbers. This is aligned with the PLIC implementation of e310x-hal.

romancardenas commented 1 year ago

Sorry for all these changes, but I've been discovering some deficiencies in my proposal.

Unlike the cortex-m crate, we need to provide a new/default constructor for the peripherals, as they are all optional and PACs are responsible for creating them when needed.

I developed an alternative version of the e310x crate that uses this generic implementation and compiles OK. Take a look to see how this peripheral would work.

I think this PR is now good to go (fingers crossed).

romancardenas commented 1 year ago

I developed an alternative version of the e310x-hal that uses the generic PLIC peripheral. Later today I'll try a toy example on mi Sparkfun RED-V to confirm and illustrate that it works.

romancardenas commented 1 year ago

Yaaay it works :D I'm having some issues with the RTC and debugging, but the PLIC works as expected.

https://github.com/romancardenas/hifive1/blob/master/examples/interrupt.rs

romancardenas commented 1 year ago

I opened a new PR with a clean commit history of this very same implementation