[test-triage] Issues with ADC's chip_sw_adc_ctrl_sleep_debug_cable_wakeup

Jacob-Levy commented 2 years ago

Test point name

[chip_sw_adc_ctrl_sleep_debug_cable_wakeup](https::// github.com/lowRISC/opentitan)

Host side component

SystemVerilog

OpenTitanTool infrastructure implemented

Unknown

Contact person

Jacob-Levy

Checklist

Please fill out this checklist as items are completed. Link to PRs and issues as appropriate.

[ ] Check if existing test covers most or all of this testpoint (if so, either extend said test to cover all points, or skip the next 3 checkboxes)
[ ] Device-side (C) component developed
[ ] Bazel build rules developed
[ ] Host-side component developed
[ ] HJSON test plan updated with test name (so it shows up in the dashboard)
[ ] Test added to dvsim nightly regression (and passing at time of checking)

Jacob-Levy commented 2 years ago

Assert fail: HwIdSelCheck_A

"../src/lowrisc_prim_subreg_0/rtl/prim_reg_cdc_arb.sv", 169: tb.dut.top_earlgrey.u_adc_ctrl_aon.u_reg.u_adc_chn_val_0_cdc.u_arb.gen_wr_req.HwIdSelCheck_A: started at 18363300000ps failed at 18363300000ps Offending '$past(dst_update_i, 1)' UVM_ERROR @ 18363.300000 us: (prim_reg_cdc_arb.sv:169) [ASSERT FAILED] HwIdSelCheck_A "../src/lowrisc_prim_subreg_0/rtl/prim_reg_cdc_arb.sv", 169:
tb.dut.top_earlgrey.u_adc_ctrl_aon.u_reg.u_adc_chn_val_1_cdc.u_arb.gen_wr_req.HwIdSelCheck_A: started at 18363300000ps failed at 18363300000ps Offending '$past(dst_update_i, 1)' UVM_ERROR @ 18363.300000 us: (prim_reg_cdc_arb.sv:169) [ASSERT FAILED] HwIdSelCheck_A

Assert fail: WakeUpChnSelIn (NTIL assert at chip level) the channel select is being set after ~15us although the spec defines “Wait 30 uS for the ADC to wake up” . (in this test adc_pd_ctl.pwrup_time was set to 2). Need to be minimum 6!

@sha-ron @ariel9678

tjaychen commented 2 years ago

@Jacob-Levy regarding the 1st issue, that's something that should be purely on the open source side. Can you let me know how you produced that? We had some issues with that assertion some time ago, and there's been open source side updates to fix it. Are you running on top of tree right now?

For the second one, if i understand correctly, it is mostly a change in configuration register is that right? Is it possible for you to add this assertion into the open source model? Then we can make sure we catch these things in regressions.

Jacob-Levy commented 2 years ago

@tjaychen We are using open source chip-level tests as a starting point by adapting them to the NTIL chip-level extended environment. For the 1st issue we failed on NTIL first and when we run the test on OS we noticed it failed the same way.

We might be missing the open source update that address the issue on the OS and on the NTIL. Can you direct us to the update?

@ariel9678 Can you please show the command that you run on the OS chip-level to replicate issue (1).

As for issue (2), The assert in question was added to the NTIL chip-level among other things. The ones that apply to OS chip-level (not related to security) should be added to the OS chip-level and not be part of the extend.

@sha-ron Can you please add the relevant non-secure asserts to the OS chip-level.

tjaychen commented 2 years ago

wait really? it failed in OS also? let me check that then. Maybe there's some corner case with the cdc reg handling that's not done.

for the assert, it would just be helpful to have something that isn't proprietary in the open that checks for interface expectations. That way we can help catch these things in the open and not let them fall onto you guys to debug only.

Jacob-Levy commented 2 years ago

@tjaychen We are using 2-weeks old opentitan version. When did you incorporated the update? In any case, I asked @sha-ron to move to the latest opentitan early next week.

tjaychen commented 2 years ago

this is the fix i'm referring to. can you check to see if your local area contains it?

Jacob-Levy commented 2 years ago

@tjaychen The update was merged 3-weeks ago. I will check the local version for the prim_reg_cdc_arb.sv version. Will also run the test on the latest OS version tomorrow.

Jacob-Levy commented 2 years ago

@tjaychen The test was run by @ariel9678 on the latest version and PASSED.

So, we left with issue (2) that will be handled next week after we review all the checks that was added in NTIL and are valid for the OS too. We will also fix the configuration to avoid the ASSERT.

tjaychen commented 2 years ago

@Jacob-Levy assigning to you for now. Once you guys update, let me know and I can have a look.

Jacob-Levy commented 2 years ago

Issue (1) resolved by PR #15047. Issue (2) requiers ast_spec_checks.sv that includes all the assertion that check the AST Spec restrictions like the "30us wait after power-up"..

engdoreis commented 1 year ago

Closing this issue based on the latests results:

Tests	latest	2022.09.28	2022.09.27	2022.09.26/1	2022.09.26/2	2022.09.24	2022.09.23	2022.09.22/1	2022.09.22/2	...	2022.09.20/2	2022.09.18	2022.09.17	2022.09.16	2022.09.15/1	2022.09.15/2	2022.09.13	2022.09.12	2022.09.11	Suite
chip_sw_adc_ctrl_sleep_debug_cable_wakeup	100.00	100.00	100.00	100.00	66.67	100.00	100.00	100.00	100.00	...	100.00	100.00	100.00	100.00	100.00	100.00	100.00	100.00	100.00	chip_sw_adc_ctrl_sleep_debug_cable_wakeup

engdoreis commented 1 year ago

Should we open an issue for the second item mentioned by @Jacob-Levy previously?

tjaychen commented 1 year ago

Should we open an issue for the second item mentioned by @Jacob-Levy previously?

ah yes good point. @Jacob-Levy did mention there will be discussions whether it can be added to open source, so keeping track of it would be good.

lowRISC / opentitan