[sival/flash_ctrl] Modify flash_ctrl test for checking flash read/write #28627

rprakas-gsc · 2025-10-31T17:56:42Z

This commit updates the flash_ctrl test files to check for unlocked regions before flash operations.

…rite This commit updates the flash_ctrl test files for read/write access before flash operations. Signed-off-by: Ramesh Prakash <[email protected]>

AlexJones0

This PR still seems to have several instances of commented-out code and old comments. Ideally, if not needed these would be removed or if they are being kept for some purpose (i.e. to be re-introduced after a TODO is resolved) then they would be annotated with a comment as such. I haven't commented on these specifically as there are multiple occurrences.

As an aside - what is the motivation for this change? I think these should currently be correct, at least for FPGA. Is the goal to enable these tests to run in environments with more/different flash memory protection regions configured?

AlexJones0 · 2025-11-04T14:31:48Z

sw/device/tests/flash_ctrl_idle_low_power_test.c


 OTTF_DEFINE_TEST_CONFIG();
-
+static uint32_t flash_region_index;


This looks like it would probably be more appropriate as a local variable to test_main, rather than a global?

AlexJones0 · 2025-11-04T14:34:33Z

sw/device/tests/flash_ctrl_idle_low_power_test.c


  uint32_t address = 0;
+
+  for (uint32_t region = 0; region < kFlashCtrlParamNumRegions + 1; region++) {


Because there are 8 regions, I think that really this should instead define

kFlashCtrlParamNumRegions = 8,

or even better:

kFlashCtrlParamNumRegions = FLASH_CTRL_PARAM_NUM_REGIONS,

and then just use region = 0; region < kFlashCtrlParamNumRegions. This applies to each of the modified files with this change.

It might also be nice to add a comment here e.g. "Find the first unlocked flash region and use that for testing".

AlexJones0 · 2025-11-04T14:36:40Z

sw/device/tests/flash_ctrl_idle_low_power_test.c

+    LOG_INFO("Testing  REGION 0x%x", region);
+    CHECK_DIF_OK(dif_flash_ctrl_data_region_is_locked(&flash, region, &locked));
+    if (!locked) {
+      // We can use this region
+      flash_region_index = region;
+      LOG_INFO("This region is unlocked REGION 0x%x", region);


Nit: the logging here is a bit verbose - could this just be one log message like:

Region %u is {locked/unlocked}.

Also, as another nit: there are only 8 reasons, no reason to use hex (%x) over decimal (%u) here.

(this also applies to other files with this change).

AlexJones0 · 2025-11-04T14:39:32Z

sw/device/tests/flash_ctrl_ops_test.c

  kPageSize = 2048,
 };

+const uint32_t FLASH_CTRL_PARAM_NUM_REGIONS = 7;


Nit: our style guide prefers that this be an enum constant (and should probably also be 8, as before).

AlexJones0 · 2025-11-04T14:44:35Z

sw/device/tests/flash_ctrl_ops_test.c

+    for (uint32_t region = 0; region < FLASH_CTRL_PARAM_NUM_REGIONS; region++) {
+      bool locked;
+      // CHECK_DIF_OK(dif_flash_ctrl_data_region_is_locked(&flash_state,
+      // flash_bank_1_data_region, &locked));
+      CHECK_DIF_OK(
+          dif_flash_ctrl_data_region_is_locked(&flash_state, region, &locked));
+      if (!locked) {
+        flash_bank_1_data_region = region;
+        LOG_INFO("This region is unlocked REGION 0x%x", region);
+        break;
+      }
+    }


This code is repeated across tests, and it should ideally also handle the case where all flash regions are locked. Also, for this test specifically, I think you need to find another region for flash_bank_1_data_region_scr as that is by default configured as flash_bank_1_data_region + 1, but flash_bank_1_data_region is updated.

Maybe this could be refactored into a flash_ctrl test util, e.g.

status_t flash_ctrl_testutils_find_unlocked_region( dif_flash_ctrl_state_t *flash_state, uint32_t start, uint32_t end, uint32_t *region);

AlexJones0 · 2025-11-04T14:53:24Z

sw/device/tests/flash_ctrl_ops_test.c

  if (kBootStage != kBootStageOwner) {
    flash_bank_0_data_region = 0;
    flash_bank_1_page_index = flash_info.data_pages;
+    LOG_INFO("Running as owner");


Nit: this log and the one below are incorrect (this is the not owner conditional branch).

Also, it seems to me that the code configuring flash_bank_0_data_region is stale with the new functionality introduced above, and so should be removed, to avoid making the test more confusing.

AlexJones0 · 2025-11-04T14:53:36Z

sw/device/tests/flash_ctrl_ops_test.c

    do_bank0_data_partition_test();
+    LOG_INFO("Running as not owner");
  }
+  LOG_INFO(" Non -owner");


This log should probably be removed?

AlexJones0 · 2025-11-04T14:55:52Z

sw/device/tests/flash_ctrl_write_clear_test.c


  // The ROM_EXT protects itself using regions 0-1.
-  kFlashRegionNum = 2,
+  kFlashRegionNum = 3,


If this is hardcoded to 3, should the above comment be updated to reflect why?

It seems like this might currently work by using the loop that finds the first unlocked region to find region 2 (previously specified) and then hardcoding region 3 for the HE enabled test. But this is hard to follow and no better than just hardcoding two separate regions.

AlexJones0 · 2025-11-04T14:58:06Z

sw/device/tests/flash_ctrl_write_clear_test.c

+        dif_flash_ctrl_get_data_region_properties(&flash, region, &cfg));
+    // Avoid this region
+    if (cfg.size > 0) {
+      flash_page_to_test = MAX(flash_page_to_test, cfg.base + cfg.size);


Nit: this logic assumes regions are assigned sequentially in flash memory, which is not true. I think this is a reasonable concession to simplify testing, but it should be documented as such.

But also, flash_page_to_test does not seem to be used?

AlexJones0 · 2025-11-04T15:02:31Z

sw/device/tests/rv_core_ibex_mem_test.c

                  kBank1StartPageNum * kFlashBytesPerPage,
  // The ROM_EXT protects itself using regions 0-1.
-  kFlashRegionNum = 2,
+  kFlashRegionNum = 5,


Same comment as for flash_ctrl_write_clear_test, but is this not unused in this test now?

rprakas-gsc requested a review from a team as a code owner October 31, 2025 17:56

rprakas-gsc requested review from engdoreis and removed request for a team October 31, 2025 17:56

[sival/flash_ctrl] Modify flash_ctrl test scripts for checking read/w…

744cc87

…rite This commit updates the flash_ctrl test files for read/write access before flash operations. Signed-off-by: Ramesh Prakash <[email protected]>

rprakas-gsc force-pushed the earlgrey_1.0.0 branch from c2c578f to 744cc87 Compare November 2, 2025 19:49

rprakas-gsc mentioned this pull request Nov 2, 2025

Flash ctrl changes #28617

Closed

rprakas-gsc assigned rprakas-gsc and luismarques and unassigned rprakas-gsc Nov 3, 2025

AlexJones0 reviewed Nov 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[sival/flash_ctrl] Modify flash_ctrl test for checking flash read/write #28627

[sival/flash_ctrl] Modify flash_ctrl test for checking flash read/write #28627

rprakas-gsc commented Oct 31, 2025 •

edited

Loading

Uh oh!

AlexJones0 left a comment

Uh oh!

AlexJones0 Nov 4, 2025

Uh oh!

AlexJones0 Nov 4, 2025

Uh oh!

AlexJones0 Nov 4, 2025

Uh oh!

AlexJones0 Nov 4, 2025

Uh oh!

AlexJones0 Nov 4, 2025

Uh oh!

AlexJones0 Nov 4, 2025

Uh oh!

AlexJones0 Nov 4, 2025

Uh oh!

AlexJones0 Nov 4, 2025

Uh oh!

AlexJones0 Nov 4, 2025

Uh oh!

AlexJones0 Nov 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants


		OTTF_DEFINE_TEST_CONFIG();

		static uint32_t flash_region_index;


		uint32_t address = 0;

		for (uint32_t region = 0; region < kFlashCtrlParamNumRegions + 1; region++) {

[sival/flash_ctrl] Modify flash_ctrl test for checking flash read/write #28627

Are you sure you want to change the base?

[sival/flash_ctrl] Modify flash_ctrl test for checking flash read/write #28627

Conversation

rprakas-gsc commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexJones0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rprakas-gsc commented Oct 31, 2025 •

edited

Loading