Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: earlgrey_1.0.0
Choose a base branch
from
+103 −10
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 20 additions & 3 deletions sw/device/tests/flash_ctrl_idle_low_power_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
#include "sw/device/lib/testing/autogen/isr_testutils.h"

OTTF_DEFINE_TEST_CONFIG();

static uint32_t flash_region_index;
Copy link
Contributor

@AlexJones0 AlexJones0 Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

static dif_rv_plic_t plic;
static dif_aon_timer_t aon;
static dif_rv_core_ibex_t rv_core_ibex;
Expand All @@ -43,7 +43,8 @@ static top_earlgrey_plic_peripheral_t peripheral_serviced;
static dif_aon_timer_irq_t irq_serviced;

enum {
kFlashDataRegion = 2, // The ROM_EXT protects itself using regions 0-1.
// kFlashDataRegion = 7, // The ROM_EXT protects itself using regions 0-1.
kFlashCtrlParamNumRegions = 7,
kRegionBasePageIndex =
256 + 32, // First non-ROM_EXT page in bank 1 (avoids program code.)
kPartitionId = 0,
Expand Down Expand Up @@ -126,8 +127,24 @@ bool test_main(void) {
rstmgr_reset_info = rstmgr_testutils_reason_get();

uint32_t address = 0;

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

@AlexJones0 AlexJones0 Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

bool locked;
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);
Comment on lines +133 to +138
Copy link
Contributor

@AlexJones0 AlexJones0 Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

break;
}
}

// CHECK_STATUS_OK(flash_ctrl_testutils_data_region_setup(
// &flash, kRegionBasePageIndex, kFlashDataRegion, kRegionSize, &address));

CHECK_STATUS_OK(flash_ctrl_testutils_data_region_setup(
&flash, kRegionBasePageIndex, kFlashDataRegion, kRegionSize, &address));
&flash, kRegionBasePageIndex, flash_region_index, kRegionSize, &address));

