Bug: memory in address space mapped to QSPI flash does not update when app is running from Daisy Bootloader

[ndonald2]

Expected behavior

When running an application from the Daisy bootloader (e.g. APP_TYPE = BOOT_SRAM), using QSPIHandle to write to flash mapped to memory should update the contents of the mapped memory space.

Actual behavior

When running an application from the Daisy bootloader (e.g. APP_TYPE = BOOT_SRAM), using QSPIHandle to write to flash mapped to memory does not update the contents of the mapped memory space while the app is running. but the mapped memory contents will be correct on the next device reboot.

Repro steps

1. Build and flash Daisy Bootloader
2. Ensure app is built using APP_TYPE = BOOT_SRAM
3. Write some code to write any known data to flash using QSPIHandle to an address offset of zero (should be safe according to docs, since it's well before 0x90040000)
4. Use QSPIHandle::GetData() to read back the data at address offset zero from mapped memory
5. Note that the contents of the mapped memory does not match the data you just wrote
6. Reset/reboot the Daisy device
7. Repeat step 4
8. Note that the data does match the previously written value now

Repeat the above with the default internal flash linkage, no daisy bootloader and note that the mapped memory contents are updated after a flash write.

Notes

• I was testing this on a Seed
• I will happily provide more details / isolate a minimal example project as requested.
• I noticed this trying to use code derived from PersistentStorage (though this also manifests when using that class directly), because every call to StoreSettingsIfChanged() was resulting in a write because the comparison always returns that there are changes, even when there shouldn't be - until the next device reboot, that is.

[stephenhensley]

Thanks for the detailed report.
This is very strange behavior indeed.

I wonder if there's some caching-issue happening when running from SRAM that doesn't happen when running from flash.
Could be a unique interaction of the D-Cache while reading from SRAM.
The flash and AXI do have different device memory area attributes (as per table 7 in the reference manual) that could be the cause.

QSPI is write-through
AXI SRAM is write-back write-allocate
Flash is write-through

I can do a bit more digging on this this week, but in the meantime, I'd suggest trying the following if you have time:

1. Does disabling the DCache SCB_DisableDCache() for the entire program result in the same behavior.
2. If that resolves it then you could try running dsy_dma_invalidate_cache_for_buffer on the chunk on the data at the address you're using from QSPI.

[ndonald2]

@stephenhensley

Thanks so much for the detailed response! you were right on the nose, disabling DCache globally fixes the issue, and explicitly invalidating the cache for the memory I need to read from also works brilliantly.

Would you like me to put this into a PR? I have only tested for BOOT_SRAM and internal flash but it's working for both.

    void StoreSettingsIfChanged()
    {
        SaveStruct s;
        s.storage_state = state_;
        s.user_data     = settings_;

        void *data_ptr = qspi_.GetData(address_offset_);

        // Caching behavior is different when running programs outside internal flash
        // so we need to explicitly invalidate the QSPI mapped memory to ensure we are
        // comparing the local settings with the most recently persisted settings.
        if (System::GetProgramMemoryRegion() != System::MemoryRegion::INTERNAL_FLASH)
        {
            dsy_dma_invalidate_cache_for_buffer((uint8_t *)data_ptr, sizeof(s));
        }

        // Only actually save if the new data is different
        // Use the `==operator` in custom SettingStruct to fine tune
        // what may or may not trigger the erase/save.
        auto storage_data = reinterpret_cast<SaveStruct *>(data_ptr);
        if(settings_ != storage_data->user_data)
        {
            qspi_.Erase(address_offset_, address_offset_ + sizeof(s));
            qspi_.Write(address_offset_, sizeof(s), (uint8_t *)&s);
        }
    }

[ndonald2]

Or maybe it makes more sense to add this directly into the QSPI handle so it fixes other use cases 🤔

GetData() is agnostic of size, it just returns a pointer, so I'm not sure how that would fit nicely into the current interface.

But perhaps invalidating the cache after a write or erase if necessary?

[stephenhensley]

@ndonald2 Your proposed fix is definitely a good stop gap.

Possible alternatives that would work more universally:

• Update MPU settings to for write-through for similar behavior as flash when executing from there (I have a feeling this could have some performance impact. So not my favorite solution).
• Add an optional size parameter to the QspiHandle::GetData function, and have the invalidate command done there.

Both of those have their own issues.. So for the time being I think patching the PersistentStorage to have the fix would be probably be solid.

If you've got the time to make a PR, that would be very welcome.

Thanks!

[ndonald2]

@stephenhensley no problem, done! Thanks for rubber ducking this.

[stephenhensley]

@ndonald2 awesome! And no problem, thanks for getting a fix together!