FatFS interface on H7

[geebee]

I'm not sure I see how that would work.

If the unaligned invalidate is done with interrupts disabled, you still have the case where after it completes the thread that invalidated is waiting on DMA to complete, so you could switch to another thread that then proceeds to read or write one of the cache lines at the edge, so potentially reading it from SRAM and caching it before DMA is done. Then once DMA completes the first thread is scheduled again, and will see the data in cache, missing some or all the DMA data for the cache lines involved. Probably more a risk at the end of the buffer than not at the beginning, but this could happen easily even at the beginning for example for a SPI transaction with a relatively slow clock. What am I missing?

The only solution I see is locking everything until DMA completes, but that would seem an unacceptably long time for a critical section.

Either way, I think it's still a good idea that the aligned flush/invalidate check their parameters when CH_DBG_ENABLE_CHECKS is set.

GB

[Giovanni]

I agree, that would not be acceptable, then we are back to square one, you need to use aligned buffers...

Giovanni

[alex31]

Thanks for the explanations, geebee.

I agree with both of you, unaligned DMA cached buffer sync workaround seems complicated and brittle, fixing all the corner cases will be difficult.

But what about having the choice to make SDIO driver use unaligned buffer in DMA accessible NO_CACHE region ?

about the suggestion made by geebee about a check on alignment in the flush/invalidate function, i thinks it's a good idea :
http://forum.chibios.org/viewtopic.php?f=38&t=5072&hilit=cache

A.

[Giovanni]

Just note that cache handling is not done inside drivers as a choice, it is upper layers that should decide how buffers are declared and how are placed. Putting those in a non-cacheable area is a good option.

Giovanni

[geebee]

Here's what I would do (also attached)

Code: Select all

--- a/os/hal/ports/common/ARMCMx/cache.h
+++ b/os/hal/ports/common/ARMCMx/cache.h
@@ -84,6 +84,8 @@
 #define cacheBufferInvalidate(saddr, n) {                                   \
   uint8_t *start = (uint8_t *)(saddr);                                      \
   uint8_t *end = start + (size_t)(n);                                       \
+  chDbgCheck((((uint32_t) start) % CACHE_LINE_SIZE) == 0);                  \^M
+  chDbgCheck((((uint32_t) end) % CACHE_LINE_SIZE) == 0);                    \^M
   __DSB();                                                                  \
   while (start < end) {                                                     \
     SCB->DCIMVAC = (uint32_t)start;                                         \
@@ -110,6 +112,8 @@
 #define cacheBufferFlush(saddr, n) {                                        \
   uint8_t *start = (uint8_t *)(saddr);                                      \
   uint8_t *end = start + (size_t)(n);                                       \
+  chDbgCheck((((uint32_t) start) % CACHE_LINE_SIZE) == 0);                  \^M
+  chDbgCheck((((uint32_t) end) % CACHE_LINE_SIZE) == 0);                    \^M
   __DSB();                                                                  \
   while (start < end) {                                                     \
     SCB->DCCIMVAC = (uint32_t)start;                                        \
diff --git a/os/various/fatfs_bindings/fatfs_diskio.c b/os/various/fatfs_bindings/fatfs_diskio.c
index ef0474634..5957175fd 100644
--- a/os/various/fatfs_bindings/fatfs_diskio.c
+++ b/os/various/fatfs_bindings/fatfs_diskio.c
@@ -122,6 +122,7 @@ DRESULT disk_read (
     UINT count        /* Number of sectors to read (1..255) */
 )
 {
+  cacheBufferInvalidate(buff, MMCSD_BLOCK_SIZE*count);^M
   switch (pdrv) {
 #if HAL_USE_MMC_SPI
   case MMC:
@@ -163,6 +164,7 @@ DRESULT disk_write (
     UINT count        /* Number of sectors to write (1..255) */
 )
 {
+  cacheBufferFlush(buff, MMCSD_BLOCK_SIZE*count);^M
   switch (pdrv) {
 #if HAL_USE_MMC_SPI
   case MMC:

And then access the FATFS object though an accessor that ensures alignment

Code: Select all

FATFS *fatfs(void)
{
  static __attribute__((aligned(CACHE_LINE_SIZE))) uint8_t _fatfs_buf[sizeof(FATFS) + CACHE_LINE_SIZE];
  const size_t offset = offsetof(FATFS, win) % CACHE_LINE_SIZE;
  return (FATFS*)(_fatfs_buf + (offset ? CACHE_LINE_SIZE - offset : 0));
}

or alternatively manually pad/layout, which works well for a given FatFS configuration, but won't work if some of the configuration parameters are changed. I'm not aware of any way to force a struct to be aligned to a specific nonzero offset from an alignment boundary, but if anyone knows how to do that it would probably be the ideal solution instead of the accessor.


Re: NO_CACHE. I tried to do this at first, and it might work on some (most?) devices. Unfortunately on the H7 I think it's only possible to do DMA between SDMMC1 and AXI SRAM

GB

[Giovanni]

Hi,

Look in mcuconf.h, there are options to make part of the RAM non-cacheable.

Giovanni

[geebee]

That also would need changes to ChibiOS. On STM32H7 SDMMC1 cannot do DMA from/to any of the memories that are currently supported for disabling caching (SRAM1+SRAM2, SRAM3).

Up to you to do what you think it's best, as I said before mine was just a suggestion on how to fix it. Thanks.

GB

[Giovanni]

Hi,

That setting actually uses the MPU, if you look in hal_lld.c take the MPU code snippet from there and you can change attributes of any area.

Giovanni

[geebee]

Giovanni,

do you plan on adding an "official" example showing the proper way to use FatFS on the H7? I'm having a hard time getting it to work via MPU region settings.

Setting up the object is still a bit awkward, because it needs to be in a region with size a power of two, and aligned to its size. To not affect other random objects that end up being linked right after it, I did this to ensure the FATFS object sits alone in its own region with the right size and alignment (a bit wasteful but I have enough memory):

Code: Select all

#define FATFS_NOCACHE_SIZE             1024
#define FATFS_MPU_SIZE     MPU_RASR_SIZE_1K

static union __attribute__((aligned(FATFS_NOCACHE_SIZE))) {
  FATFS fatfs;
  uint8_t align_buf[FATFS_NOCACHE_SIZE];
} SDC_FS;

And then before using FATFS I tried to do this

Code: Select all

  mpuDisable();
  mpuConfigureRegion(MPU_REGION_0,
                     &SDC_FS,
                     MPU_RASR_ATTR_AP_RW_RW |
                     MPU_RASR_ATTR_NON_CACHEABLE |
                     FATFS_MPU_SIZE |
                     MPU_RASR_ENABLE);
  mpuEnable(MPU_CTRL_PRIVDEFENA);
  SCB_CleanInvalidateDCache();

But it's not working. From what I can tell, things are still being cached. I also tried without mpuDisable() first, and for all the other region numbers, but with the same result.

Any idea what's wrong with my code?
Thank you,

GB

[Giovanni]

Hi,

I don't have H7 boards with SDIO slots. You code looks correct, are you sure it is still cached? it could be another issue, the SD driver is not tested on H7

Try disabling the cache entirely for confirmation.

Giovanni