[sw, tock] Extend downstream tock development environment #2087

silvestrst · 2020-04-29T16:40:52Z

This is my take on the downstream Tock development environment and request for comments. Please let me know what you think of this approach.

When it comes to actual PR, some of the commits might be merged or split out further, but for now this should suffice.

For simplicity I have added git hash to external dependencies, otherwise some of our existing board/opentitan code is behind the upstream, and Tock kernel crate API has changed.

I understand that cf6191f might be slightly controversial, however this the cleanest way I found to keep Jon Flatley's "use local repo" change without creating 3x pairs of Cargo_local.toml and Cargo_remote.toml. I quite like this approach as it clearly isolates the external dependencies from our version controlled source. This change touches a lot of files, but the changes are very minimalistic - and let us keep using namespaces through the code as they are upstream.

TODO:

Update .gitignore to include/exclude relevant .lock and .toml files.
I don't think there is any point in ealgrey/source/<uart.rs/gpio.rs> earlgrey should always be instantiated in one way (base addresses, etc...). I think UART0, GPIO0 should be moved to chip.rs or even lib.rs.
There is no reason why plic.rs and rv_timer.rs shouldn't live in peripherals instead of earlgrey. If there is need for some extra wrapping, earlgrey can add wrapping on top of peripherals/plic.rs (Non urgent, subject of further follow-up PRs).
Need to add device specific configuration (conditional compilation), same as in boards/opentitan: Build OpenTitan with target device specific parameters tock/tock#1741
Planning to update the upstream PR, and implement the downstream device config in the same way.
sync up with upstream and update main.rs and other out of date source (Non urgent, subject of further follow-up PRs).
Add Jon Flatley's bindgen change.
Add my linking against DIF libraries change.

P.S. there will be a lot of linter issues, I haven't ran it through clang-format yet.

lenary

Some initial reactions:

I don't think we should be nesting cargo packages like you have with boards/opentitan/earlgrey and boards/opentitan/earlgrey/peripherals (and also boards/opentitan/tock_dependencies). I would welcome comments from @jon-flatley as to what the approach on this is. I think tock itself has tried to avoid this.
How are sw/device/tock/boards/opentitan/src/*.rs built? Don't they need a Cargo.toml?

lenary · 2020-04-29T16:48:00Z

sw/device/tock/boards/opentitan/tock_dependencies/Cargo.lock

@@ -0,0 +1 @@
+/home/svt/development/repos/opentitan/sw/device/tock/boards/opentitan/tock_dependencies/Cargo_remote.lock


You need to update the .gitignore to ignore this file.

I don't think we should be nesting cargo packages like you have with boards/opentitan/earlgrey and boards/opentitan/earlgrey/peripherals (and also boards/opentitan/tock_dependencies).

We don't have to nest them, I am happy to pull it out and put tock_dependencies in tock top directory, tock/chips/earlgrey and tock/chips/<drivers/common_drivers>. However, could you explain your reasoning a bit more? Is it mostly to match the upstream directory layout more closely?

How are sw/device/tock/boards/opentitan/src/*.rs built? Don't they need a Cargo.toml?

They do. It is the Cargo.toml in the boards/opentitan directory. However, I couldn't find it in the committed files, I think it's maybe to do with the fact that the symlink with this name was already in git. I will investigate this.

They do. It is the Cargo.toml in the boards/opentitan directory. However, I couldn't find it in the committed files, I think it's maybe to do with the fact that the symlink with this name was already in git. I will investigate this.

It is because Cargo.tomlused to be a symlink and was in .gitignore. Nice catch, thank you!

lenary · 2020-04-29T16:48:05Z

sw/device/tock/boards/opentitan/tock_dependencies/Cargo.toml

@@ -0,0 +1 @@
+/home/svt/development/repos/opentitan/sw/device/tock/boards/opentitan/tock_dependencies/Cargo_remote.toml


You need to update the .gitignore to ignore this file.

The before this change .gitignore is:

... # Local Tock checkout sw/device/tock/tock_local sw/device/tock/boards/opentitan/Cargo.toml sw/device/tock/boards/opentitan/Cargo.lock sw/device/tock/boards/opentitan/Cargo_local.lock ...

Cargo_remote.toml definitely shouldn't be ignored, not sure if Cargo_remote.lock should be ignored (Jon in his previous change didn't include it in the .gitignore).

Thanks for mentioning this.

Cargo_remote.lock should not be ignored as this is how we pin the build to upstream tock revisions. In general the .gitignore should be updated to reflect the new paths.

silvestrst · 2020-04-30T10:22:33Z

In the latest push:

Modified .gitignore.
Fixed up the last commit in series to include board/opentitan/Cargo.<toml/lock>.

jon-flatley

This is awesome @silvestrst, thank you for putting this together! I have a couple of concerns related to changes in the build setup. Namely, I don't think we should be building Tock by default yet, and I don't think pinning the upstream revision in Cargo.toml makes sense. Modulo that and a few nits it looks great.

jon-flatley · 2020-04-30T14:55:28Z

sw/device/tock/boards/opentitan/Cargo.toml

-ibex = { git = "https://github.com/tock/tock" }
-lowrisc = { git = "https://github.com/tock/tock" }
+earlgrey = { path = "./earlgrey" }
+tock_dependencies = { path = "./tock_dependencies" } 


Rather than carrying the verbose tock_dependencies everywhere consider renaming this to just tock.

I will leave it for today, we can change it later. Just because I am planning to put it in sw/device/tock so sw/device/tock/tock is a bit weird.

What do you think of calling it tock_deps instead?

jon-flatley · 2020-04-30T15:00:33Z

sw/device/tock/boards/opentitan/earlgrey/peripherals/src/lib.rs

+#![feature(asm, concat_idents, const_fn, core_intrinsics)]
+#![feature(in_band_lifetimes)]
+#![feature(exclusive_range_pattern)]


Inconsistent pattern here, can this be made into a single #![feature(...)] or each broken out into distinct attributes?

I think this probably should be done as a follow-up. At this point it has simply been copied from the tock repository of the revision that matches our existing downstream tock board implementation.

jon-flatley · 2020-04-30T15:03:04Z

sw/device/tock/boards/opentitan/earlgrey/src/lib.rs

+#![feature(asm, concat_idents, const_fn, naked_functions)]
+#![feature(in_band_lifetimes)]


Same as above, can these be combined?

Please see the comment above.

jon-flatley · 2020-04-30T15:06:48Z

sw/device/tock/boards/opentitan/tock_dependencies/Cargo.toml

@@ -0,0 +1 @@
+/home/svt/development/repos/opentitan/sw/device/tock/boards/opentitan/tock_dependencies/Cargo_remote.toml


Cargo_remote.lock should not be ignored as this is how we pin the build to upstream tock revisions. In general the .gitignore should be updated to reflect the new paths.

jon-flatley · 2020-04-30T15:10:00Z

sw/device/tock/meson.build

-  build_by_default: false,
+  build_by_default: true,


We shouldn't enable building Rust targets by default until we have toolchain requirements sorted out at the OT project level. This will break the build for those without rustup, including CI builds.

Oh, absolutely. This is just a temporary change for work I am doing. I am not intending to merge this.

jon-flatley · 2020-04-30T15:14:00Z

sw/device/tock/boards/opentitan/tock_dependencies/Cargo_remote.toml

+components = { git = "https://github.com/tock/tock", rev = "d37ef3d2" }
+rv32i = { git = "https://github.com/tock/tock", rev = "d37ef3d2" }
+capsules = { git = "https://github.com/tock/tock", rev = "d37ef3d2"}
+kernel = { git = "https://github.com/tock/tock", rev = "d37ef3d2" }
+tock_rt0 = { git = "https://github.com/tock/tock", rev = "d37ef3d2" }


I don't think we want to specify the revision here, it's handled in Cargo.lock. Eventually we will need CI tooling to advance this revision to upstream ToT, which I believe Cargo can mostly do for us if we use Cargo.lock.

Yup I agree, I am not sure if I use "pre this change" Cargo.lock, whether it will keep the revision that it used to have, I can try it later.

Just to explain why I forced the revision here - it's just to make sure that we use the same revision as before this change for now. As the upstream kernel interface changed slightly, so latest tock version breaks our downstream board.

silvestrst · 2020-04-30T15:58:52Z

@jon-flatley thank you for your review, I will go through your comments more thoroughly tomorrow.

Signed-off-by: Silvestrs Timofejevs <silvestrst@lowrisc.org>

silvestrst · 2020-05-01T15:58:45Z

@lenary reshuffled the directories, could you please have a look?

imphil · 2020-05-01T16:09:58Z

sw/device/tock/boards/opentitan/Cargo.toml

-ibex = { git = "https://github.com/tock/tock" }
-lowrisc = { git = "https://github.com/tock/tock" }
+earlgrey = { path = "../../chips/earlgrey" }
+tock_deps = { path = "../../tock_deps" }


missing newline.

Does TOML need a newline at the EOF as well?

silvestrst · 2020-05-04T11:53:53Z

Updated description of the first commit (forgot to do after last push, and the directories layout has changed).

lenary · 2020-05-04T13:14:32Z

I am happy with the directory structure (mostly I am happy to defer to @jon-flatley and @gkelly on that question, as they're more familiar with Cargo and usual rust conventions than I am).

This change extend existing dowstream development environment. Upstream chip/lowrisc becomes chip/opentitan_common, and chip/ibex becomes chip/earlgrey. NOTE: Because our current boards/opentitan downstream implementation is behind the upstream, I have forced the git has to the current version. Some of the Tock Kernel interfaces have changes, and will need to refactor our main.rs implementation in follow-up chages. Signed-off-by: Silvestrs Timofejevs <silvestrst@lowrisc.org>

* Ensure peripherals, earlgrey, opentitan crates depend on the same version of the packages. * Makes it possible to use JFlat local development change without creating 2 Cargo.toml for every crate (6 in total). Instead only one is needed for tock_dependencies crate. * Keep all the external dependencies in one place. Signed-off-by: Silvestrs Timofejevs <silvestrst@lowrisc.org>

This commit adds to the previous commit, and moves the Cargo_local/remote split from boards/opentitan to boards/opentitan/tock_dependencies. Signed-off-by: Silvestrs Timofejevs <silvestrst@lowrisc.org>

Earlgrey can be run on different devices, which may require different initialisation parameters. This change: * Adds device specific configurations. * Adds infrostructure to choose a configuration. For the available options, see boards/opentitan/Cargo.toml. The option to build for a specific device can be passed through the TOCK_FEATURES environment variable. For example, "TOCK_FEATURES=device_verilator". If TOCK_FEATURES is not specified, tock will be built for fpga by default. Signed-off-by: Silvestrs Timofejevs <silvestrst@lowrisc.org>

silvestrst · 2020-05-04T15:04:54Z

Made it possible to compile for fpga or verilator from command line (through TOCK_FEATURES environment variable).

jon-flatley · 2020-05-04T15:24:29Z

Made it possible to compile for fpga or verilator from command line (through TOCK_FEATURES environment variable).

Do we want to use an environment variable for this or create two separate targets based on https://github.com/lowRISC/opentitan/tree/master/sw/device/lib/arch?

Edit: Not necessarily depending on these headers, but using the same naming patterns, tock_fpga_nexysvideo vs tock_sim_verilator.

Uses the bindgen crate to generate a .rs file to mirrior all headers in sw/device/lib/dif. Signed-off-by: Jon Flatley <jflat@google.com>

lenary · 2020-05-04T15:41:18Z

@jon-flatley that sounds good, but it would be good if we could do that in a follow-up PR, as I don't think it should hold up DIF integration (which is blocked by this PR).

The questions to be answered in this PR are:

Does this directory structure make sense?
I think it does, though there's an open question about calling the intermediate package tock_deps.
Is this the minimum we need to do to start downstream tock development?
I believe it is, but I need someone else to confirm this.

silvestrst · 2020-05-04T15:47:50Z

Do we want to use an environment variable for this or create two separate targets based on

Creating separate targets would be nicer, but that is something that requires more consideration and could be done in future (providing it's not too much effort). For now, I think environment variable is okay (easiest option).

silvestrst · 2020-05-04T15:50:28Z

I am piling commits, but when it comes to non-draft PR, I probably squash these commits in two/three separate PRs.

As @lenary said - the most important thing here is to decide whether the layout is good enough or not. From there we can start moving forward.

jon-flatley · 2020-05-04T15:50:46Z

@jon-flatley that sounds good, but it would be good if we could do that in a follow-up PR, as I don't think it should hold up DIF integration (which is blocked by this PR).

The questions to be answered in this PR are:

Does this directory structure make sense?
I think it does, though there's an open question about calling the intermediate package tock_deps.

Is this the minimum we need to do to start downstream tock development?
I believe it is, but I need someone else to confirm this.

Fair enough, we can move the meson targets around later without too much trouble.

I think the _deps is redundant, if it doesn't cause collisions I prefer to just call the crate tock.

Otherwise LGTM, I think the directory structure makes sense.

silvestrst · 2020-05-05T07:48:15Z

Closed in favour of #2119 , expect other related PRs.

silvestrst requested review from lenary, jon-flatley, mcy, gkelly and alphan April 29, 2020 16:41

lenary reviewed Apr 29, 2020

View reviewed changes

silvestrst force-pushed the silvestrs/tock-downstream-driver-infrastructure branch from a2d2f00 to 6a92cca Compare April 30, 2020 08:02

jon-flatley requested changes Apr 30, 2020

View reviewed changes

enable tock by default -- temporary commit, DO NOT MERGE!

8891fd7

Signed-off-by: Silvestrs Timofejevs <silvestrst@lowrisc.org>

silvestrst force-pushed the silvestrs/tock-downstream-driver-infrastructure branch from 6a92cca to 7069b84 Compare May 1, 2020 15:57

imphil reviewed May 1, 2020

View reviewed changes

silvestrst force-pushed the silvestrs/tock-downstream-driver-infrastructure branch from 7069b84 to 8b94f45 Compare May 4, 2020 11:51

Silvestrs Timofejevs added 2 commits May 4, 2020 14:28

silvestrst force-pushed the silvestrs/tock-downstream-driver-infrastructure branch from 8b94f45 to 3778956 Compare May 4, 2020 13:29

Silvestrs Timofejevs added 2 commits May 4, 2020 14:33

[sw, tock] Allow choosing local dependencies in the new layout

671adb7

This commit adds to the previous commit, and moves the Cargo_local/remote split from boards/opentitan to boards/opentitan/tock_dependencies. Signed-off-by: Silvestrs Timofejevs <silvestrst@lowrisc.org>

silvestrst force-pushed the silvestrs/tock-downstream-driver-infrastructure branch from 3778956 to d7d5857 Compare May 4, 2020 15:01

[sw, tock] Generate Rust bindings to DIF headers

f75e77b

Uses the bindgen crate to generate a .rs file to mirrior all headers in sw/device/lib/dif. Signed-off-by: Jon Flatley <jflat@google.com>

silvestrst mentioned this pull request May 4, 2020

[sw, tock] Extend downstream tock development infrostructure #2119

Merged

silvestrst closed this May 5, 2020

silvestrst deleted the silvestrs/tock-downstream-driver-infrastructure branch November 6, 2020 13:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sw, tock] Extend downstream tock development environment #2087

[sw, tock] Extend downstream tock development environment #2087

silvestrst commented Apr 29, 2020 •

edited

lenary left a comment

lenary Apr 29, 2020

silvestrst Apr 30, 2020 •

edited

silvestrst Apr 30, 2020

lenary Apr 29, 2020

silvestrst Apr 30, 2020 •

edited

silvestrst Apr 30, 2020

jon-flatley Apr 30, 2020

silvestrst commented Apr 30, 2020

jon-flatley left a comment

jon-flatley Apr 30, 2020

silvestrst May 1, 2020 •

edited

jon-flatley Apr 30, 2020

silvestrst May 1, 2020

jon-flatley Apr 30, 2020

silvestrst May 1, 2020

jon-flatley Apr 30, 2020

jon-flatley Apr 30, 2020

silvestrst Apr 30, 2020

jon-flatley Apr 30, 2020

silvestrst Apr 30, 2020

silvestrst commented Apr 30, 2020

silvestrst commented May 1, 2020

imphil May 1, 2020

silvestrst May 4, 2020 •

edited

silvestrst commented May 4, 2020 •

edited

lenary commented May 4, 2020

silvestrst commented May 4, 2020

jon-flatley commented May 4, 2020 •

edited

lenary commented May 4, 2020

silvestrst commented May 4, 2020

silvestrst commented May 4, 2020

jon-flatley commented May 4, 2020

silvestrst commented May 5, 2020

		@@ -0,0 +1 @@
		/home/svt/development/repos/opentitan/sw/device/tock/boards/opentitan/tock_dependencies/Cargo_remote.lock

		#![feature(asm, concat_idents, const_fn, naked_functions)]
		#![feature(in_band_lifetimes)]

		@@ -0,0 +1 @@
		/home/svt/development/repos/opentitan/sw/device/tock/boards/opentitan/tock_dependencies/Cargo_remote.toml

		@@ -0,0 +1 @@
		/home/svt/development/repos/opentitan/sw/device/tock/boards/opentitan/tock_dependencies/Cargo_remote.toml

[sw, tock] Extend downstream tock development environment #2087

[sw, tock] Extend downstream tock development environment #2087

Conversation

silvestrst commented Apr 29, 2020 • edited

lenary left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silvestrst Apr 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silvestrst Apr 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silvestrst commented Apr 30, 2020

jon-flatley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silvestrst May 1, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silvestrst commented Apr 30, 2020

silvestrst commented May 1, 2020

Choose a reason for hiding this comment

silvestrst May 4, 2020 • edited

Choose a reason for hiding this comment

silvestrst commented May 4, 2020 • edited

lenary commented May 4, 2020

silvestrst commented May 4, 2020

jon-flatley commented May 4, 2020 • edited

lenary commented May 4, 2020

silvestrst commented May 4, 2020

silvestrst commented May 4, 2020

jon-flatley commented May 4, 2020

silvestrst commented May 5, 2020

silvestrst commented Apr 29, 2020 •

edited

silvestrst Apr 30, 2020 •

edited

silvestrst Apr 30, 2020 •

edited

silvestrst May 1, 2020 •

edited

silvestrst May 4, 2020 •

edited

silvestrst commented May 4, 2020 •

edited

jon-flatley commented May 4, 2020 •

edited