boards/opentitan: Build OpenTitan with target device specific parameters

[silvestrst]

Pull Request Overview

I think it is useful to be able to pass options to cargo from the board specific makefiles. This PR does it in a controlled way, by not getting the CARGO_FLAGS from the environment, but modified from within the board makefiles.

Rust features add some flexibility. In this case they serve as simple #[cfg(feature = "sim_verilator")], but could be used to pull in optional dependencies.

This pull request consists of three changes:

• Opentitan makefile change to pass features to Cargo.
• Creates an infrastructure in the opentitan chain (boards/opentitan, chips/earlgrey) to pass requested features. It also adds a configuration file, which based on the requested feature initialises a structure with the device specific information.
• Uses the device specific data instead of the hardcoded values.

Testing Strategy

This pull request was tested by running on the verilator with environment variable BOARD_CONFIGURATION=sim_verilator, and observing the expected UART output. When built for the fpga, and ran on verilator, no output was observed (expected).

TODO or Help Wanted

• Test on the FPGA or verilator.

Documentation Updated

No updates are required?

Formatting

• Ran make formatall.

[bradjc]

I'm not sure if the values for the different configuration constants should come from the top (i.e. from a verilator.config file that lives in the board repo) or be set in the chip crate (as they currently are in this PR). Does it make sense for users to be able to easily customize them, or is the number of options reasonably fixed?

In the case where the configuration settings are in the chip crate, I think they should be in their own file (like config.rs in the kernel crate) where we can specify what the different versions are and what the different settings are.

[bradjc]

Why is this variable? Why would different board versions want to use different baud rates? Shouldn't this be set to 115200 like the other boards?

[silvestrst]

I think the idea was to have fast baud rate for the FPGA, and then the standard rate for the verialtor.

I guess we probably could use 115200, but for consistency with OT I wanted to have the same values. I think it could be a good idea to make this more flexible for users to set up, along with the target device frequency.

[bradjc]

This type of configuration makes using other cargo tools (like clippy for example) difficult, because things outside of the makefile flow end up with no CHIP_FREQ set, which makes rust complain.

I'm generally very opposed to cfgs as their use makes embedded code bases very difficult to comprehend. At least in this use determining which to use is at least based on hardware, which makes tracking down the correct setting somewhat reasonable, even for a new user. However, there are still weaknesses:

1. Someone looking at this file has to reason about how target_device is set. In other cases it comes from a Cargo.toml file, but now it also could be from a makefile flag. This adds complexity, which is particularly difficult for new users.
2. cfgs in general don't seem to have a good way to document them. What is a target_device? And what is nexysvideo/verliator? What are all of the choices for what is a valid target_device?

[silvestrst]

I agree. I was thinking to make a split something along the lines of what boards/nordic and others do. However it is slightly awkward as nexysvideo and verilator is the same Opentitan RTL. That also brings me to a separate question, whether in chips split into lowrisc and ibex is right, or should be done somewhat differently?

The purpose of this PR was to test out the waters, and get some advice. Thanks.

[lschuermann]

because things outside of the makefile flow end up with no CHIP_FREQ set

I think this is the primary issue: statements are not being generated. I think this problem can be circumvented or at least reduced by resorting to the cfg!(...) macro. I guess using something like this in the code should work:

let chip_freq =
    if cfg!(target_device = "verilator") {
        500_000
    } else {
        other_freq
    };

As every if is an expression, this forces you to provide a return value in every case and shouldn't be able to cause compile errors (although you can of course deliberately panic or abort compilation in a branch). Even within runtime code, this would be inlined as to have no performance impact.

[alistair23]

Maybe it would be better to just make the clock speed settable for all targets?

When running the HiFive1 executables on QEMU there are issues with clock speed as well. Some way to override the default from the command line would be useful and would apply to all platforms instead of just OT.

[silvestrst]

I agree with @alistair23 it seems like a useful feature overall, however, maybe it is worth instead of a single value be able to supply more data? We could have a config file/files under board or chip directories, and during the build point Cargo, or build.rs, or some other means to one of them? There could be a default config that is built, if nothing is specified or something similar.

At the moment it is thinking out loud, does what I said above make sense? :)

[silvestrst]

I have completely refactored my change, now it uses features, which I think is probably most appropriate approach for this kind of change.

What I have noticed in Makefile.common the build line is getting very long. If this PR gets a green light, I can split it up in multiple lines in a follow-up commit.

[silvestrst]

Could someone please help me out with the error travis is getting:
error[E0432]: unresolved import lowrisc::device_config::DEVICE_CONFIG

Local build seems to work without any issues.

[jon-flatley]

Could someone please help me out with the error travis is getting:
error[E0432]: unresolved import lowrisc::device_config::DEVICE_CONFIG

Local build seems to work without any issues.

I'm not sure what exactly travis is doing, but doing a cargo build in chips/ibex produces the same error. I've never used features before, so I'm not sure what is standard, but since one of the two features device_fpga or device_verilator is required would it make sense to move this under a single feature?

Something like

#[cfg(not(feature = "device_verilator"))]
pub const DEVICE_CONFIG: DeviceConfig = DeviceConfig {
    name: &"device_fpga",
    chip_freq: 50_000_000,
    uart_baudrate: 230400,
};

#[cfg(feature = "device_verilator")]
pub const DEVICE_CONFIG: DeviceConfig = DeviceConfig {
    name: &"device_verilator",
    chip_freq: 500_000,
    uart_baudrate: 9600,
};

