opentitan icon indicating copy to clipboard operation
opentitan copied to clipboard

[dv] Create and deploy reset agent

Open martin-velay opened this issue 1 year ago • 5 comments

martin-velay avatar Nov 29 '24 16:11 martin-velay

Hi DV team (@rswarbrick, @hcallahan-lowrisc, @antmarzam, @KinzaQamar)

Here is the PR I was talking about earlier today. I was expecting to have something in better to share by the end of this day but unfortunately things didn't went well. It compiles, elaborate, start to run and fail for a reason I cannot explain yet. I'll work on it later next week. This PR will be split into multiple later as I think these changes can happen incrementally. I haven't done that so far as I was not sure of the implementation.

You can start to have a look and give me feedback on some points which are supposed to be done at my end:

  1. About the reset agent itself.
  2. About the way I have started to make the reset shared with the other components through TLM interface.

-> Other comments are welcome either

What I still need to address:

  1. Make sure the current implementation works.
  2. Splitting the dv_lib into 3: dv_base_agent_lib, dv_base_env_lib and reset_agent.
  3. Managing multiple reset agents, as discussed it's a must have for some env.
  4. Trigger on the fly resets from the IP specific base test when rand_reset_vseq is called (not from the base test from dv_lib for the moment as not all the IP will move to this way directly, or maybe do it via a config bit somewhere).
  5. Address all my comments tagged with "TODO MVy" (BTW if you have some comments on these I' be happy to have inputs on some).

-> Do you see anything else?

When all these points will be addressed and will work:

  • Make all DV env transitioning to reset agent adoption.
  • This should simplify some piece of code as highlighted with the "TODO MVy". And it will also uniformize some points.
  • Remove the old reset triggering way.

-> Do you see anything else?

Note: @rswarbrick, I don't find Marcelo for some reason. Can you help me ?

martin-velay avatar Nov 29 '24 17:11 martin-velay

@marcelocarvalhoLowRisc (I don't seem to be able to add Marcelo as a reviewer but I'm not entirely sure why. Maybe because he hasn't had any PRs merged yet?)

rswarbrick avatar Dec 02 '24 12:12 rswarbrick

@marcelocarvalhoLowRisc (I don't seem to be able to add Marcelo as a reviewer but I'm not entirely sure why. Maybe because he hasn't had any PRs merged yet?)

@marcelocarvalhoLowRisc was a member of our GitHub org but not part of the OpenTitan GitHub team. I've now invited him.

On a different note: I've talked to @martin-velay and as outlined in his description above this is a draft PR not intended to be merged as is. There is still work to be done to get this in a mergeable state (including splitting it up into smalller, reviewable chunks) but Martin wanted to give people a chance to take a look asap and if possible to provide early feedback to check this makes sense to others as well. I think this is actually perfectly fine but of course, once the implementation becomes more mature and this needs real review and merging, the size has to be reduced. Martin is aware of this.

FYI @rswarbrick , @mundaym .

vogelpi avatar Dec 06 '24 13:12 vogelpi

Status update:

I have pushed all my changes. This PR is still not ready, the reset is not happening as it should yet. I need to pause this activity to be focused on V3 activities.

What should be done next: find a way of disabling the main process when a reset is happening and then restart. This should probably imply to have a fork with a reset and main thread as done in many components, but this be done in the main virtual sequence. But the this main sequence shouldn't be killed by using the stop_sequences from the sequencer, otherwise will run at all. Instead, a task should be spawned from the main vseq and this task should be killed by calling disable fork and then restarted. This should happen as many as defined by a control knob which says how many resets should happen. By default this number will be equal to 1 (for initial reset). Add also a way to trigger a reset sequence on the fly from the main vseq.

Note to myself: run test util/dvsim/dvsim.py hw/ip/hmac/dv/hmac_sim_cfg.hjson -t xcelium -i hmac_smoke -gd --fixed-seed=1

martin-velay avatar Dec 06 '24 17:12 martin-velay

@martin-velay Thanks a lot for putting this together!

I've left a few comments, but overall it feels this PR is taking things on the right direction.

The only thing I'm missing is updated to dv_base_driver and a new seq_item in the dv_base_lib like dv_base_item that we have all seq_items extend from. In the DV driver, the task get_and_drive shouldn't be empty. They should be able to:

  1. get_item
  2. Do something with it (in the child base driver). But probably a pure virtual function in the dv_base_driver that will get populated in the child classes.
  3. call item_done.
  4. detect reset, and set a reset = 0/1 field in the seq_item, so that the UVM sequences can self-kill whenever reset is set.

P.S. You may need to rebase to the latest master when you get the chance!

antmarzam avatar Jul 18 '25 13:07 antmarzam