Skip to content

arch/espressif: Add dedicated GPIO support for esp32[-s2|-s3|-c3|-c6|-h2] #16223

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

Merged
merged 7 commits into from
May 14, 2025

Conversation

eren-terzioglu
Copy link
Contributor

@eren-terzioglu eren-terzioglu commented Apr 16, 2025

Summary

Add dedicated gpio support to control pins with faster response time and control multiple pins at the same time to applications requires faster response time (.e.g SPI bitbang)

  • arch/xtensa: Update common layer version for esp32s3

  • Documentation/xtensa: Add dedicated GPIO docs for esp32[-s2|-s3]

  • Documentation/risc-v: Add dedicated GPIO documentation for esp32[-c3|-c6|-h2]

  • boards/xtensa: Add dedicated GPIO board level support for esp32[-s2|-s3]

  • arch/xtensa: Add dedicated GPIO support for esp32[-s2|-s3]

  • boards/risc-v: Add dedicated GPIO board level support for esp32[-c3|-c6|-h2]

  • arch/risc-v: Add Dedicated GPIO support for esp32[c3|c6|h2]

  • drivers/gpio: Add bundle ioctl commands

Impact

Impact on user: No, new feature added

Impact on build: No, old defconfigs can be used without any issue

Impact on hardware: Yes, dedicated GPIO feature added

Impact on documentation:

Impact on security: No

Impact on compatibility: No, it is compatible with old defconfigs

Testing

esp32c6-devkitc:gpio, esp32s3-devkit:gpio and esp32s2-saola-1:gpio config used with CONFIG_ESPRESSIF_DEDICATED_GPIO option enabled.

Building

Commands used for building:

esp32c6

make distclean && ./tools/configure.sh esp32c6-devkitc:gpio && kconfig-tweak -d NDEBUG; kconfig-tweak -e CONFIG_ESPRESSIF_DEDICATED_GPIO &&  make olddefconfig && make -j && make download ESPTOOL_PORT=/dev/ttyUSB0 ESPTOOL_BAUD=115200 ESPTOOL_BINDIR=../esp-bins

esp32s2

make distclean && ./tools/configure.sh esp32s2-saola-1:gpio && kconfig-tweak -d NDEBUG; kconfig-tweak -e CONFIG_ESPRESSIF_DEDICATED_GPIO &&  make olddefconfig && make -j && make download ESPTOOL_PORT=/dev/ttyUSB0 ESPTOOL_BAUD=115200 ESPTOOL_BINDIR=../esp-bins

esp32s3

make distclean && ./tools/configure.sh esp32s3-devkit:gpio && kconfig-tweak -d NDEBUG; kconfig-tweak -e CONFIG_ESPRESSIF_DEDICATED_GPIO &&  make olddefconfig && make -j && make download ESPTOOL_PORT=/dev/ttyUSB0 ESPTOOL_BAUD=115200 ESPTOOL_BINDIR=../esp-bins

Sample app I used:

#include <nuttx/config.h>
#include <inttypes.h>
#include <stdio.h>
#include <fcntl.h>
#include <unistd.h>
#include <syslog.h>
#include <sys/types.h>
#include <sys/ioctl.h>
#include <nuttx/ioexpander/gpio.h>
#include <stdlib.h>
#include <string.h>
#include <errno.h>
#include <debug.h>

#define ESP_DEDIC_GPIO_PATH "/dev/gpio3"
#define COUNT_LIMIT 5

int main(int argc, char *argv[])
{
  int fd;
  int count = 0;
  int rd_val = 0;
  int wr_mask = 0xffff;
  int wr_val = 1;

  fd = open(ESP_DEDIC_GPIO_PATH, O_RDWR);
  wr_arg.mask = 0xffff;
  wr_arg.value = 1;
  while (count < COUNT_LIMIT)
    {
      write(fd, &wr_val, wr_mask);
      read(fd, &rd_val, sizeof(uint32_t));
      printf("Read value: %d\n", rd_val);
  
      wr_val = !wr_val;
      count++;
      usleep(1000000);
    }

  close(fd);
  return OK;

}