if (rstmgr_reset_info == kDifRstmgrResetInfoPor) {
// Create data. Random data will be different than
Expand Down
30 changes: 29 additions & 1 deletion sw/device/tests/flash_ctrl_ops_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ enum {
kPageSize = 2048,
};

const uint32_t FLASH_CTRL_PARAM_NUM_REGIONS = 7;
Copy link
Contributor

@AlexJones0 AlexJones0 Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const uint32_t kRandomData1[kInfoSize] = {
0xb295d21b, 0xecdfbdcd, 0x67e7ab2d, 0x6f660b08, 0x273bf65c, 0xe80f1695,
0x586b80db, 0xc3dba27e, 0xdc124c5d, 0xb01ccd52, 0x815713e1, 0x31a141b2,
Expand Down Expand Up @@ -310,6 +311,18 @@ static void do_bank1_data_partition_test(void) {
(i == 0) ? flash_bank_1_page_index : flash_bank_1_page_index_scr;
const uint32_t *test_data = (i == 0) ? kRandomData4 : kRandomData5;

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;
}
}
Comment on lines +314 to +325
Copy link
Contributor

@AlexJones0 AlexJones0 Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

if (i == 0) {
// Set region1 for non-scrambled ecc enabled.
CHECK_STATUS_OK(flash_ctrl_testutils_data_region_setup(
Expand Down Expand Up @@ -423,16 +436,29 @@ static void do_bank1_data_partition_test(void) {
bool test_main(void) {
flash_info = dif_flash_ctrl_get_device_info();

// for(uint32_t region = 0; region < FLASH_CTRL_PARAM_NUM_REGIONS+1; region++)
// {
// bool locked;
// 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);
// }
// break;
// }

// Determine the region index and page index to use for tests.
// Test data page used for flash bank 1 should be the lowest and highest
// usable page.
if (kBootStage != kBootStageOwner) {
flash_bank_0_data_region = 0;
flash_bank_1_page_index = flash_info.data_pages;
LOG_INFO("Running as owner");
Copy link
Contributor

@AlexJones0 AlexJones0 Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
// If we boot up in owner stage, the first 2 regions will be used by
// ROM_EXT.
flash_bank_0_data_region = 2;
flash_bank_0_data_region = 3;
// First 0x20 pages are configured by ROM_EXT. To avoid conflicts, skip over
// these pages.
flash_bank_1_page_index = flash_info.data_pages + 0x20;
Expand Down Expand Up @@ -460,7 +486,9 @@ bool test_main(void) {
do_info_partition_test(kFlashInfoPageIdOwnerSecret, kRandomData2);
do_info_partition_test(kFlashInfoPageIdIsoPart, kRandomData3);
do_bank0_data_partition_test();
LOG_INFO("Running as not owner");
}
LOG_INFO(" Non -owner");
Copy link
Contributor

@AlexJones0 AlexJones0 Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This log should probably be removed?

do_bank1_data_partition_test();

return true;
Expand Down
30 changes: 28 additions & 2 deletions sw/device/tests/flash_ctrl_write_clear_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@

OTTF_DEFINE_TEST_CONFIG();

static uint32_t flash_region_index;
static uint32_t flash_page_to_test;

enum {
// Some dif_flash_ctrl functions don't require the partition parameter when
// interacting with data partitions. This constant is added to improve
Expand All @@ -40,7 +43,7 @@ enum {
kBank1StartPageNum = 256 + kRomExtPageCount,

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

@AlexJones0 AlexJones0 Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


};

Expand Down Expand Up @@ -105,6 +108,29 @@ bool test_main(void) {
CHECK_DIF_OK(dif_flash_ctrl_init_state(
&flash, mmio_region_from_addr(TOP_EARLGREY_FLASH_CTRL_CORE_BASE_ADDR)));

flash_region_index = 0;
flash_page_to_test = 0;
for (uint32_t region = 0; region < FLASH_CTRL_PARAM_NUM_REGIONS; region++) {
bool locked;
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);
break;
}
dif_flash_ctrl_data_region_properties_t cfg;
CHECK_DIF_OK(
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);
Copy link
Contributor

@AlexJones0 AlexJones0 Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}
}

// kFlashRegionNum = flash_region_index;

// The ROM_EXT configures the default region access. We can't modify the
// values after configured.
if (kBootStage != kBootStageOwner) {
Expand All @@ -114,7 +140,7 @@ bool test_main(void) {
}

LOG_INFO("ECC enabled with high endurance disabled.");
flash_ctrl_write_clear_test(/*mp_region_index=*/kFlashRegionNum,
flash_ctrl_write_clear_test(/*mp_region_index=*/flash_region_index,
(dif_flash_ctrl_data_region_properties_t){
.base = kBank1StartPageNum,
.size = 1,
Expand Down
30 changes: 26 additions & 4 deletions sw/device/tests/rv_core_ibex_mem_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,9 @@ enum {
kFlashTestLoc = TOP_EARLGREY_FLASH_CTRL_MEM_BASE_ADDR +
kBank1StartPageNum * kFlashBytesPerPage,
// The ROM_EXT protects itself using regions 0-1.
kFlashRegionNum = 2,
kFlashRegionNum = 5,
Copy link
Contributor

@AlexJones0 AlexJones0 Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};
static uint32_t flash_region_index;

// The flash test location is set to the encoding of `jalr x0, 0(x1)`
// so execution can be tested.
Expand Down Expand Up @@ -162,12 +163,33 @@ static void setup_flash(void) {
.ecc_en = kMultiBitBool4False,
.high_endurance_en = kMultiBitBool4False};
dif_flash_ctrl_data_region_properties_t data_region = {
.base = kBank1StartPageNum, .size = 0x1, .properties = region_properties};
.base = kBank1StartPageNum, .size = 0x2, .properties = region_properties};

// Added RP - Check for unlocked region
for (uint32_t region = 0; region < FLASH_CTRL_PARAM_NUM_REGIONS; region++) {
bool locked;
LOG_INFO("Testing REGION 0x%x", region);
CHECK_DIF_OK(
dif_flash_ctrl_data_region_is_locked(&flash_ctrl, region, &locked));
if (!locked) {
// We can use this region
flash_region_index = region;
LOG_INFO("This region is unlocked REGION 0x%x", region);
break;
}
// dif_flash_ctrl_data_region_properties_t cfg;
// CHECK_DIF_OK(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);
}
// Check for unlocked region ends

CHECK_DIF_OK(dif_flash_ctrl_set_data_region_properties(
&flash_ctrl, kFlashRegionNum, data_region));
&flash_ctrl, flash_region_index, data_region));
CHECK_DIF_OK(dif_flash_ctrl_set_data_region_enablement(
&flash_ctrl, kFlashRegionNum, kDifToggleEnabled));
&flash_ctrl, flash_region_index, kDifToggleEnabled));

// Make flash executable
CHECK_DIF_OK(
Expand Down
Loading