drivers: flash: bug fix stm32 ospi flash erase #53598

tu-brian · 2023-01-09T08:33:30Z

This commit fixes a bug in the STM32 ospi flash driver when attempting to erase an area that spans more than one erase sector. Without this fix, only the first sector is actually erased, the rest silently fails the erase.
Issue is that the write enable latch command is only sent for the first erase command.

samples/drivers/spi_flash is updated to check for this condition.

Tested with: west build -b b_u585i_iot02a samples/drivers/spi_flash

Signed-off-by: Brian Juel Folkmann [email protected]

erwango · 2023-01-09T08:36:56Z

Hit the wrong button. Not reviewed yet.

drivers/flash/flash_stm32_ospi.c

DhruvaG2000

In my humble opinion the commit message need not have,

Tested with: west build -b b_u585i_iot02a samples/drivers/spi_flash

In general commit message dont have tested with ... Commit should explain just what the change is doing and that should be enough.

samples/drivers/spi_flash is updated to check for this condition.

We can see with git stat which file is being edited, I think this too can be dropped.

Otherwise the commit message is fine. I am yet to review the code changes.

DhruvaG2000

Feels like you are doing one-too many things as part of a single commit, please see if you can split up these changes.
In my humble opinion we can split up the changes in sample and driver into 2 commits.
I would even split the sample changes because you are doing multiple things in the sample itself, reading, erasing and writing the flash.
I would strongly suggest splitting up these changes into 2-3 commit messages suiting each change.
Also note that each commit should independently build and be functional, so split it keeping this in mind. I dont see much new additions like change of API's so this should not be too difficult a task.

FRASTM · 2023-01-10T07:26:36Z

drivers/flash/flash_stm32_ospi.c

Does the code of the flash_stm32_ospi_write() function fits here :

Suggested change

if (stm32_ospi_write_enable(&dev_data->hospi,

ret = stm32_ospi_write_enable(&dev_data->hospi,

dev_cfg->data_mode, dev_cfg->data_rate);

if (ret != 0) {

LOG_ERR("Erase failed : write enable");

break;

```

Thanks, I will update this

FRASTM · 2023-01-10T07:29:21Z

drivers/flash/flash_stm32_ospi.c

Is it really needed ?
when breaking the
ospi_unlock_thread(dev); comes just before the return ret; out of the while loop

I will fix this

erwango

samples/drivers/spi_flash is updated to check for this condition.

Samples are mainly meant to demonstrate use of subsystems (flash drivers in this case).
If you want to test specific conditions, modify tests/drivers/flash.

tu-brian · 2023-01-10T10:16:33Z

samples/drivers/spi_flash is updated to check for this condition.

Samples are mainly meant to demonstrate use of subsystems (flash drivers in this case). If you want to test specific conditions, modify tests/drivers/flash.

The tests in 'tests/drivers/flash' tests the internal flash of the MCU. I really don't want to change this - but at the same time have an option to test my modifications

erwango · 2023-01-10T14:42:44Z

The tests in 'tests/drivers/flash' tests the internal flash of the MCU. I really don't want to change this - but at the same time have an option to test my modifications

Then we might need dedicated "SPI Flash" tests. But I stand on the position that samples should not be used for extensive testing.

tu-brian · 2023-01-11T07:38:56Z

I respect your position.

That being said, creating a new testsuite for an otherwise totally untested driver is imo way out of scope for submitting a bugfix.

My objective with this PR was is to fix a bug that we spent quite some time to narrow down, so that others would not have to spent the same effort. I tried to be a nice guy and updated the sample to prove that the change I did worked.

I have already spent way much more time on submitting this issue than I can afford, so I now see 3 options:

You accept the PR as is (yes, I will fix the signed off by issue)
You accept the PR without changes to the sample and then there are no way to verify this in the future
2I withdraw the PR and maintain the fix in our own fork. Then the next person will need to debug and fix this once again.

Option 3 would nor really improve the quality of of zephyr going forward.

Please let me know which option you chose.

erwango · 2023-01-11T08:09:04Z

@tu-brian The real issue in current case is that there is no real spi flash test suite.
We both agree this is unfortunate that there will be no way to verify this in future but we need to draw some boundaries to the modifications that are done to samples.
This being said, I would be ok to accept sample update if I know this will be converted to a test suite eventually. An issue should be raised to follow this up. @de-nordic What do you think?

de-nordic · 2023-01-16T14:44:24Z

@tu-brian The real issue in current case is that there is no real spi flash test suite. We both agree this is unfortunate that there will be no way to verify this in future but we need to draw some boundaries to the modifications that are done to samples. This being said, I would be ok to accept sample update if I know this will be converted to a test suite eventually. An issue should be raised to follow this up. @de-nordic What do you think?

I am willing to accept the sample test, although I am not fine with making multi-sector test dependent on the CONFIG_FLASH_STM32_OSPI, because the sample should have generic behaviour for all SPI boards.

@erwango Regarding lack of SPI flash tests I will log an Issue with Zephyr for the flash subsystem, as an enhancement, and will make it 3.4 target to have SPI flash test available. This sample is already unfortunate to use "test" in some of the messages, so it's could be misunderstood to serve dual purpose.

tu-brian · 2023-01-17T07:20:42Z

What Can I do to move this issue forward?

de-nordic · 2023-01-17T12:11:57Z

What Can I do to move this issue forward?

One question, why the flash tests, that are here https://github.com/zephyrproject-rtos/zephyr/tree/main/tests/drivers/flash, can not be altered to do the testing here?
I do not think that we need to have SPI flash specific flash tests, because they use the same Flash API as any other flash access.
Making the test work with Flash API is just the matter of pointing to a device hanging from SPI and adding the test cases for the boundary erase.

tu-brian · 2023-01-18T07:19:06Z

I tried to enable the tests https://github.com/zephyrproject-rtos/zephyr/tree/main/tests/drivers/flash for the board in question - only to find that the only test present failed, due to unaligned access not being support for the flash device in question.
At that time I could not afford to spend any more time on this issue, so my previous comments still stands:

I have already spent way much more time on submitting this issue than I can afford, so I now see 3 options:

You accept the PR as is (yes, I will fix the signed off by issue)
You accept the PR without changes to the sample and then there are no way to verify this in the future
I withdraw the PR and maintain the fix in our own fork. Then the next person will need to debug and fix this once again.

Option 3 would nor really improve the quality of of zephyr going forward.

Please let me know which option you chose.

de-nordic

Looks OK.
Approving with sample used to test the fix. I will log an issue for flash drivers/tests to extend tests to cover more scenarios including the one tested by sample.

fabiobaltieri · 2023-01-25T11:10:28Z

Hey there's a couple of missing CI tests, can't submit without those, I'm kicking a rebase to retrigger them.

This commit fixes a bug in the STM32 ospi flash driver when attempting to erase an area that spans more than one erase sector. Without this fix, only the first sector is actually erased, the rest silently fails the erase. Issue is that the write enable latch command is only sent for the first erase command. Signed-off-by: Brian Juel Folkmann <[email protected]>

Add tests for erase of multiple consequtive sectors. Signed-off-by: Brian Juel Folkmann <[email protected]>

FRASTM · 2023-01-25T15:06:10Z

drivers/flash/flash_stm32_ospi.c

 		} else {
 			/* Sector erase */
 			LOG_DBG("Sector Erase");
+


minor : why blank line here and not also @ 960

tu-brian requested review from de-nordic and nashif as code owners January 9, 2023 08:33

zephyrbot added area: Flash platform: STM32 ST Micro STM32 labels Jan 9, 2023

zephyrbot requested review from ABOSTM, FRASTM, GeorgeCGV, erwango and gautierg-st January 9, 2023 08:34

zephyrbot assigned erwango Jan 9, 2023

erwango previously approved these changes Jan 9, 2023

View reviewed changes

erwango self-requested a review January 9, 2023 08:36

tu-brian dismissed erwango’s stale review via 2d5b22b January 9, 2023 09:44

tu-brian force-pushed the fix_stm32_ospi_flash_erase branch 2 times, most recently from 2d5b22b to c949c35 Compare January 9, 2023 11:19

FRASTM requested changes Jan 9, 2023

View reviewed changes

drivers/flash/flash_stm32_ospi.c Outdated Show resolved Hide resolved

tu-brian force-pushed the fix_stm32_ospi_flash_erase branch from c949c35 to 0a66516 Compare January 9, 2023 14:04

DhruvaG2000 reviewed Jan 9, 2023

View reviewed changes

DhruvaG2000 suggested changes Jan 9, 2023

View reviewed changes

FRASTM reviewed Jan 10, 2023

View reviewed changes

tu-brian force-pushed the fix_stm32_ospi_flash_erase branch from 0a66516 to f67f6ba Compare January 10, 2023 08:52

erwango requested changes Jan 10, 2023

View reviewed changes

tu-brian force-pushed the fix_stm32_ospi_flash_erase branch from f67f6ba to 6d15777 Compare January 10, 2023 09:28

tu-brian force-pushed the fix_stm32_ospi_flash_erase branch from 6d15777 to cdacb84 Compare January 11, 2023 08:34

de-nordic mentioned this pull request Jan 17, 2023

RFC: Stop referring to operations performed by samples as "tests" #53861

Open

de-nordic previously approved these changes Jan 18, 2023

View reviewed changes

de-nordic requested review from DhruvaG2000, FRASTM and erwango and removed request for DhruvaG2000 January 18, 2023 11:10

erwango previously approved these changes Jan 19, 2023

View reviewed changes

FRASTM previously approved these changes Jan 19, 2023

View reviewed changes

erwango requested review from fabiobaltieri and removed request for DhruvaG2000 January 25, 2023 10:34

tu-brian added 2 commits January 25, 2023 11:10

samples: spi_flash: Add option to test multiple sectors

c6e50ed

Add tests for erase of multiple consequtive sectors. Signed-off-by: Brian Juel Folkmann <[email protected]>

fabiobaltieri force-pushed the fix_stm32_ospi_flash_erase branch from cdacb84 to c6e50ed Compare January 25, 2023 11:10

fabiobaltieri approved these changes Jan 25, 2023

View reviewed changes

fabiobaltieri dismissed stale reviews from FRASTM, erwango, and de-nordic via c6e50ed January 25, 2023 11:23

FRASTM reviewed Jan 25, 2023

View reviewed changes

FRASTM approved these changes Jan 25, 2023

View reviewed changes

erwango approved these changes Jan 26, 2023

View reviewed changes

fabiobaltieri merged commit d0a7ab5 into zephyrproject-rtos:main Jan 26, 2023

-		if (stm32_ospi_write_enable(&dev_data->hospi,
+		ret = stm32_ospi_write_enable(&dev_data->hospi,
+						    dev_cfg->data_mode, dev_cfg->data_rate);
+		if (ret != 0) {
+			LOG_ERR("Erase failed : write enable");
+			break;
+		```

drivers: flash: bug fix stm32 ospi flash erase #53598

drivers: flash: bug fix stm32 ospi flash erase #53598

Uh oh!

Conversation

tu-brian commented Jan 9, 2023

Uh oh!

erwango commented Jan 9, 2023

Uh oh!

Uh oh!

DhruvaG2000 left a comment

Choose a reason for hiding this comment

Uh oh!

DhruvaG2000 left a comment

Choose a reason for hiding this comment

Uh oh!

FRASTM Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

tu-brian Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

FRASTM Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

tu-brian Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

erwango left a comment

Choose a reason for hiding this comment

Uh oh!

tu-brian commented Jan 10, 2023

Uh oh!

erwango commented Jan 10, 2023

Uh oh!

tu-brian commented Jan 11, 2023

Uh oh!

erwango commented Jan 11, 2023

Uh oh!

de-nordic commented Jan 16, 2023

Uh oh!

tu-brian commented Jan 17, 2023

Uh oh!

de-nordic commented Jan 17, 2023

Uh oh!

tu-brian commented Jan 18, 2023

Uh oh!

de-nordic left a comment

Choose a reason for hiding this comment

Uh oh!

fabiobaltieri commented Jan 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FRASTM Jan 25, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

fabiobaltieri commented Jan 25, 2023 •

edited

Loading