[top/dv] Update RV_DM chip-level testplan #8009

weicaiyang · 2021-08-31T20:44:47Z

Updated testplan as we discussed in the meeting
Signed-off-by: Weicai Yang weicai@google.com

weicaiyang · 2021-08-31T20:46:03Z

hw/top_earlgrey/data/chip_testplan.hjson

@@ -418,7 +418,8 @@
    {
      name: chip_jtag_csr_rw
      desc: '''
-            Verify accessibility of CSRs as indicated in the RAL specification.
+            Verify accessibility of CSRs as indicated in the RAL specification, including CSRs in
+            other blocks.


@sriyerg this test is not very clear to me. After review all the test entries here, I think we want to test all CSRs in the chip through jtag, right?

Seems like there's a bit of confusion ongoing here that mixes the chip_jtag_csr_rw and chip_rv_dm_cpu_debug_mem tests.

For the chip_jtag_csr_rw test, we discussed that we want to test the access to the Debug Module Registers (https://github.com/pulp-platform/riscv-dbg/blob/master/doc/debug-system.md#debug-module-registers) through JTAG by doing a read-write round-trip test. Since most of these registers have side effects, we tried to pick one or two registers which are known to be "plain" registers.

How about a text like this:

- Write RW type Debug Module Registers (such as progbuf0 and progbuf1) through JTAG with a random value. - Read back to check their values for correctness.

Please also rename the test from chip_jtag_csr_rw to e.g. chip_jtag_debug_module_register_rw to make it clear what type of register we're talking about.

@imphil per our discussion, this test is to verify RV_DM registers. Then I realized we don't have test to access SBA as you mentioned in another comment.

Then, I feel this test isn't only testing the RV_DM CSRs but also testing other module's CSRs.

We already have a chip_csr_rw test to verify all CSRs through TL-UL. I think that's the reason that Sri named it chip_jtag_csr_rw. And we reuse the chip_csr_rw by switching the jtag agent to create this test, so that it tests RV_DM CSRs as well as other CSRs through SBA.
@sriyerg please correct me if I'm wrong

@imphil I have confirmed with @sriyerg offline.
The intent of this test is to reuse chip_csr_rw to use jtag to test all the CSRs in the chip. so this will also test SBA functionality.

Thanks for checking. In this case, please reword to clearly indicate what is meant by "CSR" and which "RAL specification" is meant (is there one for rv_dm?).

Updated the wording. It should be all the CSRs in the chip. If it's only rv_dm CSR, then we should test that in block-level. top-level is to cover interaction with other blocks.

The rv_dm section of the testplan originally was written with likely incorrect assumptions. The point of the testplan review was to fix those assumptions and update it to reflect the current state of the design as it is integrated into the chip. I assume that's done (IIRC I was unable to make it to that review). I will go with the fact that @weicaiyang has updated the testplan based on your (@imphil & @GregAC) comments and it is now up-to-date.

imphil

Thanks @weicaiyang for your update. There seems to be a bit of confusion left, let me know if my comments help.

Furthermore, I just realized that we don't have any test for the System Bus Access functionality (https://github.com/pulp-platform/riscv-dbg/blob/master/doc/debug-system.md#system-bus-access). Can you add one, e.g.

Verify access to the TL-UL bus through JTAG

Write a random data word into the main SRAM through JTAG using rv_dm's SBA functionality.

Read the same data word back and check for equality.

imphil · 2021-09-01T09:11:36Z

hw/top_earlgrey/data/chip_testplan.hjson

@@ -418,7 +418,8 @@
    {
      name: chip_jtag_csr_rw
      desc: '''
-            Verify accessibility of CSRs as indicated in the RAL specification.
+            Verify accessibility of CSRs as indicated in the RAL specification, including CSRs in
+            other blocks.


Seems like there's a bit of confusion ongoing here that mixes the chip_jtag_csr_rw and chip_rv_dm_cpu_debug_mem tests.

For the chip_jtag_csr_rw test, we discussed that we want to test the access to the Debug Module Registers (https://github.com/pulp-platform/riscv-dbg/blob/master/doc/debug-system.md#debug-module-registers) through JTAG by doing a read-write round-trip test. Since most of these registers have side effects, we tried to pick one or two registers which are known to be "plain" registers.

How about a text like this:

- Write RW type Debug Module Registers (such as progbuf0 and progbuf1) through JTAG with a random value. - Read back to check their values for correctness.

Please also rename the test from chip_jtag_csr_rw to e.g. chip_jtag_debug_module_register_rw to make it clear what type of register we're talking about.

imphil · 2021-09-01T09:19:28Z

hw/top_earlgrey/data/chip_testplan.hjson

+            - CPU programs RW type CSRs (such as progbuf0 and progbuf1) with random value.
+            - Read back to check their values for correctness.


If the goal here is to access the debug memory (https://github.com/pulp-platform/riscv-dbg/blob/master/doc/debug-system.md#debug-memory), the core needs to be in debug mode first. Since we'll already access the debug memory as part of the more complete debug test (named chip_rv_dm_cpu_debug_req currently) below, you can either remove this test completely, or make it a negative test ("ensure that debug memory is not accessible while not in debug mode").

Thanks Philipp.
I think it makes sense to make it a negative test. How can we know it's not accessible from a C program? Does it just return 0 when we read the debug memory?

imphil · 2021-09-01T09:37:48Z

hw/top_earlgrey/data/chip_testplan.hjson

            '''
      milestone: V2
      tests: []
    }
    {
      name: chip_rv_dm_jtag_debug_mem
      desc: '''Verify access to the debug mem from the external JTAG interface.
+
+            - Load debug ROM and read back through jtag to check the values for correctness.


I'm not quite sure what this test was meant to test for. The debug ROM is neither readable not writable through JTAG. The program buffer, which is part of the debug RAM is readable and writable through JTAG, but already tested below in the test that's currently named chip_rv_dm_cpu_debug_req.

Probably just remove this test?

https://github.com/pulp-platform/riscv-dbg/blob/master/doc/debug-system.md#debug-memory for more information on the debug memory and its organization.

Thanks for correcting me. Removed this test.

imphil · 2021-09-01T09:38:33Z

hw/top_earlgrey/data/chip_testplan.hjson

            '''
      milestone: V2
      tests: []
    }
    {
      name: chip_rv_dm_jtag_debug_mem
      desc: '''Verify access to the debug mem from the external JTAG interface.
+
+            - Load debug ROM and read back through jtag to check the values for correctness.
            '''
      milestone: V2
      tests: []
    }
    {
      name: chip_rv_dm_cpu_debug_req


Due to observability constraints, this test is now expanded to test a full debug cycle. So let's name it accordingly, e.g. "chip_rv_dm_perform_debug" or so.

sg, renamed it.

imphil · 2021-09-01T09:44:17Z

hw/top_earlgrey/data/chip_testplan.hjson

            '''
      milestone: V2
      tests: []
    }
    {
      name: chip_rv_dm_jtag_debug_mem
      desc: '''Verify access to the debug mem from the external JTAG interface.
+
+            - Load debug ROM and read back through jtag to check the values for correctness.
            '''
      milestone: V2
      tests: []
    }
    {
      name: chip_rv_dm_cpu_debug_req
      desc: '''Verify debug request to Ibex while it is actively executing.


how about: "Verify that Ibex enters debug mode when triggered through JTAG"

Updated, thanks

imphil · 2021-09-01T09:47:07Z

hw/top_earlgrey/data/chip_testplan.hjson

+            - Program some CSRs / mem that are under life cycle reset tree and system reset tree.
+            - Configure RV_DM to send NDM reset request to reset sytem reset tree.
+            - Read the programmed CSRs / mem to ensure that everything under system reset tree is
+              reseted to the original values, while values under life cycle reset will be


Suggested change

reseted to the original values, while values under life cycle reset will be

reset to the original values, while values under life cycle reset will be

imphil · 2021-09-01T09:47:35Z

hw/top_earlgrey/data/chip_testplan.hjson

+              reseted to the original values, while values under life cycle reset will be
+              preserved.
+            - Read CSRs / mem in the debug domain to ensure that the values survive the reset.
+            X-ref'ed with the rstmgr tests


Please add a "TODO" here -- or do these tests already exist?

I don't find that test in rstmgr. Added TODO.

I've just been using the debug module with a simple Ibex based system and think I've found a bug with the non-debug reset functionality. If we're in the halted state (where we're in debug mode in Ibex and anyhalted/allhalted will be set in the DMI dmstatus debug register) and issue an non-debug reset the debug module still believes Ibex is halted.

Still looking at a fix but need to make sure this case is covered here. I think you want to test the non-debug reset both when Ibex is and isn't halted (from the DM point of view). You want to check the debug module doesn't leave anyhalted/allhalted set after the reset.

@GregAC I don't mind either way, but it seems to me that this test should go into the block-level test, not the chip-level test. That said, I don't think we even have a IP level testplan yet for rv_dm.

Opened an issue against the Pulp debug module repo here discussing the problem: pulp-platform/riscv-dbg#118

Lets add a testpoint for the scanario @GregAC pointed out at the chip level as well.

As for the Xref line, lets just remove it to avoid confusion.

Added an entry for the scenario @GregAC pointed out.

imphil · 2021-09-01T09:47:54Z

hw/top_earlgrey/data/chip_testplan.hjson

-            TODO: `rv_dm` currently is not on the AON domain, so this feature does not exist ATM.
-            Need discussion with SW/Nuvoton.
+            - Put the chip into sleep mode and then wake up.
+            - Access some RV_DM CSRs to ensure that RV_DM doesn’t need a full reset to work.


nit: replace the non-ASCII '

Good catch. Fixed. Thanks

imphil · 2021-09-01T09:48:35Z

hw/top_earlgrey/data/chip_testplan.hjson

@@ -488,8 +502,9 @@
      name: chip_rv_dm_lc_disabled
      desc: '''Verify that the debug capabilities are disabled in certain life cycle stages.

-            Verify that the debug mem is inaccessible from the CPU as well as external JTAG.
-            Details TBD. X-ref'ed with the LC tests.
+            - Put life cycle on states othter than Test, RMA and DEV.


Suggested change

- Put life cycle on states othter than Test, RMA and DEV.

- Put life cycle on states other than Test, RMA and DEV.

Fixed. Thanks

imphil · 2021-09-01T09:49:21Z

hw/top_earlgrey/data/chip_testplan.hjson

-            Verify that the debug mem is inaccessible from the CPU as well as external JTAG.
-            Details TBD. X-ref'ed with the LC tests.
+            - Put life cycle on states othter than Test, RMA and DEV.
+            - Verify that the debug mem is inaccessible from the CPU as well as external JTAG.


Suggested change

- Verify that the debug mem is inaccessible from the CPU as well as external JTAG.

- Verify that the rv_dm bus device is inaccessible from the CPU.

- Verify that the JTAG TAP is unavailable.

The JTAG access and the rv_dm bus device are rather different things, let's have two bullet points for it.

Make sense. Updated.

imphil · 2021-09-09T12:23:46Z

@sriyerg we'll need your input on this PR to move forward. PTAL if you have a moment.

sriyerg · 2021-10-20T00:33:45Z

I will look to @imphil and @GregAC for approval.

hw/top_earlgrey/data/chip_testplan.hjson

sriyerg · 2021-10-20T00:35:26Z

hw/top_earlgrey/data/chip_testplan.hjson

-      name: chip_rv_dm_jtag_debug_mem
-      desc: '''Verify access to the debug mem from the external JTAG interface.
+      name: chip_rv_dm_cpu_debug_mem_not_accessable
+      desc: '''Verify that the debug mem can't be accessed from the CPU while not in the debug mode.


Missing stimulus and checks.

@imphil do you know what the scenario should be, in order to prove debug_mem isn't accessible when it's not in debug_mode.
Can we control ibex to read the debug_mem address locations and check the values? Thanks

We'd want a test that attempts to read, write and execute from the debug memory, seeing a synchronous exception on all of these accesses.

Is blocking off the debug memory something that's happening external to Ibex? As I don't think Ibex does this internally

it's not clear to me. Since I have a TODO here, perhaps let's figure it out later.

hw/top_earlgrey/data/chip_testplan.hjson

sriyerg · 2021-10-20T00:42:51Z

hw/top_earlgrey/data/chip_testplan.hjson

+              reseted to the original values, while values under life cycle reset will be
+              preserved.
+            - Read CSRs / mem in the debug domain to ensure that the values survive the reset.
+            X-ref'ed with the rstmgr tests


Lets add a testpoint for the scanario @GregAC pointed out at the chip level as well.

As for the Xref line, lets just remove it to avoid confusion.

Updated testplan as we discussed in the meeting Signed-off-by: Weicai Yang <weicai@google.com>

weicaiyang · 2021-10-27T21:59:10Z

@imphil @GregAC @sriyerg I have addressed all your comments. PTAL, thanks

GregAC · 2021-11-03T18:03:18Z

hw/top_earlgrey/data/chip_testplan.hjson

-      name: chip_rv_dm_jtag_debug_mem
-      desc: '''Verify access to the debug mem from the external JTAG interface.
+      name: chip_rv_dm_cpu_debug_mem_not_accessable
+      desc: '''Verify that the debug mem can't be accessed from the CPU while not in the debug mode.


We'd want a test that attempts to read, write and execute from the debug memory, seeing a synchronous exception on all of these accesses.

Is blocking off the debug memory something that's happening external to Ibex? As I don't think Ibex does this internally

weicaiyang requested review from tjaychen, imphil, timothytrippel, cindychip, sriyerg, tomeroberts and dsandrs August 31, 2021 20:44

weicaiyang commented Aug 31, 2021

View reviewed changes

imphil reviewed Sep 1, 2021

View reviewed changes

weicaiyang force-pushed the rv_dm branch from e70a6a4 to bae916c Compare September 2, 2021 05:13

weicaiyang requested a review from imphil September 2, 2021 05:14

weicaiyang force-pushed the rv_dm branch from bae916c to 356e70e Compare September 16, 2021 05:23

sriyerg reviewed Oct 20, 2021

View reviewed changes

[top/dv] Update RV_DM chip-level testplan

be0249a

Updated testplan as we discussed in the meeting Signed-off-by: Weicai Yang <weicai@google.com>

weicaiyang force-pushed the rv_dm branch from 356e70e to be0249a Compare October 26, 2021 22:45

GregAC approved these changes Nov 3, 2021

View reviewed changes

weicaiyang merged commit 9c931d3 into lowRISC:master Nov 4, 2021

weicaiyang deleted the rv_dm branch November 4, 2021 20:38

		- CPU programs RW type CSRs (such as progbuf0 and progbuf1) with random value.
		- Read back to check their values for correctness.

	reseted to the original values, while values under life cycle reset will be
	reset to the original values, while values under life cycle reset will be

	- Put life cycle on states othter than Test, RMA and DEV.
	- Put life cycle on states other than Test, RMA and DEV.

	- Verify that the debug mem is inaccessible from the CPU as well as external JTAG.
	- Verify that the rv_dm bus device is inaccessible from the CPU.
	- Verify that the JTAG TAP is unavailable.

[top/dv] Update RV_DM chip-level testplan #8009

[top/dv] Update RV_DM chip-level testplan #8009

Conversation

weicaiyang commented Aug 31, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sriyerg Oct 20, 2021 • edited

Choose a reason for hiding this comment

imphil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imphil commented Sep 9, 2021

sriyerg commented Oct 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weicaiyang commented Oct 27, 2021

Choose a reason for hiding this comment

sriyerg Oct 20, 2021 •

edited