[RV_DM]rv_dm_progbuf_read_write_execute_vseq

[BasitMehmood]

This commit contains vseq and other realted files and verify the functioanlity of program buffer.

[BasitMehmood]

@jdonjdon kindly review it.

[BasitMehmood]

I am closing this and did a new PR of this in #21619.

[rswarbrick]

It's probably much easier to re-open this PR and force-push to the branch that it uses. There are actually merge conflicts in #21619 as well, so this should be easier.

I've just done that force-push myself (I hope it's ok!) with the following commands:

rjs@sole:~/work/opentitan$ git fetch basit
rjs@sole:~/work/opentitan$ git checkout -b xxx basit/progbuf_rw
Switched to a new branch 'xxx'
rjs@sole:~/work/opentitan$ git fetch origin
rjs@sole:~/work/opentitan$ # Rebase on master
rjs@sole:~/work/opentitan$ git push -f basit HEAD:progbuf_write_execute

[BasitMehmood]

I don't know why (Run DV smoke test) is failing?

[rswarbrick]

The DV smoke test is failing because the code doesn't compile. I think you're using a new request_halt task without actually declaring it :-)

The simple solution is probably to do the 2-commit approach that I suggested above.

[rswarbrick]

Hmm, something has gone a little bit awry on the git side. I've just pulled the head of this PR and rebased onto master and will push the result. We'll then only have 2 commits, rather than 51(!)

[rswarbrick]

Something has got a little bit shuffled around. I think what we want is probably two commits:

1. Define request_halt and use it in places that might need it. This should come from #21663, (and that PR needs a bit of extending)
2. Define progbuf_read_write_execute_vseq

I think there are probably some other improvements that we can make, but it looks to me like the best thing is to sort out the basic structure first!

[BasitMehmood]

Something has got a little bit shuffled around. I think what we want is probably two commits:

1. Define request_halt and use it in places that might need it. This should come from [RV_DM]rv_dm_base_vseq #21663, (and that PR needs a bit of extending)
2. Define progbuf_read_write_execute_vseq

I think there are probably some other improvements that we can make, but it looks to me like the best thing is to sort out the basic structure first!

I have defined halt_task in #21663 and use it in vseqs that need it, as you said. But for this PR 'progbuf_write_execute' there are changes in vseqs that use that task. So my concern is how to delete those files of vseqs in which I use that halt_task? Because in this PR we only want 'progbuf_read_write_execute_vseq' file.

[BasitMehmood]

@rswarbrick we can first merge #21663 branch where we defined halt_task and use it in vseqs. Then we can remove extra files(vseqs) in this branch that also uses that task, we can merge this PR as well. what do you think?

[rswarbrick]

All sorted, thanks. Let's rebase onto the current head of master and I think we should be good to review and merge :-)

[BasitMehmood]

There is a check in scoreboard line 363. Which compare the value that is written, against the csr.get_mirrored_value(). But this check failed because item.d_data has written value but in csr.get_mirrored_value() has 0 value. I think this check is for all tl_mem_ral csrs and compare against the written values.

[rswarbrick]

This is unchecked, but can't we simplify this a bit? I'm imagining something like

  task progbuf_rw(int idx);
    uvm_reg_data_t data = $urandom;
    uvm_reg_data_t r_data;

    csr_wr(.ptr(jtag_dmi_ral.progbuf[i]), .value(data));
    csr_rd(.ptr(tl_mem_ral.program_buffer[i]), .value(r_data));

    `DV_CHECK_EQ(data, r_data)
  endtask

  task body();
    // Call the task from base_vseq to halt the hart.
    request_halt();
    for (int i = 0; i < 8; i++) progbuf_rw(i);
  endtask

This is checking that DMI writes become visible over TileLink. Do we expect it to work in the other direction as well? If so, maybe we should be randomising the direction?

[BasitMehmood]

This is unchecked, but can't we simplify this a bit? I'm imagining something like

  task progbuf_rw(int idx);
    uvm_reg_data_t data = $urandom;
    uvm_reg_data_t r_data;

    csr_wr(.ptr(jtag_dmi_ral.progbuf[i]), .value(data));
    csr_rd(.ptr(tl_mem_ral.program_buffer[i]), .value(r_data));

    `DV_CHECK_EQ(data, r_data)
  endtask

  task body();
    // Call the task from base_vseq to halt the hart.
    request_halt();
    for (int i = 0; i < 8; i++) progbuf_rw(i);
  endtask

This is checking that DMI writes become visible over TileLink. Do we expect it to work in the other direction as well? If so, maybe we should be randomising the direction?

No, we are just checking that writes on dmi should visible on tilelink.

[rswarbrick]

I agree that this you're correctly describing what the code currently does :-) But I'm trying to say that this is currently testing a reasonably trivial property ("writes to X are reflected in Y"). For very little effort, I think we could test a stricter version (adding "... and vice versa"). Unless we don't believe that property should hold, I propose that we test it.

[BasitMehmood]

I agree that this you're correctly describing what the code currently does :-) But I'm trying to say that this is currently testing a reasonably trivial property ("writes to X are reflected in Y"). For very little effort, I think we could test a stricter version (adding "... and vice versa"). Unless we don't believe that property should hold, I propose that we test it.

We can only write to DMI and can read from TL. As tl_mem_ral registers are read-only. So, we can't do vice versa.

[BasitMehmood]

@rswarbrick now what's your thoughts on this PR?

[rswarbrick]

Sorry for the slow reply.

But I'm clearly missing something! Are you saying that it's impossible to do a CSR read over the DMI interface? I don't think that's true. Note that jtag_dmi_reg_frontdoor.sv believes that it's possible to do such a read.

[BasitMehmood]

Sorry for the slow reply.

But I'm clearly missing something! Are you saying that it's impossible to do a CSR read over the DMI interface? I don't think that's true. Note that jtag_dmi_reg_frontdoor.sv believes that it's possible to do such a read.

Oh there was misunderstanding. I thought your 'vice versa' means that writing on tilelink and reading from DMI. So that is why i said that we can't do that because tl_mem_ral registers are read only. But if you are trying to say that write on DMI and read from tilelink and from DMI as well? If so, that is possible. We can read from DMI interface as well.

[BasitMehmood]

@rswarbrick i have make changes in vseq as you said to add read csr over DMI interface . Also randomize the both directions. Make changes in testpoint entry according to implementation that we have done.

[rswarbrick]

I'm so sorry: I wrote something rather silly yesterday. You were pointing out that the TL CSR interface here is read-only, so it only makes sense to send data in one direction. And you're completely right!

Your recent change is completely sensible: trying to interpret my confused message in a way that makes sense! But I was just being stupid... The best thing is probably to drop the "else" case in your if/else block.

Otherwise, the change looks completely sensible. I'm really sorry for wasting your time.

[BasitMehmood]

I'm so sorry: I wrote something rather silly yesterday. You were pointing out that the TL CSR interface here is read-only, so it only makes sense to send data in one direction. And you're completely right!

Your recent change is completely sensible: trying to interpret my confused message in a way that makes sense! But I was just being stupid... The best thing is probably to drop the "else" case in your if/else block.

Otherwise, the change looks completely sensible. I'm really sorry for wasting your time.

I did commit now to drop changes that i committed earlier. Kindly review it.