This allows cargo build in chips/ibex to run successfully.

[silvestrst]

I see, thank you @jon-flatley , my bad - I was only looking at this from the perspective of build being triggered through boards/opentitan, but chips obviously can be built separately, and chip is not exclusive to a single board. Travis is building it separately, and because the default option is in the boards/opentitan .toml, none of the structures in the device_config.rs are compiled.

Your suggestion makes sense, but I think I will refactor my change a little to (probably) move the default into chips/lowrisc/Cargo.toml.

[alistair23]

Overall this looks fine to me, it seems pretty clear but still flexible.

[ppannuto]

I'm afraid this is going to conflict with #1771, which I think is a bit more general. Can this build on top of #1771 (or should 1771 build on this)?

[vsukhoml]

I'm afraid this is going to conflict with #1771, which I think is a bit more general. Can this build on top of #1771 (or should 1771 build on this)?

i think difference is primarily in a way CARGO_FLAGS or CARGO_OPTIONS are constructed. CARGO_FLAGS allows redefinition of all flags, while CARGO_OPTIONS relies on some defaults.

I can add CARGO_OPTIONS to #1771 CARGO_FLAGS.

[ppannuto]

@silvestrst Are you okay waiting for #1771 to merge (after 1.5 release) and then rebasing this on that?

[silvestrst]

@ppannuto no problem, this PR is not urgent. Thank you for letting me know.

[silvestrst]

It seems that there are fair amount of people asking for extending cargo features capabilities, so maybe at some point in future there will be a better way of going about this change. At the moment however, I think this is prorbably a reasonable way to go.

In the latest push:

• I have updated device_config.rs and chips/lowrisc/Cargo.toml. cargo features are a bit awkward, I have considered to skip boards/opentitan/Cargo.toml entirely and pass features directly to the lowrisc crate in CARGO_OPTIONS. Unfortunately there is no way to disable the default features for dependencies (can be only done for the top package when specifying through cargo build).

My approach is to specify additional feature in lowrisc, and make any explicitly chosen feature to depend on it. That way if there are no explicitly chosen features, we can pick that up and process. In this case when there are no features explicitly requested, we use device_fpga (can be easily changed).

This is more or less what @jon-flatley suggested, but without the need to explicitly check all of the features to figure out whether none of them are requested.

[bradjc]

I think this is pretty reasonable, but I find cargo features to be terribly confusing. Is this really the only way to have a default option?

[silvestrst]

I think this is pretty reasonable, but I find cargo features to be terribly confusing. Is this really the only way to have a default option?

[default] is always enabled, unless specifically requested to be disabled with --no-default-features, unfortunately you can't tell it to "ignore when any other features are explicitly requested".

The problem is that I couldn't find any way to pass --no-default-features to the board/opentitan dependencies. It can be done for the top Cargo.toml, but that won't propagate to the dependencies.

It would be best to not have a [default] and explicitly fail when no device is specified, but that would require changes to CI.

Do you find how features are used here confusing in general, or just the device_specified part?

[silvestrst]

I think this is pretty reasonable, but I find cargo features to be terribly confusing. Is this really the only way to have a default option?

I agree it's not the best, the alternative would be to obtain these values runtime like @lschuermann suggested. The problem with that is then you wouldn't be able to simply do this:
pub const UART0_BAUDRATE: u32 = CONFIG.uart_baudrate;

Meanwhile will push what I have got now.

[silvestrst]

It seems like we want one feature ibex_hardware_configuration that can take multiple options. Is that possible?

I don't think that is possible. Cargo features don't take any additional parameters, and in general are very rigid. There is fair amount of discussion around different ways of improving Cargo "features" feature, however most of these threads and issues are stale. To name few:
https://internals.rust-lang.org/t/mutually-exclusive-feature-flags/8601
rust-lang/cargo#2980
...

To be honest I still think that Ibex is not an appropriate name for the chip level crate, which in my opinion should be earlgrey (currently the only OpenTitan chip design). Ibex is RISCV compliant, and I am not sure in that case why there is a need for separate crate.

With this change Ibex is too closely coupled with OpenTitan board.

An alternative could be to change peripheral instantiation from compile time to runtime, and also passing configuration into Ibex during board initialisation.

[silvestrst]

I guess this PR is now blocked on #1997. I think when const context control flow Rust upstream change propagates in the nightly toolchain, this change probably could be made more elegant.

[phil-levis]

Looks good.

[hudson-ayers]

This looks good. It would be nice to eventually have a make verilator target that runs the kernel in verilator in the same way make qemu runs the kernel in qemu. But that would require documentation in Tock on how users can get verilator working, so I it probably deserves to be a separate PR.

[bradjc]

I added some more documentation, and removed the explicit default configuration copy.

I also added the configuration name to the debug printout.

[silvestrst]

@bradjc thank you for polishing this change, especially the documentation changes!

[silvestrst]

Thank you guys for your help with pushing this change over the hump!

[bradjc]

@ppannuto

[bradjc]

I think Pat's request was for 1.5, which is long in the past.

Related to 1.5 which is long since been released.

[bradjc]

bors r+

[bors]

Build succeeded:

• ci-format (macos-latest)
• ci-qemu
• ci-build (macos-latest)
• ci-tests (ubuntu-latest)
• ci-build (ubuntu-latest)
• ci-format (ubuntu-latest)
• ci-tests (macos-latest)