Results

Output should look like this:

esp_dedic_gpio_bundle_write: Writing 1 with mask: 65535
esp_dedic_gpio_bundle_read: Reading int pin...
Read value: 1
esp_dedic_gpio_bundle_write: Writing 0 with mask: 65535
esp_dedic_gpio_bundle_read: Reading int pin...
Read value: 0
esp_dedic_gpio_bundle_write: Writing 1 with mask: 65535
esp_dedic_gpio_bundle_read: Reading int pin...
Read value: 1
esp_dedic_gpio_bundle_write: Writing 0 with mask: 65535
esp_dedic_gpio_bundle_read: Reading int pin...
Read value: 0
esp_dedic_gpio_bundle_write: Writing 1 with mask: 65535
esp_dedic_gpio_bundle_read: Reading int pin...
Read value: 1
nsh> 

I tested blink test without any delay and these are the results

C6 results with using POSIX calls

Freq\Number of GPIO's 1 2
Dedicated GPIO Blink Freq(khz) 285 - 290 285
Normal GPIO Blink Freq(khz) 240 120 - 123

S3 results with using POSIX calls

Freq\Number of GPIO's 1 2
Dedicated GPIO Blink Freq(khz) 475 475
Normal GPIO Blink Freq(khz) 421 210

S3 results with using arch calls (esp32s3_gpiowrite, ...)

Freq\Number of GPIO's 1 2
Dedicated GPIO Blink Freq(khz) 5300 5300
Normal GPIO Blink Freq(khz) 4000 2000

@github-actions github-actions bot added Area: Documentation Improvements or additions to documentation Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: xtensa Issues related to the Xtensa architecture Area: Drivers Drivers issues Board: risc-v Board: xtensa Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces. labels Apr 16, 2025
@nuttxpr
Copy link

nuttxpr commented Apr 16, 2025

[Experimental Bot, please feedback here]

This PR description mostly meets the NuttX requirements, but needs a few improvements:

Strengths:

  • Clear Summary: The summary explains the "why," "what," and "how" of the changes.
  • Detailed Impact: The impact section covers most of the required areas, although some could be more detailed.
  • Testing Information: Good inclusion of build instructions, sample app code, and expected output. The performance comparison is a nice addition.

Weaknesses:

  • Missing Issue References: The summary should link to any related NuttX or NuttX Apps issues. Even if there isn't a directly related issue, consider creating one to track the feature request or bug that prompted this PR.
  • Incomplete Documentation Impact: You state there is a documentation impact but don't describe what the update is. Specify which files were added/modified and the nature of the changes.
  • Insufficient "Testing Logs Before Change": You provide expected output after the change, but don't show what the behavior was before the change. This makes it difficult to assess the impact of the PR. Ideally, provide logs showing the slower performance of the existing GPIO implementation.
  • Target Details in Testing: The testing section should be more specific about the target hardware used. E.g., "esp32c6-devkitc on ESP-C6-DevKitC-1 board".
  • Build Host Information Missing: You've documented the targets, but not the host used for the build. Include OS, CPU architecture, and compiler version.
  • Overuse of Bolding: The excessive bolding in the tables makes them harder to read. Reserve bolding for key information or headings.

Suggested Improvements:

  1. Add Issue References: Create and link to a NuttX issue describing the need for dedicated GPIO support.
  2. Detail Documentation Changes: Explain what documentation was added (e.g., "Added a new section to the ESP32 driver documentation describing the dedicated GPIO interface and usage.").
  3. Provide "Before" Logs: Show the output of the sample app (or a similar test) before the dedicated GPIO changes were implemented. This demonstrates the performance improvement.
  4. Specify Build Host Details: Add the build host information (OS, CPU, compiler) to the Testing section. For example: "Build Host: Linux (Ubuntu 22.04), x86_64, GCC 11.2.0"
  5. Refine Target Descriptions: Be more precise about the target boards used. For example: "Target: esp32c6-devkitc on ESP-C6-DevKitC-1 board".
  6. Reduce Bolding in Tables: Remove the excessive bolding in the result tables. Consider using just a header row for bolding. Simple Markdown tables are fine.

