Skip to content
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.

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

TARGET_STM32F7: Refresh cache when erasing or programming flash #10248

Merged
merged 1 commit into from Apr 1, 2019

Conversation

Projects
None yet
@VVESTM
Copy link
Contributor

commented Mar 28, 2019

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

@VVESTM VVESTM closed this Mar 28, 2019

@VVESTM VVESTM reopened this Mar 28, 2019

@VVESTM VVESTM force-pushed the VVESTM:issue_9934 branch from 30f71ef to de389bb Mar 28, 2019

@@ -130,6 +130,9 @@ int32_t flash_erase_sector(flash_t *obj, uint32_t address)
status = -1;
}

SCB_CleanInvalidateDCache();

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Mar 28, 2019

Contributor

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?

This comment has been minimized.

Copy link
@VVESTM

VVESTM Mar 28, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Mar 28, 2019

Contributor

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

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Mar 28, 2019

Contributor

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.

This comment has been minimized.

Copy link
@VVESTM

VVESTM Mar 28, 2019

Author Contributor

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

This comment has been minimized.

Copy link

commented Mar 28, 2019

@offirko can you please review?

@ciarmcom ciarmcom requested review from screamerbg and ARMmbed/mbed-os-maintainers Mar 28, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

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

@VVESTM VVESTM force-pushed the VVESTM:issue_9934 branch from de389bb to 771f902 Mar 28, 2019

@@ -130,6 +132,11 @@ int32_t flash_erase_sector(flash_t *obj, uint32_t address)
status = -1;
}

address_sector = GetSectorBase(SectorId);

SCB_CleanInvalidateDCache_by_Addr(&address_sector, GetSectorSize(SectorId));

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Mar 28, 2019

Contributor

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.

@@ -167,6 +174,9 @@ int32_t flash_program_page(flash_t *obj, uint32_t address, const uint8_t *data,
}
}

SCB_CleanInvalidateDCache_by_Addr(&address, size);

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Mar 28, 2019

Contributor

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.

@VVESTM VVESTM force-pushed the VVESTM:issue_9934 branch from 771f902 to 976fefb Mar 28, 2019

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.

Signed-off-by: Vincent Veron <vincent.veron@st.com>

@VVESTM VVESTM force-pushed the VVESTM:issue_9934 branch from 976fefb to 5765c4d Mar 28, 2019

@kjbracey-arm
Copy link
Contributor

left a comment

Looks good to me now.

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

@VVESTM

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

Thanks @kjbracey-arm.

@@ -130,6 +131,9 @@ int32_t flash_erase_sector(flash_t *obj, uint32_t address)
status = -1;
}

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

This comment has been minimized.

Copy link
@offirko

offirko Mar 28, 2019

Contributor

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?

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Mar 28, 2019

Contributor

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.

This comment has been minimized.

Copy link
@VVESTM

VVESTM Mar 28, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@offirko

offirko Mar 28, 2019

Contributor

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

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Mar 28, 2019

Contributor

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
Copy link
Contributor

left a comment

LGTM

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

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
Copy link
Contributor

left a comment

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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

@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

cmonr approved these changes Mar 29, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

#10248 (comment)

Progressing PR.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 29, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2019

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

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 31, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

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

@mbed-ci

This comment has been minimized.

Copy link

commented Apr 1, 2019

Test run: SUCCESS

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

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

@jeromecoutant

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

ST_INTERNAL_REF 63073

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

28 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARMC5 Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR8 Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARMC5 Success
Details
jenkins-ci/mbed2-build-ARMC6 Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR8 Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9157 cycles
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Apr 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.