zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.52k stars 6.44k forks source link

flash_stm32_ospi.c: Unable to erase flash partition using flash_map API #46931

Closed tobias-aunbol closed 2 years ago

tobias-aunbol commented 2 years ago

Describe the bug When using: flash_area_erase(fa, 0. fa->fa_size) flash_stm32_ospi returns error with:

LOG_ERR("Error: wrong sector size 0x%x", size); and returning error code -ENOTSUP

This is due to a check on the SPI_NOR_SECTOR_SIZE and dev_cfg->flash_size in flash_stm32_ospi_erase

Which only make it (for what i can see) able to erase either a sector or the whole chip. As for the datasheet the erase command can be used on sector (4K byte), (64K byte) and whole chip.

I have successfully erased the whole chip by using the flash_driver_api. The problem with that is, that i would rather use the flash_map API as this make a cleaner cut to the rest of the application, and i would like to use different partitions, and could be nice to be able to erase data on specific partition. The partition size is alligned with the sector/block size.

Target: B-U585I-IOT02A board with the MX25LM51245 SPI flash

To Reproduce

  1. Make a flash partition in .dts. From application:
  2. Get a reference to it by using flash_area_open
  3. Call flash_area_erase with reference from flash_area_open: ex. flash_area_erase(fa, 0, fa->fa_size)

Expected behavior Expected driver to clear sectors from start address (offset) to partition size. Of course this is expected that a partition size alligned with the sector size /block size is defined in partition in .dts. If partition size isn't matching then maybe a fail is appropriate or erasing the closest possible without writing into next partition.

Impact Not able to use flash_stm32_ospi with flash_map API

Environment (please complete the following information):

Additional context

from .dts file:

