Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add esp_spiram_writeback_range support #15849

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

eren-terzioglu
Copy link
Contributor

Summary

Add esp_spiram_writeback_range function to flush some areas of spiram cache to update spiram faster and more reliable.

  • esp32[s2|s3]: Enhance SPIRAM/PSRAM support
  • esp32[s2|s3|c3|c6|h2]: Update common layer

Impact

ESP32, ESP32S2 and ESP32S3

Testing

Build for esp32 command

make -j distclean && ./tools/configure.sh esp32-devkitc:psram && make flash EXTRAFLAGS="-Wno-cpp -Werror" ESPTOOL_BINDIR=./ ESPTOOL_PORT=/dev/ttyUSB0 -s -j$(nproc) && minicom

Build for esp32s2 command

make -j distclean && ./tools/configure.sh esp32s2-saola-1:nsh && kconfig-tweak -e CONFIG_ESP32S2_FLASH_FREQ_80M && kconfig-tweak -e CONFIG_ESP32S2_SPIRAM && kconfig-tweak -e CONFIG_MM_REGIONS && kconfig-tweak -e CONFIG_TESTING_MM && make olddefconfig && make flash EXTRAFLAGS="-Wno-cpp -Werror" ESPTOOL_BINDIR=./ ESPTOOL_PORT=/dev/ttyUSB0 -s -j$(nproc) && minicom

Build for esp32s3 command

make -j distclean && ./tools/configure.sh esp32s3-devkit:psram_quad && make flash EXTRAFLAGS="-Wno-cpp -Werror" ESPTOOL_BINDIR=./ ESPTOOL_PORT=/dev/ttyUSB0 -s -j$(nproc) && minicom

Before build and flash I changed these lines which are pointed with + to check flush is working fine:

Note: Change can be applied every chip's related file (nuttx/arch/xtensa/src/esp32s2/esp32s2_spiram.c and nuttx/arch/xtensa/src/esp32s3/esp32s3_spiram.c) which are mentioned on impact section.

// nuttx/arch/xtensa/src/esp32/esp32_spiram.c
int esp_spiram_test(void)
{
  volatile int *spiram = (volatile int *)PRO_DRAM1_START_ADDR;

  /* Set size value to 4 MB which is related to psize argument on
   * cache_sram_mmu_set() calls. In this SoC, psize is 32 Mbit.
   */

  size_t s = 4 * 1024 * 1024;
  size_t p;
  int errct = 0;
  int initial_err = -1;

  for (p = 0; p < (s / sizeof(int)); p += 8)
    {
      spiram[p] = p ^ 0xaaaaaaaa;
    }

+ esp_spiram_writeback_range(spiram, s);

  for (p = 0; p < (s / sizeof(int)); p += 8)
    {
      if (spiram[p] != (p ^ 0xaaaaaaaa))
        {
          errct++;
          if (errct == 1)
            {
              initial_err = p * sizeof(int);
            }

          if (errct < 4)
            {
              merr("SPI SRAM error@%p:%08x/%08x \n", &spiram[p], spiram[p],
                   p ^ 0xaaaaaaaa);
            }
        }
    }

  if (errct != 0)
    {
      merr("SPI SRAM memory test fail. %d/%d writes failed, first @ %X\n",
           errct, s / 32, initial_err + SOC_EXTRAM_DATA_LOW);
      return ERROR;
    }
  else
    {
      minfo("SPI SRAM memory test OK!");
      return OK;
    }
}

Additionaly, Cache_Disable_DCache_Autoload function used to disable auto flush on. To enable that feature properly you can change this lines:

//nuttx/arch/xtensa/src/esp32/esp32_start.c
//nuttx/arch/xtensa/src/esp32s2/esp32s2_start.c
//nuttx/arch/xtensa/src/esp32s3/esp32s3_start.c

+     Cache_Disable_DCache_Autoload();
      if (esp_spiram_test() != OK)
        {
          ets_printf("SPIRAM test failed\n");
          PANIC();
        }

Change inspired from esp-hal-3rdparty/components/esp_common/test_apps/esp_common/main/test_attr.c:89 (write_spiram_and_reset).
Also tested with mm command

@github-actions github-actions bot added Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: xtensa Issues related to the Xtensa architecture Board: xtensa Size: M The size of the change in this PR is medium labels Feb 17, 2025
@eren-terzioglu eren-terzioglu changed the title Feature/esp spiram writeback Add esp_spiram_writeback_range support Feb 17, 2025
@nuttxpr
Copy link

nuttxpr commented Feb 17, 2025

[Experimental Bot, please feedback here]

Fill In The Commit Message: This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary.

This PR description does not fully meet the NuttX requirements. Here's a breakdown and suggestions for improvement:

Missing/Insufficient Information:

  • Summary: While it states what the change is, it lacks a clear explanation of why it's necessary. What problem does slower/less reliable SPIRAM updating cause? What benefit does this new function provide to users? Is this addressing a specific bug or performance issue? Issue references are missing.
  • Impact: Listing ESP32, ESP32S2, and ESP32S3 is not sufficient. You must explicitly answer all the impact questions (user, build, hardware, documentation, security, compatibility) with YES/NO and provide descriptions where necessary. For example:
    • Impact on user: YES. Users can now utilize the esp_spiram_writeback_range function to potentially improve performance when updating SPIRAM.
    • Impact on build: NO.
    • ...and so on for all impact categories.
  • Testing: The provided testing shows how to build and flash, but not the results of the testing. The "before" and "after" logs are missing actual output. Simply stating "tested with mm command" is not enough; show the output of the mm command before and after the change. What was the observed improvement? How did you measure "faster and more reliable"? Quantify the results whenever possible (e.g., "Reduced update time by X%").

Improvements and Clarifications:

  • Purpose/Motivation: Clearly articulate the problem this change solves and the benefits it brings. Provide concrete examples or scenarios where this improvement is noticeable.
  • Issue References: Link to related NuttX and/or NuttX Apps issues if applicable.
  • Impact Assessment: Thoroughly address each impact category with a YES/NO answer and a detailed explanation when YES.
  • Testing Logs: Include actual "before" and "after" logs demonstrating the change's effect. Quantify improvements with metrics (e.g., timing, error rates). Don't just show build commands. Show the output of the esp_spiram_test and the mm command, highlighting the difference.
  • Build Host Information: Specify the details of your build host system as requested (OS, CPU, compiler version).
  • Target Details: Be more specific about the targets tested. Instead of "esp32-devkitc:psram", provide details like "ESP32-DevKitC V4 with 4MB PSRAM".

Example of Improved Testing Section:

## Testing

I confirm that changes are verified on local setup and works as intended:

* Build Host(s): Linux (Ubuntu 22.04), x86_64 Intel i7-12700K, GCC 11.3.0
* Target(s): esp32-devkitc-v4:psram_4mb, esp32s2-saola-1:spiram_4mb, esp32s3-devkit-1:psram_8mb


Testing logs before change (esp32-devkitc-v4):

// Output of esp_spiram_test and mm command before the change, showing errors or slow performance.


Testing logs after change (esp32-devkitc-v4):

// Output of esp_spiram_test and mm command after the change, showing no errors and improved performance (e.g., "SPI SRAM memory test OK!" and faster mm operations). Quantify the improvement if possible.


// Repeat the above testing logs for each target (esp32s2 and esp32s3).

By addressing these points, the PR description will be much clearer, more complete, and easier for reviewers to understand and approve. Remember, a well-written PR saves everyone time and helps ensure the quality of the NuttX project.

@eren-terzioglu eren-terzioglu force-pushed the feature/esp_spiram_writeback branch 2 times, most recently from 2e8ed94 to 62aea9c Compare February 17, 2025 13:23
Copy link
Contributor

@hartmannathan hartmannathan left a comment

Choose a reason for hiding this comment

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

Please add git commit log...

@eren-terzioglu eren-terzioglu force-pushed the feature/esp_spiram_writeback branch 2 times, most recently from a1d3cf7 to 8793fa5 Compare February 20, 2025 13:37
@eren-terzioglu
Copy link
Contributor Author

Please add git commit log...

Thanks, updated.

@eren-terzioglu eren-terzioglu force-pushed the feature/esp_spiram_writeback branch from 8793fa5 to 03960ae Compare February 21, 2025 12:54
@github-actions github-actions bot removed the Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture label Feb 21, 2025
Add esp_spiram_writeback_range function to flush some areas of spiram cache

Signed-off-by: Eren Terzioglu <[email protected]>
Update common layer to prevent build errors

Signed-off-by: Eren Terzioglu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: xtensa Issues related to the Xtensa architecture Board: xtensa Size: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants