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.

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

ati: Add ATI timing utilities #3

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

ati: Add ATI timing utilities #3

wants to merge 7 commits into from

Conversation

colluca
Copy link

@colluca colluca commented Jan 11, 2022

ATI timing utility class. Provides convenience methods to time apply and acquire signal operations according to ATI specification.

Copy link
Contributor

@andreaskurth andreaskurth left a comment

Choose a reason for hiding this comment

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

Thanks for this useful addition, @colluca!

I agree with the design, and I have a few comments on the implementation.

src/timing_pkg.sv Outdated Show resolved Hide resolved
src/timing_pkg.sv Outdated Show resolved Hide resolved
src/timing_pkg.sv Outdated Show resolved Hide resolved
src/interfaces.sv Outdated Show resolved Hide resolved
src/timing_pkg.sv Outdated Show resolved Hide resolved

interface clk_if();
logic clk;
logic rst_n;
Copy link
Contributor

Choose a reason for hiding this comment

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

This signal is currently unused. Why is it part of clk_if?

Copy link
Author

Choose a reason for hiding this comment

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

To the purpose of this class it is not used. But since a clock is generally coupled with a reset, I thought this way one would only have to declare a single virtual interface, e.g. to pass to drivers, rather than have another interface just with the clock.

What are your opinions regarding this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would keep the reset outside the driver and thus also outside the virtual interface. As evident from recent group discussions, such a rst_n signal cannot be used as functional reset without careful consideration, so I would avoid giving that impression to users. A driver typically provides a reset method that is called by the TB-level reset controller when appropriate.

The current implementation of ati_utility demonstrates one problem quite nicely: even though it is supposed to work with rst_n, none of the methods are affected by it. For example, what would the wait_cycles method do if the reset becomes active (i.e., goes low) while it is counting down? Stop counting and restart counting when the reset is deactivated? Or pause counting and continue when the reset is deactivated?

Copy link
Author

@colluca colluca Jan 15, 2022

Choose a reason for hiding this comment

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

These are very good questions indeed, and maybe even justifies having these methods in a centralized place, so that it is taken care of for future programmers, avoiding that they incur in the same questions (or, worse, problems). How do you suggest we face these questions?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see two main alternatives:

Handling reset inside ati_utility

Define that reset asynchronously terminates any of the wait tasks. This could be implemented, e.g., as

/// Wait for `n` cycles of `clk_vif.clk` or for a negative edge of `clk_vif.rst_n`.
task wait_cycles(int unsigned n);
    fork
        repeat(n) @(posedge(clk_vif.clk));
        @(negedge(clk_vif.rst_n));
    join_any
endtask

Handling reset outside ati_utility

Remove rst_n from clk_vif. The user then has the responsibility (but also the flexibility) of handling reset behavior themselves.

Deciding on one alternative

I think this boils down to the following question pair: Do we consider resetting a feature and design goal of ati_utility and then implement it as we see fit? Or do we leave resetting entirely to the user, to be implemented outside ati_utility if necessary?

I further think a good rule of thumb is: Do we see a real use case for this feature and know we want to implement it one way or the other? If not, I think we should defer this feature to when we really need it.

src/interfaces.sv Outdated Show resolved Hide resolved
src/timing_pkg.sv Outdated Show resolved Hide resolved
colluca and others added 6 commits January 12, 2022 14:32
Co-authored-by: Andreas Kurth <akurth@iis.ee.ethz.ch>
Co-authored-by: Andreas Kurth <akurth@iis.ee.ethz.ch>
Co-authored-by: Andreas Kurth <akurth@iis.ee.ethz.ch>
Co-authored-by: Andreas Kurth <akurth@iis.ee.ethz.ch>
Co-authored-by: Andreas Kurth <akurth@iis.ee.ethz.ch>
Provide convenience methods to time apply and acquire operations according to ATI specification.
@colluca colluca changed the title ati: Add ATI timing utilities Draft: ati: Add ATI timing utilities Sep 14, 2022
@colluca colluca changed the title Draft: ati: Add ATI timing utilities ati: Add ATI timing utilities Sep 14, 2022
@colluca colluca marked this pull request as draft September 14, 2022 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants