Flash ctrl dv #28555
Merged
Flash ctrl dv #28555
+672
−161
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gautschimi
requested review from
rswarbrick and
vogelpi
and removed request for
a team
gautschimi
force-pushed
the
flash_ctrl_dv
branch
3 times, most recently
from
95acc90 to
f1e73ae
Compare
vogelpi
reviewed
Oct 24, 2025
There was a problem hiding this comment.
Thanks a lot @gautschimi for looking into this. I have some minor comments and a question.
Would you mind running a full regression (with VCS) with coverage enabled on this PR to verify it doesn't break other tests please?
Comment on lines
38
to
39
| localparam uint FLASH_DATA_BYTE_WIDTH = $clog2(flash_ctrl_top_specific_pkg::DataByteWidth); | ||
|
|
There was a problem hiding this comment.
This doesn't look write to me. DataByteWidth is vbits(8) = 3. If you take another $clog2 of that you get 2. Then below, you are using this to compute the base word address of different pages. Are you sure you end up with the right addresses?
There was a problem hiding this comment.
Indeed, that looks wrong. This function is only used for toplevel software tests, I need to run a few of them
hw/ip_templates/flash_ctrl/dv/env/seq_lib/flash_ctrl_basic_rw_vseq.sv
Outdated
Show resolved
Hide resolved
Comment on lines
331
to
345
| update_mp_region_cfg_mubifalse(region_cfg); | ||
| data = get_csr_val_with_updated_field(ral.mp_region_cfg[index].en, data, | ||
| region_cfg.en); | ||
| data = data | get_csr_val_with_updated_field(ral.mp_region_cfg[index].rd_en, data, | ||
| region_cfg.read_en); | ||
| data = data | get_csr_val_with_updated_field(ral.mp_region_cfg[index].prog_en, data, | ||
| region_cfg.program_en); | ||
| data = data | get_csr_val_with_updated_field(ral.mp_region_cfg[index].erase_en, data, | ||
| region_cfg.erase_en); | ||
| data = data | get_csr_val_with_updated_field(ral.mp_region_cfg[index].scramble_en, | ||
| data, region_cfg.scramble_en); | ||
| data = data | get_csr_val_with_updated_field(ral.mp_region_cfg[index].ecc_en, data, | ||
| region_cfg.ecc_en); | ||
| data = data | get_csr_val_with_updated_field(ral.mp_region_cfg[index].he_en, data, | ||
| region_cfg.he_en); | ||
| data = get_csr_val_with_updated_field(ral.mp_region_cfg[index].rd_en, data, | ||
| region_cfg.read_en); | ||
| data = get_csr_val_with_updated_field(ral.mp_region_cfg[index].prog_en, data, | ||
| region_cfg.program_en); | ||
| data = get_csr_val_with_updated_field(ral.mp_region_cfg[index].erase_en, data, | ||
| region_cfg.erase_en); | ||
| data = get_csr_val_with_updated_field(ral.mp_region_cfg[index].scramble_en, | ||
| data, region_cfg.scramble_en); | ||
| data = get_csr_val_with_updated_field(ral.mp_region_cfg[index].ecc_en, data, | ||
| region_cfg.ecc_en); | ||
| data = get_csr_val_with_updated_field(ral.mp_region_cfg[index].he_en, data, | ||
| region_cfg.he_en); |
There was a problem hiding this comment.
I am curious, can you explain why these changes are required and don't break other tests please? Because I can imagine that these tasks for the base sequence are quite fundamental and that they get used in almost all sequences.
There was a problem hiding this comment.
To be honest, I don't know what the purpose of this function is. I would say it should set all the CSRs with the updated values provided by the variable region_cfg. The function get_csr_val_with_updated_field() does exactly this. There is no need for an additional OR with the current value.
Infact, an additional OR will make mubi encodings invalid. (MuBi4True (0x6) | MuBi4False (0x9) = 0xF (invalid encoding)
-> the region encodings will always be invalid -> scrambling, ecc was mostly disabled
There was a problem hiding this comment.
Thanks for explaining, this makes a lot of sense.
gautschimi
force-pushed
the
flash_ctrl_dv
branch
2 times, most recently
from
eb9d025 to
58691fd
Compare
Adds a basic test that 1. configures scramble, ecc_en 2. programs the flash, 3. read back memory over the host interface, 4. read backs the memory over the controller interface Signed-off-by: Michael Gautschi <[email protected]>
FlashAddrWidth is in flash words (not bytes). Passing a byte address to a function which expects word-addresses leads to a truncation of the three most significant address bits. As a result, when accessing the upper part of each flash bank the scrambling tweak was wrong which lead to a wrong data encryption and hence also to a wrong ICV calculation for the expected responses. Signed-off-by: Michael Gautschi <[email protected]>
gautschimi
force-pushed
the
flash_ctrl_dv
branch
from
58691fd to
d3aa4e9
Compare
|
results of the latest coverage run: (corresponds to d3aa4e9) |
|
Thanks for updating the PR and for providing the regressions results. This all makes sense now and I see that not only pass rate increased quite substantially but also the functional coverage went slightly up once more. This looks good to me! |
vogelpi
approved these changes
Oct 27, 2025
vogelpi
added
the
CherryPick:earlgrey_1.0.0
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin earlgrey_1.0.0
git worktree add -d .worktree/backport-28555-to-earlgrey_1.0.0 origin/earlgrey_1.0.0
cd .worktree/backport-28555-to-earlgrey_1.0.0
git switch --create backport-28555-to-earlgrey_1.0.0
git cherry-pick -x 9581b9b742922756ced43cc2ceed907df26b46c4 d3aa4e9764fb730a0a1f61cf7b98ac3856914f22 |
lowrisc-ci
bot
added
the
Manually CherryPick
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin earlgrey_1.0.0
git worktree add -d .worktree/backport-28555-to-earlgrey_1.0.0 origin/earlgrey_1.0.0
cd .worktree/backport-28555-to-earlgrey_1.0.0
git switch --create backport-28555-to-earlgrey_1.0.0
git cherry-pick -x 9581b9b742922756ced43cc2ceed907df26b46c4 d3aa4e9764fb730a0a1f61cf7b98ac3856914f22 |
Open
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR:
The flash_controller DV dropped to ~75% success rate flash_ctrl_dv_results_march_2025 during this year. With these fixes, the flash_controller DV should achieve ~97% success rate again (similar to earlier this year: flash_ctrl_dv_results_jan_2025)