chips/earlgrey: convert CONFIG to trait, make board-configurable

[lschuermann]

Pull Request Overview

The EarlGrey chip exists in multiple variants, such as the CW310 FPGA instantiation, the Verilator simulation, or eventually the taped out chip. While the "silicon" will be idential in those chips, they can still have slightly different configurations for attributes such as clock speeds.

To drive these configuration flags, instead of using Cargo feature flags and passing them down to the earlgrey chip crate, this changes the chip crate to accept a generic EarlGreyConfig argument wherever necessary. This is a trait containing a number of constants. By implementing this trait on variant-less enums, it is guaranteed to not generate any runtime code.

This change is in the spirit of reducing Tock's reliance on conditional compilation: by plugging in a type-parameter implementing a given trait, all code is always type-checked to adhere to this trait's constraints, regardless of the actual configuration used during the build. Conditional compilation is solely used to switch between two type-aliases which expose implementations of the aforementioned trait.

In practical terms, this allows downstream users to build a customized board crate (where the board logically is also responsible for driving clocks, etc.), without maintaining patches for the chip crate. The chip crate is reduced to code related to the actual silicon. We can use this to work around issues like that of #3639, where a downstream board definition can change its clock speed configuration to be internally consistent, while waiting on a public bitstream to be available for Tock to use.

Testing Strategy

This pull request was tested by compiling.

TODO or Help Wanted

N/A

Documentation Updated

• Updated the relevant files in /docs, or no updates are required.

Formatting

• Ran make prepush.

Ping @cfrantz

[bradjc]

Wow. I'm not sure I will ever see a pull request that has me more conflicted than this one!

This is a good change. We should make it as hard as possible to introduce inconsistencies.

And while I should always be supportive of removing features, I'm a little sad to see these go because they are such a good example of how using features in a very specific, constrained way enables reasonable code reuse while not introducing code readability complexity. But, I'm sure we will get a new example in the future.

[lschuermann]

And while I should always be supportive of removing features, I'm a little sad to see these go because they are such a good example of how using features in a very specific, constrained way enables reasonable code reuse while not introducing code readability complexity.

We do still retain at least one usage of features, namely in the board-level crate:

tock/boards/opentitan/src/main.rs

Lines 56 to 59 in da59131

	#[cfg(feature = "fpga_cw310")]
	type ChipConfig = earlgrey::chip_config::EarlGreyConfigFPGACW310;
	#[cfg(feature = "sim_verilator")]
	type ChipConfig = earlgrey::chip_config::EarlGreyConfigSimVerilator;

They're used similarly in spirit to how the propagated features in the chip crate used to work, only that they're not actually propagated outside of the board crate. So hopefully we're not "losing" too much valuable precedent here. 😄

[bradjc]

So hopefully we're not "losing" too much valuable precedent here.

I actually think it's the opposite, we are potentially gaining precedent we don't want copied. I doubt there are too many cases where the board should be able to configure how a chip is implemented with features. A chip is a chip, a board is a board, the way it operates is baked into silicon/hardware. Features are useful to differentiate versions of the chip, but all of that should be set one time in the dependencies (either via which crates are used or features set in cargo.toml). The idea that configuration of a chip would be dynamic (and would need to be configurable in main.rs) in my mind goes counter to the idea of predictable code.

On one hand, I like the idea of passing board-level parameters to chips via traits and constants as you are doing here. I can imagine a chip supporting different crystals with different frequencies, and a board needing to specify which one is used on a particular hardware board. That is fixed, so it doesn't need to be dynamic at runtime.

So the part I'm less keen on is using one board file for multiple hardware platforms (which is essentially what the cfg is doing). OT is a special case where it doesn't make sense to duplicate main.rs for these couple settings. But in general I don't want approving this PR to equate with me endorsing other board-level changes like this.

[lschuermann]

@bradjc This makes sense to me, and I'm with you on the fact that usually we wouldn't want something like this in other boards.

We could (although I'd argue not in this PR) switch OpenTitan to have multiple board crates and a common shared board lib-crate, much like the nRFs. The terrible symlink of boards/opentitan/earlgrey-cw310/src -> boards/opentitan/src was clearly designed with that in mind. The actual board crates could implement as little functionality as simply calling one function with this type argument passed in.

I will say that the usage of features here, and not anywhere else, also significantly complicates CI & testing infrastructure for arguably little benefit over two board crates.

All of these changes would be independent of the EarlGreyConfig change proposed here, though.

[bradjc]

We could (although I'd argue not in this PR) switch OpenTitan to have multiple board crates and a common shared board lib-crate, much like the nRFs.

I agree in the sense that I do think it would be awesome to figure out a principled way to support things like shields, and therefore any boards which are different, but not by much. But we aren't going to solve that today in this PR.

[bradjc]

Actually, is it possible to leave the features in the chip crate? Can we have the two configs defined in chip_config, and if you want one you set the feature, otherwise if you set no features you get none and then you can define your own?

I think the immediate problem is we want to be able to create more configs without changing the upstream chip crate. I agree with supporting that, but if we don't have to change the feature strategy I would prefer that.

[lschuermann]

Actually, is it possible to leave the features in the chip crate? Can we have the two configs defined in chip_config, and if you want one you set the feature, otherwise if you set no features you get none and then you can define your own?

I personally don't know how we can make that work in even a semi-elegant way. The issue is going to be plumbing the respective config into the generic type argument, selectively from either the chip or the board crate. Here's a really hacky solution to make that work, which I'd rather not see built: (1) keep the enum $CONFIG_NAME {} definitions as is, (2) add a type-alias named SelectedConfig which is driven by #[cfg(_)] attributes, and (3) use that type alias as the default type for the generic type argument across the crate. Then a board can choose not to pass a generic argument, falling back to features, or drive that generic argument to a custom type. This seems incredibly convoluted, and I believe people would have a hard time grasping this.

The fundamental issue we're seeing here, I believe, is that conditional compilation just doesn't integrate well with Rust's language semantics and the type system. Because of it's preprocessor-style nature, you can switch behaviors deep inside the dependency tree without the top-level crates being aware of that. However, as a result, the top-level crate also fundamentally cannot influence these behaviors then (other than through some boolean flags passed to the build system). Generics are much more flexible here, with the tradeoff that such configurations always have to be actively driven by the reverse-dependency. That's much more flexible, at the expense of some ergonomics.

I used to embrace feature-flags, but working with the OT board has convinced me that our hesitance to use them is justified. Even if the current way that these feature-flags are used is limited in how it could break the resulting binary, it still gets in the way during development & CI. The wider ecosystem (i.e. libtock-rs) seems to agree here -- this PR is not too different from how libtock-rs switches between different runtime environments, such as running on Tock or host-based tests.

[bradjc]

I personally don't know how we can make that work in even a semi-elegant way. The issue is going to be plumbing the respective config into the generic type argument, selectively from either the chip or the board crate. Here's a really hacky solution to make that work, which I'd rather not see built: (1) keep the enum $CONFIG_NAME {} definitions as is, (2) add a type-alias named SelectedConfig which is driven by #[cfg(_)] attributes, and (3) use that type alias as the default type for the generic type argument across the crate. Then a board can choose not to pass a generic argument, falling back to features, or drive that generic argument to a custom type. This seems incredibly convoluted, and I believe people would have a hard time grasping this.

Right now the types have the same name, we just keep that. The only change is that rather than the config being automatically compiled in inside of the chip crate, it's "passed in" via a type parameter.

[lschuermann]

Right now the types have the same name, we just keep that. The only change is that rather than the config being automatically compiled in inside of the chip crate, it's "passed in" via a type parameter.

Right. This is what this PR effectively does in its current state, correct? Do you want me to leave as is, or change something? Sorry if I'm not following.

[bradjc]

Right. This is what this PR effectively does in its current state, correct? Do you want me to leave as is, or change something? Sorry if I'm not following.

I'm imaging this PR, but with the feature move reverted and the name in chip_config reverted.

[lschuermann]

I'm imaging this PR, but with the feature move reverted and the name in chip_config reverted.

Okay, got it. I'm not a fan, as the final configuration is then a result of both the chip-crate feature selection, and the board-crate's instantiation of the generic types. I fear with this we're gonna get the worst of both worlds, for the arguable precedent of keeping a "sensible", but actually just obsoleted use of features in the earlgrey chip crate. If this is a compromise which can get this PR over the finish line, I suppose I won't object to implementing it.

[bradjc]

as the final configuration is then a result of both the chip-crate feature selection, and the board-crate's instantiation of the generic types.

Hm, yeah, good point. Ok I agree that isn't a good outcome.

Ok, throwing out one more idea: we remove the configs from the chip crate entirely (just leave the trait). I think what I keep getting stuck on is the confusion: is this a chip config, or a board config? If it's a chip config, then what we have in master seems right. If it's a board config, then why are the config settings in the chip?

For the default peripherals, they are just that: a default (and a pretty reasonable one at that, and one that will always work). I think we could leave an earlgrey default config in chip crate for the same reason. But having two is where things get mixed.

What if we move the configs to the main.rs file, and use the feature to select them there? Then the configuration remains contained to the board, and the precedent this sets is cleaner: this is a good way to pass board-level configs to a chip-level crate.

[lschuermann]

What if we move the configs to the main.rs file, and use the feature to select them there? Then the configuration remains contained to the board, and the precedent this sets is cleaner: this is a good way to pass board-level configs to a chip-level crate.

I really like this approach. I do believe that this is not actually a "chip" config, but a "board" config, in the sense that the board imposes these constraints onto the chip (e.g., by having a certain XO / PLL config). There's no sensible default here we could keep in the chip crate itself that would be universally applicable to all boards. Thus moving it to main.rs seems like it captures this idea best.

[bradjc]

If you were writing this from scratch, would uart match timer? Or is there a reason to pass in the frequency as a variable option?

[bradjc]

How cool would it be if you didn't have to change this line :)

[bradjc]

Sorry this is irrelevant but re: #3589

[bradjc]

You would only have to change it here.

[lschuermann]

If you were writing this from scratch, would uart match timer? Or is there a reason to pass in the frequency as a variable option?

No reason whatsoever and a thorn in my side. Kept it there to keep the diff minimal, but at this point we might as well change it. This really should be only be passed into the UART constructor, and not be part of the chip config at all. We can implement this using a minimal second trait, just to be consistent.

[bradjc]

No reason whatsoever and a thorn in my side. Kept it there to keep the diff minimal, but at this point we might as well change it. This really should be only be passed into the UART constructor, and not be part of the chip config at all. We can implement this using a minimal second trait, just to be consistent.

Oh, I thought you would want it "passed in" as a const parameter.

Well, just ignore that comment, let's get this PR in, and then you can update if you want.