[tools,topgen] Enhance and simplify topgen #25933

matutem · 2025-01-17T21:25:40Z

This is a big change in topgen. Some salient changes:

Regenerates from scratch the ipgen in-memory configurations (as IpBlock objects),
so it is truly incremental: the previous flow reused the pre-existing generated ipgen
hjson if found.
Delays the actual ipgen file generation until the complete top config is generated.
This is a step towards breaking topgen into smaller and mutually independent steps,
thus enabling parallelizing them, and creating smaller and simpler tools to tackle each.
It converges with a single pass of _process_top.

In order to accomplish this some important changes are made:

Call some of the merge_top functions suitable for each specific ipgen prior to
generating the in-memory hjson config. This means not all IpBlock objects are
available, so these merge functions are instrumented to skip blocks missing.
The generation of ipgens are ordered according to dependencies. This order created
from the expected block functionality, so is not derived from a constructed graph, but
converging in one pass means it is correct.
Calling merge functions early means some of the objects in the in-memory top config
are not plain dictionaries but are classes, so the code needs to handle either.

Part of #25920

matutem · 2025-01-29T23:17:41Z

CHANGE AUTHORIZED: hw/top_earlgrey/data/top_earlgrey.hjson

a-will

These are probably mostly useless comments and nits. I still have a lot more to look at. 😄

a-will · 2025-01-30T01:30:59Z

util/topgen/merge.py

+    if not isinstance(top['clocks'], Clocks):
+        top['clocks'] = Clocks(top['clocks'])


It's a wee bit funky (a non-sequitur) that we modify the top argument here, in a function named extract_clocks(). Is the promotion to a class unable to happen until a specific point in the flow, and this happened to be a convenient place?

It's similar for amend_resets() below.

This behavior was not changed in this PR, rather this quirky behavior was exposed because we run merge operations more often. I agree it would be nice to keep the consisting of json objects only.

a-will · 2025-02-01T01:36:46Z

util/topgen/validate.py

+reset_requests_optional = {
+    'int': ['s', 'internal request list'],
+    'debug': ['s', 'debug request list'],
+    'peripheral': ['s', 'peripheral request list'],
+}


This seems like an arbitrary breakdown. What's the difference between these three categories? Is there any different handling between "int" resets and "debug" resets?

This is mostly a documentation nudge. 😄

Good point: I extended the description.

a-will · 2025-02-01T01:58:11Z

hw/top_darjeeling/ip/xbar_main/rtl/autogen/xbar_main.sv

