[dv,mem_bkdr_util] Separate out flash_bkdr_util

[alees24]

Separate flash support into another package so that designs such as Darjeeling can employ mem_bkdr_util without depending upon the flash controller/package.

[Razer6]

Are these dependencies all needed or do the already get pulled in from the mem_bkdr_util? Or is it just a safeguard?

[alees24]

I've dropped a couple but I think the rest are all needed. I would observe that many of our core files appear not to be as minimal as they could be if they relied upon transitivity. I am a bit apprehensive about breaking an untested build, and I'm sure CI is far from complete w.r.t. that.

[Razer6]

Looks good with one question.

[matutem]

Did you consider the way this same issue was handled for otp_ctrl? It basically moved the complete specialization in otp_ctrl out to hw/ip_templates/otp_ctrl/dv/env: look at https://github.com/lowRISC/opentitan/blob/master/hw/ip_templates/otp_ctrl/dv/env/otp_ctrl_mem_bkdr_util_pkg.sv and https://github.com/lowRISC/opentitan/blob/master/hw/ip_templates/otp_ctrl/dv/env/otp_ctrl_mem_bkdr_util.core.tpl.

Perhaps for flash_ctrl the flash_ctrl_mem_bkdr_util_pkg.sv will need to be a template, but at least in the otp_ctrl all specializations are accessed via the top specific pkg files.

I think that approach is cleaner since it removes all traces of the flash_ctrl details from mem_bkdr_util, and mem_bkdr_utils simply takes care of reads and writes, which is what it does for a living.

[matutem]

I left a comment with my suggestion for how to do this. Also refer to #25891.

[alees24]

Did you consider the way this same issue was handled for otp_ctrl? It basically moved the complete specialization in otp_ctrl out to hw/ip_templates/otp_ctrl/dv/env: look at https://github.com/lowRISC/opentitan/blob/master/hw/ip_templates/otp_ctrl/dv/env/otp_ctrl_mem_bkdr_util_pkg.sv and https://github.com/lowRISC/opentitan/blob/master/hw/ip_templates/otp_ctrl/dv/env/otp_ctrl_mem_bkdr_util.core.tpl.

I am familiar with the way otp_ctrl was extracted, yes, and I think that makes sense for an ip_template that needs to be specialized for the different top levels. The single additional function required for flash support is a little different, in my opinion, since we're not specializing the flash controller for Darjeeling; it's just not present in the integrated design.

Perhaps for flash_ctrl the flash_ctrl_mem_bkdr_util_pkg.sv will need to be a template, but at least in the otp_ctrl all specializations are accessed via the top specific pkg files.

I think that approach is cleaner since it removes all traces of the flash_ctrl details from mem_bkdr_util, and mem_bkdr_utils simply takes care of reads and writes, which is what it does for a living.

The separation in this PR does achieve that too. mem_bkdr_util no longer contains any reference to the flash package or any flash-specific functionality, which allows the Darjeeling top-level to brought up alongside that of Earl Grey for the first time, and simulations of both are now possible on my local branch without crosstalk between the two environments.

If it's felt that the flash_bkdr_util package is too specific to belong in the dv/sv/ directory - and the same argument could apply to ROM- and SRAM-specific behavior - it could always be moved later since it's now self-contained.

[Razer6]

I am familiar with the way otp_ctrl was extracted, yes, and I think that makes sense for an ip_template that needs to be specialized for the different top levels. The single additional function required for flash support is a little different, in my opinion, since we're not specializing the flash controller for Darjeeling; it's just not present in the integrated design.

👍

[matutem]

I see this is a very simple change, but it adds clutter rather than addressing root causes. Basically this change will need to be undone and done the right way, so this kicks the can down the road hoping someone else will do the real fix, fully knowing what that fix should be.

[alees24]

Implementing rom_, sram_ and flash_bkdr_util as extended classes of mem_bkdr_util means that they may be accessed without requiring specialized code and without needing to know what type of memory is being accessed, as requested here some time ago:

opentitan/hw/top_earlgrey/dv/env/seq_lib/chip_sw_base_vseq.sv

Line 704 in 6efc747

