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

Setting the .Options field to 0 by default #9

Open
riedel-ferringer opened this issue Jan 19, 2023 · 3 comments
Open

Setting the .Options field to 0 by default #9

riedel-ferringer opened this issue Jan 19, 2023 · 3 comments

Comments

@riedel-ferringer
Copy link

riedel-ferringer commented Jan 19, 2023

procedure WaitForClock (

I have a VU which is capable of waiting for rising and/or falling clock edges. To this end, I implemented a custom procedure
WaitForClock(transactionRec, nrOfCyles, edgeKind);
In the implementation, I use transactionRec.Options <= edgeKind to pass the repective edge which shall be awaited by the VU.

However, there is the predefined procedure WaitForClock(transactionRec, nrOfCyles) which can of course also be used - no way to avoid or detect it. The problem is that this implementation does not set the .Options field to any value (why should it, it doesn't need it). But now the options field can have any value, depending on whatever old value it contains - thus calling the predefined procedure results in different behavior, depending on whatever happened before.

Do you see the possibility to explicitly set .Options to zero in WaitForClock? Or is there another way to solve this inconsistency? One could provide another overload of WaitForClock that takes a third parameter defining the edge. I could provide a pull request if you like. Another possibility would be to require the .Options field to be reset to 0 after each transaction. This could be done in RequestTransaction, but such a change might have to much impact on current functionality.

I am aware that there are other ways to work around this issue, e.g. using .ParamToModel or BoolToModel or whatever. It would render such a problem very improbable (but in the end wouldn't fix it).

@JimLewis
Copy link
Member

I would do this by adding the parameter to the current WaitForClock and initializing the default value for that parameter to match the original behavior (rising edge).

I choose initialized parameter over overloading if the update is relatively light weight - which if done right, this one is.

A pull request against StreamTransactionPkg and StreamTransactionArrayPkg would be appreciated.

Where is edgeKind defined? Should this be added to TbUtilPkg? Do you have a similar proposal for TbUtilPkg? Edge style was largely ignored there and it would benefit from some sort of sensible edge style handling. There it is pervasive and maybe we should brainstorm in issues about it first. If implemented in a brute force fashion, in TbUtilPkg, I might do this same capability by using overloading instead - that way the original version of WaitForClock does not get slowed down by the extra code that checks for the edge style.

@JimLewis
Copy link
Member

JimLewis commented Aug 3, 2024

In 2024.07, TbUtilPkg WaitForClock now has a ClkActive input that is initialized to '1'. This allows WaitForClock to specify if it wants rising edge ('1') or falling edge ('0').

The same sort of capability could be added to the WaitForClock Directive in both StreamTransactionPkg and AddressBusTransactionPkg. The meaning of WaitForClock in the VC is to wait until nrOfCyles of its active edges go by.

In fact, if you are using WaitForTransaction with a Clock, before starting the next transaction, it is going to check and see if you are within delta cycles of the ActiveEdge of Clock - if it is not, it is going to wait until it finds the next active edge of clock. It should be noted that WaitForTransaction now also has a ClkActive input. As a result, being able to set which edge WaitForClock is meaningless if you are using WaitForTransaction with a clock.

@riedel-ferringer
Copy link
Author

Hi Jim,

first I am sorry that I obviously did not notice your answer over a year ago. Must have missed it :-(

Thanks for the update and the implementation!

Regards

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

No branches or pull requests

2 participants