Skip to content

sam0 flashpage_write issue #7667

@travisgriggs

Description

@travisgriggs

This took a while to chase this down. On the samd21 issue, the flash page_write function casts the void* data argument to a uint32_t* pointer. This works fine when the compiler places the memory for your argument aligned. But I've found that it doesn't always. I was declaring a

static u8 Row[NVMCTRL_ROW_SIZE];

The compiler was alternately placing this memory at x20002402 or x20002400. At the latter, the cast to a uint32_t* was fine, but at the former, chip level exception occured. This kind of bug is no fun to figure out.

I have three proposals I'd like to consider for a fix. With some direction, I'll work up the appropriate PR.

  1. Require a client side fix. In this case, we put the onus of getting the data argument 32bit aligned. One can use an expression like:

    static __attribute__((aligned(4))) u8 Row[NVMCTRL_ROW_SIZE];

to force the alignment. If this is the preferred solution, then the change that I believe should be made is the documentation of flashpage_write() should be modified to make this clear to those who decide to use this function.

This is not my personally preferred solution.

  1. Use uint16_t instead of uint32_t. I think it's OK to assume that memory is 2 byte aligned (see Interrupt-Handling on msp430 is buggy #3 otherwise). page_addr and data_addr become uint16_t* and the loop looks like:

     for (unsigned i = 0; i < (FLASHPAGE_SIZE / 2); i++) {
         *page_addr++ = data_addr[i];
     }
    
  2. There is a pathological case where someone does not have the argument short aligned, e.g.:

   #pragma pack(1)
    struct foo {
        u8 singleByte;
        u8 data[FLASHPAGE_SIZE];
    };
    #pragma pack(8)

In this case, it's again not safe to cast data as a uint16_t*. So data_addr must become a uint8_t* (page_addr remains uint16_t*). And the loop must do the work directly:

  for (unsigned i = 0; i < (FLASHPAGE_SIZE - 1); i+= 2) {
        uint16_t pair = ((uint16_t)data_addr[i + 1] << 8) | (uint16_t)(data_addr[i]);
        *page_addr++ = pair;
    }

This is my preferred solution, I think. It's the most robust and safe. It also matches the ASF implementation most closely.

(aside: I had hoped to use memcpy or wmemcpy, since they often dynamically deal with aligned/unaligned data nicely, but I could not get them to work--the data_addr is "special" enough to require explicit code apparently)

Metadata

Metadata

Labels

Platform: ARMPlatform: This PR/issue effects ARM-based platformsType: bugThe issue reports a bug / The PR fixes a bug (including spelling errors)

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions