Skip to content
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.

Sign up for GitHub

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

Enhance hard reset signal implementation #148

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Enhance hard reset signal implementation #148

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions lib/1.4/utility.dml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,7 @@ template hard_reset {
foreach obj in (this._each_hard_reset)
obj.hard_reset();
}
shared method hard_reset_deassert() default {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is redundant in a sense; you can override signal_lower and call default instead.

(One could argue that it would be better with a simulator-agnostic method to override, but that sort of functionality would rather belong to the signal_port template.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be better to hide the implementation and expose only the high level methods as hard_reset()
is hreset;
port HRESET {
saved bool reset_asserted;
implement signal {
method signal_lower() {
hard_reset_deassert();
default();
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm with Erik on this. What's the added value of overriding hard_reset_deassert instead of overriding signal_lower?

If we add a hard_reset_deassert method like this we'd need to consider some more questions:
For example

  • Should hard_reset_deassert be called after a dev.hard_reset()?
  • Should reset_asserted be true during a call to dev.hard_reset()?
  • Why is there no hard_reset_assert() method?

If we instead let the user override signal_lower in the HRESET port it's very clear what behavior is being overridden, and what can be expected.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

}

template _init_val_hard_reset is (init_val, hard_reset) {
Expand All @@ -342,11 +343,15 @@ template _init_val_hard_reset is (init_val, hard_reset) {
// Instantiate this on top level to support hard reset.
template hreset is hard_reset {
port HRESET {
saved bool reset_asserted;
implement signal {
method signal_raise() default {
reset_asserted = true;
dev.hard_reset();
}
method signal_lower() default {
dev.hard_reset_deassert();
reset_asserted = false;
}
Comment on lines 344 to 355
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would make more sense to start with template hreset is (hard_reset, signal_port) and continue from there. IIRC, @svenwest tried this when adding signal_port, but it was dropped since it caused too much short-term SSM breakage. I think it can be done, but requires a careful migration plan.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both templates are define default methods. Using spec violation is also doubtful.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what Erik means is this:

// Instantiate this on top level to support hard reset.
template hreset is hard_reset {
    port HRESET is signal_port {
        implement signal {
            method signal_raise() default {
                default();
                dev.hard_reset();
            }
            method signal_lower() default {
                default();
            }
        }
    }

    in each init_val {
        is _init_val_hard_reset;
    }
}

Using spec_viol is not doubtful in my opinion. The API docs for signal_interface_t specifically states:

Once raised, the same initiator may not call signal_raise() again without an intervening call to signal_lower(). Similarly, an already low signal may not be lowered again by a signal_lower() call from the same initiator.

I tried adding this template in a PR some time ago. Perhaps we could try again with careful steps not to break SSM builds.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to call default() from the default method?
method signal_lower() default {
default();
}

Copy link
Author

@AleksandrDanilovIntel AleksandrDanilovIntel Jan 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to copy a logic from signal port template

template hreset is hard_reset {
    port HRESET {
        saved bool high;
        implement signal {
            method signal_raise() default {
                high = true;
                dev.hard_reset();
            }
            method signal_lower() default {
                high = false;
            }

}
}
Expand Down