Skip to content

TARGET_STM: fix flash api 64bit address alignment on L4 and WB - #12086

Merged
adbridge merged 1 commit into
ARMmbed:masterfrom
ABOSTM:FLASH_API_64B_ALIGNMENT
Dec 17, 2019
Merged

TARGET_STM: fix flash api 64bit address alignment on L4 and WB#12086
adbridge merged 1 commit into
ARMmbed:masterfrom
ABOSTM:FLASH_API_64B_ALIGNMENT

Conversation

@ABOSTM

@ABOSTM ABOSTM commented Dec 11, 2019

Copy link
Copy Markdown
Contributor

Summary of changes

fix flash api 64bit address alignment on L4 and WB


Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

No test available with misaligned address.


@ciarmcom
ciarmcom requested a review from a team December 11, 2019 18:00
@ciarmcom

Copy link
Copy Markdown
Member

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


/* HW needs an aligned address to program flash, which data parameters doesn't ensure */
if ((uint32_t) data % 4 != 0) { // Data is not aligned, copy data in a temp buffer before programming it
if ((uint32_t) data % 8 != 0) { // Data is not aligned, copy data in a temp buffer before programming it

@kjbracey kjbracey Dec 16, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Change isn't wrong, but it would be nice to tidy a bit - get rid of the pointer aliasing and volatile. I'd rather this was

if ((uintptr_t) data % 8 != 0) {
    while ((address < (StartAddress + size)) && (status == 0)) {
        uint64_t data64 = 0;
        for (int i = 0; i < 8; i++) {
            data64 |= (uint64_t) data[i] << (i * 8);
        }
     }

Although I would have thought it should be possible in principle to just case the uint8_t * to a __packed uint64_t * to get the compiler to figure out how to do a potentially-unaligned load. Catch is I don't think either CMSIS or Mbed has the necessary compiler macros to do that portably.

CMSIS has __UNALIGNED_UINT32_READ, but not __UNALIGNED_UINT64_READ. :(

I guess you could just do

while ((address < (StartAddress + size)) && (status == 0)) {
    uint32_t data_low = __UNALIGNED_UINT32_READ(data);
    uint32_t data_high = __UNALIGNED_UINT32_READ(data + 4);
    uint64_t data64 = ((uint64_t) data_high << 32) | data_low;
    if (HAL_FLASH_Program(FLASH_TYPEPROGRAM_DOUBLEWORD, address, data64) == HAL_OK) {

which should be just the same or only a bit off having a direct 64-bit macro.

(And do that unconditionally - don't check for alignment - there's really no need to increase the code size to "speed-optimise" the aligned case).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hi

So let's wait for ARM-software/CMSIS_5#768 merge in mbed ?

Then, to be honest, as this looks as optimization, this will not come in our todo list very soon...
So any help and pull requests are welcomed!

Jerome

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, okay, we can revisit this when that appears, rather than go for a half-nice version.

@kjbracey

Copy link
Copy Markdown
Contributor

CI started

@mbed-ci

mbed-ci commented Dec 17, 2019

Copy link
Copy Markdown

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@adbridge
adbridge merged commit b1b0673 into ARMmbed:master Dec 17, 2019
@adbridge adbridge added release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 and removed ready for merge labels Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants