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

zynq-mpsoc: add support for ethernet #15720

Merged
merged 2 commits into from
Feb 2, 2025
Merged

Conversation

zouboan
Copy link
Contributor

@zouboan zouboan commented Jan 29, 2025

Summary

Zynq MPSOC The processing system (PS) is equipped with four gigabit Ethernet controllers.
Each  controller can be configured independently. Each controller uses a reduced gigabit media
independent interface (RGMII) v2.0. This PR add add support for ethernet of ZYNQ MPSOC
and net support for ZCU111 board.

Impact

Impact on user: YES. Users may need to reconfigure their UART settings if they were relying on a different clock source. Describe what they need to do.
Impact on build: YES, this PR will compaile zynq_enet.c under arch and zcu11_ethernet.c under boards.
Impact on hardware: YES. This impacts the Zynq MPSOC. there's no impaction on other hardware?
Impact on documentation: NO, documentation will finished on other PR.
Impact on security: NO.
Impact on compatibility: NO

Testing

Build Host(s): Linux Ubuntu 22.04, x86_64, Linaro GCC 7.3-2018.04-rc3.
Target(s): Zynq UltraScale+ MPSoC ZCU111 board.
Testing logs:
nsh>                                                                            
nsh> ping 10.0.0.101                                                            
task_spawn: name=ping entry=0x162ae4 file_actions=0x1f4ef0 attr=0x1f4ef8 argv=00
spawn_execattrs: Setting policy=2 priority=100 for pid=3                        
nxtask_activate: ping pid=3,TCB=0x1f58d0                                        
PING 10.0.0.101 56 bytes of data                                                
56 bytes from 10.0.0.101: icmp_seq=0 time=2.0 ms                                
56 bytes from 10.0.0.101: icmp_seq=1 time=1.0 ms                                
56 bytes from 10.0.0.101: icmp_seq=2 time=1.0 ms                                
56 bytes from 10.0.0.101: icmp_seq=3 time=1.0 ms                                
56 bytes from 10.0.0.101: icmp_seq=4 time=1.0 ms                                
56 bytes from 10.0.0.101: icmp_seq=5 time=1.0 ms                                
56 bytes from 10.0.0.101: icmp_seq=6 time=1.0 ms                                
56 bytes from 10.0.0.101: icmp_seq=7 time=1.0 ms                                
56 bytes from 10.0.0.101: icmp_seq=8 time=1.0 ms                                
56 bytes from 10.0.0.101: icmp_seq=9 time=1.0 ms                                
10 packets transmitted, 10 received, 0% packet loss, time 10011 ms              
rtt min/avg/max/mdev = 1.000/1.100/2.000/0.300 ms                               
nxtask_exit: ping pid=3,TCB=0x1f58d0                                            
nsh>   

@github-actions github-actions bot added Arch: arm64 Issues related to ARM64 (64-bit) architecture Board: arm64 Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces. labels Jan 29, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 29, 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.

The PR summary is okay, but could be improved by mentioning the specific Ethernet controller used (e.g., "Adds support for the Xilinx GEM Ethernet controller on the Zynq MPSOC").

The impact section needs significant improvement. Here's a breakdown:

  • Impact on user: You state "Users may need to reconfigure their UART settings..." This is completely unrelated to Ethernet and suggests a problem with the PR. Remove this. The actual user impact is that they can now use the Ethernet interface on the ZCU111 board. Be specific: "Users can now configure and use the Ethernet interface on the ZCU111 board. This includes assigning IP addresses, using network applications like ping, etc."
  • Impact on build: More detail is needed. Mentioning which files are compiled is good, but also specify where they are located (e.g., "Adds zynq_enet.c to arch/arm/src/zynqmp and zcu11_ethernet.c to boards/arm/zynqmp/zcu111/src"). If there are Kconfig changes, mention them here.
  • Impact on hardware: Specify that it only impacts Zynq MPSOC and explicitly mention the ZCU111. "This impacts the Zynq UltraScale+ MPSoC, specifically enabling Ethernet functionality on the ZCU111 board. Other hardware is unaffected."
  • Impact on documentation: While you intend to add documentation later, it's best to include at least basic usage instructions in the initial PR. If that's absolutely impossible, then acknowledge that documentation is missing and will be addressed in a follow-up PR. "Documentation is incomplete and will be provided in a separate PR. However, the driver config option and its usage is..."
  • Impact on security: You say "NO" but provide no justification. Briefly explain why there are no security implications (e.g., "No new network services are introduced, and existing network security measures remain applicable.")
  • Impact on compatibility: Justify the "NO" (e.g., "This adds new functionality and does not break existing functionality.")

The testing section is good, showing successful ping tests. However, more comprehensive testing is recommended. Consider including:

  • Basic connectivity: Pinging a gateway or another device on the network.
  • Throughput tests: iperf or similar tools.
  • Different network configurations: Static IP, DHCP.
  • Error handling: Testing behavior with unplugged cables, invalid IP addresses, etc.

Revised Impact Section Example:

Impact on user: YES. Users can now configure and use the Ethernet interface on the ZCU111 board. This includes assigning IP addresses and using network applications.
Impact on build: YES. Adds `zynq_enet.c` to `arch/arm/src/zynqmp` and `zcu11_ethernet.c` to `boards/arm/zynqmp/zcu111/src`.  Adds Kconfig option `CONFIG_ZYNQMP_GEM` (describe usage).
Impact on hardware: YES. This enables the Ethernet functionality on the Zynq UltraScale+ MPSoC, specifically on the ZCU111 board. Other hardware is unaffected.
Impact on documentation: NO. Documentation will be provided in a separate PR.  [Optional: Briefly describe how to enable the driver in menuconfig or via defconfig].
Impact on security: NO. This change does not introduce new network services and does not alter existing network security mechanisms.
Impact on compatibility: NO.  This adds new functionality and does not modify existing behavior.

By addressing these points, you'll create a much stronger and more informative PR. Reviewers will appreciate the clarity and completeness, leading to a faster and smoother review process.

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 commit log messages.

arch/arm64/src/zynq-mpsoc/Kconfig Outdated Show resolved Hide resolved
arch/arm64/src/zynq-mpsoc/Kconfig Outdated Show resolved Hide resolved
arch/arm64/src/zynq-mpsoc/Kconfig Outdated Show resolved Hide resolved
arch/arm64/src/zynq-mpsoc/Kconfig Outdated Show resolved Hide resolved
arch/arm64/src/zynq-mpsoc/zynq_enet.h Outdated Show resolved Hide resolved

choice
prompt "ZYNQ Ethernet Interface"
default ZYNQ_ENET4
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does ENET4 is default instead of ENET1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because GMAC3 belong to PS of ZYNQ MPSOC, GMAC0~GMAC2 may depends on PL of ZYNQ MPSOC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whats more, ZCU111 board use GMAC3


config ZYNQ_GMAC_NRXBUFFERS
int "Number of RX buffers"
default 16
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a upper limit for those 128 bytes blocks? If so, please include a range i.e. 1 to 32

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done! thanks for your review


config ZYNQ_GMAC_NTXBUFFERS
int "Number of TX buffers"
default 8
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Zynq MPSOC The processing system (PS) is equipped with four gigabit
Ethernet controllers.Each  controller can be configured independently.
Each controller uses a reduced gigabit media independent interface (RGMII)
v2.0. This commit add support for ethernet of ZYNQ MPSOC.
The PS-side Gigabit Ethernet MAC (GEM) implements a 10/100/1000 Mb/s
Ethernet interface, which connects to a TI DP83867IRPAP Ethernet
RGMII PHY before being routed to an RJ45 Ethernet connector on ZCU111
board.The RGMII Ethernet PHY is boot strapped to PHY address 5'b01100
(0x0C) and Auto Negotiation is set to Enable.This commit add Ethernet
support for ZCU111 board and TI DP83867IRPAP Ethernet RGMII PHY.
@zouboan
Copy link
Contributor Author

zouboan commented Jan 30, 2025

Please add commit log messages.

done!

@xiaoxiang781216 xiaoxiang781216 merged commit d36b2f9 into apache:master Feb 2, 2025
12 checks passed
@zouboan zouboan deleted the fft branch February 3, 2025 10:37
zouboan added a commit to zouboan/nuttx that referenced this pull request Feb 5, 2025
1. Changes the phyadd to 0xC to speed up the training of phyadd.
2. Set RX DMA buffer size configureable.
3. Create netnsh configs as ethernet boot from QSPI FLASH.
4. Fix some typo in apache#15720 which is nonsynchronous with local code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: arm64 Issues related to ARM64 (64-bit) architecture Board: arm64 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.

5 participants