Example of Improved Testing Section:

## Testing

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

* Build Host: Linux (Ubuntu 22.04), x86_64, GCC 11.2.0
* Target(s): 
    * esp32c6-devkitc on ESP-C6-DevKitC-1 board
    * esp32s3-devkit on ESP32-S3-DevKitC-1 board
    * esp32s2-saola-1 on ESP32-S2-Saola-1 board


`esp32c6-devkitc:gpio`, `esp32s3-devkit:gpio` and `esp32s2-saola-1:gpio` config used with `CONFIG_ESPRESSIF_DEDICATED_GPIO` option enabled.

Testing logs before change (using standard GPIO):

<Output of the test application before the changes, demonstrating slower performance>


Testing logs after change (using dedicated GPIO):

<Existing "Results" section, including build instructions and sample app code>

By addressing these points, your PR will be much stronger and easier for reviewers to assess. Remember, a well-written PR significantly increases its chances of being merged quickly and smoothly.

@eren-terzioglu eren-terzioglu changed the title Feature/esp dedicated gpio arch/risc-v: Add dedicated GPIO support for esp32[-s2| -s3| -c3| -c6| -h2] Apr 16, 2025
@eren-terzioglu eren-terzioglu changed the title arch/risc-v: Add dedicated GPIO support for esp32[-s2| -s3| -c3| -c6| -h2] arch/risc-v: Add dedicated GPIO support for esp32[-s2|-s3|-c3|-c6|-h2] Apr 16, 2025
Copy link
Member

@raiden00pl raiden00pl left a comment

Choose a reason for hiding this comment

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

I'm not sure if expanding /drivers/ioexpander/gpio.c this way is a good idea. This driver is designed to support single GPIO. The interface for single GPIO is different and mutually exclusive with the interface to control many GPIOs. In your implementation, when go_bundle_XXXX is defined, all the rest of the operations in the gpio driver doesn't make sense.
So what is the point of combining two not compatible implementations into one?

The correct solution is a new driver, dedicated to control many GPIOs, maybe something like gpiochip in Linux.

jerpelea
jerpelea previously approved these changes Apr 16, 2025
@eren-terzioglu
Copy link
Contributor Author

eren-terzioglu commented Apr 16, 2025

I'm not sure if expanding /drivers/ioexpander/gpio.c this way is a good idea. This driver is designed to support single GPIO. The interface for single GPIO is different and mutually exclusive with the interface to control many GPIOs. In your implementation, when go_bundle_XXXX is defined, all the rest of the operations in the gpio driver doesn't make sense. So what is the point of combining two not compatible implementations into one?

The correct solution is a new driver, dedicated to control many GPIOs, maybe something like gpiochip in Linux.

Should we implement a new driver which includes just 2 calls? Maybe we can write the driver when we implement parallel io interface for esp devices to make the interface more inclusive? That interface also can handle HUB75 led display protocol

@raiden00pl
Copy link
Member

Should we implement a new driver which includes just 2 calls? Maybe we can write the driver when we implement parallel io interface for esp devices to make the interface more inclusive?

Yes, these are two different functionalities and should be kept separate. What's the point of using these 2 calls from gpio driver if all others gpio methods are NULL and not compatible with "bundle" approach?

I'm against shortcuts, it is against INVIOLABLES.md. We already have an example of such an easy way out in upstream and it doesn't seem like anyone wants to fix it: #15431

@acassis
Copy link
Contributor

acassis commented Apr 16, 2025

I'm not sure if expanding /drivers/ioexpander/gpio.c this way is a good idea. This driver is designed to support single GPIO. The interface for single GPIO is different and mutually exclusive with the interface to control many GPIOs. In your implementation, when go_bundle_XXXX is defined, all the rest of the operations in the gpio driver doesn't make sense. So what is the point of combining two not compatible implementations into one?