@@ -1431,7 +1431,8 @@ end
  tlul_socket_m1 #(
    .HReqDepth (16'h0),
    .HRspDepth (16'h0),
-    .DRspPass  (1'b0),


Just noting this is a timing change (albeit what the hjson file asked for, hehe).

a-will · 2025-02-01T02:01:59Z

hw/top_englishbreakfast/data/top_englishbreakfast.hjson

+      {
+        "name": "Esc",
+        "desc": "escalation reset request",
+        "module": "alert_handler"


Huh, but englishbreakfast doesn't have an alert_handler. Does this field not actually do anything?

TBD: notice the earlgrey and englishbreakfast use the same toplevel.sv.tpl, so there are plenty of questions about this.

a-will · 2025-02-01T02:11:30Z

util/topgen/validate.py

+    'default': ['s', 'the default value of the parameter'],
+}
+param_optional = {
+    'expose': ['s', 'seems redundant TODO'],


This is whether the parameter bubbles up to the top of the generated toplevel.sv file. We probably need to redo some of the template stuff... but that's for another day!

a-will · 2025-02-01T02:15:56Z

util/topgen/validate.py

+    'expose': ['s', 'seems redundant TODO'],
+    'local': ['s', 'whether it is a localparam, interpreted as boolean'],
+    'name_top': ['s', 'the name in the top-level'],
+    'randcount': ['d', 'TODO'],


This is apparently the number of bits requested for a random number inserted into toplevel_rnd_cnst_pkg.sv. If I'm not misreading the code, it looks like randwidth is added ...and is just equal to randcount.

I think they are not always the same, which is what triggered the TODO. I think we need a separate PR which can generate documentation about the format of the hjson files partly based on the key's documentation.

util/topgen/validate.py

a-will

I admit my eyes started to glaze over after awhile of reading the refactoring. Also, we probably could use a canonical config output order, so we don't get these huge diffs that simply move the same information around.

However, from what I have been able to read, LGTM

a-will · 2025-02-05T07:08:21Z

util/topgen.py

+    ip_template = IpTemplate.from_template_path(IP_TEMPLATES_PATH /
+                                                template_name)


I wonder if at some point, we'll need to allow for multiple roots for IP templates, including out-of-tree roots. I'm not sure what that would look like, though, since topgen has embedded how to translate configuration in the toplevel.hjson file to the actual ipgen attributes.

A problem for another day, if it ever comes up.

a-will · 2025-02-05T07:11:02Z

util/topgen.py

    gencmd = (f"// util/topgen.py -t hw/{top_name}/data/{top_name}.hjson "
              f"-o hw/{top_name}/\n\n")


No need to change, since it's not this PR's fault, but the paths here are kind of wrong. They assume in-tree paths for user files, and they should not. We are provided the path to the top-level hjson file, so we ought to use it. Not that it matters here, since this is just a comment.

a-will · 2025-02-05T23:58:37Z

util/topgen.py

+    topname = topcfg["name"]
+    module_name = module["type"]
+    module_instance_name = params.get("module_instance_name")
+    assert not module_instance_name or module_instance_name == module_name


I'm slightly lost--What is this assertion guarding?

This is checking we will always set module_instance_name to module["type"]. This is the case now, and I expect it will continue to be the case, since we intend to use the top config to templatize the individual core files due to the renaming under ip_autogen, since we will create ip_autogen/.hjson.

Ah, understood. This is a sanity check that module["type"] got passed to the template's module_instance_name .

Yes, and it is important to keep that invariant.

a-will · 2025-02-06T00:02:29Z

util/topgen.py

+    return {
+        "src_clks":
+        OrderedDict({name: vars(obj)
+                     for name, obj in clocks.srcs.items()}),


Is that the correct indentation for style / readability? When keys and values are on separate lines, the values don't get indented?

(just a question -- it feels a bit hard to read for me)

This is what the linters suggest and I think recommend it in the style guides, or else we will get into formatting flip-flops which are a waste of time.

a-will · 2025-02-06T00:15:50Z

util/topgen.py

-        if reggen_only and alt_hjson_path:
-            hjson_path = Path(alt_hjson_path)
-        else:
-            hjson_path = ip_out_path / "data" / f"{ip}.hjson"


Are we sure we should be killing this feature? I don't know who uses it, but there doesn't seem to be a great reason to remove it.

That said... does it even work anymore? It looks like this was broken by #25129, undoing the support that was completed for #8207.

In a world where the tops in the open-source repo are reference tops and Nuvoton's top hjson is out-of-tree, I guess we don't really need that argument anymore, do we? If an integrator wanted to work with partners using a sort of "redacted" version of their internal top, I guess they could provide a separate toplevel.hjson that has the proprietary IPs replaced with shareable models.

👍 Since we now support multiple tops, I. think that may not be that beneficial and just adds complexity. We don't use that feature so happy to get rid of it.

This flag is not even sensible: imagine we had multiple reggens, then they would all use the same hjson file? I vaguely recall talking to someone else about this and agreeing we don't need it anyway.

Razer6

Thanks for doing this. So far, this looks really good and simplifies a lot. I will also apply these changes downstream and check if they work with other tops as well.

Razer6 · 2025-02-05T22:24:56Z

util/topgen.py

+    topname = topcfg["name"]
+    module_name = module["type"]
+    module_instance_name = params.get("module_instance_name")
+    assert not module_instance_name or module_instance_name == module_name


What is the second part of the assertion?

Please refer to this comment #25933 (comment)

Razer6 · 2025-02-05T22:29:26Z

util/topgen.py

+        "typed_clocks":
+        OrderedDict({ty: d
+                     for ty, d in typed_clks.items() if d}),
+        "hint_names":


Is this wrapping of key and value really more readable? Personally, for me it's not.

I let lintpy.py decide on formatting, this way we don't end up with changes that will later be undone to clean up lint. I encourage you to do the same, or we will have flip-flops on plain formatting.

Razer6 · 2025-02-06T08:14:26Z

util/topgen/merge.py

+        # were expanded already.
+        # If racl_mappings is expanded the path is one of the fields
+        for if_name, mapping_path in racl_mappings.items():
+            if isinstance(mapping_path, dict):


Should this be more of an assert? That's done once and should be there anymore, right?

amend_racl can be called multiple times, so this is simple and idempotent.

Razer6 · 2025-02-06T08:27:08Z

util/topgen.py

        "lpg_map": lpg_map,
        "top_pkg_vlnv": f"lowrisc:constants:top_{topname}_top_pkg",
    }

-    ipgen_render("alert_handler", topname, params, out_path)
+
+def generate_alert_handler(top: Dict[str, object], module: Dict[str, object],


Can we get rid of the generate_xyc functions? They all do exactly the same (except RACL but the same check is done at the outside). These functions do not serve any business logic. I guess that can be wrapped in a single location. Maybe pass the generate_modules function just the params function?

There are some subtleties: the get_params functions have slightly different signature so passing the get_params would be quirky; passing the params themselves won't work when and if we support different multiply-parameterized instances of ipgens. I'd rather punt on changing this in this PR.

Razer6 · 2025-02-06T08:32:42Z

util/topgen.py

-        if reggen_only and alt_hjson_path:
-            hjson_path = Path(alt_hjson_path)
-        else:
-            hjson_path = ip_out_path / "data" / f"{ip}.hjson"


👍 Since we now support multiple tops, I. think that may not be that beneficial and just adds complexity. We don't use that feature so happy to get rid of it.

Razer6 · 2025-02-06T08:45:28Z

util/topgen.py

+        insert_ip_attrs(ipgen_instances["ac_range_check"][0]["type"],
+                        _get_ac_range_check_params(topcfg))
+    # Pinmux depends on flash_ctrl and otp_ctrl
+    amend_pinmux_io(topcfg, name_to_block)


I think this call to amend_pinmux_io should be within the if below. Not all tops may have a pinmux so don't add a requirement here on the necessary keys in the HJSON (needed by the amend function).

Done. I guess for tops without a pinmux we may need an alternative flow to potentially connect to pads... though I imagine you may never connect pads directly to these models.

Razer6 · 2025-02-06T08:50:25Z

util/topgen.py

+        topcfg['incoming_interrupt'] = OrderedDict()
+
+    for m in topcfg['module']:
+        if m['type'] == 'alert_handler':


if m.get("template_type") == "alert_handler"):

Razer6 · 2025-02-06T08:50:37Z

util/topgen.py

+                    Path(args.topcfg).parent / alert_mappings_path)
+                for alert_group, alerts in mapping.items():
+                    topcfg['incoming_alert'][alert_group] = alerts
+        elif m['type'] == 'rv_plic':


if m.get("template_type") == "rv_plic"):

You are right! At this point we support no more than 1 of each. By the way, we may want to flag an error if there are none, probably in another PR.

Razer6 · 2025-02-06T08:52:00Z

util/topgen.py

@@ -1211,8 +1469,6 @@ def main():
        log.error('Seed "rnd_cnst_seed" not found in configuration HJSON.')
        exit(1)

-    secure_prng.reseed(topcfg["rnd_cnst_seed"])


What's the reason of this removal? Probably the follow up question is what was this used for previously?

I moved it to where it actually matters, which is prior to each loop of _process_top: without running each pass with the same seed we could not compare the output file for stability.

Razer6 · 2025-02-06T08:55:15Z

util/topgen.py

+
+    topname = topcfg["name"]
+    cfg_copy = deepcopy(topcfg)
+    for pass_idx in range(1, maximum_passes + 1):


Can we make the loop run like for pass_idx in range(maximum_passes )? May Matlab times are far behind where you start counting with 1.

This was to make f-strings be easier, but that is too much of a detail. Anyway, python range has three ways to be called, and with two arguments (a, b) it yields a ... b-1, and with three a, b, s it does as with two except s becomes the increment.

I undid it anyway and also noticed process_top doesn't need pass_idx.

matutem · 2025-02-06T09:18:40Z

@a-will regarding #25933 (review), I expect the new hjson files will be stable. The changes for this change in topgen are the cause of the present difference, but I expect going forward there should be no gratuitous diffs. If there are we can use OrderedDicts for problematic dicts.

Do not require the existence of "int", "debug", or even "peripheral" resets. Signed-off-by: Guillermo Maturana <[email protected]>

The reset_requests top config entry should really be initialized from the original hjson file rather than explicitly in merge.py, since it ends up making assumptions about what reset_requests should have. Part of lowRISC#25920 Signed-off-by: Guillermo Maturana <[email protected]>

This makes the complete top config hjson have the same names for attributes. Previously the original name for this was "clk" but the complete config renames it to "clock" because of the Clocks class field names. The Clocks class is used quite a bit more in topgen, so this rename creates less canges, and it is arguably a better name. Part of lowRISC#25920 Signed-off-by: Guillermo Maturana <[email protected]>

This is a big change in topgen. Some salient changes: - It always regenerates from scratch the ipgen in-memory configurations (as IpBlock objects) so it is truly incremental: the previous flow reused the pre-existing generated ipgen hjson if found. - It delays the actual ipgen file generation until the complete top config is generated. This is a step towards breaking topgen into smaller and mutually independent steps, thus enabling parallelizing them. - On each pass it uses the most up-to-date top config, and detects convergence if the top configs from one pass and the previous one match. - It converges with a single pass of process_top (though a second pass is done to enable the comparison). - The generated RTL with and without these changes is identical. In order to accomplish this some important changes are made: - Call some of the merge_top functions suitable for each specific ipgen prior to generating the in-memory hjson config. This means not all IpBlock objects are available, so these merge functions are instrumented to skip missing blocks. - The generation of ipgens are ordered according to dependencies. This order is created from the expected block functionality, so is not derived from a constructed graph, but converging in one pass means it is correct. - Calling merge functions early means some of the objects in the in-memory top config are not plain dictionaries but are classes, so the code needs to handle either. Part of lowRISC#25920 Signed-off-by: Guillermo Maturana <[email protected]>

Add support for added top config attributes. Dive deeper into a few attributes. Left a few TODOs for some attributes, most of these are not yet populated. Part of lowRISC#25920 Signed-off-by: Guillermo Maturana <[email protected]>

Razer6 · 2025-02-06T21:18:57Z

CHANGE_AUTHORIZED: hw/top_earlgrey/data/top_earlgrey.hjson

a-will · 2025-02-06T21:56:39Z

CHANGE AUTHORIZED: hw/top_earlgrey/data/top_earlgrey.hjson

A property name changed, and some other properties were added. However, no functional changes to earlgrey were made (except potentially for the values of random numbers, but those aren't protected).

matutem requested review from pamaury, a-will and Razer6 January 17, 2025 21:25

matutem force-pushed the topgen_split branch 4 times, most recently from 0161920 to e7d519a Compare January 29, 2025 20:22

matutem marked this pull request as ready for review January 29, 2025 20:22

matutem requested a review from msfschaffner as a code owner January 29, 2025 20:22

matutem marked this pull request as draft January 29, 2025 20:28

matutem force-pushed the topgen_split branch from e7d519a to 6b00646 Compare January 29, 2025 20:56

matutem changed the title ~~[tools,topgen] DRAFT OF Simplify topgen~~ [tools,topgen] Enhance and simplify topgen Jan 29, 2025

matutem force-pushed the topgen_split branch from 6b00646 to 03fdc1f Compare January 29, 2025 23:10

matutem requested a review from davidschrammel January 29, 2025 23:11

matutem marked this pull request as ready for review January 29, 2025 23:11

matutem force-pushed the topgen_split branch 2 times, most recently from 3936969 to 953d787 Compare January 31, 2025 10:14

a-will reviewed Feb 1, 2025

View reviewed changes

matutem marked this pull request as draft February 1, 2025 16:03

matutem force-pushed the topgen_split branch 2 times, most recently from b7c8e3d to d71541e Compare February 3, 2025 18:29

matutem marked this pull request as ready for review February 3, 2025 18:43

matutem force-pushed the topgen_split branch from d71541e to b300930 Compare February 3, 2025 23:09

matutem requested review from andreaskurth, vogelpi and moidx February 3, 2025 23:12

a-will approved these changes Feb 6, 2025

View reviewed changes

Razer6 approved these changes Feb 6, 2025

View reviewed changes

matutem force-pushed the topgen_split branch from b300930 to 7b3f1bb Compare February 6, 2025 10:36

matutem added 4 commits February 6, 2025 10:37

[ipgen,pwrmgr] Enable more flexibility regarding resets

7cbdaa1

Do not require the existence of "int", "debug", or even "peripheral" resets. Signed-off-by: Guillermo Maturana <[email protected]>

[ipgen,rstmgr] Enable more flexibility regarding resets

0425cb9

Do not require the existence of "int", "debug", or even "peripheral" resets. Signed-off-by: Guillermo Maturana <[email protected]>

matutem force-pushed the topgen_split branch from 7b3f1bb to a4edd96 Compare February 6, 2025 11:59

matutem added 2 commits February 6, 2025 20:03

[tools,topgen] Enhance validate pass

2abb93b

Add support for added top config attributes. Dive deeper into a few attributes. Left a few TODOs for some attributes, most of these are not yet populated. Part of lowRISC#25920 Signed-off-by: Guillermo Maturana <[email protected]>

matutem force-pushed the topgen_split branch from a4edd96 to 2abb93b Compare February 6, 2025 20:03

matutem merged commit 530603b into lowRISC:master Feb 7, 2025
39 checks passed

matutem deleted the topgen_split branch February 7, 2025 05:17

		if not isinstance(top['clocks'], Clocks):
		top['clocks'] = Clocks(top['clocks'])

		ip_template = IpTemplate.from_template_path(IP_TEMPLATES_PATH /
		template_name)

		gencmd = (f"// util/topgen.py -t hw/{top_name}/data/{top_name}.hjson "
		f"-o hw/{top_name}/\n\n")

[tools,topgen] Enhance and simplify topgen #25933

[tools,topgen] Enhance and simplify topgen #25933

Conversation

matutem commented Jan 17, 2025 • edited Loading

matutem commented Jan 29, 2025 • edited Loading

a-will left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-will left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Razer6 left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matutem commented Feb 6, 2025

Razer6 commented Feb 6, 2025

a-will commented Feb 6, 2025

matutem commented Jan 17, 2025 •

edited

Loading

matutem commented Jan 29, 2025 •

edited

Loading