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.76k stars 6.57k forks source link

stm32u5: Handle the remaining bytes if data length is not aligned on FLASH_STM32_WRITE_BLOCK_SIZE #67056

Closed PragatiGarg-eaton closed 9 months ago

PragatiGarg-eaton commented 10 months ago

Hello, I am working with a nucleo_u575zi_q and noticed that the flash driver does not handle the length in case the length isn't WRITE_BLOCK_SIZE aligned. At the same time, the similar case is addressed in flash_stm32h7x.c driver (added below). I tried to write a similar small test program (see below) to solve this, but getting '-EINVAL'error code at the last unaligned data chunk.

int i, j, rc = 0;
      for (i = 0; i < len; i += FLASH_STM32_WRITE_BLOCK_SIZE ) {
        rc = write_nwords(dev, offset + i, ((const uint32_t *) data + (i>>2)),
                  FLASH_STM32_WRITE_BLOCK_SIZE / 4);
        if (rc < 0) {
            break;
        }
    }
        /* Handle the remaining bytes if length is not aligned on
     * FLASH_STM32_WRITE_BLOCK_SIZE
     */
         if (i < len) {
        memset(unaligned_datas, 0xff, sizeof(unaligned_datas));
        for (j = 0; j < len - i; ++j) {
            unaligned_datas[j] = ((uint8_t *)data)[i + j];
        }
        rc = write_nwords(dev, offset,
                   (const uint32_t *)unaligned_datas,
                   FLASH_STM32_WRITE_BLOCK_SIZE / 4);
        if (rc < 0) {
            return rc;
        }
    }

Environment :

Links: https://github.com/zephyrproject-rtos/zephyr/blob/c05a483ba4251589b713a1231fb6ad4c75cebbaa/drivers/flash/flash_stm32h7x.c#L382

Laczen commented 10 months ago

@PragatiGarg-eaton, writing data of length unaligned to the write-block-size is not allowed/supported.

PragatiGarg-eaton commented 10 months ago

@Laczen, unaligned length should be managed from the application then? Is it a flash requirement or not supported by zephyr yet? Also, here in flash_stm32h7x.c driver, unaligned check is missing: https://github.com/zephyrproject-rtos/zephyr/blob/c05a483ba4251589b713a1231fb6ad4c75cebbaa/drivers/flash/flash_stm32h7x.c#L102. This should have been handled, correct?

Laczen commented 10 months ago

@Laczen, unaligned length should be managed from the application then?

Yes

Is it a flash requirement or not supported by zephyr yet?

It is a flash requirement, it is possible to create a driver with a smaller (then physical) write-block-size in some cases. In case flash uses a ecc a smaller then physical write-block-size is only possible by a read-erase-write that should be avoided by all means because it reduces expected life and might lead to data loss.

Also, here in flash_stm32h7x.c driver, unaligned check is missing:

https://github.com/zephyrproject-rtos/zephyr/blob/c05a483ba4251589b713a1231fb6ad4c75cebbaa/drivers/flash/flash_stm32h7x.c#L102

. This should have been handled, correct?

Yes, but as long as it is used to do aligned writes there is nothing wrong. Strictly speaking it could be allowed to do an unaligned write length as long as the write offset is aligned. This would however be very confusing for users.

prayassamriya commented 10 months ago

@Laczen, unaligned length should be managed from the application then?

Yes

Is it a flash requirement or not supported by zephyr yet?

It is a flash requirement, it is possible to create a driver with a smaller (then physical) write-block-size in some cases. In case flash uses a ecc a smaller then physical write-block-size is only possible by a read-erase-write that should be avoided by all means because it reduces expected life and might lead to data loss.

Also, here in flash_stm32h7x.c driver, unaligned check is missing: https://github.com/zephyrproject-rtos/zephyr/blob/c05a483ba4251589b713a1231fb6ad4c75cebbaa/drivers/flash/flash_stm32h7x.c#L102

. This should have been handled, correct?

Yes, but as long as it is used to do aligned writes there is nothing wrong. Strictly speaking it could be allowed to do an unaligned write length as long as the write offset is aligned. This would however be very confusing for users.

@Laczen, As there are 2 different approaches being followed in U5 and H7, which one do you think should be followed as an standard? H7 driver takes care of unaligned length however your answer suggest that it should be taken care by application. I think it makes more sense to address it at the place where the limitation lies which is driver not application. how otherwise the application will be portable between different targets?

Laczen commented 10 months ago

@Laczen, As there are 2 different approaches being followed in U5 and H7, which one do you think should be followed as an standard? H7 driver takes care of unaligned length however your answer suggest that it should be taken care by application. I think it makes more sense to address it at the place where the limitation lies which is driver not application. how otherwise the application will be portable between different targets?

There are not really 2 different approaches, h7 allows writing data with unaligned length however the next data write has to start on an aligned position. To use the flash for writing/reading the application needs to take into account the write-block-size. So even if it seems like the h7 solution does not need to know the write-block-size it still needs this to "advance" to the next location. The writing of data of unaligned length on the h7 will in practice not be used.

The limit of the driver is the write-block-size and the applications need to respect this limit.

henrikbrixandersen commented 9 months ago

The limit of the driver is the write-block-size and the applications need to respect this limit.

So this is a non-issue and can be closed?

PragatiGarg-eaton commented 9 months ago

@Laczen, As there are 2 different approaches being followed in U5 and H7, which one do you think should be followed as an standard? H7 driver takes care of unaligned length however your answer suggest that it should be taken care by application. I think it makes more sense to address it at the place where the limitation lies which is driver not application. how otherwise the application will be portable between different targets?

There are not really 2 different approaches, h7 allows writing data with unaligned length however the next data write has to start on an aligned position. To use the flash for writing/reading the application needs to take into account the write-block-size. So even if it seems like the h7 solution does not need to know the write-block-size it still needs this to "advance" to the next location. The writing of data of unaligned length on the h7 will in practice not be used.

The limit of the driver is the write-block-size and the applications need to respect this limit.

Alright, in case h7 allows writing data with unaligned length, then what is the purpose behind this logic introduction: https://github.com/zephyrproject-rtos/zephyr/blob/c05a483ba4251589b713a1231fb6ad4c75cebbaa/drivers/flash/flash_stm32h7x.c#L386

Laczen commented 9 months ago

The limit of the driver is the write-block-size and the applications need to respect this limit.

So this is a non-issue and can be closed?

IMHO it is a non-issue.

Laczen commented 9 months ago

Alright, in case h7 allows writing data with unaligned length, then what is the purpose behind this logic introduction:

https://github.com/zephyrproject-rtos/zephyr/blob/c05a483ba4251589b713a1231fb6ad4c75cebbaa/drivers/flash/flash_stm32h7x.c#L386

It seems that the h7 does support writing unaligned lengths. That is no problem as long as this is not understood as having a write-block-size that is smaller than the real write-block-size. It is not possible to continue writing where the last write has ended. In most applications this will be dead code as to create a portable application they will follow the write-block-size limitations.

MaureenHelm commented 9 months ago

@erwango @Laczen please close if this is not a bug

Laczen commented 9 months ago

Closing this issue, it is not a bug, just a different approach between h7 and u5.