virtual function void mem_bkdr_read8(input chip_mem_e mem,

Also at a few other sites in the top-level DV where you can see things become tidier.
flash_bkdr_util presently does not have specialized implementations of read/write access functions but perhaps it should, and similarly for the other memories, modeling what the hardware blocks actually do.

This PR only separates the files into different packages and core files and I should have realized that was necessary when the extended classes were introduced; mea culpa.

[andreaskurth]

I see this is a very simple change, but it adds clutter rather than addressing root causes. Basically this change will need to be undone and done the right way, so this kicks the can down the road hoping someone else will do the real fix, fully knowing what that fix should be.

Thanks for your feedback, @matutem. If I understand you correctly, your position is that this change should be made in a different way, right now in this PR, but I don't understand how exactly you'd prefer things to be done. You mentioned the templated otp_ctrl backdoor utils, but AFAICS that's different because otp_ctrl is templated and different for Earlgrey and Darjeeling, whereas flash_ctrl doesn't exist for Darjeeling at all. Hence the simple separation, as @alees24 implemented in this PR, makes more sense to me than templating flash_ctrl backdoor utils.

Could you please elaborate how you'd prefer flash_bkdr_util to be changed?

[matutem]

My point is that mem_bkdr_util should just worry about raw reads and writes, and any of the memory specific concerns should be in the dv code for the specific memory to avoid pollution. The otp_ctrl flagged this because of top-specificity, and I am glad it flagged it, since the memory specific code should not have been placed in mem_bkdr_util in the first place.

If we did this for flash_ctrl then darjeeling would not have needed to do anything at all since it doesn't use flash_ctrl.

The flash_bkdr_util class as it exists is pretty dependent on flash_phy, which is not very generic, and hw/dv/sv is meant for generic stuff. Also notice the code is replicated from the existing one under hw/ip_templates/flash_ctrl/dv/env.

As for the implementation of this, the only function affected is flash_write_scrambled, which is from the top dv/env/seq_lib code called as <mem_bjdr_instance>.flash_write_scrambled(...), and this can be changed to flash_write_scrambled(<mem_bkdr_instance>, ...). The function itself can be placed in flash_ctrl_env_pkg, for example.

[alees24]

Okay, I think we can go further in tidying up mem_bkdr_util than the work already done in #26045, but since this is more work than intended at this stage of Darjeeling bring up, please let's reach agreement first.

Firstly I was unaware of the duplication of code between flash_bkdr_util and flash_ctrl; this predates my involvement and I had not looked at flash_ctrl DV in detail. Thanks for drawing attention to that; it is clearly undesirable and should be eliminated.

The base class mem_bkdr_util presently still contains references to SRAM scrambling and ROM, particularly in its _pkg, so what if I restructure it as follows:

  hw/ip/sram_ctrl/dv/env/
    - sram_bkdr_util.sv
    - sram_bkdr_util.core - moving the SRAM-specific scrambling away from hw/dv/sv/mem_bkdr_util

  hw/ip/rom_ctrl/dv/env/
    - rom_bkdr_util.sv
    - rom_bkdr_util.core - similarly moving the ROM-specific behavior out of hw/dv/sv/mem_bkdr_util

  hw/ip_templates/flash_ctrl/dv/env/
    - flash_bkdr_util.sv,
    - flash_bkdr_util_pkg.sv
    - flash_bkdr_util.core - moving the flash-related behavior away from mem_bkdr_util

This leaves just:

  hw/dv/sv/mem_bkdr_util/
    - mem_bkdr_util.sv
    - mem_flash_util_pkg.sv
    - mem_flash_util.core - just the generic memory-related functions, and the read8/write8()
        functions can be overridden in the derived classes to perform the additional work -
        scrambling and unscrambling - that occurs on read/write accesses to those
        memories in the DUT.

Perhaps in the above the specialized types of memory model should really be called something like 'rom_model.sv', 'flash_model.sv' rather than '.._bkdr_util.' The test bench code in each top level can then perform read/write acceses to each of the created memory models without having to contain special checks for the memory type; this was clearly the intent of the code here:

opentitan/hw/top_earlgrey/dv/env/seq_lib/chip_sw_base_vseq.sv

Line 602 in 32b1337

// TODO: Need to deal with scrambling.

and in the other related functions performing backdoor accesses. The intention was to permit direct read/write operations on the Flash Data parititions too, which makes it a bit different from the OTP controller as I understand things.

opentitan/hw/top_earlgrey/dv/env/seq_lib/chip_sw_base_vseq.sv

Line 634 in 32b1337

`DV_CHECK_FATAL(mem inside {Rom, [RamMain0:RamMain15], FlashBank0Data, FlashBank1Data},

Whilst Darjeeling does not have flash memory, I understand there has repeatedly been talk of future OpenTitan designs employing 'execute in place' flash in which case we shall need to keep the ability to read/write symbols in the data partitions that the flash controller presents to the host.

I would contend that the flash interface has more in common with the ROM and SRAM than with the OTP controller, and that this is exactly the kind of situation for which inheritance exists.

@andreaskurth , @matutem , @Razer6 , @rswarbrick - thoughts please?