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

Move firmware to common directory #82

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

AdamWRichardson
Copy link

@AdamWRichardson AdamWRichardson commented Feb 22, 2022

This is a large set of changes which moves the firmware into a single place. Motivation can be found in the read me which I wrote as part of these changes.

Eventually the firmware directory can be made it's own repo and "submoduled" in this repo but this is the first step to that move

TODO in this PR:

  • Fiddle with the upper level makefiles to make sure each test can be run from the root of the directory
  • Delete old firmware files
  • Clarify configuration of riscv toolchain in dockerfile

TODO out of scope of this PR:

  • Make the firmware directory a separate repo
  • Add the firmware repo as a submodule to this repo
  • Clean the include tree and make proper libraries of each source and header
  • Simplify the CRT0
  • Move from C to C++

@kareefardi
Copy link
Contributor

Hey @AdamWRichardson. First, thank you for your contribution. It is always nice to see the merits of the open-source world. I can see that the main motivation here is to isolate the firmware and test benches. There were some attempts to do that here. And, currently caravel_user_project references the firmware inside litex repo. As usual there is room for improvement but perhaps you weren't aware of that structure. Nonetheless, can I ask you to highlight the benefit of what you are suggesting vs the current direction used in the litex repo.

@AdamWRichardson
Copy link
Author

AdamWRichardson commented Feb 24, 2022

Hey @AdamWRichardson. First, thank you for your contribution. It is always nice to see the merits of the open-source world. I can see that the main motivation here is to isolate the firmware and test benches. There were some attempts to do that here. And, currently caravel_user_project references the firmware inside litex repo. As usual there is room for improvement but perhaps you weren't aware of that structure. Nonetheless, can I ask you to highlight the benefit of what you are suggesting vs the current direction used in the litex repo.

Hi @kareefardi thanks for your reply, I wasn't aware of this structure but I see several advantages to this approach rather than the approach in the litex repo:

  • As mentioned in the description and readme the firmware should be separated out into it's own repo to allow independent development of firmware and design. This means the firmware repo only contains things required to build the firmware and no RTL or verilog testbenches (as there are in the litex repo).
  • The build system here is much simplified with clearer dependencies and directory structure.
  • A minor point but actually quite important is that we use CMake for the firmware. Since firmware is software it can take advantage of developments in the software world such as better build systems, compilers, static analysis tools and unit tests without bleeding these things into design repos.
  • It's easier for firmware developers that are developing on the caravel_user_project to copy the structure of the repo to aid their own firmware development by making forks of the firmware repo.
  • It's generally easier for the open source community to contribute to and extend the firmware repo. Libraries for C and C++ can be added very simply.

Can I ask how the caravel_user_project gets the firmware from the litex as it's not a submodule? I know @jeffdi said there's been some problems with versioning of submodules before, could you explain the problems because submodules can be really effective.

@AdamWRichardson
Copy link
Author

Hi again @kareefardi is there any update on this? Would you like me to close this pull request or something else?

@kareefardi
Copy link
Contributor

Hey @AdamWRichardson. Sorry, I have been quite busy and didn't have the time to review this. I did not actually catch your edit above. I will take a look and answer your questions. Meanwhile let's keep it open. I can see merits to adding this.

I skimmed through your fork and I do have one question, perhaps a silly since I didn't look thoroughly: I noticed that the simulation testbench files themselves (.v) still do exist under verilog/dv/ and then the makefile inside verilog/dv calls cmake. But, for instance, wb_port.c and wb_port.tb are somewhat coupled. Isn't that a bit confusing? Also, how would it work if we are to separate the firmware to separate repo?

One more thing, please keep in mind that if we merged this, testbenches in litex repo should be also consistent with this.

@AdamWRichardson
Copy link
Author

Hey @AdamWRichardson. Sorry, I have been quite busy and didn't have the time to review this. I did not actually catch your edit above. I will take a look and answer your questions. Meanwhile let's keep it open. I can see merits to adding this.

I skimmed through your fork and I do have one question, perhaps a silly since I didn't look thoroughly: I noticed that the simulation testbench files themselves (.v) still do exist under verilog/dv/ and then the makefile inside verilog/dv calls cmake. But, for instance, wb_port.c and wb_port.tb are somewhat coupled. Isn't that a bit confusing? Also, how would it work if we are to separate the firmware to separate repo?

One more thing, please keep in mind that if we merged this, testbenches in litex repo should be also consistent with this.

No worries, there's no rush it was just friendly nudge 👍

One of the main motivations behind this change is to allow the firmware to be developed independently of the RTL, including the testbenches. For now with this merge the two would still be in step and are closely coupled, but maybe in the future they can be decoupled more easily. One way of doing this is to have only 1 testbench that simulates everything and the firmware decides whether the test has passed or not using a standardised method (e.g. writing a particular string to a particular peripheral). In my experience this has been the more common approach to firmware testing but initially they would remain coupled.

There wouldn't necessarily be any change if the firmware is in a separate repo because it will be submoduled by all repos that need it, including this one. Then the firmware could still be visible and built from wherever is reasonable to submodule it (I'm thinking of submoduling at the root of the repo but maybe there's a better place for it).

@marwaneltoukhy
Copy link
Member

Hi @AdamWRichardson we really appreciate your contribution and we're looking into ways to merge your contribution, but please put in mind that this is a huge change that would need time to study.
Some thoughts:

  • First of all, there's a movement of having all the makefiles unified into 1 and including it in all makefiles inside testbenches, so that when there's a change to the makefile, we'd only need to change 1 makefile
  • What @kareefardi was saying about the appropriate changes in litex is that this needs to be used by litex as well for simulation of mgmt_core testbenches. In my point of view this shouldn't even be in the caravel_user_project repo, as you said it's probably better to have it in a seperate repo
  • Regarding your question about how it's getting the firmware from litex, we actually clone the litex repo in the make setup target which gets all the necessary firmware from there

Questions/Requests:

  • Have you tried to run litex simulation using this firmware?
  • Could you rebase and run the new CI on this PR?
  • Are there any new dependencies?
  • Although the current structure basically does the same thing as your initial idea, which is unifying the firmware (which is currently in litex), I can see the benifit of moving it out of litex into a standalone repo. But I still can't see the difference in the directory structure. Perhaps you could take a look here
  • What does it take to add a new testbench?

Thank you for your contribution and your quick response to questions.

@AdamWRichardson
Copy link
Author

Hi @AdamWRichardson we really appreciate your contribution and we're looking into ways to merge your contribution, but please put in mind that this is a huge change that would need time to study. Some thoughts:

Hi @marwaneltoukhy thanks for the reply and again there's no rush at all, happy to provide help and knowledge where I can 😄

* Regarding your question about how it's getting the firmware from litex, we actually clone the litex repo in the `make setup` target which gets all the necessary firmware from there

Is there a reason you're not submoduling the litex repo? I'm probably missing something here.

  • Have you tried to run litex simulation using this firmware?

I have not, could you point me to the makefiles/directories for running a litex sim? This PR is very much an initial idea and there's probably still work to be done to get everything working as expected, but happy to attempt if I can.

  • Could you rebase and run the new CI on this PR?

I will do a merge now.

  • Are there any new dependencies?

The only extra things required are cmake which can either be installed using pip or the linux distro's package manager (although this is ususally quite an old version).

  • Although the current structure basically does the same thing as your initial idea, which is unifying the firmware (which is currently in litex), I can see the benifit of moving it out of litex into a standalone repo. But I still can't see the difference in the directory structure. Perhaps you could take a look here

The directory structure I've used is a fairly standard one for C/C++ projects. Although it is also similar to RTL as it separates the libraries (includes) from the implementation (src). Is this what you mean by difference in the directory structure?

  • What does it take to add a new testbench?

For now you would add the _tb.v files under a new directory in the caravel_user_project/verilog/dv and then there would have to be a testcase in the firmware directory that matches that testbench. Then after the firmware has been built the makefile would copy the .hex file to the directory with the _tb.v file so it can be loaded by the testbench. You could just copy and paste the existing directories such as io_ports or wb_port and just change the names.
Going forward you can see my reply to @kareefardi about changing the way the testbenches are designed so that there's only one testbench overall. But this is another rather large change which is out of scope for this PR

Hope this helps!

# Conflicts:
#	Makefile
#	caravel_firmware/src/testcases/la_test1/main.c
@RTimothyEdwards RTimothyEdwards added enhancement New feature or request flow Changes in Makefile and process flow labels Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request flow Changes in Makefile and process flow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants