Closed ndonald2 closed 1 year ago
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:
SCB_DisableDCache()
for the entire program result in the same behavior.dsy_dma_invalidate_cache_for_buffer
on the chunk on the data at the address you're using from QSPI.@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);
}
}
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?
@ndonald2 Your proposed fix is definitely a good stop gap.
Possible alternatives that would work more universally:
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!
@stephenhensley no problem, done! Thanks for rubber ducking this.
@ndonald2 awesome! And no problem, thanks for getting a fix together!
Expected behavior
When running an application from the Daisy bootloader (e.g.
APP_TYPE = BOOT_SRAM
), usingQSPIHandle
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
), usingQSPIHandle
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
APP_TYPE = BOOT_SRAM
QSPIHandle
to an address offset of zero (should be safe according to docs, since it's well before0x90040000
)QSPIHandle::GetData()
to read back the data at address offset zero from mapped memoryRepeat 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
PersistentStorage
(though this also manifests when using that class directly), because every call toStoreSettingsIfChanged()
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.