h7: eth: add checks for PHYID in stm32_phyinit

[dakejahl]

Summary

Check PHYID1 and PHYID2 for the configured PHY during init. Drivers on some of the other targets do this.

Impact

Fixes bug where stm32_phyinit will succeed even when no PHY is connected. This is because there is no check that a PHY is actually communicating and returning data.

Testing

Tested on an ARKV6X running PX4 main. This uses an STM32H743IIK processor.

Here is a snippet from the driver with debugging enabled

tm32_ifup: Bringing up: 192.168.0.4
stm32_ethconfig: Reset the Ethernet block
stm32_ethconfig: Initialize the PHY
stm32_phyinit: MII_MCR: phyval: 0 ret: 0
stm32_phyinit: Phy reset in 10 ms
stm32_phyinit: ERROR: Incorrect PHYID1: 0 expected: 7
netinit_monitor: Entry
netdev_ifr_ioctl: cmd: 1819
netdev_ifr_ioctl: cmd: 1828
netdev_ifr_ioctl: cmd: 1829
netinit_monitor: eth0: devup=0 PHY address=00 MSR=ffff
netinit_monitor: Bringing the link up
netdev_ifr_ioctl: cmd: 1818
stm32_ifup: Bringing up: 192.168.0.4
stm32_ethconfig: Reset the Ethernet block
stm32_ethconfig: Initialize the PHY
stm32_phyinit: MII_MCR: phyval: 0 ret: 0
stm32_phyinit: Phy reset in 10 ms
stm32_phyinit: ERROR: Incorrect PHYID1: 0 expected: 7
netinit_monitor: ERROR: ioctl(SIOCSIFFLAGS) failed: -6
netinit_monitor: ERROR: Aborting
netinit_thread: Exit

Before this PR it would succeed

stm32_ifup: Bringing up: 192.168.0.4
stm32_ethconfig: Reset the Ethernet block
stm32_ethconfig: Initialize the PHY
stm32_phyinit: Phy reset in 10 ms
stm32_phyinit: PHYSR[31]: ffff
stm32_phyinit: Duplex: FULL Speed: 100 MBps
stm32_ethconfig: Initialize the MAC and DMA
stm32_ethconfig: Enable normal operation
stm32_macaddress: eth0 MAC: ce:f1:92:f9:9c:2b

[nuttxpr]

[Experimental Bot, please feedback here]

The provided PR description does not fully meet the NuttX requirements. Here's a breakdown and how it could be improved:

Missing/Insufficient Information:

• Summary:

  • Lacks clarity on why the existing behavior was problematic. Explain the consequences of not checking PHYID1/2. (e.g., Network initialization fails silently, leading to difficult debugging, application errors, etc.)
  • Doesn't specify the exact files changed. (e.g., drivers/net/ethernet/stm32_eth.c)

• Impact:

  • Needs to explicitly answer all the "Impact on..." questions with NO/YES and explanations where applicable. The current description only covers the user impact. Pay particular attention to:
    • Impact on build: Likely NO, but confirm.
    • Impact on hardware: Specify the affected architecture (STM32H7). Mentioning the specific board (ARKV6X) is helpful but the focus should be on the MCU.
    • Impact on documentation: If no documentation changes are needed, explicitly state "NO". If the bug fix warrants a documentation update (even a minor one), consider adding it.
    • Impact on security: Likely NO, but confirm.
    • Impact on compatibility: Important! Does this change break any existing configurations or introduce any compatibility concerns with older versions of NuttX or specific PHY chips? If no, explicitly state so and explain why.

• Testing:

  • Build Host(s): Missing operating system, compiler version, etc. Be specific. Example: Linux, Ubuntu 22.04, GCC 11.3.0
  • Target(s): Needs more detail than just "ARKV6X". Specify STM32H7, ARKV6X or similar. Crucially, mention the NuttX configuration used (e.g., nsh).
  • Testing logs: The provided logs only show the after state (the fix working and preventing initialization). You must provide logs demonstrating the before behavior (the original bug where initialization proceeds despite the missing PHY). This shows that the change actually fixes the identified problem.

Example of an Improved PR Description:

Summary

This PR fixes a bug in the STM32 Ethernet driver where network initialization would succeed even if no PHY was connected. This was caused by the driver not verifying the PHY ID registers (PHYID1 and PHYID2) during initialization. The absence of this check meant that the driver could erroneously report a successful initialization, leading to application errors and difficult debugging. This change adds the missing PHY ID checks to drivers/net/ethernet/stm32_eth.c, ensuring that initialization fails if no valid PHY is detected. Fixes #[NuttX Issue Number, if applicable].

Impact

• Is new feature added? NO
• Is existing feature changed? YES, the STM32 ethernet driver now correctly detects the absence of a PHY.
• Impact on user: YES. Users will no longer experience silent failures during network initialization when no PHY is present. Error messages will now be logged, aiding debugging.
• Impact on build: NO
• Impact on hardware: YES. This change specifically affects the STM32H7 architecture (e.g., ARKV6X board).
• Impact on documentation: NO.
• Impact on security: NO.
• Impact on compatibility: NO. This fix corrects an error condition and does not introduce any backward compatibility issues. It is expected to work with a wider range of PHY chips than the previous, unverified behavior.
• Anything else to consider: None.

Testing

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

• Build Host(s): Linux, Ubuntu 22.04, GCC 11.3.0
• Target(s): STM32H7, ARKV6X, nsh configuration

Testing logs before change (without PHY connected):

[SHOW LOGS OF FAILED INITIALIZATION *WITHOUT* THE FIX]
(e.g., showing initialization succeeding despite no PHY)

Testing logs after change (without PHY connected):

[PASTE THE PROVIDED LOGS SHOWING THE CORRECT ERROR REPORTING]

Testing logs after change (with PHY connected):

[SHOW LOGS OF SUCCESSFUL INITIALIZATION WITH THE FIX APPLIED AND A PHY CONNECTED]
This demonstrates that the fix does not break normal functionality.

By providing comprehensive information, you significantly increase the chances of your PR being reviewed and merged quickly. Reviewers will appreciate the clarity and completeness, making their job much easier. Remember to replace the bracketed placeholders with the actual information.

[dakejahl]

The AI review is pretty cool 😎

[lupyuen]

I believe this commit will need a proper description? fbdcafc

As discussed in NuttX Mailing List today. Thanks :-) https://lists.apache.org/thread/mn4l1tmr6fj46o2y9vvrmfcrgyo48s5d

[jerpelea]

Please move the include change to a separate PR. If we intend to maintain LTS releases we MUST split the PR on separate areas

[dakejahl]

Please move the include change to a separate PR. If we intend to maintain LTS releases we MUST split the PR on separate areas

Create a separate PR for changing mii.h? The changes in stm32_ethernet.c use this definition, the PR wouldn't build.

[dakejahl]

I believe this commit will need a proper description? fbdcafc

As discussed in NuttX Mailing List today. Thanks :-) https://lists.apache.org/thread/mn4l1tmr6fj46o2y9vvrmfcrgyo48s5d

squashed!