[topgen] Add support for split IPs

[etterli]

This builds on #31219 and adds the required topgen extensions.

[andreaskurth]

Thanks @etterli and @glaserf, this is a nicely structured extension! 👍

Minor changes requested; nothing fundamental.

I checked this two ways. All three in-tree tops regenerate identically with this branch, so there is no regression on the non-split path. Then I made aon_timer a synthetic split IP (partitioned clocking, one interrupt and one extra alert in the secondary partition, an intra_p2s req/rcv pair, and secondary- and both-scoped exposed parameters) and instantiated it in top_earlgrey with domain: Main / domain_secondary: Aon. That works impressively well end to end: the alert slices and LpgMap entries land in the right per-domain buses and LPGs (LPG 5 = io_div4_timers + lc_io_div4[Main] for the primary, LPG 14 = aon_timers + lc_aon[Aon] for the secondary), the intra-IP signal is routed out of earlgrey_pd_main into earlgrey_pd_aon over a top-level net, TL/RACL/scan stay primary-only, and the domain == domain_secondary variant collapses to a local wire as documented. The comments below all come out of that experiment.

[andreaskurth]

Defaulting clock_group_secondary here happens before validate_top runs, which I think makes the matching check in check_power_domains unreachable: by the time validate.py:1354 asks whether clock_group_secondary exists, this line has already created it.

The consequence is that a split instance written as clock_group: "timers" rather than clock_group: {primary: ..., secondary: ...} does not get the intended error; it silently generates the secondary partition into the secure group. I reproduced this: .clk_aon_i(clkmgr_clocks.clk_aon_secure) instead of clk_aon_timers, exit code 0.

Would it be better to default this only for a non-split instance, and let validation reject a missing clock_group_secondary?

[etterli]

Good point. I've now added a proper validation of the secondary clocking information that it is consistent. If secondary clock_srcs is defined, the secondary clock_group and domain keys must also exist.

[andreaskurth]

The upfront consistency check does the job: the config that previously generated clk_aon_secure silently is now rejected.

Small wording point on one of the messages: for a split instance that writes clock_group: "timers" (a plain string rather than a per-partition dict) the error is "defines a secondary domain but no primary information is specified for clock_group". That reads as if the primary entry were missing, when the real problem is that the key is not partitioned at all. Would something like "clock_group is not specified per partition" be clearer?

[etterli]

Thanks @andreaskurth for the detailed review. Good catches. I've now addressed the first comments. Some fixes required also one additional reggen commit. Will continue tomorrow.

I refrained from rebasing on the merged reggen commits so the diff is simpler to compare.

[etterli]

I've now also addressed the remaining feedback. These points gave raise to some additional improvements/changes.

• Power domains are now validated before any of its information is used.
  • This resulted in autogen changes. The domain attribute is now emitted earlier. Otherwise there are no functional changes to the autogen files.
• A split IP's secondary partition must always be clocked.
• Intra-IP connections support req_rsp.
• CIO signals are emitted per partition for split IPs instead of for all partitions.
• Fixed a trailing comma issue in module instantiation for a split IP's secondary partition

The first push updates the changes. The 2nd one is a rebase on the current master.

[etterli]

Good point. I've now added a proper validation of the secondary clocking information that it is consistent. If secondary clock_srcs is defined, the secondary clock_group and domain keys must also exist.

[andreaskurth]

Thanks @etterli! Every point from the last review is addressed, and you found and fixed two things I had missed (the CIO declarations and the clkmgr idle hints). Making the secondary partition always clocked is a good simplification.

Two small blockers, both CI lint failures on the current head, plus one doc slip and two nits. Everything else LGTM, so approving because CI will tell when lint passes.

[andreaskurth]

The upfront consistency check does the job: the config that previously generated clk_aon_secure silently is now rejected.

Small wording point on one of the messages: for a split instance that writes clock_group: "timers" (a plain string rather than a per-partition dict) the error is "defines a secondary domain but no primary information is specified for clock_group". That reads as if the primary entry were missing, when the real problem is that the key is not partitioned at all. Would something like "clock_group is not specified per partition" be clearer?

[etterli]

Thanks again for the review. I've now fixed it and I think this is ready to merge.

However, as the AST split already surfaced some required changes I leave this PR as draft for now. I will merge it once the AST split draft is more mature.

[etterli]

The last pushes contains the new topgen changes implemented during the AST split integration work. I don't expect further changes.

The main new changes are (last 4 commits):

• Support internal clock sources
• Extend external clock handling such that these are properly wired down to the receiving power domains.
• Add a template to support the memory configuration struct assignments inside the power domains instead of at chip level
• Make xbar connections clock aware.

[etterli]

@andreaskurth PTAL

[andreaskurth]

Thanks @etterli. The split-IP part stays in good shape. This review is for the four commits added after my previous review & approval.

Major: [topgen] Support clock sources inside the top crashes for what looks like its main use case; see the comment on merge.py. An alert-bearing module whose primary clock is sourced inside the top hits AttributeError: 'ModuleClockRef' object has no attribute 'split'. CI does not catch it because no in-tree top uses an internal clock source, so every new path here is dormant.

Couple of other comments below.

Two process points, both worth your judgement rather than mine:

• The four new commits are not split-IP work. [topgen] Support clock sources inside the top in particular is a feature of its own, with a new top-hjson schema and its own failure mode. Splitting it into a separate PR would make it easier to review now and easier to bisect later; it would also let the split-IP work land on its own.
• One nit from round 2 is still open: the "no primary information is specified for clock_group" message in _pre_validate_split_ip.

[andreaskurth]

This dereferences the raw clock_connections value, which may still be a ModuleClockRef placeholder rather than a net-name string.

create_alert_lpgs runs at topgen.py:1890, but the placeholders are only replaced by resolve_module_clocks at topgen.py:1904. So for any alert-bearing module whose primary clock is sourced inside the top, _make_lpg_entry reaches this line with a ModuleClockRef and topgen dies:

File "util/topgen/merge.py", line 1122, in _make_lpg_entry
    clock_group = clock_groups[clk.split(".")[-1]]
AttributeError: 'ModuleClockRef' object has no attribute 'split'

I reproduced this on top_earlgrey by giving io_div4 module: "rv_dm", signal: "ndmreset_req" and putting aon_timer into the ast group (src: "ext"). The unmanaged-clock loop just above is also comparing a string against the placeholder, which silently never matches.

Since alert senders sit on the block's primary clock, I think this hits any AST-domain IP with alerts that takes an internally sourced clock. Could resolve_module_clocks run before create_alert_lpgs, or could the LPG code resolve the reference itself? If neither is easy, would it be worth raising a clear error here when the value is still a ModuleClockRef, so it fails with an explanation rather than an AttributeError?

[andreaskurth]

Nothing in the repo includes this template. So it is never rendered, and neither the Python nor the generated SystemVerilog gets any coverage from CI. Would it be better to land it together with the AST-split change that uses it?

Two things inside it while it is unused, in case they get missed later:

• mem_cfg_types is defined but never referenced, and the loop unpacks kind and width without using either; only field and wire are used.
• mem_cfg_lhs_pad calls max() over mem_cfg_consumers, which raises ValueError on an empty list. A guard or a default= would make an empty consumer list a no-op instead.

[andreaskurth]

These three keys (module, signal, partition on a clock source) are new top-hjson schema, but I could not find them documented anywhere; util/topgen/README.md gained a "Split IPs" section for that feature, and there is no equivalent for this one.

Could the README get a short section covering how a clock source is declared as internally sourced, the constraint that the referenced signal must be a uni/req/logic inter-signal (which check_module_sourced_clocks enforces), and the requirement that a same-power-domain producer be listed in inter_module.top (which _resolve_module_clock raises on)? Those last two are discoverable only by hitting the errors.

[andreaskurth]

Missing space after the period: "for xbar nodes.These nodes also never reach ..."

stale