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

Initial commit of the Verilog DV style guide #22

Merged
merged 1 commit into from
Mar 16, 2020

Conversation

udinator
Copy link

@udinator udinator commented Feb 6, 2020

Signed-off-by: Udi Jonnalagadda udij@google.com

Copy link
Contributor

@weicaiyang weicaiyang left a comment

Choose a reason for hiding this comment

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

LGTM with a few nit

must match the name of the variable being assigned to.
This ensures that any reporting messages refer to the right objects.
2. To avoid name collisions, it is recommended that global types and types defined
within a package have unique prefixes. This include typedefs, enumerated
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: includes

Copy link
Author

Choose a reason for hiding this comment

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

Fixed, thanks for pointing this out!

## Directory Structure and Packages

* Define one class per file, and the filename should match the name of the
class. The class files must be `included into the package file.
Copy link
Contributor

Choose a reason for hiding this comment

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

extra `

Copy link
Author

Choose a reason for hiding this comment

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

Fixed, thanks!

8. It is recommended, but not necessary, to use the built-in UVM timeout
mechanism, `uvm_root::set_timeout(<time_limit>, 1)`.
The base test in every testbench should specify a default timeout limit,
which can be overriden by every derivative test.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: overridden

Copy link
Author

Choose a reason for hiding this comment

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

Good catch, thanks.

README.md Outdated
Comment on lines 6 to 7
- [SystemVerilog style guide](VerilogCodingStyle.md)
- [SystemVerilog style guide for DV](VerilogForVerificationStyle.md)

Choose a reason for hiding this comment

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

I thought this naming might be a little bit confusing.
Is SystemVerilog style guide for RTL only or both DV and RTL?
What do you all think?

Copy link
Member

Choose a reason for hiding this comment

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

I think the intent is that "SystemVerilog style guide" covers both as a baseline style guide, and "SystemVerilog style guide for DV" adds further guidance for DV. Maybe this could be presented in a more obvious way though...

Copy link
Author

Choose a reason for hiding this comment

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

Good point, perhaps this could be called something like Design Verification Style Guide, and omit SystemVerilog from that name entirely? I think the original guide can keep the name "SystemVerilog style guide".

Copy link
Contributor

Choose a reason for hiding this comment

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

"Coding Style Guide for Design Verification" (DVCodingStyle.md)? Since it is not only SV, but
UVM and other stuff (recognizing that UVM is a convention of SV).

Copy link
Contributor

@vogelpi vogelpi left a comment

Choose a reason for hiding this comment

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

Thanks @udinator for driving this forward. I have a few suggestions regarding the actual content. Right now the styleguide contains many non-ASCII characters. These should be replaced by their ASCII counterparts. You can detect them with the following command

grep --color='auto' -P -n "[^\x00-\x7f]" VerilogForVerificationStyle.md

