[multitop] New bazel rules to describe HW #24791

pamaury · 2024-10-15T12:31:20Z

The goal of this experiment is to design new rules to better describe the HW and eventually get rid of our current usage of all_files targets. The idea is that each IP contains a new target created by opentitan_ip where the RTL, doc and hjson files are listed. Then, those files targets are collected by an opentitan_top rule that puts together the entire list of doc, RTL and hjson files. One this is done, we start replace all direct usages of HW files by reference to this new top target where we selectively extra only the files we need. Eventually this should apply to:

all headers (C, rust, multi-top)
fusesoc
documentation

More is possible, for example we could point to testplans, we could separate RTL from DV, etc
This change will eventually be needed for multi-top because collecting all files as we currently do will not scale.

Finally, this PR contains another change: instead of pointing to //hw/ip/... for headers, we now collect everything in a new package //hw/top. There are several reasons for that:

we can start treating all IPs the same way, whether they are in hw/ip or hw/top_/ip/..., autogenerated or not
this moves us closer to a bazel-configuration-based top selection which will be used for multi-top
it solves the problem of IPs in hw/top that have top-specifiied parameters but those are not accessible when generating headers the way we currently do.

Since there are many IPs to convert and many files to patch, the approach in this PR is to create a script to automate most of the process. All commits before [util] Add script to convert... are manual. The rest is created automatically by the script to convert IPs and uses one by one, except for the last commit is manual.

Important remark: this does not duplicate any information that fusesoc has, this is completely orthogonal and only use coarse-grained globs to gather all files.

TODO:

see if we need to handle the node attribute of previous C/rust headers to select the default register node, or if we need to generate several headers for some IPs (one per node)

TODO in follow-up PRs:

cleanup the OTBN python: it's a complete mess, the way data files are handled and the the way it hack sys.path to point to util/ is not great. I couldn't quite get rid of all_files due to that.

This commit was generated by running /home/pamaury/project/opentitan/util/rewrite_hw.py -v -g --root /home/pamaury/project/opentitan --hjson hw/ip/rv_timer/data/rv_timer.hjson --top hw/top_earlgrey hw/ip/rv_timer Signed-off-by: Amaury Pouly <[email protected]>

This commit was generated by running /home/pamaury/project/opentitan/util/rewrite_hw.py -v -g --root /home/pamaury/project/opentitan --hjson hw/ip/spi_device/data/spi_device.hjson --top hw/top_earlgrey hw/ip/spi_device Signed-off-by: Amaury Pouly <[email protected]>

This commit was generated by running /home/pamaury/project/opentitan/util/rewrite_hw.py -v -g --root /home/pamaury/project/opentitan --hjson hw/ip/spi_host/data/spi_host.hjson --top hw/top_earlgrey hw/ip/spi_host Signed-off-by: Amaury Pouly <[email protected]>

This commit was generated by running /home/pamaury/project/opentitan/util/rewrite_hw.py -v -g --root /home/pamaury/project/opentitan --hjson hw/ip/sram_ctrl/data/sram_ctrl.hjson --top hw/top_earlgrey hw/ip/sram_ctrl Signed-off-by: Amaury Pouly <[email protected]>

This commit was generated by running /home/pamaury/project/opentitan/util/rewrite_hw.py -v -g --root /home/pamaury/project/opentitan --hjson hw/ip/sysrst_ctrl/data/sysrst_ctrl.hjson --top hw/top_earlgrey hw/ip/sysrst_ctrl Signed-off-by: Amaury Pouly <[email protected]>

This commit was generated by running /home/pamaury/project/opentitan/util/rewrite_hw.py -v -g --root /home/pamaury/project/opentitan --no-regs --top hw/top_earlgrey hw/ip/tlul Signed-off-by: Amaury Pouly <[email protected]>

This commit was generated by running /home/pamaury/project/opentitan/util/rewrite_hw.py -v -g --root /home/pamaury/project/opentitan --hjson hw/ip/uart/data/uart.hjson --top hw/top_earlgrey hw/ip/uart Signed-off-by: Amaury Pouly <[email protected]>

This commit was generated by running /home/pamaury/project/opentitan/util/rewrite_hw.py -v -g --root /home/pamaury/project/opentitan --hjson hw/ip/usbdev/data/usbdev.hjson --top hw/top_earlgrey hw/ip/usbdev Signed-off-by: Amaury Pouly <[email protected]>

This commit was generated by running /home/pamaury/project/opentitan/util/rewrite_hw.py -v -g --root /home/pamaury/project/opentitan --hjson hw/top_earlgrey/ip/ast/data/ast.hjson --top hw/top_earlgrey hw/top_earlgrey/ip/ast Signed-off-by: Amaury Pouly <[email protected]>

