You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "casaroli (via GitHub)" <gi...@apache.org> on 2024/09/18 15:41:59 UTC
[PR] esp32s3 openeth ethernet driver [nuttx]
casaroli opened a new pull request, #13527:
URL: https://github.com/apache/nuttx/pull/13527
## Summary
Espressif recently added support for esp32s3 on their qemu, so we migrate the driver to xtensa common and add it to the esp32s3-devkit config.
## Impact
Now we can run the esp32s3 port in qemu.
## Testing
Manual testing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] esp32s3 openeth ethernet driver [nuttx]
Posted by "casaroli (via GitHub)" <gi...@apache.org>.
casaroli commented on code in PR #13527:
URL: https://github.com/apache/nuttx/pull/13527#discussion_r1766081370
##########
boards/xtensa/esp32/esp32-devkitc/src/esp32-devkitc.h:
##########
@@ -245,7 +245,7 @@ int board_ws2812_initialize(
*
****************************************************************************/
#ifdef CONFIG_ESP32_OPENETH
Review Comment:
I will put header files in the chip specific directory and include them in the common driver. WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] esp32s3 openeth ethernet driver [nuttx]
Posted by "casaroli (via GitHub)" <gi...@apache.org>.
casaroli commented on PR #13527:
URL: https://github.com/apache/nuttx/pull/13527#issuecomment-2359331536
Can anyone help me understand why the workflow is failing? What should I do to fix it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] esp32s3 openeth ethernet driver [nuttx]
Posted by "casaroli (via GitHub)" <gi...@apache.org>.
casaroli commented on code in PR #13527:
URL: https://github.com/apache/nuttx/pull/13527#discussion_r1797688572
##########
boards/xtensa/esp32/esp32-devkitc/src/esp32-devkitc.h:
##########
@@ -245,7 +245,7 @@ int board_ws2812_initialize(
*
****************************************************************************/
#ifdef CONFIG_ESP32_OPENETH
-int esp32_openeth_initialize(void);
+int xtensa_openeth_initialize(void);
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] esp32s3 openeth ethernet driver [nuttx]
Posted by "casaroli (via GitHub)" <gi...@apache.org>.
casaroli commented on code in PR #13527:
URL: https://github.com/apache/nuttx/pull/13527#discussion_r1797688629
##########
boards/xtensa/esp32s3/esp32s3-devkit/configs/qemu-openeth/defconfig:
##########
Review Comment:
done
##########
Documentation/platforms/xtensa/esp32s3/index.rst:
##########
@@ -317,6 +317,31 @@ possible to track the root cause of the crash. Saving this output to a file and
The above output shows the backtrace of the tasks. By checking it, it is possible to track the
functions that were being executed when the crash occurred.
+Using QEMU
+==========
+
+Get or build QEMU from `here <https://github.com/espressif/qemu/wiki>`__.
+
+Enable the ``ESP32S3_QEMU_IMAGE`` config found in :menuselection:`Board Selection --> ESP32S3 binary image for QEMU`.
+
+Enable ``ESP32_APP_FORMAT_LEGACY``.
+
+Build and generate the QEMU image::
+
+ $ make bootloader
+ $ make ESPTOOL_BINDIR=.
+
+A QEMU-compatible ``nuttx.merged.bin`` binary image will be created. It can be run as::
+
+ $ qemu-system-xtensa -nographic -machine esp32s3 -drive file=nuttx.merged.bin,if=mtd,format=raw
+
+QEMU Networking
+---------------
+
+Networking is possible using the openeth MAC driver. Enable ``ESP32S3_OPENETH`` option and set the nic in QEMU:
+
+ $ qemu-system-xtensa -nographic -machine esp32 -drive file=nuttx.merged.bin,if=mtd,format=raw -nic user,model=open_eth
Review Comment:
done
##########
Documentation/platforms/xtensa/esp32s3/index.rst:
##########
@@ -317,6 +317,31 @@ possible to track the root cause of the crash. Saving this output to a file and
The above output shows the backtrace of the tasks. By checking it, it is possible to track the
functions that were being executed when the crash occurred.
+Using QEMU
+==========
+
+Get or build QEMU from `here <https://github.com/espressif/qemu/wiki>`__.
+
+Enable the ``ESP32S3_QEMU_IMAGE`` config found in :menuselection:`Board Selection --> ESP32S3 binary image for QEMU`.
+
+Enable ``ESP32_APP_FORMAT_LEGACY``.
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] esp32s3 openeth ethernet driver [nuttx]
Posted by "cederom (via GitHub)" <gi...@apache.org>.
cederom commented on PR #13527:
URL: https://github.com/apache/nuttx/pull/13527#issuecomment-2359498960
> 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 :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] esp32s3 openeth ethernet driver [nuttx]
Posted by "casaroli (via GitHub)" <gi...@apache.org>.
casaroli commented on code in PR #13527:
URL: https://github.com/apache/nuttx/pull/13527#discussion_r1766080015
##########
arch/xtensa/src/common/xtensa_openeth.c:
##########
@@ -1,5 +1,5 @@
/****************************************************************************
- * arch/xtensa/src/esp32/esp32_openeth.c
+ * arch/xtensa/src/common/xtensa_openeth.c
Review Comment:
No. AFAIK this is only for esp32 family on qemu.
Another chip with a real openeth peripheral will require a more complete driver and features that are not available in the openeth qemu implementation, which has many parts mocked.
Maybe this can be used by other qemu like arm or riscv, but I have not tested as in those cases, network can usually be done with virtio or some other better interface (than mocking openeth nic).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] esp32s3 openeth ethernet driver [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #13527:
URL: https://github.com/apache/nuttx/pull/13527#discussion_r1765958009
##########
arch/xtensa/src/common/xtensa_openeth.c:
##########
@@ -1,5 +1,5 @@
/****************************************************************************
- * arch/xtensa/src/esp32/esp32_openeth.c
+ * arch/xtensa/src/common/xtensa_openeth.c
Review Comment:
can this driver be used by other non esp chip?
##########
boards/xtensa/esp32/esp32-devkitc/src/esp32-devkitc.h:
##########
@@ -245,7 +245,7 @@ int board_ws2812_initialize(
*
****************************************************************************/
#ifdef CONFIG_ESP32_OPENETH
Review Comment:
shouldn't contain chip specific macro in common.folder
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] esp32s3 openeth ethernet driver [nuttx]
Posted by "casaroli (via GitHub)" <gi...@apache.org>.
casaroli commented on PR #13527:
URL: https://github.com/apache/nuttx/pull/13527#issuecomment-2408534164
I believe I have addressed all the concerns. Can you please provide another review?
@xiaoxiang781216 @fdcavalcanti
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] esp32s3 openeth ethernet driver [nuttx]
Posted by "casaroli (via GitHub)" <gi...@apache.org>.
casaroli commented on PR #13527:
URL: https://github.com/apache/nuttx/pull/13527#issuecomment-2408535127
> 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 :sweat_smile: :sweat_smile:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] esp32s3 openeth ethernet driver [nuttx]
Posted by "casaroli (via GitHub)" <gi...@apache.org>.
casaroli commented on code in PR #13527:
URL: https://github.com/apache/nuttx/pull/13527#discussion_r1766080015
##########
arch/xtensa/src/common/xtensa_openeth.c:
##########
@@ -1,5 +1,5 @@
/****************************************************************************
- * arch/xtensa/src/esp32/esp32_openeth.c
+ * arch/xtensa/src/common/xtensa_openeth.c
Review Comment:
No. AFAIK this is only for esp32 family on qemu.
Another chip with a real openeth peripheral will include a more complete driver and features that are not available in the openeth qemu implementation, which has many parts mocked.
Maybe this can be used by other qemu like arm or riscv, but I have not tested as in those cases, network can usually be done with virtio or some other better interface (than mocking openeth nic).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] esp32s3 openeth ethernet driver [nuttx]
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #13527:
URL: https://github.com/apache/nuttx/pull/13527#discussion_r1765902541
##########
arch/xtensa/src/common/xtensa_openeth.c:
##########
@@ -30,20 +30,40 @@
#include <netinet/if_ether.h>
#include <nuttx/net/netdev_lowerhalf.h>
+#if defined(CONFIG_ESP32_OPENETH)
#include "hardware/esp32_soc.h"
#include "esp32_irq.h"
-
-#ifdef CONFIG_ESP32_OPENETH
+#elif defined(CONFIG_ESP32S3_OPENETH)
+#include "hardware/esp32s3_soc.h"
+#include "esp32s3_irq.h"
+#endif
/****************************************************************************
* Pre-processor Definitions
****************************************************************************/
+#if defined(CONFIG_ESP32_OPENETH)
+#define OPENETH_PERIPH_MAC ESP32_PERIPH_EMAC
+#define OPENETH_CPUINT_LEVEL ESP32_CPUINT_LEVEL
+#define OPENETH_IRQ_MAC ESP32_IRQ_EMAC
+#define OPENETH_SETUP_IRQ esp32_setup_irq
+#elif defined(CONFIG_ESP32S3_OPENETH)
+#define OPENETH_PERIPH_MAC ESP32S3_PERIPH_MAC
+#define OPENETH_CPUINT_LEVEL ESP32S3_CPUINT_LEVEL
+#define OPENETH_IRQ_MAC ESP32S3_IRQ_MAC
+#define OPENETH_SETUP_IRQ esp32s3_setup_irq
Review Comment:
```suggestion
#define OPENETH_PERIPH_MAC ESP32S3_PERIPH_MAC
#define OPENETH_CPUINT_LEVEL ESP32S3_CPUINT_LEVEL
#define OPENETH_IRQ_MAC ESP32S3_IRQ_MAC
#define OPENETH_SETUP_IRQ esp32s3_setup_irq
```
##########
arch/xtensa/src/common/xtensa_openeth.c:
##########
@@ -30,20 +30,40 @@
#include <netinet/if_ether.h>
#include <nuttx/net/netdev_lowerhalf.h>
+#if defined(CONFIG_ESP32_OPENETH)
#include "hardware/esp32_soc.h"
#include "esp32_irq.h"
-
-#ifdef CONFIG_ESP32_OPENETH
+#elif defined(CONFIG_ESP32S3_OPENETH)
+#include "hardware/esp32s3_soc.h"
+#include "esp32s3_irq.h"
+#endif
/****************************************************************************
* Pre-processor Definitions
****************************************************************************/
+#if defined(CONFIG_ESP32_OPENETH)
+#define OPENETH_PERIPH_MAC ESP32_PERIPH_EMAC
+#define OPENETH_CPUINT_LEVEL ESP32_CPUINT_LEVEL
+#define OPENETH_IRQ_MAC ESP32_IRQ_EMAC
+#define OPENETH_SETUP_IRQ esp32_setup_irq
Review Comment:
```suggestion
#define OPENETH_PERIPH_MAC ESP32_PERIPH_EMAC
#define OPENETH_CPUINT_LEVEL ESP32_CPUINT_LEVEL
#define OPENETH_IRQ_MAC ESP32_IRQ_EMAC
#define OPENETH_SETUP_IRQ esp32_setup_irq
```
##########
arch/xtensa/src/common/xtensa_openeth.c:
##########
@@ -512,8 +532,8 @@ int esp32_openeth_initialize(void)
/* Setup interrupts */
- priv->cpuint = esp32_setup_irq(0, ESP32_PERIPH_EMAC,
- 1, ESP32_CPUINT_LEVEL);
+ priv->cpuint = OPENETH_SETUP_IRQ(0, OPENETH_PERIPH_MAC,
+ 1, OPENETH_CPUINT_LEVEL);
Review Comment:
```suggestion
priv->cpuint = OPENETH_SETUP_IRQ(0, OPENETH_PERIPH_MAC,
1, OPENETH_CPUINT_LEVEL);
```
```suggestion
priv->cpuint = OPENETH_SETUP_IRQ(0, OPENETH_PERIPH_MAC,
1, OPENETH_CPUINT_LEVEL);
```
```suggestion
priv->cpuint = OPENETH_SETUP_IRQ(0, OPENETH_PERIPH_MAC,
1, OPENETH_CPUINT_LEVEL);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] esp32s3 openeth ethernet driver [nuttx]
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #13527:
URL: https://github.com/apache/nuttx/pull/13527#discussion_r1765901909
##########
arch/xtensa/src/common/xtensa_openeth.c:
##########
@@ -512,8 +532,8 @@ int esp32_openeth_initialize(void)
/* Setup interrupts */
- priv->cpuint = esp32_setup_irq(0, ESP32_PERIPH_EMAC,
- 1, ESP32_CPUINT_LEVEL);
+ priv->cpuint = OPENETH_SETUP_IRQ(0, OPENETH_PERIPH_MAC,
+ 1, OPENETH_CPUINT_LEVEL);
Review Comment:
```suggestion
priv->cpuint = OPENETH_SETUP_IRQ(0, OPENETH_PERIPH_MAC,
1, OPENETH_CPUINT_LEVEL);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] esp32s3 openeth ethernet driver [nuttx]
Posted by "casaroli (via GitHub)" <gi...@apache.org>.
casaroli commented on code in PR #13527:
URL: https://github.com/apache/nuttx/pull/13527#discussion_r1797685161
##########
arch/xtensa/src/common/xtensa_openeth.c:
##########
@@ -1,5 +1,5 @@
/****************************************************************************
- * arch/xtensa/src/esp32/esp32_openeth.c
+ * arch/xtensa/src/common/xtensa_openeth.c
Review Comment:
done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] esp32s3 openeth ethernet driver [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 merged PR #13527:
URL: https://github.com/apache/nuttx/pull/13527
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] esp32s3 openeth ethernet driver [nuttx]
Posted by "casaroli (via GitHub)" <gi...@apache.org>.
casaroli commented on code in PR #13527:
URL: https://github.com/apache/nuttx/pull/13527#discussion_r1768092457
##########
boards/xtensa/esp32/esp32-devkitc/src/esp32-devkitc.h:
##########
@@ -245,7 +245,7 @@ int board_ws2812_initialize(
*
****************************************************************************/
#ifdef CONFIG_ESP32_OPENETH
Review Comment:
Hello, @xiaoxiang781216 can you please suggest me how to do this?
I thought about moving those definitions to the chip specific `chip.h` and including it from the driver. WDYT?
Or should I create another file like `openeth.h` in each chips directory?
Do you have any other idea? How should I proceed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] esp32s3 openeth ethernet driver [nuttx]
Posted by "lupyuen (via GitHub)" <gi...@apache.org>.
lupyuen commented on PR #13527:
URL: https://github.com/apache/nuttx/pull/13527#issuecomment-2358851293
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13494&type=pullrequests)
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org