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.

Sign up for GitHub

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

Jump to bottom

TARGET_STM32F7: Refresh cache when erasing or programming flash #10248

Merged
merged 1 commit into from
Apr 1, 2019
Merged

TARGET_STM32F7: Refresh cache when erasing or programming flash #10248

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions targets/TARGET_STM/TARGET_STM32F7/flash_api.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@

static uint32_t GetSector(uint32_t Address);
static uint32_t GetSectorSize(uint32_t Sector);
static uint32_t GetSectorBase(uint32_t SectorId);

int32_t flash_init(flash_t *obj)
{
Expand Down Expand Up @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


flash_lock();

return status;
Expand All @@ -139,6 +143,8 @@ int32_t flash_program_page(flash_t *obj, uint32_t address, const uint8_t *data,
uint32_t size)
{
int32_t status = 0;
uint32_t StartAddress = address;
uint32_t FullSize = size;

if ((address >= (FLASH_BASE + FLASH_SIZE)) || (address < FLASH_BASE)) {
return -1;
Expand Down Expand Up @@ -167,6 +173,9 @@ int32_t flash_program_page(flash_t *obj, uint32_t address, const uint8_t *data,
}
}

SCB_CleanInvalidateDCache_by_Addr((uint32_t *)StartAddress, FullSize);
SCB_InvalidateICache();

flash_lock();

return status;
Expand Down Expand Up @@ -265,6 +274,23 @@ static uint32_t GetSectorSize(uint32_t Sector)
return sectorsize;
}

/**
* @brief Gets sector base address
* @param SectorId
* @retval base address of a given sector
*/
static uint32_t GetSectorBase(uint32_t SectorId)
{
int i = 0;
uint32_t address_sector = FLASH_BASE;

for(i=0;i<SectorId;i++){
address_sector += GetSectorSize(i);
}
return address_sector;
}


uint8_t flash_get_erase_value(const flash_t *obj)
{
(void)obj;
Expand Down