[uart] Signoff Checklist by eunchan · Pull Request #615 · lowRISC/opentitan

eunchan · 2019-10-24T21:29:17Z

This is Proof-of-Concept for UART Signoff process.
It has checklist for each milestones. It is expected to be
auto-generated by the tool later. It is expected to be revised only to
Resolution and Note/Collaterals.

The doc is prepared by @weicaiyang, @sriyerg, and @eunchan.

Review Doc: https://docs.google.com/document/d/1Dnf61zPXDAgQbyQ-HrSqP3mKQTonv9ulUqLc_8siNq4/edit?ts=5db7195b#

sriyerg · 2019-10-24T21:37:45Z

Just say "Top level design module that meets the comportability requirements is made available."
Also, cross link it to comportability doc.

I will suggest using "design" instead of IP everywhere since we will use these checklist for the top level as well.

sriyerg · 2019-10-24T21:41:44Z

Maybe change this to "All ports for the top level design as indicated in the specification "?
If we say spec is complete, i think the design should have all the ports added in. If there are changes down the road, those modifications can happen later. WDYT?

I think we'd better remove this item. as IP_TOP covers this.

sriyerg · 2019-10-24T21:43:50Z

Should this be done after V1?

Please refer the actual checklist spreadsheet. It has the discussion about this.

sriyerg · 2019-10-24T21:45:57Z

Please add section headings here for ease of navigation.

### Documentation #### DV_PLAN_DRAFT_COMPLETE

sriyerg · 2019-10-24T21:46:44Z

Perhaps make these a bulleted list (here and below)?

sriyerg · 2019-10-24T21:49:13Z

Is this really needed? The act of filling this out and submitting a PR is in itself reflective of this checklist item, which obviates the need for it.

Make sense.

sjgitty

I put a bunch of comments here, primarily on the checklist template itself.
Not sure if you were intending that to be finalized yet, it still needs a little
bit of work on the wording.

sjgitty · 2019-10-24T22:57:06Z

Might be worth one sentence describing the purpose, something like:

This signoff checklist provides the recommended items to review when
transitioning from one [Hardware Stage](pointer to hw_stages.md) to
another, for both design and verification stages.

Or something like that.

Probably also want an explanatory:

Under each stage (D1, D2, V1, etc) is a list of criteria, with the name (eg. SPEC_COMPLETE)
and the list of items expected to complete the checklist.

sjgitty · 2019-10-24T23:34:09Z

Need a sentence, something like:

For a transition from D0 --> D1, the following items should be completed.

sjgitty · 2019-10-24T23:35:59Z

I'm misreading this sentence. I think you're intending to say

The unit must not break top level functionality, such as propagating Xs through TLUL interfaces.

sjgitty · 2019-10-24T23:36:33Z

.md files (except for READMEs) should have one sentence per file per MD guidelines.

I thought both (80 char, a sentence per line) are permitted. Generally, a sentence per line is good for review but the raw text isn't that great to view.

sjgitty · 2019-10-24T23:37:10Z

How about:

It is acceptable to have lint errors and warnings at this stage.

sjgitty · 2019-10-24T23:44:17Z

type name says V1, text says V2

sjgitty · 2019-10-24T23:45:03Z

same sentence as above about D0 --> D1 transition

sjgitty · 2019-10-24T23:45:56Z

sentence needed

sjgitty · 2019-10-24T23:48:45Z

need to have a brief sentence here.

This checklist is for Hardware Stage transitions for the UART peripheral.
All checklist items refer to the content in the Checklist File

or something like that.

sjgitty · 2019-10-24T23:49:46Z

Are we changing this to checklist.md instead of sign_off.md?
I might suggest we move it to doc/project instead of doc/rm
but I'll clean that up in another pass.

asb

Hi Eunchan, many thanks for putting this together. A couple of hopefully easy requests:

Separate out the addition of doc/rm/sign_off.md to a separate commit in the PR - it doesn't make sense under the title of "[uart] Signoff Checklist". I suggest "[doc/rm] Add signoff checklist definitions" or similar.
Sort out the unrelated changes in the commit - to uart.hjson, uart.sv, top_earlgrey.gen.hjson and top_earlgrey.sv

