TARGET_STM32F7: Refresh cache when erasing or programming flash #10248

VVESTM · 2019-03-28T10:22:39Z

TARGET_STM32F7: Refresh cache when erasing or programming flash

The cache must be refreshed when we erase or program flash memory.
It fix 2 issues :
Fix #9934
Fix #6380

This solution was initially proposed in #6380.

Solution tested on mbed-os-5.12.0 and STM32F746 Disco board with this command:
mbed test -t ARM -m DISCO_F746NG -v -n features-storage-tests-blockdevice-general_block_device -v

kjbracey · 2019-03-28T10:50:59Z

targets/TARGET_STM/TARGET_STM32F7/flash_api.c

        status = -1;
    }

+    SCB_CleanInvalidateDCache();


A ranged invalidate-only would be less disruptive than a full clean+invalidate.

The I cache invalidate should also be ranged too, but it seems that CMSIS doesn't have a ranged version of SCB_InvalidateICache for some reason, despite the M7 supporting it.

Feels like the lock should be reapplied first - as soon as possible?

For the D cache, I tried with this code :
{
int i = 0;
uint32_t address_sector = FLASH_BASE;
for(i=0;i<SectorId;i++){
address_sector += GetSectorSize(i);
}

SCB_CleanInvalidateDCache_by_Addr(&address_sector, GetSectorSize(SectorId)); SCB_InvalidateICache();

}
Unfortunately, this was not working.
Then, I think the time consumed by SCB_CleanInvalidateDCache() is not so important compared to the memory erase.

You should just need to do SCB_CleanInvalidateDCache_by_Addr(data, size), except for the fact that the original parameters have been modified.

Ah, that was for the program. I see what you mean now for the erase code - you needed to mess to try to find the sector start address.

Your code looks correct, except you have &address_sector in the invalidate where it should be address_sector.

It would make sense to package that as a GetSectorBase(SectorId) function.

Thanks for insisting ! I pushed a new version including your remarks. I invalidate and clean only the concern sector. I have also added a function to get sector base address.
The rebase on master forced me to clean my code and the test is passed now...

dannybenor · 2019-03-28T11:49:48Z

@offirko can you please review?

ciarmcom · 2019-03-28T12:00:30Z

@VVESTM, thank you for your changes.
@screamerbg @ARMmbed/mbed-os-maintainers please review.

kjbracey · 2019-03-28T13:24:39Z

targets/TARGET_STM/TARGET_STM32F7/flash_api.c


+    address_sector = GetSectorBase(SectorId);
+
+    SCB_CleanInvalidateDCache_by_Addr(&address_sector, GetSectorSize(SectorId));


That still wants to be address_sector, not &address_sector. Not sure how that's working. Or even just

SCB_InvalidateDCache_by_Addr((uint32_t *) GetSectorBase(SectorId), GetSectorSize(SectorId));

(Invalidate vs CleanInvalidate doesn't matter - it's a write-through region that we're not writing to anyway)

Bit wonky that the function takes uint32_t *, it doesn't actually need a word-aligned address.

kjbracey · 2019-03-28T13:27:56Z

targets/TARGET_STM/TARGET_STM32F7/flash_api.c

        }
    }

+    SCB_CleanInvalidateDCache_by_Addr(&address, size);


Again, wants (uint32_t *) address, not &address.

Except the loop above has incremented/decremented them, so you'll need to do something to store the original values.

The cache must be refreshed when we erase or program flash memory. It fix 2 issues : Fix ARMmbed#9934 Fix ARMmbed#6380 This solution was initially proposed in ARMmbed#6380. Signed-off-by: Vincent Veron <[email protected]>

kjbracey

Looks good to me now.

I'm considering putting in a patch to CMSIS to give you SCB_InvalidateICache_by_Addr.

VVESTM · 2019-03-28T14:41:54Z

Thanks @kjbracey-arm.

offirko · 2019-03-28T14:56:43Z

targets/TARGET_STM/TARGET_STM32F7/flash_api.c

    }

+    SCB_CleanInvalidateDCache_by_Addr((uint32_t *)GetSectorBase(SectorId), GetSectorSize(SectorId));
+    SCB_InvalidateICache();


Why is Instrcution Cache invalidated as well? (couldnt find the reason at #6380).
Also, I think the example in #6380 could potentially also fit a missing volatile .. did you try it?

Good point. In principle you could be intending to load code into that section of flash and execute it, but is that actually a supported use case for this API? Is it really just intended for storage?

If this is just for storage, then you wouldn't need the I-cache invalidate. Or if you knew you were going to reboot before executing whatever you were loading.

Seems better safe than sorry, although the unranged invalidate makes it a bit painful. If it was ranged would be no big deal.

In the #6380 example, the __IO macro is a volatile.

Originally, the patch concerns only DCache. Followings comments on #9934 , I added ICache. Do you see an issue invalidating ICache ?
Didn't try anything with volatile on #6380

I believe the api is only used for storage and not for instructions execution..
Invalidating ICache would degrade performance, but if its minor then perhaps its better to be safe than sorry, as Kevin said..

@kjbracey-arm __IO is volatile, but I believe it was given as an example.
I'm not really sure if within "flash_erase_sector" there isn't a similar loop that doesn't use volatile.. checking.. "FLASH_WaitForLastOperation" for example

Every single memory-mapped register, as used in the programming process, should be flagged with __IO or __I where it's defined, such as the FLASH->SR in that wait.

The programming process doesn't touch the actual ROM memory space, and there's no need for any further care with volatile or atomics there. If we invalidate after we've programmed, that also acts as the necessary compiler barrier, so from that point on we can just memcpy out like normal cached memory, as the default weak flash_read does - the ROM is stable and cacheable and compiler optimisable.

offirko

LGTM

kjbracey · 2019-03-28T16:35:10Z

I submitted a ranged I-cache invalidate PR to CMSIS: ARM-software/CMSIS_5#559

I also note that they've already changed the uint32_t * to void * upstream.

jeromecoutant

ST CI OK:

target	platform_name	test suite	result	elapsed_time (sec)	copy_method
DISCO_F746NG-GCC_ARM	DISCO_F746NG	tests-mbed_drivers-flashiap	OK	38.53	default
DISCO_F746NG-GCC_ARM	DISCO_F746NG	tests-mbed_hal-flash	OK	22.36	default
DISCO_F746NG-IAR	DISCO_F746NG	tests-mbed_drivers-flashiap	OK	37.97	default
DISCO_F746NG-IAR	DISCO_F746NG	tests-mbed_hal-flash	OK	21.12	default
DISCO_F769NI-GCC_ARM	DISCO_F769NI	tests-mbed_drivers-flashiap	OK	37.1	default
DISCO_F769NI-GCC_ARM	DISCO_F769NI	tests-mbed_hal-flash	OK	21.31	default
DISCO_F769NI-IAR	DISCO_F769NI	tests-mbed_drivers-flashiap	OK	36.13	default
DISCO_F769NI-IAR	DISCO_F769NI	tests-mbed_hal-flash	OK	20.56	default
NUCLEO_F746ZG-GCC_ARM	NUCLEO_F746ZG	tests-mbed_drivers-flashiap	OK	38.41	default
NUCLEO_F746ZG-GCC_ARM	NUCLEO_F746ZG	tests-mbed_hal-flash	OK	21.65	default
NUCLEO_F746ZG-IAR	NUCLEO_F746ZG	tests-mbed_drivers-flashiap	OK	38.02	default
NUCLEO_F746ZG-IAR	NUCLEO_F746ZG	tests-mbed_hal-flash	OK	20.84	default
NUCLEO_F756ZG-GCC_ARM	NUCLEO_F756ZG	tests-mbed_drivers-flashiap	OK	37.55	default
NUCLEO_F756ZG-GCC_ARM	NUCLEO_F756ZG	tests-mbed_hal-flash	OK	21.95	default
NUCLEO_F756ZG-IAR	NUCLEO_F756ZG	tests-mbed_drivers-flashiap	OK	37.32	default
NUCLEO_F756ZG-IAR	NUCLEO_F756ZG	tests-mbed_hal-flash	OK	20.73	default
NUCLEO_F767ZI-GCC_ARM	NUCLEO_F767ZI	tests-mbed_drivers-flashiap	OK	37.61	default
NUCLEO_F767ZI-GCC_ARM	NUCLEO_F767ZI	tests-mbed_hal-flash	OK	21.79	default
NUCLEO_F767ZI-IAR	NUCLEO_F767ZI	tests-mbed_drivers-flashiap	OK	36.66	default
NUCLEO_F767ZI-IAR	NUCLEO_F767ZI	tests-mbed_hal-flash	OK	20.83	default

VVESTM · 2019-03-29T08:00:41Z

@kjbracey-arm, so we can merge my PR now and then, once ARM-software/CMSIS_5#559 is available in mbed, we can make a new PR to invalidate only the requested Icache range.

cmonr · 2019-03-29T19:29:55Z

#10248 (comment)

Progressing PR.

cmonr · 2019-03-29T19:31:06Z

CI started

mbed-ci · 2019-03-29T23:58:24Z

Test run: FAILED

Summary: 1 of 9 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_build-IAR8

NirSonnenschein · 2019-03-31T08:50:46Z

failure seems to be some sort of timeout involving IAR. re-starting CI

mbed-ci · 2019-03-31T11:18:23Z

Test run: FAILED

Summary: 1 of 13 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_greentea-test

alekla01 · 2019-04-01T07:49:44Z

restarted jenkins-ci/mbed-os-ci_greentea-test

mbed-ci · 2019-04-01T11:58:16Z

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 3
Build artifacts

jeromecoutant · 2019-04-01T13:29:49Z

ST_INTERNAL_REF 63073

This copies the approach of the STM32F7 flash driver submitted via PR ARMmbed#10248 With this change the board finally passes all of the device key tests 10/10 times correctly.

Seems the F4 family is also missing the cache cleanup fixes in the flash_api.c. Same fix as in original Mbed OS PR by Vincent Veron <[email protected]> ARMmbed#10248

Seems the F4 family is also missing the cache cleanup fixes in the flash_api.c. Same fix as in original Mbed OS PR by Vincent Veron <[email protected]> ARMmbed#10248 However, as the cache invalidation instructions are not available in CM4, we're trying to use the same ones as the flash_api.c is already using for this family.

Seems the F4 family is also missing the cache cleanup fixes in the flash_api.c. Same fix as in original Mbed OS PR by Vincent Veron <[email protected]> ARMmbed#10248 However, as the cache invalidation instructions are not available in CM4, we're trying to use the FLASH_FlushCaches available in stm32f4xx_hal_flash. (STM has implemented their own flash cache). In case of programming the flash we should also flush the caches, as it is theoretically possible the programming failed but a readback test would not find it, if it gets that readback from the cache, instead of the flash. Thus a potential write failure could go unnoticed.

This copies the approach of the STM32F7 flash driver submitted via PR #10248 With this change the board finally passes all of the device key tests 10/10 times correctly.

VVESTM closed this Mar 28, 2019

VVESTM reopened this Mar 28, 2019

VVESTM force-pushed the issue_9934 branch from 30f71ef to de389bb Compare March 28, 2019 10:35

kjbracey reviewed Mar 28, 2019

View reviewed changes

ciarmcom requested review from a team and screamerbg March 28, 2019 12:00

ciarmcom added the needs: review label Mar 28, 2019

VVESTM force-pushed the issue_9934 branch from de389bb to 771f902 Compare March 28, 2019 13:12

kjbracey reviewed Mar 28, 2019

View reviewed changes

VVESTM force-pushed the issue_9934 branch from 771f902 to 976fefb Compare March 28, 2019 13:46

TARGET_STM32F7: Refresh cache when erasing or programming flash

5765c4d

The cache must be refreshed when we erase or program flash memory. It fix 2 issues : Fix ARMmbed#9934 Fix ARMmbed#6380 This solution was initially proposed in ARMmbed#6380. Signed-off-by: Vincent Veron <[email protected]>

VVESTM force-pushed the issue_9934 branch from 976fefb to 5765c4d Compare March 28, 2019 13:48

kjbracey approved these changes Mar 28, 2019

View reviewed changes

offirko reviewed Mar 28, 2019

View reviewed changes

offirko approved these changes Mar 28, 2019

View reviewed changes

jeromecoutant approved these changes Mar 28, 2019

View reviewed changes

cmonr added needs: CI release-version: 5.12.1 and removed needs: review labels Mar 29, 2019

cmonr approved these changes Mar 29, 2019

View reviewed changes

0xc0170 added ready for merge and removed needs: CI labels Apr 1, 2019

cmonr merged commit cdc2579 into ARMmbed:master Apr 1, 2019

cmonr removed the ready for merge label Apr 1, 2019


		address_sector = GetSectorBase(SectorId);

		SCB_CleanInvalidateDCache_by_Addr(&address_sector, GetSectorSize(SectorId));

TARGET_STM32F7: Refresh cache when erasing or programming flash #10248

TARGET_STM32F7: Refresh cache when erasing or programming flash #10248

Uh oh!

Conversation

VVESTM commented Mar 28, 2019

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!

kjbracey Mar 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dannybenor commented Mar 28, 2019

Uh oh!

ciarmcom commented Mar 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjbracey left a comment

Choose a reason for hiding this comment

Uh oh!

VVESTM commented Mar 28, 2019

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!

offirko left a comment

Choose a reason for hiding this comment

Uh oh!

kjbracey commented Mar 28, 2019

Uh oh!

jeromecoutant left a comment

Choose a reason for hiding this comment

Uh oh!

VVESTM commented Mar 29, 2019

Uh oh!

cmonr commented Mar 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmonr commented Mar 29, 2019

Uh oh!

mbed-ci commented Mar 29, 2019

Uh oh!

NirSonnenschein commented Mar 31, 2019

Uh oh!

mbed-ci commented Mar 31, 2019

Uh oh!

alekla01 commented Apr 1, 2019

Uh oh!

mbed-ci commented Apr 1, 2019

Uh oh!

jeromecoutant commented Apr 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

kjbracey Mar 28, 2019 •

edited

Loading

cmonr commented Mar 29, 2019 •

edited

Loading