This commit was generated by running /home/pamaury/project/opentitan/util/rewrite_hw.py -v -g --root /home/pamaury/project/opentitan --hjson hw/top_earlgrey/ip/sensor_ctrl/data/sensor_ctrl.hjson --top hw/top_earlgrey hw/top_earlgrey/ip/sensor_ctrl Signed-off-by: Amaury Pouly <[email protected]>

This commit was generated by running /home/pamaury/project/opentitan/util/rewrite_hw.py -v -g --root /home/pamaury/project/opentitan --no-regs --top hw/top_earlgrey hw/top_earlgrey/ip/xbar Signed-off-by: Amaury Pouly <[email protected]>

This commit was generated by running /home/pamaury/project/opentitan/util/rewrite_hw.py -v -g --root /home/pamaury/project/opentitan --no-regs --top hw/top_earlgrey hw/top_earlgrey/ip/xbar_main Signed-off-by: Amaury Pouly <[email protected]>

This commit was generated by running /home/pamaury/project/opentitan/util/rewrite_hw.py -v -g --root /home/pamaury/project/opentitan --no-regs --top hw/top_earlgrey hw/top_earlgrey/ip/xbar_peri Signed-off-by: Amaury Pouly <[email protected]>

This commit switches the HW targets (verilator, bitstream) to use the target create by `opentitan_top`. This allows to remove many of the hacky `all_files` targets used everywhere. This commit does keep a few and even creates some, but tries to adhere to the following convention: only use `glob()` or `all_files` for sub-directories that contain no bazel targets and should be passed "as-is" to fusesoc. Signed-off-by: Amaury Pouly <[email protected]>

matutem · 2024-11-13T08:56:29Z

rules/autogen.bzl

@@ -87,6 +88,99 @@ autogen_hjson_c_header = rule(
    },
 )

+def _opentitan_ip_c_header_impl(ctx):
+    header = ctx.actions.declare_file("{}_regs.h".format(ctx.attr.ip))


Use f-strings: ...._file(f"{ctx.attr.ip}_regs.h")

Unfortuntaley starlark doesn't support f-string

matutem · 2024-11-13T09:10:39Z

sw/device/lib/crypto/drivers/BUILD

@@ -24,7 +24,7 @@ cc_library(
    hdrs = ["aes.h"],
    deps = [
        ":entropy",
-        "//hw/ip/aes/data:aes_c_regs",
+        "//hw/top:aes_c_regs",


Too bad the dep below is top-specific. I think it could be expressed in a top-independent for each driver below. Moreover, the list of units needing drivers could be extracted into a provider, which could help.

I am not entirely sure I understand your comment @matutem . Are you saying that you would prefer that aes_c_regs was not a top-specific dependency?

matutem · 2024-11-13T09:16:36Z

sw/device/lib/dif/BUILD

@@ -37,7 +37,7 @@ cc_library(
    ],
    deps = [
        ":base",
-        "//hw/ip/adc_ctrl/data:adc_ctrl_c_regs",


Same as for lib/crypto/drivers. These are very regular rules for dif, drivers, and silicon_creator/drivers (at least).

matutem · 2024-11-13T09:27:15Z

sw/device/lib/testing/test_rom/BUILD

-        "//hw/ip/otp_ctrl/data:otp_ctrl_c_regs",
-        "//hw/ip/spi_device/data:spi_device_c_regs",
-        "//hw/top_earlgrey/ip_autogen/flash_ctrl:flash_ctrl_c_regs",
+        "//hw/top:flash_ctrl_c_regs",


Can we generate the test_rom_libs in a loop over tops?

The test_rom situation is a bit of mess: the englishbreakfast test_rom is very hacky and doesn't use the same files as earlgrey (e.g. it stubs out some some functions related to non-existent IPs) which makes this a very manual process. I think we will need to revisit how the test_rom is built and try to avoid having one cc_library per top if possible.

matutem · 2024-11-13T09:31:37Z

sw/device/silicon_creator/lib/drivers/BUILD

-        "//hw/top_earlgrey/ip_autogen/pinmux:pinmux_c_regs",
+        "//hw/top:gpio_c_regs",
+        "//hw/top:otp_ctrl_c_regs",
+        "//hw/top:pinmux_c_regs",
        "//hw/top_earlgrey/sw/autogen:top_earlgrey",


Could this top-specific autogen be selected via //hw/top: ?

Yes in the multi-top SW PR we have an alias //hw/top:top_lib for that. It's just not that in that PR.

matutem · 2024-11-13T09:45:55Z

sw/device/tock/BUILD

-        "//hw/ip/sysrst_ctrl/data:sysrst_ctrl_rust_regs",
-        "//hw/ip/uart/data:uart_rust_regs",
-        "//hw/ip/usbdev/data:usbdev_rust_regs",
+        "//hw/top:adc_ctrl_rust_regs",


Could this list of _regs targets be provide by top?

Yes we could create a single target that collects all rust_regs into one in //hw/top/

matutem · 2024-11-13T09:49:59Z

util/rewrite_hw.py

+import os
+import sys
+from pathlib import Path
+


Please add a doc string for this interesting script, so it can tell what it does.

I can add more doc, however this script will disappear once all IPs are converted, it might even be deleted as part of this PR....

matutem · 2024-11-13T09:52:02Z

util/rewrite_hw.py

+    parser.add_argument('ip_dir', help='Path to the IP directory')
+    parser.add_argument('--root', help='Path to the project root (if not specified, assume CWD)')
+    parser.add_argument('-t', '--top', action='append', default=[],
+                        help='Path to the top to edit (optional,c an be specified several times)')


... optional,c an... -> ... optional, can...

matutem · 2024-11-13T09:53:59Z

util/rewrite_hw.py

+        # We may need to go up several level for some ip_templates that don't have
+        # a BUILD file in data/
+        while not hjson_build_path.exists() and \
+                hjson_build_path.parent.parent.is_relative_to(project_root):


Maybe .parent.parent -> parents[2] ?

matutem

Some suggestions so far, and I hope you can figure out how to remove some of the top-specific dependencies and provide list of ips via top providers.

pamaury · 2024-12-10T15:52:25Z

Closed in favor of #25580

pamaury force-pushed the multitop_hw branch from dc03a65 to 0f9c83f Compare October 17, 2024 14:45

pamaury requested review from cfrantz and andreaskurth October 17, 2024 14:47

pamaury force-pushed the multitop_hw branch 4 times, most recently from aa03af3 to 0148575 Compare October 18, 2024 14:37

pamaury force-pushed the multitop_hw branch 9 times, most recently from 2a0ca50 to ad3f459 Compare October 29, 2024 13:44

pamaury requested review from matutem and Razer6 October 29, 2024 14:47

pamaury force-pushed the multitop_hw branch 2 times, most recently from 93278aa to 352da59 Compare October 29, 2024 16:22

pamaury marked this pull request as ready for review October 29, 2024 20:18

pamaury requested review from a team, msfschaffner and vogelpi as code owners October 29, 2024 20:18

pamaury requested review from engdoreis and a-will and removed request for a team October 29, 2024 20:18

pamaury changed the title ~~[experiment][multitop] New bazel rules to describe HW~~ [multitop] New bazel rules to describe HW Oct 30, 2024

pamaury force-pushed the multitop_hw branch from 352da59 to 1e4a704 Compare November 4, 2024 14:50

pamaury added 14 commits November 7, 2024 22:19

[bazel,tlul] Use new rules to describe IP

0315ee7

This commit was generated by running /home/pamaury/project/opentitan/util/rewrite_hw.py -v -g --root /home/pamaury/project/opentitan --no-regs --top hw/top_earlgrey hw/ip/tlul Signed-off-by: Amaury Pouly <[email protected]>

[bazel,xbar] Use new rules to describe IP

a319ba2

This commit was generated by running /home/pamaury/project/opentitan/util/rewrite_hw.py -v -g --root /home/pamaury/project/opentitan --no-regs --top hw/top_earlgrey hw/top_earlgrey/ip/xbar Signed-off-by: Amaury Pouly <[email protected]>

pamaury force-pushed the multitop_hw branch from 09ae04c to b1c63ee Compare November 7, 2024 21:22

This was referenced Nov 8, 2024

[multi-top] HW description rules RFC #25051

Open

[multitop_dev] New bazel rules to describe HW #25090

Merged

matutem reviewed Nov 13, 2024

View reviewed changes

pamaury mentioned this pull request Dec 10, 2024

[multitop] Implement HW rules RFC #25580

Open

pamaury closed this Dec 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[multitop] New bazel rules to describe HW #24791

[multitop] New bazel rules to describe HW #24791

pamaury commented Oct 15, 2024 •

edited

Loading

matutem Nov 13, 2024

pamaury Nov 13, 2024

matutem Nov 13, 2024 •

edited

Loading

pamaury Nov 13, 2024

matutem Nov 13, 2024

matutem Nov 13, 2024

pamaury Nov 13, 2024

matutem Nov 13, 2024

pamaury Nov 13, 2024

matutem Nov 13, 2024

pamaury Nov 13, 2024 •

edited

Loading

matutem Nov 13, 2024

pamaury Nov 13, 2024

matutem Nov 13, 2024

matutem Nov 13, 2024

matutem left a comment

pamaury commented Dec 10, 2024

[multitop] New bazel rules to describe HW #24791

[multitop] New bazel rules to describe HW #24791

Conversation

pamaury commented Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matutem Nov 13, 2024 • edited Loading

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 Nov 13, 2024 • edited Loading

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

matutem left a comment

Choose a reason for hiding this comment

pamaury commented Dec 10, 2024

pamaury commented Oct 15, 2024 •

edited

Loading

matutem Nov 13, 2024 •

edited

Loading

pamaury Nov 13, 2024 •

edited

Loading