mx25lm51245: ospi-nor-flash@0 {
        compatible = "st,stm32-ospi-nor";
        label = "MX25LM51245";
        reg = <0>;
        ospi-max-frequency = <DT_FREQ_M(50)>;
        size = <DT_SIZE_M(64)>;
        spi-bus-width = <OSPI_OPI_MODE>;
        data-rate = <OSPI_DTR_TRANSFER>;
        status = "okay";

        partitions {
            compatible = "fixed-partitions";
            #address-cells = <1>;
            #size-cells = <1>;

            external_storage_partition: partition@0 {
                label = "extstorage";
                reg = <0x00000000 0x500000>; //also tested with 1000 (works) and 4000

Test made:

void test_flash_area_erase(const struct flash_area* fa, unsigned int fa_id) {

    int err = flash_area_open(fa_id, &fa);

    if(!err && fa != nullptr) {

        err = flash_area_erase(fa, 0, fa->fa_size);

        if(err)
            LOG_ERR("Error erasing flash area: %d", err);

        flash_area_close(fa);
    }
    else {
        LOG_ERR("Unable to open flash area: %d", err);
    }

}

void main(void) {

    LOG_INF("test_flash_area_erase");

    const struct flash_area* m_flash_area = nullptr;
    unsigned int flash_area_id = FLASH_AREA_ID(extstorage);

    test_flash_area_erase(m_flash_area, flash_area_id);

}

This output the following:

[00:00:00.141,000] <inf> flash_stm32_ospi: OSPI flash config is OPI / DTR
[00:00:00.142,000] <inf> littlefs: littlefs partition at /lfs
*** Booting Zephyr OS build zephyr-v3.1.0-690-ga5cd5f4a0c9e  ***
[00:00:00.142,000] <inf>  app: test_flash_area_erase
[00:00:00.142,000] <err> flash_stm32_ospi: Error: wrong sector size 0x500000
[00:00:00.142,000] <err> app: Error erasing flash area: -134
Laczen commented 2 years ago

@tobias-aunbol, the definition of the external partition seems wrong, this is not aligned to the erase-block.

tobias-aunbol commented 2 years ago

@Laczen thanks a lot for your comment. Can you please elaborate further? Is it the size that you are thinking of?

Laczen commented 2 years ago

@Laczen thanks a lot for your comment. Can you please elaborate further? Is it the size that you are thinking of?

Yes, the size of 0x4FFFFF, this can't be correct.

tobias-aunbol commented 2 years ago

I'm no expert (yet ;) in using external SPI flashes so thanks alot! This should then be the beginning of the next block or sector to be alligned with the erase-block? so 0x500000?

Laczen commented 2 years ago

I'm no expert (yet ;) in using external SPI flashes so thanks alot! This should then be the beginning of the next block or sector to be alligned with the erase-block? so 0x500000?

It is the same for internal flashes (and also for arrays, ...). When you specify a size the largest index you can write to is size - 1 (because the index starts at 0). So yes 0x500000, but you can start with trying with 0x1000 (4kB), or 0x4000 (16kB). Set it to a value that you know is supported.

tobias-aunbol commented 2 years ago

oh my god of course, thanks a lot @Laczen, I actually feel kind of stupid now, this fixed the issue. Thanks for pointing it out. I'll mark this issue as close

Laczen commented 2 years ago

No problem, glad that your problem is resolved.

tobias-aunbol commented 2 years ago

Alright i jumped the conclusion a bit to quick, i made a new test again this morning with fresh eyes, and even though my partition wasn't alligned with the erase-block. The flash_stm32_ospi.c still returns with the same error. So i'll reopen the issue

Laczen commented 2 years ago

@tobias-aunbol, so even with a aligned erase-block (both offset and size are aligned to erase blocks) you are getting an error ? Could you please update the issue with the latest description of the setup that fails.

tobias-aunbol commented 2 years ago

@Laczen yeah unfortunatly, i have made a small update to the description

Laczen commented 2 years ago

@tobias-aunbol, could you add a little test program that you are using to validate (or are you running a zephyr test) ?

Laczen commented 2 years ago

@Laczen yeah unfortunatly, i have made a small update to the description

And it is still reporting that the sector size is wrong ?

Laczen commented 2 years ago

@tobias-aunbol, IMO the problem is in the flash_stm32_ospi driver, and is not related to flash_map: https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/flash/flash_stm32_ospi.c#L726

This check should not validate that size != SECTOR_SIZE but should verify that size is not a multiple of SECTOR_SIZE (and maybe sector size should not come a CONFIG but should be read from the device.

@GeorgeCGV, could you check this ?

tobias-aunbol commented 2 years ago

@Laczen I have updated with my test software. I totally agree with you on that it's related to the flash_stm32_ospi.c driver. Because if i run the same test with a internal flash partition it works as expected

Laczen commented 2 years ago

@Laczen I have updated with my test software. I totally agree with you on that it's related to the flash_stm32_ospi.c driver. Because if i run the same test with a internal flash partition it works as expected

To verify it should work for a specific partition size that is equal to 4kB (that I think is SPI_NOR_SECTOR_SIZE).

And to correct it you should replace size != SPI_NOR_SECTOR_SIZE line 726 with (size % SPI_NOR_SECTOR_SIZE) != 0

tobias-aunbol commented 2 years ago

@Laczen I have updated with my test software. I totally agree with you on that it's related to the flash_stm32_ospi.c driver. Because if i run the same test with a internal flash partition it works as expected

To verify it should work for a specific partition size that is equal to 4kB (that I think is SPI_NOR_SECTOR_SIZE).

And to correct it you should replace size != SPI_NOR_SECTOR_SIZE line 726 with (size % SPI_NOR_SECTOR_SIZE) != 0

Yes i agree with you, i just ran the test with size 0x1000, and that outputs: [00:00:00.141,000] flash_stm32_ospi: OSPI flash config is OPI / DTR [00:00:00.142,000] littlefs: littlefs partition at /lfs Booting Zephyr OS build zephyr-v3.1.0-690-ga5cd5f4a0c9e [00:00:00.142,000] app: test_flash_area_erase

And you proposed changes seems to work

FRASTM commented 2 years ago

PR to come with the proposed change :

@@ -724,7 +724,7 @@ static int flash_stm32_ospi_erase(const struct device *dev, off_t addr,
        return -EINVAL;
    }

-   if ((size != SPI_NOR_SECTOR_SIZE) && (size < dev_cfg->flash_size)) {
+   if (((size % SPI_NOR_SECTOR_SIZE) != 0) && (size < dev_cfg->flash_size)) {
        LOG_ERR("Error: wrong sector size 0x%x", size);
        return -ENOTSUP;
    }
FRASTM commented 2 years ago

BTW, did you face any other issue during your test ( refering to https://github.com/zephyrproject-rtos/zephyr/issues/46740) with the b_u585i_iot02a like :

JEDEC OSPI-NOR SPI flash testing                                                
==========================                                                      
SPI flash driver MX25LM51245 was not found! 
FRASTM commented 2 years ago

@tobias-aunbol, IMO the problem is in the flash_stm32_ospi driver, and is not related to flash_map: https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/flash/flash_stm32_ospi.c#L726

This check should not validate that size != SECTOR_SIZE but should verify that size is not a multiple of SECTOR_SIZE (and maybe sector size should not come a CONFIG but should be read from the device.

@GeorgeCGV, could you check this ?

--> I agree that the sector size and other octo flash info should come from the MX25LM51245 device. But unfortunately it does not answer correctly to the read SFDP command (JESD216_SFDP_MAGIC does not match). That's why we put a dummy sfdp-bfp table in the DTS node. Even though this table is probably not correctly reversely filled to hold all the expected info driver requires. Actually, the octoflash SPI_NOR_SECTOR_SIZE, is 0x1000 (from the MX25LM51245 datasheet).

Laczen commented 2 years ago

@FRASTM It is ok to use SPI_NOR_SECTOR_SIZE.