VerilogForVerificationStyle.md Outdated Show resolved Hide resolved
VerilogForVerificationStyle.md Outdated Show resolved Hide resolved
VerilogForVerificationStyle.md Outdated Show resolved Hide resolved
VerilogForVerificationStyle.md Outdated Show resolved Hide resolved
VerilogForVerificationStyle.md Outdated Show resolved Hide resolved
```


### Forbidden System Tasks and Functions
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use "discouraged" instead of "forbidden"?

Copy link
Author

Choose a reason for hiding this comment

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

I feel that 'forbidden' would be slightly better for the purpose of this section, as the listed system functions must never be used in any lowRISC DV environment, as opposed to providing a recommendation to not use the functions. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

In the design style guide we preferably use "discouraged". But I think there is no single true answer here. I guess "forbidden" is a very hard term. Question: Would you completely block the adoption/integration of code into a project if any of these functions are used? If yes, "forbidden" is the right term. If instead you would consider a step-wise process of integration first and at a later point rework the problematic parts, I would prefer "discouraged".

Anyway, I don't really mind. Please feel free to take the decision.

Copy link

Choose a reason for hiding this comment

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

'Embargoed'?

Copy link
Author

Choose a reason for hiding this comment

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

IMO, use of these functions should block adoption of code into a project, as these functions are either not in the SystemVerilog LRM or are not part of the SV randomization stability model (which could break reproducibility). I will add this rationale to the document.
What do you think @sriyerg?

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 forbidden can be justified in some cases, in particular the $random and $dist_* mentioned below which can break simulation reproducibility. Breaking reproducibility can be very painful and hard to diagnose so this seems a good reason to forbid those functions.

The others don't seem to cause such a serious problem if they occur but as they're not part of the LRM could break the build if you have a simulator that doesn't support them, again a decent reason to forbid them.

Copy link

Choose a reason for hiding this comment

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

Agree - we may have much control over 3rd party code, but if they are using these, then we file bugs and get them to fix it. We do want to forbid the active contributors of OpenTitan from using these. $psprintf is problematic too since its not a part of the LRM and some simulators may not support it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree. IMO, it's good to forbid these items.

@udinator udinator force-pushed the dv_style branch 2 times, most recently from ba8915a to fea223c Compare February 11, 2020 16:07
@udinator
Copy link
Author

Thanks @udinator for driving this forward. I have a few suggestions regarding the actual content. Right now the styleguide contains many non-ASCII characters. These should be replaced by their ASCII counterparts. You can detect them with the following command

grep --color='auto' -P -n "[^\x00-\x7f]" VerilogForVerificationStyle.md

@vogelpi , thanks for the feedback and for pointing this out! I've replaced all of the non-ascii characters with their ascii equivalents.

@udinator udinator force-pushed the dv_style branch 2 times, most recently from 76e29e5 to ae34e3f Compare February 11, 2020 16:34
Copy link
Contributor

@vogelpi vogelpi left a comment

Choose a reason for hiding this comment

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

Thanks @udinator for addressing my feedback. This LGTM!

But please also have the other comments resolved and get approval from other DV folks.

Comment on lines 120 to 123
To avoid name collisions, it is recommended that global types and types defined
within a package have unique prefixes. This include typedefs, enumerated
types/values, package parameters, package localparams, any functions/tasks, and
macros.
Copy link

Choose a reason for hiding this comment

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

This is repeated word for word from bullet 2 above - perhaps make the bullet above more concise such that this elaborates more on it.

Copy link
Author

Choose a reason for hiding this comment

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

Oops, you're right, I missed deleting this duplicate. I added the detail about prefixing types defined with a class to the bullet 2 above.

within a package have unique prefixes. This include typedefs, enumerated
types/values, package parameters, package localparams, any functions/tasks, and
macros.
This prefixing is not necessary for any types defined within a class, as they
Copy link

Choose a reason for hiding this comment

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

class or module or interface....

Copy link
Author

Choose a reason for hiding this comment

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

Updated, thanks!


## Directory Structure and Packages

* Define one class per file, and the filename should match the name of the
Copy link

Choose a reason for hiding this comment

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

Is this mandatory or a recommendation? We have sources in our repo that violates this (hw/dv/sv/tl_agent/tl_seq_lib.sv, hw/dv/sv/csr_utils/csr_seq_lib.sv).

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, shall we make an exception for sequence libraries and other similar libraries (e.g. in Ibex I use a library of uvm_test derivatives)? I think in the general case this should be a mandatory guideline, however.

| Monitor | `<dut>_monitor monitor;` |
| Scoreboard | `<dut>_scoreboard scoreboard;` |
| Virtual sequencer | `<dut>_virtual_sequencer virtual_sequencer;` |
| Sequencer | `<dut>_sequencer <dut>_sequencer;` |
Copy link

Choose a reason for hiding this comment

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

Additional naming guidelines:
Testbench file that instantiates the DUT: tb.sv
Testbench module name: tb
Instance name of the DUT in tb.sv: dut

Standardizing these naming conventions makes it useful to create supporting DV sources that work universally across all IPs (example: hw/dv/tools/vcs/cover.cfg which tells VCS what coverage patterns to collect). Without the standard tb and dut names, each IP would have to create its own version which slightly increases effort.

Copy link
Author

Choose a reason for hiding this comment

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

Updated, thanks for the suggestion!

testbench will be placed in a directory `<block>/dv/` in three corresponding
subdirectories `<block>/dv/env/, <block>/dv/tests/, <block>/dv/tb/`.
* All UVM virtual sequences for a block will be placed under `env/seq_lib/`,
along with a `<block>_vseq_list.sv` file that `includes all of the virtual
Copy link

Choose a reason for hiding this comment

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

Wrap `includes in double back-ticks.

Copy link
Author

Choose a reason for hiding this comment

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

Done.


3. After factory registration, a constructor must be defined. Use the
`uvm_object_new` macro for UVM objects and the `uvm_component_new` macro for UVM components.
Both of these macros are included in `dv_macros.svh`.
Copy link

Choose a reason for hiding this comment

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

included -> defined

Copy link
Author

Choose a reason for hiding this comment

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

Updated.

7. `DV_EOT_PRINT_..._CONTENTS` TLM fifo display macros
* It is recommended to use these macros to display the status of the
various TLM fifos and queues in the scoreboard at the end of the test
to ensure correctness of the testbench checking mechanisms.
Copy link

Choose a reason for hiding this comment

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

....to ensure those data structures have been emptied. There should be no pending transactions that have not yet been compared before the simulation ends.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, that definitely sounds better. Updated.

example below:

```systemverilog
<block>_agent/
Copy link

@sriyerg sriyerg Feb 11, 2020

Choose a reason for hiding this comment

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

Indicate that it is recommended to place the agent in a common area alongside other agents rather than with the DUT's UVM TB it was specifically written for - this creates a repository of UVC that users can find in one place for more vertical reuse.

Copy link
Author

Choose a reason for hiding this comment

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

Good idea, I've added this point.

DVCodingStyle.md Outdated
Comment on lines 136 to 137
### Function Declarations

Functions declared within packages, as well as any other static entities such as
modules and interfaces, must be explicitly scoped with either the `static` or
`automatic` keyword.

Copy link

@sriyerg sriyerg Feb 25, 2020

Choose a reason for hiding this comment

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

Making this a sub-heading of "Directory Structure" seems a little strange. Perhaps move it under "SV Language Features"?

Copy link
Author

Choose a reason for hiding this comment

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

True, it does make sense to move this to under the SV language features heading instead.
Thanks for pointing that out!

