[rom_ctrl,dv] Override timeouts more cleanly in rom_ctrl_base_vseq

[rswarbrick]

This behaviour will work more predictably and will also avoid warnings from VCS. I think that warning is saying that the behaviour with the previous code will depend on what the compiler knows about the type of a vseq. We can do a bit better in a trivial way, so let's do that instead.

Without this change, the warnings look like:

  Warning-[MDVMO] Multiple default values in method override
  ../src/lowrisc_dv_rom_ctrl_env_0.1/seq_lib/rom_ctrl_base_vseq.sv, 126
  rom_ctrl_env_pkg, "rom_ctrl_env_pkg_rom_ctrl_base_vseq_11_0::tl_access"
    The class method argument 'tl_access_timeout_ns' has different default
    values specified at the base
    ("../src/lowrisc_dv_cip_lib_0/seq_lib/cip_base_vseq.sv", 178) and the
    current definition.
    VCS resolves default arguments of functions at compile time so the current
    one will be used regardless of virtual override.

  Warning-[MDVMO] Multiple default values in method override
  ../src/lowrisc_dv_rom_ctrl_env_0.1/seq_lib/rom_ctrl_base_vseq.sv, 156
  rom_ctrl_env_pkg, "rom_ctrl_env_pkg_rom_ctrl_base_vseq_11_0::tl_access_w_abort"
    The class method argument 'tl_access_timeout_ns' has different default
    values specified at the base
    ("../src/lowrisc_dv_cip_lib_0/seq_lib/cip_base_vseq.sv", 204) and the
    current definition.
    VCS resolves default arguments of functions at compile time so the current
    one will be used regardless of virtual override.

(@antmarzam: Does this match what you'd expect?)

[rswarbrick]

Hmm. This doesn't actually work with the newest versions of VCS because I end up calling tl_access_w_abort without enough arguments (which it now notices!). It's all rather ugly but looks like we have to duplicate the default arguments. I'll rejig accordingly.

[antmarzam]

How about setting tl_access_timeout_ns to 0x0 as a default, and if an user wants a custom timeout they can override it (even if it's a smaller value than cfg.tl_access_timeout_ns).
Then you could do instead:
tl_access_timeout_ns = tl_access_timeout_ns==0 ? cfg.tl_access_timeout_ns : tl_access_timeout_ns

[rswarbrick]

Hmm, good point: that would be nicer. I'll do that now.

[rswarbrick]

Thinking about it further: that's not completely trivial. We still can't have a different default argument value from cip_base_vseq. But we could change the base class to use zero and override it using the mechanism you're suggesting. That would allow e.g. a user setting the timeout to 1ns. But we statically know that this doesn't really make sense!

I suppose another approach would be to model the state in the sequence slightly: "Use a longer timeout until at least one operation has completed since the last reset". Maybe that would make sense?

[antmarzam]

I think it's probably best to leave the best class as is in case touching it has fallout for other TBs.

Perhaps a solution could be instead of overriding the existing function, to declare a new task in the scope of this TB (maybe in the package?) and call the new one instead of the base one via using the scope operator?

Although if this TB only calls this task to use the default timeout or a greater custom one the solution you proposed originally is probably the easiest one

[rswarbrick]

I've just spent a couple of hours debugging because the existing code doesn't work with VCS 2023.03-SP2 and instead ends up blowing the stack because the super call just calls itself. My change in this PR doesn't fix things. Boo.

Will try to tidy it up next week.