-
Notifications
You must be signed in to change notification settings - Fork 779
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
[topgen] Support for unmanaged external resets #25262
base: master
Are you sure you want to change the base?
Conversation
cdfecde
to
aa04f16
Compare
aa04f16
to
6a88218
Compare
if lpg['unmanaged_reset']: | ||
rst_en = top['unmanaged_resets']._asdict()[lpg['reset_connection']['name']].rst_en_signal_name | ||
else: | ||
rst_en = lib.get_reset_lpg_path(top, lpg['reset_connection']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that lpg
is just a dict, but maybe it makes sense to check more sensibly to make sure reset_connection
is defined here? (And I realise that this is unchanged in the commit, but I'm asking because you're touching the line :-D)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you really think that is needed there? There is a single place, where something is something added to the lpg
dict:
'name': lpg_name,
'clock_group': None if unmanaged_clock else clock_group,
'clock_connection': clock,
'unmanaged_clock': unmanaged_clock,
'unmanaged_reset': is_unmanaged_reset(top, reset_name),
'reset_connection': primary_reset
})
That's already the full lpg
// List of unmanaged external resets | ||
unmanaged_resets: [ | ||
// { name: "my_ext" } | ||
] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really think we need to document the variable here (where it's getting defined for a particular toplevel). If we do want to document it, I'd probably suggest filling out the example more explicitly, but not in the list. Maybe something like this? (in the template)
// List of unmanaged external resets
//
// These should be listed as dictionaries that just give their names, in this form:
//
// { name: "my_external_reset" }
unmanaged_resets: []
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed that at all, though I'll add that for Darjeeling?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM other than a few small comments. Thx @Razer6 👍
// List of unmanaged external resets | ||
unmanaged_resets: [ | ||
// { name: "my_ext" } | ||
] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need this for Earlgrey, as far as I know any use case discussed so far. So I suggest removing it to prevent confusion. If we would need it in the future, we could take a look at Darjeeling to find out how to specify unmanaged resets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. Currently it's a required field, I will change that. In a follow up also for the unmanaged clocks I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or should we add the following to know this feature is there?
// List of unmanaged external resets
//
// These should be listed as dictionaries that just give their names, in this form:
//
// { name: "my_external_reset" }
// unmanaged_resets: []
Darjeeling would not use that but downstream tops.
Signed-off-by: Robert Schilling <[email protected]>
6a88218
to
b6f7083
Compare
@andreaskurth @rswarbrick Thanks for the review. I updated the PR accordingly. |
Tops may specify resets that are completely managed externally, i.e., with a dedicated reset manager in the uncore environment of an integrated OpenTitan. Feeding that reset to the internal reset manager is not necessary and just adds complexity in the configuration.
To address this, this PR introduces support for unmanaged external resets, allowing them to operate independently from the internal reset manager. To define an unmanaged reset, simply add an entry to the top-level unmanaged_resets list. Once defined, these resets can be used like any other reset for IP instantiation and crossbar mapping.
Additionally, when routing IP alerts to the alert manager, the alert manager requires a reset status signal to know if the reset for the alert’s IP is currently active. For unmanaged external resets, it's assumed that the top-level external environment already has knowledge of the reset status. Topgen creates a dedicated LPG for every external reset. The reset status signal is routed at top-level and connected to the associated group to the alert manager.