DVCodingStyle.md Outdated
This document is a supplemental Style Guide to the [lowRISC Verilog style
guide](https://github.com/lowRISC/style-guides/blob/master/VerilogCodingStyle.md),
with emphasis on writing code for Design Verification (DV) in SystemVerilog,
following the UVM methodology.
Copy link

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

[Or indicate this in the introduction]

Copy link
Author

Choose a reason for hiding this comment

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

I think a link would look better, added.

DVCodingStyle.md Outdated

Define one class per file, and the filename should match the name of the class.
The class files must be `` `include``-ed into the package file.
Directory structure of the `uvm_agent` and `uvm_env` testbench components
Copy link

Choose a reason for hiding this comment

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

nit: UVM agent and UVM environment instead of uvm_agent and uvm_env

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

@sjgitty sjgitty left a comment

Choose a reason for hiding this comment

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

Left one question, otherwise LGTM

DVCodingStyle.md Outdated

// define any local types/functions/tasks
typedef enum bit {
MyblockBooleanZero,
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the relationship between newblock and Myblock, or should that be NewblockBoolean...

Copy link
Author

Choose a reason for hiding this comment

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

Good catch, let me change this to NewblockBoolean.

@sjgitty
Copy link
Contributor

sjgitty commented Feb 28, 2020

would love to see a review from @rasmus-madsen but otherwise I think this is almost ready to go. Thanks @udinator

@tunghoang290780 tunghoang290780 self-requested a review March 3, 2020 22:57
Copy link

@tunghoang290780 tunghoang290780 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@rasmus-madsen rasmus-madsen left a comment

Choose a reason for hiding this comment

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

Hi Udi
thank you for putting this document together I think it is good work.

I have added some comments - most of them can be boiled down to a matter of opinion.
In general I personally I would only like to add "pre" and "post" fixes where it adds value and clarity.
and and the same time we must be careful not to add anything that can add to confusion.

the m_ prefix is a good example where I don't see any added value but see added confusion.
Many will use the m_ prefix for indication the master side of a bus.
like your AXI example in the document.

other places where I find these prefix is in arrays etc.
from a hardware perspective value larger than 1 bit is an array.
So a bus is in theory also an array - which means all signals busses should have your s or array added to it.
I am trying to think about how the _array would help me in anyway when reading code. and I just don't see any added value - the instance declaration will give me the same information.

and maybe we also want to talk about objections in its own thread?
I don't have strong feelings here - but I have seen many deadlocks from poor use of objections in scoreboards - which then requires a watchdog
that you can do without by moving these into the monitors. for most designs objections in the test and and drain time will be sufficient

module, or interface, as they will be scoped by the parent entity.
4. Note that all package variables (parameters, etc.) must have a specified type.
5. When instantiating a user-defined class member variable that is a class object:
* Prefix the instance name of the class object with `m_`, as such:

Choose a reason for hiding this comment

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

I know I am touching a minefield here, but what is the added value by the prefixing?
why is a user type special?
is there any scenario where one would look at the instance name with the prefix and NOT lookup the declaration?

Copy link

Choose a reason for hiding this comment

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

We are mainly following the UVM coding style here (UVM source code also prefixes class members with m_) considering we heavily derive from it.

Plus, it also helps quickly distinguish class members from task/function-local variables.

Example:

  class my_class;
    int m_foo;
    ...
    function void my_class::set_foo(int foo);
      m_foo = bar ? foo : 1; 
      ...
      if (m_foo == 10) begin 
        // do something
      end
      ...
    endfunction

Above is cleaner / less confusing and potentially reduces chances of bugs compared to below:

  class my_class;
    int foo;
    ...
    function void my_class::set_foo(int foo);
       this.foo = bar ? foo : 1; 
       ...
       if (foo == 10) begin // bug! easy to forget that we need to use this.foo here
         // do something
       end
       ...
    endfunction

In any case, I do agree with you - this is indeed subject to user preference. I think we should clarify that this is the recommended way of naming things and not an absolutely mandatory one.

Choose a reason for hiding this comment

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

@sriyerg thank you for the examples
I see your point. but that raises a different question.
Is the style guide to help the coder or the maintainer (which could be the same person)
I think it is to get the coder to write code in such a way it is easy to understand and maintain.

The coder should know exactly what they are doing - and can use any prefix as they will know what it means.
The maintainer might come from a different world and have different assumptions what m_ and other post/pre fixes indicate.
as mentioned I've seen m_ / s_ used as indication of master and slave driven parts of a bus.

while I do have an opinion, I am mostly playing the devils advocate here to make sure we weight pro's and cons before making decisions

Copy link
Author

Choose a reason for hiding this comment

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

@sriyerg @rasmus-madsen , based on some discussion we had about this point, we think making this prefixing optional would be a good way to go

int NUM_ANIMALS = 10;
// This plural convention is recommended, but both are equally acceptable
my_animal m_my_animals[NUM_ANIMALS];
my_animal m_my_animal_array[NUM_ANIMALS];

Choose a reason for hiding this comment

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

if we want this I think we need to include unpacked vectors in this also.
i.e
logic my_vector[0:8]

Copy link

Choose a reason for hiding this comment

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

This is also just a recommendation.

Choose a reason for hiding this comment

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

that is fine.. I am also just pointing out that
a bus is technically an array of logic - which should be included if we really want this.
and that don't see any added value as the instantiation will show that this is an array.

Copy link

Choose a reason for hiding this comment

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

Sure, we can update the style guide to add it as well.

Copy link
Author

Choose a reason for hiding this comment

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

Updated to If an array of class handles or logic is desired.


## UVM Guidelines

Always call `run_test()` without the test name argument. This allows command

Choose a reason for hiding this comment

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

nit:
I think the mark down style guide dictates only one sentence per line
and no line breaks in a sentence

Choose a reason for hiding this comment

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

this goes for the entire document!

Copy link
Contributor

Choose a reason for hiding this comment

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

nit:
I think the mark down style guide dictates only one sentence per line
and no line breaks in a sentence

Unfortunately this document falls in the category of "unrendered / README files"
in the markdown style guide
and thus has to stay at 80 characters wrapped rather than one sentence per line.

Choose a reason for hiding this comment

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

@sjgitty not sure that is unfortunate.. ;)
@udinator sorry for missing that fact.


1. A driver should only communicate with one sequencer on one SystemVerilog
interface, and should not have any analysis ports.
2. A driver should not randomize any data received from transaction

Choose a reason for hiding this comment

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

should not?
I think we need stronger wording here.
Cannot!

the driver should be "stupid" and just drive the interface with the information from a transaction/sequence item

Copy link
Author

Choose a reason for hiding this comment

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

Updated

To prevent a phase from ending in the former case, raise and lower objections
inside tests.
To prevent a phase from ending in the latter case, raise objections in the
scoreboard whenever expect queues are not empty and lower them when they are empty.

Choose a reason for hiding this comment

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

I completely disagree with the comments in line 602 og 603.
I would say to avoid using objections in scoreboards and use them in monitors instead.

Raising objections in scoreboard can lead to deadlocks. Ideally objections should only be in the test.
but if needed a monitor can raise and objection in the Phase_ready_to_end phase if data is seen.

Also raising objections outside of a test should only be done in the phase_ready_to_end to improve performance.

maybe the topic of objections warrants separate thread?

Copy link

@sriyerg sriyerg Mar 5, 2020

Choose a reason for hiding this comment

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

I see some valid usecases in allowing raising/dropping objections in the scoreboard; example: in xbar type DUT, raise obj when transaction enters the DUT and drop only when it exits. This objection (that helps ensure that the test does not exit prematurely) cannot be captured in the monitor and has to be captured in a higher level component. It cannot be captured in the test either since it only generates and sends C-R stimulus to the driver. It has no knowledge of when the transactions actually enter and exit the DUT.

We keep the performance impact low by raising objection exactly once (dont raise if it is already raised), which is what the line implies. You can find that logic in our base scoreboard (hw/dv/sv/dv_lib/dv_base_scoreboard.sv). If by deadlocks you mean hangs, then there is an obvious problem either in the design or dv, that needs fixing. We avoid hanging forever through reasonable test timeouts.

I have not used phase_ready_to_end and would like to learn more about it. Let's discuss this in a separate thread.

Choose a reason for hiding this comment

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

I have created #30 do discuss further

### Randomization

Good constraint coding style is important for faster simulations and better
simulation performance.

Choose a reason for hiding this comment

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

I would suggest we recommend using a constraint name that matches what is being constrained.
for the example below I would suggest a naming scheme with either

a_con
con_a
or
c_a

Copy link
Author

Choose a reason for hiding this comment

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

That is a good point you bring up, the style that we have generally used so far is <constraint_name>_c, let us standardize that format if that sounds good to you, and I'll add it to the style guide?
@sriyerg @cindychip @weicaiyang what do you think?

Choose a reason for hiding this comment

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

Sure I agree with adding <constraint_name>_c

Copy link

Choose a reason for hiding this comment

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

+1 for _c suffix

Copy link
Contributor

Choose a reason for hiding this comment

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

agree with <constraint_name>_c, too

Choose a reason for hiding this comment

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

I am ok with _c also

@rasmus-madsen
Copy link

@senelson7 maybe you want to look at this too

@sjgitty
Copy link
Contributor

sjgitty commented Mar 4, 2020

@senelson7 maybe you want to look at this too

Thanks Rasmus for the input. There will be some style and philosophical
disagreements for sure, but it is great to get your opinions on it. @senelson7
please weigh in too.

@sriyerg @udinator see if you have good counter arguments for some of
the issues @rasmus-madsen has raised.

DVCodingStyle.md Outdated
Comment on lines 609 to 615
4. Always perform the test `if (starting_phase != null)` before calling
`raise_objection` or `drop_objection` within a sequence. Prior to uvm-1.2,
`starting_phase` was a member of the class `uvm_sequence_base`.
From uvm-1.2 onward, the `starting_phase` variable is deprecated and instead
must be accessed using the `get_starting_phase` method.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a version of UVM that we'll always target? If >= 1.2, this can be simplified.

Copy link
Author

Choose a reason for hiding this comment

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

@sriyerg what are your thoughts about this phrasing?

Copy link

Choose a reason for hiding this comment

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

At the moment we are using UVM 1.2 with both simulators, so wouldn't this be flagged as a compile error (and hence force the writer to update the code anyway)? I don't think we will ever revert back to an older version.

Copy link
Author

Choose a reason for hiding this comment

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

Updated to solely reflect usage of the get_starting_phase() method.

DVCodingStyle.md Outdated
mechanism, `uvm_root::set_timeout(<time_limit>, 1)`.
The base test in every testbench should specify a default timeout limit,
which can be overridden by every derivative test.
A standard plusarg is recommended to be used for this override mechanism.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we name the plusarg here?

Copy link
Author

Choose a reason for hiding this comment

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

yep good point, updated

Comment on lines +620 to +621
7. When calling `raise_objection` or `drop_objection`, you must pass a string
as a second argument to describe the objection to help with debugging.
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 sensible, but can we give examples? In particular, it's a bit less clear what sensible strings for drop_objection should be.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, this has been added.

@sjgitty
Copy link
Contributor

sjgitty commented Mar 13, 2020

Are we close to landing this? what are the remaining steps? We can always
amend as needed once landed.

@udinator
Copy link
Author

Are we close to landing this? what are the remaining steps? We can always
amend as needed once landed.

@sjgitty - There are a few minor updates that I need to make, corresponding to a few open comments in this PR, and a few bigger issues (end of test mechanism, and reactive agent usage).
@sriyerg , how would you feel if I added the sections about EoT using phase_ready_to_end and reactive agents in a separate PR, just to get the initial guide landed?

@sriyerg
Copy link

sriyerg commented Mar 13, 2020

Are we close to landing this? what are the remaining steps? We can always
amend as needed once landed.

@sjgitty - There are a few minor updates that I need to make, corresponding to a few open comments in this PR, and a few bigger issues (end of test mechanism, and reactive agent usage).
@sriyerg , how would you feel if I added the sections about EoT using phase_ready_to_end and reactive agents in a separate PR, just to get the initial guide landed?

Separate PR SGTM.

@rasmus-madsen
Copy link

@sriyerg is it on purpose that I have merge permissions in this repository?

@rasmus-madsen
Copy link

Are we close to landing this? what are the remaining steps? We can always
amend as needed once landed.

@sjgitty - There are a few minor updates that I need to make, corresponding to a few open comments in this PR, and a few bigger issues (end of test mechanism, and reactive agent usage).
@sriyerg , how would you feel if I added the sections about EoT using phase_ready_to_end and reactive agents in a separate PR, just to get the initial guide landed?

Can we at least make a comment in the styleguide that this section is still TDB/WIP

@sriyerg
Copy link

sriyerg commented Mar 13, 2020

@sriyerg is it on purpose that I have merge permissions in this repository?

I think this is just style guides repo - there is nothing critical here to prevent anyone from contributing to these docs. The project/repo admins (LR) can answer this better.

@udinator
Copy link
Author

Can we at least make a comment in the styleguide that this section is still TDB/WIP

Sure @rasmus-madsen - I've added a small note that the EoT points are still under discussion.

@udinator
Copy link
Author

@sjgitty , @sriyerg , if you are okay with the current state of the style guide, I can merge it into the repo, and make amendments in the future as necessary.

Signed-off-by: Udi Jonnalagadda <udij@google.com>
@sjgitty
Copy link
Contributor

sjgitty commented Mar 16, 2020

LGTM, I'd say let's ship it and amend where necessary

@udinator udinator merged commit d0dc0ac into lowRISC:master Mar 16, 2020
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.

None yet