The correct solution is a new driver, dedicated to control many GPIOs, maybe something like gpiochip in Linux.

@eren-terzioglu I think @raiden00pl raised some important point here. Another thing that I noticed is the way it was implemented this solution seems ESP-specific. The right solution should be creating ARCH_HAS_GPIO_GROUPING and also the gpio example should have been modified to support it. I think GPIO grouping is more common term than bundle.

If you decide to create a new driver, I suggest something like iogroup, gpiowide or par_io, it should be more appropriated, gpiochip that Raiden suggested is not self-explanatory.

@xiaoxiang781216
Copy link
Contributor

can we add ioctl to expose ioexpender_lowhalf to userspace? which has api to control pin in the group.

@eren-terzioglu eren-terzioglu force-pushed the feature/esp_dedicated_gpio branch from bd0581c to 39739fa Compare May 14, 2025 08:13
@github-actions github-actions bot removed the Area: Drivers Drivers issues label May 14, 2025
@eren-terzioglu eren-terzioglu force-pushed the feature/esp_dedicated_gpio branch from 39739fa to 60adebf Compare May 14, 2025 08:19
@eren-terzioglu
Copy link
Contributor Author

eren-terzioglu commented May 14, 2025

Thanks @raiden00pl, @acassis, @xiaoxiang781216. I updated without doing any change on master branch's driver level.

@eren-terzioglu eren-terzioglu changed the title arch/risc-v: Add dedicated GPIO support for esp32[-s2|-s3|-c3|-c6|-h2] arch/espressif Add dedicated GPIO support for esp32[-s2|-s3|-c3|-c6|-h2] May 14, 2025
@eren-terzioglu eren-terzioglu changed the title arch/espressif Add dedicated GPIO support for esp32[-s2|-s3|-c3|-c6|-h2] arch/espressif: Add dedicated GPIO support for esp32[-s2|-s3|-c3|-c6|-h2] May 14, 2025
@eren-terzioglu eren-terzioglu force-pushed the feature/esp_dedicated_gpio branch from 60adebf to 3a44ede Compare May 14, 2025 08:29
@eren-terzioglu eren-terzioglu requested a review from raiden00pl May 14, 2025 08:39
acassis
acassis previously approved these changes May 14, 2025
@acassis
Copy link
Contributor

acassis commented May 14, 2025

@eren-terzioglu please include a board example and document how to use it.

@eren-terzioglu
Copy link
Contributor Author

@eren-terzioglu please include a board example and document how to use it.

I added on gpio defconfig, is there anything missing there?

@eren-terzioglu eren-terzioglu force-pushed the feature/esp_dedicated_gpio branch from 3a44ede to ec4434f Compare May 14, 2025 10:39
Add Dedicated GPIO support for risc-v based Espressif devices

Signed-off-by: Eren Terzioglu <[email protected]>
…c6|-h2]

Add board level dedicated GPIO support for risc-v based Espressif devices

Signed-off-by: Eren Terzioglu <[email protected]>
Add dedicated GPIO support for Xtensa based Espressif devices

Signed-off-by: Eren Terzioglu <[email protected]>
Add board level dedicated GPIO support for Xtensa based Espressif devices

Signed-off-by: Eren Terzioglu <[email protected]>
…-c6|-h2]

Add dedicated GPIO documentation for risc-v based Espressif chips

Signed-off-by: Eren Terzioglu <[email protected]>
Add dedicated GPIO documentations for Xtensa based Espressif devices

Signed-off-by: Eren Terzioglu <[email protected]>
Update common layer version for esp32s3 to fix warnings when building

Signed-off-by: Eren Terzioglu <[email protected]>
@raiden00pl raiden00pl dismissed szafonimateusz-mi’s stale review May 14, 2025 17:36

wrong gh account :)

@raiden00pl raiden00pl merged commit ea87446 into apache:master May 14, 2025
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: xtensa Issues related to the Xtensa architecture Area: Documentation Improvements or additions to documentation Board: risc-v Board: xtensa Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants