esp32s3 openeth ethernet driver #13527

casaroli · 2024-09-18T15:41:56Z

Summary

Espressif recently added support for esp32s3 on their qemu, so we migrate the openeth ethernet driver to xtensa common and add it to the esp32s3-devkit config.

Impact for the user

Features added
- Now we can run the esp32s3 port in qemu. By enabling the configuration ESP32S3_QEMU_IMAGE and nuttx.merged.bin will be built (as described in the documentation).
- Now the user can enable the openeth driver in esp32s3 in qemu with the option ESP32S3_OPENETH and use network in qemu as described in https://github.com/espressif/esp-toolchain-docs/blob/main/qemu/esp32/README.md#ethernet-support
- Adds a esp32s3-devkit:qemu-openeth config which automatically has networking enabled and is ready to boot on qemu with esp-idf legacy bootloader.
Impact on Build:
- No changes to the build process itself.
- Users will need to select the appropriate QEMU ESP32-S3 or ESP32 configurations.
Impact on Hardware: Support added for Opencore ethernet MAC on ESP32-S3 in QEMU.
Impact on Documentation: Documentation was updated to include instructions for running the ESP32-S3 port in QEMU with networking support.
Impact on Security: None.
Impact on Compatibility: No backward compatibility issues.

Currently, the simple boot is not working with QEMU. This issue was not introduced by this PR, as it is an effect or enabling the simple boot by default. The qemu-openeth defconfigs enable ESP32_APP_FORMAT_LEGACY or ESP32S3_APP_FORMAT_LEGACY to overcome this limitation.

Testing

Build Host: Ubuntu 22.04 LTS, esp-idf v5.3.1
Target: QEMU emulator version 9.0.0 (esp_develop_9.0.0_20240606)
Configs:
- esp32-devkitc:qemu-openeth
- esp32s3-devkit:qemu-openeth
Tests Performed:
- Verified successful boot of NuttX in QEMU.
- Network connectivity tested using ping command from the nsh console.

Commands

Download espressif qemu from https://github.com/espressif/qemu

esp32s3

$ ./tools/configure.sh -l esp32s3-devkit:qemu-openeth
$ make bootloader
$ make ESPTOOL_BINDIR=.
$ qemu-system-xtensa -nographic -machine esp32s3 -m 4 \
    -nic user,model=open_eth \
    -drive file=./nuttx.merged.bin,if=mtd,format=raw
...
NuttShell (NSH) NuttX-12.6.0-RC1
nsh> ping -c2 apache.org
PING 151.101.2.132 56 bytes of data
56 bytes from 151.101.2.132: icmp_seq=0 time=10.0 ms
56 bytes from 151.101.2.132: icmp_seq=1 time=10.0 ms
2 packets transmitted, 2 received, 0% packet loss, time 2020 ms
rtt min/avg/max/mdev = 10.000/10.000/10.000/0.000 ms
nsh>

esp32

QEMU is not emulating the chip revision v3.0, so the user has two options:

#define ESP32_IGNORE_CHIP_REVISION_CHECK in arch/xtensa/src/esp32/esp32_start.c
emulate the efuse as described here https://github.com/espressif/esp-toolchain-docs/blob/main/qemu/esp32/README.md#emulating-esp32-eco3

$ ./tools/configure.sh -l esp32-devkitc:qemu-openeth
$ make bootloader
$ make ESPTOOL_BINDIR=.
$ qemu-system-xtensa -nographic -machine esp32 -m 4 \
    -nic user,model=open_eth \
    -drive file=./nuttx.merged.bin,if=mtd,format=raw
...
WARNING: NuttX supports ESP32 chip revision >= v3.0 (chip is v0.0).
Ignoring this error and continuing because `ESP32_IGNORE_CHIP_REVISION_CHECK` is set...
THIS MAY NOT WORK! DON'T USE THIS CHIP IN PRODUCTION!

NuttShell (NSH) NuttX-12.6.0-RC1
nsh> ping -c2 apache.org
PING 151.101.2.132 56 bytes of data
56 bytes from 151.101.2.132: icmp_seq=0 time=20.0 ms
56 bytes from 151.101.2.132: icmp_seq=1 time=10.0 ms
2 packets transmitted, 2 received, 0% packet loss, time 2020 ms
rtt min/avg/max/mdev = 10.000/15.000/20.000/5.000 ms
nsh>

Out of scope

Currently the simple boot method is not working with QEMU. For this reason we enable the legacy esp-idf bootloader on the default config for qemu-openeth. As this PR did not introduce this defect, I plan on investigating this in the future, however I do not have any timeline for this.
esp32 qemu does not emulate the version v3.0, so we need to either add an efuse storage as described in the espressif documentation or define ESP32_IGNORE_CHIP_REVISION_CHECK in arch/xtensa/src/esp32/esp32_start.c. This is a defect of espressif qemu and is out of scope of NuttX. However, I have updated the esp32 documentation to reference this issue.

lupyuen · 2024-09-18T15:56:21Z

[Experimental Bot, please feedback here]

The PR summary is OK, but the other sections need significant work to meet the requirements.

Here's a breakdown of what's missing and how to improve it:

Impact:

Not specific enough: Simply stating "Now we can run the esp32s3 port in qemu" isn't sufficient.
- Be Explicit: List which features are added. Does this enable any new APIs for developers using NuttX on ESP32-S3?
- Consider the User: Will users need to change configurations, build scripts, or application code to utilize this?
- Address all points: Go through each impact category (build process, hardware, documentation, security, compatibility) and explicitly state "NO" or provide a detailed "YES" explanation.

Testing:

"Manual testing" is insufficient: Provide concrete details.
- Commands: What commands did you run to build and test? (Show the full commands, not just "make").
- Configurations: Which specific ESP32-S3 board and configurations were tested?
- Functionality: What specific tests did you perform to validate the changes? (e.g., Networking tests, peripheral tests).
Logs: The "Testing logs" sections are empty. Include relevant logs before and after your changes, demonstrating the problem being solved or the new functionality working.

Out of Scope:

Actionable information is better: While it's good to note limitations, consider:
- Issue Tracking: Is there an existing issue for the simple boot method problem? If so, link it here. If not, create one so it's tracked.
- Timeline: When can users expect the separate PR? (e.g., "Will be addressed in an upcoming PR within the next sprint").

Example improvements:

Impact:

Feature Added: Support for running the ESP32-S3 port of NuttX in QEMU using the esp-idf bootloader.
Impact on User:
- Users who want to use QEMU for ESP32-S3 development will need to use the esp-idf bootloader instead of the simple bootloader.
- Documentation will be updated to reflect this requirement.
Impact on Build:
- No changes to the build process itself.
- Users will need to select the appropriate QEMU and ESP32-S3 configurations.
Impact on Hardware: Support added for ESP32-S3 in QEMU.
Impact on Documentation: Documentation will be updated to include instructions for running the ESP32-S3 port in QEMU.
Impact on Security: None.
Impact on Compatibility: No backward compatibility issues.

Testing:

Build Host: Ubuntu 20.04, GCC 9.4.0
Target: QEMU, ESP32-S3 DevKitC (config: esp32s3-devkitc_qemu)
Commands:
- ./tools/configure.sh esp32s3-devkitc_qemu
- make
- qemu-system-xtensa -nographic -machine esp32s3 -drive file=nuttx.bin,if=mtd,format=raw
Tests Performed:
- Verified successful boot of NuttX in QEMU.
- Network connectivity tested using ping command from the NuttX console.
Testing Logs: (Include actual log output here)

Out of Scope:

The simple boot method is currently not working with QEMU for the ESP32-S3 port. This issue is being tracked in [link to GitHub issue]. A fix for this issue will be addressed in a separate PR.

By providing this level of detail, you make it much easier for reviewers to understand your changes and ensure they meet the project's requirements.

cederom

Thank you @casaroli amazing work! :-)

casaroli · 2024-09-18T20:19:12Z

Can anyone help me understand why the workflow is failing? What should I do to fix it?

cederom · 2024-09-18T22:16:17Z

Can anyone help me understand why the workflow is failing? What should I do to fix it?

Looks like internal CI problem, builds fine for me on FreeBSD :-)

arch/xtensa/src/common/xtensa_openeth.c

boards/xtensa/esp32/esp32-devkitc/src/esp32-devkitc.h

xiaoxiang781216 · 2024-10-06T01:15:36Z

need fix the ci error first:
https://github.com/apache/nuttx/actions/runs/11194947243/job/31122067924?pr=13527

casaroli · 2024-10-06T10:24:15Z

need fix the ci error first: https://github.com/apache/nuttx/actions/runs/11194947243/job/31122067924?pr=13527

done

fdcavalcanti · 2024-10-07T11:23:31Z

Hi @casaroli this is good, thanks for the contribution!
I'm reviewing this and adding to the internal CI. Will take those last comments into consideration as well.

fdcavalcanti · 2024-10-07T12:27:59Z

boards/xtensa/esp32s3/esp32s3-devkit/configs/qemu-openeth/defconfig

Can you rename this directory to qemu_openeth using underscore instead of dash? Same for ESP32.

Documentation/platforms/xtensa/esp32s3/index.rst

fdcavalcanti · 2024-10-07T19:29:19Z

Now that's what I call a well written PR. Congrats again. Was able to reproduce the issue and make some recommendations.

casaroli · 2024-10-12T11:45:23Z

I believe I have addressed all the concerns. Can you please provide another review?

@xiaoxiang781216 @fdcavalcanti

casaroli · 2024-10-12T11:49:00Z

Now that's what I call a well written PR. Congrats again. Was able to reproduce the issue and make some recommendations.

Well, you should thank @lupyuen's bot 😅 😅

Since this will be used for esp32s3 also, we can have this in common/espressif.

We add the config for esp32s3, then move the esp32 specifics to esp32/chip.h, then add the esp32s3 specifics to esp32s3/chip.h.

The current simple boot method is not working with qemu. We enable this option for now until we can make the simple-boot image boot with QEMU.

