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 STM32H5 Ethernet Driver #15583

Merged
merged 1 commit into from
Jan 16, 2025
Merged

Add STM32H5 Ethernet Driver #15583

merged 1 commit into from
Jan 16, 2025

Conversation

kywwilson11
Copy link
Contributor

Summary

STM32H5 Ethernet Driver. This driver is heavily based on the STM32H7 because the Ethernet peripheral on the STM32H5 is essentially identical to that on the STM32H7.

Differences from the STM32H7:

  1. stm32_uid.c - Reading the device electronic signature 8-bits at a time (like the H7) caused an exception. The code was modified to access the device electronic signature 32-bits at a time.
  2. ETH_SEL_PHY - These bits are located in the SBS_PMCR register on the STM32H5. This is different on the STM32H7. This change also required enabling SBS peripheral when Ethernet is configured.

Note: ICACHE and DCACHE not yet implemented on STM32H5.

Impact

STM32H5 Architecture

Testing

Hardware:

  1. Nucleo-H563ZI
  2. USB-to-Ethernet adapter (for test device)

Software:

  1. Ping
  2. Phy Tool (mdio)
  3. ifconfig

Configuration:
Use STM32H7 netnsh config as an example. Added network related options to standard nsh STM32H5 Nucleo-H563ZI config.

Procedure:

  1. Initialize the STM32H5 Ethernet device eth0. Configured IP address of 10.0.0.2. LAN8742 configuration done in menuconfig.
  2. Run ifconfig on eth0.
  3. Run a couple of MDIO commands to ensure proper functionality.
  4. Ping the test device (IP = 10.0.0.5)

Tested at 100Mb/s Half Duplex.

stm32h5_rmii_ethernet_test.txt

copied over files, not modified properly yet

Squashing lots of define errors to get ethernet.c working

quick path rename

Fix STM32_EMAC_BASE (and ICACHE/DCACHE Bases). Enable ETH, ETHTX, and ETHRX with CONFIG_STM32H5_ETHMAC (like H7 does).

Fixed stm32_get_uniqueid

Previously, an exception occurred as a result of calling this function. The problem turned out to be this line: uniqueid[i] = *((uint8_t *)(STM32_SYSMEM_UID) + i);. Dereferencing a 32-bit register as a 8-bit register seemed to be causing this issue. I think this function is better anyway, fewer register accesses and loop iterations.

Turn on SBS peripheral when using Ethernet

The SBS peripheral is required to be turned on because the ETH_SEL_PHY bits need to be set. Without turning this on, RMII mode is not possible. MII may not work either.

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

nuttxpr commented Jan 16, 2025

[Experimental Bot, please feedback here]

Yes, this PR mostly meets the NuttX requirements, but could be improved.

Strengths:

  • Clear Summary: Explains the what and why of the change, referencing the STM32H7 as a basis. The specific differences related to stm32_uid.c and ETH_SEL_PHY are helpful. The note about ICACHE/DCACHE is also relevant.
  • Impact is identified: Correctly highlights the impact is on the STM32H5 architecture.
  • Testing information provided: Includes hardware, software, configuration, and procedure used for testing. The link to the test log is helpful but the logs themselves should be inline if possible (see below).

Weaknesses & Suggestions for Improvement:

  • Summary: Missing Issue References: Even if there isn't a directly related issue, consider creating one to track the addition of STM32H5 Ethernet support. This helps with traceability and allows others to follow the development process.
  • Impact: Needs more detail: While "STM32H5 Architecture" is correct, it's too general. Specifically mention:
    • Impact on user: YES - Users will now be able to use Ethernet on STM32H5 platforms. Briefly describe how to configure it (mentioning the similarity to STM32H7 configuration).
    • Impact on build: YES - New driver code added, build system changes likely required. Briefly describe the changes, e.g., new Kconfig options, board configuration updates.
    • Impact on hardware: YES - Specifies RMII is used. Mention any specific hardware requirements/dependencies.
    • Impact on documentation: YES - Documentation needs to be updated to describe the new driver and its configuration. Confirm whether documentation updates are included in the PR or planned for a separate update.
    • Impact on security, compatibility: Explicitly state NO if there are no impacts.
  • Testing: Inline logs (if reasonable size): While the external link is acceptable, including the relevant snippets of the logs directly in the PR description improves readability and makes it easier for reviewers to assess the changes. If the logs are very long, summarize the key parts inline and keep the full log as an external link. Indicate what the logs demonstrate (e.g., successful ping, MDIO register reads/writes).

By addressing these weaknesses, the PR will be more complete and easier for maintainers to review and merge.

#if defined(CONFIG_STM32H5_HAVE_ETHERNET)
# define STM32H5_NETHERNET 1 /* Ethernet MAC */
#else
# define STM32H5_NETHERNET 0 /* No Ethernet MAC */
Copy link
Contributor

Choose a reason for hiding this comment

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

Please align the 0 to be at same column as the 1

@acassis acassis merged commit b561b44 into apache:master Jan 16, 2025
25 checks passed
@kywwilson11 kywwilson11 deleted the ethernet-driver branch January 17, 2025 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: arm Issues related to ARM (32-bit) architecture Board: arm 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.

4 participants