eunchan · 2019-10-29T06:37:56Z

Hi Eunchan, many thanks for putting this together. A couple of hopefully easy requests:

Separate out the addition of doc/rm/sign_off.md to a separate commit in the PR - it doesn't make sense under the title of "[uart] Signoff Checklist". I suggest "[doc/rm] Add signoff checklist definitions" or similar.

Sort out the unrelated changes in the commit - to uart.hjson, uart.sv, top_earlgrey.gen.hjson and top_earlgrey.sv

Thanks. I've removed the design change here as #666 was created and contains exact changes there. Also split the commit into two commits, one for signoff.md and another for uart_checklist.md.

asb

Removing my requested changes. This seems good to me in terms of approach and setting up the template markdown file. I defer to @sjgitty and others to mark that the checklist accurately reflects what was discussed in the meeting, which I wasn't present for.

One formatting note: much of these files seem to be wrapped at 80 chars, while we now prefer one line per sentence.

weicaiyang · 2019-10-31T22:59:57Z

do you want to change 2 in-progress item to done?

@moidx I didn't change the SW_FATAL_ERR, could you please update on this item?

LGTM based on DV test inspection, and interface review against current use cases.

Thanks for update!

weicaiyang

LGTM
will create another PR to sign off V3, along with new coverage waiver file

sjgitty · 2019-10-31T23:39:53Z

don't forget to update uart.prj.hjson with that PR

…

On Thu, Oct 31, 2019 at 4:30 PM weicaiyang ***@***.***> wrote: ***@***.**** approved this pull request. LGTM will create another PR to sign off V3, along with new coverage waiver file — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#615?email_source=notifications&email_token=AJZQKVPQUBZZBPTCPUUXQBDQRNTBVA5CNFSM4JE3VLF2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCJ64PSA#pullrequestreview-310233032>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJZQKVLVBYV7ARMOOQVQ7W3QRNTBVANCNFSM4JE3VLFQ> .

eunchan · 2019-10-31T23:49:33Z

On Thu, Oct 31, 2019 at 04:39:56PM -0700, Scott Johnson wrote: don't forget to update uart.prj.hjson with that PR On Thu, Oct 31, 2019 at 4:30 PM weicaiyang ***@***.***> wrote: > ***@***.**** approved this pull request. > > LGTM > will create another PR to sign off V3, along with new coverage waiver file > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#615?email_source=notifications&email_token=AJZQKVPQUBZZBPTCPUUXQBDQRNTBVA5CNFSM4JE3VLF2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCJ64PSA#pullrequestreview-310233032>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AJZQKVLVBYV7ARMOOQVQ7W3QRNTBVANCNFSM4JE3VLFQ> > . >

OK. at first, I thought this should be the completed signoff PR (D3,V3). If not, let me change the commit title also.

aytong · 2019-11-01T01:22:03Z

Mark Resolution as Done

aytong · 2019-11-01T01:23:02Z

@weicaiyang are the Xcelium warnings fixed?

aytong

once the remaining V3 item is resolved, update V3 reviewer/signoff rows, and hjson to V3.

msfschaffner

LGTM. Don't forget to mark D3 Review as Done...

moidx

LGTM

eunchan · 2019-11-01T21:13:09Z

@asb @sjgitty All the checklist items are completed. Could you please approve this PR. Then we explicitly sign off UART version 1.

sjgitty

one down!

sriyerg · 2019-11-02T00:24:04Z

Perhaps remove line 182?

You mean "TESTPLAN_COMPLETED"?

Line 182 of this updated file is a bullet point under "PRELIMINARY_ASSERTION_CHECKS_ADDED"

- All DUT outputs have X / unknown checks when out of reset

eunchan · 2019-11-02T00:29:40Z

OK.. @martin-lueker WDYT?

sjgitty · 2019-11-02T01:22:36Z

OK.. @martin-lueker WDYT?

While it would be good to have external review, @martin-lueker didn't attend the
signoff review this time, so perhaps @aytong can be the second 👍 here.

martin-lueker · 2019-11-02T04:45:43Z

Hi @eunchan, and all.
Thanks for the notification. This looks really good! I went over the final test spec, browsed the dv directory, read the signoff minutes, and of course read this signoff check list. I think it is great to see this system in place. It is great to see so many eyes on this. It is great to see a formal documentation chain, and fantastic to see this level of review as we move this to V3. This is so reassuring. Great job!
My only concerns are minor, and regard (1.) the review of the coverage databases themselves, and (2) the system for pulling this back to V2 if any of the relevant files is ever changed. Before I go into this more, I will to admit that most of my uncertainty may stem from my admitted ignorance on some of the following points:

How does one browse the VCS code-coverage history? (I saw reference that they were on bubble somewhere. Can we add coverage URLs to the signoff? Will the coverage summaries have history?) If I wanted to be completely skeptical, is there someplace where I could go to check that the code-coverage history was fully reset at the last RTL & DV change and built up from there? I saw the exclusion lists, but where are the actual coverage databases and statistics posted?
Regarding the functional coverage points. Same questions as above: Is the coverage database history posted going back to the last edit? Though after looking at the definition of the covergroups in uart/dv/env/uart_env_cov.sv. I realized that I may have a lot of catching up to do in understanding how these coverage points are defined. However looking at the revision in the "signoff" branch it seems the coverage does not consider baud rate or parity settings, but does cover direction level and reset. (Am I reading this correctly?) However the DV test spec does say that baud rate and parity settings would be included. (Maybe these are done in separate runs?)
My other concern is about maintenance of V3 status. Does this IP automatically go back to V2 if any of the supporting files are ever edited? Once this is in hjson format, will there be bots searching for such "V3 disqualifiers"? If a gap in the coverage definition is ever brought forward, will the diligent contributor get tarred and feathered for suggesting a return to V2? ;) Is there a similar review process for maintaining V3 status in the event of objections?

However putting aside these concerns about the exact nature of the coverage definitions, and diligence about maintaining V3 status, I think this looks awesome. This is exactly the level of documentation I was hoping when I first read the lifecycle docs.
Nice job everyone.

@rasmus-madsen @senelson7 @tunghoang290780, @smcmaster66 I'm interested in hearing your thoughts as well.

weicaiyang · 2019-11-03T04:50:49Z

@martin-lueker

Can we add coverage URLs to the signoff?

Agree. I think we can check in the coverage report generated by VCS if Synopsis is OK for that. It's a nice report and provides detail statistics of code and function coverage. @sriyerg WDYT?

Though after looking at the definition of the covergroups in uart/dv/env/uart_env_cov.sv. I realized that I may have a lot of catching up to do in understanding how these coverage points are defined. However looking at the revision in the "signoff" branch it seems the coverage does not consider baud rate or parity settings, but does cover direction level and reset. (Am I reading this correctly?) However the DV test spec does say that baud rate and parity settings would be included. (Maybe these are done in separate runs?)

There is another cov object in uart_agent (hw/dv/sv/uart_agent/uart_agent_cov.sv), which covers baud rate, parity setting etc. These cover-points are protocol related, so we put them in uart_agent. The others (like interrupt, fifo) are in uart/dv/env/uart_env_cov.sv. Will add file links for these coverage in dv plan.

sjgitty · 2019-11-04T21:42:41Z

should be @weicaiyang

sjgitty · 2019-11-04T21:43:00Z

please add @sriyerg

sjgitty · 2019-11-04T21:43:09Z

please add @sriyerg

sjgitty · 2019-11-04T21:43:19Z

please add @sriyerg

After helding the signoff meeting (2019-10-28), the UART IP now reached D3/V3 stage at 2019-11-01. Checklist document and project Hjson are updated accordingly. Co-authored-by: Weicai Yang <weicai@google.com>

This is temporary fix until the IP versioning discussion is completed. As PR lowRISC#965 merged, the Azure Pipeline failed on newer PRs. ```log Build log #L1010 Register headers not up-to-date. Regenerate them with 'make -C hw regs'. ``` PR lowRISC#965 didn't include `gpio`, `uart`, `rv_timer` from the collaterals as those IPs were signed off ( lowRISC#676 lowRISC#615 lowRISC#652 ). The azure pipeline checks every IPs' generated register modules. It is good as it can catch this case, which prevents silent upgrade of internal modules in the signed-off IPs. But it is also annoying every PRs from now failed. So, temporary `hw/Makefile` ignores signed-off IPs until better IP versioning rule is suggested. This is related to lowRISC#975 Signed-off-by: Eunchan Kim <eunchan@opentitan.org>

eunchan requested review from asb, sjgitty, sriyerg and weicaiyang October 24, 2019 21:29

sriyerg reviewed Oct 24, 2019

View reviewed changes

sjgitty reviewed Oct 24, 2019

View reviewed changes

sriyerg reviewed Oct 25, 2019

View reviewed changes

Comment thread hw/ip/uart/doc/uart_checklist.md Outdated

eunchan requested review from msfschaffner and tjaychen October 28, 2019 18:11

eunchan marked this pull request as ready for review October 28, 2019 18:11

This was referenced Oct 28, 2019

[dv] Add cov related targets in Makefile and coverage exclusion file #635

Merged

[rv_timer] Timer signoff #652

Merged

eunchan changed the title ~~PoC: [uart] Signoff Checklist~~ [uart] Signoff Checklist Oct 29, 2019

asb suggested changes Oct 29, 2019

View reviewed changes

asb approved these changes Oct 29, 2019

View reviewed changes

sriyerg mentioned this pull request Oct 31, 2019

[aes] Update design and verification stages #702

Merged

weicaiyang reviewed Oct 31, 2019

View reviewed changes

weicaiyang approved these changes Oct 31, 2019

View reviewed changes

aytong reviewed Nov 1, 2019

View reviewed changes

Comment thread hw/ip/uart/doc/checklist.md Outdated

Copy link
Copy Markdown

aytong Nov 1, 2019

Choose a reason for hiding this comment

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

Mark Resolution as Done

aytong reviewed Nov 1, 2019

View reviewed changes

Comment thread hw/ip/uart/doc/checklist.md Outdated

Copy link
Copy Markdown

aytong Nov 1, 2019

Choose a reason for hiding this comment

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

@weicaiyang are the Xcelium warnings fixed?

aytong reviewed Nov 1, 2019

View reviewed changes

msfschaffner approved these changes Nov 1, 2019

View reviewed changes

moidx approved these changes Nov 1, 2019

View reviewed changes

sjgitty approved these changes Nov 2, 2019

View reviewed changes

sriyerg reviewed Nov 2, 2019

View reviewed changes

sriyerg approved these changes Nov 2, 2019

View reviewed changes

sjgitty reviewed Nov 4, 2019

View reviewed changes

Comment thread hw/ip/uart/data/uart.prj.hjson Outdated

sjgitty reviewed Nov 4, 2019

View reviewed changes

sjgitty approved these changes Nov 4, 2019

View reviewed changes

[uart] Reaching D3/V3 stage

4166794

After helding the signoff meeting (2019-10-28), the UART IP now reached D3/V3 stage at 2019-11-01. Checklist document and project Hjson are updated accordingly. Co-authored-by: Weicai Yang <weicai@google.com>

eunchan merged commit 4166794 into lowRISC:master Nov 4, 2019

eunchan deleted the signoff branch November 4, 2019 21:48

eunchan mentioned this pull request Nov 13, 2019

[CI] Remove signed off IPs from the register module checker #977

Merged

marnovandermaas mentioned this pull request May 14, 2026

D1 sign-off: UART lowRISC/mocha#496

Closed

Conversation

eunchan commented Oct 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjgitty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

asb left a comment

Choose a reason for hiding this comment

Uh oh!

eunchan commented Oct 29, 2019

Uh oh!

asb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weicaiyang left a comment

Choose a reason for hiding this comment

Uh oh!

sjgitty commented Oct 31, 2019 via email

Uh oh!

eunchan commented Oct 31, 2019 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eunchan commented Oct 24, 2019 •

edited

Loading

sriyerg Nov 2, 2019 •

edited

Loading

martin-lueker commented Nov 2, 2019 •

edited

Loading