since this config requires bootloader, we skip it

lupyuen mentioned this pull request Sep 18, 2024

Update GitHub Pull Request Template. #13494

Merged

casaroli force-pushed the esp32s3-openeth branch from 0e11b48 to 43dbecb Compare September 18, 2024 16:58

cederom approved these changes Sep 18, 2024

View reviewed changes

casaroli marked this pull request as ready for review September 18, 2024 20:23

pkarashchenko approved these changes Sep 19, 2024

View reviewed changes

arch/xtensa/src/common/xtensa_openeth.c Show resolved Hide resolved

arch/xtensa/src/common/xtensa_openeth.c Outdated Show resolved Hide resolved

arch/xtensa/src/common/xtensa_openeth.c Outdated Show resolved Hide resolved

xiaoxiang781216 reviewed Sep 19, 2024

View reviewed changes

arch/xtensa/src/common/xtensa_openeth.c Outdated Show resolved Hide resolved

boards/xtensa/esp32/esp32-devkitc/src/esp32-devkitc.h Show resolved Hide resolved

github-actions bot added the Size: M The size of the change in this PR is medium label Sep 20, 2024

casaroli force-pushed the esp32s3-openeth branch from f037463 to a7bf323 Compare October 5, 2024 17:13

github-actions bot added Area: Documentation Improvements or additions to documentation Area: Tooling Arch: xtensa Issues related to the Xtensa architecture Area: Board support Board support issues labels Oct 5, 2024

casaroli requested a review from xiaoxiang781216 October 5, 2024 17:23

xiaoxiang781216 reviewed Oct 6, 2024

View reviewed changes

boards/xtensa/esp32/esp32-devkitc/src/esp32-devkitc.h Outdated Show resolved Hide resolved

github-actions bot added the Area: CI label Oct 6, 2024

casaroli requested a review from xiaoxiang781216 October 6, 2024 19:54

fdcavalcanti reviewed Oct 7, 2024

View reviewed changes

Documentation/platforms/xtensa/esp32s3/index.rst Outdated Show resolved Hide resolved

fdcavalcanti reviewed Oct 7, 2024

View reviewed changes

Documentation/platforms/xtensa/esp32s3/index.rst Outdated Show resolved Hide resolved

github-actions bot added the Board: xtensa label Oct 12, 2024

casaroli force-pushed the esp32s3-openeth branch 3 times, most recently from 62af320 to a41a4b5 Compare October 12, 2024 11:36

casaroli force-pushed the esp32s3-openeth branch 4 times, most recently from d397407 to bf7c605 Compare October 12, 2024 11:44

casaroli requested a review from fdcavalcanti October 12, 2024 11:44

casaroli added 10 commits October 12, 2024 21:32

feat(esp32_openeth): move to common/espressif

33295c0

Since this will be used for esp32s3 also, we can have this in common/espressif.

feat(esp32s3-devkit): add qemu image option

f6f9444

feat(esp32s3): add openeth ethernet driver for qemu

43ee977

We add the config for esp32s3, then move the esp32 specifics to esp32/chip.h, then add the esp32s3 specifics to esp32s3/chip.h.

feat(esp32s3-devkit): initialize opencores eth mac

d86d18a

feat(esp32s3-devkit): add qemu defconfig

26bcf30

doc(esp32s3): qemu and openeth

26a2088

fix(esp32-qemu): enable legacy esp-idf build

0bc54fb

The current simple boot method is not working with qemu. We enable this option for now until we can make the simple-boot image boot with QEMU.

doc(esp32-qemu): update chip revision and bootloader

33905ef

fix(esp32-devkit): rename defconfig to use _

f621daf

ci: skip config

c607126

since this config requires bootloader, we skip it

casaroli force-pushed the esp32s3-openeth branch from bf7c605 to c607126 Compare October 12, 2024 19:32

xiaoxiang781216 approved these changes Oct 13, 2024

View reviewed changes

xiaoxiang781216 merged commit 83455a3 into apache:master Oct 13, 2024

esp32s3 openeth ethernet driver #13527

esp32s3 openeth ethernet driver #13527

Uh oh!

Conversation

casaroli commented Sep 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Impact for the user

Testing

Commands

esp32s3

esp32

Out of scope

Uh oh!

lupyuen commented Sep 18, 2024

Uh oh!

cederom left a comment

Choose a reason for hiding this comment

Uh oh!

casaroli commented Sep 18, 2024

Uh oh!

cederom commented Sep 18, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xiaoxiang781216 commented Oct 6, 2024

Uh oh!

casaroli commented Oct 6, 2024

Uh oh!

fdcavalcanti commented Oct 7, 2024

Uh oh!

fdcavalcanti Oct 7, 2024

Choose a reason for hiding this comment

Uh oh!

casaroli Oct 12, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fdcavalcanti commented Oct 7, 2024

Uh oh!

casaroli commented Oct 12, 2024

Uh oh!

casaroli commented Oct 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

casaroli commented Sep 18, 2024 •

edited

Loading