[WIP] Multi-top SW #24453

pamaury · 2024-08-30T14:49:17Z

This is still a WIP, please refrain from commenting too much on the details at this point. High-level comments are welcome.

This is a test PR for the multi-top SW effort. The goal of this PR is to see (1) how much is broken in CI, (2) allow for high-level comments on the approach.

Explanation:

TODO

Important: LTO is not enable in this PR.

How to regenerate everything after making changes to templates:
Run

util/topgen.py -t hw/top_earlgrey/data/top_earlgrey.hjson
util/make_new_dif.py --mode=regen --only=autogen

How to test:
Run

./bazelisk.sh test --test_output=streamed //sw/device/tests:aon_timer_irq_test_fpga_cw310_rom_with_fake_keys //sw/device/tests:sysrst_ctrl_outputs_test_fpga_cw310_sival

Build EB verilator

./bazelisk.sh build --config=english_breakfast  //hw:verilator

Build EB test_rom:

./bazelisk.sh build --config=english_breakfast --config=riscv32 //sw/device/lib/testing/test_rom:test_rom

Run the test_rom_test:

./bazelisk.sh test --test_output=streamed --config=english_breakfast //sw/device/lib/testing/test_rom:test_rom_test_sim_verilator

How to look at the generated code:

For top-level:

# build
./bazelisk.sh build //hw/top_earlgrey/sw/autogen/...
# then query files:
./bazelisk.sh cquery //hw/top_earlgrey/sw/autogen:top_earlgrey_dttool_dt_api
./bazelisk.sh cquery //hw/top_earlgrey/sw/autogen:top_earlgrey_dttool_dt_lib_hdrs
./bazelisk.sh cquery //hw/top_earlgrey/sw/autogen:top_earlgrey_dttool_dt_lib_srcs

For IP:

# build
./bazelisk.sh build //hw/ip/sysrst_ctrl/data:sysrst_ctrl_dt
# then query files:
./bazelisk.sh cquery //hw/ip/sysrst_ctrl/data:sysrst_ctrl_dt

TODOs:

cleanup the templates (they are super messy and contain duplicated code)
redo the LTO work from a clean state, see Prepare for LTO #22052

This is still a WIP, please refrain from commenting too much on the details at this point. High-level comments are welcome.

a-will · 2024-08-30T20:50:33Z

Just a reminder that Co-authored-by is a way to give attribution to code that wasn't only done by you. ;)

pamaury · 2024-08-30T22:17:05Z

Just a reminder that Co-authored-by is a way to give attribution to code that wasn't only done by you. ;)

Sure no worries, I'll add you back :) I just created this PR because several people didn't like looking at code in my branch (the GitHub UI is pretty poor). I have basically just squashed all commits into a few. The original code is here if you want the messy history: master...pamaury:opentitan:dt_for_multitop

util/make_new_dif/templates/dif_autogen.c.tpl

engdoreis · 2024-09-02T09:23:27Z

hw/top_earlgrey/sw/autogen/devicetables.c

+_Static_assert(kDtUartCount == 4, "Number of uart modules mismatch");
+
+const dt_uart_t kDtUart[kDtUartCount] = {
+  // Properties for uart0


Suggested change

// Properties for uart0

[kDtUart0] =

At the moment, there is no enumeration for the indices of each block, ie there is no kDtUart0, there is just kDtUart[0]. Maybe we could create such an enumeration with per-top names but I don't know if that's really useful? The only instance where it could potentially be useful is if we had a top with two instances of a block and those instances are not obvious 0 and 1 but something more meaningful.

hw/top_earlgrey/sw/autogen/devicetables.h

engdoreis · 2024-09-02T09:48:08Z

hw/top_earlgrey/sw/autogen/devicetables.c

+_Static_assert(kDtAdcCtrlIrqTypeCount == 1, "IRQ count mismatch");
+_Static_assert(kDtAdcCtrlCount == 1, "Number of adc_ctrl modules mismatch");
+
+const dt_adc_ctrl_t kDtAdcCtrl[kDtAdcCtrlCount] = {


Have you considered using a unified type for all the peripherals?
This type would have all possible fields, but some would be left empty when it does not make sense for a particular block.
This should be safe as we expect the tests to use the accessor functions to read from the device table, so all the checks will be done by autogenerated code.

I'm not entirely convinced this would be a batter approach, but it would reduce the amount of auto-generated code.
But it may prevent LTO to do a batter work?

Yes, but I think it would have some drawbacks. It's still less type safe unless you had plenty of runtime checks, whereas by using a unique type, you literally can't write wrong code unless you really want to.
Also if you have some arrays (like pins) in the data then you would need either to point to array somewhere (more indirection and the data is not trivially copyable) or make the array of the maximal possible size.

Another advantage of using a unique type for each IP is that in the future, it is possible that some IP may want to embed more information in the DT and having a different type for each one makes that cleaner in my opinion.

hw/top_earlgrey/sw/cw310/dt_clock.c

engdoreis · 2024-09-02T10:06:50Z

sw/device/lib/dif/autogen/dif_adc_ctrl_autogen.c

@@ -25,6 +25,18 @@ dif_result_t dif_adc_ctrl_init(mmio_region_t base_addr,
  return kDifOk;
 }

+OT_WARN_UNUSED_RESULT
+dif_result_t dif_adc_ctrl_init_dt(const dt_adc_ctrl_t *dt,


This function is doing exactly the same as dif_adc_ctrl_init, I wonder if dif_adc_ctrl_init should be removed to avoid confusion.

we could also call this function dif_adc_ctrl_from_dt

We need to keep both because we cannot change all tests are once and we don't want to break existing code. There is a comment in the autogen header:

DEPRECATED This function exists solely for the transition to dt-based DIFs and will be removed in the future.

I think you renaming suggestion is good.

I have renamed it to dif_adc_ctrl_init_from_dt. I think it's important to keep the init to make it clear that this is creating a DIF.

sw/device/tests/aon_timer_irq_test.c

pamaury · 2024-09-05T14:29:17Z

I have made a change to the file structure: now instead of generating some files using reggen and some using topgen, I am generating everything using topgen and putting it in hw/top_earlgrey/sw/autogen. The rationale for this change is that:

it's simpler on both the generator and user side,
it will be very useful for english breakfast where files are generated but not checked in,
it's faster.

pamaury · 2024-09-19T15:02:35Z

I have done a major update of the PR. The main changes are as follows:

The DT headers are now all generated by bazel instead of checked-in
The generation of the devicetables is done by a new tool (dttool), separated from topgen. This is cleaner and avoiding making topgen more complex than it is already is.
I have started added a few items to the build system to make it top-independent (//hw/top).
Started work on english breakfast (not finished)
I have made the commits a lot smaller to explain the various steps.

Currently, topgen generates plic_all_irqs_test and alert_test in sw/device/tests/autogen. But this is incompatible with multi-top so change this behaviour to create a one directory per top in sw/device/test/autogen. Signed-off-by: Amaury Pouly <[email protected]>

This commit contains two changes: - regenerate the top using util/topgen.py -t hw/top_earlgrey/data/top_earlgrey.hjson - manually tell git about the moved files Signed-off-by: Amaury Pouly <[email protected]>

Signed-off-by: Amaury Pouly <[email protected]>

We now treat english_breakfast as a proper top and we check-in the autogenerated files just like earlgrey. All files were generated using util/topgen.py -t hw/top_englishbreakfast/data/top_englishbreakfast.hjson and the new files were checked-in. Signed-off-by: Amaury Pouly <[email protected]>

Signed-off-by: Amaury Pouly <[email protected]>

This commit converts just the necessary files to DT to be able to build the test_rom for english breakfast. Signed-off-by: Amaury Pouly <[email protected]>

The script previously outputed to a dedicated temporary directory and deletes some files before generating them. Now that we are checking files in, we need to change this behaviour. The script now doesn't remote anything and outputs to hw/<top_name>. Signed-off-by: Amaury Pouly <[email protected]>

All files were generated by running: ``` ./util/topgen-fusesoc.py --files-root=. --topname=top_englishbreakfast ``` and checking-in new files. Signed-off-by: Amaury Pouly <[email protected]>

The two tops do not quite use the same flow, in particular they use different flags to specify the fileset. This requires to tweak the fusesoc rule to make it more general. Signed-off-by: Amaury Pouly <[email protected]>

Signed-off-by: Amaury Pouly <[email protected]>

The script assumes that scrambling is always enabled but in fact there is a setting for that and it is used on english breakfast. Signed-off-by: Amaury Pouly <[email protected]>

This setting is not yet wired with the rest of the build system. Signed-off-by: Amaury Pouly <[email protected]>

With the upcoming change to use //hw/top:top to select the top, the per-top copts and feature will not be set on the command-line but instead added when transitioning to build RV targets. Signed-off-by: Amaury Pouly <[email protected]>

All select are now changed to use //hw/top:top Signed-off-by: Amaury Pouly <[email protected]>

To avoid a massive duplication, and as a first step toward a new exec_env-less system, we will move all exec_env to //hw/top so that they work for all tops. The easiest to change is verilator. For now, only convert the test_rom_test to point to this new env. Signed-off-by: Amaury Pouly <[email protected]>

This is enough to be able to run the test_rom_test in verilator Signed-off-by: Amaury Pouly <[email protected]>

pamaury · 2024-10-07T13:14:15Z

Quite a few changes with this push:

more fixes to the ottf so that I can compile the test_rom_test
changes to the build system so that opentitan_test can use EG or EB verilator based on the configuration

It is now possible to run the english breakfast test_rom_test in verilator using:

./bazelisk.sh test --test_output=streamed --config=english_breakfast //sw/device/lib/testing/test_rom:test_rom_test_sim_verilator

Some IP instances have a name, e.g. sram_ctrl_ret_aon and sram_ctrl_main. In some cases, it is important to refer to a specific instance, e.g. `kDtSramCtrl[<index of the ret_aon>]`. This commit generates names so that one can now do `kDtSramCtrl[kDtIndexSramCtrlRetAon]`. Signed-off-by: Amaury Pouly <[email protected]>

Signed-off-by: Amaury Pouly <[email protected]>

hw/ip/adc_ctrl/data/BUILD

matutem · 2024-10-28T16:48:26Z

Hey Amaury, where do you expect the per-top SW files to reside? Not just the parameters, but the actual functions that will be top-specific?

pamaury · 2024-10-29T09:00:29Z

Hey Amaury, where do you expect the per-top SW files to reside? Not just the parameters, but the actual functions that will be top-specific?

Hi @matutem, for now it's all in those targets:

build

./bazelisk.sh build //hw/top_earlgrey/sw/autogen/...

then query files:

./bazelisk.sh cquery //hw/top_earlgrey/sw/autogen:top_earlgrey_dttool_dt_api
./bazelisk.sh cquery //hw/top_earlgrey/sw/autogen:top_earlgrey_dttool_dt_lib_hdrs
./bazelisk.sh cquery //hw/top_earlgrey/sw/autogen:top_earlgrey_dttool_dt_lib_srcs

matutem · 2024-10-30T22:02:16Z

Regarding my last question about generated SW, this is a drawback of not committing generated files: to see the changes I will need to copy the PR, build, and inspect the generated objects: not particularly smooth. And probably more problematic, when generation changes it may be harder to view the differences, let alone see the history of those changes. I'd say that is undesirable.

pamaury · 2024-11-04T14:08:13Z

Regarding my last question about generated SW, this is a drawback of not committing generated files: to see the changes I will need to copy the PR, build, and inspect the generated objects: not particularly smooth. And probably more problematic, when generation changes it may be harder to view the differences, let alone see the history of those changes. I'd say that is undesirable.

I am not sure that we should be checking those files: they are very verbose but most of the code is of no particular interest, also it makes diff very large when doing changes which is not particularly helpful. I believe that what is more useful than the code itself is the documentation so that people can see how to use it. Unfortunately, our documentation system currently cannot handle generated file but this is something that we should fix. Note that we are also not checking in the repo the IP register header and we have the same problem with the documentation.

engdoreis reviewed Sep 2, 2024

View reviewed changes

util/make_new_dif/templates/dif_autogen.c.tpl Outdated Show resolved Hide resolved

engdoreis reviewed Sep 2, 2024

View reviewed changes

hw/top_earlgrey/sw/autogen/devicetables.h Outdated Show resolved Hide resolved

engdoreis reviewed Sep 2, 2024

View reviewed changes

hw/top_earlgrey/sw/cw310/dt_clock.c Outdated Show resolved Hide resolved

engdoreis reviewed Sep 2, 2024

View reviewed changes

sw/device/tests/aon_timer_irq_test.c Outdated Show resolved Hide resolved

pamaury force-pushed the dt_for_multitop_dev branch 5 times, most recently from d712150 to eea83bc Compare September 3, 2024 09:11

engdoreis reviewed Sep 3, 2024

View reviewed changes

sw/device/tests/aon_timer_irq_test.c Outdated Show resolved Hide resolved

pamaury force-pushed the dt_for_multitop_dev branch 6 times, most recently from bea7704 to 8cc0b87 Compare September 5, 2024 14:16

andreaskurth requested a review from sameo September 9, 2024 09:10

pamaury force-pushed the dt_for_multitop_dev branch 3 times, most recently from 1f74d32 to f4fbe56 Compare September 19, 2024 15:00

pamaury force-pushed the dt_for_multitop_dev branch 3 times, most recently from a76d5fe to 2eb720d Compare September 24, 2024 12:53

pamaury added 13 commits September 30, 2024 13:41

Regenerate top_earlgrey

47675dd

This commit contains two changes: - regenerate the top using util/topgen.py -t hw/top_earlgrey/data/top_earlgrey.hjson - manually tell git about the moved files Signed-off-by: Amaury Pouly <[email protected]>

[gitignore] Remove ignore lines for english_breakfast

f37a75a

Signed-off-by: Amaury Pouly <[email protected]>

[english_breakfast] Add BUILD file in data/ for consistency

6b2ce51

Signed-off-by: Amaury Pouly <[email protected]>

[english_breakfast] Create the various top-level BUILD files

149efbb

Signed-off-by: Amaury Pouly <[email protected]>

[hw/top] Add support for english_breakfast

b4177fb

Signed-off-by: Amaury Pouly <[email protected]>

[dif,pinmux] Add helper function to connect an MIO DT pin/pad.

a3e658c

Signed-off-by: Amaury Pouly <[email protected]>

[test_rom] Make test_rom buildable for english breakfast.

4f03305

This commit converts just the necessary files to DT to be able to build the test_rom for english breakfast. Signed-off-by: Amaury Pouly <[email protected]>

Regenerate englishbreakfast

c012fca

All files were generated by running: ``` ./util/topgen-fusesoc.py --files-root=. --topname=top_englishbreakfast ``` and checking-in new files. Signed-off-by: Amaury Pouly <[email protected]>

[topgen] Fix flake warning

6b0cf3a

Signed-off-by: Amaury Pouly <[email protected]>

pamaury force-pushed the dt_for_multitop_dev branch from 86fedd8 to 6b0cf3a Compare September 30, 2024 13:41

pamaury added 6 commits October 7, 2024 13:01

[rom_ctrl,util] Fix scrambling script to handle english breakfast

ca5db86

The script assumes that scrambling is always enabled but in fact there is a setting for that and it is used on english breakfast. Signed-off-by: Amaury Pouly <[email protected]>

[hw/top] Add new config setting to select the top

731f908

This setting is not yet wired with the rest of the build system. Signed-off-by: Amaury Pouly <[email protected]>

[english breakfast] Remove //sw/device:is_english_breakfast

13bf78a

All select are now changed to use //hw/top:top Signed-off-by: Amaury Pouly <[email protected]>

[test_framework] Partially port the test framework to the DT

3655b9e

This is enough to be able to run the test_rom_test in verilator Signed-off-by: Amaury Pouly <[email protected]>

pamaury added 2 commits October 8, 2024 14:23

[ottf] Use named constant to access sram_ctrl_main

21ab9f5

Signed-off-by: Amaury Pouly <[email protected]>

matutem reviewed Oct 28, 2024

View reviewed changes

hw/ip/adc_ctrl/data/BUILD Show resolved Hide resolved

This was referenced Nov 8, 2024

[englishbreakfast] Documentation #25041

Open

[multi-top] SW RFC #25050

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Multi-top SW #24453

[WIP] Multi-top SW #24453

pamaury commented Aug 30, 2024 •

edited

Loading

a-will commented Aug 30, 2024

pamaury commented Aug 30, 2024

engdoreis Sep 2, 2024

pamaury Sep 2, 2024

engdoreis Sep 2, 2024

pamaury Sep 2, 2024

engdoreis Sep 2, 2024

engdoreis Sep 2, 2024

pamaury Sep 2, 2024

pamaury Sep 2, 2024

pamaury commented Sep 5, 2024 •

edited

Loading

pamaury commented Sep 19, 2024

pamaury commented Oct 7, 2024

matutem commented Oct 28, 2024

pamaury commented Oct 29, 2024

matutem commented Oct 30, 2024

pamaury commented Nov 4, 2024

[WIP] Multi-top SW #24453

Are you sure you want to change the base?

[WIP] Multi-top SW #24453

Conversation

pamaury commented Aug 30, 2024 • edited Loading

a-will commented Aug 30, 2024

pamaury commented Aug 30, 2024

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

pamaury commented Sep 5, 2024 • edited Loading

pamaury commented Sep 19, 2024

pamaury commented Oct 7, 2024

matutem commented Oct 28, 2024

pamaury commented Oct 29, 2024

build

then query files:

matutem commented Oct 30, 2024

pamaury commented Nov 4, 2024

pamaury commented Aug 30, 2024 •

edited

Loading

pamaury commented Sep 5, 